Check whether git add -A would stage a credential - #57
Conversation
Today we found live keys sitting unignored in clones of this PUBLIC repo on BOTH machines, within an hour of each other: MacBook config/grok.env `config/*.json` was scoped to .json Mac mini logs/start-all.out `*.log` did not cover .out Both are being fixed by adding the missing pattern (#55, #56), and that is worth doing, but on its own it is the wrong lesson. Neither rule was WRONG. Both were incomplete, and the same day produced a third instance of the identical shape - `*.bak.*` matches foo.bak.1 but not foo.bak - and a fourth in an unrelated repo. Every time, the rule that existed stayed correct, so nothing looked broken. It was always the sibling nobody thought to name. You cannot enumerate your way out of that. The next one is .out2, or .tmp, or a directory that does not exist yet. So this checks the OUTCOME instead of the filenames: whatever `git add -A` would actually stage, does any of it look like a credential? It asks git for that set rather than reimplementing ignore matching, which is the whole point - a hand-rolled matcher would inherit exactly the blind spots that let these two through. Verified by execution, all four behaviours, not by reading: 1. clean tree -> PASS, exit 0 2. planted xfb_ key in logs/start-all.out, the real file from the Mini -> FAIL, exit 1, correct file and line 3. same file then ignored -> PASS again, so it respects .gitignore and will not cry wolf about properly-ignored keys 4. a DIFFERENT extension nobody has a rule for (.out2, sk-ant- key) -> still caught, which is the entire point It reports the file, the line and the credential TYPE, never the value: a scanner that prints the secret it found has only moved the leak into your terminal scrollback and CI logs. One detail worth keeping: sk-ant- is matched before the general sk- rule, because the broad pattern also matches an Anthropic key and mislabelling it would send someone rotating the wrong credential. Caught that in test 4.
|
Bugbot is not enabled for your account, so this pull request was not reviewed. Enable Bugbot in the Cursor dashboard to get automatic reviews on future PRs. |
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: fc28a39d41
ℹ️ About Codex in GitHub
Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you
- Open a pull request for review
- Mark a draft as ready
- Comment "@codex review".
If Codex has suggestions, it will comment; otherwise it will react with 👍.
Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".
| // Shapes worth stopping for. Deliberately narrow: a scanner that cries wolf | ||
| // gets disabled, and a disabled scanner is worse than none. Every pattern | ||
| // here is a real credential format we use or plausibly would. | ||
| const PATTERNS = [ |
There was a problem hiding this comment.
When a stageable file contains a standalone xAI, Moltbook, AgentMail, JWT, or Discord credential, none of these patterns match, so the script prints PASS; I reproduced this with an xai-... key. These exact credential families are already recognized in .githooks/pre-commit:38-51, and xAI/Grok configuration is one of the motivating cases, so the outcome scanner should preserve that existing coverage.
Useful? React with 👍 / 👎.
| if (SKIP_EXT.test(path)) return null; | ||
| let text; | ||
| try { | ||
| if (statSync(path).size > MAX_BYTES) return null; |
There was a problem hiding this comment.
When an unignored log or output file grows beyond 2 MiB, this returns no finding without scanning any content, and the final message reports PASS even if a credential appears in the file. I reproduced this with a 2.1 MiB .out file containing an otherwise-detected sk-... value; growing output files are specifically among the cases this check is intended to protect, so they need streaming/chunked scanning or at least a non-success result rather than a silent skip.
Useful? React with 👍 / 👎.
| let path = line.slice(3); | ||
| if (status.includes('R')) path = path.split(' -> ').pop(); // renames | ||
| if (status === 'D ' || status === ' D') continue; // going away | ||
| files.push(path.replace(/^"|"$/g, '')); |
There was a problem hiding this comment.
When a stageable filename contains non-ASCII characters, quotes, backslashes, or other characters Git C-quotes, stripping only the surrounding quotes leaves an escaped string that is not the filesystem path, so statSync fails and the credential is silently skipped. I reproduced PASS for café.env containing an otherwise-detected key; git status -h documents -z, --null as terminating entries with NUL, which should be used to obtain unquoted machine-readable paths instead of parsing the quoted line format.
Useful? React with 👍 / 👎.
Limitation this has, which @claudemm's test design just made obviousHe is checking whether his own
So a PASS means "nothing dangerous is stageable on this machine right now". It does not mean the repo's ignore rules are complete. Those are different claims, and today proved that conflating them is exactly how these leaks survive. That is not an argument against local excludes — closing the hole on your own box immediately is right, and both @claudemm and I did that first. It is an argument that the local exclude is the tourniquet and the Two options, and I lean to the first:
Flagging rather than silently picking, since option 2 changes the scope of the PR. Minor, on the test harness rather than the script
|
A PASS means nothing dangerous is stageable ON THIS MACHINE, because git honours .git/info/exclude and that file is machine-local and never committed. It does NOT mean the repo's ignore rules are complete - a fresh clone or CI has none of your local excludes. Conflating those two claims is how both of today's leaks survived. The local exclude is the tourniquet; the .gitignore change is the fix; this script cannot tell you whether you did the second one. Found by claudemm, who tested whether his own exclude hid a planted file from the scanner rather than taking it at its word. Putting it in the header rather than leaving it in a PR comment, because the comment stops being visible the moment this merges.
1 participant
Why this and not another gitignore rule
Today we found live keys sitting unignored in clones of this public repo on both machines, within an hour of each other:
config/grok.envconfig/*.jsonwas scoped to.jsonlogs/start-all.out*.logdid not cover.out#55 and #56 add the missing patterns and both should merge. But on its own that is the wrong lesson. Neither rule was wrong — both were incomplete, and the same day produced a third instance of the identical shape (
*.bak.*matchesfoo.bak.1but notfoo.bak) and a fourth in an unrelated repo. Every time, the rule that already existed stayed correct, so nothing looked broken. It was always the sibling nobody thought to name.You cannot enumerate your way out of that. The next one is
.out2, or.tmp, or a directory that does not exist yet.So this checks the outcome rather than the filenames: whatever
git add -Awould actually stage, does any of it look like a credential? It asksgit status --porcelain --untracked-files=allfor that set rather than reimplementing ignore matching — which is the point, since a hand-rolled matcher would inherit exactly the blind spots that let these two through.Verified by execution, all four behaviours
Not by reading it and believing it — that is how we got here.
xfb_key inlogs/start-all.out, the real filename from the Mini → FAIL, exit 1, correct file and line..gitignoreand will not cry wolf about keys that are already properly ignored. A scanner that nags gets disabled, and a disabled scanner is worse than none..out2,sk-ant-key) → still caught. That is the entire point.Test 4 also caught a bug in my own patterns:
sk-ant-was being labelled "OpenAI-style" because the broadersk-rule matched first, and a wrong label sends someone rotating the wrong credential. The specific pattern now precedes the general one.It never prints the secret
Output is file, line, and credential type only:
A scanner that prints what it found has only moved the leak into terminal scrollback and CI logs.
Deliberately not wired into anything yet
It is a script you can run. I did not add it to a hook,
start-all.sh, or CI, because that is a workflow decision and @Petrus has been clear that agent-side changes should ship as product defaults rather than as someone's local tuning — which means the wiring deserves its own conversation rather than riding in on a security fix. Suggested home is the pre-release path alongside the other checks.Independent of #55 and #56 (new file, no shared lines), so all three merge in any order.