diff --git a/CHANGELOG.md b/CHANGELOG.md index 8f4d85d..7da6669 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 af21656..dfc5895 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,15 +255,21 @@ 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: + # 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 ce1d643..b54f583 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 --------------------------------------------------------------- @@ -217,6 +226,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 -------------------------------------------------