Skip to content

Commit beaf2de

Browse files
os-zhuangclaude
andauthored
fix(metadata-protocol): strip static readonly on INSERT at the data-write ingress (#3043) (#3162)
#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 had. - Caller-forged only: at the ingress the payload is raw caller input (owner/tenant stamps land later inside engine.insert), so only keys the caller sent are dropped. - 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; 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 — ADR-0086 REJECTS (403) a forged managed_by/package_id on sys_permission_set, #3004 rejects a forged owner_id; several of those columns are readonly. 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 on an author-defined object. Flows that legitimately write read-only columns at creation must run system-context, the same requirement the UPDATE strip already imposes. Proof: metadata-protocol protocol.readonly-insert.test.ts (forge stripped, default re-derived, system exempt, batch per-row, platform objects deferred to their guards) + the showcase-static-readonly dogfood test (insert-forge stripped e2e over HTTP). Contract surfaces updated: field.zod readonly describe (+regenerated reference), spec liveness note, authz-conformance matrix row. Claude-Session: https://claude.ai/code/session_01P5fRY4ctobCeFpBba1oGrz Co-authored-by: Claude <noreply@anthropic.com>
1 parent e057f42 commit beaf2de

8 files changed

Lines changed: 290 additions & 32 deletions

File tree

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

content/docs/references/data/field.mdx

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -150,7 +150,7 @@ const result = Address.parse(data);
150150
| **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`. |
151151
| **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". |
152152
| **hidden** | `boolean` | optional | Hidden from default UI |
153-
| **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). |
153+
| **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. |
154154
| **requiredPermissions** | `string[]` | optional | [ADR-0066 D3] Capabilities required to read/edit this field (mask on read, deny on write; AND-gate). |
155155
| **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. |
156156
| **sortable** | `boolean` | optional | Whether field is sortable in list views |
Lines changed: 117 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,117 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
//
3+
// #3043 — static `readonly: true` fields must not be SEEDABLE via a non-system
4+
// create through the external data API. The strip lives at the DataProtocol
5+
// ingress (createData / createManyData / batchData / cloneData) — the seam every
6+
// external REST/GraphQL/MCP create funnels through — while trusted internal
7+
// writers call engine.insert directly and are unaffected. It runs BEFORE
8+
// engine.insert, so a stripped field falls back to its defaultValue (re-derived
9+
// by the engine, which the mock stands in for). System context is exempt.
10+
11+
import { describe, it, expect, vi } from 'vitest';
12+
import { ObjectStackProtocolImplementation } from './protocol.js';
13+
14+
const SCHEMA = {
15+
name: 'approval_case',
16+
fields: {
17+
title: { name: 'title', type: 'text' },
18+
// readonly approval column — the #3003 attack target
19+
approval_status: { name: 'approval_status', type: 'text', readonly: true, defaultValue: 'draft' },
20+
// readonly provenance stamp with no default
21+
source: { name: 'source', type: 'text', readonly: true },
22+
},
23+
};
24+
25+
function makeProtocol() {
26+
const inserts: Array<{ object: string; data: any; options: any }> = [];
27+
const engine = {
28+
registry: { getObject: (n: string) => (n === 'approval_case' ? SCHEMA : undefined) },
29+
insert: vi.fn(async (object: string, data: any, options?: any) => {
30+
inserts.push({ object, data, options });
31+
const rows = Array.isArray(data) ? data : [data];
32+
const out = rows.map((r, i) => ({ id: `rec-${i + 1}`, ...r }));
33+
return Array.isArray(data) ? out : out[0];
34+
}),
35+
};
36+
const p = new ObjectStackProtocolImplementation(engine as any);
37+
return { p, engine, inserts };
38+
}
39+
40+
describe('createData — static readonly INSERT strip (#3043)', () => {
41+
it('drops a non-system caller forging a readonly field; editable sibling lands', async () => {
42+
const { p, inserts } = makeProtocol();
43+
await p.createData({
44+
object: 'approval_case',
45+
data: { title: 'Case A', approval_status: 'approved' },
46+
context: { userId: 'u1' },
47+
});
48+
expect(inserts).toHaveLength(1);
49+
expect(inserts[0].data).toEqual({ title: 'Case A' }); // approval_status stripped
50+
expect(inserts[0].data).not.toHaveProperty('approval_status');
51+
});
52+
53+
it('ALLOWS a system-context caller to seed the readonly field', async () => {
54+
const { p, inserts } = makeProtocol();
55+
await p.createData({
56+
object: 'approval_case',
57+
data: { title: 'Seed', approval_status: 'approved' },
58+
context: { isSystem: true },
59+
});
60+
expect(inserts[0].data.approval_status).toBe('approved');
61+
});
62+
63+
it('strips a forged readonly field even when no context is supplied (non-system default)', async () => {
64+
const { p, inserts } = makeProtocol();
65+
await p.createData({ object: 'approval_case', data: { title: 'X', source: 'attacker' } });
66+
expect(inserts[0].data).not.toHaveProperty('source');
67+
});
68+
69+
it('does NOT strip a PLATFORM object — defers to its own field guards (ADR-0086 / #3004)', async () => {
70+
// A `sys_`/managedBy object carries dedicated write governance (e.g. the
71+
// ADR-0086 provenance guard REJECTS a forged managed_by/package_id with 403);
72+
// the generic silent strip must not pre-empt that. Proven with both markers.
73+
const platformSchema = {
74+
name: 'sys_permission_set',
75+
fields: { managed_by: { name: 'managed_by', type: 'select', readonly: true } },
76+
};
77+
const managedSchema = {
78+
name: 'crm_thing', managedBy: 'package',
79+
fields: { locked: { name: 'locked', type: 'text', readonly: true } },
80+
};
81+
const inserts: any[] = [];
82+
const engine = {
83+
registry: { getObject: (n: string) => (n === 'sys_permission_set' ? platformSchema : managedSchema) },
84+
insert: vi.fn(async (object: string, data: any) => { inserts.push({ object, data }); return { id: 'x', ...data }; }),
85+
};
86+
const p = new ObjectStackProtocolImplementation(engine as any);
87+
await p.createData({ object: 'sys_permission_set', data: { managed_by: 'package' }, context: { userId: 'u1' } });
88+
await p.createData({ object: 'crm_thing', data: { locked: 'forged' }, context: { userId: 'u1' } });
89+
expect(inserts[0].data.managed_by, 'sys_ object: readonly field passed through to its guard').toBe('package');
90+
expect(inserts[1].data.locked, 'managedBy object: readonly field passed through to its guard').toBe('forged');
91+
});
92+
});
93+
94+
describe('createManyData / batchData — per-row readonly INSERT strip (#3043)', () => {
95+
it('createManyData strips the forged readonly column on every row', async () => {
96+
const { p, inserts } = makeProtocol();
97+
await p.createManyData({
98+
object: 'approval_case',
99+
records: [
100+
{ title: 'A', approval_status: 'approved' },
101+
{ title: 'B', approval_status: 'approved' },
102+
],
103+
context: { userId: 'u1' },
104+
});
105+
expect(inserts[0].data).toEqual([{ title: 'A' }, { title: 'B' }]);
106+
});
107+
108+
it('batchData create strips the forged readonly column', async () => {
109+
const { p, inserts } = makeProtocol();
110+
await p.batchData({
111+
object: 'approval_case',
112+
request: { operation: 'create', records: [{ data: { title: 'A', approval_status: 'approved' } }] } as any,
113+
});
114+
expect(inserts).toHaveLength(1);
115+
expect(inserts[0].data).toEqual({ title: 'A' });
116+
});
117+
});

packages/metadata-protocol/src/protocol.ts

Lines changed: 83 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -446,6 +446,61 @@ const CLONE_STRIP_FIELDS: readonly string[] = [
446446
'id', 'created_at', 'created_by', 'updated_at', 'updated_by',
447447
];
448448

449+
/**
450+
* [#3043] Drop caller-supplied writes to statically `readonly: true` fields from
451+
* an INSERT payload, at the external DATA-WRITE INGRESS.
452+
*
453+
* #2948/#3003 made static `readonly` server-enforced on UPDATE (the engine strips
454+
* a non-system caller's write). INSERT was left exempt — but for approval/status
455+
* columns that exemption is the SHORTER attack: instead of the #3003
456+
* draft-then-PATCH move, a non-system caller can POST a record already
457+
* `approval_status: 'approved'` in one step. This closes it symmetrically, but at
458+
* the INGRESS rather than in the engine: every EXTERNAL programmatic create — the
459+
* REST CRUD route, the GraphQL/MCP dispatcher (`bridge.create` → `callData` →
460+
* here), and bulk import — lands in the DataProtocol, while TRUSTED internal
461+
* writers (better-auth's adapter, the metadata repository, the seed loader) call
462+
* `engine.insert` DIRECTLY and never pass through here. Keeping the strip at the
463+
* ingress therefore protects every agent/caller path at once WITHOUT stripping
464+
* the internal writers that legitimately seed read-only columns on create
465+
* (identity provisioning, provenance stamps, event-log cursors) — the blast
466+
* radius an engine-level insert strip would have.
467+
*
468+
* Silent by contract (like the UPDATE / `readonlyWhen` strips): the forged key is
469+
* dropped, the create still succeeds, and the engine re-derives the field's
470+
* `defaultValue` (a forged `approval_status` becomes `draft`, the enforced
471+
* initial state, not NULL). `isSystem` writes are exempt. `readonlyWhen` stays
472+
* INSERT-exempt (a conditional lock needs a prior record, which a create lacks).
473+
* Handles a single record or a batch array.
474+
*
475+
* SCOPE — author-defined business objects only. PLATFORM objects (`managedBy`
476+
* set, or the reserved `sys_` namespace) carry their OWN field-write governance
477+
* that a silent strip must not pre-empt: e.g. ADR-0086 REJECTS (403) a forged
478+
* `managed_by:'package'` / `package_id` on `sys_permission_set`, and #3004
479+
* rejects a forged `owner_id` anchor — several of those columns are `readonly`,
480+
* so stripping them here would silently swallow the payload the guard is meant to
481+
* reject. The #3043 threat is app approval/status/verdict fields (the issue's
482+
* `sporadic_application` / `assessment`), never `sys_`; this is the same
483+
* platform-vs-authored boundary `applySystemFields` uses for ownership.
484+
*/
485+
function stripReadonlyForInsert(schema: any, data: any, context: any): any {
486+
if (context?.isSystem) return data;
487+
if (!schema || schema.managedBy || String(schema.name ?? '').startsWith('sys_')) return data;
488+
const fields = schema?.fields;
489+
if (!fields || data == null) return data;
490+
const stripRow = (row: any): any => {
491+
if (row == null || typeof row !== 'object') return row;
492+
let out = row;
493+
for (const name of Object.keys(fields)) {
494+
if (!fields[name]?.readonly) continue;
495+
if (!(name in out)) continue;
496+
if (out === row) out = { ...row };
497+
delete out[name];
498+
}
499+
return out;
500+
};
501+
return Array.isArray(data) ? data.map(stripRow) : stripRow(data);
502+
}
503+
449504
/**
450505
* Service Configuration for Discovery
451506
* Maps service names to their routes and plugin providers.
@@ -2685,9 +2740,16 @@ export class ObjectStackProtocolImplementation implements ObjectStackProtocol {
26852740
}
26862741

26872742
async createData(request: { object: string, data: any, context?: any }) {
2743+
// [#3043] Ingress-level static-`readonly` strip — a non-system caller
2744+
// cannot seed a read-only column (e.g. `approval_status`) on create.
2745+
const data = stripReadonlyForInsert(
2746+
this.engine.registry?.getObject(request.object),
2747+
request.data,
2748+
request.context,
2749+
);
26882750
const result = await this.engine.insert(
26892751
request.object,
2690-
request.data,
2752+
data,
26912753
request.context !== undefined ? { context: request.context } as any : undefined,
26922754
);
26932755
return {
@@ -2765,7 +2827,13 @@ export class ObjectStackProtocolImplementation implements ObjectStackProtocol {
27652827
Object.assign(data, request.overrides);
27662828
}
27672829

2768-
const result = await this.engine.insert(request.object, data, ctxOpt as any);
2830+
// [#3043] A clone is a create: a non-system caller must not carry over (or
2831+
// override in) a read-only column — copying the source's `approval_status`
2832+
// or forging one via `overrides` would mint an approved record. Strip them
2833+
// so the insert re-derives their `defaultValue`, symmetric with createData.
2834+
const insertData = stripReadonlyForInsert(schema, data, ctx);
2835+
2836+
const result = await this.engine.insert(request.object, insertData, ctxOpt as any);
27692837
return {
27702838
object: request.object,
27712839
id: result.id,
@@ -3101,11 +3169,15 @@ export class ObjectStackProtocolImplementation implements ObjectStackProtocol {
31013169
let succeeded = 0;
31023170
let failed = 0;
31033171

3172+
// [#3043] The batch endpoint is an external ingress and threads no
3173+
// context, so its creates are non-system: strip forged read-only columns.
3174+
const batchSchema = this.engine.registry?.getObject(object);
3175+
31043176
for (const record of records) {
31053177
try {
31063178
switch (operation) {
31073179
case 'create': {
3108-
const created = await this.engine.insert(object, record.data || record);
3180+
const created = await this.engine.insert(object, stripReadonlyForInsert(batchSchema, record.data || record, undefined));
31093181
results.push({ id: created.id, success: true, record: created });
31103182
succeeded++;
31113183
break;
@@ -3175,9 +3247,16 @@ export class ObjectStackProtocolImplementation implements ObjectStackProtocol {
31753247
}
31763248

31773249
async createManyData(request: { object: string, records: any[], context?: any }): Promise<any> {
3250+
// [#3043] Ingress-level static-`readonly` strip (per row) — mirrors
3251+
// createData for the bulk-create / import surface.
3252+
const rows = stripReadonlyForInsert(
3253+
this.engine.registry?.getObject(request.object),
3254+
request.records,
3255+
request.context,
3256+
);
31783257
const records = await this.engine.insert(
31793258
request.object,
3180-
request.records,
3259+
rows,
31813260
request.context !== undefined ? { context: request.context } as any : undefined,
31823261
);
31833262
return {

0 commit comments

Comments
 (0)