Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
80 changes: 80 additions & 0 deletions crates/aisix-provider-azure-openai/src/aad_token_mint.rs
Original file line number Diff line number Diff line change
Expand Up @@ -150,6 +150,26 @@ 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. 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('/') || 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(())
}
Expand Down Expand Up @@ -617,6 +637,66 @@ 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_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 {
Expand Down
73 changes: 73 additions & 0 deletions crates/aisix-provider-vertex/src/bridge.rs
Original file line number Diff line number Diff line change
Expand Up @@ -211,6 +211,26 @@ 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. 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('/') || 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"))
Expand Down Expand Up @@ -2084,6 +2104,59 @@ 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 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!(
Expand Down
Loading