Add route-aware redirect test coverage (#34481)

## What changed

- Cover redirect method and body handling, `Referer` behavior, and credential
  stripping across origins.
- Exercise request deadlines across route selection and redirect hops, along
  with redirect limits and unsupported schemes.
- Verify that disabling pool request logging does not expose request or
  response data.

GitOrigin-RevId: a683956c468b45304d0bdc25081f5f4e03836ccd

codex-rs/http-client/src/route_aware_client_pool_tests.rs

@@ -11,6 +11,8 @@ use std::time::Duration;
 use std::time::Instant;
 
 use pretty_assertions::assert_eq;
+use tracing_subscriber::Layer;
+use tracing_subscriber::layer::SubscriberExt;
 
 use super::*;
 use crate::OutboundProxyPolicy;
@@ -210,6 +212,213 @@ async fn bounds_cached_routes_and_rebuilds_an_evicted_route() {
     );
 }
 
+#[tokio::test]
+async fn request_timeout_covers_route_selection() {
+    let pool = manual_redirect_pool();
+    let mut request = reqwest::Request::new(
+        Method::GET,
+        reqwest::Url::parse("http://route-selection-timeout.test/start")
+            .expect("request URL should parse"),
+    );
+    *request.timeout_mut() = Some(Duration::from_millis(10));
+    let resolver_calls = Arc::new(AtomicUsize::new(0));
+    let observed_resolver_calls = Arc::clone(&resolver_calls);
+
+    let error = pool
+        .send_with_resolver(request, move |_| {
+            observed_resolver_calls.fetch_add(1, Ordering::SeqCst);
+            async {
+                tokio::time::sleep(Duration::from_millis(100)).await;
+                Ok(OutboundProxyRoute::Direct)
+            }
+        })
+        .await
+        .expect_err("request should time out during route selection");
+
+    assert!(matches!(error, RouteAwareRequestError::Timeout));
+    assert_eq!(resolver_calls.load(Ordering::SeqCst), 1);
+}
+
+#[tokio::test]
+async fn request_timeout_is_shared_across_redirect_hops() {
+    let (address, server) = spawn_response_server(vec![
+        "HTTP/1.1 302 Found\r\nLocation: /final\r\nContent-Length: 0\r\nConnection: close\r\n\r\n"
+            .to_string(),
+    ]);
+    let pool = manual_redirect_pool();
+    let mut request = reqwest::Request::new(
+        Method::GET,
+        reqwest::Url::parse(&format!("http://{address}/start")).expect("request URL should parse"),
+    );
+    *request.timeout_mut() = Some(Duration::from_secs(2));
+    let resolver_calls = Arc::new(AtomicUsize::new(0));
+    let observed_resolver_calls = Arc::clone(&resolver_calls);
+
+    let error = pool
+        .send_with_resolver(request, move |_| {
+            let resolver_call = observed_resolver_calls.fetch_add(1, Ordering::SeqCst);
+            async move {
+                let delay = if resolver_call == 0 {
+                    Duration::from_millis(500)
+                } else {
+                    Duration::from_millis(1_750)
+                };
+                tokio::time::sleep(delay).await;
+                Ok(OutboundProxyRoute::Direct)
+            }
+        })
+        .await
+        .expect_err("redirect chain should exceed its shared timeout");
+
+    assert!(matches!(error, RouteAwareRequestError::Timeout));
+    assert_eq!(resolver_calls.load(Ordering::SeqCst), 2);
+    assert_eq!(
+        server.join().expect("redirect server should finish").len(),
+        1
+    );
+}
+
+#[tokio::test]
+async fn rejects_replayable_redirect_to_unsupported_scheme() {
+    let (address, server) = spawn_response_server(vec![
+        "HTTP/1.1 307 Temporary Redirect\r\nLocation: ftp://example.com/final\r\nContent-Length: 0\r\nConnection: close\r\n\r\n"
+            .to_string(),
+    ]);
+    let pool = manual_redirect_pool();
+    let request = reqwest::Request::new(
+        Method::GET,
+        reqwest::Url::parse(&format!("http://{address}/start")).expect("request URL should parse"),
+    );
+
+    let error = pool
+        .send_with_resolver(request, |_| async { Ok(OutboundProxyRoute::Direct) })
+        .await
+        .expect_err("unsupported redirect scheme should fail");
+
+    assert!(matches!(
+        error,
+        RouteAwareRequestError::UnsupportedRedirectScheme(scheme) if scheme == "ftp"
+    ));
+    assert_eq!(
+        server.join().expect("redirect server should finish").len(),
+        1
+    );
+}
+
+#[tokio::test]
+async fn rejects_redirects_beyond_the_limit() {
+    let responses = (0..=MAX_REDIRECTS)
+        .map(|redirect| {
+            format!(
+                "HTTP/1.1 302 Found\r\nLocation: /hop/{}\r\nContent-Length: 0\r\nConnection: close\r\n\r\n",
+                redirect + 1
+            )
+        })
+        .collect();
+    let (address, server) = spawn_response_server(responses);
+    let pool = manual_redirect_pool();
+    let request = reqwest::Request::new(
+        Method::GET,
+        reqwest::Url::parse(&format!("http://{address}/start")).expect("request URL should parse"),
+    );
+
+    let error = pool
+        .send_with_resolver(request, |_| async { Ok(OutboundProxyRoute::Direct) })
+        .await
+        .expect_err("redirect chain should stop at the limit");
+    let requests = server.join().expect("redirect server should finish");
+
+    assert!(matches!(error, RouteAwareRequestError::TooManyRedirects));
+    assert_eq!(requests.len(), MAX_REDIRECTS + 1);
+    assert_eq!(
+        requests.last().and_then(|request| request.lines().next()),
+        Some("GET /hop/10 HTTP/1.1")
+    );
+}
+
+#[tokio::test]
+async fn disabled_pool_logging_does_not_expose_request_or_response_data() {
+    let (address, server) = spawn_response_server(vec![
+        "HTTP/1.1 200 OK\r\nx-sensitive-response: response-secret-value\r\nContent-Length: 2\r\nConnection: close\r\n\r\nok"
+            .to_string(),
+    ]);
+    let pool = RouteAwareClientPool::with_builder_factory(
+        HttpClientFactory::new(OutboundProxyPolicy::RespectSystemProxy),
+        ClientRouteClass::Api,
+        || reqwest::Client::builder().no_proxy(),
+        PoolRequestLogging::Disabled,
+    );
+    let buffer = Arc::new(Mutex::new(Vec::new()));
+    let subscriber = tracing_subscriber::registry().with(
+        tracing_subscriber::fmt::layer()
+            .with_ansi(false)
+            .with_writer(TestLogWriter {
+                buffer: Arc::clone(&buffer),
+            })
+            .with_filter(
+                tracing_subscriber::filter::Targets::new()
+                    .with_target("codex_http_client", tracing::Level::TRACE),
+            ),
+    );
+    let _guard = tracing::subscriber::set_default(subscriber);
+    tracing::debug!(target: "codex_http_client", "log capture sentinel");
+    let request_url = format!(
+        "http://auth-user:password-secret-value@{address}/token?client_secret=query-secret-value"
+    );
+    let mut request = reqwest::Request::new(
+        Method::POST,
+        reqwest::Url::parse(&request_url).expect("request URL should parse"),
+    );
+    request.headers_mut().insert(
+        "x-sensitive-request",
+        HeaderValue::from_static("request-header-secret-value"),
+    );
+    *request.body_mut() = Some("request-body-secret-value".into());
+    *request.timeout_mut() = Some(Duration::from_secs(2));
+
+    let response = pool
+        .send_with_resolver(request, |_| async { Ok(OutboundProxyRoute::Direct) })
+        .await
+        .expect("route-aware request should succeed");
+    assert_eq!(response.status(), StatusCode::OK);
+    server.join().expect("server thread should finish");
+
+    let unresponsive_listener =
+        TcpListener::bind(("127.0.0.1", 0)).expect("unresponsive listener should bind");
+    let unresponsive_address = unresponsive_listener
+        .local_addr()
+        .expect("unresponsive listener should have an address");
+    let failure_url = format!(
+        "http://auth-user:failure-password-secret-value@{unresponsive_address}/token?client_secret=failure-query-secret-value"
+    );
+    let mut request = reqwest::Request::new(
+        Method::POST,
+        reqwest::Url::parse(&failure_url).expect("failure URL should parse"),
+    );
+    *request.timeout_mut() = Some(Duration::from_millis(100));
+
+    let error = pool
+        .send_with_resolver(request, |_| async { Ok(OutboundProxyRoute::Direct) })
+        .await
+        .expect_err("request to an unresponsive listener should time out");
+    assert!(error.is_timeout());
+
+    let logs = String::from_utf8(buffer.lock().expect("log buffer lock").clone())
+        .expect("logs should be UTF-8");
+    assert!(logs.contains("log capture sentinel"));
+    for secret in [
+        "password-secret-value",
+        "query-secret-value",
+        "request-header-secret-value",
+        "request-body-secret-value",
+        "response-secret-value",
+        "failure-password-secret-value",
+        "failure-query-secret-value",
+    ] {
+        assert!(!logs.contains(secret), "logs exposed {secret}:\n{logs}");
+    }
+}
+
 #[derive(Clone)]
 struct FakeRouteResolver {
     routes: Arc<HashMap<String, OutboundProxyRoute>>,
@@ -256,3 +465,115 @@ async fn resolve_with(
         .await?;
     Ok(client)
 }
+
+fn manual_redirect_pool() -> RouteAwareClientPool {
+    RouteAwareClientPool::with_builder_factory(
+        HttpClientFactory::new(OutboundProxyPolicy::RespectSystemProxy),
+        ClientRouteClass::Api,
+        || reqwest::Client::builder().no_proxy(),
+        PoolRequestLogging::Enabled,
+    )
+}
+
+fn spawn_response_server(
+    responses: Vec<String>,
+) -> (std::net::SocketAddr, std::thread::JoinHandle<Vec<String>>) {
+    let listener = TcpListener::bind(("127.0.0.1", 0)).expect("response listener should bind");
+    let address = listener
+        .local_addr()
+        .expect("response listener should have an address");
+    listener
+        .set_nonblocking(true)
+        .expect("response listener should become nonblocking");
+    let server = std::thread::spawn(move || {
+        let mut requests = Vec::new();
+        for response in responses {
+            let deadline = Instant::now() + Duration::from_secs(2);
+            let (mut stream, _) = loop {
+                match listener.accept() {
+                    Ok(connection) => break connection,
+                    Err(error) if error.kind() == io::ErrorKind::WouldBlock => {
+                        assert!(
+                            Instant::now() < deadline,
+                            "response server should receive the next request"
+                        );
+                        std::thread::sleep(Duration::from_millis(10));
+                    }
+                    Err(error) => panic!("response server should accept: {error}"),
+                }
+            };
+            stream
+                .set_read_timeout(Some(Duration::from_secs(2)))
+                .expect("response stream should get a read timeout");
+            requests.push(read_http_message(&mut stream));
+            stream
+                .write_all(response.as_bytes())
+                .expect("response server should write response");
+        }
+        requests
+    });
+    (address, server)
+}
+
+fn read_http_message(stream: &mut impl Read) -> String {
+    let mut buffer = Vec::new();
+    let mut chunk = [0_u8; 1024];
+    loop {
+        let bytes_read = stream.read(&mut chunk).expect("HTTP message should read");
+        if bytes_read == 0 {
+            break;
+        }
+        buffer.extend_from_slice(&chunk[..bytes_read]);
+        if let Some(header_end) = buffer.windows(4).position(|window| window == b"\r\n\r\n") {
+            let body_start = header_end + 4;
+            let headers = String::from_utf8_lossy(&buffer[..body_start]);
+            let content_length = headers
+                .lines()
+                .filter_map(|line| line.split_once(':'))
+                .find_map(|(name, value)| {
+                    name.eq_ignore_ascii_case("content-length")
+                        .then(|| value.trim().parse::<usize>().ok())
+                        .flatten()
+                })
+                .unwrap_or(0);
+            if buffer.len() >= body_start + content_length {
+                break;
+            }
+        }
+    }
+    String::from_utf8_lossy(&buffer).into_owned()
+}
+
+#[derive(Clone)]
+struct TestLogWriter {
+    buffer: Arc<Mutex<Vec<u8>>>,
+}
+
+struct TestLogSink {
+    buffer: Arc<Mutex<Vec<u8>>>,
+}
+
+impl<'a> tracing_subscriber::fmt::MakeWriter<'a> for TestLogWriter {
+    type Writer = TestLogSink;
+
+    fn make_writer(&'a self) -> Self::Writer {
+        TestLogSink {
+            buffer: Arc::clone(&self.buffer),
+        }
+    }
+}
+
+impl Write for TestLogSink {
+    fn write(&mut self, buffer: &[u8]) -> io::Result<usize> {
+        let mut log_buffer = self
+            .buffer
+            .lock()
+            .map_err(|_| io::Error::other("log buffer lock was poisoned"))?;
+        log_buffer.extend(buffer);
+        Ok(buffer.len())
+    }
+
+    fn flush(&mut self) -> io::Result<()> {
+        Ok(())
+    }
+}

codex-rs/http-client/src/route_aware_redirect.rs

@@ -136,3 +136,7 @@ fn same_origin(previous: &reqwest::Url, next: &reqwest::Url) -> bool {
         && previous.host_str() == next.host_str()
         && previous.port_or_known_default() == next.port_or_known_default()
 }
+
+#[cfg(test)]
+#[path = "route_aware_redirect_tests.rs"]
+mod tests;

codex-rs/http-client/src/route_aware_redirect_tests.rs

@@ -0,0 +1,233 @@
+use http::HeaderValue;
+use http::header::CONTENT_ENCODING;
+use http::header::CONTENT_LENGTH;
+use http::header::CONTENT_TYPE;
+use http::header::COOKIE;
+use http::header::REFERER;
+use http::header::TRANSFER_ENCODING;
+use pretty_assertions::assert_eq;
+
+use super::*;
+
+#[derive(Clone, Copy, Debug)]
+enum RedirectBodyBehavior {
+    Drop,
+    Preserve,
+}
+
+#[test]
+fn redirects_match_reqwest_method_and_body_rules() {
+    let original_url =
+        reqwest::Url::parse("https://example.com/original").expect("original URL should parse");
+    let redirected_url =
+        reqwest::Url::parse("https://example.com/next").expect("redirect URL should parse");
+
+    for (status, method, expected_method, body_behavior) in [
+        (
+            StatusCode::MOVED_PERMANENTLY,
+            Method::POST,
+            Method::GET,
+            RedirectBodyBehavior::Drop,
+        ),
+        (
+            StatusCode::FOUND,
+            Method::POST,
+            Method::GET,
+            RedirectBodyBehavior::Drop,
+        ),
+        (
+            StatusCode::SEE_OTHER,
+            Method::POST,
+            Method::GET,
+            RedirectBodyBehavior::Drop,
+        ),
+        (
+            StatusCode::SEE_OTHER,
+            Method::HEAD,
+            Method::HEAD,
+            RedirectBodyBehavior::Drop,
+        ),
+        (
+            StatusCode::MOVED_PERMANENTLY,
+            Method::PUT,
+            Method::PUT,
+            RedirectBodyBehavior::Preserve,
+        ),
+        (
+            StatusCode::FOUND,
+            Method::PUT,
+            Method::PUT,
+            RedirectBodyBehavior::Preserve,
+        ),
+        (
+            StatusCode::TEMPORARY_REDIRECT,
+            Method::POST,
+            Method::POST,
+            RedirectBodyBehavior::Preserve,
+        ),
+        (
+            StatusCode::PERMANENT_REDIRECT,
+            Method::POST,
+            Method::POST,
+            RedirectBodyBehavior::Preserve,
+        ),
+    ] {
+        let mut original = reqwest::Request::new(method.clone(), original_url.clone());
+        *original.headers_mut() = HeaderMap::from_iter([
+            (CONTENT_TYPE, HeaderValue::from_static("application/json")),
+            (CONTENT_LENGTH, HeaderValue::from_static("2")),
+            (CONTENT_ENCODING, HeaderValue::from_static("identity")),
+            (TRANSFER_ENCODING, HeaderValue::from_static("chunked")),
+            (
+                http::header::ACCEPT,
+                HeaderValue::from_static("application/json"),
+            ),
+        ]);
+        *original.version_mut() = http::Version::HTTP_2;
+        *original.timeout_mut() = Some(Duration::from_secs(7));
+        *original.body_mut() = Some("{}".into());
+        let mut expected_headers = original.headers().clone();
+        let expected_body = match body_behavior {
+            RedirectBodyBehavior::Drop => {
+                for header in [
+                    CONTENT_TYPE,
+                    CONTENT_LENGTH,
+                    CONTENT_ENCODING,
+                    TRANSFER_ENCODING,
+                ] {
+                    expected_headers.remove(header);
+                }
+                None
+            }
+            RedirectBodyBehavior::Preserve => Some(b"{}".to_vec()),
+        };
+
+        let redirected = redirect_request(
+            status,
+            original.method().clone(),
+            original.headers().clone(),
+            original.version(),
+            original.timeout().copied(),
+            original.try_clone(),
+            redirected_url.clone(),
+        )
+        .expect("supported redirect should be followed");
+
+        assert_eq!(
+            (
+                redirected.method().clone(),
+                redirected.url().clone(),
+                redirected.headers().clone(),
+                redirected.version(),
+                redirected.timeout().copied(),
+                redirected
+                    .body()
+                    .and_then(reqwest::Body::as_bytes)
+                    .map(<[u8]>::to_vec),
+            ),
+            (
+                expected_method,
+                redirected_url.clone(),
+                expected_headers,
+                http::Version::HTTP_2,
+                Some(Duration::from_secs(7)),
+                expected_body,
+            ),
+            "redirect behavior for {status} {method}",
+        );
+    }
+}
+
+#[test]
+fn redirect_referer_follows_strict_origin_when_cross_origin() {
+    let previous =
+        reqwest::Url::parse("https://user:password@example.com/start?access_token=secret#fragment")
+            .expect("valid URL");
+
+    let mut same_origin_headers = HeaderMap::new();
+    let same_origin = reqwest::Url::parse("https://example.com/next").expect("valid URL");
+    insert_referer(&mut same_origin_headers, &previous, &same_origin);
+    assert_eq!(
+        same_origin_headers.get(REFERER),
+        Some(&HeaderValue::from_static(
+            "https://example.com/start?access_token=secret"
+        ))
+    );
+
+    let mut cross_origin_headers = HeaderMap::new();
+    let cross_origin = reqwest::Url::parse("https://other.example/next").expect("valid URL");
+    insert_referer(&mut cross_origin_headers, &previous, &cross_origin);
+    assert_eq!(
+        cross_origin_headers.get(REFERER),
+        Some(&HeaderValue::from_static("https://example.com/"))
+    );
+
+    let mut downgrade_headers = HeaderMap::from_iter([(
+        REFERER,
+        HeaderValue::from_static("https://stale.example/path?secret=value"),
+    )]);
+    let downgrade = reqwest::Url::parse("http://other.example/next").expect("valid URL");
+    insert_referer(&mut downgrade_headers, &previous, &downgrade);
+    assert_eq!(downgrade_headers.get(REFERER), None);
+}
+
+#[test]
+fn non_replayable_redirect_is_not_followed_regardless_of_target_scheme() {
+    let next_url = reqwest::Url::parse("ftp://example.com/next").expect("valid URL");
+
+    let redirected = redirect_request(
+        StatusCode::TEMPORARY_REDIRECT,
+        Method::POST,
+        HeaderMap::new(),
+        http::Version::HTTP_11,
+        /*timeout*/ None,
+        /*replay*/ None,
+        next_url,
+    );
+
+    assert!(redirected.is_none());
+}
+
+#[test]
+fn redirect_credentials_are_retained_only_for_the_same_origin() {
+    for (previous, next, retain_credentials) in [
+        (
+            "https://example.com:8080/start",
+            "https://example.com:8080/next",
+            true,
+        ),
+        (
+            "https://example.com:8080/start",
+            "http://example.com:8080/next",
+            false,
+        ),
+        (
+            "https://example.com:8080/start",
+            "https://other.example:8080/next",
+            false,
+        ),
+        (
+            "https://example.com:8080/start",
+            "https://example.com:8081/next",
+            false,
+        ),
+    ] {
+        let previous = reqwest::Url::parse(previous).expect("previous URL should parse");
+        let next = reqwest::Url::parse(next).expect("next URL should parse");
+        let mut headers = HeaderMap::from_iter([
+            (AUTHORIZATION, HeaderValue::from_static("Bearer secret")),
+            (COOKIE, HeaderValue::from_static("session=secret")),
+        ]);
+
+        remove_sensitive_headers(&mut headers, &previous, &next);
+
+        assert_eq!(
+            (
+                headers.contains_key(AUTHORIZATION),
+                headers.contains_key(COOKIE),
+            ),
+            (retain_credentials, retain_credentials),
+            "credential handling for {previous} -> {next}"
+        );
+    }
+}