diff --git a/crates/aisix-gateway/src/bridge.rs b/crates/aisix-gateway/src/bridge.rs index d7488178..f99cd1a0 100644 --- a/crates/aisix-gateway/src/bridge.rs +++ b/crates/aisix-gateway/src/bridge.rs @@ -155,6 +155,15 @@ pub enum BridgeError { UpstreamDecode(String), #[error("bridge is misconfigured: {0}")] Config(String), + /// Customer-fixable upstream config — the admin's ProviderKey/Model + /// is set up wrong (empty secret, missing api_base, missing + /// model_name) or the caller's request/key is malformed. Maps to + /// 400, not 500: it's the caller's mistake, retrying won't help, and + /// a 5xx wrongly tells SDKs/monitoring it's a server fault (#367). + /// Contrast [`Config`], reserved for errors *we* cause + /// (serialization, our generated request_id) which stays 500. + #[error("invalid upstream configuration: {0}")] + InvalidUpstreamConfig(String), #[error("transport error: {0}")] Transport(String), #[error("upstream cancelled the response mid-stream")] @@ -361,6 +370,7 @@ impl BridgeError { } BridgeError::UpstreamDecode(_) => 502, BridgeError::Config(_) => 500, + BridgeError::InvalidUpstreamConfig(_) => 400, BridgeError::Transport(_) => 502, BridgeError::StreamAborted => 502, } @@ -373,6 +383,7 @@ impl BridgeError { BridgeError::UpstreamStatus { .. } => "upstream_error", BridgeError::UpstreamDecode(_) => "upstream_decode_error", BridgeError::Config(_) => "config_error", + BridgeError::InvalidUpstreamConfig(_) => "invalid_request_error", BridgeError::Transport(_) => "transport_error", BridgeError::StreamAborted => "stream_aborted", } @@ -537,6 +548,20 @@ mod tests { BridgeError::Config("missing api_key".into()).http_status(), 500 ); + assert_eq!( + BridgeError::Config("missing api_key".into()).error_type(), + "config_error" + ); + } + + #[test] + fn invalid_upstream_config_maps_to_400_invalid_request() { + // #367: customer-fixable config (empty secret, missing api_base, + // missing model_name, …) is a 400, not a 500 — retrying won't + // help and a 5xx wrongly reads as a server fault. + let e = BridgeError::InvalidUpstreamConfig("provider_key.secret is empty".into()); + assert_eq!(e.http_status(), 400); + assert_eq!(e.error_type(), "invalid_request_error"); } #[test] diff --git a/crates/aisix-provider-anthropic/src/bridge.rs b/crates/aisix-provider-anthropic/src/bridge.rs index 39189953..4999e191 100644 --- a/crates/aisix-provider-anthropic/src/bridge.rs +++ b/crates/aisix-provider-anthropic/src/bridge.rs @@ -135,7 +135,7 @@ fn resolve_base(ctx: &BridgeContext) -> Result { provider_metadata.api_base_url on the control plane; standalone: \ directly on the resource)." ); - return Err(BridgeError::Config(format!( + return Err(BridgeError::InvalidUpstreamConfig(format!( "provider_key for vendor {pk_vendor_raw:?} has no upstream base URL \ configured" ))); @@ -148,17 +148,27 @@ fn resolve_base(ctx: &BridgeContext) -> Result { fn api_key(ctx: &BridgeContext) -> Result<&str, BridgeError> { let k = &ctx.provider_key.secret; if k.is_empty() { - Err(BridgeError::Config("provider_key.secret is empty".into())) - } else { - Ok(k.as_str()) - } + return Err(BridgeError::InvalidUpstreamConfig( + "provider_key.secret is empty".into(), + )); + } + // Reject a secret that can't be a valid `x-api-key` header value + // (control bytes etc.) up front as customer-fixable config, mirroring + // the openai / azure bridges — otherwise reqwest's `.header()` fails + // later with an opaque builder error (#367). + if header::HeaderValue::from_str(k).is_err() { + return Err(BridgeError::InvalidUpstreamConfig( + "provider_key.secret contains invalid header characters".into(), + )); + } + Ok(k.as_str()) } fn upstream_model(ctx: &BridgeContext) -> Result<&str, BridgeError> { ctx.model .model_name .as_deref() - .ok_or_else(|| BridgeError::Config("model.model_name missing".into())) + .ok_or_else(|| BridgeError::InvalidUpstreamConfig("model.model_name missing".into())) } async fn map_http_error(status: StatusCode, resp: reqwest::Response) -> BridgeError { @@ -237,7 +247,7 @@ impl Bridge for AnthropicBridge { let upstream = upstream_model(ctx)?; let (system, messages) = - split_system(req).map_err(|e| BridgeError::Config(e.to_string()))?; + split_system(req).map_err(|e| BridgeError::InvalidUpstreamConfig(e.to_string()))?; let body = build_request(req, upstream, system, messages, false); let url = format!("{base}/v1/messages"); let client = self.client.clone(); @@ -281,7 +291,7 @@ impl Bridge for AnthropicBridge { let upstream = upstream_model(ctx)?; let (system, messages) = - split_system(req).map_err(|e| BridgeError::Config(e.to_string()))?; + split_system(req).map_err(|e| BridgeError::InvalidUpstreamConfig(e.to_string()))?; let body = build_request(req, upstream, system, messages, true); let url = format!("{base}/v1/messages"); let client = self.client.clone(); @@ -540,7 +550,25 @@ mod tests { let bridge = AnthropicBridge::new(); let ctx = BridgeContext::new("req-1", sample_model(), Arc::new(pk)); let err = bridge.chat(&req(), &ctx).await.unwrap_err(); - assert!(matches!(err, BridgeError::Config(_))); + assert!(matches!(err, BridgeError::InvalidUpstreamConfig(_))); + } + + #[tokio::test] + async fn secret_with_control_chars_is_invalid_config() { + // A non-empty secret that can't be an x-api-key header value + // (control bytes) is customer-fixable config, not a 500 (#367). + let pk: ProviderKey = + serde_json::from_str(r#"{"display_name":"bad","secret":"sk-live\n-injected"}"#) + .unwrap(); + let bridge = AnthropicBridge::new(); + let ctx = BridgeContext::new("req-1", sample_model(), Arc::new(pk)); + let err = bridge.chat(&req(), &ctx).await.unwrap_err(); + match err { + BridgeError::InvalidUpstreamConfig(msg) => { + assert!(msg.contains("invalid header characters"), "got {msg}"); + } + other => panic!("expected InvalidUpstreamConfig, got {other:?}"), + } } #[tokio::test] @@ -565,7 +593,7 @@ mod tests { }], ); let err = bridge.chat(&req, &ctx).await.unwrap_err(); - assert!(matches!(err, BridgeError::Config(_))); + assert!(matches!(err, BridgeError::InvalidUpstreamConfig(_))); } #[tokio::test] @@ -718,7 +746,7 @@ data: {\"type\":\"message_stop\"}\n\n"; let ctx = BridgeContext::new("rid", sample_model(), Arc::new(pk)); let err = resolve_base(&ctx).unwrap_err(); match err { - BridgeError::Config(msg) => { + BridgeError::InvalidUpstreamConfig(msg) => { assert!( msg.contains("base URL") && msg.contains(vendor.trim()), "vendor {vendor:?}: error must name vendor + base URL; got: {msg}", @@ -734,7 +762,9 @@ data: {\"type\":\"message_stop\"}\n\n"; ); } } - other => panic!("vendor {vendor:?}: expected BridgeError::Config, got {other:?}"), + other => { + panic!("vendor {vendor:?}: expected InvalidUpstreamConfig, got {other:?}") + } } } } diff --git a/crates/aisix-provider-azure-openai/src/bridge.rs b/crates/aisix-provider-azure-openai/src/bridge.rs index 79da230f..5a23c722 100644 --- a/crates/aisix-provider-azure-openai/src/bridge.rs +++ b/crates/aisix-provider-azure-openai/src/bridge.rs @@ -242,7 +242,7 @@ impl AzureUpstreamRef { let base = api_base.unwrap_or_default().trim(); if base.is_empty() { - return Err(BridgeError::Config( + return Err(BridgeError::InvalidUpstreamConfig( "azure provider_key has no api_base — \ expected https://.openai.azure.com, a bare resource name, \ or a verbatim override URL (https://[:])" @@ -286,20 +286,20 @@ impl AzureUpstreamRef { // `rest` is post-scheme; an `@` here means userinfo // (`user:pass@host`). Operators must use the // api-key / AAD path for auth, never URL-embedded. - return Err(BridgeError::Config(format!( + return Err(BridgeError::InvalidUpstreamConfig(format!( "azure api_base {base:?} must not embed userinfo (@); use the \ api-key / AAD credentials in `provider_key.secret` instead" ))); } if base.contains('?') { - return Err(BridgeError::Config(format!( + return Err(BridgeError::InvalidUpstreamConfig(format!( "azure api_base {base:?} must not contain a query string \ (the bridge appends `?api-version=…`; an operator-supplied \ query would either merge or override the pinned api-version)" ))); } if base.contains('#') { - return Err(BridgeError::Config(format!( + return Err(BridgeError::InvalidUpstreamConfig(format!( "azure api_base {base:?} must not contain a fragment" ))); } @@ -351,7 +351,7 @@ impl AzureUpstreamRef { /// (e.g. `?api-version=evil` to override the bridge's version pin). fn validate_url_token(name: &str, value: &str) -> Result<(), BridgeError> { if value.is_empty() { - return Err(BridgeError::Config(format!( + return Err(BridgeError::InvalidUpstreamConfig(format!( "azure {name} is empty (expected an identifier matching [A-Za-z0-9_-]+)" ))); } @@ -359,7 +359,7 @@ fn validate_url_token(name: &str, value: &str) -> Result<(), BridgeError> { .bytes() .all(|b| b.is_ascii_alphanumeric() || b == b'-' || b == b'_') { - return Err(BridgeError::Config(format!( + return Err(BridgeError::InvalidUpstreamConfig(format!( "azure {name} {value:?} contains URL-control characters — \ must match [A-Za-z0-9_-]+ (no spaces, slashes, dots, query params, or hash)" ))); @@ -394,12 +394,14 @@ impl AzureSecret { pub(crate) fn parse(secret: &str) -> Result { let trimmed = secret.trim(); if trimmed.is_empty() { - return Err(BridgeError::Config("provider_key.secret is empty".into())); + return Err(BridgeError::InvalidUpstreamConfig( + "provider_key.secret is empty".into(), + )); } if trimmed.starts_with('{') { let creds: crate::aad_token_mint::AadCredentials = serde_json::from_str(trimmed) .map_err(|_e| { - BridgeError::Config( + BridgeError::InvalidUpstreamConfig( "azure provider_key.secret looks JSON-shaped but failed to parse \ as AAD client_credentials \ {tenant_id, client_id, client_secret}" @@ -430,7 +432,7 @@ fn upstream_model(ctx: &BridgeContext) -> Result<&str, BridgeError> { ctx.model .model_name .as_deref() - .ok_or_else(|| BridgeError::Config("model.model_name missing".into())) + .ok_or_else(|| BridgeError::InvalidUpstreamConfig("model.model_name missing".into())) } /// Map an Azure HTTP error response to a customer-visible @@ -600,7 +602,9 @@ fn build_request_headers( match (&auth.api_key, &auth.bearer_token) { (Some(key), None) => { let value = HeaderValue::from_str(key).map_err(|e| { - BridgeError::Config(format!("api key contains invalid header chars: {e}")) + BridgeError::InvalidUpstreamConfig(format!( + "api key contains invalid header chars: {e}" + )) })?; headers.insert(HeaderName::from_static("api-key"), value); } @@ -919,13 +923,13 @@ mod tests { fn resolve_rejects_empty_deployment() { let err = AzureUpstreamRef::resolve("", Some("https://acme.openai.azure.com")).unwrap_err(); match err { - BridgeError::Config(msg) => { + BridgeError::InvalidUpstreamConfig(msg) => { assert!( msg.contains("deployment name is empty"), "must call out empty deployment; got {msg}" ); } - other => panic!("expected Config error, got {other:?}"), + other => panic!("expected InvalidUpstreamConfig error, got {other:?}"), } } @@ -933,13 +937,13 @@ mod tests { fn resolve_rejects_missing_api_base() { let err = AzureUpstreamRef::resolve("dep", None).unwrap_err(); match err { - BridgeError::Config(msg) => { + BridgeError::InvalidUpstreamConfig(msg) => { assert!( msg.contains("no api_base"), "must call out missing api_base; got {msg}" ); } - other => panic!("expected Config error, got {other:?}"), + other => panic!("expected InvalidUpstreamConfig error, got {other:?}"), } } @@ -947,10 +951,10 @@ mod tests { fn resolve_rejects_empty_api_base() { let err = AzureUpstreamRef::resolve("dep", Some(" ")).unwrap_err(); match err { - BridgeError::Config(msg) => { + BridgeError::InvalidUpstreamConfig(msg) => { assert!(msg.contains("no api_base")); } - other => panic!("expected Config error, got {other:?}"), + other => panic!("expected InvalidUpstreamConfig error, got {other:?}"), } } @@ -972,29 +976,29 @@ mod tests { fn resolve_rejects_deployment_with_query_injection() { let err = AzureUpstreamRef::resolve("foo?api-version=evil", Some("acme-east")).unwrap_err(); match err { - BridgeError::Config(msg) => { + BridgeError::InvalidUpstreamConfig(msg) => { assert!(msg.contains("URL-control characters"), "got {msg}"); } - other => panic!("expected Config error, got {other:?}"), + other => panic!("expected InvalidUpstreamConfig error, got {other:?}"), } } #[test] fn resolve_rejects_deployment_with_slash_injection() { let err = AzureUpstreamRef::resolve("foo/bar/chat", Some("acme")).unwrap_err(); - assert!(matches!(err, BridgeError::Config(_))); + assert!(matches!(err, BridgeError::InvalidUpstreamConfig(_))); } #[test] fn resolve_rejects_deployment_with_hash_fragment() { let err = AzureUpstreamRef::resolve("foo#bar", Some("acme")).unwrap_err(); - assert!(matches!(err, BridgeError::Config(_))); + assert!(matches!(err, BridgeError::InvalidUpstreamConfig(_))); } #[test] fn resolve_rejects_resource_with_query_injection() { let err = AzureUpstreamRef::resolve("dep", Some("acme?evil=1")).unwrap_err(); - assert!(matches!(err, BridgeError::Config(_))); + assert!(matches!(err, BridgeError::InvalidUpstreamConfig(_))); } #[test] @@ -1056,13 +1060,13 @@ mod tests { AzureUpstreamRef::resolve("dep", Some("https://proxy.acme.internal?api-version=evil")) .unwrap_err(); match err { - BridgeError::Config(msg) => { + BridgeError::InvalidUpstreamConfig(msg) => { assert!( msg.contains("query string"), "must call out the query rejection; got {msg}" ); } - other => panic!("expected Config error, got {other:?}"), + other => panic!("expected InvalidUpstreamConfig error, got {other:?}"), } } @@ -1070,7 +1074,7 @@ mod tests { fn resolve_rejects_override_with_fragment() { let err = AzureUpstreamRef::resolve("dep", Some("https://proxy.acme.internal#fragment")) .unwrap_err(); - assert!(matches!(err, BridgeError::Config(_))); + assert!(matches!(err, BridgeError::InvalidUpstreamConfig(_))); } #[test] @@ -1081,13 +1085,13 @@ mod tests { let err = AzureUpstreamRef::resolve("dep", Some("https://user:pass@proxy.acme.internal")) .unwrap_err(); match err { - BridgeError::Config(msg) => { + BridgeError::InvalidUpstreamConfig(msg) => { assert!( msg.contains("userinfo"), "must call out the userinfo rejection; got {msg}" ); } - other => panic!("expected Config error, got {other:?}"), + other => panic!("expected InvalidUpstreamConfig error, got {other:?}"), } } @@ -1328,7 +1332,7 @@ mod tests { // headers via the api-key value. let err = build_request_headers(&api_key_auth("legit\nx-evil: 1"), "req-1", false, None) .unwrap_err(); - assert!(matches!(err, BridgeError::Config(_))); + assert!(matches!(err, BridgeError::InvalidUpstreamConfig(_))); } #[test] @@ -1345,10 +1349,10 @@ mod tests { let req = ChatFormat::new("customer-facing-name", vec![ChatMessage::user("hi")]); let err = bridge.chat(&req, &ctx).await.unwrap_err(); match err { - BridgeError::Config(msg) => { + BridgeError::InvalidUpstreamConfig(msg) => { assert!(msg.contains("no api_base"), "got {msg}"); } - other => panic!("expected Config error, got {other:?}"), + other => panic!("expected InvalidUpstreamConfig error, got {other:?}"), } } @@ -1366,10 +1370,10 @@ mod tests { let req = ChatFormat::new("customer-facing-name", vec![ChatMessage::user("hi")]); let err = bridge.chat(&req, &ctx).await.unwrap_err(); match err { - BridgeError::Config(msg) => { + BridgeError::InvalidUpstreamConfig(msg) => { assert!(msg.contains("secret is empty"), "got {msg}"); } - other => panic!("expected Config error, got {other:?}"), + other => panic!("expected InvalidUpstreamConfig error, got {other:?}"), } } @@ -2039,14 +2043,14 @@ mod tests { #[test] fn azure_secret_rejects_empty_secret() { let err = AzureSecret::parse(" ").unwrap_err(); - assert!(matches!(err, BridgeError::Config(_))); + assert!(matches!(err, BridgeError::InvalidUpstreamConfig(_))); } #[test] fn azure_secret_rejects_json_missing_required_aad_fields() { let err = AzureSecret::parse(r#"{"tenant_id":"t"}"#).unwrap_err(); match err { - BridgeError::Config(msg) => { + BridgeError::InvalidUpstreamConfig(msg) => { // Message must NOT echo raw secret bytes (audit-aware). assert!(msg.contains("looks JSON-shaped")); assert!(!msg.contains("tenant-uuid-aaa")); diff --git a/crates/aisix-provider-bedrock/src/bridge.rs b/crates/aisix-provider-bedrock/src/bridge.rs index 7b5593d0..0be31245 100644 --- a/crates/aisix-provider-bedrock/src/bridge.rs +++ b/crates/aisix-provider-bedrock/src/bridge.rs @@ -306,7 +306,7 @@ impl BedrockSecret { /// generic shape errors. fn parse(secret: &str) -> Result { if secret.trim().is_empty() { - return Err(BridgeError::Config( + return Err(BridgeError::InvalidUpstreamConfig( "bedrock provider_key.secret is empty — \ expected JSON {access_key_id, secret_access_key, region, session_token?}" .into(), @@ -318,7 +318,7 @@ impl BedrockSecret { // "invalid character 'X' at position N" reveals what's // in the JSON). Generic shape hint is enough for the // operator who controls the registration. - BridgeError::Config( + BridgeError::InvalidUpstreamConfig( "bedrock provider_key.secret must be valid JSON: \ {access_key_id, secret_access_key, region, session_token?}" .into(), @@ -335,7 +335,7 @@ fn build_client( request: Option<&RequestOverrides>, ) -> Result { if creds.region.trim().is_empty() { - return Err(BridgeError::Config( + return Err(BridgeError::InvalidUpstreamConfig( "bedrock provider_key.secret.region is empty — \ AWS Bedrock dispatch is region-keyed and requires e.g. \"us-west-2\"" .into(), @@ -471,7 +471,7 @@ fn upstream_model(ctx: &BridgeContext) -> Result<&str, BridgeError> { ctx.model .model_name .as_deref() - .ok_or_else(|| BridgeError::Config("model.model_name missing".into())) + .ok_or_else(|| BridgeError::InvalidUpstreamConfig("model.model_name missing".into())) } /// Translate an SDK error into the canonical `BridgeError`. @@ -681,7 +681,7 @@ impl BedrockBridge { let client = self.build_client_from_ctx(ctx)?; let (system, messages) = - split_system(req).map_err(|e| BridgeError::Config(format!("{e}")))?; + split_system(req).map_err(|e| BridgeError::InvalidUpstreamConfig(format!("{e}")))?; let anthropic_req = build_request(req, upstream_id, system, messages, false); let mut body_value = serde_json::to_value(&anthropic_req) .map_err(|e| BridgeError::Config(format!("serialize Anthropic request body: {e}")))?; @@ -1456,7 +1456,7 @@ mod tests { fn bedrock_secret_rejects_empty() { let err = BedrockSecret::parse("").unwrap_err(); match err { - BridgeError::Config(msg) => { + BridgeError::InvalidUpstreamConfig(msg) => { assert!( msg.contains("secret is empty"), "must mention empty secret; got {msg}" @@ -1466,7 +1466,7 @@ mod tests { "must hint at required JSON shape; got {msg}" ); } - other => panic!("expected Config error, got {other:?}"), + other => panic!("expected InvalidUpstreamConfig error, got {other:?}"), } } @@ -1474,13 +1474,13 @@ mod tests { fn bedrock_secret_rejects_non_json() { let err = BedrockSecret::parse("AKIA-test").unwrap_err(); match err { - BridgeError::Config(msg) => { + BridgeError::InvalidUpstreamConfig(msg) => { assert!( msg.contains("must be valid JSON"), "must mention JSON requirement; got {msg}" ); } - other => panic!("expected Config error, got {other:?}"), + other => panic!("expected InvalidUpstreamConfig error, got {other:?}"), } } @@ -1492,7 +1492,7 @@ mod tests { let secret_with_distinctive_bytes = "X-DISTINCTIVE-LEAK-MARKER-Y"; let err = BedrockSecret::parse(secret_with_distinctive_bytes).unwrap_err(); match err { - BridgeError::Config(msg) => { + BridgeError::InvalidUpstreamConfig(msg) => { assert!( !msg.contains("X-DISTINCTIVE-LEAK-MARKER-Y"), "error must NOT echo raw secret bytes; got {msg}" @@ -1502,7 +1502,7 @@ mod tests { "error must NOT leak partial secret bytes; got {msg}" ); } - other => panic!("expected Config error, got {other:?}"), + other => panic!("expected InvalidUpstreamConfig error, got {other:?}"), } } @@ -1515,7 +1515,7 @@ mod tests { // required). let json = r#"{"access_key_id":"AKIA","secret_access_key":"sk"}"#; let err = BedrockSecret::parse(json).unwrap_err(); - assert!(matches!(err, BridgeError::Config(_))); + assert!(matches!(err, BridgeError::InvalidUpstreamConfig(_))); } // ─── Pre-dispatch validation tests ───────────────────────────────── @@ -1586,10 +1586,10 @@ mod tests { let req = ChatFormat::new("customer-facing", vec![ChatMessage::user("hi")]); let err = bridge.chat(&req, &ctx).await.unwrap_err(); match err { - BridgeError::Config(msg) => { + BridgeError::InvalidUpstreamConfig(msg) => { assert!(msg.contains("must be valid JSON")); } - other => panic!("expected Config error, got {other:?}"), + other => panic!("expected InvalidUpstreamConfig error, got {other:?}"), } } @@ -1604,10 +1604,10 @@ mod tests { let req = ChatFormat::new("customer-facing", vec![ChatMessage::user("hi")]); let err = bridge.chat(&req, &ctx).await.unwrap_err(); match err { - BridgeError::Config(msg) => { + BridgeError::InvalidUpstreamConfig(msg) => { assert!(msg.contains("secret is empty")); } - other => panic!("expected Config error, got {other:?}"), + other => panic!("expected InvalidUpstreamConfig error, got {other:?}"), } } @@ -1629,10 +1629,10 @@ mod tests { let req = ChatFormat::new("customer-facing", vec![ChatMessage::user("hi")]); let err = bridge.chat(&req, &ctx).await.unwrap_err(); match err { - BridgeError::Config(msg) => { + BridgeError::InvalidUpstreamConfig(msg) => { assert!(msg.contains("model_name missing")); } - other => panic!("expected Config error, got {other:?}"), + other => panic!("expected InvalidUpstreamConfig error, got {other:?}"), } } diff --git a/crates/aisix-provider-openai/src/bridge.rs b/crates/aisix-provider-openai/src/bridge.rs index ba1ff2c1..0dc59f73 100644 --- a/crates/aisix-provider-openai/src/bridge.rs +++ b/crates/aisix-provider-openai/src/bridge.rs @@ -129,7 +129,7 @@ impl OpenAiBridge { provider_metadata.api_base_url on the control plane; standalone: \ directly on the resource)." ); - return Err(BridgeError::Config(format!( + return Err(BridgeError::InvalidUpstreamConfig(format!( "provider_key for vendor {pk_vendor_raw:?} has no upstream base URL \ configured" ))); @@ -201,17 +201,27 @@ fn normalize_canonical_openai(base: &str) -> String { fn api_key(ctx: &BridgeContext) -> Result<&str, BridgeError> { let k = &ctx.provider_key.secret; if k.is_empty() { - Err(BridgeError::Config("provider_key.secret is empty".into())) - } else { - Ok(k.as_str()) + return Err(BridgeError::InvalidUpstreamConfig( + "provider_key.secret is empty".into(), + )); } + // Reject a secret that can't be a valid Authorization header value + // (control bytes etc.) up front as customer-fixable config. Several + // endpoints build the `Bearer {key}` header inline rather than via + // build_request_headers, so validating here covers them all (#367). + if HeaderValue::from_str(k).is_err() { + return Err(BridgeError::InvalidUpstreamConfig( + "provider_key.secret contains invalid header characters".into(), + )); + } + Ok(k.as_str()) } fn upstream_model(ctx: &BridgeContext) -> Result<&str, BridgeError> { ctx.model .model_name .as_deref() - .ok_or_else(|| BridgeError::Config("model.model_name missing".into())) + .ok_or_else(|| BridgeError::InvalidUpstreamConfig("model.model_name missing".into())) } async fn map_http_error(status: StatusCode, resp: reqwest::Response) -> BridgeError { @@ -332,8 +342,9 @@ fn build_request_headers( request: Option<&RequestOverrides>, ) -> Result { let mut headers = HeaderMap::new(); - let auth = HeaderValue::from_str(&format!("Bearer {api_key_str}")) - .map_err(|e| BridgeError::Config(format!("api key contains invalid header chars: {e}")))?; + let auth = HeaderValue::from_str(&format!("Bearer {api_key_str}")).map_err(|e| { + BridgeError::InvalidUpstreamConfig(format!("api key contains invalid header chars: {e}")) + })?; headers.insert(header::AUTHORIZATION, auth); headers.insert( header::CONTENT_TYPE, @@ -1074,7 +1085,7 @@ mod tests { let bridge = OpenAiBridge::new(); let ctx = BridgeContext::new("req-1", sample_model(), Arc::new(pk)); let err = bridge.chat(&req(), &ctx).await.unwrap_err(); - assert!(matches!(err, BridgeError::Config(_))); + assert!(matches!(err, BridgeError::InvalidUpstreamConfig(_))); } #[tokio::test] @@ -1194,7 +1205,7 @@ data: [DONE]\n\n"; let ctx = BridgeContext::new("rid", sample_model(), Arc::new(pk)); let err = bridge.resolve_base(&ctx).unwrap_err(); match err { - BridgeError::Config(msg) => { + BridgeError::InvalidUpstreamConfig(msg) => { assert!( msg.contains("base URL") && msg.contains(vendor.trim()), "vendor {vendor:?}: error must name vendor + base URL; got: {msg}", @@ -1210,7 +1221,9 @@ data: [DONE]\n\n"; ); } } - other => panic!("vendor {vendor:?}: expected BridgeError::Config, got {other:?}"), + other => { + panic!("vendor {vendor:?}: expected InvalidUpstreamConfig, got {other:?}") + } } } } diff --git a/crates/aisix-provider-vertex/src/bridge.rs b/crates/aisix-provider-vertex/src/bridge.rs index 6ef3d45f..84233a90 100644 --- a/crates/aisix-provider-vertex/src/bridge.rs +++ b/crates/aisix-provider-vertex/src/bridge.rs @@ -184,7 +184,7 @@ impl VertexBridge { } if let Some(b) = ctx_api_base.map(str::trim).filter(|s| !s.is_empty()) { if !(b.starts_with("https://") || b.starts_with("http://")) { - return Err(BridgeError::Config(format!( + return Err(BridgeError::InvalidUpstreamConfig(format!( "vertex provider_key api_base must use http:// or https:// scheme, got {b:?}", ))); } @@ -194,20 +194,20 @@ impl VertexBridge { // that operator-pasted credentials shouldn't appear in // logs. Audit #392 re-audit LOW-1. let redacted = redact_userinfo(b); - return Err(BridgeError::Config(format!( + return Err(BridgeError::InvalidUpstreamConfig(format!( "vertex provider_key api_base must not embed userinfo (@); use the request's \ Authorization header instead, got {redacted:?}", ))); } if b.contains('?') { - return Err(BridgeError::Config(format!( + return Err(BridgeError::InvalidUpstreamConfig(format!( "vertex provider_key api_base must not contain a query string (the bridge \ appends `?alt=sse` on streaming; an operator query would silently merge), \ got {b:?}", ))); } if b.contains('#') { - return Err(BridgeError::Config(format!( + return Err(BridgeError::InvalidUpstreamConfig(format!( "vertex provider_key api_base must not contain a fragment, got {b:?}", ))); } @@ -226,7 +226,7 @@ impl VertexBridge { .unwrap_or(b) .trim_end_matches('/'); if after_scheme.contains('/') || after_scheme.contains('\\') { - return Err(BridgeError::Config(format!( + return Err(BridgeError::InvalidUpstreamConfig(format!( "vertex provider_key api_base must be a bare origin \ (scheme://host[:port]) with no path, got {b:?}", ))); @@ -472,7 +472,7 @@ fn upstream_model(ctx: &BridgeContext) -> Result<&str, BridgeError> { ctx.model .model_name .as_deref() - .ok_or_else(|| BridgeError::Config("model.model_name missing".into())) + .ok_or_else(|| BridgeError::InvalidUpstreamConfig("model.model_name missing".into())) } /// Redact embedded userinfo from a URL string before echoing it @@ -773,7 +773,7 @@ impl VertexBridge { // Vertex `anthropic_version`). Mirrors the Bedrock `/invoke` // body shaping, differing only in the version string. let (system, messages) = - split_system(req).map_err(|e| BridgeError::Config(format!("{e}")))?; + split_system(req).map_err(|e| BridgeError::InvalidUpstreamConfig(format!("{e}")))?; let anthropic_req = build_anthropic_request(req, upstream_id, system, messages, false); let mut body_value = serde_json::to_value(&anthropic_req) .map_err(|e| BridgeError::Config(format!("serialize Anthropic request body: {e}")))?; @@ -861,7 +861,7 @@ impl VertexBridge { // in the body (only `model` is stripped into the URL). Add the // Vertex `anthropic_version`. let (system, messages) = - split_system(req).map_err(|e| BridgeError::Config(format!("{e}")))?; + split_system(req).map_err(|e| BridgeError::InvalidUpstreamConfig(format!("{e}")))?; let anthropic_req = build_anthropic_request(req, upstream_id, system, messages, true); let mut body_value = serde_json::to_value(&anthropic_req) .map_err(|e| BridgeError::Config(format!("serialize Anthropic request body: {e}")))?; @@ -1994,13 +1994,15 @@ mod tests { .err() .unwrap_or_else(|| panic!("expected error for api_base={bad:?}")); match err { - BridgeError::Config(msg) => { + BridgeError::InvalidUpstreamConfig(msg) => { assert!( msg.contains("http"), "error message should name the http(s) requirement; got: {msg}" ); } - other => panic!("expected Config error for api_base={bad:?}, got {other:?}"), + other => panic!( + "expected InvalidUpstreamConfig error for api_base={bad:?}, got {other:?}" + ), } } } @@ -2018,7 +2020,7 @@ mod tests { .err() .unwrap(); match err { - BridgeError::Config(msg) => { + BridgeError::InvalidUpstreamConfig(msg) => { assert!(msg.contains("userinfo") || msg.contains("@")); // Defense-in-depth: the error message MUST NOT echo // the original userinfo back into log output (re-audit @@ -2034,7 +2036,7 @@ mod tests { "error message should redact userinfo: {msg}" ); } - other => panic!("expected Config error, got {other:?}"), + other => panic!("expected InvalidUpstreamConfig error, got {other:?}"), } } @@ -2082,10 +2084,10 @@ mod tests { .err() .unwrap(); match err { - BridgeError::Config(msg) => { + BridgeError::InvalidUpstreamConfig(msg) => { assert!(msg.contains("query") || msg.contains('?')); } - other => panic!("expected Config error, got {other:?}"), + other => panic!("expected InvalidUpstreamConfig error, got {other:?}"), } } @@ -2097,10 +2099,10 @@ mod tests { .err() .unwrap(); match err { - BridgeError::Config(msg) => { + BridgeError::InvalidUpstreamConfig(msg) => { assert!(msg.contains("fragment") || msg.contains('#')); } - other => panic!("expected Config error, got {other:?}"), + other => panic!("expected InvalidUpstreamConfig error, got {other:?}"), } } @@ -2115,13 +2117,13 @@ mod tests { .err() .unwrap(); match err { - BridgeError::Config(msg) => { + BridgeError::InvalidUpstreamConfig(msg) => { assert!( msg.contains("bare origin") && msg.contains("no path"), "expected a bare-origin/no-path rejection; got {msg}" ); } - other => panic!("expected Config error, got {other:?}"), + other => panic!("expected InvalidUpstreamConfig error, got {other:?}"), } } @@ -2147,13 +2149,13 @@ mod tests { .err() .unwrap(); match err { - BridgeError::Config(msg) => { + BridgeError::InvalidUpstreamConfig(msg) => { assert!( msg.contains("bare origin") && msg.contains("no path"), "expected a bare-origin/no-path rejection; got {msg}" ); } - other => panic!("expected Config error, got {other:?}"), + other => panic!("expected InvalidUpstreamConfig error, got {other:?}"), } } @@ -2624,10 +2626,10 @@ mod tests { let req = ChatFormat::new("customer-facing", vec![ChatMessage::user("hi")]); let err = bridge.chat(&req, &ctx).await.unwrap_err(); match err { - BridgeError::Config(msg) => { + BridgeError::InvalidUpstreamConfig(msg) => { assert!(msg.contains("model_name missing")); } - other => panic!("expected Config error, got {other:?}"), + other => panic!("expected InvalidUpstreamConfig error, got {other:?}"), } } diff --git a/crates/aisix-provider-vertex/src/token_mint.rs b/crates/aisix-provider-vertex/src/token_mint.rs index dcf36d65..594efda5 100644 --- a/crates/aisix-provider-vertex/src/token_mint.rs +++ b/crates/aisix-provider-vertex/src/token_mint.rs @@ -76,25 +76,25 @@ impl ServiceAccountKey { /// the first mint will catch a malformed key with a clear message. pub fn validate(&self) -> Result<(), BridgeError> { if self.typ != "service_account" { - return Err(BridgeError::Config(format!( + return Err(BridgeError::InvalidUpstreamConfig(format!( "vertex service_account_json.type = {:?}, want \"service_account\"", self.typ ))); } if !self.private_key.starts_with("-----BEGIN") { - return Err(BridgeError::Config( + return Err(BridgeError::InvalidUpstreamConfig( "vertex service_account_json.private_key is not PEM-formatted \ (expected `-----BEGIN PRIVATE KEY-----` or `-----BEGIN RSA PRIVATE KEY-----`)" .into(), )); } if self.client_email.is_empty() { - return Err(BridgeError::Config( + return Err(BridgeError::InvalidUpstreamConfig( "vertex service_account_json.client_email is empty".into(), )); } if self.token_uri.is_empty() { - return Err(BridgeError::Config( + return Err(BridgeError::InvalidUpstreamConfig( "vertex service_account_json.token_uri is empty".into(), )); } @@ -450,10 +450,10 @@ mod tests { let minter = TokenMinter::new(Client::new()).with_token_endpoint_override(server.uri()); let err = minter.get_token(&sa).await.err().unwrap(); match err { - BridgeError::Config(msg) => { + BridgeError::InvalidUpstreamConfig(msg) => { assert!(msg.contains("not PEM-formatted")); } - other => panic!("expected Config error, got {other:?}"), + other => panic!("expected InvalidUpstreamConfig error, got {other:?}"), } } @@ -475,12 +475,12 @@ mod tests { let minter = TokenMinter::new(Client::new()).with_token_endpoint_override(server.uri()); let err = minter.get_token(&sa).await.err().unwrap(); match err { - BridgeError::Config(msg) => { + BridgeError::InvalidUpstreamConfig(msg) => { assert!(msg.contains("type")); assert!(msg.contains("external_account")); assert!(msg.contains("service_account")); } - other => panic!("expected Config error, got {other:?}"), + other => panic!("expected InvalidUpstreamConfig error, got {other:?}"), } } diff --git a/crates/aisix-proxy/src/chat.rs b/crates/aisix-proxy/src/chat.rs index e3517fa0..e0628ec0 100644 --- a/crates/aisix-proxy/src/chat.rs +++ b/crates/aisix-proxy/src/chat.rs @@ -92,6 +92,7 @@ fn routing_error_class(err: &BridgeError) -> &'static str { BridgeError::UpstreamStatus { .. } => "upstream_status", BridgeError::UpstreamDecode(_) => "upstream_decode", BridgeError::Config(_) => "config", + BridgeError::InvalidUpstreamConfig(_) => "invalid_config", BridgeError::Transport(_) => "transport", BridgeError::StreamAborted => "stream_aborted", } diff --git a/crates/aisix-proxy/src/routing.rs b/crates/aisix-proxy/src/routing.rs index 7281c4b3..c9212eff 100644 --- a/crates/aisix-proxy/src/routing.rs +++ b/crates/aisix-proxy/src/routing.rs @@ -44,6 +44,9 @@ pub fn is_retryable(err: &BridgeError, retry_on_429: bool) -> bool { } !(400..500).contains(status) } + // Customer-fixable config (#367) is the caller's mistake — + // retrying or failing over won't help, same as a non-429 4xx. + BridgeError::InvalidUpstreamConfig(_) => false, BridgeError::Timeout { .. } | BridgeError::Transport(_) | BridgeError::UpstreamDecode(_) @@ -591,6 +594,11 @@ mod tests { )); assert!(is_retryable(&BridgeError::Config("bad key".into()), false)); assert!(is_retryable(&BridgeError::StreamAborted, false)); + // #367: customer-fixable config is a 4xx — not retryable. + assert!(!is_retryable( + &BridgeError::InvalidUpstreamConfig("no api_base".into()), + false + )); } // ── filter_attempt_models ───────────────────────────────────── diff --git a/tests/e2e/src/cases/invalid-upstream-config-400-e2e.test.ts b/tests/e2e/src/cases/invalid-upstream-config-400-e2e.test.ts new file mode 100644 index 00000000..4ccb8d59 --- /dev/null +++ b/tests/e2e/src/cases/invalid-upstream-config-400-e2e.test.ts @@ -0,0 +1,107 @@ +import { createHash } from "node:crypto"; +import OpenAI, { APIError } from "openai"; +import { afterAll, beforeAll, describe, expect, test } from "vitest"; +import { + AdminClient, + EtcdClient, + ProxyClient, + spawnApp, + startOpenAiUpstream, + waitConfigPropagation, + type OpenAiUpstream, + type SpawnedApp, +} from "../harness/index.js"; + +// E2E: a customer-fixable upstream-config error must surface to the +// client as a 400, not a 500 (#367). The scenario is the issue's own +// example — a family-adapter vendor (openrouter) admitted without an +// api_base: the family bridge refuses to fall back to api.openai.com +// and errors before dispatch. A 5xx would tell SDKs to retry and +// monitoring to alert on a server fault, when the fix is on the +// operator's side (populate api_base on the ProviderKey). + +const CALLER_PLAINTEXT = "sk-invalid-config-400"; +const CALLER_KEY_HASH = createHash("sha256") + .update(CALLER_PLAINTEXT) + .digest("hex"); + +describe("invalid upstream config maps to 400 e2e", () => { + let app: SpawnedApp | undefined; + let upstream: OpenAiUpstream | undefined; + let admin: AdminClient | undefined; + let etcdReachable = false; + + beforeAll(async () => { + etcdReachable = await new EtcdClient().ping(); + if (!etcdReachable) return; + + upstream = await startOpenAiUpstream(); + app = await spawnApp(); + admin = new AdminClient(app.adminUrl, app.adminKey); + + // openrouter resolves via the openai adapter family. With no + // api_base the family bridge refuses to fall back to + // api.openai.com — a customer-fixable misconfig. (No api_base set.) + const pk = await admin.createProviderKey({ + display_name: "invalid-config-pk", + provider: "openrouter", + adapter: "openai", + secret: "sk-mock", + }); + await admin.createModel({ + display_name: "invalid-config-model", + provider: "openrouter", + model_name: "gpt-4o-mini", + provider_key_id: pk.id, + }); + await admin.createApiKey({ + key_hash: CALLER_KEY_HASH, + allowed_models: ["invalid-config-model"], + }); + }); + + afterAll(async () => { + await app?.exit(); + await upstream?.close(); + }); + + test("family-adapter PK without api_base surfaces as a 400, not a 500", async (ctx) => { + if (!etcdReachable || !app) { + ctx.skip(); + return; + } + + const probe = new ProxyClient(app.proxyUrl, CALLER_PLAINTEXT); + await waitConfigPropagation(async () => { + const res = await probe.listModels(); + if (res.status !== 200) return false; + const data = (res.body as { data?: Array<{ id?: string }> }).data ?? []; + return data.some((m) => m.id === "invalid-config-model"); + }); + + const client = new OpenAI({ + apiKey: CALLER_PLAINTEXT, + baseURL: `${app.proxyUrl}/v1`, + maxRetries: 0, + }); + + let caught: unknown; + try { + await client.chat.completions.create({ + model: "invalid-config-model", + messages: [{ role: "user", content: "hi" }], + }); + } catch (e) { + caught = e; + } + expect(caught).toBeInstanceOf(APIError); + if (!(caught instanceof APIError)) { + throw new Error("unreachable: caught is not APIError"); + } + // The load-bearing assertion: customer-fixable config is a 4xx. + expect(caught.status).toBe(400); + expect((caught.error as { type?: string } | undefined)?.type).toBe( + "invalid_request_error", + ); + }); +});