From 6a2c3dadf6a8866aeb434f4d3c41ea58b278d6c0 Mon Sep 17 00:00:00 2001 From: Mo <91650781+trickyfalcon@users.noreply.github.com> Date: Wed, 23 Sep 2026 19:00:58 +0000 Subject: [PATCH] fix: guard MCP streamable-HTTP client redirects against SSRF (loopback/link-local/metadata) (#11501) Co-authored-by: trickyfalcon Co-authored-by: Jasper Hugo --- .../extension_manager/streamable_http.rs | 114 +++++++++++++++++- crates/goose/src/providers/command_auth.rs | 9 +- 2 files changed, 116 insertions(+), 7 deletions(-) diff --git a/crates/goose/src/agents/extension_manager/streamable_http.rs b/crates/goose/src/agents/extension_manager/streamable_http.rs index 43b5871dec..722981b73a 100644 --- a/crates/goose/src/agents/extension_manager/streamable_http.rs +++ b/crates/goose/src/agents/extension_manager/streamable_http.rs @@ -216,7 +216,9 @@ fn http_client( timeout: Duration, ) -> ExtensionResult { #[allow(unused_mut)] - let mut builder = reqwest::Client::builder().default_headers(header_map(headers)?); + let mut builder = reqwest::Client::builder() + .redirect(reqwest::redirect::Policy::none()) + .default_headers(header_map(headers)?); #[cfg(target_os = "linux")] { builder = builder.tcp_user_timeout(Some(timeout)); @@ -953,6 +955,116 @@ mod tests { ); } + /// Option-B verification (unauthenticated client): a server 3xx must be + /// surfaced to the transport and the redirect target must never be contacted. + #[tokio::test] + async fn test_redirect_not_followed_unauthenticated() { + use wiremock::matchers::any; + use wiremock::{Mock, MockServer, ResponseTemplate}; + + let target = MockServer::start().await; + Mock::given(any()) + .respond_with(ResponseTemplate::new(200)) + .mount(&target) + .await; + + let redirector = MockServer::start().await; + Mock::given(any()) + .respond_with(ResponseTemplate::new(307).insert_header("location", target.uri())) + .mount(&redirector) + .await; + + let temp_dir = tempdir().unwrap(); + let result = connect( + test_params(&redirector.uri(), HashMap::new(), temp_dir.path()), + None, + Box::new(InMemoryCredentialStore::new()), + ) + .await; + + // The 3xx is surfaced (connection fails); it is not silently followed. + assert!(result.is_err(), "expected 3xx to be surfaced as an error"); + // The redirect target must never have been contacted. + let target_requests = target.received_requests().await.unwrap(); + assert!( + target_requests.is_empty(), + "redirect target was contacted: {target_requests:?}" + ); + // The redirector itself did receive the request. + assert!(!redirector.received_requests().await.unwrap().is_empty()); + } + + /// Option-B verification (authenticated client): same guarantee on the + /// `connect_with_auth` path, and the custom auth header is sent to the + /// redirector — never forwarded to the redirect target. + #[tokio::test] + async fn test_redirect_not_followed_with_auth_headers() { + use rmcp::transport::auth::{ + InMemoryCredentialStore, OAuthTokenResponse, StoredCredentials, + }; + use wiremock::matchers::any; + use wiremock::{Mock, MockServer, ResponseTemplate}; + + let target = MockServer::start().await; + Mock::given(any()) + .respond_with(ResponseTemplate::new(200)) + .mount(&target) + .await; + + let redirector = MockServer::start().await; + Mock::given(any()) + .respond_with(ResponseTemplate::new(307).insert_header("location", target.uri())) + .mount(&redirector) + .await; + + let mut headers = HashMap::new(); + headers.insert("x-api-key".to_string(), "test-secret-redirect".to_string()); + + let token_response: OAuthTokenResponse = serde_json::from_value(serde_json::json!({ + "access_token": "fake-test-token", + "token_type": "bearer", + })) + .expect("valid fake token JSON"); + let creds = StoredCredentials::new( + "test-client".to_string(), + Some(token_response), + vec![], + None, + ); + let store = InMemoryCredentialStore::new(); + store.save(creds).await.unwrap(); + + let mut auth_manager = rmcp::transport::AuthorizationManager::new(redirector.uri()) + .await + .expect("AuthorizationManager::new should not make network calls"); + auth_manager.set_credential_store(store); + + let temp_dir = tempdir().unwrap(); + let result = connect_with_auth( + auth_manager, + &redirector.uri(), + &headers, + test_ctx(temp_dir.path()), + ) + .await; + + assert!(result.is_err(), "expected 3xx to be surfaced as an error"); + let target_requests = target.received_requests().await.unwrap(); + assert!( + target_requests.is_empty(), + "redirect target was contacted: {target_requests:?}" + ); + // The auth header reached the redirector only. + let redirect_requests = redirector.received_requests().await.unwrap(); + assert!(!redirect_requests.is_empty()); + assert!(redirect_requests.iter().any(|req| { + req.headers + .get("x-api-key") + .map(|v| v == "test-secret-redirect") + .unwrap_or(false) + })); + } + mod static_oauth_client { use super::*; diff --git a/crates/goose/src/providers/command_auth.rs b/crates/goose/src/providers/command_auth.rs index 60d33eb27e..8f89c859e2 100644 --- a/crates/goose/src/providers/command_auth.rs +++ b/crates/goose/src/providers/command_auth.rs @@ -454,14 +454,11 @@ mod tests { #[cfg(unix)] #[tokio::test] async fn command_runs_with_configured_cwd_and_resolves_relative_path() { - use std::os::unix::fs::PermissionsExt; - let dir = tempfile::tempdir().unwrap(); - let script_path = dir.path().join("get-token.sh"); - std::fs::write(&script_path, "#!/bin/sh\npwd\n").unwrap(); - std::fs::set_permissions(&script_path, std::fs::Permissions::from_mode(0o755)).unwrap(); + // Avoid ETXTBSY when parallel subprocesses inherit a freshly written executable. + std::os::unix::fs::symlink("/bin/pwd", dir.path().join("get-token")).unwrap(); - let mut config = auth_config("./get-token.sh", Vec::<&str>::new(), 3600); + let mut config = auth_config("./get-token", Vec::<&str>::new(), 3600); config.cwd = Some(dir.path().to_string_lossy().to_string()); let provider = CommandAuthProvider::new(&config, "Authorization", "Bearer ");