Repository navigation
Reject subscriptions/listen without notifications - #1208
Open
subhashpolisetti wants to merge 1 commit into
Open
subhashpolisetti wants to merge 1 commit into
subhashpolisetti wants to merge 1 commit into
Conversation
notifications is required in SubscriptionsListenRequestParams. Check the raw params before decoding, as CompletionsFeature does, and fail with -32602 instead of opening an empty subscription. An explicit empty filter is still accepted. Fixes modelcontextprotocol#1196
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
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 #1196
Motivation and Context
notificationsis a required field ofSubscriptionsListenRequestParams. When a client sendssubscriptions/listenwithout it, or with"notifications": null,SubscriptionsFeature#handlefalls back toSubscriptionFilter.EMPTY, acknowledges the subscription and keeps an idle stream open.SubscriptionsListenRequest.fromJsonmakes the same substitution, so the missing field can no longer be detected after decoding.This change checks the raw params before decoding and fails with
McpException.invalidParams("params.notifications is required"), the same wayCompletionsFeature#handlechecksargument, which theCompleteRequestJSON creator also fills in when it is missing.HttpServletMcpTransportalready maps-32602to400. An explicit empty filter ({}) is still accepted.How Has This Been Tested?
Added three tests to
SubscriptionsFeatureTests:listenWithoutNotificationsIsRejectedAsInvalidParamslistenWithNullNotificationsIsRejectedAsInvalidParamslistenWithEmptyNotificationsIsAcceptedThe first two fail on
main(the server returns a streaming response) and pass with this change. The third passes on both and keeps{}valid.Those tests use
GsonMcpJsonMapper, like the rest of the file, so I also addedlistenWithoutNotificationsIsInvalidParamstoHttpServletMcpTransportIntegrationTests. It goes through the servlet transport with the default Jackson mapper, whereSubscriptionsListenRequest.fromJsonwould fill in the missing filter, and checks the400/-32602response. It fails onmain(status200) and passes with this change on both the Jackson 3 and-Pjackson2builds. It reads the body as a stream, so a regression fails on the status instead of hanging on an open subscription.SubscriptionsFeatureTests: 11 run, 0 failures.HttpServletMcpTransportIntegrationTests: 24 run, 0 failures.clean verify: BUILD SUCCESS on all modules (mcp-core 603, mcp-json-jackson3 45, mcp-json-jackson2 45, mcp-test 1250; 0 failures, 0 errors, 1 skipped by an existingassumeTrueinStdioMcpAsyncClientTests). The-Pjackson2run passes with the same counts.Breaking Changes
None for the 2025-11-25 API. On the 2026-07-28 server (first shipped in 2.2.0-M1), a listen request without
notificationsis now rejected with-32602instead of opening an empty subscription, as the spec and #1196 describe.Types of changes
Checklist
Additional context
I kept
SubscriptionsListenRequest.fromJsonlenient and put the check in the feature, asCompletionsFeaturedoes forCompleteRequest, so the record's deserialization is unchanged. Non-object values ("",true,[]) were already rejected as-32602by the existing "Malformed SubscriptionsListenRequest" path; I checked that on the Jackson 3, Jackson 2 and Gson mappers.Happy to adjust, or close if you'd rather fold this into the other 2.2.0-M2 work.
I used Claude Code to explore the codebase and draft the fix and tests; I reviewed the change and ran the tests myself.