From 888603f57019a785d9047dacc51a3ef3838eb1fb Mon Sep 17 00:00:00 2001 From: Garry Tan Date: Fri, 14 Aug 2026 15:51:12 -0700 Subject: [PATCH] fix(redact-prepush): close the ext-diff, header-lookalike, and ref-parse bypasses MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Three ways the pushed diff escaped scanning: (1) a user-level diff.external or textconv driver replaced the diff with its own output — zero '+' lines, so the scan saw nothing (now --no-ext-diff --no-textconv); (2) an added content line whose text begins with "++" renders as "+++…" and the blanket header skip dropped it (now hunk-aware header detection); (3) a pre-push ref line that failed to parse was silently skipped, leaving that ref unscanned (now fails closed with the offending line named). Minimal reimplementation of the two confirmed bypasses from PR #2498 by @lubosxyz (the full PR overlaps the chunked-scan work absorbed separately), plus the unparseable-ref hardening. Co-Authored-By: Claude Fable 5 --- bin/gstack-redact-prepush | 43 +++++++++++++++++++++++++++----- test/redact-prepush-hook.test.ts | 30 ++++++++++++++++++++++ 2 files changed, 67 insertions(+), 6 deletions(-) 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"); + }); +});