Enforce issuer binding for MCP OAuth endpoints (#39935)

## Why

Interactive MCP OAuth must not trust authorization metadata that could route an
authorization code or PKCE verifier to an unrelated token endpoint.

## What changed

- Require an advertised issuer to match the origin that served authorization
  metadata.
- Validate authorization and token endpoint origins before starting both
  pre-registered and dynamically registered client flows.
- Allow delegated endpoint origins when the server advertises issuer-bound
  authorization responses, while retaining narrow compatibility exceptions for
  existing providers.

## Testing

Add coverage for rejected untrusted metadata, accepted issuer-bound delegation,
legacy provider exceptions, and both client registration paths.

GitOrigin-RevId: 43c9f94c9f6ba2d23d7e373576cf9fa26861cfc3
This commit is contained in:
jif
2026-08-21 13:16:12 +00:00
committed by copyberry
parent 9c3da20b3f
commit 7f9832d0d0
8 changed files with 395 additions and 7 deletions

View File

@@ -130,9 +130,11 @@ async fn selected_executor_plugin_exposes_its_mcps_only_to_that_thread() -> Resu
let (registration_request_tx, mut registration_request_rx) = mpsc::unbounded_channel();
let (token_request_tx, mut token_request_rx) = mpsc::unbounded_channel();
let oauth_metadata = json!({
"issuer": EXECUTOR_OAUTH_MCP_URL,
"authorization_endpoint": "https://oauth-only.invalid/authorize",
"token_endpoint": "http://oauth-only.invalid/token",
"registration_endpoint": "http://oauth-only.invalid/register",
"authorization_response_iss_parameter_supported": true,
"scopes_supported": ["read", "write"],
"response_types_supported": ["code"],
"code_challenge_methods_supported": ["S256"],
@@ -345,7 +347,8 @@ startup_timeout_sec = 10
callback_url
.query_pairs_mut()
.append_pair("code", "configured-test-code")
.append_pair("state", &parameters["state"]);
.append_pair("state", &parameters["state"])
.append_pair("iss", EXECUTOR_OAUTH_MCP_URL);
HttpClientBuilder::new()
.build_direct()?
.get(callback_url)
@@ -408,7 +411,8 @@ startup_timeout_sec = 10
callback_url
.query_pairs_mut()
.append_pair("code", "executor-test-code")
.append_pair("state", &state);
.append_pair("state", &state)
.append_pair("iss", EXECUTOR_OAUTH_MCP_URL);
HttpClientBuilder::new()
.build_direct()?
.get(callback_url)

View File

@@ -71,6 +71,7 @@ use tokio::sync::Mutex;
use codex_utils_home_dir::find_codex_home;
pub(crate) use self::issuer_binding::validate_authorization_server_endpoints;
pub(crate) use self::issuer_binding::validate_refresh_token_issuer;
pub(crate) use self::refresh_transaction::install_tokens_in_manager;
pub(crate) use self::resolved_store::ResolvedOAuthCredentialStore;

View File

@@ -1,9 +1,78 @@
use anyhow::Context;
use anyhow::Result;
use anyhow::bail;
use rmcp::transport::auth::AuthError;
use rmcp::transport::auth::AuthorizationMetadata;
use url::Url;
use super::StoredOAuthTokens;
/// Reject authorization endpoints that cannot be bound to their actual issuer.
pub(crate) fn validate_authorization_server_endpoints(
metadata: &AuthorizationMetadata,
) -> Result<()> {
let authorization_endpoint = Url::parse(&metadata.authorization_endpoint)
.context("OAuth authorization endpoint must be a valid URL")?;
let issuer = metadata
.issuer
.as_deref()
.filter(|issuer| !issuer.trim().is_empty())
.map(Url::parse)
.transpose()
.context("OAuth authorization server issuer must be a valid URL")?;
let issuer_bound_callbacks = metadata
.additional_fields
.get("authorization_response_iss_parameter_supported")
.and_then(serde_json::Value::as_bool)
.unwrap_or(false);
if issuer_bound_callbacks {
if issuer.is_none() {
bail!("OAuth issuer-bound callbacks require an authorization server issuer");
}
return Ok(());
}
let token_endpoint =
Url::parse(&metadata.token_endpoint).context("OAuth token endpoint must be a valid URL")?;
if let Some(issuer) = issuer {
if authorization_endpoint.origin() == issuer.origin()
|| authorization_endpoint.origin() == token_endpoint.origin()
// Remove these narrow compatibility exceptions once both providers support RFC 9207.
|| matches!(
(
issuer.as_str(),
authorization_endpoint.origin().ascii_serialization().as_str(),
token_endpoint.origin().ascii_serialization().as_str(),
),
(
"https://api.figma.com/",
"https://www.figma.com",
"https://api.figma.com",
) | (
"https://agent.robinhood.com/mcp/trading",
"https://robinhood.com",
"https://api.robinhood.com",
)
)
{
return Ok(());
}
bail!(
"OAuth authorization endpoint origin does not match the authorization server origin without issuer-bound callbacks"
);
}
if token_endpoint.origin() != authorization_endpoint.origin() {
bail!(
"OAuth token endpoint origin does not match the authorization server origin without issuer-bound callbacks"
);
}
Ok(())
}
/// Verifies that a stored refresh token remains bound to its original issuer.
///
/// Call this with the same metadata snapshot that RMCP will use for the credentials. Missing or

View File

@@ -9,6 +9,8 @@ use rmcp::transport::auth::OAuthHttpClient;
use rmcp::transport::auth::OAuthState;
use url::Url;
use crate::oauth::validate_authorization_server_endpoints;
/// OAuth client-registration strategy for one interactive HTTP MCP login.
#[derive(Debug, Clone, Copy, PartialEq, Eq, Default)]
pub enum McpOAuthClientRegistration {
@@ -39,6 +41,7 @@ pub(crate) async fn start_authorization(
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)?;
let authorization_server_issuer = metadata.issuer.clone();
let cimd_advertised = metadata

View File

@@ -9,6 +9,7 @@ use codex_http_client::HttpClientFactory;
use codex_http_client::OutboundProxyPolicy;
use http::HeaderMap;
use pretty_assertions::assert_eq;
use rmcp::transport::auth::AuthorizationMetadata;
use rmcp::transport::auth::OAuthState;
use serde_json::Value;
use serde_json::json;
@@ -23,6 +24,7 @@ use wiremock::matchers::path;
use super::McpOAuthClientRegistration;
use super::start_authorization;
use crate::oauth::validate_authorization_server_endpoints;
use crate::oauth_http_client::OAuthHttpClientAdapter;
use crate::utils::MCP_USER_AGENT;
use crate::utils::build_default_headers;
@@ -209,6 +211,136 @@ async fn registration_selection_preserves_dcr_capabilities_and_exact_redirects()
Ok(())
}
#[test]
fn legacy_provider_exceptions_require_exact_issuer_and_endpoint_origins() -> Result<()> {
for (issuer, authorization_endpoint, token_endpoint, accepted) in [
(
"https://api.figma.com",
"https://www.figma.com/oauth/mcp",
"https://api.figma.com/v1/oauth/token",
true,
),
(
"https://agent.robinhood.com/mcp/trading",
"https://robinhood.com/oauth",
"https://api.robinhood.com/oauth2/token/",
true,
),
(
"https://api.figma.com.attacker.example",
"https://www.figma.com/oauth/mcp",
"https://api.figma.com.attacker.example/token",
false,
),
(
"https://api.figma.com",
"https://www.figma.com/oauth/mcp",
"https://attacker.example/token",
false,
),
(
"http://api.figma.com",
"https://www.figma.com/oauth/mcp",
"http://api.figma.com/v1/oauth/token",
false,
),
(
"https://agent.robinhood.com/mcp/attacker",
"https://robinhood.com/oauth",
"https://api.robinhood.com/oauth2/token/",
false,
),
(
"https://agent.robinhood.com/mcp/trading",
"https://robinhood.com.attacker.example/oauth",
"https://api.robinhood.com/oauth2/token/",
false,
),
] {
let metadata: AuthorizationMetadata = serde_json::from_value(json!({
"issuer": issuer,
"authorization_endpoint": authorization_endpoint,
"token_endpoint": token_endpoint,
}))?;
assert_eq!(
validate_authorization_server_endpoints(&metadata).is_ok(),
accepted,
"unexpected validation result for issuer {issuer}",
);
}
Ok(())
}
#[tokio::test]
async fn verified_issuer_can_delegate_authorization_and_token_to_one_origin() -> Result<()> {
let issuer_server = MockServer::start().await;
let endpoint_server = MockServer::start().await;
let issuer = format!("{}/mcp", issuer_server.uri());
let redirect_uri = format!("http://127.0.0.1:43123/callback/{CALLBACK_ID}");
Mock::given(method("GET"))
.and(path("/.well-known/oauth-authorization-server/mcp"))
.respond_with(ResponseTemplate::new(200).set_body_json(json!({
"issuer": issuer,
"authorization_endpoint": format!("{}/authorize", endpoint_server.uri()),
"token_endpoint": format!("{}/token", endpoint_server.uri()),
"registration_endpoint": format!("{}/register", endpoint_server.uri()),
"token_endpoint_auth_methods_supported": ["none"],
"code_challenge_methods_supported": ["S256"],
})))
.expect(1)
.mount(&issuer_server)
.await;
Mock::given(method("POST"))
.and(path("/register"))
.respond_with(|request: &Request| {
let registration: Value = serde_json::from_slice(&request.body)
.expect("dynamic registration should contain JSON");
ResponseTemplate::new(200).set_body_json(json!({
"client_id": "delegated-provider-client",
"redirect_uris": registration["redirect_uris"],
}))
})
.expect(1)
.mount(&endpoint_server)
.await;
Mock::given(method("POST"))
.and(path("/token"))
.respond_with(ResponseTemplate::new(200).set_body_json(json!({
"access_token": "delegated-provider-token",
"token_type": "Bearer",
})))
.expect(1)
.mount(&endpoint_server)
.await;
let (mut state, query) = authorization(
&issuer_server,
&redirect_uri,
McpOAuthClientRegistration::Dcr,
)
.await?;
assert_eq!(query["client_id"], "delegated-provider-client");
assert_eq!(
Url::parse(&state.get_authorization_url().await?)?.origin(),
Url::parse(&endpoint_server.uri())?.origin(),
);
state
.handle_callback_with_issuer("valid-authorization-code", &query["state"], None)
.await?;
let token_requests = requests_to(&endpoint_server, "/token").await;
assert_eq!(token_requests.len(), 1);
assert!(
url::form_urlencoded::parse(&token_requests[0].body)
.any(|(name, _)| name == "code_verifier")
);
issuer_server.verify().await;
endpoint_server.verify().await;
Ok(())
}
#[tokio::test]
async fn resource_headers_follow_same_origin_registration_redirect_and_sdk_auth_wins() -> Result<()>
{

View File

@@ -21,6 +21,7 @@ use http::header::TRANSFER_ENCODING;
use http::header::USER_AGENT;
use oauth2::HttpRequest;
use oauth2::HttpResponse;
use rmcp::transport::auth::AuthorizationMetadata;
use rmcp::transport::auth::OAuthHttpClient;
use rmcp::transport::auth::OAuthHttpClientError;
use rmcp::transport::auth::OAuthHttpClientFuture;
@@ -47,6 +48,8 @@ enum OAuthHttpClientAdapterError {
TooManyRedirects,
#[error("OAuth HTTP response body exceeds {maximum_bytes} bytes")]
ResponseBodyTooLarge { maximum_bytes: usize },
#[error("OAuth authorization server issuer does not match authorization metadata origin")]
AuthorizationMetadataIssuerOriginMismatch,
}
fn oauth_http_client_error(
@@ -284,6 +287,18 @@ impl OAuthHttpClientAdapter {
request_url = next_url;
redirects += 1;
};
if response.status == StatusCode::OK.as_u16()
&& let Ok(metadata) = serde_json::from_slice::<AuthorizationMetadata>(&body)
&& let Some(issuer) = metadata.issuer.as_deref()
&& Url::parse(issuer)
.map_err(oauth_http_client_error)?
.origin()
!= request_url.origin()
{
return Err(oauth_http_client_error(
OAuthHttpClientAdapterError::AuthorizationMetadataIssuerOriginMismatch,
));
}
let mut builder = oauth2::http::Response::builder().status(response.status);
for header in response.headers {
builder = builder.header(header.name, header.value);

View File

@@ -28,6 +28,7 @@ use crate::StoredOAuthTokens;
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_client_registration::McpOAuthClientRegistration;
use crate::oauth_client_registration::PreparedOAuthLogin;
use crate::oauth_client_registration::start_authorization as start_client_registration;
@@ -710,6 +711,7 @@ async fn start_authorization(
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)?;
let authorization_server_issuer = metadata.issuer.clone();
auth_manager.set_metadata(metadata);
auth_manager.configure_client(

View File

@@ -3,12 +3,16 @@ use std::sync::Arc;
use std::time::Duration;
use anyhow::Context as _;
use codex_config::types::AuthKeyringBackendKind;
use codex_config::types::OAuthCredentialsStoreMode;
use codex_exec_server::Environment;
use codex_exec_server::HttpClient;
use codex_rmcp_client::McpOAuthClientRegistration;
use codex_rmcp_client::OAuthDiscoveryTimeout;
use codex_rmcp_client::StreamableHttpOAuthDiscovery;
use codex_rmcp_client::StreamableHttpRedirectMode;
use codex_rmcp_client::discover_streamable_http_oauth;
use codex_rmcp_client::perform_oauth_login_return_url;
use pretty_assertions::assert_eq;
use rmcp::transport::auth::AuthError;
use serde_json::json;
@@ -236,11 +240,8 @@ async fn assert_legacy_oauth_without_starting_an_mcp_session(
assert!(
matches!(
error.downcast_ref::<AuthError>(),
Some(AuthError::AuthorizationServerMismatch {
expected_issuer,
received_issuer,
}) if expected_issuer.trim_end_matches('/') == authorization_server.uri()
&& received_issuer == "https://unexpected-issuer.example"
Some(AuthError::MetadataError(reason))
if reason.contains("issuer does not match authorization metadata origin")
),
"expected the original authorization-server issuer to remain bound: {error:#}",
);
@@ -520,3 +521,164 @@ async fn oauth_discovery_does_not_invent_support_for_an_unauthenticated_legacy_s
assert_eq!(local_discovery, None);
Ok(())
}
#[tokio::test]
async fn interactive_oauth_rejects_untrusted_authorization_metadata() -> anyhow::Result<()> {
for (metadata_issuer, authorization_metadata_path, issuer_bound_callbacks) in [
(
AuthorizationMetadataIssuer::Missing,
"/.well-known/untrusted-provider",
false,
),
(
AuthorizationMetadataIssuer::Mismatched,
"/.well-known/untrusted-provider",
true,
),
(
AuthorizationMetadataIssuer::Mismatched,
"/metadata.json",
false,
),
(
AuthorizationMetadataIssuer::Mismatched,
"/metadata.json",
true,
),
(
AuthorizationMetadataIssuer::Mismatched,
"/.well-known/oauth-authorization-server",
false,
),
(
AuthorizationMetadataIssuer::Mismatched,
"/.well-known/oauth-authorization-server",
true,
),
(
AuthorizationMetadataIssuer::Mismatched,
"/.well-known/openid-configuration",
true,
),
(
AuthorizationMetadataIssuer::Matching,
"/.well-known/untrusted-provider",
false,
),
] {
let resource_server = MockServer::start().await;
let authorization_server = MockServer::start().await;
let attacker_token_server = MockServer::start().await;
let resource_url = format!("{}/mcp", resource_server.uri());
let resource_metadata_url = format!("{}/resource-metadata", resource_server.uri());
let (issuer, expected_error) = match metadata_issuer {
AuthorizationMetadataIssuer::Missing => (
None,
"token endpoint origin does not match the authorization server origin",
),
AuthorizationMetadataIssuer::Mismatched => (
Some(authorization_server.uri()),
"issuer does not match authorization metadata origin",
),
AuthorizationMetadataIssuer::Matching => (
Some(resource_server.uri()),
"authorization endpoint origin does not match the authorization server origin",
),
};
let mut authorization_metadata = json!({
"authorization_endpoint": format!("{}/authorize", authorization_server.uri()),
"registration_endpoint": format!("{}/register", authorization_server.uri()),
"token_endpoint": format!("{}/token", attacker_token_server.uri()),
"authorization_response_iss_parameter_supported": issuer_bound_callbacks,
"code_challenge_methods_supported": ["S256"],
});
if let Some(issuer) = issuer {
authorization_metadata["issuer"] = json!(issuer);
}
Mock::given(method("GET"))
.and(path("/mcp"))
.respond_with(ResponseTemplate::new(401).insert_header(
"www-authenticate",
format!("Bearer resource_metadata=\"{resource_metadata_url}\""),
))
.expect(2)
.mount(&resource_server)
.await;
Mock::given(method("GET"))
.and(path("/resource-metadata"))
.respond_with(ResponseTemplate::new(200).set_body_json(json!({
"resource": resource_url,
"authorization_servers": [format!(
"{}/.well-known/untrusted-provider",
resource_server.uri()
)],
})))
.expect(2)
.mount(&resource_server)
.await;
if authorization_metadata_path != "/.well-known/untrusted-provider" {
Mock::given(method("GET"))
.and(path("/.well-known/untrusted-provider"))
.respond_with(
ResponseTemplate::new(302)
.insert_header("location", authorization_metadata_path),
)
.expect(2)
.mount(&resource_server)
.await;
}
Mock::given(method("GET"))
.and(path(authorization_metadata_path))
.respond_with(ResponseTemplate::new(200).set_body_json(authorization_metadata))
.expect(2)
.mount(&resource_server)
.await;
Mock::given(method("POST"))
.respond_with(ResponseTemplate::new(200))
.expect(0)
.mount(&attacker_token_server)
.await;
for oauth_client_id in [None, Some("preregistered-client")] {
let error = perform_oauth_login_return_url(
"untrusted-oauth-metadata",
&resource_url,
OAuthCredentialsStoreMode::File,
AuthKeyringBackendKind::Direct,
/*http_headers*/ None,
/*env_http_headers*/ None,
/*scopes*/ &[],
oauth_client_id,
McpOAuthClientRegistration::Dcr,
/*oauth_resource*/ None,
Some(/*timeout_secs*/ 5),
/*callback_port*/ None,
/*callback_url*/ None,
local_http_client(),
StreamableHttpRedirectMode::Legacy,
)
.await
.err()
.context("untrusted OAuth authorization metadata must fail")?;
assert!(
format!("{error:#}").contains(expected_error),
"unexpected authorization failure for {oauth_client_id:?}: {error:#}",
);
}
assert!(
attacker_token_server
.received_requests()
.await
.context("attacker token server request recording should be enabled")?
.is_empty(),
"the attacker must never receive an authorization code or PKCE verifier",
);
attacker_token_server.verify().await;
authorization_server.verify().await;
resource_server.verify().await;
}
Ok(())
}