diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index e037894259..8057e53364 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -435,7 +435,7 @@ importers: version: 3.1078.0 '@google/genai': specifier: ^1.29.1 - version: 1.52.0(@modelcontextprotocol/sdk@1.29.0(zod@3.25.76)) + version: 1.52.0(@modelcontextprotocol/sdk@1.31.0(zod@3.25.76)) '@lmstudio/sdk': specifier: ^1.1.1 version: 1.5.0 @@ -443,8 +443,8 @@ importers: specifier: ^1.9.18 version: 1.15.1 '@modelcontextprotocol/sdk': - specifier: 1.29.0 - version: 1.29.0(zod@3.25.76) + specifier: 1.31.0 + version: 1.31.0(zod@3.25.76) '@qdrant/js-client-rest': specifier: ^1.14.0 version: 1.18.0(typescript@5.9.3) @@ -2055,8 +2055,8 @@ packages: '@mistralai/mistralai@1.15.1': resolution: {integrity: sha512-fb995eiz3r0KsBGtRjFV+/iLbX+UpfalxpF+YitT3R6ukrPD4PN+FGwwmYcRFhNAzVzDUtTVxQYnjQWEnwV5nw==} - '@modelcontextprotocol/sdk@1.29.0': - resolution: {integrity: sha512-zo37mZA9hJWpULgkRpowewez1y6ML5GsXJPY8FI0tBBCd77HEvza4jDqRKOXgHNn867PVGCyTdzqpz0izu5ZjQ==} + '@modelcontextprotocol/sdk@1.31.0': + resolution: {integrity: sha512-UvTMgnNlnIBO/22ob2RcVGDlcvOslQs8T59+FTGdA0L27a39fdGF/EDETNtDVK4DZGpwomlsYpRdA8UXcVL/pw==} engines: {node: '>=18'} peerDependencies: '@cfworker/json-schema': ^4.1.1 @@ -10071,14 +10071,14 @@ snapshots: '@floating-ui/utils@0.2.11': {} - '@google/genai@1.52.0(@modelcontextprotocol/sdk@1.29.0(zod@3.25.76))': + '@google/genai@1.52.0(@modelcontextprotocol/sdk@1.31.0(zod@3.25.76))': dependencies: google-auth-library: 10.9.0 p-retry: 4.6.2 protobufjs: 7.6.4 ws: 8.21.3 optionalDependencies: - '@modelcontextprotocol/sdk': 1.29.0(zod@3.25.76) + '@modelcontextprotocol/sdk': 1.31.0(zod@3.25.76) transitivePeerDependencies: - bufferutil - supports-color @@ -10366,7 +10366,7 @@ snapshots: - bufferutil - utf-8-validate - '@modelcontextprotocol/sdk@1.29.0(zod@3.25.76)': + '@modelcontextprotocol/sdk@1.31.0(zod@3.25.76)': dependencies: '@hono/node-server': 1.19.14(hono@4.12.23) ajv: 8.20.0 diff --git a/src/package.json b/src/package.json index fb4cb26abc..1c633a3917 100644 --- a/src/package.json +++ b/src/package.json @@ -481,7 +481,7 @@ "@google/genai": "^1.29.1", "@lmstudio/sdk": "^1.1.1", "@mistralai/mistralai": "^1.9.18", - "@modelcontextprotocol/sdk": "1.29.0", + "@modelcontextprotocol/sdk": "1.31.0", "@qdrant/js-client-rest": "^1.14.0", "@roo-code/cloud": "workspace:^", "@roo-code/core": "workspace:^", diff --git a/src/services/mcp/McpOAuthClientProvider.ts b/src/services/mcp/McpOAuthClientProvider.ts index dba08ef60d..52b328c35a 100644 --- a/src/services/mcp/McpOAuthClientProvider.ts +++ b/src/services/mcp/McpOAuthClientProvider.ts @@ -236,7 +236,9 @@ export class McpOAuthClientProvider implements OAuthClientProvider { // Check if we have a cached client_id from previous registration const cachedData = await this._secretStorage.getOAuthData(this._serverUrl) - if (cachedData?.client_info) { + // 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) { // Use the full DCR response, override redirect_uris with the // current port (which may have changed between sessions). this._clientInfo = { @@ -279,6 +281,12 @@ export class McpOAuthClientProvider implements OAuthClientProvider { // Access token is expired or near expiry. Try to refresh if we have a refresh token. if (data.tokens.refresh_token) { + // 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) + return undefined + } if (this._refreshPromise) { return this._refreshPromise } @@ -317,6 +325,7 @@ export class McpOAuthClientProvider implements OAuthClientProvider { await this._secretStorage.saveOAuthData(this._serverUrl, { tokens, expires_at, + ...(this._authServerMeta?.issuer ? { issuer: this._authServerMeta.issuer as string } : {}), ...(clientInfo ? { client_info: clientInfo } : {}), }) } diff --git a/src/services/mcp/SecretStorageService.ts b/src/services/mcp/SecretStorageService.ts index 9a66c4c270..e2cda51715 100644 --- a/src/services/mcp/SecretStorageService.ts +++ b/src/services/mcp/SecretStorageService.ts @@ -5,6 +5,11 @@ export interface StoredMcpOAuthData { tokens: OAuthTokens /** Unix ms timestamp after which the access token should be considered expired. */ expires_at: number + /** + * Issuer of the authorization server that issued these tokens. Refresh is + * allowed only against the same issuer. Missing on data saved by older versions. + */ + issuer?: 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 2a3b360f60..700ede149c 100644 --- a/src/services/mcp/__tests__/McpOAuthClientProvider.spec.ts +++ b/src/services/mcp/__tests__/McpOAuthClientProvider.spec.ts @@ -334,6 +334,26 @@ describe("McpOAuthClientProvider", () => { await provider.close() }) + it.each([ + ["a different issuer", "https://evil.example.com"], + ["no issuer (saved by an older version)", undefined], + ])("should not refresh and should clear stored data when stored data has %s", async (_label, issuer) => { + setupCallbackServerMock() + const secretStorage = createMockSecretStorage() + const provider = await McpOAuthClientProvider.create("https://example.com/mcp", secretStorage) + + mockFetch.mockClear() // discard discovery calls made by create() + await secretStorage.saveOAuthData("https://example.com/mcp", { + tokens: { access_token: "expired-access", refresh_token: "valid-refresh", token_type: "Bearer" }, + expires_at: Date.now() - 1000, + ...(issuer ? { issuer } : {}), + }) + + expect(await provider.tokens()).toBeUndefined() + expect(mockFetch).not.toHaveBeenCalled() + expect(await secretStorage.getOAuthData("https://example.com/mcp")).toBeUndefined() + }) + it("should refresh tokens when access token is expired but refresh token exists", async () => { setupCallbackServerMock() const secretStorage = createMockSecretStorage() @@ -355,6 +375,7 @@ describe("McpOAuthClientProvider", () => { await secretStorage.saveOAuthData("https://example.com/mcp", { tokens: initialTokens, expires_at: Date.now() - 1000, + issuer: "https://auth.example.com", }) mockFetch.mockResolvedValueOnce({ @@ -788,6 +809,7 @@ describe("McpOAuthClientProvider", () => { await secretStorage.saveOAuthData("https://example.com/mcp", { tokens: { access_token: "cached-token", token_type: "Bearer" }, expires_at: Date.now() + 3600000, + issuer: "https://auth.example.com", client_info: { client_id: "cached-client-id", client_name: "Test Client", @@ -815,6 +837,7 @@ describe("McpOAuthClientProvider", () => { await secretStorage.saveOAuthData("https://example.com/mcp", { tokens: { access_token: "cached-token", token_type: "Bearer" }, expires_at: Date.now() + 3600000, + issuer: "https://auth.example.com", client_info: { client_id: "cached-client-id", client_name: "Test Client", @@ -833,6 +856,32 @@ describe("McpOAuthClientProvider", () => { await provider.close() }) + it("should not reuse cached client_info registered with a different issuer", async () => { + setupCallbackServerMock() + const secretStorage = createMockSecretStorage() + await secretStorage.saveOAuthData("https://example.com/mcp", { + tokens: { access_token: "cached-token", token_type: "Bearer" }, + expires_at: Date.now() + 3600000, + issuer: "https://evil.example.com", + client_info: { + client_id: "cached-client-id", + client_secret: "cached-secret", + redirect_uris: ["http://localhost:9999/callback"], + }, + }) + + const provider = await McpOAuthClientProvider.create("https://example.com/mcp", secretStorage) + mockFetch.mockClear() + mockFetch.mockResolvedValueOnce({ + ok: true, + json: () => Promise.resolve({ client_id: "new-client-id", redirect_uris: [provider.redirectUrl] }), + }) + await provider.registerClientIfNeeded() + + expect((await provider.clientInformation())?.client_id).toBe("new-client-id") + await provider.close() + }) + it("should perform DCR when no cached client_id exists", async () => { setupCallbackServerMock() const secretStorage = createMockSecretStorage()