Repository navigation
Gate discovered paths in follow_links(true) directory walks against the project root - #229
Merged
mikebronner merged 2 commits intoJun 18, 2026
Conversation
… the project root. scan_dir walks with WalkDir::follow_links(true), so a symlink inside an in-root directory whose target escapes the project root was still followed and its files emitted as completion candidates — the discovered-path leg PR #227 (issue #226) deferred. Add path_within_root_walk_entry, a fail-closed canonicalize-based gate mirroring path_within_root, and filter every entry scan_dir emits through it. Audit the five follow_links(true) walk sites in main.rs: gate controllers_dir (it read_to_string's each discovered path — an out-of-root read primitive); document view_path, package_path, livewire_path and base_dir, whose discovered paths become display/relative completion strings only and never reach an FS primitive at the site (navigation resolves by name through independently containment-gated resolvers). Fixes: #228 Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
mikebronner
marked this pull request as ready for review
June 18, 2026 19:40
There was a problem hiding this comment.
✅ Approved
Review Summary
- Reviewed PR #229: adds
path_within_root_walk_entry(a fail-closed walk-entry gate) topath_containment.rs, wires it intoscan_dir+ all callers (scan_anonymous_dir,scan_class_dir,collect_flux_components), gates thecontrollers_dirwalk inmain.rs, removes the deferred-scope comment, and audits the four remainingfollow_links(true)sites with per-site "no gate needed" justifications. - All 7 acceptance criteria met (7/7). Notable deliberate divergence on AC #1: the gate accepts
&Pathrather than awalkdir::DirEntry, keepingpath_containmentfree of awalkdirdependency. Nothing the criterion cared about is dropped — it still mirrorspath_within_root's fail-closed semantics exactly — and it's the cleaner module boundary. - Containment logic is sound:
canonical_containmentcanonicalizes both sides and uses component-wisePath::starts_with(immune to the/root-siblingprefix attack); every error path (canonicalize failure, vanished root, non-canonical root) refuses — fail-closed. The gate is applied to the discovered entry, whichcanonicalize()resolves through the symlink to its real target, so it gates the right path. - Tests verified: the
#[cfg(unix)]negative cases place a real under-root symlink whose target lives outside the root, run the real walk, and assert the escaping file is dropped — each with the required precondition that the target exists outside root, plus an in-root sentinel proving the walk actually ran. Positive controls confirm in-root symlinks/subdirs still yield candidates (no over-refusal). New tests registered intests/mod.rs. CI green (LSP test/fmt/clippy).
Verification notes
- I checked a flagged "out-of-root absolute path disclosure" at the
package_pathwalk (main.rs:~12497): refuted.display_pathis only built insideif let Ok(relative) = path.strip_prefix(package_path), and WalkDir'sfollow_links(true)yields the nominal traversal path (<package_path>/<symlink>/<file>), not the resolved target — so the displayed string is always in-root and no out-of-root path leaks. The "no gate needed" audit comment at this site is correct. - The canonicalize-then-read TOCTOU window at the
controllers_dirgate is the lineage's established, accepted pattern (every guard shares it) and is explicitly within the documented threat model — not a defect, and a strict improvement over the prior ungated read.
What's good
- Clean per-site audit discipline: each of the five
follow_links(true)sites is either gated (with a read primitive) or documented with a concrete reason why discovered paths never reach an FS primitive. That's exactly the invariant-class closure #228 asked for. - Honest, non-tautological tests — they exercise the real walk and prove the gate fired (target-exists precondition + in-root sentinel), not hand-rolled path math.
📋 Non-blocking follow-ups
- The
controllers_dirwalk inmain.rs(~10994–11008) is a confirmed gated site with a real out-of-root read primitive (read_to_string), but has no dedicated per-site integration test — only the gate function is unit-tested and onlyscan_diris integration-tested. The lineage convention is a test per gated site; AC #5/#6's "at minimumscan_dir" wording lets this pass, but a#[cfg(unix)]negative+positive test mirroringscan_dir_containment.rsfor the controllers walk would close it. —laravel-lsp/src/main.rs:11005— keeps the lineage's "tested at every gated site" invariant genuinely true. (Tracked as a follow-up issue.)
Ready for @mikebronner to merge.
3 tasks
mikebronner
deleted the
fix/228-gate-discovered-paths-follow-links-symlink-escape-containment
branch
June 18, 2026 20:11
mikebronner
added a commit
that referenced
this pull request
Jun 18, 2026
…s gate Cover the discovered-path containment gate on the `controllers_dir` `follow_links(true)` walk in `check_controller_view_variable` (main.rs) — the confirmed gated site PR #229 introduced but left without a dedicated end-to-end test (issue #230, Holmes's review follow-up). Mirrors tests/scan_dir_containment.rs: - #[cfg(unix)] negative: a controller reached through an under-root symlink whose target escapes the project root is NOT read, with precondition assertions that the target resolves outside root plus an in-root sentinel proving the walk still runs. - #[cfg(unix)] positive control: an in-root symlink (target inside root) is still read — the gate does not over-refuse. - positive control: an ordinary nested in-root subdir controller is still read. Verified discriminating: neutralizing the gate fails the negative case. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
8 tasks done
mikebronner
added a commit
that referenced
this pull request
Jun 18, 2026
…true) gate (#231) * chore: start work on #230 * test: ✅ add per-site containment test for controllers_dir follow_links gate Cover the discovered-path containment gate on the `controllers_dir` `follow_links(true)` walk in `check_controller_view_variable` (main.rs) — the confirmed gated site PR #229 introduced but left without a dedicated end-to-end test (issue #230, Holmes's review follow-up). Mirrors tests/scan_dir_containment.rs: - #[cfg(unix)] negative: a controller reached through an under-root symlink whose target escapes the project root is NOT read, with precondition assertions that the target resolves outside root plus an in-root sentinel proving the walk still runs. - #[cfg(unix)] positive control: an in-root symlink (target inside root) is still read — the gate does not over-refuse. - positive control: an ordinary nested in-root subdir controller is still read. Verified discriminating: neutralizing the gate fails the negative case. Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> --------- Co-authored-by: Claude Opus 4.8 <noreply@anthropic.com>
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 #228 — closes the discovered-path leg of the
path_within_rootcontainment lineage (#130 → … → #226). PR #227 gated the walk root ofscan_dirbut deferred gating the paths discovered underWalkDir::follow_links(true): a symlink encountered inside an in-root directory whose target escapes the project root was still followed and its files emitted as candidates / read.Changes
path_within_root_walk_entry(path, root)inpath_containment.rs— fail-closed, delegates topath_within_root(canonicalize-both semantics); takes&Pathso the module keeps nowalkdircoupling. Documented as a fourth named entry point alongside the existing three.scan_dir(component_completion.rs) gains a distinctroot: &Pathcontainment param (separate fromdisplay_root) and filters every discovered entry through the gate after the cheap type/extension filters, before emitting it. Callers updated:scan_anonymous_dir,scan_class_dir,collect_flux_components, the sixget_all_blade_componentscall sites inmain.rs, and the existing unit tests.main.rsfollow_links(true)audit (per-site):controllers_dir— gated: itread_to_strings each discovered path (an out-of-root read primitive).view_path,package_path,livewire_path,base_dir— documented, gate not needed: discovered paths become display/relative completion strings only, never read/opened or resolved to an FS primitive at the site (navigation resolves by name through independently containment-gated resolvers).Acceptance Criteria
path_containment.rs(path_within_root_walk_entry), fail-closed, mirroringpath_within_root.scan_diraccepts the project root and filters each discovered entry through the gate; all callers updated to supply the root.component_completion.rsis removed once the gate is applied.follow_links(true)walk site inmain.rsaudited per-site — gated where discovered paths are read primitives (controllers_dir), documented where they don't reach an FS primitive (the four completion walks).#[cfg(unix)]negative-case test: an under-root symlink whose target exists outside the root → escaping file NOT emitted, with a precondition assertion the target resolves outside the root.laravel-lsp/src/tests/scan_dir_containment.rsper the lineage convention; gate logic also unit-tested directly inpath_containment.rs.Test Plan
cargo test— lib (1898) and binary/src/tests(437) suites green, incl. 4 newscan_dir_containmentintegration tests + 4 newwalk_entrygate unit tests.cargo fmt --checkclean.cargo clippy --all-targets -- -D warningsclean (matches CI).tests/integration_tests.rsfailures seen locally are environmental only (gitignoredtest-project/.envandvendor/); CI bootstraps both (cp .env.example,composer update) before testing.Fixes #228