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/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 46eebe64b..5ddda5242 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -11,6 +11,7 @@ ### Fixed +- Fixed git package source handling to reject unsafe host/path components and keep managed clone paths inside install roots. - Fixed stored XSS in HTML session exports by sanitizing Markdown link and image URLs with a scheme allow-list after stripping control characters. - Fixed SDK embedding in bundled Node apps failing with `ENOENT` when `package.json` is not present next to the bundle entrypoint. The package metadata reader now gracefully handles missing `package.json` by using defaults, enabling `createAgentSession()` without requiring package-adjacent files at runtime ([#5226](https://github.com/earendil-works/pi/issues/5226)). - Fixed HTTP timeout setting not being respected for non-Codex providers (e.g., llama.cpp via OpenAI-compatible API). The `httpIdleTimeoutMs` setting (set via `/settings` HTTP timeout) now applies as the default SDK request timeout for all providers that support it, not just OpenAI Codex Responses. Disabling the timeout (HTTP timeout = false) now correctly disables SDK timeouts for all supported providers by sending a maximum int32 value (effectively infinite) instead of 0, since SDKs treat timeout=0 as an immediate timeout ([#5294](https://github.com/earendil-works/pi/issues/5294)). 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/git.ts b/packages/coding-agent/src/utils/git.ts index 9652e4018..1314edea7 100644 --- a/packages/coding-agent/src/utils/git.ts +++ b/packages/coding-agent/src/utils/git.ts @@ -73,6 +73,56 @@ function splitRef(url: string): { repo: string; ref?: string } { }; } +function decodeForValidation(value: string): string | null { + try { + return decodeURIComponent(value); + } catch { + return null; + } +} + +function hasUnsafeGitInstallPart(value: string, allowSlash: boolean): boolean { + const decoded = decodeForValidation(value); + if (decoded === null) { + return true; + } + const candidates = [value, decoded]; + for (const candidate of candidates) { + if (candidate.includes("\0") || candidate.includes("\\") || candidate.startsWith("/")) { + return true; + } + if (!allowSlash && candidate.includes("/")) { + return true; + } + if (candidate.split("/").includes("..")) { + return true; + } + } + return false; +} + +function buildGitSource(args: { repo: string; host: string; path: string; ref?: string }): GitSource | null { + if (args.path.startsWith("/")) { + return null; + } + const normalizedPath = args.path.replace(/\.git$/, "").replace(/^\/+/, ""); + if (!args.host || !normalizedPath || normalizedPath.split("/").length < 2) { + return null; + } + if (hasUnsafeGitInstallPart(args.host, false) || hasUnsafeGitInstallPart(normalizedPath, true)) { + return null; + } + + return { + type: "git", + repo: args.repo, + host: args.host, + path: normalizedPath, + ref: args.ref, + pinned: Boolean(args.ref), + }; +} + function parseGenericGitUrl(url: string): GitSource | null { const { repo: repoWithoutRef, ref } = splitRef(url); let repo = repoWithoutRef; @@ -109,19 +159,7 @@ function parseGenericGitUrl(url: string): GitSource | null { repo = `https://${repoWithoutRef}`; } - const normalizedPath = path.replace(/\.git$/, "").replace(/^\/+/, ""); - if (!host || !normalizedPath || normalizedPath.split("/").length < 2) { - return null; - } - - return { - type: "git", - repo, - host, - path: normalizedPath, - ref, - pinned: Boolean(ref), - }; + return buildGitSource({ repo, host, path, ref }); } /** @@ -157,14 +195,12 @@ export function parseGitUrl(source: string): GitSource | null { !split.repo.startsWith("ssh://") && !split.repo.startsWith("git://") && !split.repo.startsWith("git@"); - return { - type: "git", + return buildGitSource({ repo: useHttpsPrefix ? `https://${split.repo}` : split.repo, host: info.domain || "", - path: `${info.user}/${info.project}`.replace(/\.git$/, ""), + path: `${info.user}/${info.project}`, ref: info.committish || split.ref || undefined, - pinned: Boolean(info.committish || split.ref), - }; + }); } } @@ -177,14 +213,12 @@ export function parseGitUrl(source: string): GitSource | null { if (split.ref && info.project?.includes("@")) { continue; } - return { - type: "git", + return buildGitSource({ repo: `https://${split.repo}`, host: info.domain || "", - path: `${info.user}/${info.project}`.replace(/\.git$/, ""), + path: `${info.user}/${info.project}`, ref: info.committish || split.ref || undefined, - pinned: Boolean(info.committish || split.ref), - }; + }); } } 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/packages/coding-agent/test/git-ssh-url.test.ts b/packages/coding-agent/test/git-ssh-url.test.ts index 6913b70c4..87b8b9400 100644 --- a/packages/coding-agent/test/git-ssh-url.test.ts +++ b/packages/coding-agent/test/git-ssh-url.test.ts @@ -62,6 +62,19 @@ describe("Git URL Parsing", () => { }); }); + it("should reject unsafe git install path inputs", () => { + for (const source of [ + "git:git@evil.example:../../victim/repo", + "https://evil.example/..%2F..%2Fvictim/repo", + "https://evil.example/..%2F..%2Fvictim/repo%", + "git:git@evil.example:/absolute/repo", + "git:git@evil.example:user\\repo/name", + "git:git@evil.example:user/repo\0name", + ]) { + expect(parseGitUrl(source)).toBeNull(); + } + }); + describe("unsupported without git: prefix", () => { it("should reject git@host:path without git: prefix", () => { expect(parseGitUrl("git@github.com:user/repo")).toBeNull(); 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);