Skip to content

fix: reject invalid JSON-RPC response versions - #1162

Open
Shubchynskyi wants to merge 1 commit into
modelcontextprotocol:mainfrom
Shubchynskyi:fix/1156-jsonrpc-response-version-author
Open

Shubchynskyi wants to merge 1 commit into
modelcontextprotocol:mainfrom
Shubchynskyi:fix/1156-jsonrpc-response-version-author

Conversation

@Shubchynskyi

Copy link
Copy Markdown

Reject success and error responses with an invalid JSON-RPC version. The client currently accepts otherwise valid responses with "jsonrpc":"1.0"; validating JSONRPC_VERSION.equals(jsonrpc) in the JSONRPCResponse compact constructor rejects them while preserving valid "2.0" responses and the existing empty-version diagnostic.

Fixes #1156

Motivation and Context

Merge dependency: please merge #1158 before this PR.

On current main, a deserialization exception stops StdioClientTransport's inbound loop. Once this validation rejects an invalid version, the affected tool call and a subsequent ping time out. That recovery problem is tracked in #1157. #1158 makes the malformed response fail its request immediately and keeps the transport reading, so this PR depends on #1158 for stdio recovery. This change adds only schema validation and regression tests; transport recovery stays in #1158.

Add two regression cases through McpSchema.deserializeJsonRpcMessage: an otherwise valid success response and an otherwise valid error response, each with "jsonrpc":"1.0". The tests check the validation root cause across Jackson exception wrappers. Existing positive assertions are unchanged.

How Has This Been Tested?

Locally on Debian 13 amd64, Temurin JDK 21.0.12.1+1, Maven wrapper 3.9.9, commit c32f385d58ada525d0b234542a7ca36f0100b3b0:

This PR uses commit 36215f5059ac9ab05e97940abcde92cb752cb432, with the same source tree and parent as the tested commit. Only commit metadata changed; the test results below apply to the identical source.

  • Before the fix: both new regression tests failed because deserialization threw no exception (2 failures, 0 errors).
  • After the fix: the targeted dispatch tests and both required full local runs passed:
Check Result
JsonRpcDispatchTests, Jackson 3 7/7 passed
JsonRpcDispatchTests, Jackson 2 7/7 passed
./mvnw clean test, default Jackson 3 profile BUILD SUCCESS, exit 0; 1,556 test executions
./mvnw -pl mcp-test -am -Pjackson2 test BUILD SUCCESS, exit 0; 1,511 test executions
Maven formatting validation and git diff --check Passed

Both full runs reported zero failures, errors and skipped tests. Counts above sum Maven's per-module summaries. The actual runs also set JVM proxy/CA options and appended -DsurefireArgLine='-javaagent:/workspace/issue-1156-review/runtime-network-agent.jar -Dissue1156.networkConfig=/workspace/issue-1156-review/runtime-network.properties' for this managed environment. The temporary adapter is outside the patch: it configures the proxy/CA for Testcontainers Node processes and makes Reactor Netty use JVM proxy settings. TLS verification stays enabled; SDK validation, fixtures and assertions are unchanged. Both mapper profiles were verified from the test JVM classpaths.

Remaining validation limits:

  • Both full runs emit the existing 30s Surefire fork shutdown warning, then finish with exit 0. The same warning was reproduced in a full mcp-test control run on unchanged main (1cf7903935ac6a99ea8920b4ce85a43e4f1d0689), with zero failures/errors and exit 0. Its exact cause remains unresolved.
  • The full default reactor also compiled all four conformance modules. They have no JUnit tests; actual conformance scenarios and JDK 17 runtime validation remain for CI.

Stdio dependency check: a temporary diagnostic used the locally built SDK. Before this fix, the invalid response was accepted and ping succeeded. With this fix and the current transport, the response was rejected but the tool call and next ping timed out. With this core and the transport from #1158 (4ca7e6be893fef25f1f0556ca05d531ba5222edc), the tool call failed immediately and the next ping succeeded. This diagnostic is outside the patch and is not a full test run of #1158.

Breaking Changes

Responses using a JSON-RPC version other than "2.0" now fail validation. Compliant responses continue to work; API shape and wire serialization are unchanged.

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)
  • Documentation update

Checklist

  • I have read the MCP Documentation
  • My code follows the repository's style guidelines
  • New and existing tests pass locally
  • I have added appropriate error handling
  • I have added or updated documentation as needed

No documentation change is needed for this validation fix.

Additional context

Supersedes #1161 with corrected commit attribution; source changes are identical.

Prepared with AI assistance; human review is requested before merging. The patch consists of two files and 24 added lines. Please retain the merge order #1158, then this PR to preserve stdio recovery.

This branch has not been deployed

No deployments
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Java client accepts responses with jsonrpc: "1.0"

1 participant