diff --git a/codex-rs/app-server/src/request_processors/account_processor/enterprise_login.rs b/codex-rs/app-server/src/request_processors/account_processor/enterprise_login.rs index 18a7621d70..f1018bb7c2 100644 --- a/codex-rs/app-server/src/request_processors/account_processor/enterprise_login.rs +++ b/codex-rs/app-server/src/request_processors/account_processor/enterprise_login.rs @@ -3,9 +3,11 @@ //! The credential store separately fences logins invalidated by another process. use super::*; +use anyhow::Context; use codex_config::McpEmaRegistration; use codex_config::types::AuthKeyringBackendKind; use codex_mcp::ema_auth_scope; +use codex_rmcp_client::EmaErrorDetail; use codex_rmcp_client::EnterpriseOAuthLoginRequest; use codex_rmcp_client::perform_enterprise_oauth_login_return_url; use futures::FutureExt; @@ -21,12 +23,12 @@ pub(crate) struct EnterpriseLoginTarget { pub(crate) enum EnterpriseLoginCompletion { Pending, Succeeded, - Failed, + Failed(EmaErrorDetail), } impl EnterpriseLoginCompletion { pub(crate) fn is_complete(self) -> bool { - matches!(self, Self::Succeeded | Self::Failed) + matches!(self, Self::Succeeded | Self::Failed(_)) } } @@ -227,13 +229,19 @@ impl EnterpriseLoginState { _ = setup_cancel.cancelled() => Err(internal_error("enterprise sign-in was cancelled")), _ = cancel.cancelled() => Err(internal_error("enterprise sign-in was cancelled")), result = perform_enterprise_oauth_login_return_url(request) => - result.map_err(|_| internal_error("failed to start enterprise sign-in")), + result.map_err(|error| internal_error( + EmaErrorDetail::from_error( + &error.context(EmaErrorDetail::new("failed to start enterprise sign-in")), + ).to_string(), + )), }; let handle = match handle { Ok(handle) => handle, Err(error) => { self.clear(&login_id).await; - completed.send_replace(EnterpriseLoginCompletion::Failed); + completed.send_replace(EnterpriseLoginCompletion::Failed(EmaErrorDetail::new( + "failed to start enterprise sign-in", + ))); return Err(error); } }; @@ -244,9 +252,12 @@ impl EnterpriseLoginState { tokio::spawn(async move { let result = tokio::select! { biased; - _ = cancel.cancelled() => Err(anyhow::anyhow!("Enterprise sign-in was cancelled.")), + _ = cancel.cancelled() => + Err(EmaErrorDetail::new("Enterprise sign-in was cancelled.").into()), result = async { - let credentials = handle.wait().await?; + let credentials = handle.wait().await.context(EmaErrorDetail::new( + "Enterprise sign-in authorization failed.", + ))?; callback_closed.clone().await; let mut active = credentials.commit_if(|| async { lifecycle.auth_manager.reload().await; @@ -273,7 +284,9 @@ impl EnterpriseLoginState { // the grant, and let new threads pin only the committed credential. lifecycle.config_manager.revoke_mcp_ema(); Some(active) - }).await?; + }).await.context(EmaErrorDetail::new( + "Enterprise sign-in credential commit failed.", + ))?; // A successful commit is no longer cancelable. Clear it while // still holding the commit gate. *active = None; @@ -286,11 +299,9 @@ impl EnterpriseLoginState { if result.is_err() { lifecycle.clear(&completion_id).await; } - let success = result.is_ok(); - completed.send_replace(if success { - EnterpriseLoginCompletion::Succeeded - } else { - EnterpriseLoginCompletion::Failed + completed.send_replace(match result { + Ok(()) => EnterpriseLoginCompletion::Succeeded, + Err(error) => EnterpriseLoginCompletion::Failed(EmaErrorDetail::from_error(&error)), }); }); Ok(EnterpriseLogin { diff --git a/codex-rs/app-server/src/request_processors/mcp_processor.rs b/codex-rs/app-server/src/request_processors/mcp_processor.rs index 79f5bf0ce9..7bee469285 100644 --- a/codex-rs/app-server/src/request_processors/mcp_processor.rs +++ b/codex-rs/app-server/src/request_processors/mcp_processor.rs @@ -253,7 +253,13 @@ impl McpRequestProcessor { login.login_id, Box::pin(async move { match completion.wait_for(|state| state.is_complete()).await { - Ok(result) if *result == EnterpriseLoginCompletion::Succeeded => Ok(()), + Ok(result) => match *result { + EnterpriseLoginCompletion::Succeeded => Ok(()), + EnterpriseLoginCompletion::Failed(detail) => Err(detail.into()), + EnterpriseLoginCompletion::Pending => { + Err(anyhow::anyhow!("Enterprise sign-in failed.")) + } + }, _ => Err(anyhow::anyhow!("Enterprise sign-in failed.")), } }), diff --git a/codex-rs/app-server/tests/suite/v2/account_enterprise_tests.rs b/codex-rs/app-server/tests/suite/v2/account_enterprise_tests.rs index 3881be3e72..994bf5c39d 100644 --- a/codex-rs/app-server/tests/suite/v2/account_enterprise_tests.rs +++ b/codex-rs/app-server/tests/suite/v2/account_enterprise_tests.rs @@ -530,7 +530,7 @@ oauth = {{ client_id = "mcp-client", client_secret = "resource-secret", token_en enterprise_callback(&replacement, &issuer, callback_port)?; assert_eq!( enterprise_completion(&mut client).await?, - json!({"name":"enterprise","threadId":thread_id,"loginId":first["loginId"],"success":false,"error":"Enterprise sign-in failed."}) + json!({"name":"enterprise","threadId":thread_id,"loginId":first["loginId"],"success":false,"error":"Enterprise sign-in was cancelled."}) ); assert_eq!( enterprise_rpc( @@ -543,7 +543,7 @@ oauth = {{ client_id = "mcp-client", client_secret = "resource-secret", token_en ); assert_eq!( enterprise_completion(&mut client).await?, - json!({"name":"enterprise","threadId":thread_id,"loginId":replacement["loginId"],"success":false,"error":"Enterprise sign-in failed."}) + json!({"name":"enterprise","threadId":thread_id,"loginId":replacement["loginId"],"success":false,"error":"Enterprise sign-in was cancelled."}) ); let idp = McpServerIdpOAuthConfig { issuer: issuer.clone(), @@ -606,7 +606,7 @@ oauth = {{ client_id = "mcp-client", client_secret = "resource-secret", token_en assert_eq!(response.status(), http::StatusCode::OK); assert_eq!( enterprise_completion(&mut client).await?, - json!({"name":"enterprise","threadId":thread_id,"loginId":previous["loginId"],"success":false,"error":"Enterprise sign-in failed."}) + json!({"name":"enterprise","threadId":thread_id,"loginId":previous["loginId"],"success":false,"error":"enterprise sign-in no longer matches the active account or configuration"}) ); let retry = enterprise_rpc( @@ -967,6 +967,7 @@ oauth = {{ client_id = "mcp-client", client_secret = "resource-secret", token_en Some(codex_app_server_protocol::McpServerStartupFailureReason::ReauthenticationRequired); for (error, reason) in [ (Some("invalid_client"), None), + (Some("invalid_scope"), None), (Some("insufficient_user_authentication"), reauthentication), (Some("invalid_grant"), reauthentication), (None, reauthentication), @@ -1002,6 +1003,13 @@ oauth = {{ client_id = "mcp-client", client_secret = "resource-secret", token_en }) .await??; assert_eq!(failure.failure_reason, reason, "{error:?}: {failure:?}"); + if error == Some("invalid_scope") { + assert!( + failure.error.as_deref().is_some_and( + |error| error.contains("IdentityProvider: HTTP 400: invalid_scope") + ) + ); + } if reason.is_some() { let status = enterprise_rpc( &client, @@ -1403,7 +1411,7 @@ oauth = {{ client_id = "mcp-client", client_secret = "resource-secret", token_en .await??; assert_eq!( login_completion.context("enterprise login completion")?, - json!({"name":"enterprise","threadId":recovered["thread"]["id"],"loginId":stalled["loginId"],"success":false,"error":"Enterprise sign-in failed."}) + json!({"name":"enterprise","threadId":recovered["thread"]["id"],"loginId":stalled["loginId"],"success":false,"error":"Enterprise sign-in was cancelled."}) ); assert_eq!( resource_exchanges.load(std::sync::atomic::Ordering::SeqCst), diff --git a/codex-rs/codex-mcp/src/ema_auth.rs b/codex-rs/codex-mcp/src/ema_auth.rs index 35d03f8cd0..e7b1ddeb23 100644 --- a/codex-rs/codex-mcp/src/ema_auth.rs +++ b/codex-rs/codex-mcp/src/ema_auth.rs @@ -21,6 +21,7 @@ use codex_login::AuthManager; use codex_login::CodexAuth; use codex_rmcp_client::EmaAuthFailure; use codex_rmcp_client::EmaAuthenticatedHttpClient; +use codex_rmcp_client::EmaErrorDetail; use codex_rmcp_client::EmaRequestAuthority; use codex_rmcp_client::McpOAuthRefreshMode; use codex_rmcp_client::McpProtocolMode; @@ -124,7 +125,14 @@ impl EmaAuth { pub(crate) async fn check_account(&self) -> Result<()> { self.authority.check()?; - if !self.manager.account_login_is_current(&self.account).await? { + if !self + .manager + .account_login_is_current(&self.account) + .await + .context(EmaErrorDetail::new( + "enterprise MCP account credential validation failed", + ))? + { self.authority.revoke(); bail!(codex_rmcp_client::EmaTokenSourceError::new( codex_rmcp_client::EmaCredentialKind::Account, @@ -212,12 +220,16 @@ pub(crate) async fn stored_ema_credentials( OAuthCredentialsStoreMode::Keyring, keyring_backend, ) - .map_err(|_| anyhow!("failed to read enterprise credentials")) + .map_err(|_| anyhow!(EmaErrorDetail::new("failed to read enterprise credentials"))) }) }) }) .await - .map_err(|_| anyhow!("enterprise credential read task failed"))? + .map_err(|_| { + anyhow!(EmaErrorDetail::new( + "enterprise credential read task failed" + )) + })? } // Discovery and both credential exchanges must check the owner at each dispatch, diff --git a/codex-rs/codex-mcp/src/ema_token_source.rs b/codex-rs/codex-mcp/src/ema_token_source.rs index 37fc6ec47e..c73116ed10 100644 --- a/codex-rs/codex-mcp/src/ema_token_source.rs +++ b/codex-rs/codex-mcp/src/ema_token_source.rs @@ -16,6 +16,7 @@ use codex_rmcp_client::EmaAuthTokenExchangeRequest; use codex_rmcp_client::EmaCredentialKind; use codex_rmcp_client::EmaCredentialLease; use codex_rmcp_client::EmaCredentialSnapshot; +use codex_rmcp_client::EmaErrorDetail; use codex_rmcp_client::EmaIdpIdentityRequest; use codex_rmcp_client::EmaInvalidGrantSource; use codex_rmcp_client::EmaTokenSource; @@ -61,7 +62,11 @@ impl EnterpriseTokenSource { http_client: Arc, ) -> Result { auth.check_account().await?; - let account = auth.manager.lease_account_oauth_grant().await?; + let account = auth + .manager + .lease_account_oauth_grant() + .await + .map_err(|_| EmaErrorDetail::new("enterprise MCP account credential read failed"))?; let client_id = account .as_ref() .filter(|lease| { @@ -112,7 +117,12 @@ impl EnterpriseTokenSource { && error .downcast_ref::() .is_some_and(EmaTokenSourceError::is_authentication_required) - && let Some(lease) = self.auth.manager.lease_account_oauth_grant().await? + && let Some(lease) = self + .auth + .manager + .lease_account_oauth_grant() + .await + .map_err(|_| EmaErrorDetail::new("enterprise MCP account credential read failed"))? { let rejected = self .auth @@ -143,7 +153,8 @@ impl EnterpriseTokenSource { .auth .manager .lease_account_oauth_grant() - .await? + .await + .map_err(|_| EmaErrorDetail::new("enterprise MCP account credential read failed"))? .ok_or(EmaAuthFailure::ReauthenticationRequired)?; if !account_grant_is_usable(lease.tokens(), self.registration.idp()) || lease @@ -194,7 +205,10 @@ impl EnterpriseTokenSource { .auth .manager .lease_account_oauth_grant() - .await? + .await + .map_err(|_| { + EmaErrorDetail::new("enterprise MCP account credential read failed") + })? .ok_or(EmaAuthFailure::ReauthenticationRequired)?; if AccountOAuthGrantRevision::from(lease.tokens()) != AccountOAuthGrantRevision::from(tokens) @@ -254,14 +268,22 @@ impl EnterpriseTokenSource { .await?; match receipt { ExchangeReceipt::Account(grant) => { - if grant.current_tokens().await?.as_ref().is_none_or(|tokens| { - !account_grant_is_usable(tokens, idp) - || self - .auth - .config - .ema_state - .requires_sign_in(&self.registration, tokens) - }) { + if grant + .current_tokens() + .await + .map_err(|_| { + EmaErrorDetail::new("enterprise MCP account credential validation failed") + })? + .as_ref() + .is_none_or(|tokens| { + !account_grant_is_usable(tokens, idp) + || self + .auth + .config + .ema_state + .requires_sign_in(&self.registration, tokens) + }) + { // Discard this bearer. The bounded retry below reacquires the grant // only if its credential changed under the same admitted owner. bail!(EmaAuthFailure::ReauthenticationRequired); @@ -305,8 +327,16 @@ impl EnterpriseTokenSource { bail!(EmaTokenSourceError::unexpected(EmaCredentialKind::Account)); } if idp_rejected(&error) - && let Some(lease) = - self.auth.manager.lease_account_oauth_grant().await? + && let Some(lease) = self + .auth + .manager + .lease_account_oauth_grant() + .await + .map_err(|_| { + EmaErrorDetail::new( + "enterprise MCP account credential read failed", + ) + })? { if AccountOAuthGrantRevision::from(lease.tokens()) != AccountOAuthGrantRevision::from(grant.tokens()) @@ -374,7 +404,10 @@ impl EmaTokenSource for EnterpriseTokenSource { None if codex_rmcp_client::is_authentication_required_error(&error) => { EmaTokenSourceError::new(kind, EmaAuthFailure::ReauthenticationRequired) } - None => EmaTokenSourceError::unexpected(kind), + None => EmaTokenSourceError::Unexpected { + credential: kind, + detail: EmaErrorDetail::from_error(&error), + }, } .into() } diff --git a/codex-rs/rmcp-client/src/ema_auth.rs b/codex-rs/rmcp-client/src/ema_auth.rs index 8ed9c523b0..de4556a29c 100644 --- a/codex-rs/rmcp-client/src/ema_auth.rs +++ b/codex-rs/rmcp-client/src/ema_auth.rs @@ -16,6 +16,7 @@ use serde_json::Value; use url::Url; use crate::EmaCredentialLease; +use crate::EmaErrorDetail; use crate::ema_auth_policy::advertised_capability; use crate::ema_auth_policy::resolve_ema_client_auth; use crate::ema_auth_policy::validate_ema_auth_resource; @@ -95,7 +96,10 @@ pub async fn exchange_ema_auth_token( request.mcp_client_secret, request.token_endpoint_auth_method, ) - .await?; + .await + .context(EmaErrorDetail::new( + "enterprise MCP authorization server discovery failed", + ))?; let issuer = metadata .issuer .as_deref() @@ -179,9 +183,9 @@ async fn discover_ema_authorization_metadata( continue; }; if metadata.issuer.as_deref() != Some(issuer.as_str()) { - bail!( + bail!(EmaErrorDetail::new( "enterprise authorization metadata issuer does not match its advertised issuer" - ); + )); } validate_ema_oauth_endpoint( &metadata.token_endpoint, diff --git a/codex-rs/rmcp-client/src/ema_auth_policy.rs b/codex-rs/rmcp-client/src/ema_auth_policy.rs index 71fe5240b1..3fb2499e01 100644 --- a/codex-rs/rmcp-client/src/ema_auth_policy.rs +++ b/codex-rs/rmcp-client/src/ema_auth_policy.rs @@ -5,6 +5,8 @@ use anyhow::Result; use anyhow::anyhow; use anyhow::bail; use codex_config::McpEmaTokenEndpointAuthMethod; +use rmcp::transport::auth::enterprise::EmaError; +use rmcp::transport::auth::enterprise::EmaExchangeStage; use serde_json::Value; use url::Host; use url::Url; @@ -35,8 +37,74 @@ pub enum EmaCredentialKind { Keyring, } -/// Carries only a credential kind and an already sanitized auth failure to transport. -/// Unexpected errors deliberately retain neither provider text nor their source chain. +/// A diagnostic containing only static local text or sanitized SDK exchange fields. +/// Never retains an arbitrary error's display text, provider response, or source chain. +#[derive(Clone, Copy, Debug, Eq, PartialEq)] +pub struct EmaErrorDetail { + message: &'static str, + stage: Option, + status: Option, +} + +impl EmaErrorDetail { + pub fn new(message: &'static str) -> Self { + Self { + message, + stage: None, + status: None, + } + } + + pub fn from_error(error: &anyhow::Error) -> Self { + // SDK diagnostics are already sanitized, including allowlisted OAuth codes. + // Prefer them to an outer operation label when both are available. + if let Some(error) = error.downcast_ref::() { + let (stage, status, message) = match error { + EmaError::InvalidRequest(message) => (None, None, *message), + EmaError::InvalidResponse { stage, message } => (Some(*stage), None, *message), + EmaError::RequestFailed(stage) => (Some(*stage), None, "request failed"), + EmaError::InvalidGrant(stage) => (Some(*stage), None, "invalid_grant"), + EmaError::InsufficientUserAuthentication(stage) => { + (Some(*stage), None, "insufficient_user_authentication") + } + EmaError::OAuthRejected { + stage, + status, + code, + } => (Some(*stage), Some(*status), *code), + _ => (None, None, "enterprise MCP token exchange failed"), + }; + return Self { + message, + stage, + status, + }; + } + error + .chain() + .filter_map(|cause| cause.downcast_ref::()) + .last() + .copied() + .or_else(|| error.downcast_ref::().copied()) + .unwrap_or_else(|| Self::new("enterprise MCP token exchange failed")) + } +} + +impl std::fmt::Display for EmaErrorDetail { + fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + if let Some(stage) = self.stage { + write!(f, "{stage:?}: ")?; + } + if let Some(status) = self.status { + write!(f, "HTTP {status}: ")?; + } + f.write_str(self.message) + } +} + +impl std::error::Error for EmaErrorDetail {} + +/// Carries a credential kind and sanitized failure to transport, without raw error chains. #[derive(Clone, Copy, Debug, Eq, PartialEq)] pub enum EmaTokenSourceError { AuthFailure { @@ -45,6 +113,7 @@ pub enum EmaTokenSourceError { }, Unexpected { credential: EmaCredentialKind, + detail: EmaErrorDetail, }, } @@ -57,12 +126,17 @@ impl EmaTokenSourceError { } pub fn unexpected(credential: EmaCredentialKind) -> Self { - Self::Unexpected { credential } + Self::Unexpected { + credential, + detail: EmaErrorDetail::new("enterprise MCP token exchange failed"), + } } fn credential(self) -> EmaCredentialKind { match self { - Self::AuthFailure { credential, .. } | Self::Unexpected { credential } => credential, + Self::AuthFailure { credential, .. } | Self::Unexpected { credential, .. } => { + credential + } } } @@ -107,6 +181,9 @@ impl EmaTokenSourceError { impl std::fmt::Display for EmaTokenSourceError { fn fmt(&self, f: &mut std::fmt::Formatter<'_>) -> std::fmt::Result { + if let Self::Unexpected { detail, .. } = self { + return std::fmt::Display::fmt(detail, f); + } let message = match (self.credential(), self.failure()) { ( _, diff --git a/codex-rs/rmcp-client/src/ema_auth_tests.rs b/codex-rs/rmcp-client/src/ema_auth_tests.rs index ceeaefe39d..302b703574 100644 --- a/codex-rs/rmcp-client/src/ema_auth_tests.rs +++ b/codex-rs/rmcp-client/src/ema_auth_tests.rs @@ -233,15 +233,15 @@ async fn modern_challenge_confines_metadata_redirects() -> Result<()> { .with_priority(1) .mount(&authorization) .await; - let error = discover_ema_authorization_metadata( - EmaDiscoveryRequest::from(&exchange), - /*client_secret*/ None, - /*token_endpoint_auth_method*/ None, - ) - .await - .err() - .context("cross-origin redirect must fail")?; - assert!(error.to_string().contains("cross-origin redirect")); + let error = exchange_ema_auth_token(exchange, std::convert::identity) + .await + .err() + .context("cross-origin redirect must fail")?; + assert!(format!("{error:#}").contains("cross-origin redirect")); + assert_eq!( + EmaErrorDetail::from_error(&error).to_string(), + "enterprise MCP authorization server discovery failed" + ); assert!( attacker .received_requests() diff --git a/codex-rs/rmcp-client/src/ema_exchange_tests.rs b/codex-rs/rmcp-client/src/ema_exchange_tests.rs index c7a27ae751..37029b75c3 100644 --- a/codex-rs/rmcp-client/src/ema_exchange_tests.rs +++ b/codex-rs/rmcp-client/src/ema_exchange_tests.rs @@ -19,6 +19,9 @@ use wiremock::matchers::method; use wiremock::matchers::path; use super::*; +use crate::EmaCredentialKind; +use crate::EmaErrorDetail; +use crate::EmaTokenSourceError; use crate::ema_auth_policy::validate_ema_oauth_endpoint; use crate::ema_claims::ID_JAG_TOKEN_TYPE; use crate::ema_claims::validate_oidc_identity_assertion; @@ -387,6 +390,7 @@ async fn provider_errors_preserve_credential_policy_without_reflecting_credentia "insufficient_user_authentication", "unauthorized_client", "invalid_client", + "invalid_scope", "malformed", ] { let server = MockServer::builder().start().await; @@ -436,6 +440,24 @@ async fn provider_errors_preserve_credential_policy_without_reflecting_credentia !format!("{error:#?}").contains(SENTINEL), "provider reflected a credential" ); + // Even an unsafe outer context must not reach the transport diagnostic. + let detail = EmaErrorDetail::from_error(&error.context(SENTINEL)); + let transport_error = EmaTokenSourceError::Unexpected { + credential: EmaCredentialKind::Keyring, + detail, + }; + assert!(!format!("{transport_error:#?} {transport_error}").contains(SENTINEL)); + if code == "invalid_scope" { + let stage = if endpoint == "/idp/token" { + "IdentityProvider" + } else { + "ResourceAuthorizationServer" + }; + assert_eq!( + transport_error.to_string(), + format!("{stage}: HTTP 400: invalid_scope") + ); + } assert_eq!( server.received_requests().await.expect("requests").len(), if endpoint == "/as/token" { 2 } else { 1 }, diff --git a/codex-rs/rmcp-client/src/ema_identity.rs b/codex-rs/rmcp-client/src/ema_identity.rs index 60332d9541..c446a8d410 100644 --- a/codex-rs/rmcp-client/src/ema_identity.rs +++ b/codex-rs/rmcp-client/src/ema_identity.rs @@ -18,6 +18,7 @@ use rmcp::transport::auth::OAuthHttpClient; use tracing::instrument::WithSubscriber; use crate::EmaCredentialLease; +use crate::EmaErrorDetail; use crate::ema_auth_policy::advertised_capability; use crate::ema_auth_policy::ema_reauthentication_required; use crate::ema_auth_policy::resolve_ema_client_auth; @@ -96,7 +97,10 @@ pub async fn resolve_ema_account_idp_identity( http_client, redirect_mode, ) - .await?; + .await + .context(EmaErrorDetail::new( + "enterprise IdP discovery or validation failed", + ))?; Ok(EmaIdpIdentity { token_endpoint, refresh_token, @@ -146,13 +150,19 @@ pub(crate) async fn resolve_ema_idp_authorization_manager( let resolution = manager .resolve_metadata() .await - .context("failed to discover enterprise IdP authorization metadata")?; + .context(EmaErrorDetail::new( + "failed to discover enterprise IdP authorization metadata", + ))?; if !resolution.source.is_discovered() { - bail!("enterprise IdP must publish authorization metadata"); + bail!(EmaErrorDetail::new( + "enterprise IdP must publish authorization metadata" + )); } let metadata = resolution.metadata; if metadata.issuer.as_deref() != Some(issuer) { - bail!("enterprise IdP authorization metadata issuer does not match configuration"); + bail!(EmaErrorDetail::new( + "enterprise IdP authorization metadata issuer does not match configuration" + )); } validate_ema_oauth_endpoint(&metadata.token_endpoint, "enterprise IdP token endpoint")?; @@ -196,7 +206,10 @@ async fn resolve_ema_idp_identity_in( request.http_client, request.redirect_mode, ) - .await?; + .await + .context(EmaErrorDetail::new( + "enterprise IdP discovery or validation failed", + ))?; let credentials = request.credentials; let credential_lock = RefreshCredentialLock::acquire_for_server( &credentials.credentials.credentials().server_name, @@ -204,7 +217,7 @@ async fn resolve_ema_idp_identity_in( ) .with_subscriber(tracing::subscriber::NoSubscriber::default()) .await - .map_err(|_| anyhow!("failed to lock enterprise credentials"))?; + .map_err(|_| anyhow!(EmaErrorDetail::new("failed to lock enterprise credentials")))?; let keyring_store = keyring_store.clone(); // The worker retains the short read lock if its caller stops waiting for the reread. tokio::task::spawn_blocking(move || { @@ -230,7 +243,11 @@ async fn resolve_ema_idp_identity_in( }) }) .await - .map_err(|_| anyhow!("enterprise IdP credential reread task failed"))? + .map_err(|_| { + anyhow!(EmaErrorDetail::new( + "enterprise IdP credential reread task failed" + )) + })? } async fn resolve_ema_idp_token_endpoint( diff --git a/codex-rs/rmcp-client/src/enterprise_oauth_login.rs b/codex-rs/rmcp-client/src/enterprise_oauth_login.rs index e74381cf93..3a2bba2a53 100644 --- a/codex-rs/rmcp-client/src/enterprise_oauth_login.rs +++ b/codex-rs/rmcp-client/src/enterprise_oauth_login.rs @@ -25,6 +25,7 @@ use tracing::instrument::WithSubscriber; use url::Host; use url::Url; +use crate::EmaErrorDetail; use crate::StoredOAuthTokens; use crate::ema_auth_policy::validate_ema_oauth_endpoint; use crate::ema_claims::validate_oidc_identity_assertion; @@ -138,7 +139,7 @@ impl EnterpriseOAuthLoginHandle { .complete(/*emit_browser_url*/ false) .with_subscriber(tracing::subscriber::NoSubscriber::default()) .await - .map_err(|_| anyhow!("enterprise IdP authorization failed"))?; + .map_err(|_| anyhow!(EmaErrorDetail::new("enterprise IdP authorization failed")))?; validate_enterprise_credentials(&stored)?; Ok(EnterpriseOAuthCredentials { stored, @@ -171,15 +172,20 @@ impl EnterpriseOAuthCredentials { self.keyring_backend, ) .await?; - let generation = guard - .generation_file - .current() - .map_err(|_| anyhow!("failed to read enterprise login generation"))?; + let generation = guard.generation_file.current().map_err(|_| { + anyhow!(EmaErrorDetail::new( + "failed to read enterprise login generation" + )) + })?; if generation.as_ref() != Some(&self.generation) { - bail!("enterprise sign-in was invalidated by logout"); + bail!(EmaErrorDetail::new( + "enterprise sign-in was invalidated by logout" + )); } let Some(authority) = is_current().await else { - bail!("enterprise sign-in no longer matches the active account or configuration"); + bail!(EmaErrorDetail::new( + "enterprise sign-in no longer matches the active account or configuration" + )); }; tracing::subscriber::with_default(tracing::subscriber::NoSubscriber::default(), || { save_oauth_tokens_with_lock_held( @@ -190,7 +196,11 @@ impl EnterpriseOAuthCredentials { self.keyring_backend, ) }) - .map_err(|_| anyhow!("failed to store enterprise credentials"))?; + .map_err(|_| { + anyhow!(EmaErrorDetail::new( + "failed to store enterprise credentials" + )) + })?; Ok(authority) } } @@ -241,7 +251,14 @@ pub async fn perform_enterprise_oauth_login_return_url( ) .with_subscriber(tracing::subscriber::NoSubscriber::default()) .await - .map_err(|_| anyhow!("failed to start enterprise IdP authorization"))?; + .map_err(|error| { + if error.downcast_ref::().is_some() { + return EmaErrorDetail::from_error(&error); + } + EmaErrorDetail::from_error(&error.context(EmaErrorDetail::new( + "failed to start enterprise IdP authorization", + ))) + })?; // Keep the current attempt valid until replacement setup succeeds. A logout // or another completed setup during this work must still invalidate this start. let generation = async { diff --git a/codex-rs/rmcp-client/src/enterprise_oauth_login_tests.rs b/codex-rs/rmcp-client/src/enterprise_oauth_login_tests.rs index 3e0ed3956c..d9f019524f 100644 --- a/codex-rs/rmcp-client/src/enterprise_oauth_login_tests.rs +++ b/codex-rs/rmcp-client/src/enterprise_oauth_login_tests.rs @@ -152,6 +152,22 @@ async fn enterprise_callback_errors_and_sdk_logs_exclude_credentials() -> Result { return Ok(()); } + let server = MockServer::start().await; + let issuer = format!("{}/idp", server.uri()); + metadata(&server, &format!("https://{SECRET}.example/idp")).await; + let error = login( + &issuer, /*callback_url*/ None, /*auth_method*/ None, + ) + .await + .err() + .expect("reject mismatched discovery issuer"); + assert_eq!( + error.downcast_ref::(), + Some(&EmaErrorDetail::new( + "failed to discover enterprise IdP authorization metadata" + )) + ); + assert!(!format!("{error} {error:?} {error:#}").contains(SECRET)); for (callback_error, token_success) in [(true, false), (false, false), (false, true)] { let server = MockServer::start().await; let issuer = format!("{}/idp", server.uri()); @@ -520,6 +536,13 @@ async fn enterprise_public_api_storage_and_privacy() -> Result<()> { delete_enterprise_oauth_tokens(CREDENTIAL_NAME, &issuer, AuthKeyringBackendKind::Direct) .await .unwrap_err(); + let save_error = save_error.context(EmaErrorDetail::new( + "enterprise sign-in credential commit failed", + )); + assert_eq!( + EmaErrorDetail::from_error(&save_error).to_string(), + "failed to store enterprise credentials" + ); for error in [save_error, delete_error] { assert!(!format!("{error} {error:?} {error:#}").contains(SECRET)); } diff --git a/codex-rs/rmcp-client/src/lib.rs b/codex-rs/rmcp-client/src/lib.rs index 6c8d9e757b..a46b300771 100644 --- a/codex-rs/rmcp-client/src/lib.rs +++ b/codex-rs/rmcp-client/src/lib.rs @@ -51,6 +51,7 @@ pub use ema_auth::EmaAuthTokenExchangeRequest; pub use ema_auth::exchange_ema_auth_token; pub use ema_auth_policy::EmaAuthFailure; pub use ema_auth_policy::EmaCredentialKind; +pub use ema_auth_policy::EmaErrorDetail; pub use ema_auth_policy::EmaInvalidGrantSource; pub use ema_auth_policy::EmaTokenSourceError; pub use ema_auth_policy::validate_ema_auth_resource; diff --git a/codex-rs/rmcp-client/src/oauth/ema_identity.rs b/codex-rs/rmcp-client/src/oauth/ema_identity.rs index fd66161ff5..313ccd22a0 100644 --- a/codex-rs/rmcp-client/src/oauth/ema_identity.rs +++ b/codex-rs/rmcp-client/src/oauth/ema_identity.rs @@ -11,6 +11,7 @@ use codex_keyring_store::KeyringStore; use tracing::instrument::WithSubscriber; use crate::ema_auth_policy::EmaAuthFailure; +use crate::ema_auth_policy::EmaErrorDetail; use crate::ema_auth_policy::ema_reauthentication_required; use crate::ema_claims::OidcClaims; use crate::ema_claims::oidc_identity; @@ -41,7 +42,7 @@ pub struct EmaCredentialLease { fn sanitize_ema_credential_error(error: anyhow::Error, message: &'static str) -> anyhow::Error { match error.downcast::() { Ok(failure) => anyhow::Error::new(failure).context(message), - Err(_) => anyhow!(message), + Err(_) => EmaErrorDetail::new(message).into(), } } @@ -91,7 +92,7 @@ impl StoredOAuthCredentialSnapshot { ) .with_subscriber(tracing::subscriber::NoSubscriber::default()) .await - .map_err(|_| anyhow!("failed to lock enterprise credentials"))?; + .map_err(|_| anyhow!(EmaErrorDetail::new("failed to lock enterprise credentials")))?; let keyring_store = keyring_store.clone(); tokio::task::spawn_blocking(move || { tracing::subscriber::with_default(tracing::subscriber::NoSubscriber::default(), || { @@ -113,7 +114,11 @@ impl StoredOAuthCredentialSnapshot { }) }) .await - .map_err(|_| anyhow!("enterprise credential pinning task failed"))? + .map_err(|_| { + anyhow!(EmaErrorDetail::new( + "enterprise credential pinning task failed" + )) + })? .map_err(|error: anyhow::Error| { sanitize_ema_credential_error(error, "failed to pin enterprise credentials") }) @@ -129,7 +134,7 @@ impl EmaCredentialSnapshot { ) .with_subscriber(tracing::subscriber::NoSubscriber::default()) .await - .map_err(|_| anyhow!("failed to lock enterprise credentials"))?; + .map_err(|_| anyhow!(EmaErrorDetail::new("failed to lock enterprise credentials")))?; let pinned = self.clone(); tokio::task::spawn_blocking(move || { tracing::subscriber::with_default(tracing::subscriber::NoSubscriber::default(), || { @@ -151,7 +156,11 @@ impl EmaCredentialSnapshot { }) }) .await - .map_err(|_| anyhow!("enterprise credential validation task failed"))? + .map_err(|_| { + anyhow!(EmaErrorDetail::new( + "enterprise credential validation task failed" + )) + })? .map_err(|error: anyhow::Error| { sanitize_ema_credential_error(error, "failed to validate enterprise credentials") }) @@ -177,7 +186,7 @@ impl EmaCredentialSnapshot { ) .with_subscriber(tracing::subscriber::NoSubscriber::default()) .await - .map_err(|_| anyhow!("failed to lock enterprise credentials"))?; + .map_err(|_| anyhow!(EmaErrorDetail::new("failed to lock enterprise credentials")))?; if !is_current.await { return Ok(()); }