fix(rest): a crashing hook body answers the sanitised fault envelope, not a raw TypeError at 400 (#7543) - #7632
Merged
Conversation
… 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`. 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 `<name>: <message>`, 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UYt5skZ9r78Hnnza2b1jnK
|
The latest updates on your projects. Learn more about Vercel for GitHub. 1 Skipped Deployment
|
Contributor
📓 Docs Drift CheckThis PR changes 2 package(s): 24 hand-written doc(s) reference the affected code and may need an implementation-accuracy re-verification:
⛔ 3 release-owned page(s) also reference the affected code. These are read-only:
|
github-actions
Bot
added
documentation
…eerror-in-400-envelope-wt
…ror` on the wire (#7543) 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 <noreply@anthropic.com> Claude-Session: https://claude.ai/code/session_01UYt5skZ9r78Hnnza2b1jnK
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes #7543
The seam — located, not assumed
The card's investigation note is a negative result: the observed body —
400, raw message, anobjectkey, nocode— "matches no branch ofmapDataError". That note is wrong, and finding out how is the fix.It matches two branches: the sandbox unwraps, which are the only ones in the file that emit
{ error, object }with nocodeat 400. Reproduced byte-for-byte onmain@4ed4160(i.e. after PR #7575 landeddeclaredHttpStatus, which does not touch these branches):The full chain:
examples/app-showcase/src/data/hooks/index.tsNormalizeTaskTitleHookranif (ctx.input.title) ctx.input.title = ctx.input.title.trim();.12345is truthy and has no.trim⇒ QuickJS throws.quickjs-runner.tswraps it asSandboxError("hook '<name>' threw: …", userFacingMessage(…)).userFacingMessagestrips only a leadingError:, soinnerMessagekeepsTypeError: not a function.mapDataError'sinnerMessagebranch answers 400 with that string and deliberately nocode.This also answers the note's "the throw is upstream or downstream of
validateOne" — upstream. The hook isbeforeInsert, so it throws before the validator's safeString(value)ever sees the record.Landing package is
packages/rest⇒ this lane. Thepackages/objectqlbranch of the lane boundary does not apply:objectqlnever gets to run.The fix
Both unwrap 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
<name>: <message>, so a leadingTypeError:/ReferenceError:/RangeError:/SyntaxError:/URIError:/EvalError:/InternalError:/AggregateError:is structural evidence, not a keyword heuristic over prose.A crash answers the same sanitised
500 INTERNAL_ERRORthe mapper's terminal branch already gives. That is not new policy —UNCLASSIFIED_FAULT's own docblock (#5489) 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 unwraps simply sat above that branch and intercepted the crash first.Both doors are guarded, not one. The
innerMessagebranch and the raw-message regex fallback emit byte-identical bodies, so guarding one would make the envelope depend on whether theSandboxErrorinstance survived a rethrow. Mutation D below proves this.It also resolves a standing disagreement between two exits
runtime/src/domains/actions.ts's #3913 classifier already told aTypeErrorbody-throw apart by name and answered a server fault — and cited@objectstack/restas the source of that signal. But rest read the same evidence ("non-default names signal a genuine script bug") and then shipped the name to the client as a 400 anyway. The two exits shared the signal and drew opposite conclusions. They now agree; the stale citation inactions.tsis updated (comment only, no behaviour change).What did not move
A deliberate refusal still reaches the caller verbatim at 400 with no
code(load-bearing: older bundled@objectstack/clientbuilds prepend anycodeto the message). The fix changes which errors take that branch, not what it emits.Accepted cost: a body expressing a business rule as
throw new RangeError('…')is now sanitised. 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 — verified in the live dogfood run, not just asserted:
The dispatch's accept bar says
{"title": 12345}must join its neighbours —400 VALIDATION_FAILEDwithfields[]. It cannot, and asserting that it does would pin a false statement.packages/objectql/src/validation/record-validator.ts:503-504:The value is coerced, every length/format check runs against
"12345", and the branch returnsnull. 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, andVALIDATION_FAILEDwould be a lie about the caller. The premise behind thefields[]half of the accept bar is that this body is invalid input; the validator says it isn't.Both contract breaks the card actually names are fixed: no raw runtime error text on the wire, and the body now carries a
code. §4 of the new test pins the three control-table bodies side by side, each asserted for what it truthfully is.With the showcase hook fixed, the reported request now succeeds rather than erroring — the correct outcome given the coercion contract.
Tests
packages/rest/src/rest-hook-script-fault-envelope.test.ts— 18 cases across four sections: the defect via theinnerMessagedoor, the same via the regex fallback, the business-refusal behaviour that must not move, and the card's control table guarded as one family.packages/rest/src/rest.test.ts— the unit pin.packages/qa/dogfood/test/hook-error-format.dogfood.test.ts— the E2E pin, caught by CI on the first push. This one drives the whole chain for real (QuickJS → ObjectQLtriggerHooks→mapDataError→ HTTP) and is now the only end-to-end proof that a crashing body is sanitised on the wire. A ground-truth case was added beside it: the status changed, butonError: abortsemantics must not — a sanitised envelope must not be mistaken for a soft failure that let the write through.Mutation table — all 18 unit cases proven able to fail
isScriptFaultMessage→ alwaystrueisScriptFaultMessage→ alwaysfalseinnerMessagedoor unguarded, regex door kept^anchor droppedVALIDATION_FAILED'sfields→[]Two predictions were wrong and are recorded as measured rather than rewritten to fit:
(?::|$)limb already refuses prose like "produced a TypeError in your template", so the original assertion never exercised the anchor. The test was strengthened with a message quoting a native name with its colon mid-sentence ("rejected with TypeError: check the template"), which only^refuses. Re-measured: 1 red.Verification
Re-run after merging
origin/main(b3de0dd) and rebuilding:pnpm lint— cleanpnpm typecheck— 126/126 packages@objectstack/rest— 1421 passed (88 files)@objectstack/runtime— 2011 passed (126 files), the sandbox producer side@objectstack/dogfood— 548 passed, 3 skipped (88 files) — the suite whose shard 1/3 caught the E2E pin@objectstack/example-showcase— 171 passed (18 files)Changeset included. No
content/docs/releases/**edits.