Fix: workspace root marker must canonicalise before containment check (#298)

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:///<workspace>/../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).

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);

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)) {