Skip to content

fix(storage): reject path-traversal artifact ids from untrusted slug_hint - #671

Merged
plind-junior merged 1 commit into
vouchdev:testfrom
kai392:fix/critical-issue-artifact-id-path-traversal
Jul 30, 2026
Merged

fix(storage): reject path-traversal artifact ids from untrusted slug_hint#671
plind-junior merged 1 commit into
vouchdev:testfrom
kai392:fix/critical-issue-artifact-id-path-traversal

Conversation

@kai392

@kai392 kai392 commented Jul 30, 2026

Copy link
Copy Markdown

Summary

  • Root cause: KBStore._yaml / _page_path / _source_dir joined attacker-controlled artifact ids straight into filenames. propose_* takes slug_hint verbatim as the id, and Claim/Page/Entity/Relation models do not constrain id the way Source.id is hex-locked. Approving slug_hint="../../../../evil" (or a Windows absolute path) wrote an artifact outside .vouch/, defeating the review gate.
  • Fix: Add _validate_artifact_id at the single path-builder chokepoint (same class of guard as bundle._unsafe_name_reason / read_under_root). Reject empty ids, path separators, NUL, absolute prefixes, ./.., and .. path segments before any file is created. Covers MCP, JSONL, CLI, and direct KBStore callers.
  • Impact: Untrusted proposers can no longer turn an approval into an arbitrary filesystem write. Legitimate slug/sha/ev- ids are unaffected.

Prior attempts (#171, #301, #138, #150, #187) were closed as stale or overscoped; the bug is still present on test. This PR is the minimal storage-only fix with regression tests.

Test plan

  • test_put_rejects_path_traversal_ids — parametrized over ../, separators, absolute, ./.., NUL, empty
  • test_validate_artifact_id_rejects_non_string
  • test_approve_with_traversal_slug_hint_writes_nothing — end-to-end propose+approve must not create the escaped target
  • Full tests/test_storage.py green locally
  • CI lint / mypy / pytest matrix / diff-coverage 100% green

Risk / tradeoffs

  • Proposals already filed with a malicious slug_hint will fail at approve with ValueError instead of writing outside the KB (correct failure mode).
  • No CHANGELOG-only conflict surface beyond a single Unreleased Fixed bullet; no unrelated reformatting.

@kai392
kai392 requested a review from plind-junior as a code owner July 30, 2026 19:35
@github-actions github-actions Bot added docs documentation, specs, examples, and repo guidance storage kb storage, migrations, schemas, and proposals tests tests and fixtures size: S 50-199 changed non-doc lines labels Jul 30, 2026
@kai392
kai392 force-pushed the fix/critical-issue-artifact-id-path-traversal branch from b2f0d99 to 438cbce Compare July 30, 2026 19:42
@kai392

kai392 commented Jul 30, 2026

Copy link
Copy Markdown
Author

@coderabbitai review

@kai392
kai392 force-pushed the fix/critical-issue-artifact-id-path-traversal branch from 438cbce to 1b79a78 Compare July 30, 2026 20:03
@plind-junior
plind-junior enabled auto-merge July 30, 2026 20:03
auto-merge was automatically disabled July 30, 2026 20:11

Head branch was pushed to by a user without write access

@kai392
kai392 force-pushed the fix/critical-issue-artifact-id-path-traversal branch 5 times, most recently from 7b09baa to 7a27ed6 Compare July 30, 2026 20:26
@github-actions github-actions Bot removed the docs documentation, specs, examples, and repo guidance label Jul 30, 2026
Co-authored-by: Cursor <cursoragent@cursor.com>
@plind-junior
plind-junior merged commit 8265e53 into vouchdev:test Jul 30, 2026
16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

size: S 50-199 changed non-doc lines storage kb storage, migrations, schemas, and proposals tests tests and fixtures

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants