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