diff --git a/CHANGELOG.md b/CHANGELOG.md index 795782650..6e968ec4a 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -30,6 +30,16 @@ The format is based on [Keep a Changelog](https://keepachangelog.com/en/1.1.0/). (`zeph-a2a/src/server/router.rs`) so `rate_limit_middleware` wraps `auth_middleware`, guaranteeing every request — including failed-auth ones — increments the counter before the auth check runs. +- `zeph-mcp`: `lock_tool_list` hardening silently exempted OAuth-transport MCP servers + from post-attestation tool-injection protection (#6118). `tool_list_locked` was only + populated by the two non-OAuth connection paths (`spawn_non_oauth_connections`, + `connect_and_list_tools`); `spawn_oauth_connections` — the sole connection path for + OAuth servers — never inserted the server ID, so `tools/list_changed` notifications + from an OAuth server were never rejected regardless of the `lock_tool_list` config + value. `spawn_oauth_connections` now inserts into `tool_list_locked` before the + handshake starts, mirroring the non-OAuth path (with matching cleanup on connection + failure in `process_oauth_results`), and the `lock_tool_list`/`tool_list_locked` + invariant now has test coverage for the first time. - `IndexMcpServer` registration (`apply_code_retrieval` in `src/agent_setup.rs`) hardcoded `std::env::current_dir()` and never read `[index] workspace_root`, unlike the sibling background-indexer path (`apply_code_indexer`), which resolved it correctly. Scoping diff --git a/crates/zeph-mcp/src/manager/connect.rs b/crates/zeph-mcp/src/manager/connect.rs index bcb3bc95f..e9a20a690 100644 --- a/crates/zeph-mcp/src/manager/connect.rs +++ b/crates/zeph-mcp/src/manager/connect.rs @@ -386,7 +386,7 @@ impl McpManager { } #[tracing::instrument(name = "mcp.manager.spawn_oauth_connections", skip_all)] - async fn spawn_oauth_connections( + pub(super) async fn spawn_oauth_connections( &self, last_refresh: &Arc>, ) -> JoinSet<(String, Result)> { @@ -437,6 +437,14 @@ impl McpManager { let status_tx = self.status_tx.clone(); let last_refresh = Arc::clone(last_refresh); + // MF-2: register the lock BEFORE spawning the OAuth handshake task so there is + // no window between handshake completion and lock insertion — mirrors the + // non-OAuth path in `spawn_non_oauth_connections`. The lock entry is removed + // in `handle_connect_result`/`process_oauth_results` if the connection fails. + if self.lock_tool_list { + self.tool_list_locked.insert(server_id.clone(), ()); + } + join_set.spawn(run_oauth_handshake( server_id, url, @@ -472,6 +480,10 @@ impl McpManager { ); } Err(error) => { + // OAuth handshake failed before a client was ever established — remove + // the lock inserted in spawn_oauth_connections so the server is not left + // permanently locked (mirrors handle_connect_result's failure cleanup). + self.tool_list_locked.remove(&server_id); outputs.push(ConnectOutput { client_entry: None, tools_entry: None, diff --git a/crates/zeph-mcp/src/manager/tests.rs b/crates/zeph-mcp/src/manager/tests.rs index 1e6040e34..871481ee0 100644 --- a/crates/zeph-mcp/src/manager/tests.rs +++ b/crates/zeph-mcp/src/manager/tests.rs @@ -655,6 +655,131 @@ async fn refresh_task_populates_and_updates_fingerprints_when_expected_tools_con ); } +// lock_tool_list / tool_list_locked tests (#6118) — OAuth-transport servers must be +// covered by the same MF-2 invariant as stdio/HTTP servers. + +fn make_oauth_entry(id: &str) -> ServerEntry { + ServerEntry { + id: id.into(), + // A loopback URL is rejected synchronously by the SSRF check for an Untrusted + // server, so the handshake fails fast without any real network I/O. + transport: McpTransport::OAuth { + url: "http://127.0.0.1:1/mcp".into(), + scopes: Vec::new(), + callback_port: 0, + client_name: "test-client".into(), + }, + timeout: Duration::from_secs(5), + trust_level: McpTrustLevel::Untrusted, + tool_allowlist: None, + expected_tools: Vec::new(), + roots: Vec::new(), + tool_metadata: HashMap::new(), + elicitation_enabled: false, + elicitation_timeout_secs: 120, + env_isolation: false, + } +} + +/// Regression for #6118: `spawn_oauth_connections` must insert the server ID into +/// `tool_list_locked` before the handshake runs, exactly like `spawn_non_oauth_connections` +/// does for stdio/HTTP servers — otherwise `lock_tool_list = true` silently exempts +/// OAuth-transport servers from the post-attestation tool-injection protection. +#[tokio::test] +async fn spawn_oauth_connections_locks_tool_list_before_handshake() { + let entry = make_oauth_entry("oauth-srv"); + let mgr = McpManager::new(vec![entry], vec![], PolicyEnforcer::new(vec![])) + .with_lock_tool_list(true) + .with_oauth_credential_store( + "oauth-srv", + Arc::new(rmcp::transport::auth::InMemoryCredentialStore::default()) + as Arc, + ); + + let last_refresh = Arc::clone(&mgr.last_refresh); + // Not draining the returned JoinSet is intentional: the lock must already be in + // place as soon as spawn_oauth_connections returns, before the handshake — which + // runs concurrently in the background and will fail fast (SSRF-blocked) — completes. + let _join_set = mgr.spawn_oauth_connections(&last_refresh).await; + + assert!( + mgr.tool_list_locked.contains_key("oauth-srv"), + "OAuth server must be locked before the handshake completes (MF-2)" + ); +} + +/// Regression for #6118: when `lock_tool_list` is disabled, OAuth servers must not be +/// locked either — the lock is opt-in, not always-on. +#[tokio::test] +async fn spawn_oauth_connections_does_not_lock_when_disabled() { + let entry = make_oauth_entry("oauth-srv-unlocked"); + let mgr = McpManager::new(vec![entry], vec![], PolicyEnforcer::new(vec![])) + .with_oauth_credential_store( + "oauth-srv-unlocked", + Arc::new(rmcp::transport::auth::InMemoryCredentialStore::default()) + as Arc, + ); + + let last_refresh = Arc::clone(&mgr.last_refresh); + let _join_set = mgr.spawn_oauth_connections(&last_refresh).await; + + assert!( + !mgr.tool_list_locked.contains_key("oauth-srv-unlocked"), + "lock_tool_list defaults to false — OAuth servers must not be locked" + ); +} + +/// Regression for #6118: a failed OAuth handshake must not leave the server permanently +/// locked — `process_oauth_results` must remove the pre-inserted lock entry on failure, +/// mirroring the cleanup `handle_connect_result` already does for the non-OAuth path. +#[tokio::test] +async fn connect_oauth_deferred_removes_lock_on_connection_failure() { + let entry = make_oauth_entry("oauth-fail"); + let mgr = McpManager::new(vec![entry], vec![], PolicyEnforcer::new(vec![])) + .with_lock_tool_list(true) + .with_oauth_credential_store( + "oauth-fail", + Arc::new(rmcp::transport::auth::InMemoryCredentialStore::default()) + as Arc, + ); + + mgr.connect_oauth_deferred().await; + + assert!( + !mgr.tool_list_locked.contains_key("oauth-fail"), + "a failed OAuth connection must not leave a permanent lock entry behind" + ); +} + +/// Regression for #6118: once a server (OAuth or otherwise) is locked, the background +/// refresh task must reject `tools/list_changed` notifications for it rather than +/// silently ingesting smuggled tools. This exercises the same rejection branch +/// (`connect.rs`'s `spawn_refresh_task`) that OAuth servers previously bypassed entirely +/// because they were never inserted into `tool_list_locked` in the first place. +#[tokio::test] +async fn refresh_task_rejects_notification_for_locked_server() { + let mgr = + McpManager::new(vec![], vec![], PolicyEnforcer::new(vec![])).with_lock_tool_list(true); + let mut rx = mgr.subscribe_tool_changes(); + // Simulate the state right after spawn_oauth_connections locks the server pre-handshake. + mgr.tool_list_locked.insert("oauth-srv".into(), ()); + mgr.spawn_refresh_task(None); + + let tx = mgr.clone_refresh_tx().unwrap(); + tx.try_send(crate::client::ToolRefreshEvent { + server_id: "oauth-srv".into(), + tools: vec![make_tool("oauth-srv", "smuggled_tool")], + }) + .unwrap(); + + // The refresh task must silently drop the event — the watch channel must never update. + let changed = tokio::time::timeout(Duration::from_millis(200), rx.changed()).await; + assert!( + changed.is_err(), + "tools/list_changed for a locked server must be rejected, not applied" + ); +} + /// Regression for #6072 (critic follow-up): the sibling test above only proves the /// fingerprint *cache* is wired (previous fingerprint stored, new fingerprint differs) — /// it does not prove the drift-comparison branch in `attest_tools` actually receives that