Skip to content

fix(mcp): replace single-IP pinning with validate-at-connect SSRF guard#5823

Merged
waleedlatif1 merged 7 commits into
stagingfrom
fix/mcp-ssrf-validate-at-connect
Jul 22, 2026
Merged

fix(mcp): replace single-IP pinning with validate-at-connect SSRF guard#5823
waleedlatif1 merged 7 commits into
stagingfrom
fix/mcp-ssrf-validate-at-connect

Conversation

@waleedlatif1

Copy link
Copy Markdown
Collaborator

Summary

  • Replaces the MCP transport + OAuth guard's single-IP pinning with the standard validate-at-connect SSRF pattern (LibreChat 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).
  • Adversarially security-reviewed before shipping (dedicated red-team pass with live undici tests). Verdict: the hostname rebinding/TOCTOU window is genuinely closed (proven), IPv6 embedded-private forms all blocked (NAT64/6to4/Teredo/v4-mapped verified against isPrivateOrReservedIP). Two findings — both closed in this PR:
    1. IP-literal redirect bypass (shared by the old pin, not a regression): Node skips a custom lookup for numeric hosts, so a 3xx to http://169.254.169.254/ dialed directly. Redirects are now followed manually with per-hop validation (protocol + IP-literal checks, 5-hop cap).
    2. Cross-origin header retention: undici keeps custom headers (e.g. x-api-key) across cross-origin redirects. Custom headers are now dropped on any cross-origin hop.
  • Policy gating unchanged: guard activates exactly where the pin did (validateMcpServerSsrf non-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

  • Bug fix / security hardening

Testing

  • 397 tests green across the MCP + security suites, including new coverage: rebinding (public→private flip blocked on second connect), mixed private+public DNS answers filtered, redirect to metadata IP-literal blocked, bracketed IPv6 literal blocked, non-http(s) protocols blocked, hop cap, 303-POST→GET and 307 semantics, cross-origin header stripping, same-origin header retention.
  • tsc + biome clean.

Checklist

  • Code follows project style guidelines
  • Self-reviewed my changes
  • Tests added/updated and passing
  • No new warnings introduced
  • I confirm that I have read and agree to the terms outlined in the Contributor License Agreement (CLA)

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.
@vercel

vercel Bot commented Jul 21, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Jul 21, 2026 11:52pm

Request Review

@cursor

cursor Bot commented Jul 21, 2026

Copy link
Copy Markdown

PR Summary

High Risk
Changes security-critical outbound networking for user-controlled MCP hosts, including redirect and header semantics; regressions could break connections or weaken SSRF defenses.

Overview
Replaces single-IP pinning for MCP transport and OAuth outbound HTTP with a validate-at-connect SSRF model: DNS resolves normally, and each socket connect filters addresses against the private/reserved blocklist via createSsrfGuardedLookup, keeping multiple public addresses for fallback instead of welding to one IP.

Adds createSsrfGuardedFetchWithDispatcher and followRedirectsGuarded so redirects are followed manually with per-hop checks—blocking IP-literal targets (which bypass custom lookup), non-http(s) protocols, a 5-hop cap, cross-origin custom header stripping, and refusal to forward a request body on cross-origin 307/308.

McpClient and createSsrfGuardedMcpFetch now use the guarded connector for public resolutions; self-hosted private/loopback resolutions keep createPinnedPrivateMcpFetch / legacy pin so permitted local aliases still connect. LLM and other pinned-fetch consumers are unchanged.

Reviewed by Cursor Bugbot for commit 8ada731. Configure here.

Comment thread apps/sim/lib/core/security/input-validation.server.ts Outdated
Comment thread apps/sim/lib/core/security/input-validation.server.ts
@greptile-apps

greptile-apps Bot commented Jul 21, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR replaces single-address MCP pinning with connection-time SSRF checks. The main changes are:

  • Filters every DNS result before each guarded socket connection.
  • Follows redirects manually with protocol, address, and hop validation.
  • Removes sensitive headers and blocks request bodies on cross-origin redirects.
  • Preserves private-address pinning for permitted self-hosted deployments.
  • Adds coverage for rebinding, redirects, request options, and transport cleanup.

Confidence Score: 5/5

This looks safe to merge.

  • The Request-input fix preserves method, headers, body, and signal.
  • Bodyless redirect transitions remove the relevant entity headers.
  • No blocking issue remains in the changed paths reviewed.

Important Files Changed

Filename Overview
apps/sim/lib/core/security/input-validation.server.ts Adds connection-time DNS filtering, guarded redirect handling, and preservation of Request options.
apps/sim/lib/mcp/pinned-fetch.ts Uses guarded connections for public addresses while retaining private-address pinning for permitted self-hosted targets.
apps/sim/lib/mcp/client.ts Selects the guarded or private-pinned transport and retains idempotent Agent cleanup.
apps/sim/lib/core/security/ssrf-guarded-lookup.test.ts Covers rebinding, mixed DNS answers, redirect validation, redirect semantics, and cross-origin protections.

Reviews (5): Last reviewed commit: "fix(mcp): pin (not skip) self-hosted pri..." | Re-trigger Greptile

Comment thread apps/sim/lib/core/security/input-validation.server.ts Outdated
Comment thread apps/sim/lib/core/security/input-validation.server.ts Outdated
Comment thread apps/sim/lib/core/security/input-validation.server.ts Outdated
…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.
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

Comment thread apps/sim/lib/core/security/input-validation.server.ts
Comment thread apps/sim/lib/core/security/input-validation.server.ts Outdated
…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.
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

Comment thread apps/sim/lib/mcp/client.ts Outdated
… 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).
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

Comment thread apps/sim/lib/mcp/pinned-fetch.ts
Comment thread apps/sim/lib/core/security/input-validation.server.ts Outdated
…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.
@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

✅ 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.

@waleedlatif1
waleedlatif1 merged commit 3458220 into staging Jul 22, 2026
18 checks passed
@waleedlatif1
waleedlatif1 deleted the fix/mcp-ssrf-validate-at-connect branch July 22, 2026 00:16
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