diff --git a/.changeset/action-predicate-sparse-face-guards.md b/.changeset/action-predicate-sparse-face-guards.md new file mode 100644 index 0000000000..ad077bc7f0 --- /dev/null +++ b/.changeset/action-predicate-sparse-face-guards.md @@ -0,0 +1,17 @@ +--- +'@objectstack/platform-objects': patch +'@objectstack/plugin-approvals': patch +--- + +Guard every authored record-scoped action predicate for the sparse action face, so a list row that did not project the gated column no longer silently drops the button. + +An action's `visible` / `disabled` predicate binds whatever record the client already fetched — a record-detail read, or a list row carrying only the view's `$select` projection. That binding stays sparse by decision (it is the one record binding the platform does not make total), and CEL aborts the whole expression at key resolution when a key is absent. The abort is fail-closed, so the button is simply not offered — indistinguishable to the user from the gate having said no, and reported nowhere. + +Every authored predicate on `sys_user`, `sys_invitation`, `sys_member`, `sys_oauth_application` and `sys_approval_request` now opens each `record.*` read with `has()`. The guard is the minimal measured form per predicate, not one blanket rewrite: a bare equality against a literal needs `has()` alone, because CEL compares heterogeneously and answers `false` on a projected-null column rather than faulting. + +Two predicates change what a user sees, both on `sys_oauth_application`, whose `disabled` column is nullable upstream and therefore null on every application nobody has ever toggled: + +- `disable_oauth_application` was `!record.disabled`, which faulted on a projected-null row (`!` needs a bool) — so the Disable button was missing from every never-toggled application in the list. It is now `has(record.disabled) && record.disabled != true` and is offered. +- `enable_oauth_application` was `record.disabled`, which answered `null` rather than a boolean and left the decision to the renderer. It is now `has(record.disabled) && record.disabled == true`. + +`sys_approval_request`'s decision levers gate on the attached `record.viewer` block and traverse, so they are guarded at the leaf (`has(record.viewer) && has(record.viewer.can_act) && record.viewer.can_act == true`). Measured, that is the minimal safe form for a nested read: the canonical `has(x) && x != null` conjunction still faults when the block is present but the flag is absent or null, while a leaf `has()` subsumes the parent `!= null` half. Their intended fail-closed behaviour is unchanged — it is now a real `false` instead of an evaluation fault. diff --git a/content/docs/automation/approvals.mdx b/content/docs/automation/approvals.mdx index 6f4dcfe681..8dc18227b2 100644 --- a/content/docs/automation/approvals.mdx +++ b/content/docs/automation/approvals.mdx @@ -326,7 +326,11 @@ action on the object is gated on `record.viewer.can_act || record.viewer.can_override`, and the `viewer` block is computed and attached only by the approvals REST path (`/api/v1/approvals/*`) — never by the generic data API a plain object view reads. Browse the table directly and you get a correct, -completely inert list: the rows are there, the buttons are not. The table stays +completely inert list: the rows are there, the buttons are not. That inertness +is now a considered `false` rather than an evaluation fault — the predicates +guard the block with `has(record.viewer) && has(record.viewer.can_act)` before +reading it, so a row without the block resolves cleanly instead of aborting at +key resolution. Same empty toolbar, arrived at deliberately. The table stays available under **Setup → Approvals → Requests** for admins and diagnostics, which is what it is good for. diff --git a/content/docs/automation/flows.mdx b/content/docs/automation/flows.mdx index e65f1c5af9..22a0a72333 100644 --- a/content/docs/automation/flows.mdx +++ b/content/docs/automation/flows.mdx @@ -1268,8 +1268,11 @@ is *sparse* by decision: an [action](/docs/ui/actions) `visible` / `disabled` predicate is evaluated against whatever record the client already fetched — on a list row, only the columns that view actually projected. There `record.x != null` **is itself a fault** on an absent key, and the guard is the -conjunction `has(record.x) && record.x != null`. Flows are not that face; the -rule above holds here. Just don't carry it across to one where it doesn't. +conjunction `has(record.x) && record.x != null` — per READ, so a predicate that +traverses into `record.x` guards the leaf too +(`has(record.x) && has(record.x.k) && record.x.k == true`; the leaf guard +subsumes the parent `!= null`). Flows are not that face; the rule above holds +here. Just don't carry it across to one where it doesn't. ## Run a flow via API diff --git a/content/docs/protocol/objectui/actions.mdx b/content/docs/protocol/objectui/actions.mdx index bb7d0503c6..4b0da79b8b 100644 --- a/content/docs/protocol/objectui/actions.mdx +++ b/content/docs/protocol/objectui/actions.mdx @@ -327,6 +327,27 @@ Prefer the `record.` form: it reads unambiguously and resolves identicall Beyond `record`, action predicates also expose the authenticated user as `ctx.user` (e.g. `record.id == ctx.user.id`) and public feature flags as `features.*` (see [Capability gates](#capability-gates-requiresfeature)) — they are not limited to the record. [record-alert](/docs/protocol/objectui/record-alert) conditions surface the analogous identity values under the `os.*` namespace (`os.user` / `os.org` / `os.env`) instead. + +**The bound record is SPARSE — guard every read with `has()`.** Unlike a +validation or hook predicate, an action predicate is evaluated against whatever +record the client already fetched. On `list_item` that is the row the view's +`$select` projected, so a column the view did not ask for is **absent**, not +`null` — and CEL aborts the whole expression at key resolution (`No such key: +status`). The abort is fail-closed, so the button simply is not offered, which +looks exactly like the predicate having said no. Nothing logs it. + +| what the predicate does with `record.x` | guard | +|---|---| +| compares it to a literal (`==`, `!=`) | `has(record.x) && record.x == 'draft'` — `has()` alone. CEL compares heterogeneously, so a projected-`null` column answers `false` rather than faulting | +| traverses, calls a method, orders or does arithmetic | `has(record.x) && record.x != null && record.x.size() > 0` — both halves | +| traverses into a nested block | guard the **leaf**: `has(record.x) && has(record.x.k) && record.x.k == true`. The leaf guard subsumes the parent `!= null` | +| tests truthiness (`!record.x`, bare `record.x`) | rewrite to the equality form — `has(record.x) && record.x != true` / `== true`. `!` and the logical operators need a bool and **fault** on `null` | + +The last row is the one that bites: `visible: "!record.disabled"` faults on +every row whose `disabled` column is stored `NULL`, so the button vanishes from +exactly the records it was meant for. + + #### Capability gates: `requiresFeature` When an action (or param) only works with an opt-in auth capability behind it — the better-auth `admin` plugin, `phoneNumber`, `twoFactor`, … — gate it with **`requiresFeature: ''`** instead of a hand-written `features.*` predicate. The flag names the public feature flag served at `/api/v1/auth/config` (see `PUBLIC_AUTH_FEATURES` in `@objectstack/spec/kernel`); at parse time the schema lowers it into the canonical `visible` predicate — `features.X == true` for opt-in flags, `features.X != false` for default-on flags — AND-composing with any explicit `visible` you also declare, then strips itself from the output. An unknown flag name fails the parse. @@ -340,9 +361,9 @@ target: /api/v1/auth/admin/ban-user requiresFeature: admin # Composes with a residual row predicate: -# (!record.disabled) && features.oidcProvider != false +# (has(record.disabled) && record.disabled != true) && features.oidcProvider != false name: disable_oauth_application -visible: "!record.disabled" +visible: "has(record.disabled) && record.disabled != true" requiresFeature: oidcProvider ``` diff --git a/examples/app-crm/package.json b/examples/app-crm/package.json index a33bf9ed78..ae1f24534d 100644 --- a/examples/app-crm/package.json +++ b/examples/app-crm/package.json @@ -1,7 +1,7 @@ { "name": "@objectstack/example-crm", "version": "4.0.92", - "description": "Minimal CRM example — a smoke-test workspace that exercises the metadata loading pipeline (objects → views → app → dashboard → hook → flow → seed). For a full-featured enterprise CRM see https://github.com/objectstack-ai/hotcrm.", + "description": "Minimal CRM example \u2014 a smoke-test workspace that exercises the metadata loading pipeline (objects \u2192 views \u2192 app \u2192 dashboard \u2192 hook \u2192 flow \u2192 seed). For a full-featured enterprise CRM see https://github.com/objectstack-ai/hotcrm.", "license": "Apache-2.0", "private": true, "main": "./objectstack.config.ts", @@ -25,6 +25,7 @@ "devDependencies": { "@objectstack/cli": "workspace:*", "@objectstack/driver-sql": "workspace:*", + "@objectstack/formula": "workspace:*", "@objectstack/objectql": "workspace:*", "typescript": "^6.0.3", "vitest": "^4.1.10" diff --git a/examples/app-crm/src/actions/convert-lead.action.ts b/examples/app-crm/src/actions/convert-lead.action.ts index 4671bb4c28..e580123615 100644 --- a/examples/app-crm/src/actions/convert-lead.action.ts +++ b/examples/app-crm/src/actions/convert-lead.action.ts @@ -19,5 +19,14 @@ export const ConvertLeadAction = defineAction({ // converted — so the button disappears rather than the user clicking it and // hitting the flow's "already converted" guard screen. The flow keeps that // guard as a server-side backstop. - visible: 'record.status != "converted"', + // + // The `has()` half guards the SPARSE action face (#8990): this action reaches + // `list_item`, where the bound record is the row the view's `$select` + // projected. Without it, a lead list that does not project `status` aborts + // the predicate at key resolution (`No such key: status`) and the button + // silently vanishes for every row. `has()` alone is the guard because the + // operand is compared by bare equality against a literal — see + // `materializeDeclaredFields` in `@objectstack/objectql` for the full rule + // and for when the `!= null` half becomes load-bearing. + visible: 'has(record.status) && record.status != "converted"', }); diff --git a/examples/app-crm/test/action-predicate-sparse-face.test.ts b/examples/app-crm/test/action-predicate-sparse-face.test.ts new file mode 100644 index 0000000000..361b530b94 --- /dev/null +++ b/examples/app-crm/test/action-predicate-sparse-face.test.ts @@ -0,0 +1,67 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #8990 — the CRM example's row action survives the SPARSE action face. + * + * `crm_convert_lead` reaches `list_item`, so its `visible` predicate binds a + * LIST ROW carrying only the view's `$select` projection — not a total record. + * Unguarded, `record.status != "converted"` aborts at key resolution on any + * lead list that does not project `status`, and CEL's fault is fail-closed: the + * Convert Lead button silently is not offered, which looks exactly like the + * predicate having said no. + * + * This example is reference material — it is what an author (human or AI) + * copies when writing their first row action — so the guard being present here + * is worth a test of its own rather than review. The rule itself lives on + * `materializeDeclaredFields` in `@objectstack/objectql` (#8975). + */ + +import { describe, expect, it } from 'vitest'; +import { celEngine } from '@objectstack/formula'; +import { ConvertLeadAction } from '../src/actions/convert-lead.action.js'; + +function evaluate(source: string, record: Record): boolean | string { + const r = celEngine.evaluate({ dialect: 'cel', source }, { record, user: { id: 'u1' } }); + if (!r.ok) return `FAULT ${r.error.message.split('\n')[0].trim()}`; + return typeof r.value === 'boolean' ? r.value : `NON-BOOLEAN ${JSON.stringify(r.value)}`; +} + +/** + * `defineAction` normalizes the CEL shorthand string into a `{dialect, source}` + * envelope at parse time — read the source through the envelope, or the + * assertions run against `undefined` and pass for the wrong reason. + */ +const raw: unknown = ConvertLeadAction.visible; +const source = + typeof raw === 'string' + ? raw + : ((raw as { source?: string } | undefined)?.source ?? ''); + +describe('#8990 — crm_convert_lead visible on a sparse list row', () => { + + it('reaches list_item, so the sparse binding is the real one', () => { + expect(ConvertLeadAction.locations).toContain('list_item'); + }); + + it('is has()-guarded on the column it reads', () => { + expect(source).toContain('has(record.status)'); + }); + + it('answers false instead of faulting when the view did not project status', () => { + expect(evaluate(source, { id: 'lead_1', name: 'Acme' })).toBe(false); + // The unguarded spelling is what that binding used to do — pinned so the + // regression is recognisable rather than re-derived. + expect(evaluate('record.status != "converted"', { id: 'lead_1', name: 'Acme' })) + .toBe('FAULT No such key: status'); + }); + + it('still hides the button on a converted lead and offers it otherwise', () => { + expect(evaluate(source, { status: 'converted' })).toBe(false); + expect(evaluate(source, { status: 'new' })).toBe(true); + expect(evaluate(source, { status: 'qualified' })).toBe(true); + // A projected-but-null status is not "converted", so the button is offered + // — the equality operand never faults on null, which is why `has()` alone + // is the whole guard here. + expect(evaluate(source, { status: null })).toBe(true); + }); +}); diff --git a/examples/app-crm/tsconfig.json b/examples/app-crm/tsconfig.json index 2c1c0d4af0..3c1ac0e423 100644 --- a/examples/app-crm/tsconfig.json +++ b/examples/app-crm/tsconfig.json @@ -7,8 +7,29 @@ "esModuleInterop": true, "forceConsistentCasingInFileNames": true, "skipLibCheck": true, - "outDir": "./dist", - "rootDir": "." + // `outDir` / `rootDir` were removed and `noEmit` set as a CONSEQUENCE of + // the `paths` block below — the same correction + // `packages/qa/downstream-contract` made for the same reason. tsc emits + // nothing for this app either way (`typecheck` is `tsc --noEmit`; the app + // is BUILT by the ObjectStack CLI, not by tsc), but an emit-shaped config + // still enforces the output-tree rules: `rootDir` is inferred from the + // `include` roots, so formula's source — pulled in by an import, never a + // root — produced a wall of `TS6059: File '.../packages/formula/src/...' + // is not under 'rootDir'` that would drown any real error this typecheck + // exists to print. + "noEmit": true, + // + // #8990: `test/action-predicate-sparse-face.test.ts` imports the CEL engine + // to evaluate this app's row-action predicate. Without this rule the import + // resolves through the workspace link to `packages/formula/dist/*.d.ts` — a + // BUILD ARTIFACT — so `tsc --noEmit` would be checking against whatever was + // last built rather than against the source in this checkout, and a stale + // `dist` typechecks green over an engine contract that has since moved. + // `pnpm check:type-source-resolution` is the gate; it wants the `paths` + // rule, not a registry entry. + "paths": { + "@objectstack/formula": ["../../packages/formula/src/index.ts"] + } }, - "include": ["src/**/*", "objectstack.config.ts", "test/**/*"] -} \ No newline at end of file + "include": ["src/**/*", "objectstack.config.ts", "test/**/*", "vitest.config.ts"] +} diff --git a/examples/app-crm/vitest.config.ts b/examples/app-crm/vitest.config.ts new file mode 100644 index 0000000000..cf7e440d44 --- /dev/null +++ b/examples/app-crm/vitest.config.ts @@ -0,0 +1,28 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +import { defineConfig } from 'vitest/config'; +import path from 'node:path'; + +export default defineConfig({ + resolve: { + // `action-predicate-sparse-face.test.ts` (#8990) drives this app's row + // action predicate through the CEL engine itself, because the whole point + // of that pin is what the ENGINE answers on a sparse list row — a fault + // versus a considered `false`, which are the same pixel to the user. + // Unaliased, `@objectstack/formula` resolves through the workspace link to + // `dist/` — a build artifact — so the verdict would be a function of build + // state rather than of the source in this checkout. The loud half (a + // missing export) is the mild one; a dist merely BEHIND runs the suite + // GREEN against the engine's OLD null/absence semantics, which is exactly + // the behaviour these assertions exist to pin. `pnpm check:test-source-alias` + // is the gate. + // + // ANCHORED regex, array form, deliberately: a bare string `find` matches by + // PREFIX, so with a FILE replacement it would also swallow any subpath and + // resolve it to `…/formula/src/index.ts/` — `ENOTDIR` at run time, + // from a config that reads as correct. + alias: [ + { find: /^@objectstack\/formula$/, replacement: path.resolve(__dirname, '../../packages/formula/src/index.ts') }, + ], + }, +}); diff --git a/packages/objectql/src/declared-fields.ts b/packages/objectql/src/declared-fields.ts index 7baac95937..d48038f637 100644 --- a/packages/objectql/src/declared-fields.ts +++ b/packages/objectql/src/declared-fields.ts @@ -64,6 +64,27 @@ * from the gate having said no — so trading a null-shaped vanish for an * absence-shaped one buys nothing. * + * ## The conjunction is the guard for ONE read — a nested path has two (#8990) + * + * `has(record.x) && record.x != null` guards the read of `x`. When the + * predicate then traverses INTO `x`, the leaf is a second read and a second + * operator, and the conjunction above does not reach it. Measured on the same + * engine, migrating `sys_approval_request`'s `record.viewer.*` levers: + * + * | predicate | `{}` | `{v: null}` | `{v: {}}` | `{v: {a: null}}` | `{v: {a: true}}` | + * |:-----------------------------------------------------------|:-----|:------------|:----------|:-----------------|:-----------------| + * | `has(record.v) && record.v != null && record.v.a` | `false` | `false` | FAULT `No such key: a` | FAULT `Logical operator requires bool operands` | `true` | + * | `has(record.v) && has(record.v.a) && record.v.a == true` | `false` | `false` | `false` | `false` | `true` | + * + * So guard the LEAF, and note that doing so SUBSUMES the parent `!= null` + * half: `has()` on a path whose parent is null answers `false` rather than + * faulting, which is why the second row needs no `record.v != null` term. The + * outer `has()` is still load-bearing (`has(record.v.a)` alone faults + * `No such key: v` on `{}`), and so is the `== true` comparison — a bare + * truthy read of a null leaf faults the logical operator. Adding the parent + * `!= null` back is harmless but redundant; adding it INSTEAD of the leaf + * `has()` is the mistake this table exists to prevent. + * * One measured exception, recorded so the platform's existing predicates are * not misread: a bare EQUALITY against a literal never faults on a null value * — CEL compares heterogeneously and answers `false` — so `has(record.a) && diff --git a/packages/platform-objects/src/identity/action-predicate-sparse-face.test.ts b/packages/platform-objects/src/identity/action-predicate-sparse-face.test.ts new file mode 100644 index 0000000000..b7a7e8536c --- /dev/null +++ b/packages/platform-objects/src/identity/action-predicate-sparse-face.test.ts @@ -0,0 +1,287 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #8990 — every record-scoped action predicate on a platform object survives + * the SPARSE action face. + * + * The action `visible` / `disabled` binding is the one record binding that is + * deliberately NOT made total (#4953 item 2): it is whatever record the client + * already fetched — a record-detail read, or a LIST ROW carrying only the + * view's `$select` projection. `materializeDeclaredFields` in + * `@objectstack/objectql` is the canonical statement of the guard rule that + * follows from it (#8975); this file is the executable half of it for the + * identity objects and does not restate the rule. + * + * **What makes this worth pinning rather than reviewing.** The failure is + * fail-closed AND silent: CEL aborts the whole expression at key resolution + * (`No such key: source`), the predicate never returns, and the console simply + * does not offer the button. To the user that is indistinguishable from the + * gate having said no, so nothing anywhere reports it — not a log the operator + * reads, not a test, not a type. A predicate that regresses to the unguarded + * spelling would therefore ship green in every other sense. + * + * The assertions are deliberately at TWO levels, because either alone goes + * blind in a way measured on this exact surface: + * + * - a **sweep** over every action of every identity object, evaluating each + * predicate on the three bindings a sparse row can present (key absent, key + * projected holding null, key projected with a value). This is the one that + * catches a NEW action authored with an unguarded predicate — the sweep + * discovers the predicates rather than being handed a list. + * - **per-site verdicts** on the migrated predicates, so a guard cannot be + * "fixed" into something that never faults and never answers true either. + * A predicate wrapped in a guard that is accidentally always-false is the + * exact same user-visible outcome as the bug — the button is not offered — + * and the sweep alone cannot tell them apart. + * + * The `sys_oauth_application` pair carries a third assertion: it is the site + * where the migration CHANGES what a user sees, and the old spelling is pinned + * as faulting so the flip is recorded rather than asserted. + */ + +import { describe, expect, it } from 'vitest'; +import { celEngine } from '@objectstack/formula'; +import { SysUser } from './sys-user.object.js'; +import { SysInvitation } from './sys-invitation.object.js'; +import { SysMember } from './sys-member.object.js'; +import { SysOauthApplication } from './sys-oauth-application.object.js'; +import { SysOrganization } from './sys-organization.object.js'; +import { SysTeam } from './sys-team.object.js'; +import { SysTeamMember } from './sys-team-member.object.js'; +import { SysApiKey } from './sys-api-key.object.js'; +import { SysSsoProvider } from './sys-sso-provider.object.js'; +import { SysScimProvider } from './sys-scim-provider.object.js'; +import { SysTwoFactor } from './sys-two-factor.object.js'; +import { SysAccount } from './sys-account.object.js'; +import { SysSession } from './sys-session.object.js'; +import { SysUserPreference } from './sys-user-preference.object.js'; +import { SysBusinessUnit } from './sys-business-unit.object.js'; +import { SysBusinessUnitMember } from './sys-business-unit-member.object.js'; + +const USER = { id: 'u1', email: 'me@example.com' }; + +/** + * `defineObject` normalizes a CEL shorthand string into a `{dialect, source}` + * envelope at parse time, and the `requiresFeature` lowering AND-composes its + * own term onto the authored predicate — so the stored value is an envelope + * whose source is not byte-identical to what the file spells. Read through + * this rather than the raw key, or the assertions run against `undefined` and + * pass for the wrong reason. + */ +function sourceOf(raw: unknown): string | undefined { + if (typeof raw === 'string') return raw; + if (raw && typeof raw === 'object' && typeof (raw as { source?: unknown }).source === 'string') { + return (raw as { source: string }).source; + } + return undefined; +} + +/** + * Every capability flag ON. Several of these actions carry `requiresFeature`, + * whose lowering AND-composes a `features.*` term onto the authored predicate, + * so the stored source reads a namespace the record binding does not carry — + * without it the sweep reports `Unknown variable: features` for every gated + * action and never reaches the record guard it exists to test. All-on is the + * right setting here: a flag that is off short-circuits the composed `&&` and + * hides the record half from the sweep entirely. + */ +const FEATURES = { + organization: true, multiOrgEnabled: true, twoFactor: true, + oidcProvider: true, admin: true, phoneNumber: true, apiKey: true, sso: true, +}; + +/** Evaluate through the canonical engine; a fault is reported, never thrown. */ +function evaluate(source: string, record: Record): boolean | string { + const r = celEngine.evaluate({ dialect: 'cel', source }, { record, user: USER, extra: { features: FEATURES } }); + if (!r.ok) return `FAULT ${r.error.message.split('\n')[0].trim()}`; + return typeof r.value === 'boolean' ? r.value : `NON-BOOLEAN ${JSON.stringify(r.value)}`; +} + +/** Every `record.` path the predicate reads, deepest-first. */ +function recordPaths(source: string): string[] { + const out = new Set(); + for (const m of source.matchAll(/record((?:\.[a-z_][a-z0-9_]*)+)/gi)) out.add(m[1].slice(1)); + return [...out]; +} + +/** + * The bindings a sparse row can present for one predicate: the empty row, and + * — for each path the predicate reads — that path projected holding null while + * the rest stay absent, plus the fully-projected row with plausible values. + */ +function sparseBindings(source: string): Array<[string, Record]> { + const paths = recordPaths(source); + const cases: Array<[string, Record]> = [['{} (nothing projected)', {}]]; + const assign = (rec: Record, path: string, value: unknown) => { + const parts = path.split('.'); + let cur = rec; + for (const p of parts.slice(0, -1)) cur = (cur[p] ??= {}) as Record; + cur[parts[parts.length - 1]] = value; + }; + for (const path of paths) { + const rec: Record = {}; + assign(rec, path, null); + cases.push([`${path} projected as null`, rec]); + } + const all: Record = {}; + for (const path of paths) assign(all, path, 'x'); + cases.push(['every read projected with a value', all]); + const allTrue: Record = {}; + for (const path of paths) assign(allTrue, path, true); + cases.push(['every read projected as true', allTrue]); + return cases; +} + +const OBJECTS: Array<[string, { actions?: unknown }]> = [ + ['sys_user', SysUser], + ['sys_invitation', SysInvitation], + ['sys_member', SysMember], + ['sys_oauth_application', SysOauthApplication], + ['sys_organization', SysOrganization], + ['sys_team', SysTeam], + ['sys_team_member', SysTeamMember], + ['sys_api_key', SysApiKey], + ['sys_sso_provider', SysSsoProvider], + ['sys_scim_provider', SysScimProvider], + ['sys_two_factor', SysTwoFactor], + ['sys_account', SysAccount], + ['sys_session', SysSession], + ['sys_user_preference', SysUserPreference], + ['sys_business_unit', SysBusinessUnit], + ['sys_business_unit_member', SysBusinessUnitMember], +]; + +/** Every `(object, action, key, source)` whose predicate reads `record.*`. */ +function recordScopedPredicates(): Array<{ object: string; action: string; key: string; source: string }> { + const found: Array<{ object: string; action: string; key: string; source: string }> = []; + for (const [objectName, def] of OBJECTS) { + for (const action of (def.actions ?? []) as Array>) { + for (const key of ['visible', 'disabled'] as const) { + const source = sourceOf(action[key]); + if (!source || !/\brecord\./.test(source)) continue; + found.push({ object: objectName, action: String(action.name), key, source }); + } + } + } + return found; +} + +describe('#8990 — record-scoped action predicates on the sparse face', () => { + const PREDICATES = recordScopedPredicates(); + + it('the census is non-empty — a sweep over zero predicates proves nothing', () => { + // Guards the sweep below against silently going vacuous if the extraction + // ever stops matching (a renamed key, an envelope shape it does not read). + expect(PREDICATES.length).toBeGreaterThanOrEqual(13); + }); + + it('no predicate faults on any binding a sparse row can present', () => { + const faults: string[] = []; + for (const p of PREDICATES) { + for (const [label, record] of sparseBindings(p.source)) { + const verdict = evaluate(p.source, record); + if (typeof verdict !== 'boolean') { + faults.push(`${p.object}.${p.action}.${p.key} on ${label}: ${verdict}\n ${p.source}`); + } + } + } + expect(faults).toEqual([]); + }); + + it('every record read is opened by a has() guard on its own path', () => { + // The sweep above is a behavioural check and can be satisfied by accident + // (a predicate that reads only always-projected columns passes it today + // and breaks the day a view narrows its `$select`). This one is structural: + // it asks that the AUTHORED form carry the guard, so the next predicate is + // written correctly rather than measured lucky. + const unguarded: string[] = []; + for (const p of PREDICATES) { + for (const path of recordPaths(p.source)) { + if (!p.source.includes(`has(record.${path})`)) { + unguarded.push(`${p.object}.${p.action}.${p.key} reads record.${path} unguarded\n ${p.source}`); + } + } + } + expect(unguarded).toEqual([]); + }); +}); + +describe('#8990 — per-site verdicts (a guard must not be an always-false wrapper)', () => { + const visibleOf = (def: { actions?: unknown }, name: string): string => { + const action = ((def.actions ?? []) as Array>).find((a) => a.name === name); + if (!action) throw new Error(`no action ${name}`); + const source = sourceOf(action.visible); + if (!source) throw new Error(`action ${name} has no CEL source`); + return source; + }; + + it('sys_user account-settings actions still answer TRUE for the row owner', () => { + const own = { id: 'u1', source: 'local', email_verified: false, two_factor_enabled: false }; + expect(evaluate(visibleOf(SysUser, 'update_my_profile'), own)).toBe(true); + expect(evaluate(visibleOf(SysUser, 'change_my_password'), own)).toBe(true); + expect(evaluate(visibleOf(SysUser, 'resend_verification_email'), own)).toBe(true); + expect(evaluate(visibleOf(SysUser, 'enable_two_factor'), own)).toBe(true); + // ... and still FALSE for the cases the predicates exist to exclude. + expect(evaluate(visibleOf(SysUser, 'change_my_password'), { ...own, source: 'idp_provisioned' })).toBe(false); + expect(evaluate(visibleOf(SysUser, 'update_my_profile'), { ...own, id: 'someone_else' })).toBe(false); + expect(evaluate(visibleOf(SysUser, 'enable_two_factor'), { ...own, two_factor_enabled: true })).toBe(false); + expect(evaluate(visibleOf(SysUser, 'disable_two_factor'), { ...own, two_factor_enabled: true })).toBe(true); + }); + + it('sys_invitation accept/decline are offered to the recipient on a pending row', () => { + const row = { email: 'me@example.com', status: 'pending' }; + expect(evaluate(visibleOf(SysInvitation, 'accept_invitation'), row)).toBe(true); + expect(evaluate(visibleOf(SysInvitation, 'reject_invitation'), row)).toBe(true); + expect(evaluate(visibleOf(SysInvitation, 'accept_invitation'), { ...row, email: 'other@example.com' })).toBe(false); + expect(evaluate(visibleOf(SysInvitation, 'accept_invitation'), { ...row, status: 'accepted' })).toBe(false); + }); + + it('sys_member transfer_ownership is offered on a non-owner row only', () => { + expect(evaluate(visibleOf(SysMember, 'transfer_ownership'), { role: 'member' })).toBe(true); + expect(evaluate(visibleOf(SysMember, 'transfer_ownership'), { role: 'owner' })).toBe(false); + }); +}); + +describe('#8990 — sys_oauth_application: the binding where the migration changes what a user sees', () => { + /** + * `disabled` is nullable upstream — better-auth writes the column only when + * the flag is set — so a list row can carry it PROJECTED AND NULL, which is + * the ordinary state of every application nobody has ever toggled. Both old + * spellings broke on exactly that row, in different ways. + */ + const NEVER_TOGGLED = { disabled: null }; + + it('the OLD spelling faulted on a projected-null row (this is the defect, pinned)', () => { + // `!` is an operator that needs a bool, so a null operand is a fault, not + // a falsy read — and the fault is fail-closed, so the button vanished. + expect(evaluate('!record.disabled', NEVER_TOGGLED)).toBe('FAULT no such overload: !null'); + // The Enable side did not fault; it answered a non-boolean, which is its + // own defect — the renderer, not the predicate, decided what that meant. + expect(evaluate('record.disabled', NEVER_TOGGLED)).toBe('NON-BOOLEAN null'); + }); + + it('the migrated spellings answer a real boolean — Disable offered, Enable not', () => { + const find = (name: string) => + sourceOf(((SysOauthApplication.actions ?? []) as Array>) + .find((a) => a.name === name)!.visible)!; + // NB: both carry `requiresFeature: 'oidcProvider'`, which the lowering + // AND-composes onto the authored predicate — so these sources also read + // `features.oidcProvider`, and the bindings below supply it. + const disable = find('disable_oauth_application'); + const enable = find('enable_oauth_application'); + + expect(evaluate(disable, NEVER_TOGGLED)).toBe(true); + expect(evaluate(enable, NEVER_TOGGLED)).toBe(false); + + // The rest of the truth table is unchanged by the migration. + expect(evaluate(disable, { disabled: false })).toBe(true); + expect(evaluate(enable, { disabled: false })).toBe(false); + expect(evaluate(disable, { disabled: true })).toBe(false); + expect(evaluate(enable, { disabled: true })).toBe(true); + + // And on a row that never projected the column at all, both stay closed — + // there is nothing to decide from, so offering neither is the right answer. + expect(evaluate(disable, {})).toBe(false); + expect(evaluate(enable, {})).toBe(false); + }); +}); diff --git a/packages/platform-objects/src/identity/sys-invitation.object.ts b/packages/platform-objects/src/identity/sys-invitation.object.ts index 3d921601d3..7edc6eade5 100644 --- a/packages/platform-objects/src/identity/sys-invitation.object.ts +++ b/packages/platform-objects/src/identity/sys-invitation.object.ts @@ -103,6 +103,16 @@ export const SysInvitation = ObjectSchema.create({ // by an "Inbox / Pending invitations" list opened from the user's // own account page. The recipient-only `visible` predicate keeps // them out of the admin org-management view. + // + // Both predicates open each operand with `has()` for the SPARSE action + // face (#8990): they reach `list_item`, so the bound record is the row the + // inbox view projected, and an unprojected `email` / `status` aborts the + // whole predicate at key resolution instead of reading null — fail-closed, + // so the Accept/Decline pair vanishes with no way to tell that from the + // gate having said no. `has()` alone suffices because both operands are + // compared by bare equality against a literal; `materializeDeclaredFields` + // in `@objectstack/objectql` states the rule and when the `!= null` half + // is additionally required. { name: 'accept_invitation', label: 'Accept Invitation', @@ -112,7 +122,7 @@ export const SysInvitation = ObjectSchema.create({ type: 'api', target: '/api/v1/auth/organization/accept-invitation', recordIdParam: 'invitationId', - visible: "record.email == ctx.user.email && record.status == 'pending'", + visible: "has(record.email) && record.email == ctx.user.email && has(record.status) && record.status == 'pending'", successMessage: 'Invitation accepted', refreshAfter: true, }, @@ -125,7 +135,7 @@ export const SysInvitation = ObjectSchema.create({ type: 'api', target: '/api/v1/auth/organization/reject-invitation', recordIdParam: 'invitationId', - visible: "record.email == ctx.user.email && record.status == 'pending'", + visible: "has(record.email) && record.email == ctx.user.email && has(record.status) && record.status == 'pending'", confirmText: 'Decline this invitation? The inviter will be notified and you will need a new invitation to join.', successMessage: 'Invitation declined', refreshAfter: true, diff --git a/packages/platform-objects/src/identity/sys-member.object.ts b/packages/platform-objects/src/identity/sys-member.object.ts index 9373f1ae31..fdb79af0f9 100644 --- a/packages/platform-objects/src/identity/sys-member.object.ts +++ b/packages/platform-objects/src/identity/sys-member.object.ts @@ -115,7 +115,12 @@ export const SysMember = ObjectSchema.create({ bodyExtra: { role: 'owner' }, // The residual row predicate stays hand-written; the feature gate is // AND-composed onto it by the requiresFeature lowering. - visible: "record.role != 'owner'", + // `has()` guards the SPARSE action face (#8990): this is a `list_item` + // action, so a member list that does not project `role` would abort the + // predicate at key resolution and drop the button silently. `has()` + // alone — the operand is a bare equality against a literal (see + // `materializeDeclaredFields` in `@objectstack/objectql`). + visible: "has(record.role) && record.role != 'owner'", requiresFeature: 'organization', confirmText: 'Transfer ownership of this organization to the selected member? You will be demoted to admin and lose owner-only privileges.', successMessage: 'Ownership transferred', diff --git a/packages/platform-objects/src/identity/sys-oauth-application.object.ts b/packages/platform-objects/src/identity/sys-oauth-application.object.ts index ee4035029f..853af47f62 100644 --- a/packages/platform-objects/src/identity/sys-oauth-application.object.ts +++ b/packages/platform-objects/src/identity/sys-oauth-application.object.ts @@ -54,6 +54,24 @@ export const SysOauthApplication = ObjectSchema.create({ // the auth namespace (no generic data-layer bypass). When upstream // ships `disabled` support, retarget the enable/disable actions and // delete the bridge route. + // + // The two toggle predicates are guarded for the SPARSE action face (#8990), + // and this pair is where the guard actually changes what a user sees. Both + // reach `list_item`, and `disabled` is nullable upstream (better-auth writes + // the column only when it is set), so a list row can carry it PROJECTED AND + // NULL. Measured against the `@objectstack/formula` CEL engine, the old + // spellings both broke on exactly that row: `!record.disabled` FAULTED + // (`no such overload: !null`) — fail-closed, so the Disable button silently + // vanished from every never-toggled application — and `record.disabled` + // answered `null` rather than a bool. + // + // The rewrite is `!x` → `x != true` and bare `x` → `x == true`, not the + // conjunction: a bare EQUALITY against a literal never faults on null (CEL + // compares heterogeneously), so `has()` is the whole guard here, while `!` + // and a bare truthy read are operators that need a bool and fault on one. + // The equality form also keeps the intended meaning of a null column — + // never disabled, so Disable is offered and Enable is not. See + // `materializeDeclaredFields` in `@objectstack/objectql` for the rule. actions: [ { name: 'disable_oauth_application', @@ -74,7 +92,7 @@ export const SysOauthApplication = ObjectSchema.create({ description: 'Disable this OAuth application? Active access/refresh tokens issued to it will continue to be rejected at the token, authorize, and introspect endpoints. Existing integrations will stop working immediately.', successMessage: 'OAuth application disabled', refreshAfter: true, - visible: '!record.disabled', + visible: 'has(record.disabled) && record.disabled != true', bodyExtra: { disabled: true }, params: [ { name: 'client_id', field: 'client_id', defaultFromRow: true, required: true }, @@ -95,7 +113,7 @@ export const SysOauthApplication = ObjectSchema.create({ description: 'Re-enable this OAuth application? Token issuance, authorization, and introspection will resume immediately.', successMessage: 'OAuth application enabled', refreshAfter: true, - visible: 'record.disabled', + visible: 'has(record.disabled) && record.disabled == true', bodyExtra: { disabled: false }, params: [ { name: 'client_id', field: 'client_id', defaultFromRow: true, required: true }, diff --git a/packages/platform-objects/src/identity/sys-user.object.ts b/packages/platform-objects/src/identity/sys-user.object.ts index e63797f060..79adba85e9 100644 --- a/packages/platform-objects/src/identity/sys-user.object.ts +++ b/packages/platform-objects/src/identity/sys-user.object.ts @@ -288,6 +288,20 @@ export const SysUser = ObjectSchema.create({ // i.e. opened from the user's own detail page or a "My Account" view — // via the `visible` CEL predicate. Admin equivalents (set_user_password // for any account) are above and stay separate. + // + // Each predicate opens with `has()` per operand for the SPARSE action face + // (#8990): the action binding is whatever record the client already + // fetched, so a column the caller did not project is ABSENT, and CEL + // aborts the whole expression at key resolution (`No such key: source`) + // rather than reading null. The fault is fail-closed, so the button just + // stops being offered — indistinguishable from the gate saying no. + // `has()` alone is the guard here and the `!= null` half is deliberately + // NOT added: every operand below is compared by bare equality against a + // literal, and CEL compares heterogeneously, so a projected-null column + // answers false instead of faulting (measured). The canonical rule lives + // on `materializeDeclaredFields` in `@objectstack/objectql`; the second + // half is required only before traversal, a method call, ordering or + // arithmetic. { name: 'update_my_profile', label: 'Update Profile', @@ -297,7 +311,7 @@ export const SysUser = ObjectSchema.create({ locations: ['record_header'], type: 'api', target: '/api/v1/auth/update-user', - visible: 'record.id == ctx.user.id', + visible: 'has(record.id) && record.id == ctx.user.id', successMessage: 'Profile updated', refreshAfter: true, params: [ @@ -317,7 +331,7 @@ export const SysUser = ObjectSchema.create({ // password form so they can't self-mint a password that bypasses // enforced SSO. The break-glass owner (env-native, or flipped back when // their break-glass password is set) keeps it. ADR-0024 D4/D5.2. - visible: 'record.id == ctx.user.id && record.source != "idp_provisioned"', + visible: 'has(record.id) && record.id == ctx.user.id && has(record.source) && record.source != "idp_provisioned"', successMessage: 'Password changed', refreshAfter: false, params: [ @@ -336,7 +350,7 @@ export const SysUser = ObjectSchema.create({ target: '/api/v1/auth/change-email', // A managed user's email is owned by the IdP — a local change would // desync. Hide for IdP-provisioned; env-native users keep it. - visible: 'record.id == ctx.user.id && record.source != "idp_provisioned"', + visible: 'has(record.id) && record.id == ctx.user.id && has(record.source) && record.source != "idp_provisioned"', successMessage: 'Verification email sent — check the new address to confirm.', refreshAfter: false, params: [ @@ -353,7 +367,7 @@ export const SysUser = ObjectSchema.create({ target: '/api/v1/auth/send-verification-email', // Only render for the row owner AND when their email is still // unverified — there's nothing to resend once verified. - visible: 'record.id == ctx.user.id && record.email_verified == false', + visible: 'has(record.id) && record.id == ctx.user.id && has(record.email_verified) && record.email_verified == false', successMessage: 'Verification email sent — check your inbox.', refreshAfter: false, params: [], @@ -369,7 +383,7 @@ export const SysUser = ObjectSchema.create({ target: '/api/v1/auth/delete-user', // Self-delete needs a local password; managed users are deprovisioned // via the IdP (org-removal / SCIM), not local self-service. Hide for them. - visible: 'record.id == ctx.user.id && record.source != "idp_provisioned"', + visible: 'has(record.id) && record.id == ctx.user.id && has(record.source) && record.source != "idp_provisioned"', // Confirm question on `description` — one dialog, not two (#7278/#7309). description: 'Permanently delete your account? This cannot be undone — all your sessions will be terminated and all data you own will be removed per the configured retention policy.', successMessage: 'Account deleted', @@ -392,7 +406,7 @@ export const SysUser = ObjectSchema.create({ locations: ['record_section'], type: 'api', target: '/api/v1/auth/two-factor/enable', - visible: 'record.id == ctx.user.id && record.two_factor_enabled != true', + visible: 'has(record.id) && record.id == ctx.user.id && has(record.two_factor_enabled) && record.two_factor_enabled != true', requiresFeature: 'twoFactor', successMessage: 'Two-factor authentication enabled. Scan the QR code or paste the otpauth URI into your authenticator app, then verify a code to complete setup.', refreshAfter: true, @@ -408,7 +422,7 @@ export const SysUser = ObjectSchema.create({ locations: ['record_section'], type: 'api', target: '/api/v1/auth/two-factor/disable', - visible: 'record.id == ctx.user.id && record.two_factor_enabled == true', + visible: 'has(record.id) && record.id == ctx.user.id && has(record.two_factor_enabled) && record.two_factor_enabled == true', requiresFeature: 'twoFactor', // Confirm question on `description` — one dialog, not two (#7278/#7309). description: 'Turn off two-factor authentication? Your account will be less secure.', @@ -426,7 +440,7 @@ export const SysUser = ObjectSchema.create({ locations: ['record_section'], type: 'api', target: '/api/v1/auth/two-factor/generate-backup-codes', - visible: 'record.id == ctx.user.id && record.two_factor_enabled == true', + visible: 'has(record.id) && record.id == ctx.user.id && has(record.two_factor_enabled) && record.two_factor_enabled == true', requiresFeature: 'twoFactor', // Confirm question on `description` — one dialog, not two (#7278/#7309). description: 'Generate a new set of backup codes? Any previously generated codes will stop working.', diff --git a/packages/platform-objects/src/platform-objects.test.ts b/packages/platform-objects/src/platform-objects.test.ts index 01eb8b01a5..bced5be6e3 100644 --- a/packages/platform-objects/src/platform-objects.test.ts +++ b/packages/platform-objects/src/platform-objects.test.ts @@ -188,10 +188,10 @@ describe('@objectstack/platform-objects', () => { // predicate, so exactly one is active at any time. expect(disable?.target).toBe('/api/v1/auth/admin/oauth2/toggle-disabled'); expect(disable?.bodyExtra).toEqual({ disabled: true }); - expect((disable?.visible as any)?.source).toBe('(!record.disabled) && features.oidcProvider != false'); + expect((disable?.visible as any)?.source).toBe('(has(record.disabled) && record.disabled != true) && features.oidcProvider != false'); expect(enable?.target).toBe('/api/v1/auth/admin/oauth2/toggle-disabled'); expect(enable?.bodyExtra).toEqual({ disabled: false }); - expect((enable?.visible as any)?.source).toBe('(record.disabled) && features.oidcProvider != false'); + expect((enable?.visible as any)?.source).toBe('(has(record.disabled) && record.disabled == true) && features.oidcProvider != false'); // Generic CRUD must NOT expose mutating methods — all writes are // reserved for better-auth wrappers above so OAuth-specific @@ -456,9 +456,23 @@ describe('@objectstack/platform-objects', () => { // Every hand-written `visible: 'features.*'` gate was replaced by the // declarative `requiresFeature` sugar; these rows pin the LOWERED predicate // to the exact CEL string that was previously hand-written, so the migration -// is provably behavior-neutral. (transfer_ownership composes a residual row -// predicate with the gate — parenthesized but operand/order-identical to the -// old `record.role != 'owner' && features.organization != false`.) +// is provably behavior-neutral. +// +// The RESIDUAL row predicates are no longer byte-identical to the pre-#2874 +// hand-written strings, and that is #8990 rather than drift in this lowering: +// each `record.*` read is now opened by a `has()` guard, because the action +// binding is sparse (a list row carries only the view's `$select` projection) +// and an unguarded read aborts the whole predicate at key resolution — silently +// dropping the button. What this matrix still pins is what it was built to pin: +// the GATE term, its `&&` composition, and the operand order — the guards sit +// inside the parenthesized residual, and `features.*` is untouched. +// +// Two rows also changed OPERATOR, not just guard: `disable_oauth_application` +// was `!record.disabled` and `enable_oauth_application` was a bare +// `record.disabled`. Both are operators that need a bool and fault on the +// projected-null column better-auth leaves on every never-toggled application, +// so they became `!= true` / `== true` — the equality form CEL answers rather +// than faults on. See sys-oauth-application.object.ts for the measurement. describe('feature-gate lowering matrix (#2874)', () => { const ORG = 'features.organization != false'; const MULTI_ORG = 'features.multiOrgEnabled != false'; @@ -475,7 +489,7 @@ describe('feature-gate lowering matrix (#2874)', () => { ['SysMember', SysMember, 'add_member', ORG], ['SysMember', SysMember, 'update_member_role', ORG], ['SysMember', SysMember, 'remove_member', ORG], - ['SysMember', SysMember, 'transfer_ownership', `(record.role != 'owner') && ${ORG}`], + ['SysMember', SysMember, 'transfer_ownership', `(has(record.role) && record.role != 'owner') && ${ORG}`], ['SysInvitation', SysInvitation, 'invite_user', ORG], ['SysInvitation', SysInvitation, 'cancel_invitation', ORG], ['SysInvitation', SysInvitation, 'resend_invitation', ORG], @@ -492,16 +506,16 @@ describe('feature-gate lowering matrix (#2874)', () => { ['SysUser', SysUser, 'set_user_password', 'features.admin == true'], ['SysUser', SysUser, 'set_user_role', 'features.admin == true'], ['SysUser', SysUser, 'impersonate_user', 'features.admin == true'], - ['SysUser', SysUser, 'enable_two_factor', '(record.id == ctx.user.id && record.two_factor_enabled != true) && features.twoFactor == true'], - ['SysUser', SysUser, 'disable_two_factor', '(record.id == ctx.user.id && record.two_factor_enabled == true) && features.twoFactor == true'], - ['SysUser', SysUser, 'generate_backup_codes', '(record.id == ctx.user.id && record.two_factor_enabled == true) && features.twoFactor == true'], + ['SysUser', SysUser, 'enable_two_factor', '(has(record.id) && record.id == ctx.user.id && has(record.two_factor_enabled) && record.two_factor_enabled != true) && features.twoFactor == true'], + ['SysUser', SysUser, 'disable_two_factor', '(has(record.id) && record.id == ctx.user.id && has(record.two_factor_enabled) && record.two_factor_enabled == true) && features.twoFactor == true'], + ['SysUser', SysUser, 'generate_backup_codes', '(has(record.id) && record.id == ctx.user.id && has(record.two_factor_enabled) && record.two_factor_enabled == true) && features.twoFactor == true'], ['SysTwoFactor', SysTwoFactor, 'enable_two_factor', 'features.twoFactor == true'], ['SysTwoFactor', SysTwoFactor, 'disable_two_factor', 'features.twoFactor == true'], ['SysTwoFactor', SysTwoFactor, 'regenerate_backup_codes', 'features.twoFactor == true'], ['SysOauthApplication', SysOauthApplication, 'create_oauth_application', 'features.oidcProvider != false'], ['SysOauthApplication', SysOauthApplication, 'delete_oauth_application', 'features.oidcProvider != false'], - ['SysOauthApplication', SysOauthApplication, 'disable_oauth_application', '(!record.disabled) && features.oidcProvider != false'], - ['SysOauthApplication', SysOauthApplication, 'enable_oauth_application', '(record.disabled) && features.oidcProvider != false'], + ['SysOauthApplication', SysOauthApplication, 'disable_oauth_application', '(has(record.disabled) && record.disabled != true) && features.oidcProvider != false'], + ['SysOauthApplication', SysOauthApplication, 'enable_oauth_application', '(has(record.disabled) && record.disabled == true) && features.oidcProvider != false'], ['SysOauthApplication', SysOauthApplication, 'rotate_client_secret', 'features.oidcProvider != false'], ]; diff --git a/packages/plugins/plugin-approvals/src/action-predicate-sparse-face.test.ts b/packages/plugins/plugin-approvals/src/action-predicate-sparse-face.test.ts new file mode 100644 index 0000000000..cac4b033ee --- /dev/null +++ b/packages/plugins/plugin-approvals/src/action-predicate-sparse-face.test.ts @@ -0,0 +1,179 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * #8990 — `sys_approval_request`'s decision actions on the SPARSE action face. + * + * This object is the hardest case in the migration and the only one whose + * predicates traverse. Every approver / submitter lever gates on + * `record.viewer.*` — a per-viewer block the approvals service ATTACHES on + * `getRequest` / `listRequests`, not a declared column — and the action binding + * is whatever record the client already fetched (#4953 item 2: it stays sparse + * by decision). So `record.viewer` is absent on any read that did not go + * through those two paths, and CEL aborts the whole expression at key + * resolution rather than reading null. + * + * The object's own comment already said "where it is absent the predicate fails + * closed", and that was true — but it was true by FAULT, which is the shape + * this card exists to remove: a fault and a considered `false` are the same + * pixel to the user (no button), so the intended fail-closed and an authoring + * bug were indistinguishable. After the migration the fail-closed answer is a + * real `false` that the engine returns, and this file pins both halves. + * + * The guard form here is NOT the canonical two-term conjunction, and that is + * measured rather than preferred. `has(record.viewer) && record.viewer != null + * && record.viewer.can_act` still faults on `{viewer: {}}` and on + * `{viewer: {can_act: null}}`: the leaf is a second read and a second operator. + * Guarding the leaf instead subsumes the parent `!= null` half, because `has()` + * on a path whose parent is null answers `false` rather than faulting. Both + * claims are pinned below so the form cannot be "corrected" back into a + * faulting one by someone applying the canonical rule literally. + */ + +import { describe, expect, it } from 'vitest'; +import { celEngine } from '@objectstack/formula'; +import { SysApprovalRequest } from './sys-approval-request.object.js'; + +const USER = { id: 'u1', email: 'me@example.com' }; + +function evaluate(source: string, record: Record): boolean | string { + const r = celEngine.evaluate({ dialect: 'cel', source }, { record, user: USER }); + if (!r.ok) return `FAULT ${r.error.message.split('\n')[0].trim()}`; + return typeof r.value === 'boolean' ? r.value : `NON-BOOLEAN ${JSON.stringify(r.value)}`; +} + +/** + * `defineObject` normalizes a CEL shorthand string into a `{dialect, source}` + * envelope at parse time, so the stored value is not the string the file + * spells. Read through this rather than the raw key, or every assertion below + * runs against `undefined` and passes for the wrong reason. + */ +function sourceOf(raw: unknown): string | undefined { + if (typeof raw === 'string') return raw; + if (raw && typeof raw === 'object' && typeof (raw as { source?: unknown }).source === 'string') { + return (raw as { source: string }).source; + } + return undefined; +} + +const ACTIONS = (SysApprovalRequest.actions ?? []) as Array>; +const visibleOf = (name: string): string => { + const a = ACTIONS.find((x) => x.name === name); + if (!a) throw new Error(`no action ${name}`); + const source = sourceOf(a.visible); + if (!source) throw new Error(`action ${name} has no CEL source`); + return source; +}; + +/** The bindings a real read of this object can produce, sparse ones included. */ +const BINDINGS: Array<[string, Record]> = [ + ['a row with no viewer block at all', { status: 'pending' }], + ['a row whose viewer projected as null', { status: 'pending', viewer: null }], + ['a viewer block carrying none of the flags', { status: 'pending', viewer: {} }], + ['a viewer block whose flags are null', { status: 'pending', viewer: { can_act: null, can_override: null, is_submitter: null } }], + ['no status projected', { viewer: { can_act: true } }], + ['status projected as null', { status: null, viewer: { can_act: true } }], + ['a fully populated approver row', { status: 'pending', viewer: { can_act: true, can_override: false, is_submitter: false } }], + ['a fully populated submitter row', { status: 'pending', viewer: { can_act: false, can_override: false, is_submitter: true } }], +]; + +describe('#8990 — sys_approval_request decision actions never fault on a sparse binding', () => { + it('every action predicate reads record.* and is has()-guarded on each path', () => { + const predicates = ACTIONS.map((a) => sourceOf(a.visible)).filter((v): v is string => typeof v === 'string'); + expect(predicates.length).toBe(8); + const unguarded: string[] = []; + for (const source of predicates) { + for (const m of source.matchAll(/record((?:\.[a-z_][a-z0-9_]*)+)/gi)) { + const path = m[1].slice(1); + if (!source.includes(`has(record.${path})`)) unguarded.push(`record.${path} in: ${source}`); + } + } + expect(unguarded).toEqual([]); + }); + + it('every action predicate returns a boolean on every binding', () => { + const faults: string[] = []; + for (const action of ACTIONS) { + const source = sourceOf(action.visible); + if (!source) continue; + for (const [label, record] of BINDINGS) { + const verdict = evaluate(source, record); + if (typeof verdict !== 'boolean') faults.push(`${String(action.name)} on ${label}: ${verdict}`); + } + } + expect(faults).toEqual([]); + }); +}); + +describe('#8990 — the fail-closed intent survives as a real false, and the levers still open', () => { + it('an absent viewer block closes every viewer-gated lever', () => { + const row = { status: 'pending' }; + for (const name of ['approval_approve', 'approval_reject', 'approval_reassign', 'approval_send_back', 'approval_request_info', 'approval_remind', 'approval_recall', 'approval_resubmit']) { + expect([name, evaluate(visibleOf(name), row)]).toEqual([name, false]); + } + }); + + it('a current pending approver still gets approve / reject / reassign / send back / request info', () => { + const approver = { status: 'pending', viewer: { can_act: true, can_override: false, is_submitter: false } }; + for (const name of ['approval_approve', 'approval_reject', 'approval_reassign', 'approval_send_back', 'approval_request_info']) { + expect([name, evaluate(visibleOf(name), approver)]).toEqual([name, true]); + } + // Submitter levers stay closed for them. + expect(evaluate(visibleOf('approval_remind'), approver)).toBe(false); + }); + + it('an override-only admin still gets the three core decision levers and nothing else (#3424)', () => { + const admin = { status: 'pending', viewer: { can_act: false, can_override: true, is_submitter: false } }; + expect(evaluate(visibleOf('approval_approve'), admin)).toBe(true); + expect(evaluate(visibleOf('approval_reject'), admin)).toBe(true); + expect(evaluate(visibleOf('approval_reassign'), admin)).toBe(true); + // `can_override` was never OR'd into the secondary levers, and still is not. + expect(evaluate(visibleOf('approval_send_back'), admin)).toBe(false); + expect(evaluate(visibleOf('approval_request_info'), admin)).toBe(false); + }); + + it('the submitter still gets remind / recall on pending and resubmit / recall on returned', () => { + const submitter = { can_act: false, can_override: false, is_submitter: true }; + expect(evaluate(visibleOf('approval_remind'), { status: 'pending', viewer: submitter })).toBe(true); + expect(evaluate(visibleOf('approval_recall'), { status: 'pending', viewer: submitter })).toBe(true); + expect(evaluate(visibleOf('approval_recall'), { status: 'returned', viewer: submitter })).toBe(true); + expect(evaluate(visibleOf('approval_resubmit'), { status: 'returned', viewer: submitter })).toBe(true); + // Terminal states close all three. + expect(evaluate(visibleOf('approval_remind'), { status: 'approved', viewer: submitter })).toBe(false); + expect(evaluate(visibleOf('approval_recall'), { status: 'approved', viewer: submitter })).toBe(false); + expect(evaluate(visibleOf('approval_resubmit'), { status: 'pending', viewer: submitter })).toBe(false); + }); +}); + +describe('#8990 — why the guard form is leaf-first (measured, not preferred)', () => { + /** + * These four pins are the reason `record.viewer != null` is absent from the + * migrated predicates. They record the measurement so the next author reading + * the canonical `has(record.x) && record.x != null` rule and "completing" it + * here can see that the completion is what breaks it. + */ + it('the canonical two-term conjunction is NOT sufficient over a nested read', () => { + const canonical = 'has(record.viewer) && record.viewer != null && record.viewer.can_act'; + expect(evaluate(canonical, { viewer: {} })).toBe('FAULT No such key: can_act'); + expect(evaluate(canonical, { viewer: { can_act: null } })) + .toBe("FAULT Logical operator requires bool operands, got 'null'"); + }); + + it('guarding the leaf subsumes the parent != null half', () => { + const leafFirst = 'has(record.viewer) && has(record.viewer.can_act) && record.viewer.can_act == true'; + expect(evaluate(leafFirst, {})).toBe(false); + expect(evaluate(leafFirst, { viewer: null })).toBe(false); + expect(evaluate(leafFirst, { viewer: {} })).toBe(false); + expect(evaluate(leafFirst, { viewer: { can_act: null } })).toBe(false); + expect(evaluate(leafFirst, { viewer: { can_act: true } })).toBe(true); + }); + + it('the outer has() is load-bearing — the leaf guard alone still faults on an absent root', () => { + expect(evaluate('has(record.viewer.can_act) && record.viewer.can_act == true', {})) + .toBe('FAULT No such key: viewer'); + }); + + it('the `== true` comparison is load-bearing — a bare truthy read of a null leaf faults', () => { + expect(evaluate('has(record.viewer) && has(record.viewer.can_act) && record.viewer.can_act', { viewer: { can_act: null } })) + .toBe("FAULT Logical operator requires bool operands, got 'null'"); + }); +}); diff --git a/packages/plugins/plugin-approvals/src/sys-approval-request.object.ts b/packages/plugins/plugin-approvals/src/sys-approval-request.object.ts index dc62d8a861..98c1723220 100644 --- a/packages/plugins/plugin-approvals/src/sys-approval-request.object.ts +++ b/packages/plugins/plugin-approvals/src/sys-approval-request.object.ts @@ -252,6 +252,28 @@ export const SysApprovalRequest = ObjectSchema.create({ // approving, rejecting, or reassigning it to a real approver. `viewer` is // attached by getRequest/listRequests; where it is absent the predicate fails // closed. + // + // Every predicate below is guarded for the SPARSE action face (#8990). This + // binding is a list row or a record read carrying only what the caller + // projected, and CEL aborts the whole expression at key resolution — so the + // unguarded `record.viewer.can_act` faulted (`No such key: viewer`) on any + // row without the block, and the button silently vanished, indistinguishable + // from "the gate said no". `materializeDeclaredFields`'s doc comment in + // `@objectstack/objectql` is the canonical statement of the guard rule; this + // file follows it and does not restate it. + // + // `viewer` is a NESTED block, which needs one measurement the canonical rule + // does not spell out. Measured against the `@objectstack/formula` CEL engine: + // `has(record.viewer) && record.viewer != null && record.viewer.can_act` + // still FAULTS on `{viewer: {}}` (`No such key: can_act`) and on + // `{viewer: {can_act: null}}` (`Logical operator requires bool operands`). + // Guarding the LEAF instead — `has(record.viewer) && + // has(record.viewer.can_act) && record.viewer.can_act == true` — is total + // over every binding AND subsumes the parent `!= null` half, because `has()` + // on a path whose parent is null answers `false` rather than faulting. So the + // leaf `has()` plus the `== true` comparison is the MINIMAL safe form here, + // not a longer one: `== true` is load-bearing (a bare truthy read of a null + // leaf faults the logical operator), the parent `!= null` is not. actions: [ { name: 'approval_approve', @@ -271,7 +293,9 @@ export const SysApprovalRequest = ObjectSchema.create({ // string[]`; the decision route persists them on `sys_approval_action`. { name: 'attachments', label: 'Attachments', type: 'file', multiple: true, required: false }, ], - visible: 'record.viewer.can_act || record.viewer.can_override', + visible: + 'has(record.viewer) && has(record.viewer.can_act) && record.viewer.can_act == true' + + ' || has(record.viewer) && has(record.viewer.can_override) && record.viewer.can_override == true', locations: ['record_section', 'list_item'], successMessage: 'Approved.', refreshAfter: true, @@ -299,7 +323,9 @@ export const SysApprovalRequest = ObjectSchema.create({ { name: 'comment', label: 'Comment', type: 'textarea', required: false }, { name: 'attachments', label: 'Attachments', type: 'file', multiple: true, required: false }, ], - visible: 'record.viewer.can_act || record.viewer.can_override', + visible: + 'has(record.viewer) && has(record.viewer.can_act) && record.viewer.can_act == true' + + ' || has(record.viewer) && has(record.viewer.can_override) && record.viewer.can_override == true', locations: ['record_section', 'list_item'], successMessage: 'Rejected.', refreshAfter: true, @@ -320,7 +346,9 @@ export const SysApprovalRequest = ObjectSchema.create({ { field: 'submitter_id', name: 'to', label: 'New approver', required: true, helpText: 'User to hand this step to' }, { name: 'comment', label: 'Comment', type: 'textarea', required: false }, ], - visible: 'record.viewer.can_act || record.viewer.can_override', + visible: + 'has(record.viewer) && has(record.viewer.can_act) && record.viewer.can_act == true' + + ' || has(record.viewer) && has(record.viewer.can_override) && record.viewer.can_override == true', locations: ['record_section'], successMessage: 'Reassigned.', refreshAfter: true, @@ -340,7 +368,7 @@ export const SysApprovalRequest = ObjectSchema.create({ params: [ { name: 'comment', label: 'Reason', type: 'textarea', required: false }, ], - visible: 'record.viewer.can_act', + visible: 'has(record.viewer) && has(record.viewer.can_act) && record.viewer.can_act == true', locations: ['record_section'], successMessage: 'Sent back for revision.', refreshAfter: true, @@ -355,7 +383,7 @@ export const SysApprovalRequest = ObjectSchema.create({ params: [ { name: 'comment', label: 'What do you need?', type: 'textarea', required: true }, ], - visible: 'record.viewer.can_act', + visible: 'has(record.viewer) && has(record.viewer.can_act) && record.viewer.can_act == true', locations: ['record_section'], successMessage: 'Information requested.', refreshAfter: true, @@ -377,7 +405,7 @@ export const SysApprovalRequest = ObjectSchema.create({ params: [ { name: 'comment', label: 'Note', type: 'textarea', required: false }, ], - visible: 'record.status == "pending" && record.viewer.is_submitter', + visible: 'has(record.status) && record.status == "pending" && has(record.viewer) && has(record.viewer.is_submitter) && record.viewer.is_submitter == true', locations: ['record_section'], successMessage: 'Reminder sent.', refreshAfter: true, @@ -397,7 +425,9 @@ export const SysApprovalRequest = ObjectSchema.create({ ], // Recall applies while the request is live for the submitter — pending // (withdraw) or returned (abandon the revision instead of resubmitting). - visible: '(record.status == "pending" || record.status == "returned") && record.viewer.is_submitter', + visible: + 'has(record.status) && (record.status == "pending" || record.status == "returned")' + + ' && has(record.viewer) && has(record.viewer.is_submitter) && record.viewer.is_submitter == true', locations: ['record_section'], successMessage: 'Recalled.', refreshAfter: true, @@ -412,7 +442,7 @@ export const SysApprovalRequest = ObjectSchema.create({ params: [ { name: 'comment', label: 'What changed?', type: 'textarea', required: false }, ], - visible: 'record.status == "returned" && record.viewer.is_submitter', + visible: 'has(record.status) && record.status == "returned" && has(record.viewer) && has(record.viewer.is_submitter) && record.viewer.is_submitter == true', locations: ['record_section'], successMessage: 'Resubmitted.', refreshAfter: true, diff --git a/pnpm-lock.yaml b/pnpm-lock.yaml index 7862ce4468..60c9b3b8ce 100644 --- a/pnpm-lock.yaml +++ b/pnpm-lock.yaml @@ -165,6 +165,9 @@ importers: '@objectstack/driver-sql': specifier: workspace:* version: link:../../packages/drivers/driver-sql + '@objectstack/formula': + specifier: workspace:* + version: link:../../packages/formula '@objectstack/objectql': specifier: workspace:* version: link:../../packages/objectql