From abf27f8d642c9739bc42cbe68406ad1f51be170d Mon Sep 17 00:00:00 2001 From: Josh McKinney Date: Wed, 18 Mar 2026 14:42:06 -0700 Subject: [PATCH] feat: expose browser login progress phases Add a shared progress and failure model for browser OAuth login, with CLI rendering for user-facing milestones and classified failures. Split token-exchange error handling into a private type that preserves useful transport detail without logging sensitive URL data, and cover the rendered progress and failure text with per-message snapshots. Co-authored-by: Codex --- codex-rs/Cargo.lock | 2 + codex-rs/cli/src/login.rs | 105 +++- codex-rs/login/Cargo.toml | 2 + codex-rs/login/src/lib.rs | 7 + codex-rs/login/src/progress.rs | 477 +++++++++++++++++ codex-rs/login/src/server.rs | 498 +++++++++++------- ..._tests__failure_browser_launch_failed.snap | 8 + ...ests__failure_callback_state_mismatch.snap | 8 + ...sts__failure_local_server_unavailable.snap | 8 + ...gress__tests__failure_login_cancelled.snap | 8 + ...s__failure_missing_authorization_code.snap | 8 + ...ogress__tests__failure_persist_failed.snap | 8 + ...ests__failure_provider_callback_error.snap | 8 + ...gress__tests__failure_redirect_failed.snap | 8 + ...ests__failure_token_endpoint_rejected.snap | 8 + ...tests__failure_token_exchange_connect.snap | 8 + ...tests__failure_token_exchange_request.snap | 8 + ...tests__failure_token_exchange_timeout.snap | 8 + ...sts__failure_token_response_malformed.snap | 8 + ..._tests__failure_workspace_restriction.snap | 8 + ...ss__tests__progress_callback_received.snap | 5 + ...s__progress_cleanup_old_login_session.snap | 5 + ...ress__tests__progress_opening_browser.snap | 5 + ..._tests__progress_retry_local_listener.snap | 5 + ...s__tests__progress_saving_credentials.snap | 5 + codex-rs/login/src/token_exchange_error.rs | 296 +++++++++++ 26 files changed, 1303 insertions(+), 221 deletions(-) create mode 100644 codex-rs/login/src/progress.rs create mode 100644 codex-rs/login/src/snapshots/codex_login__progress__tests__failure_browser_launch_failed.snap create mode 100644 codex-rs/login/src/snapshots/codex_login__progress__tests__failure_callback_state_mismatch.snap create mode 100644 codex-rs/login/src/snapshots/codex_login__progress__tests__failure_local_server_unavailable.snap create mode 100644 codex-rs/login/src/snapshots/codex_login__progress__tests__failure_login_cancelled.snap create mode 100644 codex-rs/login/src/snapshots/codex_login__progress__tests__failure_missing_authorization_code.snap create mode 100644 codex-rs/login/src/snapshots/codex_login__progress__tests__failure_persist_failed.snap create mode 100644 codex-rs/login/src/snapshots/codex_login__progress__tests__failure_provider_callback_error.snap create mode 100644 codex-rs/login/src/snapshots/codex_login__progress__tests__failure_redirect_failed.snap create mode 100644 codex-rs/login/src/snapshots/codex_login__progress__tests__failure_token_endpoint_rejected.snap create mode 100644 codex-rs/login/src/snapshots/codex_login__progress__tests__failure_token_exchange_connect.snap create mode 100644 codex-rs/login/src/snapshots/codex_login__progress__tests__failure_token_exchange_request.snap create mode 100644 codex-rs/login/src/snapshots/codex_login__progress__tests__failure_token_exchange_timeout.snap create mode 100644 codex-rs/login/src/snapshots/codex_login__progress__tests__failure_token_response_malformed.snap create mode 100644 codex-rs/login/src/snapshots/codex_login__progress__tests__failure_workspace_restriction.snap create mode 100644 codex-rs/login/src/snapshots/codex_login__progress__tests__progress_callback_received.snap create mode 100644 codex-rs/login/src/snapshots/codex_login__progress__tests__progress_cleanup_old_login_session.snap create mode 100644 codex-rs/login/src/snapshots/codex_login__progress__tests__progress_opening_browser.snap create mode 100644 codex-rs/login/src/snapshots/codex_login__progress__tests__progress_retry_local_listener.snap create mode 100644 codex-rs/login/src/snapshots/codex_login__progress__tests__progress_saving_credentials.snap create mode 100644 codex-rs/login/src/token_exchange_error.rs diff --git a/codex-rs/Cargo.lock b/codex-rs/Cargo.lock index 771b714e0a..03c2b8a44f 100644 --- a/codex-rs/Cargo.lock +++ b/codex-rs/Cargo.lock @@ -2164,6 +2164,7 @@ dependencies = [ "codex-client", "codex-core", "core_test_support", + "insta", "pretty_assertions", "rand 0.9.2", "reqwest", @@ -2171,6 +2172,7 @@ dependencies = [ "serde_json", "sha2", "tempfile", + "thiserror 2.0.18", "tiny_http", "tokio", "tracing", diff --git a/codex-rs/cli/src/login.rs b/codex-rs/cli/src/login.rs index a663f393cf..96adcac153 100644 --- a/codex-rs/cli/src/login.rs +++ b/codex-rs/cli/src/login.rs @@ -16,7 +16,7 @@ use codex_core::auth::logout; use codex_core::config::Config; use codex_login::ServerOptions; use codex_login::run_device_code_login; -use codex_login::run_login_server; +use codex_login::run_login_server_with_progress; use codex_protocol::config_types::ForcedLoginMethod; use codex_utils_cli::CliConfigOverrides; use std::fs::OpenOptions; @@ -34,7 +34,7 @@ const CHATGPT_LOGIN_DISABLED_MESSAGE: &str = "ChatGPT login is disabled. Use API key login instead."; const API_KEY_LOGIN_DISABLED_MESSAGE: &str = "API key login is disabled. Use ChatGPT login instead."; -const LOGIN_SUCCESS_MESSAGE: &str = "Successfully logged in"; +const LOGIN_SUCCESS_MESSAGE: &str = "Signed in. You're good to go."; /// Installs a small file-backed tracing layer for direct `codex login` flows. /// @@ -104,12 +104,64 @@ fn init_login_file_logging(config: &Config) -> Option { Some(guard) } -fn print_login_server_start(actual_port: u16, auth_url: &str) { +fn print_login_server_start(auth_url: &str) { eprintln!( - "Starting local login server on http://localhost:{actual_port}.\nIf your browser did not open, navigate to this URL to authenticate:\n\n{auth_url}\n\nOn a remote or headless machine? Use `codex login --device-auth` instead." + "If a browser window didn't open, use this link to sign in:\n\n{auth_url}\n\nOn a remote or headless machine? Use `codex login --device-auth` instead." ); } +/// Starts the background task that renders browser-login progress events to stderr. +/// +/// The returned sender is passed into `codex-login` for one concrete attempt, and the join handle +/// resolves to `true` if a structured failure was rendered. Callers use that boolean to avoid +/// printing a second generic terminal error after the progress stream already produced the +/// user-facing failure block. +fn spawn_browser_login_progress_printer() -> ( + codex_login::LoginProgressSender, + tokio::task::JoinHandle, +) { + let (progress_tx, mut progress_rx) = + tokio::sync::mpsc::unbounded_channel::(); + + let progress_task = tokio::spawn(async move { + let mut saw_failure = false; + while let Some(phase) = progress_rx.recv().await { + if matches!(phase, codex_login::LoginPhase::Failed { .. }) { + saw_failure = true; + } + if phase.is_user_visible() { + eprintln!("{phase}"); + } + } + saw_failure + }); + + (progress_tx, progress_task) +} + +/// Runs one browser login attempt with CLI progress rendering attached. +/// +/// The result pairs the underlying login outcome with whether the progress printer already showed +/// a structured failure. Callers should only emit a fallback `Error logging in: ...` line when the +/// second value is `false`; otherwise the user would see duplicate error messages for the same +/// failure. +async fn run_browser_login(opts: ServerOptions) -> (std::io::Result<()>, bool) { + let (progress_tx, progress_task) = spawn_browser_login_progress_printer(); + let server = match run_login_server_with_progress(opts, progress_tx) { + Ok(server) => server, + Err(err) => { + let saw_failure = progress_task.await.unwrap_or(false); + return (Err(err), saw_failure); + } + }; + + print_login_server_start(&server.auth_url); + + let result = server.block_until_done().await; + let saw_failure = progress_task.await.unwrap_or(false); + (result, saw_failure) +} + pub async fn login_with_chatgpt( codex_home: PathBuf, forced_chatgpt_workspace_id: Option, @@ -121,11 +173,8 @@ pub async fn login_with_chatgpt( forced_chatgpt_workspace_id, cli_auth_credentials_store_mode, ); - let server = run_login_server(opts)?; - - print_login_server_start(server.actual_port, &server.auth_url); - - server.block_until_done().await + let (result, _saw_failure) = run_browser_login(opts).await; + result } pub async fn run_login_with_chatgpt(cli_config_overrides: CliConfigOverrides) -> ! { @@ -140,19 +189,23 @@ pub async fn run_login_with_chatgpt(cli_config_overrides: CliConfigOverrides) -> let forced_chatgpt_workspace_id = config.forced_chatgpt_workspace_id.clone(); - match login_with_chatgpt( + let opts = ServerOptions::new( config.codex_home, + CLIENT_ID.to_string(), forced_chatgpt_workspace_id, config.cli_auth_credentials_store_mode, - ) - .await - { - Ok(_) => { + ); + + let (result, saw_structured_failure) = run_browser_login(opts).await; + match result { + Ok(()) => { eprintln!("{LOGIN_SUCCESS_MESSAGE}"); std::process::exit(0); } Err(e) => { - eprintln!("Error logging in: {e}"); + if !saw_structured_failure { + eprintln!("Error logging in: {e}"); + } std::process::exit(1); } } @@ -286,22 +339,16 @@ pub async fn run_login_with_device_code_fallback_to_browser( Err(e) => { if e.kind() == std::io::ErrorKind::NotFound { eprintln!("Device code login is not enabled; falling back to browser login."); - match run_login_server(opts) { - Ok(server) => { - print_login_server_start(server.actual_port, &server.auth_url); - match server.block_until_done().await { - Ok(()) => { - eprintln!("{LOGIN_SUCCESS_MESSAGE}"); - std::process::exit(0); - } - Err(e) => { - eprintln!("Error logging in: {e}"); - std::process::exit(1); - } - } + let (result, saw_structured_failure) = run_browser_login(opts).await; + match result { + Ok(()) => { + eprintln!("{LOGIN_SUCCESS_MESSAGE}"); + std::process::exit(0); } Err(e) => { - eprintln!("Error logging in: {e}"); + if !saw_structured_failure { + eprintln!("Error logging in: {e}"); + } std::process::exit(1); } } diff --git a/codex-rs/login/Cargo.toml b/codex-rs/login/Cargo.toml index 5524fec7c1..140fddedce 100644 --- a/codex-rs/login/Cargo.toml +++ b/codex-rs/login/Cargo.toml @@ -19,6 +19,7 @@ serde = { workspace = true, features = ["derive"] } serde_json = { workspace = true } sha2 = { workspace = true } tiny_http = { workspace = true } +thiserror = { workspace = true } tokio = { workspace = true, features = [ "io-std", "macros", @@ -34,6 +35,7 @@ webbrowser = { workspace = true } [dev-dependencies] anyhow = { workspace = true } core_test_support = { workspace = true } +insta = { workspace = true } pretty_assertions = { workspace = true } tempfile = { workspace = true } wiremock = { workspace = true } diff --git a/codex-rs/login/src/lib.rs b/codex-rs/login/src/lib.rs index 60b0c57f28..63c851a667 100644 --- a/codex-rs/login/src/lib.rs +++ b/codex-rs/login/src/lib.rs @@ -1,16 +1,23 @@ mod device_code_auth; mod pkce; +mod progress; mod server; +mod token_exchange_error; pub use codex_client::BuildCustomCaTransportError as BuildLoginHttpClientError; pub use device_code_auth::DeviceCode; pub use device_code_auth::complete_device_code_login; pub use device_code_auth::request_device_code; pub use device_code_auth::run_device_code_login; +pub use progress::LoginFailureCategory; +pub use progress::LoginFailurePhase; +pub use progress::LoginPhase; +pub use progress::LoginProgressSender; pub use server::LoginServer; pub use server::ServerOptions; pub use server::ShutdownHandle; pub use server::run_login_server; +pub use server::run_login_server_with_progress; // Re-export commonly used auth types and helpers from codex-core for compatibility pub use codex_app_server_protocol::AuthMode; diff --git a/codex-rs/login/src/progress.rs b/codex-rs/login/src/progress.rs new file mode 100644 index 0000000000..ef7021fa7e --- /dev/null +++ b/codex-rs/login/src/progress.rs @@ -0,0 +1,477 @@ +//! Progress, failure vocabulary, and default text rendering for OAuth login. +//! +//! This module describes what happened in the login flow in a UI-neutral form. The `codex-login` +//! crate emits phases and coarse failure categories so direct CLI, TUI, or app-server callers can +//! choose their own presentation while still sharing the same state machine and support-oriented +//! error buckets. +//! +//! The types here are intentionally coarse and redaction-friendly. Callback events report only the +//! presence or validity of sensitive query fields, never the raw authorization code, state value, +//! or provider error payload, and the failure category is a stable bucket rather than a complete +//! transport diagnosis. + +const LOGIN_HELP_URL: &str = "https://developers.openai.com/codex/auth"; + +/// Coarse-grained progress phases for the OAuth login flow. +/// +/// This is a flow contract, not a direct UI contract. Callers should treat variants as lifecycle +/// milestones and decide locally which ones are worth rendering to users; many phases exist only so +/// logs, tests, and future UIs can tell where a failure occurred. +#[derive(Debug, Clone, PartialEq, Eq)] +pub enum LoginPhase { + /// The localhost callback server is attempting to bind its listening port. + BindingLocalServer { + /// One-based bind attempt count. + attempt: u32, + }, + /// A previous login server appears to still own the callback port, so this attempt is + /// cancelling the stale session before retrying. + PreviousLoginServerDetected { + /// One-based bind attempt count that observed the stale server. + attempt: u32, + }, + /// The local callback listener is bound and has an assigned port. + LocalServerBound { + /// The bound localhost port for the OAuth redirect URI. + port: u16, + }, + /// Codex is launching the browser for the provider authorization page. + OpeningBrowser, + /// Codex is waiting for the browser to return to the localhost callback. + WaitingForCallback, + /// A callback request reached the local server. + CallbackReceived { + /// Whether the callback carried an authorization code parameter. + has_code: bool, + /// Whether the callback carried a state parameter. + has_state: bool, + /// Whether the callback carried a provider error parameter. + has_error: bool, + /// Whether the received state matched the one generated for this attempt. + state_valid: bool, + }, + /// Codex is exchanging the authorization code for tokens at the token endpoint. + ExchangingToken, + /// Codex is writing the resulting credentials to local storage. + PersistingCredentials, + /// The browser login flow finished successfully. + Succeeded, + /// The browser login flow failed. + Failed { + /// The stage of the login flow where the failure occurred. + phase: LoginFailurePhase, + /// The stable coarse-grained failure bucket for UI and support. + category: LoginFailureCategory, + /// Human-readable detail for the specific failure instance. + message: String, + }, +} + +impl LoginPhase { + /// Returns whether this phase should be rendered by the default direct-CLI progress UI. + /// + /// Some phases are useful for logging and tests but too noisy for normal stderr output. + pub fn is_user_visible(&self) -> bool { + match self { + LoginPhase::BindingLocalServer { attempt } => *attempt > 1, + LoginPhase::PreviousLoginServerDetected { .. } + | LoginPhase::OpeningBrowser + | LoginPhase::PersistingCredentials + | LoginPhase::Failed { .. } => true, + LoginPhase::CallbackReceived { + has_error, + state_valid, + .. + } => !has_error && *state_valid, + LoginPhase::LocalServerBound { .. } + | LoginPhase::WaitingForCallback + | LoginPhase::ExchangingToken + | LoginPhase::Succeeded => false, + } + } +} + +impl std::fmt::Display for LoginPhase { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + match self { + LoginPhase::BindingLocalServer { attempt } => { + if *attempt > 1 { + write!( + f, + "Retrying the local sign-in listener... (attempt {attempt})" + ) + } else { + f.write_str("Starting local sign-in listener...") + } + } + LoginPhase::PreviousLoginServerDetected { .. } => { + f.write_str("Cleaning up an old login session first...") + } + LoginPhase::LocalServerBound { port } => { + write!(f, "Local callback server ready on http://localhost:{port}") + } + LoginPhase::OpeningBrowser => f.write_str("Opening your browser to sign in..."), + LoginPhase::WaitingForCallback => f.write_str("Waiting for browser sign-in..."), + LoginPhase::CallbackReceived { + has_error, + state_valid, + .. + } => { + if !has_error && *state_valid { + f.write_str("Browser sign-in received. Finishing up...") + } else { + f.write_str("Browser sign-in callback received") + } + } + LoginPhase::ExchangingToken => { + f.write_str("Exchanging authorization code for tokens...") + } + LoginPhase::PersistingCredentials => f.write_str("Saving your Codex credentials..."), + LoginPhase::Succeeded => f.write_str("Signed in. You're good to go."), + LoginPhase::Failed { + phase, + category, + message, + } => write!( + f, + "{}\nCodex couldn't finish signing in while {phase}. {}\nHelp: {LOGIN_HELP_URL}\nDetails: {category} - {message}", + category.title(), + category.help() + ), + } + } +} + +/// Where in the login flow a failure happened. +/// +/// This locates the failing stage independently from the failure category. For example, a token +/// exchange can fail because the server rejected the request or because the network path was +/// unavailable; both happen in `ExchangeToken`, but they produce different categories. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum LoginFailurePhase { + /// Failed before or while binding the localhost callback server. + BindLocalServer, + /// Failed while launching the browser. + OpenBrowser, + /// Failed while waiting for the OAuth callback to arrive. + WaitForCallback, + /// Failed while validating callback parameters such as state or authorization code presence. + ValidateCallback, + /// Failed while exchanging the authorization code for tokens. + ExchangeToken, + /// Failed while persisting credentials after a successful token exchange. + PersistCredentials, + /// Failed while redirecting the browser back into Codex after the callback completed. + RedirectBackToCodex, +} + +impl std::fmt::Display for LoginFailurePhase { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(match self { + LoginFailurePhase::BindLocalServer => "binding local callback server", + LoginFailurePhase::OpenBrowser => "opening browser", + LoginFailurePhase::WaitForCallback => "waiting for OAuth callback", + LoginFailurePhase::ValidateCallback => "validating OAuth callback", + LoginFailurePhase::ExchangeToken => "exchanging authorization code for tokens", + LoginFailurePhase::PersistCredentials => "saving credentials locally", + LoginFailurePhase::RedirectBackToCodex => "redirecting back to Codex", + }) + } +} + +/// Stable high-level failure categories for UI and support messaging. +/// +/// These names are intended to be durable enough for snapshots, support references, and future UI +/// branching. They are broader than the underlying transport or provider errors by design, so a +/// caller should not assume one category maps to exactly one root cause. +#[derive(Debug, Clone, Copy, PartialEq, Eq)] +pub enum LoginFailureCategory { + /// The local callback server could not become available. + LocalServerUnavailable, + /// The system browser could not be launched. + BrowserLaunchFailed, + /// The login attempt was cancelled before completion. + LoginCancelled, + /// The callback state did not match the state generated for this attempt. + CallbackStateMismatch, + /// The provider redirected back with an OAuth error instead of a code. + ProviderCallbackError, + /// The callback did not include an authorization code. + MissingAuthorizationCode, + /// The token endpoint request timed out. + TokenExchangeTimeout, + /// The token endpoint could not be reached due to a connect-level failure. + TokenExchangeConnect, + /// The token endpoint request failed for a non-timeout, non-connect transport reason. + TokenExchangeRequest, + /// The token endpoint returned a non-success HTTP status. + TokenEndpointRejected, + /// The token endpoint returned a response body Codex could not parse. + TokenResponseMalformed, + /// The signed-in account is not allowed in the selected workspace context. + WorkspaceRestriction, + /// Codex could not save credentials locally. + PersistFailed, + /// Codex could not send the browser to the final post-login page. + RedirectFailed, +} + +impl std::fmt::Display for LoginFailureCategory { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + f.write_str(match self { + LoginFailureCategory::LocalServerUnavailable => "local_server_unavailable", + LoginFailureCategory::BrowserLaunchFailed => "browser_launch_failed", + LoginFailureCategory::LoginCancelled => "login_cancelled", + LoginFailureCategory::CallbackStateMismatch => "callback_state_mismatch", + LoginFailureCategory::ProviderCallbackError => "provider_callback_error", + LoginFailureCategory::MissingAuthorizationCode => "missing_authorization_code", + LoginFailureCategory::TokenExchangeTimeout => "token_exchange_timeout", + LoginFailureCategory::TokenExchangeConnect => "token_exchange_connect", + LoginFailureCategory::TokenExchangeRequest => "token_exchange_request", + LoginFailureCategory::TokenEndpointRejected => "token_endpoint_rejected", + LoginFailureCategory::TokenResponseMalformed => "token_response_malformed", + LoginFailureCategory::WorkspaceRestriction => "workspace_restriction", + LoginFailureCategory::PersistFailed => "persist_failed", + LoginFailureCategory::RedirectFailed => "redirect_failed", + }) + } +} + +impl LoginFailureCategory { + /// Returns the short user-facing title for this failure category. + pub fn title(self) -> &'static str { + match self { + LoginFailureCategory::BrowserLaunchFailed => "Couldn't open your browser", + LoginFailureCategory::LocalServerUnavailable => "Couldn't start local sign-in", + LoginFailureCategory::LoginCancelled => "Sign-in was cancelled", + LoginFailureCategory::CallbackStateMismatch => "Couldn't verify this sign-in attempt", + LoginFailureCategory::ProviderCallbackError => "The sign-in page reported a problem", + LoginFailureCategory::MissingAuthorizationCode => "Didn't get a sign-in code back", + LoginFailureCategory::TokenExchangeTimeout => { + "The auth server took too long to respond" + } + LoginFailureCategory::TokenExchangeConnect + | LoginFailureCategory::TokenExchangeRequest => "Couldn't reach the auth server", + LoginFailureCategory::TokenEndpointRejected => "The auth server rejected the sign-in", + LoginFailureCategory::TokenResponseMalformed => { + "The auth server sent an unexpected response" + } + LoginFailureCategory::WorkspaceRestriction => "This account can't be used here", + LoginFailureCategory::PersistFailed => "Couldn't save your Codex credentials", + LoginFailureCategory::RedirectFailed => "Couldn't return to Codex after sign-in", + } + } + + /// Returns one concrete recovery hint for this failure category. + pub fn help(self) -> &'static str { + match self { + LoginFailureCategory::BrowserLaunchFailed => { + "Use the sign-in link printed above, or run `codex login --device-auth` on a remote machine." + } + LoginFailureCategory::LocalServerUnavailable => { + "Retry in a moment. If it keeps happening, another process may be holding the local callback port." + } + LoginFailureCategory::LoginCancelled => "Run `codex login` to try again.", + LoginFailureCategory::CallbackStateMismatch => { + "Retry sign-in from the same terminal, and avoid reusing an old browser tab." + } + LoginFailureCategory::ProviderCallbackError => { + "Try again, switch accounts, or contact your workspace admin if access is restricted." + } + LoginFailureCategory::MissingAuthorizationCode => { + "Try again. If it keeps happening, restart the login flow from Codex." + } + LoginFailureCategory::TokenExchangeTimeout => { + "Check your network connection or proxy, then try again." + } + LoginFailureCategory::TokenExchangeConnect + | LoginFailureCategory::TokenExchangeRequest => { + "Check your network, proxy, or custom CA setup, then try again." + } + LoginFailureCategory::TokenEndpointRejected => { + "Try again. If this repeats, your account or workspace may not be allowed to use Codex." + } + LoginFailureCategory::TokenResponseMalformed => { + "Try again. If this repeats, contact support with the details below." + } + LoginFailureCategory::WorkspaceRestriction => { + "Switch to an allowed account or contact your workspace admin." + } + LoginFailureCategory::PersistFailed => { + "Check permissions for your Codex home directory, then try again." + } + LoginFailureCategory::RedirectFailed => "Return to Codex and retry sign-in.", + } + } +} + +/// Channel sender used to publish browser-login progress events. +/// +/// The sender carries structured phases rather than formatted text so the login crate does not bake +/// in one presentation style. A caller that needs human-facing copy should render it at the UI +/// boundary instead of matching on these variants inside the auth flow. +pub type LoginProgressSender = tokio::sync::mpsc::UnboundedSender; + +#[cfg(test)] +mod tests { + use super::LoginFailureCategory; + use super::LoginFailurePhase; + use super::LoginPhase; + + #[test] + fn login_progress_snapshots() { + let samples = [ + ( + "progress_retry_local_listener", + LoginPhase::BindingLocalServer { attempt: 2 }, + ), + ( + "progress_cleanup_old_login_session", + LoginPhase::PreviousLoginServerDetected { attempt: 1 }, + ), + ("progress_opening_browser", LoginPhase::OpeningBrowser), + ( + "progress_callback_received", + LoginPhase::CallbackReceived { + has_code: true, + has_state: true, + has_error: false, + state_valid: true, + }, + ), + ( + "progress_saving_credentials", + LoginPhase::PersistingCredentials, + ), + ]; + + for (name, phase) in samples { + insta::assert_snapshot!(name, phase.to_string()); + } + } + + #[test] + fn login_failure_snapshots() { + let samples = [ + ( + "failure_local_server_unavailable", + LoginPhase::Failed { + phase: LoginFailurePhase::BindLocalServer, + category: LoginFailureCategory::LocalServerUnavailable, + message: "Port 127.0.0.1:1455 is already in use after 2000 ms".to_string(), + }, + ), + ( + "failure_browser_launch_failed", + LoginPhase::Failed { + phase: LoginFailurePhase::OpenBrowser, + category: LoginFailureCategory::BrowserLaunchFailed, + message: "No browser found".to_string(), + }, + ), + ( + "failure_login_cancelled", + LoginPhase::Failed { + phase: LoginFailurePhase::WaitForCallback, + category: LoginFailureCategory::LoginCancelled, + message: "Login was not completed".to_string(), + }, + ), + ( + "failure_callback_state_mismatch", + LoginPhase::Failed { + phase: LoginFailurePhase::ValidateCallback, + category: LoginFailureCategory::CallbackStateMismatch, + message: "State mismatch".to_string(), + }, + ), + ( + "failure_provider_callback_error", + LoginPhase::Failed { + phase: LoginFailurePhase::ValidateCallback, + category: LoginFailureCategory::ProviderCallbackError, + message: "Sign-in failed: access_denied".to_string(), + }, + ), + ( + "failure_missing_authorization_code", + LoginPhase::Failed { + phase: LoginFailurePhase::ValidateCallback, + category: LoginFailureCategory::MissingAuthorizationCode, + message: "Missing authorization code. Sign-in could not be completed." + .to_string(), + }, + ), + ( + "failure_token_exchange_timeout", + LoginPhase::Failed { + phase: LoginFailurePhase::ExchangeToken, + category: LoginFailureCategory::TokenExchangeTimeout, + message: "operation timed out".to_string(), + }, + ), + ( + "failure_token_exchange_connect", + LoginPhase::Failed { + phase: LoginFailurePhase::ExchangeToken, + category: LoginFailureCategory::TokenExchangeConnect, + message: + "error sending request (endpoint: https://auth.openai.com/oauth/token)" + .to_string(), + }, + ), + ( + "failure_token_exchange_request", + LoginPhase::Failed { + phase: LoginFailurePhase::ExchangeToken, + category: LoginFailureCategory::TokenExchangeRequest, + message: "request failed".to_string(), + }, + ), + ( + "failure_token_endpoint_rejected", + LoginPhase::Failed { + phase: LoginFailurePhase::ExchangeToken, + category: LoginFailureCategory::TokenEndpointRejected, + message: "token endpoint returned status 403 Forbidden: denied".to_string(), + }, + ), + ( + "failure_token_response_malformed", + LoginPhase::Failed { + phase: LoginFailurePhase::ExchangeToken, + category: LoginFailureCategory::TokenResponseMalformed, + message: "expected value at line 1 column 1".to_string(), + }, + ), + ( + "failure_workspace_restriction", + LoginPhase::Failed { + phase: LoginFailurePhase::ValidateCallback, + category: LoginFailureCategory::WorkspaceRestriction, + message: "Login is restricted to workspace id org-required".to_string(), + }, + ), + ( + "failure_persist_failed", + LoginPhase::Failed { + phase: LoginFailurePhase::PersistCredentials, + category: LoginFailureCategory::PersistFailed, + message: "permission denied".to_string(), + }, + ), + ( + "failure_redirect_failed", + LoginPhase::Failed { + phase: LoginFailurePhase::RedirectBackToCodex, + category: LoginFailureCategory::RedirectFailed, + message: "invalid redirect header".to_string(), + }, + ), + ]; + + for (name, phase) in samples { + insta::assert_snapshot!(name, phase.to_string()); + } + } +} diff --git a/codex-rs/login/src/server.rs b/codex-rs/login/src/server.rs index a51e038dc1..871dd15b30 100644 --- a/codex-rs/login/src/server.rs +++ b/codex-rs/login/src/server.rs @@ -22,9 +22,16 @@ use std::path::PathBuf; use std::sync::Arc; use std::thread; use std::time::Duration; +use std::time::Instant; use crate::pkce::PkceCodes; use crate::pkce::generate_pkce; +use crate::progress::LoginFailureCategory; +use crate::progress::LoginFailurePhase; +use crate::progress::LoginPhase; +use crate::progress::LoginProgressSender; +use crate::token_exchange_error::TokenExchangeError; +use crate::token_exchange_error::parse_token_endpoint_error; use base64::Engine; use chrono::Utc; use codex_app_server_protocol::AuthMode; @@ -49,21 +56,76 @@ use tracing::warn; const DEFAULT_ISSUER: &str = "https://auth.openai.com"; const DEFAULT_PORT: u16 = 1455; +/// Runtime observer for structured login progress events. +/// +/// This is intentionally separate from [`ServerOptions`]: options describe how to start a login +/// server, while this value represents an observer attached to one concrete run. If the sender is +/// absent, progress emission becomes a no-op so TUI/app-server callers can keep using the same +/// startup path without manufacturing a channel they never read. +#[derive(Clone, Default)] +struct ProgressEmitter(Option); + +impl ProgressEmitter { + /// Wraps a live progress channel for a single login attempt. + fn new(progress_tx: LoginProgressSender) -> Self { + Self(Some(progress_tx)) + } + + /// Sends one progress phase to the observer, dropping the event if nobody is listening. + /// + /// Progress is best-effort. A closed receiver must not fail the login flow itself, or a UI + /// teardown could turn a successful sign-in into a spurious auth error. + fn emit(&self, phase: LoginPhase) { + if let Some(progress_tx) = &self.0 { + let _ = progress_tx.send(phase); + } + } +} + /// Options for launching the local login callback server. -#[derive(Debug, Clone)] +/// +/// These values are startup configuration only. Per-run observation belongs in +/// [`run_login_server_with_progress`], not here; putting a live channel on this struct makes a +/// reusable configuration object look like part of the running server's state. +#[derive(Clone)] pub struct ServerOptions { + /// Codex home directory used to read and write local auth state. pub codex_home: PathBuf, + /// OAuth client ID to send to the authorization and token endpoints. pub client_id: String, + /// OAuth issuer base URL. + /// + /// The default is the production OpenAI issuer. Tests and advanced flows may override this to + /// point at a local mock or non-default deployment. pub issuer: String, + /// Preferred localhost callback port. + /// + /// Use `0` to request an ephemeral port from the OS. A fixed port is convenient for manual + /// testing, but callers should expect retries if another login server is still shutting down. pub port: u16, + /// Whether to open the system browser after the callback handler is ready. + /// + /// Headless callers should set this to `false` and present the returned [`LoginServer::auth_url`] + /// themselves. Opening the browser before the handler is ready can lose the redirect in locked + /// down environments, so the implementation waits for readiness first. pub open_browser: bool, + /// Optional state override for deterministic tests. + /// + /// Production callers should leave this unset. Reusing a fixed state outside tests weakens the + /// CSRF protection that the callback validation is meant to provide. pub force_state: Option, + /// Optional workspace restriction enforced after ID token validation. pub forced_chatgpt_workspace_id: Option, + /// Credential store mode used when persisting the completed login. pub cli_auth_credentials_store_mode: AuthCredentialsStoreMode, } impl ServerOptions { /// Creates a server configuration with the default issuer and port. + /// + /// The returned options are suitable for a normal interactive browser login. Callers that run + /// in headless mode usually set `open_browser = false` before starting the server; forgetting + /// to do so can spawn a browser on machines where the user cannot complete the flow. pub fn new( codex_home: PathBuf, client_id: String, @@ -84,8 +146,16 @@ impl ServerOptions { } /// Handle for a running login callback server. +/// +/// This handle owns the background task that waits for the OAuth callback and persists +/// credentials. Dropping it does not implicitly cancel the flow; call [`Self::cancel`] or use a +/// [`ShutdownHandle`] if the UI needs to abort an in-flight attempt. pub struct LoginServer { + /// Provider authorization URL for this login attempt. pub auth_url: String, + /// Actual localhost port bound by the callback server. + /// + /// This can differ from [`ServerOptions::port`] when the caller requested port `0`. pub actual_port: u16, server_handle: tokio::task::JoinHandle>, shutdown_handle: ShutdownHandle, @@ -93,6 +163,11 @@ pub struct LoginServer { impl LoginServer { /// Waits for the login callback loop to finish. + /// + /// Returns `Ok(())` only after credentials were persisted and the browser was redirected to + /// the local success page. Startup failures that happen before the handle is returned are + /// reported directly by [`run_login_server`] instead, so callers should not expect this method + /// to cover bind or browser-launch errors. pub async fn block_until_done(self) -> io::Result<()> { self.server_handle .await @@ -100,17 +175,27 @@ impl LoginServer { } /// Requests shutdown of the callback server. + /// + /// This is a local cancellation signal, not a provider-side OAuth revocation. If the user has + /// already completed the browser flow, cancelling here only stops the localhost server loop. pub fn cancel(&self) { self.shutdown_handle.shutdown(); } /// Returns a cloneable cancel handle for the running server. + /// + /// Use this when another task owns the UI lifetime and needs to cancel the login without + /// taking ownership of the whole [`LoginServer`]. pub fn cancel_handle(&self) -> ShutdownHandle { self.shutdown_handle.clone() } } /// Handle used to signal the login server loop to exit. +/// +/// Clones all target the same [`tokio::sync::Notify`], so any clone can cancel the active login +/// attempt. This type is intentionally narrow: it can stop the local server loop, but it does not +/// expose progress or completion state. #[derive(Clone, Debug)] pub struct ShutdownHandle { shutdown_notify: Arc, @@ -118,26 +203,60 @@ pub struct ShutdownHandle { impl ShutdownHandle { /// Signals the login loop to terminate. + /// + /// Shutdown is asynchronous. The caller should still await [`LoginServer::block_until_done`] + /// if it needs to observe the final completion error. pub fn shutdown(&self) { self.shutdown_notify.notify_waiters(); } } /// Starts a local callback server and returns the browser auth URL. +/// +/// Use this when the caller only needs the URL, cancellation, and final completion result. If the +/// UI needs live phase updates for stderr or another progress surface, use +/// [`run_login_server_with_progress`] instead; otherwise the login will still work, but the caller +/// will have no way to tell whether it is binding, waiting for the browser, or exchanging tokens. pub fn run_login_server(opts: ServerOptions) -> io::Result { + run_login_server_impl(opts, ProgressEmitter::default()) +} + +/// Starts a local callback server and publishes structured progress events to `progress_tx`. +/// +/// The returned server handle and the progress stream describe the same login attempt. The caller +/// must drain the receiver side of `progress_tx` if it cares about those events; creating a sender +/// and then ignoring the receiver makes the API look observed while silently discarding the +/// milestones this function exists to expose. +pub fn run_login_server_with_progress( + opts: ServerOptions, + progress_tx: LoginProgressSender, +) -> io::Result { + run_login_server_impl(opts, ProgressEmitter::new(progress_tx)) +} + +fn run_login_server_impl( + opts: ServerOptions, + progress: ProgressEmitter, +) -> io::Result { let pkce = generate_pkce(); let state = opts.force_state.clone().unwrap_or_else(generate_state); - let server = bind_server(opts.port)?; + let server = bind_server(opts.port, &progress)?; let actual_port = match server.server_addr().to_ip() { Some(addr) => addr.port(), None => { + progress.emit(LoginPhase::Failed { + phase: LoginFailurePhase::BindLocalServer, + category: LoginFailureCategory::LocalServerUnavailable, + message: "Unable to determine the server port".to_string(), + }); return Err(io::Error::new( io::ErrorKind::AddrInUse, "Unable to determine the server port", )); } }; + progress.emit(LoginPhase::LocalServerBound { port: actual_port }); let server = Arc::new(server); let redirect_uri = format!("http://localhost:{actual_port}/auth/callback"); @@ -150,10 +269,6 @@ pub fn run_login_server(opts: ServerOptions) -> io::Result { opts.forced_chatgpt_workspace_id.as_deref(), ); - if opts.open_browser { - let _ = webbrowser::open(&auth_url); - } - // Map blocking reads from server.recv() to an async channel. let (tx, mut rx) = tokio::sync::mpsc::channel::(16); let _server_handle = { @@ -173,23 +288,53 @@ pub fn run_login_server(opts: ServerOptions) -> io::Result { }; let shutdown_notify = Arc::new(tokio::sync::Notify::new()); + let (ready_tx, ready_rx) = std::sync::mpsc::channel::<()>(); let server_handle = { + let opts = opts.clone(); + let progress = progress.clone(); let shutdown_notify = shutdown_notify.clone(); let server = server; tokio::spawn(async move { + let _ = ready_tx.send(()); + progress.emit(LoginPhase::WaitingForCallback); + let result = loop { tokio::select! { _ = shutdown_notify.notified() => { - break Err(io::Error::other("Login was not completed")); + let message = "Login was not completed".to_string(); + progress.emit( + LoginPhase::Failed { + phase: LoginFailurePhase::WaitForCallback, + category: LoginFailureCategory::LoginCancelled, + message: message.clone(), + }, + ); + break Err(io::Error::other(message)); } maybe_req = rx.recv() => { let Some(req) = maybe_req else { - break Err(io::Error::other("Login was not completed")); + let message = "Login was not completed".to_string(); + progress.emit( + LoginPhase::Failed { + phase: LoginFailurePhase::WaitForCallback, + category: LoginFailureCategory::LoginCancelled, + message: message.clone(), + }, + ); + break Err(io::Error::other(message)); }; let url_raw = req.url().to_string(); - let response = - process_request(&url_raw, &opts, &redirect_uri, &pkce, actual_port, &state).await; + let response = process_request( + &url_raw, + &opts, + &progress, + &redirect_uri, + &pkce, + actual_port, + &state, + ) + .await; let exit_result = match response { HandledRequest::Response(response) => { @@ -228,6 +373,27 @@ pub fn run_login_server(opts: ServerOptions) -> io::Result { }) }; + if opts.open_browser { + if ready_rx.recv_timeout(Duration::from_secs(2)).is_err() { + progress.emit(LoginPhase::Failed { + phase: LoginFailurePhase::BindLocalServer, + category: LoginFailureCategory::LocalServerUnavailable, + message: "Timed out waiting for callback handler readiness".to_string(), + }); + return Err(io::Error::other( + "Timed out waiting for callback handler readiness", + )); + } + progress.emit(LoginPhase::OpeningBrowser); + if let Err(err) = webbrowser::open(&auth_url) { + progress.emit(LoginPhase::Failed { + phase: LoginFailurePhase::OpenBrowser, + category: LoginFailureCategory::BrowserLaunchFailed, + message: err.to_string(), + }); + } + } + Ok(LoginServer { auth_url, actual_port, @@ -250,6 +416,7 @@ enum HandledRequest { async fn process_request( url_raw: &str, opts: &ServerOptions, + progress: &ProgressEmitter, redirect_uri: &str, pkce: &PkceCodes, actual_port: u16, @@ -274,6 +441,12 @@ async fn process_request( 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 state_valid = params.get("state").map(String::as_str) == Some(state); + progress.emit(LoginPhase::CallbackReceived { + has_code, + has_state, + has_error, + state_valid, + }); info!( path = %path, has_code, @@ -290,6 +463,11 @@ async fn process_request( has_error, "login callback state mismatch" ); + progress.emit(LoginPhase::Failed { + phase: LoginFailurePhase::ValidateCallback, + category: LoginFailureCategory::CallbackStateMismatch, + message: "State mismatch".to_string(), + }); return HandledRequest::Response( Response::from_string("State mismatch").with_status_code(400), ); @@ -297,12 +475,16 @@ async fn process_request( 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" ); + progress.emit(LoginPhase::Failed { + phase: LoginFailurePhase::ValidateCallback, + category: LoginFailureCategory::ProviderCallbackError, + message: message.clone(), + }); return login_error_response( &message, io::ErrorKind::PermissionDenied, @@ -313,8 +495,15 @@ async fn process_request( let code = match params.get("code") { Some(c) if !c.is_empty() => c.clone(), _ => { + let message = + "Missing authorization code. Sign-in could not be completed.".to_string(); + progress.emit(LoginPhase::Failed { + phase: LoginFailurePhase::ValidateCallback, + category: LoginFailureCategory::MissingAuthorizationCode, + message: message.clone(), + }); return login_error_response( - "Missing authorization code. Sign-in could not be completed.", + &message, io::ErrorKind::InvalidData, Some("missing_authorization_code"), /*error_description*/ None, @@ -322,6 +511,7 @@ async fn process_request( } }; + progress.emit(LoginPhase::ExchangingToken); match exchange_code_for_tokens(&opts.issuer, &opts.client_id, redirect_uri, pkce, &code) .await { @@ -330,7 +520,12 @@ async fn process_request( opts.forced_chatgpt_workspace_id.as_deref(), &tokens.id_token, ) { - eprintln!("Workspace restriction error: {message}"); + warn!(%message, "workspace restriction blocked login"); + progress.emit(LoginPhase::Failed { + phase: LoginFailurePhase::ValidateCallback, + category: LoginFailureCategory::WorkspaceRestriction, + message: message.clone(), + }); return login_error_response( &message, io::ErrorKind::PermissionDenied, @@ -342,6 +537,7 @@ async fn process_request( let api_key = obtain_api_key(&opts.issuer, &opts.client_id, &tokens.id_token) .await .ok(); + progress.emit(LoginPhase::PersistingCredentials); if let Err(err) = persist_tokens_async( &opts.codex_home, api_key.clone(), @@ -352,7 +548,12 @@ async fn process_request( ) .await { - eprintln!("Persist error: {err}"); + error!(%err, "failed to persist login credentials"); + progress.emit(LoginPhase::Failed { + phase: LoginFailurePhase::PersistCredentials, + category: LoginFailureCategory::PersistFailed, + message: err.to_string(), + }); return login_error_response( "Sign-in completed but credentials could not be saved locally.", io::ErrorKind::Other, @@ -368,18 +569,34 @@ async fn process_request( &tokens.access_token, ); match tiny_http::Header::from_bytes(&b"Location"[..], success_url.as_bytes()) { - Ok(header) => HandledRequest::RedirectWithHeader(header), - Err(_) => login_error_response( - "Sign-in completed but redirecting back to Codex failed.", - io::ErrorKind::Other, - Some("redirect_failed"), - /*error_description*/ None, - ), + Ok(header) => { + progress.emit(LoginPhase::Succeeded); + HandledRequest::RedirectWithHeader(header) + } + Err(_) => { + let message = "Sign-in completed but redirecting back to Codex failed." + .to_string(); + progress.emit(LoginPhase::Failed { + phase: LoginFailurePhase::RedirectBackToCodex, + category: LoginFailureCategory::RedirectFailed, + message: message.clone(), + }); + login_error_response( + &message, + io::ErrorKind::Other, + Some("redirect_failed"), + /*error_description*/ None, + ) + } } } Err(err) => { - eprintln!("Token exchange error: {err}"); error!("login callback token exchange failed"); + progress.emit(LoginPhase::Failed { + phase: LoginFailurePhase::ExchangeToken, + category: err.failure_category(), + message: err.to_string(), + }); login_error_response( &format!("Token exchange failed: {err}"), io::ErrorKind::Other, @@ -523,14 +740,18 @@ fn send_cancel_request(port: u16) -> io::Result<()> { Ok(()) } -fn bind_server(port: u16) -> io::Result { +fn bind_server(port: u16, progress: &ProgressEmitter) -> io::Result { let bind_address = format!("127.0.0.1:{port}"); let mut cancel_attempted = false; let mut attempts = 0; const MAX_ATTEMPTS: u32 = 10; const RETRY_DELAY: Duration = Duration::from_millis(200); + let started_at = Instant::now(); loop { + progress.emit(LoginPhase::BindingLocalServer { + attempt: attempts + 1, + }); match Server::http(&bind_address) { Ok(server) => return Ok(server), Err(err) => { @@ -545,6 +766,8 @@ fn bind_server(port: u16) -> io::Result { if is_addr_in_use { if !cancel_attempted { cancel_attempted = true; + progress + .emit(LoginPhase::PreviousLoginServerDetected { attempt: attempts }); if let Err(cancel_err) = send_cancel_request(port) { eprintln!("Failed to cancel previous login server: {cancel_err}"); } @@ -553,16 +776,28 @@ fn bind_server(port: u16) -> io::Result { thread::sleep(RETRY_DELAY); if attempts >= MAX_ATTEMPTS { - return Err(io::Error::new( - io::ErrorKind::AddrInUse, - format!("Port {bind_address} is already in use"), - )); + let message = format!( + "Port {bind_address} is already in use after {} ms", + started_at.elapsed().as_millis() + ); + progress.emit(LoginPhase::Failed { + phase: LoginFailurePhase::BindLocalServer, + category: LoginFailureCategory::LocalServerUnavailable, + message: message.clone(), + }); + return Err(io::Error::new(io::ErrorKind::AddrInUse, message)); } continue; } - return Err(io::Error::other(err)); + let message = err.to_string(); + progress.emit(LoginPhase::Failed { + phase: LoginFailurePhase::BindLocalServer, + category: LoginFailureCategory::LocalServerUnavailable, + message: message.clone(), + }); + return Err(io::Error::other(message)); } } } @@ -570,24 +805,14 @@ fn bind_server(port: u16) -> io::Result { /// Tokens returned by the OAuth authorization-code exchange. pub(crate) struct ExchangedTokens { + /// OIDC ID token returned by the token endpoint. pub id_token: String, + /// OAuth access token returned by the token endpoint. pub access_token: String, + /// OAuth refresh token returned by the token endpoint. 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", @@ -651,14 +876,6 @@ fn redact_sensitive_url_parts(url: &mut url::Url) { url.set_query(Some(&redacted_query)); } -/// Redacts any URL attached to a reqwest transport error before it is logged or returned. -fn redact_sensitive_error_url(mut err: reqwest::Error) -> reqwest::Error { - 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 @@ -678,13 +895,17 @@ fn sanitize_url_for_logging(url: &str) -> String { /// non-JSON error text is preserved there. Structured logging stays narrower: it logs reviewed /// fields from parsed token responses and redacted transport errors, but does not log the final /// callback-layer `%err` string. +/// +/// The `code` argument must be the short-lived authorization code from the current callback. A +/// stale code from an old browser tab will usually fail with a rejected token endpoint response, +/// which is expected and should be surfaced as an auth failure rather than retried blindly. pub(crate) async fn exchange_code_for_tokens( issuer: &str, client_id: &str, redirect_uri: &str, pkce: &PkceCodes, code: &str, -) -> io::Result { +) -> Result { #[derive(serde::Deserialize)] struct TokenResponse { id_token: String, @@ -692,7 +913,11 @@ pub(crate) async fn exchange_code_for_tokens( refresh_token: String, } - let client = build_reqwest_client_with_custom_ca(reqwest::Client::builder())?; + let endpoint = sanitize_url_for_logging(&format!("{issuer}/oauth/token")); + let client = + build_reqwest_client_with_custom_ca(reqwest::Client::builder()).map_err(|error| { + TokenExchangeError::http_client_setup(endpoint.clone(), error.to_string()) + })?; info!( issuer = %sanitize_url_for_logging(issuer), redirect_uri = %redirect_uri, @@ -713,21 +938,24 @@ pub(crate) async fn exchange_code_for_tokens( let resp = match resp { Ok(resp) => resp, Err(error) => { - let error = redact_sensitive_error_url(error); + let error = TokenExchangeError::transport(endpoint.clone(), error); error!( - is_timeout = error.is_timeout(), - is_connect = error.is_connect(), - is_request = error.is_request(), + is_timeout = error.as_transport_error().is_some_and(reqwest::Error::is_timeout), + is_connect = error.as_transport_error().is_some_and(reqwest::Error::is_connect), + is_request = error.as_transport_error().is_some_and(reqwest::Error::is_request), error = %error, "oauth token exchange transport failure" ); - return Err(io::Error::other(error)); + return Err(error); } }; let status = resp.status(); if !status.is_success() { - let body = resp.text().await.map_err(io::Error::other)?; + let body = resp + .text() + .await + .map_err(|error| TokenExchangeError::transport(endpoint.clone(), error))?; let detail = parse_token_endpoint_error(&body); warn!( %status, @@ -735,12 +963,15 @@ pub(crate) async fn exchange_code_for_tokens( 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}" - ))); + return Err(TokenExchangeError::endpoint_rejected( + endpoint, status, detail, + )); } - let tokens: TokenResponse = resp.json().await.map_err(io::Error::other)?; + let tokens: TokenResponse = resp + .json() + .await + .map_err(|error| TokenExchangeError::response_malformed(endpoint.clone(), error))?; info!(%status, "oauth token exchange succeeded"); Ok(ExchangedTokens { id_token: tokens.id_token, @@ -750,6 +981,11 @@ pub(crate) async fn exchange_code_for_tokens( } /// Persists exchanged credentials using the configured local auth store. +/// +/// This runs the existing synchronous save path on a blocking thread so the async callback task +/// does not stall the runtime. Callers should pass the raw token strings from the current exchange +/// only; persisting mismatched ID/access/refresh tokens can leave Codex with a locally valid file +/// that fails on the next authenticated request. pub(crate) async fn persist_tokens_async( codex_home: &Path, api_key: Option, @@ -865,6 +1101,10 @@ fn jwt_auth_claims(jwt: &str) -> serde_json::Map { } /// Validates the ID token against an optional workspace restriction. +/// +/// Returns `Ok(())` when no restriction is configured. When a restriction is present, the token +/// must carry a `chatgpt_account_id` claim matching that workspace; otherwise the browser flow +/// should fail before any credentials are written. pub(crate) fn ensure_workspace_allowed( expected: Option<&str>, id_token: &str, @@ -929,75 +1169,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, @@ -1051,6 +1222,11 @@ fn html_escape(input: &str) -> String { } /// Exchanges an authenticated ID token for an API-key style access token. +/// +/// This is a compatibility step for the existing login persistence path, not the primary OAuth +/// exchange. A failure here is non-fatal to the main browser login flow today because Codex can +/// still persist the OIDC tokens; callers that treat this as mandatory would regress successful +/// sign-ins on deployments where the legacy exchange is unavailable. pub(crate) async fn obtain_api_key( issuer: &str, client_id: &str, @@ -1089,72 +1265,10 @@ pub(crate) async fn obtain_api_key( mod tests { use pretty_assertions::assert_eq; - use super::TokenEndpointErrorDetail; - use super::parse_token_endpoint_error; use super::redact_sensitive_query_value; use super::redact_sensitive_url_parts; 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!( diff --git a/codex-rs/login/src/snapshots/codex_login__progress__tests__failure_browser_launch_failed.snap b/codex-rs/login/src/snapshots/codex_login__progress__tests__failure_browser_launch_failed.snap new file mode 100644 index 0000000000..76d14729cd --- /dev/null +++ b/codex-rs/login/src/snapshots/codex_login__progress__tests__failure_browser_launch_failed.snap @@ -0,0 +1,8 @@ +--- +source: login/src/progress.rs +expression: "format_login_progress(phase).expect(\"sample should render\")" +--- +Couldn't open your browser +Codex couldn't finish signing in while opening browser. Use the sign-in link printed above, or run `codex login --device-auth` on a remote machine. +Help: https://developers.openai.com/codex/auth +Details: browser_launch_failed - No browser found diff --git a/codex-rs/login/src/snapshots/codex_login__progress__tests__failure_callback_state_mismatch.snap b/codex-rs/login/src/snapshots/codex_login__progress__tests__failure_callback_state_mismatch.snap new file mode 100644 index 0000000000..2559902a57 --- /dev/null +++ b/codex-rs/login/src/snapshots/codex_login__progress__tests__failure_callback_state_mismatch.snap @@ -0,0 +1,8 @@ +--- +source: login/src/progress.rs +expression: "format_login_progress(phase).expect(\"sample should render\")" +--- +Couldn't verify this sign-in attempt +Codex couldn't finish signing in while validating OAuth callback. Retry sign-in from the same terminal, and avoid reusing an old browser tab. +Help: https://developers.openai.com/codex/auth +Details: callback_state_mismatch - State mismatch diff --git a/codex-rs/login/src/snapshots/codex_login__progress__tests__failure_local_server_unavailable.snap b/codex-rs/login/src/snapshots/codex_login__progress__tests__failure_local_server_unavailable.snap new file mode 100644 index 0000000000..553ad269c3 --- /dev/null +++ b/codex-rs/login/src/snapshots/codex_login__progress__tests__failure_local_server_unavailable.snap @@ -0,0 +1,8 @@ +--- +source: login/src/progress.rs +expression: "format_login_progress(phase).expect(\"sample should render\")" +--- +Couldn't start local sign-in +Codex couldn't finish signing in while binding local callback server. Retry in a moment. If it keeps happening, another process may be holding the local callback port. +Help: https://developers.openai.com/codex/auth +Details: local_server_unavailable - Port 127.0.0.1:1455 is already in use after 2000 ms diff --git a/codex-rs/login/src/snapshots/codex_login__progress__tests__failure_login_cancelled.snap b/codex-rs/login/src/snapshots/codex_login__progress__tests__failure_login_cancelled.snap new file mode 100644 index 0000000000..59606d68b2 --- /dev/null +++ b/codex-rs/login/src/snapshots/codex_login__progress__tests__failure_login_cancelled.snap @@ -0,0 +1,8 @@ +--- +source: login/src/progress.rs +expression: "format_login_progress(phase).expect(\"sample should render\")" +--- +Sign-in was cancelled +Codex couldn't finish signing in while waiting for OAuth callback. Run `codex login` to try again. +Help: https://developers.openai.com/codex/auth +Details: login_cancelled - Login was not completed diff --git a/codex-rs/login/src/snapshots/codex_login__progress__tests__failure_missing_authorization_code.snap b/codex-rs/login/src/snapshots/codex_login__progress__tests__failure_missing_authorization_code.snap new file mode 100644 index 0000000000..15cfa7c3a6 --- /dev/null +++ b/codex-rs/login/src/snapshots/codex_login__progress__tests__failure_missing_authorization_code.snap @@ -0,0 +1,8 @@ +--- +source: login/src/progress.rs +expression: "format_login_progress(phase).expect(\"sample should render\")" +--- +Didn't get a sign-in code back +Codex couldn't finish signing in while validating OAuth callback. Try again. If it keeps happening, restart the login flow from Codex. +Help: https://developers.openai.com/codex/auth +Details: missing_authorization_code - Missing authorization code. Sign-in could not be completed. diff --git a/codex-rs/login/src/snapshots/codex_login__progress__tests__failure_persist_failed.snap b/codex-rs/login/src/snapshots/codex_login__progress__tests__failure_persist_failed.snap new file mode 100644 index 0000000000..890c32273a --- /dev/null +++ b/codex-rs/login/src/snapshots/codex_login__progress__tests__failure_persist_failed.snap @@ -0,0 +1,8 @@ +--- +source: login/src/progress.rs +expression: "format_login_progress(phase).expect(\"sample should render\")" +--- +Couldn't save your Codex credentials +Codex couldn't finish signing in while saving credentials locally. Check permissions for your Codex home directory, then try again. +Help: https://developers.openai.com/codex/auth +Details: persist_failed - permission denied diff --git a/codex-rs/login/src/snapshots/codex_login__progress__tests__failure_provider_callback_error.snap b/codex-rs/login/src/snapshots/codex_login__progress__tests__failure_provider_callback_error.snap new file mode 100644 index 0000000000..1aa08f6728 --- /dev/null +++ b/codex-rs/login/src/snapshots/codex_login__progress__tests__failure_provider_callback_error.snap @@ -0,0 +1,8 @@ +--- +source: login/src/progress.rs +expression: "format_login_progress(phase).expect(\"sample should render\")" +--- +The sign-in page reported a problem +Codex couldn't finish signing in while validating OAuth callback. Try again, switch accounts, or contact your workspace admin if access is restricted. +Help: https://developers.openai.com/codex/auth +Details: provider_callback_error - Sign-in failed: access_denied diff --git a/codex-rs/login/src/snapshots/codex_login__progress__tests__failure_redirect_failed.snap b/codex-rs/login/src/snapshots/codex_login__progress__tests__failure_redirect_failed.snap new file mode 100644 index 0000000000..d867ac0fd7 --- /dev/null +++ b/codex-rs/login/src/snapshots/codex_login__progress__tests__failure_redirect_failed.snap @@ -0,0 +1,8 @@ +--- +source: login/src/progress.rs +expression: "format_login_progress(phase).expect(\"sample should render\")" +--- +Couldn't return to Codex after sign-in +Codex couldn't finish signing in while redirecting back to Codex. Return to Codex and retry sign-in. +Help: https://developers.openai.com/codex/auth +Details: redirect_failed - invalid redirect header diff --git a/codex-rs/login/src/snapshots/codex_login__progress__tests__failure_token_endpoint_rejected.snap b/codex-rs/login/src/snapshots/codex_login__progress__tests__failure_token_endpoint_rejected.snap new file mode 100644 index 0000000000..7ee2c8437a --- /dev/null +++ b/codex-rs/login/src/snapshots/codex_login__progress__tests__failure_token_endpoint_rejected.snap @@ -0,0 +1,8 @@ +--- +source: login/src/progress.rs +expression: "format_login_progress(phase).expect(\"sample should render\")" +--- +The auth server rejected the sign-in +Codex couldn't finish signing in while exchanging authorization code for tokens. Try again. If this repeats, your account or workspace may not be allowed to use Codex. +Help: https://developers.openai.com/codex/auth +Details: token_endpoint_rejected - token endpoint returned status 403 Forbidden: denied diff --git a/codex-rs/login/src/snapshots/codex_login__progress__tests__failure_token_exchange_connect.snap b/codex-rs/login/src/snapshots/codex_login__progress__tests__failure_token_exchange_connect.snap new file mode 100644 index 0000000000..ce2417f311 --- /dev/null +++ b/codex-rs/login/src/snapshots/codex_login__progress__tests__failure_token_exchange_connect.snap @@ -0,0 +1,8 @@ +--- +source: login/src/progress.rs +expression: "format_login_progress(phase).expect(\"sample should render\")" +--- +Couldn't reach the auth server +Codex couldn't finish signing in while exchanging authorization code for tokens. Check your network, proxy, or custom CA setup, then try again. +Help: https://developers.openai.com/codex/auth +Details: token_exchange_connect - error sending request (endpoint: https://auth.openai.com/oauth/token) diff --git a/codex-rs/login/src/snapshots/codex_login__progress__tests__failure_token_exchange_request.snap b/codex-rs/login/src/snapshots/codex_login__progress__tests__failure_token_exchange_request.snap new file mode 100644 index 0000000000..0b9692f9db --- /dev/null +++ b/codex-rs/login/src/snapshots/codex_login__progress__tests__failure_token_exchange_request.snap @@ -0,0 +1,8 @@ +--- +source: login/src/progress.rs +expression: "format_login_progress(phase).expect(\"sample should render\")" +--- +Couldn't reach the auth server +Codex couldn't finish signing in while exchanging authorization code for tokens. Check your network, proxy, or custom CA setup, then try again. +Help: https://developers.openai.com/codex/auth +Details: token_exchange_request - request failed diff --git a/codex-rs/login/src/snapshots/codex_login__progress__tests__failure_token_exchange_timeout.snap b/codex-rs/login/src/snapshots/codex_login__progress__tests__failure_token_exchange_timeout.snap new file mode 100644 index 0000000000..28c113f7f7 --- /dev/null +++ b/codex-rs/login/src/snapshots/codex_login__progress__tests__failure_token_exchange_timeout.snap @@ -0,0 +1,8 @@ +--- +source: login/src/progress.rs +expression: "format_login_progress(phase).expect(\"sample should render\")" +--- +The auth server took too long to respond +Codex couldn't finish signing in while exchanging authorization code for tokens. Check your network connection or proxy, then try again. +Help: https://developers.openai.com/codex/auth +Details: token_exchange_timeout - operation timed out diff --git a/codex-rs/login/src/snapshots/codex_login__progress__tests__failure_token_response_malformed.snap b/codex-rs/login/src/snapshots/codex_login__progress__tests__failure_token_response_malformed.snap new file mode 100644 index 0000000000..072c313f00 --- /dev/null +++ b/codex-rs/login/src/snapshots/codex_login__progress__tests__failure_token_response_malformed.snap @@ -0,0 +1,8 @@ +--- +source: login/src/progress.rs +expression: "format_login_progress(phase).expect(\"sample should render\")" +--- +The auth server sent an unexpected response +Codex couldn't finish signing in while exchanging authorization code for tokens. Try again. If this repeats, contact support with the details below. +Help: https://developers.openai.com/codex/auth +Details: token_response_malformed - expected value at line 1 column 1 diff --git a/codex-rs/login/src/snapshots/codex_login__progress__tests__failure_workspace_restriction.snap b/codex-rs/login/src/snapshots/codex_login__progress__tests__failure_workspace_restriction.snap new file mode 100644 index 0000000000..d3b6b57def --- /dev/null +++ b/codex-rs/login/src/snapshots/codex_login__progress__tests__failure_workspace_restriction.snap @@ -0,0 +1,8 @@ +--- +source: login/src/progress.rs +expression: "format_login_progress(phase).expect(\"sample should render\")" +--- +This account can't be used here +Codex couldn't finish signing in while validating OAuth callback. Switch to an allowed account or contact your workspace admin. +Help: https://developers.openai.com/codex/auth +Details: workspace_restriction - Login is restricted to workspace id org-required diff --git a/codex-rs/login/src/snapshots/codex_login__progress__tests__progress_callback_received.snap b/codex-rs/login/src/snapshots/codex_login__progress__tests__progress_callback_received.snap new file mode 100644 index 0000000000..a135cddc9a --- /dev/null +++ b/codex-rs/login/src/snapshots/codex_login__progress__tests__progress_callback_received.snap @@ -0,0 +1,5 @@ +--- +source: login/src/progress.rs +expression: "format_login_progress(phase).expect(\"sample should render\")" +--- +Browser sign-in received. Finishing up... diff --git a/codex-rs/login/src/snapshots/codex_login__progress__tests__progress_cleanup_old_login_session.snap b/codex-rs/login/src/snapshots/codex_login__progress__tests__progress_cleanup_old_login_session.snap new file mode 100644 index 0000000000..a998fba426 --- /dev/null +++ b/codex-rs/login/src/snapshots/codex_login__progress__tests__progress_cleanup_old_login_session.snap @@ -0,0 +1,5 @@ +--- +source: login/src/progress.rs +expression: "format_login_progress(phase).expect(\"sample should render\")" +--- +Cleaning up an old login session first... diff --git a/codex-rs/login/src/snapshots/codex_login__progress__tests__progress_opening_browser.snap b/codex-rs/login/src/snapshots/codex_login__progress__tests__progress_opening_browser.snap new file mode 100644 index 0000000000..45f7a910c8 --- /dev/null +++ b/codex-rs/login/src/snapshots/codex_login__progress__tests__progress_opening_browser.snap @@ -0,0 +1,5 @@ +--- +source: login/src/progress.rs +expression: "format_login_progress(phase).expect(\"sample should render\")" +--- +Opening your browser to sign in... diff --git a/codex-rs/login/src/snapshots/codex_login__progress__tests__progress_retry_local_listener.snap b/codex-rs/login/src/snapshots/codex_login__progress__tests__progress_retry_local_listener.snap new file mode 100644 index 0000000000..bb890ced4f --- /dev/null +++ b/codex-rs/login/src/snapshots/codex_login__progress__tests__progress_retry_local_listener.snap @@ -0,0 +1,5 @@ +--- +source: login/src/progress.rs +expression: "format_login_progress(phase).expect(\"sample should render\")" +--- +Retrying the local sign-in listener... (attempt 2) diff --git a/codex-rs/login/src/snapshots/codex_login__progress__tests__progress_saving_credentials.snap b/codex-rs/login/src/snapshots/codex_login__progress__tests__progress_saving_credentials.snap new file mode 100644 index 0000000000..968ecfdd3d --- /dev/null +++ b/codex-rs/login/src/snapshots/codex_login__progress__tests__progress_saving_credentials.snap @@ -0,0 +1,5 @@ +--- +source: login/src/progress.rs +expression: "format_login_progress(phase).expect(\"sample should render\")" +--- +Saving your Codex credentials... diff --git a/codex-rs/login/src/token_exchange_error.rs b/codex-rs/login/src/token_exchange_error.rs new file mode 100644 index 0000000000..f3f8c61f24 --- /dev/null +++ b/codex-rs/login/src/token_exchange_error.rs @@ -0,0 +1,296 @@ +//! Private error types for OAuth token exchange failures. +//! +//! This module keeps transport/provider failure modeling separate from the localhost callback +//! orchestration in `server.rs`. The main goal is to preserve useful reqwest transport detail for +//! users and support while stripping the attached URL field that may carry sensitive OAuth data. + +use crate::progress::LoginFailureCategory; +use serde_json::Value as JsonValue; + +/// Failure from exchanging an OAuth authorization code for tokens. +/// +/// This type separates protocol failures from transport failures so the caller can choose a stable +/// user-facing bucket without throwing away the underlying detail. Variants that wrap +/// [`reqwest::Error`] are constructed through helpers that strip the attached URL first; callers +/// should not build those variants directly unless they preserve that redaction step. +#[derive(Debug, thiserror::Error)] +pub(crate) enum TokenExchangeError { + /// The HTTP client could not be constructed for the token request. + #[error("failed to prepare HTTP client for {endpoint}: {message}")] + HttpClientSetup { + /// Sanitized token endpoint identity for the error message. + endpoint: String, + /// Human-readable setup failure. + message: String, + }, + + /// The token request failed before a usable HTTP response was received. + #[error("{source} (endpoint: {endpoint})")] + Transport { + /// Sanitized token endpoint identity for the error message. + endpoint: String, + /// Transport error with the attached URL removed. + #[source] + source: reqwest::Error, + }, + + /// The token endpoint returned a non-success HTTP status. + #[error("token endpoint returned status {status} from {endpoint}: {detail}")] + EndpointRejected { + /// Sanitized token endpoint identity for the error message. + endpoint: String, + /// HTTP status returned by the token endpoint. + status: reqwest::StatusCode, + /// Parsed error detail from the response body. + detail: TokenEndpointErrorDetail, + }, + + /// The token endpoint returned success, but the body was not the expected token payload. + #[error("token response from {endpoint} could not be parsed: {source}")] + ResponseMalformed { + /// Sanitized token endpoint identity for the error message. + endpoint: String, + /// Body parse error with the attached URL removed. + #[source] + source: reqwest::Error, + }, +} + +impl TokenExchangeError { + /// Builds a setup failure for the token endpoint HTTP client. + pub(crate) fn http_client_setup(endpoint: String, message: String) -> Self { + Self::HttpClientSetup { endpoint, message } + } + + /// Builds a transport failure and strips the URL from the wrapped reqwest error. + /// + /// This preserves lower-level proxy/TLS/connect detail while keeping the potentially sensitive + /// URL out of the source error's `Display` output. + pub(crate) fn transport(endpoint: String, source: reqwest::Error) -> Self { + Self::Transport { + endpoint, + source: source.without_url(), + } + } + + /// Builds a rejected-endpoint failure from a non-success HTTP response. + pub(crate) fn endpoint_rejected( + endpoint: String, + status: reqwest::StatusCode, + detail: TokenEndpointErrorDetail, + ) -> Self { + Self::EndpointRejected { + endpoint, + status, + detail, + } + } + + /// Builds a malformed-response failure and strips the URL from the wrapped reqwest error. + pub(crate) fn response_malformed(endpoint: String, source: reqwest::Error) -> Self { + Self::ResponseMalformed { + endpoint, + source: source.without_url(), + } + } + + /// Maps this error to the coarse support/UI category used by progress events. + /// + /// This intentionally collapses many low-level transport causes into a small stable enum. + /// Callers that need exact diagnostics should inspect the source chain or logs instead of + /// adding more categories for every transient network failure. + pub(crate) fn failure_category(&self) -> LoginFailureCategory { + match self { + TokenExchangeError::HttpClientSetup { .. } => { + LoginFailureCategory::TokenExchangeRequest + } + TokenExchangeError::Transport { source, .. } => { + if source.is_timeout() { + LoginFailureCategory::TokenExchangeTimeout + } else if source.is_connect() { + LoginFailureCategory::TokenExchangeConnect + } else { + LoginFailureCategory::TokenExchangeRequest + } + } + TokenExchangeError::EndpointRejected { .. } => { + LoginFailureCategory::TokenEndpointRejected + } + TokenExchangeError::ResponseMalformed { .. } => { + LoginFailureCategory::TokenResponseMalformed + } + } + } + + /// Returns the wrapped reqwest error when one exists. + /// + /// This is only for structured logging and transport classification. A caller that formats the + /// returned source directly into normal logs should remember that only the URL was stripped; + /// other low-level error text is intentionally preserved. + pub(crate) fn as_transport_error(&self) -> Option<&reqwest::Error> { + match self { + TokenExchangeError::Transport { source, .. } + | TokenExchangeError::ResponseMalformed { source, .. } => Some(source), + TokenExchangeError::HttpClientSetup { .. } + | TokenExchangeError::EndpointRejected { .. } => None, + } + } +} + +/// Parsed token endpoint error detail for structured logs and caller-visible error text. +/// +/// The parsed fields are the reviewed, structured subset that can be logged directly. The +/// `Display` form may preserve a raw non-JSON body for the returned error path, so callers should +/// avoid logging this type with `{}` unless they intend to surface backend text to a human. +#[derive(Debug, Clone, PartialEq, Eq)] +pub(crate) struct TokenEndpointErrorDetail { + /// Parsed OAuth error code, when present. + pub(crate) error_code: Option, + /// Parsed provider message, when present. + pub(crate) error_message: Option, + /// Best-effort text for the caller-visible error message. + 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) + } +} + +/// 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. +pub(crate) 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(), + } +} + +#[cfg(test)] +mod tests { + use pretty_assertions::assert_eq; + + use super::TokenEndpointErrorDetail; + use super::parse_token_endpoint_error; + + #[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(), + } + ); + } +}