diff --git a/scripts/env-manifest.json b/scripts/env-manifest.json index 447d71f49..4613b0c9d 100644 --- a/scripts/env-manifest.json +++ b/scripts/env-manifest.json @@ -241,6 +241,23 @@ "purpose": "Separate AES key for the backup body — MUST NOT match ENCRYPTION_KEY." } ] + }, + { + "name": "Operational windows and cookie flags", + "description": "Values the product states back to a user or that decide whether a session cookie is accepted. Optional: each has a documented default. They are listed so `pnpm check-env` reports them, and so the compose whitelist and the example files can be held in step with this manifest.", + "required": false, + "variables": [ + { + "name": "AUDIT_LOG_RETENTION_DAYS", + "purpose": "How long audit rows survive, in days (365 by default). Published, not merely enforced: the shared-access activity feed and the dialog that ends somebody's access both state the number back to the user.", + "example": "365" + }, + { + "name": "SESSION_COOKIE_SECURE", + "purpose": "Forces the Secure flag on every cookie the app issues. Unset, it tracks NODE_ENV. Set false for a LAN / VPN-only self-host served over plain HTTP, where the default makes login look broken.", + "example": "false" + } + ] } ] } diff --git a/src/__tests__/manage-activity-inventory.test.ts b/src/__tests__/manage-activity-inventory.test.ts index 626d32e58..6f0b765ec 100644 --- a/src/__tests__/manage-activity-inventory.test.ts +++ b/src/__tests__/manage-activity-inventory.test.ts @@ -110,6 +110,18 @@ function routeFiles(dir: string): string[] { interface ModuleFacts { /** Does the module reach the record resolver at ANY level. */ delegatesRecord: boolean; + /** + * Every level the module passes to `requireRecordAuth`, in source order. + * + * Recorded so the inclusion rule's LEVEL axis has something that can + * falsify it. `delegatesRecord` used to be set only for `"manage"`, and the + * legs below could not tell the corrected rule from the old one: every + * assertion about coverage passed either way, because a manage-only sweep + * still finds every manage action. The leg that separates them asks whether + * the walked set STRICTLY EXCEEDS the manage-only one, and it needs the + * levels to ask. + */ + recordLevels: string[]; /** * Every action the module hands to `auditLog("…")`, awaited or not. * @@ -190,12 +202,17 @@ export function moduleFacts(source: string, fileName: string): ModuleFacts { }; let delegatesRecord = false; + const recordLevels: string[] = []; const auditActions: string[] = []; const visit = (node: ts.Node): void => { if (ts.isCallExpression(node)) { const name = calleeName(node.expression); - if (name === RECORD_RESOLVER) delegatesRecord = true; + if (name === RECORD_RESOLVER) { + delegatesRecord = true; + const level = firstStringArg(node); + if (level !== null) recordLevels.push(level); + } if (name === "auditLog") { const action = firstStringArg(node); if (action !== null) auditActions.push(action); @@ -205,7 +222,7 @@ export function moduleFacts(source: string, fileName: string): ModuleFacts { }; parsed.forEachChild(visit); - return { delegatesRecord, auditActions, imports: importPaths }; + return { delegatesRecord, recordLevels, auditActions, imports: importPaths }; } /** @@ -441,6 +458,30 @@ describe("every delegated action has a sentence of its own", () => { expect(actions.length).toBeGreaterThan(60); }); + it("walks strictly more modules than a manage-only sweep would", () => { + // The level axis of the inclusion rule, with something that can falsify + // it. Every other leg in this file passes under BOTH rules — a manage-only + // sweep still finds every manage action, so coverage cannot tell the two + // apart. This one can: the corrected rule is "any module that reaches + // `requireRecordAuth` at any level", so the walked set must strictly + // exceed the subset that names `manage` anywhere. + const manageOnly = delegatedModules.filter((file) => + read(file).recordLevels.includes("manage"), + ); + expect(manageOnly.length).toBeGreaterThan(0); + expect(delegatedModules.length).toBeGreaterThan(manageOnly.length); + + // A named anchor, so the claim does not rest on a count that could drift + // to equality without anybody noticing. The custom-metrics list resolves + // `requireRecordAuth("read", …)` and nothing else, so a manage-only filter + // drops it — along with the refusal breadcrumbs it and its neighbours file. + const anchor = join(API, "custom-metrics/route.ts"); + expect(read(anchor).recordLevels).toEqual(["read"]); + expect(read(anchor).recordLevels).not.toContain("manage"); + expect(delegatedModules).toContain(anchor); + expect(manageOnly).not.toContain(anchor); + }); + it("reaches the actions a route files through what it imports", () => { // Both halves of the fix, pinned against the two that were invisible. expect(actions).toContain("medication.intake.import.kickoff.denied"); diff --git a/src/__tests__/published-env-reaches-the-container.test.ts b/src/__tests__/published-env-reaches-the-container.test.ts index 1d7ca6afa..3847c4a0f 100644 --- a/src/__tests__/published-env-reaches-the-container.test.ts +++ b/src/__tests__/published-env-reaches-the-container.test.ts @@ -34,6 +34,13 @@ import { describe, expect, it } from "vitest"; * `scripts/check-env.ts` owns that question and CI runs it against * `.env.production.example`. This is the narrower one that check-env cannot * answer, because it reads a file rather than the compose whitelist. + * + * v1.37.0 — a fourth place joined the three. Both of these variables were + * documented in the two example files and whitelisted in compose, and NEITHER + * was in `scripts/env-manifest.json` — so `pnpm check-env` had never heard of + * them, and an operator running the pre-deploy check against their own + * environment was told nothing about the two settings this guard exists for. + * Three places held them in step and the fourth, the one CI runs, did not. */ const ROOT = process.cwd(); const COMPOSE = readFileSync(join(ROOT, "docker-compose.yml"), "utf8"); @@ -42,6 +49,19 @@ const PROD_EXAMPLE = readFileSync( join(ROOT, ".env.production.example"), "utf8", ); +const MANIFEST = readFileSync(join(ROOT, "scripts/env-manifest.json"), "utf8"); + +/** Every variable name the manifest declares, in any group. */ +const MANIFEST_NAMES: string[] = ( + JSON.parse(MANIFEST) as { + groups: Array<{ variables: Array<{ name: string; anyOf?: string[] }> }>; + } +).groups.flatMap((group) => + group.variables.flatMap((variable) => [ + variable.name, + ...(variable.anyOf ?? []), + ]), +); /** * Variables whose value the product states back to a user. @@ -72,6 +92,11 @@ describe("a variable the product publishes reaches the container", () => { expect(COMPOSE).toContain("environment:"); expect(ENV_EXAMPLE.length).toBeGreaterThan(1000); expect(PROD_EXAMPLE.length).toBeGreaterThan(1000); + // The manifest really parsed, and really carries names. An empty list + // would make the manifest leg below a quiet pass for every entry. + expect(MANIFEST_NAMES.length).toBeGreaterThan(20); + expect(MANIFEST_NAMES).toContain("DATABASE_URL"); + expect(MANIFEST_NAMES).not.toContain("HEALTHLOG_NOT_A_REAL_VARIABLE"); // And a name nothing declares is genuinely absent from all three, so the // matchers below are not passing on a substring that is everywhere. for (const file of [COMPOSE, ENV_EXAMPLE, PROD_EXAMPLE]) { @@ -96,6 +121,13 @@ describe("a variable the product publishes reaches the container", () => { it("is documented in the production example", () => { expect(PROD_EXAMPLE).toContain(name); }); + + it("is declared in the manifest, so `pnpm check-env` reports it", () => { + // The fourth place, and the one CI actually runs. A variable an + // operator is told to set, and which the pre-deploy check has never + // heard of, is documented everywhere except where it would be checked. + expect(MANIFEST_NAMES).toContain(name); + }); }); } });