diff --git a/bin/gstack-redact-prepush b/bin/gstack-redact-prepush index d195e5ac6..1dac0e94e 100755 --- a/bin/gstack-redact-prepush +++ b/bin/gstack-redact-prepush @@ -70,6 +70,33 @@ function objectExists(sha: string): boolean { return r.status === 0; } +/** + * The remote-tracking exclusion used when narrowing to "commits new to the + * remote" (#2592 catch-up merges, #2573 rebased force-pushes). + * + * Narrowed to the PUSH TARGET's namespace (S1): a bare `--remotes` excludes + * commits reachable from ANY remote-tracking ref, so a secret that had only + * ever been fetched from (or pushed to) a private/local-path remote was never + * scanned when later pushed to a PUBLIC remote — "already left this machine" + * is not "already reached THIS remote". Git hands pre-push the push remote's + * name as $1 (and its URL as $2); the installed hook wrapper forwards "$@". + * Fallbacks keep the historical all-remotes behavior when the name is + * unavailable (stdin/CLI invocation) or is not a configured remote (URL + * pushes have no remote-tracking namespace) — falling back scans LESS than + * the narrowed form would, but never less than the hook historically did. + */ +let _remotesExclusion: string | undefined; +function remotesExclusion(): string { + if (_remotesExclusion === undefined) { + const name = process.argv[2]; + const configured = name + ? git(["remote"]).split("\n").map((s) => s.trim()).filter(Boolean).includes(name) + : false; + _remotesExclusion = configured ? `--remotes=${name}/*` : "--remotes"; + } + return _remotesExclusion; +} + function defaultRemoteBranch(): string { // origin/HEAD → origin/main, fall back to main/master. const sym = git(["symbolic-ref", "refs/remotes/origin/HEAD"]).trim(); @@ -104,12 +131,12 @@ function unknownRemoteTipBase(localSha: string): string | null { // engine's byte cap, so `engine.input_too_large` blocks having scanned // NOTHING — "scans more, never less" inverted into "scans nothing". // - // `--remotes` covers every remote, not just the push target: content - // already published anywhere has already left this machine, so treating it - // as pre-existing is deliberate. Git hands the remote name to pre-push in - // argv, which this hook does not read; narrowing to it would only matter - // for a repo that pushes secrets to one remote but not another. - const newCommits = git(["rev-list", "--reverse", localSha, "--not", "--remotes"]).trim(); + // The exclusion is scoped to the PUSH TARGET's tracking refs (see + // remotesExclusion): content on some OTHER remote has left this machine, + // but it has not reached the remote being pushed to — a secret that only + // ever hit a private remote must still be scanned on its way to a public + // one (S1). + const newCommits = git(["rev-list", "--reverse", localSha, "--not", remotesExclusion()]).trim(); if (newCommits) { const oldest = newCommits.split("\n")[0]; const parent = git(["rev-parse", "--verify", `${oldest}^`]).trim(); @@ -149,10 +176,13 @@ function unknownRemoteTipBase(localSha: string): string | null { * * A two-dot range cannot express this: after merging main, neither the remote * tip nor the merge-base with main is an ancestor of the other, so no single - * base excludes both. `rev-list --not --remotes` is the operation that does, - * and this file already reasons that way in `unknownRemoteTipBase` step 2 — - * including why `--remotes` (every remote, not just the push target) is the - * right exclusion: content published anywhere has already left this machine. + * base excludes both. `rev-list --not --remotes=/*` is the + * operation that does, and this file already reasons that way in + * `unknownRemoteTipBase` step 2. The exclusion is scoped to the push target's + * tracking namespace (see remotesExclusion): the upstream commits a catch-up + * merge brings in came from the SAME remote being pushed to, so scoping keeps + * the #2592 fix intact while a secret known only to some OTHER (private) + * remote is still scanned on its way to this one (S1). * * Each commit is diffed alone. `--cc` on a merge shows only the conflict * RESOLUTION — content that exists in no parent — so a secret introduced while @@ -168,7 +198,7 @@ function addedLinesFromNewCommits(localSha: string, remoteSha: string): string | // direction. So it stays the base; `--remotes` only ADDS exclusions on top. if (ZERO.test(remoteSha) || !objectExists(remoteSha)) return null; - const narrowed = git(["rev-list", localSha, "--not", remoteSha, "--remotes"]).trim(); + const narrowed = git(["rev-list", localSha, "--not", remoteSha, remotesExclusion()]).trim(); if (!narrowed) return null; // If excluding remote-tracking refs changes nothing, this push has no diff --git a/lib/redact-patterns.ts b/lib/redact-patterns.ts index 0424a8e84..b447e59cd 100644 --- a/lib/redact-patterns.ts +++ b/lib/redact-patterns.ts @@ -108,6 +108,39 @@ export function shannonEntropy(s: string): number { return h; } +// env.kv name-shape calibration: the regex's zero-or-more-prefix net matches +// ANY identifier ending in a credential suffix, so `cacheKey:`, `sortKey:`, +// `partitionKey:`, `hotkey:`, even `monkey:` with an 8+-char entropic value +// all hit a MEDIUM confirm prompt — a gate that cries wolf gets ignored. +// A matched name only counts when its shape is credential-semantic: +// (i) suffix separated from the prefix by _ / - / . (api_key, x-access-key, +// AUTH.TOKEN) +// (ii) the whole name IS the bare suffix (key:, token:) +// (iii) the name is ALL-CAPS env style (APIKEY=, MY_APIKEY=) +// (iv) a lowercase/camel compound whose prefix ends in a credential word +// (apiKey, authToken, clientSecret, stripeApiKey) — cacheKey/sortKey/ +// monkey have no credential prefix and are rejected. +const ENV_KV_NAME = + /^[ \t]*(?:export[ \t]+)?["']?([A-Za-z0-9_.-]*?(?:KEY|TOKEN|SECRET|PASSWORD|PASSWD|CREDENTIALS?|DSN|AUTH|COOKIE|SESSION|PRIVATE))["']?[ \t]*[:=]/i; +const ENV_KV_SUFFIX = + /(KEY|TOKEN|SECRET|PASSWORD|PASSWD|CREDENTIALS?|DSN|AUTH|COOKIE|SESSION|PRIVATE)$/i; +const ENV_KV_CRED_PREFIX = + /(api|auth|access|secret|private|app|client|server|master|admin|signing|encryption|session|csrf|jwt|oauth|bearer)$/i; + +/** True when the full env.kv match starts with a credential-shaped name. */ +export function isCredentialShapedEnvName(fullMatch: string): boolean { + const nameMatch = ENV_KV_NAME.exec(fullMatch); + if (!nameMatch) return false; + const name = nameMatch[1]; + const suffixMatch = ENV_KV_SUFFIX.exec(name); + if (!suffixMatch) return false; + const prefix = name.slice(0, name.length - suffixMatch[1].length); + if (prefix === "") return true; // (ii) bare suffix + if (/[_.\-]$/.test(prefix)) return true; // (i) separator before suffix + if (!/[a-z]/.test(name)) return true; // (iii) ALL-CAPS env style + return ENV_KV_CRED_PREFIX.test(prefix); // (iv) credential-semantic compound +} + /** True when an IPv4 string is a public address (not RFC1918/loopback/etc). */ export function isPublicIPv4(ip: string): boolean { const m = ip.match(/^(\d{1,3})\.(\d{1,3})\.(\d{1,3})\.(\d{1,3})$/); @@ -566,9 +599,16 @@ export const PATTERNS: RedactPattern[] = [ // The name part is `[A-Za-z0-9_.-]*` + suffix (zero-or-more prefix, not // one-or-more): a mandatory first char would swallow the suffix's own // first letter and bare names like `password:` / `key:` would never match. + // The wide net is then calibrated by isCredentialShapedEnvName in + // validate — without it, any identifier that merely ENDS in a suffix + // (cacheKey:, sortKey:, monkey:) fires a MEDIUM confirm on entropic + // values. The value must stay capture group 1 (the engine masks group 1), + // so name-shape checking lives in validate, not in a second group. regex: /^[ \t]*(?:export[ \t]+)?["']?[A-Za-z0-9_.-]*(?:KEY|TOKEN|SECRET|PASSWORD|PASSWD|CREDENTIALS?|DSN|AUTH|COOKIE|SESSION|PRIVATE)["']?[ \t]*[:=][ \t]*["']?([^\s'"]{8,})["']?/i, - // Only fire on high-entropy values — kills `FOO_KEY=changeme` FPs. - validate: (span) => + // Only fire on credential-shaped names with high-entropy values — kills + // `FOO_KEY=changeme` and `cacheKey: ` FPs. + validate: (span, match) => + isCredentialShapedEnvName(match[0]) && !isPlaceholderSpan(span) && !/^\$\{?[A-Za-z_]/.test(span) && shannonEntropy(span) >= 3.0, diff --git a/test/redact-engine.test.ts b/test/redact-engine.test.ts index 2f99758f0..ab910463f 100644 --- a/test/redact-engine.test.ts +++ b/test/redact-engine.test.ts @@ -183,6 +183,29 @@ describe("MEDIUM demoted credential-shaped patterns (TENSION-1)", () => { expect(ids("apiKey: YOUR_API_KEY_HERE")).not.toContain("env.kv"); expect(ids("api_key=${MY_VAR}")).not.toContain("env.kv"); }); + // T1 calibration: the zero-or-more-prefix net matched ANY identifier ending + // in a suffix, so ordinary code (`cacheKey: `) hit a MEDIUM + // confirm prompt. Name shape must be credential-semantic to count. + test("env.kv ignores non-credential names ending in a suffix (entropic values)", () => { + const v = "8Fk2pQ9vXz4wL7mN3rT6yB1cD5eG0hJ"; + expect(ids(`cacheKey: ${v}`)).not.toContain("env.kv"); + expect(ids(`sortKey: ${v}`)).not.toContain("env.kv"); + expect(ids(`partitionKey: ${v}`)).not.toContain("env.kv"); + expect(ids(`hotkey: ${v}`)).not.toContain("env.kv"); + expect(ids(`monkey: ${v}`)).not.toContain("env.kv"); + expect(ids(`idempotencyKey: ${v}`)).not.toContain("env.kv"); + }); + test("env.kv still fires on every credential-shaped name form", () => { + const v = "8Fk2pQ9vXz4wL7mN3rT6yB1cD5eG0hJ"; + expect(ids(`api_key=${v}`)).toContain("env.kv"); // (i) separator + expect(ids(`API_KEY=${v}`)).toContain("env.kv"); // (i) + ALL-CAPS + expect(ids(`x-access-key: ${v}`)).toContain("env.kv"); // (i) dash separator + expect(ids(`key: ${v}`)).toContain("env.kv"); // (ii) bare suffix + expect(ids(`APIKEY=${v}`)).toContain("env.kv"); // (iii) ALL-CAPS compound + expect(ids(`apiKey: ${v}`)).toContain("env.kv"); // (iv) credential camel + expect(ids(`authToken: ${v}`)).toContain("env.kv"); // (iv) credential camel + expect(ids(`clientSecret: ${v}`)).toContain("env.kv"); // (iv) credential camel + }); test("env.kv stays MEDIUM (calibration: generic net, not a blocker)", () => { const f = scan("api_key=8Fk2pQ9vXz4wL7mN3rT6yB1cD5eG0hJ", { repoVisibility: "private" }) .findings.find((x) => x.id === "env.kv"); diff --git a/test/redact-prepush-scan-range.test.ts b/test/redact-prepush-scan-range.test.ts index 167e615b2..f44df28c4 100644 --- a/test/redact-prepush-scan-range.test.ts +++ b/test/redact-prepush-scan-range.test.ts @@ -159,3 +159,118 @@ describe("narrowing the range does not narrow coverage", () => { expect(addedOnly(addedLinesFromNewCommits(dir))).toContain(FAKE_AWS_NOREMOT); }); }); + +// ── S1: the exclusion is scoped to the PUSH TARGET's remote ───────────────── +// +// A bare `--remotes` excludes commits reachable from ANY remote-tracking ref, +// so a secret that had only ever reached a private/local-path remote was never +// scanned when pushed to a PUBLIC remote. Git hands pre-push the push remote's +// name as $1; the hook now scopes the exclusion to `--remotes=/*`. +// These run END-TO-END through the hook binary with the real argv + stdin +// protocol, because the behavior under test is the argv threading itself. +describe("S1: exclusion scoped to the push-target remote", () => { + const PREPUSH = join(import.meta.dir, "..", "bin", "gstack-redact-prepush"); + const FAKE_AWS_OTHERREM = ["AKIA", "IOSFODNN7OTHERRM"].join(""); + + function runHook(stdinLines: string, argv: string[]): { code: number; stderr: string } { + const r = spawnSync("bun", [PREPUSH, ...argv], { + cwd: dir, + input: Buffer.from(stdinLines), + encoding: "utf8", + env: { ...process.env }, + }); + return { code: r.status ?? 0, stderr: r.stderr ?? "" }; + } + + /** + * Build the S1 shape. Returns the feature branch's last-pushed origin tip + * (what git hands the hook as remoteSha): + * 1. main + feature pushed to origin (T0 = feature's origin tip) + * 2. a HIGH-shaped secret commit reaches a SECOND remote only + * (pushed there, fetched back → other/leaky tracking ref) + * 3. feature merges the secret commit — the next push to origin is + * the first time this content heads anywhere public + */ + function buildSecretOnSecondRemote(): { originTip: string } { + const origin = mkdtempSync(join(tmpdir(), "gstack-prepush-origin-")); + run(["init", "-q", "--bare", "-b", "main"], origin); + run(["remote", "add", "origin", origin]); + run(["push", "-q", "origin", "main"]); + run(["checkout", "-q", "-b", "feature"]); + commit("mine.ts", "export const mine = 1;\n", "my work"); + run(["push", "-q", "-u", "origin", "feature"]); + const originTip = run(["rev-parse", "HEAD"]).trim(); + + const other = mkdtempSync(join(tmpdir(), "gstack-prepush-other-")); + run(["init", "-q", "--bare", "-b", "main"], other); + run(["remote", "add", "other", other]); + run(["checkout", "-q", "-b", "leaky"]); + commit("leak.ts", `const k = "${FAKE_AWS_OTHERREM}";\n`, "secret to private remote only"); + run(["push", "-q", "other", "leaky"]); + run(["fetch", "-q", "other"]); + + run(["checkout", "-q", "feature"]); + // --no-ff: a fast-forward would make the secret commit the branch TIP, + // where the remoteSha two-dot fallback catches it regardless of the + // --remotes exclusion. The hole shape needs a real merge commit, so the + // narrowed path (per-commit --cc diffs) is what decides coverage. + run(["merge", "-q", "--no-ff", "--no-edit", "leaky"]); + return { originTip }; + } + + test("a commit known only to a SECOND remote IS scanned when pushing to origin", () => { + const { originTip } = buildSecretOnSecondRemote(); + const head = run(["rev-parse", "HEAD"]).trim(); + const { code, stderr } = runHook( + `refs/heads/feature ${head} refs/heads/feature ${originTip}\n`, + ["origin", "file:///ignored"], + ); + expect(code).toBe(1); + expect(stderr).toContain("BLOCKED"); + expect(stderr).toContain("aws.access_key"); + }); + + test("origin-published commits still are NOT re-scanned (catch-up merge, #2592 kept)", () => { + setUpRemoteWithForeignFixture(); + run(["checkout", "-q", "-b", "feature", "HEAD~1"]); + commit("mine.ts", "export const mine = 1;\n", "my work"); + run(["push", "-q", "-u", "origin", "feature"]); + const originTip = run(["rev-parse", "HEAD"]).trim(); + run(["merge", "-q", "--no-edit", "main"]); // catch-up merge brings the foreign fixture + + const head = run(["rev-parse", "HEAD"]).trim(); + const { code, stderr } = runHook( + `refs/heads/feature ${head} refs/heads/feature ${originTip}\n`, + ["origin", "file:///ignored"], + ); + expect(stderr).not.toContain("BLOCKED"); + expect(code).toBe(0); + }); + + test("no argv (stdin/CLI invocation) falls back to the historical all-remotes exclusion", () => { + // Documented contract, not a gap being celebrated: without the remote + // name there is nothing to scope to, and the fallback scans exactly what + // the hook always scanned. The installed hook wrapper forwards "$@", so + // real pushes always carry the name. + const { originTip } = buildSecretOnSecondRemote(); + const head = run(["rev-parse", "HEAD"]).trim(); + const { code, stderr } = runHook( + `refs/heads/feature ${head} refs/heads/feature ${originTip}\n`, + [], + ); + expect(stderr).not.toContain("BLOCKED"); + expect(code).toBe(0); + }); + + test("an unconfigured name (URL push) also falls back rather than erroring", () => { + const { originTip } = buildSecretOnSecondRemote(); + const head = run(["rev-parse", "HEAD"]).trim(); + const url = "file:///not-a-configured-remote"; + const { code, stderr } = runHook( + `refs/heads/feature ${head} refs/heads/feature ${originTip}\n`, + [url, url], + ); + expect(stderr).not.toContain("could not"); + expect(code).toBe(0); + }); +});