diff --git a/packages/api/src/mcp/MCPConnectionFactory.ts b/packages/api/src/mcp/MCPConnectionFactory.ts index b5b3d61bf0..2c16da0760 100644 --- a/packages/api/src/mcp/MCPConnectionFactory.ts +++ b/packages/api/src/mcp/MCPConnectionFactory.ts @@ -287,6 +287,8 @@ export class MCPConnectionFactory { serverName: string; identifier: string; clientInfo?: OAuthClientInformation; + storedTokenEndpoint?: string; + storedAuthMethods?: string[]; }, ) => Promise { return async (refreshToken, metadata) => { @@ -296,6 +298,8 @@ export class MCPConnectionFactory { serverUrl: (this.serverConfig as t.SSEOptions | t.StreamableHTTPOptions).url, serverName: metadata.serverName, clientInfo: metadata.clientInfo, + storedTokenEndpoint: metadata.storedTokenEndpoint, + storedAuthMethods: metadata.storedAuthMethods, }, this.serverConfig.oauth_headers ?? {}, this.serverConfig.oauth, diff --git a/packages/api/src/mcp/__tests__/handler.test.ts b/packages/api/src/mcp/__tests__/handler.test.ts index 3b68d88e9c..31665ce8f7 100644 --- a/packages/api/src/mcp/__tests__/handler.test.ts +++ b/packages/api/src/mcp/__tests__/handler.test.ts @@ -260,10 +260,6 @@ describe('MCPOAuthHandler - Configurable OAuth Metadata', () => { global.fetch = mockFetch; }); - afterEach(() => { - mockFetch.mockClear(); - }); - afterAll(() => { global.fetch = originalFetch; }); @@ -679,6 +675,109 @@ describe('MCPOAuthHandler - Configurable OAuth Metadata', () => { 'Token refresh failed: 400 Bad Request - {"error":"invalid_request","error_description":"refresh_token.client_id: Field required"}', ); }); + + describe('stored token endpoint fallback', () => { + it('uses stored token endpoint when discovery fails (stored clientInfo)', async () => { + const metadata = { + serverName: 'test-server', + serverUrl: 'https://mcp.example.com', + clientInfo: { + client_id: 'test-client-id', + client_secret: 'test-client-secret', + }, + storedTokenEndpoint: 'https://auth.example.com/token', + storedAuthMethods: ['client_secret_basic'], + }; + + mockDiscoverAuthorizationServerMetadata.mockResolvedValueOnce(undefined); + + mockFetch.mockResolvedValueOnce({ + ok: true, + json: async () => ({ + access_token: 'new-access-token', + refresh_token: 'new-refresh-token', + expires_in: 3600, + }), + } as Response); + + const result = await MCPOAuthHandler.refreshOAuthTokens( + 'test-refresh-token', + metadata, + {}, + {}, + ); + + expect(mockFetch).toHaveBeenCalledWith( + 'https://auth.example.com/token', + expect.objectContaining({ method: 'POST' }), + ); + expect(result.access_token).toBe('new-access-token'); + }); + + it('uses stored token endpoint when discovery fails (auto-discovered)', async () => { + const metadata = { + serverName: 'test-server', + serverUrl: 'https://mcp.example.com', + storedTokenEndpoint: 'https://auth.example.com/token', + }; + + mockDiscoverAuthorizationServerMetadata.mockResolvedValueOnce(undefined); + + mockFetch.mockResolvedValueOnce({ + ok: true, + json: async () => ({ + access_token: 'new-access-token', + expires_in: 3600, + }), + } as Response); + + const result = await MCPOAuthHandler.refreshOAuthTokens( + 'test-refresh-token', + metadata, + {}, + {}, + ); + + const [fetchUrl] = mockFetch.mock.calls[0]; + expect(fetchUrl).toBeInstanceOf(URL); + expect(fetchUrl.toString()).toBe('https://auth.example.com/token'); + expect(result.access_token).toBe('new-access-token'); + }); + + it('still throws when discovery fails and no stored endpoint (stored clientInfo)', async () => { + const metadata = { + serverName: 'test-server', + serverUrl: 'https://mcp.example.com', + clientInfo: { + client_id: 'test-client-id', + client_secret: 'test-client-secret', + }, + }; + + mockDiscoverAuthorizationServerMetadata.mockResolvedValueOnce(undefined); + + await expect( + MCPOAuthHandler.refreshOAuthTokens('test-refresh-token', metadata, {}, {}), + ).rejects.toThrow('No OAuth metadata discovered for token refresh'); + + expect(mockFetch).not.toHaveBeenCalled(); + }); + + it('still throws when discovery fails and no stored endpoint (auto-discovered)', async () => { + const metadata = { + serverName: 'test-server', + serverUrl: 'https://mcp.example.com', + }; + + mockDiscoverAuthorizationServerMetadata.mockResolvedValueOnce(undefined); + + await expect( + MCPOAuthHandler.refreshOAuthTokens('test-refresh-token', metadata, {}, {}), + ).rejects.toThrow('No OAuth metadata discovered for token refresh'); + + expect(mockFetch).not.toHaveBeenCalled(); + }); + }); }); describe('revokeOAuthToken', () => { @@ -1187,10 +1286,6 @@ describe('MCPOAuthHandler - Configurable OAuth Metadata', () => { global.fetch = mockFetch; }); - afterEach(() => { - mockFetch.mockClear(); - }); - afterAll(() => { global.fetch = originalFetch; }); @@ -1363,7 +1458,7 @@ describe('MCPOAuthHandler - Configurable OAuth Metadata', () => { ); }); - it('should use fallback /token endpoint for refresh when metadata discovery fails', async () => { + it('should throw when metadata discovery fails during refresh (stored clientInfo)', async () => { const metadata = { serverName: 'test-server', serverUrl: 'https://mcp.example.com', @@ -1373,38 +1468,16 @@ describe('MCPOAuthHandler - Configurable OAuth Metadata', () => { }, }; - // Mock metadata discovery to return undefined (no .well-known) mockDiscoverAuthorizationServerMetadata.mockResolvedValueOnce(undefined); - // Mock successful token refresh - mockFetch.mockResolvedValueOnce({ - ok: true, - json: async () => ({ - access_token: 'new-access-token', - refresh_token: 'new-refresh-token', - expires_in: 3600, - }), - } as Response); + await expect( + MCPOAuthHandler.refreshOAuthTokens('test-refresh-token', metadata, {}, {}), + ).rejects.toThrow('No OAuth metadata discovered for token refresh'); - const result = await MCPOAuthHandler.refreshOAuthTokens( - 'test-refresh-token', - metadata, - {}, - {}, - ); - - // Verify fetch was called with fallback /token endpoint - expect(mockFetch).toHaveBeenCalledWith( - 'https://mcp.example.com/token', - expect.objectContaining({ - method: 'POST', - }), - ); - - expect(result.access_token).toBe('new-access-token'); + expect(mockFetch).not.toHaveBeenCalled(); }); - it('should use fallback auth methods when metadata discovery fails during refresh', async () => { + it('should throw when metadata lacks token endpoint during refresh', async () => { const metadata = { serverName: 'test-server', serverUrl: 'https://mcp.example.com', @@ -1414,30 +1487,51 @@ describe('MCPOAuthHandler - Configurable OAuth Metadata', () => { }, }; - // Mock metadata discovery to return undefined + mockDiscoverAuthorizationServerMetadata.mockResolvedValueOnce({ + issuer: 'https://auth.example.com/', + authorization_endpoint: 'https://auth.example.com/authorize', + response_types_supported: ['code'], + } as AuthorizationServerMetadata); + + await expect( + MCPOAuthHandler.refreshOAuthTokens('test-refresh-token', metadata, {}, {}), + ).rejects.toThrow('No token endpoint found in OAuth metadata'); + + expect(mockFetch).not.toHaveBeenCalled(); + }); + + it('should throw for auto-discovered refresh when metadata discovery returns undefined', async () => { + const metadata = { + serverName: 'test-server', + serverUrl: 'https://mcp.example.com', + }; + mockDiscoverAuthorizationServerMetadata.mockResolvedValueOnce(undefined); - // Mock successful token refresh - mockFetch.mockResolvedValueOnce({ - ok: true, - json: async () => ({ - access_token: 'new-access-token', - expires_in: 3600, - }), - } as Response); + await expect( + MCPOAuthHandler.refreshOAuthTokens('test-refresh-token', metadata, {}, {}), + ).rejects.toThrow('No OAuth metadata discovered for token refresh'); - await MCPOAuthHandler.refreshOAuthTokens('test-refresh-token', metadata, {}, {}); + expect(mockFetch).not.toHaveBeenCalled(); + }); - // Verify it uses client_secret_basic (first in fallback auth methods) - const expectedAuth = `Basic ${Buffer.from('test-client-id:test-client-secret').toString('base64')}`; - expect(mockFetch).toHaveBeenCalledWith( - expect.any(String), - expect.objectContaining({ - headers: expect.objectContaining({ - Authorization: expectedAuth, - }), - }), - ); + it('should throw for auto-discovered refresh when metadata has no token_endpoint', async () => { + const metadata = { + serverName: 'test-server', + serverUrl: 'https://mcp.example.com', + }; + + mockDiscoverAuthorizationServerMetadata.mockResolvedValueOnce({ + issuer: 'https://auth.example.com/', + authorization_endpoint: 'https://auth.example.com/authorize', + response_types_supported: ['code'], + } as AuthorizationServerMetadata); + + await expect( + MCPOAuthHandler.refreshOAuthTokens('test-refresh-token', metadata, {}, {}), + ).rejects.toThrow('No token endpoint found in OAuth metadata'); + + expect(mockFetch).not.toHaveBeenCalled(); }); describe('path-based URL origin fallback', () => { @@ -1574,7 +1668,7 @@ describe('MCPOAuthHandler - Configurable OAuth Metadata', () => { expect(result.access_token).toBe('new-access-token'); }); - it('falls back to /token when both path and origin discovery fail', async () => { + it('throws when both path and origin discovery return undefined', async () => { const metadata = { serverName: 'sentry', serverUrl: 'https://mcp.sentry.dev/mcp', @@ -1585,36 +1679,19 @@ describe('MCPOAuthHandler - Configurable OAuth Metadata', () => { }, }; - // Both path AND origin discovery return undefined mockDiscoverAuthorizationServerMetadata .mockResolvedValueOnce(undefined) .mockResolvedValueOnce(undefined); - mockFetch.mockResolvedValueOnce({ - ok: true, - json: async () => ({ - access_token: 'new-access-token', - refresh_token: 'new-refresh-token', - expires_in: 3600, - }), - } as Response); - - const result = await MCPOAuthHandler.refreshOAuthTokens( - 'test-refresh-token', - metadata, - {}, - {}, - ); + await expect( + MCPOAuthHandler.refreshOAuthTokens('test-refresh-token', metadata, {}, {}), + ).rejects.toThrow('No OAuth metadata discovered for token refresh'); expect(mockDiscoverAuthorizationServerMetadata).toHaveBeenCalledTimes(2); - - // Falls back to /token relative to server URL origin - const [fetchUrl] = mockFetch.mock.calls[0]; - expect(String(fetchUrl)).toBe('https://mcp.sentry.dev/token'); - expect(result.access_token).toBe('new-access-token'); + expect(mockFetch).not.toHaveBeenCalled(); }); - it('does not retry with origin when server URL has no path (root URL)', async () => { + it('throws when root URL discovery returns undefined (no path retry)', async () => { const metadata = { serverName: 'test-server', serverUrl: 'https://auth.example.com/', @@ -1624,18 +1701,14 @@ describe('MCPOAuthHandler - Configurable OAuth Metadata', () => { }, }; - // Root URL discovery fails — no retry mockDiscoverAuthorizationServerMetadata.mockResolvedValueOnce(undefined); - mockFetch.mockResolvedValueOnce({ - ok: true, - json: async () => ({ access_token: 'new-token', expires_in: 3600 }), - } as Response); + await expect( + MCPOAuthHandler.refreshOAuthTokens('test-refresh-token', metadata, {}, {}), + ).rejects.toThrow('No OAuth metadata discovered for token refresh'); - await MCPOAuthHandler.refreshOAuthTokens('test-refresh-token', metadata, {}, {}); - - // Only one discovery attempt for a root URL expect(mockDiscoverAuthorizationServerMetadata).toHaveBeenCalledTimes(1); + expect(mockFetch).not.toHaveBeenCalled(); }); it('retries with origin when path-based discovery throws', async () => { diff --git a/packages/api/src/mcp/oauth/handler.ts b/packages/api/src/mcp/oauth/handler.ts index 0a9154ff35..873af5c66d 100644 --- a/packages/api/src/mcp/oauth/handler.ts +++ b/packages/api/src/mcp/oauth/handler.ts @@ -821,7 +821,13 @@ export class MCPOAuthHandler { */ static async refreshOAuthTokens( refreshToken: string, - metadata: { serverName: string; serverUrl?: string; clientInfo?: OAuthClientInformation }, + metadata: { + serverName: string; + serverUrl?: string; + clientInfo?: OAuthClientInformation; + storedTokenEndpoint?: string; + storedAuthMethods?: string[]; + }, oauthHeaders: Record, config?: MCPOptions['oauth'], allowedDomains?: string[] | null, @@ -862,15 +868,18 @@ export class MCPOAuthHandler { const oauthMetadata = await this.discoverWithOriginFallback(serverUrl, fetchFn); if (!oauthMetadata) { - /** - * No metadata discovered - use fallback /token endpoint. - * This mirrors the MCP SDK's behavior for legacy servers without .well-known endpoints. - */ - logger.warn( - `[MCPOAuth] No OAuth metadata discovered for token refresh, using fallback /token endpoint`, - ); - tokenUrl = new URL('/token', metadata.serverUrl).toString(); - authMethods = ['client_secret_basic', 'client_secret_post', 'none']; + if (metadata.storedTokenEndpoint) { + tokenUrl = metadata.storedTokenEndpoint; + authMethods = metadata.storedAuthMethods; + } else { + /** + * Do NOT fall back to `new URL('/token', metadata.serverUrl)`. + * metadata.serverUrl is the MCP resource server, which may differ from the + * authorization server. Sending refresh tokens there leaks them to the + * resource server operator when .well-known discovery is absent. + */ + throw new Error('No OAuth metadata discovered for token refresh'); + } } else if (!oauthMetadata.token_endpoint) { throw new Error('No token endpoint found in OAuth metadata'); } else { @@ -930,8 +939,8 @@ export class MCPOAuthHandler { } logger.debug(`[MCPOAuth] Refresh request to: ${sanitizeUrlForLogging(tokenUrl)}`, { - body: body.toString(), - headers, + grant_type: 'refresh_token', + has_auth_header: !!headers['Authorization'], }); const response = await fetch(tokenUrl, { @@ -1040,15 +1049,16 @@ export class MCPOAuthHandler { const oauthMetadata = await this.discoverWithOriginFallback(serverUrl, fetchFn); let tokenUrl: URL; - if (!oauthMetadata?.token_endpoint) { - /** - * No metadata or token_endpoint discovered - use fallback /token endpoint. - * This mirrors the MCP SDK's behavior for legacy servers without .well-known endpoints. - */ - logger.warn( - `[MCPOAuth] No OAuth metadata or token endpoint found, using fallback /token endpoint`, - ); - tokenUrl = new URL('/token', metadata.serverUrl); + if (!oauthMetadata) { + if (metadata.storedTokenEndpoint) { + tokenUrl = new URL(metadata.storedTokenEndpoint); + } else { + // Same rationale as the stored-clientInfo branch above: never fall back + // to metadata.serverUrl which is the MCP resource server, not the auth server. + throw new Error('No OAuth metadata discovered for token refresh'); + } + } else if (!oauthMetadata.token_endpoint) { + throw new Error('No token endpoint found in OAuth metadata'); } else { tokenUrl = new URL(oauthMetadata.token_endpoint); } @@ -1103,17 +1113,11 @@ export class MCPOAuthHandler { oauthHeaders: Record = {}, allowedDomains?: string[] | null, ): Promise { - if (metadata.revocationEndpoint != null) { - await this.validateOAuthUrl( - metadata.revocationEndpoint, - 'revocation_endpoint', - allowedDomains, - ); - } const revokeUrl: URL = metadata.revocationEndpoint != null ? new URL(metadata.revocationEndpoint) : new URL('/revoke', metadata.serverUrl); + await this.validateOAuthUrl(revokeUrl.href, 'revocation_endpoint', allowedDomains); const authMethods = metadata.revocationEndpointAuthMethodsSupported ?? ['client_secret_basic']; const authMethod = resolveTokenEndpointAuthMethod({ tokenAuthMethods: authMethods }); diff --git a/packages/api/src/mcp/oauth/tokens.ts b/packages/api/src/mcp/oauth/tokens.ts index 6094a05386..1e31a64511 100644 --- a/packages/api/src/mcp/oauth/tokens.ts +++ b/packages/api/src/mcp/oauth/tokens.ts @@ -41,6 +41,8 @@ interface GetTokensParams { serverName: string; identifier: string; clientInfo?: OAuthClientInformation; + storedTokenEndpoint?: string; + storedAuthMethods?: string[]; }, ) => Promise; createToken?: TokenMethods['createToken']; @@ -306,9 +308,10 @@ export class MCPTokenStorage { logger.info(`${logPrefix} Attempting to refresh token`); const decryptedRefreshToken = await decryptV2(refreshTokenData.token); - /** Client information if available */ let clientInfo; let clientInfoData; + let storedTokenEndpoint: string | undefined; + let storedAuthMethods: string[] | undefined; try { clientInfoData = await findToken({ userId, @@ -322,6 +325,19 @@ export class MCPTokenStorage { client_id: clientInfo.client_id, has_client_secret: !!clientInfo.client_secret, }); + + if (clientInfoData.metadata) { + const raw = + clientInfoData.metadata instanceof Map + ? Object.fromEntries(clientInfoData.metadata) + : (clientInfoData.metadata as Record); + if (typeof raw.token_endpoint === 'string') { + storedTokenEndpoint = raw.token_endpoint; + } + if (Array.isArray(raw.token_endpoint_auth_methods_supported)) { + storedAuthMethods = raw.token_endpoint_auth_methods_supported as string[]; + } + } } } catch { logger.debug(`${logPrefix} No client info found`); @@ -332,6 +348,8 @@ export class MCPTokenStorage { serverName, identifier, clientInfo, + storedTokenEndpoint, + storedAuthMethods, }; const newTokens = await refreshTokens(decryptedRefreshToken, metadata);