diff --git a/packages/ai/CHANGELOG.md b/packages/ai/CHANGELOG.md index cc98662b5..c703a0681 100644 --- a/packages/ai/CHANGELOG.md +++ b/packages/ai/CHANGELOG.md @@ -2,6 +2,14 @@ ## [Unreleased] +### Added + +- Added `OAuthCallbackFlowOptions.allowPortFallback` (default `true`) so subclasses representing providers that validate redirect URIs against a registered callback (e.g. MCP servers like Atlassian) can refuse the silent random-port fallback and surface an actionable `ConfigurationError` before opening the browser. + +### 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 bf5ccf54c..b6319e7c7 100644 --- a/packages/coding-agent/CHANGELOG.md +++ b/packages/coding-agent/CHANGELOG.md @@ -12,6 +12,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 unconditionally, so the flow fails fast with a `ConfigurationError` that names the busy port and the remediation (free the port, or set `oauth.callbackPort`/`oauth.redirectUri` in `mcp.json`). ([#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..b01c81cb4 100644 --- a/packages/coding-agent/src/mcp/oauth-flow.ts +++ b/packages/coding-agent/src/mcp/oauth-flow.ts @@ -161,6 +161,13 @@ function resolveCallbackOptions(config: MCPOAuthConfig): OAuthCallbackFlowOption callbackPath: resolveCallbackPath(config.callbackPath, redirectUri), callbackHostname: resolveCallbackHostname(redirectUri), redirectUri, + // MCP providers (Atlassian, GitHub MCP, etc.) generally validate the + // redirect URI against a registered callback. Silently advertising a + // random port when the preferred one is busy means the authorization + // server returns an opaque 500 and the browser callback never fires, + // leaving the user staring at the 5-minute timeout. Fail fast so the + // caller can show an actionable error before opening the browser. + allowPortFallback: false, }; } diff --git a/packages/coding-agent/test/oauth-flow.test.ts b/packages/coding-agent/test/oauth-flow.test.ts index cabee107d..22c169763 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,43 @@ 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 no redirectUri is configured", 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("exposes the dynamically registered client_id and client_secret after generateAuthUrl", async () => {