-
-
Notifications
You must be signed in to change notification settings - Fork 10.6k
fix(mitm): crash-safe system-state teardown + socket timeouts (ProxyBridge-inspired hardening) #4084
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
fix(mitm): crash-safe system-state teardown + socket timeouts (ProxyBridge-inspired hardening) #4084
Changes from all commits
1a871aa
934dcea
8a768fc
bfb00e4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,35 @@ | ||
| /** | ||
| * POST /api/tools/agent-bridge/repair | ||
| * | ||
| * Undo orphaned MITM system state (DNS spoof entries, root CA, system proxy) | ||
| * left behind by a crash or SIGKILL. Idempotent — safe to call when state is | ||
| * already clean. LOCAL_ONLY: covered by the "/api/tools/agent-bridge/" prefix | ||
| * in routeGuard.ts (Hard Rules #15 + #17). | ||
| * | ||
| * Gap 7 — the application-layer analogue of ProxyBridge's `--cleanup` flag. | ||
| */ | ||
| import { z } from "zod"; | ||
| import { repairMitm, getCachedPassword } from "@/mitm/manager"; | ||
| import { sanitizeErrorMessage } from "@omniroute/open-sse/utils/error"; | ||
| import { createErrorResponse } from "@/lib/api/errorResponse"; | ||
|
|
||
| // Exported for unit testing. Next.js only treats GET/POST/etc. as route | ||
| // handlers; additional named exports are ignored by the App Router. | ||
| export const RepairBodySchema = z.object({ | ||
| sudoPassword: z.string().optional(), | ||
| }); | ||
|
Comment on lines
+18
to
+20
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. According to the Repository Style Guide (Rule 8), all input validation schemas must be defined in or imported from Please move import { RepairBodySchema } from "@/shared/validation/schemas";References
|
||
|
|
||
| export async function POST(request: Request): Promise<Response> { | ||
| const raw = await request.json().catch(() => ({})); | ||
| const parsed = RepairBodySchema.safeParse(raw); | ||
| const sudoPassword = | ||
| (parsed.success ? parsed.data.sudoPassword : undefined) ?? getCachedPassword() ?? ""; | ||
|
|
||
| try { | ||
| const result = await repairMitm(sudoPassword); | ||
| return Response.json({ ok: true, repaired: result.repaired }); | ||
| } catch (err) { | ||
| const msg = sanitizeErrorMessage(err instanceof Error ? err.message : String(err)); | ||
| return createErrorResponse({ status: 500, message: msg }); | ||
| } | ||
| } | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -19,6 +19,7 @@ import { randomUUID } from "node:crypto"; | |
| import { sanitizeErrorMessage } from "@omniroute/open-sse/utils/error"; | ||
| import { sanitizeHeaders } from "../sanitizeHeaders.ts"; | ||
| import { maskSecret } from "../maskSecrets.ts"; | ||
| import { applyIdleTimeout, MITM_IDLE_TIMEOUT_MS } from "../socketTimeouts.ts"; | ||
| import { globalTrafficBuffer } from "./buffer.ts"; | ||
| import type { InterceptedRequest } from "./types.ts"; | ||
|
|
||
|
|
@@ -238,6 +239,13 @@ export function startHttpProxyServer(port: number = DEFAULT_PORT): Promise<HttpP | |
| return new Promise((resolve, reject) => { | ||
| const server = http.createServer(); | ||
|
|
||
| // Bound request/idle lifetimes + reap idle sockets so hung tunnels cannot | ||
| // exhaust file descriptors under load (Gap 10). | ||
| server.requestTimeout = MITM_IDLE_TIMEOUT_MS * 5; | ||
| server.headersTimeout = MITM_IDLE_TIMEOUT_MS; | ||
| server.keepAliveTimeout = MITM_IDLE_TIMEOUT_MS; | ||
| server.on("connection", (socket) => applyIdleTimeout(socket)); | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Similar to the HTTPS server, setting a raw socket idle timeout on the HTTP proxy server's connection event will unconditionally destroy the socket after 60 seconds of inactivity, even during active HTTP requests. This will prematurely abort slow requests or requests to slow upstream APIs (such as LLM reasoning models with long thinking times). Since the proxy server already configures HTTP-level timeouts ( |
||
|
|
||
| server.on("request", (req, res) => handleHttp(req, res)); | ||
| server.on("connect", (req, socket, head) => handleConnect(req, socket as net.Socket, head)); | ||
|
|
||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -2,9 +2,9 @@ import { spawn, type ChildProcess } from "child_process"; | |
| import path from "path"; | ||
| import fs from "fs"; | ||
| import { resolveMitmDataDir } from "./dataDir.ts"; | ||
| import { addDNSEntry, addDNSEntries, removeDNSEntry } from "./dns/dnsConfig.ts"; | ||
| import { addDNSEntry, addDNSEntries, removeDNSEntry, removeDNSEntries } from "./dns/dnsConfig.ts"; | ||
| import { generateCert } from "./cert/generate.ts"; | ||
| import { installCert } from "./cert/install.ts"; | ||
| import { installCert, uninstallCert } 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"; | ||
|
|
@@ -56,6 +56,15 @@ export function interpretMitmStartupError(stderr: string, port: number): string | |
| let serverProcess: ChildProcess | null = null; | ||
| let serverPid: number | null = null; | ||
|
|
||
| // 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.) | ||
| let _orphanedStateDetected = false; | ||
|
|
||
| // Guards installCleanupHandlers() so the parent-process signal handlers are | ||
| // registered at most once. (Gap 7.) | ||
| let _cleanupHandlersInstalled = false; | ||
|
|
||
| // Module-scoped password cache (not exposed on globalThis). | ||
| // Cleared automatically when the MITM proxy is stopped. | ||
| let _cachedPassword: string | null = null; | ||
|
|
@@ -182,6 +191,146 @@ 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`.) | ||
| */ | ||
| export async function repairMitm(sudoPassword: string): Promise<{ repaired: 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"); | ||
| } | ||
|
|
||
| // 4. Stale PID file. | ||
| try { | ||
| if (fs.existsSync(PID_FILE)) fs.unlinkSync(PID_FILE); | ||
| } catch { | ||
| // ignore | ||
| } | ||
|
|
||
| clearCachedPassword(); | ||
| _orphanedStateDetected = false; | ||
| log.info({ repaired }, "repairMitm completed"); | ||
| return { repaired }; | ||
| } | ||
|
|
||
| /** | ||
| * Best-effort JS surrogate for ProxyBridge's library destructor + crash signal | ||
| * handler. On SIGINT/SIGTERM we terminate the spawned child and warn that | ||
| * privileged cleanup (DNS/CA/proxy) still requires a Repair — we have no sudo | ||
| * password in a signal handler. Idempotent; never blocks process exit. (Gap 7.) | ||
| */ | ||
| export function installCleanupHandlers(): void { | ||
| if (_cleanupHandlersInstalled) return; | ||
| _cleanupHandlersInstalled = true; | ||
| const onSignal = (signal: string) => { | ||
| try { | ||
| if (serverProcess && !serverProcess.killed) serverProcess.kill("SIGTERM"); | ||
| } catch { | ||
| // ignore | ||
| } | ||
| log.warn( | ||
| { signal }, | ||
| "MITM parent received signal — child terminated; run Repair if DNS/CA/proxy were applied." | ||
| ); | ||
| }; | ||
| process.once("SIGINT", () => onSignal("SIGINT")); | ||
| process.once("SIGTERM", () => onSignal("SIGTERM")); | ||
|
Comment on lines
+319
to
+331
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. In Node.js, registering a signal listener for To fix this, you should re-send the signal to the parent process using const onSignal = (signal: string) => {
try {
if (serverProcess && !serverProcess.killed) serverProcess.kill("SIGTERM");
} catch {
// ignore
}
log.warn(
{ signal },
"MITM parent received signal — child terminated; run Repair if DNS/CA/proxy were applied."
);
process.kill(process.pid, signal);
};
process.once("SIGINT", () => onSignal("SIGINT"));
process.once("SIGTERM", () => onSignal("SIGTERM")); |
||
| } | ||
|
|
||
| /** | ||
| * Get MITM status | ||
| */ | ||
|
|
@@ -190,6 +339,7 @@ export async function getMitmStatus(): Promise<{ | |
| pid: number | null; | ||
| dnsConfigured: boolean; | ||
| certExists: boolean; | ||
| orphanedStateDetected: boolean; | ||
| }> { | ||
| // Check in-memory process first, then fallback to PID file | ||
| let running = serverProcess !== null && !serverProcess.killed; | ||
|
|
@@ -203,8 +353,12 @@ export async function getMitmStatus(): Promise<{ | |
| running = true; | ||
| pid = savedPid; | ||
| } else { | ||
| // Stale PID file, clean up | ||
| // Stale PID file: the server died without clean teardown. We cannot | ||
| // run privileged cleanup here (no sudo password in a status read), | ||
| // so flag it for the dashboard to offer a one-click Repair. (Gap 7.) | ||
| fs.unlinkSync(PID_FILE); | ||
| _orphanedStateDetected = true; | ||
| log.warn("Stale MITM PID file found — system state may be orphaned (offer Repair)."); | ||
| } | ||
| } | ||
| } catch { | ||
|
|
@@ -225,7 +379,13 @@ export async function getMitmStatus(): Promise<{ | |
| const certDir = path.join(resolveMitmDataDir(), "mitm"); | ||
| const certExists = fs.existsSync(path.join(certDir, "server.crt")); | ||
|
|
||
| return { running, pid, dnsConfigured, certExists }; | ||
| return { | ||
| running, | ||
| pid, | ||
| dnsConfigured, | ||
| certExists, | ||
| orphanedStateDetected: _orphanedStateDetected, | ||
| }; | ||
| } | ||
|
|
||
| /** | ||
|
|
@@ -243,6 +403,9 @@ export async function startMitm( | |
| throw new Error("MITM proxy is already running"); | ||
| } | ||
|
|
||
| // Register best-effort teardown on parent SIGINT/SIGTERM (Gap 7). | ||
| installCleanupHandlers(); | ||
|
|
||
| // 0. Persist the canonical targets.json so server.cjs can pick up the full | ||
| // AgentBridge target registry alongside its hard-coded antigravity baseline. | ||
| try { | ||
|
|
@@ -457,9 +620,20 @@ export async function stopMitm(sudoPassword: string): Promise<{ running: false; | |
| serverPid = null; | ||
| } | ||
|
|
||
| // 2. Remove DNS entry | ||
| log.info("Removing DNS entry..."); | ||
| // 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 | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
According to the Repository Style Guide (Rule 8), all input validation schemas must be defined in or imported from
src/shared/validation/schemas.ts.CertTrustBodySchemais currently defined locally in this file (at line 17), which violates this rule.Please move
CertTrustBodySchematosrc/shared/validation/schemas.tsand import it here.References