fix(security): data-plane P1 findings from AISIX-Cloud#911 - #683
Merged
Conversation
The Guardrail.mandatory field was parsed and stored but never read at runtime — a remote guardrail (Bedrock/Azure/Aliyun) marked mandatory still followed fail_open, so when its upstream was unreachable the request proceeded unscanned (Bypass). default_fail_open() is true, so an operator who set mandatory=true expecting fail-closed got silent fail-open. mandatory means the opposite of what it did. Wire it via a MandatoryGuardrail decorator (mirrors the existing MonitorGuardrail/enforcement_mode pattern) applied outermost in build_one: it upgrades a Bypass verdict to Block, overriding fail_open on the failure path. Allow/Block pass through untouched and only remote guardrails emit Bypass, so keyword rows are behaviourally unchanged. Tests: mandatory turns input+output Bypass into Block; non-mandatory keeps failing open; Allow/Block pass through unchanged.
The audio (transcription/translation/speech) and raw passthrough tunnel dispatched directly to the upstream WITHOUT applying the model's request_timeout, unlike every other non-streaming direct-upstream path (count_tokens/rerank/responses, wired by #554). A slow or blackholed provider could therefore pin one of these requests open past the model's configured deadline, and the timeout-driven cooldown/failover never engaged. All three sites fully buffer the response (.bytes()), so the E2E request_timeout is the correct bound (no streaming to truncate). Adds a DP E2E that drives a transcription against a stalling upstream and asserts the request is abandoned well before the upstream would respond.
The typed proxy endpoints (chat/messages/completions/count_tokens/ embeddings/images/rerank/responses and audio speech) recorded the RAW client-supplied `model` field as the Prometheus `model` label on their error paths. That field is caller-controlled free text until it resolves against the snapshot, so on a pre-resolution failure (model-not-found) a caller could mint unbounded metric series by sending many unique unknown model names — a metric-cardinality DoS. Collapse any unresolved model to a fixed "unresolved" sentinel via a shared usage_attr::metric_model_label helper (get_by_name covers direct models and virtual routers alike), the typed-endpoint analogue of passthrough's PASSTHROUGH_MODEL_LABEL guard (#451). The raw requested name still flows to the per-request access log and usage events, which are bounded by request volume rather than label cardinality. Adds a DP E2E that fires many unique unknown model names at a typed endpoint and asserts none leaks into an aisix_requests_total label and they all collapse to model="unresolved".
…orced completions, rerank, images, audio (transcription/translation/speech), messages and responses reserved the multi-layer rate-limit but dropped the reservation uncommitted, so their TPM/TPD (token-per-minute/day) counters never moved. A caller could bypass token-rate limits by routing traffic through any of these endpoints — only chat and embeddings committed the actual token cost. Each non-streaming dispatch now commits the upstream-reported token total to every reserved layer (0 for endpoints/paths that consume no tokens, e.g. TTS and the not-implemented branches), mirroring the embeddings reference. The verbatim streaming paths of /v1/messages and /v1/responses still release their reservation on drop without post-stream token accounting — closing that requires threading the reservation into the stream's end-of-stream guard (the chat.rs into_stream_hold + concurrency hold pattern, #450/#108) across the failover loop, tracked as a focused follow-up; budget ($) already gates those requests, so the residual gap is token-rate only. Adds a DP E2E: with TPM=10 and an upstream reporting 16 tokens, the first /v1/completions call succeeds and the second is 429 (pre-fix the counter stayed 0 and the second call also succeeded).
/v1/completions ran the input guardrail hook but relayed the model's completion text unscanned. A content/DLP block enforced on /v1/chat/completions was therefore bypassable by moving the response leg to the legacy completions surface — the same output-hook gap #204 closed for streaming chat and #448 for tool-call output. After the upstream returns, buffer the completion choices' text into a synthetic ChatResponse and run the resolved chain's output hook, mirroring chat / responses / messages. A block surfaces the redacted content_filter 422 (naming only the guardrail, never the matched text, per #153/#519) and the upstream tokens stay committed since the provider already billed them. Adds a DP E2E: an innocent prompt against an upstream that emits a forbidden word in its completion text is turned into a content_filter 422 that never carries the word (pre-fix the caller received it verbatim).
/passthrough/:provider/*rest forwarded requests and responses verbatim with no guardrail scanning, so a tenant that configured a content/DLP guardrail could bypass it entirely by routing traffic through passthrough. Resolve the guardrail chain for the model whose credentials the tunnel borrows and, following LiteLLM's passthrough default, scan the whole request body (before the upstream call) and the whole response body (after) as text against that chain. A block surfaces the redacted content_filter 422 that names only the guardrail, never the matched text (#153/#519). Bodies are decoded UTF-8-lossy so binary payloads degrade to replacement chars instead of being skipped; the scan is a no-op when no chain matches. Adds a DP E2E covering both directions: a forbidden request body is blocked before the upstream is called, and an upstream reply carrying a forbidden word is blocked without the word reaching the caller.
📝 WalkthroughWalkthroughThis PR adds mandatory guardrail enforcement, bounds metric model labels for unresolved models, commits quota reservations across proxy endpoints, and applies guardrail and timeout checks to completions and passthrough flows, with end-to-end coverage. ChangesMandatory Guardrail Enforcement
Bounded Metric Model Labels
Quota Token Commit Accounting
Proxy Endpoint Guardrail Enforcement
Estimated code review effort: 4 (Complex) | ~75 minutes Sequence Diagram(s)sequenceDiagram
participant Client
participant Passthrough
participant GuardrailIndex
participant Guardrail
participant Upstream
Client->>Passthrough: POST /passthrough/:provider/*rest
Passthrough->>GuardrailIndex: resolve(model, api key, team)
GuardrailIndex-->>Passthrough: guardrail chain
Passthrough->>Guardrail: check_input(body as ChatFormat)
alt input blocked
Guardrail-->>Passthrough: Block
Passthrough-->>Client: 422 ContentFiltered
else input allowed
Passthrough->>Upstream: forward request
Upstream-->>Passthrough: response body
Passthrough->>Guardrail: check_output(body as ChatResponse)
alt output blocked
Guardrail-->>Passthrough: Block
Passthrough-->>Client: 422 ContentFiltered
else output allowed
Passthrough-->>Client: relay upstream response
end
end
sequenceDiagram
participant Handler
participant Quota
participant Upstream
participant Reservation
Handler->>Quota: enforce(request)
Quota-->>Handler: reservation
Handler->>Upstream: dispatch request
alt success
Upstream-->>Handler: response + usage
Handler->>Reservation: commit_tokens(prompt+completion)
else not supported / bridge error
Handler->>Reservation: commit_tokens(0)
end
Handler-->>Handler: return response or error
Possibly related PRs
🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Comment |
The mandatory-guardrail fail-closed change updated the field doc; dump-schema picks that up as the JSON Schema description. Regenerated to clear drift.
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
crates/aisix-proxy/src/audio.rs (1)
446-464: 🩺 Stability & Availability | 🟠 Major | ⚡ Quick winMap audio timeouts through the shared bridge-error conversion.
req.timeout(d)makes the request fail with a timeout-shapedreqwest::Error, but both audio paths still wrapsend()failures asBridgeError::Transport, so the per-model deadline won’t surface as a timeout or feed the same cooldown/failover path asrerank. Usereqwest_error_to_bridgein both send sites.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aisix-proxy/src/audio.rs` around lines 446 - 464, The audio request send path is wrapping all `req.send()` failures as `BridgeError::Transport`, so timeout failures from `req.timeout(d)` are not converted consistently with other bridge paths. Update both audio send sites in `audio.rs` to route the `reqwest::Error` through `reqwest_error_to_bridge` before calling `note_failure`, using the existing request flow around `req.send()`/`req.timeout(d)` and the `BridgeError` handling in the audio transcription/translation code.
🧹 Nitpick comments (2)
tests/e2e/src/cases/completions-tpm-commit-e2e.test.ts (1)
89-114: 🩺 Stability & Availability | 🔵 Trivial | 💤 Low valuePossible flake at TPM window boundary.
If the rate limiter uses a fixed per-minute window, running the two sequential
postCompletion()calls right at a minute boundary could reset the counter between calls, causing an intermittent false failure since the second call would then return 200. Worth confirming the limiter's windowing semantics against the CI cadence.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/e2e/src/cases/completions-tpm-commit-e2e.test.ts` around lines 89 - 114, The TPM commit e2e test can flake at a minute boundary because the two sequential postCompletion() calls may land in different fixed windows and reset the counter between requests. Update the test in completions-tpm-commit-e2e.test.ts, specifically the second /v1/completions assertion in the test around postCompletion(), so it avoids relying on wall-clock boundary timing by anchoring the calls within the same window or making the setup wait for a stable window before sending the first request.crates/aisix-core/src/models/guardrail.rs (1)
460-464: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winRewrite this as public API reference text.
This docstring exposes internal implementation details (
DP,MandatoryGuardrail,Bypass,Block) in a model comment that feeds public schema/API docs. Describe the observable contract instead: whenmandatoryis enabled, evaluation failures from remote guardrails reject the request even iffail_openis enabled. As per coding guidelines, "Write descriptions in Admin API resource models and OpenAPI definitions as public API reference text, not internal implementation notes. Avoid internal shorthand (DP, CP, kine row, wire shape, mock server, bridge dispatch, issue-only context)."🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@crates/aisix-core/src/models/guardrail.rs` around lines 460 - 464, Rewrite the doc comment on the guardrail model field to describe only the public behavior of the mandatory setting, not the implementation details. In the `guardrail.rs` model docs, replace references to internal terms like `DP`, `MandatoryGuardrail`, `Bypass`, and `Block` with public API language stating that when `mandatory` is enabled, remote guardrail evaluation failures reject the request even if `fail_open` is set. Keep the wording suitable for schema/OpenAPI reference text and aligned with the observable contract of the field.Source: Coding guidelines
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@crates/aisix-guardrails/src/build.rs`:
- Around line 395-406: The MandatoryGuardrail::enforce path converts
GuardrailVerdict::Bypass into a Block via GuardrailVerdict::block(...), which
loses guardrail_name. Update the bypass-to-block handling in
MandatoryGuardrail::enforce to construct the Block variant explicitly and
preserve guardrail_name using self.row_name.clone(), so downstream 422 envelopes
keep the correct name.
In `@crates/aisix-proxy/src/completions.rs`:
- Around line 273-320: The output guardrail block path in `completions.rs` loses
billed usage metadata after `bridge.complete()` succeeds and
`reservation.commit_tokens(total_tokens)` runs, so `dispatch()` falls back to a
zeroed `emit_error_usage_event(...)` path. Update the `check_output()` /
`ProxyError::ContentFiltered` handling around `resolved_chain`, `ChatResponse`,
and `reservation.commit_tokens` to mirror the charged-failure pattern used in
`chat.rs`, preserving `model_id`, `provider_key_id`, and upstream usage when
returning the 422 error.
In `@crates/aisix-proxy/src/messages.rs`:
- Line 470: The streamed `/v1/messages` path is skipping
`reservation.commit_tokens(...)`, so it can bypass TPM/TPD enforcement. Update
the `messages.rs` stream flow around `usage_handled_by_stream` and the stream
completion guard to carry the reservation keys through completion, then invoke
post-stream token accounting with the terminal usage just like the chat stream
path. Make sure the fix is applied in both the reservation setup site and the
later stream finalization block referenced by the comment.
In `@crates/aisix-proxy/src/passthrough.rs`:
- Around line 309-339: The passthrough request flow is reserving quota before
the input guardrail check, so blocked requests still consume RPM. Move the quota
reservation in passthrough.rs to after the guardrail branch that uses
Guardrail::check_input and returns ProxyError::ContentFiltered, mirroring the
ordering used in audio.rs. Keep the reservation tied to the successful path
only, so rejected passthrough bodies never call quota::enforce() before the
upstream is reached.
In `@crates/aisix-proxy/src/responses.rs`:
- Line 349: The streamed /v1/responses path is skipping quota reconciliation
because usage_handled_by_stream bypasses reservation.commit_tokens(...) without
any post-stream token accounting. Update the responses streaming flow in
responses.rs to mirror the chat stream path: carry the reservation keys/handle
into the stream completion guard, and on terminal usage call post-stream token
accounting before releasing the reservation. Ensure the fix is applied around
the reservation setup and streaming completion logic so streamed responses still
count against TPM/TPD.
In `@tests/e2e/src/cases/metric-cardinality-model-label-e2e.test.ts`:
- Around line 113-125: The unknown-model POSTs in the metric cardinality e2e
test are being swallowed, so the test can pass without proving the request path
ran. Update the loop in metric-cardinality-model-label-e2e.test.ts to capture
each fetch response (or thrown error) and assert the expected failure/status for
the bogus model requests instead of using a catch that ignores everything. Use
the existing request block around BOGUS_COUNT and the /v1/chat/completions fetch
to verify each call actually reaches the server and fails as intended before the
scrape assertions run.
In `@tests/e2e/src/cases/passthrough-guardrail-e2e.test.ts`:
- Around line 145-166: The passthrough guardrail test depends on a prior test’s
propagation wait, so it can race when run alone or in a different order. Update
the `forbidden request body is blocked by the input guardrail before the
upstream is called` test to synchronize itself by waiting for guardrail/config
propagation before calling `passthrough`, or move that readiness setup into the
suite’s `beforeAll`. Use the existing `waitConfigPropagation()` helper (or the
same readiness logic used elsewhere in this file) so the test does not rely on
execution order.
---
Outside diff comments:
In `@crates/aisix-proxy/src/audio.rs`:
- Around line 446-464: The audio request send path is wrapping all `req.send()`
failures as `BridgeError::Transport`, so timeout failures from `req.timeout(d)`
are not converted consistently with other bridge paths. Update both audio send
sites in `audio.rs` to route the `reqwest::Error` through
`reqwest_error_to_bridge` before calling `note_failure`, using the existing
request flow around `req.send()`/`req.timeout(d)` and the `BridgeError` handling
in the audio transcription/translation code.
---
Nitpick comments:
In `@crates/aisix-core/src/models/guardrail.rs`:
- Around line 460-464: Rewrite the doc comment on the guardrail model field to
describe only the public behavior of the mandatory setting, not the
implementation details. In the `guardrail.rs` model docs, replace references to
internal terms like `DP`, `MandatoryGuardrail`, `Bypass`, and `Block` with
public API language stating that when `mandatory` is enabled, remote guardrail
evaluation failures reject the request even if `fail_open` is set. Keep the
wording suitable for schema/OpenAPI reference text and aligned with the
observable contract of the field.
In `@tests/e2e/src/cases/completions-tpm-commit-e2e.test.ts`:
- Around line 89-114: The TPM commit e2e test can flake at a minute boundary
because the two sequential postCompletion() calls may land in different fixed
windows and reset the counter between requests. Update the test in
completions-tpm-commit-e2e.test.ts, specifically the second /v1/completions
assertion in the test around postCompletion(), so it avoids relying on
wall-clock boundary timing by anchoring the calls within the same window or
making the setup wait for a stable window before sending the first request.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: Organization UI
Review profile: CHILL
Plan: Pro
Run ID: 28893282-42b9-4a6e-a9f5-ed385936c34d
📒 Files selected for processing (18)
crates/aisix-core/src/models/guardrail.rscrates/aisix-guardrails/src/build.rscrates/aisix-proxy/src/audio.rscrates/aisix-proxy/src/chat.rscrates/aisix-proxy/src/completions.rscrates/aisix-proxy/src/count_tokens.rscrates/aisix-proxy/src/embeddings.rscrates/aisix-proxy/src/images.rscrates/aisix-proxy/src/messages.rscrates/aisix-proxy/src/passthrough.rscrates/aisix-proxy/src/rerank.rscrates/aisix-proxy/src/responses.rscrates/aisix-proxy/src/usage_attr.rstests/e2e/src/cases/audio-timeout-e2e.test.tstests/e2e/src/cases/completions-output-guardrail-e2e.test.tstests/e2e/src/cases/completions-tpm-commit-e2e.test.tstests/e2e/src/cases/metric-cardinality-model-label-e2e.test.tstests/e2e/src/cases/passthrough-guardrail-e2e.test.ts
…etric fixes - mandatory guardrail: preserve row name when upgrading Bypass to Block so the 422 envelope names the guardrail instead of an unnamed content-filter - passthrough: reserve rate-limit layers AFTER the input guardrail so a content block doesn't burn an RPM slot (matches typed endpoints) - metric-cardinality e2e: assert the error status on each unresolved request instead of swallowing it, so a regression that resolves them is caught - passthrough-guardrail e2e: self-synchronize the input-block test on its own readiness gate instead of relying on the output-block test's propagation
…block path The #911 [23] output guardrail on /v1/completions returns 422 AFTER the upstream billed and reservation.commit_tokens ran. Returning a bare error made the handler fall onto emit_error_usage_event with zero tokens, dropping model_id / provider_key_id / usage for a request the provider already charged — under-reporting to cp-api's budget ledger and /logs. Mirror responses.rs #543 / chat.rs UpstreamCharge: on an output block carry the billed usage on the success struct marked guardrail_blocked and return the redacted 422 body, so the UsageEvent keeps the real counts. E2E asserts the block is recorded on the charged path (provider=openai) not the zeroed error path (provider=unknown).
This was referenced Jul 2, 2026
Closed
This was referenced Jul 10, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes the data-plane P1 findings from the two security review docs tracked in api7/AISIX-Cloud#911. Each fix ships with a DP E2E that fails before and passes after. P2-and-lower findings are intentionally out of scope (documented as won't-fix on the issue).
Fixes api7/AISIX-Cloud#911
Findings addressed
[26] Mandatory guardrails weren't fail-closed. A
mandatoryguardrail whose remote backend failed open produced aBypassverdict that was let through. Added aMandatoryGuardraildecorator (mirrorsMonitorGuardrail) that convertsBypass→Blockfor mandatory rows, so a mandatory guardrail that can't run blocks the request instead of silently passing it.[22] Audio and raw passthrough ignored the per-model request timeout.
/v1/audio/{transcriptions,translations,speech}and the passthrough tunnel dispatched to the upstream without applyingmodel.request_timeout(), so a slow/blackholed provider could pin a request open past the configured deadline. All three sites fully buffer the response, so the E2E request timeout is the correct bound.[27] Error-path metric
modellabel was caller-controlled. On a pre-resolution failure (model-not-found) the typed endpoints recorded the raw clientmodelstring as a Prometheus label, allowing unbounded time-series creation (cardinality DoS). Unresolved models now collapse to a fixedunresolvedsentinel via a shared helper (the typed-endpoint analogue of the passthroughPASSTHROUGH_MODEL_LABELguard).[21] TPM/TPD wasn't enforced on non-chat endpoints. completions / rerank / images / audio / messages / responses reserved the rate-limit layers but dropped the reservation uncommitted, so their token counters never moved — a caller could bypass token-rate limits by routing through them. Each non-streaming dispatch now commits the upstream-reported token total. The verbatim streaming paths of
/v1/messagesand/v1/responsesstill release their reservation on drop without post-stream token accounting; closing that requires threading the reservation into the end-of-stream guard (the streaminginto_stream_holdpattern) and is tracked in Streaming /v1/messages and /v1/responses skip TPM/TPD + release concurrency early #688 — budget ($) already gates those requests via the cp-api ledger, so the residual gap is token-rate + concurrent-stream count.[23]
/v1/completionsskipped output guardrails. It ran the input hook but relayed the completion text unscanned, so a content/DLP block enforced on/v1/chat/completionswas bypassable via the legacy surface. It now buffers the completion text into a synthetic response and runs the resolved chain's output hook, mirroring chat/responses/messages. Because the upstream already billed when the block fires, the 422 carries the billed usage (markedguardrail_blocked) so cp-api's ledger doesn't under-report the charged spend.[6] The raw passthrough tunnel skipped guardrails entirely.
/passthrough/:provider/*restforwarded verbatim with no scanning, so any configured content/DLP guardrail was bypassable through it. Following LiteLLM's passthrough default, the request and response bodies are now scanned as text (UTF-8-lossy) against the resolved chain; a block surfaces a redactedcontent_filter422.Behavior / compatibility
content_filter422 (typed) — same envelope shape already used by chat; the matched text is never echoed (only the guardrail name).api7/docsPR.Summary by CodeRabbit
New Features
/v1/completionsnow enforces output guardrails and can return content-filtered errors.Bug Fixes
Tests