mirror of
https://github.com/diegosouzapw/OmniRoute.git
synced 2026-08-21 22:52:19 +03:00
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:
@@ -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,
|
||||
|
||||
@@ -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) => {
|
||||
|
||||
@@ -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");
|
||||
});
|
||||
});
|
||||
|
||||
Reference in New Issue
Block a user