diff --git a/packages/ai/CHANGELOG.md b/packages/ai/CHANGELOG.md index cc98662b5..34c846ef1 100644 --- a/packages/ai/CHANGELOG.md +++ b/packages/ai/CHANGELOG.md @@ -2,6 +2,14 @@ ## [Unreleased] +### Added + +- Added `OAuthCallbackFlowOptions.allowPortFallback` (default `true`) so subclasses that gate random-port fallback per request — e.g. MCP flows whose static `client_id` is already registered against a specific redirect URI — can refuse the silent fallback and surface an actionable `ConfigurationError` before opening the browser, while flows that re-register on demand (dynamic client registration) can keep the existing fallback semantics. + +### Changed + +- Reworded the `OAuthCallbackFlow` strict-port `ConfigurationError` messages to name the busy port, the configured `oauth.redirectUri` (when set), and concrete remediation steps (free the port, change `oauth.callbackPort`/`oauth.redirectUri`). Existing `redirectUri`-strict callers see the new wording on the same code path. + ## [16.2.7] - 2026-06-30 ### Added diff --git a/packages/ai/src/registry/oauth/callback-server.ts b/packages/ai/src/registry/oauth/callback-server.ts index 4d829412e..e6fb88fa2 100644 --- a/packages/ai/src/registry/oauth/callback-server.ts +++ b/packages/ai/src/registry/oauth/callback-server.ts @@ -26,6 +26,20 @@ export interface OAuthCallbackFlowOptions { callbackHostname?: string; /** Exact redirect URI advertised to the provider; disables port fallback. */ redirectUri?: string; + /** + * Whether the flow may bind to a random port when {@link preferredPort} is + * unavailable. Defaults to `true` so historical AI-provider flows (which + * pick uncommon ports and tolerate any loopback callback) keep working. + * + * Set to `false` for providers that validate the redirect URI against a + * registered callback — silently advertising a random-port URI would be + * rejected by the authorization server, leaving the browser on an opaque + * 500 page and the local callback waiting until the 5-minute timeout fires. + * With fallback disabled, {@link OAuthCallbackFlow.login} throws a + * {@link AIError.ConfigurationError} immediately so the caller can surface + * an actionable message before opening the browser. + */ + allowPortFallback?: boolean; } /** @@ -37,6 +51,7 @@ export abstract class OAuthCallbackFlow { callbackPath: string; callbackHostname: string; redirectUri?: string; + allowPortFallback: boolean; #callbackResolve?: (result: CallbackResult) => void; #callbackReject?: (error: string) => void; @@ -50,6 +65,7 @@ export abstract class OAuthCallbackFlow { this.preferredPort = preferredPortOrOptions; this.callbackPath = callbackPath; this.callbackHostname = DEFAULT_HOSTNAME; + this.allowPortFallback = true; return; } @@ -57,6 +73,7 @@ export abstract class OAuthCallbackFlow { this.callbackPath = preferredPortOrOptions.callbackPath ?? CALLBACK_PATH; this.callbackHostname = preferredPortOrOptions.callbackHostname ?? DEFAULT_HOSTNAME; this.redirectUri = preferredPortOrOptions.redirectUri; + this.allowPortFallback = preferredPortOrOptions.allowPortFallback ?? true; } /** @@ -126,10 +143,17 @@ export abstract class OAuthCallbackFlow { } const redirectUri = `http://${this.callbackHostname}:${this.preferredPort}${this.callbackPath}`; return { server, redirectUri }; - } catch { + } catch (cause) { if (this.redirectUri) { throw new AIError.ConfigurationError( - `OAuth callback port ${this.preferredPort} unavailable; cannot fall back to a random port when oauth.redirectUri is set`, + `OAuth callback port ${this.preferredPort} is in use, but oauth.redirectUri (${this.redirectUri}) requires this exact port. Free port ${this.preferredPort} (e.g. stop the process bound to it) and retry, or change oauth.redirectUri to point at an available port.`, + { cause }, + ); + } + if (!this.allowPortFallback) { + throw new AIError.ConfigurationError( + `OAuth callback port ${this.preferredPort} is in use. The OAuth provider validates redirect URIs against its registered callback, so falling back to a random port would be rejected. Free port ${this.preferredPort} (e.g. stop the process bound to it) and retry, or set oauth.callbackPort/oauth.redirectUri to a port the provider has registered.`, + { cause }, ); } const server = this.#createServer(0, expectedState); diff --git a/packages/ai/test/callback-server-port-fallback.test.ts b/packages/ai/test/callback-server-port-fallback.test.ts new file mode 100644 index 000000000..65bdd5282 --- /dev/null +++ b/packages/ai/test/callback-server-port-fallback.test.ts @@ -0,0 +1,123 @@ +import { afterEach, describe, expect, it, vi } from "bun:test"; +import { ConfigurationError } from "@oh-my-pi/pi-ai/error"; +import { OAuthCallbackFlow } from "@oh-my-pi/pi-ai/registry/oauth/callback-server"; +import type { OAuthCredentials } from "@oh-my-pi/pi-ai/registry/oauth/types"; + +/** + * Minimal callback flow we can drive without a real authorization server. + * `generateAuthUrl` is never expected to run in the strict-port tests — + * `#startCallbackServer` must throw before `login()` can reach it — so a stray + * invocation surfaces as a counter bump the test asserts on. + */ +class TestCallbackFlow extends OAuthCallbackFlow { + authUrlCalls = 0; + lastRedirectUri?: string; + + async generateAuthUrl(_state: string, redirectUri: string): Promise<{ url: string }> { + this.authUrlCalls += 1; + this.lastRedirectUri = redirectUri; + return { url: `${redirectUri}?started=1` }; + } + + async exchangeToken(code: string, _state: string, _redirectUri: string): Promise { + return { access: `access-${code}`, refresh: "refresh", expires: Date.now() + 60_000 }; + } +} + +/** + * Bind a real loopback port so the next `Bun.serve({ port })` against the + * same port fails with EADDRINUSE. Returns the bound port plus a `release` + * callback for teardown. + */ +function occupyLoopbackPort(): { port: number; release: () => void } { + const server = Bun.serve({ port: 0, fetch: () => new Response("blocker") }); + const port = server.port; + if (typeof port !== "number") { + server.stop(true); + throw new Error("Bun.serve({ port: 0 }) did not assign a numeric port"); + } + return { port, release: () => server.stop(true) }; +} + +afterEach(() => { + vi.restoreAllMocks(); +}); + +describe("OAuthCallbackFlow port fallback policy", () => { + it("falls back to a random port by default so historical AI-provider flows keep working", async () => { + const blocker = occupyLoopbackPort(); + const progress: string[] = []; + const flow = new TestCallbackFlow( + { + onAuth: () => {}, + onProgress: msg => progress.push(msg), + // Short abort — we only care that the flow advertised the fallback URI. + signal: AbortSignal.timeout(100), + }, + { preferredPort: blocker.port }, + ); + + try { + await expect(flow.login()).rejects.toThrow(); // aborted while waiting for the browser callback + const fallbackNotice = progress.find(msg => msg.startsWith(`Preferred port ${blocker.port} unavailable`)); + expect(fallbackNotice).toBeDefined(); + // Notice carries a different (random) port, never the blocked one. + expect(fallbackNotice).not.toContain(`using port ${blocker.port}`); + // generateAuthUrl ran with the random-port redirect URI — that's the + // silent fallback behavior that MCP flows now opt out of. + expect(flow.authUrlCalls).toBe(1); + expect(flow.lastRedirectUri).toMatch(/^http:\/\/localhost:\d+\/callback$/); + expect(flow.lastRedirectUri).not.toContain(`:${blocker.port}/`); + } finally { + blocker.release(); + } + }); + + it("throws a ConfigurationError when allowPortFallback is false", async () => { + const serveSpy = vi.spyOn(Bun, "serve").mockImplementation(() => { + throw new Error("EADDRINUSE"); + }); + + const flow = new TestCallbackFlow( + { + onAuth: () => {}, + signal: AbortSignal.timeout(1_000), + }, + { preferredPort: 14581, allowPortFallback: false }, + ); + + await expect(flow.login()).rejects.toThrow(ConfigurationError); + await expect(flow.login()).rejects.toThrow( + /OAuth callback port 14581 is in use\. The OAuth provider validates redirect URIs/, + ); + // Fallback to port 0 must never be attempted: every serve call uses the preferred port. + const portArgs = serveSpy.mock.calls.map(([opts]) => opts.port); + expect(portArgs.every(port => port === 14581)).toBe(true); + // generateAuthUrl never runs: the error fires before login() opens the browser. + expect(flow.authUrlCalls).toBe(0); + }); + + it("preserves redirectUri-strict behavior with the updated error message", async () => { + vi.spyOn(Bun, "serve").mockImplementation(() => { + throw new Error("EADDRINUSE"); + }); + + const flow = new TestCallbackFlow( + { + onAuth: () => {}, + signal: AbortSignal.timeout(1_000), + }, + { + preferredPort: 14582, + redirectUri: "http://localhost:14582/callback", + }, + ); + + // redirectUri takes precedence over allowPortFallback in the error + // message so users learn exactly which configuration knob is forcing + // the strict port match. + await expect(flow.login()).rejects.toThrow( + /oauth\.redirectUri \(http:\/\/localhost:14582\/callback\) requires this exact port/, + ); + }); +}); diff --git a/packages/coding-agent/CHANGELOG.md b/packages/coding-agent/CHANGELOG.md index 46831be05..309c66204 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -14,6 +14,8 @@ ### Fixed +- Fixed MCP OAuth flows silently advertising a random-port redirect URI when the preferred callback port (default 3000) was busy. Providers that validate redirect URIs against a registered callback (e.g. Atlassian) returned an opaque 500 page, leaving the local flow waiting until its 5-minute timeout. `MCPOAuthFlow` now opts out of random-port fallback whenever a static `client_id` is already pinned (via `oauth.clientId` or embedded in the authorization URL), failing fast with a `ConfigurationError` that names the busy port and the remediation (free the port, or set `oauth.callbackPort`/`oauth.redirectUri` in `mcp.json`). Fresh dynamic-client-registration flows still fall back so first-install users on a busy default port keep working — DCR registers the actual loopback URI on the fly. ([#3887](https://github.com/can1357/oh-my-pi/issues/3887)) + - Fixed auto-compaction dead-ends by automatically triggering a shake rescue to elide oversized tails - Improved compaction warning message to suggest running `/shake images` for irreducible image tails - Fixed `grep`/`search` direct execution to accept JSON-array string `paths` for string-or-array inputs. ([#3873](https://github.com/can1357/oh-my-pi/issues/3873)) diff --git a/packages/coding-agent/src/mcp/oauth-flow.ts b/packages/coding-agent/src/mcp/oauth-flow.ts index 328d2e757..d6b2bc5ee 100644 --- a/packages/coding-agent/src/mcp/oauth-flow.ts +++ b/packages/coding-agent/src/mcp/oauth-flow.ts @@ -153,14 +153,44 @@ function resolveCallbackHostname(redirectUri: string | undefined): string | unde return parsed.hostname; } +/** + * Resolve the client_id MCPOAuthFlow would use without doing any I/O — + * either the explicitly configured value or one embedded as a query parameter + * in the authorization URL. Returns `undefined` when no client_id is known + * statically, which is the trigger for dynamic client registration in + * {@link MCPOAuthFlow.#tryRegisterClient}. + */ +function staticClientIdFromConfig(config: MCPOAuthConfig): string | undefined { + const fromConfig = config.clientId?.trim(); + if (fromConfig) return fromConfig; + try { + return new URL(config.authorizationUrl).searchParams.get("client_id") ?? undefined; + } catch { + return undefined; + } +} + function resolveCallbackOptions(config: MCPOAuthConfig): OAuthCallbackFlowOptions { const redirectUri = resolveRedirectUri(config.redirectUri); validateRedirectConfig(config, redirectUri); + // When a client_id is already pinned (config-supplied or embedded in the + // authorization URL), it was registered against a specific redirect URI. + // Silently advertising a different port at the authorize endpoint would + // be rejected by providers like Atlassian (HTTP 500 in the browser, local + // flow hangs until the 5-minute timeout), so fail fast instead. + // + // When no client_id is pinned, MCPOAuthFlow will attempt dynamic client + // registration on demand with whichever loopback URI we actually bound — + // the provider issues a client_id tied to *that* URI, so the random-port + // fallback remains safe for first-install DCR flows whose preferred port + // happens to be occupied. + const allowPortFallback = staticClientIdFromConfig(config) === undefined; return { preferredPort: resolveCallbackPort(config.callbackPort, redirectUri), callbackPath: resolveCallbackPath(config.callbackPath, redirectUri), callbackHostname: resolveCallbackHostname(redirectUri), redirectUri, + allowPortFallback, }; } @@ -453,14 +483,7 @@ export class MCPOAuthFlow extends OAuthCallbackFlow { } #resolveClientId(config: MCPOAuthConfig): string | undefined { - const fromConfig = config.clientId?.trim(); - if (fromConfig) return fromConfig; - - try { - return new URL(config.authorizationUrl).searchParams.get("client_id") ?? undefined; - } catch { - return undefined; - } + return staticClientIdFromConfig(config); } #resourceFromAuthorizationUrl(authorizationUrl: string): string | undefined { try { diff --git a/packages/coding-agent/test/oauth-flow.test.ts b/packages/coding-agent/test/oauth-flow.test.ts index cabee107d..4efcf4b00 100644 --- a/packages/coding-agent/test/oauth-flow.test.ts +++ b/packages/coding-agent/test/oauth-flow.test.ts @@ -426,7 +426,7 @@ describe("mcp oauth flow", () => { ); await expect(flow.login()).rejects.toThrow( - "OAuth callback port 80 unavailable; cannot fall back to a random port when oauth.redirectUri is set", + "OAuth callback port 80 is in use, but oauth.redirectUri (http://localhost/callback) requires this exact port", ); expect(serveSpy).toHaveBeenCalledTimes(1); }); @@ -447,7 +447,7 @@ describe("mcp oauth flow", () => { ); await expect(flow.login()).rejects.toThrow( - "OAuth callback port 3000 unavailable; cannot fall back to a random port when oauth.redirectUri is set", + "OAuth callback port 3000 is in use, but oauth.redirectUri (http://localhost:3000/callback) requires this exact port", ); expect(serveSpy).toHaveBeenCalledTimes(1); }); @@ -468,7 +468,123 @@ describe("mcp oauth flow", () => { { signal: AbortSignal.timeout(1_000) }, ); - await expect(flow.login()).rejects.toThrow("cannot fall back to a random port when oauth.redirectUri is set"); + await expect(flow.login()).rejects.toThrow( + /oauth\.redirectUri \(https:\/\/public\.example\/slack\/oauth_redirect\) requires this exact port/, + ); + }); + + it("fails fast when the preferred port is busy and a static clientId pins the registered redirect URI", async () => { + const serveSpy = vi.spyOn(Bun, "serve").mockImplementation(options => { + expect(options.port).toBe(14572); + throw new Error("EADDRINUSE"); + }); + + const progress: string[] = []; + const onAuth = vi.fn(); + const flow = new MCPOAuthFlow( + { + authorizationUrl: "https://provider.example/authorize", + tokenUrl: "https://provider.example/token", + clientId: "demo-client", + callbackPort: 14572, + }, + { + onAuth, + onProgress: msg => progress.push(msg), + signal: AbortSignal.timeout(1_000), + }, + ); + + await expect(flow.login()).rejects.toThrow( + /OAuth callback port 14572 is in use\. The OAuth provider validates redirect URIs/, + ); + // Fallback must NOT have been attempted: only the preferred-port serve call. + expect(serveSpy).toHaveBeenCalledTimes(1); + // Browser must not be opened — the error fires before generateAuthUrl runs. + expect(onAuth).not.toHaveBeenCalled(); + // And the silent "Preferred port X unavailable, using port Y" message must + // never reach the user — that's the regression this test guards against. + expect(progress.some(msg => msg.includes("Preferred port"))).toBe(false); + }); + + it("falls back to a random port when DCR will re-register with the actual loopback URI", async () => { + // The bot reviewer's concern: blocking fallback for *every* MCP flow + // would break first-install DCR users whose preferred port is busy. + // Here `clientId` is unset, so `MCPOAuthFlow.#tryRegisterClient` will + // register the actual fallback URI with the provider and the + // authorization request will use that fresh client_id. + const blocker = Bun.serve({ port: 0, fetch: () => new Response("blocker") }); + const blockerPort = blocker.port; + if (typeof blockerPort !== "number") { + blocker.stop(true); + throw new Error("Bun.serve({ port: 0 }) did not assign a numeric port"); + } + + const registrations: unknown[] = []; + const fetchImpl: FetchImpl = async (input, init) => { + const url = typeof input === "string" ? input : input instanceof URL ? input.toString() : input.url; + if (url.endsWith("/.well-known/oauth-authorization-server")) { + return new Response(JSON.stringify({ registration_endpoint: "https://provider.example/register" }), { + status: 200, + headers: { "Content-Type": "application/json" }, + }); + } + if (url === "https://provider.example/register") { + registrations.push(JSON.parse(String(init?.body))); + return new Response(JSON.stringify({ client_id: "dcr-issued-client" }), { + status: 200, + headers: { "Content-Type": "application/json" }, + }); + } + return new Response("not implemented", { status: 501 }); + }; + + const progress: string[] = []; + let authCalls = 0; + let advertisedUrl = ""; + try { + const flow = new MCPOAuthFlow( + { + authorizationUrl: "https://provider.example/authorize", + tokenUrl: "https://provider.example/token", + // No clientId, no redirectUri — pure DCR flow. + callbackPort: blockerPort, + fetch: fetchImpl, + }, + { + onAuth: ({ url }) => { + authCalls += 1; + advertisedUrl = url; + }, + onProgress: msg => progress.push(msg), + // Abort once the flow is waiting for the browser callback we never deliver. + signal: AbortSignal.timeout(500), + }, + ); + + await expect(flow.login()).rejects.toThrow(); // aborted while awaiting callback + + // 1. The user saw the silent-fallback notice — fallback was attempted, not refused. + const fallbackNotice = progress.find(msg => msg.startsWith(`Preferred port ${blockerPort} unavailable`)); + expect(fallbackNotice).toBeDefined(); + expect(fallbackNotice).not.toContain(`using port ${blockerPort}`); + + // 2. generateAuthUrl ran with a random-port redirect URI. + expect(authCalls).toBe(1); + const authParams = new URL(advertisedUrl).searchParams; + const advertisedRedirect = authParams.get("redirect_uri") ?? ""; + expect(advertisedRedirect).toMatch(/^http:\/\/localhost:\d+\/callback$/); + expect(advertisedRedirect).not.toContain(`:${blockerPort}/`); + + // 3. DCR re-registered with that same fallback URI, so the + // provider's authorization server will accept it. + expect(registrations).toEqual([expect.objectContaining({ redirect_uris: [advertisedRedirect] })]); + // And the issued client_id was used in the authorize request. + expect(authParams.get("client_id")).toBe("dcr-issued-client"); + expect(flow.resolvedClientId).toBe("dcr-issued-client"); + } finally { + blocker.stop(true); + } }); it("exposes the dynamically registered client_id and client_secret after generateAuthUrl", async () => {