fix(rest,runtime)!: both /data write doors validate the request as the caller sent it; a number expectedVersion is refused 400

.changeset/23491-patch-data-request-gate.md

@@ -0,0 +1,27 @@
+---
+'@objectstack/rest': minor
+'@objectstack/runtime': minor
+---
+
+fix(rest,runtime)!: both `/data` write doors validate the request as the caller sent it: a `PATCH` or `POST` create whose body is not a record, and a `PATCH` whose `expectedVersion` is a number, are refused `400 VALIDATION_FAILED` on both doors before the engine is asked
+
+`RestServer` validates `PATCH /data/:object/:id` against `UpdateDataRequestSchema` and `POST /data/:object` against `CreateDataRequestSchema`. The runtime dispatcher's `/data` domain, the only answer on a host that mounts just the `@objectstack/hono` catch-all, validated neither, so a body that is not a record reached the engine. On a `PATCH` it answered `500` with the engine's own error. On a `POST`, an array created one row per element and a number created an empty row, both answered `201`. `RestServer`'s own `PATCH` gate parsed a coerced copy of the request: it read a JSON `null` body as `{}`, and it read a numeric `expectedVersion` as its string. Both doors now validate the assembled request as the caller sent it, and they answer each case alike.
+
+Clause-②: no
+
+<!-- adr-0087: not-required (no-migration-prescription) No metadata moves: no spec key, authorable spelling, object definition or stored shape is added, removed, renamed or re-shaped, and no exported declaration is added, removed or narrowed, so there is nothing for `objectstack migrate meta` to rewrite. What narrows is the accept set of two HTTP routes on two doors: a write whose body is not the record the request schema declares, or whose body version token is a number where the schema declares a string, is now refused where it was applied or judged, and the remedy is in the request a caller sends, never a rewrite of a package source or a stored row. The other categories are closed on facts: both packages publish (not unpublished); no ADR-0087 id is named or touched (not registered or already-registered); and nothing exported is removed or narrowed (not runtime-interface-only, type-surface-only or runtime-export-retired). -->
+
+**BREAKING** (an accept-set narrowing), shipped as `minor` under the launch-window convention for breaking changes.
+
+What changes for you, wherever `@objectstack/rest` or the runtime dispatcher serves these routes:
+
+- `RestServer`, `PATCH /data/:object/:id` with a JSON `null` body: FROM `200`, an update that changed no field and still moved the record's `updated_at` (so every other holder of the old version saw a conflict on its next guarded save), TO `400 VALIDATION_FAILED` naming `data`, the record unchanged. The runtime dispatcher answers the same.
+- The runtime dispatcher, `PATCH /data/:object/:id` with an array, a string, a number or `null` as the body: FROM `500` carrying the engine's own error text, nothing written, TO `400 VALIDATION_FAILED` naming `data`, before the engine is asked. `RestServer` already answered `400` for the first three.
+- The runtime dispatcher, `POST /data/:object` with an array, a string or a number as the body: FROM `201` with one row created per array element, `201` with an empty row for a number, and the engine's own `400 INVALID_FIELD` for a string, TO `400 VALIDATION_FAILED` naming `data`, with no row created. `RestServer` already answered this.
+- Both doors, `PATCH /data/:object/:id` with a body `expectedVersion` that is a number (`5`, `0`): FROM judged as its string (`'5'`) and answered `409 CONCURRENT_UPDATE`, TO `400 VALIDATION_FAILED` before the protocol is asked, whatever `If-Match` is sent beside it. The request schema declares the token a string, and a number is now one of the token shapes that can never carry a version.
+
+How it renders: `RestServer` answers `{ error, code: 'VALIDATION_FAILED', fields, object }`, the envelope its request gate already used. The runtime dispatcher returns its own `400 VALIDATION_FAILED` envelope, `fields` under `error.details`, instead of throwing, so a `@objectstack/hono` `createHonoApp` host renders it as `400`.
+
+Unchanged: a record body, `{}` included; a string `expectedVersion` (judged; `409` on a mismatch); `If-Match` and the `DELETE`'s `?expectedVersion`, which are strings by transport; and a JSON `null` body on a `POST` create, which both doors still read as an empty record and answer `201`.
+
+To send a patch or a row, send the fields as a JSON object. To guard a write, send the version you read (the record's `updated_at`) as a string. The `@objectstack/client` SDK's `data.update` sends the version as an `If-Match` header string, so an SDK caller is affected only when the `data` it passes is not an object, or carries a number `expectedVersion`.

packages/rest/src/request-schema-gate.conformance.test.ts

@@ -131,7 +131,9 @@ const CASES: GateCase[] = [
   {
     route: 'PATCH /api/v1/data/:object/:id',
     params: PARAMS_ID,
-    violating: ['garbage', 42, [{ name: 'x' }]],
+    // #23491 — `null` was read as an empty patch (`data ?? {}`) and answered
+    // 200, an update that changed no field and still moved the version.
+    violating: ['garbage', 42, [{ name: 'x' }], null],
     valid: { name: 'ACME (renamed)' },
     spy: 'updateData',
   },

packages/rest/src/rest-server.ts

@@ -1743,17 +1743,25 @@ function sendMetaItemAbsent(res: any): void {
  *  - ABSENT: the spelling consulted is `undefined` or `null`. The body key or
  *    the query name falls through to `If-Match`, and no `If-Match` either is
  *    the legal unguarded write.
- *  - JUDGED: a string with something in it, or a finite number. The door
- *    forwards its string, and the protocol compares it (`409
- *    CONCURRENT_UPDATE` on a mismatch; the quoted-empty tag `""` is the
- *    protocol's own `400`, `MalformedVersionTokenError`).
+ *  - JUDGED: a string with something in it. The door forwards it, and the
+ *    protocol compares it (`409 CONCURRENT_UPDATE` on a mismatch; the
+ *    quoted-empty tag `""` is the protocol's own `400`,
+ *    `MalformedVersionTokenError`).
  *  - REFUSED `400 VALIDATION_FAILED`: anything else the caller put in a token
- *    position — an empty or blank string, a boolean, an array, an object. It
- *    can never carry a version, so it is the protocol's ruled answer to `""`
- *    (maintainer decision #20 ①: a token shape that can never carry a version
- *    is refused, neither read as "no token" nor answered `409`), given here
- *    because the protocol never sees these: it reads a blank string as no
- *    token.
+ *    position — an empty or blank string, a number, a boolean, an array, an
+ *    object. It can never carry a version, so it is the protocol's ruled
+ *    answer to `""` (maintainer decision #20 ①: a token shape that can never
+ *    carry a version is refused, neither read as "no token" nor answered
+ *    `409`), given here because the protocol never sees these: it reads a
+ *    blank string as no token.
+ *
+ * [#23517] A number is in the refused set, no longer judged as its string.
+ * `UpdateDataRequestSchema.expectedVersion` declares a string, and the door
+ * forwarded `String(5)`, so the request gate below only ever parsed the
+ * coerced copy and a token the contract never admits was answered `409` as if
+ * it were a stale version. Only a body token can be a number: `If-Match` and
+ * `?expectedVersion` are strings by transport, so the `DELETE` and every
+ * header token are unaffected.
  *
  * What it replaces is a truthiness test. The door forwarded a token only when
  * it was truthy, and `bodyVersion ?? If-Match` reads `If-Match` only when the
@@ -1779,14 +1787,13 @@ function sendMetaItemAbsent(res: any): void {
 function versionTokenRefusal(token: unknown, spelling: string): string | undefined {
     if (token === undefined || token === null) return undefined;
     if (typeof token === 'string' && token.trim() !== '') return undefined;
-    if (typeof token === 'number' && Number.isFinite(token)) return undefined;
     const shape = typeof token === 'string' ? 'empty'
         : Array.isArray(token) ? 'an array'
         : typeof token === 'object' ? 'an object'
         : `the ${typeof token} \`${String(token)}\``;
     return `${spelling} is ${shape}, which can never carry a version, so it is refused rather than read as `
-        + 'no token. Send the version you read (the record\'s `updated_at`), or omit both expectedVersion and '
-        + 'If-Match to perform an unguarded write.';
+        + 'no token. Send the version you read as a string (the record\'s `updated_at`), or omit both '
+        + 'expectedVersion and If-Match to perform an unguarded write.';
 }
 
 /**
@@ -9173,12 +9180,29 @@ export class RestServer {
                         // validated as the assembled `UpdateDataRequestSchema`
                         // request so a non-record body 400s instead of reaching
                         // the engine.
+                        //
+                        // [#23491] …the patch AS THE CALLER SENT IT. It was
+                        // `data ?? {}`, so a JSON `null` body parsed as an empty
+                        // patch and answered `200`: an update that changed no
+                        // field and still moved the record's version. `null` is
+                        // not the record the contract declares, so it is refused
+                        // like an array, a string or a number, and the runtime
+                        // dispatcher's twin (`domains/data.ts`) asks this gate
+                        // over the same assembled request.
+                        //
+                        // [#23517] …and the token as the caller sent it, never
+                        // its `String()`: this gate parsed `String(5)`, so a
+                        // number `expectedVersion: z.string()` declares no room
+                        // for was forwarded and judged `409`.
+                        // `versionTokenRefusal` above already refuses it, so what
+                        // reaches this gate and the forward below is a non-blank
+                        // string.
                         const { UpdateDataRequestSchema } = await import('@objectstack/spec/api');
                         const updateInput = {
                             object: req.params.object,
                             id: req.params.id,
-                            data: data ?? {},
-                            ...(expectedVersion != null ? { expectedVersion: String(expectedVersion) } : {}),
+                            data,
+                            ...(expectedVersion != null ? { expectedVersion } : {}),
                         };
                         const parsedUpdateOne = (UpdateDataRequestSchema as any).safeParse(updateInput);
                         if (!parsedUpdateOne.success) {
@@ -9193,8 +9217,8 @@ export class RestServer {
                         const updateRequest: ServerScopedDataRequest<UpdateDataRequest> = {
                             object: req.params.object,
                             id: req.params.id,
-                            data: data ?? {},
-                            ...(expectedVersion != null ? { expectedVersion: String(expectedVersion) } : {}),
+                            data,
+                            ...(expectedVersion != null ? { expectedVersion } : {}),
                             ...(environmentId ? { environmentId } : {}),
                             ...(context ? { context } : {}),
                         };

packages/runtime/src/domains/data-update-expected-version.test.ts

@@ -294,12 +294,12 @@ for (const [doorName, boot] of DOORS) {
  * row. What either door does with them is `RestServer`'s reading, not this
  * file's: a token is the body key when it is not nullish (else `If-Match`),
  * the key is stripped from the patch whatever its value, and a token is
- * forwarded as its string unless it can never carry a version, which is
- * refused (#23492, pinned in its own block below).
+ * forwarded unless it can never carry a version, which is refused (#23492,
+ * pinned in its own block below; a number among them since #23517, pinned in
+ * the block after it).
  */
 const EDGE_SHAPES: ReadonlyArray<readonly [string, (version: string) => PatchOptions]> = [
     ['a stale token', () => ({ body: { name: AFTER, expectedVersion: STALE } })],
-    ['a number token', () => ({ body: { name: AFTER, expectedVersion: 20000101 } })],
     ['a null body token beside a stale If-Match', () => ({ body: { name: AFTER, expectedVersion: null }, headers: { 'If-Match': STALE } })],
     ['a null body token beside the current If-Match', (v) => ({ body: { name: AFTER, expectedVersion: null }, headers: { 'If-Match': v } })],
     ['an empty-string body token beside a stale If-Match', () => ({ body: { name: AFTER, expectedVersion: '' }, headers: { 'If-Match': STALE } })],
@@ -346,7 +346,8 @@ describe('[#23462] the two doors answer each token shape with the same status, c
  * position that can never carry a version is refused with the protocol's own
  * answer to the quoted-empty tag `""`, `400 VALIDATION_FAILED`, before
  * `updateData` is asked; maintainer decision #20 ① rules that shape refused,
- * neither read as "no token" nor answered `409`.
+ * neither read as "no token" nor answered `409`. A number joined this set
+ * with #23517 and has its own block below.
  */
 const UNUSABLE: ReadonlyArray<readonly [string, PatchOptions]> = [
     ["a body expectedVersion of '' beside a stale If-Match", { body: { name: AFTER, expectedVersion: '' }, headers: { 'If-Match': STALE } }],
@@ -391,10 +392,13 @@ for (const [doorName, boot] of DOORS) {
             expect(await door.name()).toBe(BEFORE);
         }, 60_000);
 
-        it('control: a body token of 0 is a number, judged as "0" (refused 409), never dropped', async () => {
+        it('control: a body token of 0 is never dropped: it is refused 400, so the current If-Match beside it is not read', async () => {
+            // [#23517] It was judged as '0' (409) until a number joined the
+            // refused set. Either way it is not read as "no token": had it
+            // been dropped, the current If-Match would have applied the write.
             const door = await boot();
-            conflict(await door.patch({ body: { name: AFTER, expectedVersion: 0 }, headers: { 'If-Match': door.version } }));
-            expect(door.forwarded()).toEqual([{ expectedVersion: '0', data: { name: AFTER } }]);
+            refused(await door.patch({ body: { name: AFTER, expectedVersion: 0 }, headers: { 'If-Match': door.version } }));
+            expect(door.forwarded()).toEqual([]);
             expect(await door.name()).toBe(BEFORE);
         }, 60_000);
     });
@@ -411,6 +415,71 @@ describe('[#23492] the two doors refuse each token that can never carry a versio
     }
 });
 
+// ── A number token ────────────────────────────────────────────────────────────
+
+/**
+ * [#23517] `UpdateDataRequestSchema.expectedVersion` declares a string. Both
+ * doors forwarded a number as `String(token)`, so `RestServer`'s request gate
+ * only ever parsed the coerced copy and the dispatcher had no gate: a body
+ * `expectedVersion: 5` was judged as `'5'` and answered `409
+ * CONCURRENT_UPDATE` on both doors (measured on this rig before the fix,
+ * beside a stale and beside the current `If-Match`; the row unchanged). Now a
+ * number is refused `400 VALIDATION_FAILED` before `updateData` is asked, the
+ * answer maintainer decision #20 ① gives a token shape that can never carry a
+ * version. The `If-Match` beside it is never read: the body key is the token
+ * whenever it is not nullish.
+ */
+const NUMBER_TOKENS: ReadonlyArray<readonly [string, number]> = [['5', 5], ['0', 0]];
+const IF_MATCH_BESIDE: ReadonlyArray<readonly [string, (version: string) => string]> = [
+    ['a stale If-Match', () => STALE],
+    ['the current If-Match', (version) => version],
+];
+
+for (const [doorName, boot] of DOORS) {
+    describe(`[#23517] ${doorName}: PATCH /data/:object/:id refuses a number expectedVersion`, () => {
+        for (const [tokenName, token] of NUMBER_TOKENS) {
+            for (const [besideName, ifMatch] of IF_MATCH_BESIDE) {
+                it(`expectedVersion: ${tokenName} beside ${besideName}: 400 VALIDATION_FAILED, updateData never asked, the row unchanged`, async () => {
+                    const door = await boot();
+                    refused(await door.patch({ body: { name: AFTER, expectedVersion: token }, headers: { 'If-Match': ifMatch(door.version) } }));
+                    expect(door.forwarded()).toEqual([]);
+                    expect(await door.name()).toBe(BEFORE);
+                }, 60_000);
+            }
+        }
+
+        it('control: the same token as a string is judged, not refused (409 against a stale version)', async () => {
+            const door = await boot();
+            conflict(await door.patch({ body: { name: AFTER, expectedVersion: '5' }, headers: { 'If-Match': door.version } }));
+            expect(door.forwarded()).toEqual([{ expectedVersion: '5', data: { name: AFTER } }]);
+            expect(await door.name()).toBe(BEFORE);
+        }, 60_000);
+
+        it('control: If-Match alone is a string by transport, so the current one still updates', async () => {
+            const door = await boot();
+            updated(await door.patch({ body: { name: AFTER }, headers: { 'If-Match': door.version } }));
+            expect(door.forwarded()).toEqual([{ expectedVersion: door.version, data: { name: AFTER } }]);
+            expect(await door.name()).toBe(AFTER);
+        }, 60_000);
+    });
+}
+
+describe('[#23517] the two doors refuse a number expectedVersion alike', () => {
+    for (const [tokenName, token] of NUMBER_TOKENS) {
+        for (const [besideName, ifMatch] of IF_MATCH_BESIDE) {
+            it(`expectedVersion: ${tokenName} beside ${besideName}: the same status, code and sentence`, async () => {
+                const answers: Answer[] = [];
+                for (const [, boot] of DOORS) {
+                    const door = await boot();
+                    answers.push(await door.patch({ body: { name: AFTER, expectedVersion: token }, headers: { 'If-Match': ifMatch(door.version) } }));
+                }
+                const [rest, catchAllAnswer] = answers;
+                expect(catchAllAnswer).toEqual(rest);
+            }, 60_000);
+        }
+    }
+});
+
 // ── The dispatcher's fallback updater ─────────────────────────────────────────
 
 /**