diff --git a/.changeset/action-record-load-denied-signal.md b/.changeset/action-record-load-denied-signal.md new file mode 100644 index 0000000000..f84c6dda97 --- /dev/null +++ b/.changeset/action-record-load-denied-signal.md @@ -0,0 +1,63 @@ +--- +"@objectstack/runtime": minor +--- + +fix(runtime): tell an action handler when its caller-scope record load was refused (#14143) + +Both action doors load the subject row under the **caller's** own execution +context, and both then stamp the requested id back onto the record: + +```ts +if (record && record.id == null && recordId) record.id = recordId; +``` + +A refused or empty load leaves `record` as `{}`, so `record.id` is exactly +`null` — which is the stamp's own condition. The stamp condition and the +load-failure condition **coincided**. An action body runs elevated +(`isSystem: true`, settled design — #3914), so authorization has to be +re-established inside the handler, and the predicate every author reaches for +first was therefore always false: + +```js +if (!ctx.record?.id) return refuse(); // never refused anything +``` + +An app author had to rediscover this by reading the dispatcher, or ship a guard +that passes on a row the caller cannot see. Undocumented, and identically broken +on both invocation paths. + +**The addition: `ctx.recordLoadDenied`.** `true` exactly when a caller-scope +load was **attempted** and did not deliver the row; **absent** — never `false` — +otherwise, matching the `referentialFieldClear` marker convention on the same +seam, so a handler reads `ctx.recordLoadDenied === true`. + +```js +if (ctx.recordLoadDenied) { + throw Object.assign(new Error('Record not available'), { code: 'RECORD_NOT_FOUND' }); +} +``` + +- **Purely additive.** Nothing is refused that was not refused before, no + existing key changes value, and the `recordId` stamp is deliberately + **kept**: new-record / record-less actions depend on it, so `ctx.record.id` + still arrives exactly as it did. Pinned in both directions. +- **Both doors, one producer.** REST `POST /api/v1/actions/...` and the MCP + `run_action` bridge now share `loadActionSubjectRecord`. A signal only one + door emitted would be an authorization guard silently inert on the other. +- **The body face too.** The sandbox `ctx` is a fixed key set, so the flag is + marshalled explicitly into the VM — an inline `body` (the surface an AI author + writes most) reads it exactly as a registered handler does. +- **Documented**, in `docs/ui/actions` ("Authorization inside an action") and + from the action-`ctx` section of `docs/automation/hook-bodies` — half the + defect was that none of this was written down anywhere. + +**What the flag does not claim.** It reports "the row did not resolve for this +caller", not "the platform caught an authorization error". A row hidden by +row-level security and an id that names nothing both arrive as +`RECORD_NOT_FOUND` / 404 — existence non-disclosure working as designed — and +nothing in the caught error separates them, so the flag carries no code or +status and does not pretend to. For an authorization decision the two are one +answer: this caller has not demonstrated read access to that row. + +The `isSystem` elevation itself is unchanged and is not the defect (#3914); no +call that reaches a handler today stops reaching it. diff --git a/content/docs/automation/hook-bodies.mdx b/content/docs/automation/hook-bodies.mdx index 7320dccf36..483a6bc0a6 100644 --- a/content/docs/automation/hook-bodies.mdx +++ b/content/docs/automation/hook-bodies.mdx @@ -238,6 +238,8 @@ await ctx.api.object('crm_deal').updateById(ctx.recordId, { stage: 'won' }); Mutating the snapshot *as a payload* and then handing it to such a call is fine — that write is live, and the lint leaves it alone. +`ctx.record` is also **not** an authorization input. An action body runs elevated, so any caller-specific rule has to be re-established inside it — and `ctx.record.id` carries the requested `recordId` even when the caller-scope load did **not** deliver the row, so `if (!ctx.record?.id) …` never refuses. The key that distinguishes the two is `ctx.recordLoadDenied` (`=== true` exactly when a load was attempted and returned nothing; absent otherwise). See [Authorization inside an action](/docs/ui/actions#authorization-inside-an-action). + ### Engine The sandbox engine is **`quickjs-emscripten`** — pure-WASM, runs on every JS host. We considered `isolated-vm` but its native dependency disqualifies edge targets. The choice is hidden behind the `ScriptRunner` interface in `packages/runtime/src/sandbox/`, so a node-only deployment can swap in a faster engine later without touching call sites. diff --git a/content/docs/ui/actions.mdx b/content/docs/ui/actions.mdx index fc4efb999b..a0b7053ed0 100644 --- a/content/docs/ui/actions.mdx +++ b/content/docs/ui/actions.mdx @@ -161,7 +161,10 @@ rejected at authoring time.) Also note that both data surfaces a body reaches — `ctx.api.object(name)` and the handler's `ctx.engine` facade — are **trusted**: they run under the caller's identity elevated to system, so they bypass row- and field-level security (writes stay attributed to the caller and -scoped to their organization). Enforce any caller-specific rules yourself. +scoped to their organization). Enforce any caller-specific rules yourself — and +read [Authorization inside an action](#authorization-inside-an-action) before +you write that guard, because `ctx.record.id` is present even when the caller +cannot read the row. @@ -301,6 +304,61 @@ is a spec proposal for a properly named key, not a values map under this one. - **`requiresFeature`** ties visibility to a feature flag (compiled into a `visible` predicate). +### Authorization inside an action + +An action body and a registered handler both run **elevated**: `ctx.api` and +`ctx.engine` carry `isSystem`, so they bypass row- and field-level security by +design (that is what lets an action do work the caller cannot do directly). The +consequence is that any caller-specific rule has to be re-established **inside** +your handler — and the predicate most people reach for first does not work: + +```js +if (!ctx.record?.id) return refuse(); // ❌ always false — never refuses anything +``` + +Before dispatch, the platform loads the subject row in **your caller's own +scope**. If that read comes back empty — the row is invisible to them under +row-level security, or the id names nothing — the dispatcher still puts the +`recordId` from the request onto `ctx.record.id`, because a **new-record / +record-less** action legitimately needs it there. So `ctx.record.id` is present +either way, and the guard above passes for a caller who cannot see the row. + +The signal that *does* distinguish them is `ctx.recordLoadDenied`: + +```js +// ✅ the caller-scope load did not deliver the row — do not act on it +if (ctx.recordLoadDenied) { + throw Object.assign(new Error('Record not available'), { code: 'RECORD_NOT_FOUND' }); +} +``` + +| | `ctx.record.id` | `ctx.recordLoadDenied` | +|:---|:---|:---| +| Caller **can** read the row | the id | absent | +| Caller **cannot** read the row | the id (stamped) | `true` | +| New-record / record-less action | the id, if one was passed | absent | + + +Read it as `ctx.recordLoadDenied === true`. The key is **absent**, never +`false`, when nothing was refused — so an action that never loads a row (no +`recordId`, or an object-less action) never trips the guard. + +It reports **"the row did not resolve for this caller"**, not "the platform saw +an authorization error". A row hidden by row-level security and an id that names +nothing both arrive as `RECORD_NOT_FOUND`, deliberately — the platform does not +disclose whether a record you cannot see exists — and the flag does not pretend +to separate what that read fuses. For an authorization decision they are the +same answer: this caller has not demonstrated read access to that row. + +Both invocation doors set it — `POST /api/v1/actions/...` and the MCP +`run_action` tool — and it reaches inline `body` sandboxes and registered +handlers alike. + + +Set `ctx.recordLoadDenied` aside only when your action is *meant* to run without +a readable subject row (an "import this id from elsewhere" action, say). The +default for a row-scoped action on a `private` object is to refuse. + ## Call it over REST Every action is also an endpoint — the Console button and the API call run diff --git a/packages/runtime/src/action-execution.ts b/packages/runtime/src/action-execution.ts index 20ce1f47b5..5747fcc82a 100644 --- a/packages/runtime/src/action-execution.ts +++ b/packages/runtime/src/action-execution.ts @@ -1177,6 +1177,97 @@ export function buildActionEngineFacade(_deps: ActionExecutionDeps, ql: any, ec? }; } +/** + * The subject-record load's outcome, as the two action doors hand it to a + * handler (#14143). + */ +export interface ActionSubjectRecordLoad { + /** What the handler receives as `ctx.record`. Unchanged by #14143. */ + record: Record; + /** + * `true` exactly when a caller-scope load was ATTEMPTED and did not deliver + * the row. Absent-or-`false` otherwise — including for the record-less and + * new-record actions that never attempt one. + */ + recordLoadDenied: boolean; +} + +/** + * Load an action's subject record IN THE CALLER'S OWN SCOPE, and report whether + * that load actually delivered the row (#14143). ONE producer for both action + * doors — the MCP `run_action` bridge below and the REST `/actions` route + * (`domains/actions.ts`) — because the signal it emits is documented to app + * authors, and a signal only one of two doors sets is an authorization guard + * that is silently inert on the other. + * + * ## Why the signal exists + * + * The load runs under the CALLER's `ExecutionContext` deliberately: an action's + * subject row must be readable by the person invoking the action. But the body + * that follows runs ELEVATED (`buildActionExecutionContext` = `isSystem: true`, + * settled design — #3914), so authorization has to be re-established INSIDE the + * handler, and the platform's most natural predicate for that was broken: + * + * - a refused/absent load leaves `record` as `{}`, so `record.id == null`; + * - the `recordId` stamp below fires on exactly that condition. + * + * The stamp condition and the load-failure condition COINCIDED, so + * `if (!ctx.record?.id) refuse()` — the guard an author reaches for first — + * was true on a row the caller cannot read, every time. The stamp is NOT the + * defect and is kept verbatim: new-record / record-less actions legitimately + * depend on `recordId` being in place, and removing it would break them. + * What was missing is a second, independent channel saying "this id did not + * resolve in your caller's scope", which is what `recordLoadDenied` is. + * + * ## What the flag can and cannot tell you + * + * It reports "the caller-scope load did not deliver the row", NOT "the platform + * caught an authorization error". The read path collapses the two on purpose: + * a row filtered out by RLS and an id that names nothing both arrive as + * `RECORD_NOT_FOUND` / 404 (`recordNotFoundError`, `@objectstack/core`), which + * is existence non-disclosure working as designed — the same reason the doc + * comment on the call site says "an unseen record reads as not-found". Nothing + * in the caught error separates them, so this flag deliberately does not + * pretend to, and carries no code/status: for an authorization decision the two + * are ONE answer — this caller has not demonstrated read access to that row. + */ +export async function loadActionSubjectRecord( + objectName: string, + recordId: string | undefined, + getRecord: () => Promise, +): Promise { + let record: Record = {}; + let recordLoadDenied = false; + if (recordId && !isObjectLessActionKey(objectName)) { + try { + const got: any = await getRecord(); + if (got?.record) record = got.record; + // A resolved call that carried no row is the same fact as a thrown + // one — the protocol's own 404 arrives as a throw, but a data + // service that answers `{ record: undefined }` must not read as a + // successful load just because it declined to throw. + else recordLoadDenied = true; + } catch { + /* new-record / record-less actions pass an empty record */ + recordLoadDenied = true; + } + } + // ⛔ Do NOT delete: a new-record / record-less action's handler reads its + // id from here. `recordLoadDenied` is what tells the two cases apart now. + if (record && (record as any).id == null && recordId) (record as any).id = recordId; + return { record, recordLoadDenied }; +} + +/** + * The `ctx` keys that carry {@link loadActionSubjectRecord}'s verdict into an + * action context — spread so the flag is ABSENT rather than `false` when no + * load was refused, matching the `referentialFieldClear` marker convention on + * the sandbox seam: a body reads `ctx.recordLoadDenied === true`. + */ +export function actionRecordLoadSignal(load: ActionSubjectRecordLoad): { recordLoadDenied?: true } { + return load.recordLoadDenied ? { recordLoadDenied: true } : {}; +} + /** * Resolve + invoke a business action by its declarative name for the MCP * `run_action` tool. Enforces the AI-exposure gate (`ai.exposed`, #2849), the @@ -1276,16 +1367,11 @@ export async function invokeBusinessAction(deps: ActionExecutionDeps, // Load the subject record under RLS when row-context (engages the same // permission path as get_record — an unseen record reads as not-found). - let record: Record = {}; - if (recordId && !isObjectLessActionKey(objectName)) { - try { - const got: any = await callData('get', { object: objectName, id: recordId }, driver, envId, ec); - if (got?.record) record = got.record; - } catch { - /* new-record / record-less actions pass an empty record */ - } - } - if (record && (record as any).id == null && recordId) (record as any).id = recordId; + // [#14143] Through the ONE shared producer, so this door and the REST + // `/actions` door emit the same `recordLoadDenied` signal to handlers. + const subject = await loadActionSubjectRecord(objectName, recordId, () => + callData('get', { object: objectName, id: recordId }, driver, envId, ec)); + const record = subject.record; // [#5372] One shared producer for the user shape (`security/actor-user.ts`), // the same one the REST `/actions` route and the AI routes use. What stood @@ -1330,6 +1416,12 @@ export async function invokeBusinessAction(deps: ActionExecutionDeps, ); const actionContext: any = { record, + // [#14143] The caller-scope load's verdict, on the same context the + // record rides. `ctx.record.id` is present either way (the stamp is + // load-bearing for record-less actions), so this is the ONLY thing that + // tells a handler its subject row did not resolve for THIS caller — + // and the body face carries it too (`sandbox/body-runner.ts`). + ...actionRecordLoadSignal(subject), user, session: buildActionSession(deps, ec), engine: buildActionEngineFacade(deps, ql, ec), diff --git a/packages/runtime/src/action-record-load-denied.test.ts b/packages/runtime/src/action-record-load-denied.test.ts new file mode 100644 index 0000000000..98714a023c --- /dev/null +++ b/packages/runtime/src/action-record-load-denied.test.ts @@ -0,0 +1,313 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#14143] A handler must be able to tell "the caller cannot read this row" + * from "this action legitimately has no record". + * + * ## The defect + * + * Both action doors load the subject row in the CALLER's own scope, swallow the + * failure, and then stamp `record.id = recordId` under the condition + * `record && record.id == null && recordId`. A failed load leaves `record` as + * `{}` — so `record.id` is exactly `null`, and the stamp condition and the + * load-failure condition COINCIDE. The body that follows runs ELEVATED + * (`isSystem: true`, settled design — #3914), so authorization has to be + * re-established inside the handler, and the predicate an author reaches for + * first — + * + * if (!ctx.record?.id) return refuse(); + * + * — was therefore ALWAYS false, including on a row the caller cannot read. + * + * ⚠️ The stamp is NOT the defect and is deliberately kept: a new-record / + * record-less action legitimately depends on `recordId` being in place. That is + * the regression these tests pin alongside the fix — every "denied" case below + * asserts `ctx.record.id` is STILL there. + * + * ## What is pinned + * + * 1. **The predicate is real, on BOTH doors.** REST `/actions` and the MCP + * `run_action` bridge each emit `recordLoadDenied: true` when the + * caller-scope load did not deliver the row. A signal only one door sets + * would be an authorization guard silently inert on the other — the same + * defect, one door over. + * 2. **The stamp survives.** `ctx.record.id` is present in every denied case, + * and an object-less action invoked with a `recordId` still gets it. + * 3. **The flag is ABSENT, not `false`, when nothing was refused** — the + * `referentialFieldClear` marker convention on this seam. Every such + * absence assertion has a FIRING POSITIVE CONTROL in the same file, on the + * same rig: the identical expectation shape reports `true` for the + * unauthorized caller, so an absence here cannot be a rig that never + * populates the key. + * 4. **The body face carries it.** An inline `body` is the surface an AI + * author writes most, and its sandbox `ctx` is a FIXED key set — a key the + * dispatcher sets but the sandbox never marshals would read as `undefined` + * inside every body, re-manufacturing the always-false guard. Pinned by + * running a real QuickJS body. + * + * ## The RLS double is faithful on the one point that matters + * + * `find` here honours `options.context.userId`: the row exists and is returned + * to its owner, and is INVISIBLE to anyone else — which is exactly how row-level + * security manifests to `callData('get', …)`, and why the real + * `recordNotFoundError` (404 `RECORD_NOT_FOUND`) is what the dispatcher then + * catches. The tests below use the REAL `callData`, so nothing about the + * refused/absent collapse is mocked away: an unseen row and a nonexistent id + * reach the catch as the same error, which is precisely why the fix is a + * separate channel rather than an inspection of the caught error. + */ + +import { describe, it, expect, vi } from 'vitest'; + +import { HttpDispatcher } from './http-dispatcher.js'; +import { + callData, + invokeBusinessAction, + loadActionSubjectRecord, + actionRecordLoadSignal, + GLOBAL_ACTION_OBJECT_KEY, +} from './action-execution.js'; +import { actionBodyRunnerFactory } from './sandbox/body-runner.js'; +import { QuickJSScriptRunner } from './sandbox/quickjs-runner.js'; + +const OWNER = 'usr_owner'; +const STRANGER = 'usr_stranger'; +const RECORD_ID = 'case_1'; + +const ACTION = { + name: 'close_case', + label: 'Close', + objectName: 'crm_case', + type: 'script', + target: 'close_case', + ai: { exposed: true, description: 'Close a case.' }, +}; +const OBJECT_DEF = { name: 'crm_case', actions: [ACTION] }; + +/** The acting principal, as `resolveExecutionContext` builds one. */ +function ec(userId: string) { + return { userId, tenantId: 'org_1', positions: [], permissions: [], systemPermissions: [] }; +} + +/** + * An engine whose reads are ROW-SCOPED: `crm_case:case_1` is visible to its + * owner and to nobody else. This is the whole point of the double — a stub that + * returned the row to everyone would pass while the defect was live. + */ +function makeQl() { + const executeAction = vi.fn(async (_object: string, _action: string, _ctx: any) => ({ ok: true })); + const schemaOf = (n: string) => (n === OBJECT_DEF.name ? OBJECT_DEF : undefined); + const ql: any = { + executeAction, + getSchema: schemaOf, + registry: { getObject: schemaOf, getItem: () => undefined }, + find: vi.fn(async (object: string, options?: any) => { + if (object !== OBJECT_DEF.name) return []; + const caller = options?.context?.userId; + return caller === OWNER + ? [{ id: RECORD_ID, status: 'open', owner_id: OWNER }] + : []; + }), + insert: vi.fn(), update: vi.fn(), delete: vi.fn(), + }; + return ql; +} + +/** REST — `POST /actions/crm_case/close_case/case_1`. Returns the handler ctx. */ +async function dispatchRest(userId: string, ql: any, path = `/crm_case/close_case/${RECORD_ID}`) { + const metadata: any = { + load: vi.fn(async () => null), + loadDiagnosed: vi.fn(async () => ({ data: null, degraded: false, errors: [] })), + listObjects: vi.fn(async () => [OBJECT_DEF]), + getObject: vi.fn(async (n: string) => (n === OBJECT_DEF.name ? OBJECT_DEF : undefined)), + }; + const kernel: any = { + context: { + getService: (n: string) => + n === 'objectql' || n === 'data' ? ql : n === 'metadata' ? metadata : null, + }, + }; + const context: any = { request: {}, environmentId: 'platform', executionContext: ec(userId) }; + const res: any = await (new HttpDispatcher(kernel) as any).handleActions(path, 'POST', {}, context); + return { response: res.response, actionCtx: ql.executeAction.mock.calls[0]?.[2] }; +} + +/** + * MCP — `run_action`. Wired to the REAL `callData`, so the row-scoped read and + * its 404 are the ones production runs, not a hand-thrown stand-in. + */ +async function dispatchMcp(userId: string, ql: any, input: Record = { recordId: RECORD_ID }) { + const deps: any = { resolveService: async () => undefined, getObjectQL: async () => ql }; + const requestContext: any = { request: {}, environmentId: 'platform' }; + const caller = ec(userId); + await invokeBusinessAction(deps, requestContext, ACTION.name, input as any, { + driver: undefined, + envId: 'platform', + ec: caller, + getMeta: () => ({ listObjects: async () => [OBJECT_DEF] }), + callData: (action, params, dataDriver, scopeId, execCtx) => + callData(deps, requestContext, action, params, dataDriver, scopeId, execCtx), + }); + return { actionCtx: ql.executeAction.mock.calls[0]?.[2] }; +} + +describe('#14143 — REST /actions tells a handler its caller-scope load was refused', () => { + it('a caller who CANNOT read the row reaches the handler with recordLoadDenied === true', async () => { + const ql = makeQl(); + const { actionCtx } = await dispatchRest(STRANGER, ql); + + expect(actionCtx).toBeDefined(); + expect(actionCtx.recordLoadDenied).toBe(true); + + // ⛔ The stamp is NOT removed — a record-less action depends on it, and + // this is the coincidence that made the natural guard useless: the id + // is here whether or not the caller can see the row, which is why the + // flag above (and not `record.id`) is the authorization predicate. + expect(actionCtx.record.id).toBe(RECORD_ID); + expect(Boolean(actionCtx.record?.id)).toBe(true); + // …and nothing of the row itself leaked to a caller who cannot read it. + expect(actionCtx.record.status).toBeUndefined(); + expect(actionCtx.record.owner_id).toBeUndefined(); + }); + + it('the row OWNER reaches the handler with the real row and no flag at all', async () => { + const ql = makeQl(); + const { actionCtx } = await dispatchRest(OWNER, ql); + + expect(actionCtx.record).toMatchObject({ id: RECORD_ID, status: 'open', owner_id: OWNER }); + // ABSENT, not `false` — read as `ctx.recordLoadDenied === true`. The + // firing control for this zero is the case above: same rig, same + // expectation shape, and it reports `true`. + expect('recordLoadDenied' in actionCtx).toBe(false); + expect(actionCtx.recordLoadDenied).toBeUndefined(); + }); + + it('a new-record action (no recordId) is untouched — no load, no flag', async () => { + const ql = makeQl(); + const { actionCtx } = await dispatchRest(STRANGER, ql, '/crm_case/close_case'); + + expect(actionCtx.record).toEqual({}); + expect('recordLoadDenied' in actionCtx).toBe(false); + // No caller-scope read was even attempted for the subject row. + expect(ql.find.mock.calls.filter((c: any[]) => c[0] === OBJECT_DEF.name)).toHaveLength(0); + }); +}); + +describe('#14143 — MCP run_action emits the SAME signal as the REST door', () => { + it('a caller who CANNOT read the row reaches the handler with recordLoadDenied === true', async () => { + const ql = makeQl(); + const { actionCtx } = await dispatchMcp(STRANGER, ql); + + expect(actionCtx.recordLoadDenied).toBe(true); + expect(actionCtx.record.id).toBe(RECORD_ID); // stamp preserved + expect(actionCtx.record.status).toBeUndefined(); + }); + + it('the row OWNER reaches the handler with the real row and no flag at all', async () => { + const ql = makeQl(); + const { actionCtx } = await dispatchMcp(OWNER, ql); + + expect(actionCtx.record).toMatchObject({ id: RECORD_ID, status: 'open' }); + expect('recordLoadDenied' in actionCtx).toBe(false); + }); + + it('a record-less invocation (no recordId) is untouched — no load, no flag', async () => { + const ql = makeQl(); + const { actionCtx } = await dispatchMcp(STRANGER, ql, {}); + + expect(actionCtx.record).toEqual({}); + expect('recordLoadDenied' in actionCtx).toBe(false); + }); +}); + +describe('#14143 — loadActionSubjectRecord, the ONE producer both doors call', () => { + it('object-less action with a recordId: no load is attempted, and the stamp STILL lands', async () => { + const getRecord = vi.fn(async () => ({ record: { id: 'other' } })); + const out = await loadActionSubjectRecord(GLOBAL_ACTION_OBJECT_KEY, RECORD_ID, getRecord); + + expect(getRecord).not.toHaveBeenCalled(); + // ⛔ The prohibition this test exists for: a record-less action still + // gets its `recordId`. + expect(out.record).toEqual({ id: RECORD_ID }); + expect(out.recordLoadDenied).toBe(false); + expect(actionRecordLoadSignal(out)).toEqual({}); + }); + + it('a thrown load is denied, and the stamp still lands', async () => { + const out = await loadActionSubjectRecord('crm_case', RECORD_ID, async () => { + throw Object.assign(new Error('Record case_1 not found in crm_case'), { + code: 'RECORD_NOT_FOUND', status: 404, + }); + }); + + expect(out.recordLoadDenied).toBe(true); + expect(out.record).toEqual({ id: RECORD_ID }); + expect(actionRecordLoadSignal(out)).toEqual({ recordLoadDenied: true }); + }); + + it('a RESOLVED load carrying no row is denied too — declining to throw is not a successful load', async () => { + const out = await loadActionSubjectRecord('crm_case', RECORD_ID, async () => ({ record: undefined })); + + expect(out.recordLoadDenied).toBe(true); + expect(out.record).toEqual({ id: RECORD_ID }); + }); + + it('a delivered row is not denied and is passed through untouched', async () => { + const row = { id: RECORD_ID, status: 'open' }; + const out = await loadActionSubjectRecord('crm_case', RECORD_ID, async () => ({ record: row })); + + expect(out.recordLoadDenied).toBe(false); + expect(out.record).toEqual(row); + expect(actionRecordLoadSignal(out)).toEqual({}); + }); + + it('no recordId at all: no load, no flag, no stamp', async () => { + const getRecord = vi.fn(async () => ({ record: { id: 'x' } })); + const out = await loadActionSubjectRecord('crm_case', undefined, getRecord); + + expect(getRecord).not.toHaveBeenCalled(); + expect(out.record).toEqual({}); + expect(out.recordLoadDenied).toBe(false); + }); +}); + +describe('#14143 — the signal crosses into a sandboxed action body', () => { + const runner = new QuickJSScriptRunner(); + const SOURCE = + 'return { denied: ctx.recordLoadDenied === true, ' + + 'guard: !(ctx.record && ctx.record.id), id: ctx.record && ctx.record.id };'; + + function bodyFn() { + const factory = actionBodyRunnerFactory(runner, { ql: makeQl(), appId: 'crm' }); + return factory({ + name: ACTION.name, + object: OBJECT_DEF.name, + type: 'script', + body: { language: 'js', source: SOURCE, capabilities: [] }, + } as any); + } + + it('a body sees recordLoadDenied === true — while the OLD guard is still false', async () => { + const out: any = await bodyFn()!({ + record: { id: RECORD_ID }, + recordLoadDenied: true, + params: {}, + }); + + expect(out.denied).toBe(true); + // The pre-fix predicate, measured inside the VM: still false, because + // the stamp is still there. That is why a body needs the new key. + expect(out.guard).toBe(false); + expect(out.id).toBe(RECORD_ID); + }); + + it('a body sees NOTHING when the load was fine — firing control for the zero above', async () => { + const out: any = await bodyFn()!({ + record: { id: RECORD_ID, status: 'open' }, + params: {}, + }); + + expect(out.denied).toBe(false); + expect(out.id).toBe(RECORD_ID); + }); +}); diff --git a/packages/runtime/src/domains/actions.ts b/packages/runtime/src/domains/actions.ts index 9c61cc218d..8848829bca 100644 --- a/packages/runtime/src/domains/actions.ts +++ b/packages/runtime/src/domains/actions.ts @@ -615,14 +615,14 @@ export async function handleActionsRequest(deps: DomainHandlerDeps, path: string } // Load the record (best-effort) so handlers can rely on `ctx.record`. - let record: Record = {}; - if (recordId && !actionExec.isObjectLessActionKey(objectName)) { - try { - const got = await actionExec.callData(deps, _context, 'get', { object: objectName, id: recordId }, _context.dataDriver, _context.environmentId, _context.executionContext); - if (got?.record) record = got.record; - } catch { /* record may not exist for new-record actions; pass empty */ } - } - if (record && (record as any).id == null && recordId) (record as any).id = recordId; + // [#14143] Through the ONE shared producer `loadActionSubjectRecord`, which + // also reports whether the CALLER's own scope actually delivered the row. + // This door and the MCP `run_action` door must emit the same signal: a + // documented guard (`if (ctx.recordLoadDenied) …`) that only one of two + // doors sets is inert on the other, which is the defect one door over. + const subject = await actionExec.loadActionSubjectRecord(objectName, recordId, () => + actionExec.callData(deps, _context, 'get', { object: objectName, id: recordId }, _context.dataDriver, _context.environmentId, _context.executionContext)); + const record = subject.record; // Resolve the caller identity from the request's ExecutionContext — the // single source `dispatch()` populates via `resolveExecutionContext`, @@ -650,6 +650,11 @@ export async function handleActionsRequest(deps: DomainHandlerDeps, path: string const actionContext: any = { record, + // [#14143] The caller-scope load's verdict — see + // `loadActionSubjectRecord`. `ctx.record.id` is stamped either way, so + // this is the only channel that distinguishes "the caller cannot read + // this row" from "this action legitimately has no record". + ...actionExec.actionRecordLoadSignal(subject), user: userFromAuth, session: actionExec.buildActionSession(deps, ec), // Slim engine facade matching the ActionContext.engine shape used by diff --git a/packages/runtime/src/sandbox/body-runner.ts b/packages/runtime/src/sandbox/body-runner.ts index 7d56192a94..dc0edd2508 100644 --- a/packages/runtime/src/sandbox/body-runner.ts +++ b/packages/runtime/src/sandbox/body-runner.ts @@ -771,6 +771,14 @@ function buildActionSandboxContext( // downstream writes it back. `warnDiscardedRecordWrites` reports the writes // a body makes to it rather than letting them vanish. record: unwrapProxyToPlain(actionCtx?.record), + // [#14143] The caller-scope load's verdict, marshalled EXPLICITLY for the + // same reason `dispatch` / `referentialFieldClear` are on the hook face: a + // body cannot reach the dispatcher's locals, and `ctx.record.id` is stamped + // even when the caller cannot read the row, so without this key an action + // body has no way at all to tell the two apart. Both assembly sites + // (`../action-execution.ts`, `../domains/actions.ts`) write it, and only + // in its declared shape — absent, never `false`, when nothing was refused. + ...(actionCtx?.recordLoadDenied === true ? { recordLoadDenied: true } : {}), api: buildSandboxApi(actionCtx, ql, 'action body'), // [#7448] Same removal as the hook face: neither action-context assembly // site (`../domains/actions.ts`, `../action-execution.ts`) writes `logger`. diff --git a/packages/runtime/src/sandbox/quickjs-runner.ts b/packages/runtime/src/sandbox/quickjs-runner.ts index c0b0e157d2..6756fb725e 100644 --- a/packages/runtime/src/sandbox/quickjs-runner.ts +++ b/packages/runtime/src/sandbox/quickjs-runner.ts @@ -528,6 +528,14 @@ export class QuickJSScriptRunner implements ScriptRunner { if (ctx.referentialFieldClear === true) { vm.setProp(ctxObj, 'referentialFieldClear', vm.true); } + // [#14143] The action face's caller-scope load verdict — same true-only + // installation, same reason: a body reads `ctx.recordLoadDenied === true` + // and an absent key means "nothing was refused". A plain boolean, so no + // freeze/graft ceremony is needed (the write-back channel reads only + // `ctx.input`, so a VM-side reassignment travels nowhere). + if (ctx.recordLoadDenied === true) { + vm.setProp(ctxObj, 'recordLoadDenied', vm.true); + } const apiObj = vm.newObject(); const objectFn = vm.newFunction('object', (nameH) => { diff --git a/packages/runtime/src/sandbox/script-runner.ts b/packages/runtime/src/sandbox/script-runner.ts index 0608cd28cf..e0fc0086b5 100644 --- a/packages/runtime/src/sandbox/script-runner.ts +++ b/packages/runtime/src/sandbox/script-runner.ts @@ -302,6 +302,29 @@ export interface ScriptContext { * prejudges neither answer. */ record?: unknown; + /** + * Action only: `true` exactly when the dispatcher ATTEMPTED to load the + * subject row in the CALLER's own scope and that load did not deliver it + * (#14143). Absent otherwise — including on every record-less / new-record + * action, which never attempts a load — so read it as + * `ctx.recordLoadDenied === true`, the same absence semantics as + * {@link referentialFieldClear}. + * + * This is the authorization predicate {@link record} cannot be. An action + * body runs ELEVATED (`isSystem`, #3914) and therefore has to re-establish + * authorization itself, but `ctx.record.id` is present whether or not the + * caller can read the row: a refused load leaves `record.id == null`, which + * is the exact condition on which the dispatcher stamps `recordId` back on + * (a stamp record-less actions depend on). So `if (!ctx.record?.id) …` was + * always false and never refused anything. + * + * ⚠️ It says "the row did not resolve for this caller", NOT "the platform + * saw an authorization error": an RLS-invisible row and a nonexistent id both + * surface as `RECORD_NOT_FOUND`, deliberately (existence non-disclosure), and + * the flag does not pretend to separate what the read path fused. For an + * authorization decision they are one answer. + */ + recordLoadDenied?: boolean; /** Engine-side `result` (only set for after* hooks). */ result?: unknown; api?: unknown;