fix: return 405 for pre-session GET the server won't serve as SSE - #3129
fix: return 405 for pre-session GET the server won't serve as SSE#3129dosvk wants to merge 4 commits into
Conversation
Sorry, something went wrong.
There was a problem hiding this comment.
All reported issues were addressed across 4 files
Reply with feedback, questions, or to request a fix.
Re-trigger cubic
Sorry, something went wrong.
…s created A pre-session GET (one without a session ID header in stateful mode) was being handled AFTER a transport and session were already registered. The transport's _handle_get_request correctly returned 405, but by then an unused session existed indefinitely (with default no idle timeout). Restructured to reject pre-session GETs at the manager layer BEFORE any transport creation: 1. Manager now checks for GET without session ID first 2. Security validation (DNS rebinding protection) runs before the 405 so malformed/attack requests get their proper 421 response 3. Only after security passes does it return 405 Method Not Allowed The transport-level check remains as defensive code for standalone transport use (the class is public). Tests now cover both paths: - Manager path: test_pre_session_get_rejected_without_creating_transport - Standalone path: test_standalone_transport_pre_session_get_returns_405 Also added test_standalone_transport_get_with_wrong_session_returns_404 to cover the session ID mismatch validation (removed pragma: no cover). Addresses review finding: modelcontextprotocol#3129
|
Good catch on the session leak — fixed in the latest commits. The 405 rejection now happens in StreamableHTTPSessionManager._handle_stateful_request() before any transport is created or registered, so pre-session GET probes no longer leave a session behind (verified by test_pre_session_get_rejected_without_creating_transport, which asserts _server_instances stays empty). Security validation still runs first so DNS-rebinding rejections keep their 421. The transport-level check remains as a defensive path for standalone (manager-less) transport use, now with its own tests instead of a pragma: no cover. Coverage on both changed files is back to 100%, and the snapshot assertions are collapsed per ruff format. |
Sorry, something went wrong.
|
Hey — just checking in on this. CI has been green for a while now and @fgranata offered to test it against their production setup (they have the exact failing client transport on the other side). Let me know if anything needs adjusting. |
Sorry, something went wrong.
The Streamable HTTP spec requires a GET the server does not serve as an SSE stream to get 405 Method Not Allowed, but in stateful mode a pre-session GET returned 400 (missing session ID) instead. Only-405 is what client transports (e.g. the TypeScript SDK's SSE probe) treat as the graceful fall-through to POST, so stock servers aborted those handshakes before initialize. Return 405 with Allow: GET, POST, DELETE for session-less GETs in stateful mode, before Accept validation, matching the Allow value of _handle_unsupported_request. (The 406-for-wildcard-Accept arm of the report is already fixed on main via check_accept_headers.) Closes modelcontextprotocol#3102
…s created A pre-session GET (one without a session ID header in stateful mode) was being handled AFTER a transport and session were already registered. The transport's _handle_get_request correctly returned 405, but by then an unused session existed indefinitely (with default no idle timeout). Restructured to reject pre-session GETs at the manager layer BEFORE any transport creation: 1. Manager now checks for GET without session ID first 2. Security validation (DNS rebinding protection) runs before the 405 so malformed/attack requests get their proper 421 response 3. Only after security passes does it return 405 Method Not Allowed The transport-level check remains as defensive code for standalone transport use (the class is public). Tests now cover both paths: - Manager path: test_pre_session_get_rejected_without_creating_transport - Standalone path: test_standalone_transport_pre_session_get_returns_405 Also added test_standalone_transport_get_with_wrong_session_returns_404 to cover the session ID mismatch validation (removed pragma: no cover). Addresses review finding: modelcontextprotocol#3129
The 405 rejection paths return before the request body is read, so the stub receive callables never execute. Matches the existing convention for unreachable test helpers in this suite.
|
Verification from our side as offered — results below. Setup: StreamableHTTPSessionManager mounted under Starlette, served by uvicorn — the same transport class our production streamable-HTTP deployment runs (three public MCP hosts, real buyer-agent traffic). SDK installed from this PR's branch (mcp + mcp-types workspace packages). Results (all as the PR intends):
This is exactly the behavior we currently provide via a pure-ASGI middleware shim in front of the SDK (built after a partner's client transport spun on the pre-session GET) — with this PR the shim becomes deletable, which is the outcome we were hoping for. 👍 One caveat on scope: we intended to verify with our full production application, but couldn't — the app runs FastMCP 3.4.4, which imports request_ctx from mcp.server.lowlevel.server; that symbol is gone on current main (unrelated to this PR — main's restructuring). So the verification above is transport-level (the code paths this PR changes), not whole-app. Once a release containing this fix is consumable through FastMCP, we'll drop our shim and the production deployment itself becomes the ongoing verification. Thanks @dosvk — from our seat this is ready. |
Sorry, something went wrong.
|
Thank you for running this so quickly — the probe table covers exactly the code paths this PR changes, and knowing it makes your middleware shim deletable is the best possible signal that the fix matches real-world need. Noted on the FastMCP request_ctx import — agreed that's from main's restructuring and independent of this change, but it's useful context for anyone doing whole-app verification downstream. For maintainers, current state: rebased onto main, four new tests covering the pre-session GET / unknown-session paths, and independent transport-level verification above from a production streamable-HTTP operator. Ready for review. |
Sorry, something went wrong.
Reviewers
At least 1 approving review is required to merge this pull request.
Closes #3102
Summary
Per the Streamable HTTP transport spec (Listening for Messages from the Server):
In stateful mode, a pre-session GET (no mcp-session-id header) returned 400 Bad Request: Missing session ID instead. Since client transports that probe for a standalone SSE stream before initialize treat only 405 as the graceful "no SSE — fall back to POST" signal (e.g. the TypeScript SDK's _startOrAuthSse), any other status aborts the handshake — the interop break described in #3102 (credit to @fgranata for the thorough root-cause analysis).
The 406-for-wildcard-Accept arm of the report is already fixed on main via check_accept_headers; this PR addresses the remaining 400 path.
Changes
Backward compatibility
Post-session GETs (valid mcp-session-id + SSE Accept) are unchanged and still serve SSE. Stateless mode is unaffected. POST/DELETE session validation is unchanged.