From eb333d5d008141fb5c6772b4c5caa117c7750253 Mon Sep 17 00:00:00 2001 From: Ming Wen Date: Mon, 1 Jun 2026 11:08:28 +0800 Subject: [PATCH 1/2] fix(vertex,azure): reject embedded path in authority/api_base overrides (#435) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Both platform-adapter bridges accept an operator-supplied origin override for an auth/data endpoint and already reject scheme / userinfo / query / fragment, but neither rejected an embedded path component: - Vertex `resolve_api_base`: an api_base with a trailing path segment was accepted verbatim, silently redirecting every upstream call. - Azure `AadCredentials.authority_host`: same gap — a path segment would redirect the token endpoint (e.g. `.../evil/{tenant}/oauth2/v2.0/token`). Add a path-rejection check after the existing scheme check on both, so only `scheme://host[:port]` is accepted. The check trims a trailing slash before testing for a path segment, so a bare trailing slash stays tolerated (Vertex already trims it on return; Azure's resolve_token_endpoint trims it at URL-build time). It runs AFTER the userinfo/query/fragment rejections, so the error never echoes pasted `user:pass@host` credentials. Surfaced by the #434 independent audit (LOW-1); filed separately to keep the two validation paths symmetric. Tests: per bridge, a rejection test (`https://host/evil`) + a bare-origin allow test (`host:port`, trailing slash) guarding against false-positives on the `:port` colon. --- .../src/aad_token_mint.rs | 56 +++++++++++++++++++ crates/aisix-provider-vertex/src/bridge.rs | 49 ++++++++++++++++ 2 files changed, 105 insertions(+) diff --git a/crates/aisix-provider-azure-openai/src/aad_token_mint.rs b/crates/aisix-provider-azure-openai/src/aad_token_mint.rs index 84a360c6..aeace29a 100644 --- a/crates/aisix-provider-azure-openai/src/aad_token_mint.rs +++ b/crates/aisix-provider-azure-openai/src/aad_token_mint.rs @@ -150,6 +150,23 @@ impl AadCredentials { scheme, got {host:?}" ))); } + // Reject an embedded path component — only `scheme://host[:port]` + // is a valid origin. A real path segment would silently redirect + // the token endpoint (e.g. `.../evil/{tenant}/oauth2/v2.0/token`). + // A bare trailing slash is tolerated for symmetry with the Vertex + // `resolve_api_base` check. `host` has no `@`/`?`/`#` here, so + // echoing it is safe. Audit #434 LOW-1 / #435. + let after_scheme = host + .split_once("://") + .map(|(_, rest)| rest) + .unwrap_or(host) + .trim_end_matches('/'); + if after_scheme.contains('/') { + return Err(BridgeError::Config(format!( + "azure aad credentials.authority_host must be a bare origin \ + (scheme://host[:port]) with no path, got {host:?}" + ))); + } } Ok(()) } @@ -617,6 +634,45 @@ mod tests { } } + #[test] + fn validate_rejects_authority_host_with_embedded_path() { + // #435: a path segment would silently redirect the token endpoint + // (e.g. `.../evil/{tenant}/oauth2/v2.0/token`) — reject it. + let creds = AadCredentials { + tenant_id: "t".into(), + client_id: "app".into(), + client_secret: "s".into(), + authority_host: Some("https://login.microsoftonline.us/evil".into()), + }; + let err = creds.validate().err().unwrap(); + match err { + BridgeError::Config(msg) => assert!( + msg.contains("bare origin") && msg.contains("no path"), + "expected a bare-origin/no-path rejection; got {msg}" + ), + other => panic!("expected Config, got {other:?}"), + } + } + + #[test] + fn validate_allows_authority_host_bare_origin_with_port() { + // A `host:port` origin (no path) must still validate — the path + // rejection must not false-positive on the `:port` colon, and a + // bare trailing slash is tolerated (trimmed at URL-build time). + for host in [ + "https://login.microsoftonline.us:8443", + "https://login.microsoftonline.us/", + ] { + let creds = AadCredentials { + tenant_id: "t".into(), + client_id: "app".into(), + client_secret: "s".into(), + authority_host: Some(host.into()), + }; + assert!(creds.validate().is_ok(), "{host} should validate"); + } + } + #[test] fn validate_rejects_authority_host_with_userinfo_without_echoing_it() { let creds = AadCredentials { diff --git a/crates/aisix-provider-vertex/src/bridge.rs b/crates/aisix-provider-vertex/src/bridge.rs index 24c7f002..2b38f302 100644 --- a/crates/aisix-provider-vertex/src/bridge.rs +++ b/crates/aisix-provider-vertex/src/bridge.rs @@ -211,6 +211,23 @@ impl VertexBridge { "vertex provider_key api_base must not contain a fragment, got {b:?}", ))); } + // Reject an embedded path component — only `scheme://host[:port]` + // is a valid origin. A bare trailing slash is fine (trimmed + // below); a real path segment (e.g. `.../evil`) would silently + // redirect every upstream call onto the wrong path, so fail fast + // with a clear Config error. `b` has no `@`/`?`/`#` at this point + // (rejected above), so echoing it is safe. Audit #434 LOW-1 / #435. + let after_scheme = b + .split_once("://") + .map(|(_, rest)| rest) + .unwrap_or(b) + .trim_end_matches('/'); + if after_scheme.contains('/') { + return Err(BridgeError::Config(format!( + "vertex provider_key api_base must be a bare origin \ + (scheme://host[:port]) with no path, got {b:?}", + ))); + } return Ok(b.trim_end_matches('/').to_string()); } Ok(format!("https://{region}-aiplatform.googleapis.com")) @@ -2084,6 +2101,38 @@ mod tests { } } + #[test] + fn resolve_api_base_rejects_embedded_path() { + // #435: an api_base with a real path segment would silently redirect + // every upstream call onto the wrong path — reject it with a clear + // Config error rather than 404-ing the operator later. + let bridge = VertexBridge::new(); + let err = bridge + .resolve_api_base("us-central1", Some("https://proxy.internal/evil")) + .err() + .unwrap(); + match err { + BridgeError::Config(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:?}"), + } + } + + #[test] + fn resolve_api_base_allows_bare_origin_with_port() { + // The path rejection must not false-positive on a `:port` origin — + // `host:port` has no `/` after the scheme. + let bridge = VertexBridge::new(); + let resolved = bridge + .resolve_api_base("us-central1", Some("https://proxy.internal:8443")) + .unwrap(); + assert_eq!(resolved, "https://proxy.internal:8443"); + } + #[test] fn publisher_case_insensitive_on_model_name() { assert_eq!( From e74aaed75a0a06d41d86f7c526b47e3014d06368 Mon Sep 17 00:00:00 2001 From: Ming Wen Date: Mon, 1 Jun 2026 11:17:52 +0800 Subject: [PATCH 2/2] fix(vertex,azure): also reject backslash path-injection in origin overrides (#464 audit MEDIUM) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The embedded-path check tested only for '/', but the WHATWG URL parser the HTTP client (reqwest/url) uses normalizes '\' to '/' on http(s) URLs, so `https://host\evil` survived validation yet composed an upstream URL with a `/evil` path prefix — the exact silent-redirect outcome #435 set out to block. Reject '\' alongside '/' in both resolve_api_base (Vertex) and AadCredentials::validate (Azure). Adds a backslash rejection test per bridge. --- .../src/aad_token_mint.rs | 30 +++++++++++++++++-- crates/aisix-provider-vertex/src/bridge.rs | 30 +++++++++++++++++-- 2 files changed, 54 insertions(+), 6 deletions(-) diff --git a/crates/aisix-provider-azure-openai/src/aad_token_mint.rs b/crates/aisix-provider-azure-openai/src/aad_token_mint.rs index aeace29a..7d4e655f 100644 --- a/crates/aisix-provider-azure-openai/src/aad_token_mint.rs +++ b/crates/aisix-provider-azure-openai/src/aad_token_mint.rs @@ -154,14 +154,17 @@ impl AadCredentials { // is a valid origin. A real path segment would silently redirect // the token endpoint (e.g. `.../evil/{tenant}/oauth2/v2.0/token`). // A bare trailing slash is tolerated for symmetry with the Vertex - // `resolve_api_base` check. `host` has no `@`/`?`/`#` here, so - // echoing it is safe. Audit #434 LOW-1 / #435. + // `resolve_api_base` check. Backslashes are rejected too: the + // WHATWG URL parser the HTTP client uses normalizes `\` to `/` on + // http(s) URLs, so `host\evil` injects a path exactly like + // `host/evil`. `host` has no `@`/`?`/`#` here, so echoing it is + // safe. Audit #434 LOW-1 / #435 (+ #464 audit MEDIUM). let after_scheme = host .split_once("://") .map(|(_, rest)| rest) .unwrap_or(host) .trim_end_matches('/'); - if after_scheme.contains('/') { + if after_scheme.contains('/') || after_scheme.contains('\\') { return Err(BridgeError::Config(format!( "azure aad credentials.authority_host must be a bare origin \ (scheme://host[:port]) with no path, got {host:?}" @@ -673,6 +676,27 @@ mod tests { } } + #[test] + fn validate_rejects_authority_host_with_backslash_path() { + // #464 audit: the WHATWG URL parser the HTTP client uses normalizes + // `\` to `/` on http(s) URLs, so `host\evil` injects a path just like + // `host/evil` — it must be rejected the same way. + let creds = AadCredentials { + tenant_id: "t".into(), + client_id: "app".into(), + client_secret: "s".into(), + authority_host: Some("https://login.microsoftonline.us\\evil".into()), + }; + let err = creds.validate().err().unwrap(); + match err { + BridgeError::Config(msg) => assert!( + msg.contains("bare origin") && msg.contains("no path"), + "expected a bare-origin/no-path rejection; got {msg}" + ), + other => panic!("expected Config, got {other:?}"), + } + } + #[test] fn validate_rejects_authority_host_with_userinfo_without_echoing_it() { let creds = AadCredentials { diff --git a/crates/aisix-provider-vertex/src/bridge.rs b/crates/aisix-provider-vertex/src/bridge.rs index 2b38f302..6ef3d45f 100644 --- a/crates/aisix-provider-vertex/src/bridge.rs +++ b/crates/aisix-provider-vertex/src/bridge.rs @@ -215,14 +215,17 @@ impl VertexBridge { // is a valid origin. A bare trailing slash is fine (trimmed // below); a real path segment (e.g. `.../evil`) would silently // redirect every upstream call onto the wrong path, so fail fast - // with a clear Config error. `b` has no `@`/`?`/`#` at this point - // (rejected above), so echoing it is safe. Audit #434 LOW-1 / #435. + // with a clear Config error. Backslashes are rejected too: the + // WHATWG URL parser the HTTP client uses normalizes `\` to `/` on + // http(s) URLs, so `host\evil` injects a path exactly like + // `host/evil`. `b` has no `@`/`?`/`#` here (rejected above), so + // echoing it is safe. Audit #434 LOW-1 / #435 (+ #464 audit MEDIUM). let after_scheme = b .split_once("://") .map(|(_, rest)| rest) .unwrap_or(b) .trim_end_matches('/'); - if after_scheme.contains('/') { + if after_scheme.contains('/') || after_scheme.contains('\\') { return Err(BridgeError::Config(format!( "vertex provider_key api_base must be a bare origin \ (scheme://host[:port]) with no path, got {b:?}", @@ -2133,6 +2136,27 @@ mod tests { assert_eq!(resolved, "https://proxy.internal:8443"); } + #[test] + fn resolve_api_base_rejects_backslash_path() { + // #464 audit: the WHATWG URL parser the HTTP client uses normalizes + // `\` to `/` on http(s) URLs, so `host\evil` injects a path just like + // `host/evil` — it must be rejected the same way. + let bridge = VertexBridge::new(); + let err = bridge + .resolve_api_base("us-central1", Some("https://proxy.internal\\evil")) + .err() + .unwrap(); + match err { + BridgeError::Config(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:?}"), + } + } + #[test] fn publisher_case_insensitive_on_model_name() { assert_eq!(