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

[miguel-heygen]

/__hyperframes_config is unauthenticated, and the PID it reports is what --stop and --kill-all send signals to. Any local process answering on a scanned port could name an arbitrary PID and have the CLI kill it.

Reproduced

Start an unrelated process, then a twenty-line HTTP server on a scanned port that answers /__hyperframes_config with that unrelated PID, then run preview --kill-all. Before this change the unrelated process is killed. After it, the unrelated process survives and only the real listener is stopped.

The fix

The listening PID now comes from the OS — lsof, and netstat -ano 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 Chrome 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.

Stack

First of four on the preview side, based on main. u4b-preview-session-ownership builds on it.

[jrusso1020]

Reviewed at 5f426778b25 (full files, not just the diff). This is a draft, so nothing below is a merge gate — but the central idea is right and I think one gap undercuts it, so it is worth raising before the stack builds on top.

The thesis is good: /__hyperframes_config is unauthenticated, --stop / --kill-all signal whatever PID that response names, and the docstring you added on activeServerOnPort states the threat exactly. The activeServerOnPort test is the right construction too — the fake server claims pid: 999_999 and the assertion is String(process.pid), so it fails under the old behaviour rather than merely passing under the new one.

1. The two halves of this PR pick opposite failure directions for the same condition

This is the one I would want resolved before the stack lands on it, because the PR's own title is the claim it weakens.

Both new code paths depend on the OS being able to answer a question about a process, and both handle "it can't" — in opposite directions:

• isProcessDescendant fails CLOSED. processParentPid returns null on any failure, the walk returns false, and the docstring says so deliberately: "fails closed on missing, invalid, or cyclic process metadata so a stale saved PID can never authorize terminating a new process." Correct.
• activeServerOnPort fails OPEN. When getProcessOnPort returns null it falls back to config.pid — the self-reported value the same file describes as "whatever the process on the other end chose to say."

getProcessOnPort returns null for more than "unsupported platform". On Linux and macOS it is a try/catch around lsof, so it also returns null when lsof is not installed — which is the default on a lot of slim container images — as well as on timeout, and when lsof cannot see a socket owned by another user. On such a machine every scanned port falls back to the self-reported PID, --kill-all is back to exactly the pre-PR behaviour, and nothing tells the user the check is not running.

The docstring's defence is that the fallback is "the only case where it is unfalsifiable." That is true and I think it argues the other way: unfalsifiable is a reason to distrust a value, not a reason to act on it. A process that wants to be killed-by-proxy does not need lsof to be missing for its own sake — it just needs to be running somewhere lsof is missing.

I do not think the answer is obviously "fail closed", because that trades a real availability property (--kill-all stops working entirely on those machines) for a fairly exotic threat. But the choice should be explicit rather than incidental:

• decide it deliberately and say which you picked in the docstring, and
• when the fallback is taken, warn on the path that actually signals — something like "could not confirm via the OS which process owns port N; skipping" or "…trusting the server's self-reported PID" — so the degraded mode is visible, and
• add the missing test. The current one only covers the lsof-present path, so the fallback branch — the one with the security consequence — has no coverage at all. Injecting the lookup the way testPortOnAllHosts already injects probe, and the way isProcessDescendant injects parentPid, would make it a two-line test. Both patterns are already in this codebase.

2. signal is silently ignored on Windows, and always escalates to force

killProcessTree(pid, signal = "SIGTERM") now has two very different behaviours:

	POSIX	Windows
first action	process.kill(p, signal) per pid, children first	taskkill /PID n /T /F
grace period	500 ms, then SIGKILL for survivors	none
honours signal	yes	no — always /F

/F is the forced kill. So a caller asking for SIGTERM — which is the parameter's whole purpose, and what both production callers pass (preview.ts:901, previewLifecycle.ts:117) — gets no chance to shut down cleanly on Windows. On POSIX the same call gets 500 ms to flush and exit.

Going from "no-op" to "forced tree kill" is still a clear improvement, so this is not a regression. But the signature promises something it does not deliver on one platform. Either map it (/F only when signal === "SIGKILL", plain taskkill /PID n /T otherwise, which asks politely first) or state in the docstring that Windows intentionally ignores the signal and always forces, so the next caller does not pass SIGTERM expecting graceful teardown.

3. preview.ts:975 now documents the opposite of what the code does

// On Windows, killProcessTree is a no-op (pgrep/ps unavailable); Ctrl+C
// propagates via the console process group instead.
registerChildTreeShutdown(child);

That was true before this PR and is false after it. You updated the docstring at the definition ("Windows uses taskkill's tree mode") and this caller-side comment did not move with it.

Nothing is gated on it, so this is documentation only — but it is the kind that costs later: it tells the next reader Windows cleanup does not happen, which invites a redundant Windows-specific workaround, and it hides the thing they would actually want to know from §2, that the Windows path now force-kills the tree with no grace period.

4. Three of the new exports have no production callers yet

processIdentity, isProcessDescendant and windowsProcessTreeKillArgs are referenced only from orphanCleanup.test.ts. killProcessTree and killOrphanedProcesses are genuinely wired.

Entirely reasonable for the first PR of a stack, and I am not asking you to wire them here. Worth stating plainly so nobody reads the diff and concludes PID-reuse detection is live: right now processIdentity's contract — "callers must still prove the live server is a descendant before treating a saved wrapper as the owned process-tree root" — has no callers to bind, so it is a promise about code that does not exist yet. When the rung that consumes it lands, that is the sentence to check it against.

Checked and fine — recorded so nobody re-checks

• The /proc/[pid]/stat parse is correct, including the part that usually is not. Slicing on lastIndexOf(") ") is the right way to get past a comm that itself contains parentheses or spaces, and after that slice fields[19] really is field 22 (starttime), because the remainder begins at field 3 (state). The inline comment saying so is accurate.
• isProcessDescendant's walk is properly bounded — visited set for cycles, 64-iteration cap, and parent <= 1 terminates at init rather than walking into it. It also rejects childPid === ancestorPid, so a saved PID cannot authorize killing itself as its own tree root.
• Moving killOrphanedProcesses() later in preview.ts is a real fix, not a cosmetic reorder. There are four early returns between the old call site and the new one (the --user-data-dir check, the dependency error, and two others), so previously an invocation that failed validation and started nothing would still reach out and kill other processes. The comment you added explains exactly this, and it is worth keeping in that shape.
• This draft did get the full matrix. All 8 required contexts on main are present and green at this head — Build, Test, Typecheck, Test: runtime contract, regression, Semantic PR title, and both Windows lanes (Tests on windows-latest, Render on windows-latest). Only WIP is pending, which is the draft marker itself. Worth saying because a draft's checks are often skipped rather than run, and skipped would have meant no signal on precisely the platform §2 is about — here they actually ran.

Two nits

• The Windows guard in the new test silently passes instead of skipping. portUtils.test.ts:170 uses if (process.platform === "win32") return;, which reports green on Windows as though the assertion ran. orphanCleanup.test.ts in this same PR gets it right with describe.skipIf(!IS_UNIX). Worth matching the idiom, since this is a security assertion and "passed" versus "not applicable" are different facts on the one platform where the PID lookup takes a different code path entirely.
• Pre-existing, mentioned only because hardening is this PR's subject: orphanCleanup.ts still builds four shell command strings via interpolation (pgrep -P ${pid}, pgrep ${userFlag}-f ${processName}, ps -p ${pid} -o ppid=, id -u) while everything you added uses execFileSync with an argv array. Not exploitable today — the inputs are numbers and a hardcoded name list — and not yours to fix in this PR. Flagging it because a later rung passing a user-supplied name into killOrphansByName would turn the second one into a real hole, and the argv-array habit you have established here is what prevents it.

Verdict

COMMENTED. It is a draft, so there is nothing to approve and I would not stamp it in this state regardless. No objection to the direction — §1 is the one I would settle before the rest of the stack depends on this being a trust boundary, and §2 and §3 are small.

Note: /code-review max can't be invoked from my side, so this is that lens applied by hand rather than a lighter review silently substituted.

— Rames Jusso

[terencecho]

Reviewed at 5f426778b25. Draft, so no stamp — comment only. CI is green at head (every required check SUCCESS; only WIP is pending, which is the draft marker itself).

The core move is right: /__hyperframes_config is unauthenticated and --stop / --kill-all send signals to a field in its response, so preferring the OS's answer is the correct fix, and the portUtils.test.ts:165 test pins it well.

---

Concur with @jrusso1020's review

I verified all four independently and reached the same conclusions:

1. Fail-open vs. fail-closed asymmetry — confirmed. orphanCleanup.ts:156-179 fails closed on missing metadata; portUtils.ts:332-335 falls back to config.pid whenever getProcessOnPort returns null. Sharpening one point: the docstring at portUtils.ts:326-327 claims the self-reported value "is used only where the OS lookup is unavailable, which is also the only case where it is unfalsifiable." That isn't accurate. getProcessOnPort returns null for any failure — lsof absent from PATH, the 2 s timeout at :209 firing under load, a socket owned by another uid, a container without procfs tooling. In every one of those cases the OS answer is merely unobtained, while the attacker's HTTP server is still answering freely. The value isn't unfalsifiable there; it's just unchecked.
2. signal accepted then ignored on Windows — confirmed, orphanCleanup.ts:40-51. /F is hardcoded at :78. Worth adding that the win32 branch returns at :50 and so also skips the SIGTERM→SIGKILL escalation ladder at :63-75 — Windows loses graceful shutdown entirely, not just the parameter.
3. Doc drift — confirmed, preview.ts:975 still reads "On Windows, killProcessTree is a no-op (pgrep/ps unavailable)". Exactly inverted by this PR.
4. killOrphanedProcesses() move — confirmed and agreed, this is a real fix. preview.ts:287 → :351 now sits after resolveProject, the dir checks, and the arg validation, so a hyperframes preview /nonexistent no longer reaches outside the process and kills other people's PIDs on its way to an error.

---

Additional findings

1. isProcessDescendant and processIdentity are dead code — the invariant they document is enforced nowhere

git grep across the head shows both are referenced only by orphanCleanup.test.ts. No production call site:

isProcessDescendant  → orphanCleanup.ts:156 (def), orphanCleanup.test.ts (4 refs)
processIdentity      → orphanCleanup.ts:87  (def), orphanCleanup.test.ts (3 refs)

That matters because the docstrings assert guarantees the codebase does not have. orphanCleanup.ts:151-155: "a stale saved PID can never authorize terminating a new process." orphanCleanup.ts:81-84: "callers must still prove the live server is a descendant before treating a saved wrapper as the owned process-tree root." There are no such callers. A reader (or a future reviewer) grepping for PID-reuse protection will find these, read the contract, and conclude the kill path is guarded when it isn't.

Either wire them into the path below, or drop them from this PR and land them with their consumer.

2. The call site those helpers were written for still fails open — through a third trust tier

previewLifecycle.ts:265 (stopBackgroundPreview, the --stop path):

// A saved PID can be reused after a crashed preview, so only trust it while
// a currently reachable server proves this exact project is still running.
const pid = Number(server ? (server.pid ?? saved?.pid) : undefined);
...
(dependencies.kill ?? stopProcess)(pid);

Stacked with finding #1 above, --stop now degrades through three tiers of decreasing trustworthiness before it signals:

1. server.pid from the OS (getProcessOnPort) — trustworthy, the point of this PR;
2. → config.pid, self-reported over an unauthenticated endpoint (portUtils.ts:334-335);
3. → saved?.pid, read off disk from ~/.local/state/hyperframes/previews/*.json, written at spawn time and never re-validated.

The comment's reasoning doesn't hold for tier 3. A reachable server matching projectDir proves a server for this project is alive; it does not prove saved.pid is that server. If the original preview crashed and the PID was recycled while an unrelated hyperframes preview for the same dir came up on another port, matchingServer is satisfied and the recycled PID gets a tree-kill. Same for readBackgroundPreviewStatus at :190.

This is precisely the scenario isProcessDescendant's docstring describes, and it's the natural home for it: before falling back to saved.pid, require isProcessDescendant(listenerPid, saved.pid) — or drop the fallback and return false. One caveat if you wire it: isProcessDescendant bails at parent <= 1 (:174), so inside a container the detached preview reparents to PID 1 the moment the wrapper exits and ancestry proof fails permanently. Fail-closed is the right default, but --stop would stop working in Docker; worth deciding deliberately.

3. stopProcess's Windows follow-up is now a PID-reuse window, not just redundancy

previewLifecycle.ts:116-125:

function stopProcess(pid: number): void {
  killProcessTree(pid);
  if (process.platform === "win32") {
    try { process.kill(pid, "SIGTERM"); } catch {}
  }
}

Before this PR that win32 branch was load-bearing — killProcessTree returned early on Windows, so this was the only kill. Now killProcessTree runs taskkill /PID <pid> /T /F synchronously and force-kills the whole tree first. The follow-up therefore signals a PID that taskkill just freed a moment ago, which Windows is free to hand to a new process. Narrow window, but it's the same failure class this PR exists to close, and it's now pure downside. Delete the branch.

4. windowsListenerPid string-matches a localized netstat column

portUtils.ts:223:

if (columns.length < 5 || columns[3] !== "LISTENING") continue;

netstat's state column is localized on non-English Windows installations — ABHÖREN (de), À L'ÉCOUTE (fr), ESCUCHANDO (es), and so on. On any of those machines this comparison never matches, the function returns null for every port, and activeServerOnPort silently drops to the self-reported config.pid. That's exactly the fail-open from finding #1, except permanent and invisible for an entire class of users — the very machines the new Windows support was added for. (Worth confirming against a real localized box, but the locale-dependence of netstat output is well established.)

Two ways out: match on row shape rather than a translated word (columns[0] === "TCP" + local port match + numeric final column), or skip netstat and use Get-NetTCPConnection -LocalPort <n> -State Listen | Select -Expand OwningProcess, which returns structured, locale-independent data — and you already shell out to powershell.exe elsewhere in orphanCleanup.ts:90-101.

5. Both listener lookups silently collapse multiple listeners to one

windowsListenerPid (portUtils.ts:222-228) returns on the first row whose local-address port matches, never inspecting the local host. 127.0.0.1:3002 and [::]:3002 can be held by two different processes; whichever netstat prints first is the one that gets signalled. The Linux path has the same shape at :210 — lsof -ti:PORT can emit several PIDs and .split("\n")[0] discards the rest without comment.

ActiveServer already carries the loopback host the server was reached on. Filtering the candidate rows on it, or at minimum returning null when the lookup is ambiguous rather than guessing, would keep this consistent with the PR's own fail-closed stance.

6. Test coverage only reaches the branch that was already safe

portUtils.test.ts:165-183 is a good test, but note what it doesn't reach:

• The fallback branch has no coverage (@jrusso1020 flagged this; confirming). Nothing exercises getProcessOnPort → null, which is the branch that carries all the residual risk.
• windowsListenerPid has zero coverage. The test returns at :171 on win32 before asserting anything, so Tests on windows-latest is green here by no-op, not by verification. The new netstat parser — the most fiddly code in the PR — ships untested on the only platform that runs it.

getProcessOnPort is module-private, so the fallback is only reachable by stubbing execFile. Cleaner: pull the netstat and lsof line parsing into pure exported functions and table-test them against captured output — the same shape as windowsProcessTreeKillArgs, which is exactly why that one is easy to test at orphanCleanup.test.ts:15. An English and a German netstat sample in that table would have caught #4 for free.

7. Minor — one full netstat per matching port

activeServerOnPort:332 calls getProcessOnPort unconditionally for every port that answers the config probe. The old code in scanActiveServers only called it when config.pid was invalid, i.e. almost never. On Linux that's an extra lsof per live server; on Windows each call dumps the entire TCP table via netstat -ano -p tcp with a 4 s timeout, uncached, once per server. With a handful of previews running, --kill-all does a handful of full table dumps. One netstat per scan, indexed by port, would fix it.

8. Adjacent, pre-existing — killActiveServers doesn't kill trees

portUtils.ts:350-366 sends a bare process.kill(pid, "SIGTERM") rather than killProcessTree, so --kill-all leaves the server's Chrome children behind. Pre-existing and out of scope, but the PR sharpens the contrast: it now takes care to identify the right PID and then signals only that one process, while orphanCleanup exists specifically because those descendants outlive their parents.

---

Happy to re-review once it's out of draft.

— Review by tai (pr-review)