Skip to content

Commit 8c767f5

Browse files
os-zhuangos-support-aiclaude
authored
fix(plugin-security): a public_read_write OWD opens row-level writes, not just the creator's (#8023) (#8072)
The platform's wildcard `owner_only_writes` floor (object '*', operation update, `created_by == current_user.id`, positions ['org_member']) stayed composed into Layer 1 on objects declaring `sharingModel: 'public_read_write'`, because the by-id write pre-image gate only lets ISharingService REPLACE the floor on a positive `allow` and a public object makes the service abstain. Net effect: the OWD declared 'everyone reads and writes' and the runtime enforced 'only the creator writes' — a declared-but-unenforced security property on a published surface. The floor is now conditioned on the object's DECLARED OWD at collection time, before #7665's derive-from-select branch, so the write class falls through to the caller's select narrowing and by-id write visibility (#7792) is preserved. Only `update` is opened (owner_only_deletes survives), only the canonical `public_read_write` spelling qualifies (controlled_by_parent and an unset model on a system object do not), and only the PLATFORM's floor is dropped — an app-authored policy with the same predicate still reaches the compiler. Claude-Session: https://claude.ai/code/session_01PEVB6w7D7uCszR9Mw1BL73 Co-authored-by: os-zhuang <support@objectstack.ai> Co-authored-by: Claude Opus 5 <noreply@anthropic.com>
1 parent 9b959f3 commit 8c767f5

5 files changed

Lines changed: 644 additions & 1 deletion

File tree

Lines changed: 50 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,50 @@
1+
---
2+
"@objectstack/plugin-security": patch
3+
---
4+
5+
fix(plugin-security): a `public_read_write` object is writable by everyone the access matrix grants `edit`, not only by each row's creator (#8023)
6+
7+
An object declaring `sharingModel: 'public_read_write'` promised "everyone can
8+
see and edit" and delivered "everyone can see, only the creator can edit". A
9+
persona the access matrix grants `edit: true` could `GET` a row **200** and
10+
`PATCH` the same row **403 PERMISSION_DENIED**, with the record-level refusal
11+
("You do not have access to this record…"). Three declarations agreed the write
12+
was allowed — the access matrix's `edit: true`, the object's OWD, and the
13+
absence of any authored RLS — and the runtime refused it anyway.
14+
15+
The cause is the platform's own row-level write ownership floor.
16+
`member_default` ships `owner_only_writes` (object `'*'`, operation `update`,
17+
`created_by == current_user.id`, positions `['org_member']`). The by-id write
18+
pre-image gate lets `ISharingService`'s tri-state verdict **replace** that floor,
19+
but only on a positive `allow` — and on a public object the service **abstains**,
20+
because record sharing genuinely does not enforce there. An abstain keeps the
21+
floor, so the floor became the object's only row-level write gate and quietly
22+
overrode its OWD.
23+
24+
An object whose author declared `public_read_write` now never inherits the
25+
wildcard `update` floor in the first place. Three boundaries are deliberate:
26+
27+
- **`delete` is unchanged.** `public_read_write` is "see and edit"; the legacy
28+
`full` alias that also covered transfer/delete was refused a mechanical
29+
conversion for being *wider* than it (ADR-0090 D4). `owner_only_deletes` still
30+
refuses a non-creator delete.
31+
- **Only the OWD that says so.** The declared model is read, never
32+
`plugin-sharing`'s effective bucket — which folds `controlled_by_parent` and an
33+
unset model on a system object into the same `'public'` value. A detail object
34+
derives access from its master, and an unset model on a `sys_*` table is a
35+
legacy default, so neither opens writes. An unresolvable schema fails closed.
36+
- **Only the platform's floor.** Provenance decides, so an app-authored policy
37+
spelling the identical predicate still reaches the compiler and still refuses
38+
(ADR-0049).
39+
40+
Because the floor is removed at collection time, the write class is then empty
41+
and the derive-from-select scope supplies the write filter — so "you cannot
42+
mutate what you cannot see" continues to hold on these objects: a caller
43+
narrowed by select-only RLS still cannot write a row outside its readable set.
44+
45+
Objects with any other OWD are untouched: on `private` and `public_read`, a
46+
non-owner write is still refused. The object-level gate is untouched too — a
47+
persona with `edit: false` still gets the object-level refusal, with its own
48+
distinct sentence. `POST /api/v1/security/explain` follows the same composition,
49+
so it stops reporting the `rls` layer as `narrows` for `update` on an object
50+
with zero authored RLS, while continuing to report a narrowing for `delete`.

‎packages/plugins/plugin-security/src/platform-ownership-policies.test.ts‎

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -12,7 +12,10 @@ import { describe, it, expect } from 'vitest';
1212
import {
1313
isPlatformOwnershipFloorPolicy,
1414
platformOwnershipFloorPolicyCount,
15+
owdDeclaresOpenRowWrites,
16+
owdOpenWritesCoversOperation,
1517
OWNERSHIP_FLOOR_PREDICATE,
18+
OWD_OPENING_ROW_WRITES,
1619
} from './platform-ownership-policies.js';
1720
import { defaultPermissionSets } from './objects/default-permission-sets.js';
1821

@@ -95,3 +98,51 @@ describe('[#5492] platform ownership-floor provenance', () => {
9598
expect(writeClass.length).toBe(platformOwnershipFloorPolicyCount());
9699
});
97100
});
101+
102+
// [#8023] The OWD condition on the floor. The headline case is proven
103+
// end-to-end over HTTP by
104+
// `packages/qa/dogfood/test/owd-public-read-write-write-floor.dogfood.test.ts`;
105+
// what belongs HERE is the vocabulary boundary — which spellings open writes
106+
// and which look like they might but must not.
107+
describe('[#8023] owdDeclaresOpenRowWrites — the DECLARED model, never the effective bucket', () => {
108+
it('the one canonical spelling opens writes, in both the flat and the nested spot', () => {
109+
expect(owdDeclaresOpenRowWrites({ sharingModel: 'public_read_write' })).toBe(true);
110+
expect(owdDeclaresOpenRowWrites({ security: { sharingModel: 'public_read_write' } })).toBe(true);
111+
expect(OWD_OPENING_ROW_WRITES).toBe('public_read_write');
112+
});
113+
114+
it('every OWD that does NOT declare open writes keeps the floor', () => {
115+
for (const model of ['private', 'public_read', 'controlled_by_parent']) {
116+
expect(owdDeclaresOpenRowWrites({ sharingModel: model }), `${model} must keep the floor`).toBe(false);
117+
}
118+
});
119+
120+
it('⚠️ `controlled_by_parent` and an UNSET model on a system object must NOT open writes', () => {
121+
// Both collapse into plugin-sharing's `'public'` bucket
122+
// (`effectiveSharingModel`), which is why reading THAT here would have been
123+
// the bug: `controlled_by_parent` derives access from its master (which has
124+
// its own OWD and its own gate) and declares nothing about its own writers,
125+
// while an unset model on a `sys_*` table is a legacy default rather than
126+
// an author's statement — opening writes there would hand every member
127+
// cross-creator writes on the platform's identity tables.
128+
expect(owdDeclaresOpenRowWrites({ name: 'owdw_detail', sharingModel: 'controlled_by_parent' })).toBe(false);
129+
expect(owdDeclaresOpenRowWrites({ name: 'sys_user_position', isSystem: true })).toBe(false);
130+
expect(owdDeclaresOpenRowWrites({ name: 'sys_user_position' })).toBe(false);
131+
});
132+
133+
it('an unresolvable or malformed schema fails CLOSED (the floor stays)', () => {
134+
for (const schema of [null, undefined, {}, { sharingModel: null }, { sharingModel: 'read_write' }]) {
135+
expect(owdDeclaresOpenRowWrites(schema)).toBe(false);
136+
}
137+
// `read_write` above is the ADR-0090 D4 LEGACY alias. It no longer parses at
138+
// authoring, and a stored value the platform does not recognise must never
139+
// be read as the wider posture.
140+
});
141+
142+
it('the opened write class is `update` alone — `owner_only_deletes` survives', () => {
143+
expect(owdOpenWritesCoversOperation('update')).toBe(true);
144+
for (const operation of ['delete', 'select', 'insert', 'all']) {
145+
expect(owdOpenWritesCoversOperation(operation), `${operation} must not be opened`).toBe(false);
146+
}
147+
});
148+
});

‎packages/plugins/plugin-security/src/platform-ownership-policies.ts‎

Lines changed: 73 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -101,3 +101,76 @@ export function isPlatformOwnershipFloorPolicy(
101101
export function platformOwnershipFloorPolicyCount(): number {
102102
return PLATFORM_OWNERSHIP_FLOOR_KEYS.size;
103103
}
104+
105+
/**
106+
* [#8023] The ONE Org-Wide Default that DECLARES row-level writes open, and the
107+
* reason the floor above must not apply to an object carrying it.
108+
*
109+
* The floor is the platform's answer for objects whose OWD says nothing about
110+
* writes. `public_read_write` is not such an object: the author has declared
111+
* "everyone can see and edit" (`spec/security/sharing.zod.ts`), the access
112+
* matrix grants `edit: true`, and the runtime was nevertheless refusing every
113+
* write by a non-creator — the OWD contract stated and unenforced, which is the
114+
* one thing a declaration must never be.
115+
*
116+
* ## Why this is read here and not deferred to `ISharingService`
117+
*
118+
* The composition already asks the sharing service for a tri-state write
119+
* verdict, and on this object it answers `abstain` — correctly: record sharing
120+
* genuinely does not enforce on a public object. But `abstain` is ONE answer
121+
* covering three different facts (public OWD / no owner field / bypass-listed
122+
* internal), and only the first says anything about writes being open. #5492's
123+
* E2 experiment measured what collapsing all three into "permitted" costs: an
124+
* ordinary member's cross-creator UPDATE on an `owner_id`-less object turned
125+
* 403 into 200. So the abstain's meaning is left exactly as it is, and the OWD
126+
* — which is a DECLARATION about this object rather than a verdict about this
127+
* row — is read directly, on the security side, where the floor is composed.
128+
*
129+
* ## The DECLARED model, never the effective one
130+
*
131+
* `plugin-sharing`'s `effectiveSharingModel` folds `public_read_write`,
132+
* `controlled_by_parent` and an unset model on a system object into one
133+
* `'public'` bucket. That bucket is right for "does record sharing enforce
134+
* here"; it is wrong for this question, and reading it here would open writes
135+
* on two classes that never declared them:
136+
*
137+
* - `controlled_by_parent` derives its access from the MASTER record, which
138+
* has its own OWD and its own gate (`assertControlledByParentWrite`) — the
139+
* detail declares nothing about who may write it;
140+
* - an UNSET model on a `sys_*` / `isSystem` object is a legacy default, not
141+
* an author's statement. Opening writes there would hand every member
142+
* cross-creator writes on the platform's own identity tables — the shape of
143+
* the objectui#2348 incident that made an unset model fail closed in the
144+
* first place.
145+
*
146+
* So the test is EXPLICIT equality with the one canonical spelling. An
147+
* unresolvable schema yields `false` and the floor stays (fail closed).
148+
*/
149+
export const OWD_OPENING_ROW_WRITES = 'public_read_write';
150+
151+
/**
152+
* True iff this object's author DECLARED the org-wide-open write baseline.
153+
* Reads both the flat and the nested spot, exactly as every other OWD reader
154+
* in the platform does.
155+
*/
156+
export function owdDeclaresOpenRowWrites(schema: unknown): boolean {
157+
const s = schema as { sharingModel?: unknown; security?: { sharingModel?: unknown } } | null;
158+
const model = s?.sharingModel ?? s?.security?.sharingModel;
159+
return model === OWD_OPENING_ROW_WRITES;
160+
}
161+
162+
/**
163+
* The write class {@link owdDeclaresOpenRowWrites} opens — `update` and nothing
164+
* else, so `owner_only_deletes` survives on a `public_read_write` object.
165+
*
166+
* This is the OWD vocabulary's own boundary, not a conservative guess:
167+
* `public_read_write` is defined as "everyone can see and edit", and the legacy
168+
* `full` alias that ALSO covered transfer/delete was refused a mechanical
169+
* conversion precisely because it is "wider than `public_read_write`"
170+
* (`spec/conversions/registry.ts`, ADR-0090 D4). An object that wants
171+
* cross-creator deletes declares it with an app-authored policy or a share,
172+
* both of which reach the compiler untouched.
173+
*/
174+
export function owdOpenWritesCoversOperation(operation: string): boolean {
175+
return operation === 'update';
176+
}

‎packages/plugins/plugin-security/src/security-plugin.ts‎

Lines changed: 54 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -49,7 +49,11 @@ import { bootstrapDeclaredCapabilities } from './bootstrap-declared-capabilities
4949
import { RLSCompiler, RLS_DENY_FILTER } from './rls-compiler.js';
5050
import { computeTenantLayer0Filter, andComposeLayers } from './tenant-layer.js';
5151
import { isPlatformTenantPolicy, isAuthoredTenantPolicy } from './platform-tenant-policies.js';
52-
import { isPlatformOwnershipFloorPolicy } from './platform-ownership-policies.js';
52+
import {
53+
isPlatformOwnershipFloorPolicy,
54+
owdDeclaresOpenRowWrites,
55+
owdOpenWritesCoversOperation,
56+
} from './platform-ownership-policies.js';
5357
import { hasPhantomTenantAnchor } from './federated-phantom-anchors.js';
5458
import {
5559
normalizeTenancyPosture,
@@ -177,6 +181,18 @@ interface ObjectSecurityMeta {
177181
* real remote `organization_id` — both keep the tenant wall exactly as it is.
178182
*/
179183
tenantAnchorIsPhantom: boolean;
184+
/**
185+
* [#8023] The object DECLARES `sharingModel: 'public_read_write'` — the one
186+
* OWD that states row-level writes are open org-wide. Read from the author's
187+
* declaration, never from `plugin-sharing`'s effective bucket; see
188+
* `owdDeclaresOpenRowWrites` in `platform-ownership-policies.ts` for why the
189+
* two must not be confused here.
190+
*
191+
* `false` when the schema does not resolve, so the floor stays (fail closed)
192+
* — and `unresolved` meta is never cached, so a transient boot miss is
193+
* retried rather than frozen into an over-strict answer.
194+
*/
195+
owdOpensRowWrites: boolean;
180196
requiredPermissions: NormalizedRequiredPermissions;
181197
fieldRequiredPermissions: Record<string, string[]>;
182198
/**
@@ -3995,6 +4011,41 @@ export class SecurityPlugin implements Plugin {
39954011
let layer1: Record<string, unknown> | null = null;
39964012
if (!(posturePermits && superuserBypass)) {
39974013
let collected = this.collectRLSPolicies(permissionSets, object, operation, (context?.positions ?? []) as string[]);
4014+
// [#8023] An object whose author DECLARED `sharingModel:
4015+
// 'public_read_write'` does not inherit the platform's wildcard `update`
4016+
// ownership floor AT ALL. QA #7637 measured the contradiction it caused
4017+
// on the stock showcase: `showcase_project` declares that OWD, the access
4018+
// matrix grants `showcase_contributor` `edit: true`, no RLS is authored —
4019+
// and a PATCH of a row the persona had not created answered 403 with the
4020+
// record-level sentence. The floor is `member_default`'s
4021+
// `owner_only_writes` (object `'*'`, `created_by == current_user.id`,
4022+
// positions `['org_member']`), and it survived because the by-id gate
4023+
// only lets `ISharingService` REPLACE it on a positive `allow`, while a
4024+
// public object makes the service `abstain`.
4025+
//
4026+
// ⚠️ THE PLACEMENT IS THE FIX, not just where it reads best. Removing the
4027+
// floor HERE — before the #7665 derive-from-select branch below — leaves
4028+
// the update class EMPTY, so that branch then derives the write scope
4029+
// from the caller's SELECT narrowing and "you cannot mutate what you
4030+
// cannot see" still holds on this very object (#7792's by-id
4031+
// write-visibility property, which the card names as a non-regression
4032+
// criterion). Removing it at the `dropPlatformOwnershipFloor` filter
4033+
// further down would instead compile Layer 1 to null and hand back an
4034+
// UNGATED by-id write — the same hole #7665 closed, reopened by a fix for
4035+
// a different bug.
4036+
//
4037+
// Scope, stated twice because each half is load-bearing:
4038+
// - only the PLATFORM's floor is dropped (provenance, ADR-0105 D3): an
4039+
// app-authored policy spelling the identical predicate still reaches
4040+
// the compiler and still refuses (ADR-0049);
4041+
// - only `update` (`owdOpenWritesCoversOperation`): `owner_only_deletes`
4042+
// stays, because `public_read_write` is "see and edit" and the alias
4043+
// that also meant delete was removed for being wider than it.
4044+
// Layer 0 (the tenant wall) is untouched — a `public_read_write` object is
4045+
// org-wide open, never cross-tenant open.
4046+
if (meta.owdOpensRowWrites && owdOpenWritesCoversOperation(operation)) {
4047+
collected = collected.filter((p) => !isPlatformOwnershipFloorPolicy(p));
4048+
}
39984049
// [#7665] The write-visibility floor: a write target must be inside the
39994050
// caller's READABLE set. When NO policy of the write class applies to
40004051
// this (principal, object, operation) — nothing authored for the class,
@@ -4580,6 +4631,8 @@ export class SecurityPlugin implements Plugin {
45804631
// [#7835] Federated object carrying the registry's INJECTED
45814632
// `organization_id` — a column the platform provisions no storage for.
45824633
tenantAnchorIsPhantom: hasPhantomTenantAnchor(obj),
4634+
// [#8023] The author's DECLARED OWD, not the effective sharing bucket.
4635+
owdOpensRowWrites: owdDeclaresOpenRowWrites(obj),
45834636
requiredPermissions: normalizeRequiredPermissions((obj as any)?.requiredPermissions),
45844637
fieldRequiredPermissions,
45854638
unresolved: !obj,

0 commit comments

Comments
 (0)