diff --git a/.changeset/19950-rls-check-multi-row-writes.md b/.changeset/19950-rls-check-multi-row-writes.md index 8a18ab3971c..4f3fdb5bbfe 100644 --- a/.changeset/19950-rls-check-multi-row-writes.md +++ b/.changeset/19950-rls-check-multi-row-writes.md @@ -26,7 +26,7 @@ Both shapes are now judged row by row. An array insert judges each row on the im **What does not change.** -- A by-id update and a single-row insert are judged exactly as before. +- A single-row insert is judged exactly as before. A by-id update is not changed by this entry; its judgement on the row it stores is its own entry (#19989). - 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/19989-by-id-update-post-hook-check.md b/.changeset/19989-by-id-update-post-hook-check.md new file mode 100644 index 00000000000..1bf3fce9725 --- /dev/null +++ b/.changeset/19989-by-id-update-post-hook-check.md @@ -0,0 +1,31 @@ +--- +"@objectstack/plugin-security": minor +"@objectstack/objectql": minor +--- + +fix(plugin-security, objectql)!: a by-id update's row-level `check` now holds for the row it stores, after the `beforeUpdate` chain (#19989) + +Clause-②: no (narrowing) + + + +**BREAKING**: this narrows the set of writes the write gate accepts. A by-id update that is admitted today can be refused after this change. It ships as `minor` under the launch-window convention, as the multi-row check did (#19950). + +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. An insert and a predicate update are judged on the row the driver stores. A by-id update was judged only on the change set as the caller sent it, merged with the stored row, before the `beforeUpdate` chain ran. A value a hook wrote into a checked field after that point was never judged, so the row it produced could be stored outside the policy. + +A by-id update is now also judged on the row it stores: the prior row merged with the final payload, after the `beforeUpdate` chain and both readonly strips, before the statement. The engine (`@objectstack/objectql`) runs that judgement through the seam the insert and predicate update already use (`OperationContext.postHookWriteImageCheck`). The existing judgement of the change set as sent stays, so this change only ever refuses more. + +**Writes that are now refused.** Each refusal is the existing row-level CHECK denial, `403 PERMISSION_DENIED`, and nothing is stored. There is no transition switch. + +- **A by-id update whose `beforeUpdate` chain writes a checked field to a value the check refuses**, including a value derived from a field the caller changed. +- **A by-id 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 and a predicate update already are, with an `error` log saying the check was not evaluated. +- **An update whose payload `id` addresses no row while `where.id` addresses one**, under a policy with a `check`. The engine writes the `where.id` row while the gate had judged the payload id. It used to be written and then refused; it is now refused before anything runs. + +**Remedy.** A hook that must store a value the caller's `check` refuses does so in a separate write under a system context, or the policy declares a `check` that admits it. Otherwise fix the data the write carries. For the last case, address the row with one id: `update(object, { id, ...fields })` or `update(object, fields, { where: { id } })`. + +**What does not change.** + +- A by-id update whose hooks leave the checked fields inside the check is admitted as before. +- A change set the check refuses as sent is refused as before, even when a hook would have replaced the refused value. +- Inserts and predicate updates are judged exactly as before. +- A system-context write is not gated. diff --git a/.changeset/insert-check-post-image.md b/.changeset/insert-check-post-image.md index 33fd97c8e46..1bc422f7b84 100644 --- a/.changeset/insert-check-post-image.md +++ b/.changeset/insert-check-post-image.md @@ -18,7 +18,7 @@ A denormalised scoping field is exactly what an RLS predicate compares (ADR-0055 Ruled 2026-09-07 (maintainer, verbatim 「同意」, director seat, summon #17, decision batch #3). The refused alternative — keep the order and write the contract that a checked field must arrive from the caller, plus an `os validate` rule to police it — institutionalises the contradiction and needs a permanent lint to hold it in place. -**What changed, mechanically.** `OperationContext` gains `postHookWriteImageCheck` (`@objectstack/objectql`), an optional judgement an enforcement layer installs and `ObjectQL.insert` runs once the `beforeInsert` chain has produced the row — after the post-hook declared-field door, after the two value-changing strips (`stripRuntimeOwnedFields` and the static-`readonly` strip with its `defaultValue` re-default, both moved ahead of it), and before every producer with a side effect (the secret channel, the autonumber, validation, the statement), so a refusal still costs nothing. `@objectstack/plugin-security` installs its compiled `check` filter there for `insert` instead of matching it against `opCtx.data`; `update` is unchanged. The compiled filter is still built in the middleware, where the caller's permission sets, the ADR-0090 D10 delegator's, the staged membership and the request context are all resolved — only the IMAGE is deferred. A middleware that installed the judgement and finds the seam was never run refuses the write and logs at ERROR: an unjudged write is not an allowed one. +**What changed, mechanically.** `OperationContext` gains `postHookWriteImageCheck` (`@objectstack/objectql`), an optional judgement an enforcement layer installs and `ObjectQL.insert` runs once the `beforeInsert` chain has produced the row — after the post-hook declared-field door, after the two value-changing strips (`stripRuntimeOwnedFields` and the static-`readonly` strip with its `defaultValue` re-default, both moved ahead of it), and before every producer with a side effect (the secret channel, the autonumber, validation, the statement), so a refusal still costs nothing. `@objectstack/plugin-security` installs its compiled `check` filter there for `insert` instead of matching it against `opCtx.data`; this entry leaves `update` unchanged, and the predicate and by-id updates move onto the same seam in their own entries (#19950, #19989). The compiled filter is still built in the middleware, where the caller's permission sets, the ADR-0090 D10 delegator's, the staged membership and the request context are all resolved — only the IMAGE is deferred. A middleware that installed the judgement and finds the seam was never run refuses the write and logs at ERROR: an unjudged write is not an allowed one. **Who is affected.** Only objects governed by a permission set that EXPLICITLY declares `check`, on single-row inserts by a non-system caller — the gate's existing scope, unchanged. Two behaviour changes to expect, and they are the two halves of the same correction: an insert that left a hook-stamped field off the payload now succeeds where it used to be refused, and an insert whose hook-stamped field lands outside the caller's scope is now refused where it used to be admitted. Callers that were duplicating the stamp to get past the gate keep working and may stop. diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index e5f07827259..98642268d09 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -2235,8 +2235,12 @@ export interface OperationContext { * 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. + * + * [#19989] A by-id `update` uses it too. A middleware can read that one row, + * but only BEFORE the `beforeUpdate` chain runs, so its image is the change + * set as sent and a hook that rewrites a judged field is never judged. So + * {@link ObjectQL.update} calls it on the by-id path as well, once the + * payload is final, with the prior row merged with the payload. * * ABSENT is the ordinary state — no enforcement layer is mounted, or the * write is one it does not gate. The engine never invents one. @@ -2253,7 +2257,8 @@ export interface OperationContext { * 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. + * payload; on a by-id `update` ([#19989]) the one prior row 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 @@ -13420,6 +13425,41 @@ export class ObjectQL implements IObjectQLEngine { // "you sent a read-only field" should not depend on whether some // other field also failed a business rule. assertNoStrictDrops(); + // ── [#19989] The post-image seam on the BY-ID path ───────────── + // + // The by-id twin of the predicate-path call below, placed at the + // same point and for the same reason: the payload is FINAL here. + // The `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. + // + // An enforcement layer used to judge this row only in its own + // middleware, on its read of the row merged with the change set + // AS SENT. That image is taken before `next()` runs the hooks, so + // a `beforeUpdate` that rewrote a checked field (a scoping column + // derived from a re-pointed parent, a status derived from another + // field) was never judged, and the row it produced was stored + // unjudged. The layer keeps that earlier judgement and installs + // this seam as well, so the row the driver stores is judged too. + // + // The image is the prior row (read once, above, under the + // not-found gate, so it is present) merged with the final + // payload: the row `driver.update` is about to produce, and the + // shape the predicate path hands over per matched row. The + // credential channel runs above on this branch too, so a `check` + // naming a secret field judges the stored reference, as on the + // predicate path. + // + // `honoured` is set BEFORE `evaluate`: it answers "did the seam + // run", never "did the write pass". + const byIdImageCheck = opCtx.postHookWriteImageCheck; + if (byIdImageCheck) { + byIdImageCheck.honoured = true; + const payload = hookContext.input.data as Record; + await byIdImageCheck.evaluate([ + coerceBooleanFields(updateSchema as any, { ...priorRecord, ...payload } as any) as Record, + ]); + } // [#18682] The reference FK a predicate traverses may come from // the PATCH or from the stored row, so the id is read off the // POST-strip merged view `evaluateValidationRules` evaluates. @@ -13629,9 +13669,11 @@ export class ObjectQL implements IObjectQLEngine { // // 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 + // path"). For a by-id update the enforcement middleware knows the + // one row, but can read it only before the hooks run, so that + // path hands the final row to the same seam ([#19989], the by-id + // branch above). For a predicate update the middleware cannot + // even name the rows: they 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 diff --git a/packages/plugins/plugin-auth/src/sys-user-self-service-route.test.ts b/packages/plugins/plugin-auth/src/sys-user-self-service-route.test.ts index 98d69aa98ae..ec841179b3e 100644 --- a/packages/plugins/plugin-auth/src/sys-user-self-service-route.test.ts +++ b/packages/plugins/plugin-auth/src/sys-user-self-service-route.test.ts @@ -61,6 +61,7 @@ import { describe, it, expect, vi } from 'vitest'; import { SysUser } from '@objectstack/platform-objects/identity'; import { SecurityPlugin, securityDefaultPermissionSets } from '@objectstack/plugin-security'; +import { assertEngineUpdateDispatch } from '@objectstack/objectql'; import { registerIdentityWriteGuard, registerManagedUpdateWhitelist, @@ -204,9 +205,31 @@ async function route( context, }; + // [#19989] The engine's half of the write gate, which the terminal `next()` + // stands in for. `ObjectQL.update` runs the installed + // `postHookWriteImageCheck` on the row a by-id update writes (the stored row + // merged with the payload) before the statement; a terminal that skipped it + // would be refused fail-closed by the middleware, and would read here as a + // `row-scope` refusal. The row comes straight from the fixture, NOT through + // `findOne`, so `preImageWheres` still records only the middleware's reads. + // The guard below runs after this, so the image carries the pre-guard + // payload; the check this file composes (`sys_user_self`, whose `using` is + // the check: `id == current_user.id`) reads only `id`, which the guard never + // touches. Only the by-id path is modelled, through the producer's own + // dispatch predicate. + const runByIdWriteImageCheck = async () => { + const seam = opCtx.postHookWriteImageCheck; + if (!seam || operation !== 'update') return; + const dispatch = assertEngineUpdateDispatch(opCtx.data, opCtx.options); + if (dispatch.kind !== 'by-id') return; + seam.honoured = true; + const row = ROWS[String(dispatch.id)]; + await seam.evaluate(row ? [{ ...row, ...opCtx.data }] : []); + }; + const snapshotWrites: Array<{ key: string; value: any }> = []; try { - await middleware(opCtx, async () => {}); + await middleware(opCtx, runByIdWriteImageCheck); } catch (error: any) { return { // The pre-image re-read is the first engine call past the CRUD gate, so diff --git a/packages/plugins/plugin-security/src/authored-row-write-verdict.test.ts b/packages/plugins/plugin-security/src/authored-row-write-verdict.test.ts index 4f6aa7bd2c7..20ddaecd1da 100644 --- a/packages/plugins/plugin-security/src/authored-row-write-verdict.test.ts +++ b/packages/plugins/plugin-security/src/authored-row-write-verdict.test.ts @@ -231,12 +231,23 @@ function makeEngine(opts: { findOneThrows?: boolean } = {}) { }, // Both write verbs open with the PRODUCER's own dispatch predicate // (#4550 / #5480 / #6277), never a hand-mirrored guard. - async update(object: string, data: any, options?: any) { + // + // [#19989] `opCtx` is the operation the middleware chain ran on. The engine + // runs an installed write-image check on every update before it writes + // (the rows it will store, each merged with the payload), so this double + // does too: one 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 = 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; }, @@ -303,7 +314,7 @@ async function makeStack(engineOpts: { findOneThrows?: boolean } = {}): Promise< let reached = false; try { await securityMw(opCtx, async () => { - await engine.update(opCtx.object, opCtx.data, opCtx.options); + await engine.update(opCtx.object, opCtx.data, opCtx.options, opCtx); reached = true; }); } catch (e: any) { diff --git a/packages/plugins/plugin-security/src/authz-matrix-gate.test.ts b/packages/plugins/plugin-security/src/authz-matrix-gate.test.ts index a31805e7c30..c5e33193aaf 100644 --- a/packages/plugins/plugin-security/src/authz-matrix-gate.test.ts +++ b/packages/plugins/plugin-security/src/authz-matrix-gate.test.ts @@ -46,6 +46,7 @@ import { describe, it, expect, vi } from 'vitest'; import { derivePosture, POSTURE_RANK } from '@objectstack/core'; +import { assertEngineUpdateDispatch } from '@objectstack/metadata-core'; import { SecurityPlugin, hasPlatformAdminCapability } from './security-plugin.js'; import { PermissionEvaluator } from './permission-evaluator.js'; import { defaultPermissionSets, BETTER_AUTH_MANAGED_OBJECTS } from './objects/default-permission-sets.js'; @@ -154,7 +155,26 @@ function makeHarness(opts: { return services[name]; }, }; - return { ctx, findOne, run: async (opCtx: any) => { await middleware(opCtx, async () => {}); return opCtx; } }; + // [#19989] The engine's stored-row check on a by-id UPDATE, which the empty + // terminal stands in for: `ObjectQL.update` runs the installed + // `postHookWriteImageCheck` on the row it writes (here the stubbed pre-image, + // merged with the payload) before the statement, and a terminal that skipped + // it would be refused fail-closed by the middleware. Only the by-id path is + // modelled, through the producer's own dispatch predicate. + const runByIdWriteImageCheck = async (opCtx: any) => { + const seam = opCtx?.postHookWriteImageCheck; + if (!seam || opCtx.operation !== 'update') return; + const dispatch = assertEngineUpdateDispatch(opCtx.data, opCtx.options); + if (dispatch.kind !== 'by-id') return; + seam.honoured = true; + const prior = opts.findOneImpl ? opts.findOneImpl({ where: { id: dispatch.id } }) : null; + await seam.evaluate(prior ? [{ ...prior, ...opCtx.data }] : []); + }; + return { + ctx, + findOne, + run: async (opCtx: any) => { await middleware(opCtx, () => runByIdWriteImageCheck(opCtx)); return opCtx; }, + }; } /** Effective READ filter the engine would AND onto a `find` (the visible-row set). */ 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 459a1c3cbcd..810cb41f7e2 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 @@ -293,15 +293,16 @@ function makeEngine() { // [#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. + // before it writes, and [#19989] on the BY-ID path the one row it writes, + // merged with the payload. This double does the same on both: 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; + const seam = opCtx?.postHookWriteImageCheck; if (seam) { seam.honoured = true; await seam.evaluate(targets.map((r) => ({ ...r, ...data }))); diff --git a/packages/plugins/plugin-security/src/controlled-by-parent-detail-write-authority.test.ts b/packages/plugins/plugin-security/src/controlled-by-parent-detail-write-authority.test.ts index 6f05de3b9dd..7441c7afbb2 100644 --- a/packages/plugins/plugin-security/src/controlled-by-parent-detail-write-authority.test.ts +++ b/packages/plugins/plugin-security/src/controlled-by-parent-detail-write-authority.test.ts @@ -56,7 +56,7 @@ import { matchesFilterCondition } from '@objectstack/formula'; import { PermissionSetSchema } from '@objectstack/spec/security'; import type { PermissionSet } from '@objectstack/spec/security'; import { defaultPermissionSets } from './objects/default-permission-sets.js'; -import { assertEngineFindOnePredicate } from '@objectstack/metadata-core'; +import { assertEngineFindOnePredicate, assertEngineUpdateDispatch } from '@objectstack/metadata-core'; const MKT = 'usr_marketing'; const ADMIN = 'usr_admin'; @@ -380,6 +380,23 @@ function makeStore(rows: Record) { const all = rows[object] ?? []; return all.find((r) => matchesFilterCondition(r, options?.where ?? options?.filter ?? null)) ?? null; }), + /** + * [#19989] Not a write verb: the engine's stored-row check, which the + * terminal `next()` stands in for on a by-id UPDATE. `ObjectQL.update` runs + * the installed `postHookWriteImageCheck` on the row it writes, merged with + * the payload, before the statement; a terminal that skipped it would be + * refused fail-closed by the security middleware. Only the by-id path is + * modelled, through the producer's own dispatch predicate. + */ + async runByIdWriteImageCheck(opCtx: any) { + const seam = opCtx?.postHookWriteImageCheck; + if (!seam || opCtx.operation !== 'update') return; + const dispatch = assertEngineUpdateDispatch(opCtx.data, opCtx.options); + if (dispatch.kind !== 'by-id') return; + seam.honoured = true; + const targets = (rows[opCtx.object] ?? []).filter((r) => r.id === dispatch.id); + await seam.evaluate(targets.map((r) => ({ ...r, ...opCtx.data }))); + }, }; } @@ -457,6 +474,7 @@ async function boot(seed: { shares?: Row[] } = {}) { try { await securityMw(opCtx, async () => { await sharingMw(opCtx, async () => { + await store.runByIdWriteImageCheck(opCtx); reached = true; }); }); diff --git a/packages/plugins/plugin-security/src/controlled-by-parent-master-widener.test.ts b/packages/plugins/plugin-security/src/controlled-by-parent-master-widener.test.ts index d5b1d26218e..be40b831a2e 100644 --- a/packages/plugins/plugin-security/src/controlled-by-parent-master-widener.test.ts +++ b/packages/plugins/plugin-security/src/controlled-by-parent-master-widener.test.ts @@ -59,7 +59,7 @@ import { matchesFilterCondition } from '@objectstack/formula'; import { PermissionSetSchema } from '@objectstack/spec/security'; import type { PermissionSet } from '@objectstack/spec/security'; import { defaultPermissionSets } from './objects/default-permission-sets.js'; -import { assertEngineFindOnePredicate } from '@objectstack/metadata-core'; +import { assertEngineFindOnePredicate, assertEngineUpdateDispatch } from '@objectstack/metadata-core'; const MKT = 'usr_marketing'; const ADMIN = 'usr_admin'; @@ -276,6 +276,23 @@ function makeStore(rows: Record) { const all = rows[object] ?? []; return all.find((r) => matchesFilterCondition(r, options?.where ?? options?.filter ?? null)) ?? null; }), + /** + * [#19989] Not a write verb: the engine's stored-row check, which the + * terminal `next()` stands in for on a by-id UPDATE. `ObjectQL.update` runs + * the installed `postHookWriteImageCheck` on the row it writes, merged with + * the payload, before the statement; a terminal that skipped it would be + * refused fail-closed by the security middleware. Only the by-id path is + * modelled, through the producer's own dispatch predicate. + */ + async runByIdWriteImageCheck(opCtx: any) { + const seam = opCtx?.postHookWriteImageCheck; + if (!seam || opCtx.operation !== 'update') return; + const dispatch = assertEngineUpdateDispatch(opCtx.data, opCtx.options); + if (dispatch.kind !== 'by-id') return; + seam.honoured = true; + const targets = (rows[opCtx.object] ?? []).filter((r) => r.id === dispatch.id); + await seam.evaluate(targets.map((r) => ({ ...r, ...opCtx.data }))); + }, }; } @@ -339,6 +356,7 @@ async function boot(options: { share?: ShareSpec } = {}) { try { await securityMw(opCtx, async () => { await sharingMw(opCtx, async () => { + await store.runByIdWriteImageCheck(opCtx); reached = true; }); }); diff --git a/packages/plugins/plugin-security/src/insert-check-post-image.test.ts b/packages/plugins/plugin-security/src/insert-check-post-image.test.ts index 55940fab01f..d87cf203414 100644 --- a/packages/plugins/plugin-security/src/insert-check-post-image.test.ts +++ b/packages/plugins/plugin-security/src/insert-check-post-image.test.ts @@ -53,13 +53,13 @@ * and refused by the post-hook one, which is what makes it the ablation of this * whole change rather than a restatement of it. * - * ⚠️ WHAT THIS FILE DOES NOT CLAIM. The two verbs still diverge on one route: - * an UPDATE that repoints the parent so the `beforeUpdate` stamp rewrites the - * checked field AFTER the middleware merged its pre-image is admitted, and the - * row is stored in an organization the caller does not hold. Measured on this - * branch, both drivers; filed as #16790, which is this card's defect one verb - * over. It is deliberately NOT pinned here — a test asserting today's answer - * there would advertise a guarantee the runtime does not deliver (PD #10). + * The UPDATE route this file does not take — a `beforeUpdate` hook rewriting + * the checked field after the middleware merged its pre-image — was measured on + * this change's branch as admitted, with the row stored in an organization the + * caller does not hold (the original tracker no longer resolves). #19989 closed + * it: the engine now runs the same seam on a by-id update, on the row it + * stores. Its pins live in `rls-check-by-id-update-post-hook.test.ts`, not + * here. * * Ground truth is read straight off the driver's own table, past every scope — * "the check refused" and "nothing was stored" are separate facts and both are @@ -508,13 +508,10 @@ const DRIVERS: Array<[string, () => unknown]> = [ * landing). That is the update path's long-standing behaviour and it is * what this change makes insert agree with. * - * ⚠️ The route the update arm does NOT take — repointing the parent so the - * `beforeUpdate` stamp rewrites the checked field after the merge — is the same - * defect one verb over, and it is still open: measured on this branch as - * ADMITTED with the row stored in an organization the caller does not hold, and - * filed as #16790. It is deliberately not pinned here: this file asserts what - * the runtime guarantees, and ⛔ a test that pinned today's answer there would - * be advertising a guarantee the runtime does not deliver. + * The route the update arm does NOT take — repointing the parent so the + * `beforeUpdate` stamp rewrites the checked field after the merge — was the same + * defect one verb over. #19989 closed it; its cells, on both drivers, live in + * `rls-check-by-id-update-post-hook.test.ts`. */ interface VerbArm { verb: 'insert' | 'update'; diff --git a/packages/plugins/plugin-security/src/rls-check-by-id-update-post-hook.test.ts b/packages/plugins/plugin-security/src/rls-check-by-id-update-post-hook.test.ts new file mode 100644 index 00000000000..76b54355898 --- /dev/null +++ b/packages/plugins/plugin-security/src/rls-check-by-id-update-post-hook.test.ts @@ -0,0 +1,484 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#19989] A by-id UPDATE's row-level `check` holds for the row that is STORED, + * after the `beforeUpdate` chain — as the insert and the predicate update + * already do. + * + * ## 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 enforces it on the by-id write path and on the + * bulk path. The insert judges the row the `beforeInsert` chain produced, and + * the predicate update judges each matched row merged with the FINAL payload, + * both through `OperationContext.postHookWriteImageCheck`, run by the engine. + * + * The by-id update was judged only inside the security middleware, on the + * caller's pre-image merged with the change set AS SENT, before `next()` runs + * the `beforeUpdate` chain. A hook that rewrote a checked field after that + * point was never judged, so the row it produced was stored unjudged. + * + * ## What this file pins, on both SQL driver families + * + * - each reproduced shape, failing first: the by-id update is refused on the + * ADR-0112 envelope (`PERMISSION_DENIED` / 403), and the stored row, read + * under a system context, did not move; + * - the STORED row, not the change set: the image carries the fields the + * update leaves unchanged, so a check over one of them still holds; + * - the controls: an in-scope by-id update is admitted and stored; a hook + * that touches no checked field changes nothing; the judgement the + * middleware already made on the change set as sent still refuses (the fix + * only ever refuses MORE); the predicate update gives the same answer as + * its by-id twin; + * - fail-closed: a host that never runs the installed judgement on a by-id + * update is refused rather than vouched for; + * - one row address: a FALSY payload id beside a truthy `where.id` makes the + * engine write the `where.id` row while the middleware judged the payload + * id. That write used to reach the store and be refused only afterwards; + * it is now refused before anything is stored. + */ + +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 { RLS_MEMBERSHIP_RESOLVER_SERVICE } from '@objectstack/spec/contracts'; +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'; + +/** The organization the caller holds. */ +const OWN_ORG = 'org_a'; +/** An organization the caller does NOT hold. */ +const OTHER_ORG = 'org_b'; + +const OBJECTS = [ + { + name: 'qa_employer', + label: 'Employer', + sharingModel: 'public_read_write', + fields: { + id: { name: 'id', type: 'text', primaryKey: true }, + name: { name: 'name', type: 'text' }, + employer_org: { name: 'employer_org', type: 'text' }, + }, + }, + { + name: 'qa_employer_member', + label: 'Employer member', + sharingModel: 'public_read_write', + fields: { + id: { name: 'id', type: 'text', primaryKey: true }, + employer: { name: 'employer', type: 'lookup', reference: 'qa_employer' }, + employer_org: { name: 'employer_org', type: 'text' }, + role: { name: 'role', type: 'text' }, + }, + }, + { + 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' }, + }, + }, + // The ticket's shape again, under a check that also reads a field the update + // never carries: judging the change set alone would miss it. + { + name: 'qa_case', + label: 'Case', + 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' }, + }, + }, +]; + +const MEMBER_DEFAULT = defaultPermissionSets.find((p) => p.name === 'member_default')!; + +/** + * Two policies, one per shape: + * - the organization shape: `using` + `check` twins over a resolver-owned + * membership key, naming the DENORMALISED scoping field an app stamps from + * the parent (ADR-0055: a predicate cannot traverse the lookup); + * - the plain shape: a check-only policy over a field a hook derives. + */ +const WRITER: PermissionSet = PermissionSetSchema.parse({ + name: 'qa_writer', + objects: { + qa_employer: { allowRead: true, allowCreate: true, allowEdit: true }, + qa_employer_member: { allowRead: true, allowCreate: true, allowEdit: true }, + qa_ticket: { allowRead: true, allowCreate: true, allowEdit: true }, + qa_case: { allowRead: true, allowCreate: true, allowEdit: true }, + }, + rowLevelSecurity: [ + { + name: 'members_in_my_orgs', + object: 'qa_employer_member', + operation: 'all', + using: 'record.employer_org in current_user.employer_org_ids', + check: 'record.employer_org in current_user.employer_org_ids', + }, + { + name: 'tickets_not_closed', + object: 'qa_ticket', + operation: 'update', + check: "record.status != 'closed'", + }, + { + // A high-priority case may not be closed. `priority` is a field the + // update below never carries, so only an image carrying the STORED row's + // unchanged fields can see it. + name: 'high_cases_stay_open', + object: 'qa_case', + operation: 'update', + check: "record.status != 'closed' || record.priority != 'high'", + }, + ], +}); + +const SYS_CTX = { isSystem: true, userId: 'usr_system' }; +const CALLER = { + userId: 'usr_a', + email: 'a@e.example', + positions: ['writer'], + permissions: ['qa_writer'], + posture: 'MEMBER', +}; + +/** The phrase the plain-shape hook reads: a ticket so titled is closed by the app. */ +const CLOSING_TITLE = 'wrap up'; + +const engines: ObjectQL[] = []; +afterEach(async () => { + while (engines.length) { + try { await engines.pop()?.destroy(); } catch { /* noop */ } + } +}); + +interface Booted { + engine: ObjectQL; + /** Every parent id the stamp read, so "the stamp ran" is measured. */ + stampReads: string[]; + /** Every ticket id the closing hook rewrote, so "the hook ran" is measured. */ + closings: unknown[]; + /** A table's rows read straight off the driver, past every scope. */ + table: (name: string, columns: string[]) => Promise>>; +} + +async function boot(makeDriver: () => unknown): Promise { + const engine = new ObjectQL(); + engine.registerDriver(makeDriver() as never, true); + await engine.init(); + engine.registerApp({ + id: 'com.objectstack.qa.rls-check-by-id-update-post-hook', + name: 'RLS check on a by-id update, after beforeUpdate', + version: '1.0.0', + type: 'plugin', + scope: 'system', + objects: OBJECTS, + } as never); + await engine.syncSchemas(); + engines.push(engine); + + // The organization shape's stamp: it reads the parent OUTSIDE RLS and + // overwrites the scoping field from it, whatever the caller sent. + const stampReads: string[] = []; + engine.on('beforeUpdate', 'qa_employer_member', (async (ctx: { input: { data: Record } }) => { + const employerId = ctx.input.data.employer; + if (typeof employerId !== 'string' || employerId === '') return; + stampReads.push(employerId); + const parent = (await engine.findOne('qa_employer', { + where: { id: employerId }, + context: SYS_CTX, + } as never)) as Record | null; + if (parent?.employer_org != null) ctx.input.data.employer_org = parent.employer_org; + }) as never); + + // The plain shape's hook: it derives a checked field from another one. + const closings: unknown[] = []; + const closeOnTitle = (async (ctx: { input: { id?: unknown; data: Record } }) => { + if (ctx.input.data.title !== CLOSING_TITLE) return; + closings.push(ctx.input.id ?? null); + ctx.input.data.status = 'closed'; + }) as never; + engine.on('beforeUpdate', 'qa_ticket', closeOnTitle); + engine.on('beforeUpdate', 'qa_case', closeOnTitle); + + const services: Record = { + manifest: { register: vi.fn() }, + objectql: engine, + metadata: { + get: async (_type: string, name: string) => engine.getSchema(name) ?? null, + list: async () => [MEMBER_DEFAULT, WRITER], + }, + [RLS_MEMBERSHIP_RESOLVER_SERVICE]: { + keys: ['employer_org_ids'], + resolve: vi.fn(async () => ({ employer_org_ids: [OWN_ORG] })), + }, + }; + 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_employer', + [ + { id: 'emp_a', name: 'A', employer_org: OWN_ORG }, + { id: 'emp_b', name: 'B', employer_org: OTHER_ORG }, + ], + { context: SYS_CTX } as never, + ); + await engine.insert( + 'qa_employer_member', + { id: 'mem_1', employer: 'emp_a', employer_org: OWN_ORG, role: 'admin' }, + { context: SYS_CTX } as never, + ); + await engine.insert( + 'qa_ticket', + [ + { id: 't1', title: 'one', status: 'open', priority: 'low' }, + { id: 't2', title: 'two', status: 'open', priority: 'low' }, + ], + { context: SYS_CTX } as never, + ); + await engine.insert( + 'qa_case', + [ + { id: 'c_high', title: 'high', status: 'open', priority: 'high' }, + { id: 'c_low', title: 'low', status: 'open', priority: 'low' }, + ], + { context: SYS_CTX } as never, + ); + + const table = async (name: string, columns: string[]) => { + const driver = (engine as unknown as { getDriver(o: string): { knex: unknown } }).getDriver(name); + const knex = driver.knex as (t: string) => { select: (...c: string[]) => Promise>> }; + const rows = await knex(name).select(...columns); + return [...rows].sort((a, b) => String(a.id).localeCompare(String(b.id))); + }; + + return { engine, stampReads, closings, table }; +} + +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; message?: string; 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; message?: string; developerMessage?: string }; + return { + ok: false, + code: err.code, + status: err.statusCode ?? err.status, + message: String(err.message ?? e), + developerMessage: err.developerMessage, + }; + } +}; + +/** The 3.6 gate's refusal, on the ADR-0112 envelope, never a bare throw. */ +const expectCheckRefusal = (outcome: Outcome) => { + expect(outcome.ok, 'expected a refusal, got a completed update').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 update would violate a row-level CHECK'); +}; + +const byId = (b: Booted, object: string, data: Record) => + b.engine.update(object, data as never, { context: CALLER } as never); + +const MEMBER_COLUMNS = ['id', 'employer', 'employer_org', 'role']; +const TICKET_COLUMNS = ['id', 'title', 'status', 'priority']; +const SEEDED_MEMBER = [{ id: 'mem_1', employer: 'emp_a', employer_org: OWN_ORG, role: 'admin' }]; +const SEEDED_TICKETS = [ + { id: 't1', title: 'one', status: 'open', priority: 'low' }, + { id: 't2', title: 'two', status: 'open', priority: 'low' }, +]; + +for (const [driverName, makeDriver] of DRIVERS) { + describe(`[#19989] a by-id UPDATE is judged on the row the beforeUpdate chain produced — ${driverName}`, () => { + it('the organization shape: re-pointing the parent so the stamp lands an organization the caller does not hold is refused, and nothing moves', async () => { + const b = await boot(makeDriver); + + const outcome = await attempt(() => byId(b, 'qa_employer_member', { id: 'mem_1', employer: 'emp_b' })); + + expectCheckRefusal(outcome); + expect(b.stampReads, 'the stamp ran and read the re-pointed parent').toContain('emp_b'); + const rows = await b.table('qa_employer_member', MEMBER_COLUMNS); + expect(rows).toEqual(SEEDED_MEMBER); + expect(rows.some((r) => r.employer_org === OTHER_ORG), 'no row may carry an organization the caller does not hold') + .toBe(false); + }); + + it('the plain shape: a hook that rewrites a checked field to a value the check refuses is refused, and nothing moves', async () => { + const b = await boot(makeDriver); + + const outcome = await attempt(() => byId(b, 'qa_ticket', { id: 't1', title: CLOSING_TITLE })); + + expectCheckRefusal(outcome); + expect(b.closings, 'the hook ran and rewrote the checked field').toEqual(['t1']); + expect(await b.table('qa_ticket', TICKET_COLUMNS)).toEqual(SEEDED_TICKETS); + }); + + it('the stored row, not the change set: a hook-closed case fails on a field the update never carried, and the same write on a low case lands', async () => { + const b = await boot(makeDriver); + const SEEDED_CASES = [ + { id: 'c_high', title: 'high', status: 'open', priority: 'high' }, + { id: 'c_low', title: 'low', status: 'open', priority: 'low' }, + ]; + + // `priority` is on neither payload; only the stored row carries it. + expectCheckRefusal(await attempt(() => byId(b, 'qa_case', { id: 'c_high', title: CLOSING_TITLE }))); + expect(await b.table('qa_case', TICKET_COLUMNS)).toEqual(SEEDED_CASES); + + const low = await attempt(() => byId(b, 'qa_case', { id: 'c_low', title: CLOSING_TITLE })); + expect(low.ok, `expected the low-priority close to be admitted: ${low.developerMessage ?? low.message}`).toBe(true); + expect(b.closings).toEqual(['c_high', 'c_low']); + expect((await b.table('qa_case', TICKET_COLUMNS))[1]) + .toEqual({ id: 'c_low', title: CLOSING_TITLE, status: 'closed', priority: 'low' }); + }); + }); + + describe(`[#19989] controls — ${driverName}`, () => { + it('⭐ an in-scope by-id update is admitted, and the stored row is the one the stamp produced', async () => { + const b = await boot(makeDriver); + + const outcome = await attempt(() => byId(b, 'qa_employer_member', { id: 'mem_1', employer: 'emp_a', role: 'lead' })); + + expect(outcome.ok, `expected the update to be admitted: ${outcome.developerMessage ?? outcome.message}`).toBe(true); + expect(b.stampReads).toContain('emp_a'); + expect(await b.table('qa_employer_member', MEMBER_COLUMNS)) + .toEqual([{ id: 'mem_1', employer: 'emp_a', employer_org: OWN_ORG, role: 'lead' }]); + }); + + it('⭐ a hook that touches no checked field changes nothing: the update is admitted and stored', async () => { + const b = await boot(makeDriver); + + const outcome = await attempt(() => byId(b, 'qa_ticket', { id: 't1', title: 'renamed', priority: 'high' })); + + expect(outcome.ok, `expected the update to be admitted: ${outcome.developerMessage ?? outcome.message}`).toBe(true); + expect(b.closings, 'the hook did not fire its rewrite').toEqual([]); + expect((await b.table('qa_ticket', TICKET_COLUMNS))[0]) + .toEqual({ id: 't1', title: 'renamed', status: 'open', priority: 'high' }); + }); + + it('the judgement on the change set AS SENT still refuses, as before', async () => { + const b = await boot(makeDriver); + + expectCheckRefusal(await attempt(() => byId(b, 'qa_ticket', { id: 't1', status: 'closed' }))); + expect(await b.table('qa_ticket', TICKET_COLUMNS)).toEqual(SEEDED_TICKETS); + }); + + it('only refuses MORE: a caller value the check refuses stays refused even when the stamp would have replaced it with an in-scope one', async () => { + const b = await boot(makeDriver); + + // The stamp lands `org_a` (emp_a's), which the check admits; the change + // set as sent carries `org_b`, which it refuses. The existing judgement + // on the change set as sent keeps refusing it. + expectCheckRefusal(await attempt(() => + byId(b, 'qa_employer_member', { id: 'mem_1', employer: 'emp_a', employer_org: OTHER_ORG }))); + expect(await b.table('qa_employer_member', MEMBER_COLUMNS)).toEqual(SEEDED_MEMBER); + }); + + it('⭐ the predicate update gives its by-id twin’s answer: the same hook-closed row is refused, and an unrelated rename is admitted', async () => { + const b = await boot(makeDriver); + const bulk = (changes: Record) => + b.engine.update('qa_ticket', changes as never, { where: { priority: 'low' }, multi: true, context: CALLER } as never); + + expectCheckRefusal(await attempt(() => bulk({ title: CLOSING_TITLE }))); + expect(await b.table('qa_ticket', TICKET_COLUMNS)).toEqual(SEEDED_TICKETS); + + expect((await attempt(() => bulk({ title: 'renamed' }))).ok).toBe(true); + expect((await b.table('qa_ticket', TICKET_COLUMNS)).map((r) => r.title)).toEqual(['renamed', 'renamed']); + }); + }); + + describe(`[#19989] fail-closed — ${driverName}`, () => { + it('a host that never runs the installed check does not have its by-id update vouched for', async () => { + const b = await boot(makeDriver); + // 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; + b.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(() => byId(b, 'qa_ticket', { id: 't1', title: 'renamed' })); + + expect(outcome.ok, 'an unjudged by-id 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 by-id update').toBeTruthy(); + expect(seen?.honoured).not.toBe(true); + }); + }); + + describe(`[#19989] one row address — ${driverName}`, () => { + // The engine reads a falsy payload id as no row address and writes the row + // a truthy `where.id` names; the middleware's by-id gates read the payload + // id. Measured before this change: refused with the "executed without the + // row-level CHECK" 403 AFTER the driver had stored the row. + const FALSY_IDS: Array<[string, unknown]> = [["''", ''], ['0', 0]]; + for (const [label, falsyId] of FALSY_IDS) { + it(`a payload id of ${label} beside a truthy where.id is refused before anything is stored`, async () => { + const b = await boot(makeDriver); + + const outcome = await attempt(() => + b.engine.update('qa_ticket', { id: falsyId, title: 'renamed' } as never, { where: { id: 't1' }, context: CALLER } as never)); + + expect(outcome.ok, 'the row the gate judged is not the row the engine writes').toBe(false); + expect(outcome.code).toBe('PERMISSION_DENIED'); + expect(outcome.status).toBe(403); + expect(outcome.developerMessage).toContain('the row this gate judged is not the row that would be stored'); + expect(await b.table('qa_ticket', TICKET_COLUMNS), 'refused BEFORE the store, so nothing moved').toEqual(SEEDED_TICKETS); + }); + } + + it('⭐ control: a falsy payload id with NO where.id is still a predicate update, judged per matched row', async () => { + const b = await boot(makeDriver); + + const outcome = await attempt(() => + b.engine.update('qa_ticket', { id: '', title: 'renamed' } as never, { where: { priority: 'low' }, multi: true, context: CALLER } as never)); + + expect(outcome.ok, `expected the predicate update to be admitted: ${outcome.developerMessage ?? outcome.message}`).toBe(true); + expect((await b.table('qa_ticket', TICKET_COLUMNS)).map((r) => r.title)).toEqual(['renamed', 'renamed']); + }); + }); +} diff --git a/packages/plugins/plugin-security/src/rls-check-membership-staging.test.ts b/packages/plugins/plugin-security/src/rls-check-membership-staging.test.ts index 607f2c02f6d..105c5dc9902 100644 --- a/packages/plugins/plugin-security/src/rls-check-membership-staging.test.ts +++ b/packages/plugins/plugin-security/src/rls-check-membership-staging.test.ts @@ -201,12 +201,22 @@ function makeEngine() { (tables[object] ??= []).push({ ...data }); return data; }, - async update(object: string, data: any, options?: any) { + // [#19989] `opCtx` is the operation the middleware chain ran on. The engine + // runs an installed write-image check on every update before it writes, on + // the rows it will STORE, each merged with the payload: not the payload + // alone, which would judge a row missing every field the update leaves + // unchanged. So this double does the same. + 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 = 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; }, @@ -303,14 +313,17 @@ async function makeStack(resolver: Resolver | null): Promise { // past a gate that never ran, and the middleware refuses exactly that // (fail closed) rather than vouching for it. Flag first — it answers // "did the seam run", never "did the write pass". This harness runs - // no hooks, so the row that would be stored IS `opCtx.data`. - const seam = opCtx.postHookWriteImageCheck; - if (seam) { - seam.honoured = true; - await seam.evaluate([opCtx.data]); - } - if (opCtx.operation === 'insert') await engine.insert(opCtx.object, opCtx.data); - else await engine.update(opCtx.object, opCtx.data, opCtx.options); + // no hooks, so the row an insert would store IS `opCtx.data`. + // [#19989] An update installs the seam too; the double's `update` + // runs it on the stored row, the table's row merged with the payload. + if (opCtx.operation === 'insert') { + const seam = opCtx.postHookWriteImageCheck; + if (seam) { + seam.honoured = true; + await seam.evaluate([opCtx.data]); + } + await engine.insert(opCtx.object, opCtx.data); + } else await engine.update(opCtx.object, opCtx.data, opCtx.options, opCtx); reached = true; }); }); diff --git a/packages/plugins/plugin-security/src/security-denial-user-copy.test.ts b/packages/plugins/plugin-security/src/security-denial-user-copy.test.ts index c2ffc414c45..1f5f9aebf14 100644 --- a/packages/plugins/plugin-security/src/security-denial-user-copy.test.ts +++ b/packages/plugins/plugin-security/src/security-denial-user-copy.test.ts @@ -161,12 +161,23 @@ function makeEngine() { // Both write verbs open with the PRODUCER's dispatch predicate, never a // hand-mirrored guard: a fake looser than `ObjectQL` collects greens from // call shapes the real engine would refuse. - async update(object: string, data: any, options?: any) { + // + // [#19989] `opCtx` is the operation the middleware chain ran on. The engine + // runs an installed write-image check on every update before it writes + // (the rows it will store, each merged with the payload), so this double + // does too: one 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 = 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; }, @@ -251,7 +262,12 @@ async function run(op: OpShape, locale?: string, i18n?: FileI18nAdapter): Promis }; let admitted = false; try { - await securityMw(opCtx, async () => { admitted = true; }); + await securityMw(opCtx, async () => { + // [#19989] An update reaches the engine, which runs the installed + // stored-row check before it writes (see the double's `update`). + if (opCtx.operation === 'update') await engine.update(opCtx.object, opCtx.data, opCtx.options, opCtx); + admitted = true; + }); } catch (e: any) { return { admitted: false, diff --git a/packages/plugins/plugin-security/src/security-plugin.test.ts b/packages/plugins/plugin-security/src/security-plugin.test.ts index 039fc231ca0..e80eff16b33 100644 --- a/packages/plugins/plugin-security/src/security-plugin.test.ts +++ b/packages/plugins/plugin-security/src/security-plugin.test.ts @@ -37,6 +37,13 @@ import { BUILTIN_OPERATION_MESSAGES } from '@objectstack/spec/system'; * 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. + * + * [#19989] A by-id update installs it too, and a real engine hands it the one + * row it writes, merged with the final payload. These doubles hold no row to + * merge onto, so that judgement also runs over none here: the cells in this + * file exercise the middleware's own change-set judgement, which still runs, + * and the stored-row judgement is pinned against the real engine in + * `rls-check-by-id-update-post-hook.test.ts`. */ const runEngineWriteBody = async (opCtx: any): Promise => { const seam = opCtx?.postHookWriteImageCheck; diff --git a/packages/plugins/plugin-security/src/security-plugin.ts b/packages/plugins/plugin-security/src/security-plugin.ts index 9605a221879..07094cc978e 100644 --- a/packages/plugins/plugin-security/src/security-plugin.ts +++ b/packages/plugins/plugin-security/src/security-plugin.ts @@ -12,6 +12,9 @@ import { markFilterSubtreeProvenance, FieldMaskingRuleSchema, type FieldMaskingR // operation-level refusals. Second consumer, same mechanism — a second remedy // for one defect class is what that module exists to prevent. import { renderOperationMessage } from '@objectstack/spec/system'; +// [#19989] The engine's own update-dispatch predicate, asked rather than +// re-derived: step 3.6 needs to know which row the ENGINE will write. +import { resolveEngineUpdateDispatch, type EngineUpdateDispatchData } from '@objectstack/metadata-core'; import { localWriteEpochSource, resolveWriteEpochSource, @@ -208,8 +211,10 @@ export function hasPlatformAdminCapability(held: ReadonlySet): boolean { * 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. + * `beforeInsert` chain has produced it, [#19950] every row a predicate + * (`multi`) update selects, merged with the final payload, and [#19989] the one + * row a by-id update writes, merged with the payload the `beforeUpdate` chain + * left. * * Structurally identical to `OperationContext.postHookWriteImageCheck` in * `@objectstack/objectql`, and deliberately declared here rather than imported: @@ -2988,8 +2993,9 @@ export class SecurityPlugin implements Plugin { // // ── [#16608] WHICH IMAGE, on an INSERT ──────────────────────────────── // - // Both verbs judge THE ROW THAT WILL EXIST. `update` reaches it here, by - // merging the caller's pre-image with the change set. `insert` could not: + // Both verbs judge THE ROW THAT WILL EXIST. `update` has a pre-image to + // merge the change set onto here, though only the change set AS SENT + // (the [#19989] note below says what that missed). `insert` could not: // it has no pre-image, and this middleware runs BEFORE the engine's // operation — so `opCtx.data` is the caller's payload as it arrived, and // the `beforeInsert` hooks that derive the row's real values have not run. @@ -3040,6 +3046,22 @@ export class SecurityPlugin implements Plugin { // 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. + // + // ── [#19989] The BY-ID update, after its hooks ──────────────────────── + // + // The by-id image formed HERE is the caller's pre-image merged with the + // change set as sent, and this middleware runs before `next()` runs the + // `beforeUpdate` chain. A hook that rewrites a checked field (a scoping + // column stamped from a re-pointed parent, a status derived from another + // field) therefore produced a row nobody judged, and it was stored. So a + // by-id update installs the seam too, and the engine runs it on the one + // row it writes, merged with the final payload. + // + // The judgement here STAYS. It refuses a change set whose own values the + // check refuses, including one a hook would have overwritten with a + // value the check admits; dropping it would admit writes refused today, + // and this change only ever refuses more. A by-id update is now judged + // twice: the change set as sent, here, and the stored row, in the engine. if ( (opCtx.operation === 'insert' || opCtx.operation === 'update') && opCtx.data && @@ -3123,9 +3145,11 @@ export class SecurityPlugin implements Plugin { } else { const targetId = this.extractSingleId(opCtx); 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). + // BY-ID UPDATE, the change set AS SENT. Build the image: the + // caller's pre-image merged with the change set (so a check on + // an unchanged field still sees its value). Unchanged by + // [#19989], which adds the stored-row judgement below rather + // than replacing this one (see the block note above). let postImage: Record = { ...(opCtx.data as Record) }; if (this.ql) { // Shares the memoized caller pre-image with the step-3.5 owner @@ -3135,24 +3159,58 @@ export class SecurityPlugin implements Plugin { } 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; + // [#19989] A FALSY payload id (`''`, `0`) is not a row address to + // the engine (`resolveEngineUpdateDispatch`), which then binds a + // truthy scalar `where.id` instead and writes THAT row by id. The + // image above, and every other by-id gate in this middleware, was + // formed from the payload id, so what they judged is not the row + // that would be stored. That write used to reach the driver and be + // refused only afterwards, by the fail-closed guard below (the + // seam was installed for a predicate path the engine never took). + // Now that the engine runs the seam on the by-id path too, that + // guard would no longer fire, so the write is refused HERE, before + // anything runs: one row address, or no write. The question is put + // to the engine's own dispatch predicate, never re-derived. + if (targetId != null && !targetId) { + const engineRoute = resolveEngineUpdateDispatch(opCtx.data as EngineUpdateDispatchData, opCtx.options); + if (engineRoute.kind === 'by-id') { + const developerMessage = + `[Security] Access denied: the update on '${opCtx.object}' carries the payload id ` + + `'${String(targetId)}', which addresses no row, while the engine would write the row ` + + `'${String(engineRoute.id)}' named by where.id — so the row this gate judged is not the row ` + + `that would be stored, and the row-level CHECK cannot vouch for it. Address the row with ` + + `one id: update(object, { id, ...fields }) or update(object, fields, { where: { id } }).`; + ctx.logger.warn(developerMessage, { + operation: opCtx.operation, + object: opCtx.object, + positions, + userId: opCtx.context?.userId ?? 'unknown', + }); + throw new PermissionDeniedError( + userFacingDenialMessage(ctx, 'record_change_not_allowed', opCtx.context?.locale), + { operation: opCtx.operation, object: opCtx.object, positions, permissionSets: explicitPermissionSets }, + developerMessage, + ); + } } + // The stored-row judgement, installed on EVERY update and run by + // the engine once the payload is final (a write it never runs + // fails CLOSED after `next()`): + // + // • [#19989] BY-ID — the one row it writes, merged with the + // payload the `beforeUpdate` chain left; + // • [#19950] PREDICATE — every row the composed AST selects, each + // merged with the final payload. That covers a null id and a + // falsy one the engine routes to its predicate path, which is + // then judged BOTH ways: its by-id image above, and every + // matched row. Without the seam a falsy id would carry a bulk + // update past the per-row check on a change-set-only image. + // + // If no image can be formed the write is not admitted: the engine + // refuses an update it cannot route, and a seam it never runs + // fails CLOSED after `next()`. + writeImageCheckSeam = newWriteImageCheck(); + opCtx.postHookWriteImageCheck = writeImageCheckSeam; } } } @@ -3509,9 +3567,11 @@ export class SecurityPlugin implements Plugin { // [#16608] FAIL CLOSED on a seam that was never run. `honoured` is set by // the engine immediately before it calls the judgement, so an unset flag - // means one thing only: the write went past without the insert `check` - // being evaluated at all — an engine that does not implement the seam, or - // a host that executed the operation itself. The row may already be + // means one thing only: the write went past without its stored-row + // `check` being evaluated at all — an engine that does not implement the + // seam, or a host that executed the operation itself. Every insert and + // every update step 3.6 judges is covered ([#19950], [#19989] by-id + // included). The row may already be // stored, which is exactly why this is LOUD: the alternative is a gate // that silently stops gating and a deployment that never finds out. // ⛔ Do not soften this into a warning: a middleware that cannot say a diff --git a/packages/plugins/plugin-security/src/select-only-write-visibility.test.ts b/packages/plugins/plugin-security/src/select-only-write-visibility.test.ts index a76a8c004bc..43273dada74 100644 --- a/packages/plugins/plugin-security/src/select-only-write-visibility.test.ts +++ b/packages/plugins/plugin-security/src/select-only-write-visibility.test.ts @@ -279,12 +279,23 @@ function makeEngine() { }, // Both write verbs open with the PRODUCER's own dispatch predicate // (#4550 / #5480 / #6277), never a hand-mirrored guard. - async update(object: string, data: any, options?: any) { + // + // [#19989] `opCtx` is the operation the middleware chain ran on. The engine + // runs an installed write-image check on every update before it writes + // (the rows it will store, each merged with the payload), so this double + // does too: one 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 = 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; }, @@ -380,7 +391,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; }); });