diff --git a/packages/core/src/mcp/oauth-provider.test.ts b/packages/core/src/mcp/oauth-provider.test.ts index 018c2d8c71..a06d213462 100644 --- a/packages/core/src/mcp/oauth-provider.test.ts +++ b/packages/core/src/mcp/oauth-provider.test.ts @@ -1006,6 +1006,154 @@ describe('MCPOAuthProvider', () => { global.setTimeout = originalSetTimeout; }); + + it('should use port from redirectUri if provided', async () => { + const configWithPort: MCPOAuthConfig = { + ...mockConfig, + redirectUri: 'http://localhost:12345/oauth/callback', + }; + + let callbackHandler: unknown; + vi.mocked(http.createServer).mockImplementation((handler) => { + callbackHandler = handler; + return mockHttpServer as unknown as http.Server; + }); + + mockHttpServer.listen.mockImplementation((port, callback) => { + callback?.(); + setTimeout(() => { + const mockReq = { + url: '/oauth/callback?code=auth_code_123&state=bW9ja19zdGF0ZV8xNl9ieXRlcw', + }; + const mockRes = { + writeHead: vi.fn(), + end: vi.fn(), + }; + (callbackHandler as (req: unknown, res: unknown) => void)( + mockReq, + mockRes, + ); + }, 10); + }); + mockHttpServer.address.mockReturnValue({ + port: 12345, + address: '127.0.0.1', + family: 'IPv4', + }); + + mockFetch.mockResolvedValueOnce( + createMockResponse({ + ok: true, + contentType: 'application/json', + text: JSON.stringify(mockTokenResponse), + json: mockTokenResponse, + }), + ); + + const authProvider = new MCPOAuthProvider(); + await authProvider.authenticate('test-server', configWithPort); + + expect(mockHttpServer.listen).toHaveBeenCalledWith( + 12345, + expect.any(Function), + ); + }); + + it('should ignore invalid ports in redirectUri', async () => { + const configWithInvalidPort: MCPOAuthConfig = { + ...mockConfig, + redirectUri: 'http://localhost:invalid/oauth/callback', + }; + + let callbackHandler: unknown; + vi.mocked(http.createServer).mockImplementation((handler) => { + callbackHandler = handler; + return mockHttpServer as unknown as http.Server; + }); + + mockHttpServer.listen.mockImplementation((port, callback) => { + callback?.(); + setTimeout(() => { + const mockReq = { + url: '/oauth/callback?code=auth_code_123&state=bW9ja19zdGF0ZV8xNl9ieXRlcw', + }; + const mockRes = { + writeHead: vi.fn(), + end: vi.fn(), + }; + (callbackHandler as (req: unknown, res: unknown) => void)( + mockReq, + mockRes, + ); + }, 10); + }); + + mockFetch.mockResolvedValueOnce( + createMockResponse({ + ok: true, + contentType: 'application/json', + text: JSON.stringify(mockTokenResponse), + json: mockTokenResponse, + }), + ); + + const authProvider = new MCPOAuthProvider(); + await authProvider.authenticate('test-server', configWithInvalidPort); + + // Should be called with 0 (OS assigned) because the port was invalid + expect(mockHttpServer.listen).toHaveBeenCalledWith( + 0, + expect.any(Function), + ); + }); + + it('should not default to privileged ports when redirectUri has no port', async () => { + const configNoPort: MCPOAuthConfig = { + ...mockConfig, + redirectUri: 'http://localhost/oauth/callback', + }; + + let callbackHandler: unknown; + vi.mocked(http.createServer).mockImplementation((handler) => { + callbackHandler = handler; + return mockHttpServer as unknown as http.Server; + }); + + mockHttpServer.listen.mockImplementation((port, callback) => { + callback?.(); + setTimeout(() => { + const mockReq = { + url: '/oauth/callback?code=auth_code_123&state=bW9ja19zdGF0ZV8xNl9ieXRlcw', + }; + const mockRes = { + writeHead: vi.fn(), + end: vi.fn(), + }; + (callbackHandler as (req: unknown, res: unknown) => void)( + mockReq, + mockRes, + ); + }, 10); + }); + + mockFetch.mockResolvedValueOnce( + createMockResponse({ + ok: true, + contentType: 'application/json', + text: JSON.stringify(mockTokenResponse), + json: mockTokenResponse, + }), + ); + + const authProvider = new MCPOAuthProvider(); + await authProvider.authenticate('test-server', configNoPort); + + // Should be called with 0 (OS assigned), not 80 + expect(mockHttpServer.listen).toHaveBeenCalledWith( + 0, + expect.any(Function), + ); + }); }); describe('refreshAccessToken', () => { @@ -1286,7 +1434,7 @@ describe('MCPOAuthProvider', () => { const authProvider = new MCPOAuthProvider(); await authProvider.authenticate('test-server', mockConfig); - expect(crypto.randomBytes).toHaveBeenCalledWith(32); // code verifier + expect(crypto.randomBytes).toHaveBeenCalledWith(64); // code verifier expect(crypto.randomBytes).toHaveBeenCalledWith(16); // state expect(crypto.createHash).toHaveBeenCalledWith('sha256'); }); diff --git a/packages/core/src/mcp/oauth-provider.ts b/packages/core/src/mcp/oauth-provider.ts index bdadd0d54c..b79ec693a3 100644 --- a/packages/core/src/mcp/oauth-provider.ts +++ b/packages/core/src/mcp/oauth-provider.ts @@ -245,7 +245,8 @@ export class MCPOAuthProvider { */ private generatePKCEParams(): PKCEParams { // Generate code verifier (43-128 characters) - const codeVerifier = crypto.randomBytes(32).toString('base64url'); + // using 64 bytes results in ~86 characters, safely above the minimum of 43 + const codeVerifier = crypto.randomBytes(64).toString('base64url'); // Generate code challenge using SHA256 const codeChallenge = crypto @@ -266,7 +267,10 @@ export class MCPOAuthProvider { * @param expectedState The state parameter to validate * @returns Object containing the port (available immediately) and a promise for the auth response */ - private startCallbackServer(expectedState: string): { + private startCallbackServer( + expectedState: string, + port?: number, + ): { port: Promise; response: Promise; } { @@ -353,9 +357,10 @@ export class MCPOAuthProvider { reject(error); }); - // Determine which port to use (env var or OS-assigned) - const portStr = process.env['OAUTH_CALLBACK_PORT']; + // Determine which port to use (env var, argument, or OS-assigned) let listenPort = 0; // Default to OS-assigned port + + const portStr = process.env['OAUTH_CALLBACK_PORT']; if (portStr) { const envPort = parseInt(portStr, 10); if (isNaN(envPort) || envPort <= 0 || envPort > 65535) { @@ -367,6 +372,8 @@ export class MCPOAuthProvider { return; } listenPort = envPort; + } else if (port !== undefined) { + listenPort = port; } server.listen(listenPort, () => { @@ -393,7 +400,34 @@ export class MCPOAuthProvider { } /** - * Build the authorization URL with PKCE parameters. + * Extract the port number from a URL string if available and valid. + * + * @param urlString The URL string to parse + * @returns The port number or undefined if not found or invalid + */ + private getPortFromUrl(urlString?: string): number | undefined { + if (!urlString) { + return undefined; + } + + try { + const url = new URL(urlString); + if (url.port) { + const parsedPort = parseInt(url.port, 10); + if (!isNaN(parsedPort) && parsedPort > 0 && parsedPort <= 65535) { + return parsedPort; + } + } + } catch { + // Ignore invalid URL + } + + return undefined; + } + + /** + * Build the authorization URL for the OAuth flow. + * * @param config OAuth configuration * @param pkceParams PKCE parameters @@ -798,9 +832,15 @@ export class MCPOAuthProvider { // Generate PKCE parameters const pkceParams = this.generatePKCEParams(); + // Determine preferred port from redirectUri if available + const preferredPort = this.getPortFromUrl(config.redirectUri); + // Start callback server first to allocate port // This ensures we only create one server and eliminates race conditions - const callbackServer = this.startCallbackServer(pkceParams.state); + const callbackServer = this.startCallbackServer( + pkceParams.state, + preferredPort, + ); // Wait for server to start and get the allocated port // We need this port for client registration and auth URL building