From 7e63b8ddc26039d6b51f5db48510c535a3e31e32 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 05:40:03 +0000 Subject: [PATCH 1/2] fix(plugin-security): bring organization_admin_no_bypass under registry-driven managed-write denies (#14029) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The derived wall-less org-admin variant holds a write-granting '*' wildcard but was not in MANAGED_DENY_TARGET_SETS, so applyManagedWriteDenies walked it and skipped it at kernel:ready — and because deriveWallLessOrgAdmin takes a shallow copy at module load, injections into the parent could never propagate either. Add the variant to the target list, and replace the self-referential membership pin with one that derives the required floor (write-granting wildcard sets) from the real seeded sets and diffs it against the list. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016ZC5rNQj3WEet5HAmmAkMs --- .changeset/no-bypass-managed-deny-target.md | 34 ++++++ .../src/managed-object-write-denies.test.ts | 23 +++- .../src/managed-object-write-denies.ts | 43 ++++++-- .../objects/default-permission-sets.test.ts | 102 +++++++++++++++++- .../src/objects/default-permission-sets.ts | 8 ++ 5 files changed, 193 insertions(+), 17 deletions(-) create mode 100644 .changeset/no-bypass-managed-deny-target.md diff --git a/.changeset/no-bypass-managed-deny-target.md b/.changeset/no-bypass-managed-deny-target.md new file mode 100644 index 0000000000..e4337bc3b7 --- /dev/null +++ b/.changeset/no-bypass-managed-deny-target.md @@ -0,0 +1,34 @@ +--- +"@objectstack/plugin-security": patch +--- + +fix(plugin-security): registry-driven managed-object write denies now reach `organization_admin_no_bypass` (#14029) + +`MANAGED_DENY_TARGET_SETS` named four default sets, and `applyManagedWriteDenies` +matches on it exactly — so at `kernel:ready` the injection walked the derived +`organization_admin_no_bypass` variant and skipped it. The variant is a shallow +copy of `organization_admin` taken at module load (`deriveWallLessOrgAdmin` +strips only the `viewAllRecords`/`modifyAllRecords` superuser bits), which means +its `'*'` wildcard still grants create/edit/delete AND entries injected into the +parent's `objects` can never propagate to it. Its own docblock declares +"managed-write denies … carried over verbatim"; the behaviour violated that +declared contract. + +No gap opens on today's tree — the static `BETTER_AUTH_MANAGED_OBJECTS` +baseline covers the 30 declared managed tables and is copied into the variant at +derivation. The gap was the next `managedBy: 'better-auth'` schema that lands +without a hand edit to that list: `organization_admin` would receive the +injected deny while the wall-less variant's wildcard kept granting raw CRUD on +an identity table — precisely the drift the registry-driven module exists to +close (ADR-0092), on the posture (`auto-org-admin-grant` under a wall-less +deployment) where the bits are least bounded. + +- `ORGANIZATION_ADMIN_NO_BYPASS` is now a member of `MANAGED_DENY_TARGET_SETS`. + The variant's pre-existing explicit entries (static baseline, RBAC read-only + block) survive unchanged — the injection skips any object a set already + names. +- The membership pin no longer checks the list against itself: the required + floor ("default sets holding a write-granting `'*'` wildcard") is derived + from the real seeded sets and diffed against the list; a non-empty + difference is red. `admin_full_access` stays deliberately excluded (admin + rescue path) and that exclusion is pinned exactly. diff --git a/packages/plugins/plugin-security/src/managed-object-write-denies.test.ts b/packages/plugins/plugin-security/src/managed-object-write-denies.test.ts index 45b14ebb4f..31942d47a6 100644 --- a/packages/plugins/plugin-security/src/managed-object-write-denies.test.ts +++ b/packages/plugins/plugin-security/src/managed-object-write-denies.test.ts @@ -8,6 +8,7 @@ import { MANAGED_DENY_TARGET_SETS, } from './managed-object-write-denies.js'; import { MCP_AGENT_PERMISSION_SET_WRITE, MCP_AGENT_PERMISSION_SET_READ } from '@objectstack/spec/ai'; +import { ORGANIZATION_ADMIN_NO_BYPASS } from '@objectstack/spec'; // Minimal PermissionSet-shaped fixtures (only name + objects matter here). const set = (name: string, objects: Record = {}): any => ({ name, objects }); @@ -25,16 +26,17 @@ const schemas = [ ]; describe('applyManagedWriteDenies (#3325)', () => { - it('injects a read-only-write deny for every better-auth object into the four target sets', () => { + it('injects a read-only-write deny for every better-auth object into the five target sets', () => { const sets = [ set('organization_admin'), + set(ORGANIZATION_ADMIN_NO_BYPASS), // #14029 — derived at module load, so it needs its own injection set('member_default'), set('viewer_readonly'), set(MCP_AGENT_PERMISSION_SET_WRITE), ]; const res = applyManagedWriteDenies(sets, schemas); - // 2 better-auth objects × 4 sets = 8 injections. - expect(res.applied).toBe(8); + // 2 better-auth objects × 5 sets = 10 injections. + expect(res.applied).toBe(10); expect(res.skippedExisting).toBe(0); for (const s of sets) { expect(s.objects.sys_user).toEqual(DENY); @@ -100,9 +102,20 @@ describe('applyManagedWriteDenies (#3325)', () => { expect(() => applyManagedWriteDenies([{ name: 'member_default' } as any], schemas)).not.toThrow(); }); - it('the target allowlist is exactly the four write-granting sets', () => { + it('the target allowlist is exactly the five known sets (see the independent-property pin for the floor)', () => { + // Exact membership record. This is deliberately NOT the only pin on the + // list: `objects/default-permission-sets.test.ts` derives the required + // floor ("holds a write-granting '*' wildcard") from the real default sets + // and diffs it against this list, so a set that SHOULD be a member cannot + // hide behind an assertion that only restates the list (#14029). expect([...MANAGED_DENY_TARGET_SETS].sort()).toEqual( - ['member_default', 'organization_admin', 'viewer_readonly', MCP_AGENT_PERMISSION_SET_WRITE].sort(), + [ + 'member_default', + 'organization_admin', + ORGANIZATION_ADMIN_NO_BYPASS, + 'viewer_readonly', + MCP_AGENT_PERMISSION_SET_WRITE, + ].sort(), ); }); }); diff --git a/packages/plugins/plugin-security/src/managed-object-write-denies.ts b/packages/plugins/plugin-security/src/managed-object-write-denies.ts index c05f5f9c9f..4d06d8f590 100644 --- a/packages/plugins/plugin-security/src/managed-object-write-denies.ts +++ b/packages/plugins/plugin-security/src/managed-object-write-denies.ts @@ -4,12 +4,14 @@ * ADR-0092 / ADR-0103 — registry-driven managed-object write denies for the * default permission sets. * - * The default sets this module targets (`organization_admin`, `member_default`, - * `viewer_readonly`, and the MCP write set) DENY writes on the + * The default sets this module targets (`organization_admin`, its derived + * `organization_admin_no_bypass` variant, `member_default`, `viewer_readonly`, + * and the MCP write set) DENY writes on the * better-auth-managed identity tables so mutations must flow through the auth * pipeline (ADR-0092). What that entry narrows differs per set, and the - * difference is load-bearing for the rest of this file: `organization_admin` and - * the MCP write set grant CRUD through an `objects['*']` wildcard, so their + * difference is load-bearing for the rest of this file: `organization_admin`, + * its no-bypass variant and the MCP write set grant CRUD through an + * `objects['*']` wildcard, so their * managed-table entries are a narrowing overlay; `viewer_readonly`'s wildcard is * read-only, so its entries are belt-and-suspenders over a wildcard that already * denies; and `member_default` carries NO wildcard at all since #5491 (the @@ -59,6 +61,7 @@ */ import type { PermissionSet } from '@objectstack/spec/security'; +import { ORGANIZATION_ADMIN_NO_BYPASS } from '@objectstack/spec'; import { MCP_AGENT_PERMISSION_SET_WRITE } from '@objectstack/spec/ai'; /** @@ -76,16 +79,34 @@ export const MANAGED_DENY_ENTRY = { /** * The default sets that must carry an explicit entry for every managed table. - * Membership is NOT "holds a write-granting `'*'` wildcard" — only - * `organization_admin` and the MCP write set hold one; `viewer_readonly`'s - * wildcard is read-only and `member_default` has held none since #5491 (see the - * module docblock for what the injected entry does in each). Explicit allowlist - * — `admin_full_access` is deliberately excluded (it keeps its unqualified - * wildcard so an admin can rescue data directly; the runtime guards are its - * boundary), as are the MCP read / restricted sets (they grant no writes). + * + * Holding a write-granting `'*'` wildcard is the FLOOR of membership, not its + * definition: every default set whose wildcard grants create/edit/delete must + * be listed here (or carry a documented exclusion below), because the wildcard + * is what would otherwise grant raw CRUD on a newly-declared identity table — + * that floor is what `default-permission-sets.test.ts` derives independently + * and diffs against this list (#14029), so a future write-granting set that is + * not added here fails a pin instead of silently keeping its wildcard. + * Membership is WIDER than the floor: `viewer_readonly`'s wildcard is read-only + * and `member_default` has held none since #5491 — their injected entries are + * belt-and-suspenders and the read grant itself, respectively (see the module + * docblock for what the injected entry does in each). + * + * `organization_admin_no_bypass` is a member in its own right (#14029): it is + * derived from `organization_admin` by a SHALLOW copy taken at module load + * (`deriveWallLessOrgAdmin`), so entries injected into the parent's `objects` + * at `kernel:ready` can never propagate to it — dropping only the superuser + * bits leaves `allowCreate`/`allowEdit`/`allowDelete` true on its wildcard, + * exactly the shape this module exists to narrow. + * + * Documented exclusions: `admin_full_access` is deliberately NOT a member (it + * keeps its unqualified wildcard so an admin can rescue data directly; the + * runtime guards are its boundary), and the MCP read / restricted sets grant + * no writes for a deny to narrow. */ export const MANAGED_DENY_TARGET_SETS: readonly string[] = [ 'organization_admin', + ORGANIZATION_ADMIN_NO_BYPASS, 'member_default', 'viewer_readonly', MCP_AGENT_PERMISSION_SET_WRITE, diff --git a/packages/plugins/plugin-security/src/objects/default-permission-sets.test.ts b/packages/plugins/plugin-security/src/objects/default-permission-sets.test.ts index 1381ec5b16..f55f3382be 100644 --- a/packages/plugins/plugin-security/src/objects/default-permission-sets.test.ts +++ b/packages/plugins/plugin-security/src/objects/default-permission-sets.test.ts @@ -7,7 +7,7 @@ import * as PlatformObjects from '@objectstack/platform-objects'; import { PermissionSetSchema } from '@objectstack/spec/security'; import { ADMIN_FULL_ACCESS_CAPABILITIES } from '@objectstack/spec'; import { defaultPermissionSets, BETTER_AUTH_MANAGED_OBJECTS } from './default-permission-sets.js'; -import { MANAGED_DENY_TARGET_SETS } from '../managed-object-write-denies.js'; +import { applyManagedWriteDenies, MANAGED_DENY_TARGET_SETS } from '../managed-object-write-denies.js'; // Every object schema the platform-objects package exports whose bucket is // `better-auth` — the ground truth the static baseline must mirror. @@ -368,3 +368,103 @@ describe('admin_full_access imports the kernel capability declaration unchanged expect(admin.systemPermissions).toEqual(ADMIN_FULL_ACCESS_CAPABILITIES.systemPermissions); }); }); + +/** + * [#14029] The managed-deny target list, pinned against an INDEPENDENT + * property instead of against itself. + * + * The old shape of this pin iterated `MANAGED_DENY_TARGET_SETS` to assert + * membership, so it was structurally unable to see a set that SHOULD have been + * a member — which is exactly how `organization_admin_no_bypass` (a shallow + * copy of `organization_admin` taken at module load, write-granting wildcard + * intact) sat outside the list while `applyManagedWriteDenies` walked it and + * skipped it at `kernel:ready`. The floor is therefore derived here from the + * REAL seeded sets — "holds a `'*'` wildcard granting any generic write + * class" — and diffed against the list; a non-empty difference is red. + */ +describe('managed-deny targets — independent-property floor + registry union reaches the derived variant (#14029)', () => { + // Derived from `defaultPermissionSets`, never from the list under test. + const writeGrantingWildcardSets: string[] = defaultPermissionSets + .filter((s: any) => { + const wc = s.objects?.['*']; + return !!wc && (wc.allowCreate === true || wc.allowEdit === true || wc.allowDelete === true); + }) + .map((s) => s.name) + .sort(); + + /** + * The one documented exclusion: `admin_full_access` keeps its unqualified + * wildcard so an admin can rescue data directly (recorded in the + * `MANAGED_DENY_TARGET_SETS` docblock; runtime guards are its boundary). + * Pinned exactly, like `EDIT_EXCEPTIONS` above: widening it is an edit HERE, + * which is the moment a reviewer is asked why the new set may keep raw CRUD + * on identity tables. + */ + const WILDCARD_DENY_EXCLUSIONS = ['admin_full_access']; + + it('the property derivation is live (found the known write-granting sets)', () => { + // If the filter silently matched nothing, the difference below would be + // vacuously empty — guard the probe itself. + expect(writeGrantingWildcardSets).toContain('organization_admin'); + expect(writeGrantingWildcardSets).toContain('organization_admin_no_bypass'); + expect(writeGrantingWildcardSets.length).toBeGreaterThanOrEqual(3); + }); + + it('every write-granting wildcard set is a managed-deny target or a documented exclusion', () => { + const missing = writeGrantingWildcardSets.filter( + (name) => !MANAGED_DENY_TARGET_SETS.includes(name) && !WILDCARD_DENY_EXCLUSIONS.includes(name), + ); + expect(missing, 'write-granting sets missing from MANAGED_DENY_TARGET_SETS').toEqual([]); + }); + + it('the exclusion list is exactly admin_full_access, and it really is outside the target list', () => { + expect(WILDCARD_DENY_EXCLUSIONS).toEqual(['admin_full_access']); + expect([...MANAGED_DENY_TARGET_SETS]).not.toContain('admin_full_access'); + }); + + // ── The behaviour the membership buys, measured on the REAL derived set ── + // (clones so the module-level instances other tests read stay unmutated; + // the kernel path hands the same objects to the same function in place). + + const FUTURE = 'sys_future_identity_table'; + const DENY = { allowRead: true, allowCreate: false, allowEdit: false, allowDelete: false }; + + it('a managedBy:better-auth object OUTSIDE the static list now reaches the variant (the card)', () => { + const sets: any[] = structuredClone(defaultPermissionSets as any); + const variant = sets.find((s) => s.name === 'organization_admin_no_bypass'); + const parent = sets.find((s) => s.name === 'organization_admin'); + expect(variant.objects[FUTURE]).toBeUndefined(); // genuinely not in the compile-time baseline + applyManagedWriteDenies(sets, [{ name: FUTURE, managedBy: 'better-auth' }]); + expect(variant.objects[FUTURE]).toEqual(DENY); + // The fix ADDS a target; the parent keeps receiving its injection too. + expect(parent.objects[FUTURE]).toEqual(DENY); + }); + + it('reverse control: the variant pre-existing explicit entries survive the injection unchanged', () => { + const sets: any[] = structuredClone(defaultPermissionSets as any); + const variant = sets.find((s) => s.name === 'organization_admin_no_bypass'); + const before = structuredClone(variant.objects); + const registry = [ + ...BETTER_AUTH_MANAGED_OBJECTS.map((n) => ({ name: n, managedBy: 'better-auth' })), + { name: FUTURE, managedBy: 'better-auth' }, + ]; + applyManagedWriteDenies(sets, registry); + for (const name of BETTER_AUTH_MANAGED_OBJECTS) { + expect(variant.objects[name], `variant entry ${name}`).toEqual(before[name]); + } + // Wildcard and the anti-escalation RBAC read-only block untouched as well. + expect(variant.objects['*']).toEqual(before['*']); + expect(variant.objects.sys_position).toEqual(before.sys_position); + // Only the future table was new on the variant. + expect(variant.objects[FUTURE]).toEqual(DENY); + expect(Object.keys(variant.objects).sort()).toEqual([...Object.keys(before), FUTURE].sort()); + }); + + it('control: admin_full_access is untouched by the injection (admin rescue path)', () => { + const sets: any[] = structuredClone(defaultPermissionSets as any); + const admin = sets.find((s) => s.name === 'admin_full_access'); + const before = structuredClone(admin); + applyManagedWriteDenies(sets, [{ name: FUTURE, managedBy: 'better-auth' }]); + expect(admin).toEqual(before); + }); +}); 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 bca07f3b04..a700501227 100644 --- a/packages/plugins/plugin-security/src/objects/default-permission-sets.ts +++ b/packages/plugins/plugin-security/src/objects/default-permission-sets.ts @@ -1037,6 +1037,14 @@ const baseDefaultPermissionSets: PermissionSet[] = [ * silent privilege difference. The only intended delta is the superuser bits, * so the only thing this function may do is remove them. * + * ⚠️ The copy is SHALLOW and taken at MODULE LOAD, so what it carries over is + * the compile-time baseline only — the registry-driven managed-write denies + * that `applyManagedWriteDenies` injects into the parent's `objects` at + * `kernel:ready` can never propagate here. "Carried over verbatim" holds for + * the registry union because the variant is its own member of + * `MANAGED_DENY_TARGET_SETS` and receives the same injection directly + * (#14029), not because the derivation sees it. + * * `auto-org-admin-grant` picks between the two by posture: a wall-enforcing * posture bounds the bits (grant `organization_admin`); a wall-less one does * not (grant this). From 8d5cca2a8908c8154efa4abf19f2c18a50baa9a9 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 1 Sep 2026 06:50:10 +0000 Subject: [PATCH 2/2] fix(plugin-security): widen the #14029 floor to the modifyAllRecords write route (contract-review blocking item) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit The independent-property floor in default-permission-sets.test.ts recognised only the three CRUD flags, but the evaluator grants writes by a second route: MODIFY_ALL_WRITE_KEYS + objPerm.modifyAllRecords (permission-evaluator.ts:224) covers allowEdit, allowDelete and the destructive class. A future default set shaped '*': { allowRead: true, modifyAllRecords: true } therefore held edit/delete on every future identity table in the evaluator's own terms, yet had all three CRUD flags false, escaped the floor and tripped no pin — contradicting the MANAGED_DENY_TARGET_SETS docblock promise that a write-granting set not listed there fails a pin. Same "a set that should be a member can hide" class this card exists to kill, one size smaller. - default-permission-sets.test.ts: add `|| wc.modifyAllRecords === true` to the floor filter. Value test (`=== true`) kept deliberately: Zod materialises both superuser bits with .default(false) (permission.zod.ts:268), so they are present-as-false and a key-existence test would misfire today. - managed-object-write-denies.ts: align the docblock — the floor is "grants any generic write class via the three write flags OR modifyAllRecords", not "grants create/edit/delete". - .changeset: the static baseline covers 28 managed tables (BETTER_AUTH_MANAGED_OBJECTS), not 30 (non-blocking item). Zero behaviour delta on today's tree: organization_admin and admin_full_access carry all three CRUD flags true, so the derived floor set is unchanged. Measured: with a temporary '*': { allowRead: true, modifyAllRecords: true } probe set absent from MANAGED_DENY_TARGET_SETS, the widened pin goes red naming exactly the probe; the old three-flag filter stays green on the same probe (the blind spot). Probe removed; restore proven by blob hash against HEAD. plugin-security suite 94 files / 1770 tests green; all three tsc programs green. Co-Authored-By: Claude Opus 5 Claude-Session: https://claude.ai/code/session_016ZC5rNQj3WEet5HAmmAkMs --- .changeset/no-bypass-managed-deny-target.md | 2 +- .../src/managed-object-write-denies.ts | 16 ++++++++++------ .../src/objects/default-permission-sets.test.ts | 15 ++++++++++++++- 3 files changed, 25 insertions(+), 8 deletions(-) diff --git a/.changeset/no-bypass-managed-deny-target.md b/.changeset/no-bypass-managed-deny-target.md index e4337bc3b7..69627ee836 100644 --- a/.changeset/no-bypass-managed-deny-target.md +++ b/.changeset/no-bypass-managed-deny-target.md @@ -15,7 +15,7 @@ parent's `objects` can never propagate to it. Its own docblock declares declared contract. No gap opens on today's tree — the static `BETTER_AUTH_MANAGED_OBJECTS` -baseline covers the 30 declared managed tables and is copied into the variant at +baseline covers the 28 declared managed tables and is copied into the variant at derivation. The gap was the next `managedBy: 'better-auth'` schema that lands without a hand edit to that list: `organization_admin` would receive the injected deny while the wall-less variant's wildcard kept granting raw CRUD on diff --git a/packages/plugins/plugin-security/src/managed-object-write-denies.ts b/packages/plugins/plugin-security/src/managed-object-write-denies.ts index 4d06d8f590..da7a4458ff 100644 --- a/packages/plugins/plugin-security/src/managed-object-write-denies.ts +++ b/packages/plugins/plugin-security/src/managed-object-write-denies.ts @@ -81,12 +81,16 @@ export const MANAGED_DENY_ENTRY = { * The default sets that must carry an explicit entry for every managed table. * * Holding a write-granting `'*'` wildcard is the FLOOR of membership, not its - * definition: every default set whose wildcard grants create/edit/delete must - * be listed here (or carry a documented exclusion below), because the wildcard - * is what would otherwise grant raw CRUD on a newly-declared identity table — - * that floor is what `default-permission-sets.test.ts` derives independently - * and diffs against this list (#14029), so a future write-granting set that is - * not added here fails a pin instead of silently keeping its wildcard. + * definition: every default set whose wildcard grants any generic write class + * — via the three write flags (`allowCreate`/`allowEdit`/`allowDelete`) OR via + * `modifyAllRecords`, whose super-user bypass grants edit/delete and the + * destructive class by the evaluator's second route (`MODIFY_ALL_WRITE_KEYS`, + * `permission-evaluator.ts`) — must be listed here (or carry a documented + * exclusion below), because the wildcard is what would otherwise grant raw + * writes on a newly-declared identity table — that floor is what + * `default-permission-sets.test.ts` derives independently and diffs against + * this list (#14029), so a future write-granting set that is not added here + * fails a pin instead of silently keeping its wildcard. * Membership is WIDER than the floor: `viewer_readonly`'s wildcard is read-only * and `member_default` has held none since #5491 — their injected entries are * belt-and-suspenders and the read grant itself, respectively (see the module diff --git a/packages/plugins/plugin-security/src/objects/default-permission-sets.test.ts b/packages/plugins/plugin-security/src/objects/default-permission-sets.test.ts index f55f3382be..48335d7686 100644 --- a/packages/plugins/plugin-security/src/objects/default-permission-sets.test.ts +++ b/packages/plugins/plugin-security/src/objects/default-permission-sets.test.ts @@ -384,10 +384,23 @@ describe('admin_full_access imports the kernel capability declaration unchanged */ describe('managed-deny targets — independent-property floor + registry union reaches the derived variant (#14029)', () => { // Derived from `defaultPermissionSets`, never from the list under test. + // "Grants a write" in the evaluator's own terms: the three CRUD flags OR + // `modifyAllRecords` — the super-user bypass grants edit/delete and the + // destructive class by a second route (`MODIFY_ALL_WRITE_KEYS`, + // `permission-evaluator.ts`), so `'*': { modifyAllRecords: true }` is + // write-granting even with all three CRUD flags false. Value tests + // (`=== true`), not key-existence: Zod materialises the superuser bits with + // `.default(false)` (`permission.zod.ts`), so they are present-as-false. const writeGrantingWildcardSets: string[] = defaultPermissionSets .filter((s: any) => { const wc = s.objects?.['*']; - return !!wc && (wc.allowCreate === true || wc.allowEdit === true || wc.allowDelete === true); + return ( + !!wc && + (wc.allowCreate === true || + wc.allowEdit === true || + wc.allowDelete === true || + wc.modifyAllRecords === true) + ); }) .map((s) => s.name) .sort();