From ba6e5298df7a1b4a9dc58eaec4e2b3a06270ec0c Mon Sep 17 00:00:00 2001 From: Armin Ronacher Date: Tue, 2 Jun 2026 23:15:46 +0200 Subject: [PATCH] fix(oauth): harden browser launch handling --- packages/ai/src/utils/oauth/github-copilot.ts | 14 ++- packages/ai/src/utils/oauth/openai-codex.ts | 7 +- packages/ai/test/github-copilot-oauth.test.ts | 91 +++++++++++++++++++ .../interactive/components/login-dialog.ts | 15 +-- .../coding-agent/src/utils/open-browser.ts | 24 +++++ scripts/tool-stats.ts | 4 +- 6 files changed, 138 insertions(+), 17 deletions(-) create mode 100644 packages/coding-agent/src/utils/open-browser.ts diff --git a/packages/ai/src/utils/oauth/github-copilot.ts b/packages/ai/src/utils/oauth/github-copilot.ts index ba371b8f2..6a27b9d12 100644 --- a/packages/ai/src/utils/oauth/github-copilot.ts +++ b/packages/ai/src/utils/oauth/github-copilot.ts @@ -132,10 +132,22 @@ async function startDeviceFlow(domain: string): Promise { throw new Error("Invalid device code response fields"); } + // The verification URI is opened in the user's browser and to prevent `open` from + // opening an executable or similar, we force it to be a URL. + let parsedUri: URL; + try { + parsedUri = new URL(verificationUri); + } catch { + throw new Error("Untrusted verification_uri in device code response"); + } + if (parsedUri.protocol !== "https:" && parsedUri.protocol !== "http:") { + throw new Error("Untrusted verification_uri in device code response"); + } + return { device_code: deviceCode, user_code: userCode, - verification_uri: verificationUri, + verification_uri: parsedUri.href, interval, expires_in: expiresIn, }; diff --git a/packages/ai/src/utils/oauth/openai-codex.ts b/packages/ai/src/utils/oauth/openai-codex.ts index 51ea832b3..2f769a841 100644 --- a/packages/ai/src/utils/oauth/openai-codex.ts +++ b/packages/ai/src/utils/oauth/openai-codex.ts @@ -28,7 +28,6 @@ import type { OAuthProviderInterface, } from "./types.ts"; -const CALLBACK_HOST = process.env.PI_OAUTH_CALLBACK_HOST || "127.0.0.1"; const CLIENT_ID = "app_EMoamEEZ73f0CkXaXp7hrann"; const AUTH_BASE_URL = "https://auth.openai.com"; const AUTHORIZE_URL = `${AUTH_BASE_URL}/oauth/authorize`; @@ -47,6 +46,10 @@ const JWT_CLAIM_PATH = "https://api.openai.com/auth"; type OAuthToken = { access: string; refresh: string; expires: number }; type TokenOperation = "exchange" | "refresh"; +function getCallbackHost(): string { + return typeof process !== "undefined" ? process.env.PI_OAUTH_CALLBACK_HOST || "127.0.0.1" : "127.0.0.1"; +} + type DeviceAuthInfo = { deviceAuthId: string; userCode: string; @@ -368,7 +371,7 @@ function startLocalOAuthServer(state: string): Promise { return new Promise((resolve) => { server - .listen(1455, CALLBACK_HOST, () => { + .listen(1455, getCallbackHost(), () => { resolve({ close: () => server.close(), cancelWait: () => { diff --git a/packages/ai/test/github-copilot-oauth.test.ts b/packages/ai/test/github-copilot-oauth.test.ts index 324883ecc..c94370da4 100644 --- a/packages/ai/test/github-copilot-oauth.test.ts +++ b/packages/ai/test/github-copilot-oauth.test.ts @@ -84,6 +84,97 @@ describe("GitHub Copilot OAuth device flow", () => { await loginPromise; }); + it("rejects a non-http(s) verification_uri before it reaches onDeviceCode", async () => { + // A malicious enterprise OAuth server could return a verification_uri that + // the browser launcher would otherwise hand to the OS. Ensure such values + // are rejected at the deserialization boundary. + const fetchMock = vi.fn(async (input: unknown): Promise => { + const url = getUrl(input); + if (url.endsWith("/login/device/code")) { + return jsonResponse({ + device_code: "device-code", + user_code: "ABCD-EFGH", + verification_uri: "$(id>/tmp/pwned)", + interval: 1, + expires_in: 900, + }); + } + throw new Error(`Unexpected fetch URL: ${url}`); + }); + + vi.stubGlobal("fetch", fetchMock); + + const onDeviceCode = vi.fn(); + await expect( + loginGitHubCopilot({ + onDeviceCode, + onPrompt: async () => "", + }), + ).rejects.toThrow(/Untrusted verification_uri/); + expect(onDeviceCode).not.toHaveBeenCalled(); + }); + + it("normalizes verification_uri before it reaches onDeviceCode", async () => { + vi.useFakeTimers(); + vi.setSystemTime(new Date("2026-03-09T00:00:00Z")); + + const rawVerificationUri = "https://github.com/login/\x1b]8;;evil"; + const normalizedVerificationUri = new URL(rawVerificationUri).href; + expect(normalizedVerificationUri).not.toBe(rawVerificationUri); + + const fetchMock = vi.fn(async (input: unknown): Promise => { + const url = getUrl(input); + + if (url.endsWith("/login/device/code")) { + return jsonResponse({ + device_code: "device-code", + user_code: "ABCD-EFGH", + verification_uri: rawVerificationUri, + interval: 1, + expires_in: 900, + }); + } + + if (url.endsWith("/login/oauth/access_token")) { + return jsonResponse({ access_token: "ghu_refresh_token" }); + } + + if (url.includes("/copilot_internal/v2/token")) { + return jsonResponse({ + token: "tid=test;exp=9999999999;proxy-ep=proxy.individual.githubcopilot.com;", + expires_at: 9999999999, + }); + } + + if (url.includes("/models/") && url.endsWith("/policy")) { + return new Response("", { status: 200 }); + } + + throw new Error(`Unexpected fetch URL: ${url}`); + }); + + vi.stubGlobal("fetch", fetchMock); + + const onDeviceCode = vi.fn(); + const loginPromise = loginGitHubCopilot({ + onDeviceCode, + onPrompt: async () => "", + }); + + await vi.advanceTimersByTimeAsync(0); + + expect(onDeviceCode).toHaveBeenCalledWith({ + userCode: "ABCD-EFGH", + verificationUri: normalizedVerificationUri, + intervalSeconds: 1, + expiresInSeconds: 900, + }); + expect(onDeviceCode).not.toHaveBeenCalledWith(expect.objectContaining({ verificationUri: rawVerificationUri })); + + await vi.advanceTimersByTimeAsync(1000); + await loginPromise; + }); + it("polls immediately and increases the interval after slow_down", async () => { vi.useFakeTimers(); const startTime = new Date("2026-03-09T00:00:00Z"); diff --git a/packages/coding-agent/src/modes/interactive/components/login-dialog.ts b/packages/coding-agent/src/modes/interactive/components/login-dialog.ts index 3ae9c92dd..84d9a2c1b 100644 --- a/packages/coding-agent/src/modes/interactive/components/login-dialog.ts +++ b/packages/coding-agent/src/modes/interactive/components/login-dialog.ts @@ -1,6 +1,6 @@ import { getOAuthProviders, type OAuthDeviceCodeInfo } from "@earendil-works/pi-ai/oauth"; import { Container, type Focusable, getKeybindings, Input, Spacer, Text, type TUI } from "@earendil-works/pi-tui"; -import { exec } from "child_process"; +import { openBrowser } from "../../../utils/open-browser.ts"; import { theme } from "../theme/theme.ts"; import { DynamicBorder } from "./dynamic-border.ts"; import { keyHint } from "./keybinding-hints.ts"; @@ -101,7 +101,7 @@ export class LoginDialogComponent extends Container implements Focusable { this.contentContainer.addChild(new Text(theme.fg("warning", instructions), 1, 0)); } - this.openUrl(url); + openBrowser(url); this.tui.requestRender(); } @@ -120,19 +120,10 @@ export class LoginDialogComponent extends Container implements Focusable { this.contentContainer.addChild(new Spacer(1)); this.contentContainer.addChild(new Text(theme.fg("warning", `Enter code: ${info.userCode}`), 1, 0)); - this.openUrl(info.verificationUri); + openBrowser(info.verificationUri); this.tui.requestRender(); } - private openUrl(url: string): void { - const openCmd = process.platform === "darwin" ? "open" : process.platform === "win32" ? "start" : "xdg-open"; - try { - exec(`${openCmd} "${url}"`, () => {}); - } catch { - // Ignore browser launch failures. The URL remains visible for manual opening/copying. - } - } - /** * Show input for manual code/URL entry (for callback server providers) */ diff --git a/packages/coding-agent/src/utils/open-browser.ts b/packages/coding-agent/src/utils/open-browser.ts new file mode 100644 index 000000000..435e23f97 --- /dev/null +++ b/packages/coding-agent/src/utils/open-browser.ts @@ -0,0 +1,24 @@ +import { spawn } from "node:child_process"; + +/** + * Open a URL or file in the platform browser/default handler. + * + * This intentionally never invokes a shell. On Windows, do not use + * `cmd /c start`: cmd.exe re-parses metacharacters (&, |, ^, ...) before + * `start` runs, which would make attacker-controlled URLs injectable. + */ +export function openBrowser(target: string): void { + const [cmd, args]: [string, string[]] = + process.platform === "darwin" + ? ["open", [target]] + : process.platform === "win32" + ? ["rundll32", ["url.dll,FileProtocolHandler", target]] + : ["xdg-open", [target]]; + + // spawn reports launcher failures (for example, missing xdg-open) via an + // error event. Browser launch is best-effort: callers still present the target + // to the user, so keep the launcher failure from becoming a process crash. + spawn(cmd, args, { stdio: "ignore", detached: true }) + .on("error", () => {}) + .unref(); +} diff --git a/scripts/tool-stats.ts b/scripts/tool-stats.ts index 78a694187..742e0a83e 100755 --- a/scripts/tool-stats.ts +++ b/scripts/tool-stats.ts @@ -3,7 +3,7 @@ import { existsSync, mkdirSync, readdirSync, readFileSync, writeFileSync } from "node:fs"; import { homedir, tmpdir } from "node:os"; import { join, resolve } from "node:path"; -import { spawn } from "node:child_process"; +import { openBrowser } from "../packages/coding-agent/src/utils/open-browser.ts"; interface TextContent { type: "text"; text: string } interface ImageContent { type: "image"; data: string; mimeType?: string } @@ -229,4 +229,4 @@ const html = ` mkdirSync(resolve(output, ".."), { recursive: true }); writeFileSync(output, html); console.log(`Wrote ${output}`); -spawn(process.platform === "darwin" ? "open" : process.platform === "win32" ? "cmd" : "xdg-open", process.platform === "win32" ? ["/c", "start", output] : [output], { detached: true, stdio: "ignore" }).unref(); +openBrowser(output);