Repository navigation
Commit 949e99b
fix(objectql)!: refuse an engine
Fixes #20121
Clause-②: no (narrowing)
## Write verbs first — the base reading: a whole-table rewrite and a
whole-table delete
Measured on `origin/main` `a08e059c61` before any edit, through the real
`ObjectQL`, four rows seeded (two per owner), on `driver-memory` and on
`SqlDriver` (better-sqlite3 `:memory:`). The rows touched were read back
past the engine.
| call | no `SecurityPlugin` | `SecurityPlugin`, system context |
`SecurityPlugin`, RLS-scoped member |
|:--|:--|:--|:--|
| `update(o, {name:'Z'}, { where: 'amount > 100', multi: true })` | **4
of 4 rewritten** (both drivers) | **4 of 4 rewritten** (both drivers) |
refused by the driver, `INVALID_FILTER`, 0 rewritten |
| same with `where: 42` / `where: new Map(...)` | **4 of 4** | **4 of
4** | refused by the driver, 0 |
| `delete(o, { where: 'amount > 100', multi: true })` | **4 of 4
deleted** (both drivers) | **4 of 4 deleted** (both drivers) | refused
by the driver, `INVALID_FILTER`, 0 deleted |
| same with `where: 42` / `where: new Map(...)` | **4 of 4** | **4 of
4** | refused by the driver, 0 |
| either write, `where: [1, 2, 3]` | refused, `code`/`status` undefined,
0 | same | same |
| either write, same shapes, no `multi` | refused: "Update/Delete
requires an ID or options.multi=true" (no code), 0 | same | same |
| control: `where: { amount: { $gt: 100 } }` or `[['amount','>',100]]`,
`multi: true` | 2 of 4 | 2 of 4 | 1 of 4 (the member's own row) |
- **Path:** the multi-row path. `update()` and `delete()` lower at the
`[#5158]` comment ("Lower before the by-id extraction below reads
`where.id`" / "Same ordering reason as update()"), then
`resolveEngineUpdateDispatch` / `resolveEngineDeleteDispatch` look for a
`where.id` only in an object `where`, find none, and return `{ kind:
'multi' }` on `options.multi`. The seeded AST carries the string, and
the driver's `updateMany` / `deleteMany` ignores it.
- **"Every row the caller can reach"** holds without `SecurityPlugin`
and under a system context with it. For an RLS-scoped caller it depends
on the value:
- A string, number or `Map` (and a `Date`, `Set` or `true`) is
AND-composed by the security middleware into `$and`, where the driver's
node gate refuses it: 0 rows.
- The empty string is NOT refused. The composition
(`security-plugin.ts`, `opCtx.ast.where ? { $and: … } : …`) reads a
falsy `where` as absent and drops it, so the member's `''` read all 2 of
their rows, and `update(multi)` / `delete(multi)` rewrote or deleted 2
of 4 on both drivers: every row the member could reach.
- **A second door stepped around, too:** the unscoped-write guard
(`dispatchUnscopedMultiWriteHooks`, the `sys_attachment` / `sys_comment`
predicate-less-write refusals) treats only an absent or `null` `where`
as unscoped, so a string / number / `Map` `where` skipped that guard
while the driver treated it as no predicate at all.
The re-grade to p0 is the seat's act; this PR lands the refusal.
## What changed (read from the code at the head of this branch)
`packages/objectql/src/engine.ts`, `lowerWhereFilterArray` only:
1. **A shape gate at the top of the seam.** A `where` that is not
`undefined`, not `null`, not an array and not a filter object is refused
with `invalidFilterError` (the package's existing ADR-0112 builder:
`code: 'INVALID_FILTER'`, `status: 400`, `httpStatus: 400`). All six
verbs (`find`, `findOne`, `count`, `aggregate`, `update`, `delete`) call
the seam before they resolve a driver, so the refusal comes before any
driver call. The message uses the wire door's words: `VERB('OBJECT'):
'where' must be a filter object or condition array, received string
"amount > 100". It was not applied, and an unapplied filter would have
returned the unfiltered result set.` For `update` / `delete`, the last
clause reads `updated every row in scope` / `deleted every row in
scope`.
2. **The non-filter-array branch carries the same envelope.** Its
message is unchanged; it now throws through `invalidFilterError` instead
of a bare `Error`.
3. **The accept set** is decided by `isWhereFilterObject`: a non-array
object whose built-in tag (`Object.prototype.toString`, which reads
`Symbol.toStringTag` through the prototype chain) is `[object Object]`.
That covers a plain object, `Object.create(null)`, a class instance, a
`Proxy` of one, and an object from another realm. An object that
overrides `Symbol.toStringTag`, own or inherited, is refused and named
by its tag (H4 below).
No new export, no new error code, no change to the drivers, to the REST
door, to the `filter` alias or to the `having` region. The docblock hunk
draft PR #20125 edits is not touched.
## H4 — every `where` shape, base answer and head answer
Measured on both drivers with no security; `find` / `update(multi)` /
`delete(multi)` shown (count and aggregate follow `find`).
| shape | base | head |
|:--|:--|:--|
| string, number, `Map` | dropped: 4 rows read / 4 rewritten / 4 deleted
| `INVALID_FILTER` / 400, 0 driver calls |
| non-filter array `[1,2,3]` | refused, `code` / `status` undefined |
`INVALID_FILTER` / 400, 0 driver calls |
| `Date`, `Set`, `true`, `''` | dropped: 4 / 4 / 4 | `INVALID_FILTER` /
400, 0 driver calls |
| `undefined`, `null` | no filter: 4 / 4 / 4 (declared unscoped) |
unchanged |
| `{}`, `[]` | no filter: 4 / 4 / 4 | unchanged |
| `Object.create(null)` with the filter | filters correctly: 2 / 2 / 2 |
unchanged, accepted |
| an author's class instance with the filter on own keys | filters
correctly: 2 / 2 / 2 | unchanged, accepted |
| an object overriding `Symbol.toStringTag` (own or inherited) with the
filter on own keys | filters correctly: 2 / 2 / 2 (contract review's
reading) | `INVALID_FILTER` / 400, `received Criteria` — the one
correctly-answering shape this narrows; no producer in the repo creates
one |
- `null` stays accepted: `findOne`'s no-predicate guard and the
unscoped-write detector both already read `null` as absent.
- `''` is refused, although the wire door reads a blank `?filter=` as
absent. At the engine, `''` deleted every row while the unscoped-write
guard read it as scoped. Under RLS it was not refused either: the
security middleware dropped it as absent, and the member's every
reachable row (2 of 4) was rewritten or deleted. The wire door's
normalizer deletes a blank `?filter=` before the engine sees it; other
REST paths were not measured for `''`.
- `findOne` keeps its own no-predicate refusal (no `code`) for `null` /
`undefined` / `{}` / `[]`, exactly as at base.
## H3 — other doors that take a caller `where` (census, base and head,
`driver-memory`, bad value `'amount > 100'`, control `{ amount: { $gt:
100 } }`)
| door | on this seam? | base | head |
|:--|:--|:--|:--|
| `createContext().object(o).find` / `.delete({ multi: true })` | yes,
forwards to the engine | 4 rows / 4 deleted | refused `INVALID_FILTER` /
400 (pinned) |
| analytics auto-bridge (`engine.aggregate(o, { where: filter })`) |
yes, by reading `service-analytics` `plugin.ts` | follows `aggregate` |
follows `aggregate` |
| `aggregate` `aggregations[].filter` | no, its own loop | string
dropped: every group counts all rows | unchanged — Acceptance notes |
| a `beforeFind` hook assigning `ctx.input.ast.where` | no, runs after
the seam | 4 rows | unchanged — Acceptance notes |
| a middleware assigning `opCtx.ast.where` | no, runs after the seam | 4
rows | unchanged — Acceptance notes |
| a `beforeUpdate` / `beforeDelete` hook assigning `input.options.where`
on `multi: true` | no | inert: the predicate path had already seeded its
AST (2 rows, same as control) | unchanged |
| `updateMany` / `deleteMany` / `upsert` / `distinct` on the engine | —
| not engine verbs (`upsert` and `distinct` are retired) | — |
## Tests (at `aa923a1f5c`, the code head, unless noted)
Head `2bd379d22a` changes only the changeset prose, after contract
review 5831634153. `engine.ts` (`4f5d28170b`) and both pins are
blob-identical to `aa923a1f5c`, so every test reading below stands for
the head.
- New unit pin
`packages/objectql/src/engine-where-shape-refusal.test.ts`, 47 cases,
all through a counting driver double: 6 verbs × 4 bad shapes, each
asserting `code`, `status`, the `VERB('deal')` prefix, zero driver calls
and an unwritten table; 6 × 2 controls; the scoped-repository door; five
accepted edge shapes; five refused edge shapes on `delete(multi)`. Run
together with `engine-filter-array-lowering.test.ts`: `Test Files 2
passed (2) · Tests 107 passed (107)`.
- New dogfood pin
`packages/qa/dogfood/test/engine-where-shape-refusal.test.ts`:
`update(multi)` and `delete(multi)` × the 4 bad shapes on `SqlDriver`
(better-sqlite3), reading the table through knex; plus two controls.
Against the **base** `objectql/dist`: `8 failed | 2 passed` (the 6
string/number/`Map` cells: "expected the engine to refuse, and it
answered"; the 2 array cells: `expected undefined to be
'INVALID_FILTER'`). After rebuilding `objectql`: `10 passed` (rerun at
the head: `10 passed`).
- `@objectstack/objectql` whole suite (both vitest projects), at
`6485915e34`: `Test Files 317 passed (317) · Tests 5499 passed (5499)`.
Between that commit and the head, only the new test file changed: one
repository case was added, and the double now honours `limit` and
refuses combinators. `engine.ts` did not change.
- `@objectstack/plugin-security` whole suite, at `6485915e34` against
the rebuilt `objectql` `dist/`: `Test Files 135 passed (135) · Tests
2685 passed (2685)`.
- Dogfood tests that exercise an engine `where`, at `6485915e34`, a
declared subset (CI's Dogfood Regression Gate runs all of them): the new
pin, `attachments-unscoped-delete-gate`, `bulk-widener-probe`,
`owner-anchor-and-bulk-writes`, `authored-row-write-scope`,
`comments-permission-matrix`, `attachments-permission-matrix`,
`owd-public-read-write-write-floor`, `hook-runas-fls`,
`showcase-crud-persona-matrix`: `Test Files 10 passed (10) · Tests 131
passed | 1 skipped (132)`. The skip is the pre-existing
`describe.skipIf(!organizationsAvailable)` in
`attachments-permission-matrix`.
- `pnpm --filter @objectstack/objectql run typecheck` green, including
`check:test-typecheck` (ledger held, 40 files / 234 errors / 65
signatures); the new test file is in the `tsconfig.test.json` program
(`--listFiles`). `pnpm --filter @objectstack/dogfood run typecheck`
green.
- `check-changeset-no-major --base a08e059`: no `major`.
`check-adr-0087-registration --base a08e059`: 1 declared-breaking
changeset, disposition `not-required (no-migration-prescription)`.
**Ablation**, run at `544762f243` (the 46-case pin, before the
repository case was added), both legs through
`scripts/ablation-replace.mjs` (anchor hits verified on disk, restore
proven by blob equality with `HEAD` and an empty `git diff HEAD`), with
the fix committed first:
1. Shape gate disabled (the `if (where !== undefined && …
!isWhereFilterObject(where))` line → `if (false)`): `23 failed | 23
passed (46)`. Exactly the 18 string/number/`Map` cells plus the 5
refused edge shapes turned red; the 6 array cells stayed green on the
array branch. Predicted direction, observed.
2. The array branch's `invalidFilterError(` → `new Error(`: `6 failed |
40 passed (46)`, exactly the six non-filter-array cells.
## Gates
- Derived from the real diff at `aa923a1f5c` with `node
scripts/pm/dispatch-gates.mjs --commands --repo
objectstack-ai/objectstack` (4 paths vs merge base `a08e059c6`): **68
commands, all exit 0**, captured before any pipe. `--ran`
reconciliation: `68 derived, 68 run, 0 NOT-MEASURED, 0 UNRUN`, a zero
derived from the recorded exit codes.
- The first pass at `c51cd3d02b` turned up two real reds, both in the
new test double: `check:objectql-double-limit` (the double's `find`
ignored the caller's `limit`) and `check:where-matcher` (its matcher
read a combinator as a field name). Both are fixed in `aa923a1f5c`. The
same pass had `check:dual-build-cjs-loads` at exit 3, `PREREQUISITE NOT
MET` (eight unrelated packages had no `dist/`). That is not a red: those
eight were built and the gate reran green (`105 published require entry
point(s) across 67 package(s) load`).
- `node scripts/check-issue-citations.mjs --base a08e059`: `every
citation this change adds resolves` (6 judged).
- Rerun at `2bd379d22a` after the prose patch: `check-changeset-no-major
--event` over this body, `check-adr-0087-registration` and
`check-issue-citations`, all exit 0 (quoted in the patch report).
- Not run locally, and left to CI: the repo-wide `pnpm lint`, the full
dogfood suite, `Build Core`, and `Temporal Conformance` (live PG and
MySQL).
## Acceptance notes
- **`aggregate`'s per-aggregation `filter` drops a string the same way
`where` did.** Base and head, `driver-memory`: `aggregations: [{
function: 'count', field: 'id', alias: 'n', filter: 'amount > 100' }]`
counts every row (control `{ amount: { $gt: 100 } }` counts one per
group). `AggregationNodeSchema.filter` is declared
`FilterConditionSchema`. The loop that walks it (`[#10576]` in
`aggregate()`) is a separate door from `lowerWhereFilterArray`, so it is
outside this claim. No public door or real producer was measured: the
wire door parses `aggregations` through `AggregationNodeSchema`, and the
analytics `ObjectQLStrategy` produces objects.
- **A hook or middleware that rewrites the where after the seam**
(`ctx.input.ast.where` in `beforeFind`, `opCtx.ast.where` in a
middleware) is not re-checked: a string there reads every row (census
above). The written value is the hook's, not the caller's, and this seam
runs before either.
- **The RLS leg at base was refused by the driver, not by the engine,
except for `''`.** For a string, number, `Map`, `Date`, `Set` or `true`,
the security middleware wrapped the value into `$and`, where the
driver's own node gate answers `INVALID_FILTER`. That refusal is now
unreachable for those shapes, because the engine refuses first. The
empty string was dropped as absent by that composition (`opCtx.ast.where
? … : …`) and reached every row the member could reach; the engine now
refuses it too, before the middleware runs.
- **Per-verb consequence clause.** The message's last clause differs by
verb, because a write has no result set; the rest of the sentence is the
wire door's.
---
_Generated by [Claude
Code](https://claude.ai/code/session_01Bvd69VPa6puiNzzPUroDBx)_
---------
Co-authored-by: Claude <noreply@anthropic.com>where that is not a filter before any driver call — a string, number or Map where on a multi-row update or delete rewrote or removed every row (#20144)1 parent f09d412 commit 949e99b
4 files changed
Lines changed: 577 additions & 2 deletions
File tree
- .changeset
- packages
- objectql/src
- qa/dogfood/test
| 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 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 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 | + | |
| 33 | + | |
| 34 | + | |
| 35 | + | |
| 36 | + | |
| 37 | + | |
| 38 | + | |
| 39 | + | |
| 40 | + | |
| 41 | + | |
| 42 | + | |
| 43 | + | |
| 44 | + | |
| 45 | + | |
| 46 | + | |
| 47 | + | |
| 48 | + | |
| 49 | + | |
| 50 | + | |
| 51 | + | |
| 52 | + | |
| 53 | + | |
| 54 | + | |
| 55 | + | |
| 56 | + | |
| 57 | + | |
| 58 | + | |
| 59 | + | |
| 60 | + | |
| 61 | + | |
| 62 | + | |
| 63 | + | |
| 64 | + | |
| 65 | + | |
| 66 | + | |
| 67 | + | |
| 68 | + | |
| 69 | + | |
| 70 | + | |
| 71 | + | |
| 72 | + | |
| 73 | + | |
| 74 | + | |
| 75 | + | |
| 76 | + | |
| 77 | + | |
| 78 | + | |
| 79 | + | |
| 80 | + | |
| 81 | + | |
| 82 | + | |
| 83 | + | |
| 84 | + | |
| 85 | + | |
| 86 | + | |
| 87 | + | |
| 88 | + | |
| 89 | + | |
| 90 | + | |
| 91 | + | |
| 92 | + | |
| 93 | + | |
| 94 | + | |
| 95 | + | |
| 96 | + | |
| 97 | + | |
| 98 | + | |
| 99 | + | |
| 100 | + | |
| 101 | + | |
| 102 | + | |
| 103 | + | |
| 104 | + | |
| 105 | + | |
| 106 | + | |
| 107 | + | |
| 108 | + | |
| 109 | + | |
| 110 | + | |
| 111 | + | |
| 112 | + | |
| 113 | + | |
| 114 | + | |
| 115 | + | |
| 116 | + | |
| 117 | + | |
| 118 | + | |
| 119 | + | |
| 120 | + | |
| 121 | + | |
| 122 | + | |
| 123 | + | |
| 124 | + | |
| 125 | + | |
| 126 | + | |
| 127 | + | |
| 128 | + | |
| 129 | + | |
| 130 | + | |
| 131 | + | |
| 132 | + | |
| 133 | + | |
| 134 | + | |
| 135 | + | |
| 136 | + | |
| 137 | + | |
| 138 | + | |
| 139 | + | |
| 140 | + | |
| 141 | + | |
| 142 | + | |
| 143 | + | |
| 144 | + | |
| 145 | + | |
| 146 | + | |
| 147 | + | |
| 148 | + | |
| 149 | + | |
| 150 | + | |
| 151 | + | |
| 152 | + | |
| 153 | + | |
| 154 | + | |
| 155 | + | |
| 156 | + | |
| 157 | + | |
| 158 | + | |
| 159 | + | |
| 160 | + | |
| 161 | + | |
| 162 | + | |
| 163 | + | |
| 164 | + | |
| 165 | + | |
| 166 | + | |
| 167 | + | |
| 168 | + | |
| 169 | + | |
| 170 | + | |
| 171 | + | |
| 172 | + | |
| 173 | + | |
| 174 | + | |
| 175 | + | |
| 176 | + | |
| 177 | + | |
| 178 | + | |
| 179 | + | |
| 180 | + | |
| 181 | + | |
| 182 | + | |
| 183 | + | |
| 184 | + | |
| 185 | + | |
| 186 | + | |
| 187 | + | |
| 188 | + | |
| 189 | + | |
| 190 | + | |
| 191 | + | |
| 192 | + | |
| 193 | + | |
| 194 | + | |
| 195 | + | |
| 196 | + | |
| 197 | + | |
| 198 | + | |
| 199 | + | |
| 200 | + | |
| 201 | + | |
| 202 | + | |
| 203 | + | |
| 204 | + | |
| 205 | + | |
| 206 | + | |
| 207 | + | |
| 208 | + | |
| 209 | + | |
| 210 | + | |
| 211 | + | |
| 212 | + | |
| 213 | + | |
| 214 | + | |
| 215 | + | |
| 216 | + | |
| 217 | + | |
| 218 | + | |
| 219 | + | |
| 220 | + | |
| 221 | + | |
| 222 | + | |
| 223 | + | |
| 224 | + | |
| 225 | + | |
| 226 | + | |
| 227 | + | |
| 228 | + | |
| 229 | + | |
| 230 | + | |
| 231 | + | |
| 232 | + | |
| 233 | + | |
| 234 | + | |
| 235 | + | |
| 236 | + | |
| 237 | + | |
| 238 | + | |
| 239 | + | |
| 240 | + | |
| 241 | + | |
| 242 | + | |
| 243 | + | |
| 244 | + | |
| 245 | + | |
| 246 | + | |
| 247 | + | |
| 248 | + | |
| 249 | + | |
| 250 | + | |
| 251 | + | |
| 252 | + | |
| 253 | + | |
| 254 | + | |
| 255 | + | |
| 256 | + | |
| 257 | + | |
| 258 | + | |
| 259 | + | |
| 260 | + | |
| 261 | + | |
| 262 | + | |
| 263 | + | |
| 264 | + | |
| 265 | + | |
| 266 | + | |
| 267 | + | |
| 268 | + | |
| 269 | + | |
| 270 | + | |
| 271 | + | |
| 272 | + | |
| 273 | + | |
| 274 | + | |
| 275 | + | |
| 276 | + | |
| 277 | + | |
| 278 | + | |
| 279 | + | |
| 280 | + | |
| 281 | + | |
| 282 | + | |
| 283 | + | |
0 commit comments