From b8c2d29cc23b41fa7c7f5f5483e92fc71099635a Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Tue, 21 Jul 2026 07:44:22 +0000 Subject: [PATCH] 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 --- .../src/route_aware_client_pool_tests.rs | 321 ++++++++++++++++++ .../http-client/src/route_aware_redirect.rs | 4 + .../src/route_aware_redirect_tests.rs | 233 +++++++++++++ 3 files changed, 558 insertions(+) create mode 100644 codex-rs/http-client/src/route_aware_redirect_tests.rs diff --git a/codex-rs/http-client/src/route_aware_client_pool_tests.rs b/codex-rs/http-client/src/route_aware_client_pool_tests.rs index caca3454fe..fe042aa372 100644 --- a/codex-rs/http-client/src/route_aware_client_pool_tests.rs +++ b/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>, @@ -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, +) -> (std::net::SocketAddr, std::thread::JoinHandle>) { + 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::().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>>, +} + +struct TestLogSink { + buffer: Arc>>, +} + +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 { + 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(()) + } +} diff --git a/codex-rs/http-client/src/route_aware_redirect.rs b/codex-rs/http-client/src/route_aware_redirect.rs index 25ee9ed2b9..77af745725 100644 --- a/codex-rs/http-client/src/route_aware_redirect.rs +++ b/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; diff --git a/codex-rs/http-client/src/route_aware_redirect_tests.rs b/codex-rs/http-client/src/route_aware_redirect_tests.rs new file mode 100644 index 0000000000..d9e8f6053f --- /dev/null +++ b/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}" + ); + } +}