From 2c07dc0757867a138b3832d781457ecf5d9afcf7 Mon Sep 17 00:00:00 2001 From: Brian Ray <62354532+emeryray2002@users.noreply.github.com> Date: Fri, 15 Aug 2025 15:14:48 -0400 Subject: [PATCH] fixes for oauth spec - adds github oauth support. Resource paramater. (#6281) --- packages/core/src/mcp/oauth-provider.test.ts | 296 +++++++++++++------ packages/core/src/mcp/oauth-provider.ts | 281 +++++++++++++----- packages/core/src/mcp/oauth-utils.test.ts | 40 ++- packages/core/src/mcp/oauth-utils.ts | 102 +++++-- packages/core/src/tools/mcp-client.ts | 11 +- 5 files changed, 557 insertions(+), 173 deletions(-) diff --git a/packages/core/src/mcp/oauth-provider.test.ts b/packages/core/src/mcp/oauth-provider.test.ts index 3991aecc..2163d6bc 100644 --- a/packages/core/src/mcp/oauth-provider.test.ts +++ b/packages/core/src/mcp/oauth-provider.test.ts @@ -29,6 +29,55 @@ import { MCPOAuthTokenStorage, MCPOAuthToken } from './oauth-token-storage.js'; const mockFetch = vi.fn(); global.fetch = mockFetch; +// Helper function to create mock fetch responses with proper headers +const createMockResponse = (options: { + ok: boolean; + status?: number; + contentType?: string; + text?: string | (() => Promise); + json?: unknown | (() => Promise); +}) => { + const response: { + ok: boolean; + status?: number; + headers: { + get: (name: string) => string | null; + }; + text?: () => Promise; + json?: () => Promise; + } = { + ok: options.ok, + headers: { + get: (name: string) => { + if (name.toLowerCase() === 'content-type') { + return options.contentType || null; + } + return null; + }, + }, + }; + + if (options.status !== undefined) { + response.status = options.status; + } + + if (options.text !== undefined) { + response.text = + typeof options.text === 'string' + ? () => Promise.resolve(options.text as string) + : (options.text as () => Promise); + } + + if (options.json !== undefined) { + response.json = + typeof options.json === 'function' + ? (options.json as () => Promise) + : () => Promise.resolve(options.json); + } + + return response; +}; + // Define a reusable mock server with .listen, .close, and .on methods const mockHttpServer = { listen: vi.fn(), @@ -133,10 +182,14 @@ describe('MCPOAuthProvider', () => { }); // Mock token exchange - mockFetch.mockResolvedValueOnce({ - ok: true, - json: () => Promise.resolve(mockTokenResponse), - }); + mockFetch.mockResolvedValueOnce( + createMockResponse({ + ok: true, + contentType: 'application/json', + text: JSON.stringify(mockTokenResponse), + json: mockTokenResponse, + }), + ); const result = await MCPOAuthProvider.authenticate( 'test-server', @@ -165,7 +218,11 @@ describe('MCPOAuthProvider', () => { it('should handle OAuth discovery when no authorization URL provided', async () => { // Use a mutable config object - const configWithoutAuth: MCPOAuthConfig = { ...mockConfig }; + const configWithoutAuth: MCPOAuthConfig = { + ...mockConfig, + clientId: 'test-client-id', + clientSecret: 'test-client-secret', + }; delete configWithoutAuth.authorizationUrl; delete configWithoutAuth.tokenUrl; @@ -179,21 +236,30 @@ describe('MCPOAuthProvider', () => { scopes_supported: ['read', 'write'], }; + // Mock HEAD request for WWW-Authenticate check mockFetch - .mockResolvedValueOnce({ - ok: true, - json: () => Promise.resolve(mockResourceMetadata), - }) - .mockResolvedValueOnce({ - ok: true, - json: () => Promise.resolve(mockAuthServerMetadata), - }); - - // Patch config after discovery - configWithoutAuth.authorizationUrl = - mockAuthServerMetadata.authorization_endpoint; - configWithoutAuth.tokenUrl = mockAuthServerMetadata.token_endpoint; - configWithoutAuth.scopes = mockAuthServerMetadata.scopes_supported; + .mockResolvedValueOnce( + createMockResponse({ + ok: true, + status: 200, + }), + ) + .mockResolvedValueOnce( + createMockResponse({ + ok: true, + contentType: 'application/json', + text: JSON.stringify(mockResourceMetadata), + json: mockResourceMetadata, + }), + ) + .mockResolvedValueOnce( + createMockResponse({ + ok: true, + contentType: 'application/json', + text: JSON.stringify(mockAuthServerMetadata), + json: mockAuthServerMetadata, + }), + ); // Setup callback handler let callbackHandler: unknown; @@ -220,10 +286,14 @@ describe('MCPOAuthProvider', () => { }); // Mock token exchange with discovered endpoint - mockFetch.mockResolvedValueOnce({ - ok: true, - json: () => Promise.resolve(mockTokenResponse), - }); + mockFetch.mockResolvedValueOnce( + createMockResponse({ + ok: true, + contentType: 'application/json', + text: JSON.stringify(mockTokenResponse), + json: mockTokenResponse, + }), + ); const result = await MCPOAuthProvider.authenticate( 'test-server', @@ -236,7 +306,9 @@ describe('MCPOAuthProvider', () => { 'https://discovered.auth.com/token', expect.objectContaining({ method: 'POST', - headers: { 'Content-Type': 'application/x-www-form-urlencoded' }, + headers: expect.objectContaining({ + 'Content-Type': 'application/x-www-form-urlencoded', + }), }), ); }); @@ -261,14 +333,22 @@ describe('MCPOAuthProvider', () => { }; mockFetch - .mockResolvedValueOnce({ - ok: true, - json: () => Promise.resolve(mockAuthServerMetadata), - }) - .mockResolvedValueOnce({ - ok: true, - json: () => Promise.resolve(mockRegistrationResponse), - }); + .mockResolvedValueOnce( + createMockResponse({ + ok: true, + contentType: 'application/json', + text: JSON.stringify(mockAuthServerMetadata), + json: mockAuthServerMetadata, + }), + ) + .mockResolvedValueOnce( + createMockResponse({ + ok: true, + contentType: 'application/json', + text: JSON.stringify(mockRegistrationResponse), + json: mockRegistrationResponse, + }), + ); // Setup callback handler let callbackHandler: unknown; @@ -295,10 +375,14 @@ describe('MCPOAuthProvider', () => { }); // Mock token exchange - mockFetch.mockResolvedValueOnce({ - ok: true, - json: () => Promise.resolve(mockTokenResponse), - }); + mockFetch.mockResolvedValueOnce( + createMockResponse({ + ok: true, + contentType: 'application/json', + text: JSON.stringify(mockTokenResponse), + json: mockTokenResponse, + }), + ); const result = await MCPOAuthProvider.authenticate( 'test-server', @@ -397,15 +481,18 @@ describe('MCPOAuthProvider', () => { }, 10); }); - mockFetch.mockResolvedValueOnce({ - ok: false, - status: 400, - text: () => Promise.resolve('Invalid grant'), - }); + mockFetch.mockResolvedValueOnce( + createMockResponse({ + ok: false, + status: 400, + contentType: 'application/x-www-form-urlencoded', + text: 'error=invalid_grant&error_description=Invalid grant', + }), + ); await expect( MCPOAuthProvider.authenticate('test-server', mockConfig), - ).rejects.toThrow('Token exchange failed: 400 - Invalid grant'); + ).rejects.toThrow('Token exchange failed: invalid_grant - Invalid grant'); }); it('should handle callback timeout', async () => { @@ -445,10 +532,14 @@ describe('MCPOAuthProvider', () => { refresh_token: 'new_refresh_token', }; - mockFetch.mockResolvedValueOnce({ - ok: true, - json: () => Promise.resolve(refreshResponse), - }); + mockFetch.mockResolvedValueOnce( + createMockResponse({ + ok: true, + contentType: 'application/json', + text: JSON.stringify(refreshResponse), + json: refreshResponse, + }), + ); const result = await MCPOAuthProvider.refreshAccessToken( mockConfig, @@ -461,17 +552,24 @@ describe('MCPOAuthProvider', () => { 'https://auth.example.com/token', expect.objectContaining({ method: 'POST', - headers: { 'Content-Type': 'application/x-www-form-urlencoded' }, + headers: { + 'Content-Type': 'application/x-www-form-urlencoded', + Accept: 'application/json, application/x-www-form-urlencoded', + }, body: expect.stringContaining('grant_type=refresh_token'), }), ); }); it('should include client secret in refresh request when available', async () => { - mockFetch.mockResolvedValueOnce({ - ok: true, - json: () => Promise.resolve(mockTokenResponse), - }); + mockFetch.mockResolvedValueOnce( + createMockResponse({ + ok: true, + contentType: 'application/json', + text: JSON.stringify(mockTokenResponse), + json: mockTokenResponse, + }), + ); await MCPOAuthProvider.refreshAccessToken( mockConfig, @@ -484,11 +582,14 @@ describe('MCPOAuthProvider', () => { }); it('should handle refresh token failure', async () => { - mockFetch.mockResolvedValueOnce({ - ok: false, - status: 400, - text: () => Promise.resolve('Invalid refresh token'), - }); + mockFetch.mockResolvedValueOnce( + createMockResponse({ + ok: false, + status: 400, + contentType: 'application/x-www-form-urlencoded', + text: 'error=invalid_request&error_description=Invalid refresh token', + }), + ); await expect( MCPOAuthProvider.refreshAccessToken( @@ -496,7 +597,9 @@ describe('MCPOAuthProvider', () => { 'invalid_refresh_token', 'https://auth.example.com/token', ), - ).rejects.toThrow('Token refresh failed: 400 - Invalid refresh token'); + ).rejects.toThrow( + 'Token refresh failed: invalid_request - Invalid refresh token', + ); }); }); @@ -544,10 +647,14 @@ describe('MCPOAuthProvider', () => { refresh_token: 'new_refresh_token', }; - mockFetch.mockResolvedValueOnce({ - ok: true, - json: () => Promise.resolve(refreshResponse), - }); + mockFetch.mockResolvedValueOnce( + createMockResponse({ + ok: true, + contentType: 'application/json', + text: JSON.stringify(refreshResponse), + json: refreshResponse, + }), + ); const result = await MCPOAuthProvider.getValidToken( 'test-server', @@ -590,11 +697,14 @@ describe('MCPOAuthProvider', () => { vi.mocked(MCPOAuthTokenStorage.isTokenExpired).mockReturnValue(true); vi.mocked(MCPOAuthTokenStorage.removeToken).mockResolvedValue(undefined); - mockFetch.mockResolvedValueOnce({ - ok: false, - status: 400, - text: () => Promise.resolve('Invalid refresh token'), - }); + mockFetch.mockResolvedValueOnce( + createMockResponse({ + ok: false, + status: 400, + contentType: 'application/x-www-form-urlencoded', + text: 'error=invalid_request&error_description=Invalid refresh token', + }), + ); const result = await MCPOAuthProvider.getValidToken( 'test-server', @@ -664,10 +774,14 @@ describe('MCPOAuthProvider', () => { }, 10); }); - mockFetch.mockResolvedValueOnce({ - ok: true, - json: () => Promise.resolve(mockTokenResponse), - }); + mockFetch.mockResolvedValueOnce( + createMockResponse({ + ok: true, + contentType: 'application/json', + text: JSON.stringify(mockTokenResponse), + json: mockTokenResponse, + }), + ); await MCPOAuthProvider.authenticate('test-server', mockConfig); @@ -709,12 +823,20 @@ describe('MCPOAuthProvider', () => { }, 10); }); - mockFetch.mockResolvedValueOnce({ - ok: true, - json: () => Promise.resolve(mockTokenResponse), - }); + mockFetch.mockResolvedValueOnce( + createMockResponse({ + ok: true, + contentType: 'application/json', + text: JSON.stringify(mockTokenResponse), + json: mockTokenResponse, + }), + ); - await MCPOAuthProvider.authenticate('test-server', mockConfig); + await MCPOAuthProvider.authenticate( + 'test-server', + mockConfig, + 'https://auth.example.com', + ); expect(capturedUrl).toBeDefined(); expect(capturedUrl!).toContain('response_type=code'); @@ -757,10 +879,14 @@ describe('MCPOAuthProvider', () => { }, 10); }); - mockFetch.mockResolvedValueOnce({ - ok: true, - json: () => Promise.resolve(mockTokenResponse), - }); + mockFetch.mockResolvedValueOnce( + createMockResponse({ + ok: true, + contentType: 'application/json', + text: JSON.stringify(mockTokenResponse), + json: mockTokenResponse, + }), + ); const configWithParamsInUrl = { ...mockConfig, @@ -806,10 +932,14 @@ describe('MCPOAuthProvider', () => { }, 10); }); - mockFetch.mockResolvedValueOnce({ - ok: true, - json: () => Promise.resolve(mockTokenResponse), - }); + mockFetch.mockResolvedValueOnce( + createMockResponse({ + ok: true, + contentType: 'application/json', + text: JSON.stringify(mockTokenResponse), + json: mockTokenResponse, + }), + ); const configWithFragment = { ...mockConfig, diff --git a/packages/core/src/mcp/oauth-provider.ts b/packages/core/src/mcp/oauth-provider.ts index eaec5c2e..c2579d1a 100644 --- a/packages/core/src/mcp/oauth-provider.ts +++ b/packages/core/src/mcp/oauth-provider.ts @@ -144,8 +144,8 @@ export class MCPOAuthProvider { private static async discoverOAuthFromMCPServer( mcpServerUrl: string, ): Promise { - const baseUrl = OAuthUtils.extractBaseUrl(mcpServerUrl); - return OAuthUtils.discoverOAuthConfig(baseUrl); + // Use the full URL with path preserved for OAuth discovery + return OAuthUtils.discoverOAuthConfig(mcpServerUrl); } /** @@ -302,14 +302,18 @@ export class MCPOAuthProvider { } // Add resource parameter for MCP OAuth spec compliance - // Use the MCP server URL if provided, otherwise fall back to authorization URL - const resourceUrl = mcpServerUrl || config.authorizationUrl!; - try { - params.append('resource', OAuthUtils.buildResourceParameter(resourceUrl)); - } catch (error) { - throw new Error( - `Invalid resource URL: "${resourceUrl}". ${getErrorMessage(error)}`, - ); + // Only add if we have an MCP server URL (indicates MCP OAuth flow, not standard OAuth) + if (mcpServerUrl) { + try { + params.append( + 'resource', + OAuthUtils.buildResourceParameter(mcpServerUrl), + ); + } catch (error) { + console.warn( + `Could not add resource parameter: ${getErrorMessage(error)}`, + ); + } } const url = new URL(config.authorizationUrl!); @@ -355,32 +359,93 @@ export class MCPOAuthProvider { } // Add resource parameter for MCP OAuth spec compliance - // Use the MCP server URL if provided, otherwise fall back to token URL - const resourceUrl = mcpServerUrl || config.tokenUrl!; - try { - params.append('resource', OAuthUtils.buildResourceParameter(resourceUrl)); - } catch (error) { - throw new Error( - `Invalid resource URL: "${resourceUrl}". ${getErrorMessage(error)}`, - ); + // Only add if we have an MCP server URL (indicates MCP OAuth flow, not standard OAuth) + if (mcpServerUrl) { + const resourceUrl = mcpServerUrl; + try { + params.append( + 'resource', + OAuthUtils.buildResourceParameter(resourceUrl), + ); + } catch (error) { + console.warn( + `Could not add resource parameter: ${getErrorMessage(error)}`, + ); + } } const response = await fetch(config.tokenUrl!, { method: 'POST', headers: { 'Content-Type': 'application/x-www-form-urlencoded', + Accept: 'application/json, application/x-www-form-urlencoded', }, body: params.toString(), }); + const responseText = await response.text(); + const contentType = response.headers.get('content-type') || ''; + if (!response.ok) { - const errorText = await response.text(); + // Try to parse error from form-urlencoded response + let errorMessage: string | null = null; + try { + const errorParams = new URLSearchParams(responseText); + const error = errorParams.get('error'); + const errorDescription = errorParams.get('error_description'); + if (error) { + errorMessage = `Token exchange failed: ${error} - ${errorDescription || 'No description'}`; + } + } catch { + // Fall back to raw error + } throw new Error( - `Token exchange failed: ${response.status} - ${errorText}`, + errorMessage || + `Token exchange failed: ${response.status} - ${responseText}`, ); } - return (await response.json()) as OAuthTokenResponse; + // Log unexpected content types for debugging + if ( + !contentType.includes('application/json') && + !contentType.includes('application/x-www-form-urlencoded') + ) { + console.warn( + `Token endpoint returned unexpected content-type: ${contentType}. ` + + `Expected application/json or application/x-www-form-urlencoded. ` + + `Will attempt to parse response.`, + ); + } + + // Try to parse as JSON first, fall back to form-urlencoded + try { + return JSON.parse(responseText) as OAuthTokenResponse; + } catch { + // Parse form-urlencoded response + const tokenParams = new URLSearchParams(responseText); + const accessToken = tokenParams.get('access_token'); + const tokenType = tokenParams.get('token_type') || 'Bearer'; + const expiresIn = tokenParams.get('expires_in'); + const refreshToken = tokenParams.get('refresh_token'); + const scope = tokenParams.get('scope'); + + if (!accessToken) { + // Check for error in response + const error = tokenParams.get('error'); + const errorDescription = tokenParams.get('error_description'); + throw new Error( + `Token exchange failed: ${error || 'no_access_token'} - ${errorDescription || responseText}`, + ); + } + + return { + access_token: accessToken, + token_type: tokenType, + expires_in: expiresIn ? parseInt(expiresIn, 10) : undefined, + refresh_token: refreshToken || undefined, + scope: scope || undefined, + } as OAuthTokenResponse; + } } /** @@ -417,32 +482,92 @@ export class MCPOAuthProvider { } // Add resource parameter for MCP OAuth spec compliance - // Use the MCP server URL if provided, otherwise fall back to token URL - const resourceUrl = mcpServerUrl || tokenUrl; - try { - params.append('resource', OAuthUtils.buildResourceParameter(resourceUrl)); - } catch (error) { - throw new Error( - `Invalid resource URL: "${resourceUrl}". ${getErrorMessage(error)}`, - ); + // Only add if we have an MCP server URL (indicates MCP OAuth flow, not standard OAuth) + if (mcpServerUrl) { + try { + params.append( + 'resource', + OAuthUtils.buildResourceParameter(mcpServerUrl), + ); + } catch (error) { + console.warn( + `Could not add resource parameter: ${getErrorMessage(error)}`, + ); + } } const response = await fetch(tokenUrl, { method: 'POST', headers: { 'Content-Type': 'application/x-www-form-urlencoded', + Accept: 'application/json, application/x-www-form-urlencoded', }, body: params.toString(), }); + const responseText = await response.text(); + const contentType = response.headers.get('content-type') || ''; + if (!response.ok) { - const errorText = await response.text(); + // Try to parse error from form-urlencoded response + let errorMessage: string | null = null; + try { + const errorParams = new URLSearchParams(responseText); + const error = errorParams.get('error'); + const errorDescription = errorParams.get('error_description'); + if (error) { + errorMessage = `Token refresh failed: ${error} - ${errorDescription || 'No description'}`; + } + } catch { + // Fall back to raw error + } throw new Error( - `Token refresh failed: ${response.status} - ${errorText}`, + errorMessage || + `Token refresh failed: ${response.status} - ${responseText}`, ); } - return (await response.json()) as OAuthTokenResponse; + // Log unexpected content types for debugging + if ( + !contentType.includes('application/json') && + !contentType.includes('application/x-www-form-urlencoded') + ) { + console.warn( + `Token refresh endpoint returned unexpected content-type: ${contentType}. ` + + `Expected application/json or application/x-www-form-urlencoded. ` + + `Will attempt to parse response.`, + ); + } + + // Try to parse as JSON first, fall back to form-urlencoded + try { + return JSON.parse(responseText) as OAuthTokenResponse; + } catch { + // Parse form-urlencoded response + const tokenParams = new URLSearchParams(responseText); + const accessToken = tokenParams.get('access_token'); + const tokenType = tokenParams.get('token_type') || 'Bearer'; + const expiresIn = tokenParams.get('expires_in'); + const refreshToken = tokenParams.get('refresh_token'); + const scope = tokenParams.get('scope'); + + if (!accessToken) { + // Check for error in response + const error = tokenParams.get('error'); + const errorDescription = tokenParams.get('error_description'); + throw new Error( + `Token refresh failed: ${error || 'unknown_error'} - ${errorDescription || responseText}`, + ); + } + + return { + access_token: accessToken, + token_type: tokenType, + expires_in: expiresIn ? parseInt(expiresIn, 10) : undefined, + refresh_token: refreshToken || undefined, + scope: scope || undefined, + } as OAuthTokenResponse; + } } /** @@ -464,37 +589,43 @@ export class MCPOAuthProvider { 'No authorization URL provided, attempting OAuth discovery...', ); - // For SSE URLs, first check if authentication is required - if (OAuthUtils.isSSEEndpoint(mcpServerUrl)) { - try { - const response = await fetch(mcpServerUrl, { - method: 'HEAD', - headers: { - Accept: 'text/event-stream', - }, - }); + // First check if the server requires authentication via WWW-Authenticate header + try { + const headers: HeadersInit = OAuthUtils.isSSEEndpoint(mcpServerUrl) + ? { Accept: 'text/event-stream' } + : { Accept: 'application/json' }; - if (response.status === 401 || response.status === 307) { - const wwwAuthenticate = response.headers.get('www-authenticate'); - if (wwwAuthenticate) { - const discoveredConfig = - await OAuthUtils.discoverOAuthFromWWWAuthenticate( - wwwAuthenticate, - ); - if (discoveredConfig) { - config = { - ...config, - ...discoveredConfig, - scopes: discoveredConfig.scopes || config.scopes || [], - }; - } + const response = await fetch(mcpServerUrl, { + method: 'HEAD', + headers, + }); + + if (response.status === 401 || response.status === 307) { + const wwwAuthenticate = response.headers.get('www-authenticate'); + + if (wwwAuthenticate) { + const discoveredConfig = + await OAuthUtils.discoverOAuthFromWWWAuthenticate( + wwwAuthenticate, + ); + if (discoveredConfig) { + // Merge discovered config with existing config, preserving clientId and clientSecret + config = { + ...config, + authorizationUrl: discoveredConfig.authorizationUrl, + tokenUrl: discoveredConfig.tokenUrl, + scopes: discoveredConfig.scopes || config.scopes || [], + // Preserve existing client credentials + clientId: config.clientId, + clientSecret: config.clientSecret, + }; } } - } catch (error) { - console.debug( - `Failed to check SSE endpoint for authentication requirements: ${getErrorMessage(error)}`, - ); } + } catch (error) { + console.debug( + `Failed to check endpoint for authentication requirements: ${getErrorMessage(error)}`, + ); } // If we still don't have OAuth config, try the standard discovery @@ -502,8 +633,16 @@ export class MCPOAuthProvider { const discoveredConfig = await this.discoverOAuthFromMCPServer(mcpServerUrl); if (discoveredConfig) { - config = { ...config, ...discoveredConfig }; - console.log('OAuth configuration discovered successfully'); + // Merge discovered config with existing config, preserving clientId and clientSecret + config = { + ...config, + authorizationUrl: discoveredConfig.authorizationUrl, + tokenUrl: discoveredConfig.tokenUrl, + scopes: discoveredConfig.scopes || config.scopes || [], + // Preserve existing client credentials + clientId: config.clientId, + clientSecret: config.clientSecret, + }; } else { throw new Error( 'Failed to discover OAuth configuration from MCP server', @@ -633,9 +772,13 @@ export class MCPOAuthProvider { ); // Convert to our token format + if (!tokenResponse.access_token) { + throw new Error('No access token received from token endpoint'); + } + const token: MCPOAuthToken = { accessToken: tokenResponse.access_token, - tokenType: tokenResponse.token_type, + tokenType: tokenResponse.token_type || 'Bearer', refreshToken: tokenResponse.refresh_token, scope: tokenResponse.scope, }; @@ -657,12 +800,16 @@ export class MCPOAuthProvider { // Verify token was saved const savedToken = await MCPOAuthTokenStorage.getToken(serverName); - if (savedToken) { - console.log( - `Token verification successful: ${savedToken.token.accessToken.substring(0, 20)}...`, - ); + if (savedToken && savedToken.token && savedToken.token.accessToken) { + const tokenPreview = + savedToken.token.accessToken.length > 20 + ? `${savedToken.token.accessToken.substring(0, 20)}...` + : '[token]'; + console.log(`Token verification successful: ${tokenPreview}`); } else { - console.error('Token verification failed: token not found after save'); + console.error( + 'Token verification failed: token not found or invalid after save', + ); } } catch (saveError) { console.error(`Failed to save token: ${getErrorMessage(saveError)}`); diff --git a/packages/core/src/mcp/oauth-utils.test.ts b/packages/core/src/mcp/oauth-utils.test.ts index 12871ff2..47bfb9ae 100644 --- a/packages/core/src/mcp/oauth-utils.test.ts +++ b/packages/core/src/mcp/oauth-utils.test.ts @@ -28,8 +28,8 @@ describe('OAuthUtils', () => { }); describe('buildWellKnownUrls', () => { - it('should build correct well-known URLs', () => { - const urls = OAuthUtils.buildWellKnownUrls('https://example.com/path'); + it('should build standard root-based URLs by default', () => { + const urls = OAuthUtils.buildWellKnownUrls('https://example.com/mcp'); expect(urls.protectedResource).toBe( 'https://example.com/.well-known/oauth-protected-resource', ); @@ -37,6 +37,42 @@ describe('OAuthUtils', () => { 'https://example.com/.well-known/oauth-authorization-server', ); }); + + it('should build path-based URLs when includePathSuffix is true', () => { + const urls = OAuthUtils.buildWellKnownUrls( + 'https://example.com/mcp', + true, + ); + expect(urls.protectedResource).toBe( + 'https://example.com/.well-known/oauth-protected-resource/mcp', + ); + expect(urls.authorizationServer).toBe( + 'https://example.com/.well-known/oauth-authorization-server/mcp', + ); + }); + + it('should handle root path correctly', () => { + const urls = OAuthUtils.buildWellKnownUrls('https://example.com', true); + expect(urls.protectedResource).toBe( + 'https://example.com/.well-known/oauth-protected-resource', + ); + expect(urls.authorizationServer).toBe( + 'https://example.com/.well-known/oauth-authorization-server', + ); + }); + + it('should handle trailing slash in path', () => { + const urls = OAuthUtils.buildWellKnownUrls( + 'https://example.com/mcp/', + true, + ); + expect(urls.protectedResource).toBe( + 'https://example.com/.well-known/oauth-protected-resource/mcp', + ); + expect(urls.authorizationServer).toBe( + 'https://example.com/.well-known/oauth-authorization-server/mcp', + ); + }); }); describe('fetchProtectedResourceMetadata', () => { diff --git a/packages/core/src/mcp/oauth-utils.ts b/packages/core/src/mcp/oauth-utils.ts index 64fd68be..b926ce0b 100644 --- a/packages/core/src/mcp/oauth-utils.ts +++ b/packages/core/src/mcp/oauth-utils.ts @@ -43,18 +43,36 @@ export interface OAuthProtectedResourceMetadata { export class OAuthUtils { /** * Construct well-known OAuth endpoint URLs. + * By default, uses standard root-based well-known URLs. + * If includePathSuffix is true, appends any path from the base URL to the well-known endpoints. */ - static buildWellKnownUrls(baseUrl: string) { + static buildWellKnownUrls(baseUrl: string, includePathSuffix = false) { const serverUrl = new URL(baseUrl); const base = `${serverUrl.protocol}//${serverUrl.host}`; + if (!includePathSuffix) { + // Standard discovery: use root-based well-known URLs + return { + protectedResource: new URL( + '/.well-known/oauth-protected-resource', + base, + ).toString(), + authorizationServer: new URL( + '/.well-known/oauth-authorization-server', + base, + ).toString(), + }; + } + + // Path-based discovery: append path suffix to well-known URLs + const pathSuffix = serverUrl.pathname.replace(/\/$/, ''); // Remove trailing slash return { protectedResource: new URL( - '/.well-known/oauth-protected-resource', + `/.well-known/oauth-protected-resource${pathSuffix}`, base, ).toString(), authorizationServer: new URL( - '/.well-known/oauth-authorization-server', + `/.well-known/oauth-authorization-server${pathSuffix}`, base, ).toString(), }; @@ -132,25 +150,56 @@ export class OAuthUtils { serverUrl: string, ): Promise { try { - const wellKnownUrls = this.buildWellKnownUrls(serverUrl); + // First try standard root-based discovery + const wellKnownUrls = this.buildWellKnownUrls(serverUrl, false); - // First, try to get the protected resource metadata - const resourceMetadata = await this.fetchProtectedResourceMetadata( + // Try to get the protected resource metadata at root + let resourceMetadata = await this.fetchProtectedResourceMetadata( wellKnownUrls.protectedResource, ); + // If root discovery fails and we have a path, try path-based discovery + if (!resourceMetadata) { + const url = new URL(serverUrl); + if (url.pathname && url.pathname !== '/') { + const pathBasedUrls = this.buildWellKnownUrls(serverUrl, true); + resourceMetadata = await this.fetchProtectedResourceMetadata( + pathBasedUrls.protectedResource, + ); + } + } + if (resourceMetadata?.authorization_servers?.length) { // Use the first authorization server const authServerUrl = resourceMetadata.authorization_servers[0]; - const authServerMetadataUrl = new URL( - '/.well-known/oauth-authorization-server', - authServerUrl, + + // The authorization server URL may include a path (e.g., https://github.com/login/oauth) + // We need to preserve this path when constructing the metadata URL + const authServerUrlObj = new URL(authServerUrl); + const authServerPath = + authServerUrlObj.pathname === '/' ? '' : authServerUrlObj.pathname; + + // Try with the authorization server's path first + let authServerMetadataUrl = new URL( + `/.well-known/oauth-authorization-server${authServerPath}`, + `${authServerUrlObj.protocol}//${authServerUrlObj.host}`, ).toString(); - const authServerMetadata = await this.fetchAuthorizationServerMetadata( + let authServerMetadata = await this.fetchAuthorizationServerMetadata( authServerMetadataUrl, ); + // If that fails, try root as fallback + if (!authServerMetadata && authServerPath) { + authServerMetadataUrl = new URL( + '/.well-known/oauth-authorization-server', + `${authServerUrlObj.protocol}//${authServerUrlObj.host}`, + ).toString(); + authServerMetadata = await this.fetchAuthorizationServerMetadata( + authServerMetadataUrl, + ); + } + if (authServerMetadata) { const config = this.metadataToOAuthConfig(authServerMetadata); if (authServerMetadata.registration_endpoint) { @@ -221,10 +270,6 @@ export class OAuthUtils { return null; } - console.log( - `Found resource metadata URI from www-authenticate header: ${resourceMetadataUri}`, - ); - const resourceMetadata = await this.fetchProtectedResourceMetadata(resourceMetadataUri); if (!resourceMetadata?.authorization_servers?.length) { @@ -232,19 +277,36 @@ export class OAuthUtils { } const authServerUrl = resourceMetadata.authorization_servers[0]; + + // The authorization server URL may include a path (e.g., https://github.com/login/oauth) + // We need to preserve this path when constructing the metadata URL + const authServerUrlObj = new URL(authServerUrl); + const authServerPath = + authServerUrlObj.pathname === '/' ? '' : authServerUrlObj.pathname; + + // Build auth server metadata URL with the authorization server's path const authServerMetadataUrl = new URL( - '/.well-known/oauth-authorization-server', - authServerUrl, + `/.well-known/oauth-authorization-server${authServerPath}`, + `${authServerUrlObj.protocol}//${authServerUrlObj.host}`, ).toString(); - const authServerMetadata = await this.fetchAuthorizationServerMetadata( + let authServerMetadata = await this.fetchAuthorizationServerMetadata( authServerMetadataUrl, ); - if (authServerMetadata) { - console.log( - 'OAuth configuration discovered successfully from www-authenticate header', + // If that fails and we have a path, also try the root path as a fallback + if (!authServerMetadata && authServerPath) { + const rootAuthServerMetadataUrl = new URL( + '/.well-known/oauth-authorization-server', + `${authServerUrlObj.protocol}//${authServerUrlObj.host}`, + ).toString(); + + authServerMetadata = await this.fetchAuthorizationServerMetadata( + rootAuthServerMetadataUrl, ); + } + + if (authServerMetadata) { return this.metadataToOAuthConfig(authServerMetadata); } diff --git a/packages/core/src/tools/mcp-client.ts b/packages/core/src/tools/mcp-client.ts index 83bc4024..837b91fc 100644 --- a/packages/core/src/tools/mcp-client.ts +++ b/packages/core/src/tools/mcp-client.ts @@ -227,10 +227,16 @@ async function handleAutomaticOAuth( }; // Perform OAuth authentication + // Pass the server URL for proper discovery + const serverUrl = mcpServerConfig.httpUrl || mcpServerConfig.url; console.log( `Starting OAuth authentication for server '${mcpServerName}'...`, ); - await MCPOAuthProvider.authenticate(mcpServerName, oauthAuthConfig); + await MCPOAuthProvider.authenticate( + mcpServerName, + oauthAuthConfig, + serverUrl, + ); console.log( `OAuth authentication successful for server '${mcpServerName}'`, @@ -933,12 +939,15 @@ export async function connectToMcpServer( }; // Perform OAuth authentication + // Pass the server URL for proper discovery + const serverUrl = mcpServerConfig.httpUrl || mcpServerConfig.url; console.log( `Starting OAuth authentication for server '${mcpServerName}'...`, ); await MCPOAuthProvider.authenticate( mcpServerName, oauthAuthConfig, + serverUrl, ); // Retry connection with OAuth token