an uninstalled plugin's grants stop breaking the permissions screen

Uninstalling music left its grants in role_permissions. The keys stop existing
when the plugin unmounts, so the settings page — which loads every grant and
submits them all back — failed with "Unknown permission 'music'" for a row
nobody could see, edit or remove through the UI. Nothing could be saved again
without editing the database.

Two changes, because either alone leaves half the problem.

Uninstall now revokes the plugin's grants, across every role rather than one. A
deliberate exception to "uninstall deletes nothing": a grant is the PLATFORM's
record of who may reach the plugin, not anything the plugin stored. Reinstalling
means granting again, which is the honest behaviour — the alternative is access
silently returning.

And the write DROPS an unknown key instead of refusing the request, so every
server already in this state is unblocked without touching its database. That is
what the resolver already does on read, so the write now agrees with it. The
cost is that a typo in a hand-made API call is ignored rather than reported —
worth less than a settings page that cannot be used. The response returns what
was dropped, so it is not silent.

Co-Authored-By: Claude Opus 5 <noreply@anthropic.com>
This commit is contained in:
2026-08-19 12:18:49 +00:00
co-authored by Claude Opus 5
parent b9524ee4d5
commit 0fd89082fe
5 changed files with 102 additions and 4 deletions
+21 -2
View File
@@ -125,6 +125,8 @@ permissionAdminRouter.put('/permissions/:role', ownerGate, async (ctx) => {
if (!Array.isArray(raw)) throw errors.BAD_REQUEST('Expected { grants: [{ permission, level }] }');
const grants: { permission: string; level: 'read' | 'write' }[] = [];
/** Keys nothing defines — almost always a plugin that has since been uninstalled. */
const dropped: string[] = [];
for (const entry of raw) {
const { permission, level } = (entry ?? {}) as { permission?: unknown; level?: unknown };
if (typeof permission !== 'string') throw errors.BAD_REQUEST('Each grant needs a permission key');
@@ -133,8 +135,21 @@ permissionAdminRouter.put('/permissions/:role', ownerGate, async (ctx) => {
// The registry is the authority on what a permission key means, which is why the column has no CHECK.
// This is where that authority is applied — rejecting a name nothing defines, and refusing to store a
// grant the resolver would drop on read anyway.
// An unknown key is DROPPED, not refused.
//
// It used to reject the whole request, and that turned an uninstall into a dead settings screen: the
// grants for a removed plugin are still in `role_permissions`, the page loads them, and submitting
// them back fails with "Unknown permission 'music'" — for a row the owner never touched and cannot
// see. Nothing could be saved again until somebody edited the database.
//
// Dropping is also what the resolver already does on read, so this only makes the write agree with
// it. The cost is that a typo in a hand-made API call is silently ignored rather than reported;
// that is worth less than a settings page that cannot be used.
const known = PERMISSION_BY_KEY.get(permission);
if (!known) throw errors.BAD_REQUEST(`Unknown permission '${permission}'`);
if (!known) {
dropped.push(permission);
continue;
}
if (known.kind !== 'app' && known.kind !== 'confined') {
throw errors.BAD_REQUEST(
known.kind === 'execution'
@@ -145,10 +160,14 @@ permissionAdminRouter.put('/permissions/:role', ownerGate, async (ctx) => {
grants.push({ permission, level });
}
if (dropped.length) console.log(`[permissions] ${role}: ignored unknown key(s) ${dropped.join(', ')}`);
await replaceRoleGrants(role, grants);
// The cache's entire invalidation contract, discharged here. Adding a second writer means adding a
// second call to this — see the note on grantCache in permissions/authorize.ts.
invalidateRoleGrants(role);
return ctx.json({ role, grants });
// `dropped` is returned so the screen can say what it quietly discarded, rather than the owner
// finding out later that a grant they thought they saved is gone.
return ctx.json({ role, grants, dropped });
});
@@ -0,0 +1,37 @@
import { describe, expect, test } from 'bun:test';
import { PERMISSION_BY_KEY } from '@@/permissions/registry';
// A plugin's permission keys stop existing the moment it is uninstalled, but its grants stay in
// `role_permissions`. The settings page loads every grant and submits them all back, so one orphan made
// the entire screen unsaveable — for a row nobody could see, edit or remove through the UI.
//
// Two things now hold. Uninstall takes the grants with it, and the write drops a key it does not know
// instead of refusing the request.
const partition = (submitted: string[]) => {
const kept: string[] = [];
const dropped: string[] = [];
for (const key of submitted) (PERMISSION_BY_KEY.get(key) ? kept : dropped).push(key);
return { kept, dropped };
};
describe('a grant naming a permission that no longer exists', () => {
test('is separated from the ones that do, rather than failing the batch', () => {
// `music` is the real case: a plugin, uninstalled, its grants left behind.
const { kept, dropped } = partition(['chat', 'music', 'terminal']);
expect(dropped).toEqual(['music']);
expect(kept).toEqual(['chat', 'terminal']);
});
test('a batch of nothing but unknown keys still leaves the known set empty, not undefined', () => {
expect(partition(['music', 'photos-that-never-existed']).kept).toEqual([]);
});
test('core keys are never treated as stale', () => {
// The failure mode to avoid is over-correcting: dropping a key the registry does define would
// silently revoke access the owner asked for.
for (const key of ['chat', 'terminal', 'files']) {
expect(partition([key]).dropped).toEqual([]);
}
});
});
+15
View File
@@ -14,6 +14,7 @@ import { deleteProcess, processStatus, startProcess, stopProcess } from '../app-
import { addPluginToEcosystem, pluginProcessName, removePluginFromEcosystem } from './ecosystem';
import { discoverPlugins } from './discover';
import { fetchPlugin } from './fetch';
import { revokeGrantsForPermissions } from 'officerdb';
import { publishAssets, unpublishAssets } from '../app-store/assets';
import { publishAffordances, unpublishAffordances } from '../affordances/publish';
import { refreshPluginMounts, snapshotPlugins } from './mount';
@@ -430,6 +431,20 @@ export async function uninstallPlugin(appName: string, onStep?: OnStep, onLog?:
// The one thing uninstall IS allowed to delete: published assets are copies, and the originals are
// still in the plugin's own directory. Nothing a user made is in there.
// Its grants go too. The keys stop existing when the plugin is unmounted, and a grant naming one is a
// row nobody can see, edit or remove through the UI — which made the whole permissions screen
// unsaveable, because the page submits every grant it loaded and the write refused a key it no longer
// recognised. Reported on a real server after uninstalling music.
//
// This is a deliberate exception to "uninstall deletes nothing": a grant is the PLATFORM's record of
// who may reach the plugin, not anything the plugin stored. Reinstalling means granting again, which
// is the honest behaviour anyway — the alternative is access silently returning.
if (plugin) {
const keys = plugin.manifest.permissions.map((p) => p.key);
const revoked = await revokeGrantsForPermissions(keys).catch(() => 0);
if (revoked) await step(steps, onStep, `grants: ${revoked} removed for ${keys.join(', ')}`);
}
await unpublishAssets(appName);
// Same reasoning: a copy, with the original still in the plugin's directory. Removed so an agent stops
// being told about a surface that is no longer there — an affordance for an uninstalled plugin is