From 0b0d815fab73ebf33d30d3a57b4cd035513d5d72 Mon Sep 17 00:00:00 2001 From: Matt Van Horn <455140+mvanhorn@users.noreply.github.com> Date: Thu, 4 Jun 2026 17:09:58 -0700 Subject: [PATCH] fix(tui): enrich auth errors with request context Harvested from PR #2792 by @mvanhorn. Reported by @Hmbown in #2665. --- CHANGELOG.md | 4 + crates/tui/CHANGELOG.md | 4 + crates/tui/src/error_taxonomy.rs | 33 ++- crates/tui/src/llm_client/mod.rs | 370 ++++++++++++++++++++++++++++++- 4 files changed, 402 insertions(+), 9 deletions(-) diff --git a/CHANGELOG.md b/CHANGELOG.md index 9314e774..98deb5d2 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -161,6 +161,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Authentication failures now include redacted request context such as provider, + base URL authority, model, key source, key type, and key fingerprint, making + stale provider, endpoint, or API-key state diagnosable without exposing the + secret (#2665, #2792). Thanks @mvanhorn for the implementation. - Browser-opening actions now compile on non-desktop targets by delegating the unsupported-platform error to the shared URL opener instead of hiding the TUI wrapper behind a narrower macOS/Linux/Windows cfg. Thanks @ci4ic4 for the diff --git a/crates/tui/CHANGELOG.md b/crates/tui/CHANGELOG.md index 9314e774..98deb5d2 100644 --- a/crates/tui/CHANGELOG.md +++ b/crates/tui/CHANGELOG.md @@ -161,6 +161,10 @@ and this project adheres to [Semantic Versioning](https://semver.org/spec/v2.0.0 ### Fixed +- Authentication failures now include redacted request context such as provider, + base URL authority, model, key source, key type, and key fingerprint, making + stale provider, endpoint, or API-key state diagnosable without exposing the + secret (#2665, #2792). Thanks @mvanhorn for the implementation. - Browser-opening actions now compile on non-desktop targets by delegating the unsupported-platform error to the shared URL opener instead of hiding the TUI wrapper behind a narrower macOS/Linux/Windows cfg. Thanks @ci4ic4 for the diff --git a/crates/tui/src/error_taxonomy.rs b/crates/tui/src/error_taxonomy.rs index 7f14644f..be032dd3 100644 --- a/crates/tui/src/error_taxonomy.rs +++ b/crates/tui/src/error_taxonomy.rs @@ -234,12 +234,12 @@ impl From for ErrorEnvelope { "llm_timeout", format!("Request timed out after {duration:?}"), ), - LlmError::AuthenticationError(message) => Self::new( + LlmError::AuthenticationError(auth) => Self::new( ErrorCategory::Authentication, ErrorSeverity::Critical, false, "llm_auth_error", - message, + auth.to_user_message(), ), LlmError::AuthorizationError(message) => Self::new( ErrorCategory::Authorization, @@ -566,6 +566,35 @@ mod tests { } } + #[test] + fn llm_auth_error_envelope_renders_context_without_secret() { + let api_key = "tp-secret-token-plan-value"; + let env = ErrorEnvelope::from(LlmError::from_http_response_with_request_context( + 401, + &format!("Invalid API Key: {api_key}"), + Some("Xiaomi MiMo"), + Some("https://token-plan-sgp.xiaomimimo.com/v1"), + Some("mimo-v2.5"), + Some("env"), + Some(api_key), + )); + + assert_eq!(env.category, ErrorCategory::Authentication); + assert_eq!(env.severity, ErrorSeverity::Critical); + assert!(!env.recoverable); + assert!(env.message.contains("provider: Xiaomi MiMo")); + assert!( + env.message + .contains("base URL authority: token-plan-sgp.xiaomimimo.com") + ); + assert!(env.message.contains("model: mimo-v2.5")); + assert!(env.message.contains("key source: env")); + assert!(env.message.contains("key fingerprint: tp-... (len=26)")); + assert!(env.message.contains("key type: Xiaomi MiMo Token Plan key")); + assert!(!env.message.contains(api_key)); + assert!(!env.message.contains("secret-token-plan-value")); + } + #[test] fn authorization_catches_forbidden_and_denied() { for msg in [ diff --git a/crates/tui/src/llm_client/mod.rs b/crates/tui/src/llm_client/mod.rs index 90a65209..de849328 100644 --- a/crates/tui/src/llm_client/mod.rs +++ b/crates/tui/src/llm_client/mod.rs @@ -82,6 +82,194 @@ pub trait RetryConfigurable { fn set_retry_config(&mut self, config: RetryConfig); } +// === Authentication diagnostics === + +#[derive(Debug, Clone, PartialEq, Eq, Default)] +pub struct AuthenticationErrorContext { + pub provider: Option, + pub base_url_authority: Option, + pub model: Option, + pub key_source: Option, + pub key_fingerprint: Option, + pub key_kind: Option, +} + +impl AuthenticationErrorContext { + #[must_use] + pub fn new( + provider: &str, + base_url: &str, + model: &str, + key_source: &str, + api_key: &str, + ) -> Self { + Self::from_parts( + Some(provider), + Some(base_url), + Some(model), + Some(key_source), + Some(api_key), + ) + } + + #[must_use] + pub fn from_parts( + provider: Option<&str>, + base_url: Option<&str>, + model: Option<&str>, + key_source: Option<&str>, + api_key: Option<&str>, + ) -> Self { + let api_key = api_key.and_then(non_empty_trimmed); + Self { + provider: provider.and_then(non_empty_trimmed).map(str::to_string), + base_url_authority: base_url.and_then(base_url_authority), + model: model.and_then(non_empty_trimmed).map(str::to_string), + key_source: key_source.and_then(non_empty_trimmed).map(str::to_string), + key_fingerprint: api_key.map(redacted_key_fingerprint), + key_kind: api_key.map(classify_api_key_prefix).map(str::to_string), + } + } + + fn is_empty(&self) -> bool { + self.provider.is_none() + && self.base_url_authority.is_none() + && self.model.is_none() + && self.key_source.is_none() + && self.key_fingerprint.is_none() + && self.key_kind.is_none() + } + + fn detail_segments(&self) -> Vec { + let mut segments = Vec::new(); + if let Some(provider) = self.provider.as_deref() { + segments.push(format!("provider: {provider}")); + } + if let Some(authority) = self.base_url_authority.as_deref() { + segments.push(format!("base URL authority: {authority}")); + } + if let Some(model) = self.model.as_deref() { + segments.push(format!("model: {model}")); + } + if let Some(source) = self.key_source.as_deref() { + segments.push(format!("key source: {source}")); + } + if let Some(fingerprint) = self.key_fingerprint.as_deref() { + segments.push(format!("key fingerprint: {fingerprint}")); + } + if let Some(kind) = self.key_kind.as_deref() { + segments.push(format!("key type: {kind}")); + } + segments + } +} + +#[derive(Debug, Clone, PartialEq, Eq)] +pub struct AuthenticationErrorDetail { + message: String, + context: Option, +} + +impl AuthenticationErrorDetail { + #[must_use] + pub fn new(message: impl Into) -> Self { + Self { + message: message.into(), + context: None, + } + } + + #[must_use] + pub fn with_context( + message: impl Into, + context: Option, + ) -> Self { + let context = context.filter(|context| !context.is_empty()); + Self { + message: message.into(), + context, + } + } + + #[must_use] + pub fn message(&self) -> &str { + &self.message + } + + #[must_use] + pub fn to_user_message(&self) -> String { + let Some(context) = self.context.as_ref() else { + return self.message.clone(); + }; + let segments = context.detail_segments(); + if segments.is_empty() { + self.message.clone() + } else { + format!("{} ({})", self.message, segments.join(", ")) + } + } +} + +impl From for AuthenticationErrorDetail { + fn from(message: String) -> Self { + Self::new(message) + } +} + +impl From<&str> for AuthenticationErrorDetail { + fn from(message: &str) -> Self { + Self::new(message) + } +} + +#[must_use] +pub fn classify_api_key_prefix(api_key: &str) -> &'static str { + if api_key.starts_with("tp-") { + "Xiaomi MiMo Token Plan key" + } else { + "API key" + } +} + +fn non_empty_trimmed(value: &str) -> Option<&str> { + let value = value.trim(); + if value.is_empty() { None } else { Some(value) } +} + +fn base_url_authority(base_url: &str) -> Option { + let base_url = non_empty_trimmed(base_url)?; + let without_scheme = base_url + .split_once("://") + .map_or(base_url, |(_, rest)| rest); + let authority = without_scheme.split('/').next().unwrap_or(without_scheme); + let authority = authority + .rsplit_once('@') + .map_or(authority, |(_, authority)| authority); + non_empty_trimmed(authority).map(str::to_string) +} + +fn redacted_key_fingerprint(api_key: &str) -> String { + let api_key = api_key.trim(); + let len = api_key.chars().count(); + match public_key_prefix(api_key) { + Some(prefix) => format!("{prefix}... (len={len})"), + None => format!("unprefixed (len={len})"), + } +} + +fn public_key_prefix(api_key: &str) -> Option<&str> { + ["tp-", "sk-", "hf_", "hf-", "ak-", "rk-"] + .into_iter() + .find(|prefix| api_key.starts_with(prefix)) +} + +fn redact_api_key_from_message(message: &str, api_key: Option<&str>) -> String { + let Some(api_key) = api_key.and_then(non_empty_trimmed) else { + return message.to_string(); + }; + message.replace(api_key, "[redacted API key]") +} + // === LlmError - Classified Error Types === /// Classified LLM errors with retryability information. @@ -107,8 +295,8 @@ pub enum LlmError { /// Request timed out Timeout(Duration), - /// Authentication failed (HTTP 401, 403) - AuthenticationError(String), + /// Authentication failed (HTTP 401, selected HTTP 403) + AuthenticationError(AuthenticationErrorDetail), /// Authorization or provider-side blocking failed (HTTP 403) AuthorizationError(String), @@ -141,7 +329,9 @@ impl std::fmt::Display for LlmError { } LlmError::NetworkError(msg) => write!(f, "Network error: {msg}"), LlmError::Timeout(d) => write!(f, "Request timed out after {d:?}"), - LlmError::AuthenticationError(msg) => write!(f, "Authentication failed: {msg}"), + LlmError::AuthenticationError(auth) => { + write!(f, "Authentication failed: {}", auth.to_user_message()) + } LlmError::AuthorizationError(msg) => write!(f, "Authorization failed: {msg}"), LlmError::InvalidRequest { status, message } => { write!(f, "Invalid request ({status}): {message}") @@ -203,10 +393,10 @@ impl LlmError { message: body.to_string(), retry_after: None, }, - 401 => LlmError::AuthenticationError(body.to_string()), + 401 => Self::authentication_error(body), 403 => { if looks_like_authentication_failure(body) { - LlmError::AuthenticationError(body.to_string()) + Self::authentication_error(body) } else { LlmError::AuthorizationError(body.to_string()) } @@ -262,6 +452,62 @@ impl LlmError { } } + #[must_use] + pub fn authentication_error(message: impl Into) -> Self { + LlmError::AuthenticationError(AuthenticationErrorDetail::new(message)) + } + + #[must_use] + pub fn authentication_error_with_context( + message: impl Into, + context: Option, + ) -> Self { + LlmError::AuthenticationError(AuthenticationErrorDetail::with_context(message, context)) + } + + /// Constructs an `LlmError` from HTTP response data plus request context + /// that is safe to display when authentication fails. + #[must_use] + pub fn from_http_response_with_request_context( + status: u16, + body: &str, + provider: Option<&str>, + base_url: Option<&str>, + model: Option<&str>, + key_source: Option<&str>, + api_key: Option<&str>, + ) -> Self { + let body = redact_api_key_from_message(body, api_key); + let context = + AuthenticationErrorContext::from_parts(provider, base_url, model, key_source, api_key); + Self::from_http_response_with_auth_context(status, &body, Some(context)) + } + + /// Constructs an `LlmError` from HTTP status code and response body, with + /// optional structured details for authentication failures. + /// + /// The `body` passed here must already be safe for user display. Prefer + /// [`Self::from_http_response_with_request_context`] when the raw API key is + /// available so the response body can be redacted before rendering. + #[must_use] + pub fn from_http_response_with_auth_context( + status: u16, + body: &str, + auth_context: Option, + ) -> Self { + match status { + 401 => Self::authentication_error_with_context(body, auth_context), + 403 => { + if looks_like_authentication_failure(body) { + Self::authentication_error_with_context(body, auth_context) + } else { + LlmError::AuthorizationError(body.to_string()) + } + } + _ => Self::from_http_response(status, body), + } + } + /// Constructs an `LlmError` from HTTP status code, body, and optional Retry-After header. pub fn from_http_response_with_retry_after( status: u16, @@ -898,6 +1144,13 @@ mod tests { ); } + fn auth_user_message(error: LlmError) -> String { + match error { + LlmError::AuthenticationError(auth) => auth.to_user_message(), + other => panic!("expected authentication error, got {other}"), + } + } + #[test] fn test_retry_config_defaults() { let config = RetryConfig::default(); @@ -1014,7 +1267,7 @@ mod tests { assert!(LlmError::Timeout(Duration::from_secs(30)).is_retryable()); // Non-retryable errors - assert!(!LlmError::AuthenticationError("invalid key".to_string()).is_retryable()); + assert!(!LlmError::authentication_error("invalid key").is_retryable()); assert!(!LlmError::AuthorizationError("blocked".to_string()).is_retryable()); assert!( !LlmError::InvalidRequest { @@ -1071,6 +1324,109 @@ mod tests { assert!(matches!(err, LlmError::InvalidRequest { status: 400, .. })); } + #[test] + fn auth_error_with_context_includes_provider_authority_model_and_key_source() { + let err = LlmError::from_http_response_with_request_context( + 401, + "Invalid API Key", + Some("Xiaomi MiMo"), + Some("https://token-plan-sgp.xiaomimimo.com/v1"), + Some("mimo-v2.5"), + Some("env"), + Some("tp-secret-token-plan-value"), + ); + let message = auth_user_message(err); + + assert!(message.contains("Invalid API Key")); + assert!(message.contains("provider: Xiaomi MiMo")); + assert!(message.contains("base URL authority: token-plan-sgp.xiaomimimo.com")); + assert!(message.contains("model: mimo-v2.5")); + assert!(message.contains("key source: env")); + assert!(message.contains("key fingerprint: tp-... (len=26)")); + } + + #[test] + fn auth_error_redacts_full_api_key_from_body_and_context() { + let api_key = "tp-secret-token-plan-value"; + let err = LlmError::from_http_response_with_request_context( + 401, + &format!("Invalid API Key: {api_key}"), + Some("Xiaomi MiMo"), + Some("https://token-plan-sgp.xiaomimimo.com/v1"), + Some("mimo-v2.5"), + Some("config-file"), + Some(api_key), + ); + let message = auth_user_message(err); + + assert!(!message.contains(api_key)); + assert!(!message.contains("secret-token-plan-value")); + assert!(message.contains("[redacted API key]")); + assert!(message.contains("key fingerprint: tp-... (len=26)")); + } + + #[test] + fn auth_error_classifies_xiaomi_token_plan_key_prefix() { + let token_plan = AuthenticationErrorContext::from_parts( + None, + None, + None, + Some("session"), + Some("tp-secret-token-plan-value"), + ); + let generic = AuthenticationErrorContext::from_parts( + None, + None, + None, + Some("session"), + Some("sk-other"), + ); + let unprefixed = AuthenticationErrorContext::from_parts( + None, + None, + None, + Some("session"), + Some("plainsecretvalue"), + ); + + assert_eq!( + token_plan.key_kind.as_deref(), + Some("Xiaomi MiMo Token Plan key") + ); + assert_eq!(generic.key_kind.as_deref(), Some("API key")); + assert_eq!(unprefixed.key_kind.as_deref(), Some("API key")); + assert_eq!( + unprefixed.key_fingerprint.as_deref(), + Some("unprefixed (len=16)") + ); + } + + #[test] + fn authorization_403_is_not_reclassified_by_auth_context() { + let err = LlmError::from_http_response_with_request_context( + 403, + "forbidden", + Some("Arcee AI"), + Some("https://api.arcee.ai/v1"), + Some("auto"), + Some("env"), + Some("sk-arcee-secret"), + ); + + assert!(matches!(err, LlmError::AuthorizationError(_))); + } + + #[test] + fn auth_error_without_context_preserves_bare_message() { + let err = LlmError::from_http_response_with_auth_context( + 401, + "Invalid API Key", + Some(AuthenticationErrorContext::default()), + ); + + assert_eq!(auth_user_message(err), "Invalid API Key"); + } + #[test] fn cloudflare_html_error_is_summarized_without_raw_markup() { let body = r#"Access Denied