From 4d0830deacf2d1a95b5224eb669e93183b791432 Mon Sep 17 00:00:00 2001 From: Scott Lovegrove Date: Fri, 5 Jun 2026 15:02:33 +0100 Subject: [PATCH 1/2] fix(auth): caret-escape cmd metacharacters when opening the browser on WSL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `openViaCmdExe` wrapped the URL in double quotes to stop `&` from splitting the `cmd.exe /c start` command line. Under WSL that doesn't work: the interop layer re-quotes argv entries itself, mangling the literal quotes, so `&` leaks through and acts as a statement separator — only the prefix up to the first `&` reaches `start`, and Windows tries to open that fragment as a path ("Windows cannot find '\https://…'"). OAuth authorize URLs (several `&`, percent-encoded params) hit this every time. Two compounding bugs: - The surrounding quotes don't survive WSL interop, so `&` was never protected. - `url.replaceAll('%','%%')` does not collapse on the `cmd /c` command line (only inside batch files), so it would have corrupted every `%HH` byte even when the command did run. Fix: caret-escape cmd's metacharacters (`& | < > ^ ( ) "`) — which survives interop since there are no quotes to mangle — and leave `%` alone (OAuth URLs are `%HH`, which never matches `%NAME%` env-expansion). Extracted as a testable `escapeUrlForCmd`; verified the round-trip through a real `cmd.exe` on WSL. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/auth/flow.test.ts | 38 +++++++++++++++++++++++++++++--------- src/auth/flow.ts | 35 +++++++++++++++++++++++------------ 2 files changed, 52 insertions(+), 21 deletions(-) diff --git a/src/auth/flow.test.ts b/src/auth/flow.test.ts index 2354148..dbade30 100644 --- a/src/auth/flow.test.ts +++ b/src/auth/flow.test.ts @@ -27,7 +27,7 @@ import { buildTokenStore, ellieSattler, } from '../testing/accounts.js' -import { type RunOAuthFlowOptions, runOAuthFlow } from './flow.js' +import { escapeUrlForCmd, type RunOAuthFlowOptions, runOAuthFlow } from './flow.js' import type { AuthProvider, TokenBundle, TokenStore } from './types.js' /** Tiny in-memory `TokenStore` so the flow tests don't need disk I/O. */ @@ -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 cmd.exe, caret-escaping `&` and leaving `%HH` intact', async () => { + // cmd.exe is a shell: `&` must be caret-escaped or it splits the command + // (quoting doesn't survive WSL interop). `%HH` is left alone — doubling + // it would not collapse on the `cmd /c` command line and would corrupt + // the URL. Use an authorize URL with both `&` and `%` to exercise this. stubPlatform('linux') vi.mocked(readFileSync).mockReturnValue('Linux 5.15 #1 SMP microsoft-WSL2') const execFileMock = vi.mocked(execFile) @@ -523,12 +521,34 @@ describe('runOAuthFlow default opener selection', () => { 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('%', '%%')}"`]) + expect(args).toEqual(['/c', 'start', '""', escapeUrlForCmd(url)]) + // The escaped arg caret-protects `&`, leaves `%HH` untouched, and adds no + // surrounding quotes — so stripping the carets recovers the exact URL. + const escaped = (args as string[])[3] + expect(escaped).toContain('^&') + expect(escaped).toContain('%3A') + expect(escaped).not.toContain('%%') + expect(escaped.startsWith('"')).toBe(false) + expect(escaped.replaceAll('^', '')).toBe(url) // Sanity: the URL we built does contain both special chars. expect(url).toMatch(/&/) expect(url).toMatch(/%/) }) + it('escapeUrlForCmd caret-escapes cmd metacharacters and preserves percent-encoding', () => { + // `&` `|` `<` `>` `^` `(` `)` `"` each get a single caret prefix; `%HH` + // and the rest of the URL pass through verbatim. Stripping carets must + // round-trip to the original. + const url = + 'https://todoist.com/oauth/authorize?resource=https%3A%2F%2Fcomms.todoist.com' + + '&response_type=code&redirect_uri=http%3A%2F%2Flocalhost%3A8766%2Fcallback&scope=user%3Aread' + const escaped = escapeUrlForCmd(url) + expect(escaped).not.toContain('%%') + expect(escaped).not.toMatch(/(^|[^^])&/) // every `&` is caret-prefixed + expect(escaped.replaceAll('^', '')).toBe(url) + expect(escapeUrlForCmd('a&b|ce(f)g^h"i')).toBe('a^&b^|c^e^(f^)g^^h^"i') + }) + it('skips the default opener entirely on headless Linux', async () => { // No DISPLAY / WAYLAND_DISPLAY / BROWSER + non-WSL Linux → there's // no working browser launch path. Don't pay the spawn cost; the URL diff --git a/src/auth/flow.ts b/src/auth/flow.ts index 7a09237..1232a90 100644 --- a/src/auth/flow.ts +++ b/src/auth/flow.ts @@ -500,20 +500,31 @@ 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. +// `cmd.exe /c` is a shell, so cmd metacharacters in the URL have to be escaped +// or they break the command. The naive defence — wrapping the URL in double +// quotes — does NOT work under WSL: the interop layer re-quotes argv entries +// itself, so the literal quotes are mangled and an unescaped `&` (every OAuth +// URL has several) still acts as a statement separator. Only the prefix up to +// the first `&` then reaches `start`, and Windows tries to open that fragment +// as a path ("Windows cannot find '\https://…'"). +// +// Caret-escaping the metacharacters survives interop (there are no quotes for +// it to mangle) and is the reliable way to pass them through cmd verbatim. +// +// `%` is deliberately left untouched: OAuth URLs are RFC 3986 percent-encoded +// (`%HH`), which never matches cmd's `%NAME%` env-expansion, and there is no +// command-line caret-escape for `%`. Doubling it to `%%` does NOT collapse on +// the `cmd /c` command line (that only happens inside batch files), so it would +// corrupt every `%HH` byte in the URL. +export function escapeUrlForCmd(url: string): string { + return url.replace(/[&|<>^()"]/g, '^$&') +} + // `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 }) + await execFileAsync('cmd.exe', ['/c', 'start', '""', escapeUrlForCmd(url)], { + windowsHide: true, + }) } From 1153244e2ed1f53cf6feac981aa9fdd2a96879cb Mon Sep 17 00:00:00 2001 From: Scott Lovegrove Date: Fri, 5 Jun 2026 15:11:47 +0100 Subject: [PATCH 2/2] fix(auth): open the WSL browser via rundll32 instead of cmd.exe MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Addresses review: caret-escaping cmd metacharacters still left `%` exposed — a percent-encoded multi-byte UTF-8 byte like `%C3%A9` (é) contains `%C3%`, which `cmd /c` would treat as `%VAR%` env-expansion and mangle. Since runOAuthFlow is public API for custom AuthProviders, that's a real hazard, not just a built-in concern. Bypass the shell entirely: launch via `rundll32.exe url.dll,FileProtocolHandler `. CreateProcess hands the single argv to the protocol handler verbatim — no cmd parsing pass — so neither `&` (statement separator) nor `%HH` (env-expansion) can corrupt the URL, and there's no fragile escaping to maintain. Drops the now-unneeded escapeUrlForCmd helper. Co-Authored-By: Claude Opus 4.8 (1M context) --- src/auth/flow.test.ts | 46 +++++++++++-------------------------------- src/auth/flow.ts | 44 ++++++++++++++++------------------------- 2 files changed, 29 insertions(+), 61 deletions(-) diff --git a/src/auth/flow.test.ts b/src/auth/flow.test.ts index dbade30..a5f0b63 100644 --- a/src/auth/flow.test.ts +++ b/src/auth/flow.test.ts @@ -27,7 +27,7 @@ import { buildTokenStore, ellieSattler, } from '../testing/accounts.js' -import { escapeUrlForCmd, type RunOAuthFlowOptions, runOAuthFlow } from './flow.js' +import { type RunOAuthFlowOptions, runOAuthFlow } from './flow.js' import type { AuthProvider, TokenBundle, TokenStore } from './types.js' /** Tiny in-memory `TokenStore` so the flow tests don't need disk I/O. */ @@ -488,11 +488,11 @@ describe('runOAuthFlow default opener selection', () => { Object.defineProperty(process, 'platform', { value, configurable: true }) } - it('routes WSL via cmd.exe, caret-escaping `&` and leaving `%HH` intact', async () => { - // cmd.exe is a shell: `&` must be caret-escaped or it splits the command - // (quoting doesn't survive WSL interop). `%HH` is left alone — doubling - // it would not collapse on the `cmd /c` command line and would corrupt - // the URL. Use an authorize URL with both `&` and `%` to exercise this. + 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) @@ -508,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' }, }), }) @@ -520,33 +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', '""', escapeUrlForCmd(url)]) - // The escaped arg caret-protects `&`, leaves `%HH` untouched, and adds no - // surrounding quotes — so stripping the carets recovers the exact URL. - const escaped = (args as string[])[3] - expect(escaped).toContain('^&') - expect(escaped).toContain('%3A') - expect(escaped).not.toContain('%%') - expect(escaped.startsWith('"')).toBe(false) - expect(escaped.replaceAll('^', '')).toBe(url) - // Sanity: the URL we built does contain both special chars. - expect(url).toMatch(/&/) - expect(url).toMatch(/%/) - }) - - it('escapeUrlForCmd caret-escapes cmd metacharacters and preserves percent-encoding', () => { - // `&` `|` `<` `>` `^` `(` `)` `"` each get a single caret prefix; `%HH` - // and the rest of the URL pass through verbatim. Stripping carets must - // round-trip to the original. - const url = - 'https://todoist.com/oauth/authorize?resource=https%3A%2F%2Fcomms.todoist.com' + - '&response_type=code&redirect_uri=http%3A%2F%2Flocalhost%3A8766%2Fcallback&scope=user%3Aread' - const escaped = escapeUrlForCmd(url) - expect(escaped).not.toContain('%%') - expect(escaped).not.toMatch(/(^|[^^])&/) // every `&` is caret-prefixed - expect(escaped.replaceAll('^', '')).toBe(url) - expect(escapeUrlForCmd('a&b|ce(f)g^h"i')).toBe('a^&b^|c^e^(f^)g^^h^"i') + 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 1232a90..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,31 +500,21 @@ async function loadDefaultOpener(): Promise<((url: string) => Promise) | n const execFileAsync = promisify(execFile) -// `cmd.exe /c` is a shell, so cmd metacharacters in the URL have to be escaped -// or they break the command. The naive defence — wrapping the URL in double -// quotes — does NOT work under WSL: the interop layer re-quotes argv entries -// itself, so the literal quotes are mangled and an unescaped `&` (every OAuth -// URL has several) still acts as a statement separator. Only the prefix up to -// the first `&` then reaches `start`, and Windows tries to open that fragment -// as a path ("Windows cannot find '\https://…'"). +// 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. // -// Caret-escaping the metacharacters survives interop (there are no quotes for -// it to mangle) and is the reliable way to pass them through cmd verbatim. -// -// `%` is deliberately left untouched: OAuth URLs are RFC 3986 percent-encoded -// (`%HH`), which never matches cmd's `%NAME%` env-expansion, and there is no -// command-line caret-escape for `%`. Doubling it to `%%` does NOT collapse on -// the `cmd /c` command line (that only happens inside batch files), so it would -// corrupt every `%HH` byte in the URL. -export function escapeUrlForCmd(url: string): string { - return url.replace(/[&|<>^()"]/g, '^$&') -} - -// `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 { - await execFileAsync('cmd.exe', ['/c', 'start', '""', escapeUrlForCmd(url)], { +// 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, }) }