Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 11 additions & 13 deletions src/auth/flow.test.ts
Original file line number Diff line number Diff line change
Expand Up @@ -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)
Expand All @@ -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' },
}),
})
Expand All @@ -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 () => {
Expand Down
39 changes: 20 additions & 19 deletions src/auth/flow.ts
Original file line number Diff line number Diff line change
Expand Up @@ -484,9 +484,9 @@ async function openOrFallback(

async function loadDefaultOpener(): Promise<((url: string) => Promise<void>) | 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<unknown> }
Expand All @@ -500,20 +500,21 @@ async function loadDefaultOpener(): Promise<((url: string) => Promise<void>) | 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<void> {
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 "" "<url>"` 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<void> {
await execFileAsync('rundll32.exe', ['url.dll,FileProtocolHandler', url], {
windowsHide: true,
})
}
Loading