From 2bc68728fc8651dbaf095423c8201895cc126290 Mon Sep 17 00:00:00 2001 From: Scott Lovegrove Date: Sat, 16 May 2026 16:47:40 +0100 Subject: [PATCH 1/5] fix(auth): open browser on WSL via cmd.exe; always surface URL MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `runOAuthFlow` relied on the `open` npm package to launch the user's browser. On WSL `open` routes through `xdg-open` / `wslview`, both of which can silently no-op — the spawn resolves cleanly but no browser ever appears, leaving the user stuck until the 3-minute callback timeout. The same silent-failure mode hits SSH sessions, containers, CI runners, and headless servers. - Detect WSL via `/proc/version` and spawn `cmd.exe /c start "" ` directly so the real Windows browser opens. - Detect headless Linux (no DISPLAY / WAYLAND_DISPLAY / $BROWSER) and skip the doomed spawn entirely; the URL print is the only path. - Print the authorize URL unconditionally — even when the opener succeeds — so every user has a copy-pasteable fallback regardless of platform. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/auth/flow.test.ts | 30 +++++++++++++++++++ src/auth/flow.ts | 70 ++++++++++++++++++++++++++++++++++++------- 2 files changed, 89 insertions(+), 11 deletions(-) diff --git a/src/auth/flow.test.ts b/src/auth/flow.test.ts index a971c30..a647287 100644 --- a/src/auth/flow.test.ts +++ b/src/auth/flow.test.ts @@ -273,6 +273,36 @@ describe('runOAuthFlow', () => { expect(setSpy).not.toHaveBeenCalled() }) + it('always surfaces the authorize URL via onAuthorizeUrl, even when openBrowser succeeds', async () => { + // The browser spawn can resolve cleanly yet open no actual browser + // (WSL no-op, headless Linux, etc.), so the URL must reach the user + // on every successful run — not only on opener failure. + const { provider, getRedirect } = instrument() + const store = fakeStore() + const onAuthorizeUrl = vi.fn((_url: string) => undefined) + const openBrowser = vi.fn(async (url: string) => { + const state = new URL(url).searchParams.get('state') ?? '' + await fetch(`${getRedirect()}?code=abc&state=${state}`) + }) + const result = await runOAuthFlow({ + provider, + store, + scopes: [], + readOnly: false, + flags: {}, + preferredPort: 0, + renderSuccess, + renderError, + openBrowser, + onAuthorizeUrl, + timeoutMs: 5000, + }) + expect(onAuthorizeUrl).toHaveBeenCalledTimes(1) + expect(onAuthorizeUrl.mock.calls[0][0]).toMatch(/^https:\/\/example\.com\/oauth\/authorize/) + expect(openBrowser).toHaveBeenCalledTimes(1) + expect(result.token).toBe('tok-1') + }) + it('falls back to onAuthorizeUrl when the openBrowser opener throws', async () => { const { provider, getRedirect } = instrument() const store = fakeStore() diff --git a/src/auth/flow.ts b/src/auth/flow.ts index 0e5d674..375d35b 100644 --- a/src/auth/flow.ts +++ b/src/auth/flow.ts @@ -1,9 +1,37 @@ +import { execFile } from 'node:child_process' +import { readFileSync } from 'node:fs' import { type IncomingMessage, type Server, type ServerResponse, createServer } from 'node:http' import { CliError, getErrorMessage } from '../errors.js' import { isStdoutTTY } from '../terminal.js' import { generateState } from './pkce.js' import type { AuthAccount, AuthProvider, TokenStore } from './types.js' +// WSL's `open` package routes through `xdg-open` / `wslview`, both of which +// silently no-op on headless WSL installs — the spawn resolves cleanly but no +// browser ever appears, so the OAuth callback wait runs to its 3-minute +// timeout. Detect at call time and route WSL through `cmd.exe` directly. +// Non-Linux platforms short-circuit before the fs read. +function isWsl(): boolean { + if (process.platform !== 'linux') return false + try { + return /microsoft/i.test(readFileSync('/proc/version', 'utf8')) + } catch { + return false + } +} + +// SSH sessions, containers, CI runners, headless servers — same failure mode +// as WSL but with no Windows side to bounce through. With no DISPLAY / +// WAYLAND_DISPLAY (and no $BROWSER override for Codespaces-style setups +// that route through a remote bridge), `xdg-open` will either error or +// no-op, so the spawn is pure noise — skip it and let the URL print do the +// work. +function isHeadlessLinux(): boolean { + if (process.platform !== 'linux') return false + if (process.env.BROWSER) return false + return !process.env.DISPLAY && !process.env.WAYLAND_DISPLAY +} + export type RunOAuthFlowOptions = { provider: AuthProvider store: TokenStore @@ -424,22 +452,29 @@ async function openOrFallback( url: string, options: RunOAuthFlowOptions, ): Promise { - const opener = options.openBrowser ?? (await loadDefaultOpener()) - if (opener) { - try { - await opener(url) - return - } catch { - // Fall through to the URL print below. - } - } - // No opener available, or the opener threw. Surface the URL so the user - // can finish the flow manually. + // Surface the URL up-front, before attempting the browser spawn. The + // spawn can succeed yet open no browser (WSL without working interop, + // headless Linux, locked-down corporate envs, missing `open` peer), and + // we have no reliable signal that the user actually landed on the page + // — so printing here guarantees a copy-pasteable path on every platform. if (options.onAuthorizeUrl) options.onAuthorizeUrl(url) else if (isStdoutTTY()) console.log(`Open this URL in your browser:\n ${url}`) + + const opener = options.openBrowser ?? (await loadDefaultOpener()) + if (!opener) return + try { + await opener(url) + } catch { + // URL is already surfaced above. + } } 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 + if (isHeadlessLinux()) return null try { const mod = (await import('open')) as { default: (url: string) => Promise } return async (url) => { @@ -449,3 +484,16 @@ async function loadDefaultOpener(): Promise<((url: string) => Promise) | n return null } } + +// `start ""` — the empty title arg is mandatory; otherwise `start` consumes +// the URL as a window title and never launches a browser. The URL is passed +// as an `execFile` arg (not interpolated into a shell string), so URLs with +// special characters can't escape into command-injection territory. +async function openViaCmdExe(url: string): Promise { + await new Promise((resolve, reject) => { + execFile('cmd.exe', ['/c', 'start', '""', url], { windowsHide: true }, (error) => { + if (error) reject(error) + else resolve() + }) + }) +} From 576d88038c730672ccd7e7bad1e6c356a7f39d81 Mon Sep 17 00:00:00 2001 From: Scott Lovegrove Date: Sat, 16 May 2026 16:53:36 +0100 Subject: [PATCH 2/5] test(auth): extract flowOptions + driveCallback helpers in flow.test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Every test was re-listing the same seven boilerplate option fields (`scopes`, `readOnly`, `flags`, `preferredPort`, `renderSuccess`, `renderError`, `timeoutMs`) and re-implementing the same `state → fetch(redirect)` callback driver. Hoist both into helpers so each test reads as variants over the shared baseline. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/auth/flow.test.ts | 287 +++++++++++++++++------------------------- 1 file changed, 118 insertions(+), 169 deletions(-) diff --git a/src/auth/flow.test.ts b/src/auth/flow.test.ts index a647287..9669db5 100644 --- a/src/auth/flow.test.ts +++ b/src/auth/flow.test.ts @@ -1,6 +1,6 @@ import { describe, expect, it, vi } from 'vitest' -import { runOAuthFlow } from './flow.js' +import { type RunOAuthFlowOptions, runOAuthFlow } from './flow.js' import type { AuthProvider, TokenStore } from './types.js' type Account = { id: string; label?: string; email: string } @@ -68,6 +68,40 @@ function instrument(provider: Partial> = {}): { return { provider: wrapped, getRedirect: () => redirectUri } } +/** + * Fill in the boilerplate fields every test would otherwise repeat + * (`scopes`, `readOnly`, `flags`, `preferredPort`, `renderSuccess`, + * `renderError`, `timeoutMs`). Caller supplies the variants — `provider`, + * `store`, `openBrowser` are required; anything else overrides a default. + */ +function flowOptions( + overrides: Partial> & + Pick, 'provider' | 'store' | 'openBrowser'>, +): RunOAuthFlowOptions { + return { + scopes: [], + readOnly: false, + flags: {}, + preferredPort: 0, + renderSuccess, + renderError, + timeoutMs: 5000, + ...overrides, + } +} + +/** + * Drive the local callback server with a valid `code` + the `state` lifted + * from the authorize URL. Used as both an `openBrowser` and an + * `onAuthorizeUrl` stand-in across the suite. + */ +function driveCallback(getRedirect: () => string): (url: string) => Promise { + return async (url) => { + const state = new URL(url).searchParams.get('state') ?? '' + await fetch(`${getRedirect()}?code=abc&state=${state}`) + } +} + describe('runOAuthFlow', () => { it('drives prepare → authorize → exchange → validate → store and returns the result', async () => { const prepare = vi.fn(async () => ({ handshake: { dcrSecret: 'shh' } })) @@ -75,25 +109,17 @@ describe('runOAuthFlow', () => { const validateToken = vi.fn(async () => ({ id: '1', email: 'a@b' })) const { provider, getRedirect } = instrument({ prepare, exchangeCode, validateToken }) const store = fakeStore() + const openBrowser = vi.fn(driveCallback(getRedirect)) - const openBrowser = vi.fn(async (url: string) => { - const state = new URL(url).searchParams.get('state') ?? '' - await fetch(`${getRedirect()}?code=abc&state=${state}`) - }) - - const result = await runOAuthFlow({ - provider, - store, - scopes: ['read'], - readOnly: false, - flags: {}, - preferredPort: 0, - renderSuccess, - renderError, - openBrowser, - onAuthorizeUrl: () => undefined, - timeoutMs: 5000, - }) + const result = await runOAuthFlow( + flowOptions({ + provider, + store, + scopes: ['read'], + openBrowser, + onAuthorizeUrl: () => undefined, + }), + ) expect(result.token).toBe('tok-1') expect(result.account).toEqual({ id: '1', email: 'a@b' }) @@ -118,21 +144,9 @@ describe('runOAuthFlow', () => { }) const store = fakeStore() - const result = await runOAuthFlow({ - provider, - store, - scopes: [], - readOnly: false, - flags: {}, - preferredPort: 0, - renderSuccess, - renderError, - openBrowser: async (url) => { - const state = new URL(url).searchParams.get('state') ?? '' - await fetch(`${getRedirect()}?code=abc&state=${state}`) - }, - timeoutMs: 5000, - }) + const result = await runOAuthFlow( + flowOptions({ provider, store, openBrowser: driveCallback(getRedirect) }), + ) expect(result.account.id).toBe('99') expect(validateToken).not.toHaveBeenCalled() }) @@ -154,21 +168,9 @@ describe('runOAuthFlow', () => { }) const store = fakeStore() - await runOAuthFlow({ - provider, - store, - scopes: [], - readOnly: false, - flags: {}, - preferredPort: 0, - renderSuccess, - renderError, - openBrowser: async (url) => { - const state = new URL(url).searchParams.get('state') ?? '' - await fetch(`${getRedirect()}?code=abc&state=${state}`) - }, - timeoutMs: 5000, - }) + await runOAuthFlow( + flowOptions({ provider, store, openBrowser: driveCallback(getRedirect) }), + ) expect(validateToken).toHaveBeenCalledTimes(1) }) @@ -176,18 +178,14 @@ describe('runOAuthFlow', () => { const { provider } = instrument() const store = fakeStore() await expect( - runOAuthFlow({ - provider, - store, - scopes: [], - readOnly: false, - flags: {}, - preferredPort: 0, - renderSuccess, - renderError, - openBrowser: async () => {}, // never triggers a callback - timeoutMs: 50, - }), + runOAuthFlow( + flowOptions({ + provider, + store, + openBrowser: async () => {}, // never triggers a callback + timeoutMs: 50, + }), + ), ).rejects.toMatchObject({ code: 'AUTH_CALLBACK_TIMEOUT' }) }) @@ -195,30 +193,25 @@ describe('runOAuthFlow', () => { const { provider, getRedirect } = instrument() const store = fakeStore() - const result = await runOAuthFlow({ - provider, - store, - scopes: [], - readOnly: false, - flags: {}, - preferredPort: 0, - renderSuccess, - renderError, - openBrowser: async (url) => { - const state = new URL(url).searchParams.get('state') ?? '' - // Spurious requests (browser-extension prefetch, accidental - // reload) that don't match the expected state should leave the - // server listening rather than killing the in-flight flow. - const bad1 = await fetch(`${getRedirect()}?code=abc&state=wrong`) - expect(bad1.status).toBe(400) - const bad2 = await fetch(`${getRedirect()}?code=abc`) - expect(bad2.status).toBe(400) - // The legitimate redirect arriving after the noise should still - // settle the wait. - await fetch(`${getRedirect()}?code=abc&state=${state}`) - }, - timeoutMs: 5000, - }) + const result = await runOAuthFlow( + flowOptions({ + provider, + store, + openBrowser: async (url) => { + const state = new URL(url).searchParams.get('state') ?? '' + // Spurious requests (browser-extension prefetch, accidental + // reload) that don't match the expected state should leave the + // server listening rather than killing the in-flight flow. + const bad1 = await fetch(`${getRedirect()}?code=abc&state=wrong`) + expect(bad1.status).toBe(400) + const bad2 = await fetch(`${getRedirect()}?code=abc`) + expect(bad2.status).toBe(400) + // The legitimate redirect arriving after the noise should still + // settle the wait. + await fetch(`${getRedirect()}?code=abc&state=${state}`) + }, + }), + ) expect(result.token).toBe('tok-1') }) @@ -227,18 +220,9 @@ describe('runOAuthFlow', () => { const { provider } = instrument() const store = fakeStore() await expect( - runOAuthFlow({ - provider, - store, - scopes: [], - readOnly: false, - flags: {}, - preferredPort: 70_000, - renderSuccess, - renderError, - openBrowser, - timeoutMs: 5000, - }), + runOAuthFlow( + flowOptions({ provider, store, openBrowser, preferredPort: 70_000 }), + ), ).rejects.toMatchObject({ code: 'AUTH_PORT_BIND_FAILED' }) expect(openBrowser).not.toHaveBeenCalled() }) @@ -250,25 +234,20 @@ describe('runOAuthFlow', () => { const setSpy = vi.spyOn(store, 'set') await expect( - runOAuthFlow({ - provider, - store, - scopes: [], - readOnly: false, - flags: {}, - preferredPort: 0, - renderSuccess, - renderError, - openBrowser: async () => { - // Abort before the callback arrives — flow should reject - // with AUTH_OAUTH_FAILED rather than continue waiting. - controller.abort() - void getRedirect() // touch to silence unused-fn lint - }, - onAuthorizeUrl: () => undefined, - signal: controller.signal, - timeoutMs: 5000, - }), + runOAuthFlow( + flowOptions({ + provider, + store, + openBrowser: async () => { + // Abort before the callback arrives — flow should reject + // with AUTH_OAUTH_FAILED rather than continue waiting. + controller.abort() + void getRedirect() // touch to silence unused-fn lint + }, + onAuthorizeUrl: () => undefined, + signal: controller.signal, + }), + ), ).rejects.toMatchObject({ code: 'AUTH_OAUTH_FAILED' }) expect(setSpy).not.toHaveBeenCalled() }) @@ -280,23 +259,10 @@ describe('runOAuthFlow', () => { const { provider, getRedirect } = instrument() const store = fakeStore() const onAuthorizeUrl = vi.fn((_url: string) => undefined) - const openBrowser = vi.fn(async (url: string) => { - const state = new URL(url).searchParams.get('state') ?? '' - await fetch(`${getRedirect()}?code=abc&state=${state}`) - }) - const result = await runOAuthFlow({ - provider, - store, - scopes: [], - readOnly: false, - flags: {}, - preferredPort: 0, - renderSuccess, - renderError, - openBrowser, - onAuthorizeUrl, - timeoutMs: 5000, - }) + const openBrowser = vi.fn(driveCallback(getRedirect)) + const result = await runOAuthFlow( + flowOptions({ provider, store, openBrowser, onAuthorizeUrl }), + ) expect(onAuthorizeUrl).toHaveBeenCalledTimes(1) expect(onAuthorizeUrl.mock.calls[0][0]).toMatch(/^https:\/\/example\.com\/oauth\/authorize/) expect(openBrowser).toHaveBeenCalledTimes(1) @@ -306,26 +272,17 @@ describe('runOAuthFlow', () => { it('falls back to onAuthorizeUrl when the openBrowser opener throws', async () => { const { provider, getRedirect } = instrument() const store = fakeStore() - const onAuthorizeUrl = vi.fn(async (url: string) => { - // Drive the callback off the URL we received via the fallback path. - const state = new URL(url).searchParams.get('state') ?? '' - await fetch(`${getRedirect()}?code=abc&state=${state}`) - }) - const result = await runOAuthFlow({ - provider, - store, - scopes: [], - readOnly: false, - flags: {}, - preferredPort: 0, - renderSuccess, - renderError, - openBrowser: async () => { - throw new Error('opener boom') - }, - onAuthorizeUrl, - timeoutMs: 5000, - }) + const onAuthorizeUrl = vi.fn(driveCallback(getRedirect)) + const result = await runOAuthFlow( + flowOptions({ + provider, + store, + openBrowser: async () => { + throw new Error('opener boom') + }, + onAuthorizeUrl, + }), + ) expect(onAuthorizeUrl).toHaveBeenCalledTimes(1) expect(onAuthorizeUrl.mock.calls[0][0]).toMatch(/^https:\/\/example\.com\/oauth\/authorize/) expect(result.token).toBe('tok-1') @@ -347,22 +304,14 @@ describe('runOAuthFlow', () => { async setDefault() {}, } await expect( - runOAuthFlow({ - provider, - store, - scopes: [], - readOnly: false, - flags: {}, - preferredPort: 0, - renderSuccess, - renderError, - openBrowser: async (url) => { - const state = new URL(url).searchParams.get('state') ?? '' - await fetch(`${getRedirect()}?code=abc&state=${state}`) - }, - onAuthorizeUrl: () => undefined, - timeoutMs: 5000, - }), + runOAuthFlow( + flowOptions({ + provider, + store, + openBrowser: driveCallback(getRedirect), + onAuthorizeUrl: () => undefined, + }), + ), ).rejects.toMatchObject({ code: 'AUTH_STORE_WRITE_FAILED' }) }) }) From bf4e307eccfcd36eb4e2bcba8b8a053bb85e13c6 Mon Sep 17 00:00:00 2001 From: Scott Lovegrove Date: Sat, 16 May 2026 17:05:51 +0100 Subject: [PATCH 3/5] fix(auth): quote URL in cmd.exe call; add WSL/headless opener tests MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Review feedback on #29: - **P1 (correctness)**: `cmd.exe /c` re-parses the reconstructed command line, and WSL interop only auto-quotes args containing spaces. OAuth URLs have no spaces but plenty of `&`s, so `cmd.exe` was treating the first `&` as a statement separator and only passing the prefix to `start`. Wrap the URL in literal double quotes so it stays one token, and correct the comment — `execFile`'s no-shell guarantee doesn't apply when the target is itself a shell. - **P2 (test coverage)**: Partial-mock `node:fs` + `node:child_process` so the default-opener branches can be exercised through the public `runOAuthFlow` surface. New tests assert: WSL routes through `cmd.exe` with the quoted URL, headless Linux skips the opener entirely, and `$BROWSER` overrides the headless short-circuit. - **P3 (docs)**: README still said the URL was surfaced only when `open` was missing or threw. Update both spots to reflect the new always-print behavior + the WSL / headless routing. Co-Authored-By: Claude Opus 4.7 (1M context) --- README.md | 8 +-- src/auth/flow.test.ts | 117 +++++++++++++++++++++++++++++++++++++++++- src/auth/flow.ts | 13 +++-- 3 files changed, 129 insertions(+), 9 deletions(-) diff --git a/README.md b/README.md index 2f50dfc..e6828f1 100644 --- a/README.md +++ b/README.md @@ -132,7 +132,7 @@ Wire ` [auth] login` and the supporting OAuth runtime. cli-core ships the s npm install commander open ``` -`commander` is required when using `attachLoginCommand`. `open` is optional — when it's missing or `open()` throws, the authorize URL is surfaced via `onAuthorizeUrl` (or printed to stdout in human mode, stderr in `--json` / `--ndjson` mode) so the user can complete the flow manually. +`commander` is required when using `attachLoginCommand`. `open` is optional. The authorize URL is **always** surfaced via `onAuthorizeUrl` (or printed to stdout in human mode, stderr in `--json` / `--ndjson` mode) — even when the browser launch succeeds — because the launch can resolve cleanly yet open no actual browser (WSL silent no-op, headless Linux, locked-down corporate envs). WSL hosts get routed through `cmd.exe` directly so the user's real Windows browser opens. Headless Linux skips the launch entirely and relies on the URL print. #### Quick start (PKCE) @@ -229,10 +229,10 @@ Both attachers strip the standard `--json` / `--ndjson` / `--user` registrar fla | ------------------------ | -------------------------------------------------------------------------------------- | | `--read-only` | Threaded through to `resolveScopes` and the provider hooks via `readOnly`. | | `--callback-port ` | Override `preferredPort` per invocation. Validated as `[0..65535]`; `0` = OS-assigned. | -| `--json` | Machine-output mode. Authorize-URL fallback is routed to stderr. | -| `--ndjson` | Machine-output mode. Same fallback routing. | +| `--json` | Machine-output mode. Authorize-URL print is routed to stderr. | +| `--ndjson` | Machine-output mode. Same print routing. | -Under `--json` / `--ndjson`, the authorize-URL fallback (printed when `open` is missing or `open()` throws) goes to stderr so the JSON / NDJSON envelope on stdout stays clean. Pass `onAuthorizeUrl` to override the destination. The success / error HTML returned by `renderSuccess` / `renderError` is a render hook — every CLI brings its own template (no shared layout enforced). +Under `--json` / `--ndjson`, the always-printed authorize URL goes to stderr so the JSON / NDJSON envelope on stdout stays clean. Pass `onAuthorizeUrl` to override the destination. The success / error HTML returned by `renderSuccess` / `renderError` is a render hook — every CLI brings its own template (no shared layout enforced). #### Implementing `TokenStore` diff --git a/src/auth/flow.test.ts b/src/auth/flow.test.ts index 9669db5..2dbcdde 100644 --- a/src/auth/flow.test.ts +++ b/src/auth/flow.test.ts @@ -1,5 +1,20 @@ -import { describe, expect, it, vi } from 'vitest' +import { afterEach, describe, expect, it, vi } from 'vitest' +// Partial-mock both modules so the default-opener branches (WSL → `cmd.exe`, +// headless Linux → no opener) can be exercised end-to-end through +// `runOAuthFlow`'s public surface. Tests that inject `openBrowser` never +// touch either mock, so the rest of the suite is unaffected. +vi.mock('node:fs', async (importOriginal) => { + const actual = await importOriginal() + return { ...actual, readFileSync: vi.fn(actual.readFileSync) } +}) +vi.mock('node:child_process', async (importOriginal) => { + const actual = await importOriginal() + return { ...actual, execFile: vi.fn(actual.execFile) } +}) + +import { execFile } from 'node:child_process' +import { readFileSync } from 'node:fs' import { type RunOAuthFlowOptions, runOAuthFlow } from './flow.js' import type { AuthProvider, TokenStore } from './types.js' @@ -315,3 +330,103 @@ describe('runOAuthFlow', () => { ).rejects.toMatchObject({ code: 'AUTH_STORE_WRITE_FAILED' }) }) }) + +describe('runOAuthFlow default opener selection', () => { + const originalPlatform = process.platform + + afterEach(() => { + Object.defineProperty(process, 'platform', { + value: originalPlatform, + configurable: true, + }) + vi.unstubAllEnvs() + vi.mocked(readFileSync).mockReset() + vi.mocked(execFile).mockReset() + }) + + function stubPlatform(value: NodeJS.Platform): void { + Object.defineProperty(process, 'platform', { value, configurable: true }) + } + + it('routes WSL via cmd.exe with the URL wrapped in literal quotes', async () => { + // Quoting the URL is load-bearing: `cmd.exe /c` re-parses the + // command line, and WSL interop only auto-quotes args containing + // spaces. An unquoted OAuth URL would let `&` split the line into + // separate commands and `start` would only see the prefix. + stubPlatform('linux') + vi.mocked(readFileSync).mockReturnValue('Linux 5.15 #1 SMP microsoft-WSL2') + const execFileMock = vi.mocked(execFile) + // The opener wraps execFile in a Promise that resolves on the + // callback; invoke it synchronously with `null` to mimic a clean + // spawn. The 4th arg's exact shape doesn't matter to us. + execFileMock.mockImplementation((( + _cmd: string, + _args: readonly string[], + _opts: unknown, + cb: unknown, + ) => { + ;(cb as (err: Error | null) => void)(null) + return {} as never + }) as never) + + const { provider, getRedirect } = instrument() + const store = fakeStore() + const onAuthorizeUrl = vi.fn(driveCallback(getRedirect)) + + await runOAuthFlow(flowOptions({ provider, store, onAuthorizeUrl })) + + 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}"`]) + }) + + 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 + // print is the only surface. + stubPlatform('linux') + vi.mocked(readFileSync).mockImplementation(() => { + throw new Error('no /proc/version in this test env') + }) + vi.stubEnv('DISPLAY', '') + vi.stubEnv('WAYLAND_DISPLAY', '') + vi.stubEnv('BROWSER', '') + + const { provider, getRedirect } = instrument() + const store = fakeStore() + const onAuthorizeUrl = vi.fn(driveCallback(getRedirect)) + + const result = await runOAuthFlow(flowOptions({ provider, store, onAuthorizeUrl })) + + expect(result.token).toBe('tok-1') + expect(execFile).not.toHaveBeenCalled() + }) + + it('honours $BROWSER on Linux: routes through the `open` peer-dep path', async () => { + // $BROWSER is the explicit user override Codespaces / custom + // remote-bridge setups use to point `open` at their own helper. + // When set, the headless short-circuit must not fire — let `open` + // (or its absence) handle it. We can't load the real `open` here + // without a display, so just assert that the cmd.exe / no-op + // branches are *not* taken; the flow falls through and either + // imports `open` or returns null on import failure. + stubPlatform('linux') + vi.mocked(readFileSync).mockImplementation(() => { + throw new Error('not wsl') + }) + vi.stubEnv('DISPLAY', '') + vi.stubEnv('WAYLAND_DISPLAY', '') + vi.stubEnv('BROWSER', '/usr/local/bin/my-browser') + + const { provider, getRedirect } = instrument() + const store = fakeStore() + const onAuthorizeUrl = vi.fn(driveCallback(getRedirect)) + + const result = await runOAuthFlow(flowOptions({ provider, store, onAuthorizeUrl })) + + expect(result.token).toBe('tok-1') + expect(execFile).not.toHaveBeenCalled() // cmd.exe path not taken + }) +}) diff --git a/src/auth/flow.ts b/src/auth/flow.ts index 375d35b..824559a 100644 --- a/src/auth/flow.ts +++ b/src/auth/flow.ts @@ -486,12 +486,17 @@ async function loadDefaultOpener(): Promise<((url: string) => Promise) | n } // `start ""` — the empty title arg is mandatory; otherwise `start` consumes -// the URL as a window title and never launches a browser. The URL is passed -// as an `execFile` arg (not interpolated into a shell string), so URLs with -// special characters can't escape into command-injection territory. +// the URL as a window title and never launches a browser. The URL itself is +// wrapped in literal double quotes because `cmd.exe /c` is a shell: WSL +// interop reconstructs the command line and only auto-quotes args that +// contain spaces, so an OAuth URL (no spaces, plenty of `&`s) would +// otherwise be re-parsed by `cmd.exe` with `&` acting as a statement +// separator — only the prefix up to the first `&` reaches `start`. The +// embedded quotes keep the URL one token. (`execFile`'s no-shell guarantee +// doesn't apply when the target is itself a shell.) async function openViaCmdExe(url: string): Promise { await new Promise((resolve, reject) => { - execFile('cmd.exe', ['/c', 'start', '""', url], { windowsHide: true }, (error) => { + execFile('cmd.exe', ['/c', 'start', '""', `"${url}"`], { windowsHide: true }, (error) => { if (error) reject(error) else resolve() }) From 57bb1e3a27539889bade81f9e1c5b3ba04839206 Mon Sep 17 00:00:00 2001 From: Scott Lovegrove Date: Sat, 16 May 2026 17:08:57 +0100 Subject: [PATCH 4/5] test(auth): mock the `open` peer-dep in flow.test MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The $BROWSER-routing test previously fell through to the real `open` package. `open` captures `process.platform` at module-load time and ignores our runtime `Object.defineProperty` stub, so on a macOS dev machine it ran its darwin branch and spawned the real \`/usr/bin/open\` — launching live browser tabs from the test runner. Mock `open` at module scope so its mock fn records calls without ever spawning, and assert the mock was invoked with the authorize URL to actually verify the routing. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/auth/flow.test.ts | 21 +++++++++++++++------ 1 file changed, 15 insertions(+), 6 deletions(-) diff --git a/src/auth/flow.test.ts b/src/auth/flow.test.ts index 2dbcdde..8f44abd 100644 --- a/src/auth/flow.test.ts +++ b/src/auth/flow.test.ts @@ -12,9 +12,15 @@ vi.mock('node:child_process', async (importOriginal) => { const actual = await importOriginal() return { ...actual, execFile: vi.fn(actual.execFile) } }) +// Mock the `open` peer-dep too, otherwise the `$BROWSER` test below falls +// through to the real `open` package, which captures `process.platform` at +// module-load (before our runtime stub) and on macOS spawns the real `open` +// command — launching live browser tabs from the test runner. +vi.mock('open', () => ({ default: vi.fn(async () => undefined) })) import { execFile } from 'node:child_process' import { readFileSync } from 'node:fs' +import openBrowserModule from 'open' import { type RunOAuthFlowOptions, runOAuthFlow } from './flow.js' import type { AuthProvider, TokenStore } from './types.js' @@ -342,6 +348,7 @@ describe('runOAuthFlow default opener selection', () => { vi.unstubAllEnvs() vi.mocked(readFileSync).mockReset() vi.mocked(execFile).mockReset() + vi.mocked(openBrowserModule).mockReset() }) function stubPlatform(value: NodeJS.Platform): void { @@ -404,14 +411,14 @@ describe('runOAuthFlow default opener selection', () => { expect(execFile).not.toHaveBeenCalled() }) - it('honours $BROWSER on Linux: routes through the `open` peer-dep path', async () => { + it('honours $BROWSER on Linux: routes through the `open` peer-dep, not cmd.exe', async () => { // $BROWSER is the explicit user override Codespaces / custom // remote-bridge setups use to point `open` at their own helper. // When set, the headless short-circuit must not fire — let `open` - // (or its absence) handle it. We can't load the real `open` here - // without a display, so just assert that the cmd.exe / no-op - // branches are *not* taken; the flow falls through and either - // imports `open` or returns null on import failure. + // handle it. The `open` package is mocked at module scope so the + // real binary never spawns (which would launch live browser tabs + // on macOS dev machines, since `open` captures `process.platform` + // at import time and ignores our runtime stub). stubPlatform('linux') vi.mocked(readFileSync).mockImplementation(() => { throw new Error('not wsl') @@ -427,6 +434,8 @@ describe('runOAuthFlow default opener selection', () => { const result = await runOAuthFlow(flowOptions({ provider, store, onAuthorizeUrl })) expect(result.token).toBe('tok-1') - expect(execFile).not.toHaveBeenCalled() // cmd.exe path not taken + expect(execFile).not.toHaveBeenCalled() + expect(openBrowserModule).toHaveBeenCalledTimes(1) + expect(openBrowserModule).toHaveBeenCalledWith(onAuthorizeUrl.mock.calls[0][0]) }) }) From 18e805f943c7d3c61408ee95c8cb393b5dae64bc Mon Sep 17 00:00:00 2001 From: Scott Lovegrove Date: Sat, 16 May 2026 17:30:16 +0100 Subject: [PATCH 5/5] fix(auth): escape % for cmd.exe; isolate hook failures; widen onAuthorizeUrl MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Second review pass on #29: - **P1**: `cmd.exe` expands `%VAR%` even inside quoted strings. An OAuth URL's percent-encoded bytes (`%3A`, `%2F`, …) could chance-match a defined env var (`%PATH%`, `%TEMP%`, …) and get mangled before `start` sees them. Double every `%` to `%%`; cmd.exe collapses them back to a single `%`. WSL test now uses a URL with both `&` and `%` to exercise both escapes. - **P2**: Isolate `onAuthorizeUrl` from flow control. The hook is purely an output channel; wrap its invocation in try/catch so a buggy logger can't abort an otherwise-working login. New test pins this. - **P2**: Widen `onAuthorizeUrl` to `(url) => void | Promise` and await it. The public contract now matches the sync-or-async reality consumers (and the test helper) already use. - **P2**: Add a no-hook test that pins the `console.log` default print path, so a regression there can't hide behind the hook-driven tests. - **P2**: Headless test now asserts `openBrowserModule` was not called, locking down that no opener path was taken (previously a fall-through to `open` would have left the test green). - **P2**: `mockRestore()` instead of `mockReset()` in the opener-tests afterEach so the factory wraps (`actual.readFileSync`, `actual.execFile`) are restored between tests rather than stripped. - **P3**: `util.promisify(execFile)` replaces the manual Promise wrapper in `openViaCmdExe`. - **P3**: `flowOptions` doc / Pick now reflect that only `provider` and `store` are required; `openBrowser` was already optional via `RunOAuthFlowOptions`. Co-Authored-By: Claude Opus 4.7 (1M context) --- src/auth/flow.test.ts | 99 ++++++++++++++++++++++++++++++++++++------- src/auth/flow.ts | 53 +++++++++++++++-------- 2 files changed, 118 insertions(+), 34 deletions(-) diff --git a/src/auth/flow.test.ts b/src/auth/flow.test.ts index 8f44abd..a510304 100644 --- a/src/auth/flow.test.ts +++ b/src/auth/flow.test.ts @@ -92,12 +92,13 @@ function instrument(provider: Partial> = {}): { /** * Fill in the boilerplate fields every test would otherwise repeat * (`scopes`, `readOnly`, `flags`, `preferredPort`, `renderSuccess`, - * `renderError`, `timeoutMs`). Caller supplies the variants — `provider`, - * `store`, `openBrowser` are required; anything else overrides a default. + * `renderError`, `timeoutMs`). Caller supplies the variants — only + * `provider` and `store` are required; `openBrowser`, `onAuthorizeUrl`, + * etc. all stay optional, matching `RunOAuthFlowOptions`. */ function flowOptions( overrides: Partial> & - Pick, 'provider' | 'store' | 'openBrowser'>, + Pick, 'provider' | 'store'>, ): RunOAuthFlowOptions { return { scopes: [], @@ -290,6 +291,56 @@ describe('runOAuthFlow', () => { expect(result.token).toBe('tok-1') }) + it('prints the authorize URL to stdout when no onAuthorizeUrl hook is supplied (TTY)', async () => { + // No-hook default surface: when the consumer doesn't pass + // `onAuthorizeUrl`, the URL still has to land somewhere — under a + // TTY we fall back to `console.log`. Pin this so a regression in + // the default path can't hide behind the hook-driven tests above. + const originalIsTTY = process.stdout.isTTY + Object.defineProperty(process.stdout, 'isTTY', { value: true, configurable: true }) + const logSpy = vi.spyOn(console, 'log').mockImplementation(() => undefined) + + const { provider, getRedirect } = instrument() + const store = fakeStore() + + try { + const result = await runOAuthFlow( + flowOptions({ provider, store, openBrowser: driveCallback(getRedirect) }), + ) + expect(result.token).toBe('tok-1') + expect(logSpy).toHaveBeenCalledTimes(1) + expect(logSpy.mock.calls[0][0]).toMatch( + /^Open this URL in your browser:\n {2}https:\/\/example\.com\/oauth\/authorize/, + ) + } finally { + logSpy.mockRestore() + Object.defineProperty(process.stdout, 'isTTY', { + value: originalIsTTY, + configurable: true, + }) + } + }) + + it('isolates onAuthorizeUrl hook failures — a throwing hook does not abort login', async () => { + // The hook is purely an output channel. A buggy logger shouldn't + // be able to kill the auth flow. + const { provider, getRedirect } = instrument() + const store = fakeStore() + const onAuthorizeUrl = vi.fn((_url: string) => { + throw new Error('logger boom') + }) + const result = await runOAuthFlow( + flowOptions({ + provider, + store, + openBrowser: driveCallback(getRedirect), + onAuthorizeUrl, + }), + ) + expect(onAuthorizeUrl).toHaveBeenCalledTimes(1) + expect(result.token).toBe('tok-1') + }) + it('falls back to onAuthorizeUrl when the openBrowser opener throws', async () => { const { provider, getRedirect } = instrument() const store = fakeStore() @@ -346,26 +397,30 @@ describe('runOAuthFlow default opener selection', () => { configurable: true, }) vi.unstubAllEnvs() - vi.mocked(readFileSync).mockReset() - vi.mocked(execFile).mockReset() - vi.mocked(openBrowserModule).mockReset() + // `mockRestore` reverts to the factory impl (`actual.readFileSync`, + // `actual.execFile`, the empty `open` mock). `mockReset` would + // strip the factory wrap and leave the next call returning + // undefined, which would silently break any test that forgets to + // set its own implementation. + vi.mocked(readFileSync).mockRestore() + vi.mocked(execFile).mockRestore() + vi.mocked(openBrowserModule).mockRestore() }) function stubPlatform(value: NodeJS.Platform): void { Object.defineProperty(process, 'platform', { value, configurable: true }) } - it('routes WSL via cmd.exe with the URL wrapped in literal quotes', async () => { - // Quoting the URL is load-bearing: `cmd.exe /c` re-parses the - // command line, and WSL interop only auto-quotes args containing - // spaces. An unquoted OAuth URL would let `&` split the line into - // separate commands and `start` would only see the prefix. + 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. stubPlatform('linux') vi.mocked(readFileSync).mockReturnValue('Linux 5.15 #1 SMP microsoft-WSL2') const execFileMock = vi.mocked(execFile) - // The opener wraps execFile in a Promise that resolves on the - // callback; invoke it synchronously with `null` to mimic a clean - // spawn. The 4th arg's exact shape doesn't matter to us. execFileMock.mockImplementation((( _cmd: string, _args: readonly string[], @@ -376,7 +431,12 @@ describe('runOAuthFlow default opener selection', () => { return {} as never }) as never) - const { provider, getRedirect } = instrument() + const { provider, getRedirect } = instrument({ + authorize: async (input) => ({ + authorizeUrl: `https://example.com/oauth/authorize?state=${input.state}&redirect_uri=http%3A%2F%2Flocalhost%3A8080`, + handshake: { codeVerifier: 'v1' }, + }), + }) const store = fakeStore() const onAuthorizeUrl = vi.fn(driveCallback(getRedirect)) @@ -386,7 +446,10 @@ 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}"`]) + expect(args).toEqual(['/c', 'start', '""', `"${url.replaceAll('%', '%%')}"`]) + // Sanity: the URL we built does contain both special chars. + expect(url).toMatch(/&/) + expect(url).toMatch(/%/) }) it('skips the default opener entirely on headless Linux', async () => { @@ -409,6 +472,10 @@ describe('runOAuthFlow default opener selection', () => { expect(result.token).toBe('tok-1') expect(execFile).not.toHaveBeenCalled() + // Lock down "no opener taken": without this, an accidental fall-through + // to the `open` peer-dep would still pass since `onAuthorizeUrl` is + // driving the callback anyway. + expect(openBrowserModule).not.toHaveBeenCalled() }) it('honours $BROWSER on Linux: routes through the `open` peer-dep, not cmd.exe', async () => { diff --git a/src/auth/flow.ts b/src/auth/flow.ts index 824559a..d95611b 100644 --- a/src/auth/flow.ts +++ b/src/auth/flow.ts @@ -1,6 +1,7 @@ import { execFile } from 'node:child_process' import { readFileSync } from 'node:fs' import { type IncomingMessage, type Server, type ServerResponse, createServer } from 'node:http' +import { promisify } from 'node:util' import { CliError, getErrorMessage } from '../errors.js' import { isStdoutTTY } from '../terminal.js' import { generateState } from './pkce.js' @@ -55,8 +56,15 @@ export type RunOAuthFlowOptions = { renderError: (message: string) => string /** Override the browser opener (tests). When omitted, dynamically imports `open`. */ openBrowser?: (url: string) => Promise - /** Print the authorize URL to stdout as a fallback when the browser can't open it. */ - onAuthorizeUrl?: (url: string) => void + /** + * Receives the authorize URL on every login attempt — not only when the + * browser launch fails — because the launch can resolve cleanly yet + * open no browser (WSL no-op, headless Linux, locked-down hosts). + * Treat as a fire-and-forget output channel: a sync hook is awaited, + * an async hook is awaited too, and any throw / rejection is swallowed + * so a buggy logger can never abort an otherwise-working login. + */ + onAuthorizeUrl?: (url: string) => void | Promise /** Callback timeout in ms. Default 3 minutes. */ timeoutMs?: number /** Cancellation signal (Ctrl-C wiring). */ @@ -457,8 +465,15 @@ async function openOrFallback( // headless Linux, locked-down corporate envs, missing `open` peer), and // we have no reliable signal that the user actually landed on the page // — so printing here guarantees a copy-pasteable path on every platform. - if (options.onAuthorizeUrl) options.onAuthorizeUrl(url) - else if (isStdoutTTY()) console.log(`Open this URL in your browser:\n ${url}`) + // The hook is purely an output channel: isolate its failures so a buggy + // logger can't abort the login that the user is actually here to do. + if (options.onAuthorizeUrl) { + try { + await options.onAuthorizeUrl(url) + } catch { + // Hook errors don't propagate. + } + } else if (isStdoutTTY()) console.log(`Open this URL in your browser:\n ${url}`) const opener = options.openBrowser ?? (await loadDefaultOpener()) if (!opener) return @@ -485,20 +500,22 @@ 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. The URL itself is -// wrapped in literal double quotes because `cmd.exe /c` is a shell: WSL -// interop reconstructs the command line and only auto-quotes args that -// contain spaces, so an OAuth URL (no spaces, plenty of `&`s) would -// otherwise be re-parsed by `cmd.exe` with `&` acting as a statement -// separator — only the prefix up to the first `&` reaches `start`. The -// embedded quotes keep the URL one token. (`execFile`'s no-shell guarantee -// doesn't apply when the target is itself a shell.) +// 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 new Promise((resolve, reject) => { - execFile('cmd.exe', ['/c', 'start', '""', `"${url}"`], { windowsHide: true }, (error) => { - if (error) reject(error) - else resolve() - }) - }) + const escaped = url.replaceAll('%', '%%') + await execFileAsync('cmd.exe', ['/c', 'start', '""', `"${escaped}"`], { windowsHide: true }) }