From 3817075501a21fc421db58f73180d7376d565306 Mon Sep 17 00:00:00 2001 From: "Andrei G." Date: Mon, 20 Jul 2026 23:27:13 +0200 Subject: [PATCH] refactor(mcp)!: replace bool parameters with type-safe enums in McpClient connect paths McpClient::connect took two adjacent positional bools (suppress_stderr, env_isolation); connect_url/connect_url_with_headers/connect_url_oauth took a bare trusted bool downgraded from McpTrustLevel at the call site. Both shapes let a future signature edit silently swap security-relevant behavior (full parent-env inheritance, SSRF private-IP bypass) without a compiler error. Replace the two connect() bools with StderrPolicy and EnvPolicy enums, and pass McpTrustLevel through connect_url* directly instead of collapsing it to bool beforehand. No runtime behavior change. BREAKING CHANGE: McpClient::connect now takes StderrPolicy/EnvPolicy instead of two bools; connect_url/connect_url_with_headers/connect_url_oauth now take McpTrustLevel instead of trusted: bool. Closes #6478 --- CHANGELOG.md | 12 +++ crates/zeph-mcp/src/client.rs | 142 ++++++++++++++++++++++--- crates/zeph-mcp/src/lib.rs | 2 +- crates/zeph-mcp/src/manager/connect.rs | 8 +- crates/zeph-mcp/src/manager/retry.rs | 23 ++-- 5 files changed, 160 insertions(+), 27 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 1741b3420..6ce81db81 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -74,6 +74,18 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). ### Changed +- `zeph-mcp`: **breaking** — `McpClient::connect` no longer takes two adjacent positional + `bool` parameters (`suppress_stderr`, `env_isolation`); it now takes `StderrPolicy` + (`Forward`/`Suppress`) and `EnvPolicy` (`InheritAll`/`Isolated`) instead. `McpClient::connect_url`, + `connect_url_with_headers`, and `connect_url_oauth` no longer take a bare `trusted: bool`; + they now take `McpTrustLevel` directly, matched on internally instead of being downgraded + to a `bool` at the call site. Both changes eliminate a class of silent, compile-clean + argument-order-swap regressions (issue #6478) — a swap between the two new enum-typed + `connect` parameters is now a type error, and the SSRF-skip check on `connect_url*` matches + on `McpTrustLevel` directly rather than on a bool that discarded which variant it came from. + No runtime behavior change; existing callers must map their `bool`/`trusted` values to the + new enum variants. + - `zeph-context`: internal refactor of `assembler.rs`, no behavior change (issues #6573, #6577). `schedule_context_fetchers` took 15 positional parameters, including 5 consecutive `usize` budget values that duplicated fields already on `BudgetAllocation` — an undetected diff --git a/crates/zeph-mcp/src/client.rs b/crates/zeph-mcp/src/client.rs index 112fbbe07..4a08fbc19 100644 --- a/crates/zeph-mcp/src/client.rs +++ b/crates/zeph-mcp/src/client.rs @@ -30,10 +30,57 @@ use url::Url; use zeph_common::net::{ResolveError, resolve_and_validate}; use zeph_tools::is_private_ip; +use crate::McpTrustLevel; use crate::elicitation::ElicitationEvent; use crate::error::McpError; use crate::tool::McpTool; +/// Whether a spawned stdio MCP server process inherits the parent environment. +/// +/// Passed to [`McpClient::connect`] instead of a bare `bool` so it cannot be +/// silently swapped with the adjacent [`StderrPolicy`] parameter — an order-swap +/// between the two would otherwise compile cleanly while leaking the full parent +/// environment (including any secrets reachable through it) to the child process. +/// +/// # Examples +/// +/// ``` +/// use zeph_mcp::EnvPolicy; +/// +/// let policy = EnvPolicy::Isolated; +/// assert_eq!(policy, EnvPolicy::Isolated); +/// ``` +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum EnvPolicy { + /// Spawn with the full parent process environment, plus the server's declared `env` map. + InheritAll, + /// Spawn with only the minimal base env vars (see + /// [`crate::security::build_isolated_env`]) plus the server's declared `env` map. + Isolated, +} + +/// Whether a spawned stdio MCP server process's stderr is forwarded or suppressed. +/// +/// Passed to [`McpClient::connect`] instead of a bare `bool` so it cannot be +/// silently swapped with the adjacent [`EnvPolicy`] parameter. Purely a log-verbosity +/// control — unlike [`EnvPolicy`], it has no security implications. +/// +/// # Examples +/// +/// ``` +/// use zeph_mcp::StderrPolicy; +/// +/// let policy = StderrPolicy::Suppress; +/// assert_eq!(policy, StderrPolicy::Suppress); +/// ``` +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum StderrPolicy { + /// Forward the child process's stderr to the parent's stderr (default OS inheritance). + Forward, + /// Redirect the child process's stderr to a null sink. + Suppress, +} + /// Minimum interval between tool list refreshes per server (rate limiting). const MIN_REFRESH_INTERVAL: Duration = Duration::from_secs(5); @@ -413,8 +460,8 @@ impl McpClient { env: &std::collections::HashMap, allowed_commands: &[String], timeout: Duration, - suppress_stderr: bool, - env_isolation: bool, + stderr_policy: StderrPolicy, + env_policy: EnvPolicy, tx: Sender, last_refresh: Arc>, handler_cfg: HandlerConfig, @@ -422,22 +469,21 @@ impl McpClient { crate::security::validate_command(command, allowed_commands)?; crate::security::validate_env(env)?; - let effective_env = if env_isolation { - crate::security::build_isolated_env(env) - } else { - env.clone() + let effective_env = match env_policy { + EnvPolicy::Isolated => crate::security::build_isolated_env(env), + EnvPolicy::InheritAll => env.clone(), }; let mut cmd = Command::new(command); cmd.args(args); - if env_isolation { + if env_policy == EnvPolicy::Isolated { cmd.env_clear(); } for (k, v) in &effective_env { cmd.env(k, v); } - let transport = if suppress_stderr { + let transport = if stderr_policy == StderrPolicy::Suppress { let (proc, _stderr) = TokioChildProcess::builder(cmd) .stderr(std::process::Stdio::null()) .spawn() @@ -466,9 +512,9 @@ impl McpClient { /// Connect to a remote MCP server over Streamable HTTP. /// /// Performs SSRF validation before connecting — blocks URLs that resolve - /// to private, loopback, or link-local IP ranges — unless `trusted` is - /// `true`, in which case the check is skipped (use only for - /// operator-controlled static config). + /// to private, loopback, or link-local IP ranges — unless `trust_level` is + /// [`McpTrustLevel::Trusted`], in which case the check is skipped (use only + /// for operator-controlled static config). /// /// # Errors /// @@ -484,11 +530,12 @@ impl McpClient { server_id: &str, url: &str, timeout: Duration, - trusted: bool, + trust_level: McpTrustLevel, tx: Sender, last_refresh: Arc>, handler_cfg: HandlerConfig, ) -> Result { + let trusted = matches!(trust_level, McpTrustLevel::Trusted); let pinned = validate_and_pin_url(url, trusted).await?; let client = build_hardened_client(server_id, pinned.as_ref())?; let config = StreamableHttpClientTransportConfig::with_uri(url.to_owned()); @@ -511,7 +558,8 @@ impl McpClient { /// /// # Errors /// - /// Returns `McpError::SsrfBlocked` if the URL resolves to a private IP (unless `trusted`), + /// Returns `McpError::SsrfBlocked` if the URL resolves to a private IP (unless + /// `trust_level` is [`McpTrustLevel::Trusted`]), /// `McpError::Timeout` if the handshake exceeds `timeout`, or /// `McpError::Connection` if the handshake fails. #[allow(clippy::too_many_arguments)] @@ -526,11 +574,12 @@ impl McpClient { url: &str, headers: &HashMap, timeout: Duration, - trusted: bool, + trust_level: McpTrustLevel, tx: Sender, last_refresh: Arc>, handler_cfg: HandlerConfig, ) -> Result { + let trusted = matches!(trust_level, McpTrustLevel::Trusted); let pinned = validate_and_pin_url(url, trusted).await?; let custom_headers: HashMap = headers @@ -596,12 +645,13 @@ impl McpClient { callback_port: u16, client_name: &str, credential_store: Arc, - trusted: bool, + trust_level: McpTrustLevel, tx: Sender, last_refresh: Arc>, timeout: Duration, handler_cfg: HandlerConfig, ) -> Result { + let trusted = matches!(trust_level, McpTrustLevel::Trusted); let pinned = validate_and_pin_url(url, trusted).await?; let hardened_client = build_hardened_client(server_id, pinned.as_ref())?; @@ -1383,6 +1433,68 @@ mod tests { assert_matches!(err, McpError::SsrfBlocked { .. }); } + /// Minimal fixture for exercising `McpClient::connect_url` at its public signature. + /// SSRF validation runs before any network I/O, so `tx`/`last_refresh`/`handler_cfg` + /// are never touched when the call is expected to fail at that first check. + fn connect_url_test_fixture() -> ( + Sender, + Arc>, + HandlerConfig, + ) { + let (tx, _rx) = tokio::sync::mpsc::channel::(1); + let handler_cfg = HandlerConfig { + roots: Arc::new(vec![]), + max_description_bytes: 1024, + elicitation_tx: None, + elicitation_timeout: Duration::from_secs(5), + }; + (tx, Arc::new(DashMap::new()), handler_cfg) + } + + /// Exercises the `McpTrustLevel` pass-through this PR introduced at the actual public + /// `connect_url` signature (not just the private `validate_and_pin_url` helper with a + /// bare `bool`): `Untrusted` must still trigger the SSRF check. + #[tokio::test] + async fn connect_url_untrusted_blocks_private_ip() { + let (tx, last_refresh, handler_cfg) = connect_url_test_fixture(); + let err = McpClient::connect_url( + "test-server", + "http://127.0.0.1:1/mcp", + Duration::from_secs(5), + McpTrustLevel::Untrusted, + tx, + last_refresh, + handler_cfg, + ) + .await + .unwrap_err(); + assert_matches!(err, McpError::SsrfBlocked { .. }); + } + + /// Companion to `connect_url_untrusted_blocks_private_ip`: `Trusted` must skip the SSRF + /// check entirely at the same public entry point. The connection itself still fails + /// (nothing listens on `127.0.0.1:1`), but the failure must not be `SsrfBlocked` — + /// proving `trust_level` reached `validate_and_pin_url` as `Trusted`, not `Untrusted`. + #[tokio::test] + async fn connect_url_trusted_skips_ssrf_check() { + let (tx, last_refresh, handler_cfg) = connect_url_test_fixture(); + let err = McpClient::connect_url( + "test-server", + "http://127.0.0.1:1/mcp", + Duration::from_millis(500), + McpTrustLevel::Trusted, + tx, + last_refresh, + handler_cfg, + ) + .await + .unwrap_err(); + assert!( + !matches!(err, McpError::SsrfBlocked { .. }), + "trusted connect_url must not run the SSRF check, got {err:?}" + ); + } + #[tokio::test] async fn validate_and_pin_url_rejects_invalid_url() { let err = validate_and_pin_url("not-a-url", false).await.unwrap_err(); diff --git a/crates/zeph-mcp/src/lib.rs b/crates/zeph-mcp/src/lib.rs index e89f1bb5b..f495c6bc6 100644 --- a/crates/zeph-mcp/src/lib.rs +++ b/crates/zeph-mcp/src/lib.rs @@ -139,7 +139,7 @@ pub mod mock; pub use attestation::{AttestationResult, ServerTrustBoundary, ToolFingerprint, attest_tools}; pub use caller::McpCaller; -pub use client::{OAuthConnectResult, OAuthPending, ToolRefreshEvent}; +pub use client::{EnvPolicy, OAuthConnectResult, OAuthPending, StderrPolicy, ToolRefreshEvent}; pub use content::{render_content_block, render_content_blocks}; pub use elicitation::ElicitationEvent; pub use embedding_guard::{EmbeddingAnomalyGuard, EmbeddingGuardEvent, EmbeddingGuardResult}; diff --git a/crates/zeph-mcp/src/manager/connect.rs b/crates/zeph-mcp/src/manager/connect.rs index 50f431b5e..c748a3b9d 100644 --- a/crates/zeph-mcp/src/manager/connect.rs +++ b/crates/zeph-mcp/src/manager/connect.rs @@ -436,7 +436,7 @@ impl McpManager { let scopes = scopes.clone(); let client_name = client_name.clone(); let server_id = config.id.clone(); - let trusted = matches!(config.trust_level, McpTrustLevel::Trusted); + let trust_level = config.trust_level; let timeout = config.timeout; let handler_cfg = self.handler_cfg_for(&config).await; let status_tx = self.status_tx.clone(); @@ -457,7 +457,7 @@ impl McpManager { callback_port, client_name, credential_store, - trusted, + trust_level, tx, last_refresh, timeout, @@ -705,7 +705,7 @@ async fn run_oauth_handshake( callback_port: u16, client_name: String, credential_store: Arc, - trusted: bool, + trust_level: McpTrustLevel, tx: mpsc::Sender, last_refresh: Arc>, timeout: Duration, @@ -719,7 +719,7 @@ async fn run_oauth_handshake( callback_port, &client_name, credential_store, - trusted, + trust_level, tx, last_refresh, timeout, diff --git a/crates/zeph-mcp/src/manager/retry.rs b/crates/zeph-mcp/src/manager/retry.rs index 7c4abf4d0..87ae9c9bf 100644 --- a/crates/zeph-mcp/src/manager/retry.rs +++ b/crates/zeph-mcp/src/manager/retry.rs @@ -11,10 +11,10 @@ use tokio_util::sync::CancellationToken; use dashmap::DashMap; use tokio::sync::mpsc; -use crate::client::{McpClient, ToolRefreshEvent}; +use crate::client::{EnvPolicy, McpClient, StderrPolicy, ToolRefreshEvent}; use crate::error::McpError; -use super::{McpTransport, McpTrustLevel, ServerEntry, StatusTx}; +use super::{McpTransport, ServerEntry, StatusTx}; /// Compute the sleep duration before retry attempt `attempt + 1`. /// @@ -216,6 +216,16 @@ pub(super) async fn connect_entry( ) -> Result { match &entry.transport { McpTransport::Stdio { command, args, env } => { + let stderr_policy = if suppress_stderr { + StderrPolicy::Suppress + } else { + StderrPolicy::Forward + }; + let env_policy = if entry.env_isolation { + EnvPolicy::Isolated + } else { + EnvPolicy::InheritAll + }; McpClient::connect( &entry.id, command, @@ -223,8 +233,8 @@ pub(super) async fn connect_entry( env, allowed_commands, entry.timeout, - suppress_stderr, - entry.env_isolation, + stderr_policy, + env_policy, tx, last_refresh, handler_cfg.clone(), @@ -232,13 +242,12 @@ pub(super) async fn connect_entry( .await } McpTransport::Http { url, headers } => { - let trusted = matches!(entry.trust_level, McpTrustLevel::Trusted); if headers.is_empty() { McpClient::connect_url( &entry.id, url, entry.timeout, - trusted, + entry.trust_level, tx, last_refresh, handler_cfg.clone(), @@ -250,7 +259,7 @@ pub(super) async fn connect_entry( url, headers, entry.timeout, - trusted, + entry.trust_level, tx, last_refresh, handler_cfg.clone(),