Skip to content

Commit 4639cec

Browse files
Elon Muskclaude
andauthored
feat(objectql,plugin-audit): refuse an unscoped multi-UPDATE on the shape, not by accident (#9974) (#10093)
* feat(objectql,plugin-audit): refuse an unscoped multi-UPDATE on the shape (#9974) Extend #9719's whole-operation dispatch to `beforeUpdate`'s predicate path, per the maintainer's option-A ruling of 2026-08-19, and generalize the flag (`dispatchUnscopedMultiDelete` -> `dispatchUnscopedMultiWrite`) instead of adding a sibling: it is already per-registration and per-event, so a delete-only guard still says "delete only". The #4630 refusal in `resolveTargetRows` now fires on the SHAPE — no id and no `where` — on both write verbs. Previously an unscoped multi-update was refused only when it happened to sweep a row the caller lacked rights to, with the per-row message, and resolved silently when the caller owned every row. The three MEASURED-GAP pins are REPLACED with refusal assertions, not relaxed. Objects whose guards do not declare the flag are unchanged. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019yDEhPBC3tcGkW9bkce1HM * docs(changeset): grade the unscoped multi-update refusal as a behaviour change A caller entitled to every row loses a call that works today. Stated as a narrowing of the accept set with the call-site fix named, not as "restoring" a guard, so the release notes read honestly for whoever is affected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_019yDEhPBC3tcGkW9bkce1HM --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent eb2bebe commit 4639cec

10 files changed

Lines changed: 1100 additions & 543 deletions
Lines changed: 17 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,17 @@
1+
---
2+
'@objectstack/objectql': minor
3+
'@objectstack/plugin-audit': minor
4+
'@objectstack/service-storage': patch
5+
---
6+
7+
**Behaviour change:** an unscoped `multi: true` UPDATE of `sys_comment` is now refused, where it previously succeeded for a caller entitled to every row (#9974).
8+
9+
This is not the restoration of a guard that used to work — it is a deliberate narrowing of what the engine accepts, ruled by the maintainer on 2026-08-19. If you issue `ql.update('sys_comment', data, { multi: true })` with **no `where` at all**, that call works today and will start failing with `RECORD_NOT_ACCESSIBLE` / 403. **The fix at the call site is to say which rows you mean** — pass a `where`. The explicit match-all `where: {}` is still accepted and still authorizes every matched row individually; only an *absent* or `null` predicate is refused.
10+
11+
Why the accept set narrowed rather than the declaration: `resolveTargetRows` has declared this refusal for both write verbs since #4630, but on update it could only ever fire by accident — when the sweep happened to touch a row the caller lacked rights to, and then with a per-row message (`Cannot update comment c2: …`) naming a row rather than the shape. A caller who owned every row had the whole table rewritten, and a zero-match probe resolved silently. A guard that fires by accident reads as enforcement while enforcing nothing. The ruling weighed recoverability: a delete leaves a trace of who removed what, an overwrite leaves none — the old value is gone on the spot with nothing to restore from — and a forgotten `where` is the mistake generated code makes most often.
12+
13+
**Engine (`@objectstack/objectql`).** #9719's opt-in whole-operation dispatch now covers `beforeUpdate`'s predicate path as well as `beforeDelete`'s, and the registration flag is **renamed** `dispatchUnscopedMultiDelete`**`dispatchUnscopedMultiWrite`** (one flag generalized to both events rather than a second flag; it is per-registration and per-event, so a delete-only guard still says "delete only" by declaring it on `beforeDelete` alone). Declaring it on any other event is still refused at registration time. Binding `input.id` on the whole-operation context is refused on both verbs (`HookTargetRebindError`, path `unscoped-multi`), and the error now names the caller's event.
14+
15+
**Blast radius.** The dispatch is delivered ONLY to registrations that declare the flag, so `sys_comment` is the only object whose update accept set changes; every other object's unscoped `multi: true` update behaves exactly as before. `sys_attachment` keeps its delete-only declaration and is unaffected on update. A repo-wide structural sweep of 4 663 source files found no in-tree caller — none in `examples/`, none in the dogfood apps, none in `packages/` source — that issues an unscoped `multi: true` update against a declaring object.
16+
17+
**`@objectstack/service-storage`** is a rename-only follow: its `sys_attachment` guard declares the renamed flag on the same event, with the same behaviour.

packages/objectql/src/engine-unscoped-multi-delete-dispatch.test.ts

Lines changed: 0 additions & 381 deletions
This file was deleted.

packages/objectql/src/engine-unscoped-multi-write-dispatch.test.ts

Lines changed: 716 additions & 0 deletions
Large diffs are not rendered by default.

packages/objectql/src/engine.ts

Lines changed: 143 additions & 62 deletions
Large diffs are not rendered by default.

packages/objectql/src/hook-target-rebind-errors.ts

Lines changed: 13 additions & 11 deletions
Original file line numberDiff line numberDiff line change
@@ -101,12 +101,14 @@ export type HookTargetRebindPath =
101101
/** A per-row `before*` context on a predicate write (D4). */
102102
| 'per-row'
103103
/**
104-
* [#9719] The whole-operation `beforeDelete` dispatch an UNSCOPED predicate
105-
* delete delivers to registrations that declared
106-
* `dispatchUnscopedMultiDelete`. Its `input.id` is present-but-`undefined` —
107-
* the exact slot that used to be the batch dispatch's reroute lever — and
108-
* the ladder is resolved before any handler runs, so binding it retargets
109-
* nothing and is refused rather than ignored, same as the other two seams.
104+
* [#9719, both write verbs since #9974] The whole-operation
105+
* `beforeUpdate` / `beforeDelete` dispatch an UNSCOPED predicate write
106+
* delivers to registrations that declared `dispatchUnscopedMultiWrite`. Its
107+
* `input.id` is present-but-`undefined` — the exact slot that used to be the
108+
* batch dispatch's reroute lever — and the ladder is resolved before any
109+
* handler runs, so binding it retargets nothing and is refused rather than
110+
* ignored, same as the other two seams. `event` says which verb; the seam
111+
* and the rule are one.
110112
*/
111113
| 'unscoped-multi';
112114

@@ -175,11 +177,11 @@ function buildMessage(info: {
175177
`'delete()' honoured a rebind until #6752 by re-resolving the new target; that is retired ` +
176178
`too, so one rule now covers both.`
177179
: path === 'unscoped-multi'
178-
? ` This is the whole-operation dispatch an UNSCOPED predicate delete delivers to a declared ` +
179-
`shape guard (#9719): its 'id' is present-but-undefined ON PURPOSE — there is no target row ` +
180-
`and the dispatch ladder was resolved before any handler ran, so binding 'input.id' here ` +
181-
`retargets nothing. It is refused rather than ignored, because a silent no-op is the ` +
182-
`failure this contract exists to abolish.`
180+
? ` This is the whole-operation dispatch an UNSCOPED predicate write delivers to a declared ` +
181+
`shape guard (#9719, both write verbs since #9974): its 'id' is present-but-undefined ON ` +
182+
`PURPOSE — there is no target row — and the dispatch ladder was resolved before any handler ` +
183+
`ran, so binding 'input.id' here retargets nothing. It is refused rather than ignored, ` +
184+
`because a silent no-op is the failure this contract exists to abolish.`
183185
: ` On a predicate write a '${event}' context arrives with 'id' ALREADY bound to its row and the ` +
184186
`dispatch decided, so rebinding it retargets nothing (ADR-0058 Addendum II, D4). It is refused ` +
185187
`rather than ignored, because a silent no-op is the failure this contract exists to abolish.`;

packages/plugins/plugin-audit/src/comment-access-hooks.test.ts

Lines changed: 149 additions & 42 deletions
Original file line numberDiff line numberDiff line change
@@ -266,8 +266,9 @@ describe('comment access — beforeDelete (author or parent editor)', () => {
266266
// construction — a shape the engine's per-row dispatch (#5038/#5574) never
267267
// produced on either verb, so it stayed green for a behaviour the wired
268268
// engine did the opposite of. It is re-pointed at the REAL engine below —
269-
// see "#4630 through the wired engine", which also PINS the update half's
270-
// still-unreachable state rather than asserting it away.
269+
// see the two "#4630 through the wired engine" blocks. [#9974] The update
270+
// half's block pinned the gap as MEASURED until the dispatch existed; it now
271+
// pins the refusal itself, on both verbs.
271272

272273
it('a dangling-thread comment is modifiable only by its author', async () => {
273274
const orphan = { id: 'c9', thread_id: 'crm_opportunity:', author_id: 'rep1', body: 'orphan' };
@@ -670,12 +671,21 @@ const wiredComment = (id: string, authorId: string, threadId = 'crm_opportunity:
670671

671672
/** ADR-0112 envelope of the #4630 refusal. The first sentence IS the declared
672673
* contract (the issue's quoted wording), so it is asserted alongside the code
673-
* and status rather than instead of them. */
674-
const UNSCOPED_REFUSAL = expect.objectContaining({
674+
* and status rather than instead of them.
675+
*
676+
* [#9974] Parameterized by verb because the refusal now fires on BOTH, and the
677+
* VERB IN THE MESSAGE is load-bearing: before this card an unscoped multi-update
678+
* that was refused at all came back with the PER-ROW message
679+
* (`Cannot update comment c2: …`), which names a row rather than the shape. A
680+
* matcher that only checked code+status would have passed on that wording, so
681+
* the shape refusal is pinned to say "unscoped". */
682+
const unscopedRefusal = (verb: 'update' | 'delete') => expect.objectContaining({
675683
code: 'RECORD_NOT_ACCESSIBLE',
676684
status: 403,
677-
message: expect.stringContaining('Refusing an unscoped multi-delete of comments'),
685+
message: expect.stringContaining(`Refusing an unscoped multi-${verb} of comments`),
678686
});
687+
const UNSCOPED_REFUSAL = unscopedRefusal('delete');
688+
const UNSCOPED_UPDATE_REFUSAL = unscopedRefusal('update');
679689

680690
describe('unscoped multi-DELETE (no id, no where) — #4630 through the wired engine (#9798)', () => {
681691
it('refuses `{ multi: true }` even when the caller AUTHORED every matched row — and the rows survive', async () => {
@@ -779,74 +789,171 @@ describe('unscoped multi-DELETE (no id, no where) — #4630 through the wired en
779789
});
780790
});
781791

782-
describe('unscoped multi-UPDATE (no id, no where) — the still-unreachable half (#9798)', () => {
783-
// ⚠️ THESE PINS DOCUMENT A LIVE FAIL-OPEN, NOT APPROVED BEHAVIOUR. ⚠️
792+
describe('unscoped multi-UPDATE (no id, no where) — #4630 through the wired engine (#9974)', () => {
793+
// ⚠️ THESE PINS REPLACE THREE `MEASURED GAP` PINS, AS THE CARD REQUIRED. ⚠️
784794
//
785-
// `resolveTargetRows` declares the same refusal for `update`, and the block
786-
// these replace asserted it by direct handler call — green while the wired
787-
// engine did the opposite. The refusal cannot be restored the way DELETE's
788-
// was: `dispatchUnscopedMultiDelete` is valid on `beforeDelete` only, and
789-
// the engine refuses it elsewhere BY DESIGN, because extending the
790-
// whole-operation dispatch to `beforeUpdate`'s predicate path is a
791-
// product-behaviour decision rather than drift (#9719's
792-
// `assertValidUnscopedMultiDeleteFlag`).
795+
// Until #9974 this block documented a LIVE FAIL-OPEN on purpose: the same
796+
// refusal `resolveTargetRows` declares for `delete` was declared for `update`
797+
// and could not fire, because `dispatchUnscopedMultiDelete` was valid on
798+
// `beforeDelete` only and the engine refused it elsewhere BY DESIGN —
799+
// extending the whole-operation dispatch to `beforeUpdate`'s predicate path
800+
// was a product-behaviour decision, not drift. Those pins were annotated to
801+
// go RED when the decision landed, and that is what happened: the maintainer
802+
// ruled option A on 2026-08-19, the flag became `dispatchUnscopedMultiWrite`
803+
// valid on both write verbs, and each MEASURED-GAP assertion below is now the
804+
// REFUSAL it was measuring the absence of — replaced, not relaxed or removed.
793805
//
794-
// So the gap is pinned as MEASURED, deliberately: a suite that simply
795-
// dropped the false-green block would leave nothing to notice the fail-open,
796-
// and one that asserted the refusal would be false-green again. When the
797-
// decision lands and the dispatch exists, these pins go RED — that is their
798-
// job, and the fix is to replace them with the refusal assertions, not to
799-
// relax them. Tracked at #9974 (the decision card this half was split into).
800-
it('MEASURED GAP: an unscoped `{ multi: true }` update the caller AUTHORED every row of is not refused — the whole table is rewritten', async () => {
801-
// The delete half's first limb, verb-swapped: the declared refusal is about
802-
// the SHAPE, so it must fire whatever the rows say. It does not — the
803-
// per-row author shortcut licenses each row and no dispatch ever carries
804-
// the unscoped shape. The blast radius is "every row the caller happens to
805-
// be entitled to", which is the issue's measured claim.
806+
// The three limbs, in the order the card tabled them:
807+
// 1. caller authored every row — was "whole table rewritten", now refused;
808+
// 2. empty table (zero match) — was "nothing ran, resolves", now refused;
809+
// 3. a row the caller may not touch is swept — WAS refused, but with the
810+
// PER-ROW message; now refused on the SHAPE, which is a different and
811+
// stronger claim (see limb 3's own note).
812+
//
813+
// ⚠️ Limb 1 is a BEHAVIOUR CHANGE, not a restoration: that call used to
814+
// succeed for an entitled caller. It is what was ruled for — an overwrite
815+
// leaves no trace and no pre-image, so the less recoverable verb must not be
816+
// the less guarded one — and the changeset says so in those terms.
817+
818+
it('limb 1: an unscoped `{ multi: true }` update is refused even when the caller AUTHORED every row — and the bodies survive', async () => {
819+
// The behaviour change, stated as a test: this exact call resolved before
820+
// #9974 and rewrote both rows. The declared refusal is about the SHAPE, so
821+
// it must fire whatever the rows say — including when every row is the
822+
// caller's own and the per-row gate would have licensed all of them.
806823
const { ql, bodies } = await bootWired({
807824
comments: [wiredComment('c1', 'me'), wiredComment('c2', 'me', 'crm_opportunity:opp2')],
808825
sharing: { canEdit: async () => false },
809826
});
810827
await expect(
811828
ql.update('sys_comment', { body: 'rewritten' }, { multi: true, context: { userId: 'me' } } as any),
812-
).resolves.toBeDefined();
813-
expect(bodies()).toEqual(['rewritten', 'rewritten']);
829+
).rejects.toEqual(UNSCOPED_UPDATE_REFUSAL);
830+
expect(bodies()).toEqual(['body of c1', 'body of c2']);
814831
});
815832

816-
it('MEASURED GAP: an unscoped `{ multi: true }` update of an EMPTY table is not refused either', async () => {
817-
// The zero-match limb: the per-row dispatch is gated on matched rows, so
818-
// nothing runs at all and the caller sees success — the probe that tells an
819-
// attacker the unscoped shape is accepted.
833+
it('refuses an explicitly null `where` the same way', async () => {
834+
const { ql, bodies } = await bootWired({ comments: [wiredComment('c1', 'me')] });
835+
await expect(
836+
ql.update('sys_comment', { body: 'rewritten' }, {
837+
multi: true, where: null, context: { userId: 'me' },
838+
} as any),
839+
).rejects.toEqual(UNSCOPED_UPDATE_REFUSAL);
840+
expect(bodies()).toEqual(['body of c1']);
841+
});
842+
843+
it('limb 2: an unscoped `{ multi: true }` update of an EMPTY table is refused too', async () => {
844+
// The zero-match limb: the per-row dispatch is gated on matched rows, so a
845+
// handler-only fix could never fire here — a caller probing against an
846+
// empty table used to see success and ship the unscoped update.
820847
const { ql } = await bootWired({ comments: [] });
821848
await expect(
822849
ql.update('sys_comment', { body: 'rewritten' }, { multi: true, context: { userId: 'me' } } as any),
850+
).rejects.toEqual(UNSCOPED_UPDATE_REFUSAL);
851+
});
852+
853+
it('positive control: an empty table is not refused per se — a scoped `where: {}` update of it resolves', async () => {
854+
// Proves the empty-table refusal above measures the SHAPE, not emptiness.
855+
const { ql } = await bootWired({ comments: [] });
856+
await expect(
857+
ql.update('sys_comment', { body: 'rewritten' }, {
858+
multi: true, where: {}, context: { userId: 'me' },
859+
} as any),
823860
).resolves.toBeDefined();
824861
});
825862

826-
it('the per-row gate DOES still fire on an unscoped update — it is a partial, row-dependent guard', async () => {
827-
// Why the update half is a smaller hole than delete's was, and why it is
828-
// still a hole: rows the caller may not touch are refused individually
829-
// (with the PER-ROW message, not the unscoped one), so the unscoped update
830-
// is caught only when it happens to sweep a row the caller lacks rights to.
863+
it('limb 3: a swept row the caller may not touch is now refused on the SHAPE, not per-row', async () => {
864+
// The limb that was ALREADY refusing, and the reason it still had to
865+
// change. Before #9974 this answered `Cannot update comment c2: …` — the
866+
// per-row gate catching the unscoped shape by accident, on its way through
867+
// a row it happened to reject. That message names a ROW, so it taught the
868+
// caller "row c2 is protected" when the truth is "this shape is refused";
869+
// scoping the write to c1 alone would have "fixed" it and left the hole.
870+
// The shape check now runs BEFORE any row is read, so the unscoped message
871+
// arrives whatever the sweep would have found.
872+
const { ql, bodies } = await bootWired({
873+
comments: [wiredComment('c1', 'me'), wiredComment('c2', 'someone-else')],
874+
sharing: { canEdit: async () => false },
875+
});
876+
const err = await ql
877+
.update('sys_comment', { body: 'rewritten' }, { multi: true, context: { userId: 'me' } } as any)
878+
.then(() => null, (e: unknown) => e);
879+
expect(err).toEqual(UNSCOPED_UPDATE_REFUSAL);
880+
// The old per-row wording is GONE from this shape, not merely joined by the
881+
// new one — asserting only the new sentence would pass on a message that
882+
// still led with the row.
883+
expect(String((err as Error).message)).not.toContain('Cannot update comment c2');
884+
expect(bodies()).toEqual(['body of c1', 'body of c2']);
885+
});
886+
887+
it('the per-row gate is a DIFFERENT refusal and still fires through the wire', async () => {
888+
// The delete block's twin: a SCOPED update matching a row the caller may
889+
// not touch keeps answering with the per-row message. The two limbs stay
890+
// distinguishable, which is what makes limb 3's assertion meaningful.
831891
const { ql, bodies } = await bootWired({
832892
comments: [wiredComment('c1', 'me'), wiredComment('c2', 'someone-else')],
833893
sharing: { canEdit: async () => false },
834894
});
835895
await expect(
836-
ql.update('sys_comment', { body: 'rewritten' }, { multi: true, context: { userId: 'me' } } as any),
896+
ql.update('sys_comment', { body: 'rewritten' }, {
897+
multi: true,
898+
where: { thread_id: 'crm_opportunity:opp1' },
899+
context: { userId: 'me' },
900+
} as any),
837901
).rejects.toEqual(
838902
expect.objectContaining({
839903
code: 'RECORD_NOT_ACCESSIBLE',
840904
status: 403,
841-
message: expect.stringContaining('Cannot update comment c2'),
905+
message: expect.stringContaining('Cannot update comment'),
842906
}),
843907
);
844908
expect(bodies()).toEqual(['body of c1', 'body of c2']);
845909
});
846910

911+
it('scoped controls still pass: by id, by a real `where`, and by the match-all `where: {}`', async () => {
912+
// Over-firing here would break every legitimate comment edit — the blast
913+
// radius pin for a refusal-widening change.
914+
const byId = await bootWired({ comments: [wiredComment('c1', 'me')] });
915+
await expect(
916+
byId.ql.update('sys_comment', { body: 'edited' }, {
917+
where: { id: 'c1' }, context: { userId: 'me' },
918+
} as any),
919+
).resolves.toBeDefined();
920+
expect(byId.bodies()).toEqual(['edited']);
921+
922+
const byWhere = await bootWired({
923+
comments: [wiredComment('c1', 'me'), wiredComment('c2', 'me', 'crm_opportunity:opp2')],
924+
});
925+
await expect(
926+
byWhere.ql.update('sys_comment', { body: 'edited' }, {
927+
multi: true, where: { author_id: 'me' }, context: { userId: 'me' },
928+
} as any),
929+
).resolves.toBeDefined();
930+
expect(byWhere.bodies()).toEqual(['edited', 'edited']);
931+
932+
// `where: {}` is a REAL match-all query (the declared semantics): every
933+
// matched row is authorized per row, and an entitled caller may rewrite the
934+
// table with it — the refusal is about an ABSENT predicate only.
935+
const matchAll = await bootWired({ comments: [wiredComment('c1', 'me')] });
936+
await expect(
937+
matchAll.ql.update('sys_comment', { body: 'edited' }, {
938+
multi: true, where: {}, context: { userId: 'me' },
939+
} as any),
940+
).resolves.toBeDefined();
941+
expect(matchAll.bodies()).toEqual(['edited']);
942+
});
943+
944+
it('system context still bypasses the refusal — engine self-writes and seeds are not the caller', async () => {
945+
const { ql, bodies } = await bootWired({ comments: [wiredComment('c1', 'me')] });
946+
await expect(
947+
ql.update('sys_comment', { body: 'rewritten' }, {
948+
multi: true, context: { userId: 'x', isSystem: true },
949+
} as any),
950+
).resolves.toBeDefined();
951+
expect(bodies()).toEqual(['rewritten']);
952+
});
953+
847954
it('control: the scoped update paths are gated exactly as declared', async () => {
848-
// The gap above is about the UNSCOPED shape only — a real predicate still
849-
// reaches the per-row author-or-parent-editor gate, and refuses.
955+
// Unchanged from the pre-#9974 block: a real predicate still reaches the
956+
// per-row author-or-parent-editor gate, and refuses there.
850957
const { ql, bodies } = await bootWired({
851958
comments: [wiredComment('c1', 'me'), wiredComment('c2', 'someone-else')],
852959
sharing: { canEdit: async () => false },

0 commit comments

Comments
 (0)