From 24d548ce3681b9e1eaa066b8160dc2edb9671360 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 7 Aug 2026 14:23:12 +0000 Subject: [PATCH] =?UTF-8?q?fix(rest):=20normalize=20the=20`:type`=20segmen?= =?UTF-8?q?t=20once=20per=20handler=20so=20the=20plural=20spelling=20canno?= =?UTF-8?q?t=20skip=20the=20=C2=A76.7=20audience=20gate=20(#6241)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The single-item metadata read's cached branch excluded `doc` / `book` by comparing the RAW `:type` path segment against singular literals. The route serves both spellings and Prime Directive #3 makes the plural one canonical, so `GET /api/v1/meta/books/:name` did not match the exclusion, took the cached branch, and the ADR-0046 §6.7 audience gate — which lives in the uncached branch — never ran. `enableCache` defaults to true, so the failing path was the default one, and the failure was fail-open: a `{ permissionSet }`-gated book was served in full to a signed-in caller holding no set. This is #3984 recurring in the same file eight days later, so the fix takes the structural form #3984 already ruled rather than correcting two literals: the handler normalizes once at the top (`metaType`) and every gate below reads that local. The cache exclusion and the §6.7 gate now share one predicate (`isAudienceGatedType`), so they cannot drift apart. Also adds `check:meta-type-normalized` — an AST-based guard (comments invisible, so the file's own post-mortems still quote the bad pattern) refusing any raw `:type` comparison, switch discriminant or membership test in packages/rest/src. Zero exemptions. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01Wbxm29qPKnLf44AbSxizqW --- .../meta-plural-audience-gate-bypass.md | 52 ++++ .github/workflows/lint.yml | 14 + package.json | 1 + .../rest/src/meta-audience-plural.test.ts | 163 ++++++++++- packages/rest/src/rest-server.ts | 84 +++++- scripts/check-meta-type-normalized.mjs | 276 ++++++++++++++++++ 6 files changed, 574 insertions(+), 16 deletions(-) create mode 100644 .changeset/meta-plural-audience-gate-bypass.md create mode 100644 scripts/check-meta-type-normalized.mjs diff --git a/.changeset/meta-plural-audience-gate-bypass.md b/.changeset/meta-plural-audience-gate-bypass.md new file mode 100644 index 0000000000..57798bf074 --- /dev/null +++ b/.changeset/meta-plural-audience-gate-bypass.md @@ -0,0 +1,52 @@ +--- +"@objectstack/rest": patch +--- + +fix(rest): `GET /meta/books/:name` no longer bypasses the ADR-0046 §6.7 audience gate (#6241) + +The single-item metadata read has a cached branch and an uncached one, and the +ADR-0046 §6.7 audience gate lives in the uncached one. The comment above the +cached branch's entry condition has always stated why `doc` and `book` must skip +it: + +> `doc` and `book` bypass the shared cache: their §6.7 audience gate is +> per-caller, and a shared ETag would leak gated content across viewers. + +The condition beneath that sentence compared the **raw** `:type` path segment +against the literals `'doc'` / `'book'`. The route serves both spellings, and +Prime Directive #3 makes the **plural** one canonical — so +`GET /api/v1/meta/books/:name` did not match the exclusion, took the cached +branch, and the audience gate never ran. `enableCache` defaults to `true`, which +made the failing path the default one. + +Measured against a real `RestServer` — one book declaring +`audience: { permissionSet: … }`, one signed-in caller holding no permission +set: + +``` +singular "book" :: cachedCalls=0 status=[403] PERMISSION_DENIED +plural "books" :: cachedCalls=1 status=[] full gated body served +``` + +Same book, same caller, two spellings of one route. `GET /meta/docs/:name` took +the same path. This was **fail-open**: the wrong outcome is disclosure of gated +documentation, not an availability error. + +**The fix is structural, not two corrected literals.** This is #3984 recurring +in the same file eight days later, so the handler now normalizes the type +**once** at the top (`RestServer.metaTypeSingular`) and every gate below reads +that local — a per-type gate added later has no raw param in scope to compare +against by accident. The cache exclusion and the §6.7 gate now read one shared +predicate, so "which types bypass the cache" and "which types are audience +gated" can no longer drift apart. A repository guard +(`pnpm check:meta-type-normalized`, AST-based, zero exemptions) refuses the next +raw comparison in `packages/rest/src`. + +**Behaviour change worth knowing:** `GET /meta/docs/:name` and +`GET /meta/books/:name` now take the uncached branch, as their singular +spellings always did, so those two responses no longer carry an `ETag` / +`Cache-Control` validator and a conditional request no longer answers `304`. No +other metadata type is affected. The cost is only the 304's saved bytes — +`getMetaItemCached` delegates to `getMetaItem`, so the server does identical +work either way — and the ETag it gave up was a hash of the **unfiltered** +document, which is the cross-viewer leak the exclusion exists to prevent. diff --git a/.github/workflows/lint.yml b/.github/workflows/lint.yml index 659f37f398..fa442ab2c6 100644 --- a/.github/workflows/lint.yml +++ b/.github/workflows/lint.yml @@ -211,6 +211,20 @@ jobs: - name: Wildcard fall-through guard run: pnpm check:wildcard-fallthrough + # Raw `:type` route-param comparison guard (#6241). The `/meta/:type` + # routes serve BOTH spellings and Prime Directive #3 makes the PLURAL one + # canonical, so a gate comparing the raw param is a gate the canonical + # spelling skips. That is one authorization bypass fixed three times in + # one file: #3984 (every per-type gate), #5881 (the dashboard exclusion), + # #6241 (the doc/book cache exclusion, still literal eight days after + # #3984's structural fix). Each was found by hand; per-defect tests pin + # the gates that exist today and nothing refused the NEXT raw comparison. + # AST-based on purpose — the file documents the bad pattern in prose, and + # a textual scan would flag its own post-mortems. Zero exemptions today. + # Runs its own --self-test first. + - name: Normalized metadata-type guard + run: pnpm check:meta-type-normalized + # Init-service declaration guard (#4471, ADR-0116). The kernel's ordering # contract (dependencies / optionalDependencies / requiresServices / # providesServices) was complete but VOLUNTARY: a plugin that resolves diff --git a/package.json b/package.json index fef9dbea17..14bbd7e9c4 100644 --- a/package.json +++ b/package.json @@ -48,6 +48,7 @@ "check:route-envelope": "node scripts/check-route-envelope.mjs --self-test && node scripts/check-route-envelope.mjs", "check:error-code-casing": "node scripts/check-error-code-casing.mjs --self-test && node scripts/check-error-code-casing.mjs", "check:wildcard-fallthrough": "node scripts/check-wildcard-fallthrough.mjs --self-test && node scripts/check-wildcard-fallthrough.mjs", + "check:meta-type-normalized": "node scripts/check-meta-type-normalized.mjs --self-test && node scripts/check-meta-type-normalized.mjs", "check:init-service-contract": "node scripts/check-init-service-contract.mjs --self-test && node scripts/check-init-service-contract.mjs", "check:durability-log-level": "node scripts/check-durability-degradation-log-level.mjs --self-test && node scripts/check-durability-degradation-log-level.mjs", "check:startup-registry-verdict": "node scripts/check-startup-registry-verdict.mjs --self-test && node scripts/check-startup-registry-verdict.mjs", diff --git a/packages/rest/src/meta-audience-plural.test.ts b/packages/rest/src/meta-audience-plural.test.ts index 5723077406..f2dfd8632e 100644 --- a/packages/rest/src/meta-audience-plural.test.ts +++ b/packages/rest/src/meta-audience-plural.test.ts @@ -69,7 +69,10 @@ async function getItem(rest: any, type: string, name: string) { const route = rest.getRoutes().find((r: any) => r.method === 'GET' && r.path === '/api/v1/meta/:type/:name'); if (!route) throw new Error('meta/:type/:name route not registered'); const res = makeRes(); - await route.handler({ method: 'GET', params: { type, name }, query: {}, body: {} }, res); + // `headers` is not optional dressing: the cached branch reads + // `req.headers['if-none-match']`, so a request object without it would throw + // its way into a 400 and read as "the gate denied" for the wrong reason. + await route.handler({ method: 'GET', params: { type, name }, query: {}, body: {}, headers: {} }, res); return res; } @@ -129,3 +132,161 @@ describe('the same spelling sensitivity on the other per-type gates', () => { expect(names(plural.body)).toEqual(names(singular.body)); }); }); + +// --------------------------------------------------------------------------- +// [#6241] The same gate, one branch further in: the CACHED read path. +// +// Everything above tests a protocol double with no `getMetaItemCached`, so the +// single-item read always fell through to the uncached branch — the branch that +// holds the §6.7 gate. A real deployment does not look like that: `enableCache` +// defaults to `true` and the metadata protocol ships `getMetaItemCached`, so the +// DEFAULT single-item read took the cached branch, whose entry condition +// excluded `doc` / `book` by LITERAL comparison against the raw `:type` segment: +// +// … && req.params.type !== 'doc' && req.params.type !== 'book' +// +// `/meta/books/:name` is the canonical spelling (Prime Directive #3) and the +// route serves it, so the plural read walked past the exclusion, took the cached +// branch, and the audience gate never ran. Measured on the real `RestServer` +// before the fix — one `{ permissionSet }`-gated book, one signed-in caller who +// holds no set: +// +// singular "book" :: cachedCalls=0 status=[403] PERMISSION_DENIED +// plural "books" :: cachedCalls=1 status=[] full gated body served +// +// That is #3984's defect recurring in the same file, and it is why the fix +// normalizes once at the top of the handler instead of adding a third correctly +// normalized comparison beside two wrong ones. +// +// The trade this pins: `docs` / `books` plural reads now leave the cached +// branch, so they carry no ETag. Same trade #5881 made for `dashboard`, on a +// harder reason — the comment above the exclusion has always said a shared ETag +// over a per-caller-gated document leaks it across viewers, and +// `getMetaItemCached` delegates to `getMetaItem`, so only the 304's saved bytes +// are given up. +// --------------------------------------------------------------------------- +describe('#6241 — the cached branch cannot be spelled around either', () => { + /** A doc the gated book claims by rule, plus one no book claims. */ + const ADMIN_DOC = { name: 'admin_runbook', label: 'Runbook' }; + const OPEN_DOC = { name: 'intro', label: 'Intro' }; + const CLAIMING_GATED_BOOK = { + name: 'admin_guide', + label: 'Admin Guide', + audience: { permissionSet: 'crm_admin' }, + groups: [{ key: 'admin', label: 'Admin', include: 'admin_*' }], + }; + /** A type with no per-caller gate at all — the positive control's subject. */ + const VIEW_ITEM = { name: 'account_list', label: 'Accounts' }; + + /** + * The DEFAULT deployment shape: no `metadata` block at all (so `enableCache` + * is its default `true`) and a protocol offering BOTH reads, so which branch + * the handler picks is the thing under test rather than an artefact of a + * double that only implements one. + */ + function setupCached() { + const protocol: any = { + getDiscovery: vi.fn().mockResolvedValue({ version: 'v0', routes: { data: '', metadata: '', ui: '', auth: '/auth' } }), + getMetaTypes: vi.fn().mockResolvedValue([]), + getMetaItems: vi.fn(async ({ type }: any) => { + const t = String(type ?? ''); + if (t === 'book' || t === 'books') return [PUBLIC_BOOK, CLAIMING_GATED_BOOK]; + if (t === 'doc' || t === 'docs') return [ADMIN_DOC, OPEN_DOC]; + return []; + }), + getMetaItem: vi.fn(async ({ type, name }: any) => { + const all: any[] = [PUBLIC_BOOK, CLAIMING_GATED_BOOK, ADMIN_DOC, OPEN_DOC, VIEW_ITEM]; + const item = all.find((i) => i.name === name); + return item ? { type, name, item } : { type, name }; + }), + // Present and eligible — exactly what a default deployment has, and what + // every test above this line was missing. It answers the UNFILTERED + // document with an ETag over it, which is the leak the exclusion exists + // to prevent. + getMetaItemCached: vi.fn(async ({ name }: any) => ({ + data: [PUBLIC_BOOK, CLAIMING_GATED_BOOK, ADMIN_DOC, OPEN_DOC, VIEW_ITEM] + .find((i) => i.name === name), + etag: { value: 'etag-unfiltered', weak: false }, + cacheControl: { directives: ['private', 'no-cache'] }, + notModified: false, + })), + findData: vi.fn().mockResolvedValue([]), + }; + const rest: any = new RestServer(createMockServer() as any, protocol, { api: { requireAuth: false } } as any); + // A signed-in caller who holds no permission set — the 403 case, not the + // anonymous 401 one, so the pin cannot pass by accident on the auth gate. + rest.resolveExecCtx = async () => ({ userId: 'u1' }); + rest.securityServiceProvider = async () => ({ resolvePermissionSetNames: async () => [] }); + rest.registerRoutes(); + return { rest, protocol }; + } + + it('a {permissionSet}-gated book is 403 on the PLURAL spelling, not 200', async () => { + const { rest, protocol } = setupCached(); + const res = await getItem(rest, 'books', 'admin_guide'); + + // Before the fix: 200 with `{ item: { audience: { permissionSet: … } } }`. + expect(res.statusCode).toBe(403); + expect(res.body?.code ?? res.body?.error?.code).toBe('PERMISSION_DENIED'); + // …and it got there by NOT taking the cached branch, which is the actual + // mechanism — asserting only the status would leave a fix that gated the + // cached body under an unfiltered ETag looking correct. + expect(protocol.getMetaItemCached).not.toHaveBeenCalled(); + }); + + it('the singular spelling keeps denying it — no regression on the path that worked', async () => { + const { rest, protocol } = setupCached(); + const res = await getItem(rest, 'book', 'admin_guide'); + + expect(res.statusCode).toBe(403); + expect(protocol.getMetaItemCached).not.toHaveBeenCalled(); + }); + + it('a doc claimed only by the gated book is 403 on /meta/docs/:name too', async () => { + // §6.7 effective audience: the union over the books claiming the doc. The + // gated book's `include: admin_*` rule claims `admin_runbook`, so a + // non-holder is denied — on either spelling. + const { rest, protocol } = setupCached(); + const plural = await getItem(rest, 'docs', 'admin_runbook'); + expect(plural.statusCode).toBe(403); + expect(protocol.getMetaItemCached).not.toHaveBeenCalled(); + + const singular = await getItem(rest, 'doc', 'admin_runbook'); + expect(singular.statusCode).toBe(403); + }); + + it('an unclaimed doc still reads (org default) — the gate narrows, it does not close the surface', async () => { + const { rest } = setupCached(); + // `intro` is claimed by no book → effective audience `org` → a signed-in + // caller may read it. Without this the three assertions above would be + // satisfied by a fix that denied every doc/book read. + expect((await getItem(rest, 'docs', 'intro')).statusCode).toBe(200); + expect((await getItem(rest, 'books', 'manual')).statusCode).toBe(200); + }); + + it('positive control: a non-gated type still takes the cached branch, ETag and all', async () => { + // The bypass is only correct if it bypasses exactly the gated types. A fix + // that disabled the cache wholesale would satisfy every assertion above and + // silently cost every other metadata read its validator. + const { rest, protocol } = setupCached(); + const res = await getItem(rest, 'views', 'account_list'); + + expect(protocol.getMetaItemCached).toHaveBeenCalledTimes(1); + expect(protocol.getMetaItem).not.toHaveBeenCalled(); + expect(res.header.mock.calls.map((c: any[]) => c[0])).toContain('ETag'); + + // …and the singular spelling of that same non-gated type, so the fix is + // "normalize", not "move the doc/book hole onto some other type". + const { rest: rest2, protocol: protocol2 } = setupCached(); + await getItem(rest2, 'view', 'account_list'); + expect(protocol2.getMetaItemCached).toHaveBeenCalledTimes(1); + }); + + it('the price of the bypass, pinned rather than hidden: gated reads carry no ETag', async () => { + const { rest } = setupCached(); + const res = await getItem(rest, 'books', 'manual'); + + expect(res.statusCode).toBe(200); + expect(res.header.mock.calls.map((c: any[]) => c[0])).not.toContain('ETag'); + }); +}); diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index d006ac22fc..de745762f5 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -2240,6 +2240,15 @@ export class RestServer { * back to a caller who does not hold the set, and an `org` book came back to * an anonymous reader on a publicly-served deployment. Same route, gate * enforced on one spelling of it. + * + * Calling this at each gate is NOT the durable form — #6241 proved it. + * Eight days after #3984, the single-item read's cache-branch condition + * still excluded `doc`/`book` by literal comparison, so the plural read + * skipped the branch that holds the gate and the same authorization hole + * came back on the same route. The handlers therefore normalize ONCE at + * the top (`const metaType = RestServer.metaTypeSingular(req.params.type)`) + * and every gate reads that local: a gate added later has no raw param in + * scope to compare against. */ private static metaTypeSingular(type: unknown): string { const t = typeof type === 'string' ? type : ''; @@ -4208,6 +4217,33 @@ export class RestServer { const environmentId = isScoped ? req.params?.environmentId : undefined; const p = await this.resolveProtocol(environmentId, req); + // [#3984 / #6241] Normalize the `:type` segment ONCE, + // here at the top, and let every gate below read THIS + // value. The route serves both spellings and Prime + // Directive #3 makes the plural one canonical + // (`/meta/books/:name`), so any gate comparing the raw + // param is a gate the canonical spelling walks past. + // + // #3984 ruled this shape for exactly that reason ("每个 + // handler 顶部归一一次,后续所有闸门都用归一后的值"), and + // #6241 is why the ruling is written into the code + // rather than trusted to memory: eight days after + // #3984 landed, the cache-branch condition below still + // excluded `doc`/`book` by LITERAL comparison, so + // `GET /meta/books/:name` took the cached branch and + // the §6.7 audience gate — which lives in the uncached + // branch — never ran at all. Measured on the real + // server, one `{ permissionSet }`-gated book, one + // signed-in caller holding no set: + // + // singular "book" :: cachedCalls=0 status=[403] + // plural "books" :: cachedCalls=1 status=[] ← full body served + // + // A new per-type gate added below inherits the + // normalization by default now; there is no raw param + // in scope for it to compare against by accident. + const metaType = RestServer.metaTypeSingular(req.params.type); + // Phase 3a-layered-get: opt-in 3-state view when client // asks for `?layers=true` (or any non-empty value). // Skips the cache path entirely — layered view is a @@ -4249,7 +4285,7 @@ export class RestServer { // viewers of the same app schema. Drafts also // bypass cache: the cache is keyed on the // published checksum and drafts are out-of-band. - const isAppType = RestServer.metaTypeSingular(req.params.type) === 'app'; + const isAppType = metaType === 'app'; const isDraftRead = typeof req.query?.state === 'string' && req.query.state.toLowerCase() === 'draft'; // ADR-0033/0037 — `?preview=draft` overlays a pending @@ -4273,6 +4309,21 @@ export class RestServer { // audience gate is per-caller, and a shared ETag would // leak gated content across viewers. // + // [#6241] That sentence was already here while the + // exclusion beneath it compared the RAW param against + // the literals `'doc'` / `'book'`, so the canonical + // plural spelling took the cached branch and shipped + // the gated body. The exclusion is not incidental + // tidying — it is the stated security invariant above, + // and it now reads the normalized `metaType`. + // + // The predicate is ONE named value shared with the §6.7 + // gate in the uncached branch (`isAudienceGatedType`), + // so "which types bypass the cache" and "which types + // are audience-gated" can no longer drift apart: the + // bypass exists only to make that gate reachable, and a + // future third gated type joins both sites at once. + // // [#5881] `dashboard` bypasses it too, and the reason is // NOT the one above — worth writing down, because the // obvious reading says a dashboard needn't bypass at all. @@ -4305,15 +4356,19 @@ export class RestServer { // so the server does identical work either way and only // the 304's saved body bytes are given up. // - // Compared on the NORMALIZED type, like `isAppType` and - // unlike the two literals at the end of this condition - // (`/meta/dashboards/x` is the canonical plural spelling - // under Prime Directive #3, and an exclusion it could be - // spelled around would not be an exclusion). The `doc` / - // `book` literals have exactly that hole — measured and - // filed as #6241, deliberately not fixed here. - const isDashboardType = RestServer.metaTypeSingular(req.params.type) === 'dashboard'; - if (metadata.enableCache && p.getMetaItemCached && !isAppType && !isDashboardType && !isDraftRead && !previewDrafts && !packageScoped && req.params.type !== 'doc' && req.params.type !== 'book') { + // Compared on the NORMALIZED type, like every other + // exclusion in this condition (`/meta/dashboards/x` is + // the canonical plural spelling under Prime Directive + // #3, and an exclusion it could be spelled around would + // not be an exclusion). The `doc` / `book` literals + // that stood at the end of this condition had exactly + // that hole; #6241 closed it. + const isDashboardType = metaType === 'dashboard'; + // ADR-0046 §6.7 — the two audience-gated types. Read by + // the cache exclusion here AND by the gate itself in + // the uncached branch below; one predicate, two sites. + const isAudienceGatedType = metaType === 'book' || metaType === 'doc'; + if (metadata.enableCache && p.getMetaItemCached && !isAppType && !isDashboardType && !isDraftRead && !previewDrafts && !packageScoped && !isAudienceGatedType) { const cacheRequest = { ifNoneMatch: req.headers['if-none-match'] as string, ifModifiedSince: req.headers['if-modified-since'] as string, @@ -4383,7 +4438,7 @@ export class RestServer { // and never consulted the lock resolver; a caller that // needs the ADR-0008 OCC carriers reads the uncached path. const cachedEnvelope = { - type: RestServer.metaTypeSingular(req.params.type), + type: metaType, name: req.params.name, }; res.json(await this.translateMetaEnvelope( @@ -4458,8 +4513,7 @@ export class RestServer { // it, unclaimed → org). 401 for anonymous, 403 for an // authenticated non-holder; fail closed when holdings // cannot be resolved (ADR-0049). - const audienceGatedType = RestServer.metaTypeSingular(req.params.type); - if ((audienceGatedType === 'book' || audienceGatedType === 'doc') && visible) { + if (isAudienceGatedType && visible) { const { audienceAllows, docAudienceAllows, resolveDocAudiences } = await import('@objectstack/spec/system'); // The document under audience test. [#5563] This @@ -4472,7 +4526,7 @@ export class RestServer { const target = visible; let caller: { authenticated: boolean; permissionSets?: string[] }; let allowed: boolean; - if (audienceGatedType === 'book') { + if (metaType === 'book') { caller = await this.resolveAudienceCaller(environmentId, req, { needPermissionSets: RestServer.anyPermissionSetAudience([target]), }); @@ -4514,7 +4568,7 @@ export class RestServer { // ADR-0046 i18n: collapse the doc to the request // locale (label/description/content) and drop the // `translations` map so consumers get one body. - if (audienceGatedType === 'doc' && visible) { + if (metaType === 'doc' && visible) { const locale = this.extractLocale(req); const { resolveDocLocale } = await import('@objectstack/spec/system'); visible = resolveDocLocale(visible as any, locale); diff --git a/scripts/check-meta-type-normalized.mjs b/scripts/check-meta-type-normalized.mjs new file mode 100644 index 0000000000..4999c811d3 --- /dev/null +++ b/scripts/check-meta-type-normalized.mjs @@ -0,0 +1,276 @@ +#!/usr/bin/env node +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * Raw `:type` route-param comparison guard (#6241). + * + * ## What it guards + * + * The `/meta/:type` routes serve BOTH spellings of the type segment — the + * protocol normalizes singular <-> plural — and Prime Directive #3 makes the + * PLURAL one canonical (`/api/v1/meta/books/:name`). So a gate that compares + * the RAW `:type` param against a singular literal is a gate the canonical + * spelling walks straight past: + * + * if (… && req.params.type !== 'doc' && req.params.type !== 'book') { // BAD + * if (… && metaType !== 'doc' && metaType !== 'book') { // GOOD + * + * Both spellings reach the same handler, so the two halves of that `if` are + * the same request answered two different ways. + * + * ## Why a scan, and why now + * + * This is not a hypothetical. It is one defect that has now been fixed three + * times in one file: + * + * - #3984 — every per-type gate on `/meta` compared the literal singular, so + * the plural spelling bypassed ALL of them: book audience, app RBAC, + * dashboard capability. Measured: `GET /meta/book/admin_guide` -> 401, + * `GET /meta/books/admin_guide` -> 200. Its ruled fix was structural — + * normalize once at the top of each handler, let every gate read that. + * - #5881 — the same file's cached-read exclusion listed `app` but not + * `dashboard`, so the ADR-0057 D10 widget gate never ran on the default + * path. Fixed with a NORMALIZED comparison. + * - #6241 — eight days after #3984 landed, the same cached-read exclusion + * still spelled `doc` / `book` as RAW literals, so `GET /meta/books/:name` + * took the cached branch and the ADR-0046 §6.7 audience gate — which lives + * in the other branch — never ran at all. A `{ permissionSet }`-gated book + * was served in full to a signed-in caller holding no set. + * + * Each fix was correct and each was found by hand, by someone reading the file + * for another reason. Per-defect tests pin the gates that exist today; nothing + * refuses the NEXT raw comparison, which is the only thing that would have + * stopped the third instance. This scan is that refusal. + * + * ## Why AST, not grep + * + * The pattern's own documentation quotes it. `rest-server.ts` carries several + * JSDoc blocks containing the literal text `req.params.type === 'book'` to + * explain what went wrong — a textual scan would flag the explanation and force + * whoever writes the next post-mortem to obfuscate it. The AST does not see + * comments, so the guard and the history can coexist. + * + * ## What is covered, and what is not + * + * Covered, on any `*.params.type` / `*.params?.type` chain: + * - `===` / `!==` / `==` / `!=` comparisons, either side; + * - `switch` discriminants; + * - membership tests: `[...].includes(raw)`, `set.has(raw)`, `[…].indexOf(raw)`. + * + * NOT covered, and deliberately named rather than implied: a raw param copied + * into a local first (`const t = req.params.type; if (t === 'doc')`). Detecting + * that needs dataflow, and a guard that claims more than it checks is worse + * than one that says where it stops. The convention the guard exists to protect + * makes the copy unnecessary anyway — the local you want already exists, and it + * is the normalized one. + * + * PASS-THROUGHS ARE NOT COMPARISONS and are not flagged. Handing the raw param + * to the protocol (`p.getMetaItem({ type: req.params.type, … })`) is correct: + * the metadata protocol folds plural -> singular itself (#4432), and it is the + * layer that owns that normalization. What this guard bans is the REST layer + * making a DECISION on the un-normalized value. + * + * ## Exemptions + * + * `EXEMPT` maps `:` to a reason. It is empty + * today, and that is the point: #6241 removed the last raw comparison from + * `packages/rest/src`, so this starts from zero rather than from a ratchet. + * A new entry needs a reason a reader can check, not a name. + */ + +import { readFileSync, readdirSync, statSync } from 'node:fs'; +import { join, relative, sep } from 'node:path'; +import { fileURLToPath } from 'node:url'; +import ts from 'typescript'; + +const ROOT = join(fileURLToPath(new URL('.', import.meta.url)), '..'); + +/** Directories scanned. Route handlers that read `:type` live here. */ +const SCAN_DIRS = [join('packages', 'rest', 'src')]; + +/** + * Blessed raw comparisons: `'::'` -> reason. + * Empty by design — see the header. + */ +const EXEMPT = Object.create(null); + +/** Membership calls that decide on their argument the way a comparison does. */ +const MEMBERSHIP_METHODS = new Set(['includes', 'has', 'indexOf', 'lastIndexOf']); + +const COMPARISON_OPS = new Set([ + ts.SyntaxKind.EqualsEqualsEqualsToken, + ts.SyntaxKind.ExclamationEqualsEqualsToken, + ts.SyntaxKind.EqualsEqualsToken, + ts.SyntaxKind.ExclamationEqualsToken, +]); + +/** + * Is this node the RAW route param — a `.params.type` / `.params?.type` chain? + * Matches any receiver (`req`, `request`, `ctx.req`, …) so a renamed handler + * argument cannot slip past. + */ +function isRawTypeParam(node) { + if (!ts.isPropertyAccessExpression(node)) return false; + if (node.name.text !== 'type') return false; + const owner = node.expression; + if (ts.isPropertyAccessExpression(owner)) return owner.name.text === 'params'; + // `req.params?.type` parses the `params` hop as the same node shape; an + // element access (`req['params'].type`) is covered here too. + if (ts.isElementAccessExpression(owner)) { + const arg = owner.argumentExpression; + return !!arg && ts.isStringLiteralLike(arg) && arg.text === 'params'; + } + return false; +} + +function walkFiles(dir, out) { + let entries; + try { + entries = readdirSync(dir); + } catch { + return out; + } + for (const entry of entries) { + if (entry === 'node_modules' || entry === 'dist') continue; + const full = join(dir, entry); + if (statSync(full).isDirectory()) { + walkFiles(full, out); + } else if (/\.(m|c)?tsx?$/.test(entry) && !/\.(test|spec)\.(m|c)?tsx?$/.test(entry)) { + out.push(full); + } + } + return out; +} + +/** Every raw-param decision site in one file. */ +function findViolations(file) { + const text = readFileSync(file, 'utf8'); + const source = ts.createSourceFile(file, text, ts.ScriptTarget.Latest, true); + const found = []; + + const record = (node, kind) => { + const { line } = source.getLineAndCharacterOfPosition(node.getStart(source)); + found.push({ + file: relative(ROOT, file).split(sep).join('/'), + line: line + 1, + kind, + text: node.getText(source).replace(/\s+/g, ' ').trim(), + }); + }; + + const visit = (node) => { + if (ts.isBinaryExpression(node) && COMPARISON_OPS.has(node.operatorToken.kind)) { + if (isRawTypeParam(node.left) || isRawTypeParam(node.right)) { + record(node, 'comparison'); + } + } else if (ts.isSwitchStatement(node) && isRawTypeParam(node.expression)) { + record(node.expression, 'switch discriminant'); + } else if ( + ts.isCallExpression(node) + && ts.isPropertyAccessExpression(node.expression) + && MEMBERSHIP_METHODS.has(node.expression.name.text) + && node.arguments.some((a) => isRawTypeParam(a)) + ) { + record(node, 'membership test'); + } + ts.forEachChild(node, visit); + }; + ts.forEachChild(source, visit); + return found; +} + +/** + * Self-test: the guard must catch each covered shape and must NOT catch the + * two things that are legitimately raw — a pass-through into the protocol, and + * the pattern quoted inside a comment. A guard nobody tests is a guard that + * silently stops matching. + */ +function selfTest() { + const fixture = ` + // if (req.params.type === 'doc') {} -- quoted in a line comment + /** JSDoc quoting req.params.type !== 'book' for the post-mortem. */ + const a = req.params.type === 'doc'; + const b = 'book' !== req.params.type; + const c = req.params?.type === 'app'; + switch (req.params.type) { default: break; } + const d = ['doc', 'book'].includes(req.params.type); + const ok1 = p.getMetaItem({ type: req.params.type, name: req.params.name }); + const ok2 = RestServer.metaTypeSingular(req.params.type) === 'book'; + const ok3 = metaType === 'doc'; + `; + const source = ts.createSourceFile('fixture.ts', fixture, ts.ScriptTarget.Latest, true); + const hits = []; + const visit = (node) => { + if (ts.isBinaryExpression(node) && COMPARISON_OPS.has(node.operatorToken.kind) + && (isRawTypeParam(node.left) || isRawTypeParam(node.right))) hits.push('comparison'); + else if (ts.isSwitchStatement(node) && isRawTypeParam(node.expression)) hits.push('switch'); + else if (ts.isCallExpression(node) && ts.isPropertyAccessExpression(node.expression) + && MEMBERSHIP_METHODS.has(node.expression.name.text) + && node.arguments.some((a) => isRawTypeParam(a))) hits.push('membership'); + ts.forEachChild(node, visit); + }; + ts.forEachChild(source, visit); + + const comparisons = hits.filter((h) => h === 'comparison').length; + const problems = []; + // Three comparisons: a, b, c. NOT ok2 — its left side is the normalizer's + // return value, not the raw param, which is the whole distinction. + if (comparisons !== 3) problems.push(`expected 3 comparisons, saw ${comparisons}`); + if (!hits.includes('switch')) problems.push('missed the switch discriminant'); + if (!hits.includes('membership')) problems.push('missed the membership test'); + if (hits.length !== 5) problems.push(`expected 5 findings total, saw ${hits.length} — a pass-through or a comment was flagged`); + + if (problems.length) { + console.error('check:meta-type-normalized --self-test FAILED'); + for (const p of problems) console.error(` - ${p}`); + process.exit(1); + } + console.log('check:meta-type-normalized --self-test passed (5 shapes caught, pass-through and comments untouched)'); +} + +function main() { + if (process.argv.includes('--self-test')) { + selfTest(); + return; + } + selfTest(); + + const files = []; + for (const dir of SCAN_DIRS) walkFiles(join(ROOT, dir), files); + + const violations = []; + for (const file of files) { + for (const v of findViolations(file)) { + const key = `${v.file}::${v.text}`; + if (EXEMPT[key]) continue; + violations.push(v); + } + } + + if (violations.length === 0) { + console.log(`check:meta-type-normalized: OK (${files.length} file(s), no raw \`:type\` param decisions)`); + return; + } + + console.error(`check:meta-type-normalized: ${violations.length} raw \`:type\` param decision(s)\n`); + for (const v of violations) { + console.error(` ${v.file}:${v.line} [${v.kind}]`); + console.error(` ${v.text}`); + } + console.error(` +The \`/meta/:type\` routes serve BOTH spellings and the PLURAL one is canonical +(Prime Directive #3), so a decision made on the raw param is a decision the +canonical spelling skips. Normalize ONCE at the top of the handler and compare +against that local: + + const metaType = RestServer.metaTypeSingular(req.params.type); + … + if (metaType === 'book') { … } + +This has been the same authorization bypass three times (#3984, #5881, #6241). +If a site genuinely must read the raw spelling, add it to EXEMPT in +scripts/check-meta-type-normalized.mjs with a reason a reader can check.`); + process.exit(1); +} + +main();