From a73c6ca5fb9ad7ee568d9cee7c89c8c9934682da Mon Sep 17 00:00:00 2001 From: Chirag Singhal <76880977+chirag127@users.noreply.github.com> Date: Tue, 7 Jul 2026 03:13:06 +0530 Subject: [PATCH] fix(sse): reject non-string `model` with 400 before resolver (#6407) (#6433) reject non-string model with 400 before resolver (#6407, 6/6). Reconciled with #6437 early schema validation on chat.ts. Integrated into release/v3.8.46. --- CHANGELOG.md | 1 + src/sse/handlers/chat.ts | 14 ++ tests/unit/chat-non-string-model-6407.test.ts | 123 ++++++++++++++++++ 3 files changed, 138 insertions(+) create mode 100644 tests/unit/chat-non-string-model-6407.test.ts diff --git a/CHANGELOG.md b/CHANGELOG.md index a65ecebcc7..1adda9c8ae 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -21,6 +21,7 @@ ### 🐛 Bug Fixes +- **fix(sse):** a non-string `model` field is now rejected with a `400` before the resolver, instead of crashing downstream `.toLowerCase()`/`.split()` calls into an empty-body `500` that escapes the error sanitizer ([#6407](https://github.com/diegosouzapw/OmniRoute/issues/6407)). Regression guard: `tests/unit/chat-non-string-model-6407.test.ts`. (thanks @chirag127) - **fix(api):** unknown `/api/*` routes now return a JSON `404` (instead of the dashboard HTML shell) and scalar chat params (`model`/`temperature`/etc.) are validated **before** the provider lookup so malformed requests fail fast with a clear `400` ([#6424](https://github.com/diegosouzapw/OmniRoute/issues/6424), [#6412](https://github.com/diegosouzapw/OmniRoute/issues/6412)). Regression guards: `tests/unit/api/api-catchall-json-404.test.ts`, `tests/unit/chat-early-schema-validation-6412.test.ts`. (thanks @chirag127) - **fix(api):** `/v1/chat/completions` now rejects a non-JSON `Content-Type` with a `400` before parsing the body ([#6414](https://github.com/diegosouzapw/OmniRoute/issues/6414)). Regression guard: `tests/unit/v1-chat-completions-content-type-6414.test.ts`. (thanks @chirag127) - **fix(api):** the `X-OmniRoute-Compression` response header is now echoed on `/v1/chat/completions` and `/v1/completions` ([#6422](https://github.com/diegosouzapw/OmniRoute/issues/6422)). Regression guard: `tests/unit/compression-header-echo-6422.test.ts`. (thanks @chirag127) diff --git a/src/sse/handlers/chat.ts b/src/sse/handlers/chat.ts index 5be9a25842..3a72acfe39 100644 --- a/src/sse/handlers/chat.ts +++ b/src/sse/handlers/chat.ts @@ -245,6 +245,20 @@ export async function handleChat( return errorResponse(HTTP_STATUS.BAD_REQUEST, "messages: at least one message is required"); } + // Reject non-string `model` before it reaches downstream code that calls + // `.toLowerCase()` / `.split()` / `.startsWith()` on it (crash-then-500 with an + // empty body, escaping the error sanitizer — #6407). An explicit `null`/`undefined` + // stays permitted here because the existing `Missing model` guard below returns a + // clean 400 for those; anything else that is not a string is a client type error. + const rawModel = (body as { model?: unknown }).model; + if (rawModel !== undefined && rawModel !== null && typeof rawModel !== "string") { + log.warn("CHAT", `Rejecting non-string model (typeof=${typeof rawModel})`); + return errorResponse( + HTTP_STATUS.BAD_REQUEST, + `model: Expected string, received ${Array.isArray(rawModel) ? "array" : typeof rawModel}` + ); + } + // Early schema validation for scalar params BEFORE provider/model resolution (#6412). // Previously, a bad `temperature: "not-a-number"` on an unknown provider returned // 404 "model_not_found" — hiding the real schema error. Validate the param shape diff --git a/tests/unit/chat-non-string-model-6407.test.ts b/tests/unit/chat-non-string-model-6407.test.ts new file mode 100644 index 0000000000..9552402519 --- /dev/null +++ b/tests/unit/chat-non-string-model-6407.test.ts @@ -0,0 +1,123 @@ +import test from "node:test"; +import assert from "node:assert/strict"; + +import { createChatPipelineHarness } from "../integration/_chatPipelineHarness.ts"; + +// Regression test for #6407 — a `model` field of a non-string type +// (`number`/`boolean`/`array`/`object`) crashed downstream string ops +// (`.toLowerCase()`/`.split()`/`.startsWith()`) and returned HTTP 500 with an +// empty body, bypassing the error sanitizer (Hard Rule #12). +// +// The guard rejects non-string `model` with a clean 400 + typed error message +// BEFORE the model resolver runs, matching the #5110 empty-messages precedent. + +const harness = await createChatPipelineHarness("chat-non-string-model-6407"); +const { handleChat, buildRequest, resetStorage, seedConnection } = harness; + +test.beforeEach(async () => { + await resetStorage(); +}); + +test.after(async () => { + await harness.cleanup(); +}); + +for (const [label, value, expectedType] of [ + ["number", 123, "number"], + ["boolean", true, "boolean"], + ["array", [], "array"], + ["object", {}, "object"], +] as const) { + test(`#6407: model as ${label} → 400 with typed error, no upstream call`, async () => { + await seedConnection("openai", { apiKey: "sk-openai" }); + + let upstreamCalled = false; + globalThis.fetch = async () => { + upstreamCalled = true; + return new Response("{}", { + status: 200, + headers: { "content-type": "application/json" }, + }); + }; + + const response = await handleChat( + buildRequest({ + body: { + model: value, + messages: [{ role: "user", content: "hi" }], + }, + }) + ); + + assert.equal(response.status, 400, `${label} model must be a 400, not 500`); + const body = (await response.json()) as { error?: { message?: string } }; + assert.match( + body.error?.message ?? "", + /model:\s*Expected string, received/i, + `error should say "model: Expected string, received ${expectedType}"` + ); + assert.ok( + body.error?.message?.includes(expectedType), + `error should include the received type "${expectedType}"` + ); + // Sanitized: no leaked stack frames per Hard Rule #12 / #6407 impact 3. + assert.ok( + !(body.error?.message ?? "").includes("at /"), + "error must not leak stack trace frames" + ); + assert.equal(upstreamCalled, false, "must not forward a non-string-model request upstream"); + }); +} + +test("#6407: string model still routes normally (guard is not over-broad)", async () => { + await seedConnection("openai", { apiKey: "sk-openai" }); + + let upstreamCalled = false; + globalThis.fetch = async () => { + upstreamCalled = true; + return Response.json({ + id: "x", + object: "chat.completion", + choices: [ + { index: 0, message: { role: "assistant", content: "hi" }, finish_reason: "stop" }, + ], + }); + }; + + const response = await handleChat( + buildRequest({ + body: { + model: "openai/gpt-4.1", + stream: false, + messages: [{ role: "user", content: "Hello" }], + }, + }) + ); + + assert.notEqual(response.status, 400, "a valid string model must not be caught by the guard"); + assert.equal(upstreamCalled, true, "a valid request must still reach upstream"); +}); + +test("#6407: null model still routed to the existing 'Missing model' 400 (not the new guard)", async () => { + await seedConnection("openai", { apiKey: "sk-openai" }); + + globalThis.fetch = async () => + new Response("{}", { status: 200, headers: { "content-type": "application/json" } }); + + const response = await handleChat( + buildRequest({ + body: { + model: null, + messages: [{ role: "user", content: "hi" }], + }, + }) + ); + + assert.equal(response.status, 400, "null model stays a 400"); + const body = (await response.json()) as { error?: { message?: string } }; + assert.match( + body.error?.message ?? "", + /missing model/i, + "null model keeps the existing 'Missing model' message (not the new type guard)" + ); +});