diff --git a/packages/plugins/sdk/src/define-plugin.ts b/packages/plugins/sdk/src/define-plugin.ts index 3c894672e5..d2fc523222 100644 --- a/packages/plugins/sdk/src/define-plugin.ts +++ b/packages/plugins/sdk/src/define-plugin.ts @@ -166,6 +166,31 @@ export interface PluginApiResponse { body?: unknown; } +// --------------------------------------------------------------------------- +// Config change context +// --------------------------------------------------------------------------- + +/** + * Scope metadata delivered alongside a `configChanged` RPC so the worker knows + * *which company's* configuration changed. + * + * The host→worker `configChanged` message has always carried the company scope, + * but the SDK historically dropped it before invoking `onConfigChanged`, leaving + * proactive plugins to keep a single worker-global config. That is safe for a + * single-tenant plugin but silently collapses a multi-company plugin onto + * whichever company's config was delivered last. Threading the scope through + * lets a `multiCompanyConfig` plugin maintain per-company state. + * + * @see PLUGIN_SPEC.md §13.4 — `configChanged` + */ +export interface PluginConfigChangeContext { + /** + * The company whose configuration changed, or `null` for an instance/global + * save that is not bound to a specific company. + */ + companyId: string | null; +} + // --------------------------------------------------------------------------- // Plugin definition // --------------------------------------------------------------------------- @@ -207,6 +232,22 @@ export interface PluginDefinition { */ onHealth?(): Promise; + /** + * When true, this plugin's worker correctly serves configuration from more + * than one company inside a single worker process — for example by keying its + * state on `context.companyId` in `onConfigChanged` and running one connection + * / subscription set per company. + * + * When false or omitted (the default), the plugin is treated as single-tenant. + * The host then **fails closed** if `configChanged` would ever deliver a + * second, distinct company's configuration to the same worker: instead of + * silently collapsing the worker onto whichever company arrived last (a + * cross-tenant identity/secret confusion bug), the delivery is rejected with + * `PLUGIN_RPC_ERROR_CODES.CROSS_TENANT_CONFIG`. Re-delivering an unchanged + * config for a different company (idempotent replay) is still allowed. + */ + multiCompanyConfig?: boolean; + /** * Called when the operator updates this plugin's company-scoped configuration * at runtime, without restarting the worker. @@ -214,9 +255,16 @@ export interface PluginDefinition { * If not implemented, the host restarts the worker to apply the new config. * * @param newConfig - The newly resolved configuration + * @param context - Scope of the change. `context.companyId` identifies the + * company whose config changed (null for an instance/global save). A + * multi-company plugin (`multiCompanyConfig: true`) MUST key its per-company + * state on this value rather than assuming a single global config. * @see PLUGIN_SPEC.md §13.4 — `configChanged` */ - onConfigChanged?(newConfig: Record): Promise; + onConfigChanged?( + newConfig: Record, + context?: PluginConfigChangeContext, + ): Promise; /** * Called when the host is about to shut down the plugin worker. diff --git a/packages/plugins/sdk/src/index.ts b/packages/plugins/sdk/src/index.ts index 1e6d3260e0..1dabc362e3 100644 --- a/packages/plugins/sdk/src/index.ts +++ b/packages/plugins/sdk/src/index.ts @@ -94,6 +94,7 @@ export type { PluginDefinition, PaperclipPlugin, PluginHealthDiagnostics, + PluginConfigChangeContext, PluginConfigValidationResult, PluginWebhookInput, PluginApiRequestInput, diff --git a/packages/plugins/sdk/src/protocol.ts b/packages/plugins/sdk/src/protocol.ts index 2f35e5b90b..ad11522d93 100644 --- a/packages/plugins/sdk/src/protocol.ts +++ b/packages/plugins/sdk/src/protocol.ts @@ -257,6 +257,14 @@ export const PLUGIN_RPC_ERROR_CODES = { METHOD_NOT_IMPLEMENTED: -32004, /** The worker→host call attempted to escape the current invocation company scope. */ INVOCATION_SCOPE_DENIED: -32005, + /** + * A `configChanged` delivery would have collapsed a single-tenant worker onto + * a second, distinct company's configuration. The worker fails closed instead + * of silently overwriting the already-applied tenant's config. A plugin that + * genuinely serves multiple companies from one worker must opt in via + * `multiCompanyConfig: true` on its definition. + */ + CROSS_TENANT_CONFIG: -32006, /** A catch-all for errors that do not fit other categories. */ UNKNOWN: -32099, } as const; diff --git a/packages/plugins/sdk/src/worker-rpc-host.ts b/packages/plugins/sdk/src/worker-rpc-host.ts index 076751034d..72dcf9238a 100644 --- a/packages/plugins/sdk/src/worker-rpc-host.ts +++ b/packages/plugins/sdk/src/worker-rpc-host.ts @@ -201,6 +201,32 @@ function realpathOrResolvedPath(filePath: string): string { } } +/** + * Order-independent structural equality for two plugin config objects. + * + * Config arrives as parsed JSON, so plain `JSON.stringify` comparison is + * sensitive to key ordering across independent saves. Canonicalizing with + * recursively sorted object keys makes an idempotent replay of the same config + * compare equal regardless of serialization order. + */ +function configsEqual(a: unknown, b: unknown): boolean { + return canonicalize(a) === canonicalize(b); +} + +function canonicalize(value: unknown): string { + if (value === null || typeof value !== "object") { + return JSON.stringify(value) ?? "null"; + } + if (Array.isArray(value)) { + return `[${value.map(canonicalize).join(",")}]`; + } + const entries = Object.entries(value as Record) + .filter(([, v]) => v !== undefined) + .sort(([left], [right]) => (left < right ? -1 : left > right ? 1 : 0)) + .map(([key, v]) => `${JSON.stringify(key)}:${canonicalize(v)}`); + return `{${entries.join(",")}}`; +} + export function isWorkerEntrypoint(entry: string, moduleUrl: string): boolean { const thisFile = realpathOrResolvedPath(fileURLToPath(moduleUrl)); const entryPath = realpathOrResolvedPath(entry); @@ -294,6 +320,10 @@ export function startWorkerRpcHost(options: WorkerRpcHostOptions): WorkerRpcHost let initialized = false; let manifest: PaperclipPluginManifestV1 | null = null; let currentConfig: Record = {}; + // The company whose config was last applied via configChanged. Used to fail + // closed when a single-tenant plugin would be collapsed onto a second, + // distinct company's config. `null` until the first company-scoped delivery. + let configCompanyId: string | null = null; let databaseNamespace: string | null = null; const invocationContextStorage = new AsyncLocalStorage(); @@ -1584,10 +1614,52 @@ export function startWorkerRpcHost(options: WorkerRpcHostOptions): WorkerRpcHost } async function handleConfigChanged(params: ConfigChangedParams): Promise { + const incomingCompanyId = params.companyId ?? null; + + // Fail-closed cross-tenant guard. + // + // A worker is spawned once per plugin (not per company), so a proactive + // plugin that keeps a single worker-global config would silently collapse + // onto whichever company's config was delivered last if configChanged is + // called for more than one distinct company — for example the startup + // config replay fanning out every stored company's config, or two operators + // saving configs for different companies. That is a cross-tenant identity / + // secret confusion bug (one company's bot token applied to another's work). + // + // Reject the second, distinct company unless the plugin explicitly declares + // it handles multiple companies in one worker (multiCompanyConfig). An + // idempotent replay of the *same* config for a different company id is + // harmless (single-tenant plugins commonly have duplicate scope rows that + // all embed the same config), so it is allowed. + if ( + !plugin.definition.multiCompanyConfig && + incomingCompanyId !== null && + configCompanyId !== null && + configCompanyId !== incomingCompanyId && + !configsEqual(params.config, currentConfig) + ) { + throw Object.assign( + new Error( + `configChanged: refusing to overwrite configuration for company ` + + `"${configCompanyId}" with a different configuration for company ` + + `"${incomingCompanyId}". This plugin is single-tenant and cannot ` + + `safely serve multiple companies from one worker. If multi-company ` + + `support is intended, set multiCompanyConfig: true on the plugin ` + + `definition and key per-company state on context.companyId.`, + ), + { code: PLUGIN_RPC_ERROR_CODES.CROSS_TENANT_CONFIG }, + ); + } + currentConfig = params.config; + if (incomingCompanyId !== null) { + configCompanyId = incomingCompanyId; + } if (plugin.definition.onConfigChanged) { - await plugin.definition.onConfigChanged(params.config); + await plugin.definition.onConfigChanged(params.config, { + companyId: incomingCompanyId, + }); } } diff --git a/packages/plugins/sdk/tests/worker-rpc-host.test.ts b/packages/plugins/sdk/tests/worker-rpc-host.test.ts index d41aa863af..8b0c709550 100644 --- a/packages/plugins/sdk/tests/worker-rpc-host.test.ts +++ b/packages/plugins/sdk/tests/worker-rpc-host.test.ts @@ -296,3 +296,191 @@ describe("worker invocation scope propagation", () => { } }); }); + +describe("worker configChanged cross-tenant guard", () => { + // Spin up a worker-rpc-host wired to in-memory streams and expose a + // request/response `callWorker` plus `initialize`/`stop` helpers. + function makeWorker(plugin: ReturnType) { + const hostToWorker = new PassThrough(); + const workerToHost = new PassThrough(); + const hostReadline = createInterface({ input: workerToHost }); + const pending = new Map void>(); + let nextRequestId = 1; + + const worker = startWorkerRpcHost({ + plugin, + stdin: hostToWorker, + stdout: workerToHost, + }); + + function callWorker(method: string, params: unknown) { + const id = `host-${nextRequestId++}`; + const result = new Promise((resolve, reject) => { + pending.set(id, (response) => { + if ("error" in response && response.error) { + reject( + Object.assign(new Error(response.error.message), { + code: response.error.code, + }), + ); + return; + } + resolve((response as { result?: unknown }).result); + }); + }); + hostToWorker.write(serializeMessage(createRequest(method, params, id))); + return result; + } + + hostReadline.on("line", (line) => { + const message = parseMessage(line); + if (!isJsonRpcResponse(message)) return; + pending.get(String(message.id))?.(message); + pending.delete(String(message.id)); + }); + + async function initialize() { + await callWorker("initialize", { + manifest: { + id: "paperclip.config-guard-test", + apiVersion: 1, + version: "1.0.0", + displayName: "Config Guard Test", + description: "Test plugin", + author: "Paperclip", + categories: ["automation"], + capabilities: [], + entrypoints: {}, + }, + config: {}, + databaseNamespace: null, + }); + } + + function stop() { + worker.stop(); + hostReadline.close(); + hostToWorker.destroy(); + workerToHost.destroy(); + } + + return { callWorker, initialize, stop }; + } + + it("fails closed when a second, distinct company's config would overwrite a single-tenant worker", async () => { + const applied: Array<{ companyId: string | null; token: unknown }> = []; + const plugin = definePlugin({ + async setup() {}, + async onConfigChanged(newConfig, context) { + applied.push({ + companyId: context?.companyId ?? null, + token: newConfig.slackBotToken, + }); + }, + }); + const { callWorker, initialize, stop } = makeWorker(plugin); + + try { + await initialize(); + + // Company A's config is delivered first (deterministic ORDER BY companyId + // in the loader) and applied. + await expect( + callWorker("configChanged", { + config: { companyId: "company-a", slackBotToken: "xoxb-A" }, + companyId: "company-a", + }), + ).resolves.toBeNull(); + + // Company B's *distinct* config must be rejected rather than silently + // collapsing the single worker onto B's bot token (the vulnerability). + await expect( + callWorker("configChanged", { + config: { companyId: "company-b", slackBotToken: "xoxb-B" }, + companyId: "company-b", + }), + ).rejects.toMatchObject({ + code: PLUGIN_RPC_ERROR_CODES.CROSS_TENANT_CONFIG, + }); + + // The worker stayed bound to company A; company B never reached the + // plugin. Against the pre-fix code this array would be + // [company-a, company-b] (last-write-wins collapse). + expect(applied).toEqual([{ companyId: "company-a", token: "xoxb-A" }]); + } finally { + stop(); + } + }); + + it("allows an idempotent replay of the same config under a different scope row", async () => { + // Mirrors the live single-tenant gateway: several plugin_config rows keyed + // by distinct row companyIds but all embedding the same config. Replaying + // them must be a no-op, not a fail-closed rejection. + const appliedScopes: Array = []; + const plugin = definePlugin({ + async setup() {}, + async onConfigChanged(_newConfig, context) { + appliedScopes.push(context?.companyId ?? null); + }, + }); + const { callWorker, initialize, stop } = makeWorker(plugin); + + try { + await initialize(); + const embedded = { companyId: "company-a", slackBotToken: "xoxb-A" }; + + await callWorker("configChanged", { + config: { ...embedded }, + companyId: "row-scope-1", + }); + await expect( + callWorker("configChanged", { + config: { ...embedded }, + companyId: "row-scope-2", + }), + ).resolves.toBeNull(); + + expect(appliedScopes).toEqual(["row-scope-1", "row-scope-2"]); + } finally { + stop(); + } + }); + + it("threads per-company config to a plugin that opts into multiCompanyConfig", async () => { + const applied: Array<{ companyId: string | null; token: unknown }> = []; + const plugin = definePlugin({ + multiCompanyConfig: true, + async setup() {}, + async onConfigChanged(newConfig, context) { + applied.push({ + companyId: context?.companyId ?? null, + token: newConfig.slackBotToken, + }); + }, + }); + const { callWorker, initialize, stop } = makeWorker(plugin); + + try { + await initialize(); + + await callWorker("configChanged", { + config: { companyId: "company-a", slackBotToken: "xoxb-A" }, + companyId: "company-a", + }); + await expect( + callWorker("configChanged", { + config: { companyId: "company-b", slackBotToken: "xoxb-B" }, + companyId: "company-b", + }), + ).resolves.toBeNull(); + + // Both companies' configs delivered, each tagged with its own scope. + expect(applied).toEqual([ + { companyId: "company-a", token: "xoxb-A" }, + { companyId: "company-b", token: "xoxb-B" }, + ]); + } finally { + stop(); + } + }); +}); diff --git a/server/src/services/plugin-loader.ts b/server/src/services/plugin-loader.ts index aded537056..21fc986607 100644 --- a/server/src/services/plugin-loader.ts +++ b/server/src/services/plugin-loader.ts @@ -32,6 +32,7 @@ import path from "node:path"; import { fileURLToPath, pathToFileURL } from "node:url"; import { promisify } from "node:util"; import type { Db } from "@paperclipai/db"; +import { PLUGIN_RPC_ERROR_CODES } from "@paperclipai/plugin-sdk"; import type { PaperclipPluginManifestV1, PluginLauncherDeclaration, @@ -2195,15 +2196,27 @@ export function pluginLoader( companyId: row.companyId, }); } catch (configErr) { - log.debug( - { - pluginId, - pluginKey, - companyId: row.companyId, - err: configErr instanceof Error ? configErr.message : String(configErr), - }, - "plugin-loader: startup config delivery skipped for company", - ); + // A single-tenant worker fails closed (CROSS_TENANT_CONFIG) rather + // than collapse onto a second company's config — surface that at + // warn so the misconfiguration (multiple distinct companies + // configured for a single-tenant plugin) is visible, instead of + // being lost in the best-effort debug stream. + const code = (configErr as { code?: number } | null)?.code; + const details = { + pluginId, + pluginKey, + companyId: row.companyId, + code, + err: configErr instanceof Error ? configErr.message : String(configErr), + }; + if (code === PLUGIN_RPC_ERROR_CODES.CROSS_TENANT_CONFIG) { + log.warn( + details, + "plugin-loader: startup config delivery rejected — single-tenant plugin configured for multiple companies", + ); + } else { + log.debug(details, "plugin-loader: startup config delivery skipped for company"); + } } } } catch (listErr) { diff --git a/server/src/services/plugin-registry.ts b/server/src/services/plugin-registry.ts index 344cc26c9c..9e2da31954 100644 --- a/server/src/services/plugin-registry.ts +++ b/server/src/services/plugin-registry.ts @@ -297,12 +297,18 @@ export function pluginRegistryService(db: Db) { * proactive plugin that never runs inside a company-scoped invocation (and * therefore cannot resolve `ctx.config.get(companyId)`) still receives its * configuration at startup. + * + * Ordered deterministically by companyId: the startup replay delivers these + * rows to a single worker via `configChanged`, and a single-tenant worker + * binds to the first company it sees. Without a stable order the worker + * would bind to a nondeterministic (DB-dependent) company across restarts. */ listConfigs: (pluginId: string) => db .select() .from(pluginConfig) - .where(eq(pluginConfig.pluginId, pluginId)), + .where(eq(pluginConfig.pluginId, pluginId)) + .orderBy(asc(pluginConfig.companyId)), /** * Create or fully replace a plugin's company-scoped configuration.