feat: support OAuth 2.1 for streamable-http MCP servers#11874
feat: support OAuth 2.1 for streamable-http MCP servers#11874edelauna wants to merge 15 commits intoRooCodeInc:mainfrom
Conversation
|
Adding Screenshares of the sign-in and usage process. MCP OAuth Flow: oauth-connect-example-compressed.mp4MCP Usage: mcp-usage-example.mp4 |
4c87637 to
653b71e
Compare
|
@roomote is this going to break anything? |
I reviewed the PR for breaking changes. All 104 tests pass (57 new OAuth + 47 existing McpHub) and TypeScript compiles cleanly. Will it break existing functionality? No. The OAuth code path is only entered for Things worth being aware of (not breaking):
|
hannesrudolph
left a comment
There was a problem hiding this comment.
Architecturally sound PR. The OAuth 2.1 implementation correctly handles RFC 9728 + RFC 8414 discovery, works around the SDK's broken well-known URL construction for path-prefixed issuers, and the non-blocking design (detaching the browser flow from waitUntilReady()) is well done. Token lifecycle, multi-window coordination via SecretStorage.onDidChange, and the reconnect-from-scratch strategy (required because the SDK's transport can't be reused after UnauthorizedError) are all correct. Test coverage is thorough (42 unit + 2 e2e). One P1 UX concern on mid-session re-auth and four P2 items inline.
| // transparently; the browser is only opened when a 401 forces a new | ||
| // authorization flow. | ||
| const authProvider = await McpOAuthClientProvider.create(configInjected.url, this.secretStorage, name) | ||
|
|
There was a problem hiding this comment.
P2: McpOAuthClientProvider.create() calls fetchOAuthAuthServerMetadata() which does a network request to /.well-known/oauth-protected-resource for every streamable-http server, even ones that don't use OAuth. For non-OAuth servers this 404s gracefully, but it adds a round-trip to every connection setup. Consider deferring discovery until after the first UnauthorizedError, or at minimum adding a short cache/negative-cache so repeated reconnects to the same non-OAuth server skip the probe.
There was a problem hiding this comment.
This is turning out to be a bit larger of a lift - how critical would this be to include it as part of this PR? For now I've just implemented an inmemory cache.
|
@roomote do a full review please and leave inline comments. |
Completed full review with 8 inline comments and pushed security hardening fixes. All 57 unit tests pass, lint and type checks clean. |
There was a problem hiding this comment.
Full Review: OAuth 2.1 for streamable-http MCP servers
Overall this is a solid implementation. The SDK workarounds for broken RFC 8414 discovery are well-documented, the non-blocking OAuth flow design is thoughtful, and test coverage is comprehensive (57 tests across 4 files).
I've pushed a commit with security hardening fixes (see inline comments for details). The remaining comments below are observations and suggestions for the PR author to consider.
What works well
- Clean separation of concerns:
McpOAuthClientProvider,SecretStorageService,callbackServer,oauth.ts - Lazy callback server startup avoids binding ports unnecessarily
- Token refresh with deduplication (
_refreshPromise) prevents thundering herd - Multi-window awareness via
SecretStorage.onDidChangewatcher is a nice touch - The
_completeOAuthFlowfire-and-forget pattern keeps the extension responsive
Items addressed in my commit (bde2bc3)
- CSRF state token increased from 8 to 16 bytes (128-bit entropy per OAuth 2.1)
- Content-Security-Policy header added to callback HTML response
- Callback server now closes on CSRF state validation failure
stopCallbackServerhandles already-closed servers gracefullyMCP_OAUTH_TEST_MODEguard tightened withVSCODE_PIDcheck
Additional observations (not blocking)
See inline comments.
| // Step 2 – RFC 8414 §3.1: build the well-known URL. | ||
| // For issuer "https://example.com/auth/public" | ||
| // → "https://example.com/.well-known/oauth-authorization-server/auth/public" | ||
| const parsed = new URL(authServers[0]) |
There was a problem hiding this comment.
Observation: This only tries the first authorization server from the authorization_servers array. Per RFC 9728, the protected resource may advertise multiple authorization servers, and the client should be able to select among them. If the first one is down or misconfigured, the flow fails entirely. Consider adding fallback logic to try subsequent servers, or at minimum logging which server was selected.
3d0eb27 to
11798c2
Compare
11798c2 to
5aeed4f
Compare
Related GitHub Issue
Closes: #8119
Description
Implements OAuth 2.1 support for streamable-http MCP servers per RFC 9728 (Protected Resource Metadata), RFC 8414 (Authorization Server Metadata), RFC 7591 (Dynamic Client Registration), and RFC 8707 (Resource Indicators).
Key design decisions:
Non-blocking OAuth: When
client.connect()throwsUnauthorizedError, the OAuth browser flow runs in the background via_completeOAuthFlow()so the extension (chat window, other servers) isn't blocked waiting for the user's browser session.SDK workarounds: The MCP SDK's
discoverOAuthMetadata()constructs the RFC 8414 well-known URL incorrectly for auth servers with path components (e.g.,https://example.com/auth/public). It usesnew URL("/.well-known/oauth-authorization-server", issuer)which is origin-relative and discards the path. This causes cascading failures: metadata discovery fails → dynamic client registration uses a wrong fallback URL → 404. See upstream issues:We work around this by:
utils/oauth.ts)registration_endpointredirectToAuthorization()exchangeCodeForTokens()using the correcttoken_endpointToken persistence: Tokens are stored in VS Code's
SecretStorageviaSecretStorageService, keyed by server host. Cached tokens are reused on reconnect without re-running the full OAuth flow.connectToServer()is called fresh — the SDK's transport can't be reused afterUnauthorizedErrorbecause its internal_abortControlleris already set.New files:
McpOAuthClientProvider.ts— Implements the SDK'sOAuthClientProviderinterfaceSecretStorageService.ts— Thin wrapper around VS Code SecretStorage for OAuth tokensutils/oauth.ts— RFC 8414-compliant metadata discovery (replaces SDK's broken implementation)utils/callbackServer.ts— Local HTTP server for OAuth redirect callback with CSRF state validationTest Procedure
Unit tests (42 tests across 4 files):
npx vitest run services/mcp/tests/McpOAuthClientProvider.spec.ts services/mcp/tests/SecretStorageService.spec.ts services/mcp/utils/tests/oauth.spec.ts services/mcp/utils/tests/callbackServer.spec.ts
McpOAuthClientProvider.spec.ts— 25 tests covering create, clientMetadata, token storage/expiry, PKCE, redirect, auth code, closeSecretStorageService.spec.ts— 7 tests covering CRUD, key isolation, malformed dataoauth.spec.ts— 8 tests covering RFC 8414 URL construction for path/no-path issuers, error handlingcallbackServer.spec.ts— 2 tests covering callback handling and CSRF state validationE2e test (
apps/vscode-e2e/src/suite/mcp-oauth.test.ts):Manual testing:
Pre-Submission Checklist
Documentation Updates
Additional Notes
MCP_OAUTH_TEST_MODE=trueenv var enables headless e2e testing by making the callback server resolve immediately with a test auth code.client_namein OAuth client registration uses the MCP server config key (e.g., "figma") so providers see a meaningful name.Interactively review PR in Roo Code Cloud