fix: skip outputSchema validation when isError is true#1947
Closed
Zelys-DFKH wants to merge 43 commits intomodelcontextprotocol:mainfrom
Closed
fix: skip outputSchema validation when isError is true#1947Zelys-DFKH wants to merge 43 commits intomodelcontextprotocol:mainfrom
Zelys-DFKH wants to merge 43 commits intomodelcontextprotocol:mainfrom
Conversation
Co-authored-by: Claude <noreply@anthropic.com> Co-authored-by: Felix Weinberger <3823880+felixweinberger@users.noreply.github.com>
…n tools are present (modelcontextprotocol#1407) Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
…1.x backport) (modelcontextprotocol#1382) Co-authored-by: Konstantin Konstantinov <KKonstantinov@users.noreply.github.com>
* fix: add transport isolation guards to prevent cross-client data leaks When a single McpServer or stateless transport is reused across multiple client connections, responses can be routed to the wrong client due to message ID collisions. This is a data leak vulnerability (CWE-362). Two guards added: - Protocol.connect() throws if already connected to a transport - Stateless transport.handleRequest() throws if called more than once Also fixes three examples that shared a single McpServer across sessions: - standaloneSseWithGetStreamableHttp.ts - ssePollingExample.ts - elicitationFormExample.ts Related: modelcontextprotocol#820, modelcontextprotocol#204, modelcontextprotocol#243 Co-Authored-By: Claude Opus 4.5 <noreply@anthropic.com> * fix: correct misleading test name and add per-request cleanup - Rename 'should reject second SSE stream even in stateless mode' to 'should allow multiple SSE streams in stateless mode with per-request transports' since the test now asserts both streams succeed (status 200) - Add mcpServer.close() after per-request handling in stateManagementStreamableHttp.test.ts to prevent resource leaks, matching the pattern used in streamableHttp.test.ts - Remove unused mcpServers array that collected but never cleaned up per-request server instances * thread abort through sendRequest and notification to prevent crosstalk * test: remove dummy objects from stateless setupServer return In stateless mode, setupServer() was creating unused McpServer and StreamableHTTPServerTransport instances solely to satisfy the return type. Make mcpServer/serverTransport optional in the return type and simplify the stateless beforeEach/afterEach to only track server and baseUrl. * fix: use McpError for sendRequest abort guard and fix Hono example reuse - sendRequest abort guard now throws McpError(ErrorCode.ConnectionClosed) instead of plain Error for consistency with the rest of the codebase - Hono example updated to create fresh transport and server per request, fixing breakage from the stateless transport reuse guard * fix: use separate resolvers per protocol in transport isolation test Address review feedback: each protocol handler now has its own resolver returning distinct data (responseForA vs responseForB), so the test properly demonstrates that responses route to the correct transport. Uses explicit 'handler entered' promises for deterministic synchronization. --------- Co-authored-by: Claude Opus 4.5 <noreply@anthropic.com>
…col#1518) Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Paul Carleton <paulcarletonjr@gmail.com>
…#1353) Co-authored-by: Matt <77928207+mattzcarey@users.noreply.github.com>
modelcontextprotocol#1528) Co-authored-by: Konstantin Konstantinov <KKonstantinov@users.noreply.github.com>
…xtprotocol#1580) Co-authored-by: Felix Weinberger <fweinberger@anthropic.com>
🏠 Remote-Dev: homespace
…odelcontextprotocol#580) (modelcontextprotocol#757) Co-authored-by: Paul Carleton <paulc@anthropic.com>
…n_endpoint_auth_methods_supported (modelcontextprotocol#1611) Co-authored-by: Basil Hosmer <basil@anthropic.com> Co-authored-by: Matt <77928207+mattzcarey@users.noreply.github.com>
…textprotocol#1596) Co-authored-by: Matt <77928207+mattzcarey@users.noreply.github.com>
…roller cleanup (modelcontextprotocol#1462) Co-authored-by: Felix Weinberger <fweinberger@anthropic.com>
…col#1339) Co-authored-by: Konstantin Konstantinov <KKonstantinov@users.noreply.github.com>
…tocol#1575) Co-authored-by: Claude Opus 4.6 <noreply@anthropic.com> Co-authored-by: Felix Weinberger <3823880+felixweinberger@users.noreply.github.com>
…odelcontextprotocol#1640) Co-authored-by: JiangNan <1394485448@qq.com> Co-authored-by: Felix Weinberger <fweinberger@anthropic.com> Co-authored-by: Konstantin Konstantinov <KKonstantinov@users.noreply.github.com>
…col#1503) Co-authored-by: mozmo15 <mozmo@mail.com> Co-authored-by: Felix Weinberger <fweinberger@anthropic.com>
…ity registration Backport of modelcontextprotocol#1513 to v1.x. The three lazy-init methods in McpServer (setToolRequestHandlers, setResourceRequestHandlers, setPromptRequestHandlers) unconditionally registered listChanged: true when called, overwriting any explicit listChanged: false passed in capabilities at construction time. Fixed by reading the current capability value via Server.getCapabilities() and applying it with nullish coalescing so that undefined still defaults to true (backwards compatible), but explicit false is preserved. Also made Server.getCapabilities() public to allow McpServer to read it. Fixes modelcontextprotocol#1819
callTool() and callToolStream() both ran outputSchema validation on structuredContent even when the result had isError: true. The comment on the condition already said "not when there's an error" — the !result.isError guard was just missing. Fixes modelcontextprotocol#1943
🦋 Changeset detectedLatest commit: d5e4923 The changes in this PR will be included in the next version bump. Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Author
|
Closing in favor of #1945, which covers the same fix and beat me to it by a few hours. Good luck with the review. |
9 tasks
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
20 participants
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.
Summary
Fixes #1943.
callTool()andcallToolStream()both validatestructuredContentagainst the tool's
outputSchemaeven when the result hasisError: true.The comment already says "not when there's an error" — the
!result.isErrorguard was just missing.
Changes
src/client/index.tsline 760:if (result.structuredContent)→if (result.structuredContent && !result.isError)src/experimental/tasks/client.tsline 125: same change for thestreaming path
Tests
should skip outputSchema validation when isError is trueto theexisting
outputSchema validationsuite: verifiescallTool()returnsthe error payload with schema-violating
structuredContentwithout throwing.callToolStream() should skip outputSchema validation when isError is true and structuredContent is invalid. The pre-existing stream testonly covered
isErrorwithoutstructuredContent, which never triggeredthe bug.
Both new tests fail on the pre-fix code and pass after the fix.