Skip to content

fix(api): sanitize /v1/batches create error body (Hard Rule #12) - #14115

Merged
diegosouzapw merged 1 commit into
diegosouzapw:release/v3.8.51from
xiechimon:fix/batches-route-error-sanitization
Sep 22, 2026
Merged

diegosouzapw merged 1 commit into
diegosouzapw:release/v3.8.51from
xiechimon:fix/batches-route-error-sanitization

Conversation

@xiechimon

Copy link
Copy Markdown
Contributor

What

POST /api/v1/batches now routes its catch-block error message through sanitizeErrorMessage() before it reaches the client, instead of forwarding the raw error.message.

In my own words: a DB-layer failure during batch create could ship file paths, SQL fragments or internal identifiers to the caller verbatim; this puts the response through the same shared sanitizer every other /v1 route already uses, and the 400 shape stays exactly as before.

Why

Closes #14089. Hard Rule #12 (AGENTS.md): every HTTP / SSE / executor error response must go through buildErrorBody() or sanitizeErrorMessage() from open-sse/utils/error.ts. This route's catch block was the one /v1/* spot that skipped it.

The one-line core fix:

message:
  sanitizeErrorMessage(error instanceof Error ? error.message : "Create failed") ||
  "Create failed",

The || "Create failed" keeps the message non-empty when sanitization fully redacts (the sanitizer returns ""/[REDACTED] shapes depending on input), matching the agents/credentials/route.ts pattern. I left the Zod validation.error.message path (line 30) alone on purpose: that is a schema-generated message with no runtime data, and every sibling route (audio/speech, classify, embeddings, images/generations) passes it through unchanged.

Also checked the other handlers in the file per the issue: GET has no catch block and parseBatchListLimit's message is a static string, so the POST catch was the only leak path.

Testing

New tests/unit/14089-batches-route-error-sanitization.test.ts drives the real POST handler with a real API key and a real seeded input file. The harness does not support mock.module() and mock.method cannot redefine the sealed ESM namespace export, so the batch INSERT is made to throw through the repo's own setDbInstance DI seam: a thin wrapper delegates every read (auth lookup, file ownership, validation) to the real adapter and only the INSERT INTO batches statement throws a path-and-stack-laden SQLITE_CANTOPEN.

  • Leak test: the 400 body contains neither the DB path, nor the storage.sqlite filename, nor a stack frame (\n at ); type stays invalid_request_error.
  • Control test: a clean, non-sensitive message (Batch endpoint is not enabled on this instance) survives sanitization intact, so the fix does not turn every error into [REDACTED].

Red on base: the leak test fails with response leaked the DB path: SQLITE_CANTOPEN: unable to open database file /tmp/.../storage.sqlite; after the fix, 2 pass.

Adjacent regression: all six existing batch test files green (88 pass / 0 fail, incl. the GHSA-2jm2/GHSA-m3hp ownership suite that exercises the same route). npx eslint and npx prettier --check clean on both touched files; npm run typecheck:core exits 0.

…apw#12)

POST /api/v1/batches returned the raw error.message from its catch block,
so a DB-layer failure could ship file paths, SQL fragments or internal
identifiers to the client. Route it through sanitizeErrorMessage() from
open-sse/utils/error.ts, the same shared sanitizer every other /v1 route
already uses, keeping the existing 400 + invalid_request_error shape and a
non-empty message fallback.

Adds route-level coverage driving the real POST handler with a real API key
and seeded input file: the batch INSERT is made to throw a path-and-stack
laden SQLITE_CANTOPEN through the repo's own setDbInstance seam (no module
mocking), asserting the path/filename/stack never reach the body, plus a
control that a clean message survives sanitization intact.

Closes diegosouzapw#14089
Copilot AI lite review requested due to automatic review settings September 18, 2026 16:21

Copilot AI left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copilot was unable to review this pull request because the user who requested the review has reached their quota limit.

diegosouzapw added a commit to arashashrafii/OmniRoute that referenced this pull request Sep 19, 2026
Removes src/shared/contracts/openai.ts and its test — the module was
never imported by any route/handler (verified via grep across
open-sse/, src/, tests/), so it added maintenance surface with zero
runtime effect. Fixes the two OPENAI_COMPATIBILITY.md doc-links that
pointed at the removed test file to point at existing coverage
instead (chat-completions-route-shape-gate.test.ts,
audio-transcription-handler.test.ts, audio-speech-handler.test.ts).

Also drops the src/app/api/v1/batches/route.ts sanitizeErrorMessage()
hunk from this PR: it duplicates the same Hard Rule diegosouzapw#12 fix already
proposed standalone in diegosouzapw#14115 and diegosouzapw#14145, so it is left for whichever
of those lands first rather than triple-implemented here.

The real fix in this PR — DSML tool-call normalization for
DeepSeek-shaped web providers in webTools.ts/deepseekWebTools.ts —
is unaffected and remains the mergeable core.

Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
@diegosouzapw

Copy link
Copy Markdown
Owner

Thanks @xiechimon — merging via the release merge-train. Validated in local merge-train (mt-train10c) on the devbox @ train tip 4d841aa1c740bbaa03868dc0a403c62099a99a42 with the 72 sibling PRs of this batch: typecheck:core, file-size, complexity, cognitive-complexity, changelog-integrity green; changed-area node:test 831/831 (0 failing) and vitest 480/482 — the two reds are autoCombo/provider-family-combos.test.ts timing out at 20s, which reproduces on the PURE release tip under the full vitest suite (and is already tracked by the Release-Green issue #13866), so it is inherited, not this batch's. Merged --admin per merge-gates §3/§4/§7.

@diegosouzapw
diegosouzapw merged commit 6cb3402 into diegosouzapw:release/v3.8.51 Sep 22, 2026
7 of 16 checks passed
arashashrafii pushed a commit to arashashrafii/OmniRoute that referenced this pull request Oct 3, 2026
Removes src/shared/contracts/openai.ts and its test — the module was
never imported by any route/handler (verified via grep across
open-sse/, src/, tests/), so it added maintenance surface with zero
runtime effect. Fixes the two OPENAI_COMPATIBILITY.md doc-links that
pointed at the removed test file to point at existing coverage
instead (chat-completions-route-shape-gate.test.ts,
audio-transcription-handler.test.ts, audio-speech-handler.test.ts).

Also drops the src/app/api/v1/batches/route.ts sanitizeErrorMessage()
hunk from this PR: it duplicates the same Hard Rule diegosouzapw#12 fix already
proposed standalone in diegosouzapw#14115 and diegosouzapw#14145, so it is left for whichever
of those lands first rather than triple-implemented here.

The real fix in this PR — DSML tool-call normalization for
DeepSeek-shaped web providers in webTools.ts/deepseekWebTools.ts —
is unaffected and remains the mergeable core.

Co-authored-by: diegosouzapw <8016841+diegosouzapw@users.noreply.github.com>
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

fix(api): /v1/batches returns raw error.message in the response body (Hard Rule #12)

3 participants