diff --git a/bin/gstack-redact-prepush b/bin/gstack-redact-prepush index 786d522a4..14bac37c5 100755 --- a/bin/gstack-redact-prepush +++ b/bin/gstack-redact-prepush @@ -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 / 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"; // /HEAD → /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=/*` 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[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; } diff --git a/test/redact-prepush-fail-open.sh b/test/redact-prepush-fail-open.sh index f75fae3bf..f5ed452ec 100755 --- a/test/redact-prepush-fail-open.sh +++ b/test/redact-prepush-fail-open.sh @@ -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 diff --git a/test/redact-prepush-scan-range.test.ts b/test/redact-prepush-scan-range.test.ts index 2f2d31ec2..a31fe8413 100644 --- a/test/redact-prepush-scan-range.test.ts +++ b/test/redact-prepush-scan-range.test.ts @@ -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=/*` 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); }); });