From af64e72f0e276ebc0055cce362d084a39adea00a Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 24 Aug 2026 13:35:34 +0000 Subject: [PATCH 1/2] fix(rest): classify record-share and analytics refusals at the shared door --- packages/rest/src/error-response.ts | 82 ++++ .../rest-hook-refusal-message-parity.test.ts | 115 +++++- packages/rest/src/rest-server.ts | 159 +++++++- .../rest-share-refusal-classification.test.ts | 364 ++++++++++++++++++ 4 files changed, 709 insertions(+), 11 deletions(-) create mode 100644 packages/rest/src/rest-share-refusal-classification.test.ts diff --git a/packages/rest/src/error-response.ts b/packages/rest/src/error-response.ts index 48df585c42..c71cd9f13c 100644 --- a/packages/rest/src/error-response.ts +++ b/packages/rest/src/error-response.ts @@ -1605,6 +1605,88 @@ function resolveErrorResponse(error: any, object?: string): { status: number; bo return mapDataError(error, object); } +/** + * [#11683 / #11684] The wire answer the `/data` door gives for a refusal the + * PRODUCER classified — or `undefined` when it classified nothing and the + * catching route's own fault terminal is the honest answer. + * + * ## Why this exists as an export rather than as a rule each route re-states + * + * Two route families build their error body by hand and share no branch with + * either door in this file: `/analytics/dataset/query` and the three + * record-share routes, both in `rest-server.ts`. Both re-derived + * classification locally — analytics from an in-line `error.status` + + * `error.code` read, the share family from `message.startsWith(CODE)` over + * five literal prefixes — and both landed a *different* answer from `/data` + * for one refusal. That is the door-disagreement shape #7525/#8016/#11588 keep + * producing whenever a boundary open-codes a read this file already owns; + * {@link sandboxBusinessMessage} was named for exactly that reason one card + * earlier, and this is its status-side counterpart. A route that asks this + * cannot drift, because there is nothing left at the route to drift. + * + * ## The two limbs, and why each is a limb + * + * A refusal is *classified* when the producer said which condition it is. This + * repo has already ruled on two ways of saying so, and this function is their + * union — not a third rule: + * + * 1. **A declared ADR-0112 envelope** — a `status`/`statusCode` + * ({@link declaredHttpStatus}, both spellings, #7525) in the 4xx band + * *and* a non-empty string `code`. **Both halves, deliberately**, which is + * #5352's standing ruling on the analytics arm this sits beside: a 4xx + * with no code would force a hand-built envelope to invent one, and a + * producer shipping half an envelope has a bug that should be found rather + * than papered over here. This function does not reopen that. + * 2. **A sandboxed body's business `throw`** — {@link sandboxBusinessMessage} + * reads non-`undefined`, i.e. the QuickJS body REPORTED something rather + * than CRASHED (#7543). A missing `code` is *not* half an envelope here: + * the producer is a metadata-app author writing `throw new Error('…')`, + * and `classifyDataError`'s unwrap door has answered that with `400` plus + * the verbatim sentence since it existed — pinned end to end by + * `hook-error-format.dogfood.test.ts` and by + * `rest-hook-refusal-message-parity.test.ts` §3. Nothing is invented for + * the code that was not declared either: `thrownCodeFields` answers `{}`, + * which is ADR-0112's own rule. + * + * ## What it deliberately refuses to answer + * + * A **5xx**, declared or resolved. A server fault is not a refusal addressed + * to the caller, so it belongs to the catching route's own terminal — which is + * where the analytics `500 ANALYTICS_QUERY_FAILED` envelope and the share + * family's `SHARE_*_FAILED` codes keep living, message-withholding + * ({@link declaresServerFault}, #5811) and all. Both bands are checked: the + * declared one before resolution, so a declared 5xx never reaches + * {@link resolveErrorResponse}'s heuristics at all, and the resolved one + * after, so an error that looked classified but resolves to + * {@link UNCLASSIFIED_FAULT} or {@link DATA_STORE_FAULT} is handed back rather + * than dressed up as a refusal. + * + * ## What it does NOT decide + * + * The DIALECT. It returns the classification — `{ status, body }`, the same + * flat shape {@link handleRouteError} would send — and the caller re-dresses + * it in whatever envelope that route publishes. The record-share family + * answers the NESTED ADR-0112 D5 envelope (#8111) and must keep doing so; the + * analytics face answers its own flat `{ code, message }`. Deciding the wire + * POSITION here would have moved one of them, and vocabulary and position are + * two separate decisions — ADR-0112's #9232 amendment says so in as many + * words. + */ +export function classifiedRefusalAnswer( + error: any, +): { status: number; body: Record } | undefined { + const declared = declaredHttpStatus(error); + // A declared server fault is not a refusal, whatever else it carries. + if (declared !== undefined && declared >= 500) return undefined; + const declaresEnvelope = + declared !== undefined + && typeof error?.code === 'string' + && error.code.length > 0; + if (!declaresEnvelope && sandboxBusinessMessage(error) === undefined) return undefined; + const resolved = resolveErrorResponse(error); + return resolved.status < 500 ? resolved : undefined; +} + /** * Whether a mapped data-error status represents an *expected* client/lifecycle * outcome (and therefore shouldn't be logged as "[REST] Unhandled error"). diff --git a/packages/rest/src/rest-hook-refusal-message-parity.test.ts b/packages/rest/src/rest-hook-refusal-message-parity.test.ts index e563196b27..20b469a24b 100644 --- a/packages/rest/src/rest-hook-refusal-message-parity.test.ts +++ b/packages/rest/src/rest-hook-refusal-message-parity.test.ts @@ -512,6 +512,28 @@ describe('[#11588] the crash-with-a-declared-4xx divergence this card does NOT c // refusal (where `/data` answers 400) is a separate defect and is NOT touched. // // Predicted before running: §8a RED, §8b RED, §8c GREEN, §8d GREEN. +// +// ── [#11684] …and that "separate defect" is now closed, so §8b is INVERTED ── +// +// The sentence above ("neither arm's STATUS moves") was #11588's fence, not a +// verdict: it recorded that one hook body, one `throw`, produced `400` on +// `/data` and `500` here, and left the disagreement standing. #11684 asked +// which door was right; the answer was already in the repo rather than open, +// and §8b's own comment now carries the evidence. A new arm ①b sits between ① +// and ③ and asks `classifiedRefusalAnswer` — the `/data` door's own +// classification — so: +// ① unchanged — a declared 4xx + `code`; #5352's both-halves rule intact +// ①b a sandboxed body's business `throw`, and the `statusCode` spelling +// ③ unchanged — a declared 5xx, a CRASHED body, anything unclassified +// +// §8b is INVERTED rather than deleted: it recorded a measured defect, and a +// pin that records one is the only evidence the defect existed. §8e is added +// beside it — the door-to-door status pin the card's measurement table was, +// written as an assertion instead of a paragraph. +// +// Predicted before running the #11684 leg, against `4ceae8ab0`: +// §8a GREEN (① untouched) · §8b RED · §8c GREEN (③ untouched) +// §8d GREEN (① untouched) · §8e RED // --------------------------------------------------------------------------- const ANALYTICS_PATH = '/api/v1/analytics/dataset/query'; @@ -560,16 +582,62 @@ describe('[#11588] the analytics dataset face answers in the hook\'s words too', expect(JSON.stringify(res.body)).not.toMatch(WRAPPER_RE); }, 60_000); - it('§8b an UNDECLARED refusal reaches the 500 arm unwrapped — the status is NOT moved', async () => { - // The sub-case the card's repro did not exercise. `/data` answers 400 - // with this sentence and analytics answers 500 with it; that status - // disagreement is a separate defect and is deliberately left standing — - // asserted here so the next reader sees it was measured, not missed. + it('§8b [#11684 — INVERTED] an UNDECLARED refusal answers 400, the same as `/data`', async () => { + // ── This pin used to assert the 500. It is inverted, not deleted ──── + // + // What it asserted, verbatim from #11588: "an UNDECLARED refusal + // reaches the 500 arm unwrapped — the status is NOT moved", with the + // comment "that status disagreement is a separate defect and is + // deliberately left standing". The defect was real and this pin is the + // evidence it was measured rather than missed; deleting it would + // destroy that record, so the assertion is turned around and the + // reasoning kept. + // + // ── WHICH READING WON, AND WHY ────────────────────────────────────── + // + // #11684 named two candidate readings and asked which one the repo had + // already committed to. It had committed to the `/data` door's, and + // the two readings turn out not to compete — they govern different + // questions: + // + // - **ADR-0112's "the producer names the condition"** is a rule about + // the `code`, not the status. Read D1–D9 and its five amendments: + // every one of them rules on the code vocabulary, its closure, and + // the `declaredCode` demote channel. The phrase this reading is + // built on is not in the ADR at all — it is `error-response.ts`'s + // own prose, and there it applies to a DECLARED 5xx that carries no + // code ("a half-declaration is honoured for the half that was + // declared and nothing is invented for the half that was not"). It + // is not contradicted here: this arm invents no code either. + // + // - **The `/data` door's reading** is the one that rules the STATUS, + // and it is not a preference — it is a structural branch with two + // end-to-end pins behind it. `classifyDataError`'s sandbox unwrap + // door answers `declaredHttpStatus(error) ?? 400` with the verbatim + // `.innerMessage` for a body that REPORTED, and the sanitised 500 + // for a body that CRASHED (`isScriptFaultMessage`, #7543). The + // reporting half is pinned end to end by + // `hook-error-format.dogfood.test.ts` ("DELETE blocked by a + // sandboxed hook returns ONLY the business message", 400) and in + // process by §3 above. + // + // So "an undeclared throw is unclassified" was never the repo's rule + // for THIS class. A sandboxed body that reports has classified itself + // structurally — the sandbox boundary is what makes `.innerMessage` + // exist at all — and only a body that CRASHES is unclassified. That + // one still answers 500 here, in §8c's neighbour arm and in §2 of + // `rest-share-refusal-classification.test.ts`. No live pin was found + // on the other side of the question, so the fork clause did not fire. + // + // No code, deliberately: the producer declared none and nothing is + // invented for it, which is `thrownCodeFields`' answer on `/data` and + // ADR-0112's rule. `ANALYTICS_QUERY_FAILED` is ③'s code and ③ is not + // where this refusal belongs any more. const res = await analyticsRefusal(sandboxRefusal('month-end close is in progress')); - expect(res.statusCode).toBe(500); - expect(res.body.code).toBe('ANALYTICS_QUERY_FAILED'); - expect(res.body.error).toBe('month-end close is in progress'); + expect(res.statusCode).toBe(400); + expect(res.body.message).toBe('month-end close is in progress'); + expect(res.body.code).toBeUndefined(); expect(JSON.stringify(res.body)).not.toMatch(WRAPPER_RE); }, 60_000); @@ -595,4 +663,35 @@ describe('[#11588] the analytics dataset face answers in the hook\'s words too', expect(res.statusCode).toBe(409); expect(res.body.message).toBe(text); }, 60_000); + + it('§8e [#11684] the two faces answer one hook `throw` with ONE status', async () => { + // The card's measurement table, as an assertion. Both handlers are the + // REAL ones, driven in process — the analytics face through its + // service provider, the `/data` face through `createData` — because a + // hand-rolled stand-in would only reproduce whichever assumption wrote + // it. Neither status is named: the claim is that they AGREE, so a + // future move on either side reddens here even if someone also updates + // the literal in §8b. + const cases: Array<[string, any]> = [ + ['undeclared refusal', sandboxRefusal('month-end close is in progress')], + ['declared 409 + code', sandboxRefusal('locked', { code: 'RECORD_LOCKED', status: 409 })], + ['`statusCode` spelling', sandboxRefusal('locked', { code: 'RECORD_LOCKED', statusCode: 409 })], + ['declared 5xx', sandboxRefusal('boom', { code: 'SERVICE_UNAVAILABLE', status: 503 })], + ['a CRASHED body (#7543)', sandboxRefusal('TypeError: x is not a function')], + ]; + + for (const [label, error] of cases) { + const analytics = await analyticsRefusal(error); + const rest = setup({ createData: vi.fn().mockRejectedValue(error) }); + const data = await call(rest, 'POST', DATA_COLLECTION, { + params: { object: 'crm_account' }, body: { name: 'x' }, + }); + + expect( + analytics.statusCode, + `${label}: analytics ${analytics.statusCode} ${JSON.stringify(analytics.body)} ` + + `vs /data ${data.statusCode} ${JSON.stringify(data.body)}`, + ).toBe(data.statusCode); + } + }, 60_000); }); diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index ef246c4eaa..51141bd5dc 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -59,6 +59,11 @@ import { refuseRepeatedQueryParams, assertFilterParamSuppliedOnce } from './quer import { refuseUnknownQueryParams } from './query-allowlist.js'; import type { DirectMountedRoute, MountedRouteSource } from './direct-mount.js'; import { RestServerConfig, RestApiConfig, CrudEndpointsConfig, MetadataEndpointsConfig, BatchEndpointsConfig, RouteGenerationConfig } from '@objectstack/spec/api'; +// [#11683] The catalog's own floor for "a required `code` and no more specific +// one" — see its use in `registerSharingEndpoints`, where the nested ADR-0112 +// envelope declares `code` REQUIRED while the flat classification it re-dresses +// legitimately carries none. +import { standardErrorCodeForHttpStatus } from '@objectstack/spec/api'; import { DataProtocol, MetadataProtocol } from '@objectstack/spec/api'; // [#9741] Declared request shapes for the meta-read doors below — imported so // each door's request literal is compiled against the spec contract instead of @@ -194,6 +199,7 @@ import { logError, logWarn } from './log.js'; import { mapDataError, sandboxBusinessMessage, + classifiedRefusalAnswer, sendThrownError, sendDeclaredFault, sendFieldVisibilityFault, @@ -9107,6 +9113,13 @@ export class RestServer { // // Neither arm's STATUS moves. ① keeps answering the declared // 4xx and ③ keeps answering 500; only the sentence changes. + // + // [#11684] …and that last sentence was the defect report + // for the card below: leaving the status alone left an + // UNDECLARED sandbox refusal in ③, answering 500 where the + // `/data` door answers 400 for the identical throw. ①b now + // sits between the two arms and moves exactly that class. + // The sentence this line computes is unchanged. const clientMsg = sandboxBusinessMessage(error) ?? msg; // ── [#5352] ① The ADR-0112 envelope, read FIRST ────────── // A thrown error that already carries `code` + a 4xx @@ -9141,6 +9154,55 @@ export class RestServer { if (envelopeStatus !== undefined && envelopeStatus >= 400 && envelopeStatus < 500 && envelopeCode) { return res.status(envelopeStatus).json({ code: envelopeCode, message: clientMsg.slice(0, 1000) }); } + // ── [#11684] ①b The refusal ① could not read ───────────── + // ① answers a refusal that declared BOTH halves of the + // envelope, spelled `status`. Two classified refusals fell + // past it into ③ and were reported to the caller as server + // faults: + // + // - **An UNDECLARED sandboxed hook refusal.** A hook body + // running `throw new Error('month-end close is in + // progress')` — the most common shape an app author + // writes — declares no `status` and no `code`, so ① + // never opened. Measured: `500 ANALYTICS_QUERY_FAILED` + // here against `400` with the same sentence on + // `POST /data/:object`, i.e. one hook body, one + // `throw`, two statuses decided by whether the caller + // hit a dashboard tile or a list view. + // - **The `statusCode` spelling** (#7525). ①'s read is + // `error.status` alone, and `plugin-approvals`' + // lifecycle hooks — plus `runtime`'s + // `action-execution.ts` and `metadata-protocol` — spell + // it `statusCode`. Same refusal, same declaration, 500. + // + // {@link classifiedRefusalAnswer} is the `/data` door's own + // classification, imported rather than re-derived for the + // same reason {@link sandboxBusinessMessage} above it is: a + // third local opinion at this boundary is precisely how the + // two faces came to disagree. It answers `undefined` for + // everything ③ still owns — a declared 5xx, a crashed body + // (#7543), a driver fault, anything unclassified — so ③'s + // reach is unchanged except for the two shapes named above. + // + // ⛔ This is NOT a widening of ①'s both-halves gate. That + // gate is #5352's ruling for producers that ship half an + // ADR-0112 envelope, it still stands, and the seam encodes + // it. What crosses here without a `code` is the SANDBOX + // limb, where "no code" is not half a declaration: the + // author declared the condition by reporting it, and + // `classifyDataError`'s unwrap door has answered that with + // 400 and the verbatim sentence since it existed + // (`hook-error-format.dogfood.test.ts`). Nothing is + // invented — a refusal that declared no code is answered + // with no code, exactly as `/data` answers it. + const refusal = classifiedRefusalAnswer(error); + if (refusal) { + const { error: refusalText, ...refusalFields } = refusal.body; + return res.status(refusal.status).json({ + ...refusalFields, + message: String(refusalText ?? clientMsg).slice(0, 1000), + }); + } // ── ② … is GONE. The message-sniffing list is retired ──── // [#5367] `/analytics/dataset/query` used to classify six // error families by matching hardcoded substrings of their @@ -9546,7 +9608,73 @@ export class RestServer { // readers are this file's own route mappings plus one // `plugin-approvals` check on an error it threw itself in-process). // It therefore stays exactly as it is; only the response SHAPE moved. + // + // [#11683] …and it stays exactly as it is here too. What moved is that + // the prefix read is no longer the FIRST question, and no longer the + // only one. It is the ADR-0111 idiom for producers that declare + // nothing else, and the census that made it safe to keep is still + // true: every throw site in `plugin-sharing/src/sharing-service.ts` + // (11 of them, re-censused at claim) is a bare `Error` carrying one of + // these prefixes and NO `code`, NO `status`. Backward compatibility is + // therefore not a courtesy — it is the only channel this service has. + // + // What it could never read is a refusal that DID declare itself, and + // two of those reach these three catches today: + // + // - `plugin-sharing`'s own write gate throws + // `{ code: 'FORBIDDEN', status: 403 }` (`sharing-plugin.ts`). + // `FORBIDDEN` is not one of the five, so a refusal that declared + // 403 twice over was answered `500 SHARE_*_FAILED`. + // - A sandboxed hook on the `sys_record_share` write arrives with + // the QuickJS debug wrapper on `.message` and the business text on + // `.innerMessage`. The wrapper IS the prefix, so nothing matched + // and the wrapper was interpolated verbatim into the 500 — #11588's + // leak on a branch #11588 did not reach. + // + // Both are asked FIRST now, through {@link classifiedRefusalAnswer} — + // the `/data` door's own classification, so this family cannot answer + // a refusal differently from every other face that catches it. A + // declaration outranking a message read is the whole point: this + // route's defect was that classification was a property of how the + // sentence happened to start. + // + // ⛔ The DIALECT does not move. #8111 converted these arms onto the + // nested ADR-0112 D5 envelope and the `check:route-envelope` ratchet + // only ticks down, so the classification is re-dressed through + // `respondError` rather than sent by `handleRouteError` (which speaks + // the flat dialect). Vocabulary and position stay two decisions. const respondSharingError = (res: any, error: any): boolean => { + // A refusal the producer classified — answered exactly as `/data` + // answers it. `undefined` for everything else, which falls to the + // prefix idiom below and then to the caller's own 500 arm, both + // unchanged. + const refusal = classifiedRefusalAnswer(error); + if (refusal) { + // `code` is REQUIRED by the nested envelope, and the flat + // classification legitimately carries none for an undeclared + // sandbox refusal (ADR-0112: the producer names the condition, + // so nothing is invented for the half it did not name). The + // catalog's own floor fills the required field — + // `standardErrorCodeForHttpStatus`, whose docblock exists for + // exactly this ("Total by construction: a producer can always + // fill a required `code`") and which is the same derivation + // `resolveThrownHttpError` applies at every other door. This + // is the one place the two dialects genuinely differ: the flat + // body may omit `code`, the nested one may not. + // + // ⚠️ Measured and NOT repaired here: an UNREGISTERED producer + // code is demoted by the shared resolver to a `declaredCode` + // sibling (ADR-0112 #9232), and `sendError`'s `extra` does not + // accept that field — so the author's own spelling is dropped + // on this family while `/data` carries it. Widening the shared + // envelope writer is a `@objectstack/types` change outside + // this card's surface; filed separately. + const code = typeof refusal.body.code === 'string' + ? refusal.body.code as ErrorCode + : standardErrorCodeForHttpStatus(refusal.status); + respondError(res, refusal.status, code, String(refusal.body.error ?? '')); + return true; + } const msg = String(error?.message ?? error ?? ''); const map: Array<[ErrorCode, number]> = [ ['VALIDATION_FAILED', 400], @@ -9566,6 +9694,31 @@ export class RestServer { } return false; }; + /** + * [#11683] The text the three 500 arms may put on the wire. + * + * The arms interpolate the caught error's own message, and that is + * unchanged for every ordinary fault — `sharing-envelope.test.ts` pins + * a plain `Error('boom')` arriving as `boom` and it still does. + * + * The one shape it withholds is a SANDBOX error that reached a 500 at + * all, which after the classification above means one thing: a hook + * body that CRASHED rather than refused (`isScriptFaultMessage`, + * #7543). Its `.message` is the QuickJS debug wrapper and its + * `.innerMessage` is a `TypeError: …` — the wrapper the card measured + * leaking, wrapped around a runtime fault the caller must not read + * either. `/data` answers that case `INTERNAL_ERROR_MESSAGE` through + * {@link UNCLASSIFIED_FAULT}; this says the same sentence, so the leak + * is closed on this family unconditionally rather than only for the + * refusals the classification door catches. + * + * The full wrapper still reaches the operator: every caller logs the + * whole error object immediately above its `respondError`. + */ + const sharingFaultMessage = (error: any): string => + typeof error?.innerMessage === 'string' && error.innerMessage + ? INTERNAL_ERROR_MESSAGE + : String(error?.message ?? error).slice(0, 500); // GET — list shares on a record. [ADR-0111 D5] Management-gated in the // service: invisible record → 404, visible-but-not-manager → 403. @@ -9587,7 +9740,7 @@ export class RestServer { // The 500 arms keep their 500-char cap: an unexpected // fault's message is not a contract, and truncating it // stays a sanitization step — only the position moves. - respondError(res, 500, 'SHARES_LIST_FAILED', String(error?.message ?? error).slice(0, 500)); + respondError(res, 500, 'SHARES_LIST_FAILED', sharingFaultMessage(error)); } }, metadata: { summary: 'List per-record sharing grants', tags: ['sharing'] }, @@ -9621,7 +9774,7 @@ export class RestServer { } catch (error: any) { if (respondSharingError(res, error)) return; logError('[REST] Grant share error:', error); - respondError(res, 500, 'SHARE_GRANT_FAILED', String(error?.message ?? error).slice(0, 500)); + respondError(res, 500, 'SHARE_GRANT_FAILED', sharingFaultMessage(error)); } }, metadata: { summary: 'Grant a per-record share to a principal', tags: ['sharing'] }, @@ -9650,7 +9803,7 @@ export class RestServer { } catch (error: any) { if (respondSharingError(res, error)) return; logError('[REST] Revoke share error:', error); - respondError(res, 500, 'SHARE_REVOKE_FAILED', String(error?.message ?? error).slice(0, 500)); + respondError(res, 500, 'SHARE_REVOKE_FAILED', sharingFaultMessage(error)); } }, metadata: { summary: 'Revoke a per-record share by id', tags: ['sharing'] }, diff --git a/packages/rest/src/rest-share-refusal-classification.test.ts b/packages/rest/src/rest-share-refusal-classification.test.ts new file mode 100644 index 0000000000..5a6bb4b4a5 --- /dev/null +++ b/packages/rest/src/rest-share-refusal-classification.test.ts @@ -0,0 +1,364 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#11683] The record-share family classifies a refusal by what the PRODUCER + * declared, not by how the message happens to start. + * + * ## What was measured + * + * `GET`/`POST /api/v1/data/:object/:id/shares` and + * `DELETE …/shares/:shareId` recovered the verdict with + * `msg.startsWith(CODE)` over five literal prefixes and, on no match, + * interpolated `String(error.message)` into a hand-built 500. Two independent + * defects rode that one read: + * + * 1. **A declared ADR-0112 envelope was ignored entirely.** A producer + * throwing `{ code: 'RECORD_LOCKED', status: 409 }` — the envelope every + * other `/data` face honours through `handleRouteError` — was answered + * `500 SHARE_GRANT_FAILED`, because `RECORD_LOCKED` is not one of the five + * prefixes. Live rather than hypothetical: `plugin-sharing`'s own write + * gate throws `{ code: 'FORBIDDEN', status: 403 }` + * (`sharing-plugin.ts`), and `FORBIDDEN` is not a prefix either. + * 2. **The QuickJS debug wrapper reached the client.** A sandboxed hook + * refusal arrives with `.message` = `hook '' threw: Error: ` + * and `.innerMessage` = the business text. The wrapper IS the prefix, so + * no `startsWith` ever matched and the wrapper was interpolated verbatim + * into the 500 — #11588's defect on a branch #11588 (landed as #11687) did + * not reach. + * + * ## What the repair is, and what it deliberately is NOT + * + * The routes now ask {@link classifiedRefusalAnswer} — the `/data` door's own + * classification — BEFORE the prefix map, and only for a refusal the producer + * classified (a declared 4xx envelope, or a sandboxed body's business + * `throw`). Everything else is untouched: the five-prefix idiom is + * ADR-0111's declaration channel for this service and still runs, and an + * unclassified fault still leaves through this route's own + * `SHARES_LIST_FAILED` / `SHARE_GRANT_FAILED` / `SHARE_REVOKE_FAILED` 500 with + * its own message. `sharing-envelope.test.ts` pins that terminal and is + * expected to stay green character for character. + * + * ## Why the assertions are `status` + nested `error.code`, never `toThrow` + * + * These handlers send; they never throw (`sharing-envelope.test.ts` records + * the same reasoning). A bare `toThrow` would be blind in both directions + * here: the pre-fix route answered 500 without throwing at all. + * + * Predicted before running, against pre-fix `main` (`4ceae8ab0`): + * §1 RED 3 — the declared envelope is answered 500 on all three routes + * §2 RED 3 — the wrapper reaches the client on all three routes + * §3 GREEN 6 — the five-prefix idiom and the plain-500 terminal, unmoved + * §4 RED 1 — the share door and the `/data` door disagree + */ + +import { describe, it, expect, vi } from 'vitest'; +// `.js` on purpose — this package resolves `nodenext`, so an extensionless +// relative import is a `tsc` error (TS2835). +import { RestServer } from './rest-server.js'; +import { handleRouteError } from './error-response.js'; +import { INTERNAL_ERROR_MESSAGE } from '@objectstack/types'; +import { ApiErrorSchema } from '@objectstack/spec/api'; + +const LIST = '/api/v1/data/:object/:id/shares'; +const REVOKE = '/api/v1/data/:object/:id/shares/:shareId'; + +/** The wrapper text that must never reach a client. */ +const WRAPPER_RE = /threw:|hook '/; + +/** + * The shape `runtime/src/sandbox/quickjs-runner.ts` produces: `.message` is the + * ` '' threw: ` debug wrapper, `.innerMessage` the business + * text, `.status` / `.statusCode` the #7867 side-channel. Reproduced here so + * `@objectstack/rest` does not depend on `@objectstack/runtime` to run its own + * tests — the same fixture `rest-hook-refusal-message-parity.test.ts` uses. + */ +function sandboxRefusal( + businessMessage: string, + extra: Record = {}, + hook = 'guard', +) { + const err: any = new Error(`hook '${hook}' threw: Error: ${businessMessage}`); + err.name = 'SandboxError'; + err.innerMessage = businessMessage; + return Object.assign(err, extra); +} + +function mockServer() { + 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 mockRes() { + const res: any = { + statusCode: 200, + json: vi.fn(function (this: any, body: any) { this._body = body; return this; }), + send: vi.fn(function (this: any) { return this; }), + end: vi.fn(function (this: any) { return this; }), + setHeader: vi.fn(function (this: any) { return this; }), + status: vi.fn(function (this: any, code: number) { this.statusCode = code; return this; }), + header: vi.fn(function (this: any) { return this; }), + }; + return res; +} + +type Answer = { status: number; body: any }; + +/** A sharing service whose every verb rejects with `err`. */ +function throwingService(err: unknown) { + return { + listShares: vi.fn().mockRejectedValue(err), + grant: vi.fn().mockRejectedValue(err), + revoke: vi.fn().mockRejectedValue(err), + }; +} + +function boot(service?: any) { + const rest = new RestServer( + mockServer() as any, + { getDiscovery: vi.fn().mockResolvedValue({ version: 'v0', routes: {} }) } as any, + { api: { requireAuth: false } } as any, + undefined, undefined, undefined, undefined, undefined, + undefined, + service === undefined ? undefined : (async () => service) as any, + ); + (rest as any).resolveExecCtx = async () => ({ userId: 'u_admin' }); + rest.registerRoutes(); + + const drive = async (method: string, path: string): Promise => { + const found = (rest as any).getRoutes().find( + (r: any) => r.method === method && r.path === path, + ); + if (!found) throw new Error(`route not registered: ${method} ${path}`); + const res = mockRes(); + await found.handler( + { + method, path, headers: {}, query: {}, body: {}, + params: { object: 'account', id: 'a1', shareId: 'shr_X' }, + } as any, + res, + ); + return { status: res.statusCode, body: res.json.mock.calls.at(-1)?.[0] }; + }; + + return { + list: () => drive('GET', LIST), + grant: () => drive('POST', LIST), + revoke: () => drive('DELETE', REVOKE), + }; +} + +/** All three routes, driven with one rejecting service. */ +async function allThree(err: unknown): Promise> { + const api = boot(throwingService(err)); + return [ + ['GET shares', await api.list()], + ['POST shares', await api.grant()], + ['DELETE shares/:shareId', await api.revoke()], + ]; +} + +/** The wire answer the `/data` door gives for the same error. */ +function throughDataDoor(error: any): Answer { + const res = mockRes(); + handleRouteError(res, error); + return { status: res.statusCode, body: res.json.mock.calls.at(-1)?.[0] }; +} + +/** + * The ADR-0112 D5 pair at the NESTED position, which is where #8111 put this + * family's envelope. Asserted alongside every status claim so a repair that + * moved the dialect back to the flat one cannot pass here. + */ +function expectNestedEnvelope(answer: Answer, status: number, code: string) { + expect( + answer.status, + `expected ${status}, got ${answer.status} with body ${JSON.stringify(answer.body)}`, + ).toBe(status); + expect(answer.body?.error?.code).toBe(code); + expect(typeof answer.body?.error?.message).toBe('string'); + expect(answer.body).not.toHaveProperty('code'); + expect(typeof answer.body?.error).toBe('object'); +} + +// ───────────────────────────────────────────────────────────────────────────── +// §1 A declared ADR-0112 envelope is answered with the status and code it +// declared — the card's problem 1, on all three routes +// ───────────────────────────────────────────────────────────────────────────── + +describe('[#11683] a declared `status`/`code` envelope is honoured, not sniffed for a prefix', () => { + it('all three routes answer 409 RECORD_LOCKED, not 500 SHARE_*_FAILED', async () => { + const refusal = Object.assign( + new Error('This account is locked while month-end close runs.'), + { code: 'RECORD_LOCKED', status: 409 }, + ); + + for (const [label, answer] of await allThree(refusal)) { + expect(answer.status, `${label}: ${JSON.stringify(answer.body)}`).toBe(409); + expectNestedEnvelope(answer, 409, 'RECORD_LOCKED'); + expect(answer.body.error.message).toBe( + 'This account is locked while month-end close runs.', + ); + } + }); + + it('the `statusCode` spelling resolves too (#7525) — one refusal, one answer', async () => { + // `plugin-approvals`' lifecycle hooks declare `statusCode`, not + // `status`; #7525 ruled the boundary reads both. This family read + // neither. + const refusal = Object.assign(new Error('A pending approval locks this record.'), { + code: 'RECORD_LOCKED', statusCode: 409, + }); + + for (const [label, answer] of await allThree(refusal)) { + expect(answer.status, `${label}: ${JSON.stringify(answer.body)}`).toBe(409); + expect(answer.body.error.code).toBe('RECORD_LOCKED'); + } + }); + + it("plugin-sharing's OWN write-gate refusal — the live in-repo producer", async () => { + // `sharing-plugin.ts` throws exactly this for a fail-closed row denial. + // `FORBIDDEN` is not one of the five prefixes, so the route answered + // 500 for a refusal that had declared 403 twice over (code AND status). + const refusal: any = new Error('FORBIDDEN: insufficient privileges to delete account a1'); + refusal.code = 'FORBIDDEN'; + refusal.status = 403; + + for (const [label, answer] of await allThree(refusal)) { + expect(answer.status, `${label}: ${JSON.stringify(answer.body)}`).toBe(403); + expect(answer.body.error.code).toBe('FORBIDDEN'); + } + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// §2 No sandbox debug wrapper reaches the client — the card's problem 2 +// ───────────────────────────────────────────────────────────────────────────── + +describe('[#11683] a sandboxed hook refusal surfaces its business text, never the wrapper', () => { + it('all three routes answer the business sentence, wrapper-free', async () => { + const business = 'Sharing is frozen until the quarterly access review closes.'; + + for (const [label, answer] of await allThree(sandboxRefusal(business))) { + expect( + JSON.stringify(answer.body), + `${label} leaked the wrapper: ${JSON.stringify(answer.body)}`, + ).not.toMatch(WRAPPER_RE); + expect(answer.body.error.message).toBe(business); + } + }); + + it('a sandbox refusal that ALSO declares an envelope keeps both halves', async () => { + const business = 'Only the record owner may re-share this account.'; + const refusal = sandboxRefusal(business, { code: 'FORBIDDEN', status: 403 }); + + for (const [label, answer] of await allThree(refusal)) { + expect(answer.status, `${label}: ${JSON.stringify(answer.body)}`).toBe(403); + expect(answer.body.error.code).toBe('FORBIDDEN'); + expect(answer.body.error.message).toBe(business); + expect(JSON.stringify(answer.body)).not.toMatch(WRAPPER_RE); + } + }); + + it('⭐ POSITIVE CONTROL — a body that CRASHED is NOT served as a refusal (#7543)', async () => { + // `isScriptFaultMessage` declines a native error name, so this stays a + // server fault and keeps this route's own 500 code. The control that + // proves §2 is a READ of `.innerMessage` and not a pattern-strip. + const crash = sandboxRefusal('TypeError: ctx.input.title.trim is not a function'); + + const answers = await allThree(crash); + const codes = answers.map(([, a]) => a.body?.error?.code); + expect(answers.map(([, a]) => a.status)).toEqual([500, 500, 500]); + expect(codes).toEqual(['SHARES_LIST_FAILED', 'SHARE_GRANT_FAILED', 'SHARE_REVOKE_FAILED']); + // …and the wrapper does not ride out on the 500 either. This is the + // one arm the classification door cannot reach, so it is the arm the + // leak would survive on: `sharingFaultMessage` withholds a sandbox + // error's own text here, the same answer `/data` gives a crash + // through `UNCLASSIFIED_FAULT`. The whole error still reaches the + // operator through each route's `logError` line. + for (const [label, a] of answers) { + expect(a.body.error.message, label).toBe(INTERNAL_ERROR_MESSAGE); + expect(JSON.stringify(a.body), label).not.toMatch(WRAPPER_RE); + } + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// §3 What must NOT move — the ADR-0111 prefix idiom and the 500 terminal +// ───────────────────────────────────────────────────────────────────────────── + +describe('[#11683] the `CODE: message` string convention and the 500 terminal are untouched', () => { + const PREFIXED: Array<[string, number, string]> = [ + ['VALIDATION_FAILED', 400, 'recipientId is required'], + ['PERMISSION_DENIED', 403, 'you do not hold canManageShares on this record'], + ['NOT_FOUND', 404, 'record account/a1 does not exist'], + ['CONFLICT', 409, "share shr_X is materialised by source 'rule'"], + ['SHARING_NOT_ENABLED', 422, "'account' bypasses record sharing"], + ]; + + it.each(PREFIXED)('`%s:` still maps to %i with the prefix stripped', async (code, status, tail) => { + // Backward compatibility is REQUIRED: every producer in + // `plugin-sharing/src/sharing-service.ts` still throws a bare `Error` + // with this prefix and declares no envelope at all (censused at claim + // — 11 throw sites, zero declared `code`/`status`). + const api = boot(throwingService(new Error(`${code}: ${tail}`))); + const answer = await api.grant(); + expectNestedEnvelope(answer, status, code); + expect(answer.body.error.message).toBe(tail); + }); + + it('an unclassified fault still leaves through this route\'s own 500, verbatim', async () => { + const answers = await allThree(new Error('connection reset')); + expect(answers.map(([, a]) => a.status)).toEqual([500, 500, 500]); + expect(answers.map(([, a]) => a.body.error.code)).toEqual([ + 'SHARES_LIST_FAILED', 'SHARE_GRANT_FAILED', 'SHARE_REVOKE_FAILED', + ]); + for (const [, a] of answers) expect(a.body.error.message).toBe('connection reset'); + }); + + it('every answer still parses as the declared ApiErrorSchema', async () => { + const bodies = [ + ...(await allThree(Object.assign(new Error('locked'), { code: 'RECORD_LOCKED', status: 409 }))), + ...(await allThree(sandboxRefusal('frozen'))), + ...(await allThree(new Error('NOT_FOUND: nope'))), + ...(await allThree(new Error('boom'))), + ]; + for (const [label, answer] of bodies) { + const parsed = ApiErrorSchema.safeParse(answer.body.error); + expect( + parsed.success, + `${label}: ${JSON.stringify(answer.body)} → ${ + parsed.success ? '' : JSON.stringify(parsed.error.issues)}`, + ).toBe(true); + } + }); +}); + +// ───────────────────────────────────────────────────────────────────────────── +// §4 Door-to-door — the share family and the `/data` family agree on the STATUS +// ───────────────────────────────────────────────────────────────────────────── + +describe('[#11683 / #11684] the share door and the `/data` door answer one refusal alike', () => { + it('every classified refusal gets the same status at both doors', async () => { + const cases: Array<[string, any]> = [ + ['declared 409 + code', Object.assign(new Error('locked'), { code: 'RECORD_LOCKED', status: 409 })], + ['declared 403 + code', Object.assign(new Error('nope'), { code: 'FORBIDDEN', status: 403 })], + ['statusCode 409 + code', Object.assign(new Error('locked'), { code: 'RECORD_LOCKED', statusCode: 409 })], + ['undeclared sandbox refusal', sandboxRefusal('month-end close is in progress')], + ['sandbox refusal + declared 403', sandboxRefusal('nope', { code: 'FORBIDDEN', status: 403 })], + ]; + + for (const [label, error] of cases) { + const dataDoor = throughDataDoor(error); + for (const [route, answer] of await allThree(error)) { + expect( + answer.status, + `${label} @ ${route}: share door ${answer.status} vs /data door ${dataDoor.status}`, + ).toBe(dataDoor.status); + } + } + }); +}); From b468109e1e2a4bebd38caf4ec185f6b0218d71f4 Mon Sep 17 00:00:00 2001 From: Claude Date: Mon, 24 Aug 2026 14:12:53 +0000 Subject: [PATCH 2/2] fix(rest): classify record-share and analytics refusals at the shared /data door MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Fixes #11683 and #11684. Adds `classifiedRefusalAnswer` in error-response.ts — the /data door's classification, without the dialect — and routes the three record-share catches and the analytics dataset catch through it. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_019siH5jDmk5hrayvfyojUqR --- .changeset/olive-donkeys-repeat.md | 45 +++++++++++++++++++ .../rest-hook-refusal-message-parity.test.ts | 41 ++++++++++++++++- 2 files changed, 85 insertions(+), 1 deletion(-) create mode 100644 .changeset/olive-donkeys-repeat.md diff --git a/.changeset/olive-donkeys-repeat.md b/.changeset/olive-donkeys-repeat.md new file mode 100644 index 0000000000..c9949bae28 --- /dev/null +++ b/.changeset/olive-donkeys-repeat.md @@ -0,0 +1,45 @@ +--- +'@objectstack/rest': patch +--- + +fix(rest): classify a refusal by what the producer declared, not by how its message starts + +The three record-share routes and `POST /api/v1/analytics/dataset/query` each +built their error answer by hand and shared no branch with the door every +`/data` face reports through, so one refusal got a different wire answer +depending on which route caught it. Both now ask that door first, through a +single new seam (`classifiedRefusalAnswer`), for a refusal the **producer +classified** — a declared 4xx `status`/`statusCode` **plus** a `code`, or a +sandboxed hook body's business `throw`. Everything else is untouched. + +**Per route, old answer → new answer.** Check your error handling if you branch +on any of these. + +`GET /api/v1/data/:object/:id/shares`, `POST` the same path, and +`DELETE /api/v1/data/:object/:id/shares/:shareId`: + +| the thrown refusal | was | is now | +| :--- | :--- | :--- | +| `{ code: 'RECORD_LOCKED', status: 409 }` (any code outside the five prefixes) | `500` `SHARES_LIST_FAILED` / `SHARE_GRANT_FAILED` / `SHARE_REVOKE_FAILED` | `409` `RECORD_LOCKED` | +| `{ code: 'FORBIDDEN', status: 403 }` — `plugin-sharing`'s own write gate | `500` `SHARE_*_FAILED` | `403` `FORBIDDEN` | +| the same declared as `statusCode` rather than `status` | `500` `SHARE_*_FAILED` | the declared status + code | +| a sandboxed hook refusal, no status declared | `500` `SHARE_*_FAILED`, message = the QuickJS wrapper `hook '' threw: Error: ` | `400` `VALIDATION_ERROR`, message = the hook's own sentence | +| a sandboxed hook body that CRASHED | `500` `SHARE_*_FAILED`, message = the wrapper around `TypeError: …` | `500` `SHARE_*_FAILED`, message = `Internal server error` | +| `VALIDATION_FAILED:` / `PERMISSION_DENIED:` / `NOT_FOUND:` / `CONFLICT:` / `SHARING_NOT_ENABLED:` prefixed messages | 400 / 403 / 404 / 409 / 422 with the prefix stripped | **unchanged** | +| anything else | `500` `SHARE_*_FAILED` with its own message | **unchanged** | + +`POST /api/v1/analytics/dataset/query`: + +| the thrown refusal | was | is now | +| :--- | :--- | :--- | +| a sandboxed hook refusal, no status and no code declared | `500` `{ code: 'ANALYTICS_QUERY_FAILED', error: }` | `400` `{ message: }` — the same status `POST /api/v1/data/:object` answers for the identical throw, and no code, because the producer declared none | +| a declared 4xx + code spelled `statusCode` rather than `status` | `500` `ANALYTICS_QUERY_FAILED` | the declared status + code | +| a declared 4xx + code spelled `status` | the declared status + code | **unchanged** | +| a declared 5xx, a crashed hook body, a driver fault, anything unclassified | `500` `ANALYTICS_QUERY_FAILED` | **unchanged** | + +The nested `{ success: false, error: { code, message } }` envelope the sharing +family answers is unchanged — only the status and code inside it move. The +`VALIDATION_ERROR` on the sandbox row is the catalog's declared floor for a +required `code` the producer did not name (`standardErrorCodeForHttpStatus`); +the flat `/data` body omits `code` there instead, because its `code` is +optional and ADR-0112 invents nothing. diff --git a/packages/rest/src/rest-hook-refusal-message-parity.test.ts b/packages/rest/src/rest-hook-refusal-message-parity.test.ts index 20b469a24b..4ed258779c 100644 --- a/packages/rest/src/rest-hook-refusal-message-parity.test.ts +++ b/packages/rest/src/rest-hook-refusal-message-parity.test.ts @@ -672,11 +672,12 @@ describe('[#11588] the analytics dataset face answers in the hook\'s words too', // it. Neither status is named: the claim is that they AGREE, so a // future move on either side reddens here even if someone also updates // the literal in §8b. + // ⚠️ The CLIENT band only, and that bound is a finding rather than a + // convenience — see §8f immediately below, which pins what it excludes. const cases: Array<[string, any]> = [ ['undeclared refusal', sandboxRefusal('month-end close is in progress')], ['declared 409 + code', sandboxRefusal('locked', { code: 'RECORD_LOCKED', status: 409 })], ['`statusCode` spelling', sandboxRefusal('locked', { code: 'RECORD_LOCKED', statusCode: 409 })], - ['declared 5xx', sandboxRefusal('boom', { code: 'SERVICE_UNAVAILABLE', status: 503 })], ['a CRASHED body (#7543)', sandboxRefusal('TypeError: x is not a function')], ]; @@ -694,4 +695,42 @@ describe('[#11588] the analytics dataset face answers in the hook\'s words too', ).toBe(data.statusCode); } }, 60_000); + + it('§8f MEASURED AND NOT REPAIRED — the two faces still disagree in the 5xx band', async () => { + // Found by §8e's first draft, which included this case and reddened + // WITH the fix in place. Recorded rather than quietly dropped: a bound + // on a parity claim that nobody can see is how the next reader + // concludes the two faces agree everywhere. + // + // A producer declaring `503` + a code is answered `503 + // SERVICE_UNAVAILABLE` on `/data` (the #5582 passthrough: keep the + // status, keep the code, drop the prose) and `500 + // ANALYTICS_QUERY_FAILED` here — 502/503 are `isExpectedDataStatus` + // lifecycle outcomes that proxies and retry policies read differently + // from a 500, so this is the same class of loss #5582 closed one door + // over. + // + // NOT repaired by #11684, deliberately. ③'s "a declared 5xx keeps + // going through the `ANALYTICS_QUERY_FAILED` envelope" is #5352's + // ruling, re-argued by #5367 and #5811 and load-bearing for the + // read-scope refusals — moving it is a contract call on a shipped + // route that neither folded card asked for. `classifiedRefusalAnswer` + // hands a declared 5xx straight back for exactly this reason. Filed as + // its own card. + const error = sandboxRefusal('boom', { code: 'SERVICE_UNAVAILABLE', status: 503 }); + + const analytics = await analyticsRefusal(error); + const rest = setup({ createData: vi.fn().mockRejectedValue(error) }); + const data = await call(rest, 'POST', DATA_COLLECTION, { + params: { object: 'crm_account' }, body: { name: 'x' }, + }); + + expect(data.statusCode).toBe(503); + expect(data.body.code).toBe('SERVICE_UNAVAILABLE'); + expect(analytics.statusCode).toBe(500); + expect(analytics.body.code).toBe('ANALYTICS_QUERY_FAILED'); + // Both still withhold the prose — that half never disagreed (§8c). + expect(analytics.body.error).toBe(INTERNAL_ERROR_MESSAGE); + expect(data.body.error).toBe(INTERNAL_ERROR_MESSAGE); + }, 60_000); });