Reject JSON-RPC requests with null id instead of misclassifying as notifications#2075
Conversation
…tifications When a JSON-RPC message contains "id": null, Pydantic's union resolution would silently reclassify it as a JSONRPCNotification (since the Request type correctly rejects null ids, but the Notification type absorbed it as an extra field). This violates the MCP spec which requires request ids to be strings or integers only. Add a model_validator on JSONRPCNotification that rejects any input containing an "id" field, ensuring that malformed requests with null ids produce a proper validation error instead of being silently swallowed. Fixes modelcontextprotocol#2057 Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
With dict[str, Any] type annotation, isinstance(data, dict) is redundant. Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>
clouatre
left a comment
There was a problem hiding this comment.
The model_validator approach is correct. I verified that ConfigDict(extra="forbid") would also reject unknown fields like {"foo": "bar"} on notifications, which is a broader contract change. The targeted validator is the right minimal fix.
Two things to tighten up:
Docstrings. Every other model in jsonrpc.py uses a single-line docstring. The expanded class docstring (6 lines) and validator docstring (4 lines) break that consistency. Also, :issue:\2057`is Sphinx cross-reference syntax, but this project has no Sphinx issue role configured, so it renders as literal text. Use#2057` or the full URL instead.
Test count. 12 tests for one behavioral change. 8 of them (test_request_rejects_null_id, test_valid_notification_still_works, test_valid_notification_with_params, test_valid_request_with_string_id, test_valid_request_with_int_id, test_message_adapter_parses_valid_request, test_message_adapter_parses_valid_notification, test_message_adapter_parses_notification_json) verify pre-existing behavior already covered by tests/test_types.py. The convention in tests/issues/ is 1-4 focused tests per issue, as module-level functions.
Suggested test set (4 tests, module-level functions):
test_notification_rejects_id_field(core fix)test_notification_rejects_any_id_value(parameterized edge cases)test_message_adapter_rejects_null_id(the actual bug path)test_message_adapter_rejects_null_id_json(transport path)
| class JSONRPCNotification(BaseModel): | ||
| """A JSON-RPC notification which does not expect a response.""" | ||
| """A JSON-RPC notification which does not expect a response. | ||
|
|
There was a problem hiding this comment.
Every other model in this file uses a single-line docstring (JSONRPCRequest, JSONRPCResponse, JSONRPCError). Consider trimming back to:
"""A JSON-RPC notification which does not expect a response."""The spec rationale fits better as a short comment above the validator. Also, :issue:\2057`won't render as a link (no Sphinx issue role configured). Use#2057` or the full GitHub URL.
| @classmethod | ||
| def _reject_id_field(cls, data: dict[str, Any]) -> dict[str, Any]: | ||
| """Reject messages that contain an ``id`` field. | ||
|
|
There was a problem hiding this comment.
The validator logic is clean and correct. The docstring could be a one-liner to match the file's style:
"""Reject payloads containing an 'id' field (notifications must not have one)."""|
|
||
|
|
||
| class TestNullIdRejection: | ||
| """Verify that ``"id": null`` is never silently absorbed.""" |
There was a problem hiding this comment.
All 18 other files in tests/issues/ use module-level test functions. This is the only one using a test class. Switching to module-level functions would match the convention.
| """Verify that ``"id": null`` is never silently absorbed.""" | ||
|
|
||
| def test_request_rejects_null_id(self) -> None: | ||
| """JSONRPCRequest correctly rejects null id.""" |
There was a problem hiding this comment.
This test and the 7 others below it (test_valid_notification_still_works, test_valid_notification_with_params, test_valid_request_with_string_id, test_valid_request_with_int_id, test_message_adapter_parses_valid_request, test_message_adapter_parses_valid_notification, test_message_adapter_parses_notification_json) verify pre-existing behavior already covered by tests/test_types.py::test_jsonrpc_request and the broader suite.
The tests/issues/ convention is to test the specific bug and its fix. I'd keep 4 tests:
test_notification_rejects_id_field(core fix)test_notification_rejects_any_id_value(parameterized edge cases)test_message_adapter_rejects_null_id(actual bug path)test_message_adapter_rejects_null_id_json(transport path)
Summary
Fixes #2057 — prevents
"id": nullJSON-RPC requests from being silently misclassified as notifications.Root cause
When Pydantic validates a
JSONRPCMessageunion with{"jsonrpc": "2.0", "method": "initialize", "id": null}:JSONRPCRequestcorrectly rejects it (RequestId=int | str, noNone)JSONRPCNotificationabsorbs it because"id": Nonebecomes an extra fieldThis violates the MCP spec (§4.2.1) which mandates that request IDs must be strings or integers, and requests with
id: nullmust be rejected.Fix
Add a
model_validator(mode="before")onJSONRPCNotificationthat rejects any input containing an"id"field. Per JSON-RPC 2.0, a notification is defined as a request without anidmember — if"id"is present (regardless of value), it's a request, not a notification.Changes
src/mcp/types/jsonrpc.py: Add_reject_id_fieldmodel validator toJSONRPCNotificationtests/issues/test_2057_null_id_rejected.py: 12 test cases covering:JSONRPCRequest,JSONRPCNotification, andJSONRPCMessageunion levelsvalidate_pythonandvalidate_jsonpaths (matching transport usage)Test plan
JSONRPCRequeststill rejectsid: null(existing behavior preserved)JSONRPCNotificationnow rejects any input with"id"fieldjsonrpc_message_adapter.validate_python({"id": null, ...})raisesValidationErrorjsonrpc_message_adapter.validate_json(...)raisesValidationErroridfield) still workid) still workruff checkpasses🤖 Generated with Claude Code