diff --git a/crates/aisix-proxy/src/a2a.rs b/crates/aisix-proxy/src/a2a.rs index 3f95aaa3..37c5805e 100644 --- a/crates/aisix-proxy/src/a2a.rs +++ b/crates/aisix-proxy/src/a2a.rs @@ -24,12 +24,13 @@ use std::time::{Duration, Instant}; use aisix_a2a::{upstream_from_a2a_agent, A2aBridge, A2aError, HttpBridge}; use aisix_obs::{AccessLog, RequestOutcome, UsageEvent}; use axum::body::to_bytes; -use axum::extract::{Path, Request, State}; +use axum::extract::{Request, State}; use axum::http::{header, HeaderMap, StatusCode}; use axum::response::{IntoResponse, Response}; use serde::Deserialize; use crate::auth::AuthenticatedKey; +use crate::reject::AisixPath; use crate::request_id::new_request_id; use crate::state::ProxyState; @@ -52,7 +53,7 @@ struct JsonRpcPeek { /// emitted either way. pub async fn a2a_endpoint( auth: AuthenticatedKey, - Path(agent): Path, + AisixPath(agent): AisixPath, State(state): State, request: Request, ) -> Response { @@ -216,7 +217,7 @@ async fn dispatch( /// callers discover the agent through `/a2a/`. pub async fn a2a_agent_card( auth: AuthenticatedKey, - Path(agent): Path, + AisixPath(agent): AisixPath, State(state): State, headers: HeaderMap, ) -> Response { diff --git a/crates/aisix-proxy/src/audio.rs b/crates/aisix-proxy/src/audio.rs index d18aa701..2ba576ee 100644 --- a/crates/aisix-proxy/src/audio.rs +++ b/crates/aisix-proxy/src/audio.rs @@ -86,12 +86,31 @@ pub async fn transcriptions( State(state): State, auth: AuthenticatedKey, client: ClientContext, - multipart: Multipart, + multipart: Result, ) -> Response { let started = Instant::now(); let request_id = client.request_id.clone(); let api_key_id = auth.entry.id.clone(); + // Same silent class as the body-extractor rejections #863 collected: a + // non-multipart content-type answered axum's bare 400 with no access + // log, metrics, or envelope. + let multipart = match multipart { + Ok(multipart) => multipart, + Err(_) => { + return crate::reject::reject_before_dispatch( + &state, + "POST", + "/v1/audio/transcriptions", + &request_id, + Some(&api_key_id), + started, + crate::reject::Envelope::OpenAi, + ProxyError::InvalidRequest("invalid multipart form data".into()), + ); + } + }; + match multipart_dispatch( &state, &auth, @@ -186,12 +205,29 @@ pub async fn translations( State(state): State, auth: AuthenticatedKey, client: ClientContext, - multipart: Multipart, + multipart: Result, ) -> Response { let started = Instant::now(); let request_id = client.request_id.clone(); let api_key_id = auth.entry.id.clone(); + // See `transcriptions`: the rejection is recorded, not silently bare. + let multipart = match multipart { + Ok(multipart) => multipart, + Err(_) => { + return crate::reject::reject_before_dispatch( + &state, + "POST", + "/v1/audio/translations", + &request_id, + Some(&api_key_id), + started, + crate::reject::Envelope::OpenAi, + ProxyError::InvalidRequest("invalid multipart form data".into()), + ); + } + }; + match multipart_dispatch( &state, &auth, diff --git a/crates/aisix-proxy/src/jobs.rs b/crates/aisix-proxy/src/jobs.rs index 4e215ecd..1777f1ff 100644 --- a/crates/aisix-proxy/src/jobs.rs +++ b/crates/aisix-proxy/src/jobs.rs @@ -55,7 +55,7 @@ use aisix_core::resource::ResourceEntry; use aisix_core::{Model, ProviderKey}; use aisix_obs::{AccessLog, RequestOutcome, UsageEvent}; use axum::body::Body; -use axum::extract::{Multipart, Path, Query, State}; +use axum::extract::{Multipart, Query, State}; use axum::http::{header, HeaderMap, HeaderValue, Method, StatusCode}; use axum::response::{IntoResponse, Response}; use base64::engine::general_purpose::URL_SAFE_NO_PAD; @@ -66,6 +66,7 @@ use serde_json::Value; use crate::auth::AuthenticatedKey; use crate::client_ip::ClientContext; use crate::error::ProxyError; +use crate::reject::AisixPath; use crate::state::ProxyState; /// Marker prefix for gateway-minted routed ids. @@ -776,10 +777,29 @@ pub(crate) async fn create_file( client: ClientContext, Query(params): Query>, headers: HeaderMap, - mut multipart: Multipart, + multipart: Result, ) -> Response { let started = Instant::now(); let request_id = client.request_id.clone(); + + // Same silent class as the body-extractor rejections #863 collected: a + // non-multipart content-type answered axum's bare 400 with no access + // log, metrics, or envelope. + let mut multipart = match multipart { + Ok(multipart) => multipart, + Err(_) => { + return crate::reject::reject_before_dispatch( + &state, + "POST", + "/v1/files", + &request_id, + Some(&auth.entry.id), + started, + crate::reject::Envelope::OpenAi, + ProxyError::InvalidRequest("invalid multipart form data".into()), + ); + } + }; let mut monitor_hits: Vec = Vec::new(); let result = async { @@ -928,7 +948,7 @@ pub(crate) async fn get_file( State(state): State, auth: AuthenticatedKey, client: ClientContext, - Path(id): Path, + AisixPath(id): AisixPath, Query(params): Query>, headers: HeaderMap, ) -> Response { @@ -958,7 +978,7 @@ pub(crate) async fn delete_file( State(state): State, auth: AuthenticatedKey, client: ClientContext, - Path(id): Path, + AisixPath(id): AisixPath, Query(params): Query>, headers: HeaderMap, ) -> Response { @@ -988,7 +1008,7 @@ pub(crate) async fn file_content( State(state): State, auth: AuthenticatedKey, client: ClientContext, - Path(id): Path, + AisixPath(id): AisixPath, Query(params): Query>, headers: HeaderMap, ) -> Response { @@ -1135,7 +1155,7 @@ pub(crate) async fn get_batch( State(state): State, auth: AuthenticatedKey, client: ClientContext, - Path(id): Path, + AisixPath(id): AisixPath, Query(params): Query>, headers: HeaderMap, ) -> Response { @@ -1205,7 +1225,7 @@ pub(crate) async fn cancel_batch( State(state): State, auth: AuthenticatedKey, client: ClientContext, - Path(id): Path, + AisixPath(id): AisixPath, Query(params): Query>, headers: HeaderMap, ) -> Response { @@ -1378,7 +1398,7 @@ pub(crate) async fn get_ft_job( State(state): State, auth: AuthenticatedKey, client: ClientContext, - Path(id): Path, + AisixPath(id): AisixPath, Query(params): Query>, headers: HeaderMap, ) -> Response { @@ -1408,7 +1428,7 @@ pub(crate) async fn cancel_ft_job( State(state): State, auth: AuthenticatedKey, client: ClientContext, - Path(id): Path, + AisixPath(id): AisixPath, Query(params): Query>, headers: HeaderMap, ) -> Response { diff --git a/crates/aisix-proxy/src/mcp.rs b/crates/aisix-proxy/src/mcp.rs index 6c1a7a9d..2d0e8a30 100644 --- a/crates/aisix-proxy/src/mcp.rs +++ b/crates/aisix-proxy/src/mcp.rs @@ -78,7 +78,7 @@ pub async fn mcp_endpoint( /// nothing about which servers exist). pub async fn mcp_scoped_endpoint( auth: AuthenticatedKey, - axum::extract::Path(server): axum::extract::Path, + crate::reject::AisixPath(server): crate::reject::AisixPath, State(state): State, request: Request, ) -> Response { diff --git a/crates/aisix-proxy/src/passthrough.rs b/crates/aisix-proxy/src/passthrough.rs index 1e822137..1d2ed83e 100644 --- a/crates/aisix-proxy/src/passthrough.rs +++ b/crates/aisix-proxy/src/passthrough.rs @@ -24,7 +24,7 @@ use aisix_obs::{AccessLog, RequestOutcome}; use axum::body::Body; -use axum::extract::{Path, Request, State}; +use axum::extract::{Request, State}; use axum::http::{header, HeaderMap, HeaderValue, Method}; use axum::response::{IntoResponse, Response}; use bytes::Bytes; @@ -32,6 +32,7 @@ use std::time::{Duration, Instant}; use crate::auth::AuthenticatedKey; use crate::error::ProxyError; +use crate::reject::AisixPath; use crate::state::ProxyState; /// Bounded `model` metric label for passthrough requests. The wildcard @@ -131,7 +132,7 @@ pub async fn passthrough( State(state): State, auth: AuthenticatedKey, client: crate::client_ip::ClientContext, - Path((provider, rest)): Path<(String, String)>, + AisixPath((provider, rest)): AisixPath<(String, String)>, req: Request, ) -> Response { let started = Instant::now(); diff --git a/crates/aisix-proxy/src/reject.rs b/crates/aisix-proxy/src/reject.rs index 36fcde3d..b4029d18 100644 --- a/crates/aisix-proxy/src/reject.rs +++ b/crates/aisix-proxy/src/reject.rs @@ -14,12 +14,18 @@ //! the family can't drift again: the rendered envelope and the telemetry are //! produced by the same call. +use std::sync::Arc; use std::time::Instant; +use aisix_core::{ApiKey, ResourceEntry}; use aisix_obs::{AccessLog, RequestOutcome}; +use axum::extract::FromRequestParts; +use axum::http::request::Parts; use axum::response::{IntoResponse, Response}; +use serde::de::DeserializeOwned; use crate::error::ProxyError; +use crate::request_id::{new_request_id, RequestId}; use crate::state::ProxyState; use crate::usage_attr::UNRESOLVED_MODEL_LABEL; @@ -91,3 +97,241 @@ pub(crate) fn reject_before_dispatch( Envelope::Anthropic => err.into_anthropic_response(), } } + +/// `axum::extract::Path` with the rejection routed through +/// [`reject_before_dispatch`]. +/// +/// A `:param` segment that fails extraction — invalid percent-encoding such +/// as `/v1/files/%ff` — otherwise answers axum's bare 400: no access log, +/// no request metrics, no caller envelope (#880, the same silent class +/// #863 collected for body rejections). Every handler on a `:param` route +/// takes this instead of `Path`, so the family can't drift back. +/// +/// Declared after `auth: AuthenticatedKey` in handler signatures, like +/// `Path` was — extractors run in order, so authentication still precedes +/// the path parse (an unauthenticated caller gets 401, not a 400 that +/// confirms anything about the route) and the resolved key published by the +/// auth extractor attributes the rejection. +pub(crate) struct AisixPath(pub(crate) T); + +#[axum::async_trait] +impl FromRequestParts for AisixPath +where + T: DeserializeOwned + Send + 'static, +{ + type Rejection = Response; + + async fn from_request_parts( + parts: &mut Parts, + state: &ProxyState, + ) -> Result { + match axum::extract::Path::::from_request_parts(parts, state).await { + Ok(axum::extract::Path(value)) => Ok(Self(value)), + Err(rejection) => { + // axum classifies wrong parameter arity / unsupported types + // as 500 — a server wiring bug, not caller input. Keep that + // loud and unenveloped; only caller-caused 400s are recorded + // as refused requests below. + let axum_response = rejection.into_response(); + if axum_response.status() != axum::http::StatusCode::BAD_REQUEST { + return Err(axum_response); + } + // Fallback mirrors the handlers' own idiom (see mcp.rs / + // a2a.rs); unreachable in the real router, where + // `ensure_request_id` runs before routing. + let request_id = parts + .extensions + .get::() + .map(|r| r.0.clone()) + .unwrap_or_else(new_request_id); + let api_key_id = parts + .extensions + .get::>>() + .map(|entry| entry.id.clone()); + // The raw path, not a route template: the malformed segment + // IS the subject of this rejection, and the access log is + // per-request (the bounded labels live in the metrics). + Err(reject_before_dispatch( + state, + parts.method.as_str(), + parts.uri.path(), + &request_id, + api_key_id.as_deref(), + Instant::now(), + Envelope::OpenAi, + ProxyError::InvalidRequest("invalid path parameter".into()), + )) + } + } + } +} + +#[cfg(test)] +mod tests { + use aisix_core::snapshot::SnapshotHandle; + use aisix_core::{AisixSnapshot, ApiKey, ProxyConfig, ResourceEntry}; + use axum::body::Body; + use axum::http::{Request as HttpRequest, StatusCode}; + use std::sync::Arc; + use tower::ServiceExt; + + use crate::state::ProxyState; + + const TOKEN: &str = "sk-path-reject-test"; + + fn state() -> ProxyState { + let apikey: ApiKey = serde_json::from_value(serde_json::json!({ + "key_hash": ApiKey::hash_bearer(TOKEN), + "allowed_models": ["*"], + })) + .expect("valid apikey"); + let snapshot = AisixSnapshot::new(); + snapshot + .apikeys + .insert(ResourceEntry::new("ak-1", apikey, 1)); + let cfg = ProxyConfig { + addr: "127.0.0.1:0".into(), + request_body_limit_bytes: 0, + tls: None, + real_ip: Default::default(), + url_rewrites: Vec::new(), + }; + ProxyState::new( + SnapshotHandle::new(snapshot), + Arc::new(aisix_gateway::Hub::new()), + &cfg, + ) + .without_cache() + } + + fn router() -> axum::Router { + crate::build_router(state()) + } + + async fn send( + router: axum::Router, + method: &str, + path: &str, + auth: bool, + ) -> (StatusCode, String) { + let mut builder = HttpRequest::builder().method(method).uri(path); + if auth { + builder = builder.header("authorization", format!("Bearer {TOKEN}")); + } + let response = router + .oneshot(builder.body(Body::empty()).unwrap()) + .await + .expect("router responds"); + let status = response.status(); + let bytes = axum::body::to_bytes(response.into_body(), 1 << 16) + .await + .unwrap_or_default(); + (status, String::from_utf8_lossy(&bytes).into_owned()) + } + + #[tokio::test] + async fn malformed_path_params_answer_the_openai_envelope_across_the_family() { + // `%ff` is valid percent-encoding but invalid UTF-8 after decoding — + // the `Path` extractor rejects it. Pre-#880 that was axum's bare 400 + // text; every `:param` route must now answer the caller envelope + // (and, mechanically via `reject_before_dispatch`, emit the access + // log + metrics every other pre-dispatch rejection gets). + let state = state(); + let router = crate::build_router(state.clone()); + for (method, path) in [ + ("POST", "/a2a/%ff"), + ("GET", "/a2a/%ff/.well-known/agent-card.json"), + ("GET", "/mcp/%ff"), + ("GET", "/v1/files/%ff"), + ("DELETE", "/v1/files/%ff"), + ("GET", "/v1/files/%ff/content"), + ("GET", "/v1/batches/%ff"), + ("POST", "/v1/batches/%ff/cancel"), + ("GET", "/v1/fine_tuning/jobs/%ff"), + ("POST", "/v1/fine_tuning/jobs/%ff/cancel"), + ("GET", "/v1/videos/%ff"), + ("GET", "/v1/videos/%ff/content"), + ("POST", "/passthrough/%ff/v1/chat"), + ] { + let (status, body) = send(router.clone(), method, path, true).await; + assert_eq!(status, StatusCode::BAD_REQUEST, "{method} {path}: {body}"); + assert!( + body.contains("invalid_request_error"), + "{method} {path} must answer the OpenAI envelope, got: {body}" + ); + } + + // The refusals are RECORDED, not just enveloped: the chokepoint + // counts them with the unresolved labels. + let scrape = state.metrics.render(); + assert!( + scrape.contains(r#"status="400""#) && scrape.contains(r#"provider="unknown""#), + "the 400s must be counted with unresolved labels, got: {scrape}" + ); + } + + #[tokio::test] + async fn wiring_bugs_keep_their_500_instead_of_blaming_the_caller() { + // A handler whose tuple arity doesn't match the route's captures is + // a server wiring bug — axum classifies it 500, and the extractor + // must pass that through rather than record a caller-caused 400. + async fn miswired( + crate::reject::AisixPath(_x): crate::reject::AisixPath, + axum::extract::State(_): axum::extract::State, + ) -> &'static str { + "unreachable" + } + let router = axum::Router::new() + .route("/wired/:a/:b", axum::routing::get(miswired)) + .with_state(state()); + let (status, body) = send(router, "GET", "/wired/x/y", false).await; + assert_eq!(status, StatusCode::INTERNAL_SERVER_ERROR, "{body}"); + assert!( + !body.contains("invalid_request_error"), + "a wiring bug must not wear the caller envelope: {body}" + ); + } + + #[tokio::test] + async fn multipart_content_type_mismatch_answers_the_envelope() { + // Sending JSON to a multipart endpoint is a common client mistake — + // the same silent bare-400 class, one extractor over (#880 review + // follow-up). Every multipart route must answer the envelope. + let router = router(); + for path in [ + "/v1/audio/transcriptions", + "/v1/audio/translations", + "/v1/files", + ] { + let request = HttpRequest::post(path) + .header("authorization", format!("Bearer {TOKEN}")) + .header("content-type", "application/json") + .body(Body::from("{}")) + .unwrap(); + let response = router + .clone() + .oneshot(request) + .await + .expect("router responds"); + let status = response.status(); + let bytes = axum::body::to_bytes(response.into_body(), 1 << 16) + .await + .unwrap_or_default(); + let body = String::from_utf8_lossy(&bytes); + assert_eq!(status, StatusCode::BAD_REQUEST, "{path}: {body}"); + assert!( + body.contains("invalid_request_error"), + "{path} must answer the OpenAI envelope, got: {body}" + ); + } + } + + #[tokio::test] + async fn auth_still_precedes_the_path_parse() { + // Extractor order is unchanged: an unauthenticated caller gets 401, + // not a 400 that reveals how the path would have parsed. + let router = router(); + let (status, _) = send(router, "GET", "/v1/files/%ff", false).await; + assert_eq!(status, StatusCode::UNAUTHORIZED); + } +} diff --git a/crates/aisix-proxy/src/videos.rs b/crates/aisix-proxy/src/videos.rs index a16339de..63eca549 100644 --- a/crates/aisix-proxy/src/videos.rs +++ b/crates/aisix-proxy/src/videos.rs @@ -71,7 +71,7 @@ use aisix_core::AppliedGuardrail; use aisix_obs::{AccessLog, RequestOutcome, UsageEvent}; -use axum::extract::{Path, State}; +use axum::extract::State; use axum::http::{header, StatusCode}; use axum::response::{IntoResponse, Response}; use axum::Json; @@ -83,6 +83,7 @@ use std::time::{Duration, Instant, SystemTime, UNIX_EPOCH}; use crate::auth::AuthenticatedKey; use crate::client_ip::ClientContext; use crate::error::{ErrorEnvelope, ProxyError}; +use crate::reject::AisixPath; use crate::state::ProxyState; /// DashScope video-synthesis submit path (relative to the ProviderKey's @@ -1762,7 +1763,7 @@ pub async fn get_video( State(state): State, auth: AuthenticatedKey, client: ClientContext, - Path(video_id): Path, + AisixPath(video_id): AisixPath, ) -> Response { let telemetry = Telemetry { state: &state, @@ -1813,7 +1814,7 @@ pub async fn video_content( State(state): State, auth: AuthenticatedKey, client: ClientContext, - Path(video_id): Path, + AisixPath(video_id): AisixPath, ) -> Response { let telemetry = Telemetry { state: &state, diff --git a/tests/e2e/src/cases/path-param-reject-e2e.test.ts b/tests/e2e/src/cases/path-param-reject-e2e.test.ts new file mode 100644 index 00000000..36743cc3 --- /dev/null +++ b/tests/e2e/src/cases/path-param-reject-e2e.test.ts @@ -0,0 +1,107 @@ +import { createHash } from "node:crypto"; +import { afterAll, beforeAll, describe, expect, test } from "vitest"; +import { + EtcdClient, + SeedClient, + spawnApp, + type SpawnedApp, +} from "../harness/index.js"; + +// E2E: a `:param` segment that fails path extraction (valid percent-encoding, +// invalid UTF-8 after decoding — `%ff`) answers the caller envelope instead +// of axum's bare 400, across the `:param` route family (#880). The access +// log + metrics side rides the same `reject_before_dispatch` call every +// other pre-dispatch rejection uses; the envelope is the e2e-observable +// contract. Authentication still precedes the path parse. + +const KEY = "sk-path-reject-e2e"; +const sha256 = (s: string) => createHash("sha256").update(s).digest("hex"); + +describe("path-param rejection e2e: :param routes answer the envelope", () => { + let app: SpawnedApp | undefined; + let etcdReachable = false; + + beforeAll(async () => { + const etcd = new EtcdClient(); + etcdReachable = await etcd.ping(); + if (!etcdReachable) return; + + app = await spawnApp(); + const seed = new SeedClient(etcd, app.etcdPrefix); + await seed.createApiKey({ + key_hash: sha256(KEY), + allowed_models: ["*"], + }); + + // Propagation probe: the key authenticates (any status but 401). + for (let i = 0; i < 100; i += 1) { + const res = await fetch(`${app.proxyUrl}/v1/files/probe`, { + headers: { authorization: `Bearer ${KEY}` }, + }); + if (res.status !== 401) return; + await new Promise((resolve) => setTimeout(resolve, 100)); + } + throw new Error("seeded key never became active"); + }, 60_000); + + afterAll(async () => { + await app?.exit(); + }); + + test("malformed :param answers the OpenAI envelope on every family member", async (ctx) => { + if (!etcdReachable || !app) return ctx.skip(); + + for (const [method, path] of [ + ["POST", "/a2a/%ff"], + ["GET", "/mcp/%ff"], + ["GET", "/v1/files/%ff"], + ["GET", "/v1/batches/%ff"], + ["GET", "/v1/fine_tuning/jobs/%ff"], + ["GET", "/v1/videos/%ff"], + ["POST", "/passthrough/%ff/v1/chat"], + ] as const) { + const res = await fetch(`${app.proxyUrl}${path}`, { + method, + headers: { authorization: `Bearer ${KEY}` }, + }); + expect(res.status, `${method} ${path}`).toBe(400); + const body = (await res.json()) as { + error?: { type?: string; message?: string }; + }; + expect(body.error?.type, `${method} ${path}`).toBe( + "invalid_request_error", + ); + } + }); + + test("multipart content-type mismatch answers the envelope too", async (ctx) => { + if (!etcdReachable || !app) return ctx.skip(); + + // Same silent class one extractor over: JSON sent to a multipart + // endpoint used to get axum's bare 400. + for (const path of [ + "/v1/audio/transcriptions", + "/v1/audio/translations", + "/v1/files", + ]) { + const res = await fetch(`${app.proxyUrl}${path}`, { + method: "POST", + headers: { + authorization: `Bearer ${KEY}`, + "content-type": "application/json", + }, + body: "{}", + }); + expect(res.status, path).toBe(400); + const body = (await res.json()) as { error?: { type?: string } }; + expect(body.error?.type, path).toBe("invalid_request_error"); + } + }); + + test("an unauthenticated malformed :param is still 401 first", async (ctx) => { + if (!etcdReachable || !app) return ctx.skip(); + + const res = await fetch(`${app.proxyUrl}/v1/files/%ff`); + expect(res.status).toBe(401); + }); +});