diff --git a/.changeset/formula-condition-unknown-function.md b/.changeset/formula-condition-unknown-function.md new file mode 100644 index 0000000000..dfa3b34af4 --- /dev/null +++ b/.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. diff --git a/docs/adr/0049-no-unenforced-security-properties.md b/docs/adr/0049-no-unenforced-security-properties.md new file mode 100644 index 0000000000..80e9159f9b --- /dev/null +++ b/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. diff --git a/packages/formula/src/cel-engine.test.ts b/packages/formula/src/cel-engine.test.ts index 6a3776aeda..d1f1b52479 100644 --- a/packages/formula/src/cel-engine.test.ts +++ b/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 }); diff --git a/packages/formula/src/cel-engine.ts b/packages/formula/src/cel-engine.ts index e52981b1d3..4e852aa5e8 100644 --- a/packages/formula/src/cel-engine.ts +++ b/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 }; diff --git a/packages/formula/src/validate.test.ts b/packages/formula/src/validate.test.ts index 36473dc5df..1f8f1567b3 100644 --- a/packages/formula/src/validate.test.ts +++ b/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', () => { diff --git a/packages/plugins/plugin-security/src/permission-evaluator.ts b/packages/plugins/plugin-security/src/permission-evaluator.ts index c7a3985376..68ab764290 100644 --- a/packages/plugins/plugin-security/src/permission-evaluator.ts +++ b/packages/plugins/plugin-security/src/permission-evaluator.ts @@ -15,6 +15,17 @@ const OPERATION_TO_PERMISSION: Record = { 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(['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 diff --git a/packages/plugins/plugin-security/src/security-plugin.test.ts b/packages/plugins/plugin-security/src/security-plugin.test.ts index e153755744..78cd67ad8f 100644 --- a/packages/plugins/plugin-security/src/security-plugin.test.ts +++ b/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 } }); diff --git a/packages/runtime/src/api-exposure.test.ts b/packages/runtime/src/api-exposure.test.ts new file mode 100644 index 0000000000..e1d7cc73ec --- /dev/null +++ b/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); + }); + }); +}); diff --git a/packages/runtime/src/api-exposure.ts b/packages/runtime/src/api-exposure.ts new file mode 100644 index 0000000000..6bc79751c9 --- /dev/null +++ b/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 = { + 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 }; +} diff --git a/packages/runtime/src/http-dispatcher.ts b/packages/runtime/src/http-dispatcher.ts index f93054b11e..f76b25605c 100644 --- a/packages/runtime/src/http-dispatcher.ts +++ b/packages/runtime/src/http-dispatcher.ts @@ -5,6 +5,7 @@ import { CoreServiceName } from '@objectstack/spec/system'; import { pluralToSingular, PLURAL_TO_SINGULAR } from '@objectstack/spec/shared'; import type { ExecutionContext } from '@objectstack/spec/kernel'; import { setPackageDisabled } from './package-state-store.js'; +import { checkApiExposure } from './api-exposure.js'; /** Minimal local interface — full EnvironmentScopeManager was removed in Phase R. */ interface EnvironmentScopeManager { @@ -258,6 +259,24 @@ export class HttpDispatcher { scopeId?: string, executionContext?: ExecutionContext, ): Promise { + // ── Object-level API exposure gate (ADR-0049, #1889) ───── + // Honour the object's `apiEnabled` / `apiMethods` declarations for + // external traffic. System/internal contexts bypass — these flags + // govern API *exposure*, not internal engine self-writes. + if (!executionContext?.isSystem && params?.object) { + let def: any; + try { + const meta = await this.resolveService('metadata', scopeId); + def = await (meta as any)?.getObject?.(params.object); + } catch { + def = undefined; // fall open to schema defaults (apiEnabled=true) + } + const gate = checkApiExposure(def, action); + if (!gate.allowed) { + throw { statusCode: gate.status ?? 403, message: gate.reason ?? 'API access denied' }; + } + } + const protocol = await this.resolveService('protocol', scopeId); const qlService = dataDriver ?? await this.getObjectQLService(scopeId); const ql = qlService ?? await this.resolveService('objectql', scopeId); diff --git a/packages/services/service-ai/src/routes/agent-access.test.ts b/packages/services/service-ai/src/routes/agent-access.test.ts new file mode 100644 index 0000000000..46b60118ea --- /dev/null +++ b/packages/services/service-ai/src/routes/agent-access.test.ts @@ -0,0 +1,65 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +import { describe, it, expect } from 'vitest'; +import { evaluateAgentAccess } from './agent-access.js'; + +describe('evaluateAgentAccess (#1884)', () => { + const user = (over: Partial<{ userId: string; roles: string[]; permissions: string[] }> = {}) => ({ + userId: 'u1', + roles: [] as string[], + permissions: [] as string[], + ...over, + }); + + it('allows when the agent declares no access/permissions', () => { + expect(evaluateAgentAccess({}, user()).allowed).toBe(true); + expect(evaluateAgentAccess({ visibility: 'private' }, user()).allowed).toBe(true); + }); + + it('fails closed when the user is missing', () => { + const d = evaluateAgentAccess({ access: ['u1'] }, undefined); + expect(d.allowed).toBe(false); + }); + + describe('required permissions (must hold ALL)', () => { + it('denies when a required permission is missing', () => { + const d = evaluateAgentAccess({ permissions: ['hr:read', 'hr:write'] }, user({ permissions: ['hr:read'] })); + expect(d.allowed).toBe(false); + expect(d.reason).toContain('hr:write'); + }); + + it('allows when all are held via permissions', () => { + expect(evaluateAgentAccess({ permissions: ['hr:read'] }, user({ permissions: ['hr:read'] })).allowed).toBe(true); + }); + + it('satisfies a required entry via roles too (permissions OR roles)', () => { + expect(evaluateAgentAccess({ permissions: ['manager'] }, user({ roles: ['manager'] })).allowed).toBe(true); + }); + }); + + describe('access allow-list (must match one)', () => { + it('allows a listed user id', () => { + expect(evaluateAgentAccess({ access: ['u1', 'u2'] }, user()).allowed).toBe(true); + }); + + it('allows a listed role', () => { + expect(evaluateAgentAccess({ access: ['support'] }, user({ roles: ['support'] })).allowed).toBe(true); + }); + + it('denies a caller not on the list', () => { + const d = evaluateAgentAccess({ access: ['u2', 'admins'] }, user({ userId: 'u1', roles: ['sales'] })); + expect(d.allowed).toBe(false); + expect(d.reason).toMatch(/access list/); + }); + }); + + it('enforces permissions AND allow-list together', () => { + const agent = { permissions: ['ai:beta'], access: ['vip'] }; + // has permission but not on allow-list + expect(evaluateAgentAccess(agent, user({ permissions: ['ai:beta'], roles: [] })).allowed).toBe(false); + // on allow-list but missing permission + expect(evaluateAgentAccess(agent, user({ roles: ['vip'], permissions: [] })).allowed).toBe(false); + // both satisfied + expect(evaluateAgentAccess(agent, user({ roles: ['vip'], permissions: ['ai:beta'] })).allowed).toBe(true); + }); +}); diff --git a/packages/services/service-ai/src/routes/agent-access.ts b/packages/services/service-ai/src/routes/agent-access.ts new file mode 100644 index 0000000000..6b40681b9d --- /dev/null +++ b/packages/services/service-ai/src/routes/agent-access.ts @@ -0,0 +1,87 @@ +// Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license. + +import type { RouteUserContext } from './ai-routes.js'; + +/** + * The subset of an agent definition relevant to "who can chat with it". + * Kept structural (not the full Agent type) so the check is decoupled from the + * runtime's loaded-agent shape and trivially unit-testable. + */ +export interface AgentAccessSpec { + /** Allow-list of user IDs or role names permitted to chat. */ + access?: string[]; + /** Permissions or roles the caller must ALL hold to use the agent. */ + permissions?: string[]; + /** Declared scope. Only `private` is gate-able at the route layer today. */ + visibility?: 'global' | 'organization' | 'private'; +} + +export interface AgentAccessDecision { + allowed: boolean; + /** Human-readable reason when denied (safe to surface in a 403). */ + reason?: string; +} + +/** + * Enforce per-agent access control (ADR-0049, #1884). + * + * Before this check the chat route only enforced the coarse, static route-level + * permissions (`['ai:chat','ai:agents']`) — every agent's `access`/`permissions` + * were a no-op, so "who can chat with this agent" enforced nothing. This closes + * that gap for the two fields that concretely express it: + * + * - `permissions` (required): the caller must hold EVERY listed entry. An + * entry is satisfied if it appears in the caller's `permissions` OR `roles` + * (the spec field is "Required permissions or roles"). Empty → no extra + * requirement beyond the route-level gate. + * - `access` (allow-list): when present and non-empty, the caller must match + * at least one entry, by `userId` or by membership in `roles`. Empty/absent + * → not restricted by allow-list. + * + * `visibility` (`organization`/`private`) is intentionally NOT enforced here: + * the request context carries no tenant id or agent-owner, so a correct + * organization/private gate needs auth-middleware changes (tracked separately). + * Enforcing a partial/guessed version would risk both lock-out and false + * security, so we enforce only what the context can decide. + * + * Fails CLOSED on a malformed user (no userId) — an unauthenticated caller that + * slipped past the route gate is denied rather than defaulted-open. + */ +export function evaluateAgentAccess( + agent: AgentAccessSpec, + user: RouteUserContext | undefined, +): AgentAccessDecision { + const required = agent.permissions ?? []; + const allowList = agent.access ?? []; + + // No per-agent restriction declared → allowed (route-level gate already passed). + if (required.length === 0 && allowList.length === 0) { + return { allowed: true }; + } + + if (!user || !user.userId) { + return { allowed: false, reason: 'authentication required to chat with this agent' }; + } + + const held = new Set([...(user.permissions ?? []), ...(user.roles ?? [])]); + + // Required permissions: caller must hold ALL. + const missing = required.filter((p) => !held.has(p)); + if (missing.length > 0) { + return { + allowed: false, + reason: `missing required permission(s): ${missing.join(', ')}`, + }; + } + + // Allow-list: caller must match by userId or role. + if (allowList.length > 0) { + const roles = new Set(user.roles ?? []); + const matched = allowList.some((entry) => entry === user.userId || roles.has(entry)); + if (!matched) { + return { allowed: false, reason: 'not in this agent’s access list' }; + } + } + + return { allowed: true }; +} diff --git a/packages/services/service-ai/src/routes/agent-routes.ts b/packages/services/service-ai/src/routes/agent-routes.ts index a3d6fd52c1..ce3ee2957e 100644 --- a/packages/services/service-ai/src/routes/agent-routes.ts +++ b/packages/services/service-ai/src/routes/agent-routes.ts @@ -9,6 +9,7 @@ import type { RouteDefinition } from './ai-routes.js'; import type { AgentChatQuota } from '../quota/agent-chat-quota.js'; import { normalizeMessage, validateMessageContent } from './message-utils.js'; import { encodeVercelDataStream } from '../stream/vercel-stream-encoder.js'; +import { evaluateAgentAccess } from './agent-access.js'; /** * Allowed message roles for the agent chat endpoint. @@ -143,6 +144,18 @@ export function buildAgentRoutes( return { status: 403, body: { error: `Agent "${agentName}" is not active` } }; } + // ── Per-agent access control (ADR-0049, #1884) ─────────── + // The route-level `permissions` gate above is the same for every + // agent; it does NOT honour this agent's own `access`/`permissions`. + // Enforce those here so "who can chat with this agent" is real. + const access = evaluateAgentAccess(agent, req.user); + if (!access.allowed) { + return { + status: 403, + body: { error: `Access to agent "${agentName}" denied: ${access.reason}` }, + }; + } + // ── Per-turn quota gate (optional) ─────────────────────── // Refusal is HONEST at the moment of impact (ADR-0040 §5): why, when // it recovers, and the way out. Streaming clients get the copy as a diff --git a/packages/spec/src/security/permission.zod.ts b/packages/spec/src/security/permission.zod.ts index 9e0476492d..df2b7340f3 100644 --- a/packages/spec/src/security/permission.zod.ts +++ b/packages/spec/src/security/permission.zod.ts @@ -24,10 +24,19 @@ export const ObjectPermissionSchema = lazySchema(() => z.object({ /** D: Delete (Owned records or Shared records) */ allowDelete: z.boolean().default(false).describe('Delete permission'), - /** Lifecycle Operations */ - allowTransfer: z.boolean().default(false).describe('Change record ownership'), - allowRestore: z.boolean().default(false).describe('Restore from trash (Undelete)'), - allowPurge: z.boolean().default(false).describe('Permanently delete (Hard Delete/GDPR)'), + /** + * Lifecycle Operations. + * + * EXPERIMENTAL — not enforced (ADR-0049). The `transfer`/`restore`/`purge` + * operations these bits gate do not yet exist as ObjectQL operations, and no + * runtime consumer reads these bits. Authoring them is currently a no-op. + * The runtime fails CLOSED if such an operation is ever introduced without a + * matching permission mapping (see `permission-evaluator.ts` + * DESTRUCTIVE_OPERATIONS). Tracked by #1883. + */ + allowTransfer: z.boolean().default(false).describe('[EXPERIMENTAL — not enforced] Change record ownership'), + allowRestore: z.boolean().default(false).describe('[EXPERIMENTAL — not enforced] Restore from trash (Undelete)'), + allowPurge: z.boolean().default(false).describe('[EXPERIMENTAL — not enforced] Permanently delete (Hard Delete/GDPR)'), /** * View All Records: Super-user read access.