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 26 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.
|
@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.
3d0eb27 to
11798c2
Compare
11798c2 to
5aeed4f
Compare
|
@roomote is this good to go? |
I've reviewed the PR and run all checks. Here's the summary: Tests: All 69 unit tests pass across 4 test files ( Type checking: i18n: All 18 locale files have consistent OAuth callback keys. Code review findings -- the implementation looks solid:
No blocking issues found. The PR is ready for merge from a code quality and test coverage perspective. |
…ows across instances
83dbb12 to
fda3e98
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