diff --git a/src/mcp/callback-server.test.ts b/src/mcp/callback-server.test.ts index 1929e7c0c..55c887c7d 100644 --- a/src/mcp/callback-server.test.ts +++ b/src/mcp/callback-server.test.ts @@ -1,5 +1,7 @@ import { describe, expect, test } from "bun:test"; +import type { Server } from "node:http"; +import { withMockedModuleDuring } from "../../tests/helpers/mock-module.js"; import { startCallbackServer } from "./callback-server.js"; const authorize = ( @@ -70,4 +72,82 @@ describe("MCP callback server", () => { server.close(); } }); + + test("binds concurrent servers to distinct loopback ports", async () => { + const first = await startCallbackServer(); + const second = await startCallbackServer(); + try { + const firstUrl = new URL(first.redirectUrl); + const secondUrl = new URL(second.redirectUrl); + + expect(firstUrl.hostname).toBe("127.0.0.1"); + expect(secondUrl.hostname).toBe("127.0.0.1"); + expect(firstUrl.port).not.toBe(secondUrl.port); + expect(Number(firstUrl.port)).toBeGreaterThan(0); + expect(Number(secondUrl.port)).toBeGreaterThan(0); + } finally { + first.close(); + second.close(); + } + }); + + test("rejects a pending waitForCode when the server is closed", async () => { + const server = await startCallbackServer(); + const pending = server.waitForCode(new AbortController().signal).then( + () => "resolved", + (err: Error) => err.message, + ); + + server.close(); + + expect(await pending).toContain("closed before authorization completed"); + await expect(server.waitForCode(new AbortController().signal)).rejects.toThrow( + "closed before authorization completed", + ); + }); + + test("close is idempotent and does not leak a waiter after a late callback", async () => { + const server = await startCallbackServer(); + authorize(server, "expected"); + server.close(); + server.close(); + + await expect( + fetch(`${server.redirectUrl}?code=abc&state=expected`).then( + () => "fetched", + (err: unknown) => err, + ), + ).resolves.toBeInstanceOf(Error); + await expect(server.waitForCode(new AbortController().signal)).rejects.toThrow( + "closed before authorization completed", + ); + }); + + test("rejects start when listen fails without rewriting the OS error as a retry", async () => { + await withMockedModuleDuring( + import.meta.resolve("node:http"), + (real: typeof import("node:http")) => ({ + ...real, + createServer: (() => { + const listeners: Partial void>> = {}; + const fake = { + once: (event: string, cb: (err: Error) => void) => { + listeners[event] = cb; + return fake; + }, + listen: () => { + listeners.error?.(new Error("listen EACCES: permission denied")); + }, + address: (): undefined => undefined, + }; + return fake as unknown as Server; + }) as typeof real.createServer, + }), + async () => { + await expect(startCallbackServer()).rejects.toThrow( + "Could not start the OAuth callback server: listen EACCES: permission denied", + ); + }, + ); + }); }); diff --git a/src/mcp/callback-server.ts b/src/mcp/callback-server.ts index d812fcb41..8e4a25a63 100644 --- a/src/mcp/callback-server.ts +++ b/src/mcp/callback-server.ts @@ -7,7 +7,7 @@ export interface CallbackServer { redirectUrl: string; expectState: (state: string) => void; // Resolves once the authorization server redirects back with a code, or rejects - // if the signal aborts or the server reports an error. + // if the signal aborts, the server reports an error, or close() runs. waitForCode: (signal: AbortSignal) => Promise; close: () => void; } @@ -19,13 +19,13 @@ interface CallbackWaiter { } const CALLBACK_PATH = "/callback"; +const CLOSED_ERROR = "OAuth callback server closed before authorization completed."; -// Start an ephemeral loopback server to receive the OAuth redirect. `serverName` -// only names the authorization on the page the browser lands on. -// Binds to a -// random port on 127.0.0.1 so it never collides with anything and is only -// reachable locally. +// Start a loopback server to receive the OAuth redirect. close() fail-closes +// waitForCode so disposing the toolset cannot leave authorization hung. +// `serverName` only names the authorization on the page the browser lands on. export async function startCallbackServer(serverName?: string): Promise { + let closed = false; let expectedState: string | undefined; let pendingResult: CallbackResult | undefined; let waiter: CallbackWaiter | undefined; @@ -36,6 +36,7 @@ export async function startCallbackServer(serverName?: string): Promise { + if (closed) return; if (waiter === undefined) { pendingResult = result; return; @@ -77,7 +78,9 @@ export async function startCallbackServer(serverName?: string): Promise((resolve, reject) => { - server.once("error", reject); + server.once("error", (err) => { + reject(new Error(`Could not start the OAuth callback server: ${err.message}`)); + }); server.listen(0, "127.0.0.1", resolve); }); @@ -92,6 +95,10 @@ export async function startCallbackServer(serverName?: string): Promise new Promise((resolve, reject) => { + if (closed) { + reject(new Error(CLOSED_ERROR)); + return; + } if (signal.aborted) { reject(new Error("aborted")); return; @@ -115,6 +122,18 @@ export async function startCallbackServer(serverName?: string): Promise server.close(), + close: () => { + // Rejecting the waiter here is what unblocks toolset disposal: a server + // that merely stops listening would leave a pending waitForCode hung. + if (closed) return; + closed = true; + pendingResult = undefined; + if (waiter !== undefined) { + const activeWaiter = waiter; + clearAuthorization(); + activeWaiter.reject(new Error(CLOSED_ERROR)); + } + server.close(); + }, }; }