fix(rest,runtime)!: a /data PATCH or DELETE refuses a version token that can never carry a version, on both doors

.changeset/23492-empty-version-token-refused.md

@@ -0,0 +1,27 @@
+---
+'@objectstack/rest': minor
+'@objectstack/runtime': minor
+---
+
+fix(rest,runtime)!: a `/data` `PATCH` or `DELETE` whose version token can never carry a version (`expectedVersion: ''` or `false`, `?expectedVersion=`, an empty `If-Match`) is refused `400 VALIDATION_FAILED` on both doors, instead of being applied unguarded beside a stale `If-Match`
+
+Both doors of `PATCH /data/:object/:id` and `DELETE /data/:object/:id`, `RestServer` and the runtime dispatcher's `/data` domain, read an optimistic-concurrency token from the body's `expectedVersion` (`PATCH`) or `?expectedVersion` (`DELETE`), else from `If-Match`. They forwarded it to the protocol only when it was truthy. `''` and `false` are not nullish, so a stale `If-Match` beside them was never read, and they are not truthy, so no token was forwarded: the write ran unguarded and answered `200`. The protocol already refuses the quoted-empty tag `""` with `400 VALIDATION_FAILED`, under the ruling that a token shape that can never carry a version is refused, neither read as "no token" nor answered `409`. It never saw these tokens, because the door dropped them first, and it reads a blank string as no token. Both doors now judge every token a caller sends, or refuse it before the protocol is asked, and they answer each token alike.
+
+Clause-②: no (narrowing)
+
+<!-- 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 version token can never carry a version 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, on both doors, wherever `@objectstack/rest` or the runtime dispatcher serves these routes:
+
+- FROM a `PATCH` whose body `expectedVersion` is `''`, a blank string, `false` or an empty array, or a `DELETE` whose `?expectedVersion` is empty or blank, which wrote or deleted the record unguarded and answered `200` even beside a stale `If-Match`, TO `400 VALIDATION_FAILED` with the record unchanged, before the protocol is asked.
+- FROM an empty `If-Match` sent with no body or query token, which wrote or deleted unguarded and answered `200`, TO the same `400 VALIDATION_FAILED`. An `If-Match` that is sent asks for a guard, and an empty one carries nothing to judge. A request that sends no `If-Match` at all is unchanged.
+- FROM a body `expectedVersion` of `true`, an object or a non-empty array, which was judged as its string (`'true'`, `'[object Object]'`, the comma-joined elements), TO the same `400 VALIDATION_FAILED`.
+- FROM a body `expectedVersion` of `0`, which was dropped like `''` and so wrote unguarded, TO a token judged as `'0'`, as every other number already was. On a real record that is `409 CONCURRENT_UPDATE`.
+
+How it renders: `RestServer` answers `{ error, code: 'VALIDATION_FAILED', fields: [], object }`, the envelope it already gives the protocol's `""` refusal. The dispatcher returns its own `400 VALIDATION_FAILED` envelope instead of throwing, so a `@objectstack/hono` `createHonoApp` host renders it as `400`. That is unlike the refusals the protocol throws out of `dispatch()`, `409 CONCURRENT_UPDATE` and the `400` for `""`, which that host still renders as `500` (the record is unchanged), because its catch-all reads only an error's `statusCode`. Rendering their declared status is a separate fix.
+
+Unchanged: a request with no token, a body `expectedVersion: null` (it is no token, so `If-Match` is read), a non-empty string or a non-zero number token (judged; `409` on a mismatch), and `""` (the protocol's own `400`). The body key is still stripped from the patch whatever its value.
+
+To write over a version you read, send that version (the record's `updated_at`) as the token. To write unguarded, omit both `expectedVersion` and `If-Match` rather than sending either one empty. The `@objectstack/client` SDK's `data.update` and `data.delete` already omit `If-Match` when `ifMatch` is empty, so an SDK caller is affected only when its `data` carries an `expectedVersion` that can never carry a version.

packages/rest/src/rest-server.ts

@@ -1733,6 +1733,60 @@ function sendMetaItemAbsent(res: any): void {
     });
 }
 
+/**
+ * [#23492] What a `/data` write door does with the optimistic-concurrency
+ * token it read — the body's `expectedVersion` (`PATCH`) or `?expectedVersion`
+ * (`DELETE`), else `If-Match` — before the protocol is asked. One of three:
+ *
+ *  - 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`).
+ *  - 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.
+ *
+ * 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
+ * body key is nullish, so `expectedVersion: ''` or `false` — not nullish, not
+ * truthy — hid a stale `If-Match`, and the write ran unguarded with a `200`.
+ * `?expectedVersion=` did the same to a `DELETE`, and an empty `If-Match`
+ * alone, the caller asking for a guard and sending nothing to judge, ran
+ * unguarded too.
+ *
+ * Returns the refusal's sentence, or `undefined` when the token is absent or
+ * judged. Both doors build the refusal with the one constructor,
+ * `validationFailure(sentence, [])` (`@objectstack/types`), and render it with
+ * their own classifier: this file THROWS it into the route's catch
+ * (`mapDataError`, the envelope the protocol's `""` refusal gets here), and the
+ * dispatcher hands it to `deps.errorFromThrown` and RETURNS the answer (a throw
+ * out of `dispatch()` reaches a `createHonoApp` host's catch-all, which reads
+ * only `statusCode` and would answer `500`). The runtime dispatcher's `/data`
+ * domain carries this function statement for statement (`versionTokenRefusal`
+ * in `packages/runtime/src/domains/data.ts`), so the two doors answer every
+ * token alike; `data-update-expected-version.test.ts` and
+ * `data-delete-expected-version.test.ts` hold them to it.
+ */
+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.';
+}
+
 /**
  * [#20338] May this caller read PENDING metadata — a `sys_metadata` row in
  * `state: 'draft'`, unpublished authoring work? THE question every door that
@@ -9067,6 +9121,18 @@ export class RestServer {
                             const { expectedVersion: _drop, ...rest } = data as any;
                             data = rest;
                         }
+                        // [#23492] A token that can never carry a version is
+                        // refused here, before anything is asked, and every
+                        // other present token is forwarded below: no token the
+                        // caller sent is read as "no token". THROWN as
+                        // `validationFailure`, so this route's catch answers it
+                        // through `mapDataError`, in the envelope it already
+                        // gives the protocol's own refusal of `""`.
+                        const versionRefusal = versionTokenRefusal(
+                            expectedVersion,
+                            bodyVersion != null ? 'The body\'s `expectedVersion`' : 'The `If-Match` header',
+                        );
+                        if (versionRefusal) throw validationFailure(versionRefusal, []);
                         if (await this.enforceApiAccess(req, res, p, environmentId, 'update')) return;
                         // [#3899] Same gate as create: the wire body is the bare
                         // field patch (`expectedVersion` already stripped above),
@@ -9078,7 +9144,7 @@ export class RestServer {
                             object: req.params.object,
                             id: req.params.id,
                             data: data ?? {},
-                            ...(expectedVersion ? { expectedVersion: String(expectedVersion) } : {}),
+                            ...(expectedVersion != null ? { expectedVersion: String(expectedVersion) } : {}),
                         };
                         const parsedUpdateOne = (UpdateDataRequestSchema as any).safeParse(updateInput);
                         if (!parsedUpdateOne.success) {
@@ -9094,7 +9160,7 @@ export class RestServer {
                             object: req.params.object,
                             id: req.params.id,
                             data: data ?? {},
-                            ...(expectedVersion ? { expectedVersion: String(expectedVersion) } : {}),
+                            ...(expectedVersion != null ? { expectedVersion: String(expectedVersion) } : {}),
                             ...(environmentId ? { environmentId } : {}),
                             ...(context ? { context } : {}),
                         };
@@ -9142,11 +9208,19 @@ export class RestServer {
                             ? (req.query as any).expectedVersion
                             : undefined;
                         const expectedVersion = queryVersion ?? ifMatchHeader;
+                        // [#23492] The `PATCH` above's rule, thrown the same
+                        // way: a token that can never carry a version is
+                        // refused, every other present token is forwarded.
+                        const versionRefusal = versionTokenRefusal(
+                            expectedVersion,
+                            queryVersion != null ? 'The `expectedVersion` query parameter' : 'The `If-Match` header',
+                        );
+                        if (versionRefusal) throw validationFailure(versionRefusal, []);
                         if (await this.enforceApiAccess(req, res, p, environmentId, 'delete')) return;
                         const deleteRequest: ServerScopedDataRequest<DeleteDataRequest> = {
                             object: req.params.object,
                             id: req.params.id,
-                            ...(expectedVersion ? { expectedVersion: String(expectedVersion) } : {}),
+                            ...(expectedVersion != null ? { expectedVersion: String(expectedVersion) } : {}),
                             ...(environmentId ? { environmentId } : {}),
                             ...(context ? { context } : {}),
                         };

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

@@ -314,6 +314,64 @@ describe('[#23265] the two doors refuse a stale token and a repeated one with th
     }, 60_000);
 });
 
+// ── A token that can never carry a version ────────────────────────────────────
+
+/**
+ * [#23492] Both doors read `?expectedVersion ?? If-Match` and used to forward
+ * a token only when it was truthy, so `?expectedVersion=` (empty) hid a stale
+ * `If-Match` and the delete was APPLIED, `200`, on both doors (measured on
+ * this rig before the fix, the row read back gone). Now a token that can never
+ * carry a version is refused with the protocol's own answer to the
+ * quoted-empty tag `""`, `400 VALIDATION_FAILED`, before `deleteData` is
+ * asked; maintainer decision #20 ① rules that shape refused, neither read as
+ * "no token" nor answered `409`. `data-update-expected-version.test.ts` pins
+ * the body spellings on `PATCH`.
+ */
+const UNUSABLE: ReadonlyArray<readonly [string, DeleteOptions]> = [
+    ['an empty ?expectedVersion beside a stale If-Match', { query: { expectedVersion: '' }, headers: { 'If-Match': STALE } }],
+    ['an empty ?expectedVersion and no If-Match', { query: { expectedVersion: '' } }],
+    ['a blank ?expectedVersion beside a stale If-Match', { query: { expectedVersion: '  ' }, headers: { 'If-Match': STALE } }],
+    ['an empty If-Match and no ?expectedVersion', { headers: { 'If-Match': '' } }],
+];
+
+const refused = (a: Answer) =>
+    expect({ status: a.status, code: a.code }, JSON.stringify(a)).toEqual({ status: 400, code: 'VALIDATION_FAILED' });
+
+for (const [doorName, boot] of DOORS) {
+    describe(`[#23492] ${doorName}: DELETE /data/:object/:id refuses a token that can never carry a version`, () => {
+        for (const [shape, opts] of UNUSABLE) {
+            it(`${shape}: 400 VALIDATION_FAILED, deleteData never asked, the row kept`, async () => {
+                const door = await boot();
+                refused(await door.delete(opts));
+                expect(door.forwarded()).toEqual([]);
+                expect(await door.present()).toBe(true);
+            }, 60_000);
+        }
+
+        it('the refusal is the protocol\'s own answer to the quoted-empty tag: the same status and code', async () => {
+            const quoted = await boot();
+            const protocolAnswer = await quoted.delete({ query: { expectedVersion: '""' } });
+            // The protocol, not the door, refused this one: the token reached it.
+            expect(quoted.forwarded()).toEqual(['""']);
+            const empty = await boot();
+            const doorAnswer = await empty.delete({ query: { expectedVersion: '' } });
+            expect({ status: doorAnswer.status, code: doorAnswer.code })
+                .toEqual({ status: protocolAnswer.status, code: protocolAnswer.code });
+        }, 60_000);
+    });
+}
+
+describe('[#23492] the two doors refuse each token that can never carry a version alike', () => {
+    for (const [shape, opts] of UNUSABLE) {
+        it(`${shape}: the same status, code and sentence`, async () => {
+            const answers: Answer[] = [];
+            for (const [, boot] of DOORS) answers.push(await (await boot()).delete(opts));
+            const [rest, catchAllAnswer] = answers;
+            expect(catchAllAnswer).toEqual(rest);
+        }, 60_000);
+    }
+});
+
 // ── The dispatcher's fallback deleter ─────────────────────────────────────────
 
 /**