Repository navigation
[v1.x] fix(protocol): answer -32602 for requests whose params fail the schema - #2932
Open
Gauravtiwari31 wants to merge 1 commit into
Open
Gauravtiwari31 wants to merge 1 commit into
Gauravtiwari31 wants to merge 1 commit into
Conversation
…e schema Protocol.setRequestHandler parsed the request with parseWithCompat, which throws the raw ZodError. It has no numeric code, so _onrequest answered -32603 Internal error with the Zod issue dump as the message. Parse with safeParse instead and throw McpError(InvalidParams) on failure, so callers get -32602 "Invalid params for <method>: ...". This also makes the existing InvalidParams guards in the Client and Server wrappers (elicitation, sampling, tools/call) reachable again, since they run after this parse. Drops the three e2e knownFailures that pinned the old behaviour, and lets the mcpserver logging/setLevel scenario send its invalid level past the wire sniffer, as it already does on main. Backport of the fix for modelcontextprotocol#2916 / modelcontextprotocol#2284.
🦋 Changeset detectedLatest commit: 03a2264 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 |
commit: |
This branch has not been deployed
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.
Part of #2916 (same bug as #2284). This is the v1.x side; the main-branch fix is in #2492, and the triage on #2284 noted v1.x needs it too.
On v1.x,
Protocol.setRequestHandlerparses the request withparseWithCompat, which throws the rawZodError. That error has no numericcode, so_onrequestanswers-32603Internal error with the Zod issue dump as the message. It's a bit wider than on main, because this parse runs before the role-specific guards. Eventools/callwith a badnamecomes back as -32603, and theInvalidParamschecks in the Client (elicitation, sampling) and Server (tools/call) wrappers are never reached.The fix uses
safeParseand throwsMcpError(ErrorCode.InvalidParams, "Invalid params for <method>: ..."). The message is built withgetParseErrorMessage, the same helper McpServer uses for its argument errors, so it reads likeInvalid option: expected one of ... at params.levelrather than a JSON dump.Over Streamable HTTP against an
McpServer, with a well-formed control for each method:Tests
test/shared/protocol.test.tsforprompts/getwith{}andlogging/setLevelwith{ level: 'loud' }. It fails before the fix and passes after.knownFailuresthat pinned the old behaviour (protocol:error:invalid-params,logging:set-level:invalid-level,elicitation:form:schema:restricted-subset). Those 13 cells now pass.logging:set-level:invalid-levelscenario now passes{ strictValidation: false }, as it does on main. Without it the wire sniffer drops the invalid level before it reaches the server.npm test(1843 passed), typecheck and eslint are clean.npm run test:e2epasses excepttransport:stdio:shutdown-escalation, which also fails on a clean v1.x on my Windows machine.