Skip to content

Commit c620fdc

Browse files
authored
fix(security): chat attachment XSS, MCP OAuth SSRF guards, Teams clientState verification (#4877)
* fix(chat): prevent XSS in attachment preview via filename/data URL escaping Replace document.write with an escaped blob URL preview: HTML-entity encode the user-controlled filename and data URL, open with noopener,noreferrer, and revoke the blob URL after navigation. * 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. * fix(webhooks): verify Graph clientState on Teams chat-subscription notifications The microsoftteams_chat_subscription trigger set clientState=webhook.id when creating the Graph subscription but never validated it on inbound change notifications, so any request to the webhook path with a crafted notification body was treated as authentic (CWE-345). verifyAuth now requires every notification in the value array to carry a clientState matching the stored webhook id (constant-time compare) and rejects payloads without notifications. Validation handshakes (validationToken) are handled before auth and remain unaffected; outgoing-webhook HMAC auth is unchanged. * fix(webhooks): fail closed when Teams chat-subscription webhook id is unavailable Hardens the clientState check so a missing webhook id (theoretically unreachable, since the row is looked up by primary key) can never collapse the expected value to an empty string that a forged clientState could match. * docs(mcp): note AbortSignal does not bound SSRF-guard DNS lookup * improvement(chat): hoist HTML escape map to module-level constant
1 parent 4076d76 commit c620fdc

6 files changed

Lines changed: 296 additions & 36 deletions

File tree

apps/sim/app/chat/components/message/message.tsx

Lines changed: 45 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -38,6 +38,49 @@ export interface ChatMessage {
3838
files?: ChatFile[]
3939
}
4040

41+
const HTML_ESCAPES: Record<string, string> = {
42+
'&': '&amp;',
43+
'<': '&lt;',
44+
'>': '&gt;',
45+
'"': '&quot;',
46+
"'": '&#39;',
47+
} as const
48+
49+
/**
50+
* Escapes HTML entities so untrusted strings are safe to interpolate into markup.
51+
*/
52+
function escapeHtml(value: string): string {
53+
return value.replace(/[&<>"']/g, (c) => HTML_ESCAPES[c] || c)
54+
}
55+
56+
/**
57+
* Opens an image attachment preview in a new tab via a blob URL,
58+
* escaping the user-controlled filename and data URL to prevent XSS.
59+
*/
60+
function openAttachmentPreview(name: string, dataUrl: string): void {
61+
const safeName = escapeHtml(name)
62+
const safeUrl = escapeHtml(dataUrl)
63+
const html = `
64+
<!DOCTYPE html>
65+
<html>
66+
<head>
67+
<title>${safeName}</title>
68+
<style>
69+
body { margin: 0; display: flex; justify-content: center; align-items: center; min-height: 100vh; background: #000; }
70+
img { max-width: 100%; max-height: 100vh; object-fit: contain; }
71+
</style>
72+
</head>
73+
<body>
74+
<img src="${safeUrl}" alt="${safeName}" />
75+
</body>
76+
</html>
77+
`
78+
const blob = new Blob([html], { type: 'text/html' })
79+
const blobUrl = URL.createObjectURL(blob)
80+
window.open(blobUrl, '_blank', 'noopener,noreferrer')
81+
setTimeout(() => URL.revokeObjectURL(blobUrl), 60_000)
82+
}
83+
4184
export const ClientChatMessage = memo(
4285
function ClientChatMessage({ message }: { message: ChatMessage }) {
4386
const [isCopied, setIsCopied] = useState(false)
@@ -103,43 +146,15 @@ export const ClientChatMessage = memo(
103146
if (validDataUrl?.startsWith('data:')) {
104147
e.preventDefault()
105148
e.stopPropagation()
106-
const newWindow = window.open('', '_blank')
107-
if (newWindow) {
108-
newWindow.document.write(`
109-
<!DOCTYPE html>
110-
<html>
111-
<head>
112-
<title>${attachment.name}</title>
113-
<style>
114-
body { margin: 0; display: flex; justify-content: center; align-items: center; min-height: 100vh; background: #000; }
115-
img { max-width: 100%; max-height: 100vh; object-fit: contain; }
116-
</style>
117-
</head>
118-
<body>
119-
<img src="${validDataUrl}" alt="${attachment.name}" />
120-
</body>
121-
</html>
122-
`)
123-
newWindow.document.close()
124-
}
149+
openAttachmentPreview(attachment.name, validDataUrl)
125150
}
126151
}}
127152
onKeyDown={(event) => {
128153
const validDataUrl = attachment.dataUrl?.trim()
129154
if (!validDataUrl?.startsWith('data:')) return
130155
if (event.key === 'Enter' || event.key === ' ') {
131156
event.preventDefault()
132-
const newWindow = window.open('', '_blank')
133-
if (newWindow) {
134-
newWindow.document.write(`
135-
<html>
136-
<head><title>${attachment.name}</title></head>
137-
<body style="margin:0;display:flex;align-items:center;justify-content:center;min-height:100vh;background:#111;">
138-
<img src="${validDataUrl}" alt="${attachment.name}" style="max-width:100%;max-height:100vh;object-fit:contain;" />
139-
</body>
140-
</html>
141-
`)
142-
}
157+
openAttachmentPreview(attachment.name, validDataUrl)
143158
}
144159
}}
145160
>

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: 29 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,31 @@ 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+
* Note: a caller-provided `AbortSignal` in `init` only bounds the HTTP request,
71+
* not the validation DNS lookup — Node's `dns.lookup` does not accept a signal,
72+
* so a hanging resolution can extend the overall call past the caller's timeout
73+
* by up to the OS DNS timeout. Acceptable here because all consumers are
74+
* best-effort, non-blocking flows (OAuth discovery and RFC 7009 revocation).
75+
*
76+
* @throws McpSsrfError if a request URL resolves to a blocked IP address
77+
*/
78+
export function createSsrfGuardedMcpFetch(): FetchLike {
79+
return (async (url, init) => {
80+
const target = typeof url === 'string' ? url : url.href
81+
const resolvedIP = await validateMcpServerSsrf(target)
82+
const pinnedFetch: FetchLike = resolvedIP ? createMcpPinnedFetch(resolvedIP) : globalThis.fetch
83+
return pinnedFetch(url, init)
84+
}) satisfies FetchLike
85+
}
Lines changed: 120 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,120 @@
1+
/**
2+
* @vitest-environment node
3+
*/
4+
import { authOAuthUtilsMock, inputValidationMock } from '@sim/testing'
5+
import { NextRequest } from 'next/server'
6+
import { beforeEach, describe, expect, it, vi } from 'vitest'
7+
8+
vi.mock('@/lib/core/security/input-validation.server', () => inputValidationMock)
9+
vi.mock('@/app/api/auth/oauth/utils', () => authOAuthUtilsMock)
10+
11+
import { microsoftTeamsHandler } from '@/lib/webhooks/providers/microsoft-teams'
12+
13+
const WEBHOOK_ID = 'webhook-uuid-1234'
14+
15+
function makeRequest(body: string): NextRequest {
16+
return new NextRequest('https://app.example.com/api/webhooks/trigger/abc', {
17+
method: 'POST',
18+
body,
19+
headers: { 'Content-Type': 'application/json' },
20+
})
21+
}
22+
23+
function makeNotificationBody(clientState?: unknown): string {
24+
return JSON.stringify({
25+
value: [
26+
{
27+
subscriptionId: 'sub-1',
28+
changeType: 'created',
29+
resource: 'chats/19:abc@thread.v2/messages/1700000000000',
30+
resourceData: { id: '1700000000000' },
31+
...(clientState !== undefined ? { clientState } : {}),
32+
},
33+
],
34+
})
35+
}
36+
37+
async function runVerifyAuth(rawBody: string, providerConfig: Record<string, unknown>) {
38+
return microsoftTeamsHandler.verifyAuth!({
39+
webhook: { id: WEBHOOK_ID },
40+
workflow: {},
41+
request: makeRequest(rawBody),
42+
rawBody,
43+
requestId: 'test-req',
44+
providerConfig,
45+
})
46+
}
47+
48+
describe('microsoftTeamsHandler verifyAuth (chat subscription clientState)', () => {
49+
const chatSubscriptionConfig = { triggerId: 'microsoftteams_chat_subscription' }
50+
51+
beforeEach(() => {
52+
vi.clearAllMocks()
53+
})
54+
55+
it('accepts notifications whose clientState matches the webhook id', async () => {
56+
const res = await runVerifyAuth(makeNotificationBody(WEBHOOK_ID), chatSubscriptionConfig)
57+
expect(res).toBeNull()
58+
})
59+
60+
it('rejects notifications with a forged clientState', async () => {
61+
const res = await runVerifyAuth(makeNotificationBody('forged'), chatSubscriptionConfig)
62+
expect(res?.status).toBe(401)
63+
})
64+
65+
it('rejects notifications missing clientState', async () => {
66+
const res = await runVerifyAuth(makeNotificationBody(), chatSubscriptionConfig)
67+
expect(res?.status).toBe(401)
68+
})
69+
70+
it('rejects non-string clientState values', async () => {
71+
const res = await runVerifyAuth(
72+
makeNotificationBody({ nested: WEBHOOK_ID }),
73+
chatSubscriptionConfig
74+
)
75+
expect(res?.status).toBe(401)
76+
})
77+
78+
it('rejects payloads without a value array', async () => {
79+
const res = await runVerifyAuth(JSON.stringify({ hello: 'world' }), chatSubscriptionConfig)
80+
expect(res?.status).toBe(401)
81+
})
82+
83+
it('rejects payloads with an empty value array', async () => {
84+
const res = await runVerifyAuth(JSON.stringify({ value: [] }), chatSubscriptionConfig)
85+
expect(res?.status).toBe(401)
86+
})
87+
88+
it('rejects unparseable bodies', async () => {
89+
const res = await runVerifyAuth('not-json', chatSubscriptionConfig)
90+
expect(res?.status).toBe(401)
91+
})
92+
93+
it('rejects batches where any notification has a mismatched clientState', async () => {
94+
const rawBody = JSON.stringify({
95+
value: [
96+
{ subscriptionId: 'sub-1', resourceData: { id: '1' }, clientState: WEBHOOK_ID },
97+
{ subscriptionId: 'sub-2', resourceData: { id: '2' }, clientState: 'forged' },
98+
],
99+
})
100+
const res = await runVerifyAuth(rawBody, chatSubscriptionConfig)
101+
expect(res?.status).toBe(401)
102+
})
103+
104+
it('fails closed when the webhook record has no id', async () => {
105+
const res = await microsoftTeamsHandler.verifyAuth!({
106+
webhook: {},
107+
workflow: {},
108+
request: makeRequest(makeNotificationBody('')),
109+
rawBody: makeNotificationBody(''),
110+
requestId: 'test-req',
111+
providerConfig: chatSubscriptionConfig,
112+
})
113+
expect(res?.status).toBe(401)
114+
})
115+
116+
it('does not require clientState for non-subscription trigger types', async () => {
117+
const res = await runVerifyAuth(JSON.stringify({ type: 'message', text: 'hi' }), {})
118+
expect(res).toBeNull()
119+
})
120+
})

0 commit comments

Comments
 (0)