From 169562fdcf590cc4dde50a400b027c5bdcdca68e Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 11 Aug 2026 09:30:01 +0000 Subject: [PATCH 1/2] fix(rest): a crashing hook body answers the sanitised fault envelope, not a raw `TypeError` at 400 (#7543) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `POST /api/v1/data/showcase_task` with `{"title": 12345}` answered `400 {"error":"TypeError: not a function","object":"showcase_task"}` — a JS runtime error as the client-facing message, in a body with no `code`. The seam, since the card asked for it to be located rather than assumed: its investigation note says the shape "matches no branch of `mapDataError`". It matches two — the sandbox-unwrap branches, the only ones in the file that emit `{error, object}` with no `code` at 400. They exist for a hook body that runs `throw new Error('业务消息')`, whose message IS the remedy and is answered verbatim. A body that CRASHES arrives as a thrown error too, so it took the same branch. The note's "the throw is upstream or downstream of `validateOne`" is answered too: upstream — the hook is `beforeInsert`, so it throws before the validator's safe `String(value)` ever sees the record. Both branches now separate a body that reported something from a body that faulted, by the thrown error's constructor name (the sandbox stringifies a throw as `: `, so the name is structural evidence, not a keyword heuristic). A crash answers the same sanitised `500 INTERNAL_ERROR` the mapper's terminal branch gives — not new policy: that branch's docblock (#5489) names this exact case. Both doors are guarded, since they emit byte-identical bodies and guarding one would make the envelope depend on whether the `SandboxError` instance survived a rethrow. Unchanged: a deliberate refusal still reaches the caller verbatim at 400 with no `code`. The operator still gets the full text — 500 is outside `isExpectedDataStatus`, so `handleRouteError` logs it. `rest.test.ts`'s "keeps non-default error names … genuine script bugs stay identifiable" is REVERSED in place rather than deleted: it pinned the behaviour this card calls a defect, and never asked identifiable to whom. Showcase: `NormalizeTaskTitleHook` guarded its trim with truthiness, so `12345` passed the guard and had no `.trim`. Now `typeof … === 'string'`. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01UYt5skZ9r78Hnnza2b1jnK --- .changeset/hook-script-fault-envelope.md | 61 +++ examples/app-showcase/src/data/hooks/index.ts | 9 +- .../rest-hook-script-fault-envelope.test.ts | 432 ++++++++++++++++++ packages/rest/src/rest-server.ts | 70 ++- packages/rest/src/rest.test.ts | 16 +- 5 files changed, 581 insertions(+), 7 deletions(-) create mode 100644 .changeset/hook-script-fault-envelope.md create mode 100644 packages/rest/src/rest-hook-script-fault-envelope.test.ts diff --git a/.changeset/hook-script-fault-envelope.md b/.changeset/hook-script-fault-envelope.md new file mode 100644 index 0000000000..b0d252ab9c --- /dev/null +++ b/.changeset/hook-script-fault-envelope.md @@ -0,0 +1,61 @@ +--- +"@objectstack/rest": patch +"@objectstack/example-showcase": patch +--- + +fix(rest): a crashing hook body answers the sanitised fault envelope, not a raw `TypeError` at 400 (#7543) + +`POST /api/v1/data/showcase_task` with `{"title": 12345}` answered + +``` +400 { "error": "TypeError: not a function", "object": "showcase_task" } +``` + +— a JS runtime error as the client-facing message, in a body with no `code` at +all. Two contract breaks in one response: an internal fault echoed verbatim to a +caller, and an error body outside the ledgered envelope, so a client keying on +`code` got nothing. + +**The seam.** `mapDataError` has two sandbox-unwrap branches, and they are the +only ones in the file that emit `{ error, object }` with no `code` at 400. They +exist for one shape: a hook or action body that runs +`throw new Error('删除被阻断：仍有未结清的发票')` — an author writing a business +rule whose message *is* the remedy, which is answered verbatim at 400 and +deliberately without a `code`. A body that instead **crashes** arrives as a +thrown error too, so it took the same branch and its `TypeError` went out as if +it were that author's message. + +**The fix.** Both branches now separate a body that *reported* something from a +body that *faulted*, by the thrown error's constructor name — the sandbox +stringifies a throw as `: `, so a leading `TypeError:`, +`ReferenceError:`, `RangeError:`, `SyntaxError:`, `URIError:`, `EvalError:`, +`InternalError:` or `AggregateError:` is structural evidence of a crash rather +than a keyword heuristic over prose. A crash answers the same sanitised +`500 INTERNAL_ERROR` the mapper's terminal branch already gives — which is not +new policy: that branch's own contract (#5489) names this exact case ("a plain +handler bug (`TypeError: x is not a function`) … server faults that a caller +cannot fix and a caller SHOULD retry"). The unwraps simply sat above it and +intercepted the crash first. + +Both doors are guarded, not one. The `innerMessage` branch and the raw-message +regex fallback produce byte-identical bodies, so classifying in only one would +make the envelope depend on whether the `SandboxError` instance survived a +rethrow. + +**Unchanged:** a deliberate refusal still reaches the caller verbatim at 400 +with no `code`. The fix changes *which* errors take that branch, not what it +emits. A body that expresses a business rule as `throw new RangeError('…')` is +now sanitised — an accepted cost, since that is not the documented authoring +style and the fail-safe direction is the one that does not ship runtime faults to +clients. The operator still gets the full text: 500 is outside +`isExpectedDataStatus`, so `handleRouteError` logs `[REST] Unhandled error` with +the whole error. + +**Showcase.** `NormalizeTaskTitleHook` guarded its trim with truthiness +(`if (ctx.input.title)`), so the number `12345` passed the guard and had no +`.trim`. It now checks `typeof … === 'string'`. That is the actual cause of the +reported repro, and with it fixed the request **succeeds** rather than erroring: +`record-validator` coerces a `text` value with `String(value)`, so a number in a +text field breaks no declared contract. These hook bodies are read as +documentation, so the type-safe shape is the one to show — a hook must not assume +a field's runtime type just because its metadata declares one. diff --git a/examples/app-showcase/src/data/hooks/index.ts b/examples/app-showcase/src/data/hooks/index.ts index 7905a07e34..c809fe28bd 100644 --- a/examples/app-showcase/src/data/hooks/index.ts +++ b/examples/app-showcase/src/data/hooks/index.ts @@ -30,8 +30,15 @@ export const NormalizeTaskTitleHook = { object: 'showcase_task', events: ['beforeInsert', 'beforeUpdate'] as LifecycleEvent[], body: { + // [#7543] The guard is `typeof … === 'string'`, not truthiness. A JSON body + // may put a number in a `text` field — `{"title": 12345}` — which is truthy, + // has no `.trim`, and made this body throw `TypeError: not a function` on a + // write the platform otherwise ACCEPTS (`record-validator` coerces a `text` + // value with `String(value)`). These bodies are read as documentation, so + // the type-safe shape is the one to show: a hook must not assume a field's + // runtime type just because its metadata declares one. language: 'js' as const, - source: "if (ctx.input.title) ctx.input.title = ctx.input.title.trim();", + source: "if (typeof ctx.input.title === 'string') ctx.input.title = ctx.input.title.trim();", }, priority: 50, onError: 'abort' as const, diff --git a/packages/rest/src/rest-hook-script-fault-envelope.test.ts b/packages/rest/src/rest-hook-script-fault-envelope.test.ts new file mode 100644 index 0000000000..7f4c70b94b --- /dev/null +++ b/packages/rest/src/rest-hook-script-fault-envelope.test.ts @@ -0,0 +1,432 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// [#7543] A sandboxed hook body that CRASHES answers the sanitised server-fault +// envelope, not a 400 carrying its raw `TypeError` as the client-facing message. +// +// --------------------------------------------------------------------------- +// The seam, because the card asked for it to be LOCATED rather than assumed. +// +// The card's investigation note is a negative result: "the observed body — +// status 400, raw message, an `object` key, no `code` — matches NO branch of +// `mapDataError`". That note is wrong, and finding out how it was wrong is the +// whole fix. It missed the two SANDBOX-UNWRAP branches, which are the only ones +// in the file that emit `{ error, object }` with no `code` at 400 — exactly the +// reported shape. Measured on `main` @ `4ed4160` (i.e. AFTER PR #7575 landed +// `declaredHttpStatus`, which does not touch these branches): +// +// mapDataError(SandboxError( ← rest-server.ts +// "hook 'showcase_normalize_task_title' threw: TypeError: not a function", +// innerMessage: 'TypeError: not a function'), 'showcase_task') +// => {"status":400,"body":{"error":"TypeError: not a function", +// "object":"showcase_task"}} +// +// — byte-for-byte the reported response. The full chain, end to end: +// +// 1. `examples/app-showcase/src/data/hooks/index.ts` `NormalizeTaskTitleHook` +// ran `if (ctx.input.title) ctx.input.title = ctx.input.title.trim();`. +// The number `12345` is truthy and has no `.trim` ⇒ QuickJS throws. +// 2. `runtime/src/sandbox/quickjs-runner.ts` wraps it as +// `SandboxError("hook '' threw: TypeError: not a function", +// userFacingMessage(...))`. `userFacingMessage` strips only a +// leading `Error: `, so `innerMessage` KEEPS `TypeError: not a function`. +// 3. `mapDataError`'s `innerMessage` branch answers 400 with that string and +// deliberately no `code`. +// +// This also answers the card's "so the throw is upstream or downstream of +// `validateOne`". It is UPSTREAM: the hook is `beforeInsert`, so it throws +// before the validator ever sees the record — which is why `validateOne`'s safe +// `String(value)` never got the chance to handle it. +// +// --------------------------------------------------------------------------- +// Why a 500, and why this file does NOT assert `fields[]` on the repro body. +// +// The card's accept bar says `{"title": 12345}` must JOIN its neighbours — +// `{}` → `400 VALIDATION_FAILED` with `fields[]`. It cannot, and asserting that +// it does would pin a false statement. `record-validator.ts:503-504` accepts a +// number in a `text` field: +// +// if (t === 'text' || …) { const s = typeof value === 'string' ? value : String(value); … } +// +// — the value is COERCED, every length/format check runs against `"12345"`, and +// the branch returns `null`. The shape guard above it (`invalid_value_shape`) +// only refuses filter-operator objects like `{ $in: [...] }`, not scalars. So +// `{"title": 12345}` is a VALID request by the platform's own declared +// contract; there is no offending field to name, and `VALIDATION_FAILED` would +// be a lie about the caller. +// +// What was actually broken is the other two things the card names, and both are +// fixed here: the raw runtime fault no longer reaches the wire, and the body now +// carries a `code` (`INTERNAL_ERROR`) so a client keying on the ledgered +// envelope gets one. §4 pins the family side by side so the three bodies are +// guarded together, each asserted for what it truthfully is. +// +// The 500 is not a new policy either — {@link UNCLASSIFIED_FAULT}'s own docblock +// (#5489) already ruled on this exact case one door down: "or a plain handler +// bug (`TypeError: x is not a function`) … server faults that a caller cannot +// fix and a caller SHOULD retry". The sandbox unwraps sit ABOVE that branch and +// were intercepting the crash before it could reach the answer the file had +// already settled on. +// +// --------------------------------------------------------------------------- +// Mutation table. All 18 cases were 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, and where the measurement +// showed an assertion was not pulling its weight the TEST was strengthened +// rather than the prediction rewritten (see E). +// +// A · BASELINE — the file run against unmodified `main` (`4ed4160`), i.e. the +// real defect rather than a synthetic mutation. +// §1 predicted RED, measured 6/6 red — the reported body and every other +// native error name answer `400 {error:,object}`. +// §2 predicted RED, measured 1/3 red — prediction said 3/3 and was WRONG +// about the mechanism: the two cross-door AGREEMENT cases are +// direction-insensitive on baseline BY CONSTRUCTION, because both doors +// are broken in the SAME way and so still agree. Mutation D is what +// moves them, and that is exactly the partial fix they exist to catch. +// §3 predicted GREEN, measured 4/4 green — the business-refusal branch is +// what the fix must NOT move, and unmodified `main` already passes it. +// Green on baseline is the point: this is the regression half, moved by +// mutations B and E. +// §4 predicted RED, measured 3/5 red — the three crash rows. The two +// control rows pass on baseline; they never entered a sandbox branch. +// Mutation F moves them. +// Total: 10 of 18 red. +// +// B · `isScriptFaultMessage` returns `true` for EVERY message (the "sanitise +// every hook throw" overreach the fix must not become). +// predicted RED for §3 + §4's distinctness row, measured 6 red — §3's four +// cases, §4's distinctness row, and §2's refusal-agreement case (the +// prediction missed that §2 asserts on a refusal too). +// +// C · `isScriptFaultMessage` returns `false` for every message (the fix deleted, +// call sites left in place). +// predicted RED for the same set as baseline A, measured 10 red — the same +// 10, confirming the two call sites are the only carriers of the fix. +// +// D · The `innerMessage` door's guard removed, the regex door's KEPT — the +// partial fix that would make the envelope depend on whether the +// SandboxError instance happened to survive a rethrow. +// predicted RED for §1 + §4's crash rows + §2's byte-equality case, +// measured exactly that: 10 red, with §2's lost-instance and +// refusal-agreement cases GREEN. This is the mutation that justifies +// guarding BOTH doors rather than one. +// +// E · The regex anchor `^` dropped from `NATIVE_ERROR_NAME_RE`. +// predicted RED for §3's "merely mentions a native error name" case, +// measured 0 red — prediction WRONG, and the finding is the useful part: +// the `(?::|$)` limb already refuses prose like "produced a TypeError in +// your template", so the original single-string assertion did not exercise +// the anchor at all. The case was strengthened with a message that quotes a +// native name WITH its colon mid-sentence ("rejected with TypeError: check +// the template"), which only `^` refuses. Re-measured: 1 red. +// +// F · The `VALIDATION_FAILED` branch's `fields` limb hard-coded to `[]` — the +// mutation that covers §4's two control rows, which no fix-targeting +// mutation can move. +// predicted RED for exactly those two, measured 2 red. +// --------------------------------------------------------------------------- + +import { describe, it, expect } from 'vitest'; +import { INTERNAL_ERROR_MESSAGE } from '@objectstack/types'; +import { mapDataError } from './rest-server.js'; + +// --------------------------------------------------------------------------- +// Producer fixtures — COPIED from the shipping producers, not invented. +// +// `@objectstack/rest` must not depend on `@objectstack/runtime` or +// `@objectstack/objectql` to run its own tests, so the two thrown shapes are +// reproduced here. What makes them valid fixtures is that each is built the way +// its real producer builds it. +// --------------------------------------------------------------------------- + +/** + * `runtime/src/sandbox/quickjs-runner.ts`'s `SandboxError` — the wrapper on + * `.message` for server logs, the business message on `.innerMessage`. + * `innerMessage` is `userFacingMessage(formatErr(err))`, which strips a leading + * `Error: ` and nothing else. + */ +class SandboxError extends Error { + readonly innerMessage?: string; + constructor(message: string, innerMessage?: string) { + super(message); + this.name = 'SandboxError'; + this.innerMessage = innerMessage; + } +} + +/** What the sandbox throws when a hook BODY crashes. */ +function hookCrash(hook: string, thrown: string) { + return new SandboxError(`hook '${hook}' threw: ${thrown}`, thrown); +} + +/** + * The same crash after the SandboxError instance was lost crossing a + * rethrow/serialization boundary — only the wrapper text survives. + */ +function hookCrashInstanceLost(hook: string, thrown: string) { + return new Error(`hook '${hook}' threw: ${thrown}`); +} + +/** What the sandbox throws when a hook body DELIBERATELY refuses a write. */ +function hookRefusal(hook: string, businessMessage: string) { + // The real path: the author writes `throw new Error('…')`, `formatErr` + // renders `Error: …`, and `userFacingMessage` strips that prefix. + return new SandboxError(`hook '${hook}' threw: Error: ${businessMessage}`, businessMessage); +} + +/** `objectql/src/validation/record-validator.ts`'s `ValidationError`. */ +function validationError(fields: Array>) { + const err: any = new Error( + fields.map((f: any) => (f.message?.trim() ? f.message : `${f.field} (${f.code})`)).join('; '), + ); + err.name = 'ValidationError'; + err.code = 'VALIDATION_FAILED'; + err.fields = fields; + return err; +} + +/** The exact hook the report tripped, with the exact thrown text. */ +const REPORTED = () => hookCrash('showcase_normalize_task_title', 'TypeError: not a function'); + +// --------------------------------------------------------------------------- +// §1 The reported defect — the `innerMessage` door +// --------------------------------------------------------------------------- + +describe('[#7543] a crashing hook body does not put its runtime fault on the wire', () => { + it('the reported request no longer answers `400 "TypeError: not a function"`', () => { + const r = mapDataError(REPORTED(), 'showcase_task'); + + // The measured defect, pinned as a NEGATIVE so a partial fix cannot pass. + expect(r.body.error).not.toBe('TypeError: not a function'); + expect(String(r.body.error)).not.toContain('TypeError'); + expect(r.status).not.toBe(400); + + expect(r.status).toBe(500); + expect(r.body.error).toBe(INTERNAL_ERROR_MESSAGE); + }); + + it('the body joins the ledgered envelope — it carries a `code`', () => { + // The card's second contract break: a client keying on `code` got + // nothing at all from this response. + const r = mapDataError(REPORTED(), 'showcase_task'); + expect(r.body.code).toBe('INTERNAL_ERROR'); + expect(r.body).toHaveProperty('code'); + }); + + it('no part of the internal fault survives — not the name, not the object key', () => { + const r = mapDataError(REPORTED(), 'showcase_task'); + // `UNCLASSIFIED_FAULT`'s envelope is deliberately minimal, and this + // branch emits it byte-identically rather than a near-duplicate with an + // `object` key bolted on: one wire answer for "server fault, no + // attribution to the caller". + expect(r.body).toEqual({ error: INTERNAL_ERROR_MESSAGE, code: 'INTERNAL_ERROR' }); + expect(r.body).not.toHaveProperty('object'); + }); + + it('every native error constructor name is a crash, not a refusal', () => { + // Matched by CONSTRUCTOR NAME rather than by phrasing — the sandbox + // stringifies a thrown error as `: `, so the name is + // structural evidence, not a keyword heuristic over prose. + const nativeCrashes = [ + 'TypeError: not a function', + "TypeError: cannot read property 'x' of undefined", + 'ReferenceError: foo is not defined', + 'RangeError: Maximum call stack size exceeded', + 'SyntaxError: unexpected token', + 'URIError: URI malformed', + 'EvalError: eval is not permitted', + 'InternalError: stack overflow', + 'AggregateError: All promises were rejected', + ]; + for (const thrown of nativeCrashes) { + const r = mapDataError(hookCrash('h', thrown), 'showcase_task'); + expect(r.status, thrown).toBe(500); + expect(r.body.code, thrown).toBe('INTERNAL_ERROR'); + expect(String(r.body.error), thrown).toBe(INTERNAL_ERROR_MESSAGE); + } + }); + + it('a bare constructor name with no message is still a crash', () => { + // QuickJS can stringify a thrown error with an empty message. The `(?::|$)` + // limb is what keeps that from falling through to the verbatim 400. + const r = mapDataError(hookCrash('h', 'TypeError'), 'showcase_task'); + expect(r.status).toBe(500); + expect(r.body.code).toBe('INTERNAL_ERROR'); + }); + + it('an ACTION body crashes the same way a hook body does', () => { + // The unwrap branches read `hook|action` in one regex; the innerMessage + // door does not read the kind at all. One classification for both. + const err = new SandboxError( + "action 'showcase_recalc' threw: TypeError: not a function", + 'TypeError: not a function', + ); + const r = mapDataError(err, 'showcase_task'); + expect(r.status).toBe(500); + expect(r.body.code).toBe('INTERNAL_ERROR'); + }); +}); + +// --------------------------------------------------------------------------- +// §2 The same defect through the OTHER door — the regex fallback +// --------------------------------------------------------------------------- + +describe('[#7543] the lost-instance fallback classifies identically', () => { + it('the reported crash answers the same envelope when `innerMessage` is gone', () => { + const r = mapDataError( + hookCrashInstanceLost('showcase_normalize_task_title', 'TypeError: not a function'), + 'showcase_task', + ); + expect(r.status).toBe(500); + expect(r.body.code).toBe('INTERNAL_ERROR'); + expect(String(r.body.error)).not.toContain('TypeError'); + }); + + it('the two doors produce BYTE-EQUAL bodies for the same crash', () => { + // This is the assertion that makes the fix independent of whether the + // SandboxError instance survived a rethrow. Without it, a fix applied to + // one door only would look complete. + const viaInnerMessage = mapDataError(REPORTED(), 'showcase_task'); + const viaRawMessage = mapDataError( + hookCrashInstanceLost('showcase_normalize_task_title', 'TypeError: not a function'), + 'showcase_task', + ); + expect(viaRawMessage).toEqual(viaInnerMessage); + }); + + it('the two doors also agree on a REFUSAL — the unwrap still works', () => { + const msg = 'this task cannot be renamed after it is closed'; + const viaInnerMessage = mapDataError(hookRefusal('h', msg), 'showcase_task'); + const viaRawMessage = mapDataError( + hookCrashInstanceLost('h', `Error: ${msg}`), + 'showcase_task', + ); + expect(viaRawMessage).toEqual(viaInnerMessage); + expect(viaRawMessage.body.error).toBe(msg); + }); +}); + +// --------------------------------------------------------------------------- +// §3 What must NOT move — a deliberate business refusal +// --------------------------------------------------------------------------- + +describe('[#7543] a hook that deliberately refuses still speaks in its own words', () => { + it('a business message reaches the caller verbatim at 400', () => { + // The entire reason the unwrap branches exist. #5423's verbatim rule: + // a hook refusal is an answer addressed TO the caller and its message + // IS the remedy. + const r = mapDataError(hookRefusal('h', '删除被阻断：仍有未结清的发票'), 'showcase_task'); + expect(r.status).toBe(400); + expect(r.body.error).toBe('删除被阻断：仍有未结清的发票'); + expect(r.body.object).toBe('showcase_task'); + expect(r.body.error).not.toBe(INTERNAL_ERROR_MESSAGE); + }); + + it('the refusal envelope still carries NO `code` — #7543 did not relitigate that', () => { + // Deliberate and load-bearing: older @objectstack/client builds prepend + // any `code` to the human-readable message, which would reintroduce the + // English noise the unwrap removes. The fix changes WHICH errors take + // this branch, not what the branch emits. + const r = mapDataError(hookRefusal('h', 'month-end close is in progress'), 'showcase_task'); + expect(r.body).not.toHaveProperty('code'); + }); + + it('an English refusal that merely MENTIONS a native error name is still a refusal', () => { + // The regex is anchored at `^`, and the second case is the one that + // actually needs the anchor: prose can quote a native name WITH its + // colon (a hook re-reporting what a downstream call told it), which an + // unanchored pattern would match mid-sentence and sanitise away. + for (const msg of [ + 'the imported row produced a TypeError in your template — fix the template', + 'row 14 was rejected with TypeError: check the template before re-importing', + 'the upstream feed answered with a RangeError', + ]) { + const r = mapDataError(hookRefusal('h', msg), 'showcase_task'); + expect(r.status, msg).toBe(400); + expect(r.body.error, msg).toBe(msg); + } + }); + + it('a refusal whose message is empty-ish is untouched by the new guard', () => { + const r = mapDataError(hookRefusal('h', 'no'), 'showcase_task'); + expect(r.status).toBe(400); + expect(r.body.error).toBe('no'); + }); +}); + +// --------------------------------------------------------------------------- +// §4 The family, side by side — the card's control table +// +// The card measured three bodies on ONE route in ONE run and the defect is only +// visible as the odd one out, so they are pinned together rather than in three +// files that could drift apart. +// --------------------------------------------------------------------------- + +describe('[#7543] the control table from the report, guarded as one family', () => { + it('`{}` → 400 VALIDATION_FAILED with `fields[]` (control, unchanged)', () => { + const r = mapDataError( + validationError([{ field: 'title', code: 'required', message: 'Title is required' }]), + 'showcase_task', + ); + expect(r.status).toBe(400); + expect(r.body.code).toBe('VALIDATION_FAILED'); + expect(r.body.fields).toEqual([ + { field: 'title', code: 'required', message: 'Title is required' }, + ]); + }); + + it('a bad enum value → 400 with `invalid_option` and the allowed list (control, unchanged)', () => { + const r = mapDataError( + validationError([{ + field: 'status', + code: 'invalid_option', + message: 'Status must be one of: open, done', + options: ['open', 'done'], + }]), + 'showcase_task', + ); + expect(r.status).toBe(400); + expect(r.body.code).toBe('VALIDATION_FAILED'); + expect((r.body.fields as any[])[0].code).toBe('invalid_option'); + expect((r.body.fields as any[])[0].options).toEqual(['open', 'done']); + }); + + it('`{"title": 12345}` → the sanitised server fault, and it carries a `code` like its neighbours', () => { + const r = mapDataError(REPORTED(), 'showcase_task'); + + // It does NOT join the 400 VALIDATION_FAILED family, and pinning that it + // does not is the honest half: `record-validator.ts:503-504` COERCES a + // number in a `text` field via `String(value)`, so this request breaks + // no declared contract and names no offending field. What was wrong was + // the raw fault text and the missing `code`; both are fixed. + expect(r.body.code).toBe('INTERNAL_ERROR'); + expect(r.body.code).not.toBe('VALIDATION_FAILED'); + expect(r.body).not.toHaveProperty('fields'); + }); + + it('every member of the family carries a machine-readable `code`', () => { + // The card's second contract break, stated as the family invariant it + // actually is: no body on this route leaves without a `code`. + const family = [ + validationError([{ field: 'title', code: 'required', message: 'Title is required' }]), + validationError([{ field: 'status', code: 'invalid_option', message: 'bad', options: ['a'] }]), + REPORTED(), + ]; + for (const err of family) { + const r = mapDataError(err, 'showcase_task'); + expect(typeof r.body.code, String(err.message)).toBe('string'); + expect(String(r.body.code).length).toBeGreaterThan(0); + } + }); + + it('a crash and a refusal are DISTINGUISHABLE on the wire', () => { + // The one thing the defect made impossible: both used to be + // `400 {error, object}` with no `code`, so a client could not tell an + // app-authored business rule from a platform-side fault. + const crash = mapDataError(REPORTED(), 'showcase_task'); + const refusal = mapDataError(hookRefusal('h', 'not allowed while closed'), 'showcase_task'); + + expect(crash.status).not.toBe(refusal.status); + expect(crash.body).not.toEqual(refusal.body); + }); +}); diff --git a/packages/rest/src/rest-server.ts b/packages/rest/src/rest-server.ts index 515c8d05de..8b06c74cea 100644 --- a/packages/rest/src/rest-server.ts +++ b/packages/rest/src/rest-server.ts @@ -564,6 +564,58 @@ const UNCLASSIFIED_FAULT = (): { status: number; body: Record } body: { error: INTERNAL_ERROR_MESSAGE, code: 'INTERNAL_ERROR' }, }); +/** + * [#7543] Does an unwrapped sandbox message name a JS RUNTIME fault rather than + * a business refusal the hook body deliberately reported? + * + * The two sandbox-unwrap branches below exist for ONE shape: a hook or action + * body that runs `throw new Error('删除被阻断：仍有未结清的发票')`, i.e. an + * author writing a business rule whose message IS the remedy. They answer 400 + * with that message verbatim and deliberately no `code` (see each branch). + * + * A body that instead CRASHES — `ctx.input.title.trim()` where `title` is the + * number `12345` — also arrives as a thrown error, so it entered the same + * branch and its raw `TypeError: not a function` went out as the client-facing + * message of a 400 with no `code`. That is two contract breaks at once: an + * internal runtime fault echoed verbatim, and a body outside the ledgered + * envelope (a client keying on `code` gets nothing). + * + * The classification this restores is NOT new policy — it is the ruling + * {@link UNCLASSIFIED_FAULT} already records one door down, which names this + * exact case ("or a plain handler bug (`TypeError: x is not a function`) … + * server faults that a caller cannot fix and a caller SHOULD retry"). The + * sandbox unwraps simply sit ABOVE that branch and were intercepting the crash + * before it could reach the answer the file had already settled on. Same + * separation `quickjs-runner`'s own `sandboxFault` path draws (#4431/#3951): + * the sandbox REFUSING is a fault, and so is the body FAULTING — only the + * body's deliberate `throw` is an answer addressed to the caller. + * + * **Matched by constructor name, not by phrasing.** These eight are the ECMA-262 + * native error constructors (plus SpiderMonkey's `InternalError`, which QuickJS + * also raises for stack exhaustion); the sandbox stringifies a thrown error as + * `: `, so the name is structural evidence rather than a keyword + * heuristic over prose. `Error:` is deliberately absent — a plain `Error` is the + * documented way to author a refusal, and `userFacingMessage` strips that prefix + * upstream anyway. + * + * **Deliberate, accepted cost:** a body that expresses a business rule as + * `throw new RangeError('数量超出范围')` now gets the sanitised 500 instead of + * its own words. That authoring style is not the documented one, and erring + * toward "a native error name means a crash" is the fail-safe direction — the + * opposite default is what shipped `TypeError: not a function` to a client. + * + * The words are not lost: 500 is outside `isExpectedDataStatus`, so + * `handleRouteError` prints `[REST] Unhandled error` with the whole error, and + * `sendError`'s `logWithheldServerFault` (#5437) covers the routes that bypass + * it — the same operator path {@link UNCLASSIFIED_FAULT} relies on. + */ +const NATIVE_ERROR_NAME_RE = + /^(?:Type|Reference|Range|Syntax|URI|Eval|Internal|Aggregate)Error(?::|$)/; + +function isScriptFaultMessage(message: string): boolean { + return NATIVE_ERROR_NAME_RE.test(message.trim()); +} + /** * [#5462] Does a driver's missing-relation message name the very object this * request asked for? @@ -810,6 +862,11 @@ export function mapDataError(error: any, object?: string): { status: number; bod // bundled in deployed consoles) prepend any `code` to the human-readable // message, which would reintroduce the English noise this branch removes. if (typeof error?.innerMessage === 'string' && error.innerMessage) { + // [#7543] …but only when the body REPORTED something. A body that + // CRASHED arrives here too, and its `TypeError: not a function` is an + // internal fault, not a business message — see + // {@link isScriptFaultMessage}. + if (isScriptFaultMessage(error.innerMessage)) return UNCLASSIFIED_FAULT(); return { status: 400, body: { @@ -1040,14 +1097,21 @@ export function mapDataError(error: any, object?: string): { status: number; bod // Fallback for the same sandbox wrapper when the SandboxError instance // (and its `innerMessage`) was lost crossing a rethrow/serialization // boundary: strip the debug wrapper from the raw message. A leading - // default `Error: ` name is dropped; non-default names (`TypeError: …`) - // are kept — they signal a genuine script bug rather than a deliberately - // thrown business rule, which is useful context. + // default `Error: ` name is dropped. + // + // [#7543] A non-default name (`TypeError: …`) used to be KEPT here and + // shipped as the 400's message "as useful context". It is useful context — + // for an OPERATOR, in the log, which is where it still goes. On the wire it + // was a raw runtime fault presented to a client as their own mistake. This + // door and the `innerMessage` door above produce byte-identical bodies, so + // they must classify identically or the fix would depend on whether the + // SandboxError instance happened to survive the rethrow. const sandboxWrapper = /^(?:hook|action) '[^']*' threw:\s*(.+)$/s.exec(raw); if (sandboxWrapper) { const msg = sandboxWrapper[1].startsWith('Error: ') ? sandboxWrapper[1].slice('Error: '.length) : sandboxWrapper[1]; + if (isScriptFaultMessage(msg)) return UNCLASSIFIED_FAULT(); return { status: 400, body: { diff --git a/packages/rest/src/rest.test.ts b/packages/rest/src/rest.test.ts index 0103a6e4e5..f5191e38d2 100644 --- a/packages/rest/src/rest.test.ts +++ b/packages/rest/src/rest.test.ts @@ -2646,10 +2646,20 @@ describe('mapDataError — schema/constraint envelopes', () => { expect(r.body.code).toBeUndefined(); }); - it('keeps non-default error names when stripping the wrapper (genuine script bugs stay identifiable)', () => { + // [#7543] REVERSED. This case used to assert the opposite — that a non-default + // error name was KEPT on the wire so "genuine script bugs stay identifiable". + // Identifiable to WHOM is the question it did not ask: an operator reads the + // log (500 is outside `isExpectedDataStatus`, so `handleRouteError` still + // prints the whole error), while the client got a raw `TypeError` presented as + // their own mistake, in a 400 with no `code`. A crashing body is a server + // fault — the ruling `UNCLASSIFIED_FAULT`'s own docblock (#5489) already + // records for this exact text. Full coverage of the crash-vs-refusal + // classification lives in `rest-hook-script-fault-envelope.test.ts`. + it('sanitises a crashing hook body instead of shipping its runtime fault', () => { const r = mapDataError(new Error("hook 'pm_ref_base' threw: TypeError: cannot read properties of undefined"), 'pm_base'); - expect(r.status).toBe(400); - expect(r.body.error).toBe('TypeError: cannot read properties of undefined'); + expect(r.status).toBe(500); + expect(r.body.code).toBe('INTERNAL_ERROR'); + expect(String(r.body.error)).not.toContain('TypeError'); }); it("unwraps an action body's wrapper the same way", () => { From 24e3d4b0e2a0e7eb5702747822e960b97122d154 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 11 Aug 2026 10:00:13 +0000 Subject: [PATCH 2/2] test(dogfood,runtime): reverse the E2E pin that kept a hook's `TypeError` on the wire (#7543) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit CI caught a second pin of the behaviour this card calls a defect, at the level that matters most: `hook-error-format.dogfood.test.ts` drives the whole chain for real (QuickJS → ObjectQL triggerHooks → mapDataError → HTTP) and asserted `400` + `body.error === 'TypeError: boom'`. Reversed in place like its unit counterpart, not deleted — it is now the only end-to-end proof that a crashing body is sanitised on the wire. The local run also CONFIRMS the operator-visibility claim the fix rests on: ERROR [BodyRunner] sandboxed hook threw {"hook":"hef_buggy_guard","error":{"message":"hook 'hef_buggy_guard' threw: TypeError: boom", ...}} — full text in the server log, sanitised envelope on the wire. Added a ground-truth case alongside it: the status changed, the transactional outcome must not. `onError` defaults to abort, and a sanitised envelope must not be mistaken for a soft failure that let the write through. `domains/actions.ts`: comment only. Its #3913 fault classifier already told a `TypeError` body-throw apart BY NAME and answered a server fault, and it cited rest's comment as the shared signal — while rest drew the opposite conclusion from that same evidence and shipped the name to the client as a 400. The citation is updated to record that the two exits now agree on the reading, not just the signal. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01UYt5skZ9r78Hnnza2b1jnK --- .../test/hook-error-format.dogfood.test.ts | 50 ++++++++++++++++--- packages/runtime/src/domains/actions.ts | 10 ++-- 2 files changed, 49 insertions(+), 11 deletions(-) diff --git a/packages/qa/dogfood/test/hook-error-format.dogfood.test.ts b/packages/qa/dogfood/test/hook-error-format.dogfood.test.ts index 4365a35e06..58af6293b4 100644 --- a/packages/qa/dogfood/test/hook-error-format.dogfood.test.ts +++ b/packages/qa/dogfood/test/hook-error-format.dogfood.test.ts @@ -12,8 +12,23 @@ // and not a `code` field an older bundled @objectstack/client would prepend // as `[ObjectStack] CODE: …`. // -// Non-default error names (`TypeError: …`) are deliberately KEPT: they mark -// a genuine script bug rather than a thrown business rule. +// [#7543] Non-default error names (`TypeError: …`) are the OPPOSITE case, and +// this file used to assert they were KEPT on the wire "as useful context". They +// are not context for the caller — they are a server-side fault, and shipping +// one as the client-facing message of a 400 with no `code` was the defect #7543 +// reports (`POST /data/showcase_task {"title":12345}` → +// `400 {"error":"TypeError: not a function"}`). A body that CRASHES now answers +// the sanitised `500 INTERNAL_ERROR` that `mapDataError`'s terminal branch +// already gives every other handler bug (#5489). The full text still reaches the +// operator's log; only the wire is sanitised. +// +// The distinction this file pins end-to-end is therefore: a hook that REPORTS +// (`throw new Error('业务规则')`) speaks to the caller verbatim at 400, and a +// hook that FAULTS (`throw new TypeError('boom')`) does not speak to the caller +// at all. Classification lives in `packages/rest/src/rest-server.ts` +// (`isScriptFaultMessage`), unit-covered in +// `packages/rest/src/rest-hook-script-fault-envelope.test.ts`; this file is the +// only place the whole chain runs for real. import { describe, it, expect, beforeAll, afterAll } from 'vitest'; import { bootStack, type VerifyStack } from '@objectstack/verify'; @@ -55,8 +70,11 @@ const hefStack = defineStack({ }, }, { - // A non-default error name signals a script bug, not a business rule — - // the name must survive to the client as useful context. + // [#7543] A non-default error name signals a script bug, not a business + // rule — so it must NOT reach the client. This fixture is the reported + // defect in miniature: `showcase_normalize_task_title` crashed the same + // way on `{"title": 12345}` (a truthy number has no `.trim`), and the + // resulting `TypeError` went out as the 400's message. name: 'hef_buggy_guard', object: 'hef_base', events: ['beforeUpdate'], @@ -107,12 +125,28 @@ describe('objectstack verify: sandboxed hook error message format (#hef)', () => expect(r.status).toBe(200); }); - it('non-default error names (TypeError) survive as script-bug context', async () => { + // [#7543] REVERSED — see the file header. This case previously asserted + // `400` + `body.error === 'TypeError: boom'`. + it('a hook body that CRASHES is sanitised, not echoed to the client', async () => { const r = await stack.apiAs(token, 'PATCH', `/data/hef_base/${baseId}`, { name: '改名' }); - expect(r.status).toBe(400); + expect(r.status).toBe(500); const body = (await r.json()) as any; - expect(body.error).toBe('TypeError: boom'); - expect(JSON.stringify(body)).not.toMatch(/threw:|hook '/); + // Nothing of the runtime fault survives: not the constructor name, not the + // thrown text, not the sandbox debug wrapper. + expect(JSON.stringify(body)).not.toMatch(/TypeError|boom|threw:|hook '/); + // …and it joins the ledgered envelope, which the old 400 did not: a client + // keying on `code` got nothing at all from this response. + expect(body.code).toBe('INTERNAL_ERROR'); + }); + + it('ground truth: the crashing hook still aborted the write', async () => { + // The status changed; the transactional outcome must not. `onError` defaults + // to abort, and a sanitised envelope must not be mistaken for a soft failure + // that let the write through. + const r = await stack.apiAs(token, 'GET', `/data/hef_base/${baseId}`); + expect(r.status).toBe(200); + const body = (await r.json()) as any; + expect((body.record ?? body).name).toBe('华东制作基地'); }); }); diff --git a/packages/runtime/src/domains/actions.ts b/packages/runtime/src/domains/actions.ts index 26aac07e0d..cc7a8e4ace 100644 --- a/packages/runtime/src/domains/actions.ts +++ b/packages/runtime/src/domains/actions.ts @@ -448,9 +448,13 @@ export async function handleActionsRequest(deps: DomainHandlerDeps, path: string // no signal short of body-parsing at every hop. // // Told apart by the error's NAME, the same signal `@objectstack/rest` - // already uses on this exact distinction ("non-default names - // (`TypeError: …`) […] signal a genuine script bug rather than a - // deliberately thrown business rule"): + // uses on this exact distinction. [#7543] That citation used to quote + // rest's comment for the EVIDENCE while the two exits drew opposite + // conclusions from it: rest read a non-default name as "a genuine script + // bug" and then shipped it to the client as a 400 anyway. It no longer + // does — `mapDataError`'s `isScriptFaultMessage` now answers the + // sanitised `500 INTERNAL_ERROR` this branch has answered since #3913, + // so the two exits agree on the reading as well as the signal: // // name === 'Error' a deliberate `throw new Error(msg)` — the // shape a registered handler uses to reject.