From 857b1191431a5f48dfe8e38578f401a38922f86e Mon Sep 17 00:00:00 2001 From: Sawyer Date: Tue, 8 Sep 2026 14:11:43 -0700 Subject: [PATCH 1/2] Keep the OAuth callback server from blocking on stale ports The callback server already listened on port 0; this makes bind failures actionable and ensures disposal cannot leave an OAuth flow hung. A failed bind now rejects with an error naming the OAuth callback server and hinting at a retry instead of a bare EADDRINUSE, and close() rejects any pending waitForCode so a disposed toolset interrupts interactive auth instead of waiting forever. --- src/mcp/callback-server.test.ts | 63 +++++++++++++++++++++++++++++++++ src/mcp/callback-server.ts | 37 +++++++++++++++---- 2 files changed, 93 insertions(+), 7 deletions(-) diff --git a/src/mcp/callback-server.test.ts b/src/mcp/callback-server.test.ts index 1929e7c0c..cc4183ce3 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,65 @@ describe("MCP callback server", () => { server.close(); } }); + + test("binds an ephemeral OS-assigned port on 127.0.0.1", async () => { + const server = await startCallbackServer(); + try { + const url = new URL(server.redirectUrl); + + expect(url.hostname).toBe("127.0.0.1"); + expect(Number(url.port)).toBeGreaterThan(0); + } finally { + server.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("reports a clear actionable error when the server fails to bind", 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 EADDRINUSE: address already in use")); + }, + address: (): undefined => undefined, + }; + return fake as unknown as Server; + }) as typeof real.createServer, + }), + async () => { + const err: unknown = await startCallbackServer().then( + () => undefined, + (failure: unknown) => failure, + ); + const error = err as Error; + + expect(error.message).toContain("OAuth callback server"); + expect(error.message).toContain("EADDRINUSE"); + expect(error.message).toContain("retry"); + }, + ); + }); }); diff --git a/src/mcp/callback-server.ts b/src/mcp/callback-server.ts index d812fcb41..cf8e6eb2a 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` +// The listen port is always 0, so the OS assigns an ephemeral port and +// concurrent sessions can never collide on a fixed callback port. `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. export async function startCallbackServer(serverName?: string): Promise { + let closed = false; let expectedState: string | undefined; let pendingResult: CallbackResult | undefined; let waiter: CallbackWaiter | undefined; @@ -77,7 +77,15 @@ 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}. ` + + "The server always requests an ephemeral port, so this is unexpected — " + + "retry connecting to the MCP server.", + ), + ); + }); server.listen(0, "127.0.0.1", resolve); }); @@ -92,6 +100,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 +127,17 @@ 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. + closed = true; + pendingResult = undefined; + if (waiter !== undefined) { + const activeWaiter = waiter; + clearAuthorization(); + activeWaiter.reject(new Error(CLOSED_ERROR)); + } + server.close(); + }, }; } From 2e47f45a7df7793b7198e21646cfd15057868a96 Mon Sep 17 00:00:00 2001 From: Sawyer Date: Wed, 9 Sep 2026 10:40:45 -0700 Subject: [PATCH 2/2] Fail closed when the OAuth callback server is disposed close() must reject waitForCode so toolset disposal cannot leave interactive authorization hung. Bind failures keep the OS error rather than telling the user to retry an ephemeral port. --- src/mcp/callback-server.test.ts | 51 ++++++++++++++++++++++----------- src/mcp/callback-server.ts | 16 ++++------- 2 files changed, 40 insertions(+), 27 deletions(-) diff --git a/src/mcp/callback-server.test.ts b/src/mcp/callback-server.test.ts index cc4183ce3..55c887c7d 100644 --- a/src/mcp/callback-server.test.ts +++ b/src/mcp/callback-server.test.ts @@ -73,15 +73,21 @@ describe("MCP callback server", () => { } }); - test("binds an ephemeral OS-assigned port on 127.0.0.1", async () => { - const server = await startCallbackServer(); + test("binds concurrent servers to distinct loopback ports", async () => { + const first = await startCallbackServer(); + const second = await startCallbackServer(); try { - const url = new URL(server.redirectUrl); - - expect(url.hostname).toBe("127.0.0.1"); - expect(Number(url.port)).toBeGreaterThan(0); + 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 { - server.close(); + first.close(); + second.close(); } }); @@ -100,7 +106,24 @@ describe("MCP callback server", () => { ); }); - test("reports a clear actionable error when the server fails to bind", async () => { + 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")) => ({ @@ -113,7 +136,7 @@ describe("MCP callback server", () => { return fake; }, listen: () => { - listeners.error?.(new Error("listen EADDRINUSE: address already in use")); + listeners.error?.(new Error("listen EACCES: permission denied")); }, address: (): undefined => undefined, }; @@ -121,15 +144,9 @@ describe("MCP callback server", () => { }) as typeof real.createServer, }), async () => { - const err: unknown = await startCallbackServer().then( - () => undefined, - (failure: unknown) => failure, + await expect(startCallbackServer()).rejects.toThrow( + "Could not start the OAuth callback server: listen EACCES: permission denied", ); - const error = err as Error; - - expect(error.message).toContain("OAuth callback server"); - expect(error.message).toContain("EADDRINUSE"); - expect(error.message).toContain("retry"); }, ); }); diff --git a/src/mcp/callback-server.ts b/src/mcp/callback-server.ts index cf8e6eb2a..8e4a25a63 100644 --- a/src/mcp/callback-server.ts +++ b/src/mcp/callback-server.ts @@ -21,9 +21,9 @@ interface CallbackWaiter { const CALLBACK_PATH = "/callback"; const CLOSED_ERROR = "OAuth callback server closed before authorization completed."; -// The listen port is always 0, so the OS assigns an ephemeral port and -// concurrent sessions can never collide on a fixed callback port. `serverName` -// only names the authorization on the page the browser lands on. +// 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; @@ -36,6 +36,7 @@ export async function startCallbackServer(serverName?: string): Promise { + if (closed) return; if (waiter === undefined) { pendingResult = result; return; @@ -78,13 +79,7 @@ export async function startCallbackServer(serverName?: string): Promise((resolve, reject) => { server.once("error", (err) => { - reject( - new Error( - `Could not start the OAuth callback server: ${err.message}. ` + - "The server always requests an ephemeral port, so this is unexpected — " + - "retry connecting to the MCP server.", - ), - ); + reject(new Error(`Could not start the OAuth callback server: ${err.message}`)); }); server.listen(0, "127.0.0.1", resolve); }); @@ -130,6 +125,7 @@ export async function startCallbackServer(serverName?: string): Promise { // 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) {