refactor: validate JSON-RPC at the MCP/HTTP boundary#897
Conversation
Wire the previously dead jsonRpcRequestSchema into the MCP stdio decode path and add a sibling commandRpcParamsSchema for the daemon HTTP command params, replacing unchecked casts of attacker-controllable wire input with real boundary parsing. - mcp/server.ts: each inbound payload (and each batch element) is parsed via jsonRpcRequestSchema instead of being force-cast to JsonRpcMessage. Valid requests (with id), notifications (no id), and batches are accepted and routed exactly as before; only genuinely malformed input (non-object, or wrong-typed jsonrpc/method/id) is rejected with the standard -32600 Invalid Request error. JSON.parse failures still surface as -32700, and malformed input never crashes. - contracts.ts: add commandRpcParamsSchema next to jsonRpcRequestSchema validating the command RPC params; optionalStringArray helper added. - daemon/http-server.ts: replace the 'params as unknown as Partial<DaemonRequest>' double-cast with commandRpcParamsSchema.parse(params).
Size Report
Startup median (7 runs, lower is better):
Top changed chunks:
|
thymikee
left a comment
thymikee
left a comment
There was a problem hiding this comment.
Focused, well-scoped boundary-validation refactor. I traced both production paths.
MCP stdio path — verified behavior-equivalent for all valid traffic and a net improvement on malformed input: requests, no-id notifications, explicit-null id, and per-element batch routing all preserved (router's existing -32600 check is unchanged; id===undefined notification distinction kept). The reconstructed {jsonrpc,id,method,params} envelope drops only unknown top-level fields, which nothing downstream reads, and params is passed through untyped. The null-payload crash (null.jsonrpc / fallbackErrorId(null)) is genuinely fixed. New tests exercise the real router (no mocks); ran them locally, 8 passed.
One finding on the HTTP path below.
| ): DaemonRequest { | ||
| if (COMMAND_RPC_METHODS.has(method)) { | ||
| return toDaemonRequest(params as unknown as Partial<DaemonRequest>, headers); | ||
| return toDaemonRequest(commandRpcParamsSchema.parse(params), headers); |
There was a problem hiding this comment.
Schema-parse failures here surface as JSON-RPC -32000 / HTTP 500 (server error), not -32602 / 400. commandRpcParamsSchema.parse throws a plain Error, which normalizeError maps to code UNKNOWN → statusCodeForNormalizedError → 500, and the wire message leaks the internal schema path. Confirmed locally:
{command:'tap',positionals:'x'} -> UNKNOWN "$.positionals: Expected an array"
{command:123} -> UNKNOWN "$.command: Expected a string"
[...] -> UNKNOWN "$: Expected an object"
This is inconsistent with the sibling checks just above (:565, :581 return -32602/400) and with every other param validator in this file, which throw AppError('INVALID_ARGS', …) → 400. It's also a real behavior change for the attacker-input path this PR targets: a non-array positionals was previously coerced to [] (request proceeded), and a non-string command returned -32602/400 — both now become 500s. Reporting malformed client input as a server error is the wrong signal for alerting/retry.
Suggest wrapping the parse so it throws AppError('INVALID_ARGS', …) (→ 400, with a clean message), ideally surfaced as -32602 to match :565/:581.
methodToDaemonRequest validated command params via commandRpcParamsSchema.parse, which throws a plain Error on malformed input. That mapped to UNKNOWN -> HTTP 500 / -32000 and leaked the internal '$.x' schema path on the wire. Reporting malformed client input as a server error is the wrong signal.
Wrap the parse so failures throw AppError('INVALID_ARGS', 'Invalid params: ...') with the schema path sigil stripped, and map normalized INVALID_ARGS to JSON-RPC -32602 in the boundary catch so malformed command params now surface as 400/-32602, matching the explicit sibling checks.
|
Thanks for the catch @thymikee. Fixed in 38f45cb: |
|
Re-review passed — the prior HTTP finding is fully resolved; merge-ready from my side. Verified in
No regressions found:
Local: targeted + mcp/contracts/http-contract suites green (37 tests), Residual risk: low. One pre-existing nit (not introduced here): |
|
1 participant
What
Replace unchecked casts of attacker-controllable JSON-RPC wire input with real boundary validation, wiring in the
jsonRpcRequestSchemathat already existed incontracts.tsbut was dead code.src/mcp/server.ts— the stdio decode path no longer force-castsJSON.parse(raw)toJsonRpcMessage[]/JsonRpcMessage. Each inbound payload (and each batch element) is parsed throughjsonRpcRequestSchemabefore routing. The decoder now forwards the untyped parsed value andhandleMcpPayloadvalidates at the boundary.src/contracts.ts— addcommandRpcParamsSchema(and anoptionalStringArrayhelper) next tojsonRpcRequestSchemato validate theparamsof a command JSON-RPC request.src/daemon/http-server.ts— replace theparams as unknown as Partial<DaemonRequest>double-cast inmethodToDaemonRequestwithcommandRpcParamsSchema.parse(params).Why
These were the spots where untrusted wire input was force-cast straight into typed objects. Parsing at the boundary turns a silently-mistyped object into a proper validated envelope (or a clean JSON-RPC error), and revives a schema that was otherwise unused.
Preserves all valid requests / notifications / batches
jsonRpcRequestSchemadoes not mandate anid, so:id) route exactly as before.id) are accepted and still produce no response (id === undefinedis preserved distinctly fromid: null).Only genuinely malformed input is rejected:
method/jsonrpc/idor a non-object payload yields the standard-32600Invalid Request error (best-effortidpreserved), matching today's router error path, instead of crashing.JSON.parsefailures still surface as-32700.The HTTP command path stays behaviorless for every valid command request; only malformed params (e.g. non-string
command, non-arraypositionals) are now rejected at the boundary instead of being passed through untyped.Validation
All run from the worktree:
node_modules/.bin/tsc -p tsconfig.json --noEmit— cleanoxfmt --writeon the changed filesoxlint <changed> --deny-warnings— cleanvitest run src/mcp— 16 passed (addssrc/mcp/__tests__/server-validation.test.tsproving valid request/notification/batch routing and-32600rejection of malformed input)vitest run contractsandsrc/__tests__/contracts-schema-public.test.ts— passedvitest runonhttp-contract,runtime-hints,request-router-opendaemon suites — passednode scripts/sync-mcp-metadata.mjs --check— passed unchanged