Skip to content

Commit 0d36610

Browse files
committed
fix: address second bughunter pass (8 findings)
Dispatcher: - notify omits the params key entirely when params is None; the spec requires absent over null and the TS SDK client rejects null. - transport_builder calls on the read-loop path are guarded: a raising builder answers the request with INTERNAL_ERROR (or drops the notification) instead of killing the dispatcher. - bool no longer matches the int() structural patterns (progressToken, cancelled requestId, progress), so true cannot alias to request id 1. The cancelled arm and the _in_flight table now coerce ids the same way responses do, so string/int id forms correlate both ways. - Dropped the redundant outer _in_flight pop in _handle_request; it could evict a newer request that reused the id. Resumption hints passed alongside related_request_id are dropped with a debug log and the precedence is documented. - notifications/cancelled now flows through to on_notify after the dispatcher applies its cancellation, so Server.middleware observes it and servers can register custom handling. Runner / peer: - dump_params serializes meta through RequestParams by alias (the inverse of _extract_meta), so a handler passing meta=ctx.meta emits progressToken on the wire, not progress_token. - A handler returning ErrorData now raises MCPError inside call_next() so middleware observes the failure; _dump_result keeps the conversion as a backstop for middleware that itself returns ErrorData. docs/migration.md: middleware example takes the raw mapping; remove the unreachable DispatchMiddleware pointer; fix the inverted raise_exceptions claim.
1 parent 0244bf9 commit 0d36610

9 files changed

Lines changed: 611 additions & 33 deletions

File tree

docs/migration.md

Lines changed: 5 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -835,16 +835,15 @@ server.add_notification_handler("notifications/custom", MyNotifyParams, my_notif
835835
These were private, but some users subclassed `Server` and overrode them to intercept requests. Use middleware instead:
836836

837837
```python
838+
from collections.abc import Mapping
838839
from typing import Any
839840

840-
from pydantic import BaseModel
841-
842841
from mcp.server import Server, ServerRequestContext
843842
from mcp.server.context import CallNext, HandlerResult
844843

845844

846845
async def logging_middleware(
847-
ctx: ServerRequestContext[Any, Any], method: str, params: BaseModel, call_next: CallNext
846+
ctx: ServerRequestContext[Any, Any], method: str, params: Mapping[str, Any] | None, call_next: CallNext
848847
) -> HandlerResult:
849848
print(f"handling {method}")
850849
result = await call_next()
@@ -856,11 +855,11 @@ server = Server("my-server", on_call_tool=...)
856855
server.middleware.append(logging_middleware)
857856
```
858857

859-
For lower-level interception (raw method/params before validation, including unknown methods), use `DispatchMiddleware` from `mcp.shared.dispatcher`.
858+
Middleware runs before params validation, so `params` is the raw inbound mapping (or `None`), and it also wraps unknown methods.
860859

861860
### Lowlevel `Server.run(raise_exceptions=True)`: transport errors no longer re-raised
862861

863-
`raise_exceptions=True` now only governs handler exceptions: an exception raised by an `on_*` handler propagates out of `run()` instead of being converted to a JSON-RPC error response.
862+
`raise_exceptions=True` now only governs handler exceptions: an exception raised by an `on_*` handler propagates out of `run()`. The JSON-RPC error response is still written to the client first, regardless of the flag.
864863

865864
Previously it also re-raised exceptions yielded by the transport onto the read stream (e.g. JSON parse errors). Those are now debug-logged and dropped regardless of `raise_exceptions`. If you relied on `run()` exiting on a transport-level parse error, that no longer happens.
866865

@@ -1115,7 +1114,7 @@ In practice, replace direct `ServerSession` use with `Server.run(read_stream, wr
11151114
- `ServerRequestResponder` type alias.
11161115
- `ServerSession.incoming_messages` stream — there is no longer a public stream of inbound messages to iterate. Register handlers via the `on_*` constructor params (or `add_request_handler`) and use `Server.middleware` to observe every inbound request and notification (`initialize`, unknown methods, validation failures, and `notifications/initialized` included).
11171116
- `ServerSession.__aenter__` / `__aexit__``ServerSession` is no longer an async context manager.
1118-
- The private `_receive_loop`, `_received_request`, `_received_notification`, and `_handle_incoming` overrides — there is nothing to override on `ServerSession` anymore. To intercept inbound messages, use `Server.middleware` or `DispatchMiddleware` (see the `_handle_*` removal section above).
1117+
- The private `_receive_loop`, `_received_request`, `_received_notification`, and `_handle_incoming` overrides — there is nothing to override on `ServerSession` anymore. To intercept inbound messages, use `Server.middleware` (see the `_handle_*` removal section above).
11191118

11201119
### `BaseSession` / `RequestResponder`: server-side cancellation tracking removed
11211120

src/mcp/server/context.py

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -123,7 +123,9 @@ class ServerMiddleware(Protocol[_MwLifespanT]):
123123
is built but before any validation, lookup, or handshake. Wraps every
124124
inbound request and notification: `initialize`, the pre-init gate,
125125
`METHOD_NOT_FOUND`, params validation, the handler call, and
126-
`notifications/initialized` all run inside `call_next()`. A request-side
126+
`notifications/initialized` all run inside `call_next()`.
127+
`notifications/cancelled` is observed too; the dispatcher applies the
128+
cancellation itself, then forwards the notification. A request-side
127129
failure reaches the middleware as a raised `MCPError` (or
128130
`ValidationError` for malformed params) so observation/logging middleware
129131
can record it. Listed outermost-first on `Server.middleware`.

src/mcp/server/runner.py

Lines changed: 16 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -141,10 +141,12 @@ def _dump_result(result: Any) -> dict[str, Any]:
141141
if result is None:
142142
return {}
143143
if isinstance(result, ErrorData):
144-
# The existing `BaseSession._send_response` treats a handler-returned
145-
# `ErrorData` as a JSON-RPC error, not a success result. Re-raise as
146-
# `MCPError` so the dispatcher's exception boundary emits `JSONRPCError`.
147-
raise MCPError(code=result.code, message=result.message, data=result.data)
144+
# The existing `BaseSession._send_response` treats a returned
145+
# `ErrorData` as a JSON-RPC error, not a success result. Handler
146+
# returns are converted inside `_on_request`'s `_inner` (so
147+
# `Server.middleware` observes the raise); this branch is the boundary
148+
# check for middleware that itself returns `ErrorData`.
149+
raise MCPError.from_error_data(result)
148150
if isinstance(result, BaseModel):
149151
return result.model_dump(by_alias=True, mode="json", exclude_none=True)
150152
if isinstance(result, dict):
@@ -266,7 +268,16 @@ async def _inner() -> HandlerResult:
266268
typed_params = None
267269
else:
268270
typed_params = entry.params_type.model_validate(params, by_name=False)
269-
return await entry.handler(ctx, typed_params)
271+
result = await entry.handler(ctx, typed_params)
272+
if isinstance(result, ErrorData):
273+
# A handler-returned `ErrorData` is a JSON-RPC error, not a
274+
# success result (matches `BaseSession._send_response`). Raise
275+
# here, inside the middleware chain, so `Server.middleware`
276+
# observes the failure as a raised `MCPError` out of
277+
# `call_next()` per the `ServerMiddleware` contract instead of
278+
# a successful-looking `ErrorData` return.
279+
raise MCPError.from_error_data(result)
280+
return result
270281

271282
call = self._compose_server_middleware(ctx, method, params, _inner)
272283
return _dump_result(await call())

src/mcp/shared/dispatcher.py

Lines changed: 6 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -62,15 +62,19 @@ class CallOptions(TypedDict, total=False):
6262
"""Opaque token to resume a previously interrupted request.
6363
6464
Client-side, streamable-HTTP only. Ignored by server dispatchers and other
65-
transports. Supports protocol version 2025-11-25 and earlier; SSE-stream
65+
transports, and also ignored (with a debug log) for requests sent from a
66+
`DispatchContext`, where routing onto the inbound request's stream takes
67+
precedence. Supports protocol version 2025-11-25 and earlier; SSE-stream
6668
resumption is removed in the next protocol revision.
6769
"""
6870

6971
on_resumption_token: Callable[[str], Awaitable[None]]
7072
"""Receive a resumption token when the transport issues one for this request.
7173
7274
Client-side, streamable-HTTP only. Ignored by server dispatchers and other
73-
transports. Supports protocol version 2025-11-25 and earlier; SSE-stream
75+
transports, and also ignored (with a debug log) for requests sent from a
76+
`DispatchContext`, where routing onto the inbound request's stream takes
77+
precedence. Supports protocol version 2025-11-25 and earlier; SSE-stream
7478
resumption is removed in the next protocol revision.
7579
"""
7680

src/mcp/shared/jsonrpc_dispatcher.py

Lines changed: 70 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -45,6 +45,7 @@
4545
from mcp.shared.transport_context import TransportContext
4646
from mcp.types import (
4747
CONNECTION_CLOSED,
48+
INTERNAL_ERROR,
4849
INVALID_PARAMS,
4950
REQUEST_CANCELLED,
5051
REQUEST_TIMEOUT,
@@ -189,8 +190,17 @@ def _outbound_metadata(related_request_id: RequestId | None, opts: CallOptions |
189190
request it belongs to (so streamable-HTTP can route it onto that request's
190191
SSE stream). `ClientMessageMetadata` carries resumption hints to the
191192
client transport. `None` is the common case.
193+
194+
`SessionMessage.metadata` carries exactly one of these, so when
195+
`related_request_id` is set it takes precedence and any resumption hints
196+
in `opts` are dropped (with a debug log): requests made from a dispatch
197+
context are routed onto the inbound request's stream, not resumed.
192198
"""
193199
if related_request_id is not None:
200+
if opts and (opts.get("resumption_token") is not None or opts.get("on_resumption_token") is not None):
201+
logger.debug(
202+
"dropping resumption hints: related_request_id %r takes precedence on metadata", related_request_id
203+
)
194204
return ServerMessageMetadata(related_request_id=related_request_id)
195205
if opts:
196206
token = opts.get("resumption_token")
@@ -366,7 +376,14 @@ async def notify(
366376
*,
367377
_related_request_id: RequestId | None = None,
368378
) -> None:
369-
msg = JSONRPCNotification(jsonrpc="2.0", method=method, params=dict(params) if params is not None else None)
379+
# Leave `params` unset (not explicitly None) when there are none:
380+
# transports serialize with `exclude_unset=True`, and an explicit None
381+
# would survive as `"params": null`, which JSON-RPC 2.0 forbids and
382+
# strict peers (e.g. the TypeScript SDK's zod schemas) reject.
383+
if params is not None:
384+
msg = JSONRPCNotification(jsonrpc="2.0", method=method, params=dict(params))
385+
else:
386+
msg = JSONRPCNotification(jsonrpc="2.0", method=method)
370387
await self._write(msg, _outbound_metadata(_related_request_id, None))
371388

372389
async def run(
@@ -464,11 +481,26 @@ async def _dispatch_request(
464481
) -> None:
465482
progress_token: ProgressToken | None
466483
match req.params:
467-
case {"_meta": {"progressToken": str() | int() as progress_token}}:
484+
# The bool guard matters: `int()` patterns match bool (a subclass),
485+
# and `True == 1` would alias dict lookups to request id 1.
486+
case {"_meta": {"progressToken": str() | int() as progress_token}} if not isinstance(progress_token, bool):
468487
pass
469488
case _:
470489
progress_token = None
471-
transport_ctx = self._transport_builder(metadata)
490+
try:
491+
transport_ctx = self._transport_builder(metadata)
492+
except Exception:
493+
# Containment boundary for the user-supplied builder: a raising
494+
# builder must cost only this message, not the whole connection
495+
# (the exception would otherwise escape into run()'s read loop).
496+
logger.exception("transport_builder raised; rejecting request %r", req.id)
497+
self._spawn(
498+
self._write_error,
499+
req.id,
500+
ErrorData(code=INTERNAL_ERROR, message="transport context unavailable"),
501+
sender_ctx=sender_ctx,
502+
)
503+
return
472504
dctx = _JSONRPCDispatchContext(
473505
transport=transport_ctx,
474506
_dispatcher=self,
@@ -480,7 +512,11 @@ async def _dispatch_request(
480512
# TODO(maxisbey): the spec puts request-id uniqueness on the sender;
481513
# neither v1 nor the TS SDK guards a duplicate id here, so for now we
482514
# blind-overwrite (parity). Revisit rejecting with INVALID_REQUEST.
483-
self._in_flight[req.id] = _InFlight(scope=scope, dctx=dctx)
515+
# Coerced key so `notifications/cancelled` correlates regardless of
516+
# whether the peer stringifies the id between request and cancel
517+
# (`_dispatch_notification` coerces at lookup; responses still echo
518+
# `req.id` verbatim).
519+
self._in_flight[_coerce_id(req.id)] = _InFlight(scope=scope, dctx=dctx)
484520
if req.method in self._inline_methods:
485521
# Spawn (so `sender_ctx` applies, matching the concurrent path) but
486522
# park the read loop until the handler returns; that's the inline
@@ -512,22 +548,34 @@ def _dispatch_notification(
512548
`notifications/cancelled` and `notifications/progress` are intercepted
513549
here because they correlate against JSON-RPC request IDs - the
514550
`_in_flight` / `_pending` tables this layer owns - so no higher layer
515-
can act on them. See the module docstring for the design rationale.
551+
can act on them. Both are still teed to `on_notify` afterwards, so
552+
middleware and registered notification handlers observe every inbound
553+
notification. See the module docstring for the design rationale.
516554
"""
517555
if msg.method == "notifications/cancelled":
518556
match msg.params:
519-
case {"requestId": str() | int() as rid} if (in_flight := self._in_flight.get(rid)) is not None:
557+
# The bool guards here and below matter: `int()` patterns match
558+
# bool (a subclass), and `True == 1` would alias the dict lookup
559+
# to the entry keyed by request id 1.
560+
case {"requestId": str() | int() as rid} if (
561+
not isinstance(rid, bool) and (in_flight := self._in_flight.get(_coerce_id(rid))) is not None
562+
):
520563
in_flight.dctx.cancel_requested.set()
521564
if self._peer_cancel_mode == "interrupt":
522565
in_flight.scope.cancel()
523566
case _:
524567
pass
525-
return
526-
if msg.method == "notifications/progress":
568+
# fall through: cancelled is also teed to on_notify so middleware
569+
# and registered handlers can observe it (matches DirectDispatcher,
570+
# which forwards every notification).
571+
elif msg.method == "notifications/progress":
527572
match msg.params:
528573
case {"progressToken": str() | int() as token, "progress": int() | float() as progress} if (
529-
pending := self._pending.get(_coerce_id(token))
530-
) is not None and pending.on_progress is not None:
574+
not isinstance(token, bool)
575+
and not isinstance(progress, bool)
576+
and (pending := self._pending.get(_coerce_id(token))) is not None
577+
and pending.on_progress is not None
578+
):
531579
total = msg.params.get("total")
532580
message = msg.params.get("message")
533581
self._spawn(
@@ -540,7 +588,13 @@ def _dispatch_notification(
540588
case _:
541589
pass
542590
# fall through: progress is also teed to on_notify
543-
transport_ctx = self._transport_builder(metadata)
591+
try:
592+
transport_ctx = self._transport_builder(metadata)
593+
except Exception:
594+
# Same containment boundary as `_dispatch_request`: a raising
595+
# builder drops this notification instead of killing the read loop.
596+
logger.exception("transport_builder raised; dropping notification %r", msg.method)
597+
return
544598
dctx = _JSONRPCDispatchContext(
545599
transport=transport_ctx, _dispatcher=self, _request_id=None, message_metadata=metadata
546600
)
@@ -615,7 +669,7 @@ async def _handle_request(
615669
# handler return and the pop, so the cancel can't
616670
# interleave there.
617671
dctx.close()
618-
self._in_flight.pop(req.id, None)
672+
self._in_flight.pop(_coerce_id(req.id), None)
619673
await self._write_result(req.id, result)
620674
if scope.cancel_called:
621675
# Peer-cancel: `_dispatch_notification` cancelled this scope
@@ -649,8 +703,10 @@ async def _handle_request(
649703
await self._write_error(req.id, ErrorData(code=0, message=str(e)))
650704
if self._raise_handler_exceptions:
651705
raise
652-
finally:
653-
self._in_flight.pop(req.id, None)
706+
# No outer `_in_flight` pop here: the inner `finally` above already
707+
# removes the entry on every path out of the handler, and a second
708+
# pop after the awaited response writes could evict a newer request
709+
# that reused the id during that window.
654710

655711
def _allocate_id(self) -> int:
656712
self._next_id += 1

src/mcp/shared/peer.py

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -11,7 +11,7 @@
1111
"""
1212

1313
from collections.abc import Mapping
14-
from typing import Any, overload
14+
from typing import Any, cast, overload
1515

1616
from pydantic import BaseModel
1717

@@ -27,6 +27,8 @@
2727
IncludeContext,
2828
ListRootsResult,
2929
ModelPreferences,
30+
RequestParams,
31+
RequestParamsMeta,
3032
SamplingMessage,
3133
Tool,
3234
ToolChoice,
@@ -44,11 +46,18 @@ def dump_params(model: BaseModel | None, meta: Meta | None = None) -> dict[str,
4446
Shared by `ClientPeerMixin`, `Connection`, and `TypedServerRequestMixin` so every
4547
typed convenience method gets the same `_meta` handling. `meta` keys take
4648
precedence over any `_meta` already present on the model.
49+
50+
`meta` is serialized through `RequestParams` so Python field names emit
51+
their wire aliases: an inbound `ctx.meta` carries `progress_token` (the
52+
key `_extract_meta` validation produces), and forwarding it outbound via
53+
`meta=ctx.meta` must put `progressToken` back on the wire. Keys not
54+
declared on `RequestParamsMeta` pass through unchanged.
4755
"""
4856
out = model.model_dump(by_alias=True, mode="json", exclude_none=True) if model is not None else None
4957
if meta:
58+
wire_meta = RequestParams(_meta=cast(RequestParamsMeta, meta)).model_dump(by_alias=True, mode="json")["_meta"]
5059
out = dict(out or {})
51-
out["_meta"] = {**out.get("_meta", {}), **meta}
60+
out["_meta"] = {**out.get("_meta", {}), **wire_meta}
5261
return out
5362

5463

0 commit comments

Comments
 (0)