From 890bfbfccb26b09ef6ca3d79a02c524f54854755 Mon Sep 17 00:00:00 2001 From: Yufeng He <40085740+he-yufeng@users.noreply.github.com> Date: Wed, 3 Jun 2026 04:57:56 +0800 Subject: [PATCH 1/5] fix(cli): avoid headless browser open crashes --- .../cli/src/ui/commands/bugCommand.test.ts | 20 ++- packages/cli/src/ui/commands/bugCommand.ts | 4 +- .../cli/src/ui/commands/docsCommand.test.ts | 29 ++-- packages/cli/src/ui/commands/docsCommand.ts | 4 +- .../src/ui/commands/insightCommand.test.ts | 29 ++-- .../cli/src/ui/commands/insightCommand.ts | 27 ++-- packages/core/src/index.ts | 1 + .../src/utils/secure-browser-launcher.test.ts | 33 +++++ .../core/src/utils/secure-browser-launcher.ts | 125 ++++++++++++------ 9 files changed, 190 insertions(+), 82 deletions(-) diff --git a/packages/cli/src/ui/commands/bugCommand.test.ts b/packages/cli/src/ui/commands/bugCommand.test.ts index ecdc27c868..f89a8c9a6e 100644 --- a/packages/cli/src/ui/commands/bugCommand.test.ts +++ b/packages/cli/src/ui/commands/bugCommand.test.ts @@ -5,15 +5,23 @@ */ import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; -import open from 'open'; import { bugCommand } from './bugCommand.js'; import { createMockCommandContext } from '../../test-utils/mockCommandContext.js'; import { GIT_COMMIT_INFO } from '../../generated/git-commit.js'; import { AuthType } from '@qwen-code/qwen-code-core'; import * as systemInfoUtils from '../../utils/systemInfo.js'; +const mockOpenBrowserSecurely = vi.hoisted(() => vi.fn()); + // Mock dependencies -vi.mock('open'); +vi.mock('@qwen-code/qwen-code-core', async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + openBrowserSecurely: mockOpenBrowserSecurely, + }; +}); vi.mock('../../utils/systemInfo.js'); describe('bugCommand', () => { @@ -36,6 +44,8 @@ describe('bugCommand', () => { ? GIT_COMMIT_INFO : undefined, }); + mockOpenBrowserSecurely.mockClear(); + mockOpenBrowserSecurely.mockResolvedValue(undefined); vi.stubEnv('SANDBOX', 'qwen-test'); }); @@ -84,7 +94,7 @@ Memory Usage: 100 MB`; }, expect.any(Number), ); - expect(open).toHaveBeenCalledWith(expectedUrl); + expect(mockOpenBrowserSecurely).toHaveBeenCalledWith(expectedUrl); }); it('should use a custom URL template from config if provided', async () => { @@ -128,7 +138,7 @@ Memory Usage: 100 MB`; }, expect.any(Number), ); - expect(open).toHaveBeenCalledWith(expectedUrl); + expect(mockOpenBrowserSecurely).toHaveBeenCalledWith(expectedUrl); }); it('should include Base URL when auth type is OpenAI', async () => { @@ -193,6 +203,6 @@ Memory Usage: 100 MB`; }, expect.any(Number), ); - expect(open).toHaveBeenCalledWith(expectedUrl); + expect(mockOpenBrowserSecurely).toHaveBeenCalledWith(expectedUrl); }); }); diff --git a/packages/cli/src/ui/commands/bugCommand.ts b/packages/cli/src/ui/commands/bugCommand.ts index c561189a2e..2b4a1362a3 100644 --- a/packages/cli/src/ui/commands/bugCommand.ts +++ b/packages/cli/src/ui/commands/bugCommand.ts @@ -4,12 +4,12 @@ * SPDX-License-Identifier: Apache-2.0 */ -import open from 'open'; import { type CommandContext, type SlashCommand, CommandKind, } from './types.js'; +import { openBrowserSecurely } from '@qwen-code/qwen-code-core'; import { MessageType, type HistoryItem } from '../types.js'; import { getExtendedSystemInfo } from '../../utils/systemInfo.js'; import { getSystemInfoFields } from '../../utils/systemInfoFields.js'; @@ -55,7 +55,7 @@ export const bugCommand: SlashCommand = { context.ui.addItem(bugReportItem, Date.now()); try { - await open(bugReportUrl); + await openBrowserSecurely(bugReportUrl); } catch (error) { const errorMessage = error instanceof Error ? error.message : String(error); diff --git a/packages/cli/src/ui/commands/docsCommand.test.ts b/packages/cli/src/ui/commands/docsCommand.test.ts index fce3c29fa5..4678e1b279 100644 --- a/packages/cli/src/ui/commands/docsCommand.test.ts +++ b/packages/cli/src/ui/commands/docsCommand.test.ts @@ -5,24 +5,29 @@ */ import { vi, describe, it, expect, beforeEach, afterEach } from 'vitest'; -import open from 'open'; import { docsCommand } from './docsCommand.js'; import { type CommandContext } from './types.js'; import { createMockCommandContext } from '../../test-utils/mockCommandContext.js'; import { MessageType } from '../types.js'; -// Mock the 'open' library -vi.mock('open', () => ({ - default: vi.fn(), -})); +const mockOpenBrowserSecurely = vi.hoisted(() => vi.fn()); + +vi.mock('@qwen-code/qwen-code-core', async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + openBrowserSecurely: mockOpenBrowserSecurely, + }; +}); describe('docsCommand', () => { let mockContext: CommandContext; beforeEach(() => { // Create a fresh mock context before each test mockContext = createMockCommandContext(); - // Reset the `open` mock - vi.mocked(open).mockClear(); + mockOpenBrowserSecurely.mockClear(); + mockOpenBrowserSecurely.mockResolvedValue(undefined); }); afterEach(() => { @@ -47,7 +52,7 @@ describe('docsCommand', () => { expect.any(Number), ); - expect(open).toHaveBeenCalledWith(docsUrl); + expect(mockOpenBrowserSecurely).toHaveBeenCalledWith(docsUrl); }); it('should only add an info message in a sandbox environment', async () => { @@ -70,7 +75,7 @@ describe('docsCommand', () => { ); // Ensure 'open' was not called in the sandbox - expect(open).not.toHaveBeenCalled(); + expect(mockOpenBrowserSecurely).not.toHaveBeenCalled(); }); it("should not open browser for 'sandbox-exec'", async () => { @@ -93,8 +98,8 @@ describe('docsCommand', () => { expect.any(Number), ); - // 'open' should be called in this specific sandbox case - expect(open).toHaveBeenCalledWith(docsUrl); + // Browser launch should be called in this specific sandbox case. + expect(mockOpenBrowserSecurely).toHaveBeenCalledWith(docsUrl); }); describe('non-interactive mode', () => { @@ -112,7 +117,7 @@ describe('docsCommand', () => { messageType: 'info', content: expect.stringContaining('qwenlm.github.io'), }); - expect(open).not.toHaveBeenCalled(); + expect(mockOpenBrowserSecurely).not.toHaveBeenCalled(); expect(nonInteractiveContext.ui.addItem).not.toHaveBeenCalled(); }); }); diff --git a/packages/cli/src/ui/commands/docsCommand.ts b/packages/cli/src/ui/commands/docsCommand.ts index 6de9c492c3..3afd49920c 100644 --- a/packages/cli/src/ui/commands/docsCommand.ts +++ b/packages/cli/src/ui/commands/docsCommand.ts @@ -4,13 +4,13 @@ * SPDX-License-Identifier: Apache-2.0 */ -import open from 'open'; import process from 'node:process'; import { type CommandContext, type SlashCommand, CommandKind, } from './types.js'; +import { openBrowserSecurely } from '@qwen-code/qwen-code-core'; import { MessageType } from '../types.js'; import { t, getCurrentLanguage } from '../../i18n/index.js'; @@ -57,7 +57,7 @@ export const docsCommand: SlashCommand = { }, Date.now(), ); - await open(docsUrl); + await openBrowserSecurely(docsUrl); } return; }, diff --git a/packages/cli/src/ui/commands/insightCommand.test.ts b/packages/cli/src/ui/commands/insightCommand.test.ts index 22f230fed7..1e8976615a 100644 --- a/packages/cli/src/ui/commands/insightCommand.test.ts +++ b/packages/cli/src/ui/commands/insightCommand.test.ts @@ -6,13 +6,23 @@ import { beforeEach, afterEach, describe, expect, it, vi } from 'vitest'; import path from 'path'; -import open from 'open'; +import { pathToFileURL } from 'node:url'; import { parseInsightMessage, Storage } from '@qwen-code/qwen-code-core'; import { insightCommand } from './insightCommand.js'; import type { CommandContext } from './types.js'; import { createMockCommandContext } from '../../test-utils/mockCommandContext.js'; const mockGenerateStaticInsight = vi.fn(); +const mockOpenBrowserSecurely = vi.hoisted(() => vi.fn()); + +vi.mock('@qwen-code/qwen-code-core', async (importOriginal) => { + const actual = + await importOriginal(); + return { + ...actual, + openBrowserSecurely: mockOpenBrowserSecurely, + }; +}); vi.mock('../../services/insight/generators/StaticInsightGenerator.js', () => ({ StaticInsightGenerator: vi.fn(() => ({ @@ -20,10 +30,6 @@ vi.mock('../../services/insight/generators/StaticInsightGenerator.js', () => ({ })), })); -vi.mock('open', () => ({ - default: vi.fn(), -})); - describe('insightCommand', () => { let mockContext: CommandContext; @@ -32,7 +38,8 @@ describe('insightCommand', () => { mockGenerateStaticInsight.mockResolvedValue( path.resolve('runtime-output', 'insights', 'insight-2026-03-05.html'), ); - vi.mocked(open).mockResolvedValue(undefined as never); + mockOpenBrowserSecurely.mockClear(); + mockOpenBrowserSecurely.mockResolvedValue(undefined); mockContext = createMockCommandContext({ services: { @@ -62,6 +69,12 @@ describe('insightCommand', () => { path.join(Storage.getRuntimeBaseDir(), 'projects'), expect.any(Function), ); + expect(mockOpenBrowserSecurely).toHaveBeenCalledWith( + pathToFileURL( + path.resolve('runtime-output', 'insights', 'insight-2026-03-05.html'), + ).href, + { allowFile: true }, + ); }); it('streams ACP progress messages without waiting for generation to finish', async () => { @@ -198,7 +211,7 @@ describe('insightCommand', () => { expect((result as { content: string }).content).toContain( path.resolve('runtime-output', 'insights', 'insight-2026-03-05.html'), ); - expect(open).not.toHaveBeenCalled(); + expect(mockOpenBrowserSecurely).not.toHaveBeenCalled(); }); it('non_interactive: returns error message when generation fails', async () => { @@ -227,6 +240,6 @@ describe('insightCommand', () => { messageType: 'error', }); expect((result as { content: string }).content).toContain('disk full'); - expect(open).not.toHaveBeenCalled(); + expect(mockOpenBrowserSecurely).not.toHaveBeenCalled(); }); }); diff --git a/packages/cli/src/ui/commands/insightCommand.ts b/packages/cli/src/ui/commands/insightCommand.ts index 6f1ed89150..bd00604f9f 100644 --- a/packages/cli/src/ui/commands/insightCommand.ts +++ b/packages/cli/src/ui/commands/insightCommand.ts @@ -10,14 +10,15 @@ import { MessageType } from '../types.js'; import type { HistoryItemInsightProgress } from '../types.js'; import { t } from '../../i18n/index.js'; import { join } from 'path'; +import { pathToFileURL } from 'node:url'; import { StaticInsightGenerator } from '../../services/insight/generators/StaticInsightGenerator.js'; import { createDebugLogger, encodeInsightProgressMessage, encodeInsightReadyMessage, + openBrowserSecurely, Storage, } from '@qwen-code/qwen-code-core'; -import open from 'open'; const logger = createDebugLogger('DataProcessor'); @@ -215,18 +216,20 @@ export const insightCommand: SlashCommand = { Date.now(), ); - try { - await open(outputPath); + context.ui.addItem( + { + type: MessageType.INFO, + text: t('Opening insights in your browser: {{path}}', { + path: outputPath, + }), + }, + Date.now(), + ); - context.ui.addItem( - { - type: MessageType.INFO, - text: t('Opening insights in your browser: {{path}}', { - path: outputPath, - }), - }, - Date.now(), - ); + try { + await openBrowserSecurely(pathToFileURL(outputPath).href, { + allowFile: true, + }); } catch (browserError) { logger.error('Failed to open browser automatically:', browserError); diff --git a/packages/core/src/index.ts b/packages/core/src/index.ts index 6a9103df5c..8e27236853 100644 --- a/packages/core/src/index.ts +++ b/packages/core/src/index.ts @@ -346,6 +346,7 @@ export { } from './utils/runtimeFetchOptions.js'; export * from './utils/runtimeStatus.js'; export * from './utils/schemaValidator.js'; +export * from './utils/secure-browser-launcher.js'; export * from './utils/shell-utils.js'; export * from './utils/subagentGenerator.js'; export * from './utils/symlink.js'; diff --git a/packages/core/src/utils/secure-browser-launcher.test.ts b/packages/core/src/utils/secure-browser-launcher.test.ts index cd6490330c..b8cd364152 100644 --- a/packages/core/src/utils/secure-browser-launcher.test.ts +++ b/packages/core/src/utils/secure-browser-launcher.test.ts @@ -23,12 +23,14 @@ describe('secure-browser-launcher', () => { vi.clearAllMocks(); mockExecFile.mockResolvedValue({ stdout: '', stderr: '' }); originalPlatform = Object.getOwnPropertyDescriptor(process, 'platform'); + vi.stubEnv('BROWSER', ''); }); afterEach(() => { if (originalPlatform) { Object.defineProperty(process, 'platform', originalPlatform); } + vi.unstubAllEnvs(); }); function setPlatform(platform: string) { @@ -71,6 +73,24 @@ describe('secure-browser-launcher', () => { ); }); + it('should allow file URLs only when explicitly requested', async () => { + setPlatform('linux'); + + await expect(openBrowserSecurely('file:///tmp/report.html')).rejects.toThrow( + 'Unsafe protocol', + ); + + await openBrowserSecurely('file:///tmp/report.html', { + allowFile: true, + }); + + expect(mockExecFile).toHaveBeenCalledWith( + 'xdg-open', + ['file:///tmp/report.html'], + expect.any(Object), + ); + }); + it('should reject invalid URLs', async () => { await expect(openBrowserSecurely('not-a-url')).rejects.toThrow( 'Invalid URL', @@ -198,6 +218,19 @@ describe('secure-browser-launcher', () => { 'Unsupported platform', ); }); + + it('should prefer BROWSER when it is configured', async () => { + setPlatform('linux'); + vi.stubEnv('BROWSER', 'firefox --new-tab'); + + await openBrowserSecurely('https://example.com'); + + expect(mockExecFile).toHaveBeenCalledWith( + 'firefox', + ['--new-tab', 'https://example.com'], + expect.any(Object), + ); + }); }); describe('Error handling', () => { diff --git a/packages/core/src/utils/secure-browser-launcher.ts b/packages/core/src/utils/secure-browser-launcher.ts index 3a6e3219aa..e3c9d94aeb 100644 --- a/packages/core/src/utils/secure-browser-launcher.ts +++ b/packages/core/src/utils/secure-browser-launcher.ts @@ -18,7 +18,7 @@ const execFileAsync = promisify(execFile); * @param url The URL to validate * @throws Error if the URL is invalid or uses an unsafe protocol */ -function validateUrl(url: string): void { +function validateUrl(url: string, { allowFile = false } = {}): void { let parsedUrl: URL; try { @@ -27,10 +27,16 @@ function validateUrl(url: string): void { throw new Error(`Invalid URL: ${url}`); } - // Only allow HTTP and HTTPS protocols - if (parsedUrl.protocol !== 'http:' && parsedUrl.protocol !== 'https:') { + const allowedProtocols = allowFile + ? ['http:', 'https:', 'file:'] + : ['http:', 'https:']; + + // Only allow browser-safe protocols by default. + if (!allowedProtocols.includes(parsedUrl.protocol)) { throw new Error( - `Unsafe protocol: ${parsedUrl.protocol}. Only HTTP and HTTPS are allowed.`, + `Unsafe protocol: ${parsedUrl.protocol}. Only ${allowedProtocols.join( + ', ', + )} are allowed.`, ); } @@ -49,51 +55,67 @@ function validateUrl(url: string): void { * and resolves successfully to prevent application crashes. * * @param url - The URL to open. + * @param options.allowFile - Allow file:// URLs for locally generated reports. * @returns A promise that resolves when the attempt is made (whether successful or logged). */ -export async function openBrowserSecurely(url: string): Promise { +export async function openBrowserSecurely( + url: string, + browserOptions: { allowFile?: boolean } = {}, +): Promise { // Validate the URL first - validateUrl(url); + validateUrl(url, browserOptions); 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 browserEnv = process.env['BROWSER']?.trim(); + const browserBlocklist = ['www-browser']; + if (browserEnv && !browserBlocklist.includes(browserEnv)) { + const browserCommand = parseBrowserCommand(browserEnv); + if (browserCommand) { + command = browserCommand.command; + args = [...browserCommand.args, url]; + } else { + throw new Error('Invalid BROWSER environment variable'); + } + } else { + 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 = { + const execOptions: Record = { // Don't inherit parent's environment to avoid potential issues env: { ...process.env, @@ -106,7 +128,7 @@ export async function openBrowserSecurely(url: string): Promise { }; try { - await execFileAsync(command, args, options); + await execFileAsync(command, args, execOptions); } catch (_error) { // For Linux, try fallback commands if xdg-open fails if ( @@ -126,7 +148,7 @@ export async function openBrowserSecurely(url: string): Promise { for (const fallbackCommand of fallbackCommands) { try { - await execFileAsync(fallbackCommand, [url], options); + await execFileAsync(fallbackCommand, [url], execOptions); return; // Success! } catch { // Try next command @@ -145,6 +167,27 @@ export async function openBrowserSecurely(url: string): Promise { } } +function parseBrowserCommand( + browserEnv: string, +): { command: string; args: string[] } | undefined { + const parts = + browserEnv.match(/"[^"]*"|'[^']*'|[^\s]+/g)?.map((part) => { + if ( + (part.startsWith('"') && part.endsWith('"')) || + (part.startsWith("'") && part.endsWith("'")) + ) { + return part.slice(1, -1); + } + return part; + }) ?? []; + + const [command, ...args] = parts; + if (!command) { + return undefined; + } + return { command, args }; +} + /** * Checks if the current environment should attempt to launch a browser. * This is the same logic as in browser.ts for consistency. From 84fdcca0e575c6d4c50aa242c6cb0a0692e0b693 Mon Sep 17 00:00:00 2001 From: Yufeng He <40085740+he-yufeng@users.noreply.github.com> Date: Thu, 4 Jun 2026 11:07:59 +0800 Subject: [PATCH 2/5] fix(cli): reuse browser launch guard --- .../src/ui/commands/extensionsCommand.test.ts | 45 +++++++++++++++ .../cli/src/ui/commands/extensionsCommand.ts | 4 +- packages/core/src/qwen/qwenOAuth2.test.ts | 56 ++++++++++++------- packages/core/src/qwen/qwenOAuth2.ts | 26 +-------- .../src/utils/secure-browser-launcher.test.ts | 27 ++++++++- .../core/src/utils/secure-browser-launcher.ts | 51 ++++------------- 6 files changed, 120 insertions(+), 89 deletions(-) diff --git a/packages/cli/src/ui/commands/extensionsCommand.test.ts b/packages/cli/src/ui/commands/extensionsCommand.test.ts index de00a2aa83..7d66743047 100644 --- a/packages/cli/src/ui/commands/extensionsCommand.test.ts +++ b/packages/cli/src/ui/commands/extensionsCommand.test.ts @@ -22,11 +22,14 @@ import { parseInstallSource, } from '@qwen-code/qwen-code-core'; +const mockOpenBrowserSecurely = vi.hoisted(() => vi.fn()); + vi.mock('@qwen-code/qwen-code-core', async (importOriginal) => { const actual = await importOriginal(); return { ...actual, + openBrowserSecurely: mockOpenBrowserSecurely, parseInstallSource: vi.fn(), }; }); @@ -46,6 +49,7 @@ describe('extensionsCommand', () => { beforeEach(() => { vi.resetAllMocks(); + mockOpenBrowserSecurely.mockResolvedValue(undefined); mockExtensionManager = createMockExtensionManager(); mockGetExtensions.mockReturnValue([]); mockGetLoadedExtensions.mockReturnValue([]); @@ -64,6 +68,47 @@ describe('extensionsCommand', () => { }); }); + describe('explore', () => { + const exploreAction = extensionsCommand.subCommands?.find( + (cmd) => cmd.name === 'explore', + )?.action; + + if (!exploreAction) { + throw new Error('Explore action not found'); + } + + it('should open the selected extensions gallery through the shared browser helper', async () => { + vi.stubEnv('NODE_ENV', 'development'); + await exploreAction(mockContext, 'Gemini'); + + expect(mockContext.ui.addItem).toHaveBeenCalledWith( + { + type: MessageType.INFO, + text: 'Opening extensions page in your browser: https://geminicli.com/extensions/', + }, + expect.any(Number), + ); + expect(mockOpenBrowserSecurely).toHaveBeenCalledWith( + 'https://geminicli.com/extensions/', + ); + }); + + it('should show the URL when browser launch fails', async () => { + vi.stubEnv('NODE_ENV', 'development'); + mockOpenBrowserSecurely.mockRejectedValue(new Error('no browser')); + + await exploreAction(mockContext, 'ClaudeCode'); + + expect(mockContext.ui.addItem).toHaveBeenCalledWith( + { + type: MessageType.ERROR, + text: 'Failed to open browser. Check out the extensions gallery at https://claudemarketplaces.com/', + }, + expect.any(Number), + ); + }); + }); + describe('default action (manage)', () => { it('should open extensions manager dialog when extensions exist', async () => { if (!extensionsCommand.action) throw new Error('Action not defined'); diff --git a/packages/cli/src/ui/commands/extensionsCommand.ts b/packages/cli/src/ui/commands/extensionsCommand.ts index e65341cba4..09b1482c3f 100644 --- a/packages/cli/src/ui/commands/extensionsCommand.ts +++ b/packages/cli/src/ui/commands/extensionsCommand.ts @@ -14,11 +14,11 @@ import { import { t } from '../../i18n/index.js'; import { ExtensionManager, + openBrowserSecurely, parseInstallSource, createDebugLogger, redactUrlCredentials, } from '@qwen-code/qwen-code-core'; -import open from 'open'; const debugLogger = createDebugLogger('EXTENSIONS_COMMAND'); const EXTENSION_EXPLORE_URL = { @@ -77,7 +77,7 @@ async function exploreAction(context: CommandContext, args: string) { Date.now(), ); try { - await open(extensionsUrl); + await openBrowserSecurely(extensionsUrl); } catch (_error) { context.ui.addItem( { diff --git a/packages/core/src/qwen/qwenOAuth2.test.ts b/packages/core/src/qwen/qwenOAuth2.test.ts index 165f9e4607..8cc9a13738 100644 --- a/packages/core/src/qwen/qwenOAuth2.test.ts +++ b/packages/core/src/qwen/qwenOAuth2.test.ts @@ -6,7 +6,6 @@ import { afterEach, beforeEach, describe, expect, it, vi } from 'vitest'; import { EventEmitter } from 'events'; -import { type ChildProcess } from 'child_process'; import type { Config } from '../config/config.js'; import { generateCodeChallenge, @@ -36,6 +35,8 @@ interface MockSharedTokenManager { clearCache(): void; } +const mockOpenBrowserSecurely = vi.hoisted(() => vi.fn()); + // Mock SharedTokenManager vi.mock('./sharedTokenManager.js', () => ({ SharedTokenManager: class { @@ -91,9 +92,8 @@ vi.mock('./sharedTokenManager.js', () => ({ }, })); -// Mock open -vi.mock('open', () => ({ - default: vi.fn(), +vi.mock('../utils/secure-browser-launcher.js', () => ({ + openBrowserSecurely: mockOpenBrowserSecurely, })); // Mock process.stdout.write @@ -118,6 +118,11 @@ vi.mock('node:fs', () => ({ }, })); +beforeEach(() => { + mockOpenBrowserSecurely.mockReset(); + mockOpenBrowserSecurely.mockResolvedValue(undefined); +}); + describe('PKCE Code Generation', () => { describe('generateCodeVerifier', () => { it('should generate a code verifier with correct length', () => { @@ -1464,6 +1469,7 @@ describe('authWithQwenDeviceFlow - Comprehensive Testing', () => { expect(client).toBeInstanceOf(Object); expect(mockConfig.isBrowserLaunchSuppressed).toHaveBeenCalled(); + expect(mockOpenBrowserSecurely).not.toHaveBeenCalled(); SharedTokenManager.getInstance = originalGetInstance; }); @@ -1494,9 +1500,15 @@ describe('Browser Launch and Error Handling', () => { new Error('No cached credentials'), ); - // Mock open to throw error - const open = await import('open'); - vi.mocked(open.default).mockRejectedValue( + const mockTokenManager = { + getValidCredentials: vi + .fn() + .mockRejectedValue(new Error('No credentials')), + }; + const originalGetInstance = SharedTokenManager.getInstance; + SharedTokenManager.getInstance = vi.fn().mockReturnValue(mockTokenManager); + + mockOpenBrowserSecurely.mockRejectedValue( new Error('Browser launch failed'), ); @@ -1531,27 +1543,26 @@ describe('Browser Launch and Error Handling', () => { ); expect(client).toBeInstanceOf(Object); + expect(mockOpenBrowserSecurely).toHaveBeenCalledWith( + 'https://chat.qwen.ai/device?code=TEST123', + ); + + SharedTokenManager.getInstance = originalGetInstance; }); - it('should handle browser child process error gracefully', async () => { + it('should launch the device flow URL through the shared browser helper', async () => { const { promises: fs } = await import('node:fs'); vi.mocked(fs.readFile).mockRejectedValue( new Error('No cached credentials'), ); - // Mock open to return a child process that will emit error - const open = await import('open'); - const mockChildProcess = { - on: vi.fn((event: string, callback: (error: Error) => void) => { - if (event === 'error') { - // Call the error handler immediately for testing - setTimeout(() => callback(new Error('Process spawn failed')), 0); - } - }), + const mockTokenManager = { + getValidCredentials: vi + .fn() + .mockRejectedValue(new Error('No credentials')), }; - vi.mocked(open.default).mockResolvedValue( - mockChildProcess as unknown as ChildProcess, - ); + const originalGetInstance = SharedTokenManager.getInstance; + SharedTokenManager.getInstance = vi.fn().mockReturnValue(mockTokenManager); const mockAuthResponse = { ok: true, @@ -1584,6 +1595,11 @@ describe('Browser Launch and Error Handling', () => { ); expect(client).toBeInstanceOf(Object); + expect(mockOpenBrowserSecurely).toHaveBeenCalledWith( + 'https://chat.qwen.ai/device?code=TEST123', + ); + + SharedTokenManager.getInstance = originalGetInstance; }); }); diff --git a/packages/core/src/qwen/qwenOAuth2.ts b/packages/core/src/qwen/qwenOAuth2.ts index d6a5fd97dd..7594764114 100644 --- a/packages/core/src/qwen/qwenOAuth2.ts +++ b/packages/core/src/qwen/qwenOAuth2.ts @@ -7,13 +7,12 @@ import crypto from 'crypto'; import path from 'node:path'; import { promises as fs } from 'node:fs'; -import type { ChildProcess } from 'node:child_process'; -import open from 'open'; import { EventEmitter } from 'events'; import type { Config } from '../config/config.js'; import { randomUUID } from 'node:crypto'; import { formatFetchErrorForUser } from '../utils/fetch.js'; import { createDebugLogger } from '../utils/debugLogger.js'; +import { openBrowserSecurely } from '../utils/secure-browser-launcher.js'; import { SharedTokenManager, TokenManagerError, @@ -802,30 +801,9 @@ async function authWithQwenDeviceFlow( // Helper to handle browser launch with error handling const launchBrowser = async (url: string): Promise => { - let childProcess: ChildProcess | undefined; - try { - // Call open and get the process - childProcess = await open(url); - - // CRITICAL FIX: Attach error listener IMMEDIATELY if a process object exists. - // We do this outside the 'if' check scope to ensure it's bound as soon as possible. - if (childProcess && typeof childProcess.on === 'function') { - childProcess.on('error', (err: Error) => { - debugLogger.warn(`Browser launch process error: ${err.message}`); - debugLogger.info(`Please open this URL manually: ${url}`); - }); - - // Optional: Also listen for 'close' or 'exit' if needed for cleanup, - // but 'error' is the main crasher. - } else { - // Fallback: If open() didn't return a valid process object, log a warning - debugLogger.debug( - 'open() did not return a valid child process object.', - ); - } + await openBrowserSecurely(url); } catch (err) { - // Handle synchronous errors or promise rejections from open() const errorMessage = err instanceof Error ? err.message : String(err); debugLogger.warn(`Failed to open browser automatically: ${errorMessage}`); debugLogger.info(`Please open this URL manually: ${url}`); diff --git a/packages/core/src/utils/secure-browser-launcher.test.ts b/packages/core/src/utils/secure-browser-launcher.test.ts index b8cd364152..b3ed5ded2d 100644 --- a/packages/core/src/utils/secure-browser-launcher.test.ts +++ b/packages/core/src/utils/secure-browser-launcher.test.ts @@ -75,10 +75,11 @@ describe('secure-browser-launcher', () => { it('should allow file URLs only when explicitly requested', async () => { setPlatform('linux'); + vi.stubEnv('DISPLAY', ':1'); - await expect(openBrowserSecurely('file:///tmp/report.html')).rejects.toThrow( - 'Unsafe protocol', - ); + await expect( + openBrowserSecurely('file:///tmp/report.html'), + ).rejects.toThrow('Unsafe protocol'); await openBrowserSecurely('file:///tmp/report.html', { allowFile: true, @@ -204,6 +205,7 @@ describe('secure-browser-launcher', () => { it('should use xdg-open on Linux', async () => { setPlatform('linux'); + vi.stubEnv('DISPLAY', ':1'); await openBrowserSecurely('https://example.com'); expect(mockExecFile).toHaveBeenCalledWith( 'xdg-open', @@ -221,6 +223,7 @@ describe('secure-browser-launcher', () => { it('should prefer BROWSER when it is configured', async () => { setPlatform('linux'); + vi.stubEnv('DISPLAY', ':1'); vi.stubEnv('BROWSER', 'firefox --new-tab'); await openBrowserSecurely('https://example.com'); @@ -231,6 +234,23 @@ describe('secure-browser-launcher', () => { expect.any(Object), ); }); + + it('should skip browser launch in headless Linux', async () => { + setPlatform('linux'); + vi.stubEnv('DISPLAY', ''); + vi.stubEnv('WAYLAND_DISPLAY', ''); + vi.stubEnv('MIR_SOCKET', ''); + const consoleSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); + + await openBrowserSecurely('https://example.com'); + + expect(mockExecFile).not.toHaveBeenCalled(); + expect(consoleSpy).toHaveBeenCalledWith( + expect.stringContaining('Please open this URL manually'), + ); + + consoleSpy.mockRestore(); + }); }); describe('Error handling', () => { @@ -255,6 +275,7 @@ describe('secure-browser-launcher', () => { describe('Linux Fallback', () => { it('should try fallback browsers on Linux', async () => { setPlatform('linux'); + vi.stubEnv('DISPLAY', ':1'); mockExecFile.mockRejectedValueOnce(new Error('Command not found')); mockExecFile.mockResolvedValueOnce({ stdout: '', stderr: '' }); diff --git a/packages/core/src/utils/secure-browser-launcher.ts b/packages/core/src/utils/secure-browser-launcher.ts index e3c9d94aeb..279919dcdc 100644 --- a/packages/core/src/utils/secure-browser-launcher.ts +++ b/packages/core/src/utils/secure-browser-launcher.ts @@ -8,6 +8,7 @@ import { execFile } from 'node:child_process'; import { promisify } from 'node:util'; import { platform } from 'node:os'; import { URL } from 'node:url'; +import { shouldAttemptBrowserLaunch } from './browser.js'; const execFileAsync = promisify(execFile); @@ -65,6 +66,15 @@ export async function openBrowserSecurely( // Validate the URL first validateUrl(url, browserOptions); + if (!shouldAttemptBrowserLaunch()) { + /* eslint-disable no-console */ + console.warn( + `Browser launch is not available in this environment. Please open this URL manually: ${url}`, + ); + /* eslint-enable no-console */ + return; + } + const platformName = platform(); let command: string; let args: string[]; @@ -195,44 +205,5 @@ function parseBrowserCommand( * @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; + return shouldAttemptBrowserLaunch(); } From 357000cc18d8181a70d8a6cf210052180f880e12 Mon Sep 17 00:00:00 2001 From: Yufeng He <40085740+he-yufeng@users.noreply.github.com> Date: Thu, 4 Jun 2026 12:24:22 +0800 Subject: [PATCH 3/5] test(core): isolate browser launcher env --- packages/core/src/utils/secure-browser-launcher.test.ts | 3 +++ 1 file changed, 3 insertions(+) diff --git a/packages/core/src/utils/secure-browser-launcher.test.ts b/packages/core/src/utils/secure-browser-launcher.test.ts index b3ed5ded2d..f4002ba9ba 100644 --- a/packages/core/src/utils/secure-browser-launcher.test.ts +++ b/packages/core/src/utils/secure-browser-launcher.test.ts @@ -24,6 +24,9 @@ describe('secure-browser-launcher', () => { mockExecFile.mockResolvedValue({ stdout: '', stderr: '' }); originalPlatform = Object.getOwnPropertyDescriptor(process, 'platform'); vi.stubEnv('BROWSER', ''); + vi.stubEnv('CI', ''); + vi.stubEnv('DEBIAN_FRONTEND', ''); + vi.stubEnv('SSH_CONNECTION', ''); }); afterEach(() => { From d148e8a22278726d2f63fd8df52bf8b2a670da3b Mon Sep 17 00:00:00 2001 From: Yufeng He <40085740+he-yufeng@users.noreply.github.com> Date: Thu, 4 Jun 2026 14:18:57 +0800 Subject: [PATCH 4/5] fix(core): make browser env launches non-blocking --- package-lock.json | 12 +- packages/cli/package.json | 1 - .../cli/src/ui/commands/bugCommand.test.ts | 22 ++ .../cli/src/ui/commands/docsCommand.test.ts | 19 ++ packages/cli/src/ui/commands/docsCommand.ts | 14 +- .../src/ui/commands/insightCommand.test.ts | 31 +- .../cli/src/ui/commands/insightCommand.ts | 23 +- packages/core/package.json | 1 - packages/core/src/utils/browser.ts | 28 +- .../src/utils/secure-browser-launcher.test.ts | 312 +++++++++++++++++- .../core/src/utils/secure-browser-launcher.ts | 217 +++++++++--- 11 files changed, 609 insertions(+), 71 deletions(-) diff --git a/package-lock.json b/package-lock.json index 0052896fdc..7bfe8935f3 100644 --- a/package-lock.json +++ b/package-lock.json @@ -5990,6 +5990,7 @@ "version": "4.1.0", "resolved": "https://registry.npmjs.org/bundle-name/-/bundle-name-4.1.0.tgz", "integrity": "sha512-tjwM5exMg6BGRI+kNmTntNsvdZS1X8BFYS6tnJ2hdH0kVxM6/eVZ2xy+FqStSWvYmtfFMDLIxurorHwDKfDz5Q==", + "dev": true, "license": "MIT", "dependencies": { "run-applescript": "^7.0.0" @@ -6995,6 +6996,7 @@ "version": "5.2.1", "resolved": "https://registry.npmjs.org/default-browser/-/default-browser-5.2.1.tgz", "integrity": "sha512-WY/3TUME0x3KPYdRRxEJJvXRHV4PyPoUsxtZa78lwItwRQRHhd2U9xOscaT/YTf8uCXIAjeJOFBVEh/7FtD8Xg==", + "dev": true, "license": "MIT", "dependencies": { "bundle-name": "^4.1.0", @@ -7011,6 +7013,7 @@ "version": "5.0.0", "resolved": "https://registry.npmjs.org/default-browser-id/-/default-browser-id-5.0.0.tgz", "integrity": "sha512-A6p/pu/6fyBcA1TRz/GqWYPViplrftcW2gZC9q79ngNCKAeR/X3gcEdXQHl4KNXV+3wgIJ1CPkJQ3IHM6lcsyA==", + "dev": true, "license": "MIT", "engines": { "node": ">=18" @@ -7041,6 +7044,7 @@ "version": "3.0.0", "resolved": "https://registry.npmjs.org/define-lazy-prop/-/define-lazy-prop-3.0.0.tgz", "integrity": "sha512-N+MeXYoqr3pOgn8xfyRPREN7gHakLYjhsHhWGT3fWAiL4IkAt0iDw14QiiEm2bE30c5XX5q0FtAA3CK5f9/BUg==", + "dev": true, "license": "MIT", "engines": { "node": ">=12" @@ -10300,6 +10304,7 @@ "version": "3.0.0", "resolved": "https://registry.npmjs.org/is-docker/-/is-docker-3.0.0.tgz", "integrity": "sha512-eljcgEDlEns/7AXFosB5K/2nCM4P7FQPkGc/DWLy5rmFEWvZayGrik1d9/QIY5nJ4f9YsVvBkA6kJpHn9rISdQ==", + "dev": true, "license": "MIT", "bin": { "is-docker": "cli.js" @@ -10397,6 +10402,7 @@ "version": "1.0.0", "resolved": "https://registry.npmjs.org/is-inside-container/-/is-inside-container-1.0.0.tgz", "integrity": "sha512-KIYLCCJghfHZxqjYBE7rEy0OBuTd5xCHS7tHVgvCLkx7StIoaxwNW3hCALgEUjFfeRk+MG/Qxmp/vtETEF3tRA==", + "dev": true, "license": "MIT", "dependencies": { "is-docker": "^3.0.0" @@ -10686,6 +10692,7 @@ "version": "3.1.0", "resolved": "https://registry.npmjs.org/is-wsl/-/is-wsl-3.1.0.tgz", "integrity": "sha512-UcVfVfaK4Sc4m7X3dUSoHoozQGBEFeDC+zVo06t98xe8CzHSZZBekNXH+tu0NalHolcJ/QAGqS46Hef7QXBIMw==", + "dev": true, "license": "MIT", "dependencies": { "is-inside-container": "^1.0.0" @@ -12580,6 +12587,7 @@ "version": "10.2.0", "resolved": "https://registry.npmjs.org/open/-/open-10.2.0.tgz", "integrity": "sha512-YgBpdJHPyQ2UE5x+hlSXcnejzAvD0b22U2OuAP+8OnlJT+PjWPxtgmGqKKc+RgTM63U9gN0YzrYc71R2WT/hTA==", + "dev": true, "license": "MIT", "dependencies": { "default-browser": "^5.2.1", @@ -14199,6 +14207,7 @@ "version": "7.0.0", "resolved": "https://registry.npmjs.org/run-applescript/-/run-applescript-7.0.0.tgz", "integrity": "sha512-9by4Ij99JUr/MCFBUkDKLWK3G9HVXmabKz9U5MlIAIuvuzkiOicRYs8XJLxX+xahD+mLiiCYDqF9dKAgtzKP1A==", + "dev": true, "license": "MIT", "engines": { "node": ">=18" @@ -16839,6 +16848,7 @@ "version": "0.1.0", "resolved": "https://registry.npmjs.org/wsl-utils/-/wsl-utils-0.1.0.tgz", "integrity": "sha512-h3Fbisa2nKGPxCpm89Hk33lBLsnaGBvctQopaBSOW/uIs6FTe1ATyAnKFJrzVs9vpGdsTe73WF3V4lIsk4Gacw==", + "dev": true, "license": "MIT", "dependencies": { "is-wsl": "^3.1.0" @@ -17192,7 +17202,6 @@ "ink-link": "^4.1.0", "ink-spinner": "^5.0.0", "lowlight": "^3.3.0", - "open": "^10.1.2", "p-limit": "^7.3.0", "prompts": "^2.4.2", "react": "^19.2.4", @@ -17627,7 +17636,6 @@ "marked": "^15.0.12", "mime": "4.0.7", "mnemonist": "^0.40.3", - "open": "^10.1.2", "openai": "5.11.0", "picomatch": "^4.0.1", "prompts": "^2.4.2", diff --git a/packages/cli/package.json b/packages/cli/package.json index a8f615c051..5224968e6a 100644 --- a/packages/cli/package.json +++ b/packages/cli/package.json @@ -67,7 +67,6 @@ "ink-link": "^4.1.0", "ink-spinner": "^5.0.0", "lowlight": "^3.3.0", - "open": "^10.1.2", "p-limit": "^7.3.0", "prompts": "^2.4.2", "react": "^19.2.4", diff --git a/packages/cli/src/ui/commands/bugCommand.test.ts b/packages/cli/src/ui/commands/bugCommand.test.ts index f89a8c9a6e..6eb474c034 100644 --- a/packages/cli/src/ui/commands/bugCommand.test.ts +++ b/packages/cli/src/ui/commands/bugCommand.test.ts @@ -205,4 +205,26 @@ Memory Usage: 100 MB`; ); expect(mockOpenBrowserSecurely).toHaveBeenCalledWith(expectedUrl); }); + + it('should report browser launch failures without failing the command', async () => { + mockOpenBrowserSecurely.mockRejectedValueOnce(new Error('browser failed')); + const mockContext = createMockCommandContext({ + services: { + config: { + getBugCommand: () => undefined, + }, + }, + }); + + if (!bugCommand.action) throw new Error('Action is not defined'); + await bugCommand.action(mockContext, 'Browser failure'); + + expect(mockContext.ui.addItem).toHaveBeenCalledWith( + expect.objectContaining({ + type: 'error', + text: 'Could not open URL in browser: browser failed', + }), + expect.any(Number), + ); + }); }); diff --git a/packages/cli/src/ui/commands/docsCommand.test.ts b/packages/cli/src/ui/commands/docsCommand.test.ts index 4678e1b279..c4e5b83c81 100644 --- a/packages/cli/src/ui/commands/docsCommand.test.ts +++ b/packages/cli/src/ui/commands/docsCommand.test.ts @@ -102,6 +102,25 @@ describe('docsCommand', () => { expect(mockOpenBrowserSecurely).toHaveBeenCalledWith(docsUrl); }); + it('should show the docs URL when browser opening throws unexpectedly', async () => { + if (!docsCommand.action) { + throw new Error('docsCommand must have an action.'); + } + + const docsUrl = 'https://qwenlm.github.io/qwen-code-docs/en'; + mockOpenBrowserSecurely.mockRejectedValueOnce(new Error('bad url')); + + await docsCommand.action(mockContext, ''); + + expect(mockContext.ui.addItem).toHaveBeenCalledWith( + { + type: MessageType.ERROR, + text: `Failed to open browser. View documentation at ${docsUrl}`, + }, + expect.any(Number), + ); + }); + describe('non-interactive mode', () => { it('should return docs URL without opening browser', async () => { if (!docsCommand.action) throw new Error('Command has no action'); diff --git a/packages/cli/src/ui/commands/docsCommand.ts b/packages/cli/src/ui/commands/docsCommand.ts index 3afd49920c..754e3a2015 100644 --- a/packages/cli/src/ui/commands/docsCommand.ts +++ b/packages/cli/src/ui/commands/docsCommand.ts @@ -57,7 +57,19 @@ export const docsCommand: SlashCommand = { }, Date.now(), ); - await openBrowserSecurely(docsUrl); + try { + await openBrowserSecurely(docsUrl); + } catch (_error) { + context.ui.addItem( + { + type: MessageType.ERROR, + text: t('Failed to open browser. View documentation at {{url}}', { + url: docsUrl, + }), + }, + Date.now(), + ); + } } return; }, diff --git a/packages/cli/src/ui/commands/insightCommand.test.ts b/packages/cli/src/ui/commands/insightCommand.test.ts index 1e8976615a..f1e6586ea2 100644 --- a/packages/cli/src/ui/commands/insightCommand.test.ts +++ b/packages/cli/src/ui/commands/insightCommand.test.ts @@ -73,8 +73,37 @@ describe('insightCommand', () => { pathToFileURL( path.resolve('runtime-output', 'insights', 'insight-2026-03-05.html'), ).href, - { allowFile: true }, + { + allowFile: true, + allowedFilePaths: [ + path.resolve('runtime-output', 'insights', 'insight-2026-03-05.html'), + ], + }, + ); + }); + + it('shows the generated file path before attempting to open the browser', async () => { + if (!insightCommand.action) { + throw new Error('insight command must have action'); + } + + await insightCommand.action(mockContext, ''); + + const messages = vi + .mocked(mockContext.ui.addItem) + .mock.calls.map(([item]) => + 'text' in item && typeof item.text === 'string' ? item.text : '', + ); + expect(messages).toContain( + `Insights generated at: ${path.resolve( + 'runtime-output', + 'insights', + 'insight-2026-03-05.html', + )}. If the browser does not open automatically, open this file manually.`, ); + expect( + messages.some((message) => message.includes('Opening insights')), + ).toBe(false); }); it('streams ACP progress messages without waiting for generation to finish', async () => { diff --git a/packages/cli/src/ui/commands/insightCommand.ts b/packages/cli/src/ui/commands/insightCommand.ts index bd00604f9f..d25489a4c5 100644 --- a/packages/cli/src/ui/commands/insightCommand.ts +++ b/packages/cli/src/ui/commands/insightCommand.ts @@ -219,9 +219,12 @@ export const insightCommand: SlashCommand = { context.ui.addItem( { type: MessageType.INFO, - text: t('Opening insights in your browser: {{path}}', { - path: outputPath, - }), + text: t( + 'Insights generated at: {{path}}. If the browser does not open automatically, open this file manually.', + { + path: outputPath, + }, + ), }, Date.now(), ); @@ -229,22 +232,10 @@ export const insightCommand: SlashCommand = { try { await openBrowserSecurely(pathToFileURL(outputPath).href, { allowFile: true, + allowedFilePaths: [outputPath], }); } catch (browserError) { logger.error('Failed to open browser automatically:', browserError); - - context.ui.addItem( - { - type: MessageType.INFO, - text: t( - 'Insights generated at: {{path}}. Please open this file in your browser.', - { - path: outputPath, - }, - ), - }, - Date.now(), - ); } context.ui.setDebugMessage(t('Insights ready.')); diff --git a/packages/core/package.json b/packages/core/package.json index 2d16f56dd6..575756833a 100644 --- a/packages/core/package.json +++ b/packages/core/package.json @@ -61,7 +61,6 @@ "marked": "^15.0.12", "mime": "4.0.7", "mnemonist": "^0.40.3", - "open": "^10.1.2", "openai": "5.11.0", "picomatch": "^4.0.1", "prompts": "^2.4.2", diff --git a/packages/core/src/utils/browser.ts b/packages/core/src/utils/browser.ts index 35ee74611d..c1ea5983ab 100644 --- a/packages/core/src/utils/browser.ts +++ b/packages/core/src/utils/browser.ts @@ -4,6 +4,17 @@ * SPDX-License-Identifier: Apache-2.0 */ +const browserBlocklist = ['www-browser']; + +export function isBrowserCommandBlocked(command: string): boolean { + const commandName = command.replace(/\\/g, '/').split('/').pop(); + return !!commandName && browserBlocklist.includes(commandName); +} + +type BrowserLaunchEnvironmentOptions = { + ignoreBrowserBlocklist?: boolean; +}; + /** * Determines if we should attempt to launch a browser for authentication * based on the user's environment. @@ -11,12 +22,17 @@ * This is an adaptation of the logic from the Google Cloud SDK. * @returns True if the tool should attempt to launch a browser. */ -export function shouldAttemptBrowserLaunch(): 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)) { +export function shouldAttemptBrowserLaunch( + options: BrowserLaunchEnvironmentOptions = {}, +): boolean { + const browserEnv = process.env['BROWSER']?.trim(); + const browserCommand = browserEnv?.match(/^\S+/)?.[0]; + if ( + !options.ignoreBrowserBlocklist && + process.platform !== 'win32' && + browserCommand && + isBrowserCommandBlocked(browserCommand) + ) { return false; } // Common environment variables used in CI/CD or other non-interactive shells. diff --git a/packages/core/src/utils/secure-browser-launcher.test.ts b/packages/core/src/utils/secure-browser-launcher.test.ts index f4002ba9ba..48127381b1 100644 --- a/packages/core/src/utils/secure-browser-launcher.test.ts +++ b/packages/core/src/utils/secure-browser-launcher.test.ts @@ -5,13 +5,35 @@ */ import { describe, it, expect, beforeEach, afterEach, vi } from 'vitest'; +import { resolve } from 'node:path'; +import { pathToFileURL } from 'node:url'; import { openBrowserSecurely } from './secure-browser-launcher.js'; // Create mock function using vi.hoisted const mockExecFile = vi.hoisted(() => vi.fn()); +const mockSpawn = vi.hoisted(() => + vi.fn(() => { + const child: { + once: ReturnType; + unref: ReturnType; + } = { + once: vi.fn((event: string, callback: () => void) => { + if (event === 'spawn') { + callback(); + } + return child; + }), + unref: vi.fn(), + }; + return child; + }), +); // Mock modules -vi.mock('node:child_process'); +vi.mock('node:child_process', () => ({ + execFile: vi.fn(), + spawn: mockSpawn, +})); vi.mock('node:util', () => ({ promisify: () => mockExecFile, })); @@ -22,6 +44,21 @@ describe('secure-browser-launcher', () => { beforeEach(() => { vi.clearAllMocks(); mockExecFile.mockResolvedValue({ stdout: '', stderr: '' }); + mockSpawn.mockImplementation(() => { + const child: { + once: ReturnType; + unref: ReturnType; + } = { + once: vi.fn((event: string, callback: () => void) => { + if (event === 'spawn') { + callback(); + } + return child; + }), + unref: vi.fn(), + }; + return child; + }); originalPlatform = Object.getOwnPropertyDescriptor(process, 'platform'); vi.stubEnv('BROWSER', ''); vi.stubEnv('CI', ''); @@ -76,25 +113,46 @@ describe('secure-browser-launcher', () => { ); }); - it('should allow file URLs only when explicitly requested', async () => { + it('should allow file URLs only when explicitly requested with an allow-list', async () => { setPlatform('linux'); vi.stubEnv('DISPLAY', ':1'); + const filePath = resolve('report.html'); + const fileUrl = pathToFileURL(filePath).href; await expect( openBrowserSecurely('file:///tmp/report.html'), ).rejects.toThrow('Unsafe protocol'); - await openBrowserSecurely('file:///tmp/report.html', { + await expect( + openBrowserSecurely(fileUrl, { + allowFile: true, + }), + ).rejects.toThrow('allowedFilePaths is required'); + + await openBrowserSecurely(fileUrl, { allowFile: true, + allowedFilePaths: [filePath], }); expect(mockExecFile).toHaveBeenCalledWith( 'xdg-open', - ['file:///tmp/report.html'], + [fileUrl], expect.any(Object), ); }); + it('should restrict file URLs to the caller allow-list when provided', async () => { + const allowedPath = resolve('report.html'); + const otherPath = resolve('other.html'); + + await expect( + openBrowserSecurely(pathToFileURL(otherPath).href, { + allowFile: true, + allowedFilePaths: [allowedPath], + }), + ).rejects.toThrow('allowed file set'); + }); + it('should reject invalid URLs', async () => { await expect(openBrowserSecurely('not-a-url')).rejects.toThrow( 'Invalid URL', @@ -231,11 +289,239 @@ describe('secure-browser-launcher', () => { await openBrowserSecurely('https://example.com'); + expect(mockSpawn).toHaveBeenCalledWith( + 'firefox', + ['--new-tab', 'https://example.com'], + expect.objectContaining({ + detached: true, + stdio: 'ignore', + }), + ); + expect(mockExecFile).not.toHaveBeenCalled(); + }); + + it('should substitute the BROWSER placeholder instead of appending the URL', async () => { + setPlatform('linux'); + vi.stubEnv('DISPLAY', ':1'); + vi.stubEnv('BROWSER', 'firefox --new-tab %s'); + + await openBrowserSecurely('https://example.com'); + + expect(mockSpawn).toHaveBeenCalledWith( + 'firefox', + ['--new-tab', 'https://example.com'], + expect.any(Object), + ); + }); + + it('should substitute BROWSER placeholders literally', async () => { + setPlatform('linux'); + vi.stubEnv('DISPLAY', ':1'); + vi.stubEnv('BROWSER', 'firefox --new-tab %s'); + const url = 'https://example.com/callback?code=$&state=abc'; + + await openBrowserSecurely(url); + + expect(mockSpawn).toHaveBeenCalledWith( + 'firefox', + ['--new-tab', url], + expect.any(Object), + ); + }); + + it('should parse quoted arguments inside BROWSER tokens', async () => { + setPlatform('linux'); + vi.stubEnv('DISPLAY', ':1'); + vi.stubEnv( + 'BROWSER', + 'chromium --user-data-dir="/tmp/my profile" --new-tab %s', + ); + + await openBrowserSecurely('https://example.com'); + + expect(mockSpawn).toHaveBeenCalledWith( + 'chromium', + ['--user-data-dir=/tmp/my profile', '--new-tab', 'https://example.com'], + expect.any(Object), + ); + }); + + it('should parse quoted command paths in BROWSER', async () => { + setPlatform('linux'); + vi.stubEnv('DISPLAY', ':1'); + vi.stubEnv('BROWSER', '"/opt/my browser/chromium" --new-tab %s'); + + await openBrowserSecurely('https://example.com'); + + expect(mockSpawn).toHaveBeenCalledWith( + '/opt/my browser/chromium', + ['--new-tab', 'https://example.com'], + expect.any(Object), + ); + expect(mockExecFile).not.toHaveBeenCalled(); + }); + + it('should let an explicit BROWSER override headless Linux detection', async () => { + setPlatform('linux'); + vi.stubEnv('DISPLAY', ''); + vi.stubEnv('WAYLAND_DISPLAY', ''); + vi.stubEnv('MIR_SOCKET', ''); + vi.stubEnv('BROWSER', 'firefox'); + + await openBrowserSecurely('https://example.com'); + + expect(mockSpawn).toHaveBeenCalledWith( + 'firefox', + ['https://example.com'], + expect.any(Object), + ); + expect(mockExecFile).not.toHaveBeenCalled(); + }); + + it('should fall back to the platform opener for blocklisted BROWSER values', async () => { + setPlatform('linux'); + vi.stubEnv('DISPLAY', ':1'); + vi.stubEnv('BROWSER', 'www-browser --headless'); + + await openBrowserSecurely('https://example.com'); + + expect(mockSpawn).not.toHaveBeenCalled(); expect(mockExecFile).toHaveBeenCalledWith( + 'xdg-open', + ['https://example.com'], + expect.any(Object), + ); + }); + + it('should block BROWSER values by command basename before using the platform opener', async () => { + setPlatform('linux'); + vi.stubEnv('DISPLAY', ':1'); + vi.stubEnv('BROWSER', '/usr/bin/www-browser --headless'); + + await openBrowserSecurely('https://example.com'); + + expect(mockSpawn).not.toHaveBeenCalled(); + expect(mockExecFile).toHaveBeenCalledWith( + 'xdg-open', + ['https://example.com'], + expect.any(Object), + ); + }); + + it('should still skip blocklisted BROWSER values in headless Linux', async () => { + setPlatform('linux'); + vi.stubEnv('DISPLAY', ''); + vi.stubEnv('WAYLAND_DISPLAY', ''); + vi.stubEnv('MIR_SOCKET', ''); + vi.stubEnv('BROWSER', 'www-browser'); + const consoleSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); + + await openBrowserSecurely('https://example.com'); + + expect(mockSpawn).not.toHaveBeenCalled(); + expect(mockExecFile).not.toHaveBeenCalled(); + expect(consoleSpy).toHaveBeenCalledWith( + expect.stringContaining('Please open this URL manually'), + ); + + consoleSpy.mockRestore(); + }); + + it('should ignore BROWSER on Windows and keep the PowerShell opener', async () => { + setPlatform('win32'); + vi.stubEnv('BROWSER', 'firefox --new-tab'); + + await openBrowserSecurely('https://example.com'); + + expect(mockSpawn).not.toHaveBeenCalled(); + expect(mockExecFile).toHaveBeenCalledWith( + 'powershell.exe', + expect.arrayContaining([ + '-Command', + `Start-Process 'https://example.com'`, + ]), + expect.any(Object), + ); + }); + + it('should ignore blocklisted BROWSER values on Windows', async () => { + setPlatform('win32'); + vi.stubEnv('BROWSER', 'www-browser'); + + await openBrowserSecurely('https://example.com'); + + expect(mockSpawn).not.toHaveBeenCalled(); + expect(mockExecFile).toHaveBeenCalledWith( + 'powershell.exe', + expect.arrayContaining([ + '-Command', + `Start-Process 'https://example.com'`, + ]), + expect.any(Object), + ); + }); + + it('should fall back to the platform opener for invalid BROWSER values', async () => { + setPlatform('darwin'); + vi.stubEnv('BROWSER', '"'); + const consoleSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); + + await openBrowserSecurely('https://example.com'); + + expect(mockSpawn).not.toHaveBeenCalled(); + expect(mockExecFile).toHaveBeenCalledWith( + 'open', + ['https://example.com'], + expect.any(Object), + ); + expect(consoleSpy).toHaveBeenCalledWith( + expect.stringContaining('Invalid BROWSER environment variable'), + ); + + consoleSpy.mockRestore(); + }); + + it('should fall back to the platform opener when explicit BROWSER launch fails', async () => { + setPlatform('linux'); + vi.stubEnv('DISPLAY', ':1'); + vi.stubEnv('BROWSER', 'firefox --new-tab'); + mockExecFile.mockResolvedValueOnce({ stdout: '', stderr: '' }); + mockSpawn.mockImplementationOnce(() => { + const child: { + once: ReturnType; + unref: ReturnType; + } = { + once: vi.fn((event: string, callback: (error?: Error) => void) => { + if (event === 'error') { + callback(new Error('spawn failed')); + } + return child; + }), + unref: vi.fn(), + }; + return child; + }); + const consoleSpy = vi.spyOn(console, 'warn').mockImplementation(() => {}); + + await expect( + openBrowserSecurely('https://example.com'), + ).resolves.toBeUndefined(); + + expect(mockSpawn).toHaveBeenCalledWith( 'firefox', ['--new-tab', 'https://example.com'], expect.any(Object), ); + expect(mockExecFile).toHaveBeenCalledWith( + 'xdg-open', + ['https://example.com'], + expect.any(Object), + ); + expect(consoleSpy).toHaveBeenCalledWith( + expect.stringContaining('Failed to open BROWSER command firefox'), + ); + + consoleSpy.mockRestore(); }); it('should skip browser launch in headless Linux', async () => { @@ -299,5 +585,23 @@ describe('secure-browser-launcher', () => { expect.any(Object), ); }); + + it('should detach real browser fallback commands', async () => { + setPlatform('linux'); + vi.stubEnv('DISPLAY', ':1'); + + mockExecFile + .mockRejectedValueOnce(new Error('xdg-open missing')) + .mockRejectedValueOnce(new Error('gnome-open missing')) + .mockRejectedValueOnce(new Error('kde-open missing')); + + await openBrowserSecurely('https://example.com'); + + expect(mockSpawn).toHaveBeenCalledWith( + 'firefox', + ['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 index 279919dcdc..3100db8bc5 100644 --- a/packages/core/src/utils/secure-browser-launcher.ts +++ b/packages/core/src/utils/secure-browser-launcher.ts @@ -4,22 +4,35 @@ * SPDX-License-Identifier: Apache-2.0 */ -import { execFile } from 'node:child_process'; +import { execFile, spawn, type SpawnOptions } from 'node:child_process'; import { promisify } from 'node:util'; import { platform } from 'node:os'; -import { URL } from 'node:url'; -import { shouldAttemptBrowserLaunch } from './browser.js'; +import { resolve } from 'node:path'; +import { URL, fileURLToPath } from 'node:url'; +import { + isBrowserCommandBlocked, + shouldAttemptBrowserLaunch, +} from './browser.js'; const execFileAsync = promisify(execFile); +type BrowserLaunchOptions = { + allowFile?: boolean; + allowedFilePaths?: string[]; +}; + /** * Validates that a URL is safe to open in a browser. - * Only allows HTTP and HTTPS URLs to prevent command injection. + * Only allows HTTP and HTTPS URLs by default. file:// URLs are allowed only + * when a caller opts in for locally generated reports. * * @param url The URL to validate * @throws Error if the URL is invalid or uses an unsafe protocol */ -function validateUrl(url: string, { allowFile = false } = {}): void { +function validateUrl( + url: string, + { allowFile = false, allowedFilePaths }: BrowserLaunchOptions = {}, +): void { let parsedUrl: URL; try { @@ -41,6 +54,17 @@ function validateUrl(url: string, { allowFile = false } = {}): void { ); } + if (parsedUrl.protocol === 'file:' && allowFile) { + if (!allowedFilePaths?.length) { + throw new Error('allowedFilePaths is required when allowFile is true'); + } + const requestedPath = resolve(fileURLToPath(parsedUrl)); + const allowed = allowedFilePaths.map((filePath) => resolve(filePath)); + if (!allowed.includes(requestedPath)) { + throw new Error('File URL is not in the allowed file set'); + } + } + // Additional validation: ensure no newlines or control characters // eslint-disable-next-line no-control-regex if (/[\r\n\x00-\x1f]/.test(url)) { @@ -61,12 +85,35 @@ function validateUrl(url: string, { allowFile = false } = {}): void { */ export async function openBrowserSecurely( url: string, - browserOptions: { allowFile?: boolean } = {}, + browserOptions: BrowserLaunchOptions = {}, ): Promise { // Validate the URL first validateUrl(url, browserOptions); - if (!shouldAttemptBrowserLaunch()) { + const platformName = platform(); + let command: string; + let args: string[]; + + const browserEnv = process.env['BROWSER']?.trim(); + const browserCommand = + platformName === 'win32' ? undefined : buildBrowserCommand(browserEnv, url); + + if (browserCommand) { + try { + await launchDetached(browserCommand.command, browserCommand.args); + return; + } catch (_error) { + /* eslint-disable no-console */ + console.warn( + `Failed to open BROWSER command ${browserCommand.command}: ${formatLaunchError( + _error, + )}. Falling back to the platform browser opener.`, + ); + /* eslint-enable no-console */ + } + } + + if (!shouldAttemptBrowserLaunch({ ignoreBrowserBlocklist: true })) { /* eslint-disable no-console */ console.warn( `Browser launch is not available in this environment. Please open this URL manually: ${url}`, @@ -75,21 +122,7 @@ export async function openBrowserSecurely( return; } - const platformName = platform(); - let command: string; - let args: string[]; - - const browserEnv = process.env['BROWSER']?.trim(); - const browserBlocklist = ['www-browser']; - if (browserEnv && !browserBlocklist.includes(browserEnv)) { - const browserCommand = parseBrowserCommand(browserEnv); - if (browserCommand) { - command = browserCommand.command; - args = [...browserCommand.args, url]; - } else { - throw new Error('Invalid BROWSER environment variable'); - } - } else { + { switch (platformName) { case 'darwin': // macOS @@ -148,17 +181,21 @@ export async function openBrowserSecurely( command === 'xdg-open' ) { const fallbackCommands = [ - 'gnome-open', - 'kde-open', - 'firefox', - 'chromium', - 'google-chrome', - 'microsoft-edge', + { command: 'gnome-open', detached: false }, + { command: 'kde-open', detached: false }, + { command: 'firefox', detached: true }, + { command: 'chromium', detached: true }, + { command: 'google-chrome', detached: true }, + { command: 'microsoft-edge', detached: true }, ]; - for (const fallbackCommand of fallbackCommands) { + for (const { command: fallbackCommand, detached } of fallbackCommands) { try { - await execFileAsync(fallbackCommand, [url], execOptions); + if (detached) { + await launchDetached(fallbackCommand, [url]); + } else { + await execFileAsync(fallbackCommand, [url], execOptions); + } return; // Success! } catch { // Try next command @@ -177,19 +214,121 @@ export async function openBrowserSecurely( } } +async function launchDetached(command: string, args: string[]): Promise { + const spawnOptions: SpawnOptions = { + env: { + ...process.env, + SHELL: undefined, + }, + detached: true, + stdio: 'ignore', + }; + + await new Promise((resolve, reject) => { + const child = spawn(command, args, spawnOptions); + let settled = false; + + child.once('error', (error) => { + if (settled) { + return; + } + settled = true; + reject(error); + }); + + child.once('spawn', () => { + if (settled) { + return; + } + settled = true; + child.unref(); + resolve(); + }); + }); +} + +function formatLaunchError(error: unknown): string { + return error instanceof Error ? error.message : String(error); +} + +function buildBrowserCommand( + browserEnv: string | undefined, + url: string, +): { command: string; args: string[] } | undefined { + if (!browserEnv) { + return undefined; + } + + const browserCommand = parseBrowserCommand(browserEnv); + if (!browserCommand) { + /* eslint-disable no-console */ + console.warn( + 'Invalid BROWSER environment variable, falling back to platform default.', + ); + /* eslint-enable no-console */ + return undefined; + } + + if (isBrowserCommandBlocked(browserCommand.command)) { + return undefined; + } + + let usedPlaceholder = false; + const args = browserCommand.args.map((arg) => { + if (!arg.includes('%s')) { + return arg; + } + usedPlaceholder = true; + return arg.replace(/%s/g, () => url); + }); + + if (!usedPlaceholder) { + args.push(url); + } + + return { command: browserCommand.command, args }; +} + function parseBrowserCommand( browserEnv: string, ): { command: string; args: string[] } | undefined { - const parts = - browserEnv.match(/"[^"]*"|'[^']*'|[^\s]+/g)?.map((part) => { - if ( - (part.startsWith('"') && part.endsWith('"')) || - (part.startsWith("'") && part.endsWith("'")) - ) { - return part.slice(1, -1); + const parts: string[] = []; + let current = ''; + let quote: '"' | "'" | undefined; + + for (const char of browserEnv) { + if (quote) { + if (char === quote) { + quote = undefined; + } else { + current += char; + } + continue; + } + + if (char === '"' || char === "'") { + quote = char; + continue; + } + + if (/\s/.test(char)) { + if (current) { + parts.push(current); + current = ''; } - return part; - }) ?? []; + continue; + } + + current += char; + } + + if (quote) { + return undefined; + } + + if (current) { + parts.push(current); + } const [command, ...args] = parts; if (!command) { From 72b35efea021f885b8cdb3f3eab233d4d6749b17 Mon Sep 17 00:00:00 2001 From: Yufeng He <40085740+he-yufeng@users.noreply.github.com> Date: Sun, 7 Jun 2026 05:34:52 +0800 Subject: [PATCH 5/5] fix(core): handle LSP stdio write failures --- packages/core/src/lsp/LspConnectionFactory.ts | 18 ++++++++++++++++-- 1 file changed, 16 insertions(+), 2 deletions(-) diff --git a/packages/core/src/lsp/LspConnectionFactory.ts b/packages/core/src/lsp/LspConnectionFactory.ts index 364474b18c..f89c44363d 100644 --- a/packages/core/src/lsp/LspConnectionFactory.ts +++ b/packages/core/src/lsp/LspConnectionFactory.ts @@ -89,7 +89,11 @@ class JsonRpcConnection { } this.disposed = true; this.disposePending(); - this.disposer?.(); + try { + this.disposer?.(); + } catch (error) { + debugLogger.warn('LSP disposer failed:', error); + } } private sendRequest(method: string, params: unknown): Promise { @@ -231,7 +235,12 @@ class JsonRpcConnection { } const json = JSON.stringify(message); const header = `Content-Length: ${Buffer.byteLength(json, 'utf8')}\r\n\r\n`; - this.writer(header + json); + try { + this.writer(header + json); + } catch (error) { + debugLogger.warn('LSP write failed:', error); + this.end(); + } } private disposePending(error?: Error): void { @@ -318,6 +327,11 @@ export class LspConnectionFactory { () => processInstance.stdin?.end(), ); + processInstance.stdin.on('error', (err) => { + debugLogger.warn(`LSP stdin stream error for ${command}:`, err); + connection.end(); + }); + connection.listen(processInstance.stdout); const recordProcessExit = ( code: number | null,