From 1cb833acc4a7e6d63ac699bb2569833453d0f99e Mon Sep 17 00:00:00 2001 From: diegosouzapw Date: Thu, 28 May 2026 21:06:47 -0300 Subject: [PATCH] docs(mitm): clarify CONNECT handler scope + guard activeConnections double-count (R4 #5) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The round-3 C1 fix added `server.on("connect", ...)` to satisfy plan 11 §4.6's text. R4 architectural review confirmed the handler is essentially dead code on port 443: `https.Server` runs the HTTP parser ABOVE TLS, so a `server.on("connect")` handler only fires for CONNECT-tunneled-inside-TLS (HTTPS-proxy-tunneled-in-TLS), not for the "no config required" AgentBridge DNS-spoof flow where the IDE opens TLS directly to 127.0.0.1:443. Passthrough for unmapped hosts is structurally handled elsewhere (DNS scoping for default mode; httpProxyServer.ts:8080 for System-wide proxy mode). Genuine on-wire bypass-without-decrypt at :443 under direct TLS would require SNI sniffing on the raw 'connection' event — intentionally out of scope for this release. This commit: - adds a block comment above the CONNECT handler explaining the real scope so future contributors don't assume it covers the primary AgentBridge flow - guards the `connection` listener with `socket.__mitmCounted` so the CONNECT "target" branch's `server.emit("connection", clientSocket)` re-entry doesn't double-increment `stats.activeConnections` - adds 2 source-grep regression tests asserting both the doc comment and the guard remain in place C2 (x-omniroute-source/agent headers) was already correct and is unchanged. --- src/mitm/server.cjs | 20 +++++++++++++ tests/unit/mitm-server-connect.test.ts | 40 ++++++++++++++++++++++++++ 2 files changed, 60 insertions(+) diff --git a/src/mitm/server.cjs b/src/mitm/server.cjs index 0543b80a23..eb8890999a 100644 --- a/src/mitm/server.cjs +++ b/src/mitm/server.cjs @@ -547,6 +547,22 @@ function rawTcpForward(clientSocket, head, host, port, label) { }); } +// CONNECT handler — scope note (plan 11 §4.6): +// +// This fires ONLY when a client uses this server as an explicit HTTPS proxy and +// sends a `CONNECT host:port` line *inside* an already-established TLS session +// (HTTPS-proxy-tunneled-in-TLS). The primary "no config required" AgentBridge +// flow does NOT use it: there the IDE is pointed at 127.0.0.1 via /etc/hosts DNS +// spoofing and opens TLS DIRECTLY, so requests are routed by the decrypted Host +// header in the request handler above (target → intercept, otherwise passthrough). +// Likewise, bypass/passthrough for *unmapped* hosts in the DNS-spoof model is +// handled by DNS scoping (only spoofed hosts ever resolve to 127.0.0.1), and the +// System-wide proxy mode (plan 12 §2.5.4) routes through httpProxyServer.ts (:8080), +// which has its own CONNECT handling. This handler is retained for the explicit- +// proxy edge case and to honor the routeBypass precedence (bypass > target > +// passthrough); true on-wire bypass-without-decrypt at :443 under direct TLS would +// require SNI sniffing on the raw 'connection' event, which is intentionally out +// of scope for this release. server.on("connect", (req, clientSocket, head) => { const authority = String(req.url || ""); const { host: connectHost, port: connectPort } = parseConnectAuthority(authority); @@ -589,6 +605,10 @@ server.listen(LOCAL_PORT, () => { }); server.on("connection", (socket) => { + // Guard against double-counting: a CONNECT "target" tunnel re-emits an + // already-counted socket into the TLS layer via emit("connection") above. + if (socket.__mitmCounted) return; + socket.__mitmCounted = true; stats.activeConnections++; writeStats(); socket.on("close", () => { diff --git a/tests/unit/mitm-server-connect.test.ts b/tests/unit/mitm-server-connect.test.ts index bf3307ff2d..8169b43138 100644 --- a/tests/unit/mitm-server-connect.test.ts +++ b/tests/unit/mitm-server-connect.test.ts @@ -273,3 +273,43 @@ test("C1 contract — server.cjs registers a CONNECT handler", async () => { "server.cjs CONNECT path must reply with 200 Connection Established" ); }); + +test("R4 fix #5 — connection listener guards against double-count on re-emit", async () => { + const fs = await import("node:fs"); + const path = await import("node:path"); + const url = await import("node:url"); + const here = path.dirname(url.fileURLToPath(import.meta.url)); + const serverPath = path.resolve(here, "../../src/mitm/server.cjs"); + const src = fs.readFileSync(serverPath, "utf-8"); + // The CONNECT "target" branch calls server.emit("connection", clientSocket) + // which re-enters the connection listener. Without a guard, activeConnections + // would be double-incremented for the same socket. + assert.match( + src, + /socket\.__mitmCounted/, + "connection listener must use socket.__mitmCounted guard to prevent double-count on CONNECT target re-emit" + ); + assert.match( + src, + /if\s*\(\s*socket\.__mitmCounted\s*\)\s*return/, + "connection listener must early-return when socket is already counted" + ); +}); + +test("R4 fix #5 — CONNECT handler scope is documented", async () => { + const fs = await import("node:fs"); + const path = await import("node:path"); + const url = await import("node:url"); + const here = path.dirname(url.fileURLToPath(import.meta.url)); + const serverPath = path.resolve(here, "../../src/mitm/server.cjs"); + const src = fs.readFileSync(serverPath, "utf-8"); + // The CONNECT handler is documented as not exercised by the real DNS-spoof + // flow (it only fires for HTTPS-proxy-tunneled-in-TLS clients). The doc + // comment prevents future contributors from assuming it covers the primary + // AgentBridge flow. + assert.match( + src, + /HTTPS-proxy-tunneled-in-TLS|explicit HTTPS proxy/i, + "CONNECT handler must carry a comment clarifying its real scope" + ); +});