From 86137bdd02ad2928ffeec69ccf7f318b0393930c Mon Sep 17 00:00:00 2001 From: Himanshu Deshhratar Date: Wed, 23 Sep 2026 17:35:46 +0530 Subject: [PATCH 1/2] fix(policies): block Codex apply_patch secret-file writes --- .../hooks/block-secrets-write-codex.test.ts | 104 ++++++++++++++++ __tests__/hooks/builtin-policies.test.ts | 114 +++++++++++++++++- src/hooks/builtin-policies.ts | 62 ++++++++-- src/hooks/policy-catalog.ts | 4 +- 4 files changed, 274 insertions(+), 10 deletions(-) create mode 100644 __tests__/hooks/block-secrets-write-codex.test.ts diff --git a/__tests__/hooks/block-secrets-write-codex.test.ts b/__tests__/hooks/block-secrets-write-codex.test.ts new file mode 100644 index 000000000..762409c2f --- /dev/null +++ b/__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"); + }); +}); diff --git a/__tests__/hooks/builtin-policies.test.ts b/__tests__/hooks/builtin-policies.test.ts index cd3c4500b..08e15812b 100644 --- a/__tests__/hooks/builtin-policies.test.ts +++ b/__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", () => { diff --git a/src/hooks/builtin-policies.ts b/src/hooks/builtin-policies.ts index 8ca252205..4920334fa 100644 --- a/src/hooks/builtin-policies.ts +++ b/src/hooks/builtin-policies.ts @@ -99,6 +99,47 @@ 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: }` + * (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 +2226,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(); diff --git a/src/hooks/policy-catalog.ts b/src/hooks/policy-catalog.ts index 7659772b2..2892a7f5e 100644 --- a/src/hooks/policy-catalog.ts +++ b/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: { From 3b901c3ae8eccd2afce20326bc68623414c2702d Mon Sep 17 00:00:00 2001 From: Himanshu Deshhratar Date: Wed, 23 Sep 2026 23:33:44 +0530 Subject: [PATCH 2/2] fix: guard non-string command input --- src/hooks/builtin-policies.ts | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/src/hooks/builtin-policies.ts b/src/hooks/builtin-policies.ts index 4920334fa..c0945d9e6 100644 --- a/src/hooks/builtin-policies.ts +++ b/src/hooks/builtin-policies.ts @@ -92,7 +92,8 @@ 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 {