Skip to content

Commit 4f3b32a

Browse files
waleedlatif1claude
waleedlatif1
and
claude
committed
fix(mcp): address PR review — shared OAuth popup hook, PATCH symmetry, callback escape
- Extract useMcpOauthPopup hook so the tool-input "add server" flow gets the same postMessage/popup-poll lifecycle as the settings page. - PATCH /mcp/servers/:id now returns hasOauthClientSecret to mirror GET. - Escape '<' / '>' inside the JSON-stringified serverId emitted in the callback's <script> tag for defense-in-depth. Co-Authored-By: Claude Opus 4.7 <noreply@anthropic.com>
1 parent c45e164 commit 4f3b32a

5 files changed

Lines changed: 114 additions & 88 deletions

File tree

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -33,7 +33,9 @@ function escapeHtml(value: string): string {
3333
function htmlClose(message: string, ok: boolean, serverId?: string): NextResponse {
3434
const safeMessage = escapeHtml(message)
3535
const title = ok ? 'Connected' : 'Connection failed'
36-
const serverIdLiteral = serverId ? JSON.stringify(serverId) : 'undefined'
36+
const serverIdLiteral = serverId
37+
? JSON.stringify(serverId).replace(/</g, '\\u003c').replace(/>/g, '\\u003e')
38+
: 'undefined'
3739
const body = `<!doctype html><html><head><meta charset="utf-8"><title>${title}</title></head><body style="font-family: system-ui; padding: 24px"><p>${safeMessage}</p><script>
3840
try { window.opener && window.opener.postMessage({ type: 'mcp-oauth', ok: ${ok ? 'true' : 'false'}, serverId: ${serverIdLiteral} }, window.location.origin) } catch (e) {}
3941
setTimeout(function () { window.close() }, 800)

‎apps/sim/app/api/mcp/servers/[id]/route.ts‎

Lines changed: 4 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -209,8 +209,10 @@ export const PATCH = withRouteHandler(
209209
request,
210210
})
211211

212-
const { oauthClientSecret: _secret, ...safeServer } = updatedServer
213-
return createMcpSuccessResponse({ server: safeServer })
212+
const { oauthClientSecret: _secret, ...rest } = updatedServer
213+
return createMcpSuccessResponse({
214+
server: { ...rest, hasOauthClientSecret: !!_secret },
215+
})
214216
} catch (error) {
215217
logger.error(`[${requestId}] Error updating MCP server:`, error)
216218
return createMcpErrorResponse(toError(error), 'Failed to update MCP server', 500)

‎apps/sim/app/workspace/[workspaceId]/settings/components/mcp/mcp.tsx‎

Lines changed: 5 additions & 82 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,7 @@
11
'use client'
22

3-
import { useEffect, useRef, useState } from 'react'
3+
import { useEffect, useState } from 'react'
44
import { createLogger } from '@sim/logger'
5-
import { toError } from '@sim/utils/errors'
6-
import { useQueryClient } from '@tanstack/react-query'
75
import { ChevronDown, Plus, Search } from 'lucide-react'
86
import { useParams } from 'next/navigation'
97
import {
@@ -15,7 +13,6 @@ import {
1513
ModalFooter,
1614
ModalHeader,
1715
Tooltip,
18-
toast,
1916
} from '@/components/emcn'
2017
import { Input } from '@/components/ui'
2118
import { requestJson } from '@/lib/api/client/request'
@@ -28,18 +25,17 @@ import {
2825
type McpToolIssue,
2926
} from '@/lib/mcp/tool-validation'
3027
import type { McpTransport } from '@/lib/mcp/types'
28+
import { useMcpOauthPopup } from '@/hooks/mcp/use-mcp-oauth-popup'
3129
import {
3230
type McpServer,
3331
type McpTool,
34-
mcpKeys,
3532
useAllowedMcpDomains,
3633
useCreateMcpServer,
3734
useDeleteMcpServer,
3835
useForceRefreshMcpTools,
3936
useMcpServers,
4037
useMcpToolsQuery,
4138
useRefreshMcpServer,
42-
useStartMcpOauth,
4339
useStoredMcpTools,
4440
useUpdateMcpServer,
4541
} from '@/hooks/queries/mcp'
@@ -155,7 +151,6 @@ interface MCPProps {
155151
export function MCP({ initialServerId }: MCPProps) {
156152
const params = useParams()
157153
const workspaceId = params.workspaceId as string
158-
const queryClient = useQueryClient()
159154

160155
const {
161156
data: servers = [],
@@ -175,7 +170,6 @@ export function MCP({ initialServerId }: MCPProps) {
175170
const deleteServerMutation = useDeleteMcpServer()
176171
const refreshServerMutation = useRefreshMcpServer()
177172
const updateServerMutation = useUpdateMcpServer()
178-
const startOauthMutation = useStartMcpOauth()
179173
const availableEnvVars = useAvailableEnvVarKeys(workspaceId)
180174
const { data: allowedMcpDomains = null } = useAllowedMcpDomains()
181175

@@ -184,16 +178,9 @@ export function MCP({ initialServerId }: MCPProps) {
184178

185179
const [searchTerm, setSearchTerm] = useState('')
186180
const [deletingServers, setDeletingServers] = useState<Set<string>>(() => new Set())
187-
const [connectingOauthServers, setConnectingOauthServers] = useState<Set<string>>(() => new Set())
188-
const oauthPopupIntervalsRef = useRef<Map<string, number>>(new Map())
189-
190-
useEffect(() => {
191-
const intervals = oauthPopupIntervalsRef.current
192-
return () => {
193-
for (const id of intervals.values()) window.clearInterval(id)
194-
intervals.clear()
195-
}
196-
}, [])
181+
const { connectingServers: connectingOauthServers, startOauthForServer } = useMcpOauthPopup({
182+
workspaceId,
183+
})
197184

198185
const [serverToDeleteId, setServerToDeleteId] = useState<string | null>(null)
199186
const showDeleteDialog = serverToDeleteId !== null
@@ -209,72 +196,8 @@ export function MCP({ initialServerId }: MCPProps) {
209196
}
210197
}, [])
211198

212-
useEffect(() => {
213-
function onMessage(event: MessageEvent) {
214-
if (event.origin !== window.location.origin) return
215-
const data = event.data as { type?: string; ok?: boolean; serverId?: string } | null
216-
if (data?.type !== 'mcp-oauth') return
217-
if (data.serverId) {
218-
const serverId = data.serverId
219-
const interval = oauthPopupIntervalsRef.current.get(serverId)
220-
if (interval !== undefined) {
221-
window.clearInterval(interval)
222-
oauthPopupIntervalsRef.current.delete(serverId)
223-
}
224-
setConnectingOauthServers((prev) => {
225-
if (!prev.has(serverId)) return prev
226-
const next = new Set(prev)
227-
next.delete(serverId)
228-
return next
229-
})
230-
}
231-
if (data.ok) {
232-
queryClient.invalidateQueries({ queryKey: mcpKeys.serversList(workspaceId) })
233-
queryClient.invalidateQueries({ queryKey: mcpKeys.toolsList(workspaceId) })
234-
queryClient.invalidateQueries({ queryKey: mcpKeys.storedToolsList(workspaceId) })
235-
toast.success('Server authorized')
236-
} else {
237-
toast.error('Authorization failed. Please try again.')
238-
}
239-
}
240-
window.addEventListener('message', onMessage)
241-
return () => window.removeEventListener('message', onMessage)
242-
}, [queryClient, workspaceId])
243-
244199
const [expandedTools, setExpandedTools] = useState<Set<string>>(() => new Set())
245200

246-
const startOauthForServer = async (serverId: string) => {
247-
setConnectingOauthServers((prev) => new Set(prev).add(serverId))
248-
const clear = () => {
249-
const existing = oauthPopupIntervalsRef.current.get(serverId)
250-
if (existing !== undefined) {
251-
window.clearInterval(existing)
252-
oauthPopupIntervalsRef.current.delete(serverId)
253-
}
254-
setConnectingOauthServers((prev) => {
255-
const next = new Set(prev)
256-
next.delete(serverId)
257-
return next
258-
})
259-
}
260-
try {
261-
const result = await startOauthMutation.mutateAsync({ serverId, workspaceId })
262-
if (result.status === 'already_authorized') {
263-
clear()
264-
return
265-
}
266-
const { popup } = result
267-
const interval = window.setInterval(() => {
268-
if (popup.closed) clear()
269-
}, 500)
270-
oauthPopupIntervalsRef.current.set(serverId, interval)
271-
} catch (e) {
272-
clear()
273-
logger.error('Failed to start MCP OAuth', e)
274-
toast.error(toError(e).message || 'Failed to start authorization')
275-
}
276-
}
277-
278201
const handleRemoveServer = (serverId: string) => {
279202
setServerToDeleteId(serverId)
280203
}

‎apps/sim/app/workspace/[workspaceId]/w/[workflowId]/components/panel/components/editor/components/sub-block/components/tool-input/tool-input.tsx‎

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -51,6 +51,7 @@ import type { WandControlHandlers } from '@/app/workspace/[workspaceId]/w/[workf
5151
import { getAllBlocks } from '@/blocks'
5252
import type { SubBlockConfig as BlockSubBlockConfig } from '@/blocks/types'
5353
import { BUILT_IN_TOOL_TYPES } from '@/blocks/utils'
54+
import { useMcpOauthPopup } from '@/hooks/mcp/use-mcp-oauth-popup'
5455
import { useMcpTools } from '@/hooks/mcp/use-mcp-tools'
5556
import { useWorkspaceCredential } from '@/hooks/queries/credentials'
5657
import {
@@ -64,7 +65,6 @@ import {
6465
useForceRefreshMcpTools,
6566
useMcpServers,
6667
useMcpToolsEvents,
67-
useStartMcpOauth,
6868
useStoredMcpTools,
6969
} from '@/hooks/queries/mcp'
7070
import { useWorkflowState, useWorkflows } from '@/hooks/queries/workflows'
@@ -537,7 +537,7 @@ export const ToolInput = memo(function ToolInput({
537537
useMcpToolsEvents(workspaceId)
538538
const { navigateToSettings } = useSettingsNavigation()
539539
const createMcpServer = useCreateMcpServer()
540-
const startMcpOauth = useStartMcpOauth()
540+
const { startOauthForServer } = useMcpOauthPopup({ workspaceId })
541541
const { data: allowedMcpDomains = null } = useAllowedMcpDomains()
542542
const availableEnvVars = useAvailableEnvVarKeys(workspaceId)
543543
const mcpDataLoading = mcpLoading || mcpServersLoading
@@ -2142,7 +2142,7 @@ export const ToolInput = memo(function ToolInput({
21422142
config: { ...config, enabled: true },
21432143
})
21442144
if (result.authType === 'oauth') {
2145-
await startMcpOauth.mutateAsync({ serverId: result.id, workspaceId })
2145+
await startOauthForServer(result.id)
21462146
}
21472147
}}
21482148
workspaceId={workspaceId}

‎apps/sim/hooks/mcp/use-mcp-oauth-popup.ts‎

Lines changed: 99 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,99 @@
1+
'use client'
2+
3+
import { useCallback, useEffect, useRef, useState } from 'react'
4+
import { createLogger } from '@sim/logger'
5+
import { toError } from '@sim/utils/errors'
6+
import { useQueryClient } from '@tanstack/react-query'
7+
import { toast } from '@/components/emcn'
8+
import { mcpKeys, useStartMcpOauth } from '@/hooks/queries/mcp'
9+
10+
const logger = createLogger('useMcpOauthPopup')
11+
12+
interface UseMcpOauthPopupProps {
13+
workspaceId: string
14+
}
15+
16+
export function useMcpOauthPopup({ workspaceId }: UseMcpOauthPopupProps) {
17+
const queryClient = useQueryClient()
18+
const startOauthMutation = useStartMcpOauth()
19+
20+
const [connectingServers, setConnectingServers] = useState<Set<string>>(() => new Set())
21+
const popupIntervalsRef = useRef<Map<string, number>>(new Map())
22+
23+
useEffect(() => {
24+
const intervals = popupIntervalsRef.current
25+
return () => {
26+
for (const id of intervals.values()) window.clearInterval(id)
27+
intervals.clear()
28+
}
29+
}, [])
30+
31+
useEffect(() => {
32+
function onMessage(event: MessageEvent) {
33+
if (event.origin !== window.location.origin) return
34+
const data = event.data as { type?: string; ok?: boolean; serverId?: string } | null
35+
if (data?.type !== 'mcp-oauth') return
36+
if (data.serverId) {
37+
const serverId = data.serverId
38+
const interval = popupIntervalsRef.current.get(serverId)
39+
if (interval !== undefined) {
40+
window.clearInterval(interval)
41+
popupIntervalsRef.current.delete(serverId)
42+
}
43+
setConnectingServers((prev) => {
44+
if (!prev.has(serverId)) return prev
45+
const next = new Set(prev)
46+
next.delete(serverId)
47+
return next
48+
})
49+
}
50+
if (data.ok) {
51+
queryClient.invalidateQueries({ queryKey: mcpKeys.serversList(workspaceId) })
52+
queryClient.invalidateQueries({ queryKey: mcpKeys.toolsList(workspaceId) })
53+
queryClient.invalidateQueries({ queryKey: mcpKeys.storedToolsList(workspaceId) })
54+
toast.success('Server authorized')
55+
} else {
56+
toast.error('Authorization failed. Please try again.')
57+
}
58+
}
59+
window.addEventListener('message', onMessage)
60+
return () => window.removeEventListener('message', onMessage)
61+
}, [queryClient, workspaceId])
62+
63+
const startOauthForServer = useCallback(
64+
async (serverId: string) => {
65+
setConnectingServers((prev) => new Set(prev).add(serverId))
66+
const clear = () => {
67+
const existing = popupIntervalsRef.current.get(serverId)
68+
if (existing !== undefined) {
69+
window.clearInterval(existing)
70+
popupIntervalsRef.current.delete(serverId)
71+
}
72+
setConnectingServers((prev) => {
73+
const next = new Set(prev)
74+
next.delete(serverId)
75+
return next
76+
})
77+
}
78+
try {
79+
const result = await startOauthMutation.mutateAsync({ serverId, workspaceId })
80+
if (result.status === 'already_authorized') {
81+
clear()
82+
return
83+
}
84+
const { popup } = result
85+
const interval = window.setInterval(() => {
86+
if (popup.closed) clear()
87+
}, 500)
88+
popupIntervalsRef.current.set(serverId, interval)
89+
} catch (e) {
90+
clear()
91+
logger.error('Failed to start MCP OAuth', e)
92+
toast.error(toError(e).message || 'Failed to start authorization')
93+
}
94+
},
95+
[startOauthMutation, workspaceId]
96+
)
97+
98+
return { connectingServers, startOauthForServer }
99+
}

0 commit comments

Comments
 (0)