Merge pull request #518 from corbitsdev/cl-6715-deny-dangling-symlink-components-in-nearest-realpath-walks

Deny dangling symlink components in nearest-realpath walks

Author: TheGreatAxios

src/permission/path-restriction.ts

@@ -1,4 +1,4 @@
-import { realpathSync } from "node:fs";
+import { lstatSync, realpathSync } from "node:fs";
 import { dirname, join, resolve, sep } from "node:path";
 import { homedir } from "node:os";
 import type { RootsProvider } from "./worktree-roots.js";
@@ -39,22 +39,46 @@ function realpathOr(path: string): string {
   }
 }
 
+// Sentinel returned by realpathNearestOr for a path that exists but couldn't
+// be resolved (a dangling symlink, or a symlink loop) rather than one that's
+// simply missing. Contains a NUL byte, which can never appear in a real
+// filesystem path, so it can't collide with (or be mistaken for a prefix of)
+// any genuine result, and every containment compare against it fails.
+export const UNRESOLVABLE = "\0unresolvable\0";
+
 // A write/edit target usually doesn't exist yet, so realpath the nearest
 // existing ancestor and rejoin the missing tail rather than falling back to
 // the raw (possibly symlink-relative) path, which would defeat containment
 // checks whenever the workspace root itself is reached through a symlink
 // (e.g. macOS's /tmp -> /private/tmp).
+//
+// realpath failure is ambiguous: it's either "this component doesn't exist
+// yet" (safe — the missing tail gets rejoined onto the nearest real ancestor)
+// or "this component exists but is a dangling symlink / symlink loop" (unsafe
+// — the link's name must not stand in for a normal under-cwd path segment,
+// since the walk otherwise reattaches it verbatim and containment sees a
+// plain child path with no idea a symlink is involved). lstat distinguishes
+// the two: it succeeds for an existing-but-broken symlink and fails only when
+// the component is genuinely absent.
 export function realpathNearestOr(path: string): string {
   try {
     return realpathSync(path);
   } catch {
+    try {
+      lstatSync(path);
+      return UNRESOLVABLE;
+    } catch {
+      // Doesn't exist at all — fall through to the nearest-ancestor walk.
+    }
     const parent = dirname(path);
     if (parent === path) return path;
     // Root (e.g. "/") already ends in the separator, so slicing past
     // parent.length alone lands on the tail; anywhere else the separator
     // between parent and tail must be skipped too.
     const tailStart = parent.endsWith(sep) ? parent.length : parent.length + 1;
-    return join(realpathNearestOr(parent), path.slice(tailStart));
+    const parentReal = realpathNearestOr(parent);
+    if (parentReal === UNRESOLVABLE) return UNRESOLVABLE;
+    return join(parentReal, path.slice(tailStart));
   }
 }
 
@@ -93,6 +117,7 @@ export function resolveWorkspacePath(
   const abs = resolve(cwd, path);
   const realCwd = realpathOr(resolve(cwd));
   const real = realpathNearestOr(abs);
+  if (real === UNRESOLVABLE) return undefined;
   if (real === realCwd || real.startsWith(realCwd + sep)) return real;
   if (inKnownRoots(real, rootsProvider())) return real;
   if (inKnownRoots(real, rootsProvider(true))) return real;
@@ -139,6 +164,7 @@ function underRoot(abs: string, root: string): boolean {
   // unresolved while the abs path is rebuilt through an existing ancestor).
   const realRoot = realpathNearestOr(root);
   const realAbs = realpathNearestOr(abs);
+  if (realRoot === UNRESOLVABLE || realAbs === UNRESOLVABLE) return false;
   return realAbs === realRoot || realAbs.startsWith(realRoot + sep);
 }
 

src/permission/workspace-containment.test.ts

@@ -92,6 +92,41 @@ test("a symlink pointing outside the workspace is refused, even for a not-yet-ex
   expect(restricted).toBe(true);
 });
 
+test("a dangling symlink under cwd pointing outside denies a child path, and stays denied after the outside target is created (CL-6715)", async () => {
+  // The symlink target does not exist yet, so realpath fails on the link
+  // itself (not just on the not-yet-existing child) — the walk must not
+  // treat the dangling link's name as an ordinary missing tail segment.
+  const rootsProvider = () => [];
+  const link = join(cwd, "dangling-link");
+  const outsideTarget = join(outside, "not-created-yet");
+  await symlink(outsideTarget, link);
+  const target = join(link, "child.txt");
+
+  expect(resolveWorkspacePath(cwd, join("dangling-link", "child.txt"), rootsProvider)).toBeUndefined();
+
+  const restriction = createPathRestriction(cwd, rootsProvider, home);
+  expect(restriction.isRestricted(target, false)).toBe(true);
+  expect(restriction.isRestricted(target, true)).toBe(true);
+
+  // Creating the outside target after the initial check must not retroactively
+  // legitimize it: this is ordinary outside-symlink denial (the link still
+  // ultimately points outside the workspace), re-checked once the target exists.
+  await mkdir(outsideTarget, { recursive: true });
+  await writeFile(join(outsideTarget, "child.txt"), "s");
+  expect(resolveWorkspacePath(cwd, join("dangling-link", "child.txt"), rootsProvider)).toBeUndefined();
+});
+
+test("a symlink loop under cwd is denied by resolveWorkspacePath (CL-6715)", async () => {
+  const rootsProvider = () => [];
+  const linkA = join(cwd, "loop-a");
+  const linkB = join(cwd, "loop-b");
+  await symlink(linkB, linkA);
+  await symlink(linkA, linkB);
+
+  expect(resolveWorkspacePath(cwd, join("loop-a", "child.txt"), rootsProvider)).toBeUndefined();
+  expect(resolveWorkspacePath(cwd, "loop-a", rootsProvider)).toBeUndefined();
+});
+
 test("resolveWorkspacePath returns the canonical target so a later symlink retarget cannot redirect a write (CL-6712 TOCTOU)", async () => {
   // A symlink that is in-bounds at check time (target -> inside cwd) must
   // resolve to the canonical real path, not the lexical path through the

src/permission/write-path-policy.test.ts

@@ -93,6 +93,26 @@ describe("matchesWritePathAllowlist with a symlinked cwd", () => {
   });
 });
 
+describe("matchesWritePathAllowlist with an unresolvable cwd", () => {
+  test("a cwd whose final component is a dangling symlink is hard-denied, not spuriously allowed (CL-6715)", () => {
+    // If cwd itself is unresolvable, both absCwd and abs collapse to the same
+    // UNRESOLVABLE sentinel, `abs === absCwd` goes true, rel becomes ".", and
+    // a root-matching pattern (e.g. "**") would otherwise spuriously allow —
+    // turning a hard authz deny into an ask-prompt.
+    const parent = mkdtempSync(join(realpathSync(tmpdir()), "write-path-dangling-parent-"));
+    const danglingCwd = join(parent, "dangling-cwd");
+    try {
+      symlinkSync(join(parent, "does-not-exist"), danglingCwd);
+
+      expect(matchesWritePathAllowlist("anything.md", ["**"], danglingCwd)).toBe(false);
+      expect(matchesWritePathAllowlist("PRODUCT.md", ["PRODUCT.md"], danglingCwd)).toBe(false);
+    } finally {
+      rmSync(danglingCwd, { force: true });
+      rmSync(parent, { recursive: true, force: true });
+    }
+  });
+});
+
 describe("writePathDeniedReason", () => {
   test("names allowlist and subject", () => {
     const reason = writePathDeniedReason("src/x.ts", ["PRODUCT.md", "docs/*"]);

src/permission/write-path-policy.ts

@@ -1,6 +1,6 @@
 import { resolve, sep } from "node:path";
 import { matchesPattern } from "./matcher.js";
-import { realpathNearestOr } from "./path-restriction.js";
+import { realpathNearestOr, UNRESOLVABLE } from "./path-restriction.js";
 
 /**
  * Director write-path allowlist (authz, not prompt policy).
@@ -36,6 +36,11 @@ export function matchesWritePathAllowlist(
   // otherwise hard-deny a legitimate allowlisted write.
   const absCwd = realpathNearestOr(resolve(cwd));
   const abs = realpathNearestOr(resolve(cwd, subject));
+  // Either side unresolvable (dangling symlink/loop component) must hard-deny.
+  // Otherwise an unresolvable cwd and an unresolvable subject both collapse to
+  // the same sentinel, `abs === absCwd` goes true, rel becomes ".", and a
+  // root-matching allowlist pattern spuriously allows.
+  if (absCwd === UNRESOLVABLE || abs === UNRESOLVABLE) return false;
   let rel: string;
   if (abs === absCwd) {
     rel = ".";