diff --git a/src/mcp/server/session.py b/src/mcp/server/session.py index f80971b01..5acba0b70 100644 --- a/src/mcp/server/session.py +++ b/src/mcp/server/session.py @@ -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 diff --git a/src/mcp/server/sse.py b/src/mcp/server/sse.py index 489785c4c..bdbe331e4 100644 --- a/src/mcp/server/sse.py +++ b/src/mcp/server/sse.py @@ -237,11 +237,23 @@ 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 @@ -249,8 +261,7 @@ async def handle_post_message(self, scope: Scope, receive: Receive, send: Send) # 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}") diff --git a/tests/server/test_session.py b/tests/server/test_session.py index ba1b44126..fcf760504 100644 --- a/tests/server/test_session.py +++ b/tests/server/test_session.py @@ -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( @@ -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( @@ -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, @@ -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() diff --git a/tests/server/test_sse_security.py b/tests/server/test_sse_security.py index 0978b8a15..43cc98f07 100644 --- a/tests/server/test_sse_security.py +++ b/tests/server/test_sse_security.py @@ -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() diff --git a/tests/shared/test_sse.py b/tests/shared/test_sse.py index 7604450f8..a2e37b007 100644 --- a/tests/shared/test_sse.py +++ b/tests/shared/test_sse.py @@ -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 @@ -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: