fix(hooks): close the bypasses anchoring opened in the destructive-command rule
Review finding 4638fea9 (MAJOR emitted, catalogue tier BLOCKER). Live in every
session regardless of the STORM flag, so it is not deferrable to the measurement
decision.
Anchoring the rule to command position (S75-S78) was right in intent - the
unanchored word match blocked quoted grep patterns, heredoc data and commit
messages - but it ran against the whitespace-collapsed string. Measured against
normalizeCommand() output, five forms that the previous rule blocked were
allowed:
- newline separator: \s+ -> ' ' collapsed the newline BEFORE the pattern ran,
making the \n branch of the separator class dead code
- `&` background separator: absent from the class entirely
- `bash -c` / `sh -c`: the wrapped command sits inside quotes, never at
command position
- `xargs <cmd>`: no separator in front of the command at all
And it missed its own motivating case: `grep "a|b" f` stayed blocked, because
the `|` inside the quotes still read as a separator.
Fix: the rule now runs against a command-position view (`commandView: true`,
per-rule input selection) instead of the collapsed string. The view keeps
newlines, adds `&` to the separator class, and classifies each span:
- quoted spans -> data (one space), so a grep alternation, echoed prose and a
commit message pass
- EXCEPT the argument of a shell wrapper (`sh -c`, `bash -c`, with optional
sudo and absolute path) -> spliced back in at command position
- heredoc bodies -> data, keeping the operator line. Restoring the newline
separator without this would newly block every heredoc line starting with a
matched word - the exact friction anchoring existed to remove
- `xargs [flags]` -> separator inserted after the flags
Two defects found while verifying, both the same regression class and both
fixed here rather than left:
- `\name` runs name (the backslash only suppresses alias expansion). The old
unanchored rule blocked it; the anchored one allowed it.
- heredoc bodies, as above - a false positive this change would otherwise have
introduced.
Known limit, stated rather than implied: `xargs -I {} <cmd>` is not parsed, so
the inserted separator lands before the argument, not the command.
Other BLOCK rules are untouched and still run against the collapsed string.
Verified by a 37-case adversarial probe through the real hook (all five bypass
forms, both wrapper forms, backslash, heredoc, quoted alternation, ordinary
commands, and the unrelated rules): 37/37 as expected. 9 new tests.
Suite 952 (950/0/2, baseline 937 + 15 across both Track A fixes).
Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013zNqxP8qTWgJhn3wMYUFEh
This commit is contained in:
parent
cef3e7fa24
commit
fa2404b63c
2 changed files with 122 additions and 3 deletions
|
|
@ -139,6 +139,42 @@ test('pre-bash-executor BLOCKS a destructive keyword after a separator', async (
|
|||
assert.strictEqual(code, 2);
|
||||
});
|
||||
|
||||
// Bypasses opened when the rule was anchored to command position: whitespace
|
||||
// was collapsed BEFORE the pattern ran (killing the newline branch), `&` was
|
||||
// missing from the separator class, and a keyword handed to a shell wrapper
|
||||
// sits at command position without any separator in front of it.
|
||||
test('pre-bash-executor BLOCKS a destructive keyword after a newline separator', async () => {
|
||||
const { code } = await runHook(PRE_BASH, bashInput('echo done\npoweroff'));
|
||||
assert.strictEqual(code, 2);
|
||||
});
|
||||
|
||||
test('pre-bash-executor BLOCKS a destructive keyword after a background separator', async () => {
|
||||
const { code } = await runHook(PRE_BASH, bashInput('echo done & poweroff'));
|
||||
assert.strictEqual(code, 2);
|
||||
});
|
||||
|
||||
test('pre-bash-executor BLOCKS a destructive command wrapped in bash -c', async () => {
|
||||
const { code } = await runHook(PRE_BASH, bashInput('bash -c "poweroff"'));
|
||||
assert.strictEqual(code, 2);
|
||||
});
|
||||
|
||||
test('pre-bash-executor BLOCKS a destructive command wrapped in sh -c', async () => {
|
||||
const { code } = await runHook(PRE_BASH, bashInput("sh -c 'reboot'"));
|
||||
assert.strictEqual(code, 2);
|
||||
});
|
||||
|
||||
test('pre-bash-executor BLOCKS a destructive command handed to xargs', async () => {
|
||||
const { code } = await runHook(PRE_BASH, bashInput('echo x | xargs reboot'));
|
||||
assert.strictEqual(code, 2);
|
||||
});
|
||||
|
||||
test('pre-bash-executor BLOCKS a backslash-escaped destructive command', async () => {
|
||||
// `\reboot` runs reboot — the backslash suppresses alias expansion, nothing
|
||||
// else. The command-position anchor must see through it.
|
||||
const { code } = await runHook(PRE_BASH, bashInput('\\reboot'));
|
||||
assert.strictEqual(code, 2);
|
||||
});
|
||||
|
||||
// -----------------------------------------------------------------------
|
||||
// ALLOW — the same keywords as DATA, not at command position.
|
||||
// The rule matched the bare word anywhere in the string, so a quoted grep
|
||||
|
|
@ -150,6 +186,30 @@ test('pre-bash-executor ALLOWS the keyword inside a quoted grep pattern', async
|
|||
assert.strictEqual(code, 0);
|
||||
});
|
||||
|
||||
// The change's own motivating case: a quoted grep alternation. Anchoring alone
|
||||
// did not reach it — the `|` inside the quotes reads as a separator unless
|
||||
// quoted spans are treated as data.
|
||||
test('pre-bash-executor ALLOWS a quoted grep alternation over the keywords', async () => {
|
||||
const { code } = await runHook(PRE_BASH, bashInput('grep "halt|poweroff" f.mjs'));
|
||||
assert.strictEqual(code, 0);
|
||||
});
|
||||
|
||||
// Heredoc bodies are data too, and a newline separator is what makes them look
|
||||
// like command position. The rule's own comment names heredoc data as the
|
||||
// friction anchoring was meant to remove.
|
||||
test('pre-bash-executor ALLOWS the keyword at the start of a heredoc body line', async () => {
|
||||
const { code } = await runHook(PRE_BASH, bashInput('cat <<EOF\nreboot is a word here\nEOF'));
|
||||
assert.strictEqual(code, 0);
|
||||
});
|
||||
|
||||
test('pre-bash-executor ALLOWS a commit message piped through a heredoc', async () => {
|
||||
const { code } = await runHook(
|
||||
PRE_BASH,
|
||||
bashInput("git commit -F - <<'MSG'\nhalt the loop on empty turns\nMSG"),
|
||||
);
|
||||
assert.strictEqual(code, 0);
|
||||
});
|
||||
|
||||
test('pre-bash-executor ALLOWS the keyword inside echoed prose', async () => {
|
||||
const { code } = await runHook(PRE_BASH, bashInput('echo "we should halt here"'));
|
||||
assert.strictEqual(code, 0);
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue