diff --git a/packages/cli/src/commands/preview.ts b/packages/cli/src/commands/preview.ts index 1c297427f4..826c8af05e 100644 --- a/packages/cli/src/commands/preview.ts +++ b/packages/cli/src/commands/preview.ts @@ -262,8 +262,17 @@ export default defineCommand({ console.log("\n No active preview servers to kill.\n"); return; } - const killed = await killActiveServers(startPort); - console.log(`\n Killed ${killed} preview server${killed === 1 ? "" : "s"}.\n`); + const { killed, unverified } = await killActiveServers(startPort); + console.log(`\n Killed ${killed} preview server${killed === 1 ? "" : "s"}.`); + if (unverified.length > 0) { + clack.log.warn( + `Left ${unverified.length} server${unverified.length === 1 ? "" : "s"} alone ` + + `(port${unverified.length === 1 ? "" : "s"} ${unverified.join(", ")}): the OS could ` + + `not confirm which process owns the socket, and the server's own claim is not proof. ` + + `Install lsof, or stop it with its own preview --stop.`, + ); + } + console.log(); return; } @@ -287,14 +296,6 @@ export default defineCommand({ ); } - // Kill orphaned chrome-headless-shell processes from previous crashed sessions. - const orphansKilled = killOrphanedProcesses(); - if (orphansKilled > 0) { - console.log( - ` ${c.dim(`Cleaned up ${orphansKilled} orphaned process${orphansKilled === 1 ? "" : "es"} from a previous session.`)}`, - ); - } - const rawArg = args.dir; const isImplicitCwd = !rawArg || rawArg === "." || rawArg === "./"; const project = resolveProject(rawArg); @@ -352,6 +353,17 @@ export default defineCommand({ // modes all receive identical --proxy/--no-proxy + config semantics. const autoProxy = resolveAutoProxy(dir, args.proxy as boolean | undefined); + // Kill orphaned chrome-headless-shell processes from previous crashed + // sessions. Deliberately last: this 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. + const orphansKilled = killOrphanedProcesses(); + if (orphansKilled > 0) { + console.log( + ` ${c.dim(`Cleaned up ${orphansKilled} orphaned process${orphansKilled === 1 ? "" : "es"} from a previous session.`)}`, + ); + } + if (isDevMode()) { if (args.background) { clack.log.error("--background currently supports the embedded preview server only"); @@ -969,8 +981,8 @@ async function runDevMode(dir: string, options?: StudioLaunchOptions): Promise` only targets this process — the child tree (Vite + Chrome) // would survive without explicit cleanup. - // On Windows, killProcessTree is a no-op (pgrep/ps unavailable); Ctrl+C - // propagates via the console process group instead. + // On Windows, killProcessTree delegates to taskkill's tree mode, which force + // kills the whole tree immediately — no grace period, unlike the POSIX path. registerChildTreeShutdown(child); return waitForChildClose(child); } diff --git a/packages/cli/src/server/portUtils.test.ts b/packages/cli/src/server/portUtils.test.ts index 4c6ee962fd..e21a9f6a2d 100644 --- a/packages/cli/src/server/portUtils.test.ts +++ b/packages/cli/src/server/portUtils.test.ts @@ -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"; diff --git a/packages/cli/src/server/portUtils.ts b/packages/cli/src/server/portUtils.ts index ee9cb6f4a9..44de5a60c4 100644 --- a/packages/cli/src/server/portUtils.ts +++ b/packages/cli/src/server/portUtils.ts @@ -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 { - 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 { } } +async function windowsListenerPid(port: number): Promise { + 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 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 Promise = getProcessOnPort, +): Promise { + 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 { + 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 { +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( diff --git a/packages/cli/src/utils/orphanCleanup.test.ts b/packages/cli/src/utils/orphanCleanup.test.ts index b3dc2afdf2..aebe4d0b6c 100644 --- a/packages/cli/src/utils/orphanCleanup.test.ts +++ b/packages/cli/src/utils/orphanCleanup.test.ts @@ -1,13 +1,52 @@ import { describe, it, expect } from "vitest"; import { spawn } from "node:child_process"; -import { killProcessTree, killOrphanedProcesses } from "./orphanCleanup.js"; +import { + isProcessDescendant, + killProcessTree, + killOrphanedProcesses, + processIdentity, + windowsProcessTreeKillArgs, +} from "./orphanCleanup.js"; const IS_UNIX = process.platform !== "win32"; +describe("Windows process-tree cleanup", () => { + it("uses taskkill recursively and forcefully for the owned PID", () => { + expect(windowsProcessTreeKillArgs(4321)).toEqual(["/PID", "4321", "/T", "/F"]); + }); +}); + +describe("process-tree ownership", () => { + it("captures a stable birth token for the current process", () => { + const first = processIdentity(process.pid); + expect(first).toMatch(/^(?:linux|posix|windows):/); + expect(processIdentity(process.pid)).toBe(first); + expect(processIdentity(-1)).toBeNull(); + }); + + it("proves ancestry through every intermediate wrapper", () => { + const parents = new Map([ + [400, 300], + [300, 200], + [200, 1], + ]); + + expect(isProcessDescendant(400, 200, (pid) => parents.get(pid) ?? null)).toBe(true); + expect(isProcessDescendant(400, 999, (pid) => parents.get(pid) ?? null)).toBe(false); + }); + + it("fails closed on missing or cyclic process metadata", () => { + expect(isProcessDescendant(400, 200, () => null)).toBe(false); + expect(isProcessDescendant(400, 200, (pid) => (pid === 400 ? 300 : 400))).toBe(false); + }); +}); + describe.skipIf(!IS_UNIX)("killProcessTree", () => { it("kills a process and all its children", async () => { // Spawn a parent that spawns two sleeping children - const parent = spawn("bash", ["-c", "sleep 60 & sleep 60 & wait"], { stdio: "ignore" }); + const parent = spawn("bash", ["-c", "sleep 60 & sleep 60 & wait"], { + stdio: "ignore", + }); // Let children spawn await new Promise((r) => setTimeout(r, 200)); @@ -27,7 +66,9 @@ describe.skipIf(!IS_UNIX)("killProcessTree", () => { it("escalates to SIGKILL after grace period", async () => { // Spawn a process that traps SIGTERM - const proc = spawn("bash", ["-c", "trap '' TERM; sleep 60"], { stdio: "ignore" }); + const proc = spawn("bash", ["-c", "trap '' TERM; sleep 60"], { + stdio: "ignore", + }); await new Promise((r) => setTimeout(r, 100)); const exitPromise = new Promise((resolve) => proc.on("close", resolve)); diff --git a/packages/cli/src/utils/orphanCleanup.ts b/packages/cli/src/utils/orphanCleanup.ts index a34f01775c..146dd75eec 100644 --- a/packages/cli/src/utils/orphanCleanup.ts +++ b/packages/cli/src/utils/orphanCleanup.ts @@ -1,4 +1,5 @@ -import { execSync } from "node:child_process"; +import { execFileSync, execSync } from "node:child_process"; +import { readFileSync } from "node:fs"; /** * Find and kill orphaned Chrome processes from previous crashed sessions. @@ -34,11 +35,27 @@ export function killOrphanedProcesses(): number { * depth-first so children are killed before parents, preventing * re-adoption races. * - * No-op on Windows — process groups are managed differently and - * the pgrep/ps utilities are not available. + * Windows uses taskkill's tree mode because pgrep/ps are unavailable there. + * + * `signal` is honoured on POSIX only. The Windows path always passes `/F`, so a + * caller asking for SIGTERM gets a forced tree kill with no grace period, while + * the same call on POSIX gets 500 ms to flush and exit. That is deliberate — + * `taskkill` without `/F` posts WM_CLOSE, which a console process is free to + * ignore, and leaving a preview server alive is the worse failure here. Do not + * pass SIGTERM expecting a clean shutdown on Windows. */ export function killProcessTree(pid: number, signal: NodeJS.Signals = "SIGTERM"): void { - if (process.platform === "win32") return; + if (process.platform === "win32") { + try { + execFileSync("taskkill", windowsProcessTreeKillArgs(pid), { + stdio: "ignore", + timeout: 5000, + }); + } catch { + // Process already exited or taskkill could not inspect it. + } + return; + } const descendants = getDescendants(pid); const allPids = [...descendants.reverse(), pid]; @@ -65,10 +82,111 @@ export function killProcessTree(pid: number, signal: NodeJS.Signals = "SIGTERM") } } +export function windowsProcessTreeKillArgs(pid: number): string[] { + return ["/PID", String(pid), "/T", "/F"]; +} + +/** + * Return a process birth token suitable for detecting PID reuse. The token is + * diagnostic state only: callers must still prove the live server is a + * descendant before treating a saved wrapper as the owned process-tree root. + */ +export function processIdentity(pid: number): string | null { + if (!Number.isInteger(pid) || pid <= 0) return null; + try { + if (process.platform === "win32") { + const created = execFileSync( + "powershell.exe", + [ + "-NoProfile", + "-NonInteractive", + "-Command", + `(Get-CimInstance Win32_Process -Filter 'ProcessId = ${pid}').CreationDate.ToFileTimeUtc()`, + ], + { encoding: "utf8", timeout: 2000 }, + ).trim(); + return created ? `windows:${created}` : null; + } + + if (process.platform === "linux") { + const stat = readFileSync(`/proc/${pid}/stat`, "utf8"); + const fields = stat + .slice(stat.lastIndexOf(") ") + 2) + .trim() + .split(/\s+/); + const startTicks = fields[19]; // field 22 overall; fields starts at process state (3) + return startTicks ? `linux:${startTicks}` : null; + } + + const started = execFileSync("ps", ["-o", "lstart=", "-p", String(pid)], { + encoding: "utf8", + timeout: 2000, + }).trim(); + return started ? `posix:${started}` : null; + } catch { + return null; + } +} + +type ParentPidLookup = (pid: number) => number | null; + +function processParentPid(pid: number): number | null { + try { + const output = + process.platform === "win32" + ? execFileSync( + "powershell.exe", + [ + "-NoProfile", + "-NonInteractive", + "-Command", + `(Get-CimInstance Win32_Process -Filter 'ProcessId = ${pid}').ParentProcessId`, + ], + { encoding: "utf8", timeout: 2000 }, + ) + : execFileSync("ps", ["-o", "ppid=", "-p", String(pid)], { + encoding: "utf8", + timeout: 2000, + }); + const parentPid = Number(output.trim()); + return Number.isInteger(parentPid) && parentPid > 0 ? parentPid : null; + } catch { + return null; + } +} + +/** + * Prove that `childPid` currently belongs to the process tree rooted at + * `ancestorPid`. The walk fails closed on missing, invalid, or cyclic process + * metadata so a stale saved PID can never authorize terminating a new process. + */ +export function isProcessDescendant( + childPid: number, + ancestorPid: number, + parentPid: ParentPidLookup = processParentPid, +): boolean { + if (childPid <= 0 || ancestorPid <= 0 || childPid === ancestorPid) return false; + + const visited = new Set(); + let current = childPid; + for (let depth = 0; depth < 64; depth++) { + if (visited.has(current)) return false; + visited.add(current); + const parent = parentPid(current); + if (parent === ancestorPid) return true; + if (parent === null || parent <= 1) return false; + current = parent; + } + return false; +} + function getDescendants(pid: number): number[] { let children: number[]; try { - const raw = execSync(`pgrep -P ${pid}`, { encoding: "utf-8", timeout: 2000 }).trim(); + const raw = execSync(`pgrep -P ${pid}`, { + encoding: "utf-8", + timeout: 2000, + }).trim(); if (!raw) return []; children = raw .split("\n")