diff --git a/codex-rs/app-server/tests/suite/v2/executor_mcp.rs b/codex-rs/app-server/tests/suite/v2/executor_mcp.rs index b0a32df15c..8e426fe7bf 100644 --- a/codex-rs/app-server/tests/suite/v2/executor_mcp.rs +++ b/codex-rs/app-server/tests/suite/v2/executor_mcp.rs @@ -130,9 +130,11 @@ async fn selected_executor_plugin_exposes_its_mcps_only_to_that_thread() -> Resu let (registration_request_tx, mut registration_request_rx) = mpsc::unbounded_channel(); let (token_request_tx, mut token_request_rx) = mpsc::unbounded_channel(); let oauth_metadata = json!({ + "issuer": EXECUTOR_OAUTH_MCP_URL, "authorization_endpoint": "https://oauth-only.invalid/authorize", "token_endpoint": "http://oauth-only.invalid/token", "registration_endpoint": "http://oauth-only.invalid/register", + "authorization_response_iss_parameter_supported": true, "scopes_supported": ["read", "write"], "response_types_supported": ["code"], "code_challenge_methods_supported": ["S256"], @@ -345,7 +347,8 @@ startup_timeout_sec = 10 callback_url .query_pairs_mut() .append_pair("code", "configured-test-code") - .append_pair("state", ¶meters["state"]); + .append_pair("state", ¶meters["state"]) + .append_pair("iss", EXECUTOR_OAUTH_MCP_URL); HttpClientBuilder::new() .build_direct()? .get(callback_url) @@ -408,7 +411,8 @@ startup_timeout_sec = 10 callback_url .query_pairs_mut() .append_pair("code", "executor-test-code") - .append_pair("state", &state); + .append_pair("state", &state) + .append_pair("iss", EXECUTOR_OAUTH_MCP_URL); HttpClientBuilder::new() .build_direct()? .get(callback_url) diff --git a/codex-rs/rmcp-client/src/oauth.rs b/codex-rs/rmcp-client/src/oauth.rs index 304ff21002..6411551261 100644 --- a/codex-rs/rmcp-client/src/oauth.rs +++ b/codex-rs/rmcp-client/src/oauth.rs @@ -71,6 +71,7 @@ use tokio::sync::Mutex; use codex_utils_home_dir::find_codex_home; +pub(crate) use self::issuer_binding::validate_authorization_server_endpoints; pub(crate) use self::issuer_binding::validate_refresh_token_issuer; pub(crate) use self::refresh_transaction::install_tokens_in_manager; pub(crate) use self::resolved_store::ResolvedOAuthCredentialStore; diff --git a/codex-rs/rmcp-client/src/oauth/issuer_binding.rs b/codex-rs/rmcp-client/src/oauth/issuer_binding.rs index 7493117286..5a12bf4024 100644 --- a/codex-rs/rmcp-client/src/oauth/issuer_binding.rs +++ b/codex-rs/rmcp-client/src/oauth/issuer_binding.rs @@ -1,9 +1,78 @@ use anyhow::Context; use anyhow::Result; +use anyhow::bail; use rmcp::transport::auth::AuthError; use rmcp::transport::auth::AuthorizationMetadata; +use url::Url; use super::StoredOAuthTokens; + +/// Reject authorization endpoints that cannot be bound to their actual issuer. +pub(crate) fn validate_authorization_server_endpoints( + metadata: &AuthorizationMetadata, +) -> Result<()> { + let authorization_endpoint = Url::parse(&metadata.authorization_endpoint) + .context("OAuth authorization endpoint must be a valid URL")?; + let issuer = metadata + .issuer + .as_deref() + .filter(|issuer| !issuer.trim().is_empty()) + .map(Url::parse) + .transpose() + .context("OAuth authorization server issuer must be a valid URL")?; + let issuer_bound_callbacks = metadata + .additional_fields + .get("authorization_response_iss_parameter_supported") + .and_then(serde_json::Value::as_bool) + .unwrap_or(false); + + if issuer_bound_callbacks { + if issuer.is_none() { + bail!("OAuth issuer-bound callbacks require an authorization server issuer"); + } + return Ok(()); + } + + let token_endpoint = + Url::parse(&metadata.token_endpoint).context("OAuth token endpoint must be a valid URL")?; + + if let Some(issuer) = issuer { + if authorization_endpoint.origin() == issuer.origin() + || authorization_endpoint.origin() == token_endpoint.origin() + // Remove these narrow compatibility exceptions once both providers support RFC 9207. + || matches!( + ( + issuer.as_str(), + authorization_endpoint.origin().ascii_serialization().as_str(), + token_endpoint.origin().ascii_serialization().as_str(), + ), + ( + "https://api.figma.com/", + "https://www.figma.com", + "https://api.figma.com", + ) | ( + "https://agent.robinhood.com/mcp/trading", + "https://robinhood.com", + "https://api.robinhood.com", + ) + ) + { + return Ok(()); + } + bail!( + "OAuth authorization endpoint origin does not match the authorization server origin without issuer-bound callbacks" + ); + } + + if token_endpoint.origin() != authorization_endpoint.origin() { + bail!( + "OAuth token endpoint origin does not match the authorization server origin without issuer-bound callbacks" + ); + } + + Ok(()) +} + /// Verifies that a stored refresh token remains bound to its original issuer. /// /// Call this with the same metadata snapshot that RMCP will use for the credentials. Missing or diff --git a/codex-rs/rmcp-client/src/oauth_client_registration.rs b/codex-rs/rmcp-client/src/oauth_client_registration.rs index 1406c66fa9..31a547836a 100644 --- a/codex-rs/rmcp-client/src/oauth_client_registration.rs +++ b/codex-rs/rmcp-client/src/oauth_client_registration.rs @@ -9,6 +9,8 @@ use rmcp::transport::auth::OAuthHttpClient; use rmcp::transport::auth::OAuthState; use url::Url; +use crate::oauth::validate_authorization_server_endpoints; + /// OAuth client-registration strategy for one interactive HTTP MCP login. #[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] pub enum McpOAuthClientRegistration { @@ -39,6 +41,7 @@ pub(crate) async fn start_authorization( AuthorizationManager::new_with_oauth_http_client(server_url, http_client).await?; auth_manager.set_allow_missing_issuer(true); let metadata = auth_manager.resolve_metadata().await?.metadata; + validate_authorization_server_endpoints(&metadata)?; let authorization_server_issuer = metadata.issuer.clone(); let cimd_advertised = metadata diff --git a/codex-rs/rmcp-client/src/oauth_client_registration_tests.rs b/codex-rs/rmcp-client/src/oauth_client_registration_tests.rs index 935f999b7a..a776218fd8 100644 --- a/codex-rs/rmcp-client/src/oauth_client_registration_tests.rs +++ b/codex-rs/rmcp-client/src/oauth_client_registration_tests.rs @@ -9,6 +9,7 @@ use codex_http_client::HttpClientFactory; use codex_http_client::OutboundProxyPolicy; use http::HeaderMap; use pretty_assertions::assert_eq; +use rmcp::transport::auth::AuthorizationMetadata; use rmcp::transport::auth::OAuthState; use serde_json::Value; use serde_json::json; @@ -23,6 +24,7 @@ use wiremock::matchers::path; use super::McpOAuthClientRegistration; use super::start_authorization; +use crate::oauth::validate_authorization_server_endpoints; use crate::oauth_http_client::OAuthHttpClientAdapter; use crate::utils::MCP_USER_AGENT; use crate::utils::build_default_headers; @@ -209,6 +211,136 @@ async fn registration_selection_preserves_dcr_capabilities_and_exact_redirects() Ok(()) } +#[test] +fn legacy_provider_exceptions_require_exact_issuer_and_endpoint_origins() -> Result<()> { + for (issuer, authorization_endpoint, token_endpoint, accepted) in [ + ( + "https://api.figma.com", + "https://www.figma.com/oauth/mcp", + "https://api.figma.com/v1/oauth/token", + true, + ), + ( + "https://agent.robinhood.com/mcp/trading", + "https://robinhood.com/oauth", + "https://api.robinhood.com/oauth2/token/", + true, + ), + ( + "https://api.figma.com.attacker.example", + "https://www.figma.com/oauth/mcp", + "https://api.figma.com.attacker.example/token", + false, + ), + ( + "https://api.figma.com", + "https://www.figma.com/oauth/mcp", + "https://attacker.example/token", + false, + ), + ( + "http://api.figma.com", + "https://www.figma.com/oauth/mcp", + "http://api.figma.com/v1/oauth/token", + false, + ), + ( + "https://agent.robinhood.com/mcp/attacker", + "https://robinhood.com/oauth", + "https://api.robinhood.com/oauth2/token/", + false, + ), + ( + "https://agent.robinhood.com/mcp/trading", + "https://robinhood.com.attacker.example/oauth", + "https://api.robinhood.com/oauth2/token/", + false, + ), + ] { + let metadata: AuthorizationMetadata = serde_json::from_value(json!({ + "issuer": issuer, + "authorization_endpoint": authorization_endpoint, + "token_endpoint": token_endpoint, + }))?; + assert_eq!( + validate_authorization_server_endpoints(&metadata).is_ok(), + accepted, + "unexpected validation result for issuer {issuer}", + ); + } + + Ok(()) +} + +#[tokio::test] +async fn verified_issuer_can_delegate_authorization_and_token_to_one_origin() -> Result<()> { + let issuer_server = MockServer::start().await; + let endpoint_server = MockServer::start().await; + let issuer = format!("{}/mcp", issuer_server.uri()); + let redirect_uri = format!("http://127.0.0.1:43123/callback/{CALLBACK_ID}"); + + Mock::given(method("GET")) + .and(path("/.well-known/oauth-authorization-server/mcp")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "issuer": issuer, + "authorization_endpoint": format!("{}/authorize", endpoint_server.uri()), + "token_endpoint": format!("{}/token", endpoint_server.uri()), + "registration_endpoint": format!("{}/register", endpoint_server.uri()), + "token_endpoint_auth_methods_supported": ["none"], + "code_challenge_methods_supported": ["S256"], + }))) + .expect(1) + .mount(&issuer_server) + .await; + Mock::given(method("POST")) + .and(path("/register")) + .respond_with(|request: &Request| { + let registration: Value = serde_json::from_slice(&request.body) + .expect("dynamic registration should contain JSON"); + ResponseTemplate::new(200).set_body_json(json!({ + "client_id": "delegated-provider-client", + "redirect_uris": registration["redirect_uris"], + })) + }) + .expect(1) + .mount(&endpoint_server) + .await; + Mock::given(method("POST")) + .and(path("/token")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "access_token": "delegated-provider-token", + "token_type": "Bearer", + }))) + .expect(1) + .mount(&endpoint_server) + .await; + + let (mut state, query) = authorization( + &issuer_server, + &redirect_uri, + McpOAuthClientRegistration::Dcr, + ) + .await?; + assert_eq!(query["client_id"], "delegated-provider-client"); + assert_eq!( + Url::parse(&state.get_authorization_url().await?)?.origin(), + Url::parse(&endpoint_server.uri())?.origin(), + ); + + state + .handle_callback_with_issuer("valid-authorization-code", &query["state"], None) + .await?; + let token_requests = requests_to(&endpoint_server, "/token").await; + assert_eq!(token_requests.len(), 1); + assert!( + url::form_urlencoded::parse(&token_requests[0].body) + .any(|(name, _)| name == "code_verifier") + ); + issuer_server.verify().await; + endpoint_server.verify().await; + Ok(()) +} + #[tokio::test] async fn resource_headers_follow_same_origin_registration_redirect_and_sdk_auth_wins() -> Result<()> { diff --git a/codex-rs/rmcp-client/src/oauth_http_client.rs b/codex-rs/rmcp-client/src/oauth_http_client.rs index 6a6429529b..d9838bd4a5 100644 --- a/codex-rs/rmcp-client/src/oauth_http_client.rs +++ b/codex-rs/rmcp-client/src/oauth_http_client.rs @@ -21,6 +21,7 @@ use http::header::TRANSFER_ENCODING; use http::header::USER_AGENT; use oauth2::HttpRequest; use oauth2::HttpResponse; +use rmcp::transport::auth::AuthorizationMetadata; use rmcp::transport::auth::OAuthHttpClient; use rmcp::transport::auth::OAuthHttpClientError; use rmcp::transport::auth::OAuthHttpClientFuture; @@ -47,6 +48,8 @@ enum OAuthHttpClientAdapterError { TooManyRedirects, #[error("OAuth HTTP response body exceeds {maximum_bytes} bytes")] ResponseBodyTooLarge { maximum_bytes: usize }, + #[error("OAuth authorization server issuer does not match authorization metadata origin")] + AuthorizationMetadataIssuerOriginMismatch, } fn oauth_http_client_error( @@ -284,6 +287,18 @@ impl OAuthHttpClientAdapter { request_url = next_url; redirects += 1; }; + if response.status == StatusCode::OK.as_u16() + && let Ok(metadata) = serde_json::from_slice::(&body) + && let Some(issuer) = metadata.issuer.as_deref() + && Url::parse(issuer) + .map_err(oauth_http_client_error)? + .origin() + != request_url.origin() + { + return Err(oauth_http_client_error( + OAuthHttpClientAdapterError::AuthorizationMetadataIssuerOriginMismatch, + )); + } let mut builder = oauth2::http::Response::builder().status(response.status); for header in response.headers { builder = builder.header(header.name, header.value); diff --git a/codex-rs/rmcp-client/src/perform_oauth_login.rs b/codex-rs/rmcp-client/src/perform_oauth_login.rs index c44690263e..873490ed6f 100644 --- a/codex-rs/rmcp-client/src/perform_oauth_login.rs +++ b/codex-rs/rmcp-client/src/perform_oauth_login.rs @@ -28,6 +28,7 @@ use crate::StoredOAuthTokens; use crate::WrappedOAuthTokenResponse; use crate::http_client_adapter::StreamableHttpRedirectMode; use crate::oauth::compute_expires_at_millis; +use crate::oauth::validate_authorization_server_endpoints; use crate::oauth_client_registration::McpOAuthClientRegistration; use crate::oauth_client_registration::PreparedOAuthLogin; use crate::oauth_client_registration::start_authorization as start_client_registration; @@ -710,6 +711,7 @@ async fn start_authorization( AuthorizationManager::new_with_oauth_http_client(server_url, http_client).await?; auth_manager.set_allow_missing_issuer(true); let metadata = auth_manager.resolve_metadata().await?.metadata; + validate_authorization_server_endpoints(&metadata)?; let authorization_server_issuer = metadata.issuer.clone(); auth_manager.set_metadata(metadata); auth_manager.configure_client( diff --git a/codex-rs/rmcp-client/tests/mcp_2026_oauth_discovery.rs b/codex-rs/rmcp-client/tests/mcp_2026_oauth_discovery.rs index cb950a4c26..31c66bad3a 100644 --- a/codex-rs/rmcp-client/tests/mcp_2026_oauth_discovery.rs +++ b/codex-rs/rmcp-client/tests/mcp_2026_oauth_discovery.rs @@ -3,12 +3,16 @@ use std::sync::Arc; use std::time::Duration; use anyhow::Context as _; +use codex_config::types::AuthKeyringBackendKind; +use codex_config::types::OAuthCredentialsStoreMode; use codex_exec_server::Environment; use codex_exec_server::HttpClient; +use codex_rmcp_client::McpOAuthClientRegistration; use codex_rmcp_client::OAuthDiscoveryTimeout; use codex_rmcp_client::StreamableHttpOAuthDiscovery; use codex_rmcp_client::StreamableHttpRedirectMode; use codex_rmcp_client::discover_streamable_http_oauth; +use codex_rmcp_client::perform_oauth_login_return_url; use pretty_assertions::assert_eq; use rmcp::transport::auth::AuthError; use serde_json::json; @@ -236,11 +240,8 @@ async fn assert_legacy_oauth_without_starting_an_mcp_session( assert!( matches!( error.downcast_ref::(), - Some(AuthError::AuthorizationServerMismatch { - expected_issuer, - received_issuer, - }) if expected_issuer.trim_end_matches('/') == authorization_server.uri() - && received_issuer == "https://unexpected-issuer.example" + Some(AuthError::MetadataError(reason)) + if reason.contains("issuer does not match authorization metadata origin") ), "expected the original authorization-server issuer to remain bound: {error:#}", ); @@ -520,3 +521,164 @@ async fn oauth_discovery_does_not_invent_support_for_an_unauthenticated_legacy_s assert_eq!(local_discovery, None); Ok(()) } + +#[tokio::test] +async fn interactive_oauth_rejects_untrusted_authorization_metadata() -> anyhow::Result<()> { + for (metadata_issuer, authorization_metadata_path, issuer_bound_callbacks) in [ + ( + AuthorizationMetadataIssuer::Missing, + "/.well-known/untrusted-provider", + false, + ), + ( + AuthorizationMetadataIssuer::Mismatched, + "/.well-known/untrusted-provider", + true, + ), + ( + AuthorizationMetadataIssuer::Mismatched, + "/metadata.json", + false, + ), + ( + AuthorizationMetadataIssuer::Mismatched, + "/metadata.json", + true, + ), + ( + AuthorizationMetadataIssuer::Mismatched, + "/.well-known/oauth-authorization-server", + false, + ), + ( + AuthorizationMetadataIssuer::Mismatched, + "/.well-known/oauth-authorization-server", + true, + ), + ( + AuthorizationMetadataIssuer::Mismatched, + "/.well-known/openid-configuration", + true, + ), + ( + AuthorizationMetadataIssuer::Matching, + "/.well-known/untrusted-provider", + false, + ), + ] { + let resource_server = MockServer::start().await; + let authorization_server = MockServer::start().await; + let attacker_token_server = MockServer::start().await; + let resource_url = format!("{}/mcp", resource_server.uri()); + let resource_metadata_url = format!("{}/resource-metadata", resource_server.uri()); + let (issuer, expected_error) = match metadata_issuer { + AuthorizationMetadataIssuer::Missing => ( + None, + "token endpoint origin does not match the authorization server origin", + ), + AuthorizationMetadataIssuer::Mismatched => ( + Some(authorization_server.uri()), + "issuer does not match authorization metadata origin", + ), + AuthorizationMetadataIssuer::Matching => ( + Some(resource_server.uri()), + "authorization endpoint origin does not match the authorization server origin", + ), + }; + let mut authorization_metadata = json!({ + "authorization_endpoint": format!("{}/authorize", authorization_server.uri()), + "registration_endpoint": format!("{}/register", authorization_server.uri()), + "token_endpoint": format!("{}/token", attacker_token_server.uri()), + "authorization_response_iss_parameter_supported": issuer_bound_callbacks, + "code_challenge_methods_supported": ["S256"], + }); + if let Some(issuer) = issuer { + authorization_metadata["issuer"] = json!(issuer); + } + + Mock::given(method("GET")) + .and(path("/mcp")) + .respond_with(ResponseTemplate::new(401).insert_header( + "www-authenticate", + format!("Bearer resource_metadata=\"{resource_metadata_url}\""), + )) + .expect(2) + .mount(&resource_server) + .await; + Mock::given(method("GET")) + .and(path("/resource-metadata")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "resource": resource_url, + "authorization_servers": [format!( + "{}/.well-known/untrusted-provider", + resource_server.uri() + )], + }))) + .expect(2) + .mount(&resource_server) + .await; + if authorization_metadata_path != "/.well-known/untrusted-provider" { + Mock::given(method("GET")) + .and(path("/.well-known/untrusted-provider")) + .respond_with( + ResponseTemplate::new(302) + .insert_header("location", authorization_metadata_path), + ) + .expect(2) + .mount(&resource_server) + .await; + } + Mock::given(method("GET")) + .and(path(authorization_metadata_path)) + .respond_with(ResponseTemplate::new(200).set_body_json(authorization_metadata)) + .expect(2) + .mount(&resource_server) + .await; + Mock::given(method("POST")) + .respond_with(ResponseTemplate::new(200)) + .expect(0) + .mount(&attacker_token_server) + .await; + + for oauth_client_id in [None, Some("preregistered-client")] { + let error = perform_oauth_login_return_url( + "untrusted-oauth-metadata", + &resource_url, + OAuthCredentialsStoreMode::File, + AuthKeyringBackendKind::Direct, + /*http_headers*/ None, + /*env_http_headers*/ None, + /*scopes*/ &[], + oauth_client_id, + McpOAuthClientRegistration::Dcr, + /*oauth_resource*/ None, + Some(/*timeout_secs*/ 5), + /*callback_port*/ None, + /*callback_url*/ None, + local_http_client(), + StreamableHttpRedirectMode::Legacy, + ) + .await + .err() + .context("untrusted OAuth authorization metadata must fail")?; + + assert!( + format!("{error:#}").contains(expected_error), + "unexpected authorization failure for {oauth_client_id:?}: {error:#}", + ); + } + + assert!( + attacker_token_server + .received_requests() + .await + .context("attacker token server request recording should be enabled")? + .is_empty(), + "the attacker must never receive an authorization code or PKCE verifier", + ); + attacker_token_server.verify().await; + authorization_server.verify().await; + resource_server.verify().await; + } + Ok(()) +}