mirror of
https://github.com/agentscope-ai/ReMe.git
synced 2026-09-09 22:31:05 +00:00
fix(service): preserve MCP request protections
Route the exact MCP path through the complete FastMCP ASGI application so its middleware and state remain active. Reject non-literal MCP paths and validate reserved Job conflicts before tolerant service registration. Add regression coverage for middleware preservation, route syntax, and startup failure.
This commit is contained in:
parent
bb60045070
commit
4b5d34c8bc
2 changed files with 71 additions and 4 deletions
|
|
@ -11,6 +11,7 @@ from fastapi import FastAPI, HTTPException
|
|||
from fastapi.middleware.cors import CORSMiddleware
|
||||
from fastapi.responses import FileResponse, StreamingResponse
|
||||
from fastapi.staticfiles import StaticFiles
|
||||
from starlette.routing import Route
|
||||
|
||||
from .base_service import BaseService
|
||||
from ..component_registry import R
|
||||
|
|
@ -92,9 +93,31 @@ class HttpService(BaseService):
|
|||
allow_headers=["*"],
|
||||
)
|
||||
if self.mcp_app is not None:
|
||||
# Mounting at /mcp makes Starlette redirect to /mcp/. Merge the
|
||||
# generated routes so the configured path remains canonical.
|
||||
self.service.router.routes.extend(self.mcp_app.routes)
|
||||
# Forward the exact path to the complete FastMCP ASGI app. Copying
|
||||
# only its routes would bypass its middleware and application state;
|
||||
# mounting it would make the trailing-slash path canonical instead.
|
||||
self.service.router.routes.append(
|
||||
Route(
|
||||
self.mcp_path,
|
||||
endpoint=self.mcp_app,
|
||||
include_in_schema=False,
|
||||
),
|
||||
)
|
||||
|
||||
def add_jobs(self, app: "Application") -> None:
|
||||
"""Validate reserved routes before the shared tolerant registration loop."""
|
||||
if self.mcp_enabled:
|
||||
conflicts = sorted(
|
||||
job.name
|
||||
for name, job in app.context.jobs.items()
|
||||
if job.enable_serve and (self.jobs is None or name in self.jobs) and f"/{job.name}" == self.mcp_path
|
||||
)
|
||||
if conflicts:
|
||||
names = ", ".join(conflicts)
|
||||
raise ValueError(
|
||||
f"Job name conflicts with the MCP endpoint {self.mcp_path!r}: {names}",
|
||||
)
|
||||
super().add_jobs(app)
|
||||
|
||||
def add_job(self, job: BaseJob) -> bool:
|
||||
"""Register HTTP routes for every job and MCP tools for non-stream jobs."""
|
||||
|
|
@ -180,6 +203,12 @@ class HttpService(BaseService):
|
|||
raise ValueError(
|
||||
"mcp_path must start with '/', must not be '/', and must not end with '/'",
|
||||
)
|
||||
if "//" in path or any(segment in {".", ".."} for segment in path.split("/")):
|
||||
raise ValueError("mcp_path must use non-empty literal path segments")
|
||||
if any(char in path for char in "{}?#\\") or any(
|
||||
char.isspace() or ord(char) < 32 or ord(char) == 127 for char in path
|
||||
):
|
||||
raise ValueError("mcp_path must be a literal URL path without route, query, or fragment syntax")
|
||||
if path in {"/assets", "/docs", "/redoc", "/openapi.json"}:
|
||||
raise ValueError(f"mcp_path conflicts with reserved HTTP path {path!r}")
|
||||
return path
|
||||
|
|
|
|||
|
|
@ -7,6 +7,7 @@ from types import ModuleType, SimpleNamespace
|
|||
|
||||
import pytest
|
||||
from fastapi.testclient import TestClient
|
||||
from starlette.middleware.base import BaseHTTPMiddleware
|
||||
|
||||
from reme.components.service.http_service import HttpService
|
||||
from reme.components.job import BaseJob, StreamJob
|
||||
|
|
@ -133,6 +134,19 @@ def test_http_service_uses_exact_mcp_path_and_runs_one_application_lifespan() ->
|
|||
app = CountingApplication()
|
||||
service = HttpService(web_enabled=False)
|
||||
service.build_service(app) # type: ignore[arg-type]
|
||||
mcp_server = service.mcp_server
|
||||
|
||||
class VerifyFastMCPAppMiddleware(BaseHTTPMiddleware):
|
||||
"""Prove requests retain FastMCP middleware and application state."""
|
||||
|
||||
async def dispatch(self, request, call_next):
|
||||
"""Verify the child app context and mark its response."""
|
||||
assert request.app.state.fastmcp_server is mcp_server
|
||||
response = await call_next(request)
|
||||
response.headers["X-FastMCP-Middleware"] = "preserved"
|
||||
return response
|
||||
|
||||
service.mcp_app.add_middleware(VerifyFastMCPAppMiddleware)
|
||||
|
||||
with TestClient(service.service, follow_redirects=False) as client:
|
||||
response = client.post(
|
||||
|
|
@ -154,6 +168,7 @@ def test_http_service_uses_exact_mcp_path_and_runs_one_application_lifespan() ->
|
|||
)
|
||||
assert response.status_code == 200
|
||||
assert '"serverInfo"' in response.text
|
||||
assert response.headers["X-FastMCP-Middleware"] == "preserved"
|
||||
assert client.post("/mcp/mcp", json={}).status_code == 404
|
||||
|
||||
assert app.start_count == 1
|
||||
|
|
@ -171,7 +186,18 @@ def test_http_service_can_disable_mcp() -> None:
|
|||
|
||||
def test_http_service_rejects_invalid_or_conflicting_mcp_paths() -> None:
|
||||
"""Reject paths that shadow built-ins and jobs that shadow the MCP endpoint."""
|
||||
for path in ("mcp", "/", "/mcp/", "/docs"):
|
||||
for path in (
|
||||
"mcp",
|
||||
"/",
|
||||
"/mcp/",
|
||||
"/docs",
|
||||
"/{rest:path}",
|
||||
"/mcp?mode=test",
|
||||
"/mcp#fragment",
|
||||
"/mcp path",
|
||||
"/mcp//nested",
|
||||
"/mcp/../nested",
|
||||
):
|
||||
with pytest.raises(ValueError, match="mcp_path"):
|
||||
HttpService(mcp_path=path)
|
||||
|
||||
|
|
@ -181,6 +207,18 @@ def test_http_service_rejects_invalid_or_conflicting_mcp_paths() -> None:
|
|||
service.add_job(BaseJob(name="mcp"))
|
||||
|
||||
|
||||
def test_http_service_fails_startup_preflight_for_mcp_job_conflict() -> None:
|
||||
"""Do not let BaseService's tolerant registration hide reserved-route conflicts."""
|
||||
service = HttpService(web_enabled=False)
|
||||
service.build_service(_FakeApplication()) # type: ignore[arg-type]
|
||||
app = SimpleNamespace(
|
||||
context=SimpleNamespace(jobs={"mcp": BaseJob(name="mcp")}),
|
||||
)
|
||||
|
||||
with pytest.raises(ValueError, match="conflicts with the MCP endpoint"):
|
||||
service.add_jobs(app)
|
||||
|
||||
|
||||
def test_http_service_does_not_serve_symlinks_outside_static_dir(
|
||||
tmp_path: Path,
|
||||
) -> None:
|
||||
|
|
|
|||
Loading…
Add table
Reference in a new issue