mirror of
https://github.com/diegosouzapw/OmniRoute.git
synced 2026-09-19 13:23:50 +03:00
fix(chat): reject null/non-object entries in messages[] (#12643)
A messages array containing null (or any non-object entry, e.g. [null] or [42]) passed every existing entry guard in chat.ts (#5110/#6402/#6407/#6412) and reached downstream translators/session helpers that read `.role` / `.content` directly off each entry (openai-to-claude.ts, sessionManager.ts, contextManager.ts's fixToolPairs), crashing with a raw TypeError and surfacing as an HTTP 500 instead of a clean 400. The route's Zod schema is intentionally wide (z.array(z.unknown())), so this shape check belongs in the handler's guard chain. Adds one more entry-shape guard clause to the same chokepoint, rejecting the request with a clear 400 before any routing or upstream call. Regression test: tests/unit/chat-messages-entry-objects-12643.test.ts PR #12644 (@soroush5) proposed this exact fix but was closed without merging on 2026-09-12; this re-implements it fresh against the current tip using the same guard shape and error message. Originally-proposed-by: @soroush5 in #12644 Co-authored-by: soroush5 <mrsoroushahmadi@gmail.com>
This commit is contained in:
1
changelog.d/fixes/12643-messages-entry-guard.md
Normal file
1
changelog.d/fixes/12643-messages-entry-guard.md
Normal file
@@ -0,0 +1 @@
|
||||
- **fix(chat):** requests with null/non-object entries in `messages[]` are now rejected with a clear 400 instead of crashing translators with an HTTP 500 ([#12643](https://github.com/diegosouzapw/OmniRoute/issues/12643)) — thanks @soroush5
|
||||
@@ -1,4 +1,5 @@
|
||||
{
|
||||
"_rebaseline_2026_09_15_12643_messages_entry_guard": "#12643 own growth: src/sse/handlers/chat.ts 2462->2472 (+10). A `messages` array containing a null/non-object entry (e.g. `[null]`) passed every existing entry guard (#5110/#6402/#6407/#6412) and crashed downstream translators/session helpers (openai-to-claude.ts, sessionManager.ts, contextManager.ts's fixToolPairs) reading `.role`/`.content` off the raw entry, surfacing as an HTTP 500 instead of a clean 400. Adds one more entry-shape guard clause to the same chokepoint, extending the existing guard family — same pattern, irreducible call-site wiring (the check itself is a one-line `.some()` predicate, not extractable into its own leaf without hiding the chokepoint). Covered by tests/unit/chat-messages-entry-objects-12643.test.ts (3/3) plus the sibling guard suites (chat-messages-validation-6402.test.ts, chat-non-string-model-6407.test.ts, 22/22, no regression).",
|
||||
"_rebaseline_2026_09_14_13547_noauth_model_lockout": "PR #13547 own growth: src/sse/services/auth.ts 3542->3556 (+14). The synthetic noauth connection short-circuited before the per-connection status pass, so a recorded model-only lockout was never enforced and the locked model was retried on every request. Irreducible at the early-return site. Covered by tests/unit/noauth-model-lockout.test.ts (carried from #13527).",
|
||||
"_rebaseline_2026_09_14_13404_healthcheck_backup_prune": "PR #13404 own growth: src/lib/db/core.ts 1745->1767 (+22). createManagedDbBackup (the health-check-repair snapshot path) never ran retention, so every restart of a healthy DB added a full-size copy to db_backups/; it now prunes with the same env-driven limits as backup.ts, importing backupRetention directly to avoid the backup.ts cycle. Covered by tests/unit/db-backup-healthcheck-prune-13308.test.ts.",
|
||||
"_rebaseline_2026_09_14_13349_virtualfactory_custom_models_guard": "PR #13349 own growth: open-sse/services/autoCombo/virtualFactory.ts 1219->1230 (+11). The customModels key_value blob is operator-writable raw JSON, so a null or non-object row null-derefed every read and no auto/* pool could materialize; the builder now filters rows the same way catalog.ts already does. Irreducible at the read site. Covered by tests/unit/combo-auto-pool-visible-only.test.ts.",
|
||||
@@ -477,7 +478,7 @@
|
||||
"src/shared/components/RequestLoggerV2.tsx": 1718,
|
||||
"src/shared/constants/providers/apikey/gateways.ts": 1502,
|
||||
"src/shared/services/cliRuntime.ts": 1296,
|
||||
"src/sse/handlers/chat.ts": 2462,
|
||||
"src/sse/handlers/chat.ts": 2472,
|
||||
"src/sse/services/auth.ts": 3556,
|
||||
"tests/unit/account-fallback-service.test.ts": 2453,
|
||||
"tests/unit/provider-validation-specialty.test.ts": 4656,
|
||||
|
||||
@@ -476,6 +476,16 @@ async function handleChatImplementation(
|
||||
log.warn("CHAT", "Rejecting request with empty messages array");
|
||||
return errorResponse(HTTP_STATUS.BAD_REQUEST, "messages: at least one message is required");
|
||||
}
|
||||
// Reject non-object entries before they reach code that reads `msg.role` /
|
||||
// `msg.content` off them (crash-then-500 in translators — #12643). The
|
||||
// route schema accepts `z.array(z.unknown())`, so `[null]` gets this far.
|
||||
if (
|
||||
Array.isArray(msgBody.messages) &&
|
||||
msgBody.messages.some((m) => m === null || typeof m !== "object" || Array.isArray(m))
|
||||
) {
|
||||
log.warn("CHAT", "Rejecting request with non-object message entries");
|
||||
return errorResponse(HTTP_STATUS.BAD_REQUEST, "messages: Expected array of objects");
|
||||
}
|
||||
if (!("messages" in msgBody) && !("input" in msgBody) && sourceFormat !== "antigravity") {
|
||||
log.warn("CHAT", "Rejecting request with missing messages");
|
||||
return errorResponse(HTTP_STATUS.BAD_REQUEST, "messages: Expected array, received undefined");
|
||||
@@ -1014,7 +1024,15 @@ async function handleChatImplementation(
|
||||
if (isComboLiveTest) return true;
|
||||
// #12886: combo-name allow-list must not skip inner targets (#9057 still
|
||||
// checks auto/* / disableNonPublic via comboTargetPassesKeyModelPolicy).
|
||||
if (!(await comboTargetPassesKeyModelPolicy({ apiKey, apiKeyInfo, requestedModelStr: resolvedModelStr, targetModelStr: modelString, isModelAllowedForKey }))) {
|
||||
if (
|
||||
!(await comboTargetPassesKeyModelPolicy({
|
||||
apiKey,
|
||||
apiKeyInfo,
|
||||
requestedModelStr: resolvedModelStr,
|
||||
targetModelStr: modelString,
|
||||
isModelAllowedForKey,
|
||||
}))
|
||||
) {
|
||||
return false;
|
||||
}
|
||||
|
||||
|
||||
69
tests/unit/chat-messages-entry-objects-12643.test.ts
Normal file
69
tests/unit/chat-messages-entry-objects-12643.test.ts
Normal file
@@ -0,0 +1,69 @@
|
||||
import test from "node:test";
|
||||
import assert from "node:assert/strict";
|
||||
|
||||
import { createChatPipelineHarness } from "../integration/_chatPipelineHarness.ts";
|
||||
|
||||
// Regression tests for #12643 — a `messages` array containing `null` (or any
|
||||
// non-object entry) passed every entry guard and crashed translators with a
|
||||
// raw TypeError (`msg.role` off null), surfacing as HTTP 500.
|
||||
//
|
||||
// The guard at src/sse/handlers/chat.ts now rejects non-object entries with a
|
||||
// clear OmniRoute-level 400 before any routing or upstream call, extending the
|
||||
// #5110/#6402/#6407/#6412 guard family.
|
||||
|
||||
const harness = await createChatPipelineHarness("chat-messages-entry-objects-12643");
|
||||
const { handleChat, buildRequest, resetStorage, seedConnection } = harness;
|
||||
|
||||
test.beforeEach(async () => {
|
||||
await resetStorage();
|
||||
});
|
||||
|
||||
test.after(async () => {
|
||||
await harness.cleanup();
|
||||
});
|
||||
|
||||
async function postMessages(messages: unknown) {
|
||||
await seedConnection("anthropic", { apiKey: "sk-ant" });
|
||||
|
||||
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: "anthropic/claude-haiku-4-5",
|
||||
messages,
|
||||
},
|
||||
})
|
||||
);
|
||||
const body = (await response.json()) as { error?: { message?: string } };
|
||||
return { response, body, upstreamCalled };
|
||||
}
|
||||
|
||||
test("#12643: messages: [null] is rejected with a clear 400", async () => {
|
||||
const { response, body, upstreamCalled } = await postMessages([null]);
|
||||
|
||||
assert.equal(response.status, 400, "null entry must be a 400, not a 500 crash");
|
||||
assert.match(body.error?.message ?? "", /Expected array of objects/i);
|
||||
assert.equal(upstreamCalled, false, "must not forward upstream");
|
||||
});
|
||||
|
||||
test("#12643: messages with string/number entries are rejected with a clear 400", async () => {
|
||||
for (const messages of [[{ role: "user", content: "hi" }, "oops"], [42]]) {
|
||||
const { response, body, upstreamCalled } = await postMessages(messages);
|
||||
|
||||
assert.equal(response.status, 400, `must be a 400: ${JSON.stringify(messages)}`);
|
||||
assert.match(body.error?.message ?? "", /Expected array of objects/i);
|
||||
assert.equal(upstreamCalled, false, "must not forward upstream");
|
||||
}
|
||||
});
|
||||
|
||||
test("#12643: well-formed messages pass the entry guard", async () => {
|
||||
const { response, body } = await postMessages([{ role: "user", content: "hi" }]);
|
||||
|
||||
const msg = body.error?.message ?? "";
|
||||
assert.ok(!(response.status === 400 && /Expected array of objects/i.test(msg)));
|
||||
});
|
||||
Reference in New Issue
Block a user