From bf7b3951c58fb60f7e5c061c4c7001e1e2399331 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 25 Sep 2026 23:13:14 -0400 Subject: [PATCH 1/2] fix(cli): bound the OAuth browser open and say when it fails (#2410) openUrl now races `open` against a 5s timeout, so a stalled opener on a headless box or container cannot hang the interactive OAuth flow. When the open fails or times out, the navigation writes a stderr line telling the user to open the already-printed URL by hand instead of swallowing the error silently. Co-Authored-By: Claude Opus 5.5 (1M context) Signed-off-by: cliffhall --- clients/cli/README.md | 2 +- .../__tests__/cli-oauth-navigation.test.ts | 27 ++++++++++++-- clients/cli/__tests__/open-url.test.ts | 34 +++++++++++++++++- clients/cli/src/cli-oauth-navigation.ts | 9 +++-- clients/cli/src/open-url.ts | 36 +++++++++++++++++-- 5 files changed, 98 insertions(+), 10 deletions(-) diff --git a/clients/cli/README.md b/clients/cli/README.md index 78d441f745..ce939a25ae 100644 --- a/clients/cli/README.md +++ b/clients/cli/README.md @@ -301,7 +301,7 @@ Register `http://127.0.0.1:6276/oauth/callback` on static or enterprise IdPs tha | `--client-secret ` | — | OAuth client secret; overrides `client.json`. | | `--client-metadata-url ` | — | CIMD metadata URL; overrides `client.json`. | | `--callback-url ` | `MCP_OAUTH_CALLBACK_URL` | Redirect URI sent to the authorization server (default: `http://127.0.0.1:6276/oauth/callback`). Must bind a **loopback** host (`localhost` / `127.0.0.0/8` / `[::1]`) — the listener receives the authorization code over plaintext `http`, so a non-loopback host hard-errors (no opt-in; use a port-forward if the browser is elsewhere). | -| — | `MCP_AUTO_OPEN_ENABLED` | Browser auto-open **and** non-TTY interactive-OAuth admit: `true` (allow interactive OAuth without a TTY **and** force-open the browser — same as the web launcher), `false` (never open), unset (open on a TTY unless `VITEST` is set). For CI that must not hang, prefer `--stored-auth-only`. | +| — | `MCP_AUTO_OPEN_ENABLED` | Browser auto-open **and** non-TTY interactive-OAuth admit: `true` (allow interactive OAuth without a TTY **and** force-open the browser — same as the web launcher), `false` (never open), unset (open on a TTY unless `VITEST` is set). The authorization URL is always printed first; an open that fails, or does not launch within 5s, adds a stderr line saying to open it by hand. For CI that must not hang, prefer `--stored-auth-only`. | **Example** — list tools on an OAuth-protected server using stored tokens and CIMD from the command line: diff --git a/clients/cli/__tests__/cli-oauth-navigation.test.ts b/clients/cli/__tests__/cli-oauth-navigation.test.ts index 9034915270..1932d5b625 100644 --- a/clients/cli/__tests__/cli-oauth-navigation.test.ts +++ b/clients/cli/__tests__/cli-oauth-navigation.test.ts @@ -202,7 +202,7 @@ describe("createCliOAuthNavigation", () => { expect(lines.join("")).not.toContain("\u001b]8;;"); }); - it("swallows browser-open failures after printing the URL", async () => { + it("tells the user to open the URL manually when the browser open fails", async () => { const lines: string[] = []; const openBrowser = vi.fn().mockRejectedValue(new Error("no browser")); const nav = createCliOAuthNavigation({ @@ -214,8 +214,29 @@ describe("createCliOAuthNavigation", () => { autoOpenEnabled: true, }); nav.navigateToAuthorization(new URL("https://as.example/a")); - await vi.waitFor(() => expect(openBrowser).toHaveBeenCalledOnce()); - expect(lines.join("")).toContain("Please navigate to:"); + await vi.waitFor(() => expect(lines).toHaveLength(2)); + expect(lines).toEqual([ + "Please navigate to: https://as.example/a\n", + "Could not open a browser automatically (no browser). Open the URL above manually.\n", + ]); + }); + + it("reports a non-Error browser-open rejection verbatim", async () => { + const lines: string[] = []; + const openBrowser = vi.fn().mockRejectedValue("spawn failed"); + const nav = createCliOAuthNavigation({ + isTTY: true, + noColorEnv: "1", + write: (line) => lines.push(line), + openBrowser, + autoOpenControl: { armed: true }, + autoOpenEnabled: true, + }); + nav.navigateToAuthorization(new URL("https://as.example/a")); + await vi.waitFor(() => expect(lines).toHaveLength(2)); + expect(lines[1]).toBe( + "Could not open a browser automatically (spawn failed). Open the URL above manually.\n", + ); }); it("writes to stderr and uses openUrl by default when armed on a TTY", async () => { diff --git a/clients/cli/__tests__/open-url.test.ts b/clients/cli/__tests__/open-url.test.ts index 269c5596b1..44ea8ce21a 100644 --- a/clients/cli/__tests__/open-url.test.ts +++ b/clients/cli/__tests__/open-url.test.ts @@ -1,4 +1,4 @@ -import { beforeEach, describe, expect, it, vi } from "vitest"; +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; const openMock = vi.fn().mockResolvedValue(undefined); @@ -15,6 +15,10 @@ describe("openUrl", () => { openMock.mockResolvedValue(undefined); }); + afterEach(() => { + vi.useRealTimers(); + }); + it("forwards a string URL to open", async () => { const { openUrl } = await import("../src/open-url.js"); await openUrl("https://example.com/auth"); @@ -28,4 +32,32 @@ describe("openUrl", () => { "https://example.com/callback?code=1", ); }); + + it("rejects when the opener rejects", async () => { + openMock.mockRejectedValue(new Error("xdg-open not found")); + const { openUrl } = await import("../src/open-url.js"); + await expect(openUrl("https://example.com/auth")).rejects.toThrow( + "xdg-open not found", + ); + }); + + it("rejects when the opener does not settle within the timeout", async () => { + vi.useFakeTimers(); + openMock.mockReturnValue(new Promise(() => {})); + const { openUrl } = await import("../src/open-url.js"); + const pending = openUrl("https://example.com/auth", 2_000); + const assertion = expect(pending).rejects.toThrow( + "browser did not open within 2s", + ); + await vi.advanceTimersByTimeAsync(2_000); + await assertion; + }); + + it("clears its timer once the opener resolves", async () => { + vi.useFakeTimers(); + const { openUrl, OPEN_URL_TIMEOUT_MS } = await import("../src/open-url.js"); + await openUrl("https://example.com/auth"); + expect(vi.getTimerCount()).toBe(0); + expect(OPEN_URL_TIMEOUT_MS).toBe(5_000); + }); }); diff --git a/clients/cli/src/cli-oauth-navigation.ts b/clients/cli/src/cli-oauth-navigation.ts index 1f5d1111c0..f805c08278 100644 --- a/clients/cli/src/cli-oauth-navigation.ts +++ b/clients/cli/src/cli-oauth-navigation.ts @@ -121,8 +121,13 @@ export function createCliOAuthNavigation( try { await (options.openBrowser ?? openUrl)(href); - } catch { - // URL already printed; browser open is best-effort. + } catch (error) { + // URL already printed; browser open is best-effort — but say so, or a + // failed open reads as the flow silently waiting on a browser (#2410). + const reason = error instanceof Error ? error.message : String(error); + write( + `Could not open a browser automatically (${reason}). Open the URL above manually.\n`, + ); } }); } diff --git a/clients/cli/src/open-url.ts b/clients/cli/src/open-url.ts index f5b42a8aca..d2d13a6dd5 100644 --- a/clients/cli/src/open-url.ts +++ b/clients/cli/src/open-url.ts @@ -1,6 +1,36 @@ import open from "open"; -/** Open a URL in the user's default browser (best-effort). */ -export async function openUrl(url: string | URL): Promise { - await open(typeof url === "string" ? url : url.href); +/** + * How long {@link openUrl} waits for the opener before giving up. `open` + * resolves once it has launched the platform opener, but before that it may + * probe the environment (WSL's default browser, the PowerShell path), and on a + * headless box or a container any of those can stall. The OAuth flow awaits + * this call, so an unbounded wait would hang it with the URL already printed + * and nothing telling the user to act (#2410). + */ +export const OPEN_URL_TIMEOUT_MS = 5_000; + +/** + * Open a URL in the user's default browser (best-effort). Rejects when the + * opener fails or does not launch within `timeoutMs`; callers print the URL + * themselves and decide what to tell the user. + */ +export async function openUrl( + url: string | URL, + timeoutMs: number = OPEN_URL_TIMEOUT_MS, +): Promise { + const href = typeof url === "string" ? url : url.href; + let timer: ReturnType | undefined; + const timeout = new Promise((_, reject) => { + timer = setTimeout( + () => + reject(new Error(`browser did not open within ${timeoutMs / 1000}s`)), + timeoutMs, + ); + }); + try { + await Promise.race([open(href), timeout]); + } finally { + clearTimeout(timer); + } } From f3e7c66908bb6bcdd56e179cab22fe34f5ac7364 Mon Sep 17 00:00:00 2001 From: cliffhall Date: Fri, 25 Sep 2026 23:26:06 -0400 Subject: [PATCH 2/2] docs(cli): state the real purpose of the openUrl timeout (#2499 review) Co-Authored-By: Claude Opus 5.5 (1M context) Signed-off-by: cliffhall --- clients/cli/src/open-url.ts | 7 ++++--- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/clients/cli/src/open-url.ts b/clients/cli/src/open-url.ts index d2d13a6dd5..920739da52 100644 --- a/clients/cli/src/open-url.ts +++ b/clients/cli/src/open-url.ts @@ -4,9 +4,10 @@ import open from "open"; * How long {@link openUrl} waits for the opener before giving up. `open` * resolves once it has launched the platform opener, but before that it may * probe the environment (WSL's default browser, the PowerShell path), and on a - * headless box or a container any of those can stall. The OAuth flow awaits - * this call, so an unbounded wait would hang it with the URL already printed - * and nothing telling the user to act (#2410). + * headless box or a container any of those can stall. The OAuth flow itself + * does not wait on this (`CallbackNavigation` fires its callback and discards + * the promise); the timeout exists so a stalled open settles as a failure and + * the caller's "open the URL manually" line is actually printed (#2410). */ export const OPEN_URL_TIMEOUT_MS = 5_000;