From 382678beaa657b4e1d52d33672d3bbde1995499f Mon Sep 17 00:00:00 2001 From: Jesse Quijano Date: Tue, 20 Oct 2026 19:34:41 -0700 Subject: [PATCH 1/7] feat(mcp): add static OAuth client configuration (#2096) --- src/eslint-suppressions.json | 2 +- src/services/mcp/McpHub.ts | 56 +++++--- src/services/mcp/McpOAuthClientProvider.ts | 114 ++++++++++++--- src/services/mcp/SecretStorageService.ts | 29 ++-- src/services/mcp/__tests__/McpHub.spec.ts | 42 ++++++ .../__tests__/McpOAuthClientProvider.spec.ts | 135 ++++++++++++++++++ .../__tests__/SecretStorageService.spec.ts | 20 +++ 7 files changed, 346 insertions(+), 52 deletions(-) diff --git a/src/eslint-suppressions.json b/src/eslint-suppressions.json index 854a6fb91f..f357f261a4 100644 --- a/src/eslint-suppressions.json +++ b/src/eslint-suppressions.json @@ -1481,7 +1481,7 @@ }, "services/mcp/McpHub.ts": { "@typescript-eslint/no-explicit-any": { - "count": 8 + "count": 7 } }, "services/mcp/McpOAuthClientProvider.ts": { diff --git a/src/services/mcp/McpHub.ts b/src/services/mcp/McpHub.ts index f0321eed76..60a1caacc4 100644 --- a/src/services/mcp/McpHub.ts +++ b/src/services/mcp/McpHub.ts @@ -84,7 +84,7 @@ const stdioFieldsErrorMessage = const sseFieldsErrorMessage = "For 'sse' type servers, you must provide a 'url' field and can optionally include 'headers'" const streamableHttpFieldsErrorMessage = - "For 'streamable-http' type servers, you must provide a 'url' field and can optionally include 'headers'" + "For 'streamable-http' type servers, you must provide a 'url' field and can optionally include 'headers' and 'oauth'" const mixedFieldsErrorMessage = "Cannot mix 'stdio' and ('sse' or 'streamable-http') fields. For 'stdio' use 'command', 'args', and 'env'. For 'sse'/'streamable-http' use 'url' and 'headers'" const missingFieldsErrorMessage = @@ -103,6 +103,7 @@ const createServerTypeSchema = () => { // Ensure no SSE fields are present url: z.undefined().optional(), headers: z.undefined().optional(), + oauth: z.undefined().optional(), }) .transform((data) => ({ ...data, @@ -114,6 +115,7 @@ const createServerTypeSchema = () => { type: z.enum(["sse"]).optional(), url: z.string().url("URL must be a valid URL format"), headers: z.record(z.string(), z.string()).optional(), + oauth: z.undefined().optional(), // Ensure no stdio fields are present command: z.undefined().optional(), args: z.undefined().optional(), @@ -129,6 +131,14 @@ const createServerTypeSchema = () => { type: z.enum(["streamable-http"]).optional(), url: z.string().url("URL must be a valid URL format"), headers: z.record(z.string(), z.string()).optional(), + oauth: z + .object({ + clientId: z.string().min(1, "OAuth clientId cannot be empty"), + scope: z.string().optional(), + redirectUri: z.string().url("OAuth redirectUri must be a valid URL").optional(), + resource: z.union([z.literal("auto"), z.literal(false), z.string().url()]).optional(), + }) + .optional(), // Ensure no stdio fields are present command: z.undefined().optional(), args: z.undefined().optional(), @@ -821,19 +831,21 @@ export class McpHub { // 1. SecretStorage has OAuth data → known OAuth server → discover // 2. In-memory negative cache says non-OAuth → skip discovery // 3. Unknown server (first connection) → discover once, cache result - const hasOAuthData = await this.secretStorage.hasOAuthData(configInjected.url) + const oauthClientId = configInjected.oauth?.clientId + const hasOAuthData = await this.secretStorage.hasOAuthData(configInjected.url, oauthClientId) const knownNonOAuth = McpOAuthClientProvider.isKnownNonOAuth(configInjected.url) - const skipDiscovery = !hasOAuthData && knownNonOAuth + const skipDiscovery = !oauthClientId && !hasOAuthData && knownNonOAuth const authProvider = await McpOAuthClientProvider.create(configInjected.url, this.secretStorage, name, { skipDiscovery, + ...configInjected.oauth, }) // Pre-register the OAuth client so the SDK can skip its own // registration step (broken for path-prefixed issuers — see // utils/oauth.ts for upstream issue links). // Skip when discovery was skipped — there's no metadata to register with. - if (!skipDiscovery) { + if (!skipDiscovery && !oauthClientId) { try { await authProvider.registerClientIfNeeded() } catch { @@ -1066,7 +1078,8 @@ export class McpHub { transport: StreamableHTTPClientTransport, connection: ConnectedMcpConnection, ): Promise { - const serverUrl = "url" in config ? (config as any).url : undefined + const serverUrl = "url" in config ? config.url : undefined + const oauthClientId = config.type === "streamable-http" ? config.oauth?.clientId : undefined if (!serverUrl || !this.secretStorage) { return } @@ -1078,13 +1091,17 @@ export class McpHub { // Note: onDidChange fires on both writes AND deletes — the re-read below is // authoritative; we only reconnect if it returns a valid (non-expired) token. let tokenChangedDuringRead = false - const unsubscribeCrossWindow = this.secretStorage.onDidChange(serverUrl, () => { - tokenChangedDuringRead = true - }) + const unsubscribeCrossWindow = this.secretStorage.onDidChange( + serverUrl, + () => { + tokenChangedDuringRead = true + }, + oauthClientId, + ) // Check if another window already saved valid tokens. // Re-read after registering the watcher in case a write landed in the gap. - const existing = await this.secretStorage.getOAuthData(serverUrl) + const existing = await this.secretStorage.getOAuthData(serverUrl, oauthClientId) if (this.isDisposed) { unsubscribeCrossWindow() return @@ -1092,7 +1109,8 @@ export class McpHub { // If the first read missed but the watcher fired during it, re-read — a write // may have landed between subscription and read completion. const tokenToUse = - existing ?? (tokenChangedDuringRead ? await this.secretStorage.getOAuthData(serverUrl) : undefined) + existing ?? + (tokenChangedDuringRead ? await this.secretStorage.getOAuthData(serverUrl, oauthClientId) : undefined) unsubscribeCrossWindow() if (!this.isDisposed && tokenToUse && Date.now() < tokenToUse.expires_at - TOKEN_EXPIRY_BUFFER_MS) { @@ -1122,6 +1140,7 @@ export class McpHub { connection, progress, cancellationToken, + oauthClientId, ), ) } @@ -1136,6 +1155,7 @@ export class McpHub { connection: ConnectedMcpConnection, progress: vscode.Progress<{ message?: string; increment?: number }>, cancellationToken: vscode.CancellationToken, + oauthClientId?: string, ): Promise { const watcherKey = `${name}:${source}` @@ -1170,7 +1190,7 @@ export class McpHub { return } try { - const data = await this.secretStorage?.getOAuthData(serverUrl) + const data = await this.secretStorage?.getOAuthData(serverUrl, oauthClientId) // Re-check after the async yield — cancellation or completion may have // fired while getOAuthData was in flight. if (disposed || this.isDisposed) return @@ -1188,9 +1208,13 @@ export class McpHub { } } - const unsubscribe = this.secretStorage!.onDidChange(serverUrl, () => { - void onTokensChanged() - }) + const unsubscribe = this.secretStorage!.onDidChange( + serverUrl, + () => { + void onTokensChanged() + }, + oauthClientId, + ) // --- Timeout --- const timeoutHandle = setTimeout(() => { @@ -1273,7 +1297,7 @@ export class McpHub { } // Guard: another window may have authed while the toast was open - const tokens = await this.secretStorage!.getOAuthData(serverUrl) + const tokens = await this.secretStorage!.getOAuthData(serverUrl, oauthClientId) if (disposed) return if (tokens && Date.now() < tokens.expires_at - TOKEN_EXPIRY_BUFFER_MS) { cleanup(cancellationDisposable) @@ -1757,7 +1781,7 @@ export class McpHub { // Clear OAuth tokens for streamable-http servers on restart if (validatedConfig.type === "streamable-http" && this.secretStorage) { - await this.secretStorage.deleteOAuthData(validatedConfig.url) + await this.secretStorage.deleteOAuthData(validatedConfig.url, validatedConfig.oauth?.clientId) } // Try to connect again using validated config diff --git a/src/services/mcp/McpOAuthClientProvider.ts b/src/services/mcp/McpOAuthClientProvider.ts index 52b328c35a..c8a27b8c3c 100644 --- a/src/services/mcp/McpOAuthClientProvider.ts +++ b/src/services/mcp/McpOAuthClientProvider.ts @@ -21,6 +21,19 @@ import { SecretStorageService } from "./SecretStorageService" import { startCallbackServer, stopCallbackServer } from "./utils/callbackServer" import { fetchOAuthAuthServerMetadata } from "./utils/oauth" +export type McpOAuthResource = "auto" | false | string + +export interface McpOAuthClientOptions { + clientId: string + scope?: string + redirectUri?: string + resource?: McpOAuthResource +} + +interface McpOAuthProviderOptions extends Partial { + skipDiscovery?: boolean +} + /** * Implements the MCP SDK's OAuthClientProvider interface for VS Code. * @@ -93,6 +106,8 @@ export class McpOAuthClientProvider implements OAuthClientProvider { private readonly _authServerMeta: Record | null, private readonly _resourceIndicator: string | null, private readonly _clientName: string, + private readonly _configuredRedirectUri?: string, + private readonly _configuredClientId?: string, ) {} /** @@ -109,7 +124,7 @@ export class McpOAuthClientProvider implements OAuthClientProvider { serverUrl: string, secretStorage: SecretStorageService, serverName?: string, - options?: { skipDiscovery?: boolean }, + options?: McpOAuthProviderOptions, ): Promise { let authServerMeta: Record | null = null let resourceIndicator: string | null = null @@ -121,7 +136,7 @@ export class McpOAuthClientProvider implements OAuthClientProvider { // - RFC 8707 resource indicator (injected into authorization URL) const discovery = await fetchOAuthAuthServerMetadata(serverUrl) authServerMeta = discovery?.authServerMeta ?? null - resourceIndicator = discovery?.resourceIndicator ?? null + resourceIndicator = resolveResourceIndicator(options?.resource, discovery?.resourceIndicator) // Cache the result so subsequent connections can skip the probe. if (!authServerMeta) { @@ -134,7 +149,10 @@ export class McpOAuthClientProvider implements OAuthClientProvider { const authMethods: string[] = authServerMeta?.token_endpoint_auth_methods_supported ?? [] const tokenEndpointAuthMethod = authMethods.includes("none") ? "none" : "client_secret_post" const grantTypes = selectMcpOAuthGrantTypes(authServerMeta?.grant_types_supported) - const scopes: string[] = authServerMeta?.scopes_supported ?? [] + const scopes = + options?.scope !== undefined + ? options.scope.split(/\s+/).filter((scope) => scope.length > 0) + : (authServerMeta?.scopes_supported ?? []) // Generate a CSRF state token for the OAuth flow. const state = Array.from(crypto.getRandomValues(new Uint8Array(16))) @@ -144,7 +162,7 @@ export class McpOAuthClientProvider implements OAuthClientProvider { // We start the callback server lazily in `redirectToAuthorization()` or `waitForAuthCode()`. // We use a default port (0) initially; it will be updated when the server starts. - return new McpOAuthClientProvider( + const provider = new McpOAuthClientProvider( serverUrl, secretStorage, null, @@ -158,7 +176,26 @@ export class McpOAuthClientProvider implements OAuthClientProvider { authServerMeta, resourceIndicator, serverName || "Roo Code", + options?.redirectUri, + options?.clientId, ) + + if (options?.clientId) { + if (options.redirectUri) { + getCallbackPort(options.redirectUri) + } else { + // A static client skips DCR, which normally starts the callback server. + // Start it here so the SDK never receives localhost:0 as redirect_uri. + await provider._ensureCallbackServer() + } + provider._clientInfo = { + client_id: options.clientId, + redirect_uris: [provider.redirectUrl], + token_endpoint_auth_method: "none", + } + } + + return provider } // ── OAuthClientProvider interface ──────────────────────────────────────── @@ -169,7 +206,7 @@ export class McpOAuthClientProvider implements OAuthClientProvider { } get redirectUrl(): string { - return `http://localhost:${this._port}/callback` + return this._configuredRedirectUri ?? `http://localhost:${this._port}/callback` } private _ensureCallbackServer(): Promise { @@ -187,7 +224,10 @@ export class McpOAuthClientProvider implements OAuthClientProvider { private async _doStartCallbackServer(): Promise { this._closed = false - const { server, port, result, cancel } = await startCallbackServer(this._port, this._state) + const { server, port, result, cancel } = await startCallbackServer( + getCallbackPort(this._configuredRedirectUri) ?? this._port, + this._state, + ) this._server = server this._port = port this._cancelCallbackServer = cancel @@ -235,7 +275,7 @@ export class McpOAuthClientProvider implements OAuthClientProvider { if (this._clientInfo) return // already registered // Check if we have a cached client_id from previous registration - const cachedData = await this._secretStorage.getOAuthData(this._serverUrl) + const cachedData = await this._secretStorage.getOAuthData(this._serverUrl, this._configuredClientId) // Reuse the cached registration only for the issuer that created it. Otherwise // the stored client_secret would go to an authorization server the MCP server chose. if (cachedData?.client_info && cachedData.issuer === this._authServerMeta?.issuer) { @@ -271,7 +311,7 @@ export class McpOAuthClientProvider implements OAuthClientProvider { } async tokens(): Promise { - const data = await this._secretStorage.getOAuthData(this._serverUrl) + const data = await this._secretStorage.getOAuthData(this._serverUrl, this._configuredClientId) if (!data) return undefined // If the access token is still valid (with 5m buffer), return it. @@ -284,7 +324,7 @@ export class McpOAuthClientProvider implements OAuthClientProvider { // Never send the refresh token to an authorization server other than the one // that issued it. Data without an issuer (saved by older versions) is cleared. if (data.issuer !== this._authServerMeta?.issuer) { - await this._secretStorage.deleteOAuthData(this._serverUrl) + await this._secretStorage.deleteOAuthData(this._serverUrl, this._configuredClientId) return undefined } if (this._refreshPromise) { @@ -308,7 +348,7 @@ export class McpOAuthClientProvider implements OAuthClientProvider { } catch (error) { console.error(`Failed to refresh MCP OAuth token for ${this._serverUrl}:`, error) // Clear stale tokens on refresh failure so we don't keep retrying a dead refresh token - await this._secretStorage.deleteOAuthData(this._serverUrl) + await this._secretStorage.deleteOAuthData(this._serverUrl, this._configuredClientId) // Fall through to return undefined, which triggers full re-auth } } @@ -322,12 +362,16 @@ export class McpOAuthClientProvider implements OAuthClientProvider { clientIdOverride && this._clientInfo ? { ...this._clientInfo, client_id: clientIdOverride } : this._clientInfo - await this._secretStorage.saveOAuthData(this._serverUrl, { - tokens, - expires_at, - ...(this._authServerMeta?.issuer ? { issuer: this._authServerMeta.issuer as string } : {}), - ...(clientInfo ? { client_info: clientInfo } : {}), - }) + await this._secretStorage.saveOAuthData( + this._serverUrl, + { + tokens, + expires_at, + ...(this._authServerMeta?.issuer ? { issuer: this._authServerMeta.issuer as string } : {}), + ...(clientInfo ? { client_info: clientInfo } : {}), + }, + this._configuredClientId, + ) } async redirectToAuthorization(authorizationUrl: URL): Promise { @@ -347,6 +391,11 @@ export class McpOAuthClientProvider implements OAuthClientProvider { // 3. Add `scope` when the server advertises scopes but the SDK omitted it. // 4. Add RFC 8707 `resource` parameter when the protected resource metadata // advertised a resource indicator. + if (this._resourceIndicator) { + authorizationUrl.searchParams.set("resource", this._resourceIndicator) + } else { + authorizationUrl.searchParams.delete("resource") + } let correctedUrl = authorizationUrl if (this._authServerMeta?.authorization_endpoint) { try { @@ -370,11 +419,6 @@ export class McpOAuthClientProvider implements OAuthClientProvider { if (!fixed.searchParams.has("scope") && this._scopes.length > 0) { fixed.searchParams.set("scope", this._scopes.join(" ")) } - // RFC 8707: inject the resource indicator so the auth server can - // scope the issued access token to this specific resource server. - if (this._resourceIndicator && !fixed.searchParams.has("resource")) { - fixed.searchParams.set("resource", this._resourceIndicator) - } correctedUrl = fixed } catch { // Fall through and use the original URL if correction fails @@ -557,3 +601,31 @@ export class McpOAuthClientProvider implements OAuthClientProvider { } } } + +function resolveResourceIndicator(configured: McpOAuthResource | undefined, discovered?: string | null): string | null { + if (configured === false) return null + if (configured && configured !== "auto") return configured + return discovered ?? null +} + +function getCallbackPort(redirectUri?: string): number | undefined { + if (!redirectUri) return undefined + const url = new URL(redirectUri) + if (url.hostname !== "localhost" && url.hostname !== "127.0.0.1") { + throw new Error("MCP OAuth redirectUri must use a loopback host") + } + if (url.protocol !== "http:") { + throw new Error("MCP OAuth redirectUri must use HTTP on a loopback host") + } + if (url.pathname !== "/callback") { + throw new Error("MCP OAuth redirectUri path must be /callback") + } + if (url.username || url.password || url.search || url.hash) { + throw new Error("MCP OAuth redirectUri must not contain credentials, a query, or a fragment") + } + const port = Number(url.port) + if (!Number.isInteger(port) || port < 1 || port > 65535) { + throw new Error("MCP OAuth redirectUri must specify a valid port") + } + return port +} diff --git a/src/services/mcp/SecretStorageService.ts b/src/services/mcp/SecretStorageService.ts index e2cda51715..11eb3f4d01 100644 --- a/src/services/mcp/SecretStorageService.ts +++ b/src/services/mcp/SecretStorageService.ts @@ -21,8 +21,8 @@ export interface StoredMcpOAuthData { /** * 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 path and optional static client ID. Dynamic + * registrations keep the legacy per-server key for backward compatibility. */ export class SecretStorageService { private readonly _storage: vscode.SecretStorage @@ -32,16 +32,17 @@ export class SecretStorageService { this._storage = context.secrets } - private _key(serverUrl: string): string { + private _key(serverUrl: string, clientId?: string): string { const url = new URL(serverUrl) const normalizedPath = url.pathname.replace(/\/$/, "") // Use base64url encoding to avoid collisions between paths like /a-b, /a_b, /a/b. const pathSuffix = normalizedPath ? `.${Buffer.from(normalizedPath).toString("base64url")}` : "" - return `${this._namespace}${url.host}${pathSuffix}.data` + const clientSuffix = clientId ? `.${Buffer.from(clientId).toString("base64url")}` : "" + return `${this._namespace}${url.host}${pathSuffix}${clientSuffix}.data` } - async getOAuthData(serverUrl: string): Promise { - const raw = await this._storage.get(this._key(serverUrl)) + async getOAuthData(serverUrl: string, clientId?: string): Promise { + const raw = await this._storage.get(this._key(serverUrl, clientId)) if (!raw) return undefined try { return JSON.parse(raw) as StoredMcpOAuthData @@ -50,17 +51,17 @@ export class SecretStorageService { } } - async saveOAuthData(serverUrl: string, data: StoredMcpOAuthData): Promise { - await this._storage.store(this._key(serverUrl), JSON.stringify(data)) + async saveOAuthData(serverUrl: string, data: StoredMcpOAuthData, clientId?: string): Promise { + await this._storage.store(this._key(serverUrl, clientId), JSON.stringify(data)) } - async hasOAuthData(serverUrl: string): Promise { - const raw = await this._storage.get(this._key(serverUrl)) + async hasOAuthData(serverUrl: string, clientId?: string): Promise { + const raw = await this._storage.get(this._key(serverUrl, clientId)) return raw !== undefined } - async deleteOAuthData(serverUrl: string): Promise { - await this._storage.delete(this._key(serverUrl)) + async deleteOAuthData(serverUrl: string, clientId?: string): Promise { + await this._storage.delete(this._key(serverUrl, clientId)) } /** @@ -70,8 +71,8 @@ export class SecretStorageService { * * @returns A dispose function — call it to stop listening. */ - onDidChange(serverUrl: string, callback: () => void): () => void { - const key = this._key(serverUrl) + onDidChange(serverUrl: string, callback: () => void, clientId?: string): () => void { + const key = this._key(serverUrl, clientId) const disposable = this._storage.onDidChange((e) => { if (e.key === key) { callback() diff --git a/src/services/mcp/__tests__/McpHub.spec.ts b/src/services/mcp/__tests__/McpHub.spec.ts index 31ffc0ea60..2486ff960b 100644 --- a/src/services/mcp/__tests__/McpHub.spec.ts +++ b/src/services/mcp/__tests__/McpHub.spec.ts @@ -1691,6 +1691,48 @@ describe("McpHub", () => { ) }) + describe("static OAuth configuration", () => { + it("accepts static OAuth settings for streamable HTTP", () => { + const result = ServerConfigSchema.parse({ + type: "streamable-http", + url: "https://mcp.example.com/mcp", + oauth: { + clientId: "registered-client", + scope: "openid profile offline_access", + redirectUri: "http://localhost:33418/callback", + resource: false, + }, + }) + + expect(result.oauth).toEqual({ + clientId: "registered-client", + scope: "openid profile offline_access", + redirectUri: "http://localhost:33418/callback", + resource: false, + }) + }) + + it("accepts a static client without other OAuth settings", () => { + const result = ServerConfigSchema.parse({ + type: "streamable-http", + url: "https://mcp.example.com/mcp", + oauth: { clientId: "registered-client" }, + }) + + expect(result.oauth).toEqual({ clientId: "registered-client" }) + }) + + it.each([ + { type: "stdio", command: "test", oauth: { clientId: "client" } }, + { type: "sse", url: "https://mcp.example.com/sse", oauth: { clientId: "client" } }, + { type: "streamable-http", url: "https://mcp.example.com/mcp", oauth: { clientId: "" } }, + { type: "streamable-http", url: "https://mcp.example.com/mcp", oauth: { resource: "invalid" } }, + { type: "streamable-http", url: "https://mcp.example.com/mcp", oauth: { resource: false } }, + ])("rejects invalid OAuth configuration: $type", (config) => { + expect(() => ServerConfigSchema.parse(config)).toThrow() + }) + }) + describe("timeout configuration", () => { it("should validate timeout values", () => { // Test valid timeout values diff --git a/src/services/mcp/__tests__/McpOAuthClientProvider.spec.ts b/src/services/mcp/__tests__/McpOAuthClientProvider.spec.ts index 700ede149c..8398ab1138 100644 --- a/src/services/mcp/__tests__/McpOAuthClientProvider.spec.ts +++ b/src/services/mcp/__tests__/McpOAuthClientProvider.spec.ts @@ -168,6 +168,50 @@ describe("McpOAuthClientProvider", () => { expect(provider.hasMetadata).toBe(false) expect(McpOAuthClientProvider.isKnownNonOAuth("https://no-oauth.example.com/mcp")).toBe(true) }) + it("should initialize a static client with configured scopes and callback", async () => { + setupCallbackServerMock() + const provider = await McpOAuthClientProvider.create( + "https://example.com/mcp", + createMockSecretStorage(), + undefined, + { + clientId: "registered-client", + scope: "openid profile offline_access", + redirectUri: "http://localhost:33418/callback", + resource: false, + }, + ) + + expect(await provider.clientInformation()).toMatchObject({ + client_id: "registered-client", + redirect_uris: ["http://localhost:33418/callback"], + token_endpoint_auth_method: "none", + }) + expect(provider.redirectUrl).toBe("http://localhost:33418/callback") + + await provider.redirectToAuthorization( + new URL("https://auth.example.com/authorize?client_id=registered-client"), + ) + await provider.openBrowser() + const openedUri = vi.mocked(vscode.env.openExternal).mock.calls[0][0].toString() + expect(new URL(openedUri).searchParams.get("scope")).toBe("openid profile offline_access") + expect(startCallbackServer).toHaveBeenCalledWith(33418, expect.any(String)) + await provider.close() + }) + + it.each([ + "https://example.com:33418/callback", + "https://localhost:33418/callback", + "http://localhost:33418/wrong", + "http://localhost/callback", + ])("should reject unsafe or unusable configured callback %s", async (redirectUri) => { + await expect( + McpOAuthClientProvider.create("https://example.com/mcp", createMockSecretStorage(), undefined, { + clientId: "registered-client", + redirectUri, + }), + ).rejects.toThrow(/MCP OAuth redirectUri/) + }) }) describe("redirectUrl (pre-server-start)", () => { @@ -590,6 +634,45 @@ describe("McpOAuthClientProvider", () => { expect(scopeMatches).toBe(1) await provider.close() }) + it("should omit an SDK resource parameter when resource is false", async () => { + setupCallbackServerMock() + const provider = await McpOAuthClientProvider.create( + "https://example.com/mcp", + createMockSecretStorage(), + undefined, + { clientId: "registered-client", resource: false }, + ) + + const sdkUrl = new URL( + "https://auth.example.com/authorize?client_id=registered-client&resource=https%3A%2F%2Fexample.com%2F", + ) + await provider.redirectToAuthorization(sdkUrl) + await provider.openBrowser() + + const openedUri = vi.mocked(vscode.env.openExternal).mock.calls[0][0].toString() + expect(new URL(openedUri).searchParams.has("resource")).toBe(false) + await provider.close() + }) + + it("should replace the SDK resource parameter with an explicit configured value", async () => { + setupCallbackServerMock() + const provider = await McpOAuthClientProvider.create( + "https://example.com/mcp", + createMockSecretStorage(), + undefined, + { clientId: "registered-client", resource: "https://api.example.com/" }, + ) + + const sdkUrl = new URL( + "https://auth.example.com/authorize?client_id=registered-client&resource=https%3A%2F%2Fexample.com%2F", + ) + await provider.redirectToAuthorization(sdkUrl) + await provider.openBrowser() + + const openedUri = vi.mocked(vscode.env.openExternal).mock.calls[0][0].toString() + expect(new URL(openedUri).searchParams.get("resource")).toBe("https://api.example.com/") + await provider.close() + }) }) describe("exchangeCodeForTokens", () => { @@ -655,6 +738,43 @@ describe("McpOAuthClientProvider", () => { await provider.close() }) + it("should use the static client callback and omit resource when disabled", async () => { + const tokenResponse = { access_token: "access-token", token_type: "Bearer", expires_in: 3600 } + mockFetch + .mockResolvedValueOnce({ + ok: true, + json: () => + Promise.resolve({ + issuer: "https://auth.example.com", + authorization_endpoint: "https://auth.example.com/authorize", + token_endpoint: "https://auth.example.com/token", + token_endpoint_auth_methods_supported: ["none"], + grant_types_supported: ["authorization_code", "refresh_token"], + }), + }) + .mockResolvedValueOnce({ ok: true, json: () => Promise.resolve(tokenResponse) }) + const provider = await McpOAuthClientProvider.create( + "https://example.com/mcp", + createMockSecretStorage(), + undefined, + { + clientId: "registered-client", + redirectUri: "http://localhost:33418/callback", + resource: false, + }, + ) + await provider.saveCodeVerifier("verifier") + + await provider.exchangeCodeForTokens("authorization-code") + + const tokenCall = mockFetch.mock.calls.at(-1) + const body = new URLSearchParams(tokenCall?.[1]?.body as string) + expect(body.get("client_id")).toBe("registered-client") + expect(body.get("redirect_uri")).toBe("http://localhost:33418/callback") + expect(body.has("resource")).toBe(false) + await provider.close() + }) + it("should throw when no token_endpoint is available", async () => { ;(discoverOAuthProtectedResourceMetadata as any).mockResolvedValueOnce({ authorization_servers: ["https://auth.example.com"], @@ -801,6 +921,21 @@ describe("McpOAuthClientProvider", () => { }) describe("registerClientIfNeeded", () => { + it("should skip dynamic registration for a configured static client", async () => { + const provider = await McpOAuthClientProvider.create( + "https://example.com/mcp", + createMockSecretStorage(), + undefined, + { clientId: "registered-client" }, + ) + mockFetch.mockClear() + + await provider.registerClientIfNeeded() + + expect(mockFetch).not.toHaveBeenCalled() + expect((await provider.clientInformation())?.client_id).toBe("registered-client") + }) + it("should reuse cached client_info from previous registration", async () => { setupCallbackServerMock() const secretStorage = createMockSecretStorage() diff --git a/src/services/mcp/__tests__/SecretStorageService.spec.ts b/src/services/mcp/__tests__/SecretStorageService.spec.ts index 9a6833c4ab..862c0c578a 100644 --- a/src/services/mcp/__tests__/SecretStorageService.spec.ts +++ b/src/services/mcp/__tests__/SecretStorageService.spec.ts @@ -234,5 +234,25 @@ describe("SecretStorageService", () => { expect((await service.getOAuthData(url))?.tokens.access_token).toBe(`tok-${i}`) } }) + it("should isolate static OAuth clients for the same server URL", async () => { + await service.saveOAuthData( + "https://example.com/mcp", + { tokens: { access_token: "client-a-token", token_type: "Bearer" }, expires_at: 1 }, + "client-a", + ) + await service.saveOAuthData( + "https://example.com/mcp", + { tokens: { access_token: "client-b-token", token_type: "Bearer" }, expires_at: 2 }, + "client-b", + ) + + expect((await service.getOAuthData("https://example.com/mcp", "client-a"))?.tokens.access_token).toBe( + "client-a-token", + ) + expect((await service.getOAuthData("https://example.com/mcp", "client-b"))?.tokens.access_token).toBe( + "client-b-token", + ) + expect(await service.getOAuthData("https://example.com/mcp")).toBeUndefined() + }) }) }) From 9bcf50217995b22abc1eb8ac6a0899c61d722242 Mon Sep 17 00:00:00 2001 From: Jesse Quijano Date: Tue, 20 Oct 2026 22:08:19 -0700 Subject: [PATCH 2/7] fix(mcp): preserve SDK OAuth resource (#2096) --- src/services/mcp/McpOAuthClientProvider.ts | 8 +++++--- src/services/mcp/__tests__/McpHub.spec.ts | 11 ++++++++++- .../mcp/__tests__/McpOAuthClientProvider.spec.ts | 13 ++++++++----- 3 files changed, 23 insertions(+), 9 deletions(-) diff --git a/src/services/mcp/McpOAuthClientProvider.ts b/src/services/mcp/McpOAuthClientProvider.ts index c8a27b8c3c..5af5bf3abd 100644 --- a/src/services/mcp/McpOAuthClientProvider.ts +++ b/src/services/mcp/McpOAuthClientProvider.ts @@ -105,6 +105,7 @@ export class McpOAuthClientProvider implements OAuthClientProvider { private readonly _state: string, private readonly _authServerMeta: Record | null, private readonly _resourceIndicator: string | null, + private readonly _resourceDisabled: boolean, private readonly _clientName: string, private readonly _configuredRedirectUri?: string, private readonly _configuredClientId?: string, @@ -175,6 +176,7 @@ export class McpOAuthClientProvider implements OAuthClientProvider { state, authServerMeta, resourceIndicator, + options?.resource === false, serverName || "Roo Code", options?.redirectUri, options?.clientId, @@ -389,11 +391,11 @@ export class McpOAuthClientProvider implements OAuthClientProvider { // 1. Replace the origin+pathname with the real authorization_endpoint. // 2. Preserve all SDK-generated query params (client_id, code_challenge, etc.) // 3. Add `scope` when the server advertises scopes but the SDK omitted it. - // 4. Add RFC 8707 `resource` parameter when the protected resource metadata - // advertised a resource indicator. + // 4. Set an explicit or discovered RFC 8707 `resource` parameter. Preserve + // an SDK-generated value unless the configuration disables it. if (this._resourceIndicator) { authorizationUrl.searchParams.set("resource", this._resourceIndicator) - } else { + } else if (this._resourceDisabled) { authorizationUrl.searchParams.delete("resource") } let correctedUrl = authorizationUrl diff --git a/src/services/mcp/__tests__/McpHub.spec.ts b/src/services/mcp/__tests__/McpHub.spec.ts index 2486ff960b..e89883b759 100644 --- a/src/services/mcp/__tests__/McpHub.spec.ts +++ b/src/services/mcp/__tests__/McpHub.spec.ts @@ -1726,7 +1726,16 @@ describe("McpHub", () => { { type: "stdio", command: "test", oauth: { clientId: "client" } }, { type: "sse", url: "https://mcp.example.com/sse", oauth: { clientId: "client" } }, { type: "streamable-http", url: "https://mcp.example.com/mcp", oauth: { clientId: "" } }, - { type: "streamable-http", url: "https://mcp.example.com/mcp", oauth: { resource: "invalid" } }, + { + type: "streamable-http", + url: "https://mcp.example.com/mcp", + oauth: { clientId: "client", resource: "invalid" }, + }, + { + type: "streamable-http", + url: "https://mcp.example.com/mcp", + oauth: { clientId: "client", resource: true }, + }, { type: "streamable-http", url: "https://mcp.example.com/mcp", oauth: { resource: false } }, ])("rejects invalid OAuth configuration: $type", (config) => { expect(() => ServerConfigSchema.parse(config)).toThrow() diff --git a/src/services/mcp/__tests__/McpOAuthClientProvider.spec.ts b/src/services/mcp/__tests__/McpOAuthClientProvider.spec.ts index 8398ab1138..fb7e7c7724 100644 --- a/src/services/mcp/__tests__/McpOAuthClientProvider.spec.ts +++ b/src/services/mcp/__tests__/McpOAuthClientProvider.spec.ts @@ -602,20 +602,23 @@ describe("McpOAuthClientProvider", () => { await provider.close() }) - it("should not duplicate resource if the SDK already included it", async () => { + it("should preserve an SDK resource when provider discovery finds no resource", async () => { + ;(discoverOAuthProtectedResourceMetadata as any).mockResolvedValueOnce({ + authorization_servers: ["https://auth.example.com"], + }) setupCallbackServerMock() const provider = await McpOAuthClientProvider.create("https://example.com/mcp", createMockSecretStorage()) - // SDK URL already contains a resource param const sdkUrl = new URL( "https://auth.example.com/authorize?client_id=abc&resource=https%3A%2F%2Fexample.com%2F&state=123", ) await provider.redirectToAuthorization(sdkUrl) await provider.openBrowser() - const openedUri = (vscode.env.openExternal as any).mock.calls[0][0].toString() - const resourceMatches = (openedUri.match(/resource=/g) || []).length - expect(resourceMatches).toBe(1) + const openedUri = vi.mocked(vscode.env.openExternal).mock.calls[0][0].toString() + const openedUrl = new URL(openedUri) + expect(openedUrl.searchParams.get("resource")).toBe("https://example.com/") + expect(openedUrl.searchParams.getAll("resource")).toHaveLength(1) await provider.close() }) From cfdd03dfd077cda0153f516aff866a351d00b727 Mon Sep 17 00:00:00 2001 From: Jesse Quijano Date: Tue, 20 Oct 2026 22:38:00 -0700 Subject: [PATCH 3/7] fix(mcp): keep OAuth requests consistent (#2096) --- src/eslint-suppressions.json | 7 +- src/services/mcp/McpOAuthClientProvider.ts | 30 ++++-- .../__tests__/McpOAuthClientProvider.spec.ts | 99 +++++++++++++------ .../utils/__tests__/callbackServer.spec.ts | 22 ++++- src/services/mcp/utils/callbackServer.ts | 6 +- 5 files changed, 111 insertions(+), 53 deletions(-) diff --git a/src/eslint-suppressions.json b/src/eslint-suppressions.json index 1f1950c275..7cbfd72593 100644 --- a/src/eslint-suppressions.json +++ b/src/eslint-suppressions.json @@ -1501,7 +1501,7 @@ }, "services/mcp/__tests__/McpOAuthClientProvider.spec.ts": { "@typescript-eslint/no-explicit-any": { - "count": 20 + "count": 19 } }, "services/mcp/__tests__/SecretStorageService.spec.ts": { @@ -1509,11 +1509,6 @@ "count": 1 } }, - "services/mcp/utils/__tests__/callbackServer.spec.ts": { - "@typescript-eslint/no-explicit-any": { - "count": 4 - } - }, "services/mcp/utils/__tests__/oauth.spec.ts": { "@typescript-eslint/no-explicit-any": { "count": 9 diff --git a/src/services/mcp/McpOAuthClientProvider.ts b/src/services/mcp/McpOAuthClientProvider.ts index 5af5bf3abd..1737ac66cd 100644 --- a/src/services/mcp/McpOAuthClientProvider.ts +++ b/src/services/mcp/McpOAuthClientProvider.ts @@ -89,6 +89,8 @@ export class McpOAuthClientProvider implements OAuthClientProvider { private _refreshPromise: Promise | null = null /** Stored by redirectToAuthorization(); opened on-demand via openBrowser(). */ private _pendingAuthorizationUrl: URL | null = null + /** Resource used by the authorization request and subsequent token requests. */ + private _effectiveResourceIndicator: string | null /** Deduplicates concurrent _ensureCallbackServer() calls. */ private _ensureServerPromise: Promise | null = null @@ -109,7 +111,9 @@ export class McpOAuthClientProvider implements OAuthClientProvider { private readonly _clientName: string, private readonly _configuredRedirectUri?: string, private readonly _configuredClientId?: string, - ) {} + ) { + this._effectiveResourceIndicator = _resourceIndicator + } /** * Factory — discovers OAuth Authorization Server metadata once (RFC 9728 + @@ -184,7 +188,7 @@ export class McpOAuthClientProvider implements OAuthClientProvider { if (options?.clientId) { if (options.redirectUri) { - getCallbackPort(options.redirectUri) + getCallbackAddress(options.redirectUri) } else { // A static client skips DCR, which normally starts the callback server. // Start it here so the SDK never receives localhost:0 as redirect_uri. @@ -226,9 +230,11 @@ export class McpOAuthClientProvider implements OAuthClientProvider { private async _doStartCallbackServer(): Promise { this._closed = false + const { port: configuredPort, host } = getCallbackAddress(this._configuredRedirectUri) const { server, port, result, cancel } = await startCallbackServer( - getCallbackPort(this._configuredRedirectUri) ?? this._port, + configuredPort ?? this._port, this._state, + host, ) this._server = server this._port = port @@ -395,8 +401,12 @@ export class McpOAuthClientProvider implements OAuthClientProvider { // an SDK-generated value unless the configuration disables it. if (this._resourceIndicator) { authorizationUrl.searchParams.set("resource", this._resourceIndicator) + this._effectiveResourceIndicator = this._resourceIndicator } else if (this._resourceDisabled) { authorizationUrl.searchParams.delete("resource") + this._effectiveResourceIndicator = null + } else { + this._effectiveResourceIndicator = authorizationUrl.searchParams.get("resource") } let correctedUrl = authorizationUrl if (this._authServerMeta?.authorization_endpoint) { @@ -508,8 +518,8 @@ export class McpOAuthClientProvider implements OAuthClientProvider { // RFC 8707: include resource indicator so servers that bind token requests // to a specific resource can validate the exchange. - if (this._resourceIndicator) { - params.resource = this._resourceIndicator + if (this._effectiveResourceIndicator) { + params.resource = this._effectiveResourceIndicator } // Include client_secret in the body when the auth method is client_secret_post. @@ -560,8 +570,8 @@ export class McpOAuthClientProvider implements OAuthClientProvider { } // RFC 8707: include resource indicator in refresh requests too. - if (this._resourceIndicator) { - params.resource = this._resourceIndicator + if (this._effectiveResourceIndicator) { + params.resource = this._effectiveResourceIndicator } if (this._tokenEndpointAuthMethod === "client_secret_post" && this._clientInfo?.client_secret) { @@ -610,8 +620,8 @@ function resolveResourceIndicator(configured: McpOAuthResource | undefined, disc return discovered ?? null } -function getCallbackPort(redirectUri?: string): number | undefined { - if (!redirectUri) return undefined +function getCallbackAddress(redirectUri?: string): { port?: number; host: "localhost" | "127.0.0.1" } { + if (!redirectUri) return { host: "localhost" } const url = new URL(redirectUri) if (url.hostname !== "localhost" && url.hostname !== "127.0.0.1") { throw new Error("MCP OAuth redirectUri must use a loopback host") @@ -629,5 +639,5 @@ function getCallbackPort(redirectUri?: string): number | undefined { if (!Number.isInteger(port) || port < 1 || port > 65535) { throw new Error("MCP OAuth redirectUri must specify a valid port") } - return port + return { port, host: url.hostname } } diff --git a/src/services/mcp/__tests__/McpOAuthClientProvider.spec.ts b/src/services/mcp/__tests__/McpOAuthClientProvider.spec.ts index fb7e7c7724..8d0c77d376 100644 --- a/src/services/mcp/__tests__/McpOAuthClientProvider.spec.ts +++ b/src/services/mcp/__tests__/McpOAuthClientProvider.spec.ts @@ -168,36 +168,42 @@ describe("McpOAuthClientProvider", () => { expect(provider.hasMetadata).toBe(false) expect(McpOAuthClientProvider.isKnownNonOAuth("https://no-oauth.example.com/mcp")).toBe(true) }) - it("should initialize a static client with configured scopes and callback", async () => { - setupCallbackServerMock() - const provider = await McpOAuthClientProvider.create( - "https://example.com/mcp", - createMockSecretStorage(), - undefined, - { - clientId: "registered-client", - scope: "openid profile offline_access", - redirectUri: "http://localhost:33418/callback", - resource: false, - }, - ) + it.each([ + ["http://localhost:33418/callback", "localhost"], + ["http://127.0.0.1:33418/callback", "127.0.0.1"], + ] as const)( + "should initialize a static client with configured scopes and callback %s", + async (redirectUri, callbackHost) => { + setupCallbackServerMock() + const provider = await McpOAuthClientProvider.create( + "https://example.com/mcp", + createMockSecretStorage(), + undefined, + { + clientId: "registered-client", + scope: "openid profile offline_access", + redirectUri, + resource: false, + }, + ) - expect(await provider.clientInformation()).toMatchObject({ - client_id: "registered-client", - redirect_uris: ["http://localhost:33418/callback"], - token_endpoint_auth_method: "none", - }) - expect(provider.redirectUrl).toBe("http://localhost:33418/callback") + expect(await provider.clientInformation()).toMatchObject({ + client_id: "registered-client", + redirect_uris: [redirectUri], + token_endpoint_auth_method: "none", + }) + expect(provider.redirectUrl).toBe(redirectUri) - await provider.redirectToAuthorization( - new URL("https://auth.example.com/authorize?client_id=registered-client"), - ) - await provider.openBrowser() - const openedUri = vi.mocked(vscode.env.openExternal).mock.calls[0][0].toString() - expect(new URL(openedUri).searchParams.get("scope")).toBe("openid profile offline_access") - expect(startCallbackServer).toHaveBeenCalledWith(33418, expect.any(String)) - await provider.close() - }) + await provider.redirectToAuthorization( + new URL("https://auth.example.com/authorize?client_id=registered-client"), + ) + await provider.openBrowser() + const openedUri = vi.mocked(vscode.env.openExternal).mock.calls[0][0].toString() + expect(new URL(openedUri).searchParams.get("scope")).toBe("openid profile offline_access") + expect(startCallbackServer).toHaveBeenCalledWith(33418, expect.any(String), callbackHost) + await provider.close() + }, + ) it.each([ "https://example.com:33418/callback", @@ -603,22 +609,51 @@ describe("McpOAuthClientProvider", () => { }) it("should preserve an SDK resource when provider discovery finds no resource", async () => { - ;(discoverOAuthProtectedResourceMetadata as any).mockResolvedValueOnce({ + vi.mocked(discoverOAuthProtectedResourceMetadata).mockResolvedValueOnce({ + resource: "", authorization_servers: ["https://auth.example.com"], }) + mockFetch + .mockResolvedValueOnce({ + ok: true, + json: () => + Promise.resolve({ + issuer: "https://auth.example.com", + authorization_endpoint: "https://auth.example.com/authorize", + token_endpoint: "https://auth.example.com/token", + token_endpoint_auth_methods_supported: ["none"], + grant_types_supported: ["authorization_code"], + }), + }) + .mockResolvedValueOnce({ + ok: true, + json: () => Promise.resolve({ access_token: "access-token", token_type: "Bearer" }), + }) setupCallbackServerMock() - const provider = await McpOAuthClientProvider.create("https://example.com/mcp", createMockSecretStorage()) + const provider = await McpOAuthClientProvider.create( + "https://example.com/mcp", + createMockSecretStorage(), + undefined, + { clientId: "registered-client" }, + ) + const resource = "https://example.com/" const sdkUrl = new URL( - "https://auth.example.com/authorize?client_id=abc&resource=https%3A%2F%2Fexample.com%2F&state=123", + `https://auth.example.com/authorize?client_id=registered-client&resource=${encodeURIComponent(resource)}&state=123`, ) await provider.redirectToAuthorization(sdkUrl) await provider.openBrowser() + await provider.saveCodeVerifier("verifier") + await provider.exchangeCodeForTokens("authorization-code") const openedUri = vi.mocked(vscode.env.openExternal).mock.calls[0][0].toString() const openedUrl = new URL(openedUri) - expect(openedUrl.searchParams.get("resource")).toBe("https://example.com/") + expect(openedUrl.searchParams.get("resource")).toBe(resource) expect(openedUrl.searchParams.getAll("resource")).toHaveLength(1) + + const tokenCall = mockFetch.mock.calls.at(-1) + const tokenBody = new URLSearchParams(tokenCall?.[1]?.body as string) + expect(tokenBody.get("resource")).toBe(resource) await provider.close() }) diff --git a/src/services/mcp/utils/__tests__/callbackServer.spec.ts b/src/services/mcp/utils/__tests__/callbackServer.spec.ts index e2ceb48723..10b5025520 100644 --- a/src/services/mcp/utils/__tests__/callbackServer.spec.ts +++ b/src/services/mcp/utils/__tests__/callbackServer.spec.ts @@ -39,13 +39,14 @@ describe("startCallbackServer", () => { it("should start server and resolve with callback result", async () => { const mockServer = createMockServer() - ;(http.createServer as any).mockReturnValue(mockServer) + vi.mocked(http.createServer).mockReturnValue(mockServer as unknown as http.Server) const promise = startCallbackServer() const { server, port, result } = await promise expect(port).toBe(3000) expect(server).toBe(mockServer) + expect(mockServer.listen).toHaveBeenCalledWith(0, "localhost", expect.any(Function)) // Simulate callback request const requestCall = mockServer.on.mock.calls.find((call) => call[0] === "request") @@ -70,10 +71,23 @@ describe("startCallbackServer", () => { expect(mockRes.end).toHaveBeenCalledWith(expect.stringContaining('')) }) + it("should bind to the configured loopback host", async () => { + const mockServer = createMockServer() + + vi.mocked(http.createServer).mockReturnValue(mockServer as unknown as http.Server) + + const { result, cancel } = await startCallbackServer(33418, undefined, "127.0.0.1") + const cancellation = expect(result).rejects.toThrow("Callback cancelled") + cancel() + + expect(mockServer.listen).toHaveBeenCalledWith(33418, "127.0.0.1", expect.any(Function)) + await cancellation + }) + it("should reject invalid state", async () => { const mockServer = createMockServer() - ;(http.createServer as any).mockReturnValue(mockServer) + vi.mocked(http.createServer).mockReturnValue(mockServer as unknown as http.Server) const promise = startCallbackServer(undefined, "expected-state") const { result } = await promise @@ -106,7 +120,7 @@ describe("stopCallbackServer", () => { close: vi.fn((callback) => callback()), } - await stopCallbackServer(mockServer as any, () => {}) + await stopCallbackServer(mockServer as unknown as http.Server, () => {}) expect(mockServer.close).toHaveBeenCalled() }) @@ -116,7 +130,7 @@ describe("stopCallbackServer", () => { } const cancel = vi.fn() - await stopCallbackServer(mockServer as any, cancel) + await stopCallbackServer(mockServer as unknown as http.Server, cancel) expect(cancel).toHaveBeenCalledTimes(1) expect(mockServer.close).toHaveBeenCalled() }) diff --git a/src/services/mcp/utils/callbackServer.ts b/src/services/mcp/utils/callbackServer.ts index 1b49195495..0a63b27c65 100644 --- a/src/services/mcp/utils/callbackServer.ts +++ b/src/services/mcp/utils/callbackServer.ts @@ -12,15 +12,19 @@ export interface CallbackResult { state?: string } +type CallbackHost = "localhost" | "127.0.0.1" + /** * Starts a local HTTP server to handle OAuth callback. * @param port Optional port to use (defaults to random available port) * @param expectedState Optional expected state for CSRF protection + * @param host Loopback host used by the OAuth redirect URI * @returns Promise<{server: http.Server, port: number, result: Promise}> */ export function startCallbackServer( port?: number, expectedState?: string, + host: CallbackHost = "localhost", ): Promise<{ server: http.Server port: number @@ -43,7 +47,7 @@ export function startCallbackServer( return new Promise((resolve, reject) => { const server = http.createServer() - server.listen(port || 0, "127.0.0.1", () => { + server.listen(port || 0, host, () => { const address = server.address() if (!address || typeof address === "string") { reject(new Error("Failed to get server address")) From 3589f5f1f3b8375354347ace0cee48d9d62a0737 Mon Sep 17 00:00:00 2001 From: Jesse Quijano Date: Wed, 21 Oct 2026 06:05:14 -0700 Subject: [PATCH 4/7] fix(mcp): persist OAuth request constraints (#2096) --- src/services/mcp/McpOAuthClientProvider.ts | 53 +++- src/services/mcp/SecretStorageService.ts | 5 + .../__tests__/McpOAuthClientProvider.spec.ts | 260 +++++++++++++++++- .../__tests__/SecretStorageService.spec.ts | 4 +- 4 files changed, 300 insertions(+), 22 deletions(-) diff --git a/src/services/mcp/McpOAuthClientProvider.ts b/src/services/mcp/McpOAuthClientProvider.ts index 1737ac66cd..1d44ba3d01 100644 --- a/src/services/mcp/McpOAuthClientProvider.ts +++ b/src/services/mcp/McpOAuthClientProvider.ts @@ -104,6 +104,7 @@ export class McpOAuthClientProvider implements OAuthClientProvider { private readonly _tokenEndpointAuthMethod: string, private readonly _grantTypes: McpOAuthGrantType[], private readonly _scopes: string[], + private readonly _scopeConfigured: boolean, private readonly _state: string, private readonly _authServerMeta: Record | null, private readonly _resourceIndicator: string | null, @@ -154,10 +155,18 @@ export class McpOAuthClientProvider implements OAuthClientProvider { const authMethods: string[] = authServerMeta?.token_endpoint_auth_methods_supported ?? [] const tokenEndpointAuthMethod = authMethods.includes("none") ? "none" : "client_secret_post" const grantTypes = selectMcpOAuthGrantTypes(authServerMeta?.grant_types_supported) - const scopes = - options?.scope !== undefined - ? options.scope.split(/\s+/).filter((scope) => scope.length > 0) - : (authServerMeta?.scopes_supported ?? []) + const configuredScope = options?.scope + const scopeConfigured = configuredScope !== undefined + const scopes = scopeConfigured + ? configuredScope.split(/\s+/).filter((scope) => scope.length > 0) + : (authServerMeta?.scopes_supported ?? []) + + const storedData = await secretStorage.getOAuthData(serverUrl, options?.clientId) + const issuerMatches = typeof storedData?.issuer === "string" && storedData.issuer === authServerMeta?.issuer + const storedResourceIndicator = + !resourceIndicator && options?.resource !== false && issuerMatches + ? storedData?.resource_indicator + : undefined // Generate a CSRF state token for the OAuth flow. const state = Array.from(crypto.getRandomValues(new Uint8Array(16))) @@ -177,6 +186,7 @@ export class McpOAuthClientProvider implements OAuthClientProvider { tokenEndpointAuthMethod, grantTypes, scopes, + scopeConfigured, state, authServerMeta, resourceIndicator, @@ -185,6 +195,9 @@ export class McpOAuthClientProvider implements OAuthClientProvider { options?.redirectUri, options?.clientId, ) + if (storedResourceIndicator) { + provider._effectiveResourceIndicator = storedResourceIndicator + } if (options?.clientId) { if (options.redirectUri) { @@ -335,6 +348,12 @@ export class McpOAuthClientProvider implements OAuthClientProvider { await this._secretStorage.deleteOAuthData(this._serverUrl, this._configuredClientId) return undefined } + + // Restore the authorization resource after a restart only when the current + // configuration or discovery did not already select one. + if (!this._effectiveResourceIndicator && !this._resourceDisabled) { + this._effectiveResourceIndicator = data.resource_indicator ?? null + } if (this._refreshPromise) { return this._refreshPromise } @@ -376,6 +395,7 @@ export class McpOAuthClientProvider implements OAuthClientProvider { tokens, expires_at, ...(this._authServerMeta?.issuer ? { issuer: this._authServerMeta.issuer as string } : {}), + ...(this._effectiveResourceIndicator ? { resource_indicator: this._effectiveResourceIndicator } : {}), ...(clientInfo ? { client_info: clientInfo } : {}), }, this._configuredClientId, @@ -395,8 +415,8 @@ export class McpOAuthClientProvider implements OAuthClientProvider { // with path components, causing it to fall back to a default "/authorize" path. // We correct the URL using our pre-fetched metadata: // 1. Replace the origin+pathname with the real authorization_endpoint. - // 2. Preserve all SDK-generated query params (client_id, code_challenge, etc.) - // 3. Add `scope` when the server advertises scopes but the SDK omitted it. + // 2. Preserve all SDK-generated query params (client_id, code_challenge, etc.). + // 3. Override SDK scopes when the configuration explicitly supplies a scope. // 4. Set an explicit or discovered RFC 8707 `resource` parameter. Preserve // an SDK-generated value unless the configuration disables it. if (this._resourceIndicator) { @@ -406,7 +426,12 @@ export class McpOAuthClientProvider implements OAuthClientProvider { authorizationUrl.searchParams.delete("resource") this._effectiveResourceIndicator = null } else { - this._effectiveResourceIndicator = authorizationUrl.searchParams.get("resource") + const sdkResourceIndicator = authorizationUrl.searchParams.get("resource") + if (sdkResourceIndicator) { + this._effectiveResourceIndicator = sdkResourceIndicator + } else if (this._effectiveResourceIndicator) { + authorizationUrl.searchParams.set("resource", this._effectiveResourceIndicator) + } } let correctedUrl = authorizationUrl if (this._authServerMeta?.authorization_endpoint) { @@ -427,16 +452,22 @@ export class McpOAuthClientProvider implements OAuthClientProvider { authorizationUrl.searchParams.forEach((value, key) => { fixed.searchParams.set(key, value) }) - // Ensure the scope param is present — the SDK sometimes omits it - if (!fixed.searchParams.has("scope") && this._scopes.length > 0) { - fixed.searchParams.set("scope", this._scopes.join(" ")) - } correctedUrl = fixed } catch { // Fall through and use the original URL if correction fails } } + if (this._scopeConfigured) { + if (this._scopes.length > 0) { + correctedUrl.searchParams.set("scope", this._scopes.join(" ")) + } else { + correctedUrl.searchParams.delete("scope") + } + } else if (!correctedUrl.searchParams.has("scope") && this._scopes.length > 0) { + correctedUrl.searchParams.set("scope", this._scopes.join(" ")) + } + // Store the (possibly corrected) URL; it will be opened by openBrowser() // once the user confirms the "Authenticate" toast in McpHub. this._pendingAuthorizationUrl = correctedUrl diff --git a/src/services/mcp/SecretStorageService.ts b/src/services/mcp/SecretStorageService.ts index 11eb3f4d01..342dc62665 100644 --- a/src/services/mcp/SecretStorageService.ts +++ b/src/services/mcp/SecretStorageService.ts @@ -10,6 +10,11 @@ export interface StoredMcpOAuthData { * allowed only against the same issuer. Missing on data saved by older versions. */ issuer?: string + /** + * RFC 8707 resource used during authorization and token requests. This keeps + * refresh requests consistent after the extension restarts. + */ + resource_indicator?: string /** * Full DCR response from the auth server, persisted so that fields like * client_secret, grant_types, and token_endpoint_auth_method survive restarts. diff --git a/src/services/mcp/__tests__/McpOAuthClientProvider.spec.ts b/src/services/mcp/__tests__/McpOAuthClientProvider.spec.ts index 8d0c77d376..f53d09815d 100644 --- a/src/services/mcp/__tests__/McpOAuthClientProvider.spec.ts +++ b/src/services/mcp/__tests__/McpOAuthClientProvider.spec.ts @@ -55,20 +55,39 @@ import * as vscode from "vscode" function createMockSecretStorage(): SecretStorageService { const store = new Map() + const key = (url: string, clientId?: string) => `${url}:${clientId ?? "dynamic"}` return { - getOAuthData: vi.fn(async (url: string) => { - const raw = store.get(url) + getOAuthData: vi.fn(async (url: string, clientId?: string) => { + const raw = store.get(key(url, clientId)) return raw ? JSON.parse(raw) : undefined }), - saveOAuthData: vi.fn(async (url: string, data: any) => { - store.set(url, JSON.stringify(data)) + saveOAuthData: vi.fn(async (url: string, data: any, clientId?: string) => { + store.set(key(url, clientId), JSON.stringify(data)) }), - deleteOAuthData: vi.fn(async (url: string) => { - store.delete(url) + deleteOAuthData: vi.fn(async (url: string, clientId?: string) => { + store.delete(key(url, clientId)) }), } as unknown as SecretStorageService } +function mockOAuthDiscovery(resource = "") { + vi.mocked(discoverOAuthProtectedResourceMetadata).mockResolvedValueOnce({ + resource, + authorization_servers: ["https://auth.example.com"], + }) + mockFetch.mockResolvedValueOnce({ + ok: true, + json: () => + Promise.resolve({ + issuer: "https://auth.example.com", + authorization_endpoint: "https://auth.example.com/authorize", + token_endpoint: "https://auth.example.com/token", + token_endpoint_auth_methods_supported: ["none"], + grant_types_supported: ["authorization_code", "refresh_token"], + }), + }) +} + function setupCallbackServerMock(code = "test-auth-code", state?: string) { const mockServer = { close: vi.fn((cb: () => void) => cb()) } const resultPromise = Promise.resolve({ code, state }) @@ -444,6 +463,190 @@ describe("McpOAuthClientProvider", () => { await provider.close() }) + it("should restore an SDK resource for refresh after creating a new provider", async () => { + setupCallbackServerMock() + const secretStorage = createMockSecretStorage() + const serverUrl = "https://example.com/mcp" + const clientId = "registered-client" + const resource = "https://api.example.com/" + mockOAuthDiscovery() + const initialProvider = await McpOAuthClientProvider.create(serverUrl, secretStorage, undefined, { + clientId, + }) + + await initialProvider.redirectToAuthorization( + new URL( + `https://auth.example.com/authorize?client_id=${clientId}&resource=${encodeURIComponent(resource)}`, + ), + ) + await initialProvider.saveTokens({ + access_token: "expired-access", + refresh_token: "valid-refresh", + token_type: "Bearer", + expires_in: -1, + }) + await initialProvider.close() + + mockOAuthDiscovery() + const restartedProvider = await McpOAuthClientProvider.create(serverUrl, secretStorage, undefined, { + clientId, + }) + mockFetch.mockClear() + mockFetch.mockResolvedValueOnce({ + ok: true, + json: () => Promise.resolve({ access_token: "new-access", token_type: "Bearer", expires_in: 3600 }), + }) + + await restartedProvider.tokens() + + const refreshCall = mockFetch.mock.calls.at(-1) + const refreshBody = new URLSearchParams(refreshCall?.[1]?.body as string) + expect(refreshBody.get("grant_type")).toBe("refresh_token") + expect(refreshBody.get("resource")).toBe(resource) + await restartedProvider.close() + }) + + it("should prefer a discovered resource over a stored resource after restart", async () => { + setupCallbackServerMock() + const secretStorage = createMockSecretStorage() + const serverUrl = "https://example.com/mcp" + const clientId = "registered-client" + const discoveredResource = "https://discovered.example.com/" + await secretStorage.saveOAuthData( + serverUrl, + { + tokens: { + access_token: "expired-access", + refresh_token: "valid-refresh", + token_type: "Bearer", + }, + expires_at: Date.now() - 1000, + issuer: "https://auth.example.com", + resource_indicator: "https://stored.example.com/", + }, + clientId, + ) + mockOAuthDiscovery(discoveredResource) + const provider = await McpOAuthClientProvider.create(serverUrl, secretStorage, undefined, { clientId }) + mockFetch.mockClear() + mockFetch.mockResolvedValueOnce({ + ok: true, + json: () => Promise.resolve({ access_token: "new-access", token_type: "Bearer", expires_in: 3600 }), + }) + + await provider.tokens() + + const refreshCall = mockFetch.mock.calls.at(-1) + const refreshBody = new URLSearchParams(refreshCall?.[1]?.body as string) + expect(refreshBody.get("resource")).toBe(discoveredResource) + await provider.close() + }) + + it("should prefer an explicit resource over a stored resource after restart", async () => { + setupCallbackServerMock() + const secretStorage = createMockSecretStorage() + const serverUrl = "https://example.com/mcp" + const clientId = "registered-client" + const explicitResource = "https://configured.example.com/" + await secretStorage.saveOAuthData( + serverUrl, + { + tokens: { + access_token: "expired-access", + refresh_token: "valid-refresh", + token_type: "Bearer", + }, + expires_at: Date.now() - 1000, + issuer: "https://auth.example.com", + resource_indicator: "https://stored.example.com/", + }, + clientId, + ) + mockOAuthDiscovery("https://discovered.example.com/") + const provider = await McpOAuthClientProvider.create(serverUrl, secretStorage, undefined, { + clientId, + resource: explicitResource, + }) + mockFetch.mockClear() + mockFetch.mockResolvedValueOnce({ + ok: true, + json: () => Promise.resolve({ access_token: "new-access", token_type: "Bearer", expires_in: 3600 }), + }) + + await provider.tokens() + + const refreshCall = mockFetch.mock.calls.at(-1) + const refreshBody = new URLSearchParams(refreshCall?.[1]?.body as string) + expect(refreshBody.get("resource")).toBe(explicitResource) + await provider.close() + }) + + it("should not restore a stored resource when resource is disabled", async () => { + setupCallbackServerMock() + const secretStorage = createMockSecretStorage() + const serverUrl = "https://example.com/mcp" + const clientId = "registered-client" + await secretStorage.saveOAuthData( + serverUrl, + { + tokens: { + access_token: "expired-access", + refresh_token: "valid-refresh", + token_type: "Bearer", + }, + expires_at: Date.now() - 1000, + issuer: "https://auth.example.com", + resource_indicator: "https://stored.example.com/", + }, + clientId, + ) + mockOAuthDiscovery() + const provider = await McpOAuthClientProvider.create(serverUrl, secretStorage, undefined, { + clientId, + resource: false, + }) + mockFetch.mockClear() + mockFetch.mockResolvedValueOnce({ + ok: true, + json: () => Promise.resolve({ access_token: "new-access", token_type: "Bearer", expires_in: 3600 }), + }) + + await provider.tokens() + + const refreshCall = mockFetch.mock.calls.at(-1) + const refreshBody = new URLSearchParams(refreshCall?.[1]?.body as string) + expect(refreshBody.has("resource")).toBe(false) + await provider.close() + }) + + it("should not restore a stored resource from a different issuer", async () => { + setupCallbackServerMock() + const secretStorage = createMockSecretStorage() + const serverUrl = "https://example.com/mcp" + const clientId = "registered-client" + await secretStorage.saveOAuthData( + serverUrl, + { + tokens: { + access_token: "expired-access", + refresh_token: "valid-refresh", + token_type: "Bearer", + }, + expires_at: Date.now() - 1000, + issuer: "https://other.example.com", + resource_indicator: "https://stored.example.com/", + }, + clientId, + ) + mockOAuthDiscovery() + const provider = await McpOAuthClientProvider.create(serverUrl, secretStorage, undefined, { clientId }) + mockFetch.mockClear() + + expect(await provider.tokens()).toBeUndefined() + expect(mockFetch).not.toHaveBeenCalled() + expect(await secretStorage.getOAuthData(serverUrl, clientId)).toBeUndefined() + }) + it("should return undefined for expired tokens without refresh token", async () => { setupCallbackServerMock() const secretStorage = createMockSecretStorage() @@ -661,15 +864,52 @@ describe("McpOAuthClientProvider", () => { setupCallbackServerMock() const provider = await McpOAuthClientProvider.create("https://example.com/mcp", createMockSecretStorage()) - // SDK URL already includes scope=openid const sdkUrl = new URL("https://auth.example.com/authorize?client_id=abc&scope=openid&state=123") await provider.redirectToAuthorization(sdkUrl) await provider.openBrowser() - // scope should appear exactly once const openedUri = (vscode.env.openExternal as any).mock.calls[0][0].toString() - const scopeMatches = (openedUri.match(/scope=/g) || []).length - expect(scopeMatches).toBe(1) + const openedUrl = new URL(openedUri) + expect(openedUrl.searchParams.get("scope")).toBe("openid") + expect(openedUrl.searchParams.getAll("scope")).toHaveLength(1) + await provider.close() + }) + + it("should replace an SDK scope with the configured static scope", async () => { + setupCallbackServerMock() + const provider = await McpOAuthClientProvider.create( + "https://example.com/mcp", + createMockSecretStorage(), + undefined, + { clientId: "registered-client", scope: "openid offline_access" }, + ) + + await provider.redirectToAuthorization( + new URL("https://auth.example.com/authorize?client_id=registered-client&scope=openid+admin"), + ) + await provider.openBrowser() + + const openedUri = vi.mocked(vscode.env.openExternal).mock.calls[0][0].toString() + expect(new URL(openedUri).searchParams.get("scope")).toBe("openid offline_access") + await provider.close() + }) + + it("should remove an SDK scope when the configured static scope is empty", async () => { + setupCallbackServerMock() + const provider = await McpOAuthClientProvider.create( + "https://example.com/mcp", + createMockSecretStorage(), + undefined, + { clientId: "registered-client", scope: "" }, + ) + + await provider.redirectToAuthorization( + new URL("https://auth.example.com/authorize?client_id=registered-client&scope=openid+admin"), + ) + await provider.openBrowser() + + const openedUri = vi.mocked(vscode.env.openExternal).mock.calls[0][0].toString() + expect(new URL(openedUri).searchParams.has("scope")).toBe(false) await provider.close() }) it("should omit an SDK resource parameter when resource is false", async () => { diff --git a/src/services/mcp/__tests__/SecretStorageService.spec.ts b/src/services/mcp/__tests__/SecretStorageService.spec.ts index 862c0c578a..940e0975e6 100644 --- a/src/services/mcp/__tests__/SecretStorageService.spec.ts +++ b/src/services/mcp/__tests__/SecretStorageService.spec.ts @@ -164,10 +164,11 @@ describe("SecretStorageService", () => { }) describe("client_info round-trip", () => { - it("should persist and retrieve full client_info", async () => { + it("should persist and retrieve full client_info and resource indicator", async () => { const data: StoredMcpOAuthData = { tokens: { access_token: "tok", token_type: "Bearer" }, expires_at: Date.now() + 3600_000, + resource_indicator: "https://api.example.com/", client_info: { client_id: "cid-123", client_secret: "secret-456", @@ -182,6 +183,7 @@ describe("SecretStorageService", () => { const result = await service.getOAuthData("https://example.com/mcp") expect(result).toEqual(data) + expect(result?.resource_indicator).toBe("https://api.example.com/") expect(result?.client_info?.client_id).toBe("cid-123") expect(result?.client_info?.client_secret).toBe("secret-456") expect(result?.client_info?.token_endpoint_auth_method).toBe("client_secret_post") From ec652a71cf49f68f7aaa2a1ff4374a9c22f92739 Mon Sep 17 00:00:00 2001 From: Jesse Quijano Date: Wed, 21 Oct 2026 06:19:52 -0700 Subject: [PATCH 5/7] test(mcp): cover OAuth creation constraints (#2096) --- src/eslint-suppressions.json | 2 +- .../__tests__/McpOAuthClientProvider.spec.ts | 72 +++++++++++++++++-- 2 files changed, 69 insertions(+), 5 deletions(-) diff --git a/src/eslint-suppressions.json b/src/eslint-suppressions.json index 7cbfd72593..d9bcda667f 100644 --- a/src/eslint-suppressions.json +++ b/src/eslint-suppressions.json @@ -1501,7 +1501,7 @@ }, "services/mcp/__tests__/McpOAuthClientProvider.spec.ts": { "@typescript-eslint/no-explicit-any": { - "count": 19 + "count": 18 } }, "services/mcp/__tests__/SecretStorageService.spec.ts": { diff --git a/src/services/mcp/__tests__/McpOAuthClientProvider.spec.ts b/src/services/mcp/__tests__/McpOAuthClientProvider.spec.ts index f53d09815d..9bc116e2ee 100644 --- a/src/services/mcp/__tests__/McpOAuthClientProvider.spec.ts +++ b/src/services/mcp/__tests__/McpOAuthClientProvider.spec.ts @@ -48,7 +48,7 @@ mockFetch.mockResolvedValue({ }) import { McpOAuthClientProvider } from "../McpOAuthClientProvider" -import { SecretStorageService } from "../SecretStorageService" +import { SecretStorageService, StoredMcpOAuthData } from "../SecretStorageService" import { startCallbackServer, stopCallbackServer } from "../utils/callbackServer" import { discoverOAuthProtectedResourceMetadata } from "@modelcontextprotocol/sdk/client/auth.js" import * as vscode from "vscode" @@ -61,7 +61,7 @@ function createMockSecretStorage(): SecretStorageService { const raw = store.get(key(url, clientId)) return raw ? JSON.parse(raw) : undefined }), - saveOAuthData: vi.fn(async (url: string, data: any, clientId?: string) => { + saveOAuthData: vi.fn(async (url: string, data: StoredMcpOAuthData, clientId?: string) => { store.set(key(url, clientId), JSON.stringify(data)) }), deleteOAuthData: vi.fn(async (url: string, clientId?: string) => { @@ -875,13 +875,13 @@ describe("McpOAuthClientProvider", () => { await provider.close() }) - it("should replace an SDK scope with the configured static scope", async () => { + it("should normalize and replace an SDK scope with the configured static scope", async () => { setupCallbackServerMock() const provider = await McpOAuthClientProvider.create( "https://example.com/mcp", createMockSecretStorage(), undefined, - { clientId: "registered-client", scope: "openid offline_access" }, + { clientId: "registered-client", scope: " openid \t offline_access " }, ) await provider.redirectToAuthorization( @@ -894,6 +894,41 @@ describe("McpOAuthClientProvider", () => { await provider.close() }) + it("should use advertised scopes for a static client when scope is not configured", async () => { + setupCallbackServerMock() + vi.mocked(discoverOAuthProtectedResourceMetadata).mockResolvedValueOnce({ + resource: "https://example.com/", + authorization_servers: ["https://auth.example.com"], + }) + mockFetch.mockResolvedValueOnce({ + ok: true, + json: () => + Promise.resolve({ + issuer: "https://auth.example.com", + authorization_endpoint: "https://auth.example.com/authorize", + token_endpoint: "https://auth.example.com/token", + token_endpoint_auth_methods_supported: ["none"], + grant_types_supported: ["authorization_code"], + scopes_supported: ["openid", "profile"], + }), + }) + const provider = await McpOAuthClientProvider.create( + "https://example.com/mcp", + createMockSecretStorage(), + undefined, + { clientId: "registered-client" }, + ) + + await provider.redirectToAuthorization( + new URL("https://auth.example.com/authorize?client_id=registered-client"), + ) + await provider.openBrowser() + + const openedUri = vi.mocked(vscode.env.openExternal).mock.calls[0][0].toString() + expect(new URL(openedUri).searchParams.get("scope")).toBe("openid profile") + await provider.close() + }) + it("should remove an SDK scope when the configured static scope is empty", async () => { setupCallbackServerMock() const provider = await McpOAuthClientProvider.create( @@ -912,6 +947,35 @@ describe("McpOAuthClientProvider", () => { expect(new URL(openedUri).searchParams.has("scope")).toBe(false) await provider.close() }) + + it("should omit a stored resource from another issuer during authorization", async () => { + setupCallbackServerMock() + const secretStorage = createMockSecretStorage() + const serverUrl = "https://example.com/mcp" + const clientId = "registered-client" + await secretStorage.saveOAuthData( + serverUrl, + { + tokens: { access_token: "access-token", token_type: "Bearer" }, + expires_at: Date.now() + 3600_000, + issuer: "https://other.example.com", + resource_indicator: "https://stored.example.com/", + }, + clientId, + ) + mockOAuthDiscovery() + const provider = await McpOAuthClientProvider.create(serverUrl, secretStorage, undefined, { clientId }) + + await provider.redirectToAuthorization( + new URL("https://auth.example.com/authorize?client_id=registered-client"), + ) + await provider.openBrowser() + + const openedUri = vi.mocked(vscode.env.openExternal).mock.calls[0][0].toString() + expect(new URL(openedUri).searchParams.has("resource")).toBe(false) + await provider.close() + }) + it("should omit an SDK resource parameter when resource is false", async () => { setupCallbackServerMock() const provider = await McpOAuthClientProvider.create( From 8cb827e5b490973fd427d4788652f7e48466f1ae Mon Sep 17 00:00:00 2001 From: Jesse Quijano Date: Wed, 21 Oct 2026 06:45:05 -0700 Subject: [PATCH 6/7] fix(mcp): close OAuth lifecycle gaps (#2096) --- src/services/mcp/McpHub.ts | 9 ++ src/services/mcp/McpOAuthClientProvider.ts | 8 +- src/services/mcp/__tests__/McpHub.spec.ts | 147 +++++++++++++++++- .../__tests__/McpOAuthClientProvider.spec.ts | 41 ++++- 4 files changed, 200 insertions(+), 5 deletions(-) diff --git a/src/services/mcp/McpHub.ts b/src/services/mcp/McpHub.ts index 60a1caacc4..c973f35a03 100644 --- a/src/services/mcp/McpHub.ts +++ b/src/services/mcp/McpHub.ts @@ -723,6 +723,7 @@ export class McpHub { // Set up file watchers for enabled servers this.setupFileWatcher(name, config, source) + let pendingAuthProvider: McpOAuthClientProvider | undefined try { const client = new Client( { @@ -840,6 +841,11 @@ export class McpHub { skipDiscovery, ...configInjected.oauth, }) + pendingAuthProvider = authProvider + if (this.isDisposed) { + await authProvider.close() + return + } // Pre-register the OAuth client so the SDK can skip its own // registration step (broken for path-prefixed issuers — see @@ -991,6 +997,7 @@ export class McpHub { authProvider: streamableHttpAuthProvider, } this.connections.push(connection) + pendingAuthProvider = undefined // Connect (this will automatically start the transport) try { @@ -1045,6 +1052,7 @@ export class McpHub { connection.server.resources = await this.fetchResourcesList(name, source) connection.server.resourceTemplates = await this.fetchResourceTemplatesList(name, source) } catch (error) { + await pendingAuthProvider?.close() // Update status with error const connection = this.findConnection(name, source) if (connection) { @@ -1104,6 +1112,7 @@ export class McpHub { const existing = await this.secretStorage.getOAuthData(serverUrl, oauthClientId) if (this.isDisposed) { unsubscribeCrossWindow() + await authProvider.close() return } // If the first read missed but the watcher fired during it, re-read — a write diff --git a/src/services/mcp/McpOAuthClientProvider.ts b/src/services/mcp/McpOAuthClientProvider.ts index 1d44ba3d01..dc0f118040 100644 --- a/src/services/mcp/McpOAuthClientProvider.ts +++ b/src/services/mcp/McpOAuthClientProvider.ts @@ -624,8 +624,12 @@ export class McpOAuthClientProvider implements OAuthClientProvider { } const tokens = (await response.json()) as OAuthTokens - await this.saveTokens(tokens, clientId) - return tokens + const mergedTokens: OAuthTokens = { + ...tokens, + refresh_token: tokens.refresh_token ?? refreshToken, + } + await this.saveTokens(mergedTokens, clientId) + return mergedTokens } /** Close the local callback server. Always call this when done. */ diff --git a/src/services/mcp/__tests__/McpHub.spec.ts b/src/services/mcp/__tests__/McpHub.spec.ts index e89883b759..aaf3769953 100644 --- a/src/services/mcp/__tests__/McpHub.spec.ts +++ b/src/services/mcp/__tests__/McpHub.spec.ts @@ -3,11 +3,15 @@ import * as path from "path" import type { Mock } from "vitest" import type { ExtensionContext, Uri } from "vscode" +import type { Client } from "@modelcontextprotocol/sdk/client/index.js" +import type { StreamableHTTPClientTransport } from "@modelcontextprotocol/sdk/client/streamableHttp.js" import type { ClineProvider } from "../../../core/webview/ClineProvider" +import type { SecretStorageService } from "../SecretStorageService" import type { McpHub as McpHubType, McpConnection, ConnectedMcpConnection, DisconnectedMcpConnection } from "../McpHub" import { ServerConfigSchema, McpHub } from "../McpHub" +import { McpOAuthClientProvider } from "../McpOAuthClientProvider" import { OAUTH_FLOW_TIMEOUT_MS } from "../constants" import { t } from "../../../i18n" @@ -128,6 +132,18 @@ vi.mock("@modelcontextprotocol/sdk/client/index.js", () => ({ Client: vi.fn(), })) +vi.mock("@modelcontextprotocol/sdk/client/streamableHttp.js", () => ({ + StreamableHTTPClientTransport: vi.fn(), +})) + +vi.mock("../McpOAuthClientProvider", () => ({ + McpOAuthClientProvider: { + create: vi.fn(), + isKnownNonOAuth: vi.fn().mockReturnValue(false), + clearNonOAuthCache: vi.fn(), + }, +})) + // Mock chokidar vi.mock("chokidar", () => ({ default: { @@ -392,6 +408,111 @@ describe("McpHub", () => { }) }) + describe("static OAuth connection wiring", () => { + it("passes static OAuth options, performs discovery, and skips dynamic registration", async () => { + const streamableModule = await import("@modelcontextprotocol/sdk/client/streamableHttp.js") + const StreamableHTTPClientTransport = streamableModule.StreamableHTTPClientTransport as ReturnType< + typeof vi.fn + > + const clientModule = await import("@modelcontextprotocol/sdk/client/index.js") + const Client = clientModule.Client as ReturnType + const mockAuthProvider = { + registerClientIfNeeded: vi.fn(), + close: vi.fn().mockResolvedValue(undefined), + } + vi.mocked(McpOAuthClientProvider.create).mockResolvedValue( + mockAuthProvider as unknown as McpOAuthClientProvider, + ) + const mockTransport = { close: vi.fn().mockResolvedValue(undefined) } + StreamableHTTPClientTransport.mockImplementation(function () { + return mockTransport as unknown as StreamableHTTPClientTransport + }) + Client.mockImplementation(function () { + return { + connect: vi.fn().mockResolvedValue(undefined), + close: vi.fn().mockResolvedValue(undefined), + getInstructions: vi.fn(), + request: vi.fn().mockResolvedValue({ tools: [], resources: [], resourceTemplates: [] }), + } as unknown as Client + }) + const secretStorage = { + hasOAuthData: vi.fn().mockResolvedValue(false), + } as unknown as SecretStorageService + const oauth = { + clientId: "registered-client", + scope: "openid offline_access", + redirectUri: "http://localhost:33418/callback", + resource: false as const, + } + vi.mocked(fs.readFile).mockResolvedValue( + JSON.stringify({ + mcpServers: { + "static-oauth-server": { + type: "streamable-http", + url: "https://example.com/mcp", + oauth, + }, + }, + }), + ) + + const hub = new McpHub(mockProvider as ClineProvider, secretStorage) + await hub.waitUntilReady() + + expect(McpOAuthClientProvider.create).toHaveBeenCalledWith( + "https://example.com/mcp", + secretStorage, + "static-oauth-server", + { skipDiscovery: false, ...oauth }, + ) + expect(mockAuthProvider.registerClientIfNeeded).not.toHaveBeenCalled() + expect(StreamableHTTPClientTransport).toHaveBeenCalledWith( + new URL("https://example.com/mcp"), + expect.objectContaining({ authProvider: mockAuthProvider }), + ) + }) + + it("closes a static OAuth provider when the hub is disposed during provider creation", async () => { + const streamableModule = await import("@modelcontextprotocol/sdk/client/streamableHttp.js") + const StreamableHTTPClientTransport = streamableModule.StreamableHTTPClientTransport as ReturnType< + typeof vi.fn + > + let resolveProvider!: (provider: McpOAuthClientProvider) => void + vi.mocked(McpOAuthClientProvider.create).mockReturnValueOnce( + new Promise((resolve) => { + resolveProvider = resolve + }), + ) + const mockAuthProvider = { + registerClientIfNeeded: vi.fn(), + close: vi.fn().mockResolvedValue(undefined), + } as unknown as McpOAuthClientProvider + const secretStorage = { + hasOAuthData: vi.fn().mockResolvedValue(false), + } as unknown as SecretStorageService + vi.mocked(fs.readFile).mockResolvedValue( + JSON.stringify({ + mcpServers: { + "static-oauth-server": { + type: "streamable-http", + url: "https://example.com/mcp", + oauth: { clientId: "registered-client" }, + }, + }, + }), + ) + + const hub = new McpHub(mockProvider as ClineProvider, secretStorage) + await Promise.resolve() + await hub.dispose() + resolveProvider(mockAuthProvider) + await hub.waitUntilReady() + + expect(mockAuthProvider.close).toHaveBeenCalledTimes(1) + expect(StreamableHTTPClientTransport).not.toHaveBeenCalled() + }) + }) + describe("Discriminated union type handling", () => { it("should create connected connections with proper type", async () => { // Mock StdioClientTransport @@ -2669,7 +2790,7 @@ describe("McpHub", () => { const serverName = "oauth-server" const serverUrl = "https://example.com/mcp" const source = "global" as const - const config = { url: serverUrl } + const config = ServerConfigSchema.parse({ type: "streamable-http", url: serverUrl }) let mockAuthProvider: any let mockTransport: any @@ -3055,6 +3176,30 @@ describe("McpHub", () => { expect(mockAuthProvider.close).toHaveBeenCalled() }) + it("should close the provider when the hub is disposed during the initial token read", async () => { + let resolveTokenRead!: (value: null) => void + mockSecretStorage.getOAuthData.mockReturnValueOnce( + new Promise((resolve) => { + resolveTokenRead = resolve + }), + ) + + const flowPromise = mcpHub["_initiateOAuthFlow"]( + serverName, + source, + config, + mockAuthProvider, + mockTransport, + mockConnection, + ) + mcpHub["isDisposed"] = true + resolveTokenRead(null) + await flowPromise + + expect(mockAuthProvider.close).toHaveBeenCalledTimes(1) + expect(vsc.window.withProgress).not.toHaveBeenCalled() + }) + it("should not reconnect when hub is disposed while cross-window tokens arrive", async () => { vi.useFakeTimers() vsc.window.showInformationMessage.mockImplementation(() => { diff --git a/src/services/mcp/__tests__/McpOAuthClientProvider.spec.ts b/src/services/mcp/__tests__/McpOAuthClientProvider.spec.ts index 9bc116e2ee..55146b6451 100644 --- a/src/services/mcp/__tests__/McpOAuthClientProvider.spec.ts +++ b/src/services/mcp/__tests__/McpOAuthClientProvider.spec.ts @@ -1,4 +1,5 @@ import { describe, it, expect, vi, beforeEach, afterAll } from "vitest" +import type { OAuthClientInformationFull } from "@modelcontextprotocol/sdk/shared/auth.js" // Mock vscode vi.mock("vscode", () => ({ @@ -435,7 +436,6 @@ describe("McpOAuthClientProvider", () => { } const refreshedTokens = { access_token: "new-access", - refresh_token: "new-refresh", token_type: "Bearer", expires_in: 3600, } @@ -453,13 +453,50 @@ describe("McpOAuthClientProvider", () => { }) const result = await provider.tokens() - expect(result).toEqual(refreshedTokens) + expect(result).toEqual({ ...refreshedTokens, refresh_token: "valid-refresh" }) expect(mockFetch).toHaveBeenCalledWith( expect.stringContaining("/token"), expect.objectContaining({ body: expect.stringContaining("grant_type=refresh_token"), }), ) + expect((await secretStorage.getOAuthData("https://example.com/mcp"))?.tokens.refresh_token).toBe( + "valid-refresh", + ) + await provider.close() + }) + + it("should use a rotated refresh token when the server returns one", async () => { + setupCallbackServerMock() + const secretStorage = createMockSecretStorage() + const provider = await McpOAuthClientProvider.create("https://example.com/mcp", secretStorage) + await provider.saveClientInformation({ client_id: "id", redirect_uris: [] } as OAuthClientInformationFull) + await secretStorage.saveOAuthData("https://example.com/mcp", { + tokens: { + access_token: "expired-access", + refresh_token: "old-refresh", + token_type: "Bearer", + }, + expires_at: Date.now() - 1000, + issuer: "https://auth.example.com", + }) + mockFetch.mockResolvedValueOnce({ + ok: true, + json: () => + Promise.resolve({ + access_token: "new-access", + refresh_token: "new-refresh", + token_type: "Bearer", + expires_in: 3600, + }), + }) + + const result = await provider.tokens() + + expect(result?.refresh_token).toBe("new-refresh") + expect((await secretStorage.getOAuthData("https://example.com/mcp"))?.tokens.refresh_token).toBe( + "new-refresh", + ) await provider.close() }) From df3c270805f6c3424aff4421e74ae47f552e6e4d Mon Sep 17 00:00:00 2001 From: Jesse Quijano Date: Thu, 22 Oct 2026 06:33:55 -0700 Subject: [PATCH 7/7] fix(mcp): harden callback and token storage (#2096) --- src/services/mcp/McpOAuthClientProvider.ts | 6 ++--- src/services/mcp/SecretStorageService.ts | 14 +++++++--- .../__tests__/McpOAuthClientProvider.spec.ts | 26 +++++++++++-------- .../__tests__/SecretStorageService.spec.ts | 21 +++++++++++++++ .../utils/__tests__/callbackServer.spec.ts | 8 +++--- src/services/mcp/utils/callbackServer.ts | 2 +- 6 files changed, 54 insertions(+), 23 deletions(-) diff --git a/src/services/mcp/McpOAuthClientProvider.ts b/src/services/mcp/McpOAuthClientProvider.ts index dc0f118040..980c03e4a4 100644 --- a/src/services/mcp/McpOAuthClientProvider.ts +++ b/src/services/mcp/McpOAuthClientProvider.ts @@ -204,7 +204,7 @@ export class McpOAuthClientProvider implements OAuthClientProvider { getCallbackAddress(options.redirectUri) } else { // A static client skips DCR, which normally starts the callback server. - // Start it here so the SDK never receives localhost:0 as redirect_uri. + // Start it here so the SDK never receives port 0 as redirect_uri. await provider._ensureCallbackServer() } provider._clientInfo = { @@ -225,7 +225,7 @@ export class McpOAuthClientProvider implements OAuthClientProvider { } get redirectUrl(): string { - return this._configuredRedirectUri ?? `http://localhost:${this._port}/callback` + return this._configuredRedirectUri ?? `http://127.0.0.1:${this._port}/callback` } private _ensureCallbackServer(): Promise { @@ -656,7 +656,7 @@ function resolveResourceIndicator(configured: McpOAuthResource | undefined, disc } function getCallbackAddress(redirectUri?: string): { port?: number; host: "localhost" | "127.0.0.1" } { - if (!redirectUri) return { host: "localhost" } + if (!redirectUri) return { host: "127.0.0.1" } const url = new URL(redirectUri) if (url.hostname !== "localhost" && url.hostname !== "127.0.0.1") { throw new Error("MCP OAuth redirectUri must use a loopback host") diff --git a/src/services/mcp/SecretStorageService.ts b/src/services/mcp/SecretStorageService.ts index 342dc62665..14b7668e6e 100644 --- a/src/services/mcp/SecretStorageService.ts +++ b/src/services/mcp/SecretStorageService.ts @@ -40,10 +40,16 @@ export class SecretStorageService { private _key(serverUrl: string, clientId?: string): string { const url = new URL(serverUrl) const normalizedPath = url.pathname.replace(/\/$/, "") - // Use base64url encoding to avoid collisions between paths like /a-b, /a_b, /a/b. - const pathSuffix = normalizedPath ? `.${Buffer.from(normalizedPath).toString("base64url")}` : "" - const clientSuffix = clientId ? `.${Buffer.from(clientId).toString("base64url")}` : "" - return `${this._namespace}${url.host}${pathSuffix}${clientSuffix}.data` + const encodedPath = Buffer.from(normalizedPath).toString("base64url") + + if (clientId) { + const encodedClientId = Buffer.from(clientId).toString("base64url") + return `${this._namespace}${url.host}.p-${encodedPath}.c-${encodedClientId}.data` + } + + // Dynamic registrations keep the legacy key for backward compatibility. + const pathSuffix = normalizedPath ? `.${encodedPath}` : "" + return `${this._namespace}${url.host}${pathSuffix}.data` } async getOAuthData(serverUrl: string, clientId?: string): Promise { diff --git a/src/services/mcp/__tests__/McpOAuthClientProvider.spec.ts b/src/services/mcp/__tests__/McpOAuthClientProvider.spec.ts index 55146b6451..0f6b6b7c0a 100644 --- a/src/services/mcp/__tests__/McpOAuthClientProvider.spec.ts +++ b/src/services/mcp/__tests__/McpOAuthClientProvider.spec.ts @@ -155,7 +155,7 @@ describe("McpOAuthClientProvider", () => { const provider = await McpOAuthClientProvider.create("https://example.com/mcp", secretStorage) expect(startCallbackServer).not.toHaveBeenCalled() - expect(provider.redirectUrl).toBe("http://localhost:0/callback") + expect(provider.redirectUrl).toBe("http://127.0.0.1:0/callback") await provider.close() }) @@ -226,28 +226,32 @@ describe("McpOAuthClientProvider", () => { ) it.each([ - "https://example.com:33418/callback", - "https://localhost:33418/callback", - "http://localhost:33418/wrong", - "http://localhost/callback", - ])("should reject unsafe or unusable configured callback %s", async (redirectUri) => { + ["https://example.com:33418/callback", "must use a loopback host"], + ["http://example.com:33418/callback", "must use a loopback host"], + ["https://localhost:33418/callback", "must use HTTP on a loopback host"], + ["http://localhost:33418/wrong", "path must be /callback"], + ["http://localhost:33418/callback?x=1", "must not contain credentials, a query, or a fragment"], + ["http://localhost:33418/callback#fragment", "must not contain credentials, a query, or a fragment"], + ["http://u:p@localhost:33418/callback", "must not contain credentials, a query, or a fragment"], + ["http://localhost/callback", "must specify a valid port"], + ] as const)("should reject configured callback %s", async (redirectUri, expectedError) => { await expect( McpOAuthClientProvider.create("https://example.com/mcp", createMockSecretStorage(), undefined, { clientId: "registered-client", redirectUri, }), - ).rejects.toThrow(/MCP OAuth redirectUri/) + ).rejects.toThrow(expectedError) }) }) describe("redirectUrl (pre-server-start)", () => { - it("should return localhost:0 before the callback server is started (intentional lazy-init behaviour)", async () => { + it("should return 127.0.0.1:0 before the callback server is started", async () => { const secretStorage = createMockSecretStorage() const provider = await McpOAuthClientProvider.create("https://example.com/mcp", secretStorage) // Port 0 is intentional: the server starts lazily in _ensureCallbackServer(). // DCR and authorization flows always call _ensureCallbackServer() first to obtain // a real port before using redirectUrl, so port 0 is never sent to an OAuth server. - expect(provider.redirectUrl).toBe("http://localhost:0/callback") + expect(provider.redirectUrl).toBe("http://127.0.0.1:0/callback") }) }) @@ -259,7 +263,7 @@ describe("McpOAuthClientProvider", () => { const metadata = provider.clientMetadata expect(metadata.client_name).toBe("Roo Code") - expect(metadata.redirect_uris).toEqual(["http://localhost:0/callback"]) + expect(metadata.redirect_uris).toEqual(["http://127.0.0.1:0/callback"]) expect(metadata.grant_types).toEqual(["authorization_code", "refresh_token"]) expect(metadata.response_types).toContain("code") expect(metadata.token_endpoint_auth_method).toBe("none") @@ -1108,7 +1112,7 @@ describe("McpOAuthClientProvider", () => { expect(body.get("client_id")).toBe("client-id-123") expect(body.get("client_secret")).toBe("client-secret-abc") expect(body.get("code_verifier")).toBe("pkce-verifier-123") - expect(body.get("redirect_uri")).toBe("http://localhost:0/callback") + expect(body.get("redirect_uri")).toBe("http://127.0.0.1:0/callback") // Verify tokens were saved const saved = await provider.tokens() diff --git a/src/services/mcp/__tests__/SecretStorageService.spec.ts b/src/services/mcp/__tests__/SecretStorageService.spec.ts index 940e0975e6..e519e987f5 100644 --- a/src/services/mcp/__tests__/SecretStorageService.spec.ts +++ b/src/services/mcp/__tests__/SecretStorageService.spec.ts @@ -236,6 +236,27 @@ describe("SecretStorageService", () => { expect((await service.getOAuthData(url))?.tokens.access_token).toBe(`tok-${i}`) } }) + + it("should not collide between a static root client ID and a legacy dynamic path", async () => { + await service.saveOAuthData( + "https://example.test/", + { tokens: { access_token: "static-token", token_type: "Bearer" }, expires_at: 1 }, + "/mcp", + ) + + expect(await service.getOAuthData("https://example.test/mcp")).toBeUndefined() + + await service.saveOAuthData("https://example.test/mcp", { + tokens: { access_token: "dynamic-token", token_type: "Bearer" }, + expires_at: 2, + }) + + expect((await service.getOAuthData("https://example.test/", "/mcp"))?.tokens.access_token).toBe( + "static-token", + ) + expect((await service.getOAuthData("https://example.test/mcp"))?.tokens.access_token).toBe("dynamic-token") + }) + it("should isolate static OAuth clients for the same server URL", async () => { await service.saveOAuthData( "https://example.com/mcp", diff --git a/src/services/mcp/utils/__tests__/callbackServer.spec.ts b/src/services/mcp/utils/__tests__/callbackServer.spec.ts index 10b5025520..1a3e6a5be6 100644 --- a/src/services/mcp/utils/__tests__/callbackServer.spec.ts +++ b/src/services/mcp/utils/__tests__/callbackServer.spec.ts @@ -46,7 +46,7 @@ describe("startCallbackServer", () => { expect(port).toBe(3000) expect(server).toBe(mockServer) - expect(mockServer.listen).toHaveBeenCalledWith(0, "localhost", expect.any(Function)) + expect(mockServer.listen).toHaveBeenCalledWith(0, "127.0.0.1", expect.any(Function)) // Simulate callback request const requestCall = mockServer.on.mock.calls.find((call) => call[0] === "request") @@ -71,16 +71,16 @@ describe("startCallbackServer", () => { expect(mockRes.end).toHaveBeenCalledWith(expect.stringContaining('')) }) - it("should bind to the configured loopback host", async () => { + it.each(["localhost", "127.0.0.1"] as const)("should bind to the configured loopback host %s", async (host) => { const mockServer = createMockServer() vi.mocked(http.createServer).mockReturnValue(mockServer as unknown as http.Server) - const { result, cancel } = await startCallbackServer(33418, undefined, "127.0.0.1") + const { result, cancel } = await startCallbackServer(33418, undefined, host) const cancellation = expect(result).rejects.toThrow("Callback cancelled") cancel() - expect(mockServer.listen).toHaveBeenCalledWith(33418, "127.0.0.1", expect.any(Function)) + expect(mockServer.listen).toHaveBeenCalledWith(33418, host, expect.any(Function)) await cancellation }) diff --git a/src/services/mcp/utils/callbackServer.ts b/src/services/mcp/utils/callbackServer.ts index 0a63b27c65..34c2f3a91d 100644 --- a/src/services/mcp/utils/callbackServer.ts +++ b/src/services/mcp/utils/callbackServer.ts @@ -24,7 +24,7 @@ type CallbackHost = "localhost" | "127.0.0.1" export function startCallbackServer( port?: number, expectedState?: string, - host: CallbackHost = "localhost", + host: CallbackHost = "127.0.0.1", ): Promise<{ server: http.Server port: number