From 9cd62cf3bb7b9e86f0b08894a1c48bbdb79a9488 Mon Sep 17 00:00:00 2001 From: ashCold <101525936+AayushThakur1999@users.noreply.github.com> Date: Fri, 19 Jun 2026 21:28:17 +0530 Subject: [PATCH] fix(cli): handle headless browser-open failure in board auth (#8328) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open-source app for managing AI agents, very commonly self-hosted as a headless Docker container. > - Its CLI authorizes board/admin actions through an interactive browser-approval flow (`auth login`, and the auto-recover path behind commands like `run list`). > - That flow calls `openUrl()`, which `spawn`s the OS browser opener (`xdg-open` on Linux). > - In a headless container there is no `xdg-open`; `spawn` reports the missing binary asynchronously via an `'error'` event, which the surrounding `try/catch` cannot catch, so Node aborts the process before the approval can be polled. > - This makes CLI board auth impossible in the most common self-hosted deployment shape. > - This PR makes `openUrl` error-handled, async, and truthful, and adds headless affordances. > - The benefit is that board-authenticated CLI commands degrade gracefully and work headless instead of crashing. ## Linked Issues or Issue Description Closes #7941 ## What Changed - `openUrl` (`cli/src/client/board-auth.ts`) is now async and attaches an `'error'` listener to the spawned opener: resolves `false` on async spawn failure (missing binary) or sync throw, and `true` only on a successful `'spawn'`. Fixes the unhandled-`'error'` crash and makes the return value honest. - `loginBoardCli` prints an accurate "couldn't open a browser" message, supports `--no-browser` / `PAPERCLIP_NO_BROWSER` to skip the open attempt, and renders the approval URL from `PAPERCLIP_PUBLIC_URL` (or `publicBaseUrl`) so it's reachable from a remote operator's browser. - Updated the three other `openUrl` call sites (`cloud.ts` ×2, `company.ts`) to `await` it. - Added the `auth login --no-browser` flag. - Tests: new `open-url.test.ts` (launch → true, async ENOENT → false, sync throw → false); extended auth-command-registration test for `--no-browser`. ## Verification - `pnpm --filter paperclipai typecheck` — clean. - `pnpm exec vitest run cli/src/__tests__/open-url.test.ts cli/src/__tests__/auth-command-registration.test.ts` — pass. - Manual, in a headless container with no `xdg-open`: `pnpm paperclipai auth login -C ` now prints the approval URL and waits (previously crashed with `spawn xdg-open ENOENT`); `--no-browser` skips the open attempt; `PAPERCLIP_PUBLIC_URL=...` renders a reachable approval URL; completing approval in a browser stores the credential and `run list` works. ## Risks Low. `openUrl` became async; all four call sites updated to `await`. Desktop behavior is unchanged (successful spawn still resolves true and opens the browser). No API, schema, or migration changes. ## Model Used Claude Opus 4.8 (`claude-opus-4-8`) via Claude Code, with extended thinking and tool use, used to diagnose the bug and draft the fix and tests. Human-reviewed and tested on a live headless deployment. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes: #` / `Closes #` / `Refs #` OR (b) described the issue in-PR - [x] I have not referenced internal/instance-local Paperclip issues or links - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [ ] If this change affects the UI, I have included before/after screenshots (N/A — CLI only) - [ ] I have updated relevant documentation to reflect my changes - [x] I have considered and documented any risks above - [ ] All Paperclip CI gates are green - [ ] Greptile is 5/5 with no open P2s, recommendations, or follow-ups - [ ] I will address all Greptile and reviewer comments before requesting merge Co-authored-by: Paperclip --- .../auth-command-registration.test.ts | 9 +++ cli/src/__tests__/open-url.test.ts | 57 +++++++++++++++++ cli/src/client/board-auth.ts | 64 ++++++++++++------- cli/src/commands/client/auth.ts | 3 + cli/src/commands/client/cloud.ts | 4 +- cli/src/commands/client/company.ts | 2 +- 6 files changed, 114 insertions(+), 25 deletions(-) create mode 100644 cli/src/__tests__/open-url.test.ts diff --git a/cli/src/__tests__/auth-command-registration.test.ts b/cli/src/__tests__/auth-command-registration.test.ts index 76b3c51d..98553760 100644 --- a/cli/src/__tests__/auth-command-registration.test.ts +++ b/cli/src/__tests__/auth-command-registration.test.ts @@ -13,6 +13,15 @@ describe("registerClientAuthCommands", () => { expect(login).toBeDefined(); expect(login?.options.filter((option) => option.long === "--company-id")).toHaveLength(1); }); + + it("registers the --no-browser flag on login", () => { + const program = new Command(); + const auth = program.command("auth"); + registerClientAuthCommands(auth); + + const login = auth.commands.find((command) => command.name() === "login"); + expect(login?.options.some((option) => option.long === "--no-browser")).toBe(true); + }); }); describe("client auth API commands", () => { diff --git a/cli/src/__tests__/open-url.test.ts b/cli/src/__tests__/open-url.test.ts new file mode 100644 index 00000000..d5c31d7b --- /dev/null +++ b/cli/src/__tests__/open-url.test.ts @@ -0,0 +1,57 @@ +import { EventEmitter } from "node:events"; +import { afterEach, describe, expect, it, vi } from "vitest"; + +// Lets each test decide what the next spawned child does, while the +// node:child_process mock below is hoisted above the board-auth import. +const mocks = vi.hoisted(() => ({ spawn: vi.fn() })); + +vi.mock("node:child_process", async () => { + const actual = await vi.importActual("node:child_process"); + return { ...actual, spawn: mocks.spawn }; +}); + +import { openUrl } from "../client/board-auth.js"; + +function fakeChild(): EventEmitter & { unref: () => void } { + const child = new EventEmitter() as EventEmitter & { unref: () => void }; + child.unref = () => {}; + return child; +} + +describe("openUrl", () => { + afterEach(() => { + vi.clearAllMocks(); + }); + + it("resolves true when the browser opener launches", async () => { + mocks.spawn.mockImplementation(() => { + const child = fakeChild(); + queueMicrotask(() => child.emit("spawn")); + return child; + }); + + await expect(openUrl("https://example.com")).resolves.toBe(true); + }); + + it("resolves false instead of crashing when the opener is missing", async () => { + // Headless container case: e.g. no `xdg-open`. spawn reports this via an + // async 'error' event, not a thrown exception — openUrl must swallow it and + // resolve false rather than letting an unhandled 'error' abort the process. + mocks.spawn.mockImplementation(() => { + const child = fakeChild(); + const error = Object.assign(new Error("spawn xdg-open ENOENT"), { code: "ENOENT" }); + queueMicrotask(() => child.emit("error", error)); + return child; + }); + + await expect(openUrl("https://example.com")).resolves.toBe(false); + }); + + it("resolves false when spawn throws synchronously", async () => { + mocks.spawn.mockImplementation(() => { + throw new Error("synchronous spawn failure"); + }); + + await expect(openUrl("https://example.com")).resolves.toBe(false); + }); +}); diff --git a/cli/src/client/board-auth.ts b/cli/src/client/board-auth.ts index 7c1121ec..0f6fe8e8 100644 --- a/cli/src/client/board-auth.ts +++ b/cli/src/client/board-auth.ts @@ -1,4 +1,4 @@ -import { spawn } from "node:child_process"; +import { spawn, type ChildProcess } from "node:child_process"; import fs from "node:fs"; import path from "node:path"; import pc from "picocolors"; @@ -60,6 +60,11 @@ function normalizeApiBase(apiBase: string): string { return apiBase.trim().replace(/\/+$/, ""); } +function isTruthyEnv(value: string | undefined): boolean { + const v = value?.trim().toLowerCase(); + return v === "1" || v === "true" || v === "yes"; +} + export function resolveBoardAuthStorePath(overridePath?: string): string { if (overridePath?.trim()) return path.resolve(overridePath.trim()); if (process.env.PAPERCLIP_AUTH_STORE?.trim()) return path.resolve(process.env.PAPERCLIP_AUTH_STORE.trim()); @@ -169,25 +174,28 @@ async function requestJson(url: string, init?: RequestInit): Promise { return response.json() as Promise; } -export function openUrl(url: string): boolean { - const platform = process.platform; - try { - if (platform === "darwin") { - const child = spawn("open", [url], { detached: true, stdio: "ignore" }); - child.unref(); - return true; +export async function openUrl(url: string): Promise { + const { command, args } = + process.platform === "darwin" + ? { command: "open", args: [url] } + : process.platform === "win32" + ? { command: "cmd", args: ["/c", "start", "", url] } + : { command: "xdg-open", args: [url] }; + + return new Promise((resolve) => { + let child: ChildProcess; + try { + child = spawn(command, args, { detached: true, stdio: "ignore" }); + } catch { + resolve(false); + return; } - if (platform === "win32") { - const child = spawn("cmd", ["/c", "start", "", url], { detached: true, stdio: "ignore" }); + child.once("error", () => resolve(false)); + child.once("spawn", () => { child.unref(); - return true; - } - const child = spawn("xdg-open", [url], { detached: true, stdio: "ignore" }); - child.unref(); - return true; - } catch { - return false; - } + resolve(true); + }); + }); } export async function loginBoardCli(params: { @@ -198,6 +206,8 @@ export async function loginBoardCli(params: { command?: string; storePath?: string; print?: boolean; + openBrowser?: boolean; + publicBaseUrl?: string; }): Promise<{ token: string; approvalUrl: string; userId?: string | null }> { const apiBase = normalizeApiBase(params.apiBase); const createUrl = `${apiBase}/api/cli-auth/challenges`; @@ -213,15 +223,25 @@ export async function loginBoardCli(params: { }), }); - const approvalUrl = challenge.approvalUrl ?? `${apiBase}${challenge.approvalPath}`; + const publicBase = params.publicBaseUrl?.trim() || process.env.PAPERCLIP_PUBLIC_URL?.trim(); + const approvalUrl = publicBase + ? `${normalizeApiBase(publicBase)}${challenge.approvalPath}` + : challenge.approvalUrl ?? `${apiBase}${challenge.approvalPath}`; + if (params.print !== false) { console.error(pc.bold("Board authentication required")); console.error(`Open this URL in your browser to approve CLI access:\n${approvalUrl}`); } - const opened = openUrl(approvalUrl); - if (params.print !== false && opened) { - console.error(pc.dim("Opened the approval page in your browser.")); + const wantBrowser = params.openBrowser !== false && !isTruthyEnv(process.env.PAPERCLIP_NO_BROWSER); + const opened = wantBrowser ? await openUrl(approvalUrl) : false; + if (params.print !== false) { + const browserMessage = !wantBrowser + ? "Browser open skipped — open the URL above to approve." + : opened + ? "Opened the approval page in your browser." + : "Couldn't open a browser automatically — open the URL above to approve."; + console.error(pc.dim(browserMessage)); } const expiresAtMs = Date.parse(challenge.expiresAt); diff --git a/cli/src/commands/client/auth.ts b/cli/src/commands/client/auth.ts index 1d1b1551..549f119c 100644 --- a/cli/src/commands/client/auth.ts +++ b/cli/src/commands/client/auth.ts @@ -16,6 +16,7 @@ import { interface AuthLoginOptions extends BaseClientOptions { instanceAdmin?: boolean; + browser?: boolean; } interface AuthLogoutOptions extends BaseClientOptions {} @@ -32,6 +33,7 @@ export function registerClientAuthCommands(auth: Command): void { .command("login") .description("Authenticate the CLI for board-user access") .option("--instance-admin", "Request instance-admin approval instead of plain board access", false) + .option("--no-browser", "Don't try to open a browser; just print the approval URL") .action(async (opts: AuthLoginOptions) => { try { const ctx = resolveCommandContext(opts); @@ -40,6 +42,7 @@ export function registerClientAuthCommands(auth: Command): void { requestedAccess: opts.instanceAdmin ? "instance_admin_required" : "board", requestedCompanyId: ctx.companyId ?? null, command: "paperclipai auth login", + openBrowser: opts.browser, }); printOutput( { diff --git a/cli/src/commands/client/cloud.ts b/cli/src/commands/client/cloud.ts index c48a8779..1e3046f1 100644 --- a/cli/src/commands/client/cloud.ts +++ b/cli/src/commands/client/cloud.ts @@ -366,7 +366,7 @@ async function authorizeWithBrowser( try { console.error(`Open this URL to approve cloud sync:\n${authorizeUrl.toString()}`); - if (!openUrl(authorizeUrl.toString())) { + if (!(await openUrl(authorizeUrl.toString()))) { throw new Error("Could not open a browser."); } const code = await callback.waitForCode(state); @@ -410,7 +410,7 @@ async function authorizeWithDeviceCode( console.error(pc.bold("Cloud device authorization required")); console.error(`Open: ${response.verificationUri}`); console.error(`Code: ${response.userCode}`); - if (opts.openBrowser) openUrl(response.verificationUri); + if (opts.openBrowser) await openUrl(response.verificationUri); const expiresAt = resolveDeviceCodeExpiresAt(response.expiresAt); const intervalMs = Math.max(500, (response.intervalSeconds ?? 5) * 1000); diff --git a/cli/src/commands/client/company.ts b/cli/src/commands/client/company.ts index 682b82ac..ee702d1b 100644 --- a/cli/src/commands/client/company.ts +++ b/cli/src/commands/client/company.ts @@ -1594,7 +1594,7 @@ export function registerCompanyCommands(program: Command): void { initialValue: true, }); if (!p.isCancel(openImportedCompany) && openImportedCompany) { - if (openUrl(companyUrl)) { + if (await openUrl(companyUrl)) { p.log.info(`Opened ${companyUrl}`); } else { p.log.warn(`Could not open your browser automatically. Open this URL manually:\n${companyUrl}`);