From e94f8ae0544a4beeabf72091eac51464f8a73d85 Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 10 Sep 2026 07:08:20 +0000 Subject: [PATCH 1/2] fix(objectql): name the withheld read-only key when a hook faults reaching through it Since #16344 the update path hides a caller-supplied static `readonly` value from `before*` hooks. A hook reaching THROUGH such a key dereferences `undefined` and throws, and a `body` hook's default `onError: abort` refuses the caller's whole write. The refusal is correct and is not touched here. The diagnostic was the defect. Measured at both doors before this change: direct SandboxError: hook 'guard_task_body' threw: TypeError: cannot set property 'who' of undefined REST 500 {"error":"Internal server error","code":"INTERNAL_ERROR"} The REST reading is the worse of the two and it is the door an author authors against: a leading `TypeError:` is correctly classified as a script fault and sanitised (#7543), so nothing reached the author -- not the key, not the reason, not the remedy. The engine is the only actor that can tell "the platform took this away" from "nobody sent it": the sandbox face is a plain JSON snapshot, so a withheld key is simply absent there. So the explanation is composed at the hide pass, and all three `beforeUpdate` dispatch sites inside the hide window share one wrapper. Nothing is marshalled onto the sandbox face. The composed error names the withheld key, says the platform withheld it because the field is `readonly: true`, points at `ctx.previous`, carries the original fault text, and declares HTTP 400 -- which is what carries it past `mapDataError`'s sanitiser instead of into a blank 500. No error code is registered and no published payload gains a key. Three declines keep it from repeating the card's own defect in the opposite direction: nothing withheld, an authored refusal, and a non-object throw all rethrow the original error untouched. Claude-Session: https://claude.ai/code/session_01XTBcV7zZHmokdyQgXjbyEU Co-authored-by: Claude --- .../hook-withheld-readonly-key-diagnostic.md | 37 +++ .../src/engine-readonly-hook-input.test.ts | 177 ++++++++++++++ packages/objectql/src/engine.ts | 19 +- .../src/hook-withheld-readonly-fault.test.ts | 122 ++++++++++ .../src/hook-withheld-readonly-fault.ts | 218 ++++++++++++++++++ ...ck-readonly-provenance.integration.test.ts | 50 +++- 6 files changed, 611 insertions(+), 12 deletions(-) create mode 100644 .changeset/hook-withheld-readonly-key-diagnostic.md create mode 100644 packages/objectql/src/hook-withheld-readonly-fault.test.ts create mode 100644 packages/objectql/src/hook-withheld-readonly-fault.ts diff --git a/.changeset/hook-withheld-readonly-key-diagnostic.md b/.changeset/hook-withheld-readonly-key-diagnostic.md new file mode 100644 index 0000000000..bd3a4ae012 --- /dev/null +++ b/.changeset/hook-withheld-readonly-key-diagnostic.md @@ -0,0 +1,37 @@ +--- +"@objectstack/objectql": patch +--- + +fix(objectql): a hook that faults reaching through a withheld read-only key now names the key, says the platform withheld it, and points at `ctx.previous` (#17219) + +Since #16344 the update path hides a caller-supplied static `readonly` value from `before*` hooks. A hook body that reaches **through** such a key — `ctx.input.locked_meta.who = 'hook'`, where `locked_meta` is a caller-supplied read-only `json` column — therefore dereferences `undefined` and throws, and a `body` hook's default `onError: abort` refuses the caller's whole write. + +**The refusal is correct and is unchanged.** What it replaced is a write that succeeded while persisting a value derived from the caller's forgery, and #16344 exists to close exactly that route. What this fixes is the diagnostic. Measured before this change, at both doors: + +``` +direct SandboxError: hook 'guard_task_body' threw: + TypeError: cannot set property 'who' of undefined +REST 500 {"error":"Internal server error","code":"INTERNAL_ERROR"} +``` + +The REST reading is the one that matters, and it is the worse of the two: a leading `TypeError:` is correctly classified as a script fault and sanitised (#7543), so an author was told nothing at all — not which key, not that the platform had taken it away, not what to read instead. + +### Who is affected + +Anyone whose `beforeUpdate` hook reads a read-only field that the caller may also send. The write was already being refused; only the message changes. A hook that needs the stored value reads it from **`ctx.previous.`** — the same remedy PR #17195's changeset documents. + +### What the message says now + +``` +A `beforeUpdate` hook faulted while `locked_meta` was withheld from it. That field is +`readonly: true`, and the engine withholds a caller-supplied value for a read-only field +from `beforeUpdate` hooks, so `ctx.input.locked_meta` reads `undefined` — withheld by the +platform, not missing by accident. Read the stored value from `ctx.previous.locked_meta` +instead. Original fault: TypeError: cannot set property 'who' of undefined +``` + +The error declares **HTTP 400**, which is what carries it past the script-fault sanitiser onto the same "message verbatim" channel a body's own authored refusal already rides; REST callers who previously saw `500 INTERNAL_ERROR` for this case now see 400 with the text above. The original fault is carried inside the message rather than replaced. + +### Deliberate limits + +No new error code is registered and no key is added to any published payload — a dedicated `ERROR_CODE_LEDGER` entry for this refusal is a separate decision. The explanation claims only what is knowable at the seam: *faulted while these keys were withheld*, never a proven cause. An **authored** refusal (`throw new Error('…')`) is never rewritten, and a crash on an operation where nothing was withheld passes through untouched. diff --git a/packages/objectql/src/engine-readonly-hook-input.test.ts b/packages/objectql/src/engine-readonly-hook-input.test.ts index 85b7e9d940..8186577699 100644 --- a/packages/objectql/src/engine-readonly-hook-input.test.ts +++ b/packages/objectql/src/engine-readonly-hook-input.test.ts @@ -407,3 +407,180 @@ describe('#16344 — caller-forged readonly values are hidden from beforeUpdate' expect(afterSubmitted).toEqual([{ id: 'kpi_1', actual_value: 380, target_value: 1 }]); }); }); + +/** + * [#17219] The OTHER half of the same hide pass: what an author is told when a + * hook reaches THROUGH a key #16344 withheld. + * + * ⛔ The refusal itself is not under test here and is not moved: a body's + * default `onError: abort` refuses the caller's whole write, and what that + * replaced is a write that succeeded while persisting a value derived from the + * caller's forgery. Every case below re-asserts that the row is untouched, so a + * future repair of the DIAGNOSTIC cannot quietly restore the old write. + * + * Measured on `origin/main` `501959b72a` before this fix, both doors: + * + * direct SandboxError: hook '…' threw: TypeError: cannot set property 'who' of undefined + * REST 500 {"error":"Internal server error","code":"INTERNAL_ERROR"} + * + * The REST reading is the one that decides the shape of the fix: a leading + * `TypeError:` is correctly classified as a crash (#7543) and sanitised, so at + * the door an author actually authors against, the old behaviour said nothing + * at all — not the key, not the reason, not the remedy. + * + * A code hook is the subject rather than a sandboxed body deliberately: the + * dispatch sites and the hide pass are what this card wraps, and they are + * common to both. The sandbox spelling is measured end to end, through a real + * QuickJS, in `runtime`'s `hook-input-writeback-readonly-provenance` suite. + */ +describe('#17219 — a hook that faults reaching THROUGH a withheld readonly key names it', () => { + let engine: ObjectQL; + let storeFor: ReturnType['storeFor']; + + const OBJECT = 'guard_task'; + + /** Reaches through the read-only `locked_meta`, which the hide pass removed. */ + const reachThrough = async (ctx: any) => { + ctx.input.data.locked_meta.who = 'hook'; + }; + + async function boot( + hook: (ctx: any) => Promise, + opts?: { unscopedMulti?: boolean }, + ) { + engine = new ObjectQL({ logger: { + warn() {}, debug() {}, info() {}, error() {}, trace() {}, fatal() {}, + child() { return this as any; }, + } as any }); + const d = makeDriver(); + storeFor = d.storeFor; + engine.registerDriver(d.driver, true); + await engine.init(); + engine.registry.registerObject({ + name: OBJECT, + fields: { + status: { type: 'text' }, + bucket: { type: 'text' }, + locked_meta: { type: 'json', readonly: true }, + }, + } as any); + storeFor(OBJECT).set('t1', { + id: 't1', status: 'open', bucket: 'b1', locked_meta: { seeded: true }, + }); + engine.registerHook('beforeUpdate', hook, { + object: OBJECT, + priority: 50, + ...(opts?.unscopedMulti ? { dispatchUnscopedMultiWrite: true } : {}), + } as any); + } + + const row = () => storeFor(OBJECT).get('t1'); + + /** Every requirement the card places on the message, asserted as one set. */ + const expectActionable = (err: any) => { + // ① the withheld KEY is named — the old message named nothing. + expect(err.message).toContain('`locked_meta`'); + // ② WITHHELD BY THE PLATFORM, not absent by accident. + expect(err.message).toContain('withheld by the platform, not missing by accident'); + // ③ the documented remedy, reachable from the message itself. + expect(err.message).toContain('`ctx.previous.locked_meta`'); + // ④ and it reaches the author at the REST door: `declaredHttpStatus` reads + // this, and without it the body is the sanitised 500. + expect(err.status).toBe(400); + // The original fault is carried through, never swallowed. + expect(err.message).toMatch(/cannot set propert|Cannot set propert/); + }; + + it('BY-ID: the refusal stands and now names the key, the reason and the remedy', async () => { + await boot(reachThrough); + + const err = await engine.update(OBJECT, { + id: 't1', status: 'done', locked_meta: { who: 'caller' }, + } as any).then(() => null, (e) => e); + + expect(err).toBeTruthy(); + expectActionable(err); + // ⛔ RULING 1, re-pinned: the write is still refused WHOLE. Neither the + // forged read-only value nor the writable `status` reached the row. + expect(row().locked_meta).toEqual({ seeded: true }); + expect(row().status).toBe('open'); + }); + + it('PREDICATE: the per-row dispatch site answers on the same terms', async () => { + await boot(reachThrough); + + const err = await engine.update( + OBJECT, + { status: 'done', locked_meta: { who: 'caller' } } as any, + { multi: true, where: { bucket: 'b1' } } as any, + ).then(() => null, (e) => e); + + expect(err).toBeTruthy(); + expectActionable(err); + expect(row().locked_meta).toEqual({ seeded: true }); + expect(row().status).toBe('open'); + }); + + it('UNSCOPED-MULTI: the third dispatch site inside the hide window answers too', async () => { + await boot(reachThrough, { unscopedMulti: true }); + + const err = await engine.update( + OBJECT, + { status: 'done', locked_meta: { who: 'caller' } } as any, + { multi: true } as any, + ).then(() => null, (e) => e); + + expect(err).toBeTruthy(); + expectActionable(err); + expect(row().locked_meta).toEqual({ seeded: true }); + }); + + it('CONTROL — nothing withheld: an ordinary crash keeps its own raw words', async () => { + // The caller sends NO read-only key, so the hide pass never runs and + // `readonlyHiddenFromHooks` stays unset. The hook still faults (the column + // is simply absent from this payload), and that fault must pass through + // untouched: the diagnostic is tied to the WITHHOLDING, not to any crash + // that happens to occur on an object with a read-only field. Without this + // leg the case above would pass just as well for a wrapper that rewrote + // every hook error it saw. + await boot(reachThrough); + + const err = await engine.update(OBJECT, { id: 't1', status: 'done' } as any) + .then(() => null, (e) => e); + + expect(err).toBeTruthy(); + expect(err.message).not.toContain('withheld by the platform'); + expect(err.status).toBeUndefined(); + expect(row().status).toBe('open'); + }); + + it('CONTROL — an AUTHORED refusal is never rewritten, even while a key is withheld', async () => { + // The regression this guards is the card's own defect aimed the other way: + // `mapDataError` serves an authored message to the caller verbatim, so + // overwriting it would destroy the author's words to explain a key they + // never asked about. + await boot(async () => { throw new Error('仍有未结清的发票'); }); + + const err = await engine.update(OBJECT, { + id: 't1', status: 'done', locked_meta: { who: 'caller' }, + } as any).then(() => null, (e) => e); + + expect(err.message).toBe('仍有未结清的发票'); + expect(err.status).toBeUndefined(); + expect(row().status).toBe('open'); + }); + + it('CONTROL — a hook that does NOT fault still runs, and the strip still refuses the forgery', async () => { + // The over-narrowing guard: if the wrapper had broken the dispatch, every + // case above would pass for the wrong reason. Here the same withheld key is + // in play, the hook completes, and the write lands MINUS the forgery. + await boot(async (ctx: any) => { ctx.input.data.status = 'hooked'; }); + + await engine.update(OBJECT, { + id: 't1', status: 'done', locked_meta: { who: 'caller' }, + } as any); + + expect(row().status).toBe('hooked'); + expect(row().locked_meta).toEqual({ seeded: true }); + }); +}); diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index e54df038b6..efa1a3ba88 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -214,6 +214,11 @@ import { evaluateValidationRules, needsPriorRecord, stripReadonlyWhenFields, str // SAME value. Armed and sealed in `update()`; the module owns the argument for // why neither end may move. import { recordHookPayloadWrites } from './hook-write-provenance.js'; +// [#17219] The hide pass's other half: when a hook faults reaching THROUGH a +// key that pass withheld, this names the key, says the platform withheld it, +// and points at `ctx.previous` — the module owns the measurement and the +// reason the explanation cannot be composed any further downstream. +import { dispatchHooksExplainingWithheldReadonly } from './hook-withheld-readonly-fault.js'; import { divergingHookPayloadKeys, MultiUpdateHookKeyDivergenceError, @@ -11701,7 +11706,13 @@ export class ObjectQL implements IObjectQLEngine { // permanently true here: it states the invariant, and the invariant // outlives this call site. if (priorRecord) hookContext.previous = coerceBooleanFields(updateSchema as any, priorRecord as any) as any; - await this.triggerHooks('beforeUpdate', hookContext); + // [#17219] All three `beforeUpdate` dispatch sites inside the hide + // window share one wrapper, so a hook that faults reaching THROUGH a + // key this pass withheld names that key instead of surfacing the + // platform's own contract enforcement as the author's crash. It + // rethrows the original error untouched on every other path. + await dispatchHooksExplainingWithheldReadonly(readonlyHiddenFromHooks, 'beforeUpdate', + () => this.triggerHooks('beforeUpdate', hookContext)); // The retired lever, refused. Everything above — `previous`, and // below it the `readonlyWhen` strip and every validation rule — was // computed against the row the ladder chose. @@ -11772,7 +11783,8 @@ export class ObjectQL implements IObjectQLEngine { // predicate is unscoped. const rawWhere = (hookContext.input.options as { where?: unknown } | undefined)?.where; if (rawWhere === undefined || rawWhere === null) { - await this.dispatchUnscopedMultiWriteHooks('beforeUpdate', object, hookContext); + await dispatchHooksExplainingWithheldReadonly(readonlyHiddenFromHooks, 'beforeUpdate', + () => this.dispatchUnscopedMultiWriteHooks('beforeUpdate', object, hookContext)); } const preOpts = this.buildDriverOptions(object, opCtx.context, hookContext.input.options as any); readPriorRows = async () => { @@ -11804,7 +11816,8 @@ export class ObjectQL implements IObjectQLEngine { // [D1] Zero matched rows is zero dispatches — a batch that // changed nothing is not a record change. if (perRowBeforeHooks && rows.length > 0) { - await this.dispatchPerRowBeforeHooks(object, 'beforeUpdate', rows, hookContext); + await dispatchHooksExplainingWithheldReadonly(readonlyHiddenFromHooks, 'beforeUpdate', + () => this.dispatchPerRowBeforeHooks(object, 'beforeUpdate', rows, hookContext)); } } } diff --git a/packages/objectql/src/hook-withheld-readonly-fault.test.ts b/packages/objectql/src/hook-withheld-readonly-fault.test.ts new file mode 100644 index 0000000000..823da17ed6 --- /dev/null +++ b/packages/objectql/src/hook-withheld-readonly-fault.test.ts @@ -0,0 +1,122 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#17219] The composer's contract, at the seam rather than through a driver. + * + * The three DECLINE conditions carry as much weight as the accept case, and for + * the reason the card is about: a diagnostic that fires on the wrong error is + * the same defect this fixes, aimed the other way. So each decline is asserted + * with the SAME error the accept case uses wherever the condition permits it, + * which is what makes the assertions about the condition rather than about the + * error. + */ + +import { describe, it, expect } from 'vitest'; +import { + withheldReadonlyHookFault, + dispatchHooksExplainingWithheldReadonly, + HookWithheldReadonlyFaultError, +} from './hook-withheld-readonly-fault.js'; + +/** The real shape `quickjs-runner.ts` throws — name `SandboxError`, native prefix kept on `innerMessage`. */ +class SandboxErrorLike extends Error { + innerMessage?: string; + constructor(message: string, innerMessage?: string) { + super(message); + this.name = 'SandboxError'; + this.innerMessage = innerMessage; + } +} + +/** Measured verbatim on `origin/main` `501959b72a` — see the module header. */ +const REAL_CRASH = () => + new SandboxErrorLike( + "hook 'guard_task_body' threw: TypeError: cannot set property 'who' of undefined", + "TypeError: cannot set property 'who' of undefined", + ); + +const HIDDEN = { locked_meta: { who: 'caller' } }; + +describe('#17219 withheldReadonlyHookFault', () => { + it('names the key, says the platform withheld it, and points at ctx.previous', () => { + const out = withheldReadonlyHookFault(REAL_CRASH(), HIDDEN, 'beforeUpdate'); + expect(out).toBeInstanceOf(HookWithheldReadonlyFaultError); + const msg = out!.message; + // ① the withheld KEY is named … + expect(msg).toContain('`locked_meta`'); + // ② … as WITHHELD BY THE PLATFORM, not absent by accident … + expect(msg).toContain('withheld by the platform, not missing by accident'); + expect(msg).toContain('`readonly: true`'); + // ③ … and the documented remedy is reachable from the message itself. + expect(msg).toContain('`ctx.previous.locked_meta`'); + // The original fault is carried, never replaced: an author debugging the + // body still gets the line that actually threw. + expect(msg).toContain("TypeError: cannot set property 'who' of undefined"); + expect(out!.withheldKeys).toEqual(['locked_meta']); + expect(out!.cause).toBeDefined(); + }); + + it('declares 400 — the whole envelope change, and what makes the message reachable', () => { + // Measured: without a declared status `mapDataError` answers + // `UNCLASSIFIED_FAULT` (500, sanitised body) and the message above never + // reaches the author at the REST door. `packages/rest`'s + // `declaredHttpStatus` reads exactly this property. + expect(withheldReadonlyHookFault(REAL_CRASH(), HIDDEN, 'beforeUpdate')!.status).toBe(400); + }); + + it('names every withheld key when the pass hid more than one', () => { + const out = withheldReadonlyHookFault( + REAL_CRASH(), { locked_meta: {}, locked_note: 'CALLER' }, 'beforeUpdate', + ); + expect(out!.message).toContain('`locked_meta`, `locked_note`'); + expect(out!.message).toContain('`ctx.previous.locked_meta`, `ctx.previous.locked_note`'); + expect(out!.withheldKeys).toEqual(['locked_meta', 'locked_note']); + }); + + it('DECLINES when nothing was withheld — an unrelated crash keeps its own words', () => { + expect(withheldReadonlyHookFault(REAL_CRASH(), undefined, 'beforeUpdate')).toBeUndefined(); + expect(withheldReadonlyHookFault(REAL_CRASH(), {}, 'beforeUpdate')).toBeUndefined(); + }); + + it('DECLINES on an AUTHORED refusal, so a business message is never rewritten', () => { + // The one that would be a real regression: `mapDataError` serves this text + // to the caller verbatim at 400, and overwriting it would destroy the + // author's own words while a readonly key happened to be hidden. + const authored = new SandboxErrorLike("hook 'guard' threw: 仍有未结清的发票", '仍有未结清的发票'); + expect(withheldReadonlyHookFault(authored, HIDDEN, 'beforeUpdate')).toBeUndefined(); + // And the non-sandboxed spelling of the same thing. + expect(withheldReadonlyHookFault(new Error('仍有未结清的发票'), HIDDEN, 'beforeUpdate')).toBeUndefined(); + }); + + it('ACCEPTS a CODE hook crash, which carries the native name in `name` instead', () => { + const out = withheldReadonlyHookFault( + new TypeError("Cannot set properties of undefined (setting 'who')"), HIDDEN, 'beforeUpdate', + ); + expect(out).toBeInstanceOf(HookWithheldReadonlyFaultError); + expect(out!.message).toContain('`locked_meta`'); + }); + + it('declines on a non-object throw rather than fabricating a shape', () => { + expect(withheldReadonlyHookFault('boom', HIDDEN, 'beforeUpdate')).toBeUndefined(); + }); +}); + +describe('#17219 dispatchHooksExplainingWithheldReadonly', () => { + it('is transparent on success', async () => { + await expect(dispatchHooksExplainingWithheldReadonly(HIDDEN, 'beforeUpdate', async () => 'ok')) + .resolves.toBe('ok'); + }); + + it('rethrows the ORIGINAL error object when the composer declines', async () => { + const authored = new Error('仍有未结清的发票'); + await expect( + dispatchHooksExplainingWithheldReadonly(HIDDEN, 'beforeUpdate', async () => { throw authored; }), + ).rejects.toBe(authored); + }); + + it('replaces an anonymous crash with the named refusal', async () => { + await expect( + dispatchHooksExplainingWithheldReadonly(HIDDEN, 'beforeUpdate', async () => { throw REAL_CRASH(); }), + ).rejects.toBeInstanceOf(HookWithheldReadonlyFaultError); + }); +}); diff --git a/packages/objectql/src/hook-withheld-readonly-fault.ts b/packages/objectql/src/hook-withheld-readonly-fault.ts new file mode 100644 index 0000000000..4c6f3360b9 --- /dev/null +++ b/packages/objectql/src/hook-withheld-readonly-fault.ts @@ -0,0 +1,218 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#17219] Name the withheld key when a `before*` hook faults reaching THROUGH + * one — instead of letting the platform's own contract enforcement surface as + * the author's crash. + * + * ## What was measured broken + * + * Since #16344 the update path HIDES a caller-supplied static `readonly` value + * from `before*` hooks (`readonlyHiddenFromHooks`, engine.ts). The hook's view + * of that key is a plain `undefined`, so a body reaching through it — + * `ctx.input.locked_meta.who = 'hook'` — throws, and a `body` hook's default + * `onError: abort` refuses the caller's whole write. + * + * ⛔ **The refusal is correct and this module does not touch it.** What it + * replaced is a write that SUCCEEDED while persisting a value derived from the + * caller's forgery, and #16344 exists to close exactly that laundering route. + * The defect is the DIAGNOSTIC. Measured end to end on `origin/main` + * `501959b72a`, both doors: + * + * ``` + * direct SandboxError: hook 'guard_task_body' threw: + * TypeError: cannot set property 'who' of undefined + * REST 500 {"error":"Internal server error","code":"INTERNAL_ERROR"} + * ``` + * + * The REST reading is the WORSE of the two the card allowed for, and it is the + * door an author actually authors against: `error-response.ts`'s + * `isScriptFaultMessage` correctly classifies a leading `TypeError:` as a crash + * (#7543) and sanitises it, so the author is told nothing at all — not which + * key, not why, not what to read instead. A control in the same measurement + * fires: a body that throws an authored `Error` still answers 400 with its own + * words. + * + * ## Why the fault has to be explained HERE + * + * The knowledge "this key was withheld BY THE PLATFORM because it is + * `readonly`" exists only in the engine. Downstream nothing can reconstruct it: + * the sandbox face is a plain JSON snapshot (`unwrapProxyToPlain`, then a JSON + * marshal), so the key is simply absent, and *absent because the caller sent it + * and the platform took it away* is indistinguishable there from *absent + * because nobody sent it*. That distinction is precisely what the card requires + * the message to state, so the explanation is composed where the hide pass is. + * + * ⛔ Not by marshalling `ctx.submitted` onto the sandbox face: that face is + * assembled key by key, `dispatch.scope` is the standing precedent for the + * assembly discipline, and the shape was measured and refused on its merits in + * PR #17195. Nothing here adds a key to any authoring face or to any wire + * payload. + * + * ## The classification this restores + * + * The repo already draws a REFUSAL / SCRIPT-FAULT line + * (`rest-hook-refusal-classification.test.ts`, + * `rest-hook-script-fault-envelope.test.ts`, `actions-fault-vs-rejection.test.ts`). + * Mechanically this IS a crash, so the fault channel is not the wrong pipe — + * what is wrong is the CAUSAL ATTRIBUTION: one of the platform's own contract + * enforcements is reported to the author as a bug in their code. Naming the + * withheld key turns an anonymous crash back into a named refusal, which is the + * side of that line it belongs on. + * + * ## What this deliberately does NOT do + * + * ⛔ It registers no error code. A dedicated `ERROR_CODE_LEDGER` entry would be + * a new PUBLISHED member and is a separate, declared decision; the 400 answered + * here rides the existing "message verbatim, no `code`" channel that a body's + * own `throw new Error(...)` already uses (measured, `mapDataError`). ⛔ And it + * does not reuse `ERR_READONLY_FIELD_REJECTED`: that code names the + * `strictReadonlyWrites` refusal and carries `drops` as part of its contract, so + * borrowing it would make the error lie about which refusal happened. + */ + +/** + * The ECMA-262 native error constructors, plus SpiderMonkey's `InternalError` + * which QuickJS also raises. Same structural rule — and same deliberate + * omission of `Error:` — that `packages/rest`'s `isScriptFaultMessage` applies + * one door down: a body's plain `Error` is the documented way to AUTHOR a + * refusal, so it is never a crash and its words are never rewritten here. + * + * ⛔ Kept as its own copy rather than imported: `@objectstack/objectql` does not + * depend on `@objectstack/rest`, and it must not start to for a regex. + */ +const NATIVE_ERROR_NAME_RE = + /^(?:Type|Reference|Range|Syntax|URI|Eval|Internal|Aggregate)Error(?::|$)/; + +/** + * Did the hook CRASH, as opposed to deliberately refusing? + * + * Two spellings, because a hook reaches this engine by two routes and they + * carry the native name in different slots: + * + * - a CODE hook throws the real thing, so `err.name` is `'TypeError'`; + * - a SANDBOXED body's throw is wrapped by `quickjs-runner.ts` into a + * `SandboxError` whose `name` is `'SandboxError'` and whose `innerMessage` + * keeps the `TypeError: …` prefix (`userFacingMessage` strips only a leading + * `Error: `). + * + * A hook that threw an authored `Error` — sandboxed or not — answers `false` on + * both, which is what keeps this from overwriting a business message that + * `mapDataError` would otherwise serve to the caller verbatim. + */ +function isScriptCrash(err: unknown): boolean { + if (!err || typeof err !== 'object') return false; + const e = err as { name?: unknown; innerMessage?: unknown }; + if (typeof e.name === 'string' && NATIVE_ERROR_NAME_RE.test(e.name)) return true; + return typeof e.innerMessage === 'string' && NATIVE_ERROR_NAME_RE.test(e.innerMessage.trim()); +} + +/** Read a message off anything a hook may have thrown, without assuming a shape. */ +function messageOf(err: unknown): string { + if (err && typeof err === 'object') { + const e = err as { message?: unknown }; + if (typeof e.message === 'string' && e.message) return e.message; + } + return String(err); +} + +/** + * A `before*` hook faulted while the engine was withholding caller-supplied + * read-only values from it. + * + * `status` is the whole envelope change and it is deliberate: without it + * `mapDataError` answers `UNCLASSIFIED_FAULT` — `500 INTERNAL_ERROR`, the + * sanitised body — and the message composed below never reaches the author at + * the door they author against. Declaring 400 puts it on the same + * message-verbatim channel a body's own authored refusal already rides + * (measured), and 400 is the truthful status: the trigger is a value THE CALLER + * supplied for a field they may not write. + * + * ⛔ No `code`. See the module header — that is a published member and a + * separate decision. + */ +export class HookWithheldReadonlyFaultError extends Error { + /** Served by `packages/rest`'s `declaredHttpStatus`. */ + readonly status = 400; + /** The read-only keys the engine withheld from this hook, in payload order. */ + readonly withheldKeys: readonly string[]; + constructor(message: string, withheldKeys: readonly string[], options?: { cause?: unknown }) { + super(message, options as ErrorOptions); + this.name = 'HookWithheldReadonlyFaultError'; + this.withheldKeys = withheldKeys; + } +} + +const fence = (k: string) => `\`${k}\``; + +/** + * Compose the replacement error, or answer `undefined` to leave the original + * throw exactly as it is. + * + * Three conditions, all required, and each one is a way this could otherwise + * repeat the card's own harm in the opposite direction — a confidently wrong + * attribution: + * + * 1. the engine really did withhold something ON THIS OPERATION. An empty or + * absent map means no hide pass ran and there is nothing to explain; + * 2. the hook CRASHED rather than refused, so an authored business message is + * never rewritten; + * 3. there is at least one named key to report. + * + * ⚠️ What it deliberately does NOT claim is CAUSATION. Nothing at this seam can + * prove the crash was the dereference of a withheld key rather than an + * unrelated bug in the body, so the sentence is anchored on what IS known — + * "faulted while these keys were withheld from it" — and the original fault text + * is carried through verbatim rather than replaced. Asserting a cause we cannot + * establish would relocate this card's defect instead of fixing it. + */ +export function withheldReadonlyHookFault( + err: unknown, + withheld: Record | undefined, + event: string, +): HookWithheldReadonlyFaultError | undefined { + if (!withheld) return undefined; + const keys = Object.keys(withheld); + if (keys.length === 0) return undefined; + if (!isScriptCrash(err)) return undefined; + + const one = keys.length === 1; + const named = keys.map(fence).join(', '); + const message = + `A \`${event}\` hook faulted while ${named} ${one ? 'was' : 'were'} withheld from it. ` + + `${one ? 'That field is' : 'Those fields are'} \`readonly: true\`, and the engine withholds a ` + + `caller-supplied value for a read-only field from \`${event}\` hooks, so ` + + `${keys.map((k) => `\`ctx.input.${k}\``).join(', ')} ` + + `${one ? 'reads' : 'read'} \`undefined\` — withheld by the platform, not missing by accident. ` + + `Read the stored ${one ? 'value' : 'values'} from ` + + `${keys.map((k) => `\`ctx.previous.${k}\``).join(', ')} instead. ` + + `Original fault: ${messageOf(err)}`; + + return new HookWithheldReadonlyFaultError(message, keys, { cause: err }); +} + +/** + * Run a hook dispatch inside the hide window, replacing an anonymous crash with + * the named refusal above. + * + * A thunk rather than a `try`/`catch` written out at each dispatch site: the + * update path dispatches `before*` hooks from three places between the hide and + * the hand-back (by-id, unscoped-multi, per-row), and one shared wrapper is what + * keeps the three from drifting into three different answers — the divergence + * "both call sites" is the standing shape for in this file's neighbours. + * + * ⛔ Rethrows the ORIGINAL error whenever {@link withheldReadonlyHookFault} + * declines, so every path that does not meet all three conditions is + * byte-identical to having no wrapper at all. + */ +export async function dispatchHooksExplainingWithheldReadonly( + withheld: Record | undefined, + event: string, + run: () => Promise, +): Promise { + try { + return await run(); + } catch (err) { + throw withheldReadonlyHookFault(err, withheld, event) ?? err; + } +} diff --git a/packages/runtime/src/sandbox/hook-input-writeback-readonly-provenance.integration.test.ts b/packages/runtime/src/sandbox/hook-input-writeback-readonly-provenance.integration.test.ts index 84a1d784e1..f4ba6adf89 100644 --- a/packages/runtime/src/sandbox/hook-input-writeback-readonly-provenance.integration.test.ts +++ b/packages/runtime/src/sandbox/hook-input-writeback-readonly-provenance.integration.test.ts @@ -362,7 +362,7 @@ describe('#14760 — an untouched readonly key is not laundered by the sandbox w expect(after.touched_by).toBe('hook'); }, 60000); - it('[#16344] a body that reads a CALLER-supplied readonly key no longer sees it — and says so instead of deriving from it', async () => { + it('[#16344/#17219] a body reaching THROUGH a caller-supplied readonly key is refused — and the refusal names the key, the reason and the remedy', async () => { // The old path of the control above, kept and re-judged rather than // deleted, because the behaviour change is the point of the card and this // is the one place in the repo that measures it end to end through a REAL @@ -376,22 +376,54 @@ describe('#14760 — an untouched readonly key is not laundered by the sandbox w // // ⚠️ That fault is a REFUSAL, not a silent no-op: a `body` hook's default // `onError` is `abort`, so the caller's whole write is rejected and the row - // is untouched. Loud beats silent — but the message is a raw `TypeError` - // from the app's own dereference, which names nothing an author can act on. - // Recorded here rather than smoothed over: a body that needs the caller's - // submission has no `ctx.submitted` (it is deliberately not marshalled onto - // the sandbox face), and its supported source for a derived column is - // `ctx.previous`. + // is untouched. Loud beats silent — and #17219 supplied the second half the + // refusal was missing. Measured here before that card, through this very + // harness, at both doors: + // + // direct SandboxError: hook 'guard_task_body' threw: + // TypeError: cannot set property 'who' of undefined + // REST 500 {"error":"Internal server error","code":"INTERNAL_ERROR"} + // + // The REST reading is the worse one and it is the door an author authors + // against: a leading `TypeError:` is correctly classified as a crash + // (#7543) and sanitised, so the author was told nothing at all. The engine + // now names the withheld key at the dispatch site — it is the only actor + // that can tell "the platform took this away" from "nobody sent it". + // + // ⛔ Still no `ctx.submitted` on the sandbox face: that face is assembled + // key by key and the shape was measured and refused in PR #17195. The + // supported source for a derived column is `ctx.previous`, which is what + // the message now says. const { engine, driver } = await boot(WRITES_THROUGH_SOURCE); await seed(driver); const seeded = await row(engine); - await expect(engine.update('guard_task', { + const err: any = await engine.update('guard_task', { id: seeded.id, status: 'done', locked_meta: { who: 'caller' }, locked_note: 'CALLER', - } as any)).rejects.toThrow(/locked_meta|cannot set property 'who' of undefined/); + } as any).then(() => null, (e) => e); + + // ⛔ The ENVELOPE, not `toThrow`: a bare "it threw" passes for the raw + // `TypeError` this card exists to replace, which is how the old assertion + // here stayed green through the whole defect. + expect(err).toBeTruthy(); + // ① the withheld key, ② withheld BY THE PLATFORM rather than absent by + // accident, ③ the documented remedy — the three the card requires. + expect(err.message).toContain('`locked_meta`'); + expect(err.message).toContain('withheld by the platform, not missing by accident'); + expect(err.message).toContain('`ctx.previous.locked_meta`'); + // ④ and the status that carries all of the above past `mapDataError`'s + // script-fault sanitiser instead of into a blank 500. + expect(err.status).toBe(400); + // ⛔ Deliberately NOT a `code`: this rides the existing "message verbatim, + // no code" 400 channel a body's own authored refusal already uses. A + // dedicated ledger entry is a new PUBLISHED member and a separate decision. + expect(err.code).toBeUndefined(); + // The original fault survives inside the message — an author debugging the + // body still gets the line that actually threw. + expect(err.message).toContain("cannot set property 'who' of undefined"); const after = await row(engine); // ⭐ The verdict that matters: NOTHING the caller sent reached the row — From 94193545fd0d378ccca1ceddc9038f76c12cadbb Mon Sep 17 00:00:00 2001 From: Claude Date: Thu, 10 Sep 2026 07:23:48 +0000 Subject: [PATCH 2/2] fix(objectql): declare `cause` on the fault error instead of passing it through `super()` `pnpm --filter @objectstack/objectql typecheck` exited 2 on the previous commit: src/hook-withheld-readonly-fault.ts(140,20): error TS2554: Expected 0-1 arguments, but got 2. src/hook-withheld-readonly-fault.ts(140,31): error TS2304: Cannot find name 'ErrorOptions'. This repo compiles against `lib: ES2020`, where `Error` has neither a `cause` member nor the `ErrorOptions` constructor overload that carries one. The precedent is in this same package: `duplicate-record-error.ts` declares `readonly cause: unknown` on the class and assigns it by hand, and its comment already records why -- an undeclared assignment would be invisible to every TypeScript consumer of the field. Followed verbatim rather than reinvented. No behaviour change: the same original error is attached under the same name, and both suites plus the reverse verification were re-run against a forced rebuild after the edit. Claude-Session: https://claude.ai/code/session_01XTBcV7zZHmokdyQgXjbyEU Co-authored-by: Claude --- .../objectql/src/hook-withheld-readonly-fault.ts | 14 +++++++++++++- 1 file changed, 13 insertions(+), 1 deletion(-) diff --git a/packages/objectql/src/hook-withheld-readonly-fault.ts b/packages/objectql/src/hook-withheld-readonly-fault.ts index 4c6f3360b9..a1cd423cc4 100644 --- a/packages/objectql/src/hook-withheld-readonly-fault.ts +++ b/packages/objectql/src/hook-withheld-readonly-fault.ts @@ -136,10 +136,22 @@ export class HookWithheldReadonlyFaultError extends Error { readonly status = 400; /** The read-only keys the engine withheld from this hook, in payload order. */ readonly withheldKeys: readonly string[]; + /** + * The hook's original throw, whole. + * + * DECLARED on the class and assigned by hand rather than passed through the + * constructor, for the reason `duplicate-record-error.ts` already writes down + * one file over: this repo compiles against `lib: ES2020`, where `Error` has + * neither a `cause` member nor an `ErrorOptions` overload to carry one — so + * the two-argument `super()` does not compile, and an undeclared assignment + * would be invisible to every TypeScript consumer of the field. + */ + readonly cause: unknown; constructor(message: string, withheldKeys: readonly string[], options?: { cause?: unknown }) { - super(message, options as ErrorOptions); + super(message); this.name = 'HookWithheldReadonlyFaultError'; this.withheldKeys = withheldKeys; + this.cause = options?.cause; } }