New browser launcher for MCP OAuth. (#5261)

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');

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:',

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),
+      );
+    });
+  });
+});

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;
+}