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-side RLS check on the row that will be stored, after beforeInsert#16805os-trump wants to merge 5 commits into
check on the row that will be stored, after beforeInsert#16805Conversation
…ill 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
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
…sert-check-post-image
- 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
…untime 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
📓 Docs Drift CheckThis PR changes 2 package(s): 2 hand-written doc(s) NAME something this change touched and may need an implementation-accuracy re-verification:
What this run could not see
Coarse fallback — 26 page(s) merely mention a changed package (the pre-#9192 predicate, kept for the deliberately-wide backstop): Which tree this was computed onThis run read A worktree cut from an older # while this PR is open — GitHub drops the merge commit once it closes
git fetch origin 4d0db65b0eb7b1ab62135f6ef506a397c9d0d03c && git checkout 4d0db65b0eb7b1ab62135f6ef506a397c9d0d03c
# afterwards, rebuild it from the two parents, which stay fetchable
git fetch origin dd2184ae255dde66b66a042da020dcc5e54fa478 cd09d3b99f603e9bfada1ae6fd53cf9ef2fd41a7 && git checkout -B drift-repro dd2184ae255dde66b66a042da020dcc5e54fa478 && git merge --no-ff cd09d3b99f603e9bfada1ae6fd53cf9ef2fd41a7
node scripts/docs-audit/affected-docs.mjs --json dd2184ae255dde66b66a042da020dcc5e54fa478
|
Contract review (
|
Fixes #16608
The insert-side RLS
checkis evaluated on the row that will be stored — afterbeforeInsert— instead of on the caller's raw payload, soinsertandupdatejudge the same thing.Clause-②: yes— the security middleware's accept / refuse behaviour changes.needs:contract-reviewis carried on this PR and on the card. The contract-review tier is unavailable, so this stays in draft; the ruling permits the label as compensation, and nothing below was relaxed on that account.The ruling this implements
Maintainer, 2026-09-07, director seat, summon #17, decision batch #3, verbatim: 「同意」 (issue comment 5572345610).
⛔ Option 2 (keep the order and write the contract that a checked field must arrive from the caller, plus an
os validaterule) is refused, not deferred: it institutionalises the contradiction — the caller sending the value the hook exists to make un-sendable — and adds a permanent rule to keep it. Nothing here drifts back to it.The two measurements the ruling required first
Both were taken on
origin/main@941232040before the evaluation point moved, and posted in full on the card (comment 5579761278). Summary:① The bypass case — IT REPRODUCES. The ruling flagged this as reasoning, not measured. It is now measured, on a real engine with a real
SecurityPluginand a real driver. A caller holdingorg_asendsemployer_org: org_aon a child whose parent belongs toorg_b; the app'srunAs: 'system'stamp reads the parent outside RLS and overwrites the field.{ employer: 'emp_b', employer_org: 'org_a' }{ employer: 'emp_b', employer_org: 'org_b' }{ employer: 'emp_a' }— left to the hook (the card's row 2)PERMISSION_DENIED{ employer: 'emp_a', employer_org: 'org_a' }(the card's row 1)employer_org: 'org_a'Today's order admits a row whose stored scope the caller does not hold. That is a cross-organization write, and it is what regrades the card to
priority:p1. After this PR: row 1 is refused with nothing stored, row 2 is admitted and storesorg_a, row 3 is unchanged.② The census — non-zero, and large: 29 hooks. Reported as a fork, with the list, on the card. Every
beforeInserthook whose derivation reads a caller-supplied field: 4 in this repo (pinyin's__searchcompanion, plugin-auth'ssys_member.rolecanonicalisation, showcase's title normaliser, app-crm's stage to probability), 7 in ats @c3c6526(all of them, allrunAs: 'system'), 18 in hotcrm @d47e37a. The per-hook table is on the card.⛔ Not a silent fallback to option 2. What this PR installs instead of 29 per-hook declarations is one invariant, which is exactly the guard the census was asking for:
That is what makes a hook's input face stop being a security surface. The census's worry was laundering — caller sends X, hook derives checked-Y from X, Y reaches the store unjudged. After this change Y is precisely what is judged. And relative to today it is a strict narrowing in the direction that matters: today the caller can satisfy the insert check by sending any in-scope value in the checked field, because the check reads the payload and the payload is entirely the caller's. Measurement ① is that fact exercised. The insert
checkcurrently offers no guarantee about the stored row; after this it offers exactly that one.Whether the 29 input faces still warrant per-hook declaration work as a follow-up is a PM/maintainer call and is not blocking this PR. My recommendation is on the card.
What changed
@objectstack/objectql—OperationContext.postHookWriteImageCheck. An optional judgement an enforcement layer installs andObjectQL.insertruns once thebeforeInsertchain has produced the row. It sits immediately after the post-hook declared-field door (#13657) and before every producer with a side effect —resolveSystemInsertOrganization,encryptSecretFields(which writes asys_secretrow),applyAutonumbers(which CONSUMES a sequence number), validation, the statement — so a refusal still costs nothing, the same rule #8682 wrote for the door. Rows the declared-field door culled from a partial batch are skipped: they will not be written.honouredis set beforeevaluate, so a throwing check still reads as honoured — the flag answers "did the seam run", never "did the write pass".@objectstack/plugin-security— step 3.6 installs instead of matching. Forinsertthe compiledcheckfilter goes on the operation context; forupdatenothing changes (that path already merges its caller pre-image, and the ADR-0090 D10 delegator half rides along unchanged). The filter is still compiled in the middleware, where the caller's permission sets, the delegator's, the staged membership and this request's context are all resolved. (That staging is what landed for #16607 in PR #16722; it is untouched here.) Only the IMAGE is deferred; deferring the compilation would move authorization inputs into the engine's timeline for no gain. Both verbs share one refusal closure, so a caller cannot tell which side judged.Fail-closed on a seam that never runs. A middleware that installed the judgement and finds
honouredunset refuses the write and logs at ERROR, with a developer message that says the check was not evaluated rather than that it failed — different facts, and an operator debugging one must not be handed the other. ⛔ Deliberately not softened to a warning: a middleware that cannot say a write was checked must not report that it was.Scope kept, not widened. Only objects governed by a permission set that EXPLICITLY declares
check, single-row inserts, non-system caller — the gate's existing scope. Batch inserts were never post-image validated here and still are not.The conformance case
packages/plugins/plugin-security/src/insert-check-post-image.test.ts— one proposition, the scoping field's landing decides, written once and asserted for both verbs on both driver families (driver-sqlbetter-sqlite3:memory:, anddriver-sqlite-wasm):PERMISSION_DENIED/ 403 / the catalog sentence / the developer line), and nothing moved — refusal and non-landing are asserted as separate facts, because a gate that refuses after the row lands is not a gate.The insert arm's out-of-scope cell differs from its in-scope twin by the parent alone: both payloads carry an in-scope
employer_org, so they are indistinguishable to the pre-hook image and no green there can come from it. Plus the card's two rows (bare payload admitted; duplicated stamp still admitted), and the fail-closed leg against an engine double that ignores the seam. 13 cells, all green.Ablation (mutation on disk proven by anchor grep counts plus
git hash-objectagainst the HEAD blob; restore leg proven byte-identical,git diff HEADempty; both packages resolve to SOURCE in this package's vitest config, so no rebuild stands between the edit and the run). Mutating the installed judgement to readopCtx.datainstead of the rows the engine hands it — the defect put back, minimally, with the seam still in place so the fail-closed leg is not what turns the colour:Tests and gates
Measured on the final commit of this branch,
cd09d3b99.pnpm --filter @objectstack/plugin-security testpnpm --filter @objectstack/objectql testtypecheck, both packagesdispatch-gates.mjs --ranreconciliationFour gate families the diff derives were red and are fixed in it:
check:adr-0087-registration(the changeset now carriesnot-required (no-migration-prescription)— an enforcement-ORDER change moves no authorable key, spelling or stored shape),check:engine-double-contract(the fail-closed leg's double routesdelete/update/findOnethrough the real dispatch predicates; the three new pinned seams are recorded),check:test-source-aliaspluscheck:type-source-resolution(the second driver family is read from the producer's SOURCE on both axes — a vitest alias and a bare-key tsconfigpathsrule; measured: thepathsroute adds zero diagnostics from other packages here, so PR #12570's cost did not recur), andcheck:doc-authoring(the tracker id is out of the runtime string and stays in the adjacent comment).Three gates report exit 3 — PREREQUISITE NOT MET, i.e. NOT MEASURED, neither green nor red:
check:i18n,check:type-check-debtandcheck:dual-build-cjs-loadseach need the whole workspace built first. That is a CI-owned run; declared here rather than silently skipped.pnpm lint— a proven narrowing, not a skipped run. Run over the six source files this diff touches:--format jsonreports 6 files linted, 0 errors, 0 warnings. The narrowing is sound because this repo runs oneeslint.config.mjswhich never enables type-aware linting for any file (noparserOptions.project, no typed@typescript-eslintrules — stated and measured with a positive control in that file's own comment at theQUERY_OPTIONS_TEST_GLOBSblock), so nothing in this diff can move the verdict on a file it does not touch. The repo-wide sweep remains CI's.Reviewer's fast path
The whole change is two hunks. In
packages/objectql/src/engine.ts,insert()calls one new optional callback after thebeforeInsertdispatch and before the first producer. Inpackages/plugins/plugin-security/src/security-plugin.tsstep 3.6, theinsertbranch installs that callback where it used to match a filter againstopCtx.data; theupdatebranch is the old code, unmoved. Everything else is the conformance file, the doubles learning to model the engine, and gate bookkeeping.验收备注
beforeUpdatestamp 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 ({ employer: 'emp_b', employer_org: 'org_b' }for a caller holding onlyorg_a). Filed as plugin-security: the UPDATE-side RLScheckpost-image ispre-image + change setevaluated beforebeforeUpdate, so a hook-stamped scoping field can move a row into an organization the caller does not hold #16790. Not fixed here on purpose: the ruling names the insert side and treats the update path as the correct reference, so extending the evaluation point to a second verb is a second accept/refuse change that wants its own decision rather than riding in as a bounded in-place fix. The mechanism this PR adds is directly reusable for it. The conformance file states out loud that it does not claim that route, rather than pinning today's answer there — a test asserting the defective behaviour would advertise a guarantee the runtime does not deliver.check-clause2-carriers --pair 16805reports exit 4 — NOT clause-② legible — and the reason is the claim comment's SHAPE, not a missing declaration. The line is written, correctly and in the fixed spelling, in the PM's claim comment on plugin-security: the insert-side RLScheckpost-image is the raw caller payload evaluated beforebeforeInsert, so a field a hook stamps can never satisfy an insert check — the caller must send the value the hook will overwrite anyway #16608 — but that comment opens## Claim: …, andCLAIM_COMMENT_MARKER(/^\s*>?\s*Claim(?:ed)?\s*:/mi) does not accept a heading-style claim, which the checker's own doc-block says in so many words. So the predicate finds the declaration "on the thread" and not "in the claim comment". Positive control, same document with the two#characters removed from that one line: exit 0, "the clause-② declaration is readable in the fixed spelling and both carriers agree." ⛔ Not corrected from here — the claim comment belongs to the claiming seat, and the checker itself says the declaration IS the judgement and must not be filled in on that seat's behalf. One##removed on that comment makes the pair legible.checkgate has never post-image-validated batch inserts (!Array.isArray(opCtx.data)in its own guard) or predicate (multi: true) updates. Both are pre-existing, both are documented in the gate, and neither moves here.ObjectQL.insertmust run the installed judgement or the middleware refuses. Three call sites in two existing test files were updated.check:engine-double-contractcoversdelete/update/findOnebut not this seam, so a future double will learn about it from a red test rather than from that gate.Authored by Claude Code in session
session_012zTkyNHJ7TkuN2oXtP5x37.