diff --git a/__tests__/open-browser-security.test.ts b/__tests__/open-browser-security.test.ts new file mode 100644 index 000000000..5885d2b52 --- /dev/null +++ b/__tests__/open-browser-security.test.ts @@ -0,0 +1,66 @@ +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; + +// openBrowser() launches a URL that originates from the device-login server +// response (verification_uri_complete). On Windows the URL must never be handed +// to cmd.exe, which re-parses shell metacharacters (& | ^ ") and would allow a +// malicious/compromised login server to inject commands. See issue #1114. +const { spawnMock } = vi.hoisted(() => ({ spawnMock: vi.fn() })); +vi.mock('child_process', () => ({ spawn: spawnMock })); + +import { openBrowser } from '../src/reasoning/login'; + +describe('openBrowser — Windows shell-injection hardening (#1114)', () => { + const realPlatform = process.platform; + + beforeEach(() => { + spawnMock.mockReset(); + spawnMock.mockReturnValue({ on: vi.fn(), unref: vi.fn() }); + Object.defineProperty(process, 'platform', { value: 'win32', configurable: true }); + }); + + afterEach(() => { + Object.defineProperty(process, 'platform', { value: realPlatform, configurable: true }); + }); + + it('does not launch the URL through cmd.exe on Windows', async () => { + const url = 'https://app.getcodegraph.com/device?user_code=ABCD-1234&calc'; + + await openBrowser(url); + + expect(spawnMock).toHaveBeenCalledTimes(1); + const [command, args] = spawnMock.mock.calls[0] as [string, string[]]; + + // cmd.exe (and its `start` builtin) re-parse metacharacters, so the launcher + // must not be cmd and must not carry cmd's `/c` switch. + expect(command.toLowerCase()).not.toContain('cmd'); + expect(args).not.toContain('/c'); + + // The server-supplied URL is passed to the launcher as one intact argument, + // never concatenated into a shell command string. + expect(args).toContain(url); + + // And never through a shell at all. + const options = spawnMock.mock.calls[0][2] as { shell?: boolean } | undefined; + expect(options?.shell).toBeFalsy(); + }); + + it.each([ + ['file:///C:/Windows/System32/calc.exe'], + ['\\\\evil.example\\share\\payload.exe'], + ['C:\\Windows\\System32\\calc.exe'], + ['javascript:alert(1)'], + ])('refuses to launch a non-http(s) target: %s', async (target) => { + // rundll32 url.dll,FileProtocolHandler (like `open` / `xdg-open`) hands the + // string to the OS handler, which happily executes file/UNC paths — so the + // server-supplied string must be scheme-checked before launching anything. + await openBrowser(target); + + expect(spawnMock).not.toHaveBeenCalled(); + }); + + it('still launches plain http:// URLs', async () => { + await openBrowser('http://localhost:8080/device?user_code=ABCD-1234'); + + expect(spawnMock).toHaveBeenCalledTimes(1); + }); +}); diff --git a/src/reasoning/login.ts b/src/reasoning/login.ts index 70faf1857..5d1dbaade 100644 --- a/src/reasoning/login.ts +++ b/src/reasoning/login.ts @@ -75,9 +75,14 @@ export async function pollForToken(deviceCode: string, intervalSec: number, expi /** Best-effort: open a URL in the default browser. Never throws — the URL is also printed. */ export async function openBrowser(url: string): Promise { + // The OS handlers below (open / rundll32 / xdg-open) execute file and UNC + // paths as readily as they open URLs — never hand them a non-http(s) string. + if (!/^https?:\/\//i.test(url)) return; const [cmd, args] = process.platform === 'darwin' ? ['open', [url]] - : process.platform === 'win32' ? ['cmd', ['/c', 'start', '', url]] + // Avoid `cmd /c start`: cmd.exe re-parses shell metacharacters (& | ^ ") in the + // server-supplied URL. rundll32 receives the URL as a single, opaque argument. + : process.platform === 'win32' ? ['rundll32', ['url.dll,FileProtocolHandler', url]] : ['xdg-open', [url]]; try { const child = spawn(cmd as string, args as string[], { stdio: 'ignore', detached: true, windowsHide: true });