Uh oh!
There was an error while loading. Please reload this page.
fix(proxy): redact matched literal from guardrail-block error envelope - #203
Merged
Conversation
ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Free Run ID: 📒 Files selected for processing (5)
🚧 Files skipped from review as they are similar to previous changes (4)
📝 WalkthroughWalkthroughThis PR redacts guardrail-matched patterns from client-facing error messages to prevent policy bypass via echoed literals. The error type definition is simplified, input and output guardrail handlers log matched details to tracing while returning generic redacted messages, unit tests verify redaction is enforced, and a new E2E test validates the output guardrail path end-to-end. ChangesGuardrail Pattern Redaction
Estimated code review effort🎯 3 (Moderate) | ⏱️ ~20 minutes Note 🎁 Summarized by CodeRabbit FreeYour organization is on the Free plan. CodeRabbit will generate a high-level summary and a walkthrough for each pull request. For a comprehensive line-by-line review, please upgrade your subscription to CodeRabbit Pro by visiting https://app.coderabbit.ai/login. Comment |
There was a problem hiding this comment.
Pull request overview
This PR addresses a security vulnerability where keyword guardrails (especially hook_point: "output") could leak the matched forbidden literal back to callers via the OpenAI-shaped error.message. The fix ensures the wire-level error message is redacted while preserving the detailed match reason in operator logs.
Changes:
- Redacted
ProxyError::ContentFiltereddisplay output so call sites fully control the caller-visible message. - Updated input/output guardrail block paths to log the detailed verdict reason via
tracing::warn!while returning a generic, non-leaking error message. - Added/updated unit + e2e regression tests to assert the forbidden literal never appears anywhere in the caller-visible error envelope.
Reviewed changes
Copilot reviewed 4 out of 4 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
crates/aisix-proxy/src/error.rs | Changes ContentFiltered display to avoid prefixing and documents the redaction requirement. |
crates/aisix-proxy/src/chat.rs | Redacts guardrail-block error messages on input/output paths and logs detailed reasons to tracing. |
crates/aisix-proxy/src/lib.rs | Strengthens unit tests to assert matched literals are not present and messages match the redacted strings. |
tests/e2e/src/cases/guardrail-output-e2e.test.ts | Adds an e2e regression test for output keyword guardrails ensuring no forbidden literal leaks in error responses. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
+81
to
+83
| /// Constructors at `chat.rs::route_chat_completions` and | ||
| /// `chat.rs::dispatch_and_render` build a redacted public message | ||
| /// (`"request blocked by content policy"` / |
moonmingforce-pushed
the
fix/153-output-guardrail-leak
branch
from
May 10, 2026 05:58
041fcff to
6d3a9c6Comparecloses#153) When a `kind: "keyword"` guardrail blocked a request or response, the gateway's caller-visible `error.message` (OpenAI envelope) included the matched literal verbatim: ```json { "error": { "message": "content blocked by policy: output blocked by literal \"leakedsecret\"", "type": "content_filter" } } ``` For OUTPUT guardrails this is a real bypass — the whole point of an output guardrail is to keep forbidden content from reaching the caller, and echoing the matched literal in the error message defeats that. Anyone who can trigger the rule can extract the model's forbidden output via error responses. For INPUT guardrails the leak also enables blocklist enumeration: probing with suspect content and inspecting the reflected literal lets a caller learn the policy's patterns. Redact at the wire boundary, keep rich detail in operator logs. - `ProxyError::ContentFiltered`'s Display impl changed from `"content blocked by policy: {0}"` to `"{0}"` so constructors fully control the wire-level string. - Both construction sites in `crates/aisix-proxy/src/chat.rs` (input guardrail at L299, output guardrail at L651) now build a generic static message: - input: `"request blocked by content policy"` - output: `"response blocked by content policy"` and emit the verdict's rich `reason` (which contains the matched literal and rule type) via `tracing::warn!` for operator debugging. - Updated existing `input_guardrail_block_returns_422_and_skips_upstream` unit test: replaced the `.contains("forbidden-token")` assertion (which pinned the leaky behavior) with `!message.contains(...)` plus an exact-match check on the redacted string. - Strengthened `output_guardrail_block_returns_422_after_upstream_runs` to assert the matched literal `"secret-string"` does NOT appear in ANY field of the wire envelope (full-blob substring check), and that the message exactly equals the redacted string. - Added e2e regression `tests/e2e/src/cases/guardrail-output-e2e.test.ts` exercising the user journey through the OpenAI Node SDK against a live mock upstream that emits a forbidden literal in the assistant response. Asserts: - 422 with `error.type === "content_filter"` - `errorBlob.not.toContain(FORBIDDEN_WORD)` - upstream WAS hit (output guardrails fire post-dispatch) - `cargo test -p aisix-proxy --lib`: 138/138 passing - `cargo clippy -p aisix-proxy --lib --tests -- -D warnings`: clean - `cargo fmt --check`: clean - `pnpm tsc --noEmit` (e2e): clean Refs: #199 (related: BridgeError Display also leaks upstream-message bleed-through into `error.message` system-wide; out of scope here, but fix shape is similar — sanitize at proxy boundary, keep rich detail in tracing).
moonmingforce-pushed
the
fix/153-output-guardrail-leak
branch
from
May 10, 2026 06:01
6d3a9c6 to
6331374Compare
Comment on lines
+81
to
+82
| /// Constructors at `chat.rs::route_chat_completions` and | ||
| /// `chat.rs::dispatch_and_render` build a redacted public message |
Uh oh!
There was an error while loading. Please reload this page.
moonming added a commit
that referenced
this pull request
May 14, 2026
- quickstart/self-hosted: document the /livez liveness route and plain-text body (per #257); add Cleanup - quickstart/openai-sdk: switch to .mjs so \`node\` runs the example without a TypeScript loader - quickstart/first-model-first-key-first-request: add production-credentials warning, 401/403 verification step using the real proxy error envelope, Cleanup, and a per-provider api_base callout - integration/openai-compatible-api: add mermaid diagram of the request path; route list reflects the /livez rename - integration/errors-and-retries: replace placeholder error.type strings with the real ProxyError mapping; add 413 RequestTooLarge row; note the admin {error_msg} envelope distinction - configuration/provider-keys: warn about plaintext secret storage; replace the inaccurate OpenAI api_base normalization claim with a per-provider truth table (refs #270) - operations/health-checks: rewrite for the /livez liveness contract plus the /admin/v1/health per-model shape (per #256 + #257) - reference/proxy-api-reference: /livez instead of /health in the route list - tutorials/build-a-virtual-model-with-failover: rewrite end-to-end against routing-strategies-e2e (deliberately break primary, observe cooldown) - tutorials/enable-response-caching: rewrite against cache-policy-e2e (x-aisix-cache miss then hit, different prompt misses again) - tutorials/add-keyword-guardrails: rewrite against guardrail-keyword-e2e (422 content_filter; no-leak message contract per #203) - tutorials/openai-client-to-anthropic-upstream: rewrite against anthropic-upstream-e2e; document bare-host api_base for Anthropic Every command, field, header, and error code was cross-checked against the relevant crate or e2e test on this branch. No mock data; nothing speculative.
moonming added a commit
that referenced
this pull request
May 14, 2026
- quickstart/self-hosted: document the /livez liveness route and plain-text body (per #257); add Cleanup - quickstart/openai-sdk: switch to .mjs so \`node\` runs the example without a TypeScript loader - quickstart/first-model-first-key-first-request: add production-credentials warning, 401/403 verification step using the real proxy error envelope, Cleanup, and a per-provider api_base callout - integration/openai-compatible-api: add mermaid diagram of the request path; route list reflects the /livez rename - integration/errors-and-retries: replace placeholder error.type strings with the real ProxyError mapping; add 413 RequestTooLarge row; note the admin {error_msg} envelope distinction - configuration/provider-keys: warn about plaintext secret storage; replace the inaccurate OpenAI api_base normalization claim with a per-provider truth table (refs #270) - operations/health-checks: rewrite for the /livez liveness contract plus the /admin/v1/health per-model shape (per #256 + #257) - reference/proxy-api-reference: /livez instead of /health in the route list - tutorials/build-a-virtual-model-with-failover: rewrite end-to-end against routing-strategies-e2e (deliberately break primary, observe cooldown) - tutorials/enable-response-caching: rewrite against cache-policy-e2e (x-aisix-cache miss then hit, different prompt misses again) - tutorials/add-keyword-guardrails: rewrite against guardrail-keyword-e2e (422 content_filter; no-leak message contract per #203) - tutorials/openai-client-to-anthropic-upstream: rewrite against anthropic-upstream-e2e; document bare-host api_base for Anthropic Every command, field, header, and error code was cross-checked against the relevant crate or e2e test on this branch. No mock data; nothing speculative.
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 freeto join this conversation on GitHub.
Already have an account?
Sign in to comment
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.
Closes#153.
Problem
When a
kind: "keyword"guardrail (input or outputhook_point) blocks a request, the gateway's caller-visibleerror.messageechoes the matched literal verbatim:{ "error": { "message": "content blocked by policy: output blocked by literal \"leakedsecret\"", "type": "content_filter" } }Severity: for output guardrails this is a real bypass — the entire purpose of an output guardrail is to keep forbidden content from reaching the caller, and echoing the matched literal in the error envelope is a partial bypass: anyone who can trigger the rule can extract the model's forbidden output by inspecting error responses. For input guardrails the leak still enables blocklist enumeration (probe with suspect content, read the reflected literal). Labeled
vulnerabilityin the issue.Fix
Redact at the wire boundary, keep rich detail in operator logs.
ProxyError::ContentFiltered's Display impl changes from"content blocked by policy: {0}"to"{0}"so the constructor fully controls the wire string.Both construction sites in
crates/aisix-proxy/src/chat.rs(input at L299, output at L651) now build a generic static message and emit the verdict's richreason(which carries the matched-pattern detail) viatracing::warn!:Wire-level messages:
"request blocked by content policy""response blocked by content policy"Tests
Unit:
input_guardrail_block_returns_422_and_skips_upstream: replaced.contains("forbidden-token")(pinned the leaky behavior) with!message.contains("forbidden-token")+ exact-match on the redacted string.output_guardrail_block_returns_422_after_upstream_runs: asserts the matched literal"secret-string"does NOT appear in ANY field of the wire envelope (full-blob substring check), and that the message exactly equals the redacted string.E2E (regression): restored
tests/e2e/src/cases/guardrail-output-e2e.test.tsfrom the held-back queue. Configures an output keyword guardrail withvalue: "leakedsecret", sends an innocent prompt, has the mock upstream emit a response containing the forbidden literal, and asserts:error.type === "content_filter"JSON.stringify(caught.error).not.toContain(FORBIDDEN_WORD)caught.message.not.toContain(FORBIDDEN_WORD)upstream.receivedRequests.lengthincreased (output guardrails run post-dispatch)Verification
cargo test -p aisix-proxy --lib: 141/141 passing (post-rebase on main with fix(proxy): record real token counts on streamed + output-blocked chats #196)cargo clippy -p aisix-proxy --lib --tests -- -D warnings: cleancargo fmt --check: cleanpnpm tsc --noEmit(e2e): cleanOut of scope (filed as #199, #204)
BridgeError'sDisplayimpl bleeds upstream provider error text and parser-detail strings intoerror.messagesystem-wide (across both streaming and non-streaming paths). The fix shape is similar (sanitize at the proxy boundary, keep rich detail in tracing), but it's a separate scope from this guardrail-specific fix.hook_point: "output") only fire on the non-streaming path. A caller can bypass output guardrails by addingstream: trueto their request. Filed as a separatevulnerabilityissue with repro shape and proposed fix shapes.Audit follow-ups (post-push, commit 6d3a9c6)
Independent audit on the initial push (commit 041fcff) surfaced four findings; resolved or justified:
HIGH-1 → fixed: rebased onto current main (post-fix(proxy): record real token counts on streamed + output-blocked chats #196). PR fix(proxy): record real token counts on streamed + output-blocked chats #196 changed the output-block error tuple from
Err((Option<String>, ProxyError))toErr((Option<String>, Option<UpstreamCharge>, ProxyError))to plumb upstream-billed token counts. The redaction now lands inside the new tuple shape atcrates/aisix-proxy/src/chat.rs:823while keeping theUpstreamChargecapture intact. Without this rebase, my initial hunk would not have applied cleanly and the leak would have silently re-introduced post-merge.HIGH-2 → fixed: tightened
tests/e2e/src/cases/guardrail-keyword-e2e.test.ts(input-side e2e) to assert the matched literal is NOT incaught.errorJSON orcaught.message— symmetric to the new output-side e2e. Without this, a regression that re-introduced leakage on the input path would have passed silently. Replaced.toMatchObject({ status, error: { type } })with explicit try/catch + class+envelope assertions.MEDIUM-1 → filed as bug: output guardrail does not run on streaming responses (silent bypass via stream:true) #204: streaming output path has zero output-guardrail coverage (see "Out of scope" above).
MEDIUM-2 → operator note: the PR moves the matched literal from HTTP error body to a
tracing::warn!(reason = %reason, ...)field. If a deployment configurestracing-opentelemetryto export warn-level events to a backend visible to callers (e.g. shared OTel tenants, or admin APIs that surface recent log lines to non-admin viewers), the literal could reappear caller-visible. Operator guidance: ensuretracingexporters are server-only; warn-level guardrail-block events should not be forwarded to caller-visible channels. Worth covering in deployment docs.LOW-1 → noted:
ProxyError::ContentFiltered's Display impl changed from"content blocked by policy: {0}"to"{0}". Customers substring-matching on the OLD prefix"content blocked by policy"will silently fail to detect blocks. Status (422) anderror.type(content_filter) are unchanged, so programmatic clients keying off the OpenAI taxonomy are unaffected. The redacted strings ("request blocked by content policy","response blocked by content policy") are reasonable replacements. Note for release notes.References
bug,vulnerability)Summary by CodeRabbit
Bug Fixes
Tests
Documentation