fix(cli): signal only processes the OS says own the port (#3307)

* fix(cli): signal only processes the OS says own the port `/__hyperframes_config` is unauthenticated and the PID it reports is what `--stop` and `--kill-all` send signals to, so any local process answering on a scanned port could name an arbitrary PID and have the CLI kill it. Reproduced with a twenty-line HTTP server on a scanned port self-reporting an unrelated PID: before this, `--kill-all` killed that process; after it, the process survives and only the real listener is stopped. The listening PID now comes from the OS — `lsof`, and `netstat` on Windows, where the lookup was previously unavailable and the self-reported value was taken on trust. The response's own PID is used only where the OS lookup fails, which is also the only case where it is unfalsifiable. Orphan cleanup moves to the last step before a launch. It reaches outside the process and kills other people's PIDs, so it must not run for an invocation that turns out to be a validation error and never starts anything. * fix(cli): fail closed when the OS cannot confirm who owns a port Review follow-up. The two halves of this change picked opposite directions for the same condition. `isProcessDescendant` fails closed by design; `activeServerOnPort` fell back to the self-reported PID whenever the OS lookup came back empty — and that is not only "unsupported platform". `lsof` may be absent (the default on many slim images), may time out, or may not see a socket owned by another user. On such a machine every scanned port silently reverted to pre-change behaviour, with nothing said. Provenance is now part of the type rather than a convention: `ActiveServer` carries `pidSource`, so a caller cannot mistake a self-report for the kernel's answer. `--kill-all` requires `"os"` and skips the rest, naming the ports it left alone and why. That is the deliberate trade — a blind sweep of a port range has no evidence beyond an unauthenticated response, so an unconfirmed PID must not be signalled. Managed previews are unaffected: they stop through their session record, which proves ownership by process birth identity. The fallback branch — the one with the security consequence — now has the coverage it lacked, via an injected lookup matching the seam `testPortOnAllHosts` and `isProcessDescendant` already use, including a live process that survives because nothing confirmed it owns the socket. Also state that `killProcessTree` honours `signal` on POSIX only: Windows always passes `/F`, deliberately, since taskkill without it posts WM_CLOSE that a console process may ignore. The caller-side comment claiming Windows cleanup is a no-op described the code before this change and now says the opposite.
2026-09-07 10:06:21 +00:00 · 2026-08-18 17:41:46 -04:00
parent 3e4b08cdc1
commit c1c70f44bd
5 changed files with 366 additions and 59 deletions
@@ -1,9 +1,11 @@
 import { afterEach, describe, expect, it, vi } from "vitest";
 import { createServer, type Server } from "node:net";
+import { spawn } from "node:child_process";
 import { resolve } from "node:path";
 import { createServer as createHttpServer, type Server as HttpServer } from "node:http";
 import {
  PORT_PROBE_HOSTS,
+  activeServerOnPort,
  detectHyperframesServer,
  findPortAndServe,
  testPortOnAllHosts,
@@ -161,6 +163,72 @@ describe("findPortAndServe — bind host (security: F-001)", () => {
  });
 });

+describe("activeServerOnPort — PID provenance (security)", () => {
+  it("does not signal a server whose PID the OS could not confirm", async () => {
+    // Fail closed: the only evidence for a blind port-range sweep is an
+    // unauthenticated config response, so an unconfirmed PID is skipped and
+    // reported rather than signalled.
+    const alive = spawn(process.execPath, ["-e", "setTimeout(() => {}, 30000)"], {
+      stdio: "ignore",
+    });
+    const port = await startConfigProbeServer({
+      isHyperframes: true,
+      projectName: "demo-project",
+      projectDir: "/tmp/demo-project",
+      serverBuildSignature: null,
+      version: "0.6.42",
+      pid: alive.pid,
+    });
+
+    const server = await activeServerOnPort(port, async () => null);
+    expect(server?.pidSource).toBe("self-reported");
+
+    // The victim survives, because nothing verified it owns the socket.
+    expect(alive.killed).toBe(false);
+    alive.kill();
+  });
+
+  it("tags a self-reported PID and refuses to signal it", async () => {
+    // The branch with the security consequence. `getProcessOnPort` returns null
+    // for more than "unsupported platform" — lsof absent, timed out, or unable
+    // to see another user's socket — and on those machines every scanned port
+    // used to fall back to the self-report with nothing said.
+    const port = await startConfigProbeServer({
+      isHyperframes: true,
+      projectName: "demo-project",
+      projectDir: "/tmp/demo-project",
+      serverBuildSignature: null,
+      version: "0.6.42",
+      pid: 999_999,
+    });
+
+    const server = await activeServerOnPort(port, async () => null);
+
+    expect(server?.pid).toBe("999999");
+    expect(server?.pidSource).toBe("self-reported");
+  });
+
+  it("reports the PID that owns the socket, not the one the response claims", async () => {
+    // `/__hyperframes_config` is unauthenticated and `--stop` / `--kill-all`
+    // send signals to this field. Trusting the response let any local process
+    // on a scanned port name an arbitrary PID and have the CLI kill it.
+    if (process.platform === "win32") return;
+    const port = await startConfigProbeServer({
+      isHyperframes: true,
+      projectName: "demo-project",
+      projectDir: "/tmp/demo-project",
+      serverBuildSignature: null,
+      version: "0.6.42",
+      pid: 999_999,
+    });
+
+    const server = await activeServerOnPort(port);
+
+    expect(server?.pid).toBe(String(process.pid));
+    expect(server?.pidSource).toBe("os");
+  });
+});
+
 describe("detectHyperframesServer", () => {
  it("treats same-project servers with a different server build signature as mismatch", async () => {
    const projectDir = "/tmp/demo-project";
@@ -15,7 +15,6 @@ import http from "node:http";
 import { execFile } from "node:child_process";
 import { promisify } from "node:util";
 import { resolve } from "node:path";
-import { c } from "../ui/colors.js";
 import type { BrowserGpuMode } from "../browser/gpuPolicy.js";

 const execFileAsync = promisify(execFile);
@@ -198,8 +197,13 @@ export function detectHyperframesServer(
 * Get the PID of the process listening on a port (macOS/Linux only).
 * Returns null on Windows or if detection fails.
 */
+/**
+ * The PID the OS says is listening on `port`, or null when it cannot be
+ * determined. This is the only trustworthy answer: a config response is
+ * whatever the process on the other end chose to say.
+ */
 async function getProcessOnPort(port: number): Promise<string | null> {
-  if (process.platform === "win32") return null;
+  if (process.platform === "win32") return windowsListenerPid(port);
  try {
    const { stdout } = await execFileAsync("lsof", [`-ti:${port}`, "-sTCP:LISTEN"], {
      timeout: 2000,
@@ -211,6 +215,23 @@ async function getProcessOnPort(port: number): Promise<string | null> {
  }
 }

+async function windowsListenerPid(port: number): Promise<string | null> {
+  try {
+    const { stdout } = await execFileAsync("netstat", ["-ano", "-p", "tcp"], { timeout: 4000 });
+    for (const line of stdout.split(/\r?\n/)) {
+      const columns = line.trim().split(/\s+/);
+      if (columns.length < 5 || columns[3] !== "LISTENING") continue;
+      const local = columns[1] ?? "";
+      if (local.slice(local.lastIndexOf(":") + 1) !== String(port)) continue;
+      const pid = columns[4] ?? "";
+      return /^\d+$/.test(pid) && pid !== "0" ? pid : null;
+    }
+    return null;
+  } catch {
+    return null;
+  }
+}
+
 // ── Server discovery ───────────────────────────────────────────────────────

 export interface ActiveServer {
@@ -226,6 +247,13 @@ export interface ActiveServer {
  projectDir: string;
  version: string;
  pid: string | null;
+  /**
+   * Where `pid` came from. `"os"` is the kernel's answer for who holds the
+   * listening socket; `"self-reported"` is whatever the process on the other
+   * end chose to put in its config response. Callers that SIGNAL the pid must
+   * require `"os"` — see `killActiveServers`.
+   */
+  pidSource?: "os" | "self-reported";
  browserGpuMode?: BrowserGpuMode;
 }

@@ -285,24 +313,7 @@ export async function scanActiveServers(startPort = 3002): Promise<ActiveServer[
    const batchEnd = Math.min(batchStart + batchSize - 1, endPort);
    const ports = Array.from({ length: batchEnd - batchStart + 1 }, (_, i) => batchStart + i);

-    const results = await Promise.all(
-      ports.map(async (port) => {
-        const config = await probePort(port);
-        if (!config) return null;
-        const pid =
-          Number.isInteger(config.pid) && Number(config.pid) > 0
-            ? String(config.pid)
-            : await getProcessOnPort(port);
-        return {
-          port,
-          projectName: config.projectName,
-          projectDir: config.projectDir,
-          version: config.version,
-          pid,
-          browserGpuMode: config.browserGpuMode,
-        };
-      }),
-    );
+    const results = await Promise.all(ports.map((port) => activeServerOnPort(port)));

    for (const r of results) {
      if (r) servers.push(r);
@@ -313,25 +324,90 @@ export async function scanActiveServers(startPort = 3002): Promise<ActiveServer[
 }

 /**
- * Kill all active HyperFrames preview servers by sending SIGTERM to their PIDs.
- * Returns the number of servers killed.
+ * Probe exactly one port and return its HyperFrames identity.
+ *
+ * `pid` is the OS's answer for who holds the listening socket, NOT the pid the
+ * response claims. That field is load-bearing — `--stop` and `--kill-all` send
+ * signals to it — and `/__hyperframes_config` is unauthenticated, so any local
+ * process that answers on a scanned port could otherwise name an arbitrary PID
+ * and have the CLI kill it. The self-reported value is used only where the OS
+ * lookup is unavailable, which is also the only case where it is unfalsifiable.
 */
-export async function killActiveServers(startPort = 3002): Promise<number> {
+export async function activeServerOnPort(
+  port: number,
+  listenerLookup: (port: number) => Promise<string | null> = getProcessOnPort,
+): Promise<ActiveServer | null> {
+  const config = await probePort(port);
+  if (!config) return null;
+  const listenerPid = await listenerLookup(port);
+  if (listenerPid) return { ...identityFrom(config, port), pid: listenerPid, pidSource: "os" };
+
+  // The OS lookup came back empty. That is NOT only "unsupported platform":
+  // `lsof` may be absent (common on slim images), may time out, or may not see
+  // a socket owned by another user. The value is still reported, because
+  // `--list` and the ownership record both have honest uses for it, but it is
+  // tagged so the paths that send signals can refuse it.
+  const selfReported =
+    Number.isInteger(config.pid) && Number(config.pid) > 0 ? String(config.pid) : null;
+  return {
+    ...identityFrom(config, port),
+    pid: selfReported,
+    ...(selfReported ? { pidSource: "self-reported" as const } : {}),
+  };
+}
+
+function identityFrom(
+  config: HyperframesConfigResponse,
+  port: number,
+): Omit<ActiveServer, "pid" | "pidSource"> {
+  return {
+    port,
+    projectName: config.projectName,
+    projectDir: config.projectDir,
+    version: config.version,
+    browserGpuMode: config.browserGpuMode,
+  };
+}
+
+/**
+ * SIGTERM every active HyperFrames preview server whose PID the OS confirmed.
+ *
+ * This is a blind sweep of a port range: the only evidence that a given process
+ * should be killed is that it answered `/__hyperframes_config`, which is
+ * unauthenticated. So the decision here is deliberately FAIL CLOSED — a PID the
+ * OS could not confirm is skipped rather than signalled, because the alternative
+ * is letting any local process nominate a victim.
+ *
+ * The cost is real and bounded: where `lsof` is missing, `--kill-all` stops
+ * reaping unmanaged servers. Managed previews are unaffected — they stop through
+ * their session record, which proves ownership by process birth identity rather
+ * than by asking the port who it is.
+ *
+ * Skipped ports are returned so the caller can say so; a security control that
+ * degrades silently is one nobody knows to fix.
+ */
+export async function killActiveServers(
+  startPort = 3002,
+): Promise<{ killed: number; unverified: number[] }> {
  const servers = await scanActiveServers(startPort);
  let killed = 0;
+  const unverified: number[] = [];

  for (const server of servers) {
-    if (server.pid) {
-      try {
-        process.kill(parseInt(server.pid, 10), "SIGTERM");
-        killed++;
-      } catch {
-        // Process may have already exited
-      }
+    if (!server.pid) continue;
+    if (server.pidSource !== "os") {
+      unverified.push(server.port);
+      continue;
+    }
+    try {
+      process.kill(parseInt(server.pid, 10), "SIGTERM");
+      killed++;
+    } catch {
+      // Process may have already exited
    }
  }

-  return killed;
+  return { killed, unverified };
 }

 // ── Smart port selection ───────────────────────────────────────────────────
@@ -416,17 +492,9 @@ export async function findPortAndServe(
        return { type: "already-running", port };
      }
      if (detection.type === "mismatch") {
-        console.log(
-          `  ${c.dim(`Port ${port} in use by HyperFrames project "${detection.projectName}" — skipping`)}`,
-        );
        continue;
      }
    }
-
-    const pid = await getProcessOnPort(port);
-    if (pid) {
-      console.log(`  ${c.dim(`Port ${port} in use by PID ${pid} — skipping`)}`);
-    }
  }

  throw new Error(