diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 023c5dfaa366..db4350e1f947 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 b4465e7f8cf2..73ff72f66fb0 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 7bdfd745b7c4..692da254fc4a 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 3eb253d7dc1d..f50648c3afb4 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 0765d227ce1f..ac153ea8a4ae 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 be8e6bc392af..d873c70e0dc1 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 ca22adb57131..6b2b21dcf832 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 e7a31916a5dc..0d9c7244a103 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 01671bf0b11d..804d05434e23 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")