validation gap: artifact ids accept path-traversal strings and reach filesystem paths unsanitised in storage operations

[galuis116]

Problem

Every non-Source artifact model — Claim, Page, Entity, Relation, Evidence, Session, Proposal — declares id: str with no validator. Source.id is locked to a hex sha256 by _id_is_hex_sha256 (src/vouch/models.py:148-153); nothing similar exists for the others (src/vouch/models.py:179, 225, 238, 254, 163, 290, 318 — all bare id: str).

The storage layer turns those ids straight into filesystem paths with no containment check:

# src/vouch/storage.py:220-221
def _yaml(self, sub: str, obj_id: str) -> Path:
    return self.kb_dir / sub / f"{obj_id}.yaml"

_claim_path, _entity_path, _relation_path, _evidence_path, _session_path, _proposal_path, _decided_path all delegate to _yaml. _page_path does the same with .md. So an id like "../../etc/passwd" produces a Path that, when written, escapes kb_dir entirely — Python's pathlib does no normalisation, and there is no resolve().relative_to(kb_dir) guard the way bundle._safe_member_path does for tar member names (src/vouch/bundle.py).

Reachable write paths that take an id from outside and trust it:

• KBStore.update_claim(claim) / update_session(sess) — use claim.id / sess.id directly: self._claim_path(claim.id).write_text(...), self._session_path(sess.id).write_text(...).
• KBStore.put_claim / put_page / put_entity / put_relation / put_evidence / put_relation_idempotent — with self._<X>_path(obj.id).open("x"): writes a NEW file at the (possibly escaped) path.
• lifecycle.supersede / contradict / archive / confirm — take claim_id from the caller, read via get_claim(claim_id), then write via update_claim(claim). Both the read and write resolve through _claim_path(claim_id).

Two concrete exploit chains:

1. Bundle import smuggles a poisoned id; later lifecycle op writes outside kb_dir

bundle.import_apply uses _safe_member_path to keep each tar member's file path inside kb_dir (the #9 / CVE-2007-4559 fix). What it does not check is the id field inside the YAML body. A bundle can ship claims/innocent-name.yaml whose content is:

id: "../../../../../tmp/pwned"
text: "looks fine"
type: fact
status: stable
confidence: 1.0
evidence: [<a real source id>]

_validate_content runs Claim.model_validate(...) — which accepts the string because Claim.id has no validator. The content-address gate added in #125 only checks sources/<sha>/, so it ignores claims. The bundle imports cleanly; on disk you now have claims/innocent-name.yaml whose on-disk filename is safe but whose in-memory Claim.id is "../../../../../tmp/pwned".

Any subsequent operation that loads that claim and writes back to it lands the write at kb_dir/claims/../../../../../tmp/pwned.yaml, i.e. outside the project tree:

$ python repro.py
ls /tmp/pwned.yaml
# YAML body persisted outside the .vouch/ tree, with no audit trail
# that the bundle had anything to do with the off-tree path.

Triggering operations include store.update_claim(claim), lifecycle.archive(store, claim_id="../../../../../tmp/pwned"), lifecycle.supersede(...), anything that iterates store.list_claims() and persists any change.

2. Direct MCP / JSONL call with a traversal id

The MCP and JSONL servers expose kb.archive, kb.supersede, kb.contradict, kb.confirm, kb.cite (README §"MCP tools / JSONL methods"). Each takes a claim_id straight from the caller and routes it through lifecycle.<op>(store, claim_id=…) → store.get_claim(claim_id) → store.update_claim(claim).

An agent that calls kb.archive(claim_id="../../../etc/cron.d/whatever") (or any writable target) tries to read that path. If the file does not exist or does not parse as Claim, get_claim raises ArtifactNotFoundError and the attack is a no-op. If a file does parse as a Claim — either because the attacker planted one via bundle vector (1), or because another tool persists YAML-shaped content at predictable paths — the file is loaded, claim.status is mutated to archived, and update_claim overwrites the original off-tree file with the modified YAML body. That is an arbitrary-file overwrite primitive exposed through the MCP/JSONL surface.

3. Direct construction is the simplest reproducer

The cleanest demonstration does not need a bundle at all:

from vouch.models import Claim
from vouch.storage import KBStore

store = KBStore.init(tmp)
src = store.put_source(b"e")
store.put_claim(Claim(id="../../escaped", text="t", evidence=[src.id]))

# File on disk:
assert (tmp / "escaped.yaml").exists()        # outside .vouch/
assert not (tmp / ".vouch" / "claims" / "escaped.yaml").exists()

The bare Claim(id="../../escaped") is accepted by Pydantic (no validator) and put_claim's only existence check is on claim.evidence, not on claim.id. The file lands one directory above .vouch/.

Why this matters

• README §"File layout" pins every artifact kind to kb_dir/<sub>/<id>.{yaml,md}. The system contract is that kb_dir is the unit of containment. Bundle import already takes this seriously (_safe_member_path in src/vouch/bundle.py, Path Traversal in Bundle Import — Arbitrary File Write (Critical) #9 / CVE-2007-4559); read_under_root already takes it seriously for source ingestion (src/vouch/storage.py:161-196); KBStore._yaml is the one path-building site that quietly does not.
• This is the same structural shape as the prior validation-gap fixes: an invariant the codebase already enforces at one boundary, but not at the model layer. bug: Claim model has no min-evidence validator — uncited claims land via bundle import, put_claim, update_claim #81 (Claim.evidence non-empty), validation gap: every write path lands Relations/Pages with dangling foreign-id references #123 (relation/page foreign-id refs resolve), validation gap: bundle/sync import doesn't enforce Source content-addressing (sha256(content) == id) #125 (sha256(content) == source.id) all share the pattern "tighten the model + tighten every write path." Sanitised artifact ids are the next entry in that list and arguably the most security-relevant — every prior fix prevents corrupt data; this one prevents an arbitrary-file write.
• The fix is genuinely small: a model-level validator on six id fields (plus Proposal.id, Session.id) and a one-line containment check in _yaml. Honest ids (slug-style, like auth-uses-jwt) keep working unchanged; the system already produces them via _slugify in proposals.py.

Suggested fix

Two layers, mirroring how prior fixes were structured:

1. Model-layer validator. Add a shared _safe_slug_id(v: str) validator (or one per model) that rejects id values containing /, \, .., \x00, leading ., control characters, or whitespace — i.e. exactly the strings _safe_member_path already rejects for tar members. Apply via @field_validator("id") on Claim, Page, Entity, Relation, Evidence, Session, and Proposal. The existing slugifier (proposals._slugify) already produces ids that satisfy this constraint, so honest write paths are unaffected.

2. Storage-layer containment. In KBStore._yaml (and the Path helpers that build off it), resolve() the constructed path and confirm is_relative_to(self.kb_dir / sub); raise ValueError("artifact id escapes kb_dir: <id>") otherwise. This is the same belt-and-suspenders pattern read_under_root already uses for source reads. Catches anything the model validator might miss (e.g. on-disk YAMLs predating the model fix, exotic Unicode tricks, future model-layer regressions).

3. Regression tests in the existing locations:

   • tests/test_storage.py — Claim(id="../escape") raises ValidationError; store.put_claim(Claim(id="../escape", ...)) raises before any file is written; store.update_claim re-validates an in-place mutation (c.id = "../escape") before persisting.
   • tests/test_bundle.py — a bundle whose claim YAML has id: "../escape" is rejected by import_check (schema validation failed) and import_apply refuses; bundle-imported claims with safe ids continue to land cleanly (round-trip guard).
   • tests/test_storage.py lifecycle: lifecycle.archive(store, claim_id="../escape", actor=...) raises ValueError (or ArtifactNotFoundError from the storage-layer guard) without touching disk.

No on-disk-layout, schema, or bundle-format change. Existing KBs are unaffected on read — health.lint already iterates per-file and tolerates pydantic.ValidationError (after the #82 review fix), so a legacy KB that already contains a poisoned id surfaces as an invalid_claim finding instead of crashing the lint sweep. The migration story mirrors #81 exactly.

Will follow with the patch.