From e9a2885e2f548912aea37ed978feda2d17c56bff Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 16 Jul 2026 09:59:32 +0000 Subject: [PATCH] fix(security): enforce readonlyWhen on the multi-row UPDATE path (#3042) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Conditional `readonlyWhen` field locks were stripped only on the single-id UPDATE path (`stripReadonlyWhenFields`). The bulk `update({ multi: true, where })` path enforced static `readonly` (#2948) but never `readonlyWhen` — an asymmetry with both the single-id path and the static-readonly bulk strip. A programmatic or embedded caller (or a plugin) issuing a multi-row update in a user context could write a field its own `readonlyWhen` predicate should have locked. Scope note: the external REST/SDK `updateMany` endpoint was already safe — it loops single-id `engine.update` calls, each of which strips `readonlyWhen`. The gap was the engine's true where-predicate multi path, reached programmatically (embedded ObjectQL, plugins, internal services) — plus defense-in-depth for any future route that wires a bulk where-based update. engine.update now, on the multi-row path and only when the payload actually writes a `readonlyWhen` field (cheap `hasReadonlyWhenInPayload` gate), reads the row-scoped match set with the SAME composed AST the write binds (one query) and drops any field whose predicate is TRUE for at least one matched row via the new `stripReadonlyWhenFieldsMulti`. A single bulk payload cannot keep a field for some rows and drop it for others, so a field locked in any target row is fail-safe-dropped for the batch; a conditional field no matched row locks is written normally (legitimate bulk edits unaffected). Symmetric with the single-id strip; INSERT stays exempt. The per-field predicate eval is factored into a shared `isReadonlyWhenLocked` helper used by both single-id and bulk strips. Tests: rule-validator unit tests for `hasReadonlyWhenInPayload` and `stripReadonlyWhenFieldsMulti` (locked-in-all / locked-in-one / locked-in-none / empty-match / no-field-in-payload), plus an engine integration test driving the real `objectql.update(..., { multi: true, where })` path (forge stripped when a matched row is locked, sibling editable field survives, kept when no row locks it, pre-read is row-scoped with the write's predicate). objectql suite: 898 passed. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01TKR6MTrGunV4p4AUfbKMDU --- .changeset/authz-3042-readonlywhen-multi.md | 26 +++++ packages/objectql/src/engine.ts | 36 ++++--- .../objectql/src/plugin.integration.test.ts | 72 ++++++++++++++ .../src/validation/rule-validator.test.ts | 56 +++++++++++ .../objectql/src/validation/rule-validator.ts | 98 +++++++++++++++++-- 5 files changed, 268 insertions(+), 20 deletions(-) create mode 100644 .changeset/authz-3042-readonlywhen-multi.md diff --git a/.changeset/authz-3042-readonlywhen-multi.md b/.changeset/authz-3042-readonlywhen-multi.md new file mode 100644 index 0000000000..7959a92c70 --- /dev/null +++ b/.changeset/authz-3042-readonlywhen-multi.md @@ -0,0 +1,26 @@ +--- +'@objectstack/objectql': patch +--- + +fix(security): enforce `readonlyWhen` on the multi-row UPDATE path (#3042) + +Conditional `readonlyWhen` field locks were stripped only on the single-id +UPDATE path; the bulk `update({ multi: true, where })` path enforced static +`readonly` (#2948) but never `readonlyWhen`. A programmatic/embedded caller (or +a plugin) issuing a multi-row update in a user context could therefore write a +field its own `readonlyWhen` predicate should have locked — the conditional +lock held for a `PATCH /data/:object/:id` but not for a bulk where-predicate +update. (The external REST/SDK `updateMany` endpoint was unaffected: it loops +single-id `engine.update` calls, which already strip `readonlyWhen`.) + +`engine.update` now, on the multi-row path and only when the payload actually +writes a `readonlyWhen` field, reads the row-scoped match set with the same +composed AST the write binds (one query) and drops any field whose predicate is +TRUE for at least one matched row — a single bulk payload cannot keep a field +for some rows and drop it for others, so a field locked in any target row is +fail-safe-dropped for the batch (narrow the `where` to reach the rows where it +is unlocked). A conditional field NO matched row locks is written normally, so a +legitimate bulk edit is unaffected. Symmetric with the single-id +`stripReadonlyWhenFields` and with the static-`readonly` bulk strip; INSERT +stays exempt. No change for any single-id update or any object without +`readonlyWhen` fields. diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 7a01a9e9e7..474a970663 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -31,7 +31,7 @@ import type { Expression } from '@objectstack/spec'; import { isAggregatedViewContainer, expandViewContainer } from '@objectstack/spec'; import { bindHooksToEngine } from './hook-binder.js'; import { validateRecord, normalizeMultiValueFields, coerceBooleanFields } from './validation/record-validator.js'; -import { evaluateValidationRules, needsPriorRecord, stripReadonlyWhenFields, stripReadonlyFields } from './validation/rule-validator.js'; +import { evaluateValidationRules, needsPriorRecord, stripReadonlyWhenFields, stripReadonlyWhenFieldsMulti, hasReadonlyWhenInPayload, stripReadonlyFields } from './validation/rule-validator.js'; import { applyInMemoryAggregation } from './in-memory-aggregation.js'; interface FormulaPlanEntry { name: string; expression: Expression; } @@ -2443,19 +2443,13 @@ export class ObjectQL implements IDataEngine { await this.encryptSecretFields(object, hookContext.input.data as Record, opCtx.context, hookContext.input.options); normalizeMultiValueFields(updateSchema, hookContext.input.data as Record); validateRecord(updateSchema, hookContext.input.data as Record, 'update'); - // Multi-row update: per-row prior state is not fetched (one query - // per matched row would be unbounded). state_machine / - // cross_field rules are skipped here; warn so the gap is visible. + // Multi-row update: per-row prior state is not fetched for the + // object-level state_machine / cross_field / script rules (they + // need one prior per row, and a single bulk payload cannot diverge + // per row). Warn so the gap stays visible. if (needsPriorRecord(updateSchema as any)) { this.logger.warn('Object-level validation rules (state_machine/cross_field/script) are not enforced on multi-row updates', { object }); } - // [#2948] Same static-`readonly` write guard on the bulk path — - // a forged read-only column in a multi-row update is dropped for - // non-system callers (a foreign `organization_id` is additionally - // rejected upstream by the tenant write wall, #2946). - if (!opCtx.context?.isSystem) { - hookContext.input.data = stripReadonlyFields(updateSchema as any, hookContext.input.data as Record, suppliedKeys, this.logger) as any; - } // [#2982] Consume the middleware-composed AST seeded above, so // the injected row-scoping (RLS write filter, sharing's // editable-rows filter) actually binds the driver operation. Fail @@ -2470,6 +2464,26 @@ export class ObjectQL implements IDataEngine { `(a hook cleared the target id after the security filter was composed).`, ); } + // [#3042] Enforce conditional `readonlyWhen` on the bulk path too. + // Unlike static `readonly` (below), a `readonlyWhen` lock is PER + // ROW, so read the row-scoped match set with the SAME AST the write + // binds (one query, and only when the payload actually writes a + // `readonlyWhen` field) and drop any field locked in ≥1 matched row + // — a bulk write can't keep it for some rows and drop it for others, + // so a field locked in any target row is fail-safe-dropped for all + // (narrow `where` to reach the unlocked rows). Symmetric with the + // single-id `stripReadonlyWhenFields`; INSERT stays exempt. + if (hasReadonlyWhenInPayload(updateSchema as any, hookContext.input.data as Record)) { + const priorRows = await driver.find(object, ast, hookContext.input.options as any); + hookContext.input.data = stripReadonlyWhenFieldsMulti(updateSchema as any, hookContext.input.data as Record, priorRows, this.logger) as any; + } + // [#2948] Same static-`readonly` write guard on the bulk path — + // a forged read-only column in a multi-row update is dropped for + // non-system callers (a foreign `organization_id` is additionally + // rejected upstream by the tenant write wall, #2946). + if (!opCtx.context?.isSystem) { + hookContext.input.data = stripReadonlyFields(updateSchema as any, hookContext.input.data as Record, suppliedKeys, this.logger) as any; + } result = await driver.updateMany(object, ast, hookContext.input.data as Record, hookContext.input.options as any); } else { throw new Error('Update requires an ID or options.multi=true'); diff --git a/packages/objectql/src/plugin.integration.test.ts b/packages/objectql/src/plugin.integration.test.ts index 701e58fcf3..c97cda2a67 100644 --- a/packages/objectql/src/plugin.integration.test.ts +++ b/packages/objectql/src/plugin.integration.test.ts @@ -1322,4 +1322,76 @@ describe('ObjectQLPlugin - Metadata Service Integration', () => { expect(updates[0].locked).toBe('legitimate-migration-value'); }); }); + + // #3042 — conditional `readonlyWhen` must be enforced on the BULK + // (updateMany) path too, not only the single-id path. The bulk strip reads + // the matched rows' prior state and drops a field locked in ≥1 of them. + describe('readonlyWhen write enforcement on multi-row UPDATE (#3042)', () => { + async function bootWithBulkCapture(priorRows: Record[]) { + const bulkUpdates: Record[] = []; + let findAst: any = null; + const mockDriver = { + name: 'rw-capture', version: '1.0.0', + connect: async () => {}, disconnect: async () => {}, + find: async (_o: string, ast: any) => { findAst = ast; return priorRows; }, + findOne: async () => null, + create: async (_o: string, d: any) => ({ id: 'rec-1', ...d }), + update: async (_o: string, _i: any, d: any) => ({ id: _i, ...d }), + updateMany: async (_o: string, _ast: any, d: any) => { bulkUpdates.push({ ...d }); return [{ ...d }]; }, + delete: async () => true, syncSchema: async () => {}, + }; + await kernel.use({ + name: 'rw-capture-plugin', type: 'driver', version: '1.0.0', + init: async (ctx) => { ctx.registerService('driver.rw-capture', mockDriver); }, + }); + await kernel.use(new ObjectQLPlugin()); + await kernel.bootstrap(); + const objectql = kernel.getService('objectql') as any; + const obj: ObjectSchema = { + name: 'rw_obj', label: 'RW Obj', datasource: 'rw-capture', + fields: { + status: { name: 'status', label: 'Status', type: 'text' }, + note: { name: 'note', label: 'Note', type: 'text' }, + // locked once the row is paid (conditional, per-row) + tax_rate: { name: 'tax_rate', label: 'Tax Rate', type: 'number', readonlyWhen: "record.status == 'paid'" } as any, + }, + }; + objectql.registry.registerObject(obj, 'test', 'test'); + return { objectql, bulkUpdates, getFindAst: () => findAst }; + } + + it('drops a readonlyWhen field locked in ≥1 matched row, but keeps a sibling editable field', async () => { + // Match set: one draft (unlocked) + one paid (locked). The bulk payload + // cannot diverge per row, so tax_rate is fail-safe-dropped for the batch. + const { objectql, bulkUpdates, getFindAst } = await bootWithBulkCapture([ + { id: 'a', status: 'draft', tax_rate: 5 }, + { id: 'b', status: 'paid', tax_rate: 7 }, + ]); + await objectql.update( + 'rw_obj', + { tax_rate: 999, note: 'bulk edit' }, + { multi: true, where: { status: { $in: ['draft', 'paid'] } }, context: { userId: 'user-9' } }, + ); + expect(bulkUpdates.length).toBe(1); + const data = bulkUpdates[0]; + expect(data).not.toHaveProperty('tax_rate'); // conditionally-locked forge stripped + expect(data.note).toBe('bulk edit'); // editable sibling still written + // the pre-read was row-scoped with the same predicate the write binds + expect(getFindAst()?.where).toEqual({ status: { $in: ['draft', 'paid'] } }); + }); + + it('KEEPS the readonlyWhen field when NO matched row locks it', async () => { + const { objectql, bulkUpdates } = await bootWithBulkCapture([ + { id: 'a', status: 'draft', tax_rate: 5 }, + { id: 'c', status: 'sent', tax_rate: 6 }, + ]); + await objectql.update( + 'rw_obj', + { tax_rate: 999 }, + { multi: true, where: { status: { $in: ['draft', 'sent'] } }, context: { userId: 'user-9' } }, + ); + expect(bulkUpdates.length).toBe(1); + expect(bulkUpdates[0].tax_rate).toBe(999); // legitimate bulk edit unaffected + }); + }); }); diff --git a/packages/objectql/src/validation/rule-validator.test.ts b/packages/objectql/src/validation/rule-validator.test.ts index 935a7fd7ab..7b92f9e151 100644 --- a/packages/objectql/src/validation/rule-validator.test.ts +++ b/packages/objectql/src/validation/rule-validator.test.ts @@ -6,6 +6,8 @@ import { needsPriorRecord, legalNextStates, stripReadonlyWhenFields, + stripReadonlyWhenFieldsMulti, + hasReadonlyWhenInPayload, stripReadonlyFields, } from './rule-validator.js'; import { ValidationError } from './record-validator.js'; @@ -57,6 +59,60 @@ describe('stripReadonlyWhenFields (B2)', () => { }); }); +// #3042 — `readonlyWhen` on the BULK (updateMany) path. One payload is applied +// to every matched row, so the strip evaluates the predicate against EACH +// matched row's prior state and drops a field locked in ≥1 of them. +describe('hasReadonlyWhenInPayload (#3042 gate)', () => { + it('is TRUE when the payload writes a readonlyWhen field', () => { + expect(hasReadonlyWhenInPayload(invoiceFields, { amount: 999 })).toBe(true); + }); + it('is FALSE when the payload touches no readonlyWhen field', () => { + expect(hasReadonlyWhenInPayload(invoiceFields, { status: 'draft' })).toBe(false); + }); + it('is FALSE for an object with no readonlyWhen fields at all', () => { + expect(hasReadonlyWhenInPayload({ fields: { x: { type: 'number' } } }, { x: 1 })).toBe(false); + }); +}); + +describe('stripReadonlyWhenFieldsMulti (#3042)', () => { + it('drops the field when it is locked in EVERY matched row', () => { + const out = stripReadonlyWhenFieldsMulti(invoiceFields, { amount: 999 }, [ + { status: 'paid', amount: 100 }, + { status: 'paid', amount: 200 }, + ]); + expect(out).toEqual({}); + }); + + it('drops the field when it is locked in AT LEAST ONE matched row (fail-safe for the batch)', () => { + // One draft (unlocked) + one paid (locked). A single bulk payload cannot + // write to the draft and skip the paid row, so the locked field is dropped + // for the whole batch. + const out = stripReadonlyWhenFieldsMulti(invoiceFields, { amount: 999 }, [ + { status: 'draft', amount: 100 }, + { status: 'paid', amount: 200 }, + ]); + expect(out).toEqual({}); + }); + + it('KEEPS the field when NO matched row locks it (legitimate bulk edit unaffected)', () => { + const out = stripReadonlyWhenFieldsMulti(invoiceFields, { amount: 999 }, [ + { status: 'draft', amount: 100 }, + { status: 'sent', amount: 200 }, + ]); + expect(out).toEqual({ amount: 999 }); + }); + + it('keeps the field when the match set is empty (0 rows updated anyway)', () => { + const d = { amount: 999 }; + expect(stripReadonlyWhenFieldsMulti(invoiceFields, d, [])).toBe(d); + }); + + it('returns the same object when no readonlyWhen field is in the payload', () => { + const d = { status: 'draft' }; + expect(stripReadonlyWhenFieldsMulti(invoiceFields, d, [{ status: 'paid' }])).toBe(d); + }); +}); + // #2948 — static `readonly:true` write enforcement (caller-supplied only). const stampedFields = { fields: { diff --git a/packages/objectql/src/validation/rule-validator.ts b/packages/objectql/src/validation/rule-validator.ts index b626c48abd..9326d1df18 100644 --- a/packages/objectql/src/validation/rule-validator.ts +++ b/packages/objectql/src/validation/rule-validator.ts @@ -183,15 +183,7 @@ export function stripReadonlyWhenFields( let result = data; for (const [name, def] of Object.entries(fields)) { if (!def?.readonlyWhen || !(name in data)) continue; - const res = ExpressionEngine.evaluate(toExpression(def.readonlyWhen), { - record: merged, - previous: previous ?? undefined, - }); - if (!res.ok) { - logger?.warn?.(`readonlyWhen for '${name}' failed to evaluate — change allowed through`); - continue; - } - if (res.value === true) { + if (isReadonlyWhenLocked(def, merged, previous ?? undefined, name, logger)) { if (result === data) result = { ...data }; delete (result as Record)[name]; logger?.warn?.(`Field '${name}' is read-only (readonlyWhen) — ignoring incoming change`); @@ -200,6 +192,94 @@ export function stripReadonlyWhenFields( return result; } +/** + * Evaluate one field's `readonlyWhen` predicate against a (merged) record. + * TRUE ⇒ the field is locked for that record and the incoming change must be + * dropped. A broken predicate is fail-open (returns `false` — the change is + * allowed through), matching the strip's historical behaviour. Shared by the + * single-id ({@link stripReadonlyWhenFields}) and bulk + * ({@link stripReadonlyWhenFieldsMulti}) strips. + */ +function isReadonlyWhenLocked( + def: ConditionalFieldDef, + merged: Record, + previous: Record | undefined, + name: string, + logger?: EvaluateRulesOptions['logger'], +): boolean { + const res = ExpressionEngine.evaluate(toExpression(def.readonlyWhen!), { + record: merged, + previous, + }); + if (!res.ok) { + logger?.warn?.(`readonlyWhen for '${name}' failed to evaluate — change allowed through`); + return false; + } + return res.value === true; +} + +/** + * True when the UPDATE payload writes at least one field that declares a + * `readonlyWhen` predicate. A cheap gate the engine uses to decide whether the + * bulk update path must fetch its matched rows for + * {@link stripReadonlyWhenFieldsMulti} — no `readonlyWhen` field in the payload + * ⇒ no fetch, no cost. + */ +export function hasReadonlyWhenInPayload( + objectSchema: { fields?: Record } | undefined | null, + data: Record | undefined | null, +): boolean { + const fields = objectSchema?.fields; + if (!fields || !data) return false; + for (const [name, def] of Object.entries(fields)) { + if (def?.readonlyWhen && name in data) return true; + } + return false; +} + +/** + * Multi-row counterpart of {@link stripReadonlyWhenFields} (#3042). A bulk + * `updateMany` applies ONE payload to every matched row, but `readonlyWhen` is a + * PER-ROW predicate — a single merged prior is not enough. Given the matched + * rows' prior state (the engine reads them with the SAME row-scoped AST the + * write binds — one query), a `readonlyWhen` field is dropped from the batch + * when its predicate is TRUE for AT LEAST ONE matched row: a bulk write cannot + * lock the field for some rows and write it for others, so a field locked in any + * target row is fail-safe-dropped for all (narrow the `where` to reach the rows + * where it is unlocked). A field NO matched row locks is written normally — a + * legitimate bulk edit of an unlocked conditional field is unaffected. A broken + * predicate is fail-open for that row. INSERT is exempt (update path only), + * symmetric with the single-id strip. + * + * Returns the same object when nothing is stripped, else a shallow copy with the + * locked keys removed. + */ +export function stripReadonlyWhenFieldsMulti( + objectSchema: { fields?: Record } | undefined | null, + data: Record | undefined | null, + priorRows: ReadonlyArray> | undefined | null, + logger?: EvaluateRulesOptions['logger'], +): Record | undefined | null { + const fields = objectSchema?.fields; + if (!fields || !data) return data; + const rows = priorRows ?? []; + let result = data; + for (const [name, def] of Object.entries(fields)) { + if (!def?.readonlyWhen || !(name in data)) continue; + const lockedInSomeRow = rows.some((row) => + isReadonlyWhenLocked(def, { ...(row ?? {}), ...data }, row ?? undefined, name, logger), + ); + if (lockedInSomeRow) { + if (result === data) result = { ...data }; + delete (result as Record)[name]; + logger?.warn?.( + `Field '${name}' is read-only (readonlyWhen) in ≥1 matched row — ignoring incoming change on bulk update`, + ); + } + } + return result; +} + /** * Strip CALLER-SUPPLIED writes to statically `readonly: true` fields from an * UPDATE payload (#2948). Unlike `readonlyWhen` (conditional, handled above), a