CalvinBackup/gstack
1
0
Fork 0
mirror of https://github.com/garrytan/gstack.git synced 2026-09-26 14:50:55 +02:00
Code Issues Packages Projects Releases Wiki Activity

fix(redact-prepush): close the remaining shapes of three fail-open classes

Browse Source 
Three shapes still exited 0 with a credential in the push.

A push to a URL borrowed origin's tip as its base and excluded every commit
any remote had seen, so a credential already on origin shipped to a new,
empty URL remote unscanned. A URL is described by no remote-tracking ref:
the push target is now a three-way value (configured remote / URL /
unknown), a URL target anchors on nothing and excludes nothing, and an
unknown target keeps the historical origin-shaped behavior.

When git named a remote tip the local clone lacks, only an EMPTY guessed
range was blocked; a stale tracking ref holding the credential plus one
harmless local commit produced a non-empty, credential-free guess that
passed. An absent named tip now gets no local narrowing at all and the
whole reachable history is scanned, which is affordable because every scan
is sliced under the engine's cap.

The inter-slice overlap carried whole lines only, so a line longer than
the overlap contributed nothing and a label at its end never met the secret
at the start of the next slice. The overlap now carries such a line's
suffix.

Gate: test/redact-prepush-fail-open.sh grows to 30 scenarios. E5 expected
the URL-push leak to be ALLOWED and now expects the block, E5b pins that a
clean URL push is still allowed, 19 and 20 pin the other two shapes, and
E8b pins that a clean SHA-256 first push is allowed (E8 carried a
credential, so it could not tell a working empty-tree fix from a broken
one). The S1 URL-push case in redact-prepush-scan-range.test.ts keeps its
no-error assertion and now also asserts the block, because its fixture's
secret really is delivered to that URL.

Co-Authored-By: Claude Opus 5 (1M context) <noreply@anthropic.com>
This commit is contained in:
Lubos Buracinsky
2026-09-22 18:09:53 +02:00
co-authored by Claude Opus 5
parent 11734707df
commit 9c15f7bf45
3 changed files with 215 additions and 36 deletions
Download Patch File Download Diff File Expand all files Collapse all files
bin/gstack-redact-prepush
+119 -30
View File
@@ -96,24 +96,36 @@ function objectExists(sha: string): boolean {
}
/**
* The remote this push targets, as git hands it to pre-push in $1 (and its URL
* as $2); the installed hook wrapper forwards "$@". Null when the name is
* unavailable (stdin/CLI invocation) or does not name a CONFIGURED remote — a
* URL push has no remote-tracking namespace at all — and every caller then
* falls back to the historical origin-shaped behavior.
* What git told us about the push target, as three distinct cases. The
* distinction matters: "we were not told" and "we were told, and it has no
* tracking namespace" must not be collapsed, because only the first one can
* safely fall back to origin-shaped behavior.
*
* { kind: "remote", name } $1 names a CONFIGURED remote.
* { kind: "url" } $1 was given but is not a configured remote —
* a URL push. There is no <remote>/<branch> to
* anchor on, for ANY remote.
* { kind: "unknown" } $1 absent (stdin / CLI invocation).
*/
let _pushRemote: string | null | undefined;
function pushRemote(): string | null {
if (_pushRemote === undefined) {
type PushTarget = { kind: "remote"; name: string } | { kind: "url" } | { kind: "unknown" };
let _pushTarget: PushTarget | undefined;
function pushTarget(): PushTarget {
if (_pushTarget === undefined) {
const name = process.argv[2];
const configured = name
? git(["remote"]).split("\n").map((s) => s.trim()).filter(Boolean).includes(name)
: false;
_pushRemote = configured ? name! : null;
if (!name) {
_pushTarget = { kind: "unknown" };
} else {
const configured = git(["remote"]).split("\n").map((s) => s.trim()).filter(Boolean).includes(name);
_pushTarget = configured ? { kind: "remote", name } : { kind: "url" };
}
}
// `?? null` only narrows away the not-yet-computed sentinel; by here the
// cache is always set, and null legitimately means "no tracking namespace".
return _pushRemote ?? null;
return _pushTarget;
}
/** The configured remote this push targets, or null for a URL/unknown target. */
function pushRemote(): string | null {
const t = pushTarget();
return t.kind === "remote" ? t.name : null;
}
/**
@@ -124,13 +136,29 @@ function pushRemote(): string | null {
* commits reachable from ANY remote-tracking ref, so a secret that had only
* ever been fetched from (or pushed to) a private/local-path remote was never
* scanned when later pushed to a PUBLIC remote — "already left this machine"
* is not "already reached THIS remote". Falling back to the bare form scans
* LESS than the narrowed form would, but never less than the hook historically
* did.
* is not "already reached THIS remote".
*
* ⚠ A URL PUSH EXCLUDES NOTHING.
*
* The bare `--remotes` fallback was a fail-open for a URL target, measured: the
* credential sat on origin/main, the local commit added only harmless content,
* and a first push to a different empty URL remote had origin/main excluded as
* "already on the remote" — so rev-list reported only the harmless commit as
* new, the range anchored at its parent, and the credential shipped with exit 0.
* A URL we were handed is not described by ANY remote-tracking ref, so there is
* nothing legitimately excludable and everything reachable must be scanned.
*
* An UNKNOWN target (no argv, stdin/CLI invocation) keeps the historical bare
* form: we were told nothing, so we cannot single out a namespace.
*
* Returns the rev-list arguments, empty when nothing may be excluded — the
* callers must not hand `--not` an empty operand.
*/
function remotesExclusion(): string {
const name = pushRemote();
return name ? `--remotes=${name}/*` : "--remotes";
function remotesExclusionArgs(): string[] {
const t = pushTarget();
if (t.kind === "remote") return ["--not", `--remotes=${t.name}/*`];
if (t.kind === "url") return [];
return ["--not", "--remotes"];
}
/**
@@ -138,7 +166,7 @@ function remotesExclusion(): string {
*
* ⚠ SCOPED TO THE PUSH TARGET, NOT HARDCODED TO origin.
*
* remotesExclusion() above already reasons this way for the rev-list exclusion,
* remotesExclusionArgs() above already reasons this way for the rev-list exclusion,
* but this probe kept asking origin regardless of where the push was going, and
* the two together are what pick the scan range. The consequence was a
* fail-OPEN, measured: with HEAD == origin/main, `git push publish HEAD:main`
@@ -153,7 +181,17 @@ function remotesExclusion(): string {
* Scanning more is the safe direction here and matches the S1 reasoning above.
*/
function defaultRemoteBranch(): string | null {
const remote = pushRemote() ?? "origin";
const t = pushTarget();
// ⚠ A URL PUSH HAS NO ANCHOR — DO NOT BORROW origin's.
//
// Falling back to origin here was itself a fail-open, measured: with the
// credential already on origin/main and the local commit adding only harmless
// content, the first push to a different, empty URL remote resolved the range
// merge-base(HEAD, origin/main)..HEAD, scanned the harmless commit, exited 0,
// and sent the whole history — credential included — to that remote. origin's
// tip says nothing about what a DIFFERENT remote already has.
if (t.kind === "url") return null;
const remote = t.kind === "remote" ? t.name : "origin";
// <remote>/HEAD → <remote>/main, fall back to main/master.
const sym = git(["symbolic-ref", `refs/remotes/${remote}/HEAD`]).trim();
if (sym) return sym.replace("refs/remotes/", "");
@@ -191,11 +229,11 @@ function unknownRemoteTipBase(localSha: string): string | null {
// NOTHING — "scans more, never less" inverted into "scans nothing".
//
// The exclusion is scoped to the PUSH TARGET's tracking refs (see
// remotesExclusion): content on some OTHER remote has left this machine,
// remotesExclusionArgs): content on some OTHER remote has left this machine,
// but it has not reached the remote being pushed to — a secret that only
// ever hit a private remote must still be scanned on its way to a public
// one (S1).
const newCommits = git(["rev-list", "--reverse", localSha, "--not", remotesExclusion()]).trim();
const newCommits = git(["rev-list", "--reverse", localSha, ...remotesExclusionArgs()]).trim();
if (newCommits) {
const oldest = newCommits.split("\n")[0];
const parent = git(["rev-parse", "--verify", `${oldest}^`]).trim();
@@ -238,7 +276,7 @@ function unknownRemoteTipBase(localSha: string): string | null {
* base excludes both. `rev-list --not --remotes=<push-remote>/*` is the
* operation that does, and this file already reasons that way in
* `unknownRemoteTipBase` step 2. The exclusion is scoped to the push target's
* tracking namespace (see remotesExclusion): the upstream commits a catch-up
* tracking namespace (see remotesExclusionArgs): the upstream commits a catch-up
* merge brings in came from the SAME remote being pushed to, so scoping keeps
* the #2592 fix intact while a secret known only to some OTHER (private)
* remote is still scanned on its way to this one (S1).
@@ -257,7 +295,10 @@ function addedLinesFromNewCommits(localSha: string, remoteSha: string): string |
// 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, remotesExclusion()]).trim();
// `--not remoteSha` always applies; the remote-tracking exclusion is added
// only when one is legitimate for this target (see remotesExclusionArgs).
const extra = remotesExclusionArgs().filter((a) => a !== "--not");
const narrowed = git(["rev-list", localSha, "--not", remoteSha, ...extra]).trim();
if (!narrowed) return null;
// If excluding remote-tracking refs changes nothing, this push has no
@@ -303,8 +344,32 @@ function addedLinesFor(localSha: string, remoteSha: string): string {
// remote..local cannot resolve. Both need a base derived locally; scan MORE
// rather than hard-blocking a legitimate push (adversarial review finding 8).
guessedFromUnresolvableTip = !ZERO.test(remoteSha);
const base = unknownRemoteTipBase(localSha);
range = base ? `${base}..${localSha}` : `${emptyTree()}..${localSha}`;
if (guessedFromUnresolvableTip) {
// ⚠ AN ABSENT NAMED TIP INVALIDATES OUR TRACKING REFS FOR THIS REF.
//
// git told us the remote is at a sha we do not have, which is itself
// proof that our remote-tracking refs do not describe this ref — so
// narrowing by them is not conservative, it is a guess. Blocking only
// the EMPTY guess was not enough: measured, a stale origin/main holding
// the credential plus one harmless local commit produced a NON-empty,
// credential-free diff, the guard passed, and the force-push republished
// the credential with exit 0. A guess that scanned something is still a
// guess about the wrong thing.
//
// So this shape gets no local narrowing at all: scan everything reachable
// from the pushed tip. That is affordable now only because every scan is
// sliced under the engine's cap (see scanAddedLines), so a whole-history
// range can no longer degrade into engine.input_too_large — which is
// exactly why the older code avoided this path.
//
// A NEW BRANCH (zero remote sha) is deliberately NOT treated this way:
// there git named no tip, our tracking refs are not contradicted, and
// narrowing keeps the first push of a feature branch proportionate.
range = `${emptyTree()}..${localSha}`;
} else {
const base = unknownRemoteTipBase(localSha);
range = base ? `${base}..${localSha}` : `${emptyTree()}..${localSha}`;
}
} else {
// Existing branch (incl. force-push): net new content remote..local.
range = `${remoteSha}..${localSha}`;
@@ -526,7 +591,31 @@ function scanAddedLines(added: string, opts: Parameters<typeof scan>[1]): Findin
let tailBytes = 0;
for (let i = slice.length - 1; i >= 0; i--) {
const b = Buffer.byteLength(slice[i]!, "utf8") + 1;
if (tailBytes + b > SCAN_OVERLAP_BYTES) break;
if (tailBytes + b > SCAN_OVERLAP_BYTES) {
// ⚠ A LINE BIGGER THAN THE OVERLAP MUST STILL CONTRIBUTE ITS END.
//
// Carrying whole lines only means a single line longer than the whole
// overlap budget contributes NOTHING, and the seam it sits on gets no
// context at all. Measured: one ~770 KB ASCII line ending in
// `aws_secret_access_key =`, followed by a line holding the key, put the
// label and the secret in different slices with an empty overlap between
// them, so aws.secret_key never fired. Carry the line's SUFFIX instead —
// the last overlap-worth of it is exactly the part a proximity pattern
// needs.
if (tail.length === 0) {
const line = slice[i]!;
let back = 0;
let backBytes = 0;
while (back < line.length && backBytes < SCAN_OVERLAP_BYTES) {
backBytes += Buffer.byteLength(line[line.length - back - 1]!, "utf8");
back++;
}
const suffix = line.slice(safeCut(line, line.length - back));
tail.unshift(suffix);
tailBytes += Buffer.byteLength(suffix, "utf8") + 1;
}
break;
}
tail.unshift(slice[i]!);
tailBytes += b;
}
test/redact-prepush-fail-open.sh
+86 -4
View File
@@ -249,7 +249,10 @@ run_missingremote() { cd "$1" || exit 1; local sha; sha=$(git rev-parse HEAD)
printf 'refs/heads/main %s refs/heads/main cafebabecafebabecafebabecafebabecafebabe\n' "$sha" \
| bun "$CAND" origin https://example.invalid/origin.git 2>&1; echo "___EXIT:$?"; }
S16=$(run_missingremote "$R/s12")
row "16 missing-but-shaped remote sha" "$S16" "BLOCK(unscannable)"
# Now that an absent named tip drops ALL local narrowing, this blocks with the
# actual finding rather than an "unscannable" apology: the whole reachable
# range is scanned and the credential is named.
row "16 missing-but-shaped remote sha" "$S16" "BLOCK(aws.access_key)"
# 17: zero-width across slice boundary
mkrepo s17; SEC="${_S1}${_S2}" python3 -c "
@@ -280,6 +283,31 @@ git add -A >/dev/null; git commit -qm proxsplit
S18=$(run_probe "$R/s18")
row "18 proximity split by slice cut" "$S18" "BLOCK(aws.secret_key)"
# 20: an absent named tip whose GUESS is non-empty but wrong.
# Row 16 covers the absent-tip case where the guess scans nothing. This is the
# other half: the stale tracking ref holds the credential, one harmless local
# commit sits on top, so the guessed range is non-empty and credential-free.
# Blocking only the empty guess let this through with exit 0.
mkrepo s20
printf 'aws_access_key_id = %s\n' "$KEY" > secret.txt
git add -A >/dev/null
git commit -qm cred >/dev/null
git update-ref refs/remotes/origin/main HEAD
git remote add origin https://example.invalid/o.git
git remote set-head origin main >/dev/null 2>&1
echo 'harmless = 1' > ok.py
git add -A >/dev/null
git commit -qm harmless >/dev/null
run_absent_tip() {
cd "$1" || exit 1
local sha
sha=$(git rev-parse HEAD)
printf 'refs/heads/main %s refs/heads/main aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa7\n' "$sha" \
| bun "$CAND" origin https://example.invalid/o.git 2>&1
echo "___EXIT:$?"
}
S20=$(run_absent_tip "$R/s20")
row "20 absent named tip, non-empty wrong guess" "$S20" "BLOCK(aws.access_key)"
# E1: ordinary new-branch push must scan ONLY the new commit
mkrepo e1
printf 'aws_key = "%s"\n' "$KEY" > old.py
@@ -324,7 +352,11 @@ E4=$(cd "$R/e4" && printf 'refs/heads/main %s refs/heads/main %s\n' "$ZERO40" "$
| bun "$CAND" origin https://example.invalid/o.git 2>&1; echo "___EXIT:$?")
row "E4 branch delete skipped" "$E4" "ALLOW"
# E5: URL push keeps historical origin-shaped fallback
# E5: a URL push must NOT borrow origin's tip as its base.
# The credential is already on origin/main and the local commit adds only
# harmless content. Anchoring on origin (or excluding origin's commits as
# "already pushed") scans the harmless commit only and ships the credential to
# a remote that never had it. A URL target is described by no tracking ref.
mkrepo e5
printf 'aws_key = "%s"\n' "$KEY" > old.py
git add -A >/dev/null; git commit -qm old >/dev/null
@@ -334,7 +366,36 @@ git checkout -q -b feature
echo 'harmless = 1' > new.py; git add -A >/dev/null; git commit -qm new >/dev/null
E5=$(cd "$R/e5" && printf 'refs/heads/main %s refs/heads/main %s\n' "$(git rev-parse HEAD)" "$ZERO40" \
| bun "$CAND" https://example.invalid/direct.git '' 2>&1; echo "___EXIT:$?")
row "E5 URL push falls back to origin (narrow)" "$E5" "ALLOW"
row "E5 URL push does not borrow origin's base" "$E5" "BLOCK(aws.access_key)"
# E5b: the other half of E5 — over-scanning must not turn every URL push into a
# block. Same shape, no credential anywhere: the push has to be allowed.
mkrepo e5b
echo 'old = 1' > old.py
git add -A >/dev/null; git commit -qm old >/dev/null
git update-ref refs/remotes/origin/main HEAD
git remote add origin https://example.invalid/o.git
git checkout -q -b feature
echo 'harmless = 1' > new.py; git add -A >/dev/null; git commit -qm new >/dev/null
E5B=$(cd "$R/e5b" && printf 'refs/heads/main %s refs/heads/main %s\n' "$(git rev-parse HEAD)" "$ZERO40" \
| bun "$CAND" https://example.invalid/direct.git '' 2>&1; echo "___EXIT:$?")
row "E5b clean URL push still allowed" "$E5B" "ALLOW"
# 19: a line longer than the overlap must still contribute its END to the seam.
# One ~770 KB ASCII line whose tail is the qualifying label, then a line holding
# the secret. Carrying whole lines only leaves the seam with no context, so
# aws.secret_key never fires even though both halves are in the pushed diff.
mkrepo s19
SEC="$SEC" python3 -c "
import os
sec=os.environ['SEC']
filler='x'*770047
with open('big.txt','w') as f:
f.write(filler+' aws_secret_access_key =\n')
f.write(sec+' '+'y'*20000+'\n')"
git add -A >/dev/null; git commit -qm bigline >/dev/null
S19=$(run_probe "$R/s19")
row "19 label at the end of an over-overlap line" "$S19" "BLOCK(aws.secret_key)"
# E6: long-line slicer survives multi-byte text
mkrepo e6; KEY="$KEY" python3 -c "
@@ -370,7 +431,28 @@ if git init -q --object-format=sha256 "$R/.probe256" 2>/dev/null; then
ZERO64=$(printf '0%.0s' {1..64})
E8=$(cd "$d" && printf 'refs/heads/main %s refs/heads/main %s\n' "$(git rev-parse HEAD)" "$ZERO64" \
| bun "$CAND" origin https://example.invalid/o.git 2>&1; echo "___EXIT:$?")
row "E8 sha256 repo, new branch" "$E8" "BLOCK"
row "E8 sha256 repo, new branch" "$E8" "BLOCK(aws.access_key)"
# E8b: the row that actually proves the empty-tree OID fix. E8 alone does not:
# it carries a credential, so a build that cannot resolve the fallback range
# still "blocks", just with a diff error instead of a finding. A CLEAN sha256
# first push must be ALLOWED — under the hardcoded SHA-1 empty-tree OID it was
# hard-blocked, which is unusable rather than safe.
d="$R/e8b"
rm -rf "$d"
mkdir -p "$d"
cd "$d" || exit 1
git init -q --object-format=sha256 .
git config user.email t@t.t
git config user.name t
git checkout -q -b main
printf 'cfg = 1\nharmless = 2\n' > app.py
git add -A >/dev/null
git commit -qm seed >/dev/null
git remote add origin https://example.invalid/o.git
E8B=$(printf 'refs/heads/main %s refs/heads/main %s\n' "$(git rev-parse HEAD)" "$ZERO64" \
| bun "$CAND" origin https://example.invalid/o.git 2>&1; echo "___EXIT:$?")
row "E8b clean sha256 first push allowed" "$E8B" "ALLOW"
fi
echo
test/redact-prepush-scan-range.test.ts
+10 -2
View File
@@ -263,7 +263,7 @@ describe("S1: exclusion scoped to the push-target remote", () => {
expect(code).toBe(0);
});
test("an unconfigured name (URL push) also falls back rather than erroring", () => {
test("an unconfigured name (URL push) does not error, and does not exclude another remote's commits", () => {
const { originTip } = buildSecretOnSecondRemote();
const head = run(["rev-parse", "HEAD"]).trim();
const url = "file:///not-a-configured-remote";
@@ -271,7 +271,15 @@ describe("S1: exclusion scoped to the push-target remote", () => {
`refs/heads/feature ${head} refs/heads/feature ${originTip}\n`,
[url, url],
);
// The original concern, kept: an unconfigured name must not be turned into
// an invalid `--remotes=<url>/*` refspec and crash the hook.
expect(stderr).not.toContain("could not");
expect(code).toBe(0);
// The secret commit is reachable from the pushed tip and NOT from the tip
// this URL remote is at, so this push really does send it there. A URL is
// described by no remote-tracking ref, so "already on the private `other`
// remote" says nothing about this destination — excluding it would ship the
// credential with exit 0, the same shape S1 fixes for configured remotes.
expect(stderr).toContain("BLOCKED");
expect(code).not.toBe(0);
});
});