fix(redact-prepush): six paths where the hook exits 0 on a real credential - #2498
Open
lubosxyz wants to merge 1 commit into
Open
fix(redact-prepush): six paths where the hook exits 0 on a real credential#2498lubosxyz wants to merge 1 commit into
lubosxyz wants to merge 1 commit into
Conversation
…ntial
The pre-push credential scanner can be made to allow a push carrying a live
key. Each hole is a separate path to the same outcome — `exit 0`, nothing
scanned, secret on the remote:
1. Added lines are filtered with `!startsWith("+++")`, which also drops a real
added line whose *content* begins with `++`. A committed .patch or .diff
carrying a key is never scanned.
2. A ref update line that failed to parse was `continue`d, so malformed stdin
meant exit 0 having scanned nothing.
3. `diff.external` (an ordinary user config) makes `git diff` emit no `+`
lines at all; the scanner reads an empty diff and allows anything.
4. Pushing to a remote other than `origin`: the range was resolved against
origin regardless, so HEAD..HEAD came back empty.
5. A junk remote sha was treated as a resolvable base.
6. `localSha="0"` was read as a branch delete and skipped.
Also fixes the false positive that drove users to the bypass: over the engine's
1 MiB cap the hook told them to "rotate the credential (a pushed secret is
compromised)" for content it had never scanned. On this machine that produced
five entries in ~/.gstack/security/prepush-skip.jsonl, none of which contained
a credential — a gate that cries wolf trains its user to reach for the escape
valve, which then gets used on the push where the gate was right.
Fail-closed is preserved throughout: an unreadable range still blocks, and the
new "unscannable" message names the range, the reason, how much was read, and
the concrete next step instead of dying with `signal 9`.
test/redact-prepush-fail-open-verify.sh runs all 15 scenarios against both the
shipped and the fixed scanner and exits non-zero on any mismatch. Rows 2,4,5,6,
9,14 must agree between the two — that is the no-detection-regression check.
|
Merging to
After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
The pre-push credential scanner can be made to allow a push that carries a live key. Six separate paths, same outcome:
exit 0, nothing scanned, secret on the remote.++diff.externalsetoriginlocalSha="0"read as a branch delete#1 is the one I'd fix first even alone: added lines are filtered with
!line.startsWith("+++"), which is meant to drop the+++ b/fileheader — but a genuine added line whose content begins with++renders as+++…too. A committed.patch/.diffcarrying a key sails through unscanned.#3 needs no attacker:
diff.externalis ordinary user config. With it set,git diffemits no+lines, the scanner reads an empty diff, and everything is allowed. Silently.The false positive that teaches the bypass
Over the engine's 1 MiB cap the hook reports
engine.input_too_largeas HIGH and tells the user to "Rotate the credential (a pushed secret is compromised)" — for content it never scanned. On one machine that produced five entries in~/.gstack/security/prepush-skip.jsonl, including "0 HIGH at 1.33 MB" and "synthetic credential-scrubbing fixture". None held a credential.That matters beyond ergonomics: a gate that cries wolf trains its user to reach for
GSTACK_REDACT_PREPUSH=skip, and the habit is then applied to the push where the gate was right. The fix scans in windows so large-but-clean input passes, and reserves blocking for content that genuinely could not be read.Fail-closed is preserved
An unreadable range still blocks (#1946 stands). What changed is the message: instead of
died of signal 9with no explanation, it names the range, the reason, how many MB were read, and the concrete next step.Verification
test/redact-prepush-fail-open-verify.sh— 15 scenarios, each run against both the shipped and the fixed scanner, asserting an expected verdict per row and exiting non-zero on mismatch. Rows 2, 4, 5, 6, 9, 14 must agree between the two columns: that is the no-detection-regression check, including credentials straddling the window cut.I verified this independently of the change's author, against
bin/gstack-redact-prepushandlib/redact-engine.tsthat are byte-identical (sha2565c06f7de…/c1f5df32…) to currentmainat v1.61.0.0, so the result applies to this base.Relationship to open PRs
Disjoint from #2398 (pushed range vs whole repo), #2328 (path-ignore) and #2358 (chained
pre-push.localstdin newline) — I diffed all three and none touches these six paths. #2328 is motivated by the same false positive from the other direction; #2358 is a seventh fail-open path in the wrapper that this PR does not address.Happy to rebase behind any of them if you'd rather land those first.