| FazBrowse GitHub Viewer | Trending | | Home |
| Tools: [Download Repo ZIP] [Original HTTPS Page] |
⚠️ No Changeset foundLatest commit: 9981dc5 Merging this PR will not cause a version bump for any packages. If these changes should not result in a new version, you're good to go. If these changes should result in a version bump, you need to add a changeset. Click here to learn what changesets are, and how to add one. Click here if you're a maintainer who wants to add a changeset to this PR |
Sorry, something went wrong.
npm i https://pkg.pr.new/@modelcontextprotocol/sdk@2239 commit: 9981dc5 |
Sorry, something went wrong.
There was a problem hiding this comment.
Thanks — the latest commit addresses my earlier concern by plumbing maxBufferSize through StdioServerParameters and the StdioServerTransport constructor, and I didn't find any new bugs. I'm leaving this for a human since the default 10 MB cap is still a behavior change for existing users with large single-message payloads (they'll now see the transport error and close unless they opt into a higher limit), so a maintainer should confirm the default and whether this needs a changeset/breaking-change note.
Extended reasoning...This PR adds a size limit to the stdio ReadBuffer (src/shared/stdio.ts) with a 10 MB default, surfaces overflow as an onerror + transport close in both StdioClientTransport and StdioServerTransport, and exposes a maxBufferSize option via StdioServerParameters and a new options parameter on the server transport constructor. Tests cover the shared buffer behavior and both transports' overflow paths.
My previous review flagged that maxBufferSize was dead code because neither transport passed it through. The follow-up commit (9981dc5) addresses this: the client transport now constructs new ReadBuffer({ maxBufferSize: server.maxBufferSize }) and the server transport accepts an options object. The implementation looks correct — processReadBuffer() drains complete messages after each append, so the cap effectively bounds a single in-flight message line, and overflow clears the buffer before throwing.
The change is defensive in nature (bounds unbounded memory growth from a misbehaving or malicious peer writing a huge line without a newline). No injection, auth, or data-exposure surface is introduced. The main risk is availability/compatibility, not security: a legitimate >10 MB single JSON-RPC message (e.g. a large base64 tool result) that previously worked will now tear down the transport unless the consumer raises the limit.
This touches the core stdio transport of a widely used SDK and changes default runtime behavior for existing users, so it warrants maintainer-level scrutiny even though the diff is small and the implementation is sound. The choice of the 10 MB default, whether the failure mode (close the whole transport vs. drop the message) is the desired policy, and whether this should ship with a changeset / be called out as a breaking change are product decisions for a human maintainer — the PR description and changeset are currently empty.
Test coverage for the new behavior is good across shared, client, and server layers. One minor note: the limit is checked against the total buffered size before draining, so a single data chunk containing several complete messages that together exceed the cap would also trip it — unlikely with real pipe chunk sizes, but worth being aware of.
Sorry, something went wrong.
There was a problem hiding this comment.
lgtm
Sorry, something went wrong.
Ported functional changes from upstream v1.x (17 commits behind, v1.28.0 through v1.29.0+). Only retained-fork-surface changes applied; auth, HTTP, and CI-only commits skipped. Changes ported: - stdio: ReadBuffer maxBufferSize limit (modelcontextprotocol#2239) - stdio: windowsHide always on Win32, removed isElectron() (modelcontextprotocol#1640) - stdio: try/catch error handling on read buffer overflow - types: extensions capability on ClientCapabilities/ServerCapabilities (modelcontextprotocol#1811) - types: size field on ResourceSchema (modelcontextprotocol#1575) - zod-compat: prioritize zod issues with path formatting (modelcontextprotocol#1503) - protocol: conditional abort controller cleanup (modelcontextprotocol#1462) Intentionally kept: - AuthInfo import from ./shared/auth-info.js (not reverted to auth/types.js) - All removed transports, auth, and HTTP modules stay out
| Back | FazBrowse Home | New Git URL |
Motivation and Context
How Has This Been Tested?
Breaking Changes
Types of changes
Checklist
Additional context