fix(rest): :type 段每 handler 归一一次,复数拼写不再绕过 §6.7 audience 门禁 (#6241)

.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.

.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

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",

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');
+  });
+});