mirror of
https://github.com/openai/codex.git
synced 2026-06-01 19:02:59 +00:00
Support explicit MCP OAuth client IDs (#22575)
## Why Some MCP OAuth providers require a pre-registered public client ID and cannot rely on dynamic client registration. Codex already supports MCP OAuth, but it had no way to supply that client ID from config into the PKCE flow. ## What changed - add `oauth.client_id` under `[mcp_servers.<server>]` config, including config editing and schema generation - thread the configured client ID through CLI, app-server, plugin login, and MCP skill dependency OAuth entrypoints - configure RMCP authorization with the explicit client when present, while preserving the existing dynamic-registration path when it is absent - add focused coverage for config parsing/serialization and OAuth URL generation ## Verification - `cargo test -p codex-config -p codex-rmcp-client -p codex-mcp -p codex-core-plugins` - `cargo test -p codex-core blocking_replace_mcp_servers_round_trips --lib` - `cargo test -p codex-core replace_mcp_servers_streamable_http_serializes_oauth_resource --lib` - `cargo test -p codex-core config_schema_matches_fixture --lib` ## Notes Broader local package runs still hit unrelated pre-existing stack overflows in: - `codex-app-server::in_process_start_clamps_zero_channel_capacity` - `codex-core::resume_agent_from_rollout_uses_edge_data_when_descendant_metadata_source_is_stale`
This commit is contained in:
@@ -11,6 +11,9 @@ use base64::Engine;
|
||||
use base64::engine::general_purpose::URL_SAFE_NO_PAD;
|
||||
use reqwest::ClientBuilder;
|
||||
use reqwest::Url;
|
||||
use rmcp::transport::AuthorizationManager;
|
||||
use rmcp::transport::AuthorizationSession;
|
||||
use rmcp::transport::auth::OAuthClientConfig;
|
||||
use rmcp::transport::auth::OAuthState;
|
||||
use sha2::Digest;
|
||||
use sha2::Sha256;
|
||||
@@ -81,6 +84,7 @@ pub async fn perform_oauth_login(
|
||||
http_headers: Option<HashMap<String, String>>,
|
||||
env_http_headers: Option<HashMap<String, String>>,
|
||||
scopes: &[String],
|
||||
oauth_client_id: Option<&str>,
|
||||
oauth_resource: Option<&str>,
|
||||
callback_port: Option<u16>,
|
||||
callback_url: Option<&str>,
|
||||
@@ -92,6 +96,7 @@ pub async fn perform_oauth_login(
|
||||
http_headers,
|
||||
env_http_headers,
|
||||
scopes,
|
||||
oauth_client_id,
|
||||
oauth_resource,
|
||||
callback_port,
|
||||
callback_url,
|
||||
@@ -108,6 +113,7 @@ pub async fn perform_oauth_login_silent(
|
||||
http_headers: Option<HashMap<String, String>>,
|
||||
env_http_headers: Option<HashMap<String, String>>,
|
||||
scopes: &[String],
|
||||
oauth_client_id: Option<&str>,
|
||||
oauth_resource: Option<&str>,
|
||||
callback_port: Option<u16>,
|
||||
callback_url: Option<&str>,
|
||||
@@ -119,6 +125,7 @@ pub async fn perform_oauth_login_silent(
|
||||
http_headers,
|
||||
env_http_headers,
|
||||
scopes,
|
||||
oauth_client_id,
|
||||
oauth_resource,
|
||||
callback_port,
|
||||
callback_url,
|
||||
@@ -135,6 +142,7 @@ async fn perform_oauth_login_with_browser_output(
|
||||
http_headers: Option<HashMap<String, String>>,
|
||||
env_http_headers: Option<HashMap<String, String>>,
|
||||
scopes: &[String],
|
||||
oauth_client_id: Option<&str>,
|
||||
oauth_resource: Option<&str>,
|
||||
callback_port: Option<u16>,
|
||||
callback_url: Option<&str>,
|
||||
@@ -150,6 +158,7 @@ async fn perform_oauth_login_with_browser_output(
|
||||
store_mode,
|
||||
headers,
|
||||
scopes,
|
||||
oauth_client_id,
|
||||
oauth_resource,
|
||||
/*launch_browser*/ true,
|
||||
callback_port,
|
||||
@@ -169,6 +178,7 @@ pub async fn perform_oauth_login_return_url(
|
||||
http_headers: Option<HashMap<String, String>>,
|
||||
env_http_headers: Option<HashMap<String, String>>,
|
||||
scopes: &[String],
|
||||
oauth_client_id: Option<&str>,
|
||||
oauth_resource: Option<&str>,
|
||||
timeout_secs: Option<i64>,
|
||||
callback_port: Option<u16>,
|
||||
@@ -184,6 +194,7 @@ pub async fn perform_oauth_login_return_url(
|
||||
store_mode,
|
||||
headers,
|
||||
scopes,
|
||||
oauth_client_id,
|
||||
oauth_resource,
|
||||
/*launch_browser*/ false,
|
||||
callback_port,
|
||||
@@ -436,6 +447,7 @@ impl OauthLoginFlow {
|
||||
store_mode: OAuthCredentialsStoreMode,
|
||||
headers: OauthHeaders,
|
||||
scopes: &[String],
|
||||
oauth_client_id: Option<&str>,
|
||||
oauth_resource: Option<&str>,
|
||||
launch_browser: bool,
|
||||
callback_port: Option<u16>,
|
||||
@@ -471,11 +483,15 @@ impl OauthLoginFlow {
|
||||
let default_headers = build_default_headers(http_headers, env_http_headers)?;
|
||||
let http_client = apply_default_headers(ClientBuilder::new(), &default_headers).build()?;
|
||||
|
||||
let mut oauth_state = OAuthState::new(server_url, Some(http_client)).await?;
|
||||
let scope_refs: Vec<&str> = scopes.iter().map(String::as_str).collect();
|
||||
oauth_state
|
||||
.start_authorization(&scope_refs, &redirect_uri, Some("Codex"))
|
||||
.await?;
|
||||
let oauth_state = start_authorization(
|
||||
server_url,
|
||||
http_client,
|
||||
&scope_refs,
|
||||
&redirect_uri,
|
||||
oauth_client_id,
|
||||
)
|
||||
.await?;
|
||||
let auth_url = append_query_param(
|
||||
&oauth_state.get_authorization_url().await?,
|
||||
"resource",
|
||||
@@ -585,6 +601,41 @@ impl OauthLoginFlow {
|
||||
}
|
||||
}
|
||||
|
||||
async fn start_authorization(
|
||||
server_url: &str,
|
||||
http_client: reqwest::Client,
|
||||
scopes: &[&str],
|
||||
redirect_uri: &str,
|
||||
oauth_client_id: Option<&str>,
|
||||
) -> Result<OAuthState> {
|
||||
let Some(oauth_client_id) = oauth_client_id.filter(|client_id| !client_id.trim().is_empty())
|
||||
else {
|
||||
let mut oauth_state = OAuthState::new(server_url, Some(http_client)).await?;
|
||||
oauth_state
|
||||
.start_authorization(scopes, redirect_uri, Some("Codex"))
|
||||
.await?;
|
||||
return Ok(oauth_state);
|
||||
};
|
||||
|
||||
let mut auth_manager = AuthorizationManager::new(server_url).await?;
|
||||
auth_manager.with_client(http_client)?;
|
||||
let metadata = auth_manager.discover_metadata().await?;
|
||||
auth_manager.set_metadata(metadata);
|
||||
auth_manager.configure_client(OAuthClientConfig {
|
||||
client_id: oauth_client_id.to_string(),
|
||||
client_secret: None,
|
||||
scopes: scopes.iter().map(|scope| (*scope).to_string()).collect(),
|
||||
redirect_uri: redirect_uri.to_string(),
|
||||
})?;
|
||||
let auth_url = auth_manager.get_authorization_url(scopes).await?;
|
||||
|
||||
Ok(OAuthState::Session(AuthorizationSession {
|
||||
auth_manager,
|
||||
auth_url,
|
||||
redirect_uri: redirect_uri.to_string(),
|
||||
}))
|
||||
}
|
||||
|
||||
fn append_query_param(url: &str, key: &str, value: Option<&str>) -> String {
|
||||
let Some(value) = value else {
|
||||
return url.to_string();
|
||||
@@ -604,7 +655,13 @@ fn append_query_param(url: &str, key: &str, value: Option<&str>) -> String {
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use axum::Json;
|
||||
use axum::Router;
|
||||
use axum::routing::get;
|
||||
use pretty_assertions::assert_eq;
|
||||
use reqwest::Url;
|
||||
use serde_json::json;
|
||||
use tokio::net::TcpListener;
|
||||
|
||||
use super::CallbackOutcome;
|
||||
use super::OAuthProviderError;
|
||||
@@ -613,6 +670,70 @@ mod tests {
|
||||
use super::callback_id_from_server_url;
|
||||
use super::callback_path_from_redirect_uri;
|
||||
use super::parse_oauth_callback;
|
||||
use super::start_authorization;
|
||||
|
||||
async fn spawn_oauth_metadata_server() -> String {
|
||||
let listener = TcpListener::bind("127.0.0.1:0")
|
||||
.await
|
||||
.expect("bind metadata listener");
|
||||
let addr = listener.local_addr().expect("read metadata listener addr");
|
||||
let base_url = format!("http://{addr}");
|
||||
let metadata = json!({
|
||||
"authorization_endpoint": format!("{base_url}/oauth/authorize"),
|
||||
"token_endpoint": format!("{base_url}/oauth/token"),
|
||||
"scopes_supported": [""],
|
||||
});
|
||||
let path_scoped_metadata = metadata.clone();
|
||||
let app = Router::new()
|
||||
.route(
|
||||
"/.well-known/oauth-authorization-server/mcp",
|
||||
get(move || {
|
||||
let metadata = path_scoped_metadata.clone();
|
||||
async move { Json(metadata) }
|
||||
}),
|
||||
)
|
||||
.route(
|
||||
"/.well-known/oauth-authorization-server",
|
||||
get(move || {
|
||||
let metadata = metadata.clone();
|
||||
async move { Json(metadata) }
|
||||
}),
|
||||
);
|
||||
|
||||
tokio::spawn(async move {
|
||||
axum::serve(listener, app)
|
||||
.await
|
||||
.expect("serve oauth metadata");
|
||||
});
|
||||
|
||||
base_url
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn start_authorization_uses_configured_client_id() {
|
||||
let base_url = spawn_oauth_metadata_server().await;
|
||||
let oauth_state = start_authorization(
|
||||
&format!("{base_url}/mcp"),
|
||||
reqwest::Client::new(),
|
||||
&[],
|
||||
"http://127.0.0.1/callback",
|
||||
Some("eci-prd-pub-codex-123"),
|
||||
)
|
||||
.await
|
||||
.expect("start oauth authorization");
|
||||
|
||||
let authorization_url = oauth_state
|
||||
.get_authorization_url()
|
||||
.await
|
||||
.expect("read authorization url");
|
||||
let auth_url = Url::parse(&authorization_url).expect("authorization url should parse");
|
||||
let client_id = auth_url
|
||||
.query_pairs()
|
||||
.find(|(key, _)| key == "client_id")
|
||||
.map(|(_, value)| value.into_owned());
|
||||
|
||||
assert_eq!(client_id.as_deref(), Some("eci-prd-pub-codex-123"));
|
||||
}
|
||||
|
||||
#[test]
|
||||
fn parse_oauth_callback_accepts_default_path() {
|
||||
|
||||
Reference in New Issue
Block a user