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

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

Both capability gates registered on beforeInsert only, so a re-point via
UPDATE landed rows on a parent object whose declaration refuses them: a
caller who could not create a sys_attachment on a files:false object could
move an existing one onto it, and a sys_comment could be re-threaded into a
feeds-disabled object's thread. The access kits authorize the re-point
(comment-access-hooks since #4630, attachment-access-hooks since #10091) —
those are access checks; the capability half was never asked.

enable.files/enable.feeds are properties of the TARGET object, so the verb
that made a row target it does not change the answer. Register the same two
enforcement functions on beforeUpdate. Both already read only input.data, so
an absent parent_object / thread_id means "not a re-point" and returns on
the first line — a rename or body edit on a grandfathered row is untouched.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx

* test(plugin-audit): pin the update-verb capability gates; re-point the #6656 no-global-pre-image assertion

The #6656 case asserted "no beforeUpdate/beforeDelete registration" by
filtering on EVENT NAME. Its own comment recorded that as a proxy: the
property it pins is the retirement of captureBefore, an UNSCOPED pre-image
reader that made hasHooksFor true system-wide and bought a prior-row read on
every update in the stack. It carved the capability gates out by noting they
are "on a single named object each" and "read no prior row" — true of the new
beforeUpdate registrations too, which the event-name filter could not express.

Filter on GLOBAL registrations instead, and add the affirmative half: each
gate is declared on beforeInsert AND beforeUpdate, scoped to one object. The
demand gate costs nothing new on these two objects — comment-access-hooks
(#4630) and attachment-access-hooks (#10091) already declare beforeUpdate
scoped to sys_comment / sys_attachment.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx

* test(plugin-audit): type the engine query/update options instead of erasing them

check:query-options-erasure counts erased option bags in test code against a
shrink-only ceiling; the new file pushed the test surface 240 -> 246. These
inputs are all ON-contract (`where`, `multi`), so the gate's own remedy is to
type them, not to cast — the `as unknown as` escape names a contract being
bypassed deliberately, which is not what these do. Back to 240, at the
ceiling, none new.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01PnJHU45vPJj5UQrxe946Bx

---------

Co-authored-by: Claude <noreply@anthropic.com>

Author: os-warren

.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