From 4afdb90800faa4edcc0ea0d7bfe0a55ddb6e8036 Mon Sep 17 00:00:00 2001 From: Gaurav Dubey Date: Sun, 7 Jun 2026 10:31:30 +0530 Subject: [PATCH] feat(gateguard): add env knobs for routine bash gate + extra destructive patterns (#2161) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(gateguard): add env knobs for routine bash gate + extra destructive patterns The JS port of gateguard-fact-force has two bash gates: a destructive gate (rm -rf, drop table, git push --force, etc.) that operators want to keep, and a once-per-session routine gate that fires on the very first bash invocation regardless of intent. Operators on hosts where the routine gate is friction without signal (Cursor, OpenCode, etc.) have been maintaining local patches that get clobbered on every plugin update; the Python upstream gateguard-ai already exposes equivalent config via .gateguard.yml. Adds two env vars, both off-by-default so existing behavior is preserved: - GATEGUARD_BASH_ROUTINE_DISABLED — truthy values (1, true, on, yes, enabled) skip the routine bash gate. Destructive gate is unaffected. - GATEGUARD_BASH_EXTRA_DESTRUCTIVE — regex source string for additional destructive patterns. Matches against the same quote-stripped, subshell-flattened command the built-in DESTRUCTIVE_SQL_DD regex sees, so a custom phrase inside $(...) or backticks is also caught. A malformed regex is logged once to stderr and treated as not configured rather than crashing the hook (hooks must never block tool execution unexpectedly). Twelve new tests pin both env vars (truthy aliases, falsy values, unset baseline, destructive-gate-still-fires, alternation members, malformed regex degrades safely, custom phrase inside command substitution). Existing 2619/2619 tests still pass; eslint clean. Fixes #2078 * fix(gateguard): reset extra-destructive warn-once gate when env value changes Both reviewers (CodeRabbit + cubic) flagged that extraDestructiveWarnLogged was never reset when GATEGUARD_BASH_EXTRA_DESTRUCTIVE flipped from one invalid regex to a different invalid regex. The sticky boolean meant a long-running process saw bad-pattern-a's warning then silently swallowed bad-pattern-b's parse failure. Fix: clear extraDestructiveWarnLogged whenever the cache key changes (i.e. before the regex compile attempt). The warn-once-per-distinct- pattern invariant now matches the per-key cache invariant. Adds a same-process regression test via loadDirectHook() that spies on process.stderr.write and asserts: same bad pattern warns once across multiple invocations; switching to a different bad pattern emits a second warning; switching to a valid regex emits zero warnings. --- scripts/hooks/gateguard-fact-force.js | 67 ++++++- tests/hooks/gateguard-fact-force.test.js | 212 +++++++++++++++++++++++ 2 files changed, 278 insertions(+), 1 deletion(-) diff --git a/scripts/hooks/gateguard-fact-force.js b/scripts/hooks/gateguard-fact-force.js index c7c84c5a..741d3032 100644 --- a/scripts/hooks/gateguard-fact-force.js +++ b/scripts/hooks/gateguard-fact-force.js @@ -46,6 +46,7 @@ const ROUTINE_BASH_SESSION_KEY = '__bash_session__'; const EDIT_WRITE_HOOK_ID = 'pre:edit-write:gateguard-fact-force'; const BASH_HOOK_ID = 'pre:bash:gateguard-fact-force'; const ECC_DISABLE_VALUES = new Set(['0', 'false', 'off', 'disabled', 'disable']); +const ECC_ENABLE_VALUES = new Set(['1', 'true', 'on', 'enabled', 'enable', 'yes']); // SQL-keyword + dd patterns stay as a single regex — they are stable // phrases without shell-flag ordering concerns. Quoted strings are @@ -53,6 +54,54 @@ const ECC_DISABLE_VALUES = new Set(['0', 'false', 'off', 'disabled', 'disable']) // "drop table" no longer triggers a false positive. const DESTRUCTIVE_SQL_DD = /\b(drop\s+table|delete\s+from|truncate|dd\s+if=)\b/i; +// Operator-supplied additional destructive patterns. Lazily compiled from +// `GATEGUARD_BASH_EXTRA_DESTRUCTIVE` (regex source) on first use, then +// memoized keyed by the env-var value so a test or long-running process +// that flips the env between calls re-reads it without paying for a +// recompile on every invocation. A malformed regex is treated as +// "not configured" (the gate falls back to the built-in patterns) and +// the parse failure is logged once via `[gateguard-fact-force]` to +// stderr — hooks must never crash tool execution because of operator +// config errors. +let extraDestructiveCacheKey = null; +let extraDestructiveCacheRegex = null; +let extraDestructiveWarnLogged = false; +function getExtraDestructiveRegex() { + const raw = process.env.GATEGUARD_BASH_EXTRA_DESTRUCTIVE || ''; + if (!raw) { + extraDestructiveCacheKey = ''; + extraDestructiveCacheRegex = null; + return null; + } + if (raw === extraDestructiveCacheKey) { + return extraDestructiveCacheRegex; + } + // The env value just changed; reset the once-per-pattern warning gate + // so a subsequent *different* invalid regex is also reported once. The + // previous shape kept the flag sticky and silently swallowed the + // second bad pattern in a long-running process. + extraDestructiveCacheKey = raw; + extraDestructiveWarnLogged = false; + try { + extraDestructiveCacheRegex = new RegExp(raw, 'i'); + } catch (err) { + extraDestructiveCacheRegex = null; + if (!extraDestructiveWarnLogged) { + try { + process.stderr.write( + `[gateguard-fact-force] ignoring invalid GATEGUARD_BASH_EXTRA_DESTRUCTIVE regex: ${err.message}\n` + ); + } catch (_) { /* stderr write failure is non-fatal */ } + extraDestructiveWarnLogged = true; + } + } + return extraDestructiveCacheRegex; +} + +function isRoutineBashGateDisabled() { + return ECC_ENABLE_VALUES.has(normalizeEnvValue(process.env.GATEGUARD_BASH_ROUTINE_DISABLED)); +} + /** * Strip the contents of single- and double-quoted strings so phrases * mentioned inside a commit message or echoed argument do not trigger @@ -414,9 +463,17 @@ function isDestructiveBash(command) { const flattened = explodeSubshells(stripQuotedStrings(raw)); if (DESTRUCTIVE_SQL_DD.test(flattened)) return true; + // Operator-supplied additional destructive patterns. Same scope as the + // built-in SQL/dd regex: matched against the quote-stripped, subshell- + // exploded command so a phrase inside `$(...)` or backticks is caught. + const extra = getExtraDestructiveRegex(); + if (extra && extra.test(flattened)) return true; + const segments = collectExecutableBodies(raw).flatMap(splitCommandSegments); for (const segment of segments) { - if (DESTRUCTIVE_SQL_DD.test(stripQuotedStrings(segment))) return true; + const stripped = stripQuotedStrings(segment); + if (DESTRUCTIVE_SQL_DD.test(stripped)) return true; + if (extra && extra.test(stripped)) return true; const tokens = tokenize(segment); if (isDestructiveRm(tokens)) return true; if (isDestructiveGit(tokens)) return true; @@ -883,6 +940,14 @@ function run(rawInput) { return rawInput; // allow retry after facts presented } + // Operator opt-out: skip the routine-bash gate entirely. The destructive + // gate above still fires. This is the documented escape hatch for hosts + // (Cursor, OpenCode, etc.) where the once-per-session routine gate is + // friction without signal. + if (isRoutineBashGateDisabled()) { + return rawInput; // routine gate opted out via env + } + if (!isChecked(ROUTINE_BASH_SESSION_KEY)) { if (!markChecked(ROUTINE_BASH_SESSION_KEY)) { return allowWithStateWarning(); diff --git a/tests/hooks/gateguard-fact-force.test.js b/tests/hooks/gateguard-fact-force.test.js index 127b95ea..10c790c5 100644 --- a/tests/hooks/gateguard-fact-force.test.js +++ b/tests/hooks/gateguard-fact-force.test.js @@ -1406,6 +1406,218 @@ function runTests() { 'foo{ token inside brace body'); })) passed++; else failed++; + // --- Issue #2078: GATEGUARD_BASH_ROUTINE_DISABLED env var --- + // Operators on hosts that don't benefit from the once-per-session + // routine bash gate (Cursor, OpenCode, etc.) get an env-var opt-out. + // The destructive gate is unaffected. + + clearState(); + if (test('GATEGUARD_BASH_ROUTINE_DISABLED=1 skips routine bash gate', () => { + const input = { tool_name: 'Bash', tool_input: { command: 'ls -la' } }; + const result = runBashHook(input, { GATEGUARD_BASH_ROUTINE_DISABLED: '1' }); + assert.strictEqual(result.code, 0, 'exit code should be 0'); + const output = parseOutput(result.stdout); + assert.ok(output, 'should produce valid JSON output'); + if (output.hookSpecificOutput) { + assert.notStrictEqual(output.hookSpecificOutput.permissionDecision, 'deny', + 'routine bash should not be denied when env opts out'); + } else { + assert.strictEqual(output.tool_name, 'Bash', 'pass-through should preserve input'); + } + })) passed++; else failed++; + + clearState(); + if (test('GATEGUARD_BASH_ROUTINE_DISABLED accepts truthy aliases (true, on, yes, enabled)', () => { + for (const value of ['true', 'on', 'yes', 'enabled', 'TRUE', 'On']) { + clearState(); + const result = runBashHook( + { tool_name: 'Bash', tool_input: { command: 'grep foo bar.txt' } }, + { GATEGUARD_BASH_ROUTINE_DISABLED: value } + ); + const output = parseOutput(result.stdout); + assert.ok(output, `value=${value}: should produce JSON`); + if (output.hookSpecificOutput) { + assert.notStrictEqual(output.hookSpecificOutput.permissionDecision, 'deny', + `value=${value}: should not deny routine bash`); + } + } + })) passed++; else failed++; + + clearState(); + if (test('GATEGUARD_BASH_ROUTINE_DISABLED unset preserves baseline (denies first routine bash)', () => { + const input = { tool_name: 'Bash', tool_input: { command: 'ls -la' } }; + const result = runBashHook(input); + assert.strictEqual(result.code, 0, 'exit code should be 0'); + const output = parseOutput(result.stdout); + assert.ok(output, 'should produce JSON'); + assert.strictEqual(output.hookSpecificOutput.permissionDecision, 'deny', + 'baseline routine gate must still fire when env is unset'); + })) passed++; else failed++; + + clearState(); + if (test('GATEGUARD_BASH_ROUTINE_DISABLED=0 / off / false keeps current behavior', () => { + for (const value of ['0', 'false', 'off', '', 'random-value']) { + clearState(); + const result = runBashHook( + { tool_name: 'Bash', tool_input: { command: 'ls -la' } }, + { GATEGUARD_BASH_ROUTINE_DISABLED: value } + ); + const output = parseOutput(result.stdout); + assert.ok(output, `value="${value}": should produce JSON`); + assert.strictEqual(output.hookSpecificOutput.permissionDecision, 'deny', + `value="${value}": routine gate should still fire`); + } + })) passed++; else failed++; + + clearState(); + if (test('GATEGUARD_BASH_ROUTINE_DISABLED=1 does NOT disable destructive bash gate', () => { + const input = { tool_name: 'Bash', tool_input: { command: 'rm -rf /important/data' } }; + const result = runBashHook(input, { GATEGUARD_BASH_ROUTINE_DISABLED: '1' }); + const output = parseOutput(result.stdout); + assert.ok(output, 'should produce JSON'); + assert.strictEqual(output.hookSpecificOutput.permissionDecision, 'deny', + 'destructive gate must still fire even when routine gate is opted out'); + assert.ok(output.hookSpecificOutput.permissionDecisionReason.includes('Destructive'), + 'reason should mention Destructive'); + })) passed++; else failed++; + + // --- Issue #2078: GATEGUARD_BASH_EXTRA_DESTRUCTIVE env var --- + // Operators can register additional destructive patterns without + // patching the bundled JS. Same matching scope as the built-in + // SQL/dd regex (matches against quote-stripped, subshell-flattened + // command) so a custom phrase inside `$(...)` is also caught. + + clearState(); + if (test('GATEGUARD_BASH_EXTRA_DESTRUCTIVE custom phrase fires destructive gate', () => { + const input = { tool_name: 'Bash', tool_input: { command: 'supabase db reset --linked' } }; + const result = runBashHook(input, { + GATEGUARD_BASH_EXTRA_DESTRUCTIVE: 'supabase\\s+db\\s+reset|prisma\\s+migrate\\s+reset' + }); + const output = parseOutput(result.stdout); + assert.ok(output, 'should produce JSON'); + assert.strictEqual(output.hookSpecificOutput.permissionDecision, 'deny', + 'custom destructive phrase should be gated'); + assert.ok(output.hookSpecificOutput.permissionDecisionReason.includes('Destructive'), + 'reason should mention Destructive'); + })) passed++; else failed++; + + clearState(); + if (test('GATEGUARD_BASH_EXTRA_DESTRUCTIVE second member of alternation also fires', () => { + const input = { tool_name: 'Bash', tool_input: { command: 'prisma migrate reset --force' } }; + const result = runBashHook(input, { + GATEGUARD_BASH_EXTRA_DESTRUCTIVE: 'supabase\\s+db\\s+reset|prisma\\s+migrate\\s+reset' + }); + const output = parseOutput(result.stdout); + assert.ok(output, 'should produce JSON'); + assert.strictEqual(output.hookSpecificOutput.permissionDecision, 'deny', + 'second alternation member should be gated'); + })) passed++; else failed++; + + clearState(); + if (test('GATEGUARD_BASH_EXTRA_DESTRUCTIVE invalid regex degrades to baseline (no crash)', () => { + // Unbalanced paren is a regex parse error. Hook must NOT crash; it + // should fall back to the built-in patterns. A plain `ls` should + // therefore hit the routine gate (denied first time) and a + // built-in destructive (`rm -rf`) should still fire the destructive gate. + const lsResult = runBashHook( + { tool_name: 'Bash', tool_input: { command: 'ls -la' } }, + { GATEGUARD_BASH_EXTRA_DESTRUCTIVE: '(unclosed' } + ); + assert.strictEqual(lsResult.code, 0, 'malformed regex must not crash hook'); + const lsOutput = parseOutput(lsResult.stdout); + assert.ok(lsOutput, 'should produce JSON despite bad env regex'); + // Note: with invalid extra regex, the bash branch behaves as if the + // env var was unset — routine gate fires on first `ls`, destructive + // gate fires on `rm -rf`. + assert.strictEqual(lsOutput.hookSpecificOutput.permissionDecision, 'deny', + 'baseline routine gate should still fire when extra-regex is malformed'); + })) passed++; else failed++; + + clearState(); + if (test('GATEGUARD_BASH_EXTRA_DESTRUCTIVE unset does not affect baseline', () => { + const input = { tool_name: 'Bash', tool_input: { command: 'supabase db reset --linked' } }; + const result = runBashHook(input); + assert.strictEqual(result.code, 0, 'exit code should be 0'); + const output = parseOutput(result.stdout); + assert.ok(output, 'should produce JSON'); + // Without the extra regex, `supabase db reset` is a routine bash + // command and should hit the routine gate (deny first time) — the + // destructive gate's "rollback" guidance must NOT appear, since this + // is the routine, not destructive, deny path. + assert.strictEqual(output.hookSpecificOutput.permissionDecision, 'deny', + 'routine gate fires when extra-regex is unset'); + assert.ok(!output.hookSpecificOutput.permissionDecisionReason.includes('rollback'), + 'should be routine deny (no "rollback" guidance), not destructive'); + assert.ok(!output.hookSpecificOutput.permissionDecisionReason.includes('Destructive'), + 'should not be the destructive deny message'); + })) passed++; else failed++; + + clearState(); + if (test('GATEGUARD_BASH_EXTRA_DESTRUCTIVE custom phrase inside $(...) also caught', () => { + const input = { + tool_name: 'Bash', + tool_input: { command: 'echo "running" && $(supabase db reset)' } + }; + const result = runBashHook(input, { + GATEGUARD_BASH_EXTRA_DESTRUCTIVE: 'supabase\\s+db\\s+reset' + }); + const output = parseOutput(result.stdout); + assert.ok(output, 'should produce JSON'); + assert.strictEqual(output.hookSpecificOutput.permissionDecision, 'deny', + 'custom phrase inside command substitution should be gated'); + })) passed++; else failed++; + + // --- Issue #2078 review fix: warning emitted once per *distinct* + // invalid regex, not once per process. Verifies the same-process + // path that the reviewers (CodeRabbit + cubic) flagged. + clearState(); + if (test('GATEGUARD_BASH_EXTRA_DESTRUCTIVE warns once per distinct invalid regex (not once per process)', () => { + // We can't easily intercept stderr from a spawnSync child without + // re-running the hook in the same process, so we exercise + // checkCommand-equivalent behavior via a same-process require. + const originalEnv = process.env.GATEGUARD_BASH_EXTRA_DESTRUCTIVE; + const originalStderrWrite = process.stderr.write.bind(process.stderr); + const captured = []; + process.stderr.write = (chunk) => { + const s = typeof chunk === 'string' ? chunk : chunk.toString(); + if (s.includes('GATEGUARD_BASH_EXTRA_DESTRUCTIVE')) { + captured.push(s.trim()); + } + // Don't forward to real stderr — keeps test output clean. + return true; + }; + try { + // First bad pattern — should warn once. + process.env.GATEGUARD_BASH_EXTRA_DESTRUCTIVE = '(unclosed-a'; + const hook1 = loadDirectHook({ GATEGUARD_BASH_EXTRA_DESTRUCTIVE: '(unclosed-a' }); + hook1.run(JSON.stringify({ tool_name: 'Bash', tool_input: { command: 'ls' } })); + hook1.run(JSON.stringify({ tool_name: 'Bash', tool_input: { command: 'ls' } })); + assert.strictEqual(captured.length, 1, + `same invalid pattern should warn exactly once, got ${captured.length}: ${JSON.stringify(captured)}`); + + // Switch to a *different* bad pattern — should warn again (this is + // the bug both reviewers flagged: the sticky flag was never reset + // when the cache key changed). + process.env.GATEGUARD_BASH_EXTRA_DESTRUCTIVE = '(unclosed-b'; + hook1.run(JSON.stringify({ tool_name: 'Bash', tool_input: { command: 'ls' } })); + assert.strictEqual(captured.length, 2, + `distinct invalid pattern should produce a second warning, got ${captured.length}: ${JSON.stringify(captured)}`); + + // Switch back to a valid regex — no extra warning. + process.env.GATEGUARD_BASH_EXTRA_DESTRUCTIVE = 'valid\\s+pattern'; + hook1.run(JSON.stringify({ tool_name: 'Bash', tool_input: { command: 'ls' } })); + assert.strictEqual(captured.length, 2, + `valid regex should not emit a warning, got ${captured.length}: ${JSON.stringify(captured)}`); + } finally { + process.stderr.write = originalStderrWrite; + if (originalEnv === undefined) { + delete process.env.GATEGUARD_BASH_EXTRA_DESTRUCTIVE; + } else { + process.env.GATEGUARD_BASH_EXTRA_DESTRUCTIVE = originalEnv; + } + } + })) passed++; else failed++; + // Cleanup only the temp directory created by this test file. try { if (fs.existsSync(stateDir)) {