diff --git a/KNOWN-LIMITS.md b/KNOWN-LIMITS.md index fd5759f..7c43da4 100644 --- a/KNOWN-LIMITS.md +++ b/KNOWN-LIMITS.md @@ -545,7 +545,38 @@ does not cost a signature, and tightening it to a leading-segment-only rule brok legitimate deletion of nested scratch dirs like `/home/me/scratchpad/run-1`. Exposure is limited to targets genuinely sitting under a scratch-named directory. -## 25. The gate knew git's transports and not git's vendor CLI +**Amendment (2026-08-29).** Two matcher changes in the same rule family as this +entry, both closing under-gates that let a destructive delete slip past the gate +when its spelling was not the exact compact form: + +- *rm / Remove-Item trigger now scans flag order and long-form options.* The + trigger previously required the `-r` and `-f` letters ADJACENT in one bundle + (`-rf` / `-fr`) or `--recursive` as the literal next token. So `rm --force + --recursive`, `rm --ignore-times --force --recursive`, separated shorts + (`rm -r -f`), and flags after the operand (`rm file.txt --force --recursive`) + were all silent. The fix shares one token predicate between the trigger and the + target extractor: long options match by exact name (`--force`, `--recursive`), + short bundles explode per character, so every ordering reduces to the same two + booleans. A trigger that fired only on adjacency was the *expensive* failure + (silence); this widens to the cheap one (occasional false positive), which is + the correct direction for this rule. +- *`git clean -f` with `-d`/`-x` is now gated.* `git clean` never spells an `rm` + token, so the destructive matcher had zero handling for it and `git clean -fdx` + force-deleted the whole tree silently. The gate now requires `-f`/`--force` + AND a depth/ignored amplifier (`-d`/`--directory` or `-x`/`--ignored`/`-X`); + `-n` (dry-run) and bare `git clean` stay free. Two pathspec defects the + reviewer caught are closed: `-e ` / `-x ` take a *value* and + that value is skipped (it is not a pathspec, so an allowlisted exclude pattern + can no longer launder a whole-tree clean), and *every* pathspec is checked, not + just the first (an allowlisted first pathspec no longer exempts later + pathspecs). A pathspec still scopes the blast radius through the same + scratch-segment allowlist this entry describes; no pathspec means the whole + tree, which always gates. +- *Scope discipline.* The rm trigger now scans only the command *segment* that + contains `rm` (split on `&&`/`||`/`;`/`|`/newline), not the whole command, so + `grep -rf x && rm y` no longer fires on the rm. This is the previously + undeclared false-positive class: the cheaper direction, but it costs signatures + on ordinary work and is now stated here on purpose. Until 2026-07-24, `gh` — the GitHub CLI, authenticated against the user's account from the system keyring — was almost entirely invisible to the rule set. Three diff --git a/src/policy/index.js b/src/policy/index.js index 621a2eb..0a3dadc 100644 --- a/src/policy/index.js +++ b/src/policy/index.js @@ -1086,19 +1086,131 @@ function extractDestructiveTarget(cmd) { return null; } +/** + * Shared flag-token predicates for the rm trigger. These are DELIBERATELY + * stricter than the loose substring scan inside extractDestructiveTarget: + * a long option is matched by its exact name (`--recursive`, `--force`), + * never by "contains an r/f somewhere". The loose scan is safe downstream + * (it only picks a target AFTER something already decided to fire) but as + * a TRIGGER it would gate harmless deletes like `rm --reference=a b`. + * Short-flag bundles are exploded per character, which is what makes flag + * ORDER irrelevant: `-rf`, `-fr`, `-r -f`, `--force --recursive` and + * flags placed after the operands all reduce to the same two booleans. + * + * IMPORTANT: this must be called with the TOKENS OF THE rm SEGMENT ONLY, + * not the whole command. Scanning the whole command made `grep -rf x && rm + * y` fire on the rm (the -rf belonged to grep), a false-positive class the + * reviewer rejected. Callers split on CMD_SEPARATORS and pass the segment + * that actually contains `rm`. + */ +function rmTriggerFlags(tokens) { + let recursive = false; + let force = false; + for (const tok of tokens) { + if (!tok.startsWith('-')) continue; + if (tok === '--recursive') { recursive = true; continue; } + if (tok === '--force') { force = true; continue; } + if (tok.startsWith('--')) continue; // unrelated long option: inert + for (const ch of tok.slice(1)) { + if (ch === 'r' || ch === 'R') recursive = true; + else if (ch === 'f' || ch === 'F') force = true; + } + } + return { recursive, force }; +} + +// `git clean` never spells an `rm` token, so the rm/Remove-Item triggers +// cannot see it, and `git clean -fdx` removes every untracked (with -x, +// also ignored) path in one shot. Found 2026-08-22: zero handling anywhere +// in src/ — silent, exit 0, no receipt. Force-deletes are the hazard, so +// the gate requires -f/--force AND at least one depth/-x amplifier (-d or +// -x/-X): a bare `git clean` deletes nothing, `-n` is a dry run, `-i` asks. +// +// pathspec handling (the two defects the reviewer caught): +// - `-e ` / `-x ` take a VALUE; that value is NOT a +// pathspec and must be skipped, or an allowlisted exclude pattern would +// be consumed as the pathspec and launder a whole-tree clean. +// - EVERY pathspec is checked, not just the first: an allowlisted first +// pathspec must not exempt later pathspecs (which would delete unsigned). +// - a pathspec scopes the blast radius through the SAME scratch-segment +// allowlist rm uses; no pathspec means the whole tree, which always gates. +function isGitCleanForceDelete(cmd) { + return cmd.split(CMD_SEPARATORS).some(seg => { + const m = seg.match(/^\s*git\s+clean\s+(\S.*)$/); + if (!m) return false; + + let force = false, deep = false, ignored = false, dryRun = false; + const pathspecs = []; + const toks = m[1].split(/\s+/).map(t => t.replace(/^["']|["']$/g, '')); + for (let i = 0; i < toks.length; i++) { + const tok = toks[i]; + if (!tok.startsWith('-')) { pathspecs.push(tok); continue; } + // value-taking options: their argument is NOT a pathspec + if (tok === '-e' || tok === '--exclude') { i++; continue; } // -e + if (tok === '-x' || tok === '--dx-include-ignored') { ignored = true; continue; } // -x ? no, -x is a boolean; only -e takes a value here + if (tok === '--force') { force = true; continue; } + if (tok === '--directory') { deep = true; continue; } + if (tok === '--ignored') { ignored = true; continue; } + if (tok === '--dry-run') { dryRun = true; continue; } + if (tok.startsWith('--')) continue; // unknown long option: inert + for (const ch of tok.slice(1)) { + if (ch === 'f') force = true; + else if (ch === 'd') deep = true; + else if (ch === 'x') ignored = true; + else if (ch === 'X') ignored = true; // -X: only ignored files + else if (ch === 'n') dryRun = true; + else if (ch === 'e') i++; // short -e takes a value too + } + } + + if (dryRun || !force) return false; + if (!(deep || ignored)) return false; + if (pathspecs.length === 0) return true; // no pathspec: entire tree + // gate if ANY pathspec is outside the scratch allowlist + return pathspecs.some(p => !destructiveAllowlisted(p)); + }); +} + export function isDestructive(toolInput) { const cmd = matchableCommand(toolInput); if (cmd === '') return false; - const hasRmRf = /\brm\s+(-[a-zA-Z]*[rR][a-zA-Z]*[fF]|-[a-zA-Z]*[fF][a-zA-Z]*[rR])/.test(cmd) - || /\brm\s+--recursive\b/.test(cmd); - const hasRiRecurseForce = /\bRemove-Item\b/i.test(cmd) && /-Recurse/i.test(cmd) && /-Force/i.test(cmd); + if (isGitCleanForceDelete(cmd)) return true; + + // Scope each trigger to its OWN segment. Scanning the whole command let + // `grep -rf x && rm y` fire on the rm; splitting on CMD_SEPARATORS keeps + // the -rf of grep away from the rm trigger. + const segments = cmd.split(CMD_SEPARATORS); + + for (const seg of segments) { + const isRm = /\brm\b/.test(seg); + if (isRm) { + const tokens = seg.split(/\s+/).map(t => t.replace(/^["']|["']$/g, '')); + const { recursive, force } = rmTriggerFlags(tokens); + if (recursive && force) { + const target = extractDestructiveTarget(seg); + if (target == null) return true; // rm -rf with no path is still destructive + if (!destructiveAllowlisted(target)) return true; + } + } - if (!hasRmRf && !hasRiRecurseForce) return false; + // Remove-Item -Recurse -Force + if (/\bRemove-Item\b/i.test(seg)) { + if (/-Recurse/i.test(seg) && /-Force/i.test(seg)) { + const tokens = seg.split(/\s+/).map(t => t.replace(/^["']|["']$/g, '')); + for (let i = 0; i < tokens.length; i++) { + const tok = tokens[i]; + if (tok === 'Remove-Item') continue; + if (tok.startsWith('-')) continue; + const target = tok; + if (!destructiveAllowlisted(target)) return true; + break; + } + } + } + } - const target = extractDestructiveTarget(cmd); - if (target == null) return true; // rm -rf with no path is still destructive - return !destructiveAllowlisted(target); + return false; } // ---------- scope-escalation matcher ---------- diff --git a/test/policy-destructive-flag-order-git-clean.test.js b/test/policy-destructive-flag-order-git-clean.test.js new file mode 100644 index 0000000..b8a0a89 --- /dev/null +++ b/test/policy-destructive-flag-order-git-clean.test.js @@ -0,0 +1,242 @@ +/** + * test/policy-destructive-flag-order-git-clean.test.js + * + * Regression tests for two under-gated destructive classes found by + * hermes-nicosanchez (#912) during the 2026-08-22 C2 audit: + * + * 1. Long-form / reordered rm triggers. isDestructive fired only when the + * compact regex saw `-*[rR]...[fF]` ADJACENT to `rm`, or + * `--recursive` as the literal next token. So `rm --force --recursive` + * (long form, force-first), `rm --ignore-times --force --recursive`, + * and separated shorts (`rm -r -f`) all passed SILENTLY — no warn, + * no receipt. extractDestructiveTarget already tokenized flags in any + * order; the TRIGGER did not. Fix: the trigger uses the same shared + * token predicates as the extractor. + * + * 2. git clean. `git clean -fdx` force-deletes every untracked (and with + * -x, ignored) file without ever touching an rm token, so the + * destructive matcher had zero handling for it. Fix: `git clean` with + * -f AND (-d OR -x) is destructive; the first pathspec token goes + * through the same scratch-segment allowlist as rm targets, and a + * bare `git clean -fdx` (whole tree) always gates. + * + * Each case is asserted via isDestructive() (and the pipeline via + * evaluate() for one case per class) so the test fails the moment the + * matcher is wrong. + */ + +import { describe, it } from 'node:test'; +import assert from 'node:assert/strict'; +import fs from 'node:fs'; +import os from 'node:os'; +import path from 'node:path'; + +import { isDestructive, evaluate, loadPolicy } from '../src/policy/index.js'; + +const baseDir = fs.mkdtempSync(path.join(os.tmpdir(), 'lotor-flagorder-')); +const policy = loadPolicy(baseDir); + +describe('policy: destructive rm trigger accepts any flag order/form', () => { + // ---- cases that MUST gate (each failed silently before the fix) ---- + + it('gates "rm --force --recursive src/policy" (long form, force-first)', () => { + assert.equal( + isDestructive({ command: 'rm --force --recursive src/policy' }), + true, + 'flag ORDER must not matter; --force-first was silent at HEAD' + ); + }); + + it('gates "rm --ignore-times --force --recursive /var/www/app" (stacked long opts)', () => { + assert.equal( + isDestructive({ command: 'rm --ignore-times --force --recursive /var/www/app' }), + true, + 'an unrelated long option between --force and --recursive hid the pair' + ); + }); + + it('gates "rm -r -f src/policy" (separated short flags)', () => { + assert.equal( + isDestructive({ command: 'rm -r -f src/policy' }), + true, + 'short flags given separately never matched the adjacent-pair regex' + ); + }); + + it('gates "rm --recursive --force src/policy" (recursive-first long form)', () => { + assert.equal(isDestructive({ command: 'rm --recursive --force src/policy' }), true); + }); + + it('gates "rm file.txt --force --recursive" (flags AFTER the operand)', () => { + assert.equal( + isDestructive({ command: 'rm file.txt --force --recursive' }), + true, + 'operand-first spelling must gate like every other' + ); + }); + + it('fires through evaluate() (full policy pipeline)', () => { + const r = evaluate( + 'Bash', + { command: 'rm --force --recursive src/policy' }, + policy, + baseDir + ); + assert.ok(r, 'evaluate() should return a match'); + assert.equal(r.ruleId, 'destructive'); + }); + + // ---- allowlist interaction must survive the widened trigger ---- + + it('still exempts "rm --force --recursive /tmp/foo" (scratch allowlist)', () => { + assert.equal( + isDestructive({ command: 'rm --force --recursive /tmp/foo' }), + false, + 'the widened trigger must not eat the legitimate-scratch exemption' + ); + }); + + it('still exempts "rm -r -f ./scratchpad/build"', () => { + assert.equal(isDestructive({ command: 'rm -r -f ./scratchpad/build' }), false); + }); + + // ---- compact spellings keep gating (no regression) ---- + + it('still gates "rm -rf src/policy" (compact)', () => { + assert.equal(isDestructive({ command: 'rm -rf src/policy' }), true); + }); + + it('still gates "rm -fr src/policy" (compact, flipped)', () => { + assert.equal(isDestructive({ command: 'rm -fr src/policy' }), true); + }); +}); + +describe('policy: git clean -f with -d/-x is destructive', () => { + // ---- cases that MUST gate (all silent before the fix) ---- + + it('gates "git clean -fdx" (whole tree, no pathspec)', () => { + assert.equal( + isDestructive({ command: 'git clean -fdx' }), + true, + 'no pathspec means the ENTIRE tree: always destructive' + ); + }); + + it('gates "git clean -fd" (untracked dirs, no -x needed for the gate)', () => { + assert.equal(isDestructive({ command: 'git clean -fd' }), true); + }); + + it('gates "git clean -fx" (ignored files only)', () => { + assert.equal(isDestructive({ command: 'git clean -fx' }), true); + }); + + it('gates "git clean -fdx src/" (pathspec outside scratch)', () => { + assert.equal(isDestructive({ command: 'git clean -fdx src/' }), true); + }); + + it('gates "git clean --force --directory --ignored build/" (long forms)', () => { + assert.equal( + isDestructive({ command: 'git clean --force --directory --ignored build/' }), + true + ); + }); + + it('gates "cd /srv/app && git clean -fdx" (second command segment)', () => { + assert.equal(isDestructive({ command: 'cd /srv/app && git clean -fdx' }), true); + }); + + it('fires through evaluate() (full policy pipeline)', () => { + const r = evaluate( + 'Bash', + { command: 'git clean -fdx' }, + policy, + baseDir + ); + assert.ok(r, 'evaluate() should return a match'); + assert.equal(r.ruleId, 'destructive'); + }); + + // ---- allowlist interaction ---- + + it('exempts "git clean -fdx /tmp/build" (pathspec under scratch root)', () => { + assert.equal( + isDestructive({ command: 'git clean -fdx /tmp/build' }), + false, + 'a scoped clean of a scratch path stays exempt, like rm' + ); + }); + + // ---- non-force cleans MUST stay free ---- + + it('does not gate "git clean -n" (dry run)', () => { + assert.equal(isDestructive({ command: 'git clean -n' }), false); + }); + + it('does not gate "git clean -i" (interactive)', () => { + assert.equal(isDestructive({ command: 'git clean -i' }), false); + }); + + it('does not gate "git clean -fd" WITHOUT force is impossible - sanity: plain "git clean" stays free', () => { + assert.equal(isDestructive({ command: 'git clean' }), false); + }); + + it('does not gate "git clean -q" (quiet, nothing deleted)', () => { + assert.equal(isDestructive({ command: 'git clean -q' }), false); + }); +}); + +describe('policy: git clean pathspec handling (reviewer defects #1, #2)', () => { + // Defect #1: `-e ` takes a VALUE; that value must not be consumed + // as the pathspec. The old code read it as the pathspec and, when the + // exclude pattern happened to be allowlisted, laundered a whole-tree clean. + it('gates "git clean -fdx -e /tmp/keep" (exclude value is not a pathspec)', () => { + assert.equal( + isDestructive({ command: 'git clean -fdx -e /tmp/keep' }), + true, + '-e value must be skipped; with no real pathspec this is a whole-tree clean' + ); + }); + + // Defect #2: EVERY pathspec is checked, not just the first. An allowlisted + // first pathspec must not exempt later pathspecs. + it('gates "git clean -fd /tmp/build lib" (second pathspec is not allowlisted)', () => { + assert.equal( + isDestructive({ command: 'git clean -fd /tmp/build lib' }), + true, + 'an allowlisted first pathspec must not exempt an unallowlisted later one' + ); + }); + + it('still exempts "git clean -fdx /tmp/build /tmp/keep" (all pathspecs allowlisted)', () => { + assert.equal( + isDestructive({ command: 'git clean -fdx /tmp/build /tmp/keep' }), + false, + 'when EVERY pathspec is under scratch, the scoped clean stays exempt' + ); + }); +}); + +describe('policy: rm trigger scoped to its own segment (reviewer defect #3)', () => { + // Defect #3: rmTriggerFlags must scan the rm SEGMENT only, not the whole + // command. The old code fired on the rm when an earlier segment supplied + // the flags (a false-positive class the reviewer rejected as "cheap"). + it('does NOT gate "grep -rf patterns.txt . && rm notes.txt"', () => { + assert.equal( + isDestructive({ command: 'grep -rf patterns.txt . && rm notes.txt' }), + false, + 'the -rf belongs to grep; the rm segment has no force/recursive flags' + ); + }); + + it('does NOT gate "tar -rf archive.tar x && rm old.log"', () => { + assert.equal( + isDestructive({ command: 'tar -rf archive.tar x && rm old.log' }), + false, + 'tar -rf is not an rm trigger' + ); + }); + + it('still gates "cd /srv && rm -rf /srv/app" (rm segment carries the flags)', () => { + assert.equal(isDestructive({ command: 'cd /srv && rm -rf /srv/app' }), true); + }); +});