Skip to content

fix(mcp): correct stale SSRF pin docstring and bound the remaining OAuth callback steps #5845

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
waleedlatif1 wants to merge 1 commit into from
+28 −16
Closed

fix(mcp): correct stale SSRF pin docstring and bound the remaining OAuth callback steps #5845

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
25 changes: 17 additions & 8 deletions apps/sim/app/api/mcp/oauth/callback/route.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
)
Comment on lines +128 to +131

@greptile-apps greptile-apps Bot Jul 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 Timeout Becomes Invalid State

When a valid state lookup exceeds 15 seconds or the database rejects it, this catch converts the failure to null. The callback then reports invalid_state, so the client treats a transient infrastructure failure as an expired or forged OAuth state instead of receiving the labeled timeout this change intends to provide.

: 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(() => {})
Comment on lines +138 to +140

@greptile-apps greptile-apps Bot Jul 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P1 security State Cleanup Outlives Callback

When this update exceeds 10 seconds, timedStep rejects without cancelling the database operation and the catch lets the callback return immediately. The OAuth state therefore remains valid until the background update eventually completes, so another callback using the same state can be accepted after the provider-error response has already been sent.

return respond(`Authorization failed: ${errorParam}`, false, 'provider_error', stateRowServerId)
}
if (!state || !code) {
Expand All @@ -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())

@greptile-apps greptile-apps Bot Jul 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 Session Timeout Drops Server Identity

If getSession reaches the new timeout, control jumps to the callback catch before serverId is assigned from the already loaded OAuth row. The failure broadcast therefore contains no server ID, preventing the opener from associating the failed authorization with the server that initiated it.

if (!session?.user?.id) {
return respond(
'You must be signed in to complete authorization.',
Expand All @@ -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)
}
Expand Down
19 changes: 11 additions & 8 deletions apps/sim/lib/mcp/domain-check.ts
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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
Comment on lines +144 to +148

@greptile-apps greptile-apps Bot Jul 22, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

P2 security Public Probe Still Pins Address

The new text says a public result is not pinned, but the OAuth probe passes every non-null resolvedIP, including public addresses, to createPinnedFetchWithDispatcher. The main transport uses the value as a policy signal, while the probe still pins it, so this security guidance remains inaccurate for an existing caller.

Suggested change
* 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
* resolution selects the validate-at-connect guarded fetch for the main transport —
* DNS-rebinding TOCTOU and redirect escapes are prevented by re-validating every
* socket connect and following redirects under per-hop validation (see
* `createSsrfGuardedMcpFetch` / `followRedirectsGuarded`). Some one-shot probes still
* pin this address, as does 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
*/
Expand Down
Loading