fix: harden MCP endpoint after code review
Critical: the MCP SDK auto-enables DNS-rebinding protection when its host parameter is left at the 127.0.0.1 default, hard-coding a loopback-only Host allow-list. Every remote client (the only deployment this feature targets) was refused with HTTP 421 before auth or the tool ran. Now driven by a new MCP_ALLOWED_HOSTS setting, with '*' as an explicit, logged opt-out. Also bounds query/top_k to match AskRequest (top_k is amplified 4x downstream, so an unbounded value was a resource-exhaustion vector), decodes the Authorization header as latin-1 per the ASGI spec instead of raising a 500 on malformed bytes, and returns WWW-Authenticate on 401 per RFC 7235. Moves the psycopg2 import guard into backend/tests/conftest.py: duplicated across four test modules, it only worked because of alphabetical collection order, and any earlier-sorting package would have reintroduced a live connection attempt against the production database. Registers the mcp module in the authoritative backend architecture doc. 84 backend tests pass. Verified against a live server: allowed remote Host returns a valid initialize result, unknown Host returns 421, missing token returns 401 with WWW-Authenticate. Co-authored-by: Copilot <223556219+Copilot@users.noreply.github.com>
This commit is contained in:
@@ -206,6 +206,7 @@
|
||||
```text
|
||||
backend/app/
|
||||
api/
|
||||
mcp/
|
||||
application/
|
||||
documents/
|
||||
knowledge/
|
||||
@@ -314,6 +315,25 @@ backend/app/
|
||||
- `backend/app/shared/bootstrap.py` 是现阶段的 composition root,负责把端口实现、基础设施适配器和 application service 连接起来。
|
||||
- 后续如果新增 wiring 入口,应继续保持在同一类装配边界内,不要把依赖装配拆回各个路由或 service 构造函数中。
|
||||
|
||||
### 4.6 `mcp`
|
||||
|
||||
职责:
|
||||
|
||||
- 以 Model Context Protocol 对外暴露平台已有能力
|
||||
- MCP tool 注册与入参 schema 绑定
|
||||
- MCP 专用鉴权(复用现有 JWT)与 Streamable HTTP 子 ASGI 应用装配
|
||||
|
||||
非职责:
|
||||
|
||||
- 不实现任何新的检索、问答或业务编排逻辑
|
||||
- 不直接访问 Milvus、MinIO、LLM SDK
|
||||
|
||||
说明:
|
||||
|
||||
- `mcp` 与 `api` 是并列的两个 transport 适配层:`api` 面向 HTTP REST 客户端,`mcp` 面向 MCP 客户端(Claude Desktop、IDE 等)。二者共用同一套 application service。
|
||||
- 它独立成顶层模块而不是放进 `api/routes/`,因为 MCP 使用装饰器式 tool 注册和自带的子 ASGI 应用,与 `APIRouter` 是不同的传输机制。
|
||||
- 当前实现见 `backend/app/mcp/server.py`,只暴露 `search_regulations` 一个 tool,内部直接调用 `get_agent_conversation_service()`。
|
||||
|
||||
## 5. Module Responsibilities
|
||||
|
||||
### 5.1 `api`
|
||||
@@ -637,6 +657,7 @@ infrastructure -> external systems
|
||||
具体规则如下:
|
||||
|
||||
- `api` 可以依赖 `application` 和 API 自己的 request/response models
|
||||
- `mcp` 与 `api` 同级,只能依赖 `application` 和 composition root,不能依赖 `infrastructure` 或反过来被 `application` 依赖
|
||||
- `application` 只能依赖 `domain`、端口接口,以及通过 composition root 注入进来的实现实例
|
||||
- `domain` 不能依赖 `api` 或 `infrastructure`
|
||||
- `infrastructure` 可以依赖 `domain` 定义的端口和数据模型,但不能反向驱动 application 逻辑
|
||||
|
||||
@@ -469,3 +469,18 @@ Confirm `search_regulations` appears in the tool list and returns a real answer
|
||||
| 1 | `app/mcp/__init__.py`, `app/mcp/server.py`, `tests/mcp/__init__.py`, `tests/mcp/test_search_regulations_tool.py` | — | 3 |
|
||||
| 2 | `tests/mcp/test_mcp_auth_middleware.py` | `app/mcp/server.py` | 4 |
|
||||
| 3 | — | `requirements.txt`, `api/main.py` | 0 (full-suite regression check + manual e2e) |
|
||||
|
||||
## Task 4 (post-review): code-review fixes
|
||||
|
||||
Added after a code review of the three implementation commits. Findings and resolutions:
|
||||
|
||||
- [x] **Critical — every remote client rejected with HTTP 421.** The MCP SDK auto-enables DNS-rebinding protection when its `host` parameter is left at the `127.0.0.1` default, hard-coding a loopback-only `Host` allow-list; a client at `http://6.86.80.9:8000/mcp/` was refused before auth or the tool ran, making the feature non-functional in the only deployment it targets. Fixed by passing an explicit `TransportSecuritySettings` built from a new `MCP_ALLOWED_HOSTS` setting (`app/config/settings.py`, documented in `.env.example`), with `*` as a logged, explicit opt-out. Deliberately *not* fixed by passing `host="0.0.0.0"`, which would silently disable the protection.
|
||||
- [x] **Important — `top_k` unbounded on the MCP path.** `AskRequest` constrains the same parameter to 1–20, but the tool accepted any integer and `KnowledgeRetrievalService` amplifies it (`top_k * 4`), so `top_k=100000` would request 400,000 Milvus candidates. Fixed with `Annotated[int, Field(ge=1, le=20)]` (and `query` bounded to 1–2000 chars), which also publishes the bounds in the advertised JSON schema.
|
||||
- [x] **Important — order-dependent `psycopg2` test guard.** The guard was duplicated across four test modules and only worked because of pytest's alphabetical collection order; any new test package sorting earlier would have reintroduced a multi-second TCP timeout against the production database. Moved into a single `backend/tests/conftest.py` (imported before any test module regardless of order) and the four in-file copies deleted. `bootstrap.py`'s eager imports were left alone — restructuring the composition root every route depends on is disproportionate to a test-harness ordering problem.
|
||||
- [x] **Minor — non-UTF-8 `Authorization` header caused a 500.** ASGI header values are latin-1; strict UTF-8 decoding let any remote client trigger an unhandled `UnicodeDecodeError`. Now decoded as latin-1, yielding a clean 401.
|
||||
- [x] **Minor — 401 missing `WWW-Authenticate`.** Added `WWW-Authenticate: Bearer`, matching `get_current_user` and RFC 7235.
|
||||
- [x] **Minor — missing `#` comment** in `tests/mcp/test_search_regulations_tool.py` (AGENTS.md requires at least one per file). Added.
|
||||
|
||||
Reviewer-confirmed as correct, no change needed: the `AsyncExitStack` lifespan wiring (including its failure path), the absence of auth-bypass vectors, and the statelessness of `ask()` without a `session_id`.
|
||||
|
||||
New tests: `tests/mcp/test_mcp_transport_security.py` (5, exercising the real MCP app end-to-end) plus 3 more across the existing two files — 84 backend tests pass. Verified against a live server: `Host: 6.86.80.9:8000` → 200 with a valid `initialize` result, `Host: evil.example.com` → 421, no token → 401 with `WWW-Authenticate: Bearer`.
|
||||
|
||||
@@ -157,19 +157,40 @@ class MCPAuthMiddleware:
|
||||
|
||||
JWTs expire after `expire_minutes` (480 by default, see `JWTHandler`) — long-lived external tool connections will need a token refresh story, but that is an existing limitation of the JWT scheme generally (not new to MCP), so it is not addressed differently here.
|
||||
|
||||
### Transport security (Host allow-list)
|
||||
|
||||
> **Added post-design after code review.** This was missed in the original design and would have made the feature 100% non-functional in the target deployment.
|
||||
|
||||
The MCP SDK enables DNS-rebinding protection automatically whenever the transport's bind host is a loopback address (its `host` parameter defaults to `127.0.0.1`), and then hard-codes the allow-list to `127.0.0.1:*`, `localhost:*`, `[::1]:*`. `TransportSecurityMiddleware` rejects any request whose `Host` header is not on that list with **HTTP 421**, *before* `MCPAuthMiddleware` or the tool runs. A client pointed at `http://6.86.80.9:8000/mcp/` sends `Host: 6.86.80.9:8000` and is therefore refused every time.
|
||||
|
||||
The module resolves this by passing an explicit `TransportSecuritySettings` built from a new setting, `MCP_ALLOWED_HOSTS` (comma-separated, `:*` suffix matches any port, documented in `.env.example`):
|
||||
|
||||
- Default `127.0.0.1:*,localhost:*,[::1]:*` — safe for local development.
|
||||
- Deployments must add their real address, e.g. `MCP_ALLOWED_HOSTS=6.86.80.9:*,127.0.0.1:*,localhost:*`.
|
||||
- The literal value `*` disables the protection entirely. This is deliberately an explicit, log-warned opt-out rather than the default, since binding to `0.0.0.0` to sidestep the check would silently switch DNS-rebinding protection off.
|
||||
- `allowed_origins` reuses the existing `CORS_ALLOW_ORIGINS` list, so trusted browser origins are declared in exactly one place. Non-browser MCP clients send no `Origin` header, which the SDK treats as allowed.
|
||||
|
||||
### Tool input bounds
|
||||
|
||||
`search_regulations` declares `query` as 1–2000 characters and `top_k` as 1–20 via `Annotated[..., Field(...)]`, matching `AskRequest` in `app/api/models/agent.py`. This is load-bearing rather than cosmetic: `KnowledgeRetrievalService.retrieve()` amplifies the value (`candidate_k = max(top_k * 4, 20)`) when reranking is active, so an unbounded `top_k` is a cheap resource-exhaustion vector — and an LLM client hallucinating a large value is the likelier trigger than an attacker. Declaring the bounds via `Annotated` also publishes them in the advertised JSON schema, so well-behaved clients never send an out-of-range value at all.
|
||||
|
||||
---
|
||||
|
||||
## Error Handling
|
||||
|
||||
- **Auth failure** (missing/expired/invalid token, when `auth_enabled=True`): HTTP 401 from `MCPAuthMiddleware`, before the MCP protocol layer is invoked at all.
|
||||
- **Auth failure** (missing/expired/invalid token, when `auth_enabled=True`): HTTP 401 from `MCPAuthMiddleware`, before the MCP protocol layer is invoked at all. The response carries `WWW-Authenticate: Bearer`, matching the `get_current_user` dependency and RFC 7235.
|
||||
- **Malformed `Authorization` header bytes**: ASGI header values are latin-1, so the middleware decodes as latin-1; a non-UTF-8 byte yields a normal 401 rather than an unhandled `UnicodeDecodeError`/500.
|
||||
- **Rejected `Host` header**: HTTP 421 from the SDK's transport-security middleware (see above), before auth.
|
||||
- **Tool execution failure** (e.g. the underlying retrieval/LLM call raises): FastMCP's own tool-call error handling catches exceptions raised inside `@mcp.tool()`-decorated functions and returns them as a normal MCP tool-error result to the calling client — no special handling needed in `search_regulations` itself, consistent with how `/agent/ask`'s REST handler already lets the global FastAPI exception handler in `main.py` catch unexpected errors.
|
||||
- **Lifespan startup failure** (e.g. `mcp_app`'s session manager fails to start): surfaces the same way any other `lifespan()` failure does today — the app fails to start, visible immediately in logs, not a silent partial-degradation.
|
||||
|
||||
## Testing
|
||||
|
||||
- `backend/tests/mcp/test_search_regulations_tool.py` — unit test for the tool function with a mocked `AgentConversationService` (same mocking style as existing `application/agent` tests): asserts `search_regulations()` calls `.ask(query=..., top_k=...)` with no `session_id`, and shapes the returned dict correctly (`answer`, `sources`).
|
||||
- `backend/tests/mcp/test_mcp_auth_middleware.py` — unit test for `MCPAuthMiddleware`: no token → 401; invalid/expired token → 401; valid token → request passed through to the wrapped app; `auth_enabled=False` → always passed through. Uses Starlette's `TestClient` against a minimal dummy inner ASGI app, no real MCP protocol handshake needed.
|
||||
- **Manual end-to-end verification** (not automated): use the official `mcp` Python client (`mcp.client.streamable_http.streamablehttp_client` + `mcp.ClientSession`) to connect to a locally running instance, call `list_tools()`, then call `search_regulations` with a real query, confirming a real answer + sources come back. This is a one-time manual check, not a CI test — full MCP protocol handshake testing would require standing up the `mcp` client SDK as a test dependency for marginal additional confidence beyond the two unit tests above.
|
||||
- `backend/tests/mcp/test_search_regulations_tool.py` — unit test for the tool function with a mocked `AgentConversationService` (same mocking style as existing `application/agent` tests): asserts `search_regulations()` calls `.ask(query=..., top_k=...)` with no `session_id`, shapes the returned dict correctly (`answer`, `sources`), and that the advertised JSON schema carries the `query`/`top_k` bounds.
|
||||
- `backend/tests/mcp/test_mcp_auth_middleware.py` — unit test for `MCPAuthMiddleware`: no token → 401; invalid/expired token → 401; valid token → request passed through to the wrapped app; `auth_enabled=False` → always passed through; 401 carries `WWW-Authenticate`; non-UTF-8 header bytes → 401 not 500. Uses Starlette's `TestClient` against a minimal dummy inner ASGI app, no real MCP protocol handshake needed.
|
||||
- `backend/tests/mcp/test_mcp_transport_security.py` — exercises the **real** MCP app over `TestClient`: a configured remote `Host` completes a real JSON-RPC `initialize` handshake; an unconfigured `Host` is refused with 421; `*` disables protection; the comma-separated setting parses correctly. These run the app's lifespan via `with TestClient(...)`, without which the SDK's session-manager task group is uninitialized.
|
||||
- `backend/tests/conftest.py` — mocks `psycopg2` at import time for the whole suite. Individual test modules cannot do this reliably, because whether a module runs before the one that needs the mock depends on alphabetical collection order; `conftest.py` is imported before any test module in the tree.
|
||||
- **Manual end-to-end verification** (not automated): use the official `mcp` Python client (`mcp.client.streamable_http.streamable_http_client` + `mcp.ClientSession`) to connect to a locally running instance, call `list_tools()`, then call `search_regulations` with a real query, confirming a real answer + sources come back. This is a one-time manual check, not a CI test.
|
||||
|
||||
## Dependencies
|
||||
|
||||
|
||||
Reference in New Issue
Block a user