fix(guardrails): stop a broken custom script looking exactly like an enforcing policy - #1062
Merged
Merged
Conversation
…ng policy
A kind=custom script returning `{action: "allow"}` refused every request
with `422 request blocked by content policy (guardrail '<name>')` — byte
for byte what a correctly-firing policy returns. `allow` is not one of the
accepted actions, so it fell into the unknown-action arm, which is a
script FAILURE, and this release's fail-closed default turns a failure
into a block. The same held for any unrecognised action, for `{}`/`null`,
and for a hook with no `return` at all: every one of them collapsed to
`custom_bad_verdict`, and nothing in the response, the logs, or a
dashboard said "your script is broken" rather than "your policy is busy".
The defect is the indistinguishability, so that is what is fixed:
- The refusal still happens. A hook that produced no usable verdict has
screened nothing, and reading that as consent is the open door
`fail_open: false` exists to close.
- Every block site in aisix-proxy now threads the verdict's `unavailable`
tag into `guardrail_block_message` / the new `guardrail_block_error`,
so an availability refusal says the guardrail could not evaluate the
request and names the tag, while a content decision keeps its wording
unchanged. Status and `error.type` are untouched — both really are
guardrail refusals, and an alert on `content_filter` keeps counting
both — with `error.code = "guardrail_unavailable"` added for SDKs.
This covers every kind, so a Lakera/Presidio/Azure outage on a
fail-closed row stops claiming a content match too.
- The failure causes are separate tags: `custom_unknown_action` (a word
we do not know) and `custom_no_verdict` (no decision at all) join
`custom_bad_verdict`, so the histogram's `error_type` and the audit
hit's `blocked_unavailable` name the operator's actual mistake.
- The logs name the offending action AND the accepted vocabulary.
- `allow` is accepted as a synonym of `none`. The vocabulary stays closed
at `none | allow | block | mask`; anything else is reported, not
guessed at.
Two refusals the proxy raises on a guardrail's behalf — a streamed
response outgrowing the hold-back cap, and a failed mask write-back —
carry a tag for the same reason.
|
Warning Review limit reached
On-demand reviews are free for the next 24 days. After that, they cost $0.25 per reviewed file. Or wait 41 minutes for your next included review. View limit detailsLimit details: You’ve used all 2 included reviews currently available. Your 59 included PR review attempts over the past 7 days set your current allowance at 2 reviews per hour. Review configuration: ⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (22)
Comment |
The two /mcp fail-closed refusals that happen because the body could not be WALKED — an unparseable tools/call result, a failed collect walk — are not mask write-back failures. Give them their own tag so the caller's message and the operator's dashboard say what actually went wrong.
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
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.
Problem
A
kind: customguardrail whose script returns{action: "allow"}refuses every request withwhich is byte for byte what a correctly-firing policy returns.
allowis not one of the accepted actions (none/block/mask), so it lands in the unknown-action arm — a script failure — and this release's fail-closed default (#1040) turns a failure into a block.The same held for any unrecognised action, for
{}/null, and for a hook with noreturnat all. All five collapsed to one tag,custom_bad_verdict, and nothing the operator could see — the response, the logs, or a dashboard — said "your script is broken" rather than "your policy is busy". Every link is individually defensible; the composite means a one-word slip takes an operator's traffic down while looking exactly like the feature working.Reproduced on
main(d3402d3) before the fix: 5 of the 6 new e2e assertions fail, the one that passes being the control (a genuine block).What changed
The refusal still happens. A hook that produced no usable verdict has screened nothing, and reading that as consent is the open door
fail_open: falseexists to close. What was wrong was how the refusal is reported.The caller can tell the two apart. Every guardrail block site in
aisix-proxyalready funnelled througherror::guardrail_block_message; it now takes the verdict'sunavailabletag (whichGuardrailVerdict::Blockhas carried since AISIX-Cloud#1365 but nothing on the wire read), and a newguardrail_block_errorwraps it for theProxyErrorsites:error.typeerror.coderequest blocked by content policy (guardrail 'g')content_filterrequest rejected: guardrail 'g' could not evaluate it (custom_unknown_action)content_filterguardrail_unavailableStatus and
error.typeare deliberately unchanged: both really are guardrail refusals, an operator alert oncontent_filterkeeps counting both, and the dispatch-loop predicates must not diverge between them. The discrimination lives where it is read — the message, and a stable machine-readable code following the existingwith_codeconvention. Because the seam is the shared helper, this covers every endpoint family at once, and a Lakera/Presidio/Azure outage on a fail-closed row stops claiming a content match too.The failure causes are separate tags.
custom_unknown_action(a word we do not know) andcustom_no_verdict(no decision at all — noreturn,undefined,null, an object with noaction) joincustom_bad_verdict(a return value that is not a verdict object; mask problems). The tag is what a dashboard shows, so collapsing distinct operator mistakes into one catch-all costs the operator the diagnosis. It reachesaisix_guardrail_latency_seconds{error_type=...}and the audit hit'sblocked_unavailableunchanged — those two surfaces were already correct, they just had one tag to show.The logs are actionable. The warning names the offending action and the accepted vocabulary (
none | allow | block | mask), plus that the request is not screened and will be refused unlessfail_openis set.allowis accepted as a synonym ofnone. It is the word most operators reach for first — and the dashboard's own help text for the script field already says the hook "returns an allow, block, or mask verdict", so an operator who wrote{action: "allow"}was following our UI. The vocabulary stays closed at those four — an open-ended synonym list can never be complete, sopass/ok/permitare still reported as the authoring mistake they are, now with a message that says what right looks like.Refusals the proxy raises on a guardrail's behalf carry a tag for the same reason —
output_buffer_exceeded(a streamed response outgrew the hold-back cap),unscannable_body(the body could not be walked, so the guardrail was never offered the content),mask_writeback_failed(a mask verdict could not be spliced back). In each case the content was never screened, so telling the caller its content violated a policy states something that did not happen.Behavior changes
error.codefor a fail-closed availability refusal, on every endpoint family. A content block is unchanged. Worth a release-notes line.error_typelabel values onaisix_guardrail_latency_secondsand on the audit hit:custom_unknown_action,custom_no_verdict. A query oncustom_bad_verdictalone no longer sees the two split-out causes.{action: "allow"}now allows instead of refusing.Tests
tests/e2e/src/cases/guardrail-custom-verdict-diagnostics-e2e.test.ts— new, drives five real scripts through a realaisixbinary + etcd and asserts all three surfaces: the response envelope (message + code), the log line, and the metricerror_type. Includes the control (a real block keeps the content-policy message and carries no code). 5 of 6 fail onmain, all 6 pass here.guardrail-custom-script-e2e.test.ts— the existing screening-outage case now also asserts it does not look like a content block.custom.rs—allowaccepted; the vocabulary stays closed; the "decided nothing" family gets its own tag; a script fault still fails closed and still bypasses (not allows) whenfail_open.error.rs— the two messages differ, status/type are preserved, the code appears only on the availability refusal.cargo fmt --all -- --check,cargo clippy --workspace --all-targets -- -D warnings,cargo test --workspaceand 58 guardrail/mcp/messages/responses/passthrough e2e files (169 tests) pass locally.Not done here
action:also appears in the request bodies these scripts build for their own services — so a lexical check would both miss the real cases and reject good scripts on the save path. The CP's existingcheckInput/checkOutputsubstring check is safe precisely because those are required export names.on_buffer_exceededfail-open path is untouched.