Skip to content

Commit b7c792b

Browse files
fix(plugin-security): a USING-only RLS policy holds INSERT and UPDATE rows to its using (#19952)
Closes #19942 Clause-②: no (narrowing) ## What changed The runtime now does what the published contract says. When an applicable policy declares no `check`, the write post-image check (step 3.6, `computeWriteCheckFilter`) compiles that policy's `using` as its check. This applies to the INSERT row, which is judged after `beforeInsert`, and to the new row of a by-id UPDATE. The contract is `RowLevelSecurityPolicySchema.check`: "defaults to USING clause if not specified". PostgreSQL has the same rule for a policy without `WITH CHECK`. Before this change, the gate compiled only policies that declared `check`. Measured on the repro: with `using: "record.status != 'closed'"`, an INSERT of `status = 'closed'` was admitted and stored. Changes to `packages/plugins/plugin-security`: - `security-plugin.ts`: a new selector, `writeCheckPolicies`, decides which applicable policies the post-image check compiles. - `security-plugin.ts`: step 2.7 (the by-id pre-image gate) records its ownership-floor decision (`preImageFloorOpts`), and step 3.6 reads it. - `security-plugin.ts`: the two early floor drops in `computeLayeredRlsFilter` (the `public_read_write` OWD and the covering controlled-by-parent master gate) now ask a single predicate, `platformFloorYieldsToObjectWriteModel`. The post-image check asks the same predicate. The pre-image behaviour is byte-identical. - `rls-compiler.ts`: the `compileFilter` comment ("defaulting to `using` when omitted") now matches a reachable call path. "Declares `check`" is now tested with `policyDeclaresClause`, the same test the selector uses. Zero edits under `packages/spec`, `content/docs/**` and `content/docs/releases/**`. ## How a defaulted `using` composes, exactly This covers one principal's check. The delegator's check (ADR-0090 D10) and the Layer 0 tenant post-image check are still AND-ed with it, unchanged. 1. **Some applicable policy for the operation declares `check`.** Only the declared checks take part, OR-combined. That is today's result, byte for byte. A USING-only sibling adds nothing to the check. - Under PostgreSQL's permissive OR, the sibling's `using` would be OR-ed in. That can only widen the check: a grant spelled `id != null` would erase every declared check beside it. - The narrower reading was taken. It is an open question for the seat; see the report. 2. **No applicable policy declares `check`.** Every applicable write-class policy with a `using` takes part, its `using` compiled as its check, OR-combined. 3. **The platform ownership floor (`owner_only_writes`) takes part in case 2 exactly when the by-id pre-image gate kept it.** - It is dropped when the declared write authority answered `allow` (an edit-depth share, `modifyAllRecords`), on a `public_read_write` OWD, and under a covering controlled-by-parent master gate. Otherwise it stays in. - Why it stays: an app-authored USING-only `update` policy is used as a WIDENER, OR-ed with the floor at the pre-image. Leaving the floor out of the check would turn that widener into a restriction on the creator's own rows. - Why it is dropped in those three cases: keeping it would refuse, one step later, the cross-creator writes that #5492, #8757 and #8865 opened. 4. **Unchanged.** - `select` policies never gate a write's new row. - A bulk update with no single id is still not post-image checked. - The `modifyAllRecords` bypass on private and platform-global objects returns before the selector runs. ## Census (the triage's p0 condition), tree `aeaaa44292` **Added by the seat after the contract review:** the census also covers two published authoring factories in `packages/spec/src/security/rls.zod.ts:613-666`. `RLS.ownerPolicy` (`operation: 'all'`, USING-only) and `RLS.allowAllPolicy` (`all`, `1 == 1`) both emit write-class USING-only policies. Their defaults see no reachable change: `1 == 1` is always true, and `owner_id` is stamped and guarded by step 3.5. A caller who passes a custom `ownerField` to `RLS.ownerPolicy` is now held to it on writes. Searched: `packages/**`, `examples/**`, `apps/**`, the published `skills/**`, and the CLI and `create-objectstack` scaffolds. The table lists every write-class (`insert` / `update` / `all`) policy that declares `using` and no `check`. | Where | Policy | Operation | `using` | Tenant isolation? | Reachable write, and what now happens | |:---|:---|:---|:---|:---|:---| | plugin-security `member_default` | `owner_only_writes` (platform floor) | update, `org_member` | `created_by == current_user.id` | no (ownership) | Takes part only where the pre-image kept it (rule 3). An update that re-points `created_by` away from the caller is now refused. | | plugin-security `member_default`, `organization_admin` | `sys_user_preference_self` | all | `user_id == current_user.id` | no (self) | Yes, `allowCreate` / `allowEdit` are granted. A preference row created for another user, or moved to another user, is now refused (403). It was admitted. | | plugin-security `member_default`, `organization_admin` | `sys_api_key_self` | all | `user_id == current_user.id` | no (self) | The member revoke patch (#8053). A patch that also carries `user_id` of another user is now refused (403). It was admitted with `user_id` stripped later; see the dogfood change below. | | plugin-security `member_default` | `sys_user_self` | all | `id == current_user.id` | no (self) | Self edit of `sys_user` (whitelisted columns). The new row keeps its `id`, so there is no change. | | plugin-security `member_default`, `organization_admin` | `sys_session_self`, `sys_two_factor_self`, `sys_device_code_self`, `sys_oauth_consent_self`, `sys_oauth_application_self` | all | `user_id == current_user.id` | no (self) | Not reachable today. These are better-auth managed tables, and both sets deny create and edit at the object layer (`denyWritesOnManagedObjects`). | | plugin-security `member_default`, `organization_admin` | `sys_organization_self` | all | `id == current_user.organization_id` | **borderline, see below** | Not reachable today. Create and edit on `sys_organization` are denied at the object layer for both sets. | | `packages/metadata/src/__fixtures__/hotcrm-17.1-built-permissions.artifact.json` (a fixture, not shipped) | `marketing_campaign_updates`, `marketing_campaign_member_updates` | update | `id != null` | no | Always true, so there is no change. | No write-class USING-only policy was found in `examples/app-showcase` (`task_own_rows` and `invoice_own_rows` are `select`; `invoice_owner_immutable` declares `check`), `packages/verify` (a `select` probe), `skills/objectstack-data/rules/security.md` (`own_records` declares `check`; `org_isolation` is `select`) or any CLI scaffold. **Tenant-isolation verdict: the p0 condition does not fire.** - Every shipped `organization_id == current_user.organization_id` policy is `select`: `sys_member_org`, `sys_invitation_org` and `sys_team_org`. - The tenant wall is Layer 0, and it has its own post-image write check (step 3.7). The #19886 stage-1 measurement (P3) recorded a forged-organization INSERT refused 403 by it. - The one borderline row is `sys_organization_self`. It compares against `current_user.organization_id`, and it is in the platform's own tenant-policy provenance set (`isPlatformTenantPolicy`, which strips it when isolation is off). But: - it scopes the organization table to its own row, and the declaration's own comment says these carve-outs "are NOT tenant walls"; - writes to that table are refused at the object layer for both sets that carry it. - Listed so the seat can judge it rather than find it. ## Writes refused after this change A single-row INSERT or a by-id UPDATE whose resulting row falls outside the `using` of every applicable write-class policy, when none of those policies declares `check`. The refusal is `403 PERMISSION_DENIED`, the existing row-level CHECK envelope, and nothing is stored. There is no transition switch. An author who wants a write to leave a policy's scope declares a `check`. In the platform defaults, this reaches the `sys_user_preference` and `sys_api_key` rows in the census, and a `created_by` re-point under the floor. ## Tests Final head `19236d91ad`. New file `packages/plugins/plugin-security/src/rls-check-defaults-to-using.test.ts`. It uses a real `ObjectQL` engine and `SecurityPlugin` and runs on driver-sql (better-sqlite3) and driver-sqlite-wasm: 22 tests, 11 cells on each driver. - The repro: USING-only `record.status != 'closed'` refuses an INSERT with `status: 'closed'`. The test asserts `code: 'PERMISSION_DENIED'` and `status: 403`, and that nothing is stored. It admits `'open'`. The same holds for an `insert`-class policy. - The UPDATE new row: moving an in-scope row to `'closed'` is refused and the stored row does not move. An in-scope update is admitted. - A tenant-style USING-only policy (`organization_id == current_user.organization_id`) refuses a cross-organization INSERT and admits one into the caller's own organization. Layer 0 is not armed in this harness, so the refusal comes from the defaulted check. - A declared-`check` control: the policy is judged on its `check`, not on its `using`, as before. - Composition: a USING-only sibling adds nothing to a declared check (rule 1). - The floor: the creator still updates their own deal out of the widener's scope, and a non-creator admitted only by the widener cannot move the deal out of it (rule 3). Fixture triage, two existing tests: - `security-plugin.test.ts`, "owner_id is always auto-injected on insert": its `tenant_isolation` policy is `all` and USING-only. The insert payload now carries the caller's organization, which the org-scoping stamp would write before the check, and the test asserts it arrives unchanged. - `packages/qa/dogfood/test/api-key-owner-revoke.dogfood.test.ts`: the mixed smuggle patch is split in two. Re-owning (`user_id` of another user) is refused 403 `PERMISSION_DENIED` and changes nothing. `key` smuggled beside `revoked` is still admitted with `key` stripped. Reverse verification. Each run used `scripts/ablation-replace.mjs` from committed state, and each restore was proven: the blob equals HEAD and `git diff HEAD` is empty. | Mutation | Result | |:---|:---| | The `using` fallback in `writeCheckPolicies` replaced with an empty list (the pre-fix behaviour) | 10 failed, 12 passed of 22. Every refusal witness failed on both drivers: the repro, the insert-class policy, the UPDATE new row, the cross-organization INSERT and the widener non-creator. All admit controls stayed green. | | The floor excluded unconditionally | 3 failed: the creator cell on both drivers, plus `authored-row-write-verdict.test.ts` "TRANSFERRED_UPDATE_IS_ADMITTED_BY_THE_COMPOSED_PATH". | | The floor kept unconditionally (the pre-image drop ignored) | 8 failed across the whole plugin-security suite: 6 in `controlled-by-parent-detail-write-authority.test.ts` (#8757 / #8865) and 2 in `row-write-widener-composition.test.ts` (#5492). | Suites and gates: - `pnpm --filter @objectstack/plugin-security test`: 121 files, 2314 tests passed. - `pnpm --filter @objectstack/plugin-security typecheck`: exit 0, and `check:test-typecheck` OK. - `@objectstack/organizations` test: 108 passed. `@objectstack/plugin-sharing` test: 913 passed. - `@objectstack/dogfood`, full run before the dogfood fix: 1 failed (the case above), 1102 passed. After the fix, that file passes 9 of 9. - eslint on the 5 touched TypeScript files: 0 errors and 0 warnings, read from `--format json`. The config is not type-aware (no `parserOptions.project`), so this narrowing leaves no untouched file's verdict unmeasured. The full `pnpm lint` is left to CI. - `node scripts/pm/dispatch-gates.mjs --commands`: 66 derived commands. 64 exited 0. 2 exited 3, which is `PREREQUISITE NOT MET` and so **NOT MEASURED**: `check:dual-build-cjs-loads` and `check:type-check-debt` need a whole-workspace build. ## Acceptance notes - `packages/lint/src/validate-rls-predicate-enforceability.ts`, header item 4, still says `computeWriteCheckFilter` "collects only the policies that declare a `check`". It is a comment, now incomplete. The rule's `using` consequence text does not mention that an uncompilable `using` on an `insert` / `all` policy with no `check` now also refuses every insert (it fails closed). Carrier: none. - The update post-image is still the caller's payload merged with the pre-image, judged before `beforeUpdate` (#16790). The `sys_api_key` case shows the effect: a column the D2 whitelist would strip now refuses the patch at the check instead. - The spec describe and the two reference pages say an omitted `check` "defaults to USING". That is now true, except in the mixed case of rule 1. The mixed case is raised in the report, not edited here. --- _Generated by [Claude Code](https://claude.ai/code/session_01AhQASwqJr2Z7XfGWUdvnbF)_ --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent c164186 commit b7c792b

7 files changed

Lines changed: 557 additions & 37 deletions

File tree

Lines changed: 28 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,28 @@
1+
---
2+
"@objectstack/plugin-security": minor
3+
---
4+
5+
fix(plugin-security)!: a row-level security policy that declares no `check` now holds INSERTs and UPDATEs to its `using` (#19942)
6+
7+
Clause-②: no (narrowing)
8+
9+
<!-- adr-0087: not-required (no-migration-prescription) an enforcement change on the write gate: no authorable key, spelling or stored shape moves, so a stored `sys_metadata` row needs no conversion and an upgrader has nothing to hand-edit. The remedy for a newly refused write is to declare `check` on the policy, or to fix the data. -->
10+
11+
**BREAKING**: this narrows the set of writes the write gate accepts. A write that is admitted today can be refused after this change. It ships as `minor` under the launch-window convention, the same way the insert-side `check` reorder did (#16805).
12+
13+
The published contract has always said this. `RowLevelSecurityPolicySchema.check` reads "defaults to USING clause if not specified", and PostgreSQL treats a policy without `WITH CHECK` the same way. The write gate did not do it. It compiled only the policies that declared `check`, so a policy with only a `using` never checked a write. With `using: "record.status != 'closed'"`, a caller could INSERT a closed row. The row was stored even though the same caller could not read it afterwards.
14+
15+
**Writes that are now refused.** Each refusal is the existing row-level CHECK denial, `403 PERMISSION_DENIED`, and nothing is stored. There is no transition switch.
16+
17+
- **Any policy with only a `using`.** A single-row INSERT, or a by-id UPDATE, is refused when its resulting row falls outside the `using` of every applicable write-class policy (`insert`, `update` or `all`) and none of those policies declares a `check`. To let a write move a row outside a policy's scope, declare a `check` on that policy.
18+
- **The platform's `_self` policies.** These have `operation: 'all'` and `using: user_id == current_user.id`. They now refuse a write that sets `user_id` to another user, on the self-service tables a member may write: `sys_user_preference`, and the revoke patch on `sys_api_key`. A member can no longer create a preference row for someone else. A member can no longer re-own their API key by adding `user_id` to a revoke patch. Before this change both writes were admitted, and the second one had `user_id` stripped later.
19+
- **Re-pointing `created_by` under the ownership floor.** This is a by-id UPDATE that changes `created_by` while the floor still applies to that write. The readonly strip used to remove the new value, and the update was admitted. It is now refused with 403. The new row is judged before that strip runs (#16790).
20+
- **A `using` that does not compile.** This applies to an `insert` or `all` policy that has only a `using`. That `using` is now also the insert check, and the policy fails closed: every insert it governs is refused. Before this change the insert was admitted, because no check ran.
21+
22+
**What does not change.**
23+
24+
- If any applicable policy declares `check`, only the declared checks decide, exactly as before. A policy with only a `using` alongside them adds nothing to the check.
25+
- The platform's ownership floor (`owner_only_writes`) is part of a defaulted check only when the by-id write gate kept it for that write. A record share at edit depth, a `public_read_write` object, or a covering controlled-by-parent master gate still replaces the floor. Those writes are not refused again on the new row.
26+
- `select` policies never gate a write's new row.
27+
- Bulk updates without a single id are still scoped by the `using` where clause and are not checked row by row.
28+
- The `modifyAllRecords` bypass on private and platform-global objects still skips the check.

‎packages/plugins/plugin-auth/src/sys-user-self-service-route.test.ts‎

Lines changed: 25 additions & 6 deletions
Original file line numberDiff line numberDiff line change
@@ -125,7 +125,16 @@ interface RouteResult {
125125
error: any;
126126
/** The payload as it stands after every layer ran (the guard strips in place). */
127127
data: Record<string, unknown>;
128-
/** Every `where` the pre-image re-read was called with, in order. */
128+
/**
129+
* Every `where` the engine was asked to read with, in order. On an admitted
130+
* own-row update there are two: the by-id pre-image re-read (step 2.7, the
131+
* `{id}` address AND-ed with the compiled write-RLS filter) and then the
132+
* caller's own-row read (step 3.6, a bare `{id}`) that the post-image check
133+
* merges the change set onto. That second read exists because
134+
* `sys_user_self` (`operation: 'all'`, `using: id == current_user.id`)
135+
* declares no `check`, so its `using` is the write check (ADR-0058 D4
136+
* default, `RowLevelSecurityPolicySchema.check`).
137+
*/
129138
preImageWheres: unknown[];
130139
/** The secondary-storage writes the ADR-0092 D6 companion hook performed. */
131140
snapshotWrites: Array<{ key: string; value: any }>;
@@ -269,8 +278,15 @@ describe('sys_user self-service — the row scope the write actually composes',
269278
// the compiled RLS parts. Asserted verbatim because every other assertion in
270279
// this file leans on `matchesFilter` reading it correctly, and because it is
271280
// the security property in one line: the caller's id, and nothing wider.
272-
expect(r.preImageWheres).toHaveLength(1);
281+
//
282+
// Then exactly one more read: the caller's OWN row by id, which step 3.6
283+
// merges the change set onto to judge the post-image against the defaulted
284+
// `sys_user_self` check (`id == current_user.id`). It is a read of the row
285+
// already admitted above, never a scope: it cannot widen what the member may
286+
// write, and the check it feeds can only refuse more.
287+
expect(r.preImageWheres).toHaveLength(2);
273288
expect(r.preImageWheres[0]).toEqual({ $and: [{ id: ME }, { id: ME }] });
289+
expect(r.preImageWheres[1]).toEqual({ id: ME });
274290
});
275291

276292
it('the org-peer READ scope does not appear in the write filter', async () => {
@@ -285,7 +301,9 @@ describe('sys_user self-service — the row scope the write actually composes',
285301
// an object-gate refusal produces exactly that. Pin the write got as far as
286302
// composing a filter before asserting what is not in it.
287303
expect(r.refusedBy).toBeNull();
288-
expect(r.preImageWheres).toHaveLength(1);
304+
// The pre-image re-read plus the post-image check's own-row read (see the
305+
// `preImageWheres` field). The absences below hold over BOTH.
306+
expect(r.preImageWheres).toHaveLength(2);
289307
expect(JSON.stringify(r.preImageWheres)).not.toContain('$in');
290308
expect(JSON.stringify(r.preImageWheres)).not.toContain(PEER);
291309
});
@@ -354,9 +372,10 @@ describe('sys_user self-service — the four pins, each attributed to a layer',
354372
// while the guard was dead code.
355373
const r = await route(ME, { email: 'attacker@example.com' });
356374
expect(r.refusedBy).toBe('identity-guard');
357-
// The pre-image re-read ran and SUCCEEDED (the row was visible) — proof the
358-
// first two layers admitted the write before the guard refused it.
359-
expect(r.preImageWheres).toEqual([{ $and: [{ id: ME }, { id: ME }] }]);
375+
// The pre-image re-read ran and SUCCEEDED (the row was visible), and the
376+
// post-image check read the caller's own row and passed — proof the first
377+
// two layers admitted the write before the guard refused it.
378+
expect(r.preImageWheres).toEqual([{ $and: [{ id: ME }, { id: ME }] }, { id: ME }]);
360379
// ADR-0112 envelope, both discriminators (the REST boundary derives 403 from
361380
// `status`, and `mapDataError` keys on `code`).
362381
expect(r.error?.code).toBe('PERMISSION_DENIED');

0 commit comments

Comments
 (0)