From c2d6adaff5e47b03cd30e07e0284b099fea5cab4 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Sat, 25 Jul 2026 16:24:24 +0200 Subject: [PATCH] chore(audit): sweep for write-only columns from the release battery Whether a column has a reader anywhere in the tree is a whole-program property, and grep is the only cheap approximation. Grep is unsound both ways here: generic names match unrelated identifiers and read as consumed, while consumption through a select-less read plus whole-row serialization reads as unconsumed. A gate built on that would drift towards green and stop meaning anything, which is the failure this whole line of work exists to prevent. So it stays a review tool with a human reading the output, run from the deep-audit step and never in CI. The script says so itself and prints the false-positive sources under every run. It also carries the convention that makes the tool useful: a change that adds a column, payload field, or preference contains its consumer, or the schema comment carries `@internal` for something deliberately never surfaced or `@pending-consumer(#issue)` for one still coming. The second marker is re-listed on every run, so a follow-up that never happened surfaces at the next release audit instead of never. Recorded in the contributor notes alongside the two questions that are exact enough to be gates. --- CLAUDE.md | 1 + scripts/audit-column-readers.ts | 254 ++++++++++++++++++++++++++++++++ 2 files changed, 255 insertions(+) create mode 100644 scripts/audit-column-readers.ts diff --git a/CLAUDE.md b/CLAUDE.md index e529b0e80..196234aaa 100644 --- a/CLAUDE.md +++ b/CLAUDE.md @@ -89,6 +89,7 @@ The architecture map in `.planning/codebase/arch.md` walks each layer with file: - **Every API route wraps in `apiHandler`, with three documented exceptions.** The MCP OAuth routes (`mcp/oauth/{authorize,token,register}`) use `withBackgroundEvent` instead, because OAuth requires RFC-format error bodies that the standard envelope cannot express. They carry the same protections by hand — rate limit, body cap, kill switch, mandatory PKCE-S256, redirect-URI allowlist. Every other route wraps. Every body-accepting route runs Zod `safeParse` and returns 422 via `returnAllZodIssues` (multi-issue envelope sanitised against echoed input through `sanitiseZodIssues`). - **`userId` is always narrowed from session or Bearer.** No route accepts `userId` as a body field — it comes from `requireAuth()` and feeds the Prisma `where`. The batch endpoints (`/api/measurements/batch`, `/api/mood-entries/bulk`, `/api/medications/intake/bulk`, `/api/workouts/batch`) don't even declare a `userId` field in their Zod schemas. - **No mass assignment.** Every `prisma.X.{create,update}({ data: ... })` builds its `data` object field-by-field from `parsed.data`, never by spreading the parsed object whole. `tokens.create` builds `permissions` from a hardcoded literal, never from request input. +- **A two-ended change carries both ends.** Adding a column, a payload field, or a user preference without its consumer is the failure mode this repo keeps rediscovering: the column ships with the schema change, the reader is "the follow-up", and nothing in the gate notices because every other check proves one end. So the consumer lands in the same release branch, or the schema comment says why it cannot: `/// @internal: ` for something deliberately never surfaced, `/// @pending-consumer(#issue)` for a consumer that is genuinely still coming. `pnpm dlx tsx scripts/audit-column-readers.ts` sweeps for unannotated write-only columns and re-lists every `@pending-consumer` marker so a follow-up that never happened surfaces at the next release audit. Run it in the deep-audit step; it is a review tool with a grep-shaped matcher, deliberately not a gate. The two questions that can be answered exactly are gates instead — `account-payload-consumer-guard.test.ts` (every `/api/auth/me` field has a client reader) and `measurement-backup-completeness.test.ts` (every `Measurement` column reaches the backup). - **Bearer scope is fail-closed.** `requireAuth()` with no argument admits a cookie session or a wildcard (`["*"]`) token only — a narrow-scope token is refused with 403 and an `undeclared_scope` audit row. A route that means to accept a narrow token names the scope: `requireAuth(FHIR_READ_SCOPE)`. Adding a route therefore cannot silently widen an existing token's reach, because the default arm denies. Only the login / passkey / refresh paths mint `["*"]`; every user-facing mint is narrow, and narrow now means narrow. Two surfaces resolve a Bearer token outside `requireAuth` — `/api/ingest/medication` and `/mcp` — and that set is frozen by a structural test (`src/__tests__/bearer-scope-enforcement-guard.test.ts`). - **`requireAdmin()` is cookie-only.** Bearer tokens — even with `["*"]` scope — cannot reach admin endpoints. This is a structural boundary in `requireAdmin()` (`src/lib/api-handler.ts`), not a runtime check that future code can soften. - **Raw SQL is parameter-bound or whitelist-spliced.** Every `$queryRaw` uses tagged-template parameters. Every `$queryRawUnsafe` / `$executeRawUnsafe` either passes positional `$N` placeholders with an argument array OR splices a value already asserted against a closed enum + regex (see the whitelist-splice block in `src/lib/rollups/measurement-rollups.ts`, around the `$queryRawUnsafe` calls). Inline comments document the whitelist at the splice point. diff --git a/scripts/audit-column-readers.ts b/scripts/audit-column-readers.ts new file mode 100644 index 000000000..fcbfce2ab --- /dev/null +++ b/scripts/audit-column-readers.ts @@ -0,0 +1,254 @@ +/** + * Release-battery sweep: which Prisma columns are written but never read? + * + * Run it as part of the deep-audit step, not in CI: + * + * pnpm dlx tsx scripts/audit-column-readers.ts + * pnpm dlx tsx scripts/audit-column-readers.ts --model Measurement + * pnpm dlx tsx scripts/audit-column-readers.ts --all + * + * ## Why this is a script and not a test + * + * The question "does this column have a reader anywhere in the tree" is a + * whole-program property, and the only cheap way to approximate it is grep. + * Grep is unsound in both directions here. Generic field names (`value`, + * `type`, `state`, `source`) match unrelated identifiers and read as consumed + * when they are not. Consumption through a select-less `findUnique` plus + * whole-row serialization reads as unconsumed when it is not. Raw SQL and + * dynamic selects fall outside the matcher entirely. + * + * A gate built on that matcher would drift towards green and stop meaning + * anything, which is exactly the failure this whole family of guards exists to + * prevent — a pair guard that quietly degrades into a one-end guard is worse + * than no guard, because it also removes the suspicion. So this stays a review + * tool with a human reading the output. The two questions that CAN be answered + * exactly — is every account-payload field consumed, is every Measurement + * column backed up — are tests instead: + * `src/__tests__/account-payload-consumer-guard.test.ts` and + * `src/__tests__/measurement-backup-completeness.test.ts`. + * + * ## The annotation convention + * + * A pull request that adds a column, payload field, or preference contains its + * consumer. When it genuinely cannot, the schema comment says so and this + * sweep treats the marker as an expiring allowlist entry: + * + * /// @pending-consumer(#123) — iOS reads this from v1.33; server-side unread until then. + * /// @internal: cooldown bookkeeping for the reminder cron; never surfaced. + * + * `@internal` is permanent and silences the field. `@pending-consumer` is + * temporary: the field stays out of the findings list but is reported + * separately every run, so an issue that closed without the consumer landing + * becomes visible at the next release audit instead of never. + */ +import { readFileSync, readdirSync, statSync } from "node:fs"; +import { join } from "node:path"; + +const ROOT = join(import.meta.dirname, ".."); +const SCHEMA = join(ROOT, "prisma", "schema.prisma"); +const SRC = join(ROOT, "src"); + +/** `generated` is the Prisma client: megabytes of noise, and never a consumer. */ +const SKIP_DIRS = new Set([ + "generated", + "node_modules", + ".next", + "__tests__", + "__mocks__", +]); + +/** + * Names too generic for the matcher to say anything useful about. Reported in + * their own section rather than dropped, so the sweep never claims a clean + * bill it cannot back up. + */ +const GENERIC_NAMES = new Set([ + "id", + "name", + "type", + "value", + "unit", + "source", + "state", + "status", + "label", + "kind", + "data", + "key", + "code", + "url", + "title", + "notes", + "userId", + "createdAt", + "updatedAt", + "deletedAt", +]); + +interface Field { + model: string; + name: string; + annotation: "internal" | "pending-consumer" | null; + annotationText: string | null; +} + +function walk(dir: string, out: string[] = []): string[] { + for (const entry of readdirSync(dir)) { + if (SKIP_DIRS.has(entry)) continue; + const p = join(dir, entry); + if (statSync(p).isDirectory()) walk(p, out); + else if (entry.endsWith(".ts") || entry.endsWith(".tsx")) out.push(p); + } + return out; +} + +function parseSchema(schema: string): Field[] { + const models = new Set( + [...schema.matchAll(/^model\s+([A-Za-z_]\w*)\s*\{/gm)].map((m) => m[1]), + ); + const fields: Field[] = []; + + for (const model of models) { + const start = schema.indexOf(`model ${model} {`); + const end = schema.indexOf("\n}", start); + const lines = schema.slice(start, end).split("\n").slice(1); + + let pending: string[] = []; + for (const rawLine of lines) { + const line = rawLine.trim(); + if (!line) continue; + if (line.startsWith("//") || line.startsWith("///")) { + pending.push(line); + continue; + } + if (line.startsWith("@@")) { + pending = []; + continue; + } + const m = /^([a-zA-Z_]\w*)\s+([A-Za-z_]\w*)(\[\])?(\?)?/.exec(line); + if (!m) { + pending = []; + continue; + } + const [, name, type, isList] = m; + if (isList || models.has(type) || line.includes("@relation")) { + pending = []; + continue; + } + const comment = [...pending, line].join(" "); + let annotation: Field["annotation"] = null; + let annotationText: string | null = null; + const internal = /@internal:?\s*(.*)$/.exec(comment); + const pendingConsumer = /@pending-consumer\(([^)]*)\)\s*(.*)$/.exec( + comment, + ); + if (pendingConsumer) { + annotation = "pending-consumer"; + annotationText = `${pendingConsumer[1]} ${pendingConsumer[2]}`.trim(); + } else if (internal) { + annotation = "internal"; + annotationText = internal[1].trim(); + } + fields.push({ model, name, annotation, annotationText }); + pending = []; + } + } + + return fields; +} + +interface Usage { + writes: number; + reads: number; + selects: number; +} + +function countUsage(text: string, field: string): Usage { + const esc = field.replace(/[.*+?^${}()|[\]\\]/g, "\\$&"); + const selects = text.match( + new RegExp(`(? !modelFilter || f.model === modelFilter, + ); + const text = walk(SRC) + .map((f) => readFileSync(f, "utf8")) + .join("\n"); + + const findings: Array = []; + const generic: Array = []; + const pendingConsumers: Field[] = []; + + for (const field of fields) { + if (field.annotation === "pending-consumer") pendingConsumers.push(field); + if (field.annotation) continue; + const usage = countUsage(text, field.name); + if (usage.reads > 0 || usage.selects > 0) continue; + if (usage.writes === 0) continue; + if (GENERIC_NAMES.has(field.name) && !includeGeneric) { + generic.push({ ...field, ...usage }); + continue; + } + findings.push({ ...field, ...usage }); + } + + const scope = modelFilter ? `model ${modelFilter}` : "every model"; + console.log( + `Column-reader sweep over ${scope} — ${fields.length} scalar column(s).\n`, + ); + + if (findings.length === 0) { + console.log("No unannotated write-only columns matched.\n"); + } else { + console.log( + `${findings.length} column(s) written but never read or selected — verify each by hand:\n`, + ); + for (const f of findings) { + console.log(` ${f.model}.${f.name} (writes: ${f.writes})`); + } + console.log(""); + } + + if (pendingConsumers.length > 0) { + console.log( + `${pendingConsumers.length} column(s) marked @pending-consumer — check whether the issue closed without the consumer landing:\n`, + ); + for (const f of pendingConsumers) { + console.log(` ${f.model}.${f.name} ${f.annotationText ?? ""}`); + } + console.log(""); + } + + if (generic.length > 0) { + console.log( + `${generic.length} match(es) suppressed as too generic to judge (re-run with --all to see them).\n`, + ); + } + + console.log( + "Expect false positives: raw SQL, dynamic selects, and whole-row serialization\n" + + "all read columns this matcher cannot see. Confirm a finding before acting on it.", + ); +} + +main();