RomanNum3ral
/
gstack
mirror of https://github.com/garrytan/gstack.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Merge 258e6a1452 into 2be6c06ba8

This commit is contained in:
Two-Six-Alpha-1115 2026-08-15 18:13:21 -05:00 committed by GitHub
parent 2be6c06ba8 258e6a1452
commit cddc46032a
No known key found for this signature in database
GPG Key ID: B5690EEEBB952194
2 changed files with 244 additions and 1 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

93
bin/gstack-redact-prepush
View File

@ -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 <path>`, 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));

152
test/redact-prepush-scan-range.test.ts Normal file
View File

@ -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");
});
});