Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions codex-rs/Cargo.lock

Some generated files are not rendered by default. Learn more about how customized files appear on GitHub.

76 changes: 57 additions & 19 deletions codex-rs/http-client/src/default_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<U>(&self, url: U) -> RequestBuilder
Expand All @@ -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,
}
}

Expand All @@ -68,6 +101,7 @@ impl RequestBuilder {
builder: f(self.builder),
method: self.method,
url: self.url,
request_logging: self.request_logging,
}
}

Expand Down Expand Up @@ -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)
}
}
Expand Down
21 changes: 21 additions & 0 deletions codex-rs/http-client/src/outbound_proxy.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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"))]
Expand Down Expand Up @@ -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<HttpClient, BuildRouteAwareHttpClientError> {
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<HttpClient, BuildRouteAwareHttpClientError> {
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,
Expand Down
2 changes: 2 additions & 0 deletions codex-rs/login/Cargo.toml
Original file line number Diff line number Diff line change
Expand Up @@ -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 }
Expand Down Expand Up @@ -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]
Expand Down
13 changes: 5 additions & 8 deletions codex-rs/login/src/auth/default_client.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<reqwest::Client, BuildRouteAwareHttpClientError> {
auth_http_client_factory(auth_route_config).build_reqwest_client(
reqwest::Client::builder(),
endpoint,
ClientRouteClass::Auth,
)
) -> Result<HttpClient, BuildRouteAwareHttpClientError> {
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.
Expand Down
118 changes: 118 additions & 0 deletions codex-rs/login/src/auth/default_client_tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -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<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, buf: &[u8]) -> io::Result<usize> {
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() {
Expand Down Expand Up @@ -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";
Expand Down
7 changes: 6 additions & 1 deletion codex-rs/login/src/auth/revoke.rs
Original file line number Diff line number Diff line change
Expand Up @@ -155,6 +155,9 @@ fn derive_revoke_token_endpoint(refresh_endpoint: &str) -> Option<String> {
#[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;
Expand All @@ -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(),
Expand Down
Loading
Loading