Skip to content

Commit 6ee870b

Browse files
MrSampsonoliver
andauthored
fix: return a self-describing 404 for an unknown streamable-http session (#1)
* fix: return a self-describing 404 for an unknown streamable-http session StreamableHTTPSessionManager answered a request for a missing or credential-mismatched session with a bare "Session not found" -- the same misleading-error problem #19/#26 already fixed for SseServerTransport's equivalent case (a redeploy or expiry invalidating every connected client's session_id at once), just not ported to this transport. Both call sites now share one message via _session_not_found_response(), built once so the "unknown session" and "credential mismatch" cases (which must answer identically -- see the comment) can't drift apart. Verified: full suite 1140 passed / 0 regressions (95 skipped, 1 xfailed, 2 pre-existing collection errors for the unrelated optional `websockets` extra, not installed in this environment) plus ruff and pyright clean. * fix: address review -- comment direction, unvalidated input note, expire coverage - _session_not_found_response's comment said "callers below" / "check above" -- true when this sat inline in sse.py, backwards once hoisted to a module-level function above both call sites. Converted to a docstring, direction fixed. - session_id here is the raw, client-supplied mcp-session-id header, never validated against SESSION_ID_PATTERN (only IDs the server mints are) -- unlike sse.py's session_id.hex, which is already UUID-validated. Safe (JSON-escaped, application/json, truncated to 64 chars matching the file's existing convention) but the docstring now says so, rather than leaving it to be re-derived later. - Test now asserts "expire" is covered, not just "restart" -- the one genuinely new clause this message adds over the sse-side wording (this transport has a session_idle_timeout; sse doesn't), which was the one claim the prior substring checks didn't actually verify. --------- Co-authored-by: oliver <oliver.sampson@gitterdan.ai>
1 parent cd61762 commit 6ee870b

2 files changed

Lines changed: 48 additions & 16 deletions

File tree

src/mcp/server/streamable_http_manager.py

Lines changed: 37 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -32,6 +32,41 @@
3232
"""Default maximum Streamable HTTP request body size in bytes (4 MiB)."""
3333

3434

35+
def _session_not_found_response(session_id: str) -> Response:
36+
"""Both call sites in ``_handle_stateful_request`` -- the unknown/expired
37+
session branch and the credential-mismatch branch -- use this identical,
38+
self-describing message: a session can be missing either because it was
39+
never valid or because the credential doesn't match its owner, and the
40+
second case must respond exactly as if the session did not exist -- so
41+
the two responses can never diverge without leaking which case occurred.
42+
Same shape as SseServerTransport's unknown_session_response.
43+
44+
``session_id`` here is the raw, client-supplied ``mcp-session-id``
45+
header value, not one already validated against SESSION_ID_PATTERN (that
46+
check only applies to IDs the server itself mints) -- truncated to 64
47+
chars to match the file's existing logging convention, and safe to
48+
reflect back since it's JSON-escaped by model_dump_json and served as
49+
application/json, never sniffed as HTML.
50+
"""
51+
body = JSONRPCError(
52+
jsonrpc="2.0",
53+
id="server-error",
54+
error=ErrorData(
55+
code=INVALID_REQUEST,
56+
message=(
57+
f"Could not find session {session_id[:64]}: the server may have restarted since this "
58+
"session was created, the session may have expired, or the session_id was never valid. "
59+
"Reconnect and send a fresh 'initialize' request to start a new session."
60+
),
61+
),
62+
)
63+
return Response(
64+
body.model_dump_json(by_alias=True, exclude_none=True),
65+
status_code=404,
66+
media_type="application/json",
67+
)
68+
69+
3570
class StreamableHTTPSessionManager:
3671
"""
3772
Manages StreamableHTTP sessions with optional resumability via event store.
@@ -264,14 +299,7 @@ async def _handle_stateful_request(
264299
"Rejecting request for session %s: credential does not match the one that created the session",
265300
request_mcp_session_id[:64],
266301
)
267-
body = JSONRPCError(
268-
jsonrpc="2.0", id="server-error", error=ErrorData(code=INVALID_REQUEST, message="Session not found")
269-
)
270-
response = Response(
271-
body.model_dump_json(by_alias=True, exclude_none=True),
272-
status_code=404,
273-
media_type="application/json",
274-
)
302+
response = _session_not_found_response(request_mcp_session_id)
275303
await response(scope, receive, send)
276304
return
277305
logger.debug("Session already exists, handling request directly")
@@ -354,12 +382,7 @@ async def run_server(*, task_status: TaskStatus[None] = anyio.TASK_STATUS_IGNORE
354382
await http_transport.handle_request(scope, receive, send)
355383
else:
356384
# Unknown or expired session ID - return 404 per MCP spec
357-
body = JSONRPCError(
358-
jsonrpc="2.0", id="server-error", error=ErrorData(code=INVALID_REQUEST, message="Session not found")
359-
)
360-
response = Response(
361-
body.model_dump_json(by_alias=True, exclude_none=True), status_code=404, media_type="application/json"
362-
)
385+
response = _session_not_found_response(request_mcp_session_id)
363386
await response(scope, receive, send)
364387

365388

tests/server/test_streamable_http_manager.py

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -395,7 +395,9 @@ async def mock_receive():
395395

396396
@pytest.mark.anyio
397397
async def test_unknown_session_id_returns_404():
398-
"""Test that requests with unknown session IDs return HTTP 404 per MCP spec."""
398+
"""Requests with unknown session IDs return HTTP 404 per MCP spec, with a
399+
self-describing message -- not a bare "Session not found" -- naming the
400+
cause and remedy, mirroring what #19/#26 already did for SseServerTransport."""
399401
app = Server("test-unknown-session")
400402
manager = StreamableHTTPSessionManager(app=app)
401403

@@ -439,7 +441,14 @@ async def mock_receive():
439441
assert error_data["jsonrpc"] == "2.0"
440442
assert error_data["id"] == "server-error"
441443
assert error_data["error"]["code"] == INVALID_REQUEST
442-
assert error_data["error"]["message"] == "Session not found"
444+
message = error_data["error"]["message"]
445+
assert "restart" in message.lower()
446+
assert "reconnect" in message.lower() and "initialize" in message.lower()
447+
# This transport (unlike sse) has a session_idle_timeout, so an
448+
# expired session is a real, distinct cause the sse-side wording
449+
# doesn't need to name -- assert it's actually covered, not just
450+
# copied from the sse message.
451+
assert "expire" in message.lower()
443452

444453

445454
@pytest.mark.anyio

0 commit comments

Comments
 (0)