Skip to content

Commit 0f79f27

Browse files
committed
fix(mcp): let the SSRF guard own redirects on OAuth legs the SDK drives
1 parent 8d23dd2 commit 0f79f27

4 files changed

Lines changed: 146 additions & 56 deletions

File tree

‎apps/sim/lib/mcp/oauth/auth.ts‎

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -23,6 +23,8 @@ export function mcpAuthGuarded(
2323
provider.authorizationScope
2424
? { scope: provider.authorizationScope }
2525
: {}),
26-
fetchFn: options.fetchFn ?? createSsrfGuardedMcpFetch({ serverUrl: String(options.serverUrl) }),
26+
fetchFn:
27+
options.fetchFn ??
28+
createSsrfGuardedMcpFetch({ serverUrl: String(options.serverUrl), followRedirects: 'guard' }),
2729
})
2830
}

‎apps/sim/lib/mcp/oauth/revoke.ts‎

Lines changed: 4 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -37,7 +37,10 @@ export async function revokeMcpOauthTokens(
3737
const row = await loadOauthRow({ mcpServerId })
3838
if (!row?.tokens) return
3939

40-
const ssrfGuardedFetch = createSsrfGuardedMcpFetch({ serverUrl: server.url })
40+
const ssrfGuardedFetch = createSsrfGuardedMcpFetch({
41+
serverUrl: server.url,
42+
followRedirects: 'guard',
43+
})
4144
const info = await discoverOAuthServerInfo(server.url, { fetchFn: ssrfGuardedFetch }).catch(
4245
() => undefined
4346
)

‎apps/sim/lib/mcp/pinned-fetch.ts‎

Lines changed: 26 additions & 13 deletions
Original file line numberDiff line numberDiff line change
@@ -1,6 +1,7 @@
11
import type { FetchLike } from '@modelcontextprotocol/sdk/shared/transport.js'
22
import { createLogger } from '@sim/logger'
33
import { isPrivateIp } from '@sim/security/ssrf'
4+
import { omit } from '@sim/utils/object'
45
import {
56
createPinnedFetchWithDispatcher,
67
createSsrfGuardedFetchWithDispatcher,
@@ -125,6 +126,11 @@ function requestHref(input: string | URL | Request): string {
125126
return String(input)
126127
}
127128

129+
/** Drops the caller's `redirect` mode, so the guard follows under per-hop validation. */
130+
function withoutRedirectMode(init: RequestInit | undefined): RequestInit | undefined {
131+
return init?.redirect === undefined ? init : omit(init, ['redirect'])
132+
}
133+
128134
/**
129135
* Wraps the transport's same-origin fetch so a request to any other origin is
130136
* judged as content and validated per request, never inheriting the configured
@@ -137,15 +143,21 @@ function requestHref(input: string | URL | Request): string {
137143
* {@link MCP_EGRESS_PROFILE} (honors the allowlist, permits loopback off-hosted)
138144
* or, worse, be pinned onto the server's private address. Same-origin requests —
139145
* the JSON-RPC transport itself — keep the persistent guarded path.
146+
*
147+
* Both paths follow redirects themselves whatever `redirect` mode arrives: the SDK
148+
* also hands this fetch to its direct `auth()` calls (coordinated refresh), which
149+
* send `redirect: 'manual'` since 1.32.
140150
*/
141151
function splitByConfiguredOrigin(
142152
sameOrigin: typeof fetch,
143153
serverUrl: string | undefined
144154
): typeof fetch {
145-
if (!serverUrl || !URL.canParse(serverUrl)) return sameOrigin
155+
if (!serverUrl || !URL.canParse(serverUrl))
156+
return (input, init) => sameOrigin(input, withoutRedirectMode(init))
146157
const configuredOrigin = new URL(serverUrl).origin
147-
const crossOrigin = createSsrfGuardedMcpFetch({ serverUrl })
148-
return async (input, init) => {
158+
const crossOrigin = createSsrfGuardedMcpFetch({ serverUrl, followRedirects: 'guard' })
159+
return async (input, callerInit) => {
160+
const init = withoutRedirectMode(callerInit)
149161
const target = requestHref(input)
150162
const sameAsConfigured = URL.canParse(target) && new URL(target).origin === configuredOrigin
151163
return sameAsConfigured ? sameOrigin(input, init) : crossOrigin(target, init)
@@ -283,14 +295,11 @@ async function bufferUnderDeadline(response: Response, signal: AbortSignal): Pro
283295
headers.delete('content-encoding')
284296
headers.delete('content-length')
285297
const nullBody = response.status === 204 || response.status === 205 || response.status === 304
286-
const buffered = new Response(nullBody ? null : body, {
298+
return new Response(nullBody ? null : body, {
287299
status: response.status,
288300
statusText: response.statusText,
289301
headers,
290302
})
291-
// The SDK names an unfollowed redirect's target by resolving `Location` against `response.url`.
292-
Object.defineProperty(buffered, 'url', { value: response.url, configurable: true })
293-
return buffered
294303
}
295304

296305
/**
@@ -337,29 +346,33 @@ function releaseStreamOnSettle(
337346
* attacker-controllable authorization-server metadata, so every request is re-validated
338347
* against the SSRF policy up front (fail fast, friendly errors) AND at socket-connect
339348
* time via the guarded lookup, with redirects followed manually under per-hop
340-
* validation (IP-literal targets included) and cross-origin header stripping. The SDK's own
341-
* `auth()` legs pass `redirect: 'manual'`: the 3xx comes back buffered, and the SDK follows
342-
* it only within the same origin, re-entering this guard for each hop.
349+
* validation (IP-literal targets included) and cross-origin header stripping.
343350
*
344351
* The SDK provides none of the safety itself (no timeout on OAuth legs, lazy body reads),
345352
* so the guard owns it: the `timeoutMs` deadline covers validation + request + the buffered
346353
* body read; the per-request guarded Agent is `destroy()`ed on every path; and the returned
347354
* `Response` is a detached in-memory copy. Only our own deadline is relabeled to `McpError`.
348355
*
349356
* @param timeoutMs Per-request deadline in ms (defaults to 30s; override for tests).
357+
* @param followRedirects Who decides how a redirect is handled. `'caller'` (default) honors
358+
* the request's `redirect` mode, which the auth-type probe relies on to see a 3xx as is.
359+
* `'guard'` ignores it and always follows under per-hop validation: pass it for a fetch
360+
* handed to the SDK's `auth()`/discovery, which since 1.32 sends `redirect: 'manual'` and
361+
* follows only same-origin hops itself.
350362
* @throws McpSsrfError if a request URL resolves to a blocked IP address
351363
* @throws McpError if a request exceeds `timeoutMs`
352364
*/
353365
export function createSsrfGuardedMcpFetch(
354-
options: { serverUrl?: string; timeoutMs?: number } = {}
366+
options: { serverUrl?: string; timeoutMs?: number; followRedirects?: 'caller' | 'guard' } = {}
355367
): FetchLike {
356-
const { serverUrl, timeoutMs = OAUTH_FETCH_TIMEOUT_MS } = options
368+
const { serverUrl, timeoutMs = OAUTH_FETCH_TIMEOUT_MS, followRedirects = 'caller' } = options
357369
// The origin the operator configured. A leg that stays on it is the server
358370
// they chose; everything else was named by that server's metadata.
359371
let configuredOrigin: string | undefined
360372
if (serverUrl && URL.canParse(serverUrl)) configuredOrigin = new URL(serverUrl).origin
361373

362-
return (async (url, init) => {
374+
return (async (url, callerInit) => {
375+
const init = followRedirects === 'guard' ? withoutRedirectMode(callerInit) : callerInit
363376
const target = typeof url === 'string' ? url : url.href
364377
const host = URL.canParse(target) ? new URL(target).host : target
365378
const sameAsConfigured =

‎apps/sim/lib/mcp/transport-redirects.server.test.ts‎

Lines changed: 113 additions & 41 deletions
Original file line numberDiff line numberDiff line change
@@ -4,13 +4,13 @@
44
* ports are two origins, and loopback is reachable under the self-hosted profile.
55
*/
66
import { createServer, type IncomingHttpHeaders, type Server } from 'node:http'
7-
import type { AddressInfo, Socket } from 'node:net'
8-
import { exchangeAuthorization } from '@modelcontextprotocol/sdk/client/auth.js'
9-
import { ServerError } from '@modelcontextprotocol/sdk/server/auth/errors.js'
7+
import type { AddressInfo } from 'node:net'
8+
import type { OAuthClientProvider } from '@modelcontextprotocol/sdk/client/auth.js'
109
import { LATEST_PROTOCOL_VERSION } from '@modelcontextprotocol/sdk/types.js'
1110
import { afterEach, describe, expect, it, vi } from 'vitest'
1211
import { McpClient } from '@/lib/mcp/client'
13-
import { createSsrfGuardedMcpFetch } from '@/lib/mcp/pinned-fetch'
12+
import { mcpAuthGuarded } from '@/lib/mcp/oauth/auth'
13+
import { createGuardedMcpFetch } from '@/lib/mcp/pinned-fetch'
1414

1515
interface RecordedRequest {
1616
method: string
@@ -21,7 +21,6 @@ interface RecordedRequest {
2121
interface LocalServer {
2222
origin: string
2323
requests: RecordedRequest[]
24-
openSockets: Set<Socket>
2524
close: () => Promise<void>
2625
}
2726

@@ -35,7 +34,6 @@ const servers: LocalServer[] = []
3534

3635
async function startServer(handler: Handler): Promise<LocalServer> {
3736
const requests: RecordedRequest[] = []
38-
const openSockets = new Set<Socket>()
3937
const server: Server = createServer((req, res) => {
4038
const chunks: Buffer[] = []
4139
req.on('data', (chunk: Buffer) => chunks.push(chunk))
@@ -49,16 +47,11 @@ async function startServer(handler: Handler): Promise<LocalServer> {
4947
})
5048
})
5149
})
52-
server.on('connection', (socket) => {
53-
openSockets.add(socket)
54-
socket.on('close', () => openSockets.delete(socket))
55-
})
5650
await new Promise<void>((resolve) => server.listen(0, '127.0.0.1', resolve))
5751
const { port } = server.address() as AddressInfo
5852
const local: LocalServer = {
5953
origin: `http://127.0.0.1:${port}`,
6054
requests,
61-
openSockets,
6255
close: () =>
6356
new Promise<void>((resolve) => {
6457
server.closeAllConnections()
@@ -134,37 +127,116 @@ describe('MCP transport redirects', () => {
134127
})
135128
})
136129

137-
describe('guarded MCP OAuth fetch', () => {
138-
it('hands an unfollowed cross-origin token redirect back to the SDK, naming its target, and releases the socket', async () => {
139-
const elsewhere = await startServer((_request, _body, respond) => respond(200, {}, '{}'))
140-
const authServer = await startServer((_request, _body, respond) => {
141-
respond(302, { location: `${elsewhere.origin}/token` }, 'moved')
142-
})
130+
const OAUTH_FETCHES = [
131+
{ name: 'the default OAuth fetch', fetchFn: () => undefined },
132+
{
133+
name: 'the transport fetch reused by coordinated refresh',
134+
fetchFn: (serverUrl: string) => createGuardedMcpFetch(serverUrl).fetch,
135+
},
136+
] as const
143137

144-
const exchange = exchangeAuthorization(authServer.origin, {
145-
metadata: {
146-
issuer: authServer.origin,
147-
authorization_endpoint: `${authServer.origin}/authorize`,
148-
token_endpoint: `${authServer.origin}/token`,
149-
response_types_supported: ['code'],
150-
},
151-
clientInformation: { client_id: 'client-1' },
152-
authorizationCode: 'code-1',
153-
codeVerifier: 'verifier-1',
154-
redirectUri: 'http://127.0.0.1/callback',
155-
fetchFn: createSsrfGuardedMcpFetch({
156-
serverUrl: `${authServer.origin}/mcp`,
157-
timeoutMs: 5_000,
158-
}),
159-
})
138+
function recordingProvider() {
139+
const authorizationUrls: URL[] = []
140+
const provider: OAuthClientProvider = {
141+
get redirectUrl() {
142+
return 'http://127.0.0.1/callback'
143+
},
144+
get clientMetadata() {
145+
return { redirect_uris: ['http://127.0.0.1/callback'] }
146+
},
147+
clientInformation: () => ({ client_id: 'client-1' }),
148+
tokens: () => undefined,
149+
saveTokens: () => {},
150+
redirectToAuthorization: (url) => {
151+
authorizationUrls.push(url)
152+
},
153+
saveCodeVerifier: () => {},
154+
codeVerifier: () => 'verifier-1',
155+
}
156+
return { provider, authorizationUrls }
157+
}
160158

161-
const error = await exchange.catch((caught: unknown) => caught)
162-
expect(error).toBeInstanceOf(ServerError)
163-
expect((error as Error).message).toBe(
164-
`HTTP 302: Redirect to ${elsewhere.origin}/token not followed`
165-
)
166-
expect(authServer.requests.map((request) => request.method)).toEqual(['POST'])
167-
expect(elsewhere.requests).toHaveLength(0)
168-
await vi.waitFor(() => expect(authServer.openSockets.size).toBe(0), { timeout: 2_000 })
159+
function authorizationServerMetadata(origin: string) {
160+
return JSON.stringify({
161+
issuer: origin,
162+
authorization_endpoint: `${origin}/authorize`,
163+
token_endpoint: `${origin}/token`,
164+
response_types_supported: ['code'],
165+
code_challenge_methods_supported: ['S256'],
169166
})
167+
}
168+
169+
describe('MCP OAuth redirects', () => {
170+
it.each(OAUTH_FETCHES)(
171+
'follows a cross-origin redirect of protected-resource discovery through $name, without its headers',
172+
async ({ fetchFn }) => {
173+
const json = { 'content-type': 'application/json' }
174+
let apexOrigin = ''
175+
const www = await startServer((_request, _body, respond) => {
176+
respond(
177+
200,
178+
json,
179+
JSON.stringify({ resource: `${apexOrigin}/mcp`, authorization_servers: [apexOrigin] })
180+
)
181+
})
182+
const apex = await startServer((request, _body, respond) => {
183+
if (request.url === '/.well-known/oauth-protected-resource/mcp') {
184+
respond(302, { location: `${www.origin}${request.url}` }, '')
185+
} else if (request.url === '/.well-known/oauth-authorization-server') {
186+
respond(200, json, authorizationServerMetadata(apexOrigin))
187+
} else {
188+
respond(404, {}, '')
189+
}
190+
})
191+
apexOrigin = apex.origin
192+
const serverUrl = `${apex.origin}/mcp`
193+
const { provider, authorizationUrls } = recordingProvider()
194+
195+
await expect(
196+
mcpAuthGuarded(provider, { serverUrl, fetchFn: fetchFn(serverUrl) })
197+
).resolves.toBe('REDIRECT')
198+
199+
expect(apex.requests[0].headers['mcp-protocol-version']).toBeDefined()
200+
expect(www.requests).toHaveLength(1)
201+
expect(www.requests[0].headers['mcp-protocol-version']).toBeUndefined()
202+
expect(www.requests[0].headers.authorization).toBeUndefined()
203+
expect(authorizationUrls).toHaveLength(1)
204+
expect(authorizationUrls[0].searchParams.get('resource')).toBe(serverUrl)
205+
}
206+
)
207+
208+
it.each(OAUTH_FETCHES)(
209+
'refuses a cross-origin 307 of the token request through $name, as it would forward the body',
210+
async ({ fetchFn }) => {
211+
const elsewhere = await startServer((_request, _body, respond) => respond(200, {}, '{}'))
212+
const apex = await startServer((request, _body, respond) => {
213+
if (request.url === '/.well-known/oauth-authorization-server') {
214+
respond(
215+
200,
216+
{ 'content-type': 'application/json' },
217+
authorizationServerMetadata(apex.origin)
218+
)
219+
} else if (request.url === '/token') {
220+
respond(307, { location: `${elsewhere.origin}/token` }, '')
221+
} else {
222+
respond(404, {}, '')
223+
}
224+
})
225+
const serverUrl = `${apex.origin}/mcp`
226+
const { provider } = recordingProvider()
227+
228+
await expect(
229+
mcpAuthGuarded(provider, {
230+
serverUrl,
231+
authorizationCode: 'code-1',
232+
fetchFn: fetchFn(serverUrl),
233+
})
234+
).rejects.toThrow(
235+
'Blocked by SSRF policy: cross-origin redirect would forward a request body'
236+
)
237+
238+
expect(apex.requests.filter((request) => request.url === '/token')).toHaveLength(1)
239+
expect(elsewhere.requests).toHaveLength(0)
240+
}
241+
)
170242
})

0 commit comments

Comments
 (0)