feat: add opt-in bounded shell execution - #100
Conversation
There was a problem hiding this comment.
This is an auto review done by revuto.
Reviewed the full opt-in shell execution surface (src/server.rs, scripts/mcp_safety_check.py, docs, deps). The load-bearing security claims check out against source:
- Process-tree cleanup:
command_runner::output_with_inputsetskill_on_drop(true)+process_group(0), andProcessGroupGuardSIGKILLs-pgidon timeout/cancel (src/command_runner.rs:134-148, 382-434), with existing tests covering pipe-holding descendants. - Bounded execution/output: timeout clamped to 1..=120s;
read_pipehard-caps collection at 8 MiB/stream beforebounded_shell_streamtruncates responses to 512 KiB — so "bounded collection" in the README is accurate, not just response-side truncation. - Default surface preserved:
ToolRouter.mapis public, the route removal is real, and the safety script now asserts exactly 18 tools by default / 19 with opt-in, verifies annotations, excludes an ambient test secret, and exercises explicit env — enforced in CI. - Version/schema coherence: no existing tool's parameters changed;
run_shellis additive and absent by default, so no breaking MCP contract.sha2is well-known RustCrypto with a small lockfile delta.
One robustness defect found (inline below): the environment-inheritance loop uses std::env::vars(), which panics on any non-UTF-8 variable in the server's environment, taking down the connection instead of returning the structured error every other failure path produces. Everything else looks solid.
There was a problem hiding this comment.
This is an auto review done by revuto.
Reviewed the opt-in shell execution surface end to end: tool gating, env construction, execution path via command_runner, docs, and the extended safety gate.
Verified as sound:
- Default surface stays at 18 tools:
router.map.remove("run_shell")at registration and a runtime re-check inexecute_shell— discovery alone cannot reach execution, matching the stated invariant. - Timeout enforcement, process-group cleanup (
process_group(0)+ProcessGroupGuardkill-on-drop),kill_on_drop, and stdinStdio::null()all come fromcommand_runner::output_with_timeout, so the "process-tree cleanup" claim holds. - Memory is bounded during collection at 8 MiB/stream, so a
yes-style firehose cannot OOM the server within the 120 s ceiling. - Follow-up commit correctly switched to
env::vars_os()with per-entryinto_string()fallthrough (no panic on non-UTF-8 ambient env); env name validation blocks=/-injection intoenviron. - Error contract matches the existing
Json<Output>+error: Option<String>convention used by every other tool; the audit record is digest-keyed and never echoes command text. sha2 = "0.11.0"is well-known RustCrypto (pure Rust +cpufeatures), no sys/FFI surface. Version handling follows the repo's land-under-[Unreleased]convention; the default tool surface is unchanged, so no migration note is required.
Two findings where the implementation doesn't match the contract this PR documents for itself — both inline. Neither blocks the design; both are about the stated environment/output guarantees.
There was a problem hiding this comment.
This is an auto review done by revuto.
Revuto completed the review and found no evidence-backed concerns.
Summary
Closes #99
Threat model
The realistic attacker controls MCP tool arguments through untrusted model/context input but cannot set the server process environment. The security invariant is that discovery alone never grants process execution. Operator opt-in plus host approval are prerequisites; after both, same-user file and network access is intended authority, while default exposure, ambient credential inheritance, unbounded execution/output, or failed process-tree cleanup remain security defects.
Verification
cargo testcargo clippy --all-targets -- -D warningscargo buildpython3 scripts/mcp_safety_check.py --binary target/debug/computer-use-linux --repo .cargo fmt --all -- --checkgit diff --check