From f55f5c258f0588dd9acc38811a2260d5debc9bd3 Mon Sep 17 00:00:00 2001 From: Celia Chen Date: Mon, 23 Mar 2026 12:07:59 -0700 Subject: [PATCH] Fix: proactive auth refresh to reload guarded disk state first (#15357) ## Summary Fix a managed ChatGPT auth bug where a stale Codex process could proactively refresh using an old in-memory refresh token even after another process had already rotated auth on disk. This changes the proactive `AuthManager::auth()` path to reuse the existing guarded `refresh_token()` flow instead of calling the refresh endpoint directly from cached auth state. ## Original Issue Users reported repeated `codexd` log lines like: ```text ERROR codex_core::auth: Failed to refresh token: error sending request for url (https://auth.openai.com/oauth/token) ``` In practice this showed up most often when multiple `codexd` processes were left running. Killing the extra processes stopped the noise, which suggested the issue was caused by stale auth state across processes rather than invalid user credentials. ## Diagnosis The bug was in the proactive refresh path used by `AuthManager::auth()`: - Process A could refresh successfully, rotate refresh token `R0` to `R1`, and persist the updated auth state plus `last_refresh` to disk. - Process B could keep an older auth snapshot cached in memory, still holding `R0` and the old `last_refresh`. - Later, when Process B called `auth()`, it checked staleness from its cached in-memory auth instead of first reloading from disk. - Because that cached `last_refresh` was stale, Process B would proactively call `/oauth/token` with stale refresh token `R0`. - On failure, `auth()` logged the refresh error but kept returning the same stale cached auth, so repeated `auth()` calls could keep retrying with dead state. This differed from the existing unauthorized-recovery flow, which already did the safer thing: guarded reload from disk first, then refresh only if the on-disk auth was unchanged. ## What Changed - Switched proactive refresh in `AuthManager::auth()` to: - do a pure staleness check on cached auth - call `refresh_token()` when stale - return the original cached auth on genuine refresh failure, preserving existing outward behavior - Removed the direct proactive refresh-from-cached-state path - Added regression tests covering: - stale cached auth with newer same-account auth already on disk - the same scenario even when the refresh endpoint would fail if called ## Why This Fix `refresh_token()` already contains the right cross-process safety behavior: - guarded reload from disk - same-account verification - skip-refresh when another process already changed auth Reusing that path makes proactive refresh consistent with unauthorized recovery and prevents stale processes from trying to refresh already-rotated tokens. ## Testing Test shape: - create a fresh temp `CODEX_HOME` from `~/.codex/auth.json` - force `last_refresh` to an old timestamp so proactive refresh is required - start two long-lived helper processes against the same auth file - start `B` first so it caches stale auth and sleeps - start `A` second so it refreshes first - point both at a local mock `/oauth/token` server - inspect whether `B` makes a second refresh request with the stale in-memory token, or reloads the rotated token from disk ### Before the fix The repro showed the bug clearly: the mock server saw two refreshes with the same stale token, `A` rotated to a new token, and `B` still returned the stale token instead of reloading from disk. ```text POST /oauth/token refresh_token=rt_j6s0... POST /oauth/token refresh_token=rt_j6s0... B:cached_before=rt_j6s0... B:cached_after=rt_j6s0... B:returned=rt_j6s0... A:cached_before=rt_j6s0... A:cached_after=rotated-refresh-token-logged-run-v2 A:returned=rotated-refresh-token-logged-run-v2 ``` ### After the fix After the fix, the mock server saw only one refresh request. `A` refreshed once, and `B` started with the stale token but reloaded and returned the rotated token. ```text POST /oauth/token refresh_token=rt_j6s0... B:cached_before=rt_j6s0... B:cached_after=rotated-refresh-token-fix-branch B:returned=rotated-refresh-token-fix-branch A:cached_before=rt_j6s0... A:cached_after=rotated-refresh-token-fix-branch A:returned=rotated-refresh-token-fix-branch ``` This shows the new behavior: `A` refreshes once, then `B` reuses the updated auth from disk instead of making a second refresh request with the stale token. --- codex-rs/cloud-requirements/src/lib.rs | 95 +++++++++++++++---- codex-rs/core/tests/suite/auth_refresh.rs | 110 ++++++++++++++++++++++ codex-rs/login/src/auth/manager.rs | 26 ++--- 3 files changed, 196 insertions(+), 35 deletions(-) diff --git a/codex-rs/cloud-requirements/src/lib.rs b/codex-rs/cloud-requirements/src/lib.rs index e37a85dc1..fb0f62a34 100644 --- a/codex-rs/cloud-requirements/src/lib.rs +++ b/codex-rs/cloud-requirements/src/lib.rs @@ -883,6 +883,24 @@ mod tests { account_id: Option<&str>, access_token: &str, refresh_token: &str, + ) -> serde_json::Value { + chatgpt_auth_json_with_last_refresh( + plan_type, + chatgpt_user_id, + account_id, + access_token, + refresh_token, + "2025-01-01T00:00:00Z", + ) + } + + fn chatgpt_auth_json_with_last_refresh( + plan_type: &str, + chatgpt_user_id: Option<&str>, + account_id: Option<&str>, + access_token: &str, + refresh_token: &str, + last_refresh: &str, ) -> serde_json::Value { chatgpt_auth_json_with_mode( plan_type, @@ -890,6 +908,7 @@ mod tests { account_id, access_token, refresh_token, + last_refresh, None, ) } @@ -900,6 +919,7 @@ mod tests { account_id: Option<&str>, access_token: &str, refresh_token: &str, + last_refresh: &str, auth_mode: Option<&str>, ) -> serde_json::Value { let header = json!({ "alg": "none", "typ": "JWT" }); @@ -925,7 +945,7 @@ mod tests { "refresh_token": refresh_token, "account_id": account_id, }, - "last_refresh": "2025-01-01T00:00:00Z", + "last_refresh": last_refresh, }); if let Some(auth_mode) = auth_mode { auth_json["auth_mode"] = serde_json::Value::String(auth_mode.to_string()); @@ -1262,24 +1282,43 @@ enabled = false #[tokio::test] async fn fetch_cloud_requirements_recovers_after_unauthorized_reload() { - let auth = managed_auth_context( - "business", - Some("user-12345"), - Some("account-12345"), - "stale-access-token", - "test-refresh-token", - ); + let auth_home = tempdir().expect("tempdir"); write_auth_json( - auth._home.path(), - chatgpt_auth_json( + auth_home.path(), + chatgpt_auth_json_with_last_refresh( + "business", + Some("user-12345"), + Some("account-12345"), + "stale-access-token", + "test-refresh-token", + // Keep auth "fresh" so the first request hits unauthorized recovery + // instead of AuthManager::auth() proactively reloading from disk. + "3025-01-01T00:00:00Z", + ), + ) + .expect("write initial auth"); + let auth_manager = Arc::new(AuthManager::new( + auth_home.path().to_path_buf(), + false, + AuthCredentialsStoreMode::File, + )); + + write_auth_json( + auth_home.path(), + chatgpt_auth_json_with_last_refresh( "business", Some("user-12345"), Some("account-12345"), "fresh-access-token", "test-refresh-token", + "3025-01-01T00:00:00Z", ), ) .expect("write refreshed auth"); + let auth = ManagedAuthContext { + _home: auth_home, + manager: auth_manager, + }; let fetcher = Arc::new(TokenFetcher { expected_token: "fresh-access-token".to_string(), @@ -1314,24 +1353,41 @@ enabled = false #[tokio::test] async fn fetch_cloud_requirements_recovers_after_unauthorized_reload_updates_cache_identity() { - let auth = managed_auth_context( - "business", - Some("user-12345"), - Some("account-12345"), - "stale-access-token", - "test-refresh-token", - ); + let auth_home = tempdir().expect("tempdir"); write_auth_json( - auth._home.path(), - chatgpt_auth_json( + auth_home.path(), + chatgpt_auth_json_with_last_refresh( + "business", + Some("user-12345"), + Some("account-12345"), + "stale-access-token", + "test-refresh-token", + "3025-01-01T00:00:00Z", + ), + ) + .expect("write initial auth"); + let auth_manager = Arc::new(AuthManager::new( + auth_home.path().to_path_buf(), + false, + AuthCredentialsStoreMode::File, + )); + + write_auth_json( + auth_home.path(), + chatgpt_auth_json_with_last_refresh( "business", Some("user-99999"), Some("account-12345"), "fresh-access-token", "test-refresh-token", + "3025-01-01T00:00:00Z", ), ) .expect("write refreshed auth"); + let auth = ManagedAuthContext { + _home: auth_home, + manager: auth_manager, + }; let fetcher = Arc::new(TokenFetcher { expected_token: "fresh-access-token".to_string(), @@ -1432,6 +1488,7 @@ enabled = false Some("account-12345"), "test-access-token", "test-refresh-token", + "2025-01-01T00:00:00Z", Some("chatgptAuthTokens"), ), ) diff --git a/codex-rs/core/tests/suite/auth_refresh.rs b/codex-rs/core/tests/suite/auth_refresh.rs index 23ed87aa9..278124c63 100644 --- a/codex-rs/core/tests/suite/auth_refresh.rs +++ b/codex-rs/core/tests/suite/auth_refresh.rs @@ -381,6 +381,116 @@ async fn refreshes_token_when_last_refresh_is_stale() -> Result<()> { Ok(()) } +#[serial_test::serial(auth_refresh)] +#[tokio::test] +async fn auth_reloads_disk_auth_when_cached_auth_is_stale() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = MockServer::start().await; + + let ctx = RefreshTokenTestContext::new(&server)?; + let stale_refresh = Utc::now() - Duration::days(9); + let initial_tokens = build_tokens(INITIAL_ACCESS_TOKEN, INITIAL_REFRESH_TOKEN); + let initial_auth = AuthDotJson { + auth_mode: Some(AuthMode::Chatgpt), + openai_api_key: None, + tokens: Some(initial_tokens), + last_refresh: Some(stale_refresh), + }; + ctx.write_auth(&initial_auth)?; + + let fresh_refresh = Utc::now() - Duration::days(1); + let disk_tokens = build_tokens("disk-access-token", "disk-refresh-token"); + let disk_auth = AuthDotJson { + auth_mode: Some(AuthMode::Chatgpt), + openai_api_key: None, + tokens: Some(disk_tokens.clone()), + last_refresh: Some(fresh_refresh), + }; + save_auth( + ctx.codex_home.path(), + &disk_auth, + AuthCredentialsStoreMode::File, + )?; + + let cached_auth = ctx + .auth_manager + .auth() + .await + .context("auth should reload from disk")?; + let cached = cached_auth + .get_token_data() + .context("token data should reload from disk")?; + assert_eq!(cached, disk_tokens); + + let stored = ctx.load_auth()?; + assert_eq!(stored, disk_auth); + + let requests = server.received_requests().await.unwrap_or_default(); + assert!(requests.is_empty(), "expected no refresh token requests"); + + Ok(()) +} + +#[serial_test::serial(auth_refresh)] +#[tokio::test] +async fn auth_reloads_disk_auth_without_calling_expired_refresh_token() -> Result<()> { + skip_if_no_network!(Ok(())); + + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/oauth/token")) + .respond_with(ResponseTemplate::new(401).set_body_json(json!({ + "error": { + "code": "refresh_token_expired" + } + }))) + .expect(0) + .mount(&server) + .await; + + let ctx = RefreshTokenTestContext::new(&server)?; + let stale_refresh = Utc::now() - Duration::days(9); + let initial_tokens = build_tokens(INITIAL_ACCESS_TOKEN, INITIAL_REFRESH_TOKEN); + let initial_auth = AuthDotJson { + auth_mode: Some(AuthMode::Chatgpt), + openai_api_key: None, + tokens: Some(initial_tokens), + last_refresh: Some(stale_refresh), + }; + ctx.write_auth(&initial_auth)?; + + let fresh_refresh = Utc::now() - Duration::days(1); + let disk_tokens = build_tokens("disk-access-token", "disk-refresh-token"); + let disk_auth = AuthDotJson { + auth_mode: Some(AuthMode::Chatgpt), + openai_api_key: None, + tokens: Some(disk_tokens.clone()), + last_refresh: Some(fresh_refresh), + }; + save_auth( + ctx.codex_home.path(), + &disk_auth, + AuthCredentialsStoreMode::File, + )?; + + let cached_auth = ctx + .auth_manager + .auth() + .await + .context("auth should reload from disk")?; + let cached = cached_auth + .get_token_data() + .context("token data should reload from disk")?; + assert_eq!(cached, disk_tokens); + + let stored = ctx.load_auth()?; + assert_eq!(stored, disk_auth); + + server.verify().await; + Ok(()) +} + #[serial_test::serial(auth_refresh)] #[tokio::test] async fn refresh_token_returns_permanent_error_for_expired_refresh_token() -> Result<()> { diff --git a/codex-rs/login/src/auth/manager.rs b/codex-rs/login/src/auth/manager.rs index 1e4cd06d3..31860cd58 100644 --- a/codex-rs/login/src/auth/manager.rs +++ b/codex-rs/login/src/auth/manager.rs @@ -1090,10 +1090,13 @@ impl AuthManager { } /// Current cached auth (clone). May be `None` if not logged in or load failed. - /// Refreshes cached ChatGPT tokens if they are stale before returning. + /// For stale managed ChatGPT auth, first performs a guarded reload and then + /// refreshes only if the on-disk auth is unchanged. pub async fn auth(&self) -> Option { let auth = self.auth_cached()?; - if let Err(err) = self.refresh_if_stale(&auth).await { + if Self::is_stale_for_proactive_refresh(&auth) + && let Err(err) = self.refresh_token().await + { tracing::error!("Failed to refresh token: {}", err); return Some(auth); } @@ -1320,30 +1323,21 @@ impl AuthManager { self.auth_cached().as_ref().map(CodexAuth::auth_mode) } - async fn refresh_if_stale(&self, auth: &CodexAuth) -> Result { + fn is_stale_for_proactive_refresh(auth: &CodexAuth) -> bool { let chatgpt_auth = match auth { CodexAuth::Chatgpt(chatgpt_auth) => chatgpt_auth, - _ => return Ok(false), + _ => return false, }; let auth_dot_json = match chatgpt_auth.current_auth_json() { Some(auth_dot_json) => auth_dot_json, - None => return Ok(false), - }; - let tokens = match auth_dot_json.tokens { - Some(tokens) => tokens, - None => return Ok(false), + None => return false, }; let last_refresh = match auth_dot_json.last_refresh { Some(last_refresh) => last_refresh, - None => return Ok(false), + None => return false, }; - if last_refresh >= Utc::now() - chrono::Duration::days(TOKEN_REFRESH_INTERVAL) { - return Ok(false); - } - self.refresh_and_persist_chatgpt_token(chatgpt_auth, tokens.refresh_token) - .await?; - Ok(true) + last_refresh < Utc::now() - chrono::Duration::days(TOKEN_REFRESH_INTERVAL) } async fn refresh_external_auth(