fix(v0.1.10): security hardening — close the six surviving 2026-06 review findings by db-tycoon-stephen · Pull Request #153 · Database-Tycoon/tycoon-cli

db-tycoon-stephen · 2026-07-16T00:00:20Z

Summary

Implements the six security-review findings that survived the refactor triage (#62, #64–#68). One commit per stream, plus a review-fixes commit.

tycoon.yml as untrusted input (#62, #65)

${VAR} expansion limited to connection/credential fields — no more env-secret exfiltration through arbitrary fields
Strict identifier validation on source keys, schema/table names, and source type
dbt_project_dir/rill_dir must resolve near the project (sibling layouts still work; /etc-style escapes fail)

Runtime hardening (#66, #67)

Quack token file written 0600, tightened on load, token shape validated before reaching SQL
schedule rejects control chars; systemd ExecStart is fully quoted/escaped ($, %, quotes, backslashes)

Supply chain (#64, #68)

All actions SHA-pinned, permissions: floors, Dependabot
gitleaks (CI + pre-commit) and bandit gates
Runtime dlt[extra] installs pinned; SECURITY.md documents the dlt-init trust boundary

All four Gemini findings addressed (see review thread). Note for ship time: the release PR to main needs Closes #62 #64 #65 #66 #67 #68.

Tests

680 passed (69 new) / ruff clean / pre-commit clean (gitleaks + bandit) / workflow YAML validated

🤖 Generated with Claude Code

…date tycoon.yml identifiers and paths (#62, #65) - Env-var expansion now applies only to connection/credential fields (database paths, sources.*.config subtree, Fivetran api_key/secret, sync endpoints) instead of the whole config — a shared tycoon.yml can no longer exfiltrate arbitrary env secrets via ${VAR} in innocuous fields. ${VAR:-default} semantics unchanged where allowed. - Source keys, schema_name, and table names get strict identifier validation (source keys additionally allow '-', which the shipped templates use and which is inert in both SQL and path contexts). - dbt_project_dir/rill_dir reject control chars and must resolve within the project root's parent — sibling layouts (the wizard default) stay first-class while /etc-style escapes fail loudly. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

…nit escaping (#66, #67) - .tycoon/secrets.yml is created 0600 and pre-existing loose-perms files are tightened on load; tokens failing ^[A-Za-z0-9_-]+$ are rejected before they can reach DuckDB SQL literals. - schedule add() rejects newline/CR/NUL in command args and project root before rendering; systemd ExecStart uses quoted-argv form (WorkingDirectory stays raw — systemd does no quote removal on single-path settings — protected by the control-char rejection). launchd path unchanged (plistlib already escapes). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

… gates, pin runtime dlt, SECURITY.md (#64, #68) - Every third-party action pinned to a full commit SHA with version comment; pypa/gh-action-pypi-publish moves off the mutable release/v1 branch ref onto the v1.14.0 release SHA. permissions: floors added (contents: read on ci/e2e/nightly, {} on publish with job-level grants). .github/dependabot.yml covers actions + pip. - gitleaks (binary, sha256-verified) runs as a CI job over full history and as a pre-commit hook; bandit gates src/ (subprocess/ assert checks skipped by design — this CLI shells out on purpose; hardcoded-creds, shell=True, weak-crypto, unsafe-YAML all active). - Runtime dlt[extra] installs pinned to the installed dlt version; dlt init call site now states what it downloads and from where. - SECURITY.md documents reporting + the deliberate dlt-init runtime- download trust boundary. - Snapshot dir excluded from whitespace hooks (they strip Rich's trailing padding and break every snapshot assertion); repo-wide hygiene auto-fixes from the new hooks' first run included. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

db-tycoon-stephen · 2026-07-16T00:00:36Z

/gemini review

gemini-code-assist

Code Review

This pull request introduces several security hardening measures, including restricting environment variable interpolation to an allowlist, validating source, schema, and table names as safe identifiers, enforcing path containment for project directories, tightening secrets file permissions, and escaping systemd arguments. The review feedback highlights a potential path traversal bypass when the project is in a root subdirectory, recommends validating the type field in SourceConfig, and suggests escaping literal $ characters in systemd service files to prevent unintended environment variable substitution.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

gemini-code-assist

Code Review

This pull request introduces several security hardening measures, including path containment checks for project directories, restricted environment variable interpolation, strict identifier validation for sources, schemas, and tables, owner-only permissions for secrets, and argument escaping for systemd services. The review feedback highlights critical improvements to these security features: addressing a potential path containment bypass when running in top-level directories (e.g., in Docker), validating the 'type' field in 'SourceConfig' to prevent path traversal, handling 'OSError' during file permission adjustments on non-POSIX filesystems, and escaping '%' and '$' characters in systemd service arguments to prevent unintended expansion.

Important

The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.

…ype validation, systemd $/% escaping, best-effort perms - Path containment: a project in a top-level dir (/app) made the parent boundary the filesystem root, which contains everything — fall back to root-scoped containment there. (Gemini, valid HIGH) - SourceConfig.type now validated with the strict identifier charset: it reaches dlt init argv and filesystem paths. - _systemd_quote doubles $ and % so env-var/specifier expansion can't rewrite argument content. - ensure_token perms are best-effort on restricted mounts, matching _tighten_permissions semantics. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

db-tycoon-stephen · 2026-07-16T00:23:47Z

All four review findings addressed in the latest commit (Gemini posted two overlapping reviews — deduped to four distinct issues):

Containment bypass for top-level project roots (HIGH — valid catch): _resolve_contained_path now falls back to root-scoped containment when the parent boundary is the filesystem root (/app-style Docker/CI layouts). Sibling layouts under normal roots unchanged. Tests: TestTopLevelProjectBoundary.
SourceConfig.type unvalidated: now gated by the strict identifier regex (_IDENTIFIER_RE, no hyphens — dlt source types are Python module names). Tests: TestSourceTypeValidation.
systemd $/% expansion: _systemd_quote doubles both so argument content round-trips verbatim. Tests added to TestSystemdQuoting.
ensure_token chmod on restricted mounts: wrapped in best-effort except OSError, matching _tighten_permissions semantics.

Full suite: 680 passed, 2 skipped. ruff clean.

JesuFemi-O

The allowlist scoping on ${VAR} expansion, path containment with the root-boundary fallback, quack token validation, systemd quoting, and the gitleaks SHA-verification approach are all solid. Approving.

One thing worth tracking as a follow-up: load_token() now raises QuackTokenError where it previously returned None. The call path is _preflight_checks → quack.ensure_token() → load_token(), so a tampered secrets.yml will surface as an unhandled exception traceback rather than a clean CLI error in tycoon start. The error message itself is actionable ("delete the 'quack' entry and rerun"), but the presentation isn't. Small quality-of-life item — catch QuackTokenError in _preflight_checks and hand it to error() before raising typer.Exit(1).

db-tycoon-stephen · 2026-07-17T00:18:11Z

Follow-up from the review is tracked as #157 (catch QuackTokenError in _preflight_checks and present it via error() + typer.Exit(1) instead of a traceback). Keeping it out of this PR since it's presentation-only and the review approved as-is.

Conflict in src/tycoon/config.py: keep _resolve_contained_path (this branch's #65 containment fix), drop the nao_dir property removed by the v0.1.10 extras deletion (PR #152). Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>

db-tycoon-stephen and others added 3 commits July 15, 2026 19:59

This was referenced Jul 16, 2026

test: drop snapshot tests for removed nao/ask message strings #154

Open

ci: pre-commit check-yaml fails on mkdocs.yml python-specific tag #155

Open

gemini-code-assist Bot reviewed Jul 16, 2026

View reviewed changes

Comment thread src/tycoon/config.py

Comment thread src/tycoon/project.py

Comment thread src/tycoon/schedule.py

gemini-code-assist Bot reviewed Jul 16, 2026

View reviewed changes

Comment thread src/tycoon/config.py

Comment thread src/tycoon/project.py

Comment thread src/tycoon/quack.py Outdated

Comment thread src/tycoon/schedule.py Outdated

db-tycoon-stephen requested a review from JesuFemi-O July 16, 2026 00:43

JesuFemi-O approved these changes Jul 16, 2026

View reviewed changes

db-tycoon-stephen mentioned this pull request Jul 17, 2026

tycoon start: catch QuackTokenError in _preflight_checks for a clean CLI error #157

Open

Database-Tycoon deleted a comment from sciortino Jul 17, 2026

db-tycoon-stephen merged commit 0de7ef9 into v0.1.10 Jul 17, 2026
8 checks passed

db-tycoon-stephen deleted the fix/v0.1.10-security-hardening branch July 17, 2026 10:57

db-tycoon-stephen mentioned this pull request Jul 21, 2026

Release v0.1.10 — metadata backend + a slimmer install #159

Merged

Navigation Menu

Uh oh!

fix(v0.1.10): security hardening — close the six surviving 2026-06 review findings - #153

fix(v0.1.10): security hardening — close the six surviving 2026-06 review findings#153
db-tycoon-stephen merged 5 commits into
v0.1.10from
fix/v0.1.10-security-hardening

db-tycoon-stephen commented Jul 16, 2026 •

edited

Loading

Uh oh!

db-tycoon-stephen commented Jul 16, 2026

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gemini-code-assist Bot left a comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

db-tycoon-stephen commented Jul 16, 2026

Uh oh!

JesuFemi-O left a comment

Uh oh!

db-tycoon-stephen commented Jul 17, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

db-tycoon-stephen commented Jul 16, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Summary

Tests

Uh oh!

db-tycoon-stephen commented Jul 16, 2026

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

gemini-code-assist Bot left a comment

Choose a reason for hiding this comment

Code Review

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

db-tycoon-stephen commented Jul 16, 2026

Uh oh!

JesuFemi-O left a comment

Choose a reason for hiding this comment

Uh oh!

db-tycoon-stephen commented Jul 17, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

db-tycoon-stephen commented Jul 16, 2026 •

edited

Loading