From 18c45ea85fdff64cd22c6f781ee27a377a97b033 Mon Sep 17 00:00:00 2001 From: Brad Groux <3053586+BradGroux@users.noreply.github.com> Date: Sun, 16 Aug 2026 06:27:10 -0500 Subject: [PATCH 01/11] feat(acp): accept extra MCP servers via BUZZ_ACP_EXTRA_MCP_COMMANDS MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Desktop-managed Buzz agents could only use one MCP server (buzz-dev-mcp). Users who wanted web search or other third-party MCP tools alongside the built-in local tools had no way to add them — the desktop flow hardcodes a single MCP command and build_mcp_servers() always returned a vec of one entry. Added BUZZ_ACP_EXTRA_MCP_COMMANDS, a comma-separated env var where each entry is split on whitespace into command + args. build_mcp_servers() appends each as a separate McpServer after the primary server. Extra servers do not receive Buzz relay credentials (BUZZ_RELAY_URL, BUZZ_PRIVATE_KEY) or auth tags — they are third-party tools, not Buzz-native MCP servers. The primary mcp_command short-circuit is preserved: if it is empty, no servers are returned at all, even when extra commands are configured. This is option 1 from issue #6023. It unblocks the web-search use case (e.g. npx -y mcp-remote https://mcp.tavily.com/mcp/...) without desktop UI changes. Closes #6023 Signed-off-by: dm-builder Co-authored-by: Brad Groux Signed-off-by: Brad Groux Signed-off-by: wiggdevin <202901685+wiggdevin@users.noreply.github.com> --- crates/buzz-acp/src/config.rs | 10 ++++ crates/buzz-acp/src/lib.rs | 110 +++++++++++++++++++++++++++++++++- 2 files changed, 118 insertions(+), 2 deletions(-) diff --git a/crates/buzz-acp/src/config.rs b/crates/buzz-acp/src/config.rs index 5b7e27131ec..7a327a63887 100644 --- a/crates/buzz-acp/src/config.rs +++ b/crates/buzz-acp/src/config.rs @@ -267,6 +267,13 @@ pub struct CliArgs { #[arg(long, env = "BUZZ_ACP_MCP_COMMAND", default_value = "")] pub mcp_command: String, + /// Additional MCP server commands to pass to the agent session alongside + /// the primary MCP server. Each comma-separated entry is split on + /// whitespace into a command and its args. Example: + /// `npx -y mcp-remote https://mcp.tavily.com/mcp/?tavilyApiKey=...,other-server` + #[arg(long, env = "BUZZ_ACP_EXTRA_MCP_COMMANDS", value_delimiter = ',')] + pub extra_mcp_commands: Vec, + /// Idle timeout: max seconds of silence before killing a turn. /// Resets on any agent stdout activity. #[arg(long, env = "BUZZ_ACP_IDLE_TIMEOUT")] @@ -543,6 +550,7 @@ pub struct Config { pub agent_command: String, pub agent_args: Vec, pub mcp_command: String, + pub extra_mcp_commands: Vec, pub idle_timeout_secs: u64, pub max_turn_duration_secs: u64, pub agents: u32, @@ -1156,6 +1164,7 @@ impl Config { agent_command, agent_args, mcp_command: args.mcp_command, + extra_mcp_commands: args.extra_mcp_commands, idle_timeout_secs, max_turn_duration_secs, agents: args.agents, @@ -1540,6 +1549,7 @@ mod tests { agent_command: "goose".into(), agent_args: vec!["acp".into()], mcp_command: "".into(), + extra_mcp_commands: vec![], idle_timeout_secs: DEFAULT_IDLE_TIMEOUT_SECS, max_turn_duration_secs: DEFAULT_MAX_TURN_DURATION_SECS, agents: 1, diff --git a/crates/buzz-acp/src/lib.rs b/crates/buzz-acp/src/lib.rs index d72d4cf482a..fa9b4648a5e 100644 --- a/crates/buzz-acp/src/lib.rs +++ b/crates/buzz-acp/src/lib.rs @@ -5731,7 +5731,7 @@ fn build_mcp_servers(config: &Config) -> Vec { if config.mcp_command.is_empty() { return vec![]; } - vec![McpServer { + let mut servers = vec![McpServer { name: std::path::Path::new(&config.mcp_command) .file_stem() .and_then(|s| s.to_str()) @@ -5781,7 +5781,32 @@ fn build_mcp_servers(config: &Config) -> Vec { } env }, - }] + }]; + + // Append extra MCP servers from BUZZ_ACP_EXTRA_MCP_COMMANDS. + // Each entry is split on whitespace into command + args. + // Extra servers do not receive Buzz relay credentials or auth tags — + // they are third-party tools, not Buzz-native MCP servers. + for extra in &config.extra_mcp_commands { + let parts: Vec<&str> = extra.split_whitespace().collect(); + if parts.is_empty() { + continue; + } + let command = parts[0].to_string(); + let args: Vec = parts[1..].iter().map(|s| s.to_string()).collect(); + servers.push(McpServer { + name: std::path::Path::new(&command) + .file_stem() + .and_then(|s| s.to_str()) + .unwrap_or("extra-mcp") + .to_string(), + command, + args, + env: vec![], + }); + } + + servers } #[cfg(test)] @@ -8897,6 +8922,7 @@ mod build_mcp_servers_tests { agent_command: "goose".into(), agent_args: vec!["acp".into()], mcp_command: "test-mcp-server".into(), + extra_mcp_commands: vec![], idle_timeout_secs: config::DEFAULT_IDLE_TIMEOUT_SECS, max_turn_duration_secs: config::DEFAULT_MAX_TURN_DURATION_SECS, agents: 1, @@ -9086,6 +9112,85 @@ mod build_mcp_servers_tests { "Path::new(\".\").file_stem() is None — should fall back to \"mcp\"" ); } + + #[test] + fn extra_mcp_commands_append_additional_servers() { + let mut config = test_config(); + config.extra_mcp_commands = vec![ + "npx -y mcp-remote https://mcp.tavily.com/mcp/?tavilyApiKey=test-key".into(), + ]; + let servers = build_mcp_servers(&config); + assert_eq!(servers.len(), 2, "primary + 1 extra = 2 servers"); + assert_eq!(servers[0].name, "test-mcp-server"); + assert_eq!(servers[1].name, "npx"); + assert_eq!(servers[1].command, "npx"); + assert_eq!( + servers[1].args, + vec![ + "-y", + "mcp-remote", + "https://mcp.tavily.com/mcp/?tavilyApiKey=test-key" + ] + ); + // Extra servers should not receive Buzz relay credentials. + let env_names: Vec<&str> = servers[1].env.iter().map(|e| e.name.as_str()).collect(); + assert!( + !env_names.contains(&"BUZZ_RELAY_URL"), + "extra MCP servers should not get BUZZ_RELAY_URL" + ); + assert!( + !env_names.contains(&"BUZZ_PRIVATE_KEY"), + "extra MCP servers should not get BUZZ_PRIVATE_KEY" + ); + } + + #[test] + fn multiple_extra_mcp_commands_append_in_order() { + let mut config = test_config(); + config.extra_mcp_commands = vec![ + "brave-search-mcp".into(), + "npx -y mcp-remote https://mcp.tavily.com/mcp/".into(), + ]; + let servers = build_mcp_servers(&config); + assert_eq!(servers.len(), 3, "primary + 2 extra = 3 servers"); + assert_eq!(servers[0].name, "test-mcp-server"); + assert_eq!(servers[1].name, "brave-search-mcp"); + assert_eq!(servers[2].name, "npx"); + } + + #[test] + fn empty_extra_mcp_commands_are_skipped() { + let mut config = test_config(); + config.extra_mcp_commands = vec![ + "valid-server".into(), + "".into(), + " ".into(), + "another-server arg1".into(), + ]; + let servers = build_mcp_servers(&config); + assert_eq!( + servers.len(), + 3, + "primary + 2 valid extras (empty and whitespace-only skipped)" + ); + assert_eq!(servers[0].name, "test-mcp-server"); + assert_eq!(servers[1].name, "valid-server"); + assert_eq!(servers[1].args.len(), 0); + assert_eq!(servers[2].name, "another-server"); + assert_eq!(servers[2].args, vec!["arg1"]); + } + + #[test] + fn extra_mcp_commands_with_empty_mcp_command_returns_no_servers() { + let mut config = test_config(); + config.mcp_command = "".into(); + config.extra_mcp_commands = vec!["some-extra-server".into()]; + let servers = build_mcp_servers(&config); + assert!( + servers.is_empty(), + "empty primary mcp_command should still short-circuit even with extras" + ); + } } #[cfg(test)] @@ -9123,6 +9228,7 @@ mod error_outcome_emission_tests { agent_command: "true".into(), agent_args: vec![], mcp_command: "test-mcp-server".into(), + extra_mcp_commands: vec![], idle_timeout_secs: config::DEFAULT_IDLE_TIMEOUT_SECS, max_turn_duration_secs: config::DEFAULT_MAX_TURN_DURATION_SECS, agents: 1, From 768c6e37d1db40b4bb95d8f71c929fb84c9e1278 Mon Sep 17 00:00:00 2001 From: Brad Groux <3053586+BradGroux@users.noreply.github.com> Date: Mon, 17 Aug 2026 00:32:01 -0500 Subject: [PATCH 02/11] Fix extra MCP server name collisions and shell-aware argv parsing Two correctness issues in the extra MCP server path: 1. Server names derived only from the executable stem collided for common multi-server configurations. Two wrappers like 'npx -y first-mcp' and 'npx -y second-mcp' both became 'npx', tripping McpRegistry's duplicate check at spawn. Now disambiguate with a numeric suffix (npx, npx-2). 2. split_whitespace() corrupted quoted executable paths and arguments containing spaces. Replace with shlex::split, which handles standard shell quoting. Malformed entries are skipped with a warning instead of being silently reinterpreted. Also update the config doc comment to document the delimiter/quoting contract. Addresses themiguelamador's review feedback. Co-authored-by: Brad Groux Signed-off-by: Brad Groux Signed-off-by: wiggdevin <202901685+wiggdevin@users.noreply.github.com> --- Cargo.lock | 1 + crates/buzz-acp/Cargo.toml | 3 ++ crates/buzz-acp/src/config.rs | 8 ++- crates/buzz-acp/src/lib.rs | 95 ++++++++++++++++++++++++++++++----- 4 files changed, 92 insertions(+), 15 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index 9ad2a913e6b..49f9fbe0c7e 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -847,6 +847,7 @@ dependencies = [ "serde", "serde_json", "sha2 0.11.0", + "shlex", "thiserror 2.0.18", "tokio", "tokio-tungstenite 0.29.0", diff --git a/crates/buzz-acp/Cargo.toml b/crates/buzz-acp/Cargo.toml index d047849806f..e828b40bda6 100644 --- a/crates/buzz-acp/Cargo.toml +++ b/crates/buzz-acp/Cargo.toml @@ -68,6 +68,9 @@ clap = { version = "4", features = ["derive", "env"] } # Config file toml = "1.0" +# Shell-style argv splitting for BUZZ_ACP_AGENT_ARGS and extra MCP commands +shlex = "1.3" + # Filter expressions evalexpr = { workspace = true } diff --git a/crates/buzz-acp/src/config.rs b/crates/buzz-acp/src/config.rs index 7a327a63887..18c728153cb 100644 --- a/crates/buzz-acp/src/config.rs +++ b/crates/buzz-acp/src/config.rs @@ -268,8 +268,12 @@ pub struct CliArgs { pub mcp_command: String, /// Additional MCP server commands to pass to the agent session alongside - /// the primary MCP server. Each comma-separated entry is split on - /// whitespace into a command and its args. Example: + /// the primary MCP server. Entries are comma-separated; each entry is + /// shell-split (shlex) into a command and its args, so quoted paths and + /// arguments with spaces are preserved. Server names are derived from the + /// executable stem and disambiguated with a numeric suffix if duplicates + /// occur (e.g. two `npx` wrappers become `npx` and `npx-2`). Entries with + /// malformed quoting are skipped with a warning. Example: /// `npx -y mcp-remote https://mcp.tavily.com/mcp/?tavilyApiKey=...,other-server` #[arg(long, env = "BUZZ_ACP_EXTRA_MCP_COMMANDS", value_delimiter = ',')] pub extra_mcp_commands: Vec, diff --git a/crates/buzz-acp/src/lib.rs b/crates/buzz-acp/src/lib.rs index fa9b4648a5e..1ab6ac93892 100644 --- a/crates/buzz-acp/src/lib.rs +++ b/crates/buzz-acp/src/lib.rs @@ -5784,22 +5784,55 @@ fn build_mcp_servers(config: &Config) -> Vec { }]; // Append extra MCP servers from BUZZ_ACP_EXTRA_MCP_COMMANDS. - // Each entry is split on whitespace into command + args. + // Each entry is shell-split into command + args using shlex, so quoted + // paths and arguments with spaces are preserved. Malformed entries are + // preserved as-is with a warning. // Extra servers do not receive Buzz relay credentials or auth tags — // they are third-party tools, not Buzz-native MCP servers. + let mut seen_names: std::collections::HashSet = + std::collections::HashSet::from_iter([servers[0].name.clone()]); for extra in &config.extra_mcp_commands { - let parts: Vec<&str> = extra.split_whitespace().collect(); - if parts.is_empty() { + let trimmed = extra.trim(); + if trimmed.is_empty() { continue; } - let command = parts[0].to_string(); - let args: Vec = parts[1..].iter().map(|s| s.to_string()).collect(); + let parts = match shlex::split(trimmed) { + Some(p) if !p.is_empty() => p, + Some(_) => continue, + None => { + tracing::warn!( + entry = %trimmed, + "BUZZ_ACP_EXTRA_MCP_COMMANDS entry has malformed shell quoting; skipping" + ); + continue; + } + }; + let command = parts[0].clone(); + let args: Vec = parts[1..].to_vec(); + // Derive a name from the executable stem, then disambiguate so + // two wrappers like `npx -y first-mcp` and `npx -y second-mcp` + // don't both become `npx` and trip McpRegistry's duplicate check. + let base_name = std::path::Path::new(&command) + .file_stem() + .and_then(|s| s.to_str()) + .unwrap_or("extra-mcp") + .to_string(); + let name = if seen_names.contains(&base_name) { + // Append a numeric suffix until we find a unique name. + let mut i = 2; + loop { + let candidate = format!("{base_name}-{i}"); + if !seen_names.contains(&candidate) { + break candidate; + } + i += 1; + } + } else { + base_name + }; + seen_names.insert(name.clone()); servers.push(McpServer { - name: std::path::Path::new(&command) - .file_stem() - .and_then(|s| s.to_str()) - .unwrap_or("extra-mcp") - .to_string(), + name, command, args, env: vec![], @@ -9116,9 +9149,8 @@ mod build_mcp_servers_tests { #[test] fn extra_mcp_commands_append_additional_servers() { let mut config = test_config(); - config.extra_mcp_commands = vec![ - "npx -y mcp-remote https://mcp.tavily.com/mcp/?tavilyApiKey=test-key".into(), - ]; + config.extra_mcp_commands = + vec!["npx -y mcp-remote https://mcp.tavily.com/mcp/?tavilyApiKey=test-key".into()]; let servers = build_mcp_servers(&config); assert_eq!(servers.len(), 2, "primary + 1 extra = 2 servers"); assert_eq!(servers[0].name, "test-mcp-server"); @@ -9191,6 +9223,43 @@ mod build_mcp_servers_tests { "empty primary mcp_command should still short-circuit even with extras" ); } + + #[test] + fn extra_mcp_commands_disambiguate_duplicate_names() { + // Two npx-based wrappers must not both become "npx" — that would + // trip McpRegistry's duplicate-name check at spawn. + let mut config = test_config(); + config.extra_mcp_commands = vec!["npx -y first-mcp".into(), "npx -y second-mcp".into()]; + let servers = build_mcp_servers(&config); + assert_eq!(servers.len(), 3, "primary + 2 extra = 3 servers"); + assert_eq!(servers[1].name, "npx"); + assert_eq!(servers[2].name, "npx-2"); + } + + #[test] + fn extra_mcp_commands_shell_split_quoted_paths() { + // Quoted paths with spaces must be preserved as a single argv element. + let mut config = test_config(); + config.extra_mcp_commands = vec![r#""my server" --port 8080"#.into()]; + let servers = build_mcp_servers(&config); + assert_eq!(servers.len(), 2); + assert_eq!(servers[1].command, "my server"); + assert_eq!(servers[1].args, vec!["--port", "8080"]); + } + + #[test] + fn extra_mcp_commands_skip_malformed_quoting() { + let mut config = test_config(); + config.extra_mcp_commands = vec![ + "valid-server".into(), + "'unmatched-quote".into(), + "another-server".into(), + ]; + let servers = build_mcp_servers(&config); + assert_eq!(servers.len(), 3, "primary + 2 valid (malformed skipped)"); + assert_eq!(servers[1].name, "valid-server"); + assert_eq!(servers[2].name, "another-server"); + } } #[cfg(test)] From 072fb99793609b123ec7266f997a00a9ef19b25c Mon Sep 17 00:00:00 2001 From: Brad Groux <3053586+BradGroux@users.noreply.github.com> Date: Tue, 18 Aug 2026 15:01:47 -0500 Subject: [PATCH 03/11] Address P0/P1 review feedback: credential isolation, fail-closed, name sanitization MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four issues from wesbillman/Carl's review: P0 — Extra MCP processes received Buzz identity credentials despite the PR's isolation claim. build_mcp_servers gave extras an empty per-server env, but buzz-agent's spawn_one cleared and repopulated every MCP child's environment from PASSTHROUGH_ENV, which includes BUZZ_PRIVATE_KEY, BUZZ_RELAY_URL, and BUZZ_AUTH_TAG. Added a trusted flag to McpServer and McpServerStdio; spawn_one now withholds identity credentials from untrusted servers. The primary buzz-dev-mcp server is trusted; extras are not. P1 — BUZZ_ACP_EXTRA_MCP_COMMANDS was absent from Desktop's reserved env key list, allowing a portable persona or per-agent env to inject an arbitrary command. Added to reserved_env_keys.rs and its test. P1 — Malformed quoting logged the raw command (which may contain an embedded API key) and silently skipped the entry. Now fails closed with only the entry index; the raw command is never echoed. P1 — Generated names used the executable stem verbatim, violating McpRegistry's ASCII alphanumeric/hyphen and 128-byte contract. Added sanitize_mcp_name to replace non-conforming characters with hyphens, strip leading/trailing hyphens, and truncate to 128 bytes. Co-authored-by: Brad Groux Signed-off-by: Brad Groux Signed-off-by: wiggdevin <202901685+wiggdevin@users.noreply.github.com> --- crates/buzz-acp/README.md | 1 + crates/buzz-acp/src/acp.rs | 12 + crates/buzz-acp/src/config.rs | 18 +- crates/buzz-acp/src/lib.rs | 250 +++++++++++++++--- crates/buzz-acp/src/pool.rs | 1 + crates/buzz-agent/src/mcp.rs | 18 ++ crates/buzz-agent/src/types.rs | 7 + .../src/managed_agents/env_vars/tests.rs | 1 + .../src/managed_agents/reserved_env_keys.rs | 1 + 9 files changed, 259 insertions(+), 50 deletions(-) diff --git a/crates/buzz-acp/README.md b/crates/buzz-acp/README.md index 41d9a214bdd..37d9fb3bb3f 100644 --- a/crates/buzz-acp/README.md +++ b/crates/buzz-acp/README.md @@ -111,6 +111,7 @@ All configuration is via environment variables (or CLI flags — every env var h | `BUZZ_ACP_AGENT_COMMAND` | no | `goose` | Agent binary to spawn. | | `BUZZ_ACP_AGENT_ARGS` | no | `acp` | Agent arguments (comma-separated). | | `BUZZ_ACP_MCP_COMMAND` | no | `""` (empty) | Path to an optional MCP server binary to provide to the agent subprocess. | +| `BUZZ_ACP_EXTRA_MCP_COMMANDS` | no | — | Newline-separated additional MCP server commands. Each entry is shell-split with POSIX quoting (e.g. `npx -y my-mcp-server`). An optional `name=` prefix sets the server name explicitly (e.g. `memory=npx -y memory-mcp`), so reordering entries does not silently rename a server and strip the agent of its tools. Without a prefix, names are derived from the executable stem, sanitized to ASCII alphanumeric/hyphen, and disambiguated with numeric suffixes. Extra servers do **not** receive `BUZZ_PRIVATE_KEY`, `BUZZ_RELAY_URL`, or `BUZZ_AUTH_TAG` on the `buzz-agent` MCP spawn path — they are third-party tools, not Buzz-native MCP. **Note:** the credential isolation applies to MCP servers spawned directly by `buzz-agent`'s `McpRegistry`. When using a third-party ACP adapter (e.g. `claude-agent-acp`, `codex-acp`) that spawns its own MCP children, the adapter process inherits the full parent environment including `BUZZ_PRIVATE_KEY`; operators should assume those children can access Buzz credentials unless the adapter itself isolates them. Malformed quoting fails startup with the entry index (the raw command is not logged). | | `BUZZ_ACP_IDLE_TIMEOUT` | no | `620` | Idle timeout: max seconds of silence before cancelling a turn. Resets on any agent stdout activity. | | `BUZZ_ACP_MAX_TURN_DURATION` | no | `7200` | Absolute wall-clock cap per turn (safety valve). | | `BUZZ_API_TOKEN` | no | — | API token (required if relay enforces token auth). | diff --git a/crates/buzz-acp/src/acp.rs b/crates/buzz-acp/src/acp.rs index 0d87bca028c..994f6486aa2 100644 --- a/crates/buzz-acp/src/acp.rs +++ b/crates/buzz-acp/src/acp.rs @@ -26,12 +26,23 @@ const MAX_LINE_SIZE: usize = 10_000_000; // 10 MB /// /// Corresponds to the `McpServerStdio` variant in the ACP schema. /// All four fields are **required** by the schema (`args` and `env` may be empty arrays). +/// `trusted` controls whether the agent runtime passes Buzz identity credentials +/// (`BUZZ_PRIVATE_KEY`, `BUZZ_RELAY_URL`, `BUZZ_AUTH_TAG`) into the child process. +/// Only the built-in `buzz-dev-mcp` server is trusted; extra MCP servers +/// configured via `BUZZ_ACP_EXTRA_MCP_COMMANDS` are untrusted and receive no +/// Buzz credentials. #[derive(Debug, Clone, serde::Serialize)] pub struct McpServer { pub name: String, pub command: String, pub args: Vec, pub env: Vec, + #[serde(default, skip_serializing_if = "is_false")] + pub trusted: bool, +} + +fn is_false(b: &bool) -> bool { + !b } /// A single environment variable for an MCP server. @@ -2538,6 +2549,7 @@ mod tests { value: "nsec1abc".into(), }, ], + trusted: true, }; let serialized = serde_json::to_value(&server).unwrap(); assert_eq!(serialized["name"].as_str(), Some("test-mcp")); diff --git a/crates/buzz-acp/src/config.rs b/crates/buzz-acp/src/config.rs index 18c728153cb..f365000a630 100644 --- a/crates/buzz-acp/src/config.rs +++ b/crates/buzz-acp/src/config.rs @@ -268,14 +268,18 @@ pub struct CliArgs { pub mcp_command: String, /// Additional MCP server commands to pass to the agent session alongside - /// the primary MCP server. Entries are comma-separated; each entry is + /// the primary MCP server. Entries are newline-separated; each entry is /// shell-split (shlex) into a command and its args, so quoted paths and - /// arguments with spaces are preserved. Server names are derived from the - /// executable stem and disambiguated with a numeric suffix if duplicates - /// occur (e.g. two `npx` wrappers become `npx` and `npx-2`). Entries with - /// malformed quoting are skipped with a warning. Example: - /// `npx -y mcp-remote https://mcp.tavily.com/mcp/?tavilyApiKey=...,other-server` - #[arg(long, env = "BUZZ_ACP_EXTRA_MCP_COMMANDS", value_delimiter = ',')] + /// arguments with spaces are preserved. An optional `name=` prefix sets + /// the server name explicitly (e.g. `memory=npx -y memory-mcp`); without + /// it, the name is derived from the executable stem. Duplicate names are + /// disambiguated with a numeric suffix (e.g. two `npx` wrappers become + /// `npx` and `npx-2`), but explicit names are preferred so reordering + /// entries does not silently rename a server. Entries with malformed + /// quoting fail startup with the entry index (the raw command is not + /// echoed). Example: + /// `memory=npx -y mcp-remote https://mcp.tavily.com/mcp/?tavilyApiKey=...\nother-server --port 8080` + #[arg(long, env = "BUZZ_ACP_EXTRA_MCP_COMMANDS", value_delimiter = '\n')] pub extra_mcp_commands: Vec, /// Idle timeout: max seconds of silence before killing a turn. diff --git a/crates/buzz-acp/src/lib.rs b/crates/buzz-acp/src/lib.rs index 1ab6ac93892..bdfb0ff86c9 100644 --- a/crates/buzz-acp/src/lib.rs +++ b/crates/buzz-acp/src/lib.rs @@ -33,7 +33,7 @@ use buzz_core::observer::{ }; use clap::Parser; use config::{ - AuthAgentArgs, AuthMethodsArgs, AuthenticateArgs, Config, DedupMode, ModelsArgs, + AuthAgentArgs, AuthMethodsArgs, AuthenticateArgs, Config, ConfigError, DedupMode, ModelsArgs, MultipleEventHandling, RespondTo, SubscribeMode, }; use filter::SubscriptionRule; @@ -2761,7 +2761,7 @@ async fn tokio_main() -> Result<()> { let base_prompt_content = config.base_prompt_content.take(); let cwd = current_working_directory()?; let ctx = Arc::new(PromptContext { - mcp_servers: build_mcp_servers(&config), + mcp_servers: build_mcp_servers(&config)?, initial_message: config.initial_message.clone(), idle_timeout: Duration::from_secs(config.idle_timeout_secs), max_turn_duration: Duration::from_secs(config.max_turn_duration_secs), @@ -5727,9 +5727,9 @@ async fn run_models(args: ModelsArgs) -> Result<()> { Ok(()) } -fn build_mcp_servers(config: &Config) -> Vec { +fn build_mcp_servers(config: &Config) -> Result, ConfigError> { if config.mcp_command.is_empty() { - return vec![]; + return Ok(vec![]); } let mut servers = vec![McpServer { name: std::path::Path::new(&config.mcp_command) @@ -5781,42 +5781,76 @@ fn build_mcp_servers(config: &Config) -> Vec { } env }, + trusted: true, }]; // Append extra MCP servers from BUZZ_ACP_EXTRA_MCP_COMMANDS. - // Each entry is shell-split into command + args using shlex, so quoted - // paths and arguments with spaces are preserved. Malformed entries are - // preserved as-is with a warning. + // Each entry is newline-separated and shell-split into command + args + // using shlex, so quoted paths and arguments with spaces (and commas) + // are preserved. An optional `name=` prefix sets the server name + // explicitly, so reordering entries does not silently rename a server + // and strip the agent of its tools. Malformed entries cause startup to + // fail closed — the error identifies the entry index without echoing + // the command, which may contain an embedded API key. // Extra servers do not receive Buzz relay credentials or auth tags — // they are third-party tools, not Buzz-native MCP servers. let mut seen_names: std::collections::HashSet = std::collections::HashSet::from_iter([servers[0].name.clone()]); - for extra in &config.extra_mcp_commands { + for (idx, extra) in config.extra_mcp_commands.iter().enumerate() { let trimmed = extra.trim(); if trimmed.is_empty() { continue; } - let parts = match shlex::split(trimmed) { + // Parse optional `name=command` prefix. The name must be a simple + // identifier (ASCII alphanumeric + hyphen); the `=` split is on the + // first occurrence, so commands containing `=` (e.g. URLs with query + // params) are not affected when no valid name prefix is present. + let (explicit_name, command_str) = match trimmed.find('=') { + Some(pos) => { + let candidate = &trimmed[..pos]; + // Only treat as a name if it's a valid identifier and not a + // path (no slashes) — otherwise it's a command that happens + // to contain `=` (e.g. a URL with query params). + if !candidate.is_empty() + && candidate + .chars() + .all(|c| c.is_ascii_alphanumeric() || c == '-') + && !candidate.contains('/') + { + (Some(candidate.to_string()), trimmed[pos + 1..].trim().to_string()) + } else { + (None, trimmed.to_string()) + } + } + None => (None, trimmed.to_string()), + }; + let parts = match shlex::split(&command_str) { Some(p) if !p.is_empty() => p, Some(_) => continue, None => { - tracing::warn!( - entry = %trimmed, - "BUZZ_ACP_EXTRA_MCP_COMMANDS entry has malformed shell quoting; skipping" - ); - continue; + return Err(ConfigError::ConfigFile(format!( + "BUZZ_ACP_EXTRA_MCP_COMMANDS entry {} has malformed shell quoting; \ + fix the quoting or remove the entry and restart", + idx + 1 + ))); } }; let command = parts[0].clone(); let args: Vec = parts[1..].to_vec(); - // Derive a name from the executable stem, then disambiguate so - // two wrappers like `npx -y first-mcp` and `npx -y second-mcp` - // don't both become `npx` and trip McpRegistry's duplicate check. - let base_name = std::path::Path::new(&command) - .file_stem() - .and_then(|s| s.to_str()) - .unwrap_or("extra-mcp") - .to_string(); + // Use the explicit name if provided, otherwise derive from the + // executable stem. Then disambiguate so two wrappers like + // `npx -y first-mcp` and `npx -y second-mcp` don't both become + // `npx` and trip McpRegistry's duplicate check. + let base_name = match &explicit_name { + Some(n) => sanitize_mcp_name(n), + None => { + let raw_stem = std::path::Path::new(&command) + .file_stem() + .and_then(|s| s.to_str()) + .unwrap_or("extra-mcp"); + sanitize_mcp_name(raw_stem) + } + }; let name = if seen_names.contains(&base_name) { // Append a numeric suffix until we find a unique name. let mut i = 2; @@ -5836,10 +5870,40 @@ fn build_mcp_servers(config: &Config) -> Vec { command, args, env: vec![], + trusted: false, }); } - servers + Ok(servers) +} + +/// Sanitize a raw executable stem into a name that satisfies the downstream +/// `McpRegistry` validator: ASCII alphanumeric and hyphens only, ≤128 bytes. +/// Non-conforming characters are replaced with hyphens; leading/trailing +/// hyphens are stripped. An empty result falls back to `"extra-mcp"`. +fn sanitize_mcp_name(raw: &str) -> String { + let sanitized: String = raw + .chars() + .map(|c| { + if c.is_ascii_alphanumeric() { + c + } else { + '-' + } + }) + .collect::() + .trim_matches('-') + .to_string(); + let truncated = if sanitized.len() > 128 { + sanitized[..=128].to_string() + } else { + sanitized + }; + if truncated.is_empty() { + "extra-mcp".to_string() + } else { + truncated + } } #[cfg(test)] @@ -9002,7 +9066,7 @@ mod build_mcp_servers_tests { #[test] fn session_new_mcp_server_has_required_fields() { let config = test_config(); - let servers = build_mcp_servers(&config); + let servers = build_mcp_servers(&config).unwrap(); assert_eq!(servers.len(), 1); let server = &servers[0]; assert_eq!(server.name, "test-mcp-server"); @@ -9023,7 +9087,7 @@ mod build_mcp_servers_tests { let _guard = ENV_LOCK.lock().unwrap(); std::env::set_var("BUZZ_AUTH_TAG", "test-attestation-tag"); let config = test_config(); - let servers = build_mcp_servers(&config); + let servers = build_mcp_servers(&config).unwrap(); std::env::remove_var("BUZZ_AUTH_TAG"); let server = &servers[0]; @@ -9040,7 +9104,7 @@ mod build_mcp_servers_tests { let _guard = ENV_LOCK.lock().unwrap(); std::env::set_var("BUZZ_AUTH_TAG", ""); let config = test_config(); - let servers = build_mcp_servers(&config); + let servers = build_mcp_servers(&config).unwrap(); std::env::remove_var("BUZZ_AUTH_TAG"); let server = &servers[0]; @@ -9053,7 +9117,7 @@ mod build_mcp_servers_tests { let _guard = ENV_LOCK.lock().unwrap(); std::env::set_var("BUZZ_ACP_DISPLAY_NAME", "Duncan"); let config = test_config(); - let servers = build_mcp_servers(&config); + let servers = build_mcp_servers(&config).unwrap(); std::env::remove_var("BUZZ_ACP_DISPLAY_NAME"); let entry = servers[0] @@ -9072,7 +9136,7 @@ mod build_mcp_servers_tests { let _guard = ENV_LOCK.lock().unwrap(); std::env::remove_var("BUZZ_ACP_DISPLAY_NAME"); let config = test_config(); - let servers = build_mcp_servers(&config); + let servers = build_mcp_servers(&config).unwrap(); // Absent, not empty-valued: dev-mcp distinguishes the two and only // falls back to the npub when the key is missing or blank. @@ -9090,7 +9154,7 @@ mod build_mcp_servers_tests { let _guard = ENV_LOCK.lock().unwrap(); std::env::set_var("BUZZ_ACP_DISPLAY_NAME", ""); let config = test_config(); - let servers = build_mcp_servers(&config); + let servers = build_mcp_servers(&config).unwrap(); std::env::remove_var("BUZZ_ACP_DISPLAY_NAME"); assert!( @@ -9106,7 +9170,7 @@ mod build_mcp_servers_tests { fn empty_mcp_command_returns_no_servers() { let mut config = test_config(); config.mcp_command = "".into(); - let servers = build_mcp_servers(&config); + let servers = build_mcp_servers(&config).unwrap(); assert!( servers.is_empty(), "empty mcp_command should produce no MCP servers" @@ -9117,7 +9181,7 @@ mod build_mcp_servers_tests { fn absolute_path_mcp_command_uses_file_stem_as_name() { let mut config = test_config(); config.mcp_command = "/opt/bin/my-mcp-server".into(); - let servers = build_mcp_servers(&config); + let servers = build_mcp_servers(&config).unwrap(); assert_eq!(servers.len(), 1); assert_eq!(servers[0].name, "my-mcp-server"); } @@ -9138,7 +9202,7 @@ mod build_mcp_servers_tests { // Confirm a non-empty command with no stem (e.g. just a dot) also falls back. config.mcp_command = ".".into(); - let servers = build_mcp_servers(&config); + let servers = build_mcp_servers(&config).unwrap(); assert_eq!(servers.len(), 1); assert_eq!( servers[0].name, "mcp", @@ -9151,7 +9215,7 @@ mod build_mcp_servers_tests { let mut config = test_config(); config.extra_mcp_commands = vec!["npx -y mcp-remote https://mcp.tavily.com/mcp/?tavilyApiKey=test-key".into()]; - let servers = build_mcp_servers(&config); + let servers = build_mcp_servers(&config).unwrap(); assert_eq!(servers.len(), 2, "primary + 1 extra = 2 servers"); assert_eq!(servers[0].name, "test-mcp-server"); assert_eq!(servers[1].name, "npx"); @@ -9183,7 +9247,7 @@ mod build_mcp_servers_tests { "brave-search-mcp".into(), "npx -y mcp-remote https://mcp.tavily.com/mcp/".into(), ]; - let servers = build_mcp_servers(&config); + let servers = build_mcp_servers(&config).unwrap(); assert_eq!(servers.len(), 3, "primary + 2 extra = 3 servers"); assert_eq!(servers[0].name, "test-mcp-server"); assert_eq!(servers[1].name, "brave-search-mcp"); @@ -9199,7 +9263,7 @@ mod build_mcp_servers_tests { " ".into(), "another-server arg1".into(), ]; - let servers = build_mcp_servers(&config); + let servers = build_mcp_servers(&config).unwrap(); assert_eq!( servers.len(), 3, @@ -9217,7 +9281,7 @@ mod build_mcp_servers_tests { let mut config = test_config(); config.mcp_command = "".into(); config.extra_mcp_commands = vec!["some-extra-server".into()]; - let servers = build_mcp_servers(&config); + let servers = build_mcp_servers(&config).unwrap(); assert!( servers.is_empty(), "empty primary mcp_command should still short-circuit even with extras" @@ -9230,7 +9294,7 @@ mod build_mcp_servers_tests { // trip McpRegistry's duplicate-name check at spawn. let mut config = test_config(); config.extra_mcp_commands = vec!["npx -y first-mcp".into(), "npx -y second-mcp".into()]; - let servers = build_mcp_servers(&config); + let servers = build_mcp_servers(&config).unwrap(); assert_eq!(servers.len(), 3, "primary + 2 extra = 3 servers"); assert_eq!(servers[1].name, "npx"); assert_eq!(servers[2].name, "npx-2"); @@ -9241,24 +9305,124 @@ mod build_mcp_servers_tests { // Quoted paths with spaces must be preserved as a single argv element. let mut config = test_config(); config.extra_mcp_commands = vec![r#""my server" --port 8080"#.into()]; - let servers = build_mcp_servers(&config); + let servers = build_mcp_servers(&config).unwrap(); assert_eq!(servers.len(), 2); assert_eq!(servers[1].command, "my server"); assert_eq!(servers[1].args, vec!["--port", "8080"]); } #[test] - fn extra_mcp_commands_skip_malformed_quoting() { + fn extra_mcp_commands_fail_closed_on_malformed_quoting() { + // Malformed quoting must fail startup, not silently skip the entry. + // The error must not echo the command (it may contain an API key). let mut config = test_config(); config.extra_mcp_commands = vec![ "valid-server".into(), "'unmatched-quote".into(), "another-server".into(), ]; - let servers = build_mcp_servers(&config); - assert_eq!(servers.len(), 3, "primary + 2 valid (malformed skipped)"); - assert_eq!(servers[1].name, "valid-server"); - assert_eq!(servers[2].name, "another-server"); + let result = build_mcp_servers(&config); + assert!(result.is_err(), "malformed quoting must fail closed"); + let err_msg = format!("{}", result.unwrap_err()); + assert!( + err_msg.contains("entry 2"), + "error should identify the entry index" + ); + assert!( + !err_msg.contains("unmatched-quote"), + "error must not echo the raw command" + ); + } + + #[test] + fn extra_mcp_commands_sanitized_names() { + // Names with underscores, spaces, or punctuation must be sanitized + // to the McpRegistry ASCII alphanumeric/hyphen contract. + let mut config = test_config(); + config.extra_mcp_commands = vec!["my_server --port 8080".into()]; + let servers = build_mcp_servers(&config).unwrap(); + assert_eq!(servers.len(), 2); + assert_eq!(servers[1].name, "my-server"); + } + + #[test] + fn extra_mcp_commands_trusted_flag() { + // The primary server must be trusted; extras must not be. + let mut config = test_config(); + config.extra_mcp_commands = vec!["some-extra-server".into()]; + let servers = build_mcp_servers(&config).unwrap(); + assert_eq!(servers.len(), 2); + assert!(servers[0].trusted, "primary MCP server must be trusted"); + assert!(!servers[1].trusted, "extra MCP servers must not be trusted"); + } + + #[test] + fn extra_mcp_commands_explicit_name_prefix() { + // The `name=command` syntax sets the server name explicitly so + // reordering entries does not silently rename a server. + let mut config = test_config(); + config.extra_mcp_commands = vec!["memory=npx -y memory-mcp".into()]; + let servers = build_mcp_servers(&config).unwrap(); + assert_eq!(servers.len(), 2); + assert_eq!(servers[1].name, "memory"); + assert_eq!(servers[1].command, "npx"); + assert_eq!(servers[1].args, vec!["-y", "memory-mcp"]); + } + + #[test] + fn extra_mcp_commands_explicit_name_stable_on_reorder() { + // Two npx-based servers with explicit names keep their names + // regardless of entry order. + let mut config = test_config(); + config.extra_mcp_commands = vec![ + "alpha=npx -y first-mcp".into(), + "beta=npx -y second-mcp".into(), + ]; + let servers = build_mcp_servers(&config).unwrap(); + assert_eq!(servers.len(), 3); + assert_eq!(servers[1].name, "alpha"); + assert_eq!(servers[2].name, "beta"); + + // Reorder — names should still be alpha and beta, not npx and npx-2. + config.extra_mcp_commands = vec![ + "beta=npx -y second-mcp".into(), + "alpha=npx -y first-mcp".into(), + ]; + let servers = build_mcp_servers(&config).unwrap(); + assert_eq!(servers.len(), 3); + assert_eq!(servers[1].name, "beta"); + assert_eq!(servers[2].name, "alpha"); + } + + #[test] + fn extra_mcp_commands_comma_in_args_preserved() { + // With newline separation, commas inside arguments survive shlex + // splitting — the case rsaulo identified where comma-delimiter + // parsing would break `--filter 'a,b'`. + let mut config = test_config(); + config.extra_mcp_commands = vec!["npx -y srv --filter a,b".into()]; + let servers = build_mcp_servers(&config).unwrap(); + assert_eq!(servers.len(), 2); + assert_eq!(servers[1].command, "npx"); + assert_eq!( + servers[1].args, + vec!["-y", "srv", "--filter", "a,b"], + "comma inside an argument must survive — no pre-split on comma" + ); + } + + #[test] + fn extra_mcp_commands_url_with_equals_not_treated_as_name() { + // A command containing `=` in a URL query param must not be + // misinterpreted as a `name=command` prefix. + let mut config = test_config(); + config.extra_mcp_commands = + vec!["npx -y mcp-remote https://mcp.tavily.com/mcp/?tavilyApiKey=test-key".into()]; + let servers = build_mcp_servers(&config).unwrap(); + assert_eq!(servers.len(), 2); + // Name should be derived from the executable stem, not "npx -y mcp-remote https://mcp.tavily.com/mcp/?tavilyApiKey" + assert_eq!(servers[1].name, "npx"); + assert_eq!(servers[1].command, "npx"); } } diff --git a/crates/buzz-acp/src/pool.rs b/crates/buzz-acp/src/pool.rs index c73bd56031f..6190f2234d6 100644 --- a/crates/buzz-acp/src/pool.rs +++ b/crates/buzz-acp/src/pool.rs @@ -5151,6 +5151,7 @@ mod tests { command: "buzz-dev-mcp".into(), args: vec![], env: vec![], + trusted: true, } } diff --git a/crates/buzz-agent/src/mcp.rs b/crates/buzz-agent/src/mcp.rs index a848557ae2f..af46c3b2108 100644 --- a/crates/buzz-agent/src/mcp.rs +++ b/crates/buzz-agent/src/mcp.rs @@ -122,6 +122,7 @@ struct ServerSpec { args: Vec, env: Vec<(String, String)>, cwd: String, + trusted: bool, } enum ClientState { @@ -246,6 +247,7 @@ impl McpRegistry { .map(|e| (e.name.clone(), e.value.clone())) .collect(), cwd: cwd.to_owned(), + trusted: s.trusted, }; let (client, pgid, tool_names, raw_tools) = spawn_one(&spec, reg.init_timeout).await?; let server_idx = reg.servers.len(); @@ -738,6 +740,13 @@ async fn spawn_one( cmd.args(&spec.args); cmd.env_clear(); for k in PASSTHROUGH_ENV { + // Withhold Buzz identity credentials from untrusted MCP servers so + // third-party tooling cannot exfiltrate the agent's signing key, + // relay URL, or owner attestation. Only the built-in buzz-dev-mcp + // server (marked `trusted`) receives these. + if !spec.trusted && is_buzz_identity_env(k) { + continue; + } if let Ok(v) = std::env::var(k) { cmd.env(k, v); } @@ -911,6 +920,15 @@ fn valid_name(s: &str) -> bool { .all(|b| b.is_ascii_alphanumeric() || b == b'_' || b == b'-') } +/// Returns `true` for env vars that carry Buzz identity credentials. +/// These are withheld from untrusted (third-party) MCP server children. +fn is_buzz_identity_env(key: &str) -> bool { + matches!( + key, + "BUZZ_PRIVATE_KEY" | "NOSTR_PRIVATE_KEY" | "BUZZ_RELAY_URL" | "BUZZ_AUTH_TAG" + ) +} + pub(crate) fn truncate_at_boundary(s: &str, max: usize) -> &str { if s.len() <= max { return s; diff --git a/crates/buzz-agent/src/types.rs b/crates/buzz-agent/src/types.rs index 10ac65b46ef..d2edb6bf11c 100644 --- a/crates/buzz-agent/src/types.rs +++ b/crates/buzz-agent/src/types.rs @@ -541,6 +541,13 @@ pub struct McpServerStdio { pub args: Vec, #[serde(default)] pub env: Vec, + /// When `false`, the spawn boundary withholds Buzz identity credentials + /// (`BUZZ_PRIVATE_KEY`, `BUZZ_RELAY_URL`, `BUZZ_AUTH_TAG`) from the child + /// process so third-party MCP servers cannot exfiltrate the agent's + /// signing key or owner attestation. Only the built-in `buzz-dev-mcp` + /// server sets this to `true`. + #[serde(default)] + pub trusted: bool, } #[derive(Debug, Deserialize, Clone)] diff --git a/desktop/src-tauri/src/managed_agents/env_vars/tests.rs b/desktop/src-tauri/src/managed_agents/env_vars/tests.rs index dc38c3d126f..c03b9ddbf5f 100644 --- a/desktop/src-tauri/src/managed_agents/env_vars/tests.rs +++ b/desktop/src-tauri/src/managed_agents/env_vars/tests.rs @@ -190,6 +190,7 @@ fn reserved_keys_include_code_execution_surface() { "BUZZ_ACP_AGENT_COMMAND", "BUZZ_ACP_AGENT_ARGS", "BUZZ_ACP_MCP_COMMAND", + "BUZZ_ACP_EXTRA_MCP_COMMANDS", ] { assert!(is_reserved_env_key(key), "{key} should be reserved"); } diff --git a/desktop/src-tauri/src/managed_agents/reserved_env_keys.rs b/desktop/src-tauri/src/managed_agents/reserved_env_keys.rs index c01d29f3c2a..492791aa1b5 100644 --- a/desktop/src-tauri/src/managed_agents/reserved_env_keys.rs +++ b/desktop/src-tauri/src/managed_agents/reserved_env_keys.rs @@ -41,6 +41,7 @@ pub(crate) const RESERVED_ENV_KEYS: &[&str] = &[ "BUZZ_ACP_AGENT_COMMAND", "BUZZ_ACP_AGENT_ARGS", "BUZZ_ACP_MCP_COMMAND", + "BUZZ_ACP_EXTRA_MCP_COMMANDS", // Control-plane parallelism: the Desktop resolves the effective // worker-pool size (applying any per-harness cap) and writes it into // launch.policy_env. A user-supplied BUZZ_ACP_AGENTS would bypass the From ac597bd63790ebf60b7b0022393a147eb8a167fc Mon Sep 17 00:00:00 2001 From: wiggdevin <202901685+wiggdevin@users.noreply.github.com> Date: Fri, 4 Sep 2026 01:50:56 -0700 Subject: [PATCH 04/11] style(buzz-acp): rustfmt the imported PR #6651 extra-MCP parser The upstream branch predates this repo's rustfmt run, so `just fmt-check` failed on the cherry-picked `build_mcp_servers` and `sanitize_mcp_name` bodies. No behavior change. Signed-off-by: wiggdevin <202901685+wiggdevin@users.noreply.github.com> --- crates/buzz-acp/src/lib.rs | 13 +++++-------- 1 file changed, 5 insertions(+), 8 deletions(-) diff --git a/crates/buzz-acp/src/lib.rs b/crates/buzz-acp/src/lib.rs index bdfb0ff86c9..4f5a02bc795 100644 --- a/crates/buzz-acp/src/lib.rs +++ b/crates/buzz-acp/src/lib.rs @@ -5817,7 +5817,10 @@ fn build_mcp_servers(config: &Config) -> Result, ConfigError> { .all(|c| c.is_ascii_alphanumeric() || c == '-') && !candidate.contains('/') { - (Some(candidate.to_string()), trimmed[pos + 1..].trim().to_string()) + ( + Some(candidate.to_string()), + trimmed[pos + 1..].trim().to_string(), + ) } else { (None, trimmed.to_string()) } @@ -5884,13 +5887,7 @@ fn build_mcp_servers(config: &Config) -> Result, ConfigError> { fn sanitize_mcp_name(raw: &str) -> String { let sanitized: String = raw .chars() - .map(|c| { - if c.is_ascii_alphanumeric() { - c - } else { - '-' - } - }) + .map(|c| if c.is_ascii_alphanumeric() { c } else { '-' }) .collect::() .trim_matches('-') .to_string(); From aa83130550e377f7ea02a49e129fe3d6b16a7b65 Mon Sep 17 00:00:00 2001 From: wiggdevin <202901685+wiggdevin@users.noreply.github.com> Date: Fri, 4 Sep 2026 01:51:03 -0700 Subject: [PATCH 05/11] docs(mcp): name NOSTR_PRIVATE_KEY in the untrusted-server credential lists `is_buzz_identity_env` withholds four variables from an untrusted MCP server, but the doc comments and the buzz-acp README named only three, so a reader could conclude NOSTR_PRIVATE_KEY still reaches a third-party server. Also corrects the McpServer schema note, which said all four fields are required and now has a fifth, optional one. Signed-off-by: wiggdevin <202901685+wiggdevin@users.noreply.github.com> --- crates/buzz-acp/README.md | 2 +- crates/buzz-acp/src/acp.rs | 7 +++++-- crates/buzz-agent/src/types.rs | 3 ++- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/crates/buzz-acp/README.md b/crates/buzz-acp/README.md index 37d9fb3bb3f..97ec1c0aac3 100644 --- a/crates/buzz-acp/README.md +++ b/crates/buzz-acp/README.md @@ -111,7 +111,7 @@ All configuration is via environment variables (or CLI flags — every env var h | `BUZZ_ACP_AGENT_COMMAND` | no | `goose` | Agent binary to spawn. | | `BUZZ_ACP_AGENT_ARGS` | no | `acp` | Agent arguments (comma-separated). | | `BUZZ_ACP_MCP_COMMAND` | no | `""` (empty) | Path to an optional MCP server binary to provide to the agent subprocess. | -| `BUZZ_ACP_EXTRA_MCP_COMMANDS` | no | — | Newline-separated additional MCP server commands. Each entry is shell-split with POSIX quoting (e.g. `npx -y my-mcp-server`). An optional `name=` prefix sets the server name explicitly (e.g. `memory=npx -y memory-mcp`), so reordering entries does not silently rename a server and strip the agent of its tools. Without a prefix, names are derived from the executable stem, sanitized to ASCII alphanumeric/hyphen, and disambiguated with numeric suffixes. Extra servers do **not** receive `BUZZ_PRIVATE_KEY`, `BUZZ_RELAY_URL`, or `BUZZ_AUTH_TAG` on the `buzz-agent` MCP spawn path — they are third-party tools, not Buzz-native MCP. **Note:** the credential isolation applies to MCP servers spawned directly by `buzz-agent`'s `McpRegistry`. When using a third-party ACP adapter (e.g. `claude-agent-acp`, `codex-acp`) that spawns its own MCP children, the adapter process inherits the full parent environment including `BUZZ_PRIVATE_KEY`; operators should assume those children can access Buzz credentials unless the adapter itself isolates them. Malformed quoting fails startup with the entry index (the raw command is not logged). | +| `BUZZ_ACP_EXTRA_MCP_COMMANDS` | no | — | Newline-separated additional MCP server commands. Each entry is shell-split with POSIX quoting (e.g. `npx -y my-mcp-server`). An optional `name=` prefix sets the server name explicitly (e.g. `memory=npx -y memory-mcp`), so reordering entries does not silently rename a server and strip the agent of its tools. Without a prefix, names are derived from the executable stem, sanitized to ASCII alphanumeric/hyphen, and disambiguated with numeric suffixes. Extra servers do **not** receive `BUZZ_PRIVATE_KEY`, `NOSTR_PRIVATE_KEY`, `BUZZ_RELAY_URL`, or `BUZZ_AUTH_TAG` on the `buzz-agent` MCP spawn path — they are third-party tools, not Buzz-native MCP. **Note:** the credential isolation applies to MCP servers spawned directly by `buzz-agent`'s `McpRegistry`. When using a third-party ACP adapter (e.g. `claude-agent-acp`, `codex-acp`) that spawns its own MCP children, the adapter process inherits the full parent environment including `BUZZ_PRIVATE_KEY`; operators should assume those children can access Buzz credentials unless the adapter itself isolates them. Malformed quoting fails startup with the entry index (the raw command is not logged). | | `BUZZ_ACP_IDLE_TIMEOUT` | no | `620` | Idle timeout: max seconds of silence before cancelling a turn. Resets on any agent stdout activity. | | `BUZZ_ACP_MAX_TURN_DURATION` | no | `7200` | Absolute wall-clock cap per turn (safety valve). | | `BUZZ_API_TOKEN` | no | — | API token (required if relay enforces token auth). | diff --git a/crates/buzz-acp/src/acp.rs b/crates/buzz-acp/src/acp.rs index 994f6486aa2..ec0457d6993 100644 --- a/crates/buzz-acp/src/acp.rs +++ b/crates/buzz-acp/src/acp.rs @@ -25,9 +25,12 @@ const MAX_LINE_SIZE: usize = 10_000_000; // 10 MB /// An MCP server configuration passed to `session/new`. /// /// Corresponds to the `McpServerStdio` variant in the ACP schema. -/// All four fields are **required** by the schema (`args` and `env` may be empty arrays). +/// `name`, `command`, `args` and `env` are **required** by the schema (`args` +/// and `env` may be empty arrays); `trusted` is a Buzz extension and defaults +/// to `false` when absent. /// `trusted` controls whether the agent runtime passes Buzz identity credentials -/// (`BUZZ_PRIVATE_KEY`, `BUZZ_RELAY_URL`, `BUZZ_AUTH_TAG`) into the child process. +/// (`BUZZ_PRIVATE_KEY`, `NOSTR_PRIVATE_KEY`, `BUZZ_RELAY_URL`, `BUZZ_AUTH_TAG`) +/// into the child process. /// Only the built-in `buzz-dev-mcp` server is trusted; extra MCP servers /// configured via `BUZZ_ACP_EXTRA_MCP_COMMANDS` are untrusted and receive no /// Buzz credentials. diff --git a/crates/buzz-agent/src/types.rs b/crates/buzz-agent/src/types.rs index d2edb6bf11c..337d6e32a3e 100644 --- a/crates/buzz-agent/src/types.rs +++ b/crates/buzz-agent/src/types.rs @@ -542,7 +542,8 @@ pub struct McpServerStdio { #[serde(default)] pub env: Vec, /// When `false`, the spawn boundary withholds Buzz identity credentials - /// (`BUZZ_PRIVATE_KEY`, `BUZZ_RELAY_URL`, `BUZZ_AUTH_TAG`) from the child + /// (`BUZZ_PRIVATE_KEY`, `NOSTR_PRIVATE_KEY`, `BUZZ_RELAY_URL`, + /// `BUZZ_AUTH_TAG`) from the child /// process so third-party MCP servers cannot exfiltrate the agent's /// signing key or owner attestation. Only the built-in `buzz-dev-mcp` /// server sets this to `true`. From 1566240e70a02e9e67f7d826768eb91b75c88d4d Mon Sep 17 00:00:00 2001 From: wiggdevin <202901685+wiggdevin@users.noreply.github.com> Date: Fri, 4 Sep 2026 01:51:15 -0700 Subject: [PATCH 06/11] test(buzz-agent): bind the untrusted MCP spawn boundary end to end MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Adds FAKE_MCP_ENV_REPORT to the fake MCP server: tools/call returns the sorted names (never the values) of the variables the server process was spawned with. Two integration tests drive a real buzz-agent child through session/new and a tool call: - an untrusted server receives none of BUZZ_PRIVATE_KEY, NOSTR_PRIVATE_KEY, BUZZ_RELAY_URL or BUZZ_AUTH_TAG, while a trusted one in the same session receives all four — so the withholding half cannot pass merely because the parent never had them; - the filter stays narrow: an untrusted server keeps PATH, HOME, BUZZ_ACP_DISPLAY_NAME and its wire-declared env. Removing the `!spec.trusted && is_buzz_identity_env(k)` guard fails the first test. Signed-off-by: wiggdevin <202901685+wiggdevin@users.noreply.github.com> --- crates/buzz-agent/tests/bin/fake_mcp.rs | 22 ++ crates/buzz-agent/tests/untrusted_mcp_env.rs | 205 +++++++++++++++++++ 2 files changed, 227 insertions(+) create mode 100644 crates/buzz-agent/tests/untrusted_mcp_env.rs diff --git a/crates/buzz-agent/tests/bin/fake_mcp.rs b/crates/buzz-agent/tests/bin/fake_mcp.rs index 8d96779bbac..bfa315d10db 100644 --- a/crates/buzz-agent/tests/bin/fake_mcp.rs +++ b/crates/buzz-agent/tests/bin/fake_mcp.rs @@ -45,6 +45,11 @@ //! `command` string. Lets a test drive the //! reply guard's recognition of a real, //! registered shell tool. +//! FAKE_MCP_ENV_REPORT=1 — `tools/call` returns the sorted names of the +//! environment variables this process was spawned +//! with, one per line, as its text result. Names +//! only, never values, so a test can assert on the +//! spawn boundary without echoing a secret. //! FAKE_MCP_NAMED_TOOLS=a,b — expose one no-arg tool per comma-separated bare //! name (each registered as `__`), in //! addition to any `FAKE_MCP_TOOL_COUNT` tools. Lets @@ -174,6 +179,7 @@ fn main() { let mut stop_calls_seen: usize = 0; let post_compact_hook = env_flag("FAKE_MCP_POSTCOMPACT_HOOK"); let shell_tool = env_flag("FAKE_MCP_SHELL_TOOL"); + let env_report = env_flag("FAKE_MCP_ENV_REPORT"); let post_compact_text = std::env::var("FAKE_MCP_POSTCOMPACT_TEXT").unwrap_or_default(); // One extra no-arg tool per comma-separated bare name. let named_tools: Vec = std::env::var("FAKE_MCP_NAMED_TOOLS") @@ -336,6 +342,22 @@ fn main() { ); continue; } + // Report the environment this server was spawned with, so a + // test can prove the spawn boundary withheld (or kept) a + // specific variable. Names only — a value is never echoed, + // so a leaking secret cannot end up in test output. + if env_report { + let mut names: Vec = std::env::vars().map(|(k, _)| k).collect(); + names.sort(); + write_response( + id, + json!({ + "content": [{ "type": "text", "text": names.join("\n") }], + "isError": false, + }), + ); + continue; + } if tool_delay_secs > 0 { std::thread::sleep(std::time::Duration::from_secs(tool_delay_secs)); } diff --git a/crates/buzz-agent/tests/untrusted_mcp_env.rs b/crates/buzz-agent/tests/untrusted_mcp_env.rs new file mode 100644 index 00000000000..4f1cefcd6c6 --- /dev/null +++ b/crates/buzz-agent/tests/untrusted_mcp_env.rs @@ -0,0 +1,205 @@ +//! Spawn-boundary tests for the `trusted` flag on MCP servers. +//! +//! `BUZZ_ACP_EXTRA_MCP_COMMANDS` lets an operator attach third-party MCP +//! servers to an agent session. Those servers are marked untrusted, and +//! `mcp::spawn_one` must withhold the four Buzz identity variables +//! (`BUZZ_PRIVATE_KEY`, `NOSTR_PRIVATE_KEY`, `BUZZ_RELAY_URL`, +//! `BUZZ_AUTH_TAG`) from their process environment, while the built-in +//! `buzz-dev-mcp` server — the only one marked trusted — still receives them. +//! +//! These tests bind the production seam end to end: a real `buzz-agent` +//! child, a real `session/new` with `mcpServers` off the wire, a real MCP +//! subprocess, and the tool result the agent feeds back to the LLM. The fake +//! server reports the *names* of the variables it was spawned with, never +//! their values, so a leak cannot reach the test output. + +mod common; + +use serde_json::{json, Value}; + +use common::{openai_text, openai_tool_call, spawn_capturing_llm, Harness}; + +/// The four identity variables the spawn boundary withholds from untrusted +/// MCP servers. +const IDENTITY_VARS: &[&str] = &[ + "BUZZ_PRIVATE_KEY", + "NOSTR_PRIVATE_KEY", + "BUZZ_RELAY_URL", + "BUZZ_AUTH_TAG", +]; + +/// Values handed to the agent process so the passthrough allowlist has +/// something to pass. They are never asserted on — only the variable names +/// travel back through the tool result. +fn identity_env() -> Vec<(&'static str, &'static str)> { + vec![ + ("BUZZ_PRIVATE_KEY", "nsec-test-private-key"), + ("NOSTR_PRIVATE_KEY", "nsec-test-nostr-key"), + ("BUZZ_RELAY_URL", "wss://relay.test.invalid"), + ("BUZZ_AUTH_TAG", "auth-tag-test"), + ("BUZZ_ACP_DISPLAY_NAME", "Test Agent"), + ] +} + +/// Declare one fake MCP server on the wire. `trusted` is emitted only when +/// true so the untrusted case exercises the serde default, exactly as +/// `buzz-acp` writes it. +fn server_decl(name: &str, trusted: bool) -> Value { + let mut decl = json!({ + "name": name, + "command": env!("CARGO_BIN_EXE_fake-mcp"), + "args": [], + "env": [ + { "name": "FAKE_MCP_TOOL_COUNT", "value": "1" }, + { "name": "FAKE_MCP_ENV_REPORT", "value": "1" }, + ], + }); + if trusted { + decl["trusted"] = json!(true); + } + decl +} + +/// Open a session over the given server declarations and return its id. +async fn new_session(h: &mut Harness, servers: Vec) -> String { + h.send( + "initialize", + json!({"protocolVersion":1,"clientCapabilities":{}}), + ) + .await; + let _ = h.recv().await; + h.send( + "session/new", + json!({ "cwd": "/tmp", "mcpServers": servers }), + ) + .await; + let r = h + .recv_until(|v| v.get("result").is_some() || v.get("error").is_some()) + .await; + assert!(r.get("error").is_none(), "session/new failed: {r}"); + r["result"]["sessionId"] + .as_str() + .expect("sessionId") + .to_owned() +} + +/// The text of the `role: "tool"` message in the `n`th captured LLM request. +fn tool_result_text(captured: &[Value], n: usize) -> String { + let msgs = captured + .get(n) + .and_then(|c| c["messages"].as_array()) + .unwrap_or_else(|| panic!("LLM request {n} missing or has no messages")); + msgs.iter() + .rev() + .find(|m| m["role"] == "tool") + .and_then(|m| m["content"].as_str()) + .unwrap_or_else(|| panic!("no tool result message in LLM request {n}")) + .to_owned() +} + +fn env_names(report: &str) -> Vec<&str> { + report.lines().map(str::trim).collect() +} + +/// The spawn boundary withholds every Buzz identity variable from an +/// untrusted MCP server and keeps them for a trusted one — proven in a +/// single agent process, so the untrusted half cannot pass merely because +/// the parent never had the variables. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn untrusted_mcp_env_withheld_from_untrusted_kept_for_trusted() { + let llm = spawn_capturing_llm(vec![ + openai_tool_call("tc1", "extra__tool_0", json!({})), + openai_tool_call("tc2", "devmcp__tool_0", json!({})), + openai_text("done"), + ]) + .await; + let mut h = Harness::spawn_with_env(&llm.url, &identity_env()).await; + let sid = new_session( + &mut h, + vec![server_decl("devmcp", true), server_decl("extra", false)], + ) + .await; + + let p = h + .send( + "session/prompt", + json!({"sessionId": sid, "prompt": [{"type":"text","text":"go"}]}), + ) + .await; + let r = h.recv_until_approving(|v| v["id"] == json!(p)).await; + assert!(r.get("error").is_none(), "prompt errored: {r}"); + + let captured = llm.captured.lock().await; + assert_eq!( + captured.len(), + 3, + "expected 3 LLM calls (initial + 2 tool rounds), got {}", + captured.len() + ); + + let untrusted = tool_result_text(&captured, 1); + let untrusted_names = env_names(&untrusted); + for var in IDENTITY_VARS { + assert!( + !untrusted_names.contains(var), + "untrusted MCP server was spawned with {var}" + ); + } + + let trusted = tool_result_text(&captured, 2); + let trusted_names = env_names(&trusted); + for var in IDENTITY_VARS { + assert!( + trusted_names.contains(var), + "trusted MCP server lost {var}; \ + the untrusted assertion above would pass vacuously" + ); + } + + h.shutdown().await; +} + +/// The filter is narrow: an untrusted server still receives the rest of the +/// passthrough allowlist. An over-broad filter would silently break every +/// third-party server that needs `PATH` or `HOME`, and no other test would +/// catch it. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn untrusted_mcp_env_keeps_non_identity_passthrough() { + let llm = spawn_capturing_llm(vec![ + openai_tool_call("tc1", "extra__tool_0", json!({})), + openai_text("done"), + ]) + .await; + let mut h = Harness::spawn_with_env(&llm.url, &identity_env()).await; + let sid = new_session(&mut h, vec![server_decl("extra", false)]).await; + + let p = h + .send( + "session/prompt", + json!({"sessionId": sid, "prompt": [{"type":"text","text":"go"}]}), + ) + .await; + let r = h.recv_until_approving(|v| v["id"] == json!(p)).await; + assert!(r.get("error").is_none(), "prompt errored: {r}"); + + let captured = llm.captured.lock().await; + let report = tool_result_text(&captured, 1); + let names = env_names(&report); + + // PATH and HOME are unconditional passthrough entries; the display name + // is a Buzz-owned but non-secret one, set on the parent above. + for var in ["PATH", "HOME", "BUZZ_ACP_DISPLAY_NAME"] { + assert!( + names.contains(&var), + "untrusted MCP server lost non-identity passthrough {var}: {report}" + ); + } + // The wire-declared env still arrives — that is the operator's own + // declaration, not ambient parent state. + assert!( + names.contains(&"FAKE_MCP_ENV_REPORT"), + "wire-declared env did not reach the server: {report}" + ); + + h.shutdown().await; +} From 356b3ba32971892890cbe5d4c452e3c937063496 Mon Sep 17 00:00:00 2001 From: wiggdevin <202901685+wiggdevin@users.noreply.github.com> Date: Fri, 4 Sep 2026 02:35:22 -0700 Subject: [PATCH 07/11] fix(acp): cut extra MCP server names to the registry's 128-byte limit MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `sanitize_mcp_name` truncated with an inclusive range (`sanitized[..=128]`), which keeps 129 bytes. `buzz-agent`'s `McpRegistry::spawn_all` rejects any name over `MAX_NAME_LEN` (128) and fails the whole `session/new`, so an executable path or `name=` prefix longer than 128 characters silently cost the agent every one of its MCP tools, not just the extra server. The disambiguation suffix had the same class of bug: `format!("{base_name}-{i}")` appended to a base name already at the ceiling, pushing it back over. - Cut on an exclusive bound through a shared `fit_mcp_name` helper, which also re-trims a hyphen the cut may expose and takes the cut on a character boundary so it cannot panic. - Re-cut the stem against the suffix width when disambiguating, including two-digit suffixes. - Name the limit `MAX_MCP_NAME_LEN` and point it at the constant it mirrors. Tests, each verified falsifiable against the pre-fix code: - `extra_mcp_commands_long_name_truncated_to_registry_limit` — a 130-byte explicit name and a 200-byte executable stem both come back at exactly 128 bytes (fails at 129 with the inclusive slice restored). - `extra_mcp_commands_disambiguation_suffix_respects_registry_limit` — 11 entries sharing a name already at the ceiling stay unique and within the limit (fails at 130 bytes with the plain `format!` restored). - `untrusted_mcp_env_extra_command_reaches_tool_list` — a new end-to-end test that runs the whole seam in one process: the real `BUZZ_ACP_EXTRA_MCP_COMMANDS` variable, `buzz-acp`'s own argument parser, `build_mcp_servers`, a real `buzz-agent` child, `session/new` off the wire, two real MCP subprocesses, and the tool list the agent offers the model. It lives in its own test binary because it writes a process-global environment variable. Previously the two halves of this seam were tested apart, which is how a 129-byte name reached `session/new` unnoticed. `mcp_servers_wire_json` and the `CliArgs`/`Config`/`ConfigError` re-exports make that end-to-end test possible: it drives the harness's real parser rather than a hand-written copy of the wire shape that could drift from it. Also documents, in the flag help and the README, that a name near the 128-byte ceiling still fails because `buzz-agent` caps the qualified tool name `__` at 64 bytes. That tighter bound is left as upstream has it — narrowing it would change behaviour the port is carrying. Signed-off-by: wiggdevin <202901685+wiggdevin@users.noreply.github.com> --- Cargo.lock | 2 + crates/buzz-acp/README.md | 2 +- crates/buzz-acp/src/config.rs | 8 +- crates/buzz-acp/src/lib.rs | 174 ++++++++++++++++-- crates/buzz-agent/Cargo.toml | 5 + .../buzz-agent/tests/untrusted_mcp_env_e2e.rs | 163 ++++++++++++++++ 6 files changed, 334 insertions(+), 20 deletions(-) create mode 100644 crates/buzz-agent/tests/untrusted_mcp_env_e2e.rs diff --git a/Cargo.lock b/Cargo.lock index 49f9fbe0c7e..a77d7c0d4cb 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -894,6 +894,8 @@ dependencies = [ "async-trait", "axum", "base64 0.22.1", + "buzz-acp", + "clap", "dirs", "fs2", "getrandom 0.4.3", diff --git a/crates/buzz-acp/README.md b/crates/buzz-acp/README.md index 97ec1c0aac3..e100b622b90 100644 --- a/crates/buzz-acp/README.md +++ b/crates/buzz-acp/README.md @@ -111,7 +111,7 @@ All configuration is via environment variables (or CLI flags — every env var h | `BUZZ_ACP_AGENT_COMMAND` | no | `goose` | Agent binary to spawn. | | `BUZZ_ACP_AGENT_ARGS` | no | `acp` | Agent arguments (comma-separated). | | `BUZZ_ACP_MCP_COMMAND` | no | `""` (empty) | Path to an optional MCP server binary to provide to the agent subprocess. | -| `BUZZ_ACP_EXTRA_MCP_COMMANDS` | no | — | Newline-separated additional MCP server commands. Each entry is shell-split with POSIX quoting (e.g. `npx -y my-mcp-server`). An optional `name=` prefix sets the server name explicitly (e.g. `memory=npx -y memory-mcp`), so reordering entries does not silently rename a server and strip the agent of its tools. Without a prefix, names are derived from the executable stem, sanitized to ASCII alphanumeric/hyphen, and disambiguated with numeric suffixes. Extra servers do **not** receive `BUZZ_PRIVATE_KEY`, `NOSTR_PRIVATE_KEY`, `BUZZ_RELAY_URL`, or `BUZZ_AUTH_TAG` on the `buzz-agent` MCP spawn path — they are third-party tools, not Buzz-native MCP. **Note:** the credential isolation applies to MCP servers spawned directly by `buzz-agent`'s `McpRegistry`. When using a third-party ACP adapter (e.g. `claude-agent-acp`, `codex-acp`) that spawns its own MCP children, the adapter process inherits the full parent environment including `BUZZ_PRIVATE_KEY`; operators should assume those children can access Buzz credentials unless the adapter itself isolates them. Malformed quoting fails startup with the entry index (the raw command is not logged). | +| `BUZZ_ACP_EXTRA_MCP_COMMANDS` | no | — | Newline-separated additional MCP server commands. Each entry is shell-split with POSIX quoting (e.g. `npx -y my-mcp-server`). An optional `name=` prefix sets the server name explicitly (e.g. `memory=npx -y memory-mcp`), so reordering entries does not silently rename a server and strip the agent of its tools. Without a prefix, names are derived from the executable stem, sanitized to ASCII alphanumeric/hyphen, and disambiguated with numeric suffixes. Names are capped at 128 bytes (suffix included); keep them well under that, since `buzz-agent` also caps the qualified tool name `__` at 64 bytes. Extra servers do **not** receive `BUZZ_PRIVATE_KEY`, `NOSTR_PRIVATE_KEY`, `BUZZ_RELAY_URL`, or `BUZZ_AUTH_TAG` on the `buzz-agent` MCP spawn path — they are third-party tools, not Buzz-native MCP. **Note:** the credential isolation applies to MCP servers spawned directly by `buzz-agent`'s `McpRegistry`. When using a third-party ACP adapter (e.g. `claude-agent-acp`, `codex-acp`) that spawns its own MCP children, the adapter process inherits the full parent environment including `BUZZ_PRIVATE_KEY`; operators should assume those children can access Buzz credentials unless the adapter itself isolates them. Malformed quoting fails startup with the entry index (the raw command is not logged). | | `BUZZ_ACP_IDLE_TIMEOUT` | no | `620` | Idle timeout: max seconds of silence before cancelling a turn. Resets on any agent stdout activity. | | `BUZZ_ACP_MAX_TURN_DURATION` | no | `7200` | Absolute wall-clock cap per turn (safety valve). | | `BUZZ_API_TOKEN` | no | — | API token (required if relay enforces token auth). | diff --git a/crates/buzz-acp/src/config.rs b/crates/buzz-acp/src/config.rs index f365000a630..9b2cc77e5fa 100644 --- a/crates/buzz-acp/src/config.rs +++ b/crates/buzz-acp/src/config.rs @@ -275,9 +275,11 @@ pub struct CliArgs { /// it, the name is derived from the executable stem. Duplicate names are /// disambiguated with a numeric suffix (e.g. two `npx` wrappers become /// `npx` and `npx-2`), but explicit names are preferred so reordering - /// entries does not silently rename a server. Entries with malformed - /// quoting fail startup with the entry index (the raw command is not - /// echoed). Example: + /// entries does not silently rename a server. Names are capped at 128 + /// bytes; keep them well under that, since `buzz-agent` also caps the + /// qualified tool name `__` at 64 bytes. Entries with + /// malformed quoting fail startup with the entry index (the raw command + /// is not echoed). Example: /// `memory=npx -y mcp-remote https://mcp.tavily.com/mcp/?tavilyApiKey=...\nother-server --port 8080` #[arg(long, env = "BUZZ_ACP_EXTRA_MCP_COMMANDS", value_delimiter = '\n')] pub extra_mcp_commands: Vec, diff --git a/crates/buzz-acp/src/lib.rs b/crates/buzz-acp/src/lib.rs index 4f5a02bc795..413fb40276a 100644 --- a/crates/buzz-acp/src/lib.rs +++ b/crates/buzz-acp/src/lib.rs @@ -15,6 +15,17 @@ mod scope; mod setup_mode; mod usage; +/// The harness's command-line and environment arguments, exactly as the +/// `buzz-acp` binary parses them (every flag has an env-var fallback). +/// +/// Public so a caller can build a harness configuration from an explicit +/// argument vector — `CliArgs::try_parse_from(..)` then +/// [`Config::from_args`] — instead of from this process's own argv. +pub use config::CliArgs; +/// A validated harness configuration, resolved from [`CliArgs`]. +pub use config::Config; +/// A failure while reading or validating a harness configuration. +pub use config::ConfigError; pub use usage::TurnUsage; use std::collections::{HashMap, HashSet, VecDeque}; @@ -33,8 +44,8 @@ use buzz_core::observer::{ }; use clap::Parser; use config::{ - AuthAgentArgs, AuthMethodsArgs, AuthenticateArgs, Config, ConfigError, DedupMode, ModelsArgs, - MultipleEventHandling, RespondTo, SubscribeMode, + AuthAgentArgs, AuthMethodsArgs, AuthenticateArgs, DedupMode, ModelsArgs, MultipleEventHandling, + RespondTo, SubscribeMode, }; use filter::SubscriptionRule; use futures_util::FutureExt; @@ -5727,6 +5738,32 @@ async fn run_models(args: ModelsArgs) -> Result<()> { Ok(()) } +/// The `mcpServers` array a `session/new` request carries, serialized exactly +/// as the harness puts it on the wire. +/// +/// Wraps the builder the harness itself uses, so a caller — notably +/// `buzz-agent`'s integration tests — can drive the real +/// `BUZZ_ACP_EXTRA_MCP_COMMANDS` parser end to end instead of hand-writing a +/// copy of the wire shape that could drift away from it. +/// +/// # Errors +/// +/// Returns [`ConfigError::ConfigFile`] when an entry of +/// `BUZZ_ACP_EXTRA_MCP_COMMANDS` has malformed shell quoting, or when the +/// resulting array cannot be serialized. +pub fn mcp_servers_wire_json(config: &Config) -> Result { + let servers = build_mcp_servers(config)?; + serde_json::to_value(&servers) + .map_err(|e| ConfigError::ConfigFile(format!("failed to serialize mcpServers: {e}"))) +} + +/// Maximum length, in bytes, of an MCP server name on the `session/new` wire. +/// +/// Mirrors `MAX_NAME_LEN` in `buzz-agent`'s `mcp` module: a longer name is +/// rejected by `McpRegistry::spawn_all`, which fails the entire session rather +/// than just the offending server. +const MAX_MCP_NAME_LEN: usize = 128; + fn build_mcp_servers(config: &Config) -> Result, ConfigError> { if config.mcp_command.is_empty() { return Ok(vec![]); @@ -5855,10 +5892,15 @@ fn build_mcp_servers(config: &Config) -> Result, ConfigError> { } }; let name = if seen_names.contains(&base_name) { - // Append a numeric suffix until we find a unique name. + // Append a numeric suffix until we find a unique name. The stem is + // re-cut against the suffix so a base name at the length ceiling + // does not overflow it again — an over-long name is rejected by + // `McpRegistry` and fails the whole session, not just this server. let mut i = 2; loop { - let candidate = format!("{base_name}-{i}"); + let suffix = format!("-{i}"); + let head = fit_mcp_name(&base_name, MAX_MCP_NAME_LEN - suffix.len()); + let candidate = format!("{head}{suffix}"); if !seen_names.contains(&candidate) { break candidate; } @@ -5881,25 +5923,40 @@ fn build_mcp_servers(config: &Config) -> Result, ConfigError> { } /// Sanitize a raw executable stem into a name that satisfies the downstream -/// `McpRegistry` validator: ASCII alphanumeric and hyphens only, ≤128 bytes. -/// Non-conforming characters are replaced with hyphens; leading/trailing -/// hyphens are stripped. An empty result falls back to `"extra-mcp"`. +/// `McpRegistry` validator: ASCII alphanumeric and hyphens only, at most +/// [`MAX_MCP_NAME_LEN`] bytes. Non-conforming characters are replaced with +/// hyphens; leading/trailing hyphens are stripped. An empty result falls back +/// to `"extra-mcp"`. +/// +/// Note the registry applies a second, tighter bound the name alone cannot +/// satisfy: each tool is registered as `__` and that qualified +/// name is capped at 64 bytes, so a name anywhere near the 128-byte ceiling +/// still fails the session. Prefer short explicit `name=` prefixes. fn sanitize_mcp_name(raw: &str) -> String { let sanitized: String = raw .chars() .map(|c| if c.is_ascii_alphanumeric() { c } else { '-' }) - .collect::() - .trim_matches('-') - .to_string(); - let truncated = if sanitized.len() > 128 { - sanitized[..=128].to_string() - } else { - sanitized + .collect(); + fit_mcp_name(&sanitized, MAX_MCP_NAME_LEN) +} + +/// Trim surrounding hyphens from `name` and cut it to at most `max` bytes, +/// falling back to `"extra-mcp"` when nothing is left. +/// +/// The cut is taken on a character boundary, so an unsanitized multi-byte +/// input cannot panic here; for the ASCII output of [`sanitize_mcp_name`] it +/// is an exact byte cut. +fn fit_mcp_name(name: &str, max: usize) -> String { + let trimmed = name.trim_matches('-'); + let cut = match trimmed.char_indices().nth(max) { + Some((idx, _)) => &trimmed[..idx], + None => trimmed, }; - if truncated.is_empty() { + let cut = cut.trim_end_matches('-'); + if cut.is_empty() { "extra-mcp".to_string() } else { - truncated + cut.to_string() } } @@ -9408,6 +9465,91 @@ mod build_mcp_servers_tests { ); } + /// The contract `McpRegistry::spawn_all` enforces on every server name: + /// non-empty, at most 128 bytes, ASCII alphanumeric / `_` / `-` only, and + /// no `__` (which would collide with the qualified-tool-name separator). + fn assert_registry_name_contract(name: &str) { + assert!(!name.is_empty(), "server name must not be empty"); + assert!( + name.len() <= 128, + "server name is {} bytes, over McpRegistry's 128-byte limit: {name}", + name.len() + ); + assert!( + name.bytes() + .all(|b| b.is_ascii_alphanumeric() || b == b'_' || b == b'-'), + "server name has characters McpRegistry rejects: {name}" + ); + assert!( + !name.contains("__"), + "server name must not contain the qualified-name separator: {name}" + ); + } + + #[test] + fn extra_mcp_commands_long_name_truncated_to_registry_limit() { + // A raw name longer than the registry's 128-byte ceiling must come + // back at or under it. An inclusive slice (`[..=128]`) yields 129 + // bytes, which McpRegistry rejects with "invalid server name" and + // fails the whole session — this test pins the exclusive cut. + let mut config = test_config(); + let long_explicit = "a".repeat(130); + let long_stem = "b".repeat(200); + config.extra_mcp_commands = vec![ + format!("{long_explicit}=srv --port 8080"), + format!("/opt/bin/{long_stem} --port 8081"), + ]; + let servers = build_mcp_servers(&config).unwrap(); + assert_eq!(servers.len(), 3, "primary + 2 extra = 3 servers"); + assert_eq!( + servers[1].name.len(), + 128, + "an over-long explicit name is cut to exactly 128 bytes" + ); + assert_eq!( + servers[2].name.len(), + 128, + "an over-long executable stem is cut to exactly 128 bytes" + ); + for s in &servers { + assert_registry_name_contract(&s.name); + } + } + + #[test] + fn extra_mcp_commands_disambiguation_suffix_respects_registry_limit() { + // A base name already at the ceiling must not grow past it when the + // `-2`, `-3`, ... disambiguation suffix is appended, including once + // the suffix reaches two digits. + let mut config = test_config(); + let at_ceiling = "c".repeat(128); + config.extra_mcp_commands = (0..11).map(|i| format!("{at_ceiling}=srv-{i}")).collect(); + let servers = build_mcp_servers(&config).unwrap(); + assert_eq!(servers.len(), 12, "primary + 11 extra = 12 servers"); + + let names: Vec<&str> = servers.iter().map(|s| s.name.as_str()).collect(); + for name in &names { + assert_registry_name_contract(name); + } + assert_eq!(names[1], at_ceiling, "the first entry keeps the full name"); + assert!( + names[2].ends_with("-2"), + "the second entry is disambiguated: {}", + names[2] + ); + assert!( + names[11].ends_with("-11"), + "the eleventh entry carries a two-digit suffix: {}", + names[11] + ); + let unique: std::collections::HashSet<&str> = names.iter().copied().collect(); + assert_eq!( + unique.len(), + names.len(), + "truncation must not collapse two servers onto one name: {names:?}" + ); + } + #[test] fn extra_mcp_commands_url_with_equals_not_treated_as_name() { // A command containing `=` in a URL query param must not be diff --git a/crates/buzz-agent/Cargo.toml b/crates/buzz-agent/Cargo.toml index b60644bb7b6..48c1e555cb4 100644 --- a/crates/buzz-agent/Cargo.toml +++ b/crates/buzz-agent/Cargo.toml @@ -73,6 +73,11 @@ fs2 = "0.4" nix = { version = "0.31", default-features = false, features = ["signal", "process"] } [dev-dependencies] +# The extra-MCP-server end-to-end test drives buzz-acp's real +# BUZZ_ACP_EXTRA_MCP_COMMANDS parser so the `mcpServers` array it asserts on is +# the one the harness actually sends, not a hand-written copy of it. +buzz-acp = { path = "../buzz-acp" } +clap = "4" tokio = { workspace = true, features = ["test-util", "rt-multi-thread", "macros", "io-std", "io-util", "sync", "process", "time", "net"] } nix = { version = "0.31", default-features = false, features = ["signal", "process"] } axum = { workspace = true } diff --git a/crates/buzz-agent/tests/untrusted_mcp_env_e2e.rs b/crates/buzz-agent/tests/untrusted_mcp_env_e2e.rs new file mode 100644 index 00000000000..77cfa04d30e --- /dev/null +++ b/crates/buzz-agent/tests/untrusted_mcp_env_e2e.rs @@ -0,0 +1,163 @@ +//! End-to-end binding of `BUZZ_ACP_EXTRA_MCP_COMMANDS` — from the environment +//! variable an operator sets on the harness through to the tools `buzz-agent` +//! offers the model. +//! +//! The two halves of this seam used to be tested apart: `buzz-acp` proved the +//! variable becomes an `mcpServers` array, and `buzz-agent` proved an +//! `mcpServers` array spawns a server. Nothing proved the array `buzz-acp` +//! writes is one `buzz-agent` accepts, which is exactly how a server name one +//! byte over `McpRegistry`'s limit reached `session/new` unnoticed. +//! +//! This test runs the real chain in one process: the real environment +//! variable, the real `buzz-acp` argument parser, the real +//! `build_mcp_servers`, a real `buzz-agent` child, a real `session/new` off +//! the wire, two real MCP subprocesses, and the tool list the agent sends to +//! the model. +//! +//! It lives in its own test binary because it writes a process-global +//! environment variable, which must not race the process spawns of other +//! tests in the same binary. + +mod common; + +use clap::Parser; +use serde_json::{json, Value}; + +use common::{openai_text, openai_tool_call, spawn_capturing_llm, Harness}; + +/// A throwaway secp256k1 secret key (32 bytes of 0x11). `buzz-acp` requires a +/// parseable key to build a config; nothing in this test signs anything. +const TEST_PRIVATE_KEY: &str = "1111111111111111111111111111111111111111111111111111111111111111"; + +/// Every tool name the agent offered the model in its `n`th request, however +/// the provider nests it (`{type,function:{name}}` or a flat `{type,name}`). +fn offered_tool_names(captured: &[Value], n: usize) -> Vec { + let req = captured + .get(n) + .unwrap_or_else(|| panic!("no LLM request {n}")); + req["tools"] + .as_array() + .unwrap_or_else(|| panic!("LLM request {n} carries no tools array: {req}")) + .iter() + .filter_map(|t| { + t.get("name") + .or_else(|| t.get("function").and_then(|f| f.get("name"))) + .and_then(Value::as_str) + .map(str::to_owned) + }) + .collect() +} + +/// `BUZZ_ACP_EXTRA_MCP_COMMANDS` reaches the model's tool list. +/// +/// The extra command names the same executable as the primary MCP server, so +/// the derived name collides and `buzz-acp` must disambiguate it — and the +/// disambiguated name must be one `McpRegistry` accepts, spawns, and lists +/// tools for. A name the registry rejects fails `session/new` outright, so +/// this test goes red for any name-shaping bug on either side of the wire. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn untrusted_mcp_env_extra_command_reaches_tool_list() { + let fake_mcp = env!("CARGO_BIN_EXE_fake-mcp"); + + // The operator's half: the environment variable, parsed by buzz-acp's own + // clap definition (`env = "BUZZ_ACP_EXTRA_MCP_COMMANDS"`, newline-split). + std::env::set_var("BUZZ_ACP_EXTRA_MCP_COMMANDS", fake_mcp); + let args = buzz_acp::CliArgs::try_parse_from([ + "buzz-acp", + "--private-key", + TEST_PRIVATE_KEY, + "--mcp-command", + fake_mcp, + ]) + .expect("buzz-acp CLI args parse"); + let config = buzz_acp::Config::from_args(args).expect("buzz-acp config"); + let servers = buzz_acp::mcp_servers_wire_json(&config).expect("mcpServers wire JSON"); + + let decls = servers.as_array().expect("mcpServers is an array"); + assert_eq!(decls.len(), 2, "primary + 1 extra: {servers}"); + assert_eq!( + decls[0]["trusted"], + json!(true), + "the primary server is the trusted one: {servers}" + ); + assert!( + decls[1].get("trusted").is_none(), + "an extra server is untrusted by omission: {servers}" + ); + let primary_name = decls[0]["name"].as_str().expect("primary name").to_owned(); + let extra_name = decls[1]["name"].as_str().expect("extra name").to_owned(); + assert_ne!( + primary_name, extra_name, + "the colliding derived name must be disambiguated: {servers}" + ); + let extra_qname = format!("{extra_name}__tool_0"); + + // The agent's half: the same array, off the wire, into a real session. + let llm = spawn_capturing_llm(vec![ + openai_tool_call("tc1", &extra_qname, json!({})), + openai_text("done"), + ]) + .await; + let mut h = Harness::spawn(&llm.url).await; + h.send( + "initialize", + json!({"protocolVersion":1,"clientCapabilities":{}}), + ) + .await; + let _ = h.recv().await; + h.send( + "session/new", + json!({ "cwd": "/tmp", "mcpServers": servers }), + ) + .await; + let r = h + .recv_until(|v| v.get("result").is_some() || v.get("error").is_some()) + .await; + assert!( + r.get("error").is_none(), + "session/new rejected the mcpServers array buzz-acp produced: {r}" + ); + let sid = r["result"]["sessionId"] + .as_str() + .expect("sessionId") + .to_owned(); + + let p = h + .send( + "session/prompt", + json!({"sessionId": sid, "prompt": [{"type":"text","text":"go"}]}), + ) + .await; + let done = h.recv_until_approving(|v| v["id"] == json!(p)).await; + assert!(done.get("error").is_none(), "prompt errored: {done}"); + + let captured = llm.captured.lock().await; + let offered = offered_tool_names(&captured, 0); + assert!( + offered.contains(&extra_qname), + "the extra server's tool was not offered to the model: {offered:?}" + ); + assert!( + offered.contains(&format!("{primary_name}__tool_0")), + "the primary server's tool was not offered to the model: {offered:?}" + ); + + // Listed is not enough — the tool must also be callable through the + // disambiguated name, which proves the registry routed it to a live + // process rather than merely accepting the declaration. + let followup = captured + .get(1) + .and_then(|c| c["messages"].as_array()) + .expect("second LLM request carries the tool result"); + let tool_msg = followup + .iter() + .rev() + .find(|m| m["role"] == "tool") + .unwrap_or_else(|| panic!("no tool result in the second LLM request: {followup:?}")); + assert_eq!( + tool_msg["content"], "ok", + "the extra server did not answer the call: {tool_msg}" + ); + + h.shutdown().await; +} From d9a30d8ec448d7d9bfabb98a4a48883c12af796c Mon Sep 17 00:00:00 2001 From: wiggdevin <202901685+wiggdevin@users.noreply.github.com> Date: Fri, 4 Sep 2026 05:09:54 -0700 Subject: [PATCH 08/11] fix(buzz-agent): confine MCP hooks and declared env to trusted servers MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two holes in the trust boundary this port introduces, both reachable once BUZZ_ACP_EXTRA_MCP_COMMANDS can put a `trusted: false` server in the same mcpServers array as the built-in one. McpRegistry::call_hooks picked its targets from the MCP_HOOK_SERVERS allowlist alone and never read spec.trusted. Desktop launches hook-capable runtimes with a wildcard allowlist, so an operator-supplied third-party server advertising _Stop or _PostCompact became an agent-control surface: _Stop decides whether a turn may end, and _PostCompact text is spliced into the fresh context after a handoff. call_hooks now requires spec.trusted. spawn_one dropped the four Buzz identity variables from the ambient passthrough set for an untrusted server, then applied the wire-declared spec.env four lines later without the same filter — so a credential named in mcpServers[].env reached the child regardless of the flag. The declared env is filtered on the same rule. Desktop names the built-in server in MCP_HOOK_SERVERS instead of passing "*", derived from the MCP command's file stem the way build_mcp_servers derives it. The comment justifying the wildcard claimed the name was hard-coded to "buzz-mcp", which was never true and is now moot. Tests: hook_never_invoked_on_untrusted_server pins the hook guard (removing it makes the agent loop on the objection: 1 LLM call becomes 2), and untrusted_mcp_env_withheld_when_declared_on_the_wire pins the env guard against an agent process that has no ambient identity variables at all, so the wire declaration is the only possible source. The existing hook tests now declare their fake server trusted, as the harness does for its own. Signed-off-by: wiggdevin <202901685+wiggdevin@users.noreply.github.com> --- crates/buzz-agent/src/mcp.rs | 19 +++++ .../buzz-agent/tests/permission_boundary.rs | 5 +- crates/buzz-agent/tests/regressions.rs | 85 +++++++++++++++++++ crates/buzz-agent/tests/untrusted_mcp_env.rs | 73 ++++++++++++++++ .../src-tauri/src/managed_agents/runtime.rs | 17 +++- 5 files changed, 196 insertions(+), 3 deletions(-) diff --git a/crates/buzz-agent/src/mcp.rs b/crates/buzz-agent/src/mcp.rs index af46c3b2108..1be0987b419 100644 --- a/crates/buzz-agent/src/mcp.rs +++ b/crates/buzz-agent/src/mcp.rs @@ -361,6 +361,16 @@ impl McpRegistry { // regardless of HashMap iteration order or task completion order. let mut targets: Vec<(usize, String, String)> = Vec::new(); for (idx, server) in self.servers.iter().enumerate() { + // Hooks are agent-control surface, not ordinary tools: `_Stop` + // decides whether a turn may end, and `_PostCompact` text is + // spliced into the fresh context after a handoff. Only a server + // the harness marked `trusted` may run them. Operator-supplied + // extra servers (`BUZZ_ACP_EXTRA_MCP_COMMANDS`) are untrusted and + // are never hook targets, whatever the allowlist says — including + // the wildcard hook-capable runtimes are launched with. + if !server.spec.trusted { + continue; + } if !allowed.allows(&server.name) { continue; } @@ -758,6 +768,15 @@ async fn spawn_one( } } for (k, v) in &spec.env { + // The wire-declared env is filtered on the same rule as the ambient + // one. Without this the withholding above is bypassable by + // declaration: the harness itself puts variables in + // `mcpServers[].env`, so an identity credential named there would + // reach an untrusted child even though the same name was just dropped + // from the passthrough set. + if !spec.trusted && is_buzz_identity_env(k) { + continue; + } cmd.env(k, v); } cmd.current_dir(&spec.cwd); diff --git a/crates/buzz-agent/tests/permission_boundary.rs b/crates/buzz-agent/tests/permission_boundary.rs index 2873ba14d6f..cf02297f474 100644 --- a/crates/buzz-agent/tests/permission_boundary.rs +++ b/crates/buzz-agent/tests/permission_boundary.rs @@ -185,7 +185,10 @@ async fn init( let servers = if mcp_env.is_empty() { json!([]) } else { - json!([{ "name": "fake", "command": fake_mcp, "args": [], "env": env }]) + // `trusted` stands in for the harness's built-in server. Lifecycle + // hooks run only on trusted servers, so the `_Stop` exemption test + // below would pass vacuously without it. + json!([{ "name": "fake", "command": fake_mcp, "args": [], "env": env, "trusted": true }]) }; h.send("session/new", json!({ "cwd": cwd, "mcpServers": servers })) .await; diff --git a/crates/buzz-agent/tests/regressions.rs b/crates/buzz-agent/tests/regressions.rs index fd5042a1167..75e2a454cc5 100644 --- a/crates/buzz-agent/tests/regressions.rs +++ b/crates/buzz-agent/tests/regressions.rs @@ -548,7 +548,23 @@ async fn description_clamping_enforced() { /// Helper: spawn a session with a fake MCP server exposing one regular tool /// plus an optional `_Stop` hook controlled by env vars. +/// +/// The server is declared `trusted`, standing in for the harness's built-in +/// server. Hooks only run on trusted servers, so an untrusted declaration here +/// would silently disable every `_Stop` / `_PostCompact` test below — +/// `hook_never_invoked_on_untrusted_server` pins that boundary. async fn init_session_with_fake_mcp(h: &mut Harness, extra_mcp_env: &[(&str, &str)]) -> String { + init_session_with_fake_mcp_trust(h, extra_mcp_env, true).await +} + +/// As [`init_session_with_fake_mcp`], with the server's `trusted` flag under +/// the caller's control. `trusted: false` is exactly how `buzz-acp` declares an +/// operator-supplied `BUZZ_ACP_EXTRA_MCP_COMMANDS` server. +async fn init_session_with_fake_mcp_trust( + h: &mut Harness, + extra_mcp_env: &[(&str, &str)], + trusted: bool, +) -> String { let fake_mcp = env!("CARGO_BIN_EXE_fake-mcp"); let env: Vec = extra_mcp_env .iter() @@ -569,6 +585,7 @@ async fn init_session_with_fake_mcp(h: &mut Harness, extra_mcp_env: &[(&str, &st "command": fake_mcp, "args": [], "env": env, + "trusted": trusted, }], }), ) @@ -671,6 +688,74 @@ async fn hook_stop_blocks_premature_end() { h.shutdown().await; } +/// An untrusted MCP server never runs a hook, even under the wildcard +/// allowlist the Desktop launches hook-capable runtimes with. +/// +/// `MCP_HOOK_SERVERS="*"` matches every server name, so the allowlist alone +/// would make an operator-supplied `BUZZ_ACP_EXTRA_MCP_COMMANDS` server a +/// `_Stop` / `_PostCompact` target — a third-party process deciding when the +/// agent may stop and splicing text into its fresh context. The trust flag is +/// the guard; this test is its falsifier. Remove the `spec.trusted` check in +/// `McpRegistry::call_hooks` and the objection lands, the agent loops, and the +/// LLM-call count below goes from 1 to 2. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn hook_never_invoked_on_untrusted_server() { + // Two scripted responses: with the guard only the first is consumed; a + // regression consumes both instead of starving the fake LLM. + let llm = spawn_capturing_llm(vec![openai_text("done"), openai_text("looped")]).await; + let mut h = Harness::spawn_with_env( + &llm.url, + &[ + ("MCP_HOOK_SERVERS", "*"), + ("BUZZ_AGENT_STOP_MAX_REJECTIONS", "10"), + ], + ) + .await; + let sid = init_session_with_fake_mcp_trust( + &mut h, + &[ + ("FAKE_MCP_TOOL_COUNT", "1"), + ("FAKE_MCP_STOP_HOOK", "1"), + ("FAKE_MCP_STOP_TEXT", "you have open work"), + ("FAKE_MCP_STOP_COUNT", "1"), + ], + false, + ) + .await; + + let p = h + .send( + "session/prompt", + json!({"sessionId": sid, "prompt": [{"type":"text","text":"go"}]}), + ) + .await; + let r = h.recv_until_approving(|v| v["id"] == json!(p)).await; + assert!(r.get("result").is_some(), "errored: {r}"); + assert_eq!(r["result"]["stopReason"], "end_turn"); + + let captured = llm.captured.lock().await; + assert_eq!( + captured.len(), + 1, + "an untrusted server's _Stop hook objected and made the agent loop: \ + {} LLM calls", + captured.len() + ); + let objection_present = captured.iter().any(|req| { + req["messages"] + .as_array() + .into_iter() + .flatten() + .any(|m| m["content"].as_str().unwrap_or("").contains("_Stop")) + }); + assert!( + !objection_present, + "an untrusted server's hook output reached the model: {:?}", + *captured + ); + h.shutdown().await; +} + /// After `stop_max_rejections` objections, the agent honors end_turn /// even if `_Stop` would still object. Set max=1 so it trips quickly. #[tokio::test(flavor = "multi_thread", worker_threads = 2)] diff --git a/crates/buzz-agent/tests/untrusted_mcp_env.rs b/crates/buzz-agent/tests/untrusted_mcp_env.rs index 4f1cefcd6c6..ac13e7f6ccb 100644 --- a/crates/buzz-agent/tests/untrusted_mcp_env.rs +++ b/crates/buzz-agent/tests/untrusted_mcp_env.rs @@ -60,6 +60,23 @@ fn server_decl(name: &str, trusted: bool) -> Value { decl } +/// Declare one fake MCP server that also names the Buzz identity variables in +/// its own wire `env` block — the shape produced when a harness puts +/// credentials in `mcpServers[].env` instead of relying on the parent +/// environment. The values are throwaway strings; only names travel back. +fn server_decl_declaring_identity(name: &str, trusted: bool) -> Value { + let mut decl = server_decl(name, trusted); + let env = decl["env"] + .as_array_mut() + .expect("declaration has an env array"); + for (k, v) in identity_env() { + if IDENTITY_VARS.contains(&k) { + env.push(json!({ "name": k, "value": v })); + } + } + decl +} + /// Open a session over the given server declarations and return its id. async fn new_session(h: &mut Harness, servers: Vec) -> String { h.send( @@ -159,6 +176,62 @@ async fn untrusted_mcp_env_withheld_from_untrusted_kept_for_trusted() { h.shutdown().await; } +/// The filter covers the wire-declared env too, not just the ambient one. +/// +/// The agent process here is spawned with **no** identity variables, so the +/// only way one can reach a child is the `mcpServers[].env` block the harness +/// wrote. An untrusted server must still not receive them; a trusted one must, +/// or the untrusted assertion proves nothing. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn untrusted_mcp_env_withheld_when_declared_on_the_wire() { + let llm = spawn_capturing_llm(vec![ + openai_tool_call("tc1", "extra__tool_0", json!({})), + openai_tool_call("tc2", "devmcp__tool_0", json!({})), + openai_text("done"), + ]) + .await; + let mut h = Harness::spawn(&llm.url).await; + let sid = new_session( + &mut h, + vec![ + server_decl_declaring_identity("devmcp", true), + server_decl_declaring_identity("extra", false), + ], + ) + .await; + + let p = h + .send( + "session/prompt", + json!({"sessionId": sid, "prompt": [{"type":"text","text":"go"}]}), + ) + .await; + let r = h.recv_until_approving(|v| v["id"] == json!(p)).await; + assert!(r.get("error").is_none(), "prompt errored: {r}"); + + let captured = llm.captured.lock().await; + let untrusted = tool_result_text(&captured, 1); + let untrusted_names = env_names(&untrusted); + for var in IDENTITY_VARS { + assert!( + !untrusted_names.contains(var), + "untrusted MCP server received {var} through its wire-declared env" + ); + } + + let trusted = tool_result_text(&captured, 2); + let trusted_names = env_names(&trusted); + for var in IDENTITY_VARS { + assert!( + trusted_names.contains(var), + "trusted MCP server did not receive wire-declared {var}; \ + the untrusted assertion above would pass vacuously" + ); + } + + h.shutdown().await; +} + /// The filter is narrow: an untrusted server still receives the rest of the /// passthrough allowlist. An over-broad filter would silently break every /// third-party server that needs `PATH` or `HOME`, and no other test would diff --git a/desktop/src-tauri/src/managed_agents/runtime.rs b/desktop/src-tauri/src/managed_agents/runtime.rs index b8d586b32af..41efd647b0a 100644 --- a/desktop/src-tauri/src/managed_agents/runtime.rs +++ b/desktop/src-tauri/src/managed_agents/runtime.rs @@ -592,10 +592,23 @@ pub fn spawn_agent_child( } } // Enable MCP hook tools (_Stop, _PostCompact) for agents that need them. - // Uses "*" because build_mcp_servers() hard-codes the server name to "buzz-mcp". + // Name the built-in server rather than passing "*": since + // BUZZ_ACP_EXTRA_MCP_COMMANDS, the harness can put operator-supplied + // third-party servers in the same array, and a wildcard would nominate + // those as hook targets too. (buzz-agent refuses to run a hook on a server + // that is not marked `trusted`, so this is the second gate, not the only + // one.) The name is the MCP command's file stem, which is how + // buzz-acp's build_mcp_servers() derives it — it is not a fixed + // "buzz-mcp". With no MCP command there is no built-in server to hook. let runtime_meta = known_acp_runtime(effective_command); if runtime_meta.is_some_and(|r| r.mcp_hooks) { - command.env("MCP_HOOK_SERVERS", "*"); + if let Some(server_name) = resolved_mcp_command + .as_deref() + .and_then(std::path::Path::file_stem) + .and_then(|stem| stem.to_str()) + { + command.env("MCP_HOOK_SERVERS", server_name); + } } // ── Readiness check: set setup-payload if agent is not ready ───────────── From 6130fe0b10bb45e36b4385759aa0dcf72fa2915d Mon Sep 17 00:00:00 2001 From: wiggdevin <202901685+wiggdevin@users.noreply.github.com> Date: Fri, 4 Sep 2026 05:10:17 -0700 Subject: [PATCH 09/11] fix(buzz-acp): correct the extra-MCP parser's contracts and bind its wire seam MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Four defects in build_mcp_servers, three of them inherited verbatim from PR #6651 and reported upstream as parity bugs, not fork regressions. An extras-only configuration produced zero servers. The function returned early when config.mcp_command was empty, before the extras loop — BUZZ_ACP_MCP_COMMAND defaults to "" and Desktop writes "" whenever the runtime has no MCP command, so setting only BUZZ_ACP_EXTRA_MCP_COMMANDS started the harness clean and offered the model no tools at all. The primary server is now appended conditionally and the extras always parsed; only an entirely unset configuration yields an empty array. The test that pinned the old behaviour as correct is replaced by one asserting the extra server survives. A non-blank entry that shell-split to nothing was dropped at a silent `continue`: `memory=`, whitespace after the `=`, an empty quoted command, or a comment-only line each started the harness with that server missing and nothing logged, while malformed quoting correctly aborted startup. Both shapes now fail closed with the entry index and no echo of the entry. The generated-name cap enforced the wrong contract. MAX_MCP_NAME_LEN mirrored buzz-agent's 128-byte bound on the name alone, but the registry builds `__` and fails the whole session above its 64-byte MAX_QNAME_LEN — so a name cut to exactly 128 bytes was still fatal the moment the server advertised any tool. The cap is budgeted against the qualified name instead, and assert_registry_name_contract now covers that bound rather than describing the looser one. Buzz origin metadata was injected into untrusted extras. mcp_servers_with_git_origin pushed BUZZ_GIT_ORIGIN_CHANNEL_ID or BUZZ_GIT_ORIGIN_AGENT_NAME into every element of the array; before this port the array held exactly one server, and it now holds third-party ones. The origin goes only to trusted servers, and the pool tests run a mixed array so the guard is falsifiable. The last hop is now bound as well. The e2e test called mcp_servers_wire_json and hand-sent session/new, so replacing the PromptContext assignment with vec![] or breaking the pool's env injection left it green. Both steps live behind one type: McpServerSet::from_config is the only way to fill PromptContext::mcp_servers (from_servers is cfg(test)) and for_session is the only way to read it back, so the wire array cannot be produced any other way and the test drives exactly the code the harness runs. Signed-off-by: wiggdevin <202901685+wiggdevin@users.noreply.github.com> --- Cargo.lock | 1 + crates/buzz-acp/README.md | 2 +- crates/buzz-acp/src/lib.rs | 355 +++++++++++++----- crates/buzz-acp/src/pool.rs | 141 ++++--- crates/buzz-agent/Cargo.toml | 3 + .../buzz-agent/tests/untrusted_mcp_env_e2e.rs | 38 +- 6 files changed, 389 insertions(+), 151 deletions(-) diff --git a/Cargo.lock b/Cargo.lock index a77d7c0d4cb..d7450fe6b85 100644 --- a/Cargo.lock +++ b/Cargo.lock @@ -913,6 +913,7 @@ dependencies = [ "tracing-subscriber", "url", "urlencoding", + "uuid", "webbrowser", ] diff --git a/crates/buzz-acp/README.md b/crates/buzz-acp/README.md index e100b622b90..ce72b4156e4 100644 --- a/crates/buzz-acp/README.md +++ b/crates/buzz-acp/README.md @@ -111,7 +111,7 @@ All configuration is via environment variables (or CLI flags — every env var h | `BUZZ_ACP_AGENT_COMMAND` | no | `goose` | Agent binary to spawn. | | `BUZZ_ACP_AGENT_ARGS` | no | `acp` | Agent arguments (comma-separated). | | `BUZZ_ACP_MCP_COMMAND` | no | `""` (empty) | Path to an optional MCP server binary to provide to the agent subprocess. | -| `BUZZ_ACP_EXTRA_MCP_COMMANDS` | no | — | Newline-separated additional MCP server commands. Each entry is shell-split with POSIX quoting (e.g. `npx -y my-mcp-server`). An optional `name=` prefix sets the server name explicitly (e.g. `memory=npx -y memory-mcp`), so reordering entries does not silently rename a server and strip the agent of its tools. Without a prefix, names are derived from the executable stem, sanitized to ASCII alphanumeric/hyphen, and disambiguated with numeric suffixes. Names are capped at 128 bytes (suffix included); keep them well under that, since `buzz-agent` also caps the qualified tool name `__` at 64 bytes. Extra servers do **not** receive `BUZZ_PRIVATE_KEY`, `NOSTR_PRIVATE_KEY`, `BUZZ_RELAY_URL`, or `BUZZ_AUTH_TAG` on the `buzz-agent` MCP spawn path — they are third-party tools, not Buzz-native MCP. **Note:** the credential isolation applies to MCP servers spawned directly by `buzz-agent`'s `McpRegistry`. When using a third-party ACP adapter (e.g. `claude-agent-acp`, `codex-acp`) that spawns its own MCP children, the adapter process inherits the full parent environment including `BUZZ_PRIVATE_KEY`; operators should assume those children can access Buzz credentials unless the adapter itself isolates them. Malformed quoting fails startup with the entry index (the raw command is not logged). | +| `BUZZ_ACP_EXTRA_MCP_COMMANDS` | no | — | Newline-separated additional MCP server commands. Each entry is shell-split with POSIX quoting (e.g. `npx -y my-mcp-server`). An optional `name=` prefix sets the server name explicitly (e.g. `memory=npx -y memory-mcp`), so reordering entries does not silently rename a server and strip the agent of its tools. Without a prefix, names are derived from the executable stem, sanitized to ASCII alphanumeric/hyphen, and disambiguated with numeric suffixes. Names are capped at 32 bytes (suffix included), budgeted against `buzz-agent`'s 64-byte cap on the qualified tool name `__` so a generated name always leaves room for the tools it carries. Extra servers do **not** receive `BUZZ_PRIVATE_KEY`, `NOSTR_PRIVATE_KEY`, `BUZZ_RELAY_URL`, or `BUZZ_AUTH_TAG` on the `buzz-agent` MCP spawn path — they are third-party tools, not Buzz-native MCP. **Note:** the credential isolation applies to MCP servers spawned directly by `buzz-agent`'s `McpRegistry`. When using a third-party ACP adapter (e.g. `claude-agent-acp`, `codex-acp`) that spawns its own MCP children, the adapter process inherits the full parent environment including `BUZZ_PRIVATE_KEY`; operators should assume those children can access Buzz credentials unless the adapter itself isolates them. An entry that is malformed, or that shell-splits to no command at all (`memory=`, a comment-only line), fails startup with the entry index (the raw entry is not logged). The primary server is optional: an extras-only configuration is valid, and only an entirely unset one yields no servers. | | `BUZZ_ACP_IDLE_TIMEOUT` | no | `620` | Idle timeout: max seconds of silence before cancelling a turn. Resets on any agent stdout activity. | | `BUZZ_ACP_MAX_TURN_DURATION` | no | `7200` | Absolute wall-clock cap per turn (safety valve). | | `BUZZ_API_TOKEN` | no | — | API token (required if relay enforces token auth). | diff --git a/crates/buzz-acp/src/lib.rs b/crates/buzz-acp/src/lib.rs index 413fb40276a..a36d010bf5f 100644 --- a/crates/buzz-acp/src/lib.rs +++ b/crates/buzz-acp/src/lib.rs @@ -2772,7 +2772,7 @@ async fn tokio_main() -> Result<()> { let base_prompt_content = config.base_prompt_content.take(); let cwd = current_working_directory()?; let ctx = Arc::new(PromptContext { - mcp_servers: build_mcp_servers(&config)?, + mcp_servers: McpServerSet::from_config(&config)?, initial_message: config.initial_message.clone(), idle_timeout: Duration::from_secs(config.idle_timeout_secs), max_turn_duration: Duration::from_secs(config.max_turn_duration_secs), @@ -5738,88 +5738,183 @@ async fn run_models(args: ModelsArgs) -> Result<()> { Ok(()) } +/// Where a `session/new` request comes from, for the git-origin variable the +/// trusted MCP servers receive. +#[derive(Debug, Clone, Copy, Default)] +pub struct SessionOrigin<'a> { + /// Channel the session is scoped to, when it has one. + pub channel_id: Option, + /// Channel type as the relay reports it (`"stream"`, `"dm"`, …). + pub channel_type: Option<&'a str>, + /// Sanitized agent name — the git origin outside stream channels. + pub agent_name: Option<&'a str>, +} + +/// The MCP servers a harness offers its agent. +/// +/// This type is the only seam between the `BUZZ_ACP_EXTRA_MCP_COMMANDS` parser +/// and the `session/new` wire: `PromptContext::mcp_servers` can only be filled +/// by [`McpServerSet::from_config`], and the pool can only read it back through +/// `for_session`, which adds the per-session git-origin variable. A caller that +/// drives those two steps — notably `buzz-agent`'s integration tests, through +/// [`mcp_servers_wire_json`] — therefore exercises the same bytes the harness +/// sends, with no second construction path to drift from. +#[derive(Debug, Clone)] +pub struct McpServerSet(Vec); + +impl McpServerSet { + /// Build the set once, at startup, from the harness configuration. + /// + /// # Errors + /// + /// Returns [`ConfigError::ConfigFile`] when an entry of + /// `BUZZ_ACP_EXTRA_MCP_COMMANDS` is malformed or names no command. + pub fn from_config(config: &Config) -> Result { + Ok(Self(build_mcp_servers(config)?)) + } + + /// The array one `session/new` carries: the configured servers plus the + /// git-origin variable for `origin`. + /// + /// The origin names a Buzz channel or agent, so it reaches only servers the + /// harness marked `trusted`. An operator-supplied extra server is a + /// third-party process and receives no Buzz-native context — not even the + /// channel it happens to be running in. + pub(crate) fn for_session(&self, origin: SessionOrigin<'_>) -> Vec { + let mut servers = self.0.clone(); + let var = match (origin.channel_id, origin.channel_type) { + (Some(channel_id), Some("stream")) => Some(EnvVar { + name: "BUZZ_GIT_ORIGIN_CHANNEL_ID".into(), + value: channel_id.to_string(), + }), + (Some(_), _) => origin + .agent_name + .filter(|name| !name.trim().is_empty()) + .map(|name| EnvVar { + name: "BUZZ_GIT_ORIGIN_AGENT_NAME".into(), + value: name.trim().to_string(), + }), + (None, _) => None, + }; + if let Some(var) = var { + for server in servers.iter_mut().filter(|s| s.trusted) { + server.env.push(var.clone()); + } + } + servers + } + + /// Test-only constructor over a fixed server list. Production code goes + /// through [`Self::from_config`], so the parser can never be bypassed. + #[cfg(test)] + pub(crate) fn from_servers(servers: Vec) -> Self { + Self(servers) + } +} + /// The `mcpServers` array a `session/new` request carries, serialized exactly /// as the harness puts it on the wire. /// -/// Wraps the builder the harness itself uses, so a caller — notably -/// `buzz-agent`'s integration tests — can drive the real -/// `BUZZ_ACP_EXTRA_MCP_COMMANDS` parser end to end instead of hand-writing a -/// copy of the wire shape that could drift away from it. +/// Runs the whole production path — [`McpServerSet::from_config`] then the +/// per-session git-origin step — so a caller, notably `buzz-agent`'s +/// integration tests, can drive the real `BUZZ_ACP_EXTRA_MCP_COMMANDS` parser +/// end to end instead of hand-writing a copy of the wire shape. /// /// # Errors /// /// Returns [`ConfigError::ConfigFile`] when an entry of -/// `BUZZ_ACP_EXTRA_MCP_COMMANDS` has malformed shell quoting, or when the -/// resulting array cannot be serialized. -pub fn mcp_servers_wire_json(config: &Config) -> Result { - let servers = build_mcp_servers(config)?; +/// `BUZZ_ACP_EXTRA_MCP_COMMANDS` is malformed, or when the resulting array +/// cannot be serialized. +pub fn mcp_servers_wire_json( + config: &Config, + origin: SessionOrigin<'_>, +) -> Result { + let servers = McpServerSet::from_config(config)?.for_session(origin); serde_json::to_value(&servers) .map_err(|e| ConfigError::ConfigFile(format!("failed to serialize mcpServers: {e}"))) } -/// Maximum length, in bytes, of an MCP server name on the `session/new` wire. +/// `buzz-agent`'s cap on a *qualified* tool name (`__`), mirrored +/// from `MAX_QNAME_LEN` in its `mcp` module. Passing it fails the whole +/// session, not just the offending server. +const MAX_MCP_QNAME_LEN: usize = 64; + +/// Bytes reserved inside [`MAX_MCP_QNAME_LEN`] for the `__` separator and the +/// bare tool name. Wider than any tool `buzz-dev-mcp` advertises, so a name +/// generated here can carry the tools it was generated for. +const MCP_TOOL_NAME_RESERVE: usize = 32; + +/// Maximum length, in bytes, of a generated MCP server name. /// -/// Mirrors `MAX_NAME_LEN` in `buzz-agent`'s `mcp` module: a longer name is -/// rejected by `McpRegistry::spawn_all`, which fails the entire session rather -/// than just the offending server. -const MAX_MCP_NAME_LEN: usize = 128; +/// The binding constraint is not `buzz-agent`'s 128-byte `MAX_NAME_LEN` on the +/// name alone. `McpRegistry` registers every tool as `__` and +/// rejects the entire session once that qualified name passes +/// [`MAX_MCP_QNAME_LEN`], so a name cut to 128 bytes is still fatal the moment +/// the server advertises anything. Budgeting against the qualified name is what +/// makes a generated name usable. +const MAX_MCP_NAME_LEN: usize = MAX_MCP_QNAME_LEN - MCP_TOOL_NAME_RESERVE; fn build_mcp_servers(config: &Config) -> Result, ConfigError> { - if config.mcp_command.is_empty() { - return Ok(vec![]); - } - let mut servers = vec![McpServer { - name: std::path::Path::new(&config.mcp_command) - .file_stem() - .and_then(|s| s.to_str()) - .unwrap_or("mcp") - .to_string(), - command: config.mcp_command.clone(), - args: vec![], - env: { - let mut env = vec![ - EnvVar { - name: "BUZZ_RELAY_URL".into(), - value: config.relay_url.clone(), - }, - EnvVar { - name: "BUZZ_PRIVATE_KEY".into(), - // bech32 encoding of a valid secret key is infallible. - // Panic here is correct: injecting a bogus secret would cause - // delayed, hard-to-diagnose agent failures downstream. - value: config - .keys - .secret_key() - .to_bech32() - .expect("secret key bech32 encoding should never fail"), - }, - ]; - // Forward BUZZ_AUTH_TAG (NIP-OA owner attestation credential) - // so the MCP server can attach it to every signed event. - if let Ok(auth_tag) = std::env::var("BUZZ_AUTH_TAG") { - if !auth_tag.is_empty() { - env.push(EnvVar { - name: "BUZZ_AUTH_TAG".into(), - value: auth_tag, - }); + let mut servers: Vec = Vec::new(); + // The primary server is optional: `BUZZ_ACP_MCP_COMMAND` defaults to empty + // and Desktop writes an empty string whenever the runtime has no MCP + // command. An extras-only configuration is legitimate and must still + // produce servers — returning an empty array here would hand the model no + // tools at all, silently, which is a failure reported as a valid result. + if !config.mcp_command.is_empty() { + servers.push(McpServer { + name: std::path::Path::new(&config.mcp_command) + .file_stem() + .and_then(|s| s.to_str()) + .unwrap_or("mcp") + .to_string(), + command: config.mcp_command.clone(), + args: vec![], + env: { + let mut env = vec![ + EnvVar { + name: "BUZZ_RELAY_URL".into(), + value: config.relay_url.clone(), + }, + EnvVar { + name: "BUZZ_PRIVATE_KEY".into(), + // bech32 encoding of a valid secret key is infallible. + // Panic here is correct: injecting a bogus secret would cause + // delayed, hard-to-diagnose agent failures downstream. + value: config + .keys + .secret_key() + .to_bech32() + .expect("secret key bech32 encoding should never fail"), + }, + ]; + // Forward BUZZ_AUTH_TAG (NIP-OA owner attestation credential) + // so the MCP server can attach it to every signed event. + if let Ok(auth_tag) = std::env::var("BUZZ_AUTH_TAG") { + if !auth_tag.is_empty() { + env.push(EnvVar { + name: "BUZZ_AUTH_TAG".into(), + value: auth_tag, + }); + } } - } - // Forward the agent's display name so dev-mcp can use it as the git - // author name instead of the raw npub. Read from the process env - // rather than Config: this is a pass-through of a contract owned - // upstream, and absent simply means dev-mcp falls back to the npub. - if let Ok(display_name) = std::env::var("BUZZ_ACP_DISPLAY_NAME") { - if !display_name.is_empty() { - env.push(EnvVar { - name: "BUZZ_ACP_DISPLAY_NAME".into(), - value: display_name, - }); + // Forward the agent's display name so dev-mcp can use it as the git + // author name instead of the raw npub. Read from the process env + // rather than Config: this is a pass-through of a contract owned + // upstream, and absent simply means dev-mcp falls back to the npub. + if let Ok(display_name) = std::env::var("BUZZ_ACP_DISPLAY_NAME") { + if !display_name.is_empty() { + env.push(EnvVar { + name: "BUZZ_ACP_DISPLAY_NAME".into(), + value: display_name, + }); + } } - } - env - }, - trusted: true, - }]; + env + }, + trusted: true, + }); + } // Append extra MCP servers from BUZZ_ACP_EXTRA_MCP_COMMANDS. // Each entry is newline-separated and shell-split into command + args @@ -5832,7 +5927,7 @@ fn build_mcp_servers(config: &Config) -> Result, ConfigError> { // Extra servers do not receive Buzz relay credentials or auth tags — // they are third-party tools, not Buzz-native MCP servers. let mut seen_names: std::collections::HashSet = - std::collections::HashSet::from_iter([servers[0].name.clone()]); + servers.iter().map(|s| s.name.clone()).collect(); for (idx, extra) in config.extra_mcp_commands.iter().enumerate() { let trimmed = extra.trim(); if trimmed.is_empty() { @@ -5864,9 +5959,20 @@ fn build_mcp_servers(config: &Config) -> Result, ConfigError> { } None => (None, trimmed.to_string()), }; + // Fail closed on both shapes of unusable entry. A non-blank entry that + // shell-splits to nothing — `memory=`, a lone `#comment`, `" "` after + // the `=` — is as broken as malformed quoting, and dropping it + // silently starts the harness with that server missing and nothing + // logged. Neither message echoes the entry: it may carry an API key. let parts = match shlex::split(&command_str) { - Some(p) if !p.is_empty() => p, - Some(_) => continue, + Some(p) if p.first().is_some_and(|command| !command.is_empty()) => p, + Some(_) => { + return Err(ConfigError::ConfigFile(format!( + "BUZZ_ACP_EXTRA_MCP_COMMANDS entry {} names no command; \ + give it a command or remove the entry and restart", + idx + 1 + ))); + } None => { return Err(ConfigError::ConfigFile(format!( "BUZZ_ACP_EXTRA_MCP_COMMANDS entry {} has malformed shell quoting; \ @@ -5928,10 +6034,10 @@ fn build_mcp_servers(config: &Config) -> Result, ConfigError> { /// hyphens; leading/trailing hyphens are stripped. An empty result falls back /// to `"extra-mcp"`. /// -/// Note the registry applies a second, tighter bound the name alone cannot -/// satisfy: each tool is registered as `__` and that qualified -/// name is capped at 64 bytes, so a name anywhere near the 128-byte ceiling -/// still fails the session. Prefer short explicit `name=` prefixes. +/// [`MAX_MCP_NAME_LEN`] is budgeted against the registry's *qualified* tool +/// name (`__`, capped at [`MAX_MCP_QNAME_LEN`]), not against its +/// looser 128-byte bound on the name alone, so a sanitized name always leaves +/// room for the tools the server advertises. fn sanitize_mcp_name(raw: &str) -> String { let sanitized: String = raw .chars() @@ -9331,15 +9437,32 @@ mod build_mcp_servers_tests { } #[test] - fn extra_mcp_commands_with_empty_mcp_command_returns_no_servers() { + fn extra_mcp_commands_without_primary_still_produce_servers() { + // `BUZZ_ACP_MCP_COMMAND` defaults to empty, and Desktop writes an + // empty string whenever the runtime has no MCP command. An + // extras-only configuration must still reach the model: returning an + // empty array here would leave the agent with no tools, silently. let mut config = test_config(); config.mcp_command = "".into(); - config.extra_mcp_commands = vec!["some-extra-server".into()]; + config.extra_mcp_commands = vec!["memory=memory-mcp --db /tmp/m".into()]; let servers = build_mcp_servers(&config).unwrap(); - assert!( - servers.is_empty(), - "empty primary mcp_command should still short-circuit even with extras" + assert_eq!( + servers.len(), + 1, + "the extra server must survive: {servers:?}" ); + assert_eq!(servers[0].name, "memory"); + assert_eq!(servers[0].command, "memory-mcp"); + assert!(!servers[0].trusted, "an extra server is never trusted"); + } + + #[test] + fn no_mcp_command_and_no_extras_produces_no_servers() { + let mut config = test_config(); + config.mcp_command = "".into(); + config.extra_mcp_commands = vec![]; + let servers = build_mcp_servers(&config).unwrap(); + assert!(servers.is_empty(), "nothing configured, nothing produced"); } #[test] @@ -9388,6 +9511,36 @@ mod build_mcp_servers_tests { ); } + #[test] + fn extra_mcp_commands_fail_closed_on_entry_with_no_command() { + // A non-blank entry that shell-splits to nothing used to be dropped + // silently, so the harness started with that server missing and + // nothing logged — while malformed quoting aborted startup. Same + // posture for both now, and neither message echoes the entry. + for (label, entry) in [ + ("name with nothing after `=`", "memory="), + ("whitespace after `=`", "memory= "), + ("empty quoted command", "memory=''"), + ("comment-only entry", "# just a note"), + ] { + let mut config = test_config(); + config.extra_mcp_commands = vec!["valid-server".into(), entry.into()]; + let result = build_mcp_servers(&config); + let err = match result { + Err(e) => format!("{e}"), + Ok(servers) => panic!("{label} was accepted, giving {servers:?}"), + }; + assert!( + err.contains("entry 2"), + "{label}: error should identify the entry index: {err}" + ); + assert!( + !err.contains("memory") && !err.contains("just a note"), + "{label}: error must not echo the raw entry: {err}" + ); + } + } + #[test] fn extra_mcp_commands_sanitized_names() { // Names with underscores, spaces, or punctuation must be sanitized @@ -9466,8 +9619,11 @@ mod build_mcp_servers_tests { } /// The contract `McpRegistry::spawn_all` enforces on every server name: - /// non-empty, at most 128 bytes, ASCII alphanumeric / `_` / `-` only, and - /// no `__` (which would collide with the qualified-tool-name separator). + /// non-empty, ASCII alphanumeric / `_` / `-` only, no `__` (which would + /// collide with the qualified-tool-name separator), at most 128 bytes — + /// and, the bound that actually bites, short enough that + /// `__` still fits `MAX_QNAME_LEN`. The registry rejects the + /// whole session on any of these, not just the offending server. fn assert_registry_name_contract(name: &str) { assert!(!name.is_empty(), "server name must not be empty"); assert!( @@ -9475,6 +9631,17 @@ mod build_mcp_servers_tests { "server name is {} bytes, over McpRegistry's 128-byte limit: {name}", name.len() ); + assert!( + name.len() <= MAX_MCP_NAME_LEN, + "server name is {} bytes; with the `__` separator and a bare tool \ + name it cannot fit McpRegistry's {MAX_MCP_QNAME_LEN}-byte \ + qualified-name limit: {name}", + name.len() + ); + assert!( + name.len() + "__".len() < MAX_MCP_QNAME_LEN, + "no tool name at all fits after `{name}__`" + ); assert!( name.bytes() .all(|b| b.is_ascii_alphanumeric() || b == b'_' || b == b'-'), @@ -9487,11 +9654,11 @@ mod build_mcp_servers_tests { } #[test] - fn extra_mcp_commands_long_name_truncated_to_registry_limit() { - // A raw name longer than the registry's 128-byte ceiling must come - // back at or under it. An inclusive slice (`[..=128]`) yields 129 - // bytes, which McpRegistry rejects with "invalid server name" and - // fails the whole session — this test pins the exclusive cut. + fn extra_mcp_commands_long_name_cut_to_the_qualified_name_budget() { + // The registry's binding limit is on `__`, not on the + // name alone: a name cut to the looser 128-byte ceiling still fails + // the whole session the moment the server advertises a tool. The cut + // is budgeted against MAX_MCP_QNAME_LEN instead. let mut config = test_config(); let long_explicit = "a".repeat(130); let long_stem = "b".repeat(200); @@ -9503,16 +9670,22 @@ mod build_mcp_servers_tests { assert_eq!(servers.len(), 3, "primary + 2 extra = 3 servers"); assert_eq!( servers[1].name.len(), - 128, - "an over-long explicit name is cut to exactly 128 bytes" + MAX_MCP_NAME_LEN, + "an over-long explicit name is cut to the qualified-name budget" ); assert_eq!( servers[2].name.len(), - 128, - "an over-long executable stem is cut to exactly 128 bytes" + MAX_MCP_NAME_LEN, + "an over-long executable stem is cut to the qualified-name budget" ); for s in &servers { assert_registry_name_contract(&s.name); + // The point of the budget: a realistically-named tool still fits. + assert!( + s.name.len() + "__".len() + "search_files".len() <= MAX_MCP_QNAME_LEN, + "`{}__search_files` would be rejected by McpRegistry", + s.name + ); } } @@ -9522,7 +9695,7 @@ mod build_mcp_servers_tests { // `-2`, `-3`, ... disambiguation suffix is appended, including once // the suffix reaches two digits. let mut config = test_config(); - let at_ceiling = "c".repeat(128); + let at_ceiling = "c".repeat(MAX_MCP_NAME_LEN); config.extra_mcp_commands = (0..11).map(|i| format!("{at_ceiling}=srv-{i}")).collect(); let servers = build_mcp_servers(&config).unwrap(); assert_eq!(servers.len(), 12, "primary + 11 extra = 12 servers"); diff --git a/crates/buzz-acp/src/pool.rs b/crates/buzz-acp/src/pool.rs index 6190f2234d6..1b94e319b70 100644 --- a/crates/buzz-acp/src/pool.rs +++ b/crates/buzz-acp/src/pool.rs @@ -31,8 +31,8 @@ use uuid::Uuid; use crate::acp::{ extract_model_config_options, extract_model_state, extract_thought_level_config_id, - model_in_catalog, resolve_model_switch_method, AcpClient, AcpError, EnvVar, McpServer, - ModelSwitchMethod, StopReason, SystemPromptTransport, + model_in_catalog, resolve_model_switch_method, AcpClient, AcpError, ModelSwitchMethod, + StopReason, SystemPromptTransport, }; use crate::config::{compose_scoped_session_title, DedupMode, PermissionMode}; use crate::observer; @@ -43,6 +43,7 @@ use crate::queue::{ }; use crate::relay::{ChannelInfo, RestClient}; use crate::scope::SessionScope; +use crate::{McpServerSet, SessionOrigin}; /// Window within which agent activity before a hard-cap death qualifies /// the turn as "recently active" (eligible for requeue instead of dead-letter). @@ -756,7 +757,11 @@ impl ChannelInfoResolver { } pub struct PromptContext { - pub mcp_servers: Vec, + /// The MCP servers this harness offers, built once from [`crate::Config`]. + /// Held as an [`McpServerSet`] rather than a bare `Vec` so the only way to + /// fill it is the real parser, and the only way to read it back is + /// `for_session`, which applies the per-session git origin. + pub mcp_servers: McpServerSet, pub initial_message: Option, pub idle_timeout: Duration, pub max_turn_duration: Duration, @@ -1299,12 +1304,11 @@ async fn create_session_and_apply_model( channel.scope.and_then(SessionScope::root_event_id), ) }); - let mcp_servers = mcp_servers_with_git_origin( - &ctx.mcp_servers, - channel.scope.map(SessionScope::channel_id), - channel.channel_type, - ctx.session_title.as_deref(), - ); + let mcp_servers = ctx.mcp_servers.for_session(SessionOrigin { + channel_id: channel.scope.map(SessionScope::channel_id), + channel_type: channel.channel_type, + agent_name: ctx.session_title.as_deref(), + }); let resp = agent .acp @@ -1528,34 +1532,6 @@ async fn create_session_and_apply_model( Ok(resp.session_id) } -fn mcp_servers_with_git_origin( - servers: &[McpServer], - channel_id: Option, - channel_type: Option<&str>, - agent_name: Option<&str>, -) -> Vec { - let mut servers = servers.to_vec(); - let origin = match (channel_id, channel_type) { - (Some(channel_id), Some("stream")) => Some(EnvVar { - name: "BUZZ_GIT_ORIGIN_CHANNEL_ID".into(), - value: channel_id.to_string(), - }), - (Some(_), _) => agent_name - .filter(|name| !name.trim().is_empty()) - .map(|name| EnvVar { - name: "BUZZ_GIT_ORIGIN_AGENT_NAME".into(), - value: name.trim().to_string(), - }), - (None, _) => None, - }; - if let Some(origin) = origin { - for server in &mut servers { - server.env.push(origin.clone()); - } - } - servers -} - /// Outcome of a live model-switch RPC returned by [`apply_model_switch`]. /// /// `Applied` and `Rejected` are distinct outcomes and must not be collapsed: @@ -5145,6 +5121,8 @@ mod tests { SessionScope::Conversation { channel_id } } + use crate::acp::McpServer; + fn test_mcp_server() -> McpServer { McpServer { name: "dev".into(), @@ -5200,39 +5178,86 @@ mod tests { )); } + /// An untrusted extra server, exactly as `build_mcp_servers` emits one for + /// a `BUZZ_ACP_EXTRA_MCP_COMMANDS` entry. + fn untrusted_mcp_server() -> McpServer { + McpServer { + name: "extra".into(), + command: "third-party-mcp".into(), + args: vec![], + env: vec![], + trusted: false, + } + } + + fn has_env(server: &McpServer, name: &str) -> bool { + server.env.iter().any(|entry| entry.name == name) + } + #[test] fn public_session_forwards_channel_origin_to_mcp() { let channel_id = Uuid::new_v4(); - let servers = mcp_servers_with_git_origin( - &[test_mcp_server()], - Some(channel_id), - Some("stream"), - None, - ); + // A mixed array: the guard is only falsifiable when an untrusted + // server is present to be excluded. + let servers = McpServerSet::from_servers(vec![test_mcp_server(), untrusted_mcp_server()]) + .for_session(SessionOrigin { + channel_id: Some(channel_id), + channel_type: Some("stream"), + agent_name: None, + }); assert!(servers[0].env.iter().any(|entry| { entry.name == "BUZZ_GIT_ORIGIN_CHANNEL_ID" && entry.value == channel_id.to_string() })); - assert!(!servers[0] - .env - .iter() - .any(|entry| entry.name == "BUZZ_GIT_ORIGIN_AGENT_NAME")); + assert!(!has_env(&servers[0], "BUZZ_GIT_ORIGIN_AGENT_NAME")); + assert!( + !has_env(&servers[1], "BUZZ_GIT_ORIGIN_CHANNEL_ID"), + "an untrusted server was handed the channel UUID: {:?}", + servers[1].env + ); } #[test] fn private_session_forwards_agent_name_without_channel_id() { - let servers = mcp_servers_with_git_origin( - &[test_mcp_server()], - Some(Uuid::new_v4()), - Some("dm"), - Some("Builder"), - ); + let servers = McpServerSet::from_servers(vec![test_mcp_server(), untrusted_mcp_server()]) + .for_session(SessionOrigin { + channel_id: Some(Uuid::new_v4()), + channel_type: Some("dm"), + agent_name: Some("Builder"), + }); assert!(servers[0].env.iter().any(|entry| { entry.name == "BUZZ_GIT_ORIGIN_AGENT_NAME" && entry.value == "Builder" })); - assert!(!servers[0] - .env - .iter() - .any(|entry| entry.name == "BUZZ_GIT_ORIGIN_CHANNEL_ID")); + assert!(!has_env(&servers[0], "BUZZ_GIT_ORIGIN_CHANNEL_ID")); + assert!( + !has_env(&servers[1], "BUZZ_GIT_ORIGIN_AGENT_NAME"), + "an untrusted server was handed the agent name: {:?}", + servers[1].env + ); + } + + #[test] + fn untrusted_servers_receive_no_origin_on_any_channel_type() { + // Both origin shapes and the no-channel case, so no branch can hand a + // third-party process Buzz-native context. + for (channel_id, channel_type, agent_name) in [ + (Some(Uuid::new_v4()), Some("stream"), Some("Builder")), + (Some(Uuid::new_v4()), Some("dm"), Some("Builder")), + (Some(Uuid::new_v4()), None, Some("Builder")), + (None, Some("stream"), Some("Builder")), + ] { + let servers = McpServerSet::from_servers(vec![untrusted_mcp_server()]).for_session( + SessionOrigin { + channel_id, + channel_type, + agent_name, + }, + ); + assert!( + servers[0].env.is_empty(), + "untrusted server gained env for ({channel_id:?}, {channel_type:?}): {:?}", + servers[0].env + ); + } } // These pin the initial_message dispatch path (run_prompt_task, ~line 855): @@ -8660,7 +8685,7 @@ printf '%s\n' '{{"jsonrpc":"2.0","id":0,"result":{{"stopReason":"end_turn"}}}}'" ) -> PromptContext { use crate::relay::RestClient; PromptContext { - mcp_servers: vec![], + mcp_servers: McpServerSet::from_servers(vec![]), initial_message: None, idle_timeout: Duration::from_secs(60), max_turn_duration: Duration::from_secs(120), diff --git a/crates/buzz-agent/Cargo.toml b/crates/buzz-agent/Cargo.toml index 48c1e555cb4..a8359fd0af9 100644 --- a/crates/buzz-agent/Cargo.toml +++ b/crates/buzz-agent/Cargo.toml @@ -85,3 +85,6 @@ hex = { workspace = true } serde = { workspace = true } sha2 = { workspace = true } tempfile = "3" +# The extra-MCP end-to-end test names a channel origin, which buzz-acp types +# as a UUID. +uuid = { workspace = true } diff --git a/crates/buzz-agent/tests/untrusted_mcp_env_e2e.rs b/crates/buzz-agent/tests/untrusted_mcp_env_e2e.rs index 77cfa04d30e..7e8286545b7 100644 --- a/crates/buzz-agent/tests/untrusted_mcp_env_e2e.rs +++ b/crates/buzz-agent/tests/untrusted_mcp_env_e2e.rs @@ -29,6 +29,9 @@ use common::{openai_text, openai_tool_call, spawn_capturing_llm, Harness}; /// parseable key to build a config; nothing in this test signs anything. const TEST_PRIVATE_KEY: &str = "1111111111111111111111111111111111111111111111111111111111111111"; +/// A fixed channel id, so the git-origin assertions below name an exact value. +const CHANNEL_ID: uuid::Uuid = uuid::uuid!("00000000-0000-4000-8000-0000000000aa"); + /// Every tool name the agent offered the model in its `n`th request, however /// the provider nests it (`{type,function:{name}}` or a flat `{type,name}`). fn offered_tool_names(captured: &[Value], n: usize) -> Vec { @@ -71,7 +74,15 @@ async fn untrusted_mcp_env_extra_command_reaches_tool_list() { ]) .expect("buzz-acp CLI args parse"); let config = buzz_acp::Config::from_args(args).expect("buzz-acp config"); - let servers = buzz_acp::mcp_servers_wire_json(&config).expect("mcpServers wire JSON"); + // The whole request path, not just the parser: `mcp_servers_wire_json` + // runs `McpServerSet::from_config` and the per-session git-origin step the + // pool applies on the way out, which is the last hop before `session/new`. + let origin = buzz_acp::SessionOrigin { + channel_id: Some(CHANNEL_ID), + channel_type: Some("dm"), + agent_name: Some("Builder"), + }; + let servers = buzz_acp::mcp_servers_wire_json(&config, origin).expect("mcpServers wire JSON"); let decls = servers.as_array().expect("mcpServers is an array"); assert_eq!(decls.len(), 2, "primary + 1 extra: {servers}"); @@ -84,6 +95,31 @@ async fn untrusted_mcp_env_extra_command_reaches_tool_list() { decls[1].get("trusted").is_none(), "an extra server is untrusted by omission: {servers}" ); + // The git-origin hop: a private (non-stream) channel contributes the agent + // name, and only to the trusted server. Breaking either half of + // `McpServerSet::for_session` fails here. + let env_names = |decl: &Value| -> Vec { + decl["env"] + .as_array() + .map(|entries| { + entries + .iter() + .filter_map(|e| e["name"].as_str().map(str::to_owned)) + .collect() + }) + .unwrap_or_default() + }; + assert!( + env_names(&decls[0]).contains(&"BUZZ_GIT_ORIGIN_AGENT_NAME".to_string()), + "the trusted server lost its git origin: {servers}" + ); + for var in ["BUZZ_GIT_ORIGIN_AGENT_NAME", "BUZZ_GIT_ORIGIN_CHANNEL_ID"] { + assert!( + !env_names(&decls[1]).contains(&var.to_string()), + "the untrusted extra server was handed {var}: {servers}" + ); + } + let primary_name = decls[0]["name"].as_str().expect("primary name").to_owned(); let extra_name = decls[1]["name"].as_str().expect("extra name").to_owned(); assert_ne!( From 99310c917454809d9a7f328658f5a6119df04b99 Mon Sep 17 00:00:00 2001 From: wiggdevin <202901685+wiggdevin@users.noreply.github.com> Date: Fri, 4 Sep 2026 06:23:44 -0700 Subject: [PATCH 10/11] fix(buzz-agent): contain an over-long third-party tool name; narrow the trust claims MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit An untrusted MCP server's tool names are outside this repo's control: `valid_name` accepts 128 bytes, while a server name generated by buzz-acp can only reserve about 30 for the bare tool. A 32-byte server name plus a real 32-byte tool (`get_google_search_console_report`) makes a 66-byte qualified name, and `spawn_all` returned Err for the whole array — so one operator-supplied server took the built-in one down with it and the agent started a session with no tools at all. Drop that one tool, with a warning, when the server is untrusted; a trusted server keeps the hard error, because there an over-long name is our own packaging bug. Narrow what the spawn filter is documented to buy. It removes four variable names from the child's environment. It is not isolation: the child runs under the same UID with HOME and SSH_AUTH_SOCK, so it can read what the agent's user can, including a plaintext nsec in managed-agents.json when the keyring is unreachable. Real per-server isolation belongs to the MCP-registry ticket. `trusted` is `#[serde(default)]`, so an ACP client that predates the extension declares every server untrusted — fail-safe for credentials, a behavior change for hooks. Document it on the field and in the README, and stop failing silently: an allowlisted-but-untrusted server is now logged once per registry, and an untrusted spawn says so. Tests: a real-process test with a 32-byte server name and a 32-byte tool (the session survives, the fitting tools are still offered, the long one is not), and a hook test whose declaration carries no `trusted` key at all. Signed-off-by: wiggdevin <202901685+wiggdevin@users.noreply.github.com> --- crates/buzz-agent/README.md | 6 +- crates/buzz-agent/src/lib.rs | 6 ++ crates/buzz-agent/src/mcp.rs | 73 ++++++++++++++++- crates/buzz-agent/src/types.rs | 23 ++++-- crates/buzz-agent/tests/regressions.rs | 82 ++++++++++++++++---- crates/buzz-agent/tests/untrusted_mcp_env.rs | 71 ++++++++++++++++- 6 files changed, 235 insertions(+), 26 deletions(-) diff --git a/crates/buzz-agent/README.md b/crates/buzz-agent/README.md index f2d68d4d8cd..75197892225 100644 --- a/crates/buzz-agent/README.md +++ b/crates/buzz-agent/README.md @@ -288,7 +288,11 @@ Example: a single echo MCP server. } ``` -Multiple servers: just add more entries. Tool calls fan out to the right server by namespace prefix. +Multiple servers: just add more entries, up to 16 per session. Tool calls fan out to the right server by namespace prefix. + +**The `trusted` marker.** Each entry takes an optional boolean `trusted`, defaulting to `false`. A server the client does not mark `trusted` is spawned without the four Buzz identity variables (`BUZZ_PRIVATE_KEY`, `NOSTR_PRIVATE_KEY`, `BUZZ_RELAY_URL`, `BUZZ_AUTH_TAG`), whether they come from this process's environment or from the entry's own `env` block, and never runs a `_Stop` or `_PostCompact` hook, whatever `MCP_HOOK_SERVERS` allows. That is all it does: the child still runs under the agent's UID with `HOME` and `SSH_AUTH_SOCK`, so it can read what that user can read. It is not a sandbox. + +Because the field defaults to `false`, **a client that does not know about it declares every server untrusted** — fail-safe for credentials, but a behavior change for hooks: a client that spawns `buzz-dev-mcp` itself and relies on `_Stop` must now send `"trusted": true` for it. The agent logs the skip once per server when `MCP_HOOK_SERVERS` names a server that is not trusted. **Transport: stdio only.** No HTTP, no SSE. We advertise this in `agentCapabilities` (`mcpCapabilities.http: false`, `mcpCapabilities.sse: false`); spec-compliant clients won't ask for what we don't have. diff --git a/crates/buzz-agent/src/lib.rs b/crates/buzz-agent/src/lib.rs index 3de47c82a4a..4e8bbebf50b 100644 --- a/crates/buzz-agent/src/lib.rs +++ b/crates/buzz-agent/src/lib.rs @@ -17,6 +17,12 @@ pub use catalog::{ discover_databricks_models, discover_databricks_models_with_cache_dir, ModelEntry, }; pub use config::Provider; +/// The registry's cap on how many MCP servers one `session/new` may carry. +/// +/// Re-exported so `buzz-acp`, which has to refuse an over-long configuration +/// at startup, can pin its mirror of this bound in a test rather than in a +/// comment. +pub use mcp::MAX_MCP_SERVERS; pub use types::AgentError; /// Environment keys the Windows Git Bash resolver may inspect. `spawn_one()` diff --git a/crates/buzz-agent/src/mcp.rs b/crates/buzz-agent/src/mcp.rs index 1be0987b419..eb7e72b38ef 100644 --- a/crates/buzz-agent/src/mcp.rs +++ b/crates/buzz-agent/src/mcp.rs @@ -200,6 +200,9 @@ pub struct McpRegistry { init_timeout: Duration, /// Consecutive hook timeout count per server. Kill on second consecutive. hook_timeouts: std::sync::Mutex>, + /// Servers already reported as allowlisted-but-untrusted, so the warning + /// is emitted once rather than on every turn. + hook_trust_warned: std::sync::Mutex>, } impl McpRegistry { @@ -224,6 +227,7 @@ impl McpRegistry { backoff_max: Duration::from_millis(cfg.mcp_restart_max_ms.max(1)), init_timeout: cfg.mcp_init_timeout, hook_timeouts: std::sync::Mutex::new(HashMap::new()), + hook_trust_warned: std::sync::Mutex::new(HashSet::new()), }; let mut seen_names = HashSet::new(); @@ -275,6 +279,23 @@ impl McpRegistry { } let qname = format!("{}{SEP}{}", s.name, bare); if qname.len() > MAX_QNAME_LEN { + // A trusted server is the harness's own: an over-long + // qualified name there is a packaging bug and stays fatal. + // An untrusted server is operator-supplied and its tool + // names are outside this repo's control — `valid_name` + // allows 128 bytes, while a name generated for the server + // can only reserve about 30. One long third-party tool + // name must cost that tool, not the session, which would + // take the built-in server's tools down with it. + if !s.trusted { + tracing::warn!( + "MCP server '{}': tool '{bare}' not registered — qualified name \ + is {} bytes, over the {MAX_QNAME_LEN}-byte limit", + s.name, + qname.len() + ); + continue; + } return Err(AgentError::Mcp(format!( "qualified tool name too long: {} ({} > {MAX_QNAME_LEN})", qname, @@ -347,6 +368,23 @@ impl McpRegistry { /// `(server_name, text)` pairs in **config order** (deterministic), /// dropping empty/whitespace-only responses, errors and timeouts. /// Hooks are fail-open and must never block the agent. + /// Report an allowlisted server that cannot run hooks because it was not + /// declared `trusted`, at most once per server per registry. + fn warn_hook_untrusted_once(&self, name: &str) { + let first = match self.hook_trust_warned.lock() { + Ok(mut warned) => warned.insert(name.to_owned()), + // A poisoned lock must not cost the log line; at worst it repeats. + Err(poisoned) => poisoned.into_inner().insert(name.to_owned()), + }; + if first { + tracing::warn!( + "MCP server '{name}' is named by MCP_HOOK_SERVERS but was not declared \ + `trusted`; its hooks will not run. An ACP client that does not send the \ + `trusted` marker declares every server untrusted." + ); + } + } + pub async fn call_hooks( self: &Arc, hook_name: &str, @@ -369,6 +407,15 @@ impl McpRegistry { // are never hook targets, whatever the allowlist says — including // the wildcard hook-capable runtimes are launched with. if !server.spec.trusted { + // An ACP client that predates the `trusted` marker declares + // every server untrusted by omission, which silently turns + // hooks off for a server the operator explicitly allowlisted. + // Say so — once per server, since call_hooks runs on every + // turn — so the cause is in the log rather than inferred from + // a hook that never fires. + if allowed.allows(&server.name) { + self.warn_hook_untrusted_once(&server.name); + } continue; } if !allowed.allows(&server.name) { @@ -749,11 +796,29 @@ async fn spawn_one( let mut cmd = Command::new(&spec.command); cmd.args(&spec.args); cmd.env_clear(); + if !spec.trusted { + // Say it once per spawn, so an operator can see the boundary was + // applied — and, when a server misbehaves for want of a variable, + // why. + tracing::info!( + "MCP server '{}' is untrusted: withholding the Buzz identity variables", + spec.name + ); + } for k in PASSTHROUGH_ENV { - // Withhold Buzz identity credentials from untrusted MCP servers so - // third-party tooling cannot exfiltrate the agent's signing key, - // relay URL, or owner attestation. Only the built-in buzz-dev-mcp - // server (marked `trusted`) receives these. + // Withhold the four Buzz identity variables (BUZZ_PRIVATE_KEY, + // NOSTR_PRIVATE_KEY, BUZZ_RELAY_URL, BUZZ_AUTH_TAG) from an untrusted + // MCP server. That is the whole of what this filter buys: those four + // names are absent from the child's environment. It is not process + // isolation — the child runs under this process's UID and keeps HOME + // and SSH_AUTH_SOCK from PASSTHROUGH_ENV, so it can read whatever the + // agent's user can, including a plaintext key in `managed-agents.json` + // when the OS keyring is unreachable. It also only covers servers this + // registry spawns: a third-party ACP adapter that spawns its own MCP + // children inherits the harness's full environment, which is why + // `buzz-acp` refuses `BUZZ_ACP_EXTRA_MCP_COMMANDS` unless it is + // driving buzz-agent (see `crates/buzz-acp/README.md`). Real + // per-server isolation belongs to the MCP-registry trust model. if !spec.trusted && is_buzz_identity_env(k) { continue; } diff --git a/crates/buzz-agent/src/types.rs b/crates/buzz-agent/src/types.rs index 337d6e32a3e..6ac97b7dca1 100644 --- a/crates/buzz-agent/src/types.rs +++ b/crates/buzz-agent/src/types.rs @@ -541,12 +541,23 @@ pub struct McpServerStdio { pub args: Vec, #[serde(default)] pub env: Vec, - /// When `false`, the spawn boundary withholds Buzz identity credentials - /// (`BUZZ_PRIVATE_KEY`, `NOSTR_PRIVATE_KEY`, `BUZZ_RELAY_URL`, - /// `BUZZ_AUTH_TAG`) from the child - /// process so third-party MCP servers cannot exfiltrate the agent's - /// signing key or owner attestation. Only the built-in `buzz-dev-mcp` - /// server sets this to `true`. + /// Trust marker for this server, set by the ACP client that declared it. + /// + /// When `false`, two things follow. The spawn boundary keeps the four Buzz + /// identity variables (`BUZZ_PRIVATE_KEY`, `NOSTR_PRIVATE_KEY`, + /// `BUZZ_RELAY_URL`, `BUZZ_AUTH_TAG`) out of the child's environment — no + /// more and no less than that: the child still runs under this process's + /// UID and can read what the agent's user can. And `call_hooks` never runs + /// `_Stop` or `_PostCompact` on the server, whatever `MCP_HOOK_SERVERS` + /// says. Only the built-in `buzz-dev-mcp` server is declared `true`. + /// + /// The field is `#[serde(default)]`, so an ACP client that predates this + /// extension — Zed, JetBrains, anything but `buzz-acp` — declares every + /// server untrusted by omission. For credentials that is fail-safe; for + /// hooks it is a **behavior change**: such a client's own `buzz-dev-mcp` + /// declaration silently stops running hooks (logged once per server, see + /// `mcp::McpRegistry::call_hooks`). Those clients must send + /// `"trusted": true` for the servers they own. #[serde(default)] pub trusted: bool, } diff --git a/crates/buzz-agent/tests/regressions.rs b/crates/buzz-agent/tests/regressions.rs index 75e2a454cc5..ae534cbc65c 100644 --- a/crates/buzz-agent/tests/regressions.rs +++ b/crates/buzz-agent/tests/regressions.rs @@ -558,36 +558,40 @@ async fn init_session_with_fake_mcp(h: &mut Harness, extra_mcp_env: &[(&str, &st } /// As [`init_session_with_fake_mcp`], with the server's `trusted` flag under -/// the caller's control. `trusted: false` is exactly how `buzz-acp` declares an -/// operator-supplied `BUZZ_ACP_EXTRA_MCP_COMMANDS` server. +/// the caller's control. `Some(false)` is how `buzz-acp` declares an +/// operator-supplied `BUZZ_ACP_EXTRA_MCP_COMMANDS` server; `None` omits the +/// field entirely, which is how every ACP client that predates the extension +/// declares every server. async fn init_session_with_fake_mcp_trust( h: &mut Harness, extra_mcp_env: &[(&str, &str)], - trusted: bool, + trusted: impl Into>, ) -> String { + let trusted = trusted.into(); let fake_mcp = env!("CARGO_BIN_EXE_fake-mcp"); let env: Vec = extra_mcp_env .iter() .map(|(k, v)| json!({ "name": k, "value": v })) .collect(); + let marker = trusted.map(Value::from); h.send( "initialize", json!({"protocolVersion":1,"clientCapabilities":{}}), ) .await; let _ = h.recv().await; + let mut decl = json!({ + "name": "fake", + "command": fake_mcp, + "args": [], + "env": env, + }); + if let Some(marker) = marker { + decl["trusted"] = marker; + } h.send( "session/new", - json!({ - "cwd": "/tmp", - "mcpServers": [{ - "name": "fake", - "command": fake_mcp, - "args": [], - "env": env, - "trusted": trusted, - }], - }), + json!({ "cwd": "/tmp", "mcpServers": [decl] }), ) .await; let r = h @@ -756,6 +760,58 @@ async fn hook_never_invoked_on_untrusted_server() { h.shutdown().await; } +/// A server declared with **no** `trusted` field runs no hooks either. +/// +/// `trusted` is `#[serde(default)]`, so an ACP client that predates the +/// extension — Zed, JetBrains, anything but `buzz-acp` — declares every server +/// untrusted by omission, including the built-in `buzz-dev-mcp` it spawned +/// itself. That is a real compatibility break for hooks, documented on +/// `McpServerStdio::trusted` and in `crates/buzz-acp/README.md`; this test +/// pins the behavior so it cannot change silently in either direction. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn hook_never_invoked_when_trusted_marker_absent() { + let llm = spawn_capturing_llm(vec![openai_text("done"), openai_text("looped")]).await; + let mut h = Harness::spawn_with_env( + &llm.url, + &[ + ("MCP_HOOK_SERVERS", "fake"), + ("BUZZ_AGENT_STOP_MAX_REJECTIONS", "10"), + ], + ) + .await; + // `None`: the declaration carries no `trusted` key at all. + let sid = init_session_with_fake_mcp_trust( + &mut h, + &[ + ("FAKE_MCP_TOOL_COUNT", "1"), + ("FAKE_MCP_STOP_HOOK", "1"), + ("FAKE_MCP_STOP_TEXT", "you have open work"), + ("FAKE_MCP_STOP_COUNT", "1"), + ], + None, + ) + .await; + + let p = h + .send( + "session/prompt", + json!({"sessionId": sid, "prompt": [{"type":"text","text":"go"}]}), + ) + .await; + let r = h.recv_until_approving(|v| v["id"] == json!(p)).await; + assert!(r.get("result").is_some(), "errored: {r}"); + assert_eq!(r["result"]["stopReason"], "end_turn"); + + let captured = llm.captured.lock().await; + assert_eq!( + captured.len(), + 1, + "a marker-absent server's _Stop hook ran and made the agent loop: {} LLM calls", + captured.len() + ); + h.shutdown().await; +} + /// After `stop_max_rejections` objections, the agent honors end_turn /// even if `_Stop` would still object. Set max=1 so it trips quickly. #[tokio::test(flavor = "multi_thread", worker_threads = 2)] diff --git a/crates/buzz-agent/tests/untrusted_mcp_env.rs b/crates/buzz-agent/tests/untrusted_mcp_env.rs index ac13e7f6ccb..4a7ac0cc383 100644 --- a/crates/buzz-agent/tests/untrusted_mcp_env.rs +++ b/crates/buzz-agent/tests/untrusted_mcp_env.rs @@ -42,8 +42,10 @@ fn identity_env() -> Vec<(&'static str, &'static str)> { } /// Declare one fake MCP server on the wire. `trusted` is emitted only when -/// true so the untrusted case exercises the serde default, exactly as -/// `buzz-acp` writes it. +/// true, so every untrusted case below is also the *marker-absent* case: the +/// field is missing from the JSON entirely, which is both how `buzz-acp` +/// writes an extra server and how any ACP client that predates the extension +/// declares every server. The serde default carries the whole boundary. fn server_decl(name: &str, trusted: bool) -> Value { let mut decl = json!({ "name": name, @@ -276,3 +278,68 @@ async fn untrusted_mcp_env_keeps_non_identity_passthrough() { h.shutdown().await; } + +/// A server name at the generated ceiling plus a tool name longer than the +/// reserve overflows `__` — and must cost that one tool, not the +/// session. +/// +/// `buzz-acp` caps a generated name at 32 bytes, reserving 32 for the +/// separator and the bare tool name, but `buzz-agent` accepts bare tool names +/// up to 128 bytes and the fork does not control what a third-party server +/// advertises. Failing the whole array would let one operator-supplied server +/// take the built-in one down with it, leaving the agent with no tools at all. +#[tokio::test(flavor = "multi_thread", worker_threads = 2)] +async fn untrusted_mcp_env_over_long_tool_name_costs_only_that_tool() { + // 32 bytes: exactly what buzz-acp's MAX_MCP_NAME_LEN produces. + let long_server = "a".repeat(32); + // 32 bytes, a real tool name from a shipped MCP server. 32 + 2 + 32 = 66, + // over the registry's 64-byte qualified-name limit. + let long_tool = "get_google_search_console_report"; + let mut extra = server_decl(&long_server, false); + extra["env"] + .as_array_mut() + .expect("declaration has an env array") + .push(json!({ "name": "FAKE_MCP_NAMED_TOOLS", "value": long_tool })); + + let llm = spawn_capturing_llm(vec![openai_text("done")]).await; + let mut h = Harness::spawn(&llm.url).await; + // `new_session` asserts session/new succeeded: before this guard the whole + // array was rejected and this line failed. + let sid = new_session(&mut h, vec![server_decl("devmcp", true), extra]).await; + + let p = h + .send( + "session/prompt", + json!({"sessionId": sid, "prompt": [{"type":"text","text":"go"}]}), + ) + .await; + let r = h.recv_until_approving(|v| v["id"] == json!(p)).await; + assert!(r.get("error").is_none(), "prompt errored: {r}"); + + let captured = llm.captured.lock().await; + let offered: Vec = captured[0]["tools"] + .as_array() + .unwrap_or_else(|| panic!("no tools array: {}", captured[0])) + .iter() + .filter_map(|t| { + t.get("name") + .or_else(|| t.get("function").and_then(|f| f.get("name"))) + .and_then(Value::as_str) + .map(str::to_owned) + }) + .collect(); + assert!( + offered.contains(&"devmcp__tool_0".to_owned()), + "the trusted server lost its tools to the other server's long name: {offered:?}" + ); + assert!( + offered.contains(&format!("{long_server}__tool_0")), + "the untrusted server's fitting tool was dropped too: {offered:?}" + ); + assert!( + !offered.iter().any(|name| name.contains(long_tool)), + "the over-long qualified name was registered: {offered:?}" + ); + + h.shutdown().await; +} From 9308e5305d4a2bc9a56f2ef5807463b285f99414 Mon Sep 17 00:00:00 2001 From: wiggdevin <202901685+wiggdevin@users.noreply.github.com> Date: Fri, 4 Sep 2026 06:23:57 -0700 Subject: [PATCH 11/11] fix(buzz-acp): refuse extra MCP servers an adapter cannot honour, and bound them MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The credential-withholding guarantee these servers are declared under is `buzz-agent`'s: it alone reads the `trusted` marker. `BUZZ_ACP_AGENT_COMMAND` defaults to `goose`, `AcpClient::spawn` never clears the environment, and the extras went into every `session/new` regardless — so with any other adapter the entries were spawned by a process holding BUZZ_PRIVATE_KEY and BUZZ_RELAY_URL, and the marker, the spawn filter and their tests all sat off that path. Gate the extras on the same normalized command identity `default_agent_args` keys on, and fail at boot with a message that names the adapter. Mirror the registry's 16-server cap. `spawn_all` rejects the whole array past it, so without a startup bound a 17-server configuration produced a harness that came online and then failed every session — the same class the MAX_QNAME_LEN mirror fixed. A re-export from buzz-agent lets a test pin the two constants together instead of a comment. Make the `name=` promise true. Two entries claiming one explicit name are now refused instead of one being silently renamed by position, and a disambiguation suffix is hashed from the entry itself rather than counted off in order, so reordering two names that truncate alike no longer swaps which executable owns each qualified tool name. Build the server set before presence goes online, so an unusable configuration stops the process at boot rather than after it advertises itself. Narrow the doc comments to what the trust marker actually buys, and replace the CLI/README example that put an API key in argv. Signed-off-by: wiggdevin <202901685+wiggdevin@users.noreply.github.com> --- crates/buzz-acp/README.md | 2 +- crates/buzz-acp/src/acp.rs | 14 +- crates/buzz-acp/src/config.rs | 30 +- crates/buzz-acp/src/lib.rs | 587 ++++++++++++++---- .../buzz-agent/tests/untrusted_mcp_env_e2e.rs | 18 + .../2026-09-04-zs-implementation-plan.md | 1 + 6 files changed, 498 insertions(+), 154 deletions(-) diff --git a/crates/buzz-acp/README.md b/crates/buzz-acp/README.md index ce72b4156e4..ef22d33c531 100644 --- a/crates/buzz-acp/README.md +++ b/crates/buzz-acp/README.md @@ -111,7 +111,7 @@ All configuration is via environment variables (or CLI flags — every env var h | `BUZZ_ACP_AGENT_COMMAND` | no | `goose` | Agent binary to spawn. | | `BUZZ_ACP_AGENT_ARGS` | no | `acp` | Agent arguments (comma-separated). | | `BUZZ_ACP_MCP_COMMAND` | no | `""` (empty) | Path to an optional MCP server binary to provide to the agent subprocess. | -| `BUZZ_ACP_EXTRA_MCP_COMMANDS` | no | — | Newline-separated additional MCP server commands. Each entry is shell-split with POSIX quoting (e.g. `npx -y my-mcp-server`). An optional `name=` prefix sets the server name explicitly (e.g. `memory=npx -y memory-mcp`), so reordering entries does not silently rename a server and strip the agent of its tools. Without a prefix, names are derived from the executable stem, sanitized to ASCII alphanumeric/hyphen, and disambiguated with numeric suffixes. Names are capped at 32 bytes (suffix included), budgeted against `buzz-agent`'s 64-byte cap on the qualified tool name `__` so a generated name always leaves room for the tools it carries. Extra servers do **not** receive `BUZZ_PRIVATE_KEY`, `NOSTR_PRIVATE_KEY`, `BUZZ_RELAY_URL`, or `BUZZ_AUTH_TAG` on the `buzz-agent` MCP spawn path — they are third-party tools, not Buzz-native MCP. **Note:** the credential isolation applies to MCP servers spawned directly by `buzz-agent`'s `McpRegistry`. When using a third-party ACP adapter (e.g. `claude-agent-acp`, `codex-acp`) that spawns its own MCP children, the adapter process inherits the full parent environment including `BUZZ_PRIVATE_KEY`; operators should assume those children can access Buzz credentials unless the adapter itself isolates them. An entry that is malformed, or that shell-splits to no command at all (`memory=`, a comment-only line), fails startup with the entry index (the raw entry is not logged). The primary server is optional: an extras-only configuration is valid, and only an entirely unset one yields no servers. | +| `BUZZ_ACP_EXTRA_MCP_COMMANDS` | no | — | Newline-separated additional MCP server commands, each shell-split with POSIX quoting (e.g. `npx -y my-mcp-server`). **Requires `BUZZ_ACP_AGENT_COMMAND=buzz-agent`**: only `buzz-agent` reads the `trusted` marker the harness puts on these servers, so under any other adapter the harness fails at startup with that message rather than handing the entries to a process that spawns them itself out of an environment holding `BUZZ_PRIVATE_KEY`. An optional `name=` prefix sets the server name explicitly (e.g. `memory=npx -y memory-mcp`); without it the name is derived from the executable stem. Names are sanitized to ASCII alphanumeric/hyphen and capped at 32 bytes, budgeted against `buzz-agent`'s 64-byte cap on the qualified tool name `__` — so a tool name up to 30 bytes always fits, and a longer one costs that single tool (the registry drops it with a warning) rather than the session. Two entries whose names collide — including two long names that truncate alike — each take a suffix hashed from the entry itself, so reordering entries never moves a name from one executable to another. Startup fails, naming the entry number and never echoing the entry, when an entry has malformed quoting, names no command (`memory=`, a comment-only line), repeats a `name=` prefix, resolves to a name another entry already has, or pushes the total past `buzz-agent`'s 16-server limit. Extra servers are declared untrusted, which means exactly two things on the `buzz-agent` spawn path: the four variables `BUZZ_PRIVATE_KEY`, `NOSTR_PRIVATE_KEY`, `BUZZ_RELAY_URL` and `BUZZ_AUTH_TAG` are absent from the child's environment, and the server never runs a `_Stop` / `_PostCompact` hook. It is **not** process isolation — the child runs under the same user as the agent and can read what that user can. Keep secrets out of the entry itself: the argv is visible in `ps` and in crash dumps, so prefer a server that reads its key from its own config or from the agent's environment (per-server `env` is not supported yet). Example: `memory=npx -y memory-mcp --db /var/lib/memory.db` and `other-server --port 8080` on separate lines. The primary server is optional: an extras-only configuration is valid, and only an entirely unset one yields no servers. | | `BUZZ_ACP_IDLE_TIMEOUT` | no | `620` | Idle timeout: max seconds of silence before cancelling a turn. Resets on any agent stdout activity. | | `BUZZ_ACP_MAX_TURN_DURATION` | no | `7200` | Absolute wall-clock cap per turn (safety valve). | | `BUZZ_API_TOKEN` | no | — | API token (required if relay enforces token auth). | diff --git a/crates/buzz-acp/src/acp.rs b/crates/buzz-acp/src/acp.rs index ec0457d6993..f2b55106298 100644 --- a/crates/buzz-acp/src/acp.rs +++ b/crates/buzz-acp/src/acp.rs @@ -28,12 +28,14 @@ const MAX_LINE_SIZE: usize = 10_000_000; // 10 MB /// `name`, `command`, `args` and `env` are **required** by the schema (`args` /// and `env` may be empty arrays); `trusted` is a Buzz extension and defaults /// to `false` when absent. -/// `trusted` controls whether the agent runtime passes Buzz identity credentials -/// (`BUZZ_PRIVATE_KEY`, `NOSTR_PRIVATE_KEY`, `BUZZ_RELAY_URL`, `BUZZ_AUTH_TAG`) -/// into the child process. -/// Only the built-in `buzz-dev-mcp` server is trusted; extra MCP servers -/// configured via `BUZZ_ACP_EXTRA_MCP_COMMANDS` are untrusted and receive no -/// Buzz credentials. +/// `trusted` is honoured by `buzz-agent` alone, and buys exactly two things +/// there: the four Buzz identity variables (`BUZZ_PRIVATE_KEY`, +/// `NOSTR_PRIVATE_KEY`, `BUZZ_RELAY_URL`, `BUZZ_AUTH_TAG`) are absent from an +/// untrusted child's environment, and an untrusted server never runs a +/// `_Stop` / `_PostCompact` hook. It is not process isolation: the child runs +/// under the harness's own UID. Only the built-in `buzz-dev-mcp` server is +/// trusted; the servers `BUZZ_ACP_EXTRA_MCP_COMMANDS` adds are not, and the +/// harness refuses those entries outright under any adapter but `buzz-agent`. #[derive(Debug, Clone, serde::Serialize)] pub struct McpServer { pub name: String, diff --git a/crates/buzz-acp/src/config.rs b/crates/buzz-acp/src/config.rs index 9b2cc77e5fa..692154e2ded 100644 --- a/crates/buzz-acp/src/config.rs +++ b/crates/buzz-acp/src/config.rs @@ -268,19 +268,23 @@ pub struct CliArgs { pub mcp_command: String, /// Additional MCP server commands to pass to the agent session alongside - /// the primary MCP server. Entries are newline-separated; each entry is - /// shell-split (shlex) into a command and its args, so quoted paths and - /// arguments with spaces are preserved. An optional `name=` prefix sets - /// the server name explicitly (e.g. `memory=npx -y memory-mcp`); without - /// it, the name is derived from the executable stem. Duplicate names are - /// disambiguated with a numeric suffix (e.g. two `npx` wrappers become - /// `npx` and `npx-2`), but explicit names are preferred so reordering - /// entries does not silently rename a server. Names are capped at 128 - /// bytes; keep them well under that, since `buzz-agent` also caps the - /// qualified tool name `__` at 64 bytes. Entries with - /// malformed quoting fail startup with the entry index (the raw command - /// is not echoed). Example: - /// `memory=npx -y mcp-remote https://mcp.tavily.com/mcp/?tavilyApiKey=...\nother-server --port 8080` + /// the primary MCP server. Requires `--agent-command buzz-agent`: only + /// that adapter honours the trust marker these servers carry, so any + /// other one fails at startup instead of spawning them out of a process + /// holding the Buzz private key. Entries are newline-separated; each + /// entry is shell-split (shlex) into a command and its args, so quoted + /// paths and arguments with spaces are preserved. An optional `name=` + /// prefix sets the server name explicitly (e.g. `memory=npx -y + /// memory-mcp`); without it, the name is derived from the executable + /// stem. Names are capped at 32 bytes, budgeted against `buzz-agent`'s + /// 64-byte cap on the qualified tool name `__`; colliding + /// names take a suffix hashed from the entry, so reordering entries never + /// renames a server. Startup fails, naming the entry index and never + /// echoing the entry, on malformed quoting, an entry naming no command, a + /// repeated `name=`, two entries resolving to one name, or more than 16 + /// servers in total. Do not put a secret in an entry — argv is visible in + /// `ps`; prefer a server that reads its own credentials. Example: + /// `memory=npx -y memory-mcp --db /var/lib/memory.db\nother-server --port 8080` #[arg(long, env = "BUZZ_ACP_EXTRA_MCP_COMMANDS", value_delimiter = '\n')] pub extra_mcp_commands: Vec, diff --git a/crates/buzz-acp/src/lib.rs b/crates/buzz-acp/src/lib.rs index a36d010bf5f..0eb5cd92b73 100644 --- a/crates/buzz-acp/src/lib.rs +++ b/crates/buzz-acp/src/lib.rs @@ -2519,6 +2519,17 @@ async fn tokio_main() -> Result<()> { return setup_mode::run_setup_listener(config, payload).await; } + // Build the MCP server set before this harness announces itself. An + // unusable configuration — an adapter that cannot honour the `trusted` + // marker, a malformed entry, more servers than the registry accepts — has + // to stop the process here, at boot, rather than after presence goes + // online and every session/new starts failing. The setup-mode branch above + // returns first on purpose: it never opens a session, and an agent whose + // credentials are still missing must stay reachable so the desktop can + // finish configuring it. + let mcp_servers = McpServerSet::from_config(&config) + .map_err(|e| anyhow::anyhow!("configuration error: {e}"))?; + tracing::info!("buzz-acp starting: {}", config.summary()); let observer = config @@ -2772,7 +2783,7 @@ async fn tokio_main() -> Result<()> { let base_prompt_content = config.base_prompt_content.take(); let cwd = current_working_directory()?; let ctx = Arc::new(PromptContext { - mcp_servers: McpServerSet::from_config(&config)?, + mcp_servers, initial_message: config.initial_message.clone(), idle_timeout: Duration::from_secs(config.idle_timeout_secs), max_turn_duration: Duration::from_secs(config.max_turn_duration_secs), @@ -5767,8 +5778,11 @@ impl McpServerSet { /// /// # Errors /// - /// Returns [`ConfigError::ConfigFile`] when an entry of - /// `BUZZ_ACP_EXTRA_MCP_COMMANDS` is malformed or names no command. + /// Returns [`ConfigError::ConfigFile`] when `BUZZ_ACP_EXTRA_MCP_COMMANDS` + /// cannot be served: the agent command is not the one adapter that + /// honours the `trusted` marker, an entry is malformed or names no + /// command, two entries claim one name, or the total passes + /// [`MAX_MCP_SERVERS`]. See [`append_extra_mcp_servers`]. pub fn from_config(config: &Config) -> Result { Ok(Self(build_mcp_servers(config)?)) } @@ -5822,9 +5836,9 @@ impl McpServerSet { /// /// # Errors /// -/// Returns [`ConfigError::ConfigFile`] when an entry of -/// `BUZZ_ACP_EXTRA_MCP_COMMANDS` is malformed, or when the resulting array -/// cannot be serialized. +/// Returns [`ConfigError::ConfigFile`] when `BUZZ_ACP_EXTRA_MCP_COMMANDS` +/// cannot be served (see [`McpServerSet::from_config`]), or when the resulting +/// array cannot be serialized. pub fn mcp_servers_wire_json( config: &Config, origin: SessionOrigin<'_>, @@ -5835,25 +5849,55 @@ pub fn mcp_servers_wire_json( } /// `buzz-agent`'s cap on a *qualified* tool name (`__`), mirrored -/// from `MAX_QNAME_LEN` in its `mcp` module. Passing it fails the whole -/// session, not just the offending server. +/// from `MAX_QNAME_LEN` in its `mcp` module. Passing it costs the offending +/// tool, and on a trusted server the whole session. const MAX_MCP_QNAME_LEN: usize = 64; /// Bytes reserved inside [`MAX_MCP_QNAME_LEN`] for the `__` separator and the -/// bare tool name. Wider than any tool `buzz-dev-mcp` advertises, so a name -/// generated here can carry the tools it was generated for. +/// bare tool name. +/// +/// This is a budget, not a guarantee. `buzz-agent` accepts a bare tool name of +/// up to 128 bytes, so a third-party server advertising a tool longer than +/// `MCP_TOOL_NAME_RESERVE` minus the two separator bytes still overflows the +/// qualified name. What the reserve buys is that every tool at or under 30 +/// bytes fits — `buzz-dev-mcp`'s entire surface and ordinary third-party +/// naming. The overflow is contained on the other side of the wire: +/// `McpRegistry` drops that one tool of an *untrusted* server with a warning +/// instead of failing the session, so a long third-party tool name cannot take +/// the built-in server down with it (`crates/buzz-agent/src/mcp.rs`). const MCP_TOOL_NAME_RESERVE: usize = 32; /// Maximum length, in bytes, of a generated MCP server name. /// /// The binding constraint is not `buzz-agent`'s 128-byte `MAX_NAME_LEN` on the /// name alone. `McpRegistry` registers every tool as `__` and -/// rejects the entire session once that qualified name passes -/// [`MAX_MCP_QNAME_LEN`], so a name cut to 128 bytes is still fatal the moment -/// the server advertises anything. Budgeting against the qualified name is what -/// makes a generated name usable. +/// drops or rejects the tool once that qualified name passes +/// [`MAX_MCP_QNAME_LEN`], so a name cut to 128 bytes leaves no room for the +/// tools the server exists to offer. Budgeting against the qualified name is +/// what makes a generated name usable. const MAX_MCP_NAME_LEN: usize = MAX_MCP_QNAME_LEN - MCP_TOOL_NAME_RESERVE; +/// `buzz-agent`'s cap on how many MCP servers one `session/new` may carry, +/// mirrored from `buzz_agent::MAX_MCP_SERVERS`. +/// +/// `McpRegistry::spawn_all` rejects the whole array past this, so a harness +/// that accepted more would start, announce itself online, and then fail every +/// session. [`build_mcp_servers`] refuses at startup instead. +/// `mcp_server_cap_mirrors_buzz_agent`, in `buzz-agent`'s +/// `untrusted_mcp_env_e2e` test, pins the two constants together. +pub const MAX_MCP_SERVERS: usize = 16; + +/// The only ACP adapter identity that honours the `trusted` marker. +/// +/// The `mcpServers` array of a `session/new` reaches whatever adapter the +/// harness spawned, and only `buzz-agent` reads `trusted`: it withholds the +/// Buzz identity credentials from an untrusted child and refuses to run hooks +/// on one. Every other adapter spawns the declared servers itself, from a +/// process that inherited this one's environment — including +/// `BUZZ_PRIVATE_KEY`. Extras are therefore refused at startup under any other +/// adapter (see [`append_extra_mcp_servers`]). +const EXTRA_MCP_ADAPTER: &str = "buzz-agent"; + fn build_mcp_servers(config: &Config) -> Result, ConfigError> { let mut servers: Vec = Vec::new(); // The primary server is optional: `BUZZ_ACP_MCP_COMMAND` defaults to empty @@ -5916,116 +5960,243 @@ fn build_mcp_servers(config: &Config) -> Result, ConfigError> { }); } - // Append extra MCP servers from BUZZ_ACP_EXTRA_MCP_COMMANDS. - // Each entry is newline-separated and shell-split into command + args - // using shlex, so quoted paths and arguments with spaces (and commas) - // are preserved. An optional `name=` prefix sets the server name - // explicitly, so reordering entries does not silently rename a server - // and strip the agent of its tools. Malformed entries cause startup to - // fail closed — the error identifies the entry index without echoing - // the command, which may contain an embedded API key. - // Extra servers do not receive Buzz relay credentials or auth tags — - // they are third-party tools, not Buzz-native MCP servers. - let mut seen_names: std::collections::HashSet = - servers.iter().map(|s| s.name.clone()).collect(); - for (idx, extra) in config.extra_mcp_commands.iter().enumerate() { - let trimmed = extra.trim(); - if trimmed.is_empty() { - continue; + append_extra_mcp_servers(config, &mut servers)?; + Ok(servers) +} + +/// A parsed `BUZZ_ACP_EXTRA_MCP_COMMANDS` entry, before names are shaped. +struct ParsedExtra { + /// 1-based entry number, the only part of an entry an error may name. + entry: usize, + command: String, + args: Vec, + /// Sanitized, length-capped candidate name. + base: String, + /// What distinguishes this entry from another with the same `base`: the + /// explicit name when there is one, otherwise the whole argv. Only ever + /// hashed, never printed — an argv may carry an API key. + identity: String, +} + +/// A 24-bit hex digest of `identity`, used as a disambiguation suffix. +/// +/// FNV-1a written out rather than `DefaultHasher`, because the value lands in +/// a server name an operator reads and a qualified tool name the model calls: +/// it has to be the same on every build and platform, which `DefaultHasher` +/// does not promise. +fn name_disambiguator(identity: &str) -> String { + let mut hash: u64 = 0xcbf2_9ce4_8422_2325; + for byte in identity.as_bytes() { + hash ^= u64::from(*byte); + hash = hash.wrapping_mul(0x0000_0100_0000_01b3); + } + format!("{:06x}", hash & 0x00ff_ffff) +} + +/// Split an entry into its optional `name=` prefix and the command string. +/// +/// The `=` split is on the first occurrence, and the candidate only counts as +/// a name when it is a simple identifier (ASCII alphanumeric and hyphens, no +/// slashes), so a command carrying `=` — a URL with query parameters — is left +/// whole. +fn split_explicit_name(entry: &str) -> (Option, String) { + match entry.find('=') { + Some(pos) => { + let candidate = &entry[..pos]; + if !candidate.is_empty() + && candidate + .chars() + .all(|c| c.is_ascii_alphanumeric() || c == '-') + && !candidate.contains('/') + { + ( + Some(candidate.to_string()), + entry[pos + 1..].trim().to_string(), + ) + } else { + (None, entry.to_string()) + } } - // Parse optional `name=command` prefix. The name must be a simple - // identifier (ASCII alphanumeric + hyphen); the `=` split is on the - // first occurrence, so commands containing `=` (e.g. URLs with query - // params) are not affected when no valid name prefix is present. - let (explicit_name, command_str) = match trimmed.find('=') { - Some(pos) => { - let candidate = &trimmed[..pos]; - // Only treat as a name if it's a valid identifier and not a - // path (no slashes) — otherwise it's a command that happens - // to contain `=` (e.g. a URL with query params). - if !candidate.is_empty() - && candidate - .chars() - .all(|c| c.is_ascii_alphanumeric() || c == '-') - && !candidate.contains('/') - { - ( - Some(candidate.to_string()), - trimmed[pos + 1..].trim().to_string(), - ) - } else { - (None, trimmed.to_string()) - } + None => (None, entry.to_string()), + } +} + +/// Parse every non-blank `BUZZ_ACP_EXTRA_MCP_COMMANDS` entry. +/// +/// Fails closed on malformed shell quoting, on an entry that splits to no +/// command at all (`memory=`, a comment-only line), and on two entries +/// declaring the same `name=` prefix — the last of which used to be accepted +/// silently, handing one of the two servers a `-2` suffix chosen by position. +/// No message echoes an entry: it may carry an API key in its argv. +fn parse_extra_mcp_entries(entries: &[(usize, &str)]) -> Result, ConfigError> { + let mut parsed: Vec = Vec::with_capacity(entries.len()); + let mut explicit_seen: std::collections::HashMap = + std::collections::HashMap::new(); + for (entry, text) in entries { + let entry = *entry; + let (explicit_name, command_str) = split_explicit_name(text); + if let Some(name) = &explicit_name { + if let Some(first) = explicit_seen.insert(name.clone(), entry) { + return Err(ConfigError::ConfigFile(format!( + "BUZZ_ACP_EXTRA_MCP_COMMANDS entries {first} and {entry} declare the same \ + `name=` prefix; two MCP servers cannot share a name — rename one and \ + restart" + ))); } - None => (None, trimmed.to_string()), - }; - // Fail closed on both shapes of unusable entry. A non-blank entry that - // shell-splits to nothing — `memory=`, a lone `#comment`, `" "` after - // the `=` — is as broken as malformed quoting, and dropping it - // silently starts the harness with that server missing and nothing - // logged. Neither message echoes the entry: it may carry an API key. + } let parts = match shlex::split(&command_str) { Some(p) if p.first().is_some_and(|command| !command.is_empty()) => p, Some(_) => { return Err(ConfigError::ConfigFile(format!( - "BUZZ_ACP_EXTRA_MCP_COMMANDS entry {} names no command; \ - give it a command or remove the entry and restart", - idx + 1 + "BUZZ_ACP_EXTRA_MCP_COMMANDS entry {entry} names no command; \ + give it a command or remove the entry and restart" ))); } None => { return Err(ConfigError::ConfigFile(format!( - "BUZZ_ACP_EXTRA_MCP_COMMANDS entry {} has malformed shell quoting; \ - fix the quoting or remove the entry and restart", - idx + 1 + "BUZZ_ACP_EXTRA_MCP_COMMANDS entry {entry} has malformed shell quoting; \ + fix the quoting or remove the entry and restart" ))); } }; let command = parts[0].clone(); let args: Vec = parts[1..].to_vec(); - // Use the explicit name if provided, otherwise derive from the - // executable stem. Then disambiguate so two wrappers like - // `npx -y first-mcp` and `npx -y second-mcp` don't both become - // `npx` and trip McpRegistry's duplicate check. - let base_name = match &explicit_name { - Some(n) => sanitize_mcp_name(n), - None => { - let raw_stem = std::path::Path::new(&command) + let base = match &explicit_name { + Some(name) => sanitize_mcp_name(name), + None => sanitize_mcp_name( + std::path::Path::new(&command) .file_stem() .and_then(|s| s.to_str()) - .unwrap_or("extra-mcp"); - sanitize_mcp_name(raw_stem) - } + .unwrap_or("extra-mcp"), + ), }; - let name = if seen_names.contains(&base_name) { - // Append a numeric suffix until we find a unique name. The stem is - // re-cut against the suffix so a base name at the length ceiling - // does not overflow it again — an over-long name is rejected by - // `McpRegistry` and fails the whole session, not just this server. - let mut i = 2; - loop { - let suffix = format!("-{i}"); - let head = fit_mcp_name(&base_name, MAX_MCP_NAME_LEN - suffix.len()); - let candidate = format!("{head}{suffix}"); - if !seen_names.contains(&candidate) { - break candidate; - } - i += 1; - } + let identity = match &explicit_name { + Some(name) => format!("name={name}"), + None => std::iter::once(command.as_str()) + .chain(args.iter().map(String::as_str)) + .collect::>() + .join("\u{1f}"), + }; + parsed.push(ParsedExtra { + entry, + command, + args, + base, + identity, + }); + } + Ok(parsed) +} + +/// Append the operator's `BUZZ_ACP_EXTRA_MCP_COMMANDS` servers to `servers`. +/// +/// Extra servers carry no Buzz relay credentials or auth tags — they are +/// third-party tools, not Buzz-native MCP servers — and are marked untrusted +/// so `buzz-agent` withholds this process's identity variables from them and +/// never runs a hook on one. +/// +/// Startup fails, rather than a session, on: an adapter that cannot honour the +/// trust marker, a malformed entry, an entry naming no command, two entries +/// declaring the same `name=`, two entries resolving to the same server name, +/// and more servers than `buzz-agent` accepts in one session. +/// +/// # Errors +/// +/// Returns [`ConfigError::ConfigFile`] in each of those cases, naming the +/// entry number only — an entry may carry an API key in its argv. +fn append_extra_mcp_servers( + config: &Config, + servers: &mut Vec, +) -> Result<(), ConfigError> { + let entries: Vec<(usize, &str)> = config + .extra_mcp_commands + .iter() + .enumerate() + .map(|(idx, entry)| (idx + 1, entry.trim())) + .filter(|(_, entry)| !entry.is_empty()) + .collect(); + if entries.is_empty() { + return Ok(()); + } + + // The guarantees these servers are declared under — credentials withheld, + // hooks refused — live in `buzz-agent`, behind the `trusted` marker. Any + // other adapter ignores the marker and spawns the declared servers out of + // a process holding BUZZ_PRIVATE_KEY, so refuse at startup rather than + // leak on the first session. + let adapter = config::normalize_agent_command_identity(&config.agent_command); + if adapter != EXTRA_MCP_ADAPTER { + return Err(ConfigError::ConfigFile(format!( + "BUZZ_ACP_EXTRA_MCP_COMMANDS is set, but the agent command is `{adapter}`. Only \ + `{EXTRA_MCP_ADAPTER}` honours the `trusted` marker that withholds \ + BUZZ_PRIVATE_KEY, NOSTR_PRIVATE_KEY, BUZZ_RELAY_URL and BUZZ_AUTH_TAG from an \ + extra MCP server; every other ACP adapter spawns the declared servers itself, \ + from a process that inherited this one's environment. Unset \ + BUZZ_ACP_EXTRA_MCP_COMMANDS, or set BUZZ_ACP_AGENT_COMMAND={EXTRA_MCP_ADAPTER}." + ))); + } + + let total = servers.len() + entries.len(); + if total > MAX_MCP_SERVERS { + return Err(ConfigError::ConfigFile(format!( + "too many MCP servers: {total} > {MAX_MCP_SERVERS} ({} primary plus {} \ + BUZZ_ACP_EXTRA_MCP_COMMANDS entries). buzz-agent rejects the whole array past \ + {MAX_MCP_SERVERS}, so every session would fail; remove entries and restart.", + servers.len(), + entries.len() + ))); + } + + let parsed = parse_extra_mcp_entries(&entries)?; + + // Shape names in a second pass. A name is disambiguated only when it + // actually collides, and then *every* colliding entry takes a suffix + // derived from its own identity — not just the later one. The positional + // `-2` this replaced made the mapping from entry to name depend on entry + // order, so reordering two entries whose names truncate alike swapped + // which executable owned each qualified tool name. + let mut base_counts: std::collections::HashMap<&str, usize> = std::collections::HashMap::new(); + for extra in &parsed { + *base_counts.entry(extra.base.as_str()).or_insert(0) += 1; + } + let primary_names: std::collections::HashSet = + servers.iter().map(|s| s.name.clone()).collect(); + let mut taken: std::collections::HashMap = std::collections::HashMap::new(); + for extra in &parsed { + let collides = base_counts.get(extra.base.as_str()).copied().unwrap_or(0) > 1 + || primary_names.contains(&extra.base); + let name = if collides { + let suffix = format!("-{}", name_disambiguator(&extra.identity)); + format!( + "{}{suffix}", + fit_mcp_name(&extra.base, MAX_MCP_NAME_LEN - suffix.len()) + ) } else { - base_name + extra.base.clone() }; - seen_names.insert(name.clone()); + if let Some(first) = taken.insert(name.clone(), extra.entry) { + return Err(ConfigError::ConfigFile(format!( + "BUZZ_ACP_EXTRA_MCP_COMMANDS entries {first} and {} resolve to the same MCP \ + server name; give one of them a distinct `name=` prefix and restart", + extra.entry + ))); + } + if primary_names.contains(&name) { + return Err(ConfigError::ConfigFile(format!( + "BUZZ_ACP_EXTRA_MCP_COMMANDS entry {} resolves to the primary MCP server's \ + name; give it a distinct `name=` prefix and restart", + extra.entry + ))); + } servers.push(McpServer { name, - command, - args, + command: extra.command.clone(), + args: extra.args.clone(), env: vec![], trusted: false, }); } - - Ok(servers) + Ok(()) } /// Sanitize a raw executable stem into a name that satisfies the downstream @@ -6036,8 +6207,10 @@ fn build_mcp_servers(config: &Config) -> Result, ConfigError> { /// /// [`MAX_MCP_NAME_LEN`] is budgeted against the registry's *qualified* tool /// name (`__`, capped at [`MAX_MCP_QNAME_LEN`]), not against its -/// looser 128-byte bound on the name alone, so a sanitized name always leaves -/// room for the tools the server advertises. +/// looser 128-byte bound on the name alone, so a sanitized name leaves room +/// for every tool up to [`MCP_TOOL_NAME_RESERVE`] bytes of separator plus bare +/// name. A longer advertised tool name still overflows; see +/// [`MCP_TOOL_NAME_RESERVE`] for what happens then. fn sanitize_mcp_name(raw: &str) -> String { let sanitized: String = raw .chars() @@ -9223,6 +9396,16 @@ mod build_mcp_servers_tests { } } + /// A config naming the one adapter that honours the `trusted` marker, so + /// `BUZZ_ACP_EXTRA_MCP_COMMANDS` is accepted. Every extras test builds on + /// this; `extra_mcp_commands_refused_under_another_adapter` covers the + /// default `goose` case, where the same entries fail startup. + fn extras_config() -> Config { + let mut config = test_config(); + config.agent_command = "buzz-agent".into(); + config + } + #[test] fn session_new_mcp_server_has_required_fields() { let config = test_config(); @@ -9372,7 +9555,7 @@ mod build_mcp_servers_tests { #[test] fn extra_mcp_commands_append_additional_servers() { - let mut config = test_config(); + let mut config = extras_config(); config.extra_mcp_commands = vec!["npx -y mcp-remote https://mcp.tavily.com/mcp/?tavilyApiKey=test-key".into()]; let servers = build_mcp_servers(&config).unwrap(); @@ -9402,7 +9585,7 @@ mod build_mcp_servers_tests { #[test] fn multiple_extra_mcp_commands_append_in_order() { - let mut config = test_config(); + let mut config = extras_config(); config.extra_mcp_commands = vec![ "brave-search-mcp".into(), "npx -y mcp-remote https://mcp.tavily.com/mcp/".into(), @@ -9416,7 +9599,7 @@ mod build_mcp_servers_tests { #[test] fn empty_extra_mcp_commands_are_skipped() { - let mut config = test_config(); + let mut config = extras_config(); config.extra_mcp_commands = vec![ "valid-server".into(), "".into(), @@ -9442,7 +9625,7 @@ mod build_mcp_servers_tests { // empty string whenever the runtime has no MCP command. An // extras-only configuration must still reach the model: returning an // empty array here would leave the agent with no tools, silently. - let mut config = test_config(); + let mut config = extras_config(); config.mcp_command = "".into(); config.extra_mcp_commands = vec!["memory=memory-mcp --db /tmp/m".into()]; let servers = build_mcp_servers(&config).unwrap(); @@ -9468,19 +9651,31 @@ mod build_mcp_servers_tests { #[test] fn extra_mcp_commands_disambiguate_duplicate_names() { // Two npx-based wrappers must not both become "npx" — that would - // trip McpRegistry's duplicate-name check at spawn. - let mut config = test_config(); + // trip McpRegistry's duplicate-name check at spawn. Both collide, so + // both take a suffix derived from their own argv: the name a server + // gets does not depend on which entry came first. + let mut config = extras_config(); config.extra_mcp_commands = vec!["npx -y first-mcp".into(), "npx -y second-mcp".into()]; let servers = build_mcp_servers(&config).unwrap(); assert_eq!(servers.len(), 3, "primary + 2 extra = 3 servers"); - assert_eq!(servers[1].name, "npx"); - assert_eq!(servers[2].name, "npx-2"); + let (first, second) = (servers[1].name.clone(), servers[2].name.clone()); + assert_ne!(first, second, "two wrappers must not share a name"); + for name in [&first, &second] { + assert!(name.starts_with("npx-"), "derived from the stem: {name}"); + assert_registry_name_contract(name); + } + + // Reorder: each entry keeps the name it had. + config.extra_mcp_commands = vec!["npx -y second-mcp".into(), "npx -y first-mcp".into()]; + let reordered = build_mcp_servers(&config).unwrap(); + assert_eq!(reordered[1].name, second, "reordering renamed second-mcp"); + assert_eq!(reordered[2].name, first, "reordering renamed first-mcp"); } #[test] fn extra_mcp_commands_shell_split_quoted_paths() { // Quoted paths with spaces must be preserved as a single argv element. - let mut config = test_config(); + let mut config = extras_config(); config.extra_mcp_commands = vec![r#""my server" --port 8080"#.into()]; let servers = build_mcp_servers(&config).unwrap(); assert_eq!(servers.len(), 2); @@ -9492,7 +9687,7 @@ mod build_mcp_servers_tests { fn extra_mcp_commands_fail_closed_on_malformed_quoting() { // Malformed quoting must fail startup, not silently skip the entry. // The error must not echo the command (it may contain an API key). - let mut config = test_config(); + let mut config = extras_config(); config.extra_mcp_commands = vec![ "valid-server".into(), "'unmatched-quote".into(), @@ -9523,7 +9718,7 @@ mod build_mcp_servers_tests { ("empty quoted command", "memory=''"), ("comment-only entry", "# just a note"), ] { - let mut config = test_config(); + let mut config = extras_config(); config.extra_mcp_commands = vec!["valid-server".into(), entry.into()]; let result = build_mcp_servers(&config); let err = match result { @@ -9545,7 +9740,7 @@ mod build_mcp_servers_tests { fn extra_mcp_commands_sanitized_names() { // Names with underscores, spaces, or punctuation must be sanitized // to the McpRegistry ASCII alphanumeric/hyphen contract. - let mut config = test_config(); + let mut config = extras_config(); config.extra_mcp_commands = vec!["my_server --port 8080".into()]; let servers = build_mcp_servers(&config).unwrap(); assert_eq!(servers.len(), 2); @@ -9555,7 +9750,7 @@ mod build_mcp_servers_tests { #[test] fn extra_mcp_commands_trusted_flag() { // The primary server must be trusted; extras must not be. - let mut config = test_config(); + let mut config = extras_config(); config.extra_mcp_commands = vec!["some-extra-server".into()]; let servers = build_mcp_servers(&config).unwrap(); assert_eq!(servers.len(), 2); @@ -9567,7 +9762,7 @@ mod build_mcp_servers_tests { fn extra_mcp_commands_explicit_name_prefix() { // The `name=command` syntax sets the server name explicitly so // reordering entries does not silently rename a server. - let mut config = test_config(); + let mut config = extras_config(); config.extra_mcp_commands = vec!["memory=npx -y memory-mcp".into()]; let servers = build_mcp_servers(&config).unwrap(); assert_eq!(servers.len(), 2); @@ -9580,7 +9775,7 @@ mod build_mcp_servers_tests { fn extra_mcp_commands_explicit_name_stable_on_reorder() { // Two npx-based servers with explicit names keep their names // regardless of entry order. - let mut config = test_config(); + let mut config = extras_config(); config.extra_mcp_commands = vec![ "alpha=npx -y first-mcp".into(), "beta=npx -y second-mcp".into(), @@ -9606,7 +9801,7 @@ mod build_mcp_servers_tests { // With newline separation, commas inside arguments survive shlex // splitting — the case rsaulo identified where comma-delimiter // parsing would break `--filter 'a,b'`. - let mut config = test_config(); + let mut config = extras_config(); config.extra_mcp_commands = vec!["npx -y srv --filter a,b".into()]; let servers = build_mcp_servers(&config).unwrap(); assert_eq!(servers.len(), 2); @@ -9659,7 +9854,7 @@ mod build_mcp_servers_tests { // name alone: a name cut to the looser 128-byte ceiling still fails // the whole session the moment the server advertises a tool. The cut // is budgeted against MAX_MCP_QNAME_LEN instead. - let mut config = test_config(); + let mut config = extras_config(); let long_explicit = "a".repeat(130); let long_stem = "b".repeat(200); config.extra_mcp_commands = vec![ @@ -9691,12 +9886,15 @@ mod build_mcp_servers_tests { #[test] fn extra_mcp_commands_disambiguation_suffix_respects_registry_limit() { - // A base name already at the ceiling must not grow past it when the - // `-2`, `-3`, ... disambiguation suffix is appended, including once - // the suffix reaches two digits. - let mut config = test_config(); + // Explicit names that differ only past the length ceiling all truncate + // onto one base. Every one of them must still come out unique, inside + // the registry's contract, and stable when the entries are reordered. + let mut config = extras_config(); let at_ceiling = "c".repeat(MAX_MCP_NAME_LEN); - config.extra_mcp_commands = (0..11).map(|i| format!("{at_ceiling}=srv-{i}")).collect(); + let entries: Vec = (0..11) + .map(|i| format!("{at_ceiling}suffix{i}=srv-{i}")) + .collect(); + config.extra_mcp_commands = entries.clone(); let servers = build_mcp_servers(&config).unwrap(); assert_eq!(servers.len(), 12, "primary + 11 extra = 12 servers"); @@ -9704,30 +9902,151 @@ mod build_mcp_servers_tests { for name in &names { assert_registry_name_contract(name); } - assert_eq!(names[1], at_ceiling, "the first entry keeps the full name"); - assert!( - names[2].ends_with("-2"), - "the second entry is disambiguated: {}", - names[2] - ); - assert!( - names[11].ends_with("-11"), - "the eleventh entry carries a two-digit suffix: {}", - names[11] - ); let unique: std::collections::HashSet<&str> = names.iter().copied().collect(); assert_eq!( unique.len(), names.len(), "truncation must not collapse two servers onto one name: {names:?}" ); + + // The name a server gets is a function of the entry, not its position: + // reverse the entries and every command keeps the name it had. + let by_command: std::collections::HashMap<&str, &str> = servers[1..] + .iter() + .map(|s| (s.command.as_str(), s.name.as_str())) + .collect(); + let mut reversed = entries; + reversed.reverse(); + config.extra_mcp_commands = reversed; + let reordered = build_mcp_servers(&config).unwrap(); + for server in &reordered[1..] { + assert_eq!( + by_command[server.command.as_str()], + server.name, + "reordering renamed {}", + server.command + ); + } + } + + #[test] + fn extra_mcp_commands_refused_under_another_adapter() { + // The credential-withholding and hook-confinement guarantees an extra + // server is declared under live in buzz-agent, behind the `trusted` + // marker. Under any other adapter — and `goose` is the default — the + // array is spawned by a process that inherited BUZZ_PRIVATE_KEY, so + // the operator must learn at startup, not leak on the first session. + for command in ["goose", "codex-acp", "/usr/local/bin/claude-code-acp"] { + let mut config = test_config(); + config.agent_command = command.into(); + config.extra_mcp_commands = vec!["memory=memory-mcp".into()]; + let err = match build_mcp_servers(&config) { + Err(e) => format!("{e}"), + Ok(servers) => panic!("{command} accepted extra MCP servers: {servers:?}"), + }; + assert!( + err.contains("BUZZ_ACP_EXTRA_MCP_COMMANDS") && err.contains("buzz-agent"), + "{command}: the error must name the variable and the adapter that \ + honours it: {err}" + ); + } + + // Blank entries are not a configuration: an unset variable that clap + // still turned into one empty string must not fail startup. + let mut config = test_config(); + config.extra_mcp_commands = vec!["".into(), " ".into()]; + let servers = build_mcp_servers(&config).expect("blank entries are not extras"); + assert_eq!(servers.len(), 1, "primary only: {servers:?}"); + + // The identity is the normalized command, so a path or a Windows + // shim still counts as buzz-agent. + for command in ["buzz-agent", "/usr/local/bin/buzz-agent", "buzz-agent.exe"] { + let mut config = test_config(); + config.agent_command = command.into(); + config.extra_mcp_commands = vec!["memory=memory-mcp".into()]; + let servers = build_mcp_servers(&config) + .unwrap_or_else(|e| panic!("{command} should accept extras: {e}")); + assert_eq!(servers.len(), 2, "primary + 1 extra: {servers:?}"); + } + } + + #[test] + fn extra_mcp_commands_bounded_by_the_registry_server_cap() { + // McpRegistry rejects an array over MAX_MCP_SERVERS outright, so an + // over-long configuration must fail startup rather than start a + // harness that announces itself and then fails every session/new. + let mut config = extras_config(); + config.extra_mcp_commands = (0..MAX_MCP_SERVERS - 1) + .map(|i| format!("s{i}=srv-{i}")) + .collect(); + let servers = build_mcp_servers(&config).expect("exactly at the cap is fine"); + assert_eq!(servers.len(), MAX_MCP_SERVERS, "primary + 15 extras"); + + config.extra_mcp_commands = (0..MAX_MCP_SERVERS) + .map(|i| format!("s{i}=srv-{i}")) + .collect(); + let err = match build_mcp_servers(&config) { + Err(e) => format!("{e}"), + Ok(servers) => panic!("one over the cap was accepted: {servers:?}"), + }; + assert!( + err.contains(&format!("{}", MAX_MCP_SERVERS + 1)) && err.contains("16"), + "the error must give the entry count and the cap: {err}" + ); + } + + #[test] + fn extra_mcp_commands_reject_duplicate_explicit_names() { + // `memory=a` and `memory=b` used to be accepted, one of them silently + // renamed `memory-2` by position — inconsistent with the fail-closed + // handling of every other unusable entry, and a broken promise: the + // explicit name is documented as the way to pin a server's name. + let mut config = extras_config(); + config.extra_mcp_commands = vec![ + "valid-server".into(), + "memory=memory-mcp --db /tmp/a".into(), + "memory=memory-mcp --db /tmp/b".into(), + ]; + let err = match build_mcp_servers(&config) { + Err(e) => format!("{e}"), + Ok(servers) => panic!("duplicate explicit names were accepted: {servers:?}"), + }; + assert!( + err.contains("entries 2 and 3"), + "the error should name both entries: {err}" + ); + assert!( + !err.contains("/tmp/a") && !err.contains("/tmp/b"), + "the error must not echo the raw entry: {err}" + ); + } + + #[test] + fn extra_mcp_commands_reject_two_entries_resolving_to_one_name() { + // Two identical entries derive one name and no suffix can separate + // them. Failing closed beats registering one server twice under a + // name the operator cannot predict. + let mut config = extras_config(); + config.extra_mcp_commands = vec!["npx -y same-mcp".into(), "npx -y same-mcp".into()]; + let err = match build_mcp_servers(&config) { + Err(e) => format!("{e}"), + Ok(servers) => panic!("two identical entries were accepted: {servers:?}"), + }; + assert!( + err.contains("entries 1 and 2"), + "the error should name both entries: {err}" + ); + assert!( + !err.contains("same-mcp"), + "the error must not echo the raw entry: {err}" + ); } #[test] fn extra_mcp_commands_url_with_equals_not_treated_as_name() { // A command containing `=` in a URL query param must not be // misinterpreted as a `name=command` prefix. - let mut config = test_config(); + let mut config = extras_config(); config.extra_mcp_commands = vec!["npx -y mcp-remote https://mcp.tavily.com/mcp/?tavilyApiKey=test-key".into()]; let servers = build_mcp_servers(&config).unwrap(); diff --git a/crates/buzz-agent/tests/untrusted_mcp_env_e2e.rs b/crates/buzz-agent/tests/untrusted_mcp_env_e2e.rs index 7e8286545b7..afb255fa39a 100644 --- a/crates/buzz-agent/tests/untrusted_mcp_env_e2e.rs +++ b/crates/buzz-agent/tests/untrusted_mcp_env_e2e.rs @@ -65,10 +65,15 @@ async fn untrusted_mcp_env_extra_command_reaches_tool_list() { // The operator's half: the environment variable, parsed by buzz-acp's own // clap definition (`env = "BUZZ_ACP_EXTRA_MCP_COMMANDS"`, newline-split). std::env::set_var("BUZZ_ACP_EXTRA_MCP_COMMANDS", fake_mcp); + // `--agent-command buzz-agent` is not decoration: buzz-acp refuses extra + // MCP servers under any other adapter, because only buzz-agent honours the + // `trusted` marker this whole test is about. let args = buzz_acp::CliArgs::try_parse_from([ "buzz-acp", "--private-key", TEST_PRIVATE_KEY, + "--agent-command", + "buzz-agent", "--mcp-command", fake_mcp, ]) @@ -197,3 +202,16 @@ async fn untrusted_mcp_env_extra_command_reaches_tool_list() { h.shutdown().await; } + +/// buzz-acp's startup-side server cap is the registry's cap, not a number that +/// drifted from it. Mirroring the bound is what keeps an over-long +/// configuration from producing a harness that starts, announces itself, and +/// then fails every `session/new`. +#[test] +fn mcp_server_cap_mirrors_buzz_agent() { + assert_eq!( + buzz_acp::MAX_MCP_SERVERS, + buzz_agent::MAX_MCP_SERVERS, + "buzz-acp's mirrored MCP server cap drifted from McpRegistry's" + ); +} diff --git a/docs/plans/2026-09-04-zs-implementation-plan.md b/docs/plans/2026-09-04-zs-implementation-plan.md index c15e15de93c..df6cf789067 100644 --- a/docs/plans/2026-09-04-zs-implementation-plan.md +++ b/docs/plans/2026-09-04-zs-implementation-plan.md @@ -199,6 +199,7 @@ the root workspace excludes that manifest (`Cargo.toml:35`). - Branch `feat/mcp-registry`, after T4. Depends on the T4 fake-server fixture and a fake HTTP MCP fixture (a tiny Streamable HTTP server in the test tree). OpenSEO is not a dependency; the T6 post-approval run is a separate integration check. - Design memo first, `docs/plans/2026-09-xx-mcp-registry-design.md`, one page, reviewed by Sol before code. It must answer: the two server classes and the env each receives; the runtime capability matrix (buzz-agent: stdio only, since `McpServer` is the ACP `McpServerStdio` shape at `buzz-acp/src/acp.rs:25` and `buzz-agent/src/types.rs:536`; Claude and Codex: stdio and HTTP through native config); the process boundary for stdio servers under Claude and Codex, where the adapter inherits the whole harness environment including provider keys and user-defined values (`runtime.rs:563, 692-701, 753-757`, `buzz-acp/src/acp.rs:454-517`), solved by a launcher `buzz-mcp-launch` that builds the child environment from empty with only platform essentials and the server's approved values, following the `env_clear` plus allow-list pattern in `buzz-agent/src/mcp.rs:733-754`, and on Windows supervises the child rather than exec; HTTP credentials, which no launcher can inject, handled by a local credential-resolving stdio proxy in front of Streamable HTTP upstreams (the same binary in proxy mode), so no secret is ever written to JSON or TOML; where secrets live (a shared read-only secret-store crate extracted from `desktop/src-tauri/src/secret_store.rs`, since a workspace binary cannot call the private Tauri module); the launcher's crate path, workspace membership (`Cargo.toml:2-34`), sidecar stubs (`justfile:167-180`), release build list (`justfile:306-309`), `scripts/bundle-sidecars.sh` and `tauri.conf.json:52-62` plus the Windows manifest, with generated config naming the bundled launcher by absolute path; name collision rule with built-ins; per-agent toggle storage; config roots per agent with the login caveat from T6. +- Per-server environment, carried over from T4's Sol audit: `BUZZ_ACP_EXTRA_MCP_COMMANDS` can only carry a server's key in its argv, where `ps` and any crash dump can read it, so the T4 docs tell operators not to put one there and the registry has to give them somewhere else — a per-server `env` block whose values are keychain references resolved at spawn, which the design memo above already owns for Claude and Codex. Extend it to the buzz-agent path. - Capability facts added to `KnownAcpRuntime` (`mcp_transports`, `mcp_config_root_env`) and projected through core to the UI per `desktop/src/features/agents/AGENTS.md:13, 34`; the guide is updated in the same PR. - Then: `mcp_servers.json` schema and loader with `custom_harnesses`-style structure validation; Settings panel to add stdio and HTTP servers with an approve step that shows the exact command or URL; per-agent toggles in the definition dialog; generation of `BUZZ_ACP_EXTRA_MCP_COMMANDS` for buzz-acp and of native config for Claude and Codex under per-agent `CLAUDE_CONFIG_DIR` and `CODEX_HOME`. - Tests first (Rust, module `managed_agents::mcp_registry`, and the launcher crate): loader rejects a server named like a built-in, rejects an inline secret value, resolves a keychain reference; HTTP entry is refused for buzz-agent and accepted for Claude and Codex; generated Claude and Codex config is asserted structurally (command, args, URL, env references) by parsing the written files, because the config bridge readers keep only name, kind and enabled (`config_bridge/types.rs:226-233`) and cannot detect a wrong command; launcher end-to-end: spawned with the identity variables and two unrelated sentinel secrets set, the fake server it starts reports none of them; proxy end-to-end: a fake authenticated HTTP MCP fixture is invoked through Claude and Codex via the proxy and the credential appears in no generated file; the toggle changes only the named agent's generated config. Frontend test: approve step required before save.