From a4a87b70f4d865af53892733560b23b6dd23e792 Mon Sep 17 00:00:00 2001 From: luvs01 <27862058+luvs01@users.noreply.github.com> Date: Tue, 8 Sep 2026 23:01:37 +0900 Subject: [PATCH] fix(server): honor rejected identity checks during port reclaim --- .../docs/ko/reference/cli/lifecycle.md | 5 + .../content/docs/reference/cli/lifecycle.md | 5 + src/server/port-reclaim.ts | 34 ++---- structure/01_runtime.md | 6 + tests/server/port-reclaim.test.ts | 103 ++++++++++++------ 5 files changed, 91 insertions(+), 62 deletions(-) diff --git a/docs-site/src/content/docs/ko/reference/cli/lifecycle.md b/docs-site/src/content/docs/ko/reference/cli/lifecycle.md index d4c19ebd46..544907d2c8 100644 --- a/docs-site/src/content/docs/ko/reference/cli/lifecycle.md +++ b/docs-site/src/content/docs/ko/reference/cli/lifecycle.md @@ -34,6 +34,11 @@ ocx start --port 8080 백그라운드 서비스가 설치되어 있으면 `ocx stop`이 먼저 그 서비스를 중지하므로 프록시가 다시 올라올 수 없습니다. 웹 대시보드의 **Stop** 버튼도 같은 동작(`POST /api/stop`)을 하지만, Windows 작업 스케줄러는 예외입니다. 작업이 끝나도 래퍼가 프록시를 다시 띄울 수 있어서, 대시보드는 `respawnable_service`로 거절하고 아무것도 바꾸지 않은 채 `ocx stop` 실행을 안내합니다. +중지·업데이트 후 포트 회수 중에는 종료 전에 기록한 PID라도 OCX 프로세스 확인 실패를 무시하지 않습니다. +확인이 거부된 살아 있는 프로세스는 종료하지 않으며 TCP 연결 정보도 정리하지 않습니다. +계속 확인할 수 없으면 포트가 사용 중인 채로 대기 제한 시간에 도달할 수 있습니다. +현재 포트 사용 프로세스를 확인하고 충돌을 해소한 뒤 재시작을 다시 시도하세요. + ### `ocx restart` 프록시가 실행 중이면 확인된 정확한 PID와 포트에 in-place 재시작을 요청하고, 정상 드레인을 diff --git a/docs-site/src/content/docs/reference/cli/lifecycle.md b/docs-site/src/content/docs/reference/cli/lifecycle.md index a7d61a0bee..ccd2cfb212 100644 --- a/docs-site/src/content/docs/reference/cli/lifecycle.md +++ b/docs-site/src/content/docs/reference/cli/lifecycle.md @@ -38,6 +38,11 @@ and only a stop running outside the proxy can verify that restart window before your client config — so the dashboard refuses with `respawnable_service`, changes nothing, and asks you to run `ocx stop`. +Port recovery after stop or update respects a failed OCX process check even when the PID was +recorded before shutdown. A rejected live holder is left running and prevents TCP-row cleanup. +If it stays unverified, the bounded recovery wait can expire with the port still busy. Check the +current port holder and retry the restart after the conflict is resolved. + ### `ocx restart` When a proxy is running, ask that exact attested PID and port to restart in place, wait for its diff --git a/src/server/port-reclaim.ts b/src/server/port-reclaim.ts index d4023b2ddc..f1bd5628e6 100644 --- a/src/server/port-reclaim.ts +++ b/src/server/port-reclaim.ts @@ -2,11 +2,9 @@ * Reclaim a listen port after stop/update so restart can stay on the configured * port instead of hopping to an ephemeral one (Windows CLOSE_WAIT / leftover ocx). * - * Killing is never the default. A process may be killed only when the caller - * sets `killOcxHolders` and either supplies a non-empty `onlyKillPids` allowlist - * (trusted teardown PIDs, including allowlisted holders that fail ocx revalidate) - * or enables `killAllOcxOnPort` for revalidated ocx listeners. Unknown foreign - * (non-ocx, non-allowlisted) processes are never killed. + * Killing is never the default. It requires `killOcxHolders`, an allowed PID or + * `killAllOcxOnPort`, and successful ocx verification. A historical PID allowlist + * never overrides a rejected verifier result; rejected live holders stay protected. */ import { execFileSync } from "node:child_process"; import { verifyPidIdentity } from "../config/process-state"; @@ -36,8 +34,7 @@ export type ReclaimListenPortOptions = WaitForPortOptions & { * killed (re-checked each scan). Used by post-update restart so a Windows * service wrapper that respawns a *new* bun PID mid-reclaim cannot stay * protected just because it was absent from the pre-wait allowlist snapshot. - * Never kills foreign (non-ocx) processes — only allowlisted teardown PIDs - * and revalidated ocx listeners. + * Every candidate still requires ocx verifier acceptance before termination. */ killAllOcxOnPort?: boolean; /** @@ -174,8 +171,8 @@ export function listListenPids(port: number): number[] { * Never kills a process unless `killOcxHolders === true` and either * `onlyKillPids` is a non-empty allowlist or `killAllOcxOnPort` is set — then * revalidates immediately before each kill. - * Never kills foreign processes. Never drops TCP rows while a live foreign or - * protected ocx listener owns the port, or when the listener scan failed. + * Never overrides a rejected ocx verifier result. Never drops TCP rows while a + * rejected live or protected ocx listener owns the port, or when the scan failed. */ export async function reclaimListenPort( port: number, @@ -231,24 +228,9 @@ export async function reclaimListenPort( } const isOcx = verifyOcxFn(pid) === pid; const allowlisted = allowedKillPids.has(pid); - // Pre-update PIDs can fail verify while still LISTENing (dead owner still - // listed, or cmdline probe raced). Allowlisted teardown PIDs may be killed; - // unknown foreign claimants must remain fail-closed. if (!isOcx) { - if (mayKill && allowlisted) { - if (!killed.has(pid)) { - try { - killFn(pid); - killed.add(pid); - } catch { - // Kill failed: never SetTcpEntry while the process may still own the port. - protectedOcxListener = true; - } - } - if (!isAliveFn(pid)) killed.delete(pid); - else protectedOcxListener = true; - continue; - } + // A saved PID narrows eligible candidates; it cannot override verifier rejection. + // Dead ghost owners have already been skipped by the liveness check above. foreignLive = true; continue; } diff --git a/structure/01_runtime.md b/structure/01_runtime.md index 1601e5762b..3e518e7e70 100644 --- a/structure/01_runtime.md +++ b/structure/01_runtime.md @@ -79,6 +79,12 @@ Callers must not replace the latter with the former merely to avoid the Windows probe. Expected-PID and snapshot removal helpers are the TOCTOU boundary when a replacement proxy can write new state during a probe. +Port reclamation must honor a rejected OCX verifier result even for a PID captured before stop or +update. A rejected live holder prevents both termination and TCP-row deletion for that scan; later +scans may proceed if verification succeeds or the holder exits. The allowlist narrows termination +eligibility and supplies no identity evidence by itself. This contract uses the existing verifier; +it does not add process-instance proof or change the classification cache. + [Decision Log] - 목적과 의도: Separate proxy process ownership from persisted configuration without changing lifecycle behavior. - 기존 구현 및 제약 조건: `src/config.ts` mixed config transactions with cross-platform PID identity, runtime-port attestation, and stale-state cleanup; process writes still require the same config-home and atomic-write protections. diff --git a/tests/server/port-reclaim.test.ts b/tests/server/port-reclaim.test.ts index 56833cf490..930a7866c9 100644 --- a/tests/server/port-reclaim.test.ts +++ b/tests/server/port-reclaim.test.ts @@ -1,5 +1,5 @@ -import { describe, expect, test } from "bun:test"; -import { reclaimListenPort } from "../../src/server/port-reclaim"; +import { describe, expect, spyOn, test } from "bun:test"; +import { reclaimListenPort, type ReclaimListenPortOptions } from "../../src/server/port-reclaim"; import { isBareIpv6Address, parseTcpQuadsForLocalPort, @@ -7,6 +7,20 @@ import { } from "../../src/server/windows-tcp-drop"; import { parseListenPidsFromNetstat } from "../../src/server/port-reclaim"; +/** Exercise several scans and the deadline without depending on wall-clock scheduling. */ +async function reclaimWithMockClock(options: ReclaimListenPortOptions): Promise { + let now = 1_000; + const clock = spyOn(Date, "now").mockImplementation(() => now); + try { + return await reclaimListenPort(10100, "127.0.0.1", { + ...options, timeoutMs: 50, intervalMs: 10, scanIntervalMs: 10, + sleepMs: async () => { now += 10; }, + }); + } finally { + clock.mockRestore(); + } +} + describe("parseListenPidsFromNetstat", () => { test("extracts Windows LISTENING owners for the local port", () => { const output = [ @@ -299,14 +313,11 @@ describe("reclaimListenPort", () => { expect(killed).toEqual([4242]); }); - test("allowlisted pid with failing ocx revalidation is still killed (trusted teardown PID)", async () => { + test("skips kill across later scans when allowlisted pid fails revalidation", async () => { const killed: number[] = []; let available = false; let checks = 0; - await expect(reclaimListenPort(10100, "127.0.0.1", { - timeoutMs: 200, - intervalMs: 20, - scanIntervalMs: 20, + await expect(reclaimWithMockClock({ dropTcpRows: false, killOcxHolders: true, onlyKillPids: [100], @@ -315,29 +326,25 @@ describe("reclaimListenPort", () => { isAliveFn: () => !available, verifyOcxFn: pid => { checks += 1; - // First pass (scan identity) succeeds; later scans reclassify as non-ocx. - // Allowlisted teardown PIDs still take the best-effort kill path. + // Scan identity succeeds, then pre-kill revalidation and later scans reject it. return checks === 1 ? pid : null; }, killFn: pid => { killed.push(pid); available = true; }, - sleepMs: async () => {}, - })).resolves.toBe(true); - expect(killed).toEqual([100]); + })).resolves.toBe(false); + expect(checks).toBeGreaterThanOrEqual(3); + expect(killed).toEqual([]); }); - test("allowlisted revalidation failure still permits TCP drop after kill", async () => { + test("does not drop TCP rows across later scans after allowlisted revalidation fails", async () => { const killed: number[] = []; const dropped: number[] = []; let alive = true; let available = false; let checks = 0; - await expect(reclaimListenPort(10100, "127.0.0.1", { - timeoutMs: 200, - intervalMs: 20, - scanIntervalMs: 20, + await expect(reclaimWithMockClock({ dropTcpRows: true, killOcxHolders: true, onlyKillPids: [100], @@ -357,19 +364,16 @@ describe("reclaimListenPort", () => { available = true; return { dropped: 1, skippedIpv6: 0, accessDenied: 0 }; }, - sleepMs: async () => {}, - })).resolves.toBe(true); - expect(killed).toEqual([100]); - expect(dropped).toEqual([10100]); + })).resolves.toBe(false); + expect(checks).toBeGreaterThanOrEqual(3); + expect(killed).toEqual([]); + expect(dropped).toEqual([]); }); - test("does not drop TCP rows while allowlisted non-ocx survives kill", async () => { + test("does not kill or drop TCP rows for an allowlisted non-ocx listener", async () => { const killed: number[] = []; const dropped: number[] = []; - await expect(reclaimListenPort(10100, "127.0.0.1", { - timeoutMs: 80, - intervalMs: 20, - scanIntervalMs: 20, + await expect(reclaimWithMockClock({ dropTcpRows: true, killOcxHolders: true, onlyKillPids: [100], @@ -384,9 +388,8 @@ describe("reclaimListenPort", () => { dropped.push(port); return { dropped: 1, skippedIpv6: 0, accessDenied: 0 }; }, - sleepMs: async () => {}, })).resolves.toBe(false); - expect(killed).toEqual([100]); + expect(killed).toEqual([]); expect(dropped).toEqual([]); }); @@ -525,20 +528,17 @@ describe("reclaimListenPort", () => { expect(dropped).toEqual([]); }); - test("allowlisted PID that fails ocx verify still gets killed and does not block TCP drop", async () => { + test("allowlisted PID that fails ocx verify stays protected until the deadline", async () => { const killed: number[] = []; const dropped: number[] = []; let alive = true; let available = false; - await expect(reclaimListenPort(10100, "127.0.0.1", { - timeoutMs: 200, - intervalMs: 20, - scanIntervalMs: 20, + await expect(reclaimWithMockClock({ dropTcpRows: true, killOcxHolders: true, onlyKillPids: [14772], isAvailableFn: async () => available, - // Windows often keeps a dead pre-update owner listed; cmdline probe already failed. + // This holder is still alive; a historical PID does not override verifier rejection. listListenPidsFn: () => (alive ? [14772] : []), isAliveFn: () => alive, verifyOcxFn: () => null, @@ -551,9 +551,40 @@ describe("reclaimListenPort", () => { available = true; return { dropped: 1, skippedIpv6: 0, accessDenied: 0 }; }, - sleepMs: async () => {}, + })).resolves.toBe(false); + expect(killed).toEqual([]); + expect(dropped).toEqual([]); + }); + + test.each([false, true])("a different verifier PID is rejected with killAllOcxOnPort=%s", async killAllOcxOnPort => { + const killed: number[] = []; + const dropped: number[] = []; + await expect(reclaimWithMockClock({ + dropTcpRows: true, killOcxHolders: true, killAllOcxOnPort, onlyKillPids: [100], + isAvailableFn: async () => false, listListenPidsFn: () => [100], isAliveFn: () => true, + verifyOcxFn: () => 200, + killFn: pid => { killed.push(pid); }, + dropTcpFn: port => { dropped.push(port); return 1; }, + })).resolves.toBe(false); + expect(killed).toEqual([]); + expect(dropped).toEqual([]); + }); + + test("a later successful verification can reclaim a previously rejected holder", async () => { + let alive = true; + let available = false; + let checks = 0; + const checksAtKill: number[] = []; + const dropped: number[] = []; + await expect(reclaimWithMockClock({ + dropTcpRows: true, killOcxHolders: true, onlyKillPids: [100], + isAvailableFn: async () => available, listListenPidsFn: () => alive ? [100] : [], + isAliveFn: () => alive, + verifyOcxFn: pid => ++checks === 1 ? null : pid, + killFn: () => { checksAtKill.push(checks); alive = false; }, + dropTcpFn: port => { dropped.push(port); available = true; return 1; }, })).resolves.toBe(true); - expect(killed).toEqual([14772]); + expect(checksAtKill).toEqual([3]); // rejected scan, accepted scan, accepted pre-kill check expect(dropped).toEqual([10100]); });