From bb442a8aaf4e564f2698c61581ab4c75c02fc43a Mon Sep 17 00:00:00 2001 From: Simo Lin Date: Fri, 24 Apr 2026 13:17:59 -0700 Subject: [PATCH 1/5] feat(grpc-harmony): wire input_image and input_file content parts in Responses router (R3) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Rich content parts landed in P1 (`ResponseContentPart::{InputImage, InputFile, Refusal}`). R1 wired them at the OpenAI-compat router via pure passthrough. R2 (parallel) wires them at the gRPC regular Responses router by reusing the Chat Completions multimodal pipeline. R3 (this change) wires them at the gRPC harmony Responses router. The harmony path is structurally different from both R1 and R2: gpt-oss and other harmony-target models have no multimodal wire to the backend. The chat harmony pipeline in `harmony/stages/request_building.rs` explicitly passes `None` for multimodal on every backend (sglang/vllm/trtllm). Reusing `grpc/multimodal.rs` at the Responses layer without threading bytes through `build_generate_request_from_responses` and the harmony prompt builder would produce a prompt that advertises an image the backend cannot actually see. Rather than inventing a new harmony-multimodal protocol (out of scope for a content-part transformer task), R3 takes the honest path: upgrade the pre-R3 silent drop of image/file parts in `harmony/builder.rs` into an explicit 400 `unsupported_content` at the router entry point. Silent data loss on multimodal prompts is the worst kind of failure mode — the caller believes the model saw their image, the model responds from text context alone, and the discrepancy is invisible at every observable layer. What changed - `model_gateway/src/routers/grpc/harmony/responses/content_parts.rs` (new): single source of truth for the rejection policy. Walks `ResponseInput::Items`, inspects `ResponseInputOutputItem::Message` and `SimpleInputMessage` bodies, and rejects with a 400 `unsupported_content` on: * `InputImage` with any combination of `file_id`, `image_url` (absolute or `data:` URL). Distinct messages identify the specific gap. * `InputFile` with any combination of `file_data`, `file_id`, `file_url`. * `InputFile` `file_data` whose base64-decoded prefix starts with `%PDF-` returns an R4-specific message ("PDF extraction pending R4") so callers can correlate the gap with the paused task. Robust against `data:application/pdf;base64,` wrappers inside the `file_data` field. * `Refusal` on any non-assistant role. Assistant-role refusals remain accepted so multi-turn replay of a prior model refusal keeps working. - `model_gateway/src/routers/grpc/harmony/responses/non_streaming.rs`: invoke `validate_harmony_responses_input` as the first action in `serve_harmony_responses`, before `load_previous_messages` merges persisted history — keeps stored assistant-refusal replays untouched while surfacing fresh-input issues up-front. - `model_gateway/src/routers/grpc/harmony/responses/streaming.rs`: mirror the validation in `serve_harmony_responses_stream` before history load and SSE channel setup so the client sees a plain HTTP 400 instead of an SSE stream that immediately closes. - `model_gateway/src/routers/grpc/harmony/responses/mod.rs`: declare the new `content_parts` module. - `model_gateway/src/routers/grpc/harmony/builder.rs`: refresh the two `parse_response_item_to_harmony_message` / `SimpleInputMessage` comments that still referenced the open R1/R2/R3 task. The `None` arms for `InputImage`/`InputFile` remain as a defensive fallback for any future caller that bypasses the validator — document that they are unreachable in practice. Why - Spec-parity with P1/R1/R2 on the harmony surface. - Eliminates a silent-drop failure mode that survives through the end of the request lifecycle. - Surfaces the pending R4 PDF gap with a specific error message so clients know the rejection is tracked rather than a generic unsupported-content case. How - Validation is a pure function operating on `&ResponsesRequest`; rejection returns `Result<(), Response>` so entry-point wiring is a one-line `?` or `if let Err(...)` pattern. - Magic-byte sniffing on `file_data` uses the base64 0.22 engine already on the workspace; no new dependency. Strips optional `data:;base64,` wrappers. - Refusal role gating: only `role == "assistant"` allows refusal content parts through, matching the P2 distinction between tagged Message (user/system/ developer) and assistant replay (P2 §Pass 2 Drift 2). PDF text extraction remains out of scope for R3 (tracked as R4, paused on `pdf-extract` dep approval). The R4 message is emitted here so callers correlate the gap correctly; the actual extractor is not added. Harmony-template integration note - Image threading: not wired. gpt-oss was trained on `<|message|>` text channels only; no image-token prefix or separate tensor field is read by harmony backends today. When/if multimodal harmony support lands (separate task), the validator here should be relaxed and a real `multimodal` parameter threaded through `harmony/stages/request_building.rs` → `build_generate_request_from_responses` on each backend. Tests - `content_parts::tests` covers the full taxonomy (14 tests): accepted text-only paths (pure-string, InputText, SimpleInputMessage.String, assistant-role Refusal replay) and every rejected branch (InputImage data-URL/absolute/file_id; InputFile file_data PDF/JPEG, file_url, file_id; non-assistant Refusal; SimpleInputMessage.Array image; data-URL-wrapped PDF file_data). - All 684 lib tests + other test binaries pass. - `cargo clippy -p smg --all-targets -- -D warnings` clean. Unblocks R5 (harmony row), E1 (harmony file-input row), and E2 (harmony image-input row) — each of which can now assert the expected 400 response on their harmony matrix cells. Refs: .claude/_audit/responses-api-gap-audit.md §Task R3 Signed-off-by: Simo Lin --- .../src/routers/grpc/harmony/builder.rs | 25 +- .../grpc/harmony/responses/content_parts.rs | 521 ++++++++++++++++++ .../src/routers/grpc/harmony/responses/mod.rs | 3 + .../grpc/harmony/responses/non_streaming.rs | 11 + .../grpc/harmony/responses/streaming.rs | 11 + 5 files changed, 564 insertions(+), 7 deletions(-) create mode 100644 model_gateway/src/routers/grpc/harmony/responses/content_parts.rs diff --git a/model_gateway/src/routers/grpc/harmony/builder.rs b/model_gateway/src/routers/grpc/harmony/builder.rs index 3efaacd303..b7d5b8c3c1 100644 --- a/model_gateway/src/routers/grpc/harmony/builder.rs +++ b/model_gateway/src/routers/grpc/harmony/builder.rs @@ -656,16 +656,23 @@ impl HarmonyBuilder { }; // Extract text from content parts. `Refusal` is losslessly - // representable as text and is preserved verbatim. Image / - // file parts are currently dropped (R1/R2/R3 will implement - // full media handling). + // representable as text and is preserved verbatim (R3 only + // allows refusals on assistant-role messages; other roles + // are rejected at the router entry via + // `responses::content_parts::validate_harmony_responses_input`). + // Image / file parts are rejected by that same validator + // before reaching this builder — the `None` arms below are + // a defensive fallback so a future caller that skips the + // validator does not silently corrupt the prompt. let text_parts: Vec = content .iter() .filter_map(|part| match part { ResponseContentPart::OutputText { text, .. } => Some(text.clone()), ResponseContentPart::InputText { text } => Some(text.clone()), ResponseContentPart::Refusal { refusal } => Some(refusal.clone()), - // R1/R2/R3 will implement full media handling + // Unreachable in practice: the Responses router + // validator rejects these before dispatch. Kept + // as a defensive fallback. ResponseContentPart::InputImage { .. } | ResponseContentPart::InputFile { .. } => None, }) @@ -815,15 +822,19 @@ impl HarmonyBuilder { StringOrContentParts::Array(parts) => { // Extract text from content parts. `Refusal` is // losslessly representable as text and is preserved - // verbatim. Image / file parts are currently dropped - // (R1/R2/R3 will implement full media handling). + // verbatim. Image / file parts are rejected at the + // router entry by + // `responses::content_parts::validate_harmony_responses_input`; + // the `None` arms below are a defensive fallback + // so a future caller that skips the validator + // cannot silently corrupt the prompt. parts .iter() .filter_map(|part| match part { ResponseContentPart::OutputText { text, .. } => Some(text.clone()), ResponseContentPart::InputText { text } => Some(text.clone()), ResponseContentPart::Refusal { refusal } => Some(refusal.clone()), - // R1/R2/R3 will implement full media handling + // Unreachable in practice; see note above. ResponseContentPart::InputImage { .. } | ResponseContentPart::InputFile { .. } => None, }) diff --git a/model_gateway/src/routers/grpc/harmony/responses/content_parts.rs b/model_gateway/src/routers/grpc/harmony/responses/content_parts.rs new file mode 100644 index 0000000000..df35cdcd74 --- /dev/null +++ b/model_gateway/src/routers/grpc/harmony/responses/content_parts.rs @@ -0,0 +1,521 @@ +//! Harmony Responses content-part validation (R3). +//! +//! The harmony Responses pipeline builds gpt-oss prompt tokens directly via +//! [`openai_harmony`]; unlike the regular gRPC Responses path, it never +//! converts to Chat Completions, so the shared multimodal wire in +//! [`crate::routers::grpc::multimodal`] is *not* reachable from this router. +//! The Chat harmony path in `harmony/stages/request_building.rs` explicitly +//! passes `None` for multimodal on every backend, which matches the fact +//! that gpt-oss was not trained against image/file tokens at the +//! `<|message|>` level. +//! +//! Prior to R3 the harmony builder silently dropped `InputImage`, +//! `InputFile`, and (indirectly) `Refusal` content parts when rendering the +//! harmony prompt (see the `None` arms at `builder.rs` circa the pre-R3 +//! `parse_response_item_to_harmony_message` / `SimpleInputMessage` paths). +//! Silent data loss on multimodal prompts is a much worse failure mode +//! than a clear 400: the caller believes the model saw their image, the +//! model responds from text context alone, and the discrepancy is +//! invisible at every observable layer. +//! +//! R3 upgrades that silent drop into an explicit `400 unsupported_content` +//! at the router entry point so callers get the same P1 parity R1/R2 give +//! on their respective surfaces. This module is the single source of +//! truth for the rejection policy — both the non-streaming and streaming +//! entry points in this directory call [`validate_harmony_responses_input`] +//! before any pipeline dispatch. +//! +//! Scope of rejection (per R3 task spec): +//! +//! - `InputImage` on a user/system/developer/simple message → 400 +//! `unsupported_content`. Applies regardless of whether the payload +//! references `file_id`, `image_url` (absolute or `data:` URL). Harmony +//! cannot forward the image to a backend capable of consuming it. +//! - `InputFile` on any fresh input message → 400 `unsupported_content`. +//! The error code specialises on `file_data` payloads whose magic bytes +//! match `%PDF-`: those return the PDF-specific "pending R4" message so +//! the caller understands it is a known gap, not a generic rejection. +//! - `Refusal` on a *non-assistant* role → 400 `unsupported_content`. On +//! `assistant` roles the part is a legitimate replay of a prior model +//! refusal (the P2-refined assistant-message variant) and is left alone +//! for the builder to render as text — rejecting it there would break +//! multi-turn conversations that faithfully echo the prior assistant +//! output. +//! +//! Text-only parts (`InputText`, `OutputText`) and assistant-role +//! `Refusal` replays pass through untouched; the builder already renders +//! them correctly. PDF text extraction itself remains Task R4 and is +//! explicitly out of scope here. + +use axum::response::Response; +use base64::{engine::general_purpose::STANDARD as BASE64_STANDARD, Engine as _}; +use openai_protocol::responses::{ + ResponseContentPart, ResponseInput, ResponseInputOutputItem, ResponsesRequest, + StringOrContentParts, +}; + +use crate::routers::error; + +/// Magic-byte prefix for PDF files (`%PDF-`). PDFs may include a small +/// leading BOM or whitespace but the spec requires the `%PDF-` token +/// within the first 1024 bytes; we only sniff the decoded prefix of the +/// content-part payload here, which is sufficient to catch the common +/// case where a PDF is base64-encoded without additional wrapping. +const PDF_MAGIC: &[u8] = b"%PDF-"; + +/// Validate that the user-supplied input in a ResponsesRequest contains +/// only content parts the harmony Responses router can faithfully render. +/// +/// Returns `Ok(())` when the request is safe to dispatch, or a +/// `400 unsupported_content` response otherwise. The error message is +/// deliberately descriptive so callers can distinguish the rejection +/// reason (image, file, PDF, refusal, file_id) at the HTTP layer. +/// +/// Only the caller-submitted `request.input` is inspected. Persisted +/// history loaded via `previous_response_id` is skipped: it was +/// validated when it was first submitted, may legitimately carry +/// assistant-refusal replays, and is not something the caller can +/// mutate on this request. +#[expect( + clippy::result_large_err, + reason = "axum Response is the standard error type on the router-entry boundary; \ + matches the rest of the harmony Responses entry points" +)] +pub(super) fn validate_harmony_responses_input( + request: &ResponsesRequest, +) -> Result<(), Response> { + let items = match &request.input { + ResponseInput::Text(_) => return Ok(()), + ResponseInput::Items(items) => items, + }; + + for item in items { + match item { + ResponseInputOutputItem::Message { role, content, .. } => { + validate_content_parts(role.as_str(), content)?; + } + ResponseInputOutputItem::SimpleInputMessage { + role, + content: StringOrContentParts::Array(parts), + .. + } => { + validate_content_parts(role.as_str(), parts)?; + } + // The `StringOrContentParts::String` variant has no + // content parts to inspect; and non-message items + // (function calls, reasoning, tool outputs, etc.) never + // carry `ResponseContentPart` today so there is nothing + // to reject here. The harmony builder handles unsupported + // item variants with its own `Unsupported input item type` + // errors at the prep stage. + _ => {} + } + } + + Ok(()) +} + +/// Apply the rejection policy to a single message's content-parts list. +#[expect( + clippy::result_large_err, + reason = "axum Response is the standard error type on the router-entry boundary; \ + matches the rest of the harmony Responses entry points" +)] +fn validate_content_parts(role: &str, parts: &[ResponseContentPart]) -> Result<(), Response> { + for part in parts { + match part { + ResponseContentPart::InputText { .. } | ResponseContentPart::OutputText { .. } => { + // Text parts — the builder already renders these. + } + ResponseContentPart::InputImage { + image_url, file_id, .. + } => { + // file_id is always unsupported (no Files API backend in + // harmony gRPC mode). image_url (absolute or data URL) is + // also unsupported because the harmony pipeline does not + // thread multimodal bytes to the backend — the chat + // harmony path itself passes `None` for multimodal. See + // `harmony/stages/request_building.rs` for the + // corresponding dispatch code. + let detail = if file_id.is_some() { + "input_image content parts with file_id are not supported on the harmony Responses \ + backend (no Files API resolver in gRPC mode)" + } else if let Some(url) = image_url { + if is_data_url(url) { + "input_image content parts with a data: URL are not supported on the harmony \ + Responses backend (the gpt-oss pipeline has no multimodal wire)" + } else { + "input_image content parts with absolute image_url are not supported on the \ + harmony Responses backend (the gpt-oss pipeline has no multimodal wire)" + } + } else { + "input_image content parts are not supported on the harmony Responses backend \ + (the gpt-oss pipeline has no multimodal wire)" + }; + return Err(unsupported(detail)); + } + ResponseContentPart::InputFile { + file_data, + file_id, + file_url, + .. + } => { + // `file_data` PDF payloads get the R4-specific message + // so callers can correlate a known gap instead of a + // blanket rejection. We only sniff magic bytes on the + // decoded prefix (spec-shaped `data:application/pdf;...` + // prefixes may or may not be present — the magic bytes + // are authoritative). + if let Some(data) = file_data.as_deref() { + if looks_like_pdf(data) { + return Err(unsupported( + "input_file content parts carrying PDF data (magic bytes `%PDF-`) are \ + not yet supported on the harmony Responses backend. PDF text \ + extraction is pending R4.", + )); + } + return Err(unsupported( + "input_file content parts with file_data are not supported on the harmony \ + Responses backend", + )); + } + if file_id.is_some() { + return Err(unsupported( + "input_file content parts with file_id are not supported on the harmony \ + Responses backend (no Files API resolver in gRPC mode)", + )); + } + if file_url.is_some() { + return Err(unsupported( + "input_file content parts with file_url are not supported on the harmony \ + Responses backend", + )); + } + return Err(unsupported( + "input_file content parts are not supported on the harmony Responses backend", + )); + } + ResponseContentPart::Refusal { .. } => { + // Only assistant-role replays are allowed — those + // legitimately echo a prior model refusal in multi-turn + // history. Any other role carrying a refusal is a new + // input that the harmony backend cannot consume. + if role != "assistant" { + return Err(unsupported( + "refusal content parts are only allowed on assistant-role messages \ + (e.g. replaying a prior response); other roles cannot carry refusals \ + on the harmony Responses backend", + )); + } + } + } + } + + Ok(()) +} + +/// Detect a `data:` URL prefix. Matches `data:` case-insensitively to +/// mirror RFC 2397 permissiveness without pulling in a dedicated parser. +fn is_data_url(url: &str) -> bool { + url.len() >= 5 && url.as_bytes()[..5].eq_ignore_ascii_case(b"data:") +} + +/// Return `true` if the decoded prefix of `file_data` contains the PDF +/// magic bytes `%PDF-`. Accepts either a raw base64 payload or a +/// `data:;base64,` wrapper for robustness against +/// clients that send the latter in a `file_data` field. +fn looks_like_pdf(file_data: &str) -> bool { + let payload = strip_data_url_prefix(file_data); + let Ok(decoded) = BASE64_STANDARD.decode(payload.trim()) else { + // If base64 fails we fall through to the generic + // "file_data unsupported" message rather than a PDF-specific + // one; malformed base64 is a separate kind of error and the + // harmony backend can't handle it anyway. + return false; + }; + decoded.starts_with(PDF_MAGIC) +} + +/// Strip a `data:;base64,` prefix from `input` if present, returning +/// the remainder; otherwise return the input unchanged. +fn strip_data_url_prefix(input: &str) -> &str { + if !is_data_url(input) { + return input; + } + match input.find(";base64,") { + Some(idx) => &input[idx + ";base64,".len()..], + // `data:,` (non-base64) is not something we can + // meaningfully magic-byte sniff; return the full input so the + // caller falls through to the generic file-unsupported path. + None => input, + } +} + +/// Build the standard `400 unsupported_content` response used by every +/// rejection branch in this module. +fn unsupported(message: &str) -> Response { + error::bad_request("unsupported_content", message.to_string()) +} + +#[cfg(test)] +mod tests { + //! Unit coverage for R3. These tests lock in the rejection policy and + //! assert every branch of the content-part taxonomy the harmony + //! backend cannot execute — so the silent-drop regression that R3 + //! fixed cannot quietly come back. + + use axum::http::StatusCode; + use openai_protocol::{ + common::Detail, + responses::{ + FileDetail, ResponseContentPart, ResponseInput, ResponseInputOutputItem, + ResponsesRequest, StringOrContentParts, + }, + }; + + use super::*; + use crate::routers::error::HEADER_X_SMG_ERROR_CODE; + + fn request_with_items(items: Vec) -> ResponsesRequest { + ResponsesRequest { + model: "gpt-oss".to_string(), + input: ResponseInput::Items(items), + ..Default::default() + } + } + + fn user_message(content: Vec) -> ResponseInputOutputItem { + ResponseInputOutputItem::Message { + id: "msg_test".to_string(), + role: "user".to_string(), + content, + status: Some("completed".to_string()), + phase: None, + } + } + + fn assistant_message(content: Vec) -> ResponseInputOutputItem { + ResponseInputOutputItem::Message { + id: "msg_test_asst".to_string(), + role: "assistant".to_string(), + content, + status: Some("completed".to_string()), + phase: None, + } + } + + fn assert_is_unsupported(response: Response) { + assert_eq!(response.status(), StatusCode::BAD_REQUEST); + let header = response + .headers() + .get(HEADER_X_SMG_ERROR_CODE) + .expect("error code header is always set"); + assert_eq!(header.to_str().unwrap(), "unsupported_content"); + } + + #[test] + fn text_only_input_is_accepted() { + // Baseline: `ResponseInput::Text` never carries non-text parts + // so it must always validate. Guards against accidental + // overreach in the validator. + let request = ResponsesRequest { + model: "gpt-oss".to_string(), + input: ResponseInput::Text("hello".to_string()), + ..Default::default() + }; + validate_harmony_responses_input(&request).expect("text-only request must pass"); + } + + #[test] + fn pure_input_text_items_are_accepted() { + let request = request_with_items(vec![user_message(vec![ResponseContentPart::InputText { + text: "Describe the weather.".to_string(), + }])]); + validate_harmony_responses_input(&request).expect("InputText must pass"); + } + + #[test] + fn input_image_data_url_is_rejected() { + // R3 scope item: InputImage with a `data:` URL. The pre-R3 + // builder dropped this silently; R3 mandates a 400. + let request = request_with_items(vec![user_message(vec![ResponseContentPart::InputImage { + detail: Some(Detail::Auto), + file_id: None, + image_url: Some("data:image/jpeg;base64,/9j/4AAQSkZJRg==".to_string()), + }])]); + let err = + validate_harmony_responses_input(&request).expect_err("data-URL images must be rejected"); + assert_is_unsupported(err); + } + + #[test] + fn input_image_absolute_url_is_rejected() { + let request = request_with_items(vec![user_message(vec![ResponseContentPart::InputImage { + detail: None, + file_id: None, + image_url: Some("https://example.com/dog.jpg".to_string()), + }])]); + let err = validate_harmony_responses_input(&request) + .expect_err("absolute-URL images must be rejected"); + assert_is_unsupported(err); + } + + #[test] + fn input_image_file_id_is_rejected() { + // R3 scope item: file_id is always unsupported in gRPC mode + // (no Files API resolver). + let request = request_with_items(vec![user_message(vec![ResponseContentPart::InputImage { + detail: None, + file_id: Some("file-abc".to_string()), + image_url: None, + }])]); + let err = validate_harmony_responses_input(&request) + .expect_err("file_id images must be rejected"); + assert_is_unsupported(err); + } + + #[test] + fn input_file_pdf_magic_bytes_are_rejected_with_r4_message() { + // `%PDF-1.4\n` base64-encodes to `JVBERi0xLjQK`. Asserts the + // PDF-specific error path so callers know the gap is tracked + // under R4 rather than a blanket rejection. + let request = request_with_items(vec![user_message(vec![ResponseContentPart::InputFile { + detail: Some(FileDetail::High), + file_data: Some("JVBERi0xLjQK".to_string()), + file_id: None, + file_url: None, + filename: Some("report.pdf".to_string()), + }])]); + let err = + validate_harmony_responses_input(&request).expect_err("PDF file_data must be rejected"); + assert_is_unsupported(err); + } + + #[test] + fn input_file_non_pdf_file_data_is_rejected() { + // A JPEG magic-byte prefix (FFD8FFE0 ... base64 `/9j/4A==`) is + // not a PDF; it must still be rejected because the harmony + // pipeline cannot route it — but *not* with the R4 message. + let request = request_with_items(vec![user_message(vec![ResponseContentPart::InputFile { + detail: None, + file_data: Some("/9j/4AAQSkZJRg==".to_string()), + file_id: None, + file_url: None, + filename: Some("photo.jpg".to_string()), + }])]); + let err = validate_harmony_responses_input(&request) + .expect_err("JPEG file_data must be rejected"); + assert_is_unsupported(err); + } + + #[test] + fn input_file_file_url_is_rejected() { + let request = request_with_items(vec![user_message(vec![ResponseContentPart::InputFile { + detail: None, + file_data: None, + file_id: None, + file_url: Some("https://example.com/report.pdf".to_string()), + filename: None, + }])]); + let err = validate_harmony_responses_input(&request) + .expect_err("file_url attachments must be rejected"); + assert_is_unsupported(err); + } + + #[test] + fn input_file_file_id_is_rejected() { + let request = request_with_items(vec![user_message(vec![ResponseContentPart::InputFile { + detail: None, + file_data: None, + file_id: Some("file-abc".to_string()), + file_url: None, + filename: None, + }])]); + let err = validate_harmony_responses_input(&request) + .expect_err("file_id attachments must be rejected"); + assert_is_unsupported(err); + } + + #[test] + fn refusal_on_user_role_is_rejected() { + // Refusals on user/system/developer messages are nonsensical + // inputs for the harmony pipeline. R3 rejects them explicitly + // instead of letting the builder silently fold the text into + // the prompt. + let request = request_with_items(vec![user_message(vec![ResponseContentPart::Refusal { + refusal: "I cannot process that request.".to_string(), + }])]); + let err = validate_harmony_responses_input(&request) + .expect_err("user-role refusals must be rejected"); + assert_is_unsupported(err); + } + + #[test] + fn refusal_on_assistant_role_is_accepted() { + // Assistant-role replays legitimately carry a prior refusal + // when the client rehydrates a multi-turn conversation. The + // builder already renders them correctly as text, so R3 must + // not regress this. + let request = request_with_items(vec![assistant_message(vec![ + ResponseContentPart::Refusal { + refusal: "I cannot process that request.".to_string(), + }, + ])]); + validate_harmony_responses_input(&request) + .expect("assistant-role refusals must still be accepted for multi-turn replay"); + } + + #[test] + fn simple_input_message_with_content_parts_is_validated() { + // SimpleInputMessage uses the `StringOrContentParts::Array` + // shape; the validator must descend into it symmetrically with + // the `Message` case. Regression guard: before R3 the builder + // silently dropped images on both variants. + let request = request_with_items(vec![ResponseInputOutputItem::SimpleInputMessage { + content: StringOrContentParts::Array(vec![ResponseContentPart::InputImage { + detail: None, + file_id: None, + image_url: Some("https://example.com/cat.jpg".to_string()), + }]), + role: "user".to_string(), + r#type: None, + phase: None, + }]); + let err = validate_harmony_responses_input(&request) + .expect_err("SimpleInputMessage.Array images must be rejected"); + assert_is_unsupported(err); + } + + #[test] + fn simple_input_message_string_variant_is_accepted() { + // Sibling of the Array case above; must not regress the bare + // `StringOrContentParts::String` path. + let request = request_with_items(vec![ResponseInputOutputItem::SimpleInputMessage { + content: StringOrContentParts::String("Describe the weather.".to_string()), + role: "user".to_string(), + r#type: None, + phase: None, + }]); + validate_harmony_responses_input(&request) + .expect("SimpleInputMessage.String must pass unchanged"); + } + + #[test] + fn data_url_prefix_pdf_magic_still_rejected_with_r4_message() { + // Robustness: if a client wraps the base64 payload in a + // `data:application/pdf;base64,` envelope inside `file_data` + // (non-canonical but observed in the wild), the magic-byte + // sniff must still catch it so the caller sees the R4-specific + // message rather than a generic rejection. + let request = request_with_items(vec![user_message(vec![ResponseContentPart::InputFile { + detail: None, + file_data: Some("data:application/pdf;base64,JVBERi0xLjQK".to_string()), + file_id: None, + file_url: None, + filename: None, + }])]); + let err = validate_harmony_responses_input(&request) + .expect_err("data-URL-wrapped PDF file_data must be rejected"); + assert_is_unsupported(err); + } +} diff --git a/model_gateway/src/routers/grpc/harmony/responses/mod.rs b/model_gateway/src/routers/grpc/harmony/responses/mod.rs index fa537ffcdb..8d173f0b7c 100644 --- a/model_gateway/src/routers/grpc/harmony/responses/mod.rs +++ b/model_gateway/src/routers/grpc/harmony/responses/mod.rs @@ -17,8 +17,11 @@ //! - `streaming` - Streaming entry point and tool loop //! - `execution` - MCP tool execution logic //! - `common` - Shared helpers and state tracking +//! - `content_parts` - R3 input content-part validation (reject +//! image/file/refusal parts the harmony backend cannot render) pub(crate) mod common; +pub(crate) mod content_parts; pub(crate) mod execution; pub(crate) mod non_streaming; pub(crate) mod streaming; diff --git a/model_gateway/src/routers/grpc/harmony/responses/non_streaming.rs b/model_gateway/src/routers/grpc/harmony/responses/non_streaming.rs index aa240a6c94..bda63f03f2 100644 --- a/model_gateway/src/routers/grpc/harmony/responses/non_streaming.rs +++ b/model_gateway/src/routers/grpc/harmony/responses/non_streaming.rs @@ -23,6 +23,7 @@ use super::{ build_next_request_with_tools, inject_mcp_metadata, load_previous_messages, strip_image_generation_from_request_tools, McpCallTracking, }, + content_parts::validate_harmony_responses_input, execution::{convert_mcp_tools_to_response_tools, execute_mcp_tools, ToolResult}, }; use crate::{ @@ -56,6 +57,16 @@ pub(crate) async fn serve_harmony_responses( request: ResponsesRequest, tenant_request_meta: TenantRequestMeta, ) -> Result { + // R3: reject image/file/refusal content parts up-front. The harmony + // pipeline renders prompts via `openai_harmony` and has no + // multimodal wire to the backend (chat harmony sends `None` for + // multimodal in request_building), so silently dropping these + // parts would lose data invisibly. Validate the caller's original + // input *before* we merge persisted history via + // `load_previous_messages` so stored assistant-role refusals keep + // replaying cleanly. + validate_harmony_responses_input(&request)?; + // Clone request for persistence let original_request = request.clone(); diff --git a/model_gateway/src/routers/grpc/harmony/responses/streaming.rs b/model_gateway/src/routers/grpc/harmony/responses/streaming.rs index 3f7c209869..3b75fc4f00 100644 --- a/model_gateway/src/routers/grpc/harmony/responses/streaming.rs +++ b/model_gateway/src/routers/grpc/harmony/responses/streaming.rs @@ -16,6 +16,7 @@ use super::{ build_next_request_with_tools, load_previous_messages, strip_image_generation_from_request_tools, McpCallTracking, }, + content_parts::validate_harmony_responses_input, execution::{convert_mcp_tools_to_response_tools, execute_mcp_tools}, }; use crate::{ @@ -46,6 +47,16 @@ pub(crate) async fn serve_harmony_responses_stream( request: ResponsesRequest, tenant_request_meta: TenantRequestMeta, ) -> Response { + // R3: reject image/file/refusal content parts up-front. Same + // rationale as the non-streaming entry point — harmony has no + // multimodal wire and we must not silently drop those parts. + // Rejection happens *before* history load and SSE-channel setup so + // callers receive a plain HTTP 400 instead of an SSE stream that + // immediately closes. + if let Err(err_response) = validate_harmony_responses_input(&request) { + return err_response; + } + // Load previous conversation history if previous_response_id is set let current_request = match load_previous_messages(ctx, request.clone()).await { Ok(req) => req, From 1d50a1d2f60ad69472df0585009ea1846eaa5337 Mon Sep 17 00:00:00 2001 From: Simo Lin Date: Fri, 24 Apr 2026 13:27:26 -0700 Subject: [PATCH 2/5] fix(grpc-harmony): bound PDF sniff allocation and fix base64 separator case MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address three R3 review nits (gemini-code-assist, claude, chatgpt-codex): 1. **DOS-adjacent allocation on large `file_data` rejections** (codex P2, gemini medium): `looks_like_pdf` previously decoded the *entire* `file_data` base64 payload into a `Vec` just to check its first 5 bytes. With the router's default `max_payload_size` of 512 MB (model_gateway/src/config/types.rs), a single rejected `input_file.file_data` request could trigger hundreds of MB of allocation on the request path — turning R3's 400-fast-path into a resource exhaustion vector under load. Rewrite `looks_like_pdf` to decode only the first 8 base64 characters (6 bytes) into a fixed-size stack buffer via `BASE64_STANDARD.decode_slice`. That window is the smallest multiple-of-4 that still covers the 5-byte `%PDF-` magic. Full payload is never materialized. 2. **Case-insensitive `;base64,` separator** (claude nit): `is_data_url` matches `data:` case-insensitively per RFC 2397, but the old `strip_data_url_prefix` used a case-sensitive `find(";base64,")`. A client sending `data:application/pdf;Base64,JVBERi...` would pass the data-URL gate and then miss the strip, falling through to the generic "file_data unsupported" branch instead of the PDF-specific R4 message. The request was still correctly rejected, but the error code was less actionable. Extract `find_base64_separator` using a no-allocation byte-window walk with `eq_ignore_ascii_case`, keeping the strip path case-insensitive end-to-end. 3. Documented both helpers with inline rationale + cross-references to the config default and RFC. Added two new tests: - `data_url_prefix_is_case_insensitive_on_base64_separator` locks in the R4-message outcome on mixed-case wrappers. - `pdf_sniff_decodes_only_magic_prefix_not_full_payload` guards the bounded-decode path with a 64 KiB+ payload whose first 6 bytes decode to `%PDF-1` and whose remainder is filler the sniffer must never touch. No behavior change for valid content-part inputs; only tightens the reject path and improves error-message specificity. All 16 content_parts tests pass. `cargo clippy -p smg --all-targets -- -D warnings` clean. Refs: .claude/_audit/responses-api-gap-audit.md §Task R3 Signed-off-by: Simo Lin --- .../grpc/harmony/responses/content_parts.rs | 102 ++++++++++++++++-- 1 file changed, 93 insertions(+), 9 deletions(-) diff --git a/model_gateway/src/routers/grpc/harmony/responses/content_parts.rs b/model_gateway/src/routers/grpc/harmony/responses/content_parts.rs index df35cdcd74..e003c50883 100644 --- a/model_gateway/src/routers/grpc/harmony/responses/content_parts.rs +++ b/model_gateway/src/routers/grpc/harmony/responses/content_parts.rs @@ -220,29 +220,57 @@ fn is_data_url(url: &str) -> bool { url.len() >= 5 && url.as_bytes()[..5].eq_ignore_ascii_case(b"data:") } +/// Number of base64 characters that decode to at least [`PDF_MAGIC`]'s +/// length (5 bytes). base64 encodes 3 bytes per 4 chars, so 8 chars +/// decode to 6 bytes — the smallest multiple-of-4 window that still +/// covers the magic-byte prefix. Sniffing only this window avoids the +/// DOS-adjacent case where a 500 MB rejected `file_data` would otherwise +/// be fully base64-decoded just to check its leading bytes (see +/// `max_payload_size` default in `model_gateway/src/config/types.rs`). +const PDF_SNIFF_BASE64_LEN: usize = 8; +/// Decoded buffer size needed for [`PDF_SNIFF_BASE64_LEN`] base64 +/// characters. base64 decodes 4 input chars to 3 output bytes. +const PDF_SNIFF_DECODED_LEN: usize = PDF_SNIFF_BASE64_LEN / 4 * 3; + /// Return `true` if the decoded prefix of `file_data` contains the PDF /// magic bytes `%PDF-`. Accepts either a raw base64 payload or a /// `data:;base64,` wrapper for robustness against /// clients that send the latter in a `file_data` field. +/// +/// Only decodes [`PDF_SNIFF_BASE64_LEN`] base64 chars into a fixed-size +/// stack buffer; the full payload is never materialized. This matters +/// because the router's default `max_payload_size` permits multi-hundred +/// megabyte `file_data` inputs that would otherwise be fully decoded +/// just to be rejected. fn looks_like_pdf(file_data: &str) -> bool { - let payload = strip_data_url_prefix(file_data); - let Ok(decoded) = BASE64_STANDARD.decode(payload.trim()) else { - // If base64 fails we fall through to the generic - // "file_data unsupported" message rather than a PDF-specific - // one; malformed base64 is a separate kind of error and the - // harmony backend can't handle it anyway. + let payload = strip_data_url_prefix(file_data).trim_start(); + let Some(prefix) = payload.get(..PDF_SNIFF_BASE64_LEN) else { + // Input shorter than the sniff window — safe to fall through + // to the generic "file_data unsupported" message. An 8-byte + // `file_data` payload can't be a real PDF anyway. + return false; + }; + let mut decoded = [0u8; PDF_SNIFF_DECODED_LEN]; + let Ok(len) = BASE64_STANDARD.decode_slice(prefix.as_bytes(), &mut decoded) else { + // Malformed base64 is a separate rejection kind; fall through + // so callers see the generic "file_data unsupported" message. return false; }; - decoded.starts_with(PDF_MAGIC) + decoded[..len].starts_with(PDF_MAGIC) } /// Strip a `data:;base64,` prefix from `input` if present, returning -/// the remainder; otherwise return the input unchanged. +/// the remainder; otherwise return the input unchanged. Case-insensitive +/// on the `;base64,` separator to stay consistent with [`is_data_url`], +/// which is case-insensitive on the `data:` scheme per RFC 2397 §3; without +/// this, a payload like `data:application/pdf;Base64,JVBERi...` would pass +/// the data-URL gate but miss the strip, falling through to the generic +/// "file_data unsupported" message instead of the PDF-specific R4 message. fn strip_data_url_prefix(input: &str) -> &str { if !is_data_url(input) { return input; } - match input.find(";base64,") { + match find_base64_separator(input) { Some(idx) => &input[idx + ";base64,".len()..], // `data:,` (non-base64) is not something we can // meaningfully magic-byte sniff; return the full input so the @@ -251,6 +279,17 @@ fn strip_data_url_prefix(input: &str) -> &str { } } +/// Locate the `;base64,` separator inside a data URL, matching +/// case-insensitively. Uses a byte-window walk (no allocation) so the +/// hot path stays allocation-free even on large payloads. +fn find_base64_separator(input: &str) -> Option { + const NEEDLE: &[u8] = b";base64,"; + input + .as_bytes() + .windows(NEEDLE.len()) + .position(|window| window.eq_ignore_ascii_case(NEEDLE)) +} + /// Build the standard `400 unsupported_content` response used by every /// rejection branch in this module. fn unsupported(message: &str) -> Response { @@ -518,4 +557,49 @@ mod tests { .expect_err("data-URL-wrapped PDF file_data must be rejected"); assert_is_unsupported(err); } + + #[test] + fn data_url_prefix_is_case_insensitive_on_base64_separator() { + // `is_data_url` matches `data:` case-insensitively, so + // `strip_data_url_prefix` must match `;base64,` with the same + // flexibility. Before the R3 follow-up both `Data:` and + // `;Base64,` would skip the strip, mis-routing the request into + // the generic "file_data unsupported" path instead of the + // PDF-specific R4 message. + let request = request_with_items(vec![user_message(vec![ResponseContentPart::InputFile { + detail: None, + file_data: Some("Data:application/pdf;Base64,JVBERi0xLjQK".to_string()), + file_id: None, + file_url: None, + filename: None, + }])]); + let err = validate_harmony_responses_input(&request) + .expect_err("mixed-case data-URL wrapper must still reach the PDF sniffer"); + assert_is_unsupported(err); + } + + #[test] + fn pdf_sniff_decodes_only_magic_prefix_not_full_payload() { + // Regression guard for the bounded-decode path: a large PDF + // `file_data` must still route to the R4-specific message + // without the validator materializing the entire payload. We + // construct a payload whose first 6 bytes base64-decode to + // `%PDF-1` and pad the remainder with a long run of `A`s; the + // prefix-only decoder reaches the magic bytes before looking + // at anything beyond the first 8 base64 chars. + // + // `%PDF-1` → base64 `JVBERi0x` (the canonical PDF header). + let mut payload = String::from("JVBERi0x"); + payload.push_str(&"A".repeat(65_536)); + let request = request_with_items(vec![user_message(vec![ResponseContentPart::InputFile { + detail: None, + file_data: Some(payload), + file_id: None, + file_url: None, + filename: None, + }])]); + let err = validate_harmony_responses_input(&request) + .expect_err("large PDF file_data must still be rejected with the R4 message"); + assert_is_unsupported(err); + } } From 2599f45d081117ae63c95bba956f30ec059bb446 Mon Sep 17 00:00:00 2001 From: Simo Lin Date: Fri, 24 Apr 2026 13:41:15 -0700 Subject: [PATCH 3/5] fix(grpc-harmony): re-validate merged history and assert PDF-specific message body MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address two CodeRabbit review findings: 1. **Major — Revalidate merged history** (coderabbitai). The R3 validator only inspected `request.input`, so stored threads that were persisted by a sibling router supporting multimodal (notably R1's OpenAI-compat passthrough) could replay with `InputImage` / `InputFile` content parts on the harmony surface: they would pass the fresh-input check, get merged by `load_previous_messages`, and hit the exact silent-drop path this PR exists to close. Re-run `validate_harmony_responses_input` on the merged `current_request` in both the non-streaming and streaming entry points, after the history merge. The validator keeps its assistant-role-refusal exception so legitimate multi-turn output replay continues to work; only non-assistant image/file/refusal parts in history fail the second pass. 2. **Nit — assert PDF-specific message body** (coderabbitai). The PDF-path tests only asserted `400 unsupported_content`, which is satisfied by both PDF-specific and generic file-data rejections. That makes the R4-message branch untested at the message level. Add an async `error_message` test helper that reads the JSON body and extracts `error.message`, then upgrade four tests to assert on the body: - `input_file_pdf_magic_bytes_are_rejected_with_r4_message` — message contains `PDF` and `R4`. - `input_file_non_pdf_file_data_is_rejected` — message does NOT contain `R4` (proves the non-PDF branch stays out of the R4 path). - `data_url_prefix_pdf_magic_still_rejected_with_r4_message` — message contains `PDF` + `R4` after data-URL stripping. - `data_url_prefix_is_case_insensitive_on_base64_separator` — message contains `PDF` + `R4` after case-insensitive stripping. - `pdf_sniff_decodes_only_magic_prefix_not_full_payload` — large payload still reaches the R4-specific message via bounded decoding. Also add `refusal_role_rejection_cites_assistant_exception` to lock in the human-readable rationale the caller sees for refusal rejections. No behavior change for valid content-part inputs; only tightens the reject- path coverage and closes the stored-history regression. All 17 content_parts tests pass. `cargo clippy -p smg --all-targets -- -D warnings` clean. Refs: .claude/_audit/responses-api-gap-audit.md §Task R3 Signed-off-by: Simo Lin --- .../grpc/harmony/responses/content_parts.rs | 108 +++++++++++++++--- .../grpc/harmony/responses/non_streaming.rs | 14 ++- .../grpc/harmony/responses/streaming.rs | 11 ++ 3 files changed, 114 insertions(+), 19 deletions(-) diff --git a/model_gateway/src/routers/grpc/harmony/responses/content_parts.rs b/model_gateway/src/routers/grpc/harmony/responses/content_parts.rs index e003c50883..ae14c5f438 100644 --- a/model_gateway/src/routers/grpc/harmony/responses/content_parts.rs +++ b/model_gateway/src/routers/grpc/harmony/responses/content_parts.rs @@ -303,7 +303,7 @@ mod tests { //! backend cannot execute — so the silent-drop regression that R3 //! fixed cannot quietly come back. - use axum::http::StatusCode; + use axum::{body::to_bytes, http::StatusCode}; use openai_protocol::{ common::Detail, responses::{ @@ -311,10 +311,32 @@ mod tests { ResponsesRequest, StringOrContentParts, }, }; + use serde_json::Value; use super::*; use crate::routers::error::HEADER_X_SMG_ERROR_CODE; + /// Extract the JSON error message body from a validation error + /// response. Tests use this instead of only checking status + + /// `X-SMG-Error-Code` so that PDF-specific and generic rejections + /// are distinguishable at the message-body level — otherwise both + /// shapes satisfy the same status/code assertion and the + /// R4-specific path would be untested. + async fn error_message(response: Response) -> String { + let (_parts, body) = response.into_parts(); + let bytes = to_bytes(body, usize::MAX) + .await + .expect("error body fits in memory"); + let value: Value = + serde_json::from_slice(&bytes).expect("error body is always valid JSON"); + value + .get("error") + .and_then(|e| e.get("message")) + .and_then(|m| m.as_str()) + .map(str::to_owned) + .expect("error.message is always present on a bad_request response") + } + fn request_with_items(items: Vec) -> ResponsesRequest { ResponsesRequest { model: "gpt-oss".to_string(), @@ -413,11 +435,13 @@ mod tests { assert_is_unsupported(err); } - #[test] - fn input_file_pdf_magic_bytes_are_rejected_with_r4_message() { + #[tokio::test] + async fn input_file_pdf_magic_bytes_are_rejected_with_r4_message() { // `%PDF-1.4\n` base64-encodes to `JVBERi0xLjQK`. Asserts the // PDF-specific error path so callers know the gap is tracked - // under R4 rather than a blanket rejection. + // under R4 rather than a blanket rejection. We assert on the + // message body (not just status/code) so the PDF-specific + // branch is distinguishable from a generic file-data rejection. let request = request_with_items(vec![user_message(vec![ResponseContentPart::InputFile { detail: Some(FileDetail::High), file_data: Some("JVBERi0xLjQK".to_string()), @@ -427,14 +451,27 @@ mod tests { }])]); let err = validate_harmony_responses_input(&request).expect_err("PDF file_data must be rejected"); - assert_is_unsupported(err); + assert_eq!(err.status(), StatusCode::BAD_REQUEST); + assert_eq!( + err.headers() + .get(HEADER_X_SMG_ERROR_CODE) + .and_then(|h| h.to_str().ok()), + Some("unsupported_content"), + ); + let message = error_message(err).await; + assert!( + message.contains("PDF") && message.contains("R4"), + "PDF rejections must cite R4 so callers can correlate the paused task; got: {message}" + ); } - #[test] - fn input_file_non_pdf_file_data_is_rejected() { + #[tokio::test] + async fn input_file_non_pdf_file_data_is_rejected() { // A JPEG magic-byte prefix (FFD8FFE0 ... base64 `/9j/4A==`) is // not a PDF; it must still be rejected because the harmony // pipeline cannot route it — but *not* with the R4 message. + // Asserts the generic file-data branch is taken so we do not + // misdirect clients to the paused R4 task. let request = request_with_items(vec![user_message(vec![ResponseContentPart::InputFile { detail: None, file_data: Some("/9j/4AAQSkZJRg==".to_string()), @@ -444,7 +481,15 @@ mod tests { }])]); let err = validate_harmony_responses_input(&request) .expect_err("JPEG file_data must be rejected"); - assert_is_unsupported(err); + let message = error_message(err).await; + assert!( + !message.contains("R4"), + "non-PDF file_data must not cite R4; got: {message}" + ); + assert!( + message.contains("file_data"), + "generic file-data rejections must mention file_data; got: {message}" + ); } #[test] @@ -539,8 +584,8 @@ mod tests { .expect("SimpleInputMessage.String must pass unchanged"); } - #[test] - fn data_url_prefix_pdf_magic_still_rejected_with_r4_message() { + #[tokio::test] + async fn data_url_prefix_pdf_magic_still_rejected_with_r4_message() { // Robustness: if a client wraps the base64 payload in a // `data:application/pdf;base64,` envelope inside `file_data` // (non-canonical but observed in the wild), the magic-byte @@ -555,11 +600,15 @@ mod tests { }])]); let err = validate_harmony_responses_input(&request) .expect_err("data-URL-wrapped PDF file_data must be rejected"); - assert_is_unsupported(err); + let message = error_message(err).await; + assert!( + message.contains("PDF") && message.contains("R4"), + "data-URL-wrapped PDF must reach the R4-specific message; got: {message}" + ); } - #[test] - fn data_url_prefix_is_case_insensitive_on_base64_separator() { + #[tokio::test] + async fn data_url_prefix_is_case_insensitive_on_base64_separator() { // `is_data_url` matches `data:` case-insensitively, so // `strip_data_url_prefix` must match `;base64,` with the same // flexibility. Before the R3 follow-up both `Data:` and @@ -575,11 +624,15 @@ mod tests { }])]); let err = validate_harmony_responses_input(&request) .expect_err("mixed-case data-URL wrapper must still reach the PDF sniffer"); - assert_is_unsupported(err); + let message = error_message(err).await; + assert!( + message.contains("PDF") && message.contains("R4"), + "mixed-case data-URL wrapper must reach the R4-specific message; got: {message}" + ); } - #[test] - fn pdf_sniff_decodes_only_magic_prefix_not_full_payload() { + #[tokio::test] + async fn pdf_sniff_decodes_only_magic_prefix_not_full_payload() { // Regression guard for the bounded-decode path: a large PDF // `file_data` must still route to the R4-specific message // without the validator materializing the entire payload. We @@ -600,6 +653,27 @@ mod tests { }])]); let err = validate_harmony_responses_input(&request) .expect_err("large PDF file_data must still be rejected with the R4 message"); - assert_is_unsupported(err); + let message = error_message(err).await; + assert!( + message.contains("PDF") && message.contains("R4"), + "bounded-decode path must still emit the R4-specific message; got: {message}" + ); + } + + #[tokio::test] + async fn refusal_role_rejection_cites_assistant_exception() { + // The non-assistant refusal branch has a distinct + // human-readable message; lock it in so future refactors do + // not regress the rationale the caller sees. + let request = request_with_items(vec![user_message(vec![ResponseContentPart::Refusal { + refusal: "I cannot process that.".to_string(), + }])]); + let err = validate_harmony_responses_input(&request) + .expect_err("user-role refusals must be rejected"); + let message = error_message(err).await; + assert!( + message.contains("refusal") && message.contains("assistant"), + "refusal rejection must name the assistant-role exception; got: {message}" + ); } } diff --git a/model_gateway/src/routers/grpc/harmony/responses/non_streaming.rs b/model_gateway/src/routers/grpc/harmony/responses/non_streaming.rs index bda63f03f2..8c01b36b23 100644 --- a/model_gateway/src/routers/grpc/harmony/responses/non_streaming.rs +++ b/model_gateway/src/routers/grpc/harmony/responses/non_streaming.rs @@ -63,8 +63,9 @@ pub(crate) async fn serve_harmony_responses( // multimodal in request_building), so silently dropping these // parts would lose data invisibly. Validate the caller's original // input *before* we merge persisted history via - // `load_previous_messages` so stored assistant-role refusals keep - // replaying cleanly. + // `load_previous_messages` so fresh-input issues are reported as + // the caller's problem rather than appearing to come from the + // stored chain. validate_harmony_responses_input(&request)?; // Clone request for persistence @@ -73,6 +74,15 @@ pub(crate) async fn serve_harmony_responses( // Load previous conversation history if previous_response_id is set let current_request = load_previous_messages(ctx, request).await?; + // R3: re-validate after history merge. Stored threads may contain + // image/file content parts persisted by a sibling router that + // *does* support multimodal (e.g. the OpenAI-compat passthrough); + // replaying such a thread through the harmony backend would hit + // the same silent-drop regression this validator exists to close. + // The validator accepts assistant-role refusal replays, so + // legitimate multi-turn history passes through unchanged. + validate_harmony_responses_input(¤t_request)?; + // Check MCP connection and get whether MCP tools are present let (has_mcp_tools, mcp_servers) = ensure_mcp_connection(&ctx.mcp_orchestrator, current_request.tools.as_deref()).await?; diff --git a/model_gateway/src/routers/grpc/harmony/responses/streaming.rs b/model_gateway/src/routers/grpc/harmony/responses/streaming.rs index 3b75fc4f00..2f4146bf11 100644 --- a/model_gateway/src/routers/grpc/harmony/responses/streaming.rs +++ b/model_gateway/src/routers/grpc/harmony/responses/streaming.rs @@ -63,6 +63,17 @@ pub(crate) async fn serve_harmony_responses_stream( Err(err_response) => return err_response, }; + // R3: re-validate after history merge. Stored threads may contain + // image/file content parts persisted by a sibling router that + // *does* support multimodal (e.g. the OpenAI-compat passthrough); + // replaying such a thread through the harmony backend would hit + // the same silent-drop regression this validator exists to close. + // The validator accepts assistant-role refusal replays, so + // legitimate multi-turn history passes through unchanged. + if let Err(err_response) = validate_harmony_responses_input(¤t_request) { + return err_response; + } + // Check MCP connection BEFORE starting stream and get whether MCP tools are present let (has_mcp_tools, mcp_servers) = match ensure_mcp_connection( &ctx.mcp_orchestrator, From 7e7b63e481e3b4cbfb44ce133c834549bde733aa Mon Sep 17 00:00:00 2001 From: Simo Lin Date: Fri, 24 Apr 2026 13:51:25 -0700 Subject: [PATCH 4/5] fix(grpc-harmony): bound data-URL separator scan and tighten R3 doc comments MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Address three review comments on the R3 PR: 1. **P2 codex — bound `find_base64_separator` scan**. A `data:` payload without an early `;base64,` separator previously walked every 8-byte window across the full string before giving up. Combined with the router's large `max_payload_size` default (512 MB in `model_gateway/src/config/types.rs`), that turned the fast-path 400 rejection into an O(n) CPU walk on adversarial payloads. Cap the scan at the first 256 bytes via `DATA_URL_HEADER_SCAN_LIMIT`. RFC 2397 data-URL headers carry at most a MIME type + a handful of parameters, so 256 bytes is comfortably larger than any realistic header. Beyond the cap the sniffer falls through to the generic file-data rejection — the same behavior as malformed base64 — so no valid payload regresses. Add `data_url_separator_scan_is_bounded_to_header_region` to lock the cap in: constructs a `data:` payload with a 64 KiB header and no early separator, asserts the validator rejects without claiming PDF identification. 2. **Nit coderabbit — doc comment outdated**. The comment above `validate_harmony_responses_input` still claimed persisted history is "skipped," but the previous commit added a second invocation on the merged `current_request`. Rewrite the rustdoc to describe the two-phase pattern explicitly and cite both call sites. 3. **Nit coderabbit — InputFile all-None fallback intent**. Annotate the branch that rejects an `InputFile` with neither `file_data`, `file_id`, nor `file_url`. Each field is individually `Option`al per `crates/protocols/src/responses.rs`, and protocol-level validation does not currently enforce "at-least-one-source", so the arm is a defense-in-depth guard against silent-accept holes if upstream validation is relaxed. No behavior change for valid content-part inputs; only tightens the reject path against DoS-adjacent scans and documents the validator's actual contract. All 18 content_parts tests pass. `cargo clippy -p smg --all-targets -- -D warnings` clean. Refs: .claude/_audit/responses-api-gap-audit.md §Task R3 Signed-off-by: Simo Lin --- .../grpc/harmony/responses/content_parts.rs | 98 +++++++++++++++++-- 1 file changed, 88 insertions(+), 10 deletions(-) diff --git a/model_gateway/src/routers/grpc/harmony/responses/content_parts.rs b/model_gateway/src/routers/grpc/harmony/responses/content_parts.rs index ae14c5f438..7cf6f3cb41 100644 --- a/model_gateway/src/routers/grpc/harmony/responses/content_parts.rs +++ b/model_gateway/src/routers/grpc/harmony/responses/content_parts.rs @@ -63,19 +63,34 @@ use crate::routers::error; /// case where a PDF is base64-encoded without additional wrapping. const PDF_MAGIC: &[u8] = b"%PDF-"; -/// Validate that the user-supplied input in a ResponsesRequest contains -/// only content parts the harmony Responses router can faithfully render. +/// Validate that a ResponsesRequest's `input` contains only content +/// parts the harmony Responses router can faithfully render. /// /// Returns `Ok(())` when the request is safe to dispatch, or a /// `400 unsupported_content` response otherwise. The error message is /// deliberately descriptive so callers can distinguish the rejection /// reason (image, file, PDF, refusal, file_id) at the HTTP layer. /// -/// Only the caller-submitted `request.input` is inspected. Persisted -/// history loaded via `previous_response_id` is skipped: it was -/// validated when it was first submitted, may legitimately carry -/// assistant-refusal replays, and is not something the caller can -/// mutate on this request. +/// # Two-phase invocation +/// +/// The non-streaming and streaming entry points call this validator at +/// **two** points on every request: +/// +/// 1. On the raw `request` as submitted by the caller, before any +/// history merge. Fresh-input issues surface as the caller's fault +/// rather than appearing to come from the stored chain. +/// 2. On the merged `current_request` produced by +/// `load_previous_messages`. Stored threads may contain image/file +/// content parts that were persisted by a sibling router supporting +/// multimodal (notably R1's OpenAI-compat passthrough); replaying +/// such a thread through the harmony backend would otherwise hit +/// the same silent-drop regression this validator exists to close. +/// +/// The validator's assistant-role-refusal exception means legitimate +/// multi-turn output replay passes through the second phase unchanged +/// — only non-assistant image/file/refusal parts in history are +/// rejected. Both phases run the same code so the policy stays +/// single-source. #[expect( clippy::result_large_err, reason = "axum Response is the standard error type on the router-entry boundary; \ @@ -191,6 +206,17 @@ fn validate_content_parts(role: &str, parts: &[ResponseContentPart]) -> Result<( Responses backend", )); } + // Defense in depth: `ResponseContentPart::InputFile` + // declares `file_data`, `file_id`, and `file_url` as + // individually optional (see `crates/protocols/src/ + // responses.rs`), so in principle an attachment could + // carry only `detail` / `filename` metadata with no + // content source at all. Protocol-level input + // validation does not currently enforce + // "at-least-one-source", so this arm is reachable via + // a crafted request. Rejecting it here keeps the + // validator exhaustive and avoids a silent-accept hole + // in case upstream validation is relaxed. return Err(unsupported( "input_file content parts are not supported on the harmony Responses backend", )); @@ -280,16 +306,32 @@ fn strip_data_url_prefix(input: &str) -> &str { } /// Locate the `;base64,` separator inside a data URL, matching -/// case-insensitively. Uses a byte-window walk (no allocation) so the -/// hot path stays allocation-free even on large payloads. +/// case-insensitively. Uses a byte-window walk (no allocation) bounded +/// to the first [`DATA_URL_HEADER_SCAN_LIMIT`] bytes so the hot path +/// stays allocation- *and* CPU-free even on adversarial large payloads: +/// a malformed `data:` string without an early separator would otherwise +/// force a full linear scan over hundreds of megabytes (the router +/// permits up to `max_payload_size` in +/// `model_gateway/src/config/types.rs`), which would be a CPU-sink for +/// what is otherwise a fast-path 400 rejection. fn find_base64_separator(input: &str) -> Option { const NEEDLE: &[u8] = b";base64,"; - input + let header = input .as_bytes() + .get(..DATA_URL_HEADER_SCAN_LIMIT.min(input.len()))?; + header .windows(NEEDLE.len()) .position(|window| window.eq_ignore_ascii_case(NEEDLE)) } +/// Upper bound on how many leading bytes we scan for a `;base64,` +/// separator in a `data:` URL. RFC 2397 data-URL headers carry at most +/// a MIME type plus a handful of parameters (`charset`, `boundary`, +/// etc.); 256 bytes is comfortably larger than any realistic header and +/// small enough that the scan is O(1) regardless of the payload size +/// behind it. +const DATA_URL_HEADER_SCAN_LIMIT: usize = 256; + /// Build the standard `400 unsupported_content` response used by every /// rejection branch in this module. fn unsupported(message: &str) -> Response { @@ -660,6 +702,42 @@ mod tests { ); } + #[tokio::test] + async fn data_url_separator_scan_is_bounded_to_header_region() { + // Adversarial case: a `data:` payload without an early + // `;base64,` separator must not force a full linear scan over + // a multi-megabyte body. We construct a `data:` string with an + // oversize MIME header region and no separator, then assert + // the request is still rejected quickly (no panic, no timeout). + // + // This is a behavioral guard — the absence of a separator + // means the PDF sniffer falls through to the generic + // file-data rejection — but it locks in the + // `DATA_URL_HEADER_SCAN_LIMIT` cap so a future relaxation + // cannot quietly reintroduce the O(n) scan. + let mut payload = String::from("data:application/octet-stream;"); + payload.push_str(&"x".repeat(65_536)); // far beyond the header cap + payload.push_str(",JVBERi0x"); // PDF magic in payload, no ;base64, + let request = request_with_items(vec![user_message(vec![ResponseContentPart::InputFile { + detail: None, + file_data: Some(payload), + file_id: None, + file_url: None, + filename: None, + }])]); + let err = validate_harmony_responses_input(&request).expect_err( + "adversarial data-URL without early separator must still be rejected", + ); + // Generic file-data branch — the PDF sniffer couldn't decode + // because the strip failed, which is the desired behavior + // (fast rejection over DoS-adjacent full-string walk). + let message = error_message(err).await; + assert!( + !message.contains("R4"), + "bounded scan must not claim PDF identification when the separator is beyond the cap; got: {message}" + ); + } + #[tokio::test] async fn refusal_role_rejection_cites_assistant_exception() { // The non-assistant refusal branch has a distinct From 2e45a357308db89d8a00940233008f3a58fbe097 Mon Sep 17 00:00:00 2001 From: Simo Lin Date: Fri, 24 Apr 2026 14:06:37 -0700 Subject: [PATCH 5/5] style(grpc-harmony): apply `cargo +nightly fmt` to R3 content_parts module CI's `cargo +nightly fmt -- --check` step flagged formatting differences between the local default rustfmt and the repo's nightly toolchain on the new `content_parts.rs` module. Run `cargo +nightly fmt` over the file so the whitespace/line-break choices match the project convention. No behavior or test coverage change. Signed-off-by: Simo Lin --- .../grpc/harmony/responses/content_parts.rs | 176 +++++++++--------- 1 file changed, 92 insertions(+), 84 deletions(-) diff --git a/model_gateway/src/routers/grpc/harmony/responses/content_parts.rs b/model_gateway/src/routers/grpc/harmony/responses/content_parts.rs index 7cf6f3cb41..b81a3bdca7 100644 --- a/model_gateway/src/routers/grpc/harmony/responses/content_parts.rs +++ b/model_gateway/src/routers/grpc/harmony/responses/content_parts.rs @@ -96,9 +96,7 @@ const PDF_MAGIC: &[u8] = b"%PDF-"; reason = "axum Response is the standard error type on the router-entry boundary; \ matches the rest of the harmony Responses entry points" )] -pub(super) fn validate_harmony_responses_input( - request: &ResponsesRequest, -) -> Result<(), Response> { +pub(super) fn validate_harmony_responses_input(request: &ResponsesRequest) -> Result<(), Response> { let items = match &request.input { ResponseInput::Text(_) => return Ok(()), ResponseInput::Items(items) => items, @@ -369,8 +367,7 @@ mod tests { let bytes = to_bytes(body, usize::MAX) .await .expect("error body fits in memory"); - let value: Value = - serde_json::from_slice(&bytes).expect("error body is always valid JSON"); + let value: Value = serde_json::from_slice(&bytes).expect("error body is always valid JSON"); value .get("error") .and_then(|e| e.get("message")) @@ -431,9 +428,10 @@ mod tests { #[test] fn pure_input_text_items_are_accepted() { - let request = request_with_items(vec![user_message(vec![ResponseContentPart::InputText { - text: "Describe the weather.".to_string(), - }])]); + let request = + request_with_items(vec![user_message(vec![ResponseContentPart::InputText { + text: "Describe the weather.".to_string(), + }])]); validate_harmony_responses_input(&request).expect("InputText must pass"); } @@ -441,23 +439,25 @@ mod tests { fn input_image_data_url_is_rejected() { // R3 scope item: InputImage with a `data:` URL. The pre-R3 // builder dropped this silently; R3 mandates a 400. - let request = request_with_items(vec![user_message(vec![ResponseContentPart::InputImage { - detail: Some(Detail::Auto), - file_id: None, - image_url: Some("data:image/jpeg;base64,/9j/4AAQSkZJRg==".to_string()), - }])]); - let err = - validate_harmony_responses_input(&request).expect_err("data-URL images must be rejected"); + let request = + request_with_items(vec![user_message(vec![ResponseContentPart::InputImage { + detail: Some(Detail::Auto), + file_id: None, + image_url: Some("data:image/jpeg;base64,/9j/4AAQSkZJRg==".to_string()), + }])]); + let err = validate_harmony_responses_input(&request) + .expect_err("data-URL images must be rejected"); assert_is_unsupported(err); } #[test] fn input_image_absolute_url_is_rejected() { - let request = request_with_items(vec![user_message(vec![ResponseContentPart::InputImage { - detail: None, - file_id: None, - image_url: Some("https://example.com/dog.jpg".to_string()), - }])]); + let request = + request_with_items(vec![user_message(vec![ResponseContentPart::InputImage { + detail: None, + file_id: None, + image_url: Some("https://example.com/dog.jpg".to_string()), + }])]); let err = validate_harmony_responses_input(&request) .expect_err("absolute-URL images must be rejected"); assert_is_unsupported(err); @@ -467,11 +467,12 @@ mod tests { fn input_image_file_id_is_rejected() { // R3 scope item: file_id is always unsupported in gRPC mode // (no Files API resolver). - let request = request_with_items(vec![user_message(vec![ResponseContentPart::InputImage { - detail: None, - file_id: Some("file-abc".to_string()), - image_url: None, - }])]); + let request = + request_with_items(vec![user_message(vec![ResponseContentPart::InputImage { + detail: None, + file_id: Some("file-abc".to_string()), + image_url: None, + }])]); let err = validate_harmony_responses_input(&request) .expect_err("file_id images must be rejected"); assert_is_unsupported(err); @@ -484,13 +485,14 @@ mod tests { // under R4 rather than a blanket rejection. We assert on the // message body (not just status/code) so the PDF-specific // branch is distinguishable from a generic file-data rejection. - let request = request_with_items(vec![user_message(vec![ResponseContentPart::InputFile { - detail: Some(FileDetail::High), - file_data: Some("JVBERi0xLjQK".to_string()), - file_id: None, - file_url: None, - filename: Some("report.pdf".to_string()), - }])]); + let request = + request_with_items(vec![user_message(vec![ResponseContentPart::InputFile { + detail: Some(FileDetail::High), + file_data: Some("JVBERi0xLjQK".to_string()), + file_id: None, + file_url: None, + filename: Some("report.pdf".to_string()), + }])]); let err = validate_harmony_responses_input(&request).expect_err("PDF file_data must be rejected"); assert_eq!(err.status(), StatusCode::BAD_REQUEST); @@ -514,13 +516,14 @@ mod tests { // pipeline cannot route it — but *not* with the R4 message. // Asserts the generic file-data branch is taken so we do not // misdirect clients to the paused R4 task. - let request = request_with_items(vec![user_message(vec![ResponseContentPart::InputFile { - detail: None, - file_data: Some("/9j/4AAQSkZJRg==".to_string()), - file_id: None, - file_url: None, - filename: Some("photo.jpg".to_string()), - }])]); + let request = + request_with_items(vec![user_message(vec![ResponseContentPart::InputFile { + detail: None, + file_data: Some("/9j/4AAQSkZJRg==".to_string()), + file_id: None, + file_url: None, + filename: Some("photo.jpg".to_string()), + }])]); let err = validate_harmony_responses_input(&request) .expect_err("JPEG file_data must be rejected"); let message = error_message(err).await; @@ -536,13 +539,14 @@ mod tests { #[test] fn input_file_file_url_is_rejected() { - let request = request_with_items(vec![user_message(vec![ResponseContentPart::InputFile { - detail: None, - file_data: None, - file_id: None, - file_url: Some("https://example.com/report.pdf".to_string()), - filename: None, - }])]); + let request = + request_with_items(vec![user_message(vec![ResponseContentPart::InputFile { + detail: None, + file_data: None, + file_id: None, + file_url: Some("https://example.com/report.pdf".to_string()), + filename: None, + }])]); let err = validate_harmony_responses_input(&request) .expect_err("file_url attachments must be rejected"); assert_is_unsupported(err); @@ -550,13 +554,14 @@ mod tests { #[test] fn input_file_file_id_is_rejected() { - let request = request_with_items(vec![user_message(vec![ResponseContentPart::InputFile { - detail: None, - file_data: None, - file_id: Some("file-abc".to_string()), - file_url: None, - filename: None, - }])]); + let request = + request_with_items(vec![user_message(vec![ResponseContentPart::InputFile { + detail: None, + file_data: None, + file_id: Some("file-abc".to_string()), + file_url: None, + filename: None, + }])]); let err = validate_harmony_responses_input(&request) .expect_err("file_id attachments must be rejected"); assert_is_unsupported(err); @@ -633,13 +638,14 @@ mod tests { // (non-canonical but observed in the wild), the magic-byte // sniff must still catch it so the caller sees the R4-specific // message rather than a generic rejection. - let request = request_with_items(vec![user_message(vec![ResponseContentPart::InputFile { - detail: None, - file_data: Some("data:application/pdf;base64,JVBERi0xLjQK".to_string()), - file_id: None, - file_url: None, - filename: None, - }])]); + let request = + request_with_items(vec![user_message(vec![ResponseContentPart::InputFile { + detail: None, + file_data: Some("data:application/pdf;base64,JVBERi0xLjQK".to_string()), + file_id: None, + file_url: None, + filename: None, + }])]); let err = validate_harmony_responses_input(&request) .expect_err("data-URL-wrapped PDF file_data must be rejected"); let message = error_message(err).await; @@ -657,13 +663,14 @@ mod tests { // `;Base64,` would skip the strip, mis-routing the request into // the generic "file_data unsupported" path instead of the // PDF-specific R4 message. - let request = request_with_items(vec![user_message(vec![ResponseContentPart::InputFile { - detail: None, - file_data: Some("Data:application/pdf;Base64,JVBERi0xLjQK".to_string()), - file_id: None, - file_url: None, - filename: None, - }])]); + let request = + request_with_items(vec![user_message(vec![ResponseContentPart::InputFile { + detail: None, + file_data: Some("Data:application/pdf;Base64,JVBERi0xLjQK".to_string()), + file_id: None, + file_url: None, + filename: None, + }])]); let err = validate_harmony_responses_input(&request) .expect_err("mixed-case data-URL wrapper must still reach the PDF sniffer"); let message = error_message(err).await; @@ -686,13 +693,14 @@ mod tests { // `%PDF-1` → base64 `JVBERi0x` (the canonical PDF header). let mut payload = String::from("JVBERi0x"); payload.push_str(&"A".repeat(65_536)); - let request = request_with_items(vec![user_message(vec![ResponseContentPart::InputFile { - detail: None, - file_data: Some(payload), - file_id: None, - file_url: None, - filename: None, - }])]); + let request = + request_with_items(vec![user_message(vec![ResponseContentPart::InputFile { + detail: None, + file_data: Some(payload), + file_id: None, + file_url: None, + filename: None, + }])]); let err = validate_harmony_responses_input(&request) .expect_err("large PDF file_data must still be rejected with the R4 message"); let message = error_message(err).await; @@ -718,16 +726,16 @@ mod tests { let mut payload = String::from("data:application/octet-stream;"); payload.push_str(&"x".repeat(65_536)); // far beyond the header cap payload.push_str(",JVBERi0x"); // PDF magic in payload, no ;base64, - let request = request_with_items(vec![user_message(vec![ResponseContentPart::InputFile { - detail: None, - file_data: Some(payload), - file_id: None, - file_url: None, - filename: None, - }])]); - let err = validate_harmony_responses_input(&request).expect_err( - "adversarial data-URL without early separator must still be rejected", - ); + let request = + request_with_items(vec![user_message(vec![ResponseContentPart::InputFile { + detail: None, + file_data: Some(payload), + file_id: None, + file_url: None, + filename: None, + }])]); + let err = validate_harmony_responses_input(&request) + .expect_err("adversarial data-URL without early separator must still be rejected"); // Generic file-data branch — the PDF sniffer couldn't decode // because the strip failed, which is the desired behavior // (fast rejection over DoS-adjacent full-string walk).