Merge pull request #581 from corbitsdev/cl-6966-apply_patch-update-file-reads-line-numbered-read_file-output

fix(apply_patch): read raw file content for Update File matching

Author: TheGreatAxios

src/agent/apply-patch-diff.test.ts

@@ -1,12 +1,13 @@
 import { describe, expect, test } from "bun:test";
-import { mkdtemp, readFile, rm, writeFile } from "node:fs/promises";
+import { mkdir, mkdtemp, rm, symlink, writeFile } from "node:fs/promises";
 import { tmpdir } from "node:os";
 import { join } from "node:path";
 import { createPosixTools } from "@intx/tools-posix";
 import { createToolRunner } from "@intx/agent";
 import type { AgentTool } from "@intx/agent";
 
 import { createCodexToolProxies, type CodexRunTool } from "./codex-tool-proxies.js";
+import { createCodexReadRawFile } from "./codex-read-raw-file.js";
 import { buildCorePosixToolPlugins } from "./posix-tool-plugins.js";
 import { createPermissionGate } from "../permission/gate.js";
 
@@ -23,11 +24,14 @@ async function invokeApplyPatch(tools: AgentTool[], input: string) {
   );
 }
 
-async function makeApplyPatch(cwd: string): Promise<AgentTool[]> {
+async function makeApplyPatch(
+  cwd: string,
+  options: { skipPermissions?: boolean } = {},
+): Promise<AgentTool[]> {
   const gate = createPermissionGate({
     approvals: [],
     interactive: false,
-    skipPermissions: true,
+    skipPermissions: options.skipPermissions ?? true,
     auto: false,
     cwd,
   });
@@ -36,23 +40,6 @@ async function makeApplyPatch(cwd: string): Promise<AgentTool[]> {
     plugins: buildCorePosixToolPlugins({ cwd, permissionGate: gate }),
   });
   const runTool: CodexRunTool = async (name, args) => {
-    // read_file's real tool output is cat -n formatted (line numbers), which
-    // is not the raw content applyUpdateHunks needs — in production this
-    // means Update File hunks generally fail to match context (filed as
-    // CL-6966, Urgent; not this issue's bug to fix). This stub bypasses that
-    // known defect by returning raw content, so the "Update File shows the
-    // diff" test below is NOT proof that apply_patch Update works end to end
-    // — it only proves the diff-surfacing added here is correct once the op
-    // succeeds. Add File / Delete File below do not depend on read_file and
-    // are real, unstubbed coverage.
-    if (name === "read_file") {
-      const path = String((args as { path?: unknown }).path ?? "");
-      try {
-        return { content: await readFile(join(cwd, path), "utf8") };
-      } catch (err) {
-        return { content: err instanceof Error ? err.message : String(err), isError: true };
-      }
-    }
     const result = await posixTools.run(
       { id: "codex-proxy", name, arguments: args },
       new AbortController().signal,
@@ -62,14 +49,49 @@ async function makeApplyPatch(cwd: string): Promise<AgentTool[]> {
       ...(result.isError === true ? { isError: true } : {}),
     };
   };
+  // Real production path (CL-6966): Update File matches patch context against
+  // raw file content, not read_file's cat -n formatted output.
   return createCodexToolProxies({
     isCodex: true,
     runTool,
+    readRawFile: createCodexReadRawFile(cwd, gate),
     runManageTasks: async () => ({ content: "ok" }),
   });
 }
 
-describe("apply_patch surfaces the changed region", () => {
+describe("apply_patch Update File matches raw content, not read_file's numbered output (CL-6966)", () => {
+  test("multi-hunk Update File succeeds through the real production path", async () => {
+    const cwd = await mkdtemp(join(tmpdir(), "apply-patch-cl6966-"));
+    try {
+      await writeFile(
+        join(cwd, "app.py"),
+        "def greet():\n    print('hi')\n\n\ndef farewell():\n    print('bye')\n",
+      );
+      const tools = await makeApplyPatch(cwd);
+      const input = [
+        "*** Begin Patch",
+        "*** Update File: app.py",
+        "@@ def greet():",
+        "-    print('hi')",
+        "+    print('hello')",
+        "@@ def farewell():",
+        "-    print('bye')",
+        "+    print('goodbye')",
+        "*** End Patch",
+      ].join("\n");
+
+      const result = await invokeApplyPatch(tools, input);
+
+      expect(result.isError).not.toBe(true);
+      const written = await Bun.file(join(cwd, "app.py")).text();
+      expect(written).toBe(
+        "def greet():\n    print('hello')\n\n\ndef farewell():\n    print('goodbye')\n",
+      );
+    } finally {
+      await rm(cwd, { recursive: true, force: true });
+    }
+  });
+
   test("Update File shows the diff", async () => {
     const cwd = await mkdtemp(join(tmpdir(), "apply-patch-diff-"));
     try {
@@ -129,3 +151,98 @@ describe("apply_patch surfaces the changed region", () => {
     }
   });
 });
+
+describe("apply_patch Update File refuses reads outside the sanctioned workspace (CL-6966 follow-up)", () => {
+  // Insertion-only hunk (no context to match): the shape that makes an
+  // unauthorized raw read exploitable rather than merely wrong, since it
+  // requires no content match to "succeed" and hands the read content
+  // straight to write_file via Move to.
+  const insertionOnlyMoveInput = (path: string, moveTo: string) =>
+    [
+      "*** Begin Patch",
+      `*** Update File: ${path}`,
+      `*** Move to: ${moveTo}`,
+      "@@",
+      "+",
+      "*** End Patch",
+    ].join("\n");
+
+  test("../ traversal out of the workspace is refused", async () => {
+    const parent = await mkdtemp(join(tmpdir(), "apply-patch-cl6966-parent-"));
+    const cwd = join(parent, "workspace");
+    await mkdir(cwd);
+    try {
+      await writeFile(join(parent, "victim.txt"), "outside secret\n");
+      const tools = await makeApplyPatch(cwd, { skipPermissions: false });
+
+      const result = await invokeApplyPatch(
+        tools,
+        insertionOnlyMoveInput("../victim.txt", "leaked.txt"),
+      );
+
+      expect(result.isError).toBe(true);
+      expect(String(result.content)).toMatch(/escapes working directory/i);
+    } finally {
+      await rm(parent, { recursive: true, force: true });
+    }
+  });
+
+  test("a symlinked directory leading outside the workspace is refused", async () => {
+    const parent = await mkdtemp(join(tmpdir(), "apply-patch-cl6966-symlink-"));
+    const cwd = join(parent, "workspace");
+    const outside = join(parent, "outside");
+    await mkdir(cwd);
+    await mkdir(outside);
+    try {
+      await writeFile(join(outside, "victim.txt"), "outside secret\n");
+      await symlink(outside, join(cwd, "escape-link"));
+      const tools = await makeApplyPatch(cwd, { skipPermissions: false });
+
+      const result = await invokeApplyPatch(
+        tools,
+        insertionOnlyMoveInput("escape-link/victim.txt", "leaked.txt"),
+      );
+
+      expect(result.isError).toBe(true);
+      expect(String(result.content)).toMatch(/escapes working directory/i);
+    } finally {
+      await rm(parent, { recursive: true, force: true });
+    }
+  });
+
+  test("a secret-guard path (.env) is refused even with skipPermissions", async () => {
+    const cwd = await mkdtemp(join(tmpdir(), "apply-patch-cl6966-secret-"));
+    try {
+      await writeFile(join(cwd, ".env"), "API_KEY=super-secret\n");
+      // skipPermissions: true (yolo) — secret-guard has no bypass, unlike containment.
+      const tools = await makeApplyPatch(cwd, { skipPermissions: true });
+
+      const result = await invokeApplyPatch(tools, insertionOnlyMoveInput(".env", "leaked.txt"));
+
+      expect(result.isError).toBe(true);
+      expect(String(result.content)).toMatch(/sensitive file/i);
+      // The secret must never have reached the workspace under a new name.
+      expect(await Bun.file(join(cwd, "leaked.txt")).exists()).toBe(false);
+    } finally {
+      await rm(cwd, { recursive: true, force: true });
+    }
+  });
+
+  test("../ traversal to a secret file is refused (secret-guard applies to relative paths too)", async () => {
+    const parent = await mkdtemp(join(tmpdir(), "apply-patch-cl6966-secret-parent-"));
+    const cwd = join(parent, "workspace");
+    await mkdir(cwd);
+    try {
+      await writeFile(join(parent, ".env"), "API_KEY=super-secret\n");
+      const tools = await makeApplyPatch(cwd, { skipPermissions: false });
+
+      const result = await invokeApplyPatch(tools, insertionOnlyMoveInput("../.env", "leaked.txt"));
+
+      expect(result.isError).toBe(true);
+      expect(String(result.content)).toMatch(/sensitive file|escapes working directory/i);
+      expect(await Bun.file(join(cwd, "leaked.txt")).exists()).toBe(false);
+    } finally {
+      await rm(parent, { recursive: true, force: true });
+    }
+  });
+});

src/agent/codex-read-raw-file.ts

@@ -0,0 +1,89 @@
+/**
+ * Raw (non-`cat -n`) file reads for apply_patch's Update File leg (CL-6966).
+ *
+ * `applyOp` calls this directly from outside the posixTools middleware chain
+ * — no ToolPlugin ever sees `op.path` here, unlike the write leg which still
+ * goes through the full pathEscapePlugin / secretGuardPlugin / authzPlugin /
+ * permissionPlugin stack (see buildCorePosixToolPlugins in
+ * posix-tool-plugins.ts). `requireRelativePath` in codex-apply-patch.ts only
+ * rejects absolute paths — it does nothing about `../` traversal — so this
+ * reader must apply the same containment and secret-file checks itself, or
+ * an Update File op naming e.g. `../../.env` (with a no-op insertion hunk,
+ * which requires no context match) can read a secret and hand it straight to
+ * write_file as an exfiltration primitive.
+ *
+ * Reuses the existing containment and secret-file authorities rather than
+ * reimplementing them: `resolveWorkspacePath` (symlink-aware realpath
+ * containment, the same function pathEscapePlugin calls) and `isSensitivePath`
+ * (the secretGuardPlugin denylist). Both are hard denials — the latter has no
+ * yolo/allowOutside bypass, matching secretGuardPlugin's own unconditional
+ * behavior.
+ */
+
+import { readFile } from "node:fs/promises";
+import { resolve } from "node:path";
+import { hasCode } from "@intx/types";
+import { resolveWorkspacePath } from "../permission/path-restriction.js";
+import { createWorktreeRootsProvider } from "../permission/worktree-roots.js";
+import { isSensitivePath } from "../plugins/secret-guard-plugin.js";
+import type { PermissionGate } from "../permission/gate.js";
+import type { CodexReadRawFile } from "./codex-tool-proxies.js";
+
+/** `path` is workspace-relative (apply_patch's parser rejects absolute paths). */
+export function createCodexReadRawFile(
+  cwd: string,
+  permissionGate: PermissionGate,
+): CodexReadRawFile {
+  const rootsProvider = createWorktreeRootsProvider(cwd);
+  const allowOutside = (): boolean => permissionGate.getSkipPermissions();
+
+  return async (path) => {
+    // Secret-file denylist first, on the raw path — this must hold regardless
+    // of containment or yolo, exactly like secretGuardPlugin's hard deny.
+    if (isSensitivePath(path)) {
+      return {
+        content: `Access to sensitive file blocked by policy: ${path}`,
+        isError: true,
+      };
+    }
+
+    const resolved = resolveWorkspacePath(cwd, path, rootsProvider);
+    let absolutePath: string;
+    if (resolved !== undefined) {
+      absolutePath = resolved;
+    } else if (allowOutside()) {
+      // Mirrors pathEscapePlugin's own allowOutside fallback (yolo mode).
+      absolutePath = resolve(cwd, path);
+    } else {
+      return {
+        content: `Path escapes working directory: ${path}`,
+        isError: true,
+      };
+    }
+
+    // Re-check the resolved, symlink-realpath'd form too: a symlink can name
+    // something innocuous while pointing at a sensitive real path.
+    if (isSensitivePath(absolutePath)) {
+      return {
+        content: `Access to sensitive file blocked by policy: ${path}`,
+        isError: true,
+      };
+    }
+
+    try {
+      const buf = await readFile(absolutePath);
+      if (buf.includes(0)) {
+        return { content: `refusing to read binary file: ${path}`, isError: true };
+      }
+      return { content: buf.toString("utf8") };
+    } catch (err) {
+      if (hasCode(err)) {
+        if (err.code === "ENOENT") return { content: `file not found: ${path}`, isError: true };
+        if (err.code === "EACCES") return { content: `permission denied: ${path}`, isError: true };
+        if (err.code === "EISDIR")
+          return { content: `path is a directory: ${path}`, isError: true };
+      }
+      return { content: err instanceof Error ? err.message : String(err), isError: true };
+    }
+  };
+}

src/agent/codex-tool-mount.test.ts

@@ -130,6 +130,7 @@ describe("Codex tool proxy mount", () => {
     const proxies = createCodexToolProxies({
       isCodex: true,
       runTool: async () => ({ content: "ok" }),
+      readRawFile: async () => ({ content: "ok" }),
       runManageTasks: async () => ({ content: "ok" }),
     });
     expect(proxies.map((t) => t.definition.name)).toEqual(["apply_patch", "shell", "update_plan"]);
@@ -152,6 +153,7 @@ describe("Codex tool proxy mount", () => {
     const proxies = createCodexToolProxies({
       isCodex: true,
       runTool: async () => ({ content: "ok" }),
+      readRawFile: async () => ({ content: "ok" }),
       runManageTasks: async () => ({ content: "ok" }),
       allowDelete: allowDeleteFromCapabilities(docsCapabilities),
       allowShell: allowShellFromCapabilities(docsCapabilities),
@@ -167,6 +169,7 @@ describe("Codex tool proxy mount", () => {
     const proxies = createCodexToolProxies({
       isCodex: false,
       runTool: async () => ({ content: "ok" }),
+      readRawFile: async () => ({ content: "ok" }),
       runManageTasks: async () => ({ content: "ok" }),
       allowDelete: allowDeleteFromCapabilities({ mode: "allow", tools: BUILD_TOOLS }),
       allowShell: allowShellFromCapabilities({ mode: "allow", tools: BUILD_TOOLS }),