Repository navigation
fix(mcp): restrict unauthenticated HTTP listeners to loopback - #744
Conversation
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
rng1995
left a comment
There was a problem hiding this comment.
[SkillSpector Review]
Hi @yashrajp22, thank you for making the unauthenticated HTTP transport refuse to listen beyond loopback!
Value and readiness: run() now validates the bind address before it builds the FastMCP server.
- Rejected:
0.0.0.0,::, routable IPs, hostnames, and short or integer IPv4 forms (127.1,2130706433). - Accepted:
127.0.0.1,::1, andlocalhost, which maps to127.0.0.1without a DNS lookup.
The CLI turns a rejection into exit code 2 with a clear message. The default (127.0.0.1) and stdio are unchanged. This closes the exposure and is ready for final maintainer review; the notes below are optional.
Material findings
-
[Non-blocking]
src/skillspector/mcp_server.py:289:is_loopbackaccepts the whole127.0.0.0/8range, andtest_http_accepts_only_loopback_bindingslocks in127.3.2.1.- Why that bind does not work:
build_server()callsFastMCP(name)with its default host. In mcp 1.29, FastMCP then turns on Host-header checking that allows only127.0.0.1:*,localhost:*and[::1]:*. A server bound to127.3.2.1starts (on Linux) and then answers every request with 421 "Invalid Host header". - Suggestion: The README and the error message already say "127.0.0.1 or ::1". Accepting exactly those two, plus the
localhostalias, would make the code, the docs and FastMCP's allowlist agree.
- Why that bind does not work:
-
[Non-blocking] Some wording still describes remote callers:
- the
mcpcommand docstring (src/skillspector/cli.py:3317, "or remote runtime can scan a skill") - the module docstring (
src/skillspector/mcp_server.py:18) README.md:405
Consider "local agents" there, or a pointer to the reverse-proxy pattern. The next release notes should also mention that
skillspector mcp --transport http --host 0.0.0.0now exits with code 2. - the
PIC tradeoffs: There is no authenticated HTTP mode, and this PR adds no opt-in override. So off-loopback HTTP is now impossible in every configuration, including a container whose authenticating proxy runs in a separate network namespace.
In practice, little working use is lost. On current main, FastMCP already rejected (421) any request whose Host header was not a loopback name. Remote access therefore already depended on a same-host proxy forwarding to 127.0.0.1, which still works and which the README now documents. An explicit opt-in for container deployments could come later, ideally together with real authentication.
Verification and gaps:
- Code path: I traced
skillspector mcp→run(). The check runs beforebuild_server().FASTMCP_*environment variables cannot override the bind, becauserun()setssettings.hostexplicitly after validation. - No other HTTP entry point:
streamable_http_appandsse_appare not used, and the Docker image does not install themcpextra. - IPv6:
::1is accepted.::(unspecified) is rejected. Bracketed[::1]is rejected byip_address, and uvicorn expects the bare form anyway. - FastMCP behaviour: I read the FastMCP auto-enable block and
transport_security._validate_hostin the upstreamv1.29.0tag, which matches themcp>=1.29.0pin. - Tests: they cover rejection of 9 hosts before the server is built, the accepted bindings (including
::1,localhostandLOCALHOST), and CLI exit code 2. I did not run them, per policy. - CI: all 6 checks passed on
4a39cc8. The bot merges ofmain(latest206c644) leave this PR's diff unchanged (identical patch-id). Their CI runs areaction_requiredand need maintainer approval before merge. - Conflicts:
git merge-treeagainstmainis clean, and there are no conflicts with #736, #737, #738 or #572. #736 changes other hunks ofmcp_server.py,tests/unit/test_mcp_server.pyand (in its follow-up) the same README note, so landing this one first is simplest.
Decision: Approved (reviewed head 206c644c7c0224fb245da65188750f87b806122b)
Signed-off-by: yashrajbasav <yashrajbasav@nvidia.com>
|
Thanks for the helpful notes! The HTTP listener now accepts only 127.0.0.1, ::1 and the localhost alias, matching the Host headers FastMCP accepts. I also updated the wording to describe local agents. All 78 MCP tests pass, including rejection of other 127/8 addresses and normalization of the full IPv6 loopback spelling. |
Unauthenticated HTTP MCP listeners could be exposed with a wildcard or routable bind address. Reject those bindings before creating the server, retain loopback HTTP and stdio, and map localhost to 127.0.0.1 without DNS resolution. Remote access requires an authenticating reverse proxy in front of the loopback listener.
Validation: 329 MCP/CLI tests; fresh installed-wheel checks reproduce the baseline wildcard listener and confirm candidate exit 2 with no listener. Real HTTP initialize/local-target rejection and stdio sample scans still work.