P0 security: enforce-or-remove unenforced security props (ADR-0049)

.changeset/formula-condition-unknown-function.md

@@ -0,0 +1,7 @@
+---
+"@objectstack/formula": patch
+---
+
+fix(formula): catch unknown functions in CEL conditions at build (#1877)
+
+`compile()` discarded cel-js's type-check verdict because `check()` returns a `TypeCheckResult` object (`{ valid, error }`), not an array — so the `Array.isArray(checkErrors)` guard never matched. A condition calling an unknown function (`PRIOR(status)`, a typo'd `isBlnk(...)`) type-checks as `found no matching overload`, but that result never surfaced, so `objectstack compile`, `registerFlow`, and the `validate_expression` tool all accepted the predicate, which then silently no-op'd the flow at runtime. Now reads the documented `{ valid, error }` shape, closing the gap for flow conditions, validation rules, and field formulas at once.

docs/adr/0049-no-unenforced-security-properties.md

@@ -0,0 +1,116 @@
+# ADR-0049: Spec must not declare security properties the runtime does not enforce (enforce-or-remove gate)
+
+**Status**: Proposed (2026-06-15)
+**Deciders**: ObjectStack Protocol Architects
+**Builds on**: [ADR-0005](./0005-metadata-customization-overlay.md) (artifact vs runtime overlay), [ADR-0010](./0010-metadata-protection-model.md) (package provenance), [ADR-0027](./0027-metadata-authoring-lifecycle.md) (authoring lifecycle)
+**Consumers**: `@objectstack/spec` (security/identity schemas), `@objectstack/plugin-security` (`PermissionEvaluator`, `SecurityPlugin`), spec authors, the metadata-property liveness audit follow-ups (#1878 P0 cluster).
+**Surfaced by**: the metadata property liveness audit (#1878, `docs/audits/`) — which found that **roughly half of all spec properties are dead**, and that a cluster of *security* properties is **parsed but unenforced**.
+
+---
+
+## TL;DR
+
+A protocol-level audit cross-referenced every spec property against its actual
+runtime consumers. The most serious finding is a cluster of **security
+properties that imply an access-control boundary but enforce nothing**:
+`PolicySchema` (100% dead — password/session/MFA/IP/audit), permission
+lifecycle bits (`allowTransfer`/`allowRestore`/`allowPurge`), `agent`
+access-control, flow `runAs`, object `apiEnabled`/`apiMethods`, action
+`disabled`, role `parent`, and `SharingRuleSchema`.
+
+A security property that parses but does nothing is **worse than absent**: it
+produces a *false sense of compliance*. An admin who sets `allowPurge: false`
+or authors a strict password policy believes a boundary exists where none does.
+
+**Decision.** A spec property that names a security/access-control boundary
+**must be in exactly one of three states**:
+
+1. **Enforced** — a runtime consumer reads it and changes a decision (`file:line`).
+2. **`experimental`** — explicitly marked and documented as *not yet enforced*,
+   so authoring it is a known no-op (roadmapped, not a promise).
+3. **Absent** — removed from the spec.
+
+Shipping a security property in a fourth state — *parsed, unmarked, unenforced*
+— is prohibited. This is the **enforce-or-remove gate**.
+
+A second, roadmap-independent defect compounds the first: `PermissionEvaluator`
+**fails open** for operations it doesn't recognise
+(`permission-evaluator.ts:35`, `if (!permKey) return true`). Any future
+destructive operation added without registering it in `OPERATION_TO_PERMISSION`
+is silently ungated. The evaluator must **fail closed** for the destructive
+operation class.
+
+---
+
+## Context
+
+- Evidence: `docs/audits/2026-06-security-identity-property-liveness.md` and the
+  cross-type synthesis in `docs/audits/README.md` (cluster #1).
+- The CRUD path *is* enforced: `SecurityPlugin` (`security-plugin.ts:326`)
+  resolves permission sets and calls `PermissionEvaluator.checkObjectPermission`,
+  which maps the ObjectQL operation to an `ObjectPermission` key via
+  `OPERATION_TO_PERMISSION` (`permission-evaluator.ts:8-16`).
+- That map covers only `find/findOne/count/aggregate/insert/update/delete`. The
+  three destructive permission bits in the spec
+  (`permission.zod.ts:28-30` — `allowTransfer`/`allowRestore`/`allowPurge`)
+  have **no operation pointing at them**, and the operations they describe
+  (`transfer`/`restore`/`purge`) **do not yet exist** as ObjectQL operations.
+  So the bits are dangling, and the `if (!permKey) return true` default means
+  that *if* such an operation were added without a map entry, it would be
+  allowed for everyone.
+
+## Decision — staged by the platform's current (pre-MVP) phase
+
+The audit's instinct was "enforce every unenforced security prop." At the
+current milestone that is the **wrong default**: building enforcement for
+features that do not exist yet is speculative. The real, shippable liability is
+the *false promise*, not the missing feature. So we split the P0 cluster by
+**whether the feature already exists**:
+
+| Situation | Items | Phase action |
+|---|---|---|
+| **Feature does not exist; spec bit is a dangling promise** | `PolicySchema` (#1882), permission lifecycle bits (#1883), `SharingRuleSchema` spec form (#1887), flow `runAs` (#1888) | **Remove or mark `experimental`** now. Re-introduce *with* the feature + enforcement at M2/production. |
+| **Feature is live; the gate is missing or bypassed** | agent access-control (#1884), object `apiEnabled`/`apiMethods` (#1889), action `disabled` CEL (#1885) | **Enforce** now — these are real, exploitable gaps and the fix is a localized check at the route/renderer. |
+
+Plus one **no-regret correctness fix**, independent of roadmap:
+
+- `PermissionEvaluator` fails **closed** for the destructive operation class:
+  introduce an explicit set of sensitive/destructive operations; an unrecognised
+  operation in that class is **denied**, not allowed. (Non-destructive unknown
+  operations may retain default-allow to avoid breaking custom read-side ops.)
+
+### `experimental` convention for the "mark, don't remove" path
+
+For a roadmapped property we keep but cannot yet enforce, annotate it so the
+no-op is explicit to authors and tooling, rather than silently parsing:
+
+- prefix the Zod `.describe()` with **`[EXPERIMENTAL — not enforced]`**, and
+- where the surrounding schema already carries a status/stability enum (e.g.
+  `model-registry.zod.ts`, `plugin-capability.zod.ts`), prefer that enum.
+
+Removal is preferred over marking when there is no committed roadmap for the
+property — a smaller spec surface is the stronger default pre-MVP.
+
+## Consequences
+
+- **Positive.** No spec property silently misleads an admin about a security
+  boundary. The evaluator can no longer be made to fail open by adding a
+  destructive operation. The P0 cluster splits into a cheap no-regret PR
+  (evaluator fail-closed + mark/remove dangling bits) and a small enforcement
+  PR (live-but-ungated features), deferring the heavy work (policy registration,
+  sharing-rule engine reconciliation) to when the feature lands.
+- **Negative / cost.** Removing or `experimental`-tagging spec bits is a
+  spec-surface change; seeds/fixtures that author the removed bits must be
+  updated (low risk pre-MVP). The fail-closed change requires enumerating the
+  destructive operation class so legitimate custom operations are not denied.
+- **Follow-up.** This ADR is the umbrella decision for the #1878 P0 cluster;
+  each sub-issue records its enforce/experimental/remove disposition against the
+  table above.
+
+## Non-goals
+
+- Building the transfer/restore/purge, policy-enforcement, or sharing-rule
+  engines themselves — those are feature work for M2/production, tracked by
+  their respective issues.
+- The P1 (ADR-0021 analytics migration) and P2 (spec hygiene) clusters of
+  #1878 — non-security, governed separately.

packages/formula/src/cel-engine.test.ts

@@ -70,6 +70,22 @@ describe('celEngine', () => {
     expect(r.ok).toBe(true);
   });
 
+  // #1877 — cel-js `check()` returns a `{ valid, error }` object, not an array.
+  // compile() must read that shape so an UNKNOWN function (here `PRIOR`) is
+  // reported as a type fault at build time instead of slipping through.
+  it('compile() rejects an unknown function as a type error (#1877)', () => {
+    const r = celEngine.compile('PRIOR(status) != "promoted"');
+    expect(r.ok).toBe(false);
+    if (!r.ok) {
+      expect(r.error.kind).toBe('type');
+      expect(r.error.message).toMatch(/overload|PRIOR/);
+    }
+  });
+
+  it('compile() still accepts a registered stdlib function (#1877)', () => {
+    expect(celEngine.compile('!isBlank(record.target_channels)').ok).toBe(true);
+  });
+
   it('handles timestamp + duration arithmetic', () => {
     const pinned = new Date('2026-01-01T00:00:00Z');
     const r = celEngine.evaluate(cel('now() + duration("720h")'), { now: pinned });

packages/formula/src/cel-engine.ts

@@ -146,12 +146,19 @@ export const celEngine: DialectEngine = {
       // type-checking; the function is never actually called.
       const env = buildEnv(() => new Date(0));
       const compiled = env.parse(source);
-      // Surface check errors eagerly.
-      const checkErrors = compiled.check?.();
-      if (checkErrors && Array.isArray(checkErrors) && checkErrors.length > 0) {
+      // Surface check errors eagerly. cel-js's `check()` returns a
+      // `TypeCheckResult` object (`{ valid, type?, error? }`) — NOT an array —
+      // so the type fault (including `found no matching overload for 'PRIOR(dyn)'`
+      // when a condition calls an UNKNOWN function) only surfaces when we read
+      // `valid === false`. The previous `Array.isArray(...)` guard never matched
+      // an object, so unknown-function predicates type-checked clean and were
+      // silently accepted by `objectstack build` / `registerFlow`, then no-op'd
+      // the flow at runtime (#1877). Reading the documented shape closes that.
+      const checkResult = compiled.check?.();
+      if (checkResult && checkResult.valid === false) {
         return {
           ok: false,
-          error: { kind: 'type', message: checkErrors.join('; ') },
+          error: { kind: 'type', message: checkResult.error?.message ?? 'expression failed type checking' },
         };
       }
       return { ok: true, value: compiled.ast };

packages/formula/src/validate.test.ts

@@ -26,6 +26,27 @@ describe('validateExpression (ADR-0032)', () => {
       expect(validateExpression('predicate', '').ok).toBe(true);
       expect(validateExpression('predicate', null).ok).toBe(true);
     });
+
+    // #1877 — a predicate calling an UNKNOWN function (e.g. `PRIOR()`, a typo'd
+    // `isBlnk()`) must be rejected at build/registration, not silently accepted
+    // and then no-op the flow at runtime. cel-js's type checker reports these as
+    // `found no matching overload`; the engine surfaces them as an invalid CEL
+    // predicate.
+    it('rejects an unknown function call (#1877)', () => {
+      const r = validateExpression('predicate', 'PRIOR(status) != "promoted"');
+      expect(r.ok).toBe(false);
+      expect(r.errors[0].message).toMatch(/invalid CEL predicate/i);
+      expect(r.errors[0].message).toMatch(/overload|PRIOR/);
+    });
+
+    it('rejects an unknown function even when guarded by a short-circuit (#1877)', () => {
+      const r = validateExpression('predicate', 'status == "promoted" && PRIOR(status) != "promoted"');
+      expect(r.ok).toBe(false);
+    });
+
+    it('still accepts a registered stdlib function (isBlank)', () => {
+      expect(validateExpression('predicate', '!isBlank(record.target_channels)').ok).toBe(true);
+    });
   });
 
   describe('templates', () => {

packages/plugins/plugin-security/src/permission-evaluator.ts

@@ -15,6 +15,17 @@ const OPERATION_TO_PERMISSION: Record<string, keyof ObjectPermission> = {
   delete: 'allowDelete',
 };
 
+/**
+ * Destructive operation class — operations that must FAIL CLOSED when they are
+ * not mapped to a concrete permission key. See ADR-0049: an unrecognised
+ * destructive operation (e.g. a future `transfer`/`restore`/`purge` added
+ * without a matching `OPERATION_TO_PERMISSION` entry, gated by the spec's
+ * `allowTransfer`/`allowRestore`/`allowPurge` bits) must be DENIED rather than
+ * silently allowed by the default-allow fallthrough. Non-destructive unknown
+ * operations retain default-allow so custom read-side operations are not broken.
+ */
+const DESTRUCTIVE_OPERATIONS = new Set<string>(['transfer', 'restore', 'purge']);
+
 /**
  * PermissionEvaluator
  * 
@@ -32,7 +43,12 @@ export class PermissionEvaluator {
     permissionSets: PermissionSet[]
   ): boolean {
     const permKey = OPERATION_TO_PERMISSION[operation];
-    if (!permKey) return true; // Unknown operations are allowed by default
+    if (!permKey) {
+      // Fail CLOSED for the destructive operation class (ADR-0049): an
+      // unrecognised destructive op must be denied, never silently allowed.
+      // Other unknown operations are allowed by default.
+      return !DESTRUCTIVE_OPERATIONS.has(operation);
+    }
 
     for (const ps of permissionSets) {
       // Honour the `'*'` wildcard sentinel — admin permission sets typically

packages/plugins/plugin-security/src/security-plugin.test.ts

@@ -610,11 +610,24 @@ describe('PermissionEvaluator', () => {
     expect(evaluator.checkObjectPermission('insert', 'contact', [ps])).toBe(false);
   });
 
-  it('should allow unknown operations by default', () => {
+  it('should allow unknown (non-destructive) operations by default', () => {
     const evaluator = new PermissionEvaluator();
     expect(evaluator.checkObjectPermission('unknownOp', 'contact', [])).toBe(true);
   });
 
+  it('should fail CLOSED for unmapped destructive operations (ADR-0049)', () => {
+    const evaluator = new PermissionEvaluator();
+    // transfer/restore/purge are not in OPERATION_TO_PERMISSION; they must be
+    // denied rather than falling through to default-allow — even for an
+    // otherwise fully-permissioned set.
+    const ps = makePermSet('admin', {
+      contact: { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true, modifyAllRecords: true },
+    });
+    expect(evaluator.checkObjectPermission('transfer', 'contact', [ps])).toBe(false);
+    expect(evaluator.checkObjectPermission('restore', 'contact', [ps])).toBe(false);
+    expect(evaluator.checkObjectPermission('purge', 'contact', [ps])).toBe(false);
+  });
+
   it('should allow via viewAllRecords', () => {
     const evaluator = new PermissionEvaluator();
     const ps = makePermSet('viewer', { task: { allowRead: false, allowCreate: false, allowEdit: false, allowDelete: false, viewAllRecords: true } });

packages/runtime/src/api-exposure.test.ts

@@ -0,0 +1,52 @@
+// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
+
+import { describe, it, expect } from 'vitest';
+import { checkApiExposure } from './api-exposure.js';
+
+describe('checkApiExposure (#1889)', () => {
+  it('falls open when the definition is unresolvable', () => {
+    expect(checkApiExposure(undefined, 'get').allowed).toBe(true);
+    expect(checkApiExposure(null, 'create').allowed).toBe(true);
+  });
+
+  it('allows by default (apiEnabled defaults true, no whitelist)', () => {
+    expect(checkApiExposure({}, 'query').allowed).toBe(true);
+    expect(checkApiExposure({ apiEnabled: true }, 'delete').allowed).toBe(true);
+  });
+
+  it('hides the object (404) when apiEnabled is false', () => {
+    const d = checkApiExposure({ apiEnabled: false }, 'get');
+    expect(d.allowed).toBe(false);
+    expect(d.status).toBe(404);
+  });
+
+  describe('apiMethods whitelist', () => {
+    it('allows a whitelisted operation', () => {
+      // query maps to ApiMethod 'list'
+      expect(checkApiExposure({ apiMethods: ['list', 'get'] }, 'query').allowed).toBe(true);
+      expect(checkApiExposure({ apiMethods: ['list', 'get'] }, 'get').allowed).toBe(true);
+    });
+
+    it('blocks a non-whitelisted operation (405)', () => {
+      const d = checkApiExposure({ apiMethods: ['list', 'get'] }, 'create');
+      expect(d.allowed).toBe(false);
+      expect(d.status).toBe(405);
+      expect(d.reason).toContain('create');
+    });
+
+    it('maps delete/update/create/find correctly', () => {
+      const ro = { apiMethods: ['list', 'get'] };
+      expect(checkApiExposure(ro, 'delete').allowed).toBe(false);
+      expect(checkApiExposure(ro, 'update').allowed).toBe(false);
+      expect(checkApiExposure(ro, 'find').allowed).toBe(true); // find → list
+    });
+
+    it('an empty whitelist is treated as no restriction', () => {
+      expect(checkApiExposure({ apiMethods: [] }, 'create').allowed).toBe(true);
+    });
+
+    it('does not gate actions with no ApiMethod mapping', () => {
+      expect(checkApiExposure({ apiMethods: ['list'] }, 'somethingCustom').allowed).toBe(true);
+    });
+  });
+});

packages/runtime/src/api-exposure.ts

@@ -0,0 +1,72 @@
+// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
+
+/**
+ * Object-level API exposure gate (ADR-0049, #1889).
+ *
+ * Objects declare `apiEnabled` (default true) and an optional `apiMethods`
+ * whitelist, but the HTTP/MCP data dispatch previously ignored both — an object
+ * could not actually be hidden from the API, nor could its allowed operations
+ * be restricted. This module decides, for a given data action, whether the
+ * object's declared exposure permits it.
+ *
+ * Both fields are *additive restrictions* over a default-allow surface
+ * (`apiEnabled` defaults true; absent `apiMethods` means "all operations").
+ * Therefore an unresolvable object definition fails OPEN here — that matches
+ * the schema defaults and avoids breaking traffic when metadata is briefly
+ * unavailable. The gate is a no-op for system/internal contexts (callers pass
+ * `isSystem` and skip this check entirely).
+ */
+
+/** The exposure-relevant slice of an object definition. */
+export interface ObjectApiDef {
+  apiEnabled?: boolean;
+  apiMethods?: string[] | null;
+}
+
+export interface ApiExposureDecision {
+  allowed: boolean;
+  /** HTTP status to return when denied (404 hides, 405 = method not allowed). */
+  status?: number;
+  reason?: string;
+}
+
+/**
+ * Map an internal `callData` action onto the spec `ApiMethod` vocabulary
+ * (`object.zod.ts` → `ApiMethod`). Actions with no mapping are not gated by
+ * `apiMethods` (they still respect `apiEnabled`).
+ */
+const ACTION_TO_API_METHOD: Record<string, string> = {
+  create: 'create',
+  get: 'get',
+  update: 'update',
+  delete: 'delete',
+  query: 'list',
+  find: 'list',
+  batch: 'bulk',
+};
+
+export function checkApiExposure(def: ObjectApiDef | null | undefined, action: string): ApiExposureDecision {
+  // Unresolvable definition → fall open to the schema defaults.
+  if (!def) return { allowed: true };
+
+  // `apiEnabled: false` hides the object from the API entirely → 404.
+  if (def.apiEnabled === false) {
+    return { allowed: false, status: 404, reason: 'object is not exposed via the API' };
+  }
+
+  // `apiMethods` whitelist (when present and non-empty) restricts operations.
+  const whitelist = def.apiMethods;
+  if (Array.isArray(whitelist) && whitelist.length > 0) {
+    const method = ACTION_TO_API_METHOD[action];
+    // Only gate actions that map to a known ApiMethod; unmapped actions pass.
+    if (method && !whitelist.includes(method)) {
+      return {
+        allowed: false,
+        status: 405,
+        reason: `API operation '${method}' is not allowed for this object`,
+      };
+    }
+  }
+
+  return { allowed: true };
+}