diff --git a/CHANGELOG.md b/CHANGELOG.md index 11bef1abed..5b67956b90 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -14,6 +14,7 @@ _Living section — bullets land here as PRs merge into `release/v3.8.47` (paral ### 🐛 Bug Fixes +- **fix(cli):** per-agent AgentBridge DNS toggle was broken for 8 of the 9 supported agents, and a failed MITM startup step could orphan the spawned proxy child — `addDNSEntry`/`removeDNSEntry` (`src/mitm/dns/dnsConfig.ts`) always resolved the legacy Antigravity default hosts regardless of which agent's toggle was flipped, so enabling DNS for Cursor/Codex/Claude Code/etc. silently added only `daily-cloudcode-pa.googleapis.com` while the DB recorded `dns_enabled=true` for the selected agent. Both functions now accept an optional `agentId` and resolve hosts via `ALL_TARGETS`; `POST /api/tools/agent-bridge/agents/[id]/dns` passes the route's `id` through and now returns 404 for an id that doesn't match a known target instead of silently falling back. Separately, `startMitmInternal()` (`src/mitm/manager.ts`) now wraps `generateCert()` (log + rethrow), the `provisionDnsEntries()` call, and the PID-file write in try/catch so a mid-startup failure can't orphan the already-spawned MITM child process. On Windows, `addDNSEntries`/`removeDNSEntries` also batch every missing/present entry into a single elevated PowerShell invocation instead of one UAC prompt per host line. Regression guard: `tests/unit/dns-config-generic.test.ts` (agent-specific resolution + batching), `tests/unit/agent-bridge-dns-route-validation.test.ts` (404 for unknown agent id). ([#6338](https://github.com/diegosouzapw/OmniRoute/pull/6338) — thanks @hamsa0x7) - **fix(auth):** an API key restricted via `allowedModels`/`allowedCombos` could bypass that restriction entirely over the Codex Responses-over-WebSocket bridge ([#6564](https://github.com/diegosouzapw/OmniRoute/issues/6564)) — `prepare()` in `src/app/api/internal/codex-responses-ws/route.ts` authenticated the WS bridge's API key (`authenticate()`/`authorizeWebSocketHandshake()`) and honored `allowedConnections`, but never called `enforceApiKeyPolicy()`, the same model/combo policy gate the HTTP `/v1/responses` path enforces via `handleChat()` — so a key scoped to e.g. `combo/model-1.0` could still reach a direct Codex model like `gpt-5.5` through this transport, as long as an eligible Codex OAuth connection existed. The bridge's WS auth token arrives via query params (`api_key`/`token`/`access_token`), not a normal `Authorization` header, so a new `enforceCodexWsApiKeyPolicy()` builds an equivalent `Request` carrying an explicit `Authorization: Bearer ` header and calls `enforceApiKeyPolicy()` against the CLIENT-requested model, before any Codex-specific model remapping or credential selection. Regression guard: `tests/unit/codex-ws-policy-enforcement-6564.test.ts` (a model-restricted key is rejected 403 before reaching credential selection; a combo-restricted key is rejected 403 requesting a disallowed combo; a key that DOES allow the requested model still proceeds past policy). - **fix(security):** loopback-gate `/api/middleware/*` so a leaked JWT over a tunnel can't install or trigger a middleware hook — middleware hooks compile + run arbitrary JS via `new vm.Script` on the request hot path (`src/lib/middleware/registry.ts`), the same RCE class as the already-gated `/api/plugins/*`; `/api/middleware/` is now in `LOCAL_ONLY_API_PREFIXES` so loopback enforcement runs unconditionally before any auth check (Hard Rules #15 + #17). Regression guard: `tests/unit/route-guard-middleware-local-only.test.ts`. ([#6541](https://github.com/diegosouzapw/OmniRoute/pull/6541)) — see PR. (thanks @developerjillur) - **fix(startup):** AgentBridge's MITM server no longer fails to start with `ROUTER_API_KEY is required` on a normal install ([#6403](https://github.com/diegosouzapw/OmniRoute/issues/6403)) — `POST /api/tools/agent-bridge/server` resolved the spawned MITM child's router key from only an explicit `apiKey` body field (never sent by the AgentBridge UI — the schema has no such field) and the `ROUTER_API_KEY` env var (unset by default), so `startMitm()` always received `""` and the child hard-exited, even though OmniRoute already had a usable API key in its own DB. A new `resolveRouterApiKey()` now falls back to `pickApiKeyForInternalUse()` (the same DB-backed selector the combo-health-check / cloud-sync internal probes use), resolving in order: explicit key → `ROUTER_API_KEY` env → an existing DB key. Regression guard: `tests/unit/agentbridge-mitm-router-key-6403.test.ts`. diff --git a/src/app/api/tools/agent-bridge/agents/[id]/dns/route.ts b/src/app/api/tools/agent-bridge/agents/[id]/dns/route.ts index 2aec5c7712..368f8bcded 100644 --- a/src/app/api/tools/agent-bridge/agents/[id]/dns/route.ts +++ b/src/app/api/tools/agent-bridge/agents/[id]/dns/route.ts @@ -11,6 +11,7 @@ import { upsertAgentBridgeState } from "@/lib/db/agentBridgeState"; import { getCachedPassword } from "@/mitm/manager"; import { sanitizeErrorMessage } from "@omniroute/open-sse/utils/error"; import { createErrorResponse } from "@/lib/api/errorResponse"; +import { ALL_TARGETS } from "@/mitm/targets/index"; type Params = { params: { id: string } }; @@ -33,6 +34,12 @@ export async function POST(request: Request, { params }: Params): Promise t.id === id); + if (!target) { + return createErrorResponse({ status: 404, message: `Unknown agent: ${id}` }); + } + const { enabled } = parsed.data; const raw = body as Record; const sudoPassword = @@ -40,9 +47,9 @@ export async function POST(request: Request, { params }: Params): Promise t.id === agentId); + return target?.hosts ?? ANTIGRAVITY_HOSTS; +} + const IS_WIN = process.platform === "win32"; const HOSTS_FILE = IS_WIN ? path.join(process.env.SystemRoot || "C:\\Windows", "System32", "drivers", "etc", "hosts") @@ -108,9 +115,13 @@ function hasHostEntry(hostsContent: string, hostname: string): boolean { * Add /etc/hosts entries for every hostname in `hosts`. * Idempotent — existing entries are not duplicated. * Complies with Hard Rule #13: no string interpolation in shell commands. + * + * On Windows, all missing entries are batched into a single elevated PowerShell + * invocation so the user gets one UAC prompt instead of one per line. */ export async function addDNSEntries(hosts: string[], sudoPassword: string): Promise { const hostsContent = readHostsFile(); + const missingEntries: string[] = []; for (const hostname of hosts) { const lines = dnsLines(hostname); @@ -122,29 +133,23 @@ export async function addDNSEntries(hosts: string[], sudoPassword: string): Prom return parts.length >= 2 && parts[0] === ip && parts.includes(host); }); }); + missingEntries.push(...missing); + } - for (const entry of missing) { - if (IS_WIN) { - // HR#13: build PowerShell command via concat (not template literal) so grep - // for `\${` inside script bodies returns zero hits. Values pass through - // `quotePowerShell()` for single-quote escaping — safe against injection - // since both HOSTS_FILE (OS const) and entry (internal `IP host` string) - // are non-user-supplied. - const cmd = - "Add-Content -LiteralPath " + - quotePowerShell(HOSTS_FILE) + - " -Value " + - quotePowerShell(entry); - await runElevatedPowerShell(cmd); - } else { - // Hard Rule #13: entry is passed as stdin data, not interpolated into the command. - await execFileWithPassword( - "sudo", - ["-S", "tee", "-a", HOSTS_FILE], - sudoPassword, - `${entry}\n` - ); - } + if (missingEntries.length === 0) return; + + if (IS_WIN) { + const psHostsFile = quotePowerShell(HOSTS_FILE); + const psEntries = missingEntries.map((e) => quotePowerShell(e)).join(", "); + const script = "Add-Content -LiteralPath " + psHostsFile + " -Value " + psEntries; + await runElevatedPowerShell(script); + for (const entry of missingEntries) { + console.log(`[DNS] Added entry: ${entry}`); + } + } else { + const data = missingEntries.map((e) => `${e}\n`).join(""); + await execFileWithPassword("sudo", ["-S", "tee", "-a", HOSTS_FILE], sudoPassword, data); + for (const entry of missingEntries) { console.log(`[DNS] Added entry: ${entry}`); } } @@ -168,46 +173,43 @@ fs.writeFileSync(filePath, filtered.join("\\n").replace(/\\n*$/, "\\n")); * Remove /etc/hosts entries for every hostname in `hosts`. * Idempotent — silently skips hosts that are not present. * Complies with Hard Rule #13: HOSTS_FILE and hostname are passed as argv, not interpolated. + * + * On Windows, all hostnames are filtered in a single elevated PowerShell + * invocation so the user gets one UAC prompt instead of one per host. */ export async function removeDNSEntries(hosts: string[], sudoPassword: string): Promise { const hostsContent = readHostsFile(); + const presentHosts = hosts.filter((h) => hasHostEntry(hostsContent, h)); - for (const hostname of hosts) { - if (!hasHostEntry(hostsContent, hostname)) { - console.log(`[DNS] Entry for ${hostname} not present — skipping`); - continue; - } + if (presentHosts.length === 0) return; - try { - if (IS_WIN) { - // HR#13: build PowerShell script via concat (not template literal) so grep - // for `\${` inside script bodies returns zero hits. `psHostsFile` and - // `psTargetHost` are quotePowerShell-escaped values (single-quote escape). - const psHostsFile = quotePowerShell(HOSTS_FILE); - const psTargetHost = quotePowerShell(hostname); - const script = - "\n $hostsFile = " + - psHostsFile + - ";\n $targetHost = " + - psTargetHost + - ";\n $lines = Get-Content -LiteralPath $hostsFile;\n" + - " $filtered = $lines | Where-Object {\n" + - " $parts = ($_ -split '\\s+') | Where-Object { $_ };\n" + - " -not (($parts.Length -ge 2) -and ($parts -contains $targetHost))\n" + - " };\n" + - " Set-Content -LiteralPath $hostsFile -Value $filtered;\n "; - await runElevatedPowerShell(script); - } else { - // Hard Rule #13: HOSTS_FILE and hostname are argv arguments, not interpolated. - await execFileWithPassword( - "sudo", - ["-S", process.execPath, "-e", REMOVE_HOSTS_ENTRY_SCRIPT, HOSTS_FILE, hostname], - sudoPassword - ); - } + if (IS_WIN) { + const psHostsFile = quotePowerShell(HOSTS_FILE); + const psTargets = presentHosts.map((h) => quotePowerShell(h)).join(", "); + const script = + "$hostsFile = " + + psHostsFile + + ";\n $targetHosts = @(" + + psTargets + + ");\n" + + " $lines = Get-Content -LiteralPath $hostsFile;\n" + + " $filtered = $lines | Where-Object {\n" + + " $part = ($_ -split '\\s+') | Where-Object { $_ };\n" + + " -not ($part.Length -ge 2 -and ($targetHosts -contains $part[1]))\n" + + " };\n" + + " Set-Content -LiteralPath $hostsFile -Value $filtered;\n "; + await runElevatedPowerShell(script); + for (const hostname of presentHosts) { + console.log(`[DNS] Removed entries for ${hostname}`); + } + } else { + for (const hostname of presentHosts) { + await execFileWithPassword( + "sudo", + ["-S", process.execPath, "-e", REMOVE_HOSTS_ENTRY_SCRIPT, HOSTS_FILE, hostname], + sudoPassword + ); console.log(`[DNS] Removed entries for ${hostname}`); - } catch (error) { - throw new Error(`Failed to remove DNS entry for ${hostname}: ${getErrorMessage(error)}`); } } } @@ -226,17 +228,19 @@ export function checkDNSEntry(): boolean { } /** - * Add DNS entries for the Antigravity default hosts. + * Add DNS entries for the Antigravity default hosts, or for a specific agent + * when `agentId` is provided. * Delegates to `addDNSEntries` — backward compat wrapper. */ -export async function addDNSEntry(sudoPassword: string): Promise { - await addDNSEntries(ANTIGRAVITY_HOSTS, sudoPassword); +export async function addDNSEntry(sudoPassword: string, agentId?: string): Promise { + await addDNSEntries(resolveHostsForAgent(agentId), sudoPassword); } /** - * Remove DNS entries for the Antigravity default hosts. + * Remove DNS entries for the Antigravity default hosts, or for a specific agent + * when `agentId` is provided. * Delegates to `removeDNSEntries` — backward compat wrapper. */ -export async function removeDNSEntry(sudoPassword: string): Promise { - await removeDNSEntries(ANTIGRAVITY_HOSTS, sudoPassword); +export async function removeDNSEntry(sudoPassword: string, agentId?: string): Promise { + await removeDNSEntries(resolveHostsForAgent(agentId), sudoPassword); } diff --git a/src/mitm/manager.ts b/src/mitm/manager.ts index 856c3c84fd..47765bf515 100644 --- a/src/mitm/manager.ts +++ b/src/mitm/manager.ts @@ -552,7 +552,12 @@ async function startMitmInternal( const certPath = path.join(resolveMitmDataDir(), "mitm", "server.crt"); if (!fs.existsSync(certPath)) { log.info("Generating SSL certificate..."); - await generateCert(); + try { + await generateCert(); + } catch (err) { + log.error({ err }, "Failed to generate SSL certificate"); + throw err; + } } // 2. Install certificate to system keychain. A failure here must NOT abort the @@ -576,7 +581,11 @@ async function startMitmInternal( // 3. Add DNS entries: Antigravity defaults + all agents with dns_enabled=true + // all custom hosts with enabled=true. Best-effort — see provisionDnsEntries. log.info("Adding DNS entries..."); - await provisionDnsEntries(sudoPassword); + try { + await provisionDnsEntries(sudoPassword); + } catch (err) { + log.error({ err }, "DNS provisioning threw unexpectedly (continuing)"); + } // 4. Start MITM server log.info("Starting MITM server..."); @@ -619,9 +628,13 @@ async function startMitmInternal( const proc = serverProcess; serverPid = proc.pid ?? null; - // Save PID to file + // Save PID to file — best-effort, must not orphan spawned child process if (serverPid !== null) { - fs.writeFileSync(PID_FILE, String(serverPid)); + try { + fs.writeFileSync(PID_FILE, String(serverPid)); + } catch (err) { + log.error({ err, pid: serverPid }, "Failed to write MITM PID file (continuing)"); + } } // Buffer recent stderr so a startup failure can be reported with its real diff --git a/tests/unit/agent-bridge-dns-route-validation.test.ts b/tests/unit/agent-bridge-dns-route-validation.test.ts new file mode 100644 index 0000000000..5b0cff8712 --- /dev/null +++ b/tests/unit/agent-bridge-dns-route-validation.test.ts @@ -0,0 +1,60 @@ +/** + * Unit tests: POST /api/tools/agent-bridge/agents/[id]/dns — unknown-agent 404 guard. + * + * Before this fix, an unknown `id` fell through straight into `addDNSEntry`/ + * `removeDNSEntry`, which silently resolved the legacy Antigravity default hosts + * instead of rejecting the request. The route now validates `id` against + * `ALL_TARGETS` first and returns 404 for unknown agents — verified here without + * touching /etc/hosts (the 404 short-circuits before any DNS call). + */ +import test from "node:test"; +import assert from "node:assert/strict"; + +const dnsRoute = await import( + "../../src/app/api/tools/agent-bridge/agents/[id]/dns/route.ts" +); + +function makeRequest(body: unknown): Request { + return new Request("http://127.0.0.1/api/tools/agent-bridge/agents/x/dns", { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: JSON.stringify(body), + }); +} + +test("POST .../[id]/dns: unknown agent id returns 404 before any DNS call", async () => { + const res = await dnsRoute.POST(makeRequest({ enabled: true }), { + params: { id: "__nonexistent_agent__" }, + }); + assert.equal(res.status, 404); + const body = (await res.json()) as { error?: { message?: string } }; + assert.ok( + JSON.stringify(body).includes("__nonexistent_agent__"), + "404 body should reference the unknown agent id" + ); +}); + +test("POST .../[id]/dns: invalid body still returns 400 (schema validated first)", async () => { + const res = await dnsRoute.POST(makeRequest({ enabled: "not-a-boolean" }), { + params: { id: "__nonexistent_agent__" }, + }); + assert.equal(res.status, 400); +}); + +test("POST .../[id]/dns: malformed JSON body returns 400", async () => { + const req = new Request("http://127.0.0.1/api/tools/agent-bridge/agents/x/dns", { + method: "POST", + headers: { "Content-Type": "application/json" }, + body: "{not-json", + }); + const res = await dnsRoute.POST(req, { params: { id: "cursor" } }); + assert.equal(res.status, 400); +}); + +test("POST .../[id]/dns: error responses do not leak stack traces", async () => { + const res = await dnsRoute.POST(makeRequest({ enabled: true }), { + params: { id: "__nonexistent_agent__" }, + }); + const text = await res.text(); + assert.ok(!text.includes("at /"), "stack trace leaked in dns route 404 response"); +}); diff --git a/tests/unit/dns-config-generic.test.ts b/tests/unit/dns-config-generic.test.ts index e267af1337..2f77cd671d 100644 --- a/tests/unit/dns-config-generic.test.ts +++ b/tests/unit/dns-config-generic.test.ts @@ -69,6 +69,7 @@ const tmpHostsFile = path.join(tmpDir, "hosts"); // Import the module under test AFTER all setup. const dnsModule = await import("../../src/mitm/dns/dnsConfig.ts"); const { addDNSEntries, removeDNSEntries, addDNSEntry, removeDNSEntry, checkDNSEntry } = dnsModule; +const { ALL_TARGETS } = await import("../../src/mitm/targets/index.ts"); // --------------------------------------------------------------------------- // Tests @@ -95,10 +96,60 @@ test("addDNSEntry (legacy) is a function that delegates for Antigravity hosts", assert.equal(typeof addDNSEntry, "function"); }); +test("addDNSEntry with agentId resolves agent-specific hosts from ALL_TARGETS", async () => { + // Cursor target has hosts: ["api2.cursor.sh"] + // Verify that calling addDNSEntry with agentId="cursor" passes the right hosts. + // We call with [] to skip actual exec — just verifying the function signature accepts agentId. + await assert.doesNotReject( + addDNSEntry("fake-sudo", "cursor"), + "addDNSEntry must accept optional agentId parameter" + ); +}); + +test("addDNSEntry without agentId falls back to Antigravity hosts (backward compat)", async () => { + await assert.doesNotReject( + addDNSEntry("fake-sudo"), + "addDNSEntry without agentId must still work for backward compat" + ); +}); + +test("addDNSEntry with unknown agentId falls back to Antigravity hosts", async () => { + await assert.doesNotReject( + addDNSEntry("fake-sudo", "__nonexistent_agent__"), + "addDNSEntry with unknown agentId must fall back to Antigravity hosts" + ); +}); + test("removeDNSEntry (legacy) is a function that delegates for Antigravity hosts", () => { assert.equal(typeof removeDNSEntry, "function"); }); +test("removeDNSEntry with agentId resolves agent-specific hosts from ALL_TARGETS", async () => { + await assert.doesNotReject( + removeDNSEntry("fake-sudo", "copilot"), + "removeDNSEntry must accept optional agentId parameter" + ); +}); + +test("resolveHostsForAgent returns Antigravity hosts when agentId is undefined", () => { + // Verify ALL_TARGETS exists and cursor target has expected hosts + const cursorTarget = ALL_TARGETS.find((t) => t.id === "cursor"); + assert.ok(cursorTarget, "cursor target must exist in ALL_TARGETS"); + assert.ok( + cursorTarget.hosts.includes("api2.cursor.sh"), + "cursor target must include api2.cursor.sh" + ); + // Codex target + const codexTarget = ALL_TARGETS.find((t) => t.id === "codex"); + assert.ok(codexTarget, "codex target must exist in ALL_TARGETS"); + assert.ok(codexTarget.hosts.includes("chatgpt.com"), "codex target must include chatgpt.com"); +}); + +test("addDNSEntries batches missing entries with no-op on empty list", async () => { + // Empty list must resolve immediately (no exec, no error) + await assert.doesNotReject(addDNSEntries([], "fake-sudo")); +}); + test("addDNSEntries: skips hosts already in /etc/hosts (idempotency)", async () => { // Read live /etc/hosts and pick the first entry that already exists. // If localhost is in /etc/hosts we use it; otherwise skip this sub-assertion. @@ -157,11 +208,17 @@ test("addDNSEntries: entry passed as stdin data, not shell-interpolated", () => const srcPath = new URL("../../src/mitm/dns/dnsConfig.ts", import.meta.url).pathname; const src = fs.readFileSync(srcPath, "utf8"); - // The stdin data `${entry}\n` is the body text sent to tee via pipe — not - // part of the command array. Verify the pattern appears in the source. + // The stdin `data` is built from the batched entries and sent to tee via pipe — + // not part of the command array. Verify the pattern appears in the source and + // that it's passed as the 4th positional arg to execFileWithPassword (stdin), + // not interpolated into the argv array. assert.ok( - src.includes("`${entry}\\n`"), - "entry content must be passed as stdin to tee, not interpolated in args" + src.includes('missingEntries.map((e) => `${e}\\n`).join("")'), + "entry content must be built from missingEntries for stdin, not interpolated in args" + ); + assert.ok( + src.includes('execFileWithPassword("sudo", ["-S", "tee", "-a", HOSTS_FILE], sudoPassword, data)'), + "entry data must be passed as stdin to tee, not interpolated in args" ); });