Merge pull request #516 from corbitsdev/cl-6712-return-canonical-paths-from-workspace-containment-allows

Return canonical realpath from workspace containment allow

Author: TheGreatAxios

src/permission/path-restriction.ts

@@ -44,7 +44,7 @@ function realpathOr(path: string): string {
 // 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).
-function realpathNearestOr(path: string): string {
+export function realpathNearestOr(path: string): string {
   try {
     return realpathSync(path);
   } catch {
@@ -68,9 +68,18 @@ const inKnownRoots = (real: string, roots: readonly string[]): boolean =>
 // Resolves `path` (relative or absolute, possibly traversing `..`) against
 // `cwd` and checks it against the workspace boundary: `cwd` itself plus every
 // root `rootsProvider` reports (the session's registered git worktrees, or
-// any other allowlisted sibling). Returns the resolved absolute path when the
-// target is in bounds, `undefined` otherwise — callers that need a hard
-// allow/deny (rather than an allow/ask distinction) can key off that.
+// any other allowlisted sibling). Returns the CANONICAL real path (symlink
+// segments resolved; for a not-yet-created target, the nearest existing
+// ancestor's real path rejoined with the missing tail) when the target is in
+// bounds, `undefined` otherwise — callers that need a hard allow/deny (rather
+// than an allow/ask distinction) can key off that.
+//
+// Returning the canonical path rather than the lexical `abs` closes a
+// TOCTOU: a symlink segment that is in-bounds at check time can be
+// retargeted before a write actually happens. Callers (e.g. pathEscapePlugin)
+// substitute this return value into the tool call's path argument, so the
+// writer that ultimately opens the file never re-traverses the original
+// symlink — it uses the already-resolved location.
 //
 // A relative `../` is deliberately resolved and realpath-checked against the
 // allowlist rather than rejected outright: the raw path alone can't tell a
@@ -84,9 +93,9 @@ export function resolveWorkspacePath(
   const abs = resolve(cwd, path);
   const realCwd = realpathOr(resolve(cwd));
   const real = realpathNearestOr(abs);
-  if (real === realCwd || real.startsWith(realCwd + sep)) return abs;
-  if (inKnownRoots(real, rootsProvider())) return abs;
-  if (inKnownRoots(real, rootsProvider(true))) return abs;
+  if (real === realCwd || real.startsWith(realCwd + sep)) return real;
+  if (inKnownRoots(real, rootsProvider())) return real;
+  if (inKnownRoots(real, rootsProvider(true))) return real;
   return undefined;
 }
 

src/permission/permission.test.ts

@@ -2813,7 +2813,10 @@ describe("listWorktreeRoots", () => {
     const { repo, worktree } = createRepoWithWorktree();
     const roots = await listWorktreeRoots(repo);
     const relativeTarget = join("..", "secondary", "notes.md");
-    expect(resolveWorkspacePath(repo, relativeTarget, () => roots)).toBe(join(repo, "..", "secondary", "notes.md"));
+    // Canonical (realpath-resolved), not the lexical join — see CL-6712.
+    expect(resolveWorkspacePath(repo, relativeTarget, () => roots)).toBe(
+      join(realpathSync(join(repo, "..")), "secondary", "notes.md"),
+    );
   });
 
   test("resolveWorkspacePath still rejects a genuinely unrelated outside path", async () => {

src/permission/workspace-containment.test.ts

@@ -6,7 +6,7 @@ import { tmpdir } from "node:os";
 import type { ToolCall } from "@intx/types/runtime";
 
 import { isAutoAllowedShellCall } from "./classify.js";
-import { createPathRestriction } from "./path-restriction.js";
+import { createPathRestriction, resolveWorkspacePath } from "./path-restriction.js";
 
 let cwd = "";
 let worktree = "";
@@ -91,3 +91,34 @@ test("a symlink pointing outside the workspace is refused, even for a not-yet-ex
   expect(autoAllowed).toBe(false);
   expect(restricted).toBe(true);
 });
+
+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
+  // symlink. If the caller only remembered the lexical path and re-opened it
+  // after the symlink is retargeted, the write would follow the new target
+  // instead of the one that was actually approved.
+  const rootsProvider = () => [];
+  const realTarget = join(cwd, "real-target");
+  await mkdir(realTarget, { recursive: true });
+  const link = join(cwd, "link");
+  await symlink(realTarget, link);
+
+  const resolved = resolveWorkspacePath(cwd, join("link", "note.txt"), rootsProvider);
+  expect(resolved).toBe(join(realpathSync(realTarget), "note.txt"));
+
+  // Retarget the symlink to point outside the workspace, as an attacker
+  // would do between the allow check and the actual write.
+  await rm(link);
+  await symlink(outside, link);
+
+  // The canonical path captured before the retarget still points at the
+  // originally-approved location inside the workspace — it never traverses
+  // "link" again, so it is unaffected by the retarget.
+  expect(resolved).not.toContain(outside);
+
+  // A fresh check against the now-retargeted symlink correctly sees the
+  // escape and denies it.
+  const restriction = createPathRestriction(cwd, rootsProvider, home);
+  expect(restriction.isRestricted(join(link, "note.txt"), true)).toBe(true);
+});

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

@@ -1,5 +1,7 @@
 import { describe, expect, test } from "bun:test";
-import { resolve } from "node:path";
+import { mkdtempSync, realpathSync, rmSync, symlinkSync } from "node:fs";
+import { tmpdir } from "node:os";
+import { join, resolve } from "node:path";
 import {
   matchesWritePathAllowlist,
   writePathDeniedReason,
@@ -58,6 +60,39 @@ describe("matchesWritePathAllowlist", () => {
   });
 });
 
+describe("matchesWritePathAllowlist with a symlinked cwd", () => {
+  test("allows a write under the canonical target of a symlinked cwd", () => {
+    // Mirrors macOS's /tmp -> /private/tmp: cwd is spelled via the symlink,
+    // but resolveWorkspacePath (and any tool arg it rewrites) hands the
+    // subject in already realpathed. Both sides of the compare must
+    // canonicalize the same way or a legitimate write is hard-denied.
+    const real = mkdtempSync(join(realpathSync(tmpdir()), "write-path-real-"));
+    const linkDir = join(realpathSync(tmpdir()), `write-path-link-${process.pid}`);
+    try {
+      symlinkSync(real, linkDir);
+      const symlinkedCwd = linkDir; // lexically distinct from `real`
+      const canonicalSubject = join(real, "docs", "a.md"); // already realpathed
+
+      expect(
+        matchesWritePathAllowlist(canonicalSubject, ["docs/*"], symlinkedCwd),
+      ).toBe(true);
+
+      // A genuinely outside path is still denied.
+      const outsideReal = mkdtempSync(join(realpathSync(tmpdir()), "write-path-outside-"));
+      try {
+        expect(
+          matchesWritePathAllowlist(join(outsideReal, "docs", "a.md"), ["docs/*"], symlinkedCwd),
+        ).toBe(false);
+      } finally {
+        rmSync(outsideReal, { recursive: true, force: true });
+      }
+    } finally {
+      rmSync(linkDir, { force: true });
+      rmSync(real, { 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,5 +1,6 @@
 import { resolve, sep } from "node:path";
 import { matchesPattern } from "./matcher.js";
+import { realpathNearestOr } from "./path-restriction.js";
 
 /**
  * Director write-path allowlist (authz, not prompt policy).
@@ -29,8 +30,12 @@ export function matchesWritePathAllowlist(
   if (allowlist.length === 0) return false;
   if (subject.length === 0) return false;
 
-  const absCwd = resolve(cwd);
-  const abs = resolve(cwd, subject);
+  // Canonicalize both sides the same way path-restriction does: a symlinked
+  // cwd (e.g. macOS /tmp -> /private/tmp) must not desync from a subject
+  // already resolved to its realpath by resolveWorkspacePath, which would
+  // otherwise hard-deny a legitimate allowlisted write.
+  const absCwd = realpathNearestOr(resolve(cwd));
+  const abs = realpathNearestOr(resolve(cwd, subject));
   let rel: string;
   if (abs === absCwd) {
     rel = ".";

src/plugins/path-escape-plugin.test.ts

@@ -1,4 +1,8 @@
-import { describe, test, expect } from "bun:test";
+import { describe, test, expect, beforeEach, afterEach } from "bun:test";
+import { mkdir, mkdtemp, readFile, rm, symlink, writeFile } from "node:fs/promises";
+import { existsSync, realpathSync } from "node:fs";
+import { tmpdir } from "node:os";
+import { join } from "node:path";
 
 import { pathEscapePlugin } from "./path-escape-plugin.js";
 import type { ToolCall, ToolResult } from "@intx/types/runtime";
@@ -175,4 +179,55 @@ describe("pathEscapePlugin", () => {
     const args = JSON.parse(String(allowed.content)) as { path: string };
     expect(args.path).toBe("/other-repo/README.md");
   });
+
+  describe("symlink TOCTOU (CL-6712)", () => {
+    let cwd = "";
+
+    beforeEach(async () => {
+      cwd = await mkdtemp(join(tmpdir(), "corbits-path-escape-"));
+    });
+
+    afterEach(async () => {
+      await rm(cwd, { recursive: true, force: true });
+    });
+
+    test("write_file receives the canonical path, unaffected by a later symlink retarget", async () => {
+      const realTarget = join(cwd, "real-target");
+      await mkdir(realTarget, { recursive: true });
+      const link = join(cwd, "link");
+      await symlink(realTarget, link);
+
+      const plugin = pathEscapePlugin(cwd);
+      const next = async (call: ToolCall): Promise<ToolResult> => ({
+        callId: call.id,
+        content: JSON.stringify(call.arguments),
+      });
+      const handler = plugin.middleware ? plugin.middleware(next) : next;
+
+      const result = await handler(
+        makeCall("write_file", { path: join("link", "note.txt"), content: "hi" }),
+        new AbortController().signal,
+      );
+      const args = JSON.parse(String(result.content)) as { path: string };
+      // The path handed to write_file is already the resolved real-target
+      // location, not the symlink-relative path.
+      expect(args.path).toBe(join(realpathSync(realTarget), "note.txt"));
+
+      // An attacker retargets the symlink after the allow check. A writer
+      // that (correctly) uses the path it was given above is unaffected —
+      // it never re-traverses "link".
+      const outside = await mkdtemp(join(tmpdir(), "corbits-path-escape-outside-"));
+      await rm(link);
+      await symlink(outside, link);
+      expect(args.path).not.toContain(outside);
+
+      // A real writer using the resolved path lands the bytes at the
+      // canonical (safe) location, never under the retargeted symlink.
+      await writeFile(args.path, "hi");
+      expect(await readFile(args.path, "utf8")).toBe("hi");
+      expect(existsSync(join(outside, "note.txt"))).toBe(false);
+
+      await rm(outside, { recursive: true, force: true });
+    });
+  });
 });