From 90424ddc70c152da81ee9321fe8192b724595705 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 15 Oct 2026 00:04:45 +0000 Subject: [PATCH 1/2] fix(rest,runtime)!: both /data write doors validate the request as the caller sent it The runtime dispatcher's PATCH /data/:object/:id and POST /data/:object had no request-schema gate, so a body that is not a record reached the engine: a PATCH answered 500 with the engine's own error, and a POST created one row per array element (or an empty row for a number). RestServer's twin routes refuse those 400 VALIDATION_FAILED; the dispatcher now asks the same assembled request (UpdateDataRequestSchema / CreateDataRequestSchema) and returns that refusal before the engine is asked. RestServer's PATCH read a JSON null body as an empty patch (data ?? {}) and answered 200, an update that changed no field and still moved the record's version. The gate now parses the patch as sent, so null is refused 400 on both PATCH doors. A null create stays as both doors answer it today. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01FLwgbTSmGMrqsXkBDsiwfG --- .../request-schema-gate.conformance.test.ts | 4 +- packages/rest/src/rest-server.ts | 13 +- .../domains/data-write-request-gate.test.ts | 312 ++++++++++++++++++ packages/runtime/src/domains/data.ts | 67 +++- 4 files changed, 388 insertions(+), 8 deletions(-) create mode 100644 packages/runtime/src/domains/data-write-request-gate.test.ts diff --git a/packages/rest/src/request-schema-gate.conformance.test.ts b/packages/rest/src/request-schema-gate.conformance.test.ts index e8b50e96506..bbbf3fb9f5b 100644 --- a/packages/rest/src/request-schema-gate.conformance.test.ts +++ b/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', }, diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index d69a2b87312..afb6005676a 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -9173,11 +9173,20 @@ 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. const { UpdateDataRequestSchema } = await import('@objectstack/spec/api'); const updateInput = { object: req.params.object, id: req.params.id, - data: data ?? {}, + data, ...(expectedVersion != null ? { expectedVersion: String(expectedVersion) } : {}), }; const parsedUpdateOne = (UpdateDataRequestSchema as any).safeParse(updateInput); @@ -9193,7 +9202,7 @@ export class RestServer { const updateRequest: ServerScopedDataRequest = { object: req.params.object, id: req.params.id, - data: data ?? {}, + data, ...(expectedVersion != null ? { expectedVersion: String(expectedVersion) } : {}), ...(environmentId ? { environmentId } : {}), ...(context ? { context } : {}), diff --git a/packages/runtime/src/domains/data-write-request-gate.test.ts b/packages/runtime/src/domains/data-write-request-gate.test.ts new file mode 100644 index 00000000000..7667fc84dab --- /dev/null +++ b/packages/runtime/src/domains/data-write-request-gate.test.ts @@ -0,0 +1,312 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#23491] Both doors of the `/data` single-record writes validate the + * assembled protocol request before the engine is asked: `RestServer`'s + * registered routes and the runtime dispatcher's `/data` domain, the only + * answer on a host that mounts just `@objectstack/hono`'s `${prefix}/*` + * catch-all. + * + * ## What was wrong + * + * `RestServer` validates `PATCH /data/:object/:id` as `UpdateDataRequestSchema` + * and `POST /data/:object` as `CreateDataRequestSchema` (#3899), so a body + * that is not a record answers `400 VALIDATION_FAILED`. The dispatcher's two + * branches had no gate. Measured on this rig before the fix: + * + * - `PATCH` with an array, a string, a number or `null` answered `500 + * INTERNAL_ERROR` with the engine's own error text ("no such column: 0", + * "Cannot use 'in' operator …", "Cannot read properties of null …"), + * after `updateData` was asked; + * - `POST` with an array CREATED a row per element (`201`), a number created + * an empty row (`201`), and a string answered the engine's unknown-field + * `400 INVALID_FIELD`; + * - `RestServer`'s `PATCH` read a `null` body as `{}` (`data ?? {}`) and + * answered `200`: an update that changed no field and still moved the + * record's `updated_at`, so every other holder of the old version was + * turned stale by a request that wrote nothing. + * + * The `null` body now answers `400` on both `PATCH` doors: the declared type + * is a record, and one door reading it as `{}` was the consumer-side leniency + * the contract-first rule forbids. A `null` CREATE is the one shape left as it + * was: `RestServer`'s create reads it as `{}`, the dispatcher's gate asks the + * same assembled request, and the two doors answer it alike (pinned below as + * parity only). + * + * ## The rig + * + * The one `data-update-expected-version.test.ts` describes: each door gets + * its own REAL store (better-sqlite3 `:memory:`, the real `ObjectQL` and a + * real `ObjectStackProtocolImplementation`); `RestServer` is driven through + * its registered handler with the request `plugin-hono-server` builds, and + * the dispatcher through `HttpDispatcher.dispatch()` with the body parsed the + * way the catch-all parses it (`JSON.parse` of the request text, any failure + * `{}`). Every row asserts the store, not just the door's word. + */ + +import { describe, it, expect, vi, afterEach } from 'vitest'; +import { ObjectQL } from '@objectstack/objectql'; +import { SqlDriver } from '@objectstack/driver-sql'; +import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol'; +import { RestServer } from '@objectstack/rest'; +import { resolveThrownHttpError } from '@objectstack/types'; +import { HttpDispatcher } from '../http-dispatcher.js'; + +const PREFIX = '/api/v1'; +const DATA = `${PREFIX}/data`; +/** `registry.registerObject` requires a package id (see the sibling real-stack tests). */ +const TEST_PACKAGE_ID = 'objectstack-test'; +const CALLER = { userId: 'u_member', isSystem: false }; + +const TASK = { + name: 'task', + label: 'Task', + fields: { + id: { name: 'id', type: 'text' as const, primaryKey: true, label: 'ID' }, + name: { name: 'name', type: 'text' as const, label: 'Name' }, + }, +}; + +const BEFORE = 'first'; +const AFTER = 'second'; + +const clone = (v: T): T => (v === undefined ? v : JSON.parse(JSON.stringify(v))); + +const liveEngines: ObjectQL[] = []; +afterEach(async () => { + while (liveEngines.length) { + try { await liveEngines.pop()?.destroy(); } catch { /* noop */ } + } +}); + +/** What the store holds after a request: `r1`'s name and version, and the row count. */ +interface StoreState { name: unknown; version: unknown; rows: number } + +/** One real store with one `task` row, `r1`, named {@link BEFORE}. */ +async function bootStore() { + const engine = new ObjectQL(); + liveEngines.push(engine); + engine.registerDriver(new SqlDriver({ + client: 'better-sqlite3', + connection: { filename: ':memory:' }, + useNullAsDefault: true, + }), true); + await engine.init(); + engine.registry.registerObject(TASK as any, TEST_PACKAGE_ID); + await engine.syncSchemas(); + const protocol = new ObjectStackProtocolImplementation(engine as any); + await protocol.createData({ object: 'task', data: { id: 'r1', name: BEFORE } } as any); + const updateData = vi.spyOn(protocol, 'updateData'); + const createData = vi.spyOn(protocol, 'createData'); + const state = async (): Promise => { + const row: any = await engine.findOne('task', { where: { id: 'r1' } }); + const rows = ((await engine.find('task', {})) as unknown[]).length; + const v = row?.updated_at; + return { name: row?.name, version: v instanceof Date ? v.toISOString() : v, rows }; + }; + return { protocol, updateData, createData, state }; +} + +// ── The two doors ───────────────────────────────────────────────────────────── + +interface Answer { status: number; code?: string; fields?: string[] } +interface Door { + patch(body: unknown): Promise; + create(body: unknown): Promise; + state(): Promise; + updateCalls(): number; + createCalls(): number; +} + +/** The refusal's `code` in either door's envelope: nested under `error`, or beside a string `error`. */ +const codeOf = (body: any): string | undefined => + body?.error && typeof body.error === 'object' ? body.error.code : body?.code; +/** The field names a refusal carries: `RestServer`'s top-level `fields`, the dispatcher's `error.details.fields`. */ +const fieldsOf = (body: any): string[] | undefined => { + const fields = Array.isArray(body?.fields) ? body.fields : body?.error?.details?.fields; + return Array.isArray(fields) ? fields.map((f: any) => f?.field).sort() : undefined; +}; + +function makeRes() { + const res: any = { statusCode: 200, body: undefined, headers: {} as Record }; + res.status = (code: number) => { res.statusCode = code; return res; }; + res.json = (body: any) => { res.body = body; return res; }; + res.send = () => res; + res.end = () => res; + res.header = (k: string, v: string) => { res.headers[k] = v; return res; }; + res.setHeader = () => {}; + res.write = () => true; + return res; +} + +async function bootRestServer(): Promise { + const { protocol, updateData, createData, state } = await bootStore(); + const noop = () => {}; + const server = { get: noop, post: noop, put: noop, delete: noop, patch: noop, use: noop, listen: async () => {}, close: async () => {} }; + const rest: any = new RestServer(server as any, protocol as any, { api: { requireAuth: false } } as any); + rest.resolveExecCtx = async () => clone(CALLER); + rest.registerRoutes(); + const routeOf = (method: string, path: string) => { + const route = rest.getRoutes().find((r: any) => r.method === method && r.path === path); + if (!route) throw new Error(`${method} ${path} is not registered`); + return route; + }; + const update = routeOf('PATCH', `${DATA}/:object/:id`); + const create = routeOf('POST', `${DATA}/:object`); + const drive = async (route: any, path: string, params: Record, body: unknown): Promise => { + const res = makeRes(); + // `plugin-hono-server` hands a JSON body over as `c.req.json()` parsed it: `null` stays `null`. + await route.handler({ method: route.method, path, params, headers: { 'content-type': 'application/json' }, query: {}, body: clone(body) }, res); + return { status: res.statusCode, code: codeOf(res.body), fields: fieldsOf(res.body) }; + }; + return { + state, + updateCalls: () => updateData.mock.calls.length, + createCalls: () => createData.mock.calls.length, + patch: (body) => drive(update, `${DATA}/task/r1`, { object: 'task', id: 'r1' }, body), + create: (body) => drive(create, `${DATA}/task`, { object: 'task' }, body), + }; +} + +/** + * What `createHonoApp`'s `${prefix}/*` catch-all hands `dispatch()` for a + * write: the path below the prefix, the JSON body parsed from the request's + * text (any failure is `{}`, the adapter's `readDispatchBody`), and the raw + * Fetch `Request` as `context.request`. + */ +async function catchAll(dispatcher: HttpDispatcher, method: string, path: string, body: unknown): Promise { + const raw = new Request(`http://localhost${PREFIX}${path}`, { + method, + headers: { 'content-type': 'application/json' }, + body: JSON.stringify(body), + }); + let parsed: unknown; + try { + parsed = JSON.parse(await raw.clone().text()); + } catch { + parsed = {}; + } + try { + const res: any = await dispatcher.dispatch(method, path, parsed, {}, { request: raw } as any, PREFIX); + const out = res.response?.body; + return { status: res.response?.status ?? 0, code: codeOf(out), fields: fieldsOf(out) }; + } catch (e) { + // A throw out of `dispatch()` is rendered by the host's exit; read it + // through the one rule the dispatcher's exits answer a throw with. + const thrown = resolveThrownHttpError(e); + return { status: thrown.status, code: thrown.code }; + } +} + +async function bootCatchAll(): Promise { + const { protocol, updateData, createData, state } = await bootStore(); + const services: Record = { protocol }; + const get = (n: string) => services[n] ?? null; + const kernel: any = { context: { getService: get }, getService: get, getServiceAsync: async (n: string) => get(n) }; + const dispatcher = new HttpDispatcher(kernel); + // The seam `dispatch()` resolves identity through (the sibling + // dispatch-level pins supply the principal the same way). + (dispatcher as any).timedResolveExecutionContext = async () => clone(CALLER); + return { + state, + updateCalls: () => updateData.mock.calls.length, + createCalls: () => createData.mock.calls.length, + patch: (body) => catchAll(dispatcher, 'PATCH', '/data/task/r1', body), + create: (body) => catchAll(dispatcher, 'POST', '/data/task', body), + }; +} + +const DOORS: ReadonlyArray Promise]> = [ + ['RestServer', bootRestServer], + ['the @objectstack/hono catch-all', bootCatchAll], +]; + +/** The JSON values a body can be that are not the record both request schemas declare for `data`. */ +const NOT_A_RECORD: ReadonlyArray = [ + ['an array', [{ name: AFTER }]], + ['a string', 'garbage'], + ['a number', 42], + ['null', null], +]; + +/** `400 VALIDATION_FAILED`, the refusal naming `data`: the request schema answered, not the engine. */ +const refusedOnData = (a: Answer) => + expect({ status: a.status, code: a.code, fields: a.fields }, JSON.stringify(a)) + .toEqual({ status: 400, code: 'VALIDATION_FAILED', fields: ['data'] }); + +// ── PATCH /data/:object/:id ─────────────────────────────────────────────────── + +for (const [doorName, boot] of DOORS) { + describe(`[#23491] ${doorName}: PATCH /data/:object/:id refuses a body that is not a record`, () => { + for (const [shape, body] of NOT_A_RECORD) { + it(`${shape}: 400 VALIDATION_FAILED on data, updateData never asked, the row and its version unchanged`, async () => { + const door = await boot(); + const before = await door.state(); + refusedOnData(await door.patch(body)); + expect(door.updateCalls()).toBe(0); + expect(await door.state()).toEqual(before); + }, 60_000); + } + + it('control: a record body updates the row (200)', async () => { + const door = await boot(); + const answer = await door.patch({ name: AFTER }); + expect(answer.status, JSON.stringify(answer)).toBe(200); + expect(door.updateCalls()).toBe(1); + expect((await door.state()).name).toBe(AFTER); + }, 60_000); + }); +} + +describe('[#23491] the two PATCH doors refuse each non-record body alike', () => { + for (const [shape, body] of NOT_A_RECORD) { + it(`${shape}: the same status, code and fields`, async () => { + const answers: Answer[] = []; + for (const [, boot] of DOORS) answers.push(await (await boot()).patch(body)); + const [rest, catchAllAnswer] = answers; + expect(catchAllAnswer).toEqual(rest); + }, 60_000); + } +}); + +// ── POST /data/:object ──────────────────────────────────────────────────────── + +/** The create's non-record bodies `RestServer`'s assembled request refuses; `null` is read as `{}` by both doors. */ +const CREATE_REFUSED = NOT_A_RECORD.filter(([, body]) => body !== null); + +for (const [doorName, boot] of DOORS) { + describe(`[#23491] ${doorName}: POST /data/:object refuses a body that is not a record`, () => { + for (const [shape, body] of CREATE_REFUSED) { + it(`${shape}: 400 VALIDATION_FAILED on data, createData never asked, no row created`, async () => { + const door = await boot(); + const before = await door.state(); + refusedOnData(await door.create(body)); + expect(door.createCalls()).toBe(0); + expect(await door.state()).toEqual(before); + }, 60_000); + } + + it('control: a record body creates one row (201)', async () => { + const door = await boot(); + const before = await door.state(); + const answer = await door.create({ name: AFTER }); + expect(answer.status, JSON.stringify(answer)).toBe(201); + expect((await door.state()).rows).toBe(before.rows + 1); + }, 60_000); + }); +} + +describe('[#23491] the two POST doors answer each non-record body alike', () => { + for (const [shape, body] of NOT_A_RECORD) { + it(`${shape}: the same status, code, fields and row count`, async () => { + const seen: Array = []; + for (const [, boot] of DOORS) { + const door = await boot(); + const answer = await door.create(body); + seen.push({ ...answer, rows: (await door.state()).rows }); + } + const [rest, catchAllAnswer] = seen; + expect(catchAllAnswer).toEqual(rest); + }, 60_000); + } +}); diff --git a/packages/runtime/src/domains/data.ts b/packages/runtime/src/domains/data.ts index 8804b5772d2..8fda57822d8 100644 --- a/packages/runtime/src/domains/data.ts +++ b/packages/runtime/src/domains/data.ts @@ -36,7 +36,11 @@ import type { DomainHandlerDeps, DomainRoute } from '../domain-handler-registry. import { assertFilterParamSuppliedOnce, repeatedQueryParamMessage } from '@objectstack/rest'; // [#23492] The dispatcher's one constructor for a `400 VALIDATION_FAILED` // answer, which a refused version token returns in this transport's envelope. -import { validationFailure, VALIDATION_FAILED_STATUS } from '../validation-failure.js'; +// [#23491] …and the one Zod-issue → `fields[]` mapping, which a write whose +// assembled protocol request does not parse returns the same way. +import { validationFailure, fieldsFromZodIssues, VALIDATION_FAILED_STATUS } from '../validation-failure.js'; +// [#23491] The request contracts `RestServer`'s twin routes validate (#3899). +import { CreateDataRequestSchema, UpdateDataRequestSchema } from '@objectstack/spec/api'; /** * [#23265] The request's `If-Match`, from either header shape a host hands @@ -84,6 +88,32 @@ function versionTokenRefusal(token: unknown, spelling: string): string | undefin + 'If-Match to perform an unguarded write.'; } +/** + * [#23491] This domain's answer to a `PATCH` or `POST` create whose assembled + * protocol request (`UpdateDataRequestSchema` / `CreateDataRequestSchema`) + * does not parse: `400 VALIDATION_FAILED`, `details.fields[]` naming each + * failing member, before `callData` is asked. `RestServer`'s twin routes gate + * the same assembled request (#3899); this domain had no gate, so a body that + * is not a record (an array, a string, a number, `null`) reached the engine + * as the patch or the row, and answered `500` with the engine's own error, or + * created rows. RETURNED, not thrown, for the reason `versionTokenRefusal`'s + * refusal is: a throw out of `dispatch()` reaches a `createHonoApp` host's + * catch-all, which renders only some declared statuses. + */ +function writeRequestRefusal( + deps: DomainHandlerDeps, + write: 'update' | 'create', + issues: Array<{ path: Array; code: string; message: string }>, + input: unknown, +): HttpDispatcherResult { + const fields = fieldsFromZodIssues(issues, input); + const message = `Invalid ${write} request: ${fields.map((f) => `${f.field}: ${f.message}`).join('; ')}`; + return { + handled: true, + response: deps.errorFromThrown(validationFailure(message, fields), VALIDATION_FAILED_STATUS), + }; +} + export function createDataDomain(deps: DomainHandlerDeps): DomainRoute { return { prefix: '/data', @@ -231,13 +261,27 @@ export async function handleDataRequest(deps: DomainHandlerDeps, path: string, m response: deps.errorFromThrown(validationFailure(versionRefusal, []), VALIDATION_FAILED_STATUS), }; } - // Spec: returns UpdateDataResponse = { object, id, record } - const result = await actionExec.callData(deps, _context, 'update', { + // [#23491] The assembled request is validated as + // `UpdateDataRequestSchema`, the gate `RestServer`'s twin has + // asked since #3899, over the patch AS THE CALLER SENT IT (the + // token stripped, nothing defaulted): a body that is not a record + // is refused `400` here, before the engine is asked. It used to + // reach `updateData` as the patch and answer `500` with the + // engine's own error. A JSON `null` is refused like the rest: the + // twin used to read it as `{}` and answer `200`, an update that + // changed no field and still moved the record's version; both + // doors now refuse it. + const updateInput = { object: objectName, id, data, ...(expectedVersion != null ? { expectedVersion: String(expectedVersion) } : {}), - }, _context.dataDriver, _context.environmentId, _context.executionContext); + }; + const parsedUpdate = UpdateDataRequestSchema.safeParse(updateInput); + if (!parsedUpdate.success) return writeRequestRefusal(deps, 'update', parsedUpdate.error.issues, updateInput); + // Spec: returns UpdateDataResponse = { object, id, record } + const result = await actionExec.callData(deps, _context, 'update', updateInput, + _context.dataDriver, _context.environmentId, _context.executionContext); return { handled: true, response: deps.success(result) }; } @@ -327,8 +371,21 @@ export async function handleDataRequest(deps: DomainHandlerDeps, path: string, m // POST /data/:object (Create) if (m === 'POST') { + // [#23491] The `PATCH` above's gate, on the create: the assembled + // request is validated as `CreateDataRequestSchema` before the + // engine is asked. This branch had no gate, so an array body + // created one row per element (`201`), a number created an empty + // row (`201`), and a string answered the engine's own unknown-field + // `400`; `RestServer`'s twin refused all three `400 + // VALIDATION_FAILED` (#3899). The assembled request is the twin's, + // statement for statement, `null` read as `{}` included: both doors + // answer a JSON `null` create alike (an empty row), and that + // leniency is retired on both at once or not at all. + const createInput = { object: objectName, data: body ?? {} }; + const parsedCreate = CreateDataRequestSchema.safeParse(createInput); + if (!parsedCreate.success) return writeRequestRefusal(deps, 'create', parsedCreate.error.issues, createInput); // Spec: returns CreateDataResponse = { object, id, record } - const result = await actionExec.callData(deps, _context, 'create', { object: objectName, data: body }, _context.dataDriver, _context.environmentId, _context.executionContext); + const result = await actionExec.callData(deps, _context, 'create', createInput, _context.dataDriver, _context.environmentId, _context.executionContext); const res = deps.success(result); res.status = 201; return { handled: true, response: res }; From 3dd76b2a817eb8496a04e25b0c92e3ad2bd3a3d9 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 15 Oct 2026 00:06:50 +0000 Subject: [PATCH 2/2] fix(rest,runtime)!: a number expectedVersion on a /data PATCH is refused 400 on both doors UpdateDataRequestSchema.expectedVersion declares a string. Both PATCH doors judged a body number as its String() and answered 409 CONCURRENT_UPDATE; RestServer's request gate only ever parsed the coerced copy. A number now joins the token shapes that can never carry a version: versionTokenRefusal (both copies, statement for statement) refuses it 400 VALIDATION_FAILED before the protocol is asked, and both doors gate and forward the token as the caller sent it. If-Match and the DELETE's ?expectedVersion are strings by transport and are unaffected. Co-Authored-By: Claude Claude-Session: https://claude.ai/code/session_01FLwgbTSmGMrqsXkBDsiwfG --- .changeset/23491-patch-data-request-gate.md | 27 ++++++ packages/rest/src/rest-server.ts | 45 ++++++---- .../data-update-expected-version.test.ts | 83 +++++++++++++++++-- packages/runtime/src/domains/data.ts | 31 ++++--- 4 files changed, 154 insertions(+), 32 deletions(-) create mode 100644 .changeset/23491-patch-data-request-gate.md diff --git a/.changeset/23491-patch-data-request-gate.md b/.changeset/23491-patch-data-request-gate.md new file mode 100644 index 00000000000..686fac2c406 --- /dev/null +++ b/.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 + + + +**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`. diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index afb6005676a..a22d641127c 100644 --- a/packages/rest/src/rest-server.ts +++ b/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.'; } /** @@ -9182,12 +9189,20 @@ export class RestServer { // 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, - ...(expectedVersion != null ? { expectedVersion: String(expectedVersion) } : {}), + ...(expectedVersion != null ? { expectedVersion } : {}), }; const parsedUpdateOne = (UpdateDataRequestSchema as any).safeParse(updateInput); if (!parsedUpdateOne.success) { @@ -9203,7 +9218,7 @@ export class RestServer { object: req.params.object, id: req.params.id, data, - ...(expectedVersion != null ? { expectedVersion: String(expectedVersion) } : {}), + ...(expectedVersion != null ? { expectedVersion } : {}), ...(environmentId ? { environmentId } : {}), ...(context ? { context } : {}), }; diff --git a/packages/runtime/src/domains/data-update-expected-version.test.ts b/packages/runtime/src/domains/data-update-expected-version.test.ts index 2a8ae9866ec..26dd3571f2b 100644 --- a/packages/runtime/src/domains/data-update-expected-version.test.ts +++ b/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 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 = [ ["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 = [['5', 5], ['0', 0]]; +const IF_MATCH_BESIDE: ReadonlyArray 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 ───────────────────────────────────────── /** diff --git a/packages/runtime/src/domains/data.ts b/packages/runtime/src/domains/data.ts index 8fda57822d8..b2eaf6bfffa 100644 --- a/packages/runtime/src/domains/data.ts +++ b/packages/runtime/src/domains/data.ts @@ -61,12 +61,18 @@ function requestIfMatch(headers: unknown): unknown { * [#23492] What this domain's `PATCH` and `DELETE` do with the * optimistic-concurrency token they read, before `callData` is asked: absent * (`undefined` or `null`, which falls through to `If-Match`), judged (a string - * with something in it, or a finite number, forwarded as its string), or - * refused `400 VALIDATION_FAILED` (an empty or blank string, a boolean, an - * array, an object: a token shape that can never carry a version, maintainer - * decision #20 ①). It replaces a truthiness test that let `expectedVersion: ''` - * or `false`, and `?expectedVersion=`, hide a stale `If-Match` and run the - * write unguarded. + * with something in it, forwarded as it is), or refused `400 + * VALIDATION_FAILED` (an empty or blank string, a number, a boolean, an array, + * an object: a token shape that can never carry a version, maintainer decision + * #20 ①). It replaces a truthiness test that let `expectedVersion: ''` or + * `false`, and `?expectedVersion=`, hide a stale `If-Match` and run the write + * unguarded. + * + * [#23517] A number is refused, no longer judged as its string: + * `UpdateDataRequestSchema.expectedVersion` declares a string, and forwarding + * `String(5)` answered `409` for a token the contract never admits. Only a + * body token can be a number; `If-Match` and `?expectedVersion` are strings by + * transport. * * `RestServer`'s twin routes ask the same function, statement for statement * (`versionTokenRefusal` in `packages/rest/src/rest-server.ts`, whose docblock @@ -78,14 +84,13 @@ function requestIfMatch(headers: unknown): unknown { 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.'; } /** @@ -271,11 +276,17 @@ export async function handleDataRequest(deps: DomainHandlerDeps, path: string, m // twin used to read it as `{}` and answer `200`, an update that // changed no field and still moved the record's version; both // doors now refuse it. + // + // [#23517] …and the token as the caller sent it too, never its + // `String()`: the gate parsed `String(5)`, so a number the schema + // 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 updateInput = { object: objectName, id, data, - ...(expectedVersion != null ? { expectedVersion: String(expectedVersion) } : {}), + ...(expectedVersion != null ? { expectedVersion } : {}), }; const parsedUpdate = UpdateDataRequestSchema.safeParse(updateInput); if (!parsedUpdate.success) return writeRequestRefusal(deps, 'update', parsedUpdate.error.issues, updateInput);