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..8a18ab3971c --- /dev/null +++ b/.changeset/19950-rls-check-multi-row-writes.md @@ -0,0 +1,32 @@ +--- +"@objectstack/plugin-security": minor +"@objectstack/objectql": minor +--- + +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) + +Clause-②: no (narrowing) + + + +**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). + +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. + +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. + +**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. + +- **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. + +**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. 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-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..389b4bd2f68 --- /dev/null +++ b/packages/plugins/plugin-security/src/rls-check-multi-row-writes.test.ts @@ -0,0 +1,374 @@ +// 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 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. 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 + * + * - 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`); + * - 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. + */ + +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 #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 + * 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`); +}; + +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>; + return Object.fromEntries( + rows.map((r) => [r.id, { title: r.title, status: r.status, owner: r.owner }]), + ) 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); + 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..039fc231ca0 100644 --- a/packages/plugins/plugin-security/src/security-plugin.test.ts +++ b/packages/plugins/plugin-security/src/security-plugin.test.ts @@ -30,13 +30,23 @@ 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]). + * + * [#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; - await seam.evaluate([opCtx.data]); + 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 be9f98572d0..25c62e79440 100644 --- a/packages/plugins/plugin-security/src/security-plugin.ts +++ b/packages/plugins/plugin-security/src/security-plugin.ts @@ -204,9 +204,12 @@ 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. + * [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 + * `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 +219,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 +1815,15 @@ 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`. + // [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. // 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 +2054,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); @@ -2993,11 +3001,30 @@ 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. + // + // ── [#19950 / #19964] EVERY row a write stores ──────────────────────── + // + // 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 && typeof opCtx.data === 'object' && - !Array.isArray(opCtx.data) && + (opCtx.operation === 'insert' || !Array.isArray(opCtx.data)) && permissionSets.length > 0 && !!opCtx.context?.userId ) { @@ -3016,8 +3043,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)`, @@ -3049,45 +3077,62 @@ export class SecurityPlugin implements Plugin { }; const satisfiesCheck = (image: Record): boolean => checkParts.every((f) => matchesFilterCondition(image as any, f as any)); + // [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 + // 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 row the - // `beforeInsert` chain produced. 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; + // [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(); + 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(); } } } @@ -3467,9 +3512,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?)` —