diff --git a/.changeset/member-default-explicit-allow.md b/.changeset/member-default-explicit-allow.md new file mode 100644 index 0000000000..d2a6bb6374 --- /dev/null +++ b/.changeset/member-default-explicit-allow.md @@ -0,0 +1,76 @@ +--- +"@objectstack/plugin-security": major +--- + + + +fix(plugin-security)!: `member_default` no longer grants a `*` wildcard — the platform baseline is explicit-allow (#5491) + +**This is a deliberate, breaking narrowing of the default security posture. +Deployments that relied on the implicit wildcard lose that access. That is the +intended behaviour change, not a side effect — read the migration below before +upgrading.** + +`member_default` is the additive `everyone` baseline: it resolves for **every** +authenticated member, in addition to whatever else they hold. It carried +`object_permissions["*"] = {allowCreate: true, allowRead: true, allowEdit: true, +allowDelete: false}`, and object permissions merge most-permissively — so that +entry was not a default, it was a **floor no application could get under**. An +app's explicit-allow object gate was erased on three of the four axes; only +delete stayed profile-driven, because the baseline never granted it. + +HotCRM's 17.0 GA sweep measured the consequence across 5 profiles × 17 objects +(188 probes, each user with their own bearer token): + +- **21 of 21 create-DENIAL probes returned `201`** — every profile created on + every object once validation passed, including objects the profile explicitly + denied; +- a `service_agent` profile that declares no edit anywhere edited its own + `crm_account`; +- on `public_read` objects the wildcard yielded **`200` with ALL rows** for + non-holders — real unauthorized reads, not the documented "200 with 0 rows" + empty-set pattern; +- `security/explain` stated it outright for a profile carrying an all-false + deny: *"create on 'crm_opportunity' is granted by [member_default]"*. + +Because app-side authorization suites validate the app's *declarations*, CI +stayed green while the runtime posture was default-open — `declared ≠ enforced` +inside the security layer itself. + +**The change.** The wildcard is removed on all three live axes. The platform +baseline narrows to explicit-allow: object access now comes from OWDs plus +profile / permission-set **declarations** only. Deny-precedence merge semantics +were considered and rejected — permission sets remain additive capability +containers (ADR-0090); the fix is to stop the platform shipping a grant nobody +asked for, not to invent a veto. + +What `member_default` still declares, it still enforces, and nothing here is +newly granted: read on the better-auth identity tables (their writes stay +denied — that door is better-auth), self-service on `sys_user_preference` (now +an explicit entry rather than an implicit one; the effective access for a member +is byte-identical, and its `sys_user_preference_self` RLS carve-out already +declared exactly that intent), and every row-level policy it shipped before — +`owner_only_writes`, `owner_only_deletes` and the identity `_self` carve-outs +are untouched. The set stays anchor-safe, so its `everyone` binding is +unaffected. `admin_full_access`, `organization_admin` and `viewer_readonly` keep +their wildcards: those are granted deliberately to a principal, which is exactly +what the baseline was not. + +## Migration + +After upgrading, a member holding **no** application profile has no access to +application objects. Restore access by declaring it, in one of two places: + +1. **Ship an app default profile.** Mark a permission set `isDefault: true` and + the CLI wires it as the additive per-request baseline (ADR-0056 D7 / + ADR-0090 D5). This is the recommended route and what the bundled showcase app + already does — list the objects members legitimately touch, with the axes + they need. +2. **Grant per position / per user.** Bind an ordinary permission set through + `sys_position_permission_set` or `sys_user_permission_set`. + +To find what a deployment was silently relying on, ask +`GET /api/v1/security/explain?object=&operation=` for a +representative member before upgrading: any answer attributing the grant to +`[member_default]` on an application object is access that will stop. An app +whose own profiles already declare everything its users do is unaffected. diff --git a/.changeset/row-write-widener-composition.md b/.changeset/row-write-widener-composition.md new file mode 100644 index 0000000000..52aaa8a4e6 --- /dev/null +++ b/.changeset/row-write-widener-composition.md @@ -0,0 +1,62 @@ +--- +"@objectstack/plugin-security": minor +--- + +fix(plugin-security): the row-level write gate honours `modifyAllRecords` and `edit`-level record shares (#5492) + +HotCRM's 17.0 GA acceptance sweep measured two declared write-widening +mechanisms as completely inert. A manager profile carrying `viewAllRecords` + +`modifyAllRecords` got `403 … (row-level security)` on **every** cross-owner +write — update and delete, four objects — while its reads widened exactly as +declared (43/43, 9/9). And all three `edit`-level sharing rules materialised +into `sys_record_share` correctly and widened reads exactly, yet a `PATCH` by +the share target was refused every time. Read-level shares correctly denied +writes, so the machinery distinguished the levels on paper and the write gate +then ignored the distinction. + +**One root cause.** Row-level write access was two authorities AND-ed together +with no knowledge of each other. `ISharingService` reads all three declared +wideners (ownership at write DEPTH, `sys_record_share.access_level`, the +`modifyAllRecords` bypass); the security plugin's by-id write pre-image gate +read only RLS — and sitting inside that RLS is the platform's own ownership +floor, `owner_only_writes` / `owner_only_deletes` (`created_by == +current_user.id`, applicability `positions: ['org_member']`). That floor is a +second implementation of "ownership", and it is the one blind to every widener. +Every member resolves it additively from the `member_default` baseline — a +manager is an org member too — so the widener-blind copy always won. + +**The fix is composition by provenance, not a new bypass.** The pre-image gate +now asks the authority that owns those mechanisms for its tri-state verdict +(`ISharingService.checkEdit` / `checkDelete`, the contract added in #6428): + +- `allow` — a positive basis exists, so the declared authority **replaces** the + platform floor; +- `abstain` — record sharing does not enforce on this row at all (a `public` + object, an object with no owner field, a platform internal), so the floor + **stays**: it is the only row-level write gate such rows have; +- `deny` — the floor stays; the refusal belongs to the sharing middleware that + produced the verdict. + +The action boundary is inherited rather than restated (ADR-0111 D3): update asks +`checkEdit`, delete asks `checkDelete`, so an `edit` share widens update and +still does not confer delete. `modifyAllRecords` covers both verbs +(`MODIFY_ALL_WRITE_KEYS`). + +**What is deliberately unchanged.** Layer 0's tenant wall and every +**app-authored** RLS policy are untouched — only the policies the platform +itself ships are replaceable, matched by the same `(object, name, using)` +provenance key ADR-0105 D3 uses for tenant policies, so an app policy spelling +the identical predicate keeps refusing (ADR-0049: a declared security property +stays declared). This is therefore not `modifyAllRecords` bypassing write-side +RLS on an ordinary business posture, which ADR-0066 ① withholds and this change +leaves withheld; it is the platform's floor deferring to the platform's own +ownership authority. The on-behalf-of (ADR-0090 D10) path keeps both principals' +floors, matching `hasWriteBypass`, which already fails closed for a delegated +context. A deployment without `@objectstack/plugin-sharing` sees no change at +all: with nothing to consult, the gate abstains and the floor decides. + +Net effect for deployments: a Modify All Data holder can now correct, reassign +and clean up records they did not create, and an `edit`-share recipient can +finally edit the record shared with them. Nothing that was refused for lack of a +grant becomes permitted — read-share targets are still denied writes, `edit` +shares still cannot delete, and a member with neither is still refused. diff --git a/packages/plugins/plugin-security/src/authz-matrix-gate.test.ts b/packages/plugins/plugin-security/src/authz-matrix-gate.test.ts index f7b70f43fb..84eb88997d 100644 --- a/packages/plugins/plugin-security/src/authz-matrix-gate.test.ts +++ b/packages/plugins/plugin-security/src/authz-matrix-gate.test.ts @@ -35,10 +35,44 @@ import { describe, it, expect, vi } from 'vitest'; import { derivePosture, POSTURE_RANK } from '@objectstack/core'; import { SecurityPlugin, hasPlatformAdminCapability } from './security-plugin.js'; import { PermissionEvaluator } from './permission-evaluator.js'; -import { defaultPermissionSets } from './objects/default-permission-sets.js'; +import { defaultPermissionSets, BETTER_AUTH_MANAGED_OBJECTS } from './objects/default-permission-sets.js'; import { RLS_DENY_FILTER } from './rls-compiler.js'; import type { PermissionSet } from '@objectstack/spec/security'; +// [#5491] The grant the platform baseline used to IMPLY, now declared. +// +// `member_default` no longer ships a `'*'` object grant: it union-merged into +// every org member and erased app-side explicit-allow gates on create/read/edit, +// so the maintainer narrowed the baseline to explicit-allow (2026-08-07). This +// matrix is about the ROW-FILTER layers — Layer 0's tenant wall AND-composed +// with Layer 1's business RLS — and a cell can only report a filter if the CRUD +// gate ahead of it admits the operation at all. So the two member roles below +// now carry this set, which re-declares the removed wildcard BYTE-FOR-BYTE, +// identity-table carve-out included: +// +// - a PLAIN wildcard, which (deliberately, ADR-0066 D2) still does not cover a +// `private` object — so `crm_secret`'s CRUD_DENY cells stay verbatim; +// - the better-auth managed-object write denies, which under the old seed came +// from the same set and are what make `sys_user`'s write cells CRUD_DENY. +// +// `member_default` still resolves additively for both roles and still +// contributes the `owner_only_writes` / `owner_only_deletes` RLS the write cells +// pin — only the OBJECT bits moved out of the seed and into a declaration. +// Every expectation in EXPECTED_MATRIX is therefore unchanged. +const memberBaseline: PermissionSet = { + name: 'member_baseline', + label: 'Member baseline (the object grant the seed used to imply)', + objects: { + '*': { allowRead: true, allowCreate: true, allowEdit: true }, + ...Object.fromEntries( + BETTER_AUTH_MANAGED_OBJECTS.map((name) => [ + name, + { allowRead: true, allowCreate: false, allowEdit: false, allowDelete: false }, + ]), + ), + }, +} as any; + // A permissive, admin-authored business RLS policy (ADR-0095 W1's worked // example): "everyone may read rows whose status is public". At the RLS layer // this is OR-merged with the wildcard tenant policy today — so it is, by itself, @@ -65,7 +99,7 @@ const invoiceAuditor: PermissionSet = { objects: { crm_secret: { allowRead: true, allowCreate: true, allowEdit: true, viewAllRecords: true, modifyAllRecords: true } }, } as any; -const ALL_SETS: PermissionSet[] = [...defaultPermissionSets, publicReader, invoiceAuditor]; +const ALL_SETS: PermissionSet[] = [...defaultPermissionSets, memberBaseline, publicReader, invoiceAuditor]; const DENY = RLS_DENY_FILTER.id; // the fail-closed sentinel's marker value // ── Minimal middleware harness ────────────────────────────────────────────── @@ -195,10 +229,12 @@ const ROLES = { platform_admin: { userId: 'padmin', tenantId: 'org-1', positions: ['platform_admin'], permissions: ['admin_full_access'] }, // Org admin: holds organization_admin (also viewAll/modifyAll, but tenant-scoped by its RLS). org_admin: { userId: 'oadmin', tenantId: 'org-1', positions: ['org_admin'], permissions: ['organization_admin'] }, - // Rank-and-file member: only the additive member_default baseline; org_member gates owner_only_*. - member: { userId: 'u1', tenantId: 'org-1', positions: ['org_member'], permissions: [] }, + // Rank-and-file member: the additive member_default baseline (RLS: owner_only_*, + // gated by the org_member position) PLUS the declared object grant the baseline + // used to imply before #5491 — see `memberBaseline`. + member: { userId: 'u1', tenantId: 'org-1', positions: ['org_member'], permissions: ['member_baseline'] }, // Authenticated user with NO active organization → tenant scoping cannot resolve → fail-closed. - no_org_member: { userId: 'u2', positions: ['org_member'], permissions: [] }, + no_org_member: { userId: 'u2', positions: ['org_member'], permissions: ['member_baseline'] }, }; // The locked snapshot of POST-EXTRACTION behavior (Layer0 AND Layer1). Read the diff --git a/packages/plugins/plugin-security/src/member-default-explicit-allow.test.ts b/packages/plugins/plugin-security/src/member-default-explicit-allow.test.ts new file mode 100644 index 0000000000..9100a4f371 --- /dev/null +++ b/packages/plugins/plugin-security/src/member-default-explicit-allow.test.ts @@ -0,0 +1,175 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// [#5491] The platform baseline is EXPLICIT-ALLOW: `member_default` ships no +// `'*'` object grant. +// +// The measured defect (HotCRM 17.0 GA sweep, 188 probes, 5 profiles × 17 +// objects, `@objectstack/*` 17.0.0-rc.2): `member_default` carried +// `object_permissions['*'] = {allowCreate, allowRead, allowEdit}` and is +// union-merged (most-permissive) into every org member, so an application's +// explicit-allow object gate was erased on three axes. 21 of 21 create-DENIAL +// probes returned 201; a `service_agent` profile that declares no edit anywhere +// edited its own `crm_account`; on `public_read` objects a non-holder read ALL +// rows. `security/explain` stated it outright for a profile carrying an +// all-false deny: "create on 'crm_opportunity' is granted by [member_default]". +// +// Maintainer ruling (2026-08-07, issue comment 5219845380): remove the wildcard +// on all three live axes; the baseline narrows to explicit-allow, and object +// access comes from OWDs plus profile / permission-set declarations only. +// Deny-precedence merge semantics were considered and REJECTED — these cases +// must never be "fixed" by giving a deny priority over the union. +// +// The evaluator is exercised through its REAL merge (`checkObjectPermission` +// over the real seed), because the union is the mechanism at fault: pinning the +// seed's shape alone would stay green if a wildcard came back from anywhere +// else in the baseline. +import { describe, it, expect } from 'vitest'; +import { PermissionSetSchema } from '@objectstack/spec/security'; +import type { PermissionSet } from '@objectstack/spec/security'; +import { PermissionEvaluator } from './permission-evaluator.js'; +import { defaultPermissionSets, BETTER_AUTH_MANAGED_OBJECTS } from './objects/default-permission-sets.js'; + +const evaluator = new PermissionEvaluator(); +const MEMBER_DEFAULT = defaultPermissionSets.find((p) => p.name === 'member_default')!; + +/** The app object at the centre of the report. */ +const APP_OBJECT = 'crm_opportunity'; + +/** A profile that DECLARES an explicit all-false deny — the erased gate. */ +const DENYING_PROFILE: PermissionSet = PermissionSetSchema.parse({ + name: 'service_agent', + objects: { + [APP_OBJECT]: { allowRead: false, allowCreate: false, allowEdit: false, allowDelete: false }, + }, +}); + +/** A profile that DECLARES access — the positive control per axis. */ +const GRANTING_PROFILE: PermissionSet = PermissionSetSchema.parse({ + name: 'sales_rep', + objects: { + [APP_OBJECT]: { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true }, + }, +}); + +/** Operations, one per axis the ruling names, plus the axis already correct. */ +const AXES = [ + { axis: 'read', operation: 'find' }, + { axis: 'create', operation: 'insert' }, + { axis: 'edit', operation: 'update' }, + { axis: 'delete', operation: 'delete' }, +] as const; + +const allows = (operation: string, sets: PermissionSet[], objectName = APP_OBJECT, isPrivate = false) => + evaluator.checkObjectPermission(operation, objectName, sets, { isPrivate }); + +describe('[#5491] `member_default` ships no wildcard object grant', () => { + it('has no `*` key at all — not a narrowed one, not a false-valued one', () => { + expect(Object.keys(MEMBER_DEFAULT.objects ?? {})).not.toContain('*'); + }); + + it('every entry it does ship NAMES its object (explicit-allow, no sentinels)', () => { + for (const key of Object.keys(MEMBER_DEFAULT.objects ?? {})) { + expect(key, 'no wildcard or glob sentinels').not.toMatch(/\*/); + } + }); +}); + +describe('[#5491] the baseline no longer erases an app-declared object gate', () => { + it.each(AXES)('$axis: the baseline ALONE grants nothing on an app object', ({ operation }) => { + expect(allows(operation, [MEMBER_DEFAULT])).toBe(false); + }); + + it.each(AXES)( + '$axis: an explicit all-false profile stays denied once the baseline is union-merged in', + ({ operation }) => { + // This is the exact merge that produced "granted by [member_default]". + expect(allows(operation, [DENYING_PROFILE, MEMBER_DEFAULT])).toBe(false); + }, + ); + + it.each(AXES)('$axis: a profile that DECLARES the grant still works (positive probe)', ({ operation }) => { + expect(allows(operation, [GRANTING_PROFILE, MEMBER_DEFAULT])).toBe(true); + }); + + it('an object no set mentions is denied on every axis (the `public_read` all-rows leak)', () => { + for (const { operation } of AXES) { + expect(allows(operation, [GRANTING_PROFILE, MEMBER_DEFAULT], 'crm_account')).toBe(false); + } + }); + + it('the delete axis is unchanged — it was already profile-driven before this change', () => { + expect(allows('delete', [MEMBER_DEFAULT])).toBe(false); + expect(allows('delete', [GRANTING_PROFILE, MEMBER_DEFAULT])).toBe(true); + }); +}); + +describe('[#5491] what the baseline still declares, it still enforces', () => { + it('better-auth identity tables stay READABLE for a member with no app profile', () => { + for (const object of BETTER_AUTH_MANAGED_OBJECTS) { + expect(allows('find', [MEMBER_DEFAULT], object), `${object} readable`).toBe(true); + } + }); + + it('better-auth identity tables stay WRITE-DENIED (the door is better-auth, not CRUD)', () => { + for (const object of BETTER_AUTH_MANAGED_OBJECTS) { + expect(allows('insert', [MEMBER_DEFAULT], object), `${object} insert`).toBe(false); + expect(allows('update', [MEMBER_DEFAULT], object), `${object} update`).toBe(false); + expect(allows('delete', [MEMBER_DEFAULT], object), `${object} delete`).toBe(false); + } + }); + + it('self-service preferences survive the wildcard removal as an EXPLICIT grant', () => { + // `sys_user_preference` is not a better-auth table, so the managed-deny + // block does not cover it, and its `sys_user_preference_self` RLS policy + // (`operation: 'all'`) declares that a member reads and writes their own + // rows. Under the wildcard that grant was implicit; it is now named. + expect(allows('find', [MEMBER_DEFAULT], 'sys_user_preference')).toBe(true); + expect(allows('insert', [MEMBER_DEFAULT], 'sys_user_preference')).toBe(true); + expect(allows('update', [MEMBER_DEFAULT], 'sys_user_preference')).toBe(true); + expect(allows('delete', [MEMBER_DEFAULT], 'sys_user_preference')).toBe(false); + const policy = (MEMBER_DEFAULT.rowLevelSecurity ?? []) + .find((p) => p.name === 'sys_user_preference_self'); + expect(policy, 'the RLS carve-out that scopes it is still shipped').toBeTruthy(); + expect(policy!.using).toBe('user_id == current_user.id'); + }); + + it('the owner-scoped write RLS is untouched — object bits moved, row policies did not', () => { + const names = (MEMBER_DEFAULT.rowLevelSecurity ?? []).map((p) => p.name); + expect(names).toContain('owner_only_writes'); + expect(names).toContain('owner_only_deletes'); + }); + + it('the set stays anchor-safe for `everyone` (ADR-0090 D5 / #2753)', () => { + // Removing a grant can only narrow, but the bootstrap binds this set to the + // anchor and an anchor-forbidden bit makes the whole baseline unbindable. + for (const perm of Object.values(MEMBER_DEFAULT.objects ?? {}) as any[]) { + expect(perm.allowDelete ?? false).toBe(false); + expect(perm.allowExport ?? false).toBe(false); + expect(perm.viewAllRecords ?? false).toBe(false); + expect(perm.modifyAllRecords ?? false).toBe(false); + } + }); +}); + +describe('[#5491] the admin sets keep their wildcards (this is a BASELINE change only)', () => { + it.each(['admin_full_access', 'organization_admin', 'viewer_readonly'])( + '%s still carries a `*` entry', + (name) => { + const set = defaultPermissionSets.find((p) => p.name === name)!; + expect(set, `${name} is shipped`).toBeTruthy(); + expect(Object.keys(set.objects ?? {})).toContain('*'); + }, + ); + + it('viewer_readonly is read-only, unchanged — its wildcard is a CEILING, not a floor', () => { + // The distinction that makes removing one wildcard and keeping the other + // coherent: `viewer_readonly` is granted deliberately to a principal, while + // `member_default` resolved additively for EVERY member. + const viewer = defaultPermissionSets.find((p) => p.name === 'viewer_readonly')!; + const wildcard = (viewer.objects as any)['*']; + expect(wildcard.allowRead).toBe(true); + expect(wildcard.allowCreate).toBe(false); + expect(wildcard.allowEdit).toBe(false); + expect(wildcard.allowDelete).toBe(false); + }); +}); diff --git a/packages/plugins/plugin-security/src/objects/default-permission-sets.ts b/packages/plugins/plugin-security/src/objects/default-permission-sets.ts index 8b9800d74b..9260a9a154 100644 --- a/packages/plugins/plugin-security/src/objects/default-permission-sets.ts +++ b/packages/plugins/plugin-security/src/objects/default-permission-sets.ts @@ -314,27 +314,53 @@ const baseDefaultPermissionSets: PermissionSet[] = [ name: 'member_default', label: 'Member — Standard Access', objects: { - // [ADR-0090 D5, #2753] NO `allowDelete`: delete/purge/transfer are - // anchor-forbidden bits, and this set IS the `everyone` baseline — the - // bootstrap binds it to the anchor, so it must stay anchor-safe. + // [#5491] NO `'*'` WILDCARD GRANT. This set is the additive `everyone` + // baseline — it resolves for EVERY authenticated member — and object + // permissions merge most-permissively, so a wildcard here was not a + // default, it was a FLOOR no app could get under. An application that + // declared an explicit all-false deny on one of its objects still had + // create, read and edit on it (only `allowDelete` stayed profile-driven, + // because this set never granted it), and `security/explain` said so + // outright: "create on 'crm_opportunity' is granted by [member_default]". + // HotCRM's 17.0 GA sweep measured the consequence across 5 profiles × + // 17 objects: 21 of 21 create-denial probes returned 201, and on + // `public_read` objects a non-holder read ALL rows. + // + // Maintainer ruling (2026-08-07): the platform baseline narrows to + // explicit-allow. Object access comes from OWDs plus profile / + // permission-set declarations only — declared IS enforced. Deny-precedence + // merge semantics were considered and REJECTED; do not reintroduce a + // wildcard here "with a priority", and do not reintroduce one at all. + // + // What a member still gets from this set is what the set can actually + // NAME: read on the better-auth identity tables (below), self-service on + // their own preferences, and the `_self` RLS carve-outs that scope both. + // Everything else is the application's to declare. + // + // [ADR-0090 D5, #2753] There is still NO `allowDelete` anywhere in this + // set: delete/purge/transfer are anchor-forbidden bits and the bootstrap + // binds this set to the `everyone` anchor, so it must stay anchor-safe. // Deleting records is not a baseline right; grant it per object via an // ordinary (position-distributed) set where the domain calls for it. // The owner-scoped delete RLS below is KEPT as a narrowing defense for // members who receive a delete bit from such a set. // // [#3544] NO `allowExport` either, for the same reason and deliberately: - // this set is the `everyone` baseline, so granting export here would hand - // bulk egress to every authenticated user and make the opt-in axis a - // no-op. Bulk export is not a baseline right — grant it per object via an - // ordinary position-distributed set where the domain calls for it. Do not - // "fix" its absence. - '*': { - allowRead: true, - allowCreate: true, - allowEdit: true, - }, - // Identity tables are managed by better-auth — no direct writes. + // granting export here would hand bulk egress to every authenticated user + // and make the opt-in axis a no-op. Do not "fix" its absence. + // + // Identity tables are managed by better-auth — readable, never written + // directly. With the wildcard gone this is no longer a narrowing overlay + // but the grant itself: it is what keeps `/auth/me`, the org switcher and + // the Account app working for a member with no application profile. ...denyWritesOnManagedObjects(), + // Self-service preferences. NOT a better-auth table, so it is not covered + // by the block above, and its `sys_user_preference_self` RLS policy below + // (`operation: 'all'`) declares exactly this intent: a member reads and + // writes their OWN preference rows. Under the wildcard that grant was + // implicit; making it explicit is the migration, not a widening — the + // effective access for a member is byte-identical. + sys_user_preference: { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: false }, }, rowLevelSecurity: [ // [ADR-0095 D1] The wildcard `tenant_isolation` policy RETIRED here — the diff --git a/packages/plugins/plugin-security/src/platform-ownership-policies.test.ts b/packages/plugins/plugin-security/src/platform-ownership-policies.test.ts new file mode 100644 index 0000000000..72a61299c7 --- /dev/null +++ b/packages/plugins/plugin-security/src/platform-ownership-policies.test.ts @@ -0,0 +1,97 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// [#5492] Provenance for the platform's row-level write ownership floor. +// +// The pre-image gate lets a declared write authority REPLACE this floor. What +// makes that safe is that only the PLATFORM's own floor is replaceable — the +// same rule ADR-0105 D3 records for tenant policies, and for the same reason: +// its predecessor matched on a substring of the public RLS grammar and +// swallowed app-authored policies with it (finding F1), silently unenforcing a +// declared security property. +import { describe, it, expect } from 'vitest'; +import { + isPlatformOwnershipFloorPolicy, + platformOwnershipFloorPolicyCount, + OWNERSHIP_FLOOR_PREDICATE, +} from './platform-ownership-policies.js'; +import { defaultPermissionSets } from './objects/default-permission-sets.js'; + +const shippedFloorPolicies = defaultPermissionSets + .flatMap((ps) => ps.rowLevelSecurity ?? []) + .filter((p) => typeof p.using === 'string' && p.using.trim() === OWNERSHIP_FLOOR_PREDICATE); + +describe('[#5492] platform ownership-floor provenance', () => { + it('recognises exactly the shipped write-class floor policies (non-vacuous)', () => { + // Guard against a broken derivation passing every case below by matching + // nothing at all. + expect(platformOwnershipFloorPolicyCount()).toBeGreaterThan(0); + expect(platformOwnershipFloorPolicyCount()).toBe(shippedFloorPolicies.length); + }); + + it('the shipped set is `owner_only_writes` (update) + `owner_only_deletes` (delete)', () => { + expect(shippedFloorPolicies.map((p) => `${p.name}:${p.operation}`).sort()).toEqual([ + 'owner_only_deletes:delete', + 'owner_only_writes:update', + ]); + for (const policy of shippedFloorPolicies) { + expect(isPlatformOwnershipFloorPolicy(policy)).toBe(true); + } + }); + + it('an APP-AUTHORED policy spelling the same predicate is NOT the platform floor', () => { + // The ADR-0105 F1 shape: `created_by == current_user.id` is public grammar, + // so an app writes it too. Its policy must reach the compiler untouched — + // no composition may drop a declared security property (ADR-0049). + expect( + isPlatformOwnershipFloorPolicy({ + object: 'crm_opportunity', + name: 'app_owner_only', + using: OWNERSHIP_FLOOR_PREDICATE, + }), + ).toBe(false); + // Same object as the shipped wildcard, different name → still authored. + expect( + isPlatformOwnershipFloorPolicy({ + object: '*', + name: 'my_owner_rule', + using: OWNERSHIP_FLOOR_PREDICATE, + }), + ).toBe(false); + // Same name, different predicate → still authored. + expect( + isPlatformOwnershipFloorPolicy({ + object: '*', + name: 'owner_only_writes', + using: 'owner_id == current_user.id', + }), + ).toBe(false); + }); + + it('an unrelated shipped policy is not the floor (the `_self` identity carve-outs)', () => { + const selfPolicy = defaultPermissionSets + .flatMap((ps) => ps.rowLevelSecurity ?? []) + .find((p) => p.name === 'sys_user_self')!; + expect(selfPolicy, 'the carve-out is still shipped').toBeTruthy(); + expect(isPlatformOwnershipFloorPolicy(selfPolicy)).toBe(false); + }); + + it('the derivation only admits WRITE-class shipped policies', () => { + // The floor is a write-side construct. If a future seed ever ships a + // `select` policy carrying this predicate it must NOT enter the set — a + // write-gate composition dropping it would be widening reads it never + // consulted. Asserted as a property of the whole recognised set rather than + // against a hypothetical policy, because the identity key is + // `(object, name, using)` and would not carry the operation. + for (const policy of shippedFloorPolicies) { + if (isPlatformOwnershipFloorPolicy(policy)) { + expect(['update', 'delete', 'all']).toContain(String(policy.operation)); + } + } + // …and every shipped write-class policy with the predicate IS recognised, + // so the loop above can never pass by recognising nothing. + const writeClass = shippedFloorPolicies.filter((p) => + ['update', 'delete', 'all'].includes(String(p.operation)), + ); + expect(writeClass.length).toBe(platformOwnershipFloorPolicyCount()); + }); +}); diff --git a/packages/plugins/plugin-security/src/platform-ownership-policies.ts b/packages/plugins/plugin-security/src/platform-ownership-policies.ts new file mode 100644 index 0000000000..d5c610d0e7 --- /dev/null +++ b/packages/plugins/plugin-security/src/platform-ownership-policies.ts @@ -0,0 +1,103 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. + +/** + * [#5492] Provenance for the platform's OWN row-level WRITE ownership floor. + * + * `member_default` — the additive `everyone` baseline every authenticated + * member resolves — ships two wildcard write policies keyed on the column the + * engine stamps on every record: + * + * ``` + * owner_only_writes object '*' operation 'update' created_by == current_user.id + * owner_only_deletes object '*' operation 'delete' created_by == current_user.id + * ``` + * + * They are the platform's answer to #1985 (a by-id write builds no `ast`, so a + * row predicate would never be applied), and they are a **second implementation + * of "ownership"** — the one that knows nothing about the wideners the platform + * also declares. `ISharingService` is the other, and the only one that reads + * write DEPTH, `sys_record_share.access_level` and the `modifyAllRecords` + * bypass. While both ran as an unconditional AND, the widener-blind copy always + * won: every `member_default` + `org_member` principal — which is *every* + * member, a manager included — was pinned to `created_by == me` no matter what + * their profile or their shares declared (#5492's measurement: 403 on every + * cross-owner UPDATE and DELETE, and on every `edit`-level share). + * + * The composition that fixes it needs to name these two policies WITHOUT + * naming them by string: the pre-image gate lets the declared write authority + * REPLACE this floor (see `SecurityPlugin.computeLayeredRlsFilter`'s + * `dropPlatformOwnershipFloor`), and it may only replace the floor the + * PLATFORM shipped — an app-authored policy is a declared security property + * and must always reach the compiler (ADR-0049; the same reasoning + * {@link ./platform-tenant-policies.ts} records for ADR-0105 finding F1, where + * a substring match on a public grammar token silently swallowed authored + * policies). + * + * So provenance, not pattern-matching, exactly as ADR-0105 D3 does it: the + * identity key is `(object, name, using)`, built from the shipped declaration + * itself. An app policy can only collide by being byte-identical to a shipped + * one on the same object, in which case treating it as the floor is the same + * decision the platform already made for its own copy. + * + * Deliberately NOT an authorable "this is a floor" flag on the RLS schema: + * provenance is a fact about who shipped a policy, and letting metadata claim + * it would hand authors a switch that turns their own policy off. + */ + +import type { RowLevelSecurityPolicy } from '@objectstack/spec/security'; + +import { defaultPermissionSets } from './objects/default-permission-sets.js'; + +/** + * The predicate that makes a shipped policy an OWNERSHIP floor. Used ONLY to + * select which shipped policies enter the provenance set — never as a matcher + * against authored input (that is ADR-0105 finding F1's mistake). + */ +export const OWNERSHIP_FLOOR_PREDICATE = 'created_by == current_user.id'; + +/** + * RLS operations that are WRITE classes. The floor is a write-side construct: + * a shipped `select` policy carrying the same predicate would be read scoping + * and must never be dropped by a write-gate composition. + */ +const WRITE_RLS_OPERATIONS: ReadonlySet = new Set(['update', 'delete', 'all']); + +/** `\u0000` cannot appear in an object/policy name or a `using` expression. */ +function provenanceKey(policy: Pick): string { + return `${policy.object ?? ''}\u0000${policy.name ?? ''}\u0000${policy.using ?? ''}`; +} + +/** + * Identity keys of every write-side ownership-floor policy the platform itself + * ships. Built once from the compiled declaration — the same constant + * `bootstrapPlatformAdmin` seeds `sys_permission_set` rows from, so a policy + * read back from the database matches its shipped original. + */ +const PLATFORM_OWNERSHIP_FLOOR_KEYS: ReadonlySet = (() => { + const keys = new Set(); + for (const ps of defaultPermissionSets) { + for (const policy of ps.rowLevelSecurity ?? []) { + if (typeof policy.using !== 'string') continue; + if (policy.using.trim() !== OWNERSHIP_FLOOR_PREDICATE) continue; + if (!WRITE_RLS_OPERATIONS.has(String(policy.operation ?? ''))) continue; + keys.add(provenanceKey(policy)); + } + } + return keys; +})(); + +/** + * True iff this policy is one the PLATFORM ships as its row-level write + * ownership floor — the only policies the pre-image gate may let a declared + * write authority replace. + */ +export function isPlatformOwnershipFloorPolicy( + policy: Pick, +): boolean { + return PLATFORM_OWNERSHIP_FLOOR_KEYS.has(provenanceKey(policy)); +} + +/** Test/diagnostic accessor — the number of shipped floor policies recognized. */ +export function platformOwnershipFloorPolicyCount(): number { + return PLATFORM_OWNERSHIP_FLOOR_KEYS.size; +} diff --git a/packages/plugins/plugin-security/src/row-write-widener-composition.test.ts b/packages/plugins/plugin-security/src/row-write-widener-composition.test.ts new file mode 100644 index 0000000000..5f24eb2d03 --- /dev/null +++ b/packages/plugins/plugin-security/src/row-write-widener-composition.test.ts @@ -0,0 +1,517 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// [#5492] The row-level WRITE gate composes by PROVENANCE. +// +// HotCRM's 17.0 GA acceptance sweep measured two declared write-widening +// mechanisms as completely inert on `@objectstack/*` 17.0.0-rc.2: +// +// 1. a manager profile carrying `viewAllRecords` + `modifyAllRecords` got +// 403 "(row-level security)" on EVERY cross-owner write — update and +// delete, four objects — while its reads widened exactly as declared +// (43/43, 9/9); +// 2. all three `edit`-level sharing rules materialised into +// `sys_record_share` correctly and widened READS exactly, yet a PATCH by +// the share target got 403 every time. +// +// One root cause: row-level write access was two authorities AND-ed together +// with no knowledge of each other. `plugin-sharing` reads write DEPTH, the +// share level and the `modifyAllRecords` bypass; `plugin-security`'s pre-image +// gate read only RLS — and sitting in that RLS is the platform's OWN ownership +// floor (`owner_only_writes` / `owner_only_deletes`, `created_by == +// current_user.id`, applicability `positions: ['org_member']`). Every member +// resolves it additively, a manager included, so the widener-blind copy of +// "ownership" always won. +// +// Maintainer ruling (2026-08-07, issue comment 5219846435): "enforce both +// declared write-widening mechanisms. The row-level write gate must consult +// `modifyAllRecords` (profile axis) and `sys_record_share.access_level = +// 'edit'` (share axis)." Route: PR #6564's tri-state `ISharingService` verdict, +// composed by provenance — `allow` replaces the platform floor, `abstain` and +// `deny` leave it standing. +// +// This file drives the REAL stack for that composition: the real SecurityPlugin +// middleware, the real SharingService (late-bound to the security service's own +// `hasWriteBypass`, so the bypass is the same predicate `security/explain` +// reports), the real sharing middleware, and — crucially — the REAL platform +// `member_default` seed, whose wildcard `owner_only_*` policies are the co-gate +// the issue measured. `vama-write-path-convergence.test.ts` cannot see this +// defect: none of its three permission sets authors an RLS policy at all, so +// `computeRlsFilter` answers `null` for every principal there (measured in +// #5492 comment 5224112673). +// +// The object is deliberately an ORDINARY tenant business object — +// `sharingModel: 'private'` but NO `access.default: 'private'` — because that +// posture is exactly what withholds the ADR-0066 ① Layer-1 superuser +// short-circuit, and it is the shape HotCRM ships. +import { describe, it, expect, vi, beforeEach } from 'vitest'; +import { assertEngineDeleteDispatch, assertEngineUpdateDispatch } from '@objectstack/metadata-core'; +import { SharingService, buildSharingMiddleware } from '@objectstack/plugin-sharing'; +import { PermissionSetSchema } from '@objectstack/spec/security'; +import type { PermissionSet } from '@objectstack/spec/security'; +import { SecurityPlugin } from './security-plugin.js'; +import { defaultPermissionSets } from './objects/default-permission-sets.js'; + +// ── metadata ─────────────────────────────────────────────────────────────── + +/** + * The measured shape: user-owned, `private` OWD, and an ORDINARY access posture + * (no `access.default: 'private'`). That last omission is load-bearing — + * `posturePermits` is false for it, so the Layer-1 superuser short-circuit is + * withheld and the platform ownership floor is the only thing standing between + * a Modify-All holder and the row. + */ +const OPPORTUNITY_SCHEMA = { + name: 'crm_opportunity', + sharingModel: 'private', + fields: { + id: { name: 'id' }, + name: { name: 'name' }, + next_step: { name: 'next_step' }, + stage: { name: 'stage' }, + owner_id: { name: 'owner_id' }, + created_by: { name: 'created_by' }, + organization_id: { name: 'organization_id' }, + }, +}; + +/** + * The #5492 E2 control: an object with **no `owner_id` column at all** — the + * most common shape for an author-defined object. Record sharing does not + * enforce on it (`checkEdit` → `abstain`), which means the platform's + * `created_by` floor is its ONLY row-level write gate. E2 measured what happens + * when an abstention is read as permission: an ordinary member's cross-creator + * UPDATE went from 403 to 200. The cases below pin that it does not. + */ +const NOTE_SCHEMA = { + name: 'crm_note', + sharingModel: 'private', + fields: { + id: { name: 'id' }, + body: { name: 'body' }, + created_by: { name: 'created_by' }, + organization_id: { name: 'organization_id' }, + }, +}; + +const SCHEMAS: Record = { + crm_opportunity: OPPORTUNITY_SCHEMA, + crm_note: NOTE_SCHEMA, +}; + +/** The platform seed under test — the source of `owner_only_writes/deletes`. */ +const MEMBER_DEFAULT = defaultPermissionSets.find((p) => p.name === 'member_default')!; + +/** The app's manager profile: View + Modify All Data on its own objects. */ +const CRM_MANAGER: PermissionSet = PermissionSetSchema.parse({ + name: 'crm_manager', + objects: { + crm_opportunity: { + allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true, + viewAllRecords: true, modifyAllRecords: true, + }, + crm_note: { + allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true, + viewAllRecords: true, modifyAllRecords: true, + }, + }, +}); + +/** + * The app's rank-and-file profile: ordinary CRUD, no bypass of any kind. The + * DELETE bit is granted deliberately so that when a delete is refused below it + * is the ROW-level gate refusing, never the object-level CRUD bit. + */ +const CRM_REP: PermissionSet = PermissionSetSchema.parse({ + name: 'crm_rep', + objects: { + crm_opportunity: { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true }, + crm_note: { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true }, + }, +}); + +/** + * [#5493 control] The SAME profile plus an APP-AUTHORED RLS update-widener: + * "anyone may update an opportunity still in `prospecting`". Applicable policies + * OR-combine, so at the RLS layer this widens PAST the platform ownership floor + * — which is the shape #5493 reports from the other side: the security gate + * admits the row and the SHARING middleware refuses it first. + * + * Present as a CONTROL, not a fix. This PR does not touch the sharing + * middleware; the case at the bottom of this file measures that #5493's symptom + * is unchanged by the composition landed here. + */ +const CRM_REP_WIDENED: PermissionSet = PermissionSetSchema.parse({ + name: 'crm_rep_widened', + objects: { + crm_opportunity: { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true }, + }, + rowLevelSecurity: [ + { + name: 'app_open_stage_updates', + object: 'crm_opportunity', + operation: 'update', + using: "stage == 'prospecting'", + }, + ], +}); + +const PERMISSION_SETS: PermissionSet[] = [MEMBER_DEFAULT, CRM_MANAGER, CRM_REP, CRM_REP_WIDENED]; + +// ── rows ─────────────────────────────────────────────────────────────────── + +const U_OTHER = 'u_other'; +const U_MANAGER = 'u_manager'; +const U_REP = 'u_rep'; +const U_EDIT_SHARE = 'u_edit_share'; +const U_READ_SHARE = 'u_read_share'; + +/** Owned AND created by somebody else — the cross-owner row the sweep probed. */ +const OPP_THEIRS = { + id: 'opp_theirs', name: 'Theirs', next_step: 'call', stage: 'prospecting', + owner_id: U_OTHER, created_by: U_OTHER, organization_id: 'org1', +}; +/** The rep's own row — the positive control the floor must keep admitting. */ +const OPP_MINE = { + id: 'opp_mine', name: 'Mine', next_step: 'call', stage: 'prospecting', + owner_id: U_REP, created_by: U_REP, organization_id: 'org1', +}; +/** Owner-less object, created by somebody else (the E2 shape). */ +const NOTE_THEIRS = { id: 'note_theirs', body: 'theirs', created_by: U_OTHER, organization_id: 'org1' }; + +/** The share rows the rule evaluator materialises — both levels, same record. */ +const SHARE_ROWS = [ + { + id: 'shr_edit', object_name: 'crm_opportunity', record_id: OPP_THEIRS.id, + recipient_type: 'user', recipient_id: U_EDIT_SHARE, access_level: 'edit', source: 'rule', + }, + { + id: 'shr_read', object_name: 'crm_opportunity', record_id: OPP_THEIRS.id, + recipient_type: 'user', recipient_id: U_READ_SHARE, access_level: 'read', source: 'rule', + }, +]; + +// ── in-memory engine ─────────────────────────────────────────────────────── + +function makeEngine() { + const tables: Record = { + crm_opportunity: [{ ...OPP_THEIRS }, { ...OPP_MINE }], + crm_note: [{ ...NOTE_THEIRS }], + sys_record_share: SHARE_ROWS.map((r) => ({ ...r })), + }; + const matches = (row: any, filter: any): boolean => { + if (!filter || typeof filter !== 'object') return true; + if (Array.isArray(filter.$or)) return filter.$or.some((f: any) => matches(row, f)); + if (Array.isArray(filter.$and)) return filter.$and.every((f: any) => matches(row, f)); + for (const [k, v] of Object.entries(filter)) { + if (k === '$or' || k === '$and') continue; + if (v != null && typeof v === 'object' && '$in' in (v as any)) { + if (!(v as any).$in.includes(row[k])) return false; + continue; + } + if (row[k] !== v) return false; + } + return true; + }; + const middlewares: any[] = []; + return { + _tables: tables, + _middlewares: middlewares, + registerMiddleware: (mw: any) => middlewares.push(mw), + getSchema: (name: string) => SCHEMAS[name], + async find(object: string, options: any = {}) { + const rows = (tables[object] ??= []); + return rows.filter((r) => matches(r, options.filter ?? options.where)).slice(0, options.limit ?? 1000); + }, + async findOne(object: string, options: any = {}) { + const rows = await this.find(object, { ...options, limit: 1 }); + return rows[0] ?? null; + }, + async insert(object: string, data: any) { + (tables[object] ??= []).push({ ...data }); + return data; + }, + // Both write verbs open with the PRODUCER's own dispatch predicate + // (#4550 / #5480 / #6277), never a hand-mirrored guard: a fixture that + // drifts to a call shape `ObjectQL` would refuse fails loudly here instead + // of collecting a green from gates that never ran. + async update(object: string, data: any, options?: any) { + const dispatch = assertEngineUpdateDispatch(data, options); + const rows = (tables[object] ??= []); + const targets = dispatch.kind === 'by-id' + ? rows.filter((r) => r.id === dispatch.id) + : rows.filter((r) => matches(r, options?.where)); + for (const r of targets) Object.assign(r, data); + return dispatch.kind === 'by-id' ? (targets[0] ?? null) : targets.length; + }, + async delete(object: string, options?: any) { + const dispatch = assertEngineDeleteDispatch(options); + const rows = (tables[object] ??= []); + const targets = dispatch.kind === 'by-id' + ? rows.filter((r) => r.id === dispatch.id) + : rows.filter((r) => matches(r, options?.where)); + tables[object] = rows.filter((r) => !targets.includes(r)); + return dispatch.kind === 'by-id' ? targets.length > 0 : targets.length; + }, + }; +} + +// ── the stack ────────────────────────────────────────────────────────────── + +interface WriteOutcome { + ok: boolean; + /** ADR-0112 envelope of the refusal — asserted, never a bare `toThrow()`. */ + code?: string; + status?: number; + message: string; +} + +interface Stack { + security: any; + sharing: SharingService; + write: ( + operation: 'update' | 'delete', + object: string, + recordId: string, + context: any, + ) => Promise; + rows: (object: string) => any[]; +} + +async function makeStack(): Promise { + const engine = makeEngine(); + const metadata = { + get: async (_type: string, name: string) => SCHEMAS[name] ?? null, + list: async () => PERMISSION_SETS, + }; + let security: any; + let sharing: SharingService; + const services: Record = { + manifest: { register: vi.fn() }, + objectql: engine, + metadata, + // Org scoping active, exactly as a multi-tenant deployment wires it — so + // Layer 0 contributes a real tenant predicate the composition must preserve. + 'org-scoping': { name: 'org-scoping' }, + get sharing() { return sharing; }, + }; + const ctx: any = { + logger: { info: vi.fn(), warn: vi.fn(), error: vi.fn(), debug: vi.fn() }, + registerService: (name: string, impl: any) => { if (name === 'security') security = impl; }, + getService: (name: string) => { + if (!(name in services)) throw new Error(`service not registered: ${name}`); + return services[name]; + }, + }; + const plugin = new SecurityPlugin({ fallbackPermissionSet: 'member_default' }); + await plugin.init(ctx); + await plugin.start(ctx); + if (!security) throw new Error('SecurityPlugin did not register the security service'); + + sharing = new SharingService({ engine: engine as any, securityService: () => security }); + const sharingMw = buildSharingMiddleware(sharing, ctx.logger); + const securityMw = engine._middlewares[0]; + + return { + security, + sharing, + rows: (object: string) => (engine._tables[object] ??= []), + async write(operation, object, recordId, context) { + const opCtx: any = { + object, + operation, + context: { ...context }, + ...(operation === 'update' + ? { data: { id: recordId, next_step: 'updated' } } + : { options: { where: { id: recordId } } }), + }; + let reached = false; + try { + await securityMw(opCtx, async () => { + await sharingMw(opCtx, async () => { + if (operation === 'delete') await engine.delete(opCtx.object, opCtx.options); + else await engine.update(opCtx.object, opCtx.data, opCtx.options); + reached = true; + }); + }); + } catch (e: any) { + return { + ok: false, + code: e?.code, + status: e?.statusCode, + message: String(e?.message ?? e), + }; + } + return reached + ? { ok: true, message: 'written' } + : { ok: false, message: 'middleware swallowed the write' }; + }, + }; +} + +/** + * The execution-context shape `resolveAuthzContext` hands the middleware. The + * `org_member` position is not decoration: it is the applicability domain of + * `owner_only_writes` / `owner_only_deletes`, and a manager is an org member + * too — which is precisely why the floor used to override their profile. + */ +const ctxFor = (userId: string, ...permissions: string[]) => ({ + userId, tenantId: 'org1', positions: ['org_member'], permissions, +}); + +const MANAGER_CTX = ctxFor(U_MANAGER, 'crm_manager'); +const REP_CTX = ctxFor(U_REP, 'crm_rep'); +const EDIT_SHARE_CTX = ctxFor(U_EDIT_SHARE, 'crm_rep'); +const READ_SHARE_CTX = ctxFor(U_READ_SHARE, 'crm_rep'); +const WIDENED_CTX = ctxFor('u_widened', 'crm_rep_widened'); + +/** + * The refusal this issue is about, asserted as an ENVELOPE and not as "it + * threw". A bare `toThrow()` carries one bit where the defect has three: which + * gate refused, with what code, at what status. The row-level pre-image gate is + * the only place that produces this exact sentence. + */ +function expectRowLevelDenial(outcome: WriteOutcome, operation: 'update' | 'delete', object: string) { + expect(outcome.ok, `expected a refusal, got a completed ${operation}`).toBe(false); + expect(outcome.code, 'ADR-0112 error code').toBe('PERMISSION_DENIED'); + expect(outcome.status, 'ADR-0112 HTTP status').toBe(403); + expect(outcome.message).toContain( + `[Security] Access denied: not permitted to ${operation} this '${object}' record (row-level security)`, + ); +} + +const rowById = (stack: Stack, object: string, id: string) => + stack.rows(object).find((r) => r.id === id); + +// ─────────────────────────────────────────────────────────────────────────── + +describe('[#5492] `modifyAllRecords` widens the row-level write gate (profile axis)', () => { + let stack: Stack; + beforeEach(async () => { stack = await makeStack(); }); + + it('cross-owner UPDATE succeeds and the row really changes', async () => { + const out = await stack.write('update', 'crm_opportunity', OPP_THEIRS.id, MANAGER_CTX); + expect(out, out.message).toMatchObject({ ok: true }); + expect(rowById(stack, 'crm_opportunity', OPP_THEIRS.id)?.next_step).toBe('updated'); + }); + + it('cross-owner DELETE succeeds and the row is really gone (MODIFY_ALL_WRITE_KEYS covers delete)', async () => { + const out = await stack.write('delete', 'crm_opportunity', OPP_THEIRS.id, MANAGER_CTX); + expect(out, out.message).toMatchObject({ ok: true }); + expect(rowById(stack, 'crm_opportunity', OPP_THEIRS.id)).toBeUndefined(); + }); + + it('the widening is the SHARING authority answering `allow`, not this side recomputing it', async () => { + // The composition delegates; it never re-derives owner/depth/share/bypass. + // If these two verdicts ever stop being `allow`, the two cases above are + // passing for some other reason and the delegation has been bypassed. + await expect( + stack.sharing.checkEdit('crm_opportunity', OPP_THEIRS.id, MANAGER_CTX as any), + ).resolves.toBe('allow'); + await expect( + stack.sharing.checkDelete('crm_opportunity', OPP_THEIRS.id, MANAGER_CTX as any), + ).resolves.toBe('allow'); + }); +}); + +describe("[#5492] an `edit`-level `sys_record_share` widens UPDATE only (share axis, ADR-0111 D3)", () => { + let stack: Stack; + beforeEach(async () => { stack = await makeStack(); }); + + it('PATCH by the edit-share target succeeds on the shared record', async () => { + const out = await stack.write('update', 'crm_opportunity', OPP_THEIRS.id, EDIT_SHARE_CTX); + expect(out, out.message).toMatchObject({ ok: true }); + expect(rowById(stack, 'crm_opportunity', OPP_THEIRS.id)?.next_step).toBe('updated'); + }); + + it('DELETE by the same edit-share target is still REFUSED — a share widens rows, never verbs', async () => { + const out = await stack.write('delete', 'crm_opportunity', OPP_THEIRS.id, EDIT_SHARE_CTX); + expectRowLevelDenial(out, 'delete', 'crm_opportunity'); + expect(rowById(stack, 'crm_opportunity', OPP_THEIRS.id), 'row survives').toBeDefined(); + // Refused by the ROW gate, not by a missing CRUD bit: the profile grants delete. + expect((CRM_REP.objects as any).crm_opportunity.allowDelete).toBe(true); + await expect( + stack.sharing.checkDelete('crm_opportunity', OPP_THEIRS.id, EDIT_SHARE_CTX as any), + ).resolves.toBe('deny'); + }); + + it('a READ-level share target is still REFUSED an UPDATE (the guarded surface may not shrink)', async () => { + const out = await stack.write('update', 'crm_opportunity', OPP_THEIRS.id, READ_SHARE_CTX); + expectRowLevelDenial(out, 'update', 'crm_opportunity'); + expect(rowById(stack, 'crm_opportunity', OPP_THEIRS.id)?.next_step).toBe('call'); + }); + + it('an unrelated member with no share and no bypass is still REFUSED', async () => { + const out = await stack.write('update', 'crm_opportunity', OPP_THEIRS.id, REP_CTX); + expectRowLevelDenial(out, 'update', 'crm_opportunity'); + expect(rowById(stack, 'crm_opportunity', OPP_THEIRS.id)?.next_step).toBe('call'); + }); +}); + +describe('[#5492] the platform ownership floor still stands where nothing replaces it', () => { + let stack: Stack; + beforeEach(async () => { stack = await makeStack(); }); + + it("a member's own record is still writable (the floor admits its owner, unchanged)", async () => { + const out = await stack.write('update', 'crm_opportunity', OPP_MINE.id, REP_CTX); + expect(out, out.message).toMatchObject({ ok: true }); + expect(rowById(stack, 'crm_opportunity', OPP_MINE.id)?.next_step).toBe('updated'); + }); + + it('[E2] on an object with NO owner field the floor is the only gate, and it holds', async () => { + // `checkEdit` ABSTAINS here (record sharing does not enforce on an + // owner-less object). #5492's E2 experiment let an abstention count as + // permission and this exact write turned 403 into 200. It must stay 403. + await expect( + stack.sharing.checkEdit('crm_note', NOTE_THEIRS.id, REP_CTX as any), + ).resolves.toBe('abstain'); + const out = await stack.write('update', 'crm_note', NOTE_THEIRS.id, REP_CTX); + expectRowLevelDenial(out, 'update', 'crm_note'); + expect(rowById(stack, 'crm_note', NOTE_THEIRS.id)?.body).toBe('theirs'); + }); + + it('[E2] an abstention does not become permission for a Modify-All holder either', async () => { + // Deliberate and measured, not an oversight: an `abstain` is "record + // sharing does not enforce on this row", so the composition has no declared + // authority to promote and the floor stays. ADR-0066 ① withholds the + // superuser RLS short-circuit on an ordinary business posture, so a + // Modify-All holder is still bounded by `created_by` on an owner-less + // object. Widening THIS cell would require the security side to re-derive + // the bypass itself — the second implementation this composition removes. + await expect( + stack.sharing.checkEdit('crm_note', NOTE_THEIRS.id, MANAGER_CTX as any), + ).resolves.toBe('abstain'); + const out = await stack.write('update', 'crm_note', NOTE_THEIRS.id, MANAGER_CTX); + expectRowLevelDenial(out, 'update', 'crm_note'); + }); +}); + +describe('[#5493 control] the sharing middleware still refuses on its own — unchanged by this PR', () => { + let stack: Stack; + beforeEach(async () => { stack = await makeStack(); }); + + it('an APP-AUTHORED RLS update-widener passes the security gate and is still refused by sharing', async () => { + // #5493 is this composition's mirror image: there the RLS layer admits and + // the SHARING middleware answers FORBIDDEN first. This case reproduces that + // shape so the answer to "did #5492's fix change #5493?" is measured rather + // than reasoned: the app policy `stage == 'prospecting'` OR-combines past the + // platform ownership floor, so the security pre-image gate admits the row — + // and the write is still refused, by the other authority, exactly as before. + await expect( + stack.sharing.checkEdit('crm_opportunity', OPP_THEIRS.id, WIDENED_CTX as any), + ).resolves.toBe('deny'); + + const out = await stack.write('update', 'crm_opportunity', OPP_THEIRS.id, WIDENED_CTX); + expect(out.ok, 'still refused').toBe(false); + // The refusal is NOT the row-level pre-image gate's — that one admitted. + // Naming which authority refused is the whole point of the control. + expect(out.message, 'refused by the sharing middleware, not the RLS pre-image gate').not.toContain( + '(row-level security)', + ); + // …and POSITIVELY the other authority's envelope, so this case cannot pass + // by refusing for some third reason (a missing CRUD bit, a thrown probe): + // FORBIDDEN: insufficient privileges to update crm_opportunity opp_theirs + expect(out.code, "the sharing middleware's own code").toBe('FORBIDDEN'); + expect(out.message).toContain('insufficient privileges to update crm_opportunity'); + expect(rowById(stack, 'crm_opportunity', OPP_THEIRS.id)?.next_step).toBe('call'); + }); +}); diff --git a/packages/plugins/plugin-security/src/security-plugin.ts b/packages/plugins/plugin-security/src/security-plugin.ts index d6f5e19857..05c97e2089 100644 --- a/packages/plugins/plugin-security/src/security-plugin.ts +++ b/packages/plugins/plugin-security/src/security-plugin.ts @@ -44,6 +44,7 @@ import { bootstrapDeclaredCapabilities } from './bootstrap-declared-capabilities import { RLSCompiler, RLS_DENY_FILTER } from './rls-compiler.js'; import { computeTenantLayer0Filter, andComposeLayers } from './tenant-layer.js'; import { isPlatformTenantPolicy, isAuthoredTenantPolicy } from './platform-tenant-policies.js'; +import { isPlatformOwnershipFloorPolicy } from './platform-ownership-policies.js'; import { normalizeTenancyPosture, postureEnforcesWall, @@ -54,6 +55,7 @@ import { RESERVED_RLS_MEMBERSHIP_KEYS, type IRlsMembershipResolver, type ISecurityService, + type SharingWriteVerdict, } from '@objectstack/spec/contracts'; import { matchesFilterCondition } from '@objectstack/formula'; import { FieldMasker } from './field-masker.js'; @@ -170,6 +172,30 @@ const EMPTY_REQUIRED_PERMISSIONS: NormalizedRequiredPermissions = Object.freeze( all: [], read: [], create: [], update: [], delete: [], }) as NormalizedRequiredPermissions; +/** + * [#5492] Knobs on the layered RLS computation. Exactly one today, and it is a + * COMPOSITION instruction rather than a policy switch: the caller has already + * consulted the authority that owns the write-widening mechanisms and is telling + * this layer whose answer wins. + */ +interface RlsFilterOptions { + /** + * Drop the PLATFORM's own row-level write ownership floor + * (`owner_only_writes` / `owner_only_deletes` — see + * `platform-ownership-policies.ts`) from Layer 1. + * + * Set ONLY by the by-id write pre-image gate, and only when + * `ISharingService.checkEdit` / `checkDelete` answered `allow` — a positive + * basis (ownership at write DEPTH, an `edit`-level `sys_record_share`, or the + * `modifyAllRecords` bypass). `abstain` and `deny` both leave it in place, so + * a row record sharing does not enforce on keeps the floor as its only + * row-level write gate. + * + * Never affects Layer 0 (the tenant wall) or any app-authored policy. + */ + dropPlatformOwnershipFloor?: boolean; +} + /** * [ADR-0066 / #2918] Provenance spec for the platform/application asset objects * whose managed rows are write-protected by {@link SecurityPlugin.assertSystemRowWriteGate}. @@ -1151,6 +1177,46 @@ export class SecurityPlugin implements Plugin { // RLS-hidden → deny. When `computeRlsFilter` returns `null` (no policy // applies — e.g. an admin set with no RLS, or `modifyAllRecords`) the // check is skipped and behaviour is unchanged. + // + // [#5492] The filter is composed BY PROVENANCE. Two of the policies that + // can land in it are the platform's OWN ownership floor + // (`owner_only_writes` / `owner_only_deletes`, `created_by == + // current_user.id` — see `platform-ownership-policies.ts`), and that floor + // is a SECOND implementation of ownership: the one blind to every widening + // mechanism the platform also declares (write DEPTH, an `edit`-level + // `sys_record_share`, the `modifyAllRecords` bypass). Running it as an + // unconditional AND made all three inert — a manager holding Modify All + // Data and a share target holding `access_level: 'edit'` both got 403 on + // every row they did not personally create. + // + // So the floor now DEFERS to the authority that owns those wideners: + // `ISharingService`'s tri-state write verdict (#6428). + // + // allow → the declared authority REPLACES the floor (it has a positive + // basis: ownership at DEPTH, an `edit` share, or the bypass). + // abstain → record sharing does not enforce on this row at all (public + // object, no owner field, platform internal). The floor is the + // ONLY row-level write gate such objects have, so it STAYS — + // #5492's E2 experiment measured what collapsing this into + // "permitted" costs: a member's cross-creator UPDATE on an + // `owner_id`-less object turned 403 into 200. + // deny → the floor stays. The refusal itself belongs to the sharing + // middleware that produced the verdict; re-raising it here + // would be the duplicate implementation this composition + // exists to remove, and could only ever narrow a surface the + // ruling says may not shrink. + // + // Layer 0 (the tenant wall) and every APP-AUTHORED policy are untouched by + // the replacement — a declared security property stays declared (ADR-0049). + // Note what this is NOT: `modifyAllRecords` still does not bypass + // write-side RLS on an ordinary business posture (ADR-0066 ① is intact). + // The platform's own floor defers to the platform's own ownership + // authority; app-authored policies keep refusing exactly as before. + // + // The verb boundary is INHERITED, not restated (ADR-0111 D3): the same + // `rlsOperation` mapping picks `checkEdit` for the update class and + // `checkDelete` for the delete class, so an `edit` share widens update and + // still leaves delete denied without this file knowing why. if ( // update/delete today; transfer/restore/purge are pre-wired (#1883) so // the M2 ops inherit the pre-image check the moment they dispatch — @@ -1170,11 +1236,39 @@ export class SecurityPlugin implements Plugin { opCtx.operation === 'purge' ? 'delete' : opCtx.operation === 'transfer' || opCtx.operation === 'restore' ? 'update' : opCtx.operation; + // [#5492] Ask the write authority ONLY when the platform floor is + // actually in play for this (principal, object, operation) — no floor, + // nothing to replace, and no reason to spend a sharing probe. + // + // [ADR-0090 D10] The on-behalf-of path is deliberately EXCLUDED. The + // bypass predicate the verdict folds through already fails closed for a + // delegated context (`hasWriteBypass`: "no D10 delegator intersection + // on this path", ADR-0111 D2), so composing here could only produce a + // verdict resolved against the wrong identity. The delegated write + // keeps both principals' floors, exactly as before. + const floorApplies = + !delegatorSets && + this.collectRLSPolicies( + permissionSets, + opCtx.object, + rlsOperation, + (opCtx.context?.positions ?? []) as string[], + ).some(isPlatformOwnershipFloorPolicy); + const dropPlatformOwnershipFloor = floorApplies + ? (await this.resolveSharingWriteVerdict( + rlsOperation, + opCtx.object, + String(targetId), + opCtx.context, + permissionSets, + )) === 'allow' + : false; const writeFilter = await this.computeRlsFilter( permissionSets, opCtx.object, rlsOperation, opCtx.context, + { dropPlatformOwnershipFloor }, ); // [ADR-0090 D10] The target row must satisfy BOTH principals' write // RLS — a by-id write on behalf of a user may only touch rows that @@ -2383,27 +2477,7 @@ export class SecurityPlugin implements Plugin { | { canEdit?: (o: string, id: string, c: any) => Promise } | undefined; if (!sharing || typeof sharing.canEdit !== 'function') return true; - // ADR-0057 D1 depth stash, resolved for THIS object — the context may still - // carry the DETAIL's `__writeScope` from the middleware, and the master's - // own grant is what widens the master's owner-match. Always written (even as - // undefined) so the detail's value can never leak in through the spread. - let writeScope: string | undefined; - try { - const permissionSets = resolvedSets ?? (await this.resolvePermissionSetsForContext(context)); - if (permissionSets.length > 0) { - const meta = await this.getObjectSecurityMeta(object); - writeScope = this.permissionEvaluator.getEffectiveScope( - 'write', - object, - permissionSets, - { isPrivate: meta.isPrivate }, - ); - } - } catch { - // Depth is a WIDENING input: unresolved leaves the owner-match at its - // narrowest ('own'), the safe direction. The gate below still runs. - writeScope = undefined; - } + const writeScope = await this.resolveWriteScopeForSharing(object, context, resolvedSets); try { return ( (await sharing.canEdit(object, recordId, { ...context, __writeScope: writeScope })) === true @@ -2419,6 +2493,105 @@ export class SecurityPlugin implements Plugin { } } + /** + * [ADR-0057 D1] The write-DEPTH stash both sharing write probes hand the + * service, resolved for THIS object. + * + * The context may still carry another object's `__writeScope` from the + * middleware (the DETAIL's, when the caller is deriving a verdict about the + * MASTER), and it is the probed object's own grant that widens the probed + * object's owner-match. Always returned (even as `undefined`) so the caller + * can write the key unconditionally and a stale value can never leak in + * through a spread. + * + * Depth is a WIDENING input, so an unresolved one leaves the owner-match at + * its narrowest (`own`) — the safe direction — and the gate still runs. + * + * Extracted so {@link resolveSharingCanEdit} and + * {@link resolveSharingWriteVerdict} cannot drift on it: they are two forms of + * one question and must feed the service the same depth. + */ + private async resolveWriteScopeForSharing( + object: string, + context: any, + resolvedSets?: PermissionSet[], + ): Promise { + try { + const permissionSets = resolvedSets ?? (await this.resolvePermissionSetsForContext(context)); + if (permissionSets.length === 0) return undefined; + const meta = await this.getObjectSecurityMeta(object); + return this.permissionEvaluator.getEffectiveScope( + 'write', + object, + permissionSets, + { isPrivate: meta.isPrivate }, + ); + } catch { + return undefined; + } + } + + /** + * [#5492 / #6428] The TRI-STATE write verdict for a single row — the form the + * by-id write pre-image gate composes with, and the reason it does not need a + * second implementation of ownership. + * + * `ISharingService` is the one authority that reads all three declared + * write-widening mechanisms (ownership at write DEPTH, an `edit`-level + * `sys_record_share`, the `modifyAllRecords` bypass). Asking it here — rather + * than recomputing owner / depth / share / bypass on this side — is what keeps + * the ADR-0111 D3 verb boundary INHERITED: `update` asks `checkEdit`, `delete` + * asks `checkDelete`, and the fact that an `edit` share widens update but not + * delete lives in exactly one place. + * + * Why the tri-state and not `canEdit()`'s boolean: that projection collapses + * `allow` and `abstain` into one `true`, which is correct for a caller that + * only ADDS a gate and a **measured fail-open** for one that lets the answer + * override another authority's floor — #5492's E2 experiment turned an + * ordinary member's cross-creator UPDATE on an `owner_id`-less object from 403 + * into 200 that way. + * + * Fail-closed shape: + * - no plugin-sharing / no tri-state method → `abstain`. Nothing has been + * consulted, so nothing may replace the floor; behaviour is unchanged from + * a deployment without the sharing plugin. + * - an unrecognised answer (an older two-state implementation registered + * under this name) → `abstain`, for the same reason. + * - a probe that THROWS → `deny` (logged). The service itself already denies + * on an unresolvable lookup (#6428); this covers the call failing outright. + * Both leave the floor standing, which is the non-widening direction. + */ + private async resolveSharingWriteVerdict( + rlsOperation: string, + object: string, + recordId: string, + context: any, + resolvedSets?: PermissionSet[], + ): Promise { + const method = rlsOperation === 'delete' ? 'checkDelete' : 'checkEdit'; + const sharing = this.resolveKernelService?.('sharing') as + | Record Promise) | undefined> + | undefined; + const probe = sharing?.[method]; + if (typeof probe !== 'function') return 'abstain'; + const writeScope = await this.resolveWriteScopeForSharing(object, context, resolvedSets); + try { + const verdict = await probe.call(sharing, object, recordId, { + ...context, + __writeScope: writeScope, + }); + return verdict === 'allow' || verdict === 'deny' ? verdict : 'abstain'; + } catch (e) { + this.logger.error?.( + `[security] the row-level write gate could not resolve the sharing (${method}) verdict ` + + `for '${object}' record '${recordId}' (user ${context?.userId ?? 'unknown'}) — keeping ` + + `the platform ownership floor (fail-closed, #5492)`, + e instanceof Error ? e : new Error(String(e)), + ); + return 'deny'; + } + } + /** * The read scope for `object` under `context` — the filter the analytics / * raw-SQL path ANDs into its query, being the one surface that bypasses the @@ -3122,8 +3295,9 @@ export class SecurityPlugin implements Plugin { object: string, operation: string, context: any, + opts?: RlsFilterOptions, ): Promise | null> { - const { layer0, layer1 } = await this.computeLayeredRlsFilter(permissionSets, object, operation, context); + const { layer0, layer1 } = await this.computeLayeredRlsFilter(permissionSets, object, operation, context, opts); return andComposeLayers(layer0, layer1); } @@ -3152,6 +3326,7 @@ export class SecurityPlugin implements Plugin { object: string, operation: string, context: any, + opts?: RlsFilterOptions, ): Promise<{ layer0: Record | null; layer1: Record | null }> { // [ADR-0095 D1] Effective filter = Layer0(tenant) AND Layer1(business RLS). // The two are computed independently and never share a compiler, a merge @@ -3215,7 +3390,18 @@ export class SecurityPlugin implements Plugin { // longer skip the tenant wall (that is Layer 0's own exemption, below). let layer1: Record | null = null; if (!(posturePermits && superuserBypass)) { - const allRlsPolicies = this.collectRLSPolicies(permissionSets, object, operation, (context?.positions ?? []) as string[]); + const collected = this.collectRLSPolicies(permissionSets, object, operation, (context?.positions ?? []) as string[]); + // [#5492] Provenance composition: the caller (the by-id write pre-image + // gate) has already asked the declared write authority — `ISharingService` + // — and received a positive `allow`. Its answer REPLACES the platform's own + // ownership floor, which is the widener-blind second implementation of the + // same "ownership" contract. Only the PLATFORM's floor is replaceable; an + // app-authored policy — even one spelling the identical predicate under a + // different name — reaches the compiler untouched (ADR-0049, and the + // ADR-0105 F1 lesson that a token match swallows authored policies). + const allRlsPolicies = opts?.dropPlatformOwnershipFloor + ? collected.filter((p) => !isPlatformOwnershipFloorPolicy(p)) + : collected; if (allRlsPolicies.length > 0) { // Field-existence safety: a wildcard policy targeting a column the object // lacks is a *deny* contribution (fail-closed), unless the object opted diff --git a/packages/qa/dogfood/test/fixtures/attachments-fixture.ts b/packages/qa/dogfood/test/fixtures/attachments-fixture.ts index 6b1038f5e3..6eac553d34 100644 --- a/packages/qa/dogfood/test/fixtures/attachments-fixture.ts +++ b/packages/qa/dogfood/test/fixtures/attachments-fixture.ts @@ -16,10 +16,11 @@ // user's attachments on it). // att_nofiles — NO enable.files: the #2727 opt-in gate (FILES_DISABLED). // -// No custom SecurityPlugin: a fresh signUp member falls back to the real -// `member_default` wildcard-CRUD set — exactly the posture the issue's -// permission matrix questions (the gates under test are the ones layered on -// TOP of that wildcard). +// A fresh signUp member falls back to the fixture's OWN permissive baseline +// (`att_fixture_baseline` below) — exactly the posture the issue's permission +// matrix questions (the gates under test are the ones layered on TOP of that +// wildcard). Before #5491 the same posture arrived implicitly from the +// platform's `member_default`; it is now declared here. import { defineStack } from '@objectstack/spec'; import { ObjectSchema, Field } from '@objectstack/spec/data'; @@ -91,9 +92,40 @@ export const attachmentManagerSet: PermissionSet = PermissionSetSchema.parse({ }); /** SecurityPlugin carrying the platform defaults + the fixture's domain set. */ + +/** + * [#5491] The permissive baseline this matrix runs on, now DECLARED by the + * fixture instead of inherited from the platform. + * + * Until #5491 the platform's `member_default` shipped + * `objects['*'] = {allowRead, allowCreate, allowEdit}` and union-merged it into + * every authenticated member, so this fixture got the posture for free — and the + * header above says so in as many words. That wildcard is exactly what the + * maintainer removed (2026-08-07): as a PLATFORM baseline it erased app-declared + * explicit-allow gates on three axes. + * + * Re-declaring it HERE is not the same thing and is not a re-opening. It is one + * fixture modelling one deliberately permissive app, which is the posture the + * matrix questions: every gate under test is layered ON TOP of a wildcard that + * by itself scopes nothing, so removing the wildcard would delete the matrix's + * premise rather than migrate it. `allowDelete` stays absent — the delete bit is + * what the domain set below adds, and that contrast is half of what the matrix + * measures. + */ +export const attFixtureBaselineSet: PermissionSet = PermissionSetSchema.parse({ + name: 'att_fixture_baseline', + label: 'Fixture baseline — the wildcard posture this matrix questions', + objects: { + '*': { allowRead: true, allowCreate: true, allowEdit: true }, + }, +}); + export function attachmentsFixtureSecurity(): SecurityPlugin { return new SecurityPlugin({ - defaultPermissionSets: [...securityDefaultPermissionSets, attachmentManagerSet], + defaultPermissionSets: [...securityDefaultPermissionSets, attFixtureBaselineSet, attachmentManagerSet], + // [#5491] The platform baseline no longer grants objects, so the fixture's + // own permissive baseline is what a grant-less signUp member falls back to. + fallbackPermissionSet: attFixtureBaselineSet.name, }); } diff --git a/packages/qa/dogfood/test/fixtures/comments-fixture.ts b/packages/qa/dogfood/test/fixtures/comments-fixture.ts index 7527d07aae..f3e0924e9d 100644 --- a/packages/qa/dogfood/test/fixtures/comments-fixture.ts +++ b/packages/qa/dogfood/test/fixtures/comments-fixture.ts @@ -21,10 +21,11 @@ // capability gate, which is ORTHOGONAL to this authorization // and must keep behaving exactly as before. // -// No custom SecurityPlugin beyond the platform defaults: a fresh signUp member -// falls back to the real `member_default` wildcard-CRUD set — exactly the -// posture #4630 reported against (the gates under test are the ones layered on -// TOP of that wildcard, which by itself scopes nothing). +// A fresh signUp member falls back to the fixture's OWN permissive baseline +// (`cmt_fixture_baseline` below) — exactly the posture #4630 reported against +// (the gates under test are the ones layered on TOP of that wildcard, which by +// itself scopes nothing). Before #5491 the same posture arrived implicitly from +// the platform's `member_default`; it is now declared here. import { defineStack } from '@objectstack/spec'; import { ObjectSchema, Field } from '@objectstack/spec/data'; @@ -90,9 +91,40 @@ export const commentManagerSet: PermissionSet = PermissionSetSchema.parse({ }); /** SecurityPlugin carrying the platform defaults + the fixture's domain set. */ + +/** + * [#5491] The permissive baseline this matrix runs on, now DECLARED by the + * fixture instead of inherited from the platform. + * + * Until #5491 the platform's `member_default` shipped + * `objects['*'] = {allowRead, allowCreate, allowEdit}` and union-merged it into + * every authenticated member, so this fixture got the posture for free — and the + * header above says so in as many words. That wildcard is exactly what the + * maintainer removed (2026-08-07): as a PLATFORM baseline it erased app-declared + * explicit-allow gates on three axes. + * + * Re-declaring it HERE is not the same thing and is not a re-opening. It is one + * fixture modelling one deliberately permissive app, which is the posture the + * matrix questions: every gate under test is layered ON TOP of a wildcard that + * by itself scopes nothing, so removing the wildcard would delete the matrix's + * premise rather than migrate it. `allowDelete` stays absent — the delete bit is + * what the domain set below adds, and that contrast is half of what the matrix + * measures. + */ +export const cmtFixtureBaselineSet: PermissionSet = PermissionSetSchema.parse({ + name: 'cmt_fixture_baseline', + label: 'Fixture baseline — the wildcard posture this matrix questions', + objects: { + '*': { allowRead: true, allowCreate: true, allowEdit: true }, + }, +}); + export function commentsFixtureSecurity(): SecurityPlugin { return new SecurityPlugin({ - defaultPermissionSets: [...securityDefaultPermissionSets, commentManagerSet], + defaultPermissionSets: [...securityDefaultPermissionSets, cmtFixtureBaselineSet, commentManagerSet], + // [#5491] The platform baseline no longer grants objects, so the fixture's + // own permissive baseline is what a grant-less signUp member falls back to. + fallbackPermissionSet: cmtFixtureBaselineSet.name, }); } diff --git a/packages/qa/dogfood/test/me-apps-and-everyone-baseline.dogfood.test.ts b/packages/qa/dogfood/test/me-apps-and-everyone-baseline.dogfood.test.ts index a06254babc..e2e7542c6c 100644 --- a/packages/qa/dogfood/test/me-apps-and-everyone-baseline.dogfood.test.ts +++ b/packages/qa/dogfood/test/me-apps-and-everyone-baseline.dogfood.test.ts @@ -13,9 +13,18 @@ // `allowDelete` on `'*'`, so the bootstrap REFUSED to bind it to the // `everyone` position on every boot and the baseline flowed only through the // separate fallback channel (the "second distribution channel" D5 rejected). -// The wildcard is now delete-free (anchor-safe per the D5 bit list), the -// everyone binding succeeds, and deleting records is no longer a baseline -// right. +// The baseline is now anchor-safe per the D5 bit list, the everyone binding +// succeeds, and deleting records is no longer a baseline right. +// +// [#5491] The `'*'` wildcard that carried those bits is GONE entirely — not +// just its delete bit. It union-merged into every org member and erased +// app-declared explicit-allow gates on create/read/edit, so the maintainer +// narrowed the baseline to explicit-allow (2026-08-07). The two properties +// above are unaffected — the everyone binding is about the SET, not its +// grants — but the delete case below can no longer borrow its create right +// from the wildcard and now declares it. Anchor-safety of the shipped set is +// pinned statically in `plugin-security`'s `member-default-explicit-allow` +// suite; this file pins that the BINDING actually happens at bootstrap. // // @proof: me-apps-and-everyone-baseline @@ -32,6 +41,11 @@ describe('ADR-0090 D5 closures: /me/apps + anchor-bindable baseline', () => { let memberTok: string; beforeAll(async () => { + // Deliberately VANILLA: `member_default` must stay the CONFIGURED baseline, + // because the #2753 assertion below is precisely that the bootstrap binds + // THAT set to the `everyone` anchor. The #5491 consequence — the baseline no + // longer grants objects — is handled where it bites, by declaring the probe's + // create grant instead of inheriting it from a wildcard (see the delete case). stack = await bootStack(showcaseStack); adminTok = await stack.signIn(); memberTok = await stack.signUp('baseline-member@verify.test'); @@ -139,13 +153,46 @@ describe('ADR-0090 D5 closures: /me/apps + anchor-bindable baseline', () => { }); it('deleting records is no longer a baseline right (anchor-forbidden bit removed)', async () => { - // The member can still create their own record… + // [#5491] Where the create right comes from changed, and that is the point. + // It used to come from `member_default`'s `'*'` grant — the baseline handed + // every member create/read/edit on EVERY object, which is what erased + // app-declared explicit-allow gates and what the maintainer removed. So the + // create bit is now DECLARED, on one named object, and bound to this member + // the same way the tab probes above bind theirs. + // + // The delete assertion is untouched and is still ADR-0090 D5's property: the + // declared set deliberately carries no `allowDelete`, and the baseline may + // not carry one (anchor-forbidden bit) — so a member's own record is still + // undeletable. What this case can no longer be accused of is passing because + // some wildcard happened to grant create. + const memberUser = await ql.findOne('sys_user', { where: { email: 'baseline-member@verify.test' }, context: SYS }); + expect(memberUser?.id, 'baseline member resolved').toBeTruthy(); + const inquirySet = await ql.insert( + 'sys_permission_set', + { + name: 'baseline_inquiry_probe', + label: 'Baseline probe — inquiry create, deliberately no delete', + object_permissions: JSON.stringify({ + showcase_inquiry: { allowRead: true, allowCreate: true }, + }), + }, + { context: SYS }, + ); + const inquirySetId = inquirySet?.id + ?? (await ql.findOne('sys_permission_set', { where: { name: 'baseline_inquiry_probe' }, context: SYS }))?.id; + expect(inquirySetId, 'probe set stored').toBeTruthy(); + await ql.insert( + 'sys_user_permission_set', + { user_id: memberUser.id, permission_set_id: inquirySetId }, + { context: SYS }, + ); + const created = await stack.apiAs(memberTok, 'POST', '/data/showcase_inquiry', { name: 'Baseline Probe', email: 'baseline-probe@verify.test', message: 'delete-bit probe', }); - expect(created.status, 'baseline create still works').toBeLessThan(300); + expect(created.status, 'the DECLARED create grant works').toBeLessThan(300); const body: any = await created.json(); const id = body?.id ?? body?.record?.id; expect(id).toBeTruthy(); diff --git a/packages/qa/dogfood/test/shared-showcase.ts b/packages/qa/dogfood/test/shared-showcase.ts index 550b391dc4..b761722edc 100644 --- a/packages/qa/dogfood/test/shared-showcase.ts +++ b/packages/qa/dogfood/test/shared-showcase.ts @@ -15,9 +15,32 @@ // SQLite, so no explicit stop() is needed (and shared files must NOT call // stack.stop() — that would kill the instance under the worker's later files). // +// [#5491] SECURITY WIRING — the shared stack boots the showcase the way the CLI +// boots it: the app's own `isDefault` profile (`showcase_member_default`) as the +// additive per-request baseline, resolved by NAME through +// `appDefaultPermissionSetName` exactly as `objectstack dev` does (ADR-0056 D7 / +// ADR-0090 D5). `showcase-d7-default-profile.dogfood.test.ts` pins that wiring +// itself; this file now USES it. +// +// It used to boot a vanilla `new SecurityPlugin()`, which meant a fresh sign-up +// was governed by the PLATFORM baseline — and the platform baseline used to ship +// a `'*'` object grant that handed every authenticated user create/read/edit on +// every object. That wildcard was the #5491 defect (it erased app-declared +// explicit-allow gates on three axes; HotCRM measured 21/21 create-denial probes +// returning 201), and it is gone. A vanilla boot therefore no longer models the +// showcase — it models a deployment that declared no default profile at all, in +// which a member correctly has no object access and every fixture below would be +// asserting against 403s. +// +// The showcase's declared default is not a workaround for that: it is what the +// app actually ships (`examples/app-showcase/src/security/permission-sets.ts`), +// and what a real `pnpm dev:showcase` puts in force. Naming it here makes these +// fixtures MORE faithful to the running app than they were before, not less. +// // ELIGIBILITY — a file may use the shared stack ONLY if all of these hold: -// 1. It boots `bootStack(showcaseStack)` or `bootStack(showcaseStack, {})` — -// no custom security/plugins/automation/multiTenant options. +// 1. It boots the showcase with the app-default security wiring below and +// nothing else — no further custom security/plugins/automation/multiTenant +// options. // 2. It does not mutate shared global surfaces: no /meta writes, no // organization/business-unit creation, no permission-set metadata edits. // 3. Its assertions tolerate other files' rows existing in shared objects: @@ -27,12 +50,38 @@ // Anything else stays in the `isolated` project (plain vitest defaults). import showcaseStack from '@objectstack/example-showcase'; import { bootStack, type VerifyStack } from '@objectstack/verify'; +import { showcaseAppDefaultSecurity } from './showcase-security.js'; + +/** + * [#5491] The two scaffolding grants these shared fixtures add on top of the + * app's declared default — named objects, never a wildcard. + * + * Both are SCAFFOLDING for a platform property, not a change to what the + * showcase declares. Each fixture pins something about the ENGINE and merely + * needs a NON-ADMIN member able to write the substrate object; before #5491 the + * platform wildcard supplied that silently. + * + * - `showcase_announcement` create/edit — `showcase-public-read-owd` proves the + * `public_read` OWD with TWO member owners. Announcements are read-only for + * members in the app's own profile, so the fixture could only ever create one + * through the wildcard. Its row-level assertion (bob may NOT edit alice's + * announcement) gets STRONGER with the CRUD bit granted: the refusal is now + * unambiguously the OWD gate rather than a missing object bit. + * - `showcase_contact` read/create/edit — `showcase-static-readonly` reproduces + * #3003, whose entire subject is "a logged-in, NON-ADMIN user forges a + * readonly column by direct REST". Handing that fixture an admin token would + * delete the property it exists to pin, so the member needs the grant. + */ +const SHARED_FIXTURE_GRANTS = { + showcase_announcement: { allowRead: true, allowCreate: true, allowEdit: true }, + showcase_contact: { allowRead: true, allowCreate: true, allowEdit: true }, +}; let booted: Promise | undefined; /** Boot (once per worker) and return the shared plain-showcase stack. */ export function getSharedShowcase(): Promise { - booted ??= bootStack(showcaseStack).then(async (stack) => { + booted ??= bootStack(showcaseStack, { security: showcaseAppDefaultSecurity(SHARED_FIXTURE_GRANTS) }).then(async (stack) => { // First sign-in provisions the dev admin; later files' own signIn() calls // are idempotent (~0.16s) and just mint fresh admin tokens. await stack.signIn(); diff --git a/packages/qa/dogfood/test/showcase-bu-hierarchy-sharing.dogfood.test.ts b/packages/qa/dogfood/test/showcase-bu-hierarchy-sharing.dogfood.test.ts index 4d73d02953..8d0ffcb8c5 100644 --- a/packages/qa/dogfood/test/showcase-bu-hierarchy-sharing.dogfood.test.ts +++ b/packages/qa/dogfood/test/showcase-bu-hierarchy-sharing.dogfood.test.ts @@ -13,6 +13,7 @@ import { describe, it, expect, beforeAll, afterAll } from 'vitest'; import showcaseStack from '@objectstack/example-showcase'; import { bootStack, type VerifyStack } from '@objectstack/verify'; +import { showcaseAppDefaultSecurity } from './showcase-security.js'; const OBJ = '/data/showcase_private_note'; const SYS = { isSystem: true } as const; @@ -24,7 +25,11 @@ describe('showcase: business-unit hierarchy sharing rule (ADR-0057 D6 / #2077)', let noteId: string; beforeAll(async () => { - stack = await bootStack(showcaseStack); + // [#5491] The platform baseline no longer ships a `'*'` object grant, so a + // grant-less sign-up can no longer create the private note this fixture + // shares. Boot the showcase the way the CLI does — under its OWN declared + // default profile, which grants `showcase_private_note` create/read/edit. + stack = await bootStack(showcaseStack, { security: showcaseAppDefaultSecurity() }); await stack.signIn(); ownerTok = await stack.signUp('bu-owner@verify.test'); mgrTok = await stack.signUp('bu-mgr@verify.test'); // parent BU diff --git a/packages/qa/dogfood/test/showcase-mcp-http-identity.dogfood.test.ts b/packages/qa/dogfood/test/showcase-mcp-http-identity.dogfood.test.ts index 3609374cb7..f88ddaff13 100644 --- a/packages/qa/dogfood/test/showcase-mcp-http-identity.dogfood.test.ts +++ b/packages/qa/dogfood/test/showcase-mcp-http-identity.dogfood.test.ts @@ -16,6 +16,7 @@ import { describe, it, expect, beforeAll, afterAll } from 'vitest'; import showcaseStack from '@objectstack/example-showcase'; import { bootStack, type VerifyStack } from '@objectstack/verify'; +import { showcaseAppDefaultSecurity } from './showcase-security.js'; import { MCPServerPlugin } from '@objectstack/mcp'; const OBJ = '/data/showcase_private_note'; @@ -65,7 +66,13 @@ describe('showcase: MCP HTTP surface is identity-admitted (ADR-0096 / #3167)', ( // needs (in production `os serve`/`dev` auto-load it via isMcpServerEnabled; // bootStack's lean harness injects it explicitly). isMcpServerEnabled() is // default-on, so the route is live. - stack = await bootStack(showcaseStack, { extraPlugins: [new MCPServerPlugin()] }); + // [#5491] Under the app's OWN declared default profile — the platform + // baseline no longer carries a `'*'` grant, so alice and bob need the + // showcase's `showcase_private_note` declaration to own a note at all. + stack = await bootStack(showcaseStack, { + extraPlugins: [new MCPServerPlugin()], + security: showcaseAppDefaultSecurity(), + }); await stack.signIn(); // seed dev admin (first user) aliceToken = await stack.signUp('mcp-alice@verify.test'); bobToken = await stack.signUp('mcp-bob@verify.test'); diff --git a/packages/qa/dogfood/test/showcase-security.ts b/packages/qa/dogfood/test/showcase-security.ts new file mode 100644 index 0000000000..93d03148df --- /dev/null +++ b/packages/qa/dogfood/test/showcase-security.ts @@ -0,0 +1,87 @@ +// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license. +// +// [#5491] The showcase's OWN default profile, wired the way the CLI wires it. +// +// Until #5491 the platform baseline (`member_default`) shipped an +// `object_permissions['*'] = {allowCreate, allowRead, allowEdit}` grant that was +// union-merged into every authenticated member. A dogfood fixture could boot a +// vanilla `new SecurityPlugin()`, sign up a grant-less member, and that member +// could create and edit anything — so "the actor can write the substrate object" +// never had to be declared anywhere. +// +// That wildcard erased app-declared explicit-allow gates on three axes (HotCRM's +// GA sweep: 21 of 21 create-DENIAL probes returned 201; `security/explain` said +// "granted by [member_default]" for a profile carrying an all-false deny), and +// the maintainer removed it (2026-08-07): the platform baseline is explicit-allow +// and object access comes from OWDs plus profile / permission-set declarations. +// +// A vanilla boot therefore no longer models the showcase — it models a +// deployment that declared no default profile, in which a member correctly has +// no object access at all. This helper models the REAL one: the app's `isDefault` +// profile, resolved by NAME through `appDefaultPermissionSetName`, exactly as +// `objectstack dev` does (ADR-0056 D7 / ADR-0090 D5). +// `showcase-d7-default-profile.dogfood.test.ts` pins that wiring; this module +// USES it, so fixtures are now more faithful to the running app than they were. +// +// The lightweight verify harness does not seed permission metadata, so the +// declared set is handed to the plugin directly — the identical note that test +// carries. +import showcaseStack from '@objectstack/example-showcase'; +import { + SecurityPlugin, + appDefaultPermissionSetName, + securityDefaultPermissionSets, +} from '@objectstack/plugin-security'; +import { PermissionSetSchema, type PermissionSet } from '@objectstack/spec/security'; + +type ObjectGrants = Record< + string, + { allowRead?: boolean; allowCreate?: boolean; allowEdit?: boolean; allowDelete?: boolean } +>; + +const stackPermissions = ((showcaseStack as { permissions?: unknown[] }).permissions ?? []) as Array<{ name?: string }>; + +/** The app's declared default profile name — `showcase_member_default`. */ +export const showcaseAppDefaultName = appDefaultPermissionSetName(stackPermissions); + +const declaredDefault = stackPermissions.find((p) => p?.name === showcaseAppDefaultName) as unknown; + +/** + * A `SecurityPlugin` whose additive per-request baseline is the showcase's own + * declared default profile. + * + * `extraObjectGrants` adds NAMED objects on top, for a fixture whose actor must + * write a substrate object the app's profile does not open. Use it only for + * scaffolding a platform property, always naming the objects — a fixture that + * re-adds a `'*'` wildcard here would re-open #5491 under a different name, and + * a grant nobody can point at is the defect this issue is about. + */ +export function showcaseAppDefaultSecurity(extraObjectGrants?: ObjectGrants): SecurityPlugin { + if (!showcaseAppDefaultName || !declaredDefault) { + // The showcase declares one. If that ever stops being true, every fixture + // built on this helper would silently degrade to "the member has no access", + // which reads as a security regression rather than a missing declaration. + throw new Error( + '[dogfood] the showcase stack declares no `isDefault` permission set — the CLI wiring ' + + 'these fixtures model cannot be reproduced (#5491)', + ); + } + const appDefault = PermissionSetSchema.parse(declaredDefault) as PermissionSet; + if (!extraObjectGrants) { + return new SecurityPlugin({ + defaultPermissionSets: [...securityDefaultPermissionSets, appDefault], + fallbackPermissionSet: appDefault.name, + }); + } + const baseline = PermissionSetSchema.parse({ + ...appDefault, + name: 'dogfood_showcase_member', + label: 'Dogfood showcase member (app default + named fixture scaffolding)', + isDefault: false, + objects: { ...(appDefault.objects ?? {}), ...extraObjectGrants }, + }) as PermissionSet; + return new SecurityPlugin({ + defaultPermissionSets: [...securityDefaultPermissionSets, baseline], + fallbackPermissionSet: baseline.name, + }); +}