fix(cli): remove MITM DNS spoof entries before killing server process (#7117)

* fix(cli): remove MITM DNS spoof entries before killing server process (port from 9router#1809)

stopMitm() killed the spawned MITM server process first and only removed the /etc/hosts DNS-spoof entries afterward. During that window any client whose DNS still resolved a target host to 127.0.0.1 but whose MITM listener was already dead got connect ECONNREFUSED 127.0.0.1:443 — exactly the community-confirmed workaround (stop DNS before stopping the server) proves. Swap the two steps so DNS is always cleared first, mirroring the ordering already used by repairMitm() and handleExitCleanup().

Reported-by: dionisius95 (https://github.com/decolua/9router/issues/1809)

* refactor(mitm): extract repair planning out of manager to respect the file-size cap

The #1809 DNS-before-kill ordering fix pushed src/mitm/manager.ts to 813
lines, over the 800-line cap check:file-size enforces for non-frozen files.

Move the pure repair-planning pieces (collectManagedHosts, the RepairPlan
shape and its filesystem/cert/DNS sweep) into a sibling src/mitm/repair.ts.
The in-memory session bookkeeping repairMitm() owns — cached sudo password,
orphaned flag, PID file — deliberately stays in manager.ts, so the seam is
"plan the repair" vs "own the session".

manager.ts is now 731 lines; behavior is unchanged. The DNS-first ordering
fix and its regression guard (tests/unit/mitm-stop-dns-before-kill-1809.ts)
are untouched and still pass.

* fix(mitm): split stopMitm() DNS/kill steps to fix complexity ratchet regression

stopMitm()'s new DNS-before-kill ordering (#1809) pushed its cyclomatic
complexity to 18 (max 15), regressing the complexity ratchet from 2056 to
2057. Extract the DNS-removal step and the process-kill step (in-memory +
PID-file fallback) into two private helpers, mirroring the existing
performRepairSteps() extraction pattern in repair.ts. Behavior unchanged;
complexity back at 2056 (cognitive-complexity drops to 889, one under
baseline).

changelog.d/fixes/1809-mitm-stop-dns-before-kill.md

@@ -0,0 +1 @@
+- **fix(cli):** `stopMitm()` now removes /etc/hosts DNS-spoof entries before killing the MITM server process, closing the window where a client's DNS still resolved a target host to `127.0.0.1` while nothing was listening there — the cause of `connect ECONNREFUSED 127.0.0.1:443` right after stopping the MITM proxy (thanks @dionisius95).

src/mitm/manager.ts

@@ -5,15 +5,22 @@ import { resolveMitmDataDir } from "./dataDir.ts";
 import { removeDNSEntry, removeDNSEntries } from "./dns/dnsConfig.ts";
 import { provisionDnsEntries } from "./dns/provision.ts";
 import { generateCert } from "./cert/generate.ts";
-import { installCertResult, uninstallCert } from "./cert/install.ts";
+import { installCertResult } from "./cert/install.ts";
 import { ALL_TARGETS } from "./targets/index.ts";
 import { detectAgent } from "./detection/index.ts";
 import type { AgentId, DetectionResult, MitmTarget } from "./types.ts";
 import { getAllAgentBridgeStates } from "@/lib/db/agentBridgeState.ts";
-import { listCustomHosts } from "@/lib/db/inspectorCustomHosts.ts";
 import { getUserBypassPatterns } from "@/lib/db/agentBridgeBypass.ts";
 import { configureUpstreamCa } from "./upstreamTrust.ts";
 import { createLogger } from "@/shared/utils/logger.ts";
+import {
+  buildRepairPlan,
+  collectManagedHosts,
+  performRepairSteps,
+  type RepairPlan,
+} from "./repair.ts";
+
+export { buildRepairPlan, collectManagedHosts, type RepairPlan };
 
 const log = createLogger("mitm-manager");
 
@@ -57,6 +64,17 @@ export function interpretMitmStartupError(stderr: string, port: number): string
 let serverProcess: ChildProcess | null = null;
 let serverPid: number | null = null;
 
+/**
+ * Test-only seam: install a fake server process (and pid) so stopMitm() can be
+ * exercised without spawning a real MITM child. Not part of the public API —
+ * only intended for unit tests that need to assert stopMitm()'s DNS/kill
+ * ordering (#1809). No-op in production code paths.
+ */
+export function __setServerProcessForTest(proc: ChildProcess | null, pid: number | null): void {
+  serverProcess = proc;
+  serverPid = pid;
+}
+
 // 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
@@ -219,108 +237,20 @@ function isProcessAlive(pid: number): boolean {
   }
 }
 
-/**
- * Enumerate every hostname OmniRoute may have written to /etc/hosts during
- * startMitm(): the full agent-target registry plus all custom hosts. Removal
- * via removeDNSEntries() is idempotent (absent entries are skipped), so this
- * set is intentionally over-inclusive — a host that was never spoofed costs
- * nothing to "remove", but a host we forget to list leaks machine-wide.
- * (Gap 8 — clean-stop DNS leak.)
- */
-export function collectManagedHosts(): string[] {
-  const hosts = new Set<string>();
-  for (const target of ALL_TARGETS) {
-    for (const h of target.hosts) hosts.add(h);
-  }
-  try {
-    for (const ch of listCustomHosts()) hosts.add(ch.host);
-  } catch (err) {
-    log.error({ err }, "collectManagedHosts: failed to read custom hosts (continuing)");
-  }
-  return [...hosts];
-}
-
-export interface RepairPlan {
-  dnsHostsToRemove: string[];
-  removeCert: boolean;
-  revertSystemProxy: boolean;
-}
-
-/**
- * Pure description of what a repair must undo. Separated from repairMitm() so
- * the enumeration is unit-testable without touching the OS or requiring sudo.
- * (Gap 7.)
- */
-export function buildRepairPlan(): RepairPlan {
-  return {
-    dnsHostsToRemove: collectManagedHosts(),
-    removeCert: true,
-    revertSystemProxy: true,
-  };
-}
-
-/**
- * Best-effort revert of an applied system proxy. The applied state lives
- * in-memory (captureState), so this only succeeds within the same process that
- * applied it; after a crash the previousState is gone and this is a no-op. DNS
- * + cert teardown are always reversible because they read on-disk state.
- */
-async function revertSystemProxyIfApplied(): Promise<boolean> {
-  try {
-    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");
-    await revert(state.previousState);
-    clearSystemProxy();
-    return true;
-  } catch (err) {
-    log.error({ err }, "revertSystemProxyIfApplied failed (continuing)");
-    return false;
-  }
-}
-
 /**
  * Undo every system mutation startMitm() may have made, WITHOUT requiring the
  * MITM server to be running. Safe to call when state is already clean (every
  * step is idempotent). Used by: the /repair route, the CLI cleanup subcommand,
  * and the stale-PID auto-repair on app startup. (Gap 7 — the application-layer
- * analogue of ProxyBridge's destructor + `--cleanup`.)
+ * analogue of ProxyBridge's destructor + `--cleanup`.) Steps 1-3 (DNS, cert,
+ * system-proxy) are delegated to `./repair.ts::performRepairSteps()`; the PID
+ * file + in-memory session cleanup below stays here since it touches this
+ * module's private state.
  */
 export async function repairMitm(sudoPassword: string): Promise<{ repaired: string[] }> {
-  const plan = buildRepairPlan();
-  const repaired: string[] = [];
+  const repaired = await performRepairSteps(sudoPassword);
 
-  // 1. DNS — remove every host we may have spoofed (idempotent, reads /etc/hosts).
-  try {
-    await removeDNSEntry(sudoPassword);
-    if (plan.dnsHostsToRemove.length > 0) {
-      await removeDNSEntries(plan.dnsHostsToRemove, sudoPassword);
-    }
-    repaired.push("dns");
-  } catch (err) {
-    log.error({ err }, "repairMitm: DNS cleanup failed (continuing)");
-  }
-
-  // 2. Certificate — uninstall the MITM root CA from the trust store.
-  if (plan.removeCert) {
-    try {
-      const certPath = path.join(resolveMitmDataDir(), "mitm", "server.crt");
-      if (fs.existsSync(certPath)) {
-        await uninstallCert(sudoPassword, certPath);
-        repaired.push("cert");
-      }
-    } catch (err) {
-      log.error({ err }, "repairMitm: cert removal failed (continuing)");
-    }
-  }
-
-  // 3. System proxy — best-effort revert if applied in this process.
-  if (plan.revertSystemProxy) {
-    if (await revertSystemProxyIfApplied()) repaired.push("system-proxy");
-  }
-
-  // 4. Stale PID file.
+  // Stale PID file.
   try {
     if (fs.existsSync(PID_FILE)) fs.unlinkSync(PID_FILE);
   } catch {
@@ -709,11 +639,38 @@ async function startMitmInternal(
 }
 
 /**
- * Stop MITM proxy
- * @param {string} sudoPassword - Sudo password for DNS cleanup
+ * DNS teardown step of stopMitm() (#1809) — split out purely to keep
+ * stopMitm()'s own cyclomatic complexity under the repo's ratchet; behavior
+ * is unchanged from the original inline implementation.
  */
-export async function stopMitm(sudoPassword: string): Promise<{ running: false; pid: null }> {
-  // 1. Kill server process (in-memory or from PID file)
+async function removeStopDnsEntries(
+  deps: {
+    removeDNSEntry: (sudoPassword: string) => Promise<void>;
+    removeDNSEntries: (hosts: string[], sudoPassword: string) => Promise<void>;
+    collectManagedHosts: () => string[];
+  },
+  sudoPassword: string
+): Promise<void> {
+  log.info("Removing DNS entries...");
+  await deps.removeDNSEntry(sudoPassword);
+  try {
+    const managed = deps.collectManagedHosts();
+    if (managed.length > 0) {
+      await deps.removeDNSEntries(managed, sudoPassword);
+    }
+  } catch (err) {
+    log.error({ err }, "Failed to remove managed DNS entries during stop (continuing)");
+  }
+}
+
+/**
+ * Kill the MITM server process during stop — either the in-memory
+ * `serverProcess` handle or, if that's gone, the PID recorded in `PID_FILE`.
+ * Split out of stopMitm() purely to keep that function's complexity under
+ * the repo's ratchet; behavior is unchanged from the original inline
+ * implementation.
+ */
+async function killMitmServerProcessOnStop(): Promise<void> {
   const proc = serverProcess;
   if (proc && !proc.killed) {
     log.info("Stopping MITM server...");
@@ -724,41 +681,64 @@ export async function stopMitm(sudoPassword: string): Promise<{ running: false;
     }
     serverProcess = null;
     serverPid = null;
-  } else {
-    // Fallback: kill by PID file
-    try {
-      if (fs.existsSync(PID_FILE)) {
-        const savedPid = parseInt(fs.readFileSync(PID_FILE, "utf-8").trim(), 10);
-        if (savedPid && isProcessAlive(savedPid)) {
-          log.info({ pid: savedPid }, "Killing MITM server by PID...");
-          process.kill(savedPid, "SIGTERM");
-          await new Promise((resolve) => setTimeout(resolve, 1000));
-          if (isProcessAlive(savedPid)) {
-            process.kill(savedPid, "SIGKILL");
-          }
-        }
-      }
-    } catch {
-      // Ignore
-    }
-    serverProcess = null;
-    serverPid = null;
+    return;
   }
 
-  // 2. Remove DNS entries — Antigravity defaults PLUS every agent + custom host
-  //    that startMitm() may have spoofed. removeDNSEntries is idempotent, so
-  //    over-inclusion is safe; under-inclusion leaks /etc/hosts lines that
-  //    hijack resolution machine-wide after stop (Gap 8).
-  log.info("Removing DNS entries...");
-  await removeDNSEntry(sudoPassword);
+  // Fallback: kill by PID file
   try {
-    const managed = collectManagedHosts();
-    if (managed.length > 0) {
-      await removeDNSEntries(managed, sudoPassword);
+    if (fs.existsSync(PID_FILE)) {
+      const savedPid = parseInt(fs.readFileSync(PID_FILE, "utf-8").trim(), 10);
+      if (savedPid && isProcessAlive(savedPid)) {
+        log.info({ pid: savedPid }, "Killing MITM server by PID...");
+        process.kill(savedPid, "SIGTERM");
+        await new Promise((resolve) => setTimeout(resolve, 1000));
+        if (isProcessAlive(savedPid)) {
+          process.kill(savedPid, "SIGKILL");
+        }
+      }
     }
-  } catch (err) {
-    log.error({ err }, "Failed to remove managed DNS entries during stop (continuing)");
+  } catch {
+    // Ignore
   }
+  serverProcess = null;
+  serverPid = null;
+}
+
+/**
+ * Stop MITM proxy
+ *
+ * Ordering is deliberate and load-bearing (#1809 — "connect ECONNREFUSED
+ * 127.0.0.1:443" after stop). DNS entries MUST be removed BEFORE the server
+ * process is killed: if the process dies first, any client whose DNS still
+ * resolves the target host to 127.0.0.1 (from startMitm()'s spoof) but whose
+ * MITM listener is already dead gets ECONNREFUSED against a dead port for the
+ * whole window between the two steps. Removing DNS first closes that window —
+ * once /etc/hosts no longer points at 127.0.0.1, clients fall back to real
+ * resolution regardless of when the listener actually goes away. This mirrors
+ * the DNS-first ordering already used by repairMitm() and handleExitCleanup().
+ * @param {string} sudoPassword - Sudo password for DNS cleanup
+ * @param _depsOverride - optional dependency override, used in tests for DI.
+ */
+export async function stopMitm(
+  sudoPassword: string,
+  _depsOverride?: {
+    removeDNSEntry?: (sudoPassword: string) => Promise<void>;
+    removeDNSEntries?: (hosts: string[], sudoPassword: string) => Promise<void>;
+    collectManagedHosts?: () => string[];
+  }
+): Promise<{ running: false; pid: null }> {
+  const deps = {
+    removeDNSEntry: _depsOverride?.removeDNSEntry ?? removeDNSEntry,
+    removeDNSEntries: _depsOverride?.removeDNSEntries ?? removeDNSEntries,
+    collectManagedHosts: _depsOverride?.collectManagedHosts ?? collectManagedHosts,
+  };
+
+  // 1. Remove DNS entries FIRST — see function doc + module doc above for why
+  //    this must happen before the process kill (#1809, Gap 8).
+  await removeStopDnsEntries(deps, sudoPassword);
+
+  // 2. Kill server process (in-memory or from PID file)
+  await killMitmServerProcessOnStop();
 
   // 3. Clean up
   clearCachedPassword(); // Clear password from memory when proxy stops

src/mitm/repair.ts

@@ -0,0 +1,115 @@
+import path from "path";
+import fs from "fs";
+import { resolveMitmDataDir } from "./dataDir.ts";
+import { removeDNSEntry, removeDNSEntries } from "./dns/dnsConfig.ts";
+import { uninstallCert } from "./cert/install.ts";
+import { ALL_TARGETS } from "./targets/index.ts";
+import { listCustomHosts } from "@/lib/db/inspectorCustomHosts.ts";
+import { createLogger } from "@/shared/utils/logger.ts";
+
+const log = createLogger("mitm-repair");
+
+/**
+ * Enumerate every hostname OmniRoute may have written to /etc/hosts during
+ * startMitm(): the full agent-target registry plus all custom hosts. Removal
+ * via removeDNSEntries() is idempotent (absent entries are skipped), so this
+ * set is intentionally over-inclusive — a host that was never spoofed costs
+ * nothing to "remove", but a host we forget to list leaks machine-wide.
+ * (Gap 8 — clean-stop DNS leak.)
+ */
+export function collectManagedHosts(): string[] {
+  const hosts = new Set<string>();
+  for (const target of ALL_TARGETS) {
+    for (const h of target.hosts) hosts.add(h);
+  }
+  try {
+    for (const ch of listCustomHosts()) hosts.add(ch.host);
+  } catch (err) {
+    log.error({ err }, "collectManagedHosts: failed to read custom hosts (continuing)");
+  }
+  return [...hosts];
+}
+
+export interface RepairPlan {
+  dnsHostsToRemove: string[];
+  removeCert: boolean;
+  revertSystemProxy: boolean;
+}
+
+/**
+ * Pure description of what a repair must undo. Separated from repairMitm() so
+ * the enumeration is unit-testable without touching the OS or requiring sudo.
+ * (Gap 7.)
+ */
+export function buildRepairPlan(): RepairPlan {
+  return {
+    dnsHostsToRemove: collectManagedHosts(),
+    removeCert: true,
+    revertSystemProxy: true,
+  };
+}
+
+/**
+ * Best-effort revert of an applied system proxy. The applied state lives
+ * in-memory (captureState), so this only succeeds within the same process that
+ * applied it; after a crash the previousState is gone and this is a no-op. DNS
+ * + cert teardown are always reversible because they read on-disk state.
+ */
+async function revertSystemProxyIfApplied(): Promise<boolean> {
+  try {
+    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");
+    await revert(state.previousState);
+    clearSystemProxy();
+    return true;
+  } catch (err) {
+    log.error({ err }, "revertSystemProxyIfApplied failed (continuing)");
+    return false;
+  }
+}
+
+/**
+ * Run the DNS/cert/system-proxy teardown steps of a repair, WITHOUT touching
+ * any of `manager.ts`'s in-memory session state (cached password, orphaned
+ * flag, PID file) — that bookkeeping stays in `manager.ts::repairMitm()`,
+ * which calls this as its first step. Split out purely to keep
+ * `src/mitm/manager.ts` under the repo's file-size cap; behavior is
+ * unchanged from the original inline implementation. (Gap 7.)
+ */
+export async function performRepairSteps(sudoPassword: string): Promise<string[]> {
+  const plan = buildRepairPlan();
+  const repaired: string[] = [];
+
+  // 1. DNS — remove every host we may have spoofed (idempotent, reads /etc/hosts).
+  try {
+    await removeDNSEntry(sudoPassword);
+    if (plan.dnsHostsToRemove.length > 0) {
+      await removeDNSEntries(plan.dnsHostsToRemove, sudoPassword);
+    }
+    repaired.push("dns");
+  } catch (err) {
+    log.error({ err }, "repairMitm: DNS cleanup failed (continuing)");
+  }
+
+  // 2. Certificate — uninstall the MITM root CA from the trust store.
+  if (plan.removeCert) {
+    try {
+      const certPath = path.join(resolveMitmDataDir(), "mitm", "server.crt");
+      if (fs.existsSync(certPath)) {
+        await uninstallCert(sudoPassword, certPath);
+        repaired.push("cert");
+      }
+    } catch (err) {
+      log.error({ err }, "repairMitm: cert removal failed (continuing)");
+    }
+  }
+
+  // 3. System proxy — best-effort revert if applied in this process.
+  if (plan.revertSystemProxy) {
+    if (await revertSystemProxyIfApplied()) repaired.push("system-proxy");
+  }
+
+  return repaired;
+}

tests/unit/mitm-stop-dns-before-kill-1809.test.ts

@@ -0,0 +1,82 @@
+/**
+ * Regression test for upstream issue #1809: "connect ECONNREFUSED 127.0.0.1:443"
+ * after stopping the MITM proxy.
+ *
+ * Root cause: stopMitm() killed the spawned MITM server process FIRST, and only
+ * removed the /etc/hosts DNS-spoof entries AFTER. During that window any client
+ * whose DNS still resolved the target host to 127.0.0.1 (from startMitm's spoof)
+ * but whose MITM listener was already dead got ECONNREFUSED — exactly the
+ * community-confirmed workaround ("stop DNS before stopping the server") proves.
+ *
+ * This test drives stopMitm() with real DI: a fake serverProcess standing in for
+ * the spawned MITM child, and dependency-injected DNS-removal functions that
+ * record the order in which they are invoked relative to the process kill. The
+ * fix must remove DNS entries before killing the server process so no window
+ * exists where DNS points at 127.0.0.1 with nothing listening there.
+ *
+ * Uses the project's DATA_DIR-tmp + resetDbInstance pattern so the Node native
+ * test runner does not hang on open SQLite handles (CLAUDE.md PII learning #3).
+ */
+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 { EventEmitter } from "node:events";
+
+const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-mitm-stop-order-"));
+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 });
+});
+
+test("stopMitm removes DNS entries before killing the MITM server process (#1809)", async () => {
+  const events: string[] = [];
+
+  // Fake child process standing in for the spawned MITM server.
+  const fakeProc = new EventEmitter() as EventEmitter & {
+    killed: boolean;
+    kill: (signal?: string) => boolean;
+  };
+  fakeProc.killed = false;
+  fakeProc.kill = (signal?: string) => {
+    events.push(`kill:${signal}`);
+    fakeProc.killed = true;
+    return true;
+  };
+
+  manager.__setServerProcessForTest(fakeProc as unknown as import("child_process").ChildProcess, 4242);
+
+  const removeDNSEntry = async () => {
+    events.push("removeDNSEntry");
+  };
+  const removeDNSEntries = async () => {
+    events.push("removeDNSEntries");
+  };
+  const collectManagedHosts = () => ["fake.example.test"];
+
+  await manager.stopMitm("fake-sudo-password", {
+    removeDNSEntry,
+    removeDNSEntries,
+    collectManagedHosts,
+  });
+
+  const firstKillIndex = events.findIndex((e) => e.startsWith("kill:"));
+  const firstDnsIndex = events.findIndex(
+    (e) => e === "removeDNSEntry" || e === "removeDNSEntries"
+  );
+
+  assert.ok(firstKillIndex !== -1, "server process kill was never invoked");
+  assert.ok(firstDnsIndex !== -1, "DNS removal was never invoked");
+  assert.ok(
+    firstDnsIndex < firstKillIndex,
+    `DNS entries must be removed BEFORE the MITM server process is killed ` +
+      `(got order: ${JSON.stringify(events)}) — otherwise a client whose DNS still ` +
+      `points at 127.0.0.1 hits a dead listener and gets ECONNREFUSED (#1809)`
+  );
+});