Skip to content

Commit a016f08

Browse files
Trumpclaude
andauthored
fix(plugin-security)!: evaluate the insert-side RLS check on the row that will be stored, after beforeInsert (#16805)
* fix(plugin-security): evaluate the insert RLS check on the row that will be stored The security middleware runs before the engine's operation, so for an insert its post-image was the caller's payload as it arrived — ahead of every `beforeInsert` hook. A denormalised scoping field is what an RLS predicate compares (ADR-0055) and what an app stamps server-side so a caller cannot choose it, so the gate judged a value that never lands and ignored the one that does. Measured both ways on 17.3.0: a payload leaving the field to the hook was refused while the identical payload carrying it was admitted, and an insert naming an in-scope organization on a parent in another organization was admitted with the parent's organization stored on it. `OperationContext` gains `postHookWriteImageCheck`, a judgement an enforcement layer installs and `insert()` runs once the hook chain has produced the row — after the post-hook declared-field door, before every producer with a side effect. plugin-security installs its compiled check filter there; the update path, which already merges its pre-image, is unchanged. A seam that was installed and never run refuses the write rather than vouching for it. One conformance cell, both verbs, both drivers: the scoping field's landing decides. Refs #16608, ruling 2026-09-07. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37 * test(plugin-security): engine doubles honour the insert post-image seam The insert-side RLS check is installed on the operation context and run by `ObjectQL.insert`; a double whose executor is a bare `async () => {}` models an engine that carries a write past a gate that never ran, which the middleware refuses fail-closed. The doubles in `security-plugin.test.ts` and `rls-check-membership-staging.test.ts` now run the judgement the way the engine does — flag first, then evaluate — so they model the engine instead of a looser approximation of it. Also fixes the fail-closed log call to the `error(message, error?, meta?)` contract arg order (#5637). Refs #16608. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37 * chore: satisfy the gate families the #16608 diff derives - the changeset carries its ADR-0087 disposition: an enforcement-ORDER change moves no authorable key, spelling or stored shape, so no conversion entry and nothing for an upgrader to hand-edit (check:adr-0087-registration); - the fail-closed leg's engine double routes delete/update/findOne through the real dispatch predicates, so it cannot be looser than ObjectQL (check:engine-double-contract); - `@objectstack/driver-sqlite-wasm` — the conformance cell's second driver family — is read from the producer's SOURCE on both axes: a vitest alias (check:test-source-alias) and a bare-key tsconfig `paths` rule (check:type-source-resolution). Measured: the paths route adds zero diagnostics from other packages here; the test layer still compiles at 0. Refs #16608. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37 * chore: record the new pinned doubles and drop the tracker id from a runtime string `check:engine-double-contract --write` records the three newly-pinned seams in the #16608 conformance file's engine double — new pinned coverage the ledger had not learned about yet. The fail-closed developer message no longer carries the tracker id: a runtime string reaches authors and operators, none of whom can resolve `#NNNN` (check:doc-authoring). The id stays in the adjacent comment, where the reader who can resolve it is already looking. Refs #16608. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37 * fix(objectql): judge the insert post-image after every value-changing pass The contract review of this PR found one residual path where the row the seam judges is not the row that is stored: `stripRuntimeOwnedFields` and the static-`readonly` strip (with its `applyFieldDefaults` re-default) ran AFTER the seam, so a `check` over a `readonly` scoping field judged the caller's in-scope value and the store received the field's `defaultValue` — or NULL. With a default naming another organization that is this card's own headline defect one layer down: a stored row in a scope the caller does not hold. Both strips are side-effect-free, so they move ahead of the seam. The reporting half (`insertDropped` -> `strictReadonlyWrites` / `onFieldsDropped`) deliberately stays where it was, so the gate's 403 still precedes `ReadonlyFieldRejectedError` exactly as before. The seam's own comment now names what still runs between it and the driver as a closed list — the tenant fill, the secret reference, the autonumber, the multi-value normalisation — instead of claiming nothing does. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37 * docs(changeset): state the post-image invariant to its real edge The contract review measured the sentence "a stored row always satisfies the insert `check`, whatever the caller sent" against the code and found it false: two value-changing passes ran after the seam. Those passes have moved above it, so the claim is now true for every field a caller can steer — and it says exactly that, naming the four engine-owned passes (tenant fill, secret reference, autonumber, multi-value normalisation) that still substitute a platform value afterwards. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37 * test(plugin-security): pin the reorder's own consequences on both legs Moving the two strips ahead of the seam also moves them ahead of the credential loop, and that changes what happens to a caller-supplied value on a `readonly` credential column. Both directions are now measured against the reviewed head's `engine.ts` rather than argued: - a caller-forged `readonly` `secret` field was ENCRYPTED AND STORED on cd09d3b (`token: "secret:sec_1"`, one encrypt call, one sys_secret row): the credential channel ran first and replaced the row's value with a reference, so the strip's `Object.is` value test compared a ref against the caller's plaintext, read the difference as a hook write, and kept the forgery. Pre-existing on 17.3.0; closed by the reorder, and now pinned. - an empty string on a `readonly` `password` field answered VALIDATION_ERROR there and is stripped here. The 2026-08-13 ruling's guarantee is intact — `""` reaches the store on neither order — but the refusal a caller sees moves, so it is recorded rather than left to be discovered. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37 * docs(changeset): record the two behaviour changes the strip reorder produces Both measured on the reviewed order and on this one: the `readonly` `secret` forgery that used to survive the strip and reach the store, and the `readonly` `password` empty string whose refusal moves without the value ever reaching the store. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37 * test(plugin-security): run the two consequence cells on both driver families This file's whole thesis is that a claim about the write gate must not turn out to depend on which backend a deployment runs. The two cells the previous commit added ran on one; they now run on both, like the other twenty-three. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_012zTkyNHJ7TkuN2oXtP5x37 --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent e4fd55d commit a016f08

11 files changed

Lines changed: 1552 additions & 185 deletions
Lines changed: 32 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,32 @@
1+
---
2+
"@objectstack/plugin-security": minor
3+
"@objectstack/objectql": minor
4+
---
5+
6+
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)
7+
8+
<!-- adr-0087: not-required (no-migration-prescription) an enforcement-ORDER change: no authorable key, spelling or stored shape moves, so a stored `sys_metadata` row needs no conversion and an upgrader has nothing to hand-edit. What changes is which image the existing `check` predicate is evaluated against; the remedy for a newly-refused insert is to fix the policy or the data, not to migrate metadata. -->
9+
10+
**BREAKING** — an accept-set narrowing on the write gate's refusal behaviour. An insert that is admitted today can be refused after this change.
11+
12+
`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.
13+
14+
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:
15+
16+
- **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.
17+
- **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.
18+
19+
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.
20+
21+
**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.
22+
23+
**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.
24+
25+
**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:
26+
27+
- 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.
28+
- 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.
29+
30+
**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.
31+
32+
⚠️ 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.

0 commit comments

Comments
 (0)