fix: refuse standalone GET on the MCP endpoint with 405 (#136) - #140
Merged
Conversation
The mcp SDK (1.28.x) serves GET as an infinite standalone SSE stream even in stateless mode with JSON responses — a stream korvid can never send anything on. Because korvid neutralizes uvicorn's signal capture (the TUI owns signals), sse-starlette's shutdown watcher never fires, uvicorn's graceful shutdown waits forever on the open connection, and the controller's 5s deadline falls back to a hard cancel that tears the stream down mid-request: 'Exception in ASGI application' (CancelledError) plus a lifespan traceback land on the terminal the TUI is drawing on. The MCP spec allows a server that offers no SSE stream to answer GET with 405 Method Not Allowed (the TypeScript SDK's stateless mode does exactly this), so refuse everything but POST in the ASGI wrapper before it reaches the SDK session manager. No connection is ever held open, and graceful shutdown completes within the controller's deadline. Fixes #136 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
There was a problem hiding this comment.
Pull request overview
Refuses non-POST MCP requests to prevent infinite GET SSE streams from blocking shutdown.
Changes:
- Returns HTTP 405 with
Allow: POSTfor non-POST requests. - Adds GET rejection and shutdown regression tests.
Reviewed changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 2 comments.
| File | Description |
|---|---|
src/korvid/mcp/server.py |
Adds the HTTP method guard. |
tests/mcp/test_server.py |
Adds GET and shutdown coverage. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
…n-connection shutdown regression Review round 1 on #140: - The early 405 bypassed the DNS-rebinding Host/Origin validation for non-POST requests. Run the same TransportSecurityMiddleware check the session manager applies before answering 405, so a hostile Origin on a GET is refused (403), never acknowledged. New test: test_hostile_origin_get_is_rejected_not_answered_405. - The shutdown regression test awaited the GET to completion before requesting shutdown, so it never exercised shutdown with an open connection. It now synchronizes on response start and requests shutdown while the GET is held open — verified to fail (timeout, then CancelledError tracebacks) against the pre-fix server. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
![my-reviewer-agent[bot]](https://avatars.githubusercontent.com/in/4343504?s=60&v=4){kind=small}
There was a problem hiding this comment.
issue #136의 근본 원인(stateless+json_response 모드에서 SDK가 GET을 무한 standalone SSE 스트림으로 서빙 → uvicorn graceful shutdown이 영원히 대기 → hard cancel → ASGI 예외 트레이스백)을 정확히 짚고, 스펙이 허용하는 405 거부를 세션 매니저 앞단 ASGI 래퍼에서 처리한 깔끔한 수정입니다.
특히 좋은 점:
- 보안 순서 보존: 405 응답 전에
TransportSecurityMiddleware.validate_request로 동일한 DNS-rebinding Host/Origin 검증을 먼저 수행 — 악성 origin은 405로 응답받는 대신 403으로 거부되며,test_hostile_origin_get_is_rejected_not_answered_405가 이 순서를 고정합니다. - 회귀 테스트의 실질성:
test_shutdown_completes_while_a_client_holds_a_get_connection은 GET 연결을 실제로 열어둔 채(response_started이벤트로 헤더 수신 확인) shutdown을 요청하고 hard-cancel 폴백 데드라인(5s) 안에 graceful 완료를 단언하므로, 수정 전 코드에서는 타임아웃으로 실패하는 진짜 RED 테스트입니다. scope["type"] == "http"가드로 lifespan/websocket scope는 건드리지 않고,Allow: POST헤더도 스펙대로 포함.
사소한 참고(비차단): 405 경로가 GET뿐 아니라 DELETE(stateful 세션 종료용 메서드)도 거부하지만, 이 서버는 stateless라 세션 종료 개념 자체가 없으므로 올바른 동작입니다.
APPROVE
This was referenced Aug 1, 2026
Closed
hellices
added a commit
that referenced
this pull request
Aug 1, 2026
on_resources_updated can be dispatched during app teardown, after the screen stack is emptied: App.screen then raises ScreenStackError, which run_test() re-raises and fails whichever test happened to be running - the intermittent test_ctx_switch CI failures (also seen on PRs #140 and #146; the CI traceback names this exact frame). No screen simply means no tree to refresh. Fixes #147 Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #136
Problem
Quitting korvid while an external MCP host held a standalone
GET /mcpconnection printed two ERROR tracebacks over the terminal ("Exception in ASGI application" + a lifespanCancelledError).Chain (reproduced deterministically):
mcpSDK (1.28.x) serves GET as an infinite standalone SSE stream even in stateless mode withjson_response=True— a stream this server never sends anything on (no server-initiated messages exist by construction).should_exit.timeout_graceful_shutdown=None) waits forever on the open connection,MCPController.shutdown()'s 5 s deadline falls back to a hardtask.cancel(), and the CancelledError escapes the SSE task group and the lifespan.Fix
Answer anything but POST with
405 Method Not Allowedin the ASGI wrapper, before the SDK session manager. Spec-sanctioned:The TypeScript SDK's stateless mode does exactly this (405 for non-POST before any transport is constructed); upstream python-sdk PR modelcontextprotocol/python-sdk#3129 addresses only the stateful pre-session GET, so the stateless path needs this server-side guard.
Tests
test_get_is_refused_with_405_instead_of_an_sse_stream— GET gets 405, not an SSE stream.test_shutdown_completes_while_a_client_holds_a_get_connection— regression: graceful shutdown completes within the controller deadline (previously hung forever, then hard cancel, then tracebacks).Full gate (
make check) green: ruff, mypy, 2794 passed / 21 skipped, tach.