From 3235d611ed317321623bea5939d203b8d3dd0559 Mon Sep 17 00:00:00 2001
From: luvs01 <27862058+luvs01@users.noreply.github.com>
Date: Sun, 20 Sep 2026 19:44:15 +0900
Subject: [PATCH] fix(server): bound /healthz version before it reaches
 diagnostics

proxyIdentityAt carried any string the port holder returned as version into LiveProxy/version-skew diagnostics, so a hostile or confused responder could inject newlines or terminal-control sequences (e.g. OSC 52) into human-facing output. Gate it with a shared isHealthzVersion (semver-shaped, <=64 chars) and reuse it for the update restart probe, replacing the module-local isVersionLike.
---
 src/server/proxy-liveness.ts        | 12 ++++++++++--
 src/update/job.ts                   | 23 ++++++++---------------
 tests/server/proxy-liveness.test.ts | 17 +++++++++++++++++
 3 files changed, 35 insertions(+), 17 deletions(-)

diff --git a/src/server/proxy-liveness.ts b/src/server/proxy-liveness.ts
index 594be867b02..0282284e3c2 100644
--- a/src/server/proxy-liveness.ts
+++ b/src/server/proxy-liveness.ts
@@ -148,6 +148,13 @@ export function isOpencodexHealthz(body: HealthzIdentity | null): boolean {
   return body.status === "ok" && typeof body.version === "string" && typeof body.uptime === "number";
 }
 
+/** A bounded version string safe to carry beyond the untrusted health response. */
+export function isHealthzVersion(value: unknown): value is string {
+  return typeof value === "string"
+    && value.length <= 64
+    && /^\d+\.\d+\.\d+(?:-[0-9A-Za-z.-]+)?(?:\+[0-9A-Za-z.-]+)?$/.test(value);
+}
+
 /** Identity-checked /healthz probe; null when unreachable, non-OK, or not our proxy. */
 export async function proxyIdentityAt(
   port: number,
@@ -176,8 +183,9 @@ export async function proxyIdentityAt(
       if (!isOpencodexHealthz(body)) return null;
       const pid = typeof body?.pid === "number" ? body.pid : null;
       if (opts.expectedPid !== undefined && pid !== null && pid !== opts.expectedPid) return null;
-      // Guarded the same way `pid` is: a non-string version is absent, not coerced.
-      const version = typeof body?.version === "string" ? body.version : undefined;
+      // Whoever holds the port controls this response. Only carry bounded semver text into
+      // diagnostics; dropping anything else prevents terminal controls reaching human output.
+      const version = isHealthzVersion(body?.version) ? body.version : undefined;
       // Same guard for the role, for the same reason: absent on a standalone/hub proxy and on
       // a legacy body, and never coerced from a non-string.
       const role = typeof body?.role === "string" ? body.role : undefined;
diff --git a/src/update/job.ts b/src/update/job.ts
index 16310cad824..1d7af10727e 100644
--- a/src/update/job.ts
+++ b/src/update/job.ts
@@ -24,7 +24,13 @@ import {
 import { stopWinswService } from "../lib/winsw";
 import { listListenPids, reclaimListenPort, scanListenPids, type ListenPidScan } from "../server/port-reclaim";
 import { dropWindowsTcpRowsForLocalPort } from "../server/windows-tcp-drop";
-import { isOpencodexHealthz, probeHostname, proxyIdentityAt, type HealthzIdentity } from "../server/proxy-liveness";
+import {
+  isHealthzVersion,
+  isOpencodexHealthz,
+  probeHostname,
+  proxyIdentityAt,
+  type HealthzIdentity,
+} from "../server/proxy-liveness";
 import { isServiceInstalled, isServiceViable, readServiceBackend, stopWindows } from "../service";
 import {
   type Channel,
@@ -267,19 +273,6 @@ function ensureJobDir(): void {
  * TYPE and size — enough to tell a reader what class of failure occurred — and never its text,
  * which is where the paths and account names live.
  */
-/**
- * A version string we are willing to repeat in a persisted field.
- *
- * Semver plus an optional prerelease/build tail, capped in length. Anything else is dropped
- * rather than logged: `/healthz` is answered by whatever holds the port, so its `version` is
- * external input on the same footing as an error message.
- */
-function isVersionLike(value: unknown): value is string {
-  return typeof value === "string"
-    && value.length <= 64
-    && /^\d+\.\d+\.\d+(?:-[0-9A-Za-z.-]+)?(?:\+[0-9A-Za-z.-]+)?$/.test(value);
-}
-
 function withheldSummary(error: unknown): string {
   // `error.name` is writable, so it is external text like the message. A fixed classification
   // is the only part of an unknown error we can state without repeating something we were
@@ -1588,7 +1581,7 @@ async function defaultProbeProxyIdentity(
       // `/healthz` is answered by whatever is listening on that port, so a hostile or confused
       // responder can return any string here — and the restart-evidence reasons below
       // interpolate it into a persisted field. A version is a version or it is nothing.
-      ...(isVersionLike(body?.version) ? { version: body.version } : {}),
+      ...(isHealthzVersion(body?.version) ? { version: body.version } : {}),
     };
   } catch {
     return null;
diff --git a/tests/server/proxy-liveness.test.ts b/tests/server/proxy-liveness.test.ts
index d675f04c8b8..fcb2734dc68 100644
--- a/tests/server/proxy-liveness.test.ts
+++ b/tests/server/proxy-liveness.test.ts
@@ -6,6 +6,7 @@ import {
 import {
   DEFAULT_PROBE_TIMEOUT_MS,
   findLiveProxy,
+  isHealthzVersion,
   isOpencodexHealthz,
   loopbackProbeHosts,
   probeHostname,
@@ -96,6 +97,14 @@ describe("proxyIdentityAt", () => {
     expect(identity).toEqual({ pid: 4242, version: "2.6.17" });
   });
 
+  test("does not propagate an unsafe version from the process holding the port", async () => {
+    const version = "9.9.9\nFAKE OK\u001b]52;c;SGVsbG8=\u0007";
+    const identity = await proxyIdentityAt(10100, {}, {
+      fetchFn: (async () => healthz({ ...OURS, version })) as typeof fetch,
+    });
+    expect(identity).toEqual({ pid: 4242 });
+  });
+
   test("rejects foreign 200s, non-OK responses, and pid mismatches", async () => {
     expect(await proxyIdentityAt(10100, {}, { fetchFn: (async () => healthz({ ok: true })) as typeof fetch })).toBeNull();
     expect(await proxyIdentityAt(10100, {}, { fetchFn: (async () => healthz(OURS, 503)) as typeof fetch })).toBeNull();
@@ -168,6 +177,14 @@ describe("proxyIdentityAt", () => {
   });
 });
 
+describe("isHealthzVersion", () => {
+  test("accepts bounded semver and rejects unsafe or oversized display text", () => {
+    expect(isHealthzVersion("2.35.0-preview.1+build.7")).toBe(true);
+    expect(isHealthzVersion("9.9.9\nFAKE OK\u001b]52;c;SGVsbG8=\u0007")).toBe(false);
+    expect(isHealthzVersion(`1.0.0-${"a".repeat(59)}`)).toBe(false);
+  });
+});
+
 /**
  * #5004. A bare `ocx start` beside a healthy proxy printed the port-busy warning, hopped to
  * an ephemeral port, and left two proxies running with Codex pointed at the second. The hop