From 7d13b2a9fe6d51c3c087fde3441b6941100af4c4 Mon Sep 17 00:00:00 2001 From: ZacharyZcR Date: Wed, 14 Oct 2026 08:20:09 +0800 Subject: [PATCH] fix: don't auto-enable synced plugins signed only by a server-sent key (#1623) * fix: don't auto-enable synced plugins signed only by a server-sent key Registry keys a linked server sends still let its plugins load on the desktop, but no longer count as a signer for switching them on or copying their grants. Keys the desktop added itself keep their trust. * fix: enforce local plugin trust across remote source labels * fix: require positive local trust for synced plugin keys --- src/backend/plugins/registries.ts | 30 ++++-- src/backend/sync/client/plugins.ts | 41 +++++--- src/backend/tests/plugins/registries.test.ts | 21 ++++ src/backend/tests/sync/client/plugins.test.ts | 97 ++++++++++++++++++- 4 files changed, 168 insertions(+), 21 deletions(-) diff --git a/src/backend/plugins/registries.ts b/src/backend/plugins/registries.ts index ad690e8b7..a2d837cd1 100644 --- a/src/backend/plugins/registries.ts +++ b/src/backend/plugins/registries.ts @@ -19,6 +19,9 @@ import { type RegistrySource, } from "./registry-index.js"; import { + COMMUNITY_PLUGIN_KEYS, + TRUSTED_PLUGIN_KEYS, + getCustomPluginKeys, isPluginPublicKey, pluginKeyId, setCustomPluginKeys, @@ -28,6 +31,8 @@ import { const CONFIG_KEY = "plugin_registries"; /** Keys a linked desktop copied from its server, so mirrored plugins load. */ const SYNCED_KEYS_KEY = "plugin_registry_synced_keys"; +/** `addedIn` of a key only the linked server vouches for. */ +const SYNCED = "synced"; const ID_PATTERN = /^[a-z0-9][a-z0-9-]*$/; const MAX_ID_LENGTH = 40; const MAX_NAME_LENGTH = 80; @@ -117,8 +122,8 @@ async function writeRegistriesConfig(config: RegistriesConfig): Promise { await applyCustomKeys(config); } -function toKey(publicKey: string): TrustedPluginKey { - return { id: pluginKeyId(publicKey), publicKey, addedIn: "admin" }; +function toKey(publicKey: string, addedIn = "admin"): TrustedPluginKey { + return { id: pluginKeyId(publicKey), publicKey, addedIn }; } async function readSyncedKeys(): Promise { @@ -134,19 +139,32 @@ async function readSyncedKeys(): Promise { /** * Trusts the key of every custom registry, on or off: a plugin installed - * from a registry that was later turned off still has to load. + * from a registry that was later turned off still has to load. A key added + * here wins over the same key synced from a server. */ async function applyCustomKeys(config: RegistriesConfig): Promise { const keys = new Map(); + for (const publicKey of await readSyncedKeys()) { + keys.set(pluginKeyId(publicKey), toKey(publicKey, SYNCED)); + } for (const entry of config.custom) { keys.set(entry.keyId, toKey(entry.publicKey)); } - for (const publicKey of await readSyncedKeys()) { - keys.set(pluginKeyId(publicKey), toKey(publicKey)); - } setCustomPluginKeys([...keys.values()]); } +/** Whether this app trusts the key independently of its linked server. */ +export function isLocallyTrustedPluginKey(keyId: string): boolean { + return ( + [...TRUSTED_PLUGIN_KEYS, ...COMMUNITY_PLUGIN_KEYS].some( + (key) => key.id === keyId, + ) || + getCustomPluginKeys().some( + (key) => key.id === keyId && key.addedIn !== SYNCED, + ) + ); +} + /** Called before the boot scan so plugins from custom registries load. */ export async function loadCustomRegistryKeys(): Promise { try { diff --git a/src/backend/sync/client/plugins.ts b/src/backend/sync/client/plugins.ts index 8a0669954..8aabf6213 100644 --- a/src/backend/sync/client/plugins.ts +++ b/src/backend/sync/client/plugins.ts @@ -21,7 +21,10 @@ import { unloadPlugin, } from "../../plugins/index.js"; import { getPluginsDir } from "../../plugins/paths.js"; -import { setSyncedRegistryKeys } from "../../plugins/registries.js"; +import { + isLocallyTrustedPluginKey, + setSyncedRegistryKeys, +} from "../../plugins/registries.js"; import type { LoadedPlugin } from "../../plugins/loader.js"; import { createCurrentPluginPermissionGrantRepository, @@ -57,6 +60,14 @@ async function isEnabledLocally(pluginId: string): Promise { return record?.state === "enabled"; } +/** + * Signed by a key this desktop trusts on its own. A key the server sent + * would let the server's admin sign anything, so it does not count. + */ +function vouchedHere(local: LoadedPlugin): boolean { + return !!local.signedBy && isLocallyTrustedPluginKey(local.signedBy); +} + const PLUGIN_ID_PATTERN = /^[a-z0-9][a-z0-9-]*$/; /** Downloads next to the installed copy, which stays until the swap. */ @@ -155,7 +166,7 @@ async function matchGrants(plugin: RemotePlugin): Promise { if (!local || local.source !== "user") return; // Grants are only copied for code a trusted key signed. Anything else the // desktop's own admin has to review, as with any plugin dropped on disk. - if (!local.signedBy) return; + if (!vouchedHere(local)) return; const declared = new Set(local.manifest.capabilities); const wanted = new Set(plugin.granted.filter((cap) => declared.has(cap))); const existing = await repository.listByPlugin(plugin.id); @@ -196,7 +207,7 @@ async function alertUnsigned(plugin: RemotePlugin): Promise { alertedVersions.add(key); await sendCoreAlert({ title: "Review a feature from your server", - body: `${plugin.name} (${plugin.version}) was copied from your server but is not signed, so it stays off here until you turn it on.`, + body: `${plugin.name} (${plugin.version}) was copied from your server but is not signed by a key this app trusts, so it stays off here until you turn it on.`, severity: "warning", category: "termix.sync.unsigned_plugin", dedupeKey: `sync:${key}`, @@ -239,7 +250,9 @@ async function mirrorOne(link: SyncLink, plugin: RemotePlugin): Promise { await alertOlderDesktop(plugin); throw error; } - if (wasEnabled) await setPluginEnabled(plugin.id, true); + if (wasEnabled && local.source === "bundled") { + await setPluginEnabled(plugin.id, true); + } } else if (newer) { await alertOlderDesktop(plugin); } @@ -263,19 +276,19 @@ async function mirrorOne(link: SyncLink, plugin: RemotePlugin): Promise { } } await matchGrants(plugin); - // Unsigned code is only ever switched off from here, never on. - if (!local.signedBy) { - const enabledHere = await isEnabledLocally(plugin.id); - if (!plugin.enabled && enabledHere) { - await setPluginEnabled(plugin.id, false); - } else if (plugin.enabled && !enabledHere) { - await alertUnsigned(plugin); - } - return; - } } if (!local) return; + // Remote metadata cannot confer bundled trust on a locally loaded user plugin. + if (local.source === "user" && !vouchedHere(local)) { + const enabledHere = await isEnabledLocally(plugin.id); + if (!plugin.enabled && enabledHere) { + await setPluginEnabled(plugin.id, false); + } else if (plugin.enabled && !enabledHere) { + await alertUnsigned(plugin); + } + return; + } if (plugin.enabled !== (await isEnabledLocally(plugin.id))) { await setPluginEnabled(plugin.id, plugin.enabled); } else if (plugin.enabled && local.state !== "active") { diff --git a/src/backend/tests/plugins/registries.test.ts b/src/backend/tests/plugins/registries.test.ts index 905ee0243..f7b743d4a 100644 --- a/src/backend/tests/plugins/registries.test.ts +++ b/src/backend/tests/plugins/registries.test.ts @@ -256,4 +256,25 @@ describe("keys from a linked server", () => { await registries.setSyncedRegistryKeys([]); expect(getCustomPluginKeys()).toEqual([]); }); + + it("marks them as synced unless an admin added the same key here", async () => { + const synced = newKey(); + const shared = newKey(); + serve(URL_A, { registry: "acme", publicKey: shared }); + await registries.addRegistry(URL_A, pluginKeyId(shared)); + await registries.setSyncedRegistryKeys([synced, shared]); + + expect(registries.isLocallyTrustedPluginKey(pluginKeyId(synced))).toBe( + false, + ); + expect(registries.isLocallyTrustedPluginKey(pluginKeyId(shared))).toBe( + true, + ); + expect(registries.isLocallyTrustedPluginKey("ec40e29bcc4986b2")).toBe(true); + await registries.setSyncedRegistryKeys([]); + expect(registries.isLocallyTrustedPluginKey(pluginKeyId(synced))).toBe( + false, + ); + expect(registries.isLocallyTrustedPluginKey("unknown-key")).toBe(false); + }); }); diff --git a/src/backend/tests/sync/client/plugins.test.ts b/src/backend/tests/sync/client/plugins.test.ts index 569776a35..5581a898a 100644 --- a/src/backend/tests/sync/client/plugins.test.ts +++ b/src/backend/tests/sync/client/plugins.test.ts @@ -11,6 +11,8 @@ const state = vi.hoisted(() => ({ fetch: vi.fn(), loadBundled: vi.fn(), syncedKeys: vi.fn(async (_keys: unknown[]) => {}), + syncedOnlyKeys: new Set(), + grants: [] as string[], })); vi.mock("../../../utils/logger.js", () => ({ @@ -28,10 +30,16 @@ vi.mock("../../../database/repositories/factory.js", () => ({ state: state.enabled.has(id) ? "enabled" : "disabled", }), }), - createCurrentPluginPermissionGrantRepository: () => ({}), + createCurrentPluginPermissionGrantRepository: () => ({ + listByPlugin: async () => [], + grant: async ({ capability }: { capability: string }) => + void state.grants.push(capability), + }), })); vi.mock("../../../plugins/registries.js", () => ({ setSyncedRegistryKeys: state.syncedKeys, + isLocallyTrustedPluginKey: (keyId: string) => + keyId === "local-key" && !state.syncedOnlyKeys.has(keyId), })); vi.mock("../../../sync/client/http.js", () => ({ remoteFetch: state.fetch, @@ -58,6 +66,7 @@ vi.mock("../../../plugins/index.js", () => ({ const { mirrorPlugins } = await import("../../../sync/client/plugins.js"); const { remoteJson } = await import("../../../sync/client/http.js"); +const { sendCoreAlert } = await import("../../../notify/core-notify.js"); const bundled = { id: "ssh-terminal", @@ -92,6 +101,9 @@ beforeEach(() => { state.install.mockReset(); state.fetch.mockReset(); state.loadBundled.mockReset(); + state.syncedOnlyKeys = new Set(); + state.grants = []; + vi.mocked(sendCoreAlert).mockClear(); state.loadBundled.mockImplementation(async (id: string) => { const plugin = { ...bundled, id }; state.loaded.set(id, plugin); @@ -171,4 +183,87 @@ describe("mirrorPlugins", () => { expect(state.loaded.has("ssh-terminal")).toBe(true); expect(state.enabled.has("ssh-terminal")).toBe(true); }); + + describe("a user plugin the server has on", () => { + function serverRunsSignedBy(keyId: string) { + const version = keyId === "removed-key" ? "1.0.1" : "1.0.0"; + state.loaded.set("extra", { + id: "extra", + source: "user", + state: "loaded", + signedBy: keyId, + manifest: { id: "extra", version, capabilities: ["net"] }, + }); + vi.mocked(remoteJson).mockResolvedValue({ + plugins: [ + { + id: "extra", + name: "Extra", + version, + source: "user", + artifact: true, + enabled: true, + active: true, + desktop: "mirror", + syncEntities: [], + granted: ["net"], + }, + ], + }); + } + + it("turns it on when a key this desktop trusts signed it", async () => { + serverRunsSignedBy("local-key"); + + await mirrorPlugins({} as never); + + expect(state.enabled.has("extra")).toBe(true); + expect(state.grants).toEqual(["net"]); + }); + + it.each(["server-key", "removed-key"])( + "leaves it off for untrusted key %s", + async (keyId) => { + serverRunsSignedBy(keyId); + state.syncedOnlyKeys.add("server-key"); + + await mirrorPlugins({} as never); + + expect(state.enabled.has("extra")).toBe(false); + expect(state.grants).toEqual([]); + expect(sendCoreAlert).toHaveBeenCalledWith( + expect.objectContaining({ category: "termix.sync.unsigned_plugin" }), + ); + }, + ); + }); +}); + +it("server bundled label must not enable a synced-only user plugin", async () => { + state.syncedOnlyKeys.add("server-key"); + state.loaded.set("extra", { + id: "extra", + source: "user", + state: "loaded", + signedBy: "server-key", + manifest: { id: "extra", version: "1.0.0", capabilities: [] }, + }); + vi.mocked(remoteJson).mockResolvedValue({ + plugins: [ + { + id: "extra", + name: "Extra", + version: "1.0.0", + source: "bundled", + artifact: true, + enabled: true, + active: true, + desktop: "mirror", + syncEntities: [], + granted: [], + }, + ], + }); + await mirrorPlugins({} as never); + expect(state.enabled.has("extra")).toBe(false); });