fix(metadata-protocol): strip static readonly on INSERT at the data-write ingress (#3043)

.changeset/readonly-static-insert-strip.md

@@ -0,0 +1,49 @@
+---
+"@objectstack/metadata-protocol": patch
+"@objectstack/spec": patch
+---
+
+fix(metadata-protocol): strip static `readonly` on INSERT at the data-write ingress (#3043)
+
+#2948/#3003 made static `readonly: true` fields server-enforced on UPDATE (a
+non-system PATCH forging `approval_status: 'approved'` is silently stripped in
+the engine), but INSERT was exempt. For approval/status/verdict columns that
+exemption was the *shorter* attack: instead of the #3003 draft-then-PATCH move, a
+non-system caller could `POST` a record already `approval_status: 'approved'` in
+one step — and the UPDATE-only strip never reached it.
+
+The strip now also runs on INSERT, but at the **external data-write ingress**
+(`DataProtocol.createData` / `createManyData` / `batchData` / `cloneData`) rather
+than in the engine. That seam is the single point every external programmatic
+create funnels through — the REST CRUD route, the GraphQL/MCP dispatcher
+(`bridge.create` → `callData` → `createData`), and bulk import — while **trusted
+internal writers** (better-auth's adapter, the metadata repository, the seed
+loader) call `engine.insert` directly and bypass it. Enforcing at the ingress
+protects every caller/agent path at once without stripping the internal writers
+that legitimately seed read-only columns on create (identity provisioning,
+provenance stamps, event-log cursors) — the blast radius an engine-level insert
+strip would have.
+
+- **Caller-forged only, at the ingress.** The payload here is raw caller input
+  (the security middleware stamps `owner_id` / `organization_id` later, inside
+  `engine.insert`), so only keys the caller actually sent are dropped; server
+  stamps are added afterwards and are unaffected.
+- **Re-derives the default.** A stripped field falls back to its declared
+  `defaultValue` in the engine (a forged `approval_status` becomes `draft`, not
+  NULL).
+- **System-context exempt.** `isSystem` writes still seed read-only columns.
+- **Silent** (HTTP 2xx), per-row on batch/import. `readonlyWhen` stays
+  INSERT-exempt (a conditional lock needs a prior record).
+- **Author-defined business objects only.** Platform objects (`managedBy` set,
+  or the `sys_` namespace) carry their own field-write governance that a silent
+  strip must not pre-empt — e.g. ADR-0086 REJECTS (403) a forged
+  `managed_by:'package'` on `sys_permission_set`, and #3004 rejects a forged
+  `owner_id`; several of those columns are `readonly`, so stripping them here
+  would swallow the payload the guard is meant to reject. The #3043 threat is app
+  approval/status fields, never `sys_` — the same boundary `applySystemFields`
+  uses for ownership.
+
+Behavior change: a non-system create through the data API (REST / GraphQL / MCP /
+import) can no longer seed a `readonly` column from the payload. Flows that
+legitimately write read-only columns at creation must run with a system context
+(`isSystem`), the same requirement the UPDATE strip already imposes.

content/docs/references/data/field.mdx

@@ -150,7 +150,7 @@ const result = Address.parse(data);
 | **conditionalRequired** | `string \| { dialect: Enum<'cel' \| 'js' \| 'cron' \| 'template'>; source?: string; ast?: any; meta?: object }` | optional | Predicate (CEL) — field is required when TRUE. Alias of `requiredWhen`. |
 | **widget** | `string` | optional | Form widget override — names a registered field component (resolved as `field:<widget>`) to render this field instead of the `type` default. Degrades to the `type` renderer when unregistered. e.g. "object-ref", "filter-condition", "recipient-picker". |
 | **hidden** | `boolean` | optional | Hidden from default UI |
-| **readonly** | `boolean` | optional | Read-only — never editable in forms, AND server-enforced on UPDATE: a non-system write to this field is silently dropped from the payload (#2948/#3003; symmetric with `readonlyWhen`). INSERT may still seed it (defaultValue, import). |
+| **readonly** | `boolean` | optional | Read-only — never editable in forms, AND server-enforced on BOTH write paths: a non-system write to this field is silently dropped from the payload on UPDATE (#2948/#3003) and on INSERT (#3043; a create can no longer directly seed e.g. `approval_status: "approved"`), symmetric with `readonlyWhen`. A stripped INSERT field still falls back to its `defaultValue`; system-context writes (import, seed replay, migration) are exempt. |
 | **requiredPermissions** | `string[]` | optional | [ADR-0066 D3] Capabilities required to read/edit this field (mask on read, deny on write; AND-gate). |
 | **system** | `boolean` | optional | Auto-injected system/audit field (e.g. created_at, updated_by, organization_id). Tools that surface system fields separately from author-declared business fields should branch on this flag. |
 | **sortable** | `boolean` | optional | Whether field is sortable in list views |

packages/metadata-protocol/src/protocol.readonly-insert.test.ts

@@ -0,0 +1,117 @@
+// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
+//
+// #3043 — static `readonly: true` fields must not be SEEDABLE via a non-system
+// create through the external data API. The strip lives at the DataProtocol
+// ingress (createData / createManyData / batchData / cloneData) — the seam every
+// external REST/GraphQL/MCP create funnels through — while trusted internal
+// writers call engine.insert directly and are unaffected. It runs BEFORE
+// engine.insert, so a stripped field falls back to its defaultValue (re-derived
+// by the engine, which the mock stands in for). System context is exempt.
+
+import { describe, it, expect, vi } from 'vitest';
+import { ObjectStackProtocolImplementation } from './protocol.js';
+
+const SCHEMA = {
+  name: 'approval_case',
+  fields: {
+    title: { name: 'title', type: 'text' },
+    // readonly approval column — the #3003 attack target
+    approval_status: { name: 'approval_status', type: 'text', readonly: true, defaultValue: 'draft' },
+    // readonly provenance stamp with no default
+    source: { name: 'source', type: 'text', readonly: true },
+  },
+};
+
+function makeProtocol() {
+  const inserts: Array<{ object: string; data: any; options: any }> = [];
+  const engine = {
+    registry: { getObject: (n: string) => (n === 'approval_case' ? SCHEMA : undefined) },
+    insert: vi.fn(async (object: string, data: any, options?: any) => {
+      inserts.push({ object, data, options });
+      const rows = Array.isArray(data) ? data : [data];
+      const out = rows.map((r, i) => ({ id: `rec-${i + 1}`, ...r }));
+      return Array.isArray(data) ? out : out[0];
+    }),
+  };
+  const p = new ObjectStackProtocolImplementation(engine as any);
+  return { p, engine, inserts };
+}
+
+describe('createData — static readonly INSERT strip (#3043)', () => {
+  it('drops a non-system caller forging a readonly field; editable sibling lands', async () => {
+    const { p, inserts } = makeProtocol();
+    await p.createData({
+      object: 'approval_case',
+      data: { title: 'Case A', approval_status: 'approved' },
+      context: { userId: 'u1' },
+    });
+    expect(inserts).toHaveLength(1);
+    expect(inserts[0].data).toEqual({ title: 'Case A' }); // approval_status stripped
+    expect(inserts[0].data).not.toHaveProperty('approval_status');
+  });
+
+  it('ALLOWS a system-context caller to seed the readonly field', async () => {
+    const { p, inserts } = makeProtocol();
+    await p.createData({
+      object: 'approval_case',
+      data: { title: 'Seed', approval_status: 'approved' },
+      context: { isSystem: true },
+    });
+    expect(inserts[0].data.approval_status).toBe('approved');
+  });
+
+  it('strips a forged readonly field even when no context is supplied (non-system default)', async () => {
+    const { p, inserts } = makeProtocol();
+    await p.createData({ object: 'approval_case', data: { title: 'X', source: 'attacker' } });
+    expect(inserts[0].data).not.toHaveProperty('source');
+  });
+
+  it('does NOT strip a PLATFORM object — defers to its own field guards (ADR-0086 / #3004)', async () => {
+    // A `sys_`/managedBy object carries dedicated write governance (e.g. the
+    // ADR-0086 provenance guard REJECTS a forged managed_by/package_id with 403);
+    // the generic silent strip must not pre-empt that. Proven with both markers.
+    const platformSchema = {
+      name: 'sys_permission_set',
+      fields: { managed_by: { name: 'managed_by', type: 'select', readonly: true } },
+    };
+    const managedSchema = {
+      name: 'crm_thing', managedBy: 'package',
+      fields: { locked: { name: 'locked', type: 'text', readonly: true } },
+    };
+    const inserts: any[] = [];
+    const engine = {
+      registry: { getObject: (n: string) => (n === 'sys_permission_set' ? platformSchema : managedSchema) },
+      insert: vi.fn(async (object: string, data: any) => { inserts.push({ object, data }); return { id: 'x', ...data }; }),
+    };
+    const p = new ObjectStackProtocolImplementation(engine as any);
+    await p.createData({ object: 'sys_permission_set', data: { managed_by: 'package' }, context: { userId: 'u1' } });
+    await p.createData({ object: 'crm_thing', data: { locked: 'forged' }, context: { userId: 'u1' } });
+    expect(inserts[0].data.managed_by, 'sys_ object: readonly field passed through to its guard').toBe('package');
+    expect(inserts[1].data.locked, 'managedBy object: readonly field passed through to its guard').toBe('forged');
+  });
+});
+
+describe('createManyData / batchData — per-row readonly INSERT strip (#3043)', () => {
+  it('createManyData strips the forged readonly column on every row', async () => {
+    const { p, inserts } = makeProtocol();
+    await p.createManyData({
+      object: 'approval_case',
+      records: [
+        { title: 'A', approval_status: 'approved' },
+        { title: 'B', approval_status: 'approved' },
+      ],
+      context: { userId: 'u1' },
+    });
+    expect(inserts[0].data).toEqual([{ title: 'A' }, { title: 'B' }]);
+  });
+
+  it('batchData create strips the forged readonly column', async () => {
+    const { p, inserts } = makeProtocol();
+    await p.batchData({
+      object: 'approval_case',
+      request: { operation: 'create', records: [{ data: { title: 'A', approval_status: 'approved' } }] } as any,
+    });
+    expect(inserts).toHaveLength(1);
+    expect(inserts[0].data).toEqual({ title: 'A' });
+  });
+});

packages/metadata-protocol/src/protocol.ts

@@ -446,6 +446,61 @@ const CLONE_STRIP_FIELDS: readonly string[] = [
     'id', 'created_at', 'created_by', 'updated_at', 'updated_by',
 ];
 
+/**
+ * [#3043] Drop caller-supplied writes to statically `readonly: true` fields from
+ * an INSERT payload, at the external DATA-WRITE INGRESS.
+ *
+ * #2948/#3003 made static `readonly` server-enforced on UPDATE (the engine strips
+ * a non-system caller's write). INSERT was left exempt — but for approval/status
+ * columns that exemption is the SHORTER attack: instead of the #3003
+ * draft-then-PATCH move, a non-system caller can POST a record already
+ * `approval_status: 'approved'` in one step. This closes it symmetrically, but at
+ * the INGRESS rather than in the engine: every EXTERNAL programmatic create — the
+ * REST CRUD route, the GraphQL/MCP dispatcher (`bridge.create` → `callData` →
+ * here), and bulk import — lands in the DataProtocol, while TRUSTED internal
+ * writers (better-auth's adapter, the metadata repository, the seed loader) call
+ * `engine.insert` DIRECTLY and never pass through here. Keeping the strip at the
+ * ingress therefore protects every agent/caller path at once WITHOUT stripping
+ * the internal writers that legitimately seed read-only columns on create
+ * (identity provisioning, provenance stamps, event-log cursors) — the blast
+ * radius an engine-level insert strip would have.
+ *
+ * Silent by contract (like the UPDATE / `readonlyWhen` strips): the forged key is
+ * dropped, the create still succeeds, and the engine re-derives the field's
+ * `defaultValue` (a forged `approval_status` becomes `draft`, the enforced
+ * initial state, not NULL). `isSystem` writes are exempt. `readonlyWhen` stays
+ * INSERT-exempt (a conditional lock needs a prior record, which a create lacks).
+ * Handles a single record or a batch array.
+ *
+ * SCOPE — author-defined business objects only. PLATFORM objects (`managedBy`
+ * set, or the reserved `sys_` namespace) carry their OWN field-write governance
+ * that a silent strip must not pre-empt: e.g. ADR-0086 REJECTS (403) a forged
+ * `managed_by:'package'` / `package_id` on `sys_permission_set`, and #3004
+ * rejects a forged `owner_id` anchor — several of those columns are `readonly`,
+ * so stripping them here would silently swallow the payload the guard is meant to
+ * reject. The #3043 threat is app approval/status/verdict fields (the issue's
+ * `sporadic_application` / `assessment`), never `sys_`; this is the same
+ * platform-vs-authored boundary `applySystemFields` uses for ownership.
+ */
+function stripReadonlyForInsert(schema: any, data: any, context: any): any {
+    if (context?.isSystem) return data;
+    if (!schema || schema.managedBy || String(schema.name ?? '').startsWith('sys_')) return data;
+    const fields = schema?.fields;
+    if (!fields || data == null) return data;
+    const stripRow = (row: any): any => {
+        if (row == null || typeof row !== 'object') return row;
+        let out = row;
+        for (const name of Object.keys(fields)) {
+            if (!fields[name]?.readonly) continue;
+            if (!(name in out)) continue;
+            if (out === row) out = { ...row };
+            delete out[name];
+        }
+        return out;
+    };
+    return Array.isArray(data) ? data.map(stripRow) : stripRow(data);
+}
+
 /**
  * Service Configuration for Discovery
  * Maps service names to their routes and plugin providers.
@@ -2685,9 +2740,16 @@ export class ObjectStackProtocolImplementation implements ObjectStackProtocol {
     }
 
     async createData(request: { object: string, data: any, context?: any }) {
+        // [#3043] Ingress-level static-`readonly` strip — a non-system caller
+        // cannot seed a read-only column (e.g. `approval_status`) on create.
+        const data = stripReadonlyForInsert(
+            this.engine.registry?.getObject(request.object),
+            request.data,
+            request.context,
+        );
         const result = await this.engine.insert(
             request.object,
-            request.data,
+            data,
             request.context !== undefined ? { context: request.context } as any : undefined,
         );
         return {
@@ -2765,7 +2827,13 @@ export class ObjectStackProtocolImplementation implements ObjectStackProtocol {
             Object.assign(data, request.overrides);
         }
 
-        const result = await this.engine.insert(request.object, data, ctxOpt as any);
+        // [#3043] A clone is a create: a non-system caller must not carry over (or
+        // override in) a read-only column — copying the source's `approval_status`
+        // or forging one via `overrides` would mint an approved record. Strip them
+        // so the insert re-derives their `defaultValue`, symmetric with createData.
+        const insertData = stripReadonlyForInsert(schema, data, ctx);
+
+        const result = await this.engine.insert(request.object, insertData, ctxOpt as any);
         return {
             object: request.object,
             id: result.id,
@@ -3101,11 +3169,15 @@ export class ObjectStackProtocolImplementation implements ObjectStackProtocol {
         let succeeded = 0;
         let failed = 0;
 
+        // [#3043] The batch endpoint is an external ingress and threads no
+        // context, so its creates are non-system: strip forged read-only columns.
+        const batchSchema = this.engine.registry?.getObject(object);
+
         for (const record of records) {
             try {
                 switch (operation) {
                     case 'create': {
-                        const created = await this.engine.insert(object, record.data || record);
+                        const created = await this.engine.insert(object, stripReadonlyForInsert(batchSchema, record.data || record, undefined));
                         results.push({ id: created.id, success: true, record: created });
                         succeeded++;
                         break;
@@ -3175,9 +3247,16 @@ export class ObjectStackProtocolImplementation implements ObjectStackProtocol {
     }
 
     async createManyData(request: { object: string, records: any[], context?: any }): Promise<any> {
+        // [#3043] Ingress-level static-`readonly` strip (per row) — mirrors
+        // createData for the bulk-create / import surface.
+        const rows = stripReadonlyForInsert(
+            this.engine.registry?.getObject(request.object),
+            request.records,
+            request.context,
+        );
         const records = await this.engine.insert(
             request.object,
-            request.records,
+            rows,
             request.context !== undefined ? { context: request.context } as any : undefined,
         );
         return {