diff --git a/docs/plugin-development.md b/docs/plugin-development.md index 85e42e4685..c92900612d 100644 --- a/docs/plugin-development.md +++ b/docs/plugin-development.md @@ -553,13 +553,22 @@ Plugins with `"permissions": ["nodeExecution"]` can run Node.js scripts in the E desktop app after the user allows the desktop permission prompt. Both built-in and uploaded (community) plugins may request `nodeExecution`. The grant is -issued by the Electron **main** process after a native consent dialog, and is bound to -the plugin id for the current app session (it is never persisted or synced). For uploaded -plugins the app cannot verify the manifest, so the dialog flags the plugin as unverified -third-party code with full machine access that Super Productivity cannot sandbox, and -defaults to **Deny** — only allow plugins whose source you trust. If the user denies, -the plugin stays enabled but its node calls fail until it is re-enabled or the app is -restarted (consent is re-requested once per session). +issued by the Electron **main** process after a native consent dialog and is bound to the +plugin id. For uploaded plugins the app cannot verify the manifest, so the dialog flags +the plugin as unverified third-party code with full machine access that Super Productivity +cannot sandbox, and defaults to **Deny** — only allow plugins whose source you trust. If +the user denies, the plugin stays enabled but its node calls fail until it is re-enabled. + +Consent handling differs by plugin type: + +- **Uploaded (community) plugins:** consent is remembered **once per plugin** in a + main-owned, local-only store (`Allow` is not asked again on the next launch). The + consent is **never synced** — granting on one device does not auto-grant on another; + the other device prompts afresh on first node use. Consent is automatically cleared + (forcing a fresh prompt) when you **disable**, **uninstall**, or **re-upload** the + plugin, so replacing a plugin's code under the same id always re-asks. To revoke access + without removing the plugin, simply disable it. +- **Built-in plugins** (e.g. `sync-md`) keep the per-session prompt and are not persisted. > **Plugin id constraints (for `nodeExecution`):** the consent grant keys on your > manifest `id`, so it must be a single safe token — no whitespace, control/bidi diff --git a/electron/plugin-node-consent-store.test.cjs b/electron/plugin-node-consent-store.test.cjs new file mode 100644 index 0000000000..85b7664555 --- /dev/null +++ b/electron/plugin-node-consent-store.test.cjs @@ -0,0 +1,233 @@ +const test = require('node:test'); +const assert = require('node:assert/strict'); +const os = require('node:os'); +const path = require('node:path'); +const { promises: fs } = require('node:fs'); +const Module = require('node:module'); + +require('ts-node/register/transpile-only'); + +const originalModuleLoad = Module._load; +const consentStoreModulePath = path.resolve(__dirname, 'plugin-node-consent-store.ts'); +const simpleStoreModulePath = path.resolve(__dirname, 'simple-store.ts'); + +let userDataDir; +const getStorePath = () => path.join(userDataDir, 'simpleSettings'); +const readStoreFile = async () => JSON.parse(await fs.readFile(getStorePath(), 'utf8')); + +const installMocks = () => { + Module._load = function patchedLoad(request, parent, isMain) { + if (request === 'electron') { + return { app: { getPath: () => userDataDir } }; + } + if (request === 'electron-log/main') { + return { log: () => {}, error: () => {} }; + } + return originalModuleLoad.call(this, request, parent, isMain); + }; +}; + +// Both the consent store and the simple store it wraps hold module-level state (save / +// mutation queues), so reset both caches to get a clean store bound to the new temp dir. +const resetModules = () => { + delete require.cache[consentStoreModulePath]; + delete require.cache[simpleStoreModulePath]; +}; + +const loadConsentStore = () => { + resetModules(); + return require(consentStoreModulePath); +}; + +test.beforeEach(async () => { + userDataDir = await fs.mkdtemp(path.join(os.tmpdir(), 'sp-node-consent-')); + installMocks(); +}); + +test.afterEach(async () => { + Module._load = originalModuleLoad; + resetModules(); + await fs.rm(userDataDir, { recursive: true, force: true }); +}); + +test('set then get round-trips consent and pins the v1 format on disk', async () => { + const store = loadConsentStore(); + await store.setNodeExecutionConsent('community.plugin', { + name: 'Community Plugin', + version: '1.2.3', + grantedAt: 1000, + }); + + assert.deepEqual(await store.getNodeExecutionConsent('community.plugin'), { + name: 'Community Plugin', + version: '1.2.3', + grantedAt: 1000, + }); + + // On-disk shape: a top-level version field (the migration anchor) keyed under the + // dedicated simpleSettings key — never in any pfapi-synced model. + const onDisk = await readStoreFile(); + assert.equal(onDisk.pluginNodeExecutionConsent.version, 1); + assert.ok(onDisk.pluginNodeExecutionConsent.consents['community.plugin']); +}); + +test('get returns null for an unknown plugin id', async () => { + const store = loadConsentStore(); + assert.equal(await store.getNodeExecutionConsent('never-granted'), null); +}); + +test('clear removes a persisted consent', async () => { + const store = loadConsentStore(); + await store.setNodeExecutionConsent('p', { name: 'P', version: '1', grantedAt: 1 }); + await store.clearNodeExecutionConsent('p'); + assert.equal(await store.getNodeExecutionConsent('p'), null); +}); + +test('clear is a no-op (no throw) when nothing is persisted', async () => { + const store = loadConsentStore(); + await assert.doesNotReject(() => store.clearNodeExecutionConsent('absent')); +}); + +test('ignores an on-disk blob with an unknown future version (forward-safe)', async () => { + // A future client may write version 2+; an older client must re-prompt (treat as + // empty) rather than mis-read a format it does not understand into a spurious grant. + await fs.writeFile( + getStorePath(), + JSON.stringify({ + pluginNodeExecutionConsent: { + version: 999, + consents: { x: { name: 'X', version: '1', grantedAt: 1 } }, + }, + }), + 'utf8', + ); + + const store = loadConsentStore(); + assert.equal(await store.getNodeExecutionConsent('x'), null); +}); + +test('never treats a prototype-member id as a stored consent', async () => { + // SECURITY regression: the consents map is keyed on an attacker-controlled pluginId. + // A plain object would resolve e.g. consents['constructor'] to Object.prototype.constructor + // (a truthy function), which the executor would mistake for a prior grant and mint a + // token with NO dialog. A `Map` returns undefined for any unstored key, so reads must + // return null for every prototype-member id. + const store = loadConsentStore(); + const protoIds = ['constructor', 'toString', 'valueOf', 'hasOwnProperty', '__proto__']; + + for (const id of protoIds) { + assert.equal(await store.getNodeExecutionConsent(id), null, `fresh store: ${id}`); + } + + // Persisting a real plugin must not make prototype-member ids start resolving either. + await store.setNodeExecutionConsent('real-plugin', { + name: 'Real', + version: '1', + grantedAt: 1, + }); + for (const id of protoIds) { + assert.equal( + await store.getNodeExecutionConsent(id), + null, + `after a real set: ${id}`, + ); + } + + // Clearing a prototype-member id is a safe no-op that does not corrupt real entries. + await store.clearNodeExecutionConsent('constructor'); + assert.deepEqual(await store.getNodeExecutionConsent('real-plugin'), { + name: 'Real', + version: '1', + grantedAt: 1, + }); +}); + +test('a hand-edited on-disk `__proto__` data key round-trips inertly without polluting Object.prototype', async () => { + // The store serializes the consents Map via Object.fromEntries and rebuilds it via + // Object.entries. A maliciously hand-edited file naming a prototype member as a data key + // must load as an ordinary, ignorable entry — never a prototype write, never a grant. + await fs.writeFile( + getStorePath(), + '{"pluginNodeExecutionConsent":{"version":1,"consents":' + + '{"__proto__":{"name":"X","version":"1","grantedAt":1},' + + '"real-plugin":{"name":"Real","version":"1","grantedAt":2}}}}', + 'utf8', + ); + + const store = loadConsentStore(); + // A real (non-prototype) plugin still loads; loading did not pollute Object.prototype. + assert.deepEqual(await store.getNodeExecutionConsent('real-plugin'), { + name: 'Real', + version: '1', + grantedAt: 2, + }); + assert.equal({}.name, undefined, 'Object.prototype must not be polluted on load'); + + // A subsequent write re-serializes via Object.fromEntries without polluting either. + await store.setNodeExecutionConsent('another', { + name: 'A', + version: '1', + grantedAt: 3, + }); + assert.equal({}.name, undefined, 'Object.prototype must not be polluted on save'); + assert.deepEqual(await store.getNodeExecutionConsent('another'), { + name: 'A', + version: '1', + grantedAt: 3, + }); +}); + +test('keeps other consents when one is cleared', async () => { + const store = loadConsentStore(); + await store.setNodeExecutionConsent('a', { name: 'A', version: '1', grantedAt: 1 }); + await store.setNodeExecutionConsent('b', { name: 'B', version: '1', grantedAt: 2 }); + await store.clearNodeExecutionConsent('a'); + + assert.equal(await store.getNodeExecutionConsent('a'), null); + assert.deepEqual(await store.getNodeExecutionConsent('b'), { + name: 'B', + version: '1', + grantedAt: 2, + }); +}); + +test('ignores a malformed on-disk consent entry (presence alone must not authorize)', async () => { + // SECURITY: mere presence of an entry authorizes execution, so a corrupt/tampered value + // (empty object, array, or a record missing name/version/grantedAt) must be dropped — the + // user re-prompts — rather than read as a grant. Only a fully well-formed record loads. + await fs.writeFile( + getStorePath(), + JSON.stringify({ + pluginNodeExecutionConsent: { + version: 1, + consents: { + 'empty-object': {}, + 'array-entry': [], + 'missing-version': { name: 'P', grantedAt: 1 }, + 'non-number-grantedAt': { name: 'P', version: '1', grantedAt: 'soon' }, + 'good-plugin': { name: 'Good', version: '1', grantedAt: 5 }, + }, + }, + }), + 'utf8', + ); + + const store = loadConsentStore(); + for (const id of [ + 'empty-object', + 'array-entry', + 'missing-version', + 'non-number-grantedAt', + ]) { + assert.equal( + await store.getNodeExecutionConsent(id), + null, + `malformed entry ${id} must be ignored`, + ); + } + assert.deepEqual(await store.getNodeExecutionConsent('good-plugin'), { + name: 'Good', + version: '1', + grantedAt: 5, + }); +}); diff --git a/electron/plugin-node-consent-store.ts b/electron/plugin-node-consent-store.ts new file mode 100644 index 0000000000..442524cdc0 --- /dev/null +++ b/electron/plugin-node-consent-store.ts @@ -0,0 +1,144 @@ +import { loadSimpleStoreAll, saveSimpleStore } from './simple-store'; +import { SimpleStoreKey } from './shared-with-frontend/simple-store.const'; + +/** + * Persisted, per-plugin nodeExecution consent (issue #8512 Phase 2). + * + * SECURITY / TRUST MODEL + * - This store lives in the main-owned `simpleSettings` file under the OS userData + * dir. It is NEVER part of any pfapi-synced model, so a granted consent on one + * device does not auto-grant on another (a node call there triggers a fresh native + * prompt). There is no renderer IPC that can *write* a consent entry — only the + * native Allow dialog in `plugin-node-executor.ts` calls `setNodeExecutionConsent`. + * The renderer can only ask to *clear* consent (fail-safe: clearing forces a + * re-prompt, never an auto-grant). + * - Consent is keyed on `pluginId` only. The "re-ask when the plugin's code changes" + * property is achieved structurally, not by a stored code hash: the only legitimate + * way an uploaded plugin's code changes is a re-upload, and the renderer clears this + * consent on disable, uninstall, and re-upload. A renderer-computed hash would be a + * forgeable TOCTOU tripwire with no security value (a granted plugin already has full + * machine access via `executeScript`), so it is deliberately omitted. The top-level + * `version` field below is the migration anchor if a main-owned hash is ever added. + * - `name`/`version` are the self-declared display strings shown at grant time. They + * are stored for diagnostics/UX only and are NEVER used for authorization — the + * non-spoofable trust anchor is the `pluginId`. + */ + +export const NODE_EXECUTION_CONSENT_STORE_VERSION = 1 as const; + +export interface PersistedNodeExecutionConsent { + /** Self-declared display name at grant time (unverified for uploaded plugins). */ + name: string; + /** Self-declared version at grant time (unverified for uploaded plugins). */ + version: string; + /** ms epoch when the user allowed it. */ + grantedAt: number; +} + +interface NodeExecutionConsentBlob { + version: number; + // SECURITY: keyed on an attacker-controlled pluginId. A `Map` (not a plain object) is + // used so an id that names an `Object.prototype` member (`constructor`, `toString`, + // `valueOf`, `hasOwnProperty`, …) is just an ordinary key that returns `undefined` when + // absent — it can never resolve to an inherited function the executor would mistake for + // a stored grant. Mirrors the sibling `grants` Map in plugin-node-executor.ts. + consents: Map; +} + +const emptyBlob = (): NodeExecutionConsentBlob => ({ + version: NODE_EXECUTION_CONSENT_STORE_VERSION, + consents: new Map(), +}); + +const loadBlob = async (): Promise => { + const all = await loadSimpleStoreAll(); + const raw = all[SimpleStoreKey.PLUGIN_NODE_EXECUTION_CONSENT]; + if (!raw || typeof raw !== 'object') { + return emptyBlob(); + } + const blob = raw as { version?: unknown; consents?: unknown }; + // Forward-safe: a future on-disk format we don't understand is ignored (the user is + // re-prompted) rather than mis-read into a spurious grant. + if ( + blob.version !== NODE_EXECUTION_CONSENT_STORE_VERSION || + !blob.consents || + typeof blob.consents !== 'object' + ) { + return emptyBlob(); + } + // Build the Map from the persisted plain object. Only a fully well-formed entry counts: + // mere presence authorizes execution, so a corrupt/tampered value (a primitive, an array, + // an empty `{}`, or a partial record) is dropped — the user re-prompts — rather than + // mis-read as a grant. A literal `__proto__`/`constructor` key from a hand-edited file is + // just an ordinary Map key here, never a prototype write. + const consents = new Map(); + for (const [pluginId, entry] of Object.entries( + blob.consents as Record, + )) { + if (entry === null || typeof entry !== 'object' || Array.isArray(entry)) { + continue; + } + const { name, version, grantedAt } = entry as Record; + if ( + typeof name === 'string' && + typeof version === 'string' && + typeof grantedAt === 'number' && + Number.isFinite(grantedAt) + ) { + consents.set(pluginId, { name, version, grantedAt }); + } + } + return { version: NODE_EXECUTION_CONSENT_STORE_VERSION, consents }; +}; + +// Serialize read-modify-write mutations so two concurrent grants/clears can't clobber +// each other (load-load-write-write). This is NOT redundant with simple-store's own save +// queue: `loadBlob()` happens *outside* `saveSimpleStore`, so without this lock two +// interleaved mutations could both read the same blob before either writes. Reads +// (getNodeExecutionConsent) are point-in-time and need no lock. +let _mutationQueue: Promise = Promise.resolve(); + +const mutate = (apply: (blob: NodeExecutionConsentBlob) => boolean): Promise => { + const run = async (): Promise => { + const blob = await loadBlob(); + if (apply(blob)) { + // Serialize the Map to a plain object for JSON persistence. `Object.fromEntries` + // uses define-semantics, so even a literal `__proto__` key becomes an own data + // property — it cannot pollute a prototype. Downgrade note: an older client that + // finds a newer on-disk `version` reads it as empty (loadBlob) and this write then + // replaces it with a v1 blob, discarding a future client's consents — worst case a + // re-prompt, never a spurious grant. + await saveSimpleStore(SimpleStoreKey.PLUGIN_NODE_EXECUTION_CONSENT, { + version: blob.version, + consents: Object.fromEntries(blob.consents), + }); + } + }; + _mutationQueue = _mutationQueue.then(run, run); + return _mutationQueue as Promise; +}; + +export const getNodeExecutionConsent = async ( + pluginId: string, +): Promise => { + const blob = await loadBlob(); + return blob.consents.get(pluginId) ?? null; +}; + +export const setNodeExecutionConsent = async ( + pluginId: string, + consent: PersistedNodeExecutionConsent, +): Promise => + mutate((blob) => { + blob.consents.set(pluginId, { + name: consent.name, + version: consent.version, + grantedAt: consent.grantedAt, + }); + return true; + }); + +// `Map.delete` returns true only when an entry existed, which is exactly the +// "write only if changed" signal `mutate` wants — a clear of an absent id is a no-op. +export const clearNodeExecutionConsent = async (pluginId: string): Promise => + mutate((blob) => blob.consents.delete(pluginId)); diff --git a/electron/plugin-node-executor.test.cjs b/electron/plugin-node-executor.test.cjs index 0d31ba351b..320d06df1e 100644 --- a/electron/plugin-node-executor.test.cjs +++ b/electron/plugin-node-executor.test.cjs @@ -27,6 +27,9 @@ let ipcHandlers; let dialogCalls; let nextDialogResult; let nextDialogPromise; +// In-memory stand-in for the main-owned persisted consent store (plugin-node-consent-store.ts), +// so the executor's Phase 2 ask-once logic can be tested without touching the filesystem. +let consentStore; class FakeWebContents extends EventEmitter { constructor(id, url = 'app://index.html') { @@ -96,6 +99,20 @@ const installMocks = () => { }; } + // Stub the persisted-consent store so Phase 2 ask-once logic is exercised in-memory. + if ( + request === './plugin-node-consent-store' && + parent && + typeof parent.filename === 'string' && + parent.filename.endsWith('plugin-node-executor.ts') + ) { + return consentStore; + } + + if (request === 'electron-log/main') { + return { log: () => {}, error: () => {} }; + } + if (request === 'electron') { return { app: { @@ -141,6 +158,17 @@ test.beforeEach(() => { dialogCalls = []; nextDialogResult = { response: 0 }; nextDialogPromise = undefined; + const records = new Map(); + consentStore = { + __records: records, + getNodeExecutionConsent: async (pluginId) => records.get(pluginId) ?? null, + setNodeExecutionConsent: async (pluginId, consent) => { + records.set(pluginId, consent); + }, + clearNodeExecutionConsent: async (pluginId) => { + records.delete(pluginId); + }, + }; installMocks(); }); @@ -274,6 +302,10 @@ test('issues a grant to an uploaded (non-bundled) plugin and labels it unverifie // and defaults to Deny. assert.match(opts.detail, /Plugin ID: uploaded-node-plugin/); assert.match(opts.detail, /self-declared, unverified/); + // The Allow is persisted (ask-once), so the prompt must disclose that the choice is + // remembered rather than claiming it is only valid "for this app session". + assert.match(opts.detail, /remembered on this device/); + assert.equal(opts.detail.includes('for this app session'), false); assert.equal(opts.defaultId, 1); }); @@ -478,3 +510,182 @@ test('never upgrades trust: an on-disk match that does not cleanly verify uses t BUILT_IN_PLUGIN_MANIFEST.permissions = originalPermissions; } }); + +// --- Phase 2: persisted, per-plugin consent (#8512) --- + +test('uploaded plugin with persisted consent is granted without a dialog (ask-once)', async () => { + loadModule(); + // Simulate a grant remembered from a previous app session. + consentStore.__records.set('uploaded-node-plugin', { + name: 'Uploaded', + version: '1.0.0', + grantedAt: 1, + }); + const webContents = new FakeWebContents(20); + + const grant = await callIpc( + 'PLUGIN_REQUEST_NODE_EXECUTION_GRANT', + webContents, + 'uploaded-node-plugin', + { name: 'Uploaded', version: '1.0.0' }, + ); + + assert.equal(typeof grant.token, 'string'); + assert.equal(dialogCalls.length, 0); +}); + +test('granting an uploaded plugin persists consent for the next session', async () => { + loadModule(); + const webContents = new FakeWebContents(21); + + await callIpc('PLUGIN_REQUEST_NODE_EXECUTION_GRANT', webContents, 'persist-me', { + name: 'Persist Me', + version: '2.0.0', + }); + + assert.equal(dialogCalls.length, 1); + const persisted = consentStore.__records.get('persist-me'); + assert.equal(persisted.name, 'Persist Me'); + assert.equal(persisted.version, '2.0.0'); + assert.equal(typeof persisted.grantedAt, 'number'); +}); + +test('denying an uploaded plugin does not persist consent', async () => { + loadModule(); + nextDialogResult = { response: 1 }; + const webContents = new FakeWebContents(22); + + const denied = await callIpc( + 'PLUGIN_REQUEST_NODE_EXECUTION_GRANT', + webContents, + 'deny-me', + { name: 'Deny', version: '1.0.0' }, + ); + + assert.equal(denied, null); + assert.equal(consentStore.__records.has('deny-me'), false); +}); + +test('clearConsent drops the grant + persisted consent so the next request re-prompts', async () => { + loadModule(); + const wc1 = new FakeWebContents(23); + const grant = await callIpc('PLUGIN_REQUEST_NODE_EXECUTION_GRANT', wc1, 'clear-me', { + name: 'Clear', + version: '1.0.0', + }); + assert.equal(dialogCalls.length, 1); + assert.ok(consentStore.__records.has('clear-me')); + + await callIpc('PLUGIN_CLEAR_NODE_EXECUTION_CONSENT', wc1, 'clear-me'); + assert.equal(consentStore.__records.has('clear-me'), false); + + // The live session grant is gone too: exec is now unauthorized. + await assert.rejects( + () => + callIpc('PLUGIN_EXEC_NODE_SCRIPT', wc1, 'clear-me', grant.token, { + script: 'return true;', + }), + /not authorized/, + ); + + // A fresh request re-prompts because no persisted consent remains. + const wc2 = new FakeWebContents(24); + await callIpc('PLUGIN_REQUEST_NODE_EXECUTION_GRANT', wc2, 'clear-me', { + name: 'Clear', + version: '1.0.0', + }); + assert.equal(dialogCalls.length, 2); +}); + +test('rejects prototype-pollution ids before any consent lookup or mint', async () => { + // SECURITY regression: 'constructor'/'prototype' pass the id allowlist regex and would, + // against a plain-object consent map, resolve to an Object.prototype member and mint a + // grant with no dialog. They must be rejected outright. ('__proto__' fails the regex.) + loadModule(); + const webContents = new FakeWebContents(27); + + for (const badId of ['constructor', 'prototype', '__proto__']) { + await assert.rejects( + () => + callIpc('PLUGIN_REQUEST_NODE_EXECUTION_GRANT', webContents, badId, { + name: 'x', + version: '1', + }), + /Invalid pluginId/, + `expected ${badId} to be rejected`, + ); + } + assert.equal(dialogCalls.length, 0); + assert.equal(consentStore.__records.has('constructor'), false); +}); + +test('built-in plugin consent is never persisted (stays per-session, regression)', async () => { + loadModule(); + const wc1 = new FakeWebContents(25); + await callIpc('PLUGIN_REQUEST_NODE_EXECUTION_GRANT', wc1, 'sync-md'); + assert.equal(dialogCalls.length, 1); + // Built-in plugins keep the verified per-session prompt — nothing is written to the + // persisted store, so a new session prompts again. + assert.equal(consentStore.__records.has('sync-md'), false); + + const wc2 = new FakeWebContents(26); + await callIpc('PLUGIN_REQUEST_NODE_EXECUTION_GRANT', wc2, 'sync-md'); + assert.equal(dialogCalls.length, 2); +}); + +test('mints the grant before persisting consent (persist is best-effort, never gates the grant)', async () => { + // SECURITY ordering: the consent persist is a new `await` after the post-dialog + // sender-validity check. Minting BEFORE that write keeps the grant in the live table + // during the write, so a navigation/destroy cleanup that fires mid-write drops it (in + // real Electron), and a persist failure can never lose an approved grant. Asserting the + // grant already exists at the moment of the persist write pins that ordering. + let grantsSizeAtPersist = -1; + let mod; + consentStore.setNodeExecutionConsent = async (pluginId, consent) => { + grantsSizeAtPersist = mod.pluginNodeExecutor.grants.size; + consentStore.__records.set(pluginId, consent); + }; + mod = loadModule(); + const webContents = new FakeWebContents(30); + + const grant = await callIpc( + 'PLUGIN_REQUEST_NODE_EXECUTION_GRANT', + webContents, + 'uploaded-order', + { name: 'Uploaded', version: '1.0.0' }, + ); + + assert.equal(typeof grant.token, 'string'); + // The grant for this request was already live when the persist ran (mint-before-persist). + assert.equal(grantsSizeAtPersist, 1); +}); + +test('a consent persist failure still grants the approved session (best-effort)', async () => { + // Persisting is best-effort: a write failure only costs a re-prompt next session, never + // the grant the user just approved. Minting before the persist guarantees this. + let mod; + consentStore.setNodeExecutionConsent = async () => { + throw Object.assign(new Error('disk full'), { code: 'ENOSPC' }); + }; + mod = loadModule(); + assert.ok(mod); + const webContents = new FakeWebContents(31); + + const grant = await callIpc( + 'PLUGIN_REQUEST_NODE_EXECUTION_GRANT', + webContents, + 'persist-fails', + { name: 'Uploaded', version: '1.0.0' }, + ); + + assert.equal(typeof grant.token, 'string'); + const result = await callIpc( + 'PLUGIN_EXEC_NODE_SCRIPT', + webContents, + 'persist-fails', + grant.token, + { script: 'return 1 + 1;' }, + ); + assert.equal(result.success, true); + assert.equal(result.result, 2); +}); diff --git a/electron/plugin-node-executor.ts b/electron/plugin-node-executor.ts index a623b6d98e..c1d691d561 100644 --- a/electron/plugin-node-executor.ts +++ b/electron/plugin-node-executor.ts @@ -1,5 +1,6 @@ import { app, BrowserWindow, dialog, ipcMain } from 'electron'; import type { WebContents } from 'electron'; +import { error as logError } from 'electron-log/main'; import { spawn } from 'child_process'; import { randomBytes } from 'crypto'; import { existsSync, readFileSync } from 'fs'; @@ -11,6 +12,11 @@ import { PluginNodeScriptResult, PluginManifest, } from '../packages/plugin-api/src/types'; +import { + clearNodeExecutionConsent, + getNodeExecutionConsent, + setNodeExecutionConsent, +} from './plugin-node-consent-store'; const DEFAULT_TIMEOUT = 30000; // 30 seconds const MAX_TIMEOUT = 300000; // 5 minutes @@ -27,6 +33,12 @@ const BUILT_IN_PLUGIN_ID_RE = /^[a-z0-9][a-z0-9-]*$/; // with no Unicode range to keep updated as new code points are assigned. const MAX_UPLOADED_PLUGIN_ID_LENGTH = 100; const SAFE_UPLOADED_PLUGIN_ID_RE = /^[A-Za-z0-9][A-Za-z0-9._-]*$/; +// The id is used as a key into the persisted-consent store, which keys consent in a `Map` +// (returns `undefined` for any unstored key) so an `Object.prototype` member name can't +// masquerade as a stored grant. Reject the classic pollution keys at this boundary too as +// defense in depth (`__proto__` is already excluded by the leading-char allowlist; the +// others pass it). +const FORBIDDEN_PLUGIN_IDS = new Set(['__proto__', 'prototype', 'constructor']); // Self-declared name/version are display-only. Strip every Unicode control (Cc) and // format (Cf) character — this covers C0/C1 controls, all zero-width characters, the BOM, // and every bidi control (incl. U+061C ALM, the word-joiner range, and the isolate marks) @@ -48,6 +60,9 @@ const assertSafePluginId = (pluginId: unknown): string => { if (!SAFE_UPLOADED_PLUGIN_ID_RE.test(pluginId)) { throw new Error('Invalid pluginId'); } + if (FORBIDDEN_PLUGIN_IDS.has(pluginId)) { + throw new Error('Invalid pluginId'); + } return pluginId; }; @@ -120,6 +135,9 @@ class PluginNodeExecutor { const safeId = assertSafePluginId(pluginId); const webContentsId = event.sender.id; + // Captured before any await so both the ask-once and dialog paths can detect a + // navigation that happened while we were resolving consent. + const requestUrl = event.sender.getURL(); const existingGrant = this.grants.get(safeId); if (existingGrant) { if (existingGrant.webContentsId === webContentsId) { @@ -135,11 +153,31 @@ class PluginNodeExecutor { // (id mismatch, missing nodeExecution permission, unreadable manifest) returns // null and falls back to the unverified dialog, so uploaded code can never borrow // a built-in plugin's trusted name even if its id collides with a bundled dir. - const dialogOptions = - this.describeVerifiedBuiltInDialog(safeId) ?? - this.describeUnverifiedUploadedDialog(safeId, displayInfo); + const builtInManifest = this.tryGetVerifiedBuiltInManifest(safeId); + + // Phase 2 (issue #8512): persisted, ask-once consent is scoped to UPLOADED + // (community) plugins only. Built-in plugins keep the per-session verified prompt + // — no behaviour change, so `sync-md` etc. are regression-safe. Only a prior + // native Allow can have written this entry (no renderer write path), and the + // renderer clears it on disable/uninstall/re-upload, so a re-uploaded (changed) + // plugin reusing the id never silently inherits it. + if (!builtInManifest) { + const persistedConsent = await getNodeExecutionConsent(safeId); + if (persistedConsent) { + // Don't mint for a sender that was destroyed or navigated away while the + // consent was being read (parity with the post-dialog checks below). + if (event.sender.isDestroyed() || event.sender.getURL() !== requestUrl) { + return null; + } + this.registerGrantCleanup(event.sender); + return this.mintGrant(safeId, webContentsId); + } + } + + const dialogOptions = builtInManifest + ? this.buildVerifiedBuiltInDialog(safeId, builtInManifest) + : this.describeUnverifiedUploadedDialog(safeId, displayInfo); - const requestUrl = event.sender.getURL(); this.registerGrantCleanup(event.sender); let result: Electron.MessageBoxReturnValue; @@ -166,12 +204,57 @@ class PluginNodeExecutor { return null; } - const token = randomBytes(32).toString('base64url'); - this.grants.set(safeId, { - token, - webContentsId, - }); - return { token }; + // Allow → mint the grant FIRST, synchronously, right after the sender-validity + // check above. The persist below is a new `await`; minting before it keeps the + // "never mint for a navigated/destroyed sender" invariant tight — if the sender + // navigates or is destroyed during the write, the cleanup listener registered + // before the dialog drops this just-minted grant, so no untracked grant survives + // for a stale webContents. + const grant = this.mintGrant(safeId, webContentsId); + + // For uploaded plugins, remember the decision so later sessions don't re-prompt + // (ask-once). Persisting is best-effort: a write failure only costs a re-prompt + // next session, never a grant the user just approved. + if (!builtInManifest) { + try { + // Persist exactly the name/version the user saw in the dialog. + await setNodeExecutionConsent(safeId, { + ...this.sanitizedUploadedDisplay(displayInfo), + grantedAt: Date.now(), + }); + } catch (error) { + // Host diagnostic → exportable log (electron-log), distinct from the sandboxed + // plugin's own console output. Log only the validated id and the error code — + // never the raw error, whose message can embed the userData absolute path (and + // thus the OS username) for an fs failure. + const code = (error as NodeJS.ErrnoException)?.code ?? 'unknown'; + logError(`Failed to persist nodeExecution consent for ${safeId} (${code})`); + } + } + return grant; + }, + ); + + ipcMain.handle( + IPC.PLUGIN_CLEAR_NODE_EXECUTION_CONSENT, + // Explicit revoke from the trusted renderer that owns the plugin lifecycle + // (disable / uninstall / re-upload). Drops the live session grant for this id AND + // the persisted consent, so the next node call re-prompts. A delete is fail-safe: + // the worst a hostile renderer can do by calling this is force a prompt, never a + // silent grant. + async (_event, pluginId: string) => { + let safeId: string; + try { + safeId = assertSafePluginId(pluginId); + } catch { + return; + } + const grant = this.grants.get(safeId); + if (grant) { + this.grants.delete(safeId); + this.releaseGrantCleanupIfUnused(grant.webContentsId); + } + await clearNodeExecutionConsent(safeId); }, ); @@ -297,23 +380,36 @@ class PluginNodeExecutor { this.unregisterGrantCleanup(webContentsId); } + private mintGrant(pluginId: string, webContentsId: number): { token: string } { + const token = randomBytes(32).toString('base64url'); + this.grants.set(pluginId, { token, webContentsId }); + return { token }; + } + /** - * Consent dialog for a verified built-in plugin (name/version read from disk). - * Returns null when the id does not resolve to a cleanly-verified built-in - * nodeExecution manifest (no on-disk match, id mismatch, missing permission, or - * unreadable/invalid manifest), so the caller falls back to the unverified-uploaded - * dialog — a partial or colliding match must never *upgrade* trust to the built-in - * dialog. + * Resolve the cleanly-verified built-in nodeExecution manifest for an id, or null + * when the id does not resolve to one (no on-disk match, id mismatch, missing + * permission, or unreadable/invalid manifest). A partial or colliding match must + * never *upgrade* trust to the built-in dialog, so callers fall back to the + * unverified-uploaded path on null. */ - private describeVerifiedBuiltInDialog( - pluginId: string, - ): Electron.MessageBoxOptions | null { - let manifest: PluginManifest; + private tryGetVerifiedBuiltInManifest(pluginId: string): PluginManifest | null { try { - manifest = this.getVerifiedBuiltInNodeExecutionManifest(pluginId); + return this.getVerifiedBuiltInNodeExecutionManifest(pluginId); } catch { return null; } + } + + /** + * Consent dialog for a verified built-in plugin (name/version read from disk). + * Built-in plugins are NOT persisted (Phase 2 ask-once is uploaded-only), so the copy + * keeps the per-session wording. + */ + private buildVerifiedBuiltInDialog( + pluginId: string, + manifest: PluginManifest, + ): Electron.MessageBoxOptions { return { ...NODE_CONSENT_DIALOG_BASE, title: 'Allow plugin Node.js execution?', @@ -327,6 +423,21 @@ class PluginNodeExecutor { }; } + /** + * The self-declared name/version exactly as shown in the uploaded-plugin consent dialog. + * Single source of truth for the sanitize lengths and fallbacks, so the persisted record + * (setNodeExecutionConsent) always matches what the user actually saw and approved. + */ + private sanitizedUploadedDisplay(displayInfo?: NodeExecutionGrantDisplayInfo): { + name: string; + version: string; + } { + return { + name: sanitizeDialogString(displayInfo?.name, 80) || '(unnamed)', + version: sanitizeDialogString(displayInfo?.version, 32) || '(unknown)', + }; + } + /** * Consent dialog for an uploaded (community) plugin. The app cannot verify an * uploaded plugin's identity, so the dialog anchors on the validated id and marks @@ -336,8 +447,7 @@ class PluginNodeExecutor { pluginId: string, displayInfo?: NodeExecutionGrantDisplayInfo, ): Electron.MessageBoxOptions { - const name = sanitizeDialogString(displayInfo?.name, 80) || '(unnamed)'; - const version = sanitizeDialogString(displayInfo?.version, 32) || '(unknown)'; + const { name, version } = this.sanitizedUploadedDisplay(displayInfo); return { ...NODE_CONSENT_DIALOG_BASE, title: 'Allow this plugin to run code on your machine?', @@ -348,7 +458,8 @@ class PluginNodeExecutor { `Version (self-declared): ${version}`, '', 'This is a third-party plugin. Super Productivity cannot verify its identity and cannot sandbox it.', - 'If you allow it, the plugin can run any program with full access to your files and system for this app session.', + 'If you allow it, the plugin can run any program with full access to your files and system.', + 'Your choice is remembered on this device until you disable, remove, or re-upload this plugin.', '', 'Only allow this if you trust the source of this plugin.', ].join('\n'), diff --git a/electron/preload.ts b/electron/preload.ts index 0c9cfa6c30..8ef4e9dd56 100644 --- a/electron/preload.ts +++ b/electron/preload.ts @@ -269,6 +269,8 @@ const ea: ElectronAPI = { pluginId, grantToken, ) as Promise, + clearConsent: (pluginId: string) => + _invoke('PLUGIN_CLEAR_NODE_EXECUTION_CONSENT', pluginId) as Promise, }; }, diff --git a/electron/shared-with-frontend/ipc-events.const.ts b/electron/shared-with-frontend/ipc-events.const.ts index bd0f555bb1..187631ac80 100644 --- a/electron/shared-with-frontend/ipc-events.const.ts +++ b/electron/shared-with-frontend/ipc-events.const.ts @@ -86,6 +86,7 @@ export enum IPC { PLUGIN_REQUEST_NODE_EXECUTION_GRANT = 'PLUGIN_REQUEST_NODE_EXECUTION_GRANT', PLUGIN_EXEC_NODE_SCRIPT = 'PLUGIN_EXEC_NODE_SCRIPT', PLUGIN_REVOKE_NODE_EXECUTION_GRANT = 'PLUGIN_REVOKE_NODE_EXECUTION_GRANT', + PLUGIN_CLEAR_NODE_EXECUTION_CONSENT = 'PLUGIN_CLEAR_NODE_EXECUTION_CONSENT', // Plugin OAuth PLUGIN_OAUTH_PREPARE = 'PLUGIN_OAUTH_PREPARE', diff --git a/electron/shared-with-frontend/plugin-node-execution.model.ts b/electron/shared-with-frontend/plugin-node-execution.model.ts index c3a9d91558..7a46c371bb 100644 --- a/electron/shared-with-frontend/plugin-node-execution.model.ts +++ b/electron/shared-with-frontend/plugin-node-execution.model.ts @@ -21,4 +21,11 @@ export interface PluginNodeExecutionElectronApi { request: PluginNodeScriptRequest, ): Promise; revokeGrant(pluginId: string, grantToken: string): Promise; + /** + * Drop the live session grant AND the main-owned persisted consent for this plugin so + * the next node call re-prompts. Called by the renderer on disable / uninstall / + * re-upload. There is deliberately no `setConsent` counterpart — only a native Allow + * dialog in main can write consent, so the renderer can never self-grant. + */ + clearConsent(pluginId: string): Promise; } diff --git a/electron/shared-with-frontend/simple-store.const.ts b/electron/shared-with-frontend/simple-store.const.ts index 7a3a9bb90e..08c0032a37 100644 --- a/electron/shared-with-frontend/simple-store.const.ts +++ b/electron/shared-with-frontend/simple-store.const.ts @@ -3,6 +3,10 @@ export enum SimpleStoreKey { ALLOWED_COMMANDS = 'allowedCommands', // Main-owned sync folder path (issue #8228); the renderer no longer holds it. SYNC_FOLDER_PATH = 'syncFolderPath', + // Main-owned, never-synced persisted nodeExecution consent (issue #8512 Phase 2). + // The renderer has no IPC write path that can grant consent into this key — only + // a native Allow dialog in main writes it — so XSS/un-granted code cannot self-grant. + PLUGIN_NODE_EXECUTION_CONSENT = 'pluginNodeExecutionConsent', // Legacy key kept for backwards compatibility when reading persisted settings LEGACY_IS_USE_OBSIDIAN_STYLE_HEADER = 'isUseObsidianStyleHeader', } diff --git a/src/app/plugins/plugin-bridge.service.spec.ts b/src/app/plugins/plugin-bridge.service.spec.ts index c04c824efa..03234aaa09 100644 --- a/src/app/plugins/plugin-bridge.service.spec.ts +++ b/src/app/plugins/plugin-bridge.service.spec.ts @@ -515,17 +515,20 @@ describe('PluginBridgeService - nodeExecution grant tokens', () => { let service: PluginBridgeService; let originalElectronApi: typeof window.ea | undefined; let pluginExecNodeScriptSpy: jasmine.Spy; + let clearConsentSpy: jasmine.Spy; let consumePluginNodeExecutionApiSpy: jasmine.Spy; beforeEach(() => { originalElectronApi = window.ea; pluginExecNodeScriptSpy = jasmine.createSpy('pluginExecNodeScript'); + clearConsentSpy = jasmine.createSpy('clearConsent').and.resolveTo(undefined); consumePluginNodeExecutionApiSpy = jasmine .createSpy('consumePluginNodeExecutionApi') .and.returnValue({ requestGrant: jasmine.createSpy('requestGrant'), executeScript: pluginExecNodeScriptSpy, revokeGrant: jasmine.createSpy('revokeGrant'), + clearConsent: clearConsentSpy, }); window.ea = { ...(window.ea ?? {}), @@ -579,6 +582,16 @@ describe('PluginBridgeService - nodeExecution grant tokens', () => { expect(service.hasNodeExecutionGrantToken('node-plugin')).toBeFalse(); }); + it('clearNodeExecutionConsent drops the local token and asks main to clear consent', async () => { + service.setNodeExecutionGrantToken('node-plugin', 'token-1'); + expect(service.hasNodeExecutionGrantToken('node-plugin')).toBeTrue(); + + await service.clearNodeExecutionConsent('node-plugin'); + + expect(service.hasNodeExecutionGrantToken('node-plugin')).toBeFalse(); + expect(clearConsentSpy).toHaveBeenCalledOnceWith('node-plugin'); + }); + it('does not call Electron node execution in a web runtime', async () => { service.setNodeExecutionGrantToken('node-plugin', 'token-1'); const bound = service.createBoundMethods('node-plugin', { diff --git a/src/app/plugins/plugin-bridge.service.ts b/src/app/plugins/plugin-bridge.service.ts index fe69d1e421..2df06a2eb7 100644 --- a/src/app/plugins/plugin-bridge.service.ts +++ b/src/app/plugins/plugin-bridge.service.ts @@ -1724,6 +1724,16 @@ export class PluginBridgeService implements OnDestroy { await this.#nodeExecutionApi?.revokeGrant(pluginId, grantToken); } + /** + * Drop both the in-renderer session token and the main-owned persisted consent for a + * plugin (issue #8512 Phase 2). Used on disable / uninstall / re-upload so the next + * node call re-prompts. No-op on web (no node execution API). + */ + async clearNodeExecutionConsent(pluginId: string): Promise { + this.#nodeExecutionGrantTokens.delete(pluginId); + await this.#nodeExecutionApi?.clearConsent(pluginId); + } + /** * Internal method to execute Node.js script */ diff --git a/src/app/plugins/plugin-meta-persistence.service.spec.ts b/src/app/plugins/plugin-meta-persistence.service.spec.ts index d72fe2c92d..9d1a20caa0 100644 --- a/src/app/plugins/plugin-meta-persistence.service.spec.ts +++ b/src/app/plugins/plugin-meta-persistence.service.spec.ts @@ -35,6 +35,11 @@ describe('PluginMetaPersistenceService', () => { spyOn(store, 'dispatch'); }); + // SYNC-EXCLUSION GUARD (issue #8512 Phase 2). nodeExecution consent is persisted in a + // main-owned, local-only store (electron/plugin-node-consent-store.ts) and must NEVER + // leak into the pfapi-synced `pluginMetadata` — otherwise a grant on one device would + // auto-grant on another. The strict `toHaveBeenCalledOnceWith({id,isEnabled})` below + // deep-matches the dispatched payload, so any consent field reappearing here fails. it('does not re-persist stale nodeExecution consent metadata', async () => { await service.setPluginEnabled('plugin-a', true); diff --git a/src/app/plugins/plugin.service.load-from-zip.spec.ts b/src/app/plugins/plugin.service.load-from-zip.spec.ts index 15ee80fb0e..bb955f37a7 100644 --- a/src/app/plugins/plugin.service.load-from-zip.spec.ts +++ b/src/app/plugins/plugin.service.load-from-zip.spec.ts @@ -137,6 +137,7 @@ describe('PluginService loadPluginFromZip iframe-only plugins', () => { 'setNodeExecutionGrantToken', 'revokeNodeExecutionGrantToken', 'revokeNodeExecutionGrant', + 'clearNodeExecutionConsent', ]), }, { provide: TranslateService, useValue: translateService }, diff --git a/src/app/plugins/plugin.service.spec.ts b/src/app/plugins/plugin.service.spec.ts index 7f1aa43d4f..38fd01c2b4 100644 --- a/src/app/plugins/plugin.service.spec.ts +++ b/src/app/plugins/plugin.service.spec.ts @@ -72,9 +72,11 @@ describe('PluginService', () => { 'setNodeExecutionGrantToken', 'revokeNodeExecutionGrantToken', 'revokeNodeExecutionGrant', + 'clearNodeExecutionConsent', ]); pluginBridge.hasNodeExecutionGrantToken.and.returnValue(false); pluginBridge.requestNodeExecutionGrant.and.resolveTo(null); + pluginBridge.clearNodeExecutionConsent.and.resolveTo(undefined); pluginRunner = jasmine.createSpyObj('PluginRunner', [ 'loadPlugin', 'unloadPlugin', @@ -319,6 +321,94 @@ describe('PluginService', () => { ); }); + it('clearNodeExecutionConsent drops the session denial and asks the bridge to clear consent', async () => { + const pluginId = 'node-plugin'; + const deniedSet = ( + service as unknown as { _nodeExecutionDeniedThisSession: Set } + )._nodeExecutionDeniedThisSession; + deniedSet.add(pluginId); + + const result = await service.clearNodeExecutionConsent(pluginId); + + expect(result).toBe(true); + expect(deniedSet.has(pluginId)).toBe(false); + expect(pluginBridge.clearNodeExecutionConsent).toHaveBeenCalledOnceWith(pluginId); + }); + + it('clearNodeExecutionConsent returns false when the persisted clear fails (caller can fail closed)', async () => { + // A persistence failure must NOT throw (lifecycle bookkeeping ignores it), but must be + // reported via the return value so loadPluginFromZip can abort before loading new code + // under an id whose stale consent could not be revoked. + pluginBridge.clearNodeExecutionConsent.and.rejectWith(new Error('disk full')); + + const result = await service.clearNodeExecutionConsent('node-plugin'); + + expect(result).toBe(false); + }); + + it('removeUploadedPlugin clears persisted nodeExecution consent (Phase 2)', async () => { + const pluginId = 'uploaded-node-plugin'; + + await service.removeUploadedPlugin(pluginId); + + expect(pluginBridge.clearNodeExecutionConsent).toHaveBeenCalledWith(pluginId); + }); + + it('clearUploadedPluginsFromMemory clears persisted nodeExecution consent for uploaded plugins only (Phase 2)', async () => { + const setState = ( + service as unknown as { + _setPluginState: (pluginId: string, state: PluginState) => void; + } + )._setPluginState.bind(service); + setState('uploaded-1', { + manifest: { ...mockManifest, id: 'uploaded-1', permissions: ['nodeExecution'] }, + status: 'not-loaded', + path: 'uploaded://uploaded-1', + type: 'uploaded', + isEnabled: true, + }); + setState('builtin-1', { + manifest: { ...mockManifest, id: 'builtin-1', permissions: ['nodeExecution'] }, + status: 'not-loaded', + path: 'assets/bundled-plugins/builtin-1', + type: 'built-in', + isEnabled: true, + }); + + await service.clearUploadedPluginsFromMemory(); + + // Without this, a same-id re-upload after a cache clear (no `existingState`) would be + // silently re-granted node execution with no prompt — the bug this guards against. + expect(pluginBridge.clearNodeExecutionConsent).toHaveBeenCalledWith('uploaded-1'); + // Built-in plugins never persist consent, so they are not cleared here. + expect(pluginBridge.clearNodeExecutionConsent).not.toHaveBeenCalledWith('builtin-1'); + }); + + it('disablePlugin persists isEnabled=false and revokes nodeExecution consent (Phase 2)', async () => { + const pluginId = 'uploaded-node-plugin'; + ( + service as unknown as { + _setPluginState: (pluginId: string, state: PluginState) => void; + } + )._setPluginState(pluginId, { + manifest: { ...mockManifest, id: pluginId, permissions: ['nodeExecution'] }, + status: 'not-loaded', + path: 'uploaded://uploaded-node-plugin', + type: 'uploaded', + isEnabled: true, + }); + + await service.disablePlugin(pluginId); + + expect(pluginMetaPersistenceService.setPluginEnabled).toHaveBeenCalledWith( + pluginId, + false, + ); + // Disable must revoke consent so re-enabling re-prompts — the invariant that previously + // lived only in the UI handler where a future disable path could forget it. + expect(pluginBridge.clearNodeExecutionConsent).toHaveBeenCalledWith(pluginId); + }); + it('treats runner loaded:false activation results as failures', async () => { const manifest: PluginManifest = { ...mockManifest, diff --git a/src/app/plugins/plugin.service.ts b/src/app/plugins/plugin.service.ts index 691393a513..6825e8f46d 100644 --- a/src/app/plugins/plugin.service.ts +++ b/src/app/plugins/plugin.service.ts @@ -1398,6 +1398,23 @@ export class PluginService implements OnDestroy { PluginLog.info(`Plugin ${manifest.id} info:`, codeAnalysis.info); } + // An explicit upload always (re)installs code under this id, so any prior persisted + // nodeExecution consent no longer applies — clear it unconditionally, BEFORE loading + // the new code and outside the `existingState` branch (issue #8512 Phase 2). This + // closes the orphaned-consent gap: if consent survived in the main-owned store while + // the in-memory/cache record was already gone (a crash mid-uninstall, IndexedDB + // eviction, or an external/partial wipe), a same-id re-upload would otherwise skip the + // clear and be silently granted node execution with no prompt. This MUST fail closed: + // if the revoke can't be persisted we abort the upload here, before any teardown or + // code store, rather than load replacement code that could inherit the old grant. (For + // a brand-new id with nothing to clear the call is a no-op and returns true.) Re-asking + // once per upload is intended; ask-once is about sessions, not uploads. + if (!(await this.clearNodeExecutionConsent(manifest.id))) { + throw new Error( + `Aborting upload of "${manifest.id}": could not clear previous nodeExecution consent`, + ); + } + // Teardown existing plugin runtime if re-uploading same ID const existingState = this._getPluginState(manifest.id); if (existingState) { @@ -1641,9 +1658,10 @@ export class PluginService implements OnDestroy { this._pluginIcons.delete(pluginId); this._pluginIconsSignal.set(new Map(this._pluginIcons)); - // Drop any session nodeExecution denial so a fresh re-upload of this id is prompted - // again rather than silently failing closed against the removed plugin's decision. - this._nodeExecutionDeniedThisSession.delete(pluginId); + // Clear the session denial AND the main-owned persisted consent, so a fresh upload + // of this id later starts from a clean prompt and a *different* plugin reusing the id + // can never inherit the removed plugin's consent (issue #8512 Phase 2). + await this.clearNodeExecutionConsent(pluginId); // Remove from plugin states this._deletePluginState(pluginId); @@ -1652,10 +1670,18 @@ export class PluginService implements OnDestroy { } /** - * Clear all uploaded plugins from memory. Called when IndexedDB cache is cleared + * Clear all uploaded plugins from memory. Called when the IndexedDB cache is cleared * so that in-memory state matches the empty cache. + * + * Also clears each uploaded plugin's main-owned PERSISTED nodeExecution consent + * (issue #8512 Phase 2). The cache wipe removes the plugin code, but the consent lives + * in the main process, so without this a later re-upload of the same id — potentially + * *different* code — would be silently granted node execution with no prompt: the + * post-clear upload has no `existingState`, so the re-upload clear in `loadPluginFromZip` + * never fires. Mirrors `removeUploadedPlugin`, keeping "replacing code under an id always + * re-asks" true on every removal path. */ - clearUploadedPluginsFromMemory(): void { + async clearUploadedPluginsFromMemory(): Promise { const states = this._pluginStates(); const uploadedIds: string[] = []; for (const [pluginId, state] of states.entries()) { @@ -1678,6 +1704,12 @@ export class PluginService implements OnDestroy { return updated; }); this._pluginIconsSignal.set(new Map(this._pluginIcons)); + // Drop persisted consent after teardown has released the live grants. Each clear is + // fail-safe (worst case: a re-prompt) and idempotent, so a single failure can't leave + // a different id's consent behind. + await Promise.all( + uploadedIds.map((pluginId) => this.clearNodeExecutionConsent(pluginId)), + ); } /** @@ -1917,6 +1949,52 @@ export class PluginService implements OnDestroy { await this._pluginBridge.revokeNodeExecutionGrant(pluginId, grantToken ?? ''); } + /** + * Revoke a plugin's nodeExecution consent: clears the in-session grant token, the + * session "denied" marker, and the main-owned PERSISTED consent (issue #8512 Phase 2), + * so the next node call re-prompts. Called on disable, uninstall, and re-upload — the + * three explicit, user-driven lifecycle edges. Deliberately NOT called from generic + * teardown (`_teardownPluginRuntime`), which also fires on app shutdown/navigation and + * must preserve "ask once across sessions". + * + * A persistence failure is logged (id only — the raw error can embed the userData path) + * and reported via the RETURN VALUE rather than thrown: lifecycle edges (disable / + * uninstall / cache-clear) treat the clear as best-effort and ignore the result, so a rare + * disk failure can't abort their bookkeeping (zombie plugin state, or `disablePlugin` + * rejecting after the plugin is already disabled). A SECURITY-critical caller that must + * fail closed — `loadPluginFromZip`, before it loads replacement code under this id — + * instead checks the result and aborts, so replacement code can never inherit a prior + * grant just because the revoke write failed. + * + * @returns `true` if the persisted consent was cleared (or there was nothing to clear), + * `false` if the clear could not be persisted. + */ + async clearNodeExecutionConsent(pluginId: string): Promise { + this._nodeExecutionDeniedThisSession.delete(pluginId); + try { + await this._pluginBridge.clearNodeExecutionConsent(pluginId); + return true; + } catch { + PluginLog.err(`Failed to clear persisted nodeExecution consent for ${pluginId}`); + return false; + } + } + + /** + * Disable an installed plugin: persist `isEnabled=false`, tear down its runtime, and + * revoke its nodeExecution consent (session grant + persisted), so re-enabling re-prompts + * (issue #8512 Phase 2). Routing every disable through here keeps "disable revokes + * consent" a structural invariant — a future disable path cannot silently skip the revoke + * — which is why the revoke lives here rather than in `unloadPlugin` / + * `_teardownPluginRuntime` (those also run on app shutdown/navigation, where consent must + * survive). `clearNodeExecutionConsent` is a safe no-op for non-node plugins. + */ + async disablePlugin(pluginId: string): Promise { + await this._pluginMetaPersistenceService.setPluginEnabled(pluginId, false); + this.unloadPlugin(pluginId); + await this.clearNodeExecutionConsent(pluginId); + } + /** * Check if a plugin requires and has consent for Node.js execution */ diff --git a/src/app/plugins/ui/plugin-management/plugin-management.component.ts b/src/app/plugins/ui/plugin-management/plugin-management.component.ts index 567ef53974..26d99f4698 100644 --- a/src/app/plugins/ui/plugin-management/plugin-management.component.ts +++ b/src/app/plugins/ui/plugin-management/plugin-management.component.ts @@ -234,14 +234,11 @@ export class PluginManagementComponent { } try { - // Set plugin as disabled in persistence - await this._pluginMetaPersistenceService.setPluginEnabled( - plugin.manifest.id, - false, - ); - - // Unload the plugin (this will unregister hooks and remove from loaded plugins) - this._pluginService.unloadPlugin(plugin.manifest.id); + // Persist isEnabled=false, tear down the runtime, and revoke nodeExecution consent + // (session grant + persisted) in one place so re-enabling re-prompts — issue #8512 + // Phase 2: "consent is revocable" via the existing toggle, no separate UI. See + // PluginService.disablePlugin for why the revoke is funnelled there. + await this._pluginService.disablePlugin(plugin.manifest.id); // Reload plugins to get the updated state from the service } catch (error) { @@ -325,7 +322,9 @@ export class PluginManagementComponent { this.uploadError.set(null); await this._pluginCacheService.clearCache(); - this._pluginService.clearUploadedPluginsFromMemory(); + // Awaited so persisted nodeExecution consent is cleared for every wiped uploaded + // plugin before the method returns (issue #8512 Phase 2 — see the service method). + await this._pluginService.clearUploadedPluginsFromMemory(); PluginLog.log('Plugin cache cleared successfully'); } catch (error) {