Skip to content

Commit 9a882c2

Browse files
committed
fix(objectql): enforce static readonly on INSERT too, not just UPDATE (#3043)
#2948/#3003 made static `readonly: true` fields server-enforced on UPDATE (a non-system PATCH forging `approval_status: 'approved'` is silently stripped), but INSERT was deliberately 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. `stripReadonlyFields` now runs on the INSERT path as well, symmetric with the UPDATE strip: - Caller-supplied only: the keys the caller sent are snapshotted BEFORE the security middleware auto-stamps owner_id / organization_id, so those server stamps (and any beforeInsert-hook write to a read-only column) survive. - Before defaults: the strip runs before applyFieldDefaults, so a stripped field falls back to its declared defaultValue (a forged approval_status re-seeds to `draft`, not NULL). - System-context exempt: imports, seed replay, migrations and lifecycle hooks carry isSystem and still seed read-only columns. - Silent (HTTP 2xx), per-row on batch inserts. `readonlyWhen` stays INSERT-exempt (a conditional lock needs a prior record). Behavior change: a non-system INSERT can no longer seed a readonly column from the caller's payload — flows that legitimately write read-only columns at creation (import, migration, programmatic seeding) must run system-context, the same requirement the UPDATE strip already imposed. Contract + proof surfaces updated: field.zod `readonly` describe, spec liveness note, public-form server-managed-fields rationale, authz-conformance matrix row (readonly-static-write), engine #3043 integration suite, and the showcase-static-readonly dogfood proof (insert-forge now stripped end-to-end). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01P5fRY4ctobCeFpBba1oGrz
1 parent 3a18b60 commit 9a882c2

9 files changed

Lines changed: 238 additions & 45 deletions

File tree

Lines changed: 35 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,35 @@
1+
---
2+
"@objectstack/objectql": patch
3+
"@objectstack/spec": patch
4+
---
5+
6+
fix(objectql): enforce static `readonly` on INSERT too, not just UPDATE (#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), but
10+
INSERT was deliberately exempt. For approval/status/verdict columns that exemption
11+
was the *shorter* attack: instead of the #3003 draft-then-PATCH move, a non-system
12+
caller could `POST` a record already `approval_status: 'approved'` in one step —
13+
and the UPDATE-only strip never reached it.
14+
15+
`stripReadonlyFields` now runs on the INSERT path as well, symmetric with the
16+
UPDATE strip:
17+
18+
- **Caller-supplied only.** The keys the caller sent are snapshotted *before* the
19+
security middleware auto-stamps `owner_id` / `organization_id`, so those server
20+
stamps — and any `beforeInsert`-hook write to a read-only column — survive; only
21+
a forged read-only key the caller actually sent is dropped.
22+
- **Before defaults.** The strip runs before `applyFieldDefaults`, so a stripped
23+
field falls back to its declared `defaultValue` (a forged `approval_status`
24+
re-seeds to `draft`, the enforced initial state, not NULL).
25+
- **System-context exempt.** Imports, seed replay, migrations and lifecycle hooks
26+
carry `isSystem` and still seed read-only columns.
27+
- **Silent** (HTTP 2xx), matching the UPDATE and `readonlyWhen` strips. Batch
28+
inserts are stripped per row. `readonlyWhen` stays INSERT-exempt (a conditional
29+
lock needs a prior record, which an insert has none of).
30+
31+
Behavior change: a non-system INSERT can no longer seed a `readonly` column from
32+
the caller's payload. Flows that legitimately need to write read-only columns at
33+
creation (data import, migration, programmatic seeding) must run with a
34+
system-context (`isSystem`) — the same requirement the UPDATE strip already
35+
imposed.

packages/objectql/src/engine.ts

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2195,6 +2195,15 @@ export class ObjectQL implements IDataEngine {
21952195
context: options?.context,
21962196
};
21972197

2198+
// [#3043] Snapshot the keys the CALLER supplied per row, BEFORE the security
2199+
// middleware auto-stamps server-managed columns (owner_id / organization_id)
2200+
// into `opCtx.data`. The static-`readonly` INSERT strip below (symmetric
2201+
// with the #2948 UPDATE strip) drops only caller-supplied read-only writes,
2202+
// so those middleware / hook stamps — never in this set — survive.
2203+
const suppliedKeysPerRow: ReadonlySet<string>[] = (
2204+
Array.isArray(data) ? (data as Record<string, unknown>[]) : [data as Record<string, unknown>]
2205+
).map((row) => new Set(Object.keys((row ?? {}) as Record<string, unknown>)));
2206+
21982207
await this.executeWithMiddleware(opCtx, async () => {
21992208
// Resolve field `defaultValue`s (including the `current_user` token)
22002209
// BEFORE the beforeInsert hook runs, so a hook that DERIVES one field
@@ -2204,6 +2213,39 @@ export class ObjectQL implements IDataEngine {
22042213
// fills fields left `undefined`, so client-supplied values are untouched.
22052214
const nowSnap = new Date();
22062215
const isBatch = Array.isArray(opCtx.data);
2216+
2217+
// [#3043] Enforce STATIC `readonly` on the INSERT write path for non-system
2218+
// callers — symmetric with the #2948 UPDATE strip, and closing a step
2219+
// SHORTER attack than #3003: a create could directly SEED a readonly column
2220+
// (`approval_status: 'approved'`, a forged audit stamp, a squatted id) in a
2221+
// single POST, which the UPDATE-only strip never reached (it acts on UPDATE).
2222+
// We drop caller-supplied writes to statically-`readonly` fields. Two things
2223+
// keep every legitimate write intact:
2224+
// - `suppliedKeysPerRow` (captured pre-middleware) — only keys the CALLER
2225+
// sent are candidates, so the security middleware's owner/tenant stamps
2226+
// and any beforeInsert-hook server stamp survive.
2227+
// - system context — import replay, migrations and lifecycle writes carry
2228+
// `isSystem` and legitimately seed read-only columns, so they skip it.
2229+
// Runs BEFORE `applyFieldDefaults` so a stripped field falls back to its
2230+
// `defaultValue` (a forged `approval_status` re-seeds to `draft`, not NULL)
2231+
// — the enforced initial state, not the attacker's — and before the
2232+
// beforeInsert hook, which may still stamp read-only columns.
2233+
// `readonlyWhen` stays INSERT-exempt (a conditional lock needs a prior
2234+
// record, which insert has none of); only static `readonly` is enforced here.
2235+
if (!opCtx.context?.isSystem) {
2236+
const insertSchema = this._registry.getObject(object);
2237+
opCtx.data = isBatch
2238+
? (opCtx.data as Record<string, unknown>[]).map((row, i) =>
2239+
stripReadonlyFields(insertSchema as any, row, suppliedKeysPerRow[i], this.logger),
2240+
)
2241+
: stripReadonlyFields(
2242+
insertSchema as any,
2243+
opCtx.data as Record<string, unknown>,
2244+
suppliedKeysPerRow[0],
2245+
this.logger,
2246+
);
2247+
}
2248+
22072249
const defaultedData = isBatch
22082250
? (opCtx.data as any[]).map((row) =>
22092251
this.applyFieldDefaults(object, row as Record<string, unknown>, opCtx.context, nowSnap),

packages/objectql/src/plugin.integration.test.ts

Lines changed: 97 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1380,6 +1380,103 @@ describe('ObjectQLPlugin - Metadata Service Integration', () => {
13801380
});
13811381
});
13821382

1383+
// #3043 — static `readonly:true` fields must not be SEEDABLE via a NON-system
1384+
// INSERT either. Symmetric with the #2948 UPDATE strip: a create could
1385+
// otherwise POST a record already `approval_status:'approved'` — a step
1386+
// SHORTER than the #3003 draft→PATCH move, and one the UPDATE strip never
1387+
// reached. The strip runs BEFORE defaults (a stripped field re-seeds to its
1388+
// `defaultValue`) and captures caller keys BEFORE middleware, so server stamps
1389+
// (a beforeInsert-hook write, owner/tenant injection) survive; system exempt.
1390+
describe('Static readonly seed enforcement on INSERT (#3043)', () => {
1391+
async function bootWithInsertCapture() {
1392+
const inserts: Record<string, any>[] = [];
1393+
const mockDriver = {
1394+
name: 'ro-ins-capture', version: '1.0.0',
1395+
connect: async () => {}, disconnect: async () => {},
1396+
find: async () => [], findOne: async () => null,
1397+
create: async (_o: string, d: any) => { inserts.push({ ...d }); return { id: 'rec-1', ...d }; },
1398+
bulkCreate: async (_o: string, rows: any[]) => {
1399+
rows.forEach((r) => inserts.push({ ...r }));
1400+
return rows.map((r, i) => ({ id: `rec-${i + 1}`, ...r }));
1401+
},
1402+
update: async (_o: string, _i: any, d: any) => ({ id: _i, ...d }),
1403+
delete: async () => true, syncSchema: async () => {},
1404+
};
1405+
await kernel.use({
1406+
name: 'ro-ins-capture-plugin', type: 'driver', version: '1.0.0',
1407+
init: async (ctx) => { ctx.registerService('driver.ro-ins-capture', mockDriver); },
1408+
});
1409+
await kernel.use(new ObjectQLPlugin());
1410+
await kernel.bootstrap();
1411+
const objectql = kernel.getService('objectql') as any;
1412+
const obj: ObjectSchema = {
1413+
name: 'ro_ins_obj', label: 'RO Ins Obj', datasource: 'ro-ins-capture',
1414+
fields: {
1415+
name: { name: 'name', label: 'Name', type: 'text' },
1416+
// an approval/status column: readonly, with an enforced initial state
1417+
approval_status: { name: 'approval_status', label: 'Approval Status', type: 'text', readonly: true, defaultValue: 'draft' } as any,
1418+
// a provenance stamp a beforeInsert hook fills — readonly, no default
1419+
source: { name: 'source', label: 'Source', type: 'text', readonly: true } as any,
1420+
},
1421+
};
1422+
objectql.registry.registerObject(obj, 'test', 'test');
1423+
return { objectql, inserts };
1424+
}
1425+
1426+
it('drops a user-context seed of a static readonly field, re-seeding its defaultValue; editable sibling lands', async () => {
1427+
const { objectql, inserts } = await bootWithInsertCapture();
1428+
await objectql.insert(
1429+
'ro_ins_obj',
1430+
{ name: 'Probe', approval_status: 'approved' },
1431+
{ context: { userId: 'user-9', tenantId: 'org-1' } },
1432+
);
1433+
expect(inserts.length).toBe(1);
1434+
const data = inserts[0];
1435+
expect(data.name).toBe('Probe'); // editable field written
1436+
expect(data.approval_status).toBe('draft'); // forged 'approved' stripped, default re-seeded
1437+
});
1438+
1439+
it('ALLOWS a system-context seed of the same static readonly field', async () => {
1440+
const { objectql, inserts } = await bootWithInsertCapture();
1441+
await objectql.insert(
1442+
'ro_ins_obj',
1443+
{ name: 'Seed', approval_status: 'approved' },
1444+
{ context: { isSystem: true } },
1445+
);
1446+
expect(inserts.length).toBe(1);
1447+
expect(inserts[0].approval_status).toBe('approved'); // system seed honored (import/migration)
1448+
});
1449+
1450+
it('keeps a beforeInsert-hook write to a readonly field (server stamp, not caller-supplied)', async () => {
1451+
const { objectql, inserts } = await bootWithInsertCapture();
1452+
objectql.registerHook('beforeInsert', async (ctx: any) => {
1453+
ctx.input.data.source = 'system-hook';
1454+
}, { object: 'ro_ins_obj' });
1455+
await objectql.insert(
1456+
'ro_ins_obj',
1457+
{ name: 'Probe', source: 'attacker' }, // caller forges the readonly stamp
1458+
{ context: { userId: 'user-9' } },
1459+
);
1460+
expect(inserts.length).toBe(1);
1461+
expect(inserts[0].source).toBe('system-hook'); // caller forge stripped, hook stamp survives
1462+
});
1463+
1464+
it('strips per-row on a BATCH insert', async () => {
1465+
const { objectql, inserts } = await bootWithInsertCapture();
1466+
await objectql.insert(
1467+
'ro_ins_obj',
1468+
[
1469+
{ name: 'A', approval_status: 'approved' },
1470+
{ name: 'B', approval_status: 'approved' },
1471+
],
1472+
{ context: { userId: 'user-9' } },
1473+
);
1474+
expect(inserts.length).toBe(2);
1475+
expect(inserts.map((r) => r.approval_status)).toEqual(['draft', 'draft']);
1476+
expect(inserts.map((r) => r.name)).toEqual(['A', 'B']);
1477+
});
1478+
});
1479+
13831480
// #3042 — conditional `readonlyWhen` must be enforced on the BULK
13841481
// (updateMany) path too, not only the single-id path. The bulk strip reads
13851482
// the matched rows' prior state and drops a field locked in ≥1 of them.

packages/objectql/src/validation/rule-validator.ts

Lines changed: 18 additions & 12 deletions
Original file line numberDiff line numberDiff line change
@@ -281,21 +281,27 @@ export function stripReadonlyWhenFieldsMulti(
281281
}
282282

283283
/**
284-
* Strip CALLER-SUPPLIED writes to statically `readonly: true` fields from an
285-
* UPDATE payload (#2948). Unlike `readonlyWhen` (conditional, handled above), a
286-
* static `readonly` field was never enforced on the server write path: the
287-
* record validator only SKIPS it from validation, so a user-context update
288-
* could overwrite audit stamps, provenance, or any other read-only column. We
289-
* STRIP the change (symmetric with `readonlyWhen`) rather than reject it, for
290-
* compatibility.
284+
* Strip CALLER-SUPPLIED writes to statically `readonly: true` fields from a
285+
* write payload. Shared by BOTH server write paths: UPDATE (#2948) and INSERT
286+
* (#3043) — a create could otherwise directly SEED a readonly column
287+
* (`approval_status: 'approved'`, a forged audit stamp, a squatted id) a step
288+
* shorter than the #3003 draft-then-PATCH move. Unlike `readonlyWhen`
289+
* (conditional, handled above), a static `readonly` field was never enforced on
290+
* the server write path: the record validator only SKIPS it from validation, so
291+
* a user-context write could overwrite audit stamps, provenance, or any other
292+
* read-only column. We STRIP the change (symmetric with `readonlyWhen`) rather
293+
* than reject it, for compatibility.
291294
*
292295
* Two guards keep every legitimate write intact:
293296
* - `suppliedKeys` — only keys the CALLER sent are candidates. Server stamps
294-
* applied by beforeUpdate hooks or write middleware (e.g. `updated_by` /
295-
* `updated_at`, plugin.ts) land in `data` but are NOT in `suppliedKeys`, so
296-
* they survive. A caller that *explicitly* forges e.g. `updated_by` simply
297-
* has it dropped for that request (the last-modified stamp is left unchanged
298-
* — safe).
297+
* applied by before-hooks or write middleware (e.g. `updated_by` /
298+
* `updated_at` on update, `owner_id` / `organization_id` on insert) land in
299+
* `data` but are NOT in `suppliedKeys`, so they survive. A caller that
300+
* *explicitly* forges e.g. `updated_by` simply has it dropped for that
301+
* request (the server-managed value is left to stand — safe). On INSERT the
302+
* caller keys are snapshotted before the security middleware stamps, and the
303+
* strip runs before `defaultValue` resolution so a stripped field falls back
304+
* to its declared default rather than NULL.
299305
* - system context — the caller passes this strip only for NON-system writes;
300306
* system-context writes (import, seed replay, approvals, lifecycle hooks —
301307
* all `isSystem: true`) legitimately set read-only columns and skip it.

packages/qa/dogfood/test/authz-conformance.matrix.ts

Lines changed: 3 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -91,10 +91,10 @@ export const AUTHZ_CONFORMANCE: AuthzPrimitive[] = [
9191
note: 'Surface posture: system (trusted-implicit), pre-wiring — no end-user transport exists (handleUpgrade unimplemented, no REST subscribe route, client RealtimeAPI is a placeholder); the only subscribers are server-internal plugins (webhook auto-enqueuer, knowledge sync). Structural defect: Subscription carries no principal, matchesSubscription filters only by object+eventTypes (RealtimeSubscriptionOptions.filter is declared but never read), and the engine publishes the FULL after-row — so any future external subscriber would receive record bodies cross-tenant that its own find would hide. ADMISSION REQUIREMENT before any WebSocket/SSE/subscribe transport ships: per-recipient RLS/FLS/tenant re-check on delivery (subscription carries the subscriber ExecutionContext) OR id-only payload + client re-fetch. The transport tripwire probes in authz-conformance.test.ts turn a wired transport into an UNCLASSIFIED surface → red CI until this row is upgraded with the enforcement site.' },
9292
{ id: 'default-profile', summary: 'app-declared default profile (isDefault)', state: 'enforced',
9393
enforcement: 'plugin-security/security-plugin.ts fallback resolution', proof: 'showcase-default-profile.dogfood.test.ts' },
94-
{ id: 'readonly-static-write', summary: 'static `readonly: true` stripped from non-system UPDATE payloads (#2948 / #3003a direct PATCH cannot forge approval/status/amount columns the UI never renders)', state: 'enforced',
95-
enforcement: 'objectql/engine.ts update — stripReadonlyFields on both the single-id and multi-row paths (caller-supplied keys only, so audit-hook/middleware server stamps survive; isSystem exempt; symmetric with the readonlyWhen strip)',
94+
{ id: 'readonly-static-write', summary: 'static `readonly: true` stripped from non-system UPDATE (#2948 / #3003) AND INSERT (#3043) payloads — neither a direct PATCH nor a direct POST can forge approval/status/amount columns the UI never renders', state: 'enforced',
95+
enforcement: 'objectql/engine.ts — stripReadonlyFields on the update single-id + multi-row paths (#2948) and on the insert path (#3043, before applyFieldDefaults so a stripped field re-seeds to its defaultValue); caller-supplied keys only (insert snapshots them before the security middleware stamps owner/tenant), so server stamps survive; isSystem exempt; symmetric with the readonlyWhen strip',
9696
proof: 'showcase-static-readonly.dogfood.test.ts',
97-
note: 'The #3003 field report: `readonly: true` used to be UI-only, so a logged-in non-admin self-approved a 4-stage approval (approval_status/approval_stage/confirmed_total) with one same-session REST PATCH on a draft record — RECORD_LOCKED only guards pending flows, and the draft never entered one. The strip is SILENT (HTTP 200, persisted value kept — reject-vs-strip decided in #2948 for readonlyWhen symmetry). INSERT is deliberately exempt (create may seed a readonly column: defaultValue, import, migration), also symmetric with readonlyWhen. Engine-level unit/integration proof in objectql/plugin.integration.test.ts (#2948 suite: forge stripped, server stamp survives, system context allowed).' },
97+
note: 'The #3003 field report: `readonly: true` used to be UI-only, so a logged-in non-admin self-approved a 4-stage approval (approval_status/approval_stage/confirmed_total) with one same-session REST PATCH on a draft record — RECORD_LOCKED only guards pending flows, and the draft never entered one. #3043 is the INSERT face: the same non-admin could skip the draft entirely and POST a record already `approval_status:"approved"` — a step SHORTER than #3003, and one the UPDATE strip never reached. The strip is SILENT on both paths (HTTP 2xx, forged value dropped; a stripped INSERT field falls back to its defaultValue). `readonlyWhen` stays INSERT-exempt (a conditional lock needs a prior record). System-context writes (import, seed replay, migration) still seed readonly columns. Engine-level unit/integration proof in objectql/plugin.integration.test.ts (#2948 UPDATE suite + #3043 INSERT suite: forge stripped, default re-seeded, server stamp survives, system context allowed, batch rows covered).' },
9898

9999
// ── ADR-0057 — ERP authorization core (enforced + e2e proven) ──────────
100100
{ id: 'scope-depth', summary: 'permission-grant access DEPTH (own/own_and_reports/unit/unit_and_below/org)', state: 'enforced',

0 commit comments

Comments
 (0)