Repository navigation
fix(redact-prepush): don't re-scan commits a catch-up merge brought in - #2592
Closed
Two-Six-Alpha-1115 wants to merge 1 commit into
Closed
Two-Six-Alpha-1115 wants to merge 1 commit into
Two-Six-Alpha-1115 wants to merge 1 commit into
Conversation
`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 that branch's last push becomes
an added line — content that is already published, already scanned, and
not this push's doing.
Two consequences, both observed:
· FALSE HIGH FINDINGS. A placeholder connection string in a fixture
someone else had already merged blocked an unrelated push as
db.url_with_password, telling the operator to rotate a credential
over a file they never touched. A guard that cries wolf on catch-up
merges is one people learn to bypass reflexively — which is exactly
how a real secret gets through.
· OVERSIZED SCANS. The SCAN_CHUNK_BYTES comment already 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. 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.
The narrowed range is `rev-list localSha --not remoteSha --remotes`.
remoteSha STAYS the base — it is what git tells us the remote has, and is
authoritative in a way --remotes is not, since tracking refs can be
absent or stale. Using --remotes alone excludes nothing in a repo without
them, so every commit ever made reads as new. That is the same false
positive from the other direction, and it is what the existing test
"only NEW content is scanned (remote..local), not pre-existing" catches.
When excluding tracking refs changes nothing, this push has no catch-up
commits and the plain range already describes it exactly — so we defer to
it. That keeps every non-catch-up push on the original gitStrict diff
path, which is what garrytan#1946's fail-closed regression test exercises. A
narrowing that silently retired that test would be a worse trade than the
false positives it set out to fix.
Each commit is diffed alone. A merge's combined diff shows only content
present in no parent, so a secret introduced while resolving a conflict
is still caught while an ordinary merge contributes nothing.
Tests: 22/22 existing prepush tests still pass (two of them fail without
the remoteSha base and the defer-to-plain-range guard respectively —
verified by mutation). 5 new tests build real repositories on disk and
pin both directions: a catch-up merge no longer re-scans published
content, and secrets in new commits, in merge resolutions, and in
repos with no remote are all still scanned.
|
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 |
garrytan
added a commit
that referenced
this pull request
Aug 16, 2026
…usion scoped to the push remote Two calibration/coverage fixes in the redaction guard: - env.kv's zero-or-more-prefix regex fired on ANY identifier ending in a credential suffix, so ordinary code (cacheKey:, sortKey:, partitionKey:, hotkey:, even monkey:) with an 8+-char entropic value hit a MEDIUM confirm prompt — a gate that cries wolf gets ignored. A name now only counts when its shape is credential-semantic: suffix separated by _/-/. (api_key, x-access-key, AUTH.TOKEN), a bare suffix (key:, token:), ALL-CAPS env style (APIKEY=, MY_APIKEY=), or a camel compound with a credential prefix (apiKey, authToken, clientSecret). The value stays capture group 1, so the shape check lives in validate (isCredentialShapedEnvName), not the regex. - gstack-redact-prepush's narrowing excluded commits reachable from ANY remote (`--not --remotes`), so a secret that had only ever reached a private/local-path remote was never scanned when later pushed to a PUBLIC remote. The exclusion is now scoped to the push target (`--remotes=<name>/*`) via the remote name git hands pre-push as $1 (the installed wrapper already forwards "$@"); stdin/CLI invocations and URL pushes without a configured name fall back to the historical all-remotes behavior. #2592's catch-up-merge fix is unaffected: upstream commits come from the same remote being pushed to. New coverage: env.kv negative controls (cacheKey/sortKey/partitionKey/ hotkey/monkey/idempotencyKey) + positive controls for all four name shapes; end-to-end hook tests proving a second-remote secret blocks a push to origin while origin-published catch-up content still doesn't, plus both fallbacks. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
4 of 9 tasks
Owner
|
Thank you — this landed in v1.67.0.0 (credited in the CHANGELOG). Closing since the fix is on main. 🙏 |
4 of 5 tasks
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
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 problem
remoteSha..localShais "everything new on this branch", which is not the same as "everything new to the remote". Mergeorigin/maininto a feature branch and every commit main gained since that branch's last push becomes an added line — content that is already published, already scanned, and not this push's doing.Two consequences, both observed in practice:
db.url_with_password, telling the operator to rotate a credential over a file they had never touched. A guard that cries wolf on catch-up merges is one people learn to bypass reflexively — which is exactly how a real secret eventually gets through.SCAN_CHUNK_BYTEScomment already 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.
The fix
The narrowed range is
rev-list localSha --not remoteSha --remotes, with two guards that matter more than the narrowing itself:remoteShastays the base. It is what git tells the hook the remote has, and it is authoritative in a way--remotesis not — tracking refs can be absent (fresh clone that never fetched, a remote with no tracking ref) or stale. Using--remotesalone excludes nothing in a repo without them, so every commit ever made reads as "new". That is the same false positive from the other direction, and it is precisely what the existing testonly NEW content is scanned (remote..local), not pre-existingcatches.When excluding tracking refs changes nothing, defer to the plain range. Such a push has no catch-up commits, so the existing range already describes it exactly. This keeps every non-catch-up push on the original
gitStrictdiff path — the one#1946's fail-closed regression test exercises. Without this guard that test still passes, because the new path never invokesgit diffand so the test's stub never fires: it would have gone green forever while testing nothing. A narrowing that silently retires a security regression test is a worse trade than the false positives it set out to fix.Each commit is diffed alone. A merge's combined diff shows only content present in no parent, so a secret introduced while resolving a conflict is still caught, while an ordinary merge contributes nothing. Falls back to the existing range logic for a fresh repo, an absent remote object, or a >500-commit rewrite.
Verification
redact-prepush-hooktests pass, including both#1946fail-closed cases.remoteShabase fails 2 tests; dropping the defer-to-plain-range guard fails 1. Restored: 27/27.test/redact-prepush-scan-range.test.ts) build real repositories on disk, because the behaviour under test is the git plumbing — a mockedgitwould test the mock. They pin both directions: a catch-up merge no longer re-scans published content (and the old two-dot range demonstrably did), and secrets in new commits, in merge resolutions, and in repos with no remote are all still scanned.redact-pattern-lint49/49,gstack-config-redact-keys6/6,browse/proxy-redact8/8.aws.access_key.One note for reviewers:
--ccis passed explicitly on the per-commitgit show, but it is self-documenting rather than load-bearing —git showalready defaults to a combined diff for merges, so no test can pin it. The merge-resolution test pins the coverage, not the flag. Called out in a comment there so nobody over-trusts it later.