fix(modules): drop the last three fail-open gates and discover the class instead of naming it

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.
This commit is contained in:
Marc-André Bombeck committed 2026-09-10 12:14:14 +02:00
1 parent efd4efa862
commit 681e69a723
6 files changed
+169 -47

No files matched your search

+67
View File
@@ -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");
}
+97 -39
View File
@@ -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([]);
});
});
+2 -4
View File
@@ -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.
*
+1 -1
View File
@@ -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,
+1 -2
View File
@@ -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();
+1 -1
View File
@@ -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