Skip to content

Commit 35b36f2

Browse files
os-helpclaude
andauthored
fix(runtime): withhold a permission denial's authorization payload from the wire (#7450) (#7520)
* fix(runtime): withhold a permission denial's authorization payload from the wire (#7450) The runtime dispatcher's `dispatch()` catch spread the whole `PermissionDeniedError.details` into the 403 body (`{ code: 'PERMISSION_DENIED', ...(e.details ?? {}) }`), and `buildApiError` puts everything that is not the `code` on the wire as `error.details` — so the security gate's `positions` / `permissionSets` were client-facing on this transport while `@objectstack/rest`'s `mapDataError` shipped none of them. Per the maintainer's 2026-08-11 ruling both transports now carry REST's shape: message + code + the ROUTE-derived object. The object is derived from `cleanPath`, not from `error.details`. `cascadeDeleteRelations` re-enters `delete()` per child, so a cascade denial's `details.object` names a child the caller never addressed; forwarding it would have reached the ruled field set and still disclosed a third party's API name. The catch now reads no field of `error.details` at all. The full withheld payload goes to a server log line instead — it is diagnostics, not garbage. Also: the domain-registry branch returned its handler's promise without awaiting, so a rejection settled outside the enclosing `try` and never reached that catch. Every domain that can raise an object-gate denial resolves through that branch, which made the `PERMISSION_DENIED` arm unreachable in practice — denials escaped to the Hono catch-all, which answered a numeric `code` and no `PERMISSION_DENIED` string. It now awaits, which is what makes the ruled envelope apply; awaiting alone would have started shipping the leak, so the two changes land together. Non-denial errors are unchanged: the catch rethrows them. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PLXNUARnwCaXaQ9xg4ZBhe * test(runtime): pin transport parity without importing across the package boundary (#7450) The cross-transport parity test reached `mapDataError` at its declaration (`../../../rest/src/rest-server.js`). tsc follows that: it pulled eleven of `@objectstack/rest`'s modules into this package's program outside its `rootDir`, adding 13 raw errors (TS6059 ×12, TS7006 ×2) to the runtime TEST_DEBT ledger — which is a ratchet and may only shrink. The `TypeScript Type Check` gate caught it: recorded 227, measured 240. A disclosure fix must not widen the reference package's API surface or its type-check debt to buy itself a test, so parity is now pinned by TWO tests over ONE fixture instead of by a live cross-import: `packages/rest/src/rest.test.ts` (PR #7449) asserts what `mapDataError` produces for that fixture, and this file asserts the dispatcher agrees with it, field by field, against a transcribed constant that names its source. Change either side's shape and the other side's pin fails. The rewritten case also tightens what it checks — `error.details` must hold the route object ALONE, and the error member must carry no key beyond code/message/httpStatus/details. Mutation coverage is unchanged: restoring the spread still fails 4 cases, and forwarding `e.details.object` (the naive allowlist) still fails the cascade case and this one. Measured after the fix: 226 raw errors, at or under the ledger's 227, with zero attributable to these files. Co-Authored-By: Claude Opus 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01PLXNUARnwCaXaQ9xg4ZBhe --------- Co-authored-by: Claude <noreply@anthropic.com>
1 parent 23bc6e1 commit 35b36f2

5 files changed

Lines changed: 522 additions & 2 deletions

File tree

Lines changed: 51 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,51 @@
1+
---
2+
"@objectstack/runtime": patch
3+
---
4+
5+
fix(runtime): stop the dispatcher answering a permission denial's internal authorization payload (#7450)
6+
7+
`plugin-security`'s object gate attaches
8+
`{ operation, object, positions, permissionSets }` to every
9+
`PermissionDeniedError`. The two HTTP transports disagreed about what to do with
10+
it. `@objectstack/rest`'s `mapDataError` never reads `error.details` — its 403
11+
body is `{ error, code, object? }`, and the `object` on it is the one the ROUTE
12+
named. The dispatcher's `dispatch()` catch spread the whole payload
13+
(`{ code: 'PERMISSION_DENIED', ...(e.details ?? {}) }`), and `buildApiError`
14+
puts everything that is not the `code` on the wire as `error.details`.
15+
16+
Per the maintainer's 2026-08-11 ruling the two transports now agree on REST's
17+
shape: **message + code + the route-derived object**. `positions` and
18+
`permissionSets` are server-side diagnostics and are no longer serialized; the
19+
full withheld payload is written to a server log line instead, so a false
20+
denial is still diagnosable.
21+
22+
**The object is derived from the request path, not from `error.details`.** This
23+
is the part an "allowlist `operation` + `object`" reading gets wrong.
24+
`ObjectQL.cascadeDeleteRelations` re-enters `delete()` for every child of the
25+
row being deleted, so a child's own trip through the security middleware throws
26+
with `details.object` set to the CHILD. Forwarding that field would have reached
27+
the ruled field set and still answered the API name of an object the caller
28+
never addressed. The dispatcher now reads no field of `error.details` at all:
29+
`object` comes from `cleanPath`, exactly as REST takes `req.params.object`, and
30+
a denial on a route whose path names no object carries no `object` — REST's
31+
`...(object ? { object } : {})` behaviour.
32+
33+
**Also fixed, and required for the above to have any effect on the wire.** The
34+
domain-registry branch of `dispatch()` returned its handler's promise without
35+
awaiting it. In an async function a bare `return <promise>` settles outside the
36+
enclosing `try`, so a domain handler's rejection never reached that method's
37+
`catch` — and every domain that can raise an object-gate denial (`/data` among
38+
them) resolves through that branch, which made the `PERMISSION_DENIED` branch
39+
unreachable in practice. Denials escaped to the Hono catch-all instead, which
40+
answered `{ error: { message, code: 403 } }`: a numeric `code`, the shape
41+
`error-envelope.ts` exists to prevent, with no `PERMISSION_DENIED` string for a
42+
client to branch on. The branch now awaits, so a `/data` denial answers the
43+
ruled envelope. Non-denial errors are unaffected — the catch rethrows them and
44+
they reach the adapter exactly as before.
45+
46+
**Wire-visible.** A consumer reading `error.details.positions`,
47+
`error.details.permissionSets` or `error.details.operation` off a dispatcher 403
48+
no longer receives them, and `error.details.object` is now the object the
49+
request addressed rather than whichever object the gate refused. A `/data`
50+
denial's `error.code` is now the string `PERMISSION_DENIED` rather than the
51+
number `403`.
Lines changed: 237 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,237 @@
1+
// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
2+
3+
/**
4+
* [#7450] A `/data` permission denial must not answer the caller's
5+
* authorization topology — nor an object they never addressed.
6+
*
7+
* `plugin-security`'s CRUD gate attaches
8+
* `{ operation, object, positions, permissionSets }` to every
9+
* `PermissionDeniedError`. `@objectstack/rest`'s `mapDataError` never reads it
10+
* (403 body = `{ error, code, object? }`, the `object` being the one the ROUTE
11+
* named). The dispatcher spread it — `{ code: 'PERMISSION_DENIED',
12+
* ...(e.details ?? {}) }` — and `buildApiError` puts everything but the `code`
13+
* on the wire as `error.details`. `/data` is served by that dispatcher and
14+
* `domains/data.ts` has no local catch, so an ordinary CRUD denial disclosed
15+
* the caller's position and permission-set names to the browser.
16+
*
17+
* The 2026-08-11 maintainer ruling on #7450: REST's shape is the contract for
18+
* both transports.
19+
*
20+
* ## The cascade case, which is why the object is route-derived
21+
*
22+
* `ObjectQL.cascadeDeleteRelations` re-enters `delete()` for every child of the
23+
* row being deleted, so a child's own trip through the gate throws with
24+
* `details.object === <child>`. An allowlist that forwarded `details.object`
25+
* would reach the ruled field set and still answer a third party's API name on
26+
* exactly the path the card called the sharpest. The dispatcher therefore takes
27+
* `object` from the request path, like REST takes `req.params.object`, and
28+
* reads no field of `error.details` at all.
29+
*
30+
* ⚠️ Fixture discipline: every denial below carries a FULLY populated `details`
31+
* — a test whose fixture never had one would pass against the unfixed
32+
* dispatcher and prove nothing. Each assertion here fails on `main` before the
33+
* fix (mutation table in the PR).
34+
*/
35+
36+
import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest';
37+
38+
import { HttpDispatcher } from '../http-dispatcher.js';
39+
import { PermissionDeniedError } from '@objectstack/plugin-security';
40+
41+
/** The end-user sentence the CRUD gate renders (#7414) — no API names in it. */
42+
const USER_MESSAGE = '您没有执行此操作的权限,如需访问请联系管理员。';
43+
44+
/**
45+
* REST's answer to the SAME denial, transcribed from the pin PR #7449 added:
46+
* `packages/rest/src/rest.test.ts`, "never ships a PERMISSION_DENIED developer
47+
* half or its structured details to the client". That test drives the real
48+
* `mapDataError` over a fixture identical to {@link cascadeChildDenial} below,
49+
* down to the position and permission-set names, and asserts exactly this body.
50+
*
51+
* Transcribed rather than imported on purpose. `@objectstack/rest`'s public
52+
* entry does not re-export `mapDataError`, and reaching into its source from
53+
* here drags eleven of that package's modules outside this package's `rootDir`
54+
* (TS6059) — the fix for a disclosure card must not widen the reference
55+
* package's API surface or its type-check debt to buy itself a test. So parity
56+
* is pinned by TWO tests over ONE fixture: REST's asserts the constant below is
57+
* what `mapDataError` produces, this file asserts the dispatcher agrees with it.
58+
* Change either side's shape and the other side's pin fails.
59+
*/
60+
const REST_DENIAL_BODY = {
61+
error: USER_MESSAGE,
62+
code: 'PERMISSION_DENIED',
63+
object: 'app_parent_object',
64+
} as const;
65+
66+
/**
67+
* A denial raised while cascading `DELETE /data/app_parent_object/1` into a
68+
* child: the gate's own `object` is the CHILD, which the caller never named.
69+
*/
70+
const cascadeChildDenial = () =>
71+
new PermissionDeniedError(
72+
USER_MESSAGE,
73+
{
74+
operation: 'delete',
75+
object: 'app_child_object',
76+
positions: ['org_member', 'everyone'],
77+
permissionSets: ['app_reader'],
78+
},
79+
"[Security] Access denied: operation 'delete' on object 'app_child_object' " +
80+
'is not permitted for positions [org_member, everyone]',
81+
);
82+
83+
/** An ordinary denial on the object the caller addressed. */
84+
const directDenial = (object: string) =>
85+
new PermissionDeniedError(USER_MESSAGE, {
86+
operation: 'update',
87+
object,
88+
positions: ['org_member', 'everyone'],
89+
permissionSets: ['app_reader'],
90+
});
91+
92+
const CALLER = () =>
93+
({
94+
request: {},
95+
executionContext: {
96+
userId: 'u_test',
97+
isSystem: false,
98+
positions: ['org_member', 'everyone'],
99+
permissions: ['app_reader'],
100+
systemPermissions: [],
101+
},
102+
}) as any;
103+
104+
describe('/data PERMISSION_DENIED body — #7450', () => {
105+
let dispatcher: HttpDispatcher;
106+
let mockObjectQL: any;
107+
let warn: ReturnType<typeof vi.spyOn>;
108+
109+
beforeEach(() => {
110+
mockObjectQL = {
111+
insert: vi.fn(),
112+
// The protocol-less `/data` fallback reads the row before writing
113+
// it, so the pre-read has to succeed for the WRITE's denial — the
114+
// one carrying the cascade payload — to be the error under test.
115+
find: vi.fn().mockResolvedValue([{ id: '1' }]),
116+
update: vi.fn(),
117+
delete: vi.fn(),
118+
getObjects: vi.fn().mockReturnValue({}),
119+
registry: { getObject: vi.fn().mockReturnValue({ name: 'app_parent_object' }) },
120+
};
121+
const kernel = {
122+
context: {
123+
getService: (name: string) => (name === 'objectql' ? mockObjectQL : null),
124+
},
125+
} as any;
126+
dispatcher = new HttpDispatcher(kernel);
127+
warn = vi.spyOn(console, 'warn').mockImplementation(() => {});
128+
});
129+
130+
afterEach(() => {
131+
// Without this the spy stacks across cases and the log assertion below
132+
// counts every earlier case's line too.
133+
warn.mockRestore();
134+
});
135+
136+
const dispatch = (method: string, path: string, body?: any) =>
137+
dispatcher.dispatch(method, path, body, {}, CALLER());
138+
139+
it('answers 403 PERMISSION_DENIED with the message and no structured details', async () => {
140+
mockObjectQL.update.mockRejectedValue(directDenial('app_parent_object'));
141+
142+
const r = await dispatch('PATCH', '/data/app_parent_object/1', { name: 'x' });
143+
144+
expect(r.handled).toBe(true);
145+
expect(r.response?.status).toBe(403);
146+
// Positive identity first — the absence assertions below only mean
147+
// something on top of a body that really is the denial.
148+
expect(r.response?.body?.error?.code).toBe('PERMISSION_DENIED');
149+
expect(r.response?.body?.error?.message).toBe(USER_MESSAGE);
150+
// The route's own object still rides: the caller named it themselves.
151+
expect(r.response?.body?.error?.details).toEqual({ object: 'app_parent_object' });
152+
});
153+
154+
it('never serialises positions or permissionSets', async () => {
155+
mockObjectQL.update.mockRejectedValue(directDenial('app_parent_object'));
156+
157+
const r = await dispatch('PATCH', '/data/app_parent_object/1', { name: 'x' });
158+
159+
const wire = JSON.stringify(r.response?.body);
160+
expect(wire).not.toContain('positions');
161+
expect(wire).not.toContain('permissionSets');
162+
expect(wire).not.toContain('org_member');
163+
expect(wire).not.toContain('app_reader');
164+
// The operator's half of the message is not a details sibling either.
165+
expect(wire).not.toContain('developerMessage');
166+
expect(wire).not.toContain('[Security]');
167+
});
168+
169+
it('does NOT name a cascade child the caller never addressed', async () => {
170+
mockObjectQL.delete.mockRejectedValue(cascadeChildDenial());
171+
172+
const r = await dispatch('DELETE', '/data/app_parent_object/1');
173+
174+
expect(r.response?.status).toBe(403);
175+
// The whole point: `details.object` said `app_child_object`.
176+
expect(JSON.stringify(r.response?.body)).not.toContain('app_child_object');
177+
expect(r.response?.body?.error?.details?.object).toBe('app_parent_object');
178+
});
179+
180+
// The mirror case — a denial on a route whose path names no object, where
181+
// REST's `...(object ? { object } : {})` omits the field — is pinned at the
182+
// builder in `../security/permission-denied-envelope.test.ts`. It is not
183+
// reachable end-to-end through `/data`: that domain answers 400 "Object
184+
// name required" before any gate runs.
185+
186+
it('logs the withheld diagnostics server-side rather than dropping them', async () => {
187+
mockObjectQL.delete.mockRejectedValue(cascadeChildDenial());
188+
189+
await dispatch('DELETE', '/data/app_parent_object/1');
190+
191+
const lines = warn.mock.calls
192+
.map((c: unknown[]) => String(c[0]))
193+
.filter((l: string) => l.includes('PERMISSION_DENIED'));
194+
expect(lines).toHaveLength(1);
195+
const line = lines[0]!;
196+
expect(line).toContain('DELETE /data/app_parent_object/1');
197+
// Everything the wire lost is here — including the cascade child, which
198+
// is the field an operator debugging a false denial most needs.
199+
expect(line).toContain('object=app_child_object');
200+
expect(line).toContain('positions=[org_member, everyone]');
201+
expect(line).toContain('permissionSets=[app_reader]');
202+
});
203+
204+
/**
205+
* The card's third decision: "either way the two transports should agree on
206+
* what a PERMISSION_DENIED body carries".
207+
*
208+
* The two envelopes NEST differently — REST is flat, the dispatcher wraps in
209+
* `success` / `error` — and this card does not change that. What has to
210+
* agree is the semantic content, which is what leaked. So the comparison is
211+
* field-by-field against {@link REST_DENIAL_BODY}, the body REST's own pin
212+
* asserts for this identical error.
213+
*/
214+
it('discloses exactly what @objectstack/rest discloses for the same denial', async () => {
215+
mockObjectQL.delete.mockRejectedValue(cascadeChildDenial());
216+
217+
const runtime = await dispatch('DELETE', '/data/app_parent_object/1');
218+
const error = runtime.response?.body?.error;
219+
220+
expect(runtime.response?.status).toBe(403);
221+
expect({
222+
error: error?.message,
223+
code: error?.code,
224+
object: (error?.details as { object?: string } | undefined)?.object,
225+
}).toEqual({ ...REST_DENIAL_BODY });
226+
227+
// Nothing rides beyond those three: `details` holds the object alone,
228+
// and no other key was smuggled onto the error member.
229+
expect(error?.details).toEqual({ object: REST_DENIAL_BODY.object });
230+
expect(Object.keys(error ?? {}).sort()).toEqual(['code', 'details', 'httpStatus', 'message']);
231+
232+
const wire = JSON.stringify(runtime.response?.body);
233+
expect(wire).not.toContain('positions');
234+
expect(wire).not.toContain('permissionSets');
235+
expect(wire).not.toContain('app_child_object');
236+
});
237+
});

packages/runtime/src/http-dispatcher.ts

Lines changed: 46 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -43,6 +43,10 @@ import {
4343
resolveExecutionContext,
4444
isPermissionDeniedError,
4545
} from './security/resolve-execution-context.js';
46+
import {
47+
permissionDeniedErrorDetails,
48+
describeDeniedDiagnostics,
49+
} from './security/permission-denied-envelope.js';
4650
import { validationFailureDetails, VALIDATION_FAILED_STATUS } from './validation-failure.js';
4751

4852
// randomUUID moved to ./domains/auth.ts with its only consumer (D11③ PR-7).
@@ -1847,7 +1851,24 @@ export class HttpDispatcher {
18471851
// order-equivalent to their original chain positions.
18481852
const domainRoute = this.domainRegistry.resolve(cleanPath, method);
18491853
if (domainRoute) {
1850-
return domainRoute.handler({ path: cleanPath, method, body, query }, context);
1854+
// [#7450] `return await`, not `return`. In an async function a bare
1855+
// `return <promise>` settles OUTSIDE the enclosing `try`, so a
1856+
// domain handler's rejection never reached the `catch` at the foot
1857+
// of this method — and every domain that can raise an object-gate
1858+
// denial (`/data` first among them) resolves HERE, which made that
1859+
// catch's `PERMISSION_DENIED` branch unreachable in practice. The
1860+
// denial escaped to the Hono catch-all instead, which answered
1861+
// `{ error: { message, code: 403 } }` — a NUMERIC `code`, i.e. the
1862+
// #3842 shape this package's error envelope exists to prevent, and
1863+
// no `PERMISSION_DENIED` for a client to branch on.
1864+
//
1865+
// Awaiting is what makes the ruled 403 contract actually apply on
1866+
// this transport; it must land together with the details allowlist
1867+
// in the catch below, because awaiting ALONE would start shipping
1868+
// the `positions` / `permissionSets` payload the ruling withholds.
1869+
// Non-denial errors are unaffected: the catch rethrows them, so
1870+
// they reach the adapter exactly as before.
1871+
return await domainRoute.handler({ path: cleanPath, method, body, query }, context);
18511872
}
18521873

18531874
// 0. Discovery Endpoint (GET /discovery or GET /)
@@ -1943,9 +1964,32 @@ export class HttpDispatcher {
19431964
};
19441965
} catch (e) {
19451966
if (isPermissionDeniedError(e)) {
1967+
// [#7450] The denial's `details` does NOT reach the wire.
1968+
//
1969+
// This used to be `{ code: 'PERMISSION_DENIED', ...(e.details ?? {}) }`,
1970+
// and `buildApiError` puts everything that is not the `code` on
1971+
// the wire as `error.details` — so the security gate's
1972+
// `positions` / `permissionSets` (the caller's authorization
1973+
// topology) and its `object` (on a cascade delete, a CHILD the
1974+
// caller never addressed — `cascadeDeleteRelations` re-authorises
1975+
// each one independently) were answered to the browser, while
1976+
// `@objectstack/rest`'s `mapDataError` shipped none of it.
1977+
//
1978+
// Per the 2026-08-11 ruling on #7450 the two transports agree on
1979+
// REST's shape — message + code + the ROUTE-derived object. The
1980+
// object below therefore comes from `cleanPath`, the dispatcher's
1981+
// equivalent of REST's `req.params.object`, and never from
1982+
// `e.details.object`: those are different values on exactly the
1983+
// cascade path that made this a disclosure. See
1984+
// `./security/permission-denied-envelope.ts`.
1985+
const withheld = describeDeniedDiagnostics(e.details);
1986+
if (withheld) {
1987+
// Dropped from the response, not from the operator's reach.
1988+
console.warn(`[HttpDispatcher] PERMISSION_DENIED on ${method} ${cleanPath}${withheld}`);
1989+
}
19461990
return {
19471991
handled: true,
1948-
response: this.error(e.message, 403, { code: 'PERMISSION_DENIED', ...(e.details ?? {}) }),
1992+
response: this.error(e.message, 403, permissionDeniedErrorDetails(cleanPath)),
19491993
};
19501994
}
19511995
throw e;

0 commit comments

Comments
 (0)