diff --git a/.changeset/audit-plugin-boot-path-reachability.md b/.changeset/audit-plugin-boot-path-reachability.md new file mode 100644 index 0000000000..bb2813c6e4 --- /dev/null +++ b/.changeset/audit-plugin-boot-path-reachability.md @@ -0,0 +1,11 @@ +--- +"@objectstack/plugin-audit": patch +--- + +**Docs (published README) + ruling:** record-view auditing now documents how to turn it on under `objectstack serve`, and the answer to "should `os serve` grow an `appAuditPluginOptions(config)` helper?" is **no** (#9863). + +The README and `content/docs/permissions/record-view-auditing.mdx` both said the audited set is configured "where you compose the kernel", and the docs page went further: *"The CLI's `os serve` registers `AuditPlugin` with no options, so a stack served that way has record-view auditing off and no knob to turn it on."* That last clause stopped being true when #9864 declared and pinned the duplicate-registration contract. The knob is the stack's `plugins` array — a configured `new AuditPlugin({ readAudit: { objects: [...] } })` there supersedes the CLI's option-less instance by name, last-one-wins, on both kernels, with the displaced instance never reaching `init()`. Both pages now spell that path, and name the `Plugin superseded: 'com.objectstack.audit'` boot line as the opt-in working rather than a misconfiguration. + +**No new configuration surface was added, deliberately.** A `config.audit` key read by an `appAuditPluginOptions(config)` helper would reproduce, in `objectstack.config.ts`, exactly the failure #8992's ruling refused for the object-metadata spelling: a declaration that survives in a deployment which never installs this package, reading as coverage while recording nothing. It would also be a *second* configuration surface that silently loses to the first, since an app's own `plugins` entry supersedes whatever the CLI constructed. The `#7001` symmetry argument does not carry it either — `@objectstack/verify`'s `bootStack` constructs no `AuditPlugin` and does not depend on this package, so there is no second boot path to disagree with. + +No runtime behaviour changed. `packages/cli` gains only the reasoning at its registration site and `serve-audit-registration.contract.test.ts`, which pins the three facts the ruling rests on — including the load-bearing ordering (`AuditPlugin` registered above the stack `plugins` loop) that until now was asserted by a comment and nothing else. diff --git a/content/docs/permissions/record-view-auditing.mdx b/content/docs/permissions/record-view-auditing.mdx index fd4f4c6e99..3fd36bb1f1 100644 --- a/content/docs/permissions/record-view-auditing.mdx +++ b/content/docs/permissions/record-view-auditing.mdx @@ -68,9 +68,60 @@ that never installs this plugin, producing metadata that *reads* as audited and records nothing — and on a compliance surface, a declaration a reviewer mistakes for coverage is worse than an absent feature. -The practical consequence is that this is configured **where you compose the -kernel**. The CLI's `os serve` registers `AuditPlugin` with no options, so a -stack served that way has record-view auditing off and no knob to turn it on. +The practical consequence is that this is configured **where the plugin is +installed**. On the `os serve` boot path that place is your stack's `plugins` +array — see [Under `os serve`](#under-os-serve) below. + + +### Under `os serve` + +`os serve` may auto-register `AuditPlugin` for you, and when it does it passes +**no options** — that instance audits no views. To turn record-view auditing on, +put your own configured instance in the stack's `plugins` array: + +```typescript +// objectstack.config.ts +import { defineStack } from '@objectstack/spec'; +import { AuditPlugin } from '@objectstack/plugin-audit'; + +export default defineStack({ + manifest: { name: 'my-app', version: '1.0.0' }, + plugins: [ + new AuditPlugin({ readAudit: { objects: ['contact', 'account'] } }), + ], + // objects, apps, views, … +}); +``` + +That is a declared contract, not a lucky ordering. The CLI registers its +option-less instance **before** it walks `plugins`, and registering a plugin +whose `name` is already taken **overwrites** the earlier registration — +last-one-wins, identically on both kernels, with a `warn` naming both versions. +Your configured instance is the one that boots; the CLI's is discarded before it +ever reaches `init()`, so nothing is started twice and nothing leaks. A line like + +```text +Plugin superseded: 'com.objectstack.audit' — the later registration (v1.0.0) +REPLACED the earlier one (v1.0.0). Only the later instance is initialized and +started; the earlier one is discarded without ever running init(). +``` + +in your boot log **is the opt-in working**, not a misconfiguration. (You will +not always see it: the CLI's auto-registration is paired with its `AuthPlugin` +bootstrap, so a stack that supplies its own `AuthPlugin` never gets a second +audit instance to supersede in the first place.) + + +**There is no stack-config key either.** No `audit:` block in +`objectstack.config.ts` names the audited objects, deliberately, and for exactly +the reason there is no object-metadata key: such a key would survive in a +deployment that never installs `@objectstack/plugin-audit` at all, reading as +coverage while recording nothing. + +`requires: ['audit']` is not a substitute — it makes the plugin's presence a +hard boot requirement, but it constructs the plugin with no options and offers +nowhere to name objects. Naming audited objects means constructing the plugin +yourself, as above. ## What counts as a record view diff --git a/packages/cli/src/commands/serve-audit-registration.contract.test.ts b/packages/cli/src/commands/serve-audit-registration.contract.test.ts new file mode 100644 index 0000000000..3d8e4f8e87 --- /dev/null +++ b/packages/cli/src/commands/serve-audit-registration.contract.test.ts @@ -0,0 +1,240 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// `objectstack serve`'s AuditPlugin registration — the facts the #9863 ruling +// rests on, mechanised. +// +// ## The question, and the answer +// +// #9863 asked whether `os serve` should grow an `appAuditPluginOptions(config)` +// helper mirroring the `appSecurityPluginOptions(config)` sibling six lines +// above it, so that record-view auditing (`AuditPluginOptions.readAudit`) could +// be turned on from `objectstack.config.ts` — rather than only by an app +// putting its OWN configured `new AuditPlugin({ readAudit: … })` in the stack's +// `plugins` array, where it supersedes the CLI's option-less instance under the +// declared last-one-wins registration contract (#9864, maintainer ruling +// 2026-08-19, option B). +// +// Ruled NO, on four measurements — the reasoning lives at the registration site +// in `serve.ts` and in #9863's ruling comment; what lives HERE is the part that +// has to keep being true: +// +// 1. The CLI constructs `AuditPlugin` exactly once, with NO options. That is +// the ruling itself. Re-opening it means editing this file deliberately, +// not discovering later that the shape drifted. +// 2. That construction sits inside the auth-gated pair block and ABOVE the +// stack `plugins` loop. The ORDER is load-bearing and was, until this file, +// asserted by nothing: invert it and the CLI's option-less instance +// supersedes the app's configured one, silently turning record-view +// auditing back OFF for every deployment that had opted in. Nothing else +// in the repo goes red on that edit. +// 3. `@objectstack/verify`'s `bootStack` constructs no `AuditPlugin` at all. +// This is why the #7001 argument for the security helper does not transfer: +// that helper exists because TWO boot paths both built a `SecurityPlugin` +// and silently disagreed about its options. Audit has exactly one boot path +// with an opinion, so there is no disagreement for a shared helper to close +// — and `@objectstack/verify` does not even depend on +// `@objectstack/plugin-audit` (see its package.json), so it cannot grow one +// by accident. If that changes, the ruling's basis changes with it, and +// this assertion is what says so. +// +// ## Why a source scan rather than a boot +// +// Same reason as this directory's `serve-verify-security-parity.contract.test.ts` +// and `serve-email-config-parity.contract.test.ts`: the failure mode is an EDIT +// to these files, and every one of the three facts above is invisible to a +// behavioural test. `serve.ts`'s audit block is reachable only from a live +// `objectstack serve` boot with `@objectstack/plugin-auth` and +// `@objectstack/plugin-audit` installed, an auth secret set and no app-supplied +// AuthPlugin; a unit test that got there would be testing the fixture. The grep +// that WOULD have caught each edit, mechanised, is the honest instrument. + +import { describe, it, expect } from 'vitest'; +import { readFileSync } from 'node:fs'; +import path from 'node:path'; +import { fileURLToPath } from 'node:url'; +// The separator (#9367) is a plain `.mjs`, but it ships a hand-written `.d.mts` +// declaration alongside it (#10398), so this import is typed and needs no +// suppression. A `@ts-expect-error` here is an UNUSED directive, and cli's tsc +// program does include this file, so tsc fails the build on one. +import { maskComments } from '../../../../scripts/js-comment-mask.mjs'; + +const HERE = path.dirname(fileURLToPath(import.meta.url)); + +/** + * `packages/cli/src/commands/` → `packages/`. Reading verify's harness from + * here is what makes fact 3 an assertion instead of a comment; + * `@objectstack/verify` is a real dependency of this package and the read is + * test-only (tests never ship — `files: ["dist"]`). The glob is already declared + * for `@objectstack/cli` by `pnpm check:cross-package-test-inputs`, and hashed + * by `@objectstack/cli#test` in turbo.json, for the sibling parity scan; this + * file adds a second reader of the same path, not a new radius. + * + * That gate is named by its runnable script rather than by its file path on + * purpose. Its literal collector takes any quoted path without parsing, so + * spelling the path here — in prose, about a file this test never opens — would + * demand a declaration for it and put cli's whole suite on every edit of the + * gate. The reads this scan really makes are the two below. + */ +const PACKAGES_DIR = path.resolve(HERE, '../../..'); + +/** + * Absence must be loud (AGENTS.md, Route & surface ownership §3). A scan that + * reports success because it could not find its subject is worse than no scan: + * every assertion below is of the form "this shape is present / is not present", + * and an empty string satisfies half of them for free. + */ +function readBootPath(relative: string): string { + const full = path.join(PACKAGES_DIR, relative); + try { + return readFileSync(full, 'utf8'); + } catch (e) { + throw new Error( + `#9863 audit-registration scan cannot read its subject '${relative}' (looked at ${full}). ` + + 'The file moved or was renamed — repoint this scan; do NOT delete it. The ruling it pins ' + + `is still in force. (${(e as Error).message})`, + ); + } +} + +/** + * Comments MASKED, because this scan is about what the two files DO. + * + * Not optional here, and not caution copied from the sibling scans: `serve.ts`'s + * audit block DESCRIBES the very construction being counted — it spells + * `new AuditPlugin({ readAudit: … })` in prose to explain the app-side opt-in it + * documents, and it names the helper by name to record that the helper was ruled + * against. Over raw text this file would count two constructions where the code + * has one, and its own ruling assertion would fail on the sentence stating the + * ruling. + * + * ## Why the SHARED masker rather than a private `stripComments` + * + * Because the private ones are a measured defect class here (#9367), and this + * file walked straight into it. Its first draft used the two-regex strip the two + * older scans in this directory still carry, block pass first — and `serve.ts` + * has a route wildcard in a line comment (`/api/v1/auth/*` in the `5d.` header). + * A regex cannot tell that `/*` from a real opener, so it ran a phantom block + * comment to the next real terminator ten lines below, inside + * `import(/* webpackIgnore: true *\/ …)`, deleting the `hasAuthPlugin` + * computation and the auth gate this file measures against. Measured on this + * exact pair: the naive strip keeps 1895 code-bearing lines of `serve.ts`, the + * masker keeps 2098. + * + * `maskComments` also BLANKS rather than deletes — spans become spaces, newlines + * kept — so the offsets the ordering assertion below compares are offsets into + * the real file rather than into a shrunken copy of it. Cross-checked on both + * subjects: masker and naive-strip agree on all four anchor counts, and the + * masker leaves the line count identical (4638 → 4638), which is the property + * being bought. + * + * #10427 (open) has the masker desyncing on nested template literals in 16 + * files. Neither file read here is among them, and the cross-check above is what + * says so rather than assuming it. + */ + +const SERVE = maskComments(readBootPath('cli/src/commands/serve.ts')); +const HARNESS = maskComments(readBootPath('verify/src/harness.ts')); + +/** + * Every `new AuditPlugin(...)` construction in a file, with its argument text. + * + * Walks parentheses rather than matching `\(([^)]*)\)`, for the reason the + * sibling parity scan measured: an options argument is itself brace- and + * paren-bearing (`appAuditPluginOptions(config)`, `{ readAudit: { objects: [] } }`), + * and a non-nesting match stops at the first inner `)` and silently reports a + * truncation. The empty-argument case this file asserts today would be reported + * identically by both forms, which is exactly how a scan that cannot read the + * shape it guards passes until the day it matters. + */ +function auditPluginConstructions(source: string): string[] { + const NEW = 'new AuditPlugin('; + const found: string[] = []; + for (let i = source.indexOf(NEW); i !== -1; i = source.indexOf(NEW, i + 1)) { + let depth = 1; + let j = i + NEW.length; + for (; j < source.length && depth > 0; j++) { + if (source[j] === '(') depth++; + else if (source[j] === ')') depth--; + } + if (depth !== 0) throw new Error(`unbalanced \`${NEW}…\` at offset ${i} — the scan cannot read this file`); + found.push(source.slice(i + NEW.length, j - 1).trim()); + } + return found; +} + +/** + * The offset of an anchor that must appear exactly once. Both "missing" and + * "appeared twice" are reported as failures rather than folded into an offset + * comparison, because an ordering assertion between two anchors is meaningless + * if either is ambiguous — and a duplicated anchor is how a refactor most + * plausibly arrives. + */ +function soleOffset(source: string, anchor: string, role: string): number { + const first = source.indexOf(anchor); + if (first === -1) { + throw new Error( + `#9863 audit-registration scan: the ${role} anchor \`${anchor}\` is gone from serve.ts. ` + + 'It was the landmark this scan measured the AuditPlugin registration against. ' + + 'Repoint the anchor at whatever replaced it — the invariant (the CLI registration ' + + 'stays inside the auth-gated pair block and ABOVE the stack `plugins` loop) is unchanged.', + ); + } + if (source.indexOf(anchor, first + 1) !== -1) { + throw new Error( + `#9863 audit-registration scan: the ${role} anchor \`${anchor}\` now appears more than once ` + + 'in serve.ts, so "before" and "after" no longer name one place. Give this scan an ' + + 'unambiguous landmark before trusting its verdict.', + ); + } + return first; +} + +/** The `if (!hasAuthPlugin && tierEnabled('auth'))` block the audit pair lives in. */ +const AUTH_GATE = "if (!hasAuthPlugin && tierEnabled('auth'))"; +/** The stack `plugins` loop, i.e. where an app's own configured instance is registered. */ +const PLUGINS_LOOP = 'for (const plugin of plugins)'; + +describe('os serve registers AuditPlugin bare, above the stack `plugins` loop (#9863)', () => { + it('constructs AuditPlugin exactly once, with NO options — the ruling', () => { + // The empty string is the whole point: `[]` would mean "never constructed" + // and `['appAuditPluginOptions(config)']` would mean the ruling was reversed. + expect(auditPluginConstructions(SERVE)).toEqual(['']); + }); + + it('does not reach for a config-derived audit options helper', () => { + // Re-opening #9863 is allowed; doing it by accident is not. A helper wired + // in HERE would take effect only when the app supplies no AuthPlugin of its + // own and an auth secret is set (see the gate below) — a declared config key + // whose effect depends on unrelated auth conditions, on a compliance + // surface. If the ruling is revisited, the capability resolver's + // `CAPABILITY_PROVIDERS.audit` entry — which is NOT auth-gated and already + // carries the `configKey` mechanism `analytics` uses — is the site to argue + // about, and this assertion moves in the same edit as the ruling. + expect(SERVE).not.toContain('appAuditPluginOptions'); + }); + + it('registers inside the auth-gated pair block and ABOVE the stack `plugins` loop', () => { + const gate = soleOffset(SERVE, AUTH_GATE, 'auth-gate'); + const loop = soleOffset(SERVE, PLUGINS_LOOP, 'stack-plugins-loop'); + const audit = soleOffset(SERVE, 'new AuditPlugin(', 'audit-construction'); + + // ABOVE the loop: the half `serve.ts` calls load-bearing. Below it, the + // CLI's option-less instance would supersede the app's configured one and + // record-view auditing would be off wherever it had been opted in. + expect(audit).toBeLessThan(loop); + + // INSIDE the auth block: the half nothing had written down. The pair is + // registered by the `5d. Auto-register AuthPlugin (and paired + // Security/Audit)` branch, so an app that supplies its own AuthPlugin — or + // a production boot with no auth secret — gets NO CLI AuditPlugin, and the + // supersede this card is named after never happens there at all. Hoisting + // the registration out of the block is a real change of meaning, not a + // tidy-up, and it is exactly the edit an offset comparison against the gate + // catches. + expect(audit).toBeGreaterThan(gate); + }); + + it("verify's bootStack has no AuditPlugin opinion — no #7001-shaped disagreement to close", () => { + expect(auditPluginConstructions(HARNESS)).toEqual([]); + }); +}); diff --git a/packages/cli/src/commands/serve.ts b/packages/cli/src/commands/serve.ts index cca735b3c2..9b8c0863c1 100644 --- a/packages/cli/src/commands/serve.ts +++ b/packages/cli/src/commands/serve.ts @@ -2525,10 +2525,58 @@ export default class Serve extends Command { // ⚠️ The dependency is on the ORDER as much as on the overwrite: // this registration must stay ABOVE the stack's `plugins` loop, or // the CLI's option-less instance would supersede the app's - // configured one instead. #9863 remains open on its own question — - // whether `os serve` should grow an `appAuditPluginOptions(config)` - // helper like its `SecurityPlugin` sibling above, rather than - // reaching the capability only through a supersede. + // configured one instead — silently turning record-view auditing + // back OFF for every deployment that had opted in. That order is no + // longer held by this comment alone: + // `serve-audit-registration.contract.test.ts` fails on the + // inversion, and on the two facts below. + // + // ⚠️ It is also AUTH-GATED, which nothing had written down. This + // pair is registered by the `5d. Auto-register AuthPlugin (and + // paired Security/Audit)` branch, so an app that supplies its own + // AuthPlugin — or a production boot with no auth secret, or a host + // kernel — never reaches this line at all. There the app's + // `plugins` entry is the ONLY AuditPlugin and no supersede happens. + // The supersede is how the opt-in survives one particular boot + // shape; it is not the mechanism the opt-in is built on. + // + // [#9863, ruled 2026-08-20] NO `appAuditPluginOptions(config)`. + // The open question was whether to mirror the `SecurityPlugin` + // helper six lines up. Four measurements say no: + // + // • #7001's REASON DOES NOT TRANSFER. That helper exists so two + // boot paths could not disagree about one plugin's options. + // `@objectstack/verify`'s `bootStack` constructs no AuditPlugin + // at all and does not depend on `@objectstack/plugin-audit`, so + // audit has exactly one boot path with an opinion and there is + // no disagreement for a shared helper to close. + // • IT WOULD HAVE NOTHING TO READ. `appSecurityPluginOptions` + // derives from `config.permissions`, an already-declared spec + // surface. There is no `audit` key in the stack schema and no + // object-metadata audit field, so an audit helper means minting + // an authorable surface, not reading one. + // • THAT SURFACE IS THE SHAPE #8992 ALREADY REFUSED for the + // object-metadata spelling (maintainer ruling 2026-08-16): a + // declaration that survives in a deployment which never + // installs the plugin — the import below is best-effort — i.e. + // config that READS as audited and records nothing. On a + // compliance surface that is worse than an absent feature. + // • IT WOULD BE A SECOND SURFACE THAT SILENTLY LOSES TO THE + // FIRST. An app setting both would have its config-derived + // options superseded by its own `plugins` entry, by the very + // contract above — a new footgun on the same capability. + // + // Measured pull for the surface: zero `readAudit` call sites in the + // repo outside `plugin-audit` itself. The reachability story is + // instead DOCUMENTED, now that #9864 made it a contract rather than + // the accident #9863 found: `content/docs/permissions/ + // record-view-auditing.mdx` and the plugin README both spell the + // `plugins`-array opt-in and the `Plugin superseded:` line it logs. + // + // If the ruling is ever revisited, the site to argue about is + // `Serve.CAPABILITY_PROVIDERS.audit` — the `requires:` resolver is + // NOT auth-gated and already carries the `configKey` mechanism + // `analytics` uses — not this line. try { const auditPkg = '@objectstack/plugin-audit'; const { AuditPlugin } = await import(/* webpackIgnore: true */ auditPkg); diff --git a/packages/plugins/plugin-audit/README.md b/packages/plugins/plugin-audit/README.md index a8fe565207..2e5aa3cc36 100644 --- a/packages/plugins/plugin-audit/README.md +++ b/packages/plugins/plugin-audit/README.md @@ -179,6 +179,33 @@ claim. - an empty (or fully excluded) set registers **no hook at all**, so a deployment that opts nothing in pays nothing on its read path. +### Turning it on under `os serve` + +"The place the plugin is installed" is a kernel you compose yourself in most of this +README. On the `objectstack serve` boot path it is your stack's `plugins` array: + +```typescript +import { defineStack } from '@objectstack/spec'; +import { AuditPlugin } from '@objectstack/plugin-audit'; + +const stack = defineStack({ + plugins: [new AuditPlugin({ readAudit: { objects: ['contact', 'account'] } })], +}); +``` + +The CLI may auto-register an option-less `AuditPlugin` of its own earlier in the same boot. +When it has, yours **supersedes** it: registering a plugin whose `name` is already taken +overwrites the earlier registration — last-one-wins, on both `ObjectKernel` and +`LiteKernel`, with a `warn` naming both versions, and the displaced instance never reaches +`init()`. That is a declared contract (`packages/core/src/plugin-registration.ts`), not an +ordering accident, so a `Plugin superseded: 'com.objectstack.audit'` line in the boot log +is this opt-in working. + +⛔ There is no stack-config key for the audited set, for the same reason there is no +object-metadata key: a key in `objectstack.config.ts` survives in a deployment that never +installs this package, and would read as coverage while recording nothing. `requires: +['audit']` loads the plugin but constructs it with no options — it cannot name objects. + ### Only record-detail views produce a row A read is recorded when **both** hold: diff --git a/scripts/check-cross-package-test-inputs.mjs b/scripts/check-cross-package-test-inputs.mjs index d2dd593bec..ea33a5bd1e 100644 --- a/scripts/check-cross-package-test-inputs.mjs +++ b/scripts/check-cross-package-test-inputs.mjs @@ -244,16 +244,18 @@ const CROSS_PACKAGE_TEST_INPUTS = { // tell prose from code, or than rewording a comment to dodge a scanner. // // `js-comment-mask.mjs` is the first entry declared for an IMPORT rather than - // a file read: src/commands/serve-verify-security-parity.contract.test.ts - // imports `maskComments` from it to separate code from prose in the two boot - // paths it scans (#10453, adopting #9367's conversion). This gate did NOT - // demand the declaration -- its literal collector recognises path-shaped - // reads, and a relative import specifier that escapes the package is not one - // of the spellings it knows. Declared by hand because the coupling is real - // whatever the collector saw: that scan's verdict is a function of this - // module's masking behaviour, so a change to it has to re-run cli's suite. - // The undetected-import spelling is filed separately as #10452; widening a - // radius by hand is never the reason not to file it. + // a file read, and it now has TWO importers: + // src/commands/serve-verify-security-parity.contract.test.ts (#10453, + // adopting #9367's conversion) and + // src/commands/serve-audit-registration.contract.test.ts (#9863) both import + // `maskComments` from it to separate code from prose in the boot paths they + // scan. This gate did NOT demand the declaration -- its literal collector + // recognises path-shaped reads, and a relative import specifier that escapes + // the package is not one of the spellings it knows. Declared by hand because + // the coupling is real whatever the collector saw: those scans' verdicts are + // a function of this module's masking behaviour, so a change to it has to + // re-run cli's suite. The undetected-import spelling is filed separately as + // #10452; widening a radius by hand is never the reason not to file it. // // Its `.d.mts` sibling is declared for BOTH reasons this roster records. It // is named in that test's prose, and the literal collector takes quoted