fix: close two block-no-verify bypass holes (core.hooksPath case, -tn) - #1843
Conversation
Two real bypass holes in scripts/hooks/block-no-verify.js, surfaced during a CodeRabbit round-3 review on the downstream EGC port (Jamkris/everything-gemini-code#68). Both ship in current main. Bug 1 — core.hooksPath case-sensitivity bypass (critical) GIT_CONFIG_KEY_PREFIX was compared case-sensitively, but Git config section and variable names are case-insensitive (https://git-scm.com/docs/git-config — "The variable names are case-insensitive"). The following both slipped past the guard: git -c core.hookspath=/dev/null commit -m "msg" git -c core.HOOKSPATH=/dev/null commit -m "msg" Fix: lowercase the prefix constant and compare candidate tokens in lowercase form inside hasHooksPathOverride (both the "-c <key>" and "-c<key>" code paths). Bug 2 — -tn false positive (major) COMMIT_SHORT_OPTIONS_WITH_VALUE was missing 't'. Git parses `git commit -tn` as -t with template path 'n' (stuck-arg form per git-scm.com/docs/api-parse-options), not as `-t -n`. Without 't' in the set, isCommitNoVerifyShortFlag reached the 'n' check and falsely flagged the command as a --no-verify bypass — breaking legitimate `-t <template>` usage when the template path starts with the letter n (or is just the letter n). Fix: add 't' to COMMIT_SHORT_OPTIONS_WITH_VALUE so the scanner treats the rest of the combined-short token as the option's value and stops scanning before checking for 'n'. Tests (+3, 25 total in tests/hooks/block-no-verify.test.js): - blocks case-variant core.hooksPath (lowercase) - blocks case-variant core.hooksPath (uppercase) - still allows -tn (n is the -t template path, not a flag) Local verification: - yarn lint clean - all CI validators pass (validate-agents, validate-commands, validate-rules, validate-skills, validate-hooks, validate-install-manifests, validate-no-personal-paths) - node tests/run-all.js: 2369/2369 pass including the 3 new block-no-verify cases Caveat: yarn test currently fails at check-unicode-safety on skills/windows-desktop-e2e/SKILL.md (U+2605 stars) which landed in commit 1481628 — independent of this PR, but worth flagging. Provenance: originally discovered and fixed downstream in Jamkris/everything-gemini-code#68 (commit fbf7908). Backporting here since the same code path exists upstream.
📝 WalkthroughWalkthroughThis PR hardens git hook bypass detection in ChangesGit Hook Bypass Detection Hardening
Possibly Related PRs
Estimated Code Review Effort🎯 3 (Moderate) | ⏱️ ~22 minutes Poem
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 1
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@scripts/hooks/block-no-verify.js`:
- Around line 75-80: The current token scanner only checks whole-token `-n` and
misses `-an`/`-anm` clusters; update the short-option cluster parsing to iterate
characters after the leading '-' in each token (use the existing
COMMIT_SHORT_OPTIONS_WITH_VALUE set), process options sequentially: if a
character is 'n' immediately mark/return as --no-verify, if a character is in
COMMIT_SHORT_OPTIONS_WITH_VALUE treat the remainder of the token as that
option's value and stop scanning that token, otherwise continue to the next
character; apply this logic where tokens are scanned for no-verify to ensure
clusters like '-anm' and '-an' are correctly blocked.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: 6aab3fd1-76d3-42a9-9795-087d36767641
📒 Files selected for processing (2)
scripts/hooks/block-no-verify.jstests/hooks/block-no-verify.test.js
| // Short options that take a value. When seen as part of a combined | ||
| // short-option token (e.g. -tn), git's parser treats the rest of the | ||
| // token as the option's value (template path 'n' here), so the scanner | ||
| // must stop at this character — anything after it is the inline value, | ||
| // not another flag. | ||
| const COMMIT_SHORT_OPTIONS_WITH_VALUE = new Set(['m', 'F', 'C', 'c', 't']); |
There was a problem hiding this comment.
🧩 Analysis chain
🌐 Web query:
In Git, how are combined short options parsed for git commit`? Please verify from official Git documentation whether:
git commit -an -m "msg"is equivalent togit commit -a -n -m "msg";git commit -anm "msg"is equivalent togit commit -a -n -m "msg";git commit -tn -m "msg"is parsed as-t n -m "msg"rather than-t -n -m "msg".`
💡 Result:
Yes, all three are true according to Git's official parse-options documentation and source code. 1. git commit -an -m "msg" is equivalent to git commit -a -n -m "msg": Yes. Short options can be bundled/clustered after a single dash, so -an is parsed as -a followed by -n [1][2][3]. 2. git commit -anm "msg" is equivalent to git commit -a -n -m "msg": Yes. Bundling continues across multiple option groups; -anm processes -a, then -n, then -m, with "msg" as the argument to -m since -m requires one [1][3][4]. 3. git commit -tn -m "msg" is parsed as -t n -m "msg" rather than -t -n -m "msg": Yes. The -t option (for --template) requires an argument. In -tn, after processing -t, the next character n (with no dash) is taken as its required argument from the same argv entry (stuck form). -n is not processed here [5][6][3]. Git's parser uses parse_short_opt() which matches each short option char sequentially, advancing the pointer after each. For options needing args, get_value() peeks at the next char in the bundle or next argv as the arg [3]. This matches standard Unix/POSIX getopt-style parsing for short options [7].
Citations:
- 1: https://git-scm.com/docs/api-parse-options
- 2: https://mirrors.edge.kernel.org/pub/software/scm/git/docs/technical/api-parse-options.html
- 3: https://github.com/git/git/blob/master/parse-options.c
- 4: https://rtime.ciirc.cvut.cz/gitweb/git.git/blob_plain/02273fdbd07ddaa13a232db0ae63a720c4a013f0:/parse-options.c
- 5: https://man7.org/linux/man-pages/man1/git-commit.1.html
- 6: https://git-scm.com/docs/git-commit/2.25.1.html
- 7: https://stackoverflow.com/questions/18270133/command-line-options-should-short-options-be-restricted-to-1-character
Combined short tokens still allow a --no-verify bypass.
git commit -an -m "msg" and git commit -anm "msg" still slip through. The scanner only recognizes -n when the token starts with -n. According to Git's official parse-options parser, short options are processed sequentially within a cluster; when an option requires a value (like -m or -t), the remainder of the token becomes its argument. The check must iterate through the cluster and block n unless an earlier value-taking option has already consumed the remainder.
Suggested fix
function isCommitNoVerifyShortFlag(value) {
- return value === '-n' || /^-n[a-zA-Z]/.test(value);
+ if (!value.startsWith('-') || value.startsWith('--') || value === '-') {
+ return false;
+ }
+
+ for (const option of value.slice(1)) {
+ if (option === 'n') {
+ return true;
+ }
+ if (COMMIT_SHORT_OPTIONS_WITH_VALUE.has(option)) {
+ return false;
+ }
+ }
+
+ return false;
}🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@scripts/hooks/block-no-verify.js` around lines 75 - 80, The current token
scanner only checks whole-token `-n` and misses `-an`/`-anm` clusters; update
the short-option cluster parsing to iterate characters after the leading '-' in
each token (use the existing COMMIT_SHORT_OPTIONS_WITH_VALUE set), process
options sequentially: if a character is 'n' immediately mark/return as
--no-verify, if a character is in COMMIT_SHORT_OPTIONS_WITH_VALUE treat the
remainder of the token as that option's value and stop scanning that token,
otherwise continue to the next character; apply this logic where tokens are
scanned for no-verify to ensure clusters like '-anm' and '-an' are correctly
blocked.
Greptile SummaryThis PR closes two bypass holes in the
Confidence Score: 4/5Safe to merge — the case-sensitivity change is a genuine security improvement and all existing tests continue to pass. The core.hooksPath case-insensitivity fix is correct and closes a real bypass hole, confirmed by the two new tests that fail on the unfixed code. The t-in-set change is semantically correct. Minor gaps remain: the -tn test does not demonstrate an observable regression in this codebase, and the inline sticky-form case-variant has no dedicated test. The test file would benefit from one more test covering the inline mixed-case form git -ccore.HOOKSPATH= commit. Important Files Changed
Reviews (1): Last reviewed commit: "fix: close two block-no-verify bypass ho..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
| if (test('blocks case-variant core.hooksPath (lowercase)', () => { | ||
| const r = runHook({ tool_input: { command: 'git -c core.hookspath=/dev/null commit -m "msg"' } }); | ||
| assert.strictEqual(r.code, 2, `expected exit 2, got ${r.code}`); | ||
| assert.ok(/core\.hookspath/i.test(r.stderr), `stderr should mention core.hooksPath: ${r.stderr}`); | ||
| })) passed++; else failed++; | ||
|
|
||
| if (test('blocks case-variant core.hooksPath (uppercase)', () => { | ||
| const r = runHook({ tool_input: { command: 'git -c core.HOOKSPATH=/dev/null commit -m "msg"' } }); | ||
| assert.strictEqual(r.code, 2, `expected exit 2, got ${r.code}`); | ||
| })) passed++; else failed++; | ||
|
|
||
| if (test('still allows -tn (n is the -t template path, not a flag)', () => { | ||
| const r = runHook({ tool_input: { command: 'git commit -tn -m "msg"' } }); | ||
| assert.strictEqual(r.code, 0, `expected exit 0, got ${r.code}: ${r.stderr}`); | ||
| })) passed++; else failed++; |
There was a problem hiding this comment.
-tn test was already passing before this fix
Tracing git commit -tn -m "msg" through the OLD code: isCommitNoVerifyShortFlag('-tn') checks value === '-n' (false) and /^-n[a-zA-Z]/.test('-tn') (false — the regex requires the second character to be n, but -tn has t there). Neither branch fires, so the token falls through without blocking. The test would pass before and after the patch. The 't'-in-set fix is still the right semantic change, but the stated false-positive scenario does not manifest in this codebase's current isCommitNoVerifyShortFlag — worth documenting so future reviewers don't assume the test is a regression guard for an observable failure.
| if (test('blocks case-variant core.hooksPath (uppercase)', () => { | ||
| const r = runHook({ tool_input: { command: 'git -c core.HOOKSPATH=/dev/null commit -m "msg"' } }); | ||
| assert.strictEqual(r.code, 2, `expected exit 2, got ${r.code}`); | ||
| })) passed++; else failed++; |
There was a problem hiding this comment.
-c combined form with mixed-case key
The lowered.startsWith('-ccore.hookspath=') branch in hasHooksPathOverride now correctly handles git -ccore.HOOKSPATH=/dev/null commit, but there is no test covering this inline (no-space) form with mixed case. The existing suite only tests the space-separated form. A case-variant bypass via the sticky form would silently slip through if a future refactor broke this branch.
| if (test('blocks case-variant core.hooksPath (uppercase)', () => { | |
| const r = runHook({ tool_input: { command: 'git -c core.HOOKSPATH=/dev/null commit -m "msg"' } }); | |
| assert.strictEqual(r.code, 2, `expected exit 2, got ${r.code}`); | |
| })) passed++; else failed++; | |
| if (test('blocks case-variant core.hooksPath (uppercase)', () => { | |
| const r = runHook({ tool_input: { command: 'git -c core.HOOKSPATH=/dev/null commit -m "msg"' } }); | |
| assert.strictEqual(r.code, 2, `expected exit 2, got ${r.code}`); | |
| })) passed++; else failed++; | |
| if (test('blocks inline -c form with mixed-case key (e.g. -ccore.HOOKSPATH=)', () => { | |
| const r = runHook({ tool_input: { command: 'git -ccore.HOOKSPATH=/dev/null commit -m "msg"' } }); | |
| assert.strictEqual(r.code, 2, `expected exit 2, got ${r.code}`); | |
| })) passed++; else failed++; |
…tch, subshell, +refspec)
Five additional review findings on top of the round-1 tokenizer fix.
Combined patch surface is small (one push branch, new switch branch,
exploded subshell handling); all six review issues are now closed.
P1 — --force --force-if-includes still destructive (Greptile, line 217):
Previous logic treated --force-if-includes as a safety guarantee
alongside --force-with-lease. Per git-scm.com/docs/git-push,
--force-if-includes is a no-op WITHOUT --force-with-lease, so a
combination of --force --force-if-includes is just --force. Push
branch now treats only --force-with-lease as a lease, and reports
force when --force / -f is present.
P2 — git switch destructive forms not detected (Greptile, line 234):
Added a switch branch to isDestructiveGit covering:
--discard-changes (explicit discard)
--force / -f (ignore conflicts, overwrite)
-C <branch> (force-create, overwrites existing branch)
P0 — backtick + $(...) subshell bypass (CodeRabbit, line 64):
Added explodeSubshells() that promotes `...` and $(...) contents
to top-level segment separators. Run on both the SQL/dd regex
input and the per-segment shell tokenizer input. Loops up to 4
passes to catch a layer of nesting. Without this,
`echo y | $(rm -rf /tmp)` slipped past the segment splitter
because the destructive command lived inside a sub-expression.
P0 — +refspec force push (CodeRabbit, line 217):
`git push origin +main`, `+refs/heads/main:refs/heads/main`, etc.
force a non-fast-forward update of that specific ref. Push branch
now also flags any positional arg starting with `+` that matches
a refspec shape. Excludes bare `+` and numeric-only tokens.
P2 — missing --force --force-if-includes regression test
(Greptile, line 1202): added.
Tests (+10 on top of the round-1 +10):
Bypass-now-blocked:
- git push --force --force-if-includes (force-if-includes is no-op
without lease — bare force is still in effect)
- git push origin +main (+refspec bare branch)
- git push origin +refs/heads/main:refs/heads/main (+refspec full)
- git switch --discard-changes
- git switch --force
- git switch -f (short form)
- git switch -C (force-create)
- echo y | `rm -rf /tmp` (backtick subshell)
- echo y | $(rm -rf /tmp) (dollar-paren subshell)
Still-allowed:
- git switch feature (plain)
67/67 in gateguard-fact-force.test.js. 2380/2380 across the full
suite. yarn lint clean. All seven CI validators pass.
Refs affaan-m#1843.
2 participants
Summary
Two real bypass holes in
scripts/hooks/block-no-verify.js, surfaced during a CodeRabbit round-3 review on the downstream EGC port (Jamkris/everything-gemini-code#68). Both ship in currentmain.Bug 1 —
core.hooksPathcase-sensitivity bypass (critical)GIT_CONFIG_KEY_PREFIXwas compared case-sensitively, but Git config section and variable names are case-insensitive (https://git-scm.com/docs/git-config — "The variable names are case-insensitive"). The following both slipped past the guard:Fix: lowercase the prefix constant and compare candidate tokens in lowercase form inside
hasHooksPathOverride(both the-c <key>and-c<key>code paths).Bug 2 —
-tnfalse positive (major)COMMIT_SHORT_OPTIONS_WITH_VALUEwas missing't'. Git parsesgit commit -tnas-twith template path'n'(stuck-arg form per https://git-scm.com/docs/api-parse-options), not as-t -n. Without't'in the set,isCommitNoVerifyShortFlagreached thencheck and falsely flagged the command as a--no-verifybypass — breaking legitimate-t <template>usage when the template path starts with the lettern(or is just the lettern).Fix: add
't'toCOMMIT_SHORT_OPTIONS_WITH_VALUEso the scanner treats the rest of the combined-short token as the option's value and stops scanning before checking forn.Tests (+3, 25 total in
tests/hooks/block-no-verify.test.js)blocks case-variant core.hooksPath (lowercase)—git -c core.hookspath=/dev/null commit -m "msg"→ exit 2blocks case-variant core.hooksPath (uppercase)—git -c core.HOOKSPATH=/dev/null commit -m "msg"→ exit 2still allows -tn (n is the -t template path, not a flag)—git commit -tn -m "msg"→ exit 0Local verification
yarn lintcleanvalidate-agents,validate-commands,validate-rules,validate-skills,validate-hooks,validate-install-manifests,validate-no-personal-pathsnode tests/run-all.js: 2369/2369 pass including the 3 new block-no-verify casesyarn testcurrently fails atcheck-unicode-safetyonskills/windows-desktop-e2e/SKILL.md(U+2605 stars) which landed in commit 1481628. That failure exists onmainindependently of this PR and would also fail without these changes — flagging it here in case it's news.Provenance
Originally discovered and fixed downstream in Jamkris/everything-gemini-code#68 (commit
fbf7908). Backporting here since the same code path exists upstream. The downstream port has been running with these fixes against EGC's own pre-commit hooks for a day with no false-positive reports.Happy to split into two PRs (one critical, one major) if that's easier to triage — the two changes are independent. Also happy to drop the new tests or rework them in any direction.
Summary by cubic
Fixes two bypass holes in
scripts/hooks/block-no-verify.jsto correctly blockcore.hooksPathoverrides and stop false positives forgit commit -tn. This strengthens hook enforcement without breaking valid commits.core.hooksPathas case-insensitive when parsing-c(supports both-c key=...and-ckey=...forms).-tto short options that take a value so-tnis parsed as a template path, not-n.Written for commit ff37e40. Summary will update on new commits.
Summary by CodeRabbit
Bug Fixes
Tests