Skip to content

Commit cd61762

Browse files
author
oliver
committed
fix: return a self-describing 404 for an unknown SSE session
handle_post_message's session-lookup failure returned a bare "Could not find session" 404, indistinguishable from any other cause and naming neither the reason nor the remedy. This is the more common sibling of the case fixed in 8058392 (self-describing error for a request before initialization): that fix answers through the ServerSession's responder once a session object exists, but a restarted process has no session object at all for an unknown session_id -- the rejection happens in handle_post_message itself, one layer below where a JSON-RPC error could be shaped. Every redeploy invalidates every previously-connected client's session_id at once, so this 404 is what most stranded clients actually hit, and it reads as a service outage rather than "restart your MCP client." Both 404 branches (unknown session_id, and the credential-mismatch branch that deliberately mimics "session doesn't exist" so it can't be used to probe for valid session IDs under another user) now share one message via a single Response built once, so they can't drift apart and leak which case occurred. Updated test_sse_security_post_valid_content_type, which asserted the old exact bare string -- it's testing content-type handling reaches the 404 branch at all, not the message's wording, so relaxed to a substring check.
1 parent f7c8353 commit cd61762

3 files changed

Lines changed: 35 additions & 5 deletions

File tree

src/mcp/server/sse.py

Lines changed: 15 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -237,20 +237,31 @@ async def handle_post_message(self, scope: Scope, receive: Receive, send: Send)
237237
response = Response("Invalid session ID", status_code=400)
238238
return await response(scope, receive, send)
239239

240+
# Both branches below use this identical, self-describing message:
241+
# a session can be missing either because it was never valid or
242+
# because the credential doesn't match its owner, and the second
243+
# case must respond exactly as if the session did not exist (see
244+
# below) -- so the two messages can never diverge without leaking
245+
# which case occurred.
246+
unknown_session_response = Response(
247+
f"Could not find session {session_id.hex}: the server may have restarted since this "
248+
"session was created, or the session_id was never valid. Reconnect (open a new SSE "
249+
"stream) and send a fresh 'initialize' request.",
250+
status_code=404,
251+
)
252+
240253
writer = self._read_stream_writers.get(session_id)
241254
if not writer:
242255
logger.warning(f"Could not find session for ID: {session_id}")
243-
response = Response("Could not find session", status_code=404)
244-
return await response(scope, receive, send)
256+
return await unknown_session_response(scope, receive, send)
245257

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

255266
body = await request.body()
256267
logger.debug(f"Received JSON: {body}")

tests/server/test_sse_security.py

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -310,7 +310,7 @@ async def test_sse_security_post_valid_content_type(server_port: int):
310310
# Will get 404 because session doesn't exist, but that's OK
311311
# We're testing that it passes the content-type check
312312
assert response.status_code == 404
313-
assert response.text == "Could not find session"
313+
assert "Could not find session" in response.text
314314

315315
finally:
316316
process.terminate()

tests/shared/test_sse.py

Lines changed: 19 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -5,6 +5,7 @@
55
from collections.abc import AsyncGenerator, Generator
66
from typing import Any
77
from unittest.mock import AsyncMock, MagicMock, Mock, patch
8+
from uuid import uuid4
89

910
import anyio
1011
import httpx
@@ -176,6 +177,24 @@ async def connection_test() -> None:
176177
await connection_test()
177178

178179

180+
@pytest.mark.anyio
181+
async def test_post_message_unknown_session_returns_self_describing_error(http_client: httpx.AsyncClient) -> None:
182+
"""A POST for a session_id the server has no record of (e.g. because the
183+
process restarted since the client's SSE stream was opened) should
184+
explain the cause and remedy, not a bare "Could not find session" -- the
185+
same misleading-error problem #19 fixed for the uninitialized-session
186+
case, one layer down where no ServerSession exists to answer through."""
187+
unknown_session_id = uuid4().hex
188+
response = await http_client.post(
189+
f"/messages/?session_id={unknown_session_id}",
190+
json={"jsonrpc": "2.0", "id": 1, "method": "ping"},
191+
)
192+
assert response.status_code == 404
193+
body = response.text
194+
assert "restart" in body.lower()
195+
assert "reconnect" in body.lower() and "initialize" in body.lower()
196+
197+
179198
@pytest.mark.anyio
180199
async def test_sse_client_basic_connection(server: None, server_url: str) -> None:
181200
async with sse_client(server_url + "/sse") as streams:

0 commit comments

Comments
 (0)