Skip to content

Commit 74444ab

Browse files
committed
fix(mcp): bound OAuth callback steps and log each phase to pinpoint hangs
The MCP OAuth callback could hang indefinitely after burning state, with no error, no timeout, and no log — the request stalls somewhere between the burn and token persistence with no observable I/O. Wrap each awaited callback step (loadPreregisteredClient, mcpAuthGuarded, clearVerifier, discoverServerTools) with a per-step timeout so a stalled operation surfaces as a labeled error instead of hanging forever, and log start/done for every step, every guarded OAuth fetch phase (validate/request/read-body), and the token-exchange DB writes. The last start without a matching done names the exact stall point.
1 parent e01cd2c commit 74444ab

3 files changed

Lines changed: 87 additions & 11 deletions

File tree

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

Lines changed: 62 additions & 9 deletions
Original file line numberDiff line numberDiff line change
@@ -25,6 +25,46 @@ const logger = createLogger('McpOauthCallbackAPI')
2525

2626
export const dynamic = 'force-dynamic'
2727

28+
class OauthCallbackStepTimeout extends Error {
29+
constructor(step: string, ms: number) {
30+
super(`MCP OAuth callback step "${step}" did not settle within ${ms}ms`)
31+
this.name = 'OauthCallbackStepTimeout'
32+
}
33+
}
34+
35+
/**
36+
* Times and bounds one awaited step of the callback so a stalled operation
37+
* surfaces as a labeled, logged error instead of hanging the request forever.
38+
* The losing promise is not cancelled (a wedged DB/socket op can't be), so it
39+
* settles in the background with its rejection swallowed; the point is that the
40+
* request stops waiting on it and the logs name the exact step that stalled.
41+
*/
42+
async function timedStep<T>(step: string, ms: number, fn: () => Promise<T>): Promise<T> {
43+
const start = Date.now()
44+
logger.info(`OAuth callback step start: ${step}`)
45+
const work = fn()
46+
work.catch(() => {})
47+
let timer: ReturnType<typeof setTimeout> | undefined
48+
try {
49+
const value = await Promise.race([
50+
work,
51+
new Promise<never>((_, reject) => {
52+
timer = setTimeout(() => reject(new OauthCallbackStepTimeout(step, ms)), ms)
53+
timer.unref?.()
54+
}),
55+
])
56+
logger.info(`OAuth callback step done: ${step} (${Date.now() - start}ms)`)
57+
return value
58+
} catch (error) {
59+
logger.error(`OAuth callback step failed: ${step} (${Date.now() - start}ms)`, {
60+
error: toError(error).message,
61+
})
62+
throw error
63+
} finally {
64+
clearTimeout(timer)
65+
}
66+
}
67+
2868
function escapeHtml(value: string): string {
2969
return value
3070
.replace(/&/g, '&amp;')
@@ -145,8 +185,9 @@ export const GET = withRouteHandler(async (request: NextRequest) => {
145185
serverId
146186
)
147187
}
188+
const serverUrl = server.url
148189
try {
149-
assertSafeOauthServerUrl(server.url)
190+
assertSafeOauthServerUrl(serverUrl)
150191
} catch {
151192
return respond(
152193
'MCP OAuth requires https (or http://localhost for development).',
@@ -157,16 +198,22 @@ export const GET = withRouteHandler(async (request: NextRequest) => {
157198
}
158199

159200
// Burn state before token exchange so a replayed callback cannot reuse it.
160-
await clearState(row.id, 'callback:burn-before-exchange')
201+
await timedStep('clearState(burn)', 10_000, () =>
202+
clearState(row.id, 'callback:burn-before-exchange')
203+
)
161204

162-
const preregistered = await loadPreregisteredClient(server.id)
205+
const preregistered = await timedStep('loadPreregisteredClient', 15_000, () =>
206+
loadPreregisteredClient(server.id)
207+
)
163208
const provider = new SimMcpOauthProvider({ row, preregistered })
164209
let result: Awaited<ReturnType<typeof mcpAuthGuarded>>
165210
try {
166-
result = await mcpAuthGuarded(provider, {
167-
serverUrl: server.url,
168-
authorizationCode: code,
169-
})
211+
result = await timedStep('mcpAuthGuarded', 60_000, () =>
212+
mcpAuthGuarded(provider, {
213+
serverUrl,
214+
authorizationCode: code,
215+
})
216+
)
170217
} catch (e) {
171218
logger.error('Token exchange failed during MCP OAuth callback', e)
172219
return respond(
@@ -176,7 +223,11 @@ export const GET = withRouteHandler(async (request: NextRequest) => {
176223
server.id
177224
)
178225
} finally {
179-
await clearVerifier(row.id)
226+
await timedStep('clearVerifier', 10_000, () => clearVerifier(row.id)).catch((e) =>
227+
logger.error('Failed to clear PKCE verifier after MCP OAuth callback', {
228+
error: toError(e).message,
229+
})
230+
)
180231
}
181232

182233
if (result !== 'AUTHORIZED') {
@@ -185,7 +236,9 @@ export const GET = withRouteHandler(async (request: NextRequest) => {
185236

186237
try {
187238
// forceRefresh: skip any stale cache from before re-auth.
188-
await mcpService.discoverServerTools(session.user.id, server.id, server.workspaceId, true)
239+
await timedStep('discoverServerTools', 30_000, () =>
240+
mcpService.discoverServerTools(session.user.id, server.id, server.workspaceId, true)
241+
)
189242
} catch (e) {
190243
logger.warn('Post-auth tools refresh failed', toError(e).message)
191244
}

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

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -170,18 +170,22 @@ export async function saveClientInformation(
170170
info: OAuthClientInformationMixed
171171
): Promise<void> {
172172
const encrypted = await encryptClientInformation(info)
173+
logger.info('Persisting MCP OAuth client information', { rowId })
173174
await db
174175
.update(mcpServerOauth)
175176
.set({ clientInformation: encrypted, updatedAt: new Date() })
176177
.where(eq(mcpServerOauth.id, rowId))
178+
logger.info('Persisted MCP OAuth client information', { rowId })
177179
}
178180

179181
export async function saveTokens(rowId: string, tokens: OAuthTokens): Promise<void> {
180182
const encrypted = await encryptTokens(tokens)
183+
logger.info('Persisting MCP OAuth tokens', { rowId })
181184
await db
182185
.update(mcpServerOauth)
183186
.set({ tokens: encrypted, lastRefreshedAt: new Date(), updatedAt: new Date() })
184187
.where(eq(mcpServerOauth.id, rowId))
188+
logger.info('Persisted MCP OAuth tokens', { rowId })
185189
}
186190

187191
export async function saveCodeVerifier(rowId: string, verifier: string): Promise<void> {

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

Lines changed: 21 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -1,9 +1,12 @@
11
import type { FetchLike } from '@modelcontextprotocol/sdk/shared/transport.js'
2+
import { createLogger } from '@sim/logger'
23
import type { Agent } from 'undici'
34
import { createPinnedFetchWithDispatcher } from '@/lib/core/security/input-validation.server'
45
import { validateMcpServerSsrf } from '@/lib/mcp/domain-check'
56
import { McpError } from '@/lib/mcp/types'
67

8+
const logger = createLogger('McpOauthFetch')
9+
710
/** Pinned fetch for the live MCP transport, plus a handle to release its sockets. */
811
export interface PinnedMcpFetch {
912
fetch: typeof fetch
@@ -151,13 +154,17 @@ function releaseStreamOnSettle(
151154
export function createSsrfGuardedMcpFetch(timeoutMs: number = OAUTH_FETCH_TIMEOUT_MS): FetchLike {
152155
return (async (url, init) => {
153156
const target = typeof url === 'string' ? url : url.href
157+
const host = URL.canParse(target) ? new URL(target).host : target
158+
const startedAt = Date.now()
154159
const timeoutSignal = AbortSignal.timeout(timeoutMs)
155160
// Bound every phase — validation, request, body read — by the deadline + caller signal.
156161
const signal = init?.signal ? AbortSignal.any([init.signal, timeoutSignal]) : timeoutSignal
157162
// Per-request Agent must be torn down (finally): a one-shot leg never reuses its socket.
158163
let dispatcher: Agent | undefined
159164
try {
165+
logger.info('OAuth guarded fetch: validating', { host })
160166
const resolvedIP = await withDeadline(validateMcpServerSsrf(target), signal)
167+
logger.info('OAuth guarded fetch: requesting', { host, resolvedIP: resolvedIP ?? 'unpinned' })
161168
let response: Response
162169
if (resolvedIP) {
163170
const pinned = createPinnedFetchWithDispatcher(resolvedIP, {
@@ -175,11 +182,23 @@ export function createSsrfGuardedMcpFetch(timeoutMs: number = OAUTH_FETCH_TIMEOU
175182
if (contentType.includes('text/event-stream')) {
176183
const streamed = releaseStreamOnSettle(response, dispatcher, signal)
177184
dispatcher = undefined // teardown ownership moved to releaseStreamOnSettle
185+
logger.info('OAuth guarded fetch: streaming response', {
186+
host,
187+
status: response.status,
188+
ms: Date.now() - startedAt,
189+
})
178190
return streamed
179191
}
180-
return await bufferUnderDeadline(response, signal)
192+
logger.info('OAuth guarded fetch: reading body', { host, status: response.status })
193+
const buffered = await bufferUnderDeadline(response, signal)
194+
logger.info('OAuth guarded fetch: done', {
195+
host,
196+
status: response.status,
197+
ms: Date.now() - startedAt,
198+
})
199+
return buffered
181200
} catch (error) {
182-
const host = URL.canParse(target) ? new URL(target).host : target
201+
logger.warn('OAuth guarded fetch: failed', { host, ms: Date.now() - startedAt })
183202
// Relabel only our own deadline — by reason identity, not signal state (which may
184203
// abort independently just after the deadline).
185204
if (timeoutSignal.aborted && error === timeoutSignal.reason) {

0 commit comments

Comments
 (0)