login: route raw auth flows through HTTP client by bolinfest · Pull Request #31637 · openai/codex

bolinfest · 2026-07-08T20:36:35Z

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

Stack created with Sapling. Best reviewed with ReviewStack.

chatgpt-codex-connector

💡 Codex Review

Here are some automated review suggestions for this pull request.

Reviewed commit: 2738238b5d

ℹ️ About Codex in GitHub

Your team has set up Codex to review pull requests in this repo. Reviews are triggered when you

Open a pull request for review
Mark a draft as ready
Comment "@codex review".

If Codex has suggestions, it will comment; otherwise it will react with 👍.

Codex can also answer questions or update the PR. Try commenting "@codex address that feedback".

chatgpt-codex-connector · 2026-07-08T20:45:53Z

-        ClientRouteClass::Auth,
-    )
+) -> Result<HttpClient, BuildRouteAwareHttpClientError> {
+    auth_http_client_factory(auth_route_config).build_client(endpoint, ClientRouteClass::Auth)


Keep raw auth requests off the logging HTTP wrapper

When a non-default auth issuer embeds credentials or query parameters, returning the shared HttpClient here routes token/device-code requests through HttpClient::send, which debug-logs the raw request URL before server.rs gets a chance to apply its explicit URL redaction. That bypasses the redaction safeguards around custom auth endpoints and can persist sensitive issuer components in logs; keep this raw auth path on an unlogged reqwest client or add a redacted/no-log request path for auth URLs.

Useful? React with 👍 / 👎.

Addressed on the updated head. Raw auth requests now use a route-aware HttpClient with request diagnostics disabled, so the transport no longer logs raw auth URLs, response headers, or request errors while preserving proxy and custom-CA behavior. I also added raw_auth_client_does_not_log_sensitive_request_or_response_data, which captures debug logs and verifies that URL userinfo, a query credential, and a sensitive response header do not appear.

## Why Responses WebSockets are the normal lower-latency transport for WebSocket-capable providers. They must not bypass an OS-selected proxy when `features.respect_system_proxy` is enabled, but disabling WebSockets whenever the feature is enabled would impose a substantial performance penalty. Merged PR #31622 introduced the reusable proxy-aware WebSocket transport. This PR makes the Responses API its first consumer so the existing fast path uses the same effective proxy and trust policy as HTTP. ## What changed - Register `codex-websocket-client` as a workspace dependency and use it from `codex-api`. - Feed the shared crate’s route-independent `WebSocketConnection` into the existing Responses message pump. - Require a configured `HttpClientFactory` for normal Responses WebSocket connections and the CLI doctor probe, so neither path can open a connection without consulting the effective proxy policy. - Pass the session factory from `core` and the effective configuration factory from `doctor`. - Add an end-to-end Responses test that enables `RespectSystemProxy`, asserts the resolved policy, completes a turn over WebSocket, and verifies the connection and request counts. - Keep the existing Responses protocol handling, ping/pong pump, and session-scoped HTTP fallback unchanged. The DNS, proxy, TLS, custom-CA, and Happy Eyeballs implementation and its transport tests live in merged PR #31622. This PR deliberately contains only the Responses integration and does not duplicate that transport code. ## Review guide 1. `codex-rs/codex-api/src/endpoint/responses_websocket.rs` constructs the shared connector and adapts its uniform stream to the existing pump. 2. `codex-rs/core/src/client.rs` supplies the session-scoped factory for production Responses connections. 3. `codex-rs/cli/src/doctor.rs` supplies the effective configuration factory to the handshake probe. 4. `codex-rs/core/tests/suite/client_websockets.rs` covers the enabled-feature path end to end. ## Test plan - `cargo check --tests -p codex-api -p codex-core -p codex-cli` - `just test -p codex-api` - `just test -p codex-core responses_websocket_streams_with_system_proxy_feature` - `cargo shear` - `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/31441). * #31637 * #31431 * #31363 * #31362 * #31361 * __->__ #31441

## Why `ModelClient` already carries the `HttpClientFactory` resolved from session configuration, but realtime call creation and memory summarization still constructed the legacy default client directly. Consequently, those first-party API requests could ignore `features.respect_system_proxy` even when Responses traffic honored it. These are the final direct default-client constructions in `core/src/client.rs`, so they form one small migration unit on top of #31361. ## What changed - Generalize `build_responses_transport` to `build_api_transport`. - Route realtime call creation through the helper using the selected provider and `/realtime/calls` destination. - Route `/memories/trace_summarize` through the same helper. - Remove the now-unused direct `build_reqwest_client` import. ## Review guide 1. The helper rename at the bottom of `core/src/client.rs` is mechanical and keeps existing Responses behavior unchanged. 2. The realtime call path computes the route from the final provider, including `api_provider_override`. 3. The memories path supplies its existing endpoint to the same API route class. ## Validation - `cargo check --tests -p codex-core` - `just fix -p codex-core` ## Follow-up Direct HTTP clients outside `ModelClient` remain separate migration slices. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/31362). * #31637 * #31431 * #31363 * __->__ #31362

viyatb-oai · 2026-07-09T16:06:32Z

-        ClientRouteClass::Auth,
-    )
+) -> Result<HttpClient, BuildRouteAwareHttpClientError> {
+    auth_http_client_factory(auth_route_config).build_client(endpoint, ClientRouteClass::Auth)


Can we preserve the auth redaction boundary when these calls move to HttpClient, and cover it with a log-capture regression test? HttpClient::send logs the request URL and response headers verbatim, so a custom issuer containing userinfo or query credentials is recorded before sanitize_url_for_logging or redact_sensitive_error_url runs. The existing sanitizer tests do not exercise this wrapper, so they still pass while the raw values reach debug logs.

Addressed on the updated head. create_raw_auth_client now builds the shared route-aware client through build_client_without_request_logging, preserving the auth redaction boundary without giving up system-proxy/custom-CA routing. The new log-capture regression test exercises the wrapper end to end and asserts that URL userinfo, a query credential, and a sensitive response header are absent from debug logs.

## Why Codex Apps file parameters use a three-step upload flow: create a file record, PUT bytes to a returned signed URL, and finalize the upload. Each step still constructed a default `reqwest` client, so the flow could bypass `features.respect_system_proxy` even after model API requests honored it. This stack entry makes the resolved client policy a required input to the upload API and resolves each concrete destination independently. ## What changed - Require `HttpClientFactory` in `upload_openai_file`. - Build clients for the create, signed upload, and finalize URLs through the shared API route policy. - Pass the factory derived from the turn configuration at the Apps/MCP call site. - Return a destination-aware `ClientBuild` error when enabled route selection cannot construct a client. - Preserve the legacy logged fallback for the feature-off `ReqwestDefault` policy. ## Review guide 1. `codex-api/src/files.rs` changes the upload API and centralizes route-aware client construction. 2. The three request stages each supply their actual URL, including the separately hosted signed blob URL. 3. `core/src/mcp_openai_file.rs` is the only production caller and supplies the turn configuration factory. ## Validation - `cargo check --tests -p codex-api -p codex-core` - `just test -p codex-api files` (1 matching upload test passed; 135 tests skipped by filter) - `just fix -p codex-api -p codex-core` ## Follow-up Other direct HTTP clients remain separate migration slices. --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/31363). * #31637 * #31431 * __->__ #31363

## Why The `codex-http-client` migration now has a shared implementation and several migrated request paths, but nothing prevents a new crate from adding another direct `reqwest` dependency while the remaining call sites are being converted. The dependency graph should both enforce the direction of travel and make the remaining scope visible. This PR adds that ratchet on top of #31363. It does not claim the migration is complete: the allowlist deliberately records all 18 first-party crates that still depend on `reqwest` directly. ## What changed - Ban `reqwest` with cargo-deny unless its immediate parent is an explicitly listed wrapper. - Identify `codex-http-client` as the intended owner. - Record the 18 current first-party direct dependents as temporary migration exceptions. - Separately allow six third-party integrations that own their `reqwest` dependency: `oauth2`, `opentelemetry-http`, `opentelemetry-otlp`, `rmcp`, `sentry`, and `webrtc-sys-build`. - Cover both `reqwest` 0.12 and 0.13 with the same package-level rule. ## Migration rule A new first-party crate cannot add `reqwest`. When a listed crate finishes migrating, its direct Cargo dependency and its wrapper entry should be removed in the same PR, so the first-party list can only shrink. ## Review guide The entire change is the new `reqwest` entry in `codex-rs/deny.toml`: 1. `codex-http-client` is the permanent intended wrapper. 2. The next 18 entries are the first-party migration backlog. 3. The final six entries are separately documented third-party parents required by cargo-deny graph semantics. ## Validation - `cargo deny check bans --hide-inclusion-graph` (`bans ok`; existing duplicate-version warnings remain warnings) --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/31431). * #31837 * #31828 * #31825 * #31821 * #31637 * __->__ #31431

bolinfest requested a review from a team as a code owner July 8, 2026 20:36

bolinfest changed the base branch from main to pr31431 July 8, 2026 20:36

bolinfest changed the title ~~login: use route-aware HTTP client abstraction~~ login: route raw auth flows through HTTP client Jul 8, 2026

bolinfest added the code-reviewed label Jul 8, 2026

chatgpt-codex-connector Bot reviewed Jul 8, 2026

View reviewed changes

bolinfest force-pushed the pr31431 branch from 8535ca6 to 587b55a Compare July 8, 2026 21:06

bolinfest force-pushed the pr31637 branch from 2738238 to 5628d32 Compare July 8, 2026 21:06

bolinfest force-pushed the pr31431 branch from 587b55a to 6ee17f1 Compare July 8, 2026 22:31

bolinfest force-pushed the pr31637 branch 2 times, most recently from 3e87226 to 9bcec5a Compare July 9, 2026 01:24

bolinfest force-pushed the pr31431 branch 2 times, most recently from 0de9a15 to da872ce Compare July 9, 2026 01:25

bolinfest force-pushed the pr31637 branch from 9bcec5a to 6d4c722 Compare July 9, 2026 01:26

This was referenced Jul 9, 2026

📊 AI CLI 工具社区动态日报 2026-07-09 junlinzhao327-oss/big_model_radar#6

Open

📊 AI CLI 工具社区动态日报 2026-07-09 zx0828/big_model_radar#235

Open

bolinfest force-pushed the pr31637 branch from 6d4c722 to b2515fa Compare July 9, 2026 07:17

bolinfest force-pushed the pr31431 branch from da872ce to be13a49 Compare July 9, 2026 07:17

ylabo0717 mentioned this pull request Jul 9, 2026

Codex route-aware HTTP deep dive ylabo0717/ebisuke-blog#78

Merged

bolinfest force-pushed the pr31431 branch from be13a49 to 34dbb0f Compare July 9, 2026 15:59

bolinfest force-pushed the pr31637 branch from b2515fa to b0a3c14 Compare July 9, 2026 15:59

viyatb-oai reviewed Jul 9, 2026

View reviewed changes

bolinfest force-pushed the pr31431 branch from 34dbb0f to cd62594 Compare July 9, 2026 16:58

bolinfest force-pushed the pr31637 branch from b0a3c14 to 93835d1 Compare July 9, 2026 16:58

bolinfest force-pushed the pr31431 branch from cd62594 to ab1f37e Compare July 9, 2026 18:44

bolinfest force-pushed the pr31637 branch from 93835d1 to 3b84b5c Compare July 9, 2026 18:44

Base automatically changed from pr31431 to main July 9, 2026 18:53

bolinfest force-pushed the pr31637 branch 2 times, most recently from a45cdd6 to a1ef581 Compare July 9, 2026 19:01

login: use route-aware HTTP client abstraction

fc0a88a

bolinfest force-pushed the pr31637 branch from a1ef581 to fc0a88a Compare July 9, 2026 19:08

bolinfest requested review from viyatb-oai and removed request for a team July 9, 2026 19:23

viyatb-oai approved these changes Jul 9, 2026

View reviewed changes

bolinfest merged commit 6d3cde7 into main Jul 9, 2026
50 of 69 checks passed

bolinfest deleted the pr31637 branch July 9, 2026 21:26

github-actions Bot locked and limited conversation to collaborators Jul 9, 2026

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

login: route raw auth flows through HTTP client#31637

login: route raw auth flows through HTTP client#31637
bolinfest merged 1 commit into
mainfrom
pr31637

bolinfest commented Jul 8, 2026 •

edited

Loading

Uh oh!

chatgpt-codex-connector Bot left a comment

Uh oh!

chatgpt-codex-connector Bot Jul 8, 2026

Uh oh!

bolinfest Jul 9, 2026

Uh oh!

viyatb-oai Jul 9, 2026

Uh oh!

bolinfest Jul 9, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

Uh oh!

Conversation

bolinfest commented Jul 8, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Why

What changed

Review guidance

Testing

Uh oh!

chatgpt-codex-connector Bot left a comment

Choose a reason for hiding this comment

💡 Codex Review

Uh oh!

chatgpt-codex-connector Bot Jul 8, 2026

Choose a reason for hiding this comment

Uh oh!

bolinfest Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

viyatb-oai Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

bolinfest Jul 9, 2026

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants

bolinfest commented Jul 8, 2026 •

edited

Loading