From 24d7308010dfac08a0499cee130991cd24ce3d56 Mon Sep 17 00:00:00 2001 From: Xiangzhe Date: Sun, 23 Aug 2026 13:01:45 -0300 Subject: [PATCH] fix(cli): complete Windows cliproxy platform handling + pid probe (#11236) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Residuals of #11236 after #10371/#10491 landed on the tip: - Dist-fold residuals (bugs 2+3): managedBinaryName() (binaryManager), resolveSpawnArgs() (installers/cliproxy) and the per-OS memory probes in getProcessInfo() (processManager) still read the process.platform literal, which the Linux build of the published artifact constant-folds (precedent: b43a212680 / #10244). Converted to call-time os.platform() reads, matching the module's documented anti-fold pattern. Test-side process.platform uses are not bundled and stay. - Fold guard: new tests/unit/windows-platform-fold-guard-11236.test.ts pins zero out-of-comment process.platform occurrences in the four artifact runtime files, with a comment-stripping tokenizer plus mutation self-checks. - Bug 6 (pid null on Windows): portProbe.resolvePortPid() only probed lsof/ss/net-tools netstat. Added a netstat -ano probe with a dedicated LISTENING-row parser (parseWindowsNetstatPid) as the last fallback; Unix probes unchanged, and the Windows parser never matches Unix rows (LISTEN vs LISTENING). Also converted the darwin args branch in the same array to os.platform() (same fold class, same hunk). - Bug 5 hardening: runOAuthStatus coerces an out-of-contract 200 payload to an empty list with a sanitized stderr warning instead of crashing on .filter over a non-array. TDD: guard test, parser tests and the oauth hardening test all failed RED before the fix and pass GREEN after; sibling suites (binaryManager, processManager, portProbePid, cli-oauth-commands, installers, ServiceSupervisor, version-manager) green. The 6877 spawn-args test's win32 mock moved from defineProperty(process.platform) to mock.method(os, "platform") to match the new runtime read — assertion unchanged. --- bin/cli/commands/oauth.mjs | 16 +- src/lib/services/installers/cliproxy.ts | 8 +- src/lib/services/portProbe.ts | 51 +++++- src/lib/versionManager/binaryManager.ts | 10 +- src/lib/versionManager/processManager.ts | 9 +- tests/unit/cli-oauth-commands.test.ts | 35 +++- .../cliproxy-resolve-spawn-args-6877.test.ts | 13 +- tests/unit/services/portProbePid.test.ts | 40 ++++- .../windows-platform-fold-guard-11236.test.ts | 168 ++++++++++++++++++ 9 files changed, 329 insertions(+), 21 deletions(-) create mode 100644 tests/unit/windows-platform-fold-guard-11236.test.ts diff --git a/bin/cli/commands/oauth.mjs b/bin/cli/commands/oauth.mjs index c9f8386d2b..1cdaeb8267 100644 --- a/bin/cli/commands/oauth.mjs +++ b/bin/cli/commands/oauth.mjs @@ -258,8 +258,7 @@ async function runDeviceFlow(def, opts) { process.stdout.write(`\nAuthorization URL not available\n\n`); } - if (opts.browser !== false && verificationUri) - await openBrowser(verificationUri); + if (opts.browser !== false && verificationUri) await openBrowser(verificationUri); process.stderr.write("Waiting for device authorization...\n"); const deadline = Date.now() + (opts.timeout ?? 300000); const intervalMs = (start.intervalMs ?? start.interval ?? 5) * 1000; @@ -320,7 +319,18 @@ export async function runOAuthStatus(opts, cmd) { process.exit(1); } const data = await res.json(); - const connections = (data.connections ?? data.providers ?? data.items ?? data).filter( + const payload = data?.connections ?? data?.providers ?? data?.items ?? data; + // #11236 (bug 5 residual): a 200 whose body is out of contract (no + // connections/providers/items array — e.g. `{"status":"ok"}`) used to fall + // through to `.filter` on a non-array and crash with a bare TypeError plus a + // libuv teardown assertion on Windows. Coerce to an empty list with a + // sanitized one-line warning instead of dumping a stack trace. + if (!Array.isArray(payload)) { + process.stderr.write( + "Warning: unexpected response shape from /api/providers; showing no connections.\n" + ); + } + const connections = (Array.isArray(payload) ? payload : []).filter( (c) => c.authType === "oauth" || c.authType === "oauth2" ); emit(connections, globalOpts, connectionSchema); diff --git a/src/lib/services/installers/cliproxy.ts b/src/lib/services/installers/cliproxy.ts index 6ffb7e8196..dcb7dfc916 100644 --- a/src/lib/services/installers/cliproxy.ts +++ b/src/lib/services/installers/cliproxy.ts @@ -11,6 +11,7 @@ */ import fs from "node:fs"; +import os from "node:os"; import path from "node:path"; import { DATA_DIR } from "@/lib/db/core"; import { upsertVersionManagerTool } from "@/lib/db/versionManager"; @@ -101,7 +102,12 @@ export async function update(): Promise { * async file I/O is not available here. */ export function resolveSpawnArgs(port: number): SpawnArgs { - const executableName = process.platform === "win32" ? "cliproxyapi.exe" : "cliproxyapi"; + // #11236 (bug 3 residual): runtime os.platform() read — a process.platform + // literal here is constant-folded to the Linux build machine when the + // published artifact is bundled, dropping the `.exe` suffix from the spawn + // path on Windows and failing with ENOENT even when a valid .exe exists + // (same fold class as b43a212680 / #10244/#10293). + const executableName = os.platform() === "win32" ? "cliproxyapi.exe" : "cliproxyapi"; const symlinkPath = path.join(BIN_DIR, executableName); fs.mkdirSync(CONFIG_DIR, { recursive: true }); diff --git a/src/lib/services/portProbe.ts b/src/lib/services/portProbe.ts index 9a890f541b..a4111400e5 100644 --- a/src/lib/services/portProbe.ts +++ b/src/lib/services/portProbe.ts @@ -15,6 +15,7 @@ import { createConnection } from "node:net"; import { spawn } from "node:child_process"; +import os from "node:os"; /** Result of probing the service before spawning. */ export interface PreSpawnProbe { @@ -190,6 +191,31 @@ export function parseNetstatPid(stdout: string, port: number): number | null { return null; } +/** + * Windows `netstat -ano` carries the pid in its own last column (#11236): + * + * Proto Local Address Foreign Address State PID + * TCP 0.0.0.0:20128 0.0.0.0:0 LISTENING 12345 + * TCP [::]:20128 [::]:0 LISTENING 12345 + * + * Only TCP LISTENING rows carry a pid (UDP rows have no state column at all). + * The local address is matched on `:` — the `:` anchor keeps a port that + * merely shares a suffix (128 vs 20128) or a foreign address ending in the + * same digits from being read as the listener. + */ +export function parseWindowsNetstatPid(stdout: string, port: number): number | null { + for (const line of stdout.split("\n")) { + const columns = line.trim().split(/\s+/); + // proto local-address foreign-address state pid + if (columns.length < 5) continue; + if (columns[3].toUpperCase() !== "LISTENING") continue; + if (!columns[1].endsWith(`:${port}`)) continue; + const pid = Number.parseInt(columns[columns.length - 1], 10); + if (Number.isFinite(pid)) return pid; + } + return null; +} + /** * Ways to ask the OS which process holds a port, in preference order. * @@ -198,6 +224,12 @@ export function parseNetstatPid(stdout: string, port: number): number | null { * once `spawn` has turned ENOENT into a null. `ss` ships with iproute2 and * `netstat` with net-tools, so between the three there is normally something * to ask on any host the supervisor runs on. + * + * The Windows `netstat -ano` probe runs last: on Windows the earlier probes + * fail fast (lsof/ss do not exist; the net-tools flags are rejected by the + * Windows netstat), while on Unix `netstat -ano` either errors out or prints + * the Linux/macOS row shapes the Windows parser deliberately never matches + * (LISTEN vs LISTENING), so it degrades to a no-op instead of a false pid. */ const PID_PROBES: ReadonlyArray<{ command: string; @@ -212,9 +244,17 @@ const PID_PROBES: ReadonlyArray<{ }, { command: "netstat", - args: () => (process.platform === "darwin" ? ["-anv", "-p", "tcp"] : ["-tlnp"]), + // #11236: runtime os.platform() read — a process.platform literal is + // constant-folded to the Linux build machine in the published artifact, + // pruning the darwin branch on macOS (same fold class as b43a212680). + args: () => (os.platform() === "darwin" ? ["-anv", "-p", "tcp"] : ["-tlnp"]), parse: parseNetstatPid, }, + { + command: "netstat", + args: () => ["-ano"], + parse: parseWindowsNetstatPid, + }, ]; /** Run one probe, resolving null on a missing binary, a non-match or a timeout. */ @@ -264,10 +304,11 @@ function runPidProbe( * way they trust a freshly-spawned one. Returns null if nothing is found or * the lookup fails/times out (best-effort; never blocks adoption on this). * - * Tries `lsof`, then `ss`, then `netstat`, so a host missing any one of them - * still reports a real pid instead of a silent null (#10431). The probes share - * one deadline, so the whole lookup still costs at most - * `PID_RESOLVE_TIMEOUT_MS`. + * Tries `lsof`, then `ss`, then `netstat`, then the Windows `netstat -ano` + * shape, so a host missing any one of them — including a stock Windows host + * with none of the Unix tools — still reports a real pid instead of a silent + * null (#10431, #11236). The probes share one deadline, so the whole lookup + * still costs at most `PID_RESOLVE_TIMEOUT_MS`. */ export async function resolvePortPid(port: number): Promise { const deadline = Date.now() + PID_RESOLVE_TIMEOUT_MS; diff --git a/src/lib/versionManager/binaryManager.ts b/src/lib/versionManager/binaryManager.ts index 743d13f54a..764168dba4 100644 --- a/src/lib/versionManager/binaryManager.ts +++ b/src/lib/versionManager/binaryManager.ts @@ -110,8 +110,16 @@ async function verifyChecksum(filePath: string, expectedSha256: string): Promise return hash.digest("hex").toLowerCase() === expectedSha256.toLowerCase(); } +/** + * #11236: read os.platform() at call time, never the build-foldable + * process.platform literal — the published-artifact build runs on Linux and + * constant-folds it, pruning the win32 branch so the managed binary lost its + * `.exe` suffix on Windows installs (same fold class as b43a212680 / + * #10244/#10293, which converted detectPlatform/detectArch; #10371 fixed the + * name in source but left this literal read behind). + */ function managedBinaryName(): string { - return process.platform === "win32" ? "cliproxyapi.exe" : "cliproxyapi"; + return os.platform() === "win32" ? "cliproxyapi.exe" : "cliproxyapi"; } function findBinaryInDir(dir: string): string | null { diff --git a/src/lib/versionManager/processManager.ts b/src/lib/versionManager/processManager.ts index ec538ab770..55164df864 100644 --- a/src/lib/versionManager/processManager.ts +++ b/src/lib/versionManager/processManager.ts @@ -152,14 +152,19 @@ export async function getProcessInfo(pid: number): Promise<{ } try { - if (process.platform === "linux" || process.platform === "android") { + // #11236: single runtime os.platform() read for the per-OS memory probes — + // a process.platform literal is constant-folded to the build machine's + // platform in the published artifact (same fold class as b43a212680 / + // #10244/#10293), so the darwin probe branch would be pruned on macOS. + const platform = os.platform(); + if (platform === "linux" || platform === "android") { const statusFile = `/proc/${pid}/status`; const content = await fs.readFile(statusFile, "utf-8"); const match = content.match(/VmRSS:\s+(\d+)\s+kB/); if (match) { return { pid, alive: true, memoryUsage: parseInt(match[1], 10) * 1024 }; } - } else if (process.platform === "darwin") { + } else if (platform === "darwin") { const { execFile } = await import("child_process"); const { promisify } = await import("util"); const execFileAsync = promisify(execFile); diff --git a/tests/unit/cli-oauth-commands.test.ts b/tests/unit/cli-oauth-commands.test.ts index 63301f818f..9d67c340a1 100644 --- a/tests/unit/cli-oauth-commands.test.ts +++ b/tests/unit/cli-oauth-commands.test.ts @@ -108,13 +108,46 @@ test("runOAuthStatus consumes the connections envelope", async () => { const parsed = JSON.parse(out); assert.deepEqual( parsed.map((connection: { id: string }) => connection.id), - ["conn1", "conn2"], + ["conn1", "conn2"] ); } finally { globalThis.fetch = origFetch; } }); +test("runOAuthStatus tolerates an out-of-contract 200 payload (#11236)", async () => { + // Bug 5 residual: #10491 added the `data.connections ??` envelope, but a 200 + // whose body is an object without connections/providers/items still fell + // through to `data` itself and crashed on `.filter is not a function` + // (followed by a libuv teardown assertion on Windows). The guard must coerce + // to an empty list and warn on stderr — never throw a raw TypeError. + const origFetch = globalThis.fetch; + // `as unknown as` (not `as any`): this file's no-explicit-any suppression is + // frozen at its pre-existing count, so new casts must be any-free. + globalThis.fetch = (() => + Promise.resolve(makeResp({ status: "ok" }))) as unknown as typeof globalThis.fetch; + + const stderrChunks: string[] = []; + const origStderr = process.stderr.write.bind(process.stderr); + process.stderr.write = ((chunk: string | Uint8Array) => { + if (typeof chunk === "string") stderrChunks.push(chunk); + return true; + }) as typeof process.stderr.write; + + try { + const { runOAuthStatus } = await import("../../bin/cli/commands/oauth.mjs"); + const out = await captureStdout(() => runOAuthStatus({}, makeCmd())); + assert.deepEqual(JSON.parse(out), []); + } finally { + globalThis.fetch = origFetch; + process.stderr.write = origStderr; + } + + const warning = stderrChunks.join(""); + assert.ok(warning.length > 0, "a sanitized warning must be written to stderr"); + assert.ok(!warning.includes("at /"), "warning must not leak a stack trace"); +}); + test("runOAuthRevoke com --yes chama endpoint de revogação", async () => { let capturedUrl = ""; let capturedMethod = ""; diff --git a/tests/unit/services/installers/cliproxy-resolve-spawn-args-6877.test.ts b/tests/unit/services/installers/cliproxy-resolve-spawn-args-6877.test.ts index a5e9d5ae6e..cc8b0a3102 100644 --- a/tests/unit/services/installers/cliproxy-resolve-spawn-args-6877.test.ts +++ b/tests/unit/services/installers/cliproxy-resolve-spawn-args-6877.test.ts @@ -12,7 +12,7 @@ * temp-directory filesystem. */ -import { describe, it, beforeEach, after } from "node:test"; +import { describe, it, beforeEach, after, mock } from "node:test"; import assert from "node:assert/strict"; import fs from "node:fs"; import os from "node:os"; @@ -68,8 +68,11 @@ describe("resolveSpawnArgs (#6877 — real filesystem)", () => { }); it("uses the .exe command name on Windows", async () => { - const originalPlatformDescriptor = Object.getOwnPropertyDescriptor(process, "platform"); - Object.defineProperty(process, "platform", { value: "win32", configurable: true }); + // resolveSpawnArgs reads os.platform() at call time (#11236 — a + // process.platform literal is constant-folded away by the Linux build of + // the published artifact), so the Windows host is simulated through the + // same runtime os.platform() seam binaryManager.test.ts uses for #10244. + const platformMock = mock.method(os, "platform", () => "win32"); try { const { resolveSpawnArgs } = @@ -78,9 +81,7 @@ describe("resolveSpawnArgs (#6877 — real filesystem)", () => { assert.equal(result.command, path.join(dataDir, "bin", "cliproxyapi.exe")); } finally { - if (originalPlatformDescriptor) { - Object.defineProperty(process, "platform", originalPlatformDescriptor); - } + platformMock.mock.restore(); } }); diff --git a/tests/unit/services/portProbePid.test.ts b/tests/unit/services/portProbePid.test.ts index c58bf86045..b9df600eac 100644 --- a/tests/unit/services/portProbePid.test.ts +++ b/tests/unit/services/portProbePid.test.ts @@ -17,6 +17,7 @@ import { parseLsofPid, parseNetstatPid, parseSsPid, + parseWindowsNetstatPid, resolvePortPid, } from "@/lib/services/portProbe"; @@ -65,8 +66,7 @@ test("parseNetstatPid matches on the local address, not the foreign one", () => }); test("parseNetstatPid reads macOS process:pid output", () => { - const stdout = - "tcp4 0 0 127.0.0.1.20128 *.* LISTEN 0 0 131072 131072 node:596922 00100\n"; + const stdout = "tcp4 0 0 127.0.0.1.20128 *.* LISTEN 0 0 131072 131072 node:596922 00100\n"; assert.equal(parseNetstatPid(stdout, 20128), 596922); }); @@ -77,6 +77,42 @@ test("parseNetstatPid ignores non-listening rows and unknown ports", () => { assert.equal(parseNetstatPid("", 20128), null); }); +/** + * Realistic `netstat -ano` sample from Windows 11 (#11236 bug 6): the pid is + * the last whitespace-separated column and only exists on rows whose state is + * LISTENING. This is the only pid probe available on a stock Windows host — + * neither lsof nor ss nor net-tools `netstat -tlnp` exist there, so a Windows + * service adopted by the supervisor reported `pid: null` while healthy. + */ +const WINDOWS_NETSTAT_ANO = [ + "Active Connections", + "", + " Proto Local Address Foreign Address State PID", + " TCP 0.0.0.0:135 0.0.0.0:0 LISTENING 1244", + " TCP 0.0.0.0:20128 0.0.0.0:0 LISTENING 12345", + " TCP 127.0.0.1:8317 0.0.0.0:0 LISTENING 5678", + " TCP 192.168.1.10:52413 140.82.121.4:443 ESTABLISHED 9012", + " TCP [::]:20128 [::]:0 LISTENING 12345", + " UDP 0.0.0.0:5353 *:* 3460", + "", +].join("\r\n"); + +test("parseWindowsNetstatPid reads the pid from a LISTENING row (#11236)", () => { + assert.equal(parseWindowsNetstatPid(WINDOWS_NETSTAT_ANO, 20128), 12345); + assert.equal(parseWindowsNetstatPid(WINDOWS_NETSTAT_ANO, 8317), 5678); +}); + +test("parseWindowsNetstatPid matches the local address, not the foreign one", () => { + // 443 appears only as a foreign address on an ESTABLISHED row. + assert.equal(parseWindowsNetstatPid(WINDOWS_NETSTAT_ANO, 443), null); + // 5353 appears only on a UDP row, which has no LISTENING state. + assert.equal(parseWindowsNetstatPid(WINDOWS_NETSTAT_ANO, 5353), null); + // A port that shares a suffix with a listening one must not match: 0128 vs + // 20128 — the `:` anchor on the local address prevents the partial hit. + assert.equal(parseWindowsNetstatPid(WINDOWS_NETSTAT_ANO, 128), null); + assert.equal(parseWindowsNetstatPid("", 20128), null); +}); + test("resolvePortPid finds the pid holding a port", async () => { const server = createServer(); await new Promise((resolve) => server.listen(29994, "127.0.0.1", resolve)); diff --git a/tests/unit/windows-platform-fold-guard-11236.test.ts b/tests/unit/windows-platform-fold-guard-11236.test.ts new file mode 100644 index 0000000000..df2812b88b --- /dev/null +++ b/tests/unit/windows-platform-fold-guard-11236.test.ts @@ -0,0 +1,168 @@ +/** + * Structural regression guard for #11236 (Windows cliproxy residuals, bugs 2+3). + * + * Why this guard exists: the published npm artifact is bundled on Linux, and + * the bundler constant-folds every literal `process.platform` read to the + * BUILD machine's platform ("linux"), pruning the win32 branch from the + * shipped artifact. Precedent: b43a212680 (#10244/#10293), which converted + * detectPlatform/detectArch to runtime `os.platform()`/`os.arch()` reads for + * exactly this reason. #10371 later fixed the Windows `.exe` binary name in + * the source but left literal `process.platform` reads behind in the same + * runtime paths, so the shipped artifact still: + * - named the managed binary `cliproxyapi` (no `.exe`) at install time + * (binaryManager.managedBinaryName), and + * - spawned that extension-less path at start time + * (installers/cliproxy.resolveSpawnArgs) -> ENOENT on Windows even with a + * valid `.exe` in place (issue #11236 bugs 2 and 3). + * + * The runtime-safe pattern is a call-time `os.platform()` read. This guard + * fails if `process.platform` reappears outside a comment in any file whose + * platform branch feeds the published artifact's runtime behavior (binary + * name, spawn path, per-OS probe selection). + */ + +import { test } from "node:test"; +import assert from "node:assert/strict"; +import fs from "node:fs"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; + +const REPO_ROOT = path.resolve(path.dirname(fileURLToPath(import.meta.url)), "..", ".."); + +const GUARDED_FILES = [ + "src/lib/versionManager/binaryManager.ts", + "src/lib/versionManager/processManager.ts", + "src/lib/services/installers/cliproxy.ts", + "src/lib/services/portProbe.ts", +]; + +interface Offender { + line: number; + text: string; +} + +/** + * Returns the source with every `//` and `/* ... *\/` comment blanked out + * (replaced by spaces, newlines preserved so line numbers are stable). String + * literals are kept verbatim — a `process.platform` inside one is still + * flagged, which is acceptable: none of the guarded files carry the pattern + * in a string, and a false positive there is safer than a false negative in + * code. + */ +function stripComments(source: string): string { + let out = ""; + let i = 0; + let inBlock = false; + let inLine = false; + let inString: string | null = null; + while (i < source.length) { + const ch = source[i]; + const next = source[i + 1]; + if (inLine) { + if (ch === "\n") { + inLine = false; + out += ch; + } else { + out += " "; + } + i++; + continue; + } + if (inBlock) { + if (ch === "*" && next === "/") { + inBlock = false; + out += " "; + i += 2; + continue; + } + out += ch === "\n" ? "\n" : " "; + i++; + continue; + } + if (inString) { + out += ch; + if (ch === "\\") { + out += next ?? ""; + i += 2; + continue; + } + if (ch === inString) inString = null; + i++; + continue; + } + if (ch === "/" && next === "/") { + inLine = true; + out += " "; + i += 2; + continue; + } + if (ch === "/" && next === "*") { + inBlock = true; + out += " "; + i += 2; + continue; + } + if (ch === '"' || ch === "'" || ch === "`") inString = ch; + out += ch; + i++; + } + return out; +} + +/** + * Every remaining `process.platform` occurrence after comment stripping is an + * offender — the fold-explanation comments reference the pattern by name and + * must remain free to do so. + */ +function findFoldableReads(source: string): Offender[] { + const stripped = stripComments(source); + const offenders: Offender[] = []; + stripped.split("\n").forEach((line, index) => { + if (line.includes("process.platform")) { + offenders.push({ line: index + 1, text: source.split("\n")[index].trim() }); + } + }); + return offenders; +} + +for (const relPath of GUARDED_FILES) { + test(`${relPath} has no build-foldable process.platform reads (#11236)`, () => { + const source = fs.readFileSync(path.join(REPO_ROOT, relPath), "utf8"); + const offenders = findFoldableReads(source); + assert.deepEqual( + offenders, + [], + `${relPath} must read os.platform() at call time instead of the ` + + `build-foldable process.platform literal (Turbopack folds it to the ` + + `Linux build machine — b43a212680 / #10244 / #10371). Offenders: ` + + offenders.map((o) => `L${o.line}: ${o.text}`).join("; ") + ); + }); +} + +// Guard-the-guard (mutation check on synthetic input, so the real sources +// never need to be touched): a code occurrence MUST be caught, comment-only +// occurrences MUST be let through. +test("findFoldableReads catches a code occurrence (mutation self-check)", () => { + const snippet = [ + 'const name = process.platform === "win32" ? "a.exe" : "a";', + "// process.platform in a line comment is allowed", + "/**", + " * process.platform in a block comment is allowed", + " */", + "/* process.platform single-line block is allowed */", + "const ok = os.platform();", + ].join("\n"); + const offenders = findFoldableReads(snippet); + assert.equal(offenders.length, 1); + assert.equal(offenders[0].line, 1); +}); + +test("findFoldableReads reports nothing when only comments mention the pattern", () => { + const snippet = [ + "// process.platform", + "/* process.platform */", + "const p = os.platform();", + ].join("\n"); + assert.deepEqual(findFoldableReads(snippet), []); +});