Skip to content

fix(server): discard a session whose establishing request was refused - #3229

Draft
sainikhiljuluri wants to merge 1 commit into
modelcontextprotocol:mainfrom
sainikhiljuluri:fix/discard-session-when-establishing-request-refused
Draft

fix(server): discard a session whose establishing request was refused#3229
sainikhiljuluri wants to merge 1 commit into
modelcontextprotocol:mainfrom
sainikhiljuluri:fix/discard-session-when-establishing-request-refused

Conversation

@sainikhiljuluri

Copy link
Copy Markdown

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-cover

Verified on this branch:

  • ./scripts/test100.00%, TOTAL 49628 Stmts 0 Miss 3920 Branch 0 BrPart, strict-no-cover clean
  • Full suite on a real Python 3.10 interpreter → 5586 passed, 10 skipped, 1 xfailed
  • ruff check / ruff format --check → clean
  • pyright --pythonversion 3.10 → the only 2 errors are pre-existing and unrelated
    (tests/transports/stdio/test_lifecycle.py uses os.waitid, absent on macOS)

Against a default-configured server, every session-less vector before and after:

vector status before after
GET no session id 400 leaked 0
DELETE no session id 400 leaked 0
POST non-initialize 400 leaked 0
POST malformed body 400 leaked 0
POST bad Accept 406 leaked 0
GET bad Host (421) 421 leaked 0

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

  • Removes a stale # pragma: no cover on the DELETE header-validation path, now covered by the new
    parametrized test.
  • Removes four # pragma: no branch markers made unnecessary by the rewritten helper.
  • No new # pragma, # type: ignore, or # noqa anywhere in the diff.

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.

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.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Rejected streamable-HTTP requests leave live sessions behind: the session is registered before the request is validated

1 participant