fix(sync): block path traversal in storage-metadata write (GHSA-833q, 0.60.3)

[padak]

Summary

Fixes M2 from the 2026-06-12 security audit (private advisory GHSA-833q-c5wv-26r7) — a path-traversal arbitrary-file-write in kbagent sync pull's storage-metadata writer.

_write_storage_metadata (services/sync_service.py) built on-disk paths from the API response with no real sanitization: the table name was used verbatim as the JSON filename, and bucket_id.replace('.', '-') neutralizes .. but not / or an absolute path. A malicious or compromised Storage API response (in the audit threat model) returning a table named ../../../../etc/cron.d/evil could write attacker-controlled JSON outside the sync workspace. Storage metadata is written on a normal sync pull (on by default).

The config-write path in the same module already defends this exact class (sanitize_path_segment + _ensure_within_branch, issue #269); the storage-metadata + samples writers skipped both layers. This PR makes them mirror it.

Fix

• sanitize_path_segment(...) on the bucket id and table name (tables + samples writers).
  • Behavior-preserving: for the bucket dir, sanitize_path_segment(bucket_id).replace('.', '-') keeps the legacy in.c-foo → in-c-foo directory convention for legitimate ids (sanitize preserves single dots, so the trailing replace reproduces the old name exactly), while killing /, .., and absolute-path escapes. Legitimate table names ([A-Za-z0-9_-]+) are unchanged, so <table>.json filenames stay identical.
  • The original (unsanitized) table name is still stored verbatim in the metadata body — only the path segment is sanitized.
• New module-level _ensure_path_within(base_dir, target, what) helper asserts resolved.is_relative_to(workspace) before each mkdir/write — defense-in-depth mirroring _ensure_within_branch, raising ConfigError on escape.

Tests

4 new tests in TestWriteStorageMetadataPathTraversal: malicious table name / bucket id / sample table_id all stay inside the workspace (no escape file created), plus a regression test asserting legitimate in.c-data.users still lands at tables/in-c-data/users.json. Full suite green: 3974 passed, 132 skipped; lint/format/ty/changelog clean.

Audit progress

This is the last of the audit's path-traversal findings (H2 file-download merged in #409; M2 here). Remaining open advisories after this: M4 (--ui auth bypass), M5 (serve-token blast radius), M6 (CORS), M7 (plaintext-on-encrypt warning), M8 (SSRF), M10 (version regex), plus the residual of M1.

---

[devin-ai-integration]

✅ Devin Review: No Issues Found

Devin Review analyzed this PR and found no bugs or issues to report.

[padak]

Review of #412 — fix(sync): block path traversal in storage-metadata write (GHSA-833q, 0.60.3)

Generated by kbagent-pr-reviewer subagent. Verdict and findings below
are advisory; the human author retains every veto. CI-coverable issues
(lint, format, tests) are confirmed via make check, not duplicated here.

Summary

This PR fixes M2 from the 2026-06-12 security audit (GHSA-833q-c5wv-26r7): a path-traversal arbitrary-file-write in sync pull's storage-metadata writer. _write_storage_metadata previously used the API-supplied table name verbatim as a filename and bucket_id.replace(".", "-") (which neutralizes .. but not / or absolute paths), so a malicious/compromised Storage API response could write JSON outside the sync workspace. The fix routes every API-derived path segment (bucket id, table name, sample table id) through sanitize_path_segment(...) and adds a new _ensure_path_within(...) containment check before each mkdir/write — mirroring the config-write path's existing sanitize_path_segment + _ensure_within_branch defense (issue #269). Verdict: APPROVE. The fix is correct, minimal, behavior-preserving for legitimate data (verified empirically), well-tested, and make check is fully green. No blocking issues.

Verdict

• Verdict: APPROVE
• Blocking findings: 0
• Non-blocking findings: 0
• Nits: 2

Blocking findings

(none)

Non-blocking findings

(none)

Nits

• [NIT-1] tests/test_sync_storage_jobs.py:233 — test_malicious_sample_id_stays_inside_workspace only asserts containment of whatever sample.csv files rglob finds; it does not assert a file was actually written, so a hypothetical regression that wrote zero samples would pass vacuously. The sibling table/bucket tests guard this with assert written (line 196). Adding assert list(storage_dir.rglob("sample.csv")) would give this case the same teeth. (Confirmed empirically: the sample currently lands at storage/samples/in-c-data/_-_-_-_-etc-evil/sample.csv, so the test is non-vacuous today — this is purely defense against future drift.)
• [NIT-2] src/keboola_agent_cli/services/sync_service.py:1 — the file is 3712 LOC, well over the 1500-line hard ceiling in CONTRIBUTING.md "File-size budgets". This is pre-existing debt (≈2765 LOC since 0.31.0) and a minimal security hotfix is the wrong PR to force a split into — flagging only so it stays on the radar for the next feature PR that adds material here.

Verification log

What was actually run, with exit codes and trimmed output.

• gh pr view 412 --json ... → OPEN, 7 files, +153/-13, conventional fix(sync): prefix correct for a security/bug fix ✓
• git rev-parse --abbrev-ref HEAD → fix/sync-storage-metadata-traversal (matches PR head; worktree on correct branch) ✓
• Layer-compliance greps on diff (typer/click/formatter/console/httpx in services) → EMPTY ✓ (no layer violation; ConfigError is a plain Exception, not ErrorCode-bound, so the raw-string raise is correct usage matching the existing _ensure_within_branch)
• Command-surface diff (@*_app.command add/remove) → none ✓ (no new command → most Plugin synchronization map rows N/A: no OPERATION_REGISTRY, --hint, context.py, commands-reference.md, or matrix-row obligations)
• sanitize_path_segment behavior reproduced on PR inputs: in.c-data→in-c-data, in.c-foo→in-c-foo (legacy convention preserved); ../../../../evil→_-_-_-_-evil, .../../../../etc/evil→_-_-_-_-etc-evil (traversal neutralized) ✓
• no_storage flag default = False and call site at sync_service.py:1062 is gated only on not dry_run and buckets_data → confirms PR claim that storage metadata is written by default on sync pull ✓
• _ensure_path_within reviewed against _ensure_within_branch it mirrors → identical .resolve() + is_relative_to structure; .resolve() also collapses symlink escapes (strengthens, not weakens) ✓
• ConfigError handling: commands/sync.py catches ConfigError (lines 112/569/1068/…) and _helpers.py::map_error_to_exit_code maps it → a malicious response surfaces as a clean exit code, not an uncaught traceback ✓
• gotchas.md precedent check: #411 (GHSA-wm54, v0.60.2) added a gotcha because it changed agent-observable behavior (extra_args silently no-op); #409 (H2 file-download containment, v0.60.1) did NOT — pure defensive containment, no agent-visible behavior change in normal use. This PR matches the #409 shape (internal writer, byte-identical for legitimate data, no flag/opt-in), so the absence of a gotcha entry is consistent and correct ✓
• pytest tests/test_sync_storage_jobs.py::TestWriteStorageMetadataPathTraversal -v → 4 passed ✓
• Empirical teeth-check of the sample test → 1 sample.csv written inside storage_dir, 0 escape artifacts outside ✓
• make check → EXIT_CODE=0; ruff check "All checks passed", ruff format "293 files already formatted", ty clean, SKILL.md up-to-date, version in sync (plugin.json/marketplace.json/uv.lock/pyproject all 0.60.3), command-sync OK, 3974 passed, 8 skipped ✓
• make check-error-codes → "OK: no raw error_code string literals" ✓ · make changelog-check → "All 48 stable releases have changelog entries" (0.60.3 present) ✓
• Token-discipline grep on diff → no token literals, no credential-like strings, no new HTTP calls/endpoints ✓

Open questions for the author

• _write_storage_metadata is now fully contained, but the per-config-jobs and manifest writers in the same module build paths as base_dir / <alias> / KEBOOLA_DIR_NAME (e.g. sync_service.py:2670). alias is locally-controlled config (not the Storage API response), so it is outside the M2 threat model — noting only to confirm that is the intended scope boundary and not an oversight.