fix(mcp): bound OAuth discovery/DCR/token fetches with a timeout#5776
Conversation
The MCP SDK issues OAuth discovery, dynamic client registration, and token exchange with a bare fetch and no AbortSignal (only the JSON-RPC layer gets the SDK's request timeout), and undici's default headers/body timeouts are 5 min. A slow or unresponsive authorization server therefore left /oauth/start pending for minutes — the browser stuck on "Connecting…" forever. Bound each guarded OAuth/revocation leg with a 30s AbortSignal.timeout, composed with any caller signal so cancellation still works. Only our own deadline is relabeled to an McpError; caller aborts and other failures propagate unchanged. Scoped to createSsrfGuardedMcpFetch (OAuth/revoke/probe) — the live transport's timeouts are untouched.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryMedium Risk Overview
Tests cover signal attachment, timeout and stalled-validation behavior, caller abort vs deadline, and avoiding unhandled rejections when validation races with abort. Reviewed by Cursor Bugbot for commit 1a8be6f. Configure here. |
Greptile SummaryThis PR bounds SSRF-guarded MCP authorization requests so slow servers cannot leave callers waiting for minutes. The main changes are:
Confidence Score: 5/5This looks safe to merge.
Important Files Changed
Reviews (5): Last reviewed commit: "test(mcp): use sleep() instead of raw se..." | Re-trigger Greptile |
Move the timeout signal ahead of validateMcpServerSsrf and race the validation (whose dns.lookup takes no signal) against it, so the deadline covers the whole guarded call — a stalled DNS resolution now rejects at timeoutMs instead of the OS DNS timeout. Listener is cleaned up on settle so a late timeout can't surface as an unhandled rejection.
|
@cursor review |
… by reason Compose the caller's AbortSignal with the deadline up front and use the combined signal for both SSRF validation and the HTTP request, so caller cancellation now covers the whole guarded call (previously a caller abort during a stalled DNS validation waited for the full deadline). Attribute the timeout relabel by the rejection reason's identity rather than init.signal's state, so a caller signal that aborts just after the deadline can't misattribute a genuine timeout.
|
@cursor review |
…d rejection When raceWithSignal is entered with an already-aborted signal it returned without attaching to the in-flight validateMcpServerSsrf promise, so a later SSRF/DNS rejection could surface as an unhandled rejection. Swallow the orphaned promise's settlement in the early-abort branch.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 071d0cc. Configure here.
check:utils bans new Promise(resolve => setTimeout(...)) in favor of sleep() from @sim/utils/helpers.
|
@cursor review |
There was a problem hiding this comment.
✅ Bugbot reviewed your changes and found no new issues!
Comment @cursor review or bugbot run to trigger another review on this PR
Reviewed by Cursor Bugbot for commit 1a8be6f. Configure here.
Summary
app.withgauge.com/mcpand others) can leave/oauth/starthanging for minutes, stranding the UI on "Connecting…". Root cause: the MCP SDK issues OAuth discovery / dynamic client registration / token exchange with a barefetchand noAbortSignal(only the JSON-RPC layer gets the SDK's request timeout), and undici's default headers/body timeouts are 5 minutes. A slow or unresponsive authorization server therefore blocks the request handler — and the browser waiting on it — indefinitely.AbortSignal.timeout, composed with any caller-provided signal viaAbortSignal.anyso cancellation still works. Only our own deadline is relabeled to a clearMcpError; caller aborts and all other failures propagate unchanged.createSsrfGuardedMcpFetch(OAuth discovery, RFC 7009 revocation, auth-type probe). The live MCP transport (createPinnedMcpFetch) is untouched, so tool-execution timeouts are unaffected.Why this design (empirically grounded)
fetchFn. Well-behaved clients (OpenAI Codex) add an explicit per-leg discovery timeout; local clients (Claude Code/Inspector) run OAuth on the user's machine so a human masks the hang. A hosted, server-side client must bound it itself.Backwards compatibility / no regressions
auth.ts,revoke.ts,probe.ts) callcreateSsrfGuardedMcpFetch()with no args → default 30s; behavior for healthy servers (each leg completes in well under a second) is unchanged.AbortSignalis composed, not replaced.DEFAULT_CONNECTION_TIMEOUTprecedent; only pathologically slow (>30s/leg) servers change from "eventually/never" to a fast, actionable error.Type of Change
Testing
Tested manually + unit tests. Added coverage: signal is always attached, deadline surfaces an
McpError, and a caller abort propagates unchanged (composed with the deadline). Fulllib/mcp+app/api/mcpsuite: 429 passing. Lint + typecheck clean.Checklist