From 34e5125866f22d6890c2ed981e06faaa6cd32d65 Mon Sep 17 00:00:00 2001 From: Florian DAVID Date: Mon, 5 Oct 2026 11:04:17 +0200 Subject: [PATCH 1/3] Fix: workspace root marker must canonicalise before containment check (#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 --- ...t-marker-dotdot-and-symlink-containment.md | 1 + src/Lsp/WorkspaceRootResolver.php | 59 +++++++++++++++++++ tests/Unit/Lsp/WorkspaceRootResolverTest.php | 46 +++++++++++++++ 3 files changed, 106 insertions(+) create mode 100644 changelog.d/298.fixed.workspace-root-marker-dotdot-and-symlink-containment.md diff --git a/changelog.d/298.fixed.workspace-root-marker-dotdot-and-symlink-containment.md b/changelog.d/298.fixed.workspace-root-marker-dotdot-and-symlink-containment.md new file mode 100644 index 0000000..d02e283 --- /dev/null +++ b/changelog.d/298.fixed.workspace-root-marker-dotdot-and-symlink-containment.md @@ -0,0 +1 @@ +- `WorkspaceRootResolver::isDescendant()` (#298, the direct sequel to #296) used to be a plain string-prefix test over unresolved paths, so a document path containing `..` (e.g. a URI like `file:////../elsewhere/proj/x.php`), or one reached through a symlink pointing outside the workspace, could still resolve to a marker directory outside the owning workspace folder -- the same containment bypass #296 fixed, reachable through a different path spelling, including with no `workspaceFolders` declared at all. Both sides of the containment comparison are now canonicalised (`realpath()`, falling back to a purely lexical `.`/`..` collapse when the path does not yet exist on disk) before the comparison runs, closing both the `..` case and the symlink case at the same chokepoint. diff --git a/src/Lsp/WorkspaceRootResolver.php b/src/Lsp/WorkspaceRootResolver.php index 999738f..d744910 100644 --- a/src/Lsp/WorkspaceRootResolver.php +++ b/src/Lsp/WorkspaceRootResolver.php @@ -114,9 +114,68 @@ private static function ownerFolder(string $documentPath, array $folders): ?stri private static function isDescendant(string $path, string $ancestor): bool { + // #298: a plain string-prefix test over normalize()'d paths is + // fooled by a ".." segment that textually re-creates the + // ancestor's own prefix (e.g. "$owner/../elsewhere" lexically + // starts with "$owner/") and by a symlink whose literal path sits + // under the ancestor while its real target does not -- both let a + // document outside the owning workspace folder resolve as if it + // were inside it. Canonicalise both sides first so the comparison + // is against where the path actually is, not how it is spelled. + $path = self::canonicalize($path); + $ancestor = self::canonicalize($ancestor); + return $path === $ancestor || \str_starts_with($path, $ancestor . '/'); } + private static function canonicalize(string $path): string + { + // realpath() resolves "..", ".", and symlinks against the real + // filesystem -- but it returns false for a path that does not + // (yet) exist on disk: a directory this walk-up loop is still + // about to create, a synthetic path a unit test builds without + // ever touching the filesystem, or a Windows-style drive-letter + // path exercised on a non-Windows runner that can never resolve. + // In every one of those cases there is nothing on disk to canonicalise + // against, so fall back to a purely lexical collapse of "." and + // ".." segments -- that still closes the ".." escape (it cannot + // close a symlink escape, since there is no filesystem to follow + // the symlink through, but a path realpath() cannot see is also a + // path with no symlink to follow). + $resolved = \realpath($path); + + return $resolved !== false ? self::normalize($resolved) : self::collapseDotSegments($path); + } + + private static function collapseDotSegments(string $path): string + { + $isAbsolute = \str_starts_with($path, '/'); + $stack = []; + + foreach (\explode('/', $path) as $segment) { + if ($segment === '' || $segment === '.') { + continue; + } + + if ($segment === '..') { + if ($stack !== [] && \end($stack) !== '..') { + \array_pop($stack); + } elseif (!$isAbsolute) { + $stack[] = '..'; + } + // ".." above an absolute root has nowhere left to go -- + // drop it rather than let the stack go negative. + continue; + } + + $stack[] = $segment; + } + + $result = ($isAbsolute ? '/' : '') . \implode('/', $stack); + + return $result === '' ? '/' : $result; + } + private static function normalize(string $path): string { $path = \str_replace('\\', '/', $path); diff --git a/tests/Unit/Lsp/WorkspaceRootResolverTest.php b/tests/Unit/Lsp/WorkspaceRootResolverTest.php index 8ad5bf5..201d42e 100644 --- a/tests/Unit/Lsp/WorkspaceRootResolverTest.php +++ b/tests/Unit/Lsp/WorkspaceRootResolverTest.php @@ -131,6 +131,52 @@ public function testIgnoresAMarkerInAnUnrelatedDirectoryOutsideEveryWorkspaceFol self::assertSame($workspace, WorkspaceRootResolver::resolve($file, [$workspace], $workspace)); } + public function testIgnoresAMarkerReachedThroughADotDotSegmentThatTextuallyStartsWithTheWorkspacePrefix(): void + { + // #298: isDescendant() used to be a plain string-prefix test, so a + // document path containing ".." that textually starts with the + // workspace's own prefix (e.g. "$workspace/../elsewhere/...") was + // treated as "inside" the workspace even though it lexically + // escapes it -- the same containment bypass #296 fixed, reached + // through a different path spelling. Pairs with + // testIgnoresAMarkerInAnUnrelatedDirectoryOutsideEveryWorkspaceFolder() + // above, which covers the plain (no "..") escape. + $workspace = $this->base . '/ws'; + mkdir($workspace, 0o700, true); + + $elsewhere = $this->base . '/elsewhere/proj'; + mkdir($elsewhere, 0o700, true); + touch($elsewhere . '/rector.php'); + $file = $workspace . '/../elsewhere/proj/x.php'; + touch($file); + + self::assertSame($workspace, WorkspaceRootResolver::resolve($file, [$workspace], $workspace)); + } + + public function testIgnoresAMarkerReachedThroughASymlinkThatLexicallySitsInsideTheWorkspace(): void + { + // #298: a symlink inside the workspace whose real target is + // outside it is the same containment bypass as the ".." case + // above, reached a different way -- the literal path is nominally + // under the workspace, but following it lands somewhere a + // workspace folder never authorised. + $workspace = $this->base . '/ws'; + mkdir($workspace, 0o700, true); + + $outside = $this->base . '/outside'; + mkdir($outside, 0o700, true); + touch($outside . '/rector.php'); + + if (!@symlink($outside, $workspace . '/link')) { + self::markTestSkipped('symlink() is not available in this test environment'); + } + + $file = $workspace . '/link/x.php'; + touch($file); + + self::assertSame($workspace, WorkspaceRootResolver::resolve($file, [$workspace], $workspace)); + } + public function testNeverWalksAboveTheOwningFolderEvenWhenAnAncestorHasAMarker(): void { // A rector.php sitting ABOVE the workspace folder (e.g. a monorepo From 6a78b2a3d03714812f3697b84f63626c200d3812 Mon Sep 17 00:00:00 2001 From: Florian DAVID Date: Mon, 5 Oct 2026 11:14:20 +0200 Subject: [PATCH 2/3] Fix(#298): self-review follow-up -- asymmetric realpath() and Windows 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 --- ...t-marker-dotdot-and-symlink-containment.md | 2 +- src/Lsp/WorkspaceRootResolver.php | 89 ++++++++++++++++--- tests/Unit/Lsp/WorkspaceRootResolverTest.php | 43 +++++++++ 3 files changed, 121 insertions(+), 13 deletions(-) diff --git a/changelog.d/298.fixed.workspace-root-marker-dotdot-and-symlink-containment.md b/changelog.d/298.fixed.workspace-root-marker-dotdot-and-symlink-containment.md index d02e283..7d4df68 100644 --- a/changelog.d/298.fixed.workspace-root-marker-dotdot-and-symlink-containment.md +++ b/changelog.d/298.fixed.workspace-root-marker-dotdot-and-symlink-containment.md @@ -1 +1 @@ -- `WorkspaceRootResolver::isDescendant()` (#298, the direct sequel to #296) used to be a plain string-prefix test over unresolved paths, so a document path containing `..` (e.g. a URI like `file:////../elsewhere/proj/x.php`), or one reached through a symlink pointing outside the workspace, could still resolve to a marker directory outside the owning workspace folder -- the same containment bypass #296 fixed, reachable through a different path spelling, including with no `workspaceFolders` declared at all. Both sides of the containment comparison are now canonicalised (`realpath()`, falling back to a purely lexical `.`/`..` collapse when the path does not yet exist on disk) before the comparison runs, closing both the `..` case and the symlink case at the same chokepoint. +- `WorkspaceRootResolver::isDescendant()` (#298, the direct sequel to #296) used to be a plain string-prefix test over unresolved paths, so a document path containing `..` (e.g. a URI like `file:////../elsewhere/proj/x.php`), or one reached through a symlink pointing outside the workspace, could still resolve to a marker directory outside the owning workspace folder -- the same containment bypass #296 fixed, reachable through a different path spelling, including with no `workspaceFolders` declared at all. Both sides of the containment comparison are now canonicalised (`realpath()`, falling back to a purely lexical `.`/`..` collapse when the path does not yet exist on disk) before the comparison runs, closing both the `..` case and the symlink case at the same chokepoint. Self-review on the fix found and closed two further gaps before merge: a document whose own directory does not exist yet (an unsaved/new file) was compared asymmetrically against a workspace root that does exist and is itself often behind a symlink (e.g. macOS resolving `/tmp` to `/private/tmp`), which could make `resolve()` settle for the wrong, too-shallow root even for an entirely ordinary document -- fixed by resolving the longest existing ancestor through `realpath()` first and only running the lexical collapse over the non-existent tail; and the lexical fallback mishandled a Windows-style drive-letter path (`C:/...`) once it contained `..`, treating the drive letter as an ordinary, poppable path segment instead of a protected root. diff --git a/src/Lsp/WorkspaceRootResolver.php b/src/Lsp/WorkspaceRootResolver.php index d744910..3f29712 100644 --- a/src/Lsp/WorkspaceRootResolver.php +++ b/src/Lsp/WorkspaceRootResolver.php @@ -136,23 +136,82 @@ private static function canonicalize(string $path): string // about to create, a synthetic path a unit test builds without // ever touching the filesystem, or a Windows-style drive-letter // path exercised on a non-Windows runner that can never resolve. - // In every one of those cases there is nothing on disk to canonicalise - // against, so fall back to a purely lexical collapse of "." and - // ".." segments -- that still closes the ".." escape (it cannot - // close a symlink escape, since there is no filesystem to follow - // the symlink through, but a path realpath() cannot see is also a - // path with no symlink to follow). $resolved = \realpath($path); + if ($resolved !== false) { + return self::normalize($resolved); + } - return $resolved !== false ? self::normalize($resolved) : self::collapseDotSegments($path); + // #298 follow-up (review finding): falling straight to a purely + // lexical collapse of the WHOLE path the moment realpath() fails + // on it mixed two different canonicalisation strategies across + // the two sides of isDescendant()'s comparison whenever only one + // side happened to exist on disk yet -- the 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. Resolving the longest EXISTING ancestor through + // realpath() first, and only running the lexical collapse over + // the non-existent tail, keeps both sides on the same symlink + // resolution the moment any shared ancestor exists on disk. + return self::collapseDotSegments(self::resolveExistingAncestor(self::normalize($path))); + } + + private static function resolveExistingAncestor(string $normalized): string + { + $isAbsolute = \str_starts_with($normalized, '/'); + $segments = \explode('/', $normalized); + $keep = \count($segments); + $floor = $isAbsolute ? 1 : 0; + + while ($keep > $floor) { + $keep--; + $prefix = $isAbsolute && $keep === 1 ? '/' : \implode('/', \array_slice($segments, 0, $keep)); + + if ($prefix === '') { + continue; + } + + $resolved = \realpath($prefix); + if ($resolved === false) { + continue; + } + + $tail = \implode('/', \array_slice($segments, $keep)); + + return $tail === '' ? self::normalize($resolved) : self::normalize($resolved) . '/' . $tail; + } + + // No ancestor of this path exists on disk at all (every segment + // was synthetic, or the path is a Windows-style drive-letter + // path on a non-Windows runner, which can never resolve) -- + // nothing left to canonicalise against the filesystem. + return $normalized; } private static function collapseDotSegments(string $path): string { - $isAbsolute = \str_starts_with($path, '/'); + // A Windows-style drive-letter path ("C:/...", "C:") has no + // leading "/", so it would otherwise be treated as relative and + // its drive segment popped by a leading ".." like any ordinary + // path component. Split it off first and protect it exactly the + // way a POSIX leading "/" is already protected below, then + // re-attach it to the result. + $drive = ''; + $rest = $path; + if (\preg_match('/^([A-Za-z]:)(\/.*)?$/', $path, $m) === 1) { + $drive = $m[1]; + $rest = $m[2] ?? ''; + $rest = $rest === '' ? '/' : $rest; + } + + $isAbsolute = $drive !== '' || \str_starts_with($rest, '/'); $stack = []; - foreach (\explode('/', $path) as $segment) { + foreach (\explode('/', $rest) as $segment) { if ($segment === '' || $segment === '.') { continue; } @@ -163,8 +222,9 @@ private static function collapseDotSegments(string $path): string } elseif (!$isAbsolute) { $stack[] = '..'; } - // ".." above an absolute root has nowhere left to go -- - // drop it rather than let the stack go negative. + // ".." above an absolute root (POSIX "/" or a drive + // letter) has nowhere left to go -- drop it rather than + // let the stack go negative or pop the drive itself. continue; } @@ -172,8 +232,13 @@ private static function collapseDotSegments(string $path): string } $result = ($isAbsolute ? '/' : '') . \implode('/', $stack); + $result = $result === '' ? '/' : $result; + + if ($drive === '') { + return $result; + } - return $result === '' ? '/' : $result; + return $result === '/' ? $drive : $drive . $result; } private static function normalize(string $path): string diff --git a/tests/Unit/Lsp/WorkspaceRootResolverTest.php b/tests/Unit/Lsp/WorkspaceRootResolverTest.php index 201d42e..7515574 100644 --- a/tests/Unit/Lsp/WorkspaceRootResolverTest.php +++ b/tests/Unit/Lsp/WorkspaceRootResolverTest.php @@ -177,6 +177,33 @@ public function testIgnoresAMarkerReachedThroughASymlinkThatLexicallySitsInsideT self::assertSame($workspace, WorkspaceRootResolver::resolve($file, [$workspace], $workspace)); } + public function testFindsANearerMarkerUnderANotYetCreatedDocumentDirectory(): void + { + // #298 follow-up (review finding): canonicalize() used to fall + // straight to a purely lexical collapse the moment realpath() + // failed on the FULL path, which mixed two different + // canonicalisation strategies across the two sides of + // isDescendant()'s comparison whenever only one side existed on + // disk yet. A document's own directory not existing yet (an + // unsaved/new file) is ordinary, not adversarial -- it must not, + // by itself, make resolve() silently settle for a shallower root + // than the one that genuinely owns the document, purely because + // $this->base sits under a path the OS may resolve through a + // symlink (e.g. macOS's /tmp -> /private/tmp) while the + // not-yet-created directory does not resolve at all. + $workspace = $this->base . '/ws'; + mkdir($workspace, 0700, true); + touch($workspace . '/composer.json'); + + $pkg = $workspace . '/pkg'; + mkdir($pkg, 0700, true); + touch($pkg . '/rector.php'); + + $file = $pkg . '/not_yet_created_subdir/File.php'; + + self::assertSame($pkg, WorkspaceRootResolver::resolve($file, [$workspace], $workspace)); + } + public function testNeverWalksAboveTheOwningFolderEvenWhenAnAncestorHasAMarker(): void { // A rector.php sitting ABOVE the workspace folder (e.g. a monorepo @@ -211,6 +238,22 @@ public function testResolvesCleanlyAtAWindowsDriveRootWithNoMarkerAnywhereUnderI self::assertSame('C:', WorkspaceRootResolver::resolve('C:/Project/src/Sample.php', [], 'C:')); } + public function testCollapsesDotDotPastAWindowsDriveRootWithoutDroppingTheDrive(): void + { + // #298 follow-up (review finding): collapseDotSegments() used to + // decide "is this path absolute" solely by a leading "/", so a + // Windows-style drive-letter path ("C:/...") was treated as + // RELATIVE -- a leading ".." could then pop the drive letter + // itself off the stack like any ordinary segment, and once the + // stack went empty the next ".." was pushed back on, turning an + // absolute Windows path into a bogus relative one. The drive + // letter must be protected the same way a POSIX leading "/" is. + $method = new \ReflectionMethod(WorkspaceRootResolver::class, 'collapseDotSegments'); + + self::assertSame('C:/x', $method->invoke(null, 'C:/../x')); + self::assertSame('C:/x', $method->invoke(null, 'C:/../../x')); + } + private static function removeTree(string $dir): void { if (!is_dir($dir)) { From e79c1eb3a1cabcfd7083a62d8d9729b370446408 Mon Sep 17 00:00:00 2001 From: Florian DAVID Date: Mon, 5 Oct 2026 11:24:57 +0200 Subject: [PATCH 3/3] Fix(#298): second-pass review follow-up -- bound the ancestor-walk to 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 --- ...t-marker-dotdot-and-symlink-containment.md | 2 +- src/Lsp/WorkspaceRootResolver.php | 27 +++++++++++++++++-- tests/Unit/Lsp/WorkspaceRootResolverTest.php | 27 +++++++++++++++++++ 3 files changed, 53 insertions(+), 3 deletions(-) diff --git a/changelog.d/298.fixed.workspace-root-marker-dotdot-and-symlink-containment.md b/changelog.d/298.fixed.workspace-root-marker-dotdot-and-symlink-containment.md index 7d4df68..bdc096f 100644 --- a/changelog.d/298.fixed.workspace-root-marker-dotdot-and-symlink-containment.md +++ b/changelog.d/298.fixed.workspace-root-marker-dotdot-and-symlink-containment.md @@ -1 +1 @@ -- `WorkspaceRootResolver::isDescendant()` (#298, the direct sequel to #296) used to be a plain string-prefix test over unresolved paths, so a document path containing `..` (e.g. a URI like `file:////../elsewhere/proj/x.php`), or one reached through a symlink pointing outside the workspace, could still resolve to a marker directory outside the owning workspace folder -- the same containment bypass #296 fixed, reachable through a different path spelling, including with no `workspaceFolders` declared at all. Both sides of the containment comparison are now canonicalised (`realpath()`, falling back to a purely lexical `.`/`..` collapse when the path does not yet exist on disk) before the comparison runs, closing both the `..` case and the symlink case at the same chokepoint. Self-review on the fix found and closed two further gaps before merge: a document whose own directory does not exist yet (an unsaved/new file) was compared asymmetrically against a workspace root that does exist and is itself often behind a symlink (e.g. macOS resolving `/tmp` to `/private/tmp`), which could make `resolve()` settle for the wrong, too-shallow root even for an entirely ordinary document -- fixed by resolving the longest existing ancestor through `realpath()` first and only running the lexical collapse over the non-existent tail; and the lexical fallback mishandled a Windows-style drive-letter path (`C:/...`) once it contained `..`, treating the drive letter as an ordinary, poppable path segment instead of a protected root. +- `WorkspaceRootResolver::isDescendant()` (#298, the direct sequel to #296) used to be a plain string-prefix test over unresolved paths, so a document path containing `..` (e.g. a URI like `file:////../elsewhere/proj/x.php`), or one reached through a symlink pointing outside the workspace, could still resolve to a marker directory outside the owning workspace folder -- the same containment bypass #296 fixed, reachable through a different path spelling, including with no `workspaceFolders` declared at all. Both sides of the containment comparison are now canonicalised (`realpath()`, falling back to a purely lexical `.`/`..` collapse when the path does not yet exist on disk) before the comparison runs, closing both the `..` case and the symlink case at the same chokepoint. Self-review on the fix found and closed two further gaps before merge: a document whose own directory does not exist yet (an unsaved/new file) was compared asymmetrically against a workspace root that does exist and is itself often behind a symlink (e.g. macOS resolving `/tmp` to `/private/tmp`), which could make `resolve()` settle for the wrong, too-shallow root even for an entirely ordinary document -- fixed by resolving the longest existing ancestor through `realpath()` first and only running the lexical collapse over the non-existent tail; and the lexical fallback mishandled a Windows-style drive-letter path (`C:/...`) once it contained `..`, treating the drive letter as an ordinary, poppable path segment instead of a protected root. A second review pass on that fix found one more, lower-severity gap: resolving the longest existing ancestor through `realpath()` has no natural floor for a genuinely relative path, so an ordinary segment name coinciding with a real directory relative to the server's own process cwd could get silently canonicalised against that unrelated directory -- not reachable from a real document path (always absolute, from a `file://` URI), but closed anyway by only probing the filesystem for a path this process can recognise as rooted (a POSIX leading `/` or a Windows drive letter). diff --git a/src/Lsp/WorkspaceRootResolver.php b/src/Lsp/WorkspaceRootResolver.php index 3f29712..31c07ce 100644 --- a/src/Lsp/WorkspaceRootResolver.php +++ b/src/Lsp/WorkspaceRootResolver.php @@ -157,14 +157,37 @@ private static function canonicalize(string $path): string // realpath() first, and only running the lexical collapse over // the non-existent tail, keeps both sides on the same symlink // resolution the moment any shared ancestor exists on disk. - return self::collapseDotSegments(self::resolveExistingAncestor(self::normalize($path))); + // + // Only probed for a path this process can recognise as rooted + // (a POSIX leading "/" or a Windows drive letter) -- a second + // review pass found that probing shorter prefixes of a genuinely + // RELATIVE path has no natural floor to stop at, so an ordinary + // segment name ("src", "tests", ...) that happens to coincide + // with a real directory relative to this process's own cwd would + // get realpath()'d against that unrelated directory. Production + // document paths are always absolute (from a file:// URI); a + // relative path only reaches here from a synthetic caller (a + // unit test), where the old, purely lexical fallback is the + // correct, filesystem-untouched answer. + $normalized = self::normalize($path); + if (!\str_starts_with($normalized, '/') && \preg_match('/^[A-Za-z]:(\/|$)/', $normalized) !== 1) { + return self::collapseDotSegments($normalized); + } + + return self::collapseDotSegments(self::resolveExistingAncestor($normalized)); } private static function resolveExistingAncestor(string $normalized): string { - $isAbsolute = \str_starts_with($normalized, '/'); + $isDrive = \preg_match('/^[A-Za-z]:(\/|$)/', $normalized) === 1; + $isAbsolute = $isDrive || \str_starts_with($normalized, '/'); $segments = \explode('/', $normalized); $keep = \count($segments); + // A POSIX root ("" before the first "/") and a drive letter + // ("C:") are both protected the same way: never probe a prefix + // shorter than the one segment that names the root itself, so a + // coincidental filesystem match can only ever occur at the root + // segment itself, never at an arbitrary ordinary path component. $floor = $isAbsolute ? 1 : 0; while ($keep > $floor) { diff --git a/tests/Unit/Lsp/WorkspaceRootResolverTest.php b/tests/Unit/Lsp/WorkspaceRootResolverTest.php index 7515574..5fb22e8 100644 --- a/tests/Unit/Lsp/WorkspaceRootResolverTest.php +++ b/tests/Unit/Lsp/WorkspaceRootResolverTest.php @@ -254,6 +254,33 @@ public function testCollapsesDotDotPastAWindowsDriveRootWithoutDroppingTheDrive( self::assertSame('C:/x', $method->invoke(null, 'C:/../../x')); } + public function testNeverProbesTheFilesystemForARelativePathsOwnCwd(): void + { + // #298 follow-up (second review pass): resolveExistingAncestor() + // walks shorter prefixes of a path through realpath() looking for + // the longest one that exists on disk -- correct and safe for an + // absolute path (a POSIX "/" or a Windows drive letter floors the + // walk), but a genuinely RELATIVE path has no such floor, so an + // ordinary segment name that happens to coincide with a real + // directory relative to THIS PROCESS's own cwd (not the + // workspace being resolved) would get silently canonicalised + // against that unrelated directory. canonicalize() must refuse + // to probe the filesystem at all for a relative path and fall + // straight to the purely lexical collapse instead. + $cwd = getcwd(); + self::assertNotFalse($cwd, 'this test needs a real process cwd to pick a coincidentally-real segment name from'); + + $entries = array_values(array_filter(scandir($cwd) ?: [], static fn(string $e): bool => $e !== '.' && $e !== '..' && is_dir($cwd . '/' . $e))); + if ($entries === []) { + self::markTestSkipped('no real subdirectory of the process cwd to use as a coincidental-match probe'); + } + + $relative = $entries[0] . '/this-does-not-exist-anywhere/Sample.php'; + + $method = new \ReflectionMethod(WorkspaceRootResolver::class, 'canonicalize'); + self::assertSame($relative, $method->invoke(null, $relative)); + } + private static function removeTree(string $dir): void { if (!is_dir($dir)) {