From 76b38203272dd7f804667ab94e3abffb9c3be162 Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza Date: Mon, 13 Jul 2026 23:08:07 -0300 Subject: [PATCH 1/9] fix(executors): forward X-Session-ID/X-Title agent metadata headers (port from 9router#2413) Custom agent clients (e.g. non-OpenCode providers) commonly send X-Session-ID and X-Title headers for upstream request tracking/attribution, but forwardOpencodeClientHeaders() only forwarded x-opencode-* keys plus User-Agent, silently dropping these for every client. Extends the existing case-insensitive allowlist forwarding path with x-session-id/x-title. Reported-by: Atikur Rahman Chitholian (@chitholian) (https://github.com/decolua/9router/issues/2413) --- CHANGELOG.md | 4 +++ open-sse/utils/opencodeHeaders.ts | 19 +++++++++++ tests/unit/refactor-opencodeHeaders.test.ts | 37 +++++++++++++++++++++ 3 files changed, 60 insertions(+) diff --git a/CHANGELOG.md b/CHANGELOG.md index 3ab0f12866b..1f622432f87 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,6 +6,10 @@ ## [3.8.49] β€” TBD +### πŸ› Bug Fixes + +- **fix(executors):** forward agent-supplied `X-Session-ID`/`X-Title` metadata headers to upstream providers β€” previously dropped for every client outside the `x-opencode-*` allowlist. (thanks @chitholian) + --- ## [3.8.48] β€” 2026-07-13 diff --git a/open-sse/utils/opencodeHeaders.ts b/open-sse/utils/opencodeHeaders.ts index 4e1221877cd..8569c8512c4 100644 --- a/open-sse/utils/opencodeHeaders.ts +++ b/open-sse/utils/opencodeHeaders.ts @@ -12,6 +12,15 @@ const OPENCODE_HEADER_KEYS = [ "x-opencode-client", ] as const; +/** + * Common agent-metadata headers used by non-OpenCode clients (custom agents/ + * providers) for upstream request tracking and attribution. Forwarded the same + * way as the x-opencode-* set: case-insensitive lookup, client value wins. + * Added for 9router#2413 β€” these were previously dropped for every client + * outside the OpenCode allowlist. + */ +const AGENT_METADATA_HEADER_KEYS = ["x-session-id", "x-title"] as const; + /** * Case-insensitive lookup for a header in a headers record. */ @@ -26,6 +35,8 @@ function findHeader(headers: Record, name: string): string | und * 1. Forwards User-Agent from clientHeaders via `setUserAgentHeader()` * 2. Forwards x-opencode-session, x-opencode-request, x-opencode-project, * x-opencode-client headers (case-insensitive match) + * 3. Forwards x-session-id, x-title agent-metadata headers (case-insensitive + * match) β€” common conventions used by non-OpenCode agent clients (9router#2413) * * @param headers - The outbound headers record to mutate * @param clientHeaders - The client-provided headers to forward from @@ -60,6 +71,14 @@ export function forwardOpencodeClientHeaders( } } + // 2b. Forward agent-metadata headers (x-session-id, x-title) β€” 9router#2413 + for (const headerName of AGENT_METADATA_HEADER_KEYS) { + const value = findHeader(clientHeaders, headerName); + if (value) { + headers[headerName] = value; + } + } + // 3. OpencodeExecutor-only: synthesize session/request id from fallback headers if (options?.synthesizeRequestId && !headers["x-opencode-session"]) { const sessionAffinity = diff --git a/tests/unit/refactor-opencodeHeaders.test.ts b/tests/unit/refactor-opencodeHeaders.test.ts index b79865b30ee..19a5a1cdf6c 100644 --- a/tests/unit/refactor-opencodeHeaders.test.ts +++ b/tests/unit/refactor-opencodeHeaders.test.ts @@ -116,6 +116,43 @@ describe("forwardOpencodeClientHeaders – x-opencode-* headers", () => { }); }); +// ── agent metadata headers (X-Session-ID / X-Title) β€” 9router#2413 ───────── +// Non-OpenCode agent clients (e.g. custom providers) commonly send X-Session-ID +// and X-Title for upstream request tracking/attribution. These were previously +// dropped for every client outside the x-opencode-* allowlist. + +describe("forwardOpencodeClientHeaders – X-Session-ID / X-Title", () => { + it("forwards X-Session-ID from client headers", () => { + const headers = h(); + const clientHeaders = { "X-Session-ID": "sess-xyz" }; + forwardOpencodeClientHeaders(headers, clientHeaders); + assert.equal(headers["x-session-id"], "sess-xyz"); + }); + + it("forwards X-Title from client headers", () => { + const headers = h(); + const clientHeaders = { "X-Title": "My Agent" }; + forwardOpencodeClientHeaders(headers, clientHeaders); + assert.equal(headers["x-title"], "My Agent"); + }); + + it("matches X-Session-ID / X-Title case-insensitively", () => { + const headers = h(); + const clientHeaders = { "x-session-id": "sess-lower", "x-title": "lower title" }; + forwardOpencodeClientHeaders(headers, clientHeaders); + assert.equal(headers["x-session-id"], "sess-lower"); + assert.equal(headers["x-title"], "lower title"); + }); + + it("still does NOT forward unrelated unknown headers", () => { + const headers = h(); + const clientHeaders = { "X-Session-ID": "sess-1", "X-Random-Other": "nope" }; + forwardOpencodeClientHeaders(headers, clientHeaders); + assert.equal(headers["x-session-id"], "sess-1"); + assert.equal(headers["X-Random-Other"], undefined); + }); +}); + // ── synthesizeRequestId ───────────────────────────────────────────────────── describe("forwardOpencodeClientHeaders – synthesizeRequestId", () => { From 7335557613c345e5f406a2965704c8a438b0d35b Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza Date: Mon, 13 Jul 2026 23:09:29 -0300 Subject: [PATCH 2/9] fix(sse): sanitize non-ok Antigravity streaming error body (port from 9router#2461) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Root cause: the STREAMING branch of AntigravityExecutor.executeOnce() had no !response.ok check at all β€” it unconditionally wrapped the upstream response body in a pass-through TransformStream, unlike the sibling non-streaming branch which already built a sanitized error via buildAntigravityUpstreamError. When Google's 403 error body was binary/non-UTF8 (observed: gzip-magic-byte payloads), those raw bytes were forwarded verbatim, corrupting the client-visible error message ('[ERROR] [403]: '). Fix: add the same !response.ok guard to the streaming branch, routing through buildAntigravityUpstreamError()/buildErrorBody() (hard rule #12) instead of piping unknown bytes through as if they were an SSE stream. Reported-by: Duongkhanhtool (https://github.com/decolua/9router/issues/2461) --- ...461-antigravity-streaming-403-raw-bytes.md | 1 + open-sse/executors/antigravity.ts | 28 +++++++++ ...treaming-error-body-sanitized-2461.test.ts | 63 +++++++++++++++++++ 3 files changed, 92 insertions(+) create mode 100644 changelog.d/fixes/2461-antigravity-streaming-403-raw-bytes.md create mode 100644 tests/unit/antigravity-streaming-error-body-sanitized-2461.test.ts diff --git a/changelog.d/fixes/2461-antigravity-streaming-403-raw-bytes.md b/changelog.d/fixes/2461-antigravity-streaming-403-raw-bytes.md new file mode 100644 index 00000000000..84cf50b3deb --- /dev/null +++ b/changelog.d/fixes/2461-antigravity-streaming-403-raw-bytes.md @@ -0,0 +1 @@ +- **fix(sse):** Antigravity streaming requests that hit a non-ok upstream response (e.g. a 403) no longer pipe the raw upstream bytes straight through to the client β€” a binary/non-UTF8 error body (observed as gzip-magic-byte garbage) is now routed through the same sanitized `buildAntigravityUpstreamError()` path the non-streaming branch already used, instead of corrupting the client-visible error message. Regression guard: `tests/unit/antigravity-streaming-error-body-sanitized-2461.test.ts` β€” thanks @Duongkhanhtool diff --git a/open-sse/executors/antigravity.ts b/open-sse/executors/antigravity.ts index 494d750836e..49b1d8ff37d 100644 --- a/open-sse/executors/antigravity.ts +++ b/open-sse/executors/antigravity.ts @@ -1591,6 +1591,34 @@ export class AntigravityExecutor extends BaseExecutor { }; } + // #2461: a non-ok upstream response (e.g. 403) must never be piped through the + // streaming pass-through below as if it were an SSE body. Google occasionally + // returns non-UTF8/binary error bodies (observed: gzip-magic-byte payloads) for + // 403s on this endpoint; reading/forwarding those raw bytes corrupts the + // client-visible error message. Mirror the non-streaming branch above and build + // a sanitized JSON error via buildAntigravityUpstreamError (hard rule #12) + // instead of streaming unknown bytes straight through. + if (!response.ok) { + const rawBody = await response + .clone() + .text() + .catch(() => ""); + const errorBody = buildAntigravityUpstreamError( + response.status, + response.statusText, + rawBody + ); + return { + response: new Response(JSON.stringify(errorBody), { + status: response.status, + headers: { "Content-Type": "application/json" }, + }), + url, + headers: finalHeaders, + transformedBody: attachToolNameMap(transformedBody, requestToolNameMap), + }; + } + // Streaming path: wrap the response body in a pass-through TransformStream // that extracts remainingCredits from the final SSE chunk(s) without // consuming the stream. The client receives the unmodified SSE data. diff --git a/tests/unit/antigravity-streaming-error-body-sanitized-2461.test.ts b/tests/unit/antigravity-streaming-error-body-sanitized-2461.test.ts new file mode 100644 index 00000000000..41d242c1f59 --- /dev/null +++ b/tests/unit/antigravity-streaming-error-body-sanitized-2461.test.ts @@ -0,0 +1,63 @@ +import { test } from "node:test"; +import assert from "node:assert/strict"; + +import { AntigravityExecutor } from "../../open-sse/executors/antigravity.ts"; +import { + clearAntigravityVersionCache, + seedAntigravityVersionCache, +} from "../../open-sse/services/antigravityVersion.ts"; + +// Ports decolua/9router#2461: a non-ok (e.g. 403) Antigravity upstream response in the +// STREAMING path was piped straight through to the client via a raw pass-through +// TransformStream, with no `response.ok` check at all β€” unlike the non-streaming path, +// which already builds a sanitized error via buildAntigravityUpstreamError. When the +// upstream 403 body is gzip-compressed (or otherwise binary/non-UTF8), those raw bytes +// end up surfaced verbatim in the client-visible error message, corrupting it (reporters +// saw literal control-byte garbage after "[ERROR] [403]:"). +test.afterEach(() => { + clearAntigravityVersionCache(); +}); + +test("AntigravityExecutor.execute (stream=true) sanitizes a non-ok upstream body instead of piping raw bytes", async () => { + const executor = new AntigravityExecutor(); + const originalFetch = globalThis.fetch; + seedAntigravityVersionCache("2026.04.17-test"); + + // Simulate a gzip-compressed 403 body (magic bytes 0x1f 0x8b), the exact shape + // reported upstream β€” reading it as text without decoding produces garbage. + const binaryBody = new Uint8Array([0x1f, 0x8b, 0x08, 0x00, 0x02, 0xff, 0x52, 0x41, 0x4e]); + + globalThis.fetch = async () => + new Response(binaryBody, { + status: 403, + headers: { "Content-Type": "application/json" }, + }); + + try { + const result = await executor.execute({ + model: "antigravity/gemini-2.5-flash", + body: { request: { contents: [] } }, + stream: true, + credentials: { accessToken: "token", projectId: "project-1" }, + log: { debug() {}, warn() {} }, + }); + + assert.equal(result.response.status, 403); + + const bodyText = await result.response.text(); + + // The raw gzip magic bytes must never reach the client-visible error text. + assert.ok( + !bodyText.includes("\x1f\x8b"), + `expected sanitized error body, got raw bytes leaking through: ${JSON.stringify(bodyText)}` + ); + + // Must be routed through buildErrorBody()/buildAntigravityUpstreamError() β€” a clean, + // parseable JSON error shape (hard rule #12), not an arbitrary pass-through stream. + const parsed = JSON.parse(bodyText) as { error?: { message?: string } }; + assert.ok(parsed.error?.message, "expected a structured error.message"); + assert.match(parsed.error.message, /Antigravity upstream error \(403\)/); + } finally { + globalThis.fetch = originalFetch; + } +}); From b17eaed885fcc8671e748374d1cff02396bf42f7 Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza Date: Mon, 13 Jul 2026 23:17:42 -0300 Subject: [PATCH 3/9] chore(changelog): move #2413 entry to changelog.d fragment Consistency with the repo's canonical changelog.d/fixes/ workflow (avoids merge-storm re-conflicts from editing CHANGELOG.md directly). --- CHANGELOG.md | 4 ---- changelog.d/fixes/2413-preserve-agent-headers.md | 1 + 2 files changed, 1 insertion(+), 4 deletions(-) create mode 100644 changelog.d/fixes/2413-preserve-agent-headers.md diff --git a/CHANGELOG.md b/CHANGELOG.md index 1f622432f87..3ab0f12866b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -6,10 +6,6 @@ ## [3.8.49] β€” TBD -### πŸ› Bug Fixes - -- **fix(executors):** forward agent-supplied `X-Session-ID`/`X-Title` metadata headers to upstream providers β€” previously dropped for every client outside the `x-opencode-*` allowlist. (thanks @chitholian) - --- ## [3.8.48] β€” 2026-07-13 diff --git a/changelog.d/fixes/2413-preserve-agent-headers.md b/changelog.d/fixes/2413-preserve-agent-headers.md new file mode 100644 index 00000000000..6353f5dd778 --- /dev/null +++ b/changelog.d/fixes/2413-preserve-agent-headers.md @@ -0,0 +1 @@ +- **fix(executors):** forward agent-supplied `X-Session-ID`/`X-Title` metadata headers to upstream providers β€” previously dropped for every client outside the `x-opencode-*` allowlist. (thanks @chitholian) (#7104) From 8ce4ac5640b8ed01edb4b0a934dcc9d22b84e125 Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza Date: Mon, 13 Jul 2026 23:55:48 -0300 Subject: [PATCH 4/9] fix(sse): handle space-separated arg name/value in Composer tool calls (port from 9router#1811) parseInnerCall only split arg segments on a newline between the arg name and its value. Cursor's live Composer/Auto output has been observed using a single space instead, so those segments were treated as one long (space-containing) arg name with an empty value, silently no-opping Write/tool calls for Composer/Auto models. Fall back to splitting on the first whitespace boundary when no newline is present in the segment. Reported-by: way-art (https://github.com/decolua/9router/issues/1811) --- changelog.d/fixes/1811-composer-space-sep.md | 1 + open-sse/utils/composerToolCalls.ts | 23 +++++++++++++++---- tests/unit/composer-tool-calls.test.ts | 24 ++++++++++++++++++++ 3 files changed, 43 insertions(+), 5 deletions(-) create mode 100644 changelog.d/fixes/1811-composer-space-sep.md diff --git a/changelog.d/fixes/1811-composer-space-sep.md b/changelog.d/fixes/1811-composer-space-sep.md new file mode 100644 index 00000000000..856fab9e17b --- /dev/null +++ b/changelog.d/fixes/1811-composer-space-sep.md @@ -0,0 +1 @@ +- **fix(sse):** Cursor Composer/Auto tool calls that separate the arg name and value with a space instead of a newline (e.g. `path /Users/.../test`) no longer produce empty-valued, malformed argument keys, fixing silent no-op Write/tool calls. (thanks @way-art) diff --git a/open-sse/utils/composerToolCalls.ts b/open-sse/utils/composerToolCalls.ts index d688973ce3d..916903a3ca5 100644 --- a/open-sse/utils/composerToolCalls.ts +++ b/open-sse/utils/composerToolCalls.ts @@ -126,15 +126,28 @@ function parseInnerCall(body: string): { name: string; arguments: string } | nul const args: Record = {}; for (const seg of segments) { if (!seg) continue; - // Each segment is `arg_name\nvalue\n...`. The arg name is the first - // line; everything after the first newline is the value (verbatim, - // including additional newlines). + // Each segment is normally `arg_name\nvalue\n...`: the arg name is the + // first line, everything after the first newline is the value + // (verbatim, including additional newlines). Some live Composer/Auto + // captures instead separate the arg name and value with a single space + // on the same line (no newline at all in the segment) β€” fall back to + // splitting on the first whitespace boundary in that case so the value + // isn't swallowed into an empty-valued, space-containing "arg name". const idxNl = seg.indexOf("\n"); let argName: string; let argValue: string; if (idxNl < 0) { - argName = seg.trim(); - argValue = ""; + const idxSp = seg.search(/\s/); + if (idxSp < 0) { + argName = seg.trim(); + argValue = ""; + } else { + argName = seg.slice(0, idxSp).trim(); + // Unlike the newline-delimited form, a space-delimited value has no + // multi-line content to preserve β€” trim the trailing whitespace left + // over from the boundary with the next `<｜tool▁sep｜>` marker. + argValue = seg.slice(idxSp + 1).trim(); + } } else { argName = seg.slice(0, idxNl).trim(); argValue = seg.slice(idxNl + 1); diff --git a/tests/unit/composer-tool-calls.test.ts b/tests/unit/composer-tool-calls.test.ts index c1133f05135..5fba8abd3a9 100644 --- a/tests/unit/composer-tool-calls.test.ts +++ b/tests/unit/composer-tool-calls.test.ts @@ -214,3 +214,27 @@ test("feedStreamingChunk: noop after done state", () => { assert.equal(out.safeDelta, ""); assert.equal(out.ready, false); }); + +// ─── Regression: space-separated arg name/value (9router#1811) ─────────────── +// Cursor's real Composer/Auto output has been observed using a single space +// (instead of a newline) between the arg name and its value inside a +// <｜tool▁sep｜> segment, e.g. "<｜tool▁sep｜>path /Users/.../test". The parser +// must still extract {path: "/Users/.../test"} rather than treating the whole +// segment as the (empty-valued) arg name. +test("parseComposerToolCalls: parses args separated by a space instead of a newline (Cursor Composer live capture)", () => { + const text = + "<｜tool▁calls▁begin｜><｜tool▁call▁begin｜> Write " + + "<｜tool▁sep｜>path /Users/kabawagang/Desktop/Code/iOS_Review/test " + + "<｜tool▁sep｜>contents 22\n\n<｜tool▁call▁end｜><｜tool▁calls▁end｜>"; + + const result = parseComposerToolCalls(text); + + assert.equal(result.toolCalls.length, 1); + const tc = result.toolCalls[0]; + assert.equal(tc.function.name, "Write"); + const args = JSON.parse(tc.function.arguments); + assert.deepEqual(args, { + path: "/Users/kabawagang/Desktop/Code/iOS_Review/test", + contents: 22, + }); +}); From 88c2e1a4a5048ee72657be3c81e98ac906b0135c Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza Date: Mon, 13 Jul 2026 23:56:05 -0300 Subject: [PATCH 5/9] fix(cli): remove MITM DNS spoof entries before killing server process (port from 9router#1809) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit stopMitm() killed the spawned MITM server process first and only removed the /etc/hosts DNS-spoof entries afterward. During that window any client whose DNS still resolved a target host to 127.0.0.1 but whose MITM listener was already dead got connect ECONNREFUSED 127.0.0.1:443 β€” exactly the community-confirmed workaround (stop DNS before stopping the server) proves. Swap the two steps so DNS is always cleared first, mirroring the ordering already used by repairMitm() and handleExitCleanup(). Reported-by: dionisius95 (https://github.com/decolua/9router/issues/1809) --- .../fixes/1809-mitm-stop-dns-before-kill.md | 1 + src/mitm/manager.ts | 71 ++++++++++++---- .../mitm-stop-dns-before-kill-1809.test.ts | 82 +++++++++++++++++++ 3 files changed, 137 insertions(+), 17 deletions(-) create mode 100644 changelog.d/fixes/1809-mitm-stop-dns-before-kill.md create mode 100644 tests/unit/mitm-stop-dns-before-kill-1809.test.ts diff --git a/changelog.d/fixes/1809-mitm-stop-dns-before-kill.md b/changelog.d/fixes/1809-mitm-stop-dns-before-kill.md new file mode 100644 index 00000000000..06e42e461ce --- /dev/null +++ b/changelog.d/fixes/1809-mitm-stop-dns-before-kill.md @@ -0,0 +1 @@ +- **fix(cli):** `stopMitm()` now removes /etc/hosts DNS-spoof entries before killing the MITM server process, closing the window where a client's DNS still resolved a target host to `127.0.0.1` while nothing was listening there β€” the cause of `connect ECONNREFUSED 127.0.0.1:443` right after stopping the MITM proxy (thanks @dionisius95). diff --git a/src/mitm/manager.ts b/src/mitm/manager.ts index 47765bf5156..ba743469db7 100644 --- a/src/mitm/manager.ts +++ b/src/mitm/manager.ts @@ -57,6 +57,17 @@ export function interpretMitmStartupError(stderr: string, port: number): string let serverProcess: ChildProcess | null = null; let serverPid: number | null = null; +/** + * Test-only seam: install a fake server process (and pid) so stopMitm() can be + * exercised without spawning a real MITM child. Not part of the public API β€” + * only intended for unit tests that need to assert stopMitm()'s DNS/kill + * ordering (#1809). No-op in production code paths. + */ +export function __setServerProcessForTest(proc: ChildProcess | null, pid: number | null): void { + serverProcess = proc; + serverPid = pid; +} + // Set while startMitm() is in flight, from the guard check through spawn. // Guards a TOCTOU race: the "already running" check above only trips once // `serverProcess` is assigned by spawn() β€” ~130 lines and several awaits @@ -710,10 +721,51 @@ async function startMitmInternal( /** * Stop MITM proxy + * + * Ordering is deliberate and load-bearing (#1809 β€” "connect ECONNREFUSED + * 127.0.0.1:443" after stop). DNS entries MUST be removed BEFORE the server + * process is killed: if the process dies first, any client whose DNS still + * resolves the target host to 127.0.0.1 (from startMitm()'s spoof) but whose + * MITM listener is already dead gets ECONNREFUSED against a dead port for the + * whole window between the two steps. Removing DNS first closes that window β€” + * once /etc/hosts no longer points at 127.0.0.1, clients fall back to real + * resolution regardless of when the listener actually goes away. This mirrors + * the DNS-first ordering already used by repairMitm() and handleExitCleanup(). * @param {string} sudoPassword - Sudo password for DNS cleanup + * @param _depsOverride - optional dependency override, used in tests for DI. */ -export async function stopMitm(sudoPassword: string): Promise<{ running: false; pid: null }> { - // 1. Kill server process (in-memory or from PID file) +export async function stopMitm( + sudoPassword: string, + _depsOverride?: { + removeDNSEntry?: (sudoPassword: string) => Promise; + removeDNSEntries?: (hosts: string[], sudoPassword: string) => Promise; + collectManagedHosts?: () => string[]; + } +): Promise<{ running: false; pid: null }> { + const deps = { + removeDNSEntry: _depsOverride?.removeDNSEntry ?? removeDNSEntry, + removeDNSEntries: _depsOverride?.removeDNSEntries ?? removeDNSEntries, + collectManagedHosts: _depsOverride?.collectManagedHosts ?? collectManagedHosts, + }; + + // 1. Remove DNS entries FIRST β€” 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). Doing this before the process kill closes the ECONNREFUSED + // window described above (#1809). + log.info("Removing DNS entries..."); + await deps.removeDNSEntry(sudoPassword); + try { + const managed = deps.collectManagedHosts(); + if (managed.length > 0) { + await deps.removeDNSEntries(managed, sudoPassword); + } + } catch (err) { + log.error({ err }, "Failed to remove managed DNS entries during stop (continuing)"); + } + + // 2. Kill server process (in-memory or from PID file) const proc = serverProcess; if (proc && !proc.killed) { log.info("Stopping MITM server..."); @@ -745,21 +797,6 @@ export async function stopMitm(sudoPassword: string): Promise<{ running: false; serverPid = null; } - // 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 try { diff --git a/tests/unit/mitm-stop-dns-before-kill-1809.test.ts b/tests/unit/mitm-stop-dns-before-kill-1809.test.ts new file mode 100644 index 00000000000..3dd92dcaad0 --- /dev/null +++ b/tests/unit/mitm-stop-dns-before-kill-1809.test.ts @@ -0,0 +1,82 @@ +/** + * Regression test for upstream issue #1809: "connect ECONNREFUSED 127.0.0.1:443" + * after stopping the MITM proxy. + * + * Root cause: stopMitm() killed the spawned MITM server process FIRST, and only + * removed the /etc/hosts DNS-spoof entries AFTER. During that window any client + * whose DNS still resolved the target host to 127.0.0.1 (from startMitm's spoof) + * but whose MITM listener was already dead got ECONNREFUSED β€” exactly the + * community-confirmed workaround ("stop DNS before stopping the server") proves. + * + * This test drives stopMitm() with real DI: a fake serverProcess standing in for + * the spawned MITM child, and dependency-injected DNS-removal functions that + * record the order in which they are invoked relative to the process kill. The + * fix must remove DNS entries before killing the server process so no window + * exists where DNS points at 127.0.0.1 with nothing listening there. + * + * Uses the project's DATA_DIR-tmp + resetDbInstance pattern so the Node native + * test runner does not hang on open SQLite handles (CLAUDE.md PII learning #3). + */ +import test from "node:test"; +import assert from "node:assert/strict"; +import fs from "node:fs"; +import os from "node:os"; +import path from "node:path"; +import { EventEmitter } from "node:events"; + +const TEST_DATA_DIR = fs.mkdtempSync(path.join(os.tmpdir(), "omniroute-mitm-stop-order-")); +process.env.DATA_DIR = TEST_DATA_DIR; + +const core = await import("../../src/lib/db/core.ts"); +const manager = await import("../../src/mitm/manager.ts"); + +test.after(() => { + core.resetDbInstance(); + fs.rmSync(TEST_DATA_DIR, { recursive: true, force: true }); +}); + +test("stopMitm removes DNS entries before killing the MITM server process (#1809)", async () => { + const events: string[] = []; + + // Fake child process standing in for the spawned MITM server. + const fakeProc = new EventEmitter() as EventEmitter & { + killed: boolean; + kill: (signal?: string) => boolean; + }; + fakeProc.killed = false; + fakeProc.kill = (signal?: string) => { + events.push(`kill:${signal}`); + fakeProc.killed = true; + return true; + }; + + manager.__setServerProcessForTest(fakeProc as unknown as import("child_process").ChildProcess, 4242); + + const removeDNSEntry = async () => { + events.push("removeDNSEntry"); + }; + const removeDNSEntries = async () => { + events.push("removeDNSEntries"); + }; + const collectManagedHosts = () => ["fake.example.test"]; + + await manager.stopMitm("fake-sudo-password", { + removeDNSEntry, + removeDNSEntries, + collectManagedHosts, + }); + + const firstKillIndex = events.findIndex((e) => e.startsWith("kill:")); + const firstDnsIndex = events.findIndex( + (e) => e === "removeDNSEntry" || e === "removeDNSEntries" + ); + + assert.ok(firstKillIndex !== -1, "server process kill was never invoked"); + assert.ok(firstDnsIndex !== -1, "DNS removal was never invoked"); + assert.ok( + firstDnsIndex < firstKillIndex, + `DNS entries must be removed BEFORE the MITM server process is killed ` + + `(got order: ${JSON.stringify(events)}) β€” otherwise a client whose DNS still ` + + `points at 127.0.0.1 hits a dead listener and gets ECONNREFUSED (#1809)` + ); +}); From 0706a932799e8e6c71ce4829773d9a8673ff4568 Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza Date: Mon, 13 Jul 2026 23:56:57 -0300 Subject: [PATCH 6/9] fix(api): check Vercel SSO-protection PATCH response on relay deploy (port from 9router#1037) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The Vercel relay deploy route disabled Deployment Protection (SSO) by firing a PATCH request with .catch(() => {}) and never checking res.ok. When Vercel rejects or no-ops the PATCH (plan doesn't allow disabling protection, an under-scoped token, etc.), the relay was still saved and activated as a healthy proxy pool, and later requests routed through it failed with an undiagnosed 403 Access denied from Vercel's own deployment protection β€” indistinguishable from an upstream-provider rejection (e.g. Codex/ChatGPT edge-IP blocking). Extract disableSsoProtection() to check the PATCH response and surface an ssoProtectionWarning in the deploy response when it fails, so the failure source can be diagnosed instead of silently masked. Reported-by: Rico Aditya (@ricatix) (https://github.com/decolua/9router/issues/1037) --- .../1037-vercel-relay-sso-protection-check.md | 1 + .../api/settings/proxy/vercel-deploy/route.ts | 62 ++++++++++-- ...vercel-deploy-sso-protection-check.test.ts | 95 +++++++++++++++++++ 3 files changed, 149 insertions(+), 9 deletions(-) create mode 100644 changelog.d/fixes/1037-vercel-relay-sso-protection-check.md create mode 100644 tests/unit/vercel-deploy-sso-protection-check.test.ts diff --git a/changelog.d/fixes/1037-vercel-relay-sso-protection-check.md b/changelog.d/fixes/1037-vercel-relay-sso-protection-check.md new file mode 100644 index 00000000000..e03d00dd02d --- /dev/null +++ b/changelog.d/fixes/1037-vercel-relay-sso-protection-check.md @@ -0,0 +1 @@ +- **fix(api):** Vercel Relay deploy now checks the Deployment Protection (SSO) PATCH response and surfaces `ssoProtectionWarning` when Vercel rejects it, instead of silently activating a relay that later returns an undiagnosed `403 Access denied`. (thanks @ricatix) diff --git a/src/app/api/settings/proxy/vercel-deploy/route.ts b/src/app/api/settings/proxy/vercel-deploy/route.ts index 42f432f2c6b..7bbe0032e62 100644 --- a/src/app/api/settings/proxy/vercel-deploy/route.ts +++ b/src/app/api/settings/proxy/vercel-deploy/route.ts @@ -98,6 +98,43 @@ export default async function handler(req) { */ export const __buildRelayFunctionForTest = buildRelayFunction; +/** + * Disable Vercel project SSO/Deployment Protection so the relay is publicly + * reachable. The PATCH response was previously fired-and-forgotten + * (`.catch(() => {})`, no `res.ok` check) β€” if Vercel rejects or no-ops the + * request (plan does not allow disabling protection, an under-scoped token, + * etc.), the relay still got saved and activated as a healthy proxy pool, + * and later requests through it failed with an undiagnosed + * `403 Access denied` from Vercel's own deployment protection. Callers must + * now check `.ok` and surface the failure instead of assuming success. + */ +async function disableSsoProtection( + vercelApiBase: string, + projectId: string, + token: string +): Promise<{ ok: boolean; status?: number }> { + try { + const res = await fetch(`${vercelApiBase}/v9/projects/${projectId}`, { + method: "PATCH", + headers: { + Authorization: `Bearer ${token}`, + "Content-Type": "application/json", + }, + body: JSON.stringify({ ssoProtection: null }), + }); + return { ok: res.ok, status: res.status }; + } catch { + return { ok: false }; + } +} + +/** + * Test-only hook exposing `disableSsoProtection` so the regression test can + * assert the PATCH response is checked instead of silently swallowed. Not + * part of the route contract. + */ +export const __disableSsoProtectionForTest = disableSsoProtection; + async function pollDeployment(deploymentApiUrl: string, token: string): Promise<"READY" | "ERROR"> { for (let i = 0; i < POLL_MAX_ATTEMPTS; i++) { await new Promise((r) => setTimeout(r, POLL_INTERVAL_MS)); @@ -208,16 +245,22 @@ export async function POST(request: Request) { }); } - // Disable Vercel SSO protection so the relay is publicly accessible + // Disable Vercel SSO protection so the relay is publicly accessible. + // The PATCH response is checked β€” if Vercel rejects/no-ops it (plan + // doesn't allow disabling protection, under-scoped token, etc.) the + // relay is still deployed and saved, but the caller is warned so a + // later `403 Access denied` can be diagnosed as Vercel-side deployment + // protection rather than an upstream provider rejection. + let ssoProtectionWarning: string | undefined; if (deployment.projectId) { - await fetch(`${VERCEL_API_BASE}/v9/projects/${deployment.projectId}`, { - method: "PATCH", - headers: { - Authorization: `Bearer ${token}`, - "Content-Type": "application/json", - }, - body: JSON.stringify({ ssoProtection: null }), - }).catch(() => {}); + const ssoResult = await disableSsoProtection(VERCEL_API_BASE, deployment.projectId, token); + if (!ssoResult.ok) { + ssoProtectionWarning = + "Could not disable Vercel Deployment Protection (SSO) for this project" + + (ssoResult.status ? ` (status ${ssoResult.status})` : "") + + ". Requests through this relay may fail with a 403 Access denied from " + + "Vercel until protection is disabled manually in the Vercel dashboard."; + } } // Poll until READY @@ -254,6 +297,7 @@ export async function POST(request: Request) { success: true, relayUrl: `https://${deployment.url}`, poolProxyId: poolProxy?.id, + ...(ssoProtectionWarning ? { ssoProtectionWarning } : {}), }); } catch (error) { return createErrorResponseFromUnknown(error, "Vercel deploy failed"); diff --git a/tests/unit/vercel-deploy-sso-protection-check.test.ts b/tests/unit/vercel-deploy-sso-protection-check.test.ts new file mode 100644 index 00000000000..d3fb945e965 --- /dev/null +++ b/tests/unit/vercel-deploy-sso-protection-check.test.ts @@ -0,0 +1,95 @@ +// Regression guard for upstream report: "Vercel Relay with Codex returns 403 +// Access denied and lacks source diagnostics". +// +// Root cause: the Vercel deploy route disables project SSO/Deployment +// Protection via a PATCH request, but fired it with `.catch(() => {})` and +// never inspected `res.ok`. If Vercel rejects or no-ops the PATCH (plan +// doesn't allow disabling protection, stale/under-scoped token, etc.), the +// relay is still saved and activated as a healthy proxy pool β€” later +// requests routed through it fail with an undiagnosed `403 Access denied` +// from Vercel's own deployment protection, indistinguishable from an +// upstream-provider 403. +// +// Fix: check the PATCH response and surface the failure back to the caller +// (`ssoProtectionWarning` in the JSON response) instead of silently +// swallowing it, so the UI/API consumer can diagnose the 403 source. +import { describe, it, beforeEach, afterEach } from "node:test"; +import assert from "node:assert/strict"; +import { readFileSync } from "node:fs"; +import { join, dirname } from "node:path"; +import { fileURLToPath } from "node:url"; +import { __disableSsoProtectionForTest } from "../../src/app/api/settings/proxy/vercel-deploy/route"; + +const ROOT = join(dirname(fileURLToPath(import.meta.url)), "..", ".."); +const ROUTE_PATH = join( + ROOT, + "src/app/api/settings/proxy/vercel-deploy/route.ts" +); + +describe("disableSsoProtection β€” checks the Vercel PATCH response instead of swallowing it", () => { + const originalFetch = global.fetch; + + afterEach(() => { + global.fetch = originalFetch; + }); + + it("reports failure when Vercel rejects the PATCH (e.g. plan does not allow disabling protection)", async () => { + global.fetch = (async () => + new Response(JSON.stringify({ error: { message: "Forbidden" } }), { + status: 403, + })) as typeof fetch; + + const result = await __disableSsoProtectionForTest( + "https://api.vercel.com", + "proj_123", + "test-token" + ); + + assert.equal(result.ok, false, "must report ok:false on a non-2xx PATCH response"); + assert.equal(result.status, 403); + }); + + it("reports success when Vercel accepts the PATCH", async () => { + global.fetch = (async () => new Response(null, { status: 200 })) as typeof fetch; + + const result = await __disableSsoProtectionForTest( + "https://api.vercel.com", + "proj_123", + "test-token" + ); + + assert.equal(result.ok, true); + }); + + it("reports failure (not a thrown exception) when the PATCH request itself fails", async () => { + global.fetch = (async () => { + throw new Error("network down"); + }) as typeof fetch; + + const result = await __disableSsoProtectionForTest( + "https://api.vercel.com", + "proj_123", + "test-token" + ); + + assert.equal(result.ok, false); + }); +}); + +describe("vercel-deploy route β€” wires the SSO-protection check into the response", () => { + const src = readFileSync(ROUTE_PATH, "utf8"); + + it("no longer fires the PATCH with a silent `.catch(() => {})`", () => { + assert.ok( + !/ssoProtection:\s*null[\s\S]*?\.catch\(\s*\(\)\s*=>\s*\{\s*\}\s*\)/.test(src), + "the ssoProtection PATCH must not be silently swallowed with .catch(() => {})" + ); + }); + + it("surfaces a warning in the JSON response when disabling SSO protection failed", () => { + assert.ok( + src.includes("ssoProtectionWarning"), + "POST handler must surface ssoProtectionWarning in the response payload when the PATCH failed" + ); + }); +}); From 19b295fcaacd0383b9daf39a7475d3c5c89700b8 Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza Date: Tue, 14 Jul 2026 21:53:11 -0300 Subject: [PATCH 7/9] refactor(api): extract vercel-deploy POST helpers to keep the cognitive-complexity ratchet at baseline The SSO-protection check added to POST pushed its cognitive complexity from 15 to 21, regressing the cognitive-complexity ratchet (891 > baseline 890). Extract two pure helpers with identical behavior: - buildDeployErrorResponse(): the sanitized non-ok Vercel deploy response - resolveSsoProtectionWarning(): the SSO PATCH check + warning string POST now reads as a flat sequence of guards. No behavior change. --- .../api/settings/proxy/vercel-deploy/route.ts | 89 ++++++++++++------- 1 file changed, 57 insertions(+), 32 deletions(-) diff --git a/src/app/api/settings/proxy/vercel-deploy/route.ts b/src/app/api/settings/proxy/vercel-deploy/route.ts index 7bbe0032e62..6d6b859396e 100644 --- a/src/app/api/settings/proxy/vercel-deploy/route.ts +++ b/src/app/api/settings/proxy/vercel-deploy/route.ts @@ -135,6 +135,55 @@ async function disableSsoProtection( */ export const __disableSsoProtectionForTest = disableSsoProtection; +/** + * Builds the sanitized error response for a rejected Vercel deployment + * request. Extracted from POST to keep the handler's cognitive complexity + * within the ratchet β€” parses the canonical `{ error: { message } } }` shape + * and never forwards raw upstream error text (may contain project IDs, team + * slugs, deployment hashes or internal Vercel error strings). + */ +async function buildDeployErrorResponse(deployRes: Response) { + let upstreamMessage = "Vercel API rejected the deployment"; + try { + const parsed = (await deployRes.json().catch(() => null)) as { + error?: { message?: string }; + } | null; + const candidate = parsed?.error?.message; + if (typeof candidate === "string" && candidate.trim()) { + upstreamMessage = candidate.trim().slice(0, 200); + } + } catch { + /* fall through to generic message */ + } + return createErrorResponse({ + status: deployRes.status, + message: `Vercel deployment failed: ${upstreamMessage}`, + type: "upstream_error", + }); +} + +/** + * Disables Vercel SSO/Deployment Protection for the deployed project and + * returns a caller-facing warning when it could not be disabled. Extracted + * from POST to keep the handler's cognitive complexity within the ratchet. + * See `disableSsoProtection` doc comment for the bug this guards against. + */ +async function resolveSsoProtectionWarning( + projectId: string | undefined, + vercelApiBase: string, + token: string +): Promise { + if (!projectId) return undefined; + const ssoResult = await disableSsoProtection(vercelApiBase, projectId, token); + if (ssoResult.ok) return undefined; + return ( + "Could not disable Vercel Deployment Protection (SSO) for this project" + + (ssoResult.status ? ` (status ${ssoResult.status})` : "") + + ". Requests through this relay may fail with a 403 Access denied from " + + "Vercel until protection is disabled manually in the Vercel dashboard." + ); +} + async function pollDeployment(deploymentApiUrl: string, token: string): Promise<"READY" | "ERROR"> { for (let i = 0; i < POLL_MAX_ATTEMPTS; i++) { await new Promise((r) => setTimeout(r, POLL_INTERVAL_MS)); @@ -208,27 +257,9 @@ export async function POST(request: Request) { }); if (!deployRes.ok) { - // Avoid forwarding 200 bytes of raw Vercel error text β€” it may contain - // project IDs, team slugs, deployment hashes or internal Vercel error - // strings. Parse the canonical { error: { message } } shape and surface - // only the human-readable message (or a generic fallback). - let upstreamMessage = "Vercel API rejected the deployment"; - try { - const parsed = (await deployRes.json().catch(() => null)) as { - error?: { message?: string }; - } | null; - const candidate = parsed?.error?.message; - if (typeof candidate === "string" && candidate.trim()) { - upstreamMessage = candidate.trim().slice(0, 200); - } - } catch { - /* fall through to generic message */ - } - return createErrorResponse({ - status: deployRes.status, - message: `Vercel deployment failed: ${upstreamMessage}`, - type: "upstream_error", - }); + // Avoid forwarding raw Vercel error text β€” it may contain project IDs, + // team slugs, deployment hashes or internal Vercel error strings. + return buildDeployErrorResponse(deployRes); } const deployment = (await deployRes.json()) as { @@ -251,17 +282,11 @@ export async function POST(request: Request) { // relay is still deployed and saved, but the caller is warned so a // later `403 Access denied` can be diagnosed as Vercel-side deployment // protection rather than an upstream provider rejection. - let ssoProtectionWarning: string | undefined; - if (deployment.projectId) { - const ssoResult = await disableSsoProtection(VERCEL_API_BASE, deployment.projectId, token); - if (!ssoResult.ok) { - ssoProtectionWarning = - "Could not disable Vercel Deployment Protection (SSO) for this project" + - (ssoResult.status ? ` (status ${ssoResult.status})` : "") + - ". Requests through this relay may fail with a 403 Access denied from " + - "Vercel until protection is disabled manually in the Vercel dashboard."; - } - } + const ssoProtectionWarning = await resolveSsoProtectionWarning( + deployment.projectId, + VERCEL_API_BASE, + token + ); // Poll until READY const deploymentApiUrl = `${VERCEL_API_BASE}/v13/deployments/${deployment.id}`; From 6eef6828382f91b7eccb71a0ba97e17b767a48c7 Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza Date: Tue, 14 Jul 2026 21:59:17 -0300 Subject: [PATCH 8/9] refactor(mitm): extract repair planning out of manager to respect the file-size cap MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The #1809 DNS-before-kill ordering fix pushed src/mitm/manager.ts to 813 lines, over the 800-line cap check:file-size enforces for non-frozen files. Move the pure repair-planning pieces (collectManagedHosts, the RepairPlan shape and its filesystem/cert/DNS sweep) into a sibling src/mitm/repair.ts. The in-memory session bookkeeping repairMitm() owns β€” cached sudo password, orphaned flag, PID file β€” deliberately stays in manager.ts, so the seam is "plan the repair" vs "own the session". manager.ts is now 731 lines; behavior is unchanged. The DNS-first ordering fix and its regression guard (tests/unit/mitm-stop-dns-before-kill-1809.ts) are untouched and still pass. --- src/mitm/manager.ts | 111 ++++++------------------------------------ src/mitm/repair.ts | 115 ++++++++++++++++++++++++++++++++++++++++++++ 2 files changed, 130 insertions(+), 96 deletions(-) create mode 100644 src/mitm/repair.ts diff --git a/src/mitm/manager.ts b/src/mitm/manager.ts index ba743469db7..99e5cef14da 100644 --- a/src/mitm/manager.ts +++ b/src/mitm/manager.ts @@ -5,15 +5,22 @@ import { resolveMitmDataDir } from "./dataDir.ts"; import { removeDNSEntry, removeDNSEntries } from "./dns/dnsConfig.ts"; import { provisionDnsEntries } from "./dns/provision.ts"; import { generateCert } from "./cert/generate.ts"; -import { installCertResult, uninstallCert } from "./cert/install.ts"; +import { installCertResult } 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"; import { getAllAgentBridgeStates } from "@/lib/db/agentBridgeState.ts"; -import { listCustomHosts } from "@/lib/db/inspectorCustomHosts.ts"; import { getUserBypassPatterns } from "@/lib/db/agentBridgeBypass.ts"; import { configureUpstreamCa } from "./upstreamTrust.ts"; import { createLogger } from "@/shared/utils/logger.ts"; +import { + buildRepairPlan, + collectManagedHosts, + performRepairSteps, + type RepairPlan, +} from "./repair.ts"; + +export { buildRepairPlan, collectManagedHosts, type RepairPlan }; const log = createLogger("mitm-manager"); @@ -230,108 +237,20 @@ 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(); - 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 { - 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`.) + * analogue of ProxyBridge's destructor + `--cleanup`.) Steps 1-3 (DNS, cert, + * system-proxy) are delegated to `./repair.ts::performRepairSteps()`; the PID + * file + in-memory session cleanup below stays here since it touches this + * module's private state. */ 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"); - } + const repaired = await performRepairSteps(sudoPassword); - // 4. Stale PID file. + // Stale PID file. try { if (fs.existsSync(PID_FILE)) fs.unlinkSync(PID_FILE); } catch { diff --git a/src/mitm/repair.ts b/src/mitm/repair.ts new file mode 100644 index 00000000000..4ee2f160f28 --- /dev/null +++ b/src/mitm/repair.ts @@ -0,0 +1,115 @@ +import path from "path"; +import fs from "fs"; +import { resolveMitmDataDir } from "./dataDir.ts"; +import { removeDNSEntry, removeDNSEntries } from "./dns/dnsConfig.ts"; +import { uninstallCert } from "./cert/install.ts"; +import { ALL_TARGETS } from "./targets/index.ts"; +import { listCustomHosts } from "@/lib/db/inspectorCustomHosts.ts"; +import { createLogger } from "@/shared/utils/logger.ts"; + +const log = createLogger("mitm-repair"); + +/** + * 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(); + 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 { + 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; + } +} + +/** + * Run the DNS/cert/system-proxy teardown steps of a repair, WITHOUT touching + * any of `manager.ts`'s in-memory session state (cached password, orphaned + * flag, PID file) β€” that bookkeeping stays in `manager.ts::repairMitm()`, + * which calls this as its first step. Split out purely to keep + * `src/mitm/manager.ts` under the repo's file-size cap; behavior is + * unchanged from the original inline implementation. (Gap 7.) + */ +export async function performRepairSteps(sudoPassword: string): Promise { + 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"); + } + + return repaired; +} From 3de7b97964220fca381a93069e1e17c8fbc1b18a Mon Sep 17 00:00:00 2001 From: Diego Rodrigues de Sa e Souza Date: Wed, 15 Jul 2026 09:43:29 -0300 Subject: [PATCH 9/9] fix(mitm): split stopMitm() DNS/kill steps to fix complexity ratchet regression stopMitm()'s new DNS-before-kill ordering (#1809) pushed its cyclomatic complexity to 18 (max 15), regressing the complexity ratchet from 2056 to 2057. Extract the DNS-removal step and the process-kill step (in-memory + PID-file fallback) into two private helpers, mirroring the existing performRepairSteps() extraction pattern in repair.ts. Behavior unchanged; complexity back at 2056 (cognitive-complexity drops to 889, one under baseline). --- src/mitm/manager.ts | 116 ++++++++++++++++++++++++++------------------ 1 file changed, 70 insertions(+), 46 deletions(-) diff --git a/src/mitm/manager.ts b/src/mitm/manager.ts index 99e5cef14da..94fcf33b691 100644 --- a/src/mitm/manager.ts +++ b/src/mitm/manager.ts @@ -638,6 +638,72 @@ async function startMitmInternal( }; } +/** + * DNS teardown step of stopMitm() (#1809) β€” split out purely to keep + * stopMitm()'s own cyclomatic complexity under the repo's ratchet; behavior + * is unchanged from the original inline implementation. + */ +async function removeStopDnsEntries( + deps: { + removeDNSEntry: (sudoPassword: string) => Promise; + removeDNSEntries: (hosts: string[], sudoPassword: string) => Promise; + collectManagedHosts: () => string[]; + }, + sudoPassword: string +): Promise { + log.info("Removing DNS entries..."); + await deps.removeDNSEntry(sudoPassword); + try { + const managed = deps.collectManagedHosts(); + if (managed.length > 0) { + await deps.removeDNSEntries(managed, sudoPassword); + } + } catch (err) { + log.error({ err }, "Failed to remove managed DNS entries during stop (continuing)"); + } +} + +/** + * Kill the MITM server process during stop β€” either the in-memory + * `serverProcess` handle or, if that's gone, the PID recorded in `PID_FILE`. + * Split out of stopMitm() purely to keep that function's complexity under + * the repo's ratchet; behavior is unchanged from the original inline + * implementation. + */ +async function killMitmServerProcessOnStop(): Promise { + const proc = serverProcess; + if (proc && !proc.killed) { + log.info("Stopping MITM server..."); + proc.kill("SIGTERM"); + await new Promise((resolve) => setTimeout(resolve, 1000)); + if (!proc.killed) { + proc.kill("SIGKILL"); + } + serverProcess = null; + serverPid = null; + return; + } + + // Fallback: kill by PID file + try { + if (fs.existsSync(PID_FILE)) { + const savedPid = parseInt(fs.readFileSync(PID_FILE, "utf-8").trim(), 10); + if (savedPid && isProcessAlive(savedPid)) { + log.info({ pid: savedPid }, "Killing MITM server by PID..."); + process.kill(savedPid, "SIGTERM"); + await new Promise((resolve) => setTimeout(resolve, 1000)); + if (isProcessAlive(savedPid)) { + process.kill(savedPid, "SIGKILL"); + } + } + } + } catch { + // Ignore + } + serverProcess = null; + serverPid = null; +} + /** * Stop MITM proxy * @@ -667,54 +733,12 @@ export async function stopMitm( collectManagedHosts: _depsOverride?.collectManagedHosts ?? collectManagedHosts, }; - // 1. Remove DNS entries FIRST β€” 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). Doing this before the process kill closes the ECONNREFUSED - // window described above (#1809). - log.info("Removing DNS entries..."); - await deps.removeDNSEntry(sudoPassword); - try { - const managed = deps.collectManagedHosts(); - if (managed.length > 0) { - await deps.removeDNSEntries(managed, sudoPassword); - } - } catch (err) { - log.error({ err }, "Failed to remove managed DNS entries during stop (continuing)"); - } + // 1. Remove DNS entries FIRST β€” see function doc + module doc above for why + // this must happen before the process kill (#1809, Gap 8). + await removeStopDnsEntries(deps, sudoPassword); // 2. Kill server process (in-memory or from PID file) - const proc = serverProcess; - if (proc && !proc.killed) { - log.info("Stopping MITM server..."); - proc.kill("SIGTERM"); - await new Promise((resolve) => setTimeout(resolve, 1000)); - if (!proc.killed) { - proc.kill("SIGKILL"); - } - serverProcess = null; - serverPid = null; - } else { - // Fallback: kill by PID file - try { - if (fs.existsSync(PID_FILE)) { - const savedPid = parseInt(fs.readFileSync(PID_FILE, "utf-8").trim(), 10); - if (savedPid && isProcessAlive(savedPid)) { - log.info({ pid: savedPid }, "Killing MITM server by PID..."); - process.kill(savedPid, "SIGTERM"); - await new Promise((resolve) => setTimeout(resolve, 1000)); - if (isProcessAlive(savedPid)) { - process.kill(savedPid, "SIGKILL"); - } - } - } - } catch { - // Ignore - } - serverProcess = null; - serverPid = null; - } + await killMitmServerProcessOnStop(); // 3. Clean up clearCachedPassword(); // Clear password from memory when proxy stops