From 9be8d6e1c3dbb145d2d7ac3ba46729340e6d8d40 Mon Sep 17 00:00:00 2001 From: stevenlee-oai Date: Tue, 25 Aug 2026 19:55:14 +0000 Subject: [PATCH] Harden MCP OAuth callback handling (#40691) ## Why MCP servers can share an OAuth callback URL. Without a validated issuer or a server-specific callback path, an authorization response could be associated with the wrong server. ## What changed - Use stable callbacks when authorization metadata advertises issuer-bound responses, and validate the returned issuer before exchanging the code. - Retain server-specific callback IDs for providers without issuer support, including fallback to the global or default callback for legacy registered clients. - Persist registered callback URLs for MCP servers and plugins, and insert the active listener port into portless loopback redirects. ## Testing Add coverage for issuer validation, callback-mode discovery, registered and legacy clients, plugin OAuth, CLI persistence, and loopback listener ports. GitOrigin-RevId: 2878c92e237fc17fd3def0bd2e1cce3e104a3db8 --- .../src/request_processors/mcp_processor.rs | 7 + .../src/request_processors/plugins.rs | 18 +- .../tests/suite/v2/mcp_server_status.rs | 111 +++++++- .../tests/suite/v2/plugin_install.rs | 210 +++++++++++++- codex-rs/cli/src/mcp_cmd.rs | 72 +++-- codex-rs/cli/tests/mcp_add_remove.rs | 70 ++++- codex-rs/codex-mcp/src/lib.rs | 1 + codex-rs/codex-mcp/src/mcp/auth.rs | 80 ++++++ codex-rs/codex-mcp/src/mcp/mod.rs | 1 + codex-rs/codex-mcp/src/plugin_config.rs | 6 + codex-rs/codex-mcp/src/plugin_config_tests.rs | 3 +- codex-rs/config/src/mcp_types.rs | 4 + codex-rs/config/src/mcp_types_tests.rs | 2 + codex-rs/core-plugins/src/manager_tests.rs | 1 + codex-rs/core/config.schema.json | 4 + codex-rs/core/src/config/config_tests.rs | 1 + .../core/src/config/edit/document_helpers.rs | 3 + codex-rs/core/src/config/edit_tests.rs | 2 + codex-rs/core/src/mcp_skill_dependencies.rs | 3 + .../suite/mcp_startup_refresh_http_proxy.rs | 1 + codex-rs/rmcp-client/src/auth_status.rs | 53 +++- codex-rs/rmcp-client/src/lib.rs | 3 + codex-rs/rmcp-client/src/oauth_callback.rs | 148 ++++++++++ .../rmcp-client/src/oauth_callback_tests.rs | 63 +++++ .../src/oauth_client_registration.rs | 36 ++- .../src/oauth_client_registration_tests.rs | 60 +++- .../rmcp-client/src/perform_oauth_login.rs | 257 +++++++++++++----- .../tests/mcp_2026_oauth_discovery.rs | 3 + 28 files changed, 1096 insertions(+), 127 deletions(-) create mode 100644 codex-rs/rmcp-client/src/oauth_callback.rs create mode 100644 codex-rs/rmcp-client/src/oauth_callback_tests.rs diff --git a/codex-rs/app-server/src/request_processors/mcp_processor.rs b/codex-rs/app-server/src/request_processors/mcp_processor.rs index 23831743f1..299d7626fb 100644 --- a/codex-rs/app-server/src/request_processors/mcp_processor.rs +++ b/codex-rs/app-server/src/request_processors/mcp_processor.rs @@ -3,6 +3,7 @@ use super::*; use codex_core::McpManager; use codex_mcp::McpServerSource; use codex_mcp::ReadResourceRequestParams; +use codex_mcp::resolve_oauth_callback; use crate::thread_state::ThreadStateManager; @@ -204,6 +205,11 @@ impl McpRequestProcessor { let resolved_scopes = resolve_oauth_scopes(scopes, server.scopes.clone(), discovered_scopes); let oauth_credential_name = server.oauth_credential_name(&name); + let callback_url = + resolve_oauth_callback(server, &url, mcp_config.mcp_oauth_callback_url.as_deref()) + .map_err(|err| { + internal_error(format!("failed to resolve MCP OAuth callback: {err}")) + })?; let handle = perform_oauth_login_return_url( oauth_credential_name.as_ref(), @@ -218,6 +224,7 @@ impl McpRequestProcessor { server.oauth_resource.as_deref(), timeout_secs, server.oauth_callback_port(mcp_config.mcp_oauth_callback_port), + callback_url.as_deref(), mcp_config.mcp_oauth_callback_url.as_deref(), http_client, redirect_mode, diff --git a/codex-rs/app-server/src/request_processors/plugins.rs b/codex-rs/app-server/src/request_processors/plugins.rs index 64e23a01a3..b8041f0023 100644 --- a/codex-rs/app-server/src/request_processors/plugins.rs +++ b/codex-rs/app-server/src/request_processors/plugins.rs @@ -28,6 +28,7 @@ use codex_core_plugins::remote_bundle::RemotePluginBundleInstallError; use codex_mcp::McpOAuthLoginSupport; use codex_mcp::McpRuntimeContext; use codex_mcp::oauth_login_support; +use codex_mcp::resolve_oauth_callback; use codex_mcp::should_retry_without_scopes; use codex_plugin::PluginId; use codex_plugin::PluginTelemetryMetadata; @@ -1943,12 +1944,25 @@ impl PluginRequestProcessor { let store_mode = config.mcp_oauth_credentials_store_mode; let keyring_backend_kind = config.auth_keyring_backend_kind(); let callback_port = server.oauth_callback_port(config.mcp_oauth_callback_port); - let callback_url = config.mcp_oauth_callback_url.clone(); + let callback_url = match resolve_oauth_callback( + &server, + &oauth_config.url, + config.mcp_oauth_callback_url.as_deref(), + ) { + Ok(callback_url) => callback_url, + Err(error) => { + warn!( + "failed to resolve MCP OAuth callback for plugin install {name}: {error}" + ); + continue; + } + }; let outgoing = Arc::clone(&self.outgoing); let notification_name = name.clone(); let oauth_credential_name = server.oauth_credential_name(&name).into_owned(); let thread_manager = Arc::clone(&self.thread_manager); let http_client = Arc::clone(&http_client); + let global_callback_url = config.mcp_oauth_callback_url.clone(); tokio::spawn(async move { let oauth_client_id = server.oauth_client_id(); @@ -1965,6 +1979,7 @@ impl PluginRequestProcessor { server.oauth_resource.as_deref(), callback_port, callback_url.as_deref(), + global_callback_url.as_deref(), Arc::clone(&http_client), redirect_mode, ) @@ -1985,6 +2000,7 @@ impl PluginRequestProcessor { server.oauth_resource.as_deref(), callback_port, callback_url.as_deref(), + global_callback_url.as_deref(), http_client, redirect_mode, ) diff --git a/codex-rs/app-server/tests/suite/v2/mcp_server_status.rs b/codex-rs/app-server/tests/suite/v2/mcp_server_status.rs index d339c345ea..4dc085b63e 100644 --- a/codex-rs/app-server/tests/suite/v2/mcp_server_status.rs +++ b/codex-rs/app-server/tests/suite/v2/mcp_server_status.rs @@ -31,6 +31,8 @@ use codex_app_server_protocol::ThreadStartResponse; use codex_core::config::set_project_trust_level; use codex_http_client::HttpClientBuilder; use codex_protocol::config_types::TrustLevel; +use codex_rmcp_client::McpOAuthCallbackMode; +use codex_rmcp_client::resolve_mcp_oauth_callback_url; use core_test_support::skip_if_remote; use core_test_support::stdio_server_bin; use pretty_assertions::assert_eq; @@ -67,16 +69,73 @@ use wiremock::matchers::path; const DEFAULT_READ_TIMEOUT: Duration = Duration::from_secs(10); +#[test_case(false, None, None, None, true; "legacy callback")] +#[test_case( + false, + Some("http://127.0.0.1/callback/registered"), + None, + None, + true; + "ordinary configured callback falls back to default" +)] +#[test_case( + false, + Some("http://127.0.0.1/callback/registered"), + Some("http://127.0.0.1/global/callback"), + None, + true; + "ordinary configured callback falls back to global" +)] +#[test_case( + false, + Some("http://127.0.0.1/callback/registered"), + Some("http://127.0.0.1/global/callback"), + Some("https://unexpected.example"), + false; + "legacy callback fallback rejects mismatched issuer" +)] +#[test_case( + true, + Some("http://127.0.0.1/callback"), + None, + Some("matching"), + true; + "matching issuer" +)] +#[test_case( + true, + Some("http://127.0.0.1/callback"), + None, + Some("https://unexpected.example"), + false; + "mismatched issuer" +)] +#[test_case( + true, + Some("http://127.0.0.1/callback"), + None, + None, + false; + "missing issuer" +)] #[tokio::test] -async fn oauth_login_uses_http_headers_helper() -> Result<()> { +async fn oauth_login_validates_callback_issuer_and_uses_http_headers_helper( + issuer_supported: bool, + configured_callback: Option<&str>, + global_callback: Option<&str>, + callback_issuer: Option<&str>, + expected_success: bool, +) -> Result<()> { let oauth = MockServer::start().await; + let issuer = format!("{}/mcp", oauth.uri()); Mock::given(method("GET")) .and(path("/.well-known/oauth-authorization-server/mcp")) .and(header("x-gateway", "gateway-token")) .respond_with(ResponseTemplate::new(200).set_body_json(json!({ - "issuer": format!("{}/mcp", oauth.uri()), + "issuer": issuer, "authorization_endpoint": format!("{}/oauth/authorize", oauth.uri()), "token_endpoint": format!("{}/oauth/token", oauth.uri()), + "authorization_response_iss_parameter_supported": issuer_supported, }))) .mount(&oauth) .await; @@ -87,7 +146,7 @@ async fn oauth_login_uses_http_headers_helper() -> Result<()> { "access_token": "oauth-token", "token_type": "Bearer", }))) - .expect(1) + .expect(u64::from(expected_success)) .mount(&oauth) .await; @@ -97,15 +156,23 @@ async fn oauth_login_uses_http_headers_helper() -> Result<()> { } else { r#"printf '{"X-Gateway":"gateway-token"}'"# }; + let saved_callback = configured_callback + .map(|callback| format!("callback_url = \"{callback}\"\n")) + .unwrap_or_default(); + let global_callback_config = global_callback + .map(|callback| format!("mcp_oauth_callback_url = \"{callback}\"\n")) + .unwrap_or_default(); std::fs::write( codex_home.path().join("config.toml"), format!( "mcp_oauth_credentials_store = \"file\"\n\ + {global_callback_config}\ [mcp_servers.gateway]\n\ url = \"{}/mcp\"\n\ http_headers_helper = {}\n\ [mcp_servers.gateway.oauth]\n\ - client_id = \"test-client\"\n", + client_id = \"test-client\"\n\ + {saved_callback}", oauth.uri(), toml::Value::String(helper_command.to_string()), ), @@ -126,10 +193,32 @@ async fn oauth_login_uses_http_headers_helper() -> Result<()> { let authorization_url = Url::parse(&response.authorization_url)?; let query: BTreeMap<_, _> = authorization_url.query_pairs().into_owned().collect(); let mut callback_url = Url::parse(&query["redirect_uri"])?; + let expected_callback = if issuer_supported { + configured_callback + .expect("issuer-bound cases configure a stable callback") + .to_string() + } else { + resolve_mcp_oauth_callback_url( + &issuer, + global_callback, + McpOAuthCallbackMode::CallbackSpecific, + )? + }; + assert_eq!(callback_url.path(), Url::parse(&expected_callback)?.path()); callback_url .query_pairs_mut() .append_pair("code", "test-code") .append_pair("state", &query["state"]); + if let Some(callback_issuer) = callback_issuer { + let callback_issuer = if callback_issuer == "matching" { + issuer.as_str() + } else { + callback_issuer + }; + callback_url + .query_pairs_mut() + .append_pair("iss", callback_issuer); + } HttpClientBuilder::new() .build_direct()? .get(callback_url) @@ -142,13 +231,13 @@ async fn oauth_login_uses_http_headers_helper() -> Result<()> { ) .await??; assert_eq!( - completed, - McpServerOauthLoginCompletedNotification { - name: "gateway".to_string(), - thread_id: None, - success: true, - error: None, - } + ( + completed.name.as_str(), + completed.thread_id, + completed.success, + completed.error.is_some(), + ), + ("gateway", None, expected_success, !expected_success) ); oauth.verify().await; Ok(()) diff --git a/codex-rs/app-server/tests/suite/v2/plugin_install.rs b/codex-rs/app-server/tests/suite/v2/plugin_install.rs index 1d8404952e..9509fff345 100644 --- a/codex-rs/app-server/tests/suite/v2/plugin_install.rs +++ b/codex-rs/app-server/tests/suite/v2/plugin_install.rs @@ -1,4 +1,5 @@ use std::borrow::Cow; +use std::collections::BTreeMap; use std::sync::Arc; use std::sync::Mutex as StdMutex; use std::sync::atomic::AtomicUsize; @@ -27,12 +28,17 @@ use codex_app_server_protocol::AppsListParams; use codex_app_server_protocol::AppsListResponse; use codex_app_server_protocol::ListMcpServerStatusParams; use codex_app_server_protocol::ListMcpServerStatusResponse; +use codex_app_server_protocol::McpServerOauthLoginCompletedNotification; +use codex_app_server_protocol::McpServerOauthLoginResponse; use codex_app_server_protocol::PluginAuthPolicy; use codex_app_server_protocol::PluginAvailability; use codex_app_server_protocol::PluginInstallParams; use codex_app_server_protocol::PluginInstallResponse; use codex_app_server_protocol::RequestId; use codex_config::types::AuthCredentialsStoreMode; +use codex_http_client::HttpClientBuilder; +use codex_rmcp_client::McpOAuthCallbackMode; +use codex_rmcp_client::resolve_mcp_oauth_callback_url; use codex_utils_absolute_path::AbsolutePathBuf; use core_test_support::stdio_server_bin; use flate2::Compression; @@ -51,10 +57,12 @@ use rmcp::transport::StreamableHttpService; use rmcp::transport::streamable_http_server::session::local::LocalSessionManager; use serde_json::json; use tempfile::TempDir; +use test_case::test_case; use tokio::io::AsyncBufReadExt; use tokio::net::TcpListener; use tokio::task::JoinHandle; use tokio::time::timeout; +use url::Url; use wiremock::Match; use wiremock::Mock; use wiremock::MockServer; @@ -1776,7 +1784,10 @@ async fn plugin_install_starts_mcp_oauth_through_configured_http_proxy() -> Resu "sample-mcp": { "type": "http", "url": format!("{resource_url}/mcp"), - "oauth": {"callbackPort": plugin_callback_port}, + "oauth": { + "callbackPort": plugin_callback_port, + "callbackUrl": "http://127.0.0.1/plugin/callback", + }, } } }))?, @@ -1833,12 +1844,201 @@ async fn plugin_install_starts_mcp_oauth_through_configured_http_proxy() -> Resu .as_str() .expect("OAuth client registration redirect URI") .parse()?; + let expected_redirect_uri: Uri = + format!("http://127.0.0.1:{plugin_callback_port}/plugin/callback/Jb0pRxZ4-luq").parse()?; + assert_eq!(redirect_uri, expected_redirect_uri); + Ok(()) +} + +#[test_case(false, false, false; "legacy provider falls back to default callback")] +#[test_case(false, true, false; "legacy provider falls back to global callback")] +#[test_case(true, true, false; "issuer bound provider preserves registered callback")] +#[test_case(false, true, true; "legacy provider preserves server specific registered callback")] +#[tokio::test] +async fn plugin_oauth_login_preserves_registered_callbacks_or_uses_legacy_fallback( + issuer_supported: bool, + use_global_callback: bool, + callback_already_server_specific: bool, +) -> Result<()> { + let oauth = MockServer::start().await; + let authorization_server = oauth.uri(); + let server_url = format!("{authorization_server}/mcp"); + let resource_metadata_url = format!("{authorization_server}/oauth-resource"); + let challenge = format!("Bearer resource_metadata=\"{resource_metadata_url}\""); + + let global_callback = use_global_callback.then_some("http://127.0.0.1/global/callback"); + let legacy_callback = resolve_mcp_oauth_callback_url( + &server_url, + global_callback, + McpOAuthCallbackMode::CallbackSpecific, + )?; + let callback_id = legacy_callback + .rsplit('/') + .next() + .expect("legacy callback should contain the server-specific callback ID"); + let registered_callback = if callback_already_server_specific { + format!("http://127.0.0.1/callback/registered/{callback_id}") + } else { + "http://127.0.0.1/callback/registered".to_string() + }; + + let codex_home = TempDir::new()?; + let global_callback_config = global_callback + .map(|callback| format!("mcp_oauth_callback_url = \"{callback}\"\n")) + .unwrap_or_default(); + std::fs::write( + codex_home.path().join("config.toml"), + format!( + "mcp_oauth_credentials_store = \"file\"\n{global_callback_config}\n[features]\nplugins = true\n" + ), + )?; + + let repo_root = TempDir::new()?; + write_plugin_marketplace( + repo_root.path(), + "debug", + "sample-plugin", + "./sample-plugin", + /*install_policy*/ None, + /*auth_policy*/ None, + )?; + write_plugin_source(repo_root.path(), "sample-plugin", &[])?; + std::fs::write( + repo_root.path().join("sample-plugin/.mcp.json"), + serde_json::to_vec_pretty(&json!({ + "mcpServers": { + "sample-mcp": { + "type": "http", + "url": server_url, + "oauth": { + "clientId": "registered-client", + "callbackUrl": registered_callback, + }, + } + } + }))?, + )?; + let marketplace_path = + AbsolutePathBuf::try_from(repo_root.path().join(".agents/plugins/marketplace.json"))?; + + let mut mcp = TestAppServer::builder() + .with_codex_home(codex_home.path()) + .build_initialized_with_timeout(DEFAULT_TIMEOUT) + .await?; + let install_id = mcp + .send_plugin_install_request(PluginInstallParams { + marketplace_path: Some(marketplace_path), + remote_marketplace_name: None, + install_attempt_id: None, + plugin_name: "sample-plugin".to_string(), + }) + .await?; + let _: PluginInstallResponse = + timeout(DEFAULT_TIMEOUT, mcp.read_response(install_id)).await??; + + // Install before exposing OAuth metadata so automatic plugin login cannot + // launch a platform browser; the explicit public login remains end-to-end. + Mock::given(method("GET")) + .and(path("/mcp")) + .respond_with( + ResponseTemplate::new(401).insert_header("WWW-Authenticate", challenge.as_str()), + ) + .mount(&oauth) + .await; + Mock::given(method("GET")) + .and(path("/oauth-resource")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "resource": authorization_server, + "authorization_servers": [authorization_server], + }))) + .mount(&oauth) + .await; + Mock::given(method("GET")) + .and(path("/.well-known/oauth-authorization-server")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "issuer": authorization_server, + "authorization_endpoint": format!("{authorization_server}/oauth/authorize"), + "token_endpoint": format!("{authorization_server}/oauth/token"), + "registration_endpoint": format!("{authorization_server}/oauth/register"), + "response_types_supported": ["code"], + "code_challenge_methods_supported": ["S256"], + "authorization_response_iss_parameter_supported": issuer_supported, + }))) + .mount(&oauth) + .await; + Mock::given(method("POST")) + .and(path("/oauth/register")) + .respond_with(ResponseTemplate::new(400)) + .expect(0) + .mount(&oauth) + .await; + Mock::given(method("POST")) + .and(path("/oauth/token")) + .respond_with(ResponseTemplate::new(200).set_body_json(json!({ + "access_token": "registered-plugin-token", + "token_type": "Bearer", + }))) + .expect(1) + .mount(&oauth) + .await; + + let login_id = mcp + .send_raw_request( + "mcpServer/oauth/login", + Some(json!({ + "name": "sample-mcp", + "scopes": ["read"], + "timeoutSecs": 10, + })), + ) + .await?; + let response: McpServerOauthLoginResponse = + timeout(DEFAULT_TIMEOUT, mcp.read_response(login_id)).await??; + let authorization_url = Url::parse(&response.authorization_url)?; + let query: BTreeMap<_, _> = authorization_url.query_pairs().into_owned().collect(); assert_eq!( - redirect_uri - .authority() - .and_then(axum::http::uri::Authority::port_u16), - Some(plugin_callback_port) + query.get("client_id").map(String::as_str), + Some("registered-client") ); + + let mut callback_url = Url::parse(&query["redirect_uri"])?; + let expected_callback = if issuer_supported || callback_already_server_specific { + registered_callback.as_str() + } else { + legacy_callback.as_str() + }; + assert_eq!(callback_url.path(), Url::parse(expected_callback)?.path()); + callback_url + .query_pairs_mut() + .append_pair("code", "registered-plugin-code") + .append_pair("state", &query["state"]); + if issuer_supported { + callback_url + .query_pairs_mut() + .append_pair("iss", &authorization_server); + } + HttpClientBuilder::new() + .build_direct()? + .get(callback_url) + .send() + .await? + .error_for_status()?; + + let completed: McpServerOauthLoginCompletedNotification = timeout( + DEFAULT_TIMEOUT, + mcp.read_notification("mcpServer/oauthLogin/completed"), + ) + .await??; + assert_eq!( + completed, + McpServerOauthLoginCompletedNotification { + name: "sample-mcp".to_string(), + thread_id: None, + success: true, + error: None, + } + ); + oauth.verify().await; Ok(()) } diff --git a/codex-rs/cli/src/mcp_cmd.rs b/codex-rs/cli/src/mcp_cmd.rs index daaf3e4532..acc695eaf5 100644 --- a/codex-rs/cli/src/mcp_cmd.rs +++ b/codex-rs/cli/src/mcp_cmd.rs @@ -29,14 +29,17 @@ use codex_mcp::apply_http_headers_helper; use codex_mcp::compute_auth_statuses; use codex_mcp::discover_supported_scopes; use codex_mcp::oauth_login_support; +use codex_mcp::resolve_oauth_callback; use codex_mcp::resolve_oauth_scopes; use codex_mcp::should_retry_without_scopes; use codex_protocol::protocol::McpAuthStatus; +use codex_rmcp_client::McpOAuthCallbackMode; use codex_rmcp_client::McpOAuthClientRegistration; use codex_rmcp_client::OAuthDiscoveryTimeout; use codex_rmcp_client::StreamableHttpRedirectMode; use codex_rmcp_client::delete_oauth_tokens; use codex_rmcp_client::perform_oauth_login; +use codex_rmcp_client::resolve_mcp_oauth_callback_url; use codex_utils_cli::CliConfigOverrides; use codex_utils_cli::format_env_display; @@ -270,6 +273,7 @@ async fn perform_oauth_login_retry_without_scopes( oauth_resource: Option<&str>, callback_port: Option, callback_url: Option<&str>, + global_callback_url: Option<&str>, http_client: Arc, ) -> Result<()> { match perform_oauth_login( @@ -285,6 +289,7 @@ async fn perform_oauth_login_retry_without_scopes( oauth_resource, callback_port, callback_url, + global_callback_url, Arc::clone(&http_client), ) .await @@ -305,6 +310,7 @@ async fn perform_oauth_login_retry_without_scopes( oauth_resource, callback_port, callback_url, + global_callback_url, http_client, ) .await @@ -346,9 +352,6 @@ async fn run_add(config_overrides: &CliConfigOverrides, add_args: AddArgs) -> Re validate_server_name(&name)?; let codex_home = find_codex_home().context("failed to resolve CODEX_HOME")?; - let mut servers = load_global_mcp_servers(&codex_home) - .await - .with_context(|| format!("failed to load MCP servers from {}", codex_home.display()))?; let (transport, oauth_client_id, client_registration, oauth_resource) = match transport_args { AddMcpTransportArgs { @@ -405,6 +408,43 @@ async fn run_add(config_overrides: &CliConfigOverrides, add_args: AddArgs) -> Re AddMcpTransportArgs { .. } => bail!("exactly one of --command or --url must be provided"), }; + // Discover once before saving so a new registered client keeps the exact + // callback its provider expects, including issuer-bound stable callbacks. + let http_client: Arc = Arc::new( + RouteAwareHttpClient::new(config.http_client_factory()).with_tls_backend_fallback(), + ); + let login_support = oauth_login_support( + &transport, + Arc::clone(&http_client), + OAuthDiscoveryTimeout::LOCAL, + StreamableHttpRedirectMode::Legacy, + ) + .await; + // A stable callback requires confirmed issuer support. Discovery failures + // and servers without support both retain the callback-ID mix-up defense. + let callback_mode = match &login_support { + McpOAuthLoginSupport::Supported(oauth_config) => oauth_config.callback_mode, + McpOAuthLoginSupport::Unsupported | McpOAuthLoginSupport::Unknown(_) => { + McpOAuthCallbackMode::CallbackSpecific + } + }; + + // DCR and CIMD register their callbacks during login; only an existing + // client ID has a redirect that must be persisted with the MCP server. + let callback_url = oauth_client_id + .as_ref() + .map(|_| { + let McpServerTransportConfig::StreamableHttp { url, .. } = &transport else { + bail!("OAuth client IDs require a streamable HTTP MCP server"); + }; + resolve_mcp_oauth_callback_url( + url, + config.mcp_oauth_callback_url.as_deref(), + callback_mode, + ) + }) + .transpose()?; + let new_entry = McpServerConfig { auth: Default::default(), transport: transport.clone(), @@ -424,12 +464,16 @@ async fn run_add(config_overrides: &CliConfigOverrides, add_args: AddArgs) -> Re .clone() .map(|client_id| McpServerOAuthConfig { client_id: Some(client_id), + callback_url: callback_url.clone(), callback_port: None, }), oauth_resource: oauth_resource.clone(), tools: HashMap::new(), }; + let mut servers = load_global_mcp_servers(&codex_home) + .await + .with_context(|| format!("failed to load MCP servers from {}", codex_home.display()))?; servers.insert(name.clone(), new_entry); ConfigEditsBuilder::new(&codex_home) @@ -439,19 +483,9 @@ async fn run_add(config_overrides: &CliConfigOverrides, add_args: AddArgs) -> Re .with_context(|| format!("failed to write MCP servers to {}", codex_home.display()))?; println!("Added global MCP server '{name}'."); - - // `mcp add` assigns new servers to the local environment, so its immediate - // OAuth discovery uses the local route-aware HTTP client. - let http_client: Arc = Arc::new( - RouteAwareHttpClient::new(config.http_client_factory()).with_tls_backend_fallback(), - ); - let login_support = oauth_login_support( - &transport, - Arc::clone(&http_client), - OAuthDiscoveryTimeout::LOCAL, - StreamableHttpRedirectMode::Legacy, - ) - .await; + if let Some(callback_url) = &callback_url { + println!("OAuth callback URL: {callback_url}"); + } match login_support { McpOAuthLoginSupport::Supported(oauth_config) => { println!("Detected OAuth support. Starting OAuth flow…"); @@ -472,6 +506,9 @@ async fn run_add(config_overrides: &CliConfigOverrides, add_args: AddArgs) -> Re client_registration, oauth_resource.as_deref(), config.mcp_oauth_callback_port, + callback_url + .as_deref() + .or(config.mcp_oauth_callback_url.as_deref()), config.mcp_oauth_callback_url.as_deref(), http_client, ) @@ -577,6 +614,8 @@ async fn run_login(config: &Config, login_args: LoginArgs) -> Result<()> { let resolved_scopes = resolve_oauth_scopes(explicit_scopes, server.scopes.clone(), discovered_scopes); let credential_name = server.oauth_credential_name(&name); + let callback_url = + resolve_oauth_callback(server, &url, config.mcp_oauth_callback_url.as_deref())?; perform_oauth_login_retry_without_scopes( credential_name.as_ref(), @@ -590,6 +629,7 @@ async fn run_login(config: &Config, login_args: LoginArgs) -> Result<()> { client_registration, server.oauth_resource.as_deref(), server.oauth_callback_port(config.mcp_oauth_callback_port), + callback_url.as_deref(), config.mcp_oauth_callback_url.as_deref(), http_client, ) diff --git a/codex-rs/cli/tests/mcp_add_remove.rs b/codex-rs/cli/tests/mcp_add_remove.rs index 7103c3712e..c5c65b9d9f 100644 --- a/codex-rs/cli/tests/mcp_add_remove.rs +++ b/codex-rs/cli/tests/mcp_add_remove.rs @@ -342,10 +342,13 @@ async fn add_streamable_http_with_custom_env_var() -> Result<()> { #[tokio::test] async fn add_streamable_http_with_oauth_options() -> Result<()> { let codex_home = TempDir::new()?; + let expected_callback = "http://127.0.0.1/callback/w9gKTtkB7gWy"; let mut add_cmd = codex_command(codex_home.path())?; add_cmd .args([ + "-c", + "mcp_oauth_callback_port=43123", "mcp", "add", "oauth-server", @@ -357,7 +360,8 @@ async fn add_streamable_http_with_oauth_options() -> Result<()> { "https://resource.example.com", ]) .assert() - .success(); + .success() + .stdout(contains(format!("OAuth callback URL: {expected_callback}"))); let servers = load_global_mcp_servers(codex_home.path()).await?; let oauth_server = servers @@ -367,6 +371,13 @@ async fn add_streamable_http_with_oauth_options() -> Result<()> { oauth_server.oauth_client_id(), Some("eci-prd-pub-codex-123") ); + assert_eq!( + oauth_server + .oauth + .as_ref() + .and_then(|oauth| oauth.callback_url.as_deref()), + Some(expected_callback) + ); assert_eq!( oauth_server.oauth_resource.as_deref(), Some("https://resource.example.com") @@ -375,6 +386,63 @@ async fn add_streamable_http_with_oauth_options() -> Result<()> { Ok(()) } +#[tokio::test] +async fn add_persists_issuer_bound_callback_before_starting_oauth() -> Result<()> { + let codex_home = TempDir::new()?; + let oauth_server = MockServer::start().await; + let issuer = format!("{}/mcp", oauth_server.uri()); + let metadata = serde_json::json!({ + "issuer": issuer, + "authorization_endpoint": "not-a-valid-authorization-url", + "token_endpoint": format!("{}/token", oauth_server.uri()), + "authorization_response_iss_parameter_supported": true, + }); + let concurrent_codex_home = codex_home.path().to_path_buf(); + let concurrent_add = std::sync::Once::new(); + Mock::given(method("GET")) + .and(path("/.well-known/oauth-authorization-server/mcp")) + .respond_with(move |_: &wiremock::Request| { + concurrent_add.call_once(|| { + codex_command(&concurrent_codex_home) + .expect("create concurrent MCP add command") + .args(["mcp", "add", "concurrent", "--", "echo", "concurrent"]) + .assert() + .success(); + }); + ResponseTemplate::new(200).set_body_json(metadata.clone()) + }) + .mount(&oauth_server) + .await; + + let mut add_cmd = codex_command(codex_home.path())?; + add_cmd.args([ + "mcp", + "add", + "issuer-bound", + "--url", + &format!("{}/mcp", oauth_server.uri()), + "--oauth-client-id", + "registered-client", + ]); + let output = tokio::task::spawn_blocking(move || add_cmd.output()).await??; + + assert!(!output.status.success()); + assert!( + String::from_utf8(output.stdout)?.contains("OAuth callback URL: http://127.0.0.1/callback") + ); + let servers = load_global_mcp_servers(codex_home.path()).await?; + assert!(servers.contains_key("concurrent")); + assert_eq!( + servers["issuer-bound"] + .oauth + .as_ref() + .and_then(|oauth| oauth.callback_url.as_deref()), + Some("http://127.0.0.1/callback") + ); + + Ok(()) +} + #[tokio::test] async fn add_streamable_http_rejects_removed_flag() -> Result<()> { let codex_home = TempDir::new()?; diff --git a/codex-rs/codex-mcp/src/lib.rs b/codex-rs/codex-mcp/src/lib.rs index 6066839591..9d24d2ddc0 100644 --- a/codex-rs/codex-mcp/src/lib.rs +++ b/codex-rs/codex-mcp/src/lib.rs @@ -83,6 +83,7 @@ pub use mcp::ResolvedMcpOAuthScopes; pub use mcp::compute_auth_statuses; pub use mcp::discover_supported_scopes; pub use mcp::oauth_login_support; +pub use mcp::resolve_oauth_callback; pub use mcp::resolve_oauth_scopes; pub use mcp::should_retry_without_scopes; diff --git a/codex-rs/codex-mcp/src/mcp/auth.rs b/codex-rs/codex-mcp/src/mcp/auth.rs index 0b1b843690..137f0ba3ba 100644 --- a/codex-rs/codex-mcp/src/mcp/auth.rs +++ b/codex-rs/codex-mcp/src/mcp/auth.rs @@ -10,12 +10,14 @@ use codex_config::types::OAuthCredentialsStoreMode; use codex_exec_server::HttpClient; use codex_login::CodexAuth; use codex_rmcp_client::McpAuthState; +use codex_rmcp_client::McpOAuthCallbackMode; use codex_rmcp_client::OAuthDiscoveryTimeout; use codex_rmcp_client::OAuthProviderError; use codex_rmcp_client::StreamableHttpRedirectMode; use codex_rmcp_client::determine_streamable_http_auth_status; use codex_rmcp_client::determine_streamable_http_auth_status_from_credentials; use codex_rmcp_client::discover_streamable_http_oauth; +use codex_rmcp_client::resolve_mcp_oauth_callback_url; use futures::FutureExt; use futures::future::join_all; use tracing::warn; @@ -30,6 +32,7 @@ pub struct McpOAuthLoginConfig { pub http_headers: Option>, pub env_http_headers: Option>, pub discovered_scopes: Option>, + pub callback_mode: McpOAuthCallbackMode, } #[derive(Debug)] @@ -53,6 +56,35 @@ pub struct ResolvedMcpOAuthScopes { pub source: McpOAuthScopesSource, } +/// Keeps registered callbacks tied to their client while preserving legacy redirects. +pub fn resolve_oauth_callback( + server: &McpServerConfig, + server_url: &str, + global_callback_url: Option<&str>, +) -> Result> { + if let Some(callback_url) = server + .oauth + .as_ref() + .and_then(|oauth| oauth.callback_url.as_deref()) + { + return Ok(Some(callback_url.to_string())); + } + + if server + .oauth_client_id() + .is_none_or(|client_id| client_id.trim().is_empty()) + { + return Ok(global_callback_url.map(ToOwned::to_owned)); + } + + resolve_mcp_oauth_callback_url( + server_url, + global_callback_url, + McpOAuthCallbackMode::CallbackSpecific, + ) + .map(Some) +} + #[derive(Debug, Clone)] pub struct McpAuthStatusEntry { pub config: Option, @@ -80,6 +112,7 @@ pub async fn oauth_login_support( { Ok(Some(discovery)) => { config.discovered_scopes = discovery.scopes_supported; + config.callback_mode = discovery.callback_mode; McpOAuthLoginSupport::Supported(config) } Ok(None) => McpOAuthLoginSupport::Unsupported, @@ -106,6 +139,7 @@ fn oauth_login_candidate(transport: &McpServerTransportConfig) -> Option anyhow::Result<()> { + for (client_id, saved_callback, global_callback, expected_callback) in [ + ( + Some("registered-client"), + Some("http://127.0.0.1/callback"), + Some("https://override.example/callback"), + Some("http://127.0.0.1/callback"), + ), + ( + Some("legacy-client"), + None, + None, + Some("http://127.0.0.1/callback/epMNJ6P1xGQ9"), + ), + ( + None, + Some("https://plugin.example/callback"), + Some("https://override.example/callback"), + Some("https://plugin.example/callback"), + ), + ( + None, + None, + Some("https://override.example/callback"), + Some("https://override.example/callback"), + ), + ] { + let server = serde_json::from_value(serde_json::json!({ + "url": "https://mcp.example.com/mcp", + "oauth": { + "client_id": client_id, + "callback_url": saved_callback, + }, + }))?; + assert_eq!( + resolve_oauth_callback(&server, "https://mcp.example.com/mcp", global_callback)? + .as_deref(), + expected_callback + ); + } + + Ok(()) + } + #[test] fn resolve_oauth_scopes_prefers_explicit() { let resolved = resolve_oauth_scopes( diff --git a/codex-rs/codex-mcp/src/mcp/mod.rs b/codex-rs/codex-mcp/src/mcp/mod.rs index 564d690ecf..3f3e0195ef 100644 --- a/codex-rs/codex-mcp/src/mcp/mod.rs +++ b/codex-rs/codex-mcp/src/mcp/mod.rs @@ -6,6 +6,7 @@ pub use auth::ResolvedMcpOAuthScopes; pub use auth::compute_auth_statuses; pub use auth::discover_supported_scopes; pub use auth::oauth_login_support; +pub use auth::resolve_oauth_callback; pub use auth::resolve_oauth_scopes; pub use auth::should_retry_without_scopes; diff --git a/codex-rs/codex-mcp/src/plugin_config.rs b/codex-rs/codex-mcp/src/plugin_config.rs index e6fc333d86..bdd9c27b83 100644 --- a/codex-rs/codex-mcp/src/plugin_config.rs +++ b/codex-rs/codex-mcp/src/plugin_config.rs @@ -257,6 +257,12 @@ fn normalize_plugin_mcp_server_value( } if let Some(JsonValue::Object(mut oauth)) = object.remove("oauth") { + if let Some(callback_url) = oauth.remove("callbackUrl") { + oauth + .entry("callback_url".to_string()) + .or_insert(callback_url); + } + if let Some(callback_port) = oauth.remove("callbackPort") { oauth .entry("callback_port".to_string()) diff --git a/codex-rs/codex-mcp/src/plugin_config_tests.rs b/codex-rs/codex-mcp/src/plugin_config_tests.rs index 57012c9249..c09345afa4 100644 --- a/codex-rs/codex-mcp/src/plugin_config_tests.rs +++ b/codex-rs/codex-mcp/src/plugin_config_tests.rs @@ -582,6 +582,7 @@ fn declared_placement_preserves_local_plugin_normalization() { scopes: None, oauth: Some(McpServerOAuthConfig { client_id: Some("client-id".to_string()), + callback_url: Some("http://127.0.0.1/callback/registered".to_string()), callback_port: Some(9876), }), oauth_resource: None, @@ -611,7 +612,7 @@ fn declared_placement_preserves_local_plugin_normalization() { "hosted": { "type": "http", "url": "https://example.com/mcp", - "oauth": {"clientId": "client-id", "callbackPort": 9876} + "oauth": {"clientId": "client-id", "callbackUrl": "http://127.0.0.1/callback/registered", "callbackPort": 9876} }, "helper": {"type":"http","url":"https://example.com/mcp","http_headers_helper":"./auth.sh"} }"#, diff --git a/codex-rs/config/src/mcp_types.rs b/codex-rs/config/src/mcp_types.rs index b914633e26..44d3f85302 100644 --- a/codex-rs/config/src/mcp_types.rs +++ b/codex-rs/config/src/mcp_types.rs @@ -146,6 +146,10 @@ pub struct McpServerOAuthConfig { #[serde(default, skip_serializing_if = "Option::is_none")] pub client_id: Option, + /// Registered callback URL associated with this OAuth client. + #[serde(default, skip_serializing_if = "Option::is_none")] + pub callback_url: Option, + /// Fixed callback port that takes precedence over Codex's global OAuth callback port. #[serde(default, skip_serializing_if = "Option::is_none")] pub callback_port: Option, diff --git a/codex-rs/config/src/mcp_types_tests.rs b/codex-rs/config/src/mcp_types_tests.rs index d1b0754b20..d0aafaf54d 100644 --- a/codex-rs/config/src/mcp_types_tests.rs +++ b/codex-rs/config/src/mcp_types_tests.rs @@ -364,6 +364,7 @@ fn deserialize_streamable_http_server_config_with_oauth_client_id() { [oauth] client_id = "eci-prd-pub-codex-123" + callback_url = "http://127.0.0.1/callback/registered" callback_port = 9876 "#, ) @@ -373,6 +374,7 @@ fn deserialize_streamable_http_server_config_with_oauth_client_id() { cfg.oauth, Some(McpServerOAuthConfig { client_id: Some("eci-prd-pub-codex-123".to_string()), + callback_url: Some("http://127.0.0.1/callback/registered".to_string()), callback_port: Some(9876), }) ); diff --git a/codex-rs/core-plugins/src/manager_tests.rs b/codex-rs/core-plugins/src/manager_tests.rs index 1aae6242fd..9886cdef83 100644 --- a/codex-rs/core-plugins/src/manager_tests.rs +++ b/codex-rs/core-plugins/src/manager_tests.rs @@ -982,6 +982,7 @@ async fn load_plugins_loads_default_skills_and_mcp_servers() { scopes: None, oauth: Some(McpServerOAuthConfig { client_id: Some("client-id".to_string()), + callback_url: None, callback_port: Some(3118), }), oauth_resource: None, diff --git a/codex-rs/core/config.schema.json b/codex-rs/core/config.schema.json index 715225b0a7..bc51420f1a 100644 --- a/codex-rs/core/config.schema.json +++ b/codex-rs/core/config.schema.json @@ -1920,6 +1920,10 @@ "minimum": 0.0, "type": "integer" }, + "callback_url": { + "description": "Registered callback URL associated with this OAuth client.", + "type": "string" + }, "client_id": { "description": "Explicit OAuth client identifier to present during authorization and token exchange.", "type": "string" diff --git a/codex-rs/core/src/config/config_tests.rs b/codex-rs/core/src/config/config_tests.rs index cb5ce33086..4cc4bc7e90 100644 --- a/codex-rs/core/src/config/config_tests.rs +++ b/codex-rs/core/src/config/config_tests.rs @@ -7830,6 +7830,7 @@ async fn replace_mcp_servers_streamable_http_serializes_oauth_resource() -> anyh scopes: None, oauth: Some(McpServerOAuthConfig { client_id: Some("eci-prd-pub-codex-123".to_string()), + callback_url: None, callback_port: None, }), oauth_resource: Some("https://resource.example.com".to_string()), diff --git a/codex-rs/core/src/config/edit/document_helpers.rs b/codex-rs/core/src/config/edit/document_helpers.rs index 769341dce8..60883137d0 100644 --- a/codex-rs/core/src/config/edit/document_helpers.rs +++ b/codex-rs/core/src/config/edit/document_helpers.rs @@ -155,6 +155,9 @@ fn serialize_mcp_server_table(config: &McpServerConfig) -> TomlTable { { oauth_table["client_id"] = value(client_id.clone()); } + if let Some(callback_url) = &oauth.callback_url { + oauth_table["callback_url"] = value(callback_url.clone()); + } if let Some(callback_port) = oauth.callback_port { oauth_table["callback_port"] = value(i64::from(callback_port)); } diff --git a/codex-rs/core/src/config/edit_tests.rs b/codex-rs/core/src/config/edit_tests.rs index b2ace568ef..1afcafccd0 100644 --- a/codex-rs/core/src/config/edit_tests.rs +++ b/codex-rs/core/src/config/edit_tests.rs @@ -1087,6 +1087,7 @@ fn blocking_replace_mcp_servers_round_trips() { scopes: None, oauth: Some(McpServerOAuthConfig { client_id: Some("eci-prd-pub-codex-123".to_string()), + callback_url: Some("http://127.0.0.1/callback/example".to_string()), callback_port: Some(9876), }), oauth_resource: Some("https://resource.example.com".to_string()), @@ -1116,6 +1117,7 @@ Z-Header = \"z\" [mcp_servers.http.oauth] client_id = \"eci-prd-pub-codex-123\" +callback_url = \"http://127.0.0.1/callback/example\" callback_port = 9876 [mcp_servers.stdio] diff --git a/codex-rs/core/src/mcp_skill_dependencies.rs b/codex-rs/core/src/mcp_skill_dependencies.rs index fdb1cb10da..1d4f8fc26c 100644 --- a/codex-rs/core/src/mcp_skill_dependencies.rs +++ b/codex-rs/core/src/mcp_skill_dependencies.rs @@ -205,6 +205,7 @@ async fn maybe_install_mcp_dependencies( server_config.oauth_resource.as_deref(), callback_port, config.mcp_oauth_callback_url.as_deref(), + config.mcp_oauth_callback_url.as_deref(), Arc::clone(&http_client), ) .await; @@ -224,6 +225,7 @@ async fn maybe_install_mcp_dependencies( server_config.oauth_resource.as_deref(), callback_port, config.mcp_oauth_callback_url.as_deref(), + config.mcp_oauth_callback_url.as_deref(), Arc::clone(&http_client), ) .await @@ -427,6 +429,7 @@ fn mcp_dependency_to_server_config( .oauth_callback_port .map(|callback_port| McpServerOAuthConfig { client_id: None, + callback_url: None, callback_port: Some(callback_port), }), oauth_resource: None, diff --git a/codex-rs/core/tests/suite/mcp_startup_refresh_http_proxy.rs b/codex-rs/core/tests/suite/mcp_startup_refresh_http_proxy.rs index 0e8dd9d6d7..16af085f95 100644 --- a/codex-rs/core/tests/suite/mcp_startup_refresh_http_proxy.rs +++ b/codex-rs/core/tests/suite/mcp_startup_refresh_http_proxy.rs @@ -416,6 +416,7 @@ async fn skill_mcp_dependency_oauth_uses_configured_http_client() -> Result<()> .and_then(|server| server.oauth.as_ref()), Some(&McpServerOAuthConfig { client_id: None, + callback_url: None, callback_port: Some(skill_callback_port), }) ); diff --git a/codex-rs/rmcp-client/src/auth_status.rs b/codex-rs/rmcp-client/src/auth_status.rs index 1d141a36ff..479ae184f4 100644 --- a/codex-rs/rmcp-client/src/auth_status.rs +++ b/codex-rs/rmcp-client/src/auth_status.rs @@ -15,6 +15,8 @@ use tracing::debug; use crate::http_client_adapter::StreamableHttpRedirectMode; use crate::oauth::StoredOAuthTokenStatus; use crate::oauth::oauth_token_status; +use crate::oauth_callback::McpOAuthCallbackMode; +use crate::oauth_callback::callback_mode; use crate::oauth_http_client::OAuthHttpClientAdapter; use crate::utils::build_default_headers; use codex_config::types::AuthKeyringBackendKind; @@ -39,6 +41,7 @@ impl OAuthDiscoveryTimeout { #[derive(Debug, Clone, PartialEq, Eq)] pub struct StreamableHttpOAuthDiscovery { pub scopes_supported: Option>, + pub callback_mode: McpOAuthCallbackMode, } #[derive(Debug, Clone, Copy, PartialEq, Eq)] @@ -263,9 +266,14 @@ async fn discover_streamable_http_oauth_with_manager( ) -> Result> { match authorization_manager.resolve_metadata().boxed().await { Ok(resolution) if !resolution.source.is_discovered() => Ok(None), - Ok(resolution) => Ok(Some(StreamableHttpOAuthDiscovery { - scopes_supported: normalize_scopes(resolution.metadata.scopes_supported), - })), + Ok(resolution) => { + let metadata = resolution.metadata; + Ok(Some(StreamableHttpOAuthDiscovery { + callback_mode: callback_mode(&metadata) + .unwrap_or(McpOAuthCallbackMode::CallbackSpecific), + scopes_supported: normalize_scopes(metadata.scopes_supported), + })) + } Err(AuthError::NoAuthorizationSupport) => Ok(None), Err(err) => Err(err.into()), } @@ -668,6 +676,7 @@ mod tests { let server = spawn_oauth_discovery_server(serde_json::json!({ "authorization_endpoint": "https://example.com/authorize", "token_endpoint": "https://example.com/token", + "authorization_response_iss_parameter_supported": true, "scopes_supported": ["profile", " email ", "profile", "", " "], })) .await; @@ -685,8 +694,42 @@ mod tests { .expect("oauth support should be detected"); assert_eq!( - discovery.scopes_supported, - Some(vec!["profile".to_string(), "email".to_string()]) + discovery, + StreamableHttpOAuthDiscovery { + scopes_supported: Some(vec!["profile".to_string(), "email".to_string()]), + callback_mode: McpOAuthCallbackMode::IssuerBound, + } + ); + } + + #[tokio::test] + async fn issuer_support_without_a_metadata_issuer_falls_back_to_distinct_callbacks() { + let server = spawn_oauth_discovery_server(serde_json::json!({ + "issuer": null, + "authorization_endpoint": "https://example.com/authorize", + "token_endpoint": "https://example.com/token", + "authorization_response_iss_parameter_supported": true, + })) + .await; + + let discovery = discover_streamable_http_oauth( + &server.url, + /*http_headers*/ None, + /*env_http_headers*/ None, + test_http_client(), + OAuthDiscoveryTimeout::LOCAL, + StreamableHttpRedirectMode::Legacy, + ) + .await + .expect("discovery should succeed") + .expect("oauth support should be detected"); + + assert_eq!( + discovery, + StreamableHttpOAuthDiscovery { + scopes_supported: None, + callback_mode: McpOAuthCallbackMode::CallbackSpecific, + } ); } diff --git a/codex-rs/rmcp-client/src/lib.rs b/codex-rs/rmcp-client/src/lib.rs index 396e1e4853..b5184ba60b 100644 --- a/codex-rs/rmcp-client/src/lib.rs +++ b/codex-rs/rmcp-client/src/lib.rs @@ -9,6 +9,7 @@ mod in_process_transport; mod local_stdio_transport; mod logging_client_handler; mod oauth; +mod oauth_callback; mod oauth_client_registration; mod oauth_http_client; mod perform_oauth_login; @@ -38,6 +39,8 @@ pub use oauth::delete_oauth_tokens; pub use oauth::save_oauth_tokens; pub use oauth::stored_oauth_credential_snapshot; pub use oauth::stored_oauth_credentials; +pub use oauth_callback::McpOAuthCallbackMode; +pub use oauth_callback::resolve_mcp_oauth_callback_url; pub use oauth_client_registration::McpOAuthClientRegistration; pub use perform_oauth_login::OAuthProviderError; pub use perform_oauth_login::OauthLoginHandle; diff --git a/codex-rs/rmcp-client/src/oauth_callback.rs b/codex-rs/rmcp-client/src/oauth_callback.rs new file mode 100644 index 0000000000..eb3d7f3176 --- /dev/null +++ b/codex-rs/rmcp-client/src/oauth_callback.rs @@ -0,0 +1,148 @@ +//! OAuth callback identity and authorization-server mix-up protection. +//! +//! Codex can authorize against many independent MCP servers. If those servers +//! share a callback URL and a response does not identify its authorization +//! server, Codex could associate an authorization code with the wrong server +//! and send that code to an attacker-controlled token endpoint. RFC 9700 calls +//! this an authorization-server mix-up attack: +//! https://www.rfc-editor.org/rfc/rfc9700#section-4.4 +//! +//! MCP prefers issuer identification: authorization servers SHOULD include +//! `iss` in authorization responses; servers that include `iss` MUST advertise +//! that support, and clients MUST validate any returned issuer before +//! exchanging the code. This also lets a CIMD document advertise one stable +//! redirect instead of separate redirects for every authorization server: +//! https://modelcontextprotocol.io/specification/2026-07-28/basic/authorization#authorization-response-validation +//! +//! This module prefers issuer binding when available and otherwise retains a +//! callback-specific compatibility fallback: +//! +//! - `IssuerBound`: reuse a stable callback only when authorization metadata +//! advertises `authorization_response_iss_parameter_supported` and contains +//! its issuer. RMCP validates the response's `iss` against that metadata +//! issuer before exchanging the authorization code, rejecting missing or +//! mismatched issuers: +//! https://www.rfc-editor.org/rfc/rfc9700#section-4.4.2.1 +//! https://www.rfc-editor.org/rfc/rfc9207#section-2.4 +//! - `CallbackSpecific`: append an ID derived from the complete MCP server URL. +//! Distinct callback paths bind each response to its intended server. This is +//! the required fallback when issuer identification is not supported: +//! https://www.rfc-editor.org/rfc/rfc9700#section-4.4.2.2 + +use anyhow::Context; +use anyhow::Result; +use anyhow::anyhow; +use anyhow::bail; +use base64::Engine; +use base64::engine::general_purpose::URL_SAFE_NO_PAD; +use rmcp::transport::auth::AuthorizationMetadata; +use sha2::Digest; +use sha2::Sha256; +use url::Url; + +/// The OAuth mix-up defense associated with a newly registered callback. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum McpOAuthCallbackMode { + /// A server-specific redirect path identifies the authorization server. + CallbackSpecific, + /// A validated authorization-response issuer permits a shared redirect. + IssuerBound, +} + +pub(crate) fn callback_mode(metadata: &AuthorizationMetadata) -> Result { + let issuer_response_supported = metadata + .additional_fields + .get("authorization_response_iss_parameter_supported") + .and_then(serde_json::Value::as_bool) + .unwrap_or(false); + + if !issuer_response_supported { + return Ok(McpOAuthCallbackMode::CallbackSpecific); + } + if metadata.issuer.is_none() { + bail!("OAuth authorization server advertises issuer support without a metadata issuer"); + } + + Ok(McpOAuthCallbackMode::IssuerBound) +} + +/// Resolves the registered callback independently of runtime listener ports. +pub fn resolve_mcp_oauth_callback_url( + server_url: &str, + callback_url: Option<&str>, + callback_mode: McpOAuthCallbackMode, +) -> Result { + let callback_url = callback_url.unwrap_or("http://127.0.0.1/callback"); + + match callback_mode { + McpOAuthCallbackMode::IssuerBound => { + Url::parse(callback_url) + .with_context(|| format!("invalid redirect URI `{callback_url}`"))?; + Ok(callback_url.to_string()) + } + McpOAuthCallbackMode::CallbackSpecific => { + let callback_id = callback_id_from_server_url(server_url)?; + append_callback_id_to_redirect_uri(callback_url, &callback_id) + } + } +} + +pub(crate) fn callback_id_from_server_url(server_url: &str) -> Result { + // Native Codex callback IDs intentionally hash the complete MCP URL (minus its fragment) + // with SHA-256. Python connector callback IDs use SHAKE-256 over the origin and are distinct. + let mut parsed = + Url::parse(server_url).with_context(|| format!("invalid MCP server URL `{server_url}`"))?; + parsed + .host_str() + .ok_or_else(|| anyhow!("MCP server URL `{server_url}` must include a host"))?; + parsed.set_fragment(None); + + let digest = Sha256::digest(parsed.as_str().as_bytes()); + Ok(URL_SAFE_NO_PAD.encode(&digest[..9])) +} + +pub(crate) fn append_callback_id_to_redirect_uri( + redirect_uri: &str, + callback_id: &str, +) -> Result { + let mut parsed = Url::parse(redirect_uri) + .with_context(|| format!("invalid redirect URI `{redirect_uri}`"))?; + if parsed + .path_segments() + .and_then(|mut segments| segments.next_back()) + == Some(callback_id) + { + return Ok(parsed.to_string()); + } + let path = parsed.path(); + let new_path = if path.ends_with('/') { + format!("{path}{callback_id}") + } else { + format!("{path}/{callback_id}") + }; + parsed.set_path(&new_path); + Ok(parsed.to_string()) +} + +pub(crate) fn validate_callback_redirect( + redirect_uri: &str, + callback_id: &str, + callback_mode: McpOAuthCallbackMode, +) -> Result<()> { + let has_expected_callback_id = Url::parse(redirect_uri)? + .path_segments() + .and_then(|mut segments| segments.next_back()) + == Some(callback_id); + + if !has_expected_callback_id && callback_mode != McpOAuthCallbackMode::IssuerBound { + bail!( + "OAuth callback requires its expected callback ID or authorization response issuer support" + ); + } + + Ok(()) +} + +#[cfg(test)] +#[path = "oauth_callback_tests.rs"] +mod tests; diff --git a/codex-rs/rmcp-client/src/oauth_callback_tests.rs b/codex-rs/rmcp-client/src/oauth_callback_tests.rs new file mode 100644 index 0000000000..3d70c780a0 --- /dev/null +++ b/codex-rs/rmcp-client/src/oauth_callback_tests.rs @@ -0,0 +1,63 @@ +use pretty_assertions::assert_eq; + +use super::McpOAuthCallbackMode; +use super::callback_id_from_server_url; +use super::resolve_mcp_oauth_callback_url; +use super::validate_callback_redirect; + +#[test] +fn resolved_callbacks_follow_the_selected_mix_up_defense() { + let server_url = "https://mcp.example.com/mcp?tenant=one"; + let callback_id = callback_id_from_server_url(server_url).expect("resolve callback ID"); + let distinct_callback = format!("http://127.0.0.1/callback/{callback_id}"); + + for (callback, mode, expected) in [ + ( + None, + McpOAuthCallbackMode::CallbackSpecific, + distinct_callback.as_str(), + ), + ( + None, + McpOAuthCallbackMode::IssuerBound, + "http://127.0.0.1/callback", + ), + ( + Some("http://127.0.0.1:8080/oauth/callback"), + McpOAuthCallbackMode::IssuerBound, + "http://127.0.0.1:8080/oauth/callback", + ), + ] { + assert_eq!( + resolve_mcp_oauth_callback_url(server_url, callback, mode) + .expect("resolve registered callback"), + expected + ); + } +} + +#[test] +fn callback_redirect_requires_a_server_specific_id_or_issuer_support() { + for (redirect_uri, mode, expected_valid) in [ + ( + "http://127.0.0.1/callback/expected-id", + McpOAuthCallbackMode::CallbackSpecific, + true, + ), + ( + "http://127.0.0.1/callback", + McpOAuthCallbackMode::IssuerBound, + true, + ), + ( + "http://127.0.0.1/callback/wrong-id", + McpOAuthCallbackMode::CallbackSpecific, + false, + ), + ] { + assert_eq!( + validate_callback_redirect(redirect_uri, "expected-id", mode).is_ok(), + expected_valid + ); + } +} diff --git a/codex-rs/rmcp-client/src/oauth_client_registration.rs b/codex-rs/rmcp-client/src/oauth_client_registration.rs index 31a547836a..d9de5e21f7 100644 --- a/codex-rs/rmcp-client/src/oauth_client_registration.rs +++ b/codex-rs/rmcp-client/src/oauth_client_registration.rs @@ -10,6 +10,10 @@ use rmcp::transport::auth::OAuthState; use url::Url; use crate::oauth::validate_authorization_server_endpoints; +use crate::oauth_callback::McpOAuthCallbackMode; +use crate::oauth_callback::append_callback_id_to_redirect_uri; +use crate::oauth_callback::callback_mode; +use crate::oauth_callback::validate_callback_redirect; /// OAuth client-registration strategy for one interactive HTTP MCP login. #[derive(Debug, Clone, Copy, PartialEq, Eq, Default)] @@ -27,6 +31,7 @@ pub enum McpOAuthClientRegistration { pub(crate) struct PreparedOAuthLogin { pub(crate) oauth_state: OAuthState, pub(crate) authorization_server_issuer: Option, + pub(crate) redirect_uri: String, } pub(crate) async fn start_authorization( @@ -43,6 +48,7 @@ pub(crate) async fn start_authorization( let metadata = auth_manager.resolve_metadata().await?.metadata; validate_authorization_server_endpoints(&metadata)?; let authorization_server_issuer = metadata.issuer.clone(); + let callback_mode = callback_mode(&metadata)?; let cimd_advertised = metadata .additional_fields @@ -55,18 +61,30 @@ pub(crate) async fn start_authorization( .and_then(serde_json::Value::as_array) .is_some_and(|methods| methods.iter().any(|method| method.as_str() == Some("none"))); - let parsed_redirect_uri = Url::parse(redirect_uri)?; + let uses_shared_callback = callback_mode == McpOAuthCallbackMode::IssuerBound; + let redirect_uri = if uses_shared_callback { + redirect_uri.to_string() + } else { + append_callback_id_to_redirect_uri(redirect_uri, callback_id)? + }; + let parsed_redirect_uri = Url::parse(&redirect_uri)?; + let expected_callback_path = if uses_shared_callback { + "/callback".to_string() + } else { + format!("/callback/{callback_id}") + }; let native_redirect_supported = parsed_redirect_uri.scheme() == "http" && matches!( parsed_redirect_uri.host_str(), Some("127.0.0.1" | "localhost") ) && parsed_redirect_uri.port().is_some_and(|port| port > 0) - && parsed_redirect_uri.path() == format!("/callback/{callback_id}") + && parsed_redirect_uri.path() == expected_callback_path && parsed_redirect_uri.query().is_none() && parsed_redirect_uri.fragment().is_none() && parsed_redirect_uri.username().is_empty() && parsed_redirect_uri.password().is_none(); + validate_callback_redirect(&redirect_uri, callback_id, callback_mode)?; // MCP 2026-07-28 priority: pre-registered clients never reach this path; offer // advertised CIMD here and otherwise let rmcp fall back to DCR. // https://modelcontextprotocol.io/specification/2026-07-28/basic/authorization/client-registration @@ -82,7 +100,7 @@ pub(crate) async fn start_authorization( } if !native_redirect_supported { bail!( - "MCP OAuth CIMD requires an ephemeral loopback callback at `/callback/{callback_id}`" + "MCP OAuth CIMD requires an ephemeral loopback callback at `{expected_callback_path}`" ); } true @@ -91,16 +109,19 @@ pub(crate) async fn start_authorization( }; auth_manager.set_metadata(metadata); - let mut request = AuthorizationRequest::new(redirect_uri) + let mut request = AuthorizationRequest::new(redirect_uri.clone()) .with_scopes(scopes.iter().copied()) .with_client_name("Codex"); if offer_cimd { // CIMD is an active IETF Internet-Draft: this HTTPS client identifier resolves // to its self-referential JSON metadata document. // https://datatracker.ietf.org/doc/draft-ietf-oauth-client-id-metadata-document/ - request = request.with_client_metadata_url(format!( - "https://chatgpt.com/oauth/codex/{callback_id}/client.json" - )); + let client_metadata_url = if uses_shared_callback { + "https://chatgpt.com/oauth/codex/client.json".to_string() + } else { + format!("https://chatgpt.com/oauth/codex/{callback_id}/client.json") + }; + request = request.with_client_metadata_url(client_metadata_url); } let session = AuthorizationSession::new(auth_manager, request) .await @@ -109,6 +130,7 @@ pub(crate) async fn start_authorization( Ok(PreparedOAuthLogin { oauth_state: OAuthState::Session(session), authorization_server_issuer, + redirect_uri, }) } 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 a776218fd8..8835449f5d 100644 --- a/codex-rs/rmcp-client/src/oauth_client_registration_tests.rs +++ b/codex-rs/rmcp-client/src/oauth_client_registration_tests.rs @@ -128,20 +128,47 @@ async fn authorization( } #[tokio::test] -async fn automatic_cimd_uses_callback_specific_identity() -> Result<()> { - for host in ["127.0.0.1", "localhost"] { - let server = oauth_server(json!({})).await; - let redirect = format!("http://{host}:43123/callback/{CALLBACK_ID}"); +async fn automatic_cimd_uses_stable_or_callback_specific_identity() -> Result<()> { + for (host, supports_issuer, expected_client_id, expected_redirect) in [ + ( + "127.0.0.1", + false, + "https://chatgpt.com/oauth/codex/abc123ABC_-x/client.json", + "http://127.0.0.1:43123/callback/abc123ABC_-x", + ), + ( + "localhost", + false, + "https://chatgpt.com/oauth/codex/abc123ABC_-x/client.json", + "http://localhost:43123/callback/abc123ABC_-x", + ), + ( + "127.0.0.1", + true, + "https://chatgpt.com/oauth/codex/client.json", + "http://127.0.0.1:43123/callback", + ), + ] { + let server = oauth_server(json!({ + "authorization_response_iss_parameter_supported": supports_issuer, + })) + .await; + let redirect = format!("http://{host}:43123/callback"); let (mut state, query) = authorization(&server, &redirect, McpOAuthClientRegistration::Auto).await?; - let expected_id = format!("https://chatgpt.com/oauth/codex/{CALLBACK_ID}/client.json"); - assert_eq!(query["client_id"], expected_id); - assert_eq!(query["redirect_uri"], redirect); + assert_eq!(query["client_id"], expected_client_id); + assert_eq!(query["redirect_uri"], expected_redirect); assert_eq!(query["code_challenge_method"], "S256"); assert_eq!(query["scope"], "read offline_access"); state - .handle_callback_with_issuer("valid-authorization-code", &query["state"], None) + .handle_callback_with_issuer( + "valid-authorization-code", + &query["state"], + supports_issuer + .then(|| format!("{}/mcp", server.uri())) + .as_deref(), + ) .await?; let token_requests = requests_to(&server, "/token").await; assert_eq!(token_requests.len(), 1); @@ -149,8 +176,8 @@ async fn automatic_cimd_uses_callback_specific_identity() -> Result<()> { let body: HashMap<_, _> = url::form_urlencoded::parse(&request.body) .into_owned() .collect(); - assert_eq!(body["client_id"], expected_id); - assert_eq!(body["redirect_uri"], redirect); + assert_eq!(body["client_id"], expected_client_id); + assert_eq!(body["redirect_uri"], expected_redirect); assert_eq!(body["grant_type"], "authorization_code"); assert!(body.contains_key("code_verifier")); assert!(!body.contains_key("client_secret")); @@ -170,6 +197,7 @@ async fn automatic_cimd_uses_callback_specific_identity() -> Result<()> { #[tokio::test] async fn registration_selection_preserves_dcr_capabilities_and_exact_redirects() -> Result<()> { let native = "http://localhost:43123/callback/abc123ABC_-x"; + let shared_native = "http://localhost:43123/callback"; let custom = "https://callbacks.example.com/oauth/callback/abc123ABC_-x"; for (metadata, redirect, registration, expected_redirect) in [ ( @@ -193,9 +221,9 @@ async fn registration_selection_preserves_dcr_capabilities_and_exact_redirects() (json!({}), custom, McpOAuthClientRegistration::Auto, custom), ( json!({"authorization_response_iss_parameter_supported": true}), - native, + shared_native, McpOAuthClientRegistration::Dcr, - native, + shared_native, ), ] { let server = oauth_server(metadata).await; @@ -485,6 +513,14 @@ async fn resource_headers_follow_same_origin_registration_redirect_and_sdk_auth_ async fn invalid_cimd_metadata_and_redirects_fail_without_dynamic_registration() { let valid = "http://127.0.0.1:43123/callback/abc123ABC_-x"; for (metadata, redirect, expected_error) in [ + ( + json!({ + "authorization_response_iss_parameter_supported": true, + "issuer": null, + }), + valid, + "issuer-bound callbacks require an authorization server issuer", + ), ( json!({"token_endpoint_auth_methods_supported": ["private_key_jwt"]}), valid, diff --git a/codex-rs/rmcp-client/src/perform_oauth_login.rs b/codex-rs/rmcp-client/src/perform_oauth_login.rs index 873490ed6f..b2ac1bc7c4 100644 --- a/codex-rs/rmcp-client/src/perform_oauth_login.rs +++ b/codex-rs/rmcp-client/src/perform_oauth_login.rs @@ -7,16 +7,13 @@ use anyhow::Context; use anyhow::Result; use anyhow::anyhow; use anyhow::bail; -use base64::Engine; -use base64::engine::general_purpose::URL_SAFE_NO_PAD; use codex_exec_server::HttpClient; use rmcp::transport::AuthorizationManager; use rmcp::transport::AuthorizationSession; +use rmcp::transport::auth::AuthorizationMetadata; use rmcp::transport::auth::OAuthClientConfig; use rmcp::transport::auth::OAuthHttpClient; use rmcp::transport::auth::OAuthState; -use sha2::Digest; -use sha2::Sha256; use tiny_http::Response; use tiny_http::Server; use tokio::sync::oneshot; @@ -29,6 +26,11 @@ 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_callback::McpOAuthCallbackMode; +use crate::oauth_callback::append_callback_id_to_redirect_uri; +use crate::oauth_callback::callback_id_from_server_url; +use crate::oauth_callback::callback_mode; +use crate::oauth_callback::validate_callback_redirect; use crate::oauth_client_registration::McpOAuthClientRegistration; use crate::oauth_client_registration::PreparedOAuthLogin; use crate::oauth_client_registration::start_authorization as start_client_registration; @@ -99,6 +101,7 @@ pub async fn perform_oauth_login( oauth_resource: Option<&str>, callback_port: Option, callback_url: Option<&str>, + global_callback_url: Option<&str>, http_client: Arc, ) -> Result<()> { perform_oauth_login_with_browser_output( @@ -114,6 +117,7 @@ pub async fn perform_oauth_login( oauth_resource, callback_port, callback_url, + global_callback_url, http_client, /*emit_browser_url*/ true, StreamableHttpRedirectMode::Legacy, @@ -135,6 +139,7 @@ pub async fn perform_oauth_login_silent( oauth_resource: Option<&str>, callback_port: Option, callback_url: Option<&str>, + global_callback_url: Option<&str>, http_client: Arc, redirect_mode: StreamableHttpRedirectMode, ) -> Result<()> { @@ -151,6 +156,7 @@ pub async fn perform_oauth_login_silent( oauth_resource, callback_port, callback_url, + global_callback_url, http_client, /*emit_browser_url*/ false, redirect_mode, @@ -172,6 +178,7 @@ async fn perform_oauth_login_with_browser_output( oauth_resource: Option<&str>, callback_port: Option, callback_url: Option<&str>, + global_callback_url: Option<&str>, http_client: Arc, emit_browser_url: bool, redirect_mode: StreamableHttpRedirectMode, @@ -195,6 +202,7 @@ async fn perform_oauth_login_with_browser_output( /*launch_browser*/ true, callback_port, callback_url, + global_callback_url, /*timeout_secs*/ None, ) .await? @@ -217,6 +225,7 @@ pub async fn perform_oauth_login_return_url( timeout_secs: Option, callback_port: Option, callback_url: Option<&str>, + global_callback_url: Option<&str>, http_client: Arc, redirect_mode: StreamableHttpRedirectMode, ) -> Result { @@ -239,6 +248,7 @@ pub async fn perform_oauth_login_return_url( /*launch_browser*/ false, callback_port, callback_url, + global_callback_url, timeout_secs, ) .await?; @@ -443,38 +453,32 @@ fn resolve_redirect_uri(server: &Server, callback_url: Option<&str>) -> Result Result { - // Native Codex callback IDs intentionally hash the complete MCP URL (minus its fragment) - // with SHA-256. Python connector callback IDs use SHAKE-256 over the origin and are distinct. - let mut parsed = - Url::parse(server_url).with_context(|| format!("invalid MCP server URL `{server_url}`"))?; - parsed - .host_str() - .ok_or_else(|| anyhow!("MCP server URL `{server_url}` must include a host"))?; - parsed.set_fragment(None); - - let digest = Sha256::digest(parsed.as_str().as_bytes()); - Ok(URL_SAFE_NO_PAD.encode(&digest[..9])) -} - -fn append_callback_id_to_redirect_uri(redirect_uri: &str, callback_id: &str) -> Result { - let mut parsed = Url::parse(redirect_uri) - .with_context(|| format!("invalid redirect URI `{redirect_uri}`"))?; - let path = parsed.path(); - let new_path = if path.ends_with('/') { - format!("{path}{callback_id}") - } else { - format!("{path}/{callback_id}") - }; - parsed.set_path(&new_path); - Ok(parsed.to_string()) -} - fn callback_path_from_redirect_uri(redirect_uri: &str) -> Result { let parsed = Url::parse(redirect_uri) .with_context(|| format!("invalid redirect URI `{redirect_uri}`"))?; @@ -511,29 +515,24 @@ impl OauthLoginFlow { launch_browser: bool, callback_port: Option, callback_url: Option<&str>, + global_callback_url: Option<&str>, timeout_secs: Option, ) -> Result { const DEFAULT_OAUTH_TIMEOUT_SECS: i64 = 300; - let bind_host = callback_bind_host(callback_url); let callback_port = resolve_callback_port(callback_port)?; - let bind_addr = match callback_port { - Some(port) => format!("{bind_host}:{port}"), - None => format!("{bind_host}:0"), - }; - - let server = Arc::new(Server::http(&bind_addr).map_err(|err| anyhow!(err))?); - let guard = CallbackServerGuard { - server: Arc::clone(&server), - }; - - let redirect_uri = resolve_redirect_uri(&server, callback_url)?; let callback_id = callback_id_from_server_url(server_url)?; - let redirect_uri = append_callback_id_to_redirect_uri(&redirect_uri, &callback_id)?; - let callback_path = callback_path_from_redirect_uri(&redirect_uri)?; - - let (tx, rx) = oneshot::channel(); - spawn_callback_server(server, tx, callback_path); + let oauth_client_id = oauth_client_id.filter(|client_id| !client_id.trim().is_empty()); + let configured_callback = if oauth_client_id.is_some() { + callback_url + .map(|callback_url| { + Url::parse(callback_url) + .with_context(|| format!("invalid MCP OAuth callback URL `{callback_url}`")) + }) + .transpose()? + } else { + None + }; let OAuthHttpContext { http_headers, @@ -548,26 +547,75 @@ impl OauthLoginFlow { .as_ref() .is_some_and(|headers| !headers.is_empty()); let default_headers = build_default_headers(http_headers, env_http_headers)?; - let oauth_http_client = Arc::new(OAuthHttpClientAdapter::new_with_redirect_mode( - http_client, - default_headers, - server_url, - has_configured_headers, - redirect_mode, - )?); + let oauth_http_client: Arc = + Arc::new(OAuthHttpClientAdapter::new_with_redirect_mode( + http_client, + default_headers, + server_url, + has_configured_headers, + redirect_mode, + )?); + let registered_authorization = if oauth_client_id.is_some() { + Some(resolve_authorization_manager(server_url, Arc::clone(&oauth_http_client)).await?) + } else { + None + }; + let registered_callback_mode = registered_authorization + .as_ref() + .map(|(_, metadata)| callback_mode(metadata)) + .transpose()?; + let use_legacy_fallback = registered_callback_mode + == Some(McpOAuthCallbackMode::CallbackSpecific) + && configured_callback.as_ref().is_some_and(|callback_url| { + callback_url + .path_segments() + .and_then(|mut segments| segments.next_back()) + != Some(callback_id.as_str()) + }); + let callback_url = if use_legacy_fallback { + // Any preregistered client's callback can lack its required ID when + // the authorization server does not support issuer binding. This + // especially affects plugins, whose callbacks are configured before + // metadata discovery. Preserve compatibility and avoid making every + // login fail by using the global/default callback instead; its + // required server-specific callback ID is appended below. + global_callback_url + } else { + callback_url + }; + + let bind_host = callback_bind_host(callback_url); + // Port zero asks the OS for a free ephemeral port; the resolved + // redirect receives that port after the listener has been bound. + let bind_addr = match callback_port { + Some(port) => format!("{bind_host}:{port}"), + None => format!("{bind_host}:0"), + }; + let server = Arc::new(Server::http(&bind_addr).map_err(|err| anyhow!(err))?); + let guard = CallbackServerGuard { + server: Arc::clone(&server), + }; + let redirect_uri = resolve_redirect_uri(&server, callback_url)?; let scope_refs: Vec<&str> = scopes.iter().map(String::as_str).collect(); let PreparedOAuthLogin { oauth_state, authorization_server_issuer, - } = if let Some(oauth_client_id) = - oauth_client_id.filter(|client_id| !client_id.trim().is_empty()) + redirect_uri, + } = if let Some((oauth_client_id, (auth_manager, metadata))) = + oauth_client_id.zip(registered_authorization) { + let redirect_uri = if callback_url.is_some() && !use_legacy_fallback { + redirect_uri + } else { + append_callback_id_to_redirect_uri(&redirect_uri, &callback_id)? + }; start_authorization( - server_url, - oauth_http_client, + auth_manager, + metadata, &scope_refs, &redirect_uri, + &callback_id, oauth_client_id, ) .await? @@ -582,6 +630,9 @@ impl OauthLoginFlow { ) .await? }; + let callback_path = callback_path_from_redirect_uri(&redirect_uri)?; + let (tx, rx) = oneshot::channel(); + spawn_callback_server(server, tx, callback_path); let auth_url = append_query_param( &oauth_state.get_authorization_url().await?, "resource", @@ -700,19 +751,28 @@ impl OauthLoginFlow { } } -async fn start_authorization( +async fn resolve_authorization_manager( server_url: &str, http_client: Arc, - scopes: &[&str], - redirect_uri: &str, - oauth_client_id: &str, -) -> Result { +) -> Result<(AuthorizationManager, AuthorizationMetadata)> { let mut auth_manager = 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)?; + Ok((auth_manager, metadata)) +} + +async fn start_authorization( + mut auth_manager: AuthorizationManager, + metadata: AuthorizationMetadata, + scopes: &[&str], + redirect_uri: &str, + callback_id: &str, + oauth_client_id: &str, +) -> Result { let authorization_server_issuer = metadata.issuer.clone(); + validate_callback_redirect(redirect_uri, callback_id, callback_mode(&metadata)?)?; auth_manager.set_metadata(metadata); auth_manager.configure_client( OAuthClientConfig::new(oauth_client_id, redirect_uri) @@ -727,6 +787,7 @@ async fn start_authorization( redirect_uri, )), authorization_server_issuer, + redirect_uri: redirect_uri.to_string(), }) } @@ -792,6 +853,7 @@ mod tests { use super::parse_oauth_callback; use super::perform_oauth_login; use super::perform_oauth_login_silent; + use super::resolve_authorization_manager; use super::start_authorization; use crate::oauth::stored_oauth_credentials; use crate::oauth::test_support::TempCodexHome; @@ -942,6 +1004,7 @@ mod tests { /*launch_browser*/ false, /*callback_port*/ None, /*callback_url*/ None, + /*global_callback_url*/ None, Some(/*timeout_secs*/ 5), ) .await?; @@ -977,7 +1040,7 @@ mod tests { for (scopes, expected_scope) in [(&[][..], None), (&["read"][..], Some("read"))] { let (base_url, registration_requests) = spawn_oauth_metadata_server().await; let redirect_uri = "http://127.0.0.1:43123/callback/configured-client"; - let prepared = start_authorization( + let (auth_manager, metadata) = resolve_authorization_manager( &format!("{base_url}/mcp"), Arc::new(OAuthHttpClientAdapter::new( Arc::new(RouteAwareHttpClient::new(HttpClientFactory::new( @@ -986,8 +1049,15 @@ mod tests { HeaderMap::new(), &format!("{base_url}/mcp"), )), + ) + .await + .expect("resolve pre-registered OAuth metadata"); + let prepared = start_authorization( + auth_manager, + metadata, scopes, redirect_uri, + "configured-client", "eci-prd-pub-codex-123", ) .await @@ -1072,7 +1142,12 @@ mod tests { .await .expect("serve authorization metadata fixture"); }); - let prepared = start_authorization( + let redirect_uri = if supports_issuer { + "http://127.0.0.1/callback" + } else { + "http://127.0.0.1/callback/test-callback" + }; + let (auth_manager, metadata) = resolve_authorization_manager( &format!("{issuer}/mcp"), Arc::new(OAuthHttpClientAdapter::new( Arc::new(RouteAwareHttpClient::new(HttpClientFactory::new( @@ -1081,8 +1156,15 @@ mod tests { HeaderMap::new(), &format!("{issuer}/mcp"), )), + ) + .await + .expect("resolve issuer-aware authorization metadata"); + let prepared = start_authorization( + auth_manager, + metadata, &[], - "http://127.0.0.1/callback", + redirect_uri, + "test-callback", "test-client", ) .await @@ -1145,6 +1227,7 @@ mod tests { /*oauth_resource*/ None, /*callback_port*/ None, /*callback_url*/ None, + /*global_callback_url*/ None, http_client.clone(), ) .await @@ -1169,6 +1252,7 @@ mod tests { /*oauth_resource*/ None, /*callback_port*/ None, /*callback_url*/ None, + /*global_callback_url*/ None, http_client.clone(), StreamableHttpRedirectMode::Legacy, ) @@ -1276,6 +1360,11 @@ mod tests { .expect("redirect URI should parse"); assert_eq!(redirect_uri, "http://127.0.0.1:1234/callback/abc123"); + assert_eq!( + append_callback_id_to_redirect_uri(&redirect_uri, "abc123") + .expect("resolved redirect URI should parse"), + redirect_uri + ); } #[test] @@ -1292,6 +1381,38 @@ mod tests { ); } + #[test] + fn portless_loopback_callbacks_use_the_active_listener_port() { + let server = tiny_http::Server::http("127.0.0.1:0").expect("start callback listener"); + let listener_port = server + .server_addr() + .to_ip() + .expect("resolve callback listener address") + .port(); + + for path in ["/callback", "/callback/callback-id", "/custom/callback"] { + let callback = format!("http://127.0.0.1{path}"); + assert_eq!( + super::resolve_redirect_uri(&server, Some(&callback)) + .expect("insert active listener port"), + format!("http://127.0.0.1:{listener_port}{path}") + ); + } + + for callback in [ + "http://localhost/callback", + "http://127.0.0.1:3080/callback", + "https://127.0.0.1/callback", + "https://devbox.example.com/callback", + ] { + assert_eq!( + super::resolve_redirect_uri(&server, Some(callback)) + .expect("preserve configured callback origin"), + callback + ); + } + } + #[test] fn append_query_param_adds_resource_to_absolute_url() { let url = append_query_param( 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 31c66bad3a..a26dc7fbee 100644 --- a/codex-rs/rmcp-client/tests/mcp_2026_oauth_discovery.rs +++ b/codex-rs/rmcp-client/tests/mcp_2026_oauth_discovery.rs @@ -7,6 +7,7 @@ use codex_config::types::AuthKeyringBackendKind; use codex_config::types::OAuthCredentialsStoreMode; use codex_exec_server::Environment; use codex_exec_server::HttpClient; +use codex_rmcp_client::McpOAuthCallbackMode; use codex_rmcp_client::McpOAuthClientRegistration; use codex_rmcp_client::OAuthDiscoveryTimeout; use codex_rmcp_client::StreamableHttpOAuthDiscovery; @@ -230,6 +231,7 @@ async fn assert_legacy_oauth_without_starting_an_mcp_session( discovery?, Some(StreamableHttpOAuthDiscovery { scopes_supported: Some(vec!["mcp:read".to_string()]), + callback_mode: McpOAuthCallbackMode::CallbackSpecific, }), ); } @@ -655,6 +657,7 @@ async fn interactive_oauth_rejects_untrusted_authorization_metadata() -> anyhow: Some(/*timeout_secs*/ 5), /*callback_port*/ None, /*callback_url*/ None, + /*global_callback_url*/ None, local_http_client(), StreamableHttpRedirectMode::Legacy, )