Merge remote-tracking branch 'origin/farm/bcae1c50/fix-mcp-oauth-port-fallback'

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))

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 {

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 () => {