Skip to content

Commit 90424dd

Browse files
committed
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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01FLwgbTSmGMrqsXkBDsiwfG
1 parent 086fa46 commit 90424dd

4 files changed

Lines changed: 388 additions & 8 deletions

File tree

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

Lines changed: 3 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -131,7 +131,9 @@ const CASES: GateCase[] = [
131131
{
132132
route: 'PATCH /api/v1/data/:object/:id',
133133
params: PARAMS_ID,
134-
violating: ['garbage', 42, [{ name: 'x' }]],
134+
// #23491 — `null` was read as an empty patch (`data ?? {}`) and answered
135+
// 200, an update that changed no field and still moved the version.
136+
violating: ['garbage', 42, [{ name: 'x' }], null],
135137
valid: { name: 'ACME (renamed)' },
136138
spy: 'updateData',
137139
},

‎packages/rest/src/rest-server.ts‎

Lines changed: 11 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -9173,11 +9173,20 @@ export class RestServer {
91739173
// validated as the assembled `UpdateDataRequestSchema`
91749174
// request so a non-record body 400s instead of reaching
91759175
// the engine.
9176+
//
9177+
// [#23491] …the patch AS THE CALLER SENT IT. It was
9178+
// `data ?? {}`, so a JSON `null` body parsed as an empty
9179+
// patch and answered `200`: an update that changed no
9180+
// field and still moved the record's version. `null` is
9181+
// not the record the contract declares, so it is refused
9182+
// like an array, a string or a number, and the runtime
9183+
// dispatcher's twin (`domains/data.ts`) asks this gate
9184+
// over the same assembled request.
91769185
const { UpdateDataRequestSchema } = await import('@objectstack/spec/api');
91779186
const updateInput = {
91789187
object: req.params.object,
91799188
id: req.params.id,
9180-
data: data ?? {},
9189+
data,
91819190
...(expectedVersion != null ? { expectedVersion: String(expectedVersion) } : {}),
91829191
};
91839192
const parsedUpdateOne = (UpdateDataRequestSchema as any).safeParse(updateInput);
@@ -9193,7 +9202,7 @@ export class RestServer {
91939202
const updateRequest: ServerScopedDataRequest<UpdateDataRequest> = {
91949203
object: req.params.object,
91959204
id: req.params.id,
9196-
data: data ?? {},
9205+
data,
91979206
...(expectedVersion != null ? { expectedVersion: String(expectedVersion) } : {}),
91989207
...(environmentId ? { environmentId } : {}),
91999208
...(context ? { context } : {}),
Lines changed: 312 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,312 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* [#23491] Both doors of the `/data` single-record writes validate the
5+
* assembled protocol request before the engine is asked: `RestServer`'s
6+
* registered routes and the runtime dispatcher's `/data` domain, the only
7+
* answer on a host that mounts just `@objectstack/hono`'s `${prefix}/*`
8+
* catch-all.
9+
*
10+
* ## What was wrong
11+
*
12+
* `RestServer` validates `PATCH /data/:object/:id` as `UpdateDataRequestSchema`
13+
* and `POST /data/:object` as `CreateDataRequestSchema` (#3899), so a body
14+
* that is not a record answers `400 VALIDATION_FAILED`. The dispatcher's two
15+
* branches had no gate. Measured on this rig before the fix:
16+
*
17+
* - `PATCH` with an array, a string, a number or `null` answered `500
18+
* INTERNAL_ERROR` with the engine's own error text ("no such column: 0",
19+
* "Cannot use 'in' operator …", "Cannot read properties of null …"),
20+
* after `updateData` was asked;
21+
* - `POST` with an array CREATED a row per element (`201`), a number created
22+
* an empty row (`201`), and a string answered the engine's unknown-field
23+
* `400 INVALID_FIELD`;
24+
* - `RestServer`'s `PATCH` read a `null` body as `{}` (`data ?? {}`) and
25+
* answered `200`: an update that changed no field and still moved the
26+
* record's `updated_at`, so every other holder of the old version was
27+
* turned stale by a request that wrote nothing.
28+
*
29+
* The `null` body now answers `400` on both `PATCH` doors: the declared type
30+
* is a record, and one door reading it as `{}` was the consumer-side leniency
31+
* the contract-first rule forbids. A `null` CREATE is the one shape left as it
32+
* was: `RestServer`'s create reads it as `{}`, the dispatcher's gate asks the
33+
* same assembled request, and the two doors answer it alike (pinned below as
34+
* parity only).
35+
*
36+
* ## The rig
37+
*
38+
* The one `data-update-expected-version.test.ts` describes: each door gets
39+
* its own REAL store (better-sqlite3 `:memory:`, the real `ObjectQL` and a
40+
* real `ObjectStackProtocolImplementation`); `RestServer` is driven through
41+
* its registered handler with the request `plugin-hono-server` builds, and
42+
* the dispatcher through `HttpDispatcher.dispatch()` with the body parsed the
43+
* way the catch-all parses it (`JSON.parse` of the request text, any failure
44+
* `{}`). Every row asserts the store, not just the door's word.
45+
*/
46+
47+
import { describe, it, expect, vi, afterEach } from 'vitest';
48+
import { ObjectQL } from '@objectstack/objectql';
49+
import { SqlDriver } from '@objectstack/driver-sql';
50+
import { ObjectStackProtocolImplementation } from '@objectstack/metadata-protocol';
51+
import { RestServer } from '@objectstack/rest';
52+
import { resolveThrownHttpError } from '@objectstack/types';
53+
import { HttpDispatcher } from '../http-dispatcher.js';
54+
55+
const PREFIX = '/api/v1';
56+
const DATA = `${PREFIX}/data`;
57+
/** `registry.registerObject` requires a package id (see the sibling real-stack tests). */
58+
const TEST_PACKAGE_ID = 'objectstack-test';
59+
const CALLER = { userId: 'u_member', isSystem: false };
60+
61+
const TASK = {
62+
name: 'task',
63+
label: 'Task',
64+
fields: {
65+
id: { name: 'id', type: 'text' as const, primaryKey: true, label: 'ID' },
66+
name: { name: 'name', type: 'text' as const, label: 'Name' },
67+
},
68+
};
69+
70+
const BEFORE = 'first';
71+
const AFTER = 'second';
72+
73+
const clone = <T>(v: T): T => (v === undefined ? v : JSON.parse(JSON.stringify(v)));
74+
75+
const liveEngines: ObjectQL[] = [];
76+
afterEach(async () => {
77+
while (liveEngines.length) {
78+
try { await liveEngines.pop()?.destroy(); } catch { /* noop */ }
79+
}
80+
});
81+
82+
/** What the store holds after a request: `r1`'s name and version, and the row count. */
83+
interface StoreState { name: unknown; version: unknown; rows: number }
84+
85+
/** One real store with one `task` row, `r1`, named {@link BEFORE}. */
86+
async function bootStore() {
87+
const engine = new ObjectQL();
88+
liveEngines.push(engine);
89+
engine.registerDriver(new SqlDriver({
90+
client: 'better-sqlite3',
91+
connection: { filename: ':memory:' },
92+
useNullAsDefault: true,
93+
}), true);
94+
await engine.init();
95+
engine.registry.registerObject(TASK as any, TEST_PACKAGE_ID);
96+
await engine.syncSchemas();
97+
const protocol = new ObjectStackProtocolImplementation(engine as any);
98+
await protocol.createData({ object: 'task', data: { id: 'r1', name: BEFORE } } as any);
99+
const updateData = vi.spyOn(protocol, 'updateData');
100+
const createData = vi.spyOn(protocol, 'createData');
101+
const state = async (): Promise<StoreState> => {
102+
const row: any = await engine.findOne('task', { where: { id: 'r1' } });
103+
const rows = ((await engine.find('task', {})) as unknown[]).length;
104+
const v = row?.updated_at;
105+
return { name: row?.name, version: v instanceof Date ? v.toISOString() : v, rows };
106+
};
107+
return { protocol, updateData, createData, state };
108+
}
109+
110+
// ── The two doors ─────────────────────────────────────────────────────────────
111+
112+
interface Answer { status: number; code?: string; fields?: string[] }
113+
interface Door {
114+
patch(body: unknown): Promise<Answer>;
115+
create(body: unknown): Promise<Answer>;
116+
state(): Promise<StoreState>;
117+
updateCalls(): number;
118+
createCalls(): number;
119+
}
120+
121+
/** The refusal's `code` in either door's envelope: nested under `error`, or beside a string `error`. */
122+
const codeOf = (body: any): string | undefined =>
123+
body?.error && typeof body.error === 'object' ? body.error.code : body?.code;
124+
/** The field names a refusal carries: `RestServer`'s top-level `fields`, the dispatcher's `error.details.fields`. */
125+
const fieldsOf = (body: any): string[] | undefined => {
126+
const fields = Array.isArray(body?.fields) ? body.fields : body?.error?.details?.fields;
127+
return Array.isArray(fields) ? fields.map((f: any) => f?.field).sort() : undefined;
128+
};
129+
130+
function makeRes() {
131+
const res: any = { statusCode: 200, body: undefined, headers: {} as Record<string, string> };
132+
res.status = (code: number) => { res.statusCode = code; return res; };
133+
res.json = (body: any) => { res.body = body; return res; };
134+
res.send = () => res;
135+
res.end = () => res;
136+
res.header = (k: string, v: string) => { res.headers[k] = v; return res; };
137+
res.setHeader = () => {};
138+
res.write = () => true;
139+
return res;
140+
}
141+
142+
async function bootRestServer(): Promise<Door> {
143+
const { protocol, updateData, createData, state } = await bootStore();
144+
const noop = () => {};
145+
const server = { get: noop, post: noop, put: noop, delete: noop, patch: noop, use: noop, listen: async () => {}, close: async () => {} };
146+
const rest: any = new RestServer(server as any, protocol as any, { api: { requireAuth: false } } as any);
147+
rest.resolveExecCtx = async () => clone(CALLER);
148+
rest.registerRoutes();
149+
const routeOf = (method: string, path: string) => {
150+
const route = rest.getRoutes().find((r: any) => r.method === method && r.path === path);
151+
if (!route) throw new Error(`${method} ${path} is not registered`);
152+
return route;
153+
};
154+
const update = routeOf('PATCH', `${DATA}/:object/:id`);
155+
const create = routeOf('POST', `${DATA}/:object`);
156+
const drive = async (route: any, path: string, params: Record<string, string>, body: unknown): Promise<Answer> => {
157+
const res = makeRes();
158+
// `plugin-hono-server` hands a JSON body over as `c.req.json()` parsed it: `null` stays `null`.
159+
await route.handler({ method: route.method, path, params, headers: { 'content-type': 'application/json' }, query: {}, body: clone(body) }, res);
160+
return { status: res.statusCode, code: codeOf(res.body), fields: fieldsOf(res.body) };
161+
};
162+
return {
163+
state,
164+
updateCalls: () => updateData.mock.calls.length,
165+
createCalls: () => createData.mock.calls.length,
166+
patch: (body) => drive(update, `${DATA}/task/r1`, { object: 'task', id: 'r1' }, body),
167+
create: (body) => drive(create, `${DATA}/task`, { object: 'task' }, body),
168+
};
169+
}
170+
171+
/**
172+
* What `createHonoApp`'s `${prefix}/*` catch-all hands `dispatch()` for a
173+
* write: the path below the prefix, the JSON body parsed from the request's
174+
* text (any failure is `{}`, the adapter's `readDispatchBody`), and the raw
175+
* Fetch `Request` as `context.request`.
176+
*/
177+
async function catchAll(dispatcher: HttpDispatcher, method: string, path: string, body: unknown): Promise<Answer> {
178+
const raw = new Request(`http://localhost${PREFIX}${path}`, {
179+
method,
180+
headers: { 'content-type': 'application/json' },
181+
body: JSON.stringify(body),
182+
});
183+
let parsed: unknown;
184+
try {
185+
parsed = JSON.parse(await raw.clone().text());
186+
} catch {
187+
parsed = {};
188+
}
189+
try {
190+
const res: any = await dispatcher.dispatch(method, path, parsed, {}, { request: raw } as any, PREFIX);
191+
const out = res.response?.body;
192+
return { status: res.response?.status ?? 0, code: codeOf(out), fields: fieldsOf(out) };
193+
} catch (e) {
194+
// A throw out of `dispatch()` is rendered by the host's exit; read it
195+
// through the one rule the dispatcher's exits answer a throw with.
196+
const thrown = resolveThrownHttpError(e);
197+
return { status: thrown.status, code: thrown.code };
198+
}
199+
}
200+
201+
async function bootCatchAll(): Promise<Door> {
202+
const { protocol, updateData, createData, state } = await bootStore();
203+
const services: Record<string, unknown> = { protocol };
204+
const get = (n: string) => services[n] ?? null;
205+
const kernel: any = { context: { getService: get }, getService: get, getServiceAsync: async (n: string) => get(n) };
206+
const dispatcher = new HttpDispatcher(kernel);
207+
// The seam `dispatch()` resolves identity through (the sibling
208+
// dispatch-level pins supply the principal the same way).
209+
(dispatcher as any).timedResolveExecutionContext = async () => clone(CALLER);
210+
return {
211+
state,
212+
updateCalls: () => updateData.mock.calls.length,
213+
createCalls: () => createData.mock.calls.length,
214+
patch: (body) => catchAll(dispatcher, 'PATCH', '/data/task/r1', body),
215+
create: (body) => catchAll(dispatcher, 'POST', '/data/task', body),
216+
};
217+
}
218+
219+
const DOORS: ReadonlyArray<readonly [string, () => Promise<Door>]> = [
220+
['RestServer', bootRestServer],
221+
['the @objectstack/hono catch-all', bootCatchAll],
222+
];
223+
224+
/** The JSON values a body can be that are not the record both request schemas declare for `data`. */
225+
const NOT_A_RECORD: ReadonlyArray<readonly [string, unknown]> = [
226+
['an array', [{ name: AFTER }]],
227+
['a string', 'garbage'],
228+
['a number', 42],
229+
['null', null],
230+
];
231+
232+
/** `400 VALIDATION_FAILED`, the refusal naming `data`: the request schema answered, not the engine. */
233+
const refusedOnData = (a: Answer) =>
234+
expect({ status: a.status, code: a.code, fields: a.fields }, JSON.stringify(a))
235+
.toEqual({ status: 400, code: 'VALIDATION_FAILED', fields: ['data'] });
236+
237+
// ── PATCH /data/:object/:id ───────────────────────────────────────────────────
238+
239+
for (const [doorName, boot] of DOORS) {
240+
describe(`[#23491] ${doorName}: PATCH /data/:object/:id refuses a body that is not a record`, () => {
241+
for (const [shape, body] of NOT_A_RECORD) {
242+
it(`${shape}: 400 VALIDATION_FAILED on data, updateData never asked, the row and its version unchanged`, async () => {
243+
const door = await boot();
244+
const before = await door.state();
245+
refusedOnData(await door.patch(body));
246+
expect(door.updateCalls()).toBe(0);
247+
expect(await door.state()).toEqual(before);
248+
}, 60_000);
249+
}
250+
251+
it('control: a record body updates the row (200)', async () => {
252+
const door = await boot();
253+
const answer = await door.patch({ name: AFTER });
254+
expect(answer.status, JSON.stringify(answer)).toBe(200);
255+
expect(door.updateCalls()).toBe(1);
256+
expect((await door.state()).name).toBe(AFTER);
257+
}, 60_000);
258+
});
259+
}
260+
261+
describe('[#23491] the two PATCH doors refuse each non-record body alike', () => {
262+
for (const [shape, body] of NOT_A_RECORD) {
263+
it(`${shape}: the same status, code and fields`, async () => {
264+
const answers: Answer[] = [];
265+
for (const [, boot] of DOORS) answers.push(await (await boot()).patch(body));
266+
const [rest, catchAllAnswer] = answers;
267+
expect(catchAllAnswer).toEqual(rest);
268+
}, 60_000);
269+
}
270+
});
271+
272+
// ── POST /data/:object ────────────────────────────────────────────────────────
273+
274+
/** The create's non-record bodies `RestServer`'s assembled request refuses; `null` is read as `{}` by both doors. */
275+
const CREATE_REFUSED = NOT_A_RECORD.filter(([, body]) => body !== null);
276+
277+
for (const [doorName, boot] of DOORS) {
278+
describe(`[#23491] ${doorName}: POST /data/:object refuses a body that is not a record`, () => {
279+
for (const [shape, body] of CREATE_REFUSED) {
280+
it(`${shape}: 400 VALIDATION_FAILED on data, createData never asked, no row created`, async () => {
281+
const door = await boot();
282+
const before = await door.state();
283+
refusedOnData(await door.create(body));
284+
expect(door.createCalls()).toBe(0);
285+
expect(await door.state()).toEqual(before);
286+
}, 60_000);
287+
}
288+
289+
it('control: a record body creates one row (201)', async () => {
290+
const door = await boot();
291+
const before = await door.state();
292+
const answer = await door.create({ name: AFTER });
293+
expect(answer.status, JSON.stringify(answer)).toBe(201);
294+
expect((await door.state()).rows).toBe(before.rows + 1);
295+
}, 60_000);
296+
});
297+
}
298+
299+
describe('[#23491] the two POST doors answer each non-record body alike', () => {
300+
for (const [shape, body] of NOT_A_RECORD) {
301+
it(`${shape}: the same status, code, fields and row count`, async () => {
302+
const seen: Array<Answer & { rows: number }> = [];
303+
for (const [, boot] of DOORS) {
304+
const door = await boot();
305+
const answer = await door.create(body);
306+
seen.push({ ...answer, rows: (await door.state()).rows });
307+
}
308+
const [rest, catchAllAnswer] = seen;
309+
expect(catchAllAnswer).toEqual(rest);
310+
}, 60_000);
311+
}
312+
});

0 commit comments

Comments
 (0)