diff --git a/bin/gstack-redact-prepush b/bin/gstack-redact-prepush index 76c5e9626..d4fe45000 100755 --- a/bin/gstack-redact-prepush +++ b/bin/gstack-redact-prepush @@ -140,12 +140,28 @@ function addedLinesFor(localSha: string, remoteSha: string): string { // +++ file header. Unified diff added lines start with a single '+'. // Strict (#1946): a failed diff used to return "" and the push sailed // through unscanned — fail open on the exact path the guard exists for. - const diff = gitStrict(["diff", "--unified=0", "--no-color", range]); + // + // --no-ext-diff: a user's `diff.external` driver replaces the entire diff + // with its own output — with one set, `git diff` emits zero '+' lines, so an + // unhardened scanner reads an empty diff and exits 0 on a push full of + // secrets. Reachable from ordinary user config, not hypothetical. (#2498) + // --no-textconv: a .gitattributes textconv driver can likewise rewrite + // content before we ever see it. (#2498) + const diff = gitStrict([ + "diff", "--unified=0", "--no-color", "--no-ext-diff", "--no-textconv", + range, + ]); const added: string[] = []; + // Hunk-aware header skip (#2498): `+++ ` is only a FILE HEADER outside a + // hunk. Inside a hunk, an added content line whose text begins with "++" + // renders as "+++" — the old blanket startsWith("+++") skip + // silently dropped exactly those lines from the scan. + let inHunk = false; for (const line of diff.split("\n")) { - if (line.startsWith("+") && !line.startsWith("+++")) { - added.push(line.slice(1)); - } + if (line.startsWith("diff --git")) { inHunk = false; continue; } + if (line.startsWith("@@")) { inHunk = true; continue; } + if (!inHunk && (line.startsWith("+++") || line.startsWith("---"))) continue; + if (line.startsWith("+")) added.push(line.slice(1)); } return added.join("\n"); } @@ -239,8 +255,23 @@ function main() { const allHigh: Finding[] = []; let mediumCount = 0; - for (const [, localSha, , remoteSha] of refs) { - if (!localSha || ZERO.test(localSha)) continue; // branch delete → nothing pushed + for (const fields of refs) { + // Fail CLOSED on a ref line we cannot parse (#2498): git hands pre-push + // exactly " " — anything + // else means we cannot tell WHAT is being pushed, and silently skipping + // it would leave that ref unscanned. + const [, localSha, , remoteSha] = fields; + const shaShaped = (s: string | undefined) => !!s && /^[0-9a-f]{40,64}$/i.test(s); + if (fields.length !== 4 || !shaShaped(localSha) || !shaShaped(remoteSha)) { + process.stderr.write( + "\n⛔ gstack-redact-prepush BLOCKED the push — could not parse a pre-push ref line, " + + "so its content cannot be scanned.\n" + + ` line: ${JSON.stringify(fields.join(" "))}\n` + + "Bypass if you're sure: GSTACK_REDACT_PREPUSH=skip git push (or git push --no-verify)\n", + ); + process.exit(1); + } + if (ZERO.test(localSha!)) continue; // branch delete → nothing pushed let added: string; try { added = addedLinesFor(localSha, remoteSha || "0"); diff --git a/test/redact-prepush-hook.test.ts b/test/redact-prepush-hook.test.ts index 67a39db0f..8412d4fad 100644 --- a/test/redact-prepush-hook.test.ts +++ b/test/redact-prepush-hook.test.ts @@ -324,3 +324,33 @@ describe("base resolution when the default branch is neither main nor master", ( expect(stderr).toContain("aws.access_key"); }); }); + +describe("diff-extraction bypasses (#2498, minimal reimplementation)", () => { + test("a diff.external driver cannot blank the scanned diff", () => { + // With diff.external set, plain `git diff` emits the driver's output — + // typically zero '+' lines — so an unhardened scanner reads an empty diff + // and allows a push full of secrets. --no-ext-diff must neutralize it. + const head = commit("leak.txt", "AKIA1234567890ABCDEF\n", "secret behind ext driver"); + git(["config", "diff.external", "/usr/bin/true"]); + const { code, stderr } = runHook(`refs/heads/feat ${head} refs/heads/feat ${ZERO}\n`); + git(["config", "--unset", "diff.external"]); + expect(code).toBe(1); + expect(stderr).toContain("aws.access_key"); + }); + + test("an added content line starting with ++ is still scanned", () => { + // Content "++AKIA…" renders in the diff as "+++AKIA…", which a blanket + // startsWith('+++') header skip silently dropped from the scan. + const head = commit("notes.txt", "++AKIA1234567890ABCDEF\n", "content line looks like a header"); + const { code, stderr } = runHook(`refs/heads/feat ${head} refs/heads/feat ${ZERO}\n`); + expect(code).toBe(1); + expect(stderr).toContain("aws.access_key"); + }); + + test("an unparseable pre-push ref line fails closed", () => { + commit("ok.txt", "clean\n", "clean commit"); + const { code, stderr } = runHook(`refs/heads/feat not-a-sha\n`); + expect(code).toBe(1); + expect(stderr).toContain("could not parse"); + }); +});