From d4eb8894fcb0b1ff0d23af54ab6c98c24b592130 Mon Sep 17 00:00:00 2001
From: Waleed Latif <walif6@gmail.com>
Date: Wed, 22 Jul 2026 00:00:01 -0700
Subject: [PATCH] fix(mcp): correct stale SSRF pin docstring and bound the
 remaining OAuth callback steps
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Lifecycle-audit follow-ups (merged-code):
- validateMcpServerSsrf's return docstring described 'pin subsequent connections',
  which no longer holds for the public path — the returned IP is a policy signal
  selecting the validate-at-connect guarded fetch; redirect/rebind safety comes
  from per-connect validation + followRedirectsGuarded, not pinning (only the
  self-hosted private carve-out still literally pins). Corrected so a future
  reader can't reintroduce a real pin from the misleading text.
- The callback wrapped only the five post-auth steps in timedStep; the earlier
  loadOauthRowByState, getSession, and server SELECT (plus the provider_error
  clearState) were unbounded, contradicting the 'every awaited step bounded'
  invariant. Wrapped them so a wedged DB read surfaces as a labeled timeout.
---
 apps/sim/app/api/mcp/oauth/callback/route.ts | 25 +++++++++++++-------
 apps/sim/lib/mcp/domain-check.ts             | 19 ++++++++-------
 2 files changed, 28 insertions(+), 16 deletions(-)

diff --git a/apps/sim/app/api/mcp/oauth/callback/route.ts b/apps/sim/app/api/mcp/oauth/callback/route.ts
index 7694f23b010..79b7058ddea 100644
--- a/apps/sim/app/api/mcp/oauth/callback/route.ts
+++ b/apps/sim/app/api/mcp/oauth/callback/route.ts
@@ -125,12 +125,19 @@ export const GET = withRouteHandler(async (request: NextRequest) => {
     serverId?: string
   ) => htmlClose(message, ok, reason, serverId, state)
 
-  const initialRow = state ? await loadOauthRowByState(state).catch(() => null) : null
+  const initialRow = state
+    ? await timedStep('loadOauthRowByState', 15_000, () => loadOauthRowByState(state)).catch(
+        () => null
+      )
+    : null
   const stateRowServerId = initialRow?.mcpServerId
 
   if (errorParam) {
     logger.warn(`MCP OAuth callback received error: ${errorParam}`)
-    if (initialRow) await clearState(initialRow.id, 'callback:provider_error').catch(() => {})
+    if (initialRow)
+      await timedStep('clearState(provider_error)', 10_000, () =>
+        clearState(initialRow.id, 'callback:provider_error')
+      ).catch(() => {})
     return respond(`Authorization failed: ${errorParam}`, false, 'provider_error', stateRowServerId)
   }
   if (!state || !code) {
@@ -144,7 +151,7 @@ export const GET = withRouteHandler(async (request: NextRequest) => {
 
   let serverId: string | undefined
   try {
-    const session = await getSession()
+    const session = await timedStep('getSession', 15_000, () => getSession())
     if (!session?.user?.id) {
       return respond(
         'You must be signed in to complete authorization.',
@@ -169,11 +176,13 @@ export const GET = withRouteHandler(async (request: NextRequest) => {
       )
     }
 
-    const [server] = await db
-      .select({ id: mcpServers.id, url: mcpServers.url, workspaceId: mcpServers.workspaceId })
-      .from(mcpServers)
-      .where(and(eq(mcpServers.id, row.mcpServerId), isNull(mcpServers.deletedAt)))
-      .limit(1)
+    const [server] = await timedStep('loadServer', 15_000, () =>
+      db
+        .select({ id: mcpServers.id, url: mcpServers.url, workspaceId: mcpServers.workspaceId })
+        .from(mcpServers)
+        .where(and(eq(mcpServers.id, row.mcpServerId), isNull(mcpServers.deletedAt)))
+        .limit(1)
+    )
     if (!server || !server.url) {
       return respond('Server no longer exists.', false, 'server_gone', serverId)
     }
diff --git a/apps/sim/lib/mcp/domain-check.ts b/apps/sim/lib/mcp/domain-check.ts
index e04b6f04cc3..1c4c40e3ac1 100644
--- a/apps/sim/lib/mcp/domain-check.ts
+++ b/apps/sim/lib/mcp/domain-check.ts
@@ -139,14 +139,17 @@ function isLocalhostHostname(hostname: string): boolean {
  * URLs with env var references in the hostname are skipped — they will be
  * validated after resolution at execution time.
  *
- * Returns the IP address to pin subsequent connections to (the resolved IP for
- * hostnames, or the literal itself for public IP-literal URLs) so the caller can
- * prevent DNS-rebinding TOCTOU attacks and stop redirects from escaping to
- * internal hosts. Pinning matters for IP literals too: without it the transport
- * uses the default fetch, which follows an attacker-controlled 3xx redirect to a
- * private/metadata address. Returns null only when pinning is unnecessary or
- * impossible: no URL, allowlist-only mode, env-var hostnames (validated later),
- * and localhost on self-hosted (no rebinding risk against a fixed loopback).
+ * Returns the resolved IP (or the literal itself for IP-literal URLs) as a
+ * non-null **policy signal**: the SSRF guard is active for this server. A public
+ * resolution selects the validate-at-connect guarded fetch — DNS-rebinding TOCTOU
+ * and redirect escapes are prevented by re-validating every socket connect and
+ * following redirects under per-hop validation (see `createSsrfGuardedMcpFetch` /
+ * `followRedirectsGuarded`), NOT by pinning to this address. The value is literally
+ * pinned only for the self-hosted private/loopback carve-out (a policy-permitted
+ * DNS alias the guarded lookup would otherwise filter). Returns null when the guard
+ * is unnecessary or impossible: no URL, allowlist-only mode, env-var hostnames
+ * (validated later), and localhost on self-hosted (no rebinding risk against a
+ * fixed loopback).
  *
  * @throws McpSsrfError if the URL resolves to a blocked IP address
  */