diff --git a/src/permission/path-restriction.ts b/src/permission/path-restriction.ts index 43a02db76..ba42c33b4 100644 --- a/src/permission/path-restriction.ts +++ b/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); } diff --git a/src/permission/workspace-containment.test.ts b/src/permission/workspace-containment.test.ts index 1d19b2a01..1548b441c 100644 --- a/src/permission/workspace-containment.test.ts +++ b/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 diff --git a/src/permission/write-path-policy.test.ts b/src/permission/write-path-policy.test.ts index 40df9090d..259222907 100644 --- a/src/permission/write-path-policy.test.ts +++ b/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/*"]); diff --git a/src/permission/write-path-policy.ts b/src/permission/write-path-policy.ts index dd7a30547..17eb150d8 100644 --- a/src/permission/write-path-policy.ts +++ b/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 = ".";