fix: close gateguard destructive-bash regex bypasses with tokenizer - #1853
Merged
affaan-m merged 3 commits intoMay 13, 2026
Merged
Conversation
Six classes of bypass in scripts/hooks/gateguard-fact-force.js DESTRUCTIVE_BASH regex, plus a separate false-positive class. Same shape of issue as the block-no-verify holes addressed in affaan-m#1843: a single-regex shell parser can never cover the flag-order variations git and rm allow. Real bypasses observed locally (all ALLOW today, should BLOCK): git push -f origin main (short form of --force) git -c core.foo=bar reset --hard (intervening -c global) rm -fr /tmp/junk (reverse flag order) rm -r -f /tmp/junk (split flag form) git reset HEAD --hard (intervening ref token) git clean -fd (combined -f + -d flag) False positive observed locally (BLOCK today, should ALLOW): git commit -m "fix: rm -rf race in worker" (destructive phrase inside quoted message) Behavior fix that comes along: --force-if-includes is now exempted alongside --force-with-lease. Both are safety-checked variants; the previous regex used a negative lookahead that only spelled out --with-lease, so --force-if-includes blocked under the old code even though it is the safer-not-harder choice. Fix shape (mirrors block-no-verify affaan-m#1843): - DESTRUCTIVE_SQL_DD regex kept for `drop table`, `delete from`, `truncate`, `dd if=` — these are stable keyword phrases. Quoted strings are stripped before the regex runs so the phrase is not matched inside a commit message body. - isDestructiveBash() tokenizes the command into segments at unquoted ; | & boundaries, then per segment: * isDestructiveRm — detects `rm` with both r and f set across combined or split flag tokens. * isDestructiveGit — finds the git subcommand after skipping global options (-c key=val, -C path, --git-dir=, etc.), then handles reset, checkout --, clean -f*, push --force (with --force-with-lease / --force-if-includes exemption), commit --amend, and rm -r* preservation. - Command tokens go through commandBasename() so /usr/bin/rm, rm.exe, and RM all normalize to "rm". Tests (+10 in tests/hooks/gateguard-fact-force.test.js): Bypass-now-blocked (7): - denies short-form git push -f - denies git reset --hard with intervening -c global option - denies rm -fr (reverse flag order) - denies rm -r -f (split flag form) - denies git reset HEAD --hard - denies git clean -fd - denies destructive command in second chained segment False-positive-now-allowed (3): - allows destructive phrase inside `-m` commit message (rm -rf) - allows SQL phrase inside `-m` commit message (drop table) - allows --force-if-includes as a safety-checked variant Local verification: yarn lint clean scripts/ci/validate-* (agents/commands/rules/skills/hooks/ install-manifests/no-personal-paths) pass node tests/run-all.js 2380/2380 pass Caveat (unrelated): yarn test still fails at check-unicode-safety on skills/windows-desktop-e2e/SKILL.md (U+2605) per affaan-m#1843's caveat — independent of this change. Provenance: discovered during a security pass on ECC after PR affaan-m#1843 (block-no-verify shell-words rewrite) landed. Same class of regex-based shell parser issue, same shape of fix. Refs affaan-m#1843.
Contributor
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: defaults Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (2)
🚧 Files skipped from review as they are similar to previous changes (2)
📝 WalkthroughWalkthroughReplaces a fragile DESTRUCTIVE_BASH regex with a tokenized parser plus SQL/rm/git detectors; normalizes quoting/subshells, extracts command-substitution bodies, finds git subcommands while skipping global options, and integrates isDestructiveBash into the hook. Tests add tokenizer-edge-case coverage. ChangesDestructive bash command detection refactor
Estimated code review effort🎯 4 (Complex) | ⏱️ ~45 minutes Possibly related issues
Possibly related PRs
Suggested reviewers
Poem
🚥 Pre-merge checks | ✅ 5✅ Passed checks (5 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 |
Contributor
Greptile SummaryThis PR replaces the single
Confidence Score: 4/5Safe to merge; the tokenizer is a clear improvement over the old regex and all addressed bypasses are locked in with tests. The core tokenizing approach is correct and thoroughly tested. Two small gaps remain: git rm --recursive is not flagged by the isDestructiveGit rm handler, and the prev !== backslash guard in extractCommandSubstitutions misfires on double-backslash+quote sequences — both result in over-blocking rather than a bypass. The git rm subcommand handler (~line 359) and the escape-tracking logic in extractCommandSubstitutions (lines 111-118) are worth a second look. Important Files Changed
Reviews (3): Last reviewed commit: "fix: cover remaining gateguard tokenizer..." | Re-trigger Greptile |
![greptile-apps[bot]](https://avatars.githubusercontent.com/in/867647?s=60&v=4){kind=small}
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
Contributor
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
scripts/hooks/gateguard-fact-force.js (1)
114-125: 💤 Low valueInconsistent case handling for rm flags.
Line 121 checks for both
randR:/[rR]/, but line 122 checks only lowercasef:/f/. Whilermonly accepts lowercase-f(no-Foption exists), the inconsistency creates maintenance confusion. For symmetry, either check[fF]or document why only lowercase is needed.♻️ Suggested fix for consistency
if (/[rR]/.test(body)) hasR = true; - if (/f/.test(body)) hasF = true; + if (/[fF]/.test(body)) hasF = true;🤖 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/gateguard-fact-force.js` around lines 114 - 125, The isDestructiveRm function mixes case handling (uses /[rR]/ but /f/), so update the flag checks for consistency: when building body from tokens.slice(1) (variable body inside isDestructiveRm), change the regexes to both use lowercase-only checks (e.g., /r/ and /f/) and add a brief inline comment near the body check noting that -f is only a lowercase flag for rm; this keeps behavior correct while removing the confusing mixed-case handling (alternatively, if you prefer to accept uppercase flags, change both to /[rR]/ and /[fF]/—apply the one consistent approach throughout isDestructiveRm).
🤖 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/gateguard-fact-force.js`:
- Around line 60-64: stripQuotedStrings currently only removes single/double
quoted strings and misses backticks and command substitution ($(...)), allowing
destructive commands to slip through; update stripQuotedStrings to also strip
backtick-delimited segments and $(...) segments (e.g., replace
/`(?:[^`\\]|\\.)*`/g and a $() regex like /\$\((?:[^()\\]|\\.|(?R))*\)/g or
apply a simplified iterative removal of /\$\((?:[^()\\]|\\.)*?\)/g repeatedly)
so backtick and $() content are normalized the same way as quoted strings;
ensure the updated function is used by isDestructiveRm so command substitution
cannot bypass detection and consider iterating/removing nested $() until none
remain for better coverage.
- Around line 190-217: The push-check currently only sets force=true for
explicit --force flags and short -f but misses refspecs prefixed with '+'.
Update the loop that iterates over rest to also mark force=true when a token is
a refspec starting with '+' (e.g. t.startsWith('+')), so tokens like '+main' or
'+refs/heads/feature' trigger the same destructive detection; keep the existing
logic and the final return force && !safe.
---
Nitpick comments:
In `@scripts/hooks/gateguard-fact-force.js`:
- Around line 114-125: The isDestructiveRm function mixes case handling (uses
/[rR]/ but /f/), so update the flag checks for consistency: when building body
from tokens.slice(1) (variable body inside isDestructiveRm), change the regexes
to both use lowercase-only checks (e.g., /r/ and /f/) and add a brief inline
comment near the body check noting that -f is only a lowercase flag for rm; this
keeps behavior correct while removing the confusing mixed-case handling
(alternatively, if you prefer to accept uppercase flags, change both to /[rR]/
and /[fF]/—apply the one consistent approach throughout isDestructiveRm).
🪄 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: 1b53a5d5-2354-46ce-8b89-82b3409c5992
📒 Files selected for processing (2)
scripts/hooks/gateguard-fact-force.jstests/hooks/gateguard-fact-force.test.js
…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.
5 tasks
4 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
2 participants
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.
Summary
Six bypass classes in
scripts/hooks/gateguard-fact-force.jsDESTRUCTIVE_BASHregex, plus a separate false-positive class. Same shape as the block-no-verify holes addressed in #1843 — a single-regex shell parser cannot cover the flag-order / argument-order variations git and rm allow.Real bypasses (all ALLOW today, should BLOCK)
Probed against the current regex on
main:False positive (BLOCK today, should ALLOW)
A destructive phrase mentioned inside a quoted commit message body triggers the gate today because the regex runs on the raw command string.
Behavior fix that comes along
git push --force-if-includeswas BLOCKED by the old code because the negative lookahead only spelled out--force-with-lease. It's a safety-checked variant in the same family as--force-with-lease— exempting both is more correct than blocking one and allowing the other. Worth a separate eye in review; happy to revert this exemption if it's not wanted.Fix shape (mirrors #1843)
DESTRUCTIVE_SQL_DDregex kept fordrop table,delete from,truncate,dd if=— stable keyword phrases without flag-order variation. Quoted strings are stripped first so the phrase is not matched inside a commit message body.isDestructiveBash()tokenizes the command into segments at unquoted;,|,&boundaries, then per segment:isDestructiveRm— detectsrmwith bothrandfset across combined or split flag tokens.isDestructiveGit— finds the git subcommand after skipping global options (-c key=val,-C path,--git-dir=, etc.), then handlesreset,checkout --,clean -f*,push --force(with--force-with-lease/--force-if-includesexemption),commit --amend, andrm -r*preservation.commandBasename()so/usr/bin/rm,rm.exe, andRMall normalize torm.Tests (+10 in
tests/hooks/gateguard-fact-force.test.js)Bypass now blocked (7):
git push -fgit -c key=value reset --hardrm -frrm -r -fgit reset HEAD --hardgit clean -fdecho y | rm -rf /tmp/junk(second segment of chain)False-positive now allowed (3):
git commit -m "fix: rm -rf race in worker"git commit -m "docs: explain when drop table is safe"git push --force-with-lease --force-if-includes origin mainExisting 47 tests in this file still pass —
git push --force-with-lease,git commit --amend, plaingit push --force,git reset --hard, etc. all continue to behave as they did.Local verification
yarn lintcleanvalidate-agents,validate-commands,validate-rules,validate-skills,validate-hooks,validate-install-manifests,validate-no-personal-pathsnode tests/run-all.js— 2380/2380 pass (was 2369 before this PR; +11 from the new gateguard cases and a +1 unrelated test that landed since fix: close two block-no-verify bypass holes (core.hooksPath case, -tn) #1843)yarn testcontinues to fail atcheck-unicode-safetyonskills/windows-desktop-e2e/SKILL.md(U+2605) per fix: close two block-no-verify bypass holes (core.hooksPath case, -tn) #1843's caveat — exists onmainindependently of this PR.Provenance
Discovered during a security pass on ECC after #1843 (block-no-verify shell-words rewrite) landed. Same class of regex-based shell parser issue, same shape of fix. Cross-linking that PR since the maintainer just reviewed and merged the same pattern.
Happy to split or restructure in any direction.
Summary by cubic
Replaced the destructive-Bash regex with tokenized parsing in
scripts/hooks/gateguard-fact-force.jsto block dangerousrm/gitcommands, handle subshells, and avoid false positives in quoted commit messages. Force push logic now treats only--force-with-leaseas safe;--force-if-includesalone does not bypass, and+refspecs are blocked unless paired with--force-with-lease.rmwhen both recursive and force are set (combined, split, reversed, long-form--recursive/--force), including inside$(...)and backticks.git reset --hard(even with-cor an intervening ref),git clean -f*,git checkout --, andgit commit --amend.git pushwith-f/--forceand with+refspecs; allows--force-with-lease(optionally with--force-if-includes); denies--force --force-if-includes.git switchforms:--discard-changes,--force/-f, and-C.git commit -m "rm -rf"); still detects SQL/DD phrases outside quotes.Written for commit 25ea9a7. Summary will update on new commits.
Summary by CodeRabbit
Improvements
Tests