github/codex
4
0 Fork 0
mirror of https://github.com/openai/codex.git synced 2026-11-09 08:40:33 +00:00
Code Issues Packages Projects Releases Wiki Activity

Harden MCP OAuth callback handling (#40691)

Browse Source 
## 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
This commit is contained in:
stevenlee-oai authored and copyberry committed 2026-08-25 20:26:23 +00:00
1 parent 32ce703a80
commit 9be8d6e1c3
28 files changed
+1096 -127
Download Patch File Download Diff File Expand all files Collapse all files

No files matched your search

codex-rs/app-server/tests/suite/v2/mcp_server_status.rs
+100 -11
View File
@@ -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(())
codex-rs/app-server/tests/suite/v2/plugin_install.rs
+205 -5
View File
@@ -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(())
}