Fix MCP OAuth refresh-token races across Codex processes#12815
Fix MCP OAuth refresh-token races across Codex processes#12815etraut-openai wants to merge 4 commits intomainfrom
Conversation
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: c47fd545dd
ℹ️ 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 review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: 78249f7fce
ℹ️ 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 review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7479ee1cc
ℹ️ 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".
| self.persist_if_needed().await | ||
| } | ||
| GuardedRefreshOutcome::MissingOrInvalid => self.apply_runtime_credentials(None).await, | ||
| GuardedRefreshOutcome::ReloadFailed => Ok(()), |
There was a problem hiding this comment.
Refresh tokens when guarded reload fails
refresh_if_needed returns Ok(()) on ReloadFailed, even though this branch is reached only after token_needs_refresh(...) is true. If keyring/file reload fails transiently, the runtime keeps near-expired credentials and skips both refresh and logout, so subsequent MCP calls can keep sending stale bearer tokens and fail with 401s.
Useful? React with 👍 / 👎.
|
@codex review |
There was a problem hiding this comment.
💡 Codex Review
Here are some automated review suggestions for this pull request.
Reviewed commit: e7479ee1cc
ℹ️ 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".
| @@ -595,22 +604,20 @@ async fn create_oauth_transport_and_runtime( | |||
| )> { | |||
| let http_client = | |||
There was a problem hiding this comment.
would've been very nice to reuse default_client we use elsewhere, it sets all the right headers.
| } | ||
| }; | ||
|
|
||
| if let Some(runtime) = &oauth_persistor |
There was a problem hiding this comment.
nit: oauth_persistor is a bad name for this type at this point. McpAuthManager maybe
| token_response: Some(credentials.token_response.0.clone()), | ||
| }) | ||
| .await?; | ||
| guard |
There was a problem hiding this comment.
Does client id ever change on refresh?
| } | ||
| } | ||
|
|
||
| fn load_oauth_tokens_for_guarded_refresh_with_keyring_fallback<K: KeyringStore>( |
There was a problem hiding this comment.
can we avoid a separate set of reading method for guarded refresh and reuse existing read methods?
| } | ||
| } | ||
|
|
||
| fn oauth_tokens_equal_for_refresh( |
There was a problem hiding this comment.
do we need this much logic? can we just compare the access token?
| } | ||
|
|
||
| #[derive(Debug, PartialEq)] | ||
| enum GuardedRefreshPersistedCredentials { |
There was a problem hiding this comment.
can this be Option<StoredOAuthTokens> ?
| server_name: String, | ||
| url: String, | ||
| authorization_manager: Arc<Mutex<AuthorizationManager>>, | ||
| runtime_credentials: InMemoryCredentialStore, |
There was a problem hiding this comment.
unfortunate that we have multiple creadential storages to keep in sync. Any chance we can reuse runtime_credentials as last_credentials?
pakrym-oai
left a comment
There was a problem hiding this comment.
The core logic looks good but I wonder if we can collapse reader methods.
|
Closing this pull request because it has had no updates for more than 14 days. If you plan to continue working on it, feel free to reopen or open a new PR. |
Addresses #12755
Token refresh for MCP OAuth suffered from the same problem that our main Codex ChatGPT auth had until recently. This PR updates the MCP logic to mirror the solution in place for ChatGPT auth token refresh.
MCP OAuth credentials are shared across processes via keyring or
CODEX_HOME/.credentials.json, but each process kept its own in-memorysnapshot and refreshed from that local copy. That meant one process could
rotate the refresh token while another still tried to use the stale token,
causing
refresh_token_reusedfailures.This changes the MCP refresh path to use a guarded reload flow modeled on
the main ChatGPT auth fix:
refresh-specific equality
and skip the local refresh
rotated credentials
live runtime auth instead of using a stale refresh token
To keep the running RMCP auth stack in sync without reconnecting, the client
now owns the
AuthorizationManagercredential store and can update the liveruntime in place when reloaded credentials win.
Also adds:
codex-rs/rmcp-client/src/oauth.rscodex-rs/core/tests/suite/rmcp_client.rs