feat: support OAuth 2.1 for streamable-http MCP servers#1
Open
feat: support OAuth 2.1 for streamable-http MCP servers#1
Conversation
…ows across instances
📝 WalkthroughWalkthroughAdds end-to-end OAuth 2.x support for HTTP MCP servers: RFC 9728 protected-resource discovery, RFC 8414/OIDC AS discovery, optional RFC 7591 dynamic client registration, PKCE-based auth-code flow with RFC 8707 resource indicator, local callback server, secure token persistence in VS Code secrets, token refresh, and integration into MCP connection lifecycle with tests and i18n. Changes
Sequence DiagramsequenceDiagram
participant Client as Roo Client
participant MCPServer as MCP Server
participant AuthServer as Authorization Server
participant CallbackServer as Local Callback<br/>(OAuth)
participant TokenStore as VS Code<br/>SecretStorage
Client->>MCPServer: POST /mcp (initial request)
MCPServer-->>Client: 401 Unauthorized (WWW-Authenticate)
Client->>MCPServer: GET /.well-known/oauth-protected-resource
MCPServer-->>Client: protected resource metadata (authorization_servers, resource)
Client->>AuthServer: GET /.well-known/oauth-authorization-server
AuthServer-->>Client: authorization server metadata (endpoints, PKCE support)
Client->>AuthServer: POST /register (DCR) [optional]
AuthServer-->>Client: client_id (±client_secret)
Client->>AuthServer: GET /authorize?code_challenge...&resource=...
Client->>User: open browser for login
AuthServer->>CallbackServer: Redirect with code + state
CallbackServer-->>Client: resolves auth code result
Client->>AuthServer: POST /token (code, code_verifier, resource)
AuthServer-->>Client: access_token, refresh_token
Client->>TokenStore: persist tokens & client_info
Client->>MCPServer: POST /mcp (Authorization: Bearer token)
MCPServer-->>Client: 200 OK (authenticated MCP response)
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
Comment |
edelauna
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 11
🧹 Nitpick comments (1)
src/services/mcp/utils/__tests__/callbackServer.spec.ts (1)
34-35: Avoid silent fallback when request listener is missing.Using a no-op fallback (
vi.fn()) masks registration failures and weakens diagnostics. Assert listener existence explicitly.Suggested fix
- const requestCall = mockServer.on.mock.calls.find((call) => call[0] === "request") - const requestHandler = requestCall ? requestCall[1] : vi.fn() + const requestCall = mockServer.on.mock.calls.find((call) => call[0] === "request") + expect(requestCall).toBeDefined() + const requestHandler = requestCall![1]Apply the same change in both test cases.
Also applies to: 72-73
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/mcp/utils/__tests__/callbackServer.spec.ts` around lines 34 - 35, The test currently falls back to a no-op handler when the "request" listener isn't registered (mockServer.on.mock.calls.find(...) ?: vi.fn()), which hides registration failures; change the logic to assert the listener exists instead of using vi.fn() — for example, after finding requestCall assert/requestCall is defined (e.g. expect(requestCall).toBeDefined() or throw an error) and then extract requestHandler = requestCall[1]; make this change for both occurrences (the requestCall/requestHandler extraction in both tests).
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@apps/vscode-e2e/src/suite/mcp-oauth.test.ts`:
- Around line 241-244: The teardown currently calls fs.rm(..., { recursive: true
}) on workspaceDir + "/.roo" which can delete a user's real workspace; change
the logic in the teardown to only remove the ".roo" directory when it's inside
the ephemeral test tempDir (use workspaceDir.startsWith(tempDir) or compare
equality to tempDir) or when the workspace was created by the test, otherwise
skip deletion; update the code around workspaceDir, tempDir and the fs.rm(...)
call to perform this conditional check before invoking fs.rm with
recursive:true.
- Around line 327-333: The test "Should reuse stored token on reconnect without
re-running the full OAuth flow" must not rely on previous tests; instead, before
triggering the reconnect, create a valid mock OAuth token and persist it into
the same SecretStorage key the SDK expects (so the SDK will read it), then clear
endpointsHit and proceed with the reconnect assertions; reference the test name
and the endpointsHit variable to locate where to insert the token setup and use
the same SecretStorage API (the in-test secret storage instance) and token key
the SDK uses so the reconnect path is exercised in isolation.
In `@src/i18n/locales/ca/mcp.json`:
- Around line 29-49: Translate the remaining English strings into Catalan by
updating the "callback" keys ("success", "failed", "auth_failed",
"auth_success", "server_connection_complete", "tab_closing_in", "safe_to_close",
"invalid_state" as needed) and the "flow" key "dismissedHint" so the OAuth UI is
fully localized; preserve interpolation tokens like {{count}} and {{name}} and
any HTML tags (e.g., <span id="count">) and ensure punctuation/accents match
Catalan conventions.
In `@src/services/mcp/McpHub.ts`:
- Around line 1057-1065: Race occurs because we call
secretStorage.getOAuthData(serverUrl) before we register the cross-window token
change watcher; register the watcher (the onDidChange-style listener used
elsewhere in this class) before reading tokens or, if you prefer minimal change,
immediately re-check secretStorage.getOAuthData(serverUrl) after registering the
watcher and treat the second read as authoritative; if the second read returns a
non-expired token (compare with TOKEN_EXPIRY_BUFFER_MS) then close authProvider
via authProvider.close(), call deleteConnection(name, source), call
connectToServer(name, config, source), call notifyWebviewOfServerChanges(), and
return. Apply the same fix at the other occurrence that uses getOAuthData (the
block around the connect flow that mirrors this logic).
In `@src/services/mcp/McpOAuthClientProvider.ts`:
- Around line 124-129: The code currently picks tokenEndpointAuthMethod from
authServerMeta.token_endpoint_auth_methods_supported and may choose
"client_secret_basic" which we do not implement; update the selection logic in
tokenEndpointAuthMethod (and analogous logic at the other occurrences) to only
ever pick supported methods we implement—specifically allow only "none" or
"client_secret_post" (fall back to "client_secret_post" when nothing present) by
filtering authMethods to those two values before selecting; also ensure any
advertised/returned metadata uses that filtered tokenEndpointAuthMethod so
downstream clients are not told to use client_secret_basic.
- Around line 436-457: The token request builders drop the RFC 8707 "resource"
indicator that redirectToAuthorization included, causing servers that bind token
requests to a resource to reject exchanges; update the code that builds the POST
body for the authorization-code exchange (where params includes grant_type,
code, redirect_uri, client_id, code_verifier) to add params.resource when a
resource was set during redirectToAuthorization, and likewise add the same
resource parameter in the refresh token request builder (the refresh flow around
the refresh token handling). Ensure you pull the same resource value stored on
the instance (the same property redirectToAuthorization used), include it only
when present, and keep the client_secret logic and content-type headers
unchanged so token_endpoint requests continue to be form-encoded.
In `@src/services/mcp/McpServerManager.ts`:
- Around line 40-43: McpHub is constructed before SecretStorageService is
injected, allowing async init in McpHub to start and attempt connections that
fail with "SecretStorageService not initialized"; change McpHub to accept
SecretStorageService (or its interface) in its constructor and assign it before
any initialization/async startup runs, then update the call site to create
SecretStorageService(context) first and pass it into new McpHub(provider,
secretStorage) instead of calling hub.setSecretStorage afterwards; ensure
McpHub's constructor removes or delays any async work until after the injected
secret storage is assigned.
In `@src/services/mcp/SecretStorageService.ts`:
- Around line 30-36: The current _key(serverUrl: string) loses information by
replacing non-alphanumerics in sanitizedPath, causing collisions (e.g., /a-b,
/a_b, /a/b). Replace the lossy sanitization with a collision-free encoding of
the pathname in _key: compute a safe deterministic token from url.pathname (for
example a URL-safe base64 or a short hex digest like SHA-256 of the pathname),
then use that encoded token as the pathSuffix (e.g.,
`${this._namespace}${url.host}.${encodedPath}.data`), preserving the namespace
and host logic but ensuring unique keys for distinct pathnames. Ensure the
encoding is deterministic, URL-safe, and trimmed so the final key contains only
filesystem/key-store safe characters.
In `@src/services/mcp/utils/__tests__/callbackServer.spec.ts`:
- Around line 10-12: Tests assume non-test OAuth path but can be affected if
MCP_OAUTH_TEST_MODE is set by other suites; update the beforeEach (and
optionally afterEach) in this spec to clear or reset
process.env.MCP_OAUTH_TEST_MODE so the suite always runs in the non-test-mode
branch — modify the existing beforeEach (where vi.restoreAllMocks() is called)
to delete or set process.env.MCP_OAUTH_TEST_MODE = undefined (or delete
process.env.MCP_OAUTH_TEST_MODE) before running tests to harden isolation.
In `@src/services/mcp/utils/__tests__/oauth.spec.ts`:
- Around line 11-13: The test file overwrites global.fetch with mockFetch at
module scope (mockFetch and global.fetch) and never restores it, risking
cross-test pollution; modify the test suite to save the original global.fetch
before assigning mockFetch (e.g., const originalFetch = global.fetch) and
restore it in a cleanup hook (afterEach or afterAll) that resets global.fetch =
originalFetch and clears/mockReset the vi mock to avoid leaking the mock into
other tests.
In `@src/services/mcp/utils/callbackServer.ts`:
- Around line 50-59: The timeout created for resultPromise (const timeout =
setTimeout(...)) is not cleared when the server is closed, leaving a dangling
timer that can later reject; update the callback flow so every code path that
closes the server or completes the flow (including stopCallbackServer(), the
server.close() call in the timeout handler, and any other resolve/reject paths
around resultPromise and the similar block at the other location) also clears
the timeout (clearTimeout(timeout)), sets the resolved flag, and avoids calling
rejectResult/resolveResult again; ensure both instances (the Promise around
resultPromise and the similar Promise at the other spot referenced) clear their
timers on early cancellation/cleanup and when resolving to prevent late
unhandled rejections.
---
Nitpick comments:
In `@src/services/mcp/utils/__tests__/callbackServer.spec.ts`:
- Around line 34-35: The test currently falls back to a no-op handler when the
"request" listener isn't registered (mockServer.on.mock.calls.find(...) ?:
vi.fn()), which hides registration failures; change the logic to assert the
listener exists instead of using vi.fn() — for example, after finding
requestCall assert/requestCall is defined (e.g.
expect(requestCall).toBeDefined() or throw an error) and then extract
requestHandler = requestCall[1]; make this change for both occurrences (the
requestCall/requestHandler extraction in both tests).
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: d7243221-b2f9-4754-bebe-4948e2e43fa4
📒 Files selected for processing (32)
apps/vscode-e2e/src/suite/mcp-oauth.test.tssrc/esbuild.mjssrc/i18n/locales/ca/mcp.jsonsrc/i18n/locales/de/mcp.jsonsrc/i18n/locales/en/mcp.jsonsrc/i18n/locales/es/mcp.jsonsrc/i18n/locales/fr/mcp.jsonsrc/i18n/locales/hi/mcp.jsonsrc/i18n/locales/id/mcp.jsonsrc/i18n/locales/it/mcp.jsonsrc/i18n/locales/ja/mcp.jsonsrc/i18n/locales/ko/mcp.jsonsrc/i18n/locales/nl/mcp.jsonsrc/i18n/locales/pl/mcp.jsonsrc/i18n/locales/pt-BR/mcp.jsonsrc/i18n/locales/ru/mcp.jsonsrc/i18n/locales/tr/mcp.jsonsrc/i18n/locales/vi/mcp.jsonsrc/i18n/locales/zh-CN/mcp.jsonsrc/i18n/locales/zh-TW/mcp.jsonsrc/services/mcp/McpHub.tssrc/services/mcp/McpOAuthClientProvider.tssrc/services/mcp/McpServerManager.tssrc/services/mcp/SecretStorageService.tssrc/services/mcp/__tests__/McpHub.spec.tssrc/services/mcp/__tests__/McpOAuthClientProvider.spec.tssrc/services/mcp/__tests__/SecretStorageService.spec.tssrc/services/mcp/constants.tssrc/services/mcp/utils/__tests__/callbackServer.spec.tssrc/services/mcp/utils/__tests__/oauth.spec.tssrc/services/mcp/utils/callbackServer.tssrc/services/mcp/utils/oauth.ts
Comment on lines
+327
to
+333
| test("Should reuse stored token on reconnect without re-running the full OAuth flow", async function () { | ||
| // This test runs after the previous one, so a token is already stored in SecretStorage. | ||
| // Trigger another reconnect — the SDK should inject the cached token directly and skip the | ||
| // browser-based auth flow (no new register or token endpoints should be hit). | ||
|
|
||
| // Clear only mcp-related hit tracking (token endpoint should NOT be re-hit) | ||
| endpointsHit.clear() |
There was a problem hiding this comment.
Make the token-reuse test self-contained (avoid cross-test state dependency).
This test currently depends on the previous test having already persisted a token, which makes isolated/sharded execution unreliable.
Suggested fix
test("Should reuse stored token on reconnect without re-running the full OAuth flow", async function () {
- // This test runs after the previous one, so a token is already stored in SecretStorage.
+ // Ensure precondition in this test: acquire and persist token first.
+ await waitFor(() => endpointsHit.has("token"), { timeout: 30_000 })
// Trigger another reconnect — the SDK should inject the cached token directly and skip the
// browser-based auth flow (no new register or token endpoints should be hit).📝 Committable suggestion
‼️ IMPORTANT
Carefully review the code before committing. Ensure that it accurately replaces the highlighted code, contains no missing lines, and has no issues with indentation. Thoroughly test & benchmark the code to ensure it meets the requirements.
Suggested change
| test("Should reuse stored token on reconnect without re-running the full OAuth flow", async function () { | |
| // This test runs after the previous one, so a token is already stored in SecretStorage. | |
| // Trigger another reconnect — the SDK should inject the cached token directly and skip the | |
| // browser-based auth flow (no new register or token endpoints should be hit). | |
| // Clear only mcp-related hit tracking (token endpoint should NOT be re-hit) | |
| endpointsHit.clear() | |
| test("Should reuse stored token on reconnect without re-running the full OAuth flow", async function () { | |
| // Ensure precondition in this test: acquire and persist token first. | |
| await waitFor(() => endpointsHit.has("token"), { timeout: 30_000 }) | |
| // Trigger another reconnect — the SDK should inject the cached token directly and skip the | |
| // browser-based auth flow (no new register or token endpoints should be hit). | |
| // Clear only mcp-related hit tracking (token endpoint should NOT be re-hit) | |
| endpointsHit.clear() |
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed.
In `@apps/vscode-e2e/src/suite/mcp-oauth.test.ts` around lines 327 - 333, The test
"Should reuse stored token on reconnect without re-running the full OAuth flow"
must not rely on previous tests; instead, before triggering the reconnect,
create a valid mock OAuth token and persist it into the same SecretStorage key
the SDK expects (so the SDK will read it), then clear endpointsHit and proceed
with the reconnect assertions; reference the test name and the endpointsHit
variable to locate where to insert the token setup and use the same
SecretStorage API (the in-test secret storage instance) and token key the SDK
uses so the reconnect path is exercised in isolation.
Author
There was a problem hiding this comment.
Fixed in 05b399b — the test now calls await waitFor(() => endpointsHit.has("token"), { timeout: 30_000 }) before clearing endpointsHit, making it self-contained regardless of execution order.
There was a problem hiding this comment.
Only users with a collaborator, contributor, member, or owner role can interact with CodeRabbit.
There was a problem hiding this comment.
Only users with a collaborator, contributor, member, or owner role can interact with CodeRabbit.
There was a problem hiding this comment.
Seems like the humans are having a chat. I'll hop back into my burrow for now. If you need me again, just tag @coderabbitai in a new comment, and I'll come hopping out!
There was a problem hiding this comment.
@coderabbitai Fixed in 05b399b — the test now calls await waitFor(() => endpointsHit.has("token"), { timeout: 30_000 }) before clearing endpointsHit, making it self-contained regardless of execution order.
There was a problem hiding this comment.
Only users with a collaborator, contributor, member, or owner role can interact with CodeRabbit.
There was a problem hiding this comment.
@coderabbitai Fixed in 05b399b — the test now calls await waitFor(() => endpointsHit.has("token"), { timeout: 30_000 }) before clearing endpointsHit, making it self-contained regardless of execution order.
There was a problem hiding this comment.
Only users with a collaborator, contributor, member, or owner role can interact with CodeRabbit.
There was a problem hiding this comment.
Actionable comments posted: 2
🧹 Nitpick comments (1)
src/services/mcp/SecretStorageService.ts (1)
17-21: Docstring is slightly misleading — tokens are keyed by host AND path.The comment says "keyed by host" and "different servers on the same host share credentials," but the implementation at lines 30-36 actually uses both host AND pathname (base64url-encoded) for the key. This is the correct behavior for isolating different MCP servers, but the docstring suggests credentials are shared by host only.
📝 Proposed documentation fix
/** * Thin wrapper around VS Code SecretStorage for persisting MCP OAuth tokens. - * Tokens are stored per-server (keyed by host) so different servers on the - * same host share credentials, which is the common case for multi-path APIs. + * Tokens are stored per-server, keyed by host and URL path, so different MCP + * endpoints on the same host maintain separate credentials. */🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@src/services/mcp/SecretStorageService.ts` around lines 17 - 21, Update the class docstring in SecretStorageService to accurately state that tokens are keyed by both host and path rather than host only: mention that SecretStorageService (and the key-building logic that base64url-encodes the pathname) constructs keys using the host plus the encoded pathname to isolate credentials for different MCP servers on the same host, and remove or adjust the sentence implying different servers on the same host share credentials.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Inline comments:
In `@src/services/mcp/__tests__/McpOAuthClientProvider.spec.ts`:
- Around line 23-24: The test suite overwrites global.fetch with mockFetch at
module scope and never restores it, risking cross-test pollution; capture the
original fetch (e.g., const originalFetch = global.fetch), assign global.fetch =
mockFetch inside a beforeAll or beforeEach, and then restore it in an afterAll
(or afterEach) by setting global.fetch = originalFetch and clearing/resetting
mockFetch (mockFetch.mockReset() or vi.resetAllMocks()) so the mock does not
leak into other tests; reference the mockFetch and global.fetch symbols and
update McpOAuthClientProvider.spec to follow the same setup/teardown pattern
used in oauth.spec.ts.
In `@src/services/mcp/__tests__/SecretStorageService.spec.ts`:
- Around line 120-164: The onDidChange test suite is incorrectly nested inside
the deleteOAuthData suite; move the entire describe("onDidChange", ...) block so
it is a sibling of describe("deleteOAuthData", ...) (i.e., close the
deleteOAuthData describe before the onDidChange describe) to make tests for
deleteOAuthData and onDidChange separate; locate the blocks by the unique
identifiers deleteOAuthData and onDidChange in SecretStorageService.spec.ts and
adjust the braces/closing parentheses accordingly so onDidChange is not nested.
---
Nitpick comments:
In `@src/services/mcp/SecretStorageService.ts`:
- Around line 17-21: Update the class docstring in SecretStorageService to
accurately state that tokens are keyed by both host and path rather than host
only: mention that SecretStorageService (and the key-building logic that
base64url-encodes the pathname) constructs keys using the host plus the encoded
pathname to isolate credentials for different MCP servers on the same host, and
remove or adjust the sentence implying different servers on the same host share
credentials.
🪄 Autofix (Beta)
Fix all unresolved CodeRabbit comments on this PR:
- Push a commit to this branch (recommended)
- Create a new PR with the fixes
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ca1f72dd-a91b-49a2-85b7-111dc7364fe0
📒 Files selected for processing (11)
src/i18n/locales/ca/mcp.jsonsrc/services/mcp/McpHub.tssrc/services/mcp/McpOAuthClientProvider.tssrc/services/mcp/McpServerManager.tssrc/services/mcp/SecretStorageService.tssrc/services/mcp/__tests__/McpHub.spec.tssrc/services/mcp/__tests__/McpOAuthClientProvider.spec.tssrc/services/mcp/__tests__/SecretStorageService.spec.tssrc/services/mcp/utils/__tests__/callbackServer.spec.tssrc/services/mcp/utils/__tests__/oauth.spec.tssrc/services/mcp/utils/callbackServer.ts
✅ Files skipped from review due to trivial changes (2)
- src/services/mcp/tests/McpHub.spec.ts
- src/services/mcp/McpOAuthClientProvider.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/services/mcp/McpServerManager.ts
There was a problem hiding this comment.
♻️ Duplicate comments (2)
apps/vscode-e2e/src/suite/mcp-oauth.test.ts (2)
200-215:⚠️ Potential issue | 🟠 MajorPreserve pre-existing
.roo/mcp.jsoninstead of overwriting/deleting it blindly.Line 214 overwrites workspace config, and Lines 233–239 later delete it unconditionally. If the file existed before the test, local user config can be lost.
💡 Proposed fix
suite("Roo Code MCP OAuth", function () { setDefaultSuiteTimeout(this) let tempDir: string let testFiles: { mcpConfig: string } + let originalMcpConfig: string | undefined + let hadOriginalMcpConfig = false let mockServer: http.Server let mockServerPort: number @@ suiteSetup(async () => { @@ testFiles = { mcpConfig: path.join(rooDir, "mcp.json") } + try { + originalMcpConfig = await fs.readFile(testFiles.mcpConfig, "utf8") + hadOriginalMcpConfig = true + } catch { + hadOriginalMcpConfig = false + } await fs.writeFile(testFiles.mcpConfig, JSON.stringify(mcpConfig, null, 2)) @@ suiteTeardown(async () => { @@ - for (const filePath of Object.values(testFiles)) { - try { - await fs.unlink(filePath) - } catch { - // ignore - } - } + try { + if (hadOriginalMcpConfig && originalMcpConfig !== undefined) { + await fs.writeFile(testFiles.mcpConfig, originalMcpConfig, "utf8") + } else { + await fs.unlink(testFiles.mcpConfig) + } + } catch { + // ignore + }Also applies to: 233-239
🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/vscode-e2e/src/suite/mcp-oauth.test.ts` around lines 200 - 215, The test currently unconditionally writes and later deletes the workspace .roo/mcp.json (variables workspaceDir, rooDir, testFiles.mcpConfig), risking loss of a user's config; modify the test to detect if testFiles.mcpConfig already exists and preserve it—either skip writing if present or create a temporary backup copy before writing—and on cleanup restore the original from the backup (or only delete the file if the test created it), ensuring teardown only removes test-created artifacts and not pre-existing .roo/mcp.json.
331-335:⚠️ Potential issue | 🟠 MajorToken-reuse precondition is flaky and still order-dependent.
Line 334 waits for
"token"before triggering any flow in this test, whilesetup()clearsendpointsHit. In isolated/sharded runs this can timeout.💡 Proposed fix (self-contained bootstrap inside this test)
test("Should reuse stored token on reconnect without re-running the full OAuth flow", async function () { - // Ensure a token is in SecretStorage before testing reuse — this makes the - // test self-contained regardless of execution order. - await waitFor(() => endpointsHit.has("token"), { timeout: 30_000 }) + // Bootstrap once in this test to ensure token is persisted. + const workspaceDir = vscode.workspace.workspaceFolders?.[0]?.uri.fsPath || tempDir + const mcpConfigPath = path.join(workspaceDir, ".roo", "mcp.json") + await fs.writeFile( + mcpConfigPath, + JSON.stringify({ + mcpServers: { + "test-oauth-server": { type: "streamable-http", url: `http://localhost:${mockServerPort}/mcp` }, + }, + }, null, 2), + ) + await waitFor(() => endpointsHit.has("token"), { timeout: 30_000 }) // Clear hit tracking so we can assert the token endpoint is NOT re-hit. endpointsHit.clear() - const workspaceDir = vscode.workspace.workspaceFolders?.[0]?.uri.fsPath || tempDir - const mcpConfigPath = path.join(workspaceDir, ".roo", "mcp.json")🤖 Prompt for AI Agents
Verify each finding against the current code and only fix it if needed. In `@apps/vscode-e2e/src/suite/mcp-oauth.test.ts` around lines 331 - 335, The test is flakey because it waits for endpointsHit to already contain "token" even though setup() clears endpointsHit; make the test self-contained by pre-seeding the token and endpoint-hit state before the waitFor. Specifically, in the "Should reuse stored token..." test, write a valid token into the SecretStorage (or call the existing helper that stores a token) and add "token" to endpointsHit (e.g., endpointsHit.add("token")) or run the minimal bootstrap logic used elsewhere to simulate the token endpoint being hit, then proceed with the rest of the test so it no longer depends on previous tests' order.
🤖 Prompt for all review comments with AI agents
Verify each finding against the current code and only fix it if needed.
Duplicate comments:
In `@apps/vscode-e2e/src/suite/mcp-oauth.test.ts`:
- Around line 200-215: The test currently unconditionally writes and later
deletes the workspace .roo/mcp.json (variables workspaceDir, rooDir,
testFiles.mcpConfig), risking loss of a user's config; modify the test to detect
if testFiles.mcpConfig already exists and preserve it—either skip writing if
present or create a temporary backup copy before writing—and on cleanup restore
the original from the backup (or only delete the file if the test created it),
ensuring teardown only removes test-created artifacts and not pre-existing
.roo/mcp.json.
- Around line 331-335: The test is flakey because it waits for endpointsHit to
already contain "token" even though setup() clears endpointsHit; make the test
self-contained by pre-seeding the token and endpoint-hit state before the
waitFor. Specifically, in the "Should reuse stored token..." test, write a valid
token into the SecretStorage (or call the existing helper that stores a token)
and add "token" to endpointsHit (e.g., endpointsHit.add("token")) or run the
minimal bootstrap logic used elsewhere to simulate the token endpoint being hit,
then proceed with the rest of the test so it no longer depends on previous
tests' order.
ℹ️ Review info
⚙️ Run configuration
Configuration used: defaults
Review profile: CHILL
Plan: Pro Plus
Run ID: ff648812-a244-4b28-95e4-252c7493eedd
📒 Files selected for processing (3)
apps/vscode-e2e/src/suite/mcp-oauth.test.tssrc/services/mcp/__tests__/McpOAuthClientProvider.spec.tssrc/services/mcp/__tests__/SecretStorageService.spec.ts
🚧 Files skipped from review as they are similar to previous changes (1)
- src/services/mcp/tests/SecretStorageService.spec.ts
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Related GitHub Issue
Closes: RooCodeInc#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_initiateOAuthFlow()so the extension (chat window, other servers) isn't blocked waiting for the user's browser session. A persistent VS Code progress notification gives the user a visible Cancel button for the duration of the flow.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.Reconnect after auth: After token exchange, the stale connection is deleted and
connectToServer()is called fresh — the SDK's transport can't be reused afterUnauthorizedErrorbecause its internal_abortControlleris already set.Mid-session re-auth: If a 401 arrives during a tool call, the extension shows a confirmation toast before opening the browser, so the user is always in control. The
Promise.racepattern in_completeOAuthFlowensures the Cancel button on the progress bar abortswaitForAuthCode()if the user changes their mind after clicking Authenticate.Negative cache: An in-memory TTL cache (
isKnownNonOAuth) skips the OAuth discovery round-trip for servers already known not to require auth.Multi-window coordination:
SecretStorage.onDidChangeis used to detect when another VS Code window completes the OAuth flow, so the current window reconnects without prompting again.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 (across 4 files):
McpOAuthClientProvider.spec.ts— covers create, clientMetadata, token storage/expiry, PKCE, redirect, auth code, closeSecretStorageService.spec.ts— covers CRUD, key isolation, malformed dataoauth.spec.ts— covers RFC 8414 URL construction for path/no-path issuers, error handlingcallbackServer.spec.ts— covers callback handling and CSRF state validationMcpHub.spec.ts— covers_initiateOAuthFlow: persistent notification, cross-window tokens, cancellation, timeout, concurrent watcher deduplicationManual testing:
Tested against real OAuth-protected MCP servers (
https://temporal.mcp.kapa.aiandhttps://mcp.figma.com/mcp) — full flow completes successfully.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.Get in Touch
Summary by CodeRabbit
New Features
Improvements
Tests