diff --git a/.changeset/owner-id-anchor-and-bulk-write-scoping.md b/.changeset/owner-id-anchor-and-bulk-write-scoping.md new file mode 100644 index 0000000000..748519ad04 --- /dev/null +++ b/.changeset/owner-id-anchor-and-bulk-write-scoping.md @@ -0,0 +1,60 @@ +--- +'@objectstack/plugin-security': patch +'@objectstack/objectql': patch +'@objectstack/plugin-sharing': patch +--- + +fix(security): guard the `owner_id` ownership anchor and scope bulk writes to owner-visible rows (#3004, #2982) + +Two write-path holes on the row-ownership anchor (`owner_id`), the column OWD +row-level scoping keys off to decide who may update/delete a record. + +- **#3004 — client-writable, unguarded `owner_id`.** The anchor is deliberately + not `readonly` (ownership is transferable), so the static-readonly strip never + covered it and FLS doesn't gate it by default. A non-privileged writer could + therefore `insert` a record under someone else's name (forge) or `update` one + to a new owner (transfer / disown), evading the owner gate that governs + update/delete. The security middleware (plugin-security step 3.5) now treats + `owner_id` as system-managed for non-privileged writers: on insert an empty + value is auto-stamped to the acting user (batch rows too — previously only the + single-record path stamped, leaving bulk-inserted rows NULL-owned and + invisible to their creator), and a supplied foreign owner is denied; on update + a supplied `owner_id` is a transfer/disown and is denied — the unchanged no-op + echo of a form save is tolerated via a pre-image compare, and a bulk + change-set carrying `owner_id` fails closed. A non-scalar `owner_id` + (array/object) is rejected outright rather than string-coerced, and the + change-set membership test uses own-property semantics so a polluted + prototype cannot spoof an ownership write. Both require the transfer grant + (`allowTransfer`, or `modifyAllRecords` which implies it) to proceed. System + context (`ctx.isSystem`) stays fully exempt (OAuth provisioning / cron + snapshots / seed claims / migrations), and under delegation both principals + must hold the grant (ADR-0090 D10 intersection). Note a REST **import** runs + under the importer's own context (not `isSystem`), so a non-privileged user + importing a CSV whose `owner_id` column names other users is correctly denied + unless they hold the transfer grant — administrators (who carry + `modifyAllRecords`) are unaffected. + +- **#2982 — bulk writes skipped owner scoping on OWD-`private` objects.** A + `update({ multi: true })` / bulk delete rebuilt the driver AST from + `options.where` AFTER the middleware chain, discarding the owner/RLS write + filter that plugin-sharing (`buildWriteFilter`) and plugin-security compose + onto `opCtx.ast` — so a member's bulk write hit every matching row, including + peers'. The engine now seeds `opCtx.ast` from the caller's predicate BEFORE the + chain (the same seam reads use) and hands the middleware-composed AST to + `driver.updateMany` / `driver.deleteMany`, so bulk writes are constrained to the + rows the caller may edit — matching single-id write behavior. `delete` now + applies the same scalar-`id` guard `update` already had, so an id-list bulk + delete (`where: { id: { $in: […] } }, multi: true`) is owner-scoped too, and + both multi branches fail CLOSED (throw) rather than silently rebuilding an + unscoped predicate if the row-scoping AST is ever absent. + + Consequences of routing bulk writes through the AST: the anti-oracle + predicate guard now also applies to bulk `update`/`delete` (a bulk write + filtering on an FLS-unreadable field is rejected, as reads already are), and a + principal-less (no-`userId`, non-system) bulk write on an owner-scoped object + now correctly affects zero rows instead of all of them. + +Proven end-to-end on the real showcase app +(`packages/dogfood/test/owner-anchor-and-bulk-writes.dogfood.test.ts`) and pinned +in the ADR-0096 authz-conformance ledger (`ownership-anchor-guard`, +`bulk-write-owner-scoping`). diff --git a/packages/dogfood/test/authz-conformance.matrix.ts b/packages/dogfood/test/authz-conformance.matrix.ts index d12b0a4dfb..d588cc90bf 100644 --- a/packages/dogfood/test/authz-conformance.matrix.ts +++ b/packages/dogfood/test/authz-conformance.matrix.ts @@ -96,6 +96,12 @@ export const AUTHZ_CONFORMANCE: AuthzPrimitive[] = [ enforcement: 'plugin-security/field-masker.ts + detectForbiddenWrites' }, { id: 'ownership-stamp', summary: 'owner_id auto-stamp on insert', state: 'enforced', enforcement: 'plugin-security/security-plugin.ts (insert owner_id inject)' }, + { id: 'ownership-anchor-guard', summary: 'owner_id is system-managed for non-privileged writers — no client forge (insert) / transfer (update) without the transfer grant (#3004)', state: 'enforced', + enforcement: 'plugin-security/security-plugin.ts step 3.5: insert forging a foreign owner is denied unless allowTransfer/modifyAllRecords (batch rows too); update carrying owner_id is a transfer/disown, denied without the grant — single-id no-op echo tolerated via pre-image compare, bulk change-set fails closed; isSystem exempt', + proof: 'owner-anchor-and-bulk-writes.dogfood.test.ts' }, + { id: 'bulk-write-owner-scoping', summary: 'bulk (multi) update/delete are owner-scoped on OWD-private objects, not just single-id writes (#2982)', state: 'enforced', + enforcement: 'objectql/engine.ts seeds opCtx.ast for no-single-id update/delete BEFORE the middleware chain and hands the composed AST to driver.updateMany/deleteMany, so plugin-sharing buildWriteFilter (owner-match + shares) and plugin-security RLS write filters actually bind bulk writes', + proof: 'owner-anchor-and-bulk-writes.dogfood.test.ts' }, { id: 'record-share', summary: 'manual record shares (sys_record_share)', state: 'enforced', enforcement: 'plugin-sharing/sharing-service.ts buildReadFilter/canEdit' }, { id: 'sharing-rules', summary: 'criteria/owner sharing rules', state: 'enforced', diff --git a/packages/dogfood/test/owner-anchor-and-bulk-writes.dogfood.test.ts b/packages/dogfood/test/owner-anchor-and-bulk-writes.dogfood.test.ts new file mode 100644 index 0000000000..3d58dc4a63 --- /dev/null +++ b/packages/dogfood/test/owner-anchor-and-bulk-writes.dogfood.test.ts @@ -0,0 +1,184 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// #3004 + #2982 — the two write-path holes on the ownership anchor, proven on +// the REAL showcase app end-to-end: +// +// • #3004 `owner_id` forge/transfer: the anchor is SYSTEM-MANAGED for +// non-privileged writers. A plain member can neither plant a record under +// someone else's name (insert forge) nor move a record to another owner +// (update transfer / disown) — that requires the transfer grant +// (`allowTransfer`, or `modifyAllRecords` which implies it). The unchanged +// no-op echo of a form save stays tolerated. +// +// • #2982 bulk (multi) writes: `update({multi:true})` / bulk delete used to +// rebuild the driver AST from `options.where` AFTER the middleware chain, +// so the owner scoping that binds single-id writes never reached bulk +// writes — a member's bulk write hit every matching row, including peers'. +// Now the engine seeds `opCtx.ast` before the chain and hands the +// middleware-composed predicate to the driver, so bulk writes are scoped +// to rows the caller may edit. +// +// @proof: owner-anchor-and-bulk-writes + +import { describe, it, expect, beforeAll, afterAll } from 'vitest'; +import showcaseStack from '@objectstack/example-showcase'; +import { bootStack, type VerifyStack } from '@objectstack/verify'; +import { resolveAuthzContext } from '@objectstack/core'; +import { SecurityPlugin, securityDefaultPermissionSets } from '@objectstack/plugin-security'; +import { PermissionSetSchema } from '@objectstack/spec/security'; + +const OBJ = '/data/showcase_private_note'; +const idOf = (b: any) => b?.id ?? b?.record?.id ?? b?.data?.id ?? b?.recordId; + +// The everyone-anchor baseline deliberately carries NO `allowDelete` (an +// anchor-forbidden bit, ADR-0090 D5) — so the bulk-DELETE proof needs a +// position-style grant bound DIRECTLY to the two members. Deliberately NOT +// `isDefault`: it must never reach the anchor. +const noteDeleteSet = PermissionSetSchema.parse({ + name: 'anchor_note_delete', + label: 'Anchor proof — note delete', + objects: { + showcase_private_note: { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true }, + }, +}); + +describe('owner anchor guard + owner-scoped bulk writes (#3004 / #2982)', () => { + let stack: VerifyStack; + let ql: any; + let adminToken: string; + let aliceToken: string; + let bobToken: string; + let aliceId: string; + let bobId: string; + let aliceNoteId: string; + let bobNoteId: string; + + /** Resolve the SAME authz context the REST entry point would — real + * positions/permissions from the live tables, no hand-built principal. */ + const authzFor = async (token: string) => { + const authService: any = await stack.kernel.getServiceAsync('auth'); + let api: any = authService?.api; + if (!api && typeof authService?.getApi === 'function') api = await authService.getApi(); + const headers = new Headers({ authorization: `Bearer ${token}` }); + return resolveAuthzContext({ + ql, + headers, + getSession: async (h: any) => api?.getSession?.({ headers: h }), + }); + }; + + const ownerOf = async (noteId: string) => + (await ql.findOne('showcase_private_note', { where: { id: noteId }, context: { isSystem: true } }))?.owner_id; + + beforeAll(async () => { + stack = await bootStack(showcaseStack, { + security: new SecurityPlugin({ + defaultPermissionSets: [...securityDefaultPermissionSets, noteDeleteSet], + }), + }); + adminToken = await stack.signIn(); // seed dev admin (platform admin) + aliceToken = await stack.signUp('anchor-alice@verify.test'); + bobToken = await stack.signUp('anchor-bob@verify.test'); + + ql = await stack.kernel.getServiceAsync('objectql'); + const uid = async (email: string) => + (await ql.findOne('sys_user', { where: { email }, context: { isSystem: true } }))?.id; + aliceId = await uid('anchor-alice@verify.test'); + bobId = await uid('anchor-bob@verify.test'); + expect(aliceId).toBeTruthy(); + expect(bobId).toBeTruthy(); + + // Direct per-user grant of the delete set (never anchor-bound). + const SYS = { isSystem: true } as const; + const delSet = await ql.findOne('sys_permission_set', { where: { name: 'anchor_note_delete' }, context: SYS }); + expect(delSet?.id, 'declared delete set seeded').toBeTruthy(); + for (const userId of [aliceId, bobId]) { + await ql.insert('sys_user_permission_set', { user_id: userId, permission_set_id: delSet.id }, { context: { ...SYS } }); + } + + const a = await stack.apiAs(aliceToken, 'POST', OBJ, { title: 'alice note', body: 'a' }); + expect(a.status, 'alice creates her note').toBeLessThan(300); + aliceNoteId = idOf(await a.json()); + const b = await stack.apiAs(bobToken, 'POST', OBJ, { title: 'bob note', body: 'b' }); + expect(b.status, 'bob creates his note').toBeLessThan(300); + bobNoteId = idOf(await b.json()); + }, 120_000); + + afterAll(async () => { await stack?.stop(); }); + + // ── #3004 — forge on insert ──────────────────────────────────────────────── + + it('a member cannot INSERT a record owned by someone else (forge)', async () => { + const r = await stack.apiAs(aliceToken, 'POST', OBJ, { title: 'planted', owner_id: bobId }); + expect(r.status, 'forged-owner insert must be denied').toBeGreaterThanOrEqual(400); + const planted = await ql.findOne('showcase_private_note', { where: { title: 'planted' }, context: { isSystem: true } }); + expect(planted, 'no forged row may exist').toBeFalsy(); + }); + + it('a member may still INSERT with owner_id = self (explicit self-owner)', async () => { + const r = await stack.apiAs(aliceToken, 'POST', OBJ, { title: 'self-owned', owner_id: aliceId }); + expect(r.status).toBeLessThan(300); + expect(await ownerOf(idOf(await r.json()))).toBe(aliceId); + }); + + // ── #3004 — transfer / disown on update ──────────────────────────────────── + + it('a member cannot TRANSFER their own record to another user', async () => { + const r = await stack.apiAs(aliceToken, 'PATCH', `${OBJ}/${aliceNoteId}`, { owner_id: bobId }); + expect(r.status, 'ownership transfer without the grant must be denied').toBeGreaterThanOrEqual(400); + expect(await ownerOf(aliceNoteId), 'owner must be unchanged').toBe(aliceId); + }); + + it('a member cannot DISOWN their record (owner_id: null)', async () => { + const r = await stack.apiAs(aliceToken, 'PATCH', `${OBJ}/${aliceNoteId}`, { owner_id: null }); + expect(r.status, 'disowning must be denied').toBeGreaterThanOrEqual(400); + expect(await ownerOf(aliceNoteId)).toBe(aliceId); + }); + + it('the unchanged no-op echo of a form save is tolerated', async () => { + const r = await stack.apiAs(aliceToken, 'PATCH', `${OBJ}/${aliceNoteId}`, { body: 'edited', owner_id: aliceId }); + expect(r.status, 'echoing the current owner back must not 403').toBeLessThan(300); + expect(await ownerOf(aliceNoteId)).toBe(aliceId); + }); + + it('a privileged caller (modifyAllRecords ⇒ transfer) CAN reassign ownership', async () => { + const r = await stack.apiAs(adminToken, 'PATCH', `${OBJ}/${bobNoteId}`, { owner_id: aliceId }); + expect(r.status, 'platform admin transfer must pass').toBeLessThan(300); + expect(await ownerOf(bobNoteId)).toBe(aliceId); + // hand it back for the bulk proofs below + const back = await stack.apiAs(adminToken, 'PATCH', `${OBJ}/${bobNoteId}`, { owner_id: bobId }); + expect(back.status).toBeLessThan(300); + }); + + // ── #3004 × #2982 — bulk change-set carrying owner_id fails closed ───────── + + it('a bulk update whose change-set carries owner_id fails closed for a member', async () => { + const bobCtx = await authzFor(bobToken); + await expect( + ql.update('showcase_private_note', { owner_id: bobId }, { where: {}, multi: true, context: bobCtx }), + ).rejects.toThrow(/owner_id/); + expect(await ownerOf(aliceNoteId), 'no ownership moved').toBe(aliceId); + }); + + // ── #2982 — bulk writes are owner-scoped (engine surface: flows/tools) ───── + + it('a member bulk UPDATE only touches their own rows, never a peer’s', async () => { + const bobCtx = await authzFor(bobToken); + await ql.update('showcase_private_note', { body: 'bulk-rewrite' }, { where: {}, multi: true, context: bobCtx }); + + const aliceRow = await ql.findOne('showcase_private_note', { where: { id: aliceNoteId }, context: { isSystem: true } }); + const bobRow = await ql.findOne('showcase_private_note', { where: { id: bobNoteId }, context: { isSystem: true } }); + expect(bobRow?.body, 'bob’s own row is updated').toBe('bulk-rewrite'); + expect(aliceRow?.body, 'alice’s row must be untouched by bob’s bulk write').not.toBe('bulk-rewrite'); + }); + + it('a member bulk DELETE only removes their own rows, never a peer’s', async () => { + const bobCtx = await authzFor(bobToken); + await ql.delete('showcase_private_note', { where: {}, multi: true, context: bobCtx }); + + const aliceRow = await ql.findOne('showcase_private_note', { where: { id: aliceNoteId }, context: { isSystem: true } }); + const bobRow = await ql.findOne('showcase_private_note', { where: { id: bobNoteId }, context: { isSystem: true } }); + expect(aliceRow, 'alice’s row survives bob’s bulk delete').toBeTruthy(); + expect(bobRow, 'bob’s own row is deleted').toBeFalsy(); + }); +}); diff --git a/packages/objectql/src/engine.test.ts b/packages/objectql/src/engine.test.ts index ed4048d014..22b3092b06 100644 --- a/packages/objectql/src/engine.test.ts +++ b/packages/objectql/src/engine.test.ts @@ -599,6 +599,105 @@ describe('ObjectQL Engine', () => { }); }); + describe('Bulk write row-scoping — middleware-injected ast (#2982)', () => { + beforeEach(async () => { + engine.registerDriver(mockDriver, true); + await engine.init(); + vi.mocked(SchemaRegistry.getObject).mockReturnValue({ name: 'task', fields: {} } as any); + (mockDriver as any).updateMany = vi.fn().mockResolvedValue(2); + (mockDriver as any).deleteMany = vi.fn().mockResolvedValue(2); + }); + + it('seeds opCtx.ast with the caller predicate and hands the middleware-composed where to updateMany', async () => { + // Regression: the multi branch used to REBUILD the AST from + // `options.where` after the middleware chain ran, so a row-scoping + // filter AND-composed onto opCtx.ast (RLS write policies, the + // sharing plugin's editable-rows filter) never bound the driver + // operation — a member's bulk write touched every matching row. + engine.registerMiddleware(async (opCtx: any, next: () => Promise) => { + if (opCtx.operation === 'update' && opCtx.ast) { + opCtx.ast.where = { $and: [opCtx.ast.where, { owner_id: 'u1' }] }; + } + await next(); + }); + + await engine.update( + 'task', + { status: 'done' }, + { where: { status: 'pending' }, multi: true } as any, + ); + + expect((mockDriver as any).updateMany).toHaveBeenCalledTimes(1); + const [, ast] = (mockDriver as any).updateMany.mock.calls[0]; + expect(ast.where).toEqual({ $and: [{ status: 'pending' }, { owner_id: 'u1' }] }); + }); + + it('seeds opCtx.ast for multi delete and hands the composed where to deleteMany', async () => { + engine.registerMiddleware(async (opCtx: any, next: () => Promise) => { + if (opCtx.operation === 'delete' && opCtx.ast) { + opCtx.ast.where = { $and: [opCtx.ast.where, { owner_id: 'u1' }] }; + } + await next(); + }); + + await engine.delete('task', { where: { status: 'stale' }, multi: true } as any); + + expect((mockDriver as any).deleteMany).toHaveBeenCalledTimes(1); + const [, ast] = (mockDriver as any).deleteMany.mock.calls[0]; + expect(ast.where).toEqual({ $and: [{ status: 'stale' }, { owner_id: 'u1' }] }); + }); + + it('does not seed opCtx.ast for a single-id update (pre-image checks own that path)', async () => { + let seenAst: unknown = 'unset'; + engine.registerMiddleware(async (opCtx: any, next: () => Promise) => { + if (opCtx.operation === 'update') seenAst = opCtx.ast; + await next(); + }); + vi.mocked(mockDriver.update).mockResolvedValue({ id: 't1' } as any); + + await engine.update('task', { status: 'done' }, { where: { id: 't1' } } as any); + + expect(seenAst).toBeUndefined(); + expect(mockDriver.update).toHaveBeenCalledTimes(1); + expect((mockDriver as any).updateMany).not.toHaveBeenCalled(); + }); + + it('seeds opCtx.ast for an id-LIST bulk delete ({id:{$in}}) so it routes to a scoped deleteMany (#2982 parity)', async () => { + // Regression: delete() lacked update()'s scalar-id guard, so an + // operator-object where.id was mistaken for a single id — skipping + // the seed and routing to driver.delete with a garbage id, never + // reaching the owner-scoped deleteMany path. + engine.registerMiddleware(async (opCtx: any, next: () => Promise) => { + if (opCtx.operation === 'delete' && opCtx.ast) { + opCtx.ast.where = { $and: [opCtx.ast.where, { owner_id: 'u1' }] }; + } + await next(); + }); + + await engine.delete('task', { where: { id: { $in: ['a', 'b'] } }, multi: true } as any); + + expect((mockDriver as any).deleteMany).toHaveBeenCalledTimes(1); + expect(mockDriver.delete).not.toHaveBeenCalled(); + const [, ast] = (mockDriver as any).deleteMany.mock.calls[0]; + expect(ast.where).toEqual({ $and: [{ id: { $in: ['a', 'b'] } }, { owner_id: 'u1' }] }); + }); + + it('fails CLOSED (throws) if a hook clears the target id so the multi branch runs without a seeded ast', async () => { + // The only way to reach the multi branch with no seeded ast is a + // beforeUpdate hook clearing input.id after a truthy-id seed skip. + // The old `?? { object, where }` fallback would have silently + // rebuilt an UNSCOPED predicate; we now refuse it. + engine.registerHook('beforeUpdate', async (ctx: any) => { + ctx.input.id = undefined; // force the multi branch, no seeded ast + }); + + await expect( + engine.update('task', { id: 't1', status: 'done' }, { multi: true } as any), + ).rejects.toThrow(/row-scoping AST was not seeded/); + expect((mockDriver as any).updateMany).not.toHaveBeenCalled(); + }); + }); + describe('Expand Related Records', () => { beforeEach(async () => { engine.registerDriver(mockDriver, true); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index e57fb7a1ad..7a01a9e9e7 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -2368,6 +2368,21 @@ export class ObjectQL implements IDataEngine { context: options?.context, }; + // [#2982] A no-single-id update routes to `driver.updateMany` below with + // an AST that used to be REBUILT from `options.where` AFTER the + // middleware chain — so row-scoping filters a middleware AND-composed + // onto `opCtx.ast` (RLS write policies, the sharing plugin's + // editable-rows filter) never reached the driver, and a bulk write hit + // every matching row regardless of ownership. Seed the AST with the + // caller's predicate BEFORE the chain runs — the same seam the read path + // uses — and let the multi branch consume the composed result. Keyed on + // the SAME falsy-`id` test the multi branch dispatches on (below), so the + // seed and the branch never disagree. `where` is included only when + // supplied, mirroring the read path's AST shape. + if (!id) { + opCtx.ast = { object, ...(options?.where !== undefined ? { where: options.where } : {}) } as QueryAST; + } + // [#2948] Snapshot the keys the CALLER supplied, BEFORE any middleware / // beforeUpdate hook stamps server-managed columns (owner/tenant stamp, // `updated_by`/`updated_at`). The static-`readonly` strip below drops only @@ -2441,7 +2456,20 @@ export class ObjectQL implements IDataEngine { if (!opCtx.context?.isSystem) { hookContext.input.data = stripReadonlyFields(updateSchema as any, hookContext.input.data as Record, suppliedKeys, this.logger) as any; } - const ast: QueryAST = { object, where: options.where }; + // [#2982] Consume the middleware-composed AST seeded above, so + // the injected row-scoping (RLS write filter, sharing's + // editable-rows filter) actually binds the driver operation. Fail + // CLOSED if it is somehow absent — rebuilding `{ object, where }` + // here would silently drop every composed filter and reopen the + // unscoped-bulk-write hole this fix closes (AGENTS.md PD #12: no + // lenient fallback that tolerates the broken invariant). + const ast = opCtx.ast; + if (!ast) { + throw new Error( + `[Security] Refusing bulk update on '${object}': row-scoping AST was not seeded ` + + `(a hook cleared the target id after the security filter was composed).`, + ); + } result = await driver.updateMany(object, ast, hookContext.input.data as Record, hookContext.input.options as any); } else { throw new Error('Update requires an ID or options.multi=true'); @@ -2602,10 +2630,19 @@ export class ObjectQL implements IDataEngine { this.assertWriteAllowed(object, 'delete'); const driver = this.getDriver(object); - // Extract ID logic similar to update + // Extract ID logic mirroring update(): only a SCALAR `where.id` means + // "delete one row by primary key". An operator object ({ $in: [...] }, …) + // is a multi-row predicate — treating it as an id would bind the object + // literally (driver.delete(object, {$in:[…]})) and both skip the #2982 AST + // seeding below AND bypass the by-id RLS pre-image check. Leave `id` + // undefined so the call routes to deleteMany with the scoped AST. let id: any = undefined; if (options?.where && typeof options.where === 'object' && 'id' in options.where) { - id = (options.where as Record).id; + const whereId = (options.where as Record).id; + const t = typeof whereId; + if (whereId !== null && (t === 'string' || t === 'number' || t === 'bigint')) { + id = whereId; + } } const opCtx: OperationContext = { @@ -2615,6 +2652,16 @@ export class ObjectQL implements IDataEngine { context: options?.context, }; + // [#2982] Same seam as update: a no-single-id delete routes to + // `driver.deleteMany` with an AST that used to be rebuilt from + // `options.where` after the middleware chain, discarding any row-scoping a + // middleware composed onto `opCtx.ast`. Seed the caller's predicate before + // the chain, keyed on the SAME falsy-`id` test the multi branch dispatches + // on; the multi branch consumes the composed result. + if (!id) { + opCtx.ast = { object, ...(options?.where !== undefined ? { where: options.where } : {}) } as QueryAST; + } + await this.executeWithMiddleware(opCtx, async () => { const hookContext: HookContext = { object, @@ -2645,7 +2692,17 @@ export class ObjectQL implements IDataEngine { await this.cascadeDeleteRelations(object, hookContext.input.id as string | number, opCtx.context); result = await driver.delete(object, hookContext.input.id as string, hookContext.input.options as any); } else if (options?.multi && driver.deleteMany) { - const ast: QueryAST = { object, where: options.where }; + // [#2982] Consume the middleware-composed AST seeded above so the + // injected row-scoping binds the bulk delete. Fail CLOSED if it + // is absent rather than rebuilding an unscoped `{ object, where }` + // (AGENTS.md PD #12). + const ast = opCtx.ast; + if (!ast) { + throw new Error( + `[Security] Refusing bulk delete on '${object}': row-scoping AST was not seeded ` + + `(a hook cleared the target id after the security filter was composed).`, + ); + } result = await driver.deleteMany(object, ast, hookContext.input.options as any); } else { throw new Error('Delete requires an ID or options.multi=true'); diff --git a/packages/plugins/plugin-security/src/security-plugin.test.ts b/packages/plugins/plugin-security/src/security-plugin.test.ts index 25216230ab..0d44bd7b68 100644 --- a/packages/plugins/plugin-security/src/security-plugin.test.ts +++ b/packages/plugins/plugin-security/src/security-plugin.test.ts @@ -180,6 +180,197 @@ describe('SecurityPlugin', () => { expect(opCtx.data.owner_id).toBe('u1'); }); + // ------------------------------------------------------------------------- + // [#3004] `owner_id` anchor guard — the ownership anchor is system-managed + // for non-privileged writers: forging it on insert or transferring it on + // update requires the transfer grant (allowTransfer / modifyAllRecords). + // ------------------------------------------------------------------------- + describe('owner_id anchor guard (#3004)', () => { + const memberSet: PermissionSet = { + name: 'member_default', + label: 'Member', + objects: { '*': { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true } }, + } as any; + const transferSet: PermissionSet = { + name: 'can_transfer', + label: 'Transfer', + objects: { '*': { allowRead: true, allowCreate: true, allowEdit: true, allowTransfer: true } }, + } as any; + const modifyAllSet: PermissionSet = { + name: 'org_admin_like', + label: 'Admin', + objects: { '*': { allowRead: true, allowCreate: true, allowEdit: true, modifyAllRecords: true } }, + } as any; + + const boot = async (sets: PermissionSet[], findOneImpl?: (query: any) => any) => { + const plugin = new SecurityPlugin({ fallbackPermissionSet: 'member_default' }); + const harness = makeMiddlewareCtx({ permissionSets: sets, findOneImpl }); + await plugin.init(harness.ctx); + await plugin.start(harness.ctx); + return harness; + }; + const memberCtx = (extra: Record = {}) => + ({ userId: 'u1', tenantId: 'org-1', positions: [], permissions: [], ...extra }); + + it('insert forging another user as owner is denied for a plain member', async () => { + const harness = await boot([memberSet]); + const opCtx: any = { + object: 'task', operation: 'insert', data: { name: 'A', owner_id: 'victim' }, + context: memberCtx(), + }; + await expect(harness.run(opCtx)).rejects.toThrow(/owner_id.*system-managed/); + }); + + it('insert with owner_id = self passes without any transfer grant', async () => { + const harness = await boot([memberSet]); + const opCtx: any = { + object: 'task', operation: 'insert', data: { name: 'A', owner_id: 'u1' }, + context: memberCtx(), + }; + await harness.run(opCtx); + expect(opCtx.data.owner_id).toBe('u1'); + }); + + it('insert forging another owner passes with allowTransfer', async () => { + const harness = await boot([memberSet, transferSet]); + const opCtx: any = { + object: 'task', operation: 'insert', data: { name: 'A', owner_id: 'colleague' }, + context: memberCtx({ permissions: ['can_transfer'] }), + }; + await harness.run(opCtx); + expect(opCtx.data.owner_id).toBe('colleague'); + }); + + it('insert forging another owner passes with modifyAllRecords (implies transfer)', async () => { + const harness = await boot([memberSet, modifyAllSet]); + const opCtx: any = { + object: 'task', operation: 'insert', data: { name: 'A', owner_id: 'colleague' }, + context: memberCtx({ permissions: ['org_admin_like'] }), + }; + await harness.run(opCtx); + expect(opCtx.data.owner_id).toBe('colleague'); + }); + + it('batch insert: empty owners are stamped per row, a forged row denies the batch', async () => { + const harness = await boot([memberSet]); + const stamped: any = { + object: 'task', operation: 'insert', data: [{ name: 'a' }, { name: 'b', owner_id: '' }], + context: memberCtx(), + }; + await harness.run(stamped); + expect(stamped.data.map((r: any) => r.owner_id)).toEqual(['u1', 'u1']); + + const forged: any = { + object: 'task', operation: 'insert', data: [{ name: 'a' }, { name: 'b', owner_id: 'victim' }], + context: memberCtx(), + }; + await expect(harness.run(forged)).rejects.toThrow(/owner_id.*system-managed/); + }); + + it('update transferring ownership is denied for a plain member', async () => { + const harness = await boot([memberSet], () => ({ id: 't1', owner_id: 'u1' })); + const opCtx: any = { + object: 'task', operation: 'update', data: { id: 't1', owner_id: 'admin' }, + context: memberCtx(), + }; + await expect(harness.run(opCtx)).rejects.toThrow(/owner_id.*system-managed/); + }); + + it('update disowning (owner_id: null) is denied for a plain member', async () => { + const harness = await boot([memberSet], () => ({ id: 't1', owner_id: 'u1' })); + const opCtx: any = { + object: 'task', operation: 'update', data: { id: 't1', owner_id: null }, + context: memberCtx(), + }; + await expect(harness.run(opCtx)).rejects.toThrow(/owner_id.*system-managed/); + }); + + it('update echoing the unchanged owner back (form save) is tolerated', async () => { + const harness = await boot([memberSet], () => ({ id: 't1', owner_id: 'u1' })); + const opCtx: any = { + object: 'task', operation: 'update', data: { id: 't1', name: 'renamed', owner_id: 'u1' }, + context: memberCtx(), + }; + await harness.run(opCtx); // must not throw + }); + + it('update transferring ownership passes with allowTransfer', async () => { + const harness = await boot([memberSet, transferSet], () => ({ id: 't1', owner_id: 'u1' })); + const opCtx: any = { + object: 'task', operation: 'update', data: { id: 't1', owner_id: 'colleague' }, + context: memberCtx({ permissions: ['can_transfer'] }), + }; + await harness.run(opCtx); + expect(opCtx.data.owner_id).toBe('colleague'); + }); + + it('bulk update carrying owner_id fails closed for a plain member (no pre-image to compare)', async () => { + const harness = await boot([memberSet]); + const opCtx: any = { + object: 'task', operation: 'update', data: { owner_id: 'u1' }, + options: { where: { status: 'open' }, multi: true }, + context: memberCtx(), + }; + await expect(harness.run(opCtx)).rejects.toThrow(/owner_id.*system-managed/); + }); + + it('update without owner_id in the change-set is untouched by the guard', async () => { + const harness = await boot([memberSet]); + const opCtx: any = { + object: 'task', operation: 'update', data: { id: 't1', name: 'renamed' }, + context: memberCtx(), + }; + await harness.run(opCtx); // must not throw, no pre-image read needed + }); + + it('update disowning via owner_id:undefined is denied (mongo $set null hazard)', async () => { + const harness = await boot([memberSet], () => ({ id: 't1', owner_id: 'u1' })); + const opCtx: any = { + object: 'task', operation: 'update', data: { id: 't1', owner_id: undefined }, + context: memberCtx(), + }; + await expect(harness.run(opCtx)).rejects.toThrow(/owner_id.*system-managed/); + }); + + it('insert with a NON-SCALAR owner_id (array) is denied, not coerced to self', async () => { + const harness = await boot([memberSet]); + const opCtx: any = { + object: 'task', operation: 'insert', data: { name: 'A', owner_id: ['u1'] }, + context: memberCtx(), + }; + // String(['u1']) === 'u1' would have passed as self; scalar-only rejects it. + await expect(harness.run(opCtx)).rejects.toThrow(/owner_id.*system-managed/); + }); + + it('update echo with a NON-SCALAR owner_id equal-by-coercion is denied', async () => { + const harness = await boot([memberSet], () => ({ id: 't1', owner_id: 'u1' })); + const opCtx: any = { + object: 'task', operation: 'update', data: { id: 't1', owner_id: ['u1'] }, + context: memberCtx(), + }; + await expect(harness.run(opCtx)).rejects.toThrow(/owner_id.*system-managed/); + }); + + it('array-shaped update change-set carrying owner_id fails closed (no silent bypass)', async () => { + const harness = await boot([memberSet]); + const opCtx: any = { + object: 'task', operation: 'update', data: [{ id: 't1', owner_id: 'attacker' }], + context: memberCtx(), + }; + await expect(harness.run(opCtx)).rejects.toThrow(/owner_id.*system-managed/); + }); + + it('a prototype-chain owner_id (not own property) does not trip the guard', async () => { + const harness = await boot([memberSet]); + const proto = { owner_id: 'attacker' }; + const data: any = Object.create(proto); + data.id = 't1'; + data.name = 'renamed'; // only own props; owner_id is inherited + const opCtx: any = { object: 'task', operation: 'update', data, context: memberCtx() }; + await harness.run(opCtx); // must not throw — owner_id is not an own property + }); + }); + it('without org-scoping plugin — strips tenant_isolation RLS so find applies no tenant where', async () => { const plugin = new SecurityPlugin({ fallbackPermissionSet: 'member_default' }); const harness = makeMiddlewareCtx({ permissionSets: [tenantPolicySet] }); diff --git a/packages/plugins/plugin-security/src/security-plugin.ts b/packages/plugins/plugin-security/src/security-plugin.ts index cbaeb79113..981b332306 100644 --- a/packages/plugins/plugin-security/src/security-plugin.ts +++ b/packages/plugins/plugin-security/src/security-plugin.ts @@ -1038,29 +1038,131 @@ export class SecurityPlugin implements Plugin { } } - // 3.5. Auto-inject `owner_id` on insert from the - // ExecutionContext. Without this, the row has `owner_id = NULL` - // and the default `owner_only_writes` RLS policy hides it from - // the very user who just created it. + // 3.5. [#3004] `owner_id` — the row-ownership ANCHOR — is SYSTEM-MANAGED + // for non-privileged writers. It is deliberately not `readonly` in the + // schema (ownership is transferable, see registry.ts applySystemFields), + // so the #2948 static-readonly strip never covers it; FLS doesn't gate it + // by default; and OWD/RLS owner gates key OFF it — whoever controls the + // value controls who may update/delete the row. So the middleware owns + // the anchor: + // + // • INSERT: an empty `owner_id` is auto-stamped to the acting user + // (without this the row has `owner_id = NULL` and the default + // `owner_only_writes` RLS policy hides it from its own creator). + // Batch rows included. A SUPPLIED owner that is NOT the acting user + // is an ownership FORGE — denied unless the caller holds the + // transfer grant (`allowTransfer`, or `modifyAllRecords` which + // implies it). + // • UPDATE: a supplied `owner_id` is an ownership TRANSFER (or a + // disown, when null) — denied without the transfer grant. The + // single-id no-op echo (a form save sending the unchanged owner + // back) is tolerated by comparing against the pre-image; a bulk + // change-set carrying `owner_id` has no pre-image to compare and + // fails CLOSED. + // + // System/boot writes carry `isSystem` and short-circuited the whole + // middleware above — imports, OAuth provisioning, cron snapshots and + // seed claims that legitimately write foreign/NULL owners are unaffected. + // Under delegation (ADR-0090 D10) BOTH principals must hold the grant. // // `organization_id` auto-injection has moved to - // `@objectstack/organizations`. Install that plugin for - // multi-tenant deployments. + // `@objectstack/organizations`; its forge guard is step 3.7 below. if ( - opCtx.operation === 'insert' && + (opCtx.operation === 'insert' || opCtx.operation === 'update') && opCtx.data && - typeof opCtx.data === 'object' && - !Array.isArray(opCtx.data) && - !!opCtx.context?.userId + typeof opCtx.data === 'object' ) { - const fields = await this.getObjectFieldNames(metadata, opCtx.object, ql); - if (fields) { - const data = opCtx.data as Record; - if ( - fields.has('owner_id') && - (data.owner_id == null || data.owner_id === '') - ) { - data.owner_id = opCtx.context!.userId; + const isInsert = opCtx.operation === 'insert'; + const rows = (Array.isArray(opCtx.data) ? opCtx.data : [opCtx.data]) as Record[]; + // Own-property test only — never read `owner_id` through the prototype + // chain (a polluted `Object.prototype.owner_id` must not make ordinary + // field-only updates look like ownership writes). + const writesOwner = (r: unknown): r is Record => + !!r && typeof r === 'object' && !Array.isArray(r) && + Object.prototype.hasOwnProperty.call(r, 'owner_id'); + // A valid owner id is a NON-EMPTY SCALAR. Anything else (array / object / + // boolean / '' ) is neither a stampable self-owner nor a tolerable echo — + // String()-coercing it would let `owner_id: [selfId]` pass as "self" and + // corrupt the anchor into an array (self-lockout). + const isScalarId = (v: unknown): v is string | number => + (typeof v === 'string' && v !== '') || (typeof v === 'number' && Number.isFinite(v)); + + // Cheap pre-check: does any row actually WRITE owner_id? On update this + // skips the whole guard (and the field-set resolution) for the common + // path whose change-set never mentions the anchor. Insert always runs — + // it must stamp an absent owner. + if (isInsert || rows.some(writesOwner)) { + const fields = await this.getObjectFieldNames(metadata, opCtx.object, ql); + if (fields?.has('owner_id')) { + const userId = opCtx.context?.userId; + const denyOwnerWrite = (action: string): never => { + throw new PermissionDeniedError( + `[Security] Access denied: 'owner_id' on '${opCtx.object}' is system-managed — ` + + `${action} requires the transfer grant (allowTransfer or modifyAllRecords)`, + { operation: opCtx.operation, object: opCtx.object, positions, permissionSets: explicitPermissionSets }, + ); + }; + // Lazily evaluated + memoized (incl. a `false` result): the common + // path pays nothing, and ADR-0090 D10 — under delegation BOTH + // principals must independently hold the transfer grant. + let transferGrant: boolean | null = null; + const hasTransferGrant = (): boolean => { + if (transferGrant === null) { + transferGrant = + this.permissionEvaluator.checkObjectPermission('transfer', opCtx.object, permissionSets, { isPrivate: secMeta.isPrivate }) && + (!delegatorSets || this.permissionEvaluator.checkObjectPermission('transfer', opCtx.object, delegatorSets, { isPrivate: secMeta.isPrivate })); + } + return transferGrant; + }; + + if (isInsert) { + for (const row of rows) { + if (!row || typeof row !== 'object' || Array.isArray(row)) continue; + if (!writesOwner(row) || row.owner_id == null || row.owner_id === '') { + // Auto-stamp the acting user (batch rows included — the + // single-record-only stamp left bulk-inserted rows NULL-owned + // and invisible to their creator). + if (userId) row.owner_id = userId; + } else if ( + !(isScalarId(row.owner_id) && userId != null && String(row.owner_id) === String(userId)) && + !hasTransferGrant() + ) { + // A supplied owner that is not the acting user (or not a valid + // scalar id) is a forge — denied without the transfer grant. + denyOwnerWrite('creating a record owned by another user'); + } + } + } else { + // UPDATE — a supplied owner_id is a transfer/disown. Only the + // single-id no-op echo (a form save resending the UNCHANGED owner) + // is tolerated; an array / bulk change-set has no single pre-image + // to compare and fails CLOSED. + const single = !Array.isArray(opCtx.data); + for (const row of rows) { + if (!writesOwner(row)) continue; + if (hasTransferGrant()) break; // authorized transfer — allow every row + let unchanged = false; + if (single && isScalarId(row.owner_id)) { + const targetId = this.extractSingleId(opCtx); + if (targetId != null && this.ql) { + try { + // Read the pre-image under the CALLER's context (NOT + // isSystem): a form echo only makes sense for a row the + // caller can already see. This threads the caller's open + // transaction (an in-tx echo isn't spuriously denied) AND + // closes the owner-enumeration oracle a system read would + // open (a caller who can't read the row gets null → deny, + // indistinguishable from a non-owner). + const pre: any = await this.ql.findOne(opCtx.object, { where: { id: targetId }, context: opCtx.context }); + unchanged = !!pre && pre.owner_id != null && String(pre.owner_id) === String(row.owner_id); + } catch { + unchanged = false; // fail closed + } + } + } + if (!unchanged) denyOwnerWrite(`changing record ownership on ${opCtx.operation}`); + } + } } } } diff --git a/packages/plugins/plugin-sharing/src/sharing-plugin.ts b/packages/plugins/plugin-sharing/src/sharing-plugin.ts index 8c77a3b9df..7adc22ba42 100644 --- a/packages/plugins/plugin-sharing/src/sharing-plugin.ts +++ b/packages/plugins/plugin-sharing/src/sharing-plugin.ts @@ -520,11 +520,14 @@ function composeAnd(existing: unknown, addition: unknown): unknown { typeof addition === 'object' && addition !== null && !Array.isArray(addition) ) { const ex: any = existing; - if (Array.isArray(ex.$and)) { + // Flatten only when `existing` is a PURE `{$and:[…]}` — an object mixing + // `$and` with sibling top-level keys (`{$and:[…], status:'x'}`) must NOT be + // spread, or those siblings are silently DROPPED (a caller's AND-ed + // predicate quietly widening the write — data loss on bulk delete/update). + if (Array.isArray(ex.$and) && Object.keys(ex).length === 1) { return { $and: [...ex.$and, addition] }; } - // Heuristic: if existing has no operator keys, attempt shallow merge; - // otherwise nest into $and to preserve semantics. + // Otherwise nest the whole existing object into $and to preserve semantics. return { $and: [existing, addition] }; } return { $and: [existing, addition] }; diff --git a/packages/spec/src/security/permission.zod.ts b/packages/spec/src/security/permission.zod.ts index 2b5cdea77f..8509d9d422 100644 --- a/packages/spec/src/security/permission.zod.ts +++ b/packages/spec/src/security/permission.zod.ts @@ -34,17 +34,26 @@ export const ObjectPermissionSchema = lazySchema(() => z.object({ /** * Lifecycle Operations. * - * RBAC-gated, operations pending (#1883 / roadmap M2). The + * RBAC-gated, operations pending (#1883 / roadmap M2). The dedicated * `transfer`/`restore`/`purge` ObjectQL operations do not exist yet, but the * permission evaluator PRE-MAPS them to these bits * (`permission-evaluator.ts` OPERATION_TO_PERMISSION): the moment such an * operation is dispatched it is denied unless a resolved permission set * grants the bit (or `modifyAllRecords`). Until the operations ship, - * authoring these bits grants nothing — there is no ungated window either - * way (unmapped destructive ops additionally fail CLOSED via + * authoring `restore`/`purge` grants nothing — there is no ungated window + * either way (unmapped destructive ops additionally fail CLOSED via * DESTRUCTIVE_OPERATIONS, per ADR-0049). + * + * EXCEPTION (#3004): `allowTransfer` is ALREADY ENFORCED today through the + * ordinary `insert`/`update` door, not only the future `transfer` op. The + * ownership anchor `owner_id` is system-managed for non-privileged writers — + * a write that plants a record under another user (insert) or reassigns / + * disowns one (update) is DENIED unless the caller holds `allowTransfer` + * (or `modifyAllRecords`, which implies it). So granting `allowTransfer` + * grants the ownership-write capability now; the dedicated M2 `transfer` + * operation will reuse the same bit. */ - allowTransfer: z.boolean().default(false).describe('[RBAC-gated; operation pending M2] Change record ownership'), + allowTransfer: z.boolean().default(false).describe('[RBAC-gated; ENFORCED now via insert/update owner_id guard, #3004] Change record ownership (assign/reassign/disown owner_id)'), allowRestore: z.boolean().default(false).describe('[RBAC-gated; operation pending M2] Restore from trash (Undelete)'), allowPurge: z.boolean().default(false).describe('[RBAC-gated; operation pending M2] Permanently delete (Hard Delete/GDPR)'),