From 681e69a7232cd783160a98ed1726e26811e56465 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Marc-Andr=C3=A9=20Bombeck?= Date: Thu, 10 Sep 2026 12:14:14 +0200 Subject: [PATCH] fix(modules): drop the last three fail-open gates and discover the class instead of naming it MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Six surfaces were named and fixed; nine carried the expression. `/labs`, `/medications` and `/mood` still read `inSharedRecord || user?.modules?.X`, and every precondition holds for them too: all three are presentable to their own scoped grants, all three module keys map to a real section so the masked map answers correctly for a delegate, and none of the three routes behind them carries a module gate of its own — so a typed URL rendered a working page for a record whose owner had switched the module off. The guard is a discovered sweep now rather than a six-entry list. It walks every component under `src/app` and `src/components` through the shared walker, keeps the ones that read the module map, and asserts the SHAPE: no boolean `||` joins the delegate flag to a module read. The search window cannot cross a `;`, a `{` or a `}`, so it sees one expression at a time and a delegate check elsewhere in the same file is none of its business. A tenth surface fails on arrival. Writing it turned up why the first version could not have found the three: the comment stripper every structural guard in this directory re-spells removes block comments first, and cannot tell a real opener from a `/*` inside a line comment. `mood/page-client.tsx` explains its gate with "Every `/api/mood-entries/*` route also enforces the gate", and that `/*` opened a fake block that swallowed the next 2.2 kB of real code — the gate three lines below it included. A guard reading that file was searching a source with a hole in it and reporting a clean sweep. The shared walker now carries a stripper that walks each line left to right and lets whichever opener comes first win; the published-coverage sweep beside it moved onto the same one. --- src/__tests__/helpers/source-files.ts | 67 +++++++++ .../module-gate-record-scoped.test.ts | 136 +++++++++++++----- src/__tests__/openapi-sharing-denial.test.ts | 6 +- src/app/labs/page.tsx | 2 +- src/app/medications/page-client.tsx | 3 +- src/app/mood/page-client.tsx | 2 +- 6 files changed, 169 insertions(+), 47 deletions(-) diff --git a/src/__tests__/helpers/source-files.ts b/src/__tests__/helpers/source-files.ts index 0fb711277..9a2a3cff1 100644 --- a/src/__tests__/helpers/source-files.ts +++ b/src/__tests__/helpers/source-files.ts @@ -73,3 +73,70 @@ export function walkSourceFiles( } return files; } + +/** + * A source file with its comments removed, line comments FIRST. + * + * The order is the whole reason this is shared rather than re-spelled per + * guard. Every private copy in this directory strips block comments first, + * with one non-greedy `/\*[\s\S]*?\*\//` pass, and that pass cannot tell a + * real block opener from a `/*` that happens to sit inside a line comment. + * This tree has one: `src/app/mood/page-client.tsx` explains its gate with + * "Every `/api/mood-entries/*` route also enforces the gate", and the `/*` in + * that path opened a fake block that swallowed the next 2.2 kB of real code — + * the module gate three lines below it included. A guard reading that source + * was searching a file with a hole in it and reporting a clean sweep. + * + * So each line is walked once, left to right: whichever opener comes first + * wins, a line comment ends the line, and a block comment carries its state to + * the next one. Line-comment detection needs `//` at the start of a line or + * after whitespace, so a URL inside a string survives. + * + * Its honest limit: it is a scanner, not a parser. A `/*` inside a string + * literal still opens a block, and a guard that needs more than this needs a + * real parse. + */ +export function stripComments(source: string): string { + const lines: string[] = []; + let inBlock = false; + + for (const line of source.split("\n")) { + let rest = line; + let kept = ""; + + while (rest.length > 0) { + if (inBlock) { + const closes = rest.indexOf("*/"); + if (closes === -1) break; + rest = rest.slice(closes + 2); + inBlock = false; + continue; + } + + const lineComment = rest.search(/(^|\s)\/\//); + const blockComment = rest.indexOf("/*"); + + if ( + blockComment !== -1 && + (lineComment === -1 || blockComment < lineComment) + ) { + kept += rest.slice(0, blockComment); + rest = rest.slice(blockComment + 2); + inBlock = true; + continue; + } + + if (lineComment !== -1) { + kept += rest.slice(0, rest.indexOf("//", lineComment)); + break; + } + + kept += rest; + break; + } + + lines.push(kept); + } + + return lines.join("\n"); +} diff --git a/src/__tests__/module-gate-record-scoped.test.ts b/src/__tests__/module-gate-record-scoped.test.ts index f2b7fff99..a71f3de71 100644 --- a/src/__tests__/module-gate-record-scoped.test.ts +++ b/src/__tests__/module-gate-record-scoped.test.ts @@ -1,62 +1,120 @@ /** - * A module gate on a page decides from the RECORD's map, never from "am I a - * delegate". + * A module gate decides from the RECORD's map, never from "am I a delegate". * - * The six surfaces below each resolve one module before they render, and each - * of them used to read `inSharedRecord || user?.modules?.X`. The prefix was + * Every surface that paints a module reads `user?.modules?.X`, and a run of + * them used to widen that read with `inSharedRecord || …`. The prefix was * right when it was written: `GET /api/auth/me` published the ACTOR's module * map, so a delegate's own preferences would have closed a page in somebody * else's record. Since the map became the record's, masked to what the grant * opens, the same prefix fails open in the other direction — the navigation * correctly drops the entry while a typed URL still renders the page for a - * record whose owner switched the module off. + * record whose owner switched the module off, and the routes behind these + * pages carry no module gate of their own, so the page works. * - * A structural check rather than a render, because the property is about the - * EXPRESSION: there is no way to reach these pages in a unit test without - * standing up the auth query, the router and the shell, and the thing that - * regressed is one `||`. The matcher asserts it found each gate before it - * asserts anything about it, so an expression that moves or is renamed fails - * loudly instead of passing on an empty match. + * A DISCOVERED set rather than a named one. The first version of this guard + * froze the six files one sweep happened to find; three more were carrying the + * identical expression at the time, and a named list is structurally unable to + * say so. So the sweep walks every component under `src/app` and + * `src/components`, and the property is about the SHAPE: no boolean `||` may + * join `inSharedRecord` to a module read. The window the matcher searches + * cannot cross a `;`, a `{` or a `}`, so it sees one expression at a time and + * a delegate check elsewhere in the same file is none of its business. * - * Its limit, stated: it holds these six files and knows nothing about a - * seventh. A new module page reads none of this. + * A structural check rather than a render, because the thing that regressed is + * one `||`: reaching these pages in a unit test would mean standing up the + * auth query, the router and the shell, and none of that is where the defect + * lives. + * + * Its limit, stated: it holds the two spellings of the OR. A gate that widened + * itself some other way — a ternary, a helper returning the delegate flag — + * would pass. The non-vacuity floor below is what keeps a broken walker from + * reading as a clean sweep. */ import { readFileSync } from "node:fs"; import { join } from "node:path"; import { describe, expect, it } from "vitest"; -const ROOT = process.cwd(); +import { stripComments, walkSourceFiles } from "./helpers/source-files"; -/** file → the gate assignment's left-hand side. */ -const GATES: ReadonlyArray<[string, string]> = [ - ["src/app/vaccinations/page.tsx", "enabled"], - ["src/app/cycle/page.tsx", "enabled"], - ["src/app/mental-wellbeing/page.tsx", "enabled"], - ["src/app/illness/page.tsx", "enabled"], - ["src/app/illness/[id]/page.tsx", "enabled"], - ["src/components/documents/documents-view.tsx", "moduleEnabled"], +const ROOT = process.cwd(); +/** Floors pinned below the real counts, so a broken walk throws. */ +const ROOTS: ReadonlyArray<[string, number]> = [ + ["src/app", 100], + ["src/components", 700], ]; -describe("a page's module gate reads the record's map alone", () => { - it.each(GATES)("%s", (file, binding) => { - const source = readFileSync(join(ROOT, file), "utf8"); - const match = source.match( - new RegExp(`const ${binding}\\s*=\\s*([^;]+);`, "s"), +/** The module read every gated surface makes. */ +const MODULE_READ = "user?.modules?."; + +/** + * The two spellings of the widening, in one expression each. + * + * `[^;{}]*` is the whole point: it cannot cross a statement end or a block + * boundary, so a file that reads the module map in one place and asks about + * the delegate in another does not match. + */ +const WIDENED = [ + /inSharedRecord\s*\|\|[^;{}]*user\?\.modules\?\./, + /user\?\.modules\?\.[^;{}]*\|\|\s*inSharedRecord/, +]; + +/** Every `.tsx` under the roots, with its comment-stripped source. */ +function componentSources(): Array<{ file: string; source: string }> { + const out: Array<{ file: string; source: string }> = []; + for (const [root, floor] of ROOTS) { + const files = walkSourceFiles(join(ROOT, root), { + floor, + extensions: [".tsx"], + }); + for (const rel of files) { + if (rel.includes("__tests__/")) continue; + out.push({ + file: `${root}/${rel}`, + source: stripComments(readFileSync(join(ROOT, root, rel), "utf8")), + }); + } + } + return out; +} + +describe("a module gate reads the record's map alone", () => { + const sources = componentSources(); + const gated = sources.filter(({ source }) => source.includes(MODULE_READ)); + + it("discovers a real set of module-gated surfaces", () => { + // The empty-match trap: a walker that stopped walking, or a spelling of + // the module read that moved, would make every assertion below vacuous. + expect(sources.length, "no component was walked at all").toBeGreaterThan( + 500, ); - expect(match, `no \`const ${binding} = …\` gate found in ${file}`).not.toBe( - null, - ); - const expression = match![1]; - // It has to be a module gate at all — otherwise the assertion below would - // pass on any expression that simply does not mention sharing. expect( - expression, - `${binding} in ${file} is not the module gate`, - ).toContain("modules"); + gated.length, + "no surface was found reading the module map", + ).toBeGreaterThan(20); + // Spot checks, so a sweep that found twenty of the wrong files still + // fails: these are module-gated pages by construction. + const files = new Set(gated.map((entry) => entry.file)); + for (const file of [ + "src/app/vaccinations/page.tsx", + "src/app/labs/page.tsx", + "src/app/mood/page-client.tsx", + "src/components/documents/documents-view.tsx", + ]) { + expect(files, `${file} was not discovered`).toContain(file); + } + }); + + it("finds no gate widened by the delegate flag", () => { + const widened = gated + .filter(({ source }) => WIDENED.some((pattern) => pattern.test(source))) + .map(({ file }) => file); + expect( - expression, - `${file} widens its module gate for a delegate; the record's masked map already answers for both cases`, - ).not.toContain("inSharedRecord"); + widened, + `these surfaces widen a module gate for a delegate; the record's masked map already answers for both cases:\n${widened + .map((file) => ` ❌ ${file}`) + .join("\n")}`, + ).toEqual([]); }); }); diff --git a/src/__tests__/openapi-sharing-denial.test.ts b/src/__tests__/openapi-sharing-denial.test.ts index d2dafbdac..c7d285554 100644 --- a/src/__tests__/openapi-sharing-denial.test.ts +++ b/src/__tests__/openapi-sharing-denial.test.ts @@ -63,6 +63,8 @@ import { openApiPaths } from "@/lib/openapi/routes"; import { SHARING_ACCESS_DENIED_DESCRIPTION } from "@/lib/openapi/routes/shared"; import { RETIRED_ROUTES } from "@/lib/http/retired-routes"; +import { stripComments } from "./helpers/source-files"; + /** Published paths that deliberately have no handler behind them. */ const retiredPaths = new Set(RETIRED_ROUTES.map((route) => route.path)); @@ -80,10 +82,6 @@ interface PublishedOperation { handlerSource: string; } -function stripComments(text: string): string { - return text.replace(/\/\*[\s\S]*?\*\//g, "").replace(/(^|\s)\/\/.*$/gm, "$1"); -} - /** * Every `route.ts` under `src/app/api` whose handlers resolve the record fence. * diff --git a/src/app/labs/page.tsx b/src/app/labs/page.tsx index 8d6710f6e..65824629e 100644 --- a/src/app/labs/page.tsx +++ b/src/app/labs/page.tsx @@ -63,7 +63,7 @@ export default function LabsPage() { // absent key reads as enabled, so a direct URL hit only bounces on an // explicit `false`. Every `/api/labs/*` route also enforces the gate // server-side, so this is a UX redirect, not the security boundary. - const enabled = inSharedRecord || user?.modules?.labs !== false; + const enabled = user?.modules?.labs !== false; const ocrCapability = useOcrCapability( shouldProbeOcrCapability({ isAuthenticated, diff --git a/src/app/medications/page-client.tsx b/src/app/medications/page-client.tsx index 26fc988f4..ba7ae67b3 100644 --- a/src/app/medications/page-client.tsx +++ b/src/app/medications/page-client.tsx @@ -176,8 +176,7 @@ export default function MedicationsPageClient() { // gate) and the list query never fires (the API would 403 anyway). // Default-on: an absent key reads as enabled, so the page only hides on an // explicit `false`. - const medicationsEnabled = - inSharedRecord || user?.modules?.medications !== false; + const medicationsEnabled = user?.modules?.medications !== false; const router = useRouter(); const queryClient = useQueryClient(); const searchParams = useSearchParams(); diff --git a/src/app/mood/page-client.tsx b/src/app/mood/page-client.tsx index 002fb2bbb..f86962bc5 100644 --- a/src/app/mood/page-client.tsx +++ b/src/app/mood/page-client.tsx @@ -39,7 +39,7 @@ export default function MoodPageClient() { // absent key reads as enabled, so a direct URL hit only bounces on an // explicit `false`. Every `/api/mood-entries/*` route also enforces the gate // server-side, so this is a UX redirect, not the security boundary. - const enabled = inSharedRecord || user?.modules?.mood !== false; + const enabled = user?.modules?.mood !== false; // v1.16.4 — PWA pull-to-refresh: a top-anchored touch pull refetches // whatever this page currently has mounted (`type: "active"` scopes the