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" );