Skip to content

Commit a225ef5

Browse files
authored
fix(runtime,webhooks): the path object wins on /data/:object/query, and the webhook envelope owns its keys (#3946) (#3947)
Follow-up sweep for the shape behind #3897 and #3933 — a trusted, server-derived value written into an object literal with a caller-controlled bag spread OVER it. Swept all 1313 non-test TypeScript files in packages/; nine candidates, one real, one worth hardening, seven verified clean (recorded in #3946). runtime /data domain: POST /data/:object/query built `{ object: objectName, ...body }`, so a body `object` key moved the read to another object than the URL named. Not an authorization bypass — callData gates exposure on `params.object`, so the gate followed the body and agreed with the read — but the URL stopped describing the operation for audit trails, logs and path-keyed middleware. plugin-webhooks: the delivery envelope's own keys were overridable by the event payload. Behaviour-neutral for the engine's publishers (data.record.* payloads nest record fields under `after`), but the shape was wrong. The docs had documented the old precedence, and contradicted themselves doing it — fixed too.
1 parent c8124e5 commit a225ef5

5 files changed

Lines changed: 169 additions & 6 deletions

File tree

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
---
2+
"@objectstack/runtime": patch
3+
"@objectstack/plugin-webhooks": patch
4+
---
5+
6+
fix(runtime,webhooks): the path object wins on /data/:object/query, and the webhook envelope owns its keys (#3946)
7+
8+
Follow-up sweep for the shape behind #3897 and #3933 — a trusted, server-derived
9+
value written into an object literal with a caller-controlled bag spread OVER
10+
it. Both of those were in the same block of REST code, so the pattern was swept
11+
across all 1313 non-test TypeScript files in `packages/`. Nine candidate sites;
12+
one real, one worth hardening, seven verified clean (recorded in #3946 so the
13+
next sweep does not re-litigate them).
14+
15+
**`POST /data/:object/query` (runtime dispatcher).** The `/data` domain built
16+
`{ object: objectName, ...body }`, so `{"object":"other", …}` in the body moved
17+
the read to a different object than the URL named.
18+
19+
This is NOT an authorization bypass, and the tests pin why: `callData` gates
20+
API exposure on `params.object`, so the gate followed the body and agreed with
21+
the read — an object hidden by `apiEnabled: false` was refused either way. What
22+
broke is that the URL stopped describing the operation (audit trails, logs, and
23+
anything keyed on the request path saw object A while object B was read), and
24+
that one endpoint spoke a second dialect of the contract the REST side had just
25+
standardised on: the path object wins. The other handlers in that file never had
26+
the problem — they nest caller data (`data: body`, `query: normalized`) instead
27+
of splatting it, and the GET-by-id branch already allowlists its query params
28+
against exactly this pollution.
29+
30+
**Webhook delivery envelope.** `auto-enqueuer` built
31+
`{ object, recordId, action, timestamp, ...payload }`, letting an event payload
32+
rewrite the envelope a subscriber receives. Behaviour-neutral for the engine's
33+
own publishers — `data.record.*` payloads are `{ recordId, after, changes }`
34+
with record fields nested under `after`, so none of those four keys collide
35+
today — but the shape was wrong, and the `payload.id` fallback right above it
36+
suggests publishers that flatten record fields do exist. Envelope keys are
37+
written last now.

content/docs/automation/webhooks.mdx

Lines changed: 7 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -304,16 +304,16 @@ same time.
304304

305305
## 5. Payload format
306306

307-
The POST body is the realtime event payload with a small fixed prefix merged
307+
The POST body is the realtime event payload with a small fixed envelope merged
308308
on top:
309309

310310
```json
311311
{
312+
// ...fields from the originating event payload
312313
"object": "account",
313314
"recordId": "acc_123",
314315
"action": "updated",
315316
"timestamp": "2024-10-27T00:00:00.000Z"
316-
// ...remaining fields from the originating event payload
317317
}
318318
```
319319

@@ -326,7 +326,11 @@ Notes:
326326
unchanged from the originating realtime event's `timestamp` field — not
327327
an epoch-ms number).
328328
- Any additional fields the event carried (e.g. record snapshot data) are
329-
spread in after these four.
329+
spread in first. These four are written **last**, so the envelope always
330+
describes the event even if a publisher's payload happens to carry a field
331+
with one of these names (#3946). The platform's own `data.record.*` events
332+
nest the record under `after` / `changes`, so nothing collides in practice —
333+
the ordering is what guarantees it.
330334

331335
Idempotency and event correlation are carried in **headers**, not the body.
332336
Every attempt sends:

packages/plugins/plugin-webhooks/src/auto-enqueuer.ts

Lines changed: 10 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -333,12 +333,21 @@ export class AutoEnqueuer {
333333
headers: sub.headers,
334334
signingSecret: sub.secret,
335335
timeoutMs: sub.timeoutMs,
336+
// [#3946] Envelope keys are written LAST so the event payload
337+
// cannot rewrite them. Behaviour-neutral for the engine's own
338+
// publishers — `data.record.*` payloads are
339+
// `{ recordId, after, changes }`, with record fields nested
340+
// under `after`, so none of these four keys collide today. It
341+
// is the shape that was wrong: a publisher that flattened
342+
// record fields into the payload (the `payload.id` fallback
343+
// above suggests some do) would have silently rewritten the
344+
// `object` / `action` / `timestamp` a subscriber receives.
336345
payload: {
346+
...payload,
337347
object: event.object,
338348
recordId,
339349
action,
340350
timestamp: event.timestamp,
341-
...payload,
342351
},
343352
}).catch((err) =>
344353
this.logger.warn?.('[webhook-auto-enqueuer] enqueue failed', {
Lines changed: 96 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,96 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
//
3+
// [#3946] `POST /data/:object/query` spread the request body OVER
4+
// `{ object: objectName }`, so a body `object` key moved the read to a
5+
// different object than the URL named — the same shape #3933 fixed on the REST
6+
// bulk routes, found by the follow-up sweep for that pattern.
7+
//
8+
// These run through the REAL `callData`, so they pin both halves at once: the
9+
// object the ADR-0049 exposure gate consults and the object actually queried
10+
// are the same one, and it is the one in the path.
11+
12+
import { describe, it, expect, vi } from 'vitest';
13+
import { handleDataRequest } from './data.js';
14+
15+
/** Records what the protocol service was asked for. */
16+
function setup(objectDefs: Record<string, any> = {}) {
17+
const findData = vi.fn(async (req: any) => ({ object: req.object, records: [], total: 0 }));
18+
const protocol = { findData };
19+
const metadata = { getObject: async (name: string) => objectDefs[name] };
20+
const deps: any = {
21+
resolveService: async (name: string) => (name === 'protocol' ? protocol : name === 'metadata' ? metadata : null),
22+
getService: () => null,
23+
getObjectQL: async () => null,
24+
getRequestKernelService: async () => null,
25+
isMultiTenantHost: () => false,
26+
success: (data: any) => ({ status: 200, body: data }),
27+
error: (message: string, code = 500) => ({ status: code, body: { error: message } }),
28+
routeNotFound: (route: string) => ({ status: 404, body: { route } }),
29+
errorFromThrown: (e: any) => ({ status: e?.statusCode ?? e?.status ?? 500, body: { error: e?.message } }),
30+
resolveActiveOrganizationId: async () => undefined,
31+
announceKernelEvent: async () => {},
32+
};
33+
const context: any = { dataDriver: undefined, environmentId: undefined, executionContext: { userId: 'u1' } };
34+
return { deps, context, findData };
35+
}
36+
37+
const post = (deps: any, context: any, path: string, body: any) =>
38+
handleDataRequest(deps, path, 'POST', body, {}, context);
39+
40+
describe('POST /data/:object/query binds to the object in the path (#3946)', () => {
41+
it('a body `object` cannot move the read to another object', async () => {
42+
const { deps, context, findData } = setup();
43+
const res = await post(deps, context, 'crm_account/query', {
44+
object: 'sys_user',
45+
where: { name: 'x' },
46+
});
47+
48+
expect(res.handled).toBe(true);
49+
expect(findData).toHaveBeenCalledTimes(1);
50+
expect(findData.mock.calls[0][0].object).toBe('crm_account');
51+
});
52+
53+
it('the exposure gate and the read agree on the path object', async () => {
54+
// `sys_user` is hidden from the API; `crm_account` is not. Pointing the
55+
// URL at the exposed object and naming the hidden one in the body must
56+
// not read the hidden one — and must not be refused on its behalf
57+
// either. Both decisions follow the path.
58+
const { deps, context, findData } = setup({
59+
crm_account: { apiEnabled: true },
60+
sys_user: { apiEnabled: false },
61+
});
62+
63+
const res: any = await post(deps, context, 'crm_account/query', { object: 'sys_user' });
64+
expect(res.response.status).toBe(200);
65+
expect(findData.mock.calls[0][0].object).toBe('crm_account');
66+
67+
// Addressed directly, the hidden object is still refused. The gate
68+
// THROWS a `{ statusCode }` shape; the dispatcher above turns it into
69+
// an envelope (`errorFromThrown`), so assert the throw here.
70+
await expect(post(deps, context, 'sys_user/query', {})).rejects.toMatchObject({ statusCode: 404 });
71+
expect(findData).toHaveBeenCalledTimes(1); // the refused call never read
72+
});
73+
74+
it('still forwards the rest of the body as the query', async () => {
75+
const { deps, context, findData } = setup();
76+
await post(deps, context, 'crm_account/query', { where: { status: 'open' }, limit: 5 });
77+
78+
const req = findData.mock.calls[0][0];
79+
expect(req.object).toBe('crm_account');
80+
expect(req.query).toMatchObject({ where: { status: 'open' }, limit: 5 });
81+
});
82+
83+
it('still honours an explicit `query` envelope', async () => {
84+
const { deps, context, findData } = setup();
85+
await post(deps, context, 'crm_account/query', { query: { where: { status: 'open' } } });
86+
87+
expect(findData.mock.calls[0][0].query).toEqual({ where: { status: 'open' } });
88+
});
89+
90+
it('threads the caller execution context through unchanged', async () => {
91+
const { deps, context, findData } = setup();
92+
await post(deps, context, 'crm_account/query', { context: { userId: 'root', isSystem: true } });
93+
94+
expect(findData.mock.calls[0][0].context).toBe(context.executionContext);
95+
});
96+
});

packages/runtime/src/domains/data.ts

Lines changed: 19 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -52,8 +52,25 @@ export async function handleDataRequest(deps: DomainHandlerDeps, path: string, m
5252

5353
// POST /data/:object/query
5454
if (action === 'query' && m === 'POST') {
55-
// Spec: returns FindDataResponse = { object, records, total?, hasMore? }
56-
const result = await actionExec.callData(deps, 'query', { object: objectName, ...body }, _context.dataDriver, _context.environmentId, _context.executionContext);
55+
// [#3946] The PATH object is written LAST. The body used to be
56+
// spread OVER `object: objectName`, so `{"object":"other", …}`
57+
// moved the read to a different object than the URL named — the
58+
// same shape #3933 fixed on the REST bulk routes, found by the
59+
// follow-up sweep.
60+
//
61+
// Not an authorization bypass here: `callData` gates exposure on
62+
// `params.object` (action-execution.ts), so the gate followed the
63+
// body and agreed with the read. What broke is that the URL stopped
64+
// describing the operation — audit trails, logs and anything keyed
65+
// on the request path saw object A while object B was read — and
66+
// that one endpoint spoke a second dialect of a contract the REST
67+
// side had just standardised (path wins).
68+
//
69+
// The sibling handlers below never had this: they nest the caller's
70+
// data (`data: body`, `query: normalized`) instead of splatting it,
71+
// and the GET-by-id branch even allowlists its query params against
72+
// exactly this kind of parameter pollution.
73+
const result = await actionExec.callData(deps, 'query', { ...body, object: objectName }, _context.dataDriver, _context.environmentId, _context.executionContext);
5774
return { handled: true, response: deps.success(result) };
5875
}
5976

0 commit comments

Comments
 (0)