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"