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..bdc096f --- /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. 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 999738f..31c07ce 100644 --- a/src/Lsp/WorkspaceRootResolver.php +++ b/src/Lsp/WorkspaceRootResolver.php @@ -114,9 +114,156 @@ 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. + $resolved = \realpath($path); + if ($resolved !== false) { + return self::normalize($resolved); + } + + // #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. + // + // 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 + { + $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) { + $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 + { + // 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('/', $rest) as $segment) { + if ($segment === '' || $segment === '.') { + continue; + } + + if ($segment === '..') { + if ($stack !== [] && \end($stack) !== '..') { + \array_pop($stack); + } elseif (!$isAbsolute) { + $stack[] = '..'; + } + // ".." 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; + } + + $stack[] = $segment; + } + + $result = ($isAbsolute ? '/' : '') . \implode('/', $stack); + $result = $result === '' ? '/' : $result; + + if ($drive === '') { + return $result; + } + + return $result === '/' ? $drive : $drive . $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..5fb22e8 100644 --- a/tests/Unit/Lsp/WorkspaceRootResolverTest.php +++ b/tests/Unit/Lsp/WorkspaceRootResolverTest.php @@ -131,6 +131,79 @@ 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 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 @@ -165,6 +238,49 @@ 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')); + } + + 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)) {