From 11e521a7808bb3d5f107a85fdcff0781ee6c1853 Mon Sep 17 00:00:00 2001 From: Jarvis Date: Fri, 28 Aug 2026 04:31:58 +0000 Subject: [PATCH 1/2] fix(guardrails): run the input chain on every request that reaches an upstream MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- crates/aisix-guardrails/src/custom.rs | 86 +- crates/aisix-proxy/src/a2a.rs | 108 ++- crates/aisix-proxy/src/audio.rs | 9 +- crates/aisix-proxy/src/count_tokens.rs | 125 ++- crates/aisix-proxy/src/guardrail_coverage.rs | 733 ++++++++++++++++++ crates/aisix-proxy/src/images_edits.rs | 9 +- crates/aisix-proxy/src/lib.rs | 2 + crates/aisix-proxy/src/mcp.rs | 24 +- crates/aisix-proxy/src/messages.rs | 92 ++- crates/aisix-proxy/src/redact.rs | 11 +- .../guardrail-textless-request-e2e.test.ts | 320 ++++++++ 11 files changed, 1448 insertions(+), 71 deletions(-) create mode 100644 crates/aisix-proxy/src/guardrail_coverage.rs create mode 100644 tests/e2e/src/cases/guardrail-textless-request-e2e.test.ts diff --git a/crates/aisix-guardrails/src/custom.rs b/crates/aisix-guardrails/src/custom.rs index c74ff066..82e49589 100644 --- a/crates/aisix-guardrails/src/custom.rs +++ b/crates/aisix-guardrails/src/custom.rs @@ -610,9 +610,14 @@ impl Guardrail for CustomGuardrail { }) .filter(|m| !m.text.is_empty()) .collect(); - if messages.is_empty() { - return GuardrailVerdict::Allow; - } + // No early return when there is nothing to scan. A script is a + // policy over the CALL, not a text matcher: `checkInput` may block + // on `ctx.model`, on a secret-backed lookup, or unconditionally, + // and an operator who wrote such a rule expects it to decide a + // request that happens to carry no text (an argument-less MCP tool + // call, a transcription with no `prompt`). Running a local sandbox + // on an empty context is cheap; the remote kinds keep their own + // empty-text guards, so this costs no provider round-trip. let segments: Vec = messages.iter().map(|m| m.text.clone()).collect(); let text = segments.join("\n"); let ctx = ScriptContext { @@ -635,9 +640,12 @@ impl Guardrail for CustomGuardrail { return GuardrailVerdict::Allow; } let text = resp.guardrail_output_text(); - if text.is_empty() { - return GuardrailVerdict::Allow; - } + // Same rule as `check_input`: an empty response still gets a + // verdict. This hook is reached once per response by the call + // sites that use the plain `check_output` fold (jobs, passthrough, + // realtime frames) — the LLM and streaming families route a + // segment moderator through `moderate_output_segments` instead — + // so it adds at most one sandbox run to a response with no text. let messages = [ScriptMessage { role: aisix_gateway::Role::Assistant, text: text.clone(), @@ -1318,6 +1326,72 @@ mod tests { ); } + /// A script's verdict is a decision about the CALL, not a text match, + /// so it must be consulted even when the request carries nothing to + /// scan. This is the bug that let an MCP `tools/call` with + /// `"arguments": {}` — and an audio upload with no `prompt` — execute + /// under an unconditional block rule. + #[tokio::test] + async fn empty_input_still_reaches_the_script() { + let cfg = config("export function checkInput() { return { action: 'block' }; }"); + let g = guardrail(&cfg, false); + for req in [ + request(""), + ChatFormat::new("gpt-4o", vec![]), + ChatFormat::new("gpt-4o", vec![ChatMessage::user(""), ChatMessage::user("")]), + ] { + let verdict = g.check_input(&req).await; + assert!( + matches!(verdict, GuardrailVerdict::Block { .. }), + "an unconditional block must fire with no text to scan: {verdict:?}", + ); + } + } + + /// The segment hook — the path the MCP and LLM families take for a + /// segment-moderating member — has the same obligation with zero slots. + #[tokio::test] + async fn empty_segment_list_still_reaches_the_script() { + let cfg = config("export function checkInput() { return { action: 'block' }; }"); + let outcome = guardrail(&cfg, false).moderate_input_segments(&[]).await; + assert!( + matches!(outcome.verdict, GuardrailVerdict::Block { .. }), + "{:?}", + outcome.verdict, + ); + } + + /// The other half of the same rule: a script that DOES look at the text + /// must still see an empty context as a clean one, so removing the + /// short-circuit cannot turn "no text" into a spurious block. + #[tokio::test] + async fn empty_input_allows_a_text_matching_script() { + let cfg = config( + "export function checkInput(ctx) { + return ctx.text.includes('bomb') ? { action: 'block' } : { action: 'none' }; + }", + ); + let verdict = guardrail(&cfg, false).check_input(&request("")).await; + assert!(matches!(verdict, GuardrailVerdict::Allow), "{verdict:?}"); + } + + #[tokio::test] + async fn empty_output_still_reaches_the_script() { + let cfg = config("export function checkOutput() { return { action: 'block' }; }"); + let resp = aisix_gateway::ChatResponse { + id: String::new(), + model: "gpt-4o".into(), + message: aisix_gateway::ChatMessage::assistant(String::new()), + finish_reason: aisix_gateway::FinishReason::Stop, + usage: aisix_gateway::UsageStats::default(), + }; + let verdict = guardrail(&cfg, false).check_output(&resp).await; + assert!( + matches!(verdict, GuardrailVerdict::Block { .. }), + "{verdict:?}", + ); + } + #[tokio::test] async fn a_hook_the_module_does_not_export_allows() { let cfg = config("export function checkOutput() { return { action: 'block' }; }"); diff --git a/crates/aisix-proxy/src/a2a.rs b/crates/aisix-proxy/src/a2a.rs index 952aa3aa..cb1d0505 100644 --- a/crates/aisix-proxy/src/a2a.rs +++ b/crates/aisix-proxy/src/a2a.rs @@ -13,7 +13,14 @@ //! control (the key's `allowed_agents`), rate-limit + budget (`quota::enforce`), //! and a usage event into the shared sink. The upstream credential is held //! gateway-side and never reaches the caller. Guardrails over A2A message -//! content are a later step. +//! content run on the INPUT hook: the caller's message text is screened +//! before the agent is contacted, on the same env / api-key / team scopes +//! an LLM request resolves. `/a2a` carries no model or MCP-server id, so a +//! guardrail attached to one of those scopes does not apply here (the +//! attachment schema says as much) — an env-wide DLP rule does. The OUTPUT +//! hook is not wired: an A2A answer arrives as artifacts and status updates +//! across a stream that may run for hours, and moderating it needs the +//! streamed-output machinery the LLM surfaces have, not a one-shot check. //! //! The request body is forwarded verbatim to the upstream agent, so the caller //! speaks whichever A2A wire version the agent is pinned to; the gateway does @@ -272,6 +279,27 @@ async fn dispatch( // records which task the caller was asking about. call.facts.observe_request(&value); + // Input guardrails. Before the reservation, like every other surface + // (#542): a content-policy refusal must not burn an RPM slot. This + // endpoint used to run no chain at all, so an operator's env-wide + // policy was a no-op the moment a caller switched from `/v1/*` to + // `/a2a/*` with the same key. + if let Some(response) = guardrail_block_response( + state, + &snapshot, + &auth, + request_id, + agent, + &call, + &value, + rpc_id.clone(), + trace.as_ref(), + ) + .await + { + return response; + } + // Reuse the LLM path's rate-limit + budget gate. The reservation is held // for the call and released without committing tokens: the counts this // endpoint reports are the gateway's own reading of the words, not an @@ -679,6 +707,84 @@ fn a2a_error_status(err: &A2aError) -> StatusCode { } } +/// Screen the caller's A2A message text on the input hook. `Some(response)` +/// = blocked, and the blocked call is recorded as an undispatched usage +/// event the way a quota refusal is. +/// +/// The scanned text is `params.message` — the only caller-authored content +/// the protocol carries — extracted with the same walker telemetry uses, but +/// uncapped: the telemetry cap exists to bound a metric, and scanning a +/// prefix would leave the tail of a long message unscreened. The body is +/// already bounded by `request_body_limit_bytes`. +/// +/// Operations that carry no message (`tasks/get`, `tasks/cancel`) still run +/// the chain on empty text. That is the point: a guardrail that decides +/// about the CALL rather than about its words must get to decide. +#[allow(clippy::too_many_arguments)] +async fn guardrail_block_response( + state: &ProxyState, + snapshot: &aisix_core::AisixSnapshot, + auth: &AuthenticatedKey, + request_id: &str, + agent: &str, + call: &A2aCall, + value: &serde_json::Value, + rpc_id: Option, + trace: Option<&std::sync::Arc>, +) -> Option { + let chain = state + .guardrail_index + .resolve(&aisix_guardrails::RequestContext { + passthrough_route_id: "", + model_id: "", + mcp_server_id: "", + api_key_id: &auth.entry.id, + team_id: auth.key().team_id.as_deref(), + }); + if chain.is_empty() { + return None; + } + let text = request_text(value, |buf, s| buf.push_str(s)); + let chat = aisix_gateway::ChatFormat::new( + A2A_MODEL_LABEL, + vec![aisix_gateway::ChatMessage::user(text)], + ); + let (verdict, _hits) = aisix_guardrails::Guardrail::check_input_observed(&chain, &chat).await; + let aisix_guardrails::GuardrailVerdict::Block { + reason, + guardrail_name, + unavailable, + } = verdict + else { + return None; + }; + tracing::warn!( + guardrail_hook = "input", + agent = %agent, + reason = %reason, + "guardrail blocked A2A request", + ); + let message = crate::error::guardrail_block_message( + "request", + guardrail_name.as_deref(), + unavailable.as_deref(), + ); + let response = a2a_error_response(rpc_id, StatusCode::UNPROCESSABLE_ENTITY, &message); + emit_a2a_usage( + state, + snapshot, + auth, + request_id, + agent, + call, + response.status().as_u16(), + Duration::ZERO, + trace, + /* dispatched */ false, + ); + Some(response) +} + /// Build a JSON-RPC error envelope for a gateway-side failure, echoing the /// request id. A2A clients expect a JSON-RPC body, so the failure surfaces as /// an error object they can handle rather than a bare HTTP error. diff --git a/crates/aisix-proxy/src/audio.rs b/crates/aisix-proxy/src/audio.rs index 90c47c45..abaf9620 100644 --- a/crates/aisix-proxy/src/audio.rs +++ b/crates/aisix-proxy/src/audio.rs @@ -925,7 +925,14 @@ async fn multipart_dispatch( .filter(|s| !s.is_empty()) .map(|s| ChatMessage::user(s.to_string())) .collect(); - if !prompt_messages.is_empty() { + // The chain runs whether or not a `prompt` part was supplied. + // Gating on "we found text" made the check a text matcher's + // privilege: a guardrail that decides about the CALL — a policy + // script, an unconditional block scoped to this model — never + // fired on the ordinary shape of this endpoint (an upload with no `prompt`), + // so an operator's rule silently allowed exactly the requests + // that carry nothing to match. + { let chat = aisix_gateway::ChatFormat::new(&model_name, prompt_messages); let (verdict, hits) = aisix_guardrails::Guardrail::check_input_observed(&resolved_chain, &chat).await; diff --git a/crates/aisix-proxy/src/count_tokens.rs b/crates/aisix-proxy/src/count_tokens.rs index b57d4646..772b8ce1 100644 --- a/crates/aisix-proxy/src/count_tokens.rs +++ b/crates/aisix-proxy/src/count_tokens.rs @@ -13,16 +13,24 @@ //! (`/messages/count_tokens`), the absence of streaming, and the tiny //! `{"input_tokens": }` response, which is forwarded verbatim. //! -//! Guardrails: this surface is intentionally **exempt** from the -//! content-moderation guardrail chain (#545). It is a pre-flight sizing -//! call — no content reaches a model and the response is only an integer -//! token count, never generated content — so there is nothing for a -//! content-moderation hook to moderate on either side, and the same -//! `messages` payload is scanned when the caller issues the actual -//! `/v1/messages` request. (A DLP/egress policy is a separate concern: the -//! `messages` are forwarded to the provider's count endpoint here before the -//! real call, so a DLP guardrail attached at env-scope would not see them — -//! tracked in #555, out of scope for #545.) +//! Guardrails: the **input** hook runs here, exactly as on `/v1/messages`; +//! the **output** hook does not (#555, revising #545). +//! +//! The two halves are asymmetric because the endpoint is. The response is +//! `{"input_tokens": }` — the provider generated nothing, so an output +//! guardrail has no content to moderate and running one would be theatre. +//! The REQUEST is a different matter: this route ships the caller's entire +//! `system` + `messages` + `tools` payload to the provider, which is +//! precisely the transmission a PII / DLP / data-exfiltration guardrail +//! exists to govern. The original exemption argued the same payload gets +//! scanned when the caller issues the real `/v1/messages` call — but nothing +//! obliges a caller to ever issue it. `count_tokens` on its own is a +//! complete egress channel, and an operator's input policy was silently not +//! applied to it. +//! +//! Mask-action rules rewrite the body here too, before it is forwarded. That +//! also keeps the answer honest: `/v1/messages` masks the same spans, so the +//! count now describes the body the gateway would really send. //! //! Scope: Anthropic-backed models only. `count_tokens` has no upstream //! equivalent for OpenAI/Gemini/DeepSeek, so a non-Anthropic Model is @@ -70,7 +78,7 @@ pub async fn count_tokens( Err(e) => return e.into_anthropic_response(), }; let started = Instant::now(); - let Json(body) = match body { + let Json(mut body) = match body { Ok(j) => j, // Answer through `reject` — see messages.rs. Err(rej) => { @@ -99,7 +107,7 @@ pub async fn count_tokens( // One snapshot for the whole request (#941) — see `embeddings`. let snapshot = state.snapshot.load(); - match dispatch(&state, &snapshot, &auth, &body, &request_id, &client).await { + match dispatch(&state, &snapshot, &auth, &mut body, &request_id, &client).await { Ok(success) => { let elapsed = started.elapsed(); let status = success.response.status().as_u16(); @@ -164,6 +172,91 @@ pub async fn count_tokens( } } +/// Run the resolved input guardrail chain over the Anthropic-shaped body, +/// blocking before dispatch and writing mask-action rewrites back into +/// `body` (which is what `count_tokens_to_target` forwards upstream). +/// +/// Deliberately mirrors `messages::dispatch_inner`'s block rather than +/// sharing a helper with it: that one also threads applied-guardrail, +/// audit and monitor-hit telemetry into a UsageEvent, and this route emits +/// none (see [`CountTokensSuccess`]). Keeping the shapes parallel is what +/// the `guardrail_coverage` census asserts. +async fn screen_input( + state: &ProxyState, + auth: &AuthenticatedKey, + model_entry_id: &str, + model_name: &str, + body: &mut Value, +) -> Result<(), ProxyError> { + let chain = state + .guardrail_index + .resolve(&aisix_guardrails::RequestContext { + passthrough_route_id: "", + model_id: model_entry_id, + mcp_server_id: "", + api_key_id: &auth.entry.id, + team_id: auth.key().team_id.as_deref(), + }); + if chain.is_empty() { + return Ok(()); + } + // Fail closed on a body the scanner cannot read — see the same arm in + // `messages.rs`. + let chat = match aisix_provider_anthropic::parse_inbound_request(body) { + Ok(chat) => chat, + Err(err) => { + tracing::warn!( + guardrail_hook = "input", + model = %model_name, + error = %err, + "cannot scan /v1/messages/count_tokens body for guardrails; blocking", + ); + return Err(crate::error::guardrail_block_error( + "request", + None, + Some(crate::error::TAG_UNSCANNABLE_BODY), + )); + } + }; + // Monitor-mode hits and redaction counts are collected and dropped: + // this route emits no UsageEvent (see [`CountTokensSuccess`]), so there + // is nothing to attach them to. Both are out-params of the shared + // helpers rather than optional, hence the sinks. + let (verdict, _monitor_hits) = + aisix_guardrails::Guardrail::check_input_non_segment_observed(&chain, &chat).await; + let mut counts = crate::redact::RedactionCounts::new(); + let verdict = crate::redact::moderate_body( + &chain, + crate::redact::Direction::Input, + verdict, + &mut counts, + &mut Vec::new(), + |g| crate::redact::redact_anthropic_request(g, body), + ) + .await; + if let aisix_guardrails::GuardrailVerdict::Block { + reason, + guardrail_name, + unavailable, + } = verdict + { + tracing::warn!( + guardrail_hook = "input", + model = %model_name, + reason = %reason, + "guardrail blocked /v1/messages/count_tokens request", + ); + return Err(crate::error::guardrail_block_error( + "request", + guardrail_name.as_deref(), + unavailable.as_deref(), + )); + } + // Mask-action rules rewrite the body that is about to be forwarded. + crate::redact::redact_anthropic_request(&chain, body); + Ok(()) +} + /// What the winning attempt resolved. `/v1/messages/count_tokens` emits no /// UsageEvent, so the only consumer is the request-metric label set — which /// still has to match what chat / messages / responses report @@ -179,7 +272,7 @@ async fn dispatch( state: &ProxyState, snapshot: &aisix_core::AisixSnapshot, auth: &AuthenticatedKey, - body: &Value, + body: &mut Value, request_id: &str, client: &ClientContext, ) -> Result { @@ -199,6 +292,12 @@ async fn dispatch( // Client-IP allowlist gate (#557): reject before quota / upstream. crate::dispatch::check_ip_access(&model_entry.value, &client.source_ip)?; + // Input guardrails (#555). Same chain, same order and same ordering + // rationale as the `/v1/messages` sibling: before the reservation, so a + // content-policy refusal doesn't burn an RPM slot. See the module doc + // for why the input hook applies here and the output hook does not. + screen_input(state, auth, &model_entry.id, &model_name, body).await?; + let model_rl = crate::quota::ModelRateLimit::from_model(&model_name, &model_entry.id, &model_entry.value); let _reservation = crate::quota::enforce(state, snapshot, auth, Some(&model_rl)).await?; diff --git a/crates/aisix-proxy/src/guardrail_coverage.rs b/crates/aisix-proxy/src/guardrail_coverage.rs new file mode 100644 index 00000000..7fab0499 --- /dev/null +++ b/crates/aisix-proxy/src/guardrail_coverage.rs @@ -0,0 +1,733 @@ +//! A census of the input guardrail chain over every surface +//! [`crate::build_router`] mounts. +//! +//! This exists because the same defect kept landing on one sibling at a +//! time. `/v1/messages/count_tokens` shipped with no chain at all; `/mcp` +//! consulted the chain only when a `tools/call` happened to carry text; +//! `/v1/audio/*` and `/v1/images/edits` only when a `prompt` part happened +//! to be present; `/a2a` never. Each was invisible because the test that +//! was supposed to cover the family restated a hand-written list of +//! endpoints, and a list nobody updates agrees with itself forever. +//! +//! So nothing here is hand-listed. [`mounted_surfaces`] reads the routing +//! table out of `lib.rs` itself, and [`POSTURE`] must classify exactly +//! that set — mount a route without saying what it owes an operator's +//! guardrail chain and `posture_covers_every_mounted_surface` fails. A +//! surface classified [`Posture::Enforced`] must also carry a request +//! fixture, and `enforced_surfaces_refuse_a_blocking_guardrail` drives +//! each one through the real router against a guardrail that blocks +//! unconditionally. Nothing short of an actual refusal passes. +//! +//! The blocking guardrail is a `kind: custom` script, deliberately: it is +//! the one kind whose verdict does not depend on the text, so it is the +//! only one that can tell "the chain ran and decided" apart from "the +//! chain found nothing to match". That distinction is the whole bug class. + +use std::collections::BTreeSet; +use std::sync::Arc; + +use aisix_core::snapshot::SnapshotHandle; +use aisix_core::{AisixSnapshot, ApiKey, ProxyConfig, ResourceEntry}; +use axum::body::Body; +use axum::http::Request; +use tower::ServiceExt; + +/// The source of the routing table. Parsed rather than duplicated so a new +/// `.route(...)` cannot slip past this file. +const ROUTER_SRC: &str = include_str!("lib.rs"); + +/// What a mounted surface owes an operator's INPUT guardrail chain. +#[derive(Debug, Clone, Copy)] +enum Posture { + /// Caller-authored content reaches an upstream here, so the chain runs + /// and can refuse the request before the upstream is contacted. Every + /// such surface is driven for real below. + Enforced, + /// The chain runs, but the surface cannot be exercised through a + /// `oneshot` against the router. Carries why, and where it is covered. + EnforcedNotDrivableInCrate(&'static str), + /// Nothing a caller authored reaches an upstream, so there is nothing + /// for an input hook to screen. Carries why. + NoUpstreamContent(&'static str), +} + +/// Every surface `build_router` mounts, and its posture. The set of keys is +/// checked against the parsed routing table, so this cannot silently fall +/// behind the router. +const POSTURE: &[(&str, Posture)] = &[ + // --- liveness / discovery: no caller content leaves the gateway ----- + ("/livez", Posture::NoUpstreamContent("liveness probe")), + ("/readyz", Posture::NoUpstreamContent("readiness probe")), + ( + "/v1/models", + Posture::NoUpstreamContent("lists the gateway's own snapshot; contacts no upstream"), + ), + ( + "/.well-known/oauth-protected-resource", + Posture::NoUpstreamContent("RFC 9728 metadata served by the gateway itself"), + ), + ( + "/.well-known/oauth-protected-resource/mcp", + Posture::NoUpstreamContent("RFC 9728 metadata served by the gateway itself"), + ), + ( + "/a2a/:agent/.well-known/agent-card.json", + Posture::NoUpstreamContent("serves the agent's card with the URL rewritten; no caller body"), + ), + // --- reads of a job/asset the caller already created ---------------- + ( + "/v1/videos/:id", + Posture::NoUpstreamContent("polls a job by id; the prompt was screened at creation"), + ), + ( + "/v1/videos/:id/content", + Posture::NoUpstreamContent("fetches a rendered asset by id; carries no caller text"), + ), + ( + "/v1/files/:id", + Posture::NoUpstreamContent("GET/DELETE by id; carries no caller text"), + ), + ( + "/v1/files/:id/content", + Posture::NoUpstreamContent("downloads by id; carries no caller text"), + ), + ( + "/v1/batches/:id", + Posture::NoUpstreamContent("GET by id; carries no caller text"), + ), + ( + "/v1/batches/:id/cancel", + Posture::NoUpstreamContent("cancels by id; carries no caller text"), + ), + ( + "/v1/fine_tuning/jobs/:id", + Posture::NoUpstreamContent("GET by id; carries no caller text"), + ), + ( + "/v1/fine_tuning/jobs/:id/cancel", + Posture::NoUpstreamContent("cancels by id; carries no caller text"), + ), + // --- content-bearing surfaces -------------------------------------- + ("/v1/chat/completions", Posture::Enforced), + ("/v1/completions", Posture::Enforced), + ("/v1/embeddings", Posture::Enforced), + ("/v1/images/generations", Posture::Enforced), + ("/v1/images/edits", Posture::Enforced), + ("/v1/messages", Posture::Enforced), + ("/v1/messages/count_tokens", Posture::Enforced), + ("/v1/rerank", Posture::Enforced), + ("/v1/responses", Posture::Enforced), + ("/v1/audio/transcriptions", Posture::Enforced), + ("/v1/audio/translations", Posture::Enforced), + ("/v1/audio/speech", Posture::Enforced), + ("/v1/videos", Posture::Enforced), + ("/mcp", Posture::Enforced), + ("/mcp/", Posture::Enforced), + ("/mcp/:server", Posture::Enforced), + ("/a2a/:agent", Posture::Enforced), + ( + "/v1/realtime", + Posture::EnforcedNotDrivableInCrate( + "WebSocket upgrade; the per-frame scan (realtime.rs `guardrail_block_event`) needs a \ + live socket. Covered by the realtime e2e suite.", + ), + ), + ( + "/v1/files", + Posture::EnforcedNotDrivableInCrate( + "`jobs::scan_input_blob` screens the uploaded blob, but reaching it needs a resolvable \ + job target plus a multipart upload the mock upstream must accept. Covered by the jobs \ + e2e suite.", + ), + ), + ( + "/v1/batches", + Posture::EnforcedNotDrivableInCrate( + "same `jobs::scan_input_blob` gate; POST needs an `input_file_id` that resolves \ + through a prior upload.", + ), + ), + ( + "/v1/fine_tuning/jobs", + Posture::EnforcedNotDrivableInCrate( + "same `jobs::scan_input_blob` gate; POST needs a `training_file` that resolves \ + through a prior upload.", + ), + ), + ( + FALLBACK_SURFACE, + Posture::EnforcedNotDrivableInCrate( + "`passthrough_route::entry`; a passthrough route matches on a configured path prefix \ + or Host, so there is no fixed path to drive here. Covered by \ + passthrough-guardrail-e2e.", + ), + ), +]; + +/// The router's `.fallback(...)` seat, which has no path literal of its own. +const FALLBACK_SURFACE: &str = ""; + +/// Pull every surface `build_router` mounts out of its own source: the +/// `.route("", ...)` literals, plus the `.fallback(...)` seat. +/// +/// Scoped to the function body so unrelated `.route(` calls elsewhere in +/// `lib.rs` (tests, doc examples) cannot pad the census. +fn mounted_surfaces() -> BTreeSet { + let start = ROUTER_SRC + .find("pub fn build_router(") + .expect("build_router must exist in lib.rs"); + // The function ends at the first line that closes at column 0. + let body = &ROUTER_SRC[start..]; + let end = body + .find("\n}\n") + .expect("build_router must be brace-balanced at column 0"); + let body = &body[..end]; + + let mut found = BTreeSet::new(); + for (idx, _) in body.match_indices(".route(") { + let rest = &body[idx + ".route(".len()..]; + // The path is the next string literal; `.route(` is always called + // with one in this router. + let open = rest.find('"').expect(".route( must take a path literal"); + let close = open + + 1 + + rest[open + 1..] + .find('"') + .expect("unterminated route path literal"); + found.insert(rest[open + 1..close].to_string()); + } + if body.contains(".fallback(") { + found.insert(FALLBACK_SURFACE.to_string()); + } + found +} + +#[test] +fn posture_covers_every_mounted_surface() { + let mounted = mounted_surfaces(); + let declared: BTreeSet = POSTURE.iter().map(|(p, _)| (*p).to_string()).collect(); + + // Sanity: the parse must actually find the router, not silently yield + // an empty set that makes both assertions below vacuous. + assert!( + mounted.len() > 20, + "the routing-table parse found only {} surfaces — it has stopped tracking build_router", + mounted.len() + ); + + let unclassified: Vec<_> = mounted.difference(&declared).collect(); + assert!( + unclassified.is_empty(), + "these surfaces are mounted in build_router but have no guardrail posture declared in \ + POSTURE: {unclassified:?}\n\ + Say what each owes an operator's input guardrail chain. If content a caller wrote can \ + reach an upstream through it, the answer is Posture::Enforced and it needs a fixture in \ + `drive`.", + ); + + let stale: Vec<_> = declared.difference(&mounted).collect(); + assert!( + stale.is_empty(), + "POSTURE classifies surfaces build_router no longer mounts: {stale:?}", + ); + + // An exemption is only worth anything if it says why. A bare + // "not enforced here" is how the previous gaps read to every reviewer + // who looked at them. + for (surface, posture) in POSTURE { + let reason = match posture { + Posture::Enforced => continue, + Posture::EnforcedNotDrivableInCrate(reason) | Posture::NoUpstreamContent(reason) => { + reason + } + }; + assert!( + !reason.trim().is_empty(), + "{surface} is exempted from the enforced set with no reason given", + ); + } +} + +// --------------------------------------------------------------------------- +// The behavioural half: drive every Enforced surface against a guardrail +// that blocks unconditionally, and require an actual refusal. +// --------------------------------------------------------------------------- + +const CALLER: &str = "sk-census-caller"; +/// SHA-256 of `CALLER`. +const CALLER_HASH: &str = "d73b98669c1f938ed09ee3d8e81ecdbb58f7bf38c57a4f78c301e7bdadc2fdf2"; +const GUARDRAIL_ROW: &str = "census-block"; +const PK_ID: &str = "11111111-1111-1111-1111-111111111111"; +const ANTHROPIC_PK_ID: &str = "22222222-2222-2222-2222-222222222222"; +const MCP_SERVER_ID: &str = "33333333-3333-3333-3333-333333333333"; + +/// A script with a verdict that owes nothing to the text it is handed. A +/// keyword or PII row could not distinguish "the chain ran" from "the chain +/// matched nothing", which is precisely the confusion this census exists to +/// rule out. +const BLOCK_EVERYTHING: &str = r#" +export function checkInput() { + return { action: "block", reason_code: "census" }; +} +"#; + +fn cfg() -> ProxyConfig { + ProxyConfig { + addr: "127.0.0.1:0".into(), + request_body_limit_bytes: 1_048_576, + real_ip: Default::default(), + request_id: Default::default(), + url_rewrites: Vec::new(), + tls: None, + thread_per_core: None, + workers: None, + } +} + +/// A snapshot wired so every content-bearing surface can resolve what it +/// needs (key, models, MCP server, A2A agent) and reach its guardrail gate. +/// The upstreams deliberately point nowhere: a surface that reaches one has +/// already failed the test. +fn census_snapshot() -> AisixSnapshot { + let snap = AisixSnapshot::new(); + + let key: ApiKey = serde_json::from_value(serde_json::json!({ + "key_hash": CALLER_HASH, + "allowed_models": ["*"], + "allowed_routes": ["*"], + "allowed_agents": ["*"], + "mcp_access": { "allow": ["*"] }, + })) + .expect("valid api key"); + snap.apikeys.insert(ResourceEntry::new("ak-census", key, 1)); + + let pk: aisix_core::ProviderKey = serde_json::from_value(serde_json::json!({ + "display_name": "census-openai", + "secret": "sk-unused", + "api_base": "http://127.0.0.1:1/v1", + "provider": "openai", + "adapter": "openai", + })) + .expect("valid provider key"); + snap.provider_keys.insert(ResourceEntry::new(PK_ID, pk, 1)); + + let anthropic_pk: aisix_core::ProviderKey = serde_json::from_value(serde_json::json!({ + "display_name": "census-anthropic", + "secret": "sk-ant-unused", + "api_base": "http://127.0.0.1:1", + "provider": "anthropic", + "adapter": "anthropic", + })) + .expect("valid provider key"); + snap.provider_keys + .insert(ResourceEntry::new(ANTHROPIC_PK_ID, anthropic_pk, 1)); + + for (id, name, provider, model_name, pk_id) in [ + ("m-openai", "census-openai", "openai", "gpt-4o-mini", PK_ID), + ( + "m-anthropic", + "census-anthropic", + "anthropic", + "claude-haiku-4-5-20251001", + ANTHROPIC_PK_ID, + ), + ] { + let model: aisix_core::Model = serde_json::from_value(serde_json::json!({ + "display_name": name, + "provider": provider, + "model_name": model_name, + "provider_key_id": pk_id, + })) + .expect("valid model"); + snap.models.insert(ResourceEntry::new(id, model, 1)); + } + + let embedding: aisix_core::Model = serde_json::from_value(serde_json::json!({ + "display_name": "census-embedding", + "provider": "openai", + "model_name": "text-embedding-3-small", + "provider_key_id": PK_ID, + "kind": "embedding", + })) + .expect("valid embedding model"); + snap.models + .insert(ResourceEntry::new("m-embedding", embedding, 1)); + + let mcp: aisix_core::McpServer = serde_json::from_value(serde_json::json!({ + "display_name": "census", + "url": "http://127.0.0.1:1/mcp", + "enabled": true, + })) + .expect("valid mcp server"); + snap.mcp_servers + .insert(ResourceEntry::new(MCP_SERVER_ID, mcp, 1)); + + let agent: aisix_core::A2aAgent = serde_json::from_value(serde_json::json!({ + "name": "census", + "url": "http://127.0.0.1:1/a2a", + "enabled": true, + })) + .expect("valid a2a agent"); + snap.a2a_agents + .insert(ResourceEntry::new("agent-census", agent, 1)); + + let guardrail: aisix_core::Guardrail = serde_json::from_value(serde_json::json!({ + "name": GUARDRAIL_ROW, + "enabled": true, + "kind": "custom", + "hook_point": "input", + "fail_open": false, + "script": BLOCK_EVERYTHING, + "timeout_ms": 5000, + })) + .expect("valid guardrail"); + snap.guardrails + .insert(ResourceEntry::new("g-census", guardrail, 1)); + + snap +} + +/// A hub with the two provider bridges the fixtures name. `/v1/embeddings` +/// resolves its bridge BEFORE the guardrail gate, so a bare hub would answer +/// 503 and the census would never reach the check it exists to make. +fn census_hub() -> Arc { + let hub = Arc::new(aisix_gateway::Hub::new()); + hub.register_specialized( + "openai", + Arc::new(aisix_provider_openai::OpenAiBridge::new()), + ); + hub.register_specialized( + "anthropic", + Arc::new(aisix_provider_anthropic::AnthropicBridge::new()), + ); + hub +} + +fn census_router() -> axum::Router { + let handle = SnapshotHandle::new(census_snapshot()); + let index = aisix_guardrails::LiveGuardrailIndex::new(handle.clone(), None); + let state = crate::ProxyState::new(handle, census_hub(), &cfg()) + .without_cache() + .with_guardrail_index(index); + crate::build_router(state) +} + +const MULTIPART_BOUNDARY: &str = "censusboundary"; + +/// A multipart body with the parts named, in order. Values are inline +/// bytes; nothing here needs a real file. +fn multipart(parts: &[(&str, &str)]) -> String { + let mut out = String::new(); + for (name, value) in parts { + out.push_str(&format!("--{MULTIPART_BOUNDARY}\r\n")); + if *name == "file" || *name == "image" { + out.push_str(&format!( + "Content-Disposition: form-data; name=\"{name}\"; filename=\"a.bin\"\r\n\ + Content-Type: application/octet-stream\r\n\r\n" + )); + } else { + out.push_str(&format!( + "Content-Disposition: form-data; name=\"{name}\"\r\n\r\n" + )); + } + out.push_str(value); + out.push_str("\r\n"); + } + out.push_str(&format!("--{MULTIPART_BOUNDARY}--\r\n")); + out +} + +/// One driveable request per `Posture::Enforced` surface. +/// +/// Bodies are deliberately CONTENTLESS wherever the wire shape allows it — +/// no prompt, empty `arguments`, empty message text. That is the shape every +/// bug in this class hid behind, so it is the shape the census drives. A +/// guardrail with a text-independent verdict must refuse them all. +fn fixture(surface: &str) -> Option> { + let json = |uri: &str, body: serde_json::Value| { + Request::builder() + .method("POST") + .uri(uri.to_string()) + .header("authorization", format!("Bearer {CALLER}")) + .header("content-type", "application/json") + .body(Body::from(body.to_string())) + .unwrap() + }; + let anthropic = |uri: &str, body: serde_json::Value| { + Request::builder() + .method("POST") + .uri(uri.to_string()) + .header("x-api-key", CALLER) + .header("anthropic-version", "2023-06-01") + .header("content-type", "application/json") + .body(Body::from(body.to_string())) + .unwrap() + }; + let form = |uri: &str, parts: &[(&str, &str)]| { + Request::builder() + .method("POST") + .uri(uri.to_string()) + .header("authorization", format!("Bearer {CALLER}")) + .header( + "content-type", + format!("multipart/form-data; boundary={MULTIPART_BOUNDARY}"), + ) + .body(Body::from(multipart(parts))) + .unwrap() + }; + let jsonrpc = |uri: &str, body: serde_json::Value| { + Request::builder() + .method("POST") + .uri(uri.to_string()) + .header("authorization", format!("Bearer {CALLER}")) + .header("host", "census.aisix.example.com") + .header("content-type", "application/json") + .header("accept", "application/json, text/event-stream") + .body(Body::from(body.to_string())) + .unwrap() + }; + + Some(match surface { + "/v1/chat/completions" => json( + surface, + serde_json::json!({ + "model": "census-openai", + "messages": [{ "role": "user", "content": "" }], + }), + ), + "/v1/completions" => json( + surface, + serde_json::json!({ "model": "census-openai", "prompt": "" }), + ), + "/v1/embeddings" => json( + surface, + serde_json::json!({ "model": "census-embedding", "input": "" }), + ), + "/v1/images/generations" => json( + surface, + serde_json::json!({ "model": "census-openai", "prompt": "" }), + ), + // No `prompt` part at all — the exact shape that used to skip the + // chain outright. + "/v1/images/edits" => form(surface, &[("model", "census-openai"), ("image", "x")]), + "/v1/messages" => anthropic( + surface, + serde_json::json!({ + "model": "census-anthropic", + "max_tokens": 16, + "messages": [{ "role": "user", "content": "" }], + }), + ), + "/v1/messages/count_tokens" => anthropic( + surface, + serde_json::json!({ + "model": "census-anthropic", + "messages": [{ "role": "user", "content": "" }], + }), + ), + "/v1/rerank" => json( + surface, + serde_json::json!({ "model": "census-openai", "query": "", "documents": [""] }), + ), + "/v1/responses" => json( + surface, + serde_json::json!({ "model": "census-openai", "input": "" }), + ), + // No `prompt` part — an ordinary transcription upload. + "/v1/audio/transcriptions" | "/v1/audio/translations" => { + form(surface, &[("model", "census-openai"), ("file", "RIFF")]) + } + "/v1/audio/speech" => json( + surface, + serde_json::json!({ "model": "census-openai", "input": "", "voice": "alloy" }), + ), + // The only fixture carrying text: `/v1/videos` rejects an empty + // `prompt` at schema validation, so it has no contentless shape. + "/v1/videos" => json( + surface, + serde_json::json!({ "model": "census-openai", "prompt": "a cat" }), + ), + // `"arguments": {}` — the reported MCP bypass. + "/mcp" | "/mcp/" => jsonrpc( + surface, + serde_json::json!({ + "jsonrpc": "2.0", + "id": 1, + "method": "tools/call", + "params": { "name": "census__tool", "arguments": {} }, + }), + ), + "/mcp/:server" => jsonrpc( + "/mcp/census", + serde_json::json!({ + "jsonrpc": "2.0", + "id": 1, + "method": "tools/call", + "params": { "name": "tool", "arguments": {} }, + }), + ), + // `tasks/get` carries no message at all. + "/a2a/:agent" => jsonrpc( + "/a2a/census", + serde_json::json!({ + "jsonrpc": "2.0", + "id": 1, + "method": "tasks/get", + "params": { "id": "task-1" }, + }), + ), + _ => return None, + }) +} + +/// Every envelope on the proxy — OpenAI 422, the Anthropic error shape, the +/// JSON-RPC error object, and the `/mcp` `isError` tool result — renders its +/// refusal through `error::guardrail_block_message`, which names the firing +/// row. So one substring recognises a genuine guardrail refusal on all of +/// them, and cannot be satisfied by an unrelated 4xx (a missing field, an +/// unknown model, a dead upstream) — which is the failure mode a +/// status-code-only assertion would have. +fn refused_by_guardrail(body: &str) -> bool { + body.contains(&format!("guardrail '{GUARDRAIL_ROW}'")) +} + +#[tokio::test] +async fn enforced_surfaces_refuse_a_blocking_guardrail() { + let mut missing_fixture = Vec::new(); + let mut not_refused = Vec::new(); + let router = census_router(); + + for (surface, posture) in POSTURE { + if !matches!(posture, Posture::Enforced) { + continue; + } + let Some(request) = fixture(surface) else { + missing_fixture.push(*surface); + continue; + }; + let response = router + .clone() + .oneshot(request) + .await + .expect("router must answer"); + let status = response.status(); + let bytes = axum::body::to_bytes(response.into_body(), 1 << 20) + .await + .expect("body must read"); + let body = String::from_utf8_lossy(&bytes).into_owned(); + if !refused_by_guardrail(&body) { + not_refused.push(format!("{surface} -> {status}: {body}")); + } + } + + assert!( + missing_fixture.is_empty(), + "these surfaces are declared Posture::Enforced but have no fixture in `fixture()`, so \ + nothing actually checks them: {missing_fixture:?}", + ); + assert!( + not_refused.is_empty(), + "a guardrail that blocks unconditionally did NOT refuse these surfaces — the input chain \ + either did not run or did not decide:\n{}", + not_refused.join("\n"), + ); +} + +/// Drive every enforced surface with `guardrails` holding `row` — or +/// nothing at all when `row` is `None` — and return the surfaces that +/// answered with a guardrail refusal. +async fn surfaces_refused_with(row: Option) -> Vec<&'static str> { + let snap = census_snapshot(); + snap.guardrails.remove("g-census"); + if let Some(row) = row { + let guardrail: aisix_core::Guardrail = + serde_json::from_value(row).expect("valid guardrail"); + snap.guardrails + .insert(ResourceEntry::new("g-census", guardrail, 2)); + } + let handle = SnapshotHandle::new(snap); + let index = aisix_guardrails::LiveGuardrailIndex::new(handle.clone(), None); + let state = crate::ProxyState::new(handle, census_hub(), &cfg()) + .without_cache() + .with_guardrail_index(index); + let router = crate::build_router(state); + + let mut refused = Vec::new(); + for (surface, posture) in POSTURE { + if !matches!(posture, Posture::Enforced) { + continue; + } + let Some(request) = fixture(surface) else { + continue; + }; + let response = router + .clone() + .oneshot(request) + .await + .expect("router must answer"); + let bytes = axum::body::to_bytes(response.into_body(), 1 << 20) + .await + .expect("body must read"); + if refused_by_guardrail(&String::from_utf8_lossy(&bytes)) { + refused.push(*surface); + } + } + refused +} + +/// The other half of the rule, and the reason the fix is not simply +/// "always block when there is no text": a guardrail whose verdict comes +/// FROM the text has matched nothing, and a textless request is clean to +/// it. Whether the chain runs is the call site's business; what it decides +/// is the guardrail's. +#[tokio::test] +async fn a_text_matching_guardrail_leaves_textless_requests_alone() { + let refused = surfaces_refused_with(Some(serde_json::json!({ + "name": GUARDRAIL_ROW, + "enabled": true, + "kind": "keyword", + "hook_point": "input", + "patterns": [{ "kind": "literal", "value": "census-forbidden-token" }], + }))) + .await; + assert!( + refused.is_empty(), + "a keyword rule that matched nothing refused these surfaces — removing the \ + empty-text short-circuits must not turn 'no text' into a blanket block: {refused:?}", + ); +} + +/// The census is only meaningful if the fixtures would otherwise succeed +/// past the guardrail gate. With no guardrail configured, none of them may +/// answer with a guardrail refusal — otherwise the test above could be +/// passing on some unrelated error text. +#[tokio::test] +async fn fixtures_do_not_self_refuse_without_a_guardrail() { + let snap = census_snapshot(); + snap.guardrails.remove("g-census"); + let handle = SnapshotHandle::new(snap); + let state = crate::ProxyState::new(handle, census_hub(), &cfg()).without_cache(); + let router = crate::build_router(state); + + for (surface, posture) in POSTURE { + if !matches!(posture, Posture::Enforced) { + continue; + } + let Some(request) = fixture(surface) else { + continue; + }; + let response = router + .clone() + .oneshot(request) + .await + .expect("router must answer"); + let bytes = axum::body::to_bytes(response.into_body(), 1 << 20) + .await + .expect("body must read"); + let body = String::from_utf8_lossy(&bytes).into_owned(); + assert!( + !refused_by_guardrail(&body), + "{surface} reported a guardrail refusal with no guardrail configured: {body}", + ); + } +} diff --git a/crates/aisix-proxy/src/images_edits.rs b/crates/aisix-proxy/src/images_edits.rs index 88ae59f9..190ad133 100644 --- a/crates/aisix-proxy/src/images_edits.rs +++ b/crates/aisix-proxy/src/images_edits.rs @@ -334,7 +334,14 @@ async fn dispatch( .filter(|s| !s.is_empty()) .map(|s| ChatMessage::user(s.to_string())) .collect(); - if !prompt_messages.is_empty() { + // The chain runs whether or not a `prompt` part was supplied. + // Gating on "we found text" made the check a text matcher's + // privilege: a guardrail that decides about the CALL — a policy + // script, an unconditional block scoped to this model — never + // fired on the ordinary shape of this endpoint (an edit with no `prompt` part), + // so an operator's rule silently allowed exactly the requests + // that carry nothing to match. + { let chat = aisix_gateway::ChatFormat::new(&model_name, prompt_messages); let (verdict, hits) = aisix_guardrails::Guardrail::check_input_observed(&resolved_chain, &chat).await; diff --git a/crates/aisix-proxy/src/lib.rs b/crates/aisix-proxy/src/lib.rs index d5771354..112c5e32 100644 --- a/crates/aisix-proxy/src/lib.rs +++ b/crates/aisix-proxy/src/lib.rs @@ -42,6 +42,8 @@ mod embeddings; mod ensemble; mod error; mod error_translate; +#[cfg(test)] +mod guardrail_coverage; mod guardrail_embedder; mod guardrail_stream; pub mod health; diff --git a/crates/aisix-proxy/src/mcp.rs b/crates/aisix-proxy/src/mcp.rs index 9b7754a8..a13df5fc 100644 --- a/crates/aisix-proxy/src/mcp.rs +++ b/crates/aisix-proxy/src/mcp.rs @@ -408,10 +408,11 @@ async fn dispatch( .unwrap_or_default(); let chat = aisix_gateway::ChatFormat::new("", vec![aisix_gateway::ChatMessage::user(args_text)]); - // Segment-moderating members (semantic, Bedrock ANONYMIZE) are - // consulted through the segment pass below instead — the same - // check/moderate split every LLM family uses, so a member is - // never consulted (or billed) twice per hook. + // Segment-moderating members (custom scripts, Bedrock ANONYMIZE, + // Presidio, Lakera, Aliyun AI) are consulted through the segment + // pass below instead — the same check/moderate split every LLM + // family uses, so a member is never consulted (or billed) twice + // per hook. let (verdict, hits) = aisix_guardrails::Guardrail::check_input_non_segment_observed(chain, &chat).await; monitor_hits.extend(hits); @@ -527,8 +528,8 @@ async fn dispatch( _ => bytes, }; // Async segment-moderation pass over the same `params.arguments` - // string leaves the sync write-back covers (#1363): semantic rows — - // and any other segment-moderating member — mask through here. The + // string leaves the sync write-back covers (#1363): every + // segment-moderating member decides — and masks — through here. The // pass reuses the byte-splice walker, so the scan slots and the // write-back slots are the same set by construction (no fourth text // shape; the aisix#1027 scan/rewrite divergence is not widened). @@ -801,7 +802,7 @@ async fn moderate_tool_arguments( .await } -/// Run the chain's segment-moderating members (semantic rows, Bedrock +/// Run the chain's segment-moderating members (custom scripts, Bedrock /// ANONYMIZE) over the string leaves `pred` selects (#1363): collect /// the decoded slots with the same byte-splice walker the write-back /// uses, moderate them in ONE chain pass, and splice the positionally @@ -829,9 +830,10 @@ async fn moderate_selected_segments( unavailable: Some(crate::error::TAG_UNSCANNABLE_BODY.to_owned()), }; } - if texts.is_empty() { - return SegmentPassOutcome::Keep; - } + // No early return on an empty collect walk — see `redact::moderate_body`. + // A `tools/call` with `"arguments": {}` has no string leaves, and + // returning `Keep` here meant a guardrail scoped to the MCP server was + // never consulted: the tool executed under an unconditional block rule. let mut outcome = if input { aisix_guardrails::Guardrail::moderate_input_segments(chain, &texts).await } else { @@ -1051,7 +1053,7 @@ async fn apply_output_guardrails( // (`tool_result_path`; `name`/`uri` stay untouched — they address a // resource; see the scan-loop comment). Two write-back channels // compose: the sync per-field redactors (kind=pii mask rules), then - // the async segment pass (semantic rows, Bedrock ANONYMIZE) over + // the async segment pass (custom scripts, Bedrock ANONYMIZE) over // whatever the sync pass produced. let mut counts = crate::redact::RedactionCounts::new(); let mut current: Option> = None; diff --git a/crates/aisix-proxy/src/messages.rs b/crates/aisix-proxy/src/messages.rs index 6f9608ae..d32600bb 100644 --- a/crates/aisix-proxy/src/messages.rs +++ b/crates/aisix-proxy/src/messages.rs @@ -602,50 +602,72 @@ async fn dispatch( *applied_out = resolved_chain.applied().to_vec(); *audit_out = resolved_chain.audit_log(); if !resolved_chain.is_empty() { - if let Ok(chat) = aisix_provider_anthropic::parse_inbound_request(body) { - let (verdict, hits) = aisix_guardrails::Guardrail::check_input_non_segment_observed( - resolved_chain.as_ref(), - &chat, - ) - .await; - monitor_hits_out.extend(hits); - // Segment pass: one Bedrock call over the body's text slots; - // an ANONYMIZE disposition writes the masked text back into - // the Anthropic-native body (#932 bedrock follow-up). - let verdict = crate::redact::moderate_body( - resolved_chain.as_ref(), - crate::redact::Direction::Input, - verdict, - redactions_out, - monitor_hits_out, - |g| crate::redact::redact_anthropic_request(g, body), - ) - .await; - if let aisix_guardrails::GuardrailVerdict::Block { - reason, - guardrail_name, - unavailable, - } = verdict - { - // AISIX-Cloud#1013: mask before returning so the failure - // content capture exports post-mask text (see chat.rs). - crate::redact::merge_counts( - redactions_out, - crate::redact::redact_anthropic_request(resolved_chain.as_ref(), body), - ); + // Fail CLOSED when the body cannot be parsed into something + // scannable. This used to be `if let Ok(chat) = ...`, so a shape + // the gateway's Anthropic parser rejects skipped the guardrail + // and was forwarded upstream anyway — the check was only as + // complete as the parser, and the parser lags the provider by + // construction. `/mcp` already takes this arm on an unscannable + // body; this is the same rule on the LLM side. + let chat = match aisix_provider_anthropic::parse_inbound_request(body) { + Ok(chat) => chat, + Err(err) => { tracing::warn!( guardrail_hook = "input", model = %model_name, - reason = %reason, - "guardrail blocked /v1/messages request", + error = %err, + "cannot scan /v1/messages body for guardrails; blocking", ); return Err(crate::error::guardrail_block_error( "request", - guardrail_name.as_deref(), - unavailable.as_deref(), + None, + Some(crate::error::TAG_UNSCANNABLE_BODY), ) .into()); } + }; + let (verdict, hits) = aisix_guardrails::Guardrail::check_input_non_segment_observed( + resolved_chain.as_ref(), + &chat, + ) + .await; + monitor_hits_out.extend(hits); + // Segment pass: one Bedrock call over the body's text slots; + // an ANONYMIZE disposition writes the masked text back into + // the Anthropic-native body (#932 bedrock follow-up). + let verdict = crate::redact::moderate_body( + resolved_chain.as_ref(), + crate::redact::Direction::Input, + verdict, + redactions_out, + monitor_hits_out, + |g| crate::redact::redact_anthropic_request(g, body), + ) + .await; + if let aisix_guardrails::GuardrailVerdict::Block { + reason, + guardrail_name, + unavailable, + } = verdict + { + // AISIX-Cloud#1013: mask before returning so the failure + // content capture exports post-mask text (see chat.rs). + crate::redact::merge_counts( + redactions_out, + crate::redact::redact_anthropic_request(resolved_chain.as_ref(), body), + ); + tracing::warn!( + guardrail_hook = "input", + model = %model_name, + reason = %reason, + "guardrail blocked /v1/messages request", + ); + return Err(crate::error::guardrail_block_error( + "request", + guardrail_name.as_deref(), + unavailable.as_deref(), + ) + .into()); } // #932: mask-action PII rules rewrite the Anthropic-native body in // place AFTER the block check passes — both the passthrough and the diff --git a/crates/aisix-proxy/src/redact.rs b/crates/aisix-proxy/src/redact.rs index b0867760..4a49e51f 100644 --- a/crates/aisix-proxy/src/redact.rs +++ b/crates/aisix-proxy/src/redact.rs @@ -212,9 +212,14 @@ pub async fn moderate_body( let collector = SegmentCollector::default(); walk(&collector); let texts = collector.take(); - if texts.is_empty() { - return non_segment_verdict; - } + // The pass runs even with zero collected slots. "Nothing to scan" is + // not "nothing to decide": a segment-moderating member may hold a + // verdict that does not depend on the text (a `kind: custom` policy + // script), and skipping the pass turned an operator's block rule into + // a silent allow on any request whose scannable slots were all empty. + // Every member that needs a remote call short-circuits empty input + // itself (bedrock/lakera/presidio/aliyun all refuse empty content), so + // consulting the chain here costs no provider round-trip. let mut outcome = match dir { Direction::Input => chain.moderate_input_segments(&texts).await, Direction::Output => chain.moderate_output_segments(&texts).await, diff --git a/tests/e2e/src/cases/guardrail-textless-request-e2e.test.ts b/tests/e2e/src/cases/guardrail-textless-request-e2e.test.ts new file mode 100644 index 00000000..9fe8baf6 --- /dev/null +++ b/tests/e2e/src/cases/guardrail-textless-request-e2e.test.ts @@ -0,0 +1,320 @@ +import { createHash, randomUUID } from "node:crypto"; +import { afterAll, beforeAll, describe, expect, test } from "vitest"; +import { + EtcdClient, + ProxyClient, + SeedClient, + spawnApp, + startMcpUpstream, + startOpenAiUpstream, + waitConfigPropagation, + type McpUpstream, + type OpenAiUpstream, + type SpawnedApp, +} from "../harness/index.js"; + +// E2E: a request that carries no scannable text still gets a guardrail +// VERDICT. Found by release QA on v0.11.0-rc.5 and reproducing back to +// rc.4 — pre-existing, not a regression. +// +// The gateway used to treat "nothing to scan" as "nothing to decide", in +// four separate places. An operator who attached an unconditional `block` +// policy watched these succeed: +// +// - MCP `tools/call` with `"arguments": {}` — the tool EXECUTED (the +// reported bug). The segment pass collected zero string leaves under +// `params.arguments` and returned early without consulting the chain. +// - `/v1/messages/count_tokens` — ran no chain at all, while shipping +// the caller's whole `system` + `messages` + `tools` payload to the +// provider. +// - `/v1/audio/transcriptions` with no `prompt` form field — i.e. the +// ordinary shape of that endpoint. +// - `/v1/images/edits` with no `prompt` part. +// +// A guardrail whose verdict depends on the text (keyword, pii) legitimately +// allows an empty request — it matched nothing. A guardrail that decides +// about the CALL does not, and that difference belongs to the guardrail, +// never to the call site. Both halves are pinned here: the same textless +// requests are driven against an unconditional `kind: custom` policy (must +// refuse) and against a `kind: keyword` rule (must pass), so a fix that +// simply started blocking every empty request would fail this file too. +// +// The upstream request recorder is the load-bearing assertion: a refusal +// that still contacted the provider would have leaked exactly the payload +// the guardrail exists to keep in. + +const BLOCKING_KEY = "sk-textless-blocking"; +const KEYWORD_KEY = "sk-textless-keyword"; +const sha256 = (s: string) => createHash("sha256").update(s).digest("hex"); + +/** No `ctx` read at all: the verdict cannot come from the text. */ +const BLOCK_EVERYTHING = ` +export function checkInput() { + return { action: "block", reason_code: "textless-e2e" }; +} +`; + +const KEYWORD_PATTERN = "textless-e2e-forbidden"; + +interface RpcReply { + status: number; + json?: { + result?: { + content?: Array<{ type: string; text?: string }>; + isError?: boolean; + }; + error?: { code: number; message: string }; + }; +} + +/** One gateway plus its upstreams, wired with a single guardrail row. */ +interface Env { + app: SpawnedApp; + llm: OpenAiUpstream; + mcp: McpUpstream; + key: string; +} + +const startEnv = async ( + etcd: EtcdClient, + key: string, + guardrail: Record, +): Promise => { + const llm = await startOpenAiUpstream({ nonStreamBody: { input_tokens: 7 } }); + const mcp = await startMcpUpstream("textless"); + const app = await spawnApp(); + const seed = new SeedClient(etcd, app.etcdPrefix); + + const anthropicPk = await seed.createProviderKey({ + display_name: "textless-anthropic", + secret: "sk-ant-mock", + // The Anthropic bridge appends to the BARE host — no /v1 suffix here. + api_base: llm.baseUrl, + provider: "anthropic", + adapter: "anthropic", + }); + const openaiPk = await seed.createProviderKey({ + display_name: "textless-openai", + secret: "sk-mock", + api_base: `${llm.baseUrl}/v1`, + }); + await seed.createModel({ + display_name: "textless-anthropic", + provider: "anthropic", + model_name: "claude-haiku-4-5-20251001", + provider_key_id: anthropicPk.id, + }); + await seed.createModel({ + display_name: "textless-audio", + provider: "openai", + model_name: "gpt-4o-transcribe", + provider_key_id: openaiPk.id, + }); + await seed.update("mcp_servers", randomUUID(), { + display_name: "textless", + url: mcp.url, + enabled: true, + }); + // No attachment row: a guardrail with none falls back to implicit + // env scope, which is what an operator gets by default. + await seed.createGuardrail(guardrail); + + // Written LAST, so the key authenticating implies every row above it + // landed (etcd applies in revision order). The gate touches neither a + // guarded endpoint nor a guardrail, so a broken assertion below fails as + // an assertion rather than as a gate timeout. + await seed.createApiKey({ + key_hash: sha256(key), + allowed_models: ["*"], + mcp_access: { allow: ["*"] }, + }); + const proxy = new ProxyClient(app.proxyUrl, key); + await waitConfigPropagation(async () => (await proxy.listModels()).status === 200); + + return { app, llm, mcp, key }; +}; + +/** `tools/call` with NO arguments at all — the reported shape. */ +const callToolWithoutArguments = async (env: Env): Promise => { + const post = async (body: unknown): Promise => { + const res = await fetch(`${env.app.proxyUrl}/mcp`, { + method: "POST", + headers: { + authorization: `Bearer ${env.key}`, + "content-type": "application/json", + accept: "application/json, text/event-stream", + }, + body: JSON.stringify(body), + }); + const text = await res.text(); + let json: RpcReply["json"]; + try { + json = text ? (JSON.parse(text) as RpcReply["json"]) : undefined; + } catch { + json = undefined; + } + return { status: res.status, json }; + }; + // The endpoint is stateless: every operation re-handshakes. + await post({ + jsonrpc: "2.0", + id: 1, + method: "initialize", + params: { + protocolVersion: "2025-11-25", + capabilities: {}, + clientInfo: { name: "textless-e2e", version: "0.1" }, + }, + }); + return post({ + jsonrpc: "2.0", + id: 3, + method: "tools/call", + params: { name: "textless__echo", arguments: {} }, + }); +}; + +const countTokens = (env: Env) => + fetch(`${env.app.proxyUrl}/v1/messages/count_tokens`, { + method: "POST", + headers: { + "content-type": "application/json", + "x-api-key": env.key, + "anthropic-version": "2023-06-01", + }, + body: JSON.stringify({ + model: "textless-anthropic", + messages: [{ role: "user", content: "my SSN is 123-45-6789" }], + }), + }); + +/** A transcription upload with no `prompt` part — the ordinary shape. */ +const transcribe = (env: Env) => { + const form = new FormData(); + form.set("model", "textless-audio"); + form.set( + "file", + new Blob([new Uint8Array([0x49, 0x44, 0x33])], { type: "audio/mpeg" }), + "a.mp3", + ); + return fetch(`${env.app.proxyUrl}/v1/audio/transcriptions`, { + method: "POST", + headers: { authorization: `Bearer ${env.key}` }, + body: form, + }); +}; + +describe("textless requests still get a guardrail verdict", () => { + let etcdReachable = false; + let blocking: Env | undefined; + let keyword: Env | undefined; + + beforeAll(async () => { + const etcd = new EtcdClient(); + etcdReachable = await etcd.ping(); + if (!etcdReachable) return; + + blocking = await startEnv(etcd, BLOCKING_KEY, { + name: "textless-block-all", + enabled: true, + kind: "custom", + hook_point: "input", + fail_open: false, + script: BLOCK_EVERYTHING, + timeout_ms: 5000, + }); + keyword = await startEnv(etcd, KEYWORD_KEY, { + name: "textless-keyword", + enabled: true, + kind: "keyword", + hook_point: "input", + patterns: [{ kind: "literal", value: KEYWORD_PATTERN }], + }); + }, 90_000); + + afterAll(async () => { + await blocking?.app.exit(); + await blocking?.llm.close(); + await blocking?.mcp.close(); + await keyword?.app.exit(); + await keyword?.llm.close(); + await keyword?.mcp.close(); + }); + + test("MCP tools/call with empty arguments is refused, and the tool never runs", async (ctx) => { + if (!etcdReachable || !blocking) return ctx.skip(); + + const before = blocking.mcp.received.length; + const reply = await callToolWithoutArguments(blocking); + + // The MCP block contract: HTTP 200 + a TOOL-execution error, so the + // calling agent reads it as tool output rather than a protocol fault. + expect(reply.status).toBe(200); + expect(reply.json?.error).toBeUndefined(); + expect(reply.json?.result?.isError).toBe(true); + expect(reply.json?.result?.content?.[0]?.text).toContain("content policy"); + expect(reply.json?.result?.content?.[0]?.text).toContain("textless-block-all"); + + // The whole point: the tool did not execute. Pre-fix it did. + const relayed = blocking.mcp.received + .slice(before) + .filter((body) => body.includes("tools/call")); + expect(relayed).toEqual([]); + }, 30_000); + + test("count_tokens is refused, and the payload never reaches the provider", async (ctx) => { + if (!etcdReachable || !blocking) return ctx.skip(); + + const before = blocking.llm.receivedRequests.length; + const res = await countTokens(blocking); + + // Anthropic-shaped envelope, like every other error on this route. + expect(res.status).toBe(422); + const body = (await res.json()) as { + type?: string; + error?: { type?: string; message?: string }; + }; + expect(body.type).toBe("error"); + expect(body.error?.message ?? "").toContain("content policy"); + expect(body.error?.message ?? "").toContain("textless-block-all"); + + const forwarded = blocking.llm.receivedRequests + .slice(before) + .filter((r) => r.path.includes("count_tokens")); + expect(forwarded).toEqual([]); + }, 30_000); + + test("a transcription with no prompt field is refused", async (ctx) => { + if (!etcdReachable || !blocking) return ctx.skip(); + + const before = blocking.llm.receivedRequests.length; + const res = await transcribe(blocking); + + expect(res.status).toBe(422); + const body = (await res.json()) as { error?: { type?: string; message?: string } }; + expect(body.error?.type).toBe("content_filter"); + expect(body.error?.message ?? "").toContain("textless-block-all"); + + const forwarded = blocking.llm.receivedRequests + .slice(before) + .filter((r) => r.path.includes("transcriptions")); + expect(forwarded).toEqual([]); + }, 30_000); + + // The negative control. Removing the short-circuits must not turn "no + // text" into a blanket refusal: a rule that decides by matching text has + // matched nothing, and an empty request is clean to it. + test("a text-matching guardrail lets the same textless requests through", async (ctx) => { + if (!etcdReachable || !keyword) return ctx.skip(); + + const reply = await callToolWithoutArguments(keyword); + expect(reply.status).toBe(200); + expect(reply.json?.result?.isError).toBeFalsy(); + + const counted = await countTokens(keyword); + expect(counted.status).toBe(200); + + const transcribed = await transcribe(keyword); + expect(transcribed.status).toBe(200); + }, 30_000); +}); From c875d02e6d62f5e92ef7bdab0b2d13c87302a411 Mon Sep 17 00:00:00 2001 From: Jarvis Date: Fri, 28 Aug 2026 04:39:33 +0000 Subject: [PATCH 2/2] docs(agents): record the guardrail-chain call-site rule and its census --- CLAUDE.md | 1 + 1 file changed, 1 insertion(+) diff --git a/CLAUDE.md b/CLAUDE.md index a396c47b..933bdc3e 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -100,6 +100,7 @@ This repo is developed end-to-end by agents — no human reviewer needs small re - "Documented follow-up" without an issue is how gaps rot: it lives in one PR description and no one ever comes back. - **An emit function on `Metrics` with no caller is invisible to every check we run.** Its methods are `pub`, so dead-code analysis never fires; unit tests call it directly and pass; the only symptom is a series that never appears in a scrape, which is indistinguishable from "no traffic yet". A metric family is shipped when an **e2e asserts it in `GET /metrics`** after driving real traffic — not when `Metrics` can emit it. (Twice now: `record_proxy_request` until #888, then `record_deployment_request` + `record_routing_fallback` until #972.) - Test coverage must include each wired endpoint, not just chat: an e2e that only drives `/v1/chat/completions` will stay green forever while Anthropic-SDK (`/v1/messages`) and Codex (`/v1/responses`) traffic silently misbehaves. +- **Never gate the guardrail chain on having found text to scan.** "Nothing to scan" is not "nothing to decide": a `kind: custom` row's verdict can be independent of the text, so a call site that skips the chain when its collect walk came back empty converts an operator's block rule into a silent allow. The call site always consults the chain; only a guardrail kind may decide it needs text, and every remote kind already self-guards, so consulting costs no provider round-trip. `crates/aisix-proxy/src/guardrail_coverage.rs` enforces this: it parses the routing table out of `build_router`'s own source, so a new route fails the census until it declares a posture, and every surface declared `Enforced` is driven against an unconditional-block guardrail. - Prefer hoisting the shared logic into one chokepoint (e.g. `resolve_attempt_models`) so the family can't drift again. (Two recurrences of the same lesson: #471 — a Model-Group dispatch fix landed only on `/v1/messages` while `/v1/responses` and `count_tokens` had the identical gap; then #715 — `least_busy`'s in-flight counter shipped fed by chat.rs only (#684 left messages/responses as an un-filed "follow-up"), so the strategy silently degraded to declaration order for Claude Code / Codex traffic until #716. The EWMA for `least_latency` (#682) wired all three endpoints at once and never had this problem — that's the standard.)