diff --git a/.changeset/hook-body-sudo-is-not-reachable.md b/.changeset/hook-body-sudo-is-not-reachable.md new file mode 100644 index 0000000000..5a17737724 --- /dev/null +++ b/.changeset/hook-body-sudo-is-not-reachable.md @@ -0,0 +1,47 @@ +--- +"@objectstack/cli": patch +"@objectstack/lint": patch +--- + +fix(cli,lint): stop lowering hook handlers that call `ctx.api.sudo()` into bodies that cannot run it (#14010) + +`ScopedContext.sudo()` is real in-process and is **not** marshalled into the +QuickJS sandbox: the VM's `ctx.api` carries `object()` and the transaction +surface, and nothing else. Every consumer of that fact had it backwards. + +The failure this closes is the expensive shape, not a cosmetic one. An author +writes an inline `handler`, tests it the way the docs teach — calling +`hook.handler(ctx)` natively, against the in-process `ScopedContext`, where +`sudo()` exists — and the suite is green. `objectstack build` then lowers that +same source into an L2 `body`, and in production the call is +`TypeError: ctx.api.sudo is not a function`. Under a hook's default +`onError: 'abort'` the TypeError aborts the **triggering write**, so the +symptom surfaces as an unrelated save being refused. Green tests, dead feature. + +- **`@objectstack/cli`** — `.sudo(` joins `FORBIDDEN_PATTERNS` in + `extractHookBody`, so the build declines to emit such a handler as + `body.source`. This is a repair, not just a refusal: `lowerCallables` already + registers the callable and ships it through the `.mjs` bundle when extraction + throws, so the handler keeps running **in-process, where `sudo()` is real**. + The build prints the reason; `--strict-body`, which demands a body for every + callable, turns it into a hard failure — correctly, since a body needing + elevation genuinely cannot be one. Same family as the `crypto.hash` + retirement (#4391): a member advertised ahead of its implementation, where + build-time inference was the amplifier rather than the safety net. +- **`@objectstack/lint`** — `hook-api-update-readonly-field` (severity + `error`, gating) and its `readonlyWhen` sibling both *prescribed* + `ctx.api.sudo()` as the remedy. That rule reads L2 body sources and nothing + else, so the prescribed shape was a TypeError for **100%** of its population: + a gating rule pointing at a dead feature. Both hints now name the own-hook + stamp and say plainly that `sudo()` is not reachable from a body. The rule's + findings, severities and exclusions are unchanged — only the advice. + +Docs: the `readonly` table in `automation/hook-bodies.mdx` claimed the +`sudo()` row **Lands**; it now records what actually happens. + +Not addressed here, and the reason this is only half the card: a hook still has +**no declared elevation knob** — there is no hook-side `runAs` the way +`FlowSchema` has one — so "this column is computed by automation and never +hand-written" remains inexpressible whenever the maintaining write is +cross-object. That is a contract-surface decision (see #14010), left to the +review chain rather than guessed at here. diff --git a/content/docs/automation/hook-bodies.mdx b/content/docs/automation/hook-bodies.mdx index 2aceb592e3..0dabec0a61 100644 --- a/content/docs/automation/hook-bodies.mdx +++ b/content/docs/automation/hook-bodies.mdx @@ -247,7 +247,7 @@ A body may write *other* objects — e.g. `await ctx.api.object('parent').update ### Writing a `readonly` field -There is an asymmetry here that costs data if you learn it the hard way, so learn it here. A field declared `readonly: true` can still be **maintained by automation** — but only through two channels, and a nested `ctx.api` write is **not** one of them. +There is an asymmetry here that costs data if you learn it the hard way, so learn it here. A field declared `readonly: true` can still be **maintained by automation** from a body — but through exactly **one** channel (the own-hook stamp, plus INSERT), and a nested `ctx.api` write is **not** one of them. `readonly` governs the *caller* surface. On UPDATE the engine strips read-only keys from the payload, but only the ones the **caller supplied** and only when the value is still the caller's. So: @@ -255,13 +255,13 @@ There is an asymmetry here that costs data if you learn it the hard way, so lear |:---|:---| | `ctx.input. = …` in `beforeInsert`/`beforeUpdate` | **Lands.** The stamp is a *server* value, not a caller-supplied one, so the strip leaves it alone. This is the recommended shape. | | `ctx.api.object('x').update({ })` | **Silently dropped.** `ctx.api` is scoped to the *triggering* operation's context, so on any non-system trigger the payload is an ordinary caller payload and the key is stripped. The call still returns success. | -| `ctx.api.sudo().object('x').update({ })` | **Lands.** `sudo()` elevates to a system context, which the strip skips — the hook-side analogue of a flow's `runAs: 'system'`. Use it deliberately: it also bypasses the acting user's row and field permissions for that write. | +| `ctx.api.sudo().object('x').update({ })` | **`TypeError` — not available here.** `sudo()` is a member of the *in-process* `ScopedContext`; the VM's `ctx.api` carries `object()` and `transaction()` and nothing else, so a **body** cannot reach it. Worse than unavailable: the same source *works* when the handler runs in-process, so it passes a native `hook.handler(ctx)` test and throws only once the build lowers it into a body — aborting the triggering write under the default `onError: 'abort'`. `objectstack build` now refuses to lower such a handler and keeps it bundled instead. A hook has **no** declared elevation knob (no hook-side `runAs`); [#14010](https://github.com/objectstack-ai/objectstack/issues/14010) is where that gap is argued. | | `ctx.api.object('x').insert({ })` | **Lands.** INSERT is exempt — a create may legitimately seed read-only columns. | The dropped case is the dangerous one: nothing fails, the step reports success, and the column is simply always null. Because both halves of that judgement are declared in your own stack, it is checked at author time and **gates the build**: - `hook-api-update-readonly-field` — **error**. A body's literal `ctx.api.object('…').update()` / `.updateById()` writes a field the named object declares `readonly: true`. -- `hook-api-update-readonly-when-field` — **warning**. The same write against a `readonlyWhen` field, which strips per record *state*. Note that `readonlyWhen` also strips a `beforeUpdate`-derived value, so the own-hook stamp is **not** a workaround for it — `sudo()` is. +- `hook-api-update-readonly-when-field` — **warning**. The same write against a `readonlyWhen` field, which strips per record *state*. Note that `readonlyWhen` also strips a `beforeUpdate`-derived value, so the own-hook stamp is **not** a workaround for it — and neither is `sudo()`, which a body cannot reach (see the row above). On this shape, confirm the write only targets records whose predicate is `false`, or drop the field from the payload. Only literal object names and literal payload keys are seen; a `sudo()` chain, a dynamic object name, an object this stack does not declare, and `insert`/`create` are all skipped, so the rule has no opinion on them. The flow surface has carried the same gate as `flow-update-readonly-field` since [#3425](https://github.com/objectstack-ai/objectstack/issues/3425). diff --git a/packages/cli/src/utils/extract-hook-body.ts b/packages/cli/src/utils/extract-hook-body.ts index 68d5665359..6b76a59c38 100644 --- a/packages/cli/src/utils/extract-hook-body.ts +++ b/packages/cli/src/utils/extract-hook-body.ts @@ -14,7 +14,7 @@ * For v1 we apply a deliberately simple **regex allow-list** over the * extracted body — full TypeScript AST analysis is deferred to v2. Anything * the regex rejects (top-level `import`, `require(` / esbuild's `__require(`, - * `fetch(`, `process.*`, `globalThis.*`, `eval`, `new Function`) makes + * `fetch(`, `process.*`, `globalThis.*`, `eval`, `new Function`, `.sudo(`) makes * extraction **throw**. * * ⚠️ What that throw costs the BUILD depends on the flag, and the two outcomes @@ -85,6 +85,35 @@ const FORBIDDEN_PATTERNS: Array<{ rx: RegExp; reason: string }> = [ { rx: /\bglobalThis\s*\./, reason: '`globalThis` access is not allowed in hook/action bodies' }, { rx: /\beval\s*\(/, reason: '`eval()` is not allowed in hook/action bodies' }, { rx: /\bnew\s+Function\s*\(/, reason: '`new Function()` is not allowed in hook/action bodies' }, + // [#14010] `sudo()` exists on the HOST `ScopedContext` and is NOT marshalled + // into the VM, so lowering a handler that calls it turns working in-process + // code into a `TypeError` that only production sees. Refusing here is what + // makes the two runtimes agree: the callable is still registered in + // `functions` and still shipped through the `.mjs` bundle by `lowerCallables`, + // so the handler keeps running in-process where `sudo()` is real — the build + // just declines to ALSO emit it as a body that cannot run. + // + // Same family as the `crypto.hash` retirement three lines into + // CAPABILITY_PATTERNS below (#4391): a member advertised ahead of its + // implementation, where the build-time inference was the amplifier rather + // than the safety net. The difference is the remedy — `crypto.hash` had no + // working channel to fall back to, this one does. + // + // Receiver-loose, like the `.object(...)` / `.title(...)` capability patterns: + // a local alias (`const api = ctx.api; api.sudo()`) must not slip through, + // and over-refusal is the SAFE direction here (the handler is bundled and + // works; it is only `--strict-body`, which demands a body for every callable, + // that turns this into a hard failure — correctly, since a body needing + // elevation genuinely cannot be one). + { + rx: /\.\s*sudo\s*\(/, + reason: + '`sudo()` is not reachable from a sandboxed body — the VM\'s `ctx.api` carries only `object()` ' + + 'and `transaction()`, so the call is a TypeError at run time (and under a hook\'s default ' + + '`onError: \'abort\'` that aborts the triggering write). Stamp the value from the record\'s own ' + + 'before-hook (`ctx.input. = ...`), or leave this handler bundled so it runs in-process ' + + 'where `sudo()` exists', + }, ]; const CAPABILITY_PATTERNS: Array<{ rx: RegExp; cap: 'api.read' | 'api.write' | 'crypto.uuid' | 'log' }> = [ diff --git a/packages/cli/test/extract-hook-body.test.ts b/packages/cli/test/extract-hook-body.test.ts index b3f7919658..d465550950 100644 --- a/packages/cli/test/extract-hook-body.test.ts +++ b/packages/cli/test/extract-hook-body.test.ts @@ -193,6 +193,46 @@ describe('extractHookBody', () => { expect(() => extractHookBody(fn, 'hook free')).toThrow(/not in scope at runtime|moduleScopeHelper/); }); + // ── `sudo()` is not a body-reachable member (#14010) ──────────────────── + // + // `ScopedContext.sudo()` is REAL in-process and absent from the VM's + // `ctx.api`, so the same handler source passes a native `hook.handler(ctx)` + // test and TypeErrors once the build lowers it into a body. Refusing the + // extraction is what keeps the two runtimes honest: `lowerCallables` catches + // this throw and ships the callable through the .mjs bundle, so the handler + // keeps working in-process — only the unrunnable body is declined. + it('rejects a handler calling ctx.api.sudo() (#14010)', () => { + const fn = async (ctx: any) => { + await ctx.api.sudo().object('crm_account').update({ id: ctx.input.id, current_grade: 'A' }); + }; + expect(() => extractHookBody(fn, 'hook elevate')).toThrow(/`sudo\(\)` is not reachable/); + }); + + it('rejects the aliased receiver too — `const api = ctx.api; api.sudo()` (#14010)', () => { + // Receiver-loose on purpose: under-refusing here is the failure that only + // production sees, which is the whole defect. + const fn = async (ctx: any) => { + const api = ctx.api; + await api.sudo().object('crm_account').update({ id: ctx.input.id, x: 1 }); + }; + expect(() => extractHookBody(fn, 'hook elevate alias')).toThrow(/`sudo\(\)` is not reachable/); + }); + + // The reverse leg: without the pattern this body extracts CLEANLY and the + // build emits a `body.source` that TypeErrors in the sandbox. Asserting the + // ordinary shape still passes is what proves the pattern did not widen into + // the majority case it sits beside. + it('still extracts an ordinary non-elevated ctx.api write (#14010)', () => { + const fn = async (ctx: any) => { + await ctx.api.object('crm_account').update({ id: ctx.input.id, current_grade: 'A' }); + }; + const ext = extractHookBody(fn, 'hook plain'); + expect(ext.capabilities).toContain('api.write'); + // Quote-agnostic: this file is itself bundled, and esbuild rewrites the + // literal's quotes before `String(fn)` ever runs. + expect(ext.source).toMatch(/object\((['"])crm_account\1\)/); + }); + it('extracts a self-contained handler that only uses params + globals (#1876)', () => { const fn = (ctx: any) => { ctx.record.id = Math.round(Number(ctx.record.raw)); diff --git a/packages/lint/src/validate-readonly-hook-writes.test.ts b/packages/lint/src/validate-readonly-hook-writes.test.ts index 2fd2205b6c..3d90a395dd 100644 --- a/packages/lint/src/validate-readonly-hook-writes.test.ts +++ b/packages/lint/src/validate-readonly-hook-writes.test.ts @@ -74,11 +74,16 @@ describe('validateReadonlyHookWrites - RED: a ctx.api write to a readonly field' expect(findings[0].path).toBe('hooks[0].body.source'); expect(findings[0].message).toContain("'last_activity_date'"); expect(findings[0].message).toContain('crm_account'); - // The remedy must name BOTH legitimate channels, not only sudo - telling an - // author to elevate is a security-relevant instruction, and the own-hook - // stamp is the shape that needs no elevation at all. + // [#14010] The remedy is the own-hook stamp, and the hint must say so. expect(findings[0].hint).toContain('ctx.input.last_activity_date'); - expect(findings[0].hint).toContain('sudo'); + // ...and it must NOT prescribe sudo. This rule reads L2 body sources, which + // run in QuickJS, whose ctx.api has no `sudo` - so the hint used to point + // 100% of its population at a TypeError (aborting the triggering write, + // under the default onError:'abort'). Substring-matching 'sudo' is not + // enough to tell "offers it" from "warns against it": the corrected hint + // still contains the word. Pin the DIRECTION. + expect(findings[0].hint).toMatch(/sudo\(\) is NOT an option|not marshalled into the sandbox/); + expect(findings[0].hint).not.toMatch(/make the elevation explicit|write it through ctx\.api\.sudo/); }); it('flags updateById, whose payload is argument 1', () => { @@ -344,7 +349,11 @@ describe('validateReadonlyHookWrites - readonlyWhen is a SECOND shape, not the s expect(findings[0].severity).toBe('warning'); // The own-hook stamp is NOT the remedy here, and the hint must not offer it. expect(findings[0].hint).not.toContain('ctx.input.credit_hold'); - expect(findings[0].hint).toContain('sudo'); + // [#14010] Nor is sudo, for the sandbox-reachability reason above - so this + // hint offers NEITHER, and says which record states the write is safe on. + expect(findings[0].hint).toContain('not marshalled into the sandbox'); + expect(findings[0].hint).not.toMatch(/write it through ctx\.api\.sudo/); + expect(findings[0].hint).toContain('readonlyWhen predicate is FALSE'); }); it('reports a field carrying BOTH flags as the certain (static readonly) finding', () => { diff --git a/packages/lint/src/validate-readonly-hook-writes.ts b/packages/lint/src/validate-readonly-hook-writes.ts index 56d3e089b3..48f191cd4b 100644 --- a/packages/lint/src/validate-readonly-hook-writes.ts +++ b/packages/lint/src/validate-readonly-hook-writes.ts @@ -46,14 +46,30 @@ // flagged. Exactly the reason the flow sibling skips `create_record`. // // - Only a NON-ELEVATED `ctx.api`. `ScopedContext.sudo()` returns a context -// with `isSystem: true`, which the strip skips entirely - the hook-side -// analogue of a flow's `runAs:'system'`, and the intended channel for -// "users cannot edit this, but automation maintains it". A `.sudo()` chain +// with `isSystem: true`, which the strip skips entirely. A `.sudo()` chain // is structurally invisible to the extractor (its `api-crud-literal` // matcher requires a literal `ctx.api` receiver, and `ctx.api.sudo()` is a // CallExpression), so elevated writes cannot be flagged even by accident. // Measured, not assumed - `validate-readonly-hook-writes.test.ts` pins it. // +// ⚠️ [#14010] What that exclusion must NOT become is a recommendation, and +// until this edit both hints below made it one. This rule reads L2 +// (`language:'js'`) BODIES - `extractHookBodyWriteSet` parses +// `hooks[i].body.source` and nothing else - and a body runs in QuickJS, +// whose VM-side `ctx.api` carries `object()` and the transaction leaves and +// NO `sudo` (`installCtx` in runtime/src/sandbox/quickjs-runner.ts; pinned +// exhaustively in `quickjs-runner.test.ts`). `sudo()` is real only on the +// HOST `ScopedContext` handed to an in-process `handler`. So the prescribed +// remedy was a `TypeError` for 100% of this rule's population - and under a +// hook's default `onError: 'abort'` that aborts the triggering write, which +// is a gating rule pointing at a dead feature. The exclusion stands (an +// elevated write is genuinely not stripped); the ADVICE does not. +// +// A hook still has no DECLARED elevation knob - there is no hook-side +// `runAs` - so the honest hint is the own-hook stamp, and #14010 is where +// the missing knob is argued. Issue ids stay in this comment, out of the +// message an author reads and cannot act on. +// // - Only a LITERAL object name and a LITERAL payload key. A dynamic object // (`ctx.api.object(name)`) or a non-literal payload yields no extraction at // all, so nothing is guessed. @@ -286,11 +302,11 @@ export function validateReadonlyHookWrites(stack: AnyRec): ReadonlyHookWriteFind `on every non-system trigger the engine strips readonly keys from that UPDATE payload - ` + `the write never lands, while the call still returns success.`, hint: - `If automation is meant to maintain '${w.field}', either stamp it on the record's OWN hook ` + - `(ctx.input.${w.field} = ... in beforeInsert/beforeUpdate survives the strip, and is the ` + - `recommended shape), or make the elevation explicit with ctx.api.sudo().object('${objectName}') ` + - `- deliberately, since sudo bypasses the acting user's row and field permissions for that write. ` + - `Otherwise drop readonly:true from '${w.field}'.`, + `If automation is meant to maintain '${w.field}', stamp it on the record's OWN hook - ` + + `ctx.input.${w.field} = ... in beforeInsert/beforeUpdate survives the strip, and is the ` + + `recommended shape. Note that ctx.api.sudo() is NOT an option from a body: sudo() lives on ` + + `the in-process ScopedContext and is not marshalled into the sandbox, so calling it here is a ` + + `TypeError at run time. Otherwise drop readonly:true from '${w.field}'.`, }); } else if (meta.readonlyWhen) { reported.add(dedupeKey); @@ -307,9 +323,9 @@ export function validateReadonlyHookWrites(stack: AnyRec): ReadonlyHookWriteFind `write may silently not land depending on the record's state.`, hint: `readonlyWhen strips even a beforeUpdate-derived value, so an own-hook stamp is NOT a ` + - `workaround here. If automation must maintain '${w.field}' regardless of record state, write it ` + - `through ctx.api.sudo(). Otherwise confirm this call only targets records whose readonlyWhen ` + - `predicate is FALSE.`, + `workaround here - and neither is ctx.api.sudo(), which is not marshalled into the sandbox ` + + `(calling it from a body is a TypeError at run time). Confirm this call only targets records ` + + `whose readonlyWhen predicate is FALSE, or drop '${w.field}' from this payload.`, }); } } diff --git a/packages/runtime/src/sandbox/quickjs-runner.test.ts b/packages/runtime/src/sandbox/quickjs-runner.test.ts index 42e99dc10d..dfc2a3d15b 100644 --- a/packages/runtime/src/sandbox/quickjs-runner.test.ts +++ b/packages/runtime/src/sandbox/quickjs-runner.test.ts @@ -203,6 +203,108 @@ describe('QuickJSScriptRunner — L2 hook script', () => { expect(extraMembers).toEqual([]); }); + // ── the `ctx.api` surface, and `sudo`'s absence from it (#14010) ───────── + // + // Same defect shape as the `crypto.hash` pins above, one layer out: a member + // that is REAL on the host object and simply not marshalled into the VM. + // `buildSandboxApi` (runtime/src/sandbox/body-runner.ts) hands a hook body the + // engine's own `ScopedContext`, which HAS `sudo()` (objectql/src/engine.ts); + // `installCtx` then rebuilds a VM-side `ctx.api` carrying `object` and the + // transaction surface only. So the same handler source passes a native + // `hook.handler(ctx)` test and TypeErrors once the build lowers it into a + // body - and under a hook's default `onError: 'abort'` that aborts the + // triggering write. Green tests, dead feature. + // + // This pin is the ABSENCE, recorded. It is not an argument that the absence + // is wrong: putting privilege escalation on the documented body surface is a + // maintainer call (`packages/spec/src/contracts/scoped-context.ts` says so in + // as many words), and a hook has no declared `runAs` to authorize it with. + // What the absence must never again be is UNDECLARED - it was documented as + // working, and `os build` lowered handlers into it. + + it('the VM ctx.api exposes exactly object + the transaction surface, under ANY grant (#14010)', async () => { + const allGrants = ['api.read', 'api.write', 'api.transaction', 'crypto.uuid', 'log'] as const; + // A host api that DOES carry sudo - the real ScopedContext does, so this is + // the production shape, not a stripped-down double. Any leak of the host + // object into the VM would show up here as an extra key. + const api = { + object: (_n: string) => ({ count: () => 1 }), + sudo: () => ({ object: (_n: string) => ({ update: () => 1 }) }), + }; + + const keys = await runner.runScript( + { + language: 'js', + source: 'return Object.keys(ctx.api).sort().join(",");', + capabilities: [...allGrants], + }, + ctx({ api }), + hookOpts, + ); + // Exhaustive on purpose, exactly like the ctx.crypto pin: a new member has + // to come through a review that also updates this list. + expect(keys.value).toBe('__txBegin,__txCommit,__txRollback,object,transaction'); + + // The specific regression, and the reason the host double above carries a + // real `sudo`: the member is NOT missing upstream, it stops at the VM + // boundary. A body cannot reach it by any spelling. + const typeofSudo = await runner.runScript( + { + language: 'js', + source: 'return typeof ctx.api.sudo;', + capabilities: [...allGrants], + }, + ctx({ api }), + hookOpts, + ); + expect(typeofSudo.value).toBe('undefined'); + }); + + it('a body calling ctx.api.sudo() throws, and the message names NOTHING (#14010)', async () => { + // The runtime half of the story the CLI now refuses to lower + // (`extract-hook-body.ts` FORBIDDEN_PATTERNS). Two things are pinned, and + // the second is why the refusal cannot live only here: + // + // 1. It THROWS rather than silently no-opping. That much is good: a + // silent drop would leave the computed column null for the life of + // the app (the failure mode `hook-api-update-readonly-field` exists + // to catch), whereas a throw is at least visible. + // 2. The message is `TypeError: not a function` — QuickJS names neither + // the member nor the receiver, so the one diagnostic production gets + // identifies no member, no file and no line. Under a hook's default + // `onError: 'abort'` this arrives as an unrelated SAVE being refused, + // and nothing in it points at `sudo`. + // + // A message this blind cannot be the place an author learns the rule, + // which is the argument for refusing at BUILD time, where the reason can + // be spelled out and the handler can still be routed to a runtime that + // supports it. This assertion is deliberately written against what QuickJS + // actually emits: if a future runner starts naming the member, that is an + // improvement and this pin should be updated to demand it. + let elevated = 0; + const api = { + object: (_n: string) => ({ update: () => 1 }), + sudo: () => { + elevated++; + return { object: (_n: string) => ({ update: () => 1 }) }; + }, + }; + await expect( + runner.runScript( + { + language: 'js', + source: "await ctx.api.sudo().object('crm_account').update({ grade: 'A' }); return 1;", + capabilities: ['api.read', 'api.write'], + }, + ctx({ api }), + hookOpts, + ), + ).rejects.toThrow(/TypeError: not a function/); + // And the host's real sudo was never reached through the VM — the failure + // is the marshalling boundary, not a broken host double. + expect(elevated).toBe(0); + }); + it('reports script-thrown errors with origin name', async () => { await expect( runner.runScript(