docs(mitm): clarify CONNECT handler scope + guard activeConnections double-count (R4 #5)

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.
This commit is contained in:
diegosouzapw
2026-05-28 21:06:47 -03:00
parent b34e2cc4e3
commit 1cb833acc4
2 changed files with 60 additions and 0 deletions

View File

@@ -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", () => {

View File

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