diff --git a/.changeset/authz-2948-readonly-update.md b/.changeset/authz-2948-readonly-update.md new file mode 100644 index 0000000000..b26754b1f1 --- /dev/null +++ b/.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. diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 8e542e22a5..a2095b6449 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 } 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 = new Set( + Object.keys((opCtx.data ?? {}) as Record), + ); + 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, 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, suppliedKeys, this.logger) as any; + } evaluateValidationRules(updateSchema as any, hookContext.input.data as Record, '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, 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, suppliedKeys, this.logger) as any; + } const ast: QueryAST = { object, where: options.where }; result = await driver.updateMany(object, ast, hookContext.input.data as Record, hookContext.input.options as any); } else { diff --git a/packages/objectql/src/plugin.integration.test.ts b/packages/objectql/src/plugin.integration.test.ts index 3bd1595091..701e58fcf3 100644 --- a/packages/objectql/src/plugin.integration.test.ts +++ b/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[] = []; + 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'); + }); + }); }); diff --git a/packages/objectql/src/validation/rule-validator.test.ts b/packages/objectql/src/validation/rule-validator.test.ts index 8b87b9451c..935a7fd7ab 100644 --- a/packages/objectql/src/validation/rule-validator.test.ts +++ b/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); diff --git a/packages/objectql/src/validation/rule-validator.ts b/packages/objectql/src/validation/rule-validator.ts index 5bc940cab0..b626c48abd 100644 --- a/packages/objectql/src/validation/rule-validator.ts +++ b/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 } | undefined | null, + data: Record | undefined | null, + suppliedKeys: ReadonlySet, + logger?: EvaluateRulesOptions['logger'], +): Record | 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))) continue; + if (!suppliedKeys.has(name)) continue; // server-stamped, not caller-supplied — keep + if (result === data) result = { ...data }; + delete (result as Record)[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`. */