Repository navigation
Folio cursor-read guard: harden lexical normalize_path against symlink escape (path_within_root) - #132
Merged
mikebronner merged 2 commits intoJun 15, 2026
Conversation
…ard. Route folio_route_name_for_cursor's root-containment check through the existing canonicalize-based path_within_root instead of the lexical normalize_path prefix check. A symlink under the project root that resolves outside it now returns None before any disk read, closing the symlink-escape leg the lexical prefix check admitted. Updates the method's doc comment to state canonicalize-based containment so the defense-in-depth claim is accurate, and adds a discriminating #[cfg(unix)] test that fails against the old lexical guard and passes against path_within_root. Fixes: #116
mikebronner
marked this pull request as ready for review
June 15, 2026 06:09
There was a problem hiding this comment.
✅ Approved
Review Summary
- Reviewed PR #132 against issue #116's six acceptance criteria. The change swaps the lexical
normalize_path(...).starts_with(...)containment guard infolio_route_name_for_cursorfor the canonicalize-basedpath_within_root(file_path, &root), reuses that helper as-is (no new symlink logic), rewrites the doc/inline comments, and adds a#[cfg(unix)]test for the symlink-escape case. - Every acceptance criterion is met:
- AC#1 ✅ guard now calls
path_within_root(file_path, &root)(main.rs:4822). - AC#2 ✅
path_within_root(main.rs:18721) is reused untouched — not in the diff. - AC#3 ✅
under_root_symlink_to_outside_target_returns_noneseeds a live under-root symlink to an outside target, indexes the page under the link path, assertsNone. I verified it's genuinely discriminating: the link path is lexically inside root (the oldstarts_withguard would have admitted it), butcanonicalize()resolves to the outside target, so only the new guard rejects it. - AC#4 ✅
in_tree_page_still_resolvesandout_of_root_page_returns_none_without_disk_readare untouched. - AC#5 ✅ doc + inline comments now describe canonicalize-based containment.
- AC#6 ✅ CI green — LSP — test, fmt, clippy passed.
- AC#1 ✅ guard now calls
- Tests verified: discriminating coverage of the live symlink-escape, positive/negative controls intact, CI green.
This closes the actual exploitable leg (a live under-root symlink resolving outside root is now rejected, proven by the test) and strictly improves on the lexical-only status quo. Nicely minimal and exactly scoped.
Considered and dismissed (not blocking): the inline comment's "before any disk access" reads strictly-loose now that canonicalize() itself stats the disk — but in context "disk read/access" refers to the page-content read (document_or_disk_content), which still happens only after the guard, so the wording is accurate as intended.
📋 Non-blocking follow-ups
path_within_root's fallback arm is fail-open —main.rs:18724(_ => path.starts_with(root)). Whencanonicalize()fails (a dangling under-root symlink, target absent), it reverts to the lexical prefix check and admits the path. Verified the mechanism is real, but the security impact is a defense-in-depth gap, not an exploit: a dangling symlink read returnsNoneharmlessly, and even in a tight materialize-after-check TOCTOU no out-of-tree content is surfaced — the return value is a pre-indexed route name, never file bytes. Deliberately out of scope for #116 (AC#2 mandated reusingpath_within_rootas-is, and it's shared with #55'slocate_view_file). Tracking separately.
Ready for @mikebronner to merge.
Closed
6 tasks
mikebronner
deleted the
fix/116-folio-cursor-read-guard-harden-lexical-normalizepa
branch
June 15, 2026 11:37
Merged
10 tasks done
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 #116 — hardens the Folio cursor-read containment guard against the symlink-escape class (#55) that the lexical check left open.
folio_route_name_for_cursorpreviously gated its pre-read root-containment check with the lexicalnormalize_path(file_path).starts_with(normalize_path(&root)). A symlink under the project root pointing outside it passes that purely textual check while resolving to an out-of-tree file. This routes the read site through the existing canonicalize-basedpath_within_roothelper, closing the symlink leg and making the doc comment's "defense in depth" claim accurate.Changes
folio_route_name_for_cursor(laravel-lsp/src/main.rs): the containment guard now callspath_within_root(file_path, &root)instead of the lexicalnormalize_pathprefix check. No new symlink-resolution logic — reuses the existing#55helper as-is.#[cfg(unix)] #[tokio::test] under_root_symlink_to_outside_target_returns_noneinlaravel-lsp/src/tests/folio_cursor_containment.rs: seeds an under-root symlink (std::os::unix::fs::symlink) pointing outside the root, indexes the page under its in-root link path, and assertsfolio_route_name_for_cursorreturnsNone. Verified discriminating — it fails against the old lexical guard and passes againstpath_within_root.Acceptance Criteria
folio_route_name_for_cursorreplaces thenormalize_path(...).starts_with(...)guard with a call topath_within_root(file_path, &root)path_within_rootas-is — no new symlink-resolution logic#[tokio::test]seeds an under-root symlink pointing outside the root, assertsNone, and is discriminating (target exists and is indexed, so only the canonicalize guard can reject it)in_tree_page_still_resolvesandout_of_root_page_returns_none_without_disk_readcontrols remain greenpath_within_root) so the "defense in depth" wording is accuratecargo test,cargo fmt --check, andcargo clippypass cleanTest Plan
cargo fmt --check— cleancargo clippy --all-targets— clean (no lints)laravel_lsplib (1735) +mainbin (279), including the 3folio_cursor_containmenttestspath_within_roottest-projectfixture (.env +composer update) and runs the full suite onubuntu-latest(Unix), where the#[cfg(unix)]test runsFixes #116