fix(server): reject duplicate in-flight request ids in streamable HTTP#2434
Open
Sammy-Dabbas wants to merge 1 commit into
Open
fix(server): reject duplicate in-flight request ids in streamable HTTP#2434Sammy-Dabbas wants to merge 1 commit into
Sammy-Dabbas wants to merge 1 commit into
Conversation
The transport registered every request via _requestToStreamMapping.set(message.id, streamId) with no duplicate check, so a concurrent POST reusing an in-flight id overwrote the entry, cross-wired the first request's response onto the second POST's stream, and left the first POST hanging. Reject a POST containing a request whose id is already in flight, or duplicated within the same batch, with HTTP 400 and JSON-RPC -32600. Sequential reuse after completion stays allowed since deployed clients send a constant id for every request. Cancelled requests never produce a response, so notifications/cancelled now retires the transport bookkeeping for its target id; without that, a cancelled id would stay in flight forever and lock out cancel-then-reuse clients. Adds the missing isCancelledNotification guard to core-internal alongside the existing notification guards. Fixes modelcontextprotocol#2433.
|
@modelcontextprotocol/client
@modelcontextprotocol/codemod
@modelcontextprotocol/core
@modelcontextprotocol/server
@modelcontextprotocol/server-legacy
@modelcontextprotocol/express
@modelcontextprotocol/fastify
@modelcontextprotocol/hono
@modelcontextprotocol/node
commit: |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
1 participant
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #2433.
The transport registers every request via
_requestToStreamMapping.set(message.id, streamId)with no duplicate check. A concurrent POST reusing an in-flight id overwrites the entry,send()routes the first request's response onto the second POST's stream, and the completion cleanup retires the id so the first POST hangs forever. Reproduced in SSE mode, JSON response mode, and within a single batch before the fix.This rejects a POST containing a request whose id is already in flight, or duplicated within the same batch, with HTTP 400 and JSON-RPC -32600, matching the transport's existing Invalid Request rejection pattern. Sequential reuse after completion stays allowed, since deployed clients send a constant id for every request. One necessary companion change: a cancelled request never produces a response (the protocol layer suppresses responses for aborted handlers), so
notifications/cancellednow retires the transport bookkeeping for its target id. Without that, a cancelled id would stay in flight forever and lock out cancel-then-reuse clients. That required adding the missingisCancelledNotificationguard to core-internal, following the existingisInitializedNotificationpattern.Two judgment calls worth flagging: the guard enforces uniqueness among in-flight ids rather than the spec's literal never-reused-in-a-session, since strict enforcement needs unbounded memory and would break clients that reuse a constant id. And rejection uses 400 with -32600 rather than a different status, to match how the transport already rejects invalid requests.
Tests: five new tests, of which three reproduce the bug pre-fix (SSE, JSON mode, within-batch) and two pin preserved behavior (sequential reuse, cancel-then-reuse). Transport file 52/52, server package 407/407, core-internal and node middleware suites pass, typecheck and lint clean.
This is the TypeScript side of the same defect just fixed in the python SDK (modelcontextprotocol/python-sdk#3063); the two guards use the same semantics for cross-SDK consistency.