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..a1cd423cc4 --- /dev/null +++ b/packages/objectql/src/hook-withheld-readonly-fault.ts @@ -0,0 +1,230 @@ +// 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[]; + /** + * 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); + this.name = 'HookWithheldReadonlyFaultError'; + this.withheldKeys = withheldKeys; + this.cause = options?.cause; + } +} + +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 —