Consolidate the triplicated path_within_root containment helper into one shared module - #161
Conversation
|
Resume spec for the scheduled pipeline — Mike approved Option 3; this draft PR is the work surface (resume it, don't open a new one). Implement against the item's amended acceptance criteria: Design — one module, two entry points:
Critical:
Bundled (Mike's call, unrelated area): add two tests to Once green, flip to ready-for-review; Holmes reviews, Mike merges. (Note: closing #156 also unblocks #145, which is queued behind it.) |
…module. The canonical-first root-containment check lived in three copies with three divergent fallbacks: main.rs (fail-closed, #155), slot_navigation.rs (raw-textual), and an inline `retain` in salsa_impl.rs (lexical, for speculative candidates). Three copies can drift, and any future hardening had to land in all three and stay in sync. Extract one `path_containment` module with a shared canonical-first core and two public entry points: - `path_within_root` — fail-closed; the security guard, used by main.rs (no behavior change) and slot_navigation.rs (upgrade: raw-textual → fail-closed). - `path_within_root_lexical` — normalize_path lexical fallback that admits not-yet-created candidates, used by salsa_impl.rs's component-path filter (which must not fail-close). All main.rs call-sites (including the one #157 added in collect_route_declaration_targets) now route through the shared guard. Unit tests cover in-root, sibling-root, interior-`..` escape, and the fail-closed dangling-under-root-symlink leg. Fixes: #156
…eredoc legs. Two already-working but uncovered paths in `find_block_terminator`'s @php-block masking: a literal @endphp inside a `/* … */` block comment, and inside a double-quoted-label heredoc (`<<<"LABEL"`). Existing tests only reach the `//` line-comment, bare-label heredoc, and nowdoc legs. Bundled from #93/#164 (unrelated area, folded into #156's PR by request).
3bcfef3 to
4e736d6
Compare
There was a problem hiding this comment.
✅ Approved
Review Summary
- Consolidates the triplicated
path_within_rootcontainment guard into one sharedlaravel-lsp/src/path_containment.rsmodule behind a private canonical-first core (canonical_containment) and two public entry points — exactly as the contract requires. - Every acceptance criterion is met:
- ✅ New
path_containment.rscreated and declaredpub mod path_containment;(lib.rs:58). - ✅ Fail-closed
path_within_root(path_containment.rs:51,unwrap_or(false)) and lexicalpath_within_root_lexical(:63,unwrap_or_else(|| normalize_path(path).starts_with(root))). - ✅ Private
fn path_within_rootremoved frommain.rs; all call-sites resolve to the shared import (main.rs:30) — verified by grep acrossmain.rs(4843, 4874, 13927, 18958, 19515, 21759), including thecollect_route_declaration_targetssite PR #157 added. Only one production definition survives. - ✅ Private fn removed from
slot_navigation.rs; caller now uses the shared fail-closed guard (:278) — a genuine security upgrade over the old raw-textual fallback. - ✅
salsa_impl.rs's inlineretainswapped forpath_within_root_lexical(:2952), preserving speculative-candidate admission (not fail-closed). - ✅ Unit tests cover all four named cases plus a fifth (speculative in-root admit): in-root⇒true, sibling⇒false, interior-
..-escape⇒false, dangling-symlink fail-closed⇒false (path_containment.rs:72–172). - ✅ Stale "Mirrors
path_within_rootin main.rs" doc-comments retired in bothslot_navigation.rsandsalsa_impl.rs. - ✅ Two bundled
blade_var_renametests added (block-comment and double-quoted-heredoc@endphp),tests.rs:947–980.
- ✅ New
- Tests verified: CI fully green (LSP test+fmt+clippy, extension wasm+fmt+clippy, CodeQL); lens reviewers independently ran
cargo test— 1850 tests pass, new tests included. The new tests exercise distinct legs and would fail on a logic reversion (not tautologies).
The security review confirmed the consolidation introduces no bypass: the lexical entry point is used only on the speculative-candidate path in salsa_impl.rs, never on a read/emit path, and normalize_path correctly defeats interior-.. escapes. Excellent, surgical work — well-documented and the slot-navigation leg is now stronger than before.
📋 Non-blocking follow-ups
- None.
Ready for @mikebronner to merge.
Summary
Implements #156 — consolidates the canonical-first root-containment check, which had drifted into three copies with three divergent fallbacks, into one shared
path_containmentmodule. Built to Option 3 (Mike's call): one module, two entry points sharing a canonical-first core, so the security guards keep their #155 fail-closed behavior whilesalsa_impl's speculative-candidate filter keeps its lexical fallback.Changes
laravel-lsp/src/path_containment.rs, declaredpub mod path_containment;inlib.rs, with a shared canonical-first core (canonical_containment) and two public entry points:path_within_root— fail-closed (canonicalize failure ⇒false; preserves harden: make path_within_root fail-closed for security-guard callers (dangling-symlink leg) #155).path_within_root_lexical—normalize_pathlexical fallback that admits not-yet-created candidates.fn path_within_rootfrommain.rs; all call-sites (including the one Rename path resolution: wire collect_route_declaration_targets' page-path read through path_within_root #157 added incollect_route_declaration_targets) route throughlaravel_lsp::path_containment::path_within_root.fn path_within_rootfromslot_navigation.rs; its caller now uses the shared guard — upgrade: raw-textual fallback → fail-closed.salsa_impl.rswithpath_within_root_lexical(preserves speculative-candidate admission — not fail-closed).path_within_rootin main.rs" doc-comments to referencepath_containment.find_block_terminatortests inblade_var_rename/tests.rs— a fake@endphpinside a/* … */block comment, and inside a double-quoted-label heredoc (<<<"LABEL").Acceptance Criteria
laravel-lsp/src/path_containment.rsmodule, declaredpub mod path_containment;inlib.rspub fn path_within_root(path, root) -> bool— fail-closed (preserves harden: make path_within_root fail-closed for security-guard callers (dangling-symlink leg) #155)pub fn path_within_root_lexical(path, root) -> bool—normalize_pathlexical fallbackfn path_within_rootinmain.rsremoved; all call-sites use the shared guard (incl. Rename path resolution: wire collect_route_declaration_targets' page-path read through path_within_root #157'scollect_route_declaration_targets)fn path_within_rootinslot_navigation.rsremoved; caller upgraded raw-textual → fail-closedsalsa_impl.rsinline check replaced withpath_within_root_lexical(not fail-closed)cargo build --releasepasses — zero errors, zero new warningspath_containmentunit tests: in-root ⇒true; sibling-root ⇒false;..-escape via lexical ⇒false; fail-closed (dangling under-root) via the guard ⇒false(+ a lexical speculative-admit case)cargo test— lib suite 2179 green; the only integration failures are fixture-dependent, identical onmain, and bootstrapped by CI)path_within_root" doc-comments updated to point topath_containmentfind_block_terminatortests (block-comment@endphp; double-quoted heredoc@endphp)Test Plan
cargo build --release— cleancargo fmt --check— cleancargo clippy --all-targets -- -D warnings— cleancargo test --all-features— lib suite green (2179 tests), incl. all containment tests now routing through the shared module; newpath_containmenttests (5) and bundled blade tests (2) pass.env+ composervendor/), identical on cleanorigin/main; CI bootstraps bothFixes #156