From 41b65786aa3e243c10dbf09fd8b2a0349d0fd497 Mon Sep 17 00:00:00 2001 From: Cooper Gamble Date: Tue, 19 May 2026 22:10:27 +0000 Subject: [PATCH] [codex-core] skip refresh for revoked access tokens [ci changed_files] --- codex-rs/core/src/client.rs | 35 ++++++++- codex-rs/core/src/client_tests.rs | 80 +++++++++++++++++++ codex-rs/login/src/auth/manager.rs | 3 +- codex-rs/response-debug-context/src/lib.rs | 90 ++++++++++++++++++---- 4 files changed, 188 insertions(+), 20 deletions(-) diff --git a/codex-rs/core/src/client.rs b/codex-rs/core/src/client.rs index 07a1c2adf5..74da21769f 100644 --- a/codex-rs/core/src/client.rs +++ b/codex-rs/core/src/client.rs @@ -150,6 +150,8 @@ const RESPONSES_COMPACT_ENDPOINT: &str = "/responses/compact"; // period between stream events. const COMPACT_REQUEST_TIMEOUT_IDLE_MULTIPLIER: u32 = 4; const MEMORIES_SUMMARIZE_ENDPOINT: &str = "/memories/trace_summarize"; +const TOKEN_INVALIDATED_ERROR_CODE: &str = "token_invalidated"; +const TOKEN_REVOKED_ERROR_CODE: &str = "token_revoked"; #[cfg(test)] pub(crate) const WEBSOCKET_CONNECT_TIMEOUT: Duration = Duration::from_millis(DEFAULT_WEBSOCKET_CONNECT_TIMEOUT_MS); @@ -1979,7 +1981,12 @@ async fn handle_unauthorized( session_telemetry: &SessionTelemetry, ) -> Result { let debug = extract_response_debug_context(&transport); - if let Some(recovery) = auth_recovery + let revocation_recovery_reason = access_token_revocation_recovery_reason( + debug.auth_error_code.as_deref(), + debug.auth_error_type.as_deref(), + ); + if revocation_recovery_reason.is_none() + && let Some(recovery) = auth_recovery && recovery.has_next() { let mode = recovery.mode_name(); @@ -2057,13 +2064,18 @@ async fn handle_unauthorized( }; } - let (mode, phase, recovery_reason) = match auth_recovery.as_ref() { - Some(recovery) => ( + let (mode, phase, recovery_reason) = match (auth_recovery.as_ref(), revocation_recovery_reason) + { + (Some(recovery), Some(reason)) => { + (recovery.mode_name(), recovery.step_name(), Some(reason)) + } + (None, Some(reason)) => ("none", "none", Some(reason)), + (Some(recovery), None) => ( recovery.mode_name(), recovery.step_name(), Some(recovery.unavailable_reason()), ), - None => ("none", "none", Some("auth_manager_missing")), + (None, None) => ("none", "none", Some("auth_manager_missing")), }; session_telemetry.record_auth_recovery( mode, @@ -2089,6 +2101,21 @@ async fn handle_unauthorized( Err(map_api_error(ApiError::Transport(transport))) } +fn access_token_revocation_recovery_reason( + auth_error_code: Option<&str>, + auth_error_type: Option<&str>, +) -> Option<&'static str> { + match (auth_error_code, auth_error_type) { + (Some(TOKEN_INVALIDATED_ERROR_CODE), _) | (_, Some(TOKEN_INVALIDATED_ERROR_CODE)) => { + Some("access_token_invalidated") + } + (Some(TOKEN_REVOKED_ERROR_CODE), _) | (_, Some(TOKEN_REVOKED_ERROR_CODE)) => { + Some("access_token_revoked") + } + _ => None, + } +} + fn api_error_http_status(error: &ApiError) -> Option { match error { ApiError::Transport(TransportError::Http { status, .. }) => Some(status.as_u16()), diff --git a/codex-rs/core/src/client_tests.rs b/codex-rs/core/src/client_tests.rs index b9d9172c83..04f237779e 100644 --- a/codex-rs/core/src/client_tests.rs +++ b/codex-rs/core/src/client_tests.rs @@ -7,11 +7,14 @@ use super::X_CODEX_PARENT_THREAD_ID_HEADER; use super::X_CODEX_TURN_METADATA_HEADER; use super::X_CODEX_WINDOW_ID_HEADER; use super::X_OPENAI_SUBAGENT_HEADER; +use super::handle_unauthorized; use crate::AttestationContext; use crate::AttestationProvider; use crate::GenerateAttestationFuture; +use base64::Engine; use codex_api::ApiError; use codex_api::ResponseEvent; +use codex_api::TransportError; use codex_app_server_protocol::AuthMode; use codex_login::AuthManager; use codex_login::CodexAuth; @@ -23,6 +26,7 @@ use codex_model_provider_info::create_oss_provider_with_base_url; use codex_otel::SessionTelemetry; use codex_protocol::SessionId; use codex_protocol::ThreadId; +use codex_protocol::error::CodexErr; use codex_protocol::models::ContentItem; use codex_protocol::models::ResponseItem; use codex_protocol::openai_models::ModelInfo; @@ -37,6 +41,9 @@ use codex_rollout_trace::RolloutTrace; use codex_rollout_trace::TraceWriter; use codex_rollout_trace::replay_bundle; use futures::StreamExt; +use http::HeaderMap; +use http::HeaderValue; +use http::StatusCode; use pretty_assertions::assert_eq; use serde_json::json; use std::collections::BTreeMap; @@ -581,3 +588,76 @@ async fn non_chatgpt_codex_endpoints_omit_attestation_generation() { ); assert_eq!(attestation_calls.load(Ordering::Relaxed), 0); } + +#[tokio::test] +async fn token_invalidated_401_skips_unauthorized_recovery() { + let manager = + AuthManager::from_auth_for_testing(CodexAuth::create_dummy_chatgpt_auth_for_testing()); + let mut recovery = Some(manager.unauthorized_recovery()); + let x_error_json = base64::engine::general_purpose::STANDARD + .encode(r#"{"error":{"code":"token_invalidated"}}"#); + let mut headers = HeaderMap::new(); + headers.insert( + "x-error-json", + HeaderValue::from_str(&x_error_json).expect("valid x-error-json header"), + ); + + let err = handle_unauthorized( + TransportError::Http { + status: StatusCode::UNAUTHORIZED, + url: Some("https://chatgpt.com/backend-api/codex/responses".to_string()), + headers: Some(headers), + body: Some(r#"{"error":{"message":"revoked"}}"#.to_string()), + }, + &mut recovery, + &test_session_telemetry(), + ) + .await + .expect_err("revoked access tokens should not enter refresh recovery"); + + let CodexErr::UnexpectedStatus(unexpected) = err else { + panic!("expected unauthorized response to bubble up, got {err:?}"); + }; + assert_eq!( + unexpected.identity_error_code.as_deref(), + Some("token_invalidated") + ); + assert_eq!( + recovery + .as_ref() + .expect("recovery state should remain available") + .step_name(), + "reload" + ); +} + +#[tokio::test] +async fn token_invalidated_error_type_401_skips_unauthorized_recovery() { + let manager = + AuthManager::from_auth_for_testing(CodexAuth::create_dummy_chatgpt_auth_for_testing()); + let mut recovery = Some(manager.unauthorized_recovery()); + + let err = handle_unauthorized( + TransportError::Http { + status: StatusCode::UNAUTHORIZED, + url: Some("https://chatgpt.com/backend-api/codex/responses".to_string()), + headers: None, + body: Some(r#"{"error":{"type":"token_invalidated"}}"#.to_string()), + }, + &mut recovery, + &test_session_telemetry(), + ) + .await + .expect_err("invalidated access tokens should not enter refresh recovery"); + + let CodexErr::UnexpectedStatus(_) = err else { + panic!("expected unauthorized response to bubble up, got {err:?}"); + }; + assert_eq!( + recovery + .as_ref() + .expect("recovery state should remain available") + .step_name(), + "reload" + ); +} diff --git a/codex-rs/login/src/auth/manager.rs b/codex-rs/login/src/auth/manager.rs index f4c9285a6f..06dbeb2203 100644 --- a/codex-rs/login/src/auth/manager.rs +++ b/codex-rs/login/src/auth/manager.rs @@ -1064,7 +1064,8 @@ enum UnauthorizedRecoveryMode { } // UnauthorizedRecovery is a state machine that handles an attempt to refresh the authentication when requests -// to API fail with 401 status code. +// to API fail with a refreshable 401 status code. Callers must leave explicit access-token revocation signals +// outside this flow. // The client calls next() every time it encounters a 401 error, one time per retry. // For API key based authentication, we don't do anything and let the error bubble to the user. // diff --git a/codex-rs/response-debug-context/src/lib.rs b/codex-rs/response-debug-context/src/lib.rs index d9c186e02e..c47b08310e 100644 --- a/codex-rs/response-debug-context/src/lib.rs +++ b/codex-rs/response-debug-context/src/lib.rs @@ -14,15 +14,13 @@ pub struct ResponseDebugContext { pub cf_ray: Option, pub auth_error: Option, pub auth_error_code: Option, + pub auth_error_type: Option, } pub fn extract_response_debug_context(transport: &TransportError) -> ResponseDebugContext { let mut context = ResponseDebugContext::default(); - let TransportError::Http { - headers, body: _, .. - } = transport - else { + let TransportError::Http { headers, body, .. } = transport else { return context; }; @@ -38,21 +36,46 @@ pub fn extract_response_debug_context(transport: &TransportError) -> ResponseDeb extract_header(REQUEST_ID_HEADER).or_else(|| extract_header(OAI_REQUEST_ID_HEADER)); context.cf_ray = extract_header(CF_RAY_HEADER); context.auth_error = extract_header(AUTH_ERROR_HEADER); - context.auth_error_code = extract_header(X_ERROR_JSON_HEADER).and_then(|encoded| { - let decoded = base64::engine::general_purpose::STANDARD - .decode(encoded) - .ok()?; - let parsed = serde_json::from_slice::(&decoded).ok()?; - parsed - .get("error") - .and_then(|error| error.get("code")) - .and_then(serde_json::Value::as_str) - .map(str::to_string) - }); + let header_error = extract_header(X_ERROR_JSON_HEADER) + .and_then(|encoded| parse_x_error_json(encoded.as_str())); + let body_error = body + .as_deref() + .and_then(|body| serde_json::from_str::(body).ok()); + context.auth_error_code = header_error + .as_ref() + .and_then(|error| extract_error_field(error, "code")) + .or_else(|| { + body_error + .as_ref() + .and_then(|error| extract_error_field(error, "code")) + }); + context.auth_error_type = header_error + .as_ref() + .and_then(|error| extract_error_field(error, "type")) + .or_else(|| { + body_error + .as_ref() + .and_then(|error| extract_error_field(error, "type")) + }); context } +fn parse_x_error_json(encoded: &str) -> Option { + let decoded = base64::engine::general_purpose::STANDARD + .decode(encoded) + .ok()?; + serde_json::from_slice::(&decoded).ok() +} + +fn extract_error_field(error: &serde_json::Value, field: &str) -> Option { + error + .get("error") + .and_then(|error| error.get(field)) + .and_then(serde_json::Value::as_str) + .map(str::to_string) +} + pub fn extract_response_debug_context_from_api_error(error: &ApiError) -> ResponseDebugContext { match error { ApiError::Transport(transport) => extract_response_debug_context(transport), @@ -127,6 +150,43 @@ mod tests { cf_ray: Some("ray-auth".to_string()), auth_error: Some("missing_authorization_header".to_string()), auth_error_code: Some("token_expired".to_string()), + auth_error_type: None, + } + ); + } + + #[test] + fn extract_response_debug_context_reads_identity_error_code_from_body() { + let context = extract_response_debug_context(&TransportError::Http { + status: StatusCode::UNAUTHORIZED, + url: Some("https://chatgpt.com/backend-api/codex/models".to_string()), + headers: None, + body: Some(r#"{"error":{"code":"token_revoked"}}"#.to_string()), + }); + + assert_eq!( + context, + ResponseDebugContext { + auth_error_code: Some("token_revoked".to_string()), + ..ResponseDebugContext::default() + } + ); + } + + #[test] + fn extract_response_debug_context_reads_identity_error_type_from_body() { + let context = extract_response_debug_context(&TransportError::Http { + status: StatusCode::UNAUTHORIZED, + url: Some("https://chatgpt.com/backend-api/codex/models".to_string()), + headers: None, + body: Some(r#"{"error":{"type":"token_invalidated"}}"#.to_string()), + }); + + assert_eq!( + context, + ResponseDebugContext { + auth_error_type: Some("token_invalidated".to_string()), + ..ResponseDebugContext::default() } ); }