From 06d1ab576213653e17065d5bbfe2eac36b0c8c33 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 11 Aug 2026 14:44:06 +0000 Subject: [PATCH 1/2] fix(plugin-security): require by-id write targets to be within the caller's readable set under select-only RLS MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- .../select-only-rls-by-id-write-visibility.md | 33 + .../plugin-security/src/security-plugin.ts | 67 +- .../src/select-only-write-visibility.test.ts | 640 ++++++++++++++++++ .../test/fixtures/rls-owner-fixture.ts | 18 +- .../dogfood/test/rls-fixture.dogfood.test.ts | 86 ++- 5 files changed, 820 insertions(+), 24 deletions(-) create mode 100644 .changeset/select-only-rls-by-id-write-visibility.md create mode 100644 packages/plugins/plugin-security/src/select-only-write-visibility.test.ts diff --git a/.changeset/select-only-rls-by-id-write-visibility.md b/.changeset/select-only-rls-by-id-write-visibility.md new file mode 100644 index 0000000000..c6295ef52e --- /dev/null +++ b/.changeset/select-only-rls-by-id-write-visibility.md @@ -0,0 +1,33 @@ +--- +"@objectstack/plugin-security": patch +--- + +fix(plugin-security): a by-id write target must be inside the caller's readable set when only select-scope RLS is authored + +An object whose row narrowing is authored as `operation: 'select'` rules only had an +**open by-id write path**. A low-privilege user could `PATCH` records they could not +read — 200, values persisted — on the object itself and on a `controlled_by_parent` +detail, while the read side correctly hid the same rows (404 on GET, absent from list). + +The cause was a single missing scope. 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 nothing and every one of those row gates became a no-op at +once; an open sharing model (`public_read_write`) then admitted the write. Deriving the +detail's access from the same permissive master verdict spread it to details as well. + +An empty write-class policy collection now **derives its scope from the caller's +`select` narrowing** — the same policies, compiled by the same compiler, that the read +path enforces. "You cannot mutate what you cannot see" holds by construction on all +three gates, and the explain engine reports the same narrowing for `update`/`delete` +that it reports for `read` instead of "No RLS policy applies". + +Migration-visible change: on an object narrowed by select-only RLS, a by-id or bulk +`update`/`delete` of a row **outside the caller's readable set** is now refused +(`PERMISSION_DENIED`, 403) where it previously succeeded. Reads, inserts, and any +object that **does** author an update- or delete-scope policy are unaffected — where a +write-class predicate exists it keeps deciding alone, so app-authored write wideners +behave exactly as before. Callers holding a read-side superuser bypass +(`viewAllRecords` on a posture-permitting object) are not newly narrowed. An app that +relied on the previous behaviour should author an explicit `operation: 'update'` policy +expressing the wider write scope it intends. diff --git a/packages/plugins/plugin-security/src/security-plugin.ts b/packages/plugins/plugin-security/src/security-plugin.ts index 6cd324d5a0..1b9bb871a6 100644 --- a/packages/plugins/plugin-security/src/security-plugin.ts +++ b/packages/plugins/plugin-security/src/security-plugin.ts @@ -1426,9 +1426,13 @@ export class SecurityPlugin implements Plugin { // engine with `{ id } AND `; a `find` does not re-enter this // block, so there is no recursion, and read-side RLS/tenant scoping // compose naturally. A `null` result means the row is either gone or - // RLS-hidden → deny. When `computeRlsFilter` returns `null` (no policy - // applies — e.g. an admin set with no RLS, or `modifyAllRecords`) the - // check is skipped and behaviour is unchanged. + // RLS-hidden → deny. When `computeRlsFilter` returns `null` the check + // is skipped — but since #7665 that is a far smaller class than "no + // WRITE policy applies": an empty write-class collection now derives + // its scope from the caller's SELECT narrowing inside + // `computeLayeredRlsFilter`, so the skip happens only when the caller's + // READABLE set is unbounded too (an admin set with no RLS at all, or a + // posture-permitting superuser bypass). // // [#5492] The filter is composed BY PROVENANCE. Two of the policies that // can land in it are the platform's OWN ownership floor @@ -1586,8 +1590,12 @@ export class SecurityPlugin implements Plugin { // 2.8. ADR-0055 — controlled-by-parent WRITE: a detail write (insert/update/ // delete) requires edit access to its master. The detail itself carries no - // authored RLS, so the #1994 pre-image check above is a no-op for it; this - // closes the by-id write path by checking the master instead. + // authored RLS (nothing to derive from either, #7665), so the #1994 + // pre-image check above is a no-op for it; this closes the by-id write + // path by checking the master instead — and the master's own write + // filter, since #7665, derives from its SELECT narrowing when no + // write-class policy applies, so a select-only master gates its details + // by visibility too. if ( ['insert', 'update', 'delete', 'transfer', 'restore', 'purge'].includes(opCtx.operation) && permissionSets.length > 0 && @@ -3925,7 +3933,54 @@ export class SecurityPlugin implements Plugin { // longer skip the tenant wall (that is Layer 0's own exemption, below). let layer1: Record | null = null; if (!(posturePermits && superuserBypass)) { - const collected = this.collectRLSPolicies(permissionSets, object, operation, (context?.positions ?? []) as string[]); + let collected = this.collectRLSPolicies(permissionSets, object, operation, (context?.positions ?? []) as string[]); + // [#7665] The write-visibility floor: a write target must be inside the + // caller's READABLE set. When NO policy of the write class applies to + // this (principal, object, operation) — nothing authored for the class, + // and the platform ownership floor outside its `positions` domain — the + // write class used to compile to a null Layer 1, and every write-side + // row gate composed from it became a no-op at once: the by-id pre-image + // gate (step 2.7), the controlled_by_parent master check, and the bulk + // write AST injection. QA #7637 measured the result on the stock + // showcase (select-only narrowing, `contributor` position, OWD + // `public_read_write`): a contributor PATCHed by id records they could + // not read — on the master AND on a controlled_by_parent detail — while + // the read side correctly hid them. + // + // So an empty write-class collection now DERIVES the write scope from + // the caller's SELECT narrowing — the same policies, compiled by the + // same compiler, that the read path enforces. "You cannot mutate what + // you cannot see" then holds by construction, and the explain engine + // reports the same narrowing for update/delete that it reports for + // read, instead of "No RLS policy applies". + // + // Deliberately NOT derived: + // - when ANY write-class policy applies (an authored predicate, or + // the in-domain platform floor): those paths keep their exact + // semantics, including every widening direction #7401 / #6736 + // track — #7665 criterion 5 (derive ONLY when no update-scope + // predicate exists). `checkAuthoredRowWrite` is additionally + // protected by its own authored-set pre-check, so a derived scope + // can never masquerade as an authored admission (#5493 / #7281); + // - for `insert` — there is no pre-existing row to be visible (a + // controlled_by_parent detail INSERT is still gated through its + // master's derived scope by step 2.8); + // - when the caller holds the read-side superuser bypass on a + // posture-permitting object — their readable set is unbounded, so + // the readability requirement imposes nothing. This is the mirror + // of the read path's own Layer-1 short-circuit above, so the + // derived write scope can never be NARROWER than the read scope it + // is derived from. + if ( + collected.length === 0 && + (operation === 'update' || operation === 'delete') && + !( + posturePermits && + this.permissionEvaluator.hasSuperuserReadBypass(object, permissionSets, { isPrivate: meta.isPrivate }) + ) + ) { + collected = this.collectRLSPolicies(permissionSets, object, 'select', (context?.positions ?? []) as string[]); + } // [#5492] Provenance composition: the caller (the by-id write pre-image // gate) has already asked the declared write authority — `ISharingService` // — and received a positive `allow`. Its answer REPLACES the platform's own diff --git a/packages/plugins/plugin-security/src/select-only-write-visibility.test.ts b/packages/plugins/plugin-security/src/select-only-write-visibility.test.ts new file mode 100644 index 0000000000..42a7e8f630 --- /dev/null +++ b/packages/plugins/plugin-security/src/select-only-write-visibility.test.ts @@ -0,0 +1,640 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// [#7665] A by-id write must be gated by record VISIBILITY when the object's +// narrowing is authored as select-only RLS. +// +// QA run #7637 measured the strongest security finding of the sweep on the +// stock showcase: a contributor whose narrowing is authored ONLY as +// `operation: 'select'` rules (`invoice_own_rows`, `task_own_rows`) could +// PATCH by id records they cannot read — 200, values persisted — on the +// master object AND on a `controlled_by_parent` detail, while the read side +// correctly 404'd. Mechanism: the 2.7 by-id write pre-image gate composes the +// RLS filter for the WRITE operation; with no update-scope policy applicable +// (the platform ownership floor is positions-gated to `org_member`, and the +// showcase contributor holds `contributor`), that filter compiled to a null +// Layer 1 and the gate was a documented no-op. OWD `public_read_write` made +// `resolveSharingCanEdit` answer true, and `assertControlledByParentWrite` +// derived the detail's write access from that same permissive master verdict. +// +// The fix (issue option A, the platform-side one): when NO policy of the +// write class applies to (principal, object, operation), the write scope is +// DERIVED FROM THE CALLER'S SELECT NARROWING — the same predicate the read +// path enforces — inside `computeLayeredRlsFilter`. "You cannot mutate what +// you cannot see" then holds by construction on the by-id gate, the +// controlled_by_parent master check, and the bulk write AST, and the explain +// engine reports the same narrowing for update/delete that it reports for +// read. +// +// ⚠️ THE PROBE PERSONAS HERE HOLD THE OBJECT READ+EDIT (+DELETE) GRANTS and +// are outside the RECORD scope — the issue's acceptance criterion 2. The +// `verify --rls` proof persona holds no object grants at all, so every one of +// its probes is masked by the object-level CRUD gate before record scope is +// ever consulted; a re-tagged version of it is structurally unable to fail +// and is exactly what this file must not be. +// +// What must NOT change (criterion 5 — the widener-dead directions #7401 and +// #6736 track, and the bypass/floor paths): +// - an object WITH an authored update-scope rule keeps today's behavior in +// BOTH directions (the widener still widens; its boundary still refuses); +// - the `org_member` platform ownership floor path is untouched; +// - a read-side superuser bypass holder (viewAllRecords on a +// posture-permitting object) is not newly narrowed — their readable set +// is unbounded, so the readability requirement imposes nothing. +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { assertEngineDeleteDispatch, assertEngineUpdateDispatch } from '@objectstack/metadata-core'; +import { SharingService, buildSharingMiddleware } from '@objectstack/plugin-sharing'; +import { PermissionSetSchema } from '@objectstack/spec/security'; +import type { PermissionSet } from '@objectstack/spec/security'; +import { BUILTIN_OPERATION_MESSAGES } from '@objectstack/spec/system'; +import { SecurityPlugin } from './security-plugin.js'; +import { defaultPermissionSets } from './objects/default-permission-sets.js'; + +// ── metadata ─────────────────────────────────────────────────────────────── + +/** + * The measured shape, exactly as the showcase ships it: OWD + * `public_read_write` (so record sharing widens writes rather than narrowing + * them), an ORDINARY access posture, and select-only RLS authored against an + * email-shaped owner column. + */ +const TICKET_SCHEMA = { + name: 'qa_ticket', + sharingModel: 'public_read_write', + fields: { + id: { name: 'id' }, + title: { name: 'title' }, + next_step: { name: 'next_step' }, + owner: { name: 'owner' }, + created_by: { name: 'created_by' }, + organization_id: { name: 'organization_id' }, + }, +}; + +/** The DETAIL — access derived from `qa_ticket` through the master_detail FK. */ +const LINE_SCHEMA = { + name: 'qa_ticket_line', + sharingModel: 'controlled_by_parent', + fields: { + id: { name: 'id' }, + description: { name: 'description' }, + quantity: { name: 'quantity' }, + ticket: { name: 'ticket', type: 'master_detail', required: true, reference: 'qa_ticket' }, + created_by: { name: 'created_by' }, + organization_id: { name: 'organization_id' }, + }, +}; + +/** + * The #7401 / #6736 CONTROL: an object that DOES author an update-scope rule + * beside its select narrowing. Derive-from-select must never fire for it — + * the authored update predicate keeps deciding alone, in both directions. + */ +const DOC_SCHEMA = { + name: 'qa_doc', + sharingModel: 'public_read_write', + fields: { + id: { name: 'id' }, + body: { name: 'body' }, + stage: { name: 'stage' }, + owner: { name: 'owner' }, + created_by: { name: 'created_by' }, + organization_id: { name: 'organization_id' }, + }, +}; + +/** + * The read-BYPASS control: `access.default: 'private'` makes the posture + * permit the ADR-0066 ① superuser short-circuits, and the auditor below holds + * `viewAllRecords` WITHOUT `modifyAllRecords` — the one combination where the + * write class compiles to nothing applicable while the read class is + * unbounded. Derivation must recognise the unbounded readable set and impose + * nothing. + */ +const VAULT_SCHEMA = { + name: 'qa_vault', + sharingModel: 'public_read_write', + access: { default: 'private' }, + fields: { + id: { name: 'id' }, + body: { name: 'body' }, + owner: { name: 'owner' }, + created_by: { name: 'created_by' }, + organization_id: { name: 'organization_id' }, + }, +}; + +const SCHEMAS: Record = { + qa_ticket: TICKET_SCHEMA, + qa_ticket_line: LINE_SCHEMA, + qa_doc: DOC_SCHEMA, + qa_vault: VAULT_SCHEMA, +}; + +/** + * The real platform baseline — additive for every authenticated member. Its + * `owner_only_writes` / `owner_only_deletes` floor is positions-gated to + * `org_member`, and the contributor personas below deliberately do NOT hold + * that position: that domain miss is the exact reason the write class had no + * applicable policy on the showcase (#7665 root cause), so it must be present + * here for the fixture to reproduce the measured shape rather than a + * simplified one. + */ +const MEMBER_DEFAULT = defaultPermissionSets.find((p) => p.name === 'member_default')!; + +/** + * Criterion 2's probe profile: read+edit+delete on every object under test + * (delete deliberately, so a refused delete below is the ROW gate refusing, + * never the CRUD bit), narrowed only by select-scope RLS. + */ +const QA_CONTRIBUTOR: PermissionSet = PermissionSetSchema.parse({ + name: 'qa_contributor', + objects: { + qa_ticket: { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true }, + qa_ticket_line: { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true }, + qa_doc: { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true }, + }, + rowLevelSecurity: [ + { + name: 'ticket_own_rows', + object: 'qa_ticket', + operation: 'select', + using: 'owner == current_user.email', + positions: ['contributor'], + }, + // No qa_ticket_line rule is authored — the line follows its master via + // controlled_by_parent, exactly as the showcase comments promise. + { + name: 'doc_select_own', + object: 'qa_doc', + operation: 'select', + using: 'owner == current_user.email', + positions: ['contributor'], + }, + // The authored UPDATE widener — #7401/#6736's protected direction. Its + // presence must switch derive-from-select OFF for qa_doc entirely. + { + name: 'doc_update_open', + object: 'qa_doc', + operation: 'update', + using: "stage == 'open'", + positions: ['contributor'], + }, + ], +}); + +/** viewAllRecords WITHOUT modifyAllRecords — the read-bypass control. */ +const QA_AUDITOR: PermissionSet = PermissionSetSchema.parse({ + name: 'qa_auditor', + objects: { + qa_vault: { allowRead: true, allowEdit: true, allowDelete: true, viewAllRecords: true }, + }, + rowLevelSecurity: [ + { + name: 'vault_own_rows', + object: 'qa_vault', + operation: 'select', + using: 'owner == current_user.email', + positions: ['contributor'], + }, + ], +}); + +const PERMISSION_SETS: PermissionSet[] = [MEMBER_DEFAULT, QA_CONTRIBUTOR, QA_AUDITOR]; + +// ── rows ─────────────────────────────────────────────────────────────────── + +const C1 = { userId: 'u_c1', email: 'c1@example.com' }; +const C2 = { userId: 'u_c2', email: 'c2@example.com' }; + +const TICKET_C1 = { + id: 'tk_c1', title: 'C1 ticket', next_step: 'call', + owner: C1.email, created_by: C1.userId, organization_id: 'org1', +}; +const TICKET_C2 = { + id: 'tk_c2', title: 'C2 ticket', next_step: 'call', + owner: C2.email, created_by: C2.userId, organization_id: 'org1', +}; +const LINE_C1 = { + id: 'ln_c1', description: 'C1 line', quantity: 1, + ticket: TICKET_C1.id, created_by: C1.userId, organization_id: 'org1', +}; +const DOC_OPEN_C1 = { + id: 'doc_open', body: 'open doc', stage: 'open', + owner: C1.email, created_by: C1.userId, organization_id: 'org1', +}; +const DOC_CLOSED_C1 = { + id: 'doc_closed', body: 'closed doc', stage: 'closed', + owner: C1.email, created_by: C1.userId, organization_id: 'org1', +}; +const VAULT_C1 = { + id: 'va_c1', body: 'vault row', owner: C1.email, + created_by: C1.userId, organization_id: 'org1', +}; + +// ── in-memory engine ─────────────────────────────────────────────────────── + +function makeEngine() { + const tables: Record = { + qa_ticket: [{ ...TICKET_C1 }, { ...TICKET_C2 }], + qa_ticket_line: [{ ...LINE_C1 }], + qa_doc: [{ ...DOC_OPEN_C1 }, { ...DOC_CLOSED_C1 }], + qa_vault: [{ ...VAULT_C1 }], + sys_record_share: [], + }; + const matches = (row: any, filter: any): boolean => { + if (!filter || typeof filter !== 'object') return true; + if (Array.isArray(filter.$or)) return filter.$or.some((f: any) => matches(row, f)); + if (Array.isArray(filter.$and)) return filter.$and.every((f: any) => matches(row, f)); + for (const [k, v] of Object.entries(filter)) { + if (k === '$or' || k === '$and') continue; + if (v != null && typeof v === 'object' && '$in' in (v as any)) { + if (!(v as any).$in.includes(row[k])) return false; + continue; + } + if (row[k] !== v) return false; + } + return true; + }; + const middlewares: any[] = []; + return { + _tables: tables, + _middlewares: middlewares, + registerMiddleware: (mw: any) => middlewares.push(mw), + getSchema: (name: string) => SCHEMAS[name], + async find(object: string, options: any = {}) { + const rows = (tables[object] ??= []); + return rows.filter((r) => matches(r, options.filter ?? options.where)).slice(0, options.limit ?? 1000); + }, + async findOne(object: string, options: any = {}) { + const rows = await this.find(object, { ...options, limit: 1 }); + return rows[0] ?? null; + }, + async insert(object: string, data: any) { + (tables[object] ??= []).push({ ...data }); + return data; + }, + // Both write verbs open with the PRODUCER's own dispatch predicate + // (#4550 / #5480 / #6277), never a hand-mirrored guard. + async update(object: string, data: any, options?: any) { + const dispatch = assertEngineUpdateDispatch(data, options); + const rows = (tables[object] ??= []); + const targets = dispatch.kind === 'by-id' + ? rows.filter((r) => r.id === dispatch.id) + : rows.filter((r) => matches(r, options?.where)); + for (const r of targets) Object.assign(r, data); + return dispatch.kind === 'by-id' ? (targets[0] ?? null) : targets.length; + }, + async delete(object: string, options?: any) { + const dispatch = assertEngineDeleteDispatch(options); + const rows = (tables[object] ??= []); + const targets = dispatch.kind === 'by-id' + ? rows.filter((r) => r.id === dispatch.id) + : rows.filter((r) => matches(r, options?.where)); + tables[object] = rows.filter((r) => !targets.includes(r)); + return dispatch.kind === 'by-id' ? targets.length > 0 : targets.length; + }, + }; +} + +// ── the stack ────────────────────────────────────────────────────────────── + +interface WriteOutcome { + ok: boolean; + /** ADR-0112 envelope of the refusal — asserted, never a bare `toThrow()`. */ + code?: string; + status?: number; + message: string; + developerMessage?: string; +} + +interface Stack { + security: any; + engine: any; + write: ( + operation: 'insert' | 'update' | 'delete', + object: string, + payload: { recordId?: string; data?: Record; where?: Record }, + context: any, + ) => Promise; + read: (object: string, context: any, where?: Record) => Promise; + rows: (object: string) => any[]; +} + +async function makeStack(opts: { orgScoping?: boolean } = {}): Promise { + const orgScoping = opts.orgScoping ?? true; + const engine = makeEngine(); + const metadata = { + get: async (_type: string, name: string) => SCHEMAS[name] ?? null, + list: async () => PERMISSION_SETS, + }; + let security: any; + let sharing: SharingService; + const services: Record = { + manifest: { register: vi.fn() }, + objectql: engine, + metadata, + // Org scoping active by DEFAULT, as a multi-tenant deployment wires it — + // Layer 0 contributes a real tenant predicate, so a green on the + // enforcement describes can never be "the write filter was empty for the + // tenant reason instead". + // + // ⚠️ The explain describe below deliberately turns it OFF, and that is not + // a convenience: with a wall in force Layer 0 is non-null for EVERY + // operation, and the rls layer's verdict is read off the COMPOSED + // `layer0 AND layer1` (`explain-engine.ts`, `readFilter ? 'narrows' : + // 'not_applicable'`). A `narrows` assertion under an active wall is + // therefore satisfied by the tenant predicate alone and holds whether or + // not this fix exists — measured: it stayed GREEN under the full ablation. + // Posture `single` makes Layer 0 null, so the verdict is decided by Layer 1 + // alone and the assertion can actually fail. This also reproduces the + // card's measured signal exactly, which came from a deployment reporting + // `not_applicable` — only a null Layer 0 can report that. + ...(orgScoping ? { 'org-scoping': { name: 'org-scoping' } } : {}), + get sharing() { return sharing; }, + }; + const ctx: any = { + logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }, + registerService: (name: string, impl: any) => { if (name === 'security') security = impl; }, + getService: (name: string) => { + if (!(name in services)) throw new Error(`service not registered: ${name}`); + return services[name]; + }, + }; + const plugin = new SecurityPlugin({ fallbackPermissionSet: 'member_default' }); + await plugin.init(ctx); + await plugin.start(ctx); + if (!security) throw new Error('SecurityPlugin did not register the security service'); + + sharing = new SharingService({ engine: engine as any, securityService: () => security }); + const sharingMw = buildSharingMiddleware(sharing, ctx.logger); + const securityMw = engine._middlewares[0]; + + const run = async (opCtx: any): Promise => { + let reached = false; + try { + await securityMw(opCtx, async () => { + await sharingMw(opCtx, async () => { + if (opCtx.operation === 'delete') await engine.delete(opCtx.object, opCtx.options); + else if (opCtx.operation === 'insert') await engine.insert(opCtx.object, opCtx.data); + else await engine.update(opCtx.object, opCtx.data, opCtx.options); + reached = true; + }); + }); + } catch (e: any) { + return { + ok: false, + code: e?.code, + status: e?.statusCode, + message: String(e?.message ?? e), + developerMessage: e?.developerMessage, + }; + } + return reached + ? { ok: true, message: 'written' } + : { ok: false, message: 'middleware swallowed the write' }; + }; + + return { + security, + engine, + rows: (object: string) => (engine._tables[object] ??= []), + async write(operation, object, payload, context) { + // The by-id dispatch shape — no `ast`, which is the whole #1994/#7665 + // class: only the pre-image gates stand between the caller and the row. + // (The bulk path has its own describe below, driving the middleware's + // AST injection directly.) + const opCtx: any = { object, operation, context: { ...context } }; + if (operation === 'insert') { + opCtx.data = { ...payload.data }; + } else if (operation === 'update') { + opCtx.data = { id: payload.recordId, ...payload.data }; + } else { + opCtx.options = { where: { id: payload.recordId } }; + } + return run(opCtx); + }, + async read(object, context, where) { + const opCtx: any = { + object, + operation: 'find', + context: { ...context }, + options: { where: where ?? {} }, + ast: { where: where ?? {} }, + }; + let result: any[] = []; + await securityMw(opCtx, async () => { + await sharingMw(opCtx, async () => { + result = await engine.find(object, { where: opCtx.ast.where }); + }); + }); + return result; + }, + }; +} + +/** The showcase-contributor shape: NOT an org_member — the floor's domain miss. */ +const ctxFor = (u: { userId: string; email: string }, ...permissions: string[]) => ({ + userId: u.userId, email: u.email, tenantId: 'org1', positions: ['contributor'], permissions, +}); + +const C1_CTX = ctxFor(C1, 'qa_contributor'); +const C2_CTX = ctxFor(C2, 'qa_contributor'); +const AUDITOR_CTX = ctxFor(C2, 'qa_auditor'); + +/** [#7451] The 2.7 pre-image gate's exact two-axis envelope. */ +function expectRowLevelDenial(outcome: WriteOutcome, operation: 'update' | 'delete', object: string) { + expect(outcome.ok, `expected a refusal, got a completed ${operation}`).toBe(false); + expect(outcome.code, 'ADR-0112 error code').toBe('PERMISSION_DENIED'); + expect(outcome.status, 'ADR-0112 HTTP status').toBe(403); + expect(outcome.message, 'the user half is the localized catalog sentence') + .toBe(BUILTIN_OPERATION_MESSAGES.en.record_access_denied); + expect(outcome.developerMessage, 'the developer half names WHICH gate refused').toContain( + `[Security] Access denied: not permitted to ${operation} this '${object}' record (row-level security)`, + ); +} + +/** The controlled_by_parent master gate's envelope (its sentence IS `message`). */ +function expectMasterEditDenial(outcome: WriteOutcome, operation: string, object: string) { + expect(outcome.ok, `expected a refusal, got a completed ${operation}`).toBe(false); + expect(outcome.code, 'ADR-0112 error code').toBe('PERMISSION_DENIED'); + expect(outcome.status, 'ADR-0112 HTTP status').toBe(403); + expect(outcome.message).toContain( + `[Security] Access denied: ${operation} on '${object}' requires edit access to its master record`, + ); + expect(outcome.message, 'the RLS half of the master gate is what refused').toContain('(row-level security)'); +} + +const rowById = (stack: Stack, object: string, id: string) => + stack.rows(object).find((r) => r.id === id); + +// ─────────────────────────────────────────────────────────────────────────── + +describe('[#7665] select-only RLS gates the by-id WRITE on the master object', () => { + let stack: Stack; + beforeEach(async () => { stack = await makeStack(); }); + + it('out-of-scope by-id UPDATE is refused with the row-level envelope, and the row does not change', async () => { + const out = await stack.write('update', 'qa_ticket', { recordId: TICKET_C1.id, data: { next_step: 'C2-FORGED' } }, C2_CTX); + expectRowLevelDenial(out, 'update', 'qa_ticket'); + expect(rowById(stack, 'qa_ticket', TICKET_C1.id)?.next_step).toBe('call'); + }); + + it('out-of-scope by-id DELETE is refused by the ROW gate (the CRUD delete bit is held)', async () => { + const out = await stack.write('delete', 'qa_ticket', { recordId: TICKET_C1.id }, C2_CTX); + expectRowLevelDenial(out, 'delete', 'qa_ticket'); + expect(rowById(stack, 'qa_ticket', TICKET_C1.id)).toBeDefined(); + }); + + it('a legitimately in-scope by-id UPDATE still succeeds and the row really changes', async () => { + const out = await stack.write('update', 'qa_ticket', { recordId: TICKET_C2.id, data: { next_step: 'updated' } }, C2_CTX); + expect(out, out.message).toMatchObject({ ok: true }); + expect(rowById(stack, 'qa_ticket', TICKET_C2.id)?.next_step).toBe('updated'); + }); + + it('the read side is unchanged: the out-of-scope row stays invisible to find', async () => { + const rows = await stack.read('qa_ticket', C2_CTX); + expect(rows.map((r) => r.id)).toEqual([TICKET_C2.id]); + }); +}); + +describe('[#7665] the same requirement holds on the controlled_by_parent detail', () => { + let stack: Stack; + beforeEach(async () => { stack = await makeStack(); }); + + it('by-id UPDATE of a line under an unreadable master is refused by the master gate', async () => { + const out = await stack.write('update', 'qa_ticket_line', { recordId: LINE_C1.id, data: { description: 'C2-FORGED', quantity: 999 } }, C2_CTX); + expectMasterEditDenial(out, 'update', 'qa_ticket_line'); + expect(rowById(stack, 'qa_ticket_line', LINE_C1.id)?.description).toBe('C1 line'); + }); + + it('INSERT of a line under an unreadable master is refused by the master gate', async () => { + const out = await stack.write('insert', 'qa_ticket_line', { data: { id: 'ln_new', description: 'forged', ticket: TICKET_C1.id, organization_id: 'org1' } }, C2_CTX); + expectMasterEditDenial(out, 'insert', 'qa_ticket_line'); + expect(rowById(stack, 'qa_ticket_line', 'ln_new')).toBeUndefined(); + }); + + it('the owner still writes their own line — the master gate admits an in-scope master', async () => { + const out = await stack.write('update', 'qa_ticket_line', { recordId: LINE_C1.id, data: { description: 'C1 edit' } }, C1_CTX); + expect(out, out.message).toMatchObject({ ok: true }); + expect(rowById(stack, 'qa_ticket_line', LINE_C1.id)?.description).toBe('C1 edit'); + }); +}); + +describe("[#7665] criterion 5 — an authored update-scope rule keeps deciding ALONE (#7401/#6736's protected direction)", () => { + let stack: Stack; + beforeEach(async () => { stack = await makeStack(); }); + + it('the authored widener still admits a row OUTSIDE the select scope (derive-from-select must not AND in)', async () => { + // C2 cannot read doc_open (owner C1) — but the authored update rule admits + // any doc in 'open'. If derive-from-select fired despite the authored + // rule, this write would newly 403: the exact #6736 defect family. + // + // ⚠️ SCOPE OF THIS PIN, stated precisely because the green is easy to + // over-read. What it establishes is the FILTER COMPOSITION at the layer + // this fix edits: an applicable authored write predicate keeps deciding + // alone, and the derived select scope is not AND-ed into it. It does NOT + // establish that the widener works end-to-end on a real stack: there the + // pre-image gate's `findOne` runs under the CALLER's context and re-enters + // the middleware chain, so the caller's READ narrowing applies to the + // probe read as well — the mechanism #7401 measured (`public_read` → 200, + // `private` → 403, same widener). This fake engine's `findOne` does not + // re-enter the chain, so that second, independent gate is deliberately not + // simulated here. #7401 owns that question and is untouched by #7665: + // mutation-tested by forcing derivation unconditionally, which turns THIS + // case red and nothing else. + const out = await stack.write('update', 'qa_doc', { recordId: DOC_OPEN_C1.id, data: { body: 'widened edit' } }, C2_CTX); + expect(out, out.message).toMatchObject({ ok: true }); + expect(rowById(stack, 'qa_doc', DOC_OPEN_C1.id)?.body).toBe('widened edit'); + }); + + it("the authored rule's own boundary still refuses (stage != 'open')", async () => { + const out = await stack.write('update', 'qa_doc', { recordId: DOC_CLOSED_C1.id, data: { body: 'forged' } }, C2_CTX); + expectRowLevelDenial(out, 'update', 'qa_doc'); + expect(rowById(stack, 'qa_doc', DOC_CLOSED_C1.id)?.body).toBe('closed doc'); + }); + + it('class-per-class: the DELETE class (no authored delete rule) still derives from select on the same object', async () => { + // Criterion 5 protects each write CLASS that has its own authored + // predicate — qa_doc's `update` class. Its `delete` class authors nothing, + // so there is no delete-widener to kill: an out-of-visibility by-id DELETE + // is refused by the derived readable-set scope. Pinned deliberately so the + // class-per-class reading is a measured contract, not an accident. + const out = await stack.write('delete', 'qa_doc', { recordId: DOC_OPEN_C1.id }, C2_CTX); + expectRowLevelDenial(out, 'delete', 'qa_doc'); + expect(rowById(stack, 'qa_doc', DOC_OPEN_C1.id)).toBeDefined(); + }); +}); + +describe('[#7665] bypass and floor paths are untouched', () => { + let stack: Stack; + beforeEach(async () => { stack = await makeStack(); }); + + it('a viewAllRecords holder (no modifyAllRecords) is NOT newly narrowed — the readable set is unbounded', async () => { + const out = await stack.write('update', 'qa_vault', { recordId: VAULT_C1.id, data: { body: 'audit edit' } }, AUDITOR_CTX); + expect(out, out.message).toMatchObject({ ok: true }); + expect(rowById(stack, 'qa_vault', VAULT_C1.id)?.body).toBe('audit edit'); + }); + + it("an org_member's cross-creator write is still refused by the platform floor, not by derivation", async () => { + const memberCtx = { ...C2_CTX, positions: ['org_member', 'contributor'] }; + const out = await stack.write('update', 'qa_ticket', { recordId: TICKET_C1.id, data: { next_step: 'forged' } }, memberCtx); + expectRowLevelDenial(out, 'update', 'qa_ticket'); + expect(rowById(stack, 'qa_ticket', TICKET_C1.id)?.next_step).toBe('call'); + }); +}); + +describe('[#7665] the bulk write path is scoped by the same derived visibility', () => { + let stack: Stack; + beforeEach(async () => { stack = await makeStack(); }); + + it('an unfiltered multi-UPDATE touches only readable rows', async () => { + const opCtx: any = { + object: 'qa_ticket', + operation: 'update', + context: { ...C2_CTX }, + data: { next_step: 'bulk-edit' }, + options: { where: {}, multi: true }, + ast: { where: {} }, + }; + const securityMw = stack.engine._middlewares[0]; + await securityMw(opCtx, async () => { + await stack.engine.update(opCtx.object, opCtx.data, { ...opCtx.options, where: opCtx.ast.where, multi: true }); + }); + expect(rowById(stack, 'qa_ticket', TICKET_C2.id)?.next_step).toBe('bulk-edit'); + expect(rowById(stack, 'qa_ticket', TICKET_C1.id)?.next_step, 'the unreadable row must stay untouched').toBe('call'); + }); +}); + +describe('[#7665] the explain engine tells the same story it enforces', () => { + // Posture `single` (no org wall) — see the note in `makeStack`. Under an + // active wall this whole describe is vacuous: Layer 0 alone reports + // `narrows` for every operation. + let stack: Stack; + beforeEach(async () => { stack = await makeStack({ orgScoping: false }); }); + + const rlsLayer = (decision: any) => decision.layers.find((l: any) => l.layer === 'rls'); + + it("operation:'update' on a select-only object now reports rls 'narrows' (the #7637 confirmation ran the other way: 'not_applicable — No RLS policy applies')", async () => { + const layer = rlsLayer(await stack.security.explain({ object: 'qa_ticket', operation: 'update' }, C2_CTX)); + expect(layer.verdict).toBe('narrows'); + // The sentence too: `not_applicable` ships "No RLS policy applies." — the + // exact string QA #7637 quoted as the confirmation of the split. + expect(layer.detail).toContain('Row-level security narrows the row set'); + expect(layer.detail).not.toContain('No RLS policy applies'); + }); + + it("operation:'delete' reports the same narrowing (the class the card's DELETE half rides on)", async () => { + expect(rlsLayer(await stack.security.explain({ object: 'qa_ticket', operation: 'delete' }, C2_CTX)).verdict) + .toBe('narrows'); + }); + + it("operation:'read' keeps reporting 'narrows' exactly as before — the split #7637 measured is CLOSED, not inverted", async () => { + expect(rlsLayer(await stack.security.explain({ object: 'qa_ticket', operation: 'read' }, C2_CTX)).verdict) + .toBe('narrows'); + }); + + it("operation:'create' still reports 'not_applicable' — insert has no pre-image to be visible, so nothing is derived for it", async () => { + expect(rlsLayer(await stack.security.explain({ object: 'qa_ticket', operation: 'create' }, C2_CTX)).verdict) + .toBe('not_applicable'); + }); +}); diff --git a/packages/qa/dogfood/test/fixtures/rls-owner-fixture.ts b/packages/qa/dogfood/test/fixtures/rls-owner-fixture.ts index 9bc05a0b41..1108e82d05 100644 --- a/packages/qa/dogfood/test/fixtures/rls-owner-fixture.ts +++ b/packages/qa/dogfood/test/fixtures/rls-owner-fixture.ts @@ -86,11 +86,19 @@ export const ownerScopedMemberSet: PermissionSet = PermissionSetSchema.parse({ }); /** - * RED. Owner policy on SELECT only — reads stay owner-scoped (member still - * can't see others' notes) but no UPDATE/DELETE policy applies, so - * `computeRlsFilter` returns null for the write op and the pre-image check is - * skipped → the by-id write lands. The member mutated a row it could not read: - * the #1994 hole class. Expected runner verdict: `rls-hole`. + * Owner policy on SELECT only — the #1994 hole class's authoring shape, and + * the shape QA #7637 measured live on the stock showcase (#7665). + * + * Until #7665 this variant was the automated RED proof: no UPDATE/DELETE + * policy applied, `computeRlsFilter` returned null for the write op, the + * pre-image check was skipped, and the by-id write landed on a row the member + * could not read — expected runner verdict `rls-hole`. #7665 closed the class + * platform-side: an empty write-class policy collection now derives its scope + * from the caller's SELECT narrowing, so the same authoring shape yields + * `rls-consistent` — which is exactly what the consuming test now pins as the + * end-to-end regression guard for #7665. (Detector liveness — "the runner CAN + * answer `rls-hole`" — is pinned by `rls-runner.test.ts` on a scripted stack, + * where the hole can still be planted.) */ export const readOnlyScopedMemberSet: PermissionSet = PermissionSetSchema.parse({ name: FIXTURE_MEMBER_SET, diff --git a/packages/qa/dogfood/test/rls-fixture.dogfood.test.ts b/packages/qa/dogfood/test/rls-fixture.dogfood.test.ts index cd7536a871..9a773de438 100644 --- a/packages/qa/dogfood/test/rls-fixture.dogfood.test.ts +++ b/packages/qa/dogfood/test/rls-fixture.dogfood.test.ts @@ -17,12 +17,26 @@ // // • owner policy on ALL ops → `rls-consistent` (green gate). Safe ONLY // because the #1994 pre-image check enforces the by-id write — revert that -// fix and this flips to `rls-hole` (see README for the manual revert proof, -// and the RED block below for the automated analogue). -// • owner policy on SELECT only → `rls-hole` (automated red proof). The read -// is owner-scoped but no write policy applies, so the by-id write lands — -// the #1994 hole class, reproduced without touching engine code. This proves -// the gate can actually go red. +// fix and this flips to `rls-hole` (see README for the manual revert proof). +// • owner policy on SELECT only → `rls-consistent` since #7665. This block +// was the automated RED proof of the #1994 hole class (select-only reads +// scoped, by-id write landed); QA #7637 then measured the same class live +// on the stock showcase, and #7665 closed it platform-side by deriving the +// write scope from the caller's select narrowing. The block is now the +// END-TO-END regression guard for #7665: revert that derivation and it +// flips back to `rls-hole`. Measured both ways, on a real HTTP stack: +// with the fix `PATCH 403, row unchanged`; without it `PATCH 200` and the +// row mutated by a member who gets `GET 404` on the same id. +// ⚠️ To reproduce that revert you MUST rebuild the plugin, not just edit +// its source: this suite resolves `@objectstack/plugin-security` through +// its built `dist`, so a source-only revert runs against the previous +// build and reports a FALSE GREEN (`rls-consistent` with no fix present). +// `pnpm --filter @objectstack/plugin-security build` between the edit and +// the run is what makes the ablation real. +// The "runner can actually answer rls-hole" liveness this block used to +// carry lives in `rls-runner.test.ts`, whose scripted stack can still +// plant the hole — verified to stay green in BOTH states above, so it is +// an oracle this fix cannot switch off. import { describe, it, expect, beforeAll, afterAll } from 'vitest'; import { bootStack, type VerifyStack } from '@objectstack/verify'; @@ -84,17 +98,24 @@ describe('objectstack verify RLS: owner-isolated fixture (#1994 hard gate)', () }); }); - // ── RED: proof the gate can go red on the #1994 hole class ────────────────── - describe('read-only-scoped member set (select only) — #1994 hole reproduced', () => { + // ── [#7665] select-only narrowing gates the by-id write end-to-end ───────── + // The probe persona is criterion 2's shape: the fixture member set grants + // full CRUD on `rls_note` (read+edit+delete), so a refusal below is the + // record-scope gate answering — never the object-level CRUD bit. (The + // `verify --rls` proof persona that holds NO object grants is structurally + // unable to fail; this member is the persona that can.) + describe('read-only-scoped member set (select only) — #7665: the write scope derives from select', () => { let stack: VerifyStack; let report: RlsReport; + let adminToken: string; + let memberToken: string; beforeAll(async () => { stack = await bootStack(rlsFixtureStack, { security: rlsFixtureSecurity(readOnlyScopedMemberSet), }); - const adminToken = await stack.signIn(); - const memberToken = await stack.signUp('owner-red@verify.test'); + adminToken = await stack.signIn(); + memberToken = await stack.signUp('owner-red@verify.test'); report = await runRlsProofs(stack, adminToken, memberToken, rlsFixtureStack); // eslint-disable-next-line no-console console.error(formatRlsReport(report)); @@ -104,10 +125,49 @@ describe('objectstack verify RLS: owner-isolated fixture (#1994 hard gate)', () await stack?.stop(); }); - it('rls_note is rls-hole — member cannot read it yet mutated it by id', () => { + it('rls_note is rls-consistent — the select-only #1994 hole class is closed (#7665)', () => { const note = report.results.find((r) => r.object === 'rls_note'); - expect(note?.status, formatRlsReport(report)).toBe('rls-hole'); - expect(report.summary.holes).toBe(1); + expect(note?.status, formatRlsReport(report)).toBe('rls-consistent'); + expect(report.summary.holes, formatRlsReport(report)).toBe(0); + }); + + it('by-id: the member still cannot READ the admin note (read side unchanged), and a direct PATCH is refused with the row unchanged', async () => { + const created = await stack.apiAs(adminToken, 'POST', '/data/rls_note', { + name: 'admin note 7665', + body: 'admin-only secret', + }); + expect(created.status).toBeLessThan(300); + const cj = (await created.json()) as { id?: string; record?: { id?: string } }; + const id = cj.id ?? cj.record?.id; + expect(id, 'admin create should return an id').toBeTruthy(); + + const bRead = await stack.apiAs(memberToken, 'GET', `/data/rls_note/${id}`); + expect(bRead.status, 'select-only narrowing must keep hiding the row').not.toBe(200); + + const bWrite = await stack.apiAs(memberToken, 'PATCH', `/data/rls_note/${id}`, { body: 'FORGED' }); + expect(bWrite.status, 'the out-of-visibility by-id write must be refused').toBeGreaterThanOrEqual(300); + + const after = await stack.apiAs(adminToken, 'GET', `/data/rls_note/${id}`); + const afterBody = (((await after.json()) as any)?.record ?? {}).body; + expect(afterBody, 'ground truth: the row must be untouched').toBe('admin-only secret'); + }); + + it('a legitimately in-scope write still succeeds — the member edits their OWN note', async () => { + const created = await stack.apiAs(memberToken, 'POST', '/data/rls_note', { + name: 'member note 7665', + body: 'mine', + }); + expect(created.status).toBeLessThan(300); + const cj = (await created.json()) as { id?: string; record?: { id?: string } }; + const id = cj.id ?? cj.record?.id; + expect(id, 'member create should return an id').toBeTruthy(); + + const edit = await stack.apiAs(memberToken, 'PATCH', `/data/rls_note/${id}`, { body: 'mine, edited' }); + expect(edit.status, 'the in-scope by-id write must still land').toBeLessThan(300); + + const after = await stack.apiAs(memberToken, 'GET', `/data/rls_note/${id}`); + const afterBody = (((await after.json()) as any)?.record ?? {}).body; + expect(afterBody).toBe('mine, edited'); }); }); }); From 23df0a396c44b461d012d93c8a359ff7da7a1d43 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 11 Aug 2026 15:10:43 +0000 Subject: [PATCH 2/2] docs(permissions): record that a `select` policy also bounds writes when no write-class policy applies MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- content/docs/permissions/rls.mdx | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/content/docs/permissions/rls.mdx b/content/docs/permissions/rls.mdx index 7173122a70..0dfdc09999 100644 --- a/content/docs/permissions/rls.mdx +++ b/content/docs/permissions/rls.mdx @@ -58,7 +58,7 @@ export const ContributorAccess = definePermissionSet({ | `name` | `string` | snake_case identifier | | `label` | `string` | Human-readable name | | `object` | `string` | Target object — or `'*'` to apply to every object | -| `operation` | `'select' \| 'insert' \| 'update' \| 'delete' \| 'all'` | Which operation the policy guards | +| `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 | | `using` | `string` | Predicate for rows the user may **see / act on** (compiled into the query filter) | | `check` | `string` | Predicate rows must satisfy **after a write**. Omit it and `using` is reused | | `positions` | `string[]` | Which positions the policy applies to. Omit = everyone | @@ -71,6 +71,17 @@ At least one of `using` / `check` is required. everything. Internally `find` / `findOne` / `count` / `aggregate` all map to `select`. +**`select` also bounds writes when nothing else does.** A write target must be +inside the caller's **readable** set, so when **no** write-class policy applies +to a caller on an object, the `update` / `delete` scope is derived from that +caller's `select` policies — a record they cannot read is one they cannot +modify, by id or in bulk. Authoring a write-class policy switches the +derivation off for that class: an authored `update` predicate then decides +`update` alone and widens exactly as written. Nothing is derived for `insert` +(there is no pre-existing row to be visible), and a caller holding the +read-side superuser bypass (`viewAllRecords` on a private or platform-global +object) is not narrowed, because their readable set is already unbounded. + ## The expression grammar RLS predicates are **canonical CEL**, lowered into a query filter by the shared