diff --git a/CHANGELOG.md b/CHANGELOG.md index 13b7955a2..bd885f258 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -4,6 +4,12 @@ ### Features +- Make secret detection *prevent* something. The five `sanitize-*` policies run on `PostToolUse`, which replaces the tool result on Codex and Copilot and is observation-only on the other ten CLIs — so on Claude Code a firing sanitizer meant the model read the secret **and** a note saying it had been blocked. Two new policies run at `PreToolUse`, which blocks on all twelve: **`block-secret-in-write`** stops a recognised key or token being written into a file's contents (nothing scanned write content before — `warn-large-file-write` read it only to measure its length), and **`block-credential-files`** stops reads and writes of SSH private keys, `~/.aws/credentials`, `.git-credentials`, `.netrc`, `.pypirc`, Docker/gcloud config, GCP service-account keys, keystores and GnuPG material, none of which any default-on policy covered. Both are in the recommended baseline. `block-secret-in-write` reads the new content and never `old_string`, so the Edit that *removes* a leaked key is still allowed — a whole-payload scan would have blocked the remediation — and Bash stays out of scope because grepping for the pattern is how you find the leak. It also scans every string field rather than a canonical `content` key, because only three of the twelve CLIs canonicalise one, and a policy that silently sees nothing on the rest while reading "enabled" is the failure this codebase has been burned by before. (#PR) + +- Add roughly 25 credential formats and the tier that decides which of them may deny. Slack (tokens and webhook URLs), GitLab, npm, PyPI, the four non-`ghp_` GitHub token types, AWS `ASIA` and secret access keys, Google OAuth, Azure storage, Supabase, Square, Shopify, Telegram, SendGrid, Hugging Face, Vault, Doppler, Linear, Notion, Figma, Postman, and HTTP Basic. Shapes that collide with things an agent reads constantly — Twilio's 32-hex SIDs, Discord's prefix-less base64, Sentry DSNs — are redact-only and cannot reach a blocking policy, enforced by a test rather than a convention. `sanitize-private-key-content` also learns the PGP header, whose trailing `BLOCK` had put it outside the pattern. (#PR) + +- Add `warn-assigned-secret`, off by default: a credential-named variable assigned a literal value, which no vendor prefix can match. It instructs rather than denies, and its name tuning is taken from the daemon's redactor rather than the audit one — the two disagree on 7 of 12 common names, and the audit version fires on a bare `key=`, which is React's prop on every JSX list. (#PR) + - Give the canary box three images instead of one, and bake the commit into each. `failproofai-canary`, `failproofai-translate` and `failproofai-docs-audit` replace the single shared toolchain image, and each carries the checkout, its dependencies and its build products — the canary also carries a compiled `failproofaid`, which it used to cross-compile in a sibling rust container on every run. What happened at 02:00 and 11:00 in front of nobody — clone, fetch, checkout, `bun install`, two `bun build`s, a `cargo build` — happens in CI now, once per commit, where a failure is a red build rather than a night with no report. Installing the twelve vendor CLIs @latest deliberately stays at run time: that is the measurement, not setup. **Only the canary carries a docker client**, and only its cron line mounts the socket — the other two spawn nothing, and an image without the client cannot be talked into reaching the host daemon. That split is the reason for three images rather than one, and it is asserted rather than described. (#PR) - Give the canary box three images instead of one, and bake the commit into each. `failproofai-canary`, `failproofai-translate` and `failproofai-docs-audit` replace the single shared toolchain image, and each carries the checkout, its dependencies and its build products — the canary also carries a compiled `failproofaid`, which it used to cross-compile in a sibling rust container on every run. What happened at 02:00 and 11:00 in front of nobody — clone, fetch, checkout, `bun install`, two `bun build`s, a `cargo build` — happens in CI now, once per commit, where a failure is a red build rather than a night with no report. Installing the twelve vendor CLIs @latest deliberately stays at run time: that is the measurement, not setup. **Only the canary carries a docker client**, and only its cron line mounts the socket — the other two spawn nothing, and an image without the client cannot be talked into reaching the host daemon. That split is the reason for three images rather than one, and it is asserted rather than described. (#705) @@ -14,6 +20,14 @@ ### Fixes +- Stop denying tool calls over things that are not credentials. The blocking patterns had no token boundaries, so `sk-` matched inside `risk-averse` and a `kubectl get pods -n risk-scoring` was refused as an "OpenAI API key"; formats with a fixed length matched past it, so `ghp_` plus 40 characters passed as a 36-character token and left four characters of a real one unredacted in audit digests. Both are anchored now, using the same character class the daemon's redactor uses so the two agree on what a boundary is. AWS's documentation keys are allowlisted by exact value — they are correctly shaped, they appear in roughly every AWS tutorial and README, and no anchoring can tell them apart from the real thing. (#PR) + +- Stop one malformed config line from denying every tool call on the machine. `sanitize-api-keys.additionalPatterns` compiled user regexes with no validation, and `new RegExp(undefined)` — what a bare `["foo"]` entry destructures to, which is exactly the shape the identically-named parameter on `block-secrets-write` takes — is `/(?:)/`, which matches everything. Nothing threw, so the `try/catch` never fired. Entries are now shape-checked, patterns that match the empty string or carry nested quantifiers are rejected rather than run, sources and counts are capped, the haystack is bounded, and compiled patterns are cached. `block-secrets-write` skips non-string entries instead of throwing, which the evaluator would have swallowed into a silently disabled policy. (#PR) + +- Stop the scrubber describing what it scrubbed. On Codex and Copilot a `PostToolUse` deny replaces the tool result, and the text sent was "Blocked Bash by failproofai because: JWT token detected in tool output" — announcing the secret in place of it. `PolicyResult.message`, which the five sanitizers have always set to `[REDACTED: … removed by failproofai]` and which nothing had ever read, is now that replacement text. (#PR) + +- Close three gaps found while covering the above: `.envrc` was readable though direnv files hold what `.env` holds; `block-secrets-write` gated `Write` but not `Edit`, matched only `id_rsa` among key names ed25519 has long since displaced, and blocked `id_rsa.pub`, which exists to be handed out; and `block-self-pause` had no `SIGNAL_MAP` entry, so an agent pausing its own enforcement counted for nothing in the audit's archetype. A test now asserts every policy and detector is mapped, and another reads `PREFIX_RULES` out of the Rust collector to assert the engine and the daemon recognise the same vendor prefixes — they had already diverged in both directions. (#PR) + - Let the box pick the translation model per tier, and stop a re-install double-scheduling the box. `getModelForTier` now reads `TRANSLATE_MODEL_TIER1` / `TRANSLATE_MODEL_TIER23`, so the seven languages most readers actually arrive in can keep a strong model while the long tail runs on something cheap — the CLI's `--model` flag flattens every tier to one model, which is the opposite of what the tier split exists for. Any id the gateway serves over the Anthropic `/v1/messages` shape works, since that is the API the translator speaks (verified: `deepseek-v4-pro` and `deepseek-v4-flash` both answer there). Separately, `install.sh` now strips the pre-marker cron form as well as its own marker: a box set up before the marker existed carries a long-form inline `docker run … -e CANARY_JOB=` line, and matching only the marker left it in place — six entries, every job scheduled twice, one on the old image and one on the new. The per-job flock keeps that from doing damage and turns it into something worse to diagnose: which image runs becomes a coin toss. Found on the real box, whose crontab is exactly that shape. (#705) - Make a non-PASS canary verdict explain itself. `probe-cli.sh` captured each agent's stdout and stderr into `$OUTA`/`$OUTB`, used them for two greps, and threw them away; `run.sh` then echoed `tail -20` of the probe on any non-PASS verdict — and the last 20 lines of that probe are the verdict block, so the log restated the verdict instead of giving the cause. Four CLIs sat yellow on the box for three consecutive days with nothing recorded anywhere but the word INCONCLUSIVE, and re-running produced the same nothing because the evidence was discarded both times. Each failing probe now prints the last 25 lines of what the CLI actually said, plus whether a hook fired at all, and the tail window widens to 80 so the explanation lands inside it. The daemon note is corrected in the same breath: `daemon: routed, no fail-closed denies` was printed whenever the grep for `daemon-unreachable` found nothing, which is equally what **no hook log at all** looks like — a run where the daemon was never asked anything now says so instead of claiming a real evaluation. (#705) diff --git a/__tests__/audit/redact-example.test.ts b/__tests__/audit/redact-example.test.ts index ded6ce649..49e93361e 100644 --- a/__tests__/audit/redact-example.test.ts +++ b/__tests__/audit/redact-example.test.ts @@ -213,19 +213,24 @@ describe("maskAssignedSecrets — the shape the blocking patterns do not carry", // is `export VAR=…`, whose example is the WHOLE command. Every one of these // reached the server and the email verbatim before this masking existed: // `SECRET_PATTERNS` matches vendor prefixes, not assignments. + // Split by which pass is expected to fire, because that is now a real + // distinction: several of these vendor prefixes moved INTO SECRET_PATTERNS, + // so `maskSecrets` reaches them first and names them specifically. The + // invariant both groups share — the value does not survive — is asserted for + // every row; only the label differs. it("masks the value of an assignment whose name says it is a credential", () => { - const cases = [ + // No recognisable vendor shape, so the name-based pass is the ONLY thing + // standing between these and the digest. This is what maskAssignedSecrets + // exists for and the group that must keep its specific label. + const noVendorShape = [ "export DATABASE_PASSWORD=hunter2-prod-acme", - "export SLACK_BOT_TOKEN=xoxb-2314-4432-aBcDeFgHiJkLmNoPqRsTuVwX", - "export HF_TOKEN=hf_AbCdEfGhIjKlMnOpQrStUvWxYz012345", - "export NPM_TOKEN=npm_aBcDeFgHiJkLmNoPqRsTuVwXyZ0123456789", - "export GITLAB_TOKEN=glpat-AbCdEfGhIjKlMnOpQr", + // 38 chars, not AWS's 40, so the vendor rule correctly declines it. "export AWS_SECRET_ACCESS_KEY=wJalrXUtnFEMIK7MDENGbPxRfiCYEXAMPLEKEY", "FOO_SECRET=abc123 ./run.sh", "PGPASSWORD=letmein psql -h prod", "npm config set _authToken=abcdef123456", ]; - for (const input of cases) { + for (const input of noVendorShape) { const out = redactExample(input, HOME); expect(out, input).toContain("[REDACTED: assigned secret]"); // The secret itself must be gone; the NAME is kept on purpose, because @@ -235,6 +240,26 @@ describe("maskAssignedSecrets — the shape the blocking patterns do not carry", } }); + it("masks a vendor-shaped assignment value, naming the vendor", () => { + // Same threat, better label. `maskAssignedSecrets` deliberately declines to + // re-mask a value an earlier pass already named — re-masking would downgrade + // "Slack token" to the generic label and strip the marker's own tail. + const vendorShaped: Array<[string, string]> = [ + ["export SLACK_BOT_TOKEN=xoxb-2314-4432-aBcDeFgHiJkLmNoPqRsTuVwX", "Slack token"], + ["export HF_TOKEN=hf_AbCdEfGhIjKlMnOpQrStUvWxYz012345", "Hugging Face token"], + ["export NPM_TOKEN=npm_aBcDeFgHiJkLmNoPqRsTuVwXyZ0123456789", "npm access token"], + ["export GITLAB_TOKEN=glpat-AbCdEfGhIjKlMnOpQr", "GitLab personal access token"], + ]; + for (const [input, label] of vendorShaped) { + const out = redactExample(input, HOME); + expect(out, input).toContain(`[REDACTED: ${label}]`); + const value = input.split("=")[1].split(" ")[0]; + expect(out, input).not.toContain(value); + // The variable name survives either way — that is the actionable half. + expect(out, input).toContain(input.split("=")[0].split(" ").pop()!); + } + }); + it("keeps the variable name, so the digest still says what was exposed", () => { expect(redactExample("export DATABASE_PASSWORD=hunter2", HOME)).toBe( "export DATABASE_PASSWORD=[REDACTED: assigned secret]", diff --git a/__tests__/audit/signal-map-coverage.test.ts b/__tests__/audit/signal-map-coverage.test.ts new file mode 100644 index 000000000..42ad11708 --- /dev/null +++ b/__tests__/audit/signal-map-coverage.test.ts @@ -0,0 +1,73 @@ +/** + * SIGNAL_MAP must name every builtin policy and every audit-only detector. + * + * The failure this prevents is silent in both directions and visible in + * neither: a policy with no entry contributes nothing to the archetype + * classifier, so an agent that trips it repeatedly is described as though it + * never did — and an entry naming a policy that no longer exists is dead weight + * that reads as coverage. `src/audit/features.ts` asserted this in prose ("every + * one of the 39 builtin policies…"), which had already drifted by four policies + * before this test existed. + */ +import { describe, it, expect } from "vitest"; +import { SIGNAL_MAP, shortName } from "../../src/audit/features"; +import { BUILTIN_POLICIES } from "../../src/hooks/builtin-policies"; +import { AUDIT_DETECTORS } from "../../src/audit/detectors"; + +/** + * Policies deliberately left unmapped, each with the reason it is excluded. + * + * `block-read-outside-cwd` is off by default and fires on ambient absolute-path + * reads present in essentially every session. The audit replay force-registers + * every builtin regardless of config, so mapping it made it ≈37% of all signal + * and collapsed the population onto "the explorer" — the regression + * __tests__/audit/distribution.test.ts exists to hold down. + */ +const INTENTIONALLY_UNMAPPED = new Set(["block-read-outside-cwd"]); + +describe("SIGNAL_MAP coverage", () => { + it("maps every builtin policy exactly once", () => { + const missing = BUILTIN_POLICIES + .map((p) => shortName(p.name)) + .filter((n) => !INTENTIONALLY_UNMAPPED.has(n) && !(n in SIGNAL_MAP)); + expect( + missing, + `builtin policies with no SIGNAL_MAP entry — they will fire in audits and ` + + `contribute nothing to the archetype: ${missing.join(", ")}`, + ).toEqual([]); + }); + + it("maps every audit-only detector", () => { + const missing = AUDIT_DETECTORS + .map((d) => d.name) + .filter((n) => !(n in SIGNAL_MAP)); + expect(missing, `detectors with no SIGNAL_MAP entry: ${missing.join(", ")}`).toEqual([]); + }); + + it("names nothing that no longer exists", () => { + const known = new Set([ + ...BUILTIN_POLICIES.map((p) => shortName(p.name)), + ...AUDIT_DETECTORS.map((d) => d.name), + ]); + const orphans = Object.keys(SIGNAL_MAP).filter((n) => !known.has(n)); + expect( + orphans, + `SIGNAL_MAP names these, but no policy or detector does: ${orphans.join(", ")}`, + ).toEqual([]); + }); + + it("keeps the intentional exclusions real", () => { + const known = new Set(BUILTIN_POLICIES.map((p) => shortName(p.name))); + for (const name of INTENTIONALLY_UNMAPPED) { + expect(known.has(name), `${name} is excluded but no longer exists`).toBe(true); + expect(name in SIGNAL_MAP, `${name} is excluded but IS mapped`).toBe(false); + } + }); + + it("gives every entry a positive weight and a real archetype", () => { + for (const [name, entry] of Object.entries(SIGNAL_MAP)) { + expect(entry.weight, `${name} weight`).toBeGreaterThan(0); + expect(typeof entry.archetype, `${name} archetype`).toBe("string"); + } + }); +}); diff --git a/__tests__/e2e/helpers/hook-runner.ts b/__tests__/e2e/helpers/hook-runner.ts index 7252ebf43..c6b24b4cc 100644 --- a/__tests__/e2e/helpers/hook-runner.ts +++ b/__tests__/e2e/helpers/hook-runner.ts @@ -117,10 +117,19 @@ export function assertPostToolUseDeny(result: HookRunResult): void { * arrived would go green if a CLI were wired to the wrong one, in either * direction, which is precisely the regression these tests exist to catch. */ -export function assertPostToolUseBlockDecision(result: HookRunResult): void { +export function assertPostToolUseBlockDecision( + result: HookRunResult, + expectedReason?: RegExp, +): void { expect(result.exitCode).toBe(0); expect(result.parsed?.decision).toBe("block"); - expect(result.parsed?.reason).toMatch(/Blocked/i); + // On these two CLIs `reason` REPLACES the tool result the model reads, so its + // text is policy-specific — a sanitize-* policy sends its redaction marker, + // everything else sends the blocked message. What is invariant, and what + // copilot's `vK` guard fails closed on, is that it is a non-empty STRING. + expect(typeof result.parsed?.reason).toBe("string"); + expect((result.parsed?.reason as string).length).toBeGreaterThan(0); + if (expectedReason) expect(result.parsed?.reason).toMatch(expectedReason); // The nested shape must be ABSENT, not merely ignored: copilot's shipped // guard reads only the top level, so emitting both would leave the file // asserting a contract no consumer actually exercises. diff --git a/__tests__/e2e/hooks/builtin-policies.e2e.test.ts b/__tests__/e2e/hooks/builtin-policies.e2e.test.ts index 3aeefb706..e93a02822 100644 --- a/__tests__/e2e/hooks/builtin-policies.e2e.test.ts +++ b/__tests__/e2e/hooks/builtin-policies.e2e.test.ts @@ -330,10 +330,23 @@ describe("block-env-files", () => { assertPreToolUseDeny(result); }); - it("allows .envrc (different suffix)", () => { + // .envrc USED to be allowed here, on the reasoning that it is a different + // suffix. It is a direnv file and holds the same credentials .env holds, so + // the suffix was the only thing separating it from the rule that exists to + // protect exactly this content. + it("blocks .envrc, which holds the same thing", () => { const env = createFixtureEnv(); env.writeConfig({ enabledPolicies: ["block-env-files"] }); const result = runHook("PreToolUse", Payloads.preToolUse.bash("cat .envrc", env.cwd), { homeDir: env.home }); + assertPreToolUseDeny(result); + }); + + // The original point of the case above — that the rule does not swallow every + // name beginning with .env — still needs holding down. + it("allows a file whose name merely starts with .env", () => { + const env = createFixtureEnv(); + env.writeConfig({ enabledPolicies: ["block-env-files"] }); + const result = runHook("PreToolUse", Payloads.preToolUse.read(".environment.md", env.cwd), { homeDir: env.home }); assertAllow(result); }); }); diff --git a/__tests__/e2e/hooks/codex-integration.e2e.test.ts b/__tests__/e2e/hooks/codex-integration.e2e.test.ts index 1cf82c28a..05fa5afbf 100644 --- a/__tests__/e2e/hooks/codex-integration.e2e.test.ts +++ b/__tests__/e2e/hooks/codex-integration.e2e.test.ts @@ -108,7 +108,12 @@ describe("E2E: Codex integration — hook protocol", () => { ), { homeDir: env.home, cli: "codex" }, ); - assertPostToolUseBlockDecision(result); + // The replacement text is the redaction marker, NOT "Blocked ... because: + // JWT token detected". These are the only two CLIs where this string + // replaces the tool result, so sending the diagnosis here would hand the + // model a description of the very secret being scrubbed. + assertPostToolUseBlockDecision(result, /REDACTED/i); + expect(result.parsed?.reason).not.toMatch(/JWT token detected/i); } finally { env.cleanup(); } diff --git a/__tests__/e2e/hooks/copilot-integration.e2e.test.ts b/__tests__/e2e/hooks/copilot-integration.e2e.test.ts index 1bc0ffcc4..124045d7a 100644 --- a/__tests__/e2e/hooks/copilot-integration.e2e.test.ts +++ b/__tests__/e2e/hooks/copilot-integration.e2e.test.ts @@ -116,7 +116,12 @@ describe("E2E: Copilot integration — hook protocol", () => { ), { homeDir: env.home, cli: "copilot" }, ); - assertPostToolUseBlockDecision(result); + // The replacement text is the redaction marker, NOT "Blocked ... because: + // JWT token detected". These are the only two CLIs where this string + // replaces the tool result, so sending the diagnosis here would hand the + // model a description of the very secret being scrubbed. + assertPostToolUseBlockDecision(result, /REDACTED/i); + expect(result.parsed?.reason).not.toMatch(/JWT token detected/i); } finally { env.cleanup(); } diff --git a/__tests__/e2e/hooks/secret-prevention.e2e.test.ts b/__tests__/e2e/hooks/secret-prevention.e2e.test.ts new file mode 100644 index 000000000..1ae0c6856 --- /dev/null +++ b/__tests__/e2e/hooks/secret-prevention.e2e.test.ts @@ -0,0 +1,179 @@ +/** + * E2E for the PreToolUse secret policies — the real binary, real stdin/stdout, + * no mocks. + * + * Deliberately a separate file. builtin-policies-extended.e2e.test.ts exists + * because credential-shaped fixtures in the main suite trip the sanitize-* + * family for every other policy sharing the file, and these fixtures are + * nothing but credential-shaped strings. + */ +import { describe, it, expect } from "vitest"; +import { runHook, assertAllow, assertPreToolUseDeny } from "../helpers/hook-runner"; +import { createFixtureEnv } from "../helpers/fixture-env"; +import { Payloads } from "../helpers/payloads"; + +// Built at runtime so the literal never appears in the source of a file that +// this repo's own dogfood hooks read. +const ANTHROPIC = `sk-ant-api03-${"A".repeat(32)}`; +const GITHUB = `ghp_${"b".repeat(36)}`; + +describe("block-secret-in-write (e2e)", () => { + it("blocks a credential written into file content", () => { + const env = createFixtureEnv(); + env.writeConfig({ enabledPolicies: ["block-secret-in-write"] }); + const result = runHook( + "PreToolUse", + Payloads.preToolUse.write(`${env.cwd}/src/config.ts`, `export const key = "${ANTHROPIC}";`, env.cwd), + { homeDir: env.home }, + ); + assertPreToolUseDeny(result); + }); + + it("blocks a GitHub token just the same", () => { + const env = createFixtureEnv(); + env.writeConfig({ enabledPolicies: ["block-secret-in-write"] }); + const result = runHook( + "PreToolUse", + Payloads.preToolUse.write(`${env.cwd}/src/gh.ts`, `const t = "${GITHUB}";`, env.cwd), + { homeDir: env.home }, + ); + assertPreToolUseDeny(result); + }); + + it("allows ordinary source", () => { + const env = createFixtureEnv(); + env.writeConfig({ enabledPolicies: ["block-secret-in-write"] }); + const result = runHook( + "PreToolUse", + Payloads.preToolUse.write(`${env.cwd}/src/index.ts`, "export const risk_score = 1;\n", env.cwd), + { homeDir: env.home }, + ); + assertAllow(result); + }); + + it("skips a test-fixture path by default", () => { + const env = createFixtureEnv(); + env.writeConfig({ enabledPolicies: ["block-secret-in-write"] }); + const result = runHook( + "PreToolUse", + Payloads.preToolUse.write(`${env.cwd}/__tests__/keys.test.ts`, ANTHROPIC, env.cwd), + { homeDir: env.home }, + ); + assertAllow(result); + }); + + it("scans a test-fixture path when skipTestFixtures is off", () => { + const env = createFixtureEnv(); + env.writeConfig({ + enabledPolicies: ["block-secret-in-write"], + policyParams: { "block-secret-in-write": { skipTestFixtures: false } }, + }); + const result = runHook( + "PreToolUse", + Payloads.preToolUse.write(`${env.cwd}/__tests__/keys.test.ts`, ANTHROPIC, env.cwd), + { homeDir: env.home }, + ); + assertPreToolUseDeny(result); + }); + + it("excuses a literal listed in allowedSecretHashes", async () => { + const { createHash } = await import("node:crypto"); + const env = createFixtureEnv(); + env.writeConfig({ + enabledPolicies: ["block-secret-in-write"], + policyParams: { + "block-secret-in-write": { + allowedSecretHashes: [createHash("sha256").update(ANTHROPIC, "utf8").digest("hex")], + }, + }, + }); + const result = runHook( + "PreToolUse", + Payloads.preToolUse.write(`${env.cwd}/src/config.ts`, ANTHROPIC, env.cwd), + { homeDir: env.home }, + ); + assertAllow(result); + }); +}); + +describe("block-credential-files (e2e)", () => { + it("blocks reading an SSH private key", () => { + const env = createFixtureEnv(); + env.writeConfig({ enabledPolicies: ["block-credential-files"] }); + const result = runHook( + "PreToolUse", + Payloads.preToolUse.read(`${env.home}/.ssh/id_ed25519`, env.cwd), + { homeDir: env.home }, + ); + assertPreToolUseDeny(result); + }); + + it("blocks a bash read of ~/.aws/credentials", () => { + const env = createFixtureEnv(); + env.writeConfig({ enabledPolicies: ["block-credential-files"] }); + const result = runHook( + "PreToolUse", + Payloads.preToolUse.bash(`cat ${env.home}/.aws/credentials`, env.cwd), + { homeDir: env.home }, + ); + assertPreToolUseDeny(result); + }); + + it("allows the public half of a keypair", () => { + const env = createFixtureEnv(); + env.writeConfig({ enabledPolicies: ["block-credential-files"] }); + const result = runHook( + "PreToolUse", + Payloads.preToolUse.read(`${env.home}/.ssh/id_ed25519.pub`, env.cwd), + { homeDir: env.home }, + ); + assertAllow(result); + }); + + it("leaves .npmrc alone until strict is on", () => { + const env = createFixtureEnv(); + env.writeConfig({ enabledPolicies: ["block-credential-files"] }); + assertAllow(runHook( + "PreToolUse", + Payloads.preToolUse.read(`${env.cwd}/.npmrc`, env.cwd), + { homeDir: env.home }, + )); + + const strict = createFixtureEnv(); + strict.writeConfig({ + enabledPolicies: ["block-credential-files"], + policyParams: { "block-credential-files": { strict: true } }, + }); + assertPreToolUseDeny(runHook( + "PreToolUse", + Payloads.preToolUse.read(`${strict.cwd}/.npmrc`, strict.cwd), + { homeDir: strict.home }, + )); + }); +}); + +describe("warn-assigned-secret (e2e)", () => { + it("instructs rather than blocking, so the agent can proceed after explaining", () => { + const env = createFixtureEnv(); + env.writeConfig({ enabledPolicies: ["warn-assigned-secret"] }); + const result = runHook( + "PreToolUse", + Payloads.preToolUse.bash("export DATABASE_PASSWORD=hunter2-prod-acme", env.cwd), + { homeDir: env.home }, + ); + expect(result.exitCode).toBe(0); + const output = result.parsed?.hookSpecificOutput as Record | undefined; + expect(String(output?.additionalContext ?? "")).toContain("DATABASE_PASSWORD"); + }); + + it("stays quiet on a bare key= — React's prop, not a credential", () => { + const env = createFixtureEnv(); + env.writeConfig({ enabledPolicies: ["warn-assigned-secret"] }); + const result = runHook( + "PreToolUse", + Payloads.preToolUse.bash("grep -r key=abcdefghijklmnop src/", env.cwd), + { homeDir: env.home }, + ); + assertAllow(result); + }); +}); diff --git a/__tests__/hooks/builtin-policies.test.ts b/__tests__/hooks/builtin-policies.test.ts index 60d7f5743..67e31ed53 100644 --- a/__tests__/hooks/builtin-policies.test.ts +++ b/__tests__/hooks/builtin-policies.test.ts @@ -37,13 +37,13 @@ describe("hooks/builtin-policies", () => { }); describe("BUILTIN_POLICIES", () => { - it("has 40 built-in policies", () => { - expect(BUILTIN_POLICIES).toHaveLength(40); + it("has 43 built-in policies", () => { + expect(BUILTIN_POLICIES).toHaveLength(43); }); - it("has 12 default-enabled policies", () => { + it("has 14 default-enabled policies", () => { const defaults = BUILTIN_POLICIES.filter((p) => p.defaultEnabled); - expect(defaults).toHaveLength(12); + expect(defaults).toHaveLength(14); }); }); @@ -113,7 +113,10 @@ describe("hooks/builtin-policies", () => { ["sk-proj-AAAAAAAAAAAAAAAAAAAA", "OpenAI project API key"], ["sk-AAAAAAAAAAAAAAAAAAAA", "OpenAI API key"], ["ghp_AAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA", "GitHub personal access token"], - ["AKIAIOSFODNN7EXAMPLE", "AWS access key ID"], + // NOT AKIAIOSFODNN7EXAMPLE — that is the AWS documentation key and is now + // allowlisted on purpose. See the KNOWN_EXAMPLE_SECRETS suite below. + ["AKIAAAAAAAAAAAAAAAAA", "AWS access key ID"], + [`github_pat_${"A".repeat(82)}`, "GitHub fine-grained token"], ["sk_live_AAAAAAAAAAAAAAAAAAAAAAAA", "Stripe live secret key"], ["sk_test_AAAAAAAAAAAAAAAAAAAAAAAA", "Stripe test secret key"], ["AIzaAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAAA", "Google API key"], @@ -148,6 +151,74 @@ describe("hooks/builtin-policies", () => { const result = await policy.fn(ctx); expect(result.decision).toBe("allow"); }); + + /** + * Every row here denied a real tool call before token boundaries and exact + * lengths landed. A deny on this list is not a cosmetic miss — it stops + * work the user asked for, on a policy that is on by default. + */ + describe("does not fire on lookalikes", () => { + const notSecrets: Array<[string, string]> = [ + // `sk-` inside an ordinary hyphenated word. `\b` does NOT catch this — + // `-` is a non-word char, so \bsk- matches happily inside `risk-`. + ["kubectl get pods -n risk-averse-scoring-v2", "sk- inside risk-"], + ["df -h && du -sh disk-usage-report-2026-08", "sk- inside disk-"], + ["ansible-playbook task-runner-deployment-01", "sk- inside task-"], + // Fixed-length formats padded past their real length. + [`ghp_${"A".repeat(40)}`, "ghp_ over-length"], + [`AKIA${"B".repeat(20)}`, "AKIA over-length"], + [`AIza${"C".repeat(39)}`, "AIza over-length"], + // Correct length, but glued to a longer token on either side. + [`prefix_ghp_${"A".repeat(36)}`, "ghp_ preceded by a token char"], + [`ghp_${"A".repeat(36)}_suffix`, "ghp_ followed by a token char"], + // Plain base64 of `{"name":"foo"}` — starts eyJ, is not a JWT. + ["payload: eyJuYW1lIjoiZm9vIn0=", "base64 JSON that is not a JWT"], + ]; + + for (const [text, why] of notSecrets) { + it(`allows ${why}`, async () => { + const ctx = makeCtx({ eventType: "PostToolUse", payload: { output: text } }); + expect((await policy.fn(ctx)).decision).toBe("allow"); + }); + } + }); + + /** + * Vendor documentation keys. Correctly shaped, so boundaries cannot drop + * them — they are excluded by exact value instead. They appear in ~every + * AWS tutorial, README and test fixture, including this repo's own. + */ + describe("KNOWN_EXAMPLE_SECRETS", () => { + for (const example of ["AKIAIOSFODNN7EXAMPLE", "ASIAIOSFODNN7EXAMPLE"]) { + it(`allows the AWS documentation key ${example}`, async () => { + const ctx = makeCtx({ + eventType: "PostToolUse", + payload: { output: `aws_access_key_id = ${example}` }, + }); + expect((await policy.fn(ctx)).decision).toBe("allow"); + }); + } + + // The allowlist skips a match and keeps scanning; it must not short-circuit + // the whole payload. A digest carrying the docs key AND a live one denies. + it("still denies a real key sharing the payload with a documentation key", async () => { + const ctx = makeCtx({ + eventType: "PostToolUse", + payload: { output: "example: AKIAIOSFODNN7EXAMPLE\nreal: AKIAAAAAAAAAAAAAAAAA" }, + }); + const result = await policy.fn(ctx); + expect(result.decision).toBe("deny"); + expect(result.reason).toContain("AWS access key ID"); + }); + + it("does not allowlist a longer key that merely starts with the example", async () => { + const ctx = makeCtx({ + eventType: "PostToolUse", + payload: { output: "AKIAIOSFODNN7EXAMPLX" }, + }); + expect((await policy.fn(ctx)).decision).toBe("deny"); + }); + }); }); describe("sanitize-connection-strings", () => { @@ -200,6 +271,9 @@ describe("hooks/builtin-policies", () => { "-----BEGIN DSA PRIVATE KEY-----", "-----BEGIN OPENSSH PRIVATE KEY-----", "-----BEGIN ENCRYPTED PRIVATE KEY-----", + // The trailing BLOCK sits between KEY and the closing dashes, so this + // header fell outside the pattern entirely until `(?: BLOCK)?` landed. + "-----BEGIN PGP PRIVATE KEY BLOCK-----", ]; for (const header of keyHeaders) { @@ -493,6 +567,24 @@ describe("hooks/builtin-policies", () => { const ctx = makeCtx({ toolName: "Read", toolInput: { file_path: "/app/src/main.ts" } }); expect((await policy.fn(ctx)).decision).toBe("allow"); }); + + // direnv files hold exactly what .env holds. The old pattern required a `.` + // or end-of-string after `.env`, and `r` is neither, so these were readable. + it("blocks Read of .envrc", async () => { + const ctx = makeCtx({ toolName: "Read", toolInput: { file_path: "/app/.envrc" } }); + expect((await policy.fn(ctx)).decision).toBe("deny"); + }); + + it("blocks Bash cat .envrc", async () => { + const ctx = makeCtx({ toolName: "Bash", toolInput: { command: "cat .envrc" } }); + expect((await policy.fn(ctx)).decision).toBe("deny"); + }); + + // `.envrc` must not widen into any name merely starting with `.env`. + it("allows a file whose name only starts with .env", async () => { + const ctx = makeCtx({ toolName: "Read", toolInput: { file_path: "/app/.environment-notes" } }); + expect((await policy.fn(ctx)).decision).toBe("allow"); + }); }); describe("block-sudo", () => { @@ -2537,6 +2629,154 @@ describe("hooks/builtin-policies", () => { expect((await policy.fn(ctx)).decision).toBe("deny"); }); }); + + /** + * The failure mode here is not a missed secret, it is a bricked machine: + * a malformed entry used to compile to /(?:)/ and deny EVERY PostToolUse + * event, on both CLIs where a PostToolUse deny actually replaces the result. + */ + describe("sanitize-api-keys additionalPatterns — malformed config is inert", () => { + const policy = BUILTIN_POLICIES.find((p) => p.name === "sanitize-api-keys")!; + const clean = { eventType: "PostToolUse" as const, payload: { output: "nothing to see here" } }; + + it("does not deny everything when entries are bare strings", async () => { + // The shape block-secrets-write's identically-named param takes, which + // is exactly how a user arrives at it. Destructures to regex===undefined, + // and `new RegExp(undefined)` is /(?:)/ — it matches every payload. + const ctx = makeCtx({ ...clean, params: { additionalPatterns: ["foo"] } }); + expect((await policy.fn(ctx)).decision).toBe("allow"); + }); + + for (const [entry, why] of [ + [{}, "entry with no regex"], + [{ regex: "", label: "empty" }, "empty regex string"], + [{ regex: 42, label: "number" }, "non-string regex"], + [null, "null entry"], + ] as Array<[unknown, string]>) { + it(`ignores a ${why}`, async () => { + const ctx = makeCtx({ ...clean, params: { additionalPatterns: [entry] } }); + expect((await policy.fn(ctx)).decision).toBe("allow"); + }); + } + + it("ignores a pattern that matches the empty string", async () => { + // `a*` matches "" and therefore matches every payload ever produced. + const ctx = makeCtx({ ...clean, params: { additionalPatterns: [{ regex: "a*", label: "greedy" }] } }); + expect((await policy.fn(ctx)).decision).toBe("allow"); + }); + + it("ignores a non-array additionalPatterns", async () => { + const ctx = makeCtx({ ...clean, params: { additionalPatterns: "oops" } }); + expect((await policy.fn(ctx)).decision).toBe("allow"); + }); + + it("rejects a nested-quantifier pattern instead of running it", async () => { + // Catastrophic backtracking: this pattern against a long non-matching + // run is the textbook ReDoS. A builtin policy gets no timeout, so the + // guard has to be refusal, not interruption. + const ctx = makeCtx({ + eventType: "PostToolUse", + payload: { output: `${"a".repeat(2000)}!` }, + params: { additionalPatterns: [{ regex: "(a+)+$", label: "redos" }] }, + }); + const started = performance.now(); + expect((await policy.fn(ctx)).decision).toBe("allow"); + expect(performance.now() - started).toBeLessThan(1000); + }); + + it("ignores an over-long pattern source", async () => { + const ctx = makeCtx({ ...clean, params: { additionalPatterns: [{ regex: `x{1}${"y".repeat(300)}`, label: "long" }] } }); + expect((await policy.fn(ctx)).decision).toBe("allow"); + }); + + it("still denies on a valid entry sitting after several malformed ones", async () => { + const ctx = makeCtx({ + eventType: "PostToolUse", + payload: { output: "internal-abc123" }, + params: { + additionalPatterns: [ + "bare string", + {}, + { regex: "a*", label: "matches everything" }, + { regex: "internal-[a-z0-9]+", label: "Internal token" }, + ], + }, + }); + const result = await policy.fn(ctx); + expect(result.decision).toBe("deny"); + expect(result.reason).toContain("Internal token"); + }); + + it("falls back to a generic label when label is missing", async () => { + const ctx = makeCtx({ + eventType: "PostToolUse", + payload: { output: "internal-abc123" }, + params: { additionalPatterns: [{ regex: "internal-[a-z0-9]+" }] }, + }); + const result = await policy.fn(ctx); + expect(result.decision).toBe("deny"); + expect(result.reason).toContain("custom pattern"); + }); + }); + + describe("block-secrets-write — malformed additionalPatterns is inert", () => { + const bsw = BUILTIN_POLICIES.find((p) => p.name === "block-secrets-write")!; + + it("skips non-string entries instead of throwing", async () => { + // `filePath.includes(42)` throws, and the evaluator's per-policy catch + // would swallow it — silently disabling the policy for the whole event. + const ctx = makeCtx({ + toolName: "Write", + toolInput: { file_path: "/app/src/index.ts" }, + params: { additionalPatterns: [42, null, ""] }, + }); + expect((await bsw.fn(ctx)).decision).toBe("allow"); + }); + + it("still blocks on a valid entry after malformed ones", async () => { + const ctx = makeCtx({ + toolName: "Write", + toolInput: { file_path: "/app/vault/token.txt" }, + params: { additionalPatterns: [42, "vault/"] }, + }); + expect((await bsw.fn(ctx)).decision).toBe("deny"); + }); + + it("ignores a non-array additionalPatterns", async () => { + const ctx = makeCtx({ + toolName: "Write", + toolInput: { file_path: "/app/src/index.ts" }, + params: { additionalPatterns: "vault/" }, + }); + expect((await bsw.fn(ctx)).decision).toBe("allow"); + }); + }); + + describe("block-secrets-write — Edit and public keys", () => { + const bsw = BUILTIN_POLICIES.find((p) => p.name === "block-secrets-write")!; + + it("blocks an Edit to a private key, not only a Write", async () => { + const ctx = makeCtx({ toolName: "Edit", toolInput: { file_path: "/home/u/.ssh/id_rsa" } }); + expect((await bsw.fn(ctx)).decision).toBe("deny"); + }); + + it("registers for both Write and Edit", () => { + expect(bsw.match.toolNames).toEqual(["Write", "Edit"]); + }); + + // A public key is meant to be distributed. `/id_rsa/` matched id_rsa.pub. + for (const pub of ["/home/u/.ssh/id_rsa.pub", "/home/u/.ssh/id_ed25519.pub"]) { + it(`allows writing ${pub}`, async () => { + const ctx = makeCtx({ toolName: "Write", toolInput: { file_path: pub } }); + expect((await bsw.fn(ctx)).decision).toBe("allow"); + }); + } + + it("still blocks the private half next to it", async () => { + const ctx = makeCtx({ toolName: "Write", toolInput: { file_path: "/home/u/.ssh/id_ed25519" } }); + expect((await bsw.fn(ctx)).decision).toBe("deny"); + }); + }); }); describe("workflow policy metadata", () => { diff --git a/__tests__/hooks/configure-wizard.test.ts b/__tests__/hooks/configure-wizard.test.ts index 8308add51..592c82e59 100644 --- a/__tests__/hooks/configure-wizard.test.ts +++ b/__tests__/hooks/configure-wizard.test.ts @@ -418,8 +418,8 @@ describe("configure-wizard pure builders", () => { }); it("reviewLines gives a taste of the policies without listing them all", () => { - // Two names say what KIND of thing these are; naming all fifteen turned a - // four-line review into a thirteen-line one, and a screen nobody reads to + // Two names say what KIND of thing these are; naming all seventeen turned a + // four-line review into a fifteen-line one, and a screen nobody reads to // the bottom conveys less than a short one. const lines = reviewLines({ target: "user", @@ -428,12 +428,12 @@ describe("configure-wizard pure builders", () => { cwd: "/tmp/proj", }); const joined = lines.join("\n"); - expect(joined).toContain("15 enabled"); - expect(joined).toContain("block-curl-pipe-sh, block-env-files +13"); - // The other thirteen are NOT on screen. + expect(joined).toContain("17 enabled"); + expect(joined).toContain("block-credential-files, block-curl-pipe-sh +15"); + // The other fifteen are NOT on screen. expect(joined).not.toContain("sanitize-private-key-content"); // One line for the count, one for the taste — never a paragraph. - expect(lines.filter((l) => l.includes("block-curl-pipe-sh"))).toHaveLength(1); + expect(lines.filter((l) => l.includes("block-credential-files"))).toHaveLength(1); }); it("reviewLines keeps every line inside the 80-column budget", () => { diff --git a/__tests__/hooks/install-prompt.test.ts b/__tests__/hooks/install-prompt.test.ts index 5f8483772..fd97ddb80 100644 --- a/__tests__/hooks/install-prompt.test.ts +++ b/__tests__/hooks/install-prompt.test.ts @@ -34,7 +34,7 @@ describe("hooks/install-prompt", () => { expect(selected).not.toContain("block-rm-rf"); expect(selected).not.toContain("block-force-push"); expect(selected).not.toContain("block-secrets-write"); - expect(selected).toHaveLength(12); + expect(selected).toHaveLength(14); }); it("returns preSelected when stdin is not a TTY and preSelected is provided", async () => { diff --git a/__tests__/hooks/policy-evaluator.test.ts b/__tests__/hooks/policy-evaluator.test.ts index 3f98809eb..0674c2d36 100644 --- a/__tests__/hooks/policy-evaluator.test.ts +++ b/__tests__/hooks/policy-evaluator.test.ts @@ -98,6 +98,50 @@ describe("hooks/policy-evaluator", () => { }, ); + // The point of scrubbing is that the model does not read the secret OR a + // description of it. Before `message` had a consumer, codex and copilot users + // got "Blocked Read by failproofai because: JWT found" in place of the + // result — the scrubber announcing what it scrubbed. + it.each(["codex", "copilot"] as const)( + "PostToolUse deny on %s uses result.message as the replacement text when set", + async (cli) => { + registerPolicy("jwt-scrub", "desc", () => ({ + decision: "deny", + reason: "JWT token detected in tool output", + message: "[REDACTED: JWT token removed by failproofai]", + }), { events: ["PostToolUse"] }); + + const result = await evaluatePolicies("PostToolUse", { tool_name: "Read" }, { cli }); + const parsed = JSON.parse(result.stdout) as Record; + expect(parsed.decision).toBe("block"); + expect(parsed.reason).toBe("[REDACTED: JWT token removed by failproofai]"); + // The unredacted description must not travel with it. + expect(parsed.reason).not.toContain("JWT token detected"); + // `reason` on the RESULT stays the real reason — that is what the hook log + // and the audit count on; only the model-facing text is replaced. + expect(result.reason).toBe("JWT token detected in tool output"); + }, + ); + + it.each(["codex", "copilot"] as const)( + "PostToolUse deny on %s falls back to a non-empty reason when message is empty", + async (cli) => { + // copilot's guard fails CLOSED on a non-string or empty reason, so an + // empty `message` must not become the emitted text. + registerPolicy("blocker", "desc", () => ({ + decision: "deny", + reason: "nope", + message: "", + }), { events: ["PostToolUse"] }); + + const result = await evaluatePolicies("PostToolUse", { tool_name: "Read" }, { cli }); + const parsed = JSON.parse(result.stdout) as Record; + expect(typeof parsed.reason).toBe("string"); + expect((parsed.reason as string).length).toBeGreaterThan(0); + expect(parsed.reason).toContain("nope"); + }, + ); + it("PostToolUse deny on a CLI outside that pair still uses additionalContext", async () => { // Guards the blast radius of the branch above: claude reads // hookSpecificOutput here and has no top-level `decision` consumer, so diff --git a/__tests__/hooks/policy-presets.test.ts b/__tests__/hooks/policy-presets.test.ts index 80a770a5d..fa93f9789 100644 --- a/__tests__/hooks/policy-presets.test.ts +++ b/__tests__/hooks/policy-presets.test.ts @@ -63,11 +63,11 @@ describe("policy-presets", () => { }); describe("RECOMMENDED_POLICIES", () => { - it("names 15 policies and every one of them is a real non-beta builtin", () => { + it("names 17 policies and every one of them is a real non-beta builtin", () => { // The count is asserted because it is a product promise the wizard PRINTS // ("15 policies · global"). Changing the set is fine; changing it without // noticing that the screen now advertises a different number is not. - expect(RECOMMENDED_POLICIES).toHaveLength(15); + expect(RECOMMENDED_POLICIES).toHaveLength(17); for (const name of RECOMMENDED_POLICIES) { const policy = BUILTIN_POLICIES.find((p) => p.name === name); expect(policy, `${name} is not a builtin policy`).toBeDefined(); diff --git a/__tests__/hooks/secret-patterns.test.ts b/__tests__/hooks/secret-patterns.test.ts new file mode 100644 index 000000000..3b48de679 --- /dev/null +++ b/__tests__/hooks/secret-patterns.test.ts @@ -0,0 +1,174 @@ +/** + * The secret catalogue: what it covers, what it deliberately does not, and the + * boundary between the tier that can deny a tool call and the tier that cannot. + * + * Kept apart from builtin-policies.test.ts on purpose — that file's fixtures + * are shared across ~40 policies, and every credential-shaped string added + * there trips the sanitize-* family for every other suite in the file. The same + * reason __tests__/e2e/hooks/builtin-policies-extended.e2e.test.ts exists. + */ +import { describe, it, expect } from "vitest"; +import { + BUILTIN_POLICIES, + SECRET_PATTERNS, + BLOCKING_SECRET_PATTERNS, + REDACT_ONLY_PATTERNS, + KNOWN_EXAMPLE_SECRETS, +} from "../../src/hooks/builtin-policies"; +import type { PolicyContext } from "../../src/hooks/policy-types"; + +const sanitizeApiKeys = BUILTIN_POLICIES.find((p) => p.name === "sanitize-api-keys")!; + +function ctxFor(output: string): PolicyContext { + return { + eventType: "PostToolUse", + payload: { tool_name: "Bash", tool_response: { output } }, + toolName: "Bash", + toolInput: {}, + params: {}, + } as unknown as PolicyContext; +} + +async function decide(output: string) { + return await sanitizeApiKeys.fn(ctxFor(output)); +} + +/** + * Every format is asserted with its EXPECTED LABEL, not merely "something + * matched". Ordering in API_KEY_PATTERNS is load-bearing — a generic rule + * placed above a specific one reports an Anthropic key as an OpenAI one — and + * a bare "did it match" assertion cannot see that regression at all. + */ +const VENDOR_CASES: Array<[string, string, string]> = [ + // [label, sample, why this shape] + ["Anthropic API key", `sk-ant-api03-${"A".repeat(24)}`, "Anthropic, specific before generic sk-"], + ["OpenAI project API key", `sk-proj-${"A".repeat(24)}`, "OpenAI project"], + ["OpenAI service account key", `sk-svcacct-${"A".repeat(24)}`, "OpenAI service account"], + ["OpenAI admin key", `sk-admin-${"A".repeat(24)}`, "OpenAI admin"], + ["OpenAI API key", `sk-${"A".repeat(24)}`, "OpenAI generic"], + ["GitHub personal access token", `ghp_${"A".repeat(36)}`, "classic PAT"], + ["GitHub OAuth token", `gho_${"A".repeat(36)}`, "OAuth"], + ["GitHub user-to-server token", `ghu_${"A".repeat(36)}`, "user-to-server"], + ["GitHub server-to-server token", `ghs_${"A".repeat(36)}`, "server-to-server"], + ["GitHub refresh token", `ghr_${"A".repeat(36)}`, "refresh"], + ["GitHub fine-grained token", `github_pat_${"A".repeat(82)}`, "fine-grained"], + ["GitLab personal access token", `glpat-${"A".repeat(20)}`, "GitLab PAT"], + ["AWS access key ID", `AKIA${"B".repeat(16)}`, "long-lived AKID"], + ["AWS temporary access key ID", `ASIA${"B".repeat(16)}`, "STS AKID"], + ["AWS secret access key", `aws_secret_access_key = ${"a".repeat(40)}`, "name-anchored, no prefix of its own"], + ["Google API key", `AIza${"C".repeat(35)}`, "Google API"], + ["Google OAuth client secret", `GOCSPX-${"D".repeat(24)}`, "Google OAuth"], + ["Azure storage account key", `AccountKey=${"e".repeat(86)}==`, "Azure storage"], + ["Stripe live secret key", `sk_live_${"A".repeat(24)}`, "Stripe live"], + ["Stripe test secret key", `sk_test_${"A".repeat(24)}`, "Stripe test"], + ["Square access token", `sq0atp-${"A".repeat(22)}`, "Square"], + ["Shopify access token", `shpat_${"a1".repeat(16)}`, "Shopify"], + ["Slack token", `xoxb-2314-4432-${"A".repeat(24)}`, "Slack bot"], + ["Slack webhook URL", `https://hooks.slack.com/services/T00000000/B00000000/${"X".repeat(24)}`, "webhook is itself a credential"], + ["Telegram bot token", `123456789:AA${"H".repeat(33)}`, "Telegram bot"], + ["npm access token", `npm_${"A".repeat(36)}`, "npm"], + ["PyPI API token", `pypi-AgEIcHlwaS5vcmc${"A".repeat(50)}`, "PyPI"], + ["HashiCorp Vault token", `hvs.${"A".repeat(24)}`, "Vault"], + ["Doppler token", `dp.pt.${"A".repeat(40)}`, "Doppler"], + ["Linear API key", `lin_api_${"A".repeat(40)}`, "Linear"], + ["Notion integration token", `ntn_${"A".repeat(40)}`, "Notion"], + ["Figma personal access token", `figd_${"A".repeat(40)}`, "Figma"], + ["Postman API key", `PMAK-${"a".repeat(24)}-${"b".repeat(34)}`, "Postman"], + ["Hugging Face token", `hf_${"A".repeat(34)}`, "Hugging Face"], + ["SendGrid API key", `SG.${"A".repeat(22)}.${"B".repeat(43)}`, "SendGrid"], + ["HTTP basic auth credentials", "Authorization: Basic dXNlcjpwYXNzd29yZDEyMw==", "basic auth carries a password"], +]; + +describe("secret catalogue — vendor coverage", () => { + for (const [label, sample, why] of VENDOR_CASES) { + it(`denies ${label} (${why})`, async () => { + const result = await decide(`config value: ${sample}`); + expect(result.decision, sample).toBe("deny"); + // The specific label, not just any deny — this is what catches an + // ordering regression between a generic and a specific rule. + expect(result.reason, sample).toContain(label); + }); + } + + it("covers every case above with a distinct label", () => { + const labels = VENDOR_CASES.map(([l]) => l); + expect(new Set(labels).size).toBe(labels.length); + }); +}); + +describe("secret catalogue — tiering is structural, not advisory", () => { + // If a redact-only pattern ever reaches the blocking array, an agent reading + // an ordinary git SHA starts having its tool calls denied. + it("keeps the blocking and redact-only tiers disjoint", () => { + const blocking = new Set(BLOCKING_SECRET_PATTERNS.map(([re]) => re.source)); + for (const [re, label] of REDACT_ONLY_PATTERNS) { + expect(blocking.has(re.source), `${label} must not be deny-eligible`).toBe(false); + } + }); + + it("exposes both tiers through SECRET_PATTERNS, so the redactor sees everything", () => { + const all = new Set(SECRET_PATTERNS.map(([re]) => re.source)); + for (const [re, label] of [...BLOCKING_SECRET_PATTERNS, ...REDACT_ONLY_PATTERNS]) { + expect(all.has(re.source), `${label} must be redactable`).toBe(true); + } + expect(SECRET_PATTERNS.length).toBe( + BLOCKING_SECRET_PATTERNS.length + REDACT_ONLY_PATTERNS.length, + ); + }); + + // Each of these is a real credential shape. None can carry a deny, because + // each collides with something an agent reads constantly. + const collisions: Array<[string, string]> = [ + [`SK${"a1".repeat(16)}`, "Twilio API key SID — 32 hex, same shape as a git SHA"], + [`AC${"b2".repeat(16)}`, "Twilio account SID — 32 hex"], + [`https://${"a".repeat(32)}@o123.ingest.sentry.io/456`, "Sentry DSN — semi-public by design"], + [`s.${"A".repeat(24)}`, "Vault legacy token — a two-character prefix is not evidence"], + ]; + + for (const [sample, why] of collisions) { + it(`does not deny: ${why}`, async () => { + expect((await decide(`value: ${sample}`)).decision).toBe("allow"); + }); + } + + it("still redacts those shapes in an audit digest", async () => { + const { maskSecrets } = await import("../../src/audit/redact-example"); + for (const [sample] of collisions) { + expect(maskSecrets(sample), sample).toContain("[REDACTED:"); + } + }); +}); + +describe("secret catalogue — lookalikes stay allowed", () => { + // A deny here costs the user a tool call they asked for, on a default-on + // policy. Each row is a shape that a naive version of a rule above matches. + const lookalikes: Array<[string, string]> = [ + ["deploy to prod-risk-assessment-service-v2", "sk- inside risk-"], + ["git log --oneline abcdef1234567890abcdef1234567890", "40 hex, not an AWS secret"], + [`ghp_${"A".repeat(40)}`, "ghp_ past its documented length"], + [`AKIA${"B".repeat(20)}`, "AKIA past its documented length"], + ["const key = { id: 1 }", "bare key= is React's prop, not a credential"], + ["Authorization: Bearer short", "bearer value below the minimum"], + ["-----BEGIN CERTIFICATE-----", "a certificate is not a private key"], + ["eyJuYW1lIjoiZm9vIn0=", "base64 JSON, not a JWT"], + ["npm install --save-dev typescript", "ordinary npm command"], + ["export EDITOR=vim", "an assignment with no credential in it"], + ]; + + for (const [sample, why] of lookalikes) { + it(`allows: ${why}`, async () => { + expect((await decide(sample)).decision, sample).toBe("allow"); + }); + } +}); + +describe("secret catalogue — documentation keys", () => { + it("lists the AWS docs pair", () => { + expect(KNOWN_EXAMPLE_SECRETS.has("AKIAIOSFODNN7EXAMPLE")).toBe(true); + expect(KNOWN_EXAMPLE_SECRETS.has("wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY")).toBe(true); + }); + + it("allows a payload whose only credential is a documentation key", async () => { + expect((await decide("aws_access_key_id = AKIAIOSFODNN7EXAMPLE")).decision).toBe("allow"); + }); +}); diff --git a/__tests__/hooks/secret-prefix-parity.test.ts b/__tests__/hooks/secret-prefix-parity.test.ts new file mode 100644 index 000000000..061854bcb --- /dev/null +++ b/__tests__/hooks/secret-prefix-parity.test.ts @@ -0,0 +1,133 @@ +/** + * The engine and the daemon hold two hand-written copies of one prefix list, in + * two languages, with nothing generating either. + * + * `VENDOR_PREFIXES` (src/hooks/builtin-policies.ts) is what the hook layer will + * DENY on. `PREFIX_RULES` (crates/fpai-collect/src/redact.rs) is what the + * collector SCRUBS before a transcript is serialised to the spool and uploaded. + * They were already out of step before this test existed — Rust carried + * Supabase and the four extra GitHub token types the engine had never heard of, + * while the engine carried Stripe and Google keys the daemon shipped verbatim. + * + * Each direction fails differently, so each is asserted separately: + * • engine-only → the hook stops the agent using a credential in-session, + * and the same credential still leaves the machine in telemetry. + * • daemon-only → the digest masks a key the agent was never stopped from + * using, so the two halves of the product describe the same event + * differently. + * + * Modelled on __tests__/hooks/harness-extra-paths.test.ts, which does exactly + * this for HARNESS_KEYS against crates/failproofaid/src/main.rs. + */ +import { describe, it, expect } from "vitest"; +import { readFileSync } from "node:fs"; +import { resolve } from "node:path"; +import { VENDOR_PREFIXES, BLOCKING_SECRET_PATTERNS } from "../../src/hooks/builtin-policies"; + +const REDACT_RS = resolve(__dirname, "../../crates/fpai-collect/src/redact.rs"); + +/** + * Prefixes the daemon may carry WITHOUT a matching entry here. + * + * Only for a prefix already covered by a broader engine rule, where adding it + * would be redundant rather than informative: the daemon lists `sk-ant-api` + * ahead of `sk-ant-` purely to win the label race between two rules that share + * a label anyway, and the engine's `sk-ant-` matches the same strings. + * + * This is an allowlist, not a waiver — anything else appearing in redact.rs + * fails until someone decides which side is wrong. + */ +const DAEMON_ONLY_SUBSUMED = new Set(["sk-ant-api"]); + +function rustPrefixes(): string[] { + const src = readFileSync(REDACT_RS, "utf8"); + const block = /const PREFIX_RULES: &\[PrefixRule\] = &\[([\s\S]*?)\n\];/.exec(src); + expect( + block, + "PREFIX_RULES not found in redact.rs — did it move, get renamed, or change shape?", + ).toBeTruthy(); + const found = [...block![1].matchAll(/prefix:\s*"([^"]+)"/g)].map((m) => m[1]); + expect(found.length, "parsed zero prefixes — the regex no longer matches the source"). + toBeGreaterThan(0); + return found; +} + +describe("secret prefix parity — engine vs daemon", () => { + it("finds the daemon's prefix table", () => { + expect(rustPrefixes().length).toBeGreaterThan(40); + }); + + it("scrubs on the way out everything the engine blocks in-session", () => { + const daemon = new Set(rustPrefixes()); + const missing = VENDOR_PREFIXES.filter((p) => !daemon.has(p)); + expect( + missing, + `in VENDOR_PREFIXES but not PREFIX_RULES — the engine denies these, and the ` + + `collector would upload them verbatim: ${missing.join(", ")}`, + ).toEqual([]); + }); + + it("blocks in-session everything the daemon bothers to scrub", () => { + const engine = new Set(VENDOR_PREFIXES); + const extra = rustPrefixes().filter( + (p) => !engine.has(p) && !DAEMON_ONLY_SUBSUMED.has(p), + ); + expect( + extra, + `in PREFIX_RULES but not VENDOR_PREFIXES — the digest masks these but no ` + + `policy ever denied them: ${extra.join(", ")}`, + ).toEqual([]); + }); + + it("keeps the subsumed allowlist honest — each entry starts with a real engine prefix", () => { + for (const subsumed of DAEMON_ONLY_SUBSUMED) { + const covering = VENDOR_PREFIXES.find( + (p) => subsumed.startsWith(p) && p !== subsumed, + ); + expect( + covering, + `${subsumed} is allowlisted as subsumed but no engine prefix covers it`, + ).toBeTruthy(); + } + }); +}); + +describe("secret prefix declaration is honest about the engine's own patterns", () => { + // VENDOR_PREFIXES is hand-written beside the regexes rather than derived from + // them, so it can drift from the very list it claims to summarise. A prefix + // declared here but absent from every pattern would make the parity test + // above demand a daemon rule for something the engine does not detect. + it("every declared prefix is actually detected by a blocking pattern", () => { + // Behavioural, not textual: several patterns fold their prefixes into an + // alternation (`shp(?:at|ss|ca|pa)_`, `hv[sb]\.`, `xox[baprs]-`), so the + // literal never appears in the source even though the rule matches it. + // + // Formats differ in both alphabet and length — AKIA is uppercase-only, + // Postman is hex-hyphen-hex, github_pat_ needs 82 — so rather than encode + // each one here (which would just restate the patterns), try a spread of + // plausible bodies and require that SOME sample is detected. A prefix no + // sample can trip is a prefix the engine does not really cover. + const bodies = (n: number) => [ + "a".repeat(n), "A".repeat(n), "0".repeat(n), "aB3".repeat(Math.ceil(n / 3)).slice(0, n), + ]; + const samples = (prefix: string) => { + const out: string[] = []; + for (const n of [16, 20, 22, 24, 30, 32, 34, 35, 36, 40, 50, 82]) out.push(...bodies(n).map((b) => prefix + b)); + // Postman's two hex runs joined by a hyphen. + out.push(`${prefix}${"a".repeat(24)}-${"b".repeat(34)}`); + return out; + }; + + const orphans = VENDOR_PREFIXES.filter((prefix) => + !samples(prefix).some((s) => BLOCKING_SECRET_PATTERNS.some(([re]) => re.test(s))), + ); + expect( + orphans, + `declared in VENDOR_PREFIXES but detected by no pattern at any length: ${orphans.join(", ")}`, + ).toEqual([]); + }); + + it("has no duplicate declarations", () => { + expect(new Set(VENDOR_PREFIXES).size).toBe(VENDOR_PREFIXES.length); + }); +}); diff --git a/__tests__/hooks/secret-prevention.test.ts b/__tests__/hooks/secret-prevention.test.ts new file mode 100644 index 000000000..6a5202931 --- /dev/null +++ b/__tests__/hooks/secret-prevention.test.ts @@ -0,0 +1,387 @@ +/** + * The two PreToolUse policies that make secret detection actually PREVENT + * something. + * + * Why they exist: PostToolUse `block` is honoured on codex and copilot only + * (src/hooks/enforcement-capability.ts) and even there it replaces the result + * AFTER the tool ran. On the other ten CLIs — Claude Code included — a + * sanitize-* deny is an appended note and the model reads the real output + * anyway. PreToolUse blocks on all twelve, so this is the only place a secret + * finding can stop anything. + * + * Kept out of builtin-policies.test.ts because every credential-shaped fixture + * added there trips the sanitize-* family for the other ~40 policies sharing + * the file. + */ +import { describe, it, expect } from "vitest"; +import { createHash } from "node:crypto"; +import { BUILTIN_POLICIES } from "../../src/hooks/builtin-policies"; +import type { PolicyContext } from "../../src/hooks/policy-types"; + +const inWrite = BUILTIN_POLICIES.find((p) => p.name === "block-secret-in-write")!; +const credFiles = BUILTIN_POLICIES.find((p) => p.name === "block-credential-files")!; + +const KEY = `sk-ant-api03-${"A".repeat(32)}`; +const AWS = `AKIA${"B".repeat(16)}`; + +function ctx( + toolName: string, + toolInput: Record, + params: Record = {}, +): PolicyContext { + return { + eventType: "PreToolUse", + payload: { tool_name: toolName, tool_input: toolInput }, + toolName, + toolInput, + params, + } as unknown as PolicyContext; +} + +describe("block-secret-in-write", () => { + it("is registered for Write and Edit at PreToolUse, on by default", () => { + expect(inWrite.match.events).toEqual(["PreToolUse"]); + expect(inWrite.match.toolNames).toEqual(["Write", "Edit"]); + expect(inWrite.defaultEnabled).toBe(true); + }); + + it("denies a credential in Write content", async () => { + const r = await inWrite.fn(ctx("Write", { file_path: "/app/src/config.ts", content: `export const k = "${KEY}";` })); + expect(r.decision).toBe("deny"); + expect(r.reason).toContain("Anthropic API key"); + }); + + it("denies a credential introduced by an Edit", async () => { + const r = await inWrite.fn(ctx("Edit", { + file_path: "/app/src/config.ts", + old_string: "const k = process.env.KEY;", + new_string: `const k = "${KEY}";`, + })); + expect(r.decision).toBe("deny"); + }); + + /** + * The single most important case in this file. + * + * Removing a leaked key is an Edit whose `old_string` IS the key. Scanning the + * whole payload — which is what the sanitize-* family does — denies exactly + * that edit, leaving the credential in the file with no way to take it out. + */ + describe("never blocks the remediation", () => { + it("allows an Edit that REMOVES a credential", async () => { + const r = await inWrite.fn(ctx("Edit", { + file_path: "/app/src/config.ts", + old_string: `const k = "${KEY}";`, + new_string: "const k = process.env.ANTHROPIC_API_KEY;", + })); + expect(r.decision).toBe("allow"); + }); + + it("allows an Edit that replaces one credential reference with an env lookup", async () => { + const r = await inWrite.fn(ctx("Edit", { + file_path: "/app/.config", + old_string: `aws_access_key_id = ${AWS}`, + new_string: "aws_access_key_id = ${AWS_ACCESS_KEY_ID}", + })); + expect(r.decision).toBe("allow"); + }); + + it("still denies when the credential survives into new_string", async () => { + const r = await inWrite.fn(ctx("Edit", { + file_path: "/app/src/config.ts", + old_string: `const k = "${KEY}"; // old`, + new_string: `const k = "${KEY}"; // new`, + })); + expect(r.decision).toBe("deny"); + }); + + it("does not fire on Bash, where grepping for the key is how you find it", async () => { + // Registered for Write/Edit only, but assert the guard directly too — a + // widened `toolNames` would otherwise start denying `git grep`. + expect((await inWrite.fn(ctx("Bash", { command: `git grep ${AWS}` }))).decision).toBe("allow"); + }); + + it("does not fire on Read", async () => { + expect((await inWrite.fn(ctx("Read", { file_path: "/app/.config" }))).decision).toBe("allow"); + }); + }); + + /** + * Only copilot, opencode and antigravity canonicalise a content field + * (src/hooks/types.ts) — pi, goose, hermes and openclaw map the path alone. + * Scanning every string value except the known OLD-content keys is what keeps + * this policy from being silently inert on those CLIs while the UI shows it + * enabled. + */ + describe("does not depend on a content field being canonicalised", () => { + for (const key of ["content", "file_text", "contents", "text", "CodeContent", "after", "body"]) { + it(`denies a credential arriving under \`${key}\``, async () => { + const r = await inWrite.fn(ctx("Write", { file_path: "/app/x.ts", [key]: `k = "${KEY}"` })); + expect(r.decision).toBe("deny"); + }); + } + + for (const key of ["old_string", "old_str", "before", "search"]) { + it(`ignores a credential arriving under \`${key}\` (pre-existing content)`, async () => { + const r = await inWrite.fn(ctx("Edit", { file_path: "/app/x.ts", [key]: `k = "${KEY}"`, new_string: "k = env" })); + expect(r.decision).toBe("allow"); + }); + } + + it("ignores non-string values rather than throwing", async () => { + const r = await inWrite.fn(ctx("Write", { file_path: "/app/x.ts", content: null, replace_all: true, count: 3 })); + expect(r.decision).toBe("allow"); + }); + }); + + describe("test fixtures", () => { + const fixturePaths = [ + "/repo/__tests__/hooks/secret.test.ts", + "/repo/src/thing.test.ts", + "/repo/src/thing.spec.js", + "/repo/tests/data.json", + "/repo/fixtures/keys.json", + "/repo/examples/policies/index.js", + ]; + + for (const path of fixturePaths) { + it(`skips ${path} by default`, async () => { + expect((await inWrite.fn(ctx("Write", { file_path: path, content: KEY }))).decision).toBe("allow"); + }); + } + + it("scans them when skipTestFixtures is false", async () => { + const r = await inWrite.fn(ctx("Write", { file_path: "/repo/src/a.test.ts", content: KEY }, { skipTestFixtures: false })); + expect(r.decision).toBe("deny"); + }); + + // "test" appearing anywhere in a name is not evidence of a fixture. + it("does not treat src/config.test-utils.ts as a fixture", async () => { + const r = await inWrite.fn(ctx("Write", { file_path: "/repo/src/config.test-utils.ts", content: KEY })); + expect(r.decision).toBe("deny"); + }); + + it("does not treat src/latest/index.ts as a fixture", async () => { + const r = await inWrite.fn(ctx("Write", { file_path: "/repo/src/latest/index.ts", content: KEY })); + expect(r.decision).toBe("deny"); + }); + }); + + describe("allowedSecretHashes", () => { + const hash = (v: string) => createHash("sha256").update(v, "utf8").digest("hex"); + + it("excuses a specific literal by hash", async () => { + const r = await inWrite.fn( + ctx("Write", { file_path: "/app/src/a.ts", content: KEY }, { allowedSecretHashes: [hash(KEY)] }), + ); + expect(r.decision).toBe("allow"); + }); + + it("is case-insensitive about the configured digest", async () => { + const r = await inWrite.fn( + ctx("Write", { file_path: "/app/src/a.ts", content: KEY }, { allowedSecretHashes: [hash(KEY).toUpperCase()] }), + ); + expect(r.decision).toBe("allow"); + }); + + it("does not excuse a DIFFERENT credential", async () => { + const r = await inWrite.fn( + ctx("Write", { file_path: "/app/src/a.ts", content: `sk-ant-api03-${"Z".repeat(32)}` }, { allowedSecretHashes: [hash(KEY)] }), + ); + expect(r.decision).toBe("deny"); + }); + + it("tolerates a malformed allowedSecretHashes without throwing", async () => { + const r = await inWrite.fn(ctx("Write", { file_path: "/app/src/a.ts", content: KEY }, { allowedSecretHashes: "nope" })); + expect(r.decision).toBe("deny"); + }); + + it("names the escape hatch in the denial, so the fix is discoverable", async () => { + const r = await inWrite.fn(ctx("Write", { file_path: "/app/src/a.ts", content: KEY })); + expect(r.reason).toContain("allowedSecretHashes"); + }); + }); + + describe("uses the blocking tier only", () => { + it("does not deny a Twilio-shaped hex string, which collides with a git SHA", async () => { + const r = await inWrite.fn(ctx("Write", { file_path: "/app/a.ts", content: `const sid = "AC${"a1".repeat(16)}"` })); + expect(r.decision).toBe("allow"); + }); + + it("allows ordinary source with no credential in it", async () => { + const r = await inWrite.fn(ctx("Write", { + file_path: "/app/src/index.ts", + content: "export const risk_assessment = { key: 1 };\nconst token = getToken();\n", + })); + expect(r.decision).toBe("allow"); + }); + }); +}); + +describe("block-credential-files", () => { + const blocked = [ + ["/home/u/.ssh/id_rsa", "SSH RSA private key"], + ["/home/u/.ssh/id_ed25519", "SSH ed25519 private key"], + ["/home/u/.ssh/id_ecdsa", "SSH ecdsa private key"], + ["/home/u/.aws/credentials", "AWS credentials"], + ["/home/u/.git-credentials", "stored git credentials"], + ["/home/u/.netrc", "netrc"], + ["/home/u/.pypirc", "PyPI credentials"], + ["/home/u/.docker/config.json", "Docker registry auth"], + ["/app/service-account.json", "GCP service account"], + ["/app/service-account-prod.json", "GCP service account, suffixed"], + ["/app/keystore.p12", "PKCS#12 keystore"], + ["/app/release.jks", "Java keystore"], + ["/home/u/.gnupg/secring.gpg", "GnuPG material"], + ["/home/u/.config/gcloud/credentials.db", "gcloud credentials"], + ]; + + for (const [path, why] of blocked) { + it(`blocks reading ${why}`, async () => { + const r = await credFiles.fn(ctx("Read", { file_path: path })); + expect(r.decision, path).toBe("deny"); + }); + } + + it("blocks a Bash read of an absolute credential path", async () => { + expect((await credFiles.fn(ctx("Bash", { command: "cat /home/u/.aws/credentials" }))).decision).toBe("deny"); + }); + + it("blocks a Bash read of a RELATIVE credential path", async () => { + // extractAbsolutePaths only yields absolute and ~-rooted paths, so the + // relative form needs the argv pass or `cat .aws/credentials` walks through. + expect((await credFiles.fn(ctx("Bash", { command: "cat .aws/credentials" }))).decision).toBe("deny"); + }); + + it("blocks a quoted Bash path", async () => { + expect((await credFiles.fn(ctx("Bash", { command: "cat '.git-credentials'" }))).decision).toBe("deny"); + }); + + // A public key is meant to be distributed; blocking it is pure friction. + for (const pub of ["/home/u/.ssh/id_rsa.pub", "/home/u/.ssh/id_ed25519.pub"]) { + it(`allows ${pub}`, async () => { + expect((await credFiles.fn(ctx("Read", { file_path: pub }))).decision).toBe("allow"); + }); + } + + const ordinary = [ + "/app/src/index.ts", + "/app/package.json", + "/app/README.md", + "/app/src/credentials-form.tsx", + "/app/config/netrc-parser.ts", + ]; + for (const path of ordinary) { + it(`allows ${path}`, async () => { + expect((await credFiles.fn(ctx("Read", { file_path: path }))).decision, path).toBe("allow"); + }); + } + + describe("strict tier", () => { + const strictOnly = [ + "/app/.npmrc", + "/home/u/.kube/config", + "/app/prod.tfvars", + "/app/terraform.tfstate", + "/app/secrets.yaml", + ]; + + for (const path of strictOnly) { + it(`allows ${path} by default — these frequently hold no credential`, async () => { + expect((await credFiles.fn(ctx("Read", { file_path: path }))).decision, path).toBe("allow"); + }); + + it(`blocks ${path} when strict is on`, async () => { + expect((await credFiles.fn(ctx("Read", { file_path: path }, { strict: true }))).decision, path).toBe("deny"); + }); + } + }); + + it("explains what to do instead", async () => { + const r = await credFiles.fn(ctx("Read", { file_path: "/home/u/.ssh/id_ed25519" })); + expect(r.reason).toMatch(/environment|secret store|ask the user/i); + }); +}); + +/** + * The generous tier. It returns `instruct`, never `deny`, because a name-based + * rule cannot tell a live credential from a fixture — and the repo argues that + * case at length in src/audit/redact-example.ts. Its tuning is copied from the + * daemon's redactor, NOT from `isSecretName`: the two disagree on 7 of 12 common + * names, and the redact-example version matches a bare `key=`, which the Rust + * comment records hitting React's `key` prop across 40 real transcripts. + */ +describe("warn-assigned-secret", () => { + const warn = BUILTIN_POLICIES.find((p) => p.name === "warn-assigned-secret")!; + + it("warns rather than blocks, and is off by default", () => { + expect(warn.defaultEnabled).toBe(false); + expect(warn.match.events).toEqual(["PreToolUse"]); + }); + + const flagged: Array<[string, string]> = [ + ["export DATABASE_PASSWORD=hunter2-prod-acme", "compound name, literal value"], + ["export API_KEY=abcdef1234567890", "weak name, but compound"], + ["export AUTH_TOKEN=abcdef1234567890", "token in a compound name"], + ["PGPASSWORD=letmeinplease psql -h prod", "strong name needs no compound"], + ["MY_SECRET=abcdefghijklmno ./run.sh", "secret suffix"], + ["npm config set _authToken=abcdef1234567890", "flag-style assignment"], + ]; + + for (const [command, why] of flagged) { + it(`instructs on: ${why}`, async () => { + const r = await warn.fn(ctx("Bash", { command })); + expect(r.decision, command).toBe("instruct"); + }); + } + + const ignored: Array<[string, string]> = [ + // The measured false positive the Rust tuning exists to avoid. + ["const el = ", "bare `key=` is a React prop, not a credential"], + ["const t = token=abcdefghijklmnop", "bare `token=` needs a compound name"], + ["export EDITOR=vim", "not a credential name"], + ["export PASSTHROUGH=enabled-for-now", "contains PASS but does not end in it"], + ["export API_KEY=short", "value below the minimum length"], + ["export API_KEY=$OTHER_KEY_FROM_ENV", "a reference, not a literal"], + ["export API_KEY=${VAULT_VALUE_HERE}", "a braced reference"], + ["export API_KEY=$(vault read -field=x)", "a command substitution"], + ["export API_KEY=your-key-here", "an obvious placeholder"], + ["export API_KEY=xxxxxxxxxxxxxxxx", "a redaction placeholder"], + ["export API_KEY=changeme", "the classic placeholder"], + ]; + + for (const [command, why] of ignored) { + it(`stays quiet on: ${why}`, async () => { + const r = await warn.fn(ctx("Bash", { command })); + expect(r.decision, command).toBe("allow"); + }); + } + + it("sees an assignment written into file content", async () => { + const r = await warn.fn(ctx("Write", { + file_path: "/app/config.sh", + content: "DATABASE_PASSWORD=hunter2-prod-acme\n", + })); + expect(r.decision).toBe("instruct"); + }); + + it("ignores an assignment being REMOVED by an Edit", async () => { + const r = await warn.fn(ctx("Edit", { + file_path: "/app/config.sh", + old_string: "DATABASE_PASSWORD=hunter2-prod-acme", + new_string: "DATABASE_PASSWORD=${DB_PASSWORD}", + })); + expect(r.decision).toBe("allow"); + }); + + it("does not read the file path as content", async () => { + // A path like /srv/password-reset/config.ts must not trip the name rule. + const r = await warn.fn(ctx("Write", { file_path: "/srv/password-reset/config.ts", content: "export const a = 1;" })); + expect(r.decision).toBe("allow"); + }); + + it("names the variable, so the warning says what to fix", async () => { + const r = await warn.fn(ctx("Bash", { command: "export STRIPE_SECRET=abcdefghijklmnop" })); + expect(r.reason).toContain("STRIPE_SECRET"); + }); +}); diff --git a/crates/fpai-collect/src/redact.rs b/crates/fpai-collect/src/redact.rs index f955cb00f..e60453ce6 100644 --- a/crates/fpai-collect/src/redact.rs +++ b/crates/fpai-collect/src/redact.rs @@ -69,6 +69,16 @@ const PREFIX_RULES: &[PrefixRule] = &[ min_len: 16, label: "openai-key", }, + PrefixRule { + prefix: "sk-svcacct-", + min_len: 16, + label: "openai-key", + }, + PrefixRule { + prefix: "sk-admin-", + min_len: 16, + label: "openai-key", + }, PrefixRule { prefix: "sk-", min_len: 16, @@ -104,6 +114,11 @@ const PREFIX_RULES: &[PrefixRule] = &[ min_len: 20, label: "github-token", }, + PrefixRule { + prefix: "glpat-", + min_len: 16, + label: "gitlab-token", + }, PrefixRule { prefix: "sb_secret_", min_len: 16, @@ -124,6 +139,21 @@ const PREFIX_RULES: &[PrefixRule] = &[ min_len: 16, label: "slack-token", }, + PrefixRule { + prefix: "xoxa-", + min_len: 16, + label: "slack-token", + }, + PrefixRule { + prefix: "xoxr-", + min_len: 16, + label: "slack-token", + }, + PrefixRule { + prefix: "xoxs-", + min_len: 16, + label: "slack-token", + }, PrefixRule { prefix: "AKIA", min_len: 16, @@ -134,6 +164,131 @@ const PREFIX_RULES: &[PrefixRule] = &[ min_len: 16, label: "aws-access-key-id", }, + PrefixRule { + prefix: "AIza", + min_len: 30, + label: "google-api-key", + }, + PrefixRule { + prefix: "GOCSPX-", + min_len: 16, + label: "google-oauth-secret", + }, + PrefixRule { + prefix: "sk_live_", + min_len: 20, + label: "stripe-key", + }, + PrefixRule { + prefix: "sk_test_", + min_len: 20, + label: "stripe-key", + }, + PrefixRule { + prefix: "sq0atp-", + min_len: 16, + label: "square-token", + }, + PrefixRule { + prefix: "sq0csp-", + min_len: 16, + label: "square-token", + }, + PrefixRule { + prefix: "shpat_", + min_len: 24, + label: "shopify-token", + }, + PrefixRule { + prefix: "shpss_", + min_len: 24, + label: "shopify-token", + }, + PrefixRule { + prefix: "shpca_", + min_len: 24, + label: "shopify-token", + }, + PrefixRule { + prefix: "shppa_", + min_len: 24, + label: "shopify-token", + }, + PrefixRule { + prefix: "npm_", + min_len: 30, + label: "npm-token", + }, + PrefixRule { + prefix: "pypi-", + min_len: 30, + label: "pypi-token", + }, + PrefixRule { + prefix: "hvs.", + min_len: 20, + label: "vault-token", + }, + PrefixRule { + prefix: "hvb.", + min_len: 20, + label: "vault-token", + }, + PrefixRule { + prefix: "dp.pt.", + min_len: 30, + label: "doppler-token", + }, + PrefixRule { + prefix: "dp.st.", + min_len: 30, + label: "doppler-token", + }, + PrefixRule { + prefix: "dp.sa.", + min_len: 30, + label: "doppler-token", + }, + PrefixRule { + prefix: "dp.ct.", + min_len: 30, + label: "doppler-token", + }, + PrefixRule { + prefix: "dp.scim.", + min_len: 30, + label: "doppler-token", + }, + PrefixRule { + prefix: "dp.audit.", + min_len: 30, + label: "doppler-token", + }, + PrefixRule { + prefix: "lin_api_", + min_len: 30, + label: "linear-key", + }, + PrefixRule { + prefix: "ntn_", + min_len: 30, + label: "notion-token", + }, + PrefixRule { + prefix: "figd_", + min_len: 30, + label: "figma-token", + }, + PrefixRule { + prefix: "PMAK-", + min_len: 30, + label: "postman-key", + }, + PrefixRule { + prefix: "hf_", + min_len: 24, + label: "huggingface-token", + }, ]; /// Assignment names whose value is treated as secret. diff --git a/docs/policies/builtin-catalog.mdx b/docs/policies/builtin-catalog.mdx index 2296fd769..c1996d390 100644 --- a/docs/policies/builtin-catalog.mdx +++ b/docs/policies/builtin-catalog.mdx @@ -15,6 +15,7 @@ sanitize-jwt sanitize-api-keys sanitize-connection-strings sanitize-private-key-content sanitize-bearer-tokens protect-env-vars block-env-files block-secrets-write +block-secret-in-write block-credential-files block-self-pause block-failproofai-commands block-sudo block-curl-pipe-sh block-rm-rf block-push-master @@ -27,15 +28,32 @@ Recommended is deliberately narrower than **Everything**. Infrastructure and wor | Policy | Trigger | Outcome | | --- | --- | --- | -| `sanitize-jwt` | `PostToolUse` | Redact JWTs from tool output before the model sees them. | -| `sanitize-api-keys` | `PostToolUse` | Redact common OpenAI, Anthropic, GitHub, AWS, Stripe, and Google keys. | -| `sanitize-connection-strings` | `PostToolUse` | Redact database connection strings containing credentials. | -| `sanitize-private-key-content` | `PostToolUse` | Redact PEM private-key bodies. | -| `sanitize-bearer-tokens` | `PostToolUse` | Redact authorization bearer tokens. | +| `sanitize-jwt` | `PostToolUse` | Detect JWTs in tool output. Replaces the result on Codex and Copilot; elsewhere, reports only \u2014 see the note below. | +| `sanitize-api-keys` | `PostToolUse` | Detect ~35 vendor key formats (OpenAI, Anthropic, GitHub, GitLab, AWS, Google, Azure, Stripe, Slack, npm, PyPI, Vault, and more). | +| `sanitize-connection-strings` | `PostToolUse` | Detect database connection strings containing credentials. | +| `sanitize-private-key-content` | `PostToolUse` | Detect PEM private-key bodies, including OpenSSH and PGP headers. | +| `sanitize-bearer-tokens` | `PostToolUse` | Detect authorization bearer tokens. | +| `block-secret-in-write` | `PreToolUse` on write and edit tools | Block a recognised key or token being written into file **contents**. Blocks on every supported CLI. | +| `block-credential-files` | `PreToolUse` | Block reads and writes of SSH private keys, `~/.aws/credentials`, keystores, and GnuPG material. | +| `warn-assigned-secret` | `PreToolUse` | Warn when a credential-named variable is assigned a literal value. Off by default. | | `protect-env-vars` | `PreToolUse` on shell tools | Block commands that dump environment variables. | -| `block-env-files` | `PreToolUse` | Block reads and writes of `.env` files. | +| `block-env-files` | `PreToolUse` | Block reads and writes of `.env` and `.envrc` files. | | `block-read-outside-cwd` | `PreToolUse` on read, glob, grep, or shell tools | Keep reads inside the session working directory. | -| `block-secrets-write` | `PreToolUse` on write tools | Block writes to common secret-key and credential filenames. | +| `block-secrets-write` | `PreToolUse` on write and edit tools | Block writes to common secret-key and credential filenames. | + + +**Where `PostToolUse` actually enforces.** A `PostToolUse` decision only replaces +the tool result on **Codex** and **Copilot**, and even there the tool has already +run. On the other supported CLIs \u2014 Claude Code, Cursor, OpenCode, Pi, Hermes, +OpenClaw, Factory, Devin, Antigravity, and Goose \u2014 the agent still receives the +real output alongside a note that a secret was detected. + +This is a limit of what those CLIs consume, not a configuration mistake. It is +why `block-secret-in-write` and `block-credential-files` run at `PreToolUse`, +which blocks on every supported CLI, and why both are in the recommended +baseline: they prevent the credential from being written or read in the first +place rather than trying to scrub it afterwards. + ## Dangerous commands and infrastructure @@ -96,7 +114,10 @@ Configure parameters under the selected scope's `policyParams` object. Types are | Policy | Parameter | Type and default | | --- | --- | --- | -| `sanitize-api-keys` | `additionalPatterns` | `pattern[]`, `[]`; entries contain `regex` and `label` | +| `sanitize-api-keys` | `additionalPatterns` | `pattern[]`, `[]`; entries contain `regex` and `label`. Malformed entries and patterns with nested quantifiers are logged and skipped. | +| `block-secret-in-write` | `allowedSecretHashes` | `string[]`, `[]`; SHA-256 hex digests of literals to allow. Store hashes, never the credential itself. | +| `block-secret-in-write` | `skipTestFixtures` | `boolean`, `true`; skip `__tests__/`, `fixtures/`, `examples/` and `*.test.*` paths | +| `block-credential-files` | `strict` | `boolean`, `false`; also block `.npmrc`, `kubeconfig`, `*.tfvars`, `*.tfstate`, `secrets.yaml` | | `block-read-outside-cwd` | `allowPaths` | `string[]`, `[]` | | `block-sudo` | `allowPatterns` | `string[]`, `[]` | | `block-rm-rf` | `allowPaths` | `string[]`, `[]` | diff --git a/docs/reference/policy-sdk.mdx b/docs/reference/policy-sdk.mdx index 78aad8063..a3aa158f7 100644 --- a/docs/reference/policy-sdk.mdx +++ b/docs/reference/policy-sdk.mdx @@ -131,7 +131,7 @@ const filePath = String(ctx.toolInput?.file_path ?? ""); | Event | When it runs | Typical use | | --- | --- | --- | | `PreToolUse` | Before a tool executes. | Block or guide commands, writes, reads, and external actions. | -| `PostToolUse` | After a tool returns. | Inspect results before they reach the agent. A deny blocks the whole result; it does not redact selected fields. | +| `PostToolUse` | After a tool returns. | Inspect results after the tool has run. A deny replaces the whole result on Codex and Copilot only; on other CLIs it appends a note and the agent still reads the real output. It never redacts selected fields. To prevent rather than report, use `PreToolUse`. | | `PermissionRequest` | When the agent requests permission. | Apply organization-specific permission rules. | | `UserPromptSubmit` | Before a submitted prompt continues. | Reject prohibited instructions or add workflow guidance. | | `Stop` | When the agent attempts to finish. | Require a reachable completion condition, such as a local verification step. | diff --git a/src/audit/features.ts b/src/audit/features.ts index 26c6cc81d..af9d52cb2 100644 --- a/src/audit/features.ts +++ b/src/audit/features.ts @@ -69,15 +69,26 @@ export const ARCHITECT_CAUTION_SIGNALS = new Set(["reread-after-edit", "redundan /** * Mapping from policy/detector short-name → which archetype its hits feed, - * and how heavily (intensity within the cluster). Every one of the 39 builtin - * policies and 8 audit-only detectors maps exactly once — no overlaps, full - * coverage. Weights express *severity within* a persona; cross-persona + * and how heavily (intensity within the cluster). Every builtin policy and + * audit-only detector maps exactly once — no overlaps, full coverage, with the + * single documented exception of `block-read-outside-cwd` (see the note in the + * explorer block). Weights express *severity within* a persona; cross-persona * fairness is handled later by lift normalisation, not by these numbers. + * + * That coverage claim used to be a count in this sentence, which is exactly the + * kind of thing that goes stale silently — a policy added without an entry here + * contributes nothing to the classifier and nobody finds out. It is asserted by + * `__tests__/audit/signal-map-coverage.test.ts` now, so the sentence can no + * longer drift away from the code. */ export const SIGNAL_MAP: Record = { // ── cowboy ── destructive / forceful / bypasses guardrails (20) ─────────── "block-rm-rf": { archetype: "cowboy", weight: 2.0 }, "block-failproofai-commands":{ archetype: "cowboy", weight: 2.0 }, + // Was missing entirely until the coverage test above went in: an agent + // pausing its own enforcement is the same act as running the CLI to disable + // it, and it counted for nothing in the classifier. + "block-self-pause": { archetype: "cowboy", weight: 2.0 }, "block-sudo": { archetype: "cowboy", weight: 1.5 }, "block-curl-pipe-sh": { archetype: "cowboy", weight: 1.5 }, "block-force-push": { archetype: "cowboy", weight: 1.5 }, @@ -109,6 +120,13 @@ export const SIGNAL_MAP: Record = { body: "attempts to write credential-shaped strings to files that aren't typically credential stores.", cost: "could have committed live secrets to the repo.", }, + "block-secret-in-write": { + body: "the agent tried to write a recognised API key or token into a file's contents \u2014 a hardcoded credential, not a filename.", + cost: "highest exposure of any finding here. a key in a tracked file is one commit from public, and rotation is the only fix.", + }, + "block-credential-files": { + body: "reads or writes of SSH private keys, ~/.aws/credentials, keystores and similar. these files exist to hold exactly one thing.", + cost: "high. the contents are credentials by definition, and a read puts them in the model's context.", + }, + "warn-assigned-secret": { + body: "a credential-named variable assigned a literal value \u2014 the hardcoded-secret shape that no vendor prefix matches.", + cost: "medium. some of these are placeholders; the ones that aren't are live credentials sitting in plain text.", + }, "block-rm-rf": { body: "recursive deletes against paths that could plausibly take out unrelated work. `rm -rf` is the agent's preferred way of cleaning up — even when it shouldn't be.", cost: "irreversible. one wrong path argument = lost work.", @@ -237,6 +249,18 @@ const POLICY_META: Record = { displayTitle: "Tried to write a secret-key file", impact: "blocks writes to .pem, id_rsa, credentials.json, and similar.", }, + "block-secret-in-write": { + displayTitle: "Tried to write a live credential into a file", + impact: "blocks a recognised API key or token appearing in file contents.", + }, + "block-credential-files": { + displayTitle: "Tried to read or write a credential file", + impact: "blocks SSH private keys, cloud credentials, keystores and GnuPG material.", + }, + "warn-assigned-secret": { + displayTitle: "Assigned a literal credential to a variable", + impact: "warns when a credential-named variable is given a literal value.", + }, "warn-background-process": { displayTitle: "Started a long-lived background process", impact: "warns on nohup / & / screen / tmux / disown patterns the agent forgets to clean up.", diff --git a/src/audit/strengths.ts b/src/audit/strengths.ts index d3f09db36..f05bc4e5c 100644 --- a/src/audit/strengths.ts +++ b/src/audit/strengths.ts @@ -57,7 +57,7 @@ export function deriveStrengths(result: AuditResult): Strength[] { "sanitize-api-keys", "sanitize-jwt", "sanitize-connection-strings", "sanitize-private-key-content", "sanitize-bearer-tokens", "block-env-files", "block-secrets-write", "block-read-outside-cwd", - "protect-env-vars", + "protect-env-vars", "block-secret-in-write", "block-credential-files", ]; if (hitsForShort(result, credentialPolicies) === 0) { out.push({ diff --git a/src/hooks/builtin-policies.ts b/src/hooks/builtin-policies.ts index 738e630f9..344a81c89 100644 --- a/src/hooks/builtin-policies.ts +++ b/src/hooks/builtin-policies.ts @@ -6,6 +6,7 @@ import { statSync } from "node:fs"; import { readFile, writeFile } from "node:fs/promises"; import { execSync, execFileSync } from "node:child_process"; import { homedir } from "node:os"; +import { createHash } from "node:crypto"; import type { BuiltinPolicyDefinition, PolicyContext, PolicyResult, PolicyParamsSchema } from "./policy-types"; import { allow, deny, instruct } from "./policy-helpers"; import { normalizePolicyName, registerPolicy } from "./policy-registry"; @@ -115,27 +116,173 @@ const SHELL_METACHAR_RE = /[;&<>`$()\\]/; // -- Pre-compiled regex constants (hoisted to avoid per-call allocation) -- +/** + * A token must not START inside another token, or `sk-` matches inside + * `risk-averse` and `ri` is left dangling in front of the marker. + * + * The character class is `is_token_char` from the daemon's redactor + * (crates/fpai-collect/src/redact.rs) spelled as a regex, so both sides agree on + * what a boundary is. `\b` is NOT equivalent: `-` is a non-word character, so + * `\bsk-` matches happily at the `sk` inside `risk-averse`, which is the exact + * false positive this exists to stop. Measured live: `kubectl get pods -n + * risk-scoring` denied a tool call as an "OpenAI API key". + */ +const TOK = "[A-Za-z0-9_-]"; +const NOT_AFTER_TOKEN = `(? = new Set([ + // The canonical AWS docs pair — https://docs.aws.amazon.com/ ... every page. + "AKIAIOSFODNN7EXAMPLE", + "ASIAIOSFODNN7EXAMPLE", + "wJalrXUtnFEMI/K7MDENG/bPxRfiCYEXAMPLEKEY", +]); + // sanitizeJwt -const JWT_RE = /eyJ[A-Za-z0-9_-]{10,}\.[A-Za-z0-9_-]{10,}\.[A-Za-z0-9_-]{10,}/; +const JWT_RE = new RegExp( + `${NOT_AFTER_TOKEN}eyJ[A-Za-z0-9_-]{10,}\\.[A-Za-z0-9_-]{10,}\\.[A-Za-z0-9_-]{10,}`, +); -// sanitizeApiKeys +/** + * sanitizeApiKeys. + * + * Every rule carries a leading boundary. A rule whose vendor format has a FIXED + * length carries a trailing one too — without it `ghp_` + 40 characters matched + * on its first 36, so `.test()` denied a token that is not a GitHub token and + * `maskSecrets` left the last 4 characters of a real one in the digest. + * Open-ended `{n,}` rules need no trailing guard: the quantifier is greedy and + * already runs to the end of the token. + */ const API_KEY_PATTERNS: Array<[RegExp, string]> = [ - [/sk-ant-[A-Za-z0-9\-_]{20,}/, "Anthropic API key"], - [/sk-proj-[A-Za-z0-9\-_]{20,}/, "OpenAI project API key"], - [/sk-[A-Za-z0-9]{20,}/, "OpenAI API key"], - [/ghp_[A-Za-z0-9]{36}/, "GitHub personal access token"], - [/github_pat_[A-Za-z0-9_]{82}/, "GitHub fine-grained token"], - [/AKIA[A-Z0-9]{16}/, "AWS access key ID"], - [/sk_live_[A-Za-z0-9]{24,}/, "Stripe live secret key"], - [/sk_test_[A-Za-z0-9]{24,}/, "Stripe test secret key"], - [/AIza[0-9A-Za-z\-_]{35}/, "Google API key"], + // -- OpenAI / Anthropic. Specific prefixes MUST precede the generic `sk-`, + // or an Anthropic key is reported as an OpenAI one. + [new RegExp(`${NOT_AFTER_TOKEN}sk-ant-[A-Za-z0-9\\-_]{20,}`), "Anthropic API key"], + [new RegExp(`${NOT_AFTER_TOKEN}sk-proj-[A-Za-z0-9\\-_]{20,}`), "OpenAI project API key"], + [new RegExp(`${NOT_AFTER_TOKEN}sk-svcacct-[A-Za-z0-9\\-_]{20,}`), "OpenAI service account key"], + [new RegExp(`${NOT_AFTER_TOKEN}sk-admin-[A-Za-z0-9\\-_]{20,}`), "OpenAI admin key"], + [new RegExp(`${NOT_AFTER_TOKEN}sk-[A-Za-z0-9]{20,}`), "OpenAI API key"], + // -- GitHub. ghp_ is the classic PAT; gho_/ghu_/ghs_/ghr_ are the OAuth, + // user-to-server, server-to-server and refresh variants. All are 36 after + // the prefix, all leak the same access, and only ghp_ was covered. + [new RegExp(`${NOT_AFTER_TOKEN}ghp_[A-Za-z0-9]{36}${NOT_BEFORE_TOKEN}`), "GitHub personal access token"], + [new RegExp(`${NOT_AFTER_TOKEN}gho_[A-Za-z0-9]{36}${NOT_BEFORE_TOKEN}`), "GitHub OAuth token"], + [new RegExp(`${NOT_AFTER_TOKEN}ghu_[A-Za-z0-9]{36}${NOT_BEFORE_TOKEN}`), "GitHub user-to-server token"], + [new RegExp(`${NOT_AFTER_TOKEN}ghs_[A-Za-z0-9]{36}${NOT_BEFORE_TOKEN}`), "GitHub server-to-server token"], + [new RegExp(`${NOT_AFTER_TOKEN}ghr_[A-Za-z0-9]{36}${NOT_BEFORE_TOKEN}`), "GitHub refresh token"], + [new RegExp(`${NOT_AFTER_TOKEN}github_pat_[A-Za-z0-9_]{82}${NOT_BEFORE_TOKEN}`), "GitHub fine-grained token"], + // `glpat-` is a six-character literal that cannot occur in prose or code, so + // the minimum length guards format variance, not false positives. Same class + // as hf_ / figd_ / lin_api_ — distinctive prefix, no upper bound. The exact + // lengths elsewhere (ghp_, AKIA, npm_, AIza, PMAK-) are the other class: + // stable documented formats where over-length matching was a measured bug. + [new RegExp(`${NOT_AFTER_TOKEN}glpat-[A-Za-z0-9\\-_]{16,}`), "GitLab personal access token"], + // -- AWS. ASIA is the temporary/STS form; it grants the same access for its + // lifetime. The secret access key has no prefix of its own, so it is + // anchored on the assignment name instead of matching bare base64. + [new RegExp(`${NOT_AFTER_TOKEN}AKIA[A-Z0-9]{16}${NOT_BEFORE_TOKEN}`), "AWS access key ID"], + [new RegExp(`${NOT_AFTER_TOKEN}ASIA[A-Z0-9]{16}${NOT_BEFORE_TOKEN}`), "AWS temporary access key ID"], + [/aws_secret_access_key\s*[=:]\s*["']?[A-Za-z0-9/+]{40}/i, "AWS secret access key"], + // -- Cloud providers. + [new RegExp(`${NOT_AFTER_TOKEN}AIza[0-9A-Za-z\\-_]{35}${NOT_BEFORE_TOKEN}`), "Google API key"], + [new RegExp(`${NOT_AFTER_TOKEN}GOCSPX-[A-Za-z0-9\\-_]{20,}`), "Google OAuth client secret"], + [/AccountKey=[A-Za-z0-9+/]{86}==/, "Azure storage account key"], + // -- Payments. + [new RegExp(`${NOT_AFTER_TOKEN}sk_live_[A-Za-z0-9]{24,}`), "Stripe live secret key"], + [new RegExp(`${NOT_AFTER_TOKEN}sk_test_[A-Za-z0-9]{24,}`), "Stripe test secret key"], + [new RegExp(`${NOT_AFTER_TOKEN}sq0(?:atp|csp)-[A-Za-z0-9\\-_]{22,}`), "Square access token"], + [new RegExp(`${NOT_AFTER_TOKEN}shp(?:at|ss|ca|pa)_[0-9a-fA-F]{32}${NOT_BEFORE_TOKEN}`), "Shopify access token"], + // -- Supabase. The daemon's redactor has carried these since it shipped; the + // engine did not, so a service key was denied nowhere and scrubbed only on + // the way out. That asymmetry is what the prefix-parity test now prevents. + [new RegExp(`${NOT_AFTER_TOKEN}sb_secret_[A-Za-z0-9\\-_]{16,}`), "Supabase secret key"], + [new RegExp(`${NOT_AFTER_TOKEN}sbp_[A-Za-z0-9]{20,}`), "Supabase access token"], + // -- Messaging / chat. The Slack webhook URL is a credential on its own: + // anyone holding it can post to the channel. + [new RegExp(`${NOT_AFTER_TOKEN}xox[baprs]-[A-Za-z0-9\\-]{10,}`), "Slack token"], + [/hooks\.slack\.com\/services\/T[A-Za-z0-9]+\/B[A-Za-z0-9]+\/[A-Za-z0-9]{20,}/, "Slack webhook URL"], + [new RegExp(`${NOT_AFTER_TOKEN}\\d{8,10}:AA[A-Za-z0-9\\-_]{33}${NOT_BEFORE_TOKEN}`), "Telegram bot token"], + // -- Package registries. + [new RegExp(`${NOT_AFTER_TOKEN}npm_[A-Za-z0-9]{36}${NOT_BEFORE_TOKEN}`), "npm access token"], + // `pypi-` plus the body, NOT `pypi-AgEIcHlwaS5vcmc` — that literal is the + // base64 macaroon header for pypi.org specifically, so anchoring on it missed + // every test.pypi.org token, which carries a different header and the same + // publish rights on the index it belongs to. 50 token characters after a + // five-character literal is unambiguous on its own. + [new RegExp(`${NOT_AFTER_TOKEN}pypi-[A-Za-z0-9\\-_]{50,}`), "PyPI API token"], + // -- Secret managers and SaaS. + [new RegExp(`${NOT_AFTER_TOKEN}hv[sb]\\.[A-Za-z0-9\\-_]{24,}`), "HashiCorp Vault token"], + [new RegExp(`${NOT_AFTER_TOKEN}dp\\.(?:pt|st|sa|ct|scim|audit)\\.[A-Za-z0-9]{40,}`), "Doppler token"], + [new RegExp(`${NOT_AFTER_TOKEN}lin_api_[A-Za-z0-9]{40,}`), "Linear API key"], + [new RegExp(`${NOT_AFTER_TOKEN}ntn_[A-Za-z0-9]{40,}`), "Notion integration token"], + [new RegExp(`${NOT_AFTER_TOKEN}figd_[A-Za-z0-9\\-_]{40,}`), "Figma personal access token"], + [new RegExp(`${NOT_AFTER_TOKEN}PMAK-[0-9a-f]{24}-[0-9a-f]{34}${NOT_BEFORE_TOKEN}`), "Postman API key"], + // Open-ended rather than the documented 34: `hf_` plus 30+ alphanumerics is + // not a plausible English token, so the prefix carries the evidence on its + // own, and an upper bound here would buy false NEGATIVES on format drift + // (oauth and org tokens) in exchange for precision the prefix already has. + // Contrast ghp_/AKIA, where the length is stable AND the over-length false + // positive was measured. + [new RegExp(`${NOT_AFTER_TOKEN}hf_[A-Za-z0-9]{30,}`), "Hugging Face token"], + [/SG\.[A-Za-z0-9\-_]{22}\.[A-Za-z0-9\-_]{43}/, "SendGrid API key"], + // -- HTTP. Bearer has its own policy; Basic does not, and carries a password. + [/Authorization:\s*Basic\s+[A-Za-z0-9+/=]{16,}/i, "HTTP basic auth credentials"], +]; + +/** + * Credential shapes that are REAL but too collision-prone to deny on. + * + * These reach `SECRET_PATTERNS` — so the audit redactor masks them, where the + * cost of a false positive is a few characters missing from a digest — and are + * deliberately absent from `API_KEY_PATTERNS`, so no blocking policy can ever + * reach them. That split is the whole tiering contract described on + * `SECRET_PATTERNS`, made structural rather than advisory: a pattern cannot + * deny a tool call unless someone moved it into the other array on purpose. + * + * Each entry is here for a specific collision, named, not because it is + * "lower severity": + * • Twilio's SID/key forms are `SK`/`AC` plus 32 hex — indistinguishable from + * a truncated git SHA or any hex digest. + * • A Discord token is shape-only (base64.base64.base64) with no literal + * prefix, so it matches ordinary base64. + * • A Sentry DSN is semi-public by design; it belongs in a digest, not in a + * denial. + * • Vault's legacy `s.` prefix is two characters, which is not evidence. + */ +export const REDACT_ONLY_PATTERNS: Array<[RegExp, string]> = [ + [new RegExp(`${NOT_AFTER_TOKEN}SK[0-9a-fA-F]{32}${NOT_BEFORE_TOKEN}`), "Twilio API key SID"], + [new RegExp(`${NOT_AFTER_TOKEN}AC[0-9a-fA-F]{32}${NOT_BEFORE_TOKEN}`), "Twilio account SID"], + [new RegExp(`${NOT_AFTER_TOKEN}[MNO][A-Za-z0-9\\-_]{23}\\.[A-Za-z0-9\\-_]{6}\\.[A-Za-z0-9\\-_]{27,}`), "Discord bot token"], + [/https:\/\/[0-9a-f]{32}@[a-z0-9.\-]+\/\d+/, "Sentry DSN"], + [new RegExp(`${NOT_AFTER_TOKEN}s\\.[A-Za-z0-9]{24}${NOT_BEFORE_TOKEN}`), "HashiCorp Vault legacy token"], ]; // sanitizeConnectionStrings -const CONNECTION_STRING_RE = /(?:postgresql|postgres|mysql|mongodb(?:\+srv)?|redis|amqps?|smtps?):\/\/[^@\s]+@/; +const CONNECTION_STRING_RE = new RegExp( + `${NOT_AFTER_TOKEN}(?:postgresql|postgres|mysql|mongodb(?:\\+srv)?|redis|amqps?|smtps?)://[^@\\s]+@`, +); -// sanitizePrivateKeyContent -const PRIVATE_KEY_RE = /-----BEGIN (?:[A-Z]+ )?PRIVATE KEY-----/; +/** + * sanitizePrivateKeyContent. + * + * `(?:[A-Z0-9]+ )*` rather than `(?:[A-Z]+ )?` so multi-word and digit-bearing + * labels are covered, and `(?: BLOCK)?` for PGP — `-----BEGIN PGP PRIVATE KEY + * BLOCK-----` was the one PEM header this missed, because the trailing `BLOCK` + * sits between `KEY` and the closing dashes. `BEGIN CERTIFICATE` still does not + * match: the optional group can match nothing, but `PRIVATE KEY` is required. + */ +const PRIVATE_KEY_RE = /-----BEGIN (?:[A-Z0-9]+ )*PRIVATE KEY(?: BLOCK)?-----/; // sanitizeBearerTokens const BEARER_TOKEN_RE = /Authorization:\s*Bearer\s+[A-Za-z0-9\-._~+/]{20,}/i; @@ -164,6 +311,63 @@ export const SECRET_PATTERNS: ReadonlyArray = [ [BEARER_TOKEN_RE, "bearer token"], [CONNECTION_STRING_RE, "database credentials"], ...API_KEY_PATTERNS, + // The redact-only tier is included HERE and nowhere the blocking policies can + // reach. See REDACT_ONLY_PATTERNS for why each one cannot carry a deny. + ...REDACT_ONLY_PATTERNS, +]; + +/** + * The literal vendor prefixes the blocking tier recognises. + * + * Declared rather than derived, because it is one half of a contract with code + * in another language: `PREFIX_RULES` in crates/fpai-collect/src/redact.rs is + * the daemon's copy, hand-written, with no generator on either side. + * `__tests__/hooks/secret-prefix-parity.test.ts` reads the Rust source and + * asserts the two agree — the same technique + * `__tests__/hooks/harness-extra-paths.test.ts` uses for `HARNESS_KEYS`. + * + * The two directions fail differently, which is why the test checks both: + * • in the engine, not the daemon → the hook denies a credential in-session + * that still leaves the machine verbatim in telemetry. + * • in the daemon, not the engine → the digest masks something the agent was + * never stopped from using, so the two halves tell the user different + * stories about the same key. + * + * ONLY single-token literal prefixes belong here. Structural rules (JWT, + * bearer, connection strings, the Slack webhook URL, SendGrid's three-segment + * form, `AccountKey=`, Telegram's digits-then-colon) have no prefix to compare + * and are matched by shape on both sides, or deliberately only on one. + */ +export const VENDOR_PREFIXES: readonly string[] = [ + "sk-ant-", "sk-proj-", "sk-svcacct-", "sk-admin-", "sk-", + "ghp_", "gho_", "ghu_", "ghs_", "ghr_", "github_pat_", + "glpat-", + "sb_secret_", "sbp_", + "xoxb-", "xoxp-", "xoxa-", "xoxr-", "xoxs-", + "AKIA", "ASIA", + "AIza", "GOCSPX-", + "sk_live_", "sk_test_", + "sq0atp-", "sq0csp-", + "shpat_", "shpss_", "shpca_", "shppa_", + "npm_", "pypi-", + "hvs.", "hvb.", + "dp.pt.", "dp.st.", "dp.sa.", "dp.ct.", "dp.scim.", "dp.audit.", + "lin_api_", "ntn_", "figd_", "PMAK-", "hf_", +]; + +/** + * The subset of `SECRET_PATTERNS` a policy is allowed to DENY on. + * + * Exported for the test that asserts the two tiers stay disjoint. Without it + * the split is a convention, and the failure mode of a broken convention here + * is a denied tool call every time an agent reads a git SHA. + */ +export const BLOCKING_SECRET_PATTERNS: ReadonlyArray = [ + [PRIVATE_KEY_RE, "private key"], + [JWT_RE, "JWT"], + [BEARER_TOKEN_RE, "bearer token"], + [CONNECTION_STRING_RE, "database credentials"], + ...API_KEY_PATTERNS, ]; // warnDestructiveSql / warnSchemaAlteration @@ -186,8 +390,11 @@ const DOTNET_GETENV_RE = /\[Environment\]::GetEnvironment/i; const CMD_ECHO_ENV_RE = /echo\s+%[A-Za-z_]/i; // blockEnvFiles -const ENV_FILE_PATH_RE = /(?:^|[\\/])\.env(?:\.|$)/; -const ENV_CMD_RE = /\.env(?:\b|\s|$|\.)/; +// `rc` is spelled out because the trailing group needs a `.` or end-of-string: +// `.envrc` (direnv) fell through it and was readable, though it holds exactly +// what `.env` holds. +const ENV_FILE_PATH_RE = /(?:^|[\\/])\.env(?:rc)?(?:\.|$)/; +const ENV_CMD_RE = /\.env(?:rc)?(?:\b|\s|$|\.)/; // blockSudo const PS_ELEVATION_RE = /Start-Process\s+.*-Verb\s+RunAs/i; @@ -393,8 +600,14 @@ const SAFE_FORCE_PREFIXES = ["--force-with-lease", "--force-if-includes"] as con // blockSecretsWrite const SECRET_FILE_RE = /\.(?:pem|key)$/; -const SECRET_FILE_ID_RSA_RE = /id_rsa/; +// `id_rsa` alone predates every key type ssh-keygen actually defaults to. +// ed25519 has been the recommended choice for years, so the one name matched +// here was the one least likely to be on disk. +const SECRET_FILE_ID_RSA_RE = /id_(?:rsa|dsa|ecdsa(?:_sk)?|ed25519(?:_sk)?)/; const SECRET_FILE_CREDENTIALS_RE = /credentials/; +/** Public halves of a keypair. Meant to be distributed; `/id_rsa/` matched + * `id_rsa.pub` and `\.(?:pem|key)$` is one rename from `.pub` too. */ +const PUBLIC_KEY_FILE_RE = /\.pub$/; // blockWorkOnMain const GIT_COMMIT_MERGE_RE = /git\s+(commit|merge|rebase|cherry-pick)\b/; @@ -596,10 +809,128 @@ function matchesAllowedPattern(cmd: string, pattern: string): boolean { // -- Policy implementations -- +/** + * True when `text` holds a match for `pattern` that is not a documentation example. + * + * Replaces a bare `pattern.test(text)` so `KNOWN_EXAMPLE_SECRETS` can be + * consulted against the MATCHED TEXT — which `.test()` does not expose. The scan + * continues past an allowlisted hit rather than returning on it, so a payload + * carrying both the AWS docs key and a live one still denies. + * + * A fresh global clone is built per call rather than putting `g` on the shared + * literal. These patterns are module-level constants reused across every hook + * event in a long-lived worker (worker-server.ts), and a global regex carries + * `lastIndex` between calls — it would skip matches depending on where the + * previous string happened to stop, which reads as flakiness rather than logic. + * `maskSecrets` in src/audit/redact-example.ts rebuilds for the same reason. + */ +function findSecret( + text: string, + pattern: RegExp, + allowedHashes?: ReadonlySet, +): string | undefined { + const flags = pattern.flags.includes("g") ? pattern.flags : `${pattern.flags}g`; + const scanner = new RegExp(pattern.source, flags); + let m: RegExpExecArray | null; + while ((m = scanner.exec(text)) !== null) { + const hit = m[0]; + const excused = KNOWN_EXAMPLE_SECRETS.has(hit) || allowedHashes?.has(sha256(hit)); + if (!excused) return hit; + // A zero-length match would spin forever; step past it. + if (m.index === scanner.lastIndex) scanner.lastIndex++; + } + return undefined; +} + +function containsSecret(text: string, pattern: RegExp): boolean { + return findSecret(text, pattern) !== undefined; +} + +function sha256(value: string): string { + return createHash("sha256").update(value, "utf8").digest("hex"); +} + +/** + * Caps on a user-supplied `additionalPatterns` entry. + * + * A builtin policy gets NO timeout. The 10s `Promise.race` in handler.ts wraps + * CUSTOM hooks only, and it could not interrupt a synchronous regex in any case + * — the event loop is blocked, so the timer never fires. Bounding the input is + * therefore the only hard limit on worst-case work that exists on this path. + */ +const USER_PATTERN_MAX_SOURCE = 200; +const USER_PATTERN_MAX_COUNT = 32; +const USER_PATTERN_MAX_HAYSTACK = 64 * 1024; + +/** + * A quantifier applied to a group that itself contains one — `(a+)+`, `(a|a)*`, + * `(\d+)*`. This is the classic catastrophic-backtracking shape. + * + * A conservative screen, not a solver: it rejects the shape that causes trouble + * in practice and accepts patterns that are merely complex. Deciding the general + * case is not something a regex can do, and pretending otherwise would be worse + * than saying plainly what this covers. + */ +const NESTED_QUANTIFIER_RE = /\([^()]*[+*}][^()]*\)\s*[+*]|\([^()]*\)\s*\{\d+,\s*\}/; + +/** Compiled user patterns, keyed by source. The worker is long-lived and this + * runs on every tool call, so a rejected pattern is re-validated never and a + * valid one is compiled once. `null` caches a rejection. */ +const userPatternCache = new Map(); + +/** + * Validate and compile one `additionalPatterns` entry, or return null. + * + * The shape check is the load-bearing part. `additionalPatterns` is declared + * `pattern[]` here and `string[]` on block-secrets-write, so a user copying + * config between the two writes `["foo"]`, which destructures to + * `regex === undefined`. `new RegExp(undefined)` does NOT throw — per spec an + * undefined pattern becomes the empty string, producing `/(?:)/`, which matches + * EVERYTHING. One malformed entry therefore denied every PostToolUse event on + * the machine, and the try/catch never fired because nothing threw. + */ +function compileUserPattern(entry: unknown): { re: RegExp; label: string } | null { + if (typeof entry !== "object" || entry === null) { + hookLogWarn(`additionalPatterns: expected { regex, label }, got ${typeof entry}; skipping`); + return null; + } + const { regex, label } = entry as { regex?: unknown; label?: unknown }; + if (typeof regex !== "string" || regex.length === 0) { + hookLogWarn("additionalPatterns: entry has no string `regex`, skipping"); + return null; + } + const name = typeof label === "string" && label.length > 0 ? label : "custom pattern"; + + const cached = userPatternCache.get(regex); + if (cached !== undefined) return cached === null ? null : { re: cached, label: name }; + + const reject = (why: string): null => { + hookLogWarn(`additionalPatterns: ${why} "${regex}", skipping`); + userPatternCache.set(regex, null); + return null; + }; + + if (regex.length > USER_PATTERN_MAX_SOURCE) return reject("pattern too long"); + if (NESTED_QUANTIFIER_RE.test(regex)) return reject("nested quantifier (ReDoS risk) in"); + + let re: RegExp; + try { + re = new RegExp(regex); + } catch { + return reject("invalid regex"); + } + // A pattern that matches the empty string matches every payload. That is + // never what anyone configured, and it denies the whole machine. + if (re.test("")) return reject("pattern matches everything —"); + + userPatternCache.set(regex, re); + return { re, label: name }; +} + function sanitizeJwt(ctx: PolicyContext): PolicyResult { // PostToolUse: scrub JWT patterns from tool output const output = JSON.stringify(ctx.payload); - if (JWT_RE.test(output)) { + if (containsSecret(output, JWT_RE)) { return { decision: "deny", reason: "JWT token detected in tool output", @@ -613,7 +944,7 @@ function sanitizeApiKeys(ctx: PolicyContext): PolicyResult { // PostToolUse: scrub common API key patterns from tool output const output = JSON.stringify(ctx.payload); for (const [pattern, label] of API_KEY_PATTERNS) { - if (pattern.test(output)) { + if (containsSecret(output, pattern)) { return { decision: "deny", reason: `${label} detected in tool output`, @@ -622,20 +953,33 @@ function sanitizeApiKeys(ctx: PolicyContext): PolicyResult { } } - // Check additional user-configured patterns - const additional = ((ctx.params?.additionalPatterns ?? []) as Array<{ regex: string; label: string }>); - for (const { regex, label } of additional) { - try { - if (new RegExp(regex).test(output)) { + // Check additional user-configured patterns. `compileUserPattern` returns + // null for anything that does not validate, and the haystack is bounded — + // see the notes on both. + const additional = ctx.params?.additionalPatterns; + if (Array.isArray(additional)) { + const haystack = output.length > USER_PATTERN_MAX_HAYSTACK + ? output.slice(0, USER_PATTERN_MAX_HAYSTACK) + : output; + if (additional.length > USER_PATTERN_MAX_COUNT) { + hookLogWarn( + `additionalPatterns: ${additional.length} entries exceeds the cap of ` + + `${USER_PATTERN_MAX_COUNT}; evaluating the first ${USER_PATTERN_MAX_COUNT}`, + ); + } + for (const entry of additional.slice(0, USER_PATTERN_MAX_COUNT)) { + const compiled = compileUserPattern(entry); + if (!compiled) continue; + if (compiled.re.test(haystack)) { return { decision: "deny", - reason: `${label} detected in tool output`, - message: `[REDACTED: ${label} removed by failproofai]`, + reason: `${compiled.label} detected in tool output`, + message: `[REDACTED: ${compiled.label} removed by failproofai]`, }; } - } catch { - hookLogWarn(`additionalPatterns: invalid regex "${regex}", skipping`); } + } else if (additional !== undefined) { + hookLogWarn("additionalPatterns: expected an array, ignoring"); } return allow(); @@ -644,7 +988,7 @@ function sanitizeApiKeys(ctx: PolicyContext): PolicyResult { function sanitizeConnectionStrings(ctx: PolicyContext): PolicyResult { // PostToolUse: scrub database connection strings with embedded credentials const output = JSON.stringify(ctx.payload); - if (CONNECTION_STRING_RE.test(output)) { + if (containsSecret(output, CONNECTION_STRING_RE)) { return { decision: "deny", reason: "Database connection string with credentials detected in tool output", @@ -657,7 +1001,7 @@ function sanitizeConnectionStrings(ctx: PolicyContext): PolicyResult { function sanitizePrivateKeyContent(ctx: PolicyContext): PolicyResult { // PostToolUse: scrub PEM private key blocks from tool output const output = JSON.stringify(ctx.payload); - if (PRIVATE_KEY_RE.test(output)) { + if (containsSecret(output, PRIVATE_KEY_RE)) { return { decision: "deny", reason: "Private key content detected in tool output", @@ -670,7 +1014,7 @@ function sanitizePrivateKeyContent(ctx: PolicyContext): PolicyResult { function sanitizeBearerTokens(ctx: PolicyContext): PolicyResult { // PostToolUse: scrub Authorization: Bearer tokens from tool output const output = JSON.stringify(ctx.payload); - if (BEARER_TOKEN_RE.test(output)) { + if (containsSecret(output, BEARER_TOKEN_RE)) { return { decision: "deny", reason: "Bearer token detected in tool output", @@ -702,6 +1046,245 @@ function warnDestructiveSql(ctx: PolicyContext): PolicyResult { return allow(); } +/** + * Tool-input keys holding content that ALREADY EXISTS on disk. + * + * The deny must never land on these, or the policy blocks the REMEDIATION: an + * `Edit` whose `old_string` is the leaked key and whose `new_string` is + * `process.env.X` is the fix, and refusing it leaves the credential in the file + * with no way to take it out. Same reason Bash is out of scope entirely below — + * `git grep AKIA` is how you find the leak. + * + * Spelled as an exclusion list rather than an inclusion list of content keys on + * purpose. Only copilot, opencode and antigravity canonicalise a content field + * at all (types.ts) — pi, goose, hermes and openclaw map the path and nothing + * else, and their own comments say so ("no builtin inspects the edit body"). + * An inclusion list would therefore have to enumerate key names for four CLIs + * whose payloads have not been captured, and a policy that silently sees + * nothing on a third of its CLIs while reading "enabled" in the UI is the exact + * failure types.ts:274 records getting burned by. Excluding the few keys that + * are definitely OLD content, and scanning whatever else arrives, covers a CLI + * whose key names nobody has written down yet. + */ +const PRE_EXISTING_CONTENT_KEYS = new Set([ + "old_string", "old_str", "oldstring", "old", "before", "search", "find", +]); + +/** Keys that are structural rather than content, and never worth scanning. */ +const NON_CONTENT_KEYS = new Set(["replace_all", "replaceall", "encoding", "mode"]); + +/** + * Paths where a credential-shaped literal is overwhelmingly a fixture. + * + * This repo is the worked example: its own suites carry `sk-ant-api03-…`, + * `AKIA…` and a PEM header as test data, and every one of them tripped the + * sanitizers while this policy was being written. A guard that denies the tests + * for the guard gets switched off, and a policy switched off protects nothing — + * so the default trades a little recall for enough precision to survive. + * + * Governed by `skipTestFixtures` so a team that would rather take the noise can + * turn it off, and narrow on purpose: `src/config.test-utils.ts` is NOT matched, + * because "test" appearing somewhere in a name is not evidence of a fixture. + */ +const TEST_FIXTURE_PATH_RE = + /(?:^|[\\/])(?:__tests__|__fixtures__|__mocks__|tests?|spec|fixtures|examples)[\\/]|\.(?:test|spec)\.[a-z]+$/i; + +function blockSecretInWrite(ctx: PolicyContext): PolicyResult { + if (ctx.toolName !== "Write" && ctx.toolName !== "Edit") return allow(); + const input = ctx.toolInput; + if (!input) return allow(); + + const skipFixtures = ctx.params?.skipTestFixtures !== false; + if (skipFixtures && TEST_FIXTURE_PATH_RE.test(getFilePath(ctx))) return allow(); + + const raw = ctx.params?.allowedSecretHashes; + // Hashes, never literals: a policy config is committed, synced and printed by + // `failproofai policies`, so it is the last place a real credential should be + // written in order to excuse itself. + const allowedHashes = new Set( + (Array.isArray(raw) ? raw : []).filter((h): h is string => typeof h === "string") + .map((h) => h.trim().toLowerCase()), + ); + + for (const [key, value] of Object.entries(input)) { + if (typeof value !== "string" || value.length === 0) continue; + const k = key.toLowerCase(); + if (PRE_EXISTING_CONTENT_KEYS.has(k) || NON_CONTENT_KEYS.has(k)) continue; + + for (const [pattern, label] of BLOCKING_SECRET_PATTERNS) { + const hit = findSecret(value, pattern, allowedHashes); + if (hit) { + // The label is phrased without an article — "a ${label}" produces + // "a Anthropic API key" for a third of the catalogue. + return deny( + `Writing a credential (${label}) to ${getFilePath(ctx) || "a file"} is blocked. ` + + `Reference it from the environment or a secret store instead. ` + + `If this is a fixture rather than a live credential, add its SHA-256 ` + + `to this policy's allowedSecretHashes.`, + ); + } + } + } + return allow(); +} + +/** + * Credential files whose contents are a secret in every normal case. + * + * `block-env-files` covered `.env` and nothing else, and `block-secrets-write` + * only ever looked at WRITES — so an agent could read `~/.ssh/id_ed25519` or + * `~/.aws/credentials` straight into context on a default install. + * `block-read-outside-cwd` is not the answer: it is off by default and misses + * an in-repo `.npmrc` entirely. + * + * Split by confidence, because these two groups have different error costs. + */ +const CREDENTIAL_FILE_PATTERNS: Array<[RegExp, string]> = [ + [/(?:^|[\\/])id_(?:rsa|dsa|ecdsa|ed25519)(?:_sk)?$/, "an SSH private key"], + [/(?:^|[\\/])\.ssh[\\/][^\\/]*\.pem$/, "an SSH private key"], + [/(?:^|[\\/])\.aws[\\/]credentials$/, "AWS credentials"], + [/(?:^|[\\/])\.git-credentials$/, "stored git credentials"], + [/(?:^|[\\/])_?\.?netrc$/, "netrc credentials"], + [/(?:^|[\\/])\.pypirc$/, "PyPI credentials"], + [/(?:^|[\\/])\.docker[\\/]config\.json$/, "Docker registry credentials"], + [/(?:^|[\\/])service-account[^\\/]*\.json$/, "a GCP service account key"], + [/\.(?:p12|pfx|jks|keystore)$/, "a keystore"], + [/(?:^|[\\/])\.gnupg[\\/]/, "GnuPG key material"], + [/(?:^|[\\/])\.config[\\/]gcloud[\\/]/, "gcloud credentials"], +]; + +/** + * The same idea, one confidence tier down: these are frequently NOT secret. + * + * A `.npmrc` is usually just a registry setting, most `*.tfvars` hold region + * names, and plenty of `kubeconfig`s point at a local cluster with no token in + * them. Blocking those by default denies ordinary work, which is the failure + * this codebase already argues against in src/audit/redact-example.ts — so they + * are behind `strict` and off unless asked for. + */ +const STRICT_CREDENTIAL_FILE_PATTERNS: Array<[RegExp, string]> = [ + [/(?:^|[\\/])\.npmrc$/, "npm registry credentials"], + [/(?:^|[\\/])(?:kubeconfig|\.kube[\\/]config)$/, "a kubeconfig"], + [/\.tfvars(?:\.json)?$/, "Terraform variables"], + [/\.tfstate(?:\.backup)?$/, "Terraform state"], + [/(?:^|[\\/])secrets?\.ya?ml$/, "a secrets file"], +]; + +function blockCredentialFiles(ctx: PolicyContext): PolicyResult { + const strict = ctx.params?.strict === true; + const rules = strict + ? [...CREDENTIAL_FILE_PATTERNS, ...STRICT_CREDENTIAL_FILE_PATTERNS] + : CREDENTIAL_FILE_PATTERNS; + + const check = (path: string): PolicyResult | undefined => { + if (!path) return undefined; + // The public half of a keypair is meant to be read. + if (PUBLIC_KEY_FILE_RE.test(path)) return undefined; + for (const [pattern, what] of rules) { + if (pattern.test(path)) { + return deny( + `Reading or writing ${what} (${path}) is blocked. ` + + `Ask the user for the value, or read it from the environment at run time.`, + ); + } + } + return undefined; + }; + + const direct = check(getFilePath(ctx)); + if (direct) return direct; + + if (ctx.toolName === "Bash") { + // Reuse the extractor block-read-outside-cwd already uses — it has been + // hardened against grep patterns and find globs being read as paths, and a + // second implementation here would drift from those fixes. + for (const path of extractAbsolutePaths(getCommand(ctx))) { + const hit = check(path); + if (hit) return hit; + } + // Relative mentions (`cat .aws/credentials`) never reach the extractor, + // which only takes absolute and ~-rooted paths. + for (const token of parseArgvTokens(getCommand(ctx))) { + const hit = check(stripShellQuoting(token)); + if (hit) return hit; + } + } + return allow(); +} + +/** + * Assignment names strong enough to mean "credential" on their own. + * + * Deliberately copied from crates/fpai-collect/src/redact.rs rather than from + * `isSecretName` in src/audit/redact-example.ts. The two disagree on 7 of 12 + * common names, and the redact-example version fires on a BARE `key=` — which + * the Rust comment records measuring against 40 real transcripts, where it + * matched React's `key` prop on every JSX list. The redactor can afford that; + * a policy the agent has to read cannot. + */ +const STRONG_SECRET_NAMES = ["secret", "password", "passwd", "credential"]; +/** Only convincing inside a COMPOUND identifier — `API_KEY` yes, `key` no. */ +const WEAK_SECRET_NAMES = ["key", "token"]; +/** Below this a value is far more likely a placeholder or a flag. */ +const MIN_ASSIGNMENT_VALUE = 12; +const ASSIGNMENT_RE = /\b([A-Za-z_][A-Za-z0-9_-]*)\s*=\s*("[^"]*"|'[^']*'|[^\s;|&"']+)/g; +/** A reference is not a literal: `$FOO`, `${FOO}`, `$(cmd)`, ``. */ +const EXPRESSION_VALUE_RE = /^[$<]|^\{\{|^%[A-Za-z_]/; +/** Obvious stand-ins. Warning about these trains the reader to ignore warnings. */ +const PLACEHOLDER_VALUE_RE = + /^(?:x+|\.+|-+|changeme|placeholder|your[-_a-z]*|todo|tbd|none|null|undefined|true|false|dummy|example|redacted|\*+)$/i; + +function namesACredential(name: string): boolean { + const lower = name.toLowerCase(); + if (STRONG_SECRET_NAMES.some((n) => lower.endsWith(n))) return true; + const compound = lower.includes("_") || lower.includes("-"); + return compound && WEAK_SECRET_NAMES.some((n) => lower.endsWith(n)); +} + +/** + * A literal assigned to a credential-named variable. + * + * This is the GENEROUS tier, and it returns `instruct`, never `deny`. The + * distinction is the one src/audit/redact-example.ts argues for: a name-based + * rule cannot tell `DB_PASSWORD=hunter2correct` from a fixture, and the cost of + * being wrong has to stay proportional to that uncertainty. A blocking version + * of this rule would deny `export EDITOR=vim` on any machine with the wrong + * word in its environment. + */ +function warnAssignedSecret(ctx: PolicyContext): PolicyResult { + const texts: string[] = []; + if (ctx.toolName === "Bash") { + texts.push(getCommand(ctx)); + } else if (ctx.toolName === "Write" || ctx.toolName === "Edit") { + for (const [key, value] of Object.entries(ctx.toolInput ?? {})) { + if (typeof value !== "string") continue; + const k = key.toLowerCase(); + if (PRE_EXISTING_CONTENT_KEYS.has(k) || NON_CONTENT_KEYS.has(k) || k === "file_path") continue; + texts.push(value); + } + } else { + return allow(); + } + + for (const text of texts) { + ASSIGNMENT_RE.lastIndex = 0; + let m: RegExpExecArray | null; + while ((m = ASSIGNMENT_RE.exec(text)) !== null) { + const name = m[1]; + const value = m[2].replace(/^["']|["']$/g, ""); + if (!namesACredential(name)) continue; + if (value.length < MIN_ASSIGNMENT_VALUE) continue; + if (EXPRESSION_VALUE_RE.test(value) || PLACEHOLDER_VALUE_RE.test(value)) continue; + return instruct( + `STOP: \`${name}\` is being assigned what looks like a literal credential. ` + + `Read it from the environment or a secret store instead of writing the value here. ` + + `If it is a placeholder or a fixture, say so and continue.`, + ); + } + } + return allow(); +} + function warnLargeFileWrite(ctx: PolicyContext): PolicyResult { if (ctx.toolName !== "Write") return allow(); const content = ctx.toolInput?.content as string | undefined; @@ -1229,16 +1812,36 @@ function isForcePushFlag(token: string): boolean { } function blockSecretsWrite(ctx: PolicyContext): PolicyResult { - if (ctx.toolName !== "Write") return allow(); + // Edit as well as Write: an Edit to `~/.ssh/id_rsa` is the same act, and + // gating only Write left the rename one tool call away. + if (ctx.toolName !== "Write" && ctx.toolName !== "Edit") 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)) { + // A public key is meant to be readable and writable — `id_rsa.pub` is not a + // secret, and `/id_rsa/` alone blocked it. + if (!PUBLIC_KEY_FILE_RE.test(filePath) && ( + 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"); } - const additionalPatterns = ((ctx.params?.additionalPatterns ?? []) as string[]); - for (const pattern of additionalPatterns) { - if (filePath.includes(pattern)) { - return deny(`Writing blocked file pattern: ${pattern}`); + // Substring match, deliberately — unlike sanitize-api-keys' identically-named + // `pattern[]` param, this one is declared `string[]`. Non-strings are skipped + // rather than thrown on: `.includes()` on a number throws, and the evaluator's + // per-policy catch would swallow it and silently disable the whole policy. + const additionalPatterns = ctx.params?.additionalPatterns; + if (Array.isArray(additionalPatterns)) { + for (const pattern of additionalPatterns) { + if (typeof pattern !== "string" || pattern.length === 0) { + hookLogWarn(`additionalPatterns: expected a non-empty string, got ${typeof pattern}; skipping`); + continue; + } + if (filePath.includes(pattern)) { + return deny(`Writing blocked file pattern: ${pattern}`); + } } + } else if (additionalPatterns !== undefined) { + hookLogWarn("additionalPatterns: expected an array, ignoring"); } return allow(); } @@ -2347,7 +2950,7 @@ export const BUILTIN_POLICIES: BuiltinPolicyDefinition[] = [ impact: "Stops the agent from creating `.pem`, `id_rsa`, `credentials.json`, etc.", description: "Block writing secret key files", fn: blockSecretsWrite, - match: { events: ["PreToolUse"], toolNames: ["Write"] }, + match: { events: ["PreToolUse"], toolNames: ["Write", "Edit"] }, defaultEnabled: false, category: "Dangerous Commands", params: { @@ -2358,6 +2961,61 @@ export const BUILTIN_POLICIES: BuiltinPolicyDefinition[] = [ }, } satisfies PolicyParamsSchema, }, + { + name: "block-secret-in-write", + displayTitle: "Tried to write a live credential into a file", + impact: "A hardcoded key in a source file is one commit from being public.", + description: "Block writing recognised API keys and tokens into file contents", + fn: blockSecretInWrite, + match: { events: ["PreToolUse"], toolNames: ["Write", "Edit"] }, + defaultEnabled: true, + category: "Sanitize", + params: { + allowedSecretHashes: { + type: "string[]", + description: + "SHA-256 hex digests of literals to allow (for fixtures and sample keys). Hashes, never the credential itself \u2014 this config is committed and printed.", + default: [], + }, + skipTestFixtures: { + type: "boolean", + description: + "Skip files under __tests__/, fixtures/, examples/ and *.test.*/*.spec.* paths, where a credential-shaped literal is almost always a fixture. Set false to scan them too.", + default: true, + }, + } satisfies PolicyParamsSchema, + }, + { + name: "block-credential-files", + displayTitle: "Tried to read or write a credential file", + impact: "SSH private keys, ~/.aws/credentials and friends were readable straight into context.", + description: "Block reading or writing SSH keys, cloud credentials and keystores", + fn: blockCredentialFiles, + match: { events: ["PreToolUse"] }, + defaultEnabled: true, + category: "Environment", + params: { + strict: { + type: "boolean", + description: + "Also block .npmrc, kubeconfig, *.tfvars, *.tfstate and secrets.yaml. Off by default because these frequently hold no credential at all.", + default: false, + }, + } satisfies PolicyParamsSchema, + }, + { + name: "warn-assigned-secret", + displayTitle: "Assigned a literal credential to a variable", + impact: "Catches hardcoded credentials the vendor-prefix list cannot recognise.", + description: "Warn when a credential-named variable is assigned a literal value", + fn: warnAssignedSecret, + match: { events: ["PreToolUse"], toolNames: ["Bash", "Write", "Edit"] }, + // Off by default on purpose. A name-based rule cannot distinguish a real + // credential from a fixture, so it warns rather than blocks \u2014 and a + // warning nobody asked for is noise. `failproofai policies` turns it on. + defaultEnabled: false, + category: "Sanitize", + }, { name: "block-push-master", displayTitle: "Tried to push directly to main/master", diff --git a/src/hooks/policy-evaluator.ts b/src/hooks/policy-evaluator.ts index cce0235b7..8291622dc 100644 --- a/src/hooks/policy-evaluator.ts +++ b/src/hooks/policy-evaluator.ts @@ -524,10 +524,26 @@ export async function evaluatePolicies( // model reads; it does not undo the side effect. That is the only // semantic available at PostToolUse, and it is precisely what an // output-scrubbing policy needs to keep a secret out of the context. + // + // `result.message` is the REPLACEMENT TEXT the model reads when a + // policy has one — the sanitize-* family sets "[REDACTED: removed + // by failproofai]". Sending `blockedMessage` here instead meant the + // model read "Blocked Bash by failproofai because: JWT token detected in + // tool output", which ANNOUNCES the secret it was scrubbing. On the only + // two CLIs where this text actually replaces the result, that is the + // whole job done backwards. + // + // The `??` fallback is not decoration: copilot's guard is + // `t?.decision === "block" && typeof t.reason === "string"` and fails + // CLOSED on a missing or non-string reason, so this must always be a + // non-empty string. if (session?.cli === "codex" || session?.cli === "copilot") { + const replacement = typeof result.message === "string" && result.message.length > 0 + ? result.message + : blockedMessage; return { exitCode: 0, - stdout: JSON.stringify({ decision: "block", reason: blockedMessage }), + stdout: JSON.stringify({ decision: "block", reason: replacement }), stderr: "", policyName: policy.name, reason, diff --git a/src/hooks/policy-presets.ts b/src/hooks/policy-presets.ts index 6a6a86abf..67d1688aa 100644 --- a/src/hooks/policy-presets.ts +++ b/src/hooks/policy-presets.ts @@ -106,6 +106,13 @@ export const RECOMMENDED_POLICIES: readonly string[] = [ "protect-env-vars", "block-env-files", "block-secrets-write", + // Prevention, not just scrubbing. PostToolUse is observation-only on 10 of + // the 12 CLIs (see enforcement-capability.ts), so a sanitize-* deny does not + // keep a secret out of the model's context on most machines. These two run at + // PreToolUse, which blocks on all 12 — they are what makes the promise above + // this list true rather than aspirational. + "block-secret-in-write", + "block-credential-files", // The agent cannot disable its own guardrails. "block-self-pause", "block-failproofai-commands",