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}`, () => {