fix(mcp): surface a short launch URL and log Windows opener failures for OAuth

Two independent defects broke /mcp reauth against S256-only providers on
Windows boxes whose PATH no longer references System32:

1. openPath spawned bare rundll32 and swallowed the
   `Executable not found in $PATH` throw with a bare `catch {}`, so the MCP
   controller's outer try/catch was dead and the transcript unconditionally
   claimed "Opening browser automatically...".
2. TUI#prepareLine silently truncates any composed row wider than the
   viewport. MCPAuthorizationLinkPrompt rendered `Copy URL: <full URL>` as a
   single ~271-column line whose trailing parameter is
   code_challenge_method=S256. On the reporter's 270-col terminal the cut
   landed inside that parameter, dropping the method while keeping
   code_challenge — which RFC 7636 §4.3 treats as plain PKCE, which Linear
   correctly rejects with "The plain PKCE method is not allowed. Use S256
   instead."

OAuthCallbackFlow now hosts a `GET /launch` route on the same loopback
callback server it already runs; the route 302-redirects to the pending
authorization URL and is advertised as `OAuthAuthInfo.launchUrl` — a
~30-char copy target no viewport can meaningfully truncate. The MCP OAuth
fallback, /login, setup wizard, auth-broker CLI, and login-dialog all
prefer the launch URL for the visible copy target, keep the full URL in
the OSC 8 hyperlink for click-through, and the MCP flow additionally
stages the copy target on the clipboard via OSC 52 (same pattern the
setup wizard uses).

openPath now resolves rundll32.exe through %SystemRoot%\System32 (with a
C:\Windows fallback when SystemRoot is unset) and logs both synchronous
spawn throws and non-zero exits via the shared logger, so silent
misconfigurations show up in ~/.omp/logs/omp.*.log. The dead try/catch
around openPath in the MCP controller is removed.

Fixes #4418
This commit is contained in:
roboomp
2026-07-03 08:19:14 +00:00
parent d0c1890a6c
commit 97c1d08cce
16 changed files with 418 additions and 51 deletions
+4
View File
@@ -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
+8 -2
View File
@@ -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<string>;
},
@@ -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<unknown>; redirectUri: string }> {
async #startCallbackServer(
expectedState: string,
): Promise<{ server: Bun.Server<unknown>; 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<unknown>): 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 });
}
+14
View File
@@ -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;
};
@@ -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<OAuthCredentials> {
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<void>;
}> {
const abort = new AbortController();
const authFired = Promise.withResolvers<OAuthAuthInfo>();
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<void>;
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;
});
});
+4
View File
@@ -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
@@ -222,8 +222,17 @@ async function runLocalLogin(provider: OAuthProvider): Promise<void> {
// 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");
},
@@ -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`;
@@ -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<T>(promise: Promise<T>, 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)]);
@@ -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) {
@@ -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);
},
@@ -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)
@@ -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));
+51 -6
View File
@@ -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.
},
);
}
@@ -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);
});
});
@@ -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;
}
});
});