Conversation
![chatgpt-codex-connector[bot]](https://avatars.githubusercontent.com/in/1144995?s=60&v=4){kind=small}
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 4127419694
ℹ️ 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".
codex-rs/login/src/auth/manager.rs
Outdated
| if let Some(auth) = self.resolve_external_api_key_auth().await { | ||
| return Some(auth); | ||
| } | ||
|
|
||
| let auth = self.auth_cached()?; |
There was a problem hiding this comment.
When a manager is created via with_external_bearer_refresher, auth() still falls back to auth_cached() if resolve_external_api_key_auth() returns None (including resolver errors). Because the derived manager shares inner with the base manager, this can silently attach the base OpenAI credential to requests that were supposed to use external provider bearer auth, which is both a security leak and a behavior regression whenever provider token resolution is unavailable.
Useful? React with 👍 / 👎.
codex-rs/login/src/auth/manager.rs
Outdated
| let refreshed = handle.refresher.refresh(context).await?; | ||
| if handle.kind == ExternalAuthKind::Bearer { | ||
| return Ok(()); |
There was a problem hiding this comment.
In the bearer path, refresh_external_auth() calls refresh() but then immediately returns without using refreshed.access_token. For refresher implementations where resolve() does not instantly reflect the refresh side effect, the retry after a reported successful recovery will continue using the stale token and keep failing with 401. The refreshed token should be made visible to subsequent auth() calls in this manager.
Useful? React with 👍 / 👎.
bolinfest
force-pushed
the
pr16287
branch
4 times, most recently
from
346440c to
4a42882
Compare
## Summary `ExternalAuthRefresher` was still shaped around external ChatGPT auth: `ExternalAuthTokens` always implied ChatGPT account metadata even when a caller only needed a bearer token. This PR generalizes that contract so bearer-only sources are first-class, while keeping the existing ChatGPT paths strict anywhere we persist or rebuild ChatGPT auth state. ## Motivation This is the first step toward #15189. The follow-on provider-auth work needs one shared external-auth contract that can do both of these things: - resolve the current bearer token before a request is sent - return a refreshed bearer token after a `401` That should not require a second token result type just because there is no ChatGPT account metadata attached. ## What Changed - change `ExternalAuthTokens` to carry `access_token` plus optional `ExternalAuthChatgptMetadata` - add helper constructors for bearer-only tokens and ChatGPT-backed tokens - add `ExternalAuthRefresher::resolve()` with a default no-op implementation so refreshers can optionally provide the current token before a request is sent - keep ChatGPT-only persistence strict by continuing to require ChatGPT metadata anywhere the login layer seeds or reloads ChatGPT auth state - update the app-server bridge to construct the new token shape for external ChatGPT auth refreshes ## Testing - `cargo test -p codex-login` --- [//]: # (BEGIN SAPLING FOOTER) Stack created with [Sapling](https://sapling-scm.com). Best reviewed with [ReviewStack](https://reviewstack.dev/openai/codex/pull/16286). * #16288 * #16287 * __->__ #16286
1 participant
Summary
AuthManagerandUnauthorizedRecoveryalready own token resolution and staged401recovery. The missing piece for provider auth was a bearer-only mode that still fit that design, instead of pushing a second auth abstraction intocodex-core.This PR keeps the design centered on
AuthManager: it teachescodex-loginhow to own external bearer auth directly so later provider work can keep callingAuthManager.auth()andUnauthorizedRecovery.Motivation
This is the middle layer for #15189.
The intended design is still:
AuthManagerencapsulates token storage and refreshUnauthorizedRecoverypowers staged401recoveryAuthManager.auth()This PR makes that possible for provider-backed bearer tokens by adding a bearer-only auth mode inside
AuthManagerinstead of building parallel request-auth plumbing incore.What Changed
ModelProviderAuthInfointocodex-protocolsocoreandloginshare one config shapelogin/src/auth/external_bearer.rs, which runs the configured command, caches the bearer token in memory, and refreshes it after401AuthManager::external_bearer_only(...)for provider-scoped request paths that should use command-backed bearer auth without mutating the shared OpenAI auth managerAuthManager::shared_with_external_chatgpt_auth_refresher(...)and rename the otherAuthManagerhelpers that only apply to external ChatGPT auth so the ChatGPT-only path is explicit at the call siteauth.jsonTesting
cargo test -p codex-logincargo test -p codex-protocolStack created with Sapling. Best reviewed with ReviewStack.