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