From 700819a29bce4175fc51fd78523d0faa131f0c51 Mon Sep 17 00:00:00 2001 From: Webman Date: Tue, 25 Aug 2026 16:54:09 -0500 Subject: [PATCH] fix(ci): boot protocol E2E on the peer-stamped custom server with preserved open bootstrap (#11535) (#11549) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Validated in a combined 3-PR batch worktree off release/v3.8.51 tip. - Focused test: protocol-e2e-server-stamping-11535.test.ts — part of batch's 165/165 node:test run - typecheck:core, file-size, changelog-integrity, complexity, cognitive-complexity, check:docs-counts-sync — all OK - Full-repo lint: 228 pre-existing dashboard react-hooks/* findings, unrelated to this diff Thanks for the meticulous root-causing here — three distinct issues (server flavor, HOST pin, open-bootstrap env leak) traced to exact line numbers with live-boot before/after evidence. --- .../11535-protocol-e2e-peer-stamped-server.md | 1 + scripts/dev/run-next.mjs | 19 ++++ scripts/dev/run-protocol-clients-tests.mjs | 16 ++- ...protocol-e2e-server-stamping-11535.test.ts | 98 +++++++++++++++++++ 4 files changed, 133 insertions(+), 1 deletion(-) create mode 100644 changelog.d/fixes/11535-protocol-e2e-peer-stamped-server.md create mode 100644 tests/unit/protocol-e2e-server-stamping-11535.test.ts diff --git a/changelog.d/fixes/11535-protocol-e2e-peer-stamped-server.md b/changelog.d/fixes/11535-protocol-e2e-peer-stamped-server.md new file mode 100644 index 0000000000..601236ec90 --- /dev/null +++ b/changelog.d/fixes/11535-protocol-e2e-peer-stamped-server.md @@ -0,0 +1 @@ +- **fix(ci):** protocol-clients E2E harness boots the peer-stamped custom server (`run-next.mjs dev`) instead of the bare `next` CLI, so `LOCAL_ONLY` locality resolves from the real TCP peer stamp and `/api/mcp/audit` answers 200 (open bootstrap preserved via an `OMNIROUTE_E2E_BOOTSTRAP_MODE=open` credential clear in `run-next.mjs`, pinned to loopback `HOST`) instead of a deterministic 403 (#11535) diff --git a/scripts/dev/run-next.mjs b/scripts/dev/run-next.mjs index e03cfc9290..8282a88d0c 100644 --- a/scripts/dev/run-next.mjs +++ b/scripts/dev/run-next.mjs @@ -61,6 +61,25 @@ for (const [key, value] of Object.entries(mergedEnv)) { } } +// E2E open-mode bootstrap (#11535). Test harnesses that boot THIS server (protocol +// clients E2E) rely on an auth-disabled "open" bootstrap so management endpoints such +// as /api/mcp/audit are genuinely exercised unauthenticated (200), not short-circuited +// by a stray credential. bootstrap-env.mjs deliberately drops empty strings from +// process.env/.env/server.env, so an INITIAL_PASSWORD="" injected by a harness cannot +// survive the merge above and any INITIAL_PASSWORD persisted in .env or server.env +// would leak back in (401 → green-shallow suite). +// Cleared to EMPTY STRING (not deleted): Next's env loader re-reads the repo .env +// during app prepare(), AFTER this point — an absent var would be re-populated from +// the file and src/instrumentation-node.ts would bcrypt-persist it as a real login +// (401s everywhere). An existing empty var is falsy to every consumer AND wins over +// dotenv's no-override load, mirroring run-next-playwright.mjs's open-mode overrides. +// Gated on the test-only env var so production boots are untouched. +if (process.env.OMNIROUTE_E2E_BOOTSTRAP_MODE === "open") { + process.env.INITIAL_PASSWORD = ""; + process.env.OMNIROUTE_E2E_PASSWORD = ""; + process.env.OMNIROUTE_API_KEY = ""; +} + // systemd sd_notify (Type=notify / WatchdogSec=): this process owns the // watchdog pings — if its event loop blocks (freeze), the pings stop and // systemd kills the service. No-op outside systemd (no NOTIFY_SOCKET). diff --git a/scripts/dev/run-protocol-clients-tests.mjs b/scripts/dev/run-protocol-clients-tests.mjs index 070424e9c9..c32320c2f1 100644 --- a/scripts/dev/run-protocol-clients-tests.mjs +++ b/scripts/dev/run-protocol-clients-tests.mjs @@ -57,10 +57,24 @@ async function main() { OMNIROUTE_BASE_URL: baseUrl, }), OMNIROUTE_E2E_BOOTSTRAP_MODE: process.env.OMNIROUTE_E2E_BOOTSTRAP_MODE || "open", + // Pin the custom server's bind address to loopback (#11535): under the + // programmatic next() entry the middleware's nextUrl.hostname mirrors the + // configured HOST (default "0.0.0.0"), and apiAuth.isLoopbackRequest() reads + // nextUrl.hostname FIRST — an unpinned boot makes every request look remote, + // so the anonymous open-bootstrap allow never fires (401 green-shallow). + HOST: process.env.HOST || "127.0.0.1", }; if (!(await isServerReady())) { - serverProcess = spawn(process.execPath, ["scripts/dev/run-next-playwright.mjs", "dev"], { + // Boot the REAL custom server (run-next.mjs), not the bare `next dev` CLI. + // Only the custom Node server stamps the trusted PEER_IP_HEADER from the TCP + // socket; without that stamp the authz middleware fails closed on locality and + // every LOCAL_ONLY route (e.g. /api/mcp/audit) answers 403 even from loopback + // (#11535). run-next.mjs honors OMNIROUTE_E2E_BOOTSTRAP_MODE=open by clearing + // bootstrap credentials after its env merge, keeping the audit assertions live + // (200) instead of masking them behind a 401. The Playwright webServer runner is + // intentionally left untouched — it serves the whole blocking test-e2e suite. + serverProcess = spawn(process.execPath, ["scripts/dev/run-next.mjs", "dev"], { stdio: "inherit", env: testEnv, }); diff --git a/tests/unit/protocol-e2e-server-stamping-11535.test.ts b/tests/unit/protocol-e2e-server-stamping-11535.test.ts new file mode 100644 index 0000000000..dd0c42823e --- /dev/null +++ b/tests/unit/protocol-e2e-server-stamping-11535.test.ts @@ -0,0 +1,98 @@ +/** + * Issue #11535 — protocol-clients E2E must boot a peer-stamped, open-bootstrap server. + * + * The suite's harness (`scripts/dev/run-protocol-clients-tests.mjs`) used to spawn + * `run-next-playwright.mjs dev`, which runs the plain `next dev` CLI. That flavour of + * server never writes the trusted PEER_IP_HEADER stamp (only the custom Node server in + * `run-next.mjs` does), so `resolveStampedPeer()` returns null and the LOCAL_ONLY gate + * fails closed → every request to `/api/mcp/audit` answered 403 where the test accepts + * 200|401. Swapping the boot target to `run-next.mjs` fixes locality, but would go + * "green shallow": `bootstrap-env.mjs` deliberately drops empty strings from env, so an + * `.env`/server.env `INITIAL_PASSWORD` would leak back in and the audit block would + * silently never execute (401). These source contracts pin both halves of the fix: + * + * 1. the harness boots `run-next.mjs dev` (peer stamping present on this path); + * 2. `run-next.mjs` applies the E2E "open" bootstrap AFTER merging persisted/.env + * credentials into process.env, clearing them so the suite exercises 200. + */ + +import { readFileSync } from "node:fs"; +import { dirname, join } from "node:path"; +import { fileURLToPath } from "node:url"; +import { describe, it } from "node:test"; +import assert from "node:assert/strict"; + +const here = dirname(fileURLToPath(import.meta.url)); +const repoRoot = join(here, "..", ".."); +const harnessSource = readFileSync( + join(repoRoot, "scripts", "dev", "run-protocol-clients-tests.mjs"), + "utf8" +); +const runNextSource = readFileSync(join(repoRoot, "scripts", "dev", "run-next.mjs"), "utf8"); + +describe("protocol clients E2E harness (#11535)", () => { + it("boots the peer-stamped custom server (run-next.mjs), not the bare next CLI", () => { + assert.ok( + harnessSource.includes('"scripts/dev/run-next.mjs"'), + "harness must spawn scripts/dev/run-next.mjs so requests carry the trusted peer-IP stamp" + ); + assert.ok( + !harnessSource.includes("run-next-playwright.mjs"), + "harness must not boot via run-next-playwright.mjs (plain next dev has no peer stamping)" + ); + }); + + it("still passes the isolated port/data-dir/open-mode env to the spawned server", () => { + // The harness relies on these env vars to isolate the run and disable auth; + // swapping the boot target must not drop them. + for (const key of [ + "DATA_DIR:", + "PORT:", + "DASHBOARD_PORT:", + "OMNIROUTE_E2E_BOOTSTRAP_MODE", + // Under the programmatic next() entry, middleware nextUrl.hostname mirrors the + // configured HOST — an unpinned "0.0.0.0" bind makes apiAuth treat loopback + // requests as remote, so the open-bootstrap anonymous allow never fires. + '"127.0.0.1"', + ]) { + assert.ok(harnessSource.includes(key), `testEnv must still define ${key}`); + } + }); + + it("run-next.mjs stamps the real TCP peer IP into its request listener", () => { + const listenerIdx = runNextSource.indexOf("http.createServer("); + const stampIdx = runNextSource.indexOf("stampPeerIp(req)"); + assert.notEqual(listenerIdx, -1, "custom http.createServer listener expected"); + assert.notEqual(stampIdx, -1, "stampPeerIp(req) call expected"); + assert.ok( + stampIdx > listenerIdx, + "stampPeerIp must be wired inside/after the request listener setup" + ); + assert.ok(runNextSource.includes("ensurePeerStampToken()"), "per-process token required"); + }); + + it("run-next.mjs applies the E2E open-mode credential clear AFTER the env merge", () => { + const mergeLoopIdx = runNextSource.indexOf("Object.entries(mergedEnv)"); + const openModeIdx = runNextSource.indexOf('OMNIROUTE_E2E_BOOTSTRAP_MODE === "open"'); + assert.notEqual(mergeLoopIdx, -1, "mergedEnv application loop expected"); + assert.notEqual(openModeIdx, -1, "open-mode bootstrap hook missing"); + + const openModeBlock = runNextSource.slice(Math.max(0, openModeIdx - 200)); + for (const key of ["INITIAL_PASSWORD", "OMNIROUTE_E2E_PASSWORD", "OMNIROUTE_API_KEY"]) { + // Empty-string assignment, NOT delete: Next's env loader re-reads the repo .env + // during app prepare() (after this hook), so an absent var would be re-populated + // from the file and bcrypt-persisted by instrumentation — 401s everywhere. + const cleared = new RegExp(`process\\.env\\.${key} = "";`).exec(openModeBlock); + assert.ok(cleared, `open mode must set ${key} to "" after the bootstrap merge`); + const deleted = new RegExp(`delete process\\.env\\.${key}`).exec(openModeBlock); + assert.ok(!deleted, `open mode must not merely delete ${key} (dotenv would restore it)`); + } + // bootstrap-env.mjs filters empty strings, so the hook must run after the merge + // loop that copies persisted/.env values into process.env — otherwise a CI .env + // INITIAL_PASSWORD=CHANGEME would survive and the suite would go green shallow. + assert.ok( + openModeIdx > mergeLoopIdx, + "open-mode hook must be positioned after the mergedEnv -> process.env loop" + ); + }); +});