diff --git a/src/permission/path-restriction.ts b/src/permission/path-restriction.ts index 9f24bd2b4..43a02db76 100644 --- a/src/permission/path-restriction.ts +++ b/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; } diff --git a/src/permission/permission.test.ts b/src/permission/permission.test.ts index 622cea535..566156e44 100644 --- a/src/permission/permission.test.ts +++ b/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 () => { diff --git a/src/permission/workspace-containment.test.ts b/src/permission/workspace-containment.test.ts index 23354e2be..1d19b2a01 100644 --- a/src/permission/workspace-containment.test.ts +++ b/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); +}); diff --git a/src/permission/write-path-policy.test.ts b/src/permission/write-path-policy.test.ts index ca0f14064..40df9090d 100644 --- a/src/permission/write-path-policy.test.ts +++ b/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/*"]); diff --git a/src/permission/write-path-policy.ts b/src/permission/write-path-policy.ts index 6c6edee01..dd7a30547 100644 --- a/src/permission/write-path-policy.ts +++ b/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 = "."; diff --git a/src/plugins/path-escape-plugin.test.ts b/src/plugins/path-escape-plugin.test.ts index c792c4e67..fdf6b9017 100644 --- a/src/plugins/path-escape-plugin.test.ts +++ b/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 => ({ + 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 }); + }); + }); });