diff --git a/bin/gstack-redact-prepush b/bin/gstack-redact-prepush index d4fe45000..d195e5ac6 100755 --- a/bin/gstack-redact-prepush +++ b/bin/gstack-redact-prepush @@ -122,8 +122,87 @@ function unknownRemoteTipBase(localSha: string): string | null { return null; } +/** + * The commits this push actually adds — reachable from localSha and from NO + * remote-tracking ref. + * + * ⚠ WHY THIS EXISTS RATHER THAN A TWO-DOT RANGE. + * + * `remoteSha..localSha` is "everything new on this branch", which is NOT the + * same as "everything new to the remote". Merge origin/main into a feature + * branch and every commit main gained since the branch's last push becomes an + * added line — content that is already published, already scanned, and not + * this push's doing. + * + * Two things follow, and both were observed: + * + * · FALSE HIGH FINDINGS. A placeholder connection string in a test fixture, + * already merged to main by someone else, blocked an unrelated push as + * `db.url_with_password` — telling the operator to rotate a credential + * over a fixture they had never touched. A + * guard that cries wolf on catch-up merges is a guard people learn to + * bypass reflexively — which is exactly how a real secret gets through. + * · OVERSIZED SCANS. The comment on SCAN_CHUNK_BYTES below records a + * 1,146,782-byte diff from "a feature branch catching up to a busy main" + * blowing the engine's 1 MiB cap. Same root cause, treated there as a size + * problem and solved by slicing. Narrowing the range fixes the size too. + * + * 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. + * + * 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 + * resolving a merge is still caught, while an ordinary merge contributes + * nothing. Returns null when the notion does not apply, so callers fall back. + */ +function addedLinesFromNewCommits(localSha: string, remoteSha: string): string | null { + // remoteSha is what git TELLS us the remote has, and it is authoritative in a + // way `--remotes` is not: remote-tracking refs can be absent (a fresh clone + // that never fetched, a push to a remote with no tracking ref) or stale. Drop + // it and a repo with no tracking refs excludes NOTHING — every commit ever + // made reads as "new", which re-introduces the false positives from the other + // 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(); + if (!narrowed) return null; + + // If excluding remote-tracking refs changes nothing, this push has no + // catch-up commits and the plain range already describes it exactly. Defer to + // it. That is not just an optimization: it keeps every push that ISN'T a + // catch-up merge on the original gitStrict diff path, so the fail-closed + // guarantee (#1946) and its regression test keep exercising the code they + // were written for. A narrowing that silently retired that test would be a + // worse trade than the false positives it set out to fix. + const plain = git(["rev-list", `${remoteSha}..${localSha}`]).trim(); + const asSet = (s: string) => s.split("\n").filter(Boolean).sort().join("\n"); + if (asSet(narrowed) === asSet(plain)) return null; + + const shas = narrowed.split("\n").filter(Boolean); + // A rewrite of long history should fall back rather than shell out per commit. + if (shas.length > 500) return null; + const out: string[] = []; + for (const sha of shas) { + // gitStrict: a failed diff must never read as "nothing added" (#1946). + out.push(gitStrict([ + "show", "--unified=0", "--no-color", "--no-ext-diff", "--no-textconv", + "--cc", "--format=", sha, + ])); + } + return out.join("\n"); +} + /** Return the added-line text for a ref update being pushed. */ function addedLinesFor(localSha: string, remoteSha: string): string { + // Preferred ONLY when this push carries catch-up commits: scanning them again + // is the bug. Every other shape falls through to the range logic below. + const fromNew = addedLinesFromNewCommits(localSha, remoteSha); + if (fromNew !== null) return collectAddedLines(fromNew); + let range: string; if (ZERO.test(remoteSha) || !objectExists(remoteSha)) { // Either a new branch (zero remote sha), or the remote tip object is absent @@ -151,6 +230,14 @@ function addedLinesFor(localSha: string, remoteSha: string): string { "diff", "--unified=0", "--no-color", "--no-ext-diff", "--no-textconv", range, ]); + return collectAddedLines(diff); +} + +/** + * Added-line text from a unified diff. Shared by both range strategies so the + * hunk-aware header handling below cannot drift between them. + */ +function collectAddedLines(diff: string): string { 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 "++" @@ -158,7 +245,11 @@ function addedLinesFor(localSha: string, remoteSha: string): string { // silently dropped exactly those lines from the scan. let inHunk = false; for (const line of diff.split("\n")) { - if (line.startsWith("diff --git")) { inHunk = false; continue; } + // `diff --` rather than `diff --git`: a merge scanned with --cc emits + // `diff --cc `, so a --git-only reset left inHunk true across file + // boundaries and read the next file's `+++ b/...` header as content. Only + // noise (it over-scans, never under-scans), but the boundary is real. + if (line.startsWith("diff --")) { inHunk = false; continue; } if (line.startsWith("@@")) { inHunk = true; continue; } if (!inHunk && (line.startsWith("+++") || line.startsWith("---"))) continue; if (line.startsWith("+")) added.push(line.slice(1)); diff --git a/test/redact-prepush-scan-range.test.ts b/test/redact-prepush-scan-range.test.ts new file mode 100644 index 000000000..c5c9f6c7e --- /dev/null +++ b/test/redact-prepush-scan-range.test.ts @@ -0,0 +1,152 @@ +/** + * gstack-redact-prepush — WHICH commits get scanned. + * + * `remoteSha..localSha` is "everything new on this branch", not "everything new + * to the remote". Merge origin/main into a feature branch and every commit main + * gained since the last push becomes an added line: already published, already + * scanned, not this push's doing. That produces false HIGH findings on other + * people's merged fixtures, and blows the engine's size cap on busy repos. + * + * These tests build real repositories on disk, because the behaviour under test + * IS the git plumbing — a mocked `git` would test the mock. Each asserts on the + * added-line text the hook would scan. + * + * The direction that matters most is the LAST describe block: narrowing the + * range must not narrow COVERAGE. A secret in a new commit, or introduced while + * resolving a merge, still has to be seen. + */ +import { describe, test, expect, beforeEach, afterEach } from "bun:test"; +import { spawnSync } from "child_process"; +import { mkdirSync, mkdtempSync, rmSync, writeFileSync } from "fs"; +import { tmpdir } from "os"; +import { dirname, join } from "path"; + +let dir: string; +const run = (args: string[], cwd = dir): string => { + const r = spawnSync("git", args, { cwd, encoding: "utf8" }); + if (r.status !== 0) throw new Error(`git ${args.join(" ")}\n${r.stderr}`); + return r.stdout ?? ""; +}; +const commit = (file: string, body: string, msg: string, cwd = dir) => { + mkdirSync(dirname(join(cwd, file)), { recursive: true }); + writeFileSync(join(cwd, file), body); + run(["add", file], cwd); + run(["commit", "-q", "-m", msg], cwd); +}; + +/** + * The range the fixed hook uses: commits reachable from HEAD and from no + * remote-tracking ref, each diffed alone with --cc. + */ +function addedLinesFromNewCommits(cwd: string): string { + const listed = run(["rev-list", "HEAD", "--not", "--remotes"], cwd).trim(); + if (!listed) return ""; + const out: string[] = []; + for (const sha of listed.split("\n").filter(Boolean)) { + out.push(run([ + "show", "--unified=0", "--no-color", "--no-ext-diff", "--no-textconv", + "--cc", "--format=", sha, + ], cwd)); + } + return out.join("\n"); +} + +/** The old behaviour, for contrast. */ +function addedLinesFromTwoDot(cwd: string, remoteRef: string): string { + return run([ + "diff", "--unified=0", "--no-color", "--no-ext-diff", "--no-textconv", + `${remoteRef}..HEAD`, + ], cwd); +} + +const addedOnly = (diff: string): string => + diff.split("\n") + .filter((l) => l.startsWith("+") && !l.startsWith("+++")) + .join("\n"); + +beforeEach(() => { + dir = mkdtempSync(join(tmpdir(), "gstack-prepush-")); + run(["init", "-q", "-b", "main"]); + run(["config", "user.email", "t@example.com"]); + run(["config", "user.name", "T"]); + commit("README.md", "seed\n", "seed"); +}); +afterEach(() => rmSync(dir, { recursive: true, force: true })); + +/** Give the repo an "origin" whose main carries a fixture we did not write. */ +function setUpRemoteWithForeignFixture(): void { + const remote = mkdtempSync(join(tmpdir(), "gstack-prepush-remote-")); + run(["init", "-q", "--bare", "-b", "main"], remote); + run(["remote", "add", "origin", remote]); + run(["push", "-q", "origin", "main"]); + // Someone else lands a placeholder connection string on main. + commit("fixtures/db.ts", 'export const URL = "postgresql://user:pass@db.example.com/x";\n', "someone else's fixture"); + run(["push", "-q", "origin", "main"]); + run(["fetch", "-q", "origin"]); +} + +describe("a catch-up merge does not re-scan already-published content", () => { + test("the foreign fixture is absent from the scanned text", () => { + setUpRemoteWithForeignFixture(); + // Branch from BEFORE that fixture, then merge main in to catch up. + run(["checkout", "-q", "-b", "feature", "HEAD~1"]); + commit("mine.ts", "export const mine = 1;\n", "my work"); + run(["merge", "-q", "--no-edit", "main"]); + + const scanned = addedOnly(addedLinesFromNewCommits(dir)); + expect(scanned).toContain("export const mine = 1;"); + expect(scanned).not.toContain("postgresql://user:pass@db.example.com/x"); + }); + + test("the old two-dot range DID re-scan it — this is the bug", () => { + setUpRemoteWithForeignFixture(); + run(["checkout", "-q", "-b", "feature", "HEAD~1"]); + commit("mine.ts", "export const mine = 1;\n", "my work"); + run(["merge", "-q", "--no-edit", "main"]); + + // origin/feature does not exist yet, so the old code diffed against the + // remote's main — dragging in every catch-up commit. + const scanned = addedOnly(addedLinesFromTwoDot(dir, "HEAD~2")); + expect(scanned).toContain("postgresql://user:pass@db.example.com/x"); + }); +}); + +describe("narrowing the range does not narrow coverage", () => { + test("a secret in a new commit is still scanned", () => { + setUpRemoteWithForeignFixture(); + run(["checkout", "-q", "-b", "feature", "main"]); + commit("leak.ts", 'const k = "AKIAIOSFODNN7SECRETX";\n', "oops"); + + expect(addedOnly(addedLinesFromNewCommits(dir))).toContain("AKIAIOSFODNN7SECRETX"); + }); + + test("a secret introduced while RESOLVING a merge is still scanned", () => { + // A combined diff shows only content present in no parent — exactly the + // conflict resolution — so this must not slip through. + // + // Note for anyone hardening this later: removing `--cc` from the + // implementation does NOT fail this test, because `git show` already + // defaults to a combined diff for merge commits. The explicit flag is + // self-documenting, not load-bearing, and no test can pin it. What this + // test does pin is the coverage itself. + setUpRemoteWithForeignFixture(); + run(["checkout", "-q", "-b", "feature", "HEAD~1"]); + commit("conflict.txt", "mine\n", "mine"); + run(["checkout", "-q", "main"]); + commit("conflict.txt", "theirs\n", "theirs"); + run(["push", "-q", "origin", "main"]); + run(["fetch", "-q", "origin"]); + run(["checkout", "-q", "feature"]); + spawnSync("git", ["merge", "--no-edit", "main"], { cwd: dir, encoding: "utf8" }); // conflicts + writeFileSync(join(dir, "conflict.txt"), 'resolved AKIAIOSFODNN7RESOLV\n'); + run(["add", "conflict.txt"]); + run(["commit", "-q", "--no-edit"]); + + expect(addedOnly(addedLinesFromNewCommits(dir))).toContain("AKIAIOSFODNN7RESOLV"); + }); + + test("everything is scanned when no remote exists at all", () => { + commit("leak.ts", 'const k = "AKIAIOSFODNN7NOREMOT";\n', "no remote"); + expect(addedOnly(addedLinesFromNewCommits(dir))).toContain("AKIAIOSFODNN7NOREMOT"); + }); +});