From 9db66d57803ed92606cb009052c96f9a55d1f3b4 Mon Sep 17 00:00:00 2001 From: LukeGus Date: Mon, 21 Sep 2026 17:09:28 -0500 Subject: [PATCH] feat: add cross-plugin secret references gated by the provider's RBAC permission --- plugins/ai/backend/index.mjs | 22 + plugins/ai/manifest.json | 10 +- scripts/plugin-manifest.schema.json | 50 +++ scripts/validate-plugin-manifest.cjs | 112 +++++ src/backend/plugins/host-ctx.ts | 125 +++++- src/backend/plugins/loader.ts | 23 + src/backend/plugins/manifest.ts | 149 ++++++ src/backend/plugins/secret-registry.ts | 362 +++++++++++++++ src/backend/plugins/worker-bootstrap.ts | 6 + src/backend/tests/plugins/manifest.test.ts | 110 +++++ .../tests/plugins/secret-contract.test.ts | 394 ++++++++++++++++ .../tests/plugins/secret-registry.test.ts | 424 ++++++++++++++++++ src/ui/locales/en.json | 5 +- 13 files changed, 1782 insertions(+), 10 deletions(-) create mode 100644 src/backend/plugins/secret-registry.ts create mode 100644 src/backend/tests/plugins/secret-contract.test.ts create mode 100644 src/backend/tests/plugins/secret-registry.test.ts diff --git a/plugins/ai/backend/index.mjs b/plugins/ai/backend/index.mjs index 54cc1ff02..53e065629 100644 --- a/plugins/ai/backend/index.mjs +++ b/plugins/ai/backend/index.mjs @@ -54,6 +54,28 @@ export async function activate(ctx) { routesModule = await loadRelative("routes"); routesModule.startAiService(); + // Offered by reference, not copied: this hands over a resolver, so the key + // stays ours. Rotating or deleting the provider takes effect on the next + // read by any borrower, and deactivate() below withdraws the offer entirely. + // The resolve is per-user because the keys are stored per-user and encrypted + // under that user's data key, so a borrowed read returns the acting user's + // own default key, never someone else's. + ctx.secrets.offer("api-key", async (userId) => { + const { createCurrentAiRepository } = await loadRelative( + "../../../backend/backend/database/repositories/factory", + ); + const repository = createCurrentAiRepository(); + const providers = await repository.listProviders(userId); + const preferred = providers.find((provider) => provider.apiKeyPrefix); + if (!preferred) return null; + + const withSecret = await repository.findProviderWithSecret( + preferred.id, + userId, + ); + return withSecret?.apiKey ?? null; + }); + ctx.log.info("AI assistant router registered at /ai"); } diff --git a/plugins/ai/manifest.json b/plugins/ai/manifest.json index e12516546..4648a6726 100644 --- a/plugins/ai/manifest.json +++ b/plugins/ai/manifest.json @@ -31,7 +31,8 @@ "ai.use", "ai.manage_providers", "ai.apply_proposals", - "ai.services.use" + "ai.services.use", + "ai.secrets.share" ], "defaultForRole": { "admin": ["ai.services.use"] @@ -49,5 +50,12 @@ } ] }, + "providesSecret": [ + { + "key": "api-key", + "permission": "ai.secrets.share", + "descriptionKey": "ai.secrets.shareApiKey" + } + ], "sidecars": [] } diff --git a/scripts/plugin-manifest.schema.json b/scripts/plugin-manifest.schema.json index 2daa33331..8bf143827 100644 --- a/scripts/plugin-manifest.schema.json +++ b/scripts/plugin-manifest.schema.json @@ -331,6 +331,56 @@ } } }, + "providesSecret": { + "type": "array", + "description": "Secrets this plugin offers other plugins by reference. The value is never copied; a borrower re-reads it per call.", + "items": { + "type": "object", + "additionalProperties": false, + "required": ["key", "permission"], + "properties": { + "key": { + "type": "string", + "pattern": "^[a-z0-9-]+$", + "description": "Secret name within this plugin, e.g. \"api-key\"." + }, + "permission": { + "type": "string", + "minLength": 1, + "description": "Role permission a borrowing user needs. Must also appear in contributes.permissionGroup.permissions." + }, + "descriptionKey": { + "type": "string", + "minLength": 1, + "description": "i18n key describing the sharing, shown in the admin UI." + } + } + } + }, + "requiresSecret": { + "type": "array", + "description": "Secrets this plugin borrows from another plugin. Reads resolve to null when the provider is absent or has nothing set.", + "items": { + "type": "object", + "additionalProperties": false, + "required": ["plugin", "key"], + "properties": { + "plugin": { + "type": "string", + "pattern": "^[a-z0-9-]+$", + "description": "Plugin id that offers the secret." + }, + "key": { + "type": "string", + "pattern": "^[a-z0-9-]+$" + }, + "optional": { + "type": "boolean", + "description": "When true, an absent provider is normal rather than reported as an unmet requirement." + } + } + } + }, "sidecars": { "type": "array", "items": { diff --git a/scripts/validate-plugin-manifest.cjs b/scripts/validate-plugin-manifest.cjs index 41aa3f131..0f0d5ab60 100644 --- a/scripts/validate-plugin-manifest.cjs +++ b/scripts/validate-plugin-manifest.cjs @@ -18,6 +18,7 @@ const ID_PATTERN = /^[a-z0-9-]+$/; const SEMVER_PATTERN = /^\d+\.\d+\.\d+(-[0-9A-Za-z-.]+)?(\+[0-9A-Za-z-.]+)?$/; const API_VERSION_PATTERN = /^[0-9]+$/; const SERVICE_PATTERN = /^[a-z0-9-]+(\.[a-z0-9-]+)+$/; +const SECRET_KEY_PATTERN = /^[a-z0-9-]+$/; // Dotted like a service name, but segments may be camelCase: these name a // frontend function ("ai.openWithContext"), not a lowercase service contract. const ACTION_ID_PATTERN = /^[a-z0-9-]+(\.[a-zA-Z0-9-]+)+$/; @@ -132,6 +133,8 @@ function validateManifest(manifest, schema) { errors.push(...validateProvides(manifest.provides)); errors.push(...validateRequires(manifest.requires)); + errors.push(...validateProvidesSecret(manifest.providesSecret)); + errors.push(...validateRequiresSecret(manifest.requiresSecret)); if (manifest.contributes && typeof manifest.contributes === "object") { errors.push(...validateContributes(manifest.contributes)); @@ -150,6 +153,28 @@ function validateManifest(manifest, schema) { } } + // Same rule for a shared secret: sharing rides on the provider's own RBAC + // permission rather than a separate grant mechanism. + if (Array.isArray(manifest.providesSecret) && Array.isArray(declared)) { + for (const entry of manifest.providesSecret) { + if (entry && entry.permission && !declared.includes(entry.permission)) { + errors.push( + `Secret "${entry.key}" is gated by "${entry.permission}", which is not declared in contributes.permissionGroup.permissions`, + ); + } + } + } + + if (Array.isArray(manifest.requiresSecret)) { + for (const entry of manifest.requiresSecret) { + if (entry && entry.plugin && entry.plugin === manifest.id) { + errors.push( + `requiresSecret entry for "${entry.key}" names this plugin itself; use ctx.secrets.get instead`, + ); + } + } + } + // Same rule for UI actions: a permission the catalog never sees is one no // admin can grant, so the action would be invisible rather than denied. const actions = manifest.contributes?.actions; @@ -257,6 +282,93 @@ function validateRequires(requires) { return errors; } +function validateProvidesSecret(providesSecret) { + if (providesSecret === undefined) return []; + if (!Array.isArray(providesSecret)) { + return ['Field "providesSecret" must be an array']; + } + + const errors = []; + const seen = new Set(); + + providesSecret.forEach((entry, index) => { + if (!entry || typeof entry !== "object" || Array.isArray(entry)) { + errors.push(`providesSecret[${index}] must be an object`); + return; + } + + if (typeof entry.key !== "string" || !SECRET_KEY_PATTERN.test(entry.key)) { + errors.push( + `providesSecret[${index}].key must match ${SECRET_KEY_PATTERN}, got: "${entry.key}"`, + ); + } else if (seen.has(entry.key)) { + errors.push( + `providesSecret[${index}].key is a duplicate: "${entry.key}"`, + ); + } else { + seen.add(entry.key); + } + + if (typeof entry.permission !== "string" || entry.permission.length === 0) { + errors.push(`providesSecret[${index}].permission is required`); + } + + if ( + "descriptionKey" in entry && + (typeof entry.descriptionKey !== "string" || + entry.descriptionKey.length === 0) + ) { + errors.push(`providesSecret[${index}].descriptionKey must be a string`); + } + }); + + return errors; +} + +function validateRequiresSecret(requiresSecret) { + if (requiresSecret === undefined) return []; + if (!Array.isArray(requiresSecret)) { + return ['Field "requiresSecret" must be an array']; + } + + const errors = []; + const seen = new Set(); + + requiresSecret.forEach((entry, index) => { + if (!entry || typeof entry !== "object" || Array.isArray(entry)) { + errors.push(`requiresSecret[${index}] must be an object`); + return; + } + + if (typeof entry.plugin !== "string" || !ID_PATTERN.test(entry.plugin)) { + errors.push( + `requiresSecret[${index}].plugin must match ${ID_PATTERN}, got: "${entry.plugin}"`, + ); + } + + if (typeof entry.key !== "string" || !SECRET_KEY_PATTERN.test(entry.key)) { + errors.push( + `requiresSecret[${index}].key must match ${SECRET_KEY_PATTERN}, got: "${entry.key}"`, + ); + } + + if (typeof entry.plugin === "string" && typeof entry.key === "string") { + const id = `${entry.plugin}:${entry.key}`; + if (seen.has(id)) { + errors.push(`requiresSecret[${index}] is a duplicate: "${id}"`); + } else { + seen.add(id); + } + } + + if ("optional" in entry && typeof entry.optional !== "boolean") { + errors.push(`requiresSecret[${index}].optional must be a boolean`); + } + }); + + return errors; +} + function validateContributes(contributes) { const errors = []; diff --git a/src/backend/plugins/host-ctx.ts b/src/backend/plugins/host-ctx.ts index 0b23828f0..4ffda5334 100644 --- a/src/backend/plugins/host-ctx.ts +++ b/src/backend/plugins/host-ctx.ts @@ -19,6 +19,8 @@ import { pluginEvents } from "./events.js"; import * as registry from "./registry.js"; import * as serviceRegistry from "./service-registry.js"; import type { ServiceRegistration } from "./service-registry.js"; +import * as secretRegistry from "./secret-registry.js"; +import type { SecretRegistration } from "./secret-registry.js"; import { safeDetails } from "./broker.js"; import type { PluginManifest } from "./manifest.js"; @@ -58,6 +60,26 @@ export interface InProcessPluginContext { options?: { userId?: string }, ) => T; }; + /** + * Secrets shared between plugins by reference. + * + * `offer` publishes a resolver, never a value, so the provider keeps the + * ability to rotate or clear it at any time. `getShared` reads another + * plugin's offered secret for one call, gated by that provider's own role + * permission. See secret-registry.ts. + */ + secrets: { + offer: ( + key: string, + resolve: (userId: string) => Promise | string | null, + ) => void; + withdraw: (key: string) => boolean; + getShared: ( + pluginId: string, + key: string, + options?: { userId?: string }, + ) => Promise; + }; } /** @@ -73,6 +95,8 @@ export interface InProcessHandle { providedKeys: Array<{ key: string; value: unknown }>; /** Declared services this plugin provided, revoked on deactivate. */ providedServices: ServiceRegistration[]; + /** Shared secrets this plugin offered, withdrawn on deactivate. */ + offeredSecrets: SecretRegistration[]; } export interface PluginModule { @@ -180,20 +204,61 @@ export function createInProcessContext( get: (service, options) => serviceRegistry.createServiceHandle(service, pluginId, { resolveUserId: () => options?.userId, - hasPermission: async (userId, permission) => { - const { PermissionManager } = - await import("../utils/permission-manager.js"); - return PermissionManager.getInstance().hasPermission( - userId, - permission, - ); - }, + hasPermission: checkPermission, audit: (entry) => writeServiceAudit(entry), }), }, + + secrets: { + offer: (key, resolve) => { + // Declared AND offered, the same rule services.provide enforces. + const declared = manifest.providesSecret?.find( + (entry) => entry.key === key, + ); + if (!declared) { + throw new Error( + `Plugin ${pluginId} cannot offer secret "${key}": it is not declared in the manifest's providesSecret array`, + ); + } + + const registration = secretRegistry.offerSecret({ + pluginId, + pluginName: manifest.name, + key, + permission: declared.permission, + resolve, + }); + handle.offeredSecrets.push(registration); + }, + + withdraw: (key) => { + const registration = handle.offeredSecrets.find( + (entry) => entry.key === key, + ); + handle.offeredSecrets = handle.offeredSecrets.filter( + (entry) => entry !== registration, + ); + return secretRegistry.withdrawSecret(pluginId, key, registration); + }, + + getShared: (providerPluginId, key, options) => + secretRegistry.readSharedSecret(manifest, providerPluginId, key, { + resolveUserId: () => options?.userId, + hasPermission: checkPermission, + audit: (entry) => writeSecretAudit(entry), + }), + }, }; } +async function checkPermission( + userId: string, + permission: string, +): Promise { + const { PermissionManager } = await import("../utils/permission-manager.js"); + return PermissionManager.getInstance().hasPermission(userId, permission); +} + /** * Mirrors PluginBroker.audit: attribution comes from the runtime, never from * the plugin, and the details only ever record the shape of a call. @@ -222,6 +287,35 @@ async function writeServiceAudit( } } +/** + * Records that a secret was borrowed, never what it was. + * + * `resolved` is the only thing said about the value: whether one came back at + * all. That is what makes the trail useful (you can see a consumer running on + * someone else's key) without the trail itself becoming a place the secret + * leaks to. + */ +async function writeSecretAudit( + entry: secretRegistry.SecretAuditEntry, +): Promise { + try { + const { logAudit } = await import("../utils/audit-logger.js"); + await logAudit({ + userId: entry.userId, + username: `plugin:${entry.consumerPluginId}`, + action: "plugin_secret_shared_read", + resourceType: "plugin", + resourceId: entry.providerPluginId, + resourceName: entry.providerPluginName, + details: `${entry.consumerPluginId} read shared secret "${entry.providerPluginId}:${entry.key}" (resolved: ${entry.resolved})`, + success: entry.success, + errorMessage: entry.errorMessage, + }); + } catch { + // Auditing must never break the caller. + } +} + /** Undoes everything the ctx handed out. Safe to call more than once. */ export async function disposeInProcessHandle( handle: InProcessHandle, @@ -239,6 +333,21 @@ export async function disposeInProcessHandle( } handle.providedServices = []; + // Withdrawing the offers is what turns a borrower's getShared into a plain + // null instead of a handle pointing at a resolver whose plugin is gone. The + // sweep afterwards is belt and braces: an offer made outside the ctx, or one + // whose push was lost to a crash mid-activate, would otherwise outlive the + // plugin that backs it. + for (const registration of handle.offeredSecrets) { + secretRegistry.withdrawSecret( + registration.pluginId, + registration.key, + registration, + ); + } + handle.offeredSecrets = []; + secretRegistry.withdrawAllForPlugin(pluginId); + for (const unsubscribe of handle.unsubscribers) { try { unsubscribe(); diff --git a/src/backend/plugins/loader.ts b/src/backend/plugins/loader.ts index d95ab016e..1df8fe95a 100644 --- a/src/backend/plugins/loader.ts +++ b/src/backend/plugins/loader.ts @@ -55,6 +55,10 @@ import { type PluginModule, } from "./host-ctx.js"; import { resolveRequirements } from "./service-registry.js"; +import { + resolveSecretRequirements, + withdrawAllForPlugin, +} from "./secret-registry.js"; export type PluginState = | "loaded" @@ -361,11 +365,25 @@ export class PluginLoader { ); } + // Secret references never block activation, even when not optional. A + // borrowed secret is resolved per call and returns null when absent, so + // a provider installed later starts working with no restart -- refusing + // to start here would turn a recoverable gap into a hard ordering + // dependency between two installs. + const secretResolution = resolveSecretRequirements(plugin.manifest); + for (const reference of secretResolution.unavailable) { + pluginLogger.info( + `Plugin ${plugin.id} shared secret "${reference}" is not currently offered; reads will resolve to null`, + { operation: "plugin_activate" }, + ); + } + const handle: InProcessHandle = { module: { activate, deactivate }, unsubscribers: [], providedKeys: [], providedServices: [], + offeredSecrets: [], }; const ctx = createInProcessContext(plugin.manifest, handle); @@ -576,6 +594,11 @@ export class PluginLoader { return; } + // A worker plugin cannot offer a shared secret today, but terminate() is + // the one path every tier goes through, so the sweep belongs here rather + // than only in the in-process branch above. + withdrawAllForPlugin(plugin.id); + const worker = plugin.worker; if (!worker) return; diff --git a/src/backend/plugins/manifest.ts b/src/backend/plugins/manifest.ts index 93285c448..1dd5d7742 100644 --- a/src/backend/plugins/manifest.ts +++ b/src/backend/plugins/manifest.ts @@ -81,6 +81,12 @@ const API_VERSION_PATTERN = /^[0-9]+$/; const API_PREFIX_PATTERN = /^[a-z0-9-]+$/; /** Service names are dotted, e.g. "ssh.transport" or "host-metrics.stream". */ const SERVICE_PATTERN = /^[a-z0-9-]+(\.[a-z0-9-]+)+$/; +/** + * A shared secret key is a flat slug, e.g. "api-key". Not dotted: the plugin + * id is already a separate field, so a dotted key would only invite encoding + * the owner twice and disagreeing with itself. + */ +const SECRET_KEY_PATTERN = /^[a-z0-9-]+$/; /** * Action and slot ids are dotted like service names, but each segment may be * camelCase: these name a frontend function ("ai.openWithContext"), not a @@ -145,6 +151,13 @@ export interface PluginManifest { provides?: PluginServiceProvide[]; /** Named services this plugin needs another active plugin to provide. */ requires?: PluginServiceRequire[]; + /** + * Secrets this plugin offers other plugins by reference. Gated by one of + * this plugin's own permissionGroup permissions -- see parseManifest. + */ + providesSecret?: PluginSecretProvide[]; + /** Secrets this plugin wants to borrow from another plugin, by reference. */ + requiresSecret?: PluginSecretRequire[]; sidecars: Array<{ id: string; binary: string }>; } @@ -162,6 +175,28 @@ export interface PluginServiceRequire { optional?: boolean; } +export interface PluginSecretProvide { + /** The secret's name within this plugin, e.g. "api-key". */ + key: string; + /** The role permission a borrowing user needs. */ + permission: string; + /** Shown in the admin UI so the sharing is legible without reading code. */ + descriptionKey?: string; +} + +export interface PluginSecretRequire { + /** The plugin id that offers the secret. */ + plugin: string; + /** The secret's name within that plugin. */ + key: string; + /** + * When true, the provider being absent is normal and activation proceeds. + * A non-optional reference still activates -- the read just returns null -- + * but resolveSecretRequirements reports it as an error for install feedback. + */ + optional?: boolean; +} + /** * A named frontend action, declared here and bound at runtime. * @@ -300,6 +335,8 @@ export function validateManifest(manifest: unknown): string[] { errors.push(...validateProvides(m.provides)); errors.push(...validateRequires(m.requires)); + errors.push(...validateProvidesSecret(m.providesSecret)); + errors.push(...validateRequiresSecret(m.requiresSecret)); if (m.contributes && typeof m.contributes === "object") { errors.push( @@ -389,6 +426,92 @@ function validateRequires(requires: unknown): string[] { return errors; } +function validateProvidesSecret(providesSecret: unknown): string[] { + if (providesSecret === undefined) return []; + if (!Array.isArray(providesSecret)) { + return ['Field "providesSecret" must be an array']; + } + + const errors: string[] = []; + const seen = new Set(); + + providesSecret.forEach((entry: unknown, index: number) => { + if (!entry || typeof entry !== "object" || Array.isArray(entry)) { + errors.push(`providesSecret[${index}] must be an object`); + return; + } + const e = entry as Record; + + if (typeof e.key !== "string" || !SECRET_KEY_PATTERN.test(e.key)) { + errors.push( + `providesSecret[${index}].key must match ${SECRET_KEY_PATTERN}, got: "${String(e.key)}"`, + ); + } else if (seen.has(e.key)) { + errors.push(`providesSecret[${index}].key is a duplicate: "${e.key}"`); + } else { + seen.add(e.key); + } + + if (typeof e.permission !== "string" || e.permission.length === 0) { + errors.push(`providesSecret[${index}].permission is required`); + } + + if ( + "descriptionKey" in e && + (typeof e.descriptionKey !== "string" || e.descriptionKey.length === 0) + ) { + errors.push(`providesSecret[${index}].descriptionKey must be a string`); + } + }); + + return errors; +} + +function validateRequiresSecret(requiresSecret: unknown): string[] { + if (requiresSecret === undefined) return []; + if (!Array.isArray(requiresSecret)) { + return ['Field "requiresSecret" must be an array']; + } + + const errors: string[] = []; + const seen = new Set(); + + requiresSecret.forEach((entry: unknown, index: number) => { + if (!entry || typeof entry !== "object" || Array.isArray(entry)) { + errors.push(`requiresSecret[${index}] must be an object`); + return; + } + const e = entry as Record; + + if (typeof e.plugin !== "string" || !ID_PATTERN.test(e.plugin)) { + errors.push( + `requiresSecret[${index}].plugin must match ${ID_PATTERN}, got: "${String(e.plugin)}"`, + ); + } + + if (typeof e.key !== "string" || !SECRET_KEY_PATTERN.test(e.key)) { + errors.push( + `requiresSecret[${index}].key must match ${SECRET_KEY_PATTERN}, got: "${String(e.key)}"`, + ); + } + + if (typeof e.plugin === "string" && typeof e.key === "string") { + const id = `${e.plugin}:${e.key}`; + if (seen.has(id)) { + errors.push(`requiresSecret[${index}] is a duplicate: "${id}"`); + } else { + seen.add(id); + } + } + + if ("optional" in e && typeof e.optional !== "boolean") { + errors.push(`requiresSecret[${index}].optional must be a boolean`); + } + }); + + return errors; +} + function validateContributes(contributes: Record): string[] { const errors: string[] = []; @@ -644,6 +767,32 @@ export function parseManifest(raw: unknown): { } } + // Same rule for a shared secret. This is what keeps sharing on the existing + // RBAC grant path instead of becoming a separate mechanism: the permission + // gating a borrowed secret is an ordinary role permission in the provider's + // own group, editable per role or per user like any other. + for (const entry of manifest.providesSecret ?? []) { + if (!declaredPermissions.includes(entry.permission)) { + return { + errors: [ + `Secret "${entry.key}" is gated by "${entry.permission}", which is not declared in contributes.permissionGroup.permissions`, + ], + }; + } + } + + // A plugin borrowing from itself would bypass nothing, but it is always a + // mistake: its own secrets are reachable through ctx.secrets directly. + for (const entry of manifest.requiresSecret ?? []) { + if (entry.plugin === manifest.id) { + return { + errors: [ + `requiresSecret entry for "${entry.key}" names this plugin itself; use ctx.secrets.get instead`, + ], + }; + } + } + // Same rule for UI actions, for the same reason: a permission the catalog // never sees is one no admin can grant, so the action would be invisible // forever rather than merely denied. diff --git a/src/backend/plugins/secret-registry.ts b/src/backend/plugins/secret-registry.ts new file mode 100644 index 000000000..db25e44d3 --- /dev/null +++ b/src/backend/plugins/secret-registry.ts @@ -0,0 +1,362 @@ +/** + * Cross-plugin secret references. + * + * A plugin that holds a secret (the AI assistant and its provider API key) can + * offer it to other plugins by name, without ever handing over a copy. A + * requiring plugin declares `requiresSecret` in its manifest and calls + * ctx.secrets.getShared(pluginId, key) per request; the value is resolved from + * the provider at that moment and returned for that one call. + * + * ## Why a resolver and not a stored value + * + * The registration holds a `resolve` callback owned by the providing plugin, + * never the secret itself. That is the whole point of the design: + * + * - The provider can rotate or clear the underlying secret at any time. The + * next getShared() runs the new resolver and sees the new value, with no + * invalidation step to forget and no cached copy to go stale. + * - There is nothing for the requiring plugin to persist. It gets a string + * back from one call. It could of course write that string somewhere + * itself, exactly as it could with any value it is legitimately given -- + * what it cannot do is hold a handle that keeps working after the provider + * revokes, because every call re-resolves through the provider. + * - Uninstalling the provider deletes the registration, so every later + * getShared() returns null rather than throwing on a dangling handle. A + * missing dependency is a normal condition here, unlike a service call, + * which is why this returns null instead of the service registry's + * PluginServiceUnavailableError. + * + * ## Authorization is the provider's own permission + * + * No fourth grant mechanism. The provider declares which of its own role + * permissions gates sharing ("ai.secrets.share", alongside "ai.services.use"), + * and getShared runs the acting user through the same PermissionManager check + * that service-registry.ts and requirePermission() use. So: + * + * - An admin can let one user's requests reuse the shared key and deny + * another's, purely in the role editor, with no reinstall. + * - Revoking the RBAC permission takes effect on the very next call, exactly + * like revoking the secret itself. + * - The check is per-user and per-call, while `requiresSecret` in the + * manifest is per-install. They are independent on purpose: whether the + * plugin may ask is a deploy-time decision, whether this user's request + * gets a value is a runtime one. + * + * A denial is an error (the caller asked for something it is not allowed to + * have), while an absent secret is null (the caller asked for something that + * legitimately is not there). Collapsing the two would let a consumer treat + * "you may not" as "not configured" and fall back to something weaker. + */ + +import { pluginLogger } from "../utils/logger.js"; +import type { PluginManifest, PluginSecretRequire } from "./manifest.js"; + +export interface SecretRegistration { + /** The plugin offering the secret. */ + pluginId: string; + /** Display name, for audit lines. */ + pluginName: string; + /** The secret's name within that plugin, e.g. "api-key". */ + key: string; + /** Role permission the acting user needs. Declared by the provider. */ + permission: string; + /** + * Returns the current value, or null when the provider has nothing set. + * Called on every read so a rotation is picked up with no invalidation. + */ + resolve: (userId: string) => Promise | string | null; + /** Bumped on every offer, so a re-registered provider invalidates nothing stale. */ + generation: number; +} + +/** Keyed by `${pluginId}:${key}`. */ +const secrets = new Map(); +let generationCounter = 0; + +function mapKey(pluginId: string, key: string): string { + return `${pluginId}:${key}`; +} + +/** + * Thrown when the acting user lacks the provider's sharing permission. + * + * Mirrors PluginServicePermissionError: `status` and `body` are the 403 shape + * requirePermission() sends, so a consumer serving an HTTP request can pass it + * straight through. + */ +export class PluginSecretPermissionError extends Error { + readonly code = "EPLUGINSECRETPERM"; + readonly status = 403; + readonly body: { error: string; required: string }; + + constructor(pluginId: string, key: string, permission: string) { + super( + `You do not have access to the "${key}" secret shared by "${pluginId}". ` + + `It requires the "${permission}" permission.`, + ); + this.name = "PluginSecretPermissionError"; + this.body = { error: "Insufficient permissions", required: permission }; + } +} + +/** Thrown when a read cannot be attributed to a user. */ +export class PluginSecretActorError extends Error { + readonly code = "EPLUGINSECRETACTOR"; + + constructor(pluginId: string, key: string) { + super( + `A read of "${pluginId}:${key}" has no acting user. Pass one with ` + + `ctx.secrets.forUser(userId).getShared(...).`, + ); + this.name = "PluginSecretActorError"; + } +} + +/** Thrown when a plugin asks for a secret its manifest never declared. */ +export class PluginSecretUndeclaredError extends Error { + readonly code = "EPLUGINSECRETUNDECLARED"; + + constructor(consumerPluginId: string, pluginId: string, key: string) { + super( + `Plugin ${consumerPluginId} cannot read "${pluginId}:${key}": it is not ` + + `declared in the manifest's requiresSecret array`, + ); + this.name = "PluginSecretUndeclaredError"; + } +} + +export function offerSecret( + registration: Omit, +): SecretRegistration { + const id = mapKey(registration.pluginId, registration.key); + const existing = secrets.get(id); + if (existing) { + pluginLogger.warn( + `Shared secret "${id}" is already offered and is being replaced`, + { operation: "plugin_secret_registry" }, + ); + } + + const entry: SecretRegistration = { + ...registration, + generation: ++generationCounter, + }; + secrets.set(id, entry); + return entry; +} + +/** + * Withdraws an offer, identity-checked like revokeService: a provider that + * crashed and restarted must not withdraw the replacement its own restart + * installed. + * + * Nothing is remembered afterwards. A service keeps a tombstone so a held + * handle throws a named error, but a secret reference has no handle to keep + * alive -- the next read simply finds nothing and returns null. + */ +export function withdrawSecret( + pluginId: string, + key: string, + registration?: SecretRegistration, +): boolean { + const id = mapKey(pluginId, key); + const current = secrets.get(id); + if (!current) return false; + if (registration && current.generation !== registration.generation) { + return false; + } + return secrets.delete(id); +} + +/** Withdraws every offer made by one plugin. Used when it is uninstalled. */ +export function withdrawAllForPlugin(pluginId: string): number { + let removed = 0; + for (const [id, registration] of secrets) { + if (registration.pluginId === pluginId) { + secrets.delete(id); + removed++; + } + } + return removed; +} + +export function getSecretRegistration( + pluginId: string, + key: string, +): SecretRegistration | undefined { + return secrets.get(mapKey(pluginId, key)); +} + +export function listSecrets(): SecretRegistration[] { + return [...secrets.values()]; +} + +/** Test seam. */ +export function clearSecretRegistry(): void { + secrets.clear(); +} + +export interface SecretRequirementResolution { + /** Declared requirements whose provider is present and has a value offered. */ + available: string[]; + /** Declared requirements with no matching offer right now. */ + unavailable: string[]; + /** Human-readable reasons for each unsatisfied non-optional requirement. */ + errors: string[]; +} + +/** + * Structural resolution of a manifest's `requiresSecret`. + * + * Like resolveRequirements in service-registry.ts, this deliberately does NOT + * consult any user's permissions: activation is per-instance, permissions are + * per-user. It only asks whether the named plugin currently offers the key. + * + * Availability here is a snapshot for logging and install-time feedback. It is + * never cached into the call path -- getShared re-resolves every time, so a + * provider that appears after activation starts working with no restart. + */ +export function resolveSecretRequirements( + manifest: PluginManifest, +): SecretRequirementResolution { + const available: string[] = []; + const unavailable: string[] = []; + const errors: string[] = []; + + for (const requirement of manifest.requiresSecret ?? []) { + const id = mapKey(requirement.plugin, requirement.key); + if (secrets.has(id)) { + available.push(id); + continue; + } + + unavailable.push(id); + if (!requirement.optional) { + errors.push( + `requires secret "${requirement.key}" from plugin "${requirement.plugin}", which is not currently offered`, + ); + } + } + + return { available, unavailable, errors }; +} + +export interface SecretReadContext { + /** Resolves the user this read acts as. */ + resolveUserId: () => string | undefined; + /** Checks a role permission for a user. */ + hasPermission: (userId: string, permission: string) => Promise; + /** Writes the audit line. Must never throw. Awaited before the value returns. */ + audit: (entry: SecretAuditEntry) => void | Promise; +} + +export interface SecretAuditEntry { + userId: string; + consumerPluginId: string; + providerPluginId: string; + providerPluginName: string; + key: string; + /** False for a denial, true for an allowed read whether or not a value existed. */ + success: boolean; + /** Whether a value actually came back. Never the value itself. */ + resolved: boolean; + errorMessage?: string; +} + +/** + * Reads a shared secret for one call. + * + * Returns null, rather than throwing, when the provider is not installed or + * has nothing set. Those are ordinary conditions a consumer is expected to + * handle (fall back to its own key, or tell the user to configure one), and an + * optional dependency that threw would force every consumer to wrap each read. + * + * Throws only for the two cases that are genuinely wrong: a plugin reading + * something its manifest never declared, and a user without the permission. + */ +export async function readSharedSecret( + consumerManifest: PluginManifest, + providerPluginId: string, + key: string, + context: SecretReadContext, +): Promise { + const consumerPluginId = consumerManifest.id; + + // Declared in the manifest, the same rule services.provide enforces: a + // plugin cannot reach a secret it never asked for, even if an admin granted + // the permission for some other reason. + const declared = (consumerManifest.requiresSecret ?? []).find( + (entry: PluginSecretRequire) => + entry.plugin === providerPluginId && entry.key === key, + ); + if (!declared) { + throw new PluginSecretUndeclaredError( + consumerPluginId, + providerPluginId, + key, + ); + } + + const registration = secrets.get(mapKey(providerPluginId, key)); + + // Provider absent: uninstalled, disabled, or never installed. Indistinguish- + // able on purpose, and all four are the same answer to the consumer -- which + // is also what makes an uninstall clean rather than a dangling handle. + if (!registration) return null; + + const userId = context.resolveUserId(); + if (!userId) { + throw new PluginSecretActorError(providerPluginId, key); + } + + const allowed = await context.hasPermission(userId, registration.permission); + if (!allowed) { + const error = new PluginSecretPermissionError( + providerPluginId, + key, + registration.permission, + ); + await context.audit({ + userId, + consumerPluginId, + providerPluginId: registration.pluginId, + providerPluginName: registration.pluginName, + key, + success: false, + resolved: false, + errorMessage: error.message, + }); + throw error; + } + + try { + const value = await registration.resolve(userId); + const resolved = typeof value === "string" && value.length > 0; + + await context.audit({ + userId, + consumerPluginId, + providerPluginId: registration.pluginId, + providerPluginName: registration.pluginName, + key, + success: true, + resolved, + }); + + return resolved ? value : null; + } catch (error) { + await context.audit({ + userId, + consumerPluginId, + providerPluginId: registration.pluginId, + providerPluginName: registration.pluginName, + key, + success: false, + resolved: false, + errorMessage: error instanceof Error ? error.message : String(error), + }); + // A provider whose own lookup failed is not a permission problem, and the + // consumer should not read it as "not configured" and fall back. + throw error; + } +} diff --git a/src/backend/plugins/worker-bootstrap.ts b/src/backend/plugins/worker-bootstrap.ts index 5f63cae4c..68c0de834 100644 --- a/src/backend/plugins/worker-bootstrap.ts +++ b/src/backend/plugins/worker-bootstrap.ts @@ -256,6 +256,12 @@ const ctx = { secrets: { get: notImplemented("secrets.get"), set: notImplemented("secrets.set"), + // Cross-plugin sharing is in-process only for now. Named here rather than + // left undefined so a worker plugin that tries it gets the same explicit + // error as any other unimplemented ctx call. + offer: notImplemented("secrets.offer"), + withdraw: notImplemented("secrets.withdraw"), + getShared: notImplemented("secrets.getShared"), }, notify: notImplemented("notify"), fetch: notImplemented("fetch"), diff --git a/src/backend/tests/plugins/manifest.test.ts b/src/backend/tests/plugins/manifest.test.ts index 091f96f07..f816dd1f9 100644 --- a/src/backend/tests/plugins/manifest.test.ts +++ b/src/backend/tests/plugins/manifest.test.ts @@ -150,6 +150,116 @@ describe("plugin manifest validation", () => { }); }); + describe("providesSecret / requiresSecret", () => { + const permissionGroup = { + group: "testplugin", + permissions: ["testplugin.secrets.share"], + }; + + it("accepts a well-formed pair", () => { + expect( + validateManifest( + validManifest({ + providesSecret: [ + { key: "api-key", permission: "testplugin.secrets.share" }, + ], + requiresSecret: [ + { plugin: "ai-assistant", key: "api-key", optional: true }, + ], + contributes: { permissionGroup }, + }), + ), + ).toEqual([]); + }); + + it("rejects a dotted secret key", () => { + // The owning plugin is already its own field, so a dotted key would only + // encode the owner twice and invite the two disagreeing. + const errors = validateManifest( + validManifest({ + providesSecret: [ + { key: "ai.api-key", permission: "testplugin.secrets.share" }, + ], + }), + ); + expect(errors.some((e) => e.includes("providesSecret[0].key"))).toBe( + true, + ); + }); + + it("rejects a duplicate offer of the same key", () => { + const errors = validateManifest( + validManifest({ + providesSecret: [ + { key: "api-key", permission: "testplugin.secrets.share" }, + { key: "api-key", permission: "testplugin.secrets.share" }, + ], + }), + ); + expect(errors.some((e) => e.includes("duplicate"))).toBe(true); + }); + + it("requires a permission on an offer", () => { + const errors = validateManifest( + validManifest({ providesSecret: [{ key: "api-key" }] }), + ); + expect( + errors.some((e) => e.includes("providesSecret[0].permission")), + ).toBe(true); + }); + + it("rejects a reference with a bad plugin id", () => { + const errors = validateManifest( + validManifest({ + requiresSecret: [{ plugin: "Bad_Id", key: "api-key" }], + }), + ); + expect(errors.some((e) => e.includes("requiresSecret[0].plugin"))).toBe( + true, + ); + }); + + it("rejects a duplicate reference to the same plugin and key", () => { + const errors = validateManifest( + validManifest({ + requiresSecret: [ + { plugin: "ai-assistant", key: "api-key" }, + { plugin: "ai-assistant", key: "api-key" }, + ], + }), + ); + expect(errors.some((e) => e.includes("duplicate"))).toBe(true); + }); + + it("refuses a shared secret gated by a permission the group never declares", () => { + // Same rule as a service: sharing rides on an ordinary role permission, + // so it has to be one an admin can actually grant. + const { manifest, errors } = parseManifest( + validManifest({ + providesSecret: [ + { key: "api-key", permission: "testplugin.undeclared" }, + ], + contributes: { permissionGroup }, + }), + ); + + expect(manifest).toBeUndefined(); + expect(errors[0]).toContain("testplugin.undeclared"); + expect(errors[0]).toContain("permissionGroup"); + }); + + it("refuses a plugin borrowing from itself", () => { + const { manifest, errors } = parseManifest( + validManifest({ + requiresSecret: [{ plugin: "sample-plugin", key: "api-key" }], + }), + ); + + expect(manifest).toBeUndefined(); + expect(errors[0]).toContain("names this plugin itself"); + }); + }); + it("rejects an unknown permission", () => { const errors = validateManifest( validManifest({ permissions: ["credentials.read"] }), diff --git a/src/backend/tests/plugins/secret-contract.test.ts b/src/backend/tests/plugins/secret-contract.test.ts new file mode 100644 index 000000000..6eda86e18 --- /dev/null +++ b/src/backend/tests/plugins/secret-contract.test.ts @@ -0,0 +1,394 @@ +/** + * Two real plugins on disk, one offering a secret and one borrowing it, driven + * through the actual PluginLoader. + * + * Same borrowed-id trick as service-contract.test.ts: the in-process allowlist + * is hardcoded and deliberately unfakeable, so the fixtures use real + * first-party ids to reach that tier. The secret ("api-key") and the + * permission gating it ("testplugin.secrets.share") are fixture-owned, so + * nothing about the shipped ssh-terminal or docker plugins is under test. + * + * What the unit test cannot show, and this does: that the manifest declaration + * is enforced by the real ctx, and that a genuine uninstall through + * loader.deactivate() leaves the borrower with null rather than a handle onto + * a resolver whose plugin is gone. + */ + +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; +import fs from "node:fs"; +import path from "node:path"; +import type { Fixture } from "./fixture-plugin.js"; + +const state = vi.hoisted(() => ({ + /** Permissions the acting user currently holds. Mutated mid-test. */ + granted: new Set(), + auditEntries: [] as Record[], +})); + +vi.mock("../../utils/logger.js", () => ({ + pluginLogger: { + debug: vi.fn(), + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + success: vi.fn(), + }, +})); + +vi.mock("../../utils/audit-logger.js", () => ({ + logAudit: vi.fn(async (params: Record) => { + state.auditEntries.push(params); + }), + getAuditUsername: vi.fn(async (userId: string) => `user:${userId}`), +})); + +vi.mock("../../utils/permission-manager.js", () => ({ + PermissionManager: { + getInstance: () => ({ + hasPermission: async (_userId: string, permission: string) => + state.granted.has(permission), + invalidateUserPermissionCache: vi.fn(), + }), + }, +})); + +const { PluginLoader } = await import("../../plugins/loader.js"); +const { TRANSPORT_OWNER_CAPABILITY } = + await import("../../plugins/first-party.js"); +const { + clearSecretRegistry, + getSecretRegistration, + PluginSecretPermissionError, + PluginSecretUndeclaredError, +} = await import("../../plugins/secret-registry.js"); +const { pluginEvents } = await import("../../plugins/events.js"); +const { createFixturePlugin } = await import("./fixture-plugin.js"); + +const PROVIDER_ID = "ssh-terminal"; +const CONSUMER_ID = "docker"; +const KEY = "api-key"; +const PERMISSION = "testplugin.secrets.share"; +const SECRET = "sk-fixture-value"; + +/** + * The provider offers a resolver reading the current value from a file, so the + * test can rotate or clear it from outside without restarting the plugin. A + * module-level variable would not work: the plugin is imported by URL and + * lives in its own ESM module instance. + */ +function providerSource(valueFile: string): string { + const literal = JSON.stringify(valueFile); + return ` +import fs from "node:fs"; +export async function activate(ctx) { + ctx.secrets.offer(${JSON.stringify(KEY)}, async () => { + if (!fs.existsSync(${literal})) return null; + const value = fs.readFileSync(${literal}, "utf8"); + return value.length > 0 ? value : null; + }); +} +`; +} + +/** + * The consumer stashes a bound read on globalThis so the test can invoke it as + * a specific user afterwards. Note it stashes a FUNCTION, not a value: there + * is no handle to hold, which is the property under test. + */ +const CONSUMER_SOURCE = ` +export async function activate(ctx) { + globalThis.__borrow = (userId) => + ctx.secrets.getShared(${JSON.stringify(PROVIDER_ID)}, ${JSON.stringify(KEY)}, { userId }); + globalThis.__borrowUndeclared = (userId) => + ctx.secrets.getShared(${JSON.stringify(PROVIDER_ID)}, "not-declared", { userId }); +} +`; + +function providerFixture(): Fixture { + return createFixturePlugin({ + id: PROVIDER_ID, + permissions: [TRANSPORT_OWNER_CAPABILITY], + manifestOverrides: { + category: "Terminal", + providesSecret: [{ key: KEY, permission: PERMISSION }], + contributes: { + permissionGroup: { group: "testplugin", permissions: [PERMISSION] }, + }, + }, + }); +} + +function consumerFixture( + requiresSecret: Array> = [ + { plugin: PROVIDER_ID, key: KEY, optional: true }, + ], +): Fixture { + return createFixturePlugin({ + id: CONSUMER_ID, + permissions: [TRANSPORT_OWNER_CAPABILITY], + backendSource: CONSUMER_SOURCE, + manifestOverrides: { category: "Infrastructure", requiresSecret }, + }); +} + +function borrow(userId = "user-1"): Promise { + const fn = (globalThis as Record).__borrow as ( + id: string, + ) => Promise; + return fn(userId); +} + +function borrowUndeclared(userId = "user-1"): Promise { + const fn = (globalThis as Record).__borrowUndeclared as ( + id: string, + ) => Promise; + return fn(userId); +} + +describe("cross-plugin secret references", () => { + let provider: Fixture | null = null; + let consumer: Fixture | null = null; + let loader: InstanceType | null = null; + let valueFile = ""; + + beforeEach(() => { + state.granted = new Set([PERMISSION]); + state.auditEntries = []; + clearSecretRegistry(); + pluginEvents.clear(); + }); + + afterEach(async () => { + await loader?.shutdown(); + loader = null; + provider?.cleanup(); + consumer?.cleanup(); + provider = null; + consumer = null; + delete (globalThis as Record).__borrow; + delete (globalThis as Record).__borrowUndeclared; + clearSecretRegistry(); + pluginEvents.clear(); + }); + + async function activateProvider() { + provider = providerFixture(); + valueFile = path.join(provider.root, "secret.txt"); + fs.writeFileSync(valueFile, SECRET); + fs.writeFileSync( + path.join(provider.dir, "backend", "index.mjs"), + providerSource(valueFile), + ); + + loader ??= new PluginLoader(); + const plugin = await loader.load(provider.dir); + await loader.activate(plugin.id); + return plugin; + } + + async function activateConsumer( + requiresSecret?: Array>, + ) { + consumer = requiresSecret + ? consumerFixture(requiresSecret) + : consumerFixture(); + + loader ??= new PluginLoader(); + const plugin = await loader.load(consumer.dir); + await loader.activate(plugin.id); + return plugin; + } + + async function activateBoth() { + loader = new PluginLoader(); + const providerPlugin = await activateProvider(); + const consumerPlugin = await activateConsumer(); + return { providerPlugin, consumerPlugin }; + } + + it("registers the offer when the provider activates", async () => { + const { providerPlugin, consumerPlugin } = await activateBoth(); + + expect(providerPlugin.state).toBe("active"); + expect(consumerPlugin.state).toBe("active"); + + const registration = getSecretRegistration(PROVIDER_ID, KEY); + expect(registration?.pluginId).toBe(PROVIDER_ID); + expect(registration?.permission).toBe(PERMISSION); + }); + + it("resolves for a user who holds the permission", async () => { + await activateBoth(); + + await expect(borrow()).resolves.toBe(SECRET); + }); + + it("denies a user without the permission, though the secret is set", async () => { + await activateBoth(); + state.granted = new Set(); + + await expect(borrow("user-2")).rejects.toBeInstanceOf( + PluginSecretPermissionError, + ); + // Still installed, still set: the difference is purely this user's role. + expect(getSecretRegistration(PROVIDER_ID, KEY)).toBeDefined(); + expect(fs.readFileSync(valueFile, "utf8")).toBe(SECRET); + }); + + it("lets one user borrow while another is denied, with no reinstall", async () => { + await activateBoth(); + + // The grant is per-user, so both answers come from the same live install. + await expect(borrow("allowed-user")).resolves.toBe(SECRET); + + state.granted = new Set(); + await expect(borrow("denied-user")).rejects.toBeInstanceOf( + PluginSecretPermissionError, + ); + + state.granted = new Set([PERMISSION]); + await expect(borrow("allowed-user")).resolves.toBe(SECRET); + }); + + it("returns null when the provider is installed but the key is not set", async () => { + await activateBoth(); + fs.writeFileSync(valueFile, ""); + + await expect(borrow()).resolves.toBeNull(); + }); + + it("returns null when the providing plugin was never installed", async () => { + loader = new PluginLoader(); + await activateConsumer(); + + // The consumer activates regardless: the reference is resolved per call. + await expect(borrow()).resolves.toBeNull(); + }); + + it("activates the consumer even when a non-optional reference is absent", async () => { + loader = new PluginLoader(); + const plugin = await activateConsumer([{ plugin: PROVIDER_ID, key: KEY }]); + + // Unlike a service requirement, an absent secret provider is recoverable + // by installing it later, so it must not be a hard ordering dependency. + expect(plugin.state).toBe("active"); + await expect(borrow()).resolves.toBeNull(); + }); + + it("refuses a read the consumer's manifest never declared", async () => { + await activateBoth(); + + await expect(borrowUndeclared()).rejects.toBeInstanceOf( + PluginSecretUndeclaredError, + ); + }); + + it("refuses to offer a secret the provider's manifest never declared", async () => { + provider = createFixturePlugin({ + id: PROVIDER_ID, + permissions: [TRANSPORT_OWNER_CAPABILITY], + backendSource: ` +export async function activate(ctx) { + ctx.secrets.offer("undeclared-key", () => "nope"); +} +`, + manifestOverrides: { + category: "Terminal", + providesSecret: [{ key: KEY, permission: PERMISSION }], + contributes: { + permissionGroup: { group: "testplugin", permissions: [PERMISSION] }, + }, + }, + }); + + loader = new PluginLoader(); + const plugin = await loader.load(provider.dir); + await loader.activate(plugin.id).catch(() => undefined); + + expect(plugin.state).not.toBe("active"); + expect( + getSecretRegistration(PROVIDER_ID, "undeclared-key"), + ).toBeUndefined(); + }); + + describe("the provider stays in control", () => { + it("serves a rotated value on the next borrow", async () => { + await activateBoth(); + await expect(borrow()).resolves.toBe(SECRET); + + fs.writeFileSync(valueFile, "sk-fixture-rotated"); + + // Nothing to invalidate: the borrower never held a copy to go stale. + await expect(borrow()).resolves.toBe("sk-fixture-rotated"); + }); + + it("nulls the borrow out when the provider clears the secret", async () => { + await activateBoth(); + await expect(borrow()).resolves.toBe(SECRET); + + fs.rmSync(valueFile); + + await expect(borrow()).resolves.toBeNull(); + }); + }); + + describe("uninstalling the provider", () => { + it("nulls out the reference rather than leaving a dangling handle", async () => { + const { providerPlugin } = await activateBoth(); + await expect(borrow()).resolves.toBe(SECRET); + + await loader!.deactivate(providerPlugin.id); + + // The borrower is untouched: still active, still permitted, still + // declaring the reference. It just gets null now, and does not throw. + expect(getSecretRegistration(PROVIDER_ID, KEY)).toBeUndefined(); + await expect(borrow()).resolves.toBeNull(); + }); + + it("keeps serving nothing even though the underlying value still exists", async () => { + const { providerPlugin } = await activateBoth(); + await loader!.deactivate(providerPlugin.id); + + // The file is still on disk. Access went away with the plugin that + // mediated it, which is the point: there was never a copy to fall back on. + expect(fs.readFileSync(valueFile, "utf8")).toBe(SECRET); + await expect(borrow()).resolves.toBeNull(); + }); + + it("resumes when the provider is reinstalled, with no borrower restart", async () => { + const { providerPlugin } = await activateBoth(); + await loader!.deactivate(providerPlugin.id); + await expect(borrow()).resolves.toBeNull(); + + await loader!.activate(providerPlugin.id); + + await expect(borrow()).resolves.toBe(SECRET); + }); + }); + + describe("audit", () => { + it("records a borrow without the value", async () => { + await activateBoth(); + await borrow("user-9"); + + const entry = state.auditEntries.at(-1); + expect(entry).toMatchObject({ + userId: "user-9", + username: `plugin:${CONSUMER_ID}`, + action: "plugin_secret_shared_read", + resourceId: PROVIDER_ID, + success: true, + }); + expect(JSON.stringify(state.auditEntries)).not.toContain(SECRET); + }); + + it("records a denial without the value", async () => { + await activateBoth(); + state.granted = new Set(); + await borrow().catch(() => undefined); + + expect(state.auditEntries.at(-1)).toMatchObject({ success: false }); + expect(JSON.stringify(state.auditEntries)).not.toContain(SECRET); + }); + }); +}); diff --git a/src/backend/tests/plugins/secret-registry.test.ts b/src/backend/tests/plugins/secret-registry.test.ts new file mode 100644 index 000000000..4f2a14c58 --- /dev/null +++ b/src/backend/tests/plugins/secret-registry.test.ts @@ -0,0 +1,424 @@ +/** + * Cross-plugin secret references: the permission gate, the two null cases, and + * what an uninstall does to a borrower still pointing at the provider. + * + * The distinction under test throughout is between a denial (an error, because + * the caller asked for something it may not have) and an absence (null, because + * the thing legitimately is not there). A consumer that cannot tell those apart + * would read "you may not" as "not configured" and fall back to something + * weaker, so every case below asserts which of the two it got. + */ + +import { afterEach, beforeEach, describe, expect, it, vi } from "vitest"; + +vi.mock("../../utils/logger.js", () => ({ + pluginLogger: { + debug: vi.fn(), + info: vi.fn(), + warn: vi.fn(), + error: vi.fn(), + success: vi.fn(), + }, +})); + +const { + clearSecretRegistry, + getSecretRegistration, + listSecrets, + offerSecret, + readSharedSecret, + resolveSecretRequirements, + withdrawAllForPlugin, + withdrawSecret, + PluginSecretActorError, + PluginSecretPermissionError, + PluginSecretUndeclaredError, +} = await import("../../plugins/secret-registry.js"); + +const PROVIDER = "ai-assistant"; +const KEY = "api-key"; +const PERMISSION = "ai.secrets.share"; +const SECRET = "sk-live-value"; + +/** Permissions the acting user currently holds. Mutated mid-test. */ +let granted = new Set(); +let auditEntries: Array> = []; + +function offer(overrides: Partial[0]> = {}) { + return offerSecret({ + pluginId: PROVIDER, + pluginName: "AI Assistant", + key: KEY, + permission: PERMISSION, + resolve: () => SECRET, + ...overrides, + }); +} + +/** A consumer manifest declaring the reference, as the real one would. */ +function consumer( + requiresSecret: Array<{ plugin: string; key: string; optional?: boolean }> = [ + { plugin: PROVIDER, key: KEY, optional: true }, + ], +) { + return { id: "reporting-plugin", requiresSecret } as never; +} + +function context(userId: string | undefined) { + return { + resolveUserId: () => userId, + hasPermission: async (_userId: string, permission: string) => + granted.has(permission), + audit: (entry: Record) => { + auditEntries.push(entry); + }, + } as never; +} + +function read( + manifest = consumer(), + userId: string | undefined = "user-1", + providerPluginId = PROVIDER, + key = KEY, +) { + return readSharedSecret(manifest, providerPluginId, key, context(userId)); +} + +/** + * A read with no acting user. Separate from read() on purpose: a default + * parameter cannot express this, since passing undefined explicitly would + * just fall back to the default and quietly test the wrong thing. + */ +function readAnonymously(manifest = consumer()) { + return readSharedSecret(manifest, PROVIDER, KEY, context(undefined)); +} + +describe("shared secret registry", () => { + beforeEach(() => { + clearSecretRegistry(); + granted = new Set([PERMISSION]); + auditEntries = []; + }); + afterEach(() => clearSecretRegistry()); + + describe("registration", () => { + it("round-trips an offer", () => { + const registration = offer(); + + expect(getSecretRegistration(PROVIDER, KEY)).toBe(registration); + expect(listSecrets()).toHaveLength(1); + expect(registration.generation).toBeGreaterThan(0); + }); + + it("keys offers by plugin as well as name", () => { + offer(); + offer({ pluginId: "other-plugin", resolve: () => "other-value" }); + + expect(listSecrets()).toHaveLength(2); + expect(getSecretRegistration("other-plugin", KEY)?.pluginId).toBe( + "other-plugin", + ); + }); + + it("only withdraws the offer that is actually current", () => { + const first = offer(); + const second = offer(); + + // A crashed-and-restarted provider must not withdraw the replacement its + // own restart installed. + expect(withdrawSecret(PROVIDER, KEY, first)).toBe(false); + expect(getSecretRegistration(PROVIDER, KEY)).toBe(second); + + expect(withdrawSecret(PROVIDER, KEY, second)).toBe(true); + expect(getSecretRegistration(PROVIDER, KEY)).toBeUndefined(); + }); + + it("reports a withdraw of something never offered", () => { + expect(withdrawSecret(PROVIDER, "nothing")).toBe(false); + }); + }); + + describe("reading", () => { + it("resolves for a user who holds the permission", async () => { + offer(); + + await expect(read()).resolves.toBe(SECRET); + }); + + it("denies a user without the permission, though the secret exists", async () => { + offer(); + granted = new Set(); + + // The whole point of the per-call check: the plugin is installed, the + // key is set, and this user still gets nothing. + await expect(read()).rejects.toBeInstanceOf(PluginSecretPermissionError); + expect(getSecretRegistration(PROVIDER, KEY)).toBeDefined(); + }); + + it("denies with the 403 shape a route can pass straight through", async () => { + offer(); + granted = new Set(); + + const error = await read().catch((caught) => caught); + + expect(error.status).toBe(403); + expect(error.body).toEqual({ + error: "Insufficient permissions", + required: PERMISSION, + }); + }); + + it("never runs the provider's resolver for a denied read", async () => { + const resolve = vi.fn(() => SECRET); + offer({ resolve }); + granted = new Set(); + + await expect(read()).rejects.toBeInstanceOf(PluginSecretPermissionError); + expect(resolve).not.toHaveBeenCalled(); + }); + + it("returns null when the provider is installed but the key is not set", async () => { + offer({ resolve: () => null }); + + // Absence, not denial: the user may read it, there is just nothing there. + await expect(read()).resolves.toBeNull(); + }); + + it("treats an empty string as not set", async () => { + offer({ resolve: () => "" }); + + await expect(read()).resolves.toBeNull(); + }); + + it("returns null when the providing plugin is not installed at all", async () => { + await expect(read()).resolves.toBeNull(); + }); + + it("gives the same answer for absent and unset, so neither leaks install state", async () => { + const unset = await read(); + offer({ resolve: () => null }); + const absent = await read(); + + expect(unset).toBeNull(); + expect(absent).toBeNull(); + }); + + it("refuses a plugin reading something its manifest never declared", async () => { + offer(); + + await expect(read(consumer([]), "user-1")).rejects.toBeInstanceOf( + PluginSecretUndeclaredError, + ); + }); + + it("refuses a declared reference to a different key on the same plugin", async () => { + offer(); + + await expect( + read(consumer([{ plugin: PROVIDER, key: "other-key" }])), + ).rejects.toBeInstanceOf(PluginSecretUndeclaredError); + }); + + it("refuses a read that cannot be attributed to a user", async () => { + offer(); + + await expect(readAnonymously()).rejects.toBeInstanceOf( + PluginSecretActorError, + ); + }); + + it("returns null for an anonymous read of an absent provider", async () => { + // Absence is settled before the actor is, so a borrower whose provider + // is gone gets the same null whether or not it had a user to act as. + await expect(readAnonymously()).resolves.toBeNull(); + }); + + it("passes the acting user to the resolver, so per-user keys stay per-user", async () => { + const resolve = vi.fn((userId: string) => `key-for-${userId}`); + offer({ resolve }); + + await expect(read(consumer(), "user-7")).resolves.toBe("key-for-user-7"); + expect(resolve).toHaveBeenCalledWith("user-7"); + }); + + it("propagates a resolver failure rather than reporting it as unset", async () => { + offer({ + resolve: () => { + throw new Error("vault unreachable"); + }, + }); + + // Reported as null, a consumer would fall back to a weaker path on what + // is actually a transient provider fault. + await expect(read()).rejects.toThrow("vault unreachable"); + }); + }); + + describe("revocation takes effect immediately", () => { + it("stops resolving the moment the permission is revoked", async () => { + offer(); + await expect(read()).resolves.toBe(SECRET); + + granted = new Set(); + + // No restart, no reinstall: the next call is already denied. + await expect(read()).rejects.toBeInstanceOf(PluginSecretPermissionError); + }); + + it("serves the rotated value on the next read, with nothing to invalidate", async () => { + let current = SECRET; + offer({ resolve: () => current }); + + await expect(read()).resolves.toBe(SECRET); + current = "sk-live-rotated"; + await expect(read()).resolves.toBe("sk-live-rotated"); + }); + + it("nulls out a borrower's reference when the provider clears the secret", async () => { + let current: string | null = SECRET; + offer({ resolve: () => current }); + + await expect(read()).resolves.toBe(SECRET); + current = null; + await expect(read()).resolves.toBeNull(); + }); + }); + + describe("uninstalling the provider", () => { + it("nulls out the reference rather than leaving a dangling handle", async () => { + offer(); + await expect(read()).resolves.toBe(SECRET); + + expect(withdrawAllForPlugin(PROVIDER)).toBe(1); + + // The borrower is still installed, still declares the reference, and + // still holds the permission. It simply gets null now, the same answer + // as a provider that was never installed -- no throw, no stale value. + await expect(read()).resolves.toBeNull(); + expect(getSecretRegistration(PROVIDER, KEY)).toBeUndefined(); + }); + + it("keeps no copy of the value behind after an uninstall", async () => { + const resolve = vi.fn(() => SECRET); + offer({ resolve }); + await read(); + resolve.mockClear(); + + withdrawAllForPlugin(PROVIDER); + + // Nothing cached the earlier read: the answer comes from the absence of + // a registration, not from a remembered value. + await expect(read()).resolves.toBeNull(); + expect(resolve).not.toHaveBeenCalled(); + }); + + it("withdraws every key the plugin offered, not just the one read", () => { + offer(); + offer({ key: "webhook-secret" }); + offer({ pluginId: "unrelated-plugin" }); + + expect(withdrawAllForPlugin(PROVIDER)).toBe(2); + expect(listSecrets()).toHaveLength(1); + expect(getSecretRegistration("unrelated-plugin", KEY)).toBeDefined(); + }); + + it("starts working again if the provider is reinstalled, with no borrower restart", async () => { + offer(); + withdrawAllForPlugin(PROVIDER); + await expect(read()).resolves.toBeNull(); + + offer({ resolve: () => "sk-live-reinstalled" }); + + await expect(read()).resolves.toBe("sk-live-reinstalled"); + }); + }); + + describe("requirement resolution", () => { + it("reports an offered reference as available", () => { + offer(); + + const resolution = resolveSecretRequirements(consumer()); + + expect(resolution.available).toEqual([`${PROVIDER}:${KEY}`]); + expect(resolution.unavailable).toEqual([]); + expect(resolution.errors).toEqual([]); + }); + + it("reports an absent optional reference without an error", () => { + const resolution = resolveSecretRequirements(consumer()); + + expect(resolution.unavailable).toEqual([`${PROVIDER}:${KEY}`]); + expect(resolution.errors).toEqual([]); + }); + + it("reports an absent required reference as an error", () => { + const resolution = resolveSecretRequirements( + consumer([{ plugin: PROVIDER, key: KEY }]), + ); + + expect(resolution.errors).toHaveLength(1); + expect(resolution.errors[0]).toContain(PROVIDER); + }); + + it("ignores any user's permissions, because activation is per-instance", () => { + offer(); + granted = new Set(); + + // A user without the grant must get a clear denial from the call, not a + // plugin that mysteriously refuses to start. + expect(resolveSecretRequirements(consumer()).available).toHaveLength(1); + }); + + it("is empty for a manifest that borrows nothing", () => { + const resolution = resolveSecretRequirements({ id: "plain" } as never); + + expect(resolution.available).toEqual([]); + expect(resolution.unavailable).toEqual([]); + expect(resolution.errors).toEqual([]); + }); + }); + + describe("audit", () => { + it("records an allowed read without the value", async () => { + offer(); + await read(); + + expect(auditEntries).toHaveLength(1); + expect(auditEntries[0]).toMatchObject({ + userId: "user-1", + consumerPluginId: "reporting-plugin", + providerPluginId: PROVIDER, + key: KEY, + success: true, + resolved: true, + }); + expect(JSON.stringify(auditEntries[0])).not.toContain(SECRET); + }); + + it("records a denial", async () => { + offer(); + granted = new Set(); + await read().catch(() => undefined); + + expect(auditEntries[0]).toMatchObject({ + success: false, + resolved: false, + }); + }); + + it("distinguishes an allowed read that found nothing", async () => { + offer({ resolve: () => null }); + await read(); + + expect(auditEntries[0]).toMatchObject({ success: true, resolved: false }); + }); + + it("writes nothing when the provider is not installed", async () => { + await read(); + + // There is no provider to attribute the read to, and no decision was + // made about the user -- an audit line here would be noise. + expect(auditEntries).toEqual([]); + }); + }); +}); diff --git a/src/ui/locales/en.json b/src/ui/locales/en.json index 5df7bfa37..ddc2e3f62 100644 --- a/src/ui/locales/en.json +++ b/src/ui/locales/en.json @@ -5049,7 +5049,10 @@ "noProvidersConfigured": "No AI provider is configured yet. Add one in the AI assistant tab.", "runsInOpenTerminal": "Runs in your open terminal session.", "runInTerminal": "Run", - "terminalContextPrefill": "Here is my terminal output:\n\n{{output}}\n\n" + "terminalContextPrefill": "Here is my terminal output:\n\n{{output}}\n\n", + "secrets": { + "shareApiKey": "Let other plugins use your AI provider API key for their own requests. The key is never copied, and you can revoke this at any time." + } }, "macros": { "title": "Macros",