Uh oh!
There was an error while loading. Please reload this page.
- Notifications
You must be signed in to change notification settings - Fork 3.8k
fix(mcp): replace single-IP pinning with validate-at-connect SSRF guard#5823
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Merged
Uh oh!
There was an error while loading. Please reload this page.
Merged
Changes from all commits
Commits
Show all changes
7 commits
Select commit
Hold shift + click to select a range
f6bd96b
fix(mcp): replace single-IP pinning with validate-at-connect SSRF guard
waleedlatif1 a518963
fix(mcp): restore IPv4 preference and harden the guarded redirect fol…
waleedlatif1 b8c5552
Merge origin/staging (lockfile advisory fix + latest base)
waleedlatif1 b5adb51
fix(mcp): annotate headers double-cast and cancel redirect body befor…
waleedlatif1 ccde12e
fix(mcp): keep self-hosted private-resolving hosts unguarded (old-pin…
waleedlatif1 8ada731
fix(mcp): pin (not skip) self-hosted private resolutions; refuse cros…
waleedlatif1 616abae
chore(mcp): true up stale pinned-era doc comments
waleedlatif1 File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Jump to
Jump to file
Failed to load files.
Loading
Uh oh!
There was an error while loading. Please reload this page.
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -106,7 +106,8 @@ export async function validateUrlWithDNS( | ||
| try { | ||
| // Prefer IPv4: pinning strips Happy Eyeballs' fallback, and a pinned IPv6 address hangs | ||
| // on IPv4-only egress (e.g. AWS NAT gateways). See validateMcpServerSsrf. | ||
| // on IPv4-only egress (e.g. AWS NAT gateways) — still-pinned consumers (providers, SSO, | ||
| // A2A) depend on this ordering. | ||
| const resolved = await dns.lookup(cleanHostname, { all: true, verbatim: true }) | ||
| const { address } = resolved.find((entry) => entry.family === 4) ?? resolved[0] | ||
| @@ -422,6 +423,176 @@ export function createPinnedLookup(resolvedIP: string): LookupFunction { | ||
| } | ||
| } | ||
| /** | ||
| * DNS lookup that resolves normally and validates EVERY resolved address against | ||
| * the SSRF policy at socket-connect time (the LibreChat `getSSRFConnect` pattern). | ||
| * Private/reserved/loopback records are filtered out; if nothing publicly routable | ||
| * remains the connect fails. Because the check runs on each dial — including | ||
| * redirects and reconnects — there is no validated-then-trusted window for a DNS | ||
| * rebind to slip through, and unlike single-IP pinning the connector keeps the | ||
| * full public address set, so the OS/undici can fall back across addresses. | ||
| * IPv4 is ordered first (`verbatim: false`) — our egress is IPv4-only. | ||
| */ | ||
| export function createSsrfGuardedLookup(): LookupFunction { | ||
| return (hostname, options, callback) => { | ||
| dns | ||
| .lookup(hostname, { all: true, verbatim: false }) | ||
| .then((addresses) => { | ||
| const usable = addresses.filter((entry) => !isPrivateOrReservedIP(entry.address)) | ||
| if (usable.length === 0) { | ||
| callback( | ||
| new Error(`Blocked by SSRF policy: ${hostname} has no publicly routable address`), | ||
| '', | ||
| 4 | ||
| ) | ||
| return | ||
| } | ||
| if (options.all) callback(null, usable) | ||
| else callback(null, usable[0].address, usable[0].family) | ||
| }) | ||
| .catch((error) => callback(toError(error), '', 4)) | ||
| } | ||
| } | ||
| const MAX_GUARDED_REDIRECTS = 5 | ||
| /** | ||
| * Rejects a redirect hop whose target is a private/reserved IP LITERAL. Node's | ||
| * `net.connect` bypasses the custom `lookup` for numeric hosts (`isIP(host)` | ||
| * short-circuits), so the connect-time guard never sees IP-literal dials — | ||
| * a 3xx to `http://169.254.169.254/` would otherwise connect directly. Hostname | ||
| * targets are covered by {@link createSsrfGuardedLookup} at connect time. | ||
| */ | ||
| function assertGuardedRedirectTarget(url: URL): void { | ||
| if (url.protocol !== 'http:' && url.protocol !== 'https:') { | ||
| throw new Error(`Blocked by SSRF policy: redirect to unsupported protocol ${url.protocol}`) | ||
| } | ||
| const host = | ||
| url.hostname.startsWith('[') && url.hostname.endsWith(']') | ||
| ? url.hostname.slice(1, -1) | ||
| : url.hostname | ||
| if (ipaddr.isValid(host) && isPrivateOrReservedIP(host)) { | ||
| throw new Error('Blocked by SSRF policy: redirect to a private or reserved address') | ||
| } | ||
| } | ||
| /** | ||
| * Manual, revalidating redirect follower used by the guarded fetch. Auto-follow | ||
| * is unsafe here on two counts the connect-time lookup cannot cover: IP-literal | ||
| * redirect targets bypass the lookup entirely (validated per hop instead), and | ||
| * undici retains CUSTOM request headers across cross-origin redirects (it strips | ||
| * only Authorization/Cookie) — so caller headers are dropped on any cross-origin | ||
| * hop. Exported for tests. | ||
| */ | ||
| export async function followRedirectsGuarded( | ||
| rawFetch: (url: string, init: UndiciRequestInit) => Promise<Response>, | ||
| input: string, | ||
| init: UndiciRequestInit | ||
| ): Promise<Response> { | ||
| let currentUrl = new URL(input) | ||
| // The initial URL gets the same IP-literal check as redirect hops, so the exported | ||
| // guard is self-contained even when a caller skips its own up-front validation. | ||
| assertGuardedRedirectTarget(currentUrl) | ||
| let method = (init.method ?? 'GET').toUpperCase() | ||
| let body = init.body | ||
| let headers = init.headers | ||
| for (let hop = 0; ; hop++) { | ||
| const response = await rawFetch(currentUrl.href, { | ||
| ...init, | ||
| method, | ||
| body, | ||
| headers, | ||
| redirect: 'manual', | ||
| }) | ||
| const status = response.status | ||
| const location = response.headers.get('location') | ||
| if (![301, 302, 303, 307, 308].includes(status) || !location) return response | ||
| // Cancel the redirect body up front so the throw paths below (hop cap, blocked | ||
| // target) can't leave a socket checked out on the long-lived Agent. | ||
| await response.body?.cancel().catch(() => {}) | ||
| if (hop >= MAX_GUARDED_REDIRECTS) { | ||
| throw new Error(`Blocked by SSRF policy: more than ${MAX_GUARDED_REDIRECTS} redirects`) | ||
| } | ||
| const nextUrl = new URL(location, currentUrl) | ||
| assertGuardedRedirectTarget(nextUrl) | ||
| // Per the fetch spec: 303 (and 301/302 on POST) switch to a bodyless GET, dropping | ||
| // the entity headers that described the removed body (a retained Content-Length / | ||
| // Content-Type on a bodyless GET is malformed and undici rejects it). | ||
| if (status === 303 || ((status === 301 || status === 302) && method === 'POST')) { | ||
| method = 'GET' | ||
| body = undefined | ||
| if (headers !== undefined) { | ||
| const sanitized = new Headers(headers as HeadersInit) | ||
| sanitized.delete('content-length') | ||
| sanitized.delete('content-type') | ||
| sanitized.delete('content-encoding') | ||
| sanitized.delete('transfer-encoding') | ||
| // double-cast-allowed: Headers is a valid undici HeadersInit at runtime but the DOM/undici types differ | ||
| headers = sanitized as unknown as UndiciRequestInit['headers'] | ||
| } | ||
| } | ||
| if (nextUrl.origin !== currentUrl.origin) { | ||
| headers = undefined | ||
| // 307/308 preserve method+body; forwarding a body cross-origin can hand OAuth | ||
| // client secrets / tokens to an open-redirect target now that redirects really | ||
| // dial the new origin. No legitimate MCP/OAuth flow does this — refuse it. | ||
| if (body !== undefined && body !== null) { | ||
| throw new Error( | ||
| 'Blocked by SSRF policy: cross-origin redirect would forward a request body' | ||
| ) | ||
| } | ||
| } | ||
| currentUrl = nextUrl | ||
| } | ||
waleedlatif1 marked this conversation as resolved.
Uh oh!There was an error while loading. Please reload this page. | ||
| } | ||
| /** | ||
| * SSRF-guarded `fetch` + its `Agent` for outbound requests to user-controlled | ||
| * hosts: DNS resolves normally, and every socket connect validates the chosen | ||
| * addresses via {@link createSsrfGuardedLookup}; redirects are followed manually | ||
| * with per-hop validation (see {@link followRedirectsGuarded}) so IP-literal | ||
| * targets can't bypass the lookup and custom headers never cross origins. See | ||
| * {@link createPinnedFetchWithDispatcher} for the `maxResponseSize` semantics. | ||
| */ | ||
| export function createSsrfGuardedFetchWithDispatcher(options?: { maxResponseSize?: number }): { | ||
| fetch: typeof fetch | ||
| dispatcher: Agent | ||
| } { | ||
| const dispatcher = new Agent({ | ||
| allowH2: false, | ||
| connect: { lookup: createSsrfGuardedLookup() }, | ||
| ...(options?.maxResponseSize !== undefined ? { maxResponseSize: options.maxResponseSize } : {}), | ||
| }) | ||
| const rawFetch = async (url: string, init: UndiciRequestInit): Promise<Response> => { | ||
| const response = await undiciFetch(url, { ...init, dispatcher }) | ||
| // double-cast-allowed: undici Response and DOM Response are structurally compatible at runtime | ||
| return response as unknown as Response | ||
| } | ||
| const guarded = async (input: RequestInfo | URL, init?: RequestInit): Promise<Response> => { | ||
| const target = typeof input === 'string' ? input : input instanceof URL ? input.href : input.url | ||
| // A Request input carries its own method/headers/body/signal; lift them into the | ||
| // init (explicit init fields win, per fetch semantics) so the manual redirect | ||
| // follower doesn't silently downgrade a guarded POST Request to a bare GET. | ||
| let effectiveInit: RequestInit = init ?? {} | ||
| if (typeof Request !== 'undefined' && input instanceof Request) { | ||
| const bodyAllowed = input.method !== 'GET' && input.method !== 'HEAD' | ||
| effectiveInit = { | ||
| method: input.method, | ||
| headers: input.headers, | ||
| body: bodyAllowed ? await input.clone().arrayBuffer() : undefined, | ||
| signal: input.signal, | ||
| ...init, | ||
| } | ||
| } | ||
| // double-cast-allowed: DOM RequestInit and undici RequestInit are structurally compatible at runtime but the TS types differ | ||
| return followRedirectsGuarded(rawFetch, target, effectiveInit as unknown as UndiciRequestInit) | ||
| } | ||
| return { fetch: guarded, dispatcher } | ||
| } | ||
| /** | ||
| * Builds a standard `fetch`-compatible function that pins every outbound | ||
| * connection to `resolvedIP`, preventing DNS-rebinding (TOCTOU) between URL | ||
Oops, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Oops, something went wrong.
Uh oh!
There was an error while loading. Please reload this page.
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Uh oh!
There was an error while loading. Please reload this page.