From 8f73cdee456b6c46b791780d40cadf9d556d22a4 Mon Sep 17 00:00:00 2001 From: alexsong-oai Date: Thu, 17 Sep 2026 20:06:32 +0000 Subject: [PATCH] Centralize OAuth login and refresh handling with safer diagnostics (#46300) ## Why Login and token refresh have separate OAuth request and error handling. Token endpoint errors can echo credentials, and JSON decoding errors can expose token values in diagnostics. ## What changed - Extract authorization URL construction, callback validation, PKCE, token grants, and error handling into a shared `oauth` module in `codex-rs/login`. - Route authorization-code exchange and ChatGPT refresh through `OAuthClient`, retaining form and JSON encoding respectively and caller-owned HTTP and credential recovery policies. - Redact echoed request secrets from rejection details and request IDs, redact sensitive transport URL fields, and replace token decoding errors with a generic error. Preserve original error codes for refresh failure classification. - Keep callback state validation ahead of codes and provider errors, with the existing onboarding suffix handled by the login server. ## Testing Add coverage for PKCE binding, request encoding and headers, callback state rejection, credential redaction, oversized or unreadable error bodies, and preservation of stored and cached credentials after transient refresh failures. GitOrigin-RevId: d0a9583b99e24f5aafb751acd1e7200e2261a0e4 --- .../login/src/auth/account_user_id_tests.rs | 1 + codex-rs/login/src/auth/manager.rs | 130 ++--- codex-rs/login/src/callback_params.rs | 20 +- codex-rs/login/src/callback_params_tests.rs | 49 -- codex-rs/login/src/lib.rs | 1 + codex-rs/login/src/oauth/authorization.rs | 105 ++++ .../login/src/oauth/authorization_tests.rs | 32 ++ codex-rs/login/src/oauth/client.rs | 131 +++++ codex-rs/login/src/oauth/client_tests.rs | 247 ++++++++ codex-rs/login/src/oauth/diagnostics.rs | 106 ++++ codex-rs/login/src/oauth/diagnostics_tests.rs | 21 + codex-rs/login/src/oauth/error.rs | 196 +++++++ codex-rs/login/src/oauth/error_tests.rs | 71 +++ codex-rs/login/src/oauth/mod.rs | 28 + codex-rs/login/src/oauth/pkce.rs | 29 + codex-rs/login/src/pkce.rs | 29 +- codex-rs/login/src/server.rs | 537 ++++-------------- codex-rs/login/tests/suite/auth_refresh.rs | 95 ++-- .../login/tests/suite/login_server_e2e.rs | 25 + 19 files changed, 1219 insertions(+), 634 deletions(-) delete mode 100644 codex-rs/login/src/callback_params_tests.rs create mode 100644 codex-rs/login/src/oauth/authorization.rs create mode 100644 codex-rs/login/src/oauth/authorization_tests.rs create mode 100644 codex-rs/login/src/oauth/client.rs create mode 100644 codex-rs/login/src/oauth/client_tests.rs create mode 100644 codex-rs/login/src/oauth/diagnostics.rs create mode 100644 codex-rs/login/src/oauth/diagnostics_tests.rs create mode 100644 codex-rs/login/src/oauth/error.rs create mode 100644 codex-rs/login/src/oauth/error_tests.rs create mode 100644 codex-rs/login/src/oauth/mod.rs create mode 100644 codex-rs/login/src/oauth/pkce.rs diff --git a/codex-rs/login/src/auth/account_user_id_tests.rs b/codex-rs/login/src/auth/account_user_id_tests.rs index ac9db21352..ea43a2be35 100644 --- a/codex-rs/login/src/auth/account_user_id_tests.rs +++ b/codex-rs/login/src/auth/account_user_id_tests.rs @@ -3,6 +3,7 @@ use super::*; use base64::Engine; use pretty_assertions::assert_eq; +use serde_json::Value; use serde_json::json; fn access_token(auth_claims: Value) -> String { diff --git a/codex-rs/login/src/auth/manager.rs b/codex-rs/login/src/auth/manager.rs index db0f2e2057..084851a489 100644 --- a/codex-rs/login/src/auth/manager.rs +++ b/codex-rs/login/src/auth/manager.rs @@ -3,7 +3,6 @@ mod workspace_routing; use chrono::Utc; use http::StatusCode; use serde::Deserialize; -use serde::Serialize; #[cfg(test)] use serial_test::serial; use std::env; @@ -58,9 +57,15 @@ pub use crate::auth::storage::AuthDotJson; pub use crate::auth::storage::AuthKeyringBackendKind; use crate::auth::storage::AuthStorageBackend; use crate::auth::storage::create_auth_storage; -use crate::auth::util::try_parse_error_message; use crate::default_client::create_client; use crate::default_client::create_default_auth_client; +use crate::oauth::ErrorBodyLimit; +use crate::oauth::OAuthClient; +use crate::oauth::OAuthError; +use crate::oauth::RefreshTokenGrant; +use crate::oauth::TokenEncoding; +use crate::oauth::TokenEndpoint; +use crate::oauth::TokenErrorDetail; use crate::outbound_proxy::AuthRouteConfig; use crate::token_data::TokenData; use crate::token_data::parse_chatgpt_account_user_id; @@ -76,7 +81,6 @@ use codex_protocol::auth::PlanType as InternalPlanType; use codex_protocol::auth::RefreshTokenFailedError; use codex_protocol::auth::RefreshTokenFailedReason; use codex_protocol::protocol::SessionSource; -use serde_json::Value; use thiserror::Error; pub use workspace_routing::WorkspaceRouting; pub use workspace_routing::WorkspaceRoutingRequest; @@ -1608,58 +1612,54 @@ async fn request_chatgpt_token_refresh( refresh_token: String, client: &HttpClient, ) -> Result { - let refresh_request = RefreshRequest { - client_id: oauth_client_id(), - grant_type: "refresh_token", - refresh_token, - }; + let client_id = oauth_client_id(); let endpoint = refresh_token_endpoint(); - - // Use shared client factory to include standard headers - let response = client - .post(endpoint.as_str()) - .header("Content-Type", "application/json") - .json(&refresh_request) - .send() + let oauth = OAuthClient::new( + client, + TokenEndpoint { + url: &endpoint, + client_id: &client_id, + encoding: TokenEncoding::Json, + timeout: None, + error_body_limit: ErrorBodyLimit::Unlimited, + }, + ); + match oauth + .refresh(RefreshTokenGrant { + refresh_token: &refresh_token, + resource: None, + }) .await - .map_err(|err| RefreshTokenError::Transient(std::io::Error::other(err)))?; - - let status = response.status(); - if status.is_success() { - let refresh_response = response - .json::() - .await - .map_err(|err| RefreshTokenError::Transient(std::io::Error::other(err)))?; - Ok(refresh_response) - } else { - let body = response.text().await.unwrap_or_default(); - tracing::error!("Failed to refresh token: {status}: {body}"); - let code = extract_refresh_token_error_code(&body); - // RFC 6749 reports an unusable refresh token as invalid_grant without preserving - // the legacy expired/reused/revoked subtype. Keep it terminal with the generic reason. - let is_invalid_grant_bad_request = status == StatusCode::BAD_REQUEST - && code - .as_deref() - .is_some_and(|code| code.eq_ignore_ascii_case("invalid_grant")); - let failed = - classify_refresh_token_failure(code.as_deref(), &body, is_invalid_grant_bad_request); - if status == StatusCode::UNAUTHORIZED - || failed.reason != RefreshTokenFailedReason::Other - || is_invalid_grant_bad_request - { - Err(RefreshTokenError::Permanent(failed)) - } else { - let message = try_parse_error_message(&body); - Err(RefreshTokenError::Transient(std::io::Error::other( - format!("Failed to refresh token: {status}: {message}"), - ))) + { + Ok(response) => Ok(response), + Err(OAuthError::Rejected(rejection)) => { + let status = rejection.status; + let detail = &rejection.detail; + tracing::error!(%status, ?detail, "Failed to refresh token"); + let code = detail.error_code(); + let is_invalid_grant_bad_request = status == StatusCode::BAD_REQUEST + && code.is_some_and(|code| code.eq_ignore_ascii_case("invalid_grant")); + let failed = classify_refresh_token_failure(code, detail, is_invalid_grant_bad_request); + if status == StatusCode::UNAUTHORIZED + || failed.reason != RefreshTokenFailedReason::Other + || is_invalid_grant_bad_request + { + Err(RefreshTokenError::Permanent(failed)) + } else { + Err(RefreshTokenError::Transient(std::io::Error::other( + format!("Failed to refresh token: {status}: {detail}"), + ))) + } + } + Err(error @ (OAuthError::Transport(_) | OAuthError::InvalidResponse)) => { + Err(RefreshTokenError::Transient(std::io::Error::other(error))) } } } fn classify_refresh_token_failure( code: Option<&str>, - body: &str, + detail: &TokenErrorDetail, is_invalid_grant_bad_request: bool, ) -> RefreshTokenFailedError { let normalized_code = code.map(str::to_ascii_lowercase); @@ -1672,8 +1672,7 @@ fn classify_refresh_token_failure( if reason == RefreshTokenFailedReason::Other && !is_invalid_grant_bad_request { tracing::warn!( - backend_code = normalized_code.as_deref(), - backend_body = body, + backend_detail = ?detail, "Encountered unknown response while refreshing token" ); } @@ -1688,39 +1687,6 @@ fn classify_refresh_token_failure( RefreshTokenFailedError::new(reason, message) } -fn extract_refresh_token_error_code(body: &str) -> Option { - if body.trim().is_empty() { - return None; - } - - let Value::Object(map) = serde_json::from_str::(body).ok()? else { - return None; - }; - - if let Some(error_value) = map.get("error") { - match error_value { - Value::Object(obj) => { - if let Some(code) = obj.get("code").and_then(Value::as_str) { - return Some(code.to_string()); - } - } - Value::String(code) => { - return Some(code.to_string()); - } - _ => {} - } - } - - map.get("code").and_then(Value::as_str).map(str::to_string) -} - -#[derive(Serialize)] -struct RefreshRequest { - client_id: String, - grant_type: &'static str, - refresh_token: String, -} - #[derive(Deserialize, Clone)] struct RefreshResponse { id_token: Option, diff --git a/codex-rs/login/src/callback_params.rs b/codex-rs/login/src/callback_params.rs index 981cf1518e..8e6fe8fd23 100644 --- a/codex-rs/login/src/callback_params.rs +++ b/codex-rs/login/src/callback_params.rs @@ -1,4 +1,4 @@ -const LIFE_SCIENCES_OAUTH_STATE_SUFFIX: &str = ".onboarding_entrypoint=life_sciences"; +pub(crate) const LIFE_SCIENCES_OAUTH_STATE_SUFFIX: &str = ".onboarding_entrypoint=life_sciences"; #[derive(Clone, Copy, Debug, PartialEq, Eq)] pub enum LoginOnboardingEntrypoint { @@ -9,21 +9,3 @@ pub enum LoginOnboardingEntrypoint { pub struct LoginCallbackResult { pub onboarding_entrypoint: Option, } - -pub(crate) fn login_callback_result_from_state( - callback_state: &str, - expected_state: &str, -) -> Option { - if callback_state == expected_state { - return Some(LoginCallbackResult::default()); - } - - (callback_state.strip_suffix(LIFE_SCIENCES_OAUTH_STATE_SUFFIX) == Some(expected_state)) - .then_some(LoginCallbackResult { - onboarding_entrypoint: Some(LoginOnboardingEntrypoint::LifeSciences), - }) -} - -#[cfg(test)] -#[path = "callback_params_tests.rs"] -mod tests; diff --git a/codex-rs/login/src/callback_params_tests.rs b/codex-rs/login/src/callback_params_tests.rs deleted file mode 100644 index 1f06614b20..0000000000 --- a/codex-rs/login/src/callback_params_tests.rs +++ /dev/null @@ -1,49 +0,0 @@ -use super::*; -use pretty_assertions::assert_eq; - -#[test] -fn accepts_the_original_oauth_state() { - assert_eq!( - login_callback_result_from_state("expected-state", "expected-state"), - Some(LoginCallbackResult::default()) - ); -} - -#[test] -fn accepts_the_allowlisted_life_sciences_suffix() { - assert_eq!( - login_callback_result_from_state( - "expected-state.onboarding_entrypoint=life_sciences", - "expected-state", - ), - Some(LoginCallbackResult { - onboarding_entrypoint: Some(LoginOnboardingEntrypoint::LifeSciences), - }) - ); -} - -#[test] -fn rejects_a_suffix_when_the_nonce_does_not_match() { - assert_eq!( - login_callback_result_from_state( - "different-state.onboarding_entrypoint=life_sciences", - "expected-state", - ), - None - ); -} - -#[test] -fn rejects_unrecognized_or_repeated_suffixes() { - for callback_state in [ - "expected-state.onboarding_entrypoint=unknown", - "expected-state.onboarding_entrypoint=life_sciences.onboarding_entrypoint=life_sciences", - "expected-state.extra=value.onboarding_entrypoint=life_sciences", - ] { - assert_eq!( - login_callback_result_from_state(callback_state, "expected-state"), - None, - "unexpectedly accepted {callback_state}", - ); - } -} diff --git a/codex-rs/login/src/lib.rs b/codex-rs/login/src/lib.rs index 6661a78d8e..8ad2768649 100644 --- a/codex-rs/login/src/lib.rs +++ b/codex-rs/login/src/lib.rs @@ -9,6 +9,7 @@ pub use auth::WorkspaceRoutingSession; mod callback_params; mod device_code_auth; +mod oauth; mod outbound_proxy; mod pkce; mod server; diff --git a/codex-rs/login/src/oauth/authorization.rs b/codex-rs/login/src/oauth/authorization.rs new file mode 100644 index 0000000000..e1c154b258 --- /dev/null +++ b/codex-rs/login/src/oauth/authorization.rs @@ -0,0 +1,105 @@ +//! Builds authorization requests and validates callback state before consuming codes or errors. + +use base64::Engine; +use rand::RngCore; +use url::Url; + +use crate::oauth::PkceCodes; + +/// Standard authorization parameters plus issuer-specific extensions supplied by the caller. +pub(crate) struct AuthorizationRequest<'a> { + pub endpoint: &'a str, + pub client_id: &'a str, + pub redirect_uri: &'a str, + pub scope: Option<&'a str>, + pub resource: Option<&'a str>, + pub pkce: &'a PkceCodes, + pub state: &'a str, + pub extra_parameters: &'a [(&'a str, &'a str)], +} + +pub(crate) fn build_authorization_url( + request: AuthorizationRequest<'_>, +) -> Result { + let mut url = Url::parse(request.endpoint)?; + { + let mut query = url.query_pairs_mut(); + query.append_pair("response_type", "code"); + query.append_pair("client_id", request.client_id); + query.append_pair("redirect_uri", request.redirect_uri); + query.append_pair("code_challenge", &request.pkce.code_challenge); + query.append_pair("code_challenge_method", "S256"); + query.append_pair("state", request.state); + if let Some(scope) = request.scope { + query.append_pair("scope", scope); + } + if let Some(resource) = request.resource { + query.append_pair("resource", resource); + } + query.extend_pairs(request.extra_parameters.iter().copied()); + } + Ok(url) +} + +pub(crate) fn generate_state() -> String { + let mut bytes = [0_u8; 32]; + rand::rng().fill_bytes(&mut bytes); + base64::engine::general_purpose::URL_SAFE_NO_PAD.encode(bytes) +} + +/// Callback parameters are kept out of Debug output because code and state are credentials. +#[derive(Default)] +pub(crate) struct CallbackParameters { + pub code: Option, + pub state: Option, + pub error: Option, + pub error_description: Option, +} + +pub(crate) enum CallbackError<'a> { + StateMismatch, + Provider { + code: &'a str, + description: Option<&'a str>, + }, + MissingCode, +} + +impl CallbackParameters { + pub(crate) fn from_url(url: &Url) -> Self { + let mut params = Self::default(); + for (name, value) in url.query_pairs() { + match name.as_ref() { + "code" => params.code = Some(value.into_owned()), + "state" => params.state = Some(value.into_owned()), + "error" => params.error = Some(value.into_owned()), + "error_description" => params.error_description = Some(value.into_owned()), + _ => {} + } + } + params + } + + /// Checks state before accepting an authorization code or provider error. + pub(crate) fn validate(&self, expected_state: &str) -> Result<&str, CallbackError<'_>> { + if self.state.as_deref() != Some(expected_state) { + return Err(CallbackError::StateMismatch); + } + if let Some(code) = self.error.as_deref() { + return Err(CallbackError::Provider { + code, + description: self.error_description.as_deref(), + }); + } + let code = self + .code + .as_deref() + .filter(|code| !code.is_empty()) + .ok_or(CallbackError::MissingCode)?; + Ok(code) + } +} + +#[cfg(test)] +#[path = "authorization_tests.rs"] +mod tests; diff --git a/codex-rs/login/src/oauth/authorization_tests.rs b/codex-rs/login/src/oauth/authorization_tests.rs new file mode 100644 index 0000000000..537572e75c --- /dev/null +++ b/codex-rs/login/src/oauth/authorization_tests.rs @@ -0,0 +1,32 @@ +//! Callback state must match before codes or provider errors can be consumed. + +use pretty_assertions::assert_eq; + +use super::*; + +#[test] +fn callback_validation_checks_state_before_code_or_provider_errors() { + for (query, expected) in [ + ("state=expected&code=trusted-code", Ok("trusted-code")), + ("code=untrusted", Err("state")), + ( + "state=wrong&code=untrusted&error=access_denied", + Err("state"), + ), + ("state=expected.extra&code=untrusted", Err("state")), + ( + "state=expected&code=ignored&error=access_denied", + Err("provider"), + ), + ("state=expected&code=", Err("code")), + ] { + let url = Url::parse(&format!("http://localhost/callback?{query}")).unwrap(); + let params = CallbackParameters::from_url(&url); + let actual = params.validate("expected").map_err(|error| match error { + CallbackError::StateMismatch => "state", + CallbackError::Provider { .. } => "provider", + CallbackError::MissingCode => "code", + }); + assert_eq!(actual, expected, "callback query {query}"); + } +} diff --git a/codex-rs/login/src/oauth/client.rs b/codex-rs/login/src/oauth/client.rs new file mode 100644 index 0000000000..49ec82f117 --- /dev/null +++ b/codex-rs/login/src/oauth/client.rs @@ -0,0 +1,131 @@ +//! Executes OAuth grants without owning credential state or choosing a recovery policy. + +use std::collections::BTreeMap; +use std::time::Duration; + +use codex_http_client::HttpClient; +use serde::de::DeserializeOwned; + +use crate::oauth::ErrorBodyLimit; +use crate::oauth::OAuthError; +use crate::oauth::PkceCodes; +use crate::oauth::TokenRejection; +use crate::oauth::diagnostics::redact_error_url; + +/// ChatGPT refresh uses JSON; authorization-code and gateway grants use form encoding. +#[derive(Clone, Copy, Debug)] +pub(crate) enum TokenEncoding { + Form, + Json, +} + +/// Per-endpoint transport settings. The HTTP client already owns routing and CA policy. +pub(crate) struct TokenEndpoint<'a> { + pub url: &'a str, + pub client_id: &'a str, + pub encoding: TokenEncoding, + pub timeout: Option, + pub error_body_limit: ErrorBodyLimit, +} + +/// Parameters bound to an authorization attempt. Secrets deliberately have no Debug output. +pub(crate) struct AuthorizationCodeGrant<'a> { + pub code: &'a str, + pub redirect_uri: &'a str, + pub pkce: &'a PkceCodes, + pub resource: Option<&'a str>, +} + +pub(crate) struct RefreshTokenGrant<'a> { + pub refresh_token: &'a str, + pub resource: Option<&'a str>, +} + +/// Shared OAuth protocol implementation; callers retain tokens, storage, and recovery decisions. +pub(crate) struct OAuthClient<'a> { + http_client: &'a HttpClient, + endpoint: TokenEndpoint<'a>, +} + +impl<'a> OAuthClient<'a> { + pub(crate) fn new(http_client: &'a HttpClient, endpoint: TokenEndpoint<'a>) -> Self { + Self { + http_client, + endpoint, + } + } + + pub(crate) async fn exchange_code( + &self, + grant: AuthorizationCodeGrant<'_>, + ) -> Result { + let mut parameters = vec![ + ("grant_type", "authorization_code"), + ("client_id", self.endpoint.client_id), + ("code", grant.code), + ("redirect_uri", grant.redirect_uri), + ("code_verifier", grant.pkce.code_verifier.as_str()), + ]; + if let Some(resource) = grant.resource { + parameters.push(("resource", resource)); + } + self.exchange(¶meters, &[grant.code, &grant.pkce.code_verifier]) + .await + } + + pub(crate) async fn refresh( + &self, + grant: RefreshTokenGrant<'_>, + ) -> Result { + let mut parameters = vec![ + ("grant_type", "refresh_token"), + ("client_id", self.endpoint.client_id), + ("refresh_token", grant.refresh_token), + ]; + if let Some(resource) = grant.resource { + parameters.push(("resource", resource)); + } + self.exchange(¶meters, &[grant.refresh_token]).await + } + + async fn exchange( + &self, + parameters: &[(&str, &str)], + secrets: &[&str], + ) -> Result { + let mut request = self.http_client.post(self.endpoint.url); + if let Some(timeout) = self.endpoint.timeout { + request = request.timeout(timeout); + } + request = match self.endpoint.encoding { + TokenEncoding::Form => { + let mut form = url::form_urlencoded::Serializer::new(String::new()); + form.extend_pairs(parameters.iter().copied()); + request + .header("Content-Type", "application/x-www-form-urlencoded") + .body(form.finish()) + } + TokenEncoding::Json => request + .header("Content-Type", "application/json") + .json(¶meters.iter().copied().collect::>()), + }; + let response = request + .send() + .await + .map_err(|error| OAuthError::Transport(redact_error_url(error)))?; + if !response.status().is_success() { + return Err(OAuthError::Rejected(Box::new( + TokenRejection::from_response(response, self.endpoint.error_body_limit, secrets) + .await, + ))); + } + response + .json() + .await + .map_err(|_| OAuthError::InvalidResponse) + } +} + +#[cfg(test)] +#[path = "client_tests.rs"] +mod tests; diff --git a/codex-rs/login/src/oauth/client_tests.rs b/codex-rs/login/src/oauth/client_tests.rs new file mode 100644 index 0000000000..d81e2d2698 --- /dev/null +++ b/codex-rs/login/src/oauth/client_tests.rs @@ -0,0 +1,247 @@ +//! Exercises OAuth operations over HTTP, including grant binding and rejection diagnostics. + +use std::collections::BTreeMap; + +use base64::Engine; +use codex_http_client::HttpClient; +use codex_http_client::HttpClientBuilder; +use http::HeaderMap; +use http::HeaderValue; +use http::StatusCode; +use pretty_assertions::assert_eq; +use serde_json::Value; +use serde_json::json; +use sha2::Digest; +use sha2::Sha256; +use tokio::io::AsyncReadExt; +use tokio::io::AsyncWriteExt; +use tokio::net::TcpListener; +use wiremock::Mock; +use wiremock::MockServer; +use wiremock::ResponseTemplate; +use wiremock::matchers::method; + +use super::*; +use crate::oauth::AuthorizationRequest; +use crate::oauth::build_authorization_url; +use crate::oauth::generate_pkce; +use crate::oauth::generate_state; + +fn oauth<'a>(http: &'a HttpClient, url: &'a str, limit: ErrorBodyLimit) -> OAuthClient<'a> { + OAuthClient::new( + http, + TokenEndpoint { + url, + client_id: "client id", + encoding: TokenEncoding::Form, + timeout: None, + error_body_limit: limit, + }, + ) +} + +#[tokio::test] +async fn authorization_code_preserves_pkce_and_http_policy() { + let server = MockServer::start().await; + Mock::given(method("POST")) + .respond_with( + ResponseTemplate::new(/*s*/ 200).set_body_json(json!({"access_token": "access"})), + ) + .expect(/*r*/ 1) + .mount(&server) + .await; + let mut headers = HeaderMap::new(); + headers.insert("x-client-policy", HeaderValue::from_static("preserved")); + let http = HttpClientBuilder::new() + .default_headers(headers) + .build_direct() + .unwrap(); + let endpoint = server.uri(); + let form = oauth(&http, &endpoint, ErrorBodyLimit::Unlimited); + let pkce = generate_pkce(); + let state = generate_state(); + let resource = "https://gateway.example.test/a?b=c&d=e"; + let redirect_uri = "http://127.0.0.1:1234/callback"; + let authorization_url = build_authorization_url(AuthorizationRequest { + endpoint: &format!("{endpoint}/authorize?tenant=existing"), + client_id: "client id", + redirect_uri, + scope: Some("openid gateway.inference"), + resource: Some(resource), + pkce: &pkce, + state: &state, + extra_parameters: &[("prompt", "login")], + }) + .unwrap(); + let tokens: Value = form + .exchange_code(AuthorizationCodeGrant { + code: "code+with&reserved=characters", + redirect_uri, + pkce: &pkce, + resource: Some(resource), + }) + .await + .unwrap(); + assert_eq!(tokens, json!({"access_token": "access"})); + let requests = server.received_requests().await.unwrap(); + let code_parameters = decode_form(&requests[0].body); + assert_eq!( + requests[0].headers["content-type"], + "application/x-www-form-urlencoded" + ); + assert_eq!(requests[0].headers["x-client-policy"], "preserved"); + assert_eq!( + code_parameters, + json!({ + "grant_type": "authorization_code", + "client_id": "client id", + "code": "code+with&reserved=characters", + "redirect_uri": redirect_uri, + "code_verifier": pkce.code_verifier, + "resource": resource, + }) + ); + // Bind the browser challenge to the verifier actually sent in the code exchange. + let challenge = base64::engine::general_purpose::URL_SAFE_NO_PAD.encode(Sha256::digest( + code_parameters["code_verifier"] + .as_str() + .unwrap() + .as_bytes(), + )); + assert_eq!( + decode_form(authorization_url.query().unwrap().as_bytes()), + json!({ + "response_type": "code", + "client_id": "client id", + "redirect_uri": redirect_uri, + "scope": "openid gateway.inference", + "resource": resource, + "state": state, + "code_challenge": challenge, + "code_challenge_method": "S256", + "tenant": "existing", + "prompt": "login", + }) + ); +} + +#[tokio::test] +async fn rejection_preserves_status_and_omits_oversized_diagnostics() { + let http = HttpClientBuilder::new().build_direct().unwrap(); + const BODY_LIMIT: usize = 8192; + let refresh_token = "credential+crossing&the=limit /%"; + let form_encoded: String = + url::form_urlencoded::byte_serialize(refresh_token.as_bytes()).collect(); + let long_body = format!("{}{form_encoded}", "x".repeat(BODY_LIMIT - 2)); + let redacted = long_body.replace(&form_encoded, "[REDACTED]"); + let top_level_error = json!({ + "code": "configuration_error", + "message": format!("Unknown tenant: {form_encoded}"), + }) + .to_string(); + let redacted_top_level_error = top_level_error.replace(&form_encoded, "[REDACTED]"); + for (limit, body, expected_display) in [ + ( + ErrorBodyLimit::Bytes(BODY_LIMIT), + json!({"error": "invalid_grant", "error_description": form_encoded}).to_string(), + "[REDACTED]", + ), + ( + ErrorBodyLimit::Unlimited, + top_level_error, + redacted_top_level_error.as_str(), + ), + ( + ErrorBodyLimit::Bytes(BODY_LIMIT), + long_body.clone(), + "unknown error", + ), + ( + ErrorBodyLimit::Unlimited, + long_body.clone(), + redacted.as_str(), + ), + ] { + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(move |request: &wiremock::Request| { + decode_form(&request.body) + == json!({ + "client_id": "client id", "grant_type": "refresh_token", + "refresh_token": refresh_token, "resource": "urn:gateway", + }) + }) + .respond_with( + ResponseTemplate::new(/*s*/ 401) + .insert_header("x-request-id", format!("request-{form_encoded}")) + .set_body_string(body), + ) + .expect(/*r*/ 1) + .mount(&server) + .await; + let endpoint = server.uri(); + let result = oauth(&http, &endpoint, limit) + .refresh::(RefreshTokenGrant { + refresh_token, + resource: Some("urn:gateway"), + }) + .await; + let Err(OAuthError::Rejected(error)) = result else { + panic!("expected OAuth rejection") + }; + assert_eq!( + ( + error.status, + error.request_id.as_deref(), + error.detail.to_string(), + error.body_read_error.is_none() + ), + ( + StatusCode::UNAUTHORIZED, + Some("request-[REDACTED]"), + expected_display.to_string(), + true + ) + ); + } +} + +#[tokio::test] +async fn rejection_keeps_status_when_reading_the_body_fails() { + let listener = TcpListener::bind("127.0.0.1:0").await.unwrap(); + let address = listener.local_addr().unwrap(); + let server = tokio::spawn(async move { + let (mut stream, _) = listener.accept().await.unwrap(); + let mut request = Vec::new(); + while !request.ends_with(b"refresh_token=refresh") { + let mut buffer = [0; 4096]; + let read = stream.read(&mut buffer).await.unwrap(); + assert!(read > 0); + request.extend_from_slice(&buffer[..read]); + } + stream.write_all(b"HTTP/1.1 401 Unauthorized\r\nContent-Length: 100\r\nConnection: close\r\n\r\npartial").await.unwrap(); + }); + let http = HttpClientBuilder::new().build_direct().unwrap(); + let endpoint = format!("http://{address}"); + let result = oauth(&http, &endpoint, ErrorBodyLimit::Unlimited) + .refresh::(RefreshTokenGrant { + refresh_token: "refresh", + resource: None, + }) + .await; + let Err(OAuthError::Rejected(error)) = result else { + panic!("expected OAuth rejection") + }; + assert_eq!(error.status, StatusCode::UNAUTHORIZED); + assert!(error.body_read_error.is_some()); + server.await.unwrap(); +} + +fn decode_form(body: &[u8]) -> Value { + serde_json::to_value( + url::form_urlencoded::parse(body) + .into_owned() + .collect::>(), + ) + .unwrap() +} diff --git a/codex-rs/login/src/oauth/diagnostics.rs b/codex-rs/login/src/oauth/diagnostics.rs new file mode 100644 index 0000000000..024d7e6d10 --- /dev/null +++ b/codex-rs/login/src/oauth/diagnostics.rs @@ -0,0 +1,106 @@ +//! Redacts credentials in OAuth transport URLs and issuer diagnostics. + +const REDACTED_URL_VALUE: &str = ""; +const SENSITIVE_URL_QUERY_KEYS: &[&str] = &[ + "access_token", + "api_key", + "client_secret", + "code", + "code_verifier", + "id_token", + "key", + "refresh_token", + "requested_token", + "state", + "subject_token", + "token", +]; + +fn redact_sensitive_query_value(key: &str, value: &str) -> String { + if SENSITIVE_URL_QUERY_KEYS + .iter() + .any(|candidate| candidate.eq_ignore_ascii_case(key)) + { + REDACTED_URL_VALUE.to_string() + } else { + value.to_string() + } +} + +/// Redacts URL components that commonly carry auth secrets while preserving the host/path shape. +/// +/// This keeps developer-facing logs useful for debugging transport failures without persisting +/// tokens, callback codes, fragments, or embedded credentials. +fn redact_sensitive_url_parts(url: &mut url::Url) { + let _ = url.set_username(""); + let _ = url.set_password(None); + url.set_fragment(None); + + let query_pairs = url + .query_pairs() + .map(|(key, value)| { + let key = key.into_owned(); + let value = value.into_owned(); + (key.clone(), redact_sensitive_query_value(&key, &value)) + }) + .collect::>(); + + if query_pairs.is_empty() { + url.set_query(None); + return; + } + + let redacted_query = query_pairs + .into_iter() + .fold( + url::form_urlencoded::Serializer::new(String::new()), + |mut serializer, (key, value)| { + serializer.append_pair(&key, &value); + serializer + }, + ) + .finish(); + url.set_query(Some(&redacted_query)); +} + +/// Redacts any URL attached to an HTTP transport error before it is logged or returned. +pub(crate) fn redact_error_url( + mut err: codex_http_client::HttpError, +) -> codex_http_client::HttpError { + if let Some(url) = err.url_mut() { + redact_sensitive_url_parts(url); + } + err +} + +/// Sanitizes a free-form URL string for structured logging. +/// +/// This is used for caller-supplied issuer values, which may contain credentials or query +/// parameters on non-default deployments. +pub(crate) fn sanitize_url_for_logging(url: &str) -> String { + match url::Url::parse(url) { + Ok(mut url) => { + redact_sensitive_url_parts(&mut url); + url.to_string() + } + Err(_) => "".to_string(), + } +} + +pub(crate) fn redact_request_secrets(text: &str, secrets: &[&str]) -> String { + let mut redacted = text.to_string(); + for secret in secrets.iter().copied().filter(|secret| !secret.is_empty()) { + let form_encoded: String = + url::form_urlencoded::byte_serialize(secret.as_bytes()).collect(); + redacted = redacted.replace(&form_encoded, "[REDACTED]"); + if let Ok(encoded) = serde_json::to_string(secret) { + redacted = redacted.replace(&encoded[1..encoded.len() - 1], "[REDACTED]"); + } + redacted = redacted.replace(secret, "[REDACTED]"); + } + redacted +} + +#[cfg(test)] +#[path = "diagnostics_tests.rs"] +mod tests; diff --git a/codex-rs/login/src/oauth/diagnostics_tests.rs b/codex-rs/login/src/oauth/diagnostics_tests.rs new file mode 100644 index 0000000000..88573c189b --- /dev/null +++ b/codex-rs/login/src/oauth/diagnostics_tests.rs @@ -0,0 +1,21 @@ +//! Exercise URL redaction through the public logging boundary. + +use super::sanitize_url_for_logging; +use pretty_assertions::assert_eq; + +#[test] +fn sanitize_url_for_logging_preserves_only_safe_url_parts() { + for (url, expected) in [ + ( + "https://user:pass@example.com/oauth/token?code=abc&redirect_uri=http%3A%2F%2Flocalhost%2Fcallback#secret", + "https://example.com/oauth/token?code=%3Credacted%3E&redirect_uri=http%3A%2F%2Flocalhost%2Fcallback", + ), + ( + "https://example.com/base?TOKEN=abc&env=prod", + "https://example.com/base?TOKEN=%3Credacted%3E&env=prod", + ), + ("not a URL", ""), + ] { + assert_eq!(sanitize_url_for_logging(url), expected); + } +} diff --git a/codex-rs/login/src/oauth/error.rs b/codex-rs/login/src/oauth/error.rs new file mode 100644 index 0000000000..bf435936b0 --- /dev/null +++ b/codex-rs/login/src/oauth/error.rs @@ -0,0 +1,196 @@ +//! Parses OAuth rejections and sanitizes diagnostic text without deciding credential recovery. + +use std::fmt; + +use codex_http_client::HttpError; +use codex_http_client::HttpResponse; +use http::StatusCode; +use serde_json::Value; + +use crate::oauth::diagnostics::redact_error_url; +use crate::oauth::diagnostics::redact_request_secrets; + +#[derive(thiserror::Error)] +pub(crate) enum OAuthError { + #[error("OAuth token request failed: {0}")] + Transport(#[source] HttpError), + // Decoder errors can contain token values, including through their source chain. + #[error("OAuth token response is invalid")] + InvalidResponse, + #[error("{0}")] + Rejected(Box), +} + +impl fmt::Debug for OAuthError { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + match self { + Self::Transport(error) => formatter + .debug_tuple("Transport") + .field(&error.to_string()) + .finish(), + // A JSON decoder's source error can include the offending token value. + Self::InvalidResponse => formatter.write_str("InvalidResponse"), + Self::Rejected(rejection) => { + formatter.debug_tuple("Rejected").field(rejection).finish() + } + } + } +} + +/// Preserve each caller's existing diagnostic-read contract. +#[derive(Clone, Copy)] +pub(crate) enum ErrorBodyLimit { + Unlimited, + #[cfg_attr( + not(test), + expect(dead_code, reason = "Used by GatewayAuthManager in the following PR") + )] + Bytes(usize), +} + +/// Sanitized token rejection. HTTP status survives even when its body cannot be read. +#[derive(Debug)] +pub(crate) struct TokenRejection { + pub status: StatusCode, + pub request_id: Option, + pub detail: TokenErrorDetail, + pub body_read_error: Option, +} + +impl TokenRejection { + pub(crate) async fn from_response( + mut response: HttpResponse, + limit: ErrorBodyLimit, + secrets: &[&str], + ) -> Self { + let status = response.status(); + let request_id = ["x-request-id", "x-openai-request-id", "cf-ray"] + .iter() + .find_map(|name| response.headers().get(*name)) + .and_then(|value| value.to_str().ok()) + .map(|value| { + redact_request_secrets(value, secrets) + .chars() + .take(/*n*/ 128) + .collect() + }); + let body = match limit { + ErrorBodyLimit::Unlimited => response.text().await, + ErrorBodyLimit::Bytes(limit) => { + let mut body = Vec::new(); + loop { + match response.chunk().await { + Ok(Some(chunk)) if chunk.len() <= limit.saturating_sub(body.len()) => { + body.extend_from_slice(&chunk); + } + Ok(None) => break Ok(String::from_utf8_lossy(&body).into_owned()), + // Omit oversized bodies entirely so an echoed credential cannot be cut + // into a prefix that evades the caller's diagnostic redaction. + Ok(Some(_)) => break Ok(String::new()), + Err(error) => break Err(error), + } + } + } + }; + let (detail, body_read_error) = match body { + Ok(body) => (TokenErrorDetail::parse(&body, secrets), None), + Err(error) => ( + TokenErrorDetail::parse("", secrets), + Some(redact_error_url(error)), + ), + }; + Self { + status, + request_id, + detail, + body_read_error, + } + } +} + +impl fmt::Display for TokenRejection { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + write!( + formatter, + "token endpoint returned status {}: {}", + self.status, self.detail + )?; + if let Some(request_id) = &self.request_id { + write!(formatter, " (request id: {request_id})")?; + } + Ok(()) + } +} + +/// Parsed standard OAuth fields, including the nested error envelope used by existing issuers. +pub(crate) struct TokenErrorDetail { + error_code: Option, + diagnostic_code: Option, + error_message: Option, + display_message: String, +} + +impl TokenErrorDetail { + /// The issuer's unmodified code, for recovery classification rather than diagnostics. + pub(crate) fn error_code(&self) -> Option<&str> { + self.error_code.as_deref() + } + + fn parse(body: &str, secrets: &[&str]) -> Self { + let trimmed = body.trim(); + let parsed = serde_json::from_str::(trimmed).ok(); + let display_code = parsed.as_ref().and_then(|json| { + nonempty_text(json.get("error")) + .or_else(|| nonempty_text(json.get("error").and_then(|error| error.get("code")))) + }); + // Top-level codes inform refresh recovery without replacing the legacy full-body display. + let code = display_code.or_else(|| { + parsed + .as_ref() + .and_then(|json| nonempty_text(json.get("code"))) + }); + let message = parsed.as_ref().and_then(|json| { + nonempty_text(json.get("error_description")) + .or_else(|| nonempty_text(json.get("error").and_then(|error| error.get("message")))) + }); + let display = message.or(display_code).unwrap_or(if trimmed.is_empty() { + "unknown error" + } else { + trimmed + }); + // Redact plain, JSON-escaped, and form-encoded values before applying display limits. + Self { + error_code: code.map(str::to_string), + diagnostic_code: code.map(|value| redact_request_secrets(value, secrets)), + error_message: message.map(|value| redact_request_secrets(value, secrets)), + display_message: redact_request_secrets(display, secrets), + } + } +} + +fn nonempty_text(value: Option<&Value>) -> Option<&str> { + value + .and_then(Value::as_str) + .filter(|value| !value.trim().is_empty()) +} + +impl fmt::Debug for TokenErrorDetail { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + // Plain-text fallback bodies remain available to the caller's UI, not structured logs. + formatter + .debug_struct("TokenErrorDetail") + .field("error_code", &self.diagnostic_code) + .field("error_message", &self.error_message) + .finish_non_exhaustive() + } +} + +impl fmt::Display for TokenErrorDetail { + fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result { + formatter.write_str(&self.display_message) + } +} + +#[cfg(test)] +#[path = "error_tests.rs"] +mod tests; diff --git a/codex-rs/login/src/oauth/error_tests.rs b/codex-rs/login/src/oauth/error_tests.rs new file mode 100644 index 0000000000..67207ecba4 --- /dev/null +++ b/codex-rs/login/src/oauth/error_tests.rs @@ -0,0 +1,71 @@ +//! Token error parsing accepts standard OAuth fields and existing issuer error envelopes. + +use pretty_assertions::assert_eq; + +use super::TokenErrorDetail; + +#[test] +fn parses_standard_legacy_and_plain_text_rejections() { + for (body, code, display) in [ + ( + r#"{"error":"invalid_grant","error_description":"refresh token expired"}"#, + Some("invalid_grant"), + "refresh token expired", + ), + ( + r#"{"error":{"code":"proxy_auth_required","message":"proxy authentication required"}}"#, + Some("proxy_auth_required"), + "proxy authentication required", + ), + ( + r#"{"error":"temporarily_unavailable"}"#, + Some("temporarily_unavailable"), + "temporarily_unavailable", + ), + ( + r#"{"code":"refresh_token_expired"}"#, + Some("refresh_token_expired"), + r#"{"code":"refresh_token_expired"}"#, + ), + ( + r#"{"code":"configuration_error","message":"Unknown tenant: acme"}"#, + Some("configuration_error"), + r#"{"code":"configuration_error","message":"Unknown tenant: acme"}"#, + ), + ( + r#"{"code":"configuration_error","message":"top-level","error_description":"description","error":{"message":"nested"}}"#, + Some("configuration_error"), + "description", + ), + ( + r#"{"code":"configuration_error","message":"top-level","error":{"message":"nested"}}"#, + Some("configuration_error"), + "nested", + ), + ("service unavailable", None, "service unavailable"), + (" ", None, "unknown error"), + ] { + let detail = TokenErrorDetail::parse(body, &[]); + assert_eq!( + (detail.error_code(), detail.to_string()), + (code, display.to_string()) + ); + } +} + +#[test] +fn redacts_plain_and_json_escaped_credentials() { + let secret = r#"secret-"refresh\token+&= /%"#; + let description = format!("{}: {secret}", "x".repeat(/*n*/ 500)); + for body in [ + serde_json::json!({"error": secret}).to_string(), + serde_json::json!({"error": "temporarily_unavailable", "error_description": description}) + .to_string(), + serde_json::json!({"unrecognized_field": description}).to_string(), + serde_json::json!({"code": "configuration_error", "message": description}).to_string(), + ] { + let detail = TokenErrorDetail::parse(&body, &[secret]); + assert!(detail.display_message.contains("[REDACTED]")); + assert!(!format!("{detail} {detail:?}").contains("secret-")); + } +} diff --git a/codex-rs/login/src/oauth/mod.rs b/codex-rs/login/src/oauth/mod.rs new file mode 100644 index 0000000000..329e1be1e8 --- /dev/null +++ b/codex-rs/login/src/oauth/mod.rs @@ -0,0 +1,28 @@ +//! OAuth protocol operations shared by independent credential managers. +//! +//! Callers supply an HTTP client with the application's proxy and CA policy. They retain +//! ownership of credential storage, refresh scheduling, account validation, and login UX. + +mod authorization; +mod client; +mod diagnostics; +mod error; +mod pkce; + +pub(crate) use authorization::AuthorizationRequest; +pub(crate) use authorization::CallbackError; +pub(crate) use authorization::CallbackParameters; +pub(crate) use authorization::build_authorization_url; +pub(crate) use authorization::generate_state; +pub(crate) use client::AuthorizationCodeGrant; +pub(crate) use client::OAuthClient; +pub(crate) use client::RefreshTokenGrant; +pub(crate) use client::TokenEncoding; +pub(crate) use client::TokenEndpoint; +pub(crate) use diagnostics::sanitize_url_for_logging; +pub(crate) use error::ErrorBodyLimit; +pub(crate) use error::OAuthError; +pub(crate) use error::TokenErrorDetail; +pub(crate) use error::TokenRejection; +pub(crate) use pkce::PkceCodes; +pub(crate) use pkce::generate_pkce; diff --git a/codex-rs/login/src/oauth/pkce.rs b/codex-rs/login/src/oauth/pkce.rs new file mode 100644 index 0000000000..9a759cadfd --- /dev/null +++ b/codex-rs/login/src/oauth/pkce.rs @@ -0,0 +1,29 @@ +//! Generates proof keys for authorization-code grants. Verifiers must never be logged. + +use base64::Engine; +use rand::RngCore; +use sha2::Digest; +use sha2::Sha256; + +#[derive(Clone)] +pub(crate) struct PkceCodes { + pub code_verifier: String, + pub code_challenge: String, +} + +pub(crate) fn generate_pkce() -> PkceCodes { + let mut bytes = [0u8; 64]; + rand::rng().fill_bytes(&mut bytes); + + // Verifier: URL-safe base64 without padding (43..128 chars) + let code_verifier = base64::engine::general_purpose::URL_SAFE_NO_PAD.encode(bytes); + + // Challenge (S256): BASE64URL-ENCODE(SHA256(verifier)) without padding + let digest = Sha256::digest(code_verifier.as_bytes()); + let code_challenge = base64::engine::general_purpose::URL_SAFE_NO_PAD.encode(digest); + + PkceCodes { + code_verifier, + code_challenge, + } +} diff --git a/codex-rs/login/src/pkce.rs b/codex-rs/login/src/pkce.rs index a0eacfc201..0f7a84d620 100644 --- a/codex-rs/login/src/pkce.rs +++ b/codex-rs/login/src/pkce.rs @@ -1,27 +1,2 @@ -use base64::Engine; -use rand::RngCore; -use sha2::Digest; -use sha2::Sha256; - -#[derive(Debug, Clone)] -pub struct PkceCodes { - pub code_verifier: String, - pub code_challenge: String, -} - -pub fn generate_pkce() -> PkceCodes { - let mut bytes = [0u8; 64]; - rand::rng().fill_bytes(&mut bytes); - - // Verifier: URL-safe base64 without padding (43..128 chars) - let code_verifier = base64::engine::general_purpose::URL_SAFE_NO_PAD.encode(bytes); - - // Challenge (S256): BASE64URL-ENCODE(SHA256(verifier)) without padding - let digest = Sha256::digest(code_verifier.as_bytes()); - let code_challenge = base64::engine::general_purpose::URL_SAFE_NO_PAD.encode(digest); - - PkceCodes { - code_verifier, - code_challenge, - } -} +pub(crate) use crate::oauth::PkceCodes; +pub(crate) use crate::oauth::generate_pkce; diff --git a/codex-rs/login/src/server.rs b/codex-rs/login/src/server.rs index 677c740da2..ca77cf7a16 100644 --- a/codex-rs/login/src/server.rs +++ b/codex-rs/login/src/server.rs @@ -27,10 +27,23 @@ use std::time::Duration; use crate::auth::AuthDotJson; use crate::auth::AuthKeyringBackendKind; use crate::auth::save_auth; +use crate::callback_params::LIFE_SCIENCES_OAUTH_STATE_SUFFIX; use crate::callback_params::LoginCallbackResult; -use crate::callback_params::login_callback_result_from_state; +use crate::callback_params::LoginOnboardingEntrypoint; use crate::default_client::create_raw_auth_client; use crate::default_client::originator; +use crate::oauth::AuthorizationCodeGrant; +use crate::oauth::AuthorizationRequest; +use crate::oauth::CallbackError; +use crate::oauth::CallbackParameters; +use crate::oauth::ErrorBodyLimit; +use crate::oauth::OAuthClient; +use crate::oauth::OAuthError; +use crate::oauth::TokenEncoding; +use crate::oauth::TokenEndpoint; +use crate::oauth::build_authorization_url; +use crate::oauth::generate_state; +use crate::oauth::sanitize_url_for_logging; use crate::outbound_proxy::AuthRouteConfig; use crate::pkce::PkceCodes; use crate::pkce::generate_pkce; @@ -40,12 +53,10 @@ use crate::success_page::compose_success_url; use crate::success_page::jwt_auth_claims; use crate::token_data::TokenData; use crate::token_data::parse_chatgpt_jwt_claims; -use base64::Engine; use chrono::Utc; use codex_config::types::AuthCredentialsStoreMode; use codex_protocol::auth::AuthMode; use codex_utils_template::Template; -use rand::RngCore; use serde_json::Value as JsonValue; use tiny_http::Header; use tiny_http::Request; @@ -181,7 +192,7 @@ pub fn run_login_server(opts: ServerOptions) -> io::Result { &pkce, &state, opts.forced_chatgpt_workspace_id.as_deref(), - ); + )?; if opts.open_browser { let _ = webbrowser::open(&auth_url); @@ -344,54 +355,50 @@ async fn process_request( match path.as_str() { "/auth/callback" => { - let params: std::collections::HashMap = - parsed_url.query_pairs().into_owned().collect(); - let has_code = params.get("code").is_some_and(|code| !code.is_empty()); - let has_state = params.get("state").is_some_and(|state| !state.is_empty()); - let has_error = params.get("error").is_some_and(|error| !error.is_empty()); - let callback_result = params - .get("state") - .and_then(|callback_state| login_callback_result_from_state(callback_state, state)); - let state_valid = callback_result.is_some(); - info!( - path = %path, - has_code, - has_state, - has_error, - state_valid, - "received login callback" - ); - if !state_valid { - warn!( - path = %path, - has_code, - has_state, - has_error, - "login callback state mismatch" - ); - return HandledRequest::Response( - Response::from_string("State mismatch").with_status_code(400), - ); + let mut params = CallbackParameters::from_url(&parsed_url); + let mut callback_result = LoginCallbackResult::default(); + // ChatGPT may append onboarding metadata to the otherwise exact callback state. + if let Some(callback_state) = params.state.as_mut() + && callback_state.strip_suffix(LIFE_SCIENCES_OAUTH_STATE_SUFFIX) == Some(state) + { + callback_state.truncate(state.len()); + callback_result.onboarding_entrypoint = + Some(LoginOnboardingEntrypoint::LifeSciences); } - if let Some(error_code) = params.get("error") { - let error_description = params.get("error_description").map(String::as_str); - let message = oauth_callback_error_message(error_code, error_description); - eprintln!("OAuth callback error: {message}"); - warn!( - error_code, - has_error_description = error_description.is_some_and(|s| !s.trim().is_empty()), - "oauth callback returned error" - ); - return login_error_response( - &message, - io::ErrorKind::PermissionDenied, - Some(error_code), - error_description, - ); - } - let code = match params.get("code") { - Some(c) if !c.is_empty() => c.clone(), - _ => { + let validation = params.validate(state); + let has_code = params.code.as_ref().is_some_and(|code| !code.is_empty()); + let has_state = params.state.as_ref().is_some_and(|state| !state.is_empty()); + let has_error = params.error.as_ref().is_some_and(|error| !error.is_empty()); + let state_valid = !matches!(validation, Err(CallbackError::StateMismatch)); + info!(%path, has_code, has_state, has_error, state_valid, "received login callback"); + let code = match validation { + Ok(code) => code, + Err(CallbackError::StateMismatch) => { + warn!(%path, has_code, has_state, has_error, "login callback state mismatch"); + return HandledRequest::Response( + Response::from_string("State mismatch").with_status_code(400), + ); + } + Err(CallbackError::Provider { + code: error_code, + description: error_description, + }) => { + let message = oauth_callback_error_message(error_code, error_description); + eprintln!("OAuth callback error: {message}"); + warn!( + error_code, + has_error_description = + error_description.is_some_and(|s| !s.trim().is_empty()), + "oauth callback returned error" + ); + return login_error_response( + &message, + io::ErrorKind::PermissionDenied, + Some(error_code), + error_description, + ); + } + Err(CallbackError::MissingCode) => { return login_error_response( "Missing authorization code. Sign-in could not be completed.", io::ErrorKind::InvalidData, @@ -400,14 +407,13 @@ async fn process_request( ); } }; - let callback_result = callback_result.unwrap_or_default(); match exchange_code_for_tokens( &opts.issuer, &opts.client_id, redirect_uri, pkce, - &code, + code, &opts.auth_route_config, ) .await @@ -580,41 +586,31 @@ fn build_authorize_url( pkce: &PkceCodes, state: &str, forced_chatgpt_workspace_ids: Option<&[String]>, -) -> String { - let mut query = vec![ - ("response_type".to_string(), "code".to_string()), - ("client_id".to_string(), client_id.to_string()), - ("redirect_uri".to_string(), redirect_uri.to_string()), - ( - "scope".to_string(), - "openid profile email offline_access api.connectors.read api.connectors.invoke" - .to_string(), - ), - ( - "code_challenge".to_string(), - pkce.code_challenge.to_string(), - ), - ("code_challenge_method".to_string(), "S256".to_string()), - ("id_token_add_organizations".to_string(), "true".to_string()), - ("codex_cli_simplified_flow".to_string(), "true".to_string()), - ("state".to_string(), state.to_string()), - ("originator".to_string(), originator().value), +) -> io::Result { + let originator = originator().value; + let workspace_ids = forced_chatgpt_workspace_ids.map(|ids| ids.join(",")); + let mut extra_parameters = vec![ + ("id_token_add_organizations", "true"), + ("codex_cli_simplified_flow", "true"), + ("originator", originator.as_str()), ]; - if let Some(workspace_ids) = forced_chatgpt_workspace_ids { - query.push(("allowed_workspace_id".to_string(), workspace_ids.join(","))); + if let Some(workspace_ids) = workspace_ids.as_deref() { + extra_parameters.push(("allowed_workspace_id", workspace_ids)); } - let qs = query - .into_iter() - .map(|(k, v)| format!("{k}={}", urlencoding::encode(&v))) - .collect::>() - .join("&"); - format!("{issuer}/oauth/authorize?{qs}") -} - -fn generate_state() -> String { - let mut bytes = [0u8; 32]; - rand::rng().fill_bytes(&mut bytes); - base64::engine::general_purpose::URL_SAFE_NO_PAD.encode(bytes) + build_authorization_url(AuthorizationRequest { + endpoint: &format!("{issuer}/oauth/authorize"), + client_id, + redirect_uri, + scope: Some( + "openid profile email offline_access api.connectors.read api.connectors.invoke", + ), + resource: None, + pkce, + state, + extra_parameters: &extra_parameters, + }) + .map(String::from) + .map_err(io::Error::other) } fn send_cancel_request(port: u16) -> io::Result<()> { @@ -695,111 +691,13 @@ fn bind_server(port: u16) -> io::Result { } /// Tokens returned by the OAuth authorization-code exchange. +#[derive(serde::Deserialize)] pub(crate) struct ExchangedTokens { pub id_token: String, pub access_token: String, pub refresh_token: String, } -#[derive(Debug, Clone, PartialEq, Eq)] -struct TokenEndpointErrorDetail { - error_code: Option, - error_message: Option, - display_message: String, -} - -impl std::fmt::Display for TokenEndpointErrorDetail { - fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { - self.display_message.fmt(f) - } -} - -const REDACTED_URL_VALUE: &str = ""; -const SENSITIVE_URL_QUERY_KEYS: &[&str] = &[ - "access_token", - "api_key", - "client_secret", - "code", - "code_verifier", - "id_token", - "key", - "refresh_token", - "requested_token", - "state", - "subject_token", - "token", -]; - -fn redact_sensitive_query_value(key: &str, value: &str) -> String { - if SENSITIVE_URL_QUERY_KEYS - .iter() - .any(|candidate| candidate.eq_ignore_ascii_case(key)) - { - REDACTED_URL_VALUE.to_string() - } else { - value.to_string() - } -} - -/// Redacts URL components that commonly carry auth secrets while preserving the host/path shape. -/// -/// This keeps developer-facing logs useful for debugging transport failures without persisting -/// tokens, callback codes, fragments, or embedded credentials. -fn redact_sensitive_url_parts(url: &mut url::Url) { - let _ = url.set_username(""); - let _ = url.set_password(None); - url.set_fragment(None); - - let query_pairs = url - .query_pairs() - .map(|(key, value)| { - let key = key.into_owned(); - let value = value.into_owned(); - (key.clone(), redact_sensitive_query_value(&key, &value)) - }) - .collect::>(); - - if query_pairs.is_empty() { - url.set_query(None); - return; - } - - let redacted_query = query_pairs - .into_iter() - .fold( - url::form_urlencoded::Serializer::new(String::new()), - |mut serializer, (key, value)| { - serializer.append_pair(&key, &value); - serializer - }, - ) - .finish(); - url.set_query(Some(&redacted_query)); -} - -/// Redacts any URL attached to an HTTP transport error before it is logged or returned. -fn redact_sensitive_error_url( - mut err: codex_http_client::HttpError, -) -> codex_http_client::HttpError { - if let Some(url) = err.url_mut() { - redact_sensitive_url_parts(url); - } - err -} - -/// Sanitizes a free-form URL string for structured logging. -/// -/// This is used for caller-supplied issuer values, which may contain credentials or query -/// parameters on non-default deployments. -fn sanitize_url_for_logging(url: &str) -> String { - match url::Url::parse(url) { - Ok(mut url) => { - redact_sensitive_url_parts(&mut url); - url.to_string() - } - Err(_) => "".to_string(), - } -} /// Exchanges an authorization code for tokens. /// /// The returned error remains suitable for user-facing CLI/browser surfaces, so backend-provided @@ -814,72 +712,61 @@ pub(crate) async fn exchange_code_for_tokens( code: &str, auth_route_config: &AuthRouteConfig, ) -> io::Result { - #[derive(serde::Deserialize)] - struct TokenResponse { - id_token: String, - access_token: String, - refresh_token: String, - } - - // The route selected for the issuer is reused for token exchange; the token endpoint path is - // not resolved separately. + // Reuse the route selected for the issuer, rather than resolving the token path again. let client = create_raw_auth_client(issuer.trim_end_matches('/'), auth_route_config)?; - let token_endpoint = format!("{}/oauth/token", issuer.trim_end_matches('/')); + let endpoint = format!("{}/oauth/token", issuer.trim_end_matches('/')); + let oauth = OAuthClient::new( + &client, + TokenEndpoint { + url: &endpoint, + client_id, + encoding: TokenEncoding::Form, + timeout: None, + error_body_limit: ErrorBodyLimit::Unlimited, + }, + ); info!( issuer = %sanitize_url_for_logging(issuer), - token_endpoint = %sanitize_url_for_logging(&token_endpoint), - redirect_uri = %redirect_uri, + token_endpoint = %sanitize_url_for_logging(&endpoint), + %redirect_uri, "starting oauth token exchange" ); - let resp = client - .post(token_endpoint) - .header("Content-Type", "application/x-www-form-urlencoded") - .body(format!( - "grant_type=authorization_code&code={}&redirect_uri={}&client_id={}&code_verifier={}", - urlencoding::encode(code), - urlencoding::encode(redirect_uri), - urlencoding::encode(client_id), - urlencoding::encode(&pkce.code_verifier) - )) - .send() - .await; - let resp = match resp { - Ok(resp) => resp, - Err(error) => { - let error = redact_sensitive_error_url(error); + match oauth + .exchange_code(AuthorizationCodeGrant { + code, + redirect_uri, + pkce, + resource: None, + }) + .await + { + Ok(tokens) => { + info!("oauth token exchange succeeded"); + Ok(tokens) + } + Err(OAuthError::Rejected(rejection)) => { + if let Some(error) = rejection.body_read_error { + return Err(io::Error::other(error)); + } + warn!( + status = %rejection.status, + detail = ?rejection.detail, + "oauth token exchange returned non-success status" + ); + Err(io::Error::other(rejection.to_string())) + } + Err(OAuthError::Transport(error)) => { error!( is_timeout = error.is_timeout(), is_connect = error.is_connect(), is_request = error.is_request(), - error = %error, + %error, "oauth token exchange transport failure" ); - return Err(io::Error::other(error)); + Err(io::Error::other(error)) } - }; - - let status = resp.status(); - if !status.is_success() { - let body = resp.text().await.map_err(io::Error::other)?; - let detail = parse_token_endpoint_error(&body); - warn!( - %status, - error_code = detail.error_code.as_deref().unwrap_or("unknown"), - error_message = detail.error_message.as_deref().unwrap_or("unknown"), - "oauth token exchange returned non-success status" - ); - return Err(io::Error::other(format!( - "token endpoint returned status {status}: {detail}" - ))); + Err(error @ OAuthError::InvalidResponse) => Err(io::Error::other(error)), } - - let tokens: TokenResponse = resp.json().await.map_err(io::Error::other)?; - info!(%status, "oauth token exchange succeeded"); - Ok(ExchangedTokens { - id_token: tokens.id_token, - access_token: tokens.access_token, - refresh_token: tokens.refresh_token, - }) } /// Persists exchanged credentials using the configured local auth store. @@ -1010,75 +897,6 @@ fn oauth_callback_error_message(error_code: &str, error_description: Option<&str format!("Sign-in failed: {error_code}") } -/// Extracts token endpoint error detail for both structured logging and caller-visible errors. -/// -/// Parsed JSON fields are safe to log individually. If the response is not JSON, the raw body is -/// preserved only for the returned error path so the CLI/browser can still surface the backend -/// detail, while the structured log path continues to use the explicitly parsed safe fields above. -fn parse_token_endpoint_error(body: &str) -> TokenEndpointErrorDetail { - let trimmed = body.trim(); - if trimmed.is_empty() { - return TokenEndpointErrorDetail { - error_code: None, - error_message: None, - display_message: "unknown error".to_string(), - }; - } - - let parsed = serde_json::from_str::(trimmed).ok(); - if let Some(json) = parsed { - let error_code = json - .get("error") - .and_then(JsonValue::as_str) - .filter(|error_code| !error_code.trim().is_empty()) - .map(ToString::to_string) - .or_else(|| { - json.get("error") - .and_then(JsonValue::as_object) - .and_then(|error_obj| error_obj.get("code")) - .and_then(JsonValue::as_str) - .filter(|code| !code.trim().is_empty()) - .map(ToString::to_string) - }); - if let Some(description) = json.get("error_description").and_then(JsonValue::as_str) - && !description.trim().is_empty() - { - return TokenEndpointErrorDetail { - error_code, - error_message: Some(description.to_string()), - display_message: description.to_string(), - }; - } - if let Some(error_obj) = json.get("error") - && let Some(message) = error_obj.get("message").and_then(JsonValue::as_str) - && !message.trim().is_empty() - { - return TokenEndpointErrorDetail { - error_code, - error_message: Some(message.to_string()), - display_message: message.to_string(), - }; - } - if let Some(error_code) = error_code { - return TokenEndpointErrorDetail { - display_message: error_code.clone(), - error_code: Some(error_code), - error_message: None, - }; - } - } - - // Preserve non-JSON token-endpoint bodies for the returned error so CLI/browser flows still - // surface the backend detail users and admins need, but keep that text out of structured logs - // by only logging explicitly parsed fields above and avoiding `%err` logging at the callback - // layer. - TokenEndpointErrorDetail { - error_code: None, - error_message: None, - display_message: trimmed.to_string(), - } -} - /// Renders the branded error page used by callback failures. fn render_login_error_page( message: &str, @@ -1172,114 +990,9 @@ pub(crate) async fn obtain_api_key( } #[cfg(test)] mod tests { - use pretty_assertions::assert_eq; - - use super::TokenEndpointErrorDetail; use super::html_escape; use super::is_missing_codex_entitlement_error; - use super::parse_token_endpoint_error; - use super::redact_sensitive_query_value; - use super::redact_sensitive_url_parts; use super::render_login_error_page; - use super::sanitize_url_for_logging; - - #[test] - fn parse_token_endpoint_error_prefers_error_description() { - let detail = parse_token_endpoint_error( - r#"{"error":"invalid_grant","error_description":"refresh token expired"}"#, - ); - - assert_eq!( - detail, - TokenEndpointErrorDetail { - error_code: Some("invalid_grant".to_string()), - error_message: Some("refresh token expired".to_string()), - display_message: "refresh token expired".to_string(), - } - ); - } - - #[test] - fn parse_token_endpoint_error_reads_nested_error_message_and_code() { - let detail = parse_token_endpoint_error( - r#"{"error":{"code":"proxy_auth_required","message":"proxy authentication required"}}"#, - ); - - assert_eq!( - detail, - TokenEndpointErrorDetail { - error_code: Some("proxy_auth_required".to_string()), - error_message: Some("proxy authentication required".to_string()), - display_message: "proxy authentication required".to_string(), - } - ); - } - - #[test] - fn parse_token_endpoint_error_falls_back_to_error_code() { - let detail = parse_token_endpoint_error(r#"{"error":"temporarily_unavailable"}"#); - - assert_eq!( - detail, - TokenEndpointErrorDetail { - error_code: Some("temporarily_unavailable".to_string()), - error_message: None, - display_message: "temporarily_unavailable".to_string(), - } - ); - } - - #[test] - fn parse_token_endpoint_error_preserves_plain_text_for_display() { - let detail = parse_token_endpoint_error("service unavailable"); - - assert_eq!( - detail, - TokenEndpointErrorDetail { - error_code: None, - error_message: None, - display_message: "service unavailable".to_string(), - } - ); - } - - #[test] - fn redact_sensitive_query_value_only_scrubs_known_keys() { - assert_eq!( - redact_sensitive_query_value("code", "abc123"), - "".to_string() - ); - assert_eq!( - redact_sensitive_query_value("redirect_uri", "http://localhost:1455/auth/callback"), - "http://localhost:1455/auth/callback".to_string() - ); - } - - #[test] - fn redact_sensitive_url_parts_preserves_safe_url_shape() { - let mut url = url::Url::parse( - "https://user:pass@auth.openai.com/oauth/token?code=abc123&redirect_uri=http%3A%2F%2Flocalhost%3A1455%2Fauth%2Fcallback#frag", - ) - .expect("valid url"); - - redact_sensitive_url_parts(&mut url); - - assert_eq!( - url.as_str(), - "https://auth.openai.com/oauth/token?code=%3Credacted%3E&redirect_uri=http%3A%2F%2Flocalhost%3A1455%2Fauth%2Fcallback" - ); - } - - #[test] - fn sanitize_url_for_logging_redacts_sensitive_issuer_parts() { - let redacted = - sanitize_url_for_logging("https://user:pass@example.com/base?token=abc123&env=prod"); - - assert_eq!( - redacted, - "https://example.com/base?token=%3Credacted%3E&env=prod".to_string() - ); - } #[test] fn render_login_error_page_escapes_dynamic_fields() { diff --git a/codex-rs/login/tests/suite/auth_refresh.rs b/codex-rs/login/tests/suite/auth_refresh.rs index 3a8579282d..5e353de173 100644 --- a/codex-rs/login/tests/suite/auth_refresh.rs +++ b/codex-rs/login/tests/suite/auth_refresh.rs @@ -1021,7 +1021,8 @@ async fn refresh_token_does_not_retry_after_standard_invalid_grant_failure() -> let ctx = RefreshTokenTestContext::new(&server).await?; let initial_last_refresh = Utc::now() - Duration::days(1); - let initial_tokens = build_tokens(INITIAL_ACCESS_TOKEN, INITIAL_REFRESH_TOKEN); + // Redacting a credential that overlaps the error code must not change recovery policy. + let initial_tokens = build_tokens(INITIAL_ACCESS_TOKEN, "invalid"); let initial_auth = AuthDotJson { auth_mode: Some(AuthMode::Chatgpt), openai_api_key: None, @@ -1228,56 +1229,60 @@ async fn refresh_token_reloads_changed_auth_after_permanent_failure() -> Result< #[serial_test::serial(auth_env)] #[tokio::test] -async fn refresh_token_returns_transient_error_on_server_failure() -> Result<()> { +async fn refresh_token_preserves_credentials_on_server_or_decode_failure() -> Result<()> { skip_if_no_network!(Ok(())); - let server = MockServer::start().await; - Mock::given(method("POST")) - .and(path("/oauth/token")) - .respond_with(ResponseTemplate::new(500).set_body_json(json!({ - "error": "temporary-failure" - }))) - .expect(1) - .mount(&server) - .await; + for (status, body) in [ + (500, json!({"error": "temporary-failure"})), + (200, json!(INITIAL_REFRESH_TOKEN)), + ] { + let server = MockServer::start().await; + Mock::given(method("POST")) + .and(path("/oauth/token")) + .respond_with(ResponseTemplate::new(status).set_body_json(body)) + .expect(1) + .mount(&server) + .await; - let ctx = RefreshTokenTestContext::new(&server).await?; - let initial_last_refresh = Utc::now() - Duration::days(1); - 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.clone()), - last_refresh: Some(initial_last_refresh), - agent_identity: None, - personal_access_token: None, - bedrock_api_key: None, - bedrock_access_keys: None, - }; - ctx.write_auth(&initial_auth).await?; + let ctx = RefreshTokenTestContext::new(&server).await?; + let initial_last_refresh = Utc::now() - Duration::days(1); + 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.clone()), + last_refresh: Some(initial_last_refresh), + agent_identity: None, + personal_access_token: None, + bedrock_api_key: None, + bedrock_access_keys: None, + }; + ctx.write_auth(&initial_auth).await?; - let err = ctx - .auth_manager - .refresh_token_from_authority() - .await - .err() - .context("refresh should fail")?; - assert!(matches!(err, RefreshTokenError::Transient(_))); - assert_eq!(err.failed_reason(), None); + let err = ctx + .auth_manager + .refresh_token_from_authority() + .await + .err() + .context("refresh should fail")?; + assert!(matches!(err, RefreshTokenError::Transient(_))); + assert_eq!(err.failed_reason(), None); + assert!(!format!("{err} {err:?}").contains(INITIAL_REFRESH_TOKEN)); - 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); + 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); - server.verify().await; + server.verify().await; + } Ok(()) } diff --git a/codex-rs/login/tests/suite/login_server_e2e.rs b/codex-rs/login/tests/suite/login_server_e2e.rs index fc1b4e65bf..8e771735f5 100644 --- a/codex-rs/login/tests/suite/login_server_e2e.rs +++ b/codex-rs/login/tests/suite/login_server_e2e.rs @@ -151,6 +151,31 @@ async fn end_to_end_login_flow_persists_auth_json() -> Result<()> { let client = HttpClientBuilder::new() .without_redirects() .build_direct()?; + // Reject unrecognized metadata before processing codes or provider errors. + for state in [ + "wrong_state.onboarding_entrypoint=life_sciences", + "test_state_123.onboarding_entrypoint=unknown", + "test_state_123.onboarding_entrypoint=life_sciences.onboarding_entrypoint=life_sciences", + "test_state_123.extra=value.onboarding_entrypoint=life_sciences", + ] { + let response = client + .get(format!("http://127.0.0.1:{login_port}/auth/callback")) + .query(&[ + ("state", state), + ("code", "untrusted"), + ("error", "access_denied"), + ]) + .send() + .await?; + assert_eq!(response.status(), 400); + assert_eq!(response.text().await?, "State mismatch"); + } + assert_eq!( + serde_json::from_str::(&std::fs::read_to_string( + codex_home.join("auth.json") + )?)?, + stale_auth + ); let url = format!( "http://127.0.0.1:{login_port}/auth/callback?code=abc&state=test_state_123.onboarding_entrypoint=life_sciences" );