From 137b515949831c87d1d3398f963af70805d79adb Mon Sep 17 00:00:00 2001 From: minion1227 Date: Tue, 30 Jun 2026 20:48:42 -0700 Subject: [PATCH 1/2] fix(storage): reject path-traversal artifact ids at the path chokepoint MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `Source.id` is locked to a hex sha256, but `Claim`, `Page`, `Entity`, `Relation`, `Evidence`, `Session`, and `Proposal` all carry a bare `id: str` with no validator. `_yaml` / `_page_path` turned those ids straight into filesystem paths with no containment check, so a poisoned id — e.g. `"../../tmp/x"` smuggled through a bundle body that `import_apply` lands under a safe filename but whose in-memory id is traversal — could escape `kb_dir` on the next put / update / lifecycle write, or read an arbitrary file on get. add `_reject_unsafe_id`, mirroring `bundle._unsafe_name_reason`: an id must be non-empty and free of nul bytes, path separators, and `..` components. route it through `_yaml` and `_page_path` — the single chokepoint every `_*_path` helper funnels through — so all artifact read/write paths are guarded at once. a complementary model-layer id validator (mirroring `Source._id_is_hex_sha256`) is left as a follow-up; the storage guard already closes every reach path, including raw-id reads a model validator cannot see. closes #149 --- CHANGELOG.md | 1 + src/vouch/storage.py | 25 +++++++++++++++++ tests/test_storage.py | 62 +++++++++++++++++++++++++++++++++++++++++++ 3 files changed, 88 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 8f4d85de..7da66698 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -96,6 +96,7 @@ All notable changes to vouch are documented here. Format follows KB under `eval/fixture-kb/`, and an `eval` workflow gating retrieval changes (#226). ### Fixed +- artifact ids that contain a path separator, `..`, a nul byte, or are empty are now rejected at the storage layer, closing a path-traversal hole. `Source.id` is hex-locked, but the other models (`Claim`, `Page`, `Entity`, `Relation`, `Evidence`, `Session`, `Proposal`) carry a bare `id: str`, so a poisoned id — e.g. one smuggled through a bundle body under a safe on-disk filename — could resolve to a path outside `kb_dir` on the next put / update / lifecycle write (or read an arbitrary file on get). The guard sits at the `_yaml` / `_page_path` chokepoint every `_*_path` helper funnels through, mirroring the `bundle._safe_member_path` tar-traversal fix (fixes #149). - `parse_since` (the `--since` parser behind `vouch metrics`/`vouch audit`) now raises a clean `MetricsError` for a duration too large to represent (e.g. `--since 1000000000000d`), instead of letting an uncaught `OverflowError` traceback escape — restoring the documented "clean error, not a traceback" contract. - `sync_apply` now loads the sync source exactly once and passes the same `_SyncSource` instance into `sync_check`, closing a TOCTOU window where a bundle replaced on disk between the two `_load_source` calls could cause the validation and write phases to operate on different snapshots. Also eliminates redundant directory walks (KB sources) and triple tarball opens (bundle sources). Fixes #217. - `vault_to_kb` now passes `slug_hint=page_id` to `propose_page` so vault edit proposals target the existing page id from frontmatter instead of a slugified copy of the title (fixes #219). diff --git a/src/vouch/storage.py b/src/vouch/storage.py index af21656c..5ba0863f 100644 --- a/src/vouch/storage.py +++ b/src/vouch/storage.py @@ -144,6 +144,29 @@ def _yaml_load(text: str) -> Any: return yaml.safe_load(text) +def _reject_unsafe_id(obj_id: str) -> None: + """Reject an artifact id that would escape its subdirectory as a path. + + `Source.id` is locked to a hex sha256, but every other artifact model + (`Claim`, `Page`, `Entity`, `Relation`, `Evidence`, `Session`, + `Proposal`) declares a bare `id: str` with no validator. A poisoned id — + e.g. ``"../../tmp/x"`` smuggled through a bundle body that `import_apply` + lands under a safe *filename* but whose in-memory id is traversal — would + otherwise resolve to a path outside `kb_dir` on the next put / update / + lifecycle write (or read arbitrary files on get). This is the single + chokepoint every `_*_path` helper funnels through, so guarding it closes + all reach paths at once. Mirrors `bundle._unsafe_name_reason` (#149). + """ + if not obj_id: + raise ValueError("artifact id must not be empty") + if "\x00" in obj_id: + raise ValueError(f"artifact id contains a nul byte: {obj_id!r}") + if "/" in obj_id or "\\" in obj_id: + raise ValueError(f"artifact id must not contain a path separator: {obj_id!r}") + if obj_id in (".", "..") or ".." in Path(obj_id).parts: + raise ValueError(f"path traversal in artifact id: {obj_id!r}") + + _FRONTMATTER_RE = re.compile(r"^---\n(.*?)\n---\n?(.*)$", re.DOTALL) @@ -232,12 +255,14 @@ def config_path(self) -> Path: return self.kb_dir / CONFIG_FILENAME def _yaml(self, sub: str, obj_id: str) -> Path: + _reject_unsafe_id(obj_id) return self.kb_dir / sub / f"{obj_id}.yaml" def _claim_path(self, claim_id: str) -> Path: return self._yaml("claims", claim_id) def _page_path(self, page_id: str) -> Path: + _reject_unsafe_id(page_id) return self.kb_dir / "pages" / f"{page_id}.md" def _source_dir(self, source_id: str) -> Path: diff --git a/tests/test_storage.py b/tests/test_storage.py index ce1d643f..6a477d9a 100644 --- a/tests/test_storage.py +++ b/tests/test_storage.py @@ -217,6 +217,68 @@ def test_page_with_frontmatter_round_trip(store: KBStore) -> None: assert back.type == PageType.CONCEPT +# --- artifact id path-traversal guard (#149) ------------------------------ + + +@pytest.mark.parametrize( + "bad_id", + ["../escape", "../../etc/passwd", "a/b", "sub\\evil", "/abs", "..", ""], +) +def test_yaml_path_rejects_unsafe_id(store: KBStore, bad_id: str) -> None: + with pytest.raises(ValueError, match="artifact id"): + store._yaml("claims", bad_id) + + +@pytest.mark.parametrize("bad_id", ["../../pwned", "a/b", "/abs"]) +def test_page_path_rejects_unsafe_id(store: KBStore, bad_id: str) -> None: + with pytest.raises(ValueError, match="artifact id"): + store._page_path(bad_id) + + +def test_put_claim_rejects_traversal_id(store: KBStore) -> None: + src = store.put_source(b"e") + with pytest.raises(ValueError, match="artifact id"): + store.put_claim(Claim(id="../../../tmp/pwned", text="x", evidence=[src.id])) + + +def test_get_claim_rejects_traversal_id(store: KBStore) -> None: + # A crafted id on a read path must not resolve to an arbitrary file. + with pytest.raises(ValueError, match="artifact id"): + store.get_claim("../../../etc/hostname") + + +def test_put_page_rejects_traversal_id(store: KBStore) -> None: + with pytest.raises(ValueError, match="artifact id"): + store.put_page(Page(id="../../../tmp/pwned", title="x")) + + +def test_update_claim_rejects_disk_smuggled_traversal_id(store: KBStore) -> None: + """The bundle exploit: a claim lands under a *safe* filename but carries a + traversal string in its `id` field (models don't validate non-Source ids). + Reading it is fine; the next write must refuse to escape kb_dir.""" + src = store.put_source(b"e") + planted = Claim(id="innocent", text="looks fine", evidence=[src.id]) + on_disk = {**planted.model_dump(mode="json"), "id": "../../../tmp/pwned"} + claim_file = store.kb_dir / "claims" / "innocent.yaml" + claim_file.write_text(_yaml_dump(on_disk)) + + c = store.get_claim("innocent") + assert c.id == "../../../tmp/pwned" # in-memory id is traversal + + with pytest.raises(ValueError, match="artifact id"): + store.update_claim(c) + # nothing escaped, and the planted file is untouched by the refused write. + assert claim_file.read_text() == _yaml_dump(on_disk) + + +def test_legitimate_ids_still_resolve(store: KBStore) -> None: + """Regression guard: normal slug / hex ids must keep working.""" + for sub, cid in [("claims", "auth-uses-jwt"), ("entities", "proj_foo"), + ("sessions", "a1b2c3d4")]: + assert store._yaml(sub, cid) == store.kb_dir / sub / f"{cid}.yaml" + assert store._page_path("overview") == store.kb_dir / "pages" / "overview.md" + + # --- entities + relations ------------------------------------------------- From a5db33cf08073b6029a041773256460410f8acc3 Mon Sep 17 00:00:00 2001 From: minion1227 Date: Tue, 30 Jun 2026 21:02:01 -0700 Subject: [PATCH 2/2] fix(storage): guard source paths against traversal ids too address review on #301: `_yaml` / `_page_path` were guarded, but `get_source` / `read_source_content` (and the evidence-ref existence checks) route raw `source_id` strings through `_source_dir` without it, so `get_source("../../outside")` could still escape kb_dir to `/meta.yaml` / `content`. `Source.id` is hex-locked on the model but these read paths take unvalidated strings. apply `_reject_unsafe_id` at the `_source_dir` chokepoint too, closing the last storage traversal path. add get_source / read_source_content rejection tests. --- src/vouch/storage.py | 4 ++++ tests/test_storage.py | 9 +++++++++ 2 files changed, 13 insertions(+) diff --git a/src/vouch/storage.py b/src/vouch/storage.py index 5ba0863f..dfc58959 100644 --- a/src/vouch/storage.py +++ b/src/vouch/storage.py @@ -266,6 +266,10 @@ def _page_path(self, page_id: str) -> Path: return self.kb_dir / "pages" / f"{page_id}.md" def _source_dir(self, source_id: str) -> Path: + # Source.id is hex-sha256-locked on the model, but get_source / + # read_source_content / the evidence-ref existence checks route raw + # id strings through here, so guard this chokepoint too (#149). + _reject_unsafe_id(source_id) return self.kb_dir / "sources" / source_id def _entity_path(self, eid: str) -> Path: diff --git a/tests/test_storage.py b/tests/test_storage.py index 6a477d9a..b54f583a 100644 --- a/tests/test_storage.py +++ b/tests/test_storage.py @@ -131,6 +131,15 @@ def test_source_hash_field_mirrors_id(store: KBStore) -> None: assert s.hash == s.id +def test_get_source_rejects_traversal_id(store: KBStore) -> None: + # get_source / read_source_content take raw id strings; a crafted id must + # not resolve to /meta.yaml or /content (#149). + with pytest.raises(ValueError, match="artifact id"): + store.get_source("../../../etc/hostname") + with pytest.raises(ValueError, match="artifact id"): + store.read_source_content("../../outside") + + # --- claims ---------------------------------------------------------------