From 1fb616ee30e24aed12b5e5eab9208171851730ae Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 24 Sep 2026 14:20:44 +0000 Subject: [PATCH 1/5] fix(plugin-security): judge every row of an array insert against the row-level check A row-level `check` (declared, or defaulted from `using`) guarantees that no stored row fails it. The write gate installed that judgement for a single-row insert only: an array payload was excluded, so every row of an array insert was stored unjudged, including under policies that refuse every single-row insert. The gate now installs the same judgement for an array insert. The engine already hands it every live row after the `beforeInsert` chain, so each row is judged on the image that will be stored, and one failing row refuses the whole insert with the existing PERMISSION_DENIED / 403 refusal. A single-row insert is unchanged. Claude-Session: https://claude.ai/code/session_01Evb5jFDZGKQE9KG4jbMfMF Co-authored-by: Claude --- .../src/rls-check-defaults-to-using.test.ts | 2 +- .../src/rls-check-multi-row-writes.test.ts | 227 ++++++++++++++++++ .../src/rls-phantom-column-negation.test.ts | 7 +- .../src/security-plugin.test.ts | 5 +- .../plugin-security/src/security-plugin.ts | 18 +- 5 files changed, 249 insertions(+), 10 deletions(-) create mode 100644 packages/plugins/plugin-security/src/rls-check-multi-row-writes.test.ts diff --git a/packages/plugins/plugin-security/src/rls-check-defaults-to-using.test.ts b/packages/plugins/plugin-security/src/rls-check-defaults-to-using.test.ts index 0b0857b19b4..f68228572d4 100644 --- a/packages/plugins/plugin-security/src/rls-check-defaults-to-using.test.ts +++ b/packages/plugins/plugin-security/src/rls-check-defaults-to-using.test.ts @@ -164,7 +164,7 @@ const attempt = async (run: () => Promise): Promise => { } }; -/** ⚠️ A SINGLE object, never an array: step 3.6 skips a bulk payload. */ +/** A single-row insert; the array shape is pinned in `rls-check-multi-row-writes.test.ts`. */ const insert = (engine: ObjectQL, row: Record) => engine.insert('qa_ticket', { id: 't1', title: 't', ...row } as never, { context: CALLER } as never); diff --git a/packages/plugins/plugin-security/src/rls-check-multi-row-writes.test.ts b/packages/plugins/plugin-security/src/rls-check-multi-row-writes.test.ts new file mode 100644 index 00000000000..a71a94644e1 --- /dev/null +++ b/packages/plugins/plugin-security/src/rls-check-multi-row-writes.test.ts @@ -0,0 +1,227 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [ADR-0058 D4] A row-level `check` judges EVERY row a write stores — on an + * ARRAY insert as on a single-row one. + * + * ## The guarantee + * + * `RowLevelSecurityPolicySchema.check` (declared, or defaulted from `using`) is + * the write-side half of a row-level policy: a row the check refuses is never + * stored. An ARRAY insert (`insert(object, [rows])`, which `createManyData` + * calls) escaped it: step 3.6 of the security middleware excluded an array + * payload, so no judgement was installed at all. + * + * ## What this file pins, on both SQL driver families + * + * - the repro, failing first: `[admitted, refused]` is refused on the ADR-0112 + * envelope (`PERMISSION_DENIED` / 403) and NOTHING is stored; + * - the over-fix controls: an array whose every row passes is admitted and + * stored; the single insert answers exactly as before; + * - the configurations that refuse EVERY single insert refuse the array insert + * too (an unresolvable sole `using`, an unresolvable declared `check`). + * + * Ground truth is read under a system context: "the gate refused" and "nothing + * was stored" are separate facts, and both are asserted. + */ + +import { describe, it, expect, afterEach, vi } from 'vitest'; +import { ObjectQL } from '@objectstack/objectql'; +import { SqlDriver } from '@objectstack/driver-sql'; +import { SqliteWasmDriver } from '@objectstack/driver-sqlite-wasm'; +import { PermissionSetSchema } from '@objectstack/spec/security'; +import type { PermissionSet } from '@objectstack/spec/security'; +import { SecurityPlugin } from './security-plugin.js'; +import { defaultPermissionSets } from './objects/default-permission-sets.js'; + +const OBJECTS = [ + { + name: 'qa_ticket', + label: 'Ticket', + sharingModel: 'public_read_write', + fields: { + id: { name: 'id', type: 'text', primaryKey: true }, + title: { name: 'title', type: 'text' }, + status: { name: 'status', type: 'text' }, + priority: { name: 'priority', type: 'text' }, + owner: { name: 'owner', type: 'text' }, + }, + }, +]; + +const ME = 'a@e.example'; +const SOMEONE_ELSE = 'b@e.example'; + +/** The #19964 repro's predicate. */ +const NOT_ARCHIVED = "record.status != 'archived'"; +/** + * A `current_user.*` key no resolver publishes: the compiler cannot resolve it, + * drops the policy, and the gate falls back to the deny sentinel — so every + * single insert under it is refused. + */ +const UNRESOLVABLE = 'record.status in current_user.no_such_membership_key'; + +type Policy = { name: string; operation: string; using?: string; check?: string }; + +function permissionSet(policies: Policy[]): PermissionSet { + return PermissionSetSchema.parse({ + name: 'qa_writer', + objects: { qa_ticket: { allowRead: true, allowCreate: true, allowEdit: true } }, + rowLevelSecurity: policies.map((p) => ({ object: 'qa_ticket', ...p })), + }); +} + +const MEMBER_DEFAULT = defaultPermissionSets.find((p) => p.name === 'member_default')!; +const SYS_CTX = { isSystem: true, userId: 'usr_system' }; +const CALLER = { + userId: 'usr_a', + email: ME, + positions: ['writer'], + permissions: ['qa_writer'], + posture: 'MEMBER', +}; + +const engines: ObjectQL[] = []; +afterEach(async () => { + while (engines.length) { + try { await engines.pop()?.destroy(); } catch { /* noop */ } + } +}); + +const SEED = [ + { id: 't1', title: 'one', status: 'open', priority: 'low', owner: ME }, + { id: 't2', title: 'two', status: 'open', priority: 'high', owner: ME }, + { id: 't3', title: 'three', status: 'open', priority: 'low', owner: SOMEONE_ELSE }, +]; + +async function boot(makeDriver: () => unknown, ps: PermissionSet): Promise { + const engine = new ObjectQL(); + engine.registerDriver(makeDriver() as never, true); + await engine.init(); + engine.registerApp({ + id: 'com.objectstack.qa.rls-check-multi-row-writes', + name: 'RLS check on multi-row writes', + version: '1.0.0', + type: 'plugin', + scope: 'system', + objects: OBJECTS, + } as never); + await engine.syncSchemas(); + engines.push(engine); + const services: Record = { + manifest: { register: vi.fn() }, + objectql: engine, + metadata: { + get: async (_type: string, name: string) => engine.getSchema(name) ?? null, + list: async () => [MEMBER_DEFAULT, ps], + }, + }; + const ctx = { + logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }, + registerService: vi.fn(), + 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 as never); + await plugin.start(ctx as never); + // The expected refusals log at WARN through the engine's own logger. + vi.spyOn((engine as unknown as { logger: { warn: () => void } }).logger, 'warn').mockImplementation(() => undefined); + await engine.insert('qa_ticket', SEED.map((r) => ({ ...r })) as never, { context: SYS_CTX } as never); + return engine; +} + +const DRIVERS: Array<[string, () => unknown]> = [ + ['driver-sql (better-sqlite3 :memory:)', + () => new SqlDriver({ client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true } as never)], + ['driver-sqlite-wasm (:memory:)', () => new SqliteWasmDriver({ filename: ':memory:' } as never)], +]; + +interface Outcome { ok: boolean; code?: string; status?: number; developerMessage?: string } + +const attempt = async (run: () => Promise): Promise => { + try { + await run(); + return { ok: true }; + } catch (e) { + const err = e as { code?: string; statusCode?: number; status?: number; developerMessage?: string }; + return { ok: false, code: err.code, status: err.statusCode ?? err.status, developerMessage: err.developerMessage }; + } +}; + +/** The 3.6 gate's refusal, on the ADR-0112 envelope — never a bare throw. */ +const expectCheckRefusal = (outcome: Outcome, verb: 'insert' | 'update') => { + expect(outcome.ok, `expected a refusal, got a completed ${verb}`).toBe(false); + expect(outcome.code, 'ADR-0112 error code').toBe('PERMISSION_DENIED'); + expect(outcome.status, 'ADR-0112 HTTP status').toBe(403); + expect(outcome.developerMessage, 'the developer half names the check gate and the verb') + .toContain(`the ${verb} would violate a row-level CHECK`); +}; + +/** Every row, id → the columns under test, read past every scope. */ +const stored = async (engine: ObjectQL) => { + const rows = (await engine.find('qa_ticket', { context: SYS_CTX } as never)) as Array>; + return Object.fromEntries( + rows.map((r) => [r.id, { title: r.title, status: r.status, owner: r.owner }]), + ) as Record; +}; + +for (const [driverName, makeDriver] of DRIVERS) { + describe(`[#19964] an ARRAY insert is judged row by row — ${driverName}`, () => { + const insertMany = (engine: ObjectQL, rows: Array>) => + engine.insert('qa_ticket', rows as never, { context: CALLER } as never); + const insertOne = (engine: ObjectQL, row: Record) => + engine.insert('qa_ticket', row as never, { context: CALLER } as never); + + it('the repro: `[admitted, refused]` is refused on the check envelope, and neither row is stored', async () => { + const engine = await boot(makeDriver, permissionSet([{ name: 'not_archived', operation: 'insert', check: NOT_ARCHIVED }])); + expectCheckRefusal( + await attempt(() => insertMany(engine, [ + { id: 'n1', title: 'n1', status: 'closed' }, + { id: 'n2', title: 'n2', status: 'archived' }, + ])), + 'insert', + ); + expect(Object.keys(await stored(engine)).sort()).toEqual(['t1', 't2', 't3']); + }); + + it('⭐ over-fix control: an array whose every row passes is admitted and stored', async () => { + const engine = await boot(makeDriver, permissionSet([{ name: 'not_archived', operation: 'all', check: NOT_ARCHIVED }])); + expect((await attempt(() => insertMany(engine, [ + { id: 'n1', title: 'n1', status: 'closed' }, + { id: 'n2', title: 'n2', status: 'open' }, + ]))).ok).toBe(true); + const after = await stored(engine); + expect([after.n1?.status, after.n2?.status]).toEqual(['closed', 'open']); + }); + + it('⭐ the single insert answers exactly as before', async () => { + const engine = await boot(makeDriver, permissionSet([{ name: 'not_archived', operation: 'insert', check: NOT_ARCHIVED }])); + expectCheckRefusal(await attempt(() => insertOne(engine, { id: 'n2', title: 'n2', status: 'archived' })), 'insert'); + expect((await attempt(() => insertOne(engine, { id: 'n1', title: 'n1', status: 'closed' }))).ok).toBe(true); + expect(Object.keys(await stored(engine)).sort()).toEqual(['n1', 't1', 't2', 't3']); + }); + + const REFUSE_EVERY_INSERT: Array<[string, Policy]> = [ + ['an unresolvable sole `using` on `insert`', { name: 'bad_using', operation: 'insert', using: UNRESOLVABLE }], + ['an unresolvable sole `using` on `all`', { name: 'bad_using', operation: 'all', using: UNRESOLVABLE }], + ['an unresolvable declared `check`', { name: 'bad_check', operation: 'insert', check: UNRESOLVABLE }], + ]; + for (const [label, policy] of REFUSE_EVERY_INSERT) { + it(`${label}: every single insert is refused, and so is the array insert`, async () => { + const engine = await boot(makeDriver, permissionSet([policy])); + expectCheckRefusal(await attempt(() => insertOne(engine, { id: 'n1', title: 'n1', status: 'open' })), 'insert'); + expectCheckRefusal( + await attempt(() => insertMany(engine, [ + { id: 'n1', title: 'n1', status: 'open' }, + { id: 'n2', title: 'n2', status: 'closed' }, + ])), + 'insert', + ); + expect(Object.keys(await stored(engine)).sort()).toEqual(['t1', 't2', 't3']); + }); + } + }); +} diff --git a/packages/plugins/plugin-security/src/rls-phantom-column-negation.test.ts b/packages/plugins/plugin-security/src/rls-phantom-column-negation.test.ts index e59deb8cb72..c342dd6fc84 100644 --- a/packages/plugins/plugin-security/src/rls-phantom-column-negation.test.ts +++ b/packages/plugins/plugin-security/src/rls-phantom-column-negation.test.ts @@ -319,10 +319,9 @@ for (const [driverName, makeDriver] of DRIVERS) { describe(`[#17042] WRITE face, end to end — ${driverName}`, () => { /** - * ⚠️ A SINGLE object, never an array. Step 3.6 is guarded by - * `!Array.isArray(opCtx.data)`, so a bulk payload skips the check gate - * entirely and every cell below would read "permitted" for a reason that - * has nothing to do with this card. + * A single-row insert, so each cell reads one row's verdict. (An array + * insert is judged row by row too; that shape is pinned in + * `rls-check-multi-row-writes.test.ts`.) */ const insert = (engine: ObjectQL, isPrivate: boolean) => engine.insert( diff --git a/packages/plugins/plugin-security/src/security-plugin.test.ts b/packages/plugins/plugin-security/src/security-plugin.test.ts index dd95311f13a..6ddb94ae711 100644 --- a/packages/plugins/plugin-security/src/security-plugin.test.ts +++ b/packages/plugins/plugin-security/src/security-plugin.test.ts @@ -30,13 +30,14 @@ import { BUILTIN_OPERATION_MESSAGES } from '@objectstack/spec/system'; * * So the executor honours the seam the way the engine does: the flag first (it * answers "did the seam run", never "did the write pass"), then the judgement. - * These doubles run no hooks, so the row that would be stored IS `opCtx.data`. + * These doubles run no hooks, so the rows an insert would store ARE + * `opCtx.data` — each element of an array insert ([#19964]). */ const runEngineWriteBody = async (opCtx: any): Promise => { const seam = opCtx?.postHookWriteImageCheck; if (!seam) return; seam.honoured = true; - await seam.evaluate([opCtx.data]); + await seam.evaluate(Array.isArray(opCtx.data) ? opCtx.data : [opCtx.data]); }; // --------------------------------------------------------------------------- diff --git a/packages/plugins/plugin-security/src/security-plugin.ts b/packages/plugins/plugin-security/src/security-plugin.ts index be9f98572d0..9463f4ecca9 100644 --- a/packages/plugins/plugin-security/src/security-plugin.ts +++ b/packages/plugins/plugin-security/src/security-plugin.ts @@ -2993,11 +2993,21 @@ export class SecurityPlugin implements Plugin { // checked field must arrive from the caller — is REFUSED, not deferred: // it institutionalises the contradiction (the caller sending the value the // hook exists to make un-sendable) and needs a permanent lint to keep it. + // + // ── [#19964] EVERY row an insert stores ─────────────────────────────── + // + // The check is a guarantee about each stored row, so an insert that + // stores several rows is judged once per row, and ONE failing row + // refuses the whole write. An ARRAY insert used to be excluded by a + // non-array guard here, so no judgement was installed and every row was + // stored unjudged. The seam already receives every live row of an array + // insert, so the guard now admits an array for `insert` (an `update` + // still takes one payload). if ( (opCtx.operation === 'insert' || opCtx.operation === 'update') && opCtx.data && typeof opCtx.data === 'object' && - !Array.isArray(opCtx.data) && + (opCtx.operation === 'insert' || !Array.isArray(opCtx.data)) && permissionSets.length > 0 && !!opCtx.context?.userId ) { @@ -3051,8 +3061,10 @@ export class SecurityPlugin implements Plugin { checkParts.every((f) => matchesFilterCondition(image as any, f as any)); if (opCtx.operation === 'insert') { - // [#16608] Install the judgement; the engine runs it on the row the - // `beforeInsert` chain produced. The compiled filter is captured + // [#16608] Install the judgement; the engine runs it on the rows the + // `beforeInsert` chain produced — [#19964] every row of an array + // insert, and the first that fails refuses the whole write. The + // compiled filter is captured // HERE — while the caller's permission sets, the delegator's, the // staged membership and this request's context are all resolved — // and only the IMAGE is deferred. Deferring the compilation too From 6d28dfe98dfa3fbd063b878dce9ae0d674cf7f65 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 24 Sep 2026 14:20:58 +0000 Subject: [PATCH 2/5] fix(plugin-security, objectql): judge every row a predicate update stores against the row-level check A row-level `check` (declared, or defaulted from `using`) guarantees that no stored row fails it. A predicate update (`multi: true`, no row address) was never judged: the write gate skipped it on the assumption that a `using`-scoped `where` governed it. A policy that declares only `check` scopes nothing, and a scoped `where` says nothing about the new row, so the guarantee did not hold on that path for any policy. The rows such an update changes are the ones the middleware-composed query selects, which is complete only once every middleware has run. So the security layer installs its judgement on the existing `OperationContext.postHookWriteImageCheck` seam, and the engine runs it on the predicate path once the payload is final. It hands the seam every matched row merged with that payload, read by the one matched-row read the path already makes. One failing row refuses the whole update with the existing PERMISSION_DENIED / 403 refusal. A seam the engine never runs fails closed, as it does for an insert. A falsy payload id, which the engine does not treat as a row address, is judged both ways. The by-id update and the single-row insert are unchanged. The pending release note that said bulk updates are not checked row by row is corrected in place. Claude-Session: https://claude.ai/code/session_01Evb5jFDZGKQE9KG4jbMfMF Co-authored-by: Claude --- .../19950-rls-check-multi-row-writes.md | 26 +++ .changeset/rls-check-defaults-to-using.md | 2 +- packages/objectql/src/engine.ts | 73 +++++++- .../src/check-only-write-scope.test.ts | 29 ++- .../src/rls-check-multi-row-writes.test.ts | 167 ++++++++++++++++-- .../src/security-plugin.test.ts | 9 + .../plugin-security/src/security-plugin.ts | 145 +++++++++------ 7 files changed, 367 insertions(+), 84 deletions(-) create mode 100644 .changeset/19950-rls-check-multi-row-writes.md diff --git a/.changeset/19950-rls-check-multi-row-writes.md b/.changeset/19950-rls-check-multi-row-writes.md new file mode 100644 index 00000000000..ccf08057498 --- /dev/null +++ b/.changeset/19950-rls-check-multi-row-writes.md @@ -0,0 +1,26 @@ +--- +"@objectstack/plugin-security": patch +"@objectstack/objectql": patch +--- + +fix(plugin-security): a row-level `check` now holds for every row a multi-row write stores — an array insert and a predicate (`multi: true`) update (#19950, #19964) + +A row-level security `check` (declared on the policy, or defaulted from its `using`) is the write-side half of the policy: a row the check refuses is never stored. The write gate enforced it for a single-row insert and for a by-id update. It did not enforce it for the two multi-row write shapes: + +- **An array insert** (`insert(object, [rows])`, which the create-many data route calls). No check was installed for an array payload, so the rows were stored unjudged. +- **A predicate update** (`update(object, changes, { where, multi: true })`). The new rows were never judged. The gate assumed a `using`-scoped `where` covered the write, but a policy that declares only `check` scopes nothing, and a scoped `where` says nothing about the new row in any case. + +Both shapes are now judged row by row, with the same refusal the single-row shapes give: `403 PERMISSION_DENIED`, and nothing is stored. One failing row refuses the whole write. + +- **Array insert.** Every row is judged on the image the `beforeInsert` chain produced, as a single insert is. +- **Predicate update.** Every row the write selects is judged on its new image: the matched row merged with the final payload. The engine supplies the rows (`@objectstack/objectql`): the security layer installs its judgement on `OperationContext.postHookWriteImageCheck`, the seam the insert check already uses, and the engine runs it on the predicate path over the rows its composed query selects, once the payload is final. That is the one matched-row read the path already makes for validation and per-row hooks, not a second one. + +**Writes that are now refused.** A multi-row write is refused where the same row written alone would be: + +- a predicate update under a policy that declares only `check`, when any matched row's new image fails that check; +- a predicate update that moves a matched row out of a policy's `using` when no applicable policy declares `check`. The `using` is the defaulted check, and this is the answer the by-id update already gives; +- an array insert when any row fails the check, including every configuration that already refused each single insert (a `using` or `check` that does not compile). + +**What does not change.** A by-id update and a single-row insert are judged exactly as before. A predicate update still touches only the rows its scoped `where` selects; the check refuses a write, it never widens or narrows which rows are selected. A predicate update whose new rows all pass is admitted as before. A system-context write is not gated. + +**Hosts that run the security plugin with their own write executor.** A predicate update that installs the judgement and returns without the engine having run it is now refused, as an insert already is: `403`, and an `error` log saying the check was not evaluated. diff --git a/.changeset/rls-check-defaults-to-using.md b/.changeset/rls-check-defaults-to-using.md index 4866e1ef947..6069f15eceb 100644 --- a/.changeset/rls-check-defaults-to-using.md +++ b/.changeset/rls-check-defaults-to-using.md @@ -24,5 +24,5 @@ The published contract has always said this. `RowLevelSecurityPolicySchema.check - 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. - 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. - `select` policies never gate a write's new row. -- Bulk updates without a single id are still scoped by the `using` where clause and are not checked row by row. +- Bulk updates without a single id are still scoped by the `using` where clause. Their new rows are now checked row by row as well, by the separate multi-row entry (#19950). - The `modifyAllRecords` bypass on private and platform-global objects still skips the check. diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index c9b1cfc19e8..fa09ca4901e 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -2224,8 +2224,16 @@ export interface OperationContext { * against `opCtx.data`, and {@link ObjectQL.insert} calls it once the * `beforeInsert` chain has produced the row — before the first producer with * a side effect (the secret channel, the autonumber, the statement), so a - * refusal still costs nothing. `update` needs no seam: that path already - * merges its pre-image with the change set, which is the same proposition. + * refusal still costs nothing. An ARRAY insert is one operation: every live + * row is judged in the same call. + * + * [#19950] A PREDICATE `update` (`multi: true`, no row address) uses the same + * seam. The middleware cannot know which rows the write will change: they + * are the rows the COMPOSED AST selects, and that AST is complete only after + * every middleware has run. So {@link ObjectQL.update} calls it on that path, + * once the payload is final, with every matched row merged with the payload. + * A by-id `update` is never handed to the seam: the middleware judges that + * one row itself, by merging its pre-image with the change set. * * ABSENT is the ordinary state — no enforcement layer is mounted, or the * write is one it does not gate. The engine never invents one. @@ -2237,11 +2245,12 @@ export interface OperationContext { * [#16608] The judgement {@link OperationContext.postHookWriteImageCheck} * carries, and the acknowledgement its installer reads back. * - * `evaluate` receives the rows exactly as the `beforeInsert` chain left them — - * the images the driver is about to be handed — and REFUSES by throwing. It is - * called at most once per operation, and only for rows still live (a row the - * declared-field door culled from a partial batch is never judged: it will not - * be written). + * `evaluate` receives the images the driver is about to store, and REFUSES by + * throwing. On an `insert` those are the rows exactly as the `beforeInsert` + * chain left them, only the live ones (a row the declared-field door culled + * from a partial batch is never judged: it will not be written). On a + * predicate `update` they are the matched rows, each merged with the final + * payload. It is called at most once per operation. * * `honoured` is set by the engine immediately before `evaluate` runs. It exists * so the installer can fail CLOSED on a seam that was never called: an @@ -13222,6 +13231,56 @@ export class ObjectQL implements IObjectQLEngine { // caller is told before N rows are written with a column missing // — the failure mode a bulk write makes N times larger. assertNoStrictDrops(); + // ── [#19950] The post-image seam on the PREDICATE path ───────── + // + // An enforcement layer's write `check` must hold for EVERY row a + // write stores (ADR-0058 D4: "and on the AST-injected bulk + // path"). For a by-id update the enforcement middleware can judge + // the new row itself: it knows the one row and reads it. For a + // predicate update it cannot: the rows are the ones the + // middleware-COMPOSED AST selects, and that AST is complete only + // once every middleware has run (the enforcement layer's own + // scope, a sharing layer's editable-rows filter, the tenant + // wall). So the layer installs its judgement on + // `opCtx.postHookWriteImageCheck`, as it does for an insert, and + // the engine hands it the rows here. + // + // Each image is one matched row merged with the payload, the + // row `updateMany` is about to produce, and it is the same shape + // the per-row `afterUpdate` context calls `result` + // (`buildPerRowAfterContexts`). The rows come from the D7 read, + // the one `readPriorRows` memo that validation, the + // `readonlyWhen` strip and both hook phases share, bound to the + // same composed AST the statement binds. That is the one read the + // ruling allows, never a second fetch. + // + // Placement: the payload is FINAL here. The per-row + // `beforeUpdate` chain, the hand-back, both readonly strips and + // the strict-drop refusal have all run, and nothing below + // changes a value before the statement. The seam judges the rows + // that will be stored, which is the rule the insert seam was + // held to. The credential channel (`encryptSecretFields`) runs + // above on this branch, so a refused write that carried a secret + // field has already minted its `sys_secret` row. A validation + // refusal two lines down already pays the same cost here, and + // moving that channel is a separate change. A `check` naming a + // secret field judges the stored reference. + // + // `honoured` is set BEFORE `evaluate`, exactly as on the insert + // path: it answers "did the seam run", never "did the write + // pass". Zero matched rows is an empty judgement, not a skipped + // one. + const predicateImageCheck = opCtx.postHookWriteImageCheck; + if (predicateImageCheck) { + predicateImageCheck.honoured = true; + const matchedRows = (await readPriorRows()) ?? []; + const payload = hookContext.input.data as Record; + await predicateImageCheck.evaluate( + matchedRows.map( + (row) => coerceBooleanFields(updateSchema as any, { ...row, ...payload } as any) as Record, + ), + ); + } // [#3106] Same enforcement the single-id branch runs at its // `evaluateValidationRules` call, applied per matched row: any // error-severity violation rejects the WHOLE batch before diff --git a/packages/plugins/plugin-security/src/check-only-write-scope.test.ts b/packages/plugins/plugin-security/src/check-only-write-scope.test.ts index f30284f5a9a..459a1c3cbcd 100644 --- a/packages/plugins/plugin-security/src/check-only-write-scope.test.ts +++ b/packages/plugins/plugin-security/src/check-only-write-scope.test.ts @@ -289,12 +289,23 @@ function makeEngine() { }, // Both write verbs open with the PRODUCER's own dispatch predicate // (`assertEngine*Dispatch`), never a hand-mirrored guard. - async update(object: string, data: any, options?: any) { + // + // [#19950] `opCtx` is the operation the middleware chain ran on, as the + // real engine holds it. On the PREDICATE path the engine hands an + // installed write-image check every matched row merged with the payload + // before it writes, so this double does the same: a double that skipped + // it would be refused, fail-closed, by the security middleware. + async update(object: string, data: any, options?: any, opCtx?: 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)); + const seam = dispatch.kind === 'by-id' ? undefined : opCtx?.postHookWriteImageCheck; + if (seam) { + seam.honoured = true; + await seam.evaluate(targets.map((r) => ({ ...r, ...data }))); + } for (const r of targets) Object.assign(r, data); return dispatch.kind === 'by-id' ? (targets[0] ?? null) : targets.length; }, @@ -379,7 +390,7 @@ async function makeStack(opts: { orgScoping?: boolean } = {}): Promise { 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); + else await engine.update(opCtx.object, opCtx.data, opCtx.options, opCtx); reached = true; }); }); @@ -594,11 +605,13 @@ describe('[#8059 site 1] Layer 1 actually DERIVES for a check-only policy — th let stack: Stack; beforeEach(async () => { stack = await makeStack(); }); - it('the bulk UPDATE path touches only READABLE rows — a path step 3.6 explicitly declines to check', async () => { - // Step 3.6 logs "not post-image validated" and skips for a write with no - // single id, so belt 2 contributes NOTHING here by its own construction. + it('the bulk UPDATE path touches only READABLE rows — a path step 3.6 can refuse but never scope', async () => { + // Since #19950 step 3.6 judges every row a bulk update stores, but a check + // can only REFUSE a write; it cannot choose which rows the write touches. // The only thing that can scope this write is Layer 1 injected into the - // AST — i.e. the derivation. On a site-1 revert both rows are rewritten. + // AST — i.e. the derivation. On a site-1 revert the match set grows to the + // other contributor's row, whose new image fails the owner check, so the + // whole write is refused and the caller's own row is not rewritten either. const opCtx: any = { object: 'qa_invoice', operation: 'update', @@ -609,7 +622,7 @@ describe('[#8059 site 1] Layer 1 actually DERIVES for a check-only policy — th }; 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 }); + await stack.engine.update(opCtx.object, opCtx.data, { ...opCtx.options, where: opCtx.ast.where, multi: true }, opCtx); }); expect(stack.rows('qa_invoice').find((r) => r.id === INVOICE_C2.id)?.subject).toBe('bulk-edit'); expect( @@ -632,7 +645,7 @@ describe('[#8059 site 1] Layer 1 actually DERIVES for a check-only policy — th }; 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 }); + await stack.engine.update(opCtx.object, opCtx.data, { ...opCtx.options, where: opCtx.ast.where, multi: true }, opCtx); }); expect(stack.rows('qa_invoice').find((r) => r.id === INVOICE_C2.id)?.subject).toBe('own-bulk-edit'); }); diff --git a/packages/plugins/plugin-security/src/rls-check-multi-row-writes.test.ts b/packages/plugins/plugin-security/src/rls-check-multi-row-writes.test.ts index a71a94644e1..389b4bd2f68 100644 --- a/packages/plugins/plugin-security/src/rls-check-multi-row-writes.test.ts +++ b/packages/plugins/plugin-security/src/rls-check-multi-row-writes.test.ts @@ -1,25 +1,45 @@ // Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. /** - * [ADR-0058 D4] A row-level `check` judges EVERY row a write stores — on an - * ARRAY insert as on a single-row one. + * [ADR-0058 D4] A row-level `check` judges EVERY row a write stores — on the + * multi-row write shapes as on the single-row ones. * * ## The guarantee * * `RowLevelSecurityPolicySchema.check` (declared, or defaulted from `using`) is * the write-side half of a row-level policy: a row the check refuses is never - * stored. An ARRAY insert (`insert(object, [rows])`, which `createManyData` - * calls) escaped it: step 3.6 of the security middleware excluded an array - * payload, so no judgement was installed at all. + * stored. ADR-0058 D4 publishes it "on the write pre-image path that already + * exists for by-id writes … and on the AST-injected bulk path". PostgreSQL's + * `WITH CHECK` holds the same line: every new row version, bulk or not. + * + * Two multi-row shapes escaped it, both at step 3.6 of the security middleware, + * which assumed one row per write: + * + * - a bulk UPDATE (`update(object, changes, { where, multi: true })`) took the + * "no single id" branch, which skipped the post-image check on the + * assumption that a `using`-scoped `where` governed it. Nothing scopes the + * `where` for a policy that declares only `check`, and nothing ever judged the + * NEW row for any policy; + * - an ARRAY insert (`insert(object, [rows])`, which `createManyData` calls) + * was excluded by a non-array guard, so no judgement was installed at all. * * ## What this file pins, on both SQL driver families * - * - the repro, failing first: `[admitted, refused]` is refused on the ADR-0112 - * envelope (`PERMISSION_DENIED` / 403) and NOTHING is stored; - * - the over-fix controls: an array whose every row passes is admitted and - * stored; the single insert answers exactly as before; + * - each repro, failing first: the multi-row write is refused on the ADR-0112 + * envelope (`PERMISSION_DENIED` / 403) and NOTHING is stored or moved; + * - per ROW, not per change set: a bulk update is judged on each matched row + * merged with the change set, so a row whose UNCHANGED field fails the check + * refuses the whole write — a change-set-only judgement would admit it; + * - the over-fix controls: a multi-row write whose every row passes is admitted + * and stored; a `using`-scoped bulk update still touches only its rows; the + * by-id update and the single insert answer exactly as before; * - the configurations that refuse EVERY single insert refuse the array insert - * too (an unresolvable sole `using`, an unresolvable declared `check`). + * too (an unresolvable sole `using`, an unresolvable declared `check`); + * - fail-closed: a bulk update is judged by the engine, over the rows the + * composed AST selects, so a host that never runs the installed judgement + * is refused rather than vouched for; and a falsy payload `id`, which the + * engine does not read as a row address, cannot route a bulk update around + * it. * * Ground truth is read under a system context: "the gate refused" and "nothing * was stored" are separate facts, and both are asserted. @@ -52,8 +72,19 @@ const OBJECTS = [ const ME = 'a@e.example'; const SOMEONE_ELSE = 'b@e.example'; +/** The #19950 repro's predicate. */ +const NOT_CLOSED = "record.status != 'closed'"; /** The #19964 repro's predicate. */ const NOT_ARCHIVED = "record.status != 'archived'"; +/** + * A predicate over a field the bulk change set does NOT carry: a high-priority + * ticket may not be closed. Judged on the change set alone (`{ status }`), the + * missing `priority` makes the right arm vacuously true and the write passes; + * judged on each matched row merged with the change set, the high-priority row + * fails. That difference is what makes the per-row cell below a pin. + */ +const HIGH_STAYS_OPEN = "record.status != 'closed' || record.priority != 'high'"; +const OWN_ROWS = 'record.owner == current_user.email'; /** * A `current_user.*` key no resolver publishes: the compiler cannot resolve it, * drops the policy, and the gate falls back to the deny sentinel — so every @@ -160,6 +191,9 @@ const expectCheckRefusal = (outcome: Outcome, verb: 'insert' | 'update') => { .toContain(`the ${verb} would violate a row-level CHECK`); }; +const bulkUpdate = (engine: ObjectQL, changes: Record, where: Record) => + engine.update('qa_ticket', changes as never, { where, multi: true, context: CALLER } as never); + /** Every row, id → the columns under test, read past every scope. */ const stored = async (engine: ObjectQL) => { const rows = (await engine.find('qa_ticket', { context: SYS_CTX } as never)) as Array>; @@ -168,7 +202,120 @@ const stored = async (engine: ObjectQL) => { ) as Record; }; +const SEEDED = Object.fromEntries(SEED.map((r) => [r.id, { title: r.title, status: r.status, owner: r.owner }])); + for (const [driverName, makeDriver] of DRIVERS) { + describe(`[#19950] a bulk UPDATE is judged row by row — ${driverName}`, () => { + it('the repro: a check-only policy refuses a `multi` update that stores a row its check refuses, and nothing moves', async () => { + const engine = await boot(makeDriver, permissionSet([{ name: 'not_closed', operation: 'update', check: NOT_CLOSED }])); + expectCheckRefusal(await attempt(() => bulkUpdate(engine, { status: 'closed' }, { status: 'open' })), 'update'); + expect(await stored(engine)).toEqual(SEEDED); + }); + + it('per row, not per change set: one matched row failing on an UNCHANGED field refuses the whole write', async () => { + const engine = await boot(makeDriver, permissionSet([{ name: 'high_stays_open', operation: 'update', check: HIGH_STAYS_OPEN }])); + // t1 (low) would pass; t2 (high) fails on the merged image only. + expectCheckRefusal(await attempt(() => bulkUpdate(engine, { status: 'closed' }, { owner: ME })), 'update'); + expect(await stored(engine)).toEqual(SEEDED); + }); + + it('⭐ over-fix control: a `multi` update whose every new row passes is admitted, and exactly its rows move', async () => { + const engine = await boot(makeDriver, permissionSet([{ name: 'high_stays_open', operation: 'update', check: HIGH_STAYS_OPEN }])); + expect((await attempt(() => bulkUpdate(engine, { status: 'closed' }, { priority: 'low' }))).ok).toBe(true); + const after = await stored(engine); + expect(after.t1.status).toBe('closed'); + expect(after.t2.status).toBe('open'); + expect(after.t3.status).toBe('closed'); + }); + + it('a declared `check` that differs from `using` is judged on the bulk path too', async () => { + const engine = await boot(makeDriver, permissionSet([ + { name: 'own_not_closed', operation: 'update', using: OWN_ROWS, check: NOT_CLOSED }, + ])); + expectCheckRefusal(await attempt(() => bulkUpdate(engine, { status: 'closed' }, {})), 'update'); + expect(await stored(engine)).toEqual(SEEDED); + }); + + it('fail-closed: a host that never runs the installed check does not have its bulk update vouched for', async () => { + const engine = await boot(makeDriver, permissionSet([{ name: 'not_closed', operation: 'update', check: NOT_CLOSED }])); + // A middleware INSIDE the security one takes the installed judgement off + // the operation before the engine sees it — the shape of a host that + // executes the write without the seam. The payload would PASS the check, + // so a refusal here is about the check not running, not about values. + let seen: { honoured?: boolean } | undefined; + engine.registerMiddleware(async (opCtx: any, next: () => Promise) => { + if (opCtx.operation === 'update' && opCtx.postHookWriteImageCheck) { + seen = opCtx.postHookWriteImageCheck; + delete opCtx.postHookWriteImageCheck; + } + await next(); + }); + const outcome = await attempt(() => bulkUpdate(engine, { title: 'renamed' }, { status: 'open' })); + expect(outcome.ok, 'an unjudged bulk write must not be reported as an allowed one').toBe(false); + expect(outcome.code).toBe('PERMISSION_DENIED'); + expect(outcome.status).toBe(403); + expect(outcome.developerMessage).toContain( + "the update on 'qa_ticket' was executed without the row-level CHECK being evaluated", + ); + expect(seen, 'the judgement was installed for the predicate update').toBeTruthy(); + expect(seen?.honoured).not.toBe(true); + }); + + it('a falsy payload `id` does not route a bulk update around the per-row judgement', async () => { + const engine = await boot(makeDriver, permissionSet([{ name: 'high_stays_open', operation: 'update', check: HIGH_STAYS_OPEN }])); + const outcome = await attempt(() => bulkUpdate(engine, { id: '', status: 'closed' }, { owner: ME })); + expect(outcome.ok, 'a falsy id is not a row address; the write is a bulk update and is judged per row').toBe(false); + expect(outcome.code).toBe('PERMISSION_DENIED'); + expect(outcome.status).toBe(403); + expect(await stored(engine)).toEqual(SEEDED); + }); + }); + + describe(`[#19950] controls — a using-declared policy and the by-id path — ${driverName}`, () => { + it('⭐ a USING-only policy: an in-scope bulk update is admitted and still touches only the rows its `using` selects', async () => { + const engine = await boot(makeDriver, permissionSet([{ name: 'own_rows', operation: 'update', using: OWN_ROWS }])); + expect((await attempt(() => bulkUpdate(engine, { title: 'renamed' }, {}))).ok).toBe(true); + const after = await stored(engine); + expect(after.t1.title).toBe('renamed'); + expect(after.t2.title).toBe('renamed'); + expect(after.t3.title, 'a row outside the using is not touched').toBe('three'); + }); + + it('⭐ a declared `check` with a `using`: an in-scope bulk update whose new rows pass is admitted', async () => { + const engine = await boot(makeDriver, permissionSet([ + { name: 'own_not_closed', operation: 'update', using: OWN_ROWS, check: NOT_CLOSED }, + ])); + expect((await attempt(() => bulkUpdate(engine, { title: 'renamed' }, {}))).ok).toBe(true); + const after = await stored(engine); + expect([after.t1.title, after.t2.title, after.t3.title]).toEqual(['renamed', 'renamed', 'three']); + }); + + it('a USING-only policy: moving rows OUT of the `using` is refused — the defaulted check, the answer the by-id path gives', async () => { + const engine = await boot(makeDriver, permissionSet([{ name: 'own_rows', operation: 'update', using: OWN_ROWS }])); + expectCheckRefusal(await attempt(() => bulkUpdate(engine, { owner: SOMEONE_ELSE }, {})), 'update'); + expect(await stored(engine)).toEqual(SEEDED); + // The by-id twin of the same write, unchanged: refused on the same envelope. + expectCheckRefusal( + await attempt(() => engine.update('qa_ticket', { id: 't1', owner: SOMEONE_ELSE } as never, { context: CALLER } as never)), + 'update', + ); + expect(await stored(engine)).toEqual(SEEDED); + }); + + it('⭐ the by-id update under a check-only policy answers exactly as before: refused out of check, admitted in it', async () => { + const engine = await boot(makeDriver, permissionSet([{ name: 'not_closed', operation: 'update', check: NOT_CLOSED }])); + expectCheckRefusal( + await attempt(() => engine.update('qa_ticket', { id: 't1', status: 'closed' } as never, { context: CALLER } as never)), + 'update', + ); + expect((await stored(engine)).t1.status).toBe('open'); + expect( + (await attempt(() => engine.update('qa_ticket', { id: 't1', title: 'renamed' } as never, { context: CALLER } as never))).ok, + ).toBe(true); + expect((await stored(engine)).t1.title).toBe('renamed'); + }); + }); + describe(`[#19964] an ARRAY insert is judged row by row — ${driverName}`, () => { const insertMany = (engine: ObjectQL, rows: Array>) => engine.insert('qa_ticket', rows as never, { context: CALLER } as never); diff --git a/packages/plugins/plugin-security/src/security-plugin.test.ts b/packages/plugins/plugin-security/src/security-plugin.test.ts index 6ddb94ae711..039fc231ca0 100644 --- a/packages/plugins/plugin-security/src/security-plugin.test.ts +++ b/packages/plugins/plugin-security/src/security-plugin.test.ts @@ -32,11 +32,20 @@ import { BUILTIN_OPERATION_MESSAGES } from '@objectstack/spec/system'; * answers "did the seam run", never "did the write pass"), then the judgement. * These doubles run no hooks, so the rows an insert would store ARE * `opCtx.data` — each element of an array insert ([#19964]). + * + * [#19950] A predicate (`multi`) update installs the same seam, and the engine + * hands it every row the composed AST matches, merged with the payload. These + * doubles hold no table, so that match set is empty: the judgement runs over + * no rows, exactly as a real engine's would over an empty one. */ const runEngineWriteBody = async (opCtx: any): Promise => { const seam = opCtx?.postHookWriteImageCheck; if (!seam) return; seam.honoured = true; + if (opCtx.operation === 'update') { + await seam.evaluate([]); + return; + } await seam.evaluate(Array.isArray(opCtx.data) ? opCtx.data : [opCtx.data]); }; diff --git a/packages/plugins/plugin-security/src/security-plugin.ts b/packages/plugins/plugin-security/src/security-plugin.ts index 9463f4ecca9..6a8f65f0249 100644 --- a/packages/plugins/plugin-security/src/security-plugin.ts +++ b/packages/plugins/plugin-security/src/security-plugin.ts @@ -204,9 +204,11 @@ export function hasPlatformAdminCapability(held: ReadonlySet): boolean { } /** - * [#16608] The insert-side write `check`, installed on the operation context - * for the engine to run once the `beforeInsert` chain has produced the row that - * will be stored. + * [#16608] The write `check` for the writes this middleware cannot judge on + * its own, installed on the operation context for the engine to run on the + * rows that will be stored: every row of an insert, single or array, once the + * `beforeInsert` chain has produced it, and [#19950] every row a predicate + * (`multi`) update selects, merged with the final payload. * * Structurally identical to `OperationContext.postHookWriteImageCheck` in * `@objectstack/objectql`, and deliberately declared here rather than imported: @@ -216,8 +218,8 @@ export function hasPlatformAdminCapability(held: ReadonlySet): boolean { * `.d.ts`. The two spellings are welded by a test that runs BOTH packages, not * by the type system — see `insert-check-post-image.test.ts`. */ -interface InsertCheckSeam { - /** Refuses by throwing. Receives the rows as `beforeInsert` left them. */ +interface WriteImageCheckSeam { + /** Refuses by throwing. Receives the images the driver is about to store. */ evaluate(rows: readonly Record[]): void; /** Set by the engine immediately before `evaluate` runs. */ honoured?: boolean; @@ -1812,13 +1814,14 @@ export class SecurityPlugin implements Plugin { // Register security middleware ql.registerMiddleware(async (opCtx: any, next: () => Promise) => { - // [#16608] The insert-side write `check`, once step 3.6 has installed it - // on the operation context for the engine to run after `beforeInsert`. + // [#16608] The write `check` step 3.6 installs on the operation context + // for the engine to run: on an insert, after `beforeInsert`; [#19950] on + // a predicate update, over every matched row once the payload is final. // Held here so the post-`next()` assertion below can read whether the // seam was honoured — an installed judgement that never ran is a write // this middleware did not gate, and it fails CLOSED and loudly rather // than passing for an allowed one. - let insertCheckSeam: InsertCheckSeam | null = null; + let writeImageCheckSeam: WriteImageCheckSeam | null = null; // [#10757] Retire every memoized permission-set resolution the moment a // WRITE passes through the engine. Deliberately the FIRST statement in // the middleware — ahead of the `isSystem` bypass immediately below — @@ -2049,8 +2052,11 @@ export class SecurityPlugin implements Plugin { // platform ownership floor for THIS write. The post-image check (step // 3.6) reads it so that a USING-only policy set is held, on the new row, // to the same write-class policies the pre-image admitted the caller by. - // Null until step 2.7 runs; step 3.6 judges an update post-image only on - // a path where it did (same single id, same guard triple). + // Null until step 2.7 runs. A by-id update post-image is judged on a path + // where it did (same single id, same guard triple). A predicate update + // never reaches 2.7, so its per-row check (step 3.6, [#19950]) keeps the + // floor on exactly the terms its `where` scope does: step 3 composes the + // bulk scope with no floor options either. let preImageFloorOpts: RlsFilterOptions | null = null; if (permissionSets.length > 0 && opCtx.context?.onBehalfOf?.userId) { const del = await resolveDelegatorContext(this.ql, opCtx.context); @@ -2994,15 +3000,24 @@ export class SecurityPlugin implements Plugin { // it institutionalises the contradiction (the caller sending the value the // hook exists to make un-sendable) and needs a permanent lint to keep it. // - // ── [#19964] EVERY row an insert stores ─────────────────────────────── + // ── [#19950 / #19964] EVERY row a write stores ──────────────────────── // - // The check is a guarantee about each stored row, so an insert that - // stores several rows is judged once per row, and ONE failing row - // refuses the whole write. An ARRAY insert used to be excluded by a - // non-array guard here, so no judgement was installed and every row was - // stored unjudged. The seam already receives every live row of an array - // insert, so the guard now admits an array for `insert` (an `update` - // still takes one payload). + // The check is a guarantee about each stored row, so a write that stores + // several rows is judged once per row, and ONE failing row refuses the + // whole write. Two multi-row shapes used to escape it here: + // + // • an ARRAY insert was excluded by a non-array guard, so no judgement + // was installed and every row was stored unjudged. The seam already + // receives every live row of an array insert, so the guard now admits + // an array for `insert` (an `update` still takes one payload); + // • a PREDICATE update (no row address) was skipped with a log line, + // on the assumption that a `using`-scoped `where` governed it. That + // assumption fails twice: a policy that declares only `check` scopes + // nothing, and a `where` scoped by `using` still says nothing about + // the NEW row. The rows it changes are the ones the COMPOSED AST + // selects, known only once every middleware has run, so the same + // seam is installed and the engine runs it over each matched row + // merged with the final payload. if ( (opCtx.operation === 'insert' || opCtx.operation === 'update') && opCtx.data && @@ -3026,8 +3041,9 @@ export class SecurityPlugin implements Plugin { : null; const checkParts = [checkFilter, delCheckFilter].filter(Boolean) as Record[]; if (checkParts.length > 0) { - // The ONE refusal, shared by both verbs — so an insert judged inside - // the engine and an update judged here answer a caller identically. + // The ONE refusal, shared by every shape — so a write judged inside + // the engine (an insert, a predicate update) and a by-id update + // judged here answer a caller identically. const denyCheck = (): never => { this.logger.warn?.( `[Security] RLS check FAILED on ${opCtx.operation} '${opCtx.object}' — write denied (fail-closed)`, @@ -3059,47 +3075,60 @@ export class SecurityPlugin implements Plugin { }; const satisfiesCheck = (image: Record): boolean => checkParts.every((f) => matchesFilterCondition(image as any, f as any)); + // [#16608] The judgement the engine runs: every image it hands over + // must pass, and the first that fails refuses the whole write. The + // compiled filter is captured HERE — while the caller's permission + // sets, the delegator's, the staged membership and this request's + // context are all resolved — and only the IMAGES are deferred. + // Deferring the compilation too would move authorization inputs into + // the engine's timeline for no gain. + const newWriteImageCheck = (): WriteImageCheckSeam => ({ + evaluate: (rows) => { + for (const row of rows) { + if (!row || typeof row !== 'object') continue; + if (!satisfiesCheck(row)) denyCheck(); + } + }, + }); if (opCtx.operation === 'insert') { - // [#16608] Install the judgement; the engine runs it on the rows the - // `beforeInsert` chain produced — [#19964] every row of an array - // insert, and the first that fails refuses the whole write. The - // compiled filter is captured - // HERE — while the caller's permission sets, the delegator's, the - // staged membership and this request's context are all resolved — - // and only the IMAGE is deferred. Deferring the compilation too - // would move authorization inputs into the engine's timeline for no - // gain. - insertCheckSeam = { - evaluate: (rows) => { - for (const row of rows) { - if (!row || typeof row !== 'object') continue; - if (!satisfiesCheck(row)) denyCheck(); - } - }, - }; - opCtx.postHookWriteImageCheck = insertCheckSeam; + // [#16608] The engine runs it on the rows the `beforeInsert` chain + // produced — [#19964] every row of an array insert. + writeImageCheckSeam = newWriteImageCheck(); + opCtx.postHookWriteImageCheck = writeImageCheckSeam; } else { - // UPDATE — unchanged. Build the post-image: the caller's pre-image - // merged with the change set (so a check on an unchanged field - // still sees its value). A bulk update (no single id) cannot form a - // post-image here — it is governed by the using-based AST scoping - // (step 3); we log and skip rather than guess. - let postImage: Record | null = { ...(opCtx.data as Record) }; const targetId = this.extractSingleId(opCtx); - if (targetId == null) { - this.logger.warn?.( - `[Security] RLS check on bulk update '${opCtx.object}' is not post-image validated ` + - `(governed by the using-scoped where); single-id writes are checked.`, - ); - postImage = null; - } else if (this.ql) { - // Shares the memoized caller pre-image with the step-3.5 owner - // echo check — the identical (object, id, caller-context) row. - const pre = await this.getCallerPreImage(opCtx, targetId); - if (pre) postImage = { ...pre, ...(opCtx.data as Record) }; + if (targetId != null) { + // BY-ID UPDATE — unchanged. Build the post-image: the caller's + // pre-image merged with the change set (so a check on an + // unchanged field still sees its value). + let postImage: Record = { ...(opCtx.data as Record) }; + if (this.ql) { + // Shares the memoized caller pre-image with the step-3.5 owner + // echo check — the identical (object, id, caller-context) row. + const pre = await this.getCallerPreImage(opCtx, targetId); + if (pre) postImage = { ...pre, ...(opCtx.data as Record) }; + } + if (!satisfiesCheck(postImage)) denyCheck(); + } + // [#19950] PREDICATE UPDATE — the engine runs the judgement over + // every row the composed AST selects, each merged with the final + // payload (see the block note above). Installed whenever the ENGINE + // will not treat the write as addressing one row, which is wider + // than `targetId == null`: the engine reads a FALSY scalar id + // (`''`, `0`) as no row address at all + // (`resolveEngineUpdateDispatch`) and routes the write to its + // predicate path, so a falsy id is judged BOTH ways — its by-id + // image above, exactly as before, and every matched row here. + // Neither judgement alone would do: without the seam a falsy id + // would carry a bulk update past the per-row check on the strength + // of a change-set-only image. If no image can be formed the write + // is not admitted: the engine refuses a predicate update it cannot + // route, and a seam it never runs fails CLOSED after `next()`. + if (!targetId) { + writeImageCheckSeam = newWriteImageCheck(); + opCtx.postHookWriteImageCheck = writeImageCheckSeam; } - if (postImage && !satisfiesCheck(postImage)) denyCheck(); } } } @@ -3479,9 +3508,9 @@ export class SecurityPlugin implements Plugin { // that silently stops gating and a deployment that never finds out. // ⛔ Do not soften this into a warning: a middleware that cannot say a // write was checked must not report that it was. - if (insertCheckSeam && insertCheckSeam.honoured !== true) { + if (writeImageCheckSeam && writeImageCheckSeam.honoured !== true) { const developerMessage = - `[Security] Access denied: the insert on '${opCtx.object}' was executed without the row-level CHECK ` + + `[Security] Access denied: the ${opCtx.operation} on '${opCtx.object}' was executed without the row-level CHECK ` + `being evaluated — the engine did not run OperationContext.postHookWriteImageCheck. ` + `The write is NOT vouched for by this gate.`; // Contract arg order (#5637): `error(message, error?: Error, meta?)` — From 9fff66cfdc5861209b8c418f8bdbcacddf896899 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 24 Sep 2026 14:51:02 +0000 Subject: [PATCH 3/5] chore(changeset): declare the multi-row check enforcement a narrowing MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The change refuses bulk updates and array inserts the write gate used to admit, so its release note is re-declared the way the using-defaulted check's was: `minor` for both packages, a `!` headline, the `Clause-②: no (narrowing)` line, an ADR-0087 not-required disposition, and a BREAKING paragraph listing the newly refused writes and the remedy. Claude-Session: https://claude.ai/code/session_01Evb5jFDZGKQE9KG4jbMfMF Co-authored-by: Claude --- .../19950-rls-check-multi-row-writes.md | 36 +++++++++++-------- 1 file changed, 21 insertions(+), 15 deletions(-) diff --git a/.changeset/19950-rls-check-multi-row-writes.md b/.changeset/19950-rls-check-multi-row-writes.md index ccf08057498..8a18ab3971c 100644 --- a/.changeset/19950-rls-check-multi-row-writes.md +++ b/.changeset/19950-rls-check-multi-row-writes.md @@ -1,26 +1,32 @@ --- -"@objectstack/plugin-security": patch -"@objectstack/objectql": patch +"@objectstack/plugin-security": minor +"@objectstack/objectql": minor --- -fix(plugin-security): a row-level `check` now holds for every row a multi-row write stores — an array insert and a predicate (`multi: true`) update (#19950, #19964) +fix(plugin-security, objectql)!: a row-level `check` now holds for every row a multi-row write stores — an array insert and a predicate (`multi: true`) update (#19950, #19964) -A row-level security `check` (declared on the policy, or defaulted from its `using`) is the write-side half of the policy: a row the check refuses is never stored. The write gate enforced it for a single-row insert and for a by-id update. It did not enforce it for the two multi-row write shapes: +Clause-②: no (narrowing) -- **An array insert** (`insert(object, [rows])`, which the create-many data route calls). No check was installed for an array payload, so the rows were stored unjudged. -- **A predicate update** (`update(object, changes, { where, multi: true })`). The new rows were never judged. The gate assumed a `using`-scoped `where` covered the write, but a policy that declares only `check` scopes nothing, and a scoped `where` says nothing about the new row in any case. + -Both shapes are now judged row by row, with the same refusal the single-row shapes give: `403 PERMISSION_DENIED`, and nothing is stored. One failing row refuses the whole write. +**BREAKING**: this narrows the set of writes the write gate accepts. A multi-row write that is admitted today can be refused after this change. It ships as `minor` under the launch-window convention, as the using-defaulted check did (#19942). -- **Array insert.** Every row is judged on the image the `beforeInsert` chain produced, as a single insert is. -- **Predicate update.** Every row the write selects is judged on its new image: the matched row merged with the final payload. The engine supplies the rows (`@objectstack/objectql`): the security layer installs its judgement on `OperationContext.postHookWriteImageCheck`, the seam the insert check already uses, and the engine runs it on the predicate path over the rows its composed query selects, once the payload is final. That is the one matched-row read the path already makes for validation and per-row hooks, not a second one. +A row-level security `check` (declared on the policy, or defaulted from its `using`) is the write-side half of the policy: a row the check refuses is never stored. The write gate enforced it for a single-row insert and a by-id update, but not for the two multi-row write shapes. An **array insert** (`insert(object, [rows])`, which the create-many data route calls) installed no check, so its rows were stored unjudged. A **predicate update** (`update(object, changes, { where, multi: true })`) never judged its new rows. The gate assumed a `using`-scoped `where` covered the write, but a policy that declares only `check` scopes nothing, and a scoped `where` says nothing about the new row in any case. -**Writes that are now refused.** A multi-row write is refused where the same row written alone would be: +Both shapes are now judged row by row. An array insert judges each row on the image the `beforeInsert` chain produced. A predicate update judges each row the write selects on its new image: the matched row merged with the final payload. The engine (`@objectstack/objectql`) supplies those rows through the seam the insert check already uses (`OperationContext.postHookWriteImageCheck`). It runs the judgement on the predicate path over the rows its composed query selects, reusing the matched-row read that path already makes. -- a predicate update under a policy that declares only `check`, when any matched row's new image fails that check; -- a predicate update that moves a matched row out of a policy's `using` when no applicable policy declares `check`. The `using` is the defaulted check, and this is the answer the by-id update already gives; -- an array insert when any row fails the check, including every configuration that already refused each single insert (a `using` or `check` that does not compile). +**Writes that are now refused.** Each refusal is the existing row-level CHECK denial, `403 PERMISSION_DENIED`, and nothing is stored. One failing row refuses the whole write. There is no transition switch. -**What does not change.** A by-id update and a single-row insert are judged exactly as before. A predicate update still touches only the rows its scoped `where` selects; the check refuses a write, it never widens or narrows which rows are selected. A predicate update whose new rows all pass is admitted as before. A system-context write is not gated. +- **A predicate update under a policy that declares `check`**, when any matched row's new image fails that check, including when the policy has no `using` at all. +- **A predicate update that moves a matched row out of a policy's `using`**, when no applicable policy declares `check`. The `using` is the defaulted check; a by-id update already gives this answer. +- **An array insert** when any row fails the check. This includes every configuration that already refused each single insert, such as a `using` or `check` that does not compile. +- **A predicate update on a host that installs the judgement and never runs it**, for example a custom write executor in place of the engine. It is refused as an insert already is, with an `error` log saying the check was not evaluated. -**Hosts that run the security plugin with their own write executor.** A predicate update that installs the judgement and returns without the engine having run it is now refused, as an insert already is: `403`, and an `error` log saying the check was not evaluated. +**Remedy.** To let a write store a row outside a policy's scope, declare a `check` on that policy that admits it; otherwise fix the data the write carries. + +**What does not change.** + +- A by-id update and a single-row insert are judged exactly as before. +- A predicate update still touches only the rows its scoped `where` selects. The check refuses a write; it never changes which rows are selected. +- A predicate update or array insert whose rows all pass is admitted as before. +- A system-context write is not gated. From 16a28ad59f3ed859f95ba29a98f79e2e6ea0353d Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 24 Sep 2026 15:22:25 +0000 Subject: [PATCH 4/5] docs(plugin-security): say the #16608 citations this change adds no longer resolve The four comment lines this change rewrote cite an issue that answers 404 on the board. Each keeps the number and now says so, naming the live record: commit a016f08b8a on main. Claude-Session: https://claude.ai/code/session_01Evb5jFDZGKQE9KG4jbMfMF Co-authored-by: Claude --- .../plugins/plugin-security/src/security-plugin.ts | 12 ++++++++---- 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/packages/plugins/plugin-security/src/security-plugin.ts b/packages/plugins/plugin-security/src/security-plugin.ts index 6a8f65f0249..e6909a15874 100644 --- a/packages/plugins/plugin-security/src/security-plugin.ts +++ b/packages/plugins/plugin-security/src/security-plugin.ts @@ -204,7 +204,8 @@ export function hasPlatformAdminCapability(held: ReadonlySet): boolean { } /** - * [#16608] The write `check` for the writes this middleware cannot judge on + * [#16608] (no longer resolves; the live record is commit a016f08b8a) + * The write `check` for the writes this middleware cannot judge on * its own, installed on the operation context for the engine to run on the * rows that will be stored: every row of an insert, single or array, once the * `beforeInsert` chain has produced it, and [#19950] every row a predicate @@ -1814,7 +1815,8 @@ export class SecurityPlugin implements Plugin { // Register security middleware ql.registerMiddleware(async (opCtx: any, next: () => Promise) => { - // [#16608] The write `check` step 3.6 installs on the operation context + // [#16608] (no longer resolves; the live record is commit a016f08b8a) + // The write `check` step 3.6 installs on the operation context // for the engine to run: on an insert, after `beforeInsert`; [#19950] on // a predicate update, over every matched row once the payload is final. // Held here so the post-`next()` assertion below can read whether the @@ -3075,7 +3077,8 @@ export class SecurityPlugin implements Plugin { }; const satisfiesCheck = (image: Record): boolean => checkParts.every((f) => matchesFilterCondition(image as any, f as any)); - // [#16608] The judgement the engine runs: every image it hands over + // [#16608] (no longer resolves; the live record is commit a016f08b8a) + // The judgement the engine runs: every image it hands over // must pass, and the first that fails refuses the whole write. The // compiled filter is captured HERE — while the caller's permission // sets, the delegator's, the staged membership and this request's @@ -3092,7 +3095,8 @@ export class SecurityPlugin implements Plugin { }); if (opCtx.operation === 'insert') { - // [#16608] The engine runs it on the rows the `beforeInsert` chain + // [#16608] (no longer resolves; the live record is commit a016f08b8a) + // The engine runs it on the rows the `beforeInsert` chain // produced — [#19964] every row of an array insert. writeImageCheckSeam = newWriteImageCheck(); opCtx.postHookWriteImageCheck = writeImageCheckSeam; From 3247efeecd57583655db1005b027b437b53b9eab Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 24 Sep 2026 15:42:55 +0000 Subject: [PATCH 5/5] docs(plugin-security): cite the surviving insert-check commit instead of a card that no longer resolves The four comment lines this change rewrote named a card that answers 404 on the board. They now name the surviving record, commit a016f08b8a (the insert-side check), and say in words that the original card no longer resolves. Unchanged lines elsewhere in the file are left as they are. Claude-Session: https://claude.ai/code/session_01Evb5jFDZGKQE9KG4jbMfMF Co-authored-by: Claude --- packages/plugins/plugin-security/src/security-plugin.ts | 8 ++++---- 1 file changed, 4 insertions(+), 4 deletions(-) diff --git a/packages/plugins/plugin-security/src/security-plugin.ts b/packages/plugins/plugin-security/src/security-plugin.ts index e6909a15874..25c62e79440 100644 --- a/packages/plugins/plugin-security/src/security-plugin.ts +++ b/packages/plugins/plugin-security/src/security-plugin.ts @@ -204,7 +204,7 @@ export function hasPlatformAdminCapability(held: ReadonlySet): boolean { } /** - * [#16608] (no longer resolves; the live record is commit a016f08b8a) + * [insert-check commit a016f08b8a] (the original card no longer resolves) * The write `check` for the writes this middleware cannot judge on * its own, installed on the operation context for the engine to run on the * rows that will be stored: every row of an insert, single or array, once the @@ -1815,7 +1815,7 @@ export class SecurityPlugin implements Plugin { // Register security middleware ql.registerMiddleware(async (opCtx: any, next: () => Promise) => { - // [#16608] (no longer resolves; the live record is commit a016f08b8a) + // [insert-check commit a016f08b8a] (the original card no longer resolves) // The write `check` step 3.6 installs on the operation context // for the engine to run: on an insert, after `beforeInsert`; [#19950] on // a predicate update, over every matched row once the payload is final. @@ -3077,7 +3077,7 @@ export class SecurityPlugin implements Plugin { }; const satisfiesCheck = (image: Record): boolean => checkParts.every((f) => matchesFilterCondition(image as any, f as any)); - // [#16608] (no longer resolves; the live record is commit a016f08b8a) + // [insert-check commit a016f08b8a] (the original card no longer resolves) // The judgement the engine runs: every image it hands over // must pass, and the first that fails refuses the whole write. The // compiled filter is captured HERE — while the caller's permission @@ -3095,7 +3095,7 @@ export class SecurityPlugin implements Plugin { }); if (opCtx.operation === 'insert') { - // [#16608] (no longer resolves; the live record is commit a016f08b8a) + // [insert-check commit a016f08b8a] (the original card no longer resolves) // The engine runs it on the rows the `beforeInsert` chain // produced — [#19964] every row of an array insert. writeImageCheckSeam = newWriteImageCheck();