diff --git a/bin/gstack-redact-prepush b/bin/gstack-redact-prepush index e43e2af49..76c5e9626 100755 --- a/bin/gstack-redact-prepush +++ b/bin/gstack-redact-prepush @@ -80,21 +80,57 @@ function defaultRemoteBranch(): string { return "origin/main"; } +/** + * Base commit for a push whose remote tip we cannot use directly, ordered from + * most precise to most conservative. Returns null when nothing can anchor the + * range, i.e. the whole history really is new content. + */ +function unknownRemoteTipBase(localSha: string): string | null { + // 1. The common case: a merge-base with the remote's default branch. + const base = git(["merge-base", localSha, defaultRemoteBranch()]).trim(); + if (base) return base; + + // 2. No merge-base. defaultRemoteBranch() guessed a ref that does not exist + // (default branch named trunk/develop, origin/HEAD unset), or history is + // disjoint. Anything reachable from localSha but from NO remote-tracking + // branch is what this push actually adds; the parent of its oldest commit + // is the real base. + // + // Without this we drop straight to EMPTY_TREE and re-scan content that is + // already on the remote. That is not merely wasteful, it is wrong in two + // ways: a secret pushed long ago gets re-reported as if THIS push + // introduced it (telling the operator to rotate a key over someone else's + // old commit), and on any real repository the input overshoots the + // 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(); + if (newCommits) { + const oldest = newCommits.split("\n")[0]; + const parent = git(["rev-parse", "--verify", `${oldest}^`]).trim(); + if (parent) return parent; + // Oldest new commit is a root commit: there is no parent to anchor on. + } + + // 3. Nothing to anchor on — a genuinely fresh repository with no remote refs. + // Every commit IS new content, so scanning it all is the correct answer. + return null; +} + /** Return the added-line text for a ref update being pushed. */ function addedLinesFor(localSha: string, remoteSha: string): string { let range: string; - if (ZERO.test(remoteSha)) { - // New branch: prefer what's unique to localSha vs the remote default branch. - // With no merge-base (e.g. no remote yet), diff against the empty tree so ALL - // branch content is scanned as added — fail-safe (scans more, never less). - const base = git(["merge-base", localSha, defaultRemoteBranch()]).trim(); - range = base ? `${base}..${localSha}` : `${EMPTY_TREE}..${localSha}`; - } else if (!objectExists(remoteSha)) { - // Remote tip object absent locally (shallow clone, force-push without a - // prior fetch, CI checkout): remote..local can't resolve. Fall back to - // the merge-base/empty-tree path — scans MORE, never less — instead of - // hard-blocking a legitimate push (adversarial review finding 8). - const base = git(["merge-base", localSha, defaultRemoteBranch()]).trim(); + if (ZERO.test(remoteSha) || !objectExists(remoteSha)) { + // Either a new branch (zero remote sha), or the remote tip object is absent + // locally (shallow clone, force-push without a prior fetch, CI checkout) so + // remote..local cannot resolve. Both need a base derived locally; scan MORE + // rather than hard-blocking a legitimate push (adversarial review finding 8). + const base = unknownRemoteTipBase(localSha); range = base ? `${base}..${localSha}` : `${EMPTY_TREE}..${localSha}`; } else { // Existing branch (incl. force-push): net new content remote..local. @@ -239,15 +275,50 @@ function main() { } if (allHigh.length > 0) { - process.stderr.write( - "\n⛔ gstack-redact-prepush BLOCKED the push — credential(s) in the pushed diff:\n\n", - ); - for (const f of allHigh) { - process.stderr.write(` HIGH ${f.id} ${f.preview}\n`); + // A scan that could not RUN is not a scan that FOUND something. Reporting + // "credential(s) in the pushed diff — rotate the credential" for an + // `engine.*` finding tells the operator to rotate a secret that was never + // detected, on a diff that was never read. Blocking is still right (fail + // closed), but the reason must be the true one: a guardrail that cries wolf + // is a guardrail that gets bypassed by reflex, which is worse than none. + // Seen live 2026-07-30: a diff of a few hundred bytes reported HIGH + // engine.input_too_large, because an unresolvable base branch made the hook + // fall back to EMPTY_TREE..local — i.e. the WHOLE repo (~7 MiB) as "added + // lines". The size the operator sees and the size the hook measures can + // therefore differ by four orders of magnitude. + const unscanned = allHigh.filter((f) => f.id.startsWith("engine.")); + const secrets = allHigh.filter((f) => !f.id.startsWith("engine.")); + + if (secrets.length > 0) { + process.stderr.write( + "\n⛔ gstack-redact-prepush BLOCKED the push — credential(s) in the pushed diff:\n\n", + ); + for (const f of secrets) { + process.stderr.write(` HIGH ${f.id} ${f.preview}\n`); + } + process.stderr.write( + "\nRotate the credential (a pushed secret is compromised) and remove it from the diff.\n", + ); } + + if (unscanned.length > 0) { + process.stderr.write( + "\n⛔ gstack-redact-prepush BLOCKED the push — the diff could NOT be scanned.\n" + + " No credential was found; none was looked for. Blocking fail-closed.\n\n", + ); + for (const f of unscanned) { + process.stderr.write(` ${f.id}: ${f.description}\n`); + } + process.stderr.write( + "\nLikely cause: the base branch could not be resolved, so the whole repo was\n" + + "treated as added lines. Check `git rev-parse --abbrev-ref origin/HEAD` and\n" + + "`git merge-base HEAD origin/main`, then push again. Scan the diff yourself\n" + + "before bypassing: `git diff ..HEAD | grep -inE \'password|secret|token|api.?key\'`.\n", + ); + } + process.stderr.write( - "\nRotate the credential (a pushed secret is compromised) and remove it from the diff.\n" + - "This is a guardrail: `git push --no-verify` or `GSTACK_REDACT_PREPUSH=skip git push` bypass it.\n", + "This is a guardrail: `git push --no-verify` or `GSTACK_REDACT_PREPUSH=skip git push` bypass it.\n", ); process.exit(1); } diff --git a/test/redact-prepush-hook.test.ts b/test/redact-prepush-hook.test.ts index cf8598523..b738380ca 100644 --- a/test/redact-prepush-hook.test.ts +++ b/test/redact-prepush-hook.test.ts @@ -229,3 +229,50 @@ describe("install / chaining", () => { expect(restored).not.toContain("managed"); }); }); + +describe("base resolution when the default branch is neither main nor master", () => { + test("a new branch scans its own commits, not the whole repository", () => { + // The remote's default branch is `trunk` and origin/HEAD is unset, so + // defaultRemoteBranch() falls through to `origin/main` — a ref that does + // not exist — and merge-base fails. The EMPTY_TREE fallback then treats the + // WHOLE repository as added lines, re-scanning history that is already on + // the remote. Two consequences, both bad: a secret long since pushed gets + // re-reported as if this push introduced it, and on any real repository the + // input blows past the engine's byte cap, so `engine.input_too_large` + // blocks the push having scanned NOTHING — the "scans more, never less" + // fallback inverting into "scans nothing". + const bare = fs.mkdtempSync(path.join(os.tmpdir(), "prepush-remote-")); + spawnSync("git", ["init", "-q", "--bare", "-b", "trunk", bare]); + + git(["branch", "-M", "trunk"]); + const old = commit("legacy.txt", "AKIA1234567890ABCDEF\n", "secret already on the remote"); + git(["remote", "add", "origin", bare]); + git(["push", "-q", "origin", "trunk"]); + + // The remote HAS the old commit, and the default-branch guess is unresolvable. + expect(git(["rev-parse", "origin/trunk"])).toBe(old); + expect(git(["rev-parse", "--verify", "origin/main"])).toBe(""); + expect(git(["symbolic-ref", "refs/remotes/origin/HEAD"])).toBe(""); + + git(["checkout", "-q", "-b", "feat"]); + const head = commit("feature.txt", "totally clean\n", "clean feature commit"); + + const { code, stderr } = runHook(`refs/heads/feat ${head} refs/heads/feat ${ZERO}\n`); + fs.rmSync(bare, { recursive: true, force: true }); + + // The only NEW content is a clean file. The already-pushed secret must not + // be attributed to this push. + expect(stderr).not.toContain("aws.access_key"); + expect(code).toBe(0); + }); + + test("a genuinely new repository with no remote refs still scans everything", () => { + // Nothing is on any remote, so every commit IS new content: scanning the + // full history is correct here. The narrowing must not open a hole in the + // case the EMPTY_TREE fallback exists for. + const head = commit("secrets.txt", "AKIA1234567890ABCDEF\n", "secret in a fresh repo"); + const { code, stderr } = runHook(`refs/heads/feat ${head} refs/heads/feat ${ZERO}\n`); + expect(code).toBe(1); + expect(stderr).toContain("aws.access_key"); + }); +});