feat(service-storage): gate sys_attachment beforeUpdate with the uploader-or-parent-editor rule

[os-zhuang]

Fixes#10091

First deliverable — the judgement: omission, not a decision

Answered from origin/main@2d3860d before any edit. The absent beforeUpdate is a genuine omission. Every layer that could have carried a deliberate "attachments are not updatable" contract was checked and carries none:

• Object definition (packages/platform-objects/src/audit/sys-attachment.object.ts): apiEnabled: true, no immutability posture — only id and created_at are readonly; parent_object, parent_id, file_id, uploaded_by, file_name, mime_type, size, description are all writable.
• No alternate mutation seam: sys_attachment rows are written through the generic data path (the module's own header says so); storage-routes.ts touches the object only in a comment; no storage-service API owns updates.
• file-reference-lifecycle.ts's beforeUpdate is copy-on-claim mechanics on file-class fields, global and not row authorization — and sys_attachment.file_id is a plain lookup, not a file-class field, so it does not even apply here.
• attachment-lifecycle.ts registers best-effort tombstone bookkeeping only (beforeDelete/afterDelete/afterInsert), documented to never block a write.
• plugin-security / plugin-sharing have no sys_attachment-specific update handling; the default member sets grant wildcard CRUD with no row scoping (both kits' headers state this as the threat model).

The decisive structural point: the ungated verb nullifies the gated ones. With no update hook, a client-supplied uploaded_by wins on update (the server-stamp exists only in beforeInsert) — a member who cannot delete another user's attachment could rewrite uploaded_by to themselves and walk through the delete gate's uploader shortcut; a member who cannot attach to a record could re-point an existing row's parent_id at it. A deliberate immutability posture would be declared, not left as the one verb that defeats the other two. And the sys_comment kit — explicitly derived from this one — has gated update with a stated rationale since #4630.

What changed

One file of production code, packages/services/service-storage/src/attachment-access-hooks.ts:

• New beforeUpdate registration — the card's own suggested shape:
  • Row rule: uploader OR parent editor (canEdit, degrading to caller-scoped parent READ visibility without a sharing service), every matched row must pass, resolve capped at 1 000 rows (fail-closed).
  • Re-point rule: an update changing parent_object/parent_id must satisfy the attach rule on the NEW parent (edit access; the beforeInsert gate's own check) — the comment kit's thread re-point rule, adapted to the two-field polymorphic parent (partial re-points resolve the missing half from each row; an unauthorizable half — null/empty — is refused, not left to validation).
  • Unscoped refusal: dispatchUnscopedMultiWrite: true declared on the registration, so a multi: true update with no where at all is refused whole-operation-shaped, zero-match included — mirroring the delete verb's installAttachmentAccessHooks does not authorize an UNSCOPED multi-delete: no id + no where reads as "nothing to authorize" and deleteMany runs over the whole table #4757 refusal via Decision: should an unscoped multi: true UPDATE get the whole-operation dispatch that #9719 gave DELETE? (the update half split out of #9798) #9974's landed mechanism (HookEntry.dispatchUnscopedMultiWrite in @objectstack/objectql, valid on both write verbs since the 2026-08-19 option-A ruling). Declaring it was the in-scope decision left to this card; declared because the derived kit ships the same both-verbs pairing and the Decision: should an unscoped multi: true UPDATE get the whole-operation dispatch that #9719 gave DELETE? (the update half split out of #9798) #9974 ruling's recoverability rationale (an overwrite leaves no pre-image) applies to attachments verbatim.
  • uploaded_by is deliberately not re-stamped on update (re-stamping would hand the row to whoever edits it); the escalation route is closed by the row rule itself, which the code comment spells out.
• Error vocabulary: the update gate emits the standard catalog code RECORD_NOT_ACCESSIBLE (403), not a new ATTACHMENT_UPDATE_DENIED — ADR-0112 sends generic permission conditions to the catalog, the [finding] @objectstack/rest registers four generic synonyms the standard catalog already covers (CONFLICT, NOT_FOUND, FORBIDDEN, INTERNAL) — contract call, not a cleanup #8211 admission gate would mechanically refuse a new standard-synonym extension code, and REST already maps this code 403 + object-preserving. The re-point refusal reuses the existing ATTACHMENT_PARENT_ACCESS (same condition as insert). The delete gate keeps its grandfathered ATTACHMENT_DELETE_DENIED untouched. Net effect: zero spec/rest edits needed — the same choice the derived comment kit made.
• Shared helpersresolveTargetRows / authorizeRows factored out of the delete handler and used by both verbs (the comment kit's structure), so the two gates cannot drift on what "the matched rows" means. Delete's observable behaviour (codes, messages, memoization, limits) is byte-identical — the pre-existing delete suite passes unmodified.

Tests: ~20 new cases in attachment-access-hooks.test.ts — fake-driven unit coverage for the row rule, the spoof-then-delete escalation pin, degraded mode, bypasses, and the re-point rule; a wired-engine block (ObjectQL + in-memory driver, same rig as the #4757 block) proving the unscoped refusal, the per-row gate, entitled scoped writes actually writing, the re-point through the wire, and value-survival on every refusal; #7145 envelope-forward pins for both new canEdit call sites.

File surface

Dispatched surface was attachment-access-hooks.ts + its tests + changeset. One declared addition: packages/services/service-storage/src/storage-service-plugin.test.ts — its wiring pin enumerates the exact hook events registered on sys_attachment, and adding a registration is precisely what that pin exists to notice; the update is the one-line list entry (+ comment). Not a silent widening — called out here and in the report.

No ADR drafted: the change follows existing rulings (ADR-0112 vocabulary, #9974 option A, #4630's precedent) and introduces no new architectural decision.

Verification

All commands at bd92875 (HEAD of this PR), full package suite green: 24 files / 383 tests passed (pnpm --filter @objectstack/service-storage test, after building the dependency closure).

Reverse verification (both legs with on-disk proof, no dist rebuild needed — the mutated module is consumed via same-package relative src import; the only cross-package dist dependency, @objectstack/objectql, was never mutated):

• Mutation leg: guard source reverted to origin/main (grep -c "'beforeUpdate'" = 0 on disk) → 19 tests fail in the defect's own direction — AssertionError: promise resolved "2" instead of rejecting on the unscoped and per-row wired cases, plus the wiring pin.
• Restore leg: source restored from the branch (grep -c "'beforeUpdate'" = 1 on disk) → 383/383 green.

Gates (union re-derived from the actual diff via node scripts/pm/dispatch-gates.mjs, no paths passed; all at bd92875, all exit 0 with their own verdict lines):
check:slot-lookup · check:test-source-alias · check:type-source-resolution · check:nul-bytes · check:i18n ("all bundles in sync") · docs-audit/check-affected-docs · check:changeset-gate-self-tests · check:objectui-changeset · check-adr-0087-registration ("no declared-breaking changeset") · check-changeset-no-major · check-empty-changeset · check:query-options-erasure ("at the ceiling… none new") · check:engine-double-contract · check:where-matcher ("none new") · check:type-check-coverage · check:type-check-debt (--re-measure: "none above its recorded number", after the full packages closure build). The union added the changeset-family and test-file convention gates beyond the dispatch list; all named here were run.

Changeset: patch for @objectstack/service-storage, stating the tightening plainly (the same level the #9719/#9974 accept-set changes shipped at).

Out-of-scope findings filed separately (see #10091's report comment): the enable.files/enable.feeds capability gates are beforeInsert-only and a now-authorized re-point dodges them; the attachment lifecycle bookkeeping ignores file_id re-points (retention leak, not data loss); a doc example in objectql's HookEntry comment goes half-stale once this lands. None are fixed here — #10091 remains scoped to the authorization guard.

---

Generated by Claude Code

[github-actions]

📓 Docs Drift Check

This PR changes 1 package(s): @objectstack/service-storage, touching 7 documentable anchor(s).

16 hand-written doc(s) name something this change touched — list omitted above 15 rows. Re-derive on the tree named below: node scripts/docs-audit/affected-docs.mjs --json cc21aad8ed32a844f3c94267e36d95037002c254.

⛔ 4 release-owned page(s) also affected — read-only, see AGENTS.md Documentation Guardrails.

What this run could not see

• the SDK route bridge reached 45 of 221 client-bound route-ledger rows — the other 176 have no registrar path: tail to select them, so pages documenting THEIR client methods cannot appear above, on this or any run: node scripts/docs-audit/affected-docs.mjs --bridge-coverage

Coarse fallback — 5 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): node scripts/docs-audit/affected-docs.mjs --json cc21aad8ed32a844f3c94267e36d95037002c254 → packageMentionDocs.

Which tree this was computed on

This run read content/docs from f232e99c47d509f01a69eb64dfbd95b7c004b5ef — the merge of head bd928759b01479104f9208dbdff3254681786af1 into base cc21aad8ed32a844f3c94267e36d95037002c254, which is what actions/checkout gives a pull_request run. Not the PR head.

A worktree cut from an older main holds a different content/docs, so re-deriving there can legitimately return a different list — that is a different tree, not a wrong row. To answer on the same tree:

# while this PR is open — GitHub drops the merge commit once it closes
git fetch origin f232e99c47d509f01a69eb64dfbd95b7c004b5ef && git checkout f232e99c47d509f01a69eb64dfbd95b7c004b5ef
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin cc21aad8ed32a844f3c94267e36d95037002c254 bd928759b01479104f9208dbdff3254681786af1 && git checkout -B drift-repro cc21aad8ed32a844f3c94267e36d95037002c254 && git merge --no-ff bd928759b01479104f9208dbdff3254681786af1
node scripts/docs-audit/affected-docs.mjs --json cc21aad8ed32a844f3c94267e36d95037002c254

⚠️ That checkout carried uncommitted changes, so the commit above does not fully identify what was read.

Advisory only, and a precision-first one (#9192): a page is listed because it names a
symbol, wire route or SDK method this diff touched — not because it mentions a changed
package. Each row says which anchor put it there, so a wrong row is reportable rather than
merely annoying. To re-verify, run the docs-accuracy-audit workflow scoped to these files:
node scripts/docs-audit/affected-docs.mjs cc21aad8ed32a844f3c94267e36d95037002c254 → pass the list as
args.docs, on the commit named under Which tree this was computed on.