fix(policies): block Codex apply_patch secret-file writes (#814)

__tests__/hooks/block-secrets-write-codex.test.ts

@@ -0,0 +1,104 @@
+// @vitest-environment node
+/**
+ * Issue #814: Codex apply_patch must hit block-secrets-write.
+ *
+ * Drives the real evaluateHookEvent path (canonicalize apply_patch → Edit,
+ * then policy match + path extraction from tool_input.command). Telemetry and
+ * pack reads are isolated; builtins are real.
+ */
+import { describe, it, expect, vi, beforeEach, afterEach } from "vitest";
+import { mkdtempSync, rmSync, mkdirSync, writeFileSync } from "node:fs";
+import { tmpdir } from "node:os";
+import { join } from "node:path";
+
+vi.mock("../../src/hooks/hook-telemetry", () => ({
+  trackHookEvent: vi.fn(() => Promise.resolve()),
+  flushHookTelemetry: vi.fn(() => Promise.resolve()),
+}));
+vi.mock("../../src/hooks/pack-manifest", () => ({
+  readInstalledPacks: vi.fn(() => ({ packs: [], errors: [] })),
+  hasInstalledPacks: vi.fn(() => false),
+}));
+
+import { evaluateHookEvent } from "../../src/hooks/handler";
+
+let homeDir: string;
+let projectDir: string;
+
+beforeEach(() => {
+  homeDir = mkdtempSync(join(tmpdir(), "fpai-secrets-write-home-"));
+  projectDir = mkdtempSync(join(tmpdir(), "fpai-secrets-write-proj-"));
+  process.env.FAILPROOFAI_HOME = homeDir;
+  mkdirSync(join(homeDir), { recursive: true });
+  writeFileSync(
+    join(homeDir, "policies-config.json"),
+    JSON.stringify({ enabledPolicies: ["block-secrets-write"], customPoliciesEnabled: false }),
+  );
+});
+
+afterEach(() => {
+  delete process.env.FAILPROOFAI_HOME;
+  rmSync(homeDir, { recursive: true, force: true });
+  rmSync(projectDir, { recursive: true, force: true });
+});
+
+function codexApplyPatch(command: string): string {
+  return JSON.stringify({
+    session_id: "issue-814",
+    cwd: projectDir,
+    hook_event_name: "pre_tool_use",
+    tool_name: "apply_patch",
+    tool_input: { command },
+  });
+}
+
+describe("block-secrets-write × Codex apply_patch (#814)", () => {
+  it("denies apply_patch that adds .ssh/id_rsa (real Codex payload shape)", async () => {
+    const result = await evaluateHookEvent(
+      "pre_tool_use",
+      "codex",
+      codexApplyPatch(
+        "*** Begin Patch\n*** Add File: .ssh/id_rsa\n+FAKESECRET_test_only\n*** End Patch",
+      ),
+      { awaitTelemetryFlush: false },
+    );
+    expect(result.evaluation?.toolName).toBe("Edit");
+    expect(result.evaluation?.decision).toBe("deny");
+    expect(result.evaluation?.policyName).toBe("failproofai/block-secrets-write");
+    expect(result.evaluation?.matchedPolicies).toContain("failproofai/block-secrets-write");
+  });
+
+  it("allows apply_patch that only touches a safe path", async () => {
+    const result = await evaluateHookEvent(
+      "pre_tool_use",
+      "codex",
+      codexApplyPatch(
+        "*** Begin Patch\n*** Add File: src/safe.ts\n+export const ok = 1;\n*** End Patch",
+      ),
+      { awaitTelemetryFlush: false },
+    );
+    expect(result.evaluation?.toolName).toBe("Edit");
+    expect(result.evaluation?.decision).toBe("allow");
+    expect(result.evaluation?.policyName).not.toBe("failproofai/block-secrets-write");
+  });
+
+  it("denies multi-file apply_patch when a protected path follows a safe path", async () => {
+    const result = await evaluateHookEvent(
+      "pre_tool_use",
+      "codex",
+      codexApplyPatch(
+        [
+          "*** Begin Patch",
+          "*** Add File: src/safe.ts",
+          "+export const ok = 1;",
+          "*** Add File: .ssh/id_rsa",
+          "+FAKESECRET_test_only",
+          "*** End Patch",
+        ].join("\n"),
+      ),
+      { awaitTelemetryFlush: false },
+    );
+    expect(result.evaluation?.decision).toBe("deny");
+    expect(result.evaluation?.policyName).toBe("failproofai/block-secrets-write");
+  });
+});

__tests__/hooks/builtin-policies.test.ts

@@ -1156,10 +1156,122 @@ describe("hooks/builtin-policies", () => {
       expect((await policy.fn(ctx)).decision).toBe("allow");
     });
 
-    it("allows non-Write tools", async () => {
+    it("allows non-Write/Edit tools", async () => {
       const ctx = makeCtx({ toolName: "Read", toolInput: { file_path: "/home/user/cert.pem" } });
       expect((await policy.fn(ctx)).decision).toBe("allow");
     });
+
+    // Issue #814: Codex apply_patch canonicalizes to Edit and puts the patch in
+    // tool_input.command (not file_path). Fake key material only.
+    it("blocks Codex-shaped apply_patch Edit that adds .ssh/id_rsa via command", async () => {
+      const ctx = makeCtx({
+        toolName: "Edit",
+        toolInput: {
+          command:
+            "*** Begin Patch\n*** Add File: .ssh/id_rsa\n+FAKESECRET_test_only\n*** End Patch",
+        },
+      });
+      const result = await policy.fn(ctx);
+      expect(result.decision).toBe("deny");
+      expect(result.reason).toContain("secret key files");
+    });
+
+    it("blocks Edit with file_path pointing at a .pem (non-Codex Edit shape)", async () => {
+      const ctx = makeCtx({
+        toolName: "Edit",
+        toolInput: { file_path: "/home/user/cert.pem", old_string: "a", new_string: "b" },
+      });
+      expect((await policy.fn(ctx)).decision).toBe("deny");
+    });
+
+    it("allows Codex-shaped apply_patch Edit that only touches safe paths", async () => {
+      const ctx = makeCtx({
+        toolName: "Edit",
+        toolInput: {
+          command:
+            "*** Begin Patch\n*** Add File: src/safe.ts\n+export const ok = 1;\n*** End Patch",
+        },
+      });
+      expect((await policy.fn(ctx)).decision).toBe("allow");
+    });
+
+    it("denies multi-file apply_patch when a protected path appears after a safe path", async () => {
+      // Ordering matters for the regression: a first-path-only checker would
+      // see src/safe.ts and allow. Protected path is id_rsa (this policy's
+      // surface — not .env, which belongs to block-env-files).
+      const ctx = makeCtx({
+        toolName: "Edit",
+        toolInput: {
+          command: [
+            "*** Begin Patch",
+            "*** Add File: src/safe.ts",
+            "+export const ok = 1;",
+            "*** Add File: .ssh/id_rsa",
+            "+FAKESECRET_test_only",
+            "*** End Patch",
+          ].join("\n"),
+        },
+      });
+      expect((await policy.fn(ctx)).decision).toBe("deny");
+    });
+
+    it("denies multi-file apply_patch when Update File targets credentials after a safe update", async () => {
+      const ctx = makeCtx({
+        toolName: "Edit",
+        toolInput: {
+          command: [
+            "*** Begin Patch",
+            "*** Update File: README.md",
+            "@@",
+            "-old",
+            "+new",
+            "*** Update File: .aws/credentials",
+            "@@",
+            "+aws_access_key_id=AKIA_TEST_NOT_A_REAL_KEY",
+            "*** End Patch",
+          ].join("\n"),
+        },
+      });
+      expect((await policy.fn(ctx)).decision).toBe("deny");
+    });
+
+    // apply_patch rename: Update File = source, Move to = destination. Both
+    // paths must be checked so either a protected source or protected dest denies.
+    it("denies apply_patch rename when Move to targets a protected path", async () => {
+      const ctx = makeCtx({
+        toolName: "Edit",
+        toolInput: {
+          command: [
+            "*** Begin Patch",
+            "*** Update File: src/safe.ts",
+            "*** Move to: .ssh/id_rsa",
+            "@@",
+            "-old",
+            "+FAKESECRET_test_only",
+            "*** End Patch",
+          ].join("\n"),
+        },
+      });
+      expect((await policy.fn(ctx)).decision).toBe("deny");
+    });
+
+    it("denies apply_patch rename when Update File source is a protected path", async () => {
+      const ctx = makeCtx({
+        toolName: "Edit",
+        toolInput: {
+          command: [
+            "*** Begin Patch",
+            "*** Update File: .ssh/id_rsa",
+            "*** Move to: src/safe.ts",
+            "@@",
+            "-old",
+            "+new",
+            "*** End Patch",
+          ].join("\n"),
+        },
+      });
+      expect((await policy.fn(ctx)).decision).toBe("deny");
+    });
   });
 
   describe("block-work-on-main", () => {

src/hooks/builtin-policies.ts

@@ -92,13 +92,55 @@ const isClaudeInternalPath = isAgentInternalPath;
 const isClaudeSettingsFile = isAgentSettingsFile;
 
 function getCommand(ctx: PolicyContext): string {
-  return (ctx.toolInput?.command as string) ?? "";
+  const command = ctx.toolInput?.command;
+  return typeof command === "string" ? command : "";
 }
 
 function getFilePath(ctx: PolicyContext): string {
   return (ctx.toolInput?.file_path as string) ?? "";
 }
 
+/**
+ * Paths named by an OpenAI/Codex `apply_patch` freeform body.
+ *
+ * Codex PreToolUse for apply_patch delivers `tool_input: { command: <patch> }`
+ * (not `file_path`). Headers that name a target are `*** Add File:`,
+ * `*** Update File:`, `*** Delete File:`, and `*** Move to:`. A single patch
+ * may list many files — callers must check every path, not only the first.
+ *
+ * There is no shared apply_patch parser elsewhere in this repo; this stays
+ * local to the secrets-write policy rather than inventing a general layer.
+ */
+const APPLY_PATCH_PATH_RE =
+  /^\*\*\* (?:Add File|Update File|Delete File|Move to):\s*(.+?)\s*$/gm;
+
+function extractApplyPatchPaths(command: string): string[] {
+  if (!command.includes("*** ")) return [];
+  const paths: string[] = [];
+  APPLY_PATCH_PATH_RE.lastIndex = 0;
+  let m: RegExpExecArray | null;
+  while ((m = APPLY_PATCH_PATH_RE.exec(command)) !== null) {
+    const p = m[1].trim();
+    if (p) paths.push(p);
+  }
+  return paths;
+}
+
+/**
+ * Every file path a Write/Edit-shaped tool call may create or modify.
+ * Prefer `file_path` (Claude Write/Edit and harnesses that remap to it); also
+ * pull every path out of a Codex-style apply_patch `command` body.
+ */
+function getSecretWriteTargetPaths(ctx: PolicyContext): string[] {
+  const paths: string[] = [];
+  const filePath = getFilePath(ctx);
+  if (filePath) paths.push(filePath);
+  for (const p of extractApplyPatchPaths(getCommand(ctx))) {
+    if (!paths.includes(p)) paths.push(p);
+  }
+  return paths;
+}
+
 /**
  * Parse a command string into argv tokens for safe pattern matching.
  * Splits on whitespace and strips simple single/double quotes.
@@ -2185,15 +2227,20 @@ function isForcePushFlag(token: string): boolean {
 }
 
 function blockSecretsWrite(ctx: PolicyContext): PolicyResult {
-  if (ctx.toolName !== "Write") return allow();
-  const filePath = getFilePath(ctx);
-  if (SECRET_FILE_RE.test(filePath) || SECRET_FILE_ID_RSA_RE.test(filePath) || SECRET_FILE_CREDENTIALS_RE.test(filePath)) {
-    return deny("Writing secret key files is blocked");
-  }
+  // Write: Claude-shaped file create. Edit: includes Codex/OpenCode apply_patch
+  // after canonicalizeToolName maps apply_patch → Edit.
+  if (ctx.toolName !== "Write" && ctx.toolName !== "Edit") return allow();
   const additionalPatterns = ((ctx.params?.additionalPatterns ?? []) as string[]);
-  for (const pattern of additionalPatterns) {
-    if (filePath.includes(pattern)) {
-      return deny(`Writing blocked file pattern: ${pattern}`);
+  // Multi-file apply_patch: deny if ANY affected path is protected — a safe
+  // path earlier in the patch must not hide a later secret-file path.
+  for (const filePath of getSecretWriteTargetPaths(ctx)) {
+    if (SECRET_FILE_RE.test(filePath) || SECRET_FILE_ID_RSA_RE.test(filePath) || SECRET_FILE_CREDENTIALS_RE.test(filePath)) {
+      return deny("Writing secret key files is blocked");
+    }
+    for (const pattern of additionalPatterns) {
+      if (filePath.includes(pattern)) {
+        return deny(`Writing blocked file pattern: ${pattern}`);
+      }
     }
   }
   return allow();

src/hooks/policy-catalog.ts

@@ -299,7 +299,9 @@ export const POLICY_CATALOG: PolicyCatalogEntry[] = [
     displayTitle: "Tried to write a secret-key file",
     impact: "Stops the agent from creating `.pem`, `id_rsa`, `credentials.json`, etc.",
     description: "Block writing secret key files",
-    match: { events: ["PreToolUse"], toolNames: ["Write"] },
+    // Edit is included because Codex (and OpenCode) canonicalize `apply_patch` →
+    // Edit. Without it, secret-file creates via apply_patch never matched.
+    match: { events: ["PreToolUse"], toolNames: ["Write", "Edit"] },
     defaultEnabled: false,
     category: "Dangerous Commands",
     params: {