fix(security): validate MCP URLs and fail-closed sampling approval#722
fix(security): validate MCP URLs and fail-closed sampling approval#722Solaris-star wants to merge 1 commit into
Conversation
Block private/loopback/metadata hosts for network MCP transports to mitigate SSRF. Require a human input handler for local sampling so MCP servers cannot drive the LLM without approval. Fixes lastmile-ai#721
📝 WalkthroughWalkthroughThe changes add SSRF-focused MCP URL validation for network transports and apply it in both server startup paths. Sampling approval now fails closed when no human input handler is configured, with tests covering URL safety behavior. ChangesMCP security controls
Estimated code review effort: 3 (Moderate) | ~20 minutes 🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✨ Finishing Touches🧪 Generate unit tests (beta)
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 3
🧹 Nitpick comments (2)
tests/mcp/test_url_safety.py (2)
8-24: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winMissing coverage for metadata hostnames and ws(s) schemes.
Existing tests cover the metadata IP literal (
169.254.169.254) and a bad scheme (file://), but not the metadata hostnames (metadata.google.internal,metadata) already in_BLOCKED_HOSTNAMES, nor thatws:///wss://are accepted schemes. Given this module is a security control, adding these cases would tighten regression coverage.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/mcp/test_url_safety.py` around lines 8 - 24, Extend test_rejects_loopback_hostname or add a focused test to assert validate_mcp_url rejects both metadata.google.internal and metadata with UnsafeMCPUrlError. Add coverage confirming validate_mcp_url accepts valid ws:// and wss:// URLs, while preserving the existing rejection cases.
27-35: 📐 Maintainability & Code Quality | 🔵 Trivial | ⚡ Quick winTest depends on live DNS/network for
example.com.This introduces flakiness in sandboxed/offline CI runners; a partial network failure that doesn't raise
socket.gaierror(e.g., a hang or a different exception) won't be skipped gracefully. Consider mockingsocket.getaddrinfoto return a deterministic public IP instead of relying on real DNS resolution.🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the rest with a brief reason, keep changes minimal, and validate. In `@tests/mcp/test_url_safety.py` around lines 27 - 35, The test_allows_public_https test relies on live DNS and can fail or hang in offline CI. Mock socket.getaddrinfo within this test to return a deterministic publicly routable address, then validate the same example.com URL without conditional DNS exception handling or network-dependent skips.
🤖 Prompt for all review comments with AI agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
Inline comments:
In `@src/mcp_agent/mcp/url_safety.py`:
- Around line 88-106: Make DNS validation async-safe: in
src/mcp_agent/mcp/url_safety.py lines 88-106, add an awaitable wrapper around
validate_mcp_url that offloads socket.getaddrinfo via anyio.to_thread.run_sync;
in src/mcp_agent/mcp/mcp_server_registry.py lines 151-157 and
src/mcp_agent/mcp/mcp_connection_manager.py lines 444-449, await this wrapper
instead of calling validate_mcp_url synchronously.
- Around line 37-53: Update validate_mcp_url and the MCP transport setup so the
address resolved during validation is reused for the connection instead of
resolving the original hostname again. Preserve the original hostname for HTTP
Host and TLS SNI while routing the socket to the validated IP, or pass an
equivalent pinned-IP/custom-resolver configuration through the transport
clients; ensure allow_private behavior remains unchanged.
- Around line 68-89: Update the hostname and literal-IP validation in the URL
safety flow to block cloud-metadata hostnames and metadata service IPs
unconditionally, while keeping loopback/private-network checks controlled by
allow_private. Use distinct metadata-specific checks rather than gating the
combined _BLOCKED_HOSTNAMES or _is_non_public_ip handling on the same condition,
and preserve allowed local-development URLs when allow_private is true.
---
Nitpick comments:
In `@tests/mcp/test_url_safety.py`:
- Around line 8-24: Extend test_rejects_loopback_hostname or add a focused test
to assert validate_mcp_url rejects both metadata.google.internal and metadata
with UnsafeMCPUrlError. Add coverage confirming validate_mcp_url accepts valid
ws:// and wss:// URLs, while preserving the existing rejection cases.
- Around line 27-35: The test_allows_public_https test relies on live DNS and
can fail or hang in offline CI. Mock socket.getaddrinfo within this test to
return a deterministic publicly routable address, then validate the same
example.com URL without conditional DNS exception handling or network-dependent
skips.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro
Run ID: b2f2a459-5601-4126-8928-3d4d4b95f473
📒 Files selected for processing (5)
src/mcp_agent/mcp/mcp_connection_manager.pysrc/mcp_agent/mcp/mcp_server_registry.pysrc/mcp_agent/mcp/sampling_handler.pysrc/mcp_agent/mcp/url_safety.pytests/mcp/test_url_safety.py
| def validate_mcp_url(url: str, *, allow_private: bool = False) -> str: | ||
| """Validate that *url* is safe to open as an MCP network transport. | ||
|
|
||
| Blocks loopback, private, link-local, and cloud-metadata ranges by default | ||
| so untrusted MCP configs cannot SSRF into the local network (#721). | ||
|
|
||
| Args: | ||
| url: Absolute http(s)/ws(s) URL from server configuration. | ||
| allow_private: If True, skip private-range checks (local/dev only). | ||
|
|
||
| Returns: | ||
| The original URL when validation passes. | ||
|
|
||
| Raises: | ||
| UnsafeMCPUrlError: If the scheme/host is disallowed or resolves to a | ||
| non-public address. | ||
| """ |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | 🏗️ Heavy lift
🧩 Analysis chain
🏁 Script executed:
#!/bin/bash
set -euo pipefail
# Map the relevant file and its immediate neighbors
git ls-files src/mcp_agent/mcp
printf '\n--- url_safety.py outline ---\n'
ast-grep outline src/mcp_agent/mcp/url_safety.py --view expanded || true
printf '\n--- locate transport call sites ---\n'
rg -n "streamablehttp_client|sse_client|websocket_client|validate_mcp_url|UnsafeMCPUrlError" src/mcp_agent/mcp -S
printf '\n--- inspect url_safety.py ---\n'
wc -l src/mcp_agent/mcp/url_safety.py
cat -n src/mcp_agent/mcp/url_safety.py | sed -n '1,220p'Repository: lastmile-ai/mcp-agent
Length of output: 7502
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- mcp_connection_manager.py around validation and transport creation ---\n'
cat -n src/mcp_agent/mcp/mcp_connection_manager.py | sed -n '420,560p'
printf '\n--- mcp_server_registry.py around validation and transport creation ---\n'
cat -n src/mcp_agent/mcp/mcp_server_registry.py | sed -n '130,360p'
printf '\n--- search for any IP pinning / resolved-address transport logic ---\n'
rg -n "getaddrinfo|resolved address|pin.*ip|Host:|SNI|session_id|transport|socket\.create_connection|connect\(" src/mcp_agent/mcp -SRepository: lastmile-ai/mcp-agent
Length of output: 25996
🌐 Web query:
mcp.client.streamable_http websocket_client sse_client DNS resolution connect-time source code
💡 Result:
In the Model Context Protocol (MCP) Python SDK, connection management for transports like StreamableHTTP and WebSockets is handled primarily through httpx.AsyncClient configurations [1][2] and underlying transport-specific connection managers [3]. 1. StreamableHTTP and WebSocket Clients: The streamable_http_client function creates an httpx.AsyncClient to manage HTTP/SSE-based communication [1][2]. If no pre-configured httpx.AsyncClient is provided, a default client is created using internal SDK helpers that apply recommended MCP timeouts [1][2]. Similarly, websocket_client manages WebSocket connections, typically wrapping the transport to expose read/write streams for the ClientSession [4]. 2. DNS Resolution and Connect-Time: Because the Python SDK utilizes httpx for HTTP-based transports (StreamableHTTP and SSE), DNS resolution and connection-time behavior are inherited from the httpx library's underlying transport layer [1][2]. The SDK does not implement custom DNS resolution logic; instead, it relies on the standard httpx.AsyncClient defaults, which utilize system DNS settings [1]. Connection timeouts are governed by the httpx.Timeout configurations provided to the client factory [5]. 3. Source Code Context: The relevant source code for streamable_http_client is located in src/mcp/client/streamable_http.py [1][2]. This implementation manages the lifecycle of the HTTP client (using AsyncExitStack to ensure proper closure) and initializes the TransportStreams needed for communication [1]. Older versions of the SDK used a deprecated streamablehttp_client function; this has been replaced by streamable_http_client to streamline the interface [5]. For more precise control over DNS, connection timeouts, or proxy settings, you can instantiate your own httpx.AsyncClient with custom httpx.Limits or httpx.Transport settings and pass it to the streamable_http_client function [1][2].
Citations:
- 1: https://github.com/modelcontextprotocol/python-sdk/blob/main/src/mcp/client/streamable_http.py
- 2: https://github.com/modelcontextprotocol/python-sdk/blob/e8e64842/src/mcp/client/streamable_http.py
- 3: https://github.com/mcp-use/mcp-use/blob/main/libraries/python/mcp_use/client/connectors/http.py
- 4: https://github.com/paulhshort/fastmcp/blob/748d6bd0/src/fastmcp/client/transports.py
- 5: modelcontextprotocol/python-sdk@8893b02
🏁 Script executed:
#!/bin/bash
set -euo pipefail
printf '\n--- files under src/mcp/client ---\n'
git ls-files src/mcp/client || true
printf '\n--- locate streamable_http client implementation ---\n'
rg -n "def streamable_http_client|def streamablehttp_client|def sse_client|def websocket_client|AsyncClient|httpx" src/mcp/client src/mcp_agent -S
printf '\n--- inspect candidate client files ---\n'
for f in \
src/mcp/client/streamable_http.py \
src/mcp/client/sse.py \
src/mcp/client/websocket.py
do
if [ -f "$f" ]; then
echo ""
echo "### $f ###"
wc -l "$f"
cat -n "$f" | sed -n '1,260p'
fi
doneRepository: lastmile-ai/mcp-agent
Length of output: 11330
Pin the resolved IP into the transport. validate_mcp_url() checks the hostname once, but the transport clients still receive the original URL string, so the host is resolved again when the connection opens. That leaves a DNS-rebinding window where a public address can be swapped for a private or metadata IP after validation. Pass a pinned IP/custom resolver into the transport, or connect to the resolved IP while preserving Host/SNI.
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/mcp_agent/mcp/url_safety.py` around lines 37 - 53, Update
validate_mcp_url and the MCP transport setup so the address resolved during
validation is reused for the connection instead of resolving the original
hostname again. Preserve the original hostname for HTTP Host and TLS SNI while
routing the socket to the validated IP, or pass an equivalent
pinned-IP/custom-resolver configuration through the transport clients; ensure
allow_private behavior remains unchanged.
| host_l = host.lower().rstrip(".") | ||
| if host_l in _BLOCKED_HOSTNAMES or host_l.endswith(".localhost"): | ||
| if not allow_private: | ||
| raise UnsafeMCPUrlError( | ||
| f"MCP server URL host {host!r} is not allowed (loopback/metadata)" | ||
| ) | ||
|
|
||
| # Literal IP in the URL | ||
| try: | ||
| ip = ipaddress.ip_address(host) | ||
| except ValueError: | ||
| ip = None | ||
|
|
||
| if ip is not None: | ||
| if not allow_private and _is_non_public_ip(ip): | ||
| raise UnsafeMCPUrlError( | ||
| f"MCP server URL must not target non-public IP {ip}" | ||
| ) | ||
| return url | ||
|
|
||
| # Resolve hostname and check every address | ||
| if not allow_private: |
There was a problem hiding this comment.
🔒 Security & Privacy | 🟠 Major | ⚡ Quick win
allow_private=True also disables the cloud-metadata blocklist, not just the private-network checks.
_BLOCKED_HOSTNAMES mixes loopback names (localhost, localhost.localdomain) with cloud-metadata names (metadata.google.internal, metadata), and both the hostname check (Line 70) and IP-literal check (Line 82) are gated by the same if not allow_private condition. A user who sets allow_private_urls: true for a legitimate reason (e.g. pointing at a local dev server) unintentionally also re-opens SSRF access to 169.254.169.254 and metadata hostnames — a distinct, higher-severity threat category that the docstring's "local/dev only" framing doesn't cover.
Consider always blocking the metadata-specific hostnames/IPs regardless of allow_private, separate from the loopback/private-range exemption:
🛡️ Proposed fix: keep metadata blocklist unconditional
-_BLOCKED_HOSTNAMES = frozenset(
+_LOOPBACK_HOSTNAMES = frozenset(
{
"localhost",
"localhost.localdomain",
- "metadata.google.internal",
- "metadata",
}
)
+_METADATA_HOSTNAMES = frozenset({"metadata.google.internal", "metadata"})
+_METADATA_IPS = frozenset({ipaddress.ip_address("169.254.169.254")})
@@
host_l = host.lower().rstrip(".")
- if host_l in _BLOCKED_HOSTNAMES or host_l.endswith(".localhost"):
+ if host_l in _METADATA_HOSTNAMES:
+ raise UnsafeMCPUrlError(
+ f"MCP server URL host {host!r} is not allowed (cloud metadata)"
+ )
+ if host_l in _LOOPBACK_HOSTNAMES or host_l.endswith(".localhost"):
if not allow_private:
raise UnsafeMCPUrlError(
f"MCP server URL host {host!r} is not allowed (loopback/metadata)"
)
@@
if ip is not None:
+ if ip in _METADATA_IPS:
+ raise UnsafeMCPUrlError(f"MCP server URL must not target cloud metadata IP {ip}")
if not allow_private and _is_non_public_ip(ip):📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
| host_l = host.lower().rstrip(".") | |
| if host_l in _BLOCKED_HOSTNAMES or host_l.endswith(".localhost"): | |
| if not allow_private: | |
| raise UnsafeMCPUrlError( | |
| f"MCP server URL host {host!r} is not allowed (loopback/metadata)" | |
| ) | |
| # Literal IP in the URL | |
| try: | |
| ip = ipaddress.ip_address(host) | |
| except ValueError: | |
| ip = None | |
| if ip is not None: | |
| if not allow_private and _is_non_public_ip(ip): | |
| raise UnsafeMCPUrlError( | |
| f"MCP server URL must not target non-public IP {ip}" | |
| ) | |
| return url | |
| # Resolve hostname and check every address | |
| if not allow_private: | |
| host_l = host.lower().rstrip(".") | |
| if host_l in _METADATA_HOSTNAMES: | |
| raise UnsafeMCPUrlError( | |
| f"MCP server URL host {host!r} is not allowed (cloud metadata)" | |
| ) | |
| if host_l in _LOOPBACK_HOSTNAMES or host_l.endswith(".localhost"): | |
| if not allow_private: | |
| raise UnsafeMCPUrlError( | |
| f"MCP server URL host {host!r} is not allowed (loopback/metadata)" | |
| ) | |
| # Literal IP in the URL | |
| try: | |
| ip = ipaddress.ip_address(host) | |
| except ValueError: | |
| ip = None | |
| if ip is not None: | |
| if ip in _METADATA_IPS: | |
| raise UnsafeMCPUrlError( | |
| f"MCP server URL must not target cloud metadata IP {ip}" | |
| ) | |
| if not allow_private and _is_non_public_ip(ip): | |
| raise UnsafeMCPUrlError( | |
| f"MCP server URL must not target non-public IP {ip}" | |
| ) | |
| return url | |
| # Resolve hostname and check every address | |
| if not allow_private: |
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/mcp_agent/mcp/url_safety.py` around lines 68 - 89, Update the hostname
and literal-IP validation in the URL safety flow to block cloud-metadata
hostnames and metadata service IPs unconditionally, while keeping
loopback/private-network checks controlled by allow_private. Use distinct
metadata-specific checks rather than gating the combined _BLOCKED_HOSTNAMES or
_is_non_public_ip handling on the same condition, and preserve allowed
local-development URLs when allow_private is true.
| # Resolve hostname and check every address | ||
| if not allow_private: | ||
| try: | ||
| infos = socket.getaddrinfo(host, parsed.port or 0, type=socket.SOCK_STREAM) | ||
| except socket.gaierror as exc: | ||
| raise UnsafeMCPUrlError( | ||
| f"MCP server URL host {host!r} could not be resolved: {exc}" | ||
| ) from exc | ||
| for info in infos: | ||
| addr = info[4][0] | ||
| try: | ||
| resolved = ipaddress.ip_address(addr) | ||
| except ValueError: | ||
| continue | ||
| if _is_non_public_ip(resolved): | ||
| raise UnsafeMCPUrlError( | ||
| f"MCP server URL host {host!r} resolves to non-public " | ||
| f"address {resolved}" | ||
| ) |
There was a problem hiding this comment.
🩺 Stability & Availability | 🟠 Major | ⚡ Quick win
Blocking DNS resolution called synchronously from async request paths. validate_mcp_url's socket.getaddrinfo call is synchronous, and both callers invoke it directly inside async def functions without offloading — a slow/unresponsive resolver stalls the shared event loop.
src/mcp_agent/mcp/url_safety.py#L88-L106: add an async-safe wrapper (e.g., run thegetaddrinfocall viaanyio.to_thread.run_sync) that callers can await.src/mcp_agent/mcp/mcp_server_registry.py#L151-L157: await the new async wrapper instead of callingvalidate_mcp_urlsynchronously.src/mcp_agent/mcp/mcp_connection_manager.py#L444-L449: await the new async wrapper instead of callingvalidate_mcp_urlsynchronously.
📍 Affects 3 files
src/mcp_agent/mcp/url_safety.py#L88-L106(this comment)src/mcp_agent/mcp/mcp_server_registry.py#L151-L157src/mcp_agent/mcp/mcp_connection_manager.py#L444-L449
🤖 Prompt for AI Agents
Verify each finding against current code. Fix only still-valid issues, skip the
rest with a brief reason, keep changes minimal, and validate.
In `@src/mcp_agent/mcp/url_safety.py` around lines 88 - 106, Make DNS validation
async-safe: in src/mcp_agent/mcp/url_safety.py lines 88-106, add an awaitable
wrapper around validate_mcp_url that offloads socket.getaddrinfo via
anyio.to_thread.run_sync; in src/mcp_agent/mcp/mcp_server_registry.py lines
151-157 and src/mcp_agent/mcp/mcp_connection_manager.py lines 444-449, await
this wrapper instead of calling validate_mcp_url synchronously.
1 participant
Summary
Hardens MCP client integration against issues reported in #721:
human_input_handler, sampling requests/responses are rejected instead of auto-approved.STDIO already uses
get_default_environment()(safe allowlist); no full-env inherit change needed there.Change
mcp_agent.mcp.url_safety.validate_mcp_urlServerRegistry.start_serverandMCPConnectionManager.launch_serverallow_private_urlson server config (getattr-safe)Test plan
tests/mcp/test_url_safety.py(5 passed)Fixes #721
Summary by CodeRabbit
Security Improvements
Bug Fixes
Tests