Merge pull request #3044 from objectstack-ai/claude/fix-readonlywhen-multi-3042

fix(security): enforce readonlyWhen on the multi-row UPDATE path (#3042)

Author: os-zhuang

.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.

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<string, unknown>, opCtx.context, hookContext.input.options);
                normalizeMultiValueFields(updateSchema, hookContext.input.data as Record<string, unknown>);
                validateRecord(updateSchema, hookContext.input.data as Record<string, unknown>, '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<string, unknown>, 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<string, unknown>)) {
+                   const priorRows = await driver.find(object, ast, hookContext.input.options as any);
+                   hookContext.input.data = stripReadonlyWhenFieldsMulti(updateSchema as any, hookContext.input.data as Record<string, unknown>, 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<string, unknown>, suppliedKeys, this.logger) as any;
+               }
                result = await driver.updateMany(object, ast, hookContext.input.data as Record<string, unknown>, hookContext.input.options as any);
            } else {
                throw new Error('Update requires an ID or options.multi=true');

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<string, any>[]) {
+      const bulkUpdates: Record<string, any>[] = [];
+      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
+    });
+  });
 });

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: {

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<boolean>(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<string, unknown>)[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<string, unknown>,
+  previous: Record<string, unknown> | undefined,
+  name: string,
+  logger?: EvaluateRulesOptions['logger'],
+): boolean {
+  const res = ExpressionEngine.evaluate<boolean>(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<string, ConditionalFieldDef> } | undefined | null,
+  data: Record<string, unknown> | 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<string, ConditionalFieldDef> } | undefined | null,
+  data: Record<string, unknown> | undefined | null,
+  priorRows: ReadonlyArray<Record<string, unknown>> | undefined | null,
+  logger?: EvaluateRulesOptions['logger'],
+): Record<string, unknown> | 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<string, unknown>)[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