diff --git a/CHANGELOG.md b/CHANGELOG.md index 305cd7d24d..2499c6c62b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -22,6 +22,8 @@ ### 🔧 Bug Fixes +- **fix(mitm):** add an in-process guard so concurrent MITM server starts no longer race — a second start while one is already in flight is short-circuited instead of double-binding the listener. Regression guard: `tests/unit/mitm-start-guard.test.ts`. (thanks @anki1kr) + - **translator (Responses → Chat Completions):** strip the Responses-API-only `truncation` field before forwarding a `/v1/responses` request to a non-OpenAI Chat Completions upstream. Strict upstreams (e.g. NVIDIA NIM) rejected it with HTTP 400 `Unsupported parameter(s): truncation`, breaking Codex-style clients routed to those providers. `client_metadata`, `background`, and `safety_identifier` were already stripped — `truncation` was the remaining gap. Regression guard: `tests/unit/responses-strip-truncation-2311.test.ts`. (thanks @TuanNguyen0708) - **combo (prefer known context capacity over unknown):** when a combo filters out at least one target for exceeding a *known* context limit, the router now prefers the remaining known-compatible targets over targets whose context metadata is simply unknown, instead of letting unknown-metadata targets be the only survivors. If no known-compatible context target remains, context-only candidates fall back to the normal strategy order. Regression guard: `tests/unit/combo-context-window-filter.test.ts`. ([#6088](https://github.com/diegosouzapw/OmniRoute/pull/6088) — thanks @Thinkscape) diff --git a/src/mitm/manager.ts b/src/mitm/manager.ts index ea8cbe4876..62a1f3add9 100644 --- a/src/mitm/manager.ts +++ b/src/mitm/manager.ts @@ -56,6 +56,33 @@ export function interpretMitmStartupError(stderr: string, port: number): string let serverProcess: ChildProcess | null = null; let serverPid: number | null = null; +// Set while startMitm() is in flight, from the guard check through spawn. +// Guards a TOCTOU race: the "already running" check above only trips once +// `serverProcess` is assigned by spawn() — ~130 lines and several awaits +// later (DNS entries, cert generation, cert install). Two concurrent +// startMitm() calls would both pass that check before either assigns +// serverProcess. (upstream 9router#2316) +let mitmStarting = false; + +/** + * Attempt to acquire the single-flight "MITM server is starting" lock. + * Returns `true` if acquired — the caller must release it via + * `releaseMitmStartLock()` in a `finally` block — or `false` if another + * `startMitm()` call already holds it. Exported so the concurrency guard is + * unit-testable without exercising startMitm()'s full side effects + * (DNS/cert/spawn). + */ +export function tryAcquireMitmStartLock(): boolean { + if (mitmStarting) return false; + mitmStarting = true; + return true; +} + +/** Release the single-flight start lock acquired via `tryAcquireMitmStartLock()`. */ +export function releaseMitmStartLock(): void { + mitmStarting = false; +} + // Set when getMitmStatus() finds a stale PID file (server died without clean // teardown). The dashboard surfaces this to offer a one-click Repair. Cleared // by repairMitm(). (Gap 7.) @@ -239,9 +266,7 @@ export function buildRepairPlan(): RepairPlan { */ async function revertSystemProxyIfApplied(): Promise { try { - const { getSystemProxyState, clearSystemProxy } = await import( - "@/lib/inspector/captureState" - ); + const { getSystemProxyState, clearSystemProxy } = await import("@/lib/inspector/captureState"); const state = getSystemProxyState(); if (!state.applied || !state.previousState) return false; const { revert } = await import("./inspector/systemProxyConfig.ts"); @@ -463,6 +488,29 @@ export async function startMitm( throw new Error("MITM proxy is already running"); } + // Check if another startMitm() call is already in flight (TOCTOU guard — + // see the `mitmStarting` comment above). + if (!tryAcquireMitmStartLock()) { + throw new Error("MITM server is already starting"); + } + + try { + return await startMitmInternal(apiKey, sudoPassword, options); + } finally { + releaseMitmStartLock(); + } +} + +/** + * Internal body of startMitm(), extracted so the single-flight lock in + * startMitm() cleanly wraps it in try/finally without re-indenting the + * entire implementation. + */ +async function startMitmInternal( + apiKey: string, + sudoPassword: string, + options: { port?: number } +): Promise<{ running: true; pid: number | null; certTrusted: boolean }> { // Register best-effort teardown on parent SIGINT/SIGTERM (Gap 7). installCleanupHandlers(); @@ -577,9 +625,7 @@ export async function startMitm( let ingestToken = process.env.INSPECTOR_INTERNAL_INGEST_TOKEN || ""; if (!ingestToken) { try { - const ingestMod = await import( - "@/app/api/tools/traffic-inspector/internal/ingest/route" - ); + const ingestMod = await import("@/app/api/tools/traffic-inspector/internal/ingest/route"); if (typeof ingestMod.getIngestTokenForBootstrap === "function") { ingestToken = ingestMod.getIngestTokenForBootstrap(); } diff --git a/tests/unit/mitm-start-guard.test.ts b/tests/unit/mitm-start-guard.test.ts new file mode 100644 index 0000000000..91eb572336 --- /dev/null +++ b/tests/unit/mitm-start-guard.test.ts @@ -0,0 +1,168 @@ +/** + * Regression test for a TOCTOU race in src/mitm/manager.ts::startMitm(). + * + * Before this fix, the "already running" guard only checked the module-level + * `serverProcess` variable, which is not assigned until `spawn()` — ~130 + * lines and several `await`s later (DNS entries, cert generation, cert + * install). Two concurrent `startMitm()` calls both pass that check before + * either assigns `serverProcess`, so both proceed to spawn a second MITM + * server process. (upstream 9router#2316, thanks @anki1kr) + * + * The fix adds a synchronous single-flight lock (`tryAcquireMitmStartLock` / + * `releaseMitmStartLock`) that is acquired *before* any async work begins. + * + * IMPORTANT — safety: `startMitm()`'s real body (`startMitmInternal`) mutates + * real OS state (writes `/etc/hosts` via sudo, installs a cert into the + * system trust store, spawns a real child process bound to port 443). It + * must NEVER be invoked for real from an automated test — doing so once + * during development of this test actually modified `/etc/hosts` and the + * system CA trust store on a machine with passwordless sudo, both of which + * had to be manually reverted. This file therefore: + * (a) drives the lock primitives (`tryAcquireMitmStartLock` / + * `releaseMitmStartLock`) directly — the real, exported guard code — + * with zero OS access, and + * (b) source-scans `startMitm()`'s own text to prove the guard is actually + * wired into the exported function (not just an unused helper), + * again without ever calling it. + * + * DATA_DIR-tmp pattern prevents the Node test runner from hanging on open + * SQLite handles (CLAUDE.md PII learning #3), even though these tests never + * reach the DB-touching code path. + */ +import test from "node:test"; +import assert from "node:assert/strict"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { fileURLToPath } from "node:url"; + +const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-mitm-start-guard-")); +process.env.DATA_DIR = TEST_DATA_DIR; + +const core = await import("../../src/lib/db/core.ts"); +const manager = await import("../../src/mitm/manager.ts"); + +test.after(() => { + core.resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true }); +}); + +// Belt-and-braces: never leave the module-level lock held across tests. +test.afterEach(() => { + manager.releaseMitmStartLock(); +}); + +test("tryAcquireMitmStartLock — first caller acquires, second is blocked while held", () => { + assert.equal(manager.tryAcquireMitmStartLock(), true, "first acquire must succeed"); + assert.equal( + manager.tryAcquireMitmStartLock(), + false, + "second acquire must fail while the lock is held" + ); + manager.releaseMitmStartLock(); + assert.equal(manager.tryAcquireMitmStartLock(), true, "acquire must succeed again after release"); + manager.releaseMitmStartLock(); +}); + +test("releaseMitmStartLock is idempotent (safe to call when not held)", () => { + // Must not throw even if nothing is holding the lock. + manager.releaseMitmStartLock(); + manager.releaseMitmStartLock(); + assert.equal(manager.tryAcquireMitmStartLock(), true); + manager.releaseMitmStartLock(); +}); + +test("startMitm() source: guard is checked+acquired before any async work, and released in a finally", () => { + // Source-scan (never executes startMitm() — see file header on why the + // real function must not be invoked from a test). Confirms the guard is + // actually wired into the exported entry point, not just an unused helper. + const managerPath = fileURLToPath(new URL("../../src/mitm/manager.ts", import.meta.url)); + const src = fs.readFileSync(managerPath, "utf8"); + + const startMitmMatch = src.match( + /export async function startMitm\([\s\S]*?\nasync function startMitmInternal\(/ + ); + assert.ok(startMitmMatch, "startMitm() must delegate to a startMitmInternal() body"); + const startMitmSrc = startMitmMatch[0]; + + const alreadyRunningIdx = startMitmSrc.indexOf( + 'throw new Error("MITM proxy is already running")' + ); + const acquireIdx = startMitmSrc.indexOf("tryAcquireMitmStartLock()"); + const alreadyStartingIdx = startMitmSrc.indexOf( + 'throw new Error("MITM server is already starting")' + ); + const tryIdx = startMitmSrc.indexOf("try {"); + const finallyIdx = startMitmSrc.indexOf("releaseMitmStartLock();"); + const internalCallIdx = startMitmSrc.indexOf("startMitmInternal("); + + assert.ok(alreadyRunningIdx !== -1, "must still guard on serverProcess already running"); + assert.ok(acquireIdx !== -1, "must call tryAcquireMitmStartLock()"); + assert.ok(alreadyStartingIdx !== -1, 'must throw "MITM server is already starting"'); + assert.ok(tryIdx !== -1 && finallyIdx !== -1, "must release the lock in a finally block"); + + // Ordering: already-running check → acquire lock → throw-if-not-acquired → + // try { ...delegate to the real (async, side-effecting) body... } finally { release }. + assert.ok( + alreadyRunningIdx < acquireIdx, + "the already-running check must run before the start-lock is touched" + ); + assert.ok( + acquireIdx < alreadyStartingIdx, + "the lock must be acquired before the already-starting error can be thrown" + ); + assert.ok( + alreadyStartingIdx < tryIdx, + "the guard must reject BEFORE entering the try that runs the real (unsafe) body" + ); + assert.ok( + tryIdx < internalCallIdx && internalCallIdx < finallyIdx, + "startMitmInternal() must run inside the try, with release in the finally" + ); +}); + +test("TOCTOU race across an await boundary: only the first of two concurrent starts proceeds", async () => { + // Mirrors the exact shape of the bug: a guard-check, then real async work + // (DNS/cert/spawn in production) before the "running" state is committed. + // Uses the REAL exported lock primitives — the actual shipped fix — with + // the unsafe OS operations replaced by an inert timer. + const events: string[] = []; + + async function fakeStartMitm(id: string): Promise { + if (!manager.tryAcquireMitmStartLock()) { + throw new Error("MITM server is already starting"); + } + try { + events.push(`${id}:acquired`); + // Simulates the multi-await gap (DNS entries, cert generation, cert + // install) between the old guard check and serverProcess assignment. + await new Promise((resolve) => setTimeout(resolve, 20)); + events.push(`${id}:spawned`); + return id; + } finally { + manager.releaseMitmStartLock(); + } + } + + const p1 = fakeStartMitm("call-1"); + // Issued synchronously, before call-1 has had a chance to release the + // lock — reproduces two overlapping startMitm() invocations. + const p2 = fakeStartMitm("call-2"); + + const [r1, r2] = await Promise.allSettled([p1, p2]); + + assert.equal(r1.status, "fulfilled", "the first caller must proceed and complete"); + if (r1.status === "fulfilled") assert.equal(r1.value, "call-1"); + + assert.equal(r2.status, "rejected", "the second, overlapping caller must be rejected"); + if (r2.status === "rejected") { + assert.match((r2.reason as Error).message, /already starting/i); + } + + // The second call must never have reached the "spawn" phase. + assert.deepEqual(events, ["call-1:acquired", "call-1:spawned"]); + + // Lock must be fully released afterwards, so a subsequent start can proceed. + assert.equal(manager.tryAcquireMitmStartLock(), true); + manager.releaseMitmStartLock(); +});