fix(chat): answer 400 when the JSON body is not an object

changelog.d/fixes/16668-chat-non-object-body-400.md

@@ -0,0 +1 @@
+- **fix(chat):** a JSON body that is valid JSON but not an object (`null`, a string, a number or `true`) made `handleChat` throw (`Cannot use 'in' operator to search for 'messages' in x`, `Cannot convert undefined or null to object`) and the request ended as a 500 on `/v1/chat/completions`, `/v1/messages` and `/v1/completions`. It is now answered with a 400 `Request body must be a JSON object`, the message `/v1/responses` already used ([#16668](https://github.com/diegosouzapw/OmniRoute/pull/16668)) — thanks @pacocartones.

src/sse/handlers/admissionRejectionLog.ts

@@ -8,9 +8,12 @@
  * turn a rejection into a second failure. Fire-and-forget on purpose:
  * awaiting disk I/O here would hold the rejection path.
  */
+import { HTTP_STATUS } from "@omniroute/open-sse/config/constants.ts";
+import { errorResponse } from "@omniroute/open-sse/utils/error.ts";
 import { saveCallLog } from "@/lib/usageDb";
 import { cloneLogPayload } from "@/lib/logPayloads";
 import { redactVideoTranscriptFieldsForLog } from "@/lib/guardrails/videoBridgeSnapshotRedaction";
+import * as log from "../utils/logger";
 
 export interface AdmissionRejectionLogEntry {
   path: string;
@@ -64,3 +67,41 @@ export function logHandlerRejection(
   void logAdmissionRejection(rejection, entry);
   return rejection;
 }
+
+type ChatPayloadRequest = { url: string };
+
+function payloadModel(body: unknown): string {
+  if (body !== null && typeof body === "object" && !Array.isArray(body)) {
+    const model = (body as { model?: unknown }).model;
+    if (typeof model === "string" && model) return model;
+  }
+  return "-";
+}
+
+function rejectWith(
+  request: ChatPayloadRequest,
+  reqId: string,
+  body: unknown,
+  message: string
+): Response {
+  return logHandlerRejection(errorResponse(HTTP_STATUS.BAD_REQUEST, message), {
+    path: new URL(request.url).pathname,
+    model: payloadModel(body),
+    requestBody: body ?? null,
+    apiKeyId: null,
+    apiKeyName: null,
+    correlationId: reqId,
+  });
+}
+
+/**
+ * 400 before handleChat reads messages off a payload that would throw into a 500.
+ * Returns the same body when the payload may proceed.
+ */
+export function rejectPayload(body: unknown, request: ChatPayloadRequest, reqId: string): unknown {
+  if (body === null || typeof body !== "object") {
+    log.warn("CHAT", "Rejecting request whose body is not a JSON object");
+    return rejectWith(request, reqId, null, "Request body must be a JSON object");
+  }
+  return body;
+}

src/sse/handlers/chat.ts

@@ -143,7 +143,7 @@ import {
 } from "./reasoningRouting";
 import { createVirtualAutoCombo, resolveAutoRoutingState } from "./autoRouting";
 import { getComboFailureLogError } from "./comboFailureLogging";
-import { logAdmissionRejection, logHandlerRejection } from "./admissionRejectionLog";
+import { logAdmissionRejection, logHandlerRejection, rejectPayload } from "./admissionRejectionLog";
 
 // Pipeline integration — wired modules
 import { classify429FromError, type FailureKind } from "@/shared/utils/classify429";
@@ -450,6 +450,7 @@ async function handleChatImplementation(
     });
   }
 
+  if ((body = rejectPayload(body, request, reqId)) instanceof Response) return body;
   // Only the server's policy resolver may attach execution directives or route traces.
   // Discard lookalike JSON fields supplied by callers before evaluating any rule.
   if (body && typeof body === "object") {

tests/unit/chat-non-object-body.test.ts

@@ -0,0 +1,50 @@
+import test from "node:test";
+import assert from "node:assert/strict";
+
+import { createChatPipelineHarness } from "../integration/_chatPipelineHarness.ts";
+
+// A JSON body that is valid JSON but not an object made `handleChat` throw
+// (`Cannot use 'in' operator to search for 'messages' in x`, `Cannot convert undefined or null to object`) and
+// the request ended as a 500. `/v1/responses` already answers it with "Request body must be a JSON object".
+
+const harness = await createChatPipelineHarness("chat-non-object-body");
+const { handleChat, buildRequest, resetStorage } = harness;
+
+test.beforeEach(async () => {
+  await resetStorage();
+});
+
+test.after(async () => {
+  await harness.cleanup();
+});
+
+for (const [label, body] of [
+  ["null", "null"],
+  ["a string", '"x"'],
+  ["a number", "5"],
+  ["true", "true"],
+] as const) {
+  for (const url of [
+    "http://localhost/v1/chat/completions",
+    "http://localhost/v1/messages",
+    "http://localhost/v1/completions",
+  ]) {
+    test(`${label} on ${new URL(url).pathname} is rejected with a clear 400`, async () => {
+      let status = 0;
+      let message = "";
+      try {
+        const response = await handleChat(buildRequest({ url, body }));
+        status = response.status;
+        message =
+          ((await response.json()) as { error?: { message?: string } }).error
+            ?.message ?? "";
+      } catch (error) {
+        // A throw out of handleChat is what the route turns into a 500.
+        status = 500;
+        message = error instanceof Error ? error.message : String(error);
+      }
+      assert.equal(status, 400, `expected 400, got ${status}: ${message}`);
+      assert.match(message, /Request body must be a JSON object/i);
+    });
+  }
+}