Repository navigation
fix(mcp): widen transport_security to match --host - #1737
RaghavChamadiya merged 2 commits into
Conversation
FastMCP builds its module-level instance with no host= argument, at import time, before any CLI --host is known, so it auto-bakes a DNS-rebinding Host allowlist scoped to loopback only. run_mcp() later rebinds the socket via mcp.settings.host, but never updated the allowlist to match, so every request to a non-loopback --host failed Host validation with 421 Misdirected Request no matter what --host was given. _configure_transport_security() rebuilds the allowlist around the actual bind host (or disables the now-unsatisfiable Host check for a wildcard bind, where nothing else currently gates access).
|
✅ Health of changed files: 1.0 → 2.6 (+1.6) 📋 At a glance ✅ Health gate: passed 📌 Before you merge
🔎 More signals (2)🗺️ Change map flowchart LR
subgraph PR ["Changed in this PR (1 with dependents)"]
f_packages_server_src_repowise_server_mcp_server__server_py[".../mcp_server/_server.py 🔥"]:::changed
end
f_packages_cli_src_repowise_cli_helpers_py[".../cli/helpers.py"]
f_packages_server_src_repowise_server_mcp_server__server_py --> f_packages_cli_src_repowise_cli_helpers_py
f_packages_core_src_repowise_core_workspace_update_py[".../workspace/update.py"]
f_packages_server_src_repowise_server_mcp_server__server_py --> f_packages_core_src_repowise_core_workspace_update_py
f_packages_server_src_repowise_server_app_py[".../server/app.py"]
f_packages_server_src_repowise_server_mcp_server__server_py --> f_packages_server_src_repowise_server_app_py
f_packages_server_src_repowise_server_mcp_server___init___py[".../mcp_server/__init__.py"]
f_packages_server_src_repowise_server_mcp_server__server_py --> f_packages_server_src_repowise_server_mcp_server___init___py
t_tests_unit_cli_test_embedder_key_resolution_py(["✅ .../cli/test_embedder_key_resolution.py"]):::guard
t_tests_unit_cli_test_embedder_key_resolution_py -.-> f_packages_server_src_repowise_server_mcp_server__server_py
classDef changed fill:#dbeafe,stroke:#1d4ed8,color:#1e3a5f
classDef warn fill:#fef3c7,stroke:#b45309,color:#78350f
classDef guard fill:#dcfce7,stroke:#15803d,color:#14532d
Solid arrows: code that imports the changed files (4 direct dependents, from the last indexed snapshot). Dashed: history/tests. 🔥 Hotspot touched (1)
👀 Suggested reviewers @RaghavChamadiya 📊 See the full report for this PR |
|
The fix is right for the common LAN/hostname case, and the tests are genuinely good — exercising the real One real gap worth handling or explicitly scoping out: a concrete IPv6 Also worth a sentence in the PR body: disabling the Host check for Minor: |
TransportSecurityMiddleware matches by host.startswith(base_host + ":"), and a client connecting to an IPv6 literal sends a bracketed Host header ([2001:db8::1]:7338). The allowlist entry has to be shaped the same way - a bare "2001:db8::1:*" pattern never matches that header, so a concrete IPv6 --host still 421'd despite the fix. _bracket_if_ipv6() brackets the host before it goes into allowed_hosts/allowed_origins. Also documents two review-flagged, out-of-scope points inline rather than leaving them implicit: disabling the Host check for a wildcard bind rides on the existing unauthenticated-bind warning until #1400 lands auth, and allowed_origins inherits the same startswith prefix looseness as allowed_hosts from the SDK itself (pre-existing, not introduced here).
|
Thanks for the review
|
|
Update to the earlier note on the https://github.com/modelcontextprotocol/python-sdk/security/advisories/GHSA-4w35-279q-9x9p Short version for the record: |
|
Thanks for flagging this and offering to track it — that saved me the legwork. I went ahead and filed it upstream as a private advisory (their SECURITY.md prefers that over a public issue), so it's on the SDK maintainers' radar now: https://github.com/modelcontextprotocol/python-sdk/security/advisories/GHSA-4w35-279q-9x9p — status Triage, Moderate. It's inherited/FastMCP-default behavior as you noted, so nothing further is needed on this PR; when a fix lands upstream we'll just bump the dep. Appreciate the thorough response to the review. |
RaghavChamadiya
left a comment
RaghavChamadiya
left a comment
There was a problem hiding this comment.
Thanks @nathanpride. The diagnosis is the valuable part: FastMCP builds mcp.settings.transport_security at import time, before any CLI --host exists, so it bakes in a loopback-only allowlist. run_mcp then rebinds the socket through mcp.settings.host and nothing ever revisits the allowlist. That is why --host produced a 421 for every value, which reads like a networking problem and is not one.
_bracket_if_ipv6 is the detail that shows you read the SDK rather than guessed. TransportSecurityMiddleware matches on host.startswith(base + ":"), so a bare 2001:db8::1 can never match the bracketed [2001:db8::1]:7338 a client actually sends, and the "any colon means IPv6, because hostnames and IPv4 never contain one" reasoning is right and worth having written down.
On the wildcard case, I want to say plainly that I read it as a deliberate trade and I agree with it. Disabling DNS-rebinding protection sounds like a downgrade, but no single Host value can match a 0.0.0.0 bind, so the previous behaviour was not protection, it was a permanent 421. Nothing was reachable to protect. Turning it off makes the bind do what the operator asked and leaves the startup security warning as the gate until #1400 lands. Your docstring says exactly that, which is why it does not need a reviewer to reconstruct it.
Keeping loopback in the allowlist for a non-loopback bind is the right default too. An SSH tunnel or a client on the same box arrives as 127.0.0.1 and would otherwise break the moment someone set --host.
Full matrix green. Merging. This closes #1736.
FastMCP builds its module-level instance with no host= argument, at import time, before any CLI --host is known, so it auto-bakes a DNS-rebinding Host allowlist scoped to loopback only. run_mcp() later rebinds the socket via mcp.settings.host, but never updated the allowlist to match, so every request to a non-loopback --host failed Host validation with 421 Misdirected Request no matter what --host was given.
_configure_transport_security() rebuilds the allowlist around the actual bind host (or disables the now-unsatisfiable Host check for a wildcard bind, where nothing else currently gates access).
Summary
Hostallowlist to match--host, instead of leaving it frozen on loopback (fixes421 Misdirected Requeston any non-default--host)Related Issues
Fixes #1736
Related to #1400 (same function —
run_mcpin_server.py— but a distinct concern: #1400 is about missing authentication on a wide-open bind, which this PR does not address)Test Plan
pytest) —uv run pytest tests/unit/server/test_mcp_transport_security.py tests/unit/cli/test_mcp_transport.py, 28 passedruff check .)npm run build) (if frontend changes) — n/a, no frontend changesrepowise mcp --transport streamable-http --host <non-loopback IP>, then a request with a matchingHostheader —200 OK/ cleaninitializeresponse, no421/Invalid Host header(verified against both a wildcard0.0.0.0bind and a concrete interface IP)Checklist