ctx7-2586 sanitize error responses for rejected request bodies

[ramazanoacar]

Problem

express.json() sits ahead of the routes, so a rejected body throws inside
body-parser and calls next(err). With no error handler registered, the
request skipped CORS, both /mcp routes (including the /mcp/oauth auth
check) and the catch-all 404, and fell into Express's default handler.

Before

$ curl -si -X POST localhost:3000/mcp -H 'Content-Type: application/json' -d '{"jsonrpc":"2.0","method":'
HTTP/1.1 400 Bad Request
Content-Type: text/html; charset=utf-8

<pre>SyntaxError: Unexpected end of JSON input<br>
  at parse (/app/node_modules/.pnpm/body-parser@2.2.0/node_modules/body-parser/lib/types/json.js:77:19)<br>
  at /app/node_modules/.pnpm/raw-body@3.0.2/node_modules/raw-body/index.js:123:18<br> ... </pre>

Leaks absolute paths, dependency versions and the parser's message. Because the
auth check is skipped too, an unauthenticated caller gets the same trace from
/mcp/oauth.

After

HTTP/1.1 400 Bad Request
Content-Type: application/json; charset=utf-8
Access-Control-Allow-Origin: *

{"jsonrpc":"2.0","error":{"code":-32700,"message":"Parse error"},"id":null}

Identical on /mcp and /mcp/oauth.

Changes

A terminal error handler after the catch-all 404:

Case	Response
entity.parse.failed	400 -32700 "Parse error"
Other body-parser 4xx (e.g. entity.too.large)	its own status, -32600 "Invalid Request"
Anything else	500 -32603

Messages are fixed strings, since body-parser's own text quotes the body. Only
entity.parse.failed counts as a parse error, so an unrelated SyntaxError
from application code is not reclassified. Keeping the middle tier's status
matters: reporting an oversized body as a 500 would invite the client to retry
a request that can never succeed.

CORS moved above express.json() so error responses carry
Access-Control-Allow-Origin — previously they did not, and browser clients
saw a CORS failure instead of the status. Preflight still returns 200.

Logs carry method, path, content-length and body-parser's error tag only —
never the body or the headers, which hold the API key.

Preserved

Valid JSON (-32600), empty body (-32600), wrong Content-Type
(415 -32000), /mcp/oauth without credentials (401 -32001), /ping,
unknown routes and OPTIONS are all unchanged. Empty bodies are worth noting:
body-parser treats them as {} rather than a parse failure.

Tests

test/integration.test.ts, on the existing built-binary harness so the
assertions cover the real middleware order.

• Malformed body on both endpoints: 400, JSON content type, CORS header, and no
  stack trace, HTML, path, dependency name, parser message, submitted body or
  credential in the payload.
• An oversized body stays a sanitized 413.
• A non-JSON-RPC object and an empty body keep -32600.

Confirmed failing before the fix, passing after.

Verification

pnpm --filter @upstash/context7-mcp build
pnpm --filter @upstash/context7-mcp typecheck
pnpm --filter @upstash/context7-mcp lint:check
pnpm --filter @upstash/context7-mcp format:check
pnpm --filter @upstash/context7-mcp test     # 64 passed

[linear-code]

CTX7-2586

[fahreddinozcan]

rejectedBodyStatus accepts every error carrying a 4xx status, not specifically body-parser errors. A future route error such as a deliberate 401/403 would therefore be mislabeled as a rejected body and converted to JSON-RPC Invalid Request. Can we keep the body parser and its error boundary adjacent (or explicitly identify parser error types), then leave a separate generic handler for application errors?

[fahreddinozcan]

Returning when res.headersSent silently consumes the error. If a route or stream fails after starting its response, the connection can remain open indefinitely. Please accept next and use return next(err) here so Express’s default handler can close the connection.

[fahreddinozcan]

The reported malformed-JSON leak is fixed and runtime validation passes (local server exercised; 64/64 MCP tests, typecheck, lint, and formatting all pass). I am requesting changes for the two inline correctness/boundary issues: scope rejected-body handling to errors originating from express.json() instead of classifying every 4xx application error as a body rejection, and delegate with next(err) when headers have already been sent. The clean structure is a parser-local error boundary plus a small generic terminal 500 handler.

[fahreddinozcan]

This fixes a globally mounted parser by introducing globally mounted JSON-RPC error handling, but JSON-RPC is only owned by /mcp. The other current endpoints do not consume JSON bodies and should neither parse them nor return MCP error envelopes.

Can we make the ownership boundary explicit by extracting the handler to something like src/lib/mcp-body-error-handler.ts and mounting both it and express.json() on an MCP router?

import { mcpBodyErrorHandler } from "./lib/mcp-body-error-handler.js";

const mcpRouter = express.Router();
mcpRouter.use(express.json());
mcpRouter.use(mcpBodyErrorHandler);
mcpRouter.all("/", (req, res) => handleMcpRequest(req, res, false));
mcpRouter.all("/oauth", (req, res) => handleMcpRequest(req, res, true));
app.use("/mcp", mcpRouter);

This addresses the pentest across the whole application: MCP requests receive sanitized JSON-RPC parser errors, while health, discovery, challenge, and unknown routes never unnecessarily enter the JSON parser. It also keeps the parser/error mechanics out of an already busy index.ts.

handleMcpRequest already sanitizes downstream MCP failures, so the application-wide terminalErrorHandler appears unnecessary. Please also add a regression assertion that malformed JSON sent to a non-MCP path follows that route's normal contract rather than returning JSON-RPC.

[fahreddinozcan]

Re-reviewed the latest head. The JSON parser and dedicated error handler are now scoped to mcpRouter, keeping the behavior isolated to /mcp while leaving the rest of the app unchanged. I also validated the malformed-body behavior against the local server and ran the package checks/tests successfully. This looks good now.

[ramazanoacar]

Merged master (#3028, Claude Code plugin auth) into this branch. One line of
that change needed adjusting to work with the MCP router introduced here.

requiresAuthentication gates /mcp/oauth by checking req.path. On master
the MCP routes sit directly on the app, so req.path is /mcp/oauth. In this
branch they live on a router mounted at /mcp, and Express makes req.path
relative to the mount point — it becomes /oauth, the comparison silently
fails, and /mcp/oauth becomes anonymous.

Neither side is wrong on its own; the combination is. Git auto-merged the region
without flagging it since the two changes touch different lines.

Fix is to compare the mount-aware path, which is correct under both layouts:

return `${req.baseUrl}${req.path}` === "/mcp/oauth" || Boolean(plugin);

@fahreddinozcan