fix(storage): reject path-traversal artifact ids on the write path - #171
Closed
seekmistar01 wants to merge 1 commit into
Closed
fix(storage): reject path-traversal artifact ids on the write path#171seekmistar01 wants to merge 1 commit into
seekmistar01 wants to merge 1 commit into
Conversation
Artifact ids flow into filenames (claims/<id>.yaml, pages/<id>.md, sources/<id>/, ...). The proposal layer takes an attacker-controlled `slug_hint` verbatim as the id (proposals.py), and the Claim/Page/Entity/ Relation models do not validate `id` (only Source.id is checked). An untrusted proposer could therefore set `slug_hint="../../../../evil"` and, on approval, have an artifact written outside the KB — the very thing the review gate is meant to prevent. Reads are already containment-checked (`read_under_root`) and bundle import already rejects `..`/absolute/nul member names; this adds the missing write-side guard at the single point where ids become filenames, so every caller (MCP, JSONL, CLI, direct KBStore) is covered. Add `_validate_artifact_id` (rejects path separators, `..`, absolute prefixes, and nul bytes) and route `_yaml`, `_page_path`, and `_source_dir` through it. Legitimate slug/sha/timestamp ids are unaffected. Adds regression tests for the storage builders and the slug_hint approve path. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
|
Lost in the diff? Review this PR in Change Stack to follow the change map from intent to exact ranges. 📝 WalkthroughWalkthroughThis PR adds artifact ID validation to prevent path traversal attacks in the storage layer. A ChangesPath Traversal Mitigation
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Possibly related issues
Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
There was a problem hiding this comment.
Actionable comments posted: 2
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/vouch/storage.py`:
- Around line 130-153: The _validate_artifact_id function currently only rejects
".." when it's a standalone Path part; update the validation to explicitly
reject any raw substring ".." in obj_id (e.g., add a check for '".." in obj_id')
so IDs like "..evil" or "evil..yaml" are considered unsafe, while keeping the
existing checks (empty/non-string, "/" or "\" or NUL, os.path.isabs, "."/".."
exact matches, and Path.parts check) intact; modify the conditional in
_validate_artifact_id to include the raw-substring test and raise the same
ValueError on match.
In `@tests/test_storage.py`:
- Around line 377-393: The test
test_approve_with_traversal_slug_hint_writes_nothing must assert the actual
resolved filesystem target for the malicious slug_hint, not just tmp_path and
tmp_path.parent; compute the exact resolved path by joining store.kb_dir /
"claims" with the provided slug_hint ("../../../../evil") and resolving it
(e.g., via Path(...).resolve()) and then assert that that resolved target (and
resolved_target.with_suffix(".yaml") if claims use .yaml) does not exist after
approve(store, pr.id, ...); update the assertions to check the resolved path(s)
instead of tmp_path / "evil.yaml" and tmp_path.parent / "evil.yaml".
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: 91e47231-dad4-4a8a-a712-1df72de17979
📒 Files selected for processing (2)
src/vouch/storage.pytests/test_storage.py
Comment on lines
+130
to
+153
| def _validate_artifact_id(obj_id: str) -> str: | ||
| """Reject artifact ids that would escape their subdirectory when used as a | ||
| filename. Ids are flat slugs (claims/<id>.yaml, pages/<id>.md, …); a value | ||
| containing a path separator, ``..``, an absolute prefix, or a nul byte must | ||
| never reach a path builder, or an untrusted proposer could write an approved | ||
| artifact outside the KB (e.g. ``slug_hint="../../../../etc/evil"``). | ||
|
|
||
| This is the write-side counterpart to ``read_under_root``: reads are already | ||
| containment-checked, and bundle import already rejects ``..``/absolute/nul | ||
| member names (see ``bundle._unsafe_name_reason``), but the durable write path | ||
| had no equivalent guard. Validating at the single point where ids become | ||
| filenames covers every caller (MCP, JSONL, CLI, direct ``KBStore``). | ||
| """ | ||
| if not obj_id or not isinstance(obj_id, str): | ||
| raise ValueError("artifact id must be a non-empty string") | ||
| if ( | ||
| "/" in obj_id | ||
| or "\\" in obj_id | ||
| or "\x00" in obj_id | ||
| or os.path.isabs(obj_id) | ||
| or obj_id in (".", "..") | ||
| or ".." in Path(obj_id).parts | ||
| ): | ||
| raise ValueError(f"unsafe artifact id (path traversal): {obj_id!r}") |
There was a problem hiding this comment.
Reject .. substrings explicitly.
Line 151 only rejects .. when it is a standalone Path part. IDs like "..evil" or "evil..yaml" still pass, which is looser than the PR contract/docstring and the linked issue’s “contains ..” rule. If the intent is strict rejection, check the raw string directly instead of relying on Path.parts.
Suggested fix
if (
"/" in obj_id
or "\\" in obj_id
or "\x00" in obj_id
or os.path.isabs(obj_id)
- or obj_id in (".", "..")
- or ".." in Path(obj_id).parts
+ or obj_id == "."
+ or ".." in obj_id
):📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def _validate_artifact_id(obj_id: str) -> str: | |
| """Reject artifact ids that would escape their subdirectory when used as a | |
| filename. Ids are flat slugs (claims/<id>.yaml, pages/<id>.md, …); a value | |
| containing a path separator, ``..``, an absolute prefix, or a nul byte must | |
| never reach a path builder, or an untrusted proposer could write an approved | |
| artifact outside the KB (e.g. ``slug_hint="../../../../etc/evil"``). | |
| This is the write-side counterpart to ``read_under_root``: reads are already | |
| containment-checked, and bundle import already rejects ``..``/absolute/nul | |
| member names (see ``bundle._unsafe_name_reason``), but the durable write path | |
| had no equivalent guard. Validating at the single point where ids become | |
| filenames covers every caller (MCP, JSONL, CLI, direct ``KBStore``). | |
| """ | |
| if not obj_id or not isinstance(obj_id, str): | |
| raise ValueError("artifact id must be a non-empty string") | |
| if ( | |
| "/" in obj_id | |
| or "\\" in obj_id | |
| or "\x00" in obj_id | |
| or os.path.isabs(obj_id) | |
| or obj_id in (".", "..") | |
| or ".." in Path(obj_id).parts | |
| ): | |
| raise ValueError(f"unsafe artifact id (path traversal): {obj_id!r}") | |
| if ( | |
| "/" in obj_id | |
| or "\\" in obj_id | |
| or "\x00" in obj_id | |
| or os.path.isabs(obj_id) | |
| or obj_id == "." | |
| or ".." in obj_id | |
| ): |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/vouch/storage.py` around lines 130 - 153, The _validate_artifact_id
function currently only rejects ".." when it's a standalone Path part; update
the validation to explicitly reject any raw substring ".." in obj_id (e.g., add
a check for '".." in obj_id') so IDs like "..evil" or "evil..yaml" are
considered unsafe, while keeping the existing checks (empty/non-string, "/" or
"\" or NUL, os.path.isabs, "."/".." exact matches, and Path.parts check) intact;
modify the conditional in _validate_artifact_id to include the raw-substring
test and raise the same ValueError on match.
Comment on lines
+377
to
+393
| def test_approve_with_traversal_slug_hint_writes_nothing( | ||
| store: KBStore, tmp_path: Path | ||
| ) -> None: | ||
| # End-to-end: an untrusted proposer supplies a malicious slug_hint; the | ||
| # proposal may file, but approval must not write an artifact outside the KB. | ||
| src = store.put_source(b"e") | ||
| pr = propose_claim( | ||
| store, | ||
| text="t", | ||
| evidence=[src.id], | ||
| proposed_by="agent", | ||
| slug_hint="../../../../evil", | ||
| ) | ||
| with pytest.raises((ProposalError, ValueError)): | ||
| approve(store, pr.id, approved_by="reviewer") | ||
| assert not (tmp_path / "evil.yaml").exists() | ||
| assert not (tmp_path.parent / "evil.yaml").exists() |
There was a problem hiding this comment.
Assert against the real escaped write target.
Lines 392-393 do not cover the location this payload would hit. From store.kb_dir / "claims", ../../../../evil.yaml resolves to tmp_path.parent.parent / "evil.yaml", so this test can still pass if the traversal write regresses. Compute that exact resolved target and assert it never appears.
Suggested fix
-def test_approve_with_traversal_slug_hint_writes_nothing(
- store: KBStore, tmp_path: Path
-) -> None:
+def test_approve_with_traversal_slug_hint_writes_nothing(store: KBStore) -> None:
+ escaped = (store.kb_dir / "claims" / "../../../../evil.yaml").resolve()
# End-to-end: an untrusted proposer supplies a malicious slug_hint; the
# proposal may file, but approval must not write an artifact outside the KB.
src = store.put_source(b"e")
pr = propose_claim(
store,
@@
)
with pytest.raises((ProposalError, ValueError)):
approve(store, pr.id, approved_by="reviewer")
- assert not (tmp_path / "evil.yaml").exists()
- assert not (tmp_path.parent / "evil.yaml").exists()
+ assert not escaped.exists()📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| def test_approve_with_traversal_slug_hint_writes_nothing( | |
| store: KBStore, tmp_path: Path | |
| ) -> None: | |
| # End-to-end: an untrusted proposer supplies a malicious slug_hint; the | |
| # proposal may file, but approval must not write an artifact outside the KB. | |
| src = store.put_source(b"e") | |
| pr = propose_claim( | |
| store, | |
| text="t", | |
| evidence=[src.id], | |
| proposed_by="agent", | |
| slug_hint="../../../../evil", | |
| ) | |
| with pytest.raises((ProposalError, ValueError)): | |
| approve(store, pr.id, approved_by="reviewer") | |
| assert not (tmp_path / "evil.yaml").exists() | |
| assert not (tmp_path.parent / "evil.yaml").exists() | |
| def test_approve_with_traversal_slug_hint_writes_nothing(store: KBStore) -> None: | |
| escaped = (store.kb_dir / "claims" / "../../../../evil.yaml").resolve() | |
| # End-to-end: an untrusted proposer supplies a malicious slug_hint; the | |
| # proposal may file, but approval must not write an artifact outside the KB. | |
| src = store.put_source(b"e") | |
| pr = propose_claim( | |
| store, | |
| text="t", | |
| evidence=[src.id], | |
| proposed_by="agent", | |
| slug_hint="../../../../evil", | |
| ) | |
| with pytest.raises((ProposalError, ValueError)): | |
| approve(store, pr.id, approved_by="reviewer") | |
| assert not escaped.exists() |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@tests/test_storage.py` around lines 377 - 393, The test
test_approve_with_traversal_slug_hint_writes_nothing must assert the actual
resolved filesystem target for the malicious slug_hint, not just tmp_path and
tmp_path.parent; compute the exact resolved path by joining store.kb_dir /
"claims" with the provided slug_hint ("../../../../evil") and resolving it
(e.g., via Path(...).resolve()) and then assert that that resolved target (and
resolved_target.with_suffix(".yaml") if claims use .yaml) does not exist after
approve(store, pr.id, ...); update the assertions to check the resolved path(s)
instead of tmp_path / "evil.yaml" and tmp_path.parent / "evil.yaml".
Member
ReviewSummary: Fixes a path-traversal write primitive (issue #170) by adding What works
Suggestions
Verdictapprove — The fix is correct, minimal, and well-tested. It directly addresses all three sinks identified in issue #170, is asymptotically safe (all future write paths that go through |
5 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Closes #170
Rejects path-traversal artifact ids on the durable write path. Artifact ids are interpolated into filenames (
claims/<id>.yaml,pages/<id>.md,sources/<id>/, …), but an untrusted proposer fully controls the id viaslug_hint(proposals.py), andClaim/Page/Entity/Relation.idare unvalidated (onlySource.idis). Soslug_hint="../../../../evil"lets an approved artifact be written outside the KB — defeating the review gate.Reads are already containment-checked (
read_under_root) and bundle import already rejects../absolute/nul names (bundle._unsafe_name_reason); this adds the missing write-side guard at the one point where ids become filenames, so every entrypoint (MCP, JSONL, CLI, directKBStore) is covered with one chokepoint.Changes
src/vouch/storage.py_validate_artifact_id(obj_id)— rejects ids containing a path separator (/or\),.., an absolute prefix, a nul byte, or./..._yaml,_page_path,_source_dirthrough it (covers_claim_path,_entity_path,_relation_path,_evidence_path,_session_path,_proposal_path,_decided_path, which all delegate to_yaml).tests/test_storage.pytest_put_rejects_path_traversal_ids— parametrized over../evil,..,sub/evil,a\b,/abs,., nul — assertsput_claim/put_page/put_entityraiseValueError.test_approve_with_traversal_slug_hint_writes_nothing— end-to-end: a maliciousslug_hintdoes not write any file outside the KB on approve.Why it's safe (no regressions)
Legitimate ids never contain these characters:
_slugifyemits[a-z0-9-]only; source ids are 64-char hex; proposal ids areYYYYMMDD-HHMMSS-<hex>; existing tests use ids likec1,auth-uses-jwt,r1. The guard fires only on the previously-unguarded malicious case.Verification
pytest tests/test_storage.py— 39 passed (31 existing + 8 new).ruff check src/vouch/storage.py tests/test_storage.py— clean.pytest: the only failures are 4 pre-existing, Windows-only cases unrelated to this change —os.O_NOFOLLOWis absent on Windows (in the existingread_under_root) and a bundle frontmatter round-trip; both fail identically onmainwithout this patch (CI runs on Linux).Summary by CodeRabbit
Bug Fixes
Tests