Skip to content

Commit a16972b

Browse files
os-zhuangclaude
andauthored
fix(security): guard owner_id anchor + scope bulk writes to owner-visible rows (#3004, #2982) (#3018)
owner_id is the row-ownership anchor OWD scoping keys off. Close two write-path holes: - #3004: owner_id was accepted verbatim from clients on insert/update with no server guard (it is deliberately not readonly, and FLS doesn't gate it), letting a member forge a record under another user or transfer/disown one — evading the owner gate. plugin-security now treats owner_id as system-managed for non-privileged writers: empty auto-stamps to the acting user (batch rows too), a foreign owner is denied, and a supplied owner_id on update is a transfer/disown denied unless the caller holds allowTransfer/modifyAllRecords — the single-id no-op echo is tolerated via a caller-scoped pre-image compare. Non-scalar owner_id is rejected (not coerced), own-property membership prevents prototype spoofing, and array change-sets fail closed. isSystem stays exempt; delegation intersects both principals (ADR-0090 D10). - #2982: bulk update/delete rebuilt the driver AST from options.where after the middleware chain, discarding the owner/RLS write filter — a member's bulk write hit every matching row. The engine now seeds opCtx.ast before the chain and hands the composed AST to updateMany/deleteMany; delete gains update's scalar-id guard so id-list bulk deletes are scoped too, and both multi branches fail closed rather than rebuilding an unscoped predicate. plugin-sharing composeAnd preserves sibling keys. Proven end-to-end (owner-anchor-and-bulk-writes.dogfood.test.ts) and pinned in the authz-conformance ledger. Follow-ups: #3022 (public-form forge), #3023 (cascade set_null). Co-Authored-By: Claude Opus 4.8 <noreply@anthropic.com>
1 parent 3ae1639 commit a16972b

9 files changed

Lines changed: 740 additions & 29 deletions

File tree

Lines changed: 60 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,60 @@
1+
---
2+
'@objectstack/plugin-security': patch
3+
'@objectstack/objectql': patch
4+
'@objectstack/plugin-sharing': patch
5+
---
6+
7+
fix(security): guard the `owner_id` ownership anchor and scope bulk writes to owner-visible rows (#3004, #2982)
8+
9+
Two write-path holes on the row-ownership anchor (`owner_id`), the column OWD
10+
row-level scoping keys off to decide who may update/delete a record.
11+
12+
- **#3004 — client-writable, unguarded `owner_id`.** The anchor is deliberately
13+
not `readonly` (ownership is transferable), so the static-readonly strip never
14+
covered it and FLS doesn't gate it by default. A non-privileged writer could
15+
therefore `insert` a record under someone else's name (forge) or `update` one
16+
to a new owner (transfer / disown), evading the owner gate that governs
17+
update/delete. The security middleware (plugin-security step 3.5) now treats
18+
`owner_id` as system-managed for non-privileged writers: on insert an empty
19+
value is auto-stamped to the acting user (batch rows too — previously only the
20+
single-record path stamped, leaving bulk-inserted rows NULL-owned and
21+
invisible to their creator), and a supplied foreign owner is denied; on update
22+
a supplied `owner_id` is a transfer/disown and is denied — the unchanged no-op
23+
echo of a form save is tolerated via a pre-image compare, and a bulk
24+
change-set carrying `owner_id` fails closed. A non-scalar `owner_id`
25+
(array/object) is rejected outright rather than string-coerced, and the
26+
change-set membership test uses own-property semantics so a polluted
27+
prototype cannot spoof an ownership write. Both require the transfer grant
28+
(`allowTransfer`, or `modifyAllRecords` which implies it) to proceed. System
29+
context (`ctx.isSystem`) stays fully exempt (OAuth provisioning / cron
30+
snapshots / seed claims / migrations), and under delegation both principals
31+
must hold the grant (ADR-0090 D10 intersection). Note a REST **import** runs
32+
under the importer's own context (not `isSystem`), so a non-privileged user
33+
importing a CSV whose `owner_id` column names other users is correctly denied
34+
unless they hold the transfer grant — administrators (who carry
35+
`modifyAllRecords`) are unaffected.
36+
37+
- **#2982 — bulk writes skipped owner scoping on OWD-`private` objects.** A
38+
`update({ multi: true })` / bulk delete rebuilt the driver AST from
39+
`options.where` AFTER the middleware chain, discarding the owner/RLS write
40+
filter that plugin-sharing (`buildWriteFilter`) and plugin-security compose
41+
onto `opCtx.ast` — so a member's bulk write hit every matching row, including
42+
peers'. The engine now seeds `opCtx.ast` from the caller's predicate BEFORE the
43+
chain (the same seam reads use) and hands the middleware-composed AST to
44+
`driver.updateMany` / `driver.deleteMany`, so bulk writes are constrained to the
45+
rows the caller may edit — matching single-id write behavior. `delete` now
46+
applies the same scalar-`id` guard `update` already had, so an id-list bulk
47+
delete (`where: { id: { $in: […] } }, multi: true`) is owner-scoped too, and
48+
both multi branches fail CLOSED (throw) rather than silently rebuilding an
49+
unscoped predicate if the row-scoping AST is ever absent.
50+
51+
Consequences of routing bulk writes through the AST: the anti-oracle
52+
predicate guard now also applies to bulk `update`/`delete` (a bulk write
53+
filtering on an FLS-unreadable field is rejected, as reads already are), and a
54+
principal-less (no-`userId`, non-system) bulk write on an owner-scoped object
55+
now correctly affects zero rows instead of all of them.
56+
57+
Proven end-to-end on the real showcase app
58+
(`packages/dogfood/test/owner-anchor-and-bulk-writes.dogfood.test.ts`) and pinned
59+
in the ADR-0096 authz-conformance ledger (`ownership-anchor-guard`,
60+
`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
@@ -114,6 +114,12 @@ export const AUTHZ_CONFORMANCE: AuthzPrimitive[] = [
114114
enforcement: 'plugin-security/field-masker.ts + detectForbiddenWrites' },
115115
{ id: 'ownership-stamp', summary: 'owner_id auto-stamp on insert', state: 'enforced',
116116
enforcement: 'plugin-security/security-plugin.ts (insert owner_id inject)' },
117+
{ 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',
118+
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',
119+
proof: 'owner-anchor-and-bulk-writes.dogfood.test.ts' },
120+
{ 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',
121+
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',
122+
proof: 'owner-anchor-and-bulk-writes.dogfood.test.ts' },
117123
{ id: 'record-share', summary: 'manual record shares (sys_record_share)', state: 'enforced',
118124
enforcement: 'plugin-sharing/sharing-service.ts buildReadFilter/canEdit' },
119125
{ 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+
});

0 commit comments

Comments
 (0)