diff --git a/.changeset/hook-body-sandbox-globals-allowlist.md b/.changeset/hook-body-sandbox-globals-allowlist.md new file mode 100644 index 0000000000..0c510717f6 --- /dev/null +++ b/.changeset/hook-body-sandbox-globals-allowlist.md @@ -0,0 +1,64 @@ +--- +"@objectstack/cli": patch +--- + +fix(cli): stop lowering hook/action bodies that reference globals the sandbox does not provide (#14301) + +`detect-free-identifiers`' ambient allowlist was ONE generous list, documented as +"assume the runtime has it". The runtime a lowered body actually runs in is the +QuickJS sandbox, not the Node process that runs `objectstack build` — and the +list named `Intl` beside `JSON`, under the comment "Web-ish that the sandbox / +Node commonly provide". So a handler calling `Intl.DateTimeFormat` had no free +identifier at all: `extractHookBody` lowered it into `body.source`, the #13651 +lint rule had nothing to report (it fires only on a *refused* lowering), +`validate` / `typecheck` / `test` / `build` were all green because the +in-process test runs the RAW function in Node where `Intl` exists — and +production threw `ReferenceError: Intl is not defined`. Under the +`onError: 'abort'` a validation-shaped hook must declare, that refuses every +write to the object. + +The allowlist is now two sets, and membership is **measured**, never recalled: a +`typeof X`/`'X' in globalThis` probe is evaluated inside the same +`QuickJSScriptRunner` the runtime evaluates a body in, and +`sandbox-globals-probe.test.ts` fails unless each set is exactly the probe's +present/absent partition. + +- **`SANDBOX_GLOBALS` (53, measured present)** — the ECMAScript surface: + `Math` `JSON` `Date` `Object` `Array` `String` `Number` `Boolean` `RegExp` + `Map` `Set` `WeakMap` `WeakSet` `Promise` `Symbol` `BigInt` `Function` + `Reflect` `Proxy`, the typed-array/buffer family, the eight error + constructors, `parseInt` `parseFloat` `isNaN` `isFinite` and the four URI + functions, plus `undefined` `NaN` `Infinity` `globalThis`. +- **`NODE_ONLY_GLOBALS` (15, measured absent)** — `Intl`, `structuredClone`, + `queueMicrotask`, `atob`, `btoa`, `setTimeout`, `clearTimeout`, + `setInterval`, `clearInterval`, `URL`, `URLSearchParams`, `TextEncoder`, + `TextDecoder`, `console`, `arguments`. + +A free reference to one of the 15 is now a lowering refusal whose reason names +the identifier and the remedy — a string handler ref (`functions:` map plus +`handler: 'fn_name'`) or a validation rule, and for `console` specifically the +capability-gated `ctx.log`. It travels the SAME path #1876 already used: the +refusal is `kind: 'free-identifiers'` with the host-only half carried +separately as `nodeOnlyIdentifiers`, `lowerCallables` catches it and ships the +callable through the `.mjs` bundle (where it runs in-process in Node and +works), `os build` still exits 0 with a warning, and `os lint` reports it under +`hook-body/not-lowerable` as an `error` — the ACCIDENTAL class, because `Intl` +is a standard global in every browser and in Node and writing it is not the +recognisable "I am reaching for the host" act that `fetch(` and `process.` are. +The remedy sentence `os lint` prints is chosen from the refusal's own +classification: "inline the value" is impossible for a host global, and +printing it anyway would send an author after a second broken shape. + +⛔ Not changed here: whether `os build` fails on the lowering class (#13838), +and what the sandbox provides (giving it `Intl` would be a capability +expansion). Nothing under `packages/runtime/**` is touched — the probe reads +that sandbox, it does not change it. + +**Why `patch`.** No published accept-set moves: the metadata a valid app may +declare is identical, `HookBodySchema` is untouched, and no key is added, +removed or re-shaped. What narrows is which handlers the build LOWERS, and for +every handler affected the previous outcome was a body that could not run. An +app hitting this gains a warning and a working bundled closure in place of a +production `ReferenceError`; the deployment shape it loses was never one it had +in working order. Measured corpus: zero in-repo example or template handlers +reference any of the 15. diff --git a/packages/cli/src/lint/hook-body-lowering.test.ts b/packages/cli/src/lint/hook-body-lowering.test.ts index 5af4fdea76..c9b8ee737b 100644 --- a/packages/cli/src/lint/hook-body-lowering.test.ts +++ b/packages/cli/src/lint/hook-body-lowering.test.ts @@ -42,6 +42,19 @@ const forbiddenTokenHook = { }, }; +// [#14301] A global the NODE HOST provides and the sandbox does not. Reads as +// self-contained — `Intl` is standard in every browser and in Node — which is +// precisely why it is the ACCIDENTAL class and not the structural one. +const nodeOnlyGlobalHook = { + name: 'stamp_due_label', + object: 'task', + events: ['beforeInsert'], + handler: (ctx: any) => { + const f = new Intl.DateTimeFormat('en-US', { timeZone: 'UTC' }); + ctx.input.due_label = f.format(new Date(ctx.input.due_at)); + }, +}; + const selfContainedHook = { name: 'normalize_name', object: 'account', @@ -91,6 +104,41 @@ describe('checkHookBodyLowering', () => { expect(issues[0].message).toContain('deployment shape'); }); + describe('a host global the sandbox lacks is the ACCIDENTAL class (#14301)', () => { + it('reports it as an ERROR under the not-lowerable rule', () => { + const issues = checkHookBodyLowering({ hooks: [nodeOnlyGlobalHook] }); + + expect(issues).toHaveLength(1); + expect(issues[0].severity).toBe('error'); + expect(issues[0].rule).toBe(NOT_LOWERABLE_RULE); + expect(issues[0].path).toBe('hooks[0].handler'); + expect(issues[0].message).toContain("hook 'stamp_due_label'"); + expect(issues[0].message).toContain('Intl'); + expect(issues[0].message).toContain('deployment shape'); + }); + + it('prints the remedy that is POSSIBLE, not the inline one', () => { + // The wrong-remedy failure is worse than none: there is nothing to inline + // `Intl` from, so an author (or a code-writing model) told to inline it + // lands on a second broken shape. The remedy sentence is chosen from the + // refusal's own `nodeOnlyIdentifiers`, never re-derived here. + const [issue] = checkHookBodyLowering({ hooks: [nodeOnlyGlobalHook] }); + expect(issue.message).toContain('provided by the Node host'); + expect(issue.message).toContain('cannot be inlined'); + expect(issue.message).toContain('string handler ref'); + expect(issue.message).toContain('validation rule'); + expect(issue.message).not.toContain('Inline the value(s) into the handler'); + }); + + it('leaves the module-scope remedy exactly as it was', () => { + // The reverse leg: the #13651 sentence must not have been rewritten for + // every author by a branch added for one new sub-case. + const [issue] = checkHookBodyLowering({ hooks: [freeIdentifierHook] }); + expect(issue.message).toContain('Inline the value(s) into the handler, or reach them through `ctx`.'); + expect(issue.message).not.toContain('provided by the Node host'); + }); + }); + it('keeps the STRUCTURAL refusal a warning — the bundle is its designed answer', () => { const issues = checkHookBodyLowering({ hooks: [forbiddenTokenHook] }); diff --git a/packages/cli/src/lint/hook-body-lowering.ts b/packages/cli/src/lint/hook-body-lowering.ts index 009c396043..f3d9902310 100644 --- a/packages/cli/src/lint/hook-body-lowering.ts +++ b/packages/cli/src/lint/hook-body-lowering.ts @@ -38,13 +38,22 @@ * Today both arrive through one catch, so they share one fate. They are not * the same event, and the refusal kind already tells them apart: * - * `free-identifiers` ACCIDENTAL. The handler IS expressible as a metadata - * body; it merely names a module-scope const/helper/ - * import. The author wrote something that reads + * `free-identifiers` ACCIDENTAL. The author wrote something that reads * self-contained, and the platform quietly re-shaped the * deployment behind them — its own message even says "no * behavior change", which is true of behaviour and false - * of shape. Remedy is local: inline the value. => `error`. + * of shape. => `error`, for both sub-cases: + * · a module-scope const/helper/import — remedy is + * local: inline the value. + * · a global the NODE HOST has and the sandbox does not + * (#14301, `Intl`) — NOT inlinable, and still + * accidental rather than a chosen bundle: `Intl` is a + * standard global in every browser and in Node, so + * writing it is not the recognisable "I am reaching + * for the host" act that `fetch(`/`process.` are. + * That is why it is an `error` here and not the + * `warning` its structural cousin gets — and why the + * remedy sentence below is CHOSEN per sub-case. * `forbidden-token` STRUCTURAL. `fetch`/`require`/`process`/`eval`/… are * capabilities the sandbox does not have, so the handler * can NEVER be a metadata body. Writing one IS choosing a @@ -129,12 +138,37 @@ function judge(fn: AnyFn, originLabel: string, path: string): HookBodyLintIssue } catch (err: unknown) { const kind = err instanceof HookBodyExtractionError ? err.kind : 'unknown'; const free = err instanceof HookBodyExtractionError ? err.freeIdentifiers : []; + // [#14301] The half of `free` the Node HOST provides and the sandbox does + // not. Read off the refusal rather than re-derived here: this rule's whole + // parity claim is that it cannot disagree with what `os build` did to the + // same handler, and a second membership table in this file would be exactly + // such a disagreement waiting to happen. + const nodeOnly = err instanceof HookBodyExtractionError ? err.nodeOnlyIdentifiers : []; // Only the first line: the refusal text carries a multi-line offending-source // dump for `--strict-body`'s per-callable diagnostic, which would swamp a // lint report. `os build --strict-body` remains the place to read it whole. const firstLine = String((err as Error)?.message ?? err).split('\n')[0]; if (kind === 'free-identifiers') { + // The remedy sentence is chosen, not fixed. "Inline the value(s)" is + // right for a module-scope helper and IMPOSSIBLE for a host global — + // there is nothing to inline `Intl` from — so printing it for both would + // send an author (or a code-writing model) after a second broken shape. + const moduleScope = free.filter((n) => !nodeOnly.includes(n)); + const hostRemedy = + `${nodeOnly.join(', ')} ${nodeOnly.length === 1 ? 'is' : 'are'} provided by the Node host ` + + `and NOT by the sandbox, so ${nodeOnly.length === 1 ? 'it' : 'they'} cannot be inlined: ` + + `keep the check in a string handler ref, or move it to a validation rule.`; + const inlineRemedy = + `Inline the value(s) into the handler, or reach them through \`ctx\`.`; + const remedy = + nodeOnly.length === 0 + ? inlineRemedy + : moduleScope.length === 0 + ? hostRemedy + : `${hostRemedy} ${moduleScope.join(', ')} ${moduleScope.length === 1 ? 'is' : 'are'} ` + + `module-scope: inline ${moduleScope.length === 1 ? 'it' : 'them'} into the handler, ` + + `or reach ${moduleScope.length === 1 ? 'it' : 'them'} through \`ctx\`.`; return { severity: 'error', rule: NOT_LOWERABLE_RULE, @@ -143,8 +177,7 @@ function judge(fn: AnyFn, originLabel: string, path: string): HookBodyLintIssue `${free.length === 1 ? 'the identifier' : 'identifiers'} ${free.join(', ')}, which ` + `${free.length === 1 ? 'is' : 'are'} not in scope inside the sandbox. The handler is ` + `BUNDLED instead, so this app is no longer shippable as pure metadata — a change of ` + - `deployment shape, not of behaviour. Inline the value(s) into the handler, or reach ` + - `them through \`ctx\`. ${DELIBERATE_BUNDLE_REMEDY}`, + `deployment shape, not of behaviour. ${remedy} ${DELIBERATE_BUNDLE_REMEDY}`, path, }; } diff --git a/packages/cli/src/utils/detect-free-identifiers.test.ts b/packages/cli/src/utils/detect-free-identifiers.test.ts index 29d4cbbc77..e0b68c67b0 100644 --- a/packages/cli/src/utils/detect-free-identifiers.test.ts +++ b/packages/cli/src/utils/detect-free-identifiers.test.ts @@ -1,7 +1,11 @@ // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. import { describe, it, expect } from 'vitest'; -import { detectFreeIdentifiers } from './detect-free-identifiers.js'; +import { + detectFreeIdentifiers, + NODE_ONLY_GLOBALS, + SANDBOX_GLOBALS, +} from './detect-free-identifiers.js'; /** Helper: stringify a real function so we test the exact `.toString()` path. */ const src = (fn: (...a: any[]) => any) => String(fn); @@ -53,6 +57,14 @@ describe('detectFreeIdentifiers (#1876 — body self-containment)', () => { ['nested destructuring', '({ a: { b } }) => b + 1'], ['rest params', '(...args) => args.length'], ['typeof a local', '(ctx) => { const v = ctx.v; return typeof v; }'], + // #14301 — the node-only refusal is a SCOPE analysis, not a token scan. + // A locally-bound name that happens to spell a host global is bound, and + // a member NAMED after one is not a reference at all. Both would be + // false refusals, and a false refusal here costs an author a body they + // were entitled to. + ['a local shadowing a host global', '(ctx) => { const Intl = ctx.fmt; return Intl.format(ctx.d); }'], + ['a member named after a host global', '(ctx) => { return ctx.Intl.format(ctx.d); }'], + ['a param named after a host global', '(ctx, Intl) => Intl.format(ctx.d)'], ]; for (const [label, source] of selfContained) { it(label, () => { @@ -81,4 +93,79 @@ describe('detectFreeIdentifiers (#1876 — body self-containment)', () => { expect(detectFreeIdentifiers('').free).toEqual([]); expect(detectFreeIdentifiers('{ not: valid').free).toEqual([]); }); + // ── #14301 — globals the NODE HOST has and the sandbox does not ────────── + // + // The reported shape: `Intl` sat in one generous allowlist beside `JSON`, so + // a handler calling `Intl.DateTimeFormat` had no free identifier, lowered + // into `body.source`, and threw `ReferenceError` in production while every + // local gate was green. The split makes the reference visible again; the + // membership of the two sets is not asserted here from knowledge — it is + // measured inside the real sandbox by `sandbox-globals-probe.test.ts`. + describe('reports host globals the sandbox does not provide', () => { + it('the card reproduction — Intl.DateTimeFormat', () => { + const r = detectFreeIdentifiers( + "(ctx) => { const f = new Intl.DateTimeFormat('en-US'); ctx.input.label = f.format(new Date(ctx.input.at)); }", + ); + expect(r.unparsed).toBe(false); + expect(r.free).toEqual(['Intl']); + expect(r.nodeOnly).toEqual(['Intl']); + }); + + it('`nodeOnly` is a labelled SUBSET of `free`, not a second list', () => { + // Both halves at once. The caller needs them apart because the remedies + // are opposite — inline the helper, but a host global cannot be inlined + // — and needs them together because the refusal names every name. + const r = detectFreeIdentifiers('(ctx) => { ctx.x = slugify(ctx.name) + Intl.NumberFormat; }'); + expect(r.free).toEqual(['Intl', 'slugify']); + expect(r.nodeOnly).toEqual(['Intl']); + expect(r.nodeOnly.every((n) => r.free.includes(n))).toBe(true); + }); + + it('a sandbox-provided global is still waived — the positive control', () => { + const r = detectFreeIdentifiers('(ctx) => { ctx.x = JSON.stringify(Math.round(ctx.n)); }'); + expect(r.free).toEqual([]); + expect(r.nodeOnly).toEqual([]); + }); + + it('every member of NODE_ONLY_GLOBALS is reported when referenced free', () => { + // Set-wide rather than per-name: a member added to the set without the + // detector reading it would otherwise sit inert, which is the exact + // shape of the defect being closed one level up. + for (const name of NODE_ONLY_GLOBALS) { + const r = detectFreeIdentifiers(`(ctx) => { ctx.x = ${name}; }`); + expect({ name, free: r.free, nodeOnly: r.nodeOnly }).toEqual({ + name, + free: [name], + nodeOnly: [name], + }); + } + }); + + it('every member of SANDBOX_GLOBALS is waived when referenced free', () => { + for (const name of SANDBOX_GLOBALS) { + const r = detectFreeIdentifiers(`(ctx) => { ctx.x = ${name}; }`); + expect({ name, free: r.free, nodeOnly: r.nodeOnly }).toEqual({ + name, + free: [], + nodeOnly: [], + }); + } + }); + + it('junk input reports neither list (conservative — never blocks extraction)', () => { + // The bias the file's header states, restated over the NEW field: a + // source this analysis cannot make sense of must not produce a refusal. + // Asserted over the same three junk shapes the #1876 case uses, and + // without asserting `unparsed` — TS error-recovery decides that, and the + // invariant that matters is that neither list fills. + for (const junk of ['this is not a function', '', '{ not: valid']) { + const r = detectFreeIdentifiers(junk); + expect({ junk, free: r.free, nodeOnly: r.nodeOnly }).toEqual({ + junk, + free: [], + nodeOnly: [], + }); + } + }); + }); }); diff --git a/packages/cli/src/utils/detect-free-identifiers.ts b/packages/cli/src/utils/detect-free-identifiers.ts index 068637867f..e8c163f85d 100644 --- a/packages/cli/src/utils/detect-free-identifiers.ts +++ b/packages/cli/src/utils/detect-free-identifiers.ts @@ -16,13 +16,24 @@ * fall back to BUNDLING it (esbuild bundles the real closure, so `slugify` comes * along) — no ReferenceError, no build break. * - * Safety bias: this analysis is deliberately **conservative**. `bindings` + * Safety bias, and the one direction it does NOT hold. `bindings` * over-approximates (every name declared ANYWHERE in the function counts as - * bound), and `GLOBALS` is generous. Both bias toward NOT flagging — a missed - * case merely preserves today's behavior, whereas a false positive would only - * ever cause a self-contained handler to be bundled instead of inlined (a - * size/over-caution cost, never a correctness or build failure). We never - * trade that bias for completeness. + * bound), which biases toward NOT flagging — safe, because a false positive + * only ever costs a self-contained handler a bundle instead of an inline + * (size/over-caution), never correctness. + * + * The AMBIENT-NAME allowlist used to be described the same way, and that was + * the defect: "assume the runtime has it" is only conservative for names the + * runtime being assumed about actually has. The lowered body's runtime is the + * QuickJS sandbox, not the Node process that runs `objectstack build` — and the + * allowlist named globals (`Intl` the reported one) that Node has and the + * sandbox does not. Not flagging those did not preserve behaviour: it lowered a + * body that throws `ReferenceError` in production while `validate`, `typecheck`, + * `test` and `build` all stay green, because the in-process test runs the RAW + * function in Node, where the global exists. So the allowlist is now two sets — + * {@link SANDBOX_GLOBALS}, whose membership is MEASURED inside the sandbox, and + * {@link NODE_ONLY_GLOBALS}, the host-only remainder, which is REPORTED as free + * so the handler falls back to the bundle (where it runs in Node and works). */ // `ts-morph` is already a CLI runtime dependency and re-exports the full @@ -31,16 +42,25 @@ import { ts } from 'ts-morph'; /** - * Identifiers the JS runtime provides ambiently. Generous on purpose — listing - * a name here means "assume the runtime has it" → don't flag → don't over-bundle - * the rare false positive. A genuinely-missing global is a different problem - * (sandbox capability), not a module-scope-helper leak. + * Identifiers the HOOK SANDBOX provides ambiently — the allowlist proper. + * + * ⛔ MEMBERSHIP IS MEASURED, NEVER RECALLED. Every name here was read out of the + * shipped QuickJS build by `sandbox-globals-probe.test.ts`, which evaluates a + * `typeof`/`in globalThis` probe for each member INSIDE the same + * `QuickJSScriptRunner` the runtime evaluates a lowered body in, and fails if + * this set is not exactly the probe's present-set. That pin is what keeps the + * split honest: a name added here from memory reddens it. + * + * `undefined` is in this set on the `in globalThis` limb, not the `typeof` one — + * `typeof undefined` is the string `'undefined'` for a global that genuinely + * exists, so a typeof-only probe would have called the one global whose VALUE is + * undefined absent. The probe asks both questions for that reason. */ -const GLOBALS: ReadonlySet = new Set([ +export const SANDBOX_GLOBALS: ReadonlySet = new Set([ // Value/namespace globals 'Math', 'JSON', 'Date', 'Object', 'Array', 'String', 'Number', 'Boolean', 'RegExp', 'Map', 'Set', 'WeakMap', 'WeakSet', 'Promise', 'Symbol', 'BigInt', - 'Function', 'Reflect', 'Proxy', 'Intl', + 'Function', 'Reflect', 'Proxy', 'ArrayBuffer', 'SharedArrayBuffer', 'DataView', 'Int8Array', 'Uint8Array', 'Uint8ClampedArray', 'Int16Array', 'Uint16Array', 'Int32Array', 'Uint32Array', 'Float32Array', 'Float64Array', 'BigInt64Array', 'BigUint64Array', @@ -50,17 +70,66 @@ const GLOBALS: ReadonlySet = new Set([ // Global functions 'parseInt', 'parseFloat', 'isNaN', 'isFinite', 'encodeURIComponent', 'decodeURIComponent', 'encodeURI', 'decodeURI', + // Literal-ish globals + 'undefined', 'NaN', 'Infinity', 'globalThis', +]); + +/** + * Identifiers the NODE HOST provides that the sandbox does NOT — measured + * absent by the same probe, from the same allowlist this file used to hold as + * one generous list. + * + * A free reference to one of these is REPORTED (`FreeIdentifierResult.nodeOnly`) + * rather than waved through, and {@link detectFreeIdentifiers}'s caller turns it + * into a lowering refusal that names the identifier and the remedy. The refusal + * is the SAFE direction and costs nothing an author can lose: `lowerCallables` + * catches it and ships the handler through the `.mjs` bundle, which runs + * in-process in Node, where these names are real. What changes is that the + * platform stops emitting a `body.source` it knows cannot run. + * + * ⛔ This set is NOT a wish-list for sandbox capabilities. Moving a name out of + * it means the shipped QuickJS build gained the global — a runtime change, + * measured by the probe, never an edit here. + */ +export const NODE_ONLY_GLOBALS: ReadonlySet = new Set([ + // ECMA-402. Standard in every browser and in Node; absent from this QuickJS + // build. The name the defect was reported under. + 'Intl', + // Host/Web platform additions, not ECMAScript. QuickJS is the language, not + // the platform — nothing installs these into the VM. 'structuredClone', 'queueMicrotask', 'atob', 'btoa', 'setTimeout', 'clearTimeout', 'setInterval', 'clearInterval', - // Web-ish that the sandbox / Node commonly provide - 'URL', 'URLSearchParams', 'TextEncoder', 'TextDecoder', 'console', - // Literal-ish globals & implicit bindings - 'undefined', 'NaN', 'Infinity', 'globalThis', 'arguments', + 'URL', 'URLSearchParams', 'TextEncoder', 'TextDecoder', + // `console` is a HOST object, and the sandbox deliberately routes logging + // through the capability-gated `ctx.log` instead — see `buildBodyLogSurface` + // in the runtime's body runner. A lowered body calling `console.log` throws. + 'console', + // The one member that is not a global at all: `arguments` is an implicit + // binding of ordinary function scope, and the runner wraps a lowered body in + // an ARROW (`(async (ctx) => { … })(ctx)`), which provides none. It measures + // absent for a different reason than the rest and is refused for the same + // one — a body naming it throws where the raw `function (ctx) {…}` handler, + // run in-process, does not. + 'arguments', ]); export interface FreeIdentifierResult { - /** Sorted, de-duplicated free identifier names (empty when self-contained). */ + /** + * Sorted, de-duplicated names the handler references and NOTHING in the + * lowered body's world binds — module-scope helpers/imports/consts AND the + * {@link NODE_ONLY_GLOBALS} the sandbox does not provide. Empty when the + * handler really is self-contained inside the sandbox. + */ free: string[]; + /** + * The subset of {@link free} that the Node HOST provides but the sandbox does + * not. Carried separately because the two halves have opposite remedies: a + * module-scope name can be inlined into the handler, a host-only global + * cannot be inlined at all and needs a string handler ref or a validation + * rule. Callers that flatten this back into one list re-create the paragraph + * of English this field exists to replace. + */ + nodeOnly: string[]; /** True when the source could not be parsed into a single function node. */ unparsed: boolean; } @@ -220,22 +289,29 @@ function collectReferences(fn: ts.FunctionLikeDeclarationBase): Set { /** * Compute the free identifiers of a handler function source. - * Returns `{ free: [], unparsed: true }` when the source can't be parsed — the - * caller treats "unparsed" as "don't block extraction" (conservative). + * Returns `{ free: [], nodeOnly: [], unparsed: true }` when the source can't be + * parsed — the caller treats "unparsed" as "don't block extraction" + * (conservative). */ export function detectFreeIdentifiers(rawFunctionSource: string): FreeIdentifierResult { const fn = parseFunction(rawFunctionSource); - if (!fn) return { free: [], unparsed: true }; + if (!fn) return { free: [], nodeOnly: [], unparsed: true }; const bound = collectBindings(fn); const refs = collectReferences(fn); const free: string[] = []; + const nodeOnly: string[] = []; for (const name of refs) { if (bound.has(name)) continue; - if (GLOBALS.has(name)) continue; + // Only the SANDBOX set waives a name. A `NODE_ONLY_GLOBALS` member falls + // through to `free` on purpose — that is the whole fix — and is ALSO + // recorded in `nodeOnly` so the refusal can name the right remedy. + if (SANDBOX_GLOBALS.has(name)) continue; free.push(name); + if (NODE_ONLY_GLOBALS.has(name)) nodeOnly.push(name); } free.sort(); - return { free, unparsed: false }; + nodeOnly.sort(); + return { free, nodeOnly, unparsed: false }; } diff --git a/packages/cli/src/utils/extract-hook-body.ts b/packages/cli/src/utils/extract-hook-body.ts index 783c4d8ab4..3318940380 100644 --- a/packages/cli/src/utils/extract-hook-body.ts +++ b/packages/cli/src/utils/extract-hook-body.ts @@ -71,6 +71,16 @@ * (helper, import, top-level const) cannot be shipped body-only — the reference * would `ReferenceError` at runtime. {@link detectFreeIdentifiers} finds those; * extraction throws so the caller falls back to bundling the real closure. + * + * Sandbox-missing HOST globals (#14301) arrive through that same gate and for + * the same reason, once the ambient allowlist stopped conflating "Node has it" + * with "the sandbox has it". `Intl` was allowlisted next to `JSON`, so a handler + * calling `Intl.DateTimeFormat` had no free identifier at all: it lowered, every + * local gate stayed green, and the body threw `ReferenceError` in production — + * under a hook's default `onError: 'abort'`, refusing every write to the object. + * Those names are now REPORTED (`FreeIdentifierResult.nodeOnly`) and refused + * with prose that names the identifier and the remedy, which is NOT the + * module-scope one: a host global cannot be inlined. */ import { detectFreeIdentifiers } from './detect-free-identifiers.js'; @@ -85,11 +95,16 @@ import { detectFreeIdentifiers } from './detect-free-identifiers.js'; * paragraph of English where it needed a category. So the two refusals that * mean OPPOSITE things to an author shared one undifferentiated fate: * - * `free-identifiers` the handler IS expressible as a metadata-only body. - * It references a module-scope helper/import/const, so - * the deployment shape silently changed from metadata to - * bundled closure — against what the author wrote. The - * remedy is local and mechanical (inline the value). + * `free-identifiers` the handler names something the lowered body's world + * does not bind, so the deployment shape silently + * changed from metadata to bundled closure — against + * what the author wrote. Two sub-cases, told apart by + * `nodeOnlyIdentifiers`: a module-scope helper/import/ + * const, whose remedy is local and mechanical (inline + * the value), and a global the NODE HOST has that the + * sandbox does not (#14301), which cannot be inlined at + * all — that one keeps the check in a string handler ref + * or moves it to a validation rule. * `forbidden-token` the handler is NOT expressible as a metadata-only body * at ALL. `fetch`/`require`/`process`/… are capabilities * the QuickJS sandbox does not have, so writing one IS @@ -123,18 +138,32 @@ export class HookBodyExtractionError extends Error { readonly originLabel: string; /** Names the handler referenced but does not bind — `free-identifiers` only. */ readonly freeIdentifiers: readonly string[]; + /** + * The subset of {@link freeIdentifiers} the NODE HOST provides and the QuickJS + * sandbox does not (#14301) — `Intl` the reported one. + * + * Same discipline as `kind` above, one level finer: the refusing rule already + * knows which half of the free list it is looking at, and a consumer that has + * to re-derive it from the message prose is back where #13651 started. The + * two halves have OPPOSITE remedies — a module-scope name is inlined into the + * handler, a host-only global cannot be inlined at all — so a consumer that + * prints one remedy for both prints a wrong one half the time. + */ + readonly nodeOnlyIdentifiers: readonly string[]; constructor( kind: HookBodyRefusalKind, originLabel: string, message: string, freeIdentifiers: readonly string[] = [], + nodeOnlyIdentifiers: readonly string[] = [], ) { super(message); this.name = 'HookBodyExtractionError'; this.kind = kind; this.originLabel = originLabel; this.freeIdentifiers = freeIdentifiers; + this.nodeOnlyIdentifiers = nodeOnlyIdentifiers; } } @@ -268,16 +297,14 @@ export function extractHookBody(fn: (...a: unknown[]) => unknown, originLabel: s // throw — the caller catches this and keeps the handler in the BUNDLED form, // where esbuild carries the real closure along. The whole `fn` source (params // included) is analyzed so parameters are correctly in scope. - const { free, unparsed } = detectFreeIdentifiers(raw); + const { free, nodeOnly, unparsed } = detectFreeIdentifiers(raw); if (!unparsed && free.length > 0) { throw new HookBodyExtractionError( 'free-identifiers', originLabel, - `[hook-body-extract] ${originLabel}: handler references identifier(s) not in scope at runtime: ` + - `${free.join(', ')}. Module-scope helpers/imports aren't shipped with a metadata-only body, so ` + - `this handler will be BUNDLED instead (no behavior change). To make it body-only, inline the ` + - `helper(s) into the handler or move the logic behind \`ctx\` (e.g. \`ctx.api\`).`, + freeIdentifierRefusal(originLabel, free, nodeOnly), free, + nodeOnly, ); } @@ -294,6 +321,79 @@ export function extractHookBody(fn: (...a: unknown[]) => unknown, originLabel: s }; } +/** + * The closed set of host names the sandbox answers with a FIRST-CLASS + * replacement, so the refusal can name it. + * + * Deliberately one entry, not a mechanism. `console` is the host-only name an + * ordinary handler is most likely to reach for, and "keep it in a string + * handler ref" is poor advice for a log line when the platform's own answer is + * one capability away. Every other member of `NODE_ONLY_GLOBALS` has no + * in-sandbox equivalent to point at, and inventing one per name would be a + * second, unmeasured contract growing beside the measured set. + */ +const SANDBOX_REPLACEMENTS: Readonly> = { + console: 'For logging specifically the sandbox has `ctx.log` — declare the `log` capability and ' + + 'call `ctx.log.info(...)`.', +}; + +/** + * The prose for a `free-identifiers` refusal. + * + * ⛔ The module-scope-only sentence is BYTE-IDENTICAL to what this function + * threw before #14301 — `os build`'s warn-and-bundle line, `--strict-body`'s + * per-callable diagnostic and `content/docs/automation/hook-bodies.mdx` all + * quote it, and a refusal that reads differently would be a documentation break + * wearing a refactor's clothes. The host-only branch is ADDITIONAL prose for a + * case that could not previously arrive here at all (the names were allowlisted + * away), so it breaks no quotation. + * + * The two branches exist because the remedies are opposite and a wrong remedy + * is worse than none: "inline the helper" is impossible for `Intl`, and an + * author (or a code-writing model) who tries it lands on a second broken shape. + */ +function freeIdentifierRefusal( + originLabel: string, + free: readonly string[], + nodeOnly: readonly string[], +): string { + const head = + `[hook-body-extract] ${originLabel}: handler references identifier(s) not in scope at runtime: ` + + `${free.join(', ')}. `; + + if (nodeOnly.length === 0) { + return ( + head + + `Module-scope helpers/imports aren't shipped with a metadata-only body, so ` + + `this handler will be BUNDLED instead (no behavior change). To make it body-only, inline the ` + + `helper(s) into the handler or move the logic behind \`ctx\` (e.g. \`ctx.api\`).` + ); + } + + const names = nodeOnly.join(', '); + const isAre = nodeOnly.length === 1 ? 'is' : 'are'; + const itThem = nodeOnly.length === 1 ? 'it' : 'them'; + const moduleScope = free.filter((n) => !nodeOnly.includes(n)); + const tail = + moduleScope.length > 0 + ? ` The remaining name(s) — ${moduleScope.join(', ')} — are module-scope helpers/imports: ` + + `inline them into the handler or move the logic behind \`ctx\` (e.g. \`ctx.api\`).` + : ''; + const hint = nodeOnly.map((n) => SANDBOX_REPLACEMENTS[n]).filter(Boolean).join(' '); + + return ( + head + + `${names} ${isAre} not available in the hook sandbox — the QuickJS build the runtime evaluates a ` + + `lowered body in does not provide ${itThem}, so the body would throw ReferenceError in production ` + + `while validate, typecheck, test and build all stay green (they run the raw function in Node, ` + + `where ${itThem} exist${nodeOnly.length === 1 ? 's' : ''}). This handler will be BUNDLED instead ` + + `(no behavior change). To keep it as metadata, keep the check in a string handler ref — put the ` + + `function in the top-level \`functions:\` map and write \`handler: 'fn_name'\` — or move it to a ` + + `validation rule.${hint ? ' ' + hint : ''}` + + tail + ); +} + interface PeeledBody { source: string; isExpression: boolean; diff --git a/packages/cli/src/utils/sandbox-globals-probe.test.ts b/packages/cli/src/utils/sandbox-globals-probe.test.ts new file mode 100644 index 0000000000..2f0e91ce7f --- /dev/null +++ b/packages/cli/src/utils/sandbox-globals-probe.test.ts @@ -0,0 +1,121 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * The MEASUREMENT behind `detect-free-identifiers.ts`'s two-set split (#14301). + * + * ## Why this pin exists rather than a comment saying the lists were checked + * + * The defect it closes was a list written from memory. `GLOBALS` allowlisted + * `Intl` beside `JSON` under the comment "Web-ish that the sandbox / Node + * commonly provide" — true of Node, false of the sandbox — so a handler calling + * `Intl.DateTimeFormat` had no free identifier, lowered into `body.source`, and + * threw `ReferenceError` in production while `validate`, `typecheck`, `test` + * and `build` were all green (the in-process test runs the RAW function in + * Node, where `Intl` exists). A second hand-written list would fail the same + * way, silently, the first time the sandbox's globals moved. + * + * So membership is not asserted from knowledge here — it is READ OUT of the + * shipped QuickJS build and the two sets are required to equal what it says. + * A name added to either set from memory reddens this file. + * + * ## What "the same sandbox the runtime uses" means concretely + * + * `QuickJSScriptRunner` is the `ScriptRunner` `AppPlugin` wires for hook and + * action bodies, and `runScript` here takes the identical path a lowered body + * takes: a fresh `newQuickJSWASMModule()` per invocation, the same + * `(async (ctx) => { … })(ctx)` wrapper, the same empty capability set a body + * with no inferred capabilities is evaluated under. Anything the runner + * installs into the VM therefore counts as provided, and anything it does not + * counts as absent — which is the question the allowlist is answering. + * + * ⚠️ `@objectstack/runtime` resolves through its `exports` to **dist** from this + * package (it is one of the registered entries in + * `KNOWN_UNALIASED_TEST_IMPORTS['@objectstack/cli']`), so + * `pnpm --filter '@objectstack/cli^...' build` is a precondition for running + * this file. That does not weaken the reading: what is being measured is the + * global object of the `quickjs-emscripten` WASM build resolved from + * `node_modules`, which is the same artifact whether the thin runner wrapper + * around it came from `src` or `dist`. + * + * ## Why the probe asks two questions per name + * + * `typeof X !== 'undefined'` alone would report the one global whose VALUE is + * `undefined` — `undefined` itself — as absent, and quietly demand it be listed + * as host-only. `'X' in globalThis` answers existence instead of value, so the + * two limbs together say "does this name resolve", which is the question a free + * identifier actually poses. Names that are not globals at all (`arguments`, + * an implicit binding that the arrow-function wrapper does not provide) answer + * `false` on both limbs — correctly: a lowered body naming one throws. + */ + +import { describe, it, expect } from 'vitest'; +import { QuickJSScriptRunner } from '@objectstack/runtime'; +import type { ScriptContext } from '@objectstack/runtime'; +import { SANDBOX_GLOBALS, NODE_ONLY_GLOBALS } from './detect-free-identifiers.js'; + +const sorted = (xs: Iterable): string[] => [...xs].sort(); + +/** + * Anti-vacuity controls. Without them a probe that answered `true` for + * everything (or crashed into an empty object) would still satisfy the + * partition assertions whenever one side happened to be empty. + */ +const CONTROL_PRESENT = 'Math'; +const CONTROL_ABSENT = '__objectstack_probe_control_absent__'; + +function probeSource(names: readonly string[]): string { + const entry = (n: string): string => + `${JSON.stringify(n)}: ((typeof ${n} !== 'undefined') || ` + + `(typeof globalThis !== 'undefined' && ${JSON.stringify(n)} in globalThis))`; + return `return { ${names.map(entry).join(', ')} };`; +} + +/** Evaluate the `typeof`/`in globalThis` probe INSIDE the real hook sandbox. */ +async function probe(names: readonly string[]): Promise> { + // A generous CPU budget: the probe itself is trivial, but every invocation + // instantiates a fresh WASM module and a loaded box can blow the stock 250ms + // hook budget on that fixed cost alone. + const runner = new QuickJSScriptRunner({ hookTimeoutMs: 20_000 }); + const result = await runner.runScript( + { language: 'js', source: probeSource(names), capabilities: [] }, + { input: {} } as ScriptContext, + { origin: { kind: 'hook', name: 'sandbox-globals-probe' } }, + ); + return result.value as Record; +} + +describe('sandbox global probe (#14301 — the allowlist is measured, not recalled)', () => { + it('answers both ways — the control pair', async () => { + const readings = await probe([CONTROL_PRESENT, CONTROL_ABSENT]); + expect(readings[CONTROL_PRESENT]).toBe(true); + expect(readings[CONTROL_ABSENT]).toBe(false); + }, 60_000); + + it('the two sets are exactly the sandbox present/absent partition', async () => { + const names = sorted([...SANDBOX_GLOBALS, ...NODE_ONLY_GLOBALS]); + // Disjoint and non-empty, asserted before the partition so a set-union + // mistake reads as itself rather than as a measurement disagreement. + expect(names.length).toBe(SANDBOX_GLOBALS.size + NODE_ONLY_GLOBALS.size); + expect(SANDBOX_GLOBALS.size).toBeGreaterThan(0); + expect(NODE_ONLY_GLOBALS.size).toBeGreaterThan(0); + + const readings = await probe(names); + // Every probed name came back with a boolean — a missing key would + // otherwise be silently counted as "absent". + expect(Object.keys(readings).sort()).toEqual(names); + + const measuredPresent = names.filter((n) => readings[n] === true); + const measuredAbsent = names.filter((n) => readings[n] === false); + + expect(measuredPresent).toEqual(sorted(SANDBOX_GLOBALS)); + expect(measuredAbsent).toEqual(sorted(NODE_ONLY_GLOBALS)); + }, 60_000); + + it("the card's named case and its positive control", () => { + // Implied by the partition above; spelled out because these two names are + // what the report was about, and a reader should not have to re-derive + // them from a measurement to see the verdict. + expect(NODE_ONLY_GLOBALS.has('Intl')).toBe(true); + expect(SANDBOX_GLOBALS.has('JSON')).toBe(true); + }); +}); diff --git a/packages/cli/test/extract-hook-body.test.ts b/packages/cli/test/extract-hook-body.test.ts index d465550950..30812071c9 100644 --- a/packages/cli/test/extract-hook-body.test.ts +++ b/packages/cli/test/extract-hook-body.test.ts @@ -193,6 +193,98 @@ describe('extractHookBody', () => { expect(() => extractHookBody(fn, 'hook free')).toThrow(/not in scope at runtime|moduleScopeHelper/); }); + // ── #14301 — a global the NODE HOST has and the sandbox does not ──────── + // + // The reported shape. `Intl` was allowlisted beside `JSON`, so this handler + // had no free identifier at all: it lowered, `validate`/`typecheck`/`test`/ + // `build` were all green (the in-process test runs the RAW function in Node), + // and the body threw `ReferenceError: Intl is not defined` in production — + // under a hook's default `onError: 'abort'`, refusing every write to the + // object. Refusing here routes it through the SAME path as #1876: the + // callable is still bundled and still runs in-process, only the unrunnable + // body is declined. + it('refuses a handler referencing Intl, with the reason named (#14301)', () => { + const fn = (ctx: any) => { + const f = new Intl.DateTimeFormat('en-US', { timeZone: 'UTC' }); + ctx.input.due_label = f.format(new Date(ctx.input.due_at)); + }; + let err: any; + try { + extractHookBody(fn as any, "hook 'stamp_due_label'"); + } catch (e) { + err = e; + } + expect(err, 'the handler must be REFUSED, not lowered').toBeDefined(); + expect(err.kind).toBe('free-identifiers'); + expect(err.freeIdentifiers).toEqual(['Intl']); + // The classification the refusing rule already had, carried beside the + // prose rather than buried in it — `os lint` reads this, not the sentence. + expect(err.nodeOnlyIdentifiers).toEqual(['Intl']); + // The reason NAMES the identifier and the remedy, and the remedy is not + // the module-scope one: there is nothing to inline `Intl` from. + expect(err.message).toContain('Intl'); + expect(err.message).toContain('not available in the hook sandbox'); + expect(err.message).toContain('string handler ref'); + expect(err.message).toContain('validation rule'); + expect(err.message).not.toContain('Module-scope helpers/imports'); + }); + + // The reverse leg: the sibling globals the sandbox DOES provide must still + // lower, or the fix would have traded a silent production failure for a + // repo-wide fallback to bundling. + it('still extracts a handler using sandbox-provided globals only (#14301)', () => { + const fn = (ctx: any) => { + ctx.input.at = new Date(ctx.input.raw).toISOString(); + ctx.input.tags = JSON.stringify(ctx.input.list ?? []); + ctx.input.n = Math.round(Number(ctx.input.n)); + }; + const ext = extractHookBody(fn as any, 'hook plain globals'); + expect(ext.source).toContain('toISOString'); + }); + + // ⛔ The module-scope sentence is quoted verbatim by + // `content/docs/automation/hook-bodies.mdx`, by `os build`'s warn-and-bundle + // line and by `--strict-body`'s per-callable diagnostic. #14301 added a + // second branch beside it; this pin is what keeps the branch from rewriting + // the original. + it('leaves the module-scope refusal sentence byte-identical (#14301)', () => { + const fn = (ctx: any) => { + ctx.record.slug = moduleScopeHelper(ctx.record.name); + }; + let err: any; + try { + extractHookBody(fn as any, 'hook free'); + } catch (e) { + err = e; + } + expect(err.nodeOnlyIdentifiers).toEqual([]); + expect(err.message).toBe( + "[hook-body-extract] hook free: handler references identifier(s) not in scope at runtime: " + + "moduleScopeHelper. Module-scope helpers/imports aren't shipped with a metadata-only body, " + + "so this handler will be BUNDLED instead (no behavior change). To make it body-only, inline " + + "the helper(s) into the handler or move the logic behind `ctx` (e.g. `ctx.api`).", + ); + }); + + it('names both halves when a handler has each kind of free name (#14301)', () => { + const fn = (ctx: any) => { + ctx.record.slug = moduleScopeHelper(ctx.record.name); + ctx.record.fmt = Intl.NumberFormat; + }; + let err: any; + try { + extractHookBody(fn as any, 'hook mixed'); + } catch (e) { + err = e; + } + expect(err.freeIdentifiers).toEqual(['Intl', 'moduleScopeHelper']); + expect(err.nodeOnlyIdentifiers).toEqual(['Intl']); + // Each half gets ITS remedy; neither is printed for the other. + expect(err.message).toContain('not available in the hook sandbox'); + expect(err.message).toContain('module-scope helpers/imports'); + expect(err.message).toContain('moduleScopeHelper'); + }); + // ── `sudo()` is not a body-reachable member (#14010) ──────────────────── // // `ScopedContext.sudo()` is REAL in-process and absent from the VM's