Repository navigation
fix(guardrails): refuse an unscannable /v1/files upload only under a text purpose - #1114
Conversation
…text purpose #1113 refuses a `POST /v1/files` upload whose bytes are not valid UTF-8 whenever an input guardrail chain resolves, and that refusal is purpose-blind: it reads only the `file` part. A deployment running any input guardrail therefore started getting 422 for a PDF, an image, or any other binary uploaded under `purpose=assistants` / `vision` / `user_data`, which is well beyond the malformed-JSONL case the refusal was built for. The refusal is now keyed on the declared multipart `purpose`. An undecodable blob is refused only under a purpose whose payload is contractually UTF-8 text — `batch` and `fine-tune` take JSONL, and an eval data set referenced by file id is a JSONL source too. Every other purpose, and an upload declaring no purpose at all, is not refused: a payload we cannot classify is not one we can claim should have been text. The scan itself is unchanged. A non-text-purpose upload keeps the best-effort lossy scan it has always had, so a term that survives the lossy decode still blocks — narrowing the refusal must not turn into skipping the chain. The residual asymmetry (a binary upload is scanned on a lossy decode and forwarded verbatim) is the deliberate posture of this surface and stays; `scan_output_blob`'s note on the download side is updated to say which signal the input side actually keys on. This route never validates `purpose` — it forwards whatever the caller declared — so the classification is an exact match against the text set and nothing else. A case variant, a provider-specific extension, or a server-minted value such as `batch_output` is simply unrecognised and is not refused. BEHAVIOUR CHANGE: this narrows the 422 introduced by #1113. A `POST /v1/files` upload whose `file` part is not valid UTF-8 is now refused only when the declared `purpose` is `batch`, `fine-tune` or `evals`, and only when an input guardrail chain resolves for the request. Under `assistants`, `vision`, `user_data`, any unrecognised purpose, or no purpose at all, such an upload is scanned on a lossy decode and forwarded to the provider exactly as it was before #1113. Uploads under a text purpose keep #1113's envelope unchanged: 422 with `error.type: content_filter`, `error.code: guardrail_unavailable`, and `unscannable_body` in the message. Deployments with no guardrail attached to the upload remain unaffected on every purpose. Ref #1022, #1113
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Essentials Run ID: 📒 Files selected for processing (1)
Included review availability: Your plan provides up to 5 included reviews per hour; 0 remain after this review. 📝 WalkthroughWalkthroughFile uploads now classify ChangesPurpose-aware upload scanning
Estimated code review effort: 4 (Complex) | ~45 minutes Merge Risk: ⚪ Minimal · up to This change limits unscannable-upload rejection to declared text purposes while preserving binary uploads and lossy policy scanning elsewhere; no actionable merge-blocking risk remains beyond normal checks and review. Sequence Diagram(s)sequenceDiagram
participant Client
participant AisixProxy
participant GuardrailScanner
participant Upstream
Client->>AisixProxy: Send multipart upload with optional purpose
AisixProxy->>AisixProxy: Accumulate purpose classification
AisixProxy->>GuardrailScanner: Scan strict or lossy content
GuardrailScanner-->>AisixProxy: Allow, policy block, or unscannable result
AisixProxy->>Upstream: Forward original multipart bytes when allowed
Suggested reviewers: 🚥 Pre-merge checks | ✅ 5 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (5 passed)
Full details: E2e Test Quality ReviewExplanation The E2E harness is relevant and uses a spawned gateway, real etcd configuration, and a real HTTP upstream recorder. The main batch, fine-tune, binary-purpose, no-purpose, and policy-hit flows are readable and assert provider contact. However, the PR changes duplicate-purpose handling and explicitly requires that any text purpose refuses the upload regardless of field order, but only a unit test covers this. The E2E case has no duplicate-purpose request. It also does not exercise Resolution Add E2E requests through the existing spawned-app/etcd/upstream-recorder flow for Full details: Security CheckExplanation No explicit security-check failure was introduced by the assessed patch. The patch changes only
✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
There was a problem hiding this comment.
🟡 Changes recommended
The new /v1/files purpose capture overwrites on repeated purpose fields, which can allow bypassing the intended non‑UTF‑8 refusal by sending multiple purpose parts in a crafted order.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR narrows the guardrail “unscannable upload” refusal on POST /v1/files so that invalid UTF-8 uploads are only rejected when the multipart purpose indicates a contractually UTF-8 text payload (e.g., JSONL for batch / fine-tune / evals), while binary-allowed purposes continue to be scanned lossily and forwarded verbatim.
Changes:
- Add
purpose-aware classification (Undecodable) to decide whether non‑UTF‑8 uploads are refused or scanned lossily in the/v1/filesguardrail path. - Preserve fail-closed refusal behavior for text-contract purposes while allowing binary purposes (and unknown/absent
purpose) to forward. - Extend unit + e2e coverage to lock in the new
purposematrix and ensure non-text uploads are still scanned (not skipped).
File summaries
| File | Description |
|---|---|
crates/aisix-proxy/src/jobs.rs |
Introduces purpose-based undecodable posture and threads it into scan_input_blob + /v1/files multipart handling; adds unit tests for the posture table and forwarding/refusal behavior. |
tests/e2e/src/cases/files-unscannable-upload-e2e.test.ts |
Expands e2e coverage to assert forwarding for binary purposes / missing purpose, refusal for fine-tune, and continued lossy scanning under binary purposes. |
Review details
- Files reviewed: 2/2 changed files
- Comments generated: 1
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
A multipart body may carry the `purpose` field more than once. Taking the last one lets a caller append a binary purpose after a text one and pick the looser reading for us, while the provider picks whichever part it picks — the two sides would then disagree about what the payload is. Such a body is malformed either way, so the classification now folds: ANY declared text purpose refuses an undecodable blob, whatever order the parts arrive in. Bodies declaring a single `purpose`, which is every well-formed one, are unaffected.
`binary_purpose_upload_is_still_scanned_lossily` and its e2e counterpart put the matchable term BEFORE the invalid bytes, so replacing the lossy decode with a scan of the valid UTF-8 prefix alone left both legs green — the "a binary-purpose upload is still scanned" claim was not actually pinned. That is the shape a real evasion takes: a few bad bytes up front and the payload behind them. Both fixtures now put the term after the invalid bytes, which makes the prefix-only decode fail them. Also corrects two comments that stated more than the code does: no caller sets `FwdSpec::body` today, so the `scan_input_blob` call in `forward_simple` is unreachable rather than reached with re-serialised JSON; and the `evals` justification now cites what actually establishes it (every eval run data source accepting a file id declares that file a JSONL source, with no binary variant).
…ds that side and fails closed (#1115) ## Problem Two things were wrong with the `unscannable_body` refusals the gateway raises on the chain's behalf. Both come from the same gate: `!chain.is_empty()`. `GuardrailIndex::resolve` matches attachments on **scope** alone — env / model / mcp_server / api_key / team — and never filters by `hook_point`. A resolved chain is therefore non-empty whenever any attachment is in scope, including one attached on the **output** hook only. Each guardrail then no-ops on the hook it is not configured for, which makes `!chain.is_empty()` a correct gate for *running* the checks and a wrong one for *refusing*. So **a guardrail configured for the output side alone caused a request-side refusal of a payload it would never have inspected.** The same gate also ignored `fail_open`. The refusal reports itself as `guardrail_unavailable`, and this project's standing rule is that a guardrail which cannot evaluate follows its configured failure policy — fail-closed by default, `fail_open: true` honoured when the operator sets it, with no class of cause carved out. **A row explicitly set to fail open still refused the request**, contradicting what the label promises. ## The gate now > A body the scanner cannot read is refused only if the resolved chain contains at least one guardrail that **both** reads that side of the exchange **and** is fail-closed on it. Both halves must hold on the *same* member. The failure policy is per hook, mirroring how the kinds already read it: the row's `fail_open` on the input hook, each remote kind's `<Kind>Config::output_fail_open` on the output hook. `keyword` and `pii` never call out and so have no `output_fail_open`; their row-level `fail_open` governs both of their hooks, which is the only policy an operator of a local-only deployment can express. Both halves on the same member is not pedantry: a chain of [output-only fail-closed, input-only fail-open] reads the request *and* contains a fail-closed row, yet nothing in it justifies refusing a request. Folding the two predicates independently gets that case wrong, which is why `refuses_unevaluable_input` / `_output` is one predicate rather than two you could `&&`. Only a deployment that set `hook_point` or `fail_open` explicitly is affected — they default to `both` and `false`. ## Behaviour change `POST /v1/files` forwards an invalid-UTF-8 upload under a text purpose when no attachment in scope both reads the request and fails closed — exactly as it does for a deployment running no guardrails at all. A mixed chain folds to the strictest: one fail-closed request-side row restores the refusal. Nothing else about the refusal moves: same text-purpose set (`batch`, `fine-tune`, `evals`), same 422, same `content_filter` / `guardrail_unavailable` / `unscannable_body` envelope, same handling of binary purposes, missing purposes and unconfigured deployments. This narrows the refusal introduced in #1113 and narrowed to the text purposes in #1114; all three land in the same release range and are meant to read together. ## Upgrade risk `fail_open` was documented as a no-op for `keyword` and `pii`, so existing rows may carry a `fail_open: true` that was set when it did nothing — copied from a template, left in a `resources.yaml`, or written by a form that renders the field for every kind. **Those rows now take effect, so a deployment that upgrades only the data plane stops refusing bodies it refused before, with no operator action and no signal.** Operators running `keyword` or `pii` rows should audit them for an unintended `fail_open: true` before upgrading. The DP rustdoc and the generated `schemas/resources/guardrail.schema.json` are corrected here; the matching `cp-admin.yaml` prose is a separate control-plane PR. ## Sites Both changes apply at all four sites that raise this refusal: | | | |---|---| | `POST /v1/files` | invalid-UTF-8 upload under a text purpose | | `/v1/messages` | body the Anthropic scan parser rejects | | `/v1/messages/count_tokens` | same | | `/mcp` tool **result** | unparseable result — the mirror direction: it resolves one chain and uses it both ways, so an input-only row was refusing responses | ## Deliberately not changed Left alone deliberately — confirmed, not an oversight — with the reason recorded in `crates/aisix-proxy/AGENTS.md`: the refusals that need a **held-back stream** (`output_buffer_exceeded`, `mask_writeback_failed`, and `unscannable_body` on a buffered SSE body, in `messages.rs` / `responses.rs` / `passthrough_route.rs`). Those already gate on `runs_on_output(chain) && stream_output_policy().holds_back()`, so the direction half is correct there; honouring `fail_open` would not mean skipping a refusal but releasing already-buffered bytes that were never scanned, which is a different decision. `mcp.rs::moderate_selected_segments`'s collect-walk failure is gated on `moderates_segments(chain)`, which is not hook-aware, but that arm is structurally unreachable — every caller's body has already parsed as JSON — so there is no fail-before test to write for it. ## Implementation `Guardrail::runs_on_input` mirrors the existing `runs_on_output`; `fails_closed_on_input` / `_on_output` expose the per-hook failure policy; `refuses_unevaluable_input` / `_output` combine them, with `GuardrailChain` overriding to `any` over members. All default to the secure-leaning value, so a kind that forgets to override keeps today's refusal. `keyword` and `pii` did not store the row's `fail_open` before and now do. `GuardrailIndex::resolve` is untouched and what a resolved chain means elsewhere is unchanged. ## Tests Unit (`aisix-proxy`, `aisix-guardrails`) plus the `/v1/files` e2e case. Every new case is mutation-checked against the specific gate it covers: - output-only guardrail + invalid-UTF-8 upload + text purpose → forwarded byte-for-byte - `fail_open: true` request-side row, same upload → forwarded - the two halves on different rows → forwarded (the case a naive fold refuses) - one fail-closed request-side row, alone or mixed in → still 422, envelope unchanged - both hooks attached → still 422 - no guardrail → unchanged - the three sibling sites, each with its own direction and `fail_open` pair - `GuardrailChain::runs_on_input` and `refuses_unevaluable_*` across input-only / output-only / both / mixed / cross / empty The e2e file grows output-only, input+output and `fail_open: true` environments. Two legs assert the *premise* rather than assuming it — the mock now echoes the uploaded filename, so an output-hook row can be shown to block the response, and the fail-open row can be shown to still scan. Without those, "forwarded" would be the same observation as the no-guardrail environment and would pass just as well if the row had never reached the chain. Refs #1022, #1113, #1114. 🤖 Generated with [Claude Code](https://claude.com/claude-code)
…1121) `/v1/files` decoded an uploaded file with `String::from_utf8_lossy` and checked the result as one synthetic user message, while `create_file` forwarded the ORIGINAL bytes to the provider. That scan cannot do its job: it feeds JSON syntax to keyword matchers, answers a file holding thousands of independent requests with a single verdict, can rewrite nothing, and on a binary upload inspects a run of replacement characters. The download side had the same shape. Nothing narrower repairs it — screening a batch file means decomposing it per JSONL record and running the ordinary per-record request chain, which is #1120. Removed: the input blob scan on `POST /v1/files`, the output blob scan on its response and on every `/v1/files*` read, and the `unscannable_body` refusal built on the input scan (added by #1113, narrowed by #1114, gated by #1115) together with the purpose classification it keyed on. `forward_simple` skips the output chain for the files surface alone. `/v1/files` is now `Posture::Unscreened` in the guardrail coverage census, which says what the surface does rather than claiming enforcement it no longer has. Behaviour change: an upload that began answering `422` with the `unscannable_body` tag after #1113 is forwarded again, under every declared `purpose`. Capability narrowing: guardrails no longer apply to the files surface at all. An upload or a download that a keyword, PII or remote guardrail would previously have blocked on a policy match now reaches the provider and the caller respectively. The published documentation says input and output guardrails cover Files request and response payloads; that coverage is gone until #1120 lands. Unchanged: `/v1/batches` and `/v1/fine_tuning/jobs` still scan their serialised JSON request bodies and their responses — a serialised request body is not a caller-uploaded blob — and the `unscannable_body` gate stays on `/v1/messages`, `/v1/messages/count_tokens` and `/mcp` tool results, with `fail_open` governing every guardrail kind as before. Three statements elsewhere in the crate that described the files surface as screened are corrected, and the two job surfaces that still screen are promoted to `Posture::Enforced` with census fixtures, since this change's own tests disproved the premise that kept them out. Refs #1120, #1022
Problem
#1113 made
POST /v1/filesrefuse an upload whose bytes are not valid UTF-8 whenever an input guardrail chain resolves. The refusal reads only thefilepart — it is purpose-blind — so a deployment running any input guardrail started getting 422 for a PDF, an image, or any other binary uploaded underpurpose=assistants/vision/user_data. That is well beyond the malformed-JSONL case the refusal was built for, and it breaks legitimate uploads.What changed
The refusal is now keyed on the declared multipart
purpose. An undecodable blob is refused only under a purpose whose payload is contractually UTF-8 text:purposebatch,fine-tune,evalsassistants,vision,user_databatch_output)A body declaring
purposemore than once is malformed, and the provider picks whichever part it picks — so the classification folds rather than overwrites: any declared text purpose refuses, whatever order the parts arrive in. Taking the last one would let the gateway and the provider disagree about what the payload is.batchandfine-tunetake JSONL; an eval data set referenced by file id is a JSONL source too. The rest legitimately carry binary — an assistants document, a vision image, or "any purpose" — so an undecodable blob there is a normal upload, not an evasion. An upload we cannot classify is not one we can claim should have been text, so an absent or unrecognised purpose is never refused.The scan itself is unchanged. A non-text-purpose upload keeps the best-effort lossy scan it has always had, so a term that survives
from_utf8_lossystill blocks. Narrowing the refusal must not turn into skipping the chain, and one unit leg plus one e2e leg exist specifically to fail if it ever does. The residual asymmetry — a binary upload is scanned on a lossy decode and forwarded verbatim — is this surface's deliberate posture and stays;scan_output_blob's note on the download side is updated to say which signal the input side actually keys on.This route never validates
purpose; it forwards whatever the caller declared, so the classification is an exact match against the text set and nothing else. The other threescan_input_blobcall sites all declare the text posture, and the refusal arm is unreachable at every one of them:/v1/batchesand/v1/fine_tuning/jobspassserde_json::to_vecoutput, which is valid UTF-8 by construction, and no caller setsFwdSpec::body, so the shared forwarder never scans anything today.Behaviour change
This narrows the 422 introduced by #1113. An upload whose
filepart is not valid UTF-8 is refused only when the declaredpurposeisbatch,fine-tuneorevals, and an input guardrail chain resolves. Under any other purpose, an unrecognised one, or none at all, such an upload is scanned lossily and forwarded to the provider exactly as it was before #1113. Uploads under a text purpose keep #1113's envelope unchanged: 422 witherror.type: content_filter,error.code: guardrail_unavailable, andunscannable_bodyin the message. Deployments with no guardrail attached remain unaffected on every purpose.Baseline comparison
The established gateways gate file-upload guardrail treatment on
purposerather than applying it blindly — the closest equivalent scans batch input records only whenpurpose == "batch", and rejects an unlisted purpose outright at 400. This change lands on the same axis (purpose decides), with two deliberate differences: the refusal set also coversfine-tuneandevals, which are JSONL by the same contract asbatch; and this gateway keeps forwarding an unrecognised purpose rather than validating it, which is a pre-existing passthrough property this PR does not touch. The upstream file-purpose set used above is the one the official SDK declares for the upload parameter (assistants,batch,fine-tune,vision,user_data,evals);assistants_output,batch_outputandfine-tune-resultsare server-minted and appear only on a stored file object.Tests
Unit (
crates/aisix-proxy/src/jobs.rs) — each verified to fail under a mutation before passing:undecodable_posture_classifies_every_declared_purpose— the full table in one place: three text purposes refuse; the three binary upload purposes, the three server-minted values, a case variant, a leading-space variant, an empty string, an invented value andNonedo not.non_utf8_upload_under_a_binary_purpose_is_forwarded— the fix(guardrails): refuse an unscannable /v1/files upload instead of forwarding it #1113 regression: 200, bytes forwarded byte-for-byte. Fails onc2e89aa9with 422.non_utf8_upload_without_a_purpose_is_forwarded— also fails onc2e89aa9with 422.binary_purpose_upload_is_still_scanned_lossily— an ASCII term that survives the lossy decode still blocks with a policy hit (nounscannable_bodytag). Fails if the chain is skipped for non-text purposes rather than the refusal narrowed. The term sits after the invalid bytes, so replacing the lossy decode with a scan of the valid prefix alone also fails it — which is the shape a real evasion takes.duplicate_purpose_fields_take_the_stricter_reading— twopurposeparts ordered text-then-binary still refuse. Fails against a last-wins read.non_utf8_upload_is_refused_and_never_forwardedandnon_utf8_upload_without_a_guardrail_still_forwardsfrom fix(guardrails): refuse an unscannable /v1/files upload instead of forwarding it #1113 are unchanged and still green.E2E (
tests/e2e/src/cases/files-unscannable-upload-e2e.test.ts) — the same boundary against a real binary + etcd + upstream recorder:assistants/vision/user_dataand a purpose-less upload forward byte-for-byte;fine-tunerefuses on the same terms asbatch; and a binary-purpose upload carrying a matchable term is still blocked by the policy rather than refused as unreadable.Mutations checked: purpose-blind refusal (4 tests red), chain skipped for non-text purposes (1 test red), wrong purpose set (3 tests red), last-wins on a duplicated
purpose(1 test red), lossy decode replaced by a valid-prefix-only scan (1 test red).Ref #1022, #1113
🤖 Generated with Claude Code
Summary by CodeRabbit