fix(plugin-audit): ask enable.files/enable.feeds on the update verb too

.changeset/capability-gate-update-verb.md

@@ -0,0 +1,19 @@
+---
+"@objectstack/plugin-audit": patch
+---
+
+**Behaviour change (tightening):** `enable.files` / `enable.feeds` are now enforced on the **update** verb, not only on insert (#10170).
+
+Both capability gates in `audit-writers.ts` registered on `beforeInsert` only. `enable.files` says whether `sys_attachment` rows may **target** an object and `enable.feeds` whether `sys_comment` rows may target it — properties of the target object, not of the verb that got a row there — so a re-point via update landed rows the declaration refuses: a caller who could not *create* an attachment on an object without `enable.files: true` could *move* an existing one onto it, and a comment could be re-threaded into a `feeds: false` object's thread. The access kits authorize the re-point (`comment-access-hooks.ts` since #4630, `attachment-access-hooks.ts` since #10091), but those are **access** checks — the capability half was never asked on update.
+
+What an operator will now observe:
+
+- An update of `sys_attachment` whose payload sets `parent_object` to an object that does not declare `enable: { files: true }` is refused with **403 `FILES_DISABLED`** — the same envelope the insert path has emitted since #2727 (ADR-0112: `code` + `status`). Fail-closed as on insert: an absent `enable` block, an absent flag, and an unknown parent object all reject.
+- An update of `sys_comment` whose payload sets `thread_id` to a thread on an object declaring `enable: { feeds: false }` is refused with **403 `FEEDS_DISABLED`**. Opt-out semantics as on insert: only an explicit `false` rejects, and a missing or free-form `thread_id` is still allowed through — this is capability gating, not access control.
+- Both apply on **both dispatch shapes**: a by-id update (`dispatch.mode` `record`) and a predicate `multi: true` update, which is evaluated per matched row (#5574 / ADR-0058 Addendum II). An unscoped predicate update is refused on its first matched row.
+
+**No existing row is newly refused, and no update that is not a re-point changes.** The gates read the payload: an update that never names `parent_object` / `thread_id` returns on the gate's first line, so renames, body edits, reaction writes and other column updates on a row whose parent object has since had the capability flipped off keep working exactly as before. Only a write that makes a row *newly target* a walled object is refused.
+
+**Blast radius.** A structural sweep of the 4 660 in-tree source files found **no** caller — none in `packages/` source, `examples/`, or the dogfood apps — that issues an update whose payload names `parent_object`, and none that re-points `thread_id`; in the console the only `sys_attachment` write is a create, and the only `sys_comment` update writes `reactions`. If you have your own "move this attachment" or "move this comment" flow, point it at a target object that declares the capability, or declare it on the target.
+
+No new error code: both codes are existing standard-catalog members already registered in `packages/spec/src/api/error-code-ledger.zod.ts` and already mapped to 403 by `packages/rest/src/error-response.ts`.

packages/plugins/plugin-audit/src/audit-hook-object-scope.test.ts

@@ -437,24 +437,68 @@ function makeRecordingEngine() {
 const AUDIT_WRITER_EVENTS = ['afterInsert', 'afterUpdate', 'afterDelete'];
 
 describe('[#5860] the skip list is declared on the registration face', () => {
- it('plugin-audit declares NO `beforeUpdate` / `beforeDelete` hook (#6656)', () => {
+ it('plugin-audit declares NO GLOBAL `beforeUpdate` / `beforeDelete` hook (#6656)', () => {
  const { engine, registrations } = makeRecordingEngine();
  installAuditWriters(engine);
 
  // The retirement, asserted on the declaration itself rather than inferred
  // from a read count — this is the face `hasHooksFor` reads, so it is what
  // decides whether the engine's per-row bulk dispatch runs at all.
  //
- // Scoped to the two events `captureBefore` held. The plugin's OTHER
- // before-phase registrations are unrelated capability gates on a single
- // named object each (`beforeInsert` on `sys_comment` for `enable.feeds`,
- // on `sys_attachment` for `enable.files`); they read no prior row, and
- // asserting "no before-phase hook at all" would fail on them while
- // measuring nothing about this card.
- const preImageEvents = registrations
- .map((r) => r.event)
- .filter((e) => e === 'beforeUpdate' || e === 'beforeDelete');
- expect(preImageEvents).toEqual([]);
+ // [#10170] The filter is on GLOBAL registrations, not on the event names.
+ // It used to be on the event names, and the case above already recorded
+ // why that was only ever a PROXY: the plugin's capability gates are
+ // "unrelated … on a single named object each", they "read no prior row",
+ // and an assertion that caught them "would fail on them while measuring
+ // nothing about this card". While those gates were insert-only, filtering
+ // by event name expressed that carve-out exactly. #10170 registers them on
+ // `beforeUpdate` too — `enable.files`/`enable.feeds` are properties of the
+ // TARGET object, so a re-point via update is inside the declaration — and
+ // the proxy stopped tracking the property.
+ //
+ // What #6656 retired was `captureBefore`: an UNSCOPED pre-image reader
+ // that made `hasHooksFor(<any object>, 'beforeUpdate')` true system-wide
+ // and bought a prior-row read on every update in the stack. That is the
+ // invariant, and it is what this now asserts. An object-SCOPED gate costs
+ // the demand gate nothing beyond its own object — and on these two
+ // objects nothing at all: `comment-access-hooks.ts` (#4630) and
+ // service-storage's `attachment-access-hooks.ts` (#10091) already declare
+ // `beforeUpdate` scoped to `sys_comment` / `sys_attachment`, so
+ // `hasHooksFor` is already true for both wherever the access kits install.
+ const globalPreImage = registrations
+ .filter((r) => r.event === 'beforeUpdate' || r.event === 'beforeDelete')
+ .filter((r) => r.options?.object === undefined)
+ .map((r) => r.event);
+ expect(globalPreImage).toEqual([]);
+ });
+
+ it('[#10170] the two capability gates are declared on insert AND update, each scoped to one object', () => {
+ // The other half of the case above: the reason a `beforeUpdate`
+ // registration is admissible here is that it names ONE object. Assert that
+ // rather than leaving it to the negative filter — a future gate that
+ // forgot its `object` scope would otherwise only be caught by the absence
+ // test above, which reads as "nothing was retired", not "a gate went
+ // global".
+ const { engine, registrations } = makeRecordingEngine();
+ installAuditWriters(engine);
+
+ // BEFORE-phase only: `sys_comment` also carries the M10.8 @mention
+ // notification hook on `afterInsert`, which is not a capability gate.
+ const gateEvents = (object: string) =>
+ registrations
+ .filter((r) => r.options?.object === object && r.event.startsWith('before'))
+ .map((r) => r.event)
+ .sort();
+
+ expect(gateEvents('sys_comment')).toEqual(['beforeInsert', 'beforeUpdate']);
+ expect(gateEvents('sys_attachment')).toEqual(['beforeInsert', 'beforeUpdate']);
+
+ // …and neither of them widened into the global allow half.
+ for (const r of registrations) {
+ if (r.options?.object === 'sys_comment' || r.options?.object === 'sys_attachment') {
+ expect(r.options?.excludeObjects).toBeUndefined();
+ }
+ }
  });
 
  it('all writer registrations carry `excludeObjects` and stay global otherwise', () => {

packages/plugins/plugin-audit/src/audit-writers.ts

@@ -1567,6 +1567,21 @@ export function installAuditWriters(
  * unconventional thread_id is allowed through: this is capability
  * gating, not access control, and free-form threads have no object to
  * gate on.
+ *
+ * [#10170] Registered on `beforeUpdate` as well as `beforeInsert`, because
+ * the flag is a property of the TARGET OBJECT — "does this object allow
+ * comments at all" — and not of the verb that made a row target it. On
+ * insert only, a caller who could not *create* a comment on a
+ * `feeds: false` object could *re-thread* an existing one into it, and the
+ * row landed. `comment-access-hooks.ts` authorizes that re-point (#4630:
+ * the new `thread_id`'s parent must be readable) — but that is an ACCESS
+ * check; the capability half was never asked on the update verb.
+ *
+ * On update, an ABSENT `thread_id` means "not a re-thread", and the same
+ * first line returns. That is what keeps an ordinary body/reaction edit on
+ * an existing row working after its object's `enable.feeds` is flipped off:
+ * the narrowing reaches re-points, not every later write to a grandfathered
+ * row.
  */
  const enforceFeedsCapability = async (ctx: HookContext) => {
  const data: any = (ctx.input as any)?.data;
@@ -1585,6 +1600,7 @@ export function installAuditWriters(
  }
  };
  engine.registerHook('beforeInsert', enforceFeedsCapability, { object: 'sys_comment', packageId });
+ engine.registerHook('beforeUpdate', enforceFeedsCapability, { object: 'sys_comment', packageId });
 
  /**
  * `enable.files` server-side enforcement (#2727). The generic Attachments
@@ -1600,11 +1616,26 @@ export function installAuditWriters(
  * store the file URL in the record's own column via service-storage and
  * never create a sys_attachment row, so field-level attachments keep
  * working regardless of this flag.
+ *
+ * [#10170] Registered on `beforeUpdate` as well, for the feeds gate's
+ * reason one object over: `enable.files` says whether attachments may
+ * TARGET this object, so a re-point that makes a row target it is inside
+ * the declaration whether or not a creation happened. On insert only, a
+ * caller barred from *creating* an attachment on a `files: false` object
+ * could *move* an existing one onto it. `attachment-access-hooks.ts`
+ * authorizes the re-point (#10091: the new `parent_object`/`parent_id`
+ * must be editable) — access, again, not capability.
  */
  const enforceFilesCapability = async (ctx: HookContext) => {
  const data: any = (ctx.input as any)?.data;
  const parentObject = data?.parent_object;
- if (typeof parentObject !== 'string' || parentObject.length === 0) return; // schema requires it; let validation report the miss
+ // Two meanings, one line. On INSERT an absent `parent_object` is a
+ // schema violation — left to validation to report, so the gate never
+ // shadows the real diagnostic. On UPDATE (#10170) it means "this write is
+ // not a re-point", so there is no new target to ask about and the row's
+ // existing parent was already gated when it was created. Either way the
+ // gate has nothing to say.
+ if (typeof parentObject !== 'string' || parentObject.length === 0) return;
  const def = getObjectDef(parentObject);
  if (def?.enable?.files !== true) {
  const err: any = new Error(`File attachments are not enabled for object '${parentObject}' (requires enable.files: true)`);
@@ -1615,6 +1646,29 @@ export function installAuditWriters(
  }
  };
  engine.registerHook('beforeInsert', enforceFilesCapability, { object: 'sys_attachment', packageId });
+ engine.registerHook('beforeUpdate', enforceFilesCapability, { object: 'sys_attachment', packageId });
+
+ /*
+ * [#10170] Why neither `beforeUpdate` registration above declares
+ * `dispatchUnscopedMultiWrite` (#9719, widened to `beforeUpdate` by #9974).
+ *
+ * That flag buys ONE extra dispatch, with the whole-operation context and
+ * before any matched row is resolved, for guards that refuse an operation
+ * SHAPE — "a `multi: true` update with no `where` at all". These two are not
+ * shape guards: they read the PAYLOAD, which the per-row fan-out delivers
+ * verbatim to every matched row (#5574 / ADR-0058 Addendum II D1–D2 builds a
+ * fresh context per row per phase, carrying the same payload object). So an
+ * unscoped multi update that re-points onto a walled parent is already
+ * refused on the first matched row, without the flag — pinned in
+ * `capability-gate-update-verb.test.ts`.
+ *
+ * Declaring it would narrow further than the declaration justifies: the one
+ * case it would ADD is a ZERO-MATCH unscoped write, where nothing is written
+ * and therefore nothing ever comes to target the walled object. Refusing
+ * that is an operation-shape policy — the #4757 `sys_attachment` and #4630
+ * `sys_comment` guards' territory, declared on their own registrations — not
+ * the capability opt-in this card restores.
+ */
 
  /**
  * M10.8: Dedicated hook on `sys_comment` afterInsert that parses the