From 76786616f86a40a8b9ab1d9e00a34d36f91b0213 Mon Sep 17 00:00:00 2001 From: Nguyen Thanh Dat Date: Fri, 21 Aug 2026 08:10:33 +0700 Subject: [PATCH 1/5] fix(gateguard): deny dd whose input path is not word-initial DESTRUCTIVE_SQL_DD shared one trailing \b across every alternation arm. `dd\s+if=` ends in `=`, and a \b after a non-word character only holds when the NEXT character is a word character, so the arm matched `dd if=x` and missed every path starting with `/`, `.` or a quote: dd if=/dev/zero of=/dev/sda allowed dd if=./disk.img of=/dev/sdb allowed dd if="/dev/zero" of=/dev/sda allowed The word-boundary suffix now applies only to the arms that end in a word character. The split is what keeps the widening bounded: dropping the trailing \b outright would let `truncate` match `truncated`, and dropping the leading \b would let `dd if=` match inside `add if=`. Both are covered. This is the half of #2642 that survived the structural findGitSubcommand() parser, which already handles `git checkout -- .`. Not addressed here: `dd of=/dev/sda if=/dev/zero` with the operands reversed is still allowed, before and after, because the pattern requires `if=` immediately after `dd`. That is a different defect from the boundary bug and widening a P0 gate's pattern shape is a maintainer call. Refs #2642 --- scripts/hooks/gateguard-fact-force.js | 6 ++- tests/hooks/gateguard-fact-force.test.js | 61 ++++++++++++++++++++++++ 2 files changed, 66 insertions(+), 1 deletion(-) diff --git a/scripts/hooks/gateguard-fact-force.js b/scripts/hooks/gateguard-fact-force.js index bf91eb78a..1228816b6 100644 --- a/scripts/hooks/gateguard-fact-force.js +++ b/scripts/hooks/gateguard-fact-force.js @@ -53,7 +53,11 @@ const ECC_ENABLE_VALUES = new Set(['1', 'true', 'on', 'enabled', 'enable', 'yes' // phrases without shell-flag ordering concerns. Quoted strings are // stripped before this regex runs so a commit message mentioning // "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; +// The trailing \b applies only to the arms that end in a word character. +// `dd\s+if=` ends in `=`, so a shared \b demanded that the NEXT character be a +// word character and the disk-wipe spellings slipped through: `dd if=/dev/zero` +// and `dd if=./img` were allowed while `dd if=x` was denied (#2642). +const DESTRUCTIVE_SQL_DD = /\b(?:drop\s+table|delete\s+from|truncate)\b|\bdd\s+if=/i; // Operator-supplied additional destructive patterns. Lazily compiled from // `GATEGUARD_BASH_EXTRA_DESTRUCTIVE` (regex source) on first use, then diff --git a/tests/hooks/gateguard-fact-force.test.js b/tests/hooks/gateguard-fact-force.test.js index 54a19c0e0..f12fedda5 100644 --- a/tests/hooks/gateguard-fact-force.test.js +++ b/tests/hooks/gateguard-fact-force.test.js @@ -255,6 +255,67 @@ function runTests() { passed++; else failed++; + // --- Test 4b: dd targets that do not start with a word character --- + /** + * #2642: DESTRUCTIVE_SQL_DD carried one trailing \b across every alternation + * arm. `dd\s+if=` ends in `=`, so that \b demanded the NEXT character be a + * word character: `dd if=x` was denied while the disk-wipe spelling + * `dd if=/dev/zero of=/dev/sda` and the relative `dd if=./img` were allowed. + * These run through the real hook, since the report is specifically that the + * published hook lets the slash-prefixed form through. + */ + for (const command of [ + 'dd if=/dev/zero of=/dev/sda', + 'dd if=./disk.img of=/dev/sdb', + 'dd if="/dev/zero" of=/dev/sda' + ]) { + clearState(); + if ( + test(`denies dd whose input path is not word-initial: ${command}`, () => { + const result = runBashHook({ tool_name: 'Bash', tool_input: { command } }); + const output = parseOutput(result.stdout); + assert.ok(output, 'hook should produce JSON output'); + assert.ok(output.hookSpecificOutput, 'hook should return a permission decision'); + assert.strictEqual( + output.hookSpecificOutput.permissionDecision, + 'deny', + `${command} must be gated as destructive` + ); + assert.ok(output.hookSpecificOutput.permissionDecisionReason.includes('Destructive')); + }) + ) + passed++; + else failed++; + } + + // --- Test 4c: widening the dd arm must not gate ordinary commands --- + /** + * The fix drops the trailing \b only from the dd arm, so the arms that end in + * a word character keep theirs. Without that split, `truncated` would match + * `truncate`, and `add if=` would match `dd if=`. + */ + for (const command of ['echo add if=1', 'echo truncated output', 'git status']) { + clearState(); + if ( + test(`does not gate as destructive: ${command}`, () => { + // Prime the session so the separate first-command routine gate cannot + // be mistaken for a destructive denial. + runBashHook({ tool_name: 'Bash', tool_input: { command: 'printf ready' } }); + const result = runBashHook({ tool_name: 'Bash', tool_input: { command } }); + const output = parseOutput(result.stdout); + if (output && output.hookSpecificOutput) { + const reason = output.hookSpecificOutput.permissionDecisionReason || ''; + assert.ok( + output.hookSpecificOutput.permissionDecision !== 'deny' || !reason.includes('Destructive'), + `${command} must not be gated as destructive` + ); + } + }) + ) + passed++; + else failed++; + } + // --- Test 5: denies first routine Bash, allows second --- clearState(); if ( From 79d3fefad13e80184064bccb3a690990c8987fe9 Mon Sep 17 00:00:00 2001 From: Nguyen Thanh Dat Date: Fri, 21 Aug 2026 08:38:10 +0700 Subject: [PATCH 2/5] fix(gateguard): detect dd by command word, not by text match MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review on #2829 found the regex fix widened a false positive: matching `\bdd\s+if=` against the whole flattened line gated `echo dd if=/dev/zero` and `grep dd if=/dev/zero file`, neither of which runs dd. That class was already present before — `echo dd if=x` matched the old arm too — but the boundary fix extended it to the slash and dot spellings, so the arm now decides on text position rather than on what is being executed. dd moves to isDestructiveDd(tokens), next to isDestructiveRm and isDestructiveGit, and DESTRUCTIVE_SQL_DD goes back to SQL only. The per-segment loop already tokenizes every executable body, so the check runs where the command word is known. This resolves four things the text match could not: dd if=/dev/zero of=/dev/sda was allowed -> denied (the reported bug) sudo dd if=/dev/zero was allowed -> denied dd of=/dev/sda if=/dev/zero was allowed -> denied (operands are order-free) echo dd if=x was denied -> allowed (pre-existing false positive) Leading sudo/doas/env, their flags, and VAR=value assignment prefixes are skipped so a wrapped invocation still resolves to dd; flags are only skipped once a wrapper has been seen, so the scan cannot walk into an unrelated command's arguments. Tests: 6 fail on upstream main, 155 pass with this change. Refs #2642 --- scripts/hooks/gateguard-fact-force.js | 65 +++++++++++++++++++++--- tests/hooks/gateguard-fact-force.test.js | 17 ++++++- 2 files changed, 73 insertions(+), 9 deletions(-) diff --git a/scripts/hooks/gateguard-fact-force.js b/scripts/hooks/gateguard-fact-force.js index 1228816b6..1eb9c9027 100644 --- a/scripts/hooks/gateguard-fact-force.js +++ b/scripts/hooks/gateguard-fact-force.js @@ -53,11 +53,13 @@ const ECC_ENABLE_VALUES = new Set(['1', 'true', 'on', 'enabled', 'enable', 'yes' // phrases without shell-flag ordering concerns. Quoted strings are // stripped before this regex runs so a commit message mentioning // "drop table" no longer triggers a false positive. -// The trailing \b applies only to the arms that end in a word character. -// `dd\s+if=` ends in `=`, so a shared \b demanded that the NEXT character be a -// word character and the disk-wipe spellings slipped through: `dd if=/dev/zero` -// and `dd if=./img` were allowed while `dd if=x` was denied (#2642). -const DESTRUCTIVE_SQL_DD = /\b(?:drop\s+table|delete\s+from|truncate)\b|\bdd\s+if=/i; +// `dd if=` used to be a fourth arm here. Matching it as text could not work: +// the arm ended in `=`, so the shared trailing \b required the NEXT character +// to be a word character and `dd if=/dev/zero` slipped through while +// `echo dd if=x` — which runs no dd at all — was gated. The boundary decided +// the verdict instead of the command position, so dd moved to isDestructiveDd() +// alongside the other token-based detectors (#2642). +const DESTRUCTIVE_SQL = /\b(drop\s+table|delete\s+from|truncate)\b/i; // Operator-supplied additional destructive patterns. Lazily compiled from // `GATEGUARD_BASH_EXTRA_DESTRUCTIVE` (regex source) on first use, then @@ -357,6 +359,7 @@ function isDestructiveQuoteAware(raw, depth = 0) { if (tokens.length === 0) continue; if (isDestructiveRm(tokens)) return true; if (isDestructiveGit(tokens)) return true; + if (isDestructiveDd(tokens)) return true; if (isDestructiveFindExec(tokens.join(' '))) return true; const base = commandBasename(tokens[0]); if (SHELL_WRAPPERS.has(base)) { @@ -384,6 +387,52 @@ function commandBasename(token) { .toLowerCase(); } +/** + * Detect a `dd` invocation carrying an `if=` operand. + * + * Token-based rather than a regex arm because the verdict has to depend on + * `dd` being the command, not on `dd if=` appearing anywhere in the line: + * `echo dd if=/dev/zero` executes nothing. dd operands are order-free, so + * `dd of=/dev/sda if=/dev/zero` counts too — a text pattern anchored on + * `dd\s+if=` missed that spelling entirely. + * + * Leading `sudo` / `doas` / `env`, their flags, and `VAR=value` assignment + * prefixes are skipped so `sudo dd if=/dev/zero` stays the dd invocation it is. + * + * @param {string[]} tokens + * @returns {boolean} + */ +function isDestructiveDd(tokens) { + let index = 0; + let sawWrapper = false; + while (index < tokens.length) { + const token = tokens[index]; + const name = commandBasename(token); + if (name === 'sudo' || name === 'doas' || name === 'env') { + sawWrapper = true; + index += 1; + continue; + } + // `FOO=bar dd if=…` and `env FOO=bar dd if=…` both put assignments before + // the command word. `dd`'s own operands are never reached here: the loop + // stops at the first token that is neither a wrapper nor an assignment. + if (/^[A-Za-z_][A-Za-z0-9_]*=/.test(token)) { + index += 1; + continue; + } + // Only skip flags once a wrapper has been seen, so this cannot walk past + // an unrelated command's arguments. + if (sawWrapper && token.startsWith('-')) { + index += 1; + continue; + } + break; + } + + if (index >= tokens.length || commandBasename(tokens[index]) !== 'dd') return false; + return tokens.slice(index + 1).some(operand => /^if=/i.test(operand)); +} + /** * Detect `rm` invocations that recursively force-delete files. Handles * combined (`-rf`, `-fr`, `-Rf`) and split (`-r -f`) flag forms. @@ -681,9 +730,11 @@ function isDestructiveBash(command) { // after quoting AND subshell delimiters are normalized so phrases // inside `$(...)` or backticks are also caught. const raw = String(command || ''); + // Keep main's heredoc stripping: a phrase inside a heredoc body is data, not a + // command. dd is no longer part of this regex — see DESTRUCTIVE_SQL. const executable = stripHeredocBodies(raw); const flattened = explodeSubshells(stripQuotedStrings(executable)); - if (DESTRUCTIVE_SQL_DD.test(flattened)) return true; + if (DESTRUCTIVE_SQL.test(flattened)) return true; // Operator-supplied additional destructive patterns. Same scope as the // built-in SQL/dd regex: matched against the quote-stripped, subshell- @@ -710,7 +761,7 @@ function isDestructiveBash(command) { const segments = bodies.flatMap(splitCommandSegments); for (const segment of segments) { const stripped = stripQuotedStrings(segment); - if (DESTRUCTIVE_SQL_DD.test(stripped)) return true; + if (DESTRUCTIVE_SQL.test(stripped)) return true; if (extra && extra.test(stripped)) return true; const tokens = tokenize(segment); if (isDestructiveRm(tokens)) return true; diff --git a/tests/hooks/gateguard-fact-force.test.js b/tests/hooks/gateguard-fact-force.test.js index f12fedda5..fcf6f400b 100644 --- a/tests/hooks/gateguard-fact-force.test.js +++ b/tests/hooks/gateguard-fact-force.test.js @@ -267,7 +267,11 @@ function runTests() { for (const command of [ 'dd if=/dev/zero of=/dev/sda', 'dd if=./disk.img of=/dev/sdb', - 'dd if="/dev/zero" of=/dev/sda' + 'dd if="/dev/zero" of=/dev/sda', + // Wrapped invocations must still resolve to the dd command word. + 'sudo dd if=/dev/zero of=/dev/sda', + // dd operands are order-free; a text pattern anchored on `dd if=` missed this. + 'dd of=/dev/sda if=/dev/zero' ]) { clearState(); if ( @@ -294,7 +298,16 @@ function runTests() { * a word character keep theirs. Without that split, `truncated` would match * `truncate`, and `add if=` would match `dd if=`. */ - for (const command of ['echo add if=1', 'echo truncated output', 'git status']) { + for (const command of [ + 'echo add if=1', + 'echo truncated output', + 'git status', + // `dd if=` as another command's argument runs no dd at all. The old text + // match gated these; the command-word check is what keeps them out. + 'echo dd if=/dev/zero', + 'grep dd if=/dev/zero file', + 'echo dd if=x' + ]) { clearState(); if ( test(`does not gate as destructive: ${command}`, () => { From 249b0ecf7fddbf4ca16147fd4eb02e5075914cc1 Mon Sep 17 00:00:00 2001 From: Nguyen Thanh Dat Date: Fri, 21 Aug 2026 08:39:59 +0700 Subject: [PATCH 3/5] test(gateguard): fail the allow-cases when the hook returns nothing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The "does not gate as destructive" cases guarded the decision behind `if (output && output.hookSpecificOutput)`, so a crashed or silent hook made parseOutput return null and the test passed having asserted nothing. Assert the exit code and that output parsed first, then branch: a decision object must not be a Destructive deny, and pass-through must echo the input back — the same shape the existing retry case already checks. Raised in review on #2829. --- tests/hooks/gateguard-fact-force.test.js | 15 ++++++++++++--- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/tests/hooks/gateguard-fact-force.test.js b/tests/hooks/gateguard-fact-force.test.js index fcf6f400b..f83677b0d 100644 --- a/tests/hooks/gateguard-fact-force.test.js +++ b/tests/hooks/gateguard-fact-force.test.js @@ -315,13 +315,22 @@ function runTests() { // be mistaken for a destructive denial. runBashHook({ tool_name: 'Bash', tool_input: { command: 'printf ready' } }); const result = runBashHook({ tool_name: 'Bash', tool_input: { command } }); + // Assert the hook actually answered before reading the decision: a + // crashed or silent hook makes parseOutput return null, and a bare + // `if (output)` would let this case pass without testing anything. + assert.strictEqual(result.code, 0, `hook should exit 0 for ${command}`); const output = parseOutput(result.stdout); - if (output && output.hookSpecificOutput) { - const reason = output.hookSpecificOutput.permissionDecisionReason || ''; + assert.ok(output, `hook should produce JSON output for ${command}`); + const decision = output.hookSpecificOutput; + if (decision) { + const reason = decision.permissionDecisionReason || ''; assert.ok( - output.hookSpecificOutput.permissionDecision !== 'deny' || !reason.includes('Destructive'), + decision.permissionDecision !== 'deny' || !reason.includes('Destructive'), `${command} must not be gated as destructive` ); + } else { + // Pass-through echoes the input back unchanged. + assert.strictEqual(output.tool_name, 'Bash', 'pass-through should preserve input'); } }) ) From c88b3f879ab0e8ff3c2bd41afd250dafbcbdb4ca Mon Sep 17 00:00:00 2001 From: Nguyen Thanh Dat Date: Fri, 21 Aug 2026 08:41:11 +0700 Subject: [PATCH 4/5] test(gateguard): cover dd with an intervening option before if= Named in review on #2829: `dd bs=1M if=/dev/zero of=/dev/sda` is the same class as the reversed-operand case and is denied by the token-based check, but nothing pinned it. --- tests/hooks/gateguard-fact-force.test.js | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/tests/hooks/gateguard-fact-force.test.js b/tests/hooks/gateguard-fact-force.test.js index f83677b0d..323981096 100644 --- a/tests/hooks/gateguard-fact-force.test.js +++ b/tests/hooks/gateguard-fact-force.test.js @@ -270,8 +270,10 @@ function runTests() { 'dd if="/dev/zero" of=/dev/sda', // Wrapped invocations must still resolve to the dd command word. 'sudo dd if=/dev/zero of=/dev/sda', - // dd operands are order-free; a text pattern anchored on `dd if=` missed this. - 'dd of=/dev/sda if=/dev/zero' + // dd operands are order-free; a text pattern anchored on `dd if=` missed + // both the reversed and the intervening-option spellings. + 'dd of=/dev/sda if=/dev/zero', + 'dd bs=1M if=/dev/zero of=/dev/sda' ]) { clearState(); if ( From d8113492245ea46329f5e23c5c8e826c6ea4c25c Mon Sep 17 00:00:00 2001 From: affaan-m <124439313+affaan-m@users.noreply.github.com> Date: Sun, 27 Sep 2026 22:25:29 -0400 Subject: [PATCH 5/5] fix(gateguard): detect stdin dd writes and launcher-wrapped shells --- scripts/hooks/gateguard-fact-force.js | 9 ++++---- tests/hooks/gateguard-fact-force.test.js | 27 +++++++++++++++++++++++- 2 files changed, 31 insertions(+), 5 deletions(-) diff --git a/scripts/hooks/gateguard-fact-force.js b/scripts/hooks/gateguard-fact-force.js index 8f0794bdf..049591d00 100644 --- a/scripts/hooks/gateguard-fact-force.js +++ b/scripts/hooks/gateguard-fact-force.js @@ -530,7 +530,7 @@ function wrapperValueOption(arg, valueFlags) { return null; } -// Explicit external-launcher argv grammars used only by the dd classifier. +// Explicit external-launcher argv grammars for dd and shell-wrapper discovery. // Unknown flags do not justify guessing which later argument executes. const DD_LAUNCHER_OPTIONS = { xargs: { @@ -735,7 +735,7 @@ function isDestructiveQuoteAware(raw, depth = 0) { if (isDestructiveDd(tokens)) return true; if (isDestructiveSqlClient(tokens)) return true; if (isDestructiveFindExec(tokens)) return true; - const argv = unwrapLeadWrappers(tokens); + const argv = unwrapLeadWrappers(tokens, true, true); if (SHELL_WRAPPERS.has(commandBasename(argv[0]))) { const ci = argv.indexOf('-c', 1); if (ci !== -1 && argv[ci + 1] && isDestructiveQuoteAware(argv[ci + 1], depth + 1)) { @@ -763,7 +763,8 @@ function commandBasename(token) { } /** - * Detect a `dd` invocation carrying an `if=` operand. + * Detect a `dd` invocation carrying an `if=` or `of=` operand. + * Keep the existing input-file gate and include output-only writes from stdin. * * Token-based rather than a regex arm because the verdict has to depend on * `dd` being the command, not on `dd if=` appearing anywhere in the line: @@ -779,7 +780,7 @@ function commandBasename(token) { */ function isDestructiveDd(tokens, allowShellBuiltins = true) { const argv = unwrapLeadWrappers(tokens, allowShellBuiltins, true); - return commandBasename(argv[0]) === 'dd' && argv.slice(1).some(operand => /^if=/i.test(operand)); + return commandBasename(argv[0]) === 'dd' && argv.slice(1).some(operand => /^(?:if|of)=/i.test(operand)); } /** diff --git a/tests/hooks/gateguard-fact-force.test.js b/tests/hooks/gateguard-fact-force.test.js index 9d85cd61f..426e4efbe 100644 --- a/tests/hooks/gateguard-fact-force.test.js +++ b/tests/hooks/gateguard-fact-force.test.js @@ -185,6 +185,18 @@ function runDdRegressionTests() { }; const destructive = [ 'dd if=/dev/zero of=/dev/sda', + 'dd of=/dev/sda bs=1M', + 'cat /dev/zero | dd of=/dev/sda', + 'sudo dd of=/dev/sda < /dev/zero', + 'dd bs=1M of="./output"', + "timeout 60 bash -c 'dd if=/dev/zero of=/dev/sda'", + "nohup sh -c 'dd if=input'", + "nice -n 5 sh -c 'dd if=input'", + "xargs sh -c 'dd if=input'", + "timeout 2 sh -c 'dd of=output'", + "nohup sh -c 'echo $(dd of=output)'", + "nice -n 5 sh -c 'cat < { fs.rmSync(stateDir, { recursive: true, force: true }); @@ -617,6 +641,7 @@ function runTests() { if ( test(`denies dd whose input path is not word-initial: ${command}`, () => { const result = runBashHook({ tool_name: 'Bash', tool_input: { command } }); + assert.strictEqual(result.code, 0, `hook should exit successfully for ${command}`); const output = parseOutput(result.stdout); assert.ok(output, 'hook should produce JSON output'); assert.ok(output.hookSpecificOutput, 'hook should return a permission decision');