From 26d566ba5b67b563f7b8253c471b6962fb2c0c73 Mon Sep 17 00:00:00 2001 From: Claude Date: Tue, 14 Jul 2026 15:50:08 +0000 Subject: [PATCH] feat(plugin-security): built-in row-write guardrail for sys_position / sys_capability managed rows MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit Platform/application-published system rows (sys_position `managed_by` system/config, sys_capability `managed_by` platform/package) could be deleted or rewritten directly by a customer admin — the write went straight to the driver with no guardrail, silently breaking an app's authorization baseline. sys_permission_set already had the two-doors `assertPackageManagedWriteGate`; sys_position / sys_capability had no equivalent (ADR-0049: a provenance attribute that exists but is never enforced is exactly the gap to close). Add `assertSystemRowWriteGate`, wired into the data-write hook next to the package gate, as an unconditional data-layer boundary over both asset objects: - Refuse forging platform/package `managed_by` through the admin door (insert or update, single object or array) — closes update-to-forge. - Refuse delete / update / transfer / restore / purge on rows whose existing `managed_by` is platform/package-owned. Unlike sys_permission_set these objects have no ADR-0094 overlay write-through, so the refusal holds here rather than deferring downstream. - Admin-authored rows (`user`/∅ on sys_position, `admin` on sys_capability) are untouched — the admin fully owns those, incl. a delegate's rows in their own subtree. The gate fails closed and never depends on the caller's grants, so a superuser with modifyAllRecords cannot delete a platform position. The two objects' `managed_by` vocabularies differ, so the gate keys off a per-object provenance spec. Deny text is business-message only. No conflict with the delegated-admin gate — GOVERNED_OBJECTS excludes both objects, which govern the RBAC link tables, not the definitions. Tests: non-admin/admin × managed/admin-authored × delete/update matrix, forge + update-to-forge + bulk-array cases, filter-write probe, and the isSystem seeder bypass. Closes #2918 Co-Authored-By: Claude Opus 4.8 Claude-Session: https://claude.ai/code/session_01D5TEu5LzF1aZdE9rRaF6tV --- .changeset/system-row-write-guardrail.md | 17 ++ .../src/security-plugin.test.ts | 182 ++++++++++++++++++ .../plugin-security/src/security-plugin.ts | 152 +++++++++++++++ 3 files changed, 351 insertions(+) create mode 100644 .changeset/system-row-write-guardrail.md diff --git a/.changeset/system-row-write-guardrail.md b/.changeset/system-row-write-guardrail.md new file mode 100644 index 0000000000..48bf35b197 --- /dev/null +++ b/.changeset/system-row-write-guardrail.md @@ -0,0 +1,17 @@ +--- +'@objectstack/plugin-security': patch +--- + +内置行写护栏：`sys_position` / `sys_capability` 的平台/应用托管行不再可被客户管理员删改。 + +`sys_permission_set` 早有两道门写护栏（`assertPackageManagedWriteGate`）拦截对 package 托管行的写入，但 `sys_position` / `sys_capability` 缺失对应保护——平台/应用发布的系统岗位与能力（provenance 记录在 `managed_by`）可被管理员直接 delete / update 直达驱动，静默破坏应用的授权基线（ADR-0049：provenance 字段存在却无强制 = 正是要补的 enforcement gap）。 + +新增 **`assertSystemRowWriteGate`**（`packages/plugins/plugin-security/src/security-plugin.ts`，data-write hook 接线与 package 门同处），对这两个对象的托管行施加一道无条件的数据层边界： + +- **禁止伪造托管来源**：管理员门的 insert / update 载荷（单对象或数组）不得把 `managed_by` 盖成平台/应用值——只有携带 `isSystem` 的平台 seeder / 包发布路径可写；同时封堵 update-to-forge（把自建行改badge成托管行）。 +- **拒绝改删托管行**：对 `managed_by` 已是平台/应用值的行，`delete` / `update` / `transfer` / `restore` / `purge` 一律拒绝。与 `sys_permission_set` 不同，这两个对象没有 ADR-0094 overlay write-through，故写护栏必须在此层直接拒绝，而非下放给下游翻译。 +- **管理员自建行不受限**：`managed_by` 为 `user`/∅（sys_position）或 `admin`（sys_capability）的行完全归管理员所有（含委派管理员在自己 subtree 内的自建行）。 + +护栏 fail-closed 且不依赖调用方授权——持 `modifyAllRecords` 的超管也无法删除平台岗位。两对象的 `managed_by` 词表不同（sys_position：`system`/`config` 托管，`user`/∅ 自建；sys_capability：`platform`/`package` 托管，`admin` 自建），网关按对象分别判定。错误信息仅含业务文案（"此岗位/能力由 平台|应用包 提供，不可删除/修改"）。 + +与 delegated-admin 边界不冲突：`GOVERNED_OBJECTS` 本就不含这两个对象，委派管理仍治理 RBAC 链接表而非定义对象。 diff --git a/packages/plugins/plugin-security/src/security-plugin.test.ts b/packages/plugins/plugin-security/src/security-plugin.test.ts index 8cf070b788..c0bab76bf2 100644 --- a/packages/plugins/plugin-security/src/security-plugin.test.ts +++ b/packages/plugins/plugin-security/src/security-plugin.test.ts @@ -1215,6 +1215,188 @@ describe('SecurityPlugin', () => { ).resolves.toBeDefined(); }); }); + + // ── ADR-0066 / #2918 — built-in row-write guardrail (sys_position / sys_capability) ── + // Platform/application-managed asset rows are not the admin's to delete or + // rewrite. Unlike sys_permission_set these objects have no ADR-0094 overlay + // write-through, so the refusal must hold for update/delete at this gate. + // Matrix: non-admin/admin × managed/admin-authored × delete/update. + describe('system-row write gate (sys_position / sys_capability provenance)', () => { + const adminSet: PermissionSet = { + name: 'admin_full_access', label: 'Admin', + objects: { '*': { allowRead: true, allowCreate: true, allowEdit: true, allowDelete: true, modifyAllRecords: true } }, + } as any; + const adminCtx = { userId: 'admin1', tenantId: 'org-1', positions: [], permissions: ['admin_full_access'] }; + + const runGate = async (opCtx: any, findOneImpl?: (q: any) => any) => { + const plugin = new SecurityPlugin({ fallbackPermissionSet: 'admin_full_access' }); + const harness = makeMiddlewareCtx({ + permissionSets: [adminSet], + objectFields: ['id', 'name', 'label', 'managed_by'], + ...(findOneImpl ? { findOneImpl } : {}), + }); + await plugin.init(harness.ctx); + await plugin.start(harness.ctx); + return harness.run(opCtx); + }; + + it('DENIES deleting a platform-managed position (sys_position managed_by:system)', async () => { + const opCtx: any = { + object: 'sys_position', operation: 'delete', + options: { where: { id: 'pos_admin' } }, context: adminCtx, + }; + await expect( + runGate(opCtx, () => ({ id: 'pos_admin', name: 'platform_admin', managed_by: 'system' })), + ).rejects.toMatchObject({ name: 'PermissionDeniedError' }); + }); + + it('DENIES updating a package-declared position (sys_position managed_by:config)', async () => { + const opCtx: any = { + object: 'sys_position', operation: 'update', + data: { id: 'pos_sales', label: 'renamed' }, options: { where: { id: 'pos_sales' } }, + context: adminCtx, + }; + await expect( + runGate(opCtx, () => ({ id: 'pos_sales', name: 'sales_rep', managed_by: 'config' })), + ).rejects.toMatchObject({ name: 'PermissionDeniedError' }); + }); + + it('DENIES deleting a platform-owned capability (sys_capability managed_by:platform)', async () => { + const opCtx: any = { + object: 'sys_capability', operation: 'delete', + options: { where: { id: 'cap_plat' } }, context: adminCtx, + }; + await expect( + runGate(opCtx, () => ({ id: 'cap_plat', name: 'manage_users', managed_by: 'platform' })), + ).rejects.toMatchObject({ name: 'PermissionDeniedError' }); + }); + + it('DENIES updating a package-owned capability (sys_capability managed_by:package)', async () => { + const opCtx: any = { + object: 'sys_capability', operation: 'update', + data: { id: 'cap_pkg', label: 'renamed' }, options: { where: { id: 'cap_pkg' } }, + context: adminCtx, + }; + await expect( + runGate(opCtx, () => ({ id: 'cap_pkg', name: 'crm.export', managed_by: 'package' })), + ).rejects.toMatchObject({ name: 'PermissionDeniedError' }); + }); + + it('ALLOWS deleting an admin-authored position (sys_position managed_by:user)', async () => { + const opCtx: any = { + object: 'sys_position', operation: 'delete', + options: { where: { id: 'pos_user' } }, context: adminCtx, + }; + await expect( + runGate(opCtx, () => ({ id: 'pos_user', name: 'my_team', managed_by: 'user' })), + ).resolves.toBeDefined(); + }); + + it('ALLOWS updating an admin-authored position with NO managed_by (tenant-created)', async () => { + const opCtx: any = { + object: 'sys_position', operation: 'update', + data: { id: 'pos_user', label: 'renamed' }, options: { where: { id: 'pos_user' } }, + context: adminCtx, + }; + await expect( + runGate(opCtx, () => ({ id: 'pos_user', name: 'my_team', managed_by: null })), + ).resolves.toBeDefined(); + }); + + it('ALLOWS updating an admin-authored capability (sys_capability managed_by:admin)', async () => { + const opCtx: any = { + object: 'sys_capability', operation: 'update', + data: { id: 'cap_admin', label: 'renamed' }, options: { where: { id: 'cap_admin' } }, + context: adminCtx, + }; + await expect( + runGate(opCtx, () => ({ id: 'cap_admin', name: 'my_cap', managed_by: 'admin' })), + ).resolves.toBeDefined(); + }); + + it('DENIES an admin-door insert that forges platform provenance (sys_position managed_by:system)', async () => { + const opCtx: any = { + object: 'sys_position', operation: 'insert', + data: { name: 'forged', managed_by: 'system' }, context: adminCtx, + }; + await expect(runGate(opCtx)).rejects.toMatchObject({ name: 'PermissionDeniedError' }); + }); + + it('DENIES a bulk/ARRAY insert that forges package provenance on any element (sys_capability)', async () => { + const opCtx: any = { + object: 'sys_capability', operation: 'insert', + data: [ + { name: 'ok_admin_cap' }, + { name: 'forged', managed_by: 'package' }, + ], + context: adminCtx, + }; + await expect(runGate(opCtx)).rejects.toMatchObject({ name: 'PermissionDeniedError' }); + }); + + it('DENIES an update that RE-BADGES an admin row as package-managed (update-to-forge)', async () => { + const opCtx: any = { + object: 'sys_capability', operation: 'update', + data: { id: 'cap_admin', managed_by: 'package' }, options: { where: { id: 'cap_admin' } }, + context: adminCtx, + }; + await expect( + runGate(opCtx, () => ({ id: 'cap_admin', name: 'my_cap', managed_by: 'admin' })), + ).rejects.toMatchObject({ name: 'PermissionDeniedError' }); + }); + + it('ALLOWS a normal admin-door capability insert (no forged provenance)', async () => { + const opCtx: any = { + object: 'sys_capability', operation: 'insert', + data: { name: 'my_cap', label: 'Custom', managed_by: 'admin' }, + context: adminCtx, + }; + await expect(runGate(opCtx)).resolves.toBeDefined(); + }); + + it('DENIES a filter DELETE whose filter matches a platform/package-managed row', async () => { + const opCtx: any = { + object: 'sys_position', operation: 'delete', + options: { where: { active: false } }, // no single id → filter path + context: adminCtx, + }; + await expect( + runGate(opCtx, () => ({ id: 'pos_admin', managed_by: 'system' })), + ).rejects.toMatchObject({ name: 'PermissionDeniedError' }); + }); + + it('ALLOWS a filter delete that matches only admin-authored rows (probe finds none)', async () => { + const opCtx: any = { + object: 'sys_position', operation: 'delete', + options: { where: { managed_by: 'user' } }, + context: adminCtx, + }; + // The managed-row probe finds nothing → the write proceeds. + await expect(runGate(opCtx, () => null)).resolves.toBeDefined(); + }); + + it('DENIES even a principal-less delete of a managed row (gate is before the fall-open)', async () => { + const opCtx: any = { + object: 'sys_position', operation: 'delete', + options: { where: { id: 'pos_admin' } }, + context: {}, // no roles, no permissions, no userId, not isSystem + }; + await expect( + runGate(opCtx, () => ({ id: 'pos_admin', name: 'platform_admin', managed_by: 'system' })), + ).rejects.toMatchObject({ name: 'PermissionDeniedError' }); + }); + + it('lets system/boot writes through (isSystem bypass) even on a platform-managed position', async () => { + const opCtx: any = { + object: 'sys_position', operation: 'update', + data: { id: 'pos_admin', label: 'reseed' }, options: { where: { id: 'pos_admin' } }, + context: { isSystem: true }, + }; + await expect( + runGate(opCtx, () => ({ id: 'pos_admin', name: 'platform_admin', managed_by: 'system' })), + ).resolves.toBeDefined(); + }); + }); }); // --------------------------------------------------------------------------- describe('PermissionEvaluator', () => { diff --git a/packages/plugins/plugin-security/src/security-plugin.ts b/packages/plugins/plugin-security/src/security-plugin.ts index f4728fa254..3db056fab7 100644 --- a/packages/plugins/plugin-security/src/security-plugin.ts +++ b/packages/plugins/plugin-security/src/security-plugin.ts @@ -76,6 +76,36 @@ const EMPTY_REQUIRED_PERMISSIONS: NormalizedRequiredPermissions = Object.freeze( all: [], read: [], create: [], update: [], delete: [], }) as NormalizedRequiredPermissions; +/** + * [ADR-0066 / #2918] Provenance spec for the platform/application asset objects + * whose managed rows are write-protected by {@link SecurityPlugin.assertSystemRowWriteGate}. + * + * The two objects use DIFFERENT `managed_by` vocabularies but the same ownership + * idea — a row authored by the platform or an application package is not the + * admin's to delete or rewrite: + * • sys_position.managed_by — `system` (platform built-in) / `config` + * (package declared) are managed; `user`/∅ (tenant-authored) are the admin's. + * • sys_capability.managed_by — `platform` / `package` are managed; `admin` + * (created in Setup) is the admin's. + * The map value for each managed `managed_by` is the human owner label used in + * the (business-message-only) deny text. + */ +const SYSTEM_ROW_PROVENANCE: Record< + string, + { noun: string; pluralNoun: string; managed: Record } +> = { + sys_position: { + noun: 'position', + pluralNoun: 'positions', + managed: { system: 'the platform', config: 'an application package' }, + }, + sys_capability: { + noun: 'capability', + pluralNoun: 'capabilities', + managed: { platform: 'the platform', package: 'an application package' }, + }, +}; + /** Normalize a raw object `requiredPermissions` (string[] | per-op map) into buckets. */ function normalizeRequiredPermissions(raw: unknown): NormalizedRequiredPermissions { if (Array.isArray(raw)) { @@ -509,6 +539,20 @@ export class SecurityPlugin implements Plugin { // and already short-circuited the whole middleware above. await this.assertPackageManagedWriteGate(opCtx); + // [ADR-0066 / #2918] Built-in row-write guardrail for the platform/app + // ASSET objects sys_position / sys_capability. Like the package gate + // above, an unconditional data-layer boundary: a row authored by the + // platform or an application package (provenance recorded in + // `managed_by`) may not be deleted or rewritten through the admin door. + // Unlike sys_permission_set there is NO ADR-0094 overlay write-through + // for these objects, so the refusal must hold for update/delete here + // rather than deferring to a downstream translation. Runs BEFORE the + // empty-principal fall-open and the CRUD check so the boundary holds even + // for a principal-less context and a superuser with modifyAllRecords. + // System/boot writes carry `isSystem` and already short-circuited above, + // so the seeder and package publish are unaffected. + await this.assertSystemRowWriteGate(opCtx); + // [ADR-0090 D5/D9] Audience-anchor binding guard — like the package // gate above, an unconditional data-layer boundary: a permission set // carrying high-privilege bits must never be bound to the `everyone` @@ -1701,6 +1745,114 @@ export class SecurityPlugin implements Plugin { } } + /** + * [ADR-0066 / #2918] Built-in row-write guardrail for the platform/application + * ASSET objects `sys_position` and `sys_capability`. + * + * ADR-0066's asset-ownership model splits authoring from assignment: WHAT a + * position or capability *is* is defined by the platform or an application + * package developer; a customer admin only decides WHO it is assigned to (via + * the RBAC link tables, which are governed separately by the delegated-admin + * gate). The `managed_by` provenance column on each object already records + * that ownership, but until now nothing ENFORCED it at the data layer — an + * admin could delete or rewrite a platform/package-managed row and silently + * break that app's authorization baseline (ADR-0049: a provenance attribute + * that exists but is never enforced is exactly the gap to close). + * + * This gate is an unconditional data-layer boundary, mirroring the + * `sys_permission_set` two-doors gate above: + * (a) The admin door may never FORGE managed provenance — stamping + * `managed_by` to a platform/package value on insert OR update (single + * object OR array) is refused; only the platform seeder / package + * publish path (which carries `isSystem` and short-circuited the whole + * middleware above) may author it. This also closes update-to-forge. + * (b) delete / update / transfer / restore / purge on a row whose EXISTING + * `managed_by` is platform/package-owned are refused — unlike + * `sys_permission_set`, these objects have NO ADR-0094 overlay + * write-through, so the mutation would otherwise go straight to the + * driver. + * (c) admin-authored rows (`managed_by` user/∅/admin) are untouched — the + * admin fully owns those (incl. a delegate's rows in their own subtree). + * Fails CLOSED and never depends on the caller's grants, so a superuser with + * modifyAllRecords cannot delete a platform position either. + */ + private async assertSystemRowWriteGate(opCtx: any): Promise { + const spec = SYSTEM_ROW_PROVENANCE[opCtx?.object as string]; + if (!spec) return; + const op = opCtx.operation; + if (!['insert', 'update', 'delete', 'transfer', 'restore', 'purge'].includes(op)) return; + + const managedValues = Object.keys(spec.managed); + + // (a) Reject any admin-door PAYLOAD that CLAIMS platform/package provenance, + // on insert OR update, single object OR array. Only the seeder / publish + // path (which carries `isSystem` and short-circuited above) may stamp it. + const payloadRows = Array.isArray(opCtx.data) + ? opCtx.data + : (opCtx.data && typeof opCtx.data === 'object' ? [opCtx.data] : []); + if ( + payloadRows.some( + (r: unknown) => + r && + typeof r === 'object' && + managedValues.includes(String((r as Record).managed_by ?? '')), + ) + ) { + throw new PermissionDeniedError( + `[Security] Access denied: cannot stamp a platform/package 'managed_by' value on a ${spec.noun} ` + + `through the admin door — ${spec.pluralNoun} provided by the platform or an application package are ` + + `authored there and land via seeding/publish, not through Setup (ADR-0066 asset ownership).`, + { operation: op, object: opCtx.object }, + ); + } + if (op === 'insert') return; // no existing row to protect + + if (!this.ql) return; + + const targetId = this.extractSingleId(opCtx); + if (targetId == null) { + // Multi-row / filter write with no single id. Deny ONLY if a managed row + // actually falls within the write's own filter — so a bulk edit that + // targets only admin-authored rows still succeeds (no over-broad block). A + // whole-table write (no filter) matches every managed row, so it is denied. + const writeWhere = opCtx?.options?.where; + const managedWhere = + writeWhere && typeof writeWhere === 'object' + ? { $and: [writeWhere, { managed_by: { $in: managedValues } }] } + : { managed_by: { $in: managedValues } }; + const hitsManagedRow = await this.ql + .findOne(opCtx.object, { where: managedWhere, context: { isSystem: true } }) + .catch(() => null); + if (hitsManagedRow) { + throw new PermissionDeniedError( + `[Security] Access denied: this '${op}' on '${opCtx.object}' targets one or more ${spec.pluralNoun} ` + + `provided by the platform or an application package — those cannot be deleted or modified through ` + + `the admin door (ADR-0066 asset ownership).`, + { operation: op, object: opCtx.object }, + ); + } + return; + } + + const existing = await this.ql + .findOne(opCtx.object, { where: { id: targetId }, context: { isSystem: true } }) + .catch(() => null); + const existingManagedBy = existing + ? String((existing as Record).managed_by ?? '') + : ''; + const ownerLabel = spec.managed[existingManagedBy]; + if (existing && ownerLabel) { + const row = existing as Record; + const source = ownerLabel === 'the platform' ? 'platform definition' : 'application package'; + throw new PermissionDeniedError( + `[Security] Access denied: '${String(row.name ?? row.label ?? targetId)}' is a ${spec.noun} provided ` + + `by ${ownerLabel} — it cannot be deleted or modified through the admin door. Change it by editing ` + + `its ${source} and re-publishing (ADR-0066 asset ownership).`, + { operation: op, object: opCtx.object, recordId: targetId, managedBy: existingManagedBy }, + ); + } + } + private extractSingleId(opCtx: any): string | number | bigint | null { const isScalar = (v: unknown): v is string | number | bigint => v !== null && (typeof v === 'string' || typeof v === 'number' || typeof v === 'bigint');