From 6d3cde7fcd1db4f851928f5eccbb90cb4df26ca2 Mon Sep 17 00:00:00 2001 From: Michael Bolin Date: Thu, 9 Jul 2026 14:26:20 -0700 Subject: [PATCH] login: route raw auth flows through HTTP client (#31637) ## Why Login already honors `respect_system_proxy`, but several login-owned auth flows still construct and pass around raw `reqwest::Client` values. That keeps those request paths coupled to the underlying transport and leaves `codex-login` on the temporary direct-`reqwest` allowlist introduced by #31431. Auth endpoints also have a stricter logging boundary than ordinary API requests: custom issuer URLs and response headers may contain credentials. Moving these requests behind the shared HTTP abstraction must preserve that boundary while retaining route-aware proxy and custom-CA behavior. This is a bounded login migration. The separate Agent Identity and shared default-client compatibility migrations remain follow-up work. ## What changed - Add `HttpClientFactory::build_client` to construct the shared `HttpClient` abstraction for a resolved destination and route class. - Add a route-aware construction path that suppresses request URL, response-header, and transport-error diagnostics for sensitive auth endpoints. - Route device-code user-code/polling requests, OAuth authorization-code exchange, and API-key token exchange through `HttpClient`. - Build the revoke timeout test client through the same factory API. - Use the transport-neutral `http::StatusCode` in the migrated device-code flow. - Add an end-to-end log-capture regression test covering successful responses and transport failures after `RequestBuilder` transformations. ## Review guidance The request behavior is intended to be unchanged: each issuer/token endpoint selects the same auth route, including the existing system/PAC proxy and custom-CA handling, and raw auth clients still omit Codex default headers. The intentional logging change is limited to raw auth requests, whose URL userinfo, query credentials, response headers, and transport errors must not cross the auth redaction boundary. This PR deliberately does **not** remove `codex-login` from #31431's allowlist. The remaining direct `reqwest` surface belongs primarily to: - Agent Identity APIs that still accept `reqwest::Client`. - Exported default-client compatibility helpers used by other workspace crates. - A small number of tests and concrete error/header types. ## Testing - `cargo check -p codex-http-client -p codex-login --tests` - `just test -p codex-http-client` (41 tests) - `just test -p codex-login` (155 tests) - `just bazel-lock-check` --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/31637). * #31837 * #31828 * #31825 * #31821 * __->__ #31637 --- codex-rs/Cargo.lock | 2 + codex-rs/http-client/src/default_client.rs | 76 ++++++++--- codex-rs/http-client/src/outbound_proxy.rs | 21 ++++ codex-rs/login/Cargo.toml | 2 + codex-rs/login/src/auth/default_client.rs | 13 +- .../login/src/auth/default_client_tests.rs | 118 ++++++++++++++++++ codex-rs/login/src/auth/revoke.rs | 7 +- codex-rs/login/src/device_code_auth.rs | 13 +- codex-rs/login/src/server.rs | 6 +- 9 files changed, 221 insertions(+), 37 deletions(-) diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 023c5dfaa3..db4350e1f9 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -3350,6 +3350,7 @@ dependencies = [ "codex-terminal-detection", "codex-utils-template", "core_test_support", + "http 1.4.0", "jsonwebtoken", "keyring", "once_cell", @@ -3367,6 +3368,7 @@ dependencies = [ "tiny_http", "tokio", "tracing", + "tracing-subscriber", "url", "urlencoding", "webbrowser", diff --git a/codex-rs/http-client/src/default_client.rs b/codex-rs/http-client/src/default_client.rs index b4465e7f8c..73ff72f66f 100644 --- a/codex-rs/http-client/src/default_client.rs +++ b/codex-rs/http-client/src/default_client.rs @@ -16,11 +16,26 @@ use tracing_opentelemetry::OpenTelemetrySpanExt; #[derive(Clone, Debug)] pub struct HttpClient { inner: reqwest::Client, + request_logging: RequestLogging, } impl HttpClient { pub fn new(inner: reqwest::Client) -> Self { - Self { inner } + Self { + inner, + request_logging: RequestLogging::Enabled, + } + } + + /// Creates a client that suppresses request URL and response-header diagnostics. + /// + /// Use this for authentication endpoints whose URLs or headers may contain credentials that + /// are redacted by the caller above the HTTP transport boundary. + pub(crate) fn new_without_request_logging(inner: reqwest::Client) -> Self { + Self { + inner, + request_logging: RequestLogging::Disabled, + } } pub fn get(&self, url: U) -> RequestBuilder @@ -42,24 +57,42 @@ impl HttpClient { U: IntoUrl, { let url_str = url.as_str().to_string(); - RequestBuilder::new(self.inner.request(method.clone(), url), method, url_str) + RequestBuilder::new( + self.inner.request(method.clone(), url), + method, + url_str, + self.request_logging, + ) } } +#[derive(Clone, Copy, Debug, PartialEq, Eq)] +enum RequestLogging { + Enabled, + Disabled, +} + #[must_use = "requests are not sent unless `send` is awaited"] #[derive(Debug)] pub struct RequestBuilder { builder: reqwest::RequestBuilder, method: Method, url: String, + request_logging: RequestLogging, } impl RequestBuilder { - fn new(builder: reqwest::RequestBuilder, method: Method, url: String) -> Self { + fn new( + builder: reqwest::RequestBuilder, + method: Method, + url: String, + request_logging: RequestLogging, + ) -> Self { Self { builder, method, url, + request_logging, } } @@ -68,6 +101,7 @@ impl RequestBuilder { builder: f(self.builder), method: self.method, url: self.url, + request_logging: self.request_logging, } } @@ -115,26 +149,30 @@ impl RequestBuilder { match self.builder.headers(headers).send().await { Ok(response) => { - tracing::debug!( - method = %self.method, - url = %self.url, - status = %response.status(), - headers = ?response.headers(), - version = ?response.version(), - "Request completed" - ); + if self.request_logging == RequestLogging::Enabled { + tracing::debug!( + method = %self.method, + url = %self.url, + status = %response.status(), + headers = ?response.headers(), + version = ?response.version(), + "Request completed" + ); + } Ok(response) } Err(error) => { - let status = error.status(); - tracing::debug!( - method = %self.method, - url = %self.url, - status = status.map(|s| s.as_u16()), - error = %error, - "Request failed" - ); + if self.request_logging == RequestLogging::Enabled { + let status = error.status(); + tracing::debug!( + method = %self.method, + url = %self.url, + status = status.map(|s| s.as_u16()), + error = %error, + "Request failed" + ); + } Err(error) } } diff --git a/codex-rs/http-client/src/outbound_proxy.rs b/codex-rs/http-client/src/outbound_proxy.rs index 7bdfd745b7..692da254fc 100644 --- a/codex-rs/http-client/src/outbound_proxy.rs +++ b/codex-rs/http-client/src/outbound_proxy.rs @@ -15,6 +15,7 @@ use std::time::Instant; use crate::custom_ca::BuildCustomCaTransportError; use crate::custom_ca::build_reqwest_client_with_custom_ca; +use crate::default_client::HttpClient; #[cfg(any(target_os = "windows", target_os = "macos"))] use sha2::Digest; #[cfg(any(target_os = "windows", target_os = "macos"))] @@ -158,6 +159,26 @@ impl HttpClientFactory { ) } + /// Builds an HTTP client for a concrete outbound route. + pub fn build_client( + &self, + request_url: &str, + route_class: ClientRouteClass, + ) -> Result { + self.build_reqwest_client(reqwest::Client::builder(), request_url, route_class) + .map(HttpClient::new) + } + + /// Builds a route-aware client without request URL or response-header diagnostics. + pub fn build_client_without_request_logging( + &self, + request_url: &str, + route_class: ClientRouteClass, + ) -> Result { + self.build_reqwest_client(reqwest::Client::builder(), request_url, route_class) + .map(HttpClient::new_without_request_logging) + } + /// Builds a reqwest client for a concrete outbound route. pub fn build_reqwest_client( &self, diff --git a/codex-rs/login/Cargo.toml b/codex-rs/login/Cargo.toml index 3eb253d7dc..f50648c3af 100644 --- a/codex-rs/login/Cargo.toml +++ b/codex-rs/login/Cargo.toml @@ -20,6 +20,7 @@ codex-protocol = { workspace = true } codex-secrets = { workspace = true } codex-terminal-detection = { workspace = true } codex-utils-template = { workspace = true } +http = { workspace = true } once_cell = { workspace = true } os_info = { workspace = true } rand = { workspace = true } @@ -50,6 +51,7 @@ pretty_assertions = { workspace = true } regex-lite = { workspace = true } serial_test = { workspace = true } tempfile = { workspace = true } +tracing-subscriber = { workspace = true } wiremock = { workspace = true } [lib] diff --git a/codex-rs/login/src/auth/default_client.rs b/codex-rs/login/src/auth/default_client.rs index 0765d227ce..ac153ea8a4 100644 --- a/codex-rs/login/src/auth/default_client.rs +++ b/codex-rs/login/src/auth/default_client.rs @@ -295,16 +295,13 @@ fn default_reqwest_client_builder() -> reqwest::ClientBuilder { with_chatgpt_cloudflare_cookie_store(builder) } -/// Builds a raw reqwest client for an auth endpoint without Codex default headers. -pub(crate) fn build_raw_auth_reqwest_client( +/// Builds an HTTP client for an auth endpoint without Codex default headers. +pub(crate) fn create_raw_auth_client( endpoint: &str, auth_route_config: Option<&AuthRouteConfig>, -) -> Result { - auth_http_client_factory(auth_route_config).build_reqwest_client( - reqwest::Client::builder(), - endpoint, - ClientRouteClass::Auth, - ) +) -> Result { + auth_http_client_factory(auth_route_config) + .build_client_without_request_logging(endpoint, ClientRouteClass::Auth) } /// Builds the default Codex reqwest client for an auth endpoint. diff --git a/codex-rs/login/src/auth/default_client_tests.rs b/codex-rs/login/src/auth/default_client_tests.rs index be8e6bc392..d873c70e0d 100644 --- a/codex-rs/login/src/auth/default_client_tests.rs +++ b/codex-rs/login/src/auth/default_client_tests.rs @@ -2,6 +2,41 @@ use super::sanitize_user_agent; use super::*; use core_test_support::skip_if_no_network; use pretty_assertions::assert_eq; +use std::io; +use std::io::Write; +use std::sync::Arc; +use std::sync::Mutex; +use tracing_subscriber::layer::SubscriberExt; + +#[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, buf: &[u8]) -> io::Result { + self.buffer.lock().expect("log buffer lock").extend(buf); + Ok(buf.len()) + } + + fn flush(&mut self) -> io::Result<()> { + Ok(()) + } +} #[test] fn test_get_codex_user_agent() { @@ -89,6 +124,89 @@ async fn test_create_client_sets_default_headers() { set_default_client_residency_requirement(/*enforce_residency*/ None); } +#[tokio::test] +async fn raw_auth_client_does_not_log_sensitive_request_or_response_data() { + use wiremock::Mock; + use wiremock::MockServer; + use wiremock::ResponseTemplate; + use wiremock::matchers::method; + use wiremock::matchers::path; + + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/token")) + .respond_with( + ResponseTemplate::new(200) + .insert_header("x-sensitive-response", "response-secret-value"), + ) + .expect(1) + .mount(&server) + .await; + let authority = server + .uri() + .strip_prefix("http://") + .expect("wiremock URI should use HTTP") + .to_string(); + let endpoint = format!( + "http://auth-user:password-secret-value@{authority}/token?client_secret=query-secret-value" + ); + let client = create_raw_auth_client(&endpoint, /*auth_route_config*/ None) + .expect("raw auth client should build"); + 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), + }), + ); + let _guard = tracing::subscriber::set_default(subscriber); + tracing::debug!("log capture sentinel"); + + let response = client + .post(&endpoint) + .header("x-sensitive-request", "request-header-secret-value") + .body("request-body-secret-value") + .send() + .await + .expect("raw auth request should succeed"); + assert!(response.status().is_success()); + + let unresponsive_listener = std::net::TcpListener::bind("127.0.0.1:0") + .expect("unresponsive local listener should bind"); + let unresponsive_addr = unresponsive_listener + .local_addr() + .expect("unresponsive local address should be available"); + let unresponsive_endpoint = format!( + "http://auth-user:failure-password-secret-value@{unresponsive_addr}/token?client_secret=failure-query-secret-value" + ); + let unresponsive_client = + create_raw_auth_client(&unresponsive_endpoint, /*auth_route_config*/ None) + .expect("raw auth client should build"); + let error = unresponsive_client + .post(&unresponsive_endpoint) + .header("x-sensitive-request", "failure-request-header-secret-value") + .body("failure-request-body-secret-value") + .timeout(std::time::Duration::from_secs(1)) + .send() + .await + .expect_err("request to an unresponsive local 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")); + assert!(!logs.contains("password-secret-value")); + assert!(!logs.contains("query-secret-value")); + assert!(!logs.contains("request-header-secret-value")); + assert!(!logs.contains("request-body-secret-value")); + assert!(!logs.contains("response-secret-value")); + assert!(!logs.contains("failure-password-secret-value")); + assert!(!logs.contains("failure-query-secret-value")); + assert!(!logs.contains("failure-request-header-secret-value")); + assert!(!logs.contains("failure-request-body-secret-value")); +} + #[test] fn test_invalid_suffix_is_sanitized() { let prefix = "codex_cli_rs/0.0.0"; diff --git a/codex-rs/login/src/auth/revoke.rs b/codex-rs/login/src/auth/revoke.rs index ca22adb571..6b2b21dcf8 100644 --- a/codex-rs/login/src/auth/revoke.rs +++ b/codex-rs/login/src/auth/revoke.rs @@ -155,6 +155,9 @@ fn derive_revoke_token_endpoint(refresh_endpoint: &str) -> Option { #[cfg(test)] mod tests { use super::*; + use codex_http_client::ClientRouteClass; + use codex_http_client::HttpClientFactory; + use codex_http_client::OutboundProxyPolicy; use core_test_support::skip_if_no_network; use wiremock::Mock; use wiremock::MockServer; @@ -181,8 +184,10 @@ mod tests { .mount(&server) .await; - let client = HttpClient::new(reqwest::Client::new()); let endpoint = format!("{}/oauth/revoke", server.uri()); + let client = HttpClientFactory::new(OutboundProxyPolicy::ReqwestDefault) + .build_client(&endpoint, ClientRouteClass::Auth) + .expect("test HTTP client should build"); let error = revoke_oauth_token( &client, endpoint.as_str(), diff --git a/codex-rs/login/src/device_code_auth.rs b/codex-rs/login/src/device_code_auth.rs index e7a31916a5..0d9c7244a1 100644 --- a/codex-rs/login/src/device_code_auth.rs +++ b/codex-rs/login/src/device_code_auth.rs @@ -1,4 +1,5 @@ -use reqwest::StatusCode; +use codex_http_client::HttpClient; +use http::StatusCode; use serde::Deserialize; use serde::Serialize; use serde::de::Deserializer; @@ -6,7 +7,7 @@ use serde::de::{self}; use std::time::Duration; use std::time::Instant; -use crate::default_client::build_raw_auth_reqwest_client; +use crate::default_client::create_raw_auth_client; use crate::pkce::PkceCodes; use crate::server::ServerOptions; use std::io; @@ -60,7 +61,7 @@ struct CodeSuccessResp { /// Request the user code and polling interval. async fn request_user_code( - client: &reqwest::Client, + client: &HttpClient, auth_base_url: &str, client_id: &str, ) -> std::io::Result { @@ -97,7 +98,7 @@ async fn request_user_code( /// Poll token endpoint until a code is issued or timeout occurs. async fn poll_for_token( - client: &reqwest::Client, + client: &HttpClient, auth_base_url: &str, device_auth_id: &str, user_code: &str, @@ -165,7 +166,7 @@ pub async fn request_device_code(opts: &ServerOptions) -> std::io::Result std::io::Result<()> { let base_url = opts.issuer.trim_end_matches('/'); - let client = build_raw_auth_reqwest_client(base_url, opts.auth_route_config.as_ref())?; + let client = create_raw_auth_client(base_url, opts.auth_route_config.as_ref())?; let api_base_url = format!("{base_url}/api/accounts"); let code_resp = poll_for_token( diff --git a/codex-rs/login/src/server.rs b/codex-rs/login/src/server.rs index 01671bf0b1..804d05434e 100644 --- a/codex-rs/login/src/server.rs +++ b/codex-rs/login/src/server.rs @@ -27,7 +27,7 @@ use std::time::Duration; use crate::auth::AuthDotJson; use crate::auth::AuthKeyringBackendKind; use crate::auth::save_auth; -use crate::default_client::build_raw_auth_reqwest_client; +use crate::default_client::create_raw_auth_client; use crate::default_client::originator; use crate::outbound_proxy::AuthRouteConfig; use crate::pkce::PkceCodes; @@ -798,7 +798,7 @@ pub(crate) async fn exchange_code_for_tokens( // The route selected for the issuer is reused for token exchange; the token endpoint path is // not resolved separately. - let client = build_raw_auth_reqwest_client(issuer.trim_end_matches('/'), auth_route_config)?; + let client = create_raw_auth_client(issuer.trim_end_matches('/'), auth_route_config)?; let token_endpoint = format!("{}/oauth/token", issuer.trim_end_matches('/')); info!( issuer = %sanitize_url_for_logging(issuer), @@ -1120,7 +1120,7 @@ pub(crate) async fn obtain_api_key( access_token: String, } let token_endpoint = format!("{}/oauth/token", issuer.trim_end_matches('/')); - let client = build_raw_auth_reqwest_client(&token_endpoint, auth_route_config)?; + let client = create_raw_auth_client(&token_endpoint, auth_route_config)?; let resp = client .post(token_endpoint) .header("Content-Type", "application/x-www-form-urlencoded")