Skip to content

fix(guardrails): run the input chain on every request that reaches an upstream - #1064

Merged
jarvis9443 merged 2 commits into
mainfrom
fix/guardrail-family-coverage
Aug 28, 2026
Merged

fix(guardrails): run the input chain on every request that reaches an upstream#1064
jarvis9443 merged 2 commits into
mainfrom
fix/guardrail-family-coverage

Conversation

@jarvis9443

Copy link
Copy Markdown
Contributor

Release QA on v0.11.0-rc.5 found two ways a request could reach an upstream without an input guardrail verdict. Both reproduce on rc.4, so they are pre-existing rather than a regression in this release. Auditing the rest of the routed surface turned up four more instances of the same defect, so they are all fixed here.

The shared mistake

Every one of these treats "nothing to scan" as "nothing to decide". That confusion is only invisible while every guardrail is a text matcher. A kind: custom policy script is not: it can block on ctx.model, on a secret-backed lookup, or unconditionally, and only the guardrail itself can tell "I matched nothing" apart from "I was never asked". The call site had been making that call on the guardrail's behalf, and getting it wrong.

So the rule is now: the call site always consults the chain; only a guardrail kind may decide it needs text. That is safe for cost because every remote kind already short-circuits empty input on its own — bedrock (texts.iter().all(is_empty)), lakera (returns before call_api), presidio (analyze returns before the HTTP post), aliyun_ai_guardrail (joined.is_empty()). An argument-less MCP call therefore costs one local QuickJS sandbox run and zero provider round-trips.

The two reported bypasses

MCP tools/call with empty arguments executed the tool. mcp::moderate_selected_segments collects the string leaves under params.arguments and returned Keep when it found none. A kind: custom guardrail is a segment moderator, so the non-segment fold above it (check_input_non_segment_observed) deliberately skips it — the segment pass is where it was supposed to be consulted, and that pass bailed out before consulting anything.

/v1/messages/count_tokens ran no chain at all. It now runs the input hook and not the output hook, and the asymmetry is the point. The response is {"input_tokens": <int>} — the provider generated nothing, so an output guardrail has nothing to moderate. The request ships the caller's entire system + messages + tools payload to the provider, which is exactly the transmission a PII / DLP / exfiltration policy exists to govern. The original exemption argued that the same payload is scanned when the caller issues the real /v1/messages call; nothing obliges a caller to ever issue one, so count_tokens alone was a complete egress channel. Mask-action rules rewrite the body here too, which also keeps the answer honest — /v1/messages masks the same spans, so the count now describes the body the gateway would really send.

The rest of the family

Path Input chain before After
/v1/chat/completions ran, but a segment member was skipped when every slot was empty runs
/v1/completions same runs
/v1/responses same runs
/v1/messages same, plus skipped entirely when the Anthropic parse failed runs; unparseable body now fails closed
/v1/messages/count_tokens never ran runs (input only)
/v1/embeddings ran; script skipped on empty input runs
/v1/rerank ran; script skipped on empty input runs
/v1/images/generations ran; script skipped on empty prompt runs
/v1/images/edits skipped when no prompt part runs
/v1/audio/transcriptions skipped when no prompt field (the ordinary shape) runs
/v1/audio/translations same runs
/v1/audio/speech ran; script skipped on empty input runs
/v1/videos ran runs
/v1/realtime ran per text frame; script skipped on an empty frame runs
/v1/files, /v1/batches, /v1/fine_tuning/jobs ran over the blob runs
passthrough routes ran runs
MCP tools/call skipped when arguments had no string leaves runs
/a2a/:agent never ran runs (input only)

Routes with no upstream-bound caller content — /livez, /readyz, /v1/models, the .well-known metadata, and the by-id reads and cancels under videos / files / batches / fine-tuning — legitimately run no chain. Each now records that decision and its reason in the census table.

Two entries are behaviour changes rather than repairs of a skipped check, and both want a release note:

  • /a2a/:agent now screens message text on the input hook. Scoping resolves through env / api-key / team; /a2a carries no model or MCP-server id, so an attachment on one of those scopes still does not apply here. The output hook stays unwired — an A2A answer arrives as artifacts and status updates across a stream that can run for hours, and moderating it needs the streamed-output machinery the LLM surfaces have.
  • /v1/messages with a body the Anthropic parser rejects now returns 422 + unscannable_body instead of forwarding it, and only when a guardrail chain is attached. Every such body is one the provider itself rejects, so this changes the error rather than the outcome — but it stops the guardrail being only as complete as the parser, which is a property that decays every time a provider adds a shape. /mcp already took this arm.

Keeping the family from drifting again

crates/aisix-proxy/src/guardrail_coverage.rs is a census, and nothing in it is hand-listed — that is deliberate, since these bugs survived alongside a test that restated a list of endpoints instead of deriving one.

  1. It parses the routing table out of build_router's own source. Mount a route without classifying it and the test fails, naming the route and telling the author what to decide.
  2. Every classification that is not Enforced must carry a reason, and the test reads it.
  3. Every Enforced surface must carry a request fixture, and each is driven through the real router against a kind: custom guardrail that blocks unconditionally. Bodies are deliberately contentless wherever the wire shape allows — no prompt, "arguments": {}, empty message text — because that is the shape every bug in this class hid behind.

Against the unfixed code the census reports 16 of the 17 enforced surfaces reaching the upstream; the one that blocked is /v1/videos, whose schema rejects an empty prompt so its fixture is the only one carrying text.

Two counterweights keep it honest. fixtures_do_not_self_refuse_without_a_guardrail drives the same fixtures with no guardrail configured, so the pass cannot be coming from unrelated error text. a_text_matching_guardrail_leaves_textless_requests_alone drives them against a keyword rule that matches nothing and requires all of them through — the fix must not turn "no text" into a blanket block.

Tests

  • tests/e2e/src/cases/guardrail-textless-request-e2e.test.ts — real binary + etcd + a real MCP upstream, driving the caller-visible contract of each reported bypass and asserting the recorded upstream never saw the request. Fails 3/4 before the fix, passes 4/4 after; the fourth is the keyword negative control, which passes in both states.
  • guardrail_coverage.rs — 4 census tests, described above.
  • custom.rs — 4 unit tests pinning the empty-text semantics on both hooks and both the check and segment entry points, including that a text-matching script still allows.

Full DP e2e (223 files, 692 tests) green. cargo test --workspace, cargo fmt --all -- --check and cargo clippy --workspace --all-targets -- -D warnings clean.

Docs

The api7/docs guardrail pages need a paired bilingual update; the release owner has the specifics.

… upstream

A request carrying no scannable text was reaching upstreams without a
guardrail verdict. Release QA found two instances on v0.11.0-rc.5; both
reproduce on rc.4, and an audit of the whole routed surface found the
same defect in four more places.

The shared mistake is treating "nothing to scan" as "nothing to decide".
A guardrail that matches text has legitimately found nothing; a guardrail
that decides about the CALL — a `kind: custom` policy script — has a
verdict either way, and only it can tell the two apart. That judgment
belongs to the guardrail, never to the call site.

Fixed, in the order a request meets them:

* `custom.rs` returned Allow before running the script when every message
  was empty (and likewise on an empty response).
* `redact::moderate_body` and `mcp::moderate_selected_segments` returned
  early when the collect walk found zero slots, so on the chat / messages
  / responses / completions / MCP families a segment-moderating member —
  which is every custom script — was never consulted at all.
* `/v1/audio/transcriptions`, `/v1/audio/translations` and
  `/v1/images/edits` ran the chain only `if !prompt_messages.is_empty()`,
  i.e. never on the ordinary shape of those endpoints.
* `/v1/messages/count_tokens` ran no chain at all.
* `/a2a/:agent` ran no chain at all.
* `/v1/messages` skipped the check when its Anthropic parse failed,
  making the guardrail only as complete as the parser.

No new provider round-trips: every remote kind (bedrock, lakera,
presidio, aliyun) already short-circuits empty input on its own, so an
argument-less MCP call costs one local sandbox run and no network call.

`count_tokens` gets the input hook but not the output hook. Its response
is an integer the provider generated nothing for, so there is nothing to
moderate on the way back; its REQUEST ships the caller's whole `system` +
`messages` + `tools` payload to the provider, which is exactly what a PII
or exfiltration policy exists to govern. The prior exemption argued the
payload gets scanned on the real `/v1/messages` call, but nothing obliges
a caller to make one.

`crates/aisix-proxy/src/guardrail_coverage.rs` is the anti-drift half.
It parses the routing table out of `build_router`'s own source, requires
every mounted surface to carry an explicit posture with a reason, and
drives each enforced one through the real router against a guardrail that
blocks unconditionally. Against the unfixed code it reports 16 of 17
surfaces reaching the upstream.
@coderabbitai

coderabbitai Bot commented Aug 28, 2026

Copy link
Copy Markdown

Warning

Review limit reached

  • Run on-demand review

On-demand reviews are free for the next 24 days. After that, they cost $0.25 per reviewed file.

Or wait 37 minutes for your next included review.

View limit details

Limit details: You’ve used the included review currently available. Your 60 included PR review attempts over the past 7 days set your current allowance at 1 review per hour.

Learn how review limits work.

Review configuration:

⚙️ Run configuration

Configuration used: Organization UI

Review profile: CHILL

Plan: Pro

Run ID: 2e594383-88a5-4d93-b48f-71041f32e7b9

📥 Commits

Reviewing files that changed from the base of the PR and between 4dd6e05 and c875d02.

📒 Files selected for processing (12)
  • CLAUDE.md
  • crates/aisix-guardrails/src/custom.rs
  • crates/aisix-proxy/src/a2a.rs
  • crates/aisix-proxy/src/audio.rs
  • crates/aisix-proxy/src/count_tokens.rs
  • crates/aisix-proxy/src/guardrail_coverage.rs
  • crates/aisix-proxy/src/images_edits.rs
  • crates/aisix-proxy/src/lib.rs
  • crates/aisix-proxy/src/mcp.rs
  • crates/aisix-proxy/src/messages.rs
  • crates/aisix-proxy/src/redact.rs
  • tests/e2e/src/cases/guardrail-textless-request-e2e.test.ts

Comment @coderabbitai help to get the list of available commands.

@nic-6443
nic-6443 requested a lite review from Copilot August 28, 2026 04:33

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

@jarvis9443
jarvis9443 merged commit 722cfc9 into main Aug 28, 2026
15 checks passed
@jarvis9443
jarvis9443 deleted the fix/guardrail-family-coverage branch August 28, 2026 04:54
jarvis9443 added a commit that referenced this pull request Aug 28, 2026
#1064 wired the input hook into `/a2a/:agent`, which lands on the same
gap this branch closes elsewhere: the refusal emits one usage event and
left `guardrail_blocked` defaulted, so the Blocked view could never see
it. `/a2a` emits exactly one event per call, so that row is the only
place the refusal can appear at all.

Its token counters stay as they are. They are the gateway's own reading
of the request words — filled before the chain runs, flagged
`usage_estimated`, never charged — so unlike the LLM surfaces a refused
A2A call is not expected to report zero, and the census records that
exemption rather than asserting past it.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants