From 88e998fcc71f3b2fa230df332bdf262156d8d676 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 11 Sep 2026 10:26:02 +0000 Subject: [PATCH 1/2] fix(runtime): a nested sandboxed hook refusal is a rejection, not a sandbox fault MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit A `beforeUpdate` hook that refuses a state transition for a business reason, reached through a script action's `ctx.api` write, answered `500 INTERNAL_ERROR` on `POST /actions/:object/:action` — the same refusal `/data` has answered `400` with the sentence verbatim since #11588. `domains/actions.ts` is not the producer. `hostErrorToVm` marked EVERY `SandboxError` crossing into the action body's VM as the sandbox's own fault (#4431) on an `instanceof` test — and a nested sandboxed hook's refusal is a `SandboxError`, wrapped by this same runner one level down. The pump branch that reads the marker then dropped `innerMessage`, `code`, `status` and `fields`, and the classifier correctly read that absence as a crash. The marker now asks the question `/data` asks — `sandboxBusinessMessage` (#11588), spelled in-package as `sandboxRefusalMessage` because `@objectstack/rest` re-exports nothing from `error-response` and importing it would widen that package's published surface. Both of its conditions travel: a capability denial has no business message and stays a fault, and a nested CRASH carries `TypeError: …` and stays a fault too, so neither side of the pinned fault/rejection line moves. Message-neutral by construction: the client-facing sentence is byte-identical to what the 500 already carried, because the flattened `SandboxError: ` name prefix is stripped on the rejection path by the same helper the fault path already used. Claude-Session: https://claude.ai/code/session_01TSf4DV7ziu4V5j73e46b7c Co-authored-by: Claude --- ...nested-hook-refusal-is-a-rejection.test.ts | 228 ++++++++++++++++++ .../runtime/src/sandbox/quickjs-runner.ts | 88 ++++++- 2 files changed, 312 insertions(+), 4 deletions(-) create mode 100644 packages/runtime/src/sandbox/nested-hook-refusal-is-a-rejection.test.ts diff --git a/packages/runtime/src/sandbox/nested-hook-refusal-is-a-rejection.test.ts b/packages/runtime/src/sandbox/nested-hook-refusal-is-a-rejection.test.ts new file mode 100644 index 0000000000..976fac860e --- /dev/null +++ b/packages/runtime/src/sandbox/nested-hook-refusal-is-a-rejection.test.ts @@ -0,0 +1,228 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#17265] A sandboxed hook's BUSINESS REFUSAL reached through a script + * action's `ctx.api` write is a rejection, not the sandbox faulting. + * + * ## What was measured broken + * + * The card reports `POST /api/v1/actions/clm_contract/submit_contract` + * answering `500 INTERNAL_ERROR` for a `beforeUpdate` hook that refused a state + * transition with a written, user-facing sentence — the same refusal `/data` + * has answered `400` with the sentence verbatim since #11588. + * + * `domains/actions.ts`'s classifier is NOT the producer: it reads the shape it + * is handed correctly (`actions-fault-vs-rejection.test.ts` pins both sides of + * that line and neither moves). The refusal arrives at it already stripped of + * every mark that says "a body reported this on purpose", one VM hop earlier: + * + * 1. the sandboxed `beforeUpdate` hook refuses — its own runner wraps that as + * `SandboxError("hook 'g' threw: ", "")`, `innerMessage` SET; + * 2. it travels out of `engine.update()` into the ACTION body's host call, so + * `hostErrorToVm` marshals it INTO the action's VM — and marked every + * `SandboxError` reaching it as {@link SANDBOX_FAULT_PROP}, the sandbox's + * OWN fault (#4431), on an `instanceof` test; + * 3. escaping the action body uncaught, the pump loop reads that marker and + * throws a bare `SandboxError` — no `innerMessage`, no `code`, no + * `status`, no `fields`; + * 4. which is, by the #3951 contract, exactly a CRASH ⇒ `errorFromThrown(err, + * 500)` ⇒ `500 INTERNAL_ERROR`. + * + * The marker's own sibling pin already named this risk — "a marker applied too + * broadly would turn every failed write into a 500" + * (`capability-denial-is-a-fault.test.ts`) — and measured it with a plain + * `ValidationError`, which is not a `SandboxError` and so never tripped the + * `instanceof`. A NESTED sandboxed refusal is. + * + * ## The line this file pins + * + * The discriminator is the one `/data` asks (`sandboxBusinessMessage`, #11588): + * does the error carry a caller-addressed business sentence that is not a + * script fault? A refusal does and stays a rejection; a capability denial and a + * nested CRASH do not and stay faults. So `/data`'s answer and the action + * route's answer are the same answer, per route and per status. + */ + +import { describe, it, expect, vi } from 'vitest'; + +import { HttpDispatcher } from '../http-dispatcher.js'; +import { QuickJSScriptRunner, SandboxError } from './quickjs-runner.js'; +import type { ScriptContext, ScriptRunOptions } from './script-runner.js'; + +/** The sentence the consuming app's guard addressed to its end user. */ +const REFUSAL = 'A contract cannot be submitted without a version file'; + +const runner = new QuickJSScriptRunner({ hookTimeoutMs: 10_000, actionTimeoutMs: 10_000 }); +const actionOpts: ScriptRunOptions = { origin: { kind: 'action', name: 'submit_contract' } }; + +/** + * The shape a SANDBOXED `beforeUpdate` hook's deliberate refusal really has + * when `engine.update()` hands it back to the action body's host call — built + * by `quickjs-runner`'s own pump loop one level down, so `innerMessage` is set. + */ +function nestedHookRefusal(): SandboxError { + return new SandboxError(`hook 'guard_contract_submit' threw: ${REFUSAL}`, REFUSAL); +} + +/** A `ctx.api.object(x).update(...)` host seam whose write the hook refuses. */ +function refusingApi(thrown: () => unknown) { + return { + object: (_n: string) => ({ + update: async () => { throw thrown(); }, + }), + }; +} + +function ctx(over: Partial = {}): ScriptContext { + return { input: {}, ...over }; +} + +/** Run a script action whose `ctx.api` write throws `thrown`, and return the escape. */ +async function escapeOf(thrown: () => unknown): Promise { + const err = await runner.runScript( + { + language: 'js', + source: "return await ctx.api.object('clm_contract').update('c_1', { status: 'submitted' });", + capabilities: ['api.write'], + }, + ctx({ api: refusingApi(thrown) }), + actionOpts, + ).then(() => null, (e) => e); + expect(err, 'expected the action to reject, but the script resolved').toBeInstanceOf(SandboxError); + return err; +} + +// ── the wire half ──────────────────────────────────────────────────────────── + +const scriptAction = { + name: 'submit_contract', + objectName: 'clm_contract', + type: 'script', + body: { language: 'js', source: 'return 1;', capabilities: ['api.write'] }, +}; + +/** The same dispatcher harness `actions-fault-vs-rejection.test.ts` uses. */ +function makeDispatcher(thrown: unknown) { + const objectDef = { name: 'clm_contract', actions: [scriptAction] }; + const ql: any = { + executeAction: vi.fn(async () => { throw thrown; }), + getSchema: (n: string) => (n === objectDef.name ? objectDef : undefined), + registry: { getObject: (n: string) => (n === objectDef.name ? objectDef : undefined), getItem: () => undefined }, + find: vi.fn(async () => [{ id: 'c_1', status: 'draft' }]), + insert: vi.fn(), update: vi.fn(), delete: vi.fn(), + }; + const metadata: any = { + load: vi.fn(async () => null), + listObjects: vi.fn(async () => [objectDef]), + getObject: vi.fn(async () => objectDef), + }; + const kernel: any = { + context: { + getService: (n: string) => + n === 'objectql' || n === 'data' ? ql : n === 'metadata' ? metadata : null, + }, + }; + return new HttpDispatcher(kernel); +} + +async function wireAnswer(thrown: unknown) { + const res: any = await makeDispatcher(thrown).handleActions( + '/clm_contract/submit_contract/c_1', + 'POST', + {}, + { request: {}, environmentId: 'platform', executionContext: { userId: 'u1', systemPermissions: [] } } as any, + ); + return res.response; +} + +describe('[#17265] a nested sandboxed hook refusal keeps its business message', () => { + it("a beforeUpdate refusal reached through ctx.api.update is NOT the sandbox's own fault", async () => { + const err = await escapeOf(nestedHookRefusal); + + // THE defect: the marker was applied on `instanceof SandboxError`, so + // the nested refusal came back as a bare fault — no business message at + // all, which is exactly what the #3951 contract reads as a CRASH. + expect(err.innerMessage, 'the hook refusal must survive as a business message').toBeDefined(); + expect(err.innerMessage).toContain(REFUSAL); + // …and the action's own debug wrapper still identifies who threw, in + // the log-only `.message`, which keeps the WHOLE chain. + expect(err.message).toContain("action 'submit_contract' threw:"); + expect(err.message).toContain("hook 'guard_contract_submit' threw:"); + + // Message-NEUTRAL: this repair moves the status, not the sentence. The + // client-facing text is byte-identical to what the 500 already carried + // — the VM's `SandboxError: ` name prefix is a debug artefact and has + // never reached the wire. + expect(err.innerMessage).toBe(`hook 'guard_contract_submit' threw: ${REFUSAL}`); + expect(err.innerMessage).not.toContain('SandboxError:'); + }); + + it('the wire answer matches /data: 400 VALIDATION_ERROR with the sentence', async () => { + // /data answers this refusal `400` with `error.innerMessage` verbatim + // (`error-response.ts`'s sandbox unwrap door, `declared ?? 400`). The + // action route must not answer a second thing for one refusal. + const response = await wireAnswer(await escapeOf(nestedHookRefusal)); + + expect(response.status).toBe(400); + expect(response.body.error.code).toBe('VALIDATION_ERROR'); + expect(response.body.error.message).toContain(REFUSAL); + }); + + it("a nested refusal's DECLARED status and code survive the hop", async () => { + // The fault branch dropped the whole `__errorInfo` payload, not just + // `innerMessage`, so a hook declaring `{ status: 409, code: + // 'RECORD_LOCKED' }` lost both and was flattened to 500. `/data` + // answers `declared ?? 400` for this producer (#9967); the action door + // honours a declared status at its own first arm (#7867), so once the + // classification is right the two agree without a second rule. + const locked = () => { + const e: any = new SandboxError( + `hook 'guard_contract_submit' threw: ${REFUSAL}`, + REFUSAL, + { code: 'RECORD_LOCKED', status: 409 }, + ); + return e; + }; + const response = await wireAnswer(await escapeOf(locked)); + + expect(response.status).toBe(409); + expect(response.body.error.code).toBe('RECORD_LOCKED'); + expect(response.body.error.message).toContain(REFUSAL); + }); +}); + +describe('[#17265] the FAULT side of the #4431 contract is untouched', () => { + it("a nested CRASH is still a fault — sandboxBusinessMessage declines it", async () => { + // A hook that blew up arrives in the same shape with a native error + // name inside `innerMessage`. `/data` answers the sanitised 500 for it + // (#7543), so the action route must too: the business-message read is + // what separates them, never the error's class. + const crash = () => + new SandboxError( + "hook 'guard_contract_submit' threw: TypeError: cannot read properties of undefined", + 'TypeError: cannot read properties of undefined', + ); + const response = await wireAnswer(await escapeOf(crash)); + + expect(response.status).toBe(500); + expect(response.body.error.code).toBe('INTERNAL_ERROR'); + }); + + it('a capability denial inside the action body is still a fault', async () => { + // The #4431 case itself: the sandbox refused before user code ran, so + // there is no business message to carry and the 500 must stand. + const err = await runner.runScript( + { + language: 'js', + source: "return ctx.api.object('clm_contract').count({});", + capabilities: [], + }, + ctx({ api: { object: (_n: string) => ({ count: (_f: unknown) => 1 }) } }), + actionOpts, + ).then(() => null, (e: any) => e); + + expect(err).toBeInstanceOf(SandboxError); + expect(err.innerMessage).toBeUndefined(); + expect((await wireAnswer(err)).status).toBe(500); + }); +}); diff --git a/packages/runtime/src/sandbox/quickjs-runner.ts b/packages/runtime/src/sandbox/quickjs-runner.ts index 68662eca4c..7e3a89ffe8 100644 --- a/packages/runtime/src/sandbox/quickjs-runner.ts +++ b/packages/runtime/src/sandbox/quickjs-runner.ts @@ -395,11 +395,19 @@ export class QuickJSScriptRunner implements ScriptRunner { // the 400 a denial used to get — and the client sees the capability // text without the `SandboxError: ` debug prefix. if (info?.sandboxFault) { - throw new SandboxError(sandboxFaultMessage(String(errStr))); + throw new SandboxError(withoutSandboxErrorPrefix(String(errStr))); } + // [#17265] The `.message` wrapper keeps the WHOLE flattened chain for + // the log — `action 'x' threw: SandboxError: hook 'g' threw: ` + // names every frame that refused. The client-facing sentence drops the + // inner `SandboxError: ` token by the same rule the fault branch above + // applies: the VM's name prefix is a debug artefact and never reached + // the wire before a nested refusal stopped being classified as a + // fault, so this repair moves the STATUS and leaves the sentence a + // caller receives byte-identical to what it was. throw new SandboxError( `${args.origin.kind} '${args.origin.name}' threw: ${errStr}`, - userFacingMessage(String(errStr)), + userFacingMessage(withoutSandboxErrorPrefix(String(errStr))), info, ); } @@ -1291,7 +1299,19 @@ function hostErrorToVm(vm: QuickJSContext, err: unknown): QuickJSHandle { } // [#4431] Mark the sandbox's OWN faults so the pump loop can tell them // apart from a user throw after the VM has flattened both to a string. - if (err instanceof SandboxError) { + // + // [#17265] …asked as a QUESTION about the error, never as a bare + // `instanceof`. A NESTED sandboxed body's refusal is a `SandboxError` too: + // the `beforeUpdate` hook that `engine.update()` dispatched underneath this + // body's `ctx.api` write was wrapped by this very runner one level down. So + // the type test marked a deliberate business refusal as the sandbox + // faulting, the pump branch that reads the marker then dropped its + // `innerMessage` (and its `code`/`status`/`fields`), and + // `domains/actions.ts` read that absence as a CRASH — answering + // `500 INTERNAL_ERROR` for a refusal `/data` has answered `400` with the + // sentence verbatim since #11588. {@link sandboxRefusalMessage} is that + // door's own question, so the two doors answer one refusal once. + if (err instanceof SandboxError && sandboxRefusalMessage(err) === undefined) { const h = vm.true; vm.setProp(errH, SANDBOX_FAULT_PROP, h); } @@ -1345,6 +1365,59 @@ function hostErrorToVm(vm: QuickJSContext, err: unknown): QuickJSHandle { */ const SANDBOX_FAULT_PROP = '__objectstackSandboxFault'; +/** + * [#17265] The ECMA-262 native error constructors, plus SpiderMonkey's + * `InternalError` which QuickJS also raises — the third copy of one pattern, + * and deliberately a copy. + * + * `packages/rest`'s `isScriptFaultMessage` (`error-response.ts`) is the + * original and `packages/objectql`'s `isScriptCrash` + * (`hook-withheld-readonly-fault.ts`) already keeps its own, for the reason + * stated there: the importing package must not take a dependency on + * `@objectstack/rest` for a regex. This package's reason is one step narrower — + * `@objectstack/runtime` DOES depend on `@objectstack/rest`, but that package + * declares exactly one export subpath (`"."`) and re-exports nothing from + * `error-response`, so importing the predicate would mean WIDENING rest's + * published surface for an internal read. + * + * ⛔ Same deliberate omission of a bare `Error:` as both siblings: a body's + * plain `Error` is the documented way to AUTHOR a refusal, so it is never a + * crash. + */ +const NATIVE_ERROR_NAME_RE = + /^(?:Type|Reference|Range|Syntax|URI|Eval|Internal|Aggregate)Error(?::|$)/; + +/** + * [#17265] The caller-addressed BUSINESS sentence a sandboxed body threw, or + * `undefined` when this error is not a body's deliberate refusal. + * + * This is `packages/rest`'s `sandboxBusinessMessage` (#11588) — the read the + * `/data` door and, since #11684, the `/analytics/dataset/query` door both make + * instead of open-coding a local opinion. Both of its conditions travel, in the + * same order, because both are load-bearing HERE: + * + * - a non-empty string `.innerMessage` — the sandbox's own mark for "user code + * threw this deliberately", and by {@link SandboxError}'s contract the thing + * a capability denial, a timeout and a marshalling failure all lack. Its + * absence is what keeps every #4431 case marked as a fault; + * - NOT a native error name (#7543). A nested body that CRASHED arrives in the + * identical shape carrying `TypeError: …`, which is an internal fault and + * not a sentence addressed to anyone. Dropping this half would turn a nested + * crash into a 400 and move the `an unexpected FAULT is a 500` line that + * `domains/actions-fault-vs-rejection.test.ts` pins. + * + * ⛔ A READ of the field the runner populated, never a pattern-strip of the + * ` '' threw:` wrapper off `.message` — the sibling's rule, for the + * sibling's reason: a plain error whose own prose contains `threw:` must not be + * rewritten. + */ +function sandboxRefusalMessage(error: unknown): string | undefined { + const inner = (error as { innerMessage?: unknown } | null | undefined)?.innerMessage; + if (typeof inner !== 'string' || !inner) return undefined; + if (NATIVE_ERROR_NAME_RE.test(inner.trim())) return undefined; + return inner; +} + /** * [#4431] Throw a sandbox-internal fault OUT OF a host function so it reaches * the VM carrying {@link SANDBOX_FAULT_PROP}. @@ -1371,8 +1444,15 @@ function throwSandboxFault(vm: QuickJSContext, message: string): never { * for a sandbox fault there is no business message at all, so what reaches the * client is this text — the capability, the origin and the call that tripped * the gate — with the debug prefix removed. + * + * [#17265] Named for the OPERATION rather than for one of its callers, because + * it now has two: the same flattened prefix appears on the REJECTION path once + * a nested body's refusal stops being marked a fault, and the prefix belongs in + * the log there for exactly the same reason. One strip, two readers — the + * alternative was a second helper doing the same thing, which is the + * local-opinion shape this card exists to remove. */ -function sandboxFaultMessage(raw: string): string { +function withoutSandboxErrorPrefix(raw: string): string { return raw.startsWith('SandboxError: ') ? raw.slice('SandboxError: '.length) : raw; } From 83ea3ce7c53edd3416fb72a54636f30f98c0d043 Mon Sep 17 00:00:00 2001 From: Claude Date: Fri, 11 Sep 2026 10:35:41 +0000 Subject: [PATCH 2/2] chore(changeset): 17265 nested hook refusal answers 4xx Claude-Session: https://claude.ai/code/session_01TSf4DV7ziu4V5j73e46b7c Co-authored-by: Claude --- ...7265-nested-hook-refusal-is-a-rejection.md | 42 +++++++++++++++++++ 1 file changed, 42 insertions(+) create mode 100644 .changeset/17265-nested-hook-refusal-is-a-rejection.md diff --git a/.changeset/17265-nested-hook-refusal-is-a-rejection.md b/.changeset/17265-nested-hook-refusal-is-a-rejection.md new file mode 100644 index 0000000000..777560ecd9 --- /dev/null +++ b/.changeset/17265-nested-hook-refusal-is-a-rejection.md @@ -0,0 +1,42 @@ +--- +'@objectstack/runtime': patch +--- + +A sandboxed hook's business refusal reached through a script action answers 4xx, not `500 INTERNAL_ERROR` + +`POST /api/v1/actions/:object/:action` answered **`500 INTERNAL_ERROR`** when a +`beforeUpdate` hook refused a state transition for a business reason and the +refusal travelled out through the action body's `ctx.api` write. The same refusal +has answered **`400`**, with the hook's sentence verbatim, on `/data` since +objectstack#11588. A 500 tells every client "the platform broke", so a +well-behaved one retries, alerts or pages for a guard that will never say yes. + +**Where the producer was.** Not in the action route's classifier — that read the +shape it was handed correctly, and both sides of the line it pins (`a deliberate +REJECTION is a 400` / `an unexpected FAULT is a 500`) are unchanged. The refusal +arrived already stripped of every mark that says "a body reported this on +purpose", one VM hop earlier: `hostErrorToVm` marked **every** `SandboxError` +crossing into the action body's VM as the sandbox's OWN fault (objectstack#4431) +on an `instanceof` test — and a nested sandboxed hook's refusal *is* a +`SandboxError`, wrapped by the same runner one level down. The pump branch that +reads that marker then discarded `innerMessage`, `code`, `status` and `fields`, +and the classifier read the missing business message as a crash. + +**What changed.** The marker now asks the question the `/data` door asks — +`sandboxBusinessMessage`, objectstack#11588 — instead of testing the error's +class. Both of that predicate's conditions travel, because both are load-bearing: +a capability denial carries no business message and stays a fault, and a nested +body that **crashed** carries `TypeError: …` and stays a fault too. + +**No status was picked for this route.** It matches what `/data` already answers +for the same producer: the status the body declared, or `400` when it declared +none. A refusal that declares `{ status: 409, code: 'RECORD_LOCKED' }` now +reaches the caller as `409 RECORD_LOCKED` instead of losing both. + +**The sentence a caller receives is byte-identical to what the 500 carried** — +this moves the status, not the prose. The flattened `SandboxError: ` name prefix +is stripped on the rejection path by the same helper the fault path already used. + +No authorable key, accept set or export surface moves; no consumer needs a +change. Clients branching on 5xx to decide whether to retry will stop retrying +these refusals.