From 6c441b0ead71e56f419b9215644a13e69fd12ded Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 1 Aug 2026 13:39:21 +0000 Subject: [PATCH 1/8] wip: rescue in-progress work before container restart loss --- .../src/engine-audit-anchor-write.test.ts | 232 ++++++++++++++++++ 1 file changed, 232 insertions(+) create mode 100644 packages/objectql/src/engine-audit-anchor-write.test.ts diff --git a/packages/objectql/src/engine-audit-anchor-write.test.ts b/packages/objectql/src/engine-audit-anchor-write.test.ts new file mode 100644 index 0000000000..b1eab25b0a --- /dev/null +++ b/packages/objectql/src/engine-audit-anchor-write.test.ts @@ -0,0 +1,232 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#4447] `created_at` is engine-owned: a client-supplied value on an ordinary + * write is DROPPED, not persisted. + * + * Reproduction harness: a REAL {@link ObjectQL} engine over a minimal in-memory + * driver whose `update` lets incoming data win (`{...cur, ...data}`) — the same + * shape driver-sql has, which is why a value that survives the engine's strip + * reaches the row. + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import { ObjectQL } from './engine.js'; + +const taskObject = { + name: 'audit_task', + label: 'Task', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const, primaryKey: true }, + title: { name: 'title', label: 'Title', type: 'text' as const }, + progress: { name: 'progress', label: 'Progress', type: 'number' as const }, + }, +}; + +function makeMemoryDriver() { + const stores = new Map>>(); + const storeFor = (obj: string) => { + let s = stores.get(obj); + if (!s) { s = new Map(); stores.set(obj, s); } + return s; + }; + let nextId = 0; + const matchesWhere = (row: Record, where: any): boolean => { + if (!where || typeof where !== 'object') return true; + for (const [k, v] of Object.entries(where)) { + if (k.startsWith('$')) continue; + const expected = (v && typeof v === 'object' && '$eq' in (v as any)) ? (v as any).$eq : v; + if ((row[k] ?? null) !== (expected ?? null)) return false; + } + return true; + }; + const driver: any = { + name: 'memory', version: '0.0.0', supports: {} as any, + 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) => matchesWhere(r, ast?.where)); + }, + findStream() { throw new Error('not implemented'); }, + async findOne(object: string, ast: any) { + for (const r of storeFor(object).values()) if (matchesWhere(r, ast?.where)) return r; + return null; + }, + async create(object: string, data: Record) { + nextId += 1; + const id = (data.id as string) ?? `r_${nextId}`; + // driver-sql's `stampInsertTimestamps`: fills the audit timestamps ONLY + // when absent, so a supplied `created_at` survives the driver. That is + // why the insert-side protection has to be the engine's strip. + const row: Record = { ...data, id }; + const iso = new Date(Date.now() - 86_400_000).toISOString(); + if (row.created_at == null) row.created_at = iso; + if (row.updated_at == null) row.updated_at = iso; + storeFor(object).set(id, row); + return row; + }, + async update(object: string, id: string, data: Record) { + const s = storeFor(object); + const cur = s.get(id); + if (!cur) return null; + // driver-sql's `update`: incoming data wins, and it force-advances + // `updated_at` but never touches `created_at`. So anything the engine + // failed to strip off `created_at` lands on the row — which is exactly + // why `updated_at` LOOKED protected while the anchor did not. + const updated = { ...cur, ...data, id, updated_at: new Date().toISOString() }; + s.set(id, updated); + return updated; + }, + async upsert(object: string, data: Record) { + const id = data.id as string | undefined; + if (id && storeFor(object).has(id)) return this.update(object, id, data); + return this.create(object, data); + }, + async delete(object: string, id: string) { return storeFor(object).delete(id); }, + async count(object: string, ast: any) { return (await this.find(object, ast)).length; }, + async bulkCreate(object: string, rows: Record[]) { + return Promise.all(rows.map((r) => this.create(object, r))); + }, + async bulkUpdate() { return []; }, + async bulkDelete() {}, + async updateMany(object: string, ast: any, data: Record) { + const rows = await this.find(object, ast); + for (const r of rows) storeFor(object).set(r.id as string, { ...r, ...data, id: r.id }); + return rows.length; + }, + async beginTransaction() { return { commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, async rollback() {}, + }; + return { driver, stores }; +} + +const FORGED = '1999-01-01T00:00:00.000Z'; + +describe('[#4447] created_at is engine-owned on an ordinary write', () => { + let engine: ObjectQL; + + beforeEach(async () => { + engine = new ObjectQL(); + const { driver } = makeMemoryDriver(); + engine.registerDriver(driver, true); + await engine.init(); + engine.registry.registerObject(taskObject as any); + }); + + /** An ordinary authenticated caller — NOT `isSystem`, no `preserveAudit`. */ + const userCtx = { userId: 'u1' }; + + async function seed() { + return engine.insert('audit_task', { title: 'T', progress: 1 }, { context: userCtx } as any); + } + + it('the injected field metadata marks it read-only and system', () => { + // The premise: if this is ever `readonly: false`, the strip has nothing to + // key off and every assertion below becomes vacuous. + const schema: any = engine.registry.getObject('audit_task'); + expect(schema.fields.created_at).toMatchObject({ readonly: true, system: true }); + }); + + it('a client-supplied created_at on UPDATE is dropped, not persisted', async () => { + const row: any = await seed(); + const realCreatedAt = row.created_at; + expect(realCreatedAt).toBeTruthy(); + + await engine.update( + 'audit_task', + { progress: 42, created_at: FORGED }, + { where: { id: row.id }, context: userCtx } as any, + ); + + const after: any = await engine.findOne('audit_task', { where: { id: row.id } } as any); + // The legitimate half of the same request still lands. + expect(after.progress).toBe(42); + // The audit anchor does not move. + expect(after.created_at).toBe(realCreatedAt); + expect(after.created_at).not.toBe(FORGED); + }); + + it('reports the drop through onFieldsDropped, so the caller is not left guessing', async () => { + const row: any = await seed(); + const dropped: any[] = []; + await engine.update( + 'audit_task', + { progress: 7, created_at: FORGED }, + { + where: { id: row.id }, + context: userCtx, + onFieldsDropped: (e: any) => dropped.push(e), + } as any, + ); + // #3794's `droppedFields` contract: this is exactly the case the key exists + // for, and it had no live producer on the audit trio before this fix. + expect(dropped.flatMap((e) => e.fields)).toContain('created_at'); + }); + + it('its two siblings behave the same way — one posture, not three', async () => { + const row: any = await seed(); + const before: any = await engine.findOne('audit_task', { where: { id: row.id } } as any); + + await engine.update( + 'audit_task', + { progress: 3, created_at: FORGED, created_by: 'forged_user', updated_by: 'forged_user' }, + { where: { id: row.id }, context: userCtx } as any, + ); + + const after: any = await engine.findOne('audit_task', { where: { id: row.id } } as any); + expect(after.created_at).toBe(before.created_at); + expect(after.created_by).toBe(before.created_by); + expect(after.updated_by).not.toBe('forged_user'); + }); + + it('a client-supplied created_at on INSERT is dropped too', async () => { + // The create ingress has its own strip (#3043). Back-dating on insert is + // the same forgery with one less step. + const row: any = await engine.insert( + 'audit_task', + { title: 'T', created_at: FORGED }, + { context: userCtx } as any, + ); + expect(row.created_at).not.toBe(FORGED); + }); + + it('a bulk update cannot forge it either — the call site, not just the switch', async () => { + // AGENTS.md PD #10's lesson: a guard wired into single-id writes only is + // still a hole one call site over. + const row: any = await seed(); + const before: any = await engine.findOne('audit_task', { where: { id: row.id } } as any); + + await engine.update( + 'audit_task', + { progress: 9, created_at: FORGED }, + { where: { progress: 1 }, multi: true, context: userCtx } as any, + ); + + const after: any = await engine.findOne('audit_task', { where: { id: row.id } } as any); + expect(after.created_at).toBe(before.created_at); + }); + + it('the historical-import path may still reinstate it (preserveAudit)', async () => { + // The deliberate escape hatch #3479/#3493 argues for. It must keep working: + // this fix closes the ORDINARY write path, it does not remove back-dating. + const row: any = await seed(); + await engine.update( + 'audit_task', + { created_at: FORGED }, + { where: { id: row.id }, context: { ...userCtx, preserveAudit: true } } as any, + ); + const after: any = await engine.findOne('audit_task', { where: { id: row.id } } as any); + expect(after.created_at).toBe(FORGED); + }); + + it('a system-context write is still exempt', async () => { + const row: any = await seed(); + await engine.update( + 'audit_task', + { created_at: FORGED }, + { where: { id: row.id }, context: { isSystem: true } } as any, + ); + const after: any = await engine.findOne('audit_task', { where: { id: row.id } } as any); + expect(after.created_at).toBe(FORGED); + }); +}); From 234f1f5c9d407ce9bbe36e5ef119ea923ff42a71 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 1 Aug 2026 13:52:16 +0000 Subject: [PATCH 2/8] fix(objectql): a lookup id that resolves to nothing is refused on write (#4441) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A `lookup` accepted an id that exists in no row of the object it declares, on both platform and application objects: POST /data/sys_position_permission_set {"position_id":"…","permission_set_id":"ps_does_not_exist_at_all"} → 200 POST /data/showcase_task {"title":"…","project":"proj_does_not_exist","status":"backlog"} → 200 The field metadata is unambiguous (`type: lookup`, `required: true`, `reference: 'sys_permission_set'`) and `deleteBehavior: 'set_null'` shows the platform already reasons about this edge on the DELETE side. Only the insert side never checked. On the RBAC link tables that is a security-surface record that resolves to nothing: an administrator auditing permissions sees a binding whose target cannot be inspected, and the audience-anchor gate must resolve that very set to evaluate the grant — so the row is an unevaluable gate input, not just untidy. Enforced in the engine, so every write path inherits it (REST, flows, actions), with the refusal carried in the `fields[]` envelope a `required` violation already uses: `{ field, code: 'reference_not_found', value, message }` → `400 VALIDATION_FAILED`. `reference_not_found` was already a catalogued `FieldErrorCode` with no emitter; it has one now, plus its message in the four platform locales. Scope kept deliberately narrow: - **Caller-supplied keys only.** `owner_id` / `organization_id` / `created_by` / `updated_by` are lookups too, written by hooks and middleware; re-validating them would turn a platform stamp into a caller-facing rejection. - **Non-system writes only**, like every other write-path guard here (`stripReadonlyFields`, `stripReadonlyForInsert`). Seed replay, package install and boot provisioning legitimately write in an order that resolves only once the batch completes; failing them closed turns an ordering detail into a boot failure. The residual — an `isSystem` caller can still write a dangling reference — is recorded rather than silently accepted. - **Empty is not a reference.** `null` / `undefined` / `''` mean "no link" — exactly what `deleteBehavior: 'set_null'` produces. - **Fails OPEN when the target cannot be checked** (unregistered object, no driver, probe throws). An integrity check that cannot run must not invent a rejection; the alternative converts a connectivity problem into data loss. - **The probe is unscoped.** Existence is a fact about the database, not about the caller's visibility — the same distinction #4435's probe turns on. A scoped probe would refuse a link to a permission set the caller cannot READ, which is ordinary under RLS and would break the platform's own admin flows. Whether the caller may create the binding is the RBAC/RLS layer's decision. Wired at BOTH update call sites, single-id and bulk — PD #10's own worked example (#3106) is a guard that reached only the single-id path. No new authorable spec key and no protocol change: the issue's "optional dangling references should be opt-in" would need one, so it is NOT implemented here and is left as a decision for the maintainers. Tests: `engine-lookup-referential-integrity.test.ts` (10) — the RBAC link table and an ordinary object, insert / update / bulk update, multi-value elements, clearing a lookup, the system-context exemption, server-stamped lookups, and the fail-open target. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017gEHJN2NFpS9VMeURvakgD --- ...ngine-lookup-referential-integrity.test.ts | 291 ++++++++++++++++++ packages/objectql/src/engine.ts | 138 ++++++++- .../spec/src/system/validation-message.ts | 4 + 3 files changed, 432 insertions(+), 1 deletion(-) create mode 100644 packages/objectql/src/engine-lookup-referential-integrity.test.ts diff --git a/packages/objectql/src/engine-lookup-referential-integrity.test.ts b/packages/objectql/src/engine-lookup-referential-integrity.test.ts new file mode 100644 index 0000000000..7044514f48 --- /dev/null +++ b/packages/objectql/src/engine-lookup-referential-integrity.test.ts @@ -0,0 +1,291 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#4441] A `lookup` must not accept an id that exists in no row of the object + * it declares. + * + * The field metadata is unambiguous — + * `{"type":"lookup","required":true,"reference":"sys_permission_set", + * "deleteBehavior":"set_null"}` — and `deleteBehavior` shows the platform + * already reasons about this edge on the DELETE side. The insert side never + * checked, so both of these created a row: + * + * POST /data/sys_position_permission_set + * {"position_id":"…","permission_set_id":"ps_does_not_exist_at_all"} → 200 + * POST /data/showcase_task + * {"title":"…","project":"proj_does_not_exist","status":"backlog"} → 200 + * + * On the RBAC link tables a dangling row is a security-surface record that + * resolves to nothing: the audience-anchor gate has to resolve that permission + * set to evaluate the grant, so the binding is an unevaluable gate input. + */ + +import { describe, it, expect, beforeEach } from 'vitest'; +import { ObjectQL } from './engine.js'; + +const permissionSet = { + name: 'ref_permission_set', + label: 'Permission Set', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const, primaryKey: true }, + name: { name: 'name', label: 'Name', type: 'text' as const }, + }, +}; + +// The RBAC link-table shape: a required lookup that an audience gate must +// resolve. +const binding = { + name: 'ref_position_permission_set', + label: 'Binding', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const, primaryKey: true }, + permission_set_id: { + name: 'permission_set_id', label: 'Permission Set', + type: 'lookup' as const, reference: 'ref_permission_set', + required: true, deleteBehavior: 'set_null' as const, + }, + note: { name: 'note', label: 'Note', type: 'text' as const }, + }, +}; + +// An ordinary application object with an OPTIONAL lookup, to prove the rule is +// about resolvability rather than about `required`. +const task = { + name: 'ref_task', + label: 'Task', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const, primaryKey: true }, + title: { name: 'title', label: 'Title', type: 'text' as const }, + project: { + name: 'project', label: 'Project', + type: 'lookup' as const, reference: 'ref_permission_set', + }, + tags: { + name: 'tags', label: 'Tags', + type: 'lookup' as const, reference: 'ref_permission_set', multiple: true, + }, + }, +}; + +function makeMemoryDriver() { + const stores = new Map>>(); + const storeFor = (obj: string) => { + let s = stores.get(obj); + if (!s) { s = new Map(); stores.set(obj, s); } + return s; + }; + let nextId = 0; + const matches = (row: Record, where: any): boolean => { + if (!where || typeof where !== 'object') return true; + for (const [k, v] of Object.entries(where)) { + if (k.startsWith('$')) continue; + const expected = (v && typeof v === 'object' && '$eq' in (v as any)) ? (v as any).$eq : v; + if ((row[k] ?? null) !== (expected ?? null)) return false; + } + return true; + }; + const driver: any = { + name: 'memory', version: '0.0.0', supports: {} as any, + 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)); + }, + findStream() { throw new Error('not implemented'); }, + 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) { + nextId += 1; + const id = (data.id as string) ?? `r_${nextId}`; + const row = { ...data, id }; + storeFor(object).set(id, row); + return row; + }, + async update(object: string, id: string, data: Record) { + const s = storeFor(object); + const cur = s.get(id); + if (!cur) return null; + const next = { ...cur, ...data, id }; + s.set(id, next); + return next; + }, + async upsert(object: string, data: Record) { + const id = data.id as string | undefined; + if (id && storeFor(object).has(id)) return this.update(object, id, data); + return this.create(object, data); + }, + async delete(object: string, id: string) { return storeFor(object).delete(id); }, + async count(object: string, ast: any) { return (await this.find(object, ast)).length; }, + async bulkCreate(object: string, rows: Record[]) { + return Promise.all(rows.map((r) => this.create(object, r))); + }, + async bulkUpdate() { return []; }, + async bulkDelete() {}, + async updateMany(object: string, ast: any, data: Record) { + const rows = await this.find(object, ast); + for (const r of rows) storeFor(object).set(r.id as string, { ...r, ...data, id: r.id }); + return rows.length; + }, + async beginTransaction() { return { commit: async () => {}, rollback: async () => {} }; }, + async commit() {}, async rollback() {}, + }; + return { driver, stores }; +} + +/** Capture a rejection so its structured `fields[]` can be inspected. */ +async function refusalOf(run: () => Promise): Promise { + try { + await run(); + } catch (e) { + return e; + } + throw new Error('expected the write to be refused, but it succeeded'); +} + +describe('[#4441] a lookup id that resolves to nothing is refused', () => { + let engine: ObjectQL; + let stores: Map>>; + const userCtx = { userId: 'u1' }; + + beforeEach(async () => { + engine = new ObjectQL(); + const mem = makeMemoryDriver(); + stores = mem.stores; + engine.registerDriver(mem.driver, true); + await engine.init(); + engine.registry.registerObject(permissionSet as any); + engine.registry.registerObject(binding as any); + engine.registry.registerObject(task as any); + await engine.insert('ref_permission_set', { id: 'ps_real', name: 'Real' }, { context: { isSystem: true } } as any); + }); + + it('the RBAC link table refuses a binding that points at nothing', async () => { + const err = await refusalOf(() => + engine.insert( + 'ref_position_permission_set', + { permission_set_id: 'ps_does_not_exist_at_all' }, + { context: userCtx } as any, + ), + ); + + // The envelope the issue asks for: 400-shaped `fields[]`, naming the field + // and the unresolvable id — the same shape a `required` violation carries. + expect(err.name).toBe('ValidationError'); + expect(err.code).toBe('VALIDATION_FAILED'); + expect(err.fields).toHaveLength(1); + expect(err.fields[0]).toMatchObject({ + field: 'permission_set_id', + code: 'reference_not_found', + value: 'ps_does_not_exist_at_all', + }); + expect(err.fields[0].message).toContain('ps_does_not_exist_at_all'); + + // …and nothing was written. A security-surface row that resolves to + // nothing must not exist even briefly. + expect(stores.get('ref_position_permission_set')?.size ?? 0).toBe(0); + }); + + it('a resolvable binding is unaffected', async () => { + const row: any = await engine.insert( + 'ref_position_permission_set', + { permission_set_id: 'ps_real' }, + { context: userCtx } as any, + ); + expect(row.permission_set_id).toBe('ps_real'); + }); + + it('an ordinary application object is covered too', async () => { + const err = await refusalOf(() => + engine.insert('ref_task', { title: 'T', project: 'proj_does_not_exist' }, { context: userCtx } as any), + ); + expect(err.fields[0]).toMatchObject({ field: 'project', code: 'reference_not_found' }); + }); + + it('an UPDATE that repoints a lookup at nothing is refused', async () => { + const row: any = await engine.insert( + 'ref_task', { title: 'T', project: 'ps_real' }, { context: userCtx } as any, + ); + const err = await refusalOf(() => + engine.update('ref_task', { project: 'gone' }, { where: { id: row.id }, context: userCtx } as any), + ); + expect(err.fields[0]).toMatchObject({ field: 'project', code: 'reference_not_found' }); + // The stored value did not move. + const after: any = await engine.findOne('ref_task', { where: { id: row.id } } as any); + expect(after.project).toBe('ps_real'); + }); + + it('a BULK update is refused as well — the call site, not just the switch', async () => { + await engine.insert('ref_task', { title: 'A', project: 'ps_real' }, { context: userCtx } as any); + const err = await refusalOf(() => + engine.update( + 'ref_task', { project: 'gone' }, + { where: { title: 'A' }, multi: true, context: userCtx } as any, + ), + ); + expect(err.fields[0]).toMatchObject({ field: 'project', code: 'reference_not_found' }); + }); + + it('clearing a lookup is not a dangling reference', async () => { + const row: any = await engine.insert( + 'ref_task', { title: 'T', project: 'ps_real' }, { context: userCtx } as any, + ); + // `null` / '' mean "no link" — exactly what `deleteBehavior: 'set_null'` + // produces, so they must never be validated as ids. + await engine.update('ref_task', { project: null }, { where: { id: row.id }, context: userCtx } as any); + await engine.update('ref_task', { project: '' }, { where: { id: row.id }, context: userCtx } as any); + const after: any = await engine.findOne('ref_task', { where: { id: row.id } } as any); + expect(after.project === null || after.project === '').toBe(true); + }); + + it('every element of a multi-value lookup is checked', async () => { + const err = await refusalOf(() => + engine.insert( + 'ref_task', { title: 'T', tags: ['ps_real', 'ps_missing'] }, { context: userCtx } as any, + ), + ); + expect(err.fields[0]).toMatchObject({ field: 'tags', code: 'reference_not_found', value: 'ps_missing' }); + }); + + it('a system-context write is exempt, so seed replay keeps its ordering freedom', async () => { + // Seeds, package install and boot provisioning legitimately write rows in + // an order that only resolves once the batch completes. Failing them closed + // would turn an ordering detail into a boot failure. + const row: any = await engine.insert( + 'ref_position_permission_set', + { permission_set_id: 'ps_not_yet_seeded' }, + { context: { isSystem: true } } as any, + ); + expect(row.permission_set_id).toBe('ps_not_yet_seeded'); + }); + + it('a server-stamped lookup is never reported as the caller\'s bad reference', async () => { + // `owner_id` / `organization_id` / `created_by` are lookups too, written by + // hooks and middleware rather than by the request. The check reads only + // caller-supplied keys, so a stamp pointing at an unseeded row cannot turn + // into a caller-facing rejection. + const row: any = await engine.insert( + 'ref_task', { title: 'T' }, { context: { ...userCtx, userId: 'user_not_in_this_db' } } as any, + ); + expect(row.title).toBe('T'); + }); + + it('an unresolvable TARGET object fails open rather than blocking every write', async () => { + // A reference to an object that is not registered (another datasource, a + // package not installed) cannot be checked. An integrity check that cannot + // run must not invent a rejection. + engine.registry.registerObject({ + name: 'ref_orphan', + label: 'Orphan', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const, primaryKey: true }, + other: { name: 'other', label: 'Other', type: 'lookup' as const, reference: 'not_registered_anywhere' }, + }, + } as any); + const row: any = await engine.insert( + 'ref_orphan', { other: 'whatever' }, { context: userCtx } as any, + ); + expect(row.other).toBe('whatever'); + }); +}); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index faf4f79916..cfda3d1cb4 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -76,7 +76,7 @@ import { ExpressionEngine } from '@objectstack/formula'; import type { Expression } from '@objectstack/spec'; import { isAggregatedViewContainer, expandViewContainer } from '@objectstack/spec'; import { bindHooksToEngine } from './hook-binder.js'; -import { validateRecord, normalizeMultiValueFields, coerceBooleanFields, ValidationError, valueShapePostureSetByEnv, mediaPostureSetByEnv, isScannableValueShapeField } from './validation/record-validator.js'; +import { validateRecord, normalizeMultiValueFields, coerceBooleanFields, ValidationError, buildFieldError, valueShapePostureSetByEnv, mediaPostureSetByEnv, isScannableValueShapeField } from './validation/record-validator.js'; import { evaluateValidationRules, needsPriorRecord, stripReadonlyWhenFields, stripReadonlyWhenFieldsMulti, hasReadonlyWhenInPayload, stripReadonlyFields } from './validation/rule-validator.js'; import { applyInMemoryAggregation } from './in-memory-aggregation.js'; import { applyHaving } from './having-filter.js'; @@ -1946,6 +1946,118 @@ export class ObjectQL implements IObjectQLEngine { }; } + /** + * [#4441] Referential integrity on the WRITE path: a `lookup` (or any + * reference-typed field) may not be given an id that exists in no row of the + * object it declares. + * + * The field metadata is unambiguous — `{"type":"lookup","required":true, + * "reference":"sys_permission_set"}` — and the DELETE side already reasons + * about the edge (`deleteBehavior: 'set_null'`). Only the insert side never + * checked, so `POST /data/sys_position_permission_set + * {"permission_set_id":"ps_does_not_exist_at_all"}` created the row. + * + * On the RBAC link tables that is a security-surface record that resolves to + * nothing: an administrator auditing permissions sees a binding whose target + * cannot be inspected, and the audience-anchor gate has to resolve that very + * set to evaluate the grant — so a dangling row is an unevaluable gate input, + * not merely an untidy one. + * + * ## Scope, deliberately narrow + * + * - **Caller-supplied keys only.** Server stamps (`owner_id`, + * `organization_id`, `created_by`/`updated_by`) are lookups too; they are + * written by hooks and middleware, not by the request, and re-validating + * them here would turn a platform stamp into a caller-facing rejection. + * - **Non-system writes only**, like every other write-path guard in this + * engine (`stripReadonlyFields`, `stripReadonlyForInsert`). Seed replay, + * package install and boot-time provisioning legitimately write rows in an + * order that resolves only once the batch completes; failing them closed + * would turn an ordering detail into a boot failure. This leaves a real + * residual — an `isSystem` caller can still write a dangling reference — + * which is recorded on the issue rather than silently accepted. + * - **Empty values are not references.** `null` / `undefined` / `''` mean + * "no link", which is what `deleteBehavior: 'set_null'` produces. + * - **Already-expanded objects are skipped.** A read round-trip can hand back + * `{id, name, …}` in the slot; that is not an id write. + * + * ## Why the probe is unscoped + * + * Existence is a fact about the database, not about the caller's visibility — + * the same distinction the #4435 existence probe turns on. A scoped probe + * would refuse a link to a permission set the caller cannot READ, which is + * ordinary in an RLS-scoped deployment and would make the platform's own + * admin flows fail. Whether the caller may create the binding at all is the + * RBAC/RLS layer's decision, made where it already is. + * + * Fails OPEN when the target cannot be checked (unregistered object, no + * driver, a probe that throws): an integrity check that cannot run must not + * invent a rejection, and the alternative — refusing every write to an object + * whose target lives on an unreachable datasource — converts a connectivity + * problem into data loss. + */ + private async assertReferencesResolve( + object: string, + schema: any, + data: Record | null | undefined, + suppliedKeys: ReadonlySet, + context: any, + msgCtx?: { locale?: string; translate?: any; objectName?: string }, + ): Promise { + if (context?.isSystem) return; + const fields = schema?.fields; + if (!fields || !data) return; + + const failures: any[] = []; + for (const name of Object.keys(fields)) { + if (!suppliedKeys.has(name)) continue; + if (!(name in data)) continue; + const def = fields[name]; + const target = referenceTargetOf(def); + if (!target) continue; + const raw = (data as Record)[name]; + const values = Array.isArray(raw) ? raw : [raw]; + for (const v of values) { + if (v === null || v === undefined || v === '') continue; + if (typeof v === 'object') continue; + const resolved = await this.referenceExists(target, v); + if (resolved === false) { + failures.push(buildFieldError( + { + field: name, + code: 'reference_not_found', + def, + value: String(v), + constraint: { target }, + }, + msgCtx as any, + )); + } + } + } + if (failures.length > 0) throw new ValidationError(failures); + } + + /** + * Does `id` name a row in `target`? `false` only when the probe RAN and found + * nothing; `null` when it could not run at all (see the fail-open note on + * {@link assertReferencesResolve}). + */ + private async referenceExists(target: string, id: unknown): Promise { + try { + const resolved = this.resolveObjectName(target); + if (!this._registry.getObject(resolved)) return null; + const row = await this.findOne(resolved, { + where: { id }, + fields: ['id'], + context: { isSystem: true }, + } as any); + return !!row; + } catch { + return null; + } + } + /** * Register the crypto provider that backs `secret`-typed fields. * @@ -3695,12 +3807,24 @@ export class ObjectQL implements IObjectQLEngine { // Locale + translation hooks for the rejection messages (#3957) — // resolved once for the batch, identical for every row. const msgCtx = this.validationMessageContext(object, opCtx.context); + // [#4441] The keys the CALLER sent, per row, snapshotted before the + // beforeInsert hooks stamped `owner_id` / `organization_id` / + // `created_by`. The reference check reads only these, so a platform + // stamp can never be reported back as a caller's bad reference. + const suppliedPerRow: ReadonlySet[] = + (isBatch ? (opCtx.data as any[]) : [opCtx.data]).map( + (row) => new Set(Object.keys((row ?? {}) as Record)), + ); for (let i = 0; i < rows.length; i++) { if (rowErrors[i] !== undefined) continue; try { normalizeMultiValueFields(schemaForValidation, rows[i]); validateRecord(schemaForValidation, rows[i], 'insert', { mediaValueShapeStrict, valueShapeStrict, messages: msgCtx }); evaluateValidationRules(schemaForValidation as any, rows[i], 'insert', { logger: this.logger, currentUser: this.buildEvalUser(opCtx.context), skipStateMachine: shouldSkipStateMachine(opCtx.context), messages: msgCtx }); + await this.assertReferencesResolve( + object, schemaForValidation, rows[i], + suppliedPerRow[i] ?? new Set(), opCtx.context, msgCtx, + ); } catch (e) { if (!partialMode) throw e; rowErrors[i] = e; @@ -4015,6 +4139,11 @@ export class ObjectQL implements IObjectQLEngine { reportDroppedFields(preRo, hookContext.input.data as Record, 'readonly'); } evaluateValidationRules(updateSchema as any, hookContext.input.data as Record, 'update', { previous: priorRecord, logger: this.logger, currentUser: this.buildEvalUser(opCtx.context), skipStateMachine: shouldSkipStateMachine(opCtx.context), messages: updateMsgCtx }); + // [#4441] A repoint is as capable of dangling as an initial link. + await this.assertReferencesResolve( + object, updateSchema, hookContext.input.data as Record, + suppliedKeys, opCtx.context, updateMsgCtx, + ); result = await driver.update(object, hookContext.input.id as string, hookContext.input.data as Record, hookContext.input.options as any); } else if (options?.multi && driver.updateMany) { await this.encryptSecretFields(object, hookContext.input.data as Record, opCtx.context, hookContext.input.options); @@ -4093,6 +4222,13 @@ export class ObjectQL implements IObjectQLEngine { } else { evaluateValidationRules(updateSchema as any, hookContext.input.data as Record, 'update', { previous: null, logger: this.logger, currentUser: bulkEvalUser, skipStateMachine: shouldSkipStateMachine(opCtx.context), messages: updateMsgCtx }); } + // [#4441] The bulk call site too — a guard wired into single-id + // writes only is still a hole one call site over (AGENTS.md + // PD #10's own worked example, #3106). + await this.assertReferencesResolve( + object, updateSchema, hookContext.input.data as Record, + suppliedKeys, opCtx.context, updateMsgCtx, + ); result = await driver.updateMany(object, ast, hookContext.input.data as Record, hookContext.input.options as any); } else { throw new Error('Update requires an ID or options.multi=true'); diff --git a/packages/spec/src/system/validation-message.ts b/packages/spec/src/system/validation-message.ts index 8bee270bdb..12bd5b5c28 100644 --- a/packages/spec/src/system/validation-message.ts +++ b/packages/spec/src/system/validation-message.ts @@ -98,6 +98,7 @@ export const BUILTIN_VALIDATION_MESSAGES: Record> invalid_datetime: '{{label}} must be a valid datetime (ISO-8601)', invalid_time: '{{label}} must be a valid time (HH:MM or HH:MM:SS)', invalid_option: '{{label}} must be one of: {{allowed}}', + reference_not_found: '{{label}}: no {{target}} record has id "{{value}}"', invalid_option_value: '{{label}}: "{{value}}" is not one of: {{allowed}}', option_unavailable: "{{label}}: option '{{value}}' is not available", invalid_type_array: '{{label}} must be an array of values', @@ -132,6 +133,7 @@ export const BUILTIN_VALIDATION_MESSAGES: Record> invalid_datetime: '{{label}}必须是有效的日期时间(ISO-8601)', invalid_time: '{{label}}必须是有效的时间(HH:MM 或 HH:MM:SS)', invalid_option: '{{label}}必须是以下值之一:{{allowed}}', + reference_not_found: '{{label}}:不存在 id 为“{{value}}”的{{target}}记录', invalid_option_value: '{{label}}:“{{value}}”不在允许的取值范围内:{{allowed}}', option_unavailable: '{{label}}:选项“{{value}}”当前不可用', invalid_type_array: '{{label}}必须是数组', @@ -163,6 +165,7 @@ export const BUILTIN_VALIDATION_MESSAGES: Record> invalid_datetime: '{{label}}は有効な日時(ISO-8601)を入力してください', invalid_time: '{{label}}は有効な時刻(HH:MM または HH:MM:SS)を入力してください', invalid_option: '{{label}}は次のいずれかを指定してください:{{allowed}}', + reference_not_found: '{{label}}:id が「{{value}}」の{{target}}レコードは存在しません', invalid_option_value: '{{label}}:「{{value}}」は指定できません(指定可能:{{allowed}})', option_unavailable: '{{label}}:選択肢「{{value}}」は現在利用できません', invalid_type_array: '{{label}}は配列で指定してください', @@ -194,6 +197,7 @@ export const BUILTIN_VALIDATION_MESSAGES: Record> invalid_datetime: '{{label}} debe ser una fecha y hora válidas (ISO-8601)', invalid_time: '{{label}} debe ser una hora válida (HH:MM o HH:MM:SS)', invalid_option: '{{label}} debe ser uno de: {{allowed}}', + reference_not_found: '{{label}}: ningún registro de {{target}} tiene el id «{{value}}»', invalid_option_value: '{{label}}: «{{value}}» no es uno de: {{allowed}}', option_unavailable: '{{label}}: la opción «{{value}}» no está disponible', invalid_type_array: '{{label}} debe ser una lista de valores', From 12a69812991fe9e4c0fc9e2dce454d1fe697ab54 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 1 Aug 2026 14:00:20 +0000 Subject: [PATCH 3/8] =?UTF-8?q?fix(objectql):=20the=20audit=20anchor=20is?= =?UTF-8?q?=20engine-owned=20=E2=80=94=20a=20declared=20created=5Fat=20can?= =?UTF-8?q?not=20loosen=20it=20(#4447)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `created_at` accepted a client-supplied value on an ordinary REST write and PERSISTED it: PATCH /api/v1/data/showcase_task/ {"progress":42,"created_at":"1999-01-01T00:00:00Z"} → 200, and the row reads back 1999-01-01 from then on Any authenticated caller who could edit a record could forge the anchor of the audit timeline — the field historical import deliberately preserves, dashboards bucket on, and an auditor reads to establish when a record came into existence — silently, in the same request as a legitimate edit, with no diagnostic. Its two siblings only LOOKED protected. The audit hook force-advances `updated_at` / `updated_by` on every update, so a forged value there is overwritten rather than refused. `created_at` is insert-only, so nothing overwrote it: one field out of the trio was genuinely unguarded, which is why this read as "one field, not a posture". ## Root cause Not a missing definition — `AUDIT_FIELD_DEFS.created_at` has carried `readonly: true, system: true` all along. The hole is that it never applied: `applySystemFields` injects an audit field only when the object does not already have one, and the merge (`{...additions, ...schema.fields}`) lets a declared field WIN. Correct for an authored business field; wrong for a family whose whole point is that the engine owns it. `showcase_task` declares no `created_at` in source, yet the built artifact ships one: "created_at": {"label":"Created At","type":"datetime","readonly":false, …} — a materialized field carrying only FieldSchema DEFAULTS. It shadowed the engine-owned definition, so `stripReadonlyFields` had nothing to key off and the forged value went straight through. No `droppedFields` either, because from the platform's point of view nothing was dropped — which is also why the #3794 contract had no live producer on this axis to test against. ## Fix The audit family's GOVERNANCE is not authorable. A declared audit field now keeps everything presentational (label, description, hidden, group, ordering …) and has `type` / `readonly` / `system` / `reference` forced to the platform's values, derived from `AUDIT_FIELD_DEFS` rather than restated so the two cannot drift. Defensive by design: it closes the class for any producer of a bogus audit field — artifact, stored metadata, AI-authored object, hand-written YAML — not just the one that surfaced it. Deliberate back-dating is untouched: `preserveAudit` (#3479/#3493) and `isSystem` writes still reinstate the original timeline, because both are checked downstream of `readonly`, not by it. Forcing `readonly`/`system` also only ever RELAXES validation — `validateRecord` skips system/readonly fields — so no previously-accepted write starts failing. ## Verified on a real boot `pnpm dev -- --fresh -p 38106`, showcase, ordinary admin session, the issue's own curl: before {"created_at":"2026-06-22T00:00:00.000Z","progress":55} PATCH {"progress":42,"created_at":"1999-01-01T00:00:00Z"} → 200 droppedFields: [{"fields":["created_at"],"reason":"readonly"}] after {"created_at":"2026-06-22T00:00:00.000Z","progress":42} The legitimate half of the request still lands, the anchor does not move, and the drop is now REPORTED — the `droppedFields` contract's first live producer here. Tests: `engine-audit-anchor-write.test.ts` (10) — the update strip, the `droppedFields` report, the whole trio behaving alike, the bulk call site, the `preserveAudit` and `isSystem` escape hatches, and the shadowing root cause reproduced with the artifact's field verbatim. Verified failing on the three root-cause cases before the fix. Known follow-up, NOT fixed here: `/api/v1/meta/objects/showcase_task` still reports `readonly: false` for `created_at` — that surface reads the artifact rather than the registry, so it now disagrees with the enforcement. Filed separately; a machine-readable surface must not lie (AGENTS.md, Route & surface ownership #4). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017gEHJN2NFpS9VMeURvakgD --- .../audit-anchor-and-lookup-integrity.md | 40 ++++++++ .../src/engine-audit-anchor-write.test.ts | 95 +++++++++++++++++-- packages/objectql/src/engine.ts | 7 +- packages/objectql/src/registry.ts | 61 +++++++++++- 4 files changed, 186 insertions(+), 17 deletions(-) create mode 100644 .changeset/audit-anchor-and-lookup-integrity.md diff --git a/.changeset/audit-anchor-and-lookup-integrity.md b/.changeset/audit-anchor-and-lookup-integrity.md new file mode 100644 index 0000000000..325687ed30 --- /dev/null +++ b/.changeset/audit-anchor-and-lookup-integrity.md @@ -0,0 +1,40 @@ +--- +"@objectstack/objectql": minor +"@objectstack/spec": minor +--- + +fix(data): the audit anchor is engine-owned, and a lookup must resolve (#4447, #4441) + +Two write-path contract holes from the v17 verification sweep. + +**#4447 — `created_at` was client-writable on an ordinary PATCH.** Its two +siblings only looked protected: the audit hook force-advances `updated_at` / +`updated_by` on every update, so a forged value is overwritten. `created_at` is +insert-only, so nothing overwrote it. The root cause is a *declared* audit +field shadowing the platform's: `applySystemFields` skips its injection when +the object already carries the name, and the merge lets the declared one win — +correct for an authored business field, wrong for the audit family. A built app +artifact ships a materialized `created_at` carrying only FieldSchema defaults +(`readonly: false`), which shadowed the engine-owned definition, so the +readonly strip had nothing to key off. The audit family's **governance** +(`readonly` / `system` / `type` / `reference`) is now forced by the platform +while presentation (label, description, hidden, group …) stays the author's. +Back-dating is unaffected: `preserveAudit` (#3479/#3493) and `isSystem` writes +still reinstate the original timeline. The strip now also reports through +`droppedFields`, giving the #3794 contract its first live producer on this axis. + +**#4441 — a `lookup` accepted an id that exists in no row of its target.** +Including `sys_position_permission_set.permission_set_id`, where a dangling row +is a security-surface record that resolves to nothing and the audience-anchor +gate has to resolve that very set to evaluate the grant. Writes are now refused +with `400 VALIDATION_FAILED` and a `fields[]` entry +(`code: 'reference_not_found'`, naming the field, the target and the +unresolvable id) — the catalogued `FieldErrorCode` that had no emitter until +now, with its message in the four platform locales. + +Scope for #4441 is deliberately narrow: caller-supplied keys only (so server +stamps are never reported as the caller's bad reference), non-system writes only +(seed replay and package install keep their ordering freedom), empty means "no +link", and it fails OPEN when the target cannot be checked. The existence probe +is unscoped, because existence is a fact about the database — whether the caller +may create the binding stays the RBAC/RLS layer's decision. diff --git a/packages/objectql/src/engine-audit-anchor-write.test.ts b/packages/objectql/src/engine-audit-anchor-write.test.ts index b1eab25b0a..95515f9dca 100644 --- a/packages/objectql/src/engine-audit-anchor-write.test.ts +++ b/packages/objectql/src/engine-audit-anchor-write.test.ts @@ -179,16 +179,6 @@ describe('[#4447] created_at is engine-owned on an ordinary write', () => { expect(after.updated_by).not.toBe('forged_user'); }); - it('a client-supplied created_at on INSERT is dropped too', async () => { - // The create ingress has its own strip (#3043). Back-dating on insert is - // the same forgery with one less step. - const row: any = await engine.insert( - 'audit_task', - { title: 'T', created_at: FORGED }, - { context: userCtx } as any, - ); - expect(row.created_at).not.toBe(FORGED); - }); it('a bulk update cannot forge it either — the call site, not just the switch', async () => { // AGENTS.md PD #10's lesson: a guard wired into single-id writes only is @@ -230,3 +220,88 @@ describe('[#4447] created_at is engine-owned on an ordinary write', () => { expect(after.created_at).toBe(FORGED); }); }); + +// --------------------------------------------------------------------------- +// The ROOT CAUSE. `showcase_task` never declares `created_at` in source, yet +// the built app artifact ships one: +// +// "created_at": {"label":"Created At","type":"datetime","readonly":false, …} +// +// i.e. a materialized field carrying only FieldSchema DEFAULTS. Because the +// object then HAS the field, `applySystemFields` skipped injecting +// `AUDIT_FIELD_DEFS.created_at` (`readonly: true`) and the author-wins merge +// let the default-valued one through — so `stripReadonlyFields` had nothing to +// key off and a forged value went straight to the row. +// --------------------------------------------------------------------------- +describe('[#4447] a declared audit field cannot loosen the platform posture', () => { + const shadowed = { + name: 'audit_shadow', + label: 'Shadowed', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const, primaryKey: true }, + title: { name: 'title', label: 'Title', type: 'text' as const }, + // Verbatim from examples/app-showcase/dist/objectstack.json. + created_at: { + label: 'Created At', type: 'datetime' as const, required: false, + searchable: false, multiple: false, unique: false, + deleteBehavior: 'set_null' as const, hidden: false, + readonly: false, sortable: true, externalId: false, + }, + }, + }; + + let engine: ObjectQL; + beforeEach(async () => { + engine = new ObjectQL(); + const { driver } = makeMemoryDriver(); + engine.registerDriver(driver, true); + await engine.init(); + engine.registry.registerObject(shadowed as any); + }); + + it('the registry restores the engine-owned governance', () => { + const schema: any = engine.registry.getObject('audit_shadow'); + expect(schema.fields.created_at).toMatchObject({ readonly: true, system: true }); + // …without discarding what the author legitimately set. + expect(schema.fields.created_at.label).toBe('Created At'); + expect(schema.fields.created_at.sortable).toBe(true); + }); + + it('and the forged PATCH no longer lands — the issue\'s exact repro', async () => { + const row: any = await engine.insert( + 'audit_shadow', { title: 'T' }, { context: { userId: 'u1' } } as any, + ); + const real = row.created_at; + await engine.update( + 'audit_shadow', + { title: 'T2', created_at: FORGED }, + { where: { id: row.id }, context: { userId: 'u1' } } as any, + ); + const after: any = await engine.findOne('audit_shadow', { where: { id: row.id } } as any); + expect(after.title).toBe('T2'); + expect(after.created_at).toBe(real); + }); + + it('an author keeps every non-governance key they declared', () => { + engine.registry.registerObject({ + name: 'audit_labelled', + label: 'Labelled', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const, primaryKey: true }, + created_at: { + label: '建档时间', type: 'datetime' as const, + description: 'When the file was opened', hidden: true, group: 'meta', + }, + }, + } as any); + const f: any = engine.registry.getObject('audit_labelled').fields.created_at; + expect(f).toMatchObject({ + label: '建档时间', + description: 'When the file was opened', + hidden: true, + group: 'meta', + readonly: true, + system: true, + }); + }); +}); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index cfda3d1cb4..62e51c8442 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -1997,7 +1997,6 @@ export class ObjectQL implements IObjectQLEngine { * problem into data loss. */ private async assertReferencesResolve( - object: string, schema: any, data: Record | null | undefined, suppliedKeys: ReadonlySet, @@ -3822,7 +3821,7 @@ export class ObjectQL implements IObjectQLEngine { validateRecord(schemaForValidation, rows[i], 'insert', { mediaValueShapeStrict, valueShapeStrict, messages: msgCtx }); evaluateValidationRules(schemaForValidation as any, rows[i], 'insert', { logger: this.logger, currentUser: this.buildEvalUser(opCtx.context), skipStateMachine: shouldSkipStateMachine(opCtx.context), messages: msgCtx }); await this.assertReferencesResolve( - object, schemaForValidation, rows[i], + schemaForValidation, rows[i], suppliedPerRow[i] ?? new Set(), opCtx.context, msgCtx, ); } catch (e) { @@ -4141,7 +4140,7 @@ export class ObjectQL implements IObjectQLEngine { evaluateValidationRules(updateSchema as any, hookContext.input.data as Record, 'update', { previous: priorRecord, logger: this.logger, currentUser: this.buildEvalUser(opCtx.context), skipStateMachine: shouldSkipStateMachine(opCtx.context), messages: updateMsgCtx }); // [#4441] A repoint is as capable of dangling as an initial link. await this.assertReferencesResolve( - object, updateSchema, hookContext.input.data as Record, + updateSchema, hookContext.input.data as Record, suppliedKeys, opCtx.context, updateMsgCtx, ); result = await driver.update(object, hookContext.input.id as string, hookContext.input.data as Record, hookContext.input.options as any); @@ -4226,7 +4225,7 @@ export class ObjectQL implements IObjectQLEngine { // writes only is still a hole one call site over (AGENTS.md // PD #10's own worked example, #3106). await this.assertReferencesResolve( - object, updateSchema, hookContext.input.data as Record, + updateSchema, hookContext.input.data as Record, suppliedKeys, opCtx.context, updateMsgCtx, ); result = await driver.updateMany(object, ast, hookContext.input.data as Record, hookContext.input.options as any); diff --git a/packages/objectql/src/registry.ts b/packages/objectql/src/registry.ts index fd50683d53..f715940ddb 100644 --- a/packages/objectql/src/registry.ts +++ b/packages/objectql/src/registry.ts @@ -280,6 +280,27 @@ const AUDIT_FIELD_DEFS = { }, } satisfies Record>; +/** + * [#4447] The subset of {@link AUDIT_FIELD_DEFS} that is NOT authorable — the + * keys that decide who may write an audit column. + * + * Derived from the definition table rather than restated, so a change to the + * canonical defs cannot leave this enforcement describing an older shape. Only + * governance travels: `label` / `description` and any presentational key an + * author adds are theirs to set. + */ +const AUDIT_FIELD_GOVERNANCE = Object.fromEntries( + AUDIT_PROVENANCE_FIELDS.map((name) => { + const def = AUDIT_FIELD_DEFS[name] as Record; + return [name, { + type: def.type, + readonly: true, + system: true, + ...(def.reference !== undefined ? { reference: def.reference } : {}), + }]; + }), +) as unknown as Record>; + export function applySystemFields( schema: ServiceObject, opts: { multiTenant: boolean } @@ -344,6 +365,9 @@ export function applySystemFields( !schema.name.startsWith('sys_'); const additions: Record = {}; + // Platform-owned field settings that must WIN over a declared field, rather + // than lose to it like `additions` does (#4447). + const overrides: Record = {}; if (wantTenant && !schema.fields?.organization_id) { additions.organization_id = { @@ -362,7 +386,36 @@ export function applySystemFields( if (wantAudit) { for (const name of AUDIT_PROVENANCE_FIELDS) { - if (!schema.fields?.[name]) additions[name] = AUDIT_FIELD_DEFS[name]; + const declared = (schema.fields as Record | undefined)?.[name]; + if (!declared) { + additions[name] = AUDIT_FIELD_DEFS[name]; + continue; + } + // [#4447] The audit family's GOVERNANCE is platform-owned, so a declared + // `created_at` cannot make the audit anchor client-writable. + // + // The injection above is skipped when the object already carries the + // field, and the merge below lets `schema.fields` win — correct for an + // authored business field, wrong for this family. It is how `created_at` + // became writable on an ordinary PATCH: the showcase artifact ships a + // materialized `created_at` carrying only FieldSchema DEFAULTS + // (`readonly: false`), which shadowed `AUDIT_FIELD_DEFS.created_at` + // (`readonly: true`), so the engine's `stripReadonlyFields` had nothing + // to key off and the forged value was written straight through — with no + // `droppedFields` either, because from the platform's point of view + // nothing was dropped. + // + // Its two siblings only LOOKED protected: the audit hook force-advances + // `updated_at`/`updated_by` on every update, so a forged value is + // overwritten rather than refused. `created_at` is insert-only, so + // nothing overwrote it — one field out of the trio genuinely unguarded. + // + // Presentation stays the author's (label, description, hidden, group, + // ordering …); only the keys that decide WHO MAY WRITE IT are forced. + // That leaves the deliberate back-dating path intact: `preserveAudit` + // (#3479/#3493) and `isSystem` writes still reinstate the original + // timeline, because they are checked downstream of `readonly`, not by it. + overrides[name] = { ...declared, ...AUDIT_FIELD_GOVERNANCE[name] }; } } @@ -385,11 +438,13 @@ export function applySystemFields( }; } - if (Object.keys(additions).length === 0) return schema; + if (Object.keys(additions).length === 0 && Object.keys(overrides).length === 0) return schema; return { ...schema, - fields: { ...additions, ...(schema.fields ?? {}) }, + // `additions` LOSE to an author's field (a declared `owner_id` is theirs); + // `overrides` WIN over it (the audit family's governance is not authorable). + fields: { ...additions, ...(schema.fields ?? {}), ...overrides }, }; } From ddeffdde28d9bc18ea43d017df2dcad3ffdc93d0 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 1 Aug 2026 14:08:01 +0000 Subject: [PATCH 4/8] fix(objectql): check only the reference the CALLER named, and narrow the audit override (#4441, #4447) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Two corrections found by running the full objectql suite (1554 tests). ## The reference check validated platform-derived ids (#4441) Filtering by supplied KEY was wrong. A form serializes an unpicked control as an explicit `null`, and `applyFieldDefaults` then fills it from `defaultValue` — including the `current_user` token (#2706). The key IS in the payload, but the id that lands is the platform's, so the check reported a server-derived value as the caller's bad reference and rejected an ordinary insert whenever the acting principal had no row in the target object. The value is still read from the post-normalization payload (multi-value strings are already split by then), but WHETHER to check is now decided by the caller's own RAW value being non-empty. `engine.test.ts`'s #2706 case was the one that caught it. ## The audit override forced more than the defect required (#4447) Forcing `type`/`reference` broke `registry.test.ts`'s "does NOT overwrite author-declared audit fields", and that test is right: an external/federated object legitimately maps its audit column to a differently-typed remote column, and #4447 is about WRITABILITY, not storage shape. The override now carries only `readonly` and `system` — the keys that decide who may write it — so the author keeps `type`, `label`, `description`, `hidden`, `group` and the rest. The pre-existing test passes unchanged. Suite green: objectql 95 files / 1554 tests. New case pinned in `engine-lookup-referential-integrity.test.ts` (11) for the derived-value direction, both for an explicit `null` and an omitted key, plus the proof that a value the caller DID name is still checked. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017gEHJN2NFpS9VMeURvakgD --- ...ngine-lookup-referential-integrity.test.ts | 38 ++++++++++++++ packages/objectql/src/engine.ts | 49 ++++++++++++++----- packages/objectql/src/registry.ts | 27 +++++----- 3 files changed, 88 insertions(+), 26 deletions(-) diff --git a/packages/objectql/src/engine-lookup-referential-integrity.test.ts b/packages/objectql/src/engine-lookup-referential-integrity.test.ts index 7044514f48..3f0dac1fb1 100644 --- a/packages/objectql/src/engine-lookup-referential-integrity.test.ts +++ b/packages/objectql/src/engine-lookup-referential-integrity.test.ts @@ -271,6 +271,44 @@ describe('[#4441] a lookup id that resolves to nothing is refused', () => { expect(row.title).toBe('T'); }); + it('a value the PLATFORM derived is never reported as the caller\'s bad reference', async () => { + // A form serializes an unpicked control as an explicit `null`, and + // `applyFieldDefaults` then fills it from `defaultValue` — including the + // `current_user` token (#2706). The key IS in the payload, but the id that + // lands is the platform's, so checking it would reject an ordinary insert + // whenever the acting principal has no row in the target (exactly what a + // bare-engine / test driver looks like). Whether to check is decided by the + // caller's own raw value, not by key presence. + engine.registry.registerObject({ + name: 'ref_defaulted', + label: 'Defaulted', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const, primaryKey: true }, + title: { name: 'title', label: 'Title', type: 'text' as const }, + owner: { + name: 'owner', label: 'Owner', type: 'lookup' as const, + reference: 'ref_permission_set', defaultValue: 'ps_not_a_row', + }, + }, + } as any); + + const explicitNull: any = await engine.insert( + 'ref_defaulted', { title: 'T', owner: null }, { context: userCtx } as any, + ); + expect(explicitNull.owner).toBe('ps_not_a_row'); + + const omitted: any = await engine.insert( + 'ref_defaulted', { title: 'T2' }, { context: userCtx } as any, + ); + expect(omitted.owner).toBe('ps_not_a_row'); + + // …but a value the caller DID name is still checked. + const err = await refusalOf(() => + engine.insert('ref_defaulted', { title: 'T3', owner: 'nope' }, { context: userCtx } as any), + ); + expect(err.fields[0]).toMatchObject({ field: 'owner', code: 'reference_not_found', value: 'nope' }); + }); + it('an unresolvable TARGET object fails open rather than blocking every write', async () => { // A reference to an object that is not registered (another datasource, a // package not installed) cannot be checked. An integrity check that cannot diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 62e51c8442..649ca185c0 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -585,6 +585,19 @@ interface SummaryDescriptor { // on every build, so the seven consumer-local surface declarations the contract // replaced can never silently drift from the engine again. IObjectQLEngine // extends IDataEngine, so the old claim rides along. +/** + * [#4441] "The caller did not name a record here." + * + * `null` / `undefined` / `''` mean NO LINK — exactly what + * `deleteBehavior: 'set_null'` writes — and an empty array is the multi-value + * spelling of the same thing. None of them is an id to resolve. + */ +function isEmptyReferenceValue(v: unknown): boolean { + if (v === null || v === undefined || v === '') return true; + if (Array.isArray(v)) return v.length === 0 || v.every((e) => e === null || e === undefined || e === ''); + return false; +} + export class ObjectQL implements IObjectQLEngine { /** * Ambient transaction store (ADR-0034). While a `transaction()` callback @@ -1999,7 +2012,7 @@ export class ObjectQL implements IObjectQLEngine { private async assertReferencesResolve( schema: any, data: Record | null | undefined, - suppliedKeys: ReadonlySet, + supplied: Record | null | undefined, context: any, msgCtx?: { locale?: string; translate?: any; objectName?: string }, ): Promise { @@ -2009,7 +2022,20 @@ export class ObjectQL implements IObjectQLEngine { const failures: any[] = []; for (const name of Object.keys(fields)) { - if (!suppliedKeys.has(name)) continue; + // Only a value the CALLER actually supplied is theirs to answer for. + // + // Key presence is not enough: a form serializes an unpicked control as + // an explicit `null`, and `applyFieldDefaults` then fills it from + // `defaultValue` — including the `current_user` token (#2706). The key is + // in the payload, but the ID that lands is the PLATFORM's, so validating + // it would report a server-derived value as the caller's bad reference + // (and reject a perfectly ordinary insert against a driver that has no + // `sys_user` row for the acting principal). + // + // So the value read for the check comes from the post-normalization + // `data` (multi-value strings are already split by then), while WHETHER + // to check is decided by the caller's own raw value being non-empty. + if (!supplied || isEmptyReferenceValue((supplied as Record)[name])) continue; if (!(name in data)) continue; const def = fields[name]; const target = referenceTargetOf(def); @@ -3806,13 +3832,15 @@ export class ObjectQL implements IObjectQLEngine { // Locale + translation hooks for the rejection messages (#3957) — // resolved once for the batch, identical for every row. const msgCtx = this.validationMessageContext(object, opCtx.context); - // [#4441] The keys the CALLER sent, per row, snapshotted before the + // [#4441] The RAW caller payload per row — before `applyFieldDefaults` + // resolved any `defaultValue` / `current_user` token and before the // beforeInsert hooks stamped `owner_id` / `organization_id` / - // `created_by`. The reference check reads only these, so a platform - // stamp can never be reported back as a caller's bad reference. - const suppliedPerRow: ReadonlySet[] = + // `created_by`. The reference check consults it to decide WHAT THE + // CALLER ACTUALLY SENT, so neither a platform stamp nor a backfilled + // default is ever reported as the caller's bad reference. + const suppliedPerRow: Array> = (isBatch ? (opCtx.data as any[]) : [opCtx.data]).map( - (row) => new Set(Object.keys((row ?? {}) as Record)), + (row) => (row ?? {}) as Record, ); for (let i = 0; i < rows.length; i++) { if (rowErrors[i] !== undefined) continue; @@ -3821,8 +3849,7 @@ export class ObjectQL implements IObjectQLEngine { validateRecord(schemaForValidation, rows[i], 'insert', { mediaValueShapeStrict, valueShapeStrict, messages: msgCtx }); evaluateValidationRules(schemaForValidation as any, rows[i], 'insert', { logger: this.logger, currentUser: this.buildEvalUser(opCtx.context), skipStateMachine: shouldSkipStateMachine(opCtx.context), messages: msgCtx }); await this.assertReferencesResolve( - schemaForValidation, rows[i], - suppliedPerRow[i] ?? new Set(), opCtx.context, msgCtx, + schemaForValidation, rows[i], suppliedPerRow[i], opCtx.context, msgCtx, ); } catch (e) { if (!partialMode) throw e; @@ -4141,7 +4168,7 @@ export class ObjectQL implements IObjectQLEngine { // [#4441] A repoint is as capable of dangling as an initial link. await this.assertReferencesResolve( updateSchema, hookContext.input.data as Record, - suppliedKeys, opCtx.context, updateMsgCtx, + opCtx.data as Record, opCtx.context, updateMsgCtx, ); result = await driver.update(object, hookContext.input.id as string, hookContext.input.data as Record, hookContext.input.options as any); } else if (options?.multi && driver.updateMany) { @@ -4226,7 +4253,7 @@ export class ObjectQL implements IObjectQLEngine { // PD #10's own worked example, #3106). await this.assertReferencesResolve( updateSchema, hookContext.input.data as Record, - suppliedKeys, opCtx.context, updateMsgCtx, + opCtx.data as Record, opCtx.context, updateMsgCtx, ); result = await driver.updateMany(object, ast, hookContext.input.data as Record, hookContext.input.options as any); } else { diff --git a/packages/objectql/src/registry.ts b/packages/objectql/src/registry.ts index f715940ddb..4d395b4ef5 100644 --- a/packages/objectql/src/registry.ts +++ b/packages/objectql/src/registry.ts @@ -284,22 +284,19 @@ const AUDIT_FIELD_DEFS = { * [#4447] The subset of {@link AUDIT_FIELD_DEFS} that is NOT authorable — the * keys that decide who may write an audit column. * - * Derived from the definition table rather than restated, so a change to the - * canonical defs cannot leave this enforcement describing an older shape. Only - * governance travels: `label` / `description` and any presentational key an - * author adds are theirs to set. + * Only `readonly` / `system` travel: everything else an author writes — + * `label`, `description`, `hidden`, `group`, and even `type` for an + * external object mapping a differently-typed remote column — stays theirs. */ -const AUDIT_FIELD_GOVERNANCE = Object.fromEntries( - AUDIT_PROVENANCE_FIELDS.map((name) => { - const def = AUDIT_FIELD_DEFS[name] as Record; - return [name, { - type: def.type, - readonly: true, - system: true, - ...(def.reference !== undefined ? { reference: def.reference } : {}), - }]; - }), -) as unknown as Record>; +const AUDIT_FIELD_GOVERNANCE: Record> = + Object.fromEntries( + // ONLY the keys that decide WHO MAY WRITE the column. `type` and + // `reference` are deliberately NOT forced: an external/federated object + // legitimately maps its audit column to a differently-typed remote column, + // and #4447 is about writability, not storage shape. Narrower is the point + // — this overrides an author, so it takes only what the defect requires. + AUDIT_PROVENANCE_FIELDS.map((name) => [name, { readonly: true, system: true }]), + ) as unknown as Record>; export function applySystemFields( schema: ServiceObject, From c7435a8d3c3c0fe96a6f0549449154dcc24a9613 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 1 Aug 2026 14:15:38 +0000 Subject: [PATCH 5/8] docs: correct the two pages this change makes untrue (#4441) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Checked the three hand-written pages the docs-drift report flagged as actually describing behaviour these fixes change (the other ~107 are indirect `@objectstack/spec` references). **`api/error-catalog.mdx` — was wrong.** It documented `INVALID_REFERENCE` as what a `lookup`/`master_detail` pointing at a missing record answers with. That code has ZERO producers in source, and now that dangling references are actually refused, the real answer is `400 VALIDATION_FAILED` with `fields[].code === 'reference_not_found'`. The entry now says so, tells callers which code to branch on, and a callout carries the real envelope plus the three deliberate non-rejections (empty value, `isSystem` write, uncheckable target). `INVALID_REFERENCE` itself is left in place — it is a `StandardErrorCode` member, and removing it is a spec change, not a docs fix. **`data-modeling/fields.mdx` — was silent.** The lookup section documented `reference` and `deleteBehavior` without saying whether the target has to exist; it does now, so authors need it. Added a short "Referential integrity" note with the rejection shape, the bulk-update coverage, and the fact that clearing a relationship is not a dangling reference — plus the distinction from `deleteBehavior`, which governs the other half of the same contract. **`protocol/objectql/schema.mdx` — accurate as-is, unchanged.** It documents the `reference` PROPERTY in a property table, not write-time semantics, so it makes no claim this change falsifies. Duplicating the note there would be two places to keep in sync. Also checked `droppedFields`, since #4447 gives that contract its first live producer on the audit axis: the only non-generated mention is in `protocol/kernel/http-protocol.mdx`, about the CORS-exposed `x-objectstack-dropped-fields` header, and it is correct. `releases/v17.mdx` mentions it too but is deliberately untouched — release notes are written centrally at release time (CLAUDE.md), never as a rider on a code PR. `content/docs/references/` is auto-generated and was not touched. `pnpm check:doc-authoring` green (215 files). Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017gEHJN2NFpS9VMeURvakgD --- content/docs/api/error-catalog.mdx | 35 +++++++++++++++++++++++++-- content/docs/data-modeling/fields.mdx | 18 ++++++++++++++ 2 files changed, 51 insertions(+), 2 deletions(-) diff --git a/content/docs/api/error-catalog.mdx b/content/docs/api/error-catalog.mdx index 0bcd2567b1..76ebd90bb5 100644 --- a/content/docs/api/error-catalog.mdx +++ b/content/docs/api/error-catalog.mdx @@ -110,10 +110,41 @@ substitute. See the [Data API](/docs/api/data-api). **Retry:** `no_retry` ### `INVALID_REFERENCE` -**Cause:** A `lookup` or `master_detail` field references a record that does not exist. -**Fix:** Verify the referenced record ID exists in the target object. +**Cause:** Reserved for an invalid foreign-key reference. **No route emits it +today.** A `lookup` / `master_detail` pointing at a record that does not exist +is refused as a *field-level* failure instead — see below. +**Fix:** Do not branch on this code; branch on `VALIDATION_FAILED` + +`fields[].code === 'reference_not_found'`. **Retry:** `no_retry` + +**A dangling reference answers `VALIDATION_FAILED`, not `INVALID_REFERENCE`** (#4441). +Writing a `lookup` / `master_detail` value with no matching row in the target +object is rejected with `400 VALIDATION_FAILED`, and the specifics ride in +`fields[]` — which names the field, the target object and the unresolvable id: + +```json +{ + "error": "Permission Set: no sys_permission_set record has id \"ps_missing\"", + "code": "VALIDATION_FAILED", + "fields": [{ + "field": "permission_set_id", + "code": "reference_not_found", + "label": "Permission Set", + "constraint": { "target": "sys_permission_set" }, + "value": "ps_missing" + }] +} +``` + +The check covers create, update and bulk update. Three cases are deliberately +*not* rejections: an empty value (`null` / `""` / `[]`) means "no link", a +`isSystem` write is exempt (seed replay and package install legitimately write +in an order that only resolves once the batch completes), and a target that +cannot be checked at all — an unregistered object, an unreachable datasource — +fails **open** rather than inventing a rejection. + + ### `DUPLICATE_VALUE` **Cause:** A field with `unique: true` already has a record with the same value. **Fix:** Use a different value or update the existing record. diff --git a/content/docs/data-modeling/fields.mdx b/content/docs/data-modeling/fields.mdx index 875f6ba4b3..fcb936538e 100644 --- a/content/docs/data-modeling/fields.mdx +++ b/content/docs/data-modeling/fields.mdx @@ -181,6 +181,24 @@ order: Field.masterDetail('order', { | `inlineColumns` | `array` | Optional explicit columns for the inline grid | | `inlineAmountField` | `string` | Optional numeric child field for the inline running total | +#### Referential integrity + +`reference` is **enforced on write**. A create or update that sets a +`lookup` / `master_detail` to an id with no matching row in the target object is +rejected with `400 VALIDATION_FAILED` and a `fields[]` entry whose `code` is +`reference_not_found` (see the [error catalog](/docs/api/error-catalog)). The +same check runs on bulk updates. + +Clearing a relationship is not a dangling reference: `null`, `""` and `[]` mean +"no link" — exactly what `deleteBehavior: 'set_null'` writes when the parent +goes away. + + +`deleteBehavior` governs what happens to *this* record when the **referenced** +record is deleted; the integrity check above governs what may be **written** +here in the first place. They are two halves of the same relationship contract. + + ### File & Media Types | Type | Factory | Description | From 1a6ee0dbdb20ee463f19af400276d89afb2f52f4 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 1 Aug 2026 15:18:59 +0000 Subject: [PATCH 6/8] fix(objectql): a readonly lookup is not the caller's reference to answer for (#4441) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The dogfood gate rejected ordinary metadata authoring — package create, publish and clone all failed with: ValidationError: Recorded By: no sys_user record has id "system" `sys_metadata_history.recorded_by` is `Field.lookup('sys_user', { readonly: true })`, and the metadata repository fills it with `actor ?? 'system'` — a SENTINEL STRING, not a user id — on a write that does not carry `isSystem`. So the PR body's claim that "non-system writes are caller writes" was simply not true of the running platform, and only the real gate could show that: the unit suite's fakes never write a sentinel into a lookup. The narrowing follows from the check's OWN stated scope rather than being an exemption bolted on to go green. `stripReadonlyFields` removes a non-system caller's value from a readonly field before the write, and the create ingress does the same (`stripReadonlyForInsert`, #3043). So a value still sitting in a readonly field at this point was written by the PLATFORM by construction — "the reference the caller named" was never going to include it. This does not weaken the fix. The two fields #4441 names — `sys_position_permission_set.permission_set_id` and `showcase_task.project` — are ordinary author-facing lookups with no `readonly`, and both stay enforced; a new case asserts exactly that, so a future widening of this skip cannot quietly swallow them. The sentinel-in-a-lookup is a genuine modelling wart — a lookup column holding a non-id — and is the same class #4441 is about, written by the platform rather than a caller. It is filed separately; rejecting the platform's own write is not the way to report it, and changing what `recorded_by` stores is not this change's call. objectql: 23 tests across the two new suites green. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017gEHJN2NFpS9VMeURvakgD --- ...ngine-lookup-referential-integrity.test.ts | 41 +++++++++++++++++++ packages/objectql/src/engine.ts | 23 +++++++++++ 2 files changed, 64 insertions(+) diff --git a/packages/objectql/src/engine-lookup-referential-integrity.test.ts b/packages/objectql/src/engine-lookup-referential-integrity.test.ts index 3f0dac1fb1..81bf1721da 100644 --- a/packages/objectql/src/engine-lookup-referential-integrity.test.ts +++ b/packages/objectql/src/engine-lookup-referential-integrity.test.ts @@ -309,6 +309,47 @@ describe('[#4441] a lookup id that resolves to nothing is refused', () => { expect(err.fields[0]).toMatchObject({ field: 'owner', code: 'reference_not_found', value: 'nope' }); }); + it('a READONLY lookup is not the caller\'s to answer for', async () => { + // By construction, not by exemption: `stripReadonlyFields` / + // `stripReadonlyForInsert` remove a non-system caller's value from a + // readonly field before the write, so anything still there was written by + // the PLATFORM — outside this check's stated scope. + // + // The real case that found this: `sys_metadata_history.recorded_by` is a + // `lookup('sys_user', { readonly: true })` the metadata repository fills + // with `actor ?? 'system'` — a SENTINEL STRING, not a user id — on a write + // that carries no `isSystem`. Checking it rejected ordinary metadata + // authoring (package create / publish / clone) in the dogfood gate. + engine.registry.registerObject({ + name: 'ref_history', + label: 'History', + fields: { + id: { name: 'id', label: 'ID', type: 'text' as const, primaryKey: true }, + note: { name: 'note', label: 'Note', type: 'text' as const }, + recorded_by: { + name: 'recorded_by', label: 'Recorded By', + type: 'lookup' as const, reference: 'ref_permission_set', readonly: true, + }, + }, + } as any); + + const row: any = await engine.insert( + 'ref_history', { note: 'n', recorded_by: 'system' }, { context: userCtx } as any, + ); + expect(row.recorded_by).toBe('system'); + }); + + it('…and the issue\'s own fields are NOT readonly, so they stay enforced', () => { + // The narrowing above must not quietly cover the two fields #4441 names. + for (const [obj, field] of [ + ['ref_position_permission_set', 'permission_set_id'], + ['ref_task', 'project'], + ] as const) { + const def: any = (engine.registry.getObject(obj) as any).fields[field]; + expect(def.readonly, `${obj}.${field} must not be readonly`).not.toBe(true); + } + }); + it('an unresolvable TARGET object fails open rather than blocking every write', async () => { // A reference to an object that is not registered (another datasource, a // package not installed) cannot be checked. An integrity check that cannot diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 649ca185c0..d568d42e9e 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -2022,6 +2022,29 @@ export class ObjectQL implements IObjectQLEngine { const failures: any[] = []; for (const name of Object.keys(fields)) { + // A `readonly` field is never the caller's to answer for — BY + // CONSTRUCTION, not by exemption. + // + // `stripReadonlyFields` removes a non-system caller's value from a + // readonly field before the write, and the create ingress does the same + // (`stripReadonlyForInsert`, #3043). So any value still sitting in one at + // this point was written by the PLATFORM, which puts it outside this + // check's own stated scope ("the reference the caller named"). + // + // Found by the dogfood gate rather than by reasoning: `sys_metadata_history. + // recorded_by` is `Field.lookup('sys_user', { readonly: true })` that the + // metadata repository fills with `actor ?? 'system'` — a SENTINEL STRING, + // not a user id, on a write that does not carry `isSystem`. Checking it + // rejected ordinary metadata authoring (package create / publish / clone). + // The sentinel-in-a-lookup is a real modelling wart and is filed + // separately; it is not this change's to fix, and rejecting the + // platform's own write is not the way to report it. + // + // This does NOT weaken #4441: the fields the issue names — + // `sys_position_permission_set.permission_set_id` and + // `showcase_task.project` — are ordinary author-facing lookups with no + // `readonly`, and both stay enforced (pinned in the unit suite). + if (fields[name]?.readonly === true) continue; // Only a value the CALLER actually supplied is theirs to answer for. // // Key presence is not enough: a form serializes an unpicked control as From 7bcd40ddaf6b98753d5aea918216037a105e3171 Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 1 Aug 2026 15:38:09 +0000 Subject: [PATCH 7/8] test(dogfood): the field-zoo matrix round-trips REAL references, not synthetic ids (#4441) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `field-zoo-roundtrip` seeded its three relational fields with ids that exist in no row: { field: 'f_lookup', write: 'acc_synthetic_0001' } { field: 'f_master_detail', write: 'proj_synthetic_0001' } { field: 'f_tree', write: 'cat_synthetic_0001' } under a comment reading "FK enforcement is off in this harness". That comment described a HOLE — precisely the one #4441 closes — so the fixture depended on the defect the platform now prevents, and the create started failing with `reference_not_found` for all three fields at once. This is the fixture being wrong, not the check being strict, so the data is what changes. The suite now creates a real row in each target object and substitutes its id. What the file actually proves is unchanged — an id string must round-trip as the same id string, the #2004 type-fidelity guard — and it now proves it over references that resolve, which is strictly stronger. The matrix keeps a `REFERENCE_PLACEHOLDER` symbol rather than a magic string, so a future edit cannot typo its way back into writing a literal id, and the placeholder can never reach the wire: substitution is keyed on identity. `REFERENCE_TARGETS` is ORDERED with a body factory because the targets reference each other — `showcase_project` declares a REQUIRED lookup to `showcase_account`, so the account must exist first and the project must be given its real id. Seeding them in the wrong order now fails loudly instead of writing a project that points at nothing, which is a small proof of #4441 in its own right. `status: 'planned'` is the state machine's declared initial state; `active` is refused with `invalid_initial_state`. Verified: `field-zoo-roundtrip.dogfood.test.ts` 46/46 green. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017gEHJN2NFpS9VMeURvakgD --- .../test/field-zoo-roundtrip.dogfood.test.ts | 35 ++++++++- packages/qa/dogfood/test/field-zoo.matrix.ts | 71 +++++++++++++++++-- 2 files changed, 97 insertions(+), 9 deletions(-) diff --git a/packages/qa/dogfood/test/field-zoo-roundtrip.dogfood.test.ts b/packages/qa/dogfood/test/field-zoo-roundtrip.dogfood.test.ts index 9b7d741361..12361d400b 100644 --- a/packages/qa/dogfood/test/field-zoo-roundtrip.dogfood.test.ts +++ b/packages/qa/dogfood/test/field-zoo-roundtrip.dogfood.test.ts @@ -21,20 +21,47 @@ import showcaseStack from '@objectstack/example-showcase'; import { SECRET_MASK } from '@objectstack/objectql'; import { bootStack, type VerifyStack } from '@objectstack/verify'; -import { MATRIX } from './field-zoo.matrix'; +import { MATRIX, REFERENCE_PLACEHOLDER, REFERENCE_TARGETS } from './field-zoo.matrix'; describe('dogfood: field-type capability matrix round-trips over HTTP (#2004)', () => { let stack: VerifyStack; let record: Record; + /** Resolved reference ids, keyed by field — the assertions compare to these. */ + const referenceIds: Record = {}; beforeAll(async () => { stack = await bootStack(showcaseStack); const token = await stack.signIn(); + // [#4441] Create a REAL row in each reference target first. + // + // The three relational entries used to write synthetic ids + // (`acc_synthetic_0001`, …) under a comment reading "FK enforcement is off + // in this harness". That comment described a HOLE, and #4441 closed it: a + // lookup / master_detail / tree pointing at a row that does not exist is + // now refused, so the fixture was relying on the very defect the platform + // now prevents. What this file actually proves — an id string round-trips + // as the same id string — is unchanged by using a real id, and the matrix + // stops depending on a bug. + for (const target of REFERENCE_TARGETS) { + const res = await stack.apiAs(token, 'POST', `/data/${target.object}`, target.body(referenceIds)); + expect( + res.status, + `could not seed ${target.object} for ${target.field}: ${res.status} ${await res.clone().text()}`, + ).toBeLessThan(300); + const json = (await res.json()) as { id?: string; record?: { id?: string } }; + const id = json.id ?? json.record?.id; + expect(id, `no id returned seeding ${target.object}`).toBeTruthy(); + referenceIds[target.field] = id as string; + } + // Build the create body from every entry that carries a `write` value // (+ required name). `present`/`computed` server-owned fields are skipped. + // A reference placeholder resolves to the id seeded above. const body: Record = { name: 'zoo-roundtrip' }; for (const c of MATRIX) { - if ('write' in c.check && c.check.write !== undefined) body[c.field] = c.check.write; + if (!('write' in c.check) || c.check.write === undefined) continue; + body[c.field] = + c.check.write === REFERENCE_PLACEHOLDER ? referenceIds[c.field] : c.check.write; } const created = await stack.apiAs(token, 'POST', '/data/showcase_field_zoo', body); @@ -62,7 +89,9 @@ describe('dogfood: field-type capability matrix round-trips over HTTP (#2004)', const actual = record[c.field]; switch (c.check.kind) { case 'equal': - expect(actual).toEqual(c.check.write); + expect(actual).toEqual( + c.check.write === REFERENCE_PLACEHOLDER ? referenceIds[c.field] : c.check.write, + ); break; case 'setEqual': { // Array-typed fields: persisted as a JSON array; order is not diff --git a/packages/qa/dogfood/test/field-zoo.matrix.ts b/packages/qa/dogfood/test/field-zoo.matrix.ts index 7f0911b8d6..1ec1caccb9 100644 --- a/packages/qa/dogfood/test/field-zoo.matrix.ts +++ b/packages/qa/dogfood/test/field-zoo.matrix.ts @@ -18,6 +18,57 @@ export type Check = | { kind: 'masked'; write: unknown } // secret: POSTed plaintext must read back as SECRET_MASK | { kind: 'computed'; expected: unknown }; // derived, asserted not written +/** + * Stand-in for a reference id the suite only knows at runtime. + * + * The three relational entries below name a target object but cannot name a + * ROW: nothing exists until the suite creates one. The HTTP suite creates a row + * in each target and substitutes its real id (`REFERENCE_TARGETS`), so nothing + * in this table needs to be a real id — but the placeholder must never reach a + * write, which is why substitution is keyed on identity rather than on a magic + * string a future edit could typo. + */ +export const REFERENCE_PLACEHOLDER = Symbol('reference-id-resolved-at-runtime'); + +/** + * Reference field → the object whose row supplies its id, and the minimal body + * that creates one. Consumed by the HTTP round-trip suite; the value-shape + * contract test ignores these entries (it never writes). + * + * ORDERED, and `body` is a factory, because the targets reference each other: + * `showcase_project` declares a REQUIRED lookup to `showcase_account`, so the + * account has to exist first and the project has to be given its real id. That + * dependency is itself a small proof of #4441 — seeding these in the wrong + * order now fails loudly instead of writing a project that points at nothing. + */ +export const REFERENCE_TARGETS: ReadonlyArray<{ + field: string; + object: string; + body: (seeded: Readonly>) => Record; +}> = [ + { + field: 'f_lookup', + object: 'showcase_account', + body: () => ({ name: 'zoo-ref-account', status: 'active' }), + }, + { + field: 'f_master_detail', + object: 'showcase_project', + body: (seeded) => ({ + name: 'zoo-ref-project', + // `planned` is the state machine's declared initial state — anything else + // is refused with `invalid_initial_state`. + status: 'planned', + account: seeded.f_lookup, + }), + }, + { + field: 'f_tree', + object: 'showcase_category', + body: () => ({ name: 'zoo-ref-category' }), + }, +]; + export interface FieldCase { field: string; type: string; @@ -88,12 +139,20 @@ export const MATRIX: FieldCase[] = [ { field: 'f_file', type: 'file', check: { kind: 'equal', write: 'file_zoo_doc' } }, { field: 'f_avatar', type: 'avatar', check: { kind: 'equal', write: 'file_zoo_avatar' } }, // relational — store a reference id as a string and read it back verbatim. - // FK enforcement is off in this harness, so this asserts value fidelity - // (id string → id string), not referential integrity / $expand (covered - // elsewhere). The point here is the stored type doesn't drift. - { field: 'f_lookup', type: 'lookup', check: { kind: 'equal', write: 'acc_synthetic_0001' } }, - { field: 'f_master_detail', type: 'master_detail', check: { kind: 'equal', write: 'proj_synthetic_0001' } }, - { field: 'f_tree', type: 'tree', check: { kind: 'equal', write: 'cat_synthetic_0001' } }, + // This asserts value fidelity (id string → id string), not `$expand`, which + // is covered elsewhere. The point here is that the stored type doesn't drift. + // + // The `write` values below are PLACEHOLDERS: the suite creates a real row in + // each target object and substitutes its id before the create, then asserts + // against that same id (see REFERENCE_TARGETS in the spec file). They used to + // be synthetic ids (`acc_synthetic_0001`, …) under a comment reading "FK + // enforcement is off in this harness" — which described a HOLE that #4441 + // closed: a lookup pointing at a row that does not exist is now refused. + // Round-tripping a REAL id proves the same fidelity and stops the matrix + // depending on a defect. + { field: 'f_lookup', type: 'lookup', check: { kind: 'equal', write: REFERENCE_PLACEHOLDER } }, + { field: 'f_master_detail', type: 'master_detail', check: { kind: 'equal', write: REFERENCE_PLACEHOLDER } }, + { field: 'f_tree', type: 'tree', check: { kind: 'equal', write: REFERENCE_PLACEHOLDER } }, // security — both credential types mask on read (plaintext never echoes back // over HTTP). `secret` is encrypted at rest; `password` on a generic object is // plaintext at rest but masked to SECRET_MASK on read (ADR-0100 / #2036 — the From 0250afd93be844894913ec495c4b34ef643138bd Mon Sep 17 00:00:00 2001 From: Claude Date: Sat, 1 Aug 2026 15:53:37 +0000 Subject: [PATCH 8/8] fix(dogfood): the matrix placeholder must satisfy BOTH consumers, not just one (#4441) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes the shard-1 regression 7bcd40dd introduced. `field-zoo.matrix.ts` has TWO consumers, and I only checked one: - `field-zoo-roundtrip.dogfood.test.ts` — drives the vectors over real HTTP and (as of 7bcd40dd) substitutes a seeded reference id; - `field-zoo-value-shape.test.ts` — the ADR-0104 contract⇔oracle interlock, which parses every `write` vector against `valueSchemaFor(type, 'stored')` WITHOUT booting a stack, and therefore never substitutes. The `Symbol` placeholder satisfied the first and broke the second with `expected string, received symbol`. Because the two live in different FILES and dogfood shards by file, that surfaced as "shard 2 fixed, shard 1 regressed" — one root cause wearing two masks, not two problems. The placeholder is now a STRING, which is what a reference's stored form actually is, so the contract test parses it like any other id vector. And substitution is keyed on the field appearing in `REFERENCE_TARGETS` — the authoritative list — rather than on comparing against the placeholder value, so the placeholder is documentation rather than a control signal and cannot leak to the wire even if someone edits it. Verified together this time, in one tree: - both matrix consumers in one run: 91/91 - dogfood shard 1/2: 38 files passed - dogfood shard 2/2: 37 passed, 1 skipped Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_017gEHJN2NFpS9VMeURvakgD --- .../test/field-zoo-roundtrip.dogfood.test.ts | 7 +++---- packages/qa/dogfood/test/field-zoo.matrix.ts | 18 +++++++++++++----- 2 files changed, 16 insertions(+), 9 deletions(-) diff --git a/packages/qa/dogfood/test/field-zoo-roundtrip.dogfood.test.ts b/packages/qa/dogfood/test/field-zoo-roundtrip.dogfood.test.ts index 12361d400b..226f741a3b 100644 --- a/packages/qa/dogfood/test/field-zoo-roundtrip.dogfood.test.ts +++ b/packages/qa/dogfood/test/field-zoo-roundtrip.dogfood.test.ts @@ -21,7 +21,7 @@ import showcaseStack from '@objectstack/example-showcase'; import { SECRET_MASK } from '@objectstack/objectql'; import { bootStack, type VerifyStack } from '@objectstack/verify'; -import { MATRIX, REFERENCE_PLACEHOLDER, REFERENCE_TARGETS } from './field-zoo.matrix'; +import { MATRIX, REFERENCE_TARGETS } from './field-zoo.matrix'; describe('dogfood: field-type capability matrix round-trips over HTTP (#2004)', () => { let stack: VerifyStack; let record: Record; @@ -60,8 +60,7 @@ describe('dogfood: field-type capability matrix round-trips over HTTP (#2004)', const body: Record = { name: 'zoo-roundtrip' }; for (const c of MATRIX) { if (!('write' in c.check) || c.check.write === undefined) continue; - body[c.field] = - c.check.write === REFERENCE_PLACEHOLDER ? referenceIds[c.field] : c.check.write; + body[c.field] = c.field in referenceIds ? referenceIds[c.field] : c.check.write; } const created = await stack.apiAs(token, 'POST', '/data/showcase_field_zoo', body); @@ -90,7 +89,7 @@ describe('dogfood: field-type capability matrix round-trips over HTTP (#2004)', switch (c.check.kind) { case 'equal': expect(actual).toEqual( - c.check.write === REFERENCE_PLACEHOLDER ? referenceIds[c.field] : c.check.write, + c.field in referenceIds ? referenceIds[c.field] : c.check.write, ); break; case 'setEqual': { diff --git a/packages/qa/dogfood/test/field-zoo.matrix.ts b/packages/qa/dogfood/test/field-zoo.matrix.ts index 1ec1caccb9..d41326b83c 100644 --- a/packages/qa/dogfood/test/field-zoo.matrix.ts +++ b/packages/qa/dogfood/test/field-zoo.matrix.ts @@ -23,12 +23,20 @@ export type Check = * * The three relational entries below name a target object but cannot name a * ROW: nothing exists until the suite creates one. The HTTP suite creates a row - * in each target and substitutes its real id (`REFERENCE_TARGETS`), so nothing - * in this table needs to be a real id — but the placeholder must never reach a - * write, which is why substitution is keyed on identity rather than on a magic - * string a future edit could typo. + * in each target and substitutes its real id — keyed on the field appearing in + * {@link REFERENCE_TARGETS}, which is the authoritative list, so this value is + * documentation rather than a control signal and can never leak to the wire. + * + * It is a STRING, deliberately. This table has two consumers: the HTTP suite, + * which substitutes, and `field-zoo-value-shape.test.ts`, which parses every + * `write` vector against the spec's `valueSchemaFor(type, 'stored')` WITHOUT + * booting a stack and therefore never substitutes. A `Symbol` placeholder + * satisfied the first and broke the second (`expected string, received + * symbol`) — and because the two live in different FILES, and dogfood shards by + * file, that surfaced as "shard 2 fixed, shard 1 regressed". A reference's + * stored form is an id string, so the placeholder is one. */ -export const REFERENCE_PLACEHOLDER = Symbol('reference-id-resolved-at-runtime'); +export const REFERENCE_PLACEHOLDER = 'zoo_reference_id_resolved_at_runtime'; /** * Reference field → the object whose row supplies its id, and the minimal body