Skip to content

Commit 7150f1f

Browse files
committed
fix(security): guard owner_id anchor + scope bulk writes to owner-visible rows (#3004, #2982)
owner_id is the row-ownership anchor OWD scoping keys off. Two write-path holes: - #3004: owner_id was accepted verbatim from clients on insert/update with no server guard. It is deliberately not `readonly` (ownership transfers), so the static-readonly strip never covered it and FLS doesn't gate it — a member could forge a record under another user (insert) or transfer/disown one (update), evading the owner gate. plugin-security step 3.5 now treats owner_id as system-managed for non-privileged writers: empty is auto-stamped to the acting user (batch rows too), a foreign owner on insert is denied, and a supplied owner_id on update is a transfer/disown that is denied — the unchanged no-op echo is tolerated via a pre-image compare, a bulk change-set fails closed. Both require the transfer grant (allowTransfer / modifyAllRecords). isSystem stays exempt; delegation intersects both principals (ADR-0090 D10). - #2982: update({multi:true}) / bulk delete rebuilt the driver AST from options.where AFTER the middleware chain, discarding the owner/RLS write filter plugin-sharing 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 before the chain and hands the composed AST to updateMany/deleteMany, matching single-id write scoping. Proven end-to-end (owner-anchor-and-bulk-writes.dogfood.test.ts) and pinned in the authz-conformance ledger.
1 parent 4c46ee0 commit 7150f1f

7 files changed

Lines changed: 547 additions & 19 deletions

File tree

Lines changed: 42 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,42 @@
1+
---
2+
'@objectstack/plugin-security': patch
3+
'@objectstack/objectql': patch
4+
---
5+
6+
fix(security): guard the `owner_id` ownership anchor and scope bulk writes to owner-visible rows (#3004, #2982)
7+
8+
Two write-path holes on the row-ownership anchor (`owner_id`), the column OWD
9+
row-level scoping keys off to decide who may update/delete a record.
10+
11+
- **#3004 — client-writable, unguarded `owner_id`.** The anchor is deliberately
12+
not `readonly` (ownership is transferable), so the static-readonly strip never
13+
covered it and FLS doesn't gate it by default. A non-privileged writer could
14+
therefore `insert` a record under someone else's name (forge) or `update` one
15+
to a new owner (transfer / disown), evading the owner gate that governs
16+
update/delete. The security middleware (plugin-security step 3.5) now treats
17+
`owner_id` as system-managed for non-privileged writers: on insert an empty
18+
value is auto-stamped to the acting user (batch rows too — previously only the
19+
single-record path stamped, leaving bulk-inserted rows NULL-owned and
20+
invisible to their creator), and a supplied foreign owner is denied; on update
21+
a supplied `owner_id` is a transfer/disown and is denied — the unchanged no-op
22+
echo of a form save is tolerated via a pre-image compare, and a bulk
23+
change-set carrying `owner_id` fails closed. Both require the transfer grant
24+
(`allowTransfer`, or `modifyAllRecords` which implies it) to proceed. System
25+
context (`ctx.isSystem`) stays fully exempt (import / OAuth provisioning / cron
26+
snapshots / seed claims), and under delegation both principals must hold the
27+
grant (ADR-0090 D10 intersection).
28+
29+
- **#2982 — bulk writes skipped owner scoping on OWD-`private` objects.** A
30+
`update({ multi: true })` / bulk delete rebuilt the driver AST from
31+
`options.where` AFTER the middleware chain, discarding the owner/RLS write
32+
filter that plugin-sharing (`buildWriteFilter`) and plugin-security compose
33+
onto `opCtx.ast` — so a member's bulk write hit every matching row, including
34+
peers'. The engine now seeds `opCtx.ast` from the caller's predicate BEFORE the
35+
chain (the same seam reads use) and hands the middleware-composed AST to
36+
`driver.updateMany` / `driver.deleteMany`, so bulk writes are constrained to the
37+
rows the caller may edit — matching single-id write behavior.
38+
39+
Proven end-to-end on the real showcase app
40+
(`packages/dogfood/test/owner-anchor-and-bulk-writes.dogfood.test.ts`) and pinned
41+
in the ADR-0096 authz-conformance ledger (`ownership-anchor-guard`,
42+
`bulk-write-owner-scoping`).

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -96,6 +96,12 @@ export const AUTHZ_CONFORMANCE: AuthzPrimitive[] = [
9696
enforcement: 'plugin-security/field-masker.ts + detectForbiddenWrites' },
9797
{ id: 'ownership-stamp', summary: 'owner_id auto-stamp on insert', state: 'enforced',
9898
enforcement: 'plugin-security/security-plugin.ts (insert owner_id inject)' },
99+
{ 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',
100+
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',
101+
proof: 'owner-anchor-and-bulk-writes.dogfood.test.ts' },
102+
{ 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',
103+
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',
104+
proof: 'owner-anchor-and-bulk-writes.dogfood.test.ts' },
99105
{ id: 'record-share', summary: 'manual record shares (sys_record_share)', state: 'enforced',
100106
enforcement: 'plugin-sharing/sharing-service.ts buildReadFilter/canEdit' },
101107
{ id: 'sharing-rules', summary: 'criteria/owner sharing rules', state: 'enforced',
Lines changed: 184 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,184 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
//
3+
// #3004 + #2982 — the two write-path holes on the ownership anchor, proven on
4+
// the REAL showcase app end-to-end:
5+
//
6+
// • #3004 `owner_id` forge/transfer: the anchor is SYSTEM-MANAGED for
7+
// non-privileged writers. A plain member can neither plant a record under
8+
// someone else's name (insert forge) nor move a record to another owner
9+
// (update transfer / disown) — that requires the transfer grant
10+
// (`allowTransfer`, or `modifyAllRecords` which implies it). The unchanged
11+
// no-op echo of a form save stays tolerated.
12+
//
13+
// • #2982 bulk (multi) writes: `update({multi:true})` / bulk delete used to
14+
// rebuild the driver AST from `options.where` AFTER the middleware chain,
15+
// so the owner scoping that binds single-id writes never reached bulk
16+
// writes — a member's bulk write hit every matching row, including peers'.
17+
// Now the engine seeds `opCtx.ast` before the chain and hands the
18+
// middleware-composed predicate to the driver, so bulk writes are scoped
19+
// to rows the caller may edit.
20+
//
21+
// @proof: owner-anchor-and-bulk-writes
22+
23+
import { describe, it, expect, beforeAll, afterAll } from 'vitest';
24+
import showcaseStack from '@objectstack/example-showcase';
25+
import { bootStack, type VerifyStack } from '@objectstack/verify';
26+
import { resolveAuthzContext } from '@objectstack/core';
27+
import { SecurityPlugin, securityDefaultPermissionSets } from '@objectstack/plugin-security';
28+
import { PermissionSetSchema } from '@objectstack/spec/security';
29+
30+
const OBJ = '/data/showcase_private_note';
31+
const idOf = (b: any) => b?.id ?? b?.record?.id ?? b?.data?.id ?? b?.recordId;
32+
33+
// The everyone-anchor baseline deliberately carries NO `allowDelete` (an
34+
// anchor-forbidden bit, ADR-0090 D5) — so the bulk-DELETE proof needs a
35+
// position-style grant bound DIRECTLY to the two members. Deliberately NOT
36+
// `isDefault`: it must never reach the anchor.
37+
const noteDeleteSet = PermissionSetSchema.parse({
38+
name: 'anchor_note_delete',
39+
label: 'Anchor proof — note delete',
40+
objects: {
41+
showcase_private_note: { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true },
42+
},
43+
});
44+
45+
describe('owner anchor guard + owner-scoped bulk writes (#3004 / #2982)', () => {
46+
let stack: VerifyStack;
47+
let ql: any;
48+
let adminToken: string;
49+
let aliceToken: string;
50+
let bobToken: string;
51+
let aliceId: string;
52+
let bobId: string;
53+
let aliceNoteId: string;
54+
let bobNoteId: string;
55+
56+
/** Resolve the SAME authz context the REST entry point would — real
57+
* positions/permissions from the live tables, no hand-built principal. */
58+
const authzFor = async (token: string) => {
59+
const authService: any = await stack.kernel.getServiceAsync('auth');
60+
let api: any = authService?.api;
61+
if (!api && typeof authService?.getApi === 'function') api = await authService.getApi();
62+
const headers = new Headers({ authorization: `Bearer ${token}` });
63+
return resolveAuthzContext({
64+
ql,
65+
headers,
66+
getSession: async (h: any) => api?.getSession?.({ headers: h }),
67+
});
68+
};
69+
70+
const ownerOf = async (noteId: string) =>
71+
(await ql.findOne('showcase_private_note', { where: { id: noteId }, context: { isSystem: true } }))?.owner_id;
72+
73+
beforeAll(async () => {
74+
stack = await bootStack(showcaseStack, {
75+
security: new SecurityPlugin({
76+
defaultPermissionSets: [...securityDefaultPermissionSets, noteDeleteSet],
77+
}),
78+
});
79+
adminToken = await stack.signIn(); // seed dev admin (platform admin)
80+
aliceToken = await stack.signUp('anchor-alice@verify.test');
81+
bobToken = await stack.signUp('anchor-bob@verify.test');
82+
83+
ql = await stack.kernel.getServiceAsync('objectql');
84+
const uid = async (email: string) =>
85+
(await ql.findOne('sys_user', { where: { email }, context: { isSystem: true } }))?.id;
86+
aliceId = await uid('anchor-alice@verify.test');
87+
bobId = await uid('anchor-bob@verify.test');
88+
expect(aliceId).toBeTruthy();
89+
expect(bobId).toBeTruthy();
90+
91+
// Direct per-user grant of the delete set (never anchor-bound).
92+
const SYS = { isSystem: true } as const;
93+
const delSet = await ql.findOne('sys_permission_set', { where: { name: 'anchor_note_delete' }, context: SYS });
94+
expect(delSet?.id, 'declared delete set seeded').toBeTruthy();
95+
for (const userId of [aliceId, bobId]) {
96+
await ql.insert('sys_user_permission_set', { user_id: userId, permission_set_id: delSet.id }, { context: { ...SYS } });
97+
}
98+
99+
const a = await stack.apiAs(aliceToken, 'POST', OBJ, { title: 'alice note', body: 'a' });
100+
expect(a.status, 'alice creates her note').toBeLessThan(300);
101+
aliceNoteId = idOf(await a.json());
102+
const b = await stack.apiAs(bobToken, 'POST', OBJ, { title: 'bob note', body: 'b' });
103+
expect(b.status, 'bob creates his note').toBeLessThan(300);
104+
bobNoteId = idOf(await b.json());
105+
}, 120_000);
106+
107+
afterAll(async () => { await stack?.stop(); });
108+
109+
// ── #3004 — forge on insert ────────────────────────────────────────────────
110+
111+
it('a member cannot INSERT a record owned by someone else (forge)', async () => {
112+
const r = await stack.apiAs(aliceToken, 'POST', OBJ, { title: 'planted', owner_id: bobId });
113+
expect(r.status, 'forged-owner insert must be denied').toBeGreaterThanOrEqual(400);
114+
const planted = await ql.findOne('showcase_private_note', { where: { title: 'planted' }, context: { isSystem: true } });
115+
expect(planted, 'no forged row may exist').toBeFalsy();
116+
});
117+
118+
it('a member may still INSERT with owner_id = self (explicit self-owner)', async () => {
119+
const r = await stack.apiAs(aliceToken, 'POST', OBJ, { title: 'self-owned', owner_id: aliceId });
120+
expect(r.status).toBeLessThan(300);
121+
expect(await ownerOf(idOf(await r.json()))).toBe(aliceId);
122+
});
123+
124+
// ── #3004 — transfer / disown on update ────────────────────────────────────
125+
126+
it('a member cannot TRANSFER their own record to another user', async () => {
127+
const r = await stack.apiAs(aliceToken, 'PATCH', `${OBJ}/${aliceNoteId}`, { owner_id: bobId });
128+
expect(r.status, 'ownership transfer without the grant must be denied').toBeGreaterThanOrEqual(400);
129+
expect(await ownerOf(aliceNoteId), 'owner must be unchanged').toBe(aliceId);
130+
});
131+
132+
it('a member cannot DISOWN their record (owner_id: null)', async () => {
133+
const r = await stack.apiAs(aliceToken, 'PATCH', `${OBJ}/${aliceNoteId}`, { owner_id: null });
134+
expect(r.status, 'disowning must be denied').toBeGreaterThanOrEqual(400);
135+
expect(await ownerOf(aliceNoteId)).toBe(aliceId);
136+
});
137+
138+
it('the unchanged no-op echo of a form save is tolerated', async () => {
139+
const r = await stack.apiAs(aliceToken, 'PATCH', `${OBJ}/${aliceNoteId}`, { body: 'edited', owner_id: aliceId });
140+
expect(r.status, 'echoing the current owner back must not 403').toBeLessThan(300);
141+
expect(await ownerOf(aliceNoteId)).toBe(aliceId);
142+
});
143+
144+
it('a privileged caller (modifyAllRecords ⇒ transfer) CAN reassign ownership', async () => {
145+
const r = await stack.apiAs(adminToken, 'PATCH', `${OBJ}/${bobNoteId}`, { owner_id: aliceId });
146+
expect(r.status, 'platform admin transfer must pass').toBeLessThan(300);
147+
expect(await ownerOf(bobNoteId)).toBe(aliceId);
148+
// hand it back for the bulk proofs below
149+
const back = await stack.apiAs(adminToken, 'PATCH', `${OBJ}/${bobNoteId}`, { owner_id: bobId });
150+
expect(back.status).toBeLessThan(300);
151+
});
152+
153+
// ── #3004 × #2982 — bulk change-set carrying owner_id fails closed ─────────
154+
155+
it('a bulk update whose change-set carries owner_id fails closed for a member', async () => {
156+
const bobCtx = await authzFor(bobToken);
157+
await expect(
158+
ql.update('showcase_private_note', { owner_id: bobId }, { where: {}, multi: true, context: bobCtx }),
159+
).rejects.toThrow(/owner_id/);
160+
expect(await ownerOf(aliceNoteId), 'no ownership moved').toBe(aliceId);
161+
});
162+
163+
// ── #2982 — bulk writes are owner-scoped (engine surface: flows/tools) ─────
164+
165+
it('a member bulk UPDATE only touches their own rows, never a peer’s', async () => {
166+
const bobCtx = await authzFor(bobToken);
167+
await ql.update('showcase_private_note', { body: 'bulk-rewrite' }, { where: {}, multi: true, context: bobCtx });
168+
169+
const aliceRow = await ql.findOne('showcase_private_note', { where: { id: aliceNoteId }, context: { isSystem: true } });
170+
const bobRow = await ql.findOne('showcase_private_note', { where: { id: bobNoteId }, context: { isSystem: true } });
171+
expect(bobRow?.body, 'bob’s own row is updated').toBe('bulk-rewrite');
172+
expect(aliceRow?.body, 'alice’s row must be untouched by bob’s bulk write').not.toBe('bulk-rewrite');
173+
});
174+
175+
it('a member bulk DELETE only removes their own rows, never a peer’s', async () => {
176+
const bobCtx = await authzFor(bobToken);
177+
await ql.delete('showcase_private_note', { where: {}, multi: true, context: bobCtx });
178+
179+
const aliceRow = await ql.findOne('showcase_private_note', { where: { id: aliceNoteId }, context: { isSystem: true } });
180+
const bobRow = await ql.findOne('showcase_private_note', { where: { id: bobNoteId }, context: { isSystem: true } });
181+
expect(aliceRow, 'alice’s row survives bob’s bulk delete').toBeTruthy();
182+
expect(bobRow, 'bob’s own row is deleted').toBeFalsy();
183+
});
184+
});

packages/objectql/src/engine.test.ts

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -599,6 +599,70 @@ describe('ObjectQL Engine', () => {
599599
});
600600
});
601601

602+
describe('Bulk write row-scoping — middleware-injected ast (#2982)', () => {
603+
beforeEach(async () => {
604+
engine.registerDriver(mockDriver, true);
605+
await engine.init();
606+
vi.mocked(SchemaRegistry.getObject).mockReturnValue({ name: 'task', fields: {} } as any);
607+
(mockDriver as any).updateMany = vi.fn().mockResolvedValue(2);
608+
(mockDriver as any).deleteMany = vi.fn().mockResolvedValue(2);
609+
});
610+
611+
it('seeds opCtx.ast with the caller predicate and hands the middleware-composed where to updateMany', async () => {
612+
// Regression: the multi branch used to REBUILD the AST from
613+
// `options.where` after the middleware chain ran, so a row-scoping
614+
// filter AND-composed onto opCtx.ast (RLS write policies, the
615+
// sharing plugin's editable-rows filter) never bound the driver
616+
// operation — a member's bulk write touched every matching row.
617+
engine.registerMiddleware(async (opCtx: any, next: () => Promise<void>) => {
618+
if (opCtx.operation === 'update' && opCtx.ast) {
619+
opCtx.ast.where = { $and: [opCtx.ast.where, { owner_id: 'u1' }] };
620+
}
621+
await next();
622+
});
623+
624+
await engine.update(
625+
'task',
626+
{ status: 'done' },
627+
{ where: { status: 'pending' }, multi: true } as any,
628+
);
629+
630+
expect((mockDriver as any).updateMany).toHaveBeenCalledTimes(1);
631+
const [, ast] = (mockDriver as any).updateMany.mock.calls[0];
632+
expect(ast.where).toEqual({ $and: [{ status: 'pending' }, { owner_id: 'u1' }] });
633+
});
634+
635+
it('seeds opCtx.ast for multi delete and hands the composed where to deleteMany', async () => {
636+
engine.registerMiddleware(async (opCtx: any, next: () => Promise<void>) => {
637+
if (opCtx.operation === 'delete' && opCtx.ast) {
638+
opCtx.ast.where = { $and: [opCtx.ast.where, { owner_id: 'u1' }] };
639+
}
640+
await next();
641+
});
642+
643+
await engine.delete('task', { where: { status: 'stale' }, multi: true } as any);
644+
645+
expect((mockDriver as any).deleteMany).toHaveBeenCalledTimes(1);
646+
const [, ast] = (mockDriver as any).deleteMany.mock.calls[0];
647+
expect(ast.where).toEqual({ $and: [{ status: 'stale' }, { owner_id: 'u1' }] });
648+
});
649+
650+
it('does not seed opCtx.ast for a single-id update (pre-image checks own that path)', async () => {
651+
let seenAst: unknown = 'unset';
652+
engine.registerMiddleware(async (opCtx: any, next: () => Promise<void>) => {
653+
if (opCtx.operation === 'update') seenAst = opCtx.ast;
654+
await next();
655+
});
656+
vi.mocked(mockDriver.update).mockResolvedValue({ id: 't1' } as any);
657+
658+
await engine.update('task', { status: 'done' }, { where: { id: 't1' } } as any);
659+
660+
expect(seenAst).toBeUndefined();
661+
expect(mockDriver.update).toHaveBeenCalledTimes(1);
662+
expect((mockDriver as any).updateMany).not.toHaveBeenCalled();
663+
});
664+
});
665+
602666
describe('Expand Related Records', () => {
603667
beforeEach(async () => {
604668
engine.registerDriver(mockDriver, true);

0 commit comments

Comments
 (0)