Repository navigation
Conversation
…rigin When a request to /sse fails the transport-security checks (DNS-rebinding protection: disallowed Host -> 421, disallowed Origin -> 403), connect_sse sends the rejection response and then raises ValueError to signal the caller that no SSE session was established. FastMCP.sse_app()'s handle_sse had no handler for it, so the exception escaped the ASGI callable and uvicorn crashed the whole server process -- one malformed request killed every client (reported in modelcontextprotocol#3661 via ida-pro-mcp's idalib-mcp). Catch ValueError in handle_sse: the rejection response has already been sent to that one client, so just return. Also document connect_sse's send-then-raise contract so future callers know to handle it. Adds a regression test driving MCPServer.sse_app() with a disallowed Host through the in-process ASGI bridge: 421 returned and no exception escapes. 32 server/security tests pass (1367 in tests/server/).
|
This PR has been closed automatically. This repo only keeps pull requests open when they come from a maintainer, or from a contributor a maintainer has assigned to the linked issue, and you aren't currently assigned to #3661. If a maintainer assigns you to #3661, this PR reopens on its own and there's nothing more you need to do here. Assignment is a maintainer call based on capacity; comments that only ask to be assigned don't factor in. What does help is engaging on the issue itself by confirming the repro, explaining why it matters for your use case, or describing the approach you'd take. You're welcome to keep pushing commits here (just avoid force-pushing, since GitHub can't reopen a rewritten branch), but that on its own won't get the PR reviewed or the issue assigned, and realistically most auto-closed PRs stay closed. There's no need to open a new PR either way. CONTRIBUTING.md has the full reasoning, but in short:
Maintainers: reopen, remove |
Closes #3661
Bug: a single request to
/ssewith a disallowedHost/Originheader (DNS-rebinding protection, enabled by default for localhost-bound servers) crashed the whole server process.connect_ssesends the rejection response (421/403) and thenraise ValueErrorto signal that no SSE session was established, butMCPServer.sse_app()'shandle_ssehad no handler — the exception escaped the ASGI callable and uvicorn took down every connected client (one-request DoS).Fix (matching the direction suggested in the report):
handle_ssecatchesValueErrorfrom theasync with sse.connect_sse(...)block and returns: the rejection response has already been sent to that one client, so the server just keeps serving everyone else. The exception is logged at debug level.connect_sse's send-then-raise contract so other callers know to handle it (theraiseitself is kept — it's asserted by existing transport-level tests such astest_sse_connect_rejects_a_disallowed_host).Verification:
MCPServer.sse_app()through the in-process ASGI bridge: a disallowedHostgets 421, and a subsequent request to an unmatched route is still served (404) — the app did not crash.tests/server/full suite passes (1367 tests); SSE security suite 32/32.