From 8d607a66f7e6f3d502d5b56bed51edcb0fd843b6 Mon Sep 17 00:00:00 2001 From: iaj6 Date: Sun, 12 Jul 2026 11:35:41 -0400 Subject: [PATCH] fix(policy): normalize paths + scan Bash commands in policy guards MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit July 2026 audit found the policy guard layer shallower than the product story. Three findings addressed: 1. PathRestriction bypass — the pre-tool guard did a raw startsWith on the unnormalized tool-input path, and only for Edit/Write. Both the guard and the post-hoc evaluator now compare lexically-normalized paths (./ and ../ segments resolved with posix-normalize semantics, no fs access so not-yet-existing files still work) case-insensitively (macOS filesystems are case-insensitive by default), and NotebookEdit is inspected via notebook_path wherever file_path was handled (PathRestriction, FileLimitCount, BranchProtection, SecretDetection, and the hook's tool-to-action mapping). Bash-mediated writes (tee, > redirects) remain out of scope and are now documented as such at the guard. 2. SecretDetection never saw Bash commands — the pre-tool guard now scans the Bash command string (and NotebookEdit new_source); the post-hoc evaluator scans cmd.command in addition to cmd.stdout; and the hook now populates FileEdit.diff with the written content (new_string / content / new_source, capped at 10000 chars) so post-hoc re.test(edit.diff) can actually match on hook-produced runs instead of always testing the empty string. 3. minCoverage removed from TestEnforcementConfig — it was declared, persisted, and rendered in the dashboard ("min 80% coverage") but never enforced, and no coverage number exists anywhere in the domain model to enforce it against. Removed from the config type and the dashboard summary so the UI stops claiming enforcement that cannot happen; legacy rows carrying the key stay inert. Verified end-to-end through the real CLI hook: traversal Write, case-variant NotebookEdit, and secret-bearing Bash command all block with exit 2; benign writes pass; post-tool-use persists the diff. Co-Authored-By: Claude Fable 5 --- packages/cli/src/__tests__/hook.test.ts | 99 +++++ packages/cli/src/commands/hook.ts | 20 +- packages/core/src/__tests__/policy.test.ts | 400 +++++++++++++++++- packages/core/src/index.ts | 1 + packages/core/src/policy.ts | 182 ++++++-- .../src/lib/__tests__/policy-summary.test.ts | 31 +- packages/web/src/lib/policy-summary.ts | 12 +- 7 files changed, 682 insertions(+), 63 deletions(-) diff --git a/packages/cli/src/__tests__/hook.test.ts b/packages/cli/src/__tests__/hook.test.ts index 4b372f9..ed0cb03 100644 --- a/packages/cli/src/__tests__/hook.test.ts +++ b/packages/cli/src/__tests__/hook.test.ts @@ -172,6 +172,8 @@ describe("Tool-to-action mapping", () => { expect(action.toolCalls[0]!.name).toBe("Edit"); expect(action.fileEdits).toHaveLength(1); expect(action.fileEdits[0]!.path).toBe("/src/index.ts"); + // diff carries the written content so post-hoc SecretDetection can scan it + expect(action.fileEdits[0]!.diff).toBe("bar"); expect(action.commands).toHaveLength(0); }); @@ -186,6 +188,44 @@ describe("Tool-to-action mapping", () => { expect(action.fileEdits).toHaveLength(1); expect(action.fileEdits[0]!.path).toBe("/src/new.ts"); + expect(action.fileEdits[0]!.diff).toBe("hello"); + }); + + it("truncates oversized written content in the diff field", () => { + const input: HookInput = { + session_id: "test", + tool_name: "Write", + tool_input: { file_path: "/src/big.ts", content: "x".repeat(20000) }, + }; + + const action = _mapToolToAction(input); + expect(action.fileEdits[0]!.diff).toHaveLength(10000); + }); + + it("falls back to empty diff when written content is missing", () => { + const input: HookInput = { + session_id: "test", + tool_name: "Edit", + tool_input: { file_path: "/src/index.ts" }, + }; + + const action = _mapToolToAction(input); + expect(action.fileEdits).toHaveLength(1); + expect(action.fileEdits[0]!.diff).toBe(""); + }); + + it("maps NotebookEdit tool to a file edit via notebook_path", () => { + const input: HookInput = { + session_id: "test", + tool_name: "NotebookEdit", + tool_input: { notebook_path: "/nb/analysis.ipynb", new_source: "print('hi')" }, + }; + + const action = _mapToolToAction(input); + expect(action.fileEdits).toHaveLength(1); + expect(action.fileEdits[0]!.path).toBe("/nb/analysis.ipynb"); + expect(action.fileEdits[0]!.diff).toBe("print('hi')"); + expect(action.commands).toHaveLength(0); }); it("maps Read tool to action with no edits", () => { @@ -328,6 +368,40 @@ describe("Pre-tool-use policy checking", () => { expect(violations[0]!.message).toContain("~/.ssh/"); }); + it("detects blocked file paths reached via ../ traversal", () => { + const input: HookInput = { + session_id: "test", + tool_name: "Write", + tool_input: { file_path: "/tmp/../etc/passwd" }, + }; + + const violations = _checkPreToolPolicies(input, [pathPolicy]); + expect(violations).toHaveLength(1); + }); + + it("detects blocked file paths with case variants", () => { + const input: HookInput = { + session_id: "test", + tool_name: "Edit", + tool_input: { file_path: "/ETC/passwd" }, + }; + + const violations = _checkPreToolPolicies(input, [pathPolicy]); + expect(violations).toHaveLength(1); + }); + + it("detects blocked file paths on NotebookEdit via notebook_path", () => { + const input: HookInput = { + session_id: "test", + tool_name: "NotebookEdit", + tool_input: { notebook_path: "/etc/evil.ipynb", new_source: "x" }, + }; + + const violations = _checkPreToolPolicies(input, [pathPolicy]); + expect(violations).toHaveLength(1); + expect(violations[0]!.message).toContain("/etc/"); + }); + it("returns no violations for safe commands", () => { const input: HookInput = { session_id: "test", @@ -539,6 +613,31 @@ describe("Pre-tool-use policy checking", () => { expect(violations).toHaveLength(0); }); + it("blocks Bash command carrying a secret", () => { + const input: HookInput = { + session_id: "test", + tool_name: "Bash", + tool_input: { command: "export AWS_ACCESS_KEY_ID=AKIAIOSFODNN7EXAMPLE" }, + }; + + const violations = _checkPreToolPolicies(input, [secretPolicy]); + expect(violations).toHaveLength(1); + expect(violations[0]!.message).toContain("Secret pattern"); + // The block message must never echo the secret it caught. + expect(violations[0]!.message).not.toContain("AKIAIOSFODNN7EXAMPLE"); + }); + + it("allows Bash command with no secrets", () => { + const input: HookInput = { + session_id: "test", + tool_name: "Bash", + tool_input: { command: "npm run build" }, + }; + + const violations = _checkPreToolPolicies(input, [secretPolicy]); + expect(violations).toHaveLength(0); + }); + it("allows Read tool (not scanned by SecretDetection)", () => { const input: HookInput = { session_id: "test", diff --git a/packages/cli/src/commands/hook.ts b/packages/cli/src/commands/hook.ts index dcd4d23..6e3d926 100644 --- a/packages/cli/src/commands/hook.ts +++ b/packages/cli/src/commands/hook.ts @@ -218,10 +218,28 @@ function mapToolToAction(input: HookInput): Action { }); } + // Populate `diff` with the content the tool wrote — post-hoc SecretDetection + // regex-tests edit.diff, so an empty string here would make that check + // structurally unable to match on hook-produced runs. Truncated to bound + // the row size (a secret past the cap is still caught by the pre-tool + // guard, which scans the full input). + const MAX_DIFF_CHARS = 10000; + if ((toolName === "Edit" || toolName === "Write") && typeof toolInput["file_path"] === "string") { + const written = + toolName === "Write" ? toolInput["content"] : toolInput["new_string"]; fileEdits.push({ path: toolInput["file_path"] as string, - diff: "", + diff: typeof written === "string" ? written.slice(0, MAX_DIFF_CHARS) : "", + timestamp, + }); + } + + if (toolName === "NotebookEdit" && typeof toolInput["notebook_path"] === "string") { + const written = toolInput["new_source"]; + fileEdits.push({ + path: toolInput["notebook_path"] as string, + diff: typeof written === "string" ? written.slice(0, MAX_DIFF_CHARS) : "", timestamp, }); } diff --git a/packages/core/src/__tests__/policy.test.ts b/packages/core/src/__tests__/policy.test.ts index 1fa8d96..968b2ba 100644 --- a/packages/core/src/__tests__/policy.test.ts +++ b/packages/core/src/__tests__/policy.test.ts @@ -6,6 +6,8 @@ import { PolicyMode, getPolicyMode, runHasMutations, + evaluatePreToolPolicies, + normalizePathForPolicy, evaluateBudgetPolicies, evaluateBudgetWarnings, DEFAULT_BUDGET_WARN_AT_PCT, @@ -101,6 +103,51 @@ describe("PolicyEngine", () => { expect(results[0]!.passed).toBe(false); expect(results[0]!.message).toContain("secrets/api_key.txt"); }); + + it("fails when a blocked path is reached via ../ traversal", () => { + const run = makeRun({ + actions: [makeAction([{ path: "src/../.env", diff: "+SECRET=123", timestamp: "2025-01-01T00:00:00.000Z" }])], + }); + + const results = engine.evaluate(run, [policy]); + expect(results[0]!.passed).toBe(false); + }); + + it("fails when a blocked path is disguised with ./ segments", () => { + const run = makeRun({ + actions: [makeAction([{ path: "./secrets/api_key.txt", diff: "+key", timestamp: "2025-01-01T00:00:00.000Z" }])], + }); + + const results = engine.evaluate(run, [policy]); + expect(results[0]!.passed).toBe(false); + }); + + it("fails on case-variant paths (case-insensitive comparison)", () => { + const run = makeRun({ + actions: [makeAction([{ path: "SECRETS/Api_Key.txt", diff: "+key", timestamp: "2025-01-01T00:00:00.000Z" }])], + }); + + const results = engine.evaluate(run, [policy]); + expect(results[0]!.passed).toBe(false); + }); + + it("passes when traversal resolves OUT of a blocked directory", () => { + const run = makeRun({ + actions: [makeAction([{ path: "secrets/../src/index.ts", diff: "+code", timestamp: "2025-01-01T00:00:00.000Z" }])], + }); + + const results = engine.evaluate(run, [policy]); + expect(results[0]!.passed).toBe(true); + }); + + it("does not match a sibling directory sharing the blocked prefix (trailing slash preserved)", () => { + const run = makeRun({ + actions: [makeAction([{ path: "secretsandmore/notes.txt", diff: "+x", timestamp: "2025-01-01T00:00:00.000Z" }])], + }); + + const results = engine.evaluate(run, [policy]); + expect(results[0]!.passed).toBe(true); + }); }); describe("FileLimitCount policy", () => { @@ -165,7 +212,6 @@ describe("PolicyEngine", () => { config: { type: PolicyType.TestEnforcement, requirePassing: true, - minCoverage: 80, }, severity: PolicySeverity.Error, }; @@ -202,6 +248,30 @@ describe("PolicyEngine", () => { expect(results[0]!.message).toContain("No test results found"); }); + it("ignores a legacy minCoverage key left in a stored config (coverage is not enforced)", () => { + // minCoverage was removed from TestEnforcementConfig — no coverage data + // exists on a Run to enforce it against. Rows persisted before the + // removal still carry the key in their JSON blob; it must be inert. + const legacyPolicy: Policy = { + ...policy, + config: { + type: PolicyType.TestEnforcement, + requirePassing: true, + minCoverage: 99, + } as unknown as Policy["config"], + }; + const run = makeRun({ + evaluations: [{ + testResults: [{ name: "test1", passed: true, duration: 10, message: "ok" }], + policyChecks: [], + confidenceScore: 1, + }], + }); + const results = engine.evaluate(run, [legacyPolicy]); + expect(results[0]!.passed).toBe(true); + expect(results[0]!.message).not.toContain("coverage"); + }); + it("fails when some tests fail", () => { const run = makeRun({ evaluations: [{ @@ -355,6 +425,50 @@ describe("PolicyEngine", () => { expect(results[0]!.passed).toBe(false); }); + it("fails when a secret appears in a Bash command string (not just stdout)", () => { + const run = makeRun({ + actions: [makeAction([], [{ + command: "export AWS_ACCESS_KEY_ID=AKIAIOSFODNN7EXAMPLE", + exitCode: 0, + stdout: "", + stderr: "", + timestamp: "2025-01-01T00:00:00.000Z", + }])], + }); + const results = engine.evaluate(run, [policy]); + expect(results[0]!.passed).toBe(false); + expect(results[0]!.message).toContain("matched in command"); + }); + + it("fails when a secret appears in command stdout", () => { + const run = makeRun({ + actions: [makeAction([], [{ + command: "cat ~/.aws/credentials", + exitCode: 0, + stdout: "aws_access_key_id = AKIAIOSFODNN7EXAMPLE", + stderr: "", + timestamp: "2025-01-01T00:00:00.000Z", + }])], + }); + const results = engine.evaluate(run, [policy]); + expect(results[0]!.passed).toBe(false); + expect(results[0]!.message).toContain("matched in command output"); + }); + + it("passes for a benign command with benign output", () => { + const run = makeRun({ + actions: [makeAction([], [{ + command: "npm test", + exitCode: 0, + stdout: "42 passing", + stderr: "", + timestamp: "2025-01-01T00:00:00.000Z", + }])], + }); + const results = engine.evaluate(run, [policy]); + expect(results[0]!.passed).toBe(true); + }); + it("handles multiple patterns, reports all matches", () => { const run = makeRun({ actions: [makeAction([ @@ -648,6 +762,290 @@ describe("PolicyEngine", () => { }); }); +describe("normalizePathForPolicy", () => { + it("resolves ../ segments lexically", () => { + expect(normalizePathForPolicy("/repo/src/../secrets/key.pem")).toBe("/repo/secrets/key.pem"); + }); + + it("drops ./ segments and doubled slashes", () => { + expect(normalizePathForPolicy("/repo/./src//a.ts")).toBe("/repo/src/a.ts"); + expect(normalizePathForPolicy("./secrets/x")).toBe("secrets/x"); + }); + + it("lowercases for case-insensitive comparison", () => { + expect(normalizePathForPolicy("/Repo/SECRETS/Key.pem")).toBe("/repo/secrets/key.pem"); + }); + + it("keeps leading .. on relative paths", () => { + expect(normalizePathForPolicy("../secrets/key.pem")).toBe("../secrets/key.pem"); + expect(normalizePathForPolicy("a/../../b")).toBe("../b"); + }); + + it("drops .. above an absolute root", () => { + expect(normalizePathForPolicy("/../etc/passwd")).toBe("/etc/passwd"); + }); + + it("preserves a trailing slash so directory prefixes stay anchored", () => { + expect(normalizePathForPolicy("secrets/")).toBe("secrets/"); + expect(normalizePathForPolicy("/repo/secrets/")).toBe("/repo/secrets/"); + }); + + it("normalizes degenerate inputs to '.'", () => { + expect(normalizePathForPolicy("")).toBe("."); + expect(normalizePathForPolicy(".")).toBe("."); + expect(normalizePathForPolicy("a/..")).toBe("."); + }); +}); + +// ─── Pre-tool guard (direct coverage) ──────────────────────────────────────── + +describe("evaluatePreToolPolicies", () => { + const enabled = { severity: PolicySeverity.Error, enabled: true } as const; + + describe("PathRestriction guard", () => { + const policy: Policy & { enabled: boolean } = { + id: createPolicyId("pol_guard_path"), + name: "Protect secrets", + type: PolicyType.PathRestriction, + config: { + type: PolicyType.PathRestriction, + blockedPaths: ["/repo/secrets/", "/repo/.env"], + }, + ...enabled, + }; + + it("blocks a direct Write into a blocked path", () => { + const v = evaluatePreToolPolicies( + { toolName: "Write", toolInput: { file_path: "/repo/secrets/key.pem", content: "x" } }, + [policy], + ); + expect(v).toHaveLength(1); + expect(v[0]!.message).toContain("Path restriction violated"); + }); + + it("blocks ../ traversal into a blocked path", () => { + const v = evaluatePreToolPolicies( + { toolName: "Write", toolInput: { file_path: "/repo/src/../secrets/key.pem", content: "x" } }, + [policy], + ); + expect(v).toHaveLength(1); + }); + + it("blocks ./ disguised paths", () => { + const v = evaluatePreToolPolicies( + { toolName: "Edit", toolInput: { file_path: "/repo/./secrets/key.pem", new_string: "x" } }, + [policy], + ); + expect(v).toHaveLength(1); + }); + + it("blocks case-variant paths (macOS filesystems are case-insensitive)", () => { + const v = evaluatePreToolPolicies( + { toolName: "Write", toolInput: { file_path: "/REPO/Secrets/Key.pem", content: "x" } }, + [policy], + ); + expect(v).toHaveLength(1); + }); + + it("blocks NotebookEdit via notebook_path", () => { + const v = evaluatePreToolPolicies( + { + toolName: "NotebookEdit", + toolInput: { notebook_path: "/repo/secrets/nb.ipynb", new_source: "x" }, + }, + [policy], + ); + expect(v).toHaveLength(1); + expect(v[0]!.message).toContain("Path restriction violated"); + }); + + it("allows traversal that resolves OUT of the blocked directory", () => { + const v = evaluatePreToolPolicies( + { toolName: "Write", toolInput: { file_path: "/repo/secrets/../src/a.ts", content: "x" } }, + [policy], + ); + expect(v).toHaveLength(0); + }); + + it("does not match a sibling directory sharing the blocked prefix", () => { + const v = evaluatePreToolPolicies( + { toolName: "Write", toolInput: { file_path: "/repo/secretsandmore/a.ts", content: "x" } }, + [policy], + ); + expect(v).toHaveLength(0); + }); + + it("still ignores non-file-writing tools (Read, Bash)", () => { + expect( + evaluatePreToolPolicies( + { toolName: "Read", toolInput: { file_path: "/repo/secrets/key.pem" } }, + [policy], + ), + ).toHaveLength(0); + // Bash-mediated writes are documented as out of scope for this guard. + expect( + evaluatePreToolPolicies( + { toolName: "Bash", toolInput: { command: "tee /repo/secrets/key.pem" } }, + [policy], + ), + ).toHaveLength(0); + }); + + it("preserves relative blocked-path semantics (.env anchors at path start)", () => { + const relPolicy: Policy & { enabled: boolean } = { + ...policy, + config: { type: PolicyType.PathRestriction, blockedPaths: [".env"] }, + }; + // ".env" and ".env.local" match the prefix from the start of the path… + expect( + evaluatePreToolPolicies( + { toolName: "Write", toolInput: { file_path: ".env", content: "x" } }, + [relPolicy], + ), + ).toHaveLength(1); + expect( + evaluatePreToolPolicies( + { toolName: "Write", toolInput: { file_path: ".env.local", content: "x" } }, + [relPolicy], + ), + ).toHaveLength(1); + // …but a nested "src/.env" does not (prefix match, not substring match). + expect( + evaluatePreToolPolicies( + { toolName: "Write", toolInput: { file_path: "src/.env", content: "x" } }, + [relPolicy], + ), + ).toHaveLength(0); + }); + }); + + describe("FileLimitCount guard with NotebookEdit", () => { + const policy: Policy & { enabled: boolean } = { + id: createPolicyId("pol_guard_filelimit"), + name: "Max 2 files", + type: PolicyType.FileLimitCount, + config: { type: PolicyType.FileLimitCount, maxFiles: 2 }, + ...enabled, + }; + + it("counts NotebookEdit toward the file limit", () => { + const v = evaluatePreToolPolicies( + { toolName: "NotebookEdit", toolInput: { notebook_path: "/nb/analysis.ipynb", new_source: "x" } }, + [policy], + { editedFiles: new Set(["/src/a.ts", "/src/b.ts"]) }, + ); + expect(v).toHaveLength(1); + expect(v[0]!.message).toContain("File limit exceeded"); + }); + + it("allows NotebookEdit on an already-edited notebook at the limit", () => { + const v = evaluatePreToolPolicies( + { toolName: "NotebookEdit", toolInput: { notebook_path: "/nb/analysis.ipynb", new_source: "x" } }, + [policy], + { editedFiles: new Set(["/nb/analysis.ipynb", "/src/b.ts"]) }, + ); + expect(v).toHaveLength(0); + }); + }); + + describe("SecretDetection guard", () => { + const policy: Policy & { enabled: boolean } = { + id: createPolicyId("pol_guard_secret"), + name: "No secrets", + type: PolicyType.SecretDetection, + config: { + type: PolicyType.SecretDetection, + patterns: ["AKIA[0-9A-Z]{16}"], + }, + ...enabled, + }; + + it("blocks a Bash command carrying a secret", () => { + const v = evaluatePreToolPolicies( + { + toolName: "Bash", + toolInput: { command: "curl -H 'X-Key: AKIAIOSFODNN7EXAMPLE' https://api.example.com" }, + }, + [policy], + ); + expect(v).toHaveLength(1); + expect(v[0]!.message).toContain("Secret pattern"); + // Never echo the matched content — it IS the secret. + expect(v[0]!.message).not.toContain("AKIAIOSFODNN7EXAMPLE"); + }); + + it("allows a benign Bash command", () => { + const v = evaluatePreToolPolicies( + { toolName: "Bash", toolInput: { command: "npm test" } }, + [policy], + ); + expect(v).toHaveLength(0); + }); + + it("still scans Write content and Edit new_string", () => { + expect( + evaluatePreToolPolicies( + { toolName: "Write", toolInput: { file_path: "/a.ts", content: "AKIAIOSFODNN7EXAMPLE" } }, + [policy], + ), + ).toHaveLength(1); + expect( + evaluatePreToolPolicies( + { toolName: "Edit", toolInput: { file_path: "/a.ts", new_string: "AKIAIOSFODNN7EXAMPLE" } }, + [policy], + ), + ).toHaveLength(1); + }); + + it("scans NotebookEdit new_source", () => { + const v = evaluatePreToolPolicies( + { + toolName: "NotebookEdit", + toolInput: { notebook_path: "/nb/a.ipynb", new_source: "key = 'AKIAIOSFODNN7EXAMPLE'" }, + }, + [policy], + ); + expect(v).toHaveLength(1); + }); + }); + + describe("BranchProtection guard with NotebookEdit", () => { + const policy: Policy & { enabled: boolean } = { + id: createPolicyId("pol_guard_branch"), + name: "Protected branches", + type: PolicyType.BranchProtection, + config: { type: PolicyType.BranchProtection, protectedBranches: ["main"] }, + ...enabled, + }; + + it("treats NotebookEdit as a mutation on a protected branch", () => { + const v = evaluatePreToolPolicies( + { toolName: "NotebookEdit", toolInput: { notebook_path: "/nb/a.ipynb", new_source: "x" } }, + [policy], + { branch: "main" }, + ); + expect(v).toHaveLength(1); + expect(v[0]!.message).toContain("protected branch"); + }); + }); + + it("skips disabled policies", () => { + const disabled: Policy & { enabled: boolean } = { + id: createPolicyId("pol_guard_disabled"), + name: "Disabled", + type: PolicyType.PathRestriction, + config: { type: PolicyType.PathRestriction, blockedPaths: ["/repo/"] }, + severity: PolicySeverity.Error, + enabled: false, + }; + const v = evaluatePreToolPolicies( + { toolName: "Write", toolInput: { file_path: "/repo/a.ts", content: "x" } }, + [disabled], + ); + expect(v).toHaveLength(0); + }); +}); + describe("runHasMutations", () => { it("returns false for a run with no actions", () => { const run = makeRun({ actions: [] }); diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 8e59030..c79956d 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -84,6 +84,7 @@ export { getPolicyMode, runHasMutations, evaluatePreToolPolicies, + normalizePathForPolicy, evaluateBudgetPolicies, evaluateBudgetWarnings, compileRegexPatterns, diff --git a/packages/core/src/policy.ts b/packages/core/src/policy.ts index 4edac18..3f4783e 100644 --- a/packages/core/src/policy.ts +++ b/packages/core/src/policy.ts @@ -72,7 +72,11 @@ export interface FileLimitCountConfig { export interface TestEnforcementConfig { readonly type: PolicyType.TestEnforcement; readonly requirePassing: boolean; - readonly minCoverage: number; + // NOTE: a `minCoverage` field used to live here. It was declared, persisted, + // and rendered in the dashboard but never enforced — no coverage number + // exists anywhere in the domain model (TestResult carries name/passed/ + // duration/message only). Removed rather than left as UI theater; legacy + // rows that still carry the key in their JSON config are ignored. } export interface RiskyOpFlagConfig { @@ -119,14 +123,66 @@ export function runHasMutations(run: Run): boolean { ); } +// ─── Path normalization for PathRestriction ────────────────────────────────── +// +// A raw startsWith prefix check is trivially bypassed with "../" traversal +// ("/repo/src/../secrets/key.pem"), redundant "./" segments, doubled +// separators, or — on case-insensitive filesystems like the macOS default — +// a case change ("/repo/SECRETS/key.pem"). Both the pre-tool guard and the +// post-hoc evaluator therefore compare *normalized* paths. +// +// Normalization is purely lexical (node:path posix-normalize semantics, +// reimplemented here so core stays dependency-free and browser-safe): the +// guard must work for files that don't exist yet, so no fs access. "." and +// empty segments are dropped; ".." pops the previous segment (or is dropped +// at an absolute root, kept at the head of a relative path); a trailing +// slash is preserved so a "secrets/" prefix can't match "secretsandmore/". +// Finally the result is lowercased. Lowercasing can over-block on +// case-sensitive filesystems (Linux), but for a guard, failing closed on a +// case collision is the right trade. +export function normalizePathForPolicy(p: string): string { + const absolute = p.startsWith("/"); + const hadTrailingSlash = p.endsWith("/"); + const segments: string[] = []; + for (const seg of p.split("/")) { + if (seg === "" || seg === ".") continue; + if (seg === "..") { + const top = segments[segments.length - 1]; + if (top !== undefined && top !== "..") { + segments.pop(); + } else if (!absolute) { + segments.push(".."); + } + // ".." at an absolute root is dropped — nothing above "/". + continue; + } + segments.push(seg); + } + let out = (absolute ? "/" : "") + segments.join("/"); + if (out === "") out = "."; + if (hadTrailingSlash && !out.endsWith("/")) out += "/"; + return out.toLowerCase(); +} + +/** Blocked-path entries whose normalized form is a prefix of the normalized file path. */ +function matchBlockedPaths( + filePath: string, + blockedPaths: ReadonlyArray, +): string[] { + const normalized = normalizePathForPolicy(filePath); + return blockedPaths.filter((blocked) => + normalized.startsWith(normalizePathForPolicy(blocked)), + ); +} + // ─── Policy engine ─────────────────────────────────────────────────────────── function evaluatePathRestriction(run: Run, policy: Policy, config: PathRestrictionConfig): PolicyResult { const editedPaths = run.actions.flatMap((a) => a.fileEdits.map((e) => e.path) ); - const violations = editedPaths.filter((p) => - config.blockedPaths.some((blocked) => p.startsWith(blocked)) + const violations = editedPaths.filter( + (p) => matchBlockedPaths(p, config.blockedPaths).length > 0 ); return { passed: violations.length === 0, @@ -260,6 +316,12 @@ function evaluateSecretDetection(run: Run, policy: Policy, config: SecretDetecti } for (const cmd of action.commands) { for (const { source, re } of compiledPatterns) { + // Scan the command string itself, not just its output — a secret + // pasted into `export AWS_KEY=...` or `curl -H "Authorization: ..."` + // never appears in stdout. + if (re.test(cmd.command)) { + matched.push(`Pattern "${source}" matched in command`); + } if (re.test(cmd.stdout)) { matched.push(`Pattern "${source}" matched in command output`); } @@ -433,6 +495,52 @@ function summarizeCommand(cmd: string): string { return cmd.length > 80 ? cmd.slice(0, 77) + "..." : cmd; } +// Target path of a pending file write, for the tools that declare one. +// Edit/Write use `file_path`; NotebookEdit uses `notebook_path` (previously +// never inspected, so notebooks were a blanket bypass for path policies). +// +// LIMITATION: Bash-mediated writes are NOT intercepted. A `tee`, `>` +// redirect, `cp`, or heredoc can still touch a blocked path — parsing shell +// commands for write targets is out of scope for this guard (and the +// post-hoc evaluator has the same blind spot). Pair a PathRestriction with +// RiskyOpFlag patterns if shell writes to sensitive paths are a concern. +function fileWriteTargetPath( + toolName: string, + toolInput: Record, +): string | null { + const key = + toolName === "Edit" || toolName === "Write" + ? "file_path" + : toolName === "NotebookEdit" + ? "notebook_path" + : null; + if (key === null) return null; + const value = toolInput[key]; + return typeof value === "string" ? value : null; +} + +// Content a pending tool call is about to write (or run), for SecretDetection. +// Bash is included: a secret embedded in the command string itself (env +// export, curl auth header) would otherwise reach the shell unscanned. +function scannableToolContent( + toolName: string, + toolInput: Record, +): string { + const key = + toolName === "Write" + ? "content" + : toolName === "Edit" + ? "new_string" + : toolName === "NotebookEdit" + ? "new_source" + : toolName === "Bash" + ? "command" + : null; + if (key === null) return ""; + const value = toolInput[key]; + return typeof value === "string" ? value : ""; +} + export function evaluatePreToolPolicies( invocation: ToolInvocation, activePolicies: ReadonlyArray, @@ -467,48 +575,39 @@ export function evaluatePreToolPolicies( } } - if ( - policy.config.type === PolicyType.PathRestriction && - (toolName === "Edit" || toolName === "Write") && - typeof toolInput["file_path"] === "string" - ) { - const filePath = toolInput["file_path"] as string; - const blocked = policy.config.blockedPaths.filter((p) => - filePath.startsWith(p), - ); - if (blocked.length > 0) { - violations.push({ - policy: policy.name, - message: `Path restriction violated: ${summarizePath(filePath)} matches blocked path(s): ${blocked.join(", ")}`, - severity: policy.severity, - }); + if (policy.config.type === PolicyType.PathRestriction) { + // Normalized + case-insensitive comparison; see normalizePathForPolicy + // and the Bash-write limitation documented on fileWriteTargetPath. + const filePath = fileWriteTargetPath(toolName, toolInput); + if (filePath !== null) { + const blocked = matchBlockedPaths(filePath, policy.config.blockedPaths); + if (blocked.length > 0) { + violations.push({ + policy: policy.name, + message: `Path restriction violated: ${summarizePath(filePath)} matches blocked path(s): ${blocked.join(", ")}`, + severity: policy.severity, + }); + } } } - if ( - policy.config.type === PolicyType.FileLimitCount && - (toolName === "Edit" || toolName === "Write") && - typeof toolInput["file_path"] === "string" - ) { - const filePath = toolInput["file_path"] as string; - const config = policy.config as FileLimitCountConfig; - const currentFiles = context?.editedFiles ?? new Set(); - if (!currentFiles.has(filePath) && currentFiles.size >= config.maxFiles) { - violations.push({ - policy: policy.name, - message: `File limit exceeded: editing "${summarizePath(filePath)}" would be file ${currentFiles.size + 1}, limit is ${config.maxFiles}`, - severity: policy.severity, - }); + if (policy.config.type === PolicyType.FileLimitCount) { + const filePath = fileWriteTargetPath(toolName, toolInput); + if (filePath !== null) { + const config = policy.config as FileLimitCountConfig; + const currentFiles = context?.editedFiles ?? new Set(); + if (!currentFiles.has(filePath) && currentFiles.size >= config.maxFiles) { + violations.push({ + policy: policy.name, + message: `File limit exceeded: editing "${summarizePath(filePath)}" would be file ${currentFiles.size + 1}, limit is ${config.maxFiles}`, + severity: policy.severity, + }); + } } } - if ( - policy.config.type === PolicyType.SecretDetection && - (toolName === "Write" || toolName === "Edit") - ) { - const content = toolName === "Write" - ? (typeof toolInput["content"] === "string" ? toolInput["content"] as string : "") - : (typeof toolInput["new_string"] === "string" ? toolInput["new_string"] as string : ""); + if (policy.config.type === PolicyType.SecretDetection) { + const content = scannableToolContent(toolName, toolInput); if (content) { const config = policy.config as SecretDetectionConfig; @@ -530,7 +629,10 @@ export function evaluatePreToolPolicies( if ( policy.config.type === PolicyType.BranchProtection && - (toolName === "Write" || toolName === "Edit" || toolName === "Bash") + (toolName === "Write" || + toolName === "Edit" || + toolName === "NotebookEdit" || + toolName === "Bash") ) { const branch = context?.branch; if (branch) { diff --git a/packages/web/src/lib/__tests__/policy-summary.test.ts b/packages/web/src/lib/__tests__/policy-summary.test.ts index de05f40..0545ab3 100644 --- a/packages/web/src/lib/__tests__/policy-summary.test.ts +++ b/packages/web/src/lib/__tests__/policy-summary.test.ts @@ -47,35 +47,38 @@ describe("summarizePolicyConfig", () => { // TestEnforcement describe("TestEnforcement", () => { - it("shows both require-passing and min-coverage", () => { + it("shows require-passing", () => { expect( summarizePolicyConfig({ type: PolicyType.TestEnforcement, requirePassing: true, - minCoverage: 80, - }), - ).toBe("tests must pass, min 80% coverage"); - }); - - it("shows only require-passing when minCoverage is 0", () => { - expect( - summarizePolicyConfig({ - type: PolicyType.TestEnforcement, - requirePassing: true, - minCoverage: 0, }), ).toBe("tests must pass"); }); - it("returns No requirements when both flags are off", () => { + it("returns No requirements when require-passing is off", () => { expect( summarizePolicyConfig({ type: PolicyType.TestEnforcement, requirePassing: false, - minCoverage: 0, }), ).toBe("No requirements"); }); + + it("never claims coverage enforcement, even for legacy rows with minCoverage in the stored config", () => { + // Pre-removal DB rows can still carry minCoverage in their JSON blob. + // The summary must not surface it — coverage is not enforced anywhere. + const legacyConfig = { + type: PolicyType.TestEnforcement, + requirePassing: true, + minCoverage: 80, + }; + expect( + summarizePolicyConfig( + legacyConfig as unknown as Parameters[0], + ), + ).toBe("tests must pass"); + }); }); // RiskyOpFlag diff --git a/packages/web/src/lib/policy-summary.ts b/packages/web/src/lib/policy-summary.ts index 5487eb9..7acad5b 100644 --- a/packages/web/src/lib/policy-summary.ts +++ b/packages/web/src/lib/policy-summary.ts @@ -16,13 +16,11 @@ export function summarizePolicyConfig(config: PolicyConfig): string { : `Block paths: ${config.blockedPaths.join(", ")}`; case PolicyType.FileLimitCount: return `Max ${config.maxFiles} file${config.maxFiles === 1 ? "" : "s"} per session`; - case PolicyType.TestEnforcement: { - const parts = []; - if (config.requirePassing) parts.push("tests must pass"); - if (config.minCoverage > 0) - parts.push(`min ${config.minCoverage}% coverage`); - return parts.length > 0 ? parts.join(", ") : "No requirements"; - } + case PolicyType.TestEnforcement: + // minCoverage was removed from the config: it was rendered here but + // never enforced anywhere (no coverage data exists on a Run), so the + // summary was claiming enforcement that couldn't happen. + return config.requirePassing ? "tests must pass" : "No requirements"; case PolicyType.RiskyOpFlag: return config.riskyPatterns.length === 0 ? "No patterns"