Repository navigation
Commit 4463966
fix(plugin-security, objectql)!: a by-id update's row-level check holds for the row it stores (#20012)
Fixes #19989
Clause-②: no (narrowing)
## What this fixes
A row-level security `check` (declared on a policy, or defaulted from
its `using`) is a guarantee about the row that is stored
(`RowLevelSecurityPolicySchema.check`; ADR-0058 D4). An insert and a
predicate update are judged on the row the driver stores, through the
engine-run `OperationContext.postHookWriteImageCheck` seam. A by-id
update was judged only inside the security middleware, on the caller's
pre-image merged with the change set as sent, before `next()` runs the
`beforeUpdate` chain. A value a hook wrote into a checked field after
that point was never judged, so the row could be stored outside the
policy, including in an organization the caller does not hold.
A by-id update is now also judged on the row it stores, with the one
existing refusal (`PERMISSION_DENIED` / 403, nothing stored). The
middleware's existing judgement of the change set as sent stays, so this
change only ever refuses more.
## Landing: two packages
- `packages/objectql/src/engine.ts` (`domain:engine`, declared
cross-seat in the claim). The by-id branch of `update()` runs the
installed seam right after `assertNoStrictDrops()`, the same point the
predicate path uses. By then the `beforeUpdate` chain, the hand-back of
withheld read-only values, both readonly strips and the strict-drop
refusal have run, and nothing below changes a value before
`driver.update`. The image is the prior row (already read under the
not-found gate) merged with the final payload, coerced as the predicate
path's images are. The seam's doc comments now name the by-id path.
- `packages/plugins/plugin-security/src/security-plugin.ts`, step 3.6.
The seam is installed for every update, not only a predicate update. The
by-id change-set judgement is unchanged. The post-`next()` fail-closed
guard therefore covers a by-id update too.
**Why the engine is the producer (measured).** The middleware's only
image of a by-id update is taken before the hooks run. The engine is the
one place that holds the final payload and the prior row together, and
it already runs this seam for the other two write shapes.
**One adjacent edge, closed so that the change never admits more.** The
middleware reads a falsy scalar payload `id` as a row address; the
engine does not, and binds a truthy scalar `where.id` instead. In that
shape the middleware's by-id gates judged a different row than the one
written. Before this change the write reached the driver and was refused
only afterwards, by the fail-closed guard, because the seam had been
installed for a predicate path the engine never took (measured: 403 with
the row already stored). With the engine now running the seam on the
by-id path, that guard would stop firing and the 403 would become a 200.
So step 3.6 asks the engine's own dispatch predicate
(`resolveEngineUpdateDispatch`, from `@objectstack/metadata-core`,
already a runtime dependency) and refuses that shape before `next()`,
with nothing stored. It applies only where a `check` applies, which is
the set the old guard covered.
## Mechanism hypotheses (dispatch Section 2), measured
Base: `origin/main` `e8f163fc3a` at worktree creation. It is one
unrelated commit (service-analytics) after the dispatch's `009da14713`.
1. **Held.** Step 3.6's by-id branch, verbatim on the base: `// BY-ID
UPDATE — unchanged. Build the post-image: the caller's pre-image merged
with the change set`, then `let postImage = { ...(opCtx.data) }`, `if
(pre) postImage = { ...pre, ...(opCtx.data) }`, `if
(!satisfiesCheck(postImage)) denyCheck();`. All of it runs before
`next()`.
2. **Reproduced, both drivers (driver-sql on better-sqlite3,
driver-sqlite-wasm), through the real `SecurityPlugin` and `ObjectQL`.**
- The organization shape: admitted, and the stored row carried an
organization the caller does not hold.
- The simpler shape: admitted, and the stored row carried a value the
check refuses.
- Step 3.7 does not catch the organization shape independently: it
judges only an `organization_id` supplied in the change set as sent. On
the tenant-wall harness, re-pointing to a parent in another tenant is
refused earlier, by the reference check (`VALIDATION_FAILED`), not by
step 3.7. A related Layer 0 reading is handed to the seat as an
out-of-scope finding; it is not fixed here.
3. **Found.** The by-id branch of `update()` has the equivalent point:
after `assertNoStrictDrops()` and before `evaluateValidationRules`. The
predicate path's seam call sits in the same position.
## Tests (code head `7efc9eb010`; round 2 head `0474ea38f3`)
New:
`packages/plugins/plugin-security/src/rls-check-by-id-update-post-hook.test.ts`,
24 cells (12 per driver). It uses the real `SecurityPlugin` and
`ObjectQL` on both SQL drivers. Every refusal asserts `code`
`PERMISSION_DENIED` and `status` 403 plus the gate's developer half,
then reads the stored rows straight off the driver's table.
- The negative cells: the organization shape; the simpler shape; a
hook-closed row failing on a field the update never carried (the stored
row, not the change set); fail-closed on a host that strips the
installed seam; a falsy payload `id` beside a truthy `where.id` (for
both `''` and `0`).
- The controls: an in-scope by-id update is admitted and stored; a hook
that touches no checked field is unchanged; the change set as sent is
still refused; a change set the check refuses stays refused when a hook
would replace it with an admitted value; the predicate twin gives the
same answer; a falsy payload `id` with no `where.id` is still a judged
predicate update.
- **Failing first.** The file was committed first (`36f1dff9d6`). Then
both source files were set back to the base blobs and restored from
`HEAD` (blobs equal HEAD, `git diff HEAD` empty). The run gave 12 red
(the six negative cells × two drivers) and 12 green controls.
Ablations were run on the final head, and the counts were identical on
the pre-merge head `414909be35`. Each is one mutation through
`scripts/ablation-replace.mjs`: the anchor hit 1 → 0 and the blob
changed, then the file was restored with blob == HEAD and an empty `git
diff HEAD`. On resolution, 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 a mutation and the run.
| # | mutation | red |
|---|---|---|
| A1 | the engine never runs the seam on the by-id path | 10: the three
by-id negative cells (organization, plain, unchanged field) are refused
only by the fail-closed guard, after the row is stored, and the two
admitted controls are refused, × 2 |
| A2 | the plugin installs the seam only for a predicate update (the old
condition) | 8: org, plain, unchanged-field, fail-closed, × 2 |
| A3 | the by-id change-set judgement dropped | 2: the only-refuses-more
cell, × 2 |
| A4 | the falsy-id refusal dropped | 4: both falsy-id cells, × 2 |
| A5 | the engine's image is the payload alone, not merged with the
prior row | 2: the unchanged-field cell, × 2 |
| A6 | the post-`next()` fail-closed guard dropped | 2: the fail-closed
cell, × 2 |
| A7 | the engine never runs the seam on the predicate path | 4: the
predicate twin and the falsy-id predicate control, × 2 |
Suites on `7efc9eb010`, after merging `origin/main`, with the closure
rebuilt:
- plugin-security: 128 files, 2501 tests passed.
- objectql local project: two halves, 156 files / 2454 tests and 155
files / 2784 tests, all passed.
- objectql repo project: 1 file / 5 tests.
- Round 2, on `0474ea38f3`: plugin-auth 114 files / 2439 tests passed
(CI Test Core 6/6 had 8 red in `sys-user-self-service-route.test.ts` on
`7efc9eb010`; see the surface section); runtime 277 files / 3911 passed,
1 skipped; plugin-dev 8 files / 80 passed; plugin-auth `typecheck` exit
0 (test-layer ledger held at 10 files / 94 errors).
- Round 2, real engine: a member updating their OWN `sys_user` row (real
`ObjectQL` on driver-sql, the real `SysUser` schema, the shipped
permission sets, the real identity write guard) is admitted and stored
for `locale` and `name`, with the seam installed and honoured. The peer
row is refused by the by-id pre-image gate, and `role` by the identity
guard. So the plugin-auth red was the harness, not the product path.
- `typecheck` green for both packages. The plugin-security test layer is
at 0 files / 0 errors; objectql holds its ledger unchanged at 40 files /
234 errors.
## Gates
- `node scripts/pm/dispatch-gates.mjs --repo objectstack-ai/objectstack`
derived 69 families from this diff on `0474ea38f3` (68 on `7efc9eb010`,
plus `check-dev-prereqs --self-test` for the plugin-auth file). All 69
ran and `--ran` reconciled 69 derived / 69 run / 0 NOT-MEASURED. 68 exit
0; `check-empty-changeset` exits 1 by design (see Deliberate
correction). That list is a superset of the dispatch-time list; the
additions include `check:engine-double-contract`,
`check:adr-0087-registration`, `check:empty-changeset`,
`check:durability-log-level` and the type-check ratchets.
- `check:dual-build-cjs-loads`, `check:i18n` and `check:type-check-debt`
first answered PREREQUISITE NOT MET (exit 3) and were re-run green after
a full package-closure build.
- The board-probing `GITHUB_TOKEN=… node
scripts/check-issue-citations.mjs` passed: 15 citations resolve. The
dead tracker the in-tree note cited is removed, and is referred to in
words.
- Narrowed lint: `eslint --no-inline-config --format json` over the 13
touched `.ts` files gave 13 files, 0 errors and 0 warnings. Three facts
make it a full measurement for these files. The population is the
`**/*.{ts,…}` and `packages/**` blocks of `eslint.config.mjs`. The
count, 13, comes from the JSON output. The config never enables
type-aware linting (no `parserOptions.project`, no typed rules), so this
diff cannot move the verdict for any untouched file. The repo-wide `pnpm
lint` is left to CI.
## Surface beyond the claim, with reasons
- Eight existing plugin-security test files. Their engine doubles now
run the stored-row check on a by-id update the way the real engine does;
otherwise the fail-closed guard refuses them, which is the intended
behaviour. The files are `authored-row-write-verdict`,
`authz-matrix-gate`, `check-only-write-scope`,
`controlled-by-parent-detail-write-authority`,
`controlled-by-parent-master-widener`, `rls-check-membership-staging`
(its double had judged the payload alone), `security-denial-user-copy`
and `select-only-write-visibility`. `security-plugin.test.ts` changes a
comment only.
- `insert-check-post-image.test.ts`: the in-tree note that recorded this
gap as open now points at the new pins.
- Round 2:
`packages/plugins/plugin-auth/src/sys-user-self-service-route.test.ts`.
Its terminal `next()` stood in for the engine without running the seam,
so the middleware's fail-closed guard refused every own-row write the
file pins as admitted. The failing run's developer message was the
guard's own ("the update on 'sys_user' was executed without the
row-level CHECK being evaluated"), not the check refusal. The terminal
now runs the seam on the fixture row merged with the payload, through
the producer's dispatch predicate, without adding a read to the recorded
pre-image wheres. The census of `SecurityPlugin` hosts outside its own
package found one other hand-made engine double,
`runtime/src/domains/share-links-enforcement-context.test.ts`. It passes
unchanged (runtime suite green), so it is untouched.
- Round 2: the two pending changesets named under Deliberate correction.
- A merge of `origin/main` brought PR #19728, which touched both files.
It left one conflict in `engine.ts`, at this exact point: the seam and
#19728's related-record binding for validation both follow
`assertNoStrictDrops()`. The resolution keeps both, with the seam first,
as on the predicate path.
## Behaviour that changes (all in the refusing direction)
- A by-id update whose `beforeUpdate` chain leaves a checked field at a
value the check refuses is refused, and nothing is stored.
- A by-id update on a host that installs the judgement and never runs it
is refused (403, `error` log), as inserts and predicate updates already
are.
- An update whose payload `id` addresses no row while `where.id`
addresses one, under a policy with a `check`, is refused before anything
runs. It used to be written and then refused.
## Deliberate correction
This PR rewrites one sentence in each of two pending release notes it
did not add. Both sentences read as release-level claims, and this PR
makes them false in a release that ships it. Every other byte of both
files is unchanged (reversing each replacement reproduces the base blob
exactly).
- `.changeset/19950-rls-check-multi-row-writes.md`, under "What does not
change".
- Old: "A by-id update and a single-row insert are judged exactly as
before."
- New: "A single-row insert is judged exactly as before. A by-id update
is not changed by this entry; its judgement on the row it stores is its
own entry (#19989)."
- Why: this PR changes how a by-id update is judged, so "judged exactly
as before" would be false in the release.
- `.changeset/insert-check-post-image.md`, under "What changed,
mechanically".
- Old clause: "`update` is unchanged."
- New clause: "this entry leaves `update` unchanged, and the predicate
and by-id updates move onto the same seam in their own entries (#19950,
#19989)."
- Why: the by-id update now uses the same seam, so a bare "`update` is
unchanged" would be false in the release.
`check-empty-changeset` goes red on this PR, and that is expected. It
refuses any change to a changeset present on the merge base, and names
both files with the DELIBERATE CORRECTION class. Its remedy for that
class is to keep the correction and have it confirmed on the PR, not to
restore the old text. Local run on `0474ea38f3`: exit 1, "This PR
changes a changeset it did not add", listing both files as "present on
the merge base and CHANGED by this PR".
## Acceptance notes
- The change-set judgement is kept, so that this change only refuses
more. As a consequence, a change set the check refuses as sent stays
refused even when a hook would overwrite the refused value with one the
check admits (pinned). Dropping it would match the insert's ruled "judge
the stored row only" semantics, but it would widen, so it is a separate
decision.
- The seam's position is pinned relative to the hooks: the refusal cells
can only pass on post-hook values. Its position relative to the readonly
strips is not separately pinned on this path.
- Ablation A3 reading: the "change set as sent still refuses" cell stays
green without the middleware judgement, because the seam refuses the
same value after the hooks. It is a behaviour control, not a pin of the
middleware judgement; the only-refuses-more cell is that pin.
- The doubles in the two controlled-by-parent harnesses and in
`authz-matrix-gate` model only the by-id path, through the producer's
dispatch predicate. The `security-plugin.test.ts` double holds no table
and judges no rows on an update; this is documented there.
---------
Co-authored-by: Claude <noreply@anthropic.com>1 parent a0920b4 commit 4463966
17 files changed
Lines changed: 823 additions & 71 deletions
File tree
- .changeset
- packages
- objectql/src
- plugins
- plugin-auth/src
- plugin-security/src
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
26 | 26 | | |
27 | 27 | | |
28 | 28 | | |
29 | | - | |
| 29 | + | |
30 | 30 | | |
31 | 31 | | |
32 | 32 | | |
| 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 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
18 | 18 | | |
19 | 19 | | |
20 | 20 | | |
21 | | - | |
| 21 | + | |
22 | 22 | | |
23 | 23 | | |
24 | 24 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
2235 | 2235 | | |
2236 | 2236 | | |
2237 | 2237 | | |
2238 | | - | |
2239 | | - | |
| 2238 | + | |
| 2239 | + | |
| 2240 | + | |
| 2241 | + | |
| 2242 | + | |
| 2243 | + | |
2240 | 2244 | | |
2241 | 2245 | | |
2242 | 2246 | | |
| |||
2253 | 2257 | | |
2254 | 2258 | | |
2255 | 2259 | | |
2256 | | - | |
| 2260 | + | |
| 2261 | + | |
2257 | 2262 | | |
2258 | 2263 | | |
2259 | 2264 | | |
| |||
13420 | 13425 | | |
13421 | 13426 | | |
13422 | 13427 | | |
| 13428 | + | |
| 13429 | + | |
| 13430 | + | |
| 13431 | + | |
| 13432 | + | |
| 13433 | + | |
| 13434 | + | |
| 13435 | + | |
| 13436 | + | |
| 13437 | + | |
| 13438 | + | |
| 13439 | + | |
| 13440 | + | |
| 13441 | + | |
| 13442 | + | |
| 13443 | + | |
| 13444 | + | |
| 13445 | + | |
| 13446 | + | |
| 13447 | + | |
| 13448 | + | |
| 13449 | + | |
| 13450 | + | |
| 13451 | + | |
| 13452 | + | |
| 13453 | + | |
| 13454 | + | |
| 13455 | + | |
| 13456 | + | |
| 13457 | + | |
| 13458 | + | |
| 13459 | + | |
| 13460 | + | |
| 13461 | + | |
| 13462 | + | |
13423 | 13463 | | |
13424 | 13464 | | |
13425 | 13465 | | |
| |||
13629 | 13669 | | |
13630 | 13670 | | |
13631 | 13671 | | |
13632 | | - | |
13633 | | - | |
13634 | | - | |
| 13672 | + | |
| 13673 | + | |
| 13674 | + | |
| 13675 | + | |
| 13676 | + | |
13635 | 13677 | | |
13636 | 13678 | | |
13637 | 13679 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
61 | 61 | | |
62 | 62 | | |
63 | 63 | | |
| 64 | + | |
64 | 65 | | |
65 | 66 | | |
66 | 67 | | |
| |||
204 | 205 | | |
205 | 206 | | |
206 | 207 | | |
| 208 | + | |
| 209 | + | |
| 210 | + | |
| 211 | + | |
| 212 | + | |
| 213 | + | |
| 214 | + | |
| 215 | + | |
| 216 | + | |
| 217 | + | |
| 218 | + | |
| 219 | + | |
| 220 | + | |
| 221 | + | |
| 222 | + | |
| 223 | + | |
| 224 | + | |
| 225 | + | |
| 226 | + | |
| 227 | + | |
| 228 | + | |
| 229 | + | |
207 | 230 | | |
208 | 231 | | |
209 | | - | |
| 232 | + | |
210 | 233 | | |
211 | 234 | | |
212 | 235 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
231 | 231 | | |
232 | 232 | | |
233 | 233 | | |
234 | | - | |
| 234 | + | |
| 235 | + | |
| 236 | + | |
| 237 | + | |
| 238 | + | |
| 239 | + | |
| 240 | + | |
235 | 241 | | |
236 | 242 | | |
237 | 243 | | |
238 | 244 | | |
239 | 245 | | |
| 246 | + | |
| 247 | + | |
| 248 | + | |
| 249 | + | |
| 250 | + | |
240 | 251 | | |
241 | 252 | | |
242 | 253 | | |
| |||
303 | 314 | | |
304 | 315 | | |
305 | 316 | | |
306 | | - | |
| 317 | + | |
307 | 318 | | |
308 | 319 | | |
309 | 320 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
46 | 46 | | |
47 | 47 | | |
48 | 48 | | |
| 49 | + | |
49 | 50 | | |
50 | 51 | | |
51 | 52 | | |
| |||
154 | 155 | | |
155 | 156 | | |
156 | 157 | | |
157 | | - | |
| 158 | + | |
| 159 | + | |
| 160 | + | |
| 161 | + | |
| 162 | + | |
| 163 | + | |
| 164 | + | |
| 165 | + | |
| 166 | + | |
| 167 | + | |
| 168 | + | |
| 169 | + | |
| 170 | + | |
| 171 | + | |
| 172 | + | |
| 173 | + | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
158 | 178 | | |
159 | 179 | | |
160 | 180 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
293 | 293 | | |
294 | 294 | | |
295 | 295 | | |
296 | | - | |
297 | | - | |
| 296 | + | |
| 297 | + | |
| 298 | + | |
298 | 299 | | |
299 | 300 | | |
300 | 301 | | |
301 | 302 | | |
302 | 303 | | |
303 | 304 | | |
304 | | - | |
| 305 | + | |
305 | 306 | | |
306 | 307 | | |
307 | 308 | | |
| |||
0 commit comments