diff --git a/.changeset/insert-check-post-image.md b/.changeset/insert-check-post-image.md new file mode 100644 index 0000000000..33fd97c8e4 --- /dev/null +++ b/.changeset/insert-check-post-image.md @@ -0,0 +1,32 @@ +--- +"@objectstack/plugin-security": minor +"@objectstack/objectql": minor +--- + +fix(plugin-security)!: the insert-side RLS `check` is evaluated on the row that will be STORED — after `beforeInsert` — instead of on the caller's raw payload (#16608) + + + +**BREAKING** — an accept-set narrowing on the write gate's refusal behaviour. An insert that is admitted today can be refused after this change. + +`check` validates the row a write produces — the PostgreSQL `WITH CHECK` analog. `update` reached that row by merging the caller's pre-image with the change set. `insert` could not: it has no pre-image, and the security middleware runs BEFORE the engine's operation, so its post-image was `opCtx.data` — the caller's payload as it arrived, ahead of `applyFieldDefaults` and ahead of every `beforeInsert` hook. + +A denormalised scoping field is exactly what an RLS predicate compares (ADR-0055: a predicate cannot traverse a lookup) and exactly what an app stamps server-side so a caller cannot choose it. Judging the raw payload therefore inverted the policy in both directions, measured on 17.3.0 with a real engine, a real `SecurityPlugin` and both drivers: + +- **the derived value was not on the image**, so the only way to pass a `check` over it was for the caller to SEND the value the hook exists to make un-sendable. Same identity, same object, same second: the payload carrying the stamped field returned 201, the identical payload leaving it to the hook returned 403 — and the stored row was identical either way. +- **the sent value WAS on the image and was then overwritten**, so an insert naming an in-scope organization while pointing at a parent in ANOTHER organization PASSED the check and stored the parent's organization. That is a row whose stored scope the caller does not hold, and it is why this is a narrowing rather than a widening: today it is admitted, after this change it is refused with nothing stored. + +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. + +**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. + +**Two further behaviour changes the reorder produces, measured on both legs** (the reviewed order and this one), because moving the strips ahead of the seam also moves them ahead of the credential channel: + +- a caller-forged value on an author-declared `readonly` **`secret`** field is now stripped. Before, `encryptSecretFields` ran first and replaced the row's value with a `sys_secret` reference, so the strip's `Object.is` value test compared that reference against the caller's plaintext, read the difference as a hook's write, and KEPT the forgery — measured on 17.3.0's order as stored `token: "secret:sec_1"` with a `sys_secret` row minted. This is a narrowing, and it closes a hole that predates this card. +- an empty string on a `readonly` **`password`** field is stripped instead of answering `VALIDATION_ERROR`. `""` reaches the store on neither order, so the 2026-08-13 empty-credential ruling's guarantee is unchanged; only which refusal a caller sees moves, on a payload a caller was never allowed to send. ⚠️ This is the one direction of the reorder that is not a narrowing, and it is recorded rather than left to be discovered. + +**The invariant this buys, stated to its real edge.** A stored row satisfies the insert `check` on every field the CALLER can steer, whatever the caller sent. Nothing offered any such guarantee before: the check read the payload, and the payload was entirely the caller's. + +⚠️ It is deliberately not "on every field", and the difference is a boundary rather than a hedge. Four engine-owned passes still run between the judgement and the driver, and each substitutes a platform value for whatever stands on the row: the tenant fill of an ABSENT organization column (`resolveSystemInsertOrganization` plus the driver's `injectTenantOnInsert`), `encryptSecretFields` replacing a `secret` field's plaintext with a `sys_secret` reference, `applyAutonumbers` issuing a record number, and `normalizeMultiValueFields` coercing a declared multi-value field to its stored shape. A policy whose `check` names an autonumber, a `secret` or the tenant column is therefore judging a value the platform is about to replace. None of those four is caller-steerable — which is exactly why the two passes that WERE (`stripRuntimeOwnedFields` and the static-`readonly` strip) moved above the seam instead of being explained away. diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index 1cead9fce9..ac8fed120c 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -1912,6 +1912,54 @@ export interface OperationContext { * a value that does not parse reads as absent, never as an organization. */ tenantLayer0Verdict?: TenantLayer0Verdict; + /** + * [#16608] The INSERT post-image seam — where an enforcement layer gets to + * judge the row that will actually be stored. + * + * An `insert` has no pre-image, so a middleware's only image of the write is + * `opCtx.data`: the caller's payload as it arrived, before `applyFieldDefaults` + * and before the `beforeInsert` hooks. A value the app DERIVES server-side — + * an organization copied from a parent, a denormalised scoping field a caller + * is deliberately not allowed to choose — is not on that image, so a predicate + * over it judges a value that never lands and ignores the one that does. + * Measured on 17.3.0: the same identity, same object, same second, an insert + * carrying the stamped value was admitted and the identical insert leaving it + * to the hook was refused, while the stored row was identical either way — and + * an insert naming an IN-SCOPE value for a parent in ANOTHER organization was + * admitted with the parent's organization stored on it. + * + * So the enforcement layer installs its judgement here instead of running it + * 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. + * + * ABSENT is the ordinary state — no enforcement layer is mounted, or the + * write is one it does not gate. The engine never invents one. + */ + postHookWriteImageCheck?: PostHookWriteImageCheck; +} + +/** + * [#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). + * + * `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 + * enforcement layer that moved its gate here and finds the flag unset on the + * way out knows its check did not happen, and says so loudly rather than + * reading an unjudged write as an allowed one. + */ +export interface PostHookWriteImageCheck { + evaluate(rows: readonly Record[]): void | Promise; + honoured?: boolean; } /** @@ -10299,6 +10347,253 @@ export class ObjectQL implements IObjectQLEngine { const postRefusal = postHookUndeclared.find((e) => e !== undefined); if (postRefusal) throw postRefusal; } + + // ── [#16608] EVERY VALUE-CHANGING PASS, AHEAD OF THE SEAM ────────── + // + // The two strips below used to run AFTER the seam, and the contract + // review of PR #16805 measured what that cost: a static-`readonly` + // scoping field — the natural shape for a server-stamped column, and + // exactly what an RLS `check` compares (ADR-0055) — was judged by the + // seam with the CALLER’s value still on the row, then stripped and + // re-defaulted, so the store received a value the seam never saw. The + // row that was judged was not the row that was stored, which is this + // card’s own defect one layer down. + // + // So both strips run HERE, ahead of the seam. Neither has a side + // effect — they read `suppliedPerRow`, `rowHookWrittenKeys`, the + // schema and `options`, all resolved above, and they log — so moving + // them costs nothing and buys the seam the final row. What is left + // between the seam and the driver is engine-owned and named in the + // seam’s own comment below. + // + // ⚠️ The REPORTING half (`insertDropped` → `strictReadonlyWrites` / + // `onFieldsDropped`) deliberately stays where it was, after the seam: + // moving it too would put `ReadonlyFieldRejectedError` ahead of the + // gate’s 403 and change which refusal a caller sees. The strips are + // value-changing; their report is not. + const schemaForValidation = this._registry.getObject(object); + // Defaults are already resolved above (pre-hook, #2703); a hook may + // have overridden fields or replaced `input.data` — take its data as-is. + const rows = rowHookContexts.map((rowCtx) => rowCtx.input.data as Record); + // [#8682] Rows the declared-field door refused are already dead on + // arrival — seeded before the strips, before the seam, and so before + // the credential loop further down, which is the first pass with a + // real side effect (`encryptSecretFields` writes a `sys_secret` row): + // a culled row is neither stripped, nor judged, nor able to mint a + // secret for a write that will never happen. + const rowErrors: (unknown | undefined)[] = new Array(rows.length); + for (let i = 0; i < rows.length; i++) rowErrors[i] = undeclaredPerRow[i]; + // [#5503] `autonumber` is RUNTIME-owned: the engine (or the driver's + // persistent sequence) issues the value, so a non-system caller does not + // get to supply or rewrite it. Until now nothing enforced that — a POST + // carrying an explicit record number was stored verbatim, bypassing the + // sequence, and the SQL driver's `supports.autonumber` path adopted it + // too (it only fills a slot left empty). Stripping HERE, in the engine + // and before `applyAutonumbers`, is what makes the fix driver-agnostic: + // every driver — native-sequence or not — is handed a row with no + // caller-supplied record number, so no driver had to change. + // + // Runs BEFORE validation on purpose: a value the caller was never + // allowed to send must not be judged by the object's rules either (a + // `format` rule on the field would otherwise 400 on a payload we are + // about to discard). Symmetric with the UPDATE strip, which likewise + // runs before `evaluateValidationRules`. Exemptions are the update + // path's, unchanged: `isSystem` (seed replay, migration) skips the whole + // pass, and `preserveAudit` (#3493) lets a historical import reinstate + // legacy record numbers. + // + // [#6339] `suppliedPerRow[i]` is handed over WHOLE — values included — + // rather than reduced to its key set. This pass runs after the + // beforeInsert hooks, so a key set could only say "the caller named + // this", and `delete` then took whatever value was standing there: a + // hook that RE-ISSUES the record number lost its write to any caller + // that had also submitted the key, while the same hook's write survived + // on a caller that had not. The update path's twin (#5591). + const insertDropped: string[] = []; + if (!opCtx.context?.isSystem) { + const preserveAudit = opCtx.context?.preserveAudit === true; + for (let i = 0; i < rows.length; i++) { + if (rowErrors[i] !== undefined) continue; + // [#8214] The insert side carries the same claim and the same + // sequencing — this pass logs, the `ReadonlyFieldRejectedError` + // below throws before any driver dispatch. Measured on + // `origin/main`: `driverCreates 0` while the line said the write + // was "COMMITTED WITHOUT IT". The card marked this half UNVERIFIED; + // it reproduces, so the flag is threaded here too. + // [#14259] `hookWrittenKeys` — THIS row's sealed record, the other + // half of the same question `suppliedPerRow[i]` answers. #6339 + // handed values over instead of a key set because "the caller named + // this key" and "this key still holds the caller's value" are + // different facts; the record closes the case values cannot reach, + // where the hook wrote the value the caller also sent. The value + // test stays as the fallback for any row with no record. + const stripped = stripRuntimeOwnedFields( + schemaForValidation as any, rows[i], suppliedPerRow[i] ?? {}, this.logger, + { + preserveAudit, + strictReadonlyWrites: options?.strictReadonlyWrites === true, + hookWrittenKeys: rowHookWrittenKeys[i], + }, + ) as Record; + if (stripped === rows[i]) continue; + for (const k of Object.keys(rows[i])) { + if (!(k in stripped) && !insertDropped.includes(k)) insertDropped.push(k); + } + rows[i] = stripped; + rowHookContexts[i].input.data = stripped; + } + // [#14147] STATIC author-declared `readonly`, enforced HERE — one + // semantics, one enforcement point, per the maintainer ruling of + // 2026-09-03 (option C) which SUPERSEDED the 2026-07-24 row "INSERT + // (all callers) exempt". Until it landed, a non-system caller + // reaching `engine.insert` DIRECTLY wrote a read-only column with no + // refusal, no WARN and no `onFieldsDropped` event, while the very + // same payload through the DataProtocol was stripped — and + // `create_record`'s listener (`@objectstack/service-automation`) was + // wired for a readonly drop it could never receive. The boundary copy + // that produced that asymmetry (`stripReadonlyForInsert`, + // metadata-protocol) is DELETED in the same change rather than kept + // as a second implementation. + // + // The strip is {@link stripReadonlyFields} — the SAME function + // `update` runs, under the SAME `isSystem` gate (the branch above), + // reporting through the SAME channels: `readonlyStripWarning` at + // `warn`, `onFieldsDropped` under reason `readonly`, and + // `strictReadonlyWrites` refusing before any driver dispatch. Its + // guards therefore come across too, and they are wider than the + // deleted ingress copy's: a hook stamp is not caller-supplied + // (`suppliedPerRow`), and a key a `beforeInsert` hook ASSIGNED is the + // hook's write, not a forgery (`rowHookWrittenKeys`, #14259). The + // ingress copy ran BEFORE the hooks and could judge neither. + // + // ⛔ `preserveAudit` is deliberately NOT forwarded — see + // {@link preserveAuditIgnoredOnInsertWarning}: the 2026-08-08 ruling + // narrowed that exemption to the UPDATE path and left `isSystem` as + // the create side's only one. Ruling C moved WHERE this strip runs; + // it did not widen WHAT exempts it. + // + // WHICH fields it may judge is {@link staticReadonlyInsertSubject}'s + // (runtime-owned types belong to the pass above, platform objects to + // their own 403 guards); `null` — no such field on this object — is + // the cheap exit every ordinary insert takes. + const readonlySubject = staticReadonlyInsertSubject(schemaForValidation as any); + if (readonlySubject) { + const preserveAuditIgnored: string[] = []; + for (let i = 0; i < rows.length; i++) { + if (rowErrors[i] !== undefined) continue; + const stripped = stripReadonlyFields( + readonlySubject as any, rows[i], suppliedPerRow[i] ?? {}, this.logger, + { + strictReadonlyWrites: options?.strictReadonlyWrites === true, + hookWrittenKeys: rowHookWrittenKeys[i], + verb: 'insert', + }, + ) as Record; + if (stripped === rows[i]) continue; + const takenFromRow: string[] = []; + for (const k of Object.keys(rows[i])) { + if (k in stripped) continue; + takenFromRow.push(k); + if (!insertDropped.includes(k)) insertDropped.push(k); + if (preserveAudit && !preserveAuditIgnored.includes(k)) preserveAuditIgnored.push(k); + } + // The field's `defaultValue` is RE-DERIVED for every key this + // pass took, which is #3043's stated contract and a guarantee in + // its own right: a forged `approval_status` becomes `draft` — the + // enforced initial state — never NULL, so a stripped forgery + // cannot leave a row in a state the object's own rules + // (`requiredWhen`, the state machine) were written to exclude. + // The deleted ingress copy got this for free by running BEFORE + // `applyFieldDefaults`; a strip that runs after the hooks has to + // ask. Asked over the STRIPPED row, so a `defaultValue` + // expression reads the payload it will really be stored beside, + // and copied back key by key: `applyFieldDefaults` also fills + // every OTHER absent field, and a hook that deliberately wrote + // `null` must keep its null (the first defaults pass, ahead of + // the hooks, is the one that owns those keys). + if (takenFromRow.length > 0) { + const redefaulted = this.applyFieldDefaults(object, stripped, opCtx.context, nowSnap); + for (const k of takenFromRow) { + if (redefaulted[k] !== undefined) stripped[k] = redefaulted[k]; + } + } + rows[i] = stripped; + rowHookContexts[i].input.data = stripped; + } + // One line per CALL, not per row, and only when the exemption was + // ASKED FOR and something was actually removed — the union is + // faithful because the strip is schema-uniform. + if (preserveAuditIgnored.length > 0) { + this.logger.warn(preserveAuditIgnoredOnInsertWarning(object, preserveAuditIgnored)); + } + } + } + + // ── [#16608] The INSERT POST-IMAGE seam ────────────────────────────── + // + // The enforcement layer's write `check` used to be evaluated in its + // middleware, against `opCtx.data` — the caller's payload as it arrived. + // For an `update` that is a merged pre-image, which is the row that will + // exist; for an `insert` it is the row the caller ASKED for, and the + // hooks above have since produced the row that will actually be stored. + // So the judgement is made HERE, on `rowHookContexts[i].input.data` — + // the same objects `rows` is built from below and the driver is handed. + // + // Placement obeys the rule #8682 wrote for the declared-field door and + // #13657 restated for its post-hook half: a refusal must cost nothing. + // This sits after that door and BEFORE every producer — + // `resolveSystemInsertOrganization`, `encryptSecretFields` (which writes + // a `sys_secret` row), `applyAutonumbers` (which CONSUMES a sequence + // number), validation and the statement. + // + // ## What runs between here and the driver — stated, not waved at + // + // The contract review of PR #16805 measured the version of this comment + // that said "nothing between here and the driver adds a value the caller + // could have steered" and then let TWO caller-steerable passes run after + // the seam. Both now run ABOVE (`stripRuntimeOwnedFields` and the static + // `readonly` strip with its re-default), which is why the seam judges the + // row the driver is handed on every key a caller can reach. What is left + // after this point is engine-owned, and it is a CLOSED list rather than a + // reassurance: + // + // - `resolveSystemInsertOrganization` + the driver's `injectTenantOnInsert` + // fill an ABSENT tenant column; an explicit value on the row is left + // alone, and after the strips above that value is a hook's or the + // caller's own organization. (The Layer 0 tenant wall still judges the + // PRE-hook image — filed separately, and the fix's host is this seam.) + // - `encryptSecretFields` replaces a `secret` field's plaintext with a + // `sys_secret` REFERENCE, and `applyAutonumbers` fills an autonumber + // the strips above just guaranteed the caller did not supply. Both + // substitute a platform-owned value for a caller-owned one; a `check` + // naming either field judges the caller's value and the store receives + // the platform's. + // - `normalizeMultiValueFields` coerces a declared multi-value field to + // its stored array shape. + // + // So the invariant this seam buys is stated to its real edge: a stored row + // satisfies the insert `check` on every field the CALLER can steer. ⛔ It + // is not "on every field", and writing it that way is what the review + // caught. A policy whose `check` names an autonumber, a `secret` or the + // tenant column is judging a value the platform is about to replace, and + // that is a boundary rather than a guarantee. + // + // A culled row is skipped rather than judged: it will not be written, so + // refusing it would replace one verdict with another for a row that has + // already lost. `honoured` is set BEFORE `evaluate`, so a throwing check + // still reads as honoured — the flag answers "did the seam run", never + // "did the write pass". + const postHookWriteImageCheck = opCtx.postHookWriteImageCheck; + if (postHookWriteImageCheck) { + postHookWriteImageCheck.honoured = true; + const live: Record[] = []; + for (let i = 0; i < rowHookContexts.length; i++) { + if (undeclaredPerRow[i] !== undefined) continue; + live.push(rowHookContexts[i]!.input.data as Record); + } + await postHookWriteImageCheck.evaluate(live); + } + // Thread the open transaction (if any) into the driver-facing // options so that knex's `.transacting(trx)` is honoured. Without // this, calls inside a `engine.transaction(...)` block would deadlock @@ -10340,13 +10635,9 @@ export class ObjectQL implements IObjectQLEngine { try { let result: any; - const schemaForValidation = this._registry.getObject(object); // When the driver generates autonumbers natively (persistent SQL // sequence), the engine defers to it — see #1603. const driverOwnsAutonumber = (driver as any)?.supports?.autonumber === true; - // Defaults are already resolved above (pre-hook, #2703); a hook may - // have overridden fields or replaced `input.data` — take its data as-is. - const rows = rowHookContexts.map((rowCtx) => rowCtx.input.data as Record); // Partial-success mode (framework#3172, entered via insertMany): a row // that fails validation is culled and reported per-row instead of // aborting the whole batch — so a bulkWrite caller never needs the @@ -10354,13 +10645,6 @@ export class ObjectQL implements IObjectQLEngine { // rows. rowErrors[i] set = row i is dead; only live rows reach the // driver / afterInsert / summaries. const partialMode = partialRowMode; - const rowErrors: (unknown | undefined)[] = new Array(rows.length); - // [#8682] Rows the declared-field door refused are already dead on - // arrival — seeded BEFORE the credential loop below, which is the first - // pass with a real side effect (`encryptSecretFields` writes a - // `sys_secret` row), so a culled row cannot mint a secret for a write - // that will never happen. - for (let i = 0; i < rows.length; i++) rowErrors[i] = undeclaredPerRow[i]; for (let i = 0; i < rows.length; i++) { if (rowErrors[i] !== undefined) continue; try { @@ -10385,151 +10669,6 @@ export class ObjectQL implements IObjectQLEngine { // Locale + translation hooks for the rejection messages (#3957) — // resolved once for the batch, identical for every row. const msgCtx = this.validationMessageContext(object, opCtx.context); - // [#5503] `autonumber` is RUNTIME-owned: the engine (or the driver's - // persistent sequence) issues the value, so a non-system caller does not - // get to supply or rewrite it. Until now nothing enforced that — a POST - // carrying an explicit record number was stored verbatim, bypassing the - // sequence, and the SQL driver's `supports.autonumber` path adopted it - // too (it only fills a slot left empty). Stripping HERE, in the engine - // and before `applyAutonumbers`, is what makes the fix driver-agnostic: - // every driver — native-sequence or not — is handed a row with no - // caller-supplied record number, so no driver had to change. - // - // Runs BEFORE validation on purpose: a value the caller was never - // allowed to send must not be judged by the object's rules either (a - // `format` rule on the field would otherwise 400 on a payload we are - // about to discard). Symmetric with the UPDATE strip, which likewise - // runs before `evaluateValidationRules`. Exemptions are the update - // path's, unchanged: `isSystem` (seed replay, migration) skips the whole - // pass, and `preserveAudit` (#3493) lets a historical import reinstate - // legacy record numbers. - // - // [#6339] `suppliedPerRow[i]` is handed over WHOLE — values included — - // rather than reduced to its key set. This pass runs after the - // beforeInsert hooks, so a key set could only say "the caller named - // this", and `delete` then took whatever value was standing there: a - // hook that RE-ISSUES the record number lost its write to any caller - // that had also submitted the key, while the same hook's write survived - // on a caller that had not. The update path's twin (#5591). - const insertDropped: string[] = []; - if (!opCtx.context?.isSystem) { - const preserveAudit = opCtx.context?.preserveAudit === true; - for (let i = 0; i < rows.length; i++) { - if (rowErrors[i] !== undefined) continue; - // [#8214] The insert side carries the same claim and the same - // sequencing — this pass logs, the `ReadonlyFieldRejectedError` - // below throws before any driver dispatch. Measured on - // `origin/main`: `driverCreates 0` while the line said the write - // was "COMMITTED WITHOUT IT". The card marked this half UNVERIFIED; - // it reproduces, so the flag is threaded here too. - // [#14259] `hookWrittenKeys` — THIS row's sealed record, the other - // half of the same question `suppliedPerRow[i]` answers. #6339 - // handed values over instead of a key set because "the caller named - // this key" and "this key still holds the caller's value" are - // different facts; the record closes the case values cannot reach, - // where the hook wrote the value the caller also sent. The value - // test stays as the fallback for any row with no record. - const stripped = stripRuntimeOwnedFields( - schemaForValidation as any, rows[i], suppliedPerRow[i] ?? {}, this.logger, - { - preserveAudit, - strictReadonlyWrites: options?.strictReadonlyWrites === true, - hookWrittenKeys: rowHookWrittenKeys[i], - }, - ) as Record; - if (stripped === rows[i]) continue; - for (const k of Object.keys(rows[i])) { - if (!(k in stripped) && !insertDropped.includes(k)) insertDropped.push(k); - } - rows[i] = stripped; - rowHookContexts[i].input.data = stripped; - } - // [#14147] STATIC author-declared `readonly`, enforced HERE — one - // semantics, one enforcement point, per the maintainer ruling of - // 2026-09-03 (option C) which SUPERSEDED the 2026-07-24 row "INSERT - // (all callers) exempt". Until it landed, a non-system caller - // reaching `engine.insert` DIRECTLY wrote a read-only column with no - // refusal, no WARN and no `onFieldsDropped` event, while the very - // same payload through the DataProtocol was stripped — and - // `create_record`'s listener (`@objectstack/service-automation`) was - // wired for a readonly drop it could never receive. The boundary copy - // that produced that asymmetry (`stripReadonlyForInsert`, - // metadata-protocol) is DELETED in the same change rather than kept - // as a second implementation. - // - // The strip is {@link stripReadonlyFields} — the SAME function - // `update` runs, under the SAME `isSystem` gate (the branch above), - // reporting through the SAME channels: `readonlyStripWarning` at - // `warn`, `onFieldsDropped` under reason `readonly`, and - // `strictReadonlyWrites` refusing before any driver dispatch. Its - // guards therefore come across too, and they are wider than the - // deleted ingress copy's: a hook stamp is not caller-supplied - // (`suppliedPerRow`), and a key a `beforeInsert` hook ASSIGNED is the - // hook's write, not a forgery (`rowHookWrittenKeys`, #14259). The - // ingress copy ran BEFORE the hooks and could judge neither. - // - // ⛔ `preserveAudit` is deliberately NOT forwarded — see - // {@link preserveAuditIgnoredOnInsertWarning}: the 2026-08-08 ruling - // narrowed that exemption to the UPDATE path and left `isSystem` as - // the create side's only one. Ruling C moved WHERE this strip runs; - // it did not widen WHAT exempts it. - // - // WHICH fields it may judge is {@link staticReadonlyInsertSubject}'s - // (runtime-owned types belong to the pass above, platform objects to - // their own 403 guards); `null` — no such field on this object — is - // the cheap exit every ordinary insert takes. - const readonlySubject = staticReadonlyInsertSubject(schemaForValidation as any); - if (readonlySubject) { - const preserveAuditIgnored: string[] = []; - for (let i = 0; i < rows.length; i++) { - if (rowErrors[i] !== undefined) continue; - const stripped = stripReadonlyFields( - readonlySubject as any, rows[i], suppliedPerRow[i] ?? {}, this.logger, - { - strictReadonlyWrites: options?.strictReadonlyWrites === true, - hookWrittenKeys: rowHookWrittenKeys[i], - verb: 'insert', - }, - ) as Record; - if (stripped === rows[i]) continue; - const takenFromRow: string[] = []; - for (const k of Object.keys(rows[i])) { - if (k in stripped) continue; - takenFromRow.push(k); - if (!insertDropped.includes(k)) insertDropped.push(k); - if (preserveAudit && !preserveAuditIgnored.includes(k)) preserveAuditIgnored.push(k); - } - // The field's `defaultValue` is RE-DERIVED for every key this - // pass took, which is #3043's stated contract and a guarantee in - // its own right: a forged `approval_status` becomes `draft` — the - // enforced initial state — never NULL, so a stripped forgery - // cannot leave a row in a state the object's own rules - // (`requiredWhen`, the state machine) were written to exclude. - // The deleted ingress copy got this for free by running BEFORE - // `applyFieldDefaults`; a strip that runs after the hooks has to - // ask. Asked over the STRIPPED row, so a `defaultValue` - // expression reads the payload it will really be stored beside, - // and copied back key by key: `applyFieldDefaults` also fills - // every OTHER absent field, and a hook that deliberately wrote - // `null` must keep its null (the first defaults pass, ahead of - // the hooks, is the one that owns those keys). - if (takenFromRow.length > 0) { - const redefaulted = this.applyFieldDefaults(object, stripped, opCtx.context, nowSnap); - for (const k of takenFromRow) { - if (redefaulted[k] !== undefined) stripped[k] = redefaulted[k]; - } - } - rows[i] = stripped; - rowHookContexts[i].input.data = stripped; - } - // One line per CALL, not per row, and only when the exemption was - // ASKED FOR and something was actually removed — the union is - // faithful because the strip is schema-uniform. - if (preserveAuditIgnored.length > 0) { - this.logger.warn(preserveAuditIgnoredOnInsertWarning(object, preserveAuditIgnored)); - } - } - } // [#3407 / #5126] This is the strip site both standing notes on // `insert()` pointed at, so both members of `WriteObservabilityOptions` // discharge here — the same one-per-call choice `update` offers, and by diff --git a/packages/plugins/plugin-security/package.json b/packages/plugins/plugin-security/package.json index ab395ff107..18dbc3dff6 100644 --- a/packages/plugins/plugin-security/package.json +++ b/packages/plugins/plugin-security/package.json @@ -28,6 +28,7 @@ }, "devDependencies": { "@objectstack/driver-sql": "workspace:*", + "@objectstack/driver-sqlite-wasm": "workspace:*", "@objectstack/metadata-protocol": "workspace:*", "@objectstack/objectql": "workspace:*", "@objectstack/plugin-sharing": "workspace:*", 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 new file mode 100644 index 0000000000..55940fab01 --- /dev/null +++ b/packages/plugins/plugin-security/src/insert-check-post-image.test.ts @@ -0,0 +1,1001 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#16608] The write `check` judges THE ROW THAT WILL BE STORED — on `insert` + * as on `update`. + * + * ## What was measured, on `origin/main` @ `941232040` (already carrying + * #16607's membership staging, PR #16722) + * + * The app stamps a denormalised scoping field in `beforeInsert` — an + * organization copied from the parent, read OUTSIDE RLS under `runAs: 'system'` + * — precisely so a caller cannot choose it. That is the shape ADR-0055 forces: + * a predicate cannot traverse a lookup, so the field an RLS policy compares is + * the denormalised one, and the denormalised one is what an app stamps. + * + * The security middleware runs BEFORE the engine's operation, so for an insert + * its post-image was `opCtx.data` — the caller's payload as it arrived, before + * `applyFieldDefaults` and before any hook. Both directions were measured, same + * identity, same object, same second: + * + * • the derived value is NOT on that image, so the ONLY way to pass a `check` + * over it was to SEND the value the hook exists to make un-sendable: + * payload WITH the stamped field 201, payload WITHOUT it 403, stored row + * identical either way; + * • the sent value IS on that image and is then overwritten, so an insert + * naming an IN-SCOPE organization while pointing at a parent in ANOTHER + * organization PASSED the check and stored the parent's organization — a + * row whose stored scope the caller does not hold. Measured here as the + * second cell: admitted, `employer_org` stored as `org_b` for a caller + * holding only `org_a`. That is a cross-organization write, and it is why + * this change is a NARROWING with a BREAKING banner rather than a widening. + * + * Ruled 2026-09-07 (maintainer, verbatim 「同意」, director seat, summon #17, + * decision batch #3): the insert post-image becomes the hook-mutated payload. + * ⛔ The refused alternative — keep the order and write the contract that a + * checked field must arrive from the caller, plus an `os validate` rule — is + * refused, not deferred: it institutionalises the contradiction and needs a + * permanent lint to hold it in place. + * + * ## What this file pins + * + * ONE conformance cell, written once and run for BOTH verbs and BOTH driver + * families, because "insert and update judge the same thing" is a property that + * only means something if a single assertion holds on both sides: + * + * IN scope ⇒ admitted, and the STORED row carries the in-scope value; + * OUT of scope ⇒ refused on the ADR-0112 check envelope, and NOTHING moved. + * + * The stamped value is always the parent's, never the caller's, on every cell — + * so a cell can only pass by judging the post-hook row. The insert arm's + * out-of-scope cell carries an IN-scope value in the payload and differs from + * its in-scope twin by the PARENT alone: it is admitted by the pre-hook image + * 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). + * + * 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 + * asserted, because a gate that refuses AFTER the row lands is not a gate. + * + * The seam itself is welded by running both packages together: the plugin + * installs `OperationContext.postHookWriteImageCheck` and the engine calls it. + * Each package spells that member in its own file (plugin-security declares the + * structural type locally — it depends on the engine only as a devDependency), + * so a drift between the two spellings would leave both unit suites green and + * the seam dead. Only a run through both catches it: here, as a silently + * ungated insert. The fail-closed leg below pins the other half — an engine + * that does NOT honour the seam must not have its writes vouched for. + */ + +import { describe, it, expect, afterEach, vi } from 'vitest'; +import { + assertEngineDeleteDispatch, + assertEngineUpdateDispatch, + assertEngineFindOnePredicate, + type EngineFindOneQueryInput, + type EngineUpdateDispatchData, + type EngineUpdateDispatchInput, + type EngineDeleteDispatchInput, +} from '@objectstack/metadata-core'; +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 { BUILTIN_OPERATION_MESSAGES } from '@objectstack/spec/system'; +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 app, transcribed from the card ───────────────────────────────────── + +/** The organization the caller holds. */ +const OWN_ORG = 'org_a'; +/** An organization the caller does NOT hold — the parent's, on the bypass cell. */ +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' }, + }, + }, + // ── the contract review's F1 subject ──────────────────────────────────── + // + // The SAME object, except that the checked scoping field is declared static + // `readonly` — which is not an exotic decoration but the natural shape for a + // column the server stamps and a caller may not choose. That is the whole + // point of the field: ADR-0055 forces the predicate onto the denormalised + // column, and `readonly: true` is how an author says "not yours to send". + // + // It is also what turns `engine.insert`'s static-`readonly` strip into a + // second writer of the checked field, AFTER the middleware has had its say. + { + name: 'qa_ro_member', + label: 'Employer member, readonly scope', + 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', readonly: true }, + role: { name: 'role', type: 'text' }, + }, + }, + // The same again, with a `defaultValue` the caller does NOT hold — the strip's + // re-default (#3043's contract: every key this pass takes is re-derived) then + // puts a FOREIGN organization on the row rather than leaving it empty. + { + name: 'qa_ro_member_default', + label: 'Employer member, readonly scope with a foreign default', + 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', readonly: true, defaultValue: OTHER_ORG }, + role: { name: 'role', type: 'text' }, + }, + }, + // F4 (i): governed by a policy whose `check` cannot be compiled. + { + name: 'qa_unevaluable_member', + label: 'Employer member, unevaluable check', + 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' }, + }, + }, + // F4 (ii): the two producers a refusal must not pay for — a sequence number + // and a `sys_secret` row. + { + name: 'qa_cost_member', + label: 'Employer member, with an autonumber and a secret', + 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' }, + code: { name: 'code', type: 'autonumber', autonumberFormat: 'C-{0000}', format: 'C-{0000}' }, + token: { name: 'token', type: 'secret' }, + }, + }, + // The contract review's F1 reorder moves both strips ahead of the credential + // channel. These two columns are where that is OBSERVABLE rather than argued: + // an author-declared `readonly` on a credential field. + { + name: 'qa_ro_cred', + label: 'Readonly credential columns', + sharingModel: 'public_read_write', + fields: { + id: { name: 'id', type: 'text', primaryKey: true }, + name: { name: 'name', type: 'text' }, + token: { name: 'token', type: 'secret', readonly: true }, + pw: { name: 'pw', type: 'password', readonly: true }, + }, + }, + // The secret store the credential channel writes into, declared here so + // `syncSchemas()` creates a real table for it and "no secret was minted" can + // be READ rather than inferred. + { + name: 'sys_secret', + label: 'Secret', + sharingModel: 'public_read_write', + fields: { + id: { name: 'id', type: 'text', primaryKey: true }, + namespace: { name: 'namespace', type: 'text' }, + key: { name: 'key', type: 'text' }, + kms_key_id: { name: 'kms_key_id', type: 'text' }, + alg: { name: 'alg', type: 'text' }, + version: { name: 'version', type: 'number' }, + ciphertext: { name: 'ciphertext', type: 'text' }, + created_at: { name: 'created_at', type: 'datetime' }, + }, + }, +]; + +/** The real platform baseline, as the app runs under it. */ +const MEMBER_DEFAULT = defaultPermissionSets.find((p) => p.name === 'member_default')!; + +/** + * The card's policy: `using` + `check` twins over a resolver-owned membership + * key. The `check` names the DENORMALISED scoping field — the only shape + * ADR-0055 leaves an author (a predicate cannot traverse `employer`). + */ +const EMPLOYER_ADMIN: PermissionSet = PermissionSetSchema.parse({ + name: 'qa_employer_admin', + objects: { + qa_employer: { allowRead: true, allowCreate: true, allowEdit: true }, + qa_employer_member: { allowRead: true, allowCreate: true, allowEdit: true }, + qa_ro_member: { allowRead: true, allowCreate: true, allowEdit: true }, + qa_ro_member_default: { allowRead: true, allowCreate: true, allowEdit: true }, + qa_unevaluable_member: { allowRead: true, allowCreate: true, allowEdit: true }, + qa_cost_member: { allowRead: true, allowCreate: true, allowEdit: true }, + // No `rowLevelSecurity` policy for this one on purpose: its cells are about + // the ENGINE's pass order, not about the check gate. + qa_ro_cred: { allowRead: true, allowCreate: true, allowEdit: true }, + }, + rowLevelSecurity: [ + { + name: 'employer_admin_members', + 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: 'employer_admin_ro_members', + object: 'qa_ro_member', + operation: 'all', + using: 'record.employer_org in current_user.employer_org_ids', + check: 'record.employer_org in current_user.employer_org_ids', + }, + { + name: 'employer_admin_ro_members_default', + object: 'qa_ro_member_default', + operation: 'all', + using: 'record.employer_org in current_user.employer_org_ids', + check: 'record.employer_org in current_user.employer_org_ids', + }, + { + // The `check` names a `current_user.*` key NO resolver publishes, so the + // compiler cannot evaluate it and drops the policy — which upstream is + // `RLS_DENY_FILTER`, the fail-closed sentinel that matches no row. `using` + // stays evaluable so the read leg is not what is under test. + name: 'employer_admin_unevaluable', + object: 'qa_unevaluable_member', + operation: 'all', + using: 'record.employer_org in current_user.employer_org_ids', + check: 'record.employer_org in current_user.no_such_membership_key', + }, + { + name: 'employer_admin_cost', + object: 'qa_cost_member', + operation: 'all', + using: 'record.employer_org in current_user.employer_org_ids', + check: 'record.employer_org in current_user.employer_org_ids', + }, + ], +}); + +const SYS_CTX = { isSystem: true, userId: 'usr_system' }; +const CALLER = { + userId: 'usr_admin_a', + email: 'admin@a.example', + positions: ['employer_admin'], + permissions: ['qa_employer_admin'], + posture: 'MEMBER', +}; + +// ── the stack ────────────────────────────────────────────────────────────── + +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, not assumed. */ + stampReads: string[]; + /** The stored rows, read past every scope, straight off the driver's table. */ + stored: () => Promise>>; + /** The same, for any table — ground truth for the cells below. */ + table: (name: string, columns: string[]) => Promise>>; + /** How many times the credential channel actually minted a secret. */ + crypto: { encrypt: number; decrypt: number }; +} + +/** + * A reversible stub `ICryptoProvider`, so `encryptSecretFields` runs its REAL + * path — mint a handle, write a `sys_secret` row, put an opaque ref on the row — + * and both halves of "a refusal costs nothing" become readable facts rather than + * an argument about ordering. + */ +function makeFakeCrypto() { + let n = 0; + const calls = { encrypt: 0, decrypt: 0 }; + const provider = { + async encrypt(plain: string) { + calls.encrypt += 1; + n += 1; + return { + id: `sec_${n}`, + kmsKeyId: 'local', + alg: 'test-b64', + version: 1, + ciphertext: Buffer.from(plain, 'utf8').toString('base64'), + }; + }, + async decrypt(handle: { ciphertext: string }) { + calls.decrypt += 1; + return Buffer.from(handle.ciphertext, 'base64').toString('utf8'); + }, + async rotateKey(handle: { version: number }) { return { ...handle, version: handle.version + 1 }; }, + digest(plain: string) { return `d:${plain.length}`; }, + }; + return { provider, calls }; +} + +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.insert-check-post-image-16608', + name: 'Insert check post-image', + version: '1.0.0', + type: 'plugin', + scope: 'system', + objects: OBJECTS, + } as never); + await engine.syncSchemas(); + engines.push(engine); + + // The app's stamp, on BOTH write events — the card's `runAs: 'system'` shape: + // it reads the parent OUTSIDE RLS and overwrites the scoping field from it, + // whatever the caller sent. This is what makes the caller's value on the + // payload a value that never lands. + const stampReads: string[] = []; + const stamp = 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; + }; + for (const object of [ + 'qa_employer_member', + 'qa_ro_member', + 'qa_ro_member_default', + 'qa_unevaluable_member', + 'qa_cost_member', + ]) { + engine.on('beforeInsert', object, stamp as never); + engine.on('beforeUpdate', object, stamp as never); + } + const crypto = makeFakeCrypto(); + engine.setCryptoProvider(crypto.provider as never); + + const resolver = { + keys: ['employer_org_ids'], + resolve: vi.fn(async () => ({ employer_org_ids: [OWN_ORG] })), + }; + const services: Record = { + manifest: { register: vi.fn() }, + objectql: engine, + metadata: { + get: async (_type: string, name: string) => engine.getSchema(name) ?? null, + list: async () => [MEMBER_DEFAULT, EMPLOYER_ADMIN], + }, + [RLS_MEMBERSHIP_RESOLVER_SERVICE]: resolver, + }; + 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 check 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, + ); + + const table = async (name: string, columns: string[]) => { + const driver = (engine as unknown as { getDriver(o: string): { knex: (t: string) => Promise } }) + .getDriver(name); + return (await (driver.knex as unknown as (t: string) => { select: (...c: string[]) => Promise>> })( + name, + ).select(...columns)) as Array>; + }; + + return { + engine, + stampReads, + crypto: crypto.calls, + table, + stored: () => table('qa_employer_member', ['id', 'employer', 'employer_org']), + }; +} + +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, asserted on its ADR-0112 envelope — never on a bare + * `toThrow()`, which a driver throwing a raw `Error` would also satisfy. + */ +function expectCheckDenial(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.message, 'the user half is the localized catalog sentence') + .toBe(BUILTIN_OPERATION_MESSAGES.en.record_change_not_allowed); + expect(outcome.developerMessage, 'the developer half names the gate and the verb') + .toContain(`the ${verb} would violate a row-level CHECK`); +} + +const DRIVERS: Array<[string, () => unknown]> = [ + ['driver-sql (better-sqlite3 :memory:)', () => + new SqlDriver({ client: 'better-sqlite3', connection: { filename: ':memory:' }, useNullAsDefault: true })], + ['driver-sqlite-wasm (:memory:)', () => new SqliteWasmDriver({ filename: ':memory:' })], +]; + +// ── THE CONFORMANCE CELL, one proposition, both verbs, both drivers ──────── + +/** + * ONE proposition — **the scoping field's LANDING decides**: it lands inside the + * caller's scope, the write is admitted and that value is what is stored; it + * lands outside, the write is refused and nothing moves. Written once, asserted + * for both verbs on both drivers, because "insert and update judge the same + * thing" is only worth saying if a single assertion holds on both sides. + * + * Each verb supplies only HOW to reach each landing, and the two routes differ + * for a reason worth stating rather than hiding: + * + * • INSERT reaches the out-of-scope landing through the HOOK. The payload + * carries an IN-scope `employer_org` and differs from the in-scope cell by + * the PARENT alone — so the two payloads are indistinguishable to the + * pre-hook image, and no green here can come from it. This is the measured + * bypass: admitted before this change with `org_b` stored, refused after. + * + * • UPDATE reaches it through the CHANGE SET, on a write that touches no + * parent (so the stamp early-returns and the merged pre-image IS the + * 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. + */ +interface VerbArm { + verb: 'insert' | 'update'; + /** Puts the row in place, inside the caller's scope, before the cell runs. */ + seed: (b: Booted) => Promise; + /** A write whose scoping field LANDS inside the caller's scope. */ + landsInScope: (b: Booted) => Promise; + /** The same write, reaching a landing OUTSIDE the caller's scope. */ + landsOutOfScope: (b: Booted) => Promise; + /** The parent id the stamp must be seen to have read on the out-of-scope cell. */ + outOfScopeStampRead?: string; +} + +const VERBS: VerbArm[] = [ + { + verb: 'insert', + seed: async () => { /* an insert needs no row in place */ }, + landsInScope: (b) => + b.engine.insert( + 'qa_employer_member', + { id: 'mem_1', employer: 'emp_a', employer_org: OWN_ORG, role: 'admin' }, + { context: CALLER } as never, + ), + landsOutOfScope: (b) => + b.engine.insert( + 'qa_employer_member', + { id: 'mem_1', employer: 'emp_b', employer_org: OWN_ORG, role: 'admin' }, + { context: CALLER } as never, + ), + outOfScopeStampRead: 'emp_b', + }, + { + verb: 'update', + // The row starts inside the caller's scope, so the `using` pre-image gate + // lets the caller touch it and the CHECK is the gate under test. + seed: async (b) => + void (await b.engine.insert( + 'qa_employer_member', + { id: 'mem_1', employer: 'emp_a', employer_org: OWN_ORG, role: 'admin' }, + { context: SYS_CTX } as never, + )), + landsInScope: (b) => + b.engine.update( + 'qa_employer_member', + { id: 'mem_1', employer: 'emp_a', role: 'lead' }, + { context: CALLER } as never, + ), + landsOutOfScope: (b) => + b.engine.update( + 'qa_employer_member', + { id: 'mem_1', employer_org: OTHER_ORG, role: 'lead' }, + { context: CALLER } as never, + ), + }, +]; + +for (const [driverName, makeDriver] of DRIVERS) { + for (const arm of VERBS) { + describe(`[#16608] ${arm.verb} on ${driverName} — the check judges the row that will be stored`, () => { + it('the scoping field lands IN scope — admitted, and that value is what is stored', async () => { + const booted = await boot(makeDriver); + await arm.seed(booted); + + const outcome = await attempt(() => arm.landsInScope(booted)); + + expect(outcome.ok, `expected the ${arm.verb} to be admitted: ${outcome.developerMessage ?? outcome.message}`).toBe(true); + // The stamp ran, and it read the parent — recorded, not assumed. + expect(booted.stampReads).toContain('emp_a'); + const rows = await booted.stored(); + expect(rows).toHaveLength(1); + expect(rows[0]!.employer).toBe('emp_a'); + expect(rows[0]!.employer_org, 'the STORED scope is the parent’s, and it is in scope').toBe(OWN_ORG); + }); + + it('the scoping field lands OUT of scope — refused, and nothing moved', async () => { + const booted = await boot(makeDriver); + await arm.seed(booted); + const before = await booted.stored(); + + const outcome = await attempt(() => arm.landsOutOfScope(booted)); + + expectCheckDenial(outcome, arm.verb); + if (arm.outOfScopeStampRead) { + expect(booted.stampReads, 'the stamp ran and read the out-of-scope parent').toContain(arm.outOfScopeStampRead); + } + // Refused AND nothing landed: two facts, both asserted. A gate that + // refuses after the row has moved is not a gate. + const after = await booted.stored(); + expect(after).toEqual(before); + expect( + after.some((r) => r.employer_org === OTHER_ORG), + 'no row may carry an organization the caller does not hold', + ).toBe(false); + }); + }); + } + + describe(`[#16608] ${driverName} — the caller's own value is neither required nor sufficient`, () => { + it('a bare payload, the scoping field left entirely to the hook, is admitted (the card’s row 2)', async () => { + const booted = await boot(makeDriver); + + const outcome = await attempt(() => + booted.engine.insert( + 'qa_employer_member', + { id: 'mem_bare', employer: 'emp_a', role: 'admin' }, + { context: CALLER } as never, + ), + ); + + expect(outcome.ok, `a caller must not have to send the value the hook exists to make un-sendable: ${outcome.developerMessage}`).toBe(true); + const rows = await booted.stored(); + expect(rows.map((r) => r.employer_org)).toEqual([OWN_ORG]); + }); + + it('a payload duplicating the stamp with an in-scope value is still admitted (the card’s row 1 — unchanged)', async () => { + const booted = await boot(makeDriver); + + const outcome = await attempt(() => + booted.engine.insert( + 'qa_employer_member', + { id: 'mem_dup', employer: 'emp_a', employer_org: OWN_ORG, role: 'admin' }, + { context: CALLER } as never, + ), + ); + + expect(outcome.ok).toBe(true); + const rows = await booted.stored(); + expect(rows.map((r) => r.employer_org)).toEqual([OWN_ORG]); + }); + }); +} + +// ── the fail-closed leg: a seam that never runs is not an allowed write ──── + +/** + * The insert judgement is installed on the operation context and executed by + * the engine. That seam has a failure mode the behavioural cells above cannot + * reach, because a real engine always honours it: a HOST that executes the + * operation itself, or an engine that does not implement the member, would + * carry the write past a gate that never ran. + * + * ⛔ The wrong answer is to let it pass and log. This asserts the right one: + * the middleware refuses, on the same ADR-0112 envelope, with a developer half + * that says the check was not evaluated rather than that it failed — the two + * are different facts and an operator debugging one must not be handed the + * other. + */ +describe('[#16608] fail-closed — an engine that does not run the installed check', () => { + it('refuses the insert instead of vouching for it', async () => { + const middlewares: Array<(opCtx: unknown, next: () => Promise) => Promise> = []; + const rows: Array> = []; + const engine = { + registerMiddleware: (mw: (opCtx: unknown, next: () => Promise) => Promise) => middlewares.push(mw), + getSchema: (name: string) => OBJECTS.find((o) => o.name === name), + async find() { return []; }, + // The write verbs route through the real engine's dispatch predicates — + // a double looser than `ObjectQL` turns a green suite into no suite + // (`check:engine-double-contract`, from #4434). This double refuses a + // call the engine refuses even though the leg below never makes one. + async findOne(object: string, query: EngineFindOneQueryInput) { + assertEngineFindOnePredicate(object, query); + return null; + }, + async insert(_object: string, data: Record) { rows.push({ ...data }); return data; }, + async update(_object: string, data: EngineUpdateDispatchData, options?: EngineUpdateDispatchInput | null) { + assertEngineUpdateDispatch(data, options); + return data; + }, + async delete(_object: string, options?: EngineDeleteDispatchInput | null) { + assertEngineDeleteDispatch(options); + return true; + }, + }; + const services: Record = { + manifest: { register: vi.fn() }, + objectql: engine, + metadata: { + get: async (_type: string, name: string) => OBJECTS.find((o) => o.name === name) ?? null, + list: async () => [MEMBER_DEFAULT, EMPLOYER_ADMIN], + }, + [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); + + const opCtx: Record = { + object: 'qa_employer_member', + operation: 'insert', + // An in-scope payload: it would PASS the check. The refusal below is + // therefore about the seam never running, not about the values. + data: { id: 'mem_x', employer: 'emp_a', employer_org: OWN_ORG }, + context: CALLER, + }; + // The engine double never touches `opCtx.postHookWriteImageCheck`. + const outcome = await attempt(() => + middlewares[0]!(opCtx, async () => { + await engine.insert('qa_employer_member', opCtx.data as Record); + }), + ); + + expect(outcome.ok, 'an unjudged 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('without the row-level CHECK being evaluated'); + expect(outcome.developerMessage).toContain('postHookWriteImageCheck'); + // The seam WAS installed — this is a host that ignored it, not a write the + // gate declined to cover. + expect(opCtx.postHookWriteImageCheck, 'the judgement was installed').toBeTruthy(); + expect((opCtx.postHookWriteImageCheck as { honoured?: boolean }).honoured).not.toBe(true); + // Logged at ERROR, not WARN: this is an enforcement outage, not a denial. + expect(ctx.logger.error).toHaveBeenCalled(); + }); +}); + +// ── the contract review's cells, on the same both-drivers footing ────────── + +/** + * [contract review of PR #16805, F1 — BLOCKING] **The row the seam judges must + * be the row that is stored.** + * + * The first delivery of this card put the judgement immediately after the + * post-hook declared-field door — ahead of every producer with a side effect, + * which was the right rule — but two VALUE-CHANGING passes still ran after it: + * `stripRuntimeOwnedFields` and the static-`readonly` strip with its re-default + * (`engine.insert`). For a `readonly` scoping field, which is what an author + * declares precisely so a caller cannot choose it, that left the old defect + * intact one layer down: + * + * caller sends an IN-scope value → the seam judges it and admits → the strip + * removes it (caller-supplied, no hook wrote it) → `applyFieldDefaults` + * re-derives the key → the STORE receives a value the seam never saw. + * + * With no `defaultValue` the stored row simply violates the `check`. With a + * `defaultValue` naming another organization it is the card's own headline + * defect verbatim: a row stored in an organization the caller does not hold. + * + * The fix moves both strips AHEAD of the seam, so the seam judges the row after + * every pass that can change a value a caller could steer and before every pass + * with a side effect. These two cells are what that fix is measured by: they are + * RED on the reviewed head `cd09d3b99` and green after. + * + * ⛔ Note what is NOT asserted: that the caller's value survives. It must not — + * the field is `readonly`. The invariant is the disjunction the review named: + * either the insert is refused, or the STORED row satisfies the check. Both + * cells assert the disjunction over the driver's own table first, and only then + * pin the answer the runtime actually gives. + */ +for (const [driverName, makeDriver] of DRIVERS) { + describe(`[#16608 F1] ${driverName} — a static \`readonly\` scoping field: the seam judges what the STRIP leaves`, () => { + it('no default — the caller’s in-scope value is stripped, so the insert is refused and nothing is stored', async () => { + const booted = await boot(makeDriver); + + // No `employer`, so the stamp early-returns and writes nothing: the only + // author of `employer_org` on this payload is the CALLER, and the strip + // is the only thing that touches it afterwards. + const outcome = await attempt(() => + booted.engine.insert( + 'qa_ro_member', + { id: 'ro_1', employer_org: OWN_ORG, role: 'admin' }, + { context: CALLER } as never, + ), + ); + + const rows = await booted.table('qa_ro_member', ['id', 'employer_org']); + // THE INVARIANT, asserted before any verdict is pinned: a stored row + // satisfies the check, or there is no stored row. + for (const row of rows) { + expect( + row.employer_org, + 'a stored row must satisfy the insert check the seam claims to enforce', + ).toBe(OWN_ORG); + } + // And the answer the runtime gives: refused, on the gate's own envelope. + expectCheckDenial(outcome, 'insert'); + expect(rows, 'nothing was stored').toEqual([]); + }); + + it('a `defaultValue` OUTSIDE the caller’s scope — refused, and no row lands in an organization the caller does not hold', async () => { + const booted = await boot(makeDriver); + + const outcome = await attempt(() => + booted.engine.insert( + 'qa_ro_member_default', + { id: 'rod_1', employer_org: OWN_ORG, role: 'admin' }, + { context: CALLER } as never, + ), + ); + + const rows = await booted.table('qa_ro_member_default', ['id', 'employer_org']); + expect( + rows.some((r) => r.employer_org === OTHER_ORG), + 'the strip’s re-default must not be able to store an organization the caller does not hold', + ).toBe(false); + for (const row of rows) { + expect(row.employer_org, 'a stored row must satisfy the insert check').toBe(OWN_ORG); + } + expectCheckDenial(outcome, 'insert'); + expect(rows, 'nothing was stored').toEqual([]); + }); + }); + + /** + * [contract review F4 (i)] An `check` the compiler cannot evaluate compiles to + * `RLS_DENY_FILTER` — the fail-closed sentinel that matches no row — and the + * seam must REFUSE on it rather than wave the write through. + * + * The claim was argued in the first delivery and never pinned. It is pinned + * here through the seam specifically: the `beforeInsert` stamp is observed to + * have RUN, which it can only have done if the middleware handed the write to + * the engine — so the refusal below is `evaluate`'s, not the middleware's. + */ + describe(`[#16608 F4] ${driverName} — an unevaluable \`check\` refuses`, () => { + it('refuses the insert through the seam, on the ADR-0112 envelope, with nothing stored', async () => { + const booted = await boot(makeDriver); + + const outcome = await attempt(() => + booted.engine.insert( + 'qa_unevaluable_member', + // In-scope in every readable sense: the parent is the caller's own, + // so the stamp lands `org_a`. Only the UNEVALUABLE check refuses it. + { id: 'unev_1', employer: 'emp_a', employer_org: OWN_ORG, role: 'admin' }, + { context: CALLER } as never, + ), + ); + + expectCheckDenial(outcome, 'insert'); + expect( + booted.stampReads, + 'the hook ran, so the write reached the engine and the refusal is the seam’s', + ).toContain('emp_a'); + expect(await booted.table('qa_unevaluable_member', ['id', 'employer_org'])).toEqual([]); + }); + }); + + /** + * [contract review F4 (ii)] **A refusal costs nothing** — the rule #8682 wrote + * for the declared-field door, now owed by this seam because it moved the + * judgement into the engine's own timeline. + * + * Two producers sit downstream of the seam and both are irreversible in the + * way that matters: `applyAutonumbers` (or the driver's native sequence) + * CONSUMES a number, and `encryptSecretFields` MINTS a `sys_secret` row. The + * PR body claimed both are safe. Neither was pinned. + * + * The autonumber half is asserted format-agnostically, against a control boot + * that never refuses anything: if the refused insert had drawn a number, the + * survivor's number would differ from the control's. + */ + describe(`[#16608 F4] ${driverName} — a refusal consumes no autonumber and mints no secret`, () => { + it('the refused insert draws no sequence value and writes no sys_secret row', async () => { + // The control: the very first admitted insert on a fresh engine. + const control = await boot(makeDriver); + await control.engine.insert( + 'qa_cost_member', + { id: 'cost_ctl', employer: 'emp_a', employer_org: OWN_ORG, token: 'sekrit' }, + { context: CALLER } as never, + ); + const baseline = (await control.table('qa_cost_member', ['id', 'code']))[0]!.code; + expect(baseline, 'the control drew a sequence value').toBeTruthy(); + expect(control.crypto.encrypt, 'the control minted its secret — the field IS on the credential path').toBe(1); + + // The subject: one refusal, then the same admitted insert. + const booted = await boot(makeDriver); + const refused = await attempt(() => + booted.engine.insert( + 'qa_cost_member', + // In-scope on the payload, out-of-scope parent — the measured bypass. + { id: 'cost_bad', employer: 'emp_b', employer_org: OWN_ORG, token: 'sekrit' }, + { context: CALLER } as never, + ), + ); + expectCheckDenial(refused, 'insert'); + expect(await booted.table('qa_cost_member', ['id', 'code'])).toEqual([]); + // MINTED NOTHING: the credential channel never ran for the refused row. + expect(booted.crypto.encrypt, 'a refused insert must not mint a secret').toBe(0); + expect(await booted.table('sys_secret', ['id']), 'no sys_secret row for a refused insert').toEqual([]); + + const admitted = await attempt(() => + booted.engine.insert( + 'qa_cost_member', + { id: 'cost_ok', employer: 'emp_a', employer_org: OWN_ORG, token: 'sekrit' }, + { context: CALLER } as never, + ), + ); + expect(admitted.ok, `expected the follow-up insert to be admitted: ${admitted.developerMessage ?? admitted.message}`).toBe(true); + + const survivors = await booted.table('qa_cost_member', ['id', 'code']); + expect(survivors).toHaveLength(1); + // CONSUMED NOTHING: the survivor gets exactly the number the control got, + // so the refused attempt left the sequence where it found it. + expect(survivors[0]!.code, 'the refusal consumed no sequence value').toBe(baseline); + expect(booted.crypto.encrypt, 'exactly one mint, for the one write that happened').toBe(1); + expect(await booted.table('sys_secret', ['id'])).toHaveLength(1); + }); + }); +} + +/** + * [contract review F1, the reorder's OWN consequences — measured, both legs] + * + * Moving the two strips ahead of the seam also moves them ahead of the + * credential loop (`refuseEmptyPasswordFields` + `encryptSecretFields`), which + * used to run first. That is not a detail to wave at: it changes what happens + * to a caller-supplied value on a `readonly` CREDENTIAL column, in two + * different directions, and both were measured on `cd09d3b99` (the reviewed + * head, `engine.ts` checked out over this tree) and on the fix. + * + * ⚠️ These cells run without a `check` policy and outside the RLS gate + * entirely — they are about the ENGINE's pass order, which is what the F1 fix + * moved. They live here because this file is where that reorder is justified. + */ +for (const [credDriverName, makeCredDriver] of DRIVERS) { +describe(`[#16608 F1] ${credDriverName} — what moving the strips ahead of the credential channel changes`, () => { + const OBJ = 'qa_ro_cred'; + // The same non-system caller the cells above use — granted create on this + // object, and governed by NO `check`, so nothing here is the RLS gate's doing. + + it('a caller-forged `readonly` `secret` field is stripped, so nothing is stored and no secret is minted', async () => { + // MEASURED on the reviewed head cd09d3b99, same harness: + // stored `token: "secret:sec_1"` · encrypt calls 1 · sys_secret rows 1 + // The forgery REACHED THE STORE. `encryptSecretFields` ran first and + // replaced the row's value with a reference, so the strip's `Object.is` + // value test then compared a REF against the caller's plaintext, read the + // difference as "a hook rewrote this key", and KEPT it — the one input + // where that test inverts. Pre-existing on 17.3.0, closed by the reorder. + const booted = await boot(makeCredDriver); + + const outcome = await attempt(() => + booted.engine.insert( + OBJ, + { id: 'cred_1', name: 'n', token: 'forged-plaintext' }, + { context: CALLER } as never, + ), + ); + expect(outcome.ok, `expected the insert to be admitted with the forgery dropped: ${outcome.message}`).toBe(true); + + const rows = await booted.table(OBJ, ['id', 'token']); + expect(rows).toHaveLength(1); + expect(rows[0]!.token, 'a caller may not seed a `readonly` credential column').toBeNull(); + expect(booted.crypto.encrypt, 'nothing was encrypted for a value the strip discards').toBe(0); + expect(await booted.table('sys_secret', ['id']), 'and no sys_secret row was minted').toEqual([]); + }); + + it('an empty string on a `readonly` `password` field is stripped rather than refused — and `""` still never reaches the store', async () => { + // ⚠️ THE ONE DIRECTION OF THE REORDER THAT IS NOT A NARROWING, recorded + // here so it is visible rather than discovered. MEASURED on the reviewed + // head cd09d3b99, same harness: + // VALIDATION_ERROR — 'Empty string refused for password field + // "qa_ro_cred.pw"' · nothing stored + // and on the fix: admitted, `pw` stored as NULL. + // + // What the 2026-08-13 ruling guarantees is that a masked credential column + // never holds `""` while every read reports "a password is set". That + // guarantee is INTACT — `""` is discarded on both orders; only which + // refusal a caller sees moved, on a payload the caller was never allowed to + // send. ⛔ The seam's 403 deliberately still precedes this: moving + // `refuseEmptyPasswordFields` up too would let a field-level validation + // verdict answer a write that RLS refuses, which is the wrong precedence + // for a security gate. + const booted = await boot(makeCredDriver); + + const outcome = await attempt(() => + booted.engine.insert(OBJ, { id: 'cred_2', name: 'n', pw: '' }, { context: CALLER } as never), + ); + expect(outcome.ok).toBe(true); + + const rows = await booted.table(OBJ, ['id', 'pw']); + expect(rows).toHaveLength(1); + expect(rows[0]!.pw, 'the empty credential is not stored — the ruling’s guarantee, unchanged').toBeNull(); + }); +}); +} 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 a8e6f3a156..607f2c02f6 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 @@ -295,6 +295,20 @@ async function makeStack(resolver: Resolver | null): Promise { try { await securityMw(opCtx, async () => { await sharingMw(opCtx, async () => { + // [#16608] The engine's own half of the write gate, which this + // executor stands in for: the insert-side RLS `check` is INSTALLED on + // the operation context by the middleware and run by `ObjectQL.insert` + // once the `beforeInsert` chain has produced the row that will be + // stored. A double that skips it models an engine carrying a write + // 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); reached = true; diff --git a/packages/plugins/plugin-security/src/security-plugin.test.ts b/packages/plugins/plugin-security/src/security-plugin.test.ts index ffdf2caecb..7daa709a5f 100644 --- a/packages/plugins/plugin-security/src/security-plugin.test.ts +++ b/packages/plugins/plugin-security/src/security-plugin.test.ts @@ -15,6 +15,30 @@ import type { PermissionSet } from '@objectstack/spec/security'; import { RLS } from '@objectstack/spec/security'; import { BUILTIN_OPERATION_MESSAGES } from '@objectstack/spec/system'; +/** + * [#16608] What the ENGINE does inside the middleware's `next()` — the part of + * `ObjectQL.insert` the doubles in this file stand in for. + * + * Since #16608 the insert-side RLS `check` is not evaluated in the middleware: + * it is INSTALLED on the operation context and run by the engine once the + * `beforeInsert` chain has produced the row that will be stored. A double whose + * executor is a bare `async () => {}` therefore models an engine that carries a + * write past a gate that never ran — and the middleware refuses exactly that, + * fail-closed, rather than vouching for it. A looser double would convert a + * green suite into no suite at all, which is the family `check:engine-double-contract` + * exists for. + * + * 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`. + */ +const runEngineWriteBody = async (opCtx: any): Promise => { + const seam = opCtx?.postHookWriteImageCheck; + if (!seam) return; + seam.honoured = true; + await seam.evaluate([opCtx.data]); +}; + // --------------------------------------------------------------------------- // SecurityPlugin – basic metadata // --------------------------------------------------------------------------- @@ -162,7 +186,7 @@ describe('SecurityPlugin', () => { const drive = async (opCtx: any) => { for (const mw of middlewares) { try { - await mw(opCtx, async () => {}); + await mw(opCtx, () => runEngineWriteBody(opCtx)); } catch { // Another middleware (e.g. the CRUD-authorization one) may refuse a // bare opCtx — irrelevant here: the replay middleware never throws. @@ -287,7 +311,7 @@ describe('SecurityPlugin', () => { ctx, findOne, run: async (opCtx: any) => { - await middleware(opCtx, async () => {}); + await middleware(opCtx, () => runEngineWriteBody(opCtx)); return opCtx; }, }; @@ -3594,7 +3618,7 @@ describe('SecurityPlugin — ADR-0066 D3 field-level requiredPermissions', () => getService: (n: string) => { if (!(n in services)) throw new Error(`service not registered: ${n}`); return services[n]; }, }; const plugin = new SecurityPlugin({ fallbackPermissionSet: fallback }); - return { plugin, ctx, run: async (opCtx: any) => { await middleware(opCtx, async () => {}); return opCtx; } }; + return { plugin, ctx, run: async (opCtx: any) => { await middleware(opCtx, () => runEngineWriteBody(opCtx)); return opCtx; } }; }; it('masks a capability-gated field on read when the caller lacks the capability', async () => { @@ -3834,7 +3858,7 @@ describe('SecurityPlugin — ADR-0090 D10 agent intersection', () => { }; return { ctx, taskFindOne, - run: async (opCtx: any) => { await middleware(opCtx, async () => {}); return opCtx; }, + run: async (opCtx: any) => { await middleware(opCtx, () => runEngineWriteBody(opCtx)); return opCtx; }, }; }; diff --git a/packages/plugins/plugin-security/src/security-plugin.ts b/packages/plugins/plugin-security/src/security-plugin.ts index 81b0e94045..4a6238600f 100644 --- a/packages/plugins/plugin-security/src/security-plugin.ts +++ b/packages/plugins/plugin-security/src/security-plugin.ts @@ -193,6 +193,26 @@ export function hasPlatformAdminCapability(held: ReadonlySet): boolean { return false; } +/** + * [#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. + * + * Structurally identical to `OperationContext.postHookWriteImageCheck` in + * `@objectstack/objectql`, and deliberately declared here rather than imported: + * this package depends on the engine only as a devDependency (the middleware is + * registered through a duck-typed handle), so importing the engine's type would + * put a package that is not a runtime dependency into this package's published + * `.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. */ + evaluate(rows: readonly Record[]): void; + /** Set by the engine immediately before `evaluate` runs. */ + honoured?: boolean; +} + /** * [ADR-0066 D3/⑤] Object `requiredPermissions` normalized into per-CRUD buckets. * `all` holds capabilities required for EVERY operation (the `string[]` form); @@ -1661,6 +1681,13 @@ 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`. + // 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; // [#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 — @@ -2769,6 +2796,42 @@ export class SecurityPlugin implements Plugin { // row that fails the check is DENIED (fail closed, D5) — never silently // written. Scoped to policies that EXPLICITLY declare `check`, so an // object governed only by `using` is unaffected. + // + // ── [#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: + // 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. + // + // A denormalised scoping field is exactly what an RLS predicate compares + // (ADR-0055: a predicate cannot traverse a lookup) and exactly what an app + // stamps in `beforeInsert` so a caller cannot choose it. Judging the raw + // payload therefore inverted the policy on both sides, measured on 17.3.0: + // + // • the value the app derives is NOT on the image, so the only way to + // pass a `check` over it was for the caller to SEND the value the hook + // exists to make un-sendable (same identity, same object, same second: + // payload with the stamped field 201, payload without it 403, and the + // stored row identical either way); + // • the value the caller sent IS on the image and is then overwritten, + // so an insert naming an in-scope organization on a parent belonging + // to ANOTHER organization PASSED the check and stored the parent's + // organization — a row whose stored scope the caller does not hold. + // + // Ruled 2026-09-07 (maintainer, 「同意」): the insert post-image becomes the + // hook-mutated payload — the row that will be stored — so insert and + // update judge the same thing. Mechanically that means the judgement can + // no longer happen HERE for an insert; it is installed on the operation + // context and run by the engine once the `beforeInsert` chain is done + // (`OperationContext.postHookWriteImageCheck`, `@objectstack/objectql`), + // still ahead of every producer and every statement. + // + // ⛔ The alternative — keep the order and write the contract that a + // 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. if ( (opCtx.operation === 'insert' || opCtx.operation === 'update') && opCtx.data && @@ -2791,28 +2854,9 @@ export class SecurityPlugin implements Plugin { : null; const checkParts = [checkFilter, delCheckFilter].filter(Boolean) as Record[]; if (checkParts.length > 0) { - // Build the post-image. Insert → the new row. Update by-id → the - // 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) }; - if (opCtx.operation === 'update') { - 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 (postImage && !checkParts.every((f) => matchesFilterCondition(postImage as any, f as any))) { + // The ONE refusal, shared by both verbs — so an insert judged inside + // the engine and an 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)`, ); @@ -2840,6 +2884,48 @@ export class SecurityPlugin implements Plugin { { operation: opCtx.operation, object: opCtx.object, positions, permissionSets: explicitPermissionSets }, developerMessage, ); + }; + const satisfiesCheck = (image: Record): boolean => + checkParts.every((f) => matchesFilterCondition(image as any, f as any)); + + if (opCtx.operation === 'insert') { + // [#16608] Install the judgement; the engine runs it on the row the + // `beforeInsert` chain produced. The compiled filter is captured + // 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; + } 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 (postImage && !satisfiesCheck(postImage)) denyCheck(); } } } @@ -3210,6 +3296,36 @@ export class SecurityPlugin implements Plugin { await next(); + // [#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 + // 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 + // write was checked must not report that it was. + if (insertCheckSeam && insertCheckSeam.honoured !== true) { + const developerMessage = + `[Security] Access denied: the insert 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?)` — + // the structured fields ride in the THIRD position. There is no `Error` + // to carry here: nothing threw, the seam simply never ran. + ctx.logger.error(developerMessage, undefined, { + 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, + ); + } + // 4. Field-level security: mask restricted fields in returned records. // Covers reads AND the record echoed back by a write — otherwise a caller // with edit-but-not-field-read could PATCH a record and read a diff --git a/packages/plugins/plugin-security/tsconfig.json b/packages/plugins/plugin-security/tsconfig.json index 3c1094dd19..d65a8f7e50 100644 --- a/packages/plugins/plugin-security/tsconfig.json +++ b/packages/plugins/plugin-security/tsconfig.json @@ -25,8 +25,17 @@ // `@objectstack/types/*` subpath, and a `paths` target that matches // nothing on disk would silently fall back to node resolution (see the // rest package's block for the measured traps). + // [#16608] `@objectstack/driver-sqlite-wasm` is imported as a VALUE by + // `src/insert-check-post-image.test.ts`, whose conformance cell runs on two + // driver families. Same rule, same reason and the same bare (star-free) + // key shape as the entry above — an exact match, so it cannot swallow a + // subpath, and this package imports none. It is declared HERE and not in + // `tsconfig.test.json` because a child that declared its own `paths` would + // REPLACE this map rather than merge into it, silently sending + // `@objectstack/types` back to `dist/`. "paths": { - "@objectstack/types": ["../../types/src/index.ts"] + "@objectstack/types": ["../../types/src/index.ts"], + "@objectstack/driver-sqlite-wasm": ["../../drivers/driver-sqlite-wasm/src/index.ts"] } }, "include": [ diff --git a/packages/plugins/plugin-security/vitest.config.ts b/packages/plugins/plugin-security/vitest.config.ts index 8d43c6bfad..f04dc185ea 100644 --- a/packages/plugins/plugin-security/vitest.config.ts +++ b/packages/plugins/plugin-security/vitest.config.ts @@ -71,6 +71,19 @@ export default defineConfig({ find: /^@objectstack\/metadata-protocol$/, replacement: path.resolve(__dirname, '../../metadata-protocol/src/index.ts'), }, + // [#16608] `insert-check-post-image.test.ts` imports `SqliteWasmDriver` + // as a VALUE: its conformance cell runs on two driver families, because + // "the check judges the row that will be stored" is a claim about the + // write gate that must not turn out to depend on which backend a + // deployment happens to run. Same reason as `driver-sql` above — left + // unaliased the specifier resolves through the workspace link to `dist/`, + // a BUILD ARTIFACT, and a dist merely BEHIND runs GREEN against the + // driver's old behaviour while saying nothing (`check:test-source-alias` + // refuses exactly that). + { + find: /^@objectstack\/driver-sqlite-wasm$/, + replacement: path.resolve(__dirname, '../../drivers/driver-sqlite-wasm/src/index.ts'), + }, ], }, }); diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index b3cb1c7551..298700f65e 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -380,7 +380,7 @@ importers: version: 6.0.3 vitest: specifier: ^4.1.11 - version: 4.1.11(@opentelemetry/api@1.9.1)(@types/node@26.2.0)(@vitest/coverage-v8@4.1.11)(happy-dom@20.10.2)(jsdom@30.0.1(@noble/hashes@2.3.0))(msw@2.14.6(@types/node@26.2.0)(typescript@6.0.3))(vite@8.0.16(@types/node@26.2.0)(esbuild@0.28.2)(jiti@2.7.0)(tsx@4.23.12)(yaml@2.9.0)) + version: 4.1.11(@opentelemetry/api@1.9.1)(@types/node@26.2.0)(@vitest/coverage-v8@4.1.11)(happy-dom@20.10.2)(jsdom@30.0.1(@noble/hashes@2.3.0))(msw@2.14.6(@types/node@26.2.0)(typescript@6.0.3))(vite@8.0.16(@types/node@26.2.0)(esbuild@0.28.1)(jiti@2.7.0)(tsx@4.23.12)(yaml@2.9.0)) packages/apps/setup: dependencies: @@ -424,7 +424,7 @@ importers: version: 6.0.3 vitest: specifier: ^4.1.11 - version: 4.1.11(@opentelemetry/api@1.9.1)(@types/node@26.2.0)(@vitest/coverage-v8@4.1.11)(happy-dom@20.10.2)(jsdom@30.0.1(@noble/hashes@2.3.0))(msw@2.14.6(@types/node@26.2.0)(typescript@6.0.3))(vite@8.0.16(@types/node@26.2.0)(esbuild@0.28.1)(jiti@2.7.0)(tsx@4.23.12)(yaml@2.9.0)) + version: 4.1.11(@opentelemetry/api@1.9.1)(@types/node@26.2.0)(@vitest/coverage-v8@4.1.11)(happy-dom@20.10.2)(jsdom@30.0.1(@noble/hashes@2.3.0))(msw@2.14.6(@types/node@26.2.0)(typescript@6.0.3))(vite@8.0.16(@types/node@26.2.0)(esbuild@0.28.2)(jiti@2.7.0)(tsx@4.23.12)(yaml@2.9.0)) packages/cli: dependencies: @@ -1892,6 +1892,9 @@ importers: '@objectstack/driver-sql': specifier: workspace:* version: link:../../drivers/driver-sql + '@objectstack/driver-sqlite-wasm': + specifier: workspace:* + version: link:../../drivers/driver-sqlite-wasm '@objectstack/metadata-protocol': specifier: workspace:* version: link:../../metadata-protocol diff --git a/scripts/engine-double-contract.pinned.json b/scripts/engine-double-contract.pinned.json index 8cf909684b..9a100fabb3 100644 --- a/scripts/engine-double-contract.pinned.json +++ b/scripts/engine-double-contract.pinned.json @@ -2731,6 +2731,21 @@ "verb": "findOne", "pinned": 2 }, + { + "file": "packages/plugins/plugin-security/src/insert-check-post-image.test.ts", + "verb": "delete", + "pinned": 1 + }, + { + "file": "packages/plugins/plugin-security/src/insert-check-post-image.test.ts", + "verb": "findOne", + "pinned": 1 + }, + { + "file": "packages/plugins/plugin-security/src/insert-check-post-image.test.ts", + "verb": "update", + "pinned": 1 + }, { "file": "packages/plugins/plugin-security/src/invitation-placement.test.ts", "verb": "findOne",