From 4e81036dcc8d5bfbe5f8ef55f74724c29962e1ef Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 22 Oct 2026 16:26:35 +0000 Subject: [PATCH 1/4] =?UTF-8?q?fix(plugin-security)!:=20refuse=20an=20inse?= =?UTF-8?q?rt=20naming=20a=20master=20under=20a=20controlled=5Fby=5Fparent?= =?UTF-8?q?=20declaration=20whose=20one=20master=20reference=20is=20readon?= =?UTF-8?q?ly=20or=20system=20(ADR-0055=20=C2=A71)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The parse refuses that declaration, so an object carries it only through a door that does not parse. There the write gate now refuses an insert that names a master, with the parse's own message (422 INVALID_METADATA), before any master is asked. Stored rows keep their master and their reads. Claude-Session: https://claude.ai/code/session_01LQG7z3CxDLtiMaHLbTFSeB Co-authored-by: Claude --- .../25095-cbp-unwritable-master-insert.md | 24 ++ ...by-parent-unwritable-master-insert.test.ts | 343 ++++++++++++++++++ .../plugins/plugin-security/src/errors.ts | 43 ++- .../plugin-security/src/security-plugin.ts | 62 ++++ 4 files changed, 467 insertions(+), 5 deletions(-) create mode 100644 .changeset/25095-cbp-unwritable-master-insert.md create mode 100644 packages/plugins/plugin-security/src/controlled-by-parent-unwritable-master-insert.test.ts diff --git a/.changeset/25095-cbp-unwritable-master-insert.md b/.changeset/25095-cbp-unwritable-master-insert.md new file mode 100644 index 00000000000..ef14583f4c2 --- /dev/null +++ b/.changeset/25095-cbp-unwritable-master-insert.md @@ -0,0 +1,24 @@ +--- +'@objectstack/plugin-security': patch +--- + +fix(plugin-security)!: an insert that names a master under a `controlled_by_parent` declaration whose one master reference is `readonly` or `system` is refused (ADR-0055 §1) + +Clause-②: no (narrowing) + +Metadata-protocol: none + + + +**BREAKING** for a caller that relied on such an insert landing. A non-protocol change, shipped as `patch`. + +ADR-0055 §1 derives a `controlled_by_parent` detail's access from exactly one required `master_detail` field that every write of a detail supplies. The parse refuses that master when it is declared `readonly: true` or `system: true`, so an object with it reaches the runtime only through a door that does not parse: a raw `registerObject` or `registerApp`, a `defineStack` call with `strict: false`, or a stored object the boot registers under its `[metadata_spec_invalid]` diagnostic. On such an object the security plugin's write gate let an insert that named a master through, and the detail could be stored without the master its access derives from, readable by nobody, while the insert reported success. + +- **Before.** An insert naming a master under that declaration was written. +- **Now.** It answers `INVALID_METADATA` (422) and writes nothing, single and batch creates alike. The message is the parse's own refusal of the declaration, prefixed with the write it refused, so it names the object, the field, each flag and the fix. +- **Unchanged.** + - The rows already stored: the gate still derives their read access from that field, they keep their master, and updates and deletes of them are answered as before. + - An insert that names no master, which still answers `MISSING_REQUIRED_FIELD` (422). + - System contexts, every object declared with an ordinary required master, and every object that does not declare `controlled_by_parent`. + +**What to do.** Fix the declaration as the parse says: remove `readonly: true` and `system: true` from the master reference and declare it as an ordinary required field. If the rows must outlive their parent, the object is not a detail: give it its own `sharingModel` (`private`, `public_read` or `public_read_write`). diff --git a/packages/plugins/plugin-security/src/controlled-by-parent-unwritable-master-insert.test.ts b/packages/plugins/plugin-security/src/controlled-by-parent-unwritable-master-insert.test.ts new file mode 100644 index 00000000000..098e2cb3228 --- /dev/null +++ b/packages/plugins/plugin-security/src/controlled-by-parent-unwritable-master-insert.test.ts @@ -0,0 +1,343 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [ADR-0055 §1] A `controlled_by_parent` detail whose one required + * `master_detail` is not writable (`readonly: true`, `system: true`, or both) + * is a declaration the parse refuses (`controlledByParentMasterRefusal`, shape + * `master-not-writable`). An object reaches the registry with it only through a + * door that does not parse — this suite registers it raw, through + * `registerApp` — and there the write gate holds the parse's line for the one + * write that mints a detail: + * + * - an insert that names a master is refused `422 INVALID_METADATA` with the + * parse's own message, before any master is asked, and nothing is stored — + * a single create and a batch create, for every flag; + * - an insert that names no master keeps the answer it had + * (`422 MISSING_REQUIRED_FIELD`); + * - rows already stored keep their master: the caller still reads them, a + * note-only update lands, and an update that names the master reference + * leaves the row under a master the caller reads; + * - the same detail declared with an ordinary required master takes the same + * inserts, so the refusal is the declaration's and nothing else's; + * - a system write is not the write gate's, and is untouched. + * + * Real stack: `ObjectQL` over better-sqlite3, the real `SecurityPlugin`, the + * real sharing service and middleware, and a member's context assembled by + * `resolveAuthzContext` and `assembleExecutionContext`. + */ + +import { describe, it, expect, vi, beforeAll, afterAll } from 'vitest'; +import { AsyncLocalStorage } from 'node:async_hooks'; +import { ObjectQL } from '@objectstack/objectql'; +import { SqlDriver } from '@objectstack/driver-sql'; +import { resolveAuthzContext, assembleExecutionContext } from '@objectstack/core'; +import { SharingService, buildSharingMiddleware, SysRecordShare } from '@objectstack/plugin-sharing'; +import { PermissionSetSchema } from '@objectstack/spec/security'; +import { ObjectSchema, controlledByParentMasterRefusal } from '@objectstack/spec/data'; +import { SysOrganization, SysUser, SysMember } from '@objectstack/platform-objects/identity'; + +import { SysPosition, SysPermissionSet, SysPositionPermissionSet } from './__tests__/legacy-catalog-objects.testkit.js'; +import { SysUserPosition } from './objects/sys-user-position.object.js'; +import { SysUserPermissionSet } from './objects/sys-user-permission-set.object.js'; +import { SecurityPlugin } from './security-plugin.js'; +import { defaultPermissionSets } from './objects/default-permission-sets.js'; +import { relaxTenantColumns } from './pre-18-tenant-column.testkit.js'; + +const SYS = { context: { isSystem: true } } as never; +const ORG = 'org_cbp_unw'; +const POSTURE = 'isolated' as const; +const MEMBER = 'u_cbp_unw_member'; + +const MASTER = 'qa_cbp_unw_master'; +const MASTER_ROWS = ['qa_cbp_unw_master_1', 'qa_cbp_unw_master_2'] as const; + +/** The flags a master reference may not carry under ADR-0055 §1, one detail each. */ +const FLAGGED = { + readonly: { readonly: true }, + system: { system: true }, + 'readonly and system': { readonly: true, system: true }, +} as const; +type Flagged = keyof typeof FLAGGED; +const detailName = (flags: Flagged | 'none') => `qa_cbp_unw_detail_${flags.replace(/ /g, '_')}`; + +const masterFields = { + name: { name: 'name', type: 'text' }, + owner_id: { name: 'owner_id', type: 'text' }, + created_by: { name: 'created_by', type: 'text' }, +}; +const detailFields = (flags: Record) => ({ + note: { name: 'note', type: 'text' }, + created_by: { name: 'created_by', type: 'text' }, + master: { name: 'master', type: 'master_detail', required: true, reference: MASTER, ...flags }, +}); +const detailOf = (flags: Flagged | 'none') => ({ + name: detailName(flags), + label: detailName(flags), + sharingModel: 'controlled_by_parent', + fields: detailFields(flags === 'none' ? {} : FLAGGED[flags]), +}); + +type Outcome = + | { kind: 'landed' } + | { kind: 'refused'; code?: string; status?: number; message: string }; + +const outcomeOf = (p: Promise): Promise => + p.then( + () => ({ kind: 'landed' as const }), + (e: unknown) => { + const x = e as { code?: unknown; status?: unknown; statusCode?: unknown; message?: unknown } | null; + const status = x?.statusCode ?? x?.status; + return { + kind: 'refused' as const, + code: x?.code == null ? undefined : String(x.code), + status: status == null ? undefined : Number(status), + message: String(x?.message), + }; + }, + ); + +/** See the explain-enforce-parity suite: a torn-down rig's async storages are released once its last row is judged. */ +const releaseAsyncContexts = (...owners: object[]): void => { + for (const owner of owners) { + for (const value of Object.values(owner)) if (value instanceof AsyncLocalStorage) value.disable(); + } +}; + +async function boot() { + const engine = new ObjectQL(); + engine.registerDriver( + new SqlDriver({ client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true }) as never, + true, + ); + await engine.init(); + // A door that does not parse: the declarations go to the registry as written. + engine.registerApp({ + id: 'com.objectstack.qa.cbp-unwritable-master-insert', + name: 'An insert naming a master under a master reference the declaration keeps from the author is refused', + version: '1.0.0', + type: 'plugin', + scope: 'system', + objects: [ + SysPosition, SysPermissionSet, SysPositionPermissionSet, SysUserPosition, SysUserPermissionSet, + SysOrganization, SysUser, SysMember, SysRecordShare, + { name: MASTER, label: 'Master', sharingModel: 'public_read_write', fields: masterFields }, + detailOf('none'), + ...(Object.keys(FLAGGED) as Flagged[]).map(detailOf), + ], + } as never); + await engine.syncSchemas(); + // The organization-less permission-set catalogue row below exists only on a + // store built before 18.x (ADR-0131 D1 makes a fresh tenant column NOT + // NULL): that table is this store's. + await relaxTenantColumns(engine as never, ['sys_permission_set']); + + const e = engine as unknown as { + insert: (o: string, row: Record, opts: never) => Promise; + getDriver: (o: string) => { create: (o: string, row: Record) => Promise }; + logger: { warn: () => void }; + }; + await e.insert('sys_organization', { id: ORG, name: ORG, slug: ORG }, SYS); + await e.insert('sys_user', { id: MEMBER, name: MEMBER, email: `${MEMBER}@example.test` }, SYS); + await e.insert('sys_member', { id: `mem_${MEMBER}`, user_id: MEMBER, organization_id: ORG, role: 'member' }, SYS); + const CRUD = { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true } as const; + const WRITER = 'qa_cbp_unw_writer'; + const sets = [ + ...defaultPermissionSets, + PermissionSetSchema.parse({ + name: WRITER, + objects: { + [MASTER]: { allowRead: true, allowEdit: true }, + [detailName('none')]: CRUD, + ...Object.fromEntries((Object.keys(FLAGGED) as Flagged[]).map((f) => [detailName(f), CRUD])), + }, + }), + ]; + await e.getDriver('sys_permission_set').create('sys_permission_set', { + id: `ps_${WRITER}`, name: WRITER, label: WRITER, organization_id: null, managed_by: 'admin', active: true, + object_permissions: JSON.stringify({}), system_permissions: JSON.stringify([]), + }); + await e.insert('sys_user_permission_set', { id: `ups_${MEMBER}`, user_id: MEMBER, permission_set: WRITER, organization_id: ORG }, SYS); + for (const id of MASTER_ROWS) { + await e.insert(MASTER, { id, name: id, owner_id: MEMBER, created_by: MEMBER, organization_id: ORG }, SYS); + } + + let security: unknown; + let sharing: SharingService | undefined; + const services: Record = { + manifest: { register: vi.fn() }, + objectql: engine, + metadata: { + get: async (type: string, name: string) => + type === 'permission' ? sets.find((s: { name?: string }) => s.name === name) ?? null : engine.getSchema(name) ?? null, + list: async () => sets, + }, + tenancy: { posture: POSTURE }, + get sharing() { return sharing; }, + }; + const ctx = { + logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }, + registerService: (name: string, svc: unknown) => { + if (name === 'security') security = svc; + else services[name] = svc; + }, + hook: () => undefined, + getService: (name: string) => { + if (!(name in services)) throw new Error(`service not registered: ${name}`); + return services[name]; + }, + }; + const plugin = new SecurityPlugin({ fallbackPermissionSet: 'member_default' }); + await plugin.init(ctx as never); + await plugin.start(ctx as never); + sharing = new SharingService({ engine: engine as never, securityService: () => security as never }); + engine.registerMiddleware(buildSharingMiddleware(sharing, ctx.logger) as never, { object: '*' }); + // The row gates log the writes they refuse. + vi.spyOn(e.logger, 'warn').mockImplementation(() => undefined); + + const authz = await resolveAuthzContext({ + userAuthGates: undefined, + apiKeyLifecycle: undefined, + ql: engine, + headers: {}, + getSession: async () => ({ + user: { id: MEMBER }, + session: { id: `sess_${MEMBER}`, userId: MEMBER, activeOrganizationId: ORG }, + }), + tenancyPosture: POSTURE, + } as never); + const member = assembleExecutionContext({ + authz, + oauth: undefined, + localization: undefined, + requestLocale: undefined, + accessToken: undefined, + } as never) as Record; + + return { + member: () => ({ ...member }), + /** The master walk the gate runs once per master it asks about, for counting. */ + walk: () => + vi.spyOn(plugin as unknown as { assertControlledByParentChainEditable: () => Promise }, 'assertControlledByParentChainEditable'), + insert: (detail: string, data: Record | Record[], context: Record) => + outcomeOf(engine.insert(detail, data as never, { context } as never)), + update: (detail: string, id: string, data: Record, context: Record) => + outcomeOf(engine.update(detail, data, { where: { id }, context } as never)), + /** A row as a system read sees it: its master and its note, or `null` when there is none. */ + stored: async (detail: string, id: string) => { + const row = (await engine.findOne(detail, { where: { id }, context: { isSystem: true } } as never)) as + | Record + | null; + return row ? { master: row.master ?? null, note: row.note ?? null } : null; + }, + /** The ids of the rows the caller's own read returns, of those named. */ + readable: async (detail: string, ids: readonly string[], context: Record) => { + const rows = (await engine.find(detail, { where: { id: { $in: [...ids] } }, context } as never)) as Array<{ id: string }>; + return rows.map((r) => r.id).sort(); + }, + /** A detail row written as the platform would write it, under a master. */ + seed: (detail: string, id: string, master: string) => + e.getDriver(detail).create(detail, { id, note: 'seeded', created_by: MEMBER, master, organization_id: ORG }), + systemInsert: (detail: string, row: Record) => outcomeOf(e.insert(detail, row, SYS)), + teardown: async () => { + try { await engine.destroy(); } catch { /* noop */ } + releaseAsyncContexts(engine, plugin); + }, + }; +} + +type Rig = Awaited>; +const FLAG_NAMES = Object.keys(FLAGGED) as Flagged[]; + +/** The parse's own refusal of a flagged declaration — the text the gate's refusal carries. */ +const parseRefusal = (flags: Flagged) => + controlledByParentMasterRefusal(detailName(flags), 'controlled_by_parent', detailFields(FLAGGED[flags])); + +describe('ADR-0055 §1 — an insert naming a master under a master reference the declaration keeps from the author', () => { + let rig: Rig; + beforeAll(async () => { rig = await boot(); }); + afterAll(async () => { await rig.teardown(); }); + + it('each flagged declaration is one the parse refuses, at `fields`, as `master-not-writable`; the ordinary one parses', () => { + for (const flags of FLAG_NAMES) { + expect({ flags, shape: parseRefusal(flags)?.shape }).toEqual({ flags, shape: 'master-not-writable' }); + const parsed = ObjectSchema.safeParse(detailOf(flags)); + expect({ flags, success: parsed.success }).toEqual({ flags, success: false }); + if (!parsed.success) expect(parsed.error.issues.map((i) => i.path.join('.'))).toContain('fields'); + } + expect(ObjectSchema.safeParse(detailOf('none')).success).toBe(true); + }); + + for (const flags of FLAG_NAMES) { + it(`${flags}: a single create naming a master is refused 422 INVALID_METADATA with the parse's message, before any master is asked, and nothing is stored`, async () => { + const detail = detailName(flags); + const walk = rig.walk(); + try { + const id = `${detail}_single`; + const outcome = await rig.insert(detail, { id, note: 'x', master: MASTER_ROWS[0] }, rig.member()); + expect(outcome).toMatchObject({ kind: 'refused', code: 'INVALID_METADATA', status: 422 }); + expect((outcome as { message: string }).message).toBe( + `[Security] Configuration defect: insert on '${detail}' cannot be written. ${parseRefusal(flags)!.message}`, + ); + expect(walk).not.toHaveBeenCalled(); + expect(await rig.stored(detail, id)).toBeNull(); + } finally { + walk.mockRestore(); + } + }); + + it(`${flags}: a batch create is refused whole when any row names a master, and nothing is stored`, async () => { + const detail = detailName(flags); + const ids = [`${detail}_batch_1`, `${detail}_batch_2`]; + const outcome = await rig.insert( + detail, + [{ id: ids[0], note: 'x' }, { id: ids[1], note: 'x', master: MASTER_ROWS[1] }], + rig.member(), + ); + expect(outcome).toMatchObject({ kind: 'refused', code: 'INVALID_METADATA', status: 422 }); + for (const id of ids) expect({ id, row: await rig.stored(detail, id) }).toEqual({ id, row: null }); + }); + + it(`${flags}: a create naming no master keeps its own answer, 422 MISSING_REQUIRED_FIELD`, async () => { + const detail = detailName(flags); + const id = `${detail}_no_master`; + const outcome = await rig.insert(detail, { id, note: 'x' }, rig.member()); + expect(outcome).toMatchObject({ kind: 'refused', code: 'MISSING_REQUIRED_FIELD', status: 422 }); + expect(await rig.stored(detail, id)).toBeNull(); + }); + + it(`${flags}: a stored row keeps its master — still read, a note-only update lands, and an update naming the reference leaves it under a master the caller reads`, async () => { + const detail = detailName(flags); + const id = `${detail}_stored`; + await rig.seed(detail, id, MASTER_ROWS[0]); + expect(await rig.readable(detail, [id], rig.member())).toEqual([id]); + + expect(await rig.update(detail, id, { note: 'written' }, rig.member())).toEqual({ kind: 'landed' }); + expect(await rig.stored(detail, id)).toEqual({ master: MASTER_ROWS[0], note: 'written' }); + + expect(await rig.update(detail, id, { master: MASTER_ROWS[1] }, rig.member())).toEqual({ kind: 'landed' }); + const after = await rig.stored(detail, id); + expect(MASTER_ROWS).toContain(after?.master); + expect(await rig.readable(detail, [id], rig.member())).toEqual([id]); + }); + + it(`${flags}: a system create is not the write gate's, and lands under its master`, async () => { + const detail = detailName(flags); + const id = `${detail}_system`; + expect(await rig.systemInsert(detail, { id, note: 'x', master: MASTER_ROWS[0], organization_id: ORG })).toEqual({ kind: 'landed' }); + expect(await rig.stored(detail, id)).toEqual({ master: MASTER_ROWS[0], note: 'x' }); + }); + } + + it('the same detail with an ordinary required master takes the same creates, and reads them back under that master', async () => { + const detail = detailName('none'); + expect(await rig.insert(detail, { id: `${detail}_single`, note: 'x', master: MASTER_ROWS[0] }, rig.member())).toEqual({ kind: 'landed' }); + expect( + await rig.insert(detail, [{ id: `${detail}_batch_1`, note: 'x', master: MASTER_ROWS[0] }, { id: `${detail}_batch_2`, note: 'x', master: MASTER_ROWS[1] }], rig.member()), + ).toEqual({ kind: 'landed' }); + expect(await rig.stored(detail, `${detail}_single`)).toEqual({ master: MASTER_ROWS[0], note: 'x' }); + expect(await rig.stored(detail, `${detail}_batch_2`)).toEqual({ master: MASTER_ROWS[1], note: 'x' }); + expect(await rig.readable(detail, [`${detail}_single`, `${detail}_batch_1`, `${detail}_batch_2`], rig.member())).toEqual( + [`${detail}_batch_1`, `${detail}_batch_2`, `${detail}_single`], + ); + }); +}); diff --git a/packages/plugins/plugin-security/src/errors.ts b/packages/plugins/plugin-security/src/errors.ts index 06a09b29805..4c3cd182f86 100644 --- a/packages/plugins/plugin-security/src/errors.ts +++ b/packages/plugins/plugin-security/src/errors.ts @@ -142,6 +142,38 @@ export class MasterDetailRelationMissingError extends Error { } } +/** + * [ADR-0055 §1] An insert names a master under a `controlled_by_parent` + * declaration whose one required `master_detail` is not writable (`readonly: + * true` or `system: true`) → `422 INVALID_METADATA`. + * + * The parse refuses that declaration (`controlledByParentMasterRefusal`, shape + * `master-not-writable`), so an object reaches this only through a door that + * does not parse: a raw `registerObject` / `registerApp`, a non-strict + * `defineStack`, or a stored row the boot registers under its + * `[metadata_spec_invalid]` diagnostic. The write gate still derives the + * object's READ access from that field, so rows already stored keep their + * master; what it refuses is a new detail. The engine does not treat a + * caller's value in a field the declaration keeps from the author as an + * ordinary master reference, so such a row can land without the master its + * access derives from: readable by nobody, while the insert reports success. + * + * The message is the parse's own, prefixed with the write it refused, so the + * prescription is the one `os validate` and the metadata save door give for + * the same declaration, and the two cannot drift. Same code and status as + * {@link MasterDetailRelationMissingError}: a broken declaration, not an access + * verdict. + */ +export class MasterReferenceNotWritableError extends Error { + readonly code = 'INVALID_METADATA'; + readonly status = 422; + readonly statusCode = 422; + constructor(object: string, operation: string, declarationRefusal: string) { + super(`[Security] Configuration defect: ${operation} on '${object}' cannot be written. ${declarationRefusal}`); + this.name = 'MasterReferenceNotWritableError'; + } +} + /** * The by-id write names a detail row that does not exist → `404 * RECORD_NOT_FOUND`. @@ -206,11 +238,12 @@ export class DetailRecordNotFoundError extends Error { * (a `master_detail` with no `required`; `required` + `readonly`; * `required` + `system`) — there this gate is the only refusal there is, * and its 422 is what stops an unreadable orphan detail row being minted. - * The 2026-08-16 ruling (commit 8abada3ba) orders a publish-time lint ramp, but - * it has not landed — nothing refuses them at publish today, so an app - * can newly declare one and land here (#8959). This is the answer those - * shapes get for as long as they stay declarable, not merely until some - * legacy app is republished. + * The parse now refuses all three (the first since #23100, which also + * took it out of the relation resolver; the two flagged ones since + * #25092), so they arrive only through a door that does not parse. An + * insert that omits the master still gets this answer there; one that + * names a master under a flagged declaration gets + * {@link MasterReferenceNotWritableError}'s. * * The wording is unchanged in both, so a surface that pins either sentence sees * the same string it always did — what moved is WHICH inserts arrive here. diff --git a/packages/plugins/plugin-security/src/security-plugin.ts b/packages/plugins/plugin-security/src/security-plugin.ts index 15d9a5abd15..24aa78f4104 100644 --- a/packages/plugins/plugin-security/src/security-plugin.ts +++ b/packages/plugins/plugin-security/src/security-plugin.ts @@ -13,6 +13,9 @@ import { MCP_AGENT_PERMISSION_SET_RESTRICTED } from '@objectstack/spec/ai'; // predicate write, which also bounds the id list a predicate write's read // question composes (see PREDICATE_WRITE_READABLE_ID_CAP). import { markFilterSubtreeProvenance, FieldMaskingRuleSchema, MAX_BULK_PER_ROW_HOOK_ROWS, type FieldMaskingRule } from '@objectstack/spec/data'; +// [ADR-0055 §1] The parse's own one-master judgement, asked of an object that +// reached the registry without the parse (see `resolveCbpRelation`). +import { controlledByParentMasterRefusal } from '@objectstack/spec/data'; import { StandardErrorCode } from '@objectstack/spec/api'; import type { NumberComparandDoorFieldMeta } from '@objectstack/spec/data'; // [#7414] The SHARED operation-message catalog #7307 built for the data path's @@ -92,6 +95,7 @@ import { isPlatformOwnershipFloorPolicy, masterGateCoversOperation, masterGovernsRowWrites, + OWD_DELEGATING_ROW_WRITES, owdDeclaresOpenRowWrites, owdOpenWritesCoversOperation, withOwnershipFloorAlternates, @@ -140,6 +144,7 @@ import { MasterDetailRelationMissingError, DetailRecordNotFoundError, MasterReferenceMissingError, + MasterReferenceNotWritableError, MaskedValueWriteError, } from './errors.js'; // [ADR-0138 D4] The guest entry's organization step, and the located refusal @@ -678,6 +683,45 @@ interface CbpRelation { * for why. */ omissionRefusedByValidation: boolean; + /** + * [ADR-0055 §1] The parse's refusal of this declaration when its one master + * reference is not writable (`readonly: true` or `system: true`, the + * `master-not-writable` shape of `controlledByParentMasterRefusal`), else + * `null`. + * + * The relation still resolves for such an object, on purpose: its stored + * rows keep the master their reads derive from, so an object registered + * without the parse does not lose access to rows it already has. What the + * write gate refuses is a NEW detail that names a master in that field + * ({@link MasterReferenceNotWritableError}). The engine does not treat a + * caller's value in a field the declaration keeps from the author as an + * ordinary master reference, so a detail written through it can land outside + * its master. The refusal follows the declaration the parse refuses, not one + * of its flags: one rule and one prescription. + */ + masterNotWritable: string | null; +} + +/** + * [ADR-0055 §1] {@link CbpRelation.masterNotWritable} for one registered + * object: the parse's refusal message when the object delegates its row access + * to a master and its one master reference is not writable, else `null`. + */ +function masterNotWritableRefusal(object: string, schema: unknown, fields: Record): string | null { + if (!masterGovernsRowWrites(schema)) return null; + const refusal = controlledByParentMasterRefusal(object, OWD_DELEGATING_ROW_WRITES, fields); + return refusal?.shape === 'master-not-writable' ? refusal.message : null; +} + +/** + * [ADR-0055 §1] Does an insert's body name a master in `fk`? A single create's + * body, or any row of a batch create's, carrying a value there. A body that + * names none is the empty-master branch's, unchanged. + */ +function insertNamesMaster(data: unknown, fk: string): boolean { + const names = (row: unknown) => + row != null && typeof row === 'object' && !Array.isArray(row) && (row as Record)[fk] != null; + return Array.isArray(data) ? data.some(names) : names(data); } /** @@ -8877,6 +8921,9 @@ export class SecurityPlugin implements Plugin { // mirrors and cannot widen if the resolver ever changes. omissionRefusedByValidation: def?.type === 'master_detail' && !!def?.required && !def?.readonly && !def?.system, + // Asked of the parse's own predicate, never re-derived here, so the + // shape and the prescription are the ones `ObjectSchema` refuses. + masterNotWritable: masterNotWritableRefusal(object, schema, Object.fromEntries(entries)), }; } this.cbpRelCache.set(object, rel); @@ -9618,6 +9665,15 @@ export class SecurityPlugin implements Plugin { // narrowing is real — the non-null assertions this branch used to need // (`rel!.fk`) were the load-bearing half of the same defect. if (!rel) throw new MasterDetailRelationMissingError(object, operation); + // [ADR-0055 §1] Also a metadata defect, and answered before any master is + // asked: an insert naming a master under a declaration the parse refuses + // because that one master reference is not writable + // ({@link CbpRelation.masterNotWritable}). Only an insert mints a detail + // this way. The rows already stored keep the master their reads derive + // from, and an update's value there never re-points one outside it. + if (operation === 'insert' && rel.masterNotWritable !== null && insertNamesMaster(opCtx.data, rel.fk)) { + throw new MasterReferenceNotWritableError(object, operation, rel.masterNotWritable); + } // [ADR-0055 §3 · #25044] Every master the write involves is asked, not one // of them. A body that names a master reference involves that master too, @@ -9771,6 +9827,12 @@ export class SecurityPlugin implements Plugin { // master reference authored with no `required` comes out of // `create()` as `required: true` plus the flag, which is exactly a // shape `validateRecord` skips. + // ⚠️ [#25092] SUPERSEDED for these two: the parse now refuses them + // (`master-not-writable`), `create()` included, since it ends in the + // parse. They reach this branch only through a door that does not + // parse, and there an insert that names a master never gets here: it + // is refused above ({@link CbpRelation.masterNotWritable}). The + // freeze below still binds the omitted-master insert for both. // // And nothing warns on the way past: the lint measured by #8959 is // unchanged — the predicate is still `def.required !== true` at From 77e4f310787df991f6a9e909ccc84a1ac5a65091 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 22 Oct 2026 16:55:36 +0000 Subject: [PATCH 2/4] =?UTF-8?q?fix(plugin-security)!:=20refuse=20every=20w?= =?UTF-8?q?rite=20that=20sets=20a=20controlled=5Fby=5Fparent=20master=20re?= =?UTF-8?q?ference=20declared=20readonly=20or=20system=20(ADR-0055=20?= =?UTF-8?q?=C2=A71)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Ruling 6345869077 on #25095 (route B, refuse at the write gate). The relation still resolves for such a declaration, so reads, the cache and the unresolved report are unchanged and stored rows keep their master. CbpRelation.masterNotWritable is now read from the same field def the relation came from, judged by the parse's own predicate on that one def. The gate refuses, before any master is asked and before anything is written, every insert (single or batch, with or without a master in the body) and every update whose body carries the reference key (any value, null included), 422 INVALID_METADATA with the parse's message. A delete and an update that leaves the key alone take the path they took before. The same answer for every caller the gate judges; the system bypass is unchanged. Tests: the suite is renamed to controlled-by-parent-unwritable-master-write and now covers a member and a platform admin, ordinary and sys_-named objects, inserts with and without a master, batch inserts, updates naming a master or null, and the controls (stored rows read, an update without the key, a delete, a system create, the in-repo declaration shape). The [#8688] flagged-insert pin in controlled-by-parent-sharing and the unjudged-reference pins in controlled-by-parent-every-row-master move to the new answer; the stored-master-first ordering pin moves to the validated detail. The stale AUTHORABLE wording and the two stale freeze-note paragraphs are corrected. Claude-Session: https://claude.ai/code/session_01LQG7z3CxDLtiMaHLbTFSeB Co-authored-by: Claude --- .../25095-cbp-unwritable-master-insert.md | 24 - .../25095-cbp-unwritable-master-write.md | 24 + ...trolled-by-parent-every-row-master.test.ts | 53 +- .../src/controlled-by-parent-sharing.test.ts | 34 +- ...by-parent-unwritable-master-insert.test.ts | 343 ------------- ...-by-parent-unwritable-master-write.test.ts | 451 ++++++++++++++++++ .../plugins/plugin-security/src/errors.ts | 37 +- .../plugin-security/src/security-plugin.ts | 158 +++--- 8 files changed, 612 insertions(+), 512 deletions(-) delete mode 100644 .changeset/25095-cbp-unwritable-master-insert.md create mode 100644 .changeset/25095-cbp-unwritable-master-write.md delete mode 100644 packages/plugins/plugin-security/src/controlled-by-parent-unwritable-master-insert.test.ts create mode 100644 packages/plugins/plugin-security/src/controlled-by-parent-unwritable-master-write.test.ts diff --git a/.changeset/25095-cbp-unwritable-master-insert.md b/.changeset/25095-cbp-unwritable-master-insert.md deleted file mode 100644 index ef14583f4c2..00000000000 --- a/.changeset/25095-cbp-unwritable-master-insert.md +++ /dev/null @@ -1,24 +0,0 @@ ---- -'@objectstack/plugin-security': patch ---- - -fix(plugin-security)!: an insert that names a master under a `controlled_by_parent` declaration whose one master reference is `readonly` or `system` is refused (ADR-0055 §1) - -Clause-②: no (narrowing) - -Metadata-protocol: none - - - -**BREAKING** for a caller that relied on such an insert landing. A non-protocol change, shipped as `patch`. - -ADR-0055 §1 derives a `controlled_by_parent` detail's access from exactly one required `master_detail` field that every write of a detail supplies. The parse refuses that master when it is declared `readonly: true` or `system: true`, so an object with it reaches the runtime only through a door that does not parse: a raw `registerObject` or `registerApp`, a `defineStack` call with `strict: false`, or a stored object the boot registers under its `[metadata_spec_invalid]` diagnostic. On such an object the security plugin's write gate let an insert that named a master through, and the detail could be stored without the master its access derives from, readable by nobody, while the insert reported success. - -- **Before.** An insert naming a master under that declaration was written. -- **Now.** It answers `INVALID_METADATA` (422) and writes nothing, single and batch creates alike. The message is the parse's own refusal of the declaration, prefixed with the write it refused, so it names the object, the field, each flag and the fix. -- **Unchanged.** - - The rows already stored: the gate still derives their read access from that field, they keep their master, and updates and deletes of them are answered as before. - - An insert that names no master, which still answers `MISSING_REQUIRED_FIELD` (422). - - System contexts, every object declared with an ordinary required master, and every object that does not declare `controlled_by_parent`. - -**What to do.** Fix the declaration as the parse says: remove `readonly: true` and `system: true` from the master reference and declare it as an ordinary required field. If the rows must outlive their parent, the object is not a detail: give it its own `sharingModel` (`private`, `public_read` or `public_read_write`). diff --git a/.changeset/25095-cbp-unwritable-master-write.md b/.changeset/25095-cbp-unwritable-master-write.md new file mode 100644 index 00000000000..ddff093e51d --- /dev/null +++ b/.changeset/25095-cbp-unwritable-master-write.md @@ -0,0 +1,24 @@ +--- +'@objectstack/plugin-security': patch +--- + +fix(plugin-security)!: under a `controlled_by_parent` declaration whose one master reference is `readonly` or `system`, every write that sets that reference is refused (ADR-0055 §1) + +Clause-②: no (narrowing) + +Metadata-protocol: none + + + +**BREAKING** for a caller that relied on such a write landing. A non-protocol change, shipped as `patch`. + +ADR-0055 §1 derives a `controlled_by_parent` detail's access from exactly one required `master_detail` field that every write of a detail supplies. The parse refuses that master when it is declared `readonly: true` or `system: true`, so an object with it reaches the runtime only through a door that does not parse: a raw `registerObject` or `registerApp`, a `defineStack` call with `strict: false`, or a stored object the boot registers under its `[metadata_spec_invalid]` diagnostic. On such an object the security plugin's write gate let writes that set the master reference through, and a detail could end up outside the master its access derives from, readable by nobody, while the write reported success. + +- **Before.** An insert under that declaration was written when it named a master. An update could set the reference. +- **Now.** Every write that sets the reference answers `INVALID_METADATA` (422) and writes nothing, before any master is asked: every insert, single or batch, whether or not it names a master, and every update whose body carries the reference key, whatever the value, `null` included. The message is the parse's own refusal of the declaration, prefixed with the write it refused, so it names the object, the field, each flag and the fix. It is the same for every caller the gate judges. +- **Unchanged.** + - The rows already stored: the gate still derives their read access from that field, so they keep their master and are read as before. + - A delete, and an update whose body leaves the reference key out. + - System contexts, every object declared with an ordinary required master, and every object that does not declare `controlled_by_parent`. + +**What to do.** Fix the declaration as the parse says: remove `readonly: true` and `system: true` from the master reference and declare it as an ordinary required field. If the rows must outlive their parent, the object is not a detail: give it its own `sharingModel` (`private`, `public_read` or `public_read_write`). diff --git a/packages/plugins/plugin-security/src/controlled-by-parent-every-row-master.test.ts b/packages/plugins/plugin-security/src/controlled-by-parent-every-row-master.test.ts index 6fc3f2f8467..48428902f70 100644 --- a/packages/plugins/plugin-security/src/controlled-by-parent-every-row-master.test.ts +++ b/packages/plugins/plugin-security/src/controlled-by-parent-every-row-master.test.ts @@ -27,16 +27,16 @@ * - each master a batch names is asked once, in row order; * - a row naming no master, under a reference required-field validation * judges, is answered by validation as before; - * - under a reference required-field validation does not judge, a batch - * create naming a master the caller may not edit is refused on that master - * (for an agent, whose own pass runs first, on the empty-master answer), - * and every other batch create is still refused whole, as before; - * - an update that empties the master reference, under a reference - * required-field validation does not judge, is refused — addressed by - * `where`, in its body and by predicate — with the empty-master refusal, and - * nothing is written; under a reference validation judges, validation's - * answer is unchanged; and an update that leaves the reference out lands as - * it did. + * - under a reference required-field validation does not judge — one the + * declaration keeps from the author, which the parse refuses — every batch + * create is refused whole as the declaration defect it is (#25095), before + * any master is asked, and nothing is written; + * - an update that empties the master reference is refused — addressed by + * `where`, in its body and by predicate — and nothing is written: under a + * reference validation judges, by validation as before, after the master + * the row is stored under is asked; under one it does not judge, as the + * declaration defect (#25095); and an update that leaves the reference out + * lands as it did. * * Real stack: `ObjectQL` over better-sqlite3, the real `SecurityPlugin`, the * real sharing service and middleware, the real protocol, and contexts @@ -83,7 +83,9 @@ const D = { rls: 'qa_cbp_erm_detail_rls', chain: 'qa_cbp_erm_detail_chain' } as /** * A detail under `M.rls` whose master reference required-field validation does * not judge (`validateRecord` skips a provenance-flagged field), so the gate is - * the one thing that holds the reference populated. + * the one thing that holds the reference populated. The parse refuses the + * declaration (`master-not-writable`), and since #25095 the gate refuses every + * write that sets the reference as that defect. */ const UNJUDGED = 'qa_cbp_erm_detail_unjudged'; @@ -457,27 +459,20 @@ describe('ADR-0055 §1 · §3 — every row a write stores in a controlled_by_pa }); }); - it('under a reference validation does not judge: a batch create naming a master the caller may not edit is refused on that master, and every other batch create is still refused whole', async () => { + it('under a reference validation does not judge: every batch create is refused whole as the declaration defect, before any master is asked, and nothing is written', async () => { for (const caller of Object.keys(CALLERS) as Caller[]) { const { user, onBehalf } = CALLERS[caller]; const context = await rig.contextOf(user, onBehalf); + // [#25095] The single create's answer, even naming a master the caller may not edit. const refusal = await rig.insert(UNJUDGED, rig.rows([held('rls')])[0]!, context); - expect(refusal).toMatchObject({ kind: 'refused', code: 'PERMISSION_DENIED', status: 403 }); + expect(refusal).toMatchObject({ kind: 'refused', code: 'INVALID_METADATA', status: 422 }); for (const door of DOORS) { const refused = rig.rows([free('rls'), held('rls')]); - const outcome = await rig.batch[door](UNJUDGED, refused, context); - if (onBehalf) { - // The agent's own pass runs first and whole: its sets may edit both - // masters, so it meets the batch's empty-master answer before the - // member's pass asks the master the member may not edit. Refused all - // the same, and nothing is written. - expect({ caller, door, outcome }).toMatchObject({ caller, door, outcome: { kind: 'refused', code: 'MISSING_REQUIRED_FIELD', status: 422 } }); - } else { + const whole = rig.rows([free('rls'), free('rls', 2)]); + for (const rows of [refused, whole]) { + const outcome = await rig.batch[door](UNJUDGED, rows, context); expect({ caller, door, outcome: envelope(outcome) }).toEqual({ caller, door, outcome: envelope(refusal) }); } - const whole = rig.rows([free('rls'), free('rls', 2)]); - expect({ caller, door, outcome: await rig.batch[door](UNJUDGED, whole, context) }) - .toMatchObject({ caller, door, outcome: { kind: 'refused', code: 'MISSING_REQUIRED_FIELD', status: 422 } }); const after = await rig.stored(UNJUDGED, [...ids(refused), ...ids(whole)]); for (const [id, row] of Object.entries(after)) expect({ caller, door, id, row }).toEqual({ caller, door, id, row: null }); } @@ -486,13 +481,13 @@ describe('ADR-0055 §1 · §3 — every row a write stores in a controlled_by_pa describe('an update never empties the master reference a detail derives its access from', () => { for (const caller of Object.keys(CALLERS) as Caller[]) { - it(`${caller}: under a reference validation does not judge, it is refused on every update shape, and nothing is written`, async () => { + it(`${caller}: under a reference validation does not judge, it is refused on every update shape as the declaration defect, and nothing is written`, async () => { const { user, onBehalf } = CALLERS[caller]; const context = await rig.contextOf(user, onBehalf); for (const shape of UPDATES) { const id = await rig.seed(UNJUDGED, free('rls'), user); const outcome = await rig.update[shape](UNJUDGED, id, { master: null, note: 'written' }, context); - expect({ shape, outcome }).toMatchObject({ shape, outcome: { kind: 'refused', code: 'MISSING_REQUIRED_FIELD', status: 422 } }); + expect({ shape, outcome }).toMatchObject({ shape, outcome: { kind: 'refused', code: 'INVALID_METADATA', status: 422 } }); expect({ shape, row: (await rig.stored(UNJUDGED, [id]))[id] }).toEqual({ shape, row: { master: free('rls'), note: 'seed' } }); // The control: the same update leaving the reference out lands. const control = await rig.update[shape](UNJUDGED, id, { note: 'written' }, context); @@ -515,10 +510,10 @@ describe('ADR-0055 §1 · §3 — every row a write stores in a controlled_by_pa it('the master the row is stored under is asked first: one the caller may not edit refuses on it', async () => { const context = await rig.contextOf(MEMBER, false); for (const shape of UPDATES) { - const id = await rig.seed(UNJUDGED, held('rls'), MEMBER); - const outcome = await rig.update[shape](UNJUDGED, id, { master: null }, context); + const id = await rig.seed(D.rls, held('rls'), MEMBER); + const outcome = await rig.update[shape](D.rls, id, { master: null }, context); expect({ shape, outcome }).toMatchObject({ shape, outcome: { kind: 'refused', code: 'PERMISSION_DENIED', status: 403 } }); - expect({ shape, row: (await rig.stored(UNJUDGED, [id]))[id] }).toEqual({ shape, row: { master: held('rls'), note: 'seed' } }); + expect({ shape, row: (await rig.stored(D.rls, [id]))[id] }).toEqual({ shape, row: { master: held('rls'), note: 'seed' } }); } }); }); diff --git a/packages/plugins/plugin-security/src/controlled-by-parent-sharing.test.ts b/packages/plugins/plugin-security/src/controlled-by-parent-sharing.test.ts index cac1868247c..f6ff31f4e42 100644 --- a/packages/plugins/plugin-security/src/controlled-by-parent-sharing.test.ts +++ b/packages/plugins/plugin-security/src/controlled-by-parent-sharing.test.ts @@ -48,7 +48,7 @@ import { matchesFilterCondition } from '@objectstack/formula'; // to the source in the checkout rather than a prebuilt dist. import { validateRecord, assertEngineFindOnePredicate } from '@objectstack/objectql'; import type { PermissionSet } from '@objectstack/spec/security'; -import { ObjectSchema } from '@objectstack/spec/data'; +import { ObjectSchema, controlledByParentMasterRefusal } from '@objectstack/spec/data'; const REP = 'usr_rep'; const OTHER = 'usr_other'; @@ -100,11 +100,13 @@ const CONTACT_SCHEMA_NO_MASTER_DETAIL = { * `validateRecord` (`@objectstack/objectql`) walks every declared field on an * insert but skips provenance-flagged ones outright — `if (def.system || * def.readonly) continue;` — before the required check, and never fires on a - * field that is not `required` at all. The `readonly` / `system` shapes below - * are AUTHORABLE (nothing at runtime refuses either flag on a required - * `master_detail`), so on each of them this gate is the only refusal an absent - * master FK meets, and each must still answer 422 — the suite asserts that - * against the REAL validator rather than against a belief about it. + * field that is not `required` at all. On the `readonly` / `system` shapes + * below this gate is therefore the only refusal an absent master FK meets — + * the suite asserts that against the REAL validator rather than against a + * belief about it. Neither is authorable any more: the parse refuses both + * (`master-not-writable`, #25092), so they reach the runtime only through a + * door that does not parse, and there the gate refuses every insert under + * them as the declaration defect, `422 INVALID_METADATA` (#25095). * * [#23100] The first shape, a `master_detail` with no `required`, was the third * member of this set while `resolveCbpRelation`'s second fallback resolved it. @@ -718,8 +720,9 @@ describe('[#5815] getReadFilter enforces the same read scope as the engine middl * 2026-08-15 supersedes #7474's envelope choice for one of its two shapes. An * INSERT that omits the FK is handed to required-field validation (`400 * VALIDATION_FAILED` with `fields[]`) wherever validation provably refuses it; - * the row above still holds for the stored-row shape and for the three - * declarations validation does not cover. The insert leg below was rewritten + * the row above still holds for the stored-row shape. [#25095] Of the three + * declarations validation does not cover, the two flagged ones now answer an + * insert `422 INVALID_METADATA` instead (the third resolves no relation). The insert leg below was rewritten * for that ruling, not adjusted to match observed behaviour — the successor * asserts both sides of the condition. */ @@ -917,7 +920,7 @@ describe('[#7474] the six refusal legs answer with six envelopes, not one', () = }); /** - * [#8688] The three shapes the stand-down deliberately does NOT cover. + * [#8688] The shapes the stand-down deliberately does NOT cover. * * `validateRecord` skips provenance-flagged fields before the required check * (`if (def.system || def.readonly) continue;`) and never fires on a field @@ -926,23 +929,28 @@ describe('[#7474] the six refusal legs answer with six envelopes, not one', () = * (`fk IN (readable masters)`), so readable by nobody, and answering 422 on * every later by-id write. Each case asserts BOTH halves: the gate still * refuses, AND the real validator raises nothing, which is why it must. + * + * [#25095] The gate's answer for the two flagged shapes moved: the parse + * refuses the declaration (`master-not-writable`), and the gate refuses every + * insert under it as that defect, `422 INVALID_METADATA` with the parse's own + * message, before the empty-master branch is reached. */ it.each([ ['`required` + `readonly`', 'master-readonly' as const, CONTACT_SCHEMA_MASTER_READONLY], ['`required` + `system`', 'master-system' as const, CONTACT_SCHEMA_MASTER_SYSTEM], - ])('[#8688] 422 MISSING_REQUIRED_FIELD survives on insert: %s', async (_label, detail, schema) => { + ])('[#8688 → #25095] the gate still refuses an insert omitting the master: %s', async (_label, detail, schema) => { const payload = { name: 'New contact' }; // Validation does not cover this shape — nothing downstream would refuse. expect(() => validateRecord(schema as any, payload, 'insert')).not.toThrow(); - // …so the gate keeps answering, in the envelope and wording it always had. + // …so the gate answers, as the declaration defect the parse refuses. const h = await boot({ shareLevel: 'edit', detail }); const err = await refusalOf(h.insertContact(payload)); - expect(err.code).toBe('MISSING_REQUIRED_FIELD'); + expect(err.code).toBe('INVALID_METADATA'); expect(err.status).toBe(422); expect(err.statusCode).toBe(422); - expect(err.message).toContain("did not supply 'account'"); + expect(err.message).toContain(controlledByParentMasterRefusal(schema.name, schema.sharingModel, schema.fields)!.message); expect(err.message).not.toContain('requires edit access to its master record'); }); diff --git a/packages/plugins/plugin-security/src/controlled-by-parent-unwritable-master-insert.test.ts b/packages/plugins/plugin-security/src/controlled-by-parent-unwritable-master-insert.test.ts deleted file mode 100644 index 098e2cb3228..00000000000 --- a/packages/plugins/plugin-security/src/controlled-by-parent-unwritable-master-insert.test.ts +++ /dev/null @@ -1,343 +0,0 @@ -// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. - -/** - * [ADR-0055 §1] A `controlled_by_parent` detail whose one required - * `master_detail` is not writable (`readonly: true`, `system: true`, or both) - * is a declaration the parse refuses (`controlledByParentMasterRefusal`, shape - * `master-not-writable`). An object reaches the registry with it only through a - * door that does not parse — this suite registers it raw, through - * `registerApp` — and there the write gate holds the parse's line for the one - * write that mints a detail: - * - * - an insert that names a master is refused `422 INVALID_METADATA` with the - * parse's own message, before any master is asked, and nothing is stored — - * a single create and a batch create, for every flag; - * - an insert that names no master keeps the answer it had - * (`422 MISSING_REQUIRED_FIELD`); - * - rows already stored keep their master: the caller still reads them, a - * note-only update lands, and an update that names the master reference - * leaves the row under a master the caller reads; - * - the same detail declared with an ordinary required master takes the same - * inserts, so the refusal is the declaration's and nothing else's; - * - a system write is not the write gate's, and is untouched. - * - * Real stack: `ObjectQL` over better-sqlite3, the real `SecurityPlugin`, the - * real sharing service and middleware, and a member's context assembled by - * `resolveAuthzContext` and `assembleExecutionContext`. - */ - -import { describe, it, expect, vi, beforeAll, afterAll } from 'vitest'; -import { AsyncLocalStorage } from 'node:async_hooks'; -import { ObjectQL } from '@objectstack/objectql'; -import { SqlDriver } from '@objectstack/driver-sql'; -import { resolveAuthzContext, assembleExecutionContext } from '@objectstack/core'; -import { SharingService, buildSharingMiddleware, SysRecordShare } from '@objectstack/plugin-sharing'; -import { PermissionSetSchema } from '@objectstack/spec/security'; -import { ObjectSchema, controlledByParentMasterRefusal } from '@objectstack/spec/data'; -import { SysOrganization, SysUser, SysMember } from '@objectstack/platform-objects/identity'; - -import { SysPosition, SysPermissionSet, SysPositionPermissionSet } from './__tests__/legacy-catalog-objects.testkit.js'; -import { SysUserPosition } from './objects/sys-user-position.object.js'; -import { SysUserPermissionSet } from './objects/sys-user-permission-set.object.js'; -import { SecurityPlugin } from './security-plugin.js'; -import { defaultPermissionSets } from './objects/default-permission-sets.js'; -import { relaxTenantColumns } from './pre-18-tenant-column.testkit.js'; - -const SYS = { context: { isSystem: true } } as never; -const ORG = 'org_cbp_unw'; -const POSTURE = 'isolated' as const; -const MEMBER = 'u_cbp_unw_member'; - -const MASTER = 'qa_cbp_unw_master'; -const MASTER_ROWS = ['qa_cbp_unw_master_1', 'qa_cbp_unw_master_2'] as const; - -/** The flags a master reference may not carry under ADR-0055 §1, one detail each. */ -const FLAGGED = { - readonly: { readonly: true }, - system: { system: true }, - 'readonly and system': { readonly: true, system: true }, -} as const; -type Flagged = keyof typeof FLAGGED; -const detailName = (flags: Flagged | 'none') => `qa_cbp_unw_detail_${flags.replace(/ /g, '_')}`; - -const masterFields = { - name: { name: 'name', type: 'text' }, - owner_id: { name: 'owner_id', type: 'text' }, - created_by: { name: 'created_by', type: 'text' }, -}; -const detailFields = (flags: Record) => ({ - note: { name: 'note', type: 'text' }, - created_by: { name: 'created_by', type: 'text' }, - master: { name: 'master', type: 'master_detail', required: true, reference: MASTER, ...flags }, -}); -const detailOf = (flags: Flagged | 'none') => ({ - name: detailName(flags), - label: detailName(flags), - sharingModel: 'controlled_by_parent', - fields: detailFields(flags === 'none' ? {} : FLAGGED[flags]), -}); - -type Outcome = - | { kind: 'landed' } - | { kind: 'refused'; code?: string; status?: number; message: string }; - -const outcomeOf = (p: Promise): Promise => - p.then( - () => ({ kind: 'landed' as const }), - (e: unknown) => { - const x = e as { code?: unknown; status?: unknown; statusCode?: unknown; message?: unknown } | null; - const status = x?.statusCode ?? x?.status; - return { - kind: 'refused' as const, - code: x?.code == null ? undefined : String(x.code), - status: status == null ? undefined : Number(status), - message: String(x?.message), - }; - }, - ); - -/** See the explain-enforce-parity suite: a torn-down rig's async storages are released once its last row is judged. */ -const releaseAsyncContexts = (...owners: object[]): void => { - for (const owner of owners) { - for (const value of Object.values(owner)) if (value instanceof AsyncLocalStorage) value.disable(); - } -}; - -async function boot() { - const engine = new ObjectQL(); - engine.registerDriver( - new SqlDriver({ client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true }) as never, - true, - ); - await engine.init(); - // A door that does not parse: the declarations go to the registry as written. - engine.registerApp({ - id: 'com.objectstack.qa.cbp-unwritable-master-insert', - name: 'An insert naming a master under a master reference the declaration keeps from the author is refused', - version: '1.0.0', - type: 'plugin', - scope: 'system', - objects: [ - SysPosition, SysPermissionSet, SysPositionPermissionSet, SysUserPosition, SysUserPermissionSet, - SysOrganization, SysUser, SysMember, SysRecordShare, - { name: MASTER, label: 'Master', sharingModel: 'public_read_write', fields: masterFields }, - detailOf('none'), - ...(Object.keys(FLAGGED) as Flagged[]).map(detailOf), - ], - } as never); - await engine.syncSchemas(); - // The organization-less permission-set catalogue row below exists only on a - // store built before 18.x (ADR-0131 D1 makes a fresh tenant column NOT - // NULL): that table is this store's. - await relaxTenantColumns(engine as never, ['sys_permission_set']); - - const e = engine as unknown as { - insert: (o: string, row: Record, opts: never) => Promise; - getDriver: (o: string) => { create: (o: string, row: Record) => Promise }; - logger: { warn: () => void }; - }; - await e.insert('sys_organization', { id: ORG, name: ORG, slug: ORG }, SYS); - await e.insert('sys_user', { id: MEMBER, name: MEMBER, email: `${MEMBER}@example.test` }, SYS); - await e.insert('sys_member', { id: `mem_${MEMBER}`, user_id: MEMBER, organization_id: ORG, role: 'member' }, SYS); - const CRUD = { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true } as const; - const WRITER = 'qa_cbp_unw_writer'; - const sets = [ - ...defaultPermissionSets, - PermissionSetSchema.parse({ - name: WRITER, - objects: { - [MASTER]: { allowRead: true, allowEdit: true }, - [detailName('none')]: CRUD, - ...Object.fromEntries((Object.keys(FLAGGED) as Flagged[]).map((f) => [detailName(f), CRUD])), - }, - }), - ]; - await e.getDriver('sys_permission_set').create('sys_permission_set', { - id: `ps_${WRITER}`, name: WRITER, label: WRITER, organization_id: null, managed_by: 'admin', active: true, - object_permissions: JSON.stringify({}), system_permissions: JSON.stringify([]), - }); - await e.insert('sys_user_permission_set', { id: `ups_${MEMBER}`, user_id: MEMBER, permission_set: WRITER, organization_id: ORG }, SYS); - for (const id of MASTER_ROWS) { - await e.insert(MASTER, { id, name: id, owner_id: MEMBER, created_by: MEMBER, organization_id: ORG }, SYS); - } - - let security: unknown; - let sharing: SharingService | undefined; - const services: Record = { - manifest: { register: vi.fn() }, - objectql: engine, - metadata: { - get: async (type: string, name: string) => - type === 'permission' ? sets.find((s: { name?: string }) => s.name === name) ?? null : engine.getSchema(name) ?? null, - list: async () => sets, - }, - tenancy: { posture: POSTURE }, - get sharing() { return sharing; }, - }; - const ctx = { - logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }, - registerService: (name: string, svc: unknown) => { - if (name === 'security') security = svc; - else services[name] = svc; - }, - hook: () => undefined, - getService: (name: string) => { - if (!(name in services)) throw new Error(`service not registered: ${name}`); - return services[name]; - }, - }; - const plugin = new SecurityPlugin({ fallbackPermissionSet: 'member_default' }); - await plugin.init(ctx as never); - await plugin.start(ctx as never); - sharing = new SharingService({ engine: engine as never, securityService: () => security as never }); - engine.registerMiddleware(buildSharingMiddleware(sharing, ctx.logger) as never, { object: '*' }); - // The row gates log the writes they refuse. - vi.spyOn(e.logger, 'warn').mockImplementation(() => undefined); - - const authz = await resolveAuthzContext({ - userAuthGates: undefined, - apiKeyLifecycle: undefined, - ql: engine, - headers: {}, - getSession: async () => ({ - user: { id: MEMBER }, - session: { id: `sess_${MEMBER}`, userId: MEMBER, activeOrganizationId: ORG }, - }), - tenancyPosture: POSTURE, - } as never); - const member = assembleExecutionContext({ - authz, - oauth: undefined, - localization: undefined, - requestLocale: undefined, - accessToken: undefined, - } as never) as Record; - - return { - member: () => ({ ...member }), - /** The master walk the gate runs once per master it asks about, for counting. */ - walk: () => - vi.spyOn(plugin as unknown as { assertControlledByParentChainEditable: () => Promise }, 'assertControlledByParentChainEditable'), - insert: (detail: string, data: Record | Record[], context: Record) => - outcomeOf(engine.insert(detail, data as never, { context } as never)), - update: (detail: string, id: string, data: Record, context: Record) => - outcomeOf(engine.update(detail, data, { where: { id }, context } as never)), - /** A row as a system read sees it: its master and its note, or `null` when there is none. */ - stored: async (detail: string, id: string) => { - const row = (await engine.findOne(detail, { where: { id }, context: { isSystem: true } } as never)) as - | Record - | null; - return row ? { master: row.master ?? null, note: row.note ?? null } : null; - }, - /** The ids of the rows the caller's own read returns, of those named. */ - readable: async (detail: string, ids: readonly string[], context: Record) => { - const rows = (await engine.find(detail, { where: { id: { $in: [...ids] } }, context } as never)) as Array<{ id: string }>; - return rows.map((r) => r.id).sort(); - }, - /** A detail row written as the platform would write it, under a master. */ - seed: (detail: string, id: string, master: string) => - e.getDriver(detail).create(detail, { id, note: 'seeded', created_by: MEMBER, master, organization_id: ORG }), - systemInsert: (detail: string, row: Record) => outcomeOf(e.insert(detail, row, SYS)), - teardown: async () => { - try { await engine.destroy(); } catch { /* noop */ } - releaseAsyncContexts(engine, plugin); - }, - }; -} - -type Rig = Awaited>; -const FLAG_NAMES = Object.keys(FLAGGED) as Flagged[]; - -/** The parse's own refusal of a flagged declaration — the text the gate's refusal carries. */ -const parseRefusal = (flags: Flagged) => - controlledByParentMasterRefusal(detailName(flags), 'controlled_by_parent', detailFields(FLAGGED[flags])); - -describe('ADR-0055 §1 — an insert naming a master under a master reference the declaration keeps from the author', () => { - let rig: Rig; - beforeAll(async () => { rig = await boot(); }); - afterAll(async () => { await rig.teardown(); }); - - it('each flagged declaration is one the parse refuses, at `fields`, as `master-not-writable`; the ordinary one parses', () => { - for (const flags of FLAG_NAMES) { - expect({ flags, shape: parseRefusal(flags)?.shape }).toEqual({ flags, shape: 'master-not-writable' }); - const parsed = ObjectSchema.safeParse(detailOf(flags)); - expect({ flags, success: parsed.success }).toEqual({ flags, success: false }); - if (!parsed.success) expect(parsed.error.issues.map((i) => i.path.join('.'))).toContain('fields'); - } - expect(ObjectSchema.safeParse(detailOf('none')).success).toBe(true); - }); - - for (const flags of FLAG_NAMES) { - it(`${flags}: a single create naming a master is refused 422 INVALID_METADATA with the parse's message, before any master is asked, and nothing is stored`, async () => { - const detail = detailName(flags); - const walk = rig.walk(); - try { - const id = `${detail}_single`; - const outcome = await rig.insert(detail, { id, note: 'x', master: MASTER_ROWS[0] }, rig.member()); - expect(outcome).toMatchObject({ kind: 'refused', code: 'INVALID_METADATA', status: 422 }); - expect((outcome as { message: string }).message).toBe( - `[Security] Configuration defect: insert on '${detail}' cannot be written. ${parseRefusal(flags)!.message}`, - ); - expect(walk).not.toHaveBeenCalled(); - expect(await rig.stored(detail, id)).toBeNull(); - } finally { - walk.mockRestore(); - } - }); - - it(`${flags}: a batch create is refused whole when any row names a master, and nothing is stored`, async () => { - const detail = detailName(flags); - const ids = [`${detail}_batch_1`, `${detail}_batch_2`]; - const outcome = await rig.insert( - detail, - [{ id: ids[0], note: 'x' }, { id: ids[1], note: 'x', master: MASTER_ROWS[1] }], - rig.member(), - ); - expect(outcome).toMatchObject({ kind: 'refused', code: 'INVALID_METADATA', status: 422 }); - for (const id of ids) expect({ id, row: await rig.stored(detail, id) }).toEqual({ id, row: null }); - }); - - it(`${flags}: a create naming no master keeps its own answer, 422 MISSING_REQUIRED_FIELD`, async () => { - const detail = detailName(flags); - const id = `${detail}_no_master`; - const outcome = await rig.insert(detail, { id, note: 'x' }, rig.member()); - expect(outcome).toMatchObject({ kind: 'refused', code: 'MISSING_REQUIRED_FIELD', status: 422 }); - expect(await rig.stored(detail, id)).toBeNull(); - }); - - it(`${flags}: a stored row keeps its master — still read, a note-only update lands, and an update naming the reference leaves it under a master the caller reads`, async () => { - const detail = detailName(flags); - const id = `${detail}_stored`; - await rig.seed(detail, id, MASTER_ROWS[0]); - expect(await rig.readable(detail, [id], rig.member())).toEqual([id]); - - expect(await rig.update(detail, id, { note: 'written' }, rig.member())).toEqual({ kind: 'landed' }); - expect(await rig.stored(detail, id)).toEqual({ master: MASTER_ROWS[0], note: 'written' }); - - expect(await rig.update(detail, id, { master: MASTER_ROWS[1] }, rig.member())).toEqual({ kind: 'landed' }); - const after = await rig.stored(detail, id); - expect(MASTER_ROWS).toContain(after?.master); - expect(await rig.readable(detail, [id], rig.member())).toEqual([id]); - }); - - it(`${flags}: a system create is not the write gate's, and lands under its master`, async () => { - const detail = detailName(flags); - const id = `${detail}_system`; - expect(await rig.systemInsert(detail, { id, note: 'x', master: MASTER_ROWS[0], organization_id: ORG })).toEqual({ kind: 'landed' }); - expect(await rig.stored(detail, id)).toEqual({ master: MASTER_ROWS[0], note: 'x' }); - }); - } - - it('the same detail with an ordinary required master takes the same creates, and reads them back under that master', async () => { - const detail = detailName('none'); - expect(await rig.insert(detail, { id: `${detail}_single`, note: 'x', master: MASTER_ROWS[0] }, rig.member())).toEqual({ kind: 'landed' }); - expect( - await rig.insert(detail, [{ id: `${detail}_batch_1`, note: 'x', master: MASTER_ROWS[0] }, { id: `${detail}_batch_2`, note: 'x', master: MASTER_ROWS[1] }], rig.member()), - ).toEqual({ kind: 'landed' }); - expect(await rig.stored(detail, `${detail}_single`)).toEqual({ master: MASTER_ROWS[0], note: 'x' }); - expect(await rig.stored(detail, `${detail}_batch_2`)).toEqual({ master: MASTER_ROWS[1], note: 'x' }); - expect(await rig.readable(detail, [`${detail}_single`, `${detail}_batch_1`, `${detail}_batch_2`], rig.member())).toEqual( - [`${detail}_batch_1`, `${detail}_batch_2`, `${detail}_single`], - ); - }); -}); diff --git a/packages/plugins/plugin-security/src/controlled-by-parent-unwritable-master-write.test.ts b/packages/plugins/plugin-security/src/controlled-by-parent-unwritable-master-write.test.ts new file mode 100644 index 00000000000..478aee95d85 --- /dev/null +++ b/packages/plugins/plugin-security/src/controlled-by-parent-unwritable-master-write.test.ts @@ -0,0 +1,451 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [ADR-0055 §1 · #25095, ruling 6345869077] A `controlled_by_parent` detail + * whose one required `master_detail` is not writable (`readonly: true`, + * `system: true`, or both) is a declaration the parse refuses + * (`controlledByParentMasterRefusal`, shape `master-not-writable`). An object + * reaches the registry with it only through a door that does not parse — this + * suite registers it raw, through `registerApp` — and there the write gate + * refuses every write that sets the master reference, `422 INVALID_METADATA` + * with the parse's own message, before any master is asked and before + * anything is written: + * + * - every insert, single or batch, whether or not its body names a master; + * - every update whose body carries the reference key, a master or `null`. + * + * One rule for both flags, for a member and for a platform admin alike, on an + * ordinary object and on a `sys_`-named one (which the engine treats + * differently on insert — the rule does not). What stays as it was: + * + * - rows already stored keep their master: the caller still reads them, an + * update that leaves the key alone lands, and a delete lands; + * - a system write is not the write gate's; + * - a detail declared as the in-repo `controlled_by_parent` objects declare + * theirs (`Field.masterDetail`, `required: true`, through the parse) takes + * the same writes, so the refusal is the declaration's and nothing else's. + * + * Real stack: `ObjectQL` over better-sqlite3, the real `SecurityPlugin`, the + * real sharing service and middleware, a member's context assembled by + * `resolveAuthzContext` and `assembleExecutionContext`, and a platform admin's + * carried posture. + */ + +import { describe, it, expect, vi, beforeAll, afterAll } from 'vitest'; +import { AsyncLocalStorage } from 'node:async_hooks'; +import { ObjectQL } from '@objectstack/objectql'; +import { SqlDriver } from '@objectstack/driver-sql'; +import { resolveAuthzContext, assembleExecutionContext } from '@objectstack/core'; +import { SharingService, buildSharingMiddleware, SysRecordShare } from '@objectstack/plugin-sharing'; +import { PermissionSetSchema } from '@objectstack/spec/security'; +import { ObjectSchema, Field, controlledByParentMasterRefusal } from '@objectstack/spec/data'; +import { ADMIN_FULL_ACCESS } from '@objectstack/spec/identity'; +import { SysOrganization, SysUser, SysMember } from '@objectstack/platform-objects/identity'; + +import { SysPosition, SysPermissionSet, SysPositionPermissionSet } from './__tests__/legacy-catalog-objects.testkit.js'; +import { SysUserPosition } from './objects/sys-user-position.object.js'; +import { SysUserPermissionSet } from './objects/sys-user-permission-set.object.js'; +import { SecurityPlugin } from './security-plugin.js'; +import { defaultPermissionSets } from './objects/default-permission-sets.js'; +import { relaxTenantColumns } from './pre-18-tenant-column.testkit.js'; + +const SYS = { context: { isSystem: true } } as never; +const ORG = 'org_cbp_unw'; +const POSTURE = 'isolated' as const; +const MEMBER = 'u_cbp_unw_member'; +const ADMIN = 'u_cbp_unw_admin'; + +const MASTER = 'qa_cbp_unw_master'; +const MASTER_ROWS = ['qa_cbp_unw_master_1', 'qa_cbp_unw_master_2'] as const; + +/** The flags a master reference may not carry under ADR-0055 §1. */ +const FLAGGED = { + readonly: { readonly: true }, + system: { system: true }, + 'readonly and system': { readonly: true, system: true }, +} as const; +type Flagged = keyof typeof FLAGGED; +const FLAG_NAMES = Object.keys(FLAGGED) as Flagged[]; + +/** An ordinary object, and one whose `sys_` name the engine exempts from its readonly insert strip. */ +const PREFIXES = { ordinary: '', 'sys_-named': 'sys_' } as const; +type Prefix = keyof typeof PREFIXES; +const PREFIX_NAMES = Object.keys(PREFIXES) as Prefix[]; + +const detailName = (prefix: Prefix, flags: Flagged) => `${PREFIXES[prefix]}qa_cbp_unw_${flags.replace(/ /g, '_')}`; +const FLAGGED_DETAILS = PREFIX_NAMES.flatMap((prefix) => + FLAG_NAMES.map((flags) => ({ prefix, flags, name: detailName(prefix, flags) })), +); +/** Declared as the in-repo `controlled_by_parent` objects declare their master, and parsed. */ +const IN_REPO_SHAPE = 'qa_cbp_unw_in_repo_shape'; + +const masterFields = { + name: { name: 'name', type: 'text' }, + owner_id: { name: 'owner_id', type: 'text' }, + created_by: { name: 'created_by', type: 'text' }, +}; +const detailFields = (flags: Flagged) => ({ + note: { name: 'note', type: 'text' }, + created_by: { name: 'created_by', type: 'text' }, + master: { name: 'master', type: 'master_detail', required: true, reference: MASTER, ...FLAGGED[flags] }, +}); +const flaggedDetail = (prefix: Prefix, flags: Flagged) => ({ + name: detailName(prefix, flags), + label: detailName(prefix, flags), + sharingModel: 'controlled_by_parent', + fields: detailFields(flags), +}); +const inRepoShape = () => + ObjectSchema.parse({ + name: IN_REPO_SHAPE, + label: IN_REPO_SHAPE, + sharingModel: 'controlled_by_parent', + fields: { + note: Field.text({ label: 'Note' }), + created_by: Field.text({ label: 'Created by' }), + master: Field.masterDetail(MASTER, { label: 'Master', required: true, deleteBehavior: 'cascade' }), + }, + }); + +/** The parse's own refusal of a flagged declaration — the text the gate's refusal carries. */ +const parseRefusal = (prefix: Prefix, flags: Flagged) => + controlledByParentMasterRefusal(detailName(prefix, flags), 'controlled_by_parent', detailFields(flags)); +const refusedWith = (operation: 'insert' | 'update', prefix: Prefix, flags: Flagged) => ({ + kind: 'refused', + code: 'INVALID_METADATA', + status: 422, + message: + `[Security] Configuration defect: ${operation} on '${detailName(prefix, flags)}' cannot be written. ` + + parseRefusal(prefix, flags)!.message, +}); + +type Outcome = + | { kind: 'landed' } + | { kind: 'refused'; code?: string; status?: number; message: string }; + +const outcomeOf = (p: Promise): Promise => + p.then( + () => ({ kind: 'landed' as const }), + (e: unknown) => { + const x = e as { code?: unknown; status?: unknown; statusCode?: unknown; message?: unknown } | null; + const status = x?.statusCode ?? x?.status; + return { + kind: 'refused' as const, + code: x?.code == null ? undefined : String(x.code), + status: status == null ? undefined : Number(status), + message: String(x?.message), + }; + }, + ); + +/** See the explain-enforce-parity suite: a torn-down rig's async storages are released once its last row is judged. */ +const releaseAsyncContexts = (...owners: object[]): void => { + for (const owner of owners) { + for (const value of Object.values(owner)) if (value instanceof AsyncLocalStorage) value.disable(); + } +}; + +const CALLERS = ['a member', 'a platform admin'] as const; +type Caller = (typeof CALLERS)[number]; +/** A short, id-safe tag per caller, so each case writes rows of its own. */ +const tag = (caller: Caller) => (caller === 'a member' ? 'm' : 'a'); + +async function boot() { + const engine = new ObjectQL(); + engine.registerDriver( + new SqlDriver({ client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true }) as never, + true, + ); + await engine.init(); + // A door that does not parse: the flagged declarations go to the registry as written. + engine.registerApp({ + id: 'com.objectstack.qa.cbp-unwritable-master', + name: 'A write that sets a master reference the declaration keeps from the author is refused', + version: '1.0.0', + type: 'plugin', + scope: 'system', + objects: [ + SysPosition, SysPermissionSet, SysPositionPermissionSet, SysUserPosition, SysUserPermissionSet, + SysOrganization, SysUser, SysMember, SysRecordShare, + { name: MASTER, label: 'Master', sharingModel: 'public_read_write', fields: masterFields }, + inRepoShape(), + ...FLAGGED_DETAILS.map(({ prefix, flags }) => flaggedDetail(prefix, flags)), + ], + } as never); + await engine.syncSchemas(); + // The organization-less permission-set catalogue row below exists only on a + // store built before 18.x (ADR-0131 D1 makes a fresh tenant column NOT + // NULL): that table is this store's. + await relaxTenantColumns(engine as never, ['sys_permission_set']); + + const e = engine as unknown as { + insert: (o: string, row: Record, opts: never) => Promise; + getDriver: (o: string) => { create: (o: string, row: Record) => Promise }; + logger: { warn: () => void }; + }; + await e.insert('sys_organization', { id: ORG, name: ORG, slug: ORG }, SYS); + await e.insert('sys_user', { id: MEMBER, name: MEMBER, email: `${MEMBER}@example.test` }, SYS); + await e.insert('sys_member', { id: `mem_${MEMBER}`, user_id: MEMBER, organization_id: ORG, role: 'member' }, SYS); + const CRUD = { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true } as const; + const WRITER = 'qa_cbp_unw_writer'; + const sets = [ + ...defaultPermissionSets, + PermissionSetSchema.parse({ + name: WRITER, + objects: { + [MASTER]: { allowRead: true, allowEdit: true }, + [IN_REPO_SHAPE]: CRUD, + ...Object.fromEntries(FLAGGED_DETAILS.map(({ name }) => [name, CRUD])), + }, + }), + ]; + await e.getDriver('sys_permission_set').create('sys_permission_set', { + id: `ps_${WRITER}`, name: WRITER, label: WRITER, organization_id: null, managed_by: 'admin', active: true, + object_permissions: JSON.stringify({}), system_permissions: JSON.stringify([]), + }); + await e.insert('sys_user_permission_set', { id: `ups_${MEMBER}`, user_id: MEMBER, permission_set: WRITER, organization_id: ORG }, SYS); + for (const id of MASTER_ROWS) { + await e.insert(MASTER, { id, name: id, owner_id: MEMBER, created_by: MEMBER, organization_id: ORG }, SYS); + } + + let security: unknown; + let sharing: SharingService | undefined; + const services: Record = { + manifest: { register: vi.fn() }, + objectql: engine, + metadata: { + get: async (type: string, name: string) => + type === 'permission' ? sets.find((s: { name?: string }) => s.name === name) ?? null : engine.getSchema(name) ?? null, + list: async () => sets, + }, + tenancy: { posture: POSTURE }, + get sharing() { return sharing; }, + }; + const ctx = { + logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }, + registerService: (name: string, svc: unknown) => { + if (name === 'security') security = svc; + else services[name] = svc; + }, + hook: () => undefined, + getService: (name: string) => { + if (!(name in services)) throw new Error(`service not registered: ${name}`); + return services[name]; + }, + }; + const plugin = new SecurityPlugin({ fallbackPermissionSet: 'member_default' }); + await plugin.init(ctx as never); + await plugin.start(ctx as never); + sharing = new SharingService({ engine: engine as never, securityService: () => security as never }); + engine.registerMiddleware(buildSharingMiddleware(sharing, ctx.logger) as never, { object: '*' }); + // The row gates log the writes they refuse. + vi.spyOn(e.logger, 'warn').mockImplementation(() => undefined); + + const authz = await resolveAuthzContext({ + userAuthGates: undefined, + apiKeyLifecycle: undefined, + ql: engine, + headers: {}, + getSession: async () => ({ + user: { id: MEMBER }, + session: { id: `sess_${MEMBER}`, userId: MEMBER, activeOrganizationId: ORG }, + }), + tenancyPosture: POSTURE, + } as never); + const member = assembleExecutionContext({ + authz, + oauth: undefined, + localization: undefined, + requestLocale: undefined, + accessToken: undefined, + } as never) as Record; + const callers: Record Record> = { + 'a member': () => ({ ...member }), + 'a platform admin': () => ({ + userId: ADMIN, tenantId: ORG, positions: [], permissions: [ADMIN_FULL_ACCESS], posture: 'PLATFORM_ADMIN', + }), + }; + + /** A row as a system read sees it: its master and its note, or `null` when there is none. */ + const stored = async (detail: string, id: string) => { + const row = (await engine.findOne(detail, { where: { id }, context: { isSystem: true } } as never)) as + | Record + | null; + return row ? { master: row.master ?? null, note: row.note ?? null } : null; + }; + + return { + as: (caller: Caller) => callers[caller](), + /** The master walk the gate runs once per master it asks about, for counting. */ + walk: () => + vi.spyOn(plugin as unknown as { assertControlledByParentChainEditable: () => Promise }, 'assertControlledByParentChainEditable'), + /** An insert (one row, or a batch), and what it left stored under each id it named. */ + insert: async (detail: string, rows: Record[], context: Record, batch: boolean) => { + const outcome = await outcomeOf(engine.insert(detail, (batch ? rows : rows[0]) as never, { context } as never)); + const left: Record = {}; + for (const row of rows) left[String(row.id)] = await stored(detail, String(row.id)); + return { outcome, stored: left }; + }, + /** A by-id update, and the row it left. */ + update: async (detail: string, id: string, data: Record, context: Record) => ({ + outcome: await outcomeOf(engine.update(detail, data, { where: { id }, context } as never)), + stored: await stored(detail, id), + }), + /** A by-id delete, and the row it left. */ + remove: async (detail: string, id: string, context: Record) => ({ + outcome: await outcomeOf(engine.delete(detail, { where: { id }, context } as never)), + stored: await stored(detail, id), + }), + stored, + /** The ids of the rows the caller's own read returns, of those named. */ + readable: async (detail: string, ids: readonly string[], context: Record) => { + const rows = (await engine.find(detail, { where: { id: { $in: [...ids] } }, context } as never)) as Array<{ id: string }>; + return rows.map((r) => r.id).sort(); + }, + /** A detail row written as the platform would write it, under a master. */ + seed: async (detail: string, id: string, master: string) => { + await e.getDriver(detail).create(detail, { id, note: 'seeded', created_by: MEMBER, master, organization_id: ORG }); + }, + systemInsert: (detail: string, row: Record) => outcomeOf(e.insert(detail, row, SYS)), + teardown: async () => { + try { await engine.destroy(); } catch { /* noop */ } + releaseAsyncContexts(engine, plugin); + }, + }; +} + +type Rig = Awaited>; + +describe('ADR-0055 §1 — a write that sets a master reference the declaration keeps from the author', () => { + let rig: Rig; + beforeAll(async () => { rig = await boot(); }); + afterAll(async () => { await rig.teardown(); }); + + it('each flagged declaration is one the parse refuses, at `fields`, as `master-not-writable`; the in-repo shape parses', () => { + for (const { prefix, flags, name } of FLAGGED_DETAILS) { + expect({ name, shape: parseRefusal(prefix, flags)?.shape }).toEqual({ name, shape: 'master-not-writable' }); + const parsed = ObjectSchema.safeParse(flaggedDetail(prefix, flags)); + expect({ name, success: parsed.success }).toEqual({ name, success: false }); + if (!parsed.success) expect(parsed.error.issues.map((i) => i.path.join('.'))).toContain('fields'); + } + expect(inRepoShape().fields.master).toMatchObject({ type: 'master_detail', required: true, readonly: false }); + }); + + describe.each(CALLERS)('as %s', (caller) => { + describe.each(FLAGGED_DETAILS)('on $name', ({ prefix, flags, name: detail }) => { + const id = (what: string) => `${detail}_${tag(caller)}_${what}`; + + it('a single create naming a master is refused before any master is asked, and nothing is stored', async () => { + const walk = rig.walk(); + try { + const row = { id: id('single'), note: 'x', master: MASTER_ROWS[0] }; + expect(await rig.insert(detail, [row], rig.as(caller), false)).toEqual({ + outcome: refusedWith('insert', prefix, flags), + stored: { [row.id]: null }, + }); + expect(walk).not.toHaveBeenCalled(); + } finally { + walk.mockRestore(); + } + }); + + it('a single create naming no master is refused the same way, and nothing is stored', async () => { + const row = { id: id('no_master'), note: 'x' }; + expect(await rig.insert(detail, [row], rig.as(caller), false)).toEqual({ + outcome: refusedWith('insert', prefix, flags), + stored: { [row.id]: null }, + }); + }); + + it('a batch create is refused whole, and nothing is stored', async () => { + const rows = [ + { id: id('batch_1'), note: 'x', master: MASTER_ROWS[0] }, + { id: id('batch_2'), note: 'x', master: MASTER_ROWS[1] }, + ]; + expect(await rig.insert(detail, rows, rig.as(caller), true)).toEqual({ + outcome: refusedWith('insert', prefix, flags), + stored: { [rows[0].id]: null, [rows[1].id]: null }, + }); + }); + + it('an update whose body names a master is refused before any master is asked, and the row keeps its own', async () => { + await rig.seed(detail, id('repoint'), MASTER_ROWS[0]); + const walk = rig.walk(); + try { + expect(await rig.update(detail, id('repoint'), { master: MASTER_ROWS[1] }, rig.as(caller))).toEqual({ + outcome: refusedWith('update', prefix, flags), + stored: { master: MASTER_ROWS[0], note: 'seeded' }, + }); + expect(walk).not.toHaveBeenCalled(); + } finally { + walk.mockRestore(); + } + }); + + it('an update whose body empties the reference is refused, and the row keeps its master', async () => { + await rig.seed(detail, id('empty'), MASTER_ROWS[0]); + expect(await rig.update(detail, id('empty'), { note: 'x', master: null }, rig.as(caller))).toEqual({ + outcome: refusedWith('update', prefix, flags), + stored: { master: MASTER_ROWS[0], note: 'seeded' }, + }); + }); + + it('a stored row keeps its master: still read, an update that leaves the key alone lands, and a delete lands', async () => { + const kept = id('kept'); + const gone = id('gone'); + await rig.seed(detail, kept, MASTER_ROWS[0]); + await rig.seed(detail, gone, MASTER_ROWS[1]); + expect(await rig.readable(detail, [kept, gone], rig.as(caller))).toEqual([gone, kept].sort()); + + expect(await rig.update(detail, kept, { note: 'written' }, rig.as(caller))).toEqual({ + outcome: { kind: 'landed' }, + stored: { master: MASTER_ROWS[0], note: 'written' }, + }); + expect(await rig.remove(detail, gone, rig.as(caller))).toEqual({ outcome: { kind: 'landed' }, stored: null }); + expect(await rig.readable(detail, [kept, gone], rig.as(caller))).toEqual([kept]); + }); + }); + + describe(`on ${IN_REPO_SHAPE}, declared as the in-repo controlled_by_parent objects declare their master`, () => { + const id = (what: string) => `${IN_REPO_SHAPE}_${tag(caller)}_${what}`; + + it('single and batch creates land, and read back under their masters', async () => { + const single = await rig.insert(IN_REPO_SHAPE, [{ id: id('single'), note: 'x', master: MASTER_ROWS[0] }], rig.as(caller), false); + expect(single).toEqual({ outcome: { kind: 'landed' }, stored: { [id('single')]: { master: MASTER_ROWS[0], note: 'x' } } }); + const batch = await rig.insert( + IN_REPO_SHAPE, + [{ id: id('batch_1'), note: 'x', master: MASTER_ROWS[0] }, { id: id('batch_2'), note: 'x', master: MASTER_ROWS[1] }], + rig.as(caller), + true, + ); + expect(batch).toEqual({ + outcome: { kind: 'landed' }, + stored: { [id('batch_1')]: { master: MASTER_ROWS[0], note: 'x' }, [id('batch_2')]: { master: MASTER_ROWS[1], note: 'x' } }, + }); + expect(await rig.readable(IN_REPO_SHAPE, [id('single'), id('batch_1'), id('batch_2')], rig.as(caller))).toEqual( + [id('batch_1'), id('batch_2'), id('single')], + ); + }); + + it('an update naming a master re-points the row, one leaving the key alone lands, and a delete lands', async () => { + await rig.seed(IN_REPO_SHAPE, id('repoint'), MASTER_ROWS[0]); + await rig.seed(IN_REPO_SHAPE, id('gone'), MASTER_ROWS[0]); + expect(await rig.update(IN_REPO_SHAPE, id('repoint'), { master: MASTER_ROWS[1] }, rig.as(caller))).toEqual({ + outcome: { kind: 'landed' }, + stored: { master: MASTER_ROWS[1], note: 'seeded' }, + }); + expect(await rig.update(IN_REPO_SHAPE, id('repoint'), { note: 'written' }, rig.as(caller))).toEqual({ + outcome: { kind: 'landed' }, + stored: { master: MASTER_ROWS[1], note: 'written' }, + }); + expect(await rig.remove(IN_REPO_SHAPE, id('gone'), rig.as(caller))).toEqual({ outcome: { kind: 'landed' }, stored: null }); + }); + }); + }); + + it.each(FLAGGED_DETAILS)("a system create on $name is not the write gate's, and lands under its master", async ({ name: detail }) => { + const id = `${detail}_system`; + expect(await rig.systemInsert(detail, { id, note: 'x', master: MASTER_ROWS[0], organization_id: ORG })).toEqual({ kind: 'landed' }); + expect(await rig.stored(detail, id)).toEqual({ master: MASTER_ROWS[0], note: 'x' }); + }); +}); diff --git a/packages/plugins/plugin-security/src/errors.ts b/packages/plugins/plugin-security/src/errors.ts index 4c3cd182f86..473c6b554fd 100644 --- a/packages/plugins/plugin-security/src/errors.ts +++ b/packages/plugins/plugin-security/src/errors.ts @@ -143,9 +143,11 @@ export class MasterDetailRelationMissingError extends Error { } /** - * [ADR-0055 §1] An insert names a master under a `controlled_by_parent` - * declaration whose one required `master_detail` is not writable (`readonly: - * true` or `system: true`) → `422 INVALID_METADATA`. + * [ADR-0055 §1 · #25095] A write sets the master reference of a + * `controlled_by_parent` declaration whose one required `master_detail` is not + * writable (`readonly: true` or `system: true`) → `422 INVALID_METADATA`. + * Every insert sets it, single or batch; an update sets it when its body + * carries the key, whatever the value. * * The parse refuses that declaration (`controlledByParentMasterRefusal`, shape * `master-not-writable`), so an object reaches this only through a door that @@ -153,10 +155,11 @@ export class MasterDetailRelationMissingError extends Error { * `defineStack`, or a stored row the boot registers under its * `[metadata_spec_invalid]` diagnostic. The write gate still derives the * object's READ access from that field, so rows already stored keep their - * master; what it refuses is a new detail. The engine does not treat a - * caller's value in a field the declaration keeps from the author as an - * ordinary master reference, so such a row can land without the master its - * access derives from: readable by nobody, while the insert reports success. + * master, and a delete or an update that leaves the key alone is answered as + * before. The engine does not take a caller's value in a field the + * declaration keeps from the author as an ordinary master reference, so a + * write that sets it could leave a detail outside the master its access + * derives from: readable by nobody, while the write reports success. * * The message is the parse's own, prefixed with the write it refused, so the * prescription is the one `os validate` and the metadata save door give for @@ -229,21 +232,23 @@ export class DetailRecordNotFoundError extends Error { * insert whose master FK is absent and lets `validateRecord` answer, wherever * validation provably covers that omission. * - * This class keeps BOTH shapes, and both are live: + * This class keeps BOTH shapes: * * • the STORED-ROW shape, unconditionally — no reordering of insert-path * validation can reach a persisted null FK, and `fields[]` would name a * field that was never in the request; * • the INSERT shape for the three declarations validation does NOT cover * (a `master_detail` with no `required`; `required` + `readonly`; - * `required` + `system`) — there this gate is the only refusal there is, - * and its 422 is what stops an unreadable orphan detail row being minted. - * The parse now refuses all three (the first since #23100, which also - * took it out of the relation resolver; the two flagged ones since - * #25092), so they arrive only through a door that does not parse. An - * insert that omits the master still gets this answer there; one that - * names a master under a flagged declaration gets - * {@link MasterReferenceNotWritableError}'s. + * `required` + `system`) — there this gate was the only refusal there + * was, and its 422 is what stopped an unreadable orphan detail row being + * minted. The parse now refuses all three (the first since #23100, which + * also took it out of the relation resolver; the two flagged ones since + * #25092), so they arrive only through a door that does not parse, and + * there no insert of theirs reaches this class any more: the first + * resolves no relation ({@link MasterDetailRelationMissingError}), and + * every insert under a flagged one is + * {@link MasterReferenceNotWritableError}'s (#25095). The shape stays as + * the backstop the freeze note at the gate's empty-master branch keeps. * * The wording is unchanged in both, so a surface that pins either sentence sees * the same string it always did — what moved is WHICH inserts arrive here. diff --git a/packages/plugins/plugin-security/src/security-plugin.ts b/packages/plugins/plugin-security/src/security-plugin.ts index a6d4d96f14a..0375af99589 100644 --- a/packages/plugins/plugin-security/src/security-plugin.ts +++ b/packages/plugins/plugin-security/src/security-plugin.ts @@ -684,44 +684,52 @@ interface CbpRelation { */ omissionRefusedByValidation: boolean; /** - * [ADR-0055 §1] The parse's refusal of this declaration when its one master - * reference is not writable (`readonly: true` or `system: true`, the - * `master-not-writable` shape of `controlledByParentMasterRefusal`), else - * `null`. - * - * The relation still resolves for such an object, on purpose: its stored - * rows keep the master their reads derive from, so an object registered - * without the parse does not lose access to rows it already has. What the - * write gate refuses is a NEW detail that names a master in that field - * ({@link MasterReferenceNotWritableError}). The engine does not treat a - * caller's value in a field the declaration keeps from the author as an - * ordinary master reference, so a detail written through it can land outside - * its master. The refusal follows the declaration the parse refuses, not one - * of its flags: one rule and one prescription. + * [ADR-0055 §1 · #25095] The relation's one master reference is not + * writable: its field def carries `readonly: true` or `system: true`, the + * declaration the parse refuses as `master-not-writable`. Then this is the + * parse's own sentence for it, else `null`. + * + * Read from the SAME field def the relation came from, like + * {@link CbpRelation.omissionRefusedByValidation}, and the relation still + * resolves, on purpose: the read leg, the cache and the unresolved report do + * not change, so the rows already stored keep the master their reads derive + * from. What it changes is the write gate's answer + * ({@link MasterReferenceNotWritableError}): every insert, and every update + * whose body carries the reference key, is refused before anything is + * written. The engine does not take a caller's value in a field the + * declaration keeps from the author as an ordinary master reference, so such + * a write could mint or re-point a detail outside the master its access + * derives from. One rule for both flags, with one prescription. */ masterNotWritable: string | null; } /** - * [ADR-0055 §1] {@link CbpRelation.masterNotWritable} for one registered - * object: the parse's refusal message when the object delegates its row access - * to a master and its one master reference is not writable, else `null`. + * [ADR-0055 §1] {@link CbpRelation.masterNotWritable} for a resolved relation: + * the parse's refusal of its one master reference, judged on that one def, + * else `null`. `controlledByParentMasterRefusal` reads `readonly === true` and + * `system === true` on it, so the flags and the sentence are spelled once, in + * the parse, and never re-derived here. */ -function masterNotWritableRefusal(object: string, schema: unknown, fields: Record): string | null { - if (!masterGovernsRowWrites(schema)) return null; - const refusal = controlledByParentMasterRefusal(object, OWD_DELEGATING_ROW_WRITES, fields); +function masterNotWritableRefusal(object: string, fk: string, def: unknown): string | null { + const refusal = controlledByParentMasterRefusal(object, OWD_DELEGATING_ROW_WRITES, { [fk]: def }); return refusal?.shape === 'master-not-writable' ? refusal.message : null; } /** - * [ADR-0055 §1] Does an insert's body name a master in `fk`? A single create's - * body, or any row of a batch create's, carrying a value there. A body that - * names none is the empty-master branch's, unchanged. + * [ADR-0055 §1] Does this write set the master reference `fk`? An insert sets + * every field of the rows it creates, so always, single or batch and whatever + * its body names. An update sets it when its body carries the key as an own + * property, whatever the value, `null` included; an upsert reaches the gate as + * one of those two. A delete, and every other write the gate judges, sets no + * field. */ -function insertNamesMaster(data: unknown, fk: string): boolean { - const names = (row: unknown) => - row != null && typeof row === 'object' && !Array.isArray(row) && (row as Record)[fk] != null; - return Array.isArray(data) ? data.some(names) : names(data); +function writesMasterReference(operation: string, data: unknown, fk: string): boolean { + if (operation === 'insert') return true; + if (operation !== 'update') return false; + const carries = (row: unknown) => + row != null && typeof row === 'object' && !Array.isArray(row) && Object.prototype.hasOwnProperty.call(row, fk); + return Array.isArray(data) ? data.some(carries) : carries(data); } /** @@ -8920,9 +8928,8 @@ export class SecurityPlugin implements Plugin { // mirrors and cannot widen if the resolver ever changes. omissionRefusedByValidation: def?.type === 'master_detail' && !!def?.required && !def?.readonly && !def?.system, - // Asked of the parse's own predicate, never re-derived here, so the - // shape and the prescription are the ones `ObjectSchema` refuses. - masterNotWritable: masterNotWritableRefusal(object, schema, Object.fromEntries(entries)), + // [#25095] From the same def, asked of the parse's own predicate. + masterNotWritable: masterNotWritableRefusal(object, String(found[0]), def), }; } this.cbpRelCache.set(object, rel); @@ -9608,6 +9615,13 @@ export class SecurityPlugin implements Plugin { * An update whose body empties the reference is the insert shape's twin and * is answered by the same rule (#25049). * + * [#25095] The first of them has a sibling, answered before any master is + * asked: a relation whose one master reference is `readonly` or `system` + * ({@link CbpRelation.masterNotWritable}) refuses every write that sets it, + * every insert and every update whose body carries the key, `422 + * INVALID_METADATA` ({@link MasterReferenceNotWritableError}), with the + * parse's own refusal of the declaration. + * * [#7505] A seventh outcome is not a leg of this gate at all: if the store * cannot be read, the engine's own error propagates (typically * `ERR_DATASOURCE_UNAVAILABLE` → `503`) and this gate answers nothing. It @@ -9664,13 +9678,14 @@ export class SecurityPlugin implements Plugin { // narrowing is real — the non-null assertions this branch used to need // (`rel!.fk`) were the load-bearing half of the same defect. if (!rel) throw new MasterDetailRelationMissingError(object, operation); - // [ADR-0055 §1] Also a metadata defect, and answered before any master is - // asked: an insert naming a master under a declaration the parse refuses - // because that one master reference is not writable - // ({@link CbpRelation.masterNotWritable}). Only an insert mints a detail - // this way. The rows already stored keep the master their reads derive - // from, and an update's value there never re-points one outside it. - if (operation === 'insert' && rel.masterNotWritable !== null && insertNamesMaster(opCtx.data, rel.fk)) { + // [ADR-0055 §1 · #25095] Also a metadata defect, and answered before any + // master is asked: a declaration the parse refuses because its one master + // reference is not writable ({@link CbpRelation.masterNotWritable}). Every + // write that sets that reference is refused, for every caller the gate + // judges: an insert (single or batch), and an update whose body carries + // the key. A delete, and an update that leaves the key alone, take the + // path below as before, so the rows already stored keep their master. + if (rel.masterNotWritable !== null && writesMasterReference(operation, opCtx.data, rel.fk)) { throw new MasterReferenceNotWritableError(object, operation, rel.masterNotWritable); } @@ -9792,57 +9807,26 @@ export class SecurityPlugin implements Plugin { // every later by-id write through the stored-row leg below. The residual // envelope asymmetry is confined to exactly those shapes. // - // [#8959, re-measured 2026-09-01] "Confined" is now a PARTIAL publish-time - // bound: of the three shapes above, ONE is fenced at authoring time and - // two are not. The 2026-08-16 ruling (recorded in commit 8abada3ba) ordered a - // ramp with two code legs, of which exactly one has landed. - // Direction 2 (#9138) IS merged: `ObjectSchema.create()` now runs - // `forceCbpMasterDetailRequired` (`packages/spec/src/data/object.zod.ts`) - // — under `controlled_by_parent`, a `master_detail` reference with - // `required` omitted is forced to `required: true`, and an explicit - // `required: false` is refused with a located error. Direction 1 - // (`relationship/master-detail-required` promoted `warning` → `error`, - // scoped to `controlled_by_parent`, #9139) has NOT landed — that card is - // open and `pm:on-hold`, deliberately held for the **v18** boundary - // (`main` is still 17.x as of this note). - // - // ⛔ #9138 landing is NOT "the residue is gone". It narrows the FIRST - // shape only, and only on the builder path: - // • no `required` — no longer newly authorable through - // `ObjectSchema.create()`, but it survives on the raw - // `.parse()`/`.safeParse()` path, which #9138 left untouched ON - // PURPOSE so metadata already at rest keeps loading (its docblock - // says so). Measured: `safeParse` of a `controlled_by_parent` object - // takes both an omitted `required` and an explicit `required: false`. - // ⚠️ [#23100, ruling B] SUPERSEDED for this shape: the parse now - // refuses it, and `resolveCbpRelation` resolves only a required - // `master_detail`, so a non-required one never becomes `rel` and this - // branch is unreachable for it (the object meets the `!rel` 422 - // above instead). The freeze below still binds the other two shapes. - // • `required` + `readonly` and `required` + `system` — STILL newly - // authorable through `ObjectSchema.create()` itself: the force - // returns early on `required === true` and never inspects - // `readonly`/`system`. It in fact MINTS them — a `readonly`/`system` - // master reference authored with no `required` comes out of - // `create()` as `required: true` plus the flag, which is exactly a - // shape `validateRecord` skips. - // ⚠️ [#25092] SUPERSEDED for these two: the parse now refuses them - // (`master-not-writable`), `create()` included, since it ends in the - // parse. They reach this branch only through a door that does not - // parse, and there an insert that names a master never gets here: it - // is refused above ({@link CbpRelation.masterNotWritable}). The - // freeze below still binds the omitted-master insert for both. + // [#8959 → #25095] "Confined" is now a closed publish-time bound. Both + // legs of the 2026-08-16 ruling (recorded in commit 8abada3ba) have + // landed — the builder force (#9138, `forceCbpMasterDetailRequired`) and + // the lint's `error` tier for all three shapes (#9139, + // `relationship/master-detail-required` in + // `packages/lint/src/data-model-rules.ts`) — and the parse refuses all + // three at every door that parses (`controlledByParentMasterRefusal`: a + // missing `required` since #23100 ruling B, `readonly` / `system` on the + // one master since #25092). They reach the runtime only through a door + // that does not parse, and there no insert of theirs gets this far: + // • no `required` — `resolveCbpRelation` resolves only a required + // `master_detail`, so the object meets the `!rel` 422 above; + // • `required` + `readonly` and `required` + `system` — every insert, + // with or without a master in its body, is refused above + // ({@link CbpRelation.masterNotWritable}, ruling 6345869077). // - // And nothing warns on the way past: the lint measured by #8959 is - // unchanged — the predicate is still `def.required !== true` at - // `severity: 'warning'` (`packages/lint/src/data-model-rules.ts`), so the - // two flagged shapes draw NO finding at ANY severity and only the - // un-flagged one draws a warning. A freshly authored detail therefore - // still reaches this branch and its `422`: for the two flagged shapes - // straight through the builder, for the third through raw parse. It is a - // live surface, not a shrinking legacy tail. Re-check this paragraph - // before trusting it — it goes stale when #9139 lands, or when the - // builder force grows to cover the two flagged shapes. + // What still reaches this branch for the two flagged shapes is the + // stored-row shape below. The freeze is no less binding for it: it is + // what keeps an omitted master refused if either refusal above ever + // moves. // // ⛔ [#9137] FREEZE NOTE — maintainer ruling (commit 8abada3ba), Direction 4, // "immediately": until the two legs above both land, this `if` is the From 0d24478467295f7e3e95d81ccc8a225ea0013ddb Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 22 Oct 2026 17:35:46 +0000 Subject: [PATCH 3/4] test(plugin-security): wire the engine's security source in the unwritable-master suite after #25198 The detail read is now the engine's ReadableIds leaf over its master, so the rig fills the engine's security-source slot as a kernel does. The read-access sentence in the error docblock and the changeset names the security plugin, not the write gate. Claude-Session: https://claude.ai/code/session_01LQG7z3CxDLtiMaHLbTFSeB Co-authored-by: Claude --- .changeset/25095-cbp-unwritable-master-write.md | 2 +- .../controlled-by-parent-unwritable-master-write.test.ts | 8 ++++++-- packages/plugins/plugin-security/src/errors.ts | 2 +- 3 files changed, 8 insertions(+), 4 deletions(-) diff --git a/.changeset/25095-cbp-unwritable-master-write.md b/.changeset/25095-cbp-unwritable-master-write.md index ddff093e51d..fde67374531 100644 --- a/.changeset/25095-cbp-unwritable-master-write.md +++ b/.changeset/25095-cbp-unwritable-master-write.md @@ -17,7 +17,7 @@ ADR-0055 §1 derives a `controlled_by_parent` detail's access from exactly one r - **Before.** An insert under that declaration was written when it named a master. An update could set the reference. - **Now.** Every write that sets the reference answers `INVALID_METADATA` (422) and writes nothing, before any master is asked: every insert, single or batch, whether or not it names a master, and every update whose body carries the reference key, whatever the value, `null` included. The message is the parse's own refusal of the declaration, prefixed with the write it refused, so it names the object, the field, each flag and the fix. It is the same for every caller the gate judges. - **Unchanged.** - - The rows already stored: the gate still derives their read access from that field, so they keep their master and are read as before. + - The rows already stored: the security plugin still derives their read access from that field, so they keep their master and are read as before. - A delete, and an update whose body leaves the reference key out. - System contexts, every object declared with an ordinary required master, and every object that does not declare `controlled_by_parent`. diff --git a/packages/plugins/plugin-security/src/controlled-by-parent-unwritable-master-write.test.ts b/packages/plugins/plugin-security/src/controlled-by-parent-unwritable-master-write.test.ts index 478aee95d85..b4120515035 100644 --- a/packages/plugins/plugin-security/src/controlled-by-parent-unwritable-master-write.test.ts +++ b/packages/plugins/plugin-security/src/controlled-by-parent-unwritable-master-write.test.ts @@ -27,8 +27,9 @@ * * Real stack: `ObjectQL` over better-sqlite3, the real `SecurityPlugin`, the * real sharing service and middleware, a member's context assembled by - * `resolveAuthzContext` and `assembleExecutionContext`, and a platform admin's - * carried posture. + * `resolveAuthzContext` and `assembleExecutionContext`, a platform admin's + * carried posture, and the engine's security source wired as a kernel wires + * it, so a detail read composes the engine's `ReadableIds` leaf over its master. */ import { describe, it, expect, vi, beforeAll, afterAll } from 'vitest'; @@ -48,6 +49,7 @@ import { SysUserPermissionSet } from './objects/sys-user-permission-set.object.j import { SecurityPlugin } from './security-plugin.js'; import { defaultPermissionSets } from './objects/default-permission-sets.js'; import { relaxTenantColumns } from './pre-18-tenant-column.testkit.js'; +import { wireEngineSecuritySource } from './engine-security-source.testkit.js'; const SYS = { context: { isSystem: true } } as never; const ORG = 'org_cbp_unw'; @@ -236,6 +238,8 @@ async function boot() { const plugin = new SecurityPlugin({ fallbackPermissionSet: 'member_default' }); await plugin.init(ctx as never); await plugin.start(ctx as never); + // The engine's security source, as `ObjectQLPlugin.init` wires it on a kernel (ADR-0139 D2). + wireEngineSecuritySource(engine, () => plugin); sharing = new SharingService({ engine: engine as never, securityService: () => security as never }); engine.registerMiddleware(buildSharingMiddleware(sharing, ctx.logger) as never, { object: '*' }); // The row gates log the writes they refuse. diff --git a/packages/plugins/plugin-security/src/errors.ts b/packages/plugins/plugin-security/src/errors.ts index 473c6b554fd..e3c68660afe 100644 --- a/packages/plugins/plugin-security/src/errors.ts +++ b/packages/plugins/plugin-security/src/errors.ts @@ -153,7 +153,7 @@ export class MasterDetailRelationMissingError extends Error { * `master-not-writable`), so an object reaches this only through a door that * does not parse: a raw `registerObject` / `registerApp`, a non-strict * `defineStack`, or a stored row the boot registers under its - * `[metadata_spec_invalid]` diagnostic. The write gate still derives the + * `[metadata_spec_invalid]` diagnostic. The security plugin still derives the * object's READ access from that field, so rows already stored keep their * master, and a delete or an update that leaves the key alone is answered as * before. The engine does not take a caller's value in a field the From 5aea100c1554460a0b97e78c3006f312f7b74981 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 22 Oct 2026 18:01:15 +0000 Subject: [PATCH 4/4] docs(error-handling): the controlled_by_parent master-reference rows answer INVALID_METADATA for a readonly or system master This PR changes what the two flagged declarations answer, so the two pages that published the old 422 MISSING_REQUIRED_FIELD rows are corrected here: the stored-null-master shape keeps MISSING_REQUIRED_FIELD (its example now shows that message), the flagged rows answer 422 INVALID_METADATA for every write that sets the reference, and the stale "authorable today" paragraphs, false since the parse refuses both declarations, are removed. Claude-Session: https://claude.ai/code/session_01LQG7z3CxDLtiMaHLbTFSeB Co-authored-by: Claude --- content/docs/api/error-catalog.mdx | 57 +++++++++------- .../docs/protocol/kernel/error-handling.mdx | 67 ++++++++++--------- 2 files changed, 67 insertions(+), 57 deletions(-) diff --git a/content/docs/api/error-catalog.mdx b/content/docs/api/error-catalog.mdx index e2c3da3c04e..81d301bc5da 100644 --- a/content/docs/api/error-catalog.mdx +++ b/content/docs/api/error-catalog.mdx @@ -178,24 +178,35 @@ substitute. See the [Data API](/docs/api/data-api). ### `MISSING_REQUIRED_FIELD` **Cause:** A required field was not provided in the request body. **Fix:** Include the missing field. Check `fields` for the field name — **except** on the -`controlled_by_parent` master-reference paths below, where the status is `422` and no -`fields` array is sent; read the field name out of the message there. +`controlled_by_parent` stored-master path below, where the status is `422` and no `fields` +array is sent; read the field name out of the message there. **Retry:** `no_retry` -**`MISSING_REQUIRED_FIELD` is `400` with one exception: an absent `controlled_by_parent` -master reference answers `422`, without `fields`.** +**`MISSING_REQUIRED_FIELD` is `400` with one exception: a `controlled_by_parent` detail +whose stored master reference is null answers `422`, without `fields`.** An object whose `sharingModel` is `controlled_by_parent` derives its access from a master record, so the gate authorizing writes to it resolves that master *before* the executor — -and the executor is where required-field validation runs. Which of the two refuses first -depends on how the master reference is declared. A `master_detail` that is `required` and -neither `readonly` nor `system` reaches validation and answers the documented -`400 VALIDATION_FAILED` with `fields`. The other two declarable shapes never reach it and -answer `422 MISSING_REQUIRED_FIELD` with no `fields`: - -- a `master_detail` that is `required` + `readonly` -- a `master_detail` that is `required` + `system` +and the executor is where required-field validation runs. An update or delete **by id** of +a detail record whose *stored* master reference is null answers +`422 MISSING_REQUIRED_FIELD`, whatever the declaration: the caller sent no such field, so +nothing could be named in `fields` and no payload would fix it. + +A request that leaves the master reference out, or empties it, does not answer this code. +On a `master_detail` that is `required` and neither `readonly` nor `system`, validation +answers it with the documented `400 VALIDATION_FAILED` and `fields`. A master reference +marked `readonly` or `system` is not one an author's write supplies, so it cannot be the +master (ADR-0055 §1): `ObjectSchema` refuses that declaration at parse +(`master-not-writable`), and on an object that reaches the runtime without the parse, every +write that sets the reference answers `422 INVALID_METADATA`: + +- every insert, single or batch, whether or not it names a master; +- every update whose body carries the reference key, whatever the value, `null` included. + +Its message starts `[Security] Configuration defect:` and carries the parse's own refusal, +naming the object, the field, the flag and the fix. A delete, and an update that leaves the +key out, are answered as on any detail. A `master_detail` with no `required`, and a `required` `lookup` on an object that declares no `master_detail`, are not master references at all: the master is exactly one `master_detail` @@ -205,20 +216,14 @@ one that reaches the runtime unparsed answers every write `422 INVALID_METADATA` The full matrix is on the protocol page: [`MISSING_REQUIRED_FIELD`](/docs/protocol/kernel/error-handling#missing_required_field). -An update or delete **by id** whose *stored* master reference is null answers the same -`422`, whatever the declaration — the caller sent no such field, so nothing could be named -in `fields` and no payload would fix it. - -The refusal itself is correct and is not going to be relaxed: on those two shapes -required-field validation does not fire (it skips `system` and `readonly` fields before its -required check), so the gate is the only -thing standing between the request and a detail record with a null master reference — which -the `controlled_by_parent` read filter (`fk IN (readable masters)`) can never match, leaving -a record readable by nobody. +The `INVALID_METADATA` refusal is not going to be relaxed: required-field validation skips +`system` and `readonly` fields before its required check, so on those two declarations the +gate is the only thing standing between the request and a detail record outside its master +— which the `controlled_by_parent` read filter (`fk IN (readable masters)`) can never match, +leaving a record readable by nobody. -These shapes are **authorable today**: `os lint` reports both at error, but the publish gate -does not run that rule. **Branch on `code`, treat `fields` as optional, and read the status -off the response** rather than deriving it from the table at the end of this page. +**Branch on `code`, treat `fields` as optional, and read the status off the response** +rather than deriving it from the table at the end of this page. ### `INVALID_FORMAT` @@ -973,7 +978,7 @@ async function handleApiCall() { | 404 | `not_found` | `RECORD_NOT_FOUND`, `OBJECT_NOT_FOUND`, `ENDPOINT_NOT_FOUND` | | 409 | `conflict` | `CONCURRENT_MODIFICATION`, `UNIQUE_VIOLATION`, `DELETE_RESTRICTED` | | 413 | `validation` | `PAYLOAD_TOO_LARGE` — an upload over the storage settings' `max_upload_mb`, or an import over its row ceiling (see [above](#payload_too_large)) | -| 422 | `validation` | `MISSING_REQUIRED_FIELD` on an absent `controlled_by_parent` master reference (see [above](#missing_required_field)) — this row is an exception to the 400 row, not a second home for the code | +| 422 | `validation` | `MISSING_REQUIRED_FIELD` on a `controlled_by_parent` detail whose stored master reference is null (see [above](#missing_required_field)) — this row is an exception to the 400 row, not a second home for the code | | 429 | `rate_limit` | `RATE_LIMIT_EXCEEDED`, `QUOTA_EXCEEDED` | | 500 | `server` | `INTERNAL_ERROR`, `DATABASE_ERROR` | | 501 | `server` | `NOT_IMPLEMENTED` — the storage routes answer it when the adapter cannot issue presigned URLs | diff --git a/content/docs/protocol/kernel/error-handling.mdx b/content/docs/protocol/kernel/error-handling.mdx index 09fdffe38f5..18e0b4c6313 100644 --- a/content/docs/protocol/kernel/error-handling.mdx +++ b/content/docs/protocol/kernel/error-handling.mdx @@ -281,7 +281,7 @@ ObjectStack uses standard HTTP status codes: | **404** | Not Found | Resource doesn't exist | | **409** | Conflict | Resource already exists or version mismatch | | **413** | Content Too Large | An upload over the storage settings' `max_upload_mb`, or an import over its row ceiling — answered as [`PAYLOAD_TOO_LARGE`](/docs/api/error-catalog#payload_too_large) | -| **422** | Unprocessable Entity | Semantic validation failed (e.g. metadata spec validation); also an absent `controlled_by_parent` master reference — see [`MISSING_REQUIRED_FIELD`](#missing_required_field) | +| **422** | Unprocessable Entity | Semantic validation failed (e.g. metadata spec validation); also a `controlled_by_parent` detail whose stored master reference is null — see [`MISSING_REQUIRED_FIELD`](#missing_required_field) | | **429** | Too Many Requests | Rate limit exceeded | | **500** | Internal Server Error | Server-side error | | **503** | Service Unavailable | Server overloaded or maintenance | @@ -534,7 +534,7 @@ code is the one the *master-access gate* and the package-publish route use. **Flat** (the 422 exception, on the `/data` door): ```json { - "error": "[Security] Missing master reference: insert on 'invoice_line' did not supply 'invoice_id'. A controlled_by_parent detail derives its access from its master, so 'invoice_id' must carry a master record id on every write.", + "error": "[Security] Missing master reference: update on 'invoice_line' cannot be authorized because record 'line_42' has no value in 'invoice_id'. A controlled_by_parent detail derives its access from its master, so that reference must be populated.", "code": "MISSING_REQUIRED_FIELD", "object": "invoice_line" } @@ -552,24 +552,38 @@ code is the one the *master-access gate* and the package-publish route use. ``` ⛔ Neither body carries a `details` bag. On the flat 422 there is no `fields` array either -— see the table below for why. +— see below for why. -**Exception — an absent `controlled_by_parent` master reference answers 422 with no `fields`.** -An object whose `sharingModel` is `controlled_by_parent` derives its access from a master -record, so the gate that authorizes writes to it must resolve that master *before* the -executor runs — and the executor is where required-field validation lives. When the master -reference is absent, whichever of the two refuses first decides the envelope, and that -depends on how the reference is declared: +**Exception — a `controlled_by_parent` detail whose stored master reference is null answers +422 with no `fields`.** An object whose `sharingModel` is `controlled_by_parent` derives its +access from a master record, so the gate that authorizes writes to it must resolve that +master *before* the executor runs — and the executor is where required-field validation +lives. An update or delete **by id** of a detail record whose *stored* master reference is +null answers `422 MISSING_REQUIRED_FIELD`, whatever the declaration, with the message +prefixed `[Security] Missing master reference:`. The caller supplied no such field, so there +is no request field to name in `fields` and no payload that would fix it. + +A request that leaves the master reference out, or empties it, is answered by whichever +refuses first, and that depends on how the reference is declared: | Master reference declared as | Refused by | Status | `code` | `fields` | |---|---|:---:|---|:---:| | `master_detail` + `required`, not `readonly`/`system` | required-field validation | 400 | `VALIDATION_FAILED` | yes | -| `master_detail` + `required` + `readonly` | the master-access gate | **422** | `MISSING_REQUIRED_FIELD` | **absent** | -| `master_detail` + `required` + `system` | the master-access gate | **422** | `MISSING_REQUIRED_FIELD` | **absent** | - -Only the first row is the documented 400 shape. On the other two, messages are prefixed -`[Security] Missing master reference:` and name the object and the field, but no `fields` -array rides along. +| `master_detail` + `required` + `readonly` | the master-access gate | **422** | `INVALID_METADATA` | **absent** | +| `master_detail` + `required` + `system` | the master-access gate | **422** | `INVALID_METADATA` | **absent** | + +Only the first row is a declaration the parse accepts. A master reference marked `readonly` +or `system` is not one an author's write supplies, so it cannot be the master (ADR-0055 §1): +`ObjectSchema` refuses both at parse (`master-not-writable`). An object reaches the runtime +with one only through a door that does not parse — a raw `registerObject` / `registerApp`, +`defineStack(…, { strict: false })`, or a stored object the boot registers under its +`[metadata_spec_invalid]` diagnostic. There the gate refuses **every** write that sets the +reference, not only one that leaves it out: every insert, single or batch, whether or not it +names a master, and every update whose body carries the key, whatever the value, `null` +included. The message is prefixed `[Security] Configuration defect:` and carries the parse's +own refusal, naming the object, the field, the flag and the fix. A delete, and an update +that leaves the key out, are answered as on any detail, so the rows already stored keep +their master. Two declarations are not rows of this table, because they name **no master at all**. A `controlled_by_parent` object derives its master from exactly one `master_detail` field @@ -579,25 +593,16 @@ required `master_detail` fields. `ObjectSchema` refuses such an object at parse, one that reaches the runtime unparsed has every read denied and every write refused with `422 INVALID_METADATA`, whatever the request carries. -The same `422 MISSING_REQUIRED_FIELD` also answers an update or delete **by id** whose -*stored* master reference is null, whatever the declaration. That is a different path: the -caller supplied no such field, so there is no request field to name in `fields` and no -payload that would fix it. - **Why the gate refuses rather than handing over.** Required-field validation skips provenance-flagged fields before its required check is reached (`system` and `readonly` fields are skipped outright). On those two shapes the gate is the only thing refusing the -write, and letting it through was -measured to create a detail record whose master reference is null — a record the -`controlled_by_parent` read filter (`fk IN (readable masters)`) can never match, so it is -readable by nobody and answers 422 on every later write by id. The refusal is correct; only -its status departs from the rule above. - -**These shapes are authorable today.** `os lint` refuses both at *error* -(`relationship/master-detail-required`), but that rule is not part of the publish gate — -`os build`, `os validate` and the metadata save door do not run it. So a stack can publish -clean and still reach the 422. Branch on `code`, and read the status off the response rather -than deriving it from this page. +write, and letting it through was measured to store a detail record outside its master — a +record the `controlled_by_parent` read filter (`fk IN (readable masters)`) can never match, +so it is readable by nobody. No payload fixes the declaration; only its owner can, by +dropping the flag. + +Branch on `code`, and read the status off the response rather than deriving it from this +page. #### `INVALID_FIELD` **HTTP Status:** 400