From 50f5cecb80badffc307bfd805ec9f992255146b5 Mon Sep 17 00:00:00 2001 From: Xiangzhe Date: Fri, 21 Aug 2026 14:10:31 -0300 Subject: [PATCH] fix(security): don't auto-adopt an unverified listener on a service port MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit decidePreSpawn adopted any listener that returned a 2xx on the health path, so a local process that squats an embedded-service port before the supervisor starts it would be adopted — receiving the injected service API key and script execution inside the dashboard origin. Adoption is now opt-in (OMNIROUTE_ADOPT_EXISTING_SERVICE=1); by default a healthy-but-unverified listener yields the same actionable error as a held-but-unhealthy port. The embedded-UI CSP hardening (strict embed CSP / the dead scriptSrc ternary) is a separate follow-up. Reported by @rafaelfiguereod-stack via GHSA-wg9p-6m2g-4v27. --- src/lib/services/ServiceSupervisor.ts | 9 ++++- src/lib/services/portProbe.ts | 38 +++++++++++++++++-- tests/unit/ninerouter-embed-port-6205.test.ts | 19 +++++++--- 3 files changed, 55 insertions(+), 11 deletions(-) diff --git a/src/lib/services/ServiceSupervisor.ts b/src/lib/services/ServiceSupervisor.ts index 9ec8e44f6a..df1011bb45 100644 --- a/src/lib/services/ServiceSupervisor.ts +++ b/src/lib/services/ServiceSupervisor.ts @@ -7,7 +7,12 @@ import { sanitizeErrorMessage } from "@omniroute/open-sse/utils/error"; import { getServiceRow, updateServiceField, setToolStatus } from "@/lib/db/versionManager"; import { RingBuffer } from "./ringBuffer"; import { HealthChecker } from "./healthCheck"; -import { decidePreSpawn, probeBeforeSpawn, resolvePortPid } from "./portProbe"; +import { + decidePreSpawn, + isAdoptExistingEnabled, + probeBeforeSpawn, + resolvePortPid, +} from "./portProbe"; import type { ServiceConfig, ServiceState, ServiceStatus, LogLine, HealthState } from "./types"; const CRASH_FAST_THRESHOLD_MS = 5_000; @@ -111,7 +116,7 @@ export class ServiceSupervisor extends EventEmitter { // Opt-in per ServiceConfig so the default spawn path is unchanged. if (this.config.probeBeforeSpawn) { const probe = await probeBeforeSpawn(this.config.healthUrl(), this.config.port); - const decision = decidePreSpawn(probe, this.config.port); + const decision = decidePreSpawn(probe, this.config.port, isAdoptExistingEnabled()); if (decision.action === "adopt") { // Something healthy already serves this port. We didn't spawn it, diff --git a/src/lib/services/portProbe.ts b/src/lib/services/portProbe.ts index 81c3d8a846..2a9fd502bd 100644 --- a/src/lib/services/portProbe.ts +++ b/src/lib/services/portProbe.ts @@ -37,11 +37,30 @@ const PID_RESOLVE_TIMEOUT_MS = 2_000; * * Pure — no I/O — so it can be exhaustively unit-tested. */ -export function decidePreSpawn(probe: PreSpawnProbe, port: number): PreSpawnDecision { - // A healthy instance is already serving on the port — adopt it rather than - // spawn a duplicate that would immediately die with EADDRINUSE. +export function decidePreSpawn( + probe: PreSpawnProbe, + port: number, + allowAdopt = false +): PreSpawnDecision { if (probe.healthy) { - return { action: "adopt" }; + // A 2xx on the health path does NOT prove the listener is our service: a + // local process can squat the port, answer 200, and get adopted — receiving + // the injected service API key and script execution inside the dashboard + // origin (GHSA-wg9p-6m2g-4v27). Adopt an already-healthy listener only when + // the operator explicitly opts in; otherwise surface the same actionable + // error we already use for a held-but-unhealthy port instead of silently + // trusting the listener. + if (allowAdopt) { + return { action: "adopt" }; + } + return { + action: "error", + message: + `Port ${port} is already serving a healthy response, but adopting an ` + + `existing listener is disabled by default (a 2xx cannot prove the listener ` + + `is this service). Set OMNIROUTE_ADOPT_EXISTING_SERVICE=1 to allow adoption, ` + + `or stop the process holding the port and start the service again.`, + }; } // Port is held but nothing healthy answers: an orphaned or unrelated process // is squatting on it. Surface a clear, actionable error instead of letting @@ -59,6 +78,17 @@ export function decidePreSpawn(probe: PreSpawnProbe, port: number): PreSpawnDeci return { action: "spawn" }; } +/** + * Whether the operator opted in to adopting an already-healthy listener on a + * service port. Off by default (GHSA-wg9p-6m2g-4v27): a squatter can answer a + * 2xx, so auto-adoption is only safe when the operator knows the listener is + * genuinely their (externally-managed) instance. + */ +export function isAdoptExistingEnabled(env: NodeJS.ProcessEnv = process.env): boolean { + const v = env.OMNIROUTE_ADOPT_EXISTING_SERVICE; + return v === "1" || v === "true"; +} + /** TCP connect check: resolves true when something accepts a connection. */ function isPortInUse(port: number, timeoutMs: number): Promise { return new Promise((resolve) => { diff --git a/tests/unit/ninerouter-embed-port-6205.test.ts b/tests/unit/ninerouter-embed-port-6205.test.ts index be967d2f53..3306bc557a 100644 --- a/tests/unit/ninerouter-embed-port-6205.test.ts +++ b/tests/unit/ninerouter-embed-port-6205.test.ts @@ -70,11 +70,19 @@ describe("#6205 A — embed panel root no longer 404s", () => { // ─── SUB-BUG B: pre-spawn port/health decision ─────────────────────────────── describe("#6205 B — pre-spawn port probe avoids raw EADDRINUSE", () => { - it("adopts a healthy existing instance (no spawn)", () => { - const decision = decidePreSpawn({ healthy: true, portInUse: true }, 20130); + it("adopts a healthy existing instance when adoption is opted in (no spawn)", () => { + const decision = decidePreSpawn({ healthy: true, portInUse: true }, 20130, true); assert.equal(decision.action, "adopt"); }); + it("does NOT adopt a healthy listener by default — a 2xx cannot prove identity (GHSA-wg9p-6m2g-4v27)", () => { + const decision = decidePreSpawn({ healthy: true, portInUse: true }, 20130); + assert.equal(decision.action, "error"); + assert.match(decision.message, /adopt/i); + assert.match(decision.message, /OMNIROUTE_ADOPT_EXISTING_SERVICE/); + assert.ok(!decision.message.includes("at /"), "must not leak a stack trace"); + }); + it("returns a clear error object (not a throw) when the port is held but unhealthy", () => { let decision; assert.doesNotThrow(() => { @@ -92,9 +100,10 @@ describe("#6205 B — pre-spawn port probe avoids raw EADDRINUSE", () => { assert.equal(decision.action, "spawn"); }); - it("adopts a healthy instance even if the TCP probe missed it", () => { - // Health is authoritative: a 2xx means a real instance is serving. - const decision = decidePreSpawn({ healthy: true, portInUse: false }, 20130); + it("adopts a healthy instance (opted in) even if the TCP probe missed it", () => { + // With adoption opted in, health is authoritative: a 2xx means a real + // instance is serving even when the TCP connect probe raced and missed it. + const decision = decidePreSpawn({ healthy: true, portInUse: false }, 20130, true); assert.equal(decision.action, "adopt"); }); });