From 02297d2915d92d06909e61d2d1b55bc62c6bd782 Mon Sep 17 00:00:00 2001 From: Jarvis Date: Thu, 3 Sep 2026 00:25:38 +0000 Subject: [PATCH 1/3] fix(guardrails): refuse an unscannable /v1/files upload only under a text purpose MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit #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 --- crates/aisix-proxy/src/jobs.rs | 298 ++++++++++++++++-- .../files-unscannable-upload-e2e.test.ts | 133 +++++++- 2 files changed, 388 insertions(+), 43 deletions(-) diff --git a/crates/aisix-proxy/src/jobs.rs b/crates/aisix-proxy/src/jobs.rs index c318f5019..7d050059a 100644 --- a/crates/aisix-proxy/src/jobs.rs +++ b/crates/aisix-proxy/src/jobs.rs @@ -447,15 +447,48 @@ async fn send_upstream( )) } +/// Purposes whose uploaded payload is contractually UTF-8 text, and so the +/// only ones under which a blob the scanner cannot decode is refused. +/// +/// `batch` and `fine-tune` take JSONL; an eval data set referenced by file +/// id is a JSONL source too. The remaining upload purposes — `assistants`, +/// `vision`, `user_data` — legitimately carry binary (a PDF, an image, or +/// "any purpose" respectively), so an undecodable blob there is a normal +/// upload rather than an evasion. This route never validates `purpose`: +/// it forwards whatever the caller declared, so an unrecognised or absent +/// value is not classifiable as text and is not refused. +const TEXT_PURPOSES: [&str; 3] = ["batch", "fine-tune", "evals"]; + +/// What [`scan_input_blob`] does with a payload it cannot decode as UTF-8. +#[derive(Clone, Copy, PartialEq, Eq, Debug)] +enum Undecodable { + /// The payload is contractually UTF-8 text, so invalid bytes are + /// refused rather than hidden behind a lossy decode (#1022). + Refuse, + /// The payload may legitimately be binary — keep the best-effort lossy + /// scan and let the upload through. + ScanLossily, +} + +/// Classify a declared multipart `purpose` for the refusal above. +fn undecodable_posture(purpose: Option<&str>) -> Undecodable { + match purpose { + Some(p) if TEXT_PURPOSES.contains(&p) => Undecodable::Refuse, + _ => Undecodable::ScanLossily, + } +} + /// Run the resolved input-guardrail chain over an opaque blob (whole-body /// text scan — the `/passthrough` precedent from #911 [6]). A blob that is -/// not valid UTF-8 is refused rather than scanned lossily; see the arm -/// below. +/// not valid UTF-8 is refused rather than scanned lossily when the payload +/// is contractually text; see the arm below. +#[allow(clippy::too_many_arguments)] async fn scan_input_blob( state: &ProxyState, auth: &AuthenticatedKey, target: &JobTarget, blob: &[u8], + undecodable: Undecodable, monitor_hits: &mut Vec, // Accumulated rather than handed back as one handle: the input and // output scans resolve their own chain, so a request can produce two @@ -474,21 +507,26 @@ async fn scan_input_blob( if chain.is_empty() { return Ok(()); } - // Fail closed on a blob the scanner cannot read — the same arm as - // `messages.rs` / `count_tokens.rs` / `mcp.rs`, and the reason the - // scan below no longer decodes lossily. `from_utf8_lossy` replaces - // every invalid sequence with U+FFFD, so a term written in a + // Fail closed on a text payload the scanner cannot read — the same arm + // as `messages.rs` / `count_tokens.rs` / `mcp.rs`. `from_utf8_lossy` + // replaces every invalid sequence with U+FFFD, so a term written in a // non-UTF-8 encoding never appeared in the scanned text while // `create_file` forwarded the ORIGINAL bytes to the provider (#1022): // the guardrail was offered a redacted copy of the payload and the // payload left the boundary anyway. // - // Reached only once a chain is attached, which is deliberate. An + // Only a payload the declared `purpose` says is UTF-8 text can be + // refused on that ground; a binary-purpose upload keeps the lossy scan + // it has always had, and is still forwarded verbatim. That asymmetry is + // deliberate — refusing it would break lawful PDF/image uploads, which + // is a product decision, not a guardrail one. + // + // Reached only once a chain is attached, which is also deliberate. An // upload nobody screens keeps forwarding whatever it forwards today; // this is a guardrail refusal, not a structural check on the file. - let text = match std::str::from_utf8(blob) { - Ok(text) => text, - Err(err) => { + let text = match (std::str::from_utf8(blob), undecodable) { + (Ok(text), _) => std::borrow::Cow::Borrowed(text), + (Err(err), Undecodable::Refuse) => { tracing::warn!( guardrail_hook = "input", model = %target.display_name(), @@ -501,10 +539,11 @@ async fn scan_input_blob( Some(crate::error::TAG_UNSCANNABLE_BODY), )); } + (Err(_), Undecodable::ScanLossily) => String::from_utf8_lossy(blob), }; let chat = aisix_gateway::ChatFormat::new( target.display_name(), - vec![aisix_gateway::ChatMessage::user(text.to_owned())], + vec![aisix_gateway::ChatMessage::user(text.into_owned())], ); let (verdict, hits) = aisix_guardrails::Guardrail::check_input_observed(&chain, &chat).await; monitor_hits.extend(hits); @@ -536,16 +575,17 @@ async fn scan_input_blob( /// Output-side counterpart of [`scan_input_blob`] — but NOT a symmetric /// one, and deliberately not renamed to hide that. /// -/// The input side refuses a blob it cannot decode (#1022). This side still -/// scans `from_utf8_lossy` and still relays the original bytes, so the same +/// The input side refuses a blob it cannot decode when the declared +/// `purpose` says the payload is UTF-8 text (#1022). This side still scans +/// `from_utf8_lossy` and still relays the original bytes, so the same /// evasion is open on `GET /v1/files/{id}/content`, whose `relay_raw_body` /// arm returns the provider's bytes untouched. That is not an oversight to /// tidy up in passing: the download path legitimately carries binary the -/// caller never chose (whatever the provider holds under a file id), -/// whereas an upload's bytes come from the caller and a batch/fine-tune -/// input is contractually UTF-8 JSONL. Failing closed here would refuse -/// lawful downloads, so the direction needs a product decision rather than -/// a mirrored `match`. Tracked in #1022. +/// caller never chose (whatever the provider holds under a file id), and it +/// has no `purpose` to classify it by — the upload's `purpose` is what lets +/// the input side tell a JSONL payload from a lawful PDF. Failing closed +/// here would refuse lawful downloads, so the direction needs a product +/// decision rather than a mirrored `match`. Tracked in #1022. async fn scan_output_blob( state: &ProxyState, auth: &AuthenticatedKey, @@ -940,6 +980,9 @@ pub(crate) async fn create_file( let mut form = reqwest::multipart::Form::new(); let mut form_model: Option = None; let mut file_bytes: Option = None; + // Read but not consumed: `purpose` forwards verbatim like any other + // text field, and only classifies the blob for the scan below. + let mut purpose: Option = None; while let Some(field) = multipart.next_field().await.map_err(|e| { crate::error::proxy_error_from_multipart( @@ -989,6 +1032,9 @@ pub(crate) async fn create_file( "malformed multipart field", ) })?; + if name == "purpose" { + purpose = Some(v.clone()); + } form = form.text(name, v); } @@ -1000,13 +1046,15 @@ pub(crate) async fn create_file( let snapshot = &**snapshot.insert(state.snapshot.load()); let target = resolve_target(snapshot, &auth, wanted.as_deref(), &client)?; - // Batch/fine-tune input files carry end-user content — scan them - // like any other inbound payload. + // Uploads carry end-user content — scan them like any other inbound + // payload. Only a text `purpose` may be refused for being + // undecodable; see `undecodable_posture`. scan_input_blob( &state, &auth, &target, &file_bytes, + undecodable_posture(purpose.as_deref()), &mut monitor_hits, &mut enforced_hits, &mut scores, @@ -1272,6 +1320,10 @@ pub(crate) async fn create_batch( &auth, &target, &out_body, + // A `serde_json` re-serialisation is valid UTF-8 by + // construction, so the refusal arm is unreachable here; the + // posture still states what the payload is. + Undecodable::Refuse, &mut monitor_hits, &mut enforced_hits, &mut scores, @@ -1560,6 +1612,10 @@ pub(crate) async fn create_ft_job( &auth, &target, &out_body, + // A `serde_json` re-serialisation is valid UTF-8 by + // construction, so the refusal arm is unreachable here; the + // posture still states what the payload is. + Undecodable::Refuse, &mut monitor_hits, &mut enforced_hits, &mut scores, @@ -1766,6 +1822,8 @@ async fn forward_simple( &auth, &target, body, + // As above: `spec.body` is a re-serialised JSON body. + Undecodable::Refuse, &mut monitor_hits, &mut enforced_hits, &mut scores, @@ -2235,6 +2293,17 @@ mod tests { /// [`multipart_body`] with the `file` part's bytes chosen by the /// caller, so a test can upload something that is not valid UTF-8. fn multipart_body_with_file(boundary: &str, model: Option<&str>, file: &[u8]) -> Vec { + multipart_body_with_purpose(boundary, model, Some("batch"), file) + } + + /// [`multipart_body_with_file`] with the declared `purpose` chosen by + /// the caller — `None` omits the field entirely. + fn multipart_body_with_purpose( + boundary: &str, + model: Option<&str>, + purpose: Option<&str>, + file: &[u8], + ) -> Vec { let mut b = Vec::new(); if let Some(m) = model { b.extend_from_slice( @@ -2244,12 +2313,14 @@ mod tests { .as_bytes(), ); } - b.extend_from_slice( - format!( - "--{boundary}\r\ncontent-disposition: form-data; name=\"purpose\"\r\n\r\nbatch\r\n" - ) - .as_bytes(), - ); + if let Some(p) = purpose { + b.extend_from_slice( + format!( + "--{boundary}\r\ncontent-disposition: form-data; name=\"purpose\"\r\n\r\n{p}\r\n" + ) + .as_bytes(), + ); + } b.extend_from_slice( format!( "--{boundary}\r\ncontent-disposition: form-data; name=\"file\"; filename=\"input.jsonl\"\r\ncontent-type: application/jsonl\r\n\r\n" @@ -2832,6 +2903,14 @@ mod tests { } fn upload_request(boundary: &str, file: &[u8]) -> Request { + upload_request_with_purpose(boundary, Some("batch"), file) + } + + fn upload_request_with_purpose( + boundary: &str, + purpose: Option<&str>, + file: &[u8], + ) -> Request { Request::builder() .method("POST") .uri("/v1/files") @@ -2840,9 +2919,10 @@ mod tests { "content-type", format!("multipart/form-data; boundary={boundary}"), ) - .body(axum::body::Body::from(multipart_body_with_file( + .body(axum::body::Body::from(multipart_body_with_purpose( boundary, Some("jobs-a"), + purpose, file, ))) .unwrap() @@ -2961,4 +3041,168 @@ mod tests { "a decodable upload must still forward" ); } + + /// The whole set the refusal keys on, in one place. The route itself + /// never validates `purpose` — it forwards whatever the caller sent — + /// so this table is the only statement of which declared values mean + /// "the payload is contractually UTF-8 text". + /// + /// The six upload purposes are the ones a caller may declare; the + /// three `*_output` / `*-results` values are server-minted and appear + /// only on a stored file object, but are listed so a future reader + /// does not have to guess whether they were considered. + #[test] + fn undecodable_posture_classifies_every_declared_purpose() { + for text in ["batch", "fine-tune", "evals"] { + assert_eq!( + undecodable_posture(Some(text)), + Undecodable::Refuse, + "{text} takes JSONL and must refuse an undecodable blob" + ); + } + for binary in [ + // Upload purposes whose payload may legitimately be binary. + "assistants", + "vision", + "user_data", + // Server-minted purposes; a caller cannot upload under them. + "assistants_output", + "batch_output", + "fine-tune-results", + // Nothing normalises the value, so a case variant or a + // provider-specific extension is simply unrecognised. + "BATCH", + "fine_tune", + " batch", + "something-a-provider-invented", + "", + ] { + assert_eq!( + undecodable_posture(Some(binary)), + Undecodable::ScanLossily, + "{binary:?} must not be refused for being undecodable" + ); + } + assert_eq!( + undecodable_posture(None), + Undecodable::ScanLossily, + "an upload we cannot classify is not one we can call text" + ); + } + + /// The regression #1113 introduced: its refusal was purpose-blind, so + /// a PDF or an image uploaded under `purpose=assistants` started + /// returning 422 for any deployment running an input guardrail. Fails + /// on `c2e89aa9` with 422. + #[tokio::test] + async fn non_utf8_upload_under_a_binary_purpose_is_forwarded() { + let upstream = MockServer::start().await; + files_upstream_mock().expect(1).mount(&upstream).await; + + let snap = AisixSnapshot::new(); + snap.provider_keys.insert(openai_pk(PK_A, &upstream.uri())); + snap.models.insert(model("m-a", "jobs-a", PK_A)); + snap.apikeys.insert(apikey_entry(&["*"])); + seed_never_matching_guardrail(&snap); + let app = build_app(snap); + + let resp = app + .oneshot(upload_request_with_purpose( + "XBOUNDARYX", + Some("assistants"), + NON_UTF8_UPLOAD, + )) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::OK); + + let received = upstream.received_requests().await.unwrap(); + assert_eq!(received.len(), 1); + assert!( + received[0] + .body + .windows(NON_UTF8_UPLOAD.len()) + .any(|w| w == NON_UTF8_UPLOAD), + "a binary-purpose upload must still forward byte-for-byte" + ); + } + + /// An upload we cannot classify is not one we can claim should have + /// been text. Also fails on `c2e89aa9` with 422. + #[tokio::test] + async fn non_utf8_upload_without_a_purpose_is_forwarded() { + let upstream = MockServer::start().await; + files_upstream_mock().expect(1).mount(&upstream).await; + + let snap = AisixSnapshot::new(); + snap.provider_keys.insert(openai_pk(PK_A, &upstream.uri())); + snap.models.insert(model("m-a", "jobs-a", PK_A)); + snap.apikeys.insert(apikey_entry(&["*"])); + seed_never_matching_guardrail(&snap); + let app = build_app(snap); + + let resp = app + .oneshot(upload_request_with_purpose( + "XBOUNDARYX", + None, + NON_UTF8_UPLOAD, + )) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::OK); + assert_eq!(upstream.received_requests().await.unwrap().len(), 1); + } + + /// Not refusing is not the same as not scanning. A binary-purpose + /// upload keeps the best-effort lossy scan it has always had, so a + /// term that survives the lossy decode still blocks — this is the + /// assertion that fails if someone "fixes" the regression by skipping + /// the chain for non-text purposes instead of narrowing the refusal. + #[tokio::test] + async fn binary_purpose_upload_is_still_scanned_lossily() { + let upstream = MockServer::start().await; + files_upstream_mock().expect(0).mount(&upstream).await; + + let snap = AisixSnapshot::new(); + snap.provider_keys.insert(openai_pk(PK_A, &upstream.uri())); + snap.models.insert(model("m-a", "jobs-a", PK_A)); + snap.apikeys.insert(apikey_entry(&["*"])); + let g: aisix_core::Guardrail = serde_json::from_str( + r#"{"name":"blocks-a-term","enabled":true,"hook_point":"input","fail_open":false,"kind":"keyword","patterns":[{"kind":"literal","value":"forbidden-term"}]}"#, + ) + .unwrap(); + crate::seed_env_scoped_guardrail( + &snap, + aisix_core::resource::ResourceEntry::new("g-1", g, 1), + ); + let app = build_app(snap); + + // The term is ASCII, so it survives `from_utf8_lossy` intact even + // though the blob as a whole does not decode. + let blob: &[u8] = b"forbidden-term \xc4\xe3\xba\xc3 trailing"; + let resp = app + .oneshot(upload_request_with_purpose( + "XBOUNDARYX", + Some("assistants"), + blob, + )) + .await + .unwrap(); + assert_eq!(resp.status(), StatusCode::UNPROCESSABLE_ENTITY); + let bytes = to_bytes(resp.into_body(), 65536).await.unwrap(); + let v: Value = serde_json::from_slice(&bytes).unwrap(); + assert_eq!(v["error"]["type"], "content_filter", "{v}"); + // A policy hit, not the unscannable-body refusal: that one carries + // `code: guardrail_unavailable` and names the tag in the message, + // while a real block carries no code and names the guardrail. + assert!(v["error"]["code"].is_null(), "{v}"); + assert!( + v["error"]["message"] + .as_str() + .unwrap() + .contains("blocks-a-term"), + "the blob was scanned and hit the policy, not refused as \ + unreadable: {v}" + ); + } } diff --git a/tests/e2e/src/cases/files-unscannable-upload-e2e.test.ts b/tests/e2e/src/cases/files-unscannable-upload-e2e.test.ts index 3c45c9698..f056daf6c 100644 --- a/tests/e2e/src/cases/files-unscannable-upload-e2e.test.ts +++ b/tests/e2e/src/cases/files-unscannable-upload-e2e.test.ts @@ -19,11 +19,15 @@ import { // The upstream recorder is the load-bearing assertion in all three legs: // the whole bug was that a "scanned" upload still reached the provider. // -// The scope boundary is pinned here too, deliberately. The refusal is -// conditional on a guardrail being attached — this is NOT structural -// validation of the file, and an operator running no guardrails sees no -// behaviour change at all. The second leg is the one that fails if someone -// later widens this into an unconditional UTF-8 check on the files API. +// The scope boundary is pinned here too, deliberately, on two axes. The +// refusal is conditional on a guardrail being attached — this is NOT +// structural validation of the file, and an operator running no guardrails +// sees no behaviour change at all. And it is conditional on the declared +// `purpose` naming a payload that is contractually UTF-8 text (`batch`, +// `fine-tune`, `evals`); an `assistants`/`vision`/`user_data` upload, or +// one that declares no purpose at all, legitimately carries binary and is +// forwarded as it always was. The later legs are what fail if someone +// widens this back into an unconditional UTF-8 check on the files API. const GUARDED_KEY = "sk-files-unscannable-guarded"; const UNGUARDED_KEY = "sk-files-unscannable-open"; @@ -40,18 +44,29 @@ const NON_UTF8_LINE = Buffer.concat([ /** The same term, correctly encoded. */ const CLEAN_LINE = Buffer.from('{"custom_id":"r1","note":"你好"}\n', "utf8"); -/** A keyword row that cannot match these fixtures: the refusal must come - * from the blob being unscannable, not from a hit. A fix that simply - * started blocking every upload fails the third leg. */ -const NEVER_MATCHING_GUARDRAIL = { +/** The input chain these tests attach. Its first pattern cannot match any + * fixture, so a refusal must come from the blob being unscannable rather + * than from a hit — a fix that simply started blocking every upload fails + * the forwarding legs. The second pattern appears in exactly one fixture, + * the one that proves a binary-purpose upload is still scanned. */ +const GUARDRAIL = { name: "files-unscannable-e2e", enabled: true, hook_point: "input", fail_open: false, kind: "keyword", - patterns: [{ kind: "literal", value: "zzz-no-such-term-zzz" }], + patterns: [ + { kind: "literal", value: "zzz-no-such-term-zzz" }, + // The one term these fixtures can hit, used by the final leg to prove + // a binary-purpose upload is still scanned rather than skipped. Kept + // out of every other fixture above. + { kind: "literal", value: "zzz-blocked-term-zzz" }, + ], }; +/** The one term {@link GUARDRAIL} can actually hit. */ +const BLOCKED_TERM = "zzz-blocked-term-zzz"; + interface FilesUpstream { baseUrl: string; uploads: Buffer[]; @@ -94,12 +109,22 @@ async function startFilesUpstream(): Promise { }; } -/** Hand-built multipart so the `file` part's bytes survive verbatim. */ -function multipartUpload(boundary: string, model: string, file: Buffer): Buffer { +/** Hand-built multipart so the `file` part's bytes survive verbatim. + * `purpose` is omitted entirely when null. */ +function multipartUpload( + boundary: string, + model: string, + purpose: string | null, + file: Buffer, +): Buffer { + const purposePart = + purpose === null + ? "" + : `--${boundary}\r\ncontent-disposition: form-data; name="purpose"\r\n\r\n${purpose}\r\n`; return Buffer.concat([ Buffer.from( `--${boundary}\r\ncontent-disposition: form-data; name="model"\r\n\r\n${model}\r\n` + - `--${boundary}\r\ncontent-disposition: form-data; name="purpose"\r\n\r\nbatch\r\n` + + purposePart + `--${boundary}\r\ncontent-disposition: form-data; name="file"; filename="input.jsonl"\r\n` + `content-type: application/jsonl\r\n\r\n`, "utf8", @@ -138,7 +163,7 @@ const startEnv = async ( provider_key_id: pk.id, }); if (guarded) { - await seed.createGuardrail(NEVER_MATCHING_GUARDRAIL); + await seed.createGuardrail(GUARDRAIL); } // Written LAST: the key authenticating implies every row above it // landed (etcd applies in revision order). @@ -161,7 +186,7 @@ const startEnv = async ( return { app, upstream, key, model }; }; -const upload = (env: Env, file: Buffer) => { +const upload = (env: Env, file: Buffer, purpose: string | null = "batch") => { const boundary = "XFILESUNSCANNABLEX"; return fetch(`${env.app.proxyUrl}/v1/files`, { method: "POST", @@ -169,7 +194,7 @@ const upload = (env: Env, file: Buffer) => { authorization: `Bearer ${env.key}`, "content-type": `multipart/form-data; boundary=${boundary}`, }, - body: multipartUpload(boundary, env.model, file), + body: multipartUpload(boundary, env.model, purpose, file), }); }; @@ -235,4 +260,80 @@ describe("files: an unscannable upload is refused, not forwarded", () => { expect(guarded.upstream.uploads).toHaveLength(before + 1); expect(guarded.upstream.uploads[before].includes(CLEAN_LINE)).toBe(true); }); + + // A binary upload under a purpose whose payload is not contractually + // text is the case the first version of this refusal broke: a PDF or an + // image started returning 422 for every deployment running any input + // guardrail. + for (const purpose of ["assistants", "vision", "user_data"]) { + test(`a non-UTF-8 blob under purpose=${purpose} is forwarded, not refused`, async (ctx) => { + if (!etcdReachable || !guarded) { + ctx.skip(); + return; + } + const before = guarded.upstream.uploads.length; + const res = await upload(guarded, NON_UTF8_LINE, purpose); + expect(res.status).toBe(200); + expect(guarded.upstream.uploads).toHaveLength(before + 1); + // Byte-for-byte: the refusal was narrowed, the forwarding was not. + expect(guarded.upstream.uploads[before].includes(NON_UTF8_LINE)).toBe( + true, + ); + }); + } + + test("a non-UTF-8 blob with no declared purpose is forwarded", async (ctx) => { + if (!etcdReachable || !guarded) { + ctx.skip(); + return; + } + // An upload we cannot classify is not one we can claim should have + // been text. + const before = guarded.upstream.uploads.length; + const res = await upload(guarded, NON_UTF8_LINE, null); + expect(res.status).toBe(200); + expect(guarded.upstream.uploads).toHaveLength(before + 1); + }); + + test("purpose=fine-tune is refused on the same terms as batch", async (ctx) => { + if (!etcdReachable || !guarded) { + ctx.skip(); + return; + } + const before = guarded.upstream.uploads.length; + const res = await upload(guarded, NON_UTF8_LINE, "fine-tune"); + expect(res.status).toBe(422); + const body = (await res.json()) as { error?: { message?: string } }; + expect(body.error?.message).toContain("unscannable_body"); + expect(guarded.upstream.uploads).toHaveLength(before); + }); + + test("a binary purpose is still SCANNED, just not refused", async (ctx) => { + if (!etcdReachable || !guarded) { + ctx.skip(); + return; + } + // The chain still runs on a lossy decode, so a term that survives it + // still blocks. This is the leg that fails if someone "fixes" the + // regression by skipping the chain for non-text purposes instead of + // narrowing the refusal. + const before = guarded.upstream.uploads.length; + const res = await upload( + guarded, + Buffer.concat([ + Buffer.from(`{"note":"${BLOCKED_TERM}","x":"`, "utf8"), + Buffer.from([0xc4, 0xe3, 0xba, 0xc3]), + Buffer.from('"}\n', "utf8"), + ]), + "assistants", + ); + expect(res.status).toBe(422); + const body = (await res.json()) as { + error?: { type?: string; message?: string }; + }; + expect(body.error?.type).toBe("content_filter"); + // A policy hit, not the unscannable-body refusal. + expect(body.error?.message).not.toContain("unscannable_body"); + expect(guarded.upstream.uploads).toHaveLength(before); + }); }); From 80c28f808c261bb810c12bb9faf6cc9bcf228742 Mon Sep 17 00:00:00 2001 From: Jarvis Date: Thu, 3 Sep 2026 00:31:30 +0000 Subject: [PATCH 2/3] fix(guardrails): fold a duplicated /v1/files `purpose` fail-closed MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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. --- crates/aisix-proxy/src/jobs.rs | 68 ++++++++++++++++++++++++++++++++-- 1 file changed, 64 insertions(+), 4 deletions(-) diff --git a/crates/aisix-proxy/src/jobs.rs b/crates/aisix-proxy/src/jobs.rs index 7d050059a..e3c883844 100644 --- a/crates/aisix-proxy/src/jobs.rs +++ b/crates/aisix-proxy/src/jobs.rs @@ -982,7 +982,13 @@ pub(crate) async fn create_file( let mut file_bytes: Option = None; // Read but not consumed: `purpose` forwards verbatim like any other // text field, and only classifies the blob for the scan below. - let mut purpose: Option = None; + // + // Folded rather than overwritten, so a body declaring `purpose` + // more than once takes the STRICTER reading. Such a body is + // malformed and the provider picks whichever part it picks; taking + // the last one would let a caller append a binary purpose after a + // text one to pick the looser reading for us. + let mut undecodable = Undecodable::ScanLossily; while let Some(field) = multipart.next_field().await.map_err(|e| { crate::error::proxy_error_from_multipart( @@ -1032,8 +1038,8 @@ pub(crate) async fn create_file( "malformed multipart field", ) })?; - if name == "purpose" { - purpose = Some(v.clone()); + if name == "purpose" && undecodable_posture(Some(v.as_str())) == Undecodable::Refuse { + undecodable = Undecodable::Refuse; } form = form.text(name, v); } @@ -1054,7 +1060,7 @@ pub(crate) async fn create_file( &auth, &target, &file_bytes, - undecodable_posture(purpose.as_deref()), + undecodable, &mut monitor_hits, &mut enforced_hits, &mut scores, @@ -3127,6 +3133,60 @@ mod tests { ); } + /// A body declaring `purpose` more than once is malformed, and the + /// provider picks whichever part it picks — so the classification takes + /// the stricter reading rather than the last one written. Ordered + /// text-then-binary, which is the order that would pick `ScanLossily` + /// if the field were simply overwritten. + #[tokio::test] + async fn duplicate_purpose_fields_take_the_stricter_reading() { + let upstream = MockServer::start().await; + files_upstream_mock().expect(0).mount(&upstream).await; + + let snap = AisixSnapshot::new(); + snap.provider_keys.insert(openai_pk(PK_A, &upstream.uri())); + snap.models.insert(model("m-a", "jobs-a", PK_A)); + snap.apikeys.insert(apikey_entry(&["*"])); + seed_never_matching_guardrail(&snap); + let app = build_app(snap); + + let boundary = "XBOUNDARYX"; + let mut body = + multipart_body_with_purpose(boundary, Some("jobs-a"), Some("batch"), NON_UTF8_UPLOAD); + // Splice a second, binary `purpose` part in after the first: the + // closing delimiter is rewritten into another part. + let closing = format!("--{boundary}--\r\n"); + let extra = format!( + "--{boundary}\r\ncontent-disposition: form-data; \ + name=\"purpose\"\r\n\r\nassistants\r\n--{boundary}--\r\n" + ); + let at = body.len() - closing.len(); + body.truncate(at); + body.extend_from_slice(extra.as_bytes()); + + let req = Request::builder() + .method("POST") + .uri("/v1/files") + .header("authorization", "Bearer sk-caller") + .header( + "content-type", + format!("multipart/form-data; boundary={boundary}"), + ) + .body(axum::body::Body::from(body)) + .unwrap(); + let resp = app.oneshot(req).await.unwrap(); + assert_eq!(resp.status(), StatusCode::UNPROCESSABLE_ENTITY); + let bytes = to_bytes(resp.into_body(), 65536).await.unwrap(); + let v: Value = serde_json::from_slice(&bytes).unwrap(); + assert!( + v["error"]["message"] + .as_str() + .unwrap() + .contains(crate::error::TAG_UNSCANNABLE_BODY), + "{v}" + ); + } + /// An upload we cannot classify is not one we can claim should have /// been text. Also fails on `c2e89aa9` with 422. #[tokio::test] From acf7d87ffde0bf83729046bf009f41493c74093b Mon Sep 17 00:00:00 2001 From: Jarvis Date: Thu, 3 Sep 2026 00:39:01 +0000 Subject: [PATCH 3/3] test(guardrails): pin the lossy scan against a valid-prefix-only decode MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `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). --- crates/aisix-proxy/src/jobs.rs | 18 +++++++++++++----- .../cases/files-unscannable-upload-e2e.test.ts | 8 ++++++-- 2 files changed, 19 insertions(+), 7 deletions(-) diff --git a/crates/aisix-proxy/src/jobs.rs b/crates/aisix-proxy/src/jobs.rs index e3c883844..1480e94bf 100644 --- a/crates/aisix-proxy/src/jobs.rs +++ b/crates/aisix-proxy/src/jobs.rs @@ -450,8 +450,10 @@ async fn send_upstream( /// Purposes whose uploaded payload is contractually UTF-8 text, and so the /// only ones under which a blob the scanner cannot decode is refused. /// -/// `batch` and `fine-tune` take JSONL; an eval data set referenced by file -/// id is a JSONL source too. The remaining upload purposes — `assistants`, +/// `batch` and `fine-tune` take JSONL, and so does an eval data set: every +/// eval run data source that accepts a file id declares that file to be a +/// JSONL source, with no binary variant. The remaining upload purposes of +/// the six a caller may declare — `assistants`, /// `vision`, `user_data` — legitimately carry binary (a PDF, an image, or /// "any purpose" respectively), so an undecodable blob there is a normal /// upload rather than an evasion. This route never validates `purpose`: @@ -1828,7 +1830,9 @@ async fn forward_simple( &auth, &target, body, - // As above: `spec.body` is a re-serialised JSON body. + // No caller sets `spec.body` today, so this is unreachable + // too; if one ever does it will be a re-serialised JSON + // body, which makes text the right posture for it. Undecodable::Refuse, &mut monitor_hits, &mut enforced_hits, @@ -3238,8 +3242,12 @@ mod tests { let app = build_app(snap); // The term is ASCII, so it survives `from_utf8_lossy` intact even - // though the blob as a whole does not decode. - let blob: &[u8] = b"forbidden-term \xc4\xe3\xba\xc3 trailing"; + // though the blob as a whole does not decode. It sits AFTER the + // invalid bytes deliberately: scanning only the valid prefix (a + // natural-looking "optimisation" of the lossy decode) would miss + // it, and that is the shape a real evasion takes — a few bad bytes + // up front, the payload behind them. + let blob: &[u8] = b"leading \xc4\xe3\xba\xc3 forbidden-term trailing"; let resp = app .oneshot(upload_request_with_purpose( "XBOUNDARYX", diff --git a/tests/e2e/src/cases/files-unscannable-upload-e2e.test.ts b/tests/e2e/src/cases/files-unscannable-upload-e2e.test.ts index f056daf6c..0acfe32df 100644 --- a/tests/e2e/src/cases/files-unscannable-upload-e2e.test.ts +++ b/tests/e2e/src/cases/files-unscannable-upload-e2e.test.ts @@ -317,13 +317,17 @@ describe("files: an unscannable upload is refused, not forwarded", () => { // still blocks. This is the leg that fails if someone "fixes" the // regression by skipping the chain for non-text purposes instead of // narrowing the refusal. + // + // The term sits AFTER the invalid bytes on purpose: scanning only the + // valid UTF-8 prefix would miss it, and that is the shape a real + // evasion takes — a few bad bytes up front, the payload behind them. const before = guarded.upstream.uploads.length; const res = await upload( guarded, Buffer.concat([ - Buffer.from(`{"note":"${BLOCKED_TERM}","x":"`, "utf8"), + Buffer.from('{"x":"', "utf8"), Buffer.from([0xc4, 0xe3, 0xba, 0xc3]), - Buffer.from('"}\n', "utf8"), + Buffer.from(`","note":"${BLOCKED_TERM}"}\n`, "utf8"), ]), "assistants", );