Files
codex/codex-rs/http-client/src/default_client.rs
Michael Bolin 6d3cde7fcd 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
2026-07-09 14:26:20 -07:00

257 lines
7.1 KiB
Rust

use http::Error as HttpError;
use http::HeaderMap;
use http::HeaderName;
use http::HeaderValue;
use opentelemetry::global;
use opentelemetry::propagation::Injector;
use reqwest::IntoUrl;
use reqwest::Method;
use reqwest::Response;
use serde::Serialize;
use std::fmt::Display;
use std::time::Duration;
use tracing::Span;
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,
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
where
U: IntoUrl,
{
self.request(Method::GET, url)
}
pub fn post<U>(&self, url: U) -> RequestBuilder
where
U: IntoUrl,
{
self.request(Method::POST, url)
}
pub fn request<U>(&self, method: Method, url: U) -> RequestBuilder
where
U: IntoUrl,
{
let url_str = url.as_str().to_string();
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,
request_logging: RequestLogging,
) -> Self {
Self {
builder,
method,
url,
request_logging,
}
}
fn map(self, f: impl FnOnce(reqwest::RequestBuilder) -> reqwest::RequestBuilder) -> Self {
Self {
builder: f(self.builder),
method: self.method,
url: self.url,
request_logging: self.request_logging,
}
}
pub fn headers(self, headers: HeaderMap) -> Self {
self.map(|builder| builder.headers(headers))
}
pub fn header<K, V>(self, key: K, value: V) -> Self
where
HeaderName: TryFrom<K>,
<HeaderName as TryFrom<K>>::Error: Into<HttpError>,
HeaderValue: TryFrom<V>,
<HeaderValue as TryFrom<V>>::Error: Into<HttpError>,
{
self.map(|builder| builder.header(key, value))
}
pub fn bearer_auth<T>(self, token: T) -> Self
where
T: Display,
{
self.map(|builder| builder.bearer_auth(token))
}
pub fn timeout(self, timeout: Duration) -> Self {
self.map(|builder| builder.timeout(timeout))
}
pub fn json<T>(self, value: &T) -> Self
where
T: ?Sized + Serialize,
{
self.map(|builder| builder.json(value))
}
pub fn body<B>(self, body: B) -> Self
where
B: Into<reqwest::Body>,
{
self.map(|builder| builder.body(body))
}
pub async fn send(self) -> Result<Response, reqwest::Error> {
let headers = trace_headers();
match self.builder.headers(headers).send().await {
Ok(response) => {
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) => {
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)
}
}
}
}
struct HeaderMapInjector<'a>(&'a mut HeaderMap);
impl<'a> Injector for HeaderMapInjector<'a> {
fn set(&mut self, key: &str, value: String) {
if let (Ok(name), Ok(val)) = (
HeaderName::from_bytes(key.as_bytes()),
HeaderValue::from_str(&value),
) {
self.0.insert(name, val);
}
}
}
fn trace_headers() -> HeaderMap {
let mut headers = HeaderMap::new();
global::get_text_map_propagator(|prop| {
prop.inject_context(
&Span::current().context(),
&mut HeaderMapInjector(&mut headers),
);
});
headers
}
#[cfg(test)]
mod tests {
use super::*;
use opentelemetry::propagation::Extractor;
use opentelemetry::propagation::TextMapPropagator;
use opentelemetry::trace::TraceContextExt;
use opentelemetry::trace::TracerProvider;
use opentelemetry_sdk::propagation::TraceContextPropagator;
use opentelemetry_sdk::trace::SdkTracerProvider;
use tracing::trace_span;
use tracing_subscriber::layer::SubscriberExt;
use tracing_subscriber::util::SubscriberInitExt;
#[test]
fn inject_trace_headers_uses_current_span_context() {
global::set_text_map_propagator(TraceContextPropagator::new());
let provider = SdkTracerProvider::builder().build();
let tracer = provider.tracer("test-tracer");
let subscriber =
tracing_subscriber::registry().with(tracing_opentelemetry::layer().with_tracer(tracer));
let _guard = subscriber.set_default();
let span = trace_span!("client_request");
let _entered = span.enter();
let span_context = span.context().span().span_context().clone();
let headers = trace_headers();
let extractor = HeaderMapExtractor(&headers);
let extracted = TraceContextPropagator::new().extract(&extractor);
let extracted_span = extracted.span();
let extracted_context = extracted_span.span_context();
assert!(extracted_context.is_valid());
assert_eq!(extracted_context.trace_id(), span_context.trace_id());
assert_eq!(extracted_context.span_id(), span_context.span_id());
}
struct HeaderMapExtractor<'a>(&'a HeaderMap);
impl<'a> Extractor for HeaderMapExtractor<'a> {
fn get(&self, key: &str) -> Option<&str> {
self.0.get(key).and_then(|value| value.to_str().ok())
}
fn keys(&self) -> Vec<&str> {
self.0.keys().map(HeaderName::as_str).collect()
}
}
}