fix(objectql): readonly 剥离改为「说清后果 + 给出出路」,并把写路径语义写进文档 (#4903)

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

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.
+
+<Callout type="warn">
+**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".
+</Callout>
+
+**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

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<string, Map<string, any>>();
+  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<string, unknown>) {
+      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<string, unknown>) {
+      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<string, unknown>) {
+      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<string, unknown>[]) {
+      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<typeof makeCapturingLogger>;
+  let storeFor: ReturnType<typeof makeDriver>['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({});
+    });
+  });
+});