From 0e32d476544be72a88f429027571927769ddc79a Mon Sep 17 00:00:00 2001 From: Ayaan Kapoor Date: Mon, 28 Sep 2026 09:41:29 +0530 Subject: [PATCH 1/2] fix: prevent snap-confine errors by sanitizing environment for xdg-open on Linux Closes #3495 --- src/utils/__tests__/browserLauncher.spec.ts | 72 ++++++++++++++++ src/utils/browserLauncher.ts | 91 +++++++++++++++++++-- 2 files changed, 158 insertions(+), 5 deletions(-) diff --git a/src/utils/__tests__/browserLauncher.spec.ts b/src/utils/__tests__/browserLauncher.spec.ts index 677d3dd5bd..dd65d006aa 100644 --- a/src/utils/__tests__/browserLauncher.spec.ts +++ b/src/utils/__tests__/browserLauncher.spec.ts @@ -1,5 +1,6 @@ import { getAvailableBrowsers, launchBrowser } from 'detect-browsers'; import { shell } from 'electron'; +import { spawn } from 'child_process'; import { dispatch } from '../../store'; import { readSetting } from '../../store/readSetting'; @@ -17,6 +18,13 @@ jest.mock('electron', () => ({ }, })); +jest.mock('child_process', () => ({ + spawn: jest.fn(() => ({ + on: jest.fn(), + unref: jest.fn(), + })), +})); + jest.mock('../../store', () => ({ dispatch: jest.fn(), })); @@ -30,6 +38,18 @@ const mockLaunchBrowser = launchBrowser as jest.Mock; const mockOpenExternal = shell.openExternal as jest.Mock; const mockDispatch = dispatch as jest.Mock; const mockReadSetting = readSetting as jest.Mock; +const mockSpawn = spawn as jest.Mock; + +let originalPlatform: string; +beforeAll(() => { + originalPlatform = process.platform; +}); +afterAll(() => { + Object.defineProperty(process, 'platform', { + value: originalPlatform, + }); +}); + // browserLauncher caches state in module scope, so re-require it per test // to exercise the lazy-load / promise-caching paths from a clean slate. @@ -47,6 +67,7 @@ const firefoxBrowser = { browser: 'firefox', name: 'Firefox' } as any; beforeEach(() => { jest.clearAllMocks(); jest.useFakeTimers(); + Object.defineProperty(process, 'platform', { value: 'darwin' }); mockOpenExternal.mockResolvedValue(undefined); mockLaunchBrowser.mockResolvedValue(undefined); jest.spyOn(console, 'error').mockImplementation(() => undefined); @@ -198,6 +219,57 @@ describe('openExternal', () => { expect.any(Error) ); }); + + describe('on Linux', () => { + beforeEach(() => { + Object.defineProperty(process, 'platform', { value: 'linux' }); + }); + + it('spawns xdg-open directly instead of using shell.openExternal', async () => { + mockReadSetting.mockReturnValue(undefined); + const { openExternal } = loadModule(); + + await openExternal('https://example.com'); + + expect(mockOpenExternal).not.toHaveBeenCalled(); + expect(mockSpawn).toHaveBeenCalledWith('xdg-open', ['https://example.com'], { + detached: true, + env: expect.any(Object), + stdio: 'ignore', + }); + + // Verify the environment is cleaned up + const envArgs = mockSpawn.mock.calls[0][2].env; + expect(envArgs.APPDIR).toBeUndefined(); + expect(envArgs.APPIMAGE).toBeUndefined(); + expect(envArgs.LD_LIBRARY_PATH).toBeUndefined(); + expect(envArgs.APPIMAGE_SILENT_INSTALL).toBeUndefined(); + expect(envArgs.APPIMAGE_START_CWD).toBeUndefined(); + expect(envArgs.GDK_BACKEND).toBeUndefined(); + }); + + it('falls back to shell.openExternal if xdg-open fails', async () => { + mockReadSetting.mockReturnValue(undefined); + + const mockChildProcess = { + on: jest.fn(), + unref: jest.fn(), + }; + mockSpawn.mockReturnValueOnce(mockChildProcess); + + const { openExternal } = loadModule(); + + const promise = openExternal('https://example.com'); + + // Simulate spawn error + const errorCallback = mockChildProcess.on.mock.calls.find((call) => call[0] === 'error')[1]; + errorCallback(new Error('spawn error')); + + await promise; + + expect(mockOpenExternal).toHaveBeenCalledWith('https://example.com'); + }); + }); }); describe('preloadBrowsersList', () => { diff --git a/src/utils/browserLauncher.ts b/src/utils/browserLauncher.ts index d43dd8b17d..e8389acee6 100644 --- a/src/utils/browserLauncher.ts +++ b/src/utils/browserLauncher.ts @@ -1,3 +1,5 @@ +import { spawn } from 'child_process'; + import { getAvailableBrowsers, launchBrowser } from 'detect-browsers'; import type { Browser } from 'detect-browsers'; import { shell } from 'electron'; @@ -49,6 +51,78 @@ const loadBrowsersLazy = (): Promise => { return browserLoadPromise; }; +const getCleanEnvForLinux = (): Record => { + const cleanEnv = { ...process.env }; + const varsToRemove = [ + 'APPDIR', + 'APPIMAGE', + 'LD_LIBRARY_PATH', + 'APPIMAGE_SILENT_INSTALL', + 'APPIMAGE_START_CWD', + 'GDK_BACKEND', + ]; + + for (const key of varsToRemove) { + delete cleanEnv[key]; + } + + return cleanEnv; +}; + +const linuxOpenExternal = (url: string): Promise => { + return new Promise((resolve, reject) => { + const child = spawn('xdg-open', [url], { + detached: true, + env: getCleanEnvForLinux(), + stdio: 'ignore', + }); + + child.on('error', (error) => { + console.error('Failed to open xdg-open', error); + // Fallback to electron's shell.openExternal + shell.openExternal(url).then(resolve).catch(reject); + }); + + child.unref(); + resolve(); + }); +}; + +const withCleanEnv = async (action: () => Promise): Promise => { + if (process.platform !== 'linux') { + return action(); + } + + const originalEnv: Record = {}; + const varsToRemove = [ + 'APPDIR', + 'APPIMAGE', + 'LD_LIBRARY_PATH', + 'APPIMAGE_SILENT_INSTALL', + 'APPIMAGE_START_CWD', + 'GDK_BACKEND', + ]; + + for (const key of varsToRemove) { + if (process.env[key] !== undefined) { + originalEnv[key] = process.env[key]; + delete process.env[key]; + } + } + + try { + return await action(); + } finally { + for (const key of varsToRemove) { + if (originalEnv[key] !== undefined) { + process.env[key] = originalEnv[key]; + } else { + delete process.env[key]; + } + } + } +}; + /** * Launches a URL in the selected browser from settings or falls back to system default * @@ -56,12 +130,19 @@ const loadBrowsersLazy = (): Promise => { * @returns Promise that resolves when the browser is launched */ export const openExternal = async (url: string): Promise => { + const systemOpenExternal = (url: string): Promise => { + if (process.platform === 'linux') { + return linuxOpenExternal(url); + } + return shell.openExternal(url); + }; + // Get the selected browser from settings const selectedBrowser = readSetting('selectedBrowser'); // If no specific browser is selected, use the system default if (!selectedBrowser) { - return shell.openExternal(url); + return systemOpenExternal(url); } try { @@ -74,18 +155,18 @@ export const openExternal = async (url: string): Promise => { ); if (browser) { - // Launch the selected browser with the URL - return launchBrowser(browser, url); + // Launch the selected browser with the URL using a clean environment + return withCleanEnv(() => launchBrowser(browser, url)); } // If the selected browser isn't available, fall back to system default console.warn( `Selected browser "${selectedBrowser}" not found, using system default.` ); - return shell.openExternal(url); + return systemOpenExternal(url); } catch (error) { console.error('Error launching browser:', error); // Fall back to shell.openExternal on error - return shell.openExternal(url); + return systemOpenExternal(url); } }; From 874bc13e5737c82752561348818ed1620bb38f9e Mon Sep 17 00:00:00 2001 From: Ayaan Kapoor Date: Thu, 8 Oct 2026 12:59:34 +0530 Subject: [PATCH 2/2] fix(linux): pass cleaned env copy and await xdg-open exit --- src/utils/__tests__/browserLauncher.spec.ts | 116 ++++++++++++++++---- src/utils/browserLauncher.ts | 77 +++++++------ 2 files changed, 130 insertions(+), 63 deletions(-) diff --git a/src/utils/__tests__/browserLauncher.spec.ts b/src/utils/__tests__/browserLauncher.spec.ts index dd65d006aa..0f8abbc3ae 100644 --- a/src/utils/__tests__/browserLauncher.spec.ts +++ b/src/utils/__tests__/browserLauncher.spec.ts @@ -1,6 +1,7 @@ +import { spawn } from 'child_process'; + import { getAvailableBrowsers, launchBrowser } from 'detect-browsers'; import { shell } from 'electron'; -import { spawn } from 'child_process'; import { dispatch } from '../../store'; import { readSetting } from '../../store/readSetting'; @@ -50,7 +51,6 @@ afterAll(() => { }); }); - // browserLauncher caches state in module scope, so re-require it per test // to exercise the lazy-load / promise-caching paths from a clean slate. const loadModule = () => { @@ -227,44 +227,114 @@ describe('openExternal', () => { it('spawns xdg-open directly instead of using shell.openExternal', async () => { mockReadSetting.mockReturnValue(undefined); - const { openExternal } = loadModule(); - - await openExternal('https://example.com'); - expect(mockOpenExternal).not.toHaveBeenCalled(); - expect(mockSpawn).toHaveBeenCalledWith('xdg-open', ['https://example.com'], { - detached: true, - env: expect.any(Object), - stdio: 'ignore', + const envKeys = [ + 'APPDIR', + 'APPIMAGE', + 'LD_LIBRARY_PATH', + 'APPIMAGE_SILENT_INSTALL', + 'APPIMAGE_START_CWD', + 'GDK_BACKEND', + ] as const; + const originalEnv = Object.fromEntries( + envKeys.map((key) => [key, process.env[key]]) + ); + envKeys.forEach((key) => { + process.env[key] = `sentinel-${key}`; }); - // Verify the environment is cleaned up - const envArgs = mockSpawn.mock.calls[0][2].env; - expect(envArgs.APPDIR).toBeUndefined(); - expect(envArgs.APPIMAGE).toBeUndefined(); - expect(envArgs.LD_LIBRARY_PATH).toBeUndefined(); - expect(envArgs.APPIMAGE_SILENT_INSTALL).toBeUndefined(); - expect(envArgs.APPIMAGE_START_CWD).toBeUndefined(); - expect(envArgs.GDK_BACKEND).toBeUndefined(); + try { + const mockChildProcess = { + on: jest.fn(), + unref: jest.fn(), + }; + mockSpawn.mockReturnValueOnce(mockChildProcess); + + const { openExternal } = loadModule(); + + const promise = openExternal('https://example.com'); + + const closeCallback = mockChildProcess.on.mock.calls.find( + (call) => call[0] === 'close' + )[1]; + closeCallback(0); + + await promise; + + expect(mockOpenExternal).not.toHaveBeenCalled(); + expect(mockSpawn).toHaveBeenCalledWith( + 'xdg-open', + ['https://example.com'], + { + detached: true, + env: expect.any(Object), + stdio: 'ignore', + } + ); + + // Verify the environment is cleaned up + const envArgs = mockSpawn.mock.calls[0][2].env; + expect(envArgs.APPDIR).toBeUndefined(); + expect(envArgs.APPIMAGE).toBeUndefined(); + expect(envArgs.LD_LIBRARY_PATH).toBeUndefined(); + expect(envArgs.APPIMAGE_SILENT_INSTALL).toBeUndefined(); + expect(envArgs.APPIMAGE_START_CWD).toBeUndefined(); + expect(envArgs.GDK_BACKEND).toBeUndefined(); + } finally { + envKeys.forEach((key) => { + const originalValue = originalEnv[key]; + if (originalValue === undefined) { + delete process.env[key]; + } else { + process.env[key] = originalValue; + } + }); + } }); it('falls back to shell.openExternal if xdg-open fails', async () => { mockReadSetting.mockReturnValue(undefined); - + const mockChildProcess = { on: jest.fn(), unref: jest.fn(), }; mockSpawn.mockReturnValueOnce(mockChildProcess); - + const { openExternal } = loadModule(); const promise = openExternal('https://example.com'); - + // Simulate spawn error - const errorCallback = mockChildProcess.on.mock.calls.find((call) => call[0] === 'error')[1]; + const errorCallback = mockChildProcess.on.mock.calls.find( + (call) => call[0] === 'error' + )[1]; errorCallback(new Error('spawn error')); - + + await promise; + + expect(mockOpenExternal).toHaveBeenCalledWith('https://example.com'); + }); + + it('falls back to shell.openExternal if xdg-open exits with non-zero code', async () => { + mockReadSetting.mockReturnValue(undefined); + + const mockChildProcess = { + on: jest.fn(), + unref: jest.fn(), + }; + mockSpawn.mockReturnValueOnce(mockChildProcess); + + const { openExternal } = loadModule(); + + const promise = openExternal('https://example.com'); + + // Simulate non-zero exit code + const closeCallback = mockChildProcess.on.mock.calls.find( + (call) => call[0] === 'close' + )[1]; + closeCallback(1); + await promise; expect(mockOpenExternal).toHaveBeenCalledWith('https://example.com'); diff --git a/src/utils/browserLauncher.ts b/src/utils/browserLauncher.ts index e8389acee6..2441971933 100644 --- a/src/utils/browserLauncher.ts +++ b/src/utils/browserLauncher.ts @@ -77,50 +77,30 @@ const linuxOpenExternal = (url: string): Promise => { stdio: 'ignore', }); + let fallbackTriggered = false; + const triggerFallback = () => { + if (fallbackTriggered) return; + fallbackTriggered = true; + shell.openExternal(url).then(resolve).catch(reject); + }; + child.on('error', (error) => { console.error('Failed to open xdg-open', error); - // Fallback to electron's shell.openExternal - shell.openExternal(url).then(resolve).catch(reject); + triggerFallback(); }); - child.unref(); - resolve(); - }); -}; - -const withCleanEnv = async (action: () => Promise): Promise => { - if (process.platform !== 'linux') { - return action(); - } - - const originalEnv: Record = {}; - const varsToRemove = [ - 'APPDIR', - 'APPIMAGE', - 'LD_LIBRARY_PATH', - 'APPIMAGE_SILENT_INSTALL', - 'APPIMAGE_START_CWD', - 'GDK_BACKEND', - ]; - - for (const key of varsToRemove) { - if (process.env[key] !== undefined) { - originalEnv[key] = process.env[key]; - delete process.env[key]; - } - } - - try { - return await action(); - } finally { - for (const key of varsToRemove) { - if (originalEnv[key] !== undefined) { - process.env[key] = originalEnv[key]; + child.on('close', (code) => { + if (fallbackTriggered) return; + if (code !== 0) { + console.error(`xdg-open exited with code ${code}`); + triggerFallback(); } else { - delete process.env[key]; + resolve(); } - } - } + }); + + child.unref(); + }); }; /** @@ -155,8 +135,25 @@ export const openExternal = async (url: string): Promise => { ); if (browser) { - // Launch the selected browser with the URL using a clean environment - return withCleanEnv(() => launchBrowser(browser, url)); + if (process.platform === 'linux') { + return new Promise((resolve, reject) => { + const child = spawn(browser.executable, [url], { + detached: true, + env: getCleanEnvForLinux(), + stdio: 'ignore', + }); + child.on('error', reject); + child.on('close', (code) => { + if (code === 0) { + resolve(); + } else { + reject(new Error(`Browser exited with code ${code}`)); + } + }); + child.unref(); + }); + } + return await launchBrowser(browser, url); } // If the selected browser isn't available, fall back to system default console.warn(