Skip to content

Commit 8d23dd2

Browse files
committed
improvement(mcp): upgrade MCP SDK to 1.32.1 and keep cross-origin redirect handling in our SSRF guard
1 parent efc8106 commit 8d23dd2

6 files changed

Lines changed: 184 additions & 6 deletions

File tree

‎apps/sim/app/api/mcp/oauth/start/route.ts‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,7 @@ const DCR_UNSUPPORTED_MESSAGE =
4949
/**
5050
* The MCP SDK throws a plain `Error` (no typed class or code) when an auth server lacks a
5151
* `registration_endpoint`, so this string-match is the only available signal. Pinned to
52-
* `@modelcontextprotocol/sdk` v1.31.0 `registerClient` (client/auth.js): "Incompatible auth
52+
* `@modelcontextprotocol/sdk` v1.32.1 `registerClient` (client/auth.js): "Incompatible auth
5353
* server: does not support dynamic client registration". If a version bump rephrases this, the
5454
* 422 branch silently stops firing (users get a generic 500) — update the substring here.
5555
*/

‎apps/sim/lib/mcp/client.ts‎

Lines changed: 3 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -150,6 +150,9 @@ export class McpClient {
150150
authProvider: useOauth ? this.authProvider : undefined,
151151
...(useOauth ? {} : { requestInit: { headers: this.config.headers } }),
152152
fetch: (input, init) => runInOutboundScope(() => transportFetch(input, init)),
153+
// The SDK's default ('same-origin') refuses cross-origin redirects; the guarded
154+
// fetch already follows them with per-hop SSRF checks and cross-origin header stripping.
155+
redirectPolicy: 'follow',
153156
})
154157

155158
this.client = new Client(

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

Lines changed: 7 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -283,11 +283,14 @@ async function bufferUnderDeadline(response: Response, signal: AbortSignal): Pro
283283
headers.delete('content-encoding')
284284
headers.delete('content-length')
285285
const nullBody = response.status === 204 || response.status === 205 || response.status === 304
286-
return new Response(nullBody ? null : body, {
286+
const buffered = new Response(nullBody ? null : body, {
287287
status: response.status,
288288
statusText: response.statusText,
289289
headers,
290290
})
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
291294
}
292295

293296
/**
@@ -334,7 +337,9 @@ function releaseStreamOnSettle(
334337
* attacker-controllable authorization-server metadata, so every request is re-validated
335338
* against the SSRF policy up front (fail fast, friendly errors) AND at socket-connect
336339
* time via the guarded lookup, with redirects followed manually under per-hop
337-
* validation (IP-literal targets included) and cross-origin header stripping.
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.
338343
*
339344
* The SDK provides none of the safety itself (no timeout on OAuth legs, lazy body reads),
340345
* so the guard owns it: the `timeoutMs` deadline covers validation + request + the buffered
Lines changed: 170 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,170 @@
1+
/**
2+
* Redirects across the MCP SDK and our SSRF guard, over real loopback sockets.
3+
* Nothing between the SDK and the wire is mocked: two local servers on different
4+
* ports are two origins, and loopback is reachable under the self-hosted profile.
5+
*/
6+
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'
10+
import { LATEST_PROTOCOL_VERSION } from '@modelcontextprotocol/sdk/types.js'
11+
import { afterEach, describe, expect, it, vi } from 'vitest'
12+
import { McpClient } from '@/lib/mcp/client'
13+
import { createSsrfGuardedMcpFetch } from '@/lib/mcp/pinned-fetch'
14+
15+
interface RecordedRequest {
16+
method: string
17+
url: string
18+
headers: IncomingHttpHeaders
19+
}
20+
21+
interface LocalServer {
22+
origin: string
23+
requests: RecordedRequest[]
24+
openSockets: Set<Socket>
25+
close: () => Promise<void>
26+
}
27+
28+
type Handler = (
29+
request: RecordedRequest,
30+
body: string,
31+
respond: (status: number, headers: Record<string, string>, body?: string) => void
32+
) => void
33+
34+
const servers: LocalServer[] = []
35+
36+
async function startServer(handler: Handler): Promise<LocalServer> {
37+
const requests: RecordedRequest[] = []
38+
const openSockets = new Set<Socket>()
39+
const server: Server = createServer((req, res) => {
40+
const chunks: Buffer[] = []
41+
req.on('data', (chunk: Buffer) => chunks.push(chunk))
42+
req.on('end', () => {
43+
const recorded = { method: req.method ?? '', url: req.url ?? '', headers: req.headers }
44+
requests.push(recorded)
45+
handler(recorded, Buffer.concat(chunks).toString('utf8'), (status, headers, body) => {
46+
res.writeHead(status, headers)
47+
if (body === undefined) res.flushHeaders()
48+
else res.end(body)
49+
})
50+
})
51+
})
52+
server.on('connection', (socket) => {
53+
openSockets.add(socket)
54+
socket.on('close', () => openSockets.delete(socket))
55+
})
56+
await new Promise<void>((resolve) => server.listen(0, '127.0.0.1', resolve))
57+
const { port } = server.address() as AddressInfo
58+
const local: LocalServer = {
59+
origin: `http://127.0.0.1:${port}`,
60+
requests,
61+
openSockets,
62+
close: () =>
63+
new Promise<void>((resolve) => {
64+
server.closeAllConnections()
65+
server.close(() => resolve())
66+
}),
67+
}
68+
servers.push(local)
69+
return local
70+
}
71+
72+
afterEach(async () => {
73+
await Promise.all(servers.splice(0).map((server) => server.close()))
74+
})
75+
76+
describe('MCP transport redirects', () => {
77+
it('follows a cross-origin redirect of the SSE stream through the guard, without the configured credentials', async () => {
78+
const www = await startServer((_request, _body, respond) => {
79+
respond(200, { 'content-type': 'text/event-stream' })
80+
})
81+
const apex = await startServer((request, body, respond) => {
82+
if (request.method === 'GET') {
83+
respond(302, { location: `${www.origin}/mcp` }, '')
84+
return
85+
}
86+
if (request.method === 'DELETE') {
87+
respond(200, {}, '')
88+
return
89+
}
90+
const message = JSON.parse(body) as { id?: number; method: string }
91+
if (message.method !== 'initialize') {
92+
respond(202, {}, '')
93+
return
94+
}
95+
respond(
96+
200,
97+
{ 'content-type': 'application/json', 'mcp-session-id': 'session-1' },
98+
JSON.stringify({
99+
jsonrpc: '2.0',
100+
id: message.id,
101+
result: {
102+
protocolVersion: LATEST_PROTOCOL_VERSION,
103+
capabilities: {},
104+
serverInfo: { name: 'apex', version: '1.0.0' },
105+
},
106+
})
107+
)
108+
})
109+
110+
const client = new McpClient({
111+
config: {
112+
id: 'server-1',
113+
name: 'Apex',
114+
transport: 'streamable-http',
115+
url: `${apex.origin}/mcp`,
116+
headers: { Authorization: 'Bearer configured-secret', 'X-Api-Key': 'configured-key' },
117+
},
118+
})
119+
try {
120+
await client.connect()
121+
await vi.waitFor(() => expect(www.requests).toHaveLength(1), { timeout: 5_000 })
122+
} finally {
123+
await client.disconnect()
124+
}
125+
126+
const apexStream = apex.requests.find((request) => request.method === 'GET')
127+
expect(apexStream?.headers.authorization).toBe('Bearer configured-secret')
128+
const [redirected] = www.requests
129+
expect(redirected.method).toBe('GET')
130+
expect(redirected.url).toBe('/mcp')
131+
expect(redirected.headers.authorization).toBeUndefined()
132+
expect(redirected.headers['x-api-key']).toBeUndefined()
133+
expect(redirected.headers['mcp-session-id']).toBeUndefined()
134+
})
135+
})
136+
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+
})
143+
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+
})
160+
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 })
169+
})
170+
})

‎apps/sim/package.json‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -91,7 +91,7 @@
9191
"@hookform/resolvers": "5.9.1",
9292
"@linear/sdk": "97.1.0",
9393
"@marsidev/react-turnstile": "1.6.2",
94-
"@modelcontextprotocol/sdk": "1.31.0",
94+
"@modelcontextprotocol/sdk": "1.32.1",
9595
"@monaco-editor/react": "4.7.0",
9696
"@napi-rs/canvas": "0.1.100",
9797
"@opentelemetry/api": "^1.9.0",

‎bun.lock‎

Lines changed: 2 additions & 2 deletions
Some generated files are not rendered by default. Learn more about customizing how changed files appear on GitHub.

0 commit comments

Comments
 (0)