Skip to content

Commit fccec22

Browse files
claudeos-zhuang
authored andcommitted
fix(rest): bulk writes bind to the object in the path, not the one in the body (#3933)
`POST /data/:object/updateMany` spread the request body over `object: req.params.object`, and `enforceApiAccess` gates on the PATH object (`const objectName = req?.params?.object`). So `enable.apiEnabled` / `apiMethods` (ADR-0049) was enforced on the object in the URL while the object named in the body got written. Measured on a stock CRM dev deployment: `POST /data/crm_account/updateMany` with `{"object":"crm_contact","records":[…]}` returned `succeeded: 1` and changed the crm_contact row. The path object is now written LAST, so the object the gate cleared is the object that gets written. The body is parsed against `UpdateManyDataRequestSchema` first, which also strips a body `context` — the execution context on a deployment where none resolves (`requireAuth: false` + anonymous), the only case where the trailing `...(context ? {context} : {})` has nothing to overwrite it with. deleteMany gets the same ordering: #3897 moved it behind a schema parse but fed that parse `{ object: req.params.object, ...req.body }`, still body-wins. createMany (`records: req.body || []`) and batch (`request: req.body`) never splatted the body at the top level and are unaffected. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01TzLE9cw4gZKNyPN2ZP4iTt
1 parent f4d7f1d commit fccec22

4 files changed

Lines changed: 221 additions & 4 deletions

File tree

Lines changed: 47 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,47 @@
1+
---
2+
"@objectstack/rest": minor
3+
---
4+
5+
fix(rest): bulk writes bind to the object in the path, not the one in the body (#3933)
6+
7+
`POST /data/:object/updateMany` spread the request body over the value it had
8+
just taken from the URL:
9+
10+
```js
11+
const result = await p.updateManyData!({
12+
object: req.params.object, // trusted, written first
13+
...req.body, // …and spread over it
14+
...
15+
});
16+
```
17+
18+
The gate on the line above reads the PATH object — `enforceApiAccess` starts
19+
with `const objectName = req?.params?.object` — so `enable.apiEnabled` /
20+
`enable.apiMethods` (ADR-0049 / #1889) was enforced on the object in the URL
21+
while the object named in the body got written. Measured on a stock CRM dev
22+
deployment: `POST /data/crm_account/updateMany` with
23+
`{"object":"crm_contact", "records":[…]}` returned `succeeded: 1` and changed
24+
the `crm_contact` row. Point the URL at any exposed object, name a hidden one in
25+
the body, and the gate clears the wrong object every time.
26+
27+
This is not a row-authorization bypass — the engine middleware still evaluates
28+
RLS/FLS against the object actually written, and `assertObjectRegistered` (#3770)
29+
still resolves it. What it defeats is the object-level exposure policy, the layer
30+
ADR-0049 exists to make enforceable rather than advisory.
31+
32+
The path object is now written LAST, after the body, so the object the gate
33+
cleared is the object that gets written — a property of the code rather than of
34+
the caller declining to send that key. The body is parsed against
35+
`UpdateManyDataRequestSchema` first, which (Zod strips unknown keys) also stops a
36+
body `context` from becoming the execution context on a deployment where none
37+
resolves — `requireAuth: false` plus an anonymous caller, the one case where the
38+
trailing `...(context ? { context } : {})` has nothing to overwrite it with.
39+
40+
`deleteMany` gets the same ordering: #3897 moved it behind a schema parse, but
41+
fed that parse `{ object: req.params.object, ...req.body }` — still body-wins.
42+
`createMany` (`records: req.body || []`) and `batch` (`request: req.body`) never
43+
splatted the body at the top level and are unaffected.
44+
45+
**Behaviour change.** A malformed `updateMany` body is now `400
46+
VALIDATION_FAILED` naming the offending path, instead of reaching the protocol
47+
and failing further in. A body `object` key is ignored rather than honoured.

content/docs/api/data-api.mdx

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -122,6 +122,12 @@ Batch update multiple records.
122122
}
123123
```
124124

125+
The body is validated against the contract and unknown keys are dropped. The
126+
target object always comes from the URL — an `object` key in the body is
127+
ignored, on this route and on `deleteMany`.
128+
129+
**Response**: `BatchUpdateResponse`, one `results` entry per record.
130+
125131
### `POST /data/:object/deleteMany`
126132

127133
Batch delete records by ID list.
Lines changed: 137 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,137 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
//
3+
// [#3933] The bulk write routes used to spread the request body OVER
4+
// `object: req.params.object`, so a body key moved the write to a different
5+
// object than the one in the URL. `enforceApiAccess` gates on `req.params.object`
6+
// — so `enable.apiEnabled` / `enable.apiMethods` (ADR-0049 / #1889) was enforced
7+
// on the object in the PATH while the object in the BODY got written. The path
8+
// object is now written last, and the body is parsed against the spec contract
9+
// (which also strips a body `context`).
10+
11+
import { describe, it, expect, vi } from 'vitest';
12+
import { RestServer } from './rest-server';
13+
14+
const UPDATE_MANY = '/api/v1/data/:object/updateMany';
15+
const DELETE_MANY = '/api/v1/data/:object/deleteMany';
16+
17+
function createMockServer() {
18+
return {
19+
get: vi.fn(), post: vi.fn(), put: vi.fn(), delete: vi.fn(), patch: vi.fn(), use: vi.fn(),
20+
listen: vi.fn().mockResolvedValue(undefined), close: vi.fn().mockResolvedValue(undefined),
21+
};
22+
}
23+
24+
function makeRes() {
25+
const res: any = { statusCode: 200, body: undefined };
26+
res.status = vi.fn((c: number) => { res.statusCode = c; return res; });
27+
res.json = vi.fn((b: any) => { res.body = b; return res; });
28+
res.setHeader = vi.fn(); res.write = vi.fn(); res.end = vi.fn();
29+
return res;
30+
}
31+
32+
/**
33+
* `open` is exposed to the API; `locked` is not. The exploit points the URL at
34+
* `open` (so the gate clears) and names `locked` in the body.
35+
*/
36+
function setup() {
37+
const updateManyData = vi.fn().mockResolvedValue({
38+
success: true, operation: 'update', total: 1, succeeded: 1, failed: 0, results: [],
39+
});
40+
const deleteManyData = vi.fn().mockResolvedValue({
41+
success: true, operation: 'delete', total: 1, succeeded: 1, failed: 0, results: [],
42+
});
43+
const protocol: any = {
44+
getDiscovery: vi.fn().mockResolvedValue({ version: 'v0', endpoints: { data: '', metadata: '', ui: '', auth: '/auth' } }),
45+
getMetaTypes: vi.fn().mockResolvedValue([]),
46+
getMetaItems: vi.fn().mockResolvedValue([
47+
{ name: 'open' },
48+
{ name: 'locked', enable: { apiEnabled: false } },
49+
]),
50+
getMetaItem: vi.fn().mockResolvedValue({}),
51+
findData: vi.fn().mockResolvedValue([]),
52+
updateManyData,
53+
deleteManyData,
54+
};
55+
const rest = new RestServer(createMockServer() as any, protocol, { api: { requireAuth: false } } as any);
56+
rest.registerRoutes();
57+
return { rest, updateManyData, deleteManyData };
58+
}
59+
60+
async function post(rest: any, path: string, object: string, body: any) {
61+
const route = rest.getRoutes().find((r: any) => r.method === 'POST' && r.path === path);
62+
if (!route) throw new Error(`route not registered: ${path}`);
63+
const res = makeRes();
64+
await route.handler({ method: 'POST', params: { object }, query: {}, body }, res);
65+
return res;
66+
}
67+
68+
const ONE_UPDATE = [{ id: 'r1', data: { name: 'x' } }];
69+
70+
describe('bulk writes bind to the object in the PATH (#3933)', () => {
71+
it('updateMany: a body `object` cannot redirect the write', async () => {
72+
const { rest, updateManyData } = setup();
73+
const res = await post(rest, UPDATE_MANY, 'open', { object: 'locked', records: ONE_UPDATE });
74+
75+
expect(res.statusCode).toBe(200);
76+
expect(updateManyData.mock.calls[0][0].object).toBe('open');
77+
});
78+
79+
it('deleteMany: a body `object` cannot redirect the delete', async () => {
80+
const { rest, deleteManyData } = setup();
81+
const res = await post(rest, DELETE_MANY, 'open', { object: 'locked', ids: ['r1'] });
82+
83+
expect(res.statusCode).toBe(200);
84+
expect(deleteManyData.mock.calls[0][0].object).toBe('open');
85+
});
86+
87+
it('the apiEnabled gate cannot be bypassed by naming the locked object in the body', async () => {
88+
const { rest, updateManyData, deleteManyData } = setup();
89+
// Sanity: addressed directly, `locked` is hidden (404, ADR-0049).
90+
expect((await post(rest, UPDATE_MANY, 'locked', { records: ONE_UPDATE })).statusCode).toBe(404);
91+
expect((await post(rest, DELETE_MANY, 'locked', { ids: ['r1'] })).statusCode).toBe(404);
92+
expect(updateManyData).not.toHaveBeenCalled();
93+
expect(deleteManyData).not.toHaveBeenCalled();
94+
95+
// Via the exploit: the gate clears `open`, and `open` is what gets written.
96+
await post(rest, UPDATE_MANY, 'open', { object: 'locked', records: ONE_UPDATE });
97+
await post(rest, DELETE_MANY, 'open', { object: 'locked', ids: ['r1'] });
98+
expect(updateManyData.mock.calls.every((c) => c[0].object === 'open')).toBe(true);
99+
expect(deleteManyData.mock.calls.every((c) => c[0].object === 'open')).toBe(true);
100+
});
101+
});
102+
103+
describe('updateMany ingress validation (#3933)', () => {
104+
it('strips a body-supplied context so the principal cannot be forged', async () => {
105+
const { rest, updateManyData } = setup();
106+
// `requireAuth: false` → no execution context resolves, so a body `context`
107+
// used to be the only one the protocol (and then the engine) ever saw.
108+
await post(rest, UPDATE_MANY, 'open', {
109+
records: ONE_UPDATE,
110+
context: { userId: 'nobody', isSystem: true, roles: ['admin'] },
111+
});
112+
113+
expect(updateManyData.mock.calls[0][0].context).toBeUndefined();
114+
});
115+
116+
it('forwards a well-formed request unchanged', async () => {
117+
const { rest, updateManyData } = setup();
118+
const res = await post(rest, UPDATE_MANY, 'open', {
119+
records: ONE_UPDATE,
120+
options: { atomic: false, continueOnError: true },
121+
});
122+
123+
expect(res.statusCode).toBe(200);
124+
const req = updateManyData.mock.calls[0][0];
125+
expect(req.records).toEqual(ONE_UPDATE);
126+
expect(req.options).toMatchObject({ atomic: false, continueOnError: true });
127+
});
128+
129+
it('rejects a malformed records list with 400 VALIDATION_FAILED', async () => {
130+
const { rest, updateManyData } = setup();
131+
const res = await post(rest, UPDATE_MANY, 'open', { records: [{ data: { name: 'x' } }] });
132+
133+
expect(res.statusCode).toBe(400);
134+
expect(res.body.code).toBe('VALIDATION_FAILED');
135+
expect(updateManyData).not.toHaveBeenCalled();
136+
});
137+
});

packages/rest/src/rest-server.ts

Lines changed: 31 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -6817,15 +6817,38 @@ export class RestServer {
68176817
if (this.enforceAuth(req, res, context)) return;
68186818
// [#3391] bulk ∧ update — updateMany requires the `bulk` primitive.
68196819
if (await this.enforceApiAccess(req, res, p, environmentId, 'bulk', { bulkChild: 'update' })) return;
6820-
const result = await p.updateManyData!({
6820+
// [#3933] Validate against the spec contract, and write the
6821+
// PATH object last. The body used to be spread over
6822+
// `object: req.params.object`, so `{"object":"other", …}`
6823+
// moved the write to a different object than the one
6824+
// `enforceApiAccess` had just cleared — that gate reads
6825+
// `req.params.object`, so `enable.apiEnabled` / `apiMethods`
6826+
// (ADR-0049) was enforced on A while B was written. Zod also
6827+
// strips unknown keys, which keeps a body `context` from
6828+
// becoming the execution context on a deployment where none
6829+
// resolves (anonymous-reachable `requireAuth: false`).
6830+
const { UpdateManyDataRequestSchema } = await import('@objectstack/spec/api');
6831+
const parsedUpdate = (UpdateManyDataRequestSchema as any).safeParse({
6832+
...(req.body ?? {}),
68216833
object: req.params.object,
6822-
...req.body,
6834+
});
6835+
if (!parsedUpdate.success) {
6836+
res.status(400).json({
6837+
error: 'Invalid updateMany request',
6838+
code: 'VALIDATION_FAILED',
6839+
object: req.params?.object,
6840+
issues: parsedUpdate.error?.issues,
6841+
});
6842+
return;
6843+
}
6844+
const result = await p.updateManyData!({
6845+
...parsedUpdate.data,
68236846
...(environmentId ? { environmentId } : {}),
68246847
...(context ? { context } : {}),
68256848
} as any);
68266849
res.json(result);
68276850
} catch (error: any) {
6828-
logError("[REST] Unhandled error:", error);
6851+
if (error?.code !== 'VALIDATION_FAILED') logError("[REST] Unhandled error:", error);
68296852
sendError(res, error, req.params?.object);
68306853
}
68316854
},
@@ -6861,10 +6884,14 @@ export class RestServer {
68616884
// refuses the same shapes independently (defence in
68626885
// depth) — this stops them one hop earlier, with a 400
68636886
// the caller can act on.
6887+
// [#3933] The PATH object is written LAST for the same
6888+
// reason: `enforceApiAccess` gates on `req.params.object`,
6889+
// so a body `object` would move the delete to an object
6890+
// whose exposure policy was never checked.
68646891
const { DeleteManyDataRequestSchema } = await import('@objectstack/spec/api');
68656892
const parsed = (DeleteManyDataRequestSchema as any).safeParse({
6866-
object: req.params.object,
68676893
...(req.body ?? {}),
6894+
object: req.params.object,
68686895
});
68696896
if (!parsed.success) {
68706897
res.status(400).json({

0 commit comments

Comments
 (0)