Repository navigation
feat(mitm): TPROXY capture-mode listener + connectMarked (Epic A) - #4169
Conversation
The listener that ties the validated TPROXY primitives into a transparent capture mode for LOCAL outbound traffic (IDE agents on the same host), no /etc/hosts spoofing or OS-wide proxy mutation: - apply the OUTPUT-based rules, open the IP_TRANSPARENT listener - per connection: read the ORIGINAL destination (socket.localAddress — TPROXY preserves it), report it via onIntercept, and forward to it over a connectMarked (SO_MARK bypass) socket so the OUTPUT rule excludes the forward (anti-loop), with a raw bidirectional pipe - stop() closes the listener and reverts the rules (crash-safe) Adds connectMarked(ip, port, mark) to the addon (socket+SO_MARK BEFORE a non-blocking connect, so the forward SYN is excluded) + the wrapper export; the loader now requires all three addon fns. All effectful seams are injected so the orchestration + per-connection logic are unit-tested without root. Validated e2e on the VPS (kernel 6.8.0), isolated to a test port + dedicated fwmark: full tunnel works — client connected, original destination preserved, PONG:PING round-tripped (intercept + connectMarked forward + pipe); anti-loop confirmed (marked SYN excluded, plain SYN looped). 14/14 tproxy unit tests green. TLS termination / content decryption (reusing the MITM core) and the capture-mode route + UI tab remain follow-ups; this layer is a transparent tunnel that surfaces intercepted connections via onIntercept.
|
You have reached your Codex usage limits for code reviews. You can see your limits in the Codex usage dashboard. |
There was a problem hiding this comment.
Code Review
This pull request implements a Linux TPROXY capture-mode listener in TypeScript along with a native C addon function ConnectMarked to support marked sockets for anti-loop forwarding, accompanied by comprehensive unit tests. The feedback highlights critical improvements: adding argument validation in the native C code to prevent potential crashes, implementing a unified socket cleanup handler to avoid resource leaks from half-open sockets, and utilizing server.closeAllConnections() to ensure timely shutdowns when active connections exist.
Important
The consumer version of Gemini Code Assist on GitHub is being sunset. Starting June 18, 2026, new organization installations will be blocked, and all code review activity will officially cease on July 17, 2026.
For more details on the timeline and next steps, please review the Help Documentation.
| static napi_value ConnectMarked(napi_env env, napi_callback_info info) { | ||
| size_t argc = 3; | ||
| napi_value argv[3]; | ||
| napi_get_cb_info(env, info, &argc, argv, NULL, NULL); | ||
| char ip[64] = {0}; | ||
| size_t ip_len = 0; | ||
| napi_get_value_string_utf8(env, argv[0], ip, sizeof(ip), &ip_len); | ||
| int32_t port = 0, mark = 0; | ||
| napi_get_value_int32(env, argv[1], &port); | ||
| napi_get_value_int32(env, argv[2], &mark); |
There was a problem hiding this comment.
In native C addons, failing to validate the number and types of arguments passed from JavaScript can lead to undefined behavior, memory corruption, or hard process crashes (segfaults) if the JS caller passes incorrect or fewer arguments.
We should explicitly check that napi_get_cb_info succeeded, that argc >= 3, and that each argument is of the expected type before using them.
static napi_value ConnectMarked(napi_env env, napi_callback_info info) {
size_t argc = 3;
napi_value argv[3];
if (napi_get_cb_info(env, info, &argc, argv, NULL, NULL) != napi_ok || argc < 3) {
THROW(env, "EINVAL", "Expected 3 arguments: ip, port, mark");
}
char ip[64] = {0};
size_t ip_len = 0;
if (napi_get_value_string_utf8(env, argv[0], ip, sizeof(ip), &ip_len) != napi_ok) {
THROW(env, "EINVAL", "Argument 'ip' must be a string");
}
int32_t port = 0, mark = 0;
if (napi_get_value_int32(env, argv[1], &port) != napi_ok) {
THROW(env, "EINVAL", "Argument 'port' must be an integer");
}
if (napi_get_value_int32(env, argv[2], &mark) != napi_ok) {
THROW(env, "EINVAL", "Argument 'mark' must be an integer");
}| upstream.on("error", () => client.destroy()); | ||
| client.on("error", () => upstream.destroy()); | ||
| client.pipe(upstream); | ||
| upstream.pipe(client); |
There was a problem hiding this comment.
In Node.js, stream.pipe() does not propagate close or destroy events. If the upstream socket is abruptly destroyed (e.g., due to an idle timeout or remote reset), it will emit a 'close' event but not necessarily an 'end' event. This leaves the client socket in a half-open state, leaking the socket and its associated file descriptor.
To prevent resource leaks, we should register a unified cleanup handler that destroys both sockets as soon as either of them emits an 'error' or 'close' event.
const destroyBoth = () => {
client.destroy();
upstream.destroy();
};
client.on("error", destroyBoth);
upstream.on("error", destroyBoth);
client.on("close", destroyBoth);
upstream.on("close", destroyBoth);
client.pipe(upstream);
upstream.pipe(client);| stop: async () => { | ||
| await new Promise<void>((resolve) => server.close(() => resolve())); | ||
| await deps.revertTproxy(cfg).catch(() => {}); | ||
| }, |
There was a problem hiding this comment.
By default, server.close() waits for all active connections to close before invoking its callback. If there are active, long-lived TCP connections (such as IDE agent tunnels), await handle.stop() will hang indefinitely, preventing clean or timely shutdowns.
We should use server.closeAllConnections() (available in Node.js v18.2.0+) to immediately terminate all active connections when stopping the capture mode.
stop: async () => {
const closePromise = new Promise<void>((resolve) => server.close(() => resolve()));
if (typeof server.closeAllConnections === "function") {
server.closeAllConnections();
}
await closePromise;
await deps.revertTproxy(cfg).catch(() => {});
},Integrated into release/v3.8.29 — TLS-terminating capture for TPROXY (decrypt 2/N). New src/mitm/tproxy/tlsCapture.ts: TLS-terminates the intercepted client with a per-SNI leaf from the dynamic CA (#4173), feeds plaintext to an internal http.Server, captures to the Traffic Inspector as source 'tproxy' (sanitizeHeaders + maskSecret), and forwards re-encrypted via an injected seam (realForward uses connectMarked for anti-loop, #4169). Secure-by-default: rejectUnauthorized defaults true; errors via sanitizeErrorMessage; bounded timeouts + closeAllConnections. CaptureSource enum + Zod widened with 'tproxy'. Validated: 17/17 unit tests (real local TLS round-trip + secret masking + 502/error path), targeted typecheck of changed files clean, file-size + test-discovery green.
…eaper-inference#4169) Integrated into release/v3.8.29
…-inference#4179) Integrated into release/v3.8.29 — TLS-terminating capture for TPROXY (decrypt 2/N). New src/mitm/tproxy/tlsCapture.ts: TLS-terminates the intercepted client with a per-SNI leaf from the dynamic CA (cheaper-inference#4173), feeds plaintext to an internal http.Server, captures to the Traffic Inspector as source 'tproxy' (sanitizeHeaders + maskSecret), and forwards re-encrypted via an injected seam (realForward uses connectMarked for anti-loop, cheaper-inference#4169). Secure-by-default: rejectUnauthorized defaults true; errors via sanitizeErrorMessage; bounded timeouts + closeAllConnections. CaptureSource enum + Zod widened with 'tproxy'. Validated: 17/17 unit tests (real local TLS round-trip + secret masking + 502/error path), targeted typecheck of changed files clean, file-size + test-discovery green.
Summary
The listener for Fase 3 / Epic A — ties the validated TPROXY primitives (#4144 setup, #4156 OUTPUT recipe, #4148/#4160 addon) into a transparent capture mode for local outbound traffic (IDE agents on the same host), with no
/etc/hostsspoofing or OS-wide proxy mutation.src/mitm/tproxy/captureMode.ts:IP_TRANSPARENTlistener (addon fd →net.Server).socket.localAddress— TPROXY preserves it), report it viaonIntercept, and forward to it over aconnectMarked(SO_MARK bypass) socket so the OUTPUT rule excludes the forward (anti-loop), with a raw bidirectional pipe.stop()closes the listener and reverts the rules (crash-safe).Adds
connectMarked(ip, port, mark)to the native addon —socket()+SO_MARKbefore a non-blockingconnect(), so the forward's SYN already carries the bypass mark — plus the wrapper export; the loader now requires all three addon functions.All effectful seams are injected (
deps) so the orchestration + per-connection logic are unit-testable without root.Validated end-to-end on the VPS (Hard Rule #18)
Kernel
6.8.0-124, isolated to a test port (9999) + dedicated fwmark (0x2333) so production 443/80 was untouched:Follow-ups (last bits of camera 4)
server.cjs) — this layer is currently a transparent tunnel that surfaces connections viaonIntercept; decrypt is the next sub-layer.repairMitm()callingrevertTproxy()once a TPROXY config is persisted.source:"tproxy"inCaptureSource, addon prebuilds).Test Plan
node --test tproxy-capture-mode.test.ts tproxy-transparent-socket.test.ts— 14/14 (normalizeDest, dest-parse + connectMarked forward, destroy-on-bad-dest, apply/listen/stop+revert order, invalid-config-reverts-nothing, loader requires all 3 fns)npm run build:native:tproxycompiles withconnectMarkedtypecheck:core— 0 ·eslint— 0🤖 Generated with Claude Code