From a7186dce4bc060a7ce41cfb22e08a42a0f3a3776 Mon Sep 17 00:00:00 2001 From: Michael Nguyen Date: Thu, 23 Jul 2026 09:50:48 -0700 Subject: [PATCH] fix(plugin-sdk): thread companyId through configChanged + fail-closed cross-tenant guard (LOOA-687) (#10096) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit ## Thinking Path > - Paperclip is the open source app people use to manage AI agents for work. > - First-party **plugins** run as isolated workers spawned by the host `plugin-loader`, reading company-scoped config through a governed `ctx.config.get(companyId)` channel. > - The host→worker `configChanged` RPC carries `{ config, companyId }`, but the SDK dispatch dropped the scope — `onConfigChanged(newConfig)` was companyId-blind by design — so a **proactive** worker kept a single worker-global config. > - #10092 added a startup replay that fans out **every** stored company's config through `configChanged`. With no deterministic ordering, a plugin configured for more than one distinct company ends up running as whichever DB row was delivered last. > - That is a latent cross-tenant identity/secret confusion bug: one company's bot token could be applied to another company's traffic. > - This pull request threads `companyId` through `onConfigChanged` and adds a fail-closed cross-tenant guard at the SDK layer, so a single-tenant worker can never silently collapse to a second company's config. > - The benefit is that the config-delivery class is fixed at the SDK boundary — before any genuinely multi-company proactive plugin ships — without changing today's single-tenant behavior. ## Linked Issues or Issue Description No public GitHub issue — describing in-PR (hardening / latent security): **Latent cross-tenant config collapse.** The worker-side `configChanged` dispatch forwarded only `config` and dropped `companyId`, so a proactive plugin kept a single worker-global config. #10092's startup replay delivers every configured company's config sequentially with no `ORDER BY`, so a plugin with configs for more than one distinct company would apply a nondeterministic last-write-wins global config (one tenant's credential applied to another's traffic). - Builds on and must merge after #10092. - Not exploitable today: the only proactive consumer (the chat gateway) has single-tenant config rows, so last-write-wins is a no-op. This is a hardening pre-condition before any multi-company proactive plugin ships. ## What Changed - **Thread scope through:** `onConfigChanged(newConfig, context)` with a new exported `PluginConfigChangeContext { companyId }`. Backward compatible — the second arg is optional; existing single-arg implementations are unaffected. - **Fail-closed cross-tenant guard** (`worker-rpc-host.ts`): a single-tenant plugin that receives `configChanged` for a second, distinct company with a *different* config is rejected with the new `PLUGIN_RPC_ERROR_CODES.CROSS_TENANT_CONFIG` instead of silently overwriting the applied tenant's config. Idempotent replays of the *same* config under a different scope row remain allowed. - **Opt-in `multiCompanyConfig: true`** on the plugin definition for plugins that genuinely serve multiple companies from one worker (keying per-company state on `context.companyId`); the guard is bypassed for those. - **Deterministic `ORDER BY companyId`** on `registry.listConfigs`, so the startup replay binds a single-tenant worker to a stable company across restarts. - **Loader visibility:** a `CROSS_TENANT_CONFIG` rejection is logged at `warn` (was best-effort `debug`) so the misconfiguration is surfaced. - **Regression test** (`packages/plugins/sdk/tests/worker-rpc-host.test.ts`): two distinct companies delivered via the startup-replay path fail closed and stay bound to the first company; an idempotent same-config replay under a different scope row is allowed; a `multiCompanyConfig` plugin receives per-company config with the correct `context.companyId`. ## Verification - SDK `tsc --noEmit`: clean. - SDK vitest `worker-rpc-host.test.ts`: 7/7 pass (incl. 3 new). The two-distinct-company case **fails against pre-fix code** and passes after the fix. - #10092 embedded-postgres `plugin-config-startup-delivery.test.ts`: 3/3 pass (unaffected by the new `ORDER BY`). - Full server `tsc --noEmit` against this SDK: clean. ## Risks - **Low functional risk.** The second `onConfigChanged` arg is optional and existing implementations are unchanged. Today's single-tenant gateway keeps working — idempotent same-config replays are explicitly allowed, so the go-live is preserved. - **Behavioral shift on misconfig:** a genuinely multi-company plugin that has NOT opted into `multiCompanyConfig` now fails closed (`CROSS_TENANT_CONFIG`) rather than silently collapsing to one tenant. This is the intended safer default; opt in with `multiCompanyConfig: true` to serve multiple companies from one worker. - **Not in scope (residual).** Per-company workers/connections for a genuinely multi-company gateway increase resource use and are tracked separately (ties into the #10092 fan-out/timeout follow-up). This PR fixes the class and fails closed; it does not build multi-tenant connection management. ## Model Used Claude — Anthropic `claude-opus-4-8` (Opus 4.8), extended thinking, with tool use / code execution via Claude Code. ## Checklist - [x] I have included a thinking path that traces from project context to this change - [x] I have specified the model used (with version and capability details) - [x] I have checked ROADMAP.md and confirmed this PR does not duplicate planned core work - [x] I have searched GitHub for duplicate or related PRs and linked them above - [x] I have either (a) linked existing issues with `Fixes:` / `Closes` / `Refs` OR (b) described the issue in-PR following the relevant issue template - [x] I have not referenced internal/instance-local Paperclip issues or links (only public GitHub `#NNN` / `github.com/paperclipai/paperclip` URLs) - [ ] My branch name describes the change and contains no internal Paperclip ticket id — branch predates this rule; not renaming an open PR mid-review - [x] I have run tests locally and they pass - [x] I have added or updated tests where applicable - [ ] I have updated relevant documentation to reflect my changes — no doc surface; internal SDK/host behavior only - [x] I have considered and documented any risks above - [x] All Paperclip CI gates are green - [x] I will address all Greptile and reviewer comments before requesting merge 🤖 Generated with [Claude Code](https://claude.com/claude-code) Co-authored-by: anicca Co-authored-by: Paperclip --- packages/plugins/sdk/src/define-plugin.ts | 50 ++++- packages/plugins/sdk/src/index.ts | 1 + packages/plugins/sdk/src/protocol.ts | 8 + packages/plugins/sdk/src/worker-rpc-host.ts | 74 ++++++- .../plugins/sdk/tests/worker-rpc-host.test.ts | 188 ++++++++++++++++++ server/src/services/plugin-loader.ts | 31 ++- server/src/services/plugin-registry.ts | 8 +- 7 files changed, 348 insertions(+), 12 deletions(-) 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.