From ee14b9d56440921c231b07fc201057fffe852477 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Thu, 3 Sep 2026 02:13:02 +0200 Subject: [PATCH] test: gate every env read against the compose whitelist MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Derives both sets mechanically — the whitelist from docker-compose.yml, the read set from the source — and fails when a variable is read without being forwarded. Written red against the tree before the fix, where it named 35. A search for process.env. alone is not the read set: four modules resolve the name through a helper and index process.env dynamically, which hides every OIDC_* variable and ALLOW_LOCAL_AI_PRIVATE_HOSTS. Each is registered with its own call-shape matcher and its own non-zero assertion, so a rename fails the file loudly instead of quietly shrinking the set. The walk and the matchers both assert a floor, because an empty match set agrees with any allowlist. Thirteen deliberate exceptions each carry a written reason, and the list is kept honest from both ends: an entry that stops being read, or that later appears in the whitelist, fails too. --- .../compose-env-whitelist-guard.test.ts | 252 ++++++++++++++++++ 1 file changed, 252 insertions(+) create mode 100644 src/__tests__/compose-env-whitelist-guard.test.ts diff --git a/src/__tests__/compose-env-whitelist-guard.test.ts b/src/__tests__/compose-env-whitelist-guard.test.ts new file mode 100644 index 000000000..20c3ad68e --- /dev/null +++ b/src/__tests__/compose-env-whitelist-guard.test.ts @@ -0,0 +1,252 @@ +/** + * Structural guard: every runtime environment variable the server reads is on + * the compose `environment:` whitelist. + * + * ## The failure this freezes + * + * `docker-compose.yml`'s `environment:` block is a WHITELIST. Compose reads + * `.env` for `${VAR}` substitution at compose-up time, so a value an operator + * sets there appears to be configured — but a variable the block does not name + * never reaches the Node process. The symptom is the worst kind: no error, no + * warning, the documented knob simply does nothing and the default silently + * stands. v1.5.2 closed one instance of this (`SESSION_COOKIE_SECURE`); it + * came back at scale, which is why the question is a gate now rather than a + * comment on each entry. + * + * ## What it proves and what it does not + * + * It proves one thing exactly: no variable is read by shipped server code + * without either reaching the container or carrying a written reason why it + * must not. It does NOT prove the whitelist's defaults are right, that the + * `.env.production.example` prose is accurate, or that a variable behaves as + * documented. Those stay review questions. + * + * ## Why the matchers are what they are + * + * A grep for `process.env.X` alone is not the read set. Four modules resolve a + * name through a helper and index `process.env` dynamically, so their reads + * are invisible to the direct matcher — `ALLOW_LOCAL_AI_PRIVATE_HOSTS` and + * every `OIDC_*` among them. Each is registered below with its own call-shape + * matcher AND its own non-zero assertion, so a refactor that renames the + * helper fails this file loudly instead of quietly shrinking the read set. + * + * An empty match set is the other way a sweep goes quiet: it agrees with any + * allowlist. Both the walk and every matcher therefore assert a floor. + */ +import { describe, it, expect } from "vitest"; +import { readFileSync } from "node:fs"; +import { join } from "node:path"; + +import { walkSourceFiles } from "./helpers/source-files"; + +const ROOT = process.cwd(); +const SRC = join(ROOT, "src"); + +/** `process.env.NAME` and `process.env["NAME"]`. */ +const DIRECT_READ = + /process\.env(?:\.([A-Za-z_][A-Za-z0-9_]*)|\[\s*["'`]([A-Za-z_][A-Za-z0-9_]*)["'`]\s*\])/g; + +/** + * Modules that index `process.env` through a variable rather than a literal. + * The direct matcher cannot see their reads at all, so each names the exact + * call shape that carries the variable name, and each is asserted non-empty + * in T2 below. `minimum` is the count that must still match — pinned under + * the real number so an added read does not need a test edit, but a removed + * matcher does. + */ +const INDIRECT_READERS: ReadonlyArray<{ + file: string; + pattern: RegExp; + minimum: number; + why: string; +}> = [ + { + file: "lib/auth/oidc.ts", + pattern: /envOrEmpty\(\s*["']([A-Z][A-Z0-9_]*)["']/g, + minimum: 6, + why: "every OIDC_* name goes through the envOrEmpty() helper", + }, + { + file: "app/api/settings/privacy-summary/route.ts", + pattern: /intEnv\(\s*["']([A-Z][A-Z0-9_]*)["']/g, + minimum: 1, + why: "the retention windows are parsed through a local intEnv() helper", + }, + { + file: "lib/ai/local-host-allowlist.ts", + pattern: /ENV_VAR\s*=\s*["']([A-Z][A-Z0-9_]*)["']/g, + minimum: 1, + why: "the name is hoisted into an ENV_VAR constant, then indexed", + }, + { + file: "lib/boot/readiness-summary.ts", + pattern: /\benv\.([A-Z][A-Z0-9_]*)/g, + minimum: 5, + why: "the readiness report reads a `process.env`-defaulted parameter", + }, +]; + +/** + * Read but deliberately NOT on the compose whitelist. Every entry carries the + * reason adding it would be wrong — not merely why nobody got round to it. + * Kept honest from both ends by T4: an entry that stops being read, or that + * later appears in compose, fails this file. + */ +const NOT_OPERATOR_CONFIGURABLE: Readonly> = { + // --- Provided by the runtime or the platform, never by an operator. --- + NEXT_RUNTIME: "set by Next.js to distinguish the node and edge runtimes", + npm_package_version: "set by the package manager when it spawns the process", + + // --- Baked at build time by the Dockerfile's ARG/ENV pairs. --- + NEXT_PUBLIC_APP_VERSION: + "Dockerfile build arg; a runtime value cannot change what is compiled in", + NEXT_PUBLIC_APP_BUILD_SHA: + "Dockerfile build arg, stamped from the release commit", + NEXT_PUBLIC_APP_BUILT_AT: "Dockerfile build arg, stamped at image build", + BUILD_TIMESTAMP: + "build-provenance stamp; an operator value would be a lie about the image", + GIT_COMMIT: + "build-provenance stamp, read only as a fallback for the build SHA", + COMMIT_SHA: + "build-provenance stamp, read only as a fallback for the build SHA", + SOURCE_COMMIT: + "build-provenance stamp, read only as a fallback for the build SHA", + + // --- Reachable, but through a different name that IS on the whitelist. --- + DB_CONNECTION_LIMIT: + "compose splices it into DATABASE_URL's connection_limit parameter; forwarding it as a second env var would be two paths to one knob", + DB_POOL_TIMEOUT: + "compose splices it into DATABASE_URL's pool_timeout parameter, same reasoning as DB_CONNECTION_LIMIT", + DATABASE_POOL_MAX: + "a third spelling of DB_CONNECTION_LIMIT, read only as its fallback; one operator-facing name is enough", + + // --- Not an operator decision. --- + CODEX_OAUTH_CLIENT_ID: + "the ChatGPT OAuth application id is a fixed protocol constant, overridable only for development against a different app", + + // --- Never executes inside the container. --- + COACH_EVAL_API_KEY: + "repository secret for the nightly model-graded Coach eval; that workflow runs in CI, never in the app image", + COACH_EVAL_GENERATOR_MODEL: "CI-only knob for the same nightly eval workflow", + COACH_EVAL_JUDGE_MODEL: "CI-only knob for the same nightly eval workflow", + HEALTHLOG_MCP_TOKEN: + "read by the local stdio MCP bridge the operator runs on their own machine; the production image strips tsx so it cannot run there", + + // --- A whitelist entry would be obeyed by half the readers. --- + NEXT_PUBLIC_DASHBOARD_SNAPSHOT: + "NEXT_PUBLIC_* is inlined into the client bundle at build time and three of its four readers are client components, so a runtime value would steer the server half only — a split-brain worse than no knob. .env.production.example already documents it as a build-time toggle and says it is deliberately absent from the whitelist; adding it would make that note false.", +}; + +function sourceFiles(): string[] { + return walkSourceFiles(SRC, { floor: 3000 }) + .filter((rel) => !rel.startsWith("generated/")) + .filter((rel) => !rel.includes("__tests__")) + .filter((rel) => !rel.endsWith(".test.ts") && !rel.endsWith(".test.tsx")); +} + +function read(rel: string): string { + return readFileSync(join(SRC, rel), "utf8"); +} + +/** Every environment variable shipped server code reads, however it reads it. */ +function runtimeReads(): Set { + const names = new Set(); + for (const rel of sourceFiles()) { + const src = read(rel); + for (const m of src.matchAll(DIRECT_READ)) names.add(m[1] ?? m[2]); + } + for (const reader of INDIRECT_READERS) { + for (const m of read(reader.file).matchAll(reader.pattern)) names.add(m[1]); + } + return names; +} + +/** + * The keys the compose `environment:` block (via the shared `x-healthlog-env` + * anchor) forwards into the container. Two-space-indented `KEY: value` lines + * are the anchor's own entries; the services merge the anchor rather than + * repeating it. + */ +function composeWhitelist(): Set { + const compose = readFileSync(join(ROOT, "docker-compose.yml"), "utf8"); + const keys = new Set(); + for (const line of compose.split("\n")) { + const m = line.match(/^\s{2,}([A-Z][A-Z0-9_]*):\s/); + if (m) keys.add(m[1]); + } + // The `db` service's own three; the app never reads them. + for (const k of ["POSTGRES_USER", "POSTGRES_PASSWORD", "POSTGRES_DB"]) { + keys.delete(k); + } + return keys; +} + +describe("compose env whitelist", () => { + it("T1 — the sweep actually reads something", () => { + const reads = runtimeReads(); + // Pinned well under the real count. A sweep that collapses to a handful + // agrees with any allowlist, which is how an empty scan passes as green. + expect(reads.size).toBeGreaterThan(80); + expect(composeWhitelist().size).toBeGreaterThan(50); + }); + + it("T2 — every indirect reader still matches its call shape", () => { + for (const reader of INDIRECT_READERS) { + const found = [...read(reader.file).matchAll(reader.pattern)].map( + (m) => m[1], + ); + expect( + found.length, + `${reader.file}: matched ${found.length} name(s), expected at least ` + + `${reader.minimum} — ${reader.why}. A zero here means the matcher ` + + `went stale, not that the reads went away.`, + ).toBeGreaterThanOrEqual(reader.minimum); + } + }); + + it("T3 — no runtime variable is read without reaching the container", () => { + const whitelist = composeWhitelist(); + const missing = [...runtimeReads()] + .filter((name) => !whitelist.has(name)) + .filter((name) => !(name in NOT_OPERATOR_CONFIGURABLE)) + .sort(); + + expect( + missing, + `These variables are read by shipped server code but are not on the ` + + `docker-compose.yml \`environment:\` whitelist, so a value set in a ` + + `self-hoster's .env never reaches the process and the default stands ` + + `silently. Either add each to the whitelist (with a default and a ` + + `line in .env.production.example) or record in ` + + `NOT_OPERATOR_CONFIGURABLE why forwarding it would be wrong.`, + ).toEqual([]); + }); + + it("T4 — the exception list carries no stale entries", () => { + const reads = runtimeReads(); + const whitelist = composeWhitelist(); + + const unread = Object.keys(NOT_OPERATOR_CONFIGURABLE) + .filter((name) => !reads.has(name)) + .sort(); + expect( + unread, + "Listed as a deliberate exception but no longer read anywhere — drop the entry.", + ).toEqual([]); + + const alsoForwarded = Object.keys(NOT_OPERATOR_CONFIGURABLE) + .filter((name) => whitelist.has(name)) + .sort(); + expect( + alsoForwarded, + "Listed as deliberately not forwarded, yet present in the compose whitelist — one of the two is wrong.", + ).toEqual([]); + + for (const [name, reason] of Object.entries(NOT_OPERATOR_CONFIGURABLE)) { + expect( + reason.length, + `${name} needs a real reason, not a placeholder`, + ).toBeGreaterThan(30); + } + }); +});