From 27ec885b548df5d05c4b83201107f65bb05df592 Mon Sep 17 00:00:00 2001 From: zodyp Date: Sun, 20 Sep 2026 06:49:39 -0300 Subject: [PATCH] =?UTF-8?q?fix(test):=20the=20sweep's=20two=20integration?= =?UTF-8?q?=20reds=20=E2=80=94=20one=20real,=20one=20my=20own=20guard?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The first full-CI verdict this release line has ever produced came back red. Unit ×4 and vitest green; both integration shards failed. Both were worth having. REAL — tests/integration/api-routes-critical.test.ts was making a live HTTPS request to aihorde.net, three attempts counting the retry, on every run. `GET /api/v1/models` refreshes the AI Horde image catalog whenever `aihorde` is active, and it is active by default: a no-auth provider has no connection row to switch off. aiHordeImageCatalog exposes setFetch for exactly this case; the test now injects a stub. The route's own catch keeps the last good snapshot, so an empty worker list changes none of the assertions. FALSE POSITIVE, and mine — tests/integration/api-keys.test.ts sets CLOUD_URL to http://cloud.example on purpose, so the cloud-sync branch is taken and fails. `cloud.example` is reserved by RFC 2606 / RFC 6761: there is no delegation for it anywhere, so it cannot reach a host. The guard I added in #56 counted it as "the suite reached the network" and failed a file that never left the machine. The first fix I wrote for that was wrong, and three existing guard tests caught it: I exempted reserved names from being BLOCKED, which let the connection through to a real DNS lookup — more network activity, not less. Blocking and counting are two decisions. A reserved name is now still refused, and only the counting changes. The log line says which case it was, so a future reader does not mistake one for the other. This is not a hole: the exemption is not "hosts a test asked for", it is "names that by standard resolve to nothing", and a provider smuggled in under `.test` would be just as unreachable. A test asserts the exemption does not reach aihorde.net, api.openai.com, example.com or a literal IP. Also: the shard jobs now upload _artifacts/release-green/. Without the per-gate logs a red shard reports only its first failure line and the assertion dies with the runner — which is why both of these had to be reproduced locally before they could be read at all. 25/25 api-keys + api-routes-critical, 0 guard violations 20/20 block-network-guard + block-network-wiring before: 15 attempts to cloud.example:80, 3 to aihorde.net:443 after: 0 counted, 0 to aihorde.net YAML parses; prettier clean Co-Authored-By: Claude Opus 5 --- .github/workflows/nightly-release-green.yml | 5 +++ changelog.d/fixes/sweep-integration-reds.md | 1 + tests/_setup/blockNetwork.ts | 34 ++++++++++++++- tests/integration/api-routes-critical.test.ts | 15 +++++++ tests/unit/block-network-guard.test.ts | 42 +++++++++++++++++++ 5 files changed, 95 insertions(+), 2 deletions(-) create mode 100644 changelog.d/fixes/sweep-integration-reds.md diff --git a/.github/workflows/nightly-release-green.yml b/.github/workflows/nightly-release-green.yml index 55ee7de50a06..6650c8087380 100644 --- a/.github/workflows/nightly-release-green.yml +++ b/.github/workflows/nightly-release-green.yml @@ -176,6 +176,10 @@ jobs: node scripts/quality/validate-release-green.mjs --json --hermetic --no-static --serial-slow $SUITE_FLAGS 1> slow-report.json 2> >(tee "slow-$SUITE_NAME.log" >&2) echo "[slow-suite] $SUITE_NAME finished with exit=$? (the verdict is the aggregator's)" + # The per-gate logs go up too. Without them a red shard reports only its FIRST + # failure line — "✖ tests/integration/api-keys.test.ts" — and the actual assertion + # lives in _artifacts/release-green/.log on a runner that is already gone. + # Both reds from run 35501782210 had to be reproduced locally to be read at all. - name: Upload the suite report if: always() uses: actions/upload-artifact@043fb46d1a93c77aae656e7c1c64a875d1fc6a0a # v7 @@ -184,6 +188,7 @@ jobs: path: | slow-report.json slow-${{ matrix.name }}.log + _artifacts/release-green/ retention-days: 14 if-no-files-found: warn diff --git a/changelog.d/fixes/sweep-integration-reds.md b/changelog.d/fixes/sweep-integration-reds.md new file mode 100644 index 000000000000..be4a650fa85d --- /dev/null +++ b/changelog.d/fixes/sweep-integration-reds.md @@ -0,0 +1 @@ +- **fix(test):** the first full-CI sweep this release line has ever completed came back red, and both reds were real. `tests/integration/api-routes-critical.test.ts` was making a **live HTTPS request to `aihorde.net`** on every run: `GET /api/v1/models` refreshes the AI Horde image catalog whenever `aihorde` is active, and it is active by default because it is a no-auth provider with no connection row to switch off. The service already exposes `setFetch` for exactly this, and the test now injects a stub. The second red was a false positive of the network guard itself: `tests/integration/api-keys.test.ts` points `CLOUD_URL` at `http://cloud.example` on purpose, to take a failing outbound branch, and the guard counted an RFC 2606 / RFC 6761 reserved name — one that resolves to nothing, anywhere — as "the suite reached the network". Such a name is now **still refused** (nothing leaves the machine, not even a DNS lookup) but is no longer counted as a violation. The shard jobs also upload `_artifacts/release-green/` now: without the per-gate logs a red shard reports only its first failure line and the assertion dies with the runner, which is why both of these had to be reproduced locally to be read at all. diff --git a/tests/_setup/blockNetwork.ts b/tests/_setup/blockNetwork.ts index 5789b7ef0f1f..8a9850eca236 100644 --- a/tests/_setup/blockNetwork.ts +++ b/tests/_setup/blockNetwork.ts @@ -92,6 +92,30 @@ const loopback = new net.BlockList(); loopback.addSubnet("127.0.0.0", 8, "ipv4"); loopback.addAddress("::1", "ipv6"); +/** + * Top-level domains the IETF reserves as permanently unresolvable (RFC 2606 / RFC 6761). + * A name under one of these cannot reach a host: there is no delegation for it, anywhere. + * + * Tests use them on purpose to exercise a FAILING outbound path — `CLOUD_URL` is set to + * `http://cloud.example` in tests/integration/api-keys.test.ts so the cloud-sync branch is + * taken and fails. Counting that as "the suite reached the network" made this guard fail a + * file that never left the machine, which is a false positive on a guard whose value is + * that its reds are real. + * + * This is not a hole: the exemption is not "hosts a test asked for", it is "names that by + * standard resolve to nothing". A provider smuggled in under `.test` would still not be + * reachable, so there is nothing to smuggle. + */ +const UNRESOLVABLE_TLDS = ["test", "example", "invalid", "localhost"]; + +/** True for a name under an RFC-reserved, permanently unresolvable TLD. */ +export function isUnresolvableHost(host: string): boolean { + const normalized = host.trim().replace(/\.$/, "").toLowerCase(); + if (net.isIP(normalized)) return false; + const tld = normalized.split(".").pop() ?? ""; + return UNRESOLVABLE_TLDS.includes(tld); +} + /** True for loopback IPs (v4, v6, IPv4-mapped v6) and the name `localhost`. */ export function isLoopbackHost(host: string): boolean { const normalized = host @@ -245,9 +269,15 @@ function installNetworkGuard(decision: GuardDecision): NetworkGuard { function block(host: string, port: string, via: string): NetworkAccessBlockedError { const testFile = currentTestFile(); const violation: Violation = { host, port, via, testFile, stack: captureStack() }; - violations.push(violation); + // Refuse it either way — the connection must never leave the machine, not even as a + // DNS lookup. What an RFC-reserved name changes is only whether it is COUNTED: a test + // that points at `cloud.example` on purpose, to exercise a failing outbound branch, + // has not reached the network and must not fail the file for it. + const counted = !isUnresolvableHost(host); + if (counted) violations.push(violation); process.stderr.write( - `${LOG_PREFIX} ${decision.mode === "report" ? "REPORT" : "BLOCKED"} ` + + `${LOG_PREFIX} ${decision.mode === "report" ? "REPORT" : "BLOCKED"}` + + `${counted ? "" : " (reserved name — refused, not counted)"} ` + `host=${host} port=${port} via=${via} file=${testFile}\n${violation.stack}\n` ); return new NetworkAccessBlockedError(host, port, via, testFile); diff --git a/tests/integration/api-routes-critical.test.ts b/tests/integration/api-routes-critical.test.ts index ccc8df4fc278..6991cd45cbfc 100644 --- a/tests/integration/api-routes-critical.test.ts +++ b/tests/integration/api-routes-critical.test.ts @@ -19,6 +19,21 @@ const proxiesRoute = await import("../../src/app/api/v1/management/proxies/route const settingsProxyRoute = await import("../../src/app/api/settings/proxy/route.ts"); const settingsMitmRoute = await import("../../src/app/api/settings/mitm/route.ts"); const v1ModelsRoute = await import("../../src/app/api/v1/models/route.ts"); +const { aiHordeImageCatalog } = await import("@omniroute/open-sse/services/aihordeImageCatalog"); + +// GET /api/v1/models refreshes the AI Horde image catalog whenever `aihorde` is active — +// and it is active by default, because it is a no-auth provider with no connection row to +// switch off. So this file was making a live HTTPS request to aihorde.net on every run +// (3 attempts, counting the retry), which is what tests/_setup/blockNetwork.ts caught. +// The service exposes setFetch precisely for this; the route's own catch keeps the last +// good snapshot, so an empty worker list changes none of the assertions below. +aiHordeImageCatalog.setFetch( + async () => + new Response(JSON.stringify([]), { + status: 200, + headers: { "content-type": "application/json" }, + }) +); const MACHINE_ID = "1234567890abcdef"; diff --git a/tests/unit/block-network-guard.test.ts b/tests/unit/block-network-guard.test.ts index e732aa71a399..2ef3db23af2a 100644 --- a/tests/unit/block-network-guard.test.ts +++ b/tests/unit/block-network-guard.test.ts @@ -18,6 +18,7 @@ import { networkGuard, resolveGuardMode, targetFromConnectArgs, + isUnresolvableHost, } from "../_setup/blockNetwork.ts"; const PROBE = "tests/unit/fixtures/network-guard-probe.ts"; @@ -247,3 +248,44 @@ test("the wreq-js native transport (outside net.Socket) is blocked too", () => { /\[network-guard\] BLOCKED host=192\.0\.2\.1 port=443 via=wreq-js\.request/ ); }); + +// ─── RFC-reserved names are not "the network" ────────────────────────────── + +test("names under an RFC-reserved TLD are not counted as reaching the network", () => { + // RFC 2606 / RFC 6761 reserve these as permanently unresolvable, and tests use them to + // exercise a FAILING outbound path on purpose — api-keys.test.ts sets CLOUD_URL to + // http://cloud.example so the cloud-sync branch is taken and fails. Counting that as a + // violation failed a file that never left the machine. + for (const host of [ + "cloud.example", + "api.test", + "nothing.invalid", + "foo.localhost", + "DEEP.sub.Example", + "trailing.example.", + ]) { + assert.equal(isUnresolvableHost(host), true, `${host} must be treated as unresolvable`); + } +}); + +test("the exemption does not reach real names or addresses", () => { + // The value of this guard is that its reds are real. An exemption that leaked to + // `aihorde.net` — the live call the sweep actually caught — would destroy that. + for (const host of [ + "aihorde.net", + "api.openai.com", + "example.com", + "exampletest", + "192.0.2.1", + "2001:db8::1", + ]) { + assert.equal(isUnresolvableHost(host), false, `${host} must still be a violation`); + } +}); + +test("an unresolvable name is still not loopback", () => { + // Two separate questions, kept separate: `cloud.example` is exempt from the violation + // count, but it is not a local address and must never be treated as one. + assert.equal(isLoopbackHost("cloud.example"), false); + assert.equal(isLoopbackHost("localhost"), true); +});