Repository navigation
Commit 009da14
fix(plugin-security, objectql): a row-level check holds for every row of an array insert and a predicate update (#19988)
Fixes #19950
Fixes #19964
Clause-②: no (narrowing)
## What this fixes
A row-level security `check` (declared on the policy, or defaulted from
its `using`) is the write-side half of the policy: a row the check
refuses is never stored (ADR-0058 D4: "on the write pre-image path that
already exists for by-id writes … and on the AST-injected bulk path").
The write gate enforced it for a single-row insert and a by-id update,
and not for the two multi-row write shapes:
- **#19964, array insert.** Step 3.6 excluded an array payload, so no
judgement was installed and every row was stored unjudged, including
under configurations that refuse every single-row insert.
- **#19950, predicate update** (`multi: true`, no row address). Step 3.6
skipped the new-row check and logged "governed by the using-scoped
where". A policy that declares only `check` scopes nothing, and a scoped
`where` says nothing about the new row in any case. The skip was
unconditional, so it also covered a declared `check` that differs from
`using` and a `check` defaulted from `using`.
Both shapes are now judged row by row with the existing refusal
(`PERMISSION_DENIED` / 403, nothing stored). One failing row refuses the
whole write. The judgement is the existing `satisfiesCheck` over
`matchesFilterCondition`: no predicate is compiled differently, and
nothing is lowered into the `where`, so no compile surface moves.
## Landing: two packages, and why the engine is one of them
- `packages/plugins/plugin-security/src/security-plugin.ts`, step 3.6
plus the seam declaration and the post-`next()` fail-closed guard
(generalised from "the insert" to the operation). The by-id update
branch is unchanged. The `explainAccessForCaller` wiring is not touched.
- `packages/objectql/src/engine.ts`, the predicate-update branch of
`update()` (`domain:engine`), plus the seam's doc comments. **Why the
producer is the engine (measured, not assumed):** the rows a predicate
update changes are the rows the middleware-COMPOSED AST selects. That
AST is complete only after every middleware has run: step 3 of this
middleware composes its own scope after step 3.6, and plugin-sharing
composes its editable-rows filter onto the same AST
(`sharing-plugin.ts:1405`), in plugin order. The suggested route,
reading "under the caller's context" in the middleware, was measured two
ways and does not hold:
- A caller-context read applies READ scope and field masking. Where the
read scope is narrower than the write scope, written rows go unjudged
(fail-open). Where it is wider, rows that will not be written get judged
(false refusals). Ablation 3 below shows the second: a judgement that is
not over the actual matched rows falsely refuses the USING-only in-scope
control.
- The engine already holds the exact set: the D7 matched-row read
(`readPriorRows`, bound to the composed AST), which the ruling says is
read once and reused, and which already serves validation, the
`readonlyWhen` strip and both per-row hook phases.
So the security layer installs its judgement on the existing
`OperationContext.postHookWriteImageCheck` seam (the one #19952 built
for inserts), and the engine calls it on the predicate branch. It hands
over every matched row merged with the payload (the same shape as the
per-row `afterUpdate` `result`), placed after `assertNoStrictDrops()`,
where the payload is final. That placement follows the insert seam's
contract review: "the row the seam judges must be the row that is
stored". The readonly strips run earlier on this branch, so a pre-strip
placement would judge values that never land.
## Mechanism hypotheses (dispatch Section 2), as measured on
`2c1011b01b`
1. **Held.** Line 3000 carried `!Array.isArray(opCtx.data)`. Lines
3078-3083 set `postImage = null` for `extractSingleId(opCtx) == null`
and logged "governed by the using-scoped where".
2. **Held.** `engine.ts` (the `postHookWriteImageCheck` call in
`insert()`) hands `evaluate` every live row of an array insert. The
array fix is plugin-side only.
3. **Refined.** The memoized `getCallerPreImage` is by-id and
caller-context, so it is not reusable per row for the reasons above. The
engine's memo serves instead, at no extra read wherever per-row hooks
already read it.
4. **Held.** The skip was unconditional. A cell pins a `using` plus a
differing `check`.
One further hole, found and closed: the middleware treats a falsy scalar
id (`''`, `0`) as a row address, and the engine does not
(`resolveEngineUpdateDispatch`). A falsy payload id therefore carried a
bulk update past the per-row judgement, admitted on a change-set-only
image. The seam is now installed whenever the engine will not treat the
write as addressing one row. The falsy case keeps its by-id judgement
too, so it only refuses more.
## Surface beyond the claim, with reasons
- `packages/objectql/src/engine.ts`: see above (cross-lane,
`domain:engine`).
- Existing plugin-security tests: `check-only-write-scope.test.ts` and
`security-plugin.test.ts` carry engine doubles that must now honour the
seam on a predicate update, the way the real engine does. Otherwise the
fail-closed guard refuses them, which is the intended behaviour. One
test title and comment said step 3.6 "declines to check" the bulk path;
it now says 3.6 can refuse a bulk write but never scope one. That pin
still discriminates a site-1 revert, now by refusal. Two comment-only
edits (`rls-check-defaults-to-using.test.ts`,
`rls-phantom-column-negation.test.ts`) stated the array exclusion as a
fact.
- **A pending release note, corrected in place:
`.changeset/rls-check-defaults-to-using.md`** (from #19952, not yet
released). Its "What does not change" list said bulk updates "are not
checked row by row", which this PR makes false. The bullet now says
their new rows are checked row by row too, by this PR's entry.
`check-empty-changeset` is RED on this by design: it is the DELIBERATE
CORRECTION class (ruling D on #17712). Its prescribed remedy is to keep
the correction and get it confirmed on the PR; restoring the file would
publish a false sentence. Under the landing rule #19970 set, 「DELIBERATE
CORRECTION 红:同 head 达档复核 PASS 记录即确认,⛔ 不等维护者」, the confirmation is a
same-head at-tier review record with a PASS verdict, not the maintainer.
The red is expected until that record is on the PR for the landing head.
## Behaviour that changes (all in the refusing direction)
- A predicate update under a check-only policy is refused when any
matched row's new image fails the check.
- A predicate update that moves a matched row out of a policy's `using`,
when no applicable policy declares `check`, is refused: the defaulted
check, which is the answer the by-id update has given since #19952.
Triage's "已声明 `using` 的策略,行为保持不变" is held as: in-scope bulk updates
under a `using` policy are admitted and scoped exactly as before
(pinned). Only a bulk write that moves rows out of the `using` is newly
refused, on the same terms as by-id. The seat confirmed this reading:
by-id and bulk are two implementations of one operation and must not
disagree (`RowLevelSecurityPolicySchema.check` 「defaults to USING clause
if not specified」; ADR-0058 D4).
- An array insert is refused when any row fails, including every
configuration that already refused each single insert.
- A host that installs the judgement on a predicate update and never
runs it is refused (403, `error` log), as an insert already is.
## Tests
**Patch round 2 (head `3247efeecd`, after merging `origin/main`
`9bfbacbf8b` as merge commit `938b2acfde`):**
`rls-check-multi-row-writes.test.ts` 32 passed (32); plugin-security
suite 123 files, 2348 tests passed; objectql re-run because #19979
touched that package: local project 155 files / 2434 tests + 154 files /
2758 tests, repo project 1 file / 5 tests; `typecheck` green for
objectql and plugin-security (VERDICT command-exit 0 each).
**Patch round 1 (head `9fff66cfdc`, after merging `origin/main`
`3fd3a4f91b` as merge commit `a5ca1db166`):**
`rls-check-multi-row-writes.test.ts` 32 passed (32); plugin-security
suite 123 files, 2348 tests passed (VERDICT command-exit 0 each). The
merge brought no change under `packages/objectql` or
`packages/plugins/plugin-security`, so the objectql suite was not
re-run; its last run is the one below, on a byte-identical `engine.ts`.
**Round 1 (head `6d28dfe98d`):**
New:
`packages/plugins/plugin-security/src/rls-check-multi-row-writes.test.ts`,
32 cells on driver-sql (better-sqlite3) and driver-sqlite-wasm, real
`SecurityPlugin` + `ObjectQL`.
- Failing first, on the unmodified tree: 18 red and 12 green (the
controls). Every negative cell failed as "admitted" (`expected true to
be false`). In the unresolvable-policy cells the single-insert leg was
refused and only the array leg was admitted.
- #19950 cells: the check-only repro; per row not per change set (a
matched row failing on an unchanged field refuses the whole write);
`using` plus a differing `check`; fail-closed (an inner middleware
strips the seam, and the write is refused with "the update on
'qa_ticket' was executed without the row-level CHECK being evaluated");
falsy payload id. Controls: over-fix admit, USING-only in-scope admit
and scoped, `using`+`check` in-scope admit, by-id unchanged, and
USING-only move-out refused on both bulk and by-id.
- #19964 cells: the `[admitted, refused]` repro; over-fix admit; single
insert unchanged; three refuse-every-insert configurations (an
unresolvable sole `using` on `insert` and on `all`, an unresolvable
declared `check`).
- Every refusal asserts `code` `PERMISSION_DENIED`, `status` 403 and the
developer half naming the gate and verb, then reads the stored rows back
under a system context.
Suites: plugin-security 123 files, 2348 tests pass. objectql 309 files,
5182 tests pass (local project in two halves, plus the repo project).
`typecheck` is green for both packages (plugin-security test-layer debt:
0 files, 0 errors).
Ablations: each committed first, mutated through
`scripts/ablation-replace.mjs` (anchor hit 1 to 0, blob changed),
restored with blob equal to HEAD and an empty `git diff HEAD`.
Resolution path: the plugin is imported relatively, and
`@objectstack/objectql` is aliased to `src/index.ts` in this package's
`vitest.config.ts`, so no `dist/` sits between the mutation and the
test.
| # | mutation | result |
|---|---|---|
| 1 | restore the non-array guard for inserts | 8 red: the 4
array-insert negative cells x 2 drivers |
| 2 | never install the seam on a predicate update | 12 red: the 6 bulk
negative cells x 2 |
| 3 | engine judges the payload alone, not the matched rows | 6 red: the
per-row cell, the falsy-id cell, and the USING-only in-scope control
(falsely refused) |
| 4 | install only when the id is null (falsy counts as by-id) | 2 red:
the falsy-id cell, admitted |
| 5 | engine never calls the seam | 16 red: refusals now carry the
not-evaluated message, controls refused |
| 6 | disable the post-`next()` fail-closed guard | 2 red: the
fail-closed cell, admitted |
## Gates
**Patch round 2 (head `3247efeecd`):** the four comment lines this
change rewrote now cite the surviving record, commit `a016f08b8a` (the
insert-side check), instead of a card that answers 404, and say in words
that the original card no longer resolves. `GITHUB_TOKEN="$GH_TOKEN"
node scripts/check-issue-citations.mjs` probes the board and exits 0:
"every citation this change adds resolves (or is a declared cross-repo
reference)", with 9 judged, 9 resolving and 0 unresolved added.
`dispatch-gates --commands` derived the same 68 families from the same 9
paths against merge base `9bfbacbf8`. All 68 were run; `--ran` answers
"68 derived, 68 run, 0 NOT-MEASURED, 0 UNRUN". 67 exit 0.
`check-empty-changeset` exits 1 on
`.changeset/rls-check-defaults-to-using.md` only.
**Patch round 1 (head `9fff66cfdc`):** `dispatch-gates --commands`
derived the same 68 families from the same 9 paths, now against merge
base `3fd3a4f91`. All 68 were run; `--ran` answers "68 derived, 68 run,
0 NOT-MEASURED, 0 UNRUN". 67 exit 0. `check-empty-changeset` exits 1 on
`.changeset/rls-check-defaults-to-using.md` only (the deliberate
correction under "Surface beyond the claim"). The changeset gates the
seat named: `check-adr-0087-registration --base origin/main` exits 0 ("1
declared-breaking changeset(s), each carrying an ADR-0087 disposition"),
`check-changeset-no-major --base origin/main` exits 0, and `pnpm
check:changeset-gate-self-tests` exits 0.
**Round 1 (head `6d28dfe98d`):**
- `node scripts/pm/dispatch-gates.mjs --commands` derived 68 families
from the 9 changed paths. All 68 were run with exit codes recorded.
`--ran` answers "68 derived, 68 run, 0 NOT-MEASURED, 0 UNRUN".
- 67 exit 0. One exits 1: `check-empty-changeset`, the deliberate
correction above.
- Three first answered `PREREQUISITE NOT MET` (exit 3):
`check:dual-build-cjs-loads`, `check:i18n` and `check:type-check-debt`.
They pass after the workspace closure build. `check-engine-split-ratio`
passes after deepening the shallow clone to its window.
- Lint, as a proven narrowing: `eslint --no-inline-config --format json`
over the 7 changed `.ts` files reports 7 files linted, 0 errors and 0
warnings. All 7 fall in the config's `**/*.{ts,…}` block. The config
never enables type-aware linting (`eslint.config.mjs:326-328`), so this
diff cannot move a verdict on an untouched file. The full `pnpm lint` is
CI's.
## Acceptance notes
- **Refusal cost on the predicate path.** The judgement runs where the
payload is final, after the per-row `beforeUpdate` hooks and after the
credential channel (`encryptSecretFields`), which runs above the strips
on this branch. A refused bulk update whose payload carries a `secret`
field has therefore already minted its `sys_secret` row. A validation
refusal two lines below pays the same cost today. Moving the credential
channel below the strips is a separate engine change. A `check` naming a
secret field judges the stored reference.
- **Image timing differs between the two update paths.** A predicate
update is judged on the post-hook image; a by-id update is still judged
in the middleware on the pre-hook change set merged with the
caller-visible pre-image. Where a `beforeUpdate` hook rewrites a checked
field, the bulk path is the stricter of the two. The by-id path is
untouched here.
- **Read cost.** On an object with no per-row hooks, a checked predicate
update now reads its matched rows. The read is unbounded by the per-row
hook ceiling, which applies only when hooks dispatch. On a kernel with
the usual global hooks the read already happens and is shared.
- **Partial-row array insert** (`__partialRowErrors`): a failing row
refuses the whole call rather than being reported per row.
- **Version skew.** A plugin-security built from this change, run over
an engine without the predicate-path call, refuses checked bulk updates
(fail-closed). Both packages carry the changeset.
- `.changeset/19950-rls-check-multi-row-writes.md` is declared a
narrowing, per the seat's ruling and following #19952: `minor` for
`@objectstack/plugin-security` and `@objectstack/objectql`, a `!`
headline, `Clause-②: no (narrowing)`, an ADR-0087 `not-required
(no-migration-prescription)` disposition, and a **BREAKING** paragraph
listing the newly refused writes and the remedy (declare `check` on the
policy, or fix the data).
- `origin/main` was merged twice with merge commits, both clean with no
regeneration owed: at `3fd3a4f91b` (`a5ca1db166`) and at `9bfbacbf8b`
(`938b2acfde`). PR #19984 (#19963, the explain wiring in the same file)
had not landed by the second merge.
- Patch round 2: citation fix only (four comment lines in
`security-plugin.ts`); no behaviour change.
---------
Co-authored-by: Claude <noreply@anthropic.com>1 parent 615c085 commit 009da14
9 files changed
Lines changed: 604 additions & 72 deletions
File tree
- .changeset
- packages
- objectql/src
- plugins/plugin-security/src
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
| 1 | + | |
| 2 | + | |
| 3 | + | |
| 4 | + | |
| 5 | + | |
| 6 | + | |
| 7 | + | |
| 8 | + | |
| 9 | + | |
| 10 | + | |
| 11 | + | |
| 12 | + | |
| 13 | + | |
| 14 | + | |
| 15 | + | |
| 16 | + | |
| 17 | + | |
| 18 | + | |
| 19 | + | |
| 20 | + | |
| 21 | + | |
| 22 | + | |
| 23 | + | |
| 24 | + | |
| 25 | + | |
| 26 | + | |
| 27 | + | |
| 28 | + | |
| 29 | + | |
| 30 | + | |
| 31 | + | |
| 32 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
24 | 24 | | |
25 | 25 | | |
26 | 26 | | |
27 | | - | |
| 27 | + | |
28 | 28 | | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
2224 | 2224 | | |
2225 | 2225 | | |
2226 | 2226 | | |
2227 | | - | |
2228 | | - | |
| 2227 | + | |
| 2228 | + | |
| 2229 | + | |
| 2230 | + | |
| 2231 | + | |
| 2232 | + | |
| 2233 | + | |
| 2234 | + | |
| 2235 | + | |
| 2236 | + | |
2229 | 2237 | | |
2230 | 2238 | | |
2231 | 2239 | | |
| |||
2237 | 2245 | | |
2238 | 2246 | | |
2239 | 2247 | | |
2240 | | - | |
2241 | | - | |
2242 | | - | |
2243 | | - | |
2244 | | - | |
| 2248 | + | |
| 2249 | + | |
| 2250 | + | |
| 2251 | + | |
| 2252 | + | |
| 2253 | + | |
2245 | 2254 | | |
2246 | 2255 | | |
2247 | 2256 | | |
| |||
13222 | 13231 | | |
13223 | 13232 | | |
13224 | 13233 | | |
| 13234 | + | |
| 13235 | + | |
| 13236 | + | |
| 13237 | + | |
| 13238 | + | |
| 13239 | + | |
| 13240 | + | |
| 13241 | + | |
| 13242 | + | |
| 13243 | + | |
| 13244 | + | |
| 13245 | + | |
| 13246 | + | |
| 13247 | + | |
| 13248 | + | |
| 13249 | + | |
| 13250 | + | |
| 13251 | + | |
| 13252 | + | |
| 13253 | + | |
| 13254 | + | |
| 13255 | + | |
| 13256 | + | |
| 13257 | + | |
| 13258 | + | |
| 13259 | + | |
| 13260 | + | |
| 13261 | + | |
| 13262 | + | |
| 13263 | + | |
| 13264 | + | |
| 13265 | + | |
| 13266 | + | |
| 13267 | + | |
| 13268 | + | |
| 13269 | + | |
| 13270 | + | |
| 13271 | + | |
| 13272 | + | |
| 13273 | + | |
| 13274 | + | |
| 13275 | + | |
| 13276 | + | |
| 13277 | + | |
| 13278 | + | |
| 13279 | + | |
| 13280 | + | |
| 13281 | + | |
| 13282 | + | |
| 13283 | + | |
13225 | 13284 | | |
13226 | 13285 | | |
13227 | 13286 | | |
| |||
Lines changed: 21 additions & 8 deletions
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
289 | 289 | | |
290 | 290 | | |
291 | 291 | | |
292 | | - | |
| 292 | + | |
| 293 | + | |
| 294 | + | |
| 295 | + | |
| 296 | + | |
| 297 | + | |
| 298 | + | |
293 | 299 | | |
294 | 300 | | |
295 | 301 | | |
296 | 302 | | |
297 | 303 | | |
| 304 | + | |
| 305 | + | |
| 306 | + | |
| 307 | + | |
| 308 | + | |
298 | 309 | | |
299 | 310 | | |
300 | 311 | | |
| |||
379 | 390 | | |
380 | 391 | | |
381 | 392 | | |
382 | | - | |
| 393 | + | |
383 | 394 | | |
384 | 395 | | |
385 | 396 | | |
| |||
594 | 605 | | |
595 | 606 | | |
596 | 607 | | |
597 | | - | |
598 | | - | |
599 | | - | |
| 608 | + | |
| 609 | + | |
| 610 | + | |
600 | 611 | | |
601 | | - | |
| 612 | + | |
| 613 | + | |
| 614 | + | |
602 | 615 | | |
603 | 616 | | |
604 | 617 | | |
| |||
609 | 622 | | |
610 | 623 | | |
611 | 624 | | |
612 | | - | |
| 625 | + | |
613 | 626 | | |
614 | 627 | | |
615 | 628 | | |
| |||
632 | 645 | | |
633 | 646 | | |
634 | 647 | | |
635 | | - | |
| 648 | + | |
636 | 649 | | |
637 | 650 | | |
638 | 651 | | |
| |||
Lines changed: 1 addition & 1 deletion
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
164 | 164 | | |
165 | 165 | | |
166 | 166 | | |
167 | | - | |
| 167 | + | |
168 | 168 | | |
169 | 169 | | |
170 | 170 | | |
| |||
0 commit comments