diff --git a/packages/ai/CHANGELOG.md b/packages/ai/CHANGELOG.md index 20ef357ef..ab2f19c99 100644 --- a/packages/ai/CHANGELOG.md +++ b/packages/ai/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Added + +- `OAuthCallbackFlow` now serves a `GET /launch` route on its loopback callback server that 302-redirects to the pending authorization URL, and exposes that short URL as `OAuthAuthInfo.launchUrl`. UIs can advertise it as a truncation-safe copy target (~30 chars) instead of the full authorize URL, so terminals narrower than the composed row cannot silently drop OAuth query parameters like `code_challenge_method=S256` ([#4418](https://github.com/can1357/oh-my-pi/issues/4418)). + ## [16.3.4] - 2026-07-03 ### Added diff --git a/packages/ai/src/auth-storage.ts b/packages/ai/src/auth-storage.ts index 5a835f2ba..fc71c9f70 100644 --- a/packages/ai/src/auth-storage.ts +++ b/packages/ai/src/auth-storage.ts @@ -16,7 +16,13 @@ import * as AIError from "./error"; import { isUsageLimitOutcome } from "./error/rate-limit"; import { getProviderDefinition, PASTE_CODE_LOGIN_PROVIDERS } from "./registry"; import { getOAuthApiKey, getOAuthProvider, refreshOAuthToken } from "./registry/oauth"; -import type { OAuthController, OAuthCredentials, OAuthProvider, OAuthProviderId } from "./registry/oauth/types"; +import type { + OAuthAuthInfo, + OAuthController, + OAuthCredentials, + OAuthProvider, + OAuthProviderId, +} from "./registry/oauth/types"; import { getEnvApiKey, getEnvApiKeyName } from "./stream"; import type { Provider } from "./types"; import type { @@ -1863,7 +1869,7 @@ export class AuthStorage { provider: OAuthProviderId, ctrl: OAuthController & { /** onAuth is required by auth-storage but optional in OAuthController */ - onAuth: (info: { url: string; instructions?: string }) => void; + onAuth: (info: OAuthAuthInfo) => void; /** onPrompt is required for some providers (github-copilot, openai-codex) */ onPrompt: (prompt: { message: string; placeholder?: string }) => Promise; }, diff --git a/packages/ai/src/registry/oauth/callback-server.ts b/packages/ai/src/registry/oauth/callback-server.ts index b0f084d7d..3f7cf6884 100644 --- a/packages/ai/src/registry/oauth/callback-server.ts +++ b/packages/ai/src/registry/oauth/callback-server.ts @@ -17,6 +17,14 @@ import type { OAuthController, OAuthCredentials } from "./types"; const DEFAULT_TIMEOUT = 300_000; const DEFAULT_HOSTNAME = "localhost"; const CALLBACK_PATH = "/callback"; +/** + * Path served by {@link OAuthCallbackFlow} that 302-redirects to the pending + * authorization URL. Kept out of {@link OAuthCallbackFlowOptions} because it + * lives on the loopback callback server alongside {@link CALLBACK_PATH} and + * must never clash with a provider-registered redirect URI (all known + * providers register `/callback`-shaped paths). + */ +const LAUNCH_PATH = "/launch"; export type CallbackResult = { code: string; state: string }; @@ -54,6 +62,14 @@ export abstract class OAuthCallbackFlow { allowPortFallback: boolean; #callbackResolve?: (result: CallbackResult) => void; #callbackReject?: (error: string) => void; + /** + * Authorization URL the `/launch` route currently redirects to. Set by + * {@link login} after {@link generateAuthUrl} and before {@link OAuthController.onAuth} + * fires, cleared when the server stops. `undefined` before the flow reaches + * that point and after it finishes, so `/launch` returns 503 rather than + * a stale URL. + */ + #pendingAuthUrl?: string; constructor( ctrl: OAuthController, @@ -120,7 +136,7 @@ export abstract class OAuthCallbackFlow { this.#throwIfCancelled(); // Start callback server first to get actual redirect URI - const { server, redirectUri } = await this.#startCallbackServer(state); + const { server, redirectUri, launchUrl } = await this.#startCallbackServer(state); try { this.#throwIfCancelled(); @@ -128,8 +144,14 @@ export abstract class OAuthCallbackFlow { const { url: authUrl, instructions } = await this.generateAuthUrl(state, redirectUri); this.#throwIfCancelled(); + // Publish the auth URL to the `/launch` route BEFORE handing it to + // callers. `onAuth` immediately renders a UI that advertises the + // launch URL as a copy target, so `/launch` must already resolve if + // the user clicks/pastes it during the same render pass. + this.#pendingAuthUrl = authUrl; + // Notify controller that auth is ready - this.ctrl.onAuth?.({ url: authUrl, instructions }); + this.ctrl.onAuth?.({ url: authUrl, launchUrl, instructions }); this.ctrl.onProgress?.("Waiting for browser authentication..."); // Wait for callback or manual input @@ -140,6 +162,7 @@ export abstract class OAuthCallbackFlow { return await this.exchangeToken(code, state, redirectUri); } finally { + this.#pendingAuthUrl = undefined; server.stop(); } } @@ -147,14 +170,21 @@ export abstract class OAuthCallbackFlow { /** * Start callback server, trying preferred port first, falling back to random. */ - async #startCallbackServer(expectedState: string): Promise<{ server: Bun.Server; redirectUri: string }> { + async #startCallbackServer( + expectedState: string, + ): Promise<{ server: Bun.Server; redirectUri: string; launchUrl: string }> { try { const server = this.#createServer(this.preferredPort, expectedState); + // `preferredPort: 0` opts into a random port — read the actual bound + // port from the server so both the redirect URI and launch URL point at + // a reachable socket, not the sentinel. + const actualPort = this.#resolveServerPort(server); + const launchUrl = this.#launchUrlFor(actualPort); if (this.redirectUri) { - return { server, redirectUri: this.redirectUri }; + return { server, redirectUri: this.redirectUri, launchUrl }; } - const redirectUri = `http://${this.callbackHostname}:${this.preferredPort}${this.callbackPath}`; - return { server, redirectUri }; + const redirectUri = `http://${this.callbackHostname}:${actualPort}${this.callbackPath}`; + return { server, redirectUri, launchUrl }; } catch (cause) { if (this.redirectUri) { throw new AIError.ConfigurationError( @@ -169,13 +199,39 @@ export abstract class OAuthCallbackFlow { ); } const server = this.#createServer(0, expectedState); - const actualPort = server.port; + const actualPort = this.#resolveServerPort(server); const redirectUri = `http://${this.callbackHostname}:${actualPort}${this.callbackPath}`; + const launchUrl = this.#launchUrlFor(actualPort); this.ctrl.onProgress?.(`Preferred port ${this.preferredPort} unavailable, using port ${actualPort}`); - return { server, redirectUri }; + return { server, redirectUri, launchUrl }; } } + /** + * Read the numeric port a callback server bound to. `Bun.Server.port` is + * declared `number | undefined` because Unix-socket servers have no port, + * but every callback flow uses TCP; a missing port here indicates a + * configuration error rather than a fallback case. + */ + #resolveServerPort(server: Bun.Server): number { + const port = server.port; + if (typeof port !== "number") { + throw new AIError.ConfigurationError( + "OAuth callback server bound to a non-TCP endpoint; expected a numeric port. Check `oauth.callbackPort`/`oauth.redirectUri`.", + ); + } + return port; + } + + /** + * Build the `/launch` URL served by the callback server bound to `port`. + * Kept short (~30 chars) so UIs can advertise it as a viewport-truncation-safe + * copy target for the full authorization URL. + */ + #launchUrlFor(port: number): string { + return `http://${this.callbackHostname}:${port}${LAUNCH_PATH}`; + } + /** * Create HTTP server for OAuth callback. */ @@ -190,11 +246,22 @@ export abstract class OAuthCallbackFlow { } /** - * Handle OAuth callback HTTP request. + * Handle OAuth callback HTTP request. Two routes on the same loopback server: + * - `callbackPath` (default `/callback`) — the provider redirect target. + * - `LAUNCH_PATH` (`/launch`) — 302 to the pending authorization URL so + * viewport-safe copy targets can survive TUI truncation. */ #handleCallback(req: Request, expectedState: string): Response { const url = new URL(req.url); + if (url.pathname === LAUNCH_PATH) { + const pending = this.#pendingAuthUrl; + if (!pending) { + return new Response("OAuth launch URL is no longer active", { status: 503 }); + } + return Response.redirect(pending, 302); + } + if (url.pathname !== this.callbackPath) { return new Response("Not Found", { status: 404 }); } diff --git a/packages/ai/src/registry/oauth/types.ts b/packages/ai/src/registry/oauth/types.ts index 20bc43219..a11b4e5fd 100644 --- a/packages/ai/src/registry/oauth/types.ts +++ b/packages/ai/src/registry/oauth/types.ts @@ -23,7 +23,21 @@ export type OAuthPrompt = { }; export type OAuthAuthInfo = { + /** + * Full authorization URL. Suitable for direct browser launch, OSC 8 + * hyperlinks, and clipboard when the target UI can guarantee the full + * string reaches the user unmodified. + */ url: string; + /** + * Short loopback URL that 302-redirects to {@link url}. Provided by flows + * that host the redirect on the same callback server they already run + * ({@link OAuthCallbackFlow}). UIs SHOULD prefer this as the copy target + * so viewport truncation cannot corrupt OAuth query parameters. Undefined + * for flows without a loopback callback server (device code, paste-code + * providers with fixed non-loopback redirects, etc.). + */ + launchUrl?: string; instructions?: string; }; diff --git a/packages/ai/test/callback-server-launch-route.test.ts b/packages/ai/test/callback-server-launch-route.test.ts new file mode 100644 index 000000000..9cc040a4d --- /dev/null +++ b/packages/ai/test/callback-server-launch-route.test.ts @@ -0,0 +1,116 @@ +import { afterEach, describe, expect, it, vi } from "bun:test"; +import { OAuthCallbackFlow } from "@oh-my-pi/pi-ai/registry/oauth/callback-server"; +import type { OAuthAuthInfo, OAuthCredentials } from "@oh-my-pi/pi-ai/registry/oauth/types"; + +/** + * Regression harness for #4418 — the `/launch` route the callback server hosts + * so UIs can advertise a short (~30-char) copy target that survives TUI viewport + * truncation. Without it, the full authorize URL (~260+ chars on Linear/GitHub/…) + * gets silently truncated mid-parameter and downgrades the flow to plain PKCE. + */ +class LaunchProbeFlow extends OAuthCallbackFlow { + authUrls: string[] = []; + // Long enough that a 270-col TUI would clip `code_challenge_method=S256`. + static readonly PADDING = "x".repeat(200); + + async generateAuthUrl(state: string, redirectUri: string): Promise<{ url: string }> { + const url = + "https://mcp.example.com/authorize?" + + new URLSearchParams({ + response_type: "code", + client_id: "test-client", + redirect_uri: redirectUri, + state, + scope: LaunchProbeFlow.PADDING, + code_challenge: "test-challenge", + code_challenge_method: "S256", + }).toString(); + this.authUrls.push(url); + return { url }; + } + + async exchangeToken(): Promise { + return { access: "unused", refresh: "unused", expires: Date.now() + 60_000 }; + } +} + +/** + * Start a flow and resolve once `onAuth` fires — that's the exact instant + * `/launch` becomes live, so tests can hit it without a wall-clock sleep. + * Returns the captured auth info, the abort controller (so tests can shut the + * flow down), and the pending `login` promise (so tests can await teardown). + */ +async function startFlowAndWaitForAuth(): Promise<{ + info: OAuthAuthInfo; + abort: AbortController; + login: Promise; +}> { + const abort = new AbortController(); + const authFired = Promise.withResolvers(); + const flow = new LaunchProbeFlow( + { + onAuth: info => { + authFired.resolve(info); + }, + signal: abort.signal, + }, + { preferredPort: 0, allowPortFallback: true }, + ); + // Kick off login in the background; tests own its lifetime via `abort`. + const login = flow.login().catch(() => undefined) as Promise; + const info = await authFired.promise; + return { info, abort, login }; +} + +afterEach(() => { + vi.restoreAllMocks(); +}); + +describe("OAuthCallbackFlow /launch route", () => { + it("advertises a short launch URL and 302s it to the pending authorization URL", async () => { + const { info, abort, login } = await startFlowAndWaitForAuth(); + + // Contract 1 — launch URL is short and shaped like a loopback URL. A + // terminal that truncates below ~40 columns is degenerate; anything above + // that keeps the launch URL intact regardless of the full URL length. + expect(info.launchUrl).toBeDefined(); + expect(info.launchUrl!.length).toBeLessThan(40); + expect(info.launchUrl).toMatch(/^http:\/\/localhost:\d+\/launch$/); + + // Contract 2 — GET /launch returns 302 pointing at the pending authorize URL, + // byte-for-byte (the whole point: no truncation surface between UI and provider). + const response = await fetch(info.launchUrl!, { redirect: "manual" }); + expect(response.status).toBe(302); + expect(response.headers.get("location")).toBe(info.url); + + abort.abort("test done"); + await login; + }); + + it("stops answering /launch once the flow completes so no stale URL is redirected", async () => { + const { info, abort, login } = await startFlowAndWaitForAuth(); + expect(info.launchUrl).toBeDefined(); + + abort.abort("test done"); + await login; + + // Server has stopped and `#pendingAuthUrl` was cleared — the launch URL + // no longer connects. The correct end-state is that the redirect NEVER + // points at a stale URL; the loopback socket is gone so `fetch` rejects. + await expect(fetch(info.launchUrl!)).rejects.toThrow(); + }); + + it("routes `/callback` and `/launch` on the same server without interfering", async () => { + const { info, abort, login } = await startFlowAndWaitForAuth(); + expect(info.launchUrl).toBeDefined(); + + // A GET at an unrelated path still 404s — `/launch` is additive, not a + // blanket catch-all. + const origin = new URL(info.launchUrl!).origin; + const stray = await fetch(`${origin}/nope`); + expect(stray.status).toBe(404); + + abort.abort("test done"); + await login; + }); +}); diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 860266c53..39812cf13 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -2,6 +2,10 @@ ## [Unreleased] +### Fixed + +- Fixed `/mcp reauth` against S256-only OAuth providers (Linear, and OAuth 2.1 flows generally) failing on Windows and other environments where the browser cannot auto-open. Two independent defects converged on Linear's `The plain PKCE method is not allowed. Use S256 instead.` error page: `openPath` invoked bare `rundll32` and silently swallowed the resulting `Executable not found in $PATH` when the Windows machine PATH no longer referenced `System32`, and `MCPAuthorizationLinkPrompt` composed a `Copy URL:` line wider than the viewport that `TUI#prepareLine` then silently truncated — trimming the trailing `code_challenge_method=S256` (RFC 7636 §4.3 treats a request with `code_challenge` and no method as `plain`). The MCP OAuth fallback, `/login`, setup wizard, auth-broker CLI, and login-dialog now advertise the short `OAuthAuthInfo.launchUrl` (loopback `/launch` route hosted by `OAuthCallbackFlow`) as the copy target; the OSC 8 hyperlink still carries the full URL for click-through; the MCP flow additionally stages the copy target on the clipboard via OSC 52; and the Windows opener now resolves `rundll32.exe` through `%SystemRoot%\System32\`, logging spawn failures and non-zero exits instead of dropping them ([#4418](https://github.com/can1357/oh-my-pi/issues/4418)). + ## [16.3.4] - 2026-07-03 ### Fixed diff --git a/packages/coding-agent/src/cli/auth-broker-cli.ts b/packages/coding-agent/src/cli/auth-broker-cli.ts index 2d04ee61f..0f504f6f1 100644 --- a/packages/coding-agent/src/cli/auth-broker-cli.ts +++ b/packages/coding-agent/src/cli/auth-broker-cli.ts @@ -222,8 +222,17 @@ async function runLocalLogin(provider: OAuthProvider): Promise { // for non-paste-code providers, so this is defense-in-depth on the same gate. const usesManualInput = PASTE_CODE_LOGIN_PROVIDERS.has(provider); await storage.login(provider, { - onAuth({ url, instructions }) { - process.stdout.write(`\nOpen this URL in your browser:\n${url}\n`); + onAuth({ url, launchUrl, instructions }) { + process.stdout.write("\nOpen this URL in your browser:\n"); + // Advertise the short launch URL first when the flow exposes one — it + // survives narrow terminals that would truncate the trailing + // `code_challenge_method=S256` (or worse, drop `state`/`code_challenge` + // entirely) from the full authorize URL. The full URL still prints + // beneath it so headless callers can capture it programmatically. + if (launchUrl && launchUrl !== url) { + process.stdout.write(`${launchUrl}\n(redirects to)\n`); + } + process.stdout.write(`${url}\n`); if (instructions) process.stdout.write(`${instructions}\n`); process.stdout.write("\n"); }, diff --git a/packages/coding-agent/src/modes/components/login-dialog.ts b/packages/coding-agent/src/modes/components/login-dialog.ts index 628cb8691..27c4ed6a5 100644 --- a/packages/coding-agent/src/modes/components/login-dialog.ts +++ b/packages/coding-agent/src/modes/components/login-dialog.ts @@ -68,12 +68,20 @@ export class LoginDialogComponent extends Container { } /** - * Called by onAuth callback - show URL and optional instructions + * Called by the OAuth `onAuth` callback. Renders the copy target on its own + * row and attaches an OSC 8 hyperlink carrying the full URL so terminals + * that support it can click-through even when the copy target is truncated. + * + * `launchUrl` (when present) is a short loopback URL that 302s to `url`. + * Preferred as the copy target because viewport truncation on a long + * authorize URL silently drops trailing OAuth query parameters — e.g. + * `code_challenge_method=S256`. */ - showAuth(url: string, instructions?: string): void { + showAuth(url: string, instructions?: string, launchUrl?: string): void { + const copyTarget = launchUrl ?? url; this.#contentContainer.clear(); this.#contentContainer.addChild(new Spacer(1)); - this.#contentContainer.addChild(new Text(theme.fg("accent", url), 1, 0)); + this.#contentContainer.addChild(new Text(theme.fg("accent", copyTarget), 1, 0)); const clickHint = process.platform === "darwin" ? "Cmd+click to open" : "Ctrl+click to open"; const hyperlink = `\x1b]8;;${url}\x07${clickHint}\x1b]8;;\x07`; diff --git a/packages/coding-agent/src/modes/controllers/mcp-command-controller.ts b/packages/coding-agent/src/modes/controllers/mcp-command-controller.ts index 8e0e0aaec..a72507ba6 100644 --- a/packages/coding-agent/src/modes/controllers/mcp-command-controller.ts +++ b/packages/coding-agent/src/modes/controllers/mcp-command-controller.ts @@ -43,6 +43,7 @@ import { import type { MCPAuthConfig, MCPServerConfig, MCPServerConnection } from "../../mcp/types"; import { shortenPath } from "../../tools/render-utils"; import { urlHyperlinkAlways } from "../../tui"; +import { copyToClipboard } from "../../utils/clipboard"; import { openPath } from "../../utils/open"; import { ChatBlock } from "../components/chat-block"; import { MCPAddWizard } from "../components/mcp-add-wizard"; @@ -73,22 +74,33 @@ function raceAbortSignal(promise: Promise, signal: AbortSignal, createErro }); } -/** Renders the MCP OAuth fallback URL without hard-wrapping the copy target. */ +/** + * Renders the MCP OAuth fallback URL without hard-wrapping the copy target. + * + * When the flow's callback server hosts a `/launch` short URL (`launchUrl`), + * that is advertised as the copy target instead of the full authorization + * URL: a Linear-shaped authorize URL routinely exceeds 260 columns, and the + * TUI silently truncates any composed row wider than the viewport — dropping + * trailing OAuth parameters like `code_challenge_method=S256`. The OSC 8 + * hyperlink still carries the full URL for terminals that support it. + */ export class MCPAuthorizationLinkPrompt implements Component { - readonly #url: string; + readonly #fullUrl: string; + readonly #copyTarget: string; - constructor(url: string) { - this.#url = url; + constructor(url: string, launchUrl?: string) { + this.#fullUrl = url; + this.#copyTarget = launchUrl ?? url; } invalidate(): void {} render(_width: number): readonly string[] { - const link = urlHyperlinkAlways(this.#url, "Click here to authorize"); + const link = urlHyperlinkAlways(this.#fullUrl, "Click here to authorize"); return [ ` ${theme.fg("success", "Open authorization URL:")}`, ` ${theme.fg("accent", link)}`, - ` ${theme.fg("muted", `Copy URL: ${replaceTabs(this.#url)}`)}`, + ` ${theme.fg("muted", `Copy URL: ${replaceTabs(this.#copyTarget)}`)}`, ]; } } @@ -689,7 +701,7 @@ export class MCPCommandController { stripSameOriginResource: opts?.stripSameOriginResource, }, { - onAuth: (info: { url: string; instructions?: string }) => { + onAuth: (info: { url: string; launchUrl?: string; instructions?: string }) => { // Show auth URL prominently in chat as one block const block = new TranscriptBlock(); this.ctx.present(block); @@ -707,24 +719,23 @@ export class MCPCommandController { block.addChild(new Text(theme.fg("muted", MCP_MANUAL_LOGIN_TIP), 1, 0)); block.addChild(new Spacer(1)); block.addChild(new Text(theme.fg("accent", "━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━━"), 1, 0)); - // Try to open browser automatically - try { - openPath(info.url); - - // Show confirmation that browser should open - block.addChild(new Spacer(1)); - block.addChild(new Text(theme.fg("success", "→ Opening browser automatically..."), 1, 0)); - block.addChild(new Spacer(1)); - block.addChild(new Text(theme.fg("muted", "Alternative if browser did not open:"), 1, 0)); - block.addChild(new MCPAuthorizationLinkPrompt(info.url)); - this.ctx.ui.requestRender(); - } catch (_error) { - // Show error if browser doesn't open - block.addChild(new Spacer(1)); - block.addChild(new Text(theme.fg("warning", "→ Could not open browser automatically"), 1, 0)); - block.addChild(new MCPAuthorizationLinkPrompt(info.url)); - this.ctx.ui.requestRender(); - } + // `openPath` is best-effort — it logs spawn failures but never + // throws, so we always render the copy-URL fallback beneath the + // "attempting to open browser" line and no earlier try/catch is + // worth keeping. + openPath(info.url); + const copyTarget = info.launchUrl ?? info.url; + // Stage the copy target on the clipboard via OSC 52 (same + // pattern the setup wizard uses). Best-effort: falls back to + // the visible "Copy URL:" line whether or not the terminal + // honors OSC 52. + void copyToClipboard(copyTarget).catch(() => {}); + block.addChild(new Spacer(1)); + block.addChild(new Text(theme.fg("success", "→ Attempting to open browser..."), 1, 0)); + block.addChild(new Spacer(1)); + block.addChild(new Text(theme.fg("muted", "Alternative if browser did not open:"), 1, 0)); + block.addChild(new MCPAuthorizationLinkPrompt(info.url, info.launchUrl)); + this.ctx.ui.requestRender(); }, onProgress: (message: string) => { this.ctx.present([new Spacer(1), new Text(theme.fg("muted", message), 1, 0)]); diff --git a/packages/coding-agent/src/modes/controllers/selector-controller.ts b/packages/coding-agent/src/modes/controllers/selector-controller.ts index ec061edae..dac8f48bd 100644 --- a/packages/coding-agent/src/modes/controllers/selector-controller.ts +++ b/packages/coding-agent/src/modes/controllers/selector-controller.ts @@ -1117,9 +1117,10 @@ export class SelectorController { const useManualInput = PASTE_CODE_LOGIN_PROVIDERS.has(providerId); try { await this.ctx.session.modelRegistry.authStorage.login(providerId as OAuthProvider, { - onAuth: (info: { url: string; instructions?: string }) => { + onAuth: (info: { url: string; launchUrl?: string; instructions?: string }) => { + const copyTarget = info.launchUrl ?? info.url; const block = new TranscriptBlock(); - block.addChild(new Text(theme.fg("dim", info.url), 1, 0)); + block.addChild(new Text(theme.fg("dim", copyTarget), 1, 0)); const hyperlink = `\x1b]8;;${info.url}\x07Click here to login\x1b]8;;\x07`; block.addChild(new Text(theme.fg("accent", hyperlink), 1, 0)); if (info.instructions) { diff --git a/packages/coding-agent/src/modes/rpc/rpc-mode.ts b/packages/coding-agent/src/modes/rpc/rpc-mode.ts index 5c4cabd89..ac2a59ac1 100644 --- a/packages/coding-agent/src/modes/rpc/rpc-mode.ts +++ b/packages/coding-agent/src/modes/rpc/rpc-mode.ts @@ -1213,6 +1213,7 @@ export async function runRpcMode( id: Snowflake.next() as string, method: "open_url", url: info.url, + launchUrl: info.launchUrl, instructions: info.instructions, } as RpcExtensionUIRequest); }, diff --git a/packages/coding-agent/src/modes/rpc/rpc-types.ts b/packages/coding-agent/src/modes/rpc/rpc-types.ts index 10863bc79..a877079cd 100644 --- a/packages/coding-agent/src/modes/rpc/rpc-types.ts +++ b/packages/coding-agent/src/modes/rpc/rpc-types.ts @@ -370,7 +370,19 @@ export type RpcExtensionUIRequest = } | { type: "extension_ui_request"; id: string; method: "setTitle"; title: string } | { type: "extension_ui_request"; id: string; method: "set_editor_text"; text: string } - | { type: "extension_ui_request"; id: string; method: "open_url"; url: string; instructions?: string }; + | { + type: "extension_ui_request"; + id: string; + method: "open_url"; + url: string; + /** + * Short loopback URL that 302-redirects to {@link url}. When present, + * hosts SHOULD surface it as the copy target so terminal viewport + * truncation cannot corrupt OAuth query parameters on the full URL. + */ + launchUrl?: string; + instructions?: string; + }; // ============================================================================ // Host Tool Frames (bidirectional) diff --git a/packages/coding-agent/src/modes/setup-wizard/scenes/sign-in.ts b/packages/coding-agent/src/modes/setup-wizard/scenes/sign-in.ts index c0896c4da..7735e39ce 100644 --- a/packages/coding-agent/src/modes/setup-wizard/scenes/sign-in.ts +++ b/packages/coding-agent/src/modes/setup-wizard/scenes/sign-in.ts @@ -189,7 +189,11 @@ export class SignInTab implements SetupTab { await this.#authStorage.login(providerId as OAuthProvider, { signal: this.#loginAbort.signal, onAuth: info => { - this.#authUrl = info.url; + // Store the short launch URL when available; the setup wizard + // renders `#authUrl` as a plain text row (no OSC 8 hyperlink), so + // the copy/display target must survive viewport truncation. The + // browser is still launched against the full URL. + this.#authUrl = info.launchUrl ?? info.url; this.#statusLines = []; if (info.instructions) { this.#statusLines.push(theme.fg("warning", info.instructions)); diff --git a/packages/coding-agent/src/utils/open.ts b/packages/coding-agent/src/utils/open.ts index db4090e5d..ca3390438 100644 --- a/packages/coding-agent/src/utils/open.ts +++ b/packages/coding-agent/src/utils/open.ts @@ -1,7 +1,7 @@ import * as fs from "node:fs"; import * as path from "node:path"; import * as url from "node:url"; -import * as piUtils from "@oh-my-pi/pi-utils"; +import { $which, logger } from "@oh-my-pi/pi-utils"; const URL_SCHEME_PATTERN = /^[a-zA-Z][a-zA-Z\d+.-]*:/; @@ -9,7 +9,7 @@ function getExistingWslLocalPath(urlOrPath: string): string | undefined { if ( process.platform !== "linux" || !(process.env.WSL_DISTRO_NAME || process.env.WSL_INTEROP) || - !piUtils.$which("wslview") + !$which("wslview") ) { return undefined; } @@ -31,6 +31,24 @@ function getExistingWslLocalPath(urlOrPath: string): string | undefined { } } +/** + * Resolve the Windows `rundll32.exe` command used to hand a URL/path to the + * user's registered protocol handler. Anchoring to `%SystemRoot%\System32` + * (rather than relying on `rundll32` being on `PATH`) survives environments + * where the machine `PATH` no longer references `System32` — a common + * real-world misconfiguration where `System32\Wbem` / `WindowsPowerShell` / + * `OpenSSH` survive but `System32` itself is dropped. Bare `rundll32` on + * such boxes throws `Executable not found in $PATH: "rundll32"` from + * `Bun.spawn` before ShellExecute ever sees the URL. + */ +function windowsOpenerCommand(target: string): string[] { + const systemRoot = process.env.SystemRoot?.trim() || process.env.SYSTEMROOT?.trim() || "C:\\Windows"; + // `path.win32` (not the platform-adaptive `path.join`) keeps Windows path + // separators when tests run under a POSIX host and matches Windows call + // conventions on the real target. + const rundll32 = path.win32.join(systemRoot, "System32", "rundll32.exe"); + return [rundll32, "url.dll,FileProtocolHandler", target]; +} /** Open a URL or file path in the default browser/application. Best-effort, never throws. */ export function openPath(urlOrPath: string): void { let cmd: string[]; @@ -39,7 +57,7 @@ export function openPath(urlOrPath: string): void { cmd = ["open", urlOrPath]; break; case "win32": - cmd = ["rundll32", "url.dll,FileProtocolHandler", urlOrPath]; + cmd = windowsOpenerCommand(urlOrPath); break; default: { const wslPath = getExistingWslLocalPath(urlOrPath); @@ -47,9 +65,36 @@ export function openPath(urlOrPath: string): void { break; } } + let child: Bun.Subprocess | undefined; try { - Bun.spawn(cmd, { stdin: "ignore", stdout: "ignore", stderr: "ignore" }); - } catch { - // Best-effort: browser opening is non-critical + child = Bun.spawn(cmd, { stdin: "ignore", stdout: "ignore", stderr: "ignore" }); + } catch (error) { + // Spawn threw synchronously (missing binary, denied exec, sandbox + // restriction, …). Best-effort: log so the failure isn't invisible while + // still letting the caller advertise a copy-URL fallback. + logger.warn("Failed to open external URL/path", { + command: cmd[0], + target: urlOrPath, + error: error instanceof Error ? error.message : String(error), + }); + return; } + // Detect delayed failures (exec succeeded but the opener exited non-zero) + // without blocking the caller. Recording them makes silent misconfigurations + // (e.g. `xdg-open` present but no MIME handler for `https`) diagnosable from + // `~/.omp/logs/omp.*.log`. + child.exited.then( + exitCode => { + if (typeof exitCode === "number" && exitCode !== 0) { + logger.warn("External opener exited with non-zero status", { + command: cmd[0], + target: urlOrPath, + exitCode, + }); + } + }, + () => { + // Ignore — awaiting the subprocess is best-effort telemetry. + }, + ); } diff --git a/packages/coding-agent/test/modes/controllers/mcp-authorization-link.test.ts b/packages/coding-agent/test/modes/controllers/mcp-authorization-link.test.ts index 666ce0b47..6e77e95de 100644 --- a/packages/coding-agent/test/modes/controllers/mcp-authorization-link.test.ts +++ b/packages/coding-agent/test/modes/controllers/mcp-authorization-link.test.ts @@ -37,4 +37,22 @@ describe("MCPAuthorizationLinkPrompt", () => { expect(plainLines[1]).toContain("Click here to authorize"); expect(plainLines[2]).toBe(` Copy URL: ${LONG_AUTH_URL}`); }); + + it("advertises the launch URL as the copy target while keeping OSC 8 pointing at the full URL", () => { + const launchUrl = "http://localhost:14570/launch"; + const lines = new MCPAuthorizationLinkPrompt(LONG_AUTH_URL, launchUrl).render(80); + const plainLines = lines.map(line => stripVTControlCharacters(line)); + + expect(lines).toHaveLength(3); + // OSC 8 hyperlink still carries the full URL — click-through targets + // the provider directly on terminals that support the escape. + expect(extractLinkUri(lines[1])).toBe(LONG_AUTH_URL); + expect(plainLines[1]).toContain("Click here to authorize"); + // Copy target is the short loopback URL. Terminals that don't render + // OSC 8, and every copy-paste operation, hit this line — and it must + // survive viewport truncation without dropping OAuth parameters like + // `code_challenge_method=S256`. + expect(plainLines[2]).toBe(` Copy URL: ${launchUrl}`); + expect(plainLines[2].length).toBeLessThan(50); + }); }); diff --git a/packages/coding-agent/test/utils/open.test.ts b/packages/coding-agent/test/utils/open.test.ts index 6495da1dc..467c28f4e 100644 --- a/packages/coding-agent/test/utils/open.test.ts +++ b/packages/coding-agent/test/utils/open.test.ts @@ -132,4 +132,51 @@ describe("openPath", () => { expect(spawnSyncSpy).not.toHaveBeenCalled(); expect(spawnCalls.map(call => call.cmd)).toEqual([["xdg-open", existingLinuxPath]]); }); + + it("resolves rundll32 through %SystemRoot% so a broken machine PATH cannot silence the opener", () => { + setPlatform("win32"); + const originalSystemRoot = process.env.SystemRoot; + process.env.SystemRoot = "D:\\CustomWindows"; + try { + const spawnCalls: SpawnCall[] = []; + spySpawn(spawnCalls); + + openPath("https://mcp.linear.app/authorize?state=xyz&code_challenge_method=S256"); + + expect(spawnCalls).toHaveLength(1); + const [call] = spawnCalls; + // Absolute rundll32 path — bare `rundll32` was the whole bug on Windows + // boxes where the machine PATH no longer references System32. + expect(call?.cmd[0]).toBe("D:\\CustomWindows\\System32\\rundll32.exe"); + // Handler + URL forwarded verbatim as a single argv slot so `&` in the + // query string cannot be interpreted as a shell separator. + expect(call?.cmd.slice(1)).toEqual([ + "url.dll,FileProtocolHandler", + "https://mcp.linear.app/authorize?state=xyz&code_challenge_method=S256", + ]); + } finally { + if (originalSystemRoot === undefined) delete process.env.SystemRoot; + else process.env.SystemRoot = originalSystemRoot; + } + }); + + it("falls back to C:\\Windows for rundll32 when SystemRoot is unset", () => { + setPlatform("win32"); + const originalSystemRoot = process.env.SystemRoot; + const originalSystemRootLower = process.env.SYSTEMROOT; + delete process.env.SystemRoot; + delete process.env.SYSTEMROOT; + try { + const spawnCalls: SpawnCall[] = []; + spySpawn(spawnCalls); + + openPath("https://example.com"); + + expect(spawnCalls).toHaveLength(1); + expect(spawnCalls[0]?.cmd[0]).toBe("C:\\Windows\\System32\\rundll32.exe"); + } finally { + if (originalSystemRoot !== undefined) process.env.SystemRoot = originalSystemRoot; + if (originalSystemRootLower !== undefined) process.env.SYSTEMROOT = originalSystemRootLower; + } + }); });