Merge pull request #849 from corbitsdev/cl-7529-prevent-callback-server-port-conflicts-across-mcp-sessions

Keep OAuth callback sessions on ephemeral ports

Author: TheGreatAxios

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<Record<string, (err: Error) => 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",
+        );
+      },
+    );
+  });
 });

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<string>;
   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<CallbackServer> {
+  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<Callback
   };
 
   const deliver = (result: CallbackResult): void => {
+    if (closed) return;
     if (waiter === undefined) {
       pendingResult = result;
       return;
@@ -77,7 +78,9 @@ export async function startCallbackServer(serverName?: string): Promise<Callback
   });
 
   await new Promise<void>((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<Callback
     },
     waitForCode: (signal: AbortSignal) =>
       new Promise<string>((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<Callback
           { once: true },
         );
       }),
-    close: () => 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();
+    },
   };
 }