fix(security): enforce static readonly fields on the UPDATE write path (#2948)

.changeset/authz-2948-readonly-update.md

@@ -0,0 +1,28 @@
+---
+'@objectstack/objectql': minor
+---
+
+fix(security): enforce static `readonly` fields on the UPDATE write path (#2948)
+
+A field's static `readonly: true` was never enforced server-side on update: the
+record validator only *skipped* read-only columns from validation, and only the
+conditional `readonlyWhen` variant was stripped from the write payload. A
+non-system (user-context) update could therefore overwrite any `readonly`
+column — audit stamps, provenance (`managed_by`), or other system-computed
+values — unless a field-level permission happened to guard it. (The
+cross-tenant `organization_id` face was already closed by #2946; this is the
+broader in-tenant integrity face.)
+
+`engine.update` now strips **caller-supplied** writes to statically-`readonly`
+fields for non-system contexts, on both the single-id and multi-row paths
+(symmetric with `readonlyWhen` — it strips, does not reject). Two guards keep
+every legitimate write intact:
+
+- **caller-supplied only** — the strip runs against a snapshot of the keys the
+  caller sent *before* hooks/middleware ran, so server stamps applied by the
+  audit hook (`updated_by`/`updated_at`) and write middleware survive; only a
+  client that explicitly forged a read-only field has it dropped.
+- **system-context exempt** — `isSystem` writes (import, seed replay, approvals,
+  lifecycle hooks) legitimately set read-only columns and skip the strip.
+
+No change for single-org or any write that does not forge a read-only column.

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 } from './validation/rule-validator.js';
+import { evaluateValidationRules, needsPriorRecord, stripReadonlyWhenFields, stripReadonlyFields } from './validation/rule-validator.js';
 import { applyInMemoryAggregation } from './in-memory-aggregation.js';
 
 interface FormulaPlanEntry { name: string; expression: Expression; }
@@ -2357,6 +2357,14 @@ export class ObjectQL implements IDataEngine {
        context: options?.context,
      };
 
+     // [#2948] Snapshot the keys the CALLER supplied, BEFORE any middleware /
+     // beforeUpdate hook stamps server-managed columns (owner/tenant stamp,
+     // `updated_by`/`updated_at`). The static-`readonly` strip below drops only
+     // caller-supplied read-only writes, so hook/middleware stamps survive.
+     const suppliedKeys: ReadonlySet<string> = new Set(
+       Object.keys((opCtx.data ?? {}) as Record<string, unknown>),
+     );
+
      await this.executeWithMiddleware(opCtx, async () => {
        const hookContext: HookContext = {
           object,
@@ -2395,6 +2403,14 @@ export class ObjectQL implements IDataEngine {
                // field is read-only for this record's state, so the incoming
                // change is ignored (the persisted value is kept).
                hookContext.input.data = stripReadonlyWhenFields(updateSchema as any, hookContext.input.data as Record<string, unknown>, priorRecord, this.logger) as any;
+               // [#2948] Enforce STATIC `readonly` on the write path for
+               // non-system callers (system writes legitimately set read-only
+               // columns and are exempt). Runs AFTER hooks/middleware stamped
+               // their columns; `suppliedKeys` ensures only caller-forged
+               // read-only writes are dropped, never the server stamps.
+               if (!opCtx.context?.isSystem) {
+                   hookContext.input.data = stripReadonlyFields(updateSchema as any, hookContext.input.data as Record<string, unknown>, suppliedKeys, this.logger) as any;
+               }
                evaluateValidationRules(updateSchema as any, hookContext.input.data as Record<string, unknown>, 'update', { previous: priorRecord, logger: this.logger, currentUser: this.buildEvalUser(opCtx.context) });
                result = await driver.update(object, hookContext.input.id as string, hookContext.input.data as Record<string, unknown>, hookContext.input.options as any);
            } else if (options?.multi && driver.updateMany) {
@@ -2407,6 +2423,13 @@ export class ObjectQL implements IDataEngine {
                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;
+               }
                const ast: QueryAST = { object, where: options.where };
                result = await driver.updateMany(object, ast, hookContext.input.data as Record<string, unknown>, hookContext.input.options as any);
            } else {

packages/objectql/src/plugin.integration.test.ts

@@ -1262,4 +1262,64 @@ describe('ObjectQLPlugin - Metadata Service Integration', () => {
       expect(row.tenant_id).toBe('org-7');
     });
   });
+
+  // #2948 — static `readonly:true` fields must not be writable via a
+  // NON-system UPDATE. The strip is caller-supplied-only, so server stamps
+  // (updated_by) written by the audit hook survive; system context is exempt.
+  describe('Static readonly write enforcement on UPDATE (#2948)', () => {
+    async function bootWithCapture() {
+      const updates: Record<string, any>[] = [];
+      const mockDriver = {
+        name: 'ro-capture', version: '1.0.0',
+        connect: async () => {}, disconnect: async () => {},
+        find: async () => [], findOne: async () => null,
+        create: async (_o: string, d: any) => ({ id: 'rec-1', ...d }),
+        update: async (_o: string, _i: any, d: any) => { updates.push({ ...d }); return { id: _i, ...d }; },
+        delete: async () => true, syncSchema: async () => {},
+      };
+      await kernel.use({
+        name: 'ro-capture-plugin', type: 'driver', version: '1.0.0',
+        init: async (ctx) => { ctx.registerService('driver.ro-capture', mockDriver); },
+      });
+      await kernel.use(new ObjectQLPlugin());
+      await kernel.bootstrap();
+      const objectql = kernel.getService('objectql') as any;
+      const obj: ObjectSchema = {
+        name: 'ro_obj', label: 'RO Obj', datasource: 'ro-capture',
+        fields: {
+          name: { name: 'name', label: 'Name', type: 'text' },
+          // a statically read-only business column (e.g. a provenance stamp)
+          locked: { name: 'locked', label: 'Locked', type: 'text', readonly: true } as any,
+          updated_by: { name: 'updated_by', label: 'Updated By', type: 'text', readonly: true } as any,
+        },
+      };
+      objectql.registry.registerObject(obj, 'test', 'test');
+      return { objectql, updates };
+    }
+
+    it('drops a user-context write to a static readonly field, but keeps the server updated_by stamp', async () => {
+      const { objectql, updates } = await bootWithCapture();
+      await objectql.update(
+        'ro_obj',
+        { id: 'rec-1', name: 'new-name', locked: 'attacker-value' },
+        { context: { userId: 'user-9', tenantId: 'org-1' } },
+      );
+      expect(updates.length).toBe(1);
+      const data = updates[0];
+      expect(data.name).toBe('new-name');           // editable field written
+      expect(data).not.toHaveProperty('locked');    // readonly forge stripped
+      expect(data.updated_by).toBe('user-9');        // server stamp survived
+    });
+
+    it('ALLOWS a system-context write to the same static readonly field', async () => {
+      const { objectql, updates } = await bootWithCapture();
+      await objectql.update(
+        'ro_obj',
+        { id: 'rec-1', locked: 'legitimate-migration-value' },
+        { context: { isSystem: true } },
+      );
+      expect(updates.length).toBe(1);
+      expect(updates[0].locked).toBe('legitimate-migration-value');
+    });
+  });
 });

packages/objectql/src/validation/rule-validator.test.ts

@@ -6,6 +6,7 @@ import {
   needsPriorRecord,
   legalNextStates,
   stripReadonlyWhenFields,
+  stripReadonlyFields,
 } from './rule-validator.js';
 import { ValidationError } from './record-validator.js';
 
@@ -56,6 +57,46 @@ describe('stripReadonlyWhenFields (B2)', () => {
   });
 });
 
+// #2948 — static `readonly:true` write enforcement (caller-supplied only).
+const stampedFields = {
+  fields: {
+    title: { type: 'text' },
+    created_by: { type: 'lookup', readonly: true },
+    updated_by: { type: 'lookup', readonly: true },
+  },
+};
+
+describe('stripReadonlyFields (#2948)', () => {
+  it('drops a caller-supplied write to a static readonly field', () => {
+    const supplied = new Set(['title', 'created_by']);
+    const out = stripReadonlyFields(stampedFields, { title: 'x', created_by: 'attacker' }, supplied);
+    expect(out).toEqual({ title: 'x' });
+  });
+
+  it('KEEPS a readonly field the caller did NOT supply (server stamp survives)', () => {
+    // `updated_by` was written into `data` by the audit-stamp hook, not the
+    // caller — it must not be stripped.
+    const supplied = new Set(['title']);
+    const out = stripReadonlyFields(stampedFields, { title: 'x', updated_by: 'u1' }, supplied);
+    expect(out).toEqual({ title: 'x', updated_by: 'u1' });
+  });
+
+  it('returns the SAME object when nothing is stripped', () => {
+    const d = { title: 'x' };
+    expect(stripReadonlyFields(stampedFields, d, new Set(['title']))).toBe(d);
+  });
+
+  it('drops a caller-forged readonly field even when it also carries a server stamp key', () => {
+    const supplied = new Set(['title', 'created_by']);
+    const out = stripReadonlyFields(
+      stampedFields,
+      { title: 'x', created_by: 'attacker', updated_by: 'u1' },
+      supplied,
+    );
+    expect(out).toEqual({ title: 'x', updated_by: 'u1' });
+  });
+});
+
 describe('needsPriorRecord — field conditional rules (B2)', () => {
   it('is true when a field declares requiredWhen / readonlyWhen', () => {
     expect(needsPriorRecord(invoiceFields as any)).toBe(true);

packages/objectql/src/validation/rule-validator.ts

@@ -200,6 +200,49 @@ export function stripReadonlyWhenFields(
   return result;
 }
 
+/**
+ * Strip CALLER-SUPPLIED writes to statically `readonly: true` fields from an
+ * UPDATE payload (#2948). Unlike `readonlyWhen` (conditional, handled above), a
+ * static `readonly` field was never enforced on the server write path: the
+ * record validator only SKIPS it from validation, so a user-context update
+ * could overwrite audit stamps, provenance, or any other read-only column. We
+ * STRIP the change (symmetric with `readonlyWhen`) rather than reject it, for
+ * compatibility.
+ *
+ * Two guards keep every legitimate write intact:
+ *  - `suppliedKeys` — only keys the CALLER sent are candidates. Server stamps
+ *    applied by beforeUpdate hooks or write middleware (e.g. `updated_by` /
+ *    `updated_at`, plugin.ts) land in `data` but are NOT in `suppliedKeys`, so
+ *    they survive. A caller that *explicitly* forges e.g. `updated_by` simply
+ *    has it dropped for that request (the last-modified stamp is left unchanged
+ *    — safe).
+ *  - system context — the caller passes this strip only for NON-system writes;
+ *    system-context writes (import, seed replay, approvals, lifecycle hooks —
+ *    all `isSystem: true`) legitimately set read-only columns and skip it.
+ *
+ * Returns the same object when nothing is stripped, else a shallow copy with the
+ * offending keys removed.
+ */
+export function stripReadonlyFields(
+  objectSchema: { fields?: Record<string, ConditionalFieldDef> } | undefined | null,
+  data: Record<string, unknown> | undefined | null,
+  suppliedKeys: ReadonlySet<string>,
+  logger?: EvaluateRulesOptions['logger'],
+): Record<string, unknown> | undefined | null {
+  const fields = objectSchema?.fields;
+  if (!fields || !data) return data;
+  let result = data;
+  for (const [name, def] of Object.entries(fields)) {
+    if (!def?.readonly) continue;
+    if (!(name in (result as Record<string, unknown>))) continue;
+    if (!suppliedKeys.has(name)) continue; // server-stamped, not caller-supplied — keep
+    if (result === data) result = { ...data };
+    delete (result as Record<string, unknown>)[name];
+    logger?.warn?.(`Field '${name}' is read-only — ignoring incoming change (#2948)`);
+  }
+  return result;
+}
+
 /**
  * A rule needs the prior record if it reasons about the transition or compares
  * against unchanged fields (`state_machine` / `cross_field` / `script`), or if
@@ -233,6 +276,8 @@ interface ConditionalFieldDef {
   requiredWhen?: string | Expression;
   conditionalRequired?: string | Expression; // back-compat alias of requiredWhen
   readonlyWhen?: string | Expression;
+  /** Static, unconditional read-only flag (`field.readonly`). #2948. */
+  readonly?: boolean;
   /** Field type — scopes per-option `visibleWhen` enforcement to choice fields. */
   type?: string;
   /** Select/multiselect/radio options; an option may gate itself with `visibleWhen`. */