From 49ee50c1046b8caa966a8ef1475aaa28c07336c5 Mon Sep 17 00:00:00 2001 From: wangwei Date: Wed, 29 Jul 2026 17:11:54 +0800 Subject: [PATCH] =?UTF-8?q?=EF=BB=BFfix:=20harden=20MCP=20endpoint=20after?= =?UTF-8?q?=20code=20review?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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> --- .env.example | 10 ++ backend/app/config/settings.py | 14 +++ backend/app/mcp/server.py | 64 ++++++++++- backend/tests/conftest.py | 27 +++++ backend/tests/mcp/test_mcp_auth_middleware.py | 42 ++++--- .../tests/mcp/test_mcp_transport_security.py | 107 ++++++++++++++++++ .../tests/mcp/test_search_regulations_tool.py | 21 ++++ .../test_model_usage_bootstrap.py | 11 +- .../test_model_usage_persistence.py | 10 +- .../perception/test_postgres_event_store.py | 10 +- .../backend-project-architecture.md | 21 ++++ .../2026-07-29-mcp-search-regulations.md | 15 +++ ...026-07-29-mcp-search-regulations-design.md | 29 ++++- 13 files changed, 330 insertions(+), 51 deletions(-) create mode 100644 backend/tests/conftest.py create mode 100644 backend/tests/mcp/test_mcp_transport_security.py diff --git a/.env.example b/.env.example index 0aef281..6765915 100644 --- a/.env.example +++ b/.env.example @@ -166,3 +166,13 @@ AGENTIC_GROUNDING_MAX_TOKENS=250 # 逗号分隔的允许跨域来源列表，生产环境绝不能使用 * CORS_ALLOW_ORIGINS=http://localhost:5173 +# ===== MCP (Model Context Protocol) ===== +# MCP 端点（/mcp/）的 Host 头白名单，逗号分隔。MCP SDK 默认开启 DNS rebinding +# 防护，任何不在此列表中的 Host 都会被直接返回 HTTP 421，请求根本到不了鉴权和 +# 工具逻辑。因此**远程部署必须把真实访问地址写进来**，否则所有外部 MCP 客户端 +# （Claude Desktop / IDE 等）100% 连不上。 +# 语法：`:*` 后缀表示匹配任意端口；填 `*` 表示彻底关闭该防护（不推荐）。 +# 例如部署在 6.86.80.9:8000 时： +# MCP_ALLOWED_HOSTS=6.86.80.9:*,127.0.0.1:*,localhost:* +MCP_ALLOWED_HOSTS=127.0.0.1:*,localhost:*,[::1]:* + diff --git a/backend/app/config/settings.py b/backend/app/config/settings.py index eb3dcef..51c7112 100644 --- a/backend/app/config/settings.py +++ b/backend/app/config/settings.py @@ -198,6 +198,20 @@ class Settings(BaseSettings): description="Comma-separated allowed CORS origins. Never use * in production.", ) + # ── MCP ─────────────────────────────────────────────────────────────────── + # The MCP SDK enables DNS-rebinding protection whenever the transport is + # bound to a loopback host, which rejects any Host header not in this list + # with HTTP 421. Deployments reachable by a real hostname/IP must list it + # here or every remote MCP client is refused before the handler runs. + mcp_allowed_hosts: str = Field( + default="127.0.0.1:*,localhost:*,[::1]:*", + description=( + "Comma-separated Host header values accepted by the MCP endpoint. " + "A ':*' suffix matches any port. Set to '*' to disable DNS-rebinding " + "protection entirely (not recommended)." + ), + ) + @lru_cache def get_settings() -> Settings: """Return settings.""" diff --git a/backend/app/mcp/server.py b/backend/app/mcp/server.py index a4c25e6..6f40b7a 100644 --- a/backend/app/mcp/server.py +++ b/backend/app/mcp/server.py @@ -8,13 +8,20 @@ dict. No new retrieval, ranking, or LLM orchestration logic lives here. from __future__ import annotations +import logging +from typing import Annotated + from mcp.server import MCPServer +from mcp.server.transport_security import TransportSecuritySettings +from pydantic import Field from starlette.responses import PlainTextResponse from starlette.types import ASGIApp, Receive, Scope, Send from app.config.settings import settings from app.shared.bootstrap import get_agent_conversation_service, get_jwt_handler +logger = logging.getLogger(__name__) + # Single shared MCPServer instance — analogous to the single shared FastAPI # `app` instance in app/api/main.py. Tools registered via @mcp.tool() below. # Note: the installed mcp SDK (2.0.0) renamed the older "FastMCP" class to @@ -24,12 +31,22 @@ mcp = MCPServer("ai-regulations") @mcp.tool() -def search_regulations(query: str, top_k: int = 5) -> dict: +def search_regulations( + query: Annotated[str, Field(min_length=1, max_length=2000)], + top_k: Annotated[int, Field(ge=1, le=20)] = 5, +) -> dict: """Search the compliance knowledge base and return a grounded answer. query: Natural-language search question, e.g. "国六排放标准最新要求". - top_k: Maximum number of cited sources to return (default 5). + top_k: Maximum number of cited sources to return (1-20, default 5). """ + # Bounds mirror AskRequest in app/api/models/agent.py so the MCP path cannot + # be used to bypass the REST endpoint's limits. They matter more here than + # there: KnowledgeRetrievalService amplifies top_k (candidate_k = top_k * 4) + # when reranking, and an LLM client can easily hallucinate a huge value. + # Declaring them via Annotated puts them in the advertised JSON schema too, + # so well-behaved clients never send an out-of-range value in the first place. + # # No session_id is passed: this keeps each call stateless (no # ConversationStore reads/writes), matching "search" semantics rather # than multi-turn chat semantics. @@ -61,19 +78,53 @@ class MCPAuthMiddleware: return headers = dict(scope["headers"]) - auth_header = headers.get(b"authorization", b"").decode() + # ASGI header values are raw bytes specified as latin-1, not UTF-8; + # decoding strictly as UTF-8 would raise on a malformed byte and turn a + # bad request into an unhandled 500. + auth_header = headers.get(b"authorization", b"").decode("latin-1") token = auth_header.removeprefix("Bearer ").strip() try: get_jwt_handler().decode_token(token) except ValueError as exc: # Reject before the MCP session/protocol layer ever sees the request. - response = PlainTextResponse(str(exc), status_code=401) + # WWW-Authenticate matches the get_current_user dependency (auth.py) + # and is required by RFC 7235 so clients can tell "needs credentials" + # apart from a generic failure. + response = PlainTextResponse( + str(exc), status_code=401, headers={"WWW-Authenticate": "Bearer"} + ) await response(scope, receive, send) return await self.app(scope, receive, send) +def _build_transport_security() -> TransportSecuritySettings: + """Translate the configured MCP host allow-list into SDK transport settings. + + Without this the SDK infers its own allow-list from the bind host, which + defaults to 127.0.0.1 and therefore rejects every remote client with HTTP + 421 — fatal for a remotely deployed backend. + """ + allowed = [h.strip() for h in settings.mcp_allowed_hosts.split(",") if h.strip()] + if "*" in allowed: + # Explicit, logged opt-out. Kept as an escape hatch for environments + # behind a proxy that rewrites Host unpredictably, but never the default. + logger.warning( + "MCP DNS-rebinding protection is disabled (mcp_allowed_hosts='*'). " + "Set MCP_ALLOWED_HOSTS to the real deployment host(s) instead." + ) + return TransportSecuritySettings(enable_dns_rebinding_protection=False) + return TransportSecuritySettings( + enable_dns_rebinding_protection=True, + allowed_hosts=allowed, + # Browser clients send Origin; reuse the already-maintained CORS list so + # there is one place to declare trusted web origins. Non-browser MCP + # clients send no Origin at all, which the SDK treats as allowed. + allowed_origins=[o.strip() for o in settings.cors_allow_origins.split(",") if o.strip()], + ) + + def build_mcp_asgi_app() -> ASGIApp: """Return the Streamable HTTP ASGI app for the MCP server, auth-guarded. @@ -83,6 +134,9 @@ def build_mcp_asgi_app() -> ASGIApp: path to "/", the effective external path would be the confusing "/mcp/mcp" instead of "/mcp". """ - asgi_app = mcp.streamable_http_app(streamable_http_path="/") + asgi_app = mcp.streamable_http_app( + streamable_http_path="/", + transport_security=_build_transport_security(), + ) asgi_app.add_middleware(MCPAuthMiddleware) return asgi_app diff --git a/backend/tests/conftest.py b/backend/tests/conftest.py new file mode 100644 index 0000000..4e91bee --- /dev/null +++ b/backend/tests/conftest.py @@ -0,0 +1,27 @@ +"""Shared pytest fixtures and import-time guards for the backend test suite. + +pytest imports this file before any test module beneath backend/tests/, which +makes it the only reliable place to install import-time guards: individual test +modules cannot guarantee they run first, because collection order follows +directory names. +""" + +from __future__ import annotations + +import sys +from unittest.mock import MagicMock + +# app/shared/bootstrap.py (the composition root) eagerly imports the Postgres +# store modules, which do `import psycopg2` at their own module scope and later +# open a real connection pool. Any test that transitively imports bootstrap +# would therefore bind the real driver and attempt a live TCP connection to the +# configured production database, surfacing as a multi-second timeout rather +# than an obvious error. Binding mocks here — before the first test module is +# imported — makes that impossible regardless of collection order. +# setdefault (not assignment) keeps a real psycopg2 in place if something has +# already imported it deliberately. +_mock_psycopg2 = MagicMock() +_mock_psycopg2.extras = MagicMock() +sys.modules.setdefault("psycopg2", _mock_psycopg2) +sys.modules.setdefault("psycopg2.extras", _mock_psycopg2.extras) +sys.modules.setdefault("psycopg2.pool", MagicMock()) diff --git a/backend/tests/mcp/test_mcp_auth_middleware.py b/backend/tests/mcp/test_mcp_auth_middleware.py index 1116e7d..03a42ef 100644 --- a/backend/tests/mcp/test_mcp_auth_middleware.py +++ b/backend/tests/mcp/test_mcp_auth_middleware.py @@ -7,23 +7,7 @@ independent of FastMCP internals. from __future__ import annotations -import sys -from unittest.mock import MagicMock, patch - -# app.mcp.server imports app.shared.bootstrap at module scope (needed for -# get_agent_conversation_service/get_jwt_handler), which in turn eagerly -# imports several Postgres store modules that do `import psycopg2` at their -# own module scope. Since this is the only test file in backend/tests/mcp/ -# that imports app.mcp.server at module scope (not inside a test function), -# it is the first thing to trigger that chain during pytest collection — -# guard psycopg2 here the same way backend/tests/observability/ -# test_model_usage_bootstrap.py and test_model_usage_persistence.py already -# do, so a real (network-connecting) psycopg2 never gets bound first. -mock_psycopg2 = MagicMock() -mock_psycopg2.extras = MagicMock() -sys.modules.setdefault("psycopg2", mock_psycopg2) -sys.modules.setdefault("psycopg2.extras", mock_psycopg2.extras) -sys.modules.setdefault("psycopg2.pool", MagicMock()) +from unittest.mock import patch from starlette.applications import Starlette from starlette.responses import PlainTextResponse @@ -83,3 +67,27 @@ def test_auth_disabled_always_passes_through(): client = TestClient(_dummy_app()) response = client.get("/ping") assert response.status_code == 200 + + +def test_401_includes_www_authenticate_header(): + """RFC 7235 requires WWW-Authenticate on 401 so clients can tell why they failed.""" + with patch("app.mcp.server.settings") as fake_settings: + fake_settings.auth_enabled = True + client = TestClient(_dummy_app()) + response = client.get("/ping") + assert response.status_code == 401 + assert response.headers["WWW-Authenticate"] == "Bearer" + + +def test_non_utf8_authorization_header_is_rejected_not_crashed(): + """A non-UTF-8 header byte must yield a clean 401, not an unhandled 500. + + ASGI header values are latin-1 bytes, so any remote client could otherwise + trigger a UnicodeDecodeError inside the middleware at will. + """ + with patch("app.mcp.server.settings") as fake_settings: + fake_settings.auth_enabled = True + client = TestClient(_dummy_app(), raise_server_exceptions=False) + # Bypass the http client's own header encoding by writing raw bytes. + response = client.get("/ping", headers={"Authorization": b"Bearer \xff\xfe"}) + assert response.status_code == 401 diff --git a/backend/tests/mcp/test_mcp_transport_security.py b/backend/tests/mcp/test_mcp_transport_security.py new file mode 100644 index 0000000..273873b --- /dev/null +++ b/backend/tests/mcp/test_mcp_transport_security.py @@ -0,0 +1,107 @@ +"""Tests for the MCP endpoint's DNS-rebinding (Host header) protection. + +The MCP SDK auto-enables DNS-rebinding protection and derives its allow-list +from the bind host, which defaults to 127.0.0.1. Left alone, that rejects every +request whose Host header is the real deployment address (6.86.80.9:8000) with +HTTP 421 — before the auth middleware or the tool ever runs. These tests pin +the configured allow-list behavior so that failure mode cannot come back. +""" + +from __future__ import annotations + +import json +from contextlib import contextmanager +from unittest.mock import patch + +from starlette.testclient import TestClient + +from app.mcp.server import _build_transport_security, build_mcp_asgi_app + +# A minimal JSON-RPC initialize call. Reaching the MCP handler at all is what +# matters here; transport security rejects the request long before this body is +# parsed, so its exact contents only need to be structurally valid. +_INITIALIZE = { + "jsonrpc": "2.0", + "id": 1, + "method": "initialize", + "params": { + "protocolVersion": "2025-06-18", + "capabilities": {}, + "clientInfo": {"name": "test", "version": "1.0"}, + }, +} + +_HEADERS = { + "Content-Type": "application/json", + "Accept": "application/json, text/event-stream", +} + + +@contextmanager +def _mcp_client(allowed_hosts: str): + """Yield a TestClient over the real MCP app with auth off and hosts configured. + + The settings patch must stay active for the requests themselves, not just + for app construction, because MCPAuthMiddleware reads settings per request. + Entering the TestClient as a context manager is also required: it runs the + app's lifespan, without which the SDK's session manager task group is never + initialized and every request raises RuntimeError. + """ + with patch("app.mcp.server.settings") as fake_settings: + fake_settings.mcp_allowed_hosts = allowed_hosts + fake_settings.cors_allow_origins = "http://localhost:5173" + fake_settings.auth_enabled = False + with TestClient(build_mcp_asgi_app()) as client: + yield client + + +def test_remote_host_allowed_when_configured(): + """A configured non-loopback Host must reach the MCP handler, not 421.""" + with _mcp_client("6.86.80.9:*,127.0.0.1:*") as client: + response = client.post( + "/", json=_INITIALIZE, headers={**_HEADERS, "Host": "6.86.80.9:8000"} + ) + assert response.status_code == 200 + assert "Invalid Host header" not in response.text + + +def test_unconfigured_host_still_rejected(): + """Protection must stay on: a Host outside the allow-list is refused with 421.""" + with _mcp_client("6.86.80.9:*") as client: + response = client.post( + "/", json=_INITIALIZE, headers={**_HEADERS, "Host": "evil.example.com"} + ) + assert response.status_code == 421 + + +def test_initialize_response_is_event_stream(): + """Sanity check that a permitted request really completes the MCP handshake.""" + with _mcp_client("6.86.80.9:*") as client: + response = client.post( + "/", json=_INITIALIZE, headers={**_HEADERS, "Host": "6.86.80.9:8000"} + ) + assert response.status_code == 200 + # The Streamable HTTP transport replies as SSE; the JSON-RPC result is + # embedded in a "data:" line rather than being the whole body. + payload = json.loads(response.text.split("data:", 1)[1].strip()) + assert payload["result"]["serverInfo"]["name"] == "ai-regulations" + + +def test_wildcard_disables_protection_explicitly(): + """'*' is the documented opt-out; it must disable the check, not allow-list '*'.""" + with patch("app.mcp.server.settings") as fake_settings: + fake_settings.mcp_allowed_hosts = "*" + fake_settings.cors_allow_origins = "http://localhost:5173" + security = _build_transport_security() + assert security.enable_dns_rebinding_protection is False + + +def test_allow_list_is_parsed_into_transport_settings(): + """Comma-separated config must become the SDK's allowed_hosts list verbatim.""" + with patch("app.mcp.server.settings") as fake_settings: + fake_settings.mcp_allowed_hosts = "6.86.80.9:*, localhost:* ," + fake_settings.cors_allow_origins = "http://localhost:5173" + security = _build_transport_security() + assert security.enable_dns_rebinding_protection is True + assert security.allowed_hosts == ["6.86.80.9:*", "localhost:*"] + assert security.allowed_origins == ["http://localhost:5173"] diff --git a/backend/tests/mcp/test_search_regulations_tool.py b/backend/tests/mcp/test_search_regulations_tool.py index 0a416cd..3d4e7e0 100644 --- a/backend/tests/mcp/test_search_regulations_tool.py +++ b/backend/tests/mcp/test_search_regulations_tool.py @@ -7,6 +7,7 @@ correct dict shape out. from __future__ import annotations +import asyncio from dataclasses import dataclass from unittest.mock import MagicMock, patch @@ -15,6 +16,9 @@ from unittest.mock import MagicMock, patch class _FakeSource: """Minimal stand-in for a real Source dataclass (only __dict__ is used).""" + # A dataclass, not a MagicMock: the adapter serializes sources via + # source.__dict__, and a MagicMock's __dict__ is full of internal mock + # attributes, which would make the assertions meaningless. doc_id: str doc_title: str score: float @@ -75,3 +79,20 @@ def test_search_regulations_default_top_k(): search_regulations(query="q") assert fake_service.ask.call_args.kwargs["top_k"] == 5 + + +def test_advertised_schema_bounds_top_k_and_query(): + """The advertised JSON schema must carry the same bounds as AskRequest. + + Bounds declared via Annotated are what the SDK validates against and what + clients see, so asserting on the generated schema is the only way to catch + a regression that silently drops them. + """ + from app.mcp.server import mcp + + schema = asyncio.run(mcp.list_tools())[0].input_schema["properties"] + + assert schema["top_k"]["minimum"] == 1 + assert schema["top_k"]["maximum"] == 20 + assert schema["query"]["minLength"] == 1 + assert schema["query"]["maxLength"] == 2000 diff --git a/backend/tests/observability/test_model_usage_bootstrap.py b/backend/tests/observability/test_model_usage_bootstrap.py index 67163aa..fa07fd7 100644 --- a/backend/tests/observability/test_model_usage_bootstrap.py +++ b/backend/tests/observability/test_model_usage_bootstrap.py @@ -12,17 +12,10 @@ is needed anywhere in this file — asyncio.create_task itself is also mocked. from __future__ import annotations -import sys from unittest.mock import MagicMock, patch -# Patch psycopg2 before importing anything that transitively imports it, in -# case this file is collected before test_model_usage_persistence.py. -mock_psycopg2 = MagicMock() -mock_psycopg2.extras = MagicMock() -sys.modules.setdefault("psycopg2", mock_psycopg2) -sys.modules.setdefault("psycopg2.extras", mock_psycopg2.extras) -sys.modules.setdefault("psycopg2.pool", MagicMock()) - +# psycopg2 is mocked centrally in backend/tests/conftest.py, which pytest +# imports before any test module regardless of collection order. from app.shared import bootstrap from app.shared.model_usage_tracker import ModelUsageEntry, ModelUsageTracker diff --git a/backend/tests/observability/test_model_usage_persistence.py b/backend/tests/observability/test_model_usage_persistence.py index cf7f393..9fe7311 100644 --- a/backend/tests/observability/test_model_usage_persistence.py +++ b/backend/tests/observability/test_model_usage_persistence.py @@ -6,17 +6,11 @@ Mirrors the mocking pattern in backend/tests/perception/test_postgres_event_stor from __future__ import annotations -import sys from datetime import datetime, timezone from unittest.mock import MagicMock, patch -# Patch psycopg2 before importing the module under test. -mock_psycopg2 = MagicMock() -mock_psycopg2.extras = MagicMock() -sys.modules.setdefault("psycopg2", mock_psycopg2) -sys.modules.setdefault("psycopg2.extras", mock_psycopg2.extras) -sys.modules.setdefault("psycopg2.pool", MagicMock()) - +# psycopg2 is mocked centrally in backend/tests/conftest.py, so importing the +# module under test here never binds the real driver. from app.shared.model_usage_tracker import ModelUsageEntry diff --git a/backend/tests/perception/test_postgres_event_store.py b/backend/tests/perception/test_postgres_event_store.py index 95957bf..fb98df3 100644 --- a/backend/tests/perception/test_postgres_event_store.py +++ b/backend/tests/perception/test_postgres_event_store.py @@ -4,14 +4,8 @@ import json from unittest.mock import MagicMock, patch import pytest -# Patch psycopg2 before importing the module under test -import sys -mock_psycopg2 = MagicMock() -mock_psycopg2.extras = MagicMock() -sys.modules.setdefault("psycopg2", mock_psycopg2) -sys.modules.setdefault("psycopg2.extras", mock_psycopg2.extras) -sys.modules.setdefault("psycopg2.pool", MagicMock()) - +# psycopg2 is mocked centrally in backend/tests/conftest.py, so importing the +# module under test here never binds the real driver. from app.infrastructure.perception.base_event_store import BaseEventStore diff --git a/docs/architecture/backend-project-architecture.md b/docs/architecture/backend-project-architecture.md index d968fd9..f527287 100644 --- a/docs/architecture/backend-project-architecture.md +++ b/docs/architecture/backend-project-architecture.md @@ -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 逻辑 diff --git a/docs/superpowers/plans/2026-07-29-mcp-search-regulations.md b/docs/superpowers/plans/2026-07-29-mcp-search-regulations.md index ec88727..bb27462 100644 --- a/docs/superpowers/plans/2026-07-29-mcp-search-regulations.md +++ b/docs/superpowers/plans/2026-07-29-mcp-search-regulations.md @@ -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`. diff --git a/docs/superpowers/specs/2026-07-29-mcp-search-regulations-design.md b/docs/superpowers/specs/2026-07-29-mcp-search-regulations-design.md index 64b68e5..821dab0 100644 --- a/docs/superpowers/specs/2026-07-29-mcp-search-regulations-design.md +++ b/docs/superpowers/specs/2026-07-29-mcp-search-regulations-design.md @@ -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