fix(config): treat an empty environment variable as unset

docker-compose.yml forwards its whitelist as "${NAME:-}", so a variable
the operator never set reaches the process as the empty string. Reads
written as a nullish fallback kept that string: the weather client built
an empty base URL and every geocode failed with ERR_INVALID_URL, and the
IP geo lookup requested a relative path.

Reads with a fallback now go through src/lib/env.ts (envValue, envOr,
envFlag), which treats empty and whitespace as unset. IP_GEO_LOOKUP_DISABLED
accepts 1, true, yes and on instead of only "1". A structural test refuses
a ?? fallback written directly against process.env anywhere in src/, and
the compose whitelist guard counts the helper's call shape as a read.
This commit is contained in:
Marc-André Bombeck committed 2026-09-26 15:20:42 +02:00
1 parent 4af6b87a2a
commit 3c3e5ceab7
25 files changed
+337 -37

No files matched your search

@@ -46,6 +46,13 @@ const SRC = join(ROOT, "src");
const DIRECT_READ =
/process\.env(?:\.([A-Za-z_][A-Za-z0-9_]*)|\[\s*["'`]([A-Za-z_][A-Za-z0-9_]*)["'`]\s*\])/g;
/**
* `envValue("NAME")`, `envOr("NAME", …)` and `envFlag("NAME")` from
* `src/lib/env.ts` — the one helper every read with a fallback goes through,
* so its call shape is a read wherever it appears. Asserted non-empty in T2.
*/
const HELPER_READ = /\benv(?:Value|Or|Flag)\(\s*["'`]([A-Z][A-Z0-9_]*)["'`]/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
@@ -154,6 +161,7 @@ function runtimeReads(): Set<string> {
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 m of src.matchAll(HELPER_READ)) names.add(m[1]);
}
for (const reader of INDIRECT_READERS) {
for (const m of read(reader.file).matchAll(reader.pattern)) names.add(m[1]);
@@ -191,6 +199,13 @@ describe("compose env whitelist", () => {
});
it("T2 — every indirect reader still matches its call shape", () => {
const helperReads = sourceFiles().flatMap((rel) =>
[...read(rel).matchAll(HELPER_READ)].map((m) => m[1]),
);
// The env helper is the common path for reads with a fallback; a matcher
// that stopped seeing it would drop those names from the read set.
expect(helperReads.length).toBeGreaterThan(10);
expect(helperReads).toContain("IP_GEO_LOOKUP_URL");
for (const reader of INDIRECT_READERS) {
const found = [...read(reader.file).matchAll(reader.pattern)].map(
(m) => m[1],
@@ -0,0 +1,107 @@
/**
* Structural guard: an environment variable with a fallback is read through
* `src/lib/env.ts`, never as `process.env.NAME ?? fallback`.
*
* ## The failure this freezes
*
* `docker-compose.yml` forwards its whitelist as `"${NAME:-}"`, so every
* listed variable the operator left unset arrives as the empty string. `??`
* only replaces `null` and `undefined`, so the empty string wins and the
* fallback never applies. On a stock compose stack that sent Withings an
* OAuth `redirect_uri=""`, handed the weather client an empty base URL
* (`ERR_INVALID_URL` on every fetch) and pointed the IP geo lookup at a
* relative path. The same shape with a method in between,
* `process.env.NAME?.replace(...) ?? fallback`, fails the same way.
*
* ## What it proves and what it does not
*
* It proves no source file under `src/` writes a nullish fallback directly
* against a `process.env` read. It does not see a read stored in a local
* first and defaulted later (`const x = process.env.NAME; x ?? y`); those
* stay a review question. `||` is allowed: it already treats the empty
* string as unset, and client components need a literal
* `process.env.NEXT_PUBLIC_X` for build-time inlining.
*/
import { describe, expect, it } from "vitest";
import { readFileSync } from "node:fs";
import { join } from "node:path";
import { walkSourceFiles } from "./helpers/source-files";
const SRC = join(process.cwd(), "src");
/**
* `process.env.NAME` or `process.env["NAME"]`, optionally followed by
* optional-chained calls such as `?.trim()` or `?.replace(/x/, "")`, then
* `??` across any whitespace including a line break.
*/
const NULLISH_ENV_FALLBACK =
/process\.env(?:\.[A-Za-z_][A-Za-z0-9_]*|\[\s*["'`][A-Za-z_][A-Za-z0-9_]*["'`]\s*\])(?:\?\.[A-Za-z_]+\((?:[^()]|\([^()]*\))*\))*\s*\?\?/g;
/**
* Files that still carry the pattern because another change owns them.
* Each entry fails T3 once the file no longer matches, so the list can only
* shrink.
*/
const PENDING: Readonly<Record<string, string>> = {
"lib/withings/client.ts":
"WITHINGS_REDIRECT_URI ?? derived callback; the integration clients are being reworked in a parallel change that owns this file",
};
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"))
.filter((rel) => rel !== "lib/env.ts");
}
function offenders(): string[] {
return sourceFiles().filter((rel) => {
const src = readFileSync(join(SRC, rel), "utf8");
return [...src.matchAll(NULLISH_ENV_FALLBACK)].length > 0;
});
}
describe("an empty environment variable counts as unset", () => {
it("T1 — the matcher catches every shape it claims to", () => {
const shapes = [
`const a = process.env.FOO ?? "x";`,
`const b = process.env.FOO\n ?? "x";`,
`const c = process.env["FOO"] ?? "x";`,
`const d = process.env.FOO?.replace(/\\/$/, "") ?? "x";`,
`const e = process.env.FOO?.trim()?.toLowerCase() ?? "x";`,
];
for (const shape of shapes) {
expect(
[...shape.matchAll(NULLISH_ENV_FALLBACK)].length,
shape,
).toBeGreaterThan(0);
}
for (const allowed of [
`const f = process.env.FOO || "x";`,
`const g = envOr("FOO", "x");`,
`const h = process.env.FOO === "1" ? a ?? b : c;`,
]) {
expect([...allowed.matchAll(NULLISH_ENV_FALLBACK)], allowed).toEqual([]);
}
});
it("T2 — no source file defaults a process.env read with ??", () => {
const found = offenders().filter((rel) => !(rel in PENDING));
expect(
found,
"Read these through envValue/envOr/envFlag from @/lib/env. Compose " +
"passes an unset variable as the empty string, and ?? keeps it.",
).toEqual([]);
});
it("T3 — every pending entry still matches, so the list only shrinks", () => {
const current = new Set(offenders());
const stale = Object.keys(PENDING).filter((rel) => !current.has(rel));
expect(
stale,
"No longer carries the pattern; drop the entry from PENDING.",
).toEqual([]);
});
});