Skip to content

Commit b54aaab

Browse files
os-helpclaude
andauthored
fix(plugin-security): require by-id write targets to be within the caller's readable set under select-only RLS (#7792)
* fix(plugin-security): require by-id write targets to be within the caller's readable set under select-only RLS An object whose row narrowing is authored as `operation: 'select'` rules only had an open by-id write path. A contributor could PATCH records they could not read — 200, values persisted — on the master object AND on a `controlled_by_parent` detail, while the read side correctly hid the same rows (GET 404, absent from list). Measured live on the stock showcase by QA run #7637, on three objects, twice each. The by-id write pre-image gate, the controlled_by_parent master check and the bulk write filter all compose the RLS filter for the WRITE operation. With no update-scope policy applicable to the caller that filter compiled to a null Layer 1, and all three row gates became a no-op at once; OWD `public_read_write` then let `resolveSharingCanEdit` admit the write, and the detail derived its access from that same permissive master verdict. An empty write-class policy collection now derives its scope from the caller's SELECT narrowing, at the single decision site in `computeLayeredRlsFilter` — the same policies, compiled by the same compiler, that the read path enforces. Deliberately not derived: when any write-class policy applies (an authored predicate or the in-domain platform ownership floor), so app-authored write wideners keep deciding alone and the #7401 / #6736 directions are preserved exactly; for `insert`, which has no pre-image to be visible; and when the caller holds the read-side superuser bypass on a posture-permitting object, whose readable set is unbounded — the mirror of the read path's own Layer 1 short-circuit, so a derived write scope can never be narrower than the read scope it comes from. Fixes #7665 Co-Authored-By: Claude <noreply@anthropic.com> * docs(permissions): record that a `select` policy also bounds writes when no write-class policy applies The "Choosing `operation`" paragraph described `select` as narrowing reads and the write classes as guarding "the matching write" — the exact mental model that produced #7665, and now an understatement of what the platform enforces. States the derivation and its three boundaries: it applies only when no write-class policy applies to the caller (an authored write predicate keeps deciding its class alone), never for `insert`, and never for a caller holding the read-side superuser bypass. Adds a pointer from the `operation` property row. Co-Authored-By: Claude <noreply@anthropic.com> --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 098f4bb commit b54aaab

6 files changed

Lines changed: 832 additions & 25 deletions

File tree

Lines changed: 33 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,33 @@
1+
---
2+
"@objectstack/plugin-security": patch
3+
---
4+
5+
fix(plugin-security): a by-id write target must be inside the caller's readable set when only select-scope RLS is authored
6+
7+
An object whose row narrowing is authored as `operation: 'select'` rules only had an
8+
**open by-id write path**. A low-privilege user could `PATCH` records they could not
9+
read — 200, values persisted — on the object itself and on a `controlled_by_parent`
10+
detail, while the read side correctly hid the same rows (404 on GET, absent from list).
11+
12+
The cause was a single missing scope. The by-id write pre-image gate, the
13+
controlled_by_parent master check and the bulk write filter all compose the RLS filter
14+
for the **write** operation. With no update-scope policy applicable to the caller,
15+
that filter compiled to nothing and every one of those row gates became a no-op at
16+
once; an open sharing model (`public_read_write`) then admitted the write. Deriving the
17+
detail's access from the same permissive master verdict spread it to details as well.
18+
19+
An empty write-class policy collection now **derives its scope from the caller's
20+
`select` narrowing** — the same policies, compiled by the same compiler, that the read
21+
path enforces. "You cannot mutate what you cannot see" holds by construction on all
22+
three gates, and the explain engine reports the same narrowing for `update`/`delete`
23+
that it reports for `read` instead of "No RLS policy applies".
24+
25+
Migration-visible change: on an object narrowed by select-only RLS, a by-id or bulk
26+
`update`/`delete` of a row **outside the caller's readable set** is now refused
27+
(`PERMISSION_DENIED`, 403) where it previously succeeded. Reads, inserts, and any
28+
object that **does** author an update- or delete-scope policy are unaffected — where a
29+
write-class predicate exists it keeps deciding alone, so app-authored write wideners
30+
behave exactly as before. Callers holding a read-side superuser bypass
31+
(`viewAllRecords` on a posture-permitting object) are not newly narrowed. An app that
32+
relied on the previous behaviour should author an explicit `operation: 'update'` policy
33+
expressing the wider write scope it intends.

content/docs/permissions/rls.mdx

Lines changed: 12 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ export const ContributorAccess = definePermissionSet({
5858
| `name` | `string` | snake_case identifier |
5959
| `label` | `string` | Human-readable name |
6060
| `object` | `string` | Target object — or `'*'` to apply to every object |
61-
| `operation` | `'select' \| 'insert' \| 'update' \| 'delete' \| 'all'` | Which operation the policy guards |
61+
| `operation` | `'select' \| 'insert' \| 'update' \| 'delete' \| 'all'` | Which operation the policy guards. A `select` policy also bounds writes when no write-class policy applies — see below |
6262
| `using` | `string` | Predicate for rows the user may **see / act on** (compiled into the query filter) |
6363
| `check` | `string` | Predicate rows must satisfy **after a write**. Omit it and `using` is reused |
6464
| `positions` | `string[]` | Which positions the policy applies to. Omit = everyone |
@@ -71,6 +71,17 @@ At least one of `using` / `check` is required.
7171
everything. Internally `find` / `findOne` / `count` / `aggregate` all map to
7272
`select`.
7373

74+
**`select` also bounds writes when nothing else does.** A write target must be
75+
inside the caller's **readable** set, so when **no** write-class policy applies
76+
to a caller on an object, the `update` / `delete` scope is derived from that
77+
caller's `select` policies — a record they cannot read is one they cannot
78+
modify, by id or in bulk. Authoring a write-class policy switches the
79+
derivation off for that class: an authored `update` predicate then decides
80+
`update` alone and widens exactly as written. Nothing is derived for `insert`
81+
(there is no pre-existing row to be visible), and a caller holding the
82+
read-side superuser bypass (`viewAllRecords` on a private or platform-global
83+
object) is not narrowed, because their readable set is already unbounded.
84+
7485
## The expression grammar
7586

7687
RLS predicates are **canonical CEL**, lowered into a query filter by the shared

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

Lines changed: 61 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -1426,9 +1426,13 @@ export class SecurityPlugin implements Plugin {
14261426
// engine with `{ id } AND <writeFilter>`; a `find` does not re-enter this
14271427
// block, so there is no recursion, and read-side RLS/tenant scoping
14281428
// compose naturally. A `null` result means the row is either gone or
1429-
// RLS-hidden → deny. When `computeRlsFilter` returns `null` (no policy
1430-
// applies — e.g. an admin set with no RLS, or `modifyAllRecords`) the
1431-
// check is skipped and behaviour is unchanged.
1429+
// RLS-hidden → deny. When `computeRlsFilter` returns `null` the check
1430+
// is skipped — but since #7665 that is a far smaller class than "no
1431+
// WRITE policy applies": an empty write-class collection now derives
1432+
// its scope from the caller's SELECT narrowing inside
1433+
// `computeLayeredRlsFilter`, so the skip happens only when the caller's
1434+
// READABLE set is unbounded too (an admin set with no RLS at all, or a
1435+
// posture-permitting superuser bypass).
14321436
//
14331437
// [#5492] The filter is composed BY PROVENANCE. Two of the policies that
14341438
// can land in it are the platform's OWN ownership floor
@@ -1586,8 +1590,12 @@ export class SecurityPlugin implements Plugin {
15861590

15871591
// 2.8. ADR-0055 — controlled-by-parent WRITE: a detail write (insert/update/
15881592
// delete) requires edit access to its master. The detail itself carries no
1589-
// authored RLS, so the #1994 pre-image check above is a no-op for it; this
1590-
// closes the by-id write path by checking the master instead.
1593+
// authored RLS (nothing to derive from either, #7665), so the #1994
1594+
// pre-image check above is a no-op for it; this closes the by-id write
1595+
// path by checking the master instead — and the master's own write
1596+
// filter, since #7665, derives from its SELECT narrowing when no
1597+
// write-class policy applies, so a select-only master gates its details
1598+
// by visibility too.
15911599
if (
15921600
['insert', 'update', 'delete', 'transfer', 'restore', 'purge'].includes(opCtx.operation) &&
15931601
permissionSets.length > 0 &&
@@ -3925,7 +3933,54 @@ export class SecurityPlugin implements Plugin {
39253933
// longer skip the tenant wall (that is Layer 0's own exemption, below).
39263934
let layer1: Record<string, unknown> | null = null;
39273935
if (!(posturePermits && superuserBypass)) {
3928-
const collected = this.collectRLSPolicies(permissionSets, object, operation, (context?.positions ?? []) as string[]);
3936+
let collected = this.collectRLSPolicies(permissionSets, object, operation, (context?.positions ?? []) as string[]);
3937+
// [#7665] The write-visibility floor: a write target must be inside the
3938+
// caller's READABLE set. When NO policy of the write class applies to
3939+
// this (principal, object, operation) — nothing authored for the class,
3940+
// and the platform ownership floor outside its `positions` domain — the
3941+
// write class used to compile to a null Layer 1, and every write-side
3942+
// row gate composed from it became a no-op at once: the by-id pre-image
3943+
// gate (step 2.7), the controlled_by_parent master check, and the bulk
3944+
// write AST injection. QA #7637 measured the result on the stock
3945+
// showcase (select-only narrowing, `contributor` position, OWD
3946+
// `public_read_write`): a contributor PATCHed by id records they could
3947+
// not read — on the master AND on a controlled_by_parent detail — while
3948+
// the read side correctly hid them.
3949+
//
3950+
// So an empty write-class collection now DERIVES the write scope from
3951+
// the caller's SELECT narrowing — the same policies, compiled by the
3952+
// same compiler, that the read path enforces. "You cannot mutate what
3953+
// you cannot see" then holds by construction, and the explain engine
3954+
// reports the same narrowing for update/delete that it reports for
3955+
// read, instead of "No RLS policy applies".
3956+
//
3957+
// Deliberately NOT derived:
3958+
// - when ANY write-class policy applies (an authored predicate, or
3959+
// the in-domain platform floor): those paths keep their exact
3960+
// semantics, including every widening direction #7401 / #6736
3961+
// track — #7665 criterion 5 (derive ONLY when no update-scope
3962+
// predicate exists). `checkAuthoredRowWrite` is additionally
3963+
// protected by its own authored-set pre-check, so a derived scope
3964+
// can never masquerade as an authored admission (#5493 / #7281);
3965+
// - for `insert` — there is no pre-existing row to be visible (a
3966+
// controlled_by_parent detail INSERT is still gated through its
3967+
// master's derived scope by step 2.8);
3968+
// - when the caller holds the read-side superuser bypass on a
3969+
// posture-permitting object — their readable set is unbounded, so
3970+
// the readability requirement imposes nothing. This is the mirror
3971+
// of the read path's own Layer-1 short-circuit above, so the
3972+
// derived write scope can never be NARROWER than the read scope it
3973+
// is derived from.
3974+
if (
3975+
collected.length === 0 &&
3976+
(operation === 'update' || operation === 'delete') &&
3977+
!(
3978+
posturePermits &&
3979+
this.permissionEvaluator.hasSuperuserReadBypass(object, permissionSets, { isPrivate: meta.isPrivate })
3980+
)
3981+
) {
3982+
collected = this.collectRLSPolicies(permissionSets, object, 'select', (context?.positions ?? []) as string[]);
3983+
}
39293984
// [#5492] Provenance composition: the caller (the by-id write pre-image
39303985
// gate) has already asked the declared write authority — `ISharingService`
39313986
// — and received a positive `allow`. Its answer REPLACES the platform's own

0 commit comments

Comments
 (0)