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.) 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); +});