Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
35 changes: 34 additions & 1 deletion src/mcp/server/session.py
Original file line number Diff line number Diff line change
Expand Up @@ -202,7 +202,40 @@ async def _received_request(self, responder: RequestResponder[types.ClientReques
pass
case _:
if self._initialization_state != InitializationState.Initialized:
raise RuntimeError("Received request before initialization was complete")
# Answer the request directly with a self-describing error
# instead of raising. A bare exception here would propagate
# to BaseSession._receive_loop's blanket except-Exception
# handler, which discards the exception's message entirely
# and always responds with the generic, misleading
# ErrorData(code=INVALID_PARAMS, message="Invalid request
# parameters") -- indistinguishable from an actually
# malformed request. INVALID_REQUEST (not INVALID_PARAMS)
# is used because the request's parameters aren't the
# problem; the session's state is -- matching the existing
# "Session not found" usage in streamable_http_manager.py
# for the same class of session-validity condition.
#
# The message states the fact (an initialize handshake is
# required) without asserting *why* this particular
# session never completed one -- this branch can't tell a
# genuinely uninitialized session (e.g. a client bug) from
# a stream that reconnected without reinitializing, so it
# doesn't claim either.
with responder:
await responder.respond(
types.ErrorData(
code=types.INVALID_REQUEST,
message=(
"MCP session not initialized: an 'initialize' "
"request must complete successfully before "
"other requests are handled. If this session "
"was previously initialized, its stream may "
"have been reconnected without a fresh "
"handshake."
),
data="session_not_initialized",
)
)

async def _received_notification(self, notification: types.ClientNotification) -> None:
# Need this to avoid ASYNC910
Expand Down
19 changes: 15 additions & 4 deletions src/mcp/server/sse.py
Original file line number Diff line number Diff line change
Expand Up @@ -237,20 +237,31 @@ async def handle_post_message(self, scope: Scope, receive: Receive, send: Send)
response = Response("Invalid session ID", status_code=400)
return await response(scope, receive, send)

# Both branches below use this identical, self-describing message:
# a session can be missing either because it was never valid or
# because the credential doesn't match its owner, and the second
# case must respond exactly as if the session did not exist (see
# below) -- so the two messages can never diverge without leaking
# which case occurred.
unknown_session_response = Response(
f"Could not find session {session_id.hex}: the server may have restarted since this "
"session was created, or the session_id was never valid. Reconnect (open a new SSE "
"stream) and send a fresh 'initialize' request.",
status_code=404,
)

writer = self._read_stream_writers.get(session_id)
if not writer:
logger.warning(f"Could not find session for ID: {session_id}")
response = Response("Could not find session", status_code=404)
return await response(scope, receive, send)
return await unknown_session_response(scope, receive, send)

user = scope.get("user")
requestor = authorization_context(user) if isinstance(user, AuthenticatedUser) else None
if requestor != self._session_owners.get(session_id):
# A session can only be used with the credential that created it.
# Respond exactly as if the session did not exist.
logger.warning("Rejecting message for session %s: credential does not match", session_id)
response = Response("Could not find session", status_code=404)
return await response(scope, receive, send)
return await unknown_session_response(scope, receive, send)

body = await request.body()
logger.debug(f"Received JSON: {body}")
Expand Down
30 changes: 27 additions & 3 deletions tests/server/test_session.py
Original file line number Diff line number Diff line change
Expand Up @@ -472,6 +472,9 @@ async def test_other_requests_blocked_before_initialization():

error_response_received = False
error_code = None
error_message_text = None
error_data = None
session_ref: list[ServerSession] = []

async def run_server():
async with ServerSession(
Expand All @@ -482,13 +485,14 @@ async def run_server():
server_version="0.1.0",
capabilities=ServerCapabilities(),
),
):
) as session:
session_ref.append(session)
# Server should handle the request and send an error response
# No need to process incoming_messages since the error is handled automatically
await anyio.sleep(0.1) # Give time for the request to be processed

async def mock_client():
nonlocal error_response_received, error_code
nonlocal error_response_received, error_code, error_message_text, error_data

# Try to send a non-ping request before initialization
await client_to_server_send.send(
Expand All @@ -508,6 +512,8 @@ async def mock_client():
if isinstance(error_message.message.root, types.JSONRPCError): # pragma: no branch
error_response_received = True
error_code = error_message.message.root.error.code
error_message_text = error_message.message.root.error.message
error_data = error_message.message.root.error.data

async with (
client_to_server_send,
Expand All @@ -520,4 +526,22 @@ async def mock_client():
tg.start_soon(mock_client)

assert error_response_received
assert error_code == types.INVALID_PARAMS
# INVALID_REQUEST (not INVALID_PARAMS): the request's parameters aren't
# the problem, the session's initialization state is. Previously this
# fell through to BaseSession._receive_loop's generic exception handler,
# which always reported INVALID_PARAMS with a generic "Invalid request
# parameters" message regardless of the actual cause -- indistinguishable
# from a genuinely malformed request.
assert error_code == types.INVALID_REQUEST
assert error_message_text is not None and "session not initialized" in error_message_text.lower()
# data is the machine-readable discriminator -- message text isn't meant
# to be programmatically parsed, this is.
assert error_data == "session_not_initialized"
# Answering via responder.respond() (rather than raising, as before this
# fix) means RequestResponder.__exit__ sees _completed=True and fires
# on_complete, which pops the request out of _in_flight. Before this fix,
# the bare RuntimeError bypassed the responder's context-manager cleanup
# entirely, so every rejected pre-init request leaked an _in_flight
# entry for the life of the session.
assert session_ref and len(session_ref[0]._in_flight) == 0
assert error_message_text is not None and "session not initialized" in error_message_text.lower()
2 changes: 1 addition & 1 deletion tests/server/test_sse_security.py
Original file line number Diff line number Diff line change
Expand Up @@ -310,7 +310,7 @@ async def test_sse_security_post_valid_content_type(server_port: int):
# Will get 404 because session doesn't exist, but that's OK
# We're testing that it passes the content-type check
assert response.status_code == 404
assert response.text == "Could not find session"
assert "Could not find session" in response.text

finally:
process.terminate()
Expand Down
19 changes: 19 additions & 0 deletions tests/shared/test_sse.py
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@
from collections.abc import AsyncGenerator, Generator
from typing import Any
from unittest.mock import AsyncMock, MagicMock, Mock, patch
from uuid import uuid4

import anyio
import httpx
Expand Down Expand Up @@ -176,6 +177,24 @@ async def connection_test() -> None:
await connection_test()


@pytest.mark.anyio
async def test_post_message_unknown_session_returns_self_describing_error(http_client: httpx.AsyncClient) -> None:
"""A POST for a session_id the server has no record of (e.g. because the
process restarted since the client's SSE stream was opened) should
explain the cause and remedy, not a bare "Could not find session" -- the
same misleading-error problem #19 fixed for the uninitialized-session
case, one layer down where no ServerSession exists to answer through."""
unknown_session_id = uuid4().hex
response = await http_client.post(
f"/messages/?session_id={unknown_session_id}",
json={"jsonrpc": "2.0", "id": 1, "method": "ping"},
)
assert response.status_code == 404
body = response.text
assert "restart" in body.lower()
assert "reconnect" in body.lower() and "initialize" in body.lower()


@pytest.mark.anyio
async def test_sse_client_basic_connection(server: None, server_url: str) -> None:
async with sse_client(server_url + "/sse") as streams:
Expand Down
Loading