Skip to content

Commit cf87495

Browse files
committed
fix(streamable-http): also reject PUT/PATCH/OPTIONS/HEAD without session-id
cubic-dev-ai flagged that the previous fix only short-circuited GET and DELETE — the other non-POST methods still entered the "new session" branch, allocated a transport, and spawned run_server before the transport-layer 405 was returned. Same leak, different method. This commit extends the check to every non-POST method. GET/DELETE remain 400 "Missing session ID" (protocol-valid when a session exists, so the caller may have simply forgotten the header). Everything else becomes 405 "Method Not Allowed" at the manager layer — matching the old transport-layer 405 exactly, minus the leaked session and task. Also applies the repository's ruff format to the test module, which the pre-commit hook flagged on CI, and switches the regression test's assertions to use the parametrized ``expected_status`` / ``expected_message_substring`` values so the new PUT/PATCH/OPTIONS/HEAD rows can piggy-back on the same test body.
1 parent dbadb12 commit cf87495

2 files changed

Lines changed: 68 additions & 41 deletions

File tree

src/mcp/server/streamable_http_manager.py

Lines changed: 36 additions & 24 deletions
Original file line numberDiff line numberDiff line change
@@ -283,31 +283,43 @@ async def _handle_stateful_request(
283283
if request_mcp_session_id is None:
284284
# Only POST may initialize a new session (per MCP spec: a session is
285285
# opened by the response to the ``initialize`` JSON-RPC request,
286-
# which is always a POST). GET and DELETE without a session-id are
287-
# protocol errors and must be rejected here — the transport-layer
288-
# rejection happens too late to matter: by the time
289-
# `StreamableHTTPServerTransport.handle_request()` sees the request,
290-
# the session has already been allocated in `_server_instances`
291-
# AND a background `run_server` task has been spawned, waiting on a
286+
# which is always a POST). Any non-POST request without a session-id
287+
# is a protocol error and must be rejected here — the transport-layer
288+
# rejection happens too late: by the time
289+
# ``StreamableHTTPServerTransport.handle_request()`` sees the request,
290+
# a session has already been allocated in ``_server_instances`` and
291+
# a background ``run_server`` task has been spawned waiting on a
292292
# stream that will never receive anything. Both leak forever unless
293-
# `session_idle_timeout` is set (which is opt-in and off by default).
294-
# Other unsupported methods (PUT/PATCH/OPTIONS/...) are intentionally
295-
# let through to the existing ``_handle_unsupported_request`` path,
296-
# which returns 405 — preserved for API stability.
297-
if request.method in ("GET", "DELETE"):
298-
error_response = JSONRPCError(
299-
jsonrpc="2.0",
300-
id="server-error",
301-
error=ErrorData(
302-
code=INVALID_REQUEST,
303-
message="Bad Request: Missing session ID",
304-
),
305-
)
306-
response = Response(
307-
content=error_response.model_dump_json(by_alias=True, exclude_none=True),
308-
status_code=400,
309-
media_type="application/json",
310-
)
293+
# ``session_idle_timeout`` is set (opt-in, off by default). Rejecting
294+
# here also holds for PUT/PATCH/OPTIONS/HEAD — the transport would
295+
# answer 405 to those, but only after the leaky allocation.
296+
if request.method != "POST":
297+
# GET/DELETE are protocol-valid methods when a session exists,
298+
# so the correct response for a missing session is 400 — matching
299+
# the transport's existing "Missing session ID" wording. Anything
300+
# else is a genuinely unsupported method, so 405 is more accurate.
301+
if request.method in ("GET", "DELETE"):
302+
error_body = JSONRPCError(
303+
jsonrpc="2.0",
304+
id="server-error",
305+
error=ErrorData(code=INVALID_REQUEST, message="Bad Request: Missing session ID"),
306+
)
307+
response = Response(
308+
content=error_body.model_dump_json(by_alias=True, exclude_none=True),
309+
status_code=400,
310+
media_type="application/json",
311+
)
312+
else:
313+
error_body = JSONRPCError(
314+
jsonrpc="2.0",
315+
id="server-error",
316+
error=ErrorData(code=INVALID_REQUEST, message=f"Method Not Allowed ({request.method})"),
317+
)
318+
response = Response(
319+
content=error_body.model_dump_json(by_alias=True, exclude_none=True),
320+
status_code=405,
321+
media_type="application/json",
322+
)
311323
await response(scope, receive, send)
312324
return
313325

tests/server/test_streamable_http_manager.py

Lines changed: 32 additions & 17 deletions
Original file line numberDiff line numberDiff line change
@@ -384,18 +384,35 @@ async def capture_send(message: Message):
384384

385385

386386
@pytest.mark.anyio
387-
@pytest.mark.parametrize("method", ["GET", "DELETE"])
388-
async def test_non_post_without_session_id_does_not_allocate_session(method: str):
389-
"""Regression test: GET/DELETE without a session-id must return 400 without
390-
allocating a session or spawning a background task.
391-
392-
Before this fix, any request without a session id — including GET/DELETE —
393-
entered the "new session" branch of the manager, created a transport,
394-
registered it in ``_server_instances``, and launched a ``run_server`` task
395-
that would wait forever for messages that never come. The transport-level
396-
validation then rejected the request (with 400 or 406), but the allocated
397-
session + task were leaked. Under a Docker healthcheck polling /mcp every
398-
30 seconds this accumulated ~2 sessions/min indefinitely (~1 GiB/week).
387+
@pytest.mark.parametrize(
388+
("method", "expected_status", "expected_message_substring"),
389+
[
390+
("GET", 400, "Missing session ID"),
391+
("DELETE", 400, "Missing session ID"),
392+
("PUT", 405, "Method Not Allowed"),
393+
("PATCH", 405, "Method Not Allowed"),
394+
("OPTIONS", 405, "Method Not Allowed"),
395+
("HEAD", 405, "Method Not Allowed"),
396+
],
397+
)
398+
async def test_non_post_without_session_id_does_not_allocate_session(
399+
method: str, expected_status: int, expected_message_substring: str
400+
):
401+
"""Regression test: no non-POST method without a session-id may allocate a
402+
session or spawn a background task.
403+
404+
Before this fix, any request without a session id — including GET/DELETE
405+
(per this bug report) but also PUT/PATCH/OPTIONS/HEAD — entered the
406+
"new session" branch of the manager, created a transport, registered it
407+
in ``_server_instances``, and launched a ``run_server`` task that would
408+
wait forever for messages that never come. The transport-level validation
409+
then returned 400/406/405, but the allocated session + task were leaked.
410+
Under a Docker healthcheck polling /mcp every 30 seconds this accumulated
411+
~2 sessions/min indefinitely (~1 GiB/week).
412+
413+
GET/DELETE are protocol-valid methods when a session exists, so they get
414+
``400 "Missing session ID"``. Other methods are genuinely unsupported on
415+
the MCP endpoint, so they get ``405 "Method Not Allowed"``.
399416
"""
400417
app = Server("test-non-post-no-session")
401418
manager = StreamableHTTPSessionManager(app=app)
@@ -445,12 +462,12 @@ async def mock_receive(): # pragma: no cover
445462
None,
446463
)
447464
assert response_start is not None, "Should have sent a response"
448-
assert response_start["status"] == 400
465+
assert response_start["status"] == expected_status
449466

450467
error_data = json.loads(response_body)
451468
assert error_data["jsonrpc"] == "2.0"
452469
assert error_data["error"]["code"] == INVALID_REQUEST
453-
assert "Missing session ID" in error_data["error"]["message"]
470+
assert expected_message_substring in error_data["error"]["message"]
454471

455472

456473
@pytest.mark.anyio
@@ -466,9 +483,7 @@ async def test_bad_host_header_rejected_before_session_allocation():
466483
app = Server("test-bad-host")
467484
manager = StreamableHTTPSessionManager(
468485
app=app,
469-
security_settings=TransportSecuritySettings(
470-
enable_dns_rebinding_protection=True, allowed_hosts=["127.0.0.1"]
471-
),
486+
security_settings=TransportSecuritySettings(enable_dns_rebinding_protection=True, allowed_hosts=["127.0.0.1"]),
472487
)
473488

474489
async with manager.run():

0 commit comments

Comments
 (0)