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): stream pinned transport under Bun (providers + self-hosted-private MCP)#5901
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
5 commits
Select commit
Hold shift + click to select a range
ceac9a2
fix(mcp): stream pinned transport via undici.request + redirect inter…
waleedlatif1 128943d
fix(mcp): honor redirect mode + drop cross-origin credentials on pinn…
waleedlatif1 80b5d08
fix(mcp): don't block private IP-literal URLs on the pinned fetch path
waleedlatif1 39c7515
fix(mcp): carry redirect mode from a Request input in liftFetchArgs
waleedlatif1 8b376a0
fix(mcp): permit the pinned IP as a redirect target (initial + hops),…
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 |
|---|---|---|
| @@ -14,7 +14,6 @@ import { | ||
| Agent, | ||
| type Dispatcher, | ||
| type RequestInit as UndiciRequestInit, | ||
| fetch as undiciFetch, | ||
| request as undiciRequest, | ||
| } from 'undici' | ||
| import { isHosted, isPrivateDatabaseHostsAllowed } from '@/lib/core/config/env-flags' | ||
| @@ -544,7 +543,7 @@ const MAX_GUARDED_REDIRECTS = 5 | ||
| * 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 { | ||
| function assertGuardedRedirectTarget(url: URL, allowedPinnedIp?: string): void { | ||
| if (url.protocol !== 'http:' && url.protocol !== 'https:') { | ||
| throw new Error(`Blocked by SSRF policy: redirect to unsupported protocol ${url.protocol}`) | ||
| } | ||
| @@ -553,6 +552,16 @@ function assertGuardedRedirectTarget(url: URL): void { | ||
| ? url.hostname.slice(1, -1) | ||
| : url.hostname | ||
| if (ipaddr.isValid(host) && isPrivateOrReservedIP(host)) { | ||
| // The pinned-private carve-out permits exactly its own validated IP as a target (a | ||
| // self-hosted MCP on a private IP, or a same-host redirect that stays on it) — but nothing | ||
| // else private (a redirect to e.g. the cloud metadata IP is still blocked). | ||
| if ( | ||
| allowedPinnedIp && | ||
| ipaddr.isValid(allowedPinnedIp) && | ||
| ipaddr.process(host).toString() === ipaddr.process(allowedPinnedIp).toString() | ||
| ) { | ||
| return | ||
| } | ||
| throw new Error('Blocked by SSRF policy: redirect to a private or reserved address') | ||
| } | ||
| } | ||
| @@ -568,12 +577,15 @@ function assertGuardedRedirectTarget(url: URL): void { | ||
| export async function followRedirectsGuarded( | ||
| rawFetch: (url: string, init: UndiciRequestInit) => Promise<Response>, | ||
| input: string, | ||
| init: UndiciRequestInit | ||
| init: UndiciRequestInit, | ||
| options?: { allowRedirectToIp?: string } | ||
| ): 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) | ||
| // 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. `allowRedirectToIp` | ||
| // (the pinned-private MCP carve-out's validated IP) permits that one private target — both the | ||
| // initial URL and any hop that stays on it — while everything else private stays blocked. | ||
| assertGuardedRedirectTarget(currentUrl, options?.allowRedirectToIp) | ||
| let method = (init.method ?? 'GET').toUpperCase() | ||
| let body = init.body | ||
| let headers = init.headers | ||
| @@ -587,15 +599,21 @@ export async function followRedirectsGuarded( | ||
| }) | ||
| const status = response.status | ||
| const location = response.headers.get('location') | ||
| if (![301, 302, 303, 307, 308].includes(status) || !location) return response | ||
| if (![301, 302, 303, 307, 308].includes(status) || !location) { | ||
| // `response.url` is already the final hop's URL (set per-request by the raw fetch); flag | ||
| // `redirected` too when at least one hop was followed, matching fetch semantics. | ||
| if (hop > 0) | ||
| Object.defineProperty(response, 'redirected', { value: true, configurable: true }) | ||
| 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) | ||
| assertGuardedRedirectTarget(nextUrl, options?.allowRedirectToIp) | ||
| // 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). | ||
| @@ -781,7 +799,7 @@ function nodeReadableToWebStream(nodeStream: Readable): ReadableStream<Uint8Arra | ||
| async function undiciRequestAsResponse( | ||
| input: RequestInfo | URL, | ||
| init: RequestInit, | ||
| dispatcher: Agent | ||
| dispatcher: Dispatcher | ||
| ): Promise<Response> { | ||
| let url: string | ||
| let effectiveInit = init as UndiciRequestInit | ||
| @@ -880,6 +898,36 @@ async function undiciRequestAsResponse( | ||
| } | ||
| } | ||
| /** | ||
| * Normalizes a `fetch(input, init)` call into a URL string + init. A `Request` input carries | ||
| * its own method/headers/body/signal; lift them into the init (explicit init fields win, per | ||
| * fetch semantics) so a manual redirect follower can't silently downgrade a POST Request to a | ||
| * bare GET or lose its headers. | ||
| */ | ||
| async function liftFetchArgs( | ||
| input: RequestInfo | URL, | ||
| init?: RequestInit | ||
| ): Promise<{ target: string; effectiveInit: RequestInit }> { | ||
| const target = typeof input === 'string' ? input : input instanceof URL ? input.href : input.url | ||
| if (typeof Request !== 'undefined' && input instanceof Request) { | ||
| const bodyAllowed = input.method !== 'GET' && input.method !== 'HEAD' | ||
| return { | ||
| target, | ||
| effectiveInit: { | ||
| method: input.method, | ||
| headers: input.headers, | ||
| body: bodyAllowed ? await input.clone().arrayBuffer() : undefined, | ||
| signal: input.signal, | ||
| // Carry the Request's redirect mode so the pinned fetch honors `manual`/`error` | ||
| // instead of defaulting a `Request({ redirect: 'manual' })` to `follow`. | ||
| redirect: input.redirect, | ||
| ...init, | ||
| }, | ||
| } | ||
| } | ||
| return { target, effectiveInit: init ?? {} } | ||
| } | ||
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 | ||
| @@ -903,21 +951,7 @@ export function createSsrfGuardedFetchWithDispatcher(options?: { maxResponseSize | ||
| undiciRequestAsResponse(url, init as unknown as RequestInit, dispatcher) | ||
| 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, | ||
| } | ||
| } | ||
| const { target, effectiveInit } = await liftFetchArgs(input, 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) | ||
| } | ||
| @@ -977,14 +1011,42 @@ export function createPinnedFetchWithDispatcher( | ||
| ...(options?.maxResponseSize !== undefined ? { maxResponseSize: options.maxResponseSize } : {}), | ||
| }) | ||
| const rawFetch = (url: string, init: UndiciRequestInit): Promise<Response> => | ||
| // double-cast-allowed: DOM RequestInit and undici RequestInit differ in TS but match at runtime | ||
| undiciRequestAsResponse(url, init as unknown as RequestInit, dispatcher) | ||
| // Requests go through `undici.request` (not `undici.fetch`) because fetch's streaming | ||
| // `response.body` never delivers under the Bun runtime the server runs on — the same bug | ||
| // {@link createSsrfGuardedFetchWithDispatcher} works around. Redirects are handled here (not | ||
| // by a caller's wrapper — the pinned fetch is passed straight to provider/A2A SDKs), honoring | ||
| // the request's `redirect` mode: `manual`/`error` must NOT transparently follow (e.g. | ||
| // `detectMcpAuthType` inspects the 3xx to classify auth). The default `follow` uses | ||
| // {@link followRedirectsGuarded}, which drops headers on cross-origin hops (so a redirect | ||
| // can't disclose a provider `api-key` to another origin) and stamps the final `response.url`. | ||
| // Every hop still dispatches through the pinned `Agent` (its `connect.lookup` forces | ||
| // `resolvedIP`), so a redirect can't escape to another address. | ||
| const pinned = async (input: RequestInfo | URL, init?: RequestInit): Promise<Response> => { | ||
| // double-cast-allowed: DOM RequestInfo/URL and undici fetch input types differ but are structurally compatible at runtime (Node's global fetch IS undici) | ||
| const undiciInput = input as unknown as Parameters<typeof undiciFetch>[0] | ||
| const { target, effectiveInit } = await liftFetchArgs(input, init) | ||
| const mode = effectiveInit.redirect ?? 'follow' | ||
| // double-cast-allowed: DOM RequestInit and undici RequestInit are structurally compatible at runtime but the TS types differ | ||
| const undiciInit: UndiciRequestInit = { ...(init as unknown as UndiciRequestInit), dispatcher } | ||
| const response = await undiciFetch(undiciInput, undiciInit) | ||
| // double-cast-allowed: undici Response and DOM Response are structurally compatible at runtime | ||
| return response as unknown as Response | ||
| const undiciInit = effectiveInit as unknown as UndiciRequestInit | ||
| if (mode === 'manual') { | ||
| return rawFetch(target, undiciInit) | ||
| } | ||
| if (mode === 'error') { | ||
| const response = await rawFetch(target, undiciInit) | ||
| const location = response.headers.get('location') | ||
| if (response.status >= 300 && response.status < 400 && location) { | ||
| await response.body?.cancel().catch(() => {}) | ||
| throw new TypeError('Pinned fetch received an unexpected redirect (redirect: "error")') | ||
| } | ||
| return response | ||
| } | ||
| // Permit this pinned IP as a redirect/initial target even when it's private (the | ||
| // self-hosted MCP carve-out on a private/loopback IP, and same-host redirects that stay on | ||
| // it) — otherwise the guarded policy would block a self-hosted server reaching itself. Any | ||
| // OTHER private target (e.g. a redirect to the cloud metadata IP) is still blocked. | ||
| return followRedirectsGuarded(rawFetch, target, undiciInit, { allowRedirectToIp: resolvedIP }) | ||
| } | ||
| return { fetch: pinned, dispatcher } | ||
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.