Skip to content

fix(mcp): make SSRF-guarded fetch structurally mandatory in OAuth auth() calls#5419

Merged
waleedlatif1 merged 1 commit into
stagingfrom
fix/mcp-oauth-guarded-fetch-wrapper
Jul 4, 2026
Merged

fix(mcp): make SSRF-guarded fetch structurally mandatory in OAuth auth() calls#5419
waleedlatif1 merged 1 commit into
stagingfrom
fix/mcp-oauth-guarded-fetch-wrapper

Conversation

@waleedlatif1

Copy link
Copy Markdown
Collaborator

Summary

  • Follow-up to fix(mcp): pass SSRF-guarded fetch into OAuth callback token exchange #5399, from a post-merge /cleanup + /simplify audit of that fix.
  • fix(mcp): pass SSRF-guarded fetch into OAuth callback token exchange #5399 fixed the callback route forgetting to pass fetchFn: createSsrfGuardedMcpFetch() into the MCP SDK's auth(), but every call site of auth() (start and callback routes) still imports the raw SDK function and has to remember to thread the guard through by hand — the exact same class of omission remains possible at any future call site.
  • Adds mcpAuthGuarded() in apps/sim/lib/mcp/oauth/auth.ts: a thin wrapper around the SDK's auth() that always defaults fetchFn to the SSRF-guarded fetch (still overridable, e.g. in tests). Both routes now import mcpAuthGuarded from @/lib/mcp/oauth instead of auth as mcpAuth from the SDK directly, so the guard can no longer be omitted by forgetting a per-call-site argument.
  • apps/sim/lib/mcp/oauth/revoke.ts and probe.ts are untouched — they call different SDK functions (discoverOAuthServerInfo) that already thread fetchFn through consistently and are out of scope for this wrapper.

Test plan

  • New unit test apps/sim/lib/mcp/oauth/auth.test.ts covers the wrapper's default fetchFn and override behavior
  • Updated start/route.test.ts and callback/route.test.ts to assert the routes call mcpAuthGuarded (mocked via the shared @/lib/mcp/oauth barrel mock) instead of mocking the raw SDK auth() + pinned-fetch per test file
  • Added mcpAuthGuarded mock function to the shared mcpOauthMock in packages/testing
  • bunx vitest run apps/sim/app/api/mcp/oauth apps/sim/lib/mcp/oauth — 6 files, 36 tests, all pass
  • Full-repo bun run type-check (tsc --noEmit) — clean, no errors
  • bunx biome check on all changed files — clean

…h() calls

PR #5399 fixed the callback route forgetting to pass fetchFn into the SDK's
auth(), but every call site (start + callback routes) still imported the raw
SDK auth() directly and had to remember to pass fetchFn: createSsrfGuardedMcpFetch()
by hand — the same omission was possible again at any future call site.

Add mcpAuthGuarded() in lib/mcp/oauth/auth.ts, a thin wrapper around the SDK's
auth() that always defaults fetchFn to the SSRF-guarded fetch (still overridable
for tests). Both routes now import mcpAuthGuarded from @/lib/mcp/oauth instead
of the raw SDK auth, so omitting the guard is no longer possible by omission.
@vercel

vercel Bot commented Jul 4, 2026

Copy link
Copy Markdown

The latest updates on your projects. Learn more about Vercel for GitHub.

1 Skipped Deployment
Project Deployment Actions Updated (UTC)
docs Skipped Skipped Jul 4, 2026 10:23pm

Request Review

@cursor

cursor Bot commented Jul 4, 2026

Copy link
Copy Markdown

PR Summary

Low Risk
Narrow security hardening with equivalent intended runtime behavior; main risk is a subtle merge-order bug in fetchFn defaults, covered by new unit tests.

Overview
Introduces mcpAuthGuarded() in apps/sim/lib/mcp/oauth/auth.ts, a thin wrapper around the MCP SDK's auth() that always supplies fetchFn: createSsrfGuardedMcpFetch() (callers can still override for tests). OAuth discovery and token exchange URLs can come from server metadata, so the guard can no longer be skipped by forgetting fetchFn at each call site.

MCP OAuth start and callback routes now call mcpAuthGuarded from @/lib/mcp/oauth instead of importing the SDK auth() and wiring pinned-fetch locally. Route tests assert the wrapper is used; SSRF default/override behavior is covered in auth.test.ts. Shared mcpOauthMock gains mockMcpAuthGuarded.

Reviewed by Cursor Bugbot for commit acb2b20. Configure here.

@greptile-apps

greptile-apps Bot commented Jul 4, 2026

Copy link
Copy Markdown
Contributor

Greptile Summary

This PR introduces mcpAuthGuarded(), a thin wrapper around the MCP SDK's auth() that structurally injects the SSRF-guarded fetch as a default, eliminating the per-call-site risk of forgetting to pass fetchFn. Both OAuth route handlers (start and callback) now import mcpAuthGuarded from the shared @/lib/mcp/oauth barrel instead of calling the raw SDK auth() directly.

  • apps/sim/lib/mcp/oauth/auth.ts (new): Implements mcpAuthGuarded using { fetchFn: createSsrfGuardedMcpFetch(), ...options } spread order so the guard is the default but callers can override (e.g. in tests).
  • start/route.ts and callback/route.ts: Drop the direct SDK auth import and the manual createSsrfGuardedMcpFetch() call; delegate entirely to mcpAuthGuarded.
  • Tests: auth.test.ts covers the wrapper's default and override behaviour; route tests are simplified to assert mcpAuthGuarded is called, with fetchFn coverage delegated to the unit test.

Confidence Score: 5/5

Safe to merge — all auth() call sites now route through the guarded wrapper, and no other file in the codebase calls the raw SDK auth function directly.

The change is a focused structural hardening: a one-line wrapper centralises SSRF-guard injection, both OAuth route handlers are updated, and grep confirms no remaining direct uses of the raw SDK auth() anywhere in the app. Unit tests cover the wrapper's default and override paths, and the route tests verify the new call site. The spread order { fetchFn: guard, ...options } correctly makes the guard the default while allowing test overrides, and there are no logic regressions introduced.

No files require special attention.

Important Files Changed

Filename Overview
apps/sim/lib/mcp/oauth/auth.ts New wrapper; correctly uses spread order { fetchFn: guard, ...options } so the SSRF guard is the default but is overridable for tests. Guard instance is created even when caller supplies a fetchFn override, but this is a trivial cost with no correctness impact.
apps/sim/lib/mcp/oauth/auth.test.ts New unit test covering both the default-fetchFn path and the caller-override path; mocks are set up correctly with vi.hoisted and vi.mock ordering.
apps/sim/lib/mcp/oauth/index.ts Barrel export updated to include mcpAuthGuarded; no other changes.
apps/sim/app/api/mcp/oauth/start/route.ts Removes direct SDK auth import and manual createSsrfGuardedMcpFetch() call; delegates to mcpAuthGuarded — change is minimal and correct.
apps/sim/app/api/mcp/oauth/callback/route.ts Same clean swap as start/route.ts; ReturnType<typeof mcpAuthGuarded> correctly replaces the old type annotation.
apps/sim/app/api/mcp/oauth/start/route.test.ts Simplified by removing SDK/pinned-fetch mocks; now asserts mcpAuthGuarded is called via the shared barrel mock with correct arguments.
apps/sim/app/api/mcp/oauth/callback/route.test.ts Simplified analogously to start route test; comment in test correctly explains the split of coverage responsibility.
packages/testing/src/mocks/mcp-oauth.mock.ts Adds mockMcpAuthGuarded to both mcpOauthMockFns and mcpOauthMock; follows the established pattern of the shared mock file.

Sequence Diagram

%%{init: {'theme': 'neutral'}}%%
sequenceDiagram
    participant Route as start/callback route
    participant Wrapper as mcpAuthGuarded()
    participant Guard as createSsrfGuardedMcpFetch()
    participant SDK as MCP SDK auth()
    participant Remote as OAuth Server

    Route->>Wrapper: "mcpAuthGuarded(provider, { serverUrl, ... })"
    Wrapper->>Guard: createSsrfGuardedMcpFetch()
    Guard-->>Wrapper: guardedFetch
    Wrapper->>SDK: "auth(provider, { fetchFn: guardedFetch, serverUrl, ... })"
    SDK->>Remote: discovery / token requests (via guardedFetch)
    Remote-->>SDK: OAuth response
    SDK-->>Route: "AUTHORIZED | redirect"
Loading
%%{init: {'theme': 'base', 'themeVariables': {"darkMode": true, "background": "#0d1117", "primaryColor": "#21262d", "primaryTextColor": "#e6edf3", "primaryBorderColor": "#8b949e", "lineColor": "#8b949e", "textColor": "#e6edf3", "edgeLabelBackground": "#161b22", "actorBkg": "#21262d", "actorBorder": "#8b949e", "actorTextColor": "#e6edf3", "actorLineColor": "#8b949e", "signalColor": "#8b949e", "signalTextColor": "#e6edf3", "noteBkgColor": "#373320", "noteBorderColor": "#d4a72c", "noteTextColor": "#f0e6c0", "labelBoxBkgColor": "#21262d", "labelBoxBorderColor": "#8b949e", "labelTextColor": "#e6edf3", "loopTextColor": "#e6edf3", "activationBkgColor": "#30363d", "activationBorderColor": "#8b949e"}}}%%
sequenceDiagram
    participant Route as start/callback route
    participant Wrapper as mcpAuthGuarded()
    participant Guard as createSsrfGuardedMcpFetch()
    participant SDK as MCP SDK auth()
    participant Remote as OAuth Server

    Route->>Wrapper: "mcpAuthGuarded(provider, { serverUrl, ... })"
    Wrapper->>Guard: createSsrfGuardedMcpFetch()
    Guard-->>Wrapper: guardedFetch
    Wrapper->>SDK: "auth(provider, { fetchFn: guardedFetch, serverUrl, ... })"
    SDK->>Remote: discovery / token requests (via guardedFetch)
    Remote-->>SDK: OAuth response
    SDK-->>Route: "AUTHORIZED | redirect"
Loading

Reviews (2): Last reviewed commit: "fix(mcp): make SSRF-guarded fetch struct..." | Re-trigger Greptile

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@greptile review

@waleedlatif1

Copy link
Copy Markdown
Collaborator Author

@cursor review

@cursor cursor Bot left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

✅ 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 acb2b20. Configure here.

@waleedlatif1 waleedlatif1 merged commit 4b13398 into staging Jul 4, 2026
18 checks passed
@waleedlatif1 waleedlatif1 deleted the fix/mcp-oauth-guarded-fetch-wrapper branch July 4, 2026 22:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant