fix(security): don't auto-adopt an unverified listener on a service port

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.
This commit is contained in:
Xiangzhe
2026-08-21 14:10:31 -03:00
parent 60060a6dca
commit 50f5cecb80
3 changed files with 55 additions and 11 deletions

View File

@@ -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,

View File

@@ -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<boolean> {
return new Promise<boolean>((resolve) => {

View File

@@ -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");
});
});