Repository navigation
Internalize the root-containment invariant into locate_slot_in_view so callers can't reopen the traversal hole - #154
Merged
mikebronner merged 2 commits intoJun 15, 2026
Conversation
locate_slot_in_view was pub and read from disk with no containment check, so the safety invariant lived entirely at the single create_slot_location call site (#143). A future caller that forgot the pre-check would silently reopen the out-of-root read that #130 closed. Extend the signature with root: &Path, check path_within_root before read_to_string, and return None for out-of-root paths — no disk access occurs. The local helper mirrors main.rs::path_within_root (canonicalize both sides, textual fallback); the library crate can't call the binary's private fn, and the repo already duplicates this logic (main.rs + salsa_impl.rs). The existing call-site guard stays: it still prevents building a LocationLink to an out-of-root path (the unwrap_or((0,0)) fallback would otherwise leak it). This internal check is defense-in-depth so the invariant holds regardless of how many call sites exist. Add unit tests for the out-of-root (file exists on disk) and in-root cases. Fixes #149
mikebronner
marked this pull request as ready for review
June 15, 2026 19:38
There was a problem hiding this comment.
✅ Approved
Review Summary
- Internalizes the project-root containment invariant into
locate_slot_in_view: the newroot: &Pathparam drives an earlypath_within_rootguard that short-circuits toNonebeforestd::fs::read_to_string(slot_navigation.rs:274-277), so an out-of-root view never hits the disk. The single call site increate_slot_locationis updated to pass&config.root(main.rs:13708). - Every acceptance criterion is met (7/7). Spot-checked the test arithmetic:
VIEW_WITH_SLOTputs$titleat(1, 7)— 4 spaces +{{= 7 columns — matching the assertedSome((1, 7)). The out-of-root test writes the file to disk first, so itsNonecan only come from the guard, not a missing file. Honest tests. - CI green: LSP test/fmt/clippy, Extension wasm/fmt/clippy, and both CodeQL analyses all pass.
- Bonus: an integration test (
slot_navigation_containment.rs::under_root_symlink_to_outside_target_returns_none) covers the symlink-escape case via the canonicalize-based(Ok, Ok)arm — the discriminating test a purely-lexical guard would fail.
What's Good
- The new internal guard is real defense-in-depth, not a replacement for the outer call-site check: that outer guard is still load-bearing because
.unwrap_or((0, 0))would otherwise emit aLocationLinkto an out-of-root URI. Keeping it is correct, and the PR body says so explicitly. - The local
path_within_rootfaithfully mirrors the mergedmain.rstwin (canonicalize both sides + lexical fallback), exactly as the AC required. - Doc comments cite the originating issues (#55, #130, #149) and explain why the invariant is internalized — future-caller-proofing.
I ran the security concerns about the _ => view_path.starts_with(root) fallback (symlink escape, TOCTOU, comment wording) through adversarial verification. All refuted: the symlink-escape window is not constructible — canonicalizing view_path = <root>/x must validate <root>, so root.canonicalize() can't fail while view_path.canonicalize() succeeds; and in the genuine fallback arm the path doesn't resolve, so read_to_string fails and yields None — no out-of-root content is ever read. The TOCTOU and fallback-wording are exact mirrors of the established main.rs pattern (repo convention), and the fail-closed hardening is already separately tracked in #134.
📋 Non-blocking follow-ups
- Consolidate the now-triplicated
path_within_rootcontainment logic —main.rs:18847,slot_navigation.rs:287(this PR), and the inline copy insalsa_impl.rs:2950-2953— into one shared helper. —laravel-lsp/src/slot_navigation.rs:287— A security-critical canonicalization check copied across three sites can drift; any future hardening (e.g. the fail-closed work in #134) would otherwise have to be applied three times. The duplication here was AC-sanctioned ("mirrormain.rs") and outside this PR's remit, so it's a follow-up, not a blocker.
Ready for @mikebronner to merge.
12 tasks
mikebronner
deleted the
fix/149-internalize-the-root-containment-invariant-into-lo
branch
June 15, 2026 20:36
7 tasks
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Implements #149. Internalizes the project-root containment invariant into
locate_slot_in_viewso the safety check no longer depends on every caller remembering to pre-guard.Follow-up from Holmes's review of #130 (PR #143): #143 closed the slot-navigation traversal hole with a
path_within_rootguard at the singlecreate_slot_locationcall site, butlocate_slot_in_viewitself waspuband read from disk with no internal check. A future caller that forgot the pre-check would silently reopen the same out-of-root read.Changes
locate_slot_in_viewsignature extended with aroot: &Pathparameter.path_within_root(view_path, root)and returnsNoneimmediately for out-of-root paths — noread_to_stringoccurs for an out-of-root view.main.rs::path_within_root(canonicalize both sides, textual-prefix fallback). Thelaravel_lsplibrary crate can't reach the binary's privatefn, and the repo already duplicates this logic (main.rs + an inline copy in salsa_impl.rs), so a local helper is the minimal faithful implementation. A crate-wide extraction would be scope creep beyond this defense-in-depth issue.create_slot_locationcall site passes&config.rootas the third argument.LocationLinkto an out-of-root path (theunwrap_or((0, 0))fallback would otherwise leak it). The new internal check is defense-in-depth.Acceptance Criteria
locate_slot_in_viewsignature extended to accept aroot: &Pathparameterlocate_slot_in_viewcallspath_within_root(view_path, root)beforestd::fs::read_to_stringand returnsNoneimmediately for out-of-root paths — no disk access occurscreate_slot_location(main.rs) passes&config.rootas the third argument; compiles without the old two-argument formlocate_slot_in_viewreturnsNonefor a path outside root (even though the file exists on disk)locate_slot_in_viewreturns the correctSome((line, col))for a valid in-root path containing the named slot variableslot_navigationtests remain greencargo fmt --checkpasses andcargo clippy --all-targetsemits no new warningsTest Plan
cargo test slot_navigation— 20 unit tests (incl. 2 new) + 3slot_navigation_containmentintegration tests passcargo fmt --checkcleancargo clippy --all-targets— no new warningsFixes #149