diff --git a/CHANGELOG.md b/CHANGELOG.md index 5847923db8..1856c6de3b 100644 --- a/CHANGELOG.md +++ b/CHANGELOG.md @@ -18,6 +18,7 @@ - **security(oauth):** migrate the five public OAuth client_ids (Claude, Codex, Qwen, Kimi, GitHub Copilot — 9 server-side call-sites in `providerRegistry.ts` + `oauth.ts`) from string literals to `resolvePublicCred()` (Hard Rule #11), matching the existing Gemini/Antigravity pattern. The values decode byte-for-byte to the same public client_ids (env overrides still win), so OAuth flows are unchanged; the `check-public-creds` allowlist is now empty. The browser-bundled `codexDeviceFlow.ts` copy stays a literal by necessity (it cannot import `open-sse`). ([#3493](https://github.com/diegosouzapw/OmniRoute/issues/3493)) - **fix(mcp):** `omniroute --mcp` no longer crashes on npm installs with `ERR_MODULE_NOT_FOUND` (e.g. `src/lib/combos/steps.ts`) — the MCP server runs from raw TypeScript and imports across `src/` + `open-sse/`, but the published `files` allowlist only shipped a handful of cherry-picked paths, so the transitive closure (~400 files) was absent from the tarball. `files` now ships the backend source the MCP server needs (`open-sse/` + `src/{domain,lib,mitm,server,shared,sse,types}/`, excluding the `src/app` UI), and a new regression test computes the MCP import closure and fails if any reachable source file is not covered by `files`. ([#3578](https://github.com/diegosouzapw/OmniRoute/issues/3578)) - **fix(api):** `API_REFERENCE.md` no longer documents a non-existent `/api/guardrails*` / `/api/shadow*` surface (doc-fiction flagged by `check-docs-symbols`, frozen in `KNOWN_STALE_DOC_REFS`). The guardrail pipeline is real (`src/lib/guardrails`), so the two routes that map to actual behavior are now implemented — `GET /api/guardrails` (list the registered guardrails + status) and `POST /api/guardrails/test` (dry-run the pre-call pipeline over a sample input), both management-scoped — while the fictional `enable`/`disable`/`logs` rows and the entire `/api/shadow*` table (shadow A-B comparison is combo-config + `/api/combos/metrics`) were removed from the doc and dropped from the allowlist. ([#3496](https://github.com/diegosouzapw/OmniRoute/issues/3496)) +- **fix(agent-bridge):** the MITM "Start" button no longer reports a misleading "port 443 may be in use" for every failure cause — `startMitm()` only matched the EADDRINUSE stderr line and always threw the port-443 message, so a missing `ROUTER_API_KEY` or an `EACCES` permission error sent users debugging the wrong thing. The startup watcher now buffers the MITM child's stderr and `interpretMitmStartupError()` maps the real `server.cjs` `❌` cause (port-in-use / permission-denied / missing API key / any other diagnostic line) into the surfaced error; with no captured output it stays generic instead of guessing port 443. ([#3606](https://github.com/diegosouzapw/OmniRoute/issues/3606)) --- diff --git a/src/mitm/manager.ts b/src/mitm/manager.ts index 8058f5fe2e..38bf86ef57 100644 --- a/src/mitm/manager.ts +++ b/src/mitm/manager.ts @@ -16,6 +16,42 @@ import { createLogger } from "@/shared/utils/logger.ts"; const log = createLogger("mitm-manager"); +/** + * Map the MITM child process (`server.cjs`) stderr to the actual startup-failure + * cause. `server.cjs` emits one of several "❌"-prefixed lines on `server.on("error")` + * or on a missing API key, then exits. The old code only matched EADDRINUSE and so + * always blamed "port 443", misleading users whose real problem was a permission + * error or a missing ROUTER_API_KEY (#3606). The returned string is a controlled, + * secret-free diagnostic (it carries no stack and no credentials). (#3606) + */ +export function interpretMitmStartupError(stderr: string, port: number): string { + const text = (stderr || "").trim(); + const lower = text.toLowerCase(); + + if (lower.includes("already in use")) { + return `MITM server failed to start: port ${port} is already in use`; + } + if (lower.includes("permission denied")) { + return `MITM server failed to start: permission denied for port ${port} (run with elevated privileges, or use a port ≥ 1024)`; + } + if (lower.includes("router_api_key")) { + return "MITM server failed to start: no API key was provided (ROUTER_API_KEY is required). Set a router API key in OmniRoute and retry."; + } + + // Surface the first "❌ " diagnostic line verbatim (marker stripped), + // so any other server.cjs failure is reported with its real cause. + for (const line of text.split(/\r?\n/)) { + const trimmed = line.trim(); + if (trimmed.startsWith("❌")) { + const detail = trimmed.replace(/^❌\s*/, "").trim(); + if (detail) return `MITM server failed to start: ${detail}`; + } + } + + // Nothing diagnostic was captured — stay generic instead of guessing port 443. + return "MITM server failed to start (no diagnostic output was captured from the MITM server)"; +} + // Store server process let serverProcess: ChildProcess | null = null; let serverPid: number | null = null; @@ -314,13 +350,19 @@ export async function startMitm( fs.writeFileSync(PID_FILE, String(serverPid)); } + // Buffer recent stderr so a startup failure can be reported with its real + // cause (capped to avoid unbounded growth on a chatty/looping process). (#3606) + let stderrBuffer = ""; + // Log server output proc.stdout?.on("data", (data) => { log.info({ source: "mitm-server" }, data.toString().trim()); }); proc.stderr?.on("data", (data) => { - log.error({ source: "mitm-server" }, data.toString().trim()); + const chunk = data.toString(); + stderrBuffer = (stderrBuffer + chunk).slice(-4000); + log.error({ source: "mitm-server" }, chunk.trim()); }); proc.on("exit", (code) => { @@ -354,10 +396,11 @@ export async function startMitm( } }); - // Check stderr for error messages + // Fail fast on any "❌" diagnostic line from server.cjs (covers EADDRINUSE, + // EACCES, missing ROUTER_API_KEY, and any other server.on("error") cause). proc.stderr?.on("data", (data) => { - const msg = data.toString().trim(); - if (msg.includes("Port") && msg.includes("already in use")) { + const msg = data.toString(); + if (msg.includes("❌")) { clearTimeout(timeout); if (!resolved) { resolved = true; @@ -368,7 +411,7 @@ export async function startMitm( }); if (!started) { - throw new Error("MITM server failed to start (port 443 may be in use)"); + throw new Error(interpretMitmStartupError(stderrBuffer, port)); } return { diff --git a/tests/unit/mitm-startup-error-3606.test.ts b/tests/unit/mitm-startup-error-3606.test.ts new file mode 100644 index 0000000000..e418477c5d --- /dev/null +++ b/tests/unit/mitm-startup-error-3606.test.ts @@ -0,0 +1,49 @@ +/** + * Regression test for #3606 — MITM startup failure message is misleading. + * + * `startMitm()` used to always throw "port 443 may be in use" regardless of the + * real cause, because the stderr parser only matched EADDRINUSE. `server.cjs` + * already distinguishes EADDRINUSE / EACCES / missing-ROUTER_API_KEY / other on + * stderr (each prefixed with "❌"). `interpretMitmStartupError()` now maps the + * captured stderr to the actual cause so the user is not sent debugging port 443 + * when the real problem is a missing API key or a permission error. + */ +import { test } from "node:test"; +import assert from "node:assert/strict"; +import { interpretMitmStartupError } from "../../src/mitm/manager.ts"; + +test("EADDRINUSE stderr still reports the port-in-use cause", () => { + const msg = interpretMitmStartupError("❌ Port 443 already in use", 443); + assert.match(msg, /443/); + assert.match(msg, /in use/i); +}); + +test("EACCES stderr reports a permission cause, not port-in-use", () => { + const msg = interpretMitmStartupError("❌ Permission denied for port 443", 443); + assert.match(msg, /permission/i); + assert.doesNotMatch(msg, /in use/i); +}); + +test("missing ROUTER_API_KEY stderr reports the API-key cause, not port-in-use", () => { + const msg = interpretMitmStartupError("❌ ROUTER_API_KEY required", 443); + assert.match(msg, /ROUTER_API_KEY|API key/i); + assert.doesNotMatch(msg, /in use/i); +}); + +test("an arbitrary ❌ error line is surfaced verbatim (without the marker)", () => { + const msg = interpretMitmStartupError("some log\n❌ ENOENT: server.cjs missing\nmore log", 8443); + assert.match(msg, /ENOENT: server\.cjs missing/); + assert.doesNotMatch(msg, /❌/); +}); + +test("respects a non-default port in the port-in-use message", () => { + const msg = interpretMitmStartupError("❌ Port 8443 already in use", 8443); + assert.match(msg, /8443/); +}); + +test("with no captured stderr, falls back to a generic (non-misleading) message", () => { + const msg = interpretMitmStartupError("", 443); + // Must NOT assert it is specifically a port-443 problem when nothing was captured. + assert.doesNotMatch(msg, /port 443 may be in use/i); + assert.match(msg, /failed to start/i); +});