diff --git a/src/auth/flow.test.ts b/src/auth/flow.test.ts index 2354148..a5f0b63 100644 --- a/src/auth/flow.test.ts +++ b/src/auth/flow.test.ts @@ -488,13 +488,11 @@ describe('runOAuthFlow default opener selection', () => { Object.defineProperty(process, 'platform', { value, configurable: true }) } - it('routes WSL via cmd.exe with the URL quoted and `%` doubled', async () => { - // Two escapes are load-bearing for cmd.exe: - // 1. quote the URL so `&` doesn't split the command line - // 2. double `%` so cmd.exe doesn't try to expand percent-encoded - // OAuth params (`%3A`, `%2F`, …) as env-var references. - // Use an authorize URL with both `&` and `%` so the assertion - // exercises both escapes. + it('routes WSL via rundll32, passing the URL verbatim (no shell, no escaping)', async () => { + // rundll32 is not a shell, so the URL is handed to the protocol handler + // as-is: `&` can't split the command and `%HH` (incl. multi-byte UTF-8 + // like `%C3%A9`) can't be mistaken for `%VAR%` env-expansion. The URL + // here carries `&`, ASCII `%HH`, and an encoded `é` to lock that in. stubPlatform('linux') vi.mocked(readFileSync).mockReturnValue('Linux 5.15 #1 SMP microsoft-WSL2') const execFileMock = vi.mocked(execFile) @@ -510,7 +508,7 @@ describe('runOAuthFlow default opener selection', () => { const { provider, getRedirect } = instrument({ authorize: async (input) => ({ - authorizeUrl: `https://example.com/oauth/authorize?state=${input.state}&redirect_uri=http%3A%2F%2Flocalhost%3A8080`, + authorizeUrl: `https://example.com/oauth/authorize?state=${input.state}&redirect_uri=http%3A%2F%2Flocalhost%3A8080&name=Andr%C3%A9`, handshake: { codeVerifier: 'v1' }, }), }) @@ -522,11 +520,11 @@ describe('runOAuthFlow default opener selection', () => { expect(execFileMock).toHaveBeenCalledTimes(1) const [cmd, args] = execFileMock.mock.calls[0] const url = onAuthorizeUrl.mock.calls[0][0] as string - expect(cmd).toBe('cmd.exe') - expect(args).toEqual(['/c', 'start', '""', `"${url.replaceAll('%', '%%')}"`]) - // Sanity: the URL we built does contain both special chars. - expect(url).toMatch(/&/) - expect(url).toMatch(/%/) + expect(cmd).toBe('rundll32.exe') + // URL is passed verbatim — no quoting, no caret-escaping, no `%%`. + expect(args).toEqual(['url.dll,FileProtocolHandler', url]) + expect(url).toContain('&') + expect(url).toContain('%C3%A9') }) it('skips the default opener entirely on headless Linux', async () => { diff --git a/src/auth/flow.ts b/src/auth/flow.ts index 7a09237..0d6e616 100644 --- a/src/auth/flow.ts +++ b/src/auth/flow.ts @@ -484,9 +484,9 @@ async function openOrFallback( async function loadDefaultOpener(): Promise<((url: string) => Promise) | null> { // WSL check must run before the headless check: WSL is `platform === 'linux'` - // and often has no DISPLAY, but `cmd.exe` does work and reaches the user's - // real Windows browser, so it's worth the spawn. - if (isWsl()) return openViaCmdExe + // and often has no DISPLAY, but the Windows interop opener does work and + // reaches the user's real Windows browser, so it's worth the spawn. + if (isWsl()) return openViaWindows if (isHeadlessLinux()) return null try { const mod = (await import('open')) as { default: (url: string) => Promise } @@ -500,20 +500,21 @@ async function loadDefaultOpener(): Promise<((url: string) => Promise) | n const execFileAsync = promisify(execFile) -// Two layers of escaping are needed because `cmd.exe /c` is a shell: -// 1. Wrap the URL in literal double quotes. WSL interop only auto-quotes -// args that contain spaces; an OAuth URL (no spaces, plenty of `&`s) -// would otherwise be re-parsed by cmd.exe with `&` acting as a -// statement separator, so only the prefix up to the first `&` would -// reach `start`. -// 2. Double every `%` to `%%`. cmd.exe expands `%NAME%` even inside -// quoted strings; OAuth URLs are full of percent-encoded bytes -// (`%3A`, `%2F`, …) and a chance match against a defined env var -// (`%PATH%`, `%TEMP%`, …) would silently mangle the URL. -// `start ""` — the empty title arg is mandatory; otherwise `start` consumes -// the URL as a window title and never launches a browser. (`execFile`'s -// no-shell guarantee doesn't apply when the target is itself a shell.) -async function openViaCmdExe(url: string): Promise { - const escaped = url.replaceAll('%', '%%') - await execFileAsync('cmd.exe', ['/c', 'start', '""', `"${escaped}"`], { windowsHide: true }) +// Launch the Windows default browser through rundll32's URL protocol handler. +// Crucially this is NOT a shell, so the URL is passed as a single argv that +// `CreateProcess` hands to the handler verbatim — there is no `cmd.exe` parsing +// pass, so `&` can't split the command and percent-encoded bytes (including +// multi-byte UTF-8 like `%C3%A9` for `é`) can't be mistaken for `%VAR%` +// env-expansion. +// +// The previous `cmd.exe /c start "" ""` approach was doubly broken on WSL: +// its literal double-quotes don't survive WSL interop's own argv re-quoting (so +// `&` leaked through and Windows opened the truncated fragment as a path — +// "Windows cannot find '\https://…'"), and `%`→`%%` doubling doesn't collapse on +// the `cmd /c` command line, so it corrupted every `%HH` byte. Avoiding the +// shell sidesteps both, with no fragile escaping to maintain. +async function openViaWindows(url: string): Promise { + await execFileAsync('rundll32.exe', ['url.dll,FileProtocolHandler', url], { + windowsHide: true, + }) }