fix(ci): boot protocol E2E on the peer-stamped custom server with preserved open bootstrap (#11535) (#11549)

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.
This commit is contained in:
Webman
2026-08-25 16:54:09 -05:00
committed by GitHub
parent 7cfabfc5c9
commit 700819a29b
4 changed files with 133 additions and 1 deletions

View File

@@ -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)

View File

@@ -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).

View File

@@ -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,
});

View File

@@ -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"
);
});
});