fix: close two gaps the last audit round left open

- give the delegated-action inventory a leg that can falsify its level
  axis: the walked module set must strictly exceed the manage-only one,
  anchored on a route that never asks for manage
- declare AUDIT_LOG_RETENTION_DAYS and SESSION_COOKIE_SECURE in the env
  manifest, so the pre-deploy check reports the two settings operators
  are told to set, and hold manifest, compose whitelist and both example
  files in step
This commit is contained in:
Marc-André Bombeck committed 2026-08-07 02:26:29 +02:00
1 parent 6bfcccd755
commit e4870f3b5f
3 files changed
+92 -2

No files matched your search

+17
View File
@@ -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"
}
]
}
]
}
@@ -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");
@@ -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);
});
});
}
});