diff --git a/.changeset/25653-app-hook-inherited-elevation-identity-write-refusal.md b/.changeset/25653-app-hook-inherited-elevation-identity-write-refusal.md new file mode 100644 index 00000000000..3264dbf2e47 --- /dev/null +++ b/.changeset/25653-app-hook-inherited-elevation-identity-write-refusal.md @@ -0,0 +1,23 @@ +--- +'@objectstack/objectql': patch +'@objectstack/plugin-security': patch +--- + +fix(security): an app-authored hook that inherits a system context, its `ctx.api.sudo()`, and an action it reaches through `execute()` are refused writes to position holdings, permission-set grants, invitations and memberships + +Clause-②: no (narrowing: an app-authored hook's inherited or sudo() elevation, and an action reached through ObjectRepository.execute(), are refused the four identity objects at run time, as a runAs: 'system' hook already is) +Metadata-protocol: none + + + +**BREAKING** (a served-set narrowing) for an app-authored hook that writes `sys_user_position`, `sys_user_permission_set`, `sys_invitation` or `sys_member` while elevated. A census of the hooks in this repository and in HotCRM found none that does. + +Since the previous release, a hook that declares `runAs: 'system'` and an action body are refused these writes. Three other ways app-authored code ran elevated were not: + +- a hook that declares no `runAs` and fires on a write made under a system context, which it inherits; +- a hook's in-process `ctx.api.sudo()`; +- an action reached through `ctx.api.object(name).execute(action)`. + +Each is now refused the same way: `403 PERMISSION_DENIED`, nothing written, and under the default `onError: 'abort'` the triggering write aborts. The message names the hook and the fix: set its `runAs` to `'user'` and leave `ctx.api.sudo()` out, or use the organization endpoints for invitations and memberships. + +A hook is app-authored when it is bound from metadata through the hook binder (the same hooks the `skipAutomations` opt-out skips). A hook a plugin registers in code with `engine.registerHook`, and platform code's own `sudo()`, keep writing these objects. Reads of the four objects, and writes to every other object, are unchanged. diff --git a/content/docs/permissions/system-context.mdx b/content/docs/permissions/system-context.mdx index 8d405627af9..8922abc32af 100644 --- a/content/docs/permissions/system-context.mdx +++ b/content/docs/permissions/system-context.mdx @@ -97,7 +97,7 @@ that silently does not happen. | # | Behaviour when `isSystem` | Package | What you get / what you lose | Anchor | |:--|:---|:---|:---|:---| | 1 | **The whole security middleware short-circuits** before any gate runs | plugin-security | Get: every CRUD/FLS/tenant/owner gate below skipped in one branch. Lose: every gate under it in this lane at once, down to and including the write-bypass row that widens the effective write scope to `org` (row 7) — this is the single largest behaviour on the page | `packages/plugins/plugin-security/src/security-plugin-authz-middleware.ts#registerAuthzMiddleware` | -| 1b | **App-authored automation's system context is REFUSED governed writes to the identity-assignment objects** — a flow run's, a `runAs: 'system'` hook's and an action body's; the one check ahead of the middleware's short-circuit | plugin-security | Get: nothing — an insert, update, delete, transfer, restore or purge of `sys_user_position`, `sys_user_permission_set`, `sys_invitation` or `sys_member` from a context carrying `isSystem` and either a `flowRunId` (a `runAs: 'system'` flow's data nodes) or the app-automation elevation mark (`@objectstack/core`; set only by a hook's `runAs: 'system'` derivation and by the action body's execution-context builder), at any depth of their writes, is refused `PERMISSION_DENIED` before the short-circuit skips the ADR-0090 D12 gate and the identity write guard. Lose: nothing a platform writer held — a system context with neither (the seeders, better-auth and its reconciliation, invitation placement, any `sudo()`, which never sets the mark) passes on to the short-circuit unchanged, reads are not judged, and a `runAs: 'user'` flow or hook carries no `isSystem`, so the D12 gate judges it as the user it acts for | `packages/plugins/plugin-security/src/flow-system-identity-write-refusal.ts#elevationSource` | +| 1b | **App-authored automation's system context is REFUSED governed writes to the identity-assignment objects** — a flow run's, an app-authored hook's (`runAs: 'system'`, a system context it inherits, its `sudo()`) and an action body's; the one check ahead of the middleware's short-circuit | plugin-security | Get: nothing — an insert, update, delete, transfer, restore or purge of `sys_user_position`, `sys_user_permission_set`, `sys_invitation` or `sys_member` from a context carrying `isSystem` and either a `flowRunId` (a `runAs: 'system'` flow's data nodes) or the app-automation elevation mark (`@objectstack/core`; set only by a hook's `runAs: 'system'` derivation, by the action body's execution-context builder and `ObjectRepository.execute`, and on the api of a hook bound through the hook binder, whose inherited system context and `sudo()` it marks), at any depth of their writes, is refused `PERMISSION_DENIED` before the short-circuit skips the ADR-0090 D12 gate and the identity write guard. Lose: nothing a platform writer held — a system context with neither (the seeders, better-auth and its reconciliation, invitation placement, platform code's `sudo()`, which never sets the mark, and a hook registered in code) passes on to the short-circuit unchanged, reads are not judged, and a `runAs: 'user'` flow or hook carries no `isSystem`, so the D12 gate judges it as the user it acts for | `packages/plugins/plugin-security/src/flow-system-identity-write-refusal.ts#elevationSource`, `packages/objectql/src/engine.ts#inheritsElevation` | | 2 | **`owner_id` is not auto-stamped on INSERT** (the step 3.5 anchor guard is inside the block row 1 skips) | plugin-security | Lose: the row lands `owner_id = NULL`, so the default `owner_only_writes` policy hides it **from its own creator**. Get: nothing — this is a gap, not a capability | the step 3.5 guard block and the short-circuit that skips it are both inside `packages/plugins/plugin-security/src/security-plugin-authz-middleware.ts#registerAuthzMiddleware` | | 3 | Row-level read filter resolves to "no filter" | plugin-security | Get: unscoped reads. Lose: row-level scoping entirely | `packages/plugins/plugin-security/src/security-plugin-read-filter.ts#getReadFilter` | | 4 | Field-level security returns **all** fields | plugin-security | Get: every column readable (`getReadableFields`) and writable (`getWritableFields`). Lose: field masking | `packages/plugins/plugin-security/src/security-plugin-field-readers.ts#resolveProjectionFieldMask` | diff --git a/packages/objectql/src/engine.ts b/packages/objectql/src/engine.ts index de1a322d51a..d25eef0449d 100644 --- a/packages/objectql/src/engine.ts +++ b/packages/objectql/src/engine.ts @@ -153,9 +153,12 @@ import { resolveUserAuthzGrants, // [#25653] The app-automation elevation mark: set by a hook's // `runAs: 'system'` derivation, carried across `createContext`'s parse — - // see `ScopedContext.withRunAs`. + // see `ScopedContext.withRunAs` — and by an app-authored hook's inherited + // elevation and `sudo()`, and `ObjectRepository.execute` — see + // `ScopedContext[APP_AUTHORED_HOOK_API]`. markAppAutomationElevation, carryAppAutomationElevation, + appAutomationElevationOf, } from '@objectstack/core'; import { WriteEpoch, isWriteEpochOperation } from './write-epoch.js'; import { bridgeAuthzInvalidation } from './authz-invalidation-bridge.js'; @@ -312,7 +315,7 @@ import { divergingHookPayloadKeys, MultiUpdateHookKeyDivergenceError, } from './multi-update-hook-key-divergence.js'; -import { UnscopedHookApi, SEED_USER_GRANTS, type HookRunAs, type HookRunAsRef, type RunAsDerivableApi } from './hook-run-as.js'; +import { UnscopedHookApi, SEED_USER_GRANTS, APP_AUTHORED_HOOK_API, type HookRunAs, type HookRunAsRef, type RunAsDerivableApi } from './hook-run-as.js'; import type { HookWriteRecording } from './hook-write-provenance.js'; import { resolveMasterDetailRelation } from './master-detail.js'; // [#19911] The settlement's question "does judging the FK's own lock need the @@ -22957,7 +22960,14 @@ export class ObjectRepository implements IScopedObjectRepository { */ async execute(actionName: string, params?: any): Promise { if (this.engine.executeAction) { - const executionContext: ExecutionContext = { ...this.context, isSystem: true }; + // [#25653] With the `'action'` mark, as `buildActionExecutionContext` + // (runtime) gives the REST and MCP doors' action bodies: the action this + // reaches is app-authored automation running elevated, so its writes to + // the four identity-assignment objects are refused (plugin-security). + const executionContext: ExecutionContext = markAppAutomationElevation( + { ...this.context, isSystem: true }, + 'action', + ); return this.engine.executeAction(this.objectName, actionName, { ...params, userId: this.context.userId, @@ -22990,19 +23000,62 @@ export class ScopedContext implements IScopedContext, RunAsDerivableApi { private engine: IDataEngine ) {} + /** + * [#25653] Set on the api an app-authored hook is handed + * ({@link APP_AUTHORED_HOOK_API}) and kept by every api derived from it + * ({@link derive}); read by {@link sudo}. + */ + private appAuthored = false; + + /** A child over `executionContext` that keeps this api's provenance. */ + private derive(executionContext: ExecutionContext): ScopedContext { + const child = new ScopedContext(executionContext, this.engine); + child.appAuthored = this.appAuthored; + return child; + } + /** Get a repository scoped to this context */ object(name: string): ObjectRepository { return new ObjectRepository(name, this.executionContext, this.engine as any); } - /** Create an elevated (system) context */ + /** + * Create an elevated (system) context. + * + * [#25653] Platform code's `sudo()` carries no app-automation mark, so its + * writes to the four identity-assignment objects land. An app-authored + * hook's in-process `ctx.api.sudo()` is the hook's elevation: it carries + * the `'hook'` mark (unless the context already carries one). + */ sudo(): ScopedContext { - return new ScopedContext( - { ...this.executionContext, isSystem: true }, - this.engine + const elevated = { ...this.executionContext, isSystem: true }; + return this.derive( + this.appAuthored && !appAutomationElevationOf(elevated) + ? markAppAutomationElevation(elevated, 'hook') + : elevated, ); } + /** + * [#25653] The api an app-authored hook runs with — see + * {@link APP_AUTHORED_HOOK_API} (`hook-run-as.ts`), where the line between + * app-authored and platform hooks is drawn. Derived by `wrapDeclarativeHook` + * for a hook the binder bound, before its `runAs` is applied. + * + * An inherited system context with no `flowRunId` and no mark gets the + * `'hook'` mark: the hook runs with that elevation, so its writes to the four + * identity-assignment objects are refused as a `runAs: 'system'` hook's are. + * A run's context (`flowRunId`) is already judged as the run's; a marked one + * keeps its mark; a user's context is not elevated and needs none. + */ + [APP_AUTHORED_HOOK_API](): ScopedContext { + const ctx = this.executionContext; + const inheritsElevation = ctx.isSystem && !ctx.flowRunId && !appAutomationElevationOf(ctx); + const api = new ScopedContext(inheritsElevation ? markAppAutomationElevation(ctx, 'hook') : ctx, this.engine); + api.appAuthored = true; + return api; + } + /** * [#14010] The api a hook that declared `runAs` is handed — derived from the * engine-built one (`buildHookApi(opCtx.context)`) at dispatch, by @@ -23031,16 +23084,12 @@ export class ScopedContext implements IScopedContext, RunAsDerivableApi { */ withRunAs(runAs: Exclude, ref: HookRunAsRef): IScopedContext { if (runAs === 'system') { - return new ScopedContext( + return this.derive( markAppAutomationElevation({ ...this.executionContext, isSystem: true }, 'hook'), - this.engine ); } if (!this.executionContext.userId) return new UnscopedHookApi(ref); - return new ScopedContext( - { ...this.executionContext, isSystem: false }, - this.engine - ); + return this.derive({ ...this.executionContext, isSystem: false }); } /** @@ -23108,15 +23157,12 @@ export class ScopedContext implements IScopedContext, RunAsDerivableApi { ctx.userId, ctx.tenantId ? { tenantId: ctx.tenantId } : {}, ); - return new ScopedContext( - { - ...ctx, - ...(needsGrants ? { positions: grants.positions, permissions: grants.permissions } : {}), - ...(needsPeers ? { org_user_ids: grants.org_user_ids } : {}), - ...(needsEmail && grants.email ? { email: grants.email } : {}), - }, - this.engine - ); + return this.derive({ + ...ctx, + ...(needsGrants ? { positions: grants.positions, permissions: grants.permissions } : {}), + ...(needsPeers ? { org_user_ids: grants.org_user_ids } : {}), + ...(needsEmail && grants.email ? { email: grants.email } : {}), + }); } /** @@ -23201,10 +23247,7 @@ export class ScopedContext implements IScopedContext, RunAsDerivableApi { // boundaries the store does not. It is identity-equal to the store's // handle, so `transactionCoversDriverFor` still attributes it to the // OUTER owner and the #5351 same-origin gate judges it unchanged. - const joinedCtx = new ScopedContext( - { ...this.executionContext, transaction: ambient.transaction }, - this.engine - ); + const joinedCtx = this.derive({ ...this.executionContext, transaction: ambient.transaction }); // JOINED, not owned: the outer caller decides commit vs rollback (#5696). return callback(joinedCtx, { owned: false }); } @@ -23230,10 +23273,7 @@ export class ScopedContext implements IScopedContext, RunAsDerivableApi { } const trx = await driver.beginTransaction(); - const trxCtx = new ScopedContext( - { ...this.executionContext, transaction: trx }, - this.engine - ); + const trxCtx = this.derive({ ...this.executionContext, transaction: trx }); // Publish this transaction into the engine's ambient store so internal // queries during writes reuse its connection (ADR-0034) — and so a nested // `transaction()` on either surface can JOIN it. The store entry also @@ -23384,20 +23424,14 @@ export class ScopedContext implements IScopedContext, RunAsDerivableApi { // store's handle — so `buildDriverOptions` binds every op on it to the // outer connection and `transactionCoversDriverFor` still attributes the // handle to the OUTER owner (#5351 unchanged). - const ctx = new ScopedContext( - { ...this.executionContext, transaction: ambient.transaction }, - this.engine - ); + const ctx = this.derive({ ...this.executionContext, transaction: ambient.transaction }); this.joinedHandles.add(ambient.transaction); return { ctx, handle: ambient.transaction, owned: false }; } const driver = this.txDriver(); if (!driver) return null; const trx = await driver.beginTransaction(); - const ctx = new ScopedContext( - { ...this.executionContext, transaction: trx }, - this.engine - ); + const ctx = this.derive({ ...this.executionContext, transaction: trx }); return { ctx, handle: trx, owned: true }; } diff --git a/packages/objectql/src/hook-binder.ts b/packages/objectql/src/hook-binder.ts index 39d0c639169..a275fdc8edc 100644 --- a/packages/objectql/src/hook-binder.ts +++ b/packages/objectql/src/hook-binder.ts @@ -312,7 +312,11 @@ export function bindHooksToEngine( continue; } - const wrapped = wrapDeclarativeHook(hook, resolved, { logger, metrics: opts.metrics }); + // [#25653] `appAuthored`: every hook this binder binds is app-authored — + // the same set the `skipAutomations` opt-out keys on through `meta` + // below — so its inherited elevation and its `sudo()` carry the + // app-automation mark (`APP_AUTHORED_HOOK_API`, `hook-run-as.ts`). + const wrapped = wrapDeclarativeHook(hook, resolved, { logger, metrics: opts.metrics, appAuthored: true }); const events = Array.isArray(hook.events) ? hook.events : []; for (const event of events) { diff --git a/packages/objectql/src/hook-run-as.test.ts b/packages/objectql/src/hook-run-as.test.ts index dc0557cb136..03fbd6d1ff4 100644 --- a/packages/objectql/src/hook-run-as.test.ts +++ b/packages/objectql/src/hook-run-as.test.ts @@ -34,6 +34,7 @@ */ import { describe, it, expect } from 'vitest'; +import { appAutomationElevationOf } from '@objectstack/core'; import { ObjectQL, ScopedContext } from './engine.js'; import { bindHooksToEngine } from './hook-binder.js'; import { wrapDeclarativeHook } from './hook-wrappers.js'; @@ -389,7 +390,9 @@ describe('#14010 Hook.runAs — the identity a hook\'s ctx.api presents', () => const entry = await import('./index.js'); expect('SEED_USER_GRANTS' in entry).toBe(false); expect(Object.getOwnPropertyNames(ScopedContext.prototype)).not.toContain('withUserGrantsSeeded'); - expect(Object.getOwnPropertySymbols(ScopedContext.prototype)).toHaveLength(1); + // [#25653] The app-authored hook api's derivation is the other symbol-keyed member, unexported too. + expect('APP_AUTHORED_HOOK_API' in entry).toBe(false); + expect(Object.getOwnPropertySymbols(ScopedContext.prototype)).toHaveLength(2); }); it('a resolver failure keeps the UNSEEDED pin — narrower, never elevated — and says so once at warn', async () => { @@ -587,3 +590,59 @@ describe('#14010 Hook.runAs — through a real ObjectQL dispatch', () => { expect(contexts.runas_rating[0]?.isSystem).toBe(true); }); }); + +// --------------------------------------------------------------------------- +// [#25653] An app-authored hook (bound through the binder) carries its +// provenance: an inherited system context and its `sudo()` get the +// app-automation mark. A platform wrap (no `appAuthored`) is unchanged. +// --------------------------------------------------------------------------- + +describe('#25653 an app-authored hook\'s elevation carries the app-automation mark', () => { + const SYSTEM_TRIGGER = { isSystem: true, tenantId: 'org_1' } as ExecutionContext; + + /** The context of the hook's insert, made through `pick(api)`. */ + async function hookInsert(execCtx: ExecutionContext, opts: { appAuthored?: boolean; runAs?: unknown }, pick: (api: any) => any = (a) => a) { + const engine = recordingEngine(); + const wrapped = wrapDeclarativeHook(hookOf(opts.runAs), async (c) => { + await pick(c.api).object('sys_member').insert({ id: 'm1' }); + }, { logger: silentLogger, appAuthored: opts.appAuthored }); + await wrapped(makeCtx(execCtx, engine)); + return engine.seen.find((e: { method: string }) => e.method === 'insert')!.context; + } + + it('an inherited system context is marked; a run\'s context and a user\'s are not', async () => { + expect(appAutomationElevationOf(await hookInsert(SYSTEM_TRIGGER, { appAuthored: true }))).toBe('hook'); + expect(appAutomationElevationOf(await hookInsert({ ...SYSTEM_TRIGGER, flowRunId: 'run_1' } as ExecutionContext, { appAuthored: true }))).toBeUndefined(); + expect(appAutomationElevationOf(await hookInsert(OPERATOR, { appAuthored: true }))).toBeUndefined(); + }); + + it('its sudo() is marked, under every runAs', async () => { + for (const runAs of [undefined, 'user', 'system']) { + const context = await hookInsert(OPERATOR, { appAuthored: true, runAs }, (a) => a.sudo()); + expect(context.isSystem).toBe(true); + expect(appAutomationElevationOf(context)).toBe('hook'); + } + }); + + it('a platform wrap (no appAuthored) keeps the api by reference, and neither its inherited context nor its sudo() is marked', async () => { + const engine = recordingEngine(); + const ctx = makeCtx(OPERATOR, engine); + const original = ctx.api; + let seenApi: unknown; + await wrapDeclarativeHook(hookOf(undefined), async (c) => { seenApi = c.api; }, { logger: silentLogger })(ctx); + expect(seenApi).toBe(original); + expect(appAutomationElevationOf(await hookInsert(SYSTEM_TRIGGER, {}))).toBeUndefined(); + expect(appAutomationElevationOf(await hookInsert(OPERATOR, {}, (a) => a.sudo()))).toBeUndefined(); + }); + + it('the binder binds every hook as app-authored', async () => { + const engine = new ObjectQL(); + const seen: unknown[] = []; + bindHooksToEngine(engine, [{ ...hookOf(undefined), handler: async (c: HookContext) => { seen.push(c.api); } } as any], { packageId: 'qa' }); + const hook = (engine as any).hooks.get('afterInsert')?.[0]; + const ctx = makeCtx(SYSTEM_TRIGGER, recordingEngine()); + await hook.handler(ctx); + expect(seen[0]).not.toBe(ctx.api); + expect(appAutomationElevationOf((seen[0] as any).executionContext)).toBe('hook'); + }); +}); diff --git a/packages/objectql/src/hook-run-as.ts b/packages/objectql/src/hook-run-as.ts index d7c3529fb7d..74e3f1210d0 100644 --- a/packages/objectql/src/hook-run-as.ts +++ b/packages/objectql/src/hook-run-as.ts @@ -242,3 +242,44 @@ export async function seedHookUserGrants(api: unknown): Promise { if (!seedable || typeof seedable[SEED_USER_GRANTS] !== 'function') return api; return seedable[SEED_USER_GRANTS](); } + +/** + * [#25653] The key the engine's `ScopedContext` derives an APP-AUTHORED hook's + * api under: the api a hook bound through the hook binder (`bindHooksToEngine`, + * the one door metadata hooks come through) is handed, whatever its `runAs`. + * + * "App-authored" is the line the engine already draws for `skipAutomations`: + * a hook bound through the binder carries a metadata binding (`meta`) and is an + * automation; a hook registered in code with `engine.registerHook` and no + * `meta` (audit, security, every plugin's own hook) is platform code. The binder + * says so to the wrapper (`WrapDeclarativeOptions.appAuthored`), and the wrapper + * derives through this key. + * + * The derived api carries app-automation provenance in two places the refusal + * of identity-assignment writes (plugin-security) reads: + * - an inherited system context with no `flowRunId` and no mark of its own + * gets the `'hook'` mark (`@objectstack/core`): a hook that declares no + * `runAs` and fires on a platform system write runs elevated, and that + * elevation is the hook's; + * - its `sudo()`, on it and on every api derived from it (`transaction`, + * `withRunAs`), marks the elevation it makes. + * + * A symbol, and ⛔ not re-exported from the package entry, for the reason + * {@link SEED_USER_GRANTS} gives. + */ +export const APP_AUTHORED_HOOK_API: unique symbol = Symbol('objectstack.objectql.appAuthoredHookApi'); + +interface AppAuthoredDerivableApi { + [APP_AUTHORED_HOOK_API](): IScopedContext; +} + +/** + * Derive the api an app-authored hook runs with ({@link APP_AUTHORED_HOOK_API}). + * An api that cannot derive one (a hook context assembled by hand) is returned + * as is. + */ +export function deriveAppAuthoredHookApi(api: unknown): unknown { + const derivable = api as Partial | undefined; + if (!derivable || typeof derivable[APP_AUTHORED_HOOK_API] !== 'function') return api; + return derivable[APP_AUTHORED_HOOK_API](); +} diff --git a/packages/objectql/src/hook-wrappers.ts b/packages/objectql/src/hook-wrappers.ts index d89e3d5c963..57ef297afa4 100644 --- a/packages/objectql/src/hook-wrappers.ts +++ b/packages/objectql/src/hook-wrappers.ts @@ -23,7 +23,7 @@ import { noopHookMetricsRecorder, type HookMetricsRecorder, type HookMetricOutco import { materializeDeclaredFields } from './declared-fields.js'; import { describeCelFault, type CelFault } from './cel-fault.js'; import { nullPreviousReadDetail, ruleScope, type PreviousBinding } from './validation/rule-validator.js'; -import { hookRunAs, deriveHookApi, seedHookUserGrants, type HookRunAs } from './hook-run-as.js'; +import { hookRunAs, deriveHookApi, deriveAppAuthoredHookApi, seedHookUserGrants, type HookRunAs } from './hook-run-as.js'; /** * The logger the hook layer writes its diagnostics to — the `Logger` CONTRACT @@ -61,6 +61,14 @@ export interface WrapDeclarativeOptions { logger?: HookDiagnosticsLogger; /** Optional per-execution metrics sink. Defaults to no-op. */ metrics?: HookMetricsRecorder; + /** + * [#25653] The hook is app-authored: bound through the hook binder, which + * sets this. Its `ctx.api` is then derived through `APP_AUTHORED_HOOK_API` + * (`hook-run-as.ts`), so an inherited system context and its `sudo()` carry + * the app-automation mark. Absent for a platform hook wrapped in code (the + * audit builtins), whose api is handed through as before. + */ + appAuthored?: boolean; } const noopLogger: HookDiagnosticsLogger = { @@ -379,6 +387,7 @@ export function wrapDeclarativeHook( // binder skips (or, under `strict`, fails) the hook instead of running it // under an identity it never declared. `undefined` is the schema default. const runAs: HookRunAs = hookRunAs(meta as { name?: unknown; runAs?: unknown }); + const appAuthored = opts.appAuthored === true; const runWithTimeout = async (ctx: HookContext): Promise => { if (!timeoutMs) { @@ -470,7 +479,12 @@ export function wrapDeclarativeHook( // already ran against it; the readonly strip and the session keep reading // it). Under `'inherit'` this is a no-op that leaves the engine-built api // in place by reference — and, deliberately, without an extra `await`. - const restoreApi = runAs === 'inherit' ? noRestore : await installRunAsApi(ctx, runAs, meta.name, logger); + // [#25653] An app-authored hook's api is derived first (synchronously), so + // its inherited elevation and its `sudo()` carry the app-automation mark. + const installed = runAs === 'inherit' && !appAuthored + ? noRestore + : installRunAsApi(ctx, runAs, meta.name, logger, appAuthored); + const restoreApi = typeof installed === 'function' ? installed : await installed; const startedAt = Date.now(); const recordOutcome = (err?: any) => { @@ -497,8 +511,9 @@ export function wrapDeclarativeHook( // first `await` returns. So a non-`'inherit'` hook runs against a // detached view that keeps the derived api past that restore. Under // `'inherit'` the context object is handed through unchanged, exactly - // as before this key existed. - const detached = runAs === 'inherit' ? ctx : { ...ctx }; + // as before this key existed — unless the hook is app-authored, whose + // derived api (#25653) the same restore would take away. + const detached = restoreApi === noRestore ? ctx : { ...ctx }; void runWithErrorPolicy(detached) .then(() => recordOutcome()) .catch((err) => { @@ -531,7 +546,12 @@ export function wrapDeclarativeHook( * it, returning the restore. `'inherit'` installs nothing and restores nothing * — the engine-built api stays in place BY REFERENCE, which is the "byte- * identical to today" guarantee the ruling's default rests on and the pin in - * `hook-run-as.test.ts` asserts with `toBe`. + * `hook-run-as.test.ts` asserts with `toBe`. [#25653] Except for an + * app-authored hook, whose api is first derived through + * `APP_AUTHORED_HOOK_API`, under every `runAs`. + * + * Synchronous except for the `'user'` grant seeding, so a caller awaits only + * what is a promise. * * The derivation itself lives on the api (`ScopedContext.withRunAs`) because * only the api holds the full triggering `ExecutionContext` — `ctx.session` is @@ -542,21 +562,27 @@ function installRunAsApi( runAs: HookRunAs, hookName: string, logger: HookDiagnosticsLogger, -): Promise<() => void> { - if (runAs === 'inherit') return Promise.resolve(noRestore); + appAuthored = false, +): (() => void) | Promise<() => void> { const original = ctx.api; - const derived = deriveHookApi(original, runAs, { - hook: hookName, - object: ctx.object, - event: ctx.event, - }); + // [#25653] App-authored: derive the api that carries the hook's provenance + // BEFORE the `runAs` derivation, which keeps it. Synchronous, and the only + // step an `'inherit'` hook takes. + const base = appAuthored ? deriveAppAuthoredHookApi(original) : original; const install = (api: unknown) => { + if (api === original) return noRestore; ctx.api = api as HookContext['api']; return () => { ctx.api = original; }; }; - if (runAs !== 'user') return Promise.resolve(install(derived)); + if (runAs === 'inherit') return install(base); + const derived = deriveHookApi(base, runAs, { + hook: hookName, + object: ctx.object, + event: ctx.event, + }); + if (runAs !== 'user') return install(derived); // [#24497] A `'user'` pin carries the user's grants, peer list and email // (#24639), seeded through the resolver the request path uses // (`ScopedContext[SEED_USER_GRANTS]`, `hook-run-as.ts`). Cost: one diff --git a/packages/plugins/plugin-security/src/flow-system-identity-write-refusal.test.ts b/packages/plugins/plugin-security/src/flow-system-identity-write-refusal.test.ts index 85e38a3e099..03375401a38 100644 --- a/packages/plugins/plugin-security/src/flow-system-identity-write-refusal.test.ts +++ b/packages/plugins/plugin-security/src/flow-system-identity-write-refusal.test.ts @@ -355,7 +355,7 @@ describe('[#25653] the predicate also reads the app-automation elevation mark', const ASSIGNMENT = new Set(['sys_user_position', 'sys_user_permission_set']); for (const [source, context, subject, prescription] of [ - ['hook', HOOK_SYSTEM, /a hook that runs as system \(runAs: 'system'\)/, /Set the hook's runAs to 'user'/], + ['hook', HOOK_SYSTEM, /a hook that runs as system \(runAs: 'system', a system context it inherits, or ctx\.api\.sudo\(\)\)/, /Set the hook's runAs to 'user' and leave ctx\.api\.sudo\(\) out/], ['action', ACTION_SYSTEM, /an action body \(an action body runs as system\)/, /a flow whose runAs is 'user'/], ] as const) { for (const object of OBJECTS) { diff --git a/packages/plugins/plugin-security/src/flow-system-identity-write-refusal.ts b/packages/plugins/plugin-security/src/flow-system-identity-write-refusal.ts index a01fc4cfce8..92f9461dc35 100644 --- a/packages/plugins/plugin-security/src/flow-system-identity-write-refusal.ts +++ b/packages/plugins/plugin-security/src/flow-system-identity-write-refusal.ts @@ -54,10 +54,16 @@ * body's `ctx.api` / `ctx.engine` (`buildActionExecutionContext`, runtime). * Each is derived with the app-automation elevation mark * (`appAutomationElevationOf`, `@objectstack/core`), and this predicate reads - * it beside `flowRunId`: `isSystem` AND (`flowRunId` OR the mark). The mark is - * set only at those two derivations, never on `sudo()`, so a platform writer's - * system context — `sudo()`-derived or bare — carries neither and passes as - * before. Under a hook's `runAs: 'user'` the context loses `isSystem`, and the + * it beside `flowRunId`: `isSystem` AND (`flowRunId` OR the mark). + * + * The same mark covers an app-authored hook's other elevations: a system + * context it inherits when it declares no `runAs`, and its in-process + * `ctx.api.sudo()` (`ScopedContext[APP_AUTHORED_HOOK_API]`, objectql), and an + * action reached through `ctx.api.object(x).execute(y)` (`'action'`). A hook is + * app-authored when it is bound through the hook binder (a metadata binding, + * the line `skipAutomations` draws); a hook registered in code (audit, + * security, any plugin's) is platform code. Platform code's `sudo()` and a + * platform writer's bare system context carry no mark and pass as before. Under a hook's `runAs: 'user'` the context loses `isSystem`, and the * D12 gate judges the user it acts for, as for a `runAs: 'user'` run. * * A context carrying both a run and a mark (a `runAs: 'system'` hook fired by a @@ -99,7 +105,7 @@ type ElevationSource = 'flow' | AppAutomationElevation; /** How each source is named in the refusal. */ const SUBJECT: Readonly> = { flow: "a flow that runs as system (runAs: 'system')", - hook: "a hook that runs as system (runAs: 'system')", + hook: "a hook that runs as system (runAs: 'system', a system context it inherits, or ctx.api.sudo())", action: 'an action body (an action body runs as system)', }; @@ -115,7 +121,7 @@ const ASSIGNMENT_PRESCRIPTION: Readonly> = { "Set the flow's runAs to 'user': the delegated-administration gate then judges the user the run acts for, " + 'and records that user as the grant\'s granted_by.', hook: - "Set the hook's runAs to 'user': the delegated-administration gate then judges the user the hook acts for, " + "Set the hook's runAs to 'user' and leave ctx.api.sudo() out: the delegated-administration gate then judges the user the hook acts for, " + 'and records that user as the grant\'s granted_by.', action: "Make the grant from a flow whose runAs is 'user', which an action of type 'flow' runs as the invoking user: " @@ -140,8 +146,8 @@ function elevationSource(context: { isSystem?: unknown; flowRunId?: unknown } | /** * Refuse a governed write to {@link FLOW_SYSTEM_IDENTITY_WRITE_OBJECTS} from a - * system context a flow run opened, a hook's `runAs: 'system'` derived or an - * action body runs under; return for every other operation. + * system context a flow run opened, an app-authored hook elevated, or an action + * body runs under; return for every other operation. * * Called by the data middleware immediately before its `isSystem` bypass, so * the refused write never reaches the engine's write path. diff --git a/packages/runtime/src/app-automation-identity-write-refusal.integration.test.ts b/packages/runtime/src/app-automation-identity-write-refusal.integration.test.ts index 4dc9eae61cc..d5fa6224d61 100644 --- a/packages/runtime/src/app-automation-identity-write-refusal.integration.test.ts +++ b/packages/runtime/src/app-automation-identity-write-refusal.integration.test.ts @@ -28,6 +28,12 @@ * - platform code's `sudo()` and a platform writer's own system context: the writes land, * because neither carries the mark (`sudo()` never sets it). * + * The second slice closes the inherited path: a hook bound through the hook + * binder (app-authored) that declares no `runAs` and runs under a system + * context, its in-process `ctx.api.sudo()`, and an action it reaches through + * `ctx.api.object(x).execute(y)` are refused the same way. A hook registered in + * code, with no metadata binding (platform), keeps writing the four. + * * The system flow's own refusal is pinned, unchanged, beside the predicate * (`flow-system-identity-write-refusal.test.ts`, plugin-security). * @@ -36,7 +42,7 @@ */ import { describe, it, expect, vi, afterEach } from 'vitest'; -import { ObjectQL, bindHooksToEngine } from '@objectstack/objectql'; +import { ObjectQL, bindHooksToEngine, wrapDeclarativeHook } from '@objectstack/objectql'; import { SqlDriver } from '@objectstack/driver-sql'; import { PermissionSetSchema } from '@objectstack/spec/security'; import { SecurityPlugin } from '@objectstack/plugin-security'; @@ -352,3 +358,152 @@ describe('CONTROL: platform system writes carry no mark and still land', () => { }, 30000); } }); + +/** A host handler writing `verb` on `object` through the api it is handed. */ +function handlerWrite(object: string, verb: (typeof VERBS)[number]) { + return async (api: any): Promise => { + const repo = api.object(object); + if (verb === 'insert') await repo.insert(ROWS[object]('born')); + else if (verb === 'update') await repo.update(EDITS[object], { where: { id: 'seeded' } }); + else await repo.delete({ where: { id: 'seeded' } }); + }; +} + +/** The id of the `qa_note` row a hook fired for, on either face of its input. */ +const noteId = (hctx: any): unknown => hctx.result?.id ?? hctx.input?.data?.id ?? hctx.input?.id; + +/** Bind an app-authored hook with no `runAs`, fire it by `trigger`'s write, return the refusal. */ +async function fireInheritingHook(engine: ObjectQL, hook: Record, trigger: Record): Promise { + bindHooksToEngine(engine, [{ name: 'qa_inherit_hook', object: 'qa_note', events: ['afterInsert'], ...hook } as any], { packageId: 'qa' }); + return refusal(() => engine.insert('qa_note', { id: 'note_1', title: 'Note' }, { context: { ...trigger } } as never)); +} + +describe('an app-authored hook with no runAs, under a platform system write: governed writes to the four objects are refused', () => { + for (const surface of ['body', 'handler'] as const) { + for (const object of OBJECTS) { + for (const verb of VERBS) { + it(`${surface}: ${verb} on ${object} is refused and the store does not move`, async () => { + const engine = await boot(); + const before = await seed(engine, object); + const write = handlerWrite(object, verb); + const hook = surface === 'body' + ? { body: { language: 'js', source: bodySource(object, verb), capabilities: ['api.read', 'api.write'] } } + : { handler: async (hctx: any) => write(hctx.api) }; + + expectRefused(await fireInheritingHook(engine, hook, PLATFORM), object, verb, 'hook'); + + expect(await stored(engine, object, 'seeded')).toEqual(before); + expect(await stored(engine, object, 'born')).toBe(null); + expect(await stored(engine, 'qa_note', 'note_1')).toBe(null); + }, 30000); + } + } + } + + it('the same hook still writes every other object', async () => { + const engine = await boot(); + const err = await fireInheritingHook(engine, { + body: { + language: 'js', + source: `await ctx.api.object('qa_note').update({ title: 'stamped' }, { where: { id: 'note_1' } });`, + capabilities: ['api.read', 'api.write'], + }, + }, PLATFORM); + expect(err).toBe(null); + expect(await stored(engine, 'qa_note', 'note_1')).toMatchObject({ title: 'stamped' }); + }, 30000); +}); + +describe("an app-authored hook's in-process ctx.api.sudo(): governed writes to the four objects are refused", () => { + for (const object of OBJECTS) { + for (const verb of VERBS) { + it(`${verb} on ${object} is refused and the store does not move`, async () => { + const engine = await boot(); + const before = await seed(engine, object); + const write = handlerWrite(object, verb); + + expectRefused( + await fireInheritingHook(engine, { handler: async (hctx: any) => write(hctx.api.sudo()) }, PLAIN), + object, verb, 'hook', + ); + + expect(await stored(engine, object, 'seeded')).toEqual(before); + expect(await stored(engine, object, 'born')).toBe(null); + }, 30000); + } + } + + it("inside ctx.api.transaction, and under runAs: 'user', the same", async () => { + const engine = await boot(); + const write = handlerWrite('sys_member', 'insert'); + expectRefused( + await fireInheritingHook(engine, { + handler: async (hctx: any) => hctx.api.transaction(async (trx: any) => write(trx.sudo())), + }, PLAIN), + 'sys_member', 'insert', 'hook', + ); + expectRefused( + await fireInheritingHook(engine, { runAs: 'user', handler: async (hctx: any) => write(hctx.api.sudo()) }, PLAIN), + 'sys_member', 'insert', 'hook', + ); + expect(await stored(engine, 'sys_member', 'born')).toBe(null); + }, 30000); +}); + +describe("an action reached through ctx.api.object(x).execute(y): governed writes to the four objects are refused", () => { + for (const object of OBJECTS) { + it(`insert on ${object} is refused`, async () => { + const engine = await boot(); + const write = handlerWrite(object, 'insert'); + engine.registerAction('qa_note', 'qa_reached_action', async (actx: any) => write(actx.api)); + + expectRefused( + await fireInheritingHook(engine, { handler: async (hctx: any) => hctx.api.object('qa_note').execute('qa_reached_action') }, PLAIN), + object, 'insert', 'action', + ); + + expect(await stored(engine, object, 'born')).toBe(null); + }, 30000); + } +}); + +describe("CONTROL: an app-authored hook with no runAs, under an ordinary user's write, takes the ordinary gates", () => { + for (const object of OBJECTS) { + it(`${object}: the plain member's insert is refused by the ordinary gates, not by the elevation refusal`, async () => { + const engine = await boot(); + const write = handlerWrite(object, 'insert'); + const err = await fireInheritingHook(engine, { handler: async (hctx: any) => write(hctx.api) }, PLAIN); + expect(err, `the plain member's ${object} insert landed`).not.toBe(null); + expect(rootRefusal(err).code).toBe('PERMISSION_DENIED'); + expect(String(err.message)).not.toMatch(/governed identity writes/); + expect(await stored(engine, object, 'born')).toBe(null); + }, 30000); + } +}); + +describe('CONTROL: a hook registered in code, with no metadata binding, still writes the four objects', () => { + for (const object of OBJECTS) { + it(`${object}: inherited system context, its sudo(), and the declarative wrapper with no binding all land`, async () => { + const engine = await boot(); + const insert = (id: string) => async (api: any) => api.object(object).insert(ROWS[object](id)); + const byInherit = insert('by_inherit'); + const bySudo = insert('by_sudo'); + const byWrapped = insert('by_wrapped'); + engine.registerHook('afterInsert', async (hctx: any) => { + if (noteId(hctx) === 'note_sys') await byInherit(hctx.api); + if (noteId(hctx) === 'note_user') await bySudo(hctx.api.sudo()); + }, { object: 'qa_note', packageId: 'sys:qa' }); + const meta = { name: 'qa_platform_wrapped', object: 'qa_note', events: ['afterInsert'] } as any; + engine.registerHook('afterInsert', wrapDeclarativeHook(meta, async (hctx: any) => { + if (noteId(hctx) === 'note_sys') await byWrapped(hctx.api); + }), { object: 'qa_note', packageId: 'sys:qa' }); + + await engine.insert('qa_note', { id: 'note_sys', title: 'Note' }, { context: { ...PLATFORM } } as never); + await engine.insert('qa_note', { id: 'note_user', title: 'Note' }, { context: { ...PLAIN } } as never); + + for (const id of ['by_inherit', 'by_sudo', 'by_wrapped']) { + expect(await stored(engine, object, id), `${id} on ${object} did not land`).toMatchObject({ id }); + } + }, 30000); + } +});