fix(mcp): replace single-IP pinning with validate-at-connect SSRF guard#5823
Conversation
Swaps the MCP transport + OAuth guard from pin-one-resolved-IP to the standard pattern (LibreChat getSSRFConnect): DNS resolves normally and EVERY socket connect filters the resolved addresses against the private/reserved blocklist, closing the validate-then-trust window a rebind could race and removing the non-standard mechanism implicated in the headers-then-no-body stalls (no reference MCP client pins IPs; a pinned attempt welded to a bad flow cannot escape, while retries rotate via resolver round-robin and succeed). Adversarially reviewed before shipping; both findings closed here: - Redirects are now followed manually with per-hop validation: an IP-literal redirect target (which bypasses ANY connect-time lookup - Node skips the custom lookup for numeric hosts) is checked explicitly, closing a metadata- endpoint redirect hole the old pin also had. - Custom request headers are dropped on cross-origin hops, so a redirect to a second attacker host cannot harvest configured auth headers. Policy gating unchanged: the guard activates exactly where the pin did (validateMcpServerSsrf non-null; allowlist mode / localhost-on-self-hosted stay unguarded). Non-MCP consumers of the pinned fetch are untouched. 397 tests incl. new rebinding, mixed-answer, IP-literal-redirect, and hop-cap coverage.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryHigh Risk Overview Adds
Reviewed by Cursor Bugbot for commit 8ada731. Configure here. |
Greptile SummaryThis PR replaces single-address MCP pinning with connection-time SSRF checks. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (5): Last reviewed commit: "fix(mcp): pin (not skip) self-hosted pri..." | Re-trigger Greptile |
…lower - Restore validateUrlWithDNS's prefer-IPv4 resolution (a stale working-tree hunk accidentally reverted #5798 for the still-pinned non-MCP consumers). - Validate the INITIAL url's IP-literal in followRedirectsGuarded, not just redirect hops, so the exported guard is self-contained. - Drop entity headers (content-length/type/encoding) when a 301/302/303 switches a POST to a bodyless GET, which undici would otherwise reject. - Lift method/headers/body/signal from a Request input instead of silently downgrading a guarded POST Request to a bare GET.
|
@cursor review |
…e throw paths - Add the missing double-cast-allowed annotation on the sanitized-headers cast (strict boundary audit). - Cancel the redirect response body before the hop-cap / blocked-target throws so those paths can't leave a socket checked out on the long-lived Agent.
|
@cursor review |
… parity) A DNS alias resolving to loopback/private is only reachable on self-hosted, where the policy explicitly permits it; the guarded lookup would filter the address and strand the connect where the old pin connected. Both MCP gates (transport + OAuth guard) now route private/loopback resolutions over the unguarded path, same as the localhost carve-out. Test IPs moved off RFC-5737 TEST-NET (which is correctly classified reserved).
|
@cursor review |
…s-origin body forwards - The private/loopback carve-out now keeps the LEGACY PIN to the validated address instead of falling back to unguarded fetch — preserving both the old behavior and its anti-rebinding property for self-hosted DNS aliases. - Cross-origin redirect hops now also refuse to forward a request body (307/308 preserve method+body; post-pin those redirects really dial the new origin, so an open redirect could exfiltrate OAuth client secrets). Bodyless cross-origin redirects still follow. Tests for both.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 8ada731. Configure here.
Summary
getSSRFConnect): DNS resolves normally, and every socket connect filters the resolved addresses against the private/reserved blocklist. No reference MCP client pins IPs; pinning welded each attempt to one address with no fallback and is the prime suspect in the headers-then-zero-body stalls (failed first attempt → instant retry via resolver rotation, 50/50 rotation proven).isPrivateOrReservedIP). Two findings — both closed in this PR:lookupfor numeric hosts, so a 3xx tohttp://169.254.169.254/dialed directly. Redirects are now followed manually with per-hop validation (protocol + IP-literal checks, 5-hop cap).x-api-key) across cross-origin redirects. Custom headers are now dropped on any cross-origin hop.validateMcpServerSsrfnon-null); allowlist mode / localhost-on-self-hosted remain unguarded; OAuth legs keep per-hop up-front validation. Non-MCP pinned-fetch consumers (LLM providers, A2A, probe) untouched.Type of Change
Testing
Checklist