diff --git a/src/cli/commands/broker/index.ts b/src/cli/commands/broker/index.ts index 664140c..b829aa6 100644 --- a/src/cli/commands/broker/index.ts +++ b/src/cli/commands/broker/index.ts @@ -1,15 +1,27 @@ +import type { Server } from "node:http"; import { createBrokerHttpServer, TunnelBroker } from "../../../tunnel/index.js"; import { parseArgs, flagString } from "../../args.js"; import type { CliContext } from "../../context.js"; +import { listenErrorMessage, listenOrError, type ListenOutcome } from "../listen.js"; -export async function runBrokerCommand(argv: string[], context: CliContext): Promise { +export interface BrokerCommandHooks { + // Injectable so tests can deterministically exercise the bind-error path without + // depending on OS hostname resolution. + listen?: (server: Server, port: number, host: string) => Promise; +} + +export async function runBrokerCommand( + argv: string[], + context: CliContext, + hooks: BrokerCommandHooks = {} +): Promise { const [subcommand, ...rest] = argv; - if (subcommand === "serve") return brokerServe(rest, context); + if (subcommand === "serve") return brokerServe(rest, context, hooks); context.stderr.write(`Unknown broker command: ${subcommand ?? ""}\n`); return 1; } -async function brokerServe(argv: string[], context: CliContext): Promise { +async function brokerServe(argv: string[], context: CliContext, hooks: BrokerCommandHooks): Promise { const args = parseArgs(argv); const host = flagString(args, "host") ?? "127.0.0.1"; @@ -22,9 +34,12 @@ async function brokerServe(argv: string[], context: CliContext): Promise logSink: (record) => context.stdout.write(`${JSON.stringify(record)}\n`) }); const server = createBrokerHttpServer(broker); - await new Promise((resolve) => { - server.listen(port, host, resolve); - }); + const listen = hooks.listen ?? listenOrError; + const outcome = await listen(server, port, host); + if (!outcome.ok) { + context.stderr.write(`${listenErrorMessage(host, port, outcome.error)}\n`); + return 1; + } context.stdout.write(`Agent Gather broker serving on ${host}:${port}\n`); if (publicUrl !== undefined) context.stdout.write(`Public URL: ${publicUrl}\n`); diff --git a/src/cli/commands/launch/index.ts b/src/cli/commands/launch/index.ts index 1d9c2f7..5feeb44 100644 --- a/src/cli/commands/launch/index.ts +++ b/src/cli/commands/launch/index.ts @@ -20,6 +20,7 @@ import type { Server } from "node:http"; import { platform as osPlatform } from "node:os"; import { parseArgs, type ParsedArgs } from "../../args.js"; import type { CliContext } from "../../context.js"; +import { listenOrError } from "../listen.js"; import { DEFAULT_PLATFORM_PORT, defaultWaitForShutdown } from "../platform/index.js"; import { buildHelpText, VERSION } from "../../help.js"; import { createPlatformHttpServer } from "../../../platform/index.js"; @@ -179,18 +180,11 @@ function isConnectionRefused(error: unknown): boolean { return code === "ECONNREFUSED"; } +// #232 launcher bind: localhost-only, and a bind failure resolves false so the +// caller refuses with a token-free message (never a kill). Delegates to the shared +// listen helper without changing that behavior. function listen(server: Server, port: number): Promise { - return new Promise((resolve) => { - const onError = (): void => { - server.removeListener("error", onError); - resolve(false); - }; - server.once("error", onError); - server.listen(port, "127.0.0.1", () => { - server.removeListener("error", onError); - resolve(true); - }); - }); + return listenOrError(server, port, "127.0.0.1").then((outcome) => outcome.ok); } // Open a URL in the default browser without a shell: the platform opener is spawned diff --git a/src/cli/commands/listen.ts b/src/cli/commands/listen.ts new file mode 100644 index 0000000..2b273f1 --- /dev/null +++ b/src/cli/commands/listen.ts @@ -0,0 +1,43 @@ +import type { Server } from "node:http"; + +export interface ListenOutcome { + ok: boolean; + error?: NodeJS.ErrnoException; +} + +// Bind a foreground server, guarding the one-shot 'error' event so an occupied or +// invalid bind resolves as a failure outcome instead of throwing an uncaught +// Server error or hanging the listen promise forever. This only observes its own +// listen attempt — it never inspects, kills, or mutates any other process's +// listener. On success the error guard is removed so post-bind behavior is +// unchanged from a bare `server.listen`. +export function listenOrError(server: Server, port: number, host: string): Promise { + return new Promise((resolve) => { + const onError = (error: NodeJS.ErrnoException): void => { + server.removeListener("error", onError); + resolve({ ok: false, error }); + }; + server.once("error", onError); + server.listen(port, host, () => { + server.removeListener("error", onError); + resolve({ ok: true }); + }); + }); +} + +// A controlled, token-free message for a bind failure. It uses only the bind +// coordinates (host/port) and the OS error code — never request, token, or +// message data — so it is always safe to write to stderr. +export function listenErrorMessage(host: string, port: number, error: NodeJS.ErrnoException | undefined): string { + const target = `${host}:${port}`; + switch (error?.code) { + case "EADDRINUSE": + return `Cannot bind ${target}: address already in use.`; + case "EACCES": + return `Cannot bind ${target}: permission denied.`; + case "EADDRNOTAVAIL": + return `Cannot bind ${target}: address not available on this host.`; + default: + return `Cannot bind ${target}${error?.code ? `: ${error.code}` : ""}.`; + } +} diff --git a/src/cli/commands/platform/index.ts b/src/cli/commands/platform/index.ts index a2cbbc5..427525d 100644 --- a/src/cli/commands/platform/index.ts +++ b/src/cli/commands/platform/index.ts @@ -1,6 +1,7 @@ import type { Server } from "node:http"; import { flagBoolean, flagString, parseArgs } from "../../args.js"; import type { CliContext } from "../../context.js"; +import { listenErrorMessage, listenOrError, type ListenOutcome } from "../listen.js"; import { createPlatformHttpServer } from "../../../platform/index.js"; // Default control-plane port: room serve uses 8787, the broker 8799; the owner @@ -12,6 +13,9 @@ export interface PlatformCommandHooks { // Injectable so tests can drive the running server and shut it down instead of // blocking on process signals forever. waitForShutdown?: (server: Server) => Promise; + // Injectable so tests can deterministically exercise the bind-error path without + // depending on OS hostname resolution. + listen?: (server: Server, port: number, host: string) => Promise; } export async function runPlatformCommand( @@ -43,7 +47,12 @@ async function platformServe(argv: string[], context: CliContext, hooks: Platfor } const server = createPlatformHttpServer({ root: context.home, allowInsecureRemote: allowRemote }); - await new Promise((resolve) => server.listen(port, host, resolve)); + const listen = hooks.listen ?? listenOrError; + const outcome = await listen(server, port, host); + if (!outcome.ok) { + context.stderr.write(`${listenErrorMessage(host, port, outcome.error)}\n`); + return 1; + } const address = server.address(); const boundPort = typeof address === "object" && address !== null ? address.port : port; const url = `http://${host}:${boundPort}`; diff --git a/src/cli/commands/room/index.ts b/src/cli/commands/room/index.ts index 996b0d9..37f546a 100644 --- a/src/cli/commands/room/index.ts +++ b/src/cli/commands/room/index.ts @@ -1,4 +1,5 @@ import { execFile } from "node:child_process"; +import type { Server } from "node:http"; import { readFile } from "node:fs/promises"; import path from "node:path"; import { promisify } from "node:util"; @@ -55,8 +56,15 @@ import { import { readPublicBaseUrl } from "../../../tunnel/index.js"; import { parseArgs, flagBoolean, flagString, type ParsedArgs } from "../../args.js"; import type { CliContext } from "../../context.js"; +import { listenErrorMessage, listenOrError, type ListenOutcome } from "../listen.js"; import { readCurrent, readToken, recordJoinedRoom, writeCurrent, writeToken } from "../../state.js"; +export interface RoomCommandHooks { + // Injectable so tests can deterministically exercise the bind-error path without + // depending on OS hostname resolution. + listen?: (server: Server, port: number, host: string) => Promise; +} + // Printed to stderr whenever `room serve --allow-remote` starts (#180). Assumes // HTTPS is terminated at the edge (tunnel/reverse proxy); Agent Gather adds no // TLS of its own. See docs/self-tunnel.md for the full BYO-tunnel trust boundary. @@ -68,7 +76,11 @@ export const ALLOW_REMOTE_WARNING = [ " and let the tunnel own public TLS. See docs/self-tunnel.md." ].join("\n"); -export async function runRoomCommand(argv: string[], context: CliContext): Promise { +export async function runRoomCommand( + argv: string[], + context: CliContext, + hooks: RoomCommandHooks = {} +): Promise { const [subcommand, ...rest] = argv; if (subcommand === "start") return roomStart(rest, context); if (subcommand === "create-boardroom") return roomCreateBoardroom(rest, context); @@ -84,7 +96,7 @@ export async function runRoomCommand(argv: string[], context: CliContext): Promi if (subcommand === "brief") return roomBrief(rest, context); if (subcommand === "attendance") return roomAttendance(rest, context); if (subcommand === "session") return roomSession(rest, context); - if (subcommand === "serve") return roomServe(rest, context); + if (subcommand === "serve") return roomServe(rest, context, hooks); if (subcommand === "launch") return roomLaunch(rest, context); if (subcommand === "runtime-status") return roomRuntimeStatus(rest, context); if (subcommand === "invite") return roomInvite(rest, context); @@ -896,7 +908,7 @@ async function probeRuntime(publicUrl: string): Promise { } } -async function roomServe(argv: string[], context: CliContext): Promise { +async function roomServe(argv: string[], context: CliContext, hooks: RoomCommandHooks = {}): Promise { const args = parseArgs(argv); const current = await readCurrent(context.home); const currentUrl = new URL(current.baseUrl); @@ -920,9 +932,12 @@ async function roomServe(argv: string[], context: CliContext): Promise { // wait commands; otherwise keep advertising the local serve URL. publicBaseUrl: () => readPublicBaseUrl(context.home, current.roomId) ?? localBaseUrl }); - await new Promise((resolve) => { - server.listen(port, host, resolve); - }); + const listen = hooks.listen ?? listenOrError; + const outcome = await listen(server, port, host); + if (!outcome.ok) { + context.stderr.write(`${listenErrorMessage(host, port, outcome.error)}\n`); + return 1; + } await writeCurrent(context.home, { ...current, baseUrl: localBaseUrl }); // Sharp guardrail (#180): --allow-remote assumes HTTPS is terminated at the // edge (tunnel/reverse proxy). Warn loudly so nobody exposes plain HTTP or diff --git a/test/cli-listen-errors.test.ts b/test/cli-listen-errors.test.ts new file mode 100644 index 0000000..81557b0 --- /dev/null +++ b/test/cli-listen-errors.test.ts @@ -0,0 +1,194 @@ +import assert from "node:assert/strict"; +import { mkdtemp } from "node:fs/promises"; +import { createServer, type Server } from "node:http"; +import { AddressInfo, createServer as createNetServer } from "node:net"; +import os from "node:os"; +import path from "node:path"; +import { Writable } from "node:stream"; +import test from "node:test"; +import type { CliContext } from "../src/cli/context.js"; +import { runBrokerCommand } from "../src/cli/commands/broker/index.js"; +import { runPlatformCommand } from "../src/cli/commands/platform/index.js"; +import { runRoomCommand } from "../src/cli/commands/room/index.js"; +import { listenErrorMessage, listenOrError, type ListenOutcome } from "../src/cli/commands/listen.js"; + +class Capture extends Writable { + chunks: string[] = []; + _write(chunk: Buffer | string, _e: BufferEncoding, cb: (error?: Error | null) => void): void { + this.chunks.push(Buffer.isBuffer(chunk) ? chunk.toString("utf8") : chunk); + cb(); + } + text(): string { + return this.chunks.join(""); + } +} + +async function makeContext(): Promise<{ context: CliContext; out: Capture; err: Capture }> { + const out = new Capture(); + const err = new Capture(); + const context: CliContext = { + home: await mkdtemp(path.join(os.tmpdir(), "agentgather-listen-test-")), + stdout: out, + stderr: err + }; + return { context, out, err }; +} + +// Occupy a real localhost port for the duration of a test so a serve command hits +// a genuine EADDRINUSE. Returns the port and a close function. +async function occupyPort(): Promise<{ port: number; server: Server; close: () => Promise }> { + const server = createServer((_req, res) => res.end("ok")); + await new Promise((resolve) => server.listen(0, "127.0.0.1", resolve)); + const { port } = server.address() as AddressInfo; + return { port, server, close: () => new Promise((resolve) => server.close(() => resolve())) }; +} + +function isListening(server: Server): boolean { + return server.listening; +} + +// A deterministic injected listen error — no OS hostname resolution — for the +// invalid-bind path. +function injectListenError(code: string): { listen: (s: Server, p: number, h: string) => Promise } { + return { + listen: async () => ({ ok: false, error: Object.assign(new Error("listen failed"), { code }) }) + }; +} + +const TOKEN_MARKERS = /tgl_|token=|Bearer|Authorization/i; + +// ---- shared helper (the seam launch/#232 now delegates to) ----------------- + +test("listenOrError resolves ok on a free port and failure on an occupied one, without throwing", async () => { + const free = await new Promise((resolve) => { + const s = createNetServer(); + s.listen(0, "127.0.0.1", () => { + const { port } = s.address() as AddressInfo; + s.close(() => resolve(port)); + }); + }); + + const okServer = createServer((_req, res) => res.end()); + const ok = await listenOrError(okServer, free, "127.0.0.1"); + assert.equal(ok.ok, true); + await new Promise((resolve) => okServer.close(() => resolve())); + + const occupied = await occupyPort(); + const clash = createServer((_req, res) => res.end()); + try { + const outcome = await listenOrError(clash, occupied.port, "127.0.0.1"); + assert.equal(outcome.ok, false); + assert.equal(outcome.error?.code, "EADDRINUSE"); + assert.equal(clash.listening, false); + } finally { + await occupied.close(); + } +}); + +test("listenErrorMessage is controlled and token-free for each bind failure", () => { + const inUse = listenErrorMessage("127.0.0.1", 8799, Object.assign(new Error(), { code: "EADDRINUSE" })); + assert.match(inUse, /127\.0\.0\.1:8799/); + assert.match(inUse, /already in use/); + assert.equal(TOKEN_MARKERS.test(inUse), false); + + assert.match(listenErrorMessage("127.0.0.1", 1, Object.assign(new Error(), { code: "EACCES" })), /permission denied/); + assert.match( + listenErrorMessage("127.0.0.1", 8788, Object.assign(new Error(), { code: "EADDRNOTAVAIL" })), + /not available/ + ); + assert.match(listenErrorMessage("127.0.0.1", 8787, undefined), /Cannot bind 127\.0\.0\.1:8787/); +}); + +// ---- broker serve ---------------------------------------------------------- + +test("broker serve: occupied port exits non-zero with a token-free error and never touches the foreign listener", async () => { + const { context, out, err } = await makeContext(); + const occupied = await occupyPort(); + try { + const code = await runBrokerCommand(["serve", "--port", String(occupied.port)], context); + assert.equal(code, 1); + assert.match(err.text(), new RegExp(`Cannot bind 127\\.0\\.0\\.1:${occupied.port}: address already in use`)); + assert.equal(TOKEN_MARKERS.test(err.text()), false); + assert.equal(out.text().includes("broker serving"), false); // never announced a bind + assert.equal(isListening(occupied.server), true); // foreign listener untouched + } finally { + await occupied.close(); + } +}); + +test("broker serve: an injected invalid-bind error exits non-zero, token-free (no hostname dependency)", async () => { + const { context, err } = await makeContext(); + const code = await runBrokerCommand( + ["serve", "--port", "8799"], + context, + injectListenError("EADDRNOTAVAIL") + ); + assert.equal(code, 1); + assert.match(err.text(), /Cannot bind 127\.0\.0\.1:8799: address not available/); + assert.equal(TOKEN_MARKERS.test(err.text()), false); +}); + +// ---- platform serve -------------------------------------------------------- + +test("platform serve: occupied port exits non-zero, token-free, foreign listener untouched", async () => { + const { context, out, err } = await makeContext(); + const occupied = await occupyPort(); + try { + const code = await runPlatformCommand(["serve", "--port", String(occupied.port)], context); + assert.equal(code, 1); + assert.match(err.text(), new RegExp(`Cannot bind 127\\.0\\.0\\.1:${occupied.port}: address already in use`)); + assert.equal(TOKEN_MARKERS.test(err.text()), false); + assert.equal(out.text().includes("Serving the control-plane"), false); + assert.equal(isListening(occupied.server), true); + } finally { + await occupied.close(); + } +}); + +test("platform serve: an injected invalid-bind error exits non-zero, token-free", async () => { + const { context, err } = await makeContext(); + const code = await runPlatformCommand( + ["serve", "--port", "8788"], + context, + injectListenError("EADDRNOTAVAIL") + ); + assert.equal(code, 1); + assert.match(err.text(), /Cannot bind 127\.0\.0\.1:8788: address not available/); + assert.equal(TOKEN_MARKERS.test(err.text()), false); +}); + +// ---- room serve ------------------------------------------------------------ + +async function startRoom(context: CliContext): Promise { + const code = await runRoomCommand(["start", "svc-room", "--alias", "operator", "--json"], context); + assert.equal(code, 0); +} + +test("room serve: occupied port exits non-zero, token-free, foreign listener untouched", async () => { + const { context, out, err } = await makeContext(); + await startRoom(context); + const occupied = await occupyPort(); + try { + const code = await runRoomCommand(["serve", "--port", String(occupied.port)], context); + assert.equal(code, 1); + assert.match(err.text(), new RegExp(`Cannot bind 127\\.0\\.0\\.1:${occupied.port}: address already in use`)); + assert.equal(TOKEN_MARKERS.test(err.text()), false); + assert.equal(out.text().includes("Serving svc-room"), false); + assert.equal(isListening(occupied.server), true); + } finally { + await occupied.close(); + } +}); + +test("room serve: an injected invalid-bind error exits non-zero, token-free", async () => { + const { context, err } = await makeContext(); + await startRoom(context); + const code = await runRoomCommand( + ["serve", "--port", "8787"], + context, + injectListenError("EADDRNOTAVAIL") + ); + assert.equal(code, 1); + assert.match(err.text(), /Cannot bind 127\.0\.0\.1:8787: address not available/); + assert.equal(TOKEN_MARKERS.test(err.text()), false); +});