diff --git a/src/mcp/server/session.py b/src/mcp/server/session.py index f80971b012..5acba0b707 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 489785c4c9..bdbe331e4a 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/src/mcp/server/streamable_http_manager.py b/src/mcp/server/streamable_http_manager.py index 0ee6d362b5..89412b057d 100644 --- a/src/mcp/server/streamable_http_manager.py +++ b/src/mcp/server/streamable_http_manager.py @@ -32,6 +32,41 @@ """Default maximum Streamable HTTP request body size in bytes (4 MiB).""" +def _session_not_found_response(session_id: str) -> Response: + """Both call sites in ``_handle_stateful_request`` -- the unknown/expired + session branch and the credential-mismatch branch -- 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 -- so + the two responses can never diverge without leaking which case occurred. + Same shape as SseServerTransport's unknown_session_response. + + ``session_id`` here is the raw, client-supplied ``mcp-session-id`` + header value, not one already validated against SESSION_ID_PATTERN (that + check only applies to IDs the server itself mints) -- truncated to 64 + chars to match the file's existing logging convention, and safe to + reflect back since it's JSON-escaped by model_dump_json and served as + application/json, never sniffed as HTML. + """ + body = JSONRPCError( + jsonrpc="2.0", + id="server-error", + error=ErrorData( + code=INVALID_REQUEST, + message=( + f"Could not find session {session_id[:64]}: the server may have restarted since this " + "session was created, the session may have expired, or the session_id was never valid. " + "Reconnect and send a fresh 'initialize' request to start a new session." + ), + ), + ) + return Response( + body.model_dump_json(by_alias=True, exclude_none=True), + status_code=404, + media_type="application/json", + ) + + class StreamableHTTPSessionManager: """ Manages StreamableHTTP sessions with optional resumability via event store. @@ -264,14 +299,7 @@ async def _handle_stateful_request( "Rejecting request for session %s: credential does not match the one that created the session", request_mcp_session_id[:64], ) - body = JSONRPCError( - jsonrpc="2.0", id="server-error", error=ErrorData(code=INVALID_REQUEST, message="Session not found") - ) - response = Response( - body.model_dump_json(by_alias=True, exclude_none=True), - status_code=404, - media_type="application/json", - ) + response = _session_not_found_response(request_mcp_session_id) await response(scope, receive, send) return logger.debug("Session already exists, handling request directly") @@ -354,12 +382,7 @@ async def run_server(*, task_status: TaskStatus[None] = anyio.TASK_STATUS_IGNORE await http_transport.handle_request(scope, receive, send) else: # Unknown or expired session ID - return 404 per MCP spec - body = JSONRPCError( - jsonrpc="2.0", id="server-error", error=ErrorData(code=INVALID_REQUEST, message="Session not found") - ) - response = Response( - body.model_dump_json(by_alias=True, exclude_none=True), status_code=404, media_type="application/json" - ) + response = _session_not_found_response(request_mcp_session_id) await response(scope, receive, send) diff --git a/tests/server/test_session.py b/tests/server/test_session.py index ba1b44126d..fcf7605045 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 0978b8a150..43cc98f077 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/server/test_streamable_http_manager.py b/tests/server/test_streamable_http_manager.py index 9deeeeb37a..ef25caff63 100644 --- a/tests/server/test_streamable_http_manager.py +++ b/tests/server/test_streamable_http_manager.py @@ -395,7 +395,9 @@ async def mock_receive(): @pytest.mark.anyio async def test_unknown_session_id_returns_404(): - """Test that requests with unknown session IDs return HTTP 404 per MCP spec.""" + """Requests with unknown session IDs return HTTP 404 per MCP spec, with a + self-describing message -- not a bare "Session not found" -- naming the + cause and remedy, mirroring what #19/#26 already did for SseServerTransport.""" app = Server("test-unknown-session") manager = StreamableHTTPSessionManager(app=app) @@ -439,7 +441,14 @@ async def mock_receive(): assert error_data["jsonrpc"] == "2.0" assert error_data["id"] == "server-error" assert error_data["error"]["code"] == INVALID_REQUEST - assert error_data["error"]["message"] == "Session not found" + message = error_data["error"]["message"] + assert "restart" in message.lower() + assert "reconnect" in message.lower() and "initialize" in message.lower() + # This transport (unlike sse) has a session_idle_timeout, so an + # expired session is a real, distinct cause the sse-side wording + # doesn't need to name -- assert it's actually covered, not just + # copied from the sse message. + assert "expire" in message.lower() @pytest.mark.anyio diff --git a/tests/shared/test_sse.py b/tests/shared/test_sse.py index 7604450f81..a2e37b007f 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: