fix: close gateguard destructive-bash regex bypasses with tokenizer

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
#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 #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 #1843's
caveat — independent of this change.

Provenance: discovered during a security pass on ECC after PR
#1843 (block-no-verify shell-words rewrite) landed. Same class of
regex-based shell parser issue, same shape of fix.

Refs #1843.
This commit is contained in:
Jamkris
2026-05-13 14:54:27 +09:00
parent 209abd403b
commit 231c1fdbe8
2 changed files with 282 additions and 2 deletions

View File

@@ -1143,6 +1143,76 @@ function runTests() {
'second subagent edit should pass even on a new file');
})) passed++; else failed++;
// --- Shell-words tokenizer: bypasses the old regex missed ---
function expectDestructiveDeny(command, label) {
clearState();
const input = { tool_name: 'Bash', tool_input: { command } };
const result = runBashHook(input);
assert.strictEqual(result.code, 0, `${label}: exit code should be 0`);
const output = parseOutput(result.stdout);
assert.ok(output, `${label}: should produce JSON output`);
assert.strictEqual(output.hookSpecificOutput.permissionDecision, 'deny', `${label}: should deny`);
assert.ok(output.hookSpecificOutput.permissionDecisionReason.includes('Destructive'),
`${label}: reason should mention "Destructive"`);
}
function expectAllow(command, label) {
clearState();
writeState({ checked: ['__bash_session__'], last_active: Date.now() });
const input = { tool_name: 'Bash', tool_input: { command } };
const result = runBashHook(input);
assert.strictEqual(result.code, 0, `${label}: exit code should be 0`);
const output = parseOutput(result.stdout);
assert.ok(output, `${label}: should produce JSON output`);
if (output.hookSpecificOutput) {
assert.notStrictEqual(output.hookSpecificOutput.permissionDecision, 'deny', `${label}: should not deny`);
} else {
assert.strictEqual(output.tool_name, 'Bash', `${label}: pass-through should preserve input`);
}
}
if (test('denies short-form git push -f as destructive', () => {
expectDestructiveDeny('git push -f origin main', 'git push -f');
})) passed++; else failed++;
if (test('denies git reset --hard even with intervening -c global option', () => {
expectDestructiveDeny('git -c core.foo=bar reset --hard', 'git -c ... reset --hard');
})) passed++; else failed++;
if (test('denies rm -fr (reverse flag order)', () => {
expectDestructiveDeny('rm -fr /tmp/junk', 'rm -fr');
})) passed++; else failed++;
if (test('denies rm -r -f (split flag form)', () => {
expectDestructiveDeny('rm -r -f /tmp/junk', 'rm -r -f');
})) passed++; else failed++;
if (test('denies git reset HEAD --hard (with intervening ref)', () => {
expectDestructiveDeny('git reset HEAD --hard', 'git reset HEAD --hard');
})) passed++; else failed++;
if (test('denies git clean -fd (combined force+dirs flag)', () => {
expectDestructiveDeny('git clean -fd', 'git clean -fd');
})) passed++; else failed++;
if (test('denies destructive command in second chained segment', () => {
expectDestructiveDeny('echo y | rm -rf /tmp/junk', 'echo y | rm -rf');
})) passed++; else failed++;
if (test('allows destructive phrase quoted inside a commit message', () => {
expectAllow('git commit -m "fix: rm -rf race in worker"', 'rm -rf in -m');
})) passed++; else failed++;
if (test('allows SQL phrase quoted inside a commit message', () => {
expectAllow('git commit -m "docs: explain when drop table is safe"', 'drop table in -m');
})) passed++; else failed++;
if (test('allows git push --force-if-includes as a safety-checked variant', () => {
expectAllow('git push --force-with-lease --force-if-includes origin main',
'git push --force-if-includes');
})) passed++; else failed++;
// Cleanup only the temp directory created by this test file.
try {
if (fs.existsSync(stateDir)) {