diff --git a/bin/cli/runtime/nativeDeps.mjs b/bin/cli/runtime/nativeDeps.mjs index 2a0787bd04b..1dc442270e3 100644 --- a/bin/cli/runtime/nativeDeps.mjs +++ b/bin/cli/runtime/nativeDeps.mjs @@ -52,6 +52,31 @@ export function hasModule(name) { return existsSync(join(runtimeModules(), name, "package.json")); } +/** + * Probe whether a native addon (.node) file can actually be dlopen'd by the Node runtime that + * is going to load it. Runs in a throwaway subprocess so a real ABI mismatch (which can segfault + * the process instead of throwing) never takes down the caller — only the probe subprocess. + */ +function probeNativeBinaryLoadable(binary) { + try { + const res = spawnSync( + process.execPath, + [ + "-e", + "try { require(process.argv[1]); process.exit(0); } catch (e) { process.exit(1); }", + binary, + ], + { timeout: 10_000, stdio: "ignore" } + ); + // status === 0 means require() (and therefore dlopen) succeeded. Anything else — a thrown + // ERR_DLOPEN_FAILED/NODE_MODULE_VERSION mismatch (status 1) or a crash (status null with a + // signal, e.g. SIGSEGV) — means the binary is not safe to load. + return res.status === 0; + } catch { + return false; + } +} + export function isBetterSqliteBinaryValid() { const binary = join( runtimeModules(), @@ -68,10 +93,18 @@ export function isBetterSqliteBinaryValid() { closeSync(fd); const magic = buf.toString("hex"); const os = platform(); - if (os === "linux") return magic.startsWith("7f454c46"); // ELF - if (os === "darwin") return magic.startsWith("cffaedfe") || magic.startsWith("cefaedfe"); // Mach-O - if (os === "win32") return magic.startsWith("4d5a"); // PE/MZ - return true; + let formatOk; + if (os === "linux") formatOk = magic.startsWith("7f454c46"); // ELF + else if (os === "darwin") + formatOk = magic.startsWith("cffaedfe") || magic.startsWith("cefaedfe"); // Mach-O + else if (os === "win32") formatOk = magic.startsWith("4d5a"); // PE/MZ + else formatOk = true; + if (!formatOk) return false; + // File-format magic bytes alone do not guarantee the binary was built for the Node ABI + // (NODE_MODULE_VERSION) that will load it — a stale/foreign-ABI binary passes the header + // check and then crashes (segfault) on load instead of triggering a rebuild. Actually + // attempt to load it, isolated in a subprocess. + return probeNativeBinaryLoadable(binary); } catch { return false; } 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/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/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) 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/changelog.d/fixes/2493-better-sqlite3-abi-validation.md b/changelog.d/fixes/2493-better-sqlite3-abi-validation.md new file mode 100644 index 00000000000..dba246809de --- /dev/null +++ b/changelog.d/fixes/2493-better-sqlite3-abi-validation.md @@ -0,0 +1 @@ +- **fix(cli):** the runtime self-heal now verifies a cached `better-sqlite3` native binary actually loads for the running Node before trusting it — the old check only inspected the file's magic bytes (ELF/Mach-O/PE header), so a binary built for a different Node ABI passed validation and segfaulted the process on first use instead of triggering a rebuild. (thanks @mrprohack) (#7105) 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/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/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/src/mitm/manager.ts b/src/mitm/manager.ts index 47765bf5156..94fcf33b691 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"); @@ -57,6 +64,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 @@ -219,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 { @@ -709,11 +639,38 @@ async function startMitmInternal( } /** - * Stop MITM proxy - * @param {string} sudoPassword - Sudo password for DNS cleanup + * 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. */ -export async function stopMitm(sudoPassword: string): Promise<{ running: false; pid: null }> { - // 1. Kill server process (in-memory or from PID file) +async function killMitmServerProcessOnStop(): Promise { const proc = serverProcess; if (proc && !proc.killed) { log.info("Stopping MITM server..."); @@ -724,41 +681,64 @@ export async function stopMitm(sudoPassword: string): Promise<{ running: false; } 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"); - } + 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; + } catch { + // Ignore } + serverProcess = null; + 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)"); +/** + * 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, + _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 — 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) + await killMitmServerProcessOnStop(); // 3. Clean up clearCachedPassword(); // Clear password from memory when proxy stops 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; +} 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; + } +}); diff --git a/tests/unit/cli-runtime.test.ts b/tests/unit/cli-runtime.test.ts index 46a3e3204cc..293f6a39334 100644 --- a/tests/unit/cli-runtime.test.ts +++ b/tests/unit/cli-runtime.test.ts @@ -71,20 +71,60 @@ test("buildEnvWithRuntime preserva NODE_PATH existente", async () => { assert.ok(env.NODE_PATH.includes("/existing/path"), "NODE_PATH original deve ser preservado"); }); -test("isBetterSqliteBinaryValid detecta ELF magic bytes (Linux)", async () => { +test("isBetterSqliteBinaryValid rejeita binário com magic bytes válidos mas ABI incompatível (regressão #2493)", async () => { + // Regression for upstream 9router#2493: a binary that only "looks" native (correct ELF/Mach-O/PE + // header) but was built for a different Node ABI (NODE_MODULE_VERSION) must NOT be reported as + // valid — loading it crashes the process (segfault) instead of triggering a rebuild. const { getRuntimeNodeModules, isBetterSqliteBinaryValid } = await import("../../bin/cli/runtime/nativeDeps.mjs"); const nm = getRuntimeNodeModules(); const buildDir = join(nm, "better-sqlite3", "build", "Release"); mkdirSync(buildDir, { recursive: true }); const binary = join(buildDir, "better_sqlite3.node"); - const buf = Buffer.from([0x7f, 0x45, 0x4c, 0x46, 0x00, 0x00, 0x00, 0x00]); + const { platform } = await import("node:os"); + const os = platform(); + // Correct file-format magic bytes for the current OS, but not a real, loadable native addon — + // this is exactly what the old magic-bytes-only check let through. + const magicByPlatform = { + linux: [0x7f, 0x45, 0x4c, 0x46], + darwin: [0xcf, 0xfa, 0xed, 0xfe], + win32: [0x4d, 0x5a], + }; + const magic = magicByPlatform[os] ?? magicByPlatform.linux; + const buf = Buffer.concat([Buffer.from(magic), Buffer.alloc(64, 0)]); writeFileSync(binary, buf); const result = isBetterSqliteBinaryValid(); - const { platform } = await import("node:os"); - if (platform() === "linux") { - assert.equal(result, true, "ELF magic bytes devem ser válidos no Linux"); + assert.equal( + result, + false, + "binário com header válido mas ABI/conteúdo incompatível deve ser inválido" + ); + rmSync(join(nm, "better-sqlite3"), { recursive: true, force: true }); +}); + +test("isBetterSqliteBinaryValid aceita um binário nativo real e carregável", async () => { + const { getRuntimeNodeModules, isBetterSqliteBinaryValid } = + await import("../../bin/cli/runtime/nativeDeps.mjs"); + const { existsSync, copyFileSync } = await import("node:fs"); + const realBinary = join( + process.cwd(), + "node_modules", + "better-sqlite3", + "build", + "Release", + "better_sqlite3.node" + ); + if (!existsSync(realBinary)) { + // Ambient runtime without a compiled better-sqlite3 binary — nothing to assert here. + return; } + const nm = getRuntimeNodeModules(); + const buildDir = join(nm, "better-sqlite3", "build", "Release"); + mkdirSync(buildDir, { recursive: true }); + const binary = join(buildDir, "better_sqlite3.node"); + copyFileSync(realBinary, binary); + const result = isBetterSqliteBinaryValid(); + assert.equal(result, true, "um binário real, compatível com o Node atual, deve ser válido"); rmSync(join(nm, "better-sqlite3"), { recursive: true, force: true }); }); 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, + }); +}); 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)` + ); +}); 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", () => {