fix(mcp): correct stale SSRF pin docstring and bound the remaining OAuth callback steps#5845
fix(mcp): correct stale SSRF pin docstring and bound the remaining OAuth callback steps#5845waleedlatif1 wants to merge 1 commit into
Conversation
…uth callback steps 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.
|
The latest updates on your projects. Learn more about Vercel for GitHub. |
PR SummaryLow Risk Overview SSRF docs on Reviewed by Cursor Bugbot for commit d4eb889. Configure here. |
Greptile SummaryThis PR bounds more OAuth callback operations and updates the SSRF validator documentation. The main changes are:
Confidence Score: 3/5The OAuth timeout paths can report the wrong failure and can return before state cleanup completes.
apps/sim/app/api/mcp/oauth/callback/route.ts and apps/sim/lib/mcp/domain-check.ts
|
| Filename | Overview |
|---|---|
| apps/sim/app/api/mcp/oauth/callback/route.ts | Adds callback timeouts, but misclassifies state lookup failures and can return before state cleanup completes. |
| apps/sim/lib/mcp/domain-check.ts | Updates SSRF documentation, but the policy-signal description does not cover the pinned OAuth probe. |
Reviews (1): Last reviewed commit: "fix(mcp): correct stale SSRF pin docstri..." | Re-trigger Greptile
|
Consolidated into #5842 — the SSRF docstring correction and OAuth callback timedStep bounding are now part of that single PR alongside the push-subscription and lifecycle-audit fixes. Closing to keep everything in one clean PR. |
| const initialRow = state | ||
| ? await timedStep('loadOauthRowByState', 15_000, () => loadOauthRowByState(state)).catch( | ||
| () => null | ||
| ) |
There was a problem hiding this comment.
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.
| await timedStep('clearState(provider_error)', 10_000, () => | ||
| clearState(initialRow.id, 'callback:provider_error') | ||
| ).catch(() => {}) |
There was a problem hiding this comment.
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.
| let serverId: string | undefined | ||
| try { | ||
| const session = await getSession() | ||
| const session = await timedStep('getSession', 15_000, () => getSession()) |
There was a problem hiding this comment.
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.
| * 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 |
There was a problem hiding this comment.
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.
| * 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 |
Summary
Two follow-ups from a full MCP lifecycle audit — both on merged code, both low-risk:
validateMcpServerSsrf's return-value docstring still said it returns an IP to "pin subsequent connections to ... stop redirects from escaping." That's no longer true for the public path (post validate-at-connect): the returned IP is a policy signal selecting the guarded fetch, and 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. Doc-only, no behavior change.timedStep;loadOauthRowByState,getSession, the server SELECT, and theprovider_errorclearStatewere unbounded — contradicting the "every awaited step is bounded" invariant. Now a wedged DB read on those surfaces as a labeled timeout instead of hanging (the client 10-min backstop already prevented a stranded tab; this closes the server-side consistency gap).Type of Change
Testing
Checklist