mirror of
https://github.com/openai/codex.git
synced 2026-10-21 04:41:48 +00:00
Isolate MCP resource headers during OAuth requests (#39019)
## Why Headers configured for an MCP resource can contain credentials and must not be forwarded to a different origin during OAuth discovery, registration, or token exchange. ## What changed - Apply configured resource headers only to OAuth requests on the MCP resource origin, while allowing OAuth-generated headers to take precedence. - Follow credentialed OAuth redirects only when each destination remains on the resource origin. Preserve redirect method semantics, the overall timeout, the response size limit, and a maximum of 10 redirects. - Use the Codex MCP user agent for requests to authorization-server origins. ## Testing Add coverage for cross-origin header isolation, same-origin dynamic client registration redirects, OAuth header precedence, token refresh, timeout accounting, and response size limits. GitOrigin-RevId: 609d06e2b065e4aa9ca5382b4ae60455f9e26db9
This commit is contained in:
@@ -225,17 +225,19 @@ async fn discover_streamable_http_oauth_with_headers_and_http_client(
|
||||
OAuthDiscoveryTimeout::Requested => OAuthHttpClientAdapter::new_with_redirect_mode(
|
||||
http_client,
|
||||
default_headers,
|
||||
url,
|
||||
has_configured_headers,
|
||||
redirect_mode,
|
||||
),
|
||||
)?,
|
||||
OAuthDiscoveryTimeout::Capped(max_timeout) => {
|
||||
OAuthHttpClientAdapter::new_with_max_timeout_and_redirect_mode(
|
||||
http_client,
|
||||
default_headers,
|
||||
url,
|
||||
max_timeout,
|
||||
has_configured_headers,
|
||||
redirect_mode,
|
||||
)
|
||||
)?
|
||||
}
|
||||
};
|
||||
let mut authorization_manager =
|
||||
|
||||
@@ -407,6 +407,7 @@ async fn authorization_manager_for(
|
||||
OutboundProxyPolicy::ReqwestDefault,
|
||||
))),
|
||||
HeaderMap::new(),
|
||||
&tokens.url,
|
||||
));
|
||||
let mut state =
|
||||
OAuthState::new_with_oauth_http_client(tokens.url.clone(), oauth_http_client).await?;
|
||||
|
||||
@@ -2,6 +2,8 @@ use std::collections::HashMap;
|
||||
use std::sync::Arc;
|
||||
|
||||
use anyhow::Result;
|
||||
use base64::Engine;
|
||||
use base64::engine::general_purpose::STANDARD;
|
||||
use codex_exec_server::RouteAwareHttpClient;
|
||||
use codex_http_client::HttpClientFactory;
|
||||
use codex_http_client::OutboundProxyPolicy;
|
||||
@@ -15,12 +17,15 @@ use wiremock::Mock;
|
||||
use wiremock::MockServer;
|
||||
use wiremock::Request;
|
||||
use wiremock::ResponseTemplate;
|
||||
use wiremock::matchers::header;
|
||||
use wiremock::matchers::method;
|
||||
use wiremock::matchers::path;
|
||||
|
||||
use super::McpOAuthClientRegistration;
|
||||
use super::start_authorization;
|
||||
use crate::oauth_http_client::OAuthHttpClientAdapter;
|
||||
use crate::utils::MCP_USER_AGENT;
|
||||
use crate::utils::build_default_headers;
|
||||
|
||||
const CALLBACK_ID: &str = "abc123ABC_-x";
|
||||
|
||||
@@ -103,6 +108,7 @@ async fn authorization(
|
||||
OutboundProxyPolicy::ReqwestDefault,
|
||||
))),
|
||||
HeaderMap::new(),
|
||||
&format!("{}/mcp", server.uri()),
|
||||
)),
|
||||
&["read"],
|
||||
redirect_uri,
|
||||
@@ -202,6 +208,141 @@ async fn registration_selection_preserves_dcr_capabilities_and_exact_redirects()
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn resource_headers_follow_same_origin_registration_redirect_and_sdk_auth_wins() -> Result<()>
|
||||
{
|
||||
const RESOURCE_AUTHORIZATION: &str = "Bearer resource-only-secret";
|
||||
const RESOURCE_API_KEY: &str = "resource-api-key-secret";
|
||||
// These dummy OAuth credentials exist only in this test's local mock server.
|
||||
const DUMMY_CLIENT_ID: &str = "dummy-test-client-id";
|
||||
const DUMMY_CLIENT_SECRET: &str = "dummy-test-client-secret";
|
||||
let sdk_authorization = format!(
|
||||
"Basic {}",
|
||||
STANDARD.encode(format!("{DUMMY_CLIENT_ID}:{DUMMY_CLIENT_SECRET}"))
|
||||
);
|
||||
|
||||
let server = MockServer::start().await;
|
||||
let authorization_server = MockServer::start().await;
|
||||
let resource_url = format!("{}/mcp", server.uri());
|
||||
let resource_metadata_url = format!("{}/resource-metadata", server.uri());
|
||||
let redirect_uri = format!("http://127.0.0.1:43123/callback/{CALLBACK_ID}");
|
||||
|
||||
Mock::given(method("GET"))
|
||||
.and(path("/mcp"))
|
||||
.and(header("authorization", RESOURCE_AUTHORIZATION))
|
||||
.and(header("x-api-key", RESOURCE_API_KEY))
|
||||
.respond_with(ResponseTemplate::new(401).insert_header(
|
||||
"www-authenticate",
|
||||
format!("Bearer resource_metadata=\"{resource_metadata_url}\""),
|
||||
))
|
||||
.expect(1)
|
||||
.mount(&server)
|
||||
.await;
|
||||
Mock::given(method("GET"))
|
||||
.and(path("/resource-metadata"))
|
||||
.and(header("authorization", RESOURCE_AUTHORIZATION))
|
||||
.and(header("x-api-key", RESOURCE_API_KEY))
|
||||
.respond_with(ResponseTemplate::new(200).set_body_json(json!({
|
||||
"resource": resource_url,
|
||||
"authorization_servers": [authorization_server.uri()],
|
||||
})))
|
||||
.expect(1)
|
||||
.mount(&server)
|
||||
.await;
|
||||
Mock::given(method("GET"))
|
||||
.and(path("/.well-known/oauth-authorization-server"))
|
||||
.and(header("user-agent", MCP_USER_AGENT))
|
||||
.respond_with(ResponseTemplate::new(200).set_body_json(json!({
|
||||
"issuer": authorization_server.uri(),
|
||||
"authorization_endpoint": format!("{}/authorize", authorization_server.uri()),
|
||||
"token_endpoint": format!("{}/token", server.uri()),
|
||||
"registration_endpoint": format!("{}/register", server.uri()),
|
||||
"token_endpoint_auth_methods_supported": ["client_secret_basic"],
|
||||
"code_challenge_methods_supported": ["S256"],
|
||||
})))
|
||||
.expect(1)
|
||||
.mount(&authorization_server)
|
||||
.await;
|
||||
Mock::given(method("POST"))
|
||||
.and(path("/register"))
|
||||
.and(header("authorization", RESOURCE_AUTHORIZATION))
|
||||
.and(header("x-api-key", RESOURCE_API_KEY))
|
||||
.and(header("content-type", "application/json"))
|
||||
.respond_with(ResponseTemplate::new(307).insert_header("location", "/register/"))
|
||||
.expect(1)
|
||||
.mount(&server)
|
||||
.await;
|
||||
Mock::given(method("POST"))
|
||||
.and(path("/register/"))
|
||||
.and(header("authorization", RESOURCE_AUTHORIZATION))
|
||||
.and(header("x-api-key", RESOURCE_API_KEY))
|
||||
.respond_with(ResponseTemplate::new(201).set_body_json(json!({
|
||||
"client_id": DUMMY_CLIENT_ID,
|
||||
"client_secret": DUMMY_CLIENT_SECRET,
|
||||
"redirect_uris": [redirect_uri],
|
||||
})))
|
||||
.expect(1)
|
||||
.mount(&server)
|
||||
.await;
|
||||
Mock::given(method("POST"))
|
||||
.and(path("/token"))
|
||||
.and(header("authorization", sdk_authorization))
|
||||
.and(header("x-api-key", RESOURCE_API_KEY))
|
||||
.respond_with(ResponseTemplate::new(200).set_body_json(json!({
|
||||
"access_token": "test-access-token",
|
||||
"token_type": "Bearer",
|
||||
})))
|
||||
.expect(1)
|
||||
.mount(&server)
|
||||
.await;
|
||||
|
||||
let default_headers = build_default_headers(
|
||||
Some(HashMap::from([
|
||||
(
|
||||
"Authorization".to_string(),
|
||||
RESOURCE_AUTHORIZATION.to_string(),
|
||||
),
|
||||
("X-Api-Key".to_string(), RESOURCE_API_KEY.to_string()),
|
||||
])),
|
||||
/*env_http_headers*/ None,
|
||||
)?;
|
||||
let mut state = start_authorization(
|
||||
&resource_url,
|
||||
Arc::new(OAuthHttpClientAdapter::new(
|
||||
Arc::new(RouteAwareHttpClient::new(HttpClientFactory::new(
|
||||
OutboundProxyPolicy::ReqwestDefault,
|
||||
))),
|
||||
default_headers,
|
||||
&resource_url,
|
||||
)),
|
||||
&[],
|
||||
&redirect_uri,
|
||||
CALLBACK_ID,
|
||||
McpOAuthClientRegistration::Dcr,
|
||||
)
|
||||
.await?;
|
||||
let csrf_state = Url::parse(&state.get_authorization_url().await?)?
|
||||
.query_pairs()
|
||||
.find(|(name, _)| name == "state")
|
||||
.expect("authorization request should contain a CSRF state")
|
||||
.1
|
||||
.into_owned();
|
||||
state
|
||||
.handle_callback_with_issuer("valid-authorization-code", &csrf_state, None)
|
||||
.await?;
|
||||
|
||||
let authorization_requests = authorization_server
|
||||
.received_requests()
|
||||
.await
|
||||
.expect("authorization server should record requests");
|
||||
assert_eq!(authorization_requests.len(), 1);
|
||||
assert_eq!(authorization_requests[0].headers.get("authorization"), None);
|
||||
assert_eq!(authorization_requests[0].headers.get("x-api-key"), None);
|
||||
server.verify().await;
|
||||
authorization_server.verify().await;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn invalid_cimd_metadata_and_redirects_fail_without_dynamic_registration() {
|
||||
let valid = "http://127.0.0.1:43123/callback/abc123ABC_-x";
|
||||
|
||||
@@ -2,13 +2,23 @@ use std::sync::Arc;
|
||||
use std::sync::atomic::AtomicU64;
|
||||
use std::sync::atomic::Ordering;
|
||||
use std::time::Duration;
|
||||
use std::time::Instant;
|
||||
|
||||
use codex_exec_server::HttpClient;
|
||||
use codex_exec_server::HttpHeader;
|
||||
use codex_exec_server::HttpRedirectPolicy;
|
||||
use codex_exec_server::HttpRequestParams;
|
||||
use http::HeaderMap;
|
||||
use http::HeaderValue;
|
||||
use http::Method;
|
||||
use http::StatusCode;
|
||||
use http::header::AUTHORIZATION;
|
||||
use http::header::CONTENT_ENCODING;
|
||||
use http::header::CONTENT_LENGTH;
|
||||
use http::header::CONTENT_TYPE;
|
||||
use http::header::LOCATION;
|
||||
use http::header::TRANSFER_ENCODING;
|
||||
use http::header::USER_AGENT;
|
||||
use oauth2::HttpRequest;
|
||||
use oauth2::HttpResponse;
|
||||
use rmcp::transport::auth::OAuthHttpClient;
|
||||
@@ -16,17 +26,25 @@ use rmcp::transport::auth::OAuthHttpClientError;
|
||||
use rmcp::transport::auth::OAuthHttpClientFuture;
|
||||
use rmcp::transport::auth::OAuthHttpRedirectPolicy;
|
||||
use rmcp::transport::auth::OAuthHttpRequest;
|
||||
use url::Origin;
|
||||
use url::Url;
|
||||
|
||||
use crate::auth_status::OAuthDiscoveryTimeout;
|
||||
use crate::http_client_adapter::StreamableHttpRedirectMode;
|
||||
use crate::utils::MCP_USER_AGENT;
|
||||
|
||||
const MAX_OAUTH_HTTP_RESPONSE_BODY_BYTES: usize = 1024 * 1024;
|
||||
const MAX_OAUTH_HTTP_REDIRECTS: usize = 10;
|
||||
static NEXT_OAUTH_REQUEST_ID: AtomicU64 = AtomicU64::new(0);
|
||||
|
||||
#[derive(Debug, thiserror::Error)]
|
||||
enum OAuthHttpClientAdapterError {
|
||||
#[error("unsupported OAuth HTTP redirect policy")]
|
||||
UnsupportedRedirectPolicy,
|
||||
#[error("OAuth HTTP request timed out")]
|
||||
TimedOut,
|
||||
#[error("OAuth HTTP request exceeded {MAX_OAUTH_HTTP_REDIRECTS} redirects")]
|
||||
TooManyRedirects,
|
||||
#[error("OAuth HTTP response body exceeds {maximum_bytes} bytes")]
|
||||
ResponseBodyTooLarge { maximum_bytes: usize },
|
||||
}
|
||||
@@ -41,6 +59,7 @@ fn oauth_http_client_error(
|
||||
pub(crate) struct OAuthHttpClientAdapter {
|
||||
http_client: Arc<dyn HttpClient>,
|
||||
default_headers: HeaderMap,
|
||||
resource_origin: Origin,
|
||||
timeout: OAuthDiscoveryTimeout,
|
||||
has_configured_headers: bool,
|
||||
redirect_mode: StreamableHttpRedirectMode,
|
||||
@@ -48,45 +67,54 @@ pub(crate) struct OAuthHttpClientAdapter {
|
||||
|
||||
impl OAuthHttpClientAdapter {
|
||||
#[cfg(test)]
|
||||
pub(crate) fn new(http_client: Arc<dyn HttpClient>, default_headers: HeaderMap) -> Self {
|
||||
Self {
|
||||
pub(crate) fn new(
|
||||
http_client: Arc<dyn HttpClient>,
|
||||
default_headers: HeaderMap,
|
||||
resource_url: &str,
|
||||
) -> Self {
|
||||
Self::new_with_redirect_mode(
|
||||
http_client,
|
||||
default_headers,
|
||||
timeout: OAuthDiscoveryTimeout::Requested,
|
||||
has_configured_headers: false,
|
||||
redirect_mode: StreamableHttpRedirectMode::Legacy,
|
||||
}
|
||||
resource_url,
|
||||
/*has_configured_headers*/ false,
|
||||
StreamableHttpRedirectMode::Legacy,
|
||||
)
|
||||
.expect("OAuth resource URL should be valid")
|
||||
}
|
||||
|
||||
pub(crate) fn new_with_redirect_mode(
|
||||
http_client: Arc<dyn HttpClient>,
|
||||
default_headers: HeaderMap,
|
||||
resource_url: &str,
|
||||
has_configured_headers: bool,
|
||||
redirect_mode: StreamableHttpRedirectMode,
|
||||
) -> Self {
|
||||
Self {
|
||||
) -> Result<Self, url::ParseError> {
|
||||
Ok(Self {
|
||||
http_client,
|
||||
default_headers,
|
||||
resource_origin: Url::parse(resource_url)?.origin(),
|
||||
timeout: OAuthDiscoveryTimeout::Requested,
|
||||
has_configured_headers,
|
||||
redirect_mode,
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
pub(crate) fn new_with_max_timeout_and_redirect_mode(
|
||||
http_client: Arc<dyn HttpClient>,
|
||||
default_headers: HeaderMap,
|
||||
resource_url: &str,
|
||||
max_timeout: Duration,
|
||||
has_configured_headers: bool,
|
||||
redirect_mode: StreamableHttpRedirectMode,
|
||||
) -> Self {
|
||||
Self {
|
||||
) -> Result<Self, url::ParseError> {
|
||||
Ok(Self {
|
||||
http_client,
|
||||
default_headers,
|
||||
resource_origin: Url::parse(resource_url)?.origin(),
|
||||
timeout: OAuthDiscoveryTimeout::Capped(max_timeout),
|
||||
has_configured_headers,
|
||||
redirect_mode,
|
||||
}
|
||||
})
|
||||
}
|
||||
|
||||
async fn execute_request(
|
||||
@@ -105,17 +133,35 @@ impl OAuthHttpClientAdapter {
|
||||
}
|
||||
};
|
||||
let (parts, body) = request.into_parts();
|
||||
let mut headers = self.default_headers.clone();
|
||||
let mut request_url =
|
||||
Url::parse(&parts.uri.to_string()).map_err(oauth_http_client_error)?;
|
||||
let is_resource_origin = request_url.origin() == self.resource_origin;
|
||||
let mut headers = if is_resource_origin {
|
||||
self.default_headers.clone()
|
||||
} else {
|
||||
HeaderMap::new()
|
||||
};
|
||||
for name in parts.headers.keys() {
|
||||
headers.remove(name);
|
||||
}
|
||||
let has_resource_only_headers = is_resource_origin
|
||||
&& headers.iter().any(|(name, value)| {
|
||||
name != USER_AGENT || value != HeaderValue::from_static(MCP_USER_AGENT)
|
||||
});
|
||||
headers.extend(parts.headers);
|
||||
if !is_resource_origin {
|
||||
headers.insert(USER_AGENT, HeaderValue::from_static(MCP_USER_AGENT));
|
||||
}
|
||||
let redirect_policy = oauth_redirect_policy(
|
||||
self.redirect_mode,
|
||||
&headers,
|
||||
self.has_configured_headers,
|
||||
redirect_policy,
|
||||
);
|
||||
// The executor can only follow every redirect or none, so replay credentialed
|
||||
// requests ourselves after checking that each destination stays on the resource origin.
|
||||
let follow_same_origin_redirects =
|
||||
has_resource_only_headers && redirect_policy == HttpRedirectPolicy::Follow;
|
||||
|
||||
let headers = headers
|
||||
.iter()
|
||||
@@ -137,32 +183,106 @@ impl OAuthHttpClientAdapter {
|
||||
.unwrap_or(u64::MAX)
|
||||
.max(1)
|
||||
});
|
||||
let request_id = NEXT_OAUTH_REQUEST_ID.fetch_add(1, Ordering::Relaxed);
|
||||
let (response, mut body_stream) = self
|
||||
.http_client
|
||||
.http_request_stream(HttpRequestParams {
|
||||
method: parts.method.to_string(),
|
||||
url: parts.uri.to_string(),
|
||||
headers,
|
||||
body: (!body.is_empty()).then_some(body.into()),
|
||||
timeout_ms,
|
||||
redirect_policy,
|
||||
request_id: format!("oauth-request-{request_id}"),
|
||||
stream_response: true,
|
||||
})
|
||||
.await
|
||||
.map_err(oauth_http_client_error)?;
|
||||
let mut body = Vec::new();
|
||||
while let Some(chunk) = body_stream.recv().await.map_err(oauth_http_client_error)? {
|
||||
if chunk.len() > MAX_OAUTH_HTTP_RESPONSE_BODY_BYTES - body.len() {
|
||||
let deadline = timeout.map(|timeout| Instant::now() + timeout);
|
||||
let mut params = HttpRequestParams {
|
||||
method: parts.method.to_string(),
|
||||
url: parts.uri.to_string(),
|
||||
headers,
|
||||
body: (!body.is_empty()).then_some(body.into()),
|
||||
timeout_ms,
|
||||
redirect_policy: if follow_same_origin_redirects {
|
||||
HttpRedirectPolicy::Stop
|
||||
} else {
|
||||
redirect_policy
|
||||
},
|
||||
request_id: String::new(),
|
||||
stream_response: true,
|
||||
};
|
||||
let mut redirects = 0;
|
||||
let (response, body) = loop {
|
||||
let request_id = NEXT_OAUTH_REQUEST_ID.fetch_add(1, Ordering::Relaxed);
|
||||
params.request_id = format!("oauth-request-{request_id}");
|
||||
let (response, mut body_stream) = self
|
||||
.http_client
|
||||
.http_request_stream(params.clone())
|
||||
.await
|
||||
.map_err(oauth_http_client_error)?;
|
||||
let mut body = Vec::new();
|
||||
while let Some(chunk) = body_stream.recv().await.map_err(oauth_http_client_error)? {
|
||||
if chunk.len() > MAX_OAUTH_HTTP_RESPONSE_BODY_BYTES - body.len() {
|
||||
return Err(oauth_http_client_error(
|
||||
OAuthHttpClientAdapterError::ResponseBodyTooLarge {
|
||||
maximum_bytes: MAX_OAUTH_HTTP_RESPONSE_BODY_BYTES,
|
||||
},
|
||||
));
|
||||
}
|
||||
body.extend_from_slice(&chunk);
|
||||
}
|
||||
let Ok(status) = StatusCode::from_u16(response.status) else {
|
||||
break (response, body);
|
||||
};
|
||||
if !follow_same_origin_redirects
|
||||
|| !matches!(
|
||||
status,
|
||||
StatusCode::MOVED_PERMANENTLY
|
||||
| StatusCode::FOUND
|
||||
| StatusCode::SEE_OTHER
|
||||
| StatusCode::TEMPORARY_REDIRECT
|
||||
| StatusCode::PERMANENT_REDIRECT
|
||||
)
|
||||
{
|
||||
break (response, body);
|
||||
}
|
||||
let Some(next_url) = response
|
||||
.headers
|
||||
.iter()
|
||||
.find(|header| header.name.eq_ignore_ascii_case(LOCATION.as_str()))
|
||||
.and_then(|header| request_url.join(&header.value).ok())
|
||||
.filter(|url| url.origin() == self.resource_origin)
|
||||
else {
|
||||
break (response, body);
|
||||
};
|
||||
if redirects >= MAX_OAUTH_HTTP_REDIRECTS {
|
||||
return Err(oauth_http_client_error(
|
||||
OAuthHttpClientAdapterError::ResponseBodyTooLarge {
|
||||
maximum_bytes: MAX_OAUTH_HTTP_RESPONSE_BODY_BYTES,
|
||||
},
|
||||
OAuthHttpClientAdapterError::TooManyRedirects,
|
||||
));
|
||||
}
|
||||
body.extend_from_slice(&chunk);
|
||||
}
|
||||
if status == StatusCode::SEE_OTHER
|
||||
|| matches!(status, StatusCode::MOVED_PERMANENTLY | StatusCode::FOUND)
|
||||
&& params.method == Method::POST.as_str()
|
||||
{
|
||||
if params.method != Method::HEAD.as_str() {
|
||||
params.method = Method::GET.to_string();
|
||||
}
|
||||
params.body = None;
|
||||
params.headers.retain(|header| {
|
||||
![
|
||||
CONTENT_TYPE,
|
||||
CONTENT_LENGTH,
|
||||
CONTENT_ENCODING,
|
||||
TRANSFER_ENCODING,
|
||||
]
|
||||
.iter()
|
||||
.any(|name| header.name.eq_ignore_ascii_case(name.as_str()))
|
||||
});
|
||||
}
|
||||
params.url = next_url.to_string();
|
||||
if let Some(deadline) = deadline {
|
||||
let remaining =
|
||||
deadline
|
||||
.checked_duration_since(Instant::now())
|
||||
.ok_or_else(|| {
|
||||
oauth_http_client_error(OAuthHttpClientAdapterError::TimedOut)
|
||||
})?;
|
||||
params.timeout_ms = Some(
|
||||
u64::try_from(remaining.as_millis())
|
||||
.unwrap_or(u64::MAX)
|
||||
.max(1),
|
||||
);
|
||||
}
|
||||
request_url = next_url;
|
||||
redirects += 1;
|
||||
};
|
||||
let mut builder = oauth2::http::Response::builder().status(response.status);
|
||||
for header in response.headers {
|
||||
builder = builder.header(header.name, header.value);
|
||||
@@ -192,6 +312,10 @@ impl OAuthHttpClient for OAuthHttpClientAdapter {
|
||||
}
|
||||
}
|
||||
|
||||
#[cfg(test)]
|
||||
#[path = "oauth_http_client_security_tests.rs"]
|
||||
mod security_tests;
|
||||
|
||||
#[cfg(test)]
|
||||
mod tests {
|
||||
use http::HeaderValue;
|
||||
|
||||
@@ -0,0 +1,170 @@
|
||||
use std::collections::HashMap;
|
||||
use std::sync::Arc;
|
||||
use std::time::Duration;
|
||||
|
||||
use anyhow::Result;
|
||||
use codex_exec_server::RouteAwareHttpClient;
|
||||
use codex_http_client::HttpClientFactory;
|
||||
use codex_http_client::OutboundProxyPolicy;
|
||||
use pretty_assertions::assert_eq;
|
||||
use rmcp::transport::auth::OAuthHttpRedirectPolicy;
|
||||
use wiremock::Mock;
|
||||
use wiremock::MockServer;
|
||||
use wiremock::ResponseTemplate;
|
||||
use wiremock::matchers::header;
|
||||
use wiremock::matchers::method;
|
||||
use wiremock::matchers::path;
|
||||
|
||||
use super::MAX_OAUTH_HTTP_RESPONSE_BODY_BYTES;
|
||||
use super::OAuthHttpClientAdapter;
|
||||
use crate::http_client_adapter::StreamableHttpRedirectMode;
|
||||
use crate::utils::MCP_USER_AGENT;
|
||||
use crate::utils::build_default_headers;
|
||||
|
||||
#[tokio::test]
|
||||
async fn oauth_registration_redirects_never_forward_resource_only_headers() -> Result<()> {
|
||||
const RESOURCE_API_KEY: &str = "resource-api-key-secret";
|
||||
const RESOURCE_USER_AGENT: &str = "resource-only-user-agent";
|
||||
|
||||
for (redirect_mode, has_resource_only_headers) in [
|
||||
(StreamableHttpRedirectMode::Legacy, true),
|
||||
(StreamableHttpRedirectMode::AgentPluginV1, true),
|
||||
(StreamableHttpRedirectMode::Legacy, false),
|
||||
] {
|
||||
let resource_server = MockServer::start().await;
|
||||
let redirect_target = MockServer::start().await;
|
||||
let resource_url = format!("{}/mcp", resource_server.uri());
|
||||
|
||||
Mock::given(method("POST"))
|
||||
.and(path("/register"))
|
||||
.and(header("content-type", "application/json"))
|
||||
.and(header(
|
||||
"user-agent",
|
||||
if has_resource_only_headers {
|
||||
RESOURCE_USER_AGENT
|
||||
} else {
|
||||
MCP_USER_AGENT
|
||||
},
|
||||
))
|
||||
.respond_with(ResponseTemplate::new(307).insert_header(
|
||||
"location",
|
||||
format!("{}/redirected-register", redirect_target.uri()),
|
||||
))
|
||||
.expect(1)
|
||||
.mount(&resource_server)
|
||||
.await;
|
||||
Mock::given(method("POST"))
|
||||
.and(path("/redirected-register"))
|
||||
.and(header("content-type", "application/json"))
|
||||
.and(header("user-agent", MCP_USER_AGENT))
|
||||
.respond_with(ResponseTemplate::new(201))
|
||||
.expect(u64::from(!has_resource_only_headers))
|
||||
.mount(&redirect_target)
|
||||
.await;
|
||||
|
||||
let configured_headers = if has_resource_only_headers {
|
||||
HashMap::from([
|
||||
("X-Api-Key".to_string(), RESOURCE_API_KEY.to_string()),
|
||||
("User-Agent".to_string(), RESOURCE_USER_AGENT.to_string()),
|
||||
])
|
||||
} else {
|
||||
HashMap::from([(
|
||||
"Content-Type".to_string(),
|
||||
"resource-only-content-type".to_string(),
|
||||
)])
|
||||
};
|
||||
let adapter = OAuthHttpClientAdapter::new_with_redirect_mode(
|
||||
Arc::new(RouteAwareHttpClient::new(HttpClientFactory::new(
|
||||
OutboundProxyPolicy::ReqwestDefault,
|
||||
))),
|
||||
build_default_headers(Some(configured_headers), /*env_http_headers*/ None)?,
|
||||
&resource_url,
|
||||
/*has_configured_headers*/ true,
|
||||
redirect_mode,
|
||||
)?;
|
||||
let response = adapter
|
||||
.execute_request(
|
||||
oauth2::http::Request::builder()
|
||||
.method("POST")
|
||||
.uri(format!("{}/register", resource_server.uri()))
|
||||
.header("content-type", "application/json")
|
||||
.body(br#"{"client_name":"Codex"}"#.to_vec())?,
|
||||
OAuthHttpRedirectPolicy::Follow,
|
||||
/*timeout*/ None,
|
||||
)
|
||||
.await
|
||||
.map_err(|error| anyhow::anyhow!(error))?;
|
||||
|
||||
assert_eq!(
|
||||
response.status(),
|
||||
if has_resource_only_headers {
|
||||
oauth2::http::StatusCode::TEMPORARY_REDIRECT
|
||||
} else {
|
||||
oauth2::http::StatusCode::CREATED
|
||||
}
|
||||
);
|
||||
resource_server.verify().await;
|
||||
redirect_target.verify().await;
|
||||
}
|
||||
|
||||
Ok(())
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
async fn same_origin_redirects_preserve_timeout_and_response_body_limits() -> Result<()> {
|
||||
for oversized_redirect_body in [false, true] {
|
||||
let server = MockServer::start().await;
|
||||
let resource_url = format!("{}/mcp", server.uri());
|
||||
let redirect = ResponseTemplate::new(307).insert_header("location", "/register/");
|
||||
let redirect = if oversized_redirect_body {
|
||||
redirect.set_body_bytes(vec![0; MAX_OAUTH_HTTP_RESPONSE_BODY_BYTES + 1])
|
||||
} else {
|
||||
redirect.set_delay(Duration::from_millis(/*millis*/ 400))
|
||||
};
|
||||
Mock::given(method("POST"))
|
||||
.and(path("/register"))
|
||||
.respond_with(redirect)
|
||||
.expect(1)
|
||||
.mount(&server)
|
||||
.await;
|
||||
Mock::given(method("POST"))
|
||||
.and(path("/register/"))
|
||||
.respond_with(
|
||||
ResponseTemplate::new(201).set_delay(Duration::from_millis(/*millis*/ 400)),
|
||||
)
|
||||
.expect(u64::from(!oversized_redirect_body))
|
||||
.mount(&server)
|
||||
.await;
|
||||
|
||||
let adapter = OAuthHttpClientAdapter::new(
|
||||
Arc::new(RouteAwareHttpClient::new(HttpClientFactory::new(
|
||||
OutboundProxyPolicy::ReqwestDefault,
|
||||
))),
|
||||
build_default_headers(
|
||||
Some(HashMap::from([(
|
||||
"X-Api-Key".to_string(),
|
||||
"resource-api-key-secret".to_string(),
|
||||
)])),
|
||||
/*env_http_headers*/ None,
|
||||
)?,
|
||||
&resource_url,
|
||||
);
|
||||
let error = adapter
|
||||
.execute_request(
|
||||
oauth2::http::Request::builder()
|
||||
.method("POST")
|
||||
.uri(format!("{}/register", server.uri()))
|
||||
.body(Vec::new())?,
|
||||
OAuthHttpRedirectPolicy::Follow,
|
||||
(!oversized_redirect_body).then_some(Duration::from_millis(/*millis*/ 700)),
|
||||
)
|
||||
.await
|
||||
.expect_err("redirects must preserve request timeout and response body limits");
|
||||
if oversized_redirect_body {
|
||||
assert!(error.to_string().contains("exceeds"));
|
||||
}
|
||||
server.verify().await;
|
||||
}
|
||||
|
||||
Ok(())
|
||||
}
|
||||
@@ -548,9 +548,10 @@ impl OauthLoginFlow {
|
||||
let oauth_http_client = Arc::new(OAuthHttpClientAdapter::new_with_redirect_mode(
|
||||
http_client,
|
||||
default_headers,
|
||||
server_url,
|
||||
has_configured_headers,
|
||||
redirect_mode,
|
||||
));
|
||||
)?);
|
||||
|
||||
let scope_refs: Vec<&str> = scopes.iter().map(String::as_str).collect();
|
||||
let oauth_state = if let Some(oauth_client_id) =
|
||||
@@ -867,6 +868,7 @@ mod tests {
|
||||
OutboundProxyPolicy::ReqwestDefault,
|
||||
))),
|
||||
HeaderMap::new(),
|
||||
&format!("{base_url}/mcp"),
|
||||
)),
|
||||
scopes,
|
||||
redirect_uri,
|
||||
@@ -960,6 +962,7 @@ mod tests {
|
||||
OutboundProxyPolicy::ReqwestDefault,
|
||||
))),
|
||||
HeaderMap::new(),
|
||||
&format!("{issuer}/mcp"),
|
||||
)),
|
||||
&[],
|
||||
"http://127.0.0.1/callback",
|
||||
|
||||
@@ -1484,9 +1484,10 @@ async fn create_oauth_transport_and_runtime(
|
||||
let oauth_http_client = Arc::new(OAuthHttpClientAdapter::new_with_redirect_mode(
|
||||
http_client.clone(),
|
||||
default_headers.clone(),
|
||||
url,
|
||||
has_configured_headers,
|
||||
redirect_mode,
|
||||
));
|
||||
)?);
|
||||
let mut manager =
|
||||
AuthorizationManager::new_with_oauth_http_client(url.to_string(), oauth_http_client)
|
||||
.await?;
|
||||
|
||||
@@ -11,7 +11,7 @@ use std::collections::HashMap;
|
||||
use std::env;
|
||||
use std::ffi::OsString;
|
||||
|
||||
const MCP_USER_AGENT: &str = concat!("codex-mcp-client/", env!("CARGO_PKG_VERSION"));
|
||||
pub(crate) const MCP_USER_AGENT: &str = concat!("codex-mcp-client/", env!("CARGO_PKG_VERSION"));
|
||||
|
||||
pub(crate) fn create_env_for_mcp_server(
|
||||
extra_env: Option<HashMap<OsString, OsString>>,
|
||||
|
||||
@@ -20,6 +20,8 @@ use wiremock::matchers::path;
|
||||
|
||||
const RESOURCE_AUTHORIZATION: &str = "Bearer resource-only-secret";
|
||||
const RESOURCE_API_KEY: &str = "resource-api-key-secret";
|
||||
const RESOURCE_USER_AGENT: &str = "resource-only-user-agent";
|
||||
const MCP_USER_AGENT: &str = concat!("codex-mcp-client/", env!("CARGO_PKG_VERSION"));
|
||||
|
||||
type DiscoveryResult = anyhow::Result<Option<StreamableHttpOAuthDiscovery>>;
|
||||
|
||||
@@ -37,6 +39,7 @@ fn resource_headers() -> Option<HashMap<String, String>> {
|
||||
RESOURCE_AUTHORIZATION.to_string(),
|
||||
),
|
||||
("X-Api-Key".to_string(), RESOURCE_API_KEY.to_string()),
|
||||
("User-Agent".to_string(), RESOURCE_USER_AGENT.to_string()),
|
||||
]))
|
||||
}
|
||||
|
||||
@@ -52,6 +55,7 @@ async fn discover_legacy_oauth_without_starting_an_mcp_session(
|
||||
.and(path("/mcp"))
|
||||
.and(header("authorization", RESOURCE_AUTHORIZATION))
|
||||
.and(header("x-api-key", RESOURCE_API_KEY))
|
||||
.and(header("user-agent", RESOURCE_USER_AGENT))
|
||||
.respond_with(ResponseTemplate::new(401).insert_header(
|
||||
"www-authenticate",
|
||||
format!("Bearer resource_metadata=\"{resource_metadata_url}\""),
|
||||
@@ -71,6 +75,7 @@ async fn discover_legacy_oauth_without_starting_an_mcp_session(
|
||||
.and(path("/resource-metadata"))
|
||||
.and(header("authorization", RESOURCE_AUTHORIZATION))
|
||||
.and(header("x-api-key", RESOURCE_API_KEY))
|
||||
.and(header("user-agent", RESOURCE_USER_AGENT))
|
||||
.respond_with(ResponseTemplate::new(200).set_body_json(json!({
|
||||
"resource": resource_url,
|
||||
"authorization_servers": [authorization_server.uri()],
|
||||
@@ -112,7 +117,7 @@ async fn discover_legacy_oauth_without_starting_an_mcp_session(
|
||||
StreamableHttpRedirectMode::Legacy,
|
||||
)
|
||||
.await;
|
||||
let routed_discovery = discover_streamable_http_oauth(
|
||||
let plugin_discovery = discover_streamable_http_oauth(
|
||||
&resource_url,
|
||||
resource_headers(),
|
||||
/*env_http_headers*/ None,
|
||||
@@ -120,13 +125,30 @@ async fn discover_legacy_oauth_without_starting_an_mcp_session(
|
||||
OutboundProxyPolicy::ReqwestDefault,
|
||||
))),
|
||||
OAuthDiscoveryTimeout::LOCAL,
|
||||
StreamableHttpRedirectMode::Legacy,
|
||||
StreamableHttpRedirectMode::AgentPluginV1,
|
||||
)
|
||||
.await;
|
||||
|
||||
resource_server.verify().await;
|
||||
authorization_server.verify().await;
|
||||
Ok((local_discovery, routed_discovery))
|
||||
for request in authorization_server
|
||||
.received_requests()
|
||||
.await
|
||||
.ok_or_else(|| {
|
||||
anyhow::anyhow!("authorization-server request recording should be enabled")
|
||||
})?
|
||||
{
|
||||
assert_eq!(request.headers.get("authorization"), None);
|
||||
assert_eq!(request.headers.get("x-api-key"), None);
|
||||
assert_eq!(
|
||||
request
|
||||
.headers
|
||||
.get("user-agent")
|
||||
.map(wiremock::http::HeaderValue::as_bytes),
|
||||
Some(MCP_USER_AGENT.as_bytes())
|
||||
);
|
||||
}
|
||||
Ok((local_discovery, plugin_discovery))
|
||||
}
|
||||
|
||||
#[tokio::test]
|
||||
|
||||
@@ -1,5 +1,6 @@
|
||||
mod streamable_http_test_support;
|
||||
|
||||
use std::collections::HashMap;
|
||||
use std::time::Duration;
|
||||
use std::time::SystemTime;
|
||||
use std::time::UNIX_EPOCH;
|
||||
@@ -44,8 +45,12 @@ const SERVER_NAME: &str = "test-streamable-http-oauth-startup";
|
||||
const EXPIRED_ACCESS_TOKEN: &str = "expired-access-token";
|
||||
const REFRESH_TOKEN: &str = "valid-refresh-token";
|
||||
const REFRESHED_ACCESS_TOKEN: &str = "refreshed-access-token";
|
||||
const RESOURCE_API_KEY: &str = "resource-api-key-secret";
|
||||
const RESOURCE_USER_AGENT: &str = "resource-only-user-agent";
|
||||
const MCP_USER_AGENT: &str = concat!("codex-mcp-client/", env!("CARGO_PKG_VERSION"));
|
||||
const CHILD_SERVER_URL_ENV: &str = "MCP_TEST_OAUTH_STARTUP_SERVER_URL";
|
||||
const CHILD_HELPER_COMMAND_ENV: &str = "MCP_TEST_OAUTH_STARTUP_HELPER_COMMAND";
|
||||
const CHILD_RESOURCE_API_KEY_ENV: &str = "MCP_TEST_OAUTH_STARTUP_RESOURCE_API_KEY";
|
||||
const UNREFRESHABLE_SERVER_URL: &str = "https://unrefreshable.example/mcp";
|
||||
const UNEXPIRED_SERVER_URL: &str = "https://unexpired.example/mcp";
|
||||
const REFRESHABLE_SERVER_URL: &str = "https://refreshable.example/mcp";
|
||||
@@ -62,15 +67,14 @@ async fn refreshes_oauth_with_gateway_headers_helper() -> anyhow::Result<()> {
|
||||
|
||||
async fn assert_expired_token_refresh(with_headers_helper: bool) -> anyhow::Result<()> {
|
||||
let server = MockServer::start().await;
|
||||
let authorization_server = MockServer::start().await;
|
||||
Mock::given(method("GET"))
|
||||
.and(path("/.well-known/oauth-authorization-server/mcp"))
|
||||
.and(header(
|
||||
"user-agent",
|
||||
concat!("codex-mcp-client/", env!("CARGO_PKG_VERSION")),
|
||||
))
|
||||
.and(header("user-agent", RESOURCE_USER_AGENT))
|
||||
.and(header("x-api-key", RESOURCE_API_KEY))
|
||||
.respond_with(ResponseTemplate::new(200).set_body_json(json!({
|
||||
"authorization_endpoint": format!("{}/oauth/authorize", server.uri()),
|
||||
"token_endpoint": format!("{}/oauth/token", server.uri()),
|
||||
"authorization_endpoint": format!("{}/oauth/authorize", authorization_server.uri()),
|
||||
"token_endpoint": format!("{}/oauth/token", authorization_server.uri()),
|
||||
"scopes_supported": [""],
|
||||
})))
|
||||
.expect(1)
|
||||
@@ -78,10 +82,7 @@ async fn assert_expired_token_refresh(with_headers_helper: bool) -> anyhow::Resu
|
||||
.await;
|
||||
Mock::given(method("POST"))
|
||||
.and(path("/oauth/token"))
|
||||
.and(header(
|
||||
"user-agent",
|
||||
concat!("codex-mcp-client/", env!("CARGO_PKG_VERSION")),
|
||||
))
|
||||
.and(header("user-agent", MCP_USER_AGENT))
|
||||
.and(body_string_contains("grant_type=refresh_token"))
|
||||
.and(body_string_contains(format!(
|
||||
"refresh_token={REFRESH_TOKEN}"
|
||||
@@ -93,14 +94,12 @@ async fn assert_expired_token_refresh(with_headers_helper: bool) -> anyhow::Resu
|
||||
"refresh_token": REFRESH_TOKEN,
|
||||
})))
|
||||
.expect(1)
|
||||
.mount(&server)
|
||||
.mount(&authorization_server)
|
||||
.await;
|
||||
Mock::given(method("POST"))
|
||||
.and(path("/mcp"))
|
||||
.and(header(
|
||||
"user-agent",
|
||||
concat!("codex-mcp-client/", env!("CARGO_PKG_VERSION")),
|
||||
))
|
||||
.and(header("user-agent", RESOURCE_USER_AGENT))
|
||||
.and(header("x-api-key", RESOURCE_API_KEY))
|
||||
.and(header(
|
||||
"authorization",
|
||||
format!("Bearer {REFRESHED_ACCESS_TOKEN}"),
|
||||
@@ -148,6 +147,7 @@ async fn assert_expired_token_refresh(with_headers_helper: bool) -> anyhow::Resu
|
||||
.args(["oauth_startup_child", "--exact", "--ignored", "--nocapture"])
|
||||
.env("CODEX_HOME", codex_home.path())
|
||||
.env(CHILD_SERVER_URL_ENV, server_url)
|
||||
.env(CHILD_RESOURCE_API_KEY_ENV, RESOURCE_API_KEY)
|
||||
.env("MCP_TEST_AMBIENT_SECRET", "must-not-reach-helper");
|
||||
if with_headers_helper {
|
||||
command.env(
|
||||
@@ -169,7 +169,25 @@ async fn assert_expired_token_refresh(with_headers_helper: bool) -> anyhow::Resu
|
||||
.is_some_and(|value| value == "Bearer gateway-token")
|
||||
}));
|
||||
}
|
||||
let authorization_requests = authorization_server
|
||||
.received_requests()
|
||||
.await
|
||||
.ok_or_else(|| anyhow::anyhow!("authorization server should record requests"))?;
|
||||
assert_eq!(authorization_requests.len(), 1);
|
||||
assert_eq!(authorization_requests[0].headers.get("x-api-key"), None);
|
||||
assert_eq!(
|
||||
authorization_requests[0].headers.get("proxy-authorization"),
|
||||
None
|
||||
);
|
||||
assert_eq!(
|
||||
authorization_requests[0]
|
||||
.headers
|
||||
.get("user-agent")
|
||||
.map(http::HeaderValue::as_bytes),
|
||||
Some(MCP_USER_AGENT.as_bytes())
|
||||
);
|
||||
server.verify().await;
|
||||
authorization_server.verify().await;
|
||||
Ok(())
|
||||
}
|
||||
|
||||
@@ -383,8 +401,14 @@ async fn oauth_startup_child() -> anyhow::Result<()> {
|
||||
SERVER_NAME,
|
||||
&server_url,
|
||||
/*bearer_token*/ None,
|
||||
/*http_headers*/ None,
|
||||
/*env_http_headers*/ None,
|
||||
Some(HashMap::from([(
|
||||
"User-Agent".to_string(),
|
||||
RESOURCE_USER_AGENT.to_string(),
|
||||
)])),
|
||||
Some(HashMap::from([(
|
||||
"X-Api-Key".to_string(),
|
||||
CHILD_RESOURCE_API_KEY_ENV.to_string(),
|
||||
)])),
|
||||
OAuthCredentialsStoreMode::File,
|
||||
AuthKeyringBackendKind::default(),
|
||||
http_client,
|
||||
|
||||
Reference in New Issue
Block a user