fix(retrieval): contain indexed file paths within the project root - #112

Every join of an index-stored file_path onto the project root now goes through one containment helper, path_containment::resolve_indexed_path (crates/vera-core/src/path_containment.rs). It rejects an empty path and any non-Normal component, joins onto the canonicalized project root, canonicalizes the result, and returns it only if it is still under that root. The root is canonicalized once per query by canonical_project_root, which also absorbs the index_dir.parent() resolution the four call sites each did by hand.

Fixes #108

Threat model

.vera/ is an ordinary committable directory and nothing validates an index's provenance: MetadataStore::open opens whatever is present. So a repository can ship an index whose stored paths point outside itself, and a contributor who clones it and runs a search reads those files, supplying the search pattern themselves. The attacker needs no write access to the victim's machine at any point; the whole attack travels as committed data.

The four sites

All four took a path from SELECT DISTINCT file_path FROM chunks (or from the references / type_relations tables) and joined it onto the project root with no validation. Path::join replaces the base when the right-hand side is absolute, so an absolute stored path escaped outright; .. components escaped by traversal.

Shared, not duplicated

parsing/sphinx.rs:130-143 already did this correctly for .. include:: targets, and that inlined tail is now the shared resolve_within, so the security-critical comparison exists once. The Sphinx resolver keeps its own input contract (an include ref may be root-anchored with a leading / or relative to the including file, neither of which is a repo-relative path) and its own Ok(None) policy, so only the canonicalize-and-prefix core is shared. One behaviour change falls out of the reordering: a repo root that cannot be canonicalized is now an error even when the include target also does not exist, where before the missing target returned Ok(None) first.

Canonicalizing the root is load-bearing rather than incidental: the resolved file path is canonical, so a project reached through a symlinked path would otherwise fail its own containment check.

Index dir shapes

Path::new(".vera").parent() is Some(""), not None, so a relative index dir reached canonicalize() with an empty path and failed with a bare No such file or directory naming nothing. An empty parent names the current directory, and now resolves to it. Every production caller builds the index dir from std::env::current_dir() (vera-cli/src/helpers.rs:177, vera-mcp/src/tools.rs:437) or from canonicalize() (vera-mcp/src/watcher.rs:49), so the broken shape was only reachable by a library caller. A trailing slash is not a separate case: Path::components normalizes it away before the parent is taken, so .vera/ and .vera are the same path.

What a rejected path does

A path that escapes the root is skipped and the row is dropped from the result set, not treated as fatal. A search is a bulk operation over every indexed file, and failing the whole query on one bad row turns a corrupt or hostile index into a denial of service on a tool the user is running for something else.

A silent skip would hide the attack, so each escape logs at warn, naming the offending stored path and the root. tracing is initialized at warn by default, so it surfaces without extra flags.

The three outcomes are kept distinct in Containment for exactly this reason: Escaped warns, Unresolved (deleted, unreadable, broken symlink) stays a silent skip because that is the ordinary case of a file removed since the index was built, and warning on it would bury the real signal.

What this does not close

Tracked as #135, filed at CodeRabbit's request to cover the whole boundary rather than the retrieval half. This PR does not close it, in either direction.

Containment is decided by path, and the file is then opened by path on the next statement. An attacker who can write to the working tree concurrently with a running query can in principle replace a validated entry between the two. That window is not closed here, and this PR should not be read as closing it. Two things bound it:

• A symlink planted before the query is already rejected, because canonicalize resolves the link before the prefix comparison. Only the interleaving remains, and it is one statement wide at each of the four sites.
• The precondition is concurrent write access to the checkout, which already yields code execution on the next build or commit through .git/hooks, build.rs or editor config. That is strictly stronger than reading one file outside the root.

Closing it properly means descriptor-based access: openat per component with O_NOFOLLOW, or cap-std. discovery::read_source_lossy is pub and path-taking and is shared with the three indexing call sites, x86_64-pc-windows-msvc is a release target with no O_NOFOLLOW, and vera-core carries no libc/rustix/cap-std dependency today. The same finding was raised on #111 against discovery/mod.rs and declined there for the same reason; hardening retrieval alone would leave the indexing window, which decides what is stored in the first place, untouched. It belongs in one follow-up covering both, not split across two PRs. That follow-up is #135.

Symlink following inside the repository is a separate finding (#103, in discovery/mod.rs) and is deliberately untouched here.

Verification

End to end, on the same poisoned index and the same query, with a canary file placed one directory above the project root:

• released vera 1.0.0: three hits, two of them the canary's contents, one reached by ../canary.txt and one by its absolute path.
• this branch: one hit, the in-repo file, plus two WARN skipping indexed path that escapes the project root lines.

Tests cover three escape shapes at the helper level (absolute, .. traversal, and a symlink pointing out of the root) and one at a call site (retrieval::regex_search::tests::stored_paths_outside_the_project_root_are_not_read, which asserts no returned result carries content from outside the root). Everything they touch lives in a tempfile::tempdir(); the canary is a file the test writes, never a real one.

Confirmed by reinjection. With the containment check removed and the naive join restored, the helper tests fail with the escaping path returned instead of None (the symlink one returning .../repo/src/leak.rs, which resolves to the canary), and the call-site test fails with the canary's line returned twice, once under ../canary.txt and once under its absolute path. With the empty-parent normalization reverted, relative_index_dir_takes_the_current_directory_as_the_root fails with the production error, failed to canonicalize project root: / No such file or directory (os error 2).

• cargo fmt --check: clean
• cargo test -p vera-core --lib: 802 passed, 0 failed
• cargo test -p vera-cli --bin vera: 97 passed, 0 failed
• cargo clippy -p vera-core --lib: 5 warnings, the pre-existing set, none added

Summary by CodeRabbit

• Bug Fixes
  • Improved repository path validation across search, reference, structural, and type-relation retrieval.
  • Prevented searches and lookups from accessing files outside the project directory.
  • Safely ignore missing, unresolved, absolute, or traversal-based paths.
  • Added coverage for valid paths, escaped paths, unavailable files, and external content.