Skip to content

Commit 896cd5b

Browse files
committed
fix(rest): pass through explicit 4xx status+code in mapDataError (#2926 ⑦)
Record-scope authorization denials (plugin-sharing throws status=403, code=FORBIDDEN) degraded to a bare 400 with no code because the generic data routes call mapDataError directly, bypassing sendError's status passthrough. Add a guarded 4xx passthrough after the structured-code branches (409 envelopes keep their rich fields; 5xx still goes through the sanitizing heuristics; oversized messages fall back to generic text), and stop logging expected statuses as unhandled on the list route. Co-Authored-By: Claude Fable 5 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_017bJWtKmZ2mFpRFAmmmoeqe
1 parent 4e0d6b2 commit 896cd5b

3 files changed

Lines changed: 91 additions & 1 deletion

File tree

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
---
2+
'@objectstack/rest': patch
3+
---
4+
5+
fix(rest): mapDataError now honors an explicit 4xx `error.status`/`error.code` carried by domain errors (#2926 ⑦). Record-scope authorization denials from plugin-sharing (status 403, code FORBIDDEN) previously degraded to a bare 400 with no machine-readable code because the generic data routes bypass sendError's status passthrough. Structured 409 envelopes (CONCURRENT_UPDATE, DELETE_RESTRICTED) keep their dedicated branches; 5xx statuses still go through the message-sanitizing heuristics.

packages/rest/src/rest-server.ts

Lines changed: 22 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -157,6 +157,27 @@ export function mapDataError(error: any, object?: string): { status: number; bod
157157
},
158158
};
159159
}
160+
// Generic passthrough for domain errors that already carry an explicit
161+
// HTTP status (e.g. plugin-sharing's record-scope denial: status 403 +
162+
// code FORBIDDEN) — mirrors sendError's `.status` handling, which the
163+
// generic data routes bypass by calling mapDataError directly (#2926 ⑦).
164+
// Placed AFTER the structured-code branches above (409s carry rich
165+
// fields this envelope would drop) and deliberately limited to 4xx:
166+
// 5xx messages keep going through the sanitizing heuristics below so
167+
// internal/SQL details never reach the client verbatim.
168+
if (typeof error?.status === 'number' && error.status >= 400 && error.status < 500) {
169+
const msg = typeof error?.message === 'string' && error.message.length > 0 && error.message.length < 500
170+
? error.message
171+
: 'Request failed';
172+
return {
173+
status: error.status,
174+
body: {
175+
error: msg,
176+
...(typeof error?.code === 'string' && error.code ? { code: error.code } : {}),
177+
...(object ? { object } : {}),
178+
},
179+
};
180+
}
160181

161182
const raw = String(error?.message ?? error ?? '');
162183
const lower = raw.toLowerCase();
@@ -3259,7 +3280,7 @@ export class RestServer {
32593280
res.json(result);
32603281
} catch (error: any) {
32613282
const mapped = mapDataError(error, req.params?.object);
3262-
if (mapped.status === 404 || mapped.status === 503 || mapped.status === 502) {
3283+
if (isExpectedDataStatus(mapped.status) || mapped.body?.code === 'UNSUPPORTED_QUERY_PARAM') {
32633284
res.status(mapped.status).json(mapped.body);
32643285
} else {
32653286
logError("[REST] Unhandled error:", error);

packages/rest/src/rest.test.ts

Lines changed: 64 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -2140,6 +2140,70 @@ describe('mapDataError — schema/constraint envelopes', () => {
21402140
expect(r.body.object).toBe('crm_lead');
21412141
});
21422142

2143+
// #2926 ⑦: plugin-sharing's record-scope denial throws with an explicit
2144+
// status 403 + code FORBIDDEN, but the generic data routes bypass
2145+
// sendError's `.status` passthrough — mapDataError must honor the
2146+
// explicit status itself or the 403 degrades to the catch-all 400.
2147+
it('passes through an explicit 4xx status + code (sharing FORBIDDEN → 403)', () => {
2148+
const r = mapDataError(
2149+
Object.assign(new Error('FORBIDDEN: insufficient privileges to update showcase_inquiry rec1'), {
2150+
code: 'FORBIDDEN',
2151+
status: 403,
2152+
}),
2153+
'showcase_inquiry',
2154+
);
2155+
expect(r.status).toBe(403);
2156+
expect(r.body.code).toBe('FORBIDDEN');
2157+
expect(r.body.object).toBe('showcase_inquiry');
2158+
expect(r.body.error).toContain('insufficient privileges');
2159+
});
2160+
2161+
it('does NOT pass through an explicit 5xx status (message stays sanitized)', () => {
2162+
const r = mapDataError(
2163+
Object.assign(new Error('connect ECONNREFUSED 10.0.0.5:5432 (internal pool)'), { status: 502 }),
2164+
);
2165+
// 5xx bypasses the passthrough and falls into the sanitizing heuristics.
2166+
expect(r.status).not.toBe(502);
2167+
expect(r.body.code).not.toBe('FORBIDDEN');
2168+
});
2169+
2170+
it('guards the passthrough message length (oversized → generic text)', () => {
2171+
const r = mapDataError(Object.assign(new Error('x'.repeat(600)), { status: 403, code: 'FORBIDDEN' }));
2172+
expect(r.status).toBe(403);
2173+
expect(r.body.error).toBe('Request failed');
2174+
});
2175+
2176+
it('keeps CONCURRENT_UPDATE 409 structured fields despite its own status property', () => {
2177+
const r = mapDataError(
2178+
Object.assign(new Error('Record was modified'), {
2179+
code: 'CONCURRENT_UPDATE',
2180+
status: 409,
2181+
currentVersion: 7,
2182+
currentRecord: { id: 'r1' },
2183+
}),
2184+
'sys_task',
2185+
);
2186+
expect(r.status).toBe(409);
2187+
expect(r.body.code).toBe('CONCURRENT_UPDATE');
2188+
expect(r.body.currentVersion).toBe(7);
2189+
expect(r.body.currentRecord).toEqual({ id: 'r1' });
2190+
});
2191+
2192+
it('keeps DELETE_RESTRICTED 409 structured fields despite its own status property', () => {
2193+
const r = mapDataError(
2194+
Object.assign(new Error('Cannot delete sys_position (p1): 1 dependent record'), {
2195+
code: 'DELETE_RESTRICTED',
2196+
status: 409,
2197+
dependentObject: 'sys_position_permission_set',
2198+
dependentCount: 1,
2199+
}),
2200+
'sys_position',
2201+
);
2202+
expect(r.status).toBe(409);
2203+
expect(r.body.code).toBe('DELETE_RESTRICTED');
2204+
expect(r.body.dependentCount).toBe(1);
2205+
});
2206+
21432207
it('maps SQLite "has no column named" → 400 INVALID_FIELD with the field', () => {
21442208
const r = mapDataError(
21452209
sqliteError(

0 commit comments

Comments
 (0)