fix(mcp): classify loopback from the bound address and keep startup failures out of onerror

Addresses the third automated review round: loopback is classified from the
address the listener bound with the configured name added to the Host and
Origin allowlist; scoped IPv6 literals are refused before binding and a URL
that cannot be formed closes the listener; listener errors reach onerror only
after a successful bind; the MCP runtime keeps its factory owner as the single
owner of instance cleanup through `closeInstance: false`.

Co-Authored-By: Claude Fable 5.1 <noreply@anthropic.com>

docs/mcp-contracts.md

@@ -447,12 +447,16 @@ application/json` and an `Accept` naming both `application/json` and
 session; any other sessionless POST goes to the live transport, which refuses
 it without disturbing the session. Requests that carry no valid session id are
 refused with `400` or `404` as the SDK transport defines. A bind is loopback by
-address class, not spelling: any `127.0.0.0/8` address, `::1`, an IPv4-mapped
-loopback and `localhost` names. On such a bind, requests whose `Host` header is
-outside the SDK loopback allowlist plus the bind's own hostname are refused with
-`403`, and a browser `Origin` outside that allowlist is refused the same way (a
-missing `Origin` passes); a non-loopback bind relaxes both checks and logs the
-relaxation. Request bodies are bounded by the SDK's
+address class, not spelling, and is classified from the address the listener
+actually bound: any `127.0.0.0/8` address, `::1`, an IPv4-mapped loopback and
+`localhost` names, including a host alias that resolves to one of them. On such
+a bind, requests whose `Host` header is outside the SDK loopback allowlist plus
+the configured name and the bound address are refused with `403`, and a browser
+`Origin` outside that allowlist is refused the same way (a missing `Origin`
+passes); a non-loopback bind relaxes both checks and logs the relaxation. Scoped
+IPv6 literals (`fe80::1%eth0`) are refused before binding, because no URL can
+name them. Listener errors reach the runtime's connection-loss handler only
+after a successful bind; a bind failure prints the listen-failure message alone. Request bodies are bounded by the SDK's
 4 MiB limit, and the response budget keeps its stdio-derived floor. The
 listener ends on SIGINT or SIGTERM only, not on stdin EOF, and exits `130` or
 `143` after cleanup settles.

docs/specs/agent-environments/http-transport/spec.md

@@ -30,9 +30,14 @@ client registration keep stdio.
   response chunks as they are produced and turning a client disconnect into a
   request abort and body cancellation.
 - Classify a loopback bind by address class (`127.0.0.0/8`, `::1`, IPv4-mapped
-  loopback, `localhost` names), validate the `Host` and `Origin` headers with
-  the SDK loopback allowlist plus the bind's own hostname on such binds, and
-  relax both for other binds and log that relaxation.
+  loopback, `localhost` names) from the address the listener bound, validate
+  the `Host` and `Origin` headers with the SDK loopback allowlist plus the
+  configured name and the bound address on such binds, and relax both for
+  other binds and log that relaxation. Refuse scoped IPv6 literals before
+  binding, close the listener if its URL cannot be formed, and forward
+  listener errors to `onerror` only after a successful bind.
+- Let the caller own instance cleanup (`closeInstance: false`) when its factory
+  owner already closes every instance, as the MCP runtime does.
 - Let only an `initialize` that carries the streamable headers and passes those
   checks replace an open session; refuse requests once the slot is closed, join
   an arming session during shutdown, and keep close stages retryable.

docs/specs/agent-environments/http-transport/verification.md

@@ -204,3 +204,15 @@ initialize is refused while the connected client keeps echoing), and the
 server instance is created only after the listener has bound, so a bind
 failure has nothing to clean up and a factory failure releases the listener
 and rethrows (test: a throwing factory rejects without announcing a URL).
+
+A third round (Greptile and Codex, five findings) was addressed next: the
+listener's `error` forwarding to `onerror` is registered after the bind, so
+`EADDRINUSE` prints only the listen-failure message (test: an occupied port
+rejects without calling `onerror`); scoped IPv6 literals are refused before
+binding and a URL that cannot be formed from the bound address closes the
+listener (test: `fe80::1%lo0` rejects before the factory runs); loopback is
+classified from the bound address with the configured name added to the
+allowlist (test: `ip6-localhost` bound to `::1`, `dev.local` bound to
+`127.0.0.1`); and the MCP runtime passes `closeInstance: false` so the factory
+owner in `startMcpTransport` remains the single owner of instance cleanup
+(test: `close` leaves the instance open until its owner closes it).

src/main.ts

@@ -51,9 +51,11 @@ const httpRuntimeDependencies = (
       // Loaded here so the default stdio entry never loads the HTTP listener.
       const { serveStreamableHttp } =
         await import("./server/http/serveStreamableHttp.js");
+      // The transport's factory owner closes every instance it created; the listener must not race it.
       return serveStreamableHttp({
         ...endpoint,
         createMcpServer: factory,
+        closeInstance: false,
         ...options,
       });
     },

src/server/http/serveStreamableHttp.test.ts

@@ -359,3 +359,65 @@ it("releases the listener when the server factory fails after the port is bound"
   for (const line of logs) seen.push((JSON.parse(line) as { msg: string }).msg);
   expect(seen).not.toContain("MCP transport listening");
 });
+
+it("classifies loopback from the bound address and lists the configured name beside it", () => {
+  const sdk = ["localhost", "127.0.0.1", "[::1]"];
+  expect(hostHeaderAllowlist("ip6-localhost", "::1")).toEqual([
+    ...sdk,
+    "ip6-localhost",
+  ]);
+  expect(hostHeaderAllowlist("dev.local", "127.0.0.1")).toEqual([
+    ...sdk,
+    "dev.local",
+  ]);
+  expect(hostHeaderAllowlist("0.0.0.0", "0.0.0.0")).toBeUndefined();
+  expect(hostHeaderAllowlist("dev.local", "192.0.2.10")).toBeUndefined();
+});
+
+it("refuses a scoped IPv6 bind before creating anything and keeps bind failures away from onerror", async () => {
+  let factoryCalls = 0;
+  const onerror = (): void => {
+    throw new Error("onerror must not run during startup");
+  };
+  await expect(
+    serveStreamableHttp({
+      host: "fe80::1%lo0",
+      port: 0,
+      onerror,
+      createMcpServer: () => {
+        factoryCalls += 1;
+        return createEchoServer(() => undefined);
+      },
+    }),
+  ).rejects.toBeInstanceOf(StreamableHttpListenError);
+  expect(factoryCalls).toBe(0);
+  const { served } = await start();
+  await expect(
+    serveStreamableHttp({
+      host: "127.0.0.1",
+      port: Number(new URL(served.url).port),
+      onerror,
+      createMcpServer: () => createEchoServer(() => undefined),
+    }),
+  ).rejects.toThrow(/EADDRINUSE/u);
+});
+
+it("leaves the instance to its owner when closeInstance is false", async () => {
+  let instanceCloses = 0;
+  const server = createEchoServer(() => undefined);
+  const original = server.close.bind(server);
+  server.close = async () => {
+    instanceCloses += 1;
+    await original();
+  };
+  const served = await serveStreamableHttp({
+    host: "127.0.0.1",
+    port: 0,
+    closeInstance: false,
+    createMcpServer: () => server,
+  });
+  await served.close();
+  expect(instanceCloses).toBe(0);
+  await server.close();
+  expect(instanceCloses).toBe(1);
+});

src/server/http/serveStreamableHttp.ts

@@ -41,6 +41,8 @@ export interface ServeStreamableHttpOptions {
   readonly logger?: Logger;
   /** Reports listener failures after binding; request failures are logged instead. */
   readonly onerror?: (error: Error) => void;
+  /** Close the server instance as the last stage of `close`; false when the factory's owner closes it. */
+  readonly closeInstance?: boolean;
 }
 
 export interface StreamableHttpServer {
@@ -78,11 +80,15 @@ const isLoopbackHostname = (hostname: string): boolean => {
   const hex = /^([0-9a-f]{1,4}):[0-9a-f]{1,4}$/u.exec(mapped);
   return hex !== null && Number.parseInt(hex[1] ?? "0", 16) >> 8 === 0x7f;
 };
-/** Host and Origin allowlist for a loopback bind, including the bind's own hostname; undefined relaxes validation. */
-export const hostHeaderAllowlist = (host: string): string[] | undefined => {
-  const hostname = normalizedHostname(host);
-  if (hostname === undefined || !isLoopbackHostname(hostname)) return undefined;
-  return [...new Set([...localhostAllowedHostnames(), hostname])];
+/** Host and Origin allowlist when the configured host or the address it bound is loopback, including both names; undefined relaxes validation. */
+export const hostHeaderAllowlist = (
+  host: string,
+  boundAddress = host,
+): string[] | undefined => {
+  const names = [normalizedHostname(host), normalizedHostname(boundAddress)];
+  const known = names.filter((name): name is string => name !== undefined);
+  if (!known.some((name) => isLoopbackHostname(name))) return undefined;
+  return [...new Set([...localhostAllowedHostnames(), ...known])];
 };
 
 const jsonError = (status: number, code: number, message: string): Response =>
@@ -240,9 +246,15 @@ const endpointUrl = (
 export const serveStreamableHttp = async (
   options: ServeStreamableHttpOptions,
 ): Promise<StreamableHttpServer> => {
-  const { host, port, logger, routes = [] } = options;
-  const allowedHosts = hostHeaderAllowlist(host);
-  // The instance is created after the listener binds, so a bind failure leaves nothing to clean up.
+  const { host, port, logger, routes = [], closeInstance = true } = options;
+  // Scoped IPv6 literals bind but cannot be named in a URL; refuse them before anything exists.
+  if (host.includes("%"))
+    throw new StreamableHttpListenError(
+      new Error(`scoped IPv6 bind address ${host} is not supported`),
+    );
+  // Loopback is classified from the address the listener binds, after it binds; the instance
+  // is created after that, so a bind failure leaves nothing to clean up.
+  let allowedHosts: string[] | undefined;
   let slot: McpSessionSlot | undefined;
   let origin = "";
   let stopped = false;
@@ -269,31 +281,15 @@ export const serveStreamableHttp = async (
       : match.handle(request);
   };
   const httpServer = createHttpServer();
-  const handleRequest = async (
+  const handleRequest = (
     request: IncomingMessage,
     response: ServerResponse,
-  ): Promise<void> => {
-    try {
-      const webResponse = await route(toWebRequest(request, response, origin));
-      await writeWebResponse(webResponse, response);
-    } catch (cause: unknown) {
-      // A client that left mid-stream is routine; a failure before headers is not.
-      logger?.[response.headersSent ? "debug" : "warn"](
-        { error: describe(cause) },
-        "MCP HTTP request did not complete",
-      );
-      if (response.headersSent) response.destroy();
-      else
-        await writeWebResponse(
-          jsonError(500, -32603, "Internal server error"),
-          response,
-        ).catch(() => undefined);
-    }
-  };
-  httpServer.on("request", (request, response) => {
-    void handleRequest(request, response);
-  });
-  httpServer.on("error", (error) => options.onerror?.(error));
+  ): Promise<void> =>
+    serveNodeRequest(
+      { route, origin: () => origin, logger },
+      request,
+      response,
+    );
   const closeListener = async (): Promise<void> => {
     stopped = true;
     httpServer.closeAllConnections();
@@ -309,8 +305,24 @@ export const serveStreamableHttp = async (
   } catch (cause: unknown) {
     throw new StreamableHttpListenError(cause);
   }
-  const url = endpointUrl(httpServer.address());
-  origin = new URL(url).origin;
+  const bound = httpServer.address();
+  let url: string;
+  try {
+    url = endpointUrl(bound);
+    origin = new URL(url).origin;
+  } catch (cause: unknown) {
+    await closeListener().catch(() => undefined);
+    throw new StreamableHttpListenError(cause);
+  }
+  allowedHosts = hostHeaderAllowlist(
+    host,
+    bound !== null && typeof bound === "object" ? bound.address : host,
+  );
+  httpServer.on("request", (request, response) => {
+    void handleRequest(request, response);
+  });
+  // Bind failures already rejected the listening wait above; only later listener errors are reported.
+  httpServer.on("error", (error) => options.onerror?.(error));
   let instance: McpServer | Server;
   try {
     instance = await options.createMcpServer({ era: "legacy" });
@@ -335,11 +347,40 @@ export const serveStreamableHttp = async (
   const close = stagedClose([
     closeListener,
     () => live.close(),
-    () => instance.close(),
+    ...(closeInstance ? [() => instance.close()] : []),
   ]);
   return { url, close };
 };
 
+/** Translate one node:http exchange through `route`, logging a failure and answering it when headers are still open. */
+const serveNodeRequest = async (
+  target: {
+    readonly route: (request: Request) => Promise<Response>;
+    readonly origin: () => string;
+    readonly logger: Logger | undefined;
+  },
+  request: IncomingMessage,
+  response: ServerResponse,
+): Promise<void> => {
+  const { route, origin, logger } = target;
+  try {
+    const webResponse = await route(toWebRequest(request, response, origin()));
+    await writeWebResponse(webResponse, response);
+  } catch (cause: unknown) {
+    // A client that left mid-stream is routine; a failure before headers is not.
+    logger?.[response.headersSent ? "debug" : "warn"](
+      { error: describe(cause) },
+      "MCP HTTP request did not complete",
+    );
+    if (response.headersSent) response.destroy();
+    else
+      await writeWebResponse(
+        jsonError(500, -32603, "Internal server error"),
+        response,
+      ).catch(() => undefined);
+  }
+};
+
 /** Run close stages in order once each; a failed stage is retried by the next call instead of replaying its rejection. */
 const stagedClose = (
   stages: readonly (() => Promise<void>)[],