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)

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

@@ -57,6 +57,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
@@ -710,10 +721,51 @@ async function startMitmInternal(
 
 /**
  * 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): Promise<{ running: false; pid: null }> {
-  // 1. Kill server process (in-memory or from PID file)
+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 — 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). Doing this before the process kill closes the ECONNREFUSED
+  //    window described above (#1809).
+  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)");
+  }
+
+  // 2. Kill server process (in-memory or from PID file)
   const proc = serverProcess;
   if (proc && !proc.killed) {
     log.info("Stopping MITM server...");
@@ -745,21 +797,6 @@ export async function stopMitm(sudoPassword: string): Promise<{ running: false;
     serverPid = null;
   }
 
-  // 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);
-  try {
-    const managed = collectManagedHosts();
-    if (managed.length > 0) {
-      await removeDNSEntries(managed, sudoPassword);
-    }
-  } catch (err) {
-    log.error({ err }, "Failed to remove managed DNS entries during stop (continuing)");
-  }
-
   // 3. Clean up
   clearCachedPassword(); // Clear password from memory when proxy stops
   try {

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)`
+  );
+});