fix(security): revoke package permission rows on uninstall — no ghost grants (#2747)

.changeset/uninstall-permission-cleanup.md

@@ -0,0 +1,34 @@
+---
+"@objectstack/metadata-protocol": minor
+"@objectstack/plugin-security": minor
+"@objectstack/rest": patch
+---
+
+Package uninstall now revokes the package's data-plane permission rows (#2747, ADR-0086 D3 / ADR-0090 D5 "no ghost grants").
+
+**`@objectstack/metadata-protocol`**: `deletePackage` gains an
+uninstall-cleanup seam — the exact mirror of the publish materializer:
+domain plugins register named cleanups via `registerUninstallCleanup(name,
+fn)` and every cleanup runs with the uninstalled package id, its outcome
+reported on the new `cleanups` array of the response (a failed revocation is
+visible, never silent). `deletePackage` also unregisters the package from
+the in-memory SchemaRegistry (best-effort), so the running kernel stops
+serving it without waiting for a restart.
+
+**`@objectstack/plugin-security`**: registers the
+`security.package-permissions` cleanup — deletes the package's own
+`sys_permission_set` rows (`managed_by: 'package'` + matching `package_id`
+only; env-authored and foreign-package rows are never touched, ADR-0086 D4),
+their `sys_position_permission_set` / `sys_user_permission_set` bindings
+(bindings first, so no dangling grants), and the package's
+`sys_audience_binding_suggestion` rows (a reinstall re-prompts fresh).
+Also fixes the engine-call signature in the suggestion module: `find`/`delete`
+read `context` from their second argument — the previous trailing
+`{ context }` argument was ignored, so deletes ran principal-less.
+
+**`@objectstack/rest`**: `DELETE /api/v1/packages/:id` (no version pin) now
+goes through `protocol.deletePackage` — one uninstall semantic instead of a
+bare `sys_packages` row delete — removing the package's metadata, durable
+record, registry entry, and running the cleanups; the response carries
+`deletedCount` + `cleanups`. A version-scoped delete keeps the narrow
+durable-registry semantics.

content/docs/permissions/permission-sets.mdx

@@ -193,7 +193,11 @@ A package ships its own sets (`managedBy: 'package'` + `packageId`), seeded
 idempotently at boot and re-seeded on upgrade; environment-authored sets
 (`platform`/`user`) are never clobbered. The data layer refuses admin-door
 writes to package rows (two-doors separation), which is what makes package
-uninstall well-defined.
+uninstall well-defined — and enforced: uninstalling a package
+(`DELETE /api/v1/packages/:id`) revokes its own sets, their position/user
+bindings, and its pending audience-binding suggestions in the same request
+(no ghost grants); the uninstall response reports the revocation under
+`cleanups`. Environment-authored sets and other packages' rows survive.
 
 ---
 

packages/metadata-protocol/src/durable-package.test.ts

@@ -77,3 +77,43 @@ describe('deletePackage — durable un-registration (#2532 counterpart)', () =>
     expect(del).toHaveBeenCalledWith('com.example.orders');
   });
 });
+
+describe('deletePackage — uninstall cleanups (#2747)', () => {
+  it('invokes registered cleanups with the package id and reports outcomes', async () => {
+    const { impl } = makeImpl();
+    const cleanup = vi.fn(async () => ({ success: true, removed: 3 }));
+    (impl as any).registerUninstallCleanup('security.package-permissions', cleanup);
+
+    const res: any = await (impl as any).deletePackage({ packageId: 'com.example.orders', actor: 'usr_1' });
+
+    expect(cleanup).toHaveBeenCalledWith(expect.objectContaining({ packageId: 'com.example.orders', actor: 'usr_1' }));
+    expect(res.cleanups).toEqual([
+      { name: 'security.package-permissions', success: true, removed: 3 },
+    ]);
+  });
+
+  it('reports a throwing cleanup as failed instead of aborting the uninstall', async () => {
+    const warn = vi.spyOn(console, 'warn').mockImplementation(() => {});
+    try {
+      const { impl } = makeImpl();
+      (impl as any).registerUninstallCleanup('boom', async () => { throw new Error('db down'); });
+      const res: any = await (impl as any).deletePackage({ packageId: 'com.example.orders' });
+      expect(res.cleanups).toEqual([
+        { name: 'boom', success: false, removed: 0, error: 'db down' },
+      ]);
+    } finally {
+      warn.mockRestore();
+    }
+  });
+
+  it('re-registration under the same name replaces (idempotent re-init)', async () => {
+    const { impl } = makeImpl();
+    const first = vi.fn(async () => ({ success: true, removed: 1 }));
+    const second = vi.fn(async () => ({ success: true, removed: 2 }));
+    (impl as any).registerUninstallCleanup('x', first);
+    (impl as any).registerUninstallCleanup('x', second);
+    const res: any = await (impl as any).deletePackage({ packageId: 'p' });
+    expect(first).not.toHaveBeenCalled();
+    expect(res.cleanups[0].removed).toBe(2);
+  });
+});

packages/metadata-protocol/src/index.ts

@@ -1,6 +1,7 @@
 // Copyright (c) 2025 ObjectStack. Licensed under the Apache-2.0 license.
 
 export { ObjectStackProtocolImplementation, ConcurrentUpdateError, normalizeViewMetadata } from './protocol.js';
+export type { UninstallCleanup, UninstallCleanupOutcome } from './protocol.js';
 
 export { SysMetadataRepository, resetEnvWritableMetadataTypes } from './sys-metadata-repository.js';
 export type {

packages/metadata-protocol/src/protocol.ts

@@ -720,6 +720,31 @@ export type PublishMaterializer = (args: {
     actor: string;
 }) => Promise<PublishMaterializeResult>;
 
+/**
+ * Uninstall-time data-plane cleanup (ADR-0086 D3, #2747). The exact mirror of
+ * {@link PublishMaterializer}: domain plugins own data-plane tables the
+ * protocol layer must not know the shape of (e.g. plugin-security's
+ * `sys_permission_set` and its binding tables), so they register a named
+ * cleanup here and {@link ObjectStackProtocolImplementation.deletePackage}
+ * invokes every cleanup with the uninstalled package id. Cleanups run
+ * best-effort — a failure is REPORTED on the uninstall response (`cleanups`),
+ * never thrown — but ghost grants are a security condition, so callers must
+ * surface a failed cleanup, not swallow it.
+ */
+export type UninstallCleanup = (args: {
+    packageId: string;
+    organizationId?: string;
+    actor?: string;
+}) => Promise<{ success: boolean; removed: number; error?: string }>;
+
+/** Per-cleanup outcome reported on the `deletePackage` response. */
+export interface UninstallCleanupOutcome {
+    name: string;
+    success: boolean;
+    removed: number;
+    error?: string;
+}
+
 /**
  * Post-persistence metadata-mutation notification (#2588). Emitted by
  * `saveMetaItem` / `publishMetaItem` / `deleteMetaItem` AFTER the write
@@ -769,6 +794,9 @@ export class ObjectStackProtocolImplementation implements ObjectStackProtocol {
      */
     private publishMaterializers = new Map<string, PublishMaterializer>();
 
+    /** [#2747] Named uninstall cleanups, run by {@link deletePackage}. */
+    private uninstallCleanups = new Map<string, UninstallCleanup>();
+
     constructor(
         engine: IDataEngine,
         getServicesRegistry?: () => Map<string, any>,
@@ -793,6 +821,18 @@ export class ObjectStackProtocolImplementation implements ObjectStackProtocol {
         this.publishMaterializers.set(singular, materializer);
     }
 
+    /**
+     * Register a named uninstall-time data-plane cleanup (ADR-0086 D3, #2747).
+     * Called by domain plugins at init — e.g. plugin-security registers the
+     * cleanup that removes its package-owned `sys_permission_set` rows and
+     * their bindings when the owning package is uninstalled, so grants are
+     * revoked everywhere at once (no ghost grants). One cleanup per name; a
+     * second registration replaces the first (idempotent re-init).
+     */
+    registerUninstallCleanup(name: string, cleanup: UninstallCleanup): void {
+        this.uninstallCleanups.set(name, cleanup);
+    }
+
     /**
      * Runtime-mutation listeners (#2588). Every metadata mutation that lands
      * through this protocol — `saveMetaItem` (draft AND direct-active saves),
@@ -4716,6 +4756,7 @@ export class ObjectStackProtocolImplementation implements ObjectStackProtocol {
         failedCount: number;
         deleted: Array<{ type: string; name: string; state: string }>;
         failed: Array<{ type: string; name: string; error: string; code?: string }>;
+        cleanups: UninstallCleanupOutcome[];
     }> {
         const where: Record<string, unknown> = { package_id: request.packageId };
         if (request.organizationId) where.organization_id = request.organizationId;
@@ -4766,12 +4807,56 @@ export class ObjectStackProtocolImplementation implements ObjectStackProtocol {
             );
         }
 
+        // [#2747] Unregister from the in-memory SchemaRegistry too, so the
+        // running kernel stops serving the package without waiting for a
+        // restart. Best-effort: the HTTP dispatcher already unregisters
+        // before calling us (second call is a no-op warn), and a package
+        // with live extenders refuses unregistration — that failure is
+        // logged, not fatal (the durable row is gone, so the next boot is
+        // clean either way).
+        try {
+            (this.engine as any)?.registry?.uninstallPackage?.(request.packageId);
+        } catch (e) {
+            console.warn(
+                `[protocol.deletePackage] registry unregistration skipped for '${request.packageId}': ${(e as Error)?.message}`,
+            );
+        }
+
+        // [#2747] Data-plane cleanups registered by domain plugins (mirror of
+        // the publish materializers): revoke what the package's metadata
+        // granted — e.g. plugin-security removes its package-owned
+        // sys_permission_set rows and their bindings. Best-effort per cleanup;
+        // outcomes ride on the response so a failed revocation (ghost grants —
+        // a security condition) is visible to the caller, never silent.
+        const cleanups: UninstallCleanupOutcome[] = [];
+        for (const [name, cleanup] of this.uninstallCleanups) {
+            try {
+                const r = await cleanup({
+                    packageId: request.packageId,
+                    ...(request.organizationId ? { organizationId: request.organizationId } : {}),
+                    ...(request.actor ? { actor: request.actor } : {}),
+                });
+                cleanups.push({
+                    name,
+                    success: r?.success !== false,
+                    removed: typeof r?.removed === 'number' ? r.removed : 0,
+                    ...(r?.error ? { error: r.error } : {}),
+                });
+            } catch (e: any) {
+                cleanups.push({ name, success: false, removed: 0, error: e?.message ?? 'cleanup failed' });
+                console.warn(
+                    `[protocol.deletePackage] uninstall cleanup '${name}' failed for '${request.packageId}': ${e?.message}`,
+                );
+            }
+        }
+
         return {
             success: failed.length === 0 && deleted.length > 0,
             deletedCount: deleted.length,
             failedCount: failed.length,
             deleted,
             failed,
+            cleanups,
         };
     }
 

packages/plugins/plugin-security/src/cleanup-package-permissions.test.ts

@@ -0,0 +1,87 @@
+// Copyright (c) 2026 ObjectStack. Licensed under the Apache-2.0 license.
+// #2747 — uninstall-time revocation of a package's data-plane permission rows
+// (ADR-0086 D3 provenance consumed at last; ADR-0090 D5 "no ghost grants").
+
+import { describe, it, expect } from 'vitest';
+import { cleanupPackagePermissions } from './cleanup-package-permissions';
+
+function makeQl() {
+  const tables: Record<string, any[]> = {
+    sys_permission_set: [
+      // the uninstalled package's own rows
+      { id: 'ps_crm_ro', name: 'crm_readonly', package_id: 'com.example.crm', managed_by: 'package' },
+      { id: 'ps_crm_admin', name: 'crm_admin', package_id: 'com.example.crm', managed_by: 'package' },
+      // another package's row — must survive
+      { id: 'ps_other', name: 'other_set', package_id: 'com.other', managed_by: 'package' },
+      // env-authored rows — must survive even if a package_id is present
+      { id: 'ps_env', name: 'member_default', managed_by: 'user' },
+      { id: 'ps_legacy', name: 'admin_full_access' },
+    ],
+    sys_position_permission_set: [
+      { id: 'pps_1', position_id: 'pos_everyone', permission_set_id: 'ps_crm_ro' },
+      { id: 'pps_2', position_id: 'pos_sales', permission_set_id: 'ps_crm_admin' },
+      { id: 'pps_other', position_id: 'pos_sales', permission_set_id: 'ps_other' },
+    ],
+    sys_user_permission_set: [
+      { id: 'ups_1', user_id: 'u1', permission_set_id: 'ps_crm_admin' },
+      { id: 'ups_env', user_id: 'u1', permission_set_id: 'ps_env' },
+    ],
+    sys_audience_binding_suggestion: [
+      { id: 'sug_1', package_id: 'com.example.crm', permission_set_name: 'crm_readonly', anchor: 'everyone', status: 'confirmed' },
+      { id: 'sug_2', package_id: 'com.example.crm', permission_set_name: 'crm_admin', anchor: 'everyone', status: 'dismissed' },
+      { id: 'sug_other', package_id: 'com.other', permission_set_name: 'other_set', anchor: 'everyone', status: 'pending' },
+    ],
+  };
+  return {
+    tables,
+    async find(object: string, opts: any) {
+      const where = opts?.where ?? {};
+      return (tables[object] ?? []).filter((r) =>
+        Object.entries(where).every(([k, v]) => (r as any)[k] === v),
+      );
+    },
+    async delete(object: string, opts: any) {
+      const id = opts?.where?.id;
+      const t = tables[object] ?? [];
+      const i = t.findIndex((r) => r.id === id);
+      if (i >= 0) t.splice(i, 1);
+      return true;
+    },
+  } as any;
+}
+
+describe('cleanupPackagePermissions (#2747)', () => {
+  it('removes the package-owned sets, their bindings/grants, and its suggestion rows', async () => {
+    const ql = makeQl();
+    const out = await cleanupPackagePermissions(ql, 'com.example.crm');
+
+    expect(out).toEqual({ sets: 2, positionBindings: 2, userGrants: 1, suggestions: 2 });
+
+    // no ghost grants: nothing referencing the removed sets survives
+    expect(ql.tables.sys_permission_set.map((r: any) => r.id)).toEqual(['ps_other', 'ps_env', 'ps_legacy']);
+    expect(ql.tables.sys_position_permission_set.map((r: any) => r.id)).toEqual(['pps_other']);
+    expect(ql.tables.sys_user_permission_set.map((r: any) => r.id)).toEqual(['ups_env']);
+    expect(ql.tables.sys_audience_binding_suggestion.map((r: any) => r.id)).toEqual(['sug_other']);
+  });
+
+  it('never touches env-authored or foreign-package rows (ADR-0086 D4 provenance)', async () => {
+    const ql = makeQl();
+    await cleanupPackagePermissions(ql, 'com.example.crm');
+    const names = ql.tables.sys_permission_set.map((r: any) => r.name);
+    expect(names).toContain('other_set');
+    expect(names).toContain('member_default');
+    expect(names).toContain('admin_full_access');
+  });
+
+  it('is idempotent — a second run removes nothing', async () => {
+    const ql = makeQl();
+    await cleanupPackagePermissions(ql, 'com.example.crm');
+    const second = await cleanupPackagePermissions(ql, 'com.example.crm');
+    expect(second).toEqual({ sets: 0, positionBindings: 0, userGrants: 0, suggestions: 0 });
+  });
+
+  it('no-ops safely on a missing package id or a non-engine handle', async () => {
+    expect(await cleanupPackagePermissions(makeQl(), '')).toEqual({ sets: 0, positionBindings: 0, userGrants: 0, suggestions: 0 });
+    expect(await cleanupPackagePermissions(null, 'x')).toEqual({ sets: 0, positionBindings: 0, userGrants: 0, suggestions: 0 });
+  });
+});