Skip to content

Commit 94e6f88

Browse files
committed
fix(mcp): guard OAuth discovery and token revocation against SSRF
Route discoverOAuthServerInfo and the RFC 7009 revocation POST through an SSRF-guarded fetch that validates every request URL via validateMcpServerSsrf (blocking private/reserved/loopback targets, honoring ALLOWED_MCP_DOMAINS and self-hosted localhost rules) and pins the connection to the resolved IP to prevent DNS-rebinding TOCTOU. Previously these fetches used unvalidated global fetch against URLs taken verbatim from attacker-controllable authorization-server metadata.
1 parent 983370e commit 94e6f88

3 files changed

Lines changed: 87 additions & 5 deletions

File tree

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

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -1,11 +1,13 @@
11
import { discoverOAuthServerInfo } from '@modelcontextprotocol/sdk/client/auth.js'
2+
import type { FetchLike } from '@modelcontextprotocol/sdk/shared/transport.js'
23
import { db } from '@sim/db'
34
import { mcpServers } from '@sim/db/schema'
45
import { createLogger } from '@sim/logger'
56
import { toError } from '@sim/utils/errors'
67
import { eq } from 'drizzle-orm'
78
import { decryptSecret } from '@/lib/core/security/encryption'
89
import { loadOauthRow } from '@/lib/mcp/oauth/storage'
10+
import { createSsrfGuardedMcpFetch } from '@/lib/mcp/pinned-fetch'
911

1012
const logger = createLogger('McpOauthRevoke')
1113
const REVOKE_TIMEOUT_MS = 5000
@@ -30,7 +32,10 @@ export async function revokeMcpOauthTokens(mcpServerId: string): Promise<void> {
3032
.limit(1)
3133
if (!server?.url) return
3234

33-
const info = await discoverOAuthServerInfo(server.url).catch(() => undefined)
35+
const ssrfGuardedFetch = createSsrfGuardedMcpFetch()
36+
const info = await discoverOAuthServerInfo(server.url, { fetchFn: ssrfGuardedFetch }).catch(
37+
() => undefined
38+
)
3439
const metadata = info?.authorizationServerMetadata as
3540
| (Record<string, unknown> & { revocation_endpoint?: string })
3641
| undefined
@@ -60,7 +65,7 @@ export async function revokeMcpOauthTokens(mcpServerId: string): Promise<void> {
6065
}
6166

6267
for (const { token, hint } of tokensToRevoke) {
63-
await postRevoke(revocationEndpoint, token, hint, clientId, clientSecret)
68+
await postRevoke(revocationEndpoint, token, hint, clientId, clientSecret, ssrfGuardedFetch)
6469
}
6570
} catch (error) {
6671
logger.warn(`Token revocation failed for server ${mcpServerId}`, {
@@ -74,7 +79,8 @@ async function postRevoke(
7479
token: string,
7580
hint: 'refresh_token' | 'access_token',
7681
clientId: string,
77-
clientSecret: string | undefined
82+
clientSecret: string | undefined,
83+
fetchFn: FetchLike
7884
): Promise<void> {
7985
const controller = new AbortController()
8086
const timer = setTimeout(() => controller.abort(), REVOKE_TIMEOUT_MS)
@@ -89,7 +95,7 @@ async function postRevoke(
8995
} else {
9096
params.set('client_id', clientId)
9197
}
92-
const res = await fetch(endpoint, {
98+
const res = await fetchFn(endpoint, {
9399
method: 'POST',
94100
headers,
95101
body: params.toString(),

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

Lines changed: 54 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -25,12 +25,23 @@ const { mockAgent, mockCreatePinnedLookup, mockUndiciFetch, capturedAgentOptions
2525
}
2626
})
2727

28+
const { mockValidateMcpServerSsrf } = vi.hoisted(() => ({
29+
mockValidateMcpServerSsrf: vi.fn(),
30+
}))
31+
2832
vi.mock('undici', () => ({ Agent: mockAgent, fetch: mockUndiciFetch }))
2933
vi.mock('@/lib/core/security/input-validation.server', () => ({
3034
createPinnedLookup: mockCreatePinnedLookup,
3135
}))
36+
vi.mock('@/lib/mcp/domain-check', () => ({
37+
validateMcpServerSsrf: mockValidateMcpServerSsrf,
38+
}))
3239

33-
import { __resetPinnedAgentsForTests, createMcpPinnedFetch } from '@/lib/mcp/pinned-fetch'
40+
import {
41+
__resetPinnedAgentsForTests,
42+
createMcpPinnedFetch,
43+
createSsrfGuardedMcpFetch,
44+
} from '@/lib/mcp/pinned-fetch'
3445

3546
describe('createMcpPinnedFetch', () => {
3647
beforeEach(() => {
@@ -125,3 +136,45 @@ describe('createMcpPinnedFetch', () => {
125136
expect(mockUndiciFetch).toHaveBeenCalledTimes(1)
126137
})
127138
})
139+
140+
describe('createSsrfGuardedMcpFetch', () => {
141+
beforeEach(() => {
142+
vi.clearAllMocks()
143+
capturedAgentOptions.length = 0
144+
__resetPinnedAgentsForTests()
145+
mockCreatePinnedLookup.mockReturnValue('pinned-lookup-fn')
146+
mockUndiciFetch.mockResolvedValue(new Response('ok'))
147+
})
148+
149+
it('validates each request URL and pins to the resolved IP', async () => {
150+
mockValidateMcpServerSsrf.mockResolvedValue('203.0.113.10')
151+
const fetchLike = createSsrfGuardedMcpFetch()
152+
await fetchLike('https://attacker.example/revoke', { method: 'POST' })
153+
154+
expect(mockValidateMcpServerSsrf).toHaveBeenCalledWith('https://attacker.example/revoke')
155+
expect(mockUndiciFetch).toHaveBeenCalledTimes(1)
156+
const [url, init] = mockUndiciFetch.mock.calls[0]
157+
expect(url).toBe('https://attacker.example/revoke')
158+
expect((init as { dispatcher?: unknown }).dispatcher).toBeInstanceOf(mockAgent)
159+
expect((init as { method?: string }).method).toBe('POST')
160+
})
161+
162+
it('rejects URLs that resolve to blocked IPs without issuing the request', async () => {
163+
mockValidateMcpServerSsrf.mockRejectedValue(new Error('blocked'))
164+
const fetchLike = createSsrfGuardedMcpFetch()
165+
166+
await expect(
167+
fetchLike('http://169.254.169.254/latest/meta-data/', { method: 'POST' })
168+
).rejects.toThrow('blocked')
169+
expect(mockUndiciFetch).not.toHaveBeenCalled()
170+
})
171+
172+
it('accepts URL objects and validates their href', async () => {
173+
mockValidateMcpServerSsrf.mockResolvedValue('203.0.113.10')
174+
const fetchLike = createSsrfGuardedMcpFetch()
175+
await fetchLike(new URL('https://attacker.example/discover'))
176+
177+
expect(mockValidateMcpServerSsrf).toHaveBeenCalledWith('https://attacker.example/discover')
178+
expect(mockUndiciFetch).toHaveBeenCalledTimes(1)
179+
})
180+
})

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

Lines changed: 23 additions & 0 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 { Agent, type RequestInit as UndiciRequestInit, fetch as undiciFetch } from 'undici'
33
import { createPinnedLookup } from '@/lib/core/security/input-validation.server'
4+
import { validateMcpServerSsrf } from '@/lib/mcp/domain-check'
45

56
/**
67
* Pins outbound HTTP connections to a pre-resolved IP to prevent DNS-rebinding
@@ -54,3 +55,25 @@ export function createMcpPinnedFetch(resolvedIP: string): FetchLike {
5455
return response as unknown as Response
5556
}) satisfies FetchLike
5657
}
58+
59+
/**
60+
* Builds a `FetchLike` that validates every outbound request URL against the
61+
* MCP SSRF policy before issuing it, then pins the connection to the resolved
62+
* IP. Unlike the live transport — where the server URL is validated once up
63+
* front — OAuth discovery and RFC 7009 revocation follow URLs taken verbatim
64+
* from attacker-controllable authorization-server metadata
65+
* (`authorization_servers`, `token_endpoint`, `revocation_endpoint`, …). Each
66+
* such hop must be re-validated, so this guard runs `validateMcpServerSsrf`
67+
* per request and rejects private/reserved/loopback targets (honoring
68+
* `ALLOWED_MCP_DOMAINS` and self-hosted localhost rules).
69+
*
70+
* @throws McpSsrfError if a request URL resolves to a blocked IP address
71+
*/
72+
export function createSsrfGuardedMcpFetch(): FetchLike {
73+
return (async (url, init) => {
74+
const target = typeof url === 'string' ? url : url.href
75+
const resolvedIP = await validateMcpServerSsrf(target)
76+
const pinnedFetch: FetchLike = resolvedIP ? createMcpPinnedFetch(resolvedIP) : globalThis.fetch
77+
return pinnedFetch(url, init)
78+
}) satisfies FetchLike
79+
}

0 commit comments

Comments
 (0)