Repository navigation
Commit cac0be8
fix(rest,runtime)!: both /data write doors validate the request as the caller sent it; a number expectedVersion is refused 400 (#23571)
Fixes #23491
Fixes #23517
Clause-②: no
One PR for a two-card family fold: both `PATCH /data/:object/:id` doors,
`RestServer`'s registered handler and the runtime dispatcher's `/data`
domain (the only answer on a `createHonoApp` host), now check
`UpdateDataRequestSchema` against the request **as the caller sent it**.
The first commit is #23491 and the second is #23517.
## What changed
**Commit 1, #23491: a request-schema gate on the dispatcher, and no
coerced copy on `RestServer`**
- `packages/runtime/src/domains/data.ts`, `PATCH` branch: the assembled
request `{ object, id, data, expectedVersion? }` is parsed with
`UpdateDataRequestSchema` after the version-token check and before
`callData`. The token is stripped from `data`, and nothing is defaulted.
A failure returns `400 VALIDATION_FAILED` with `error.details.fields`
through `deps.errorFromThrown(validationFailure(…))`. It is returned
rather than thrown, as the sibling version refusal is.
- `packages/runtime/src/domains/data.ts`, `POST /data/:object` create
branch: the same gate with `CreateDataRequestSchema`. H4 measured the
same gap there, and a worse one, so it is folded in (see the table).
- `packages/rest/src/rest-server.ts`, `PATCH` handler: `data ?? {}` is
gone from both the gate input and the forward. The gate now parses the
patch as sent.
**Commit 2, #23517: a number token is refused, and both doors gate and
forward the raw token**
- `versionTokenRefusal`, both copies, still statement for statement
(`diff` of the two function bodies is empty): the `typeof token ===
'number' && Number.isFinite(token)` arm is deleted, so a number joins
the refused shapes. The prescription now says "send the version you read
as a string". Both docblocks name the change.
- Both `PATCH` doors put the raw token into the gate input and the
forward (`{ expectedVersion }`, no longer `String(expectedVersion)`).
With the arm gone, anything that reaches the gate is already a non-blank
string, so the gate and the forward see the same value the caller sent.
- `DELETE` is untouched. Its `?expectedVersion` and every `If-Match` are
strings by transport. `data-delete-expected-version.test.ts` (34 tests)
runs green on both doors, unchanged.
Landing is exactly the claimed file surface: the two branches of
`data.ts`, the `PATCH` handler and the `versionTokenRefusal` of
`rest-server.ts`, their tests, and one changeset. No other region of
`rest-server.ts` was touched.
## The `null` answer: `400` on both `PATCH` doors
Measured first, on the rig PR #23515 built (real better-sqlite3 store,
real ObjectQL, real protocol; `RestServer`'s registered handler and
`HttpDispatcher.dispatch()` side by side), at `086fa4640f`:
- `RestServer` answered a JSON `null` body `200`, called `updateData`
with `{}`, and **moved the record's `updated_at`** (`…26.716Z` to
`…26.723Z`) with no field changed. Every other holder of the old version
would then lose its next guarded save to a request that wrote nothing.
- The dispatcher answered the same body `500 INTERNAL_ERROR` ("Cannot
read properties of null (reading 'id')").
I picked `400 VALIDATION_FAILED` for both. The declared type is a
record, and `data ?? {}` was the consumer-side leniency Prime Directive
12 forbids. **This narrows `RestServer`'s accept set (`200` to `400`)**.
It stays `Clause-②: no`: no key is added and no code is new. The
changeset carries the narrowing in its FROM/TO list.
## Before / after, both doors (measured, `086fa4640f` vs this head)
| request | `RestServer` before | dispatcher before | both doors after |
|---|---|---|---|
| `PATCH` body `[{…}]` | 400 VALIDATION_FAILED | 500 INTERNAL_ERROR ("no
such column: 0") | 400 VALIDATION_FAILED, `fields: [data]` |
| `PATCH` body `"garbage"` | 400 VALIDATION_FAILED | 500 INTERNAL_ERROR
| 400 VALIDATION_FAILED, `fields: [data]` |
| `PATCH` body `42` | 400 VALIDATION_FAILED | 500 INTERNAL_ERROR | 400
VALIDATION_FAILED, `fields: [data]` |
| `PATCH` body `null` | **200, version moved** | 500 INTERNAL_ERROR |
400 VALIDATION_FAILED, `fields: [data]`, version unchanged |
| `PATCH` `expectedVersion: 5` / `0`, stale or current `If-Match` | 409
CONCURRENT_UPDATE (forwarded `"5"` / `"0"`) | 409 CONCURRENT_UPDATE |
400 VALIDATION_FAILED, `updateData` never asked |
| `POST` body `[{…}]` | 400 VALIDATION_FAILED | **201, one row created
per element** | 400 VALIDATION_FAILED, no row |
| `POST` body `"garbage"` | 400 VALIDATION_FAILED | 400 INVALID_FIELD
(engine) | 400 VALIDATION_FAILED, no row |
| `POST` body `42` | 400 VALIDATION_FAILED | **201, an empty row** | 400
VALIDATION_FAILED, no row |
| `POST` body `null` | 201, an empty row | 201, an empty row | unchanged
on both (see below) |
Envelopes keep each door's own convention (H5). `RestServer`: `{ error:
'Invalid update request', code, fields, object }`, and for a token
refusal the `mapDataError` envelope it already gave `""`. Dispatcher: `{
success: false, error: { code, message, details: { fields } } }`. The
pins compare status, code, field names and store state across doors.
They never compare envelope bytes.
## `POST` create with `null`: left as both doors answer it today
`RestServer`'s create assembles `{ object, data: req.body ?? {} }`. That
region is outside this PR's surface, and the seat's same-file serial
rule holds it. So the dispatcher's new create gate asks **the same
assembled request, statement for statement**, `?? {}` included. The two
doors answer a `null` create alike (`201`, an empty row), and the parity
row pins that agreement without pinning the `201` itself. This is the
one `??` default the diff adds, on purpose. The alternative was a
dispatcher-only `400`, which would split one route into two answers.
That leniency is retired on both doors at once or not at all. It is
raised as an open question in the report.
## Pins
- New: `packages/runtime/src/domains/data-write-request-gate.test.ts`.
`PATCH` and `POST` through `dispatch()` beside `RestServer`'s registered
handler. It covers an array, a string, a number and `null`. Each row
asserts `400` and `VALIDATION_FAILED`, `fields` naming `data`, the
protocol method never called, and the store unchanged. For `PATCH`, the
store check includes `updated_at`, so the flipped `null` row asserts
that the version did not move. A cross-door parity block and a
record-body control for each verb are included. 26 tests.
- New in `data-update-expected-version.test.ts`: a `[#23517]` block.
`expectedVersion: 5` and `0`, each beside a stale and a current
`If-Match`, on both doors: `400 VALIDATION_FAILED`, `updateData` never
asked, the row unchanged. A parity block checks status, code and
sentence. Controls: the same token as the string `'5'` is still judged
(`409`), and the current `If-Match` alone still updates.
- `packages/rest/src/request-schema-gate.conformance.test.ts`: `null`
joins the `PATCH` row's violating bodies.
**Flipped pins** (repo-wide grep for a numeric `expectedVersion`
answering `409` and for a `null` `PATCH` answering `200`: two hits, both
in `data-update-expected-version.test.ts`, and no `null`-200 pin
anywhere):
- `control: a body token of 0 … judged as "0" (refused 409)` now asserts
`400 VALIDATION_FAILED`, `updateData` never asked, and the row unchanged
beside the **current** `If-Match`. That keeps its original point: `0` is
never dropped as "no token".
- `EDGE_SHAPES` `a number token` (`20000101`) is removed from the
judged-shape parity list. Numbers are pinned in the new `[#23517]`
block.
## Tests and gates (on `3dd76b2a8`)
All local runs below are on `3dd76b2a8` (the final commit). Exit codes
were captured before any pipe.
- `@objectstack/rest` suite, `vitest run --project local`: **284 files,
5,856 passed, 326 skipped, exit 0**.
- `@objectstack/runtime` suite, `vitest run --project local`: **382
files, 5,673 passed, 19 skipped, exit 0**.
- Targeted: `data-write-request-gate.test.ts` 26/26,
`data-update-expected-version.test.ts` 65/65,
`data-delete-expected-version.test.ts` 34/34;
`request-schema-gate.conformance.test.ts` 16/16.
- `pnpm --filter @objectstack/rest typecheck` exit 0, and `pnpm --filter
@objectstack/runtime typecheck` exit 0 (the test layer compiles; the
runtime debt ledger is unchanged).
- eslint (the repo config, `--no-inline-config`) over
`packages/rest/src` and `packages/runtime/src`: exit 0, no findings.
Repo-wide `pnpm lint` is left to CI.
- `node scripts/pm/dispatch-gates.mjs --commands --repo
objectstack-ai/objectstack` derived 65 gate lines at this head. 64 exit
0. `pnpm check:dual-build-cjs-loads` exit 3 `PREREQUISITE NOT MET` (it
needs every package's `dist/`): **NOT MEASURED locally**, CI's check-run
is the reading. `--ran` reconciliation: 65 derived, 64 run, 1
NOT-MEASURED, 0 UNRUN.
**Ablations.** One per behaviour. Each ran through
`scripts/ablation-replace.mjs` against the committed fix, with the
anchor proven to hit and a restore proven against `HEAD`. These suites
resolve `data.ts` and `@objectstack/rest` from source, so no `dist/`
step was involved.
| ablated | red |
|---|---|
| dispatcher `PATCH` gate line deleted | 8/26 in the new file: the 4
dispatcher `PATCH` rows and the 4 `PATCH` parity rows |
| `RestServer` gate input back to `data ?? {}` | the `RestServer` `null`
row and the `null` parity row (2/26), and the conformance `PATCH` row
(1/16) |
| dispatcher `versionTokenRefusal` number arm restored, alone | **4/65,
only the four `[#23517]` parity rows (sentence differs)**. The raw-token
request gate still refused the number `400` on the dispatcher, so the
per-door rows stayed green. That is the second guard holding, and it is
reported as observed. |
| the number arm restored **and** the gate input back to
`String(expectedVersion)` | 9/65: the 4 dispatcher `[#23517]` rows, the
4 parity rows, and the flipped `0` control |
| dispatcher create gate line deleted | 6/26: the 3 dispatcher `POST`
rows and the 3 `POST` parity rows |
Not re-run: this branch is three commits behind `origin/main`
(`252775d1d`). Those commits touch `packages/cli`,
`packages/metadata-protocol` and `packages/spec` tests, nothing under
`packages/rest` or `packages/runtime`, and `git merge-tree` is clean. CI
runs the merge ref.
## Acceptance notes
- **A sibling with the same gap, out of scope:** the dispatcher's `POST
/data/:object/query` has no `FindDataRequestSchema` gate either.
Measured on the same rig at this head: `{ limit: 'ten' }` and the
retired `{ joins: [] }` answer `200` with the full set on the
dispatcher, while `RestServer` refuses both `400 VALIDATION_FAILED`
(`query.limit`, and the `joins` tombstone). It is reported to the seat
as a finding and is not fixed here.
- A numeric token against a legacy epoch-ms `updated_at`: the protocol's
`canonicalVersionInstant` reads a `number` column as an instant, so I
checked whether a number token could ever have matched. On
better-sqlite3 the column reads back as ISO text, and `expectedVersion:
1700000000000` answered `409` before this change (now `400`). The string
spelling `'1700000000000'` still answers `409`, as before. Other drivers
were not measured.
- Check order differs on an object whose API exposure denies the update:
RestServer runs enforceApiAccess before its request gate (403), while
the dispatcher runs its gate before callData, whose exposure check is
inside (400). Before this PR the dispatcher answered that exposure
refusal too, so this ordering is new on the dispatcher. Read in source,
not measured.
- *(This bullet replaces the PR's original last bullet, which the dev's
report `os-dev-report` on #23491 marked wrong. It was written by the
`domain:cli` seat 1 PM, session `session_01FLwgbTSmGMrqsXkBDsiwfG`, as a
read-modify-write of the live body. The dev does not PATCH a PR body.)*
---
_Generated by [Claude
Code](https://claude.ai/code/session_01FLwgbTSmGMrqsXkBDsiwfG)_
---------
Co-authored-by: Claude <noreply@anthropic.com>1 parent 70d4c06 commit cac0be8
6 files changed
Lines changed: 542 additions & 40 deletions
File tree
- .changeset
- packages
- rest/src
- runtime/src/domains
| 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 | + | |
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
131 | 131 | | |
132 | 132 | | |
133 | 133 | | |
134 | | - | |
| 134 | + | |
| 135 | + | |
| 136 | + | |
135 | 137 | | |
136 | 138 | | |
137 | 139 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
1743 | 1743 | | |
1744 | 1744 | | |
1745 | 1745 | | |
1746 | | - | |
1747 | | - | |
1748 | | - | |
1749 | | - | |
| 1746 | + | |
| 1747 | + | |
| 1748 | + | |
| 1749 | + | |
1750 | 1750 | | |
1751 | | - | |
1752 | | - | |
1753 | | - | |
1754 | | - | |
1755 | | - | |
1756 | | - | |
| 1751 | + | |
| 1752 | + | |
| 1753 | + | |
| 1754 | + | |
| 1755 | + | |
| 1756 | + | |
| 1757 | + | |
| 1758 | + | |
| 1759 | + | |
| 1760 | + | |
| 1761 | + | |
| 1762 | + | |
| 1763 | + | |
| 1764 | + | |
1757 | 1765 | | |
1758 | 1766 | | |
1759 | 1767 | | |
| |||
1779 | 1787 | | |
1780 | 1788 | | |
1781 | 1789 | | |
1782 | | - | |
1783 | 1790 | | |
1784 | 1791 | | |
1785 | 1792 | | |
1786 | 1793 | | |
1787 | 1794 | | |
1788 | | - | |
1789 | | - | |
| 1795 | + | |
| 1796 | + | |
1790 | 1797 | | |
1791 | 1798 | | |
1792 | 1799 | | |
| |||
9173 | 9180 | | |
9174 | 9181 | | |
9175 | 9182 | | |
| 9183 | + | |
| 9184 | + | |
| 9185 | + | |
| 9186 | + | |
| 9187 | + | |
| 9188 | + | |
| 9189 | + | |
| 9190 | + | |
| 9191 | + | |
| 9192 | + | |
| 9193 | + | |
| 9194 | + | |
| 9195 | + | |
| 9196 | + | |
| 9197 | + | |
| 9198 | + | |
| 9199 | + | |
9176 | 9200 | | |
9177 | 9201 | | |
9178 | 9202 | | |
9179 | 9203 | | |
9180 | | - | |
9181 | | - | |
| 9204 | + | |
| 9205 | + | |
9182 | 9206 | | |
9183 | 9207 | | |
9184 | 9208 | | |
| |||
9193 | 9217 | | |
9194 | 9218 | | |
9195 | 9219 | | |
9196 | | - | |
9197 | | - | |
| 9220 | + | |
| 9221 | + | |
9198 | 9222 | | |
9199 | 9223 | | |
9200 | 9224 | | |
| |||
| Original file line number | Diff line number | Diff line change | |
|---|---|---|---|
| |||
294 | 294 | | |
295 | 295 | | |
296 | 296 | | |
297 | | - | |
298 | | - | |
| 297 | + | |
| 298 | + | |
| 299 | + | |
299 | 300 | | |
300 | 301 | | |
301 | 302 | | |
302 | | - | |
303 | 303 | | |
304 | 304 | | |
305 | 305 | | |
| |||
346 | 346 | | |
347 | 347 | | |
348 | 348 | | |
349 | | - | |
| 349 | + | |
| 350 | + | |
350 | 351 | | |
351 | 352 | | |
352 | 353 | | |
| |||
391 | 392 | | |
392 | 393 | | |
393 | 394 | | |
394 | | - | |
| 395 | + | |
| 396 | + | |
| 397 | + | |
| 398 | + | |
395 | 399 | | |
396 | | - | |
397 | | - | |
| 400 | + | |
| 401 | + | |
398 | 402 | | |
399 | 403 | | |
400 | 404 | | |
| |||
411 | 415 | | |
412 | 416 | | |
413 | 417 | | |
| 418 | + | |
| 419 | + | |
| 420 | + | |
| 421 | + | |
| 422 | + | |
| 423 | + | |
| 424 | + | |
| 425 | + | |
| 426 | + | |
| 427 | + | |
| 428 | + | |
| 429 | + | |
| 430 | + | |
| 431 | + | |
| 432 | + | |
| 433 | + | |
| 434 | + | |
| 435 | + | |
| 436 | + | |
| 437 | + | |
| 438 | + | |
| 439 | + | |
| 440 | + | |
| 441 | + | |
| 442 | + | |
| 443 | + | |
| 444 | + | |
| 445 | + | |
| 446 | + | |
| 447 | + | |
| 448 | + | |
| 449 | + | |
| 450 | + | |
| 451 | + | |
| 452 | + | |
| 453 | + | |
| 454 | + | |
| 455 | + | |
| 456 | + | |
| 457 | + | |
| 458 | + | |
| 459 | + | |
| 460 | + | |
| 461 | + | |
| 462 | + | |
| 463 | + | |
| 464 | + | |
| 465 | + | |
| 466 | + | |
| 467 | + | |
| 468 | + | |
| 469 | + | |
| 470 | + | |
| 471 | + | |
| 472 | + | |
| 473 | + | |
| 474 | + | |
| 475 | + | |
| 476 | + | |
| 477 | + | |
| 478 | + | |
| 479 | + | |
| 480 | + | |
| 481 | + | |
| 482 | + | |
414 | 483 | | |
415 | 484 | | |
416 | 485 | | |
| |||
0 commit comments