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
This commit is contained in:
jif
2026-07-31 11:23:46 +00:00
committed by copyberry
parent 5e8b22488f
commit 164b3bfeab
12 changed files with 194 additions and 39 deletions
@@ -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,
@@ -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,
@@ -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::<Vec<_>>();
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(),
+4 -2
View File
@@ -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(),
+3 -1
View File
@@ -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(),
+2 -1
View File
@@ -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(),
+2 -1
View File
@@ -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,
+9 -5
View File
@@ -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!(
+18
View File
@@ -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()
+3 -2
View File
@@ -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(),
+25 -16
View File
@@ -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 {
+35 -5
View File
@@ -625,6 +625,9 @@ struct FallbackTokenEntry {
refresh_token: Option<String>,
#[serde(default)]
scopes: Vec<String>,
// 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<Option<StoredOAuthTokens>> {
@@ -634,10 +637,22 @@ fn load_oauth_tokens_from_file(server_name: &str, url: &str) -> Result<Option<St
};
let key = compute_store_key(server_name, url)?;
let local_server_name = server_name.strip_prefix("local:").unwrap_or(server_name);
for entry in store.values() {
let entry_key = compute_store_key(&entry.server_name, &entry.server_url)?;
if entry_key != key {
for (stored_key, entry) in &store {
let matches_credential = if server_name.starts_with("executor:") {
stored_key == &key
&& entry.executor_owned
&& entry.server_name == server_name
&& entry.server_url == url
} else if entry.executor_owned {
false
} else {
entry.server_url == url
&& (entry.server_name == local_server_name
|| (stored_key == &key && entry.server_name == server_name))
};
if !matches_credential {
continue;
}
@@ -682,6 +697,10 @@ fn save_oauth_tokens_to_file(tokens: &StoredOAuthTokens) -> 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<bool> {
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<u64>) -> bool {
}
fn compute_store_key(server_name: &str, server_url: &str) -> Result<String> {
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<String> {
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<SecretName> {