Repository navigation
Conversation
…#298) WorkspaceRootResolver::isDescendant() was a plain string-prefix test over normalize()'d paths, which never resolves ".." segments or symlinks. A document path containing ".." (e.g. "$workspace/../elsewhere/proj/x.php") textually re-creates the workspace's own prefix and was treated as "inside" it, and a symlink inside the workspace whose real target sits outside it was treated the same way -- both the same containment bypass #296 fixed, reached through a different path spelling, reproducing even with no workspaceFolders declared at all. Both sides of isDescendant()'s comparison are now canonicalised via realpath() first, falling back to a purely lexical "."/".." collapse when realpath() returns false for a path that does not yet exist on disk (a directory the walk-up loop has not reached yet, a synthetic path in a unit test, or a Windows drive-letter path exercised on a non-Windows runner). Adds a ".."-bearing regression test paired with the existing #296 test, plus a symlinked-subdirectory case (skipped if symlink() is unavailable in the test environment). This changes which project root a warm worker boots against, so per CLAUDE.md it needs an independent end-to-end check (warm vs cold) before merge, same as #296/#297. Co-Authored-By: Claude Sonnet 5 <[email protected]>
… drive-letter dot-dot Two further gaps found by the self-review spawns on the first #298 commit, both in WorkspaceRootResolver, both fixed before merge: 1. canonicalize() fell straight to a purely lexical "."/".." collapse of the WHOLE path the moment realpath() failed on it. That mixed two canonicalisation strategies across the two sides of isDescendant()'s comparison whenever only one side existed on disk yet: a workspace root (which almost always exists, and is routinely itself behind a symlink -- macOS resolves sys_get_temp_dir() under /tmp, itself a symlink to /private/tmp) got realpath()'d through the symlink, while a brand-new/unsaved document's own directory (which often does not exist yet) did not -- so the two no longer shared a prefix even though one was genuinely inside the other, and resolve() silently fell back to the wrong, too shallow root for an entirely ordinary document. Fixed by resolving the longest EXISTING ancestor through realpath() first (new resolveExistingAncestor()), and only running the lexical collapse over the non-existent tail. 2. collapseDotSegments() decided "is this path absolute" solely by a leading "/", so a Windows-style drive-letter path ("C:/...") was treated as relative -- a leading ".." could pop the drive letter itself off the stack like any ordinary segment. Fixed by protecting the drive letter the same way a POSIX leading "/" is already protected. Both pinned with a new regression test, confirmed red against the first commit and green with the fix. Full tests/Unit/Lsp suite: 133/133, 3 skipped by design. Co-Authored-By: Claude Sonnet 5 <[email protected]>
… rooted paths only Second, lightweight review round over the previous follow-up commit (fix_commit_scope.py flagged needs-second-pass: 3 files touched) found one more real, newly-introduced gap: resolveExistingAncestor() probes shorter prefixes of a path through realpath() looking for the longest existing one -- correct and bounded for an absolute path (a POSIX "/" or a Windows drive letter floors the walk at the root segment), but a genuinely relative path has no such floor, so an ordinary segment name that happens to coincide with a real directory relative to the SERVER PROCESS's own cwd (not the workspace being resolved) would get silently canonicalised against that unrelated directory. Production document paths are always absolute (derived from a file:// URI), so this is not reachable from real LSP traffic -- but it is a genuine, previously-absent surface this follow-up itself introduced (the pre-follow-up lexical-only fallback never touched the filesystem once realpath() had already failed), and it is cheap to close at the same chokepoint: canonicalize() now only runs the ancestor-walk for a path it recognises as rooted, and falls straight to the lexical collapse for anything else, which is exactly the old, already-reviewed-safe behaviour for that case. Pinned with a new regression test (confirmed red against the prior commit, green with the fix). Full test suite: 556 tests green (ran by the review spawn), tests/Unit/Lsp: 134/134, 3 skipped by design. Co-Authored-By: Claude Sonnet 5 <[email protected]>
Independent E2E check (2 of 2) against head
|
| scenario | e79c1eb (this PR) |
56300c9 (red control) |
|---|---|---|
.. vector: W/sub/../../elsewhere/D/doc.php |
refused: "path is outside the configured working directory", 0 code actions, SENTINEL_D absent, no .rector-warm-* ever seen in D, doc.php unchanged |
D's config used: SimplifyIfReturnBool diagnostic + 2 code actions, SENTINEL_D created, .rector-warm-<pid> seen in D |
symlink vector: W/sub/linked/doc.php |
refused (same message), 0 code actions, SENTINEL_D absent, no .rector-warm-* in D, doc.php unchanged |
D's config used, SENTINEL_D created, temp dir seen in D |
positive control: W/src/Inside.php (no tricks) |
diagnostic + 2 actions, SENTINEL_W fires (SENTINEL_D absent) | same |
positive control: D's own sentinel and temp-dir watcher are live (workspaceFolders=[D], open D/doc.php) |
SENTINEL_D fires, .rector-warm-<pid> seen in D |
same |
Every server exited 0. No .rector-warm-* was left anywhere in any fixture tree.
Check 2 -- in-workspace stability when the workspace folder is behind a symlink: PASS (warm == cold, byte-identical)
I read canonicalize() / resolveExistingAncestor() at e79c1eb, and the 6a78b2a diff, before running this. Both $owner and $dir go through the same canonicalize(). For existing paths that means a full realpath() on both sides, so a symlinked workspace folder and a document under it land on the same real prefix. Observed:
Fixture: realW (rector.php withPaths([__DIR__.'/src']) + codeQuality set, src/Target.php with two fixable smells, and a vendor/autoload.php sentinel that records the cwd it was loaded from). W -> realW is a symlink. Flow: didOpen W/src/Target.php, then codeAction "Apply all Rector fixes", then the edits are applied client-side. The cold oracle is vendor/bin/rector process --config=rector.php -- src/Target.php run in realW on the same original file.
| variant | diagnostics | warm root (from sentinel cwd) | warm vs cold |
|---|---|---|---|
workspaceFolders=[W] (symlink), server cwd = an unrelated dir with no markers |
SimplifyIfReturnBool, RemoveAlwaysElse | realW |
byte-identical (sha256 a127a836...e1e91, both changed the file) |
workspaceFolders=[W] (symlink), server cwd = W |
same | realW |
byte-identical (same sha) |
macOS /tmp case: folder and document spelled /tmp/... (/tmp -> /private/tmp), server cwd unrelated |
same | realW |
byte-identical (same sha) |
The unrelated server cwd makes the check discriminating. If resolution had fallen back to the boot cwd, the containment check would have refused the document and produced no diagnostics. The same three variants on 56300c9 give the same diagnostics, the same root and the same sha, so ordinary symlinked-workspace use resolves to the same root as before #298.
Reasoned, not observed: the 6a78b2a case (an unsaved document whose directory does not exist yet) is not exercised end to end. The buffer path refuses a non-existent directory (realpath false) whatever root is chosen, so it cannot produce a write. Windows and Linux were not run.
Verdict: Check 1 PASS, Check 2 PASS. No merge-blocking finding from this check.
Independent E2E checks at e79c1eb (requested before merge, per #296/#297's merge-gate oversight)Built fresh fixtures in throwaway worktrees (not reusing any other agent's worktree); both cleaned up afterward. Check 1 -- sentinel test extended to canonicalisation (
|
1 participant
Summary
#296/#297 fixed the plain-path containment bypass in
WorkspaceRootResolver-- a marker directory outside every workspace folder hijacking root resolution. This issue is the direct sequel: the same bypass survives through a path spelling the earlier fix never canonicalised -- a document path containing..that textually re-creates the workspace's own prefix, or one reached through a symlink pointing outside the workspace. Reproducible with noworkspaceFoldersdeclared at all.isDescendant()canonicalises both sides (via realpath, falling back to a lexical..-collapse or an existing-ancestor walk for a not-yet-existing path) before the containment prefix check.Three commits: the primary fix, then two review follow-ups fixing real regressions the lane's own self-review rounds found (an asymmetric realpath fallback that could silently resolve to the wrong root for an ordinary unsaved document, a Windows drive-letter mishandling in the new dot-segment collapse, and a relative-path filesystem-probing gap the first follow-up itself introduced). Full review history, tree-snapshot receipts and test evidence are in the accompanying report.
Merge gate
This diff changes which project root gets
chdir()'d into (WorkspaceRootResolver::resolve()'s return value isRootedDiagnosticsSourcePool::get()'s chokepoint), so per this repo's CLAUDE.md, "A pull request that can change what warm writes gets an independent end-to-end check before merge" applies -- the same gate #296/#297 needed. An independent E2E check (warm vs cold rector output, byte-for-byte) is required before merge; the lane's own unit tests are not enough.Test plan
..bypass and the symlink bypass, confirmed red against pre-fix code, green with the fixtests/Unit/Lspsuite green (134/134, 3 skipped by design)Closes #298