fix(proxy): route Path-extractor rejections through the reject chokepoint - #884
Conversation
…oint A ':param' segment that fails extraction — valid percent-encoding that is invalid UTF-8 after decoding, like /v1/files/%ff — answered axum's bare 400: no access log, no request metrics, no caller envelope. The same silent pre-dispatch class #863 collected for body rejections. New AisixPath extractor wraps axum's Path and routes the rejection through reject_before_dispatch, so the refusal gets the OpenAI envelope plus the access log + metrics every other pre-dispatch rejection gets; the resolved key published by the auth extractor attributes it. Every ':param' handler (a2a, mcp scoped, files, batches, fine-tuning jobs, videos, passthrough — 13 sites) takes AisixPath instead of Path, so the family can't drift back. Extractor order is unchanged: authentication still precedes the path parse, pinned by test. Fixes #880.
|
Warning Review limit reachedYou’ve reached a temporary PR review limit under our Fair Usage Limits Policy. Next review available in: 54 minutes Enable usage-based reviews in Billing to review now. Otherwise, wait until the next included review is available. How can I continue?After more reviews become available, a review can be triggered using the To avoid repeated limits, reduce automatic review volume by pausing incremental auto-reviews earlier, using label-based review opt-in, excluding WIP or generated PR titles, or requesting reviews manually when the PR is ready. If your team needs uninterrupted high-volume reviews, an organization admin can enable usage-based reviews. How do review limits work?CodeRabbit enforces per-developer PR review limits for each organization. Most developers receive the normal plan review availability. For paid Pro and Pro+ PR reviews, CodeRabbit uses adaptive limits for sustained high-volume activity. When a developer's recent PR review activity reaches the 95th percentile or higher among CodeRabbit users, additional reviews become available more gradually as earlier reviews age out of the rolling window. Please refer docs for additional details. Review details⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (8)
Comment |
…ections too Independent review findings: - The extractor rendered every PathRejection as a caller 400, including axum's 500-class programmer errors (wrong parameter arity, unsupported type). Those keep axum's own loud 500 now — a wiring bug must not be recorded as caller input — pinned by a deliberately mis-wired test route. - The same silent bare-400 class existed one extractor over: JSON sent to a multipart endpoint (/v1/audio/transcriptions, /v1/audio/ translations, /v1/files) bypassed the envelope and telemetry the same way. All three now route through the chokepoint. The WebSocketUpgrade rejection on /v1/realtime is deferred to #885: its correct status is 426 and ProxyError has no such variant yet. - The family test now also asserts the refusals are counted (metrics scrape with the unresolved labels), covers DELETE /v1/files/:id, and the request-id fallback carries a comment naming why it mirrors the handlers' idiom.
1 participant
What
A
:paramsegment that fails path extraction — valid percent-encoding that decodes to invalid UTF-8, like/v1/files/%ff— answered axum's bare 400 from inside the extractor: no access log line, no request metrics, no caller envelope. That is the same silent pre-dispatch class #863 collected intocrate::rejectfor body rejections: the caller sees an error the operator has no record of.New
AisixPath<T>extractor (inreject.rs, next to the chokepoint it feeds) wrapsaxum::extract::Pathand routes the rejection throughreject_before_dispatch:invalid_request_error) and emit the access log + request metrics every other pre-dispatch rejection gets (unresolved provider/model labels — cardinality unchanged; the raw path only reaches the access log);api_key_id).Every
:paramhandler takesAisixPathinstead ofPath— 13 sites across/a2a/:agent(+card),/mcp/:server,/v1/files/:id(+content/DELETE),/v1/batches/:id(+cancel),/v1/fine_tuning/jobs/:id(+cancel),/v1/videos/:id(+content), and/passthrough/:provider/*rest. Behavior for valid parameters is byte-identical (pure delegation to axum'sPath); extractor order is unchanged, so authentication still precedes the path parse.Same class, one extractor over (review follow-up): JSON sent to a multipart endpoint —
/v1/audio/transcriptions,/v1/audio/translations,/v1/filesupload — was the identical silent bare-400; all three now route through the chokepoint. TheWebSocketUpgraderejection on/v1/realtimeis deferred to #885: its correct refusal is426 Upgrade RequiredandProxyErrorhas no such variant, so folding it in here would mis-report the status.Envelope note: all touched routes speak the OpenAI error shape; the Anthropic-envelope routes (
/v1/messages*) carry no path parameters and no multipart.Tests
reject.rs): the whole:paramfamily — 13 malformed-path requests assert 400 + envelope and that the refusals are counted (metrics scrape with unresolved labels); the three multipart routes assert the envelope on a content-type mismatch; a deliberately mis-wired route pins that arity bugs keep their 500 without the caller envelope; auth-precedes-parse pinned.path-param-reject-e2e.test.ts): real binary + etcd — every:paramfamily member answers the envelope on%ff, the multipart mismatch answers it too, and the unauthenticated variant stays 401-first.No LiteLLM baseline applies: gateway-internal rejection telemetry/envelope consistency with no equivalent surface there.
Fixes #880.