Skip to content

Commit 4dda4e9

Browse files
baozhoutaoclaude
andcommitted
fix(rest): route batch creates through the create ingress so readonly is enforced (#3835)
`POST /batch` called `ql.insert` directly, and the engine's INSERT path is static-`readonly`-exempt by design (#3413) — the strip that stops a non-system caller from seeding a read-only column lives at the protocol's create ingress (#3043). So the same forged value was dropped on `POST /data/:object` and written through on `/batch`: one rule, two answers. Create ops now go through `p.createData`, the ingress itself, rather than a second copy of the strip at the REST layer. One ingress means a future change to its policy covers the batch for free, and the carve-outs it already encodes stay intact — the platform-object exemption (a `sys_`/`managedBy` object's own guard must REJECT a forged value, not silently swallow it) and the `isSystem` exemption. `trxCtx` is passed as the context, so the insert still joins the batch transaction and `$ref` resolution is unaffected; the ingress's `droppedFields` fold into the batch's per-op list. Update ops are untouched — the engine enforces both strips on its update path. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
1 parent 18516c8 commit 4dda4e9

3 files changed

Lines changed: 155 additions & 3 deletions

File tree

Lines changed: 39 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,39 @@
1+
---
2+
"@objectstack/rest": minor
3+
---
4+
5+
fix(rest): a batch create goes through the same create ingress as a single create (#3835)
6+
7+
`readonly` meant two different things depending on which create endpoint you
8+
used. `POST /data/:object` runs the #3043 ingress strip, so a non-system caller
9+
cannot seed a read-only column — the field is dropped and reported. The
10+
cross-object transactional batch (`POST /batch`) called `ql.insert` directly and
11+
skipped that ingress entirely, and the engine's INSERT path is
12+
static-`readonly`-exempt **by design** (#3413, the strip lives one layer up), so
13+
nothing enforced it: the same forged `readonly` value that was rejected on one
14+
route was written through on the other.
15+
16+
Measured on the showcase (`showcase_contact.lead_score`, `readonly: true`, same
17+
signed-in non-system user):
18+
19+
| | before | after |
20+
|---|---|---|
21+
| `POST /data/showcase_contact` | `lead_score = null`, reported | unchanged |
22+
| `POST /batch` create | **`lead_score = 999` written, silent** | `null`, reported |
23+
24+
The fix routes the batch's create ops through the protocol's `createData` rather
25+
than re-implementing the strip at the REST layer. That keeps **one** create
26+
ingress: a future change to its policy covers the batch for free, and the
27+
carve-outs already encoded there stay intact — notably the platform-object
28+
exemption (a `sys_`/`managedBy` object's own guard must *reject* a forged value
29+
with 403, not have it silently swallowed) and the `isSystem` exemption. The
30+
context passed through is the transaction context, so the insert still joins the
31+
batch transaction and rolls back with it, and `{ $ref: <opIndex> }` resolution is
32+
unaffected.
33+
34+
`createData`'s `droppedFields` are folded into the batch response's per-op
35+
`droppedFields` list (#3794), so a batch create now reports its strips the same
36+
way an update does.
37+
38+
Update ops are untouched: the engine enforces `readonly` and `readonlyWhen` on
39+
its own update path.

packages/rest/src/rest-batch-endpoint.test.ts

Lines changed: 88 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -42,13 +42,42 @@ function makeQl(overrides: Partial<Record<'insert' | 'update' | 'delete', any>>
4242
return ql;
4343
}
4444

45+
/**
46+
* A fake `createData` standing in for the protocol's create ingress (#3835):
47+
* the batch's create ops go through it rather than calling `ql.insert`, so the
48+
* #3043 static-`readonly` strip applies to a batch create exactly as it does to
49+
* `POST /data/:object`. Delegates to the same `ql.insert` the other ops use, so
50+
* assertions about what reached the engine still read off `ql.insert`.
51+
*/
52+
function makeCreateData(ql: any, opts: { readonlyFields?: string[] } = {}) {
53+
const readonlyFields = opts.readonlyFields ?? [];
54+
return vi.fn(async (request: any) => {
55+
const supplied = request?.data ?? {};
56+
const stripped: string[] = readonlyFields.filter((f) => f in supplied);
57+
const data = { ...supplied };
58+
for (const f of stripped) delete data[f];
59+
const record = await ql.insert(request.object, data, { context: request.context });
60+
return {
61+
object: request.object,
62+
id: record?.id,
63+
record,
64+
...(stripped.length > 0
65+
? { droppedFields: [{ object: request.object, fields: stripped, reason: 'readonly' }] }
66+
: {}),
67+
};
68+
});
69+
}
70+
4571
/** Build a RestServer with an optional ObjectQL provider and object metadata. */
46-
function buildServer(opts: { ql?: any; objects?: any[] } = {}) {
72+
function buildServer(opts: { ql?: any; objects?: any[]; readonlyFields?: string[] } = {}) {
4773
const server = mockServer();
4874
const protocol: any = {
4975
getDiscovery: vi.fn().mockResolvedValue({ version: 'v0', endpoints: {} }),
5076
getMetaTypes: vi.fn().mockResolvedValue([]),
5177
getMetaItems: vi.fn().mockResolvedValue(opts.objects ?? []),
78+
createData: opts.ql
79+
? makeCreateData(opts.ql, { readonlyFields: opts.readonlyFields ?? [] })
80+
: vi.fn(),
5281
};
5382
const objectQLProvider = opts.ql ? async () => opts.ql : undefined;
5483
const rest = new RestServer(
@@ -258,6 +287,64 @@ describe('POST {basePath}/batch — cross-object transactional batch', () => {
258287
expect(res.body).not.toHaveProperty('droppedFields');
259288
});
260289

290+
// ── create ingress parity (#3835) ─────────────────────────────────────────
291+
//
292+
// The engine's INSERT path is static-`readonly`-exempt by design (#3413), so
293+
// the #3043 strip that stops a non-system caller from seeding a read-only
294+
// column lives at the protocol's create ingress. This route used to call
295+
// `ql.insert` directly and skip it, so `readonly` meant two different things
296+
// depending on which create endpoint you used.
297+
298+
it('routes create ops through the protocol create ingress, not ql.insert', async () => {
299+
const ql = makeQl();
300+
const { route, protocol } = buildServer({ ql });
301+
const res = await post(route, {
302+
operations: [{ object: 'account', action: 'create', data: { name: 'Acme' } }],
303+
});
304+
expect(res.statusCode).toBe(200);
305+
expect(protocol.createData).toHaveBeenCalledTimes(1);
306+
const request = protocol.createData.mock.calls[0][0];
307+
expect(request).toMatchObject({ object: 'account', data: { name: 'Acme' } });
308+
// The ingress must run INSIDE this transaction — same context the other ops
309+
// bind, or the create would commit independently of the rollback.
310+
expect(request.context).toMatchObject({ __trx: true });
311+
});
312+
313+
it('strips a caller-forged readonly column on a batch create and reports it', async () => {
314+
const ql = makeQl();
315+
const { route } = buildServer({ ql, readonlyFields: ['approval_status'] });
316+
const res = await post(route, {
317+
operations: [
318+
{ object: 'account', action: 'create', data: { name: 'Acme', approval_status: 'approved' } },
319+
],
320+
});
321+
expect(res.statusCode).toBe(200);
322+
// Never reached the engine…
323+
expect(ql.insert).toHaveBeenCalledWith('account', { name: 'Acme' }, expect.anything());
324+
// …and the caller is told, tagged with the op that dropped it.
325+
expect(res.body.droppedFields).toEqual([
326+
{ object: 'account', fields: ['approval_status'], reason: 'readonly', index: 0 },
327+
]);
328+
});
329+
330+
it('keeps $ref resolution working through the ingress', async () => {
331+
// `results` now holds the ingress's echoed record rather than the raw
332+
// engine return — a later op must still resolve `{ $ref: 0 }` to its id.
333+
const ql = makeQl();
334+
const { route } = buildServer({ ql });
335+
const res = await post(route, {
336+
operations: [
337+
{ object: 'project', action: 'create', data: { name: 'Apollo' } },
338+
{ object: 'task', action: 'create', data: { title: 'Kickoff', project: { $ref: 0 } } },
339+
],
340+
});
341+
expect(res.statusCode).toBe(200);
342+
expect(res.body.results[0].id).toBe('id_1');
343+
expect(ql.insert).toHaveBeenLastCalledWith(
344+
'task', { title: 'Kickoff', project: 'id_1' }, expect.anything(),
345+
);
346+
});
347+
261348
// ── request validation ────────────────────────────────────────────────────
262349

263350
it('rejects a non-atomic request (this endpoint is always atomic)', async () => {

packages/rest/src/rest-server.ts

Lines changed: 28 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -6609,10 +6609,36 @@ export class RestServer {
66096609
const out: any[] = [];
66106610
for (const [index, op] of ops.entries()) {
66116611
const data = resolveRefs(op.data, out);
6612-
const onFieldsDropped = (e: DroppedFieldsEvent) => { dropped.push({ ...e, index }); };
66136612
if (op.action === 'create') {
6614-
out.push(await ql.insert(op.object, data, { context: trxCtx, onFieldsDropped }));
6613+
// [#3835] Go through the protocol's create ingress —
6614+
// the SAME one `POST /data/:object` uses — rather than
6615+
// calling `ql.insert` directly. The engine's INSERT path
6616+
// is static-`readonly`-exempt by design (#3413), so the
6617+
// #3043 strip that stops a non-system caller from seeding
6618+
// a read-only column lives at that ingress. Bypassing it
6619+
// here made `readonly` mean two different things on two
6620+
// create paths: rejected on the single route, written
6621+
// through the batch. `createData` also owns the platform-
6622+
// object carve-out (a `sys_`/`managedBy` object's own
6623+
// guard must REJECT a forged value, not silently swallow
6624+
// it), which is why this routes to the ingress instead of
6625+
// re-implementing the strip here — one create ingress,
6626+
// and a future change to its policy covers the batch for
6627+
// free. `trxCtx` carries the caller's context (including
6628+
// `isSystem`) plus the open transaction, so the strip
6629+
// decides exactly as it does on the single route and the
6630+
// insert still joins this transaction.
6631+
const created: any = await p.createData({ object: op.object, data, context: trxCtx } as any);
6632+
for (const e of (created?.droppedFields ?? []) as DroppedFieldsEvent[]) {
6633+
dropped.push({ ...e, index });
6634+
}
6635+
out.push(created?.record);
66156636
} else if (op.action === 'update') {
6637+
// Update needs no ingress detour: the engine enforces
6638+
// both static `readonly` (#2948) and `readonlyWhen`
6639+
// (#3042) on its own update path, and reports them
6640+
// through this listener.
6641+
const onFieldsDropped = (e: DroppedFieldsEvent) => { dropped.push({ ...e, index }); };
66166642
const id = op.id ?? data?.id;
66176643
out.push(await ql.update(op.object, { ...data, id }, { context: trxCtx, onFieldsDropped }));
66186644
} else { // 'delete'

0 commit comments

Comments
 (0)