diff --git a/src/server/proxy-liveness.ts b/src/server/proxy-liveness.ts index 04dbd50930..655626f4f9 100644 --- a/src/server/proxy-liveness.ts +++ b/src/server/proxy-liveness.ts @@ -102,6 +102,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, @@ -130,8 +137,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; return version === undefined ? { pid } : { pid, version }; } catch { // Transport failure (timeout / refused) — retry while budget remains; a proxy that diff --git a/src/update/job.ts b/src/update/job.ts index 2f021715e8..103ace61aa 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, @@ -255,19 +261,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 @@ -1519,7 +1512,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/proxy-liveness.test.ts b/tests/proxy-liveness.test.ts index 7fd806fd62..4124e60e33 100644 --- a/tests/proxy-liveness.test.ts +++ b/tests/proxy-liveness.test.ts @@ -5,6 +5,7 @@ import { } from "../src/server/readiness"; import { findLiveProxy, + isHealthzVersion, isOpencodexHealthz, probeHostname, probeReadiness, @@ -56,6 +57,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(); @@ -128,6 +137,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); + }); +}); + describe("findLiveProxy", () => { test("prefers the runtime-port record over config.port (fallback-port starts are found)", async () => { const urls: string[] = [];