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/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 | 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..95515f9dca --- /dev/null +++ b/packages/objectql/src/engine-audit-anchor-write.test.ts @@ -0,0 +1,307 @@ +// 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 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); + }); +}); + +// --------------------------------------------------------------------------- +// 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-lookup-referential-integrity.test.ts b/packages/objectql/src/engine-lookup-referential-integrity.test.ts new file mode 100644 index 0000000000..81bf1721da --- /dev/null +++ b/packages/objectql/src/engine-lookup-referential-integrity.test.ts @@ -0,0 +1,370 @@ +// 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('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('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 + // 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..d568d42e9e 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'; @@ -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 @@ -1946,6 +1959,153 @@ 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( + schema: any, + data: Record | null | undefined, + supplied: Record | null | undefined, + 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)) { + // 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 + // 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); + 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 +3855,25 @@ 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 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 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) => (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( + schemaForValidation, rows[i], suppliedPerRow[i], opCtx.context, msgCtx, + ); } catch (e) { if (!partialMode) throw e; rowErrors[i] = e; @@ -4015,6 +4188,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( + updateSchema, hookContext.input.data as Record, + 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) { await this.encryptSecretFields(object, hookContext.input.data as Record, opCtx.context, hookContext.input.options); @@ -4093,6 +4271,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( + updateSchema, hookContext.input.data as Record, + opCtx.data as Record, 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/objectql/src/registry.ts b/packages/objectql/src/registry.ts index fd50683d53..4d395b4ef5 100644 --- a/packages/objectql/src/registry.ts +++ b/packages/objectql/src/registry.ts @@ -280,6 +280,24 @@ 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. + * + * 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: 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, opts: { multiTenant: boolean } @@ -344,6 +362,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 +383,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 +435,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 }, }; } 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..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,20 +21,46 @@ 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_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.field in referenceIds ? referenceIds[c.field] : c.check.write; } const created = await stack.apiAs(token, 'POST', '/data/showcase_field_zoo', body); @@ -62,7 +88,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.field in referenceIds ? 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..d41326b83c 100644 --- a/packages/qa/dogfood/test/field-zoo.matrix.ts +++ b/packages/qa/dogfood/test/field-zoo.matrix.ts @@ -18,6 +18,65 @@ 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 — 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 = 'zoo_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 +147,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 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',