Skip to content

Commit 264e719

Browse files
fix(server): discard a session whose establishing request was refused
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() (#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 #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.
1 parent a4f4ccd commit 264e719

4 files changed

Lines changed: 142 additions & 32 deletions

File tree

src/mcp/server/streamable_http.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -793,7 +793,7 @@ async def _handle_delete_request(self, request: Request, send: Send) -> None:
793793
await response(request.scope, request.receive, send)
794794
return
795795

796-
if not await self._validate_request_headers(request, send): # pragma: no cover
796+
if not await self._validate_request_headers(request, send):
797797
return
798798

799799
await self.terminate()

src/mcp/server/streamable_http_manager.py

Lines changed: 23 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -355,8 +355,30 @@ async def run_server(*, task_status: TaskStatus[None] = anyio.TASK_STATUS_IGNORE
355355
# Start the server task
356356
await self._task_group.start(run_server)
357357

358+
# The session above is provisional: every validation (Host, Accept,
359+
# Content-Type, JSON parse, JSON-RPC shape, "Missing session ID")
360+
# lives inside `handle_request`, so it runs only now. If the request
361+
# that was meant to establish this session is refused, the session
362+
# must not survive it -- otherwise a rejected request leaves live
363+
# state behind and hands the caller a usable session id.
364+
establishing_status: int | None = None
365+
366+
async def send_tracking_status(message: Message) -> None:
367+
nonlocal establishing_status
368+
if message["type"] == "http.response.start":
369+
establishing_status = message["status"]
370+
await send(message)
371+
358372
# Handle the HTTP request and return the response
359-
await http_transport.handle_request(scope, receive, send)
373+
await http_transport.handle_request(scope, receive, send_tracking_status)
374+
375+
if establishing_status is not None and establishing_status >= 400:
376+
logger.debug(
377+
f"Discarding session {new_session_id}: establishing request returned {establishing_status}"
378+
)
379+
self._server_instances.pop(new_session_id, None)
380+
self._session_owners.pop(new_session_id, None)
381+
await http_transport.terminate()
360382
else:
361383
# Unknown or expired session ID - return 404 per MCP spec
362384
# TODO(L62): Align error code once spec clarifies

tests/server/test_streamable_http_manager.py

Lines changed: 96 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -3,13 +3,14 @@
33
import json
44
import logging
55
from collections.abc import Iterator
6-
from typing import Any
6+
from typing import Any, Final
77
from unittest.mock import AsyncMock, patch
88

99
import anyio
1010
import httpx2
1111
import pytest
1212
from mcp_types import INVALID_REQUEST, ListToolsResult, PaginatedRequestParams
13+
from mcp_types.version import HANDSHAKE_PROTOCOL_VERSIONS
1314
from starlette.types import Message, Receive, Scope, Send
1415

1516
from mcp import Client
@@ -24,6 +25,19 @@
2425
StreamableHTTPSessionManager,
2526
)
2627

28+
_INITIALIZE_BODY: Final[bytes] = json.dumps(
29+
{
30+
"jsonrpc": "2.0",
31+
"id": 1,
32+
"method": "initialize",
33+
"params": {
34+
"protocolVersion": HANDSHAKE_PROTOCOL_VERSIONS[-1],
35+
"capabilities": {},
36+
"clientInfo": {"name": "test-client", "version": "1.0"},
37+
},
38+
}
39+
).encode()
40+
2741

2842
@pytest.mark.anyio
2943
async def test_run_can_only_be_called_once():
@@ -146,6 +160,67 @@ async def send(message: Message) -> None:
146160
assert response_start["status"] == 413
147161

148162

163+
@pytest.mark.anyio
164+
@pytest.mark.parametrize(
165+
("method", "headers", "body", "expected_status"),
166+
[
167+
pytest.param(
168+
"POST",
169+
[(b"content-type", b"application/json"), (b"accept", b"application/json, text/event-stream")],
170+
json.dumps({"jsonrpc": "2.0", "id": 1, "method": "tools/list", "params": {}}).encode(),
171+
400,
172+
id="post-that-is-not-initialize",
173+
),
174+
pytest.param(
175+
"POST",
176+
[(b"content-type", b"application/json"), (b"accept", b"application/json, text/event-stream")],
177+
b"{not valid json",
178+
400,
179+
id="post-with-malformed-body",
180+
),
181+
pytest.param(
182+
"POST",
183+
[(b"content-type", b"application/json"), (b"accept", b"text/plain")],
184+
_INITIALIZE_BODY,
185+
406,
186+
id="post-with-unacceptable-accept",
187+
),
188+
pytest.param("GET", [(b"accept", b"text/event-stream")], b"", 400, id="get-without-session"),
189+
pytest.param("DELETE", [], b"", 400, id="delete-without-session"),
190+
],
191+
)
192+
async def test_refused_request_leaves_no_session_behind(
193+
method: str, headers: list[tuple[bytes, bytes]], body: bytes, expected_status: int
194+
) -> None:
195+
"""SDK-defined: a request the transport refuses must not leave a registered session behind.
196+
197+
The session is minted before any validation runs -- Host, Accept, Content-Type, JSON parse,
198+
JSON-RPC shape and the "Missing session ID" check all live downstream in the transport -- so a
199+
refusal has to undo it. Otherwise a rejected request grows `_server_instances` forever and hands
200+
the caller a session id that later requests can still use.
201+
202+
This is the same property the suite already asserts by name for the 413 path in
203+
`test_oversized_content_length_is_rejected_before_body_read_or_session_creation`.
204+
"""
205+
manager = StreamableHTTPSessionManager(app=Server("test-refused-request"))
206+
sent_messages: list[Message] = []
207+
208+
async def mock_send(message: Message) -> None:
209+
sent_messages.append(message)
210+
211+
async def mock_receive() -> Message:
212+
return {"type": "http.request", "body": body, "more_body": False}
213+
214+
scope: Scope = {"type": "http", "method": method, "path": "/mcp", "headers": headers}
215+
216+
async with manager.run():
217+
await manager.handle_request(scope, mock_receive, mock_send)
218+
219+
response_start = next(msg for msg in sent_messages if msg["type"] == "http.response.start")
220+
assert response_start["status"] == expected_status
221+
assert manager._server_instances == {}
222+
223+
149224
@pytest.mark.anyio
150225
async def test_client_disconnect_while_streaming_request_body_is_replayed() -> None:
151226
"""SDK-defined: raw ASGI is required to prove a disconnect before body completion reaches the transport."""
@@ -513,35 +588,13 @@ async def test_idle_session_is_reaped(caplog: pytest.LogCaptureFixture, request:
513588
caplog.set_level(logging.INFO, logger=streamable_http_manager.__name__)
514589

515590
async with manager.run():
516-
sent_messages: list[Message] = []
517-
518-
async def mock_send(message: Message):
519-
sent_messages.append(message)
591+
# Establish the session with a real `initialize`: a request the transport refuses no
592+
# longer leaves a session behind, so there would be nothing for the reaper to reap.
593+
session_id = await _open_session(manager, None)
520594

521-
scope = {
522-
"type": "http",
523-
"method": "POST",
524-
"path": "/mcp",
525-
"headers": [(b"content-type", b"application/json")],
526-
}
527-
528-
async def mock_receive():
595+
async def mock_receive() -> Message:
529596
return {"type": "http.request", "body": b"", "more_body": False}
530597

531-
await manager.handle_request(scope, mock_receive, mock_send)
532-
533-
session_id = None
534-
for msg in sent_messages: # pragma: no branch
535-
if msg["type"] == "http.response.start": # pragma: no branch
536-
for header_name, header_value in msg.get("headers", []): # pragma: no branch
537-
if header_name.decode().lower() == MCP_SESSION_ID_HEADER.lower():
538-
session_id = header_value.decode()
539-
break
540-
if session_id: # pragma: no branch
541-
break
542-
543-
assert session_id is not None, "Session ID not found in response headers"
544-
545598
# Wait for the 50ms idle timeout to fire and the session to be unregistered. Re-requesting
546599
# the session to poll for the 404 would push its idle deadline forward and keep it alive.
547600
with anyio.fail_after(5):
@@ -613,18 +666,31 @@ def _request_scope(
613666

614667

615668
async def _open_session(manager: StreamableHTTPSessionManager, user: AuthenticatedUser | None) -> str:
616-
"""Create a new session as `user` and return its session ID."""
669+
"""Create a new session as `user` and return its session ID.
670+
671+
Establishes the session the way a real client does, with an `initialize` request, because
672+
a request the transport refuses no longer leaves a session behind. The reply is an SSE
673+
stream, so the body is followed by a disconnect: that ends the stream and lets
674+
`handle_request` return, while the session itself lives on in the manager's task group.
675+
"""
617676
sent_messages: list[Message] = []
677+
body_sent = False
618678

619679
async def mock_send(message: Message) -> None:
620680
sent_messages.append(message)
621681

622682
async def mock_receive() -> Message:
623-
return {"type": "http.request", "body": b"", "more_body": False}
683+
nonlocal body_sent
684+
if body_sent:
685+
return {"type": "http.disconnect"}
686+
body_sent = True
687+
return {"type": "http.request", "body": _INITIALIZE_BODY, "more_body": False}
624688

625-
await manager.handle_request(_request_scope(user=user), mock_receive, mock_send)
689+
with anyio.fail_after(5):
690+
await manager.handle_request(_request_scope(user=user), mock_receive, mock_send)
626691

627692
response_start = next(msg for msg in sent_messages if msg["type"] == "http.response.start")
693+
assert response_start["status"] == 200, f"initialize was refused with {response_start['status']}"
628694
headers = dict(response_start.get("headers", []))
629695
return headers[MCP_SESSION_ID_HEADER.encode()].decode()
630696

tests/server/test_streamable_http_router.py

Lines changed: 22 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,7 @@
11
"""Regression coverage for the StreamableHTTP per-session response router."""
22

3+
import logging
4+
35
import anyio
46
import pytest
57
from mcp_types import JSONRPCMessage, JSONRPCResponse
@@ -141,3 +143,23 @@ async def test_json_post_answers_500_when_session_terminates_mid_request() -> No
141143

142144
assert post.sent[0]["type"] == "http.response.start"
143145
assert post.sent[0]["status"] == 500
146+
147+
148+
@pytest.mark.anyio
149+
async def test_router_reports_a_stream_closure_it_did_not_cause(caplog: pytest.LogCaptureFixture) -> None:
150+
"""A stream closed without terminating the transport is an anomaly, not a clean shutdown.
151+
152+
`terminate()` closes these streams deliberately and the router says so at debug level.
153+
Anything else closing them is unexplained, so the router must surface it at exception
154+
level instead of swallowing it as a normal end-of-stream.
155+
"""
156+
transport = StreamableHTTPServerTransport(mcp_session_id="sid", is_json_response_enabled=True)
157+
caplog.set_level(logging.ERROR, logger="mcp.server.streamable_http")
158+
159+
async with transport.connect():
160+
assert not transport.is_terminated
161+
# Close the stream the router is iterating without going through terminate().
162+
assert transport._write_stream_reader is not None
163+
await transport._write_stream_reader.aclose()
164+
165+
assert "Unexpected closure of read stream in message router" in caplog.text

0 commit comments

Comments
 (0)