Skip to content

Extend path_within_root containment guard to the component, directive, and view goto-definition flows - #192

Merged
mikebronner merged 2 commits into
mainfrom
fix/148-extend-path-within-root-guard-to-component-directive-view
Jun 16, 2026
Merged

mikebronner merged 2 commits into
mainfrom
fix/148-extend-path-within-root-guard-to-component-directive-view

Conversation

@mikebronner

@mikebronner mikebronner commented Jun 16, 2026 •

Copy link
Copy Markdown
Contributor

Summary

Implements #148

Extends the slot-navigation path_within_root containment guard from #130 (PR #143) to the remaining filesystem-touching goto-definition flows: view, component, and directive. A loadViewsFrom(__DIR__ . '/../../etc', 'ns')-style namespace can resolve an absolute path that escapes the project root; before this change these flows handed the LSP client a LocationLink pointing outside the root with only a file_exists_cached check. This is hardening / defense-in-depth — these flows surface a navigation target rather than reading out-of-root file content — bringing the containment invariant to every FS-touching goto-definition path.

Changes

  • create_view_location_from_salsa: guard inside the candidate loop, after file_exists_cached, before building the LocationLink, continue on failure.
  • resolve_component_existing_file: guard after file_exists_cached, before return Some(path). resolve_component_path already filters its candidates against the root, but component_candidate_paths appends class-backed (Blade::component('tag', Class::class)) and PSR-4 componentNamespace candidates past that filter — the guard covers those.
  • create_directive_location_from_salsa: guard inside every candidate loop — the two @component try-blocks, the view-directives-first-arg loop (@extends/@include/@includeIf/@includeUnless/@each), the @includeWhen loop, the @includeFirst loop, and the @livewire loop. The @feature branch builds its path from root.join(..) and is already contained, so it is left as-is.
  • All guards use config.root from the in-scope get_cached_config() result, consistent with the slot-navigation pattern; no new self.root_path read is introduced.
  • Three new test modules registered in tests/mod.rs.

Acceptance Criteria

  • path_within_root guard in create_view_location_from_salsa candidate loop (after existence check, before LocationLink, continue on failure)
  • path_within_root guard in resolve_component_existing_file (after existence check, before return Some(path), continue)
  • path_within_root guard in every candidate loop of create_directive_location_from_salsa (@component both-try blocks, view-first-arg, @includeWhen, @includeFirst, @livewire)
  • All guards use config.root; self.root_path not introduced as an additional read
  • View-flow containment test (view_navigation_containment.rs): out-of-root → None, in-root → Some, under-root symlink → None (Unix)
  • Component-flow containment test (component_navigation_containment.rs): out-of-root → None, in-root → Some (resolver + create_component_location_from_salsa), under-root symlink → None
  • Directive-flow containment test (directive_navigation_containment.rs): @include (view directive) and @livewire out-of-root → None, in-root → Some, under-root symlink → None
  • All three test modules registered in tests/mod.rs
  • Existing slot_navigation_containment, folio_cursor_containment, slot_variable_resolution tests remain green
  • cargo fmt --check clean, cargo clippy --all-targets clean

Test Plan

  • cargo fmt --check — clean
  • cargo clippy --all-targets — clean (no lints)
  • cargo test lib (1843) + bin (339) targets — all green, including the 20 containment tests (10 new: view×3, directive×4, component×3)
  • New tests cover out-of-root rejection, in-root resolution, and the discriminating under-root-symlink-to-outside case (Unix)

Note: 8 failures in the integration_tests target (environment, env_parsing, architecture::test_all_pattern_types_have_fixtures, routes::test_route_index_resolves_package_route, test_project_exists) are pre-existing — they panic on a missing .env test-project fixture and reproduce identically on the base commit (8ccc805) without this change. Unrelated to these guards.

Fixes #148

…to-definition.

Extend the #130 slot-navigation containment guard to the remaining
filesystem-touching goto-definition flows. A loadViewsFrom-style namespace
can resolve an absolute path that escapes the project root; these flows
previously handed the LSP client a LocationLink pointing outside the root
with only a file_exists_cached check.

Add a path_within_root(&path, &config.root) guard inside each candidate
loop, after the existence check and before building the link, with
continue on failure (mirrors the slot-navigation pattern):

- create_view_location_from_salsa
- resolve_component_existing_file, over the class-backed and PSR-4
  candidates component_candidate_paths appends past resolve_component_path's
  own root filter
- create_directive_location_from_salsa, every candidate loop (@component
  x2, view-first-arg, @includeWhen, @includeFirst, @livewire)

Cover each flow with out-of-root, in-root, and under-root-symlink
containment tests in the slot_navigation_containment.rs style.

Fixes: #148
@mikebronner
mikebronner marked this pull request as ready for review June 16, 2026 16:46

@mr-sherlock-holmes mr-sherlock-holmes Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ Approved

Review Summary

  • Reviewed PR #192 — extends the path_within_root(&path, &config.root) containment guard to the view, component, and directive goto-definition flows in laravel-lsp/src/main.rs, mirroring the slot-navigation guard from #130/#143.
  • All 10 acceptance criteria met. Guards sit in every candidate loop after file_exists_cached and before the LocationLink/return, with continue (not return None) so a later in-root candidate can still resolve:
    • create_view_location_from_salsa (main.rs:13821) ✅
    • resolve_component_existing_file (main.rs:13871) ✅
    • create_directive_location_from_salsa — all six loops: @component both try-blocks (14048, 14060), view-directives-first-arg (14077), @includeWhen (14094), @includeFirst (14111), @livewire (14134) ✅
    • Every guard reads config.root from the in-scope get_cached_config(); the pre-existing self.root_path read (13856) feeds only ComposerAutoload and is not introduced as a new guard read — AC4 satisfied as written.
    • The @feature branch is correctly exempt — it builds its path from root.join(..) and is already contained (comment at 14021).
  • Tests are honest and discriminating. All three new modules create the out-of-root target on disk before asserting None, so each test would fail if the guard were removed — not no-ops. Positive (in-root) cases assert the exact target_uri (not just is_some()), and each flow has a Unix-gated symlink-under-root→outside-target case that only the canonicalize-based path_within_root can reject. Real functions are exercised via LspService::new + inner() — no stubs. All three modules registered in tests/mod.rs.
  • Security: path_within_root canonicalizes both sides and uses component-wise starts_with — symlink-safe and not foolable by an adjacent-directory prefix. The escape vector this PR targets is closed across all three flows.
  • CI green — LSP test/fmt/clippy, extension wasm/fmt/clippy, and CodeQL all pass.

📋 Non-blocking follow-ups

  • The <livewire:ns::component> tag flow is still unguarded — create_livewire_location_from_salsa (main.rs:13981), reached via PatternAtPosition::Livewire (~20689), is not one of the loops this PR touched (that's the @livewire directive, which is guarded at 14134). It returns a LocationLink after only file_exists_cached, with no path_within_root. Since LivewireConfig.component_namespaces accepts bare absolute paths (livewire_config.rs:293), a <livewire:x::passwd> tag could hand the client an out-of-root navigation target — the same namespace-escape class this PR fixes elsewhere. Navigation-target escape only (defense-in-depth), not an active server-side read — the same tier the issue body assigns to the view/component/directive flows. The next sibling in the #130 → #148 chain; tracking as a follow-up, not blocking this PR.
  • (secondary, lower value) Diagnostic-validation loops at main.rs:15925 and 16603 call .exists() on resolve_view_path results with no containment check, so an out-of-root namespace causes stat probes outside the root. No client leak and no file read — folding into the same follow-up for tracking.

Ready for @mikebronner to merge.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Extend path_within_root containment guard to the component, directive, and view goto-definition flows

1 participant