[Security] Bound MCP stdio framing buffers#258
Conversation
|
@0xSero Ready for maintainer review. Exact head: This revision preserves the official MCP SDK client and HTTP transport while bounding owned stdio framing, retaining the separate bootstrap limit, and making terminal settlement and process-tree cleanup deterministic. Two independent exact-head P0/P1 reviews found no issues. Validation is green: focused suites 49/49 with 490 assertions, independent five-repeat stress 245/245, complete agent runtime 138/138 with 707 assertions plus TypeScript and Windows-helper verification, full Exact-head workflow records: CI run 29735798436 and Security run 29735798320. Both are |
d577ca9 to
070dd1b
Compare
Summary
Root cause
The stdio connector accumulated decoded text without a size bound until a newline arrived. A faulty or hostile child could therefore retain arbitrary stdout, force repeated buffer copies, and defer request failure until unrelated request timeouts. Decoding before a complete frame also made split multi-byte UTF-8 and exact byte limits difficult to enforce reliably.
Transport termination was spread across stream and process callbacks. That made it possible for pending and future operations to observe different errors, for listeners or timers to outlive the connection, and for descendants holding stdout open to delay cleanup indefinitely.
Solution design
Bounded byte framing
StdioJsonLineFramerretains raw bytes, grows capacity only as needed, emits every complete newline-delimited frame, and rejects a frame above 4 MiB before unbounded retention can occur. It uses fatal UTF-8 decoding after frame completion, supports split code points and multiple frames per chunk, and releases retained data on terminal failure.Bootstrap remains independently capped at 64 KiB. The framer changes to the 4 MiB runtime limit only while empty, and bytes following the initialize response in the same stdout chunk continue through the runtime parser without loss.
SDK-compatible protocol handling
The existing official SDK
ClientandStreamableHTTPClientTransportremain authoritative. Stdio messages are validated with the SDK's canonicalJSONRPCMessageSchemaand forwarded through the transport'sonmessagecallback, preserving SDK handling for responses, notifications, server requests, request timeouts, and abort signals.Deterministic terminal lifecycle
A single terminal-failure state rejects pending and future operations with the same error object, notifies the SDK promptly, and closes the owned transport once. Stable stdout, stdin, stderr, child-process, exit, and close listeners are removed exactly once.
Parent exit starts a 100 ms stdout drain so an already-piped final response can settle. Clean or partial EOF from a live child becomes terminal, partial frames produce a typed unexpected-EOF error, and descendants that retain stdout are bounded by the drain before POSIX process-group or Windows job cleanup completes. Pre-spawn failures never signal a nonexistent process group, and explicit close remains idempotent.
Acceptance criteria
Validation
Validated at exact head
070dd1bb35905c4cb7badb407951541d676277dc, whose direct parent is the reviewed #257 head01ac7ff18f1097da281f4603e77d751568e130be.npm run checkaction_requiredwith zero jobs pending upstream approval for fork-authored workflowsdesktop:distwith embedded runtime, package-signature, and stdio MCP verification/Applications/Local Studio.appreinstall andGET /api/desktop-healthreturning HTTP 200 with{ "ok": true }Dependency
Depends on #257. This branch is based directly on its exact reviewed head because its official GitHub MCP artifact and plugin bootstrap boundaries are part of the compatibility surface exercised here.
Closes #238