Files
optimclaw/tests/shell_risk_regression.rs
b58b421535 feat(shell): add Low/Medium/High risk levels for graduated command approval (closes #172) (#368)
* feat(shell): add Low/Medium/High risk levels for graduated approval (#172)

- Add `RiskLevel` enum (Low/Medium/High, Ord-comparable) to `tool.rs`
  and re-export from `tools/mod.rs`
- Add `risk_level_for(&params) -> RiskLevel` to the `Tool` trait
  (default: Low); override on `ShellTool` via `classify_command_risk`
- Add `classify_command_risk(command: &str) -> RiskLevel` to `shell.rs`:
  High for NEVER_AUTO_APPROVE patterns, Low for read-only prefixes,
  Medium for reversible mutations, Medium as the unknown-command default
- Add `extract_command_param` helper to de-duplicate JSON extraction
- Add `sudo ` to `NEVER_AUTO_APPROVE_PATTERNS` (now classified High)
- Wire `risk_level_for` into `requires_approval`: Low → Never,
  Medium → UnlessAutoApproved, High → Always (uses upstream's new API)
- Log risk level at INFO on every tool call in `worker.rs`
- Replace `requires_explicit_approval` (simple bool) with the richer
  `classify_command_risk`; update dispatcher.rs test
- Add tests: `test_classify_command_risk_high/low/medium/pipeline`,
  `test_risk_level_for_via_tool_trait`, updated approval tests

Co-Authored-By: Claude Sonnet 4.6 <[email protected]>

* style: apply cargo fmt to shell.rs and dispatcher.rs

Co-Authored-By: Claude Sonnet 4.6 <[email protected]>

* fix(shell): fix pipeline risk aggregation and word-boundary matching

Address reviewer feedback:

- `classify_command_risk` now iterates ALL pipeline segments and takes
  the maximum risk, so `echo hello | cargo build` → Medium instead of
  the previous (wrong) Low
- Replace `starts_with` with `matches_command_pattern`: single-word
  patterns use exact first-token comparison so `lsblk` no longer
  matches `ls`, `makeself` no longer matches `make`, etc.; multi-word
  patterns (e.g. `git status`) still use starts_with + space boundary
- Drop `--help` / `-h` from LOW_RISK_PATTERNS (can never be first token)
- Add `test_classify_command_risk_word_boundary` and extend pipeline
  test with mixed Low+Medium and unknown-command cases

Co-Authored-By: Claude Sonnet 4.6 <[email protected]>

* fix(shell): move sed/awk/find from Low to Medium risk

`sed -i`, `awk -i inplace`, and `find -delete`/`find -exec rm` can all
modify or delete files. Classifying these as Low (auto-approve) was
unsafe. Moving to Medium requires UnlessAutoApproved approval, which
prompts the user unless they have explicitly enabled auto-approve mode.

Fixes review feedback from zmanian on PR #368.

Co-Authored-By: Claude Sonnet 4.6 <[email protected]>

* fix(shell): update test to use classify_command_risk after requires_explicit_approval removal

The rebase brought in upstream commits that removed requires_explicit_approval.
Update the mixed-case destructive command test to assert RiskLevel::High via
classify_command_risk instead.

Co-Authored-By: Claude Sonnet 4.6 <[email protected]>

* fix(shell): use word-boundary matching for High-risk patterns to prevent false positives

The NEVER_AUTO_APPROVE_PATTERNS check used `contains()` on the full command
string, causing false positives: `makeshutdownscript` matched `shutdown`,
`nftables-config` matched `nft`, and `passwdqc-check` matched `passwd`.

Fix: move the High-risk check inside the per-segment loop and use
`matches_command_pattern` (the same word-boundary logic used for Low/Medium),
so classification is consistent across all three risk levels.

Also remove the trailing spaces from `"nft "` and `"sudo "` in
NEVER_AUTO_APPROVE_PATTERNS since `matches_command_pattern` handles
word-boundary detection without them.

Adds three regression tests for the false-positive cases.

Co-Authored-By: Claude Sonnet 4.6 <[email protected]>

* fix(shell): address zmanian review — redirect safety + explicit git push pattern

Two issues from zmanian's CHANGES_REQUESTED review on PR #368:

1. **Security (Low → UnlessAutoApproved)**: `Low` was mapped to
   `ApprovalRequirement::Never`, bypassing approval entirely for commands like
   `cat /etc/shadow > /tmp/out` since the pipeline splitter does not split on
   shell redirections (`>`, `>>`). Changing to `UnlessAutoApproved` preserves
   the graduated risk metadata for audit while keeping approval policy
   conservative until redirect-aware parsing is in place.

2. **Minor (explicit git push pattern)**: `git push origin feature-branch`
   fell through to the unknown-command Medium default rather than matching an
   explicit pattern. Adding `"git push"` to MEDIUM_RISK_PATTERNS makes the
   classification intentional. Force-push variants (`git push --force`,
   `git push -f`) remain in NEVER_AUTO_APPROVE_PATTERNS (High).

Co-Authored-By: Claude Sonnet 4.6 <[email protected]>

* test(shell): add regression tests for redirect bypass and git push pattern fixes

Two regression tests for the fixes in the previous commit:

1. `test_low_risk_with_redirect_not_never` — verifies that Low-risk commands
   containing shell redirections (`echo x > /etc/passwd`, `cat /etc/shadow > /tmp/out`,
   etc.) return `UnlessAutoApproved`, not `Never`. Before the fix, `Low` mapped to
   `Never` which would have allowed these writes to bypass approval entirely.

2. `test_git_push_explicit_medium_pattern` — verifies that `git push origin branch`
   is classified `Medium` via the explicit `MEDIUM_RISK_PATTERNS` entry (not the
   unknown-command fallthrough). Force variants (`--force`, `-f`) remain `High`.

Co-Authored-By: Claude Sonnet 4.6 <[email protected]>

* test(shell): add integration regression tests for redirect bypass and git push

Covers the two fixes from the previous commits at the integration-test level
(tests/ directory) to ensure the CI regression-test gate is satisfied:

1. `low_risk_command_with_redirect_is_unless_auto_approved` -- verifies that
   Low-risk commands containing shell redirections return UnlessAutoApproved,
   not Never (the pre-fix behaviour that allowed redirect-based bypass).

2. `git_push_is_unless_auto_approved` -- verifies git push is Medium risk
   (UnlessAutoApproved) via the explicit pattern, not unknown-command fallthrough.

3. `git_push_force_requires_always_approval` -- verifies force-push variants
   remain High risk (Always approval required).

Co-Authored-By: Claude Sonnet 4.6 <[email protected]>

* refactor(test): move inline assertions to tests/ to satisfy no-panics CI check

The project's no-panics CI check (code_style.yml) scans src/**/*.rs for
assert_eq!/assert_ne!/.unwrap() in added lines. Moving classify_command_risk
tests to tests/shell_risk_regression.rs and adding // safety: comments on
the two remaining assertions in dispatcher.rs eliminates all false positives.

- Remove test_classify_command_risk_* and related functions from shell.rs
- Remove test_low_risk_with_redirect_not_never and test_git_push_* from
  shell.rs (covered by integration tests in tests/)
- Expand tests/shell_risk_regression.rs with full coverage via public API
- Add // safety: test code comments on dispatcher.rs assert lines

Co-Authored-By: Claude Sonnet 4.6 <[email protected]>

* fix(shell): address review findings — force-with-lease, test runners, Display

- Add `git push --force-with-lease` to NEVER_AUTO_APPROVE_PATTERNS — the
  word-boundary matching in matches_command_pattern would not match it
  against the existing `git push --force` pattern (next char is `-`, not
  space), causing it to fall through to Medium instead of High.

- Move `cargo test`, `npm test`, `npm run test`, `yarn test` from
  LOW_RISK_PATTERNS to MEDIUM_RISK_PATTERNS — test runners execute
  arbitrary code and can have side effects (file creation, network calls,
  process spawning).

- Add `Display` impl for `RiskLevel` (lowercase: low/medium/high) and
  switch worker logging from `?risk` (Debug) to `%risk` (Display) for
  cleaner audit logs.

- Fix integration test helper to call `register_dev_tools()` since
  ShellTool is registered there, not in `register_builtin_tools()`.

Co-Authored-By: Claude Opus 4.6 (1M context) <[email protected]>

---------

Co-authored-by: Claude Sonnet 4.6 <[email protected]>
Co-authored-by: [email protected] <[email protected]>
2026-03-21 22:05:18 -07:00

281 lines
9.0 KiB
Rust

//! Regression and unit tests for shell command risk-level classification
//! (issue #172, PR #368).
//!
//! These tests live here (instead of inline in `src/tools/builtin/shell.rs`)
//! because the project's no-panics CI check scans `src/**/*.rs` for
//! `assert_eq!` / `assert_ne!` / `.unwrap()` in added lines. All assertions
//! on the public `ShellTool` API belong here.
//!
//! All tests access the shell tool through the public `ToolRegistry` +
//! `Tool` trait surface (`risk_level_for`, `requires_approval`).
//!
//! ## What is tested
//!
//! 1. **Risk level tiers** (`High`, `Medium`, `Low`) for representative commands.
//! 2. **Word-boundary matching** — commands whose names are substrings of other
//! words must not be misclassified.
//! 3. **Pipeline aggregation** — the whole pipeline takes the maximum risk of
//! its segments.
//! 4. **Redirect bypass regression** — Low-risk commands with shell redirections
//! must return `UnlessAutoApproved`, not `Never`.
//! 5. **`git push` regression** — non-force push is explicitly `Medium`; force
//! variants remain `High`.
//! 6. **`risk_level_for` trait method** — delegates to classify_command_risk.
use ironclaw::tools::{ApprovalRequirement, RiskLevel, Tool, ToolRegistry};
use std::sync::Arc;
// ---------------------------------------------------------------------------
// Helper: obtain a `ShellTool` from the registry
// ---------------------------------------------------------------------------
async fn shell_tool() -> Arc<dyn Tool> {
let registry = ToolRegistry::new();
registry.register_builtin_tools();
registry.register_dev_tools();
registry
.all()
.await
.into_iter()
.find(|t| t.name() == "shell")
.expect("shell tool must be registered")
}
fn risk(tool: &Arc<dyn Tool>, cmd: &str) -> RiskLevel {
tool.risk_level_for(&serde_json::json!({ "command": cmd }))
}
fn approval(tool: &Arc<dyn Tool>, cmd: &str) -> ApprovalRequirement {
tool.requires_approval(&serde_json::json!({ "command": cmd }))
}
// ---------------------------------------------------------------------------
// 1. Risk level tiers
// ---------------------------------------------------------------------------
#[tokio::test]
async fn high_risk_commands() {
let tool = shell_tool().await;
let cmds = [
"rm -rf /tmp/stuff",
"git push --force origin main",
"git reset --hard HEAD~5",
"docker rm container_name",
"kill -9 12345",
"DROP TABLE users;",
"sudo apt install something",
];
for cmd in &cmds {
assert_eq!(
risk(&tool, cmd),
RiskLevel::High,
"command `{cmd}` should be High risk"
);
}
}
#[tokio::test]
async fn low_risk_commands() {
let tool = shell_tool().await;
let cmds = [
"ls -la",
"cat file.txt",
"grep foo bar.txt",
"git status",
"git log --oneline",
"echo hello",
"cargo check",
];
for cmd in &cmds {
assert_eq!(
risk(&tool, cmd),
RiskLevel::Low,
"command `{cmd}` should be Low risk"
);
}
}
#[tokio::test]
async fn medium_risk_commands() {
let tool = shell_tool().await;
let cmds = [
"cargo build",
"cargo test",
"npm test",
"yarn test",
"git commit -m 'foo'",
"mkdir /tmp/dir",
"npm install lodash",
"git push origin feature-branch",
"my-custom-tool --flag",
"sed 's/foo/bar/g' file.txt",
"sed -i 's/foo/bar/' file.txt",
"awk '{print $1}' file.txt",
"find . -name '*.rs'",
"find . -delete",
];
for cmd in &cmds {
assert_eq!(
risk(&tool, cmd),
RiskLevel::Medium,
"command `{cmd}` should be Medium risk"
);
}
}
// ---------------------------------------------------------------------------
// 2. Word-boundary matching (no false positives for substrings)
// ---------------------------------------------------------------------------
#[tokio::test]
async fn word_boundary_no_false_positives() {
let tool = shell_tool().await;
// "lsblk" must NOT match "ls" (Low-risk prefix)
assert_eq!(risk(&tool, "lsblk"), RiskLevel::Medium);
// "makeself" must NOT match "make"
assert_eq!(risk(&tool, "makeself output.run"), RiskLevel::Medium);
// "git statusbar" must NOT match "git status"
assert_eq!(risk(&tool, "git statusbar"), RiskLevel::Medium);
// Commands with High-risk names as substrings must not be tagged High
assert_eq!(risk(&tool, "makeshutdownscript --help"), RiskLevel::Medium);
assert_eq!(risk(&tool, "nftables-config"), RiskLevel::Medium);
assert_eq!(risk(&tool, "passwdqc-check"), RiskLevel::Medium);
}
#[tokio::test]
async fn word_boundary_correct_positive_matches() {
let tool = shell_tool().await;
assert_eq!(risk(&tool, "ls -la"), RiskLevel::Low);
assert_eq!(risk(&tool, "make install"), RiskLevel::Medium);
assert_eq!(risk(&tool, "git status"), RiskLevel::Low);
}
// ---------------------------------------------------------------------------
// 3. Pipeline aggregation
// ---------------------------------------------------------------------------
#[tokio::test]
async fn pipeline_takes_max_risk() {
let tool = shell_tool().await;
// High-risk segment → whole pipeline is High
assert_eq!(risk(&tool, "ls /tmp | rm -rf /tmp/stuff"), RiskLevel::High);
// All-low pipeline stays Low
assert_eq!(risk(&tool, "ls -la | grep foo"), RiskLevel::Low);
// Low + Medium → max is Medium
assert_eq!(risk(&tool, "echo hello | cargo build"), RiskLevel::Medium);
// Unknown command in pipeline → Medium (safe default)
assert_eq!(
risk(&tool, "cat file.txt | my-custom-tool"),
RiskLevel::Medium
);
}
// ---------------------------------------------------------------------------
// 4. Redirect bypass regression (Low → UnlessAutoApproved, not Never)
// ---------------------------------------------------------------------------
#[tokio::test]
async fn low_risk_command_with_redirect_is_unless_auto_approved() {
let tool = shell_tool().await;
let cases = [
"echo secret_data > /etc/passwd",
"cat /etc/shadow > /tmp/exfil.txt",
"printf '%s' value > /tmp/leak",
"ls -la >> /tmp/log.txt",
];
for cmd in &cases {
let result = approval(&tool, cmd);
assert_eq!(
result,
ApprovalRequirement::UnlessAutoApproved,
"command `{cmd}` must be UnlessAutoApproved (not Never), got {result:?}"
);
}
}
// ---------------------------------------------------------------------------
// 5. git push regressions
// ---------------------------------------------------------------------------
#[tokio::test]
async fn git_push_classifies_as_medium_risk() {
let tool = shell_tool().await;
let cmds = [
"git push",
"git push origin main",
"git push --set-upstream origin feature",
"git push upstream feature/foo",
];
for cmd in &cmds {
assert_eq!(risk(&tool, cmd), RiskLevel::Medium, "command `{cmd}`");
}
}
#[tokio::test]
async fn git_push_force_remains_high_risk() {
let tool = shell_tool().await;
let cmds = [
"git push --force",
"git push -f",
"git push --force-with-lease",
"git push --force origin main",
"git push -f origin main",
];
for cmd in &cmds {
assert_eq!(risk(&tool, cmd), RiskLevel::High, "command `{cmd}`");
}
}
#[tokio::test]
async fn git_push_non_force_is_unless_auto_approved() {
let tool = shell_tool().await;
let cmds = [
"git push",
"git push origin main",
"git push upstream feature/foo",
];
for cmd in &cmds {
let result = approval(&tool, cmd);
assert_eq!(
result,
ApprovalRequirement::UnlessAutoApproved,
"command `{cmd}` should be UnlessAutoApproved, got {result:?}"
);
}
}
#[tokio::test]
async fn git_push_force_requires_always_approval() {
let tool = shell_tool().await;
let cmds = [
"git push --force",
"git push -f",
"git push --force-with-lease",
];
for cmd in &cmds {
let result = approval(&tool, cmd);
assert_eq!(
result,
ApprovalRequirement::Always,
"force-push `{cmd}` should require Always approval, got {result:?}"
);
}
}
// ---------------------------------------------------------------------------
// 6. risk_level_for trait method
// ---------------------------------------------------------------------------
#[tokio::test]
async fn risk_level_for_via_tool_trait() {
let tool = shell_tool().await;
assert_eq!(risk(&tool, "ls -la"), RiskLevel::Low);
assert_eq!(risk(&tool, "cargo build"), RiskLevel::Medium);
assert_eq!(risk(&tool, "rm -rf /tmp"), RiskLevel::High);
// Missing params → Medium (safe default)
assert_eq!(
tool.risk_level_for(&serde_json::json!({})),
RiskLevel::Medium
);
}