diff options
| -rw-r--r-- | packages/core/src/mcp/oauth-provider.test.ts | 21 | ||||
| -rw-r--r-- | packages/core/src/mcp/oauth-provider.ts | 6 | ||||
| -rw-r--r-- | packages/core/src/utils/secure-browser-launcher.test.ts | 242 | ||||
| -rw-r--r-- | packages/core/src/utils/secure-browser-launcher.ts | 188 |
4 files changed, 445 insertions, 12 deletions
diff --git a/packages/core/src/mcp/oauth-provider.test.ts b/packages/core/src/mcp/oauth-provider.test.ts index 20dc9fab..5bfd637b 100644 --- a/packages/core/src/mcp/oauth-provider.test.ts +++ b/packages/core/src/mcp/oauth-provider.test.ts @@ -7,7 +7,6 @@ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; import * as http from 'node:http'; import * as crypto from 'node:crypto'; -import open from 'open'; import { MCPOAuthProvider, MCPOAuthConfig, @@ -17,7 +16,10 @@ import { import { MCPOAuthTokenStorage, MCPOAuthToken } from './oauth-token-storage.js'; // Mock dependencies -vi.mock('open'); +const mockOpenBrowserSecurely = vi.hoisted(() => vi.fn()); +vi.mock('../utils/secure-browser-launcher.js', () => ({ + openBrowserSecurely: mockOpenBrowserSecurely, +})); vi.mock('node:crypto'); vi.mock('./oauth-token-storage.js'); @@ -64,6 +66,7 @@ describe('MCPOAuthProvider', () => { beforeEach(() => { vi.clearAllMocks(); + mockOpenBrowserSecurely.mockClear(); vi.spyOn(console, 'log').mockImplementation(() => {}); vi.spyOn(console, 'warn').mockImplementation(() => {}); vi.spyOn(console, 'error').mockImplementation(() => {}); @@ -145,7 +148,9 @@ describe('MCPOAuthProvider', () => { expiresAt: expect.any(Number), }); - expect(open).toHaveBeenCalledWith(expect.stringContaining('authorize')); + expect(mockOpenBrowserSecurely).toHaveBeenCalledWith( + expect.stringContaining('authorize'), + ); expect(MCPOAuthTokenStorage.saveToken).toHaveBeenCalledWith( 'test-server', expect.objectContaining({ accessToken: 'access_token_123' }), @@ -672,13 +677,10 @@ describe('MCPOAuthProvider', () => { describe('Authorization URL building', () => { it('should build correct authorization URL with all parameters', async () => { // Mock to capture the URL that would be opened - let capturedUrl: string; - vi.mocked(open).mockImplementation((url) => { + let capturedUrl: string | undefined; + mockOpenBrowserSecurely.mockImplementation((url: string) => { capturedUrl = url; - // Return a minimal mock ChildProcess - return Promise.resolve({ - pid: 1234, - } as unknown as import('child_process').ChildProcess); + return Promise.resolve(); }); let callbackHandler: unknown; @@ -711,6 +713,7 @@ describe('MCPOAuthProvider', () => { await MCPOAuthProvider.authenticate('test-server', mockConfig); + expect(capturedUrl).toBeDefined(); expect(capturedUrl!).toContain('response_type=code'); expect(capturedUrl!).toContain('client_id=test-client-id'); expect(capturedUrl!).toContain('code_challenge=code_challenge_mock'); diff --git a/packages/core/src/mcp/oauth-provider.ts b/packages/core/src/mcp/oauth-provider.ts index 2f65f051..491ec477 100644 --- a/packages/core/src/mcp/oauth-provider.ts +++ b/packages/core/src/mcp/oauth-provider.ts @@ -7,7 +7,7 @@ import * as http from 'node:http'; import * as crypto from 'node:crypto'; import { URL } from 'node:url'; -import open from 'open'; +import { openBrowserSecurely } from '../utils/secure-browser-launcher.js'; import { MCPOAuthToken, MCPOAuthTokenStorage } from './oauth-token-storage.js'; import { getErrorMessage } from '../utils/errors.js'; import { OAuthUtils } from './oauth-utils.js'; @@ -593,9 +593,9 @@ export class MCPOAuthProvider { // Start callback server const callbackPromise = this.startCallbackServer(pkceParams.state); - // Open browser + // Open browser securely try { - await open(authUrl); + await openBrowserSecurely(authUrl); } catch (error) { console.warn( 'Failed to open browser automatically:', diff --git a/packages/core/src/utils/secure-browser-launcher.test.ts b/packages/core/src/utils/secure-browser-launcher.test.ts new file mode 100644 index 00000000..de27ce6f --- /dev/null +++ b/packages/core/src/utils/secure-browser-launcher.test.ts @@ -0,0 +1,242 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { openBrowserSecurely } from './secure-browser-launcher.js'; + +// Create mock function using vi.hoisted +const mockExecFile = vi.hoisted(() => vi.fn()); + +// Mock modules +vi.mock('node:child_process'); +vi.mock('node:util', () => ({ + promisify: () => mockExecFile, +})); + +describe('secure-browser-launcher', () => { + let originalPlatform: PropertyDescriptor | undefined; + + beforeEach(() => { + vi.clearAllMocks(); + mockExecFile.mockResolvedValue({ stdout: '', stderr: '' }); + originalPlatform = Object.getOwnPropertyDescriptor(process, 'platform'); + }); + + afterEach(() => { + if (originalPlatform) { + Object.defineProperty(process, 'platform', originalPlatform); + } + }); + + function setPlatform(platform: string) { + Object.defineProperty(process, 'platform', { + value: platform, + configurable: true, + }); + } + + describe('URL validation', () => { + it('should allow valid HTTP URLs', async () => { + setPlatform('darwin'); + await openBrowserSecurely('http://example.com'); + expect(mockExecFile).toHaveBeenCalledWith( + 'open', + ['http://example.com'], + expect.any(Object), + ); + }); + + it('should allow valid HTTPS URLs', async () => { + setPlatform('darwin'); + await openBrowserSecurely('https://example.com'); + expect(mockExecFile).toHaveBeenCalledWith( + 'open', + ['https://example.com'], + expect.any(Object), + ); + }); + + it('should reject non-HTTP(S) protocols', async () => { + await expect(openBrowserSecurely('file:///etc/passwd')).rejects.toThrow( + 'Unsafe protocol', + ); + await expect(openBrowserSecurely('javascript:alert(1)')).rejects.toThrow( + 'Unsafe protocol', + ); + await expect(openBrowserSecurely('ftp://example.com')).rejects.toThrow( + 'Unsafe protocol', + ); + }); + + it('should reject invalid URLs', async () => { + await expect(openBrowserSecurely('not-a-url')).rejects.toThrow( + 'Invalid URL', + ); + await expect(openBrowserSecurely('')).rejects.toThrow('Invalid URL'); + }); + + it('should reject URLs with control characters', async () => { + await expect( + openBrowserSecurely('http://example.com\nmalicious-command'), + ).rejects.toThrow('invalid characters'); + await expect( + openBrowserSecurely('http://example.com\rmalicious-command'), + ).rejects.toThrow('invalid characters'); + await expect( + openBrowserSecurely('http://example.com\x00'), + ).rejects.toThrow('invalid characters'); + }); + }); + + describe('Command injection prevention', () => { + it('should prevent PowerShell command injection on Windows', async () => { + setPlatform('win32'); + + // The POC from the vulnerability report + const maliciousUrl = + "http://127.0.0.1:8080/?param=example#$(Invoke-Expression([System.Text.Encoding]::UTF8.GetString([System.Convert]::FromBase64String('Y2FsYy5leGU='))))"; + + await openBrowserSecurely(maliciousUrl); + + // Verify that execFile was called (not exec) and the URL is passed safely + expect(mockExecFile).toHaveBeenCalledWith( + 'powershell.exe', + [ + '-NoProfile', + '-NonInteractive', + '-WindowStyle', + 'Hidden', + '-Command', + `Start-Process '${maliciousUrl.replace(/'/g, "''")}'`, + ], + expect.any(Object), + ); + }); + + it('should handle URLs with special shell characters safely', async () => { + setPlatform('darwin'); + + const urlsWithSpecialChars = [ + 'http://example.com/path?param=value&other=$value', + 'http://example.com/path#fragment;command', + 'http://example.com/$(whoami)', + 'http://example.com/`command`', + 'http://example.com/|pipe', + 'http://example.com/>redirect', + ]; + + for (const url of urlsWithSpecialChars) { + await openBrowserSecurely(url); + // Verify the URL is passed as an argument, not interpreted by shell + expect(mockExecFile).toHaveBeenCalledWith( + 'open', + [url], + expect.any(Object), + ); + } + }); + + it('should properly escape single quotes in URLs on Windows', async () => { + setPlatform('win32'); + + const urlWithSingleQuotes = + "http://example.com/path?name=O'Brien&test='value'"; + await openBrowserSecurely(urlWithSingleQuotes); + + // Verify that single quotes are escaped by doubling them + expect(mockExecFile).toHaveBeenCalledWith( + 'powershell.exe', + [ + '-NoProfile', + '-NonInteractive', + '-WindowStyle', + 'Hidden', + '-Command', + `Start-Process 'http://example.com/path?name=O''Brien&test=''value'''`, + ], + expect.any(Object), + ); + }); + }); + + describe('Platform-specific behavior', () => { + it('should use correct command on macOS', async () => { + setPlatform('darwin'); + await openBrowserSecurely('https://example.com'); + expect(mockExecFile).toHaveBeenCalledWith( + 'open', + ['https://example.com'], + expect.any(Object), + ); + }); + + it('should use PowerShell on Windows', async () => { + setPlatform('win32'); + await openBrowserSecurely('https://example.com'); + expect(mockExecFile).toHaveBeenCalledWith( + 'powershell.exe', + expect.arrayContaining([ + '-Command', + `Start-Process 'https://example.com'`, + ]), + expect.any(Object), + ); + }); + + it('should use xdg-open on Linux', async () => { + setPlatform('linux'); + await openBrowserSecurely('https://example.com'); + expect(mockExecFile).toHaveBeenCalledWith( + 'xdg-open', + ['https://example.com'], + expect.any(Object), + ); + }); + + it('should throw on unsupported platforms', async () => { + setPlatform('aix'); + await expect(openBrowserSecurely('https://example.com')).rejects.toThrow( + 'Unsupported platform', + ); + }); + }); + + describe('Error handling', () => { + it('should handle browser launch failures gracefully', async () => { + setPlatform('darwin'); + mockExecFile.mockRejectedValueOnce(new Error('Command not found')); + + await expect(openBrowserSecurely('https://example.com')).rejects.toThrow( + 'Failed to open browser', + ); + }); + + it('should try fallback browsers on Linux', async () => { + setPlatform('linux'); + + // First call to xdg-open fails + mockExecFile.mockRejectedValueOnce(new Error('Command not found')); + // Second call to gnome-open succeeds + mockExecFile.mockResolvedValueOnce({ stdout: '', stderr: '' }); + + await openBrowserSecurely('https://example.com'); + + expect(mockExecFile).toHaveBeenCalledTimes(2); + expect(mockExecFile).toHaveBeenNthCalledWith( + 1, + 'xdg-open', + ['https://example.com'], + expect.any(Object), + ); + expect(mockExecFile).toHaveBeenNthCalledWith( + 2, + 'gnome-open', + ['https://example.com'], + expect.any(Object), + ); + }); + }); +}); diff --git a/packages/core/src/utils/secure-browser-launcher.ts b/packages/core/src/utils/secure-browser-launcher.ts new file mode 100644 index 00000000..ec8357be --- /dev/null +++ b/packages/core/src/utils/secure-browser-launcher.ts @@ -0,0 +1,188 @@ +/** + * @license + * Copyright 2025 Google LLC + * SPDX-License-Identifier: Apache-2.0 + */ + +import { execFile } from 'node:child_process'; +import { promisify } from 'node:util'; +import { platform } from 'node:os'; +import { URL } from 'node:url'; + +const execFileAsync = promisify(execFile); + +/** + * Validates that a URL is safe to open in a browser. + * Only allows HTTP and HTTPS URLs to prevent command injection. + * + * @param url The URL to validate + * @throws Error if the URL is invalid or uses an unsafe protocol + */ +function validateUrl(url: string): void { + let parsedUrl: URL; + + try { + parsedUrl = new URL(url); + } catch (_error) { + throw new Error(`Invalid URL: ${url}`); + } + + // Only allow HTTP and HTTPS protocols + if (parsedUrl.protocol !== 'http:' && parsedUrl.protocol !== 'https:') { + throw new Error( + `Unsafe protocol: ${parsedUrl.protocol}. Only HTTP and HTTPS are allowed.`, + ); + } + + // Additional validation: ensure no newlines or control characters + // eslint-disable-next-line no-control-regex + if (/[\r\n\x00-\x1f]/.test(url)) { + throw new Error('URL contains invalid characters'); + } +} + +/** + * Opens a URL in the default browser using platform-specific commands. + * This implementation avoids shell injection vulnerabilities by: + * 1. Validating the URL to ensure it's HTTP/HTTPS only + * 2. Using execFile instead of exec to avoid shell interpretation + * 3. Passing the URL as an argument rather than constructing a command string + * + * @param url The URL to open + * @throws Error if the URL is invalid or if opening the browser fails + */ +export async function openBrowserSecurely(url: string): Promise<void> { + // Validate the URL first + validateUrl(url); + + const platformName = platform(); + let command: string; + let args: string[]; + + switch (platformName) { + case 'darwin': + // macOS + command = 'open'; + args = [url]; + break; + + case 'win32': + // Windows - use PowerShell with Start-Process + // This avoids the cmd.exe shell which is vulnerable to injection + command = 'powershell.exe'; + args = [ + '-NoProfile', + '-NonInteractive', + '-WindowStyle', + 'Hidden', + '-Command', + `Start-Process '${url.replace(/'/g, "''")}'`, + ]; + break; + + case 'linux': + case 'freebsd': + case 'openbsd': + // Linux and BSD variants + // Try xdg-open first, fall back to other options + command = 'xdg-open'; + args = [url]; + break; + + default: + throw new Error(`Unsupported platform: ${platformName}`); + } + + const options: Record<string, unknown> = { + // Don't inherit parent's environment to avoid potential issues + env: { + ...process.env, + // Ensure we're not in a shell that might interpret special characters + SHELL: undefined, + }, + // Detach the browser process so it doesn't block + detached: true, + stdio: 'ignore', + }; + + try { + await execFileAsync(command, args, options); + } catch (error) { + // For Linux, try fallback commands if xdg-open fails + if ( + (platformName === 'linux' || + platformName === 'freebsd' || + platformName === 'openbsd') && + command === 'xdg-open' + ) { + const fallbackCommands = [ + 'gnome-open', + 'kde-open', + 'firefox', + 'chromium', + 'google-chrome', + ]; + + for (const fallbackCommand of fallbackCommands) { + try { + await execFileAsync(fallbackCommand, [url], options); + return; // Success! + } catch { + // Try next command + continue; + } + } + } + + // Re-throw the error if all attempts failed + throw new Error( + `Failed to open browser: ${error instanceof Error ? error.message : 'Unknown error'}`, + ); + } +} + +/** + * Checks if the current environment should attempt to launch a browser. + * This is the same logic as in browser.ts for consistency. + * + * @returns True if the tool should attempt to launch a browser + */ +export function shouldLaunchBrowser(): boolean { + // A list of browser names that indicate we should not attempt to open a + // web browser for the user. + const browserBlocklist = ['www-browser']; + const browserEnv = process.env.BROWSER; + if (browserEnv && browserBlocklist.includes(browserEnv)) { + return false; + } + + // Common environment variables used in CI/CD or other non-interactive shells. + if (process.env.CI || process.env.DEBIAN_FRONTEND === 'noninteractive') { + return false; + } + + // The presence of SSH_CONNECTION indicates a remote session. + // We should not attempt to launch a browser unless a display is explicitly available + // (checked below for Linux). + const isSSH = !!process.env.SSH_CONNECTION; + + // On Linux, the presence of a display server is a strong indicator of a GUI. + if (platform() === 'linux') { + // These are environment variables that can indicate a running compositor on Linux. + const displayVariables = ['DISPLAY', 'WAYLAND_DISPLAY', 'MIR_SOCKET']; + const hasDisplay = displayVariables.some((v) => !!process.env[v]); + if (!hasDisplay) { + return false; + } + } + + // If in an SSH session on a non-Linux OS (e.g., macOS), don't launch browser. + // The Linux case is handled above (it's allowed if DISPLAY is set). + if (isSSH && platform() !== 'linux') { + return false; + } + + // For non-Linux OSes, we generally assume a GUI is available + // unless other signals (like SSH) suggest otherwise. + return true; +} |