fix(server): discard a session whose establishing request was refused - #3229
Conversation
Sorry, something went wrong.
This comment was marked as spam.
|
Thanks for digging into this — that's a specific, checkable claim, so I ran it against PR head cb521ea before answering. The Starlette half is exactly right: Request.body() does raise ClientDisconnect for a partial http.request followed by http.disconnect. Where it comes apart is the next step — that exception does not propagate out of http_transport.handle_request(...). _handle_post_request has a blanket except Exception (streamable_http.py:676) that logs "Error handling POST request" and sends a 500. So a response does start, establishing_status becomes 500, and the cleanup block runs. I wrote the end-to-end manager test you described, in both shapes: partial initialize body -> http.disconnect
handle_request raised: nothing
response.start count: 1 status: [500]
_server_instances = {}
http.disconnect as the first message
handle_request raised: nothing
response.start count: 1 status: [500]
_server_instances = {}
Both pass on the current head. The traceback you predicted is there in the log — ClientDisconnect at streamable_http.py:537 — it just gets converted into a response rather than escaping. This is actually the case for keying the discard off the response status rather than an enumerated list of validation failures: 500 is not a validation refusal and nobody would have thought to enumerate it, but >= 400 catches it for free. I'll add these two as regression tests so the disconnect path is pinned by name rather than covered by accident. On the wider point — you're right that a finally would be strictly more robust, since handle_request itself has no wrapper and the cleanup is skipped for any exception that escapes it. I couldn't construct one: POST, GET and DELETE all send a response on every path I could reach. Happy to move it to a finally keyed on "no successful establishing response" if a maintainer would rather have the guarantee than the reachability argument. Worth noting for anyone finding this thread: this PR is still a draft and I can't mark it ready — markPullRequestReadyForReview returns FORBIDDEN for this account on this repo. It needs a maintainer. |
Sorry, something went wrong.
A stateful streamable-HTTP session is minted, registered in _server_instances and given a running task BEFORE the request is validated: Host/DNS-rebinding, Accept, Content-Type, JSON parse, JSON-RPC shape and the "Missing session ID" check all live downstream in the transport. So every request the server itself refuses left a live, non-terminated session behind, and nothing reclaimed it -- the idle reaper is off by default and unreachable from streamable_http_app() (modelcontextprotocol#2455). A refused 406 also handed back a usable Mcp-Session-Id, and a follow-up request on that never-initialized id was served 200. Track the establishing response status and, if it is >= 400, drop the session and terminate the transport. All six session-less vectors now leak nothing, while a legitimate initialize still establishes a session that keeps serving. This is a correctness fix, not a DoS fix: 200 valid initialize requests create 200 sessions on the same server, so unbounded growth is already reachable with legitimate traffic. That gap is modelcontextprotocol#2455. The suite previously obtained sessions *via requests the transport rejects* -- _open_session POSTed an empty body answered 400/406 and used the session id it still returned. That helper now performs a real initialize, which is plausibly why this went unnoticed. Removes a stale "pragma: no cover" on the DELETE header-validation path and four "pragma: no branch" markers that the rewritten helper made unnecessary.
HEAD and OPTIONS are refused by `_handle_unsupported_request`, which runs downstream of session registration just like the validation failures already covered here, and echoes the session id back in the same way. Both therefore leave a live session behind on an unpatched tree. They need no production change: the discard keys off the establishing response status, so 405 is already subsumed by `>= 400`. Pinning them stops a later refactor from narrowing that to an enumerated list of validation failures and silently reopening the method-independent half of the leak. Reported by @pete-builds on modelcontextprotocol#3228, reproduced against released 1.29.0. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
A client that disconnects mid-initialize never reaches the refusal paths: Request.body() raises ClientDisconnect, which _handle_post_request turns into a 500. The discard keys off the response status, so it covers this too. Pins that by name for both disconnect shapes. Reported by @keeltrace on modelcontextprotocol#3229. Also realigns with upstream modelcontextprotocol#3336, which moved RequestBodyLimitMiddleware and its replay tests into transport_security. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Fixes #3228
Problem
A stateful streamable-HTTP session is minted, registered in _server_instances and given a
running task before the request is validated. Every check — Host / DNS-rebinding, Accept,
Content-Type, JSON parse, JSON-RPC shape, and the "Missing session ID" check for non-initialize
POSTs — lives downstream in the transport, so it runs only after the session already exists.
Every request the server itself refuses therefore left a live, non-terminated session behind, and
nothing reclaimed it: session_idle_timeout defaults to None and is not reachable from
streamable_http_app() (#2455). A refused 406 also handed back a usable Mcp-Session-Id, and a
follow-up request on that never-initialized id was served 200.
Solution
Track the status of the response to the establishing request; if it is >= 400, drop the session
and terminate the transport.
Chosen over the alternative — "only a POST carrying initialize may establish a session" — because
that requires the manager to peek the body, which currently lives in the transport
(is_initialization_request). This shape needs no body parsing and closes the same vectors. If you
would rather have the stricter shape, say so and I will rewrite it; it would also subsume #3129.
Scope: correctness, not a DoS fix
Worth stating plainly, because an earlier draft of my own report got this wrong. This does not
close an availability hole: on the same no-auth server, 200 valid initialize requests create 200
sessions, so unbounded session growth is already reachable with entirely legitimate traffic. The
real gap there is the absence of a session cap plus an unreachable idle reaper, which is #2455 and
is untouched here.
What this fixes is that the server contradicts itself — requests it refuses leave state behind, a
421 from the default-on DNS-rebinding protection creates a session anyway, and a refusal returns a
working session handle.
How to test
uv sync --frozen --all-extras --dev uv run --frozen pytest tests/server/test_streamable_http_manager.py tests/server/test_streamable_http_router.py -q uv run --frozen --python 3.10 --all-extras --dev pytest -q -n auto ./scripts/test # coverage + strict-no-coverVerified on this branch:
(tests/transports/stdio/test_lifecycle.py uses os.waitid, absent on macOS)
Against a default-configured server, every session-less vector before and after:
A legitimate initialize still returns 200 with a session header, and a follow-up tools/list on
that session is still served 200.
Tests
test_refused_request_leaves_no_session_behind — five parametrized vectors, each asserting the
status and manager._server_instances == {}. Red before this change (all five fail), green after.
It pins the same property the suite already asserts by name for the 413 path in
test_oversized_content_length_is_rejected_before_body_read_or_session_creation.
test_router_reports_a_stream_closure_it_did_not_cause — covers the router's "unexpected closure"
branch. Worth explaining: that branch was previously reached only incidentally, by leaked sessions
whose streams closed at teardown without ever being terminated. Now that refused sessions are
terminated properly, nothing reaches it by accident, so it gets a deliberate test instead of losing
coverage.
A note on the existing test helper
_open_session previously obtained a session via a request the transport rejects — it POSTed an
empty body, was answered 400/406, and used the session id that came back anyway. Eight tests
depended on that, so they fail against any correct fix.
It now performs a real initialize. The reply is an SSE stream, so the body is followed by an
http.disconnect, which ends the stream and lets handle_request return while the session lives on
in the manager's task group. test_idle_session_is_reaped was building its session the same way and
is switched over too.
That the suite encoded the buggy behaviour as the way to obtain a session is plausibly why this
went unnoticed.
Incidental
parametrized test.
Disclosure
Written with AI assistance (Claude Code). Filed by me as the human contributor — I own this change
and will answer any questions on the implementation or the trade-offs above.