From 819d4a4ffc9a6b482d5191afd9ee3fbfbcd23ce7 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 4 Aug 2026 05:24:06 +0000 Subject: [PATCH] fix(objectql): make the static-readonly write strip state its consequence and remedy (#4903) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A `readonly: true` column written by server-side code — a cron/background job reaching the engine via `ctx.getService('data')` — was dropped while the call reported success, leaving one log line that said what the engine did but not what it cost the caller or how to fix it. The strip now names the object and field, states that the update was COMMITTED WITHOUT the field, and carries both remedies: `{ context: { isSystem: true } }` for genuinely trusted server code, and `options.onFieldsDropped` (#3407) for a machine-readable signal. Level stays `warn` — this seam cannot tell a forged client body from trusted server code (`ExecutionContext` has no origin marker; `isSystem` is the only trust bit and it is the exemption), so `error` would be client-triggerable log spam and `debug` would restore the silent drop. Behaviour unchanged. Adds a pin suite for the hook-backfill asymmetry and its mechanism (`suppliedKeys` is snapshotted at engine entry, before hooks run), and documents the semantics on the security protocol page. Co-Authored-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01NrmBxj8rK2uGCnh9aipjwX --- .../readonly-strip-actionable-warning.md | 36 +++ content/docs/protocol/objectql/security.mdx | 58 ++++ .../src/engine-readonly-strip-signal.test.ts | 268 ++++++++++++++++++ .../objectql/src/validation/rule-validator.ts | 44 ++- 4 files changed, 404 insertions(+), 2 deletions(-) create mode 100644 .changeset/readonly-strip-actionable-warning.md create mode 100644 packages/objectql/src/engine-readonly-strip-signal.test.ts diff --git a/.changeset/readonly-strip-actionable-warning.md b/.changeset/readonly-strip-actionable-warning.md new file mode 100644 index 0000000000..9365c94f8d --- /dev/null +++ b/.changeset/readonly-strip-actionable-warning.md @@ -0,0 +1,36 @@ +--- +'@objectstack/objectql': patch +--- + +fix(objectql): the static-`readonly` write strip now logs its consequence and its remedy + +Writing a `readonly: true` column from server-side code — a cron job or background +task reaching the engine through `ctx.getService('data')` — dropped the value and +reported success. The only trace was: + +``` +WARN Field 'work_duration' is read-only — ignoring incoming change (#2948) +``` + +which says what the engine did, not what it cost the caller or how to fix it. The +downstream symptom is a field that persists fine through every REST path and never +persists from cron (os-project-titanwind-ehr#750), which reads as "cron is broken" +rather than "the value was stripped". The strip now names the object, states that +the update was **committed without the field**, and carries both remedies: trusted +server code declares itself with `{ context: { isSystem: true } }`, and any caller +can detect drops programmatically with `options.onFieldsDropped` (the machine-readable +strip signal that has existed since #3407 — one event per strip pass, with `fields` +and `reason`). + +The level stays `warn`, deliberately: this seam cannot distinguish a hostile REST +body forging `created_by` from trusted server code, because `ExecutionContext` +carries no origin marker and `isSystem` — the only trust bit — is precisely the +exemption. `error` would make the error log client-triggerable; `debug` would restore +the silent drop. + +Behaviour is unchanged: what is stripped, what survives, and what `onFieldsDropped` +reports are all identical. Documented in the [security protocol +page](/docs/protocol/objectql/security) — strip condition, the caller-supplied-keys +scope, why a `beforeUpdate` hook's backfill is exempt (the key snapshot is taken at +engine entry, before hooks run), and the `isSystem` convention for plugin writes — +and pinned in `engine-readonly-strip-signal.test.ts`. diff --git a/content/docs/protocol/objectql/security.mdx b/content/docs/protocol/objectql/security.mdx index e11ee3f383..a8a9468a36 100644 --- a/content/docs/protocol/objectql/security.mdx +++ b/content/docs/protocol/objectql/security.mdx @@ -246,6 +246,64 @@ fields: > There is no separate `create` flag for fields. `editable` governs both create-time and update-time writes; read stripping is governed by `readable`. +### Static `readonly` fields on the write path + +`readonly: true` on a field definition is a *schema-level* lock, independent of permission +sets: no permission set can grant a write to it. It is enforced by **stripping**, not by +rejecting — the offending key is removed from the payload and the rest of the write is +committed. The write therefore **succeeds** (REST answers `200`), and the read-only column +simply keeps its stored value. + +Four rules decide whether a given value survives: + +| # | Rule | Effect | +|:--|:---|:---| +| 1 | **Trusted context is exempt** | A write carrying `context.isSystem === true` skips the strip entirely and may set read-only columns. | +| 2 | **Only *caller-supplied* keys are candidates** | The engine snapshots the payload's keys at entry (`suppliedKeys`), *before* middleware and `beforeUpdate` hooks run. Only keys in that snapshot can be stripped. | +| 3 | **Hook / middleware backfill survives** | A key a `beforeUpdate` hook *adds* to `data` is absent from the entry snapshot, so it is not a candidate — this is why the built-in `updated_by` / `updated_at` stamps land even though those columns are `readonly`. | +| 4 | **`context.preserveAudit` admits a whitelist** | An opt-in historical import reinstates the audit/timestamp family and author-declared business `readonly` fields; platform-managed `system` columns (tenancy, generated) stay stripped. | + +Rule 2 is scoped to keys, not values: a key the caller sent stays a strip candidate even if +a hook later overwrites its value. So a `beforeUpdate` hook can *backfill* a read-only +field, but cannot *rescue* one the caller supplied. + + +**Server-side plugins are not trusted by default.** `ctx.getService('data')` hands back the +same engine the REST layer uses, with an **empty execution context** — so a cron job or +background task writing a system-computed read-only column travels the same strip as +untrusted client input, and its value is dropped while the call reports success. Trusted +server code must say so: + +```ts +const data = ctx.getService('data'); +await data.update('attendance', { id, status: 'closed', work_duration: 480 }, + { context: { isSystem: true } }); // ← required to write a `readonly` column +``` + +`isSystem` is the documented convention for this, and it is deliberately explicit: it +bypasses permission checks too, so it declares "this write is platform code acting as the +platform", not "this write came from a plugin". + + +**Detecting a drop programmatically.** Every strip pass reports itself to the caller +through `options.onFieldsDropped`, so a caller that reports per-field success (a flow's +`update_record` step, an import runner) can surface a warning instead of a clean success: + +```ts +await data.update('attendance', { id, work_duration: 480 }, { + onFieldsDropped: ({ object, fields, reason }) => { + // { object: 'attendance', fields: ['work_duration'], reason: 'readonly' } + logger.warn(`dropped ${fields.join(', ')} on ${object} (${reason})`); + }, +}); +``` + +`reason` is `'readonly'` for this static lock and `'readonly_when'` for a conditional +[`readonlyWhen`](/docs/references/data/field) predicate. The listener is an in-process +callback: it is delivered by the local engine, and does **not** cross the RPC / Virtual +Data Engine boundary, so a remote caller never receives these events. Without a listener, +the only trace is a server-side `WARN` naming the object, the field, and both remedies. + --- ## 4. Data Masking diff --git a/packages/objectql/src/engine-readonly-strip-signal.test.ts b/packages/objectql/src/engine-readonly-strip-signal.test.ts new file mode 100644 index 0000000000..f18992d2ff --- /dev/null +++ b/packages/objectql/src/engine-readonly-strip-signal.test.ts @@ -0,0 +1,268 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// #4903 — the static `readonly: true` write strip, from the CALLER's side. +// +// A server-side plugin (cron / background job) that writes a `readonly` column +// through `ctx.getService('data').update(...)` gets a SUCCESSFUL call whose +// value never lands: `getService('data')` hands back the plain engine, whose +// execution context is empty, so `!context.isSystem` sends trusted server code +// down the same strip as untrusted client input. Downstream report: +// os-project-titanwind-ehr#750 — a settled `work_duration` stayed `null` when +// (and only when) the auto-clock-out cron wrote it, while every REST path for +// the same field worked, which reads as "cron is broken" rather than "the field +// was stripped". +// +// This suite pins the three things that decide how expensive that is to +// diagnose: +// +// 1. WHY the hook path differs (`suppliedKeys` is snapshotted at engine entry, +// BEFORE middleware and beforeUpdate hooks run) — the asymmetry the issue +// reports is pinned as EXISTING behaviour so the next change to it is +// deliberate. Nothing here endorses it. +// 2. The strip's log states the CONSEQUENCE and both REMEDIES, so the log is +// actionable on its own (#4632's second-class shape: caller believes +// persisted, database disagrees, log is the only trace). +// 3. The machine-readable signal that ALREADY exists — `onFieldsDropped` +// (#3407) — is reachable from exactly the caller shape the issue describes +// (in-process engine, no context), and `{ context: { isSystem: true } }` +// makes the same write land. +// +// What is NOT here: a strict/reject mode. That needs a new write-option key, +// and both homes for it (`EngineUpdateOptionsSchema`, `WriteObservabilityOptions`) +// live in `packages/spec` — see the issue thread. + +import { describe, it, expect, beforeEach } from 'vitest'; +import { ObjectQL } from './engine.js'; +import { readonlyStripWarning, stripReadonlyFields } from './validation/rule-validator.js'; + +function makeDriver() { + const stores = new Map>(); + const storeFor = (o: string) => { + let s = stores.get(o); + if (!s) { s = new Map(); stores.set(o, s); } + return s; + }; + const matches = (row: any, where: any): boolean => { + if (!where || typeof where !== 'object') return true; + return Object.entries(where).every(([k, v]: [string, any]) => row?.[k] === v); + }; + let n = 0; + const driver: any = { + name: 'memory', version: '0.0.0', supports: {}, + async connect() {}, async disconnect() {}, async checkHealth() { return true; }, async execute() { return null; }, + async find(object: string, ast: any) { + return Array.from(storeFor(object).values()).filter((r) => matches(r, ast?.where)); + }, + async findOne(object: string, ast: any) { + for (const r of storeFor(object).values()) if (matches(r, ast?.where)) return r; + return null; + }, + async create(object: string, data: Record) { + n += 1; + const id = (data.id as string) ?? `r_${n}`; + const row = { ...data, id }; + storeFor(object).set(id, row); + return row; + }, + async update(object: string, id: string, data: Record) { + const s = storeFor(object); + const row = { ...s.get(id), ...data, id }; + s.set(id, row); + return row; + }, + async updateMany(object: string, ast: any, data: Record) { + const s = storeFor(object); + let count = 0; + for (const row of [...s.values()]) { + if (!matches(row, ast?.where)) continue; + s.set(row.id, { ...row, ...data, id: row.id }); + count += 1; + } + return count; + }, + async delete(object: string, id: string) { return storeFor(object).delete(id); }, + async count() { return 0; }, + async bulkCreate(object: string, rows: Record[]) { + return Promise.all(rows.map((r) => this.create(object, r, undefined))); + }, + async bulkUpdate() { return []; }, async bulkDelete() {}, + async beginTransaction() { return { __trx: true, commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, async rollback() {}, + }; + return { driver, storeFor }; +} + +/** A logger that records the lines the engine writes, for the log-contract pin. */ +function makeCapturingLogger() { + const warns: string[] = []; + const logger: any = { + warns, + debug() {}, info() {}, error() {}, trace() {}, fatal() {}, + warn(msg: string) { warns.push(String(msg)); }, + child() { return logger; }, + }; + return logger; +} + +describe('static `readonly` write strip — caller-facing signal (#4903)', () => { + let engine: ObjectQL; + let logger: ReturnType; + let storeFor: ReturnType['storeFor']; + + beforeEach(async () => { + logger = makeCapturingLogger(); + engine = new ObjectQL({ logger }); + const d = makeDriver(); + storeFor = d.storeFor; + engine.registerDriver(d.driver, true); + await engine.init(); + // The downstream shape, trimmed: an attendance record whose worked-hours + // column is settled by the platform, never typed by a user. + engine.registry.registerObject({ + name: 'attendance', + fields: { + status: { type: 'text' }, + check_out_time: { type: 'datetime' }, + work_duration: { type: 'number', readonly: true }, + }, + } as any); + storeFor('attendance').set('att_1', { id: 'att_1', status: 'open', work_duration: null }); + }); + + const att = () => storeFor('attendance').get('att_1'); + + // ── 1. the reported behaviour, and the documented way out ─────────────── + + it('THE REPORT: a contextless plugin write lands every field EXCEPT the readonly one', async () => { + await engine.update('attendance', { + id: 'att_1', status: 'closed', check_out_time: '2026-08-04T09:00:00Z', work_duration: 480, + }); + // The call resolved without throwing — that is the whole complaint. + expect(att()).toMatchObject({ status: 'closed', check_out_time: '2026-08-04T09:00:00Z' }); + expect(att().work_duration).toBeNull(); + }); + + it('the SAME write lands in full once the caller declares itself trusted', async () => { + await engine.update( + 'attendance', + { id: 'att_1', status: 'closed', work_duration: 480 }, + { context: { isSystem: true } } as any, + ); + expect(att()).toMatchObject({ status: 'closed', work_duration: 480 }); + }); + + // ── 2. the machine-readable signal that already exists (#3407) ─────────── + + it('reports the drop to a contextless caller via onFieldsDropped', async () => { + // The listener is reachable from exactly the shape the issue describes: + // `ctx.getService('data')` registers the in-process engine (plugin.ts), so + // no RPC boundary swallows the event. + const events: any[] = []; + await engine.update( + 'attendance', + { id: 'att_1', status: 'closed', work_duration: 480 }, + { onFieldsDropped: (e: any) => events.push(e) } as any, + ); + expect(events).toEqual([{ object: 'attendance', fields: ['work_duration'], reason: 'readonly' }]); + }); + + it('reports the drop on the BULK path too', async () => { + const events: any[] = []; + await engine.update( + 'attendance', + { work_duration: 480 }, + { where: { status: 'open' }, multi: true, onFieldsDropped: (e: any) => events.push(e) } as any, + ); + expect(events).toEqual([{ object: 'attendance', fields: ['work_duration'], reason: 'readonly' }]); + expect(att().work_duration).toBeNull(); + }); + + // ── 3. the log is actionable on its own ───────────────────────────────── + + it('the strip WARNs the consequence and BOTH remedies, naming the object and field', async () => { + await engine.update('attendance', { id: 'att_1', work_duration: 480 }); + const line = logger.warns.find((w: string) => w.includes('work_duration')); + expect(line, 'the strip must log').toBeDefined(); + // Consequence — not just "ignoring incoming change". + expect(line).toContain("Field 'work_duration' on 'attendance'"); + expect(line).toContain('COMMITTED WITHOUT IT'); + // Remedy A: the documented convention for genuinely trusted server code. + expect(line).toContain('{ context: { isSystem: true } }'); + // Remedy B: the machine-readable signal, so the log is not the only way out. + expect(line).toContain('onFieldsDropped'); + }); + + it('stays at WARN — the seam cannot tell a forged client body from trusted server code', () => { + // Pinned deliberately: `ExecutionContext` carries no origin/channel marker, + // so an `error` level here would be client-triggerable log spam and a + // `debug` level would restore the silent drop. One level, chosen. + const calls: Array<[string, string]> = []; + const probe: any = { + warn: (m: string) => calls.push(['warn', m]), + error: (m: string) => calls.push(['error', m]), + info: (m: string) => calls.push(['info', m]), + debug: (m: string) => calls.push(['debug', m]), + }; + stripReadonlyFields( + { name: 'attendance', fields: { work_duration: { type: 'number', readonly: true } } } as any, + { work_duration: 480 }, + new Set(['work_duration']), + probe, + ); + expect(calls.map(([level]) => level)).toEqual(['warn']); + expect(calls[0][1]).toBe(readonlyStripWarning('work_duration', 'attendance')); + }); + + it('omits the object clause when the schema carries no name', () => { + expect(readonlyStripWarning('work_duration')).toContain("Field 'work_duration' is read-only"); + }); + + // ── 4. the hook/plugin asymmetry, PINNED as-is ────────────────────────── + // + // NOT an endorsement. The issue asks whether a beforeUpdate hook SHOULD be + // able to write a column a plugin cannot; that question is open. This pins + // the current answer and — more usefully — the MECHANISM, so a future change + // is made on purpose instead of by accident. + + describe('beforeUpdate backfill vs. caller supply (pinned mechanism)', () => { + it('a hook-written readonly field LANDS while the same field supplied by the caller is stripped', async () => { + engine.registerHook('beforeUpdate', async (ctx: any) => { + ctx.input.data.work_duration = 480; + }, { object: 'attendance' }); + + // Caller supplies nothing read-only; the hook backfills it → persisted. + await engine.update('attendance', { id: 'att_1', status: 'closed' }); + expect(att().work_duration).toBe(480); + }); + + it('a hook cannot rescue a key the CALLER supplied — the snapshot is taken first', async () => { + // The mechanism, stated: `suppliedKeys` is `new Set(Object.keys(data))` + // captured at engine entry, BEFORE middleware and beforeUpdate hooks run. + // A key the hook ADDS is absent from that snapshot and survives; a key the + // caller sent is in it and is stripped no matter what the hook does to the + // value afterwards. + storeFor('attendance').set('att_2', { id: 'att_2', status: 'open', work_duration: null }); + engine.registerHook('beforeUpdate', async (ctx: any) => { + if (ctx.input.data.work_duration !== undefined) ctx.input.data.work_duration = 999; + }, { object: 'attendance' }); + + await engine.update('attendance', { id: 'att_2', status: 'closed', work_duration: 480 }); + expect(storeFor('attendance').get('att_2')).toMatchObject({ status: 'closed' }); + expect(storeFor('attendance').get('att_2').work_duration).toBeNull(); + }); + + it('the engine-stamped audit column is the same exemption, not a special case', () => { + // `updated_by` survives a user write for exactly one reason: the audit + // hook writes it, so it is not in `suppliedKeys`. Supplied explicitly, it + // is dropped like any other readonly field. + const schema = { + name: 'attendance', + fields: { updated_by: { type: 'text', readonly: true, system: true } }, + } as any; + const stamped = stripReadonlyFields(schema, { updated_by: 'hook-stamp' }, new Set()); + expect(stamped).toEqual({ updated_by: 'hook-stamp' }); + const forged = stripReadonlyFields(schema, { updated_by: 'attacker' }, new Set(['updated_by'])); + expect(forged).toEqual({}); + }); + }); +}); diff --git a/packages/objectql/src/validation/rule-validator.ts b/packages/objectql/src/validation/rule-validator.ts index 3ae1b858e0..8f21bfadef 100644 --- a/packages/objectql/src/validation/rule-validator.ts +++ b/packages/objectql/src/validation/rule-validator.ts @@ -542,9 +542,30 @@ export function stripReadonlyWhenFieldsMulti( * * Returns the same object when nothing is stripped, else a shallow copy with the * offending keys removed. + * + * ### What the strip LOGS, and why it stays at `warn` (#4903) + * + * A dropped field is the #4632 second-class shape: the caller is told the write + * succeeded, the database disagrees about one column, and the server log is the + * only trace. So the message must carry the CONSEQUENCE (committed without the + * field) and the REMEDY, not just the fact — a bare "ignoring incoming change" + * is what made os-project-titanwind-ehr#750 read as "only cron can't write". + * + * It stays at `warn`, one level for every caller, because THIS SEAM CANNOT TELL + * THE TWO CALLERS APART. The strip runs on `!context.isSystem`, and + * `ExecutionContext` carries no origin / channel / transport marker — `isSystem` + * is the only trust bit, and it is precisely the exemption. A hostile REST body + * forging `created_by` and a trusted cron plugin writing a computed column + * arrive here indistinguishable (an absent context is not proof of server code + * either: a plugin may legitimately act as a user, and an unauthenticated REST + * write also has no principal). Escalating to `error` would therefore let any + * client fill the error log on demand; demoting to `debug` would re-hide the + * silent-drop. `warn` + a message that names both remedies is the honest single + * level. Changing this means giving `ExecutionContext` a real origin marker + * first — not guessing from the absence of a principal. */ export function stripReadonlyFields( - objectSchema: { fields?: Record } | undefined | null, + objectSchema: { name?: string; fields?: Record } | undefined | null, data: Record | undefined | null, suppliedKeys: ReadonlySet, logger?: EvaluateRulesOptions['logger'], @@ -561,11 +582,30 @@ export function stripReadonlyFields( if (preserveAudit && isPreservableUnderAudit(name, def)) continue; // historical import reinstates it if (result === data) result = { ...data }; delete (result as Record)[name]; - logger?.warn?.(`Field '${name}' is read-only — ignoring incoming change (#2948)`); + logger?.warn?.(readonlyStripWarning(name, objectSchema?.name)); } return result; } +/** + * The message {@link stripReadonlyFields} logs per dropped field (#4903). + * Exported so the pin test asserts the CONTRACT of this text — consequence, + * `isSystem` remedy, `onFieldsDropped` remedy — rather than its wording. + */ +export function readonlyStripWarning(field: string, object?: string): string { + const on = object ? ` on '${object}'` : ''; + return ( + `Field '${field}'${on} is read-only: the caller-supplied value was DROPPED and the update ` + + `is being COMMITTED WITHOUT IT — the call returns success while this column keeps its stored ` + + `value (#2948). Server-side code that legitimately writes read-only columns (a plugin, a cron / ` + + `background job persisting a system-computed value) must declare itself trusted by passing ` + + `{ context: { isSystem: true } } on the write; a beforeUpdate hook does NOT need this because ` + + `hook-written keys are not caller-supplied. To detect drops programmatically instead of reading ` + + `this log, pass options.onFieldsDropped (#3407). Forged read-only keys from untrusted client ` + + `input are expected here and need no action.` + ); +} + /** * The audit / attribution family — the "original timeline" a historical import * (`preserveAudit`) is allowed to reinstate even though these columns are