Repository navigation
fix(api): relax provider-scoped chat completion validation #5907
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -3,8 +3,6 @@ import { initTranslators } from "@omniroute/open-sse/translator/index.ts"; | |
| import { errorResponse } from "@omniroute/open-sse/utils/error.ts"; | ||
| import { HTTP_STATUS } from "@omniroute/open-sse/config/constants.ts"; | ||
| import { getRegistryEntry } from "@omniroute/open-sse/config/providerRegistry.ts"; | ||
| import { providerChatCompletionSchema } from "@/shared/validation/schemas"; | ||
| import { isValidationFailure, validateBody } from "@/shared/validation/helpers"; | ||
|
|
||
| let initialized = false; | ||
|
|
||
|
|
@@ -30,6 +28,7 @@ export async function OPTIONS() { | |
| /** | ||
| * POST /v1/providers/{provider}/chat/completions | ||
| * Routes to the specified provider, validating model/provider match. | ||
| * Full body format validation is delegated to handleChat. | ||
| */ | ||
| export async function POST(request, { params }) { | ||
| const { provider: rawProvider } = await params; | ||
|
|
@@ -45,18 +44,25 @@ export async function POST(request, { params }) { | |
|
|
||
| await ensureInitialized(); | ||
|
|
||
| // Clone request with provider-prefixed model | ||
| let rawBody; | ||
| // Parse body once so this provider-scoped route can normalize the model prefix | ||
| // before delegating full chat-format validation to handleChat. | ||
| let rawBody: unknown; | ||
| try { | ||
| rawBody = await request.json(); | ||
| } catch { | ||
| return errorResponse(HTTP_STATUS.BAD_REQUEST, "Invalid JSON body"); | ||
| } | ||
| const validation = validateBody(providerChatCompletionSchema, rawBody); | ||
| if (isValidationFailure(validation)) { | ||
| return errorResponse(HTTP_STATUS.BAD_REQUEST, validation.error.message); | ||
|
|
||
| if (!rawBody || typeof rawBody !== "object" || Array.isArray(rawBody)) { | ||
| return errorResponse(HTTP_STATUS.BAD_REQUEST, "Request body must be a JSON object"); | ||
| } | ||
|
|
||
| const body = rawBody as { model?: string; [key: string]: unknown }; | ||
|
Comment on lines
+56
to
+60
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Potential Bug / Content-Length Mismatch:\nWhen the model prefix is normalized (e.g., prepending |
||
|
|
||
| // Keep the route-level checks minimal: only guard fields needed for provider prefix handling. | ||
| if (body.model !== undefined && typeof body.model !== "string") { | ||
| return errorResponse(HTTP_STATUS.BAD_REQUEST, "model must be a string"); | ||
| } | ||
| const body = validation.data; | ||
|
|
||
| // Validate model belongs to this provider | ||
| if (body.model) { | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,86 @@ | ||
| // Regression guard for #5907 — the provider-scoped chat/completions route | ||
| // (`/v1/providers/{provider}/chat/completions`) must NOT re-apply the strict | ||
| // `providerChatCompletionSchema`. Full body-format validation is delegated to | ||
| // handleChat; the route keeps only the minimal guards it needs to normalize the | ||
| // model prefix. This test locks that contract without mocking handleChat (the | ||
| // project's node:test runner does not enable --experimental-test-module-mocks, | ||
| // and the Stryker tap-runner rejects mock.module), by exercising the branches | ||
| // that return BEFORE delegation: | ||
| // - a loosely-valid body (no `messages`, which the removed strict schema would | ||
| // have 400'd) now reaches the model-prefix logic instead of a schema 400; | ||
| // - the minimal route-level guards still reject invalid JSON, non-object | ||
| // bodies, non-string models, and unknown providers. | ||
| import { test, after } from "node:test"; | ||
| import assert from "node:assert/strict"; | ||
|
|
||
| const { POST } = | ||
| await import("../../src/app/api/v1/providers/[provider]/chat/completions/route.ts"); | ||
|
|
||
| // Importing the route transitively opens the SQLite handle (handleChat's graph). | ||
| // Release it so Node's native test runner does not hang on open handles. | ||
| after(async () => { | ||
| try { | ||
| const core = await import("../../src/lib/db/core.ts"); | ||
| core.resetDbInstance(); | ||
| } catch { | ||
| // best-effort cleanup — never fail the suite on teardown | ||
| } | ||
| }); | ||
|
|
||
| function makeRequest(body: string) { | ||
| return new Request("http://localhost/v1/providers/openai/chat/completions", { | ||
| method: "POST", | ||
| headers: { "Content-Type": "application/json" }, | ||
| body, | ||
| }); | ||
| } | ||
|
|
||
| const params = (provider: string) => ({ params: Promise.resolve({ provider }) }); | ||
|
|
||
| test("#5907 a loosely-valid body (no messages) is no longer rejected by the removed strict schema", async () => { | ||
| // `{ model: "anthropic/..." }` has NO `messages`. Under the old strict | ||
| // providerChatCompletionSchema this 400'd at the route before any prefix | ||
| // check. The relaxed route must instead run the model-prefix validation and | ||
| // return the *specific* cross-provider error — proving the schema is gone. | ||
| const res = await POST( | ||
| makeRequest(JSON.stringify({ model: "anthropic/claude-3-5-sonnet" })), | ||
| params("openai") | ||
| ); | ||
| assert.equal(res.status, 400); | ||
| const body = await res.json(); | ||
| assert.match( | ||
| body.error.message, | ||
| /does not belong to provider/i, | ||
| "expected the model-prefix check to run — a schema validation 400 would mean the strict schema is still applied" | ||
| ); | ||
| }); | ||
|
|
||
| test("#5907 rejects invalid JSON with 400", async () => { | ||
| const res = await POST(makeRequest("{not json"), params("openai")); | ||
| assert.equal(res.status, 400); | ||
| }); | ||
|
|
||
| test("#5907 rejects a non-object body (array) with 400", async () => { | ||
| const res = await POST(makeRequest(JSON.stringify([1, 2, 3])), params("openai")); | ||
| assert.equal(res.status, 400); | ||
| const body = await res.json(); | ||
| assert.match(body.error.message, /must be a JSON object/i); | ||
| }); | ||
|
|
||
| test("#5907 rejects a non-string model with 400", async () => { | ||
| const res = await POST(makeRequest(JSON.stringify({ model: 123 })), params("openai")); | ||
| assert.equal(res.status, 400); | ||
| const body = await res.json(); | ||
| assert.match(body.error.message, /model must be a string/i); | ||
| }); | ||
|
|
||
| test("#5907 rejects an unknown provider with 400", async () => { | ||
| const res = await POST(makeRequest(JSON.stringify({ model: "gpt-4o" })), params("nope-xyz")); | ||
| assert.equal(res.status, 400); | ||
| }); | ||
|
|
||
| test("#5907 route errors are sanitized (no stack trace leak in body)", async () => { | ||
| const res = await POST(makeRequest("{bad"), params("openai")); | ||
| const body = await res.json(); | ||
| assert.ok(!JSON.stringify(body).includes("at /"), "error body must not leak a stack trace"); | ||
| }); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Style Guide Violation (Rule 9 - Missing Tests):\nAccording to the Repository Style Guide (Section 2, Rule 9):\n> 9. Always include tests when changing production code (
src/,open-sse/,electron/,bin/).\n\nSince this PR modifies production code insrc/app/api/v1/providers/[provider]/chat/completions/route.tsto relax the validation logic, we must include or update corresponding tests.\n\nPlease add or update unit tests in thetests/directory (e.g., intests/unit/chat-route-coverage.test.tsor a new test file) to verify:\n1. That requests with extra fields (which were previously rejected byproviderChatCompletionSchema) are now successfully accepted and forwarded.\n2. That invalid JSON or a non-stringmodelare still rejected with a400 Bad Requestas expected.References