From 6670ecc637ae269f9855bad329a404a77c74d175 Mon Sep 17 00:00:00 2001 From: Cooper Gamble Date: Fri, 17 Apr 2026 15:42:08 +0000 Subject: [PATCH] [codex-login] clear refresh token after terminal refresh failures [ci changed_files] Co-authored-by: Codex --- codex-rs/app-server/tests/suite/auth.rs | 4 +- codex-rs/app-server/tests/suite/v2/account.rs | 81 +++++ .../app-server/tests/suite/v2/thread_fork.rs | 2 +- .../tests/suite/v2/thread_resume.rs | 2 +- .../app-server/tests/suite/v2/thread_start.rs | 2 +- codex-rs/login/src/auth/auth_tests.rs | 276 ++++++++++++++++++ codex-rs/login/src/auth/manager.rs | 134 +++++++-- codex-rs/login/tests/suite/auth_refresh.rs | 72 ++--- 8 files changed, 515 insertions(+), 58 deletions(-) diff --git a/codex-rs/app-server/tests/suite/auth.rs b/codex-rs/app-server/tests/suite/auth.rs index b35a6fcba1..5bb0aabe13 100644 --- a/codex-rs/app-server/tests/suite/auth.rs +++ b/codex-rs/app-server/tests/suite/auth.rs @@ -310,7 +310,7 @@ async fn get_auth_status_omits_token_after_permanent_refresh_failure() -> Result assert_eq!( status, GetAuthStatusResponse { - auth_method: Some(AuthMode::Chatgpt), + auth_method: None, auth_token: None, requires_openai_auth: Some(true), } @@ -394,7 +394,7 @@ async fn get_auth_status_omits_token_after_proactive_refresh_failure() -> Result assert_eq!( status, GetAuthStatusResponse { - auth_method: Some(AuthMode::Chatgpt), + auth_method: None, auth_token: None, requires_openai_auth: Some(true), } diff --git a/codex-rs/app-server/tests/suite/v2/account.rs b/codex-rs/app-server/tests/suite/v2/account.rs index f798122635..a46fd47652 100644 --- a/codex-rs/app-server/tests/suite/v2/account.rs +++ b/codex-rs/app-server/tests/suite/v2/account.rs @@ -29,6 +29,7 @@ use codex_app_server_protocol::ServerRequest; use codex_app_server_protocol::TurnCompletedNotification; use codex_app_server_protocol::TurnStatus; use codex_config::types::AuthCredentialsStoreMode; +use codex_login::REFRESH_TOKEN_URL_OVERRIDE_ENV_VAR; use codex_login::login_with_api_key; use codex_protocol::account::PlanType as AccountPlanType; use core_test_support::responses; @@ -1591,6 +1592,86 @@ async fn get_account_with_chatgpt() -> Result<()> { Ok(()) } +#[tokio::test] +async fn get_account_returns_no_account_after_permanent_refresh_failure() -> Result<()> { + let codex_home = TempDir::new()?; + create_config_toml( + codex_home.path(), + CreateConfigTomlParams { + requires_openai_auth: Some(true), + ..Default::default() + }, + )?; + write_chatgpt_auth( + codex_home.path(), + ChatGptAuthFixture::new("stale-access-token") + .refresh_token("stale-refresh-token") + .account_id("acct_123") + .email("user@example.com") + .plan_type("pro"), + AuthCredentialsStoreMode::File, + )?; + + 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_reused" + } + }))) + .expect(1) + .mount(&server) + .await; + + let refresh_url = format!("{}/oauth/token", server.uri()); + let mut mcp = McpProcess::new_with_env( + codex_home.path(), + &[ + ("OPENAI_API_KEY", None), + ( + REFRESH_TOKEN_URL_OVERRIDE_ENV_VAR, + Some(refresh_url.as_str()), + ), + ], + ) + .await?; + timeout(DEFAULT_READ_TIMEOUT, mcp.initialize()).await??; + + let request_id = mcp + .send_get_account_request(GetAccountParams { + refresh_token: true, + }) + .await?; + let resp: JSONRPCResponse = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(request_id)), + ) + .await??; + let received: GetAccountResponse = to_response(resp)?; + let expected = GetAccountResponse { + account: None, + requires_openai_auth: true, + }; + assert_eq!(received, expected); + + let second_request_id = mcp + .send_get_account_request(GetAccountParams { + refresh_token: true, + }) + .await?; + let second_resp: JSONRPCResponse = timeout( + DEFAULT_READ_TIMEOUT, + mcp.read_stream_until_response_message(RequestId::Integer(second_request_id)), + ) + .await??; + let second_received: GetAccountResponse = to_response(second_resp)?; + assert_eq!(second_received, expected); + + server.verify().await; + Ok(()) +} + #[tokio::test] async fn get_account_with_chatgpt_missing_plan_claim_returns_unknown() -> Result<()> { let codex_home = TempDir::new()?; diff --git a/codex-rs/app-server/tests/suite/v2/thread_fork.rs b/codex-rs/app-server/tests/suite/v2/thread_fork.rs index f5bc5a73c9..1fa8778f9d 100644 --- a/codex-rs/app-server/tests/suite/v2/thread_fork.rs +++ b/codex-rs/app-server/tests/suite/v2/thread_fork.rs @@ -420,7 +420,7 @@ async fn thread_fork_surfaces_cloud_requirements_load_errors() -> Result<()> { "errorCode": "Auth", "action": "relogin", "statusCode": 401, - "detail": "Your access token could not be refreshed because your refresh token was revoked. Please log out and sign in again.", + "detail": "Your access token could not be refreshed because your refresh token was revoked. Please sign in again.", })) ); diff --git a/codex-rs/app-server/tests/suite/v2/thread_resume.rs b/codex-rs/app-server/tests/suite/v2/thread_resume.rs index edf792ca1d..8b6f51ac30 100644 --- a/codex-rs/app-server/tests/suite/v2/thread_resume.rs +++ b/codex-rs/app-server/tests/suite/v2/thread_resume.rs @@ -1832,7 +1832,7 @@ async fn thread_resume_surfaces_cloud_requirements_load_errors() -> Result<()> { "errorCode": "Auth", "action": "relogin", "statusCode": 401, - "detail": "Your access token could not be refreshed because your refresh token was revoked. Please log out and sign in again.", + "detail": "Your access token could not be refreshed because your refresh token was revoked. Please sign in again.", })) ); diff --git a/codex-rs/app-server/tests/suite/v2/thread_start.rs b/codex-rs/app-server/tests/suite/v2/thread_start.rs index 508c77f7ff..6988ec1ca2 100644 --- a/codex-rs/app-server/tests/suite/v2/thread_start.rs +++ b/codex-rs/app-server/tests/suite/v2/thread_start.rs @@ -656,7 +656,7 @@ async fn thread_start_surfaces_cloud_requirements_load_errors() -> Result<()> { "errorCode": "Auth", "action": "relogin", "statusCode": 401, - "detail": "Your access token could not be refreshed because your refresh token was revoked. Please log out and sign in again.", + "detail": "Your access token could not be refreshed because your refresh token was revoked. Please sign in again.", })) ); diff --git a/codex-rs/login/src/auth/auth_tests.rs b/codex-rs/login/src/auth/auth_tests.rs index 30244ac268..16085955d6 100644 --- a/codex-rs/login/src/auth/auth_tests.rs +++ b/codex-rs/login/src/auth/auth_tests.rs @@ -3,6 +3,7 @@ use crate::auth::storage::FileAuthStorage; use crate::auth::storage::get_auth_file; use crate::token_data::IdTokenInfo; use codex_app_server_protocol::AuthMode; +use codex_client::CodexHttpClient; use codex_protocol::account::PlanType as AccountPlanType; use codex_protocol::auth::KnownPlan as InternalKnownPlan; use codex_protocol::auth::PlanType as InternalPlanType; @@ -10,6 +11,7 @@ use codex_protocol::auth::PlanType as InternalPlanType; use base64::Engine; use codex_protocol::config_types::ForcedLoginMethod; use codex_protocol::config_types::ModelProviderAuthInfo; +use core_test_support::skip_if_no_network; use pretty_assertions::assert_eq; use serde::Serialize; use serde_json::json; @@ -18,6 +20,11 @@ use tempfile::TempDir; use tempfile::tempdir; use tokio::time::Duration; use tokio::time::timeout; +use wiremock::Mock; +use wiremock::MockServer; +use wiremock::ResponseTemplate; +use wiremock::matchers::method; +use wiremock::matchers::path; #[tokio::test] async fn refresh_without_id_token() { @@ -50,6 +57,205 @@ async fn refresh_without_id_token() { assert_eq!(tokens.refresh_token, "new-refresh-token"); } +#[test] +fn clear_refresh_token_preserves_auth_record() { + let codex_home = tempdir().unwrap(); + write_auth_file( + AuthFileParams { + openai_api_key: None, + chatgpt_plan_type: Some("pro".to_string()), + chatgpt_account_id: Some("account-123".to_string()), + }, + codex_home.path(), + ) + .expect("failed to write auth file"); + + let storage = create_auth_storage( + codex_home.path().to_path_buf(), + AuthCredentialsStoreMode::File, + ); + let mut expected = storage + .load() + .expect("auth should load") + .expect("auth should exist"); + expected + .tokens + .as_mut() + .expect("tokens should exist") + .refresh_token + .clear(); + + assert!( + super::clear_refresh_token(&storage).expect("refresh token should clear"), + "first clear should report a storage change" + ); + let actual = storage + .load() + .expect("auth should load") + .expect("auth should exist"); + assert_eq!(actual, expected); + assert!( + !super::clear_refresh_token(&storage).expect("second clear should succeed"), + "second clear should be a no-op" + ); +} + +#[test] +fn refresh_failure_clear_policy_matches_terminal_client_errors() { + assert!(super::should_clear_refresh_token_after_refresh_failure( + reqwest::StatusCode::BAD_REQUEST + )); + assert!(super::should_clear_refresh_token_after_refresh_failure( + reqwest::StatusCode::UNAUTHORIZED + )); + assert!(!super::should_clear_refresh_token_after_refresh_failure( + reqwest::StatusCode::TOO_MANY_REQUESTS + )); + assert!(!super::should_clear_refresh_token_after_refresh_failure( + reqwest::StatusCode::INTERNAL_SERVER_ERROR + )); +} + +#[tokio::test] +async fn token_refresh_treats_terminal_client_errors_as_permanent() { + skip_if_no_network!(); + + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/oauth/token")) + .respond_with(ResponseTemplate::new(400).set_body_json(json!({ + "error": { + "code": "refresh_token_expired" + } + }))) + .expect(1) + .mount(&server) + .await; + + let client = CodexHttpClient::new(reqwest::Client::new()); + let endpoint = format!("{}/oauth/token", server.uri()); + let result = + request_chatgpt_token_refresh("test-refresh-token".to_string(), &client, &endpoint).await; + let err = match result { + Ok(_) => panic!("terminal client error should fail"), + Err(err) => err, + }; + + assert_eq!(err.failed_reason(), Some(RefreshTokenFailedReason::Expired)); + server.verify().await; +} + +#[tokio::test] +async fn token_refresh_treats_rate_limits_as_transient() { + skip_if_no_network!(); + + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/oauth/token")) + .respond_with(ResponseTemplate::new(429).set_body_json(json!({ + "error": { + "message": "slow down" + } + }))) + .expect(1) + .mount(&server) + .await; + + let client = CodexHttpClient::new(reqwest::Client::new()); + let endpoint = format!("{}/oauth/token", server.uri()); + let result = + request_chatgpt_token_refresh("test-refresh-token".to_string(), &client, &endpoint).await; + let err = match result { + Ok(_) => panic!("rate limit should fail"), + Err(err) => err, + }; + + assert_eq!(err.failed_reason(), None); + assert!( + err.to_string().contains("429"), + "transient error should include response status" + ); + server.verify().await; +} + +#[tokio::test] +#[serial(refresh_token_url_override)] +async fn refresh_token_clears_persisted_refresh_token_after_terminal_client_error() { + skip_if_no_network!(); + + let codex_home = tempdir().unwrap(); + write_auth_file( + AuthFileParams { + openai_api_key: None, + chatgpt_plan_type: Some("pro".to_string()), + chatgpt_account_id: Some("account-123".to_string()), + }, + codex_home.path(), + ) + .expect("failed to write auth file"); + + let storage = create_auth_storage( + codex_home.path().to_path_buf(), + AuthCredentialsStoreMode::File, + ); + let mut auth_dot_json = storage + .load() + .expect("auth should load") + .expect("auth should exist"); + auth_dot_json + .tokens + .as_mut() + .expect("tokens should exist") + .account_id = Some("account-123".to_string()); + storage.save(&auth_dot_json).expect("auth should save"); + + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/oauth/token")) + .respond_with(ResponseTemplate::new(400).set_body_json(json!({ + "error": { + "code": "refresh_token_expired" + } + }))) + .expect(1) + .mount(&server) + .await; + + let endpoint = format!("{}/oauth/token", server.uri()); + let _guard = EnvVarGuard::set(REFRESH_TOKEN_URL_OVERRIDE_ENV_VAR, &endpoint); + let manager = AuthManager::shared( + codex_home.path().to_path_buf(), + /*enable_codex_api_key_env*/ false, + AuthCredentialsStoreMode::File, + ); + + let err = manager + .refresh_token() + .await + .expect_err("terminal client error should fail refresh"); + assert_eq!(err.failed_reason(), Some(RefreshTokenFailedReason::Expired)); + + let stored = storage + .load() + .expect("auth should load") + .expect("auth should exist"); + assert_eq!( + stored.tokens.expect("tokens should exist").refresh_token, + "" + ); + + assert!( + manager.auth_cached().is_none(), + "manager should clear cached auth after a terminal refresh failure" + ); + + manager + .refresh_token() + .await + .expect("cleared auth refresh should be a no-op without another server request"); + server.verify().await; +} + #[test] fn login_with_api_key_overwrites_existing_auth_json() { let dir = tempdir().unwrap(); @@ -88,6 +294,76 @@ fn missing_auth_json_returns_none() { assert_eq!(auth, None); } +#[test] +fn managed_chatgpt_auth_with_missing_refresh_token_loads_as_none() { + let codex_home = tempdir().unwrap(); + write_auth_file( + AuthFileParams { + openai_api_key: None, + chatgpt_plan_type: Some("pro".to_string()), + chatgpt_account_id: Some("account-123".to_string()), + }, + codex_home.path(), + ) + .expect("failed to write auth file"); + + let storage = create_auth_storage( + codex_home.path().to_path_buf(), + AuthCredentialsStoreMode::File, + ); + let mut auth_dot_json = storage + .load() + .expect("auth should load") + .expect("auth should exist"); + auth_dot_json + .tokens + .as_mut() + .expect("tokens should exist") + .refresh_token + .clear(); + storage + .save(&auth_dot_json) + .expect("auth should save with an empty refresh token"); + + let auth = super::load_auth( + codex_home.path(), + /*enable_codex_api_key_env*/ false, + AuthCredentialsStoreMode::File, + ) + .expect("load auth"); + assert_eq!(auth, None); +} + +#[test] +fn external_chatgpt_auth_tokens_load_without_refresh_token() { + let codex_home = tempdir().unwrap(); + let access_token = fake_jwt_for_auth_file_params(&AuthFileParams { + openai_api_key: None, + chatgpt_plan_type: Some("pro".to_string()), + chatgpt_account_id: Some("account-123".to_string()), + }) + .expect("failed to build access token"); + + super::login_with_chatgpt_auth_tokens( + codex_home.path(), + &access_token, + "account-123", + Some("pro"), + ) + .expect("external auth tokens should save"); + + let auth = super::load_auth( + codex_home.path(), + /*enable_codex_api_key_env*/ false, + AuthCredentialsStoreMode::File, + ) + .expect("load auth") + .expect("auth should be available"); + let tokens = auth.get_token_data().expect("token data should exist"); + assert_eq!(auth.api_auth_mode(), ApiAuthMode::ChatgptAuthTokens); + assert_eq!(tokens.refresh_token, ""); +} + #[tokio::test] #[serial(codex_api_key)] async fn pro_account_with_no_api_key_uses_chatgpt_auth() { diff --git a/codex-rs/login/src/auth/manager.rs b/codex-rs/login/src/auth/manager.rs index c67d73fedf..8849fdbcc0 100644 --- a/codex-rs/login/src/auth/manager.rs +++ b/codex-rs/login/src/auth/manager.rs @@ -80,12 +80,14 @@ impl PartialEq for CodexAuth { const TOKEN_REFRESH_INTERVAL: i64 = 8; -const REFRESH_TOKEN_EXPIRED_MESSAGE: &str = "Your access token could not be refreshed because your refresh token has expired. Please log out and sign in again."; -const REFRESH_TOKEN_REUSED_MESSAGE: &str = "Your access token could not be refreshed because your refresh token was already used. Please log out and sign in again."; -const REFRESH_TOKEN_INVALIDATED_MESSAGE: &str = "Your access token could not be refreshed because your refresh token was revoked. Please log out and sign in again."; +const REFRESH_TOKEN_EXPIRED_MESSAGE: &str = "Your access token could not be refreshed because your refresh token has expired. Please sign in again."; +const REFRESH_TOKEN_REUSED_MESSAGE: &str = "Your access token could not be refreshed because your refresh token was already used. Please sign in again."; +const REFRESH_TOKEN_INVALIDATED_MESSAGE: &str = "Your access token could not be refreshed because your refresh token was revoked. Please sign in again."; const REFRESH_TOKEN_UNKNOWN_MESSAGE: &str = - "Your access token could not be refreshed. Please log out and sign in again."; + "Your access token could not be refreshed. Please sign in again."; const REFRESH_TOKEN_ACCOUNT_MISMATCH_MESSAGE: &str = "Your access token could not be refreshed because you have since logged out or signed in to another account. Please sign in again."; +const REFRESH_TOKEN_MISSING_MESSAGE: &str = + "Your access token could not be refreshed. Please sign in again."; const REFRESH_TOKEN_URL: &str = "https://auth.openai.com/oauth/token"; pub(super) const REVOKE_TOKEN_URL: &str = "https://auth.openai.com/oauth/revoke"; pub const REFRESH_TOKEN_URL_OVERRIDE_ENV_VAR: &str = "CODEX_REFRESH_TOKEN_URL_OVERRIDE"; @@ -688,6 +690,10 @@ fn load_auth( AuthCredentialsStoreMode::Ephemeral, ); if let Some(auth_dot_json) = ephemeral_storage.load()? { + if managed_chatgpt_auth_is_missing_refresh_token(&auth_dot_json) { + tracing::info!("Ignoring managed ChatGPT auth because the refresh token is missing."); + return Ok(None); + } let auth = build_auth(auth_dot_json, AuthCredentialsStoreMode::Ephemeral)?; return Ok(Some(auth)); } @@ -703,11 +709,26 @@ fn load_auth( Some(auth) => auth, None => return Ok(None), }; + if managed_chatgpt_auth_is_missing_refresh_token(&auth_dot_json) { + tracing::info!("Ignoring managed ChatGPT auth because the refresh token is missing."); + return Ok(None); + } let auth = build_auth(auth_dot_json, auth_credentials_store_mode)?; Ok(Some(auth)) } +fn managed_chatgpt_auth_is_missing_refresh_token(auth_dot_json: &AuthDotJson) -> bool { + if auth_dot_json.resolved_mode() != ApiAuthMode::Chatgpt { + return false; + } + + match auth_dot_json.tokens.as_ref() { + Some(tokens) => tokens.refresh_token.is_empty(), + None => true, + } +} + // Persist refreshed tokens into auth storage and update last_refresh. fn persist_tokens( storage: &Arc, @@ -734,11 +755,28 @@ fn persist_tokens( Ok(auth_dot_json) } +fn clear_refresh_token(storage: &Arc) -> std::io::Result { + let Some(mut auth_dot_json) = storage.load()? else { + return Ok(false); + }; + let Some(tokens) = auth_dot_json.tokens.as_mut() else { + return Ok(false); + }; + if tokens.refresh_token.is_empty() { + return Ok(false); + } + + tokens.refresh_token.clear(); + storage.save(&auth_dot_json)?; + Ok(true) +} + // Requests refreshed ChatGPT OAuth tokens from the auth service using a refresh token. // The caller is responsible for persisting any returned tokens. async fn request_chatgpt_token_refresh( refresh_token: String, client: &CodexHttpClient, + endpoint: &str, ) -> Result { let refresh_request = RefreshRequest { client_id: CLIENT_ID, @@ -746,11 +784,9 @@ async fn request_chatgpt_token_refresh( refresh_token, }; - let endpoint = refresh_token_endpoint(); - // Use shared client factory to include standard headers let response = client - .post(endpoint.as_str()) + .post(endpoint) .header("Content-Type", "application/json") .json(&refresh_request) .send() @@ -767,8 +803,8 @@ async fn request_chatgpt_token_refresh( } else { let body = response.text().await.unwrap_or_default(); tracing::error!("Failed to refresh token: {status}: {body}"); - if status == StatusCode::UNAUTHORIZED { - let failed = classify_refresh_token_failure(&body); + if should_clear_refresh_token_after_refresh_failure(status) { + let failed = classify_refresh_token_failure(status, &body); Err(RefreshTokenError::Permanent(failed)) } else { let message = try_parse_error_message(&body); @@ -779,7 +815,11 @@ async fn request_chatgpt_token_refresh( } } -fn classify_refresh_token_failure(body: &str) -> RefreshTokenFailedError { +fn should_clear_refresh_token_after_refresh_failure(status: StatusCode) -> bool { + status.is_client_error() && status != StatusCode::TOO_MANY_REQUESTS +} + +fn classify_refresh_token_failure(status: StatusCode, body: &str) -> RefreshTokenFailedError { let code = extract_refresh_token_error_code(body); let normalized_code = code.as_deref().map(str::to_ascii_lowercase); @@ -792,9 +832,10 @@ fn classify_refresh_token_failure(body: &str) -> RefreshTokenFailedError { if reason == RefreshTokenFailedReason::Other { tracing::warn!( + status = status.as_u16(), backend_code = normalized_code.as_deref(), backend_body = body, - "Encountered unknown 401 response while refreshing token" + "Encountered unknown client error response while refreshing token" ); } @@ -1338,6 +1379,9 @@ impl AuthManager { && let Err(err) = self.refresh_token().await { tracing::error!("Failed to refresh token: {}", err); + if matches!(err, RefreshTokenError::Permanent(_)) { + return self.auth_cached(); + } return Some(auth); } self.auth_cached() @@ -1364,6 +1408,13 @@ impl AuthManager { let new_account_id = new_auth.as_ref().and_then(CodexAuth::get_account_id); if new_account_id.as_deref() != Some(expected_account_id) { + if new_auth.is_none() { + tracing::info!( + "Reloading auth to unauthenticated because no matching auth is available." + ); + self.set_cached_auth(None); + return ReloadOutcome::ReloadedChanged; + } let found_account_id = new_account_id.as_deref().unwrap_or("unknown"); tracing::info!( "Skipping auth reload due to account id mismatch (expected: {expected_account_id}, found: {found_account_id})" @@ -1573,19 +1624,29 @@ impl AuthManager { /// token is the same as the cached, then ask the token authority to refresh. pub async fn refresh_token(&self) -> Result<(), RefreshTokenError> { let _refresh_guard = self.refresh_lock.lock().await; - let auth_before_reload = self.auth_cached(); - if auth_before_reload - .as_ref() - .is_some_and(CodexAuth::is_api_key_auth) - { + let Some(auth_before_reload) = self.auth_cached() else { + return Ok(()); + }; + if auth_before_reload.is_api_key_auth() { return Ok(()); } - let expected_account_id = auth_before_reload - .as_ref() - .and_then(CodexAuth::get_account_id); + let expected_account_id = auth_before_reload.get_account_id(); match self.reload_if_account_id_matches(expected_account_id.as_deref()) { ReloadOutcome::ReloadedChanged => { + let auth_has_empty_refresh_token = + self.auth_cached().as_ref().is_some_and(|auth| match auth { + CodexAuth::Chatgpt(chatgpt_auth) => chatgpt_auth + .current_token_data() + .is_some_and(|token_data| token_data.refresh_token.is_empty()), + CodexAuth::ApiKey(_) | CodexAuth::ChatgptAuthTokens(_) => false, + }); + if auth_has_empty_refresh_token { + tracing::info!( + "Continuing token refresh because auth changed to an empty refresh token." + ); + return self.refresh_token_from_authority_impl().await; + } tracing::info!("Skipping token refresh because auth changed after guarded reload."); Ok(()) } @@ -1768,7 +1829,40 @@ impl AuthManager { auth: &ChatgptAuth, refresh_token: String, ) -> Result<(), RefreshTokenError> { - let refresh_response = request_chatgpt_token_refresh(refresh_token, auth.client()).await?; + if refresh_token.is_empty() { + self.reload(); + return Err(RefreshTokenError::Permanent(RefreshTokenFailedError::new( + RefreshTokenFailedReason::Other, + REFRESH_TOKEN_MISSING_MESSAGE.to_string(), + ))); + } + + let endpoint = refresh_token_endpoint(); + let refresh_response = match request_chatgpt_token_refresh( + refresh_token, + auth.client(), + endpoint.as_str(), + ) + .await + { + Ok(response) => response, + Err(error @ RefreshTokenError::Permanent(_)) => { + match clear_refresh_token(auth.storage()) { + Ok(true) => { + tracing::warn!("Cleared refresh token after terminal refresh failure"); + } + Ok(false) => {} + Err(err) => { + tracing::warn!( + "Failed to clear refresh token after terminal refresh failure: {err}" + ); + } + } + self.reload(); + return Err(error); + } + Err(error) => return Err(error), + }; persist_tokens( auth.storage(), diff --git a/codex-rs/login/tests/suite/auth_refresh.rs b/codex-rs/login/tests/suite/auth_refresh.rs index d754c9589d..bc5a0fd4b8 100644 --- a/codex-rs/login/tests/suite/auth_refresh.rs +++ b/codex-rs/login/tests/suite/auth_refresh.rs @@ -542,17 +542,23 @@ async fn refresh_token_returns_permanent_error_for_expired_refresh_token() -> Re .context("refresh should fail")?; assert_eq!(err.failed_reason(), Some(RefreshTokenFailedReason::Expired)); + let mut expected_stored = initial_auth.clone(); + expected_stored + .tokens + .as_mut() + .context("tokens should exist")? + .refresh_token + .clear(); let stored = ctx.load_auth()?; - assert_eq!(stored, initial_auth); - let cached_auth = ctx - .auth_manager - .auth() - .await - .context("auth should remain cached")?; - let cached = cached_auth - .get_token_data() - .context("token data should remain cached")?; - assert_eq!(cached, initial_tokens); + assert_eq!(stored, expected_stored); + assert!( + ctx.auth_manager.auth_cached().is_none(), + "auth should be cleared after a terminal refresh failure" + ); + assert!( + ctx.auth_manager.auth().await.is_none(), + "terminal refresh failure should be observed as logged out" + ); server.verify().await; Ok(()) @@ -598,28 +604,24 @@ async fn refresh_token_does_not_retry_after_permanent_failure() -> Result<()> { Some(RefreshTokenFailedReason::Exhausted) ); - let second_err = ctx - .auth_manager + ctx.auth_manager .refresh_token() .await - .err() - .context("second refresh should fail without retrying")?; - assert_eq!( - second_err.failed_reason(), - Some(RefreshTokenFailedReason::Exhausted) - ); + .context("second refresh should be a no-op without retrying")?; + let mut expected_stored = initial_auth.clone(); + expected_stored + .tokens + .as_mut() + .context("tokens should exist")? + .refresh_token + .clear(); let stored = ctx.load_auth()?; - assert_eq!(stored, initial_auth); - let cached_auth = ctx - .auth_manager - .auth() - .await - .context("auth should remain cached")?; - let cached = cached_auth - .get_token_data() - .context("token data should remain cached")?; - assert_eq!(cached, initial_tokens); + assert_eq!(stored, expected_stored); + assert!( + ctx.auth_manager.auth_cached().is_none(), + "auth should remain cleared after the no-op refresh" + ); server.verify().await; Ok(()) @@ -627,7 +629,7 @@ async fn refresh_token_does_not_retry_after_permanent_failure() -> Result<()> { #[serial_test::serial(auth_refresh)] #[tokio::test] -async fn refresh_token_reloads_changed_auth_after_permanent_failure() -> Result<()> { +async fn new_login_reloads_auth_after_permanent_failure() -> Result<()> { skip_if_no_network!(Ok(())); let server = MockServer::start().await; @@ -664,6 +666,10 @@ async fn refresh_token_reloads_changed_auth_after_permanent_failure() -> Result< first_err.failed_reason(), Some(RefreshTokenFailedReason::Exhausted) ); + assert!( + ctx.auth_manager.auth_cached().is_none(), + "auth should be cleared after the terminal refresh failure" + ); let fresh_refresh = Utc::now() - Duration::hours(1); let disk_tokens = build_tokens("disk-access-token", "disk-refresh-token"); @@ -680,10 +686,10 @@ async fn refresh_token_reloads_changed_auth_after_permanent_failure() -> Result< AuthCredentialsStoreMode::File, )?; - ctx.auth_manager - .refresh_token() - .await - .context("refresh should reload changed auth without retrying")?; + assert!( + ctx.auth_manager.reload(), + "explicit login reload should publish the new auth state" + ); let stored = ctx.load_auth()?; assert_eq!(stored, disk_auth);