Skip to content

fix(mcp-store): connect to validated IPs for upstream MCP calls - #95848

Open
AUTHENSOR wants to merge 1 commit into
PostHog:masterfrom
AUTHENSOR:redthread/fix-mcp-store-ssrf-pinning
Open

fix(mcp-store): connect to validated IPs for upstream MCP calls#95848
AUTHENSOR wants to merge 1 commit into
PostHog:masterfrom
AUTHENSOR:redthread/fix-mcp-store-ssrf-pinning

Conversation

@AUTHENSOR

Copy link
Copy Markdown

Problem

  • Adding an MCP server means the store fetches from a member-supplied URL. Before connecting, check_mcp_url_policy validates the URL and resolves the hostname — then the connection itself is opened by a plain httpx.Client, which resolves the hostname a second time.

  • Two resolutions of the same attacker-controlled hostname is a DNS-rebinding window. A DNS record can answer the validator with a public IP and answer the actual connect with 127.0.0.1, a cluster-internal address, or a cloud metadata IP.

  • When that happens, the proxy POSTs the org's upstream credential to the internal host and streams the response back. From a pre-fix repro of this chain, the internal listener captured:

    [internal] Authorization header captured: Bearer sk-live-ORG-MCP-SECRET-4f1a
    
  • The repo already documents this exact requirement in posthog/security/pinned_requests.py: validate_url_and_pin_ips alone leaves a TOCTOU window, and callers that open a connection afterwards must use the returned IPs via PinnedIPAdapter instead of re-resolving DNS. Teams webhook delivery, CIMD, and business_knowledge do this. The MCP store (proxy.py, and the same shape twice in tools.py) did not.

Changes

  • New ValidatingPinnedTransport in products/mcp_store/backend/pinned_transport.py: an httpx.HTTPTransport subclass that runs the repo's own validate_url_and_pin_ips at connect time, for every request through the client, and rewrites the connection to the validated IP so httpcore never re-resolves the hostname.
    • Blocked URLs raise before any socket exists; SSRFBlockedError maps to a 400 {"error": "URL not allowed: …"} in the proxy and ToolsFetchError/ToolCallError in tools, matching the existing error surface.
    • The Host header keeps the original netloc and sni_hostname keeps TLS verification on the original hostname.
    • Redirects are never auto-followed; each same-origin redirect retry goes through the transport again, so every hop gets a fresh resolve-validate-pin.
    • The validated IP set is fed through the existing select_pinned_ip (the same chooser the requests-side machinery uses). An empty pin set (dev-mode SSRF bypass) passes through untouched, and the operator's internal allowlist is honored, both matching PinnedIPAdapter's posture.
  • Wired into all three call sites: proxy_mcp_request, fetch_upstream_tools, call_upstream_tool. The transport is passed where the httpx.Client is constructed, so existing tests' patch targets and timeout-kwarg assertions stay intact. The check_mcp_url_policy preflights are kept as fast-fail defense in depth.
  • No signature changes; callers in presentation/views.py, presentation/agent_views.py, and gateway tasks are unaffected.

How did you test this code?

  • New test_pinned_transport.py, mirroring the conventions of posthog/security/test/test_pinned_requests.py: boundary patches of validate_url_and_pin_ips, host-rewrite/Host/SNI assertions for both IPv4 and IPv6, and a live-socket test asserting the connection lands on the pinned IP.
  • New ssrf_blocked_at_connect row in test_proxy.py's parameterized error-mapping test.
  • A stdlib harness exercising both directions of the fix: the original rebind (validator sees public, connect resolves internal) no longer reaches the internal service and no credential leaves the process; a resolution that only turns internal at connect time is refused with 400; and a benign allowed fetch still succeeds end-to-end while DNS for it is poisoned to a dead address, proving the connection uses the pinned IP.
  • ruff check and ruff format pass on the touched files at the repo-pinned version.
  • Not run locally: the full pytest suite (local checkout has no dev deps installed). Deferred to CI on this branch.

Automatic notifications

  • Publish to changelog?

Docs update

None. This is an internal hardening change with no user-facing behavior change for allowed URLs.

🤖 Agent context

Autonomy: Fully autonomous

Found during an authorized security review of the MCP store's outbound request path; this PR ships only the fix. The key decision was pinning at connect time inside an httpx.HTTPTransport rather than pre-resolving at the call sites, so validation cannot be raced by a second resolution, and every redirect hop is covered without new per-call-site state.

The MCP store validated each upstream URL (check_mcp_url_policy) and then
opened the connection with a plain httpx.Client, which re-resolved the
hostname at connect time. A DNS record that answers the validator with a
public IP and the connector with an internal one slipped the org's
upstream credential past the check. Add ValidatingPinnedTransport, which
runs validate_url_and_pin_ips per request at connect time and rewrites the
connection to the validated IP, mirroring pinned_requests used elsewhere.
@trunk-io

trunk-io Bot commented Sep 7, 2026

Copy link
Copy Markdown

Merging to master in this repository is managed by Trunk.

  • To merge this pull request, check the box to the left or comment /trunk merge below.

After your PR is submitted to the merge queue, this comment will be automatically updated with its status. If the PR fails, failure details will also be posted here

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant