From 52d4be08563a58bf413b60fb2dc0e03548f47aa4 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 27 Apr 2026 09:34:57 +0000 Subject: [PATCH] fix(agent): expose --scope admin + lock down regression test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Issue #311 (SEC-A-09) original fix in PR #389 added `--scope {messaging,read,full}` with Messaging default. Two gaps remain. 1. Issue spec called for `readonly|messaging|full|admin`. The `admin` value is not exposed on the CLI even though `TokenScope::Admin` exists in scopes.rs and is semantically distinct from Full for audit log consumers. Added Admin variant to ScopeArg with mapping to TokenScope::Admin. 2. No test verified that omitting --scope yields the safer Messaging default — the regression that was the heart of SEC-A-09. Added tests::default_scope_is_messaging plus parse coverage for every documented value, an unknown-value-rejected case, and the ScopeArg → TokenScope wiring map. Covers behaviour at the lowest tier (Rust unit on the binary) per CLAUDE.md test-tier guidance. Doc on ScopeArg now lists per-scope tool sets so operators can pick without reading scopes.rs. Refs #311 Tradeoffs: - Considered changing TokenScope::default() from Full to Messaging to remove the footgun globally. Rejected: stdio transport (serve_stdio uses WillowMcpServer::new which reads the Default) is a local-process channel where Full is appropriate; flipping Default would either break stdio silently or force every test in tests/e2e.rs to call with_scope(TokenScope::Full). Out of scope for this issue. Worth a follow-up to make the Default explicit per call site and remove the Default impl entirely. --- crates/agent/src/main.rs | 96 +++++++++++++++++++++++++++++++++++++++- 1 file changed, 95 insertions(+), 1 deletion(-) diff --git a/crates/agent/src/main.rs b/crates/agent/src/main.rs index d1639ded..96d3e65a 100644 --- a/crates/agent/src/main.rs +++ b/crates/agent/src/main.rs @@ -72,7 +72,16 @@ struct Cli { } /// CLI-friendly enum for `--scope`. Maps to `TokenScope`. -#[derive(Copy, Clone, Debug, ValueEnum)] +/// +/// Per-scope tool list: +/// - `messaging` (default): `send_message`, `send_reply`, `edit_message`, +/// `delete_message`, `react`, `pin_message`, `unpin_message`, `send_typing`. +/// All resources readable. +/// - `read`: no tools. All resources readable. +/// - `full`: every tool reachable from the bearer token, all resources. +/// - `admin`: every tool, all resources. Semantically distinct from `full` +/// so audit log consumers can tell intent apart. +#[derive(Copy, Clone, Debug, PartialEq, Eq, ValueEnum)] enum ScopeArg { /// Messaging tools only (least-privilege default). Messaging, @@ -80,6 +89,8 @@ enum ScopeArg { Read, /// Full access: all tools, all resources. Full, + /// Admin access: all tools, all resources (audit-distinct from `full`). + Admin, } impl From for TokenScope { @@ -88,6 +99,7 @@ impl From for TokenScope { ScopeArg::Messaging => TokenScope::Messaging, ScopeArg::Read => TokenScope::ReadOnly, ScopeArg::Full => TokenScope::Full, + ScopeArg::Admin => TokenScope::Admin, } } } @@ -209,3 +221,85 @@ fn load_or_generate_identity(path: &str) -> anyhow::Result { } Ok(identity) } + +#[cfg(test)] +mod tests { + //! CLI parsing tests for `--scope`. Verifies that: + //! + //! - omitting `--scope` yields the least-privilege `Messaging` default + //! (closes the SEC-A-09 footgun where `serve_http` previously got + //! `TokenScope::default()` == `Full`), + //! - every documented value (`messaging`, `read`, `full`, `admin`) parses, + //! - each `ScopeArg` maps to the expected `TokenScope` variant. + //! + //! Regression test for https://github.com/intendednull/willow/issues/311. + use super::*; + use clap::Parser; + use willow_agent::scopes::TokenScope; + // Required arg for clap to accept a successful parse. + const MIN_ARGS: &[&str] = &["willow-agent"]; + + fn parse(extra: &[&str]) -> Cli { + let mut argv: Vec<&str> = MIN_ARGS.to_vec(); + argv.extend_from_slice(extra); + Cli::parse_from(argv) + } + + #[test] + fn default_scope_is_messaging() { + // SEC-A-09: when operator does not pass --scope, HTTP transport must + // not silently fall back to full admin authority. + let cli = parse(&[]); + assert_eq!(cli.scope, ScopeArg::Messaging); + } + + #[test] + fn scope_messaging_parses() { + let cli = parse(&["--scope", "messaging"]); + assert_eq!(cli.scope, ScopeArg::Messaging); + } + + #[test] + fn scope_read_parses() { + let cli = parse(&["--scope", "read"]); + assert_eq!(cli.scope, ScopeArg::Read); + } + + #[test] + fn scope_full_parses() { + let cli = parse(&["--scope", "full"]); + assert_eq!(cli.scope, ScopeArg::Full); + } + + #[test] + fn scope_admin_parses() { + let cli = parse(&["--scope", "admin"]); + assert_eq!(cli.scope, ScopeArg::Admin); + } + + #[test] + fn unknown_scope_rejected() { + let mut argv: Vec<&str> = MIN_ARGS.to_vec(); + argv.extend_from_slice(&["--scope", "root"]); + assert!(Cli::try_parse_from(argv).is_err()); + } + + #[test] + fn scope_arg_maps_to_token_scope() { + // ScopeArg → TokenScope wiring. If this drifts, the wrong scope + // ends up gating tool calls in serve_http. + assert!(matches!( + TokenScope::from(ScopeArg::Messaging), + TokenScope::Messaging + )); + assert!(matches!( + TokenScope::from(ScopeArg::Read), + TokenScope::ReadOnly + )); + assert!(matches!(TokenScope::from(ScopeArg::Full), TokenScope::Full)); + assert!(matches!( + TokenScope::from(ScopeArg::Admin), + TokenScope::Admin + )); + } +}