fix(storage): reject path traversal in artifact ids

[glorysr1209-png]

Summary

Closes #137. Reject artifact IDs containing path separators or .. before writing under .vouch/.

Changes

• src/vouch/storage.py: _unsafe_artifact_id_reason, _assert_safe_artifact_id on yaml/page/source paths.
• tests/test_storage.py: regression for slug_hint="../../../pwned" on approve.

Test plan

• pytest tests/test_storage.py::test_slug_hint_path_traversal_rejected_on_approve (Python 3.11+ / package installed)

Summary by CodeRabbit

• Bug Fixes

  • Strengthened validation to prevent path traversal and unsafe identifiers so stored artifacts cannot escape the designated storage area.
  • Rejects identifiers containing path separators, NUL bytes, empty/whitespace values, and traversal segments.

• Tests

  • Added a test ensuring proposals with path-traversal hints are rejected and do not create files outside the storage directory.

[coderabbitai]

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info

⚙️ Run configuration

Configuration used: defaults

Review profile: CHILL

Plan: Pro Plus

Run ID: fe331265-5e93-4e4d-a513-8fa11d513373

📥 Commits

Reviewing files that changed from the base of the PR and between 84e3a17 and 58ffd89.

📒 Files selected for processing (2)

• src/vouch/storage.py
• tests/test_storage.py

🚧 Files skipped from review as they are similar to previous changes (2)

• tests/test_storage.py
• src/vouch/storage.py

---

📝 Walkthrough

Walkthrough

This PR hardens artifact storage by validating artifact identifiers (rejecting empty, separators, NUL, and ..) and ensuring resolved artifact paths stay inside the intended .vouch/<sub> directory; the checks are applied to _yaml, _page_path, and _source_dir. A test verifies approve() rejects traversal slug_hints.

Changes

Path Traversal Prevention

Layer / File(s)	Summary
Artifact ID validation contract src/vouch/storage.py	_unsafe_artifact_id_reason() validates artifact identifiers by rejecting empty/whitespace, path separators (/, \), NUL bytes, and .. traversal segments.
Path safety enforcement in KBStore src/vouch/storage.py	KBStore._assert_safe_artifact_id() combines identifier validation with resolved-path containment checks and is wired into _yaml, _page_path, and _source_dir to prevent directory escapes.
Test for path traversal rejection tests/test_storage.py	test_slug_hint_path_traversal_rejected_on_approve() verifies that a traversal slug_hint is rejected during approval and no artifact is created outside the KB storage directory.

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related issues

• validation gap: artifact ids accept path-traversal strings and reach filesystem paths unsanitised in storage operations #149 — Fix aligns with suggested storage-layer containment and artifact-id safety checks to reject traversal/separators/NUL and ensure resolved paths remain under .vouch/<sub>.

Poem

🐰 I found a sneaky little trail,
But .. and slashes will not prevail.
I guard the vault with quiet cheer,
No pwned files will appear here.
Safe paths hop home, the KB stays hale.

🚥 Pre-merge checks | ✅ 4 | ❌ 1

❌ Failed checks (1 warning)

Check name	Status	Explanation	Resolution
Docstring Coverage	⚠️ Warning	Docstring coverage is 0.00% which is insufficient. The required threshold is 80.00%.	Write docstrings for the functions missing them to satisfy the coverage threshold.

✅ Passed checks (4 passed)

Check name	Status	Explanation
Description Check	✅ Passed	Check skipped - CodeRabbit’s high-level summary is enabled.
Title check	✅ Passed	The title 'fix(storage): reject path traversal in artifact ids' accurately and clearly summarizes the main change: adding path traversal rejection for artifact identifiers in the storage module.
Linked Issues check	✅ Passed	The PR successfully implements the requirements from issue #137: validates artifact IDs by rejecting path separators (/ and \), the .. segment, and ensures resolved paths remain under the .vouch/ directory with containment checks.
Out of Scope Changes check	✅ Passed	All changes are directly scoped to addressing issue #137: validation logic in storage.py and a regression test in test_storage.py, with no unrelated modifications present.

_{✏️ Tip: You can configure your own custom pre-merge checks in the settings.}

✨ Finishing Touches

🧪 Generate unit tests (beta)

• Create PR with unit tests

---

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.

❤️ Share

• X
• Mastodon
• Reddit
• LinkedIn

_{Comment @coderabbitai help to get the list of available commands and usage tips.}

[coderabbitai]

Actionable comments posted: 1

🧹 Nitpick comments (1)

src/vouch/storage.py (1)

217-223: 💤 Low value

Redundant containment checks with mismatched extensions.

_assert_safe_artifact_id("pages", page_id) performs a containment check for {page_id}.yaml, but pages use .md. The subsequent check for .md is correct, making the .yaml check redundant.

Consider extracting ID-only validation from _assert_safe_artifact_id to avoid the redundant path resolution, or parameterizing the extension/suffix.

🤖 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 217 - 223, The _page_path method currently
calls _assert_safe_artifact_id("pages", page_id) which validates containment
using a .yaml suffix while _page_path builds a .md path, causing
redundant/mismatched checks; refactor by extracting the ID-only validation from
_assert_safe_artifact_id (e.g., _assert_safe_id or is_valid_artifact_id) or
modify _assert_safe_artifact_id to accept a suffix/extension parameter (e.g.,
ext=".md") and use that when called from _page_path, update _page_path to call
the new ID-only validator or pass ext=".md", and remove the redundant .md
containment resolution so both checks consistently validate the same target path
(reference symbols: _page_path, _assert_safe_artifact_id, kb_dir, "pages").

🤖 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 `@tests/test_storage.py`:
- Around line 232-237: The canary path and the slug_hint disagree: with
slug_hint="../../../pwned" from .vouch/claims/ traversal lands at
store.root.parent/"pwned.yaml" but the test checks store.root/"pwned.yaml". Fix
by making the test and slug_hint consistent—either change slug_hint to
"../../pwned" so approve(pr.id, approved_by="reviewer") would target
store.root/"pwned.yaml", or keep slug_hint="../../../pwned" and update the
canary variable to canary = store.root.parent / "pwned.yaml" so the existence
assertion matches the actual traversal; ensure references to approve, slug_hint,
and canary are updated accordingly.

---

Nitpick comments:
In `@src/vouch/storage.py`:
- Around line 217-223: The _page_path method currently calls
_assert_safe_artifact_id("pages", page_id) which validates containment using a
.yaml suffix while _page_path builds a .md path, causing redundant/mismatched
checks; refactor by extracting the ID-only validation from
_assert_safe_artifact_id (e.g., _assert_safe_id or is_valid_artifact_id) or
modify _assert_safe_artifact_id to accept a suffix/extension parameter (e.g.,
ext=".md") and use that when called from _page_path, update _page_path to call
the new ID-only validator or pass ext=".md", and remove the redundant .md
containment resolution so both checks consistently validate the same target path
(reference symbols: _page_path, _assert_safe_artifact_id, kb_dir, "pages").

🪄 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: 35522639-cc78-4d33-9d77-19a5afecf686

📥 Commits

Reviewing files that changed from the base of the PR and between 3beb821 and 84e3a17.

📒 Files selected for processing (2)

• src/vouch/storage.py
• tests/test_storage.py

[plind-junior]

Review

Summary: Adds layered path-traversal validation to all artifact-ID path helpers in KBStore, closing the slug_hint escape reported in #137. The implementation is correct and complete with a targeted regression test.

What works

• src/vouch/storage.py:50-58 — _unsafe_artifact_id_reason uses three independent guards (slash/backslash/NUL string check → .. segment check via Path.parts → resolve() + is_relative_to belt-and-suspenders) in the right order; cheaper string checks run first.
• src/vouch/storage.py:202-210 — _assert_safe_artifact_id is correctly wired into _yaml() (covers claims, entities, relations, evidence, sessions, proposed, decided) and also called explicitly from _page_path and _source_dir with the correct suffix arguments, so all nine artifact path helpers are guarded with no double-validation.
• tests/test_storage.py:225-239 — Regression test exercises the exact attack vector from the issue (slug_hint="../../pwned" → approve()), checks both the exception and that the canary file was never created. store.root / "pwned.yaml" correctly mirrors where .vouch/claims/../../pwned.yaml would resolve to.

Suggestions

• [non-blocking] src/vouch/storage.py:56 — The resolve()-based containment check is valuable as a belt-and-suspenders layer, but when the subdirectory doesn't exist yet (first write), Path.resolve() performs a lexical normalization rather than a full filesystem canonicalization (symlinks in the existing prefix could still bypass it). This is an edge case in typical deployments, but if the KB root is ever under a symlinked path the check may give a false sense of security. Consider documenting this limitation or adding a note that _assert_safe_artifact_id is not a substitute for OS-level confinement.
• [non-blocking] tests/test_storage.py:235 — The test only covers propose_claim / approve. Adding a parallel case for propose_page with a slug_hint containing .. would confirm that _page_path's explicit guard also fires, since it doesn't route through _yaml().

Verdict

approve — The fix directly addresses all the affected code paths identified in #137, the validation logic is sound for the stated threat model, and the regression test covers the primary attack vector. The suggestions above are minor hardening notes, not correctness issues.