diff --git a/redisvl/cli/mcp.py b/redisvl/cli/mcp.py index 31f98809..9a5caaa6 100644 --- a/redisvl/cli/mcp.py +++ b/redisvl/cli/mcp.py @@ -5,6 +5,15 @@ import inspect import sys +# The HTTP transports the CLI offers. One tuple drives both the `--transport` +# choices and every HTTP-only branch below, so offering another HTTP transport +# here cannot silently skip the unauthenticated-bind check or the bind +# arguments. Deliberately local: the CLI imports nothing from `redisvl.mcp` at +# module level, so `rvl` stays usable without the optional `mcp` extra. The +# server's own set, which also covers FastMCP's `"http"`, is +# `redisvl.mcp.transport_security.HTTP_TRANSPORTS`. +_HTTP_TRANSPORT_CHOICES = ("sse", "streamable-http") + class _MCPArgumentParser(argparse.ArgumentParser): """ArgumentParser variant that reports usage errors with exit code 2.""" @@ -51,7 +60,7 @@ def __init__(self): parser.add_argument( "--transport", help="Transport protocol (default: stdio)", - choices=["stdio", "sse", "streamable-http"], + choices=["stdio", *_HTTP_TRANSPORT_CHOICES], default="stdio", ) parser.add_argument( @@ -129,7 +138,7 @@ def _check_http_auth(cls, transport, host, auth_enabled, allow_unauthenticated): No-op for stdio or when auth is enabled. Warns for loopback binds and fails closed for non-loopback binds unless explicitly allowed. """ - if transport not in ("sse", "streamable-http") or auth_enabled: + if transport not in _HTTP_TRANSPORT_CHOICES or auth_enabled: return None if host not in cls._LOOPBACK_HOSTS and not allow_unauthenticated: @@ -164,7 +173,7 @@ async def _serve( print(warning, file=sys.stderr) transport_kwargs = {} - if transport in ("sse", "streamable-http"): + if transport in _HTTP_TRANSPORT_CHOICES: transport_kwargs["host"] = host transport_kwargs["port"] = port diff --git a/redisvl/mcp/server.py b/redisvl/mcp/server.py index 5521e2f0..69c83abc 100644 --- a/redisvl/mcp/server.py +++ b/redisvl/mcp/server.py @@ -23,6 +23,7 @@ from redisvl.mcp.tools.search import register_search_tool from redisvl.mcp.tools.upsert import register_upsert_tool from redisvl.mcp.transport_security import ( + HTTP_TRANSPORTS, build_host_origin_middleware, resolve_transport_security_config, ) @@ -145,18 +146,34 @@ async def run_async( outermost, rejecting DNS-rebinding requests before auth or tool handlers. ``stdio`` is untouched. """ - # Resolved the way FastMCP resolves it, so `run_async()` with no - # argument is recorded as the stdio it will actually serve. Imported - # here because this module stays importable without the `mcp` extra. + # Resolved the way FastMCP resolves it, because both the guard and the + # tenant-injection check have to know what will actually be served. An + # omitted transport falls back to `fastmcp.settings.transport` + # (FASTMCP_TRANSPORT), which can name an HTTP transport; FastMCP before + # 3.1 has no such setting and always served stdio, so that is the + # fallback. Imported here because this module stays importable without + # the `mcp` extra. import fastmcp - self._transport = ( - transport if transport is not None else fastmcp.settings.transport + settings = getattr(fastmcp, "settings", None) + resolved = ( + transport + if transport is not None + else getattr(settings, "transport", "stdio") ) - - if transport in ("sse", "streamable-http"): - host = transport_kwargs.get("host", "127.0.0.1") - port = transport_kwargs.get("port", 8000) + self._transport = resolved + + if resolved in HTTP_TRANSPORTS: + # The allowlist must match the address FastMCP binds, which falls + # back to settings (FASTMCP_HOST / FASTMCP_PORT) the same way. + # Hardcoded defaults would reject legitimate Host headers on a + # settings-driven bind. + host: Any = transport_kwargs.get("host") + if host is None: + host = getattr(settings, "host", "127.0.0.1") + port: Any = transport_kwargs.get("port") + if port is None: + port = getattr(settings, "port", 8000) guard = build_host_origin_middleware(self._transport_security, host, port) if guard: existing = transport_kwargs.get("middleware") or [] diff --git a/redisvl/mcp/transport_security.py b/redisvl/mcp/transport_security.py index 9018e4bf..1f875548 100644 --- a/redisvl/mcp/transport_security.py +++ b/redisvl/mcp/transport_security.py @@ -19,6 +19,12 @@ from redisvl.mcp.config import MCPTransportSecurityConfig, _substitute_env from redisvl.mcp.settings import MCPSettings +# Every transport name FastMCP serves over HTTP. `"http"` is FastMCP's own +# default HTTP name, distinct from `"streamable-http"` though served the same +# way, and testing for only the two names the CLI offers left the guard off +# for it. Defined once so the server and the CLI cannot drift apart again. +HTTP_TRANSPORTS = frozenset({"http", "sse", "streamable-http"}) + # Hosts that only ever refer to the local machine. Kept in sync with # ``redisvl.cli.mcp.MCP._LOOPBACK_HOSTS``. _LOOPBACK_HOSTS = frozenset({"127.0.0.1", "::1", "localhost"}) diff --git a/tests/unit/test_mcp/test_transport_security.py b/tests/unit/test_mcp/test_transport_security.py index 209371bc..bf8ceb05 100644 --- a/tests/unit/test_mcp/test_transport_security.py +++ b/tests/unit/test_mcp/test_transport_security.py @@ -237,3 +237,106 @@ def test_build_middleware_merges_configured_hosts(): kwargs = built[0].kwargs assert "proxy.internal:8000" in kwargs["allowed_hosts"] assert "127.0.0.1:8000" in kwargs["allowed_hosts"] + + +# --- which transports get the guard ------------------------------------------- + + +@pytest.mark.parametrize( + "transport, configured_default, guarded", + [ + ("streamable-http", "stdio", True), + ("sse", "stdio", True), + # FastMCP's own default HTTP name, served exactly like streamable-http. + ("http", "stdio", True), + # An omitted transport falls back to fastmcp.settings.transport, which + # can name an HTTP transport just as an explicit argument can. + (None, "streamable-http", True), + (None, "http", True), + ("stdio", "stdio", False), + (None, "stdio", False), + ], +) +def test_every_http_transport_is_served_behind_the_guard( + monkeypatch, transport, configured_default, guarded +): + fastmcp = pytest.importorskip( + "fastmcp", reason="fastmcp not installed (install redisvl[mcp])" + ) + from redisvl.mcp.server import RedisVLMCPServer + + served: dict = {} + + async def record_serve(self, transport=None, show_banner=None, **kwargs): + served["middleware"] = kwargs.get("middleware") + + monkeypatch.setattr(fastmcp.FastMCP, "run_async", record_serve) + monkeypatch.setattr(fastmcp.settings, "transport", configured_default) + guard = object() + monkeypatch.setattr( + "redisvl.mcp.server.build_host_origin_middleware", lambda *args: [guard] + ) + server = RedisVLMCPServer.__new__(RedisVLMCPServer) + server._transport_security = MCPTransportSecurityConfig() + + asyncio.run(server.run_async(transport=transport, host="0.0.0.0", port=8000)) + + assert (served["middleware"] == [guard]) is guarded + + +def _run_with_settings(monkeypatch, settings, **run_kwargs): + """Run `run_async` against substitute FastMCP settings, recording the guard.""" + fastmcp = pytest.importorskip( + "fastmcp", reason="fastmcp not installed (install redisvl[mcp])" + ) + from redisvl.mcp.server import RedisVLMCPServer + + built: list = [] + served: dict = {} + + async def record_serve(self, transport=None, show_banner=None, **kwargs): + served["middleware"] = kwargs.get("middleware") + + monkeypatch.setattr(fastmcp.FastMCP, "run_async", record_serve) + monkeypatch.setattr(fastmcp, "settings", settings) + monkeypatch.setattr( + "redisvl.mcp.server.build_host_origin_middleware", + lambda config, host, port: built.append((host, port)) or ["guard"], + ) + server = RedisVLMCPServer.__new__(RedisVLMCPServer) + server._transport_security = MCPTransportSecurityConfig() + asyncio.run(server.run_async(**run_kwargs)) + return built, served.get("middleware") + + +def test_an_omitted_transport_serves_stdio_on_fastmcp_without_the_setting( + monkeypatch, +): + # `settings.transport` arrived in FastMCP 3.1; before it an omitted + # transport was always stdio. The supported range starts at 2.0. + from types import SimpleNamespace + + built, middleware = _run_with_settings( + monkeypatch, SimpleNamespace(host="127.0.0.1", port=8000), transport=None + ) + assert built == [] + assert middleware is None + + +def test_the_guard_allowlists_the_address_fastmcp_actually_binds(monkeypatch): + # FastMCP binds FASTMCP_HOST / FASTMCP_PORT when no host or port is passed, + # so a hardcoded 127.0.0.1:8000 allowlist would reject legitimate Host + # headers on that bind. + from types import SimpleNamespace + + settings = SimpleNamespace(transport="streamable-http", host="0.0.0.0", port=9123) + + built, middleware = _run_with_settings(monkeypatch, settings, transport=None) + assert built == [("0.0.0.0", 9123)] + assert middleware == ["guard"] + + # An explicit host and port still take precedence over settings. + built, _ = _run_with_settings( + monkeypatch, settings, transport="http", host="10.0.0.5", port=7000 + ) + assert built == [("10.0.0.5", 7000)]