mirror of
https://github.com/garrytan/gstack.git
synced 2026-09-11 15:39:04 +02:00
fix(redact): prepush guard fails closed on git failure; /ship owns hook install (#1946)
Two gaps closed:
1. Fail closed. The git() helper returned "" on ANY non-zero exit or
maxBuffer overflow (status null), addedLinesFor produced an empty
string, and the push sailed through unscanned — fail-open on exactly
the oversized-diff case where a large secret-bearing blob is most
likely. The diff call now uses a strict variant that throws; main
blocks with a clear message naming the GSTACK_REDACT_PREPUSH=skip
escape valve. Probe calls (symbolic-ref, rev-parse, merge-base) keep
the permissive helper — their failures are normal control flow.
2. Install path. The hook was installed by nothing ("opt-in, installed by
nothing" was the issue's words). ./setup runs in the gstack checkout —
the wrong repo for a per-project hook — so it gets a one-line hint
only. /ship owns per-repo install: config redact_prepush_hook=true +
hook missing → silent install (consent already given); config unset +
no ~/.gstack/.redact-prepush-prompted marker → one-time machine-wide
AskUserQuestion offer, answer persisted. ship/SKILL.md regenerated in
this same commit (check-freshness bisect discipline).
Tests: unscannable diff (bogus SHAs) → exit 1 + valve named; empty-but-
successful diff → exit 0; static asserts pin setup as hint-only and the
ship template as the installer surface.
Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
This commit is contained in:
co-authored by
Claude Fable 5
parent
c8f078b482
commit
6bc00abb0f
@@ -35,11 +35,33 @@ const ZERO = /^0+$/;
|
||||
// The canonical empty-tree object; diffing against it yields all content as added.
|
||||
const EMPTY_TREE = "4b825dc642cb6eb9a060e54bf8d69288fbee4904";
|
||||
|
||||
/**
|
||||
* Permissive git for legitimately-fallible PROBES (symbolic-ref, rev-parse,
|
||||
* merge-base) where a non-zero exit is normal control flow. The DIFF call
|
||||
* must NOT use this — see gitStrict (#1946 fail-closed).
|
||||
*/
|
||||
function git(args: string[]): string {
|
||||
const r = spawnSync("git", args, { encoding: "utf8", maxBuffer: 64 * 1024 * 1024 });
|
||||
return r.status === 0 ? (r.stdout ?? "") : "";
|
||||
}
|
||||
|
||||
/**
|
||||
* Fail-closed git for the diff that decides whether the push is scanned
|
||||
* (#1946). status !== 0 covers repo errors; status === null covers a killed
|
||||
* process AND maxBuffer overflow — the oversized-diff case is exactly where
|
||||
* a large secret-bearing blob is most likely, so "couldn't read the diff"
|
||||
* must block, not silently allow.
|
||||
*/
|
||||
function gitStrict(args: string[]): string {
|
||||
const r = spawnSync("git", args, { encoding: "utf8", maxBuffer: 64 * 1024 * 1024 });
|
||||
if (r.status !== 0 || r.status === null) {
|
||||
throw new Error(
|
||||
`git ${args[0]} failed (status=${r.status ?? "killed/overflow"}): ${(r.stderr ?? "").slice(0, 300)}`,
|
||||
);
|
||||
}
|
||||
return r.stdout ?? "";
|
||||
}
|
||||
|
||||
function defaultRemoteBranch(): string {
|
||||
// origin/HEAD → origin/main, fall back to main/master.
|
||||
const sym = git(["symbolic-ref", "refs/remotes/origin/HEAD"]).trim();
|
||||
@@ -65,7 +87,9 @@ function addedLinesFor(localSha: string, remoteSha: string): string {
|
||||
}
|
||||
// -U0: only changed lines; we keep lines starting with '+' (added), drop the
|
||||
// +++ file header. Unified diff added lines start with a single '+'.
|
||||
const diff = git(["diff", "--unified=0", "--no-color", range]);
|
||||
// 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]);
|
||||
const added: string[] = [];
|
||||
for (const line of diff.split("\n")) {
|
||||
if (line.startsWith("+") && !line.startsWith("+++")) {
|
||||
@@ -108,7 +132,21 @@ function main() {
|
||||
|
||||
for (const [, localSha, , remoteSha] of refs) {
|
||||
if (!localSha || ZERO.test(localSha)) continue; // branch delete → nothing pushed
|
||||
const added = addedLinesFor(localSha, remoteSha || "0");
|
||||
let added: string;
|
||||
try {
|
||||
added = addedLinesFor(localSha, remoteSha || "0");
|
||||
} catch (err) {
|
||||
// Fail CLOSED (#1946): if we can't compute the pushed diff we can't
|
||||
// scan it, and unscanned-but-allowed is the failure mode this hook
|
||||
// exists to prevent.
|
||||
process.stderr.write(
|
||||
"\n⛔ gstack-redact-prepush BLOCKED the push — could not compute the pushed diff, " +
|
||||
"so it cannot be scanned for credentials.\n" +
|
||||
` (${err instanceof Error ? err.message.split("\n")[0] : String(err)})\n` +
|
||||
"Bypass if you're sure: GSTACK_REDACT_PREPUSH=skip git push (or git push --no-verify)\n",
|
||||
);
|
||||
process.exit(1);
|
||||
}
|
||||
if (!added.trim()) continue;
|
||||
// Visibility doesn't change HIGH behavior; pass private so nothing is treated
|
||||
// as public-strict (HIGH blocks regardless either way).
|
||||
|
||||
Reference in New Issue
Block a user