From 164b3bfeabdbc8e33c7320437e7cd875f93a534e Mon Sep 17 00:00:00 2001 From: jif Date: Fri, 31 Jul 2026 11:18:09 +0000 Subject: [PATCH] Isolate MCP OAuth credentials by environment (#36310) ## Why MCP servers running in an executor must not reuse host-owned OAuth credentials, even when they have the same configured server name and URL. ## What changed - Derive environment-scoped credential names for executor-owned MCP servers and use them consistently for login, logout, authentication, and connection setup. - Mark executor-owned entries in the file credential store and fail closed when loading or saving an entry could cross the host/executor boundary. - Preserve compatibility with existing local OAuth credentials, including local server names that overlap the new reserved prefixes. ## Testing - Extend the executor MCP integration test to verify that executor requests use the executor token, never send the host token, and persist both credentials separately. GitOrigin-RevId: 4fc92d0533b0fe2e0df34f6d47b81e3d20d07807 --- .../src/request_processors/mcp_processor.rs | 3 +- .../src/request_processors/plugins.rs | 5 +- .../app-server/tests/suite/v2/executor_mcp.rs | 91 ++++++++++++++++++- codex-rs/cli/src/mcp_cmd.rs | 6 +- codex-rs/codex-mcp/src/connection_manager.rs | 4 +- codex-rs/codex-mcp/src/mcp/auth.rs | 3 +- codex-rs/codex-mcp/src/rmcp_client.rs | 3 +- codex-rs/codex-mcp/src/server.rs | 14 ++- codex-rs/config/src/mcp_types.rs | 18 ++++ codex-rs/core/src/mcp_skill_dependencies.rs | 5 +- codex-rs/core/tests/suite/rmcp_client.rs | 41 +++++---- codex-rs/rmcp-client/src/oauth.rs | 40 +++++++- 12 files changed, 194 insertions(+), 39 deletions(-) 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 0751f26e16..0ea24d4226 100644 --- a/codex-rs/app-server/src/request_processors/mcp_processor.rs +++ b/codex-rs/app-server/src/request_processors/mcp_processor.rs @@ -183,9 +183,10 @@ impl McpRequestProcessor { }; let resolved_scopes = resolve_oauth_scopes(scopes, server.scopes.clone(), discovered_scopes); + let oauth_credential_name = server.oauth_credential_name(&name); let handle = perform_oauth_login_return_url( - &name, + oauth_credential_name.as_ref(), &url, mcp_config.mcp_oauth_credentials_store_mode, mcp_config.auth_keyring_backend_kind, diff --git a/codex-rs/app-server/src/request_processors/plugins.rs b/codex-rs/app-server/src/request_processors/plugins.rs index 7324e2aa2f..347675ee04 100644 --- a/codex-rs/app-server/src/request_processors/plugins.rs +++ b/codex-rs/app-server/src/request_processors/plugins.rs @@ -1923,13 +1923,14 @@ impl PluginRequestProcessor { let callback_url = config.mcp_oauth_callback_url.clone(); 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); tokio::spawn(async move { let oauth_client_id = server.oauth_client_id(); let first_attempt = perform_oauth_login_silent( - &name, + &oauth_credential_name, &oauth_config.url, store_mode, keyring_backend_kind, @@ -1947,7 +1948,7 @@ impl PluginRequestProcessor { let final_result = match first_attempt { Err(err) if should_retry_without_scopes(&resolved_scopes, &err) => { perform_oauth_login_silent( - &name, + &oauth_credential_name, &oauth_config.url, store_mode, keyring_backend_kind, 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 f5d931471d..b02a6d2ccb 100644 --- a/codex-rs/app-server/tests/suite/v2/executor_mcp.rs +++ b/codex-rs/app-server/tests/suite/v2/executor_mcp.rs @@ -53,6 +53,8 @@ const HTTP_MCP_SERVER_NAME: &str = "executor_http"; const MCP_SERVER_NAME: &str = "executor_demo"; const OAUTH_MCP_SERVER_NAME: &str = "executor_oauth"; const EXECUTOR_OAUTH_MCP_URL: &str = "http://oauth-only.invalid/oauth-mcp"; +const HOST_OAUTH_ACCESS_TOKEN: &str = "host-access-token"; +const EXECUTOR_OAUTH_ACCESS_TOKEN: &str = "executor-access-token"; const EXECUTOR_ENV_NAME: &str = "MCP_EXECUTOR_MARKER"; const EXECUTOR_ENV_VALUE: &str = "executor-only"; const EXECUTOR_ID: &str = "executor-1"; @@ -76,6 +78,24 @@ async fn selected_executor_plugin_exposes_its_mcps_only_to_that_thread() -> Resu Arc::new(LocalSessionManager::default()), http_server_config, ); + let (oauth_authorization_tx, mut oauth_authorization_rx) = mpsc::unbounded_channel(); + let oauth_mcp_router = Router::new() + .nest_service("/oauth-mcp", oauth_mcp_service) + .layer(axum::middleware::from_fn( + move |request: axum::extract::Request, next: axum::middleware::Next| { + let oauth_authorization_tx = oauth_authorization_tx.clone(); + async move { + if let Some(authorization) = request + .headers() + .get(axum::http::header::AUTHORIZATION) + .and_then(|value| value.to_str().ok()) + { + let _ = oauth_authorization_tx.send(authorization.to_string()); + } + next.run(request).await + } + }, + )); let (token_request_tx, mut token_request_rx) = mpsc::unbounded_channel(); let oauth_metadata = json!({ "authorization_endpoint": "https://oauth-only.invalid/authorize", @@ -99,7 +119,7 @@ async fn selected_executor_plugin_exposes_its_mcps_only_to_that_thread() -> Resu async move { let _ = token_request_tx.send(String::from_utf8_lossy(&body).into_owned()); Json(json!({ - "access_token": "executor-access-token", + "access_token": EXECUTOR_OAUTH_ACCESS_TOKEN, "token_type": "Bearer", "expires_in": 3600, "refresh_token": "executor-refresh-token", @@ -108,15 +128,35 @@ async fn selected_executor_plugin_exposes_its_mcps_only_to_that_thread() -> Resu }), ) .nest_service("/mcp", http_mcp_service) - .nest_service("/oauth-mcp", oauth_mcp_service); + .merge(oauth_mcp_router); let http_server_handle = tokio::spawn(async move { let _ = axum::serve(http_listener, http_router).await; }); let codex_home = TempDir::new()?; MockResponsesConfig::new(&responses_server.uri()) - .with_root_config("compact_prompt = \"compact\"\nmodel_auto_compact_token_limit = 1024") + .with_root_config( + "compact_prompt = \"compact\"\nmodel_auto_compact_token_limit = 1024\nmcp_oauth_credentials_store = \"file\"", + ) .with_provider_config("supports_websockets = false") .write(codex_home.path())?; + let executor_config: codex_config::types::McpServerConfig = serde_json::from_value(json!({ + "url": EXECUTOR_OAUTH_MCP_URL, + "environment_id": EXECUTOR_ID, + }))?; + let host_oauth_credential = json!({ + "server_name": executor_config.oauth_credential_name(OAUTH_MCP_SERVER_NAME), + "server_url": EXECUTOR_OAUTH_MCP_URL, + "client_id": "host-oauth-client", + "access_token": HOST_OAUTH_ACCESS_TOKEN, + "expires_at": null, + "refresh_token": null, + "scopes": [], + }); + let oauth_credentials_path = codex_home.path().join(".credentials.json"); + std::fs::write( + &oauth_credentials_path, + serde_json::to_vec(&json!({"host": host_oauth_credential.clone()}))?, + )?; let codex_bin = toml::Value::String( codex_utils_cargo_bin::cargo_bin("codex")? .to_string_lossy() @@ -344,6 +384,51 @@ startup_timeout_sec = 10 Some(json!({"echo": "ECHOING: hello over executor HTTP"})) ); + let request_id = app_server + .send_mcp_server_tool_call_request(McpServerToolCallParams { + thread_id: selected_thread.clone(), + server: OAUTH_MCP_SERVER_NAME.to_string(), + tool: "echo".to_string(), + arguments: Some(json!({"message": "hello over executor OAuth"})), + meta: None, + }) + .await?; + let response: McpServerToolCallResponse = + timeout(DEFAULT_READ_TIMEOUT, app_server.read_response(request_id)).await??; + assert_eq!( + response.structured_content, + Some(json!({"echo": "ECHOING: hello over executor OAuth"})) + ); + + let authorization_headers = + std::iter::from_fn(|| oauth_authorization_rx.try_recv().ok()).collect::>(); + assert!( + !authorization_headers + .iter() + .any(|header| header == &format!("Bearer {HOST_OAUTH_ACCESS_TOKEN}")), + "host-owned OAuth credentials must never reach the executor: {authorization_headers:?}" + ); + assert!( + authorization_headers + .iter() + .any(|header| header == &format!("Bearer {EXECUTOR_OAUTH_ACCESS_TOKEN}")), + "executor-owned OAuth credentials must authenticate executor requests: {authorization_headers:?}" + ); + let oauth_credentials: serde_json::Value = + serde_json::from_slice(&std::fs::read(&oauth_credentials_path)?)?; + assert_eq!(oauth_credentials.get("host"), Some(&host_oauth_credential)); + assert!( + oauth_credentials + .as_object() + .expect("OAuth credentials should remain a JSON object") + .values() + .any(|credential| { + credential.get("server_name") != Some(&json!(OAUTH_MCP_SERVER_NAME)) + && credential.get("access_token") == Some(&json!(EXECUTOR_OAUTH_ACCESS_TOKEN)) + }), + "executor login must persist credentials separately from the host: {oauth_credentials}" + ); + let request_id = app_server .send_mcp_server_tool_call_request(McpServerToolCallParams { thread_id: selected_thread.clone(), diff --git a/codex-rs/cli/src/mcp_cmd.rs b/codex-rs/cli/src/mcp_cmd.rs index 242b34ccfa..b157b1b82d 100644 --- a/codex-rs/cli/src/mcp_cmd.rs +++ b/codex-rs/cli/src/mcp_cmd.rs @@ -517,9 +517,10 @@ 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); perform_oauth_login_retry_without_scopes( - &name, + credential_name.as_ref(), &url, config.mcp_oauth_credentials_store_mode, config.auth_keyring_backend_kind(), @@ -551,9 +552,10 @@ async fn run_logout(config: &Config, logout_args: LogoutArgs) -> Result<()> { McpServerTransportConfig::StreamableHttp { url, .. } => url.clone(), _ => bail!("OAuth logout is only supported for streamable_http transports."), }; + let credential_name = server.oauth_credential_name(&name); match delete_oauth_tokens( - &name, + credential_name.as_ref(), &url, config.mcp_oauth_credentials_store_mode, config.auth_keyring_backend_kind(), diff --git a/codex-rs/codex-mcp/src/connection_manager.rs b/codex-rs/codex-mcp/src/connection_manager.rs index 34a0f35613..90288db8ff 100644 --- a/codex-rs/codex-mcp/src/connection_manager.rs +++ b/codex-rs/codex-mcp/src/connection_manager.rs @@ -419,7 +419,9 @@ impl McpConnectionSet { env_http_headers, } => { match determine_streamable_http_auth_status_from_credentials( - &server_name, + configured_config + .oauth_credential_name(&server_name) + .as_ref(), url, bearer_token_env_var.as_deref(), http_headers.clone(), diff --git a/codex-rs/codex-mcp/src/mcp/auth.rs b/codex-rs/codex-mcp/src/mcp/auth.rs index 091d91d2ea..9e093e8d88 100644 --- a/codex-rs/codex-mcp/src/mcp/auth.rs +++ b/codex-rs/codex-mcp/src/mcp/auth.rs @@ -245,8 +245,9 @@ async fn compute_auth_status( } else { OAuthDiscoveryTimeout::Requested }; + let oauth_credential_name = config.oauth_credential_name(server_name); determine_streamable_http_auth_status( - server_name, + oauth_credential_name.as_ref(), url, bearer_token_env_var.as_deref(), http_headers.clone(), diff --git a/codex-rs/codex-mcp/src/rmcp_client.rs b/codex-rs/codex-mcp/src/rmcp_client.rs index 74519672f9..eac72c0e72 100644 --- a/codex-rs/codex-mcp/src/rmcp_client.rs +++ b/codex-rs/codex-mcp/src/rmcp_client.rs @@ -1008,6 +1008,7 @@ async fn make_rmcp_client( let resolved_environment = resolved_environment.map_err(|err| StartupOutcomeError::from(anyhow!(err)))?; let is_local_environment = config.is_local_environment(); + let oauth_credential_name = config.oauth_credential_name(server_name); let McpServerConfig { transport, .. } = config; match transport { @@ -1073,7 +1074,7 @@ async fn make_rmcp_client( Err(error) => return Err(error.into()), }; RmcpClient::new_streamable_http_client_with_protocol_mode( - server_name, + oauth_credential_name.as_ref(), &url, resolved_bearer_token, http_headers, diff --git a/codex-rs/codex-mcp/src/server.rs b/codex-rs/codex-mcp/src/server.rs index f4e05166f6..c7abf92e20 100644 --- a/codex-rs/codex-mcp/src/server.rs +++ b/codex-rs/codex-mcp/src/server.rs @@ -128,12 +128,16 @@ impl McpServerConnectionIdentity { None }; let oauth_credentials = stored_oauth_url.map_or(Ok(None), |url| { - stored_oauth_credentials(server_name, url, store_mode, keyring_backend_kind).map_err( - |error| { - warn!(server_name, %error, "failed to read stored MCP OAuth credentials"); - error.to_string() - }, + stored_oauth_credentials( + config.oauth_credential_name(server_name).as_ref(), + url, + store_mode, + keyring_backend_kind, ) + .map_err(|error| { + warn!(server_name, %error, "failed to read stored MCP OAuth credentials"); + error.to_string() + }) }); let local_stdio_fallback_cwd = (config.is_local_environment() && matches!( diff --git a/codex-rs/config/src/mcp_types.rs b/codex-rs/config/src/mcp_types.rs index 7043e0cb69..ed5c34643b 100644 --- a/codex-rs/config/src/mcp_types.rs +++ b/codex-rs/config/src/mcp_types.rs @@ -1,9 +1,12 @@ //! MCP server configuration types. +use std::borrow::Cow; use std::collections::HashMap; use std::fmt; use std::time::Duration; +use base64::Engine; +use base64::engine::general_purpose::URL_SAFE_NO_PAD; use codex_utils_path_uri::LegacyAppPathString; use schemars::JsonSchema; use serde::Deserialize; @@ -227,6 +230,21 @@ impl McpServerConfig { self.environment_id == DEFAULT_MCP_SERVER_ENVIRONMENT_ID } + /// Keeps local OAuth credentials compatible while isolating executor-owned servers. + pub fn oauth_credential_name<'a>(&self, server_name: &'a str) -> Cow<'a, str> { + if self.is_local_environment() { + if server_name.starts_with("executor:") || server_name.starts_with("local:") { + Cow::Owned(format!("local:{server_name}")) + } else { + Cow::Borrowed(server_name) + } + } else { + let environment = URL_SAFE_NO_PAD.encode(self.environment_id.as_bytes()); + let server = URL_SAFE_NO_PAD.encode(server_name.as_bytes()); + Cow::Owned(format!("executor:{environment}:{server}")) + } + } + pub fn oauth_client_id(&self) -> Option<&str> { self.oauth .as_ref() diff --git a/codex-rs/core/src/mcp_skill_dependencies.rs b/codex-rs/core/src/mcp_skill_dependencies.rs index dc6d41c351..9510edce8e 100644 --- a/codex-rs/core/src/mcp_skill_dependencies.rs +++ b/codex-rs/core/src/mcp_skill_dependencies.rs @@ -169,8 +169,9 @@ pub(crate) async fn maybe_install_mcp_dependencies( oauth_config.discovered_scopes.clone(), ); let oauth_client_id = server_config.oauth_client_id(); + let oauth_credential_name = server_config.oauth_credential_name(&name); let first_attempt = perform_oauth_login( - &name, + oauth_credential_name.as_ref(), &oauth_config.url, config.mcp_oauth_credentials_store_mode, config.auth_keyring_backend_kind(), @@ -188,7 +189,7 @@ pub(crate) async fn maybe_install_mcp_dependencies( if let Err(err) = first_attempt { if should_retry_without_scopes(&resolved_scopes, &err) { if let Err(err) = perform_oauth_login( - &name, + oauth_credential_name.as_ref(), &oauth_config.url, config.mcp_oauth_credentials_store_mode, config.auth_keyring_backend_kind(), diff --git a/codex-rs/core/tests/suite/rmcp_client.rs b/codex-rs/core/tests/suite/rmcp_client.rs index 9c309bf36d..297ad4da40 100644 --- a/codex-rs/core/tests/suite/rmcp_client.rs +++ b/codex-rs/core/tests/suite/rmcp_client.rs @@ -21,6 +21,7 @@ use codex_config::types::McpServerAuth; use codex_config::types::McpServerConfig; use codex_config::types::McpServerEnvVar; use codex_config::types::McpServerTransportConfig; +use codex_config::types::OAuthCredentialsStoreMode; use codex_core::config::Config; use codex_exec_server::CreateDirectoryOptions; use codex_exec_server::Environment; @@ -3168,9 +3169,14 @@ async fn streamable_http_with_oauth_round_trip_impl() -> anyhow::Result<()> { // server so the test does not share credentials with other suite cases. let temp_home = Arc::new(tempdir()?); let _codex_home_guard = EnvVarGuard::set("CODEX_HOME", temp_home.path().as_os_str()); + let environment_id = remote_aware_environment_id(); + let credential_config: McpServerConfig = serde_json::from_value(json!({ + "url": &server_url, + "environment_id": &environment_id, + }))?; + let credential_name = credential_config.oauth_credential_name(server_name); write_fallback_oauth_tokens( - temp_home.path(), - server_name, + credential_name.as_ref(), &server_url, client_id, expected_token, @@ -3196,7 +3202,7 @@ async fn streamable_http_with_oauth_round_trip_impl() -> anyhow::Result<()> { env_http_headers: None, }, TestMcpServerOptions { - environment_id: remote_aware_environment_id(), + environment_id, ..Default::default() }, ); @@ -3628,7 +3634,6 @@ fn streamable_http_metadata_url(server_url: &str) -> String { } fn write_fallback_oauth_tokens( - home: &Path, server_name: &str, server_url: &str, client_id: &str, @@ -3641,21 +3646,25 @@ fn write_fallback_oauth_tokens( .duration_since(UNIX_EPOCH)? .as_millis() as u64; - let store = serde_json::json!({ - "stub": { - "server_name": server_name, - "server_url": server_url, - "client_id": client_id, + let tokens = serde_json::from_value(json!({ + "server_name": server_name, + "url": server_url, + "client_id": client_id, + "token_response": { "access_token": access_token, - "expires_at": expires_at, + "token_type": "Bearer", "refresh_token": refresh_token, - "scopes": ["profile"], - } - }); + "scope": "profile", + }, + "expires_at": expires_at, + }))?; - let file_path = home.join(".credentials.json"); - fs::write(&file_path, serde_json::to_vec(&store)?)?; - Ok(()) + codex_rmcp_client::save_oauth_tokens( + server_name, + &tokens, + OAuthCredentialsStoreMode::File, + codex_config::types::AuthKeyringBackendKind::default(), + ) } struct EnvVarGuard { diff --git a/codex-rs/rmcp-client/src/oauth.rs b/codex-rs/rmcp-client/src/oauth.rs index ea6c24172a..1d36887fc5 100644 --- a/codex-rs/rmcp-client/src/oauth.rs +++ b/codex-rs/rmcp-client/src/oauth.rs @@ -625,6 +625,9 @@ struct FallbackTokenEntry { refresh_token: Option, #[serde(default)] scopes: Vec, + // Legacy host entries omit this marker, so executor lookups fail closed. + #[serde(default, skip_serializing_if = "std::ops::Not::not")] + executor_owned: bool, } fn load_oauth_tokens_from_file(server_name: &str, url: &str) -> Result> { @@ -634,10 +637,22 @@ fn load_oauth_tokens_from_file(server_name: &str, url: &str) -> Result Result<()> { fn save_oauth_tokens_to_file_with_lock_held(tokens: &StoredOAuthTokens) -> Result<()> { let key = compute_store_key(&tokens.server_name, &tokens.url)?; let mut store = read_fallback_file_unlocked()?.unwrap_or_default(); + let executor_owned = tokens.server_name.starts_with("executor:"); + if executor_owned && store.get(&key).is_some_and(|entry| !entry.executor_owned) { + anyhow::bail!("executor OAuth credential key conflicts with a host-owned credential"); + } let token_response = &tokens.token_response.0; let expires_at = tokens @@ -702,6 +721,7 @@ fn save_oauth_tokens_to_file_with_lock_held(tokens: &StoredOAuthTokens) -> Resul expires_at, refresh_token, scopes, + executor_owned, }; store.insert(key, entry); @@ -715,6 +735,13 @@ fn delete_oauth_tokens_from_file(key: &str) -> Result { None => return Ok(false), }; + if key.starts_with("executor:") + && !key.contains('|') + && store.get(key).is_some_and(|entry| !entry.executor_owned) + { + anyhow::bail!("executor OAuth credential key conflicts with a host-owned credential"); + } + let removed = store.remove(key).is_some(); if removed { @@ -765,6 +792,8 @@ fn token_needs_refresh(expires_at: Option) -> bool { } fn compute_store_key(server_name: &str, server_url: &str) -> Result { + let executor_owned = server_name.starts_with("executor:"); + let server_name = server_name.strip_prefix("local:").unwrap_or(server_name); let mut payload = JsonMap::new(); payload.insert( "type".to_string(), @@ -774,13 +803,14 @@ fn compute_store_key(server_name: &str, server_url: &str) -> Result { payload.insert("headers".to_string(), Value::Object(JsonMap::new())); let truncated = sha_256_prefix(&Value::Object(payload))?; - Ok(format!("{server_name}|{truncated}")) + let separator = if executor_owned { ':' } else { '|' }; + Ok(format!("{server_name}{separator}{truncated}")) } /// Derive a valid secret-store name from the MCP OAuth store key. /// /// `compute_store_key` intentionally includes readable identity components and -/// a pipe separator, but `SecretName` only allows `A-Z`, `0-9`, and `_`. +/// punctuation, but `SecretName` only allows `A-Z`, `0-9`, and `_`. /// Re-hashing keeps the secret key deterministic while satisfying that /// restricted alphabet. fn compute_secret_name(server_name: &str, server_url: &str) -> Result {