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..920739da52 100644 --- a/clients/cli/src/open-url.ts +++ b/clients/cli/src/open-url.ts @@ -1,6 +1,37 @@ 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 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; + +/** + * 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); + } }