From 6c568d6bb98badadc39fdea621b909ccc7ba83e3 Mon Sep 17 00:00:00 2001 From: Claude Date: Wed, 26 Aug 2026 00:07:07 +0000 Subject: [PATCH] fix(objectql,runtime): route delete and defineProperty into the row a hook persists MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit `delete ctx.input.x` in a hook was a no-op on both execution paths while an assignment on the same object in the same call landed. In-process: `installFlatInput`'s flat-record Proxy trapped get/set/has/ownKeys/ getOwnPropertyDescriptor but not `deleteProperty`, so the delete fell through to the WRAPPER one level above `data` and returned true. `defineProperty` had the same gap and the worse shape — the `get` trap's fall-through read the value back, so the read-back CONFIRMED a write the record never received. Both now route into `data`, like `set`. Sandboxed: `applyMutationsToInput` wrote a QuickJS body's mutations home with `Object.assign`, which cannot represent a removal. Keys the VM deleted are now diffed against the entry snapshot, filtered through the same JSON lens the sandbox boundary uses so a key that never crossed cannot be destroyed on its absence. Both halves land together: closing one alone would make the same authored `delete` behave differently in-process than in the sandbox. Card: #12277 Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_01W6HFzyH98W1YaQXhJUJt6o --- .changeset/hook-input-delete-lands.md | 95 ++++++++++ .../src/hook-input-mutation-traps.test.ts | 167 ++++++++++++++++++ packages/objectql/src/hook-wrappers.ts | 55 ++++++ packages/runtime/src/sandbox/body-runner.ts | 93 +++++++++- .../hook-input-delete-writeback.test.ts | 146 +++++++++++++++ 5 files changed, 552 insertions(+), 4 deletions(-) create mode 100644 .changeset/hook-input-delete-lands.md create mode 100644 packages/objectql/src/hook-input-mutation-traps.test.ts create mode 100644 packages/runtime/src/sandbox/hook-input-delete-writeback.test.ts diff --git a/.changeset/hook-input-delete-lands.md b/.changeset/hook-input-delete-lands.md new file mode 100644 index 0000000000..fa9fbb6783 --- /dev/null +++ b/.changeset/hook-input-delete-lands.md @@ -0,0 +1,95 @@ +--- +'@objectstack/objectql': minor +'@objectstack/runtime': minor +--- + +fix(objectql,runtime): `delete ctx.input.x` in a hook actually removes the field (#12277) + +A hook that stripped a field from its input with `delete` did nothing, on BOTH +execution paths, while an assignment made two lines above it on the same object +in the same call landed normally. Nothing raised, and nothing in the platform +reported it. + +Graded `minor` rather than `patch` deliberately: it moves data that reaches +downstream consumers. Any shipped hook that already contains +`delete ctx.input.` has been a no-op until now and starts taking effect +on upgrade — which is the point, and is also exactly why it must not arrive as +a silent patch. No API is removed and no accept set narrows. + +### The two mechanisms, which were unrelated and produced one outcome + +**In-process (`installFlatInput`, `packages/objectql/src/hook-wrappers.ts`).** +The flat-record `Proxy` a declarative hook receives over the engine's +`{ data, options, id? }` wrapper trapped `get` / `set` / `has` / `ownKeys` / +`getOwnPropertyDescriptor` — but not `deleteProperty`. The delete therefore fell +through to `Reflect.deleteProperty` on the WRAPPER, one level above the record, +removing a key that was never there and returning `true`. `set` was trapped and +wrote into `data`, which is what the engine persists; hence assignment survived +and deletion evaporated. + +**Sandboxed (`applyMutationsToInput`, +`packages/runtime/src/sandbox/body-runner.ts`).** A QuickJS body's mutations +were written home with `Object.assign(target, result.mutatedInput)`. +`Object.assign` copies own enumerable properties and **has no way to represent a +removal**: a key the VM deleted is simply not in the snapshot, and the host's +key stayed. Deletions are now diffed against the entry snapshot and applied +separately. + +Both are fixed in one change on purpose. Closing either alone would make the +same authored `delete` behave differently depending on whether the hook body +runs in-process or in the sandbox — a worse contract than the symmetric silence +it replaced. + +### What an author could see, before and after + +The sandboxed path is the one with no tell at all. Measured on the pre-fix code, +one hook call, host row alongside: + +``` +delete ctx.input.internal_notes -> true +'internal_notes' in ctx.input -> false <- the VM agrees +Object.keys(ctx.input) -> ['subject'] <- ...and so does this +host ctx.input after write-back -> { subject: 'HELP', + internal_notes: 'STAFF-ONLY' } +``` + +The in-process path was less deceptive than reported, and the correction is +worth having in writing: only `delete`'s own return value lied there. `'k' in +input`, `input.k` and `Object.keys(input)` all went on honestly reporting the key +as present, so an author who checked with anything other than the return value +would have seen the no-op. + +### `Object.defineProperty(ctx.input, …)` was the same gap, and nobody reported it + +Found while enumerating the trap set, fixed in the same stroke because it is the +strictly worse shape: it defined on the wrapper, and the `get` trap's +fall-through then read the value straight back — so `input.k` CONFIRMED a write +that never reached `data`, while `Object.keys(input)` denied it and the record +never received it. It now routes into `data` like `set` and `deleteProperty` do. +One inherited JS invariant follows: a proxy may not report success for an +explicitly `configurable: false` descriptor its target does not carry, so +`Object.defineProperty(input, 'x', { value: 1, configurable: false })` now throws +a `TypeError` where it used to define, silently and uselessly, on the wrapper. +Omitting `configurable` — the common spelling, and the one spread and +`Object.assign` produce — is unaffected. + +### The direction the sandbox write-back deliberately does not overreach in + +Absence from the exit snapshot is the only evidence a deletion leaves, and on its +own it is ambiguous: a key whose host value is `undefined` (or a function, or a +symbol) never survived `JSON.stringify` INTO the VM either, so it is missing from +the dump without anyone having deleted it. The diff is filtered through the same +JSON lens the boundary uses, so such a key is left alone. Every failure mode of +that probe is conservative — an unprobeable key is simply not deletable — because +losing a delete is recoverable and destroying a field on evidence that was never +there is not. One residual miss follows and is named here rather than discovered +later: a `bigint`-valued key crosses into the VM as a string but is dropped by the +probe, so deleting one is still lost. + +Measured consumer cost of the reported half: a guest-intake app stripped the +fields an anonymous web-to-case / web-to-lead submitter must not write — +internal staff notes, the resolution, the escalation flag, the owner — with +fifteen `delete` statements, every one inert. A submission carrying +`internal_notes` and `resolution` stored them verbatim, and the app's unit tests +stayed green throughout, because they drive the handler with a plain object where +`delete` genuinely works. diff --git a/packages/objectql/src/hook-input-mutation-traps.test.ts b/packages/objectql/src/hook-input-mutation-traps.test.ts new file mode 100644 index 0000000000..34ca038347 --- /dev/null +++ b/packages/objectql/src/hook-input-mutation-traps.test.ts @@ -0,0 +1,167 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#12277] Every mutation JS offers on `ctx.input` lands in the row the engine + * persists — not just assignment. + * + * `installFlatInput` (`hook-wrappers.ts`) hands a declarative hook a flat-record + * Proxy over the engine's `{ data, options, id? }` wrapper. It trapped `set` + * but not `deleteProperty` or `defineProperty`, so those two fell through to + * `Reflect.*` on the WRAPPER — one level above `data` — and changed a key that + * was never there, on an object the engine does not read. + * + * ## What each assertion is worth, and why the two gaps are not the same shape + * + * The measurement that produced this file (pre-fix, one hook call): + * + * ``` + * delete Object.defineProperty + * operation's own result → true (no throw) + * `k in input` → true — + * `input.k` → CALLER-VALUE DEFINED ← agrees! + * `Object.keys(input)` → includes k excludes k + * what the engine persisted → CALLER-VALUE absent + * ``` + * + * `delete`'s lie was confined to its own return value: the three other + * read-backs stayed honest and reported the key still present. That is a + * silent no-op, and it is what the card reported. + * + * `Object.defineProperty` — which no one reported — is the strictly worse + * shape, and the reason this file pins BOTH: the `get` trap's fall-through to + * the wrapper read the value straight back, so `input.k` CONFIRMED a write + * that never reached `data`. A read-back that corroborates a write that did + * not happen leaves an author no instrument to catch it with. + * + * So every case below asserts the CONJUNCTION — what the hook observes AND + * what the engine is left holding — rather than either alone. Asserting only + * the stored row would pass on an engine whose read-backs lie in the other + * direction; asserting only the read-backs is what shipped the defect. + * + * The `assign-then-delete` case is the DISCRIMINATOR carried over from the + * report: a `{...callerData, ...hookInput}` merge upstream would produce the + * same symptoms as a missing trap, and it would restore the CALLER's value. + * Seeing the hook's own assigned value survive a delete rules the merge out — + * and post-fix, seeing the key vanish entirely rules out a merge just as + * firmly, from the other side. + * + * `wrapDeclarativeHook` is driven directly rather than through `ObjectQL`: the + * defect is in the wrapper's Proxy, and a full engine dispatch would put a + * driver's own copy semantics between the hook and the assertion. + */ + +import { describe, it, expect } from 'vitest'; +import { wrapDeclarativeHook } from './hook-wrappers.js'; + +const silentLogger = { debug: () => {}, info: () => {}, warn: () => {}, error: () => {} }; + +/** Run `handler` as a declarative hook over a caller payload; return the row the engine keeps. */ +async function runHook( + data: Record, + handler: (input: any) => void, +): Promise> { + const meta: any = { name: 'trap_probe', object: 'case', event: 'beforeInsert' }; + const wrapped = wrapDeclarativeHook(meta, (async (ctx: any) => handler(ctx.input)) as any, { + logger: silentLogger, + }); + const raw: any = { data, options: {} }; + await wrapped({ object: 'case', event: 'beforeInsert', input: raw } as any); + return raw.data as Record; +} + +describe('[#12277] `delete ctx.input.x` removes the field from the persisted row', () => { + it('the hook read-backs and the stored row agree that the key is gone', async () => { + const seen: Record = {}; + const persisted = await runHook( + { subject: 'help', owner_id: 'CALLER-VALUE' }, + (input) => { + seen.deleteReturned = delete input.owner_id; + seen.inOperator = 'owner_id' in input; + seen.propertyRead = input.owner_id; + seen.objectKeys = Object.keys(input); + seen.spread = { ...input }; + seen.descriptor = Object.getOwnPropertyDescriptor(input, 'owner_id'); + }, + ); + + // What the author observes. Pre-fix, only the first of these was `true` + // and every other line reported the key still present. + expect(seen.deleteReturned).toBe(true); + expect(seen.inOperator).toBe(false); + expect(seen.propertyRead).toBeUndefined(); + expect(seen.objectKeys).toEqual(['subject']); + expect(seen.spread).toEqual({ subject: 'help' }); + expect(seen.descriptor).toBeUndefined(); + + // …and what the engine is left holding. This is the half the author cannot + // reach from inside the hook, and the half the defect falsified. + expect(persisted).toEqual({ subject: 'help' }); + }); + + it('POSITIVE CONTROL — an assignment in the same call still lands', async () => { + // Without this, every assertion above would also pass against a wrapper + // that had stopped writing anything through to `data` at all. + const persisted = await runHook({ subject: 'help', owner_id: 'CALLER-VALUE' }, (input) => { + input.subject = 'HELP'; + delete input.owner_id; + }); + expect(persisted).toEqual({ subject: 'HELP' }); + }); + + it('DISCRIMINATOR — assign-then-delete leaves no key, not the caller value', async () => { + // A `{...callerData, ...hookInput}` merge would answer `CALLER-NOTE` here. + const seen: Record = {}; + const persisted = await runHook({ note: 'CALLER-NOTE' }, (input) => { + input.note = 'ASSIGNED-THEN-DELETED'; + seen.afterAssign = input.note; + delete input.note; + seen.afterDelete = input.note; + }); + expect(seen.afterAssign).toBe('ASSIGNED-THEN-DELETED'); + expect(seen.afterDelete).toBeUndefined(); + expect(persisted).toEqual({}); + }); + + it('deleting a key that was never in the payload is a no-op that reports success', async () => { + const persisted = await runHook({ subject: 'help' }, (input) => { + expect(delete input.never_here).toBe(true); + }); + expect(persisted).toEqual({ subject: 'help' }); + }); + + it('the operation envelope is addressed separately from the record fields', async () => { + // `id`/`options`/`ast`/`data` are wrapper keys on every other trap, and + // `deleteProperty` routes them the same way — a hook deleting `options` + // must not punch a hole in a record field that happens to share the name. + const meta: any = { name: 'envelope', object: 'case', event: 'beforeUpdate' }; + const wrapped = wrapDeclarativeHook(meta, (async (ctx: any) => { + delete ctx.input.options; + }) as any, { logger: silentLogger }); + const raw: any = { id: 'r1', data: { options: 'A RECORD FIELD CALLED OPTIONS' }, options: { multi: true } }; + await wrapped({ object: 'case', event: 'beforeUpdate', input: raw } as any); + expect('options' in raw).toBe(false); + expect(raw.data).toEqual({ options: 'A RECORD FIELD CALLED OPTIONS' }); + }); +}); + +describe('[#12277] `Object.defineProperty(ctx.input, …)` lands in the persisted row', () => { + it('the confirming read-back is now telling the truth', async () => { + // The pre-fix failure this case exists for: `input.defined_key` read back + // `DEFINED` while `data` never received it, so the instrument an author + // would reach for to check AGREED with a write that did not happen. + const seen: Record = {}; + const persisted = await runHook({ subject: 'help' }, (input) => { + Object.defineProperty(input, 'defined_key', { + value: 'DEFINED', + enumerable: true, + writable: true, + configurable: true, + }); + seen.propertyRead = input.defined_key; + seen.inKeys = Object.keys(input).includes('defined_key'); + }); + expect(seen.propertyRead).toBe('DEFINED'); + expect(seen.inKeys).toBe(true); + expect(persisted).toEqual({ subject: 'help', defined_key: 'DEFINED' }); + }); +}); diff --git a/packages/objectql/src/hook-wrappers.ts b/packages/objectql/src/hook-wrappers.ts index fe5d16aec2..14835d37ce 100644 --- a/packages/objectql/src/hook-wrappers.ts +++ b/packages/objectql/src/hook-wrappers.ts @@ -498,6 +498,8 @@ export function wrapDeclarativeHook( * of any other key fall through to `data`. Writes always go to `data` * (creating it if missing) so the engine's downstream `input.data` * read picks up mutations made by user code as `input.field = value`. + * "Writes" means every mutation JS has, not assignment alone: `delete` and + * `Object.defineProperty` route into `data` too (#12277). */ function installFlatInput(ctx: HookContext): () => void { const raw: any = ctx.input ?? {}; @@ -532,6 +534,59 @@ function installFlatInput(ctx: HookContext): () => void { ensureData()[prop as string] = value; return true; }, + // [#12277] The mutation traps are a SET, not a list: every operation JS + // offers for changing a property has to land in `data`, because `data` is + // the object the engine persists. `set` alone was trapped, so + // `delete input.x` and `Object.defineProperty(input, 'x', …)` fell through + // to `Reflect.*` on the WRAPPER — one level above the record — and did + // nothing to the row while reporting success. + // + // The two gaps had different shapes, and the worse-shaped one is the one + // nobody reported: + // + // - `delete input.x` returned `true` and changed nothing. The other + // read-backs stayed HONEST (`'x' in input`, `input.x`, + // `Object.keys(input)` all still showed the key), so the lie was + // confined to `delete`'s own return value. + // - `Object.defineProperty(input, 'x', …)` defined on the wrapper, and + // the `get` trap's fall-through to the wrapper then READ IT BACK — so + // `input.x` confirmed a write that never reached `data`. That is the + // shape with no instrument to catch it from inside a hook. + // + // Measured cost of the `delete` half before this landed: a guest-intake + // app stripped the fields an anonymous submitter must not write with 15 + // `delete` statements, every one inert, and its unit tests stayed green + // because they drive the handler with a plain object. + // + // `deleteProperty` deliberately does NOT call `ensureData()`: with no + // `data` on the wrapper, `get` reads fall through to the wrapper itself, + // so that is where the key would live and where the delete belongs. + // Materialising an empty `data` just to delete out of it would be a write + // performed by a removal. + deleteProperty(target, prop) { + if (prop === 'id' || prop === 'options' || prop === 'ast' || prop === 'data') { + return Reflect.deleteProperty(target, prop); + } + const data = target.data; + if (data && typeof data === 'object') { + return Reflect.deleteProperty(data as object, prop); + } + return Reflect.deleteProperty(target, prop); + }, + // Routed for the same reason `set` is. One inherited JS invariant is worth + // naming: a proxy may not report success for an explicitly + // `configurable: false` descriptor the TARGET does not carry, so + // `Object.defineProperty(input, 'x', { value: 1, configurable: false })` + // now throws a TypeError where it used to silently define on the wrapper. + // A throw is a diagnosis; the silence was not. Omitting `configurable` + // entirely (the common spelling, and every spelling `Object.assign` and + // spread produce) is unaffected. + defineProperty(target, prop, desc) { + if (prop === 'id' || prop === 'options' || prop === 'ast' || prop === 'data') { + return Reflect.defineProperty(target, prop, desc); + } + return Reflect.defineProperty(ensureData(), prop, desc); + }, has(target, prop) { if (prop === 'id' || prop === 'options' || prop === 'ast' || prop === 'data') { return prop in target; diff --git a/packages/runtime/src/sandbox/body-runner.ts b/packages/runtime/src/sandbox/body-runner.ts index 843608b88a..0f73e89371 100644 --- a/packages/runtime/src/sandbox/body-runner.ts +++ b/packages/runtime/src/sandbox/body-runner.ts @@ -29,7 +29,10 @@ * shallow-merged on top of `mutatedInput` as an explicit patch. * Writes go through `Object.assign`, which means the host engine's * flat-record Proxy (installed by `wrapDeclarativeHook`) sees them - * via its set trap. + * via its set trap. `Object.assign` cannot express a REMOVAL, so + * keys the VM deleted are diffed out of the entry snapshot and + * deleted on the host separately (#12277) — see + * {@link applyMutationsToInput}. * * The ACTION path deliberately has no step 4: its output is the script's * return value, and its write channel is `ctx.api.object(...)`. In particular @@ -317,7 +320,7 @@ export function hookBodyRunnerFactory( // constructor option dead for hooks. timeoutMs: (body as any).timeoutMs, }); - applyMutationsToInput(engineCtx, result); + applyMutationsToInput(engineCtx, result, sandboxCtx.input); } catch (err: any) { opts.logger?.error?.('[BodyRunner] sandboxed hook threw', err, { appId: opts.appId, @@ -467,11 +470,93 @@ function warnDiscardedRecordWrites( ); } -function applyMutationsToInput(engineCtx: any, result: ScriptResult): void { +/** + * [#12277] Keys of the ENTRY snapshot that the VM could actually see, and + * therefore the only keys whose absence from the exit snapshot is evidence of + * a deletion. + * + * Both directions of the sandbox boundary are JSON (`safeJsonStringify` in, + * `JSON.stringify` out), and JSON has no spelling for `undefined`, a function, + * or a symbol. A key carrying one of those is absent from the VM's `ctx.input` + * from the start, so it is absent from the dump too — indistinguishable, on the + * dump alone, from a key the body deleted. Filtering the entry side through the + * SAME lens removes that ambiguity at the source instead of guessing at it. + * + * Every failure mode here is deliberately conservative — a key that cannot be + * probed is simply not deletable, so the worst outcome is the pre-#12277 + * behaviour (a delete that does not land) rather than a field destroyed on + * evidence that was never there. One residual miss follows from that and is + * worth naming: `safeJsonStringify` marshals a `bigint` into the VM as a + * string, while the probe below throws on it and drops it — so a delete of a + * bigint-valued key is still lost. Losing a delete is the recoverable + * direction; inventing one is not. + */ +function vmVisibleEntryKeys(entryInput: unknown): string[] { + if (!entryInput || typeof entryInput !== 'object' || Array.isArray(entryInput)) return []; + const out: string[] = []; + for (const [k, v] of Object.entries(entryInput as Record)) { + try { + if (JSON.stringify(v) !== undefined) out.push(k); + } catch { + /* unserialisable (cycle, bigint) — not deletable on this evidence */ + } + } + return out; +} + +/** + * Write one settled body's mutations back onto the host `ctx.input`. + * + * ## [#12277] Why a key diff, and not `Object.assign` alone + * + * `Object.assign` copies own enumerable properties, and **has no way to + * represent a deletion**: a key the VM removed simply is not in `mutatedInput`, + * and the original stays. So `delete ctx.input.internal_notes` in a sandboxed + * body was lost on the way home — and lost in the worst possible shape, + * because INSIDE the VM the delete is real. Measured on the pre-fix code, with + * the host row alongside: + * + * ``` + * delete ctx.input.internal_notes -> true + * 'internal_notes' in ctx.input -> false // the VM agrees + * Object.keys(ctx.input) -> ['subject'] // …and so does this + * host ctx.input after write-back -> { subject: 'HELP', + * internal_notes: 'STAFF-ONLY' } + * ``` + * + * Every instrument reachable from inside the body confirms the removal, an + * assignment made in the same call lands, and the field is stored anyway. + * There is no diagnostic to notice and nothing to notice it with. + * + * The sibling half of the same defect was the engine's flat-input Proxy + * missing its `deleteProperty` trap (`installFlatInput`, + * `packages/objectql/src/hook-wrappers.ts`); the two are unrelated mechanisms + * with one author-visible outcome. They are fixed together on purpose: closing + * one alone would make the same authored `delete` behave differently depending + * on whether the body runs in-process or in QuickJS, which is a worse contract + * than the symmetric silence it replaces. + * + * Deletions apply BEFORE both merges, so a body that deletes a key and then + * returns it (`delete ctx.input.x; return { x: 1 };`) keeps the explicit patch + * — the return value is the later, more deliberate statement of the two. + */ +function applyMutationsToInput( + engineCtx: any, + result: ScriptResult, + // `unknown`, not `Record`: that is what `ScriptContext.input` + // declares, and narrowing is {@link vmVisibleEntryKeys}'s job. Widening the + // declared type here to make the call site typecheck would move the guard to + // the producer's word rather than this consumer's own check. + entryInput?: unknown, +): void { const target = engineCtx?.input; if (!target || typeof target !== 'object') return; if (result.mutatedInput && typeof result.mutatedInput === 'object') { - Object.assign(target, result.mutatedInput); + const mutated = result.mutatedInput; + for (const key of vmVisibleEntryKeys(entryInput)) { + if (!(key in mutated)) delete (target as Record)[key]; + } + Object.assign(target, mutated); } if ( result.value && diff --git a/packages/runtime/src/sandbox/hook-input-delete-writeback.test.ts b/packages/runtime/src/sandbox/hook-input-delete-writeback.test.ts new file mode 100644 index 0000000000..4dc7c8be86 --- /dev/null +++ b/packages/runtime/src/sandbox/hook-input-delete-writeback.test.ts @@ -0,0 +1,146 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#12277] A sandboxed hook body's `delete ctx.input.x` reaches the host row. + * + * ## Why this is the worse half of the card, and what it takes to pin it + * + * The engine-side half of #12277 was a missing `deleteProperty` trap on the + * flat-input Proxy: a no-op whose lie was confined to `delete`'s own return + * value, since `k in input` and `Object.keys(input)` went on honestly + * reporting the key. + * + * This half has no such tell. Inside QuickJS the delete is REAL — the body + * holds a JSON snapshot, and every read-back it can reach agrees. What was + * lost is the trip home: `applyMutationsToInput` wrote mutations back with + * `Object.assign(target, result.mutatedInput)`, and `Object.assign` copies own + * enumerable properties. **It has no way to represent a deletion.** A key the + * VM removed is simply not in `mutatedInput`, and the host's key stays. + * + * Measured on the pre-fix code, one hook call: + * + * ``` + * delete ctx.input.internal_notes -> true + * 'internal_notes' in ctx.input -> false ← the VM agrees + * Object.keys(ctx.input) -> ['subject'] ← …and so does this + * host ctx.input after write-back -> { subject: 'HELP', + * internal_notes: 'STAFF-ONLY' } + * ``` + * + * That is why the first case asserts BOTH ENDS of the same call — what the + * body observed and what the host was left holding. Asserting only the host + * row would leave the pin passing on a runner that had stopped executing the + * body at all; asserting only the body's view is precisely the reading that + * shipped the defect, because it was true the whole time. + * + * ## The direction the write-back must NOT overreach in + * + * Absence from the exit dump is the only evidence a deletion leaves, and it is + * ambiguous on its own: a key whose host value is `undefined` (or a function, + * or a symbol) never survived `JSON.stringify` into the VM either, so it is + * missing from the dump without anyone having deleted it. The last case pins + * that such a key is LEFT ALONE. Losing a delete is recoverable; destroying a + * field on evidence that was never there is not, so the diff is filtered + * through the same JSON lens the boundary uses rather than trusting absence. + */ + +import { describe, it, expect } from 'vitest'; +import { hookBodyRunnerFactory } from './body-runner.js'; +import { QuickJSScriptRunner } from './quickjs-runner.js'; + +const runner = new QuickJSScriptRunner(); + +/** Build a sandboxed `beforeInsert` hook around one JS body. */ +function jsHook(source: string) { + return hookBodyRunnerFactory(runner, { ql: {}, appId: 'crm' })({ + name: 'guest_intake', + object: 'case', + events: ['beforeInsert'], + body: { language: 'js', source, capabilities: [] }, + } as any)!; +} + +describe('[#12277] a sandboxed body can delete an input field', () => { + it('the body’s read-backs and the host row agree the field is gone', async () => { + const fn = jsHook( + [ + 'var seen = {};', + 'seen.deleteReturned = delete ctx.input.internal_notes;', + "seen.inOperator = 'internal_notes' in ctx.input;", + 'seen.propertyRead = ctx.input.internal_notes;', + 'seen.objectKeys = Object.keys(ctx.input);', + // An assignment in the same call: the POSITIVE CONTROL. Without it the + // host assertions below would also pass against a runner whose + // write-back had stopped doing anything at all. + 'ctx.input.subject = String(ctx.input.subject).toUpperCase();', + 'return { __probe: JSON.stringify(seen) };', + ].join('\n'), + ); + const engineCtx: any = { input: { subject: 'help', internal_notes: 'STAFF-ONLY' } }; + await fn(engineCtx); + + const seen = JSON.parse(String(engineCtx.input.__probe)); + expect(seen.deleteReturned).toBe(true); + expect(seen.inOperator).toBe(false); + expect(seen.propertyRead).toBeUndefined(); + expect(seen.objectKeys).toEqual(['subject']); + + expect('internal_notes' in engineCtx.input).toBe(false); + expect(engineCtx.input.subject).toBe('HELP'); + }); + + it('an explicit return patch outranks a delete of the same key', async () => { + // Deletions apply before both merges, so the later, more deliberate + // statement wins rather than the two racing on write-back order. + const fn = jsHook('delete ctx.input.status; return { status: "reopened" };'); + const engineCtx: any = { input: { subject: 'help', status: 'closed' } }; + await fn(engineCtx); + expect(engineCtx.input.status).toBe('reopened'); + }); + + it('the whole guest-intake shape: several deletes in one body, all of them landing', async () => { + // The consumer measurement behind the card — an anonymous web-to-case + // submission that must not be able to write staff-only fields. Fifteen + // `delete` statements were inert; a submission carrying `internal_notes` + // and `resolution` stored them verbatim. + const fn = jsHook( + [ + 'delete ctx.input.internal_notes;', + 'delete ctx.input.resolution;', + 'delete ctx.input.escalated;', + 'delete ctx.input.owner_id;', + ].join('\n'), + ); + const engineCtx: any = { + input: { + subject: 'printer on fire', + internal_notes: 'STAFF-ONLY', + resolution: 'FORGED', + escalated: true, + owner_id: 'usr_admin', + description: 'it is on fire', + }, + }; + await fn(engineCtx); + expect(Object.keys(engineCtx.input).sort()).toEqual(['description', 'subject']); + }); + + it('a key the VM never saw is LEFT ALONE, not deleted', async () => { + // `undefined` has no JSON spelling, so `absent_from_the_dump` proves + // nothing about `undef_key` — the body could not have deleted what it + // could not see. The conservative direction is mandatory here: the + // alternative destroys a field on absent evidence. + const fn = jsHook('ctx.input.subject = "HELP";'); + const engineCtx: any = { input: { subject: 'help', undef_key: undefined } }; + await fn(engineCtx); + expect('undef_key' in engineCtx.input).toBe(true); + expect(engineCtx.input.subject).toBe('HELP'); + }); + + it('a body that deletes nothing changes nothing', async () => { + const fn = jsHook('return { subject: ctx.input.subject.trim() };'); + const engineCtx: any = { input: { subject: ' help ', internal_notes: 'KEEP ME' } }; + await fn(engineCtx); + expect(engineCtx.input).toEqual({ subject: 'help', internal_notes: 'KEEP ME' }); + }); +});