From 53d809859e9f32d4c4c70725d0187f32b5b896b6 Mon Sep 17 00:00:00 2001 From: os-help Date: Tue, 11 Aug 2026 05:31:36 +0000 Subject: [PATCH] fix(rest): honour a hook refusal's declared `statusCode` at the `/api/v1/data` error boundary (#7525) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A write refused by an engine lifecycle hook that declared an explicit status came back as an opaque `500 INTERNAL_ERROR` with no `code`, while the server log held the correctly-shaped refusal. Two cases, each reproduced 2x by QA: `PATCH` a record with a `lockRecord:true` approval pending (log: `RECORD_LOCKED`) and `POST sys_approval_delegation` with a `delegator_id` the caller does not own (log: `FORBIDDEN ... statusCode 403`). The seam is `mapDataError`'s explicit-status passthrough, which opened on `typeof error.status === 'number'` and nothing else. The hooks declare their status as `statusCode`, so the refusal never entered that branch at all: it fell past every structured branch, matched no message heuristic, and left through `UNCLASSIFIED_FAULT`. #5582 widened the same branch's RANGE (4xx -> 400-599) and could not have covered this — the status was lost one question earlier. Fixed at the boundary, not at the two hooks: `status` -> `statusCode` -> default is already what every other HTTP exit in this repo reads (`errorFromThrown`, `errorResponseBase`, `endpoint-executor`, `domains/actions`, plugin-hono-server), so one thrown error answered 403 through a dispatcher route and 500 through `/api/v1/data`. The gate becomes a named `declaredHttpStatus()` asking the same 400-599 band over both spellings. `declaresServerFault`'s own `status`-only read is unchanged (#5811 ruled a DISCLOSURE rule must not depend on a spelling); the one call site inside the passthrough is handed the status just resolved, so a `{ statusCode: 5xx, code }` producer cannot ship its status with the code dropped. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01FGZ4X1ezVSLg8E9NvJitAE --- .../rest-hook-refusal-status-passthrough.md | 80 +++ ...st-hook-refusal-status-passthrough.test.ts | 534 ++++++++++++++++++ packages/rest/src/rest-server.ts | 94 ++- 3 files changed, 703 insertions(+), 5 deletions(-) create mode 100644 .changeset/rest-hook-refusal-status-passthrough.md create mode 100644 packages/rest/src/rest-hook-refusal-status-passthrough.test.ts diff --git a/.changeset/rest-hook-refusal-status-passthrough.md b/.changeset/rest-hook-refusal-status-passthrough.md new file mode 100644 index 0000000000..0cea6aa857 --- /dev/null +++ b/.changeset/rest-hook-refusal-status-passthrough.md @@ -0,0 +1,80 @@ +--- +'@objectstack/rest': patch +--- + +fix(rest): a hook refusal that declares its status as `statusCode` reaches the wire with that status, not `500 INTERNAL_ERROR` (#7525) + +**Observable behaviour change — read this if you alert or retry on `/api/v1/data` statuses.** +A write refused by an engine lifecycle hook that declared an explicit status used +to answer `500 INTERNAL_ERROR` with no `code`. It now answers the status the hook +declared, carrying the hook's ADR-0112 `code`. Two refusals QA reproduced 2× each +move from `500` to `409 RECORD_LOCKED` and `403 FORBIDDEN`. Monitoring that counted +these as server faults will see a 5xx disappear and a 4xx appear, and a client +retrying on 5xx will stop retrying a request that can never succeed. + +## What was wrong + +`mapDataError` — the error exit for the ~11 CRUD data routes, which bypass +`resolveErrorResponse` entirely — opened its explicit-status passthrough on +`typeof error.status === 'number'` and nothing else. An engine lifecycle hook +declares its status as `statusCode`: + +```ts +// plugin-approvals/src/lifecycle-hooks.ts +err.code = 'RECORD_LOCKED'; err.statusCode = 409; // a pending lockRecord approval +err.code = 'FORBIDDEN'; err.statusCode = 403; // a delegation row the caller does not own +``` + +so the refusal never entered that branch at all. It fell past every structured +branch, matched no message heuristic, and left through `UNCLASSIFIED_FAULT` as +`500 INTERNAL_ERROR` — for a deliberate, well-understood business refusal, with +the correctly-shaped original sitting in the server log. The console never hit +the record-lock case (the affordance is disabled while a lock is live); a direct +API caller — script, integration, second-party client — got an unactionable 500. + +**#5582 is not the fix and could not have been.** It widened this same +passthrough's *range* (4xx → 400-599) for producers that declared `status`. The +loss here is one question earlier: *whether* a status was declared at all. + +## The fix, and why it is at the boundary + +`status` → `statusCode` → default is what **every other HTTP exit in this repo** +already reads — `runtime`'s `HttpDispatcher.errorFromThrown` (#3867), +`dispatcher-plugin.errorResponseBase`, `endpoint-executor`, `domains/actions`, +`plugin-hono-server`'s user endpoints. `mapDataError` was the single exit that +read one spelling, which is why one thrown error came back as `403` through a +dispatcher route and as `500` through `/api/v1/data`. The gate is now a named +`declaredHttpStatus(error)` helper asking the same 400-599 band over both +spellings. + +Teaching the two approvals hooks to spell it `status` would have fixed two +producers and left the boundary answering 500 for the next one — including +`runtime`'s own `action-execution.ts` (`{ statusCode: 503 | 501 | 400 }`) and +`metadata-protocol` (`{ statusCode: 404 }`). The hooks are unchanged. + +## What deliberately did NOT change + +- ⛔ **`declaresServerFault`'s own read is still `status`-only.** #5811 ruled that + a *disclosure* rule must not depend on a producer's spelling, and that is + untouched. This is *status resolution*, a different question, and the one call + site inside the passthrough hands the predicate the status this boundary just + resolved — otherwise a `{ statusCode: 5xx, code }` producer would take the 5xx + arm and then be told it declared no fault, dropping its code. +- **The 5xx withhold is unconditional as before.** A `statusCode`-declared 5xx + gets `INTERNAL_ERROR_MESSAGE` plus its code; no producer prose crosses the + boundary, and the full text still reaches the operator. +- **A hook that declares NO status is unchanged** — still judged by the + classifiers, still the terminal sanitised `500 INTERNAL_ERROR`. Promoting a + bare `code` to a 4xx would be consumer-side leniency; that belongs with #7463, + not here. +- **The structured branches keep their precedence.** `OBJECT_NOT_FOUND`, + `DELETE_RESTRICTED`, `VALIDATION_FAILED` and the rest still sit above the + passthrough and still win, `statusCode` or not. +- **`resolveErrorResponse` still reads `status` only.** It delegates to + `mapDataError` for everything it does not pass through, so both doors already + give one wire answer without a second copy of the two-spelling read. + +Coverage: `rest-hook-refusal-status-passthrough.test.ts` — 26 cases, including +both reported requests walked in process on the real `PATCH /data/:object/:id` +and `POST /data/:object` routes. Run against unmodified `main` the file is 15/26 +red; three further mutations cover the remaining 11, so no case is unfalsifiable. diff --git a/packages/rest/src/rest-hook-refusal-status-passthrough.test.ts b/packages/rest/src/rest-hook-refusal-status-passthrough.test.ts new file mode 100644 index 0000000000..0bf1fab4e4 --- /dev/null +++ b/packages/rest/src/rest-hook-refusal-status-passthrough.test.ts @@ -0,0 +1,534 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// [#7525] An engine lifecycle HOOK that refuses a write with an explicit status +// gets that status — and its ADR-0112 `code` — onto the wire through +// `/api/v1/data`, instead of an opaque `500 INTERNAL_ERROR`. +// +// --------------------------------------------------------------------------- +// The seam, because the card explicitly asked where the status was lost rather +// than assuming the passthrough needed widening again. +// +// The two producers QA reproduced (2× each) declare their status as +// `statusCode`, which is what `runtime`'s own exits have read as a second +// spelling since #3867: +// +// plugin-approvals/src/lifecycle-hooks.ts:122 lockedError() +// err.code = 'RECORD_LOCKED'; err.statusCode = 409; +// plugin-approvals/src/lifecycle-hooks.ts:440 deny() (delegation guard) +// err.code = 'FORBIDDEN'; err.statusCode = 403; +// +// `mapDataError`'s passthrough asked `typeof error.status === 'number'` and +// nothing else, so the refusal never entered that branch at all: it fell past +// every structured branch, matched no message heuristic, and left through +// `UNCLASSIFIED_FAULT` as `500 INTERNAL_ERROR` with no `code`. #5582 (PR #7402) +// widened that branch's RANGE (4xx → 400-599) and could not have covered this — +// the loss is one question earlier, at "did the producer declare a status". +// +// Every OTHER HTTP exit in the repo already reads both spellings — +// `HttpDispatcher.errorFromThrown`, `dispatcher-plugin.errorResponseBase` +// (`runtime/src/dispatcher-plugin.ts:494-497`), `endpoint-executor`, +// `domains/actions`, `plugin-hono-server`'s user endpoints — so one thrown +// error answered `403` through a dispatcher route and `500` through +// `/api/v1/data`. Fixed at the boundary, not at the two hooks: the hooks are +// well-behaved producers, and `runtime/src/action-execution.ts` +// (`{ statusCode: 503 | 501 | 400 }`) and `metadata-protocol` +// (`{ statusCode: 404 }`) throw the same shape at the same routes. +// +// ⚠️ `declaresServerFault`'s own `status`-only read is UNCHANGED (#5811 ruled +// that a DISCLOSURE rule must not depend on a producer's spelling). This file +// pins that separation from both sides: §4 asserts the resolved-status call +// keeps the `code` on a `statusCode`-declared 5xx, and that an empty / numeric +// code is still not a declaration. +// +// --------------------------------------------------------------------------- +// Mutation table. Every one of the 26 cases below was PROVEN able to fail — +// none is covered by fewer than one mutation. Directions were predicted BEFORE +// running; where a prediction was wrong it is recorded as MEASURED rather than +// rewritten to fit. +// +// A · BASELINE — the file run against unmodified `main`, i.e. the real defect +// rather than a synthetic mutation (identical result to deleting the +// `?? error.statusCode` limb from `declaredHttpStatus`). +// §1 predicted RED, measured 6/8 red — prediction said 7/8. The two +// survivors are direction-insensitive BY CONSTRUCTION: "a `statusCode` +// outside 400-599 is not a declaration" expects the 500 the unfixed +// code already gives, and "`status` wins over `statusCode`" reads a +// `status` the unfixed code already honours. Mutation D moves both. +// §2 predicted RED, measured 4/4 red — the two reported requests +// answer `500 INTERNAL_ERROR` on the wire, exactly as QA measured. +// §3 predicted GREEN, measured 3/3 green — the no-status default is what +// this section pins; nothing about `statusCode` can move it. +// §4 predicted RED, measured 5/6 red — prediction said 2/6 and was +// WRONG about the mechanism: the empty-code / numeric-code cases fail +// too, because their STATUS collapses to 500 before their `code` +// assertion is ever the interesting half. +// §5 predicted GREEN, measured 6/6 green — the structured branches sit +// ABOVE the passthrough and the undeclared band never enters it. +// Total: 15 of 26 red. +// +// B · `declaredHttpStatus` returns 400 for an UNDECLARED error (the "every hook +// refusal is now a 4xx" overreach the fix must not become). +// predicted RED for §3 + the undeclared half of §5, measured 4 red: +// §3's two undeclared cases and §5's raw-driver / non-object cases. +// +// C · The passthrough is allowed to OUTRANK the structured branches +// (`&& declaredHttpStatus(error) === undefined` added to the +// OBJECT_NOT_FOUND / DELETE_RESTRICTED / VALIDATION_FAILED guards) and the +// sandbox unwrap is removed. +// predicted RED for the ordering pins, measured 4 red: §5's three +// structured cases and §3's sandbox case. +// +// D · Three independent loosenings of the helper at once — the 400-599 band +// check dropped, the `status`/`statusCode` precedence inverted, and the 5xx +// arm's `declaresServerFault` call deleted. +// predicted RED for the four cases A left green, measured exactly those +// 4 red. +// --------------------------------------------------------------------------- + +import { describe, it, expect, vi, beforeEach, afterEach } from 'vitest'; +import { INTERNAL_ERROR_MESSAGE } from '@objectstack/types'; +import { mapDataError, RestServer } from './rest-server.js'; + +const DATA_ITEM = '/api/v1/data/:object/:id'; +const DATA_COLLECTION = '/api/v1/data/:object'; + +// --------------------------------------------------------------------------- +// Producer fixtures — COPIED from the shipping hooks, not invented. +// +// `@objectstack/rest` must not depend on `plugin-approvals` to run its own +// tests, so the shapes are reproduced here byte for byte. What makes them a +// valid fixture is precisely what makes the unfixed code answer 500: the status +// is on `statusCode`, and there is no `status` property at all. +// --------------------------------------------------------------------------- + +/** `lifecycle-hooks.ts` `lockedError()` — case (a) of the report. */ +function recordLockedError(message = 'showcase_task rec1 has a pending approval that locks it') { + const err: any = new Error(`RECORD_LOCKED: ${message}`); + err.code = 'RECORD_LOCKED'; + err.statusCode = 409; + return err; +} + +/** `lifecycle-hooks.ts` `bindDelegationWriteGuard`'s `deny()` — case (b). */ +function delegationForbiddenError(userId = 'u1') { + const err: any = new Error( + 'FORBIDDEN: you may only manage out-of-office delegations where you are the delegator' + + ` ('${userId}')`, + ); + err.code = 'FORBIDDEN'; + err.statusCode = 403; + return err; +} + +/** + * A hook that refuses with NO status at all — the third case the card asked for, + * so what the default STAYS is pinned rather than left to drift. This is the + * overwhelming majority shape: `throw new Error('…')` from a hook body. + */ +function unstatusedHookRefusal() { + const err: any = new Error('this record cannot be edited during month-end close'); + err.code = 'CLOSE_PERIOD_LOCKED'; + return err; +} + +// --------------------------------------------------------------------------- +// §1 The mapping itself +// --------------------------------------------------------------------------- + +describe('[#7525] mapDataError: a hook refusal keeps the status it declared', () => { + it('case (a): RECORD_LOCKED answers 409 with its code, not 500 INTERNAL_ERROR', () => { + const r = mapDataError(recordLockedError(), 'showcase_task'); + + expect(r.status).toBe(409); + expect(r.body.code).toBe('RECORD_LOCKED'); + // The measured defect, pinned as a NEGATIVE so a partial fix (status + // restored, code still overwritten) cannot pass. + expect(r.status).not.toBe(500); + expect(r.body.code).not.toBe('INTERNAL_ERROR'); + }); + + it('case (b): the delegation refusal answers 403 FORBIDDEN', () => { + const r = mapDataError(delegationForbiddenError(), 'sys_approval_delegation'); + + expect(r.status).toBe(403); + expect(r.body.code).toBe('FORBIDDEN'); + expect(r.status).not.toBe(500); + }); + + it("the refusal's own words reach the caller — a 4xx message IS the remedy", () => { + // The whole complaint in the report is "no located guidance". A hook + // refusal is a deliberate business answer addressed TO the caller, so + // the 4xx arm's verbatim rule (#5423) applies to it like any other. + const r = mapDataError(recordLockedError(), 'showcase_task'); + expect(r.body.error).toContain('pending approval'); + expect(r.body.error).not.toBe(INTERNAL_ERROR_MESSAGE); + expect(r.body.object).toBe('showcase_task'); + }); + + it('the envelope is the same one a `status`-declaring producer gets — one wire answer', () => { + // The point of the fix: the two spellings are one question. Byte-equal + // bodies, not merely equal statuses. + const viaStatusCode = mapDataError(delegationForbiddenError('u1'), 'sys_approval_delegation'); + const asStatus = delegationForbiddenError('u1'); + delete asStatus.statusCode; + asStatus.status = 403; + const viaStatus = mapDataError(asStatus, 'sys_approval_delegation'); + + expect(viaStatusCode).toEqual(viaStatus); + }); + + it('the whole 400-599 band is read off `statusCode`, not a hand-picked list', () => { + for (const statusCode of [400, 403, 404, 409, 422, 429, 451, 499]) { + const err: any = new Error('refused by hook'); + err.code = 'HOOK_REFUSED'; + err.statusCode = statusCode; + const r = mapDataError(err, 'showcase_task'); + expect(r.status).toBe(statusCode); + expect(r.body.code).toBe('HOOK_REFUSED'); + } + }); + + it('a `statusCode` outside 400-599 is not a declaration — the heuristics still judge it', () => { + // Same upper/lower bound `resolveErrorResponse` opens. A `statusCode: + // 200` or `600` is nonsense as a refusal and must not become the wire + // status; it falls to the classifiers, which recognise nothing here. + for (const statusCode of [0, 200, 302, 399, 600, 999]) { + const err: any = new Error('nothing recognisable here'); + err.statusCode = statusCode; + const r = mapDataError(err, 'showcase_task'); + expect(r.status).toBe(500); + expect(r.body.code).toBe('INTERNAL_ERROR'); + } + }); + + it('a numeric `status` still WINS over `statusCode` — precedence matches every other exit', () => { + // `status` → `statusCode` is the order `errorFromThrown` and + // `errorResponseBase` read. Pinned so a future edit cannot silently + // invert it. + const err: any = new Error('refused'); + err.code = 'HOOK_REFUSED'; + err.status = 409; + err.statusCode = 403; + expect(mapDataError(err, 'showcase_task').status).toBe(409); + }); + + it("a STRING `status` does not block `statusCode` — better-auth's APIError resolves", () => { + // `better-call` throws `{ statusCode: 403, status: 'FORBIDDEN' }` — the + // `status` field is a string there. `plugin-auth` already reads it + // `statusCode`-first for exactly this reason. + const err: any = new Error('You are not allowed to do this'); + err.code = 'FORBIDDEN'; + err.status = 'FORBIDDEN'; + err.statusCode = 403; + const r = mapDataError(err, 'sys_user'); + expect(r.status).toBe(403); + expect(r.body.code).toBe('FORBIDDEN'); + }); +}); + +// --------------------------------------------------------------------------- +// §2 The two reported routes, walked in process +// +// §1 calls `mapDataError` directly. This section proves the WIRE answer on the +// exact two requests QA reproduced — the direct-call territory that bypasses +// `resolveErrorResponse` entirely. +// --------------------------------------------------------------------------- + +function createMockServer() { + return { + get: vi.fn(), post: vi.fn(), put: vi.fn(), delete: vi.fn(), patch: vi.fn(), use: vi.fn(), + listen: vi.fn().mockResolvedValue(undefined), close: vi.fn().mockResolvedValue(undefined), + }; +} + +function makeRes() { + const res: any = { statusCode: 200, body: undefined }; + res.status = vi.fn((c: number) => { res.statusCode = c; return res; }); + res.json = vi.fn((b: any) => { res.body = b; return res; }); + res.header = vi.fn(() => res); + res.setHeader = vi.fn(); res.write = vi.fn(); res.end = vi.fn(); res.send = vi.fn(); + return res; +} + +function setup(protocolOverrides: Record = {}) { + const protocol: any = { + getDiscovery: vi.fn().mockResolvedValue({ + version: 'v0', routes: { data: '', metadata: '', ui: '', auth: '/auth' }, + }), + getMetaTypes: vi.fn().mockResolvedValue([]), + getMetaItems: vi.fn().mockResolvedValue([{ name: 'showcase_task' }]), + getMetaItem: vi.fn().mockResolvedValue({}), + findData: vi.fn().mockResolvedValue([]), + createData: vi.fn().mockResolvedValue({}), + updateData: vi.fn().mockResolvedValue({}), + ...protocolOverrides, + }; + const rest = new RestServer( + createMockServer() as any, + protocol, + { api: { requireAuth: false } } as any, + ); + (rest as any).resolveExecCtx = async () => ({ userId: 'u1' }); + rest.registerRoutes(); + return rest; +} + +function routeOf(rest: any, method: string, path: string) { + const route = rest.getRoutes().find((r: any) => r.method === method && r.path === path); + if (!route) throw new Error(`${method} ${path} route not registered`); + return route; +} + +/** `PATCH /api/v1/data/{object}/{id}` — the report's case (a) request. */ +async function callPatch(rest: any, object: string, id: string, body: Record) { + const res = makeRes(); + await routeOf(rest, 'PATCH', DATA_ITEM).handler( + { method: 'PATCH', params: { object, id }, query: {}, headers: {}, body }, + res, + ); + return res; +} + +/** `POST /api/v1/data/{object}` — the report's case (b) request. */ +async function callPost(rest: any, object: string, body: Record) { + const res = makeRes(); + await routeOf(rest, 'POST', DATA_COLLECTION).handler( + { method: 'POST', params: { object }, query: {}, headers: {}, body }, + res, + ); + return res; +} + +let errorSpy: ReturnType; +beforeEach(() => { errorSpy = vi.spyOn(console, 'error').mockImplementation(() => {}); }); +afterEach(() => { errorSpy.mockRestore(); }); + +describe('[#7525] the reported requests, on the real CRUD data routes', () => { + it('(a) PATCH a locked record → 409 RECORD_LOCKED on the wire', async () => { + const rest = setup({ updateData: vi.fn().mockRejectedValue(recordLockedError()) }); + + const res = await callPatch(rest, 'showcase_task', 'rec1', { name: 'edited' }); + + expect(res.statusCode).toBe(409); + expect(res.body.code).toBe('RECORD_LOCKED'); + expect(res.statusCode).not.toBe(500); + expect(res.body.error).not.toBe(INTERNAL_ERROR_MESSAGE); + }, 60_000); + + it('(b) POST a delegation the caller does not own → 403 FORBIDDEN on the wire', async () => { + const rest = setup({ createData: vi.fn().mockRejectedValue(delegationForbiddenError('u1')) }); + + const res = await callPost(rest, 'sys_approval_delegation', { + delegator_id: 'someone_else', delegate_id: 'u2', + }); + + expect(res.statusCode).toBe(403); + expect(res.body.code).toBe('FORBIDDEN'); + expect(res.body.error).toContain('delegator'); + }, 60_000); + + it('the refusal is not logged as an unhandled fault — 403/409 are expected outcomes', async () => { + // `isExpectedDataStatus` covers 403 and 409, so a deliberate refusal + // reaching its real status also stops producing a scary + // "[REST] Unhandled error" line. Before the fix it was a 500 and did. + const rest = setup({ updateData: vi.fn().mockRejectedValue(recordLockedError()) }); + + await callPatch(rest, 'showcase_task', 'rec1', { name: 'edited' }); + + const logged = errorSpy.mock.calls.some( + (call: unknown[]) => JSON.stringify(call.map(String)).includes('Unhandled error'), + ); + expect(logged).toBe(false); + }, 60_000); + + it('a GET list refused by a beforeFind hook keeps its status too — not a write-only fix', async () => { + const err: any = new Error('this object is not readable outside business hours'); + err.code = 'FORBIDDEN'; + err.statusCode = 403; + const rest = setup({ findData: vi.fn().mockRejectedValue(err) }); + + const res = makeRes(); + await routeOf(rest, 'GET', DATA_COLLECTION).handler( + { method: 'GET', params: { object: 'showcase_task' }, query: {}, headers: {} }, + res, + ); + + expect(res.statusCode).toBe(403); + expect(res.body.code).toBe('FORBIDDEN'); + }, 60_000); +}); + +// --------------------------------------------------------------------------- +// §3 The hook that declares NO status — what the default STAYS +// +// GREEN under the mutation by design. The card asked for it so the fix cannot +// be read as "every hook refusal is now a 4xx": a hook that declares nothing is +// still judged by the classifiers, exactly as before. +// --------------------------------------------------------------------------- + +describe('[#7525] a hook that refuses WITHOUT a status is unchanged', () => { + it('an undeclared refusal still answers the terminal sanitised 500', () => { + const r = mapDataError(unstatusedHookRefusal(), 'showcase_task'); + + expect(r.status).toBe(500); + expect(r.body.code).toBe('INTERNAL_ERROR'); + // Its own `code` is NOT promoted to the wire by this fix: ADR-0112 says + // the producer names the condition, and this producer named a code but + // no status. Inventing a 4xx from the code alone would be the + // consumer-side leniency Prime Directive #12 removes — the follow-up + // that belongs with #7463, not here. + expect(r.body.code).not.toBe('CLOSE_PERIOD_LOCKED'); + }); + + it('a sandboxed hook body is still unwrapped to its business message at 400', () => { + // The QuickJS path (`SandboxError.innerMessage`) sits ABOVE the + // passthrough and is untouched: it has no status to declare. + const err: any = new Error("hook 'block_edit' threw: Error: 删除被阻断"); + err.innerMessage = '删除被阻断'; + const r = mapDataError(err, 'showcase_task'); + expect(r.status).toBe(400); + expect(r.body.error).toBe('删除被阻断'); + }); + + it('an undeclared refusal on the wire is a 500 with nothing of its words', async () => { + const rest = setup({ updateData: vi.fn().mockRejectedValue(unstatusedHookRefusal()) }); + + const res = await callPatch(rest, 'showcase_task', 'rec1', { name: 'edited' }); + + expect(res.statusCode).toBe(500); + expect(res.body.code).toBe('INTERNAL_ERROR'); + }, 60_000); +}); + +// --------------------------------------------------------------------------- +// §4 The 5xx half — `declaresServerFault` is asked over the RESOLVED status +// +// The predicate's own read stays `status`-only (#5811, a disclosure rule). What +// this section pins is that the ONE call site inside the passthrough hands it +// the status the boundary just resolved — otherwise a `statusCode`-declared 5xx +// would take the 5xx arm and then be told it declared no fault, shipping the +// status with its ADR-0112 code silently dropped. +// --------------------------------------------------------------------------- + +describe('[#7525] a `statusCode`-declared 5xx keeps its code, and the prose is still withheld', () => { + it('{ statusCode: 503, code } ships the status AND the code', () => { + // `runtime/src/action-execution.ts` throws exactly this shape. + const err: any = new Error('Data service not available: connect ECONNREFUSED 10.0.0.5:5432'); + err.code = 'SERVICE_UNAVAILABLE'; + err.statusCode = 503; + const r = mapDataError(err, 'showcase_task'); + + expect(r.status).toBe(503); + expect(r.body.code).toBe('SERVICE_UNAVAILABLE'); + // The 5xx arm's withhold is unconditional and applies here identically. + expect(r.body.error).toBe(INTERNAL_ERROR_MESSAGE); + expect(JSON.stringify(r.body)).not.toContain('10.0.0.5'); + expect(JSON.stringify(r.body)).not.toContain('ECONNREFUSED'); + }); + + it('{ statusCode: 501, code } — the same answer the `status` spelling gets', () => { + const viaStatusCode: any = Object.assign(new Error('not compiled by this backend'), { + code: 'NOT_IMPLEMENTED', statusCode: 501, + }); + const viaStatus: any = Object.assign(new Error('not compiled by this backend'), { + code: 'NOT_IMPLEMENTED', status: 501, + }); + expect(mapDataError(viaStatusCode, 'showcase_task')).toEqual(mapDataError(viaStatus, 'showcase_task')); + }); + + it('a `statusCode` 5xx with NO code invents nothing', () => { + const err: any = new Error('boom'); + err.statusCode = 502; + const r = mapDataError(err); + expect(r.status).toBe(502); + expect(r.body.code).toBeUndefined(); + expect(r.body.error).toBe(INTERNAL_ERROR_MESSAGE); + }); + + it('an EMPTY-string code is still not a declaration', () => { + const err: any = new Error('boom'); + err.code = ''; + err.statusCode = 503; + const r = mapDataError(err); + expect(r.status).toBe(503); + expect(r.body.code).toBeUndefined(); + }); + + it('a NON-STRING code is still not a declaration — a driver errno is not a catalog entry', () => { + const err: any = new Error('boom'); + err.code = 1062; + err.statusCode = 502; + const r = mapDataError(err); + expect(r.status).toBe(502); + expect(r.body.code).toBeUndefined(); + }); + + it('#5582 is untouched — a `status`-declared 5xx answers exactly as it did', () => { + const r = mapDataError( + Object.assign(new Error('internal detail'), { status: 501, code: 'NOT_IMPLEMENTED' }), + 'showcase_account', + ); + expect(r).toEqual({ + status: 501, + body: { error: INTERNAL_ERROR_MESSAGE, code: 'NOT_IMPLEMENTED' }, + }); + }); +}); + +// --------------------------------------------------------------------------- +// §5 Non-regression: the branches ABOVE the passthrough, and the undeclared band +// +// GREEN under the mutation by design. +// --------------------------------------------------------------------------- + +describe('[#7525] the structured branches and the heuristics are untouched', () => { + it('a `statusCode`-declaring OBJECT_NOT_FOUND keeps its canonical 404 envelope', () => { + const err: any = new Error('gone'); + err.code = 'OBJECT_NOT_FOUND'; + err.statusCode = 503; + const r = mapDataError(err, 'showcase_account'); + expect(r.status).toBe(404); + expect(r.body.code).toBe('OBJECT_NOT_FOUND'); + }); + + it('a `statusCode`-declaring DELETE_RESTRICTED still answers 409 with its fields', () => { + const err: any = new Error('dependents exist'); + err.code = 'DELETE_RESTRICTED'; + err.statusCode = 500; + err.dependentCount = 3; + const r = mapDataError(err, 'sys_position'); + expect(r.status).toBe(409); + expect(r.body.dependentCount).toBe(3); + }); + + it('a `statusCode`-declaring VALIDATION_FAILED still answers 400 with `fields[]`', () => { + const err: any = new Error('Validation failed'); + err.code = 'VALIDATION_FAILED'; + err.statusCode = 500; + err.fields = [{ field: 'email', code: 'invalid', message: 'bad email' }]; + const r = mapDataError(err, 'sys_user'); + expect(r.status).toBe(400); + expect(r.body.fields).toEqual(err.fields); + }); + + it('a raw driver error carries no status in EITHER spelling and is still classified', () => { + const leak = mapDataError(new Error('SQLITE_ERROR: no such table: some_aux_table'), 'showcase_account'); + expect(leak.status).toBe(500); + expect(leak.body.code).toBe('DATABASE_ERROR'); + + const dup = mapDataError( + Object.assign(new Error("ER_DUP_ENTRY: Duplicate entry 'a@b.com' for key 'idx_email_unique'"), { + code: 'ER_DUP_ENTRY', + }), + 'sys_user', + ); + expect(dup.status).toBe(409); + expect(dup.body.code).toBe('UNIQUE_VIOLATION'); + }); + + it('a non-object throw is not a declaration and does not crash the read', () => { + expect(mapDataError('a bare string').status).toBe(500); + expect(mapDataError(undefined).status).toBe(500); + expect(mapDataError(null).status).toBe(500); + }); +}); diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index 973b446155..1418e3c3a5 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -583,6 +583,63 @@ const UNCLASSIFIED_FAULT = (): { status: number; body: Record } * match: the fail-loud direction is what this issue asked for, and there is no * producer of the bare `table not found` phrasing in this repo to regress. */ +/** + * [#7525] The HTTP status a producer DECLARED for this error, or `undefined` + * when it declared none — read over BOTH spellings the repo's producers use, + * `status` first and `statusCode` second. + * + * **This is the seam the hook-refusal defect lived on.** `mapDataError`'s + * passthrough asked `typeof error.status === 'number'` and nothing else, while + * an engine lifecycle hook that refuses a write declares its status as + * `statusCode`: + * + * ```ts + * // plugin-approvals/src/lifecycle-hooks.ts + * err.code = 'RECORD_LOCKED'; err.statusCode = 409; // a pending lockRecord approval + * err.code = 'FORBIDDEN'; err.statusCode = 403; // a forged delegation row + * ``` + * + * So the refusal never reached the passthrough at all: it fell past every + * structured branch, matched no message heuristic, and left through + * `UNCLASSIFIED_FAULT` as `500 INTERNAL_ERROR` with no `code` — for a + * deliberate, well-understood business refusal, on every direct `/api/v1/data` + * caller. #5582 widened that same passthrough's *range* (4xx -> 400-599) and is + * not the fix here; the status was being dropped one question earlier, at + * "did the producer declare one". + * + * **Why the boundary rather than the two hooks.** `status` -> `statusCode` -> default + * is already what EVERY other HTTP exit in this repo reads — `runtime`'s + * `HttpDispatcher.errorFromThrown` (#3867), `dispatcher-plugin.errorResponseBase`, + * `endpoint-executor`, `domains/actions`, `plugin-hono-server`'s user endpoints. + * `mapDataError` was the one exit that read a single spelling, which is why one + * thrown error came back as `403` through a dispatcher route and as `500` + * through `/api/v1/data`. Teaching the two approvals hooks to spell it `status` + * would fix two producers and leave the boundary answering 500 for the next + * one — including `runtime`'s own `action-execution.ts`, which throws + * `{ statusCode: 503 | 501 | 400 }`, and `metadata-protocol`'s + * `{ statusCode: 404 }`. The producers are well-behaved; the exit was strict + * about a spelling nobody standardised. + * + * ⚠️ Deliberately NOT the same question as {@link declaresServerFault}, whose + * `status`-only read is UNCHANGED and stays that way (#5811): that predicate is + * a *disclosure* rule — "may this message be withheld" — and was ruled to not + * depend on which spelling a producer reached for. This is *status resolution*, + * the read that has always been two-spelling everywhere else. + * + * The band is the same 400-599 {@link resolveErrorResponse} opens, so a + * nonsense status is not a declaration. A non-numeric `status` falls through to + * `statusCode` rather than blocking it, which is what makes better-auth's + * `APIError` (`{ statusCode: 403, status: 'FORBIDDEN' }` — the status field is a + * STRING there) resolve to the status it meant instead of to nothing. + */ +function declaredHttpStatus(error: any): number | undefined { + const declared = + (typeof error?.status === 'number' ? error.status : undefined) ?? + (typeof error?.statusCode === 'number' ? error.statusCode : undefined); + if (declared === undefined || !(declared >= 400 && declared < 600)) return undefined; + return declared; +} + function missingRelationIsObject(raw: string, object: string | undefined): boolean { if (!object) return false; const named = @@ -843,7 +900,13 @@ export function mapDataError(error: any, object?: string): { status: number; bod // fires precisely when a response dropped the error's own message — so the // 502/503 band that `isExpectedRouteError` keeps quiet still leaves the // operator a line carrying the full original error. - if (typeof error?.status === 'number' && error.status >= 400 && error.status < 600) { + // + // [#7525] The gate is {@link declaredHttpStatus} rather than an in-line read + // of `error.status`: the same 400-599 band, asked over both spellings a + // producer may have declared it in. See that docblock for why an engine + // hook's refusal never reached this branch at all. + const declaredStatus = declaredHttpStatus(error); + if (declaredStatus !== undefined) { // [#5582] A declared server fault: keep the status, keep the // machine-readable `code`, drop the prose. Byte-identical to // {@link resolveErrorResponse}'s 5xx arm — one condition, one wire @@ -869,12 +932,26 @@ export function mapDataError(error: any, object?: string): { status: number; bod // would put a code on the wire the producer never wrote — while // re-deriving the status from the message text is the defect this // branch exists to remove. - if (error.status >= 500) { + // + // [#7525] It is asked over the RESOLVED status — `declaresServerFault({ + // status: declaredStatus, code: error?.code })` — not over the raw + // error, and the two arguments are the predicate's entire input, so + // nothing about its verdict is loosened. Asking it over the raw error + // instead would split this branch against itself: a producer declaring + // `{ statusCode: 503, code: 'SERVICE_UNAVAILABLE' }` would take the 5xx + // arm (the status resolved) and then be told it declared no server + // fault (the `status` field being absent), shipping a 503 with its + // ADR-0112 code silently dropped. The predicate's OWN read stays + // `status`-only for its own callers — this is one call site handing it + // the status this boundary just resolved. + if (declaredStatus >= 500) { return { - status: error.status, + status: declaredStatus, body: { error: INTERNAL_ERROR_MESSAGE, - ...(declaresServerFault(error) ? { code: error.code as string } : {}), + ...(declaresServerFault({ status: declaredStatus, code: error?.code }) + ? { code: error.code as string } + : {}), }, }; } @@ -888,7 +965,7 @@ export function mapDataError(error: any, object?: string): { status: number; bod ? truncateClientMessage(error.message) : 'Request failed'; return { - status: error.status, + status: declaredStatus, body: { error: msg, ...(typeof error?.code === 'string' && error.code ? { code: error.code } : {}), @@ -1254,6 +1331,13 @@ function resolveErrorResponse(error: any, object?: string): { status: number; bo // status-passthrough: `mapDataError` owns its canonical envelope // (`OBJECT_NOT_FOUND`), and short-circuiting here would ship a second wire // code for the same condition depending on which route caught it. + // + // [#7525] Deliberately still a `status`-only read HERE. An error that + // declares its status as `statusCode` instead is not skipped — it falls to + // `mapDataError` below, whose {@link declaredHttpStatus} gate reads both + // spellings and answers with the same status/code/withhold rules this arm + // applies. So the two doors already agree on the wire answer, and this one + // is not duplicating the two-spelling read to say so. const passThroughStatus = error?.code !== 'OBJECT_NOT_FOUND' && typeof error?.status === 'number' && error.status >= 400 && error.status < 600; if (passThroughStatus) {