Expand OAuth metadata redirect test coverage (#39320)

## What changed

- Cover same-origin redirects for protected-resource and authorization-server metadata.
- Verify that cross-origin redirects are rejected before contacting their targets and that redirect cycles fail within a bounded timeout.
- Exercise issuer validation and resource-header isolation for both direct and redirected metadata across legacy and Agent Plugin discovery modes.
- Reuse the test environment's local HTTP client throughout the discovery tests.

GitOrigin-RevId: bd12ac232c3b020a1c098a413361f8e205699699

codex-rs/rmcp-client/tests/mcp_2026_oauth_discovery.rs

@@ -1,9 +1,10 @@
 use std::collections::HashMap;
 use std::sync::Arc;
+use std::time::Duration;
 
-use codex_exec_server::RouteAwareHttpClient;
-use codex_http_client::HttpClientFactory;
-use codex_http_client::OutboundProxyPolicy;
+use anyhow::Context as _;
+use codex_exec_server::Environment;
+use codex_exec_server::HttpClient;
 use codex_rmcp_client::OAuthDiscoveryTimeout;
 use codex_rmcp_client::StreamableHttpOAuthDiscovery;
 use codex_rmcp_client::StreamableHttpRedirectMode;
@@ -11,6 +12,7 @@ use codex_rmcp_client::discover_streamable_http_oauth;
 use pretty_assertions::assert_eq;
 use rmcp::transport::auth::AuthError;
 use serde_json::json;
+use tokio::time::timeout;
 use wiremock::Mock;
 use wiremock::MockServer;
 use wiremock::ResponseTemplate;
@@ -22,6 +24,9 @@ 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"));
+// This is a test safety ceiling, not rmcp's private redirect limit.
+const MAX_METADATA_REDIRECT_REQUESTS: u64 = 100;
+const REDIRECT_DISCOVERY_TEST_TIMEOUT: Duration = Duration::from_secs(5);
 
 type DiscoveryResult = anyhow::Result<Option<StreamableHttpOAuthDiscovery>>;
 
@@ -32,6 +37,12 @@ enum AuthorizationMetadataIssuer {
     Mismatched,
 }
 
+#[derive(Clone, Copy)]
+enum MetadataDelivery {
+    Direct,
+    SameOriginRedirects,
+}
+
 fn resource_headers() -> Option<HashMap<String, String>> {
     Some(HashMap::from([
         (
@@ -43,9 +54,73 @@ fn resource_headers() -> Option<HashMap<String, String>> {
     ]))
 }
 
-async fn discover_legacy_oauth_without_starting_an_mcp_session(
+fn local_http_client() -> Arc<dyn HttpClient> {
+    Environment::default_for_tests().get_http_client()
+}
+
+async fn discover_with_local_http_client(
+    resource_url: &str,
+    redirect_mode: StreamableHttpRedirectMode,
+) -> DiscoveryResult {
+    discover_streamable_http_oauth(
+        resource_url,
+        resource_headers(),
+        /*env_http_headers*/ None,
+        local_http_client(),
+        OAuthDiscoveryTimeout::LOCAL,
+        redirect_mode,
+    )
+    .await
+}
+
+async fn assert_authorization_requests_exclude_resource_headers(
+    authorization_server: &MockServer,
+) -> anyhow::Result<()> {
+    let requests = authorization_server
+        .received_requests()
+        .await
+        .context("authorization-server request recording should be enabled")?;
+    assert!(
+        !requests.is_empty(),
+        "OAuth discovery must contact the authorization server"
+    );
+    for request in requests {
+        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(())
+}
+
+fn assert_cross_origin_redirect_rejected(
+    discovery: DiscoveryResult,
+    redirect_target: &str,
+) -> anyhow::Result<()> {
+    let error = discovery
+        .err()
+        .context("cross-origin OAuth metadata redirects must be rejected")?;
+    assert!(
+        matches!(
+            error.downcast_ref::<AuthError>(),
+            Some(AuthError::MetadataError(reason))
+                if reason.contains("OAuth discovery redirect to non-same-origin URL rejected")
+                    && reason.contains(redirect_target)
+        ),
+        "expected the cross-origin redirect rejection for `{redirect_target}`: {error:#}",
+    );
+    Ok(())
+}
+
+async fn assert_legacy_oauth_without_starting_an_mcp_session(
     metadata_issuer: AuthorizationMetadataIssuer,
-) -> anyhow::Result<(DiscoveryResult, DiscoveryResult)> {
+    metadata_delivery: MetadataDelivery,
+) -> anyhow::Result<()> {
     let resource_server = MockServer::start().await;
     let authorization_server = MockServer::start().await;
     let resource_url = format!("{}/mcp", resource_server.uri());
@@ -71,8 +146,42 @@ async fn discover_legacy_oauth_without_starting_an_mcp_session(
         .mount(&resource_server)
         .await;
 
+    let (resource_metadata_path, authorization_metadata_path) = match metadata_delivery {
+        MetadataDelivery::Direct => (
+            "/resource-metadata",
+            "/.well-known/oauth-authorization-server",
+        ),
+        MetadataDelivery::SameOriginRedirects => {
+            Mock::given(method("GET"))
+                .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(302)
+                        .insert_header("location", "/redirected-resource-metadata"),
+                )
+                .expect(2)
+                .mount(&resource_server)
+                .await;
+            Mock::given(method("GET"))
+                .and(path("/.well-known/oauth-authorization-server"))
+                .respond_with(
+                    ResponseTemplate::new(302)
+                        .insert_header("location", "/redirected-authorization-metadata"),
+                )
+                .expect(2)
+                .mount(&authorization_server)
+                .await;
+            (
+                "/redirected-resource-metadata",
+                "/redirected-authorization-metadata",
+            )
+        }
+    };
+
     Mock::given(method("GET"))
-        .and(path("/resource-metadata"))
+        .and(path(resource_metadata_path))
         .and(header("authorization", RESOURCE_AUTHORIZATION))
         .and(header("x-api-key", RESOURCE_API_KEY))
         .and(header("user-agent", RESOURCE_USER_AGENT))
@@ -100,110 +209,294 @@ async fn discover_legacy_oauth_without_starting_an_mcp_session(
         }
     }
     Mock::given(method("GET"))
-        .and(path("/.well-known/oauth-authorization-server"))
+        .and(path(authorization_metadata_path))
         .respond_with(ResponseTemplate::new(200).set_body_json(metadata))
         .expect(2)
         .mount(&authorization_server)
         .await;
 
-    let local_discovery = discover_streamable_http_oauth(
-        &resource_url,
-        resource_headers(),
-        /*env_http_headers*/ None,
-        Arc::new(RouteAwareHttpClient::new(HttpClientFactory::new(
-            OutboundProxyPolicy::ReqwestDefault,
-        ))),
-        OAuthDiscoveryTimeout::LOCAL,
+    for redirect_mode in [
         StreamableHttpRedirectMode::Legacy,
-    )
-    .await;
-    let plugin_discovery = discover_streamable_http_oauth(
-        &resource_url,
-        resource_headers(),
-        /*env_http_headers*/ None,
-        Arc::new(RouteAwareHttpClient::new(HttpClientFactory::new(
-            OutboundProxyPolicy::ReqwestDefault,
-        ))),
-        OAuthDiscoveryTimeout::LOCAL,
         StreamableHttpRedirectMode::AgentPluginV1,
-    )
-    .await;
+    ] {
+        let discovery = discover_with_local_http_client(&resource_url, redirect_mode).await;
+        match metadata_issuer {
+            AuthorizationMetadataIssuer::Matching | AuthorizationMetadataIssuer::Missing => {
+                assert_eq!(
+                    discovery?,
+                    Some(StreamableHttpOAuthDiscovery {
+                        scopes_supported: Some(vec!["mcp:read".to_string()]),
+                    }),
+                );
+            }
+            AuthorizationMetadataIssuer::Mismatched => {
+                let error = discovery
+                    .err()
+                    .context("a mismatched issuer must not be accepted")?;
+                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"
+                    ),
+                    "expected the original authorization-server issuer to remain bound: {error:#}",
+                );
+            }
+        }
+    }
 
     resource_server.verify().await;
     authorization_server.verify().await;
-    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))
+    assert_authorization_requests_exclude_resource_headers(&authorization_server).await?;
+    Ok(())
 }
 
 #[tokio::test]
 async fn oauth_discovery_uses_get_first_without_starting_a_legacy_mcp_session() -> anyhow::Result<()>
 {
-    let discoveries = discover_legacy_oauth_without_starting_an_mcp_session(
+    assert_legacy_oauth_without_starting_an_mcp_session(
         AuthorizationMetadataIssuer::Matching,
+        MetadataDelivery::Direct,
     )
-    .await?;
+    .await
+}
 
-    for discovery in [discoveries.0, discoveries.1] {
-        assert_eq!(
-            discovery?,
-            Some(StreamableHttpOAuthDiscovery {
-                scopes_supported: Some(vec!["mcp:read".to_string()]),
-            }),
-        );
-    }
+#[tokio::test]
+async fn legacy_oauth_discovery_follows_same_origin_metadata_redirects() -> anyhow::Result<()> {
+    assert_legacy_oauth_without_starting_an_mcp_session(
+        AuthorizationMetadataIssuer::Matching,
+        MetadataDelivery::SameOriginRedirects,
+    )
+    .await
+}
+
+#[tokio::test]
+async fn legacy_oauth_discovery_rejects_cross_origin_authorization_metadata_redirects()
+-> anyhow::Result<()> {
+    let resource_server = MockServer::start().await;
+    let authorization_server = MockServer::start().await;
+    let redirect_target = MockServer::start().await;
+    let resource_url = format!("{}/mcp", resource_server.uri());
+    let resource_metadata_url = format!("{}/resource-metadata", resource_server.uri());
+
+    Mock::given(method("GET"))
+        .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}\""),
+        ))
+        .expect(1)
+        .mount(&resource_server)
+        .await;
+
+    Mock::given(method("GET"))
+        .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()],
+        })))
+        .expect(1)
+        .mount(&resource_server)
+        .await;
+
+    Mock::given(method("GET"))
+        .and(path("/.well-known/oauth-authorization-server"))
+        .respond_with(ResponseTemplate::new(302).insert_header(
+            "location",
+            format!(
+                "{}/redirected-authorization-metadata",
+                redirect_target.uri()
+            ),
+        ))
+        .expect(1)
+        .mount(&authorization_server)
+        .await;
+
+    Mock::given(method("GET"))
+        .respond_with(ResponseTemplate::new(200))
+        .expect(0)
+        .mount(&redirect_target)
+        .await;
+
+    let discovery =
+        discover_with_local_http_client(&resource_url, StreamableHttpRedirectMode::Legacy).await;
+
+    assert_cross_origin_redirect_rejected(discovery, &redirect_target.uri())?;
+    assert!(
+        redirect_target
+            .received_requests()
+            .await
+            .context("cross-origin request recording should be enabled")?
+            .is_empty(),
+        "OAuth authorization-server metadata discovery must not contact a cross-origin redirect target",
+    );
+    redirect_target.verify().await;
+    authorization_server.verify().await;
+    resource_server.verify().await;
+    assert_authorization_requests_exclude_resource_headers(&authorization_server).await?;
+    Ok(())
+}
+
+#[tokio::test]
+async fn legacy_oauth_discovery_rejects_authorization_metadata_redirect_cycles()
+-> anyhow::Result<()> {
+    let resource_server = MockServer::start().await;
+    let authorization_server = MockServer::start().await;
+    let resource_url = format!("{}/mcp", resource_server.uri());
+    let resource_metadata_url = format!("{}/resource-metadata", resource_server.uri());
+
+    Mock::given(method("GET"))
+        .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}\""),
+        ))
+        .expect(1)
+        .mount(&resource_server)
+        .await;
+
+    Mock::given(method("GET"))
+        .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()],
+        })))
+        .expect(1)
+        .mount(&resource_server)
+        .await;
+
+    Mock::given(method("GET"))
+        .and(path("/.well-known/oauth-authorization-server"))
+        .respond_with(
+            ResponseTemplate::new(302)
+                .insert_header("location", "/.well-known/oauth-authorization-server"),
+        )
+        .expect(2..=MAX_METADATA_REDIRECT_REQUESTS)
+        .mount(&authorization_server)
+        .await;
+
+    let discovery = timeout(
+        REDIRECT_DISCOVERY_TEST_TIMEOUT,
+        discover_with_local_http_client(&resource_url, StreamableHttpRedirectMode::Legacy),
+    )
+    .await
+    .context("OAuth metadata redirect cycles must fail within the bounded discovery timeout")?;
+    let error = discovery
+        .err()
+        .context("OAuth metadata redirect cycles must be rejected")?;
+    assert!(
+        matches!(
+            error.downcast_ref::<AuthError>(),
+            Some(AuthError::MetadataError(reason))
+                if reason.contains("OAuth discovery exceeded ") && reason.contains(" redirects")
+        ),
+        "expected the SDK to report its bounded OAuth discovery redirect limit: {error:#}",
+    );
+    authorization_server.verify().await;
+    resource_server.verify().await;
+    assert_authorization_requests_exclude_resource_headers(&authorization_server).await?;
+    Ok(())
+}
+
+#[tokio::test]
+async fn legacy_oauth_discovery_rejects_cross_origin_resource_metadata_redirects()
+-> anyhow::Result<()> {
+    let resource_server = MockServer::start().await;
+    let redirect_target = MockServer::start().await;
+    let resource_url = format!("{}/mcp", resource_server.uri());
+    let resource_metadata_url = format!("{}/resource-metadata", resource_server.uri());
+
+    Mock::given(method("GET"))
+        .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}\""),
+        ))
+        .expect(1)
+        .mount(&resource_server)
+        .await;
+
+    Mock::given(method("GET"))
+        .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(302).insert_header(
+            "location",
+            format!("{}/redirect-target", redirect_target.uri()),
+        ))
+        .expect(1)
+        .mount(&resource_server)
+        .await;
+
+    Mock::given(method("GET"))
+        .respond_with(ResponseTemplate::new(200))
+        .expect(0)
+        .mount(&redirect_target)
+        .await;
+
+    let discovery =
+        discover_with_local_http_client(&resource_url, StreamableHttpRedirectMode::Legacy).await;
+
+    assert_cross_origin_redirect_rejected(discovery, &redirect_target.uri())?;
+    assert!(
+        redirect_target
+            .received_requests()
+            .await
+            .context("cross-origin request recording should be enabled")?
+            .is_empty(),
+        "OAuth discovery must not contact a cross-origin redirect target",
+    );
+    redirect_target.verify().await;
+    resource_server.verify().await;
     Ok(())
 }
 
 #[tokio::test]
 async fn legacy_oauth_discovery_accepts_authorization_metadata_without_an_issuer()
 -> anyhow::Result<()> {
-    let discoveries =
-        discover_legacy_oauth_without_starting_an_mcp_session(AuthorizationMetadataIssuer::Missing)
-            .await?;
-
-    for discovery in [discoveries.0, discoveries.1] {
-        assert_eq!(
-            discovery?,
-            Some(StreamableHttpOAuthDiscovery {
-                scopes_supported: Some(vec!["mcp:read".to_string()]),
-            }),
-        );
+    for metadata_delivery in [
+        MetadataDelivery::Direct,
+        MetadataDelivery::SameOriginRedirects,
+    ] {
+        assert_legacy_oauth_without_starting_an_mcp_session(
+            AuthorizationMetadataIssuer::Missing,
+            metadata_delivery,
+        )
+        .await?;
     }
     Ok(())
 }
 
 #[tokio::test]
 async fn legacy_oauth_discovery_rejects_an_explicit_mismatched_issuer() -> anyhow::Result<()> {
-    let discoveries = discover_legacy_oauth_without_starting_an_mcp_session(
-        AuthorizationMetadataIssuer::Mismatched,
-    )
-    .await?;
-
-    for discovery in [discoveries.0, discoveries.1] {
-        let error = discovery.expect_err("a mismatched issuer must not be accepted");
-        assert!(
-            matches!(
-                error.downcast_ref::<AuthError>(),
-                Some(AuthError::AuthorizationServerMismatch { .. }),
-            ),
-            "expected an authorization-server issuer mismatch: {error:#}",
-        );
+    for metadata_delivery in [
+        MetadataDelivery::Direct,
+        MetadataDelivery::SameOriginRedirects,
+    ] {
+        assert_legacy_oauth_without_starting_an_mcp_session(
+            AuthorizationMetadataIssuer::Mismatched,
+            metadata_delivery,
+        )
+        .await?;
     }
     Ok(())
 }
@@ -214,30 +507,16 @@ async fn oauth_discovery_does_not_invent_support_for_an_unauthenticated_legacy_s
     let resource_server = MockServer::start().await;
 
     let server_url = format!("{}/mcp", resource_server.uri());
-    let executor_discovery = discover_streamable_http_oauth(
-        &server_url,
-        /*http_headers*/ None,
-        /*env_http_headers*/ None,
-        Arc::new(RouteAwareHttpClient::new(HttpClientFactory::new(
-            OutboundProxyPolicy::ReqwestDefault,
-        ))),
-        OAuthDiscoveryTimeout::LOCAL,
-        StreamableHttpRedirectMode::Legacy,
-    )
-    .await?;
     let local_discovery = discover_streamable_http_oauth(
         &server_url,
         /*http_headers*/ None,
         /*env_http_headers*/ None,
-        Arc::new(RouteAwareHttpClient::new(HttpClientFactory::new(
-            OutboundProxyPolicy::ReqwestDefault,
-        ))),
+        local_http_client(),
         OAuthDiscoveryTimeout::LOCAL,
         StreamableHttpRedirectMode::Legacy,
     )
     .await?;
 
-    assert_eq!(executor_discovery, None);
     assert_eq!(local_discovery, None);
     Ok(())
 }