Route the external-PHP disk loader through the path_within_root containment guard

[mikebronner]

Summary

SalsaActor::ensure_external_php_source_loaded read file metadata and content straight from disk and registered the result as a Salsa SourceFile, which handle_blade_backing_class_resolution maps into BladeBackingResolutionData::files — the goto-definition target list. It carried no containment guard of its own.

Nothing escapes today: render-index candidates come from the project's own directory walk, and Livewire candidates come from livewire_resolver::resolve_component, which gates every path segment through naming::is_safe_path_segment. The hazard is structural — the guard lived in the callers, and a new read site does not inherit the guard its neighbours carry. That exact shape has already become a security fix three times in this repo (#294, #348 round 1, #348 round 2).

The guard now lives at the primitive.

Fixes #364

The guard, split by branch

The two branches ask different questions, so they take different guards. The split is the AC bullet 2 waiver granted in review — the read branch keeps the full fail-closed guard, and the emit-safe guard is an addition to a branch the read guard never covered.

// Root unknown: refuse before any state is read, mutated, or stat'd.
let root = self.config_root.clone()?;

// Gate against the candidate's OWNING MODULE where it has one.
let gate = crate::config::owning_module(&self.module_dirs, path)
    .map(|(_, dir)| dir.to_path_buf())
    .unwrap_or(root);

if self.external_php_text.get(path) == Some(&ExternalPhpText::PushedByClient) {
    // Reads no disk — but the path IS emitted as a goto target.
    if !path_within_root_emit_safe(path, &gate) { return None; }
    if let Some(file) = self.files.get(path).copied() { return Some(file); }
}

// Reads real bytes, so containment must be proven.
let real = canonical_within_root_registration(path, &gate)?;
let current_mtime = std::fs::metadata(&real).ok()?.modified().ok()?;
// …
let text = std::fs::read_to_string(&real).ok()?;

• Ownership fast path → path_within_root_emit_safe. Its lexical pre-gate refuses an out-of-root candidate with no stat probe (Run slot-navigation containment guard before file_exists_cached to close out-of-root existence oracle #145). Its None arm admits a genuinely absent in-root path — the unsaved-buffer case Component-member navigation: follow-ups from #335 #361 exists to protect — while refusing a dangling under-root symlink and every non-NotFound lstat error. The fail-closed read guard cannot serve this branch: it refuses exactly the momentarily-absent buffer, which would reintroduce Component-member navigation: follow-ups from #335 #361. There is a test that proves this.
• Disk branch → canonical_within_root_registration. Documented for precisely this shape: a path minted from discovered source data and then read. Keeps the lexical pre-gate, fails closed on anything it cannot canonicalize.
• The gate is the candidate's owning module, falling back to the project root. config::expand_module_dirs admits a module directory whose real target sits outside the project on purpose — that is the composer path-repository layout — and livewire_namespaces::contained_class_path gates the registrations that mint these paths against the same owning module so they survive. Gating this read against the root alone dropped every backing class inside such a module, silently. The swap does not loosen the guard; for a module path it tightens it, because a candidate lexically under a module must canonicalize inside that module, so one reaching into a sibling module or into bare app/ is refused despite being in-root. config::owning_module collapses .. before its prefix test, so a traversing path cannot elect itself a laxer gate, and with no modules.paths configured it never matches — the gate is exactly the root and behaviour is unchanged.
• Both filesystem calls go through real, the verified canonical path the guard returns — never re-derived from path, so a symlink swapped between guard and read cannot hand back a target the guard never approved. path stays the key for self.files and external_php_text so caller lookups still resolve.

Tests

Every rejection test below was verified to fail with the guards removed; both positive tests still pass without them, so they pin non-regression rather than the guard.

salsa_impl/tests.rs — direct-actor, asserting the internal maps (AC bullets 3, 6, 7). No SalsaHandle message exposes files or external_php_text, so SalsaActor::new was extracted from spawn (first commit, no behaviour change) to let the in-crate test module hold an actor.

Test	Pins
loader_refuses_an_out_of_root_candidate_and_caches_nothing	rejection leaves no files / external_php_text entry
loader_refuses_a_candidate_when_the_root_is_unknown	the config_root short-circuit — proved by loading the same path once the root is set
loader_refuses_an_under_root_symlink_that_escapes	symlink escape, with an identical-content in-root contrast
retargeting_a_symlink_out_of_root_cannot_disturb_the_cached_load	cached text and recorded mtime survive a later escape attempt
a_client_pushed_out_of_root_path_is_refused_even_when_cached	client ownership is not a containment exemption
a_client_pushed_in_root_buffer_still_answers_while_its_file_is_absent	#361 stays whole under the guard
a_backing_class_inside_a_symlinked_module_still_loads	a composer path-repository module resolves
a_path_escaping_its_own_module_is_still_refused	electing a module gate is not a way out of it
a_module_path_reaching_back_into_the_app_is_refused	in-root is not enough for a module candidate
a_traversing_path_cannot_elect_a_module_as_its_gate	.. is collapsed before the gate is chosen
without_modules_the_gate_is_the_project_root	zero change for a project without modules

src/tests/external_php_loader_containment.rs — through the real pipeline (AC bullets 4, 5, 8). Candidates enter via SalsaHandle::blade_backing_class_resolution, the same entry point main.rs uses, and travel blade_backing_class_files rather than being handed to the loader directly. Each rejection test asserts the fixture's canonicalized path really is outside the root first, so it cannot pass vacuously. The positive test compares the resolved source against bytes read back off disk, not a literal duplicated on both sides.

Fixture correction

Three test harnesses set the backend's root_path but never the actor's config_root. handle_register_project_files does not set it — only handle_register_config_files and the RegisterCachedConfig request do. Production always registers config first (main.rs:8229 for the mid-session root change, main.rs:23644 for startup, with RegisterCachedConfig covering the warm-cache branch), so this was fixture drift, not a behaviour change:

• src/tests/render_index_and_buffer_integrity.rs
• src/tests/component_ancestor_navigation.rs
• src/tests/component_member_navigation.rs

Sibling-site audit (AC bullet 9)

Every site in salsa_impl.rs that reads file content/metadata from disk, or reads/writes self.files, with its containment status.

Direct filesystem reads

Line	Site	Status
2057, 2076	TranslationCache::ensure_file / ensure_dir	Guarded — path_within_root immediately above each (#248)
2120	TranslationCache::ensure_config	Contained by construction — the only caller is config_value, whose only caller is completion_locale, which passes the two literals "app.locale" / "app.fallback_locale". group is therefore always "app"; no project-derived text reaches config_group_files
2537	TranslationCache::ensure_provider	Contained by construction, already documented — paths come from walking the project's own vendor/ and app/Providers/. This PR follows its doc-comment precedent
10099, 10110	ensure_external_php_source_loaded	Guarded by this PR, and read through the guard's verified canonical path
11360	ensure_file_registered	Contained by construction — rationale comment added by this PR. See below
11936	handle_resolve_facade_receiver_at	Guarded upstream — class_file comes from class_locator::find_php_class_file_in_app_or_vendor, which applies path_within_root to every candidate (class_locator.rs:405) and revalidates cache hits with it (:163, :234)

self.files accesses

2009, 2014, 2035, 2052 are TranslationCache::files (a HashMap<PathBuf, LangFile>), a different map from SalsaActor::files; its read is guarded at 2057. On the actor:

Line	Site	Status
9146	RemoveFile handler	Eviction only, keyed by a path the client named
9816, 9823	handle_update_file	Client push of the client's own open document
9997, 10013	ensure_blade_source_registered	Client push (live editor buffer), no disk read
10076, 10103, 10118, 10122, 10128	inside the loader	Guarded by this PR — every one sits after the branch's guard
10133, 10154	handle_get_php_assignments, handle_get_document_symbols	Read-only lookup of an already-registered input; no filesystem call, no registration
10209, 11594, 11687, 11899, 11976	reads immediately after ensure_file_registered	Same containment as that function
11359	ensure_file_registered	See below

ensure_file_registered — resolved as contained, with a comment

All five call sites (handle_get_patterns, handle_find_magic_member_references, hover_for_magic_member, handle_resolve_facade_receiver_at, handle_magic_member_rename_data) pass the request's own textDocument.uri — the document the client already has open and is asking about. That is not a path minted by joining project-derived text onto a directory, so there is no traversal to fence: the client supplied the path, holds the file open, and did_open/did_change already register arbitrary client paths through handle_update_file. The text read is parsed for the answer; the path itself is never emitted as a new navigation target.

Guarding it would also be a behaviour change rather than a hardening — a file legitimately open outside the workspace root would stop answering hover and goto entirely. The rationale is recorded as a doc comment on the function, following the ensure_provider precedent.

Review round 1 — the module-gate regression

The first push gated against config_root alone. That silently dropped every backing class inside a module symlinked in from a composer path repository — the exact silent-failure mode livewire_namespaces.rs:205 records having already been made and fixed one module over. Caught by adversarial review, reproduced with a failing test, fixed by the owning-module gate described above.

Every containment test was then verified against two mutations — reverting the gate to root-only, and removing both guards. That sweep also caught one new test passing under both mutations: the traversal case had been built on a symlinked module directory, where the OS resolves an interior .. against the link's target, so the escape landed nowhere and metadata() failed regardless of any guard. Rebuilt on a real directory so the escape target is genuinely readable; it now fails when the guard is removed.

Verification

• cargo test — 3609 passed, 0 failed
• cargo clippy --all-targets — clean
• cargo fmt --check — clean

[dr-john-h-watson]

The block

AC bullet 1 orders the containment guard before the loader touches self.files. The loader's first statement today is the client-ownership fast path, which reads self.files. That ordering exists on purpose. I cannot satisfy both without a guard the AC forbids.

The evidence

ensure_external_php_source_loaded starts at salsa_impl.rs:10013. Its first statement is the ownership check at :10019. The comment above it at :10014-10018 states the reason:

Ordering the stat first would drop an unsaved buffer out of backing-class resolution whenever its file is momentarily absent — a branch switch, a stash, an artisan make:* regeneration.

That branch reads self.files and returns client-supplied text. It runs no filesystem call. It mutates nothing. The first filesystem call is std::fs::metadata at :10029.

Why the tension is irreducible

The two orderings differ for exactly one path: client-pushed, present in self.files, and failing containment. That case splits in two.

• Out of root. Containment-first refuses it. Ownership-first serves it, and handle_blade_backing_class_resolution then returns the path as a goto-definition target. Ownership-first is wrong here.
• In root, momentarily absent on disk. Both path_within_root (path_containment.rs:95) and canonical_within_root_registration (:236) fail closed on a path that cannot canonicalize. Containment-first refuses it and reintroduces the Component-member navigation: follow-ups from #335 #361 defect. Containment-first is wrong here.

Telling the two apart needs a lexical containment test next to the canonical one. AC bullet 2 forbids that: no ||/&& wrapper, no path_within_root_lexical.

Options

1. Containment first, literal AC. Call canonical_within_root_registration as the first statement. The ownership fast path moves below it.
   Pros: satisfies bullets 1 and 2 exactly. Closes the out-of-root goto target.
   Cons: narrows Component-member navigation: follow-ups from #335 #361. An unsaved buffer drops out of backing-class resolution while its file is absent from disk.

2. Ownership first, containment before every filesystem call and every mutation. Keep :10019 where it is. Put the guard directly after it.
   Pros: keeps Component-member navigation: follow-ups from #335 #361 whole. Matches your AC verification note, which says "before any filesystem touch and before any mutation."
   Cons: violates bullet 1 as written. An out-of-root client-pushed path still surfaces as a goto target.

3. Split the guard by branch. Gate the ownership fast path with path_within_root_lexical. Gate the disk path with canonical_within_root_registration.
   Pros: correct in both failure cases. Refuses the out-of-root path with no disk probe, and still serves the in-root absent buffer.
   Cons: needs a waiver on bullet 2, which names the lexical guard as a prohibited substitution.

Recommendation: 3. It is the only option that fails closed on the out-of-root case and keeps #361. Each guard answers the question its own branch asks: the ownership branch serves client text and needs containment without a disk probe, the disk branch reads real bytes and needs proof. Bullet 2 bans the lexical guard as a substitution for the read guard. Here it is an addition to a branch the read guard never covered.

If you reject the waiver, I will take option 1 and pin the #361 narrowing with a test and a comment.

One correction to the AC

Bullets 1 and 3 name a field external_php_mtimes. That field does not exist. PR #361 replaced it with external_php_text: HashMap<PathBuf, ExternalPhpText> at salsa_impl.rs:8777. I will read the criteria against external_php_text.

[mr-sherlock-holmes]

Take option 3, with one correction: the ownership branch uses path_within_root_emit_safe, not path_within_root_lexical. Waiver on bullet 2 granted. 🔒️

Your analysis is right, and I verified every claim against the tree. The ownership fast path at salsa_impl.rs:10019 reads self.external_php_text and self.files before any filesystem call. The first filesystem call is std::fs::metadata at :10029. The tension is real, and it is irreducible with a single guard.

I also confirmed the hazard is not theoretical. handle_blade_backing_class_resolution maps the returned SourceFile handles into files: Vec<PathBuf> (:9950-9953), and that vector is the goto-definition target list. An out-of-root path served by the ownership branch reaches the client as a location to open.

Why the lexical guard is the wrong instrument

path_within_root_lexical's own contract disqualifies it here (path_containment.rs:142-143):

Not a security guard for paths that will be read or emitted: those must use path_within_root, which is fail-closed.

The ownership branch performs no read. It does emit. So the lexical guard is banned for this branch by the module's documented contract, not just by AC bullet 2.

Why path_within_root_emit_safe is the right one

path_within_root_emit_safe (path_containment.rs:181) has exactly the semantics your branch needs:

• It shares the lexical pre-gate, so an out-of-root candidate is refused with no stat probe. Issue Run slot-navigation containment guard before file_exists_cached to close out-of-root existence oracle #145's existence oracle stays closed.
• Its None arm admits a path that lstat reports as NotFound — a genuinely absent in-root file. That is the unsaved-buffer case Component-member navigation: follow-ups from #335 #361 protects, so Component-member navigation: follow-ups from #335 #361 stays whole.
• It refuses a dangling under-root symlink and every non-NotFound lstat error (EACCES, ENOTDIR, ELOOP). An unprovable path cannot be used to escape.

The guard was written for a path emitted to a client that may act on it. A goto target is that.

The shape I want

// Root unknown: refuse before any state is touched.
let root = self.config_root.clone()?;

// Ownership fast path serves client-pushed text and reads no disk, but the
// path is emitted as a goto target. Emit-safe containment admits a
// genuinely-absent in-root buffer (#361) and refuses out-of-root with no probe.
if self.external_php_text.get(path) == Some(&ExternalPhpText::PushedByClient) {
    if !path_within_root_emit_safe(path, &root) {
        return None;
    }
    if let Some(file) = self.files.get(path).copied() {
        return Some(file);
    }
}

// Disk path reads real bytes, so containment must be proven.
let real = canonical_within_root_registration(path, &root)?;

Three notes on that shape:

1. let root = self.config_root.clone()?; as the first statement satisfies AC bullet 1's root-unknown short-circuit literally. No self.files access happens before it.
2. Use canonical_within_root_registration on the disk branch. Its doc names this exact case — a path minted from discovered source data and then read.
3. Read through real, not path, for the metadata and read_to_string calls. The guard returns the verified canonical path so the caller cannot canonicalize a second time and read a target the guard never approved. Keep path as the key for self.files and external_php_text, so the caller lookups still resolve.

AC amendments

• Bullet 2 — waived, as scoped above. The intent of bullet 2 is that no weaker guard substitutes for the read guard. The read branch still takes the full fail-closed canonical_within_root_registration. The emit-safe guard is an addition to a branch the read guard never covered and cannot cover without regressing Component-member navigation: follow-ups from #335 #361. Bullet 2 already asks the implementer to pick between two guards and justify the choice in a comment, so a per-branch split with the same written rationale is within its spirit.
• Bullets 1 and 3 — your correction is right. external_php_mtimes does not exist. The field is external_php_text: HashMap<PathBuf, ExternalPhpText> at salsa_impl.rs:8777. Read the criteria against external_php_text.
• Bullet 1 ordering — amended. The ownership fast path may run before the disk guard, provided the fast path is itself gated by path_within_root_emit_safe. The requirement that survives is the one Lestrade's verification note describes: a rejected path causes zero filesystem touch and zero mutation, and never escapes the function.

@mikebronner — flagging the bullet 2 waiver for the record. Override me if you want the AC held literally.

Tests I expect on top of the AC list

Add two cases the current bullets do not reach:

• A client-pushed out-of-root path, present in self.files, returns None. This must fail if the emit-safe guard is removed.
• A client-pushed in-root path whose file is absent from disk still returns the pushed text. This pins Component-member navigation: follow-ups from #335 #361 against the guard.