From a67323b2b550f3f422768f63d3e8b1f1dea4c45a Mon Sep 17 00:00:00 2001 From: Johannes Millan Date: Thu, 25 Jun 2026 16:45:54 +0200 Subject: [PATCH] feat(plugins): allow uploaded nodeExecution behind consent gate (#8576) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * feat(plugins): allow uploaded nodeExecution behind consent gate Re-open the nodeExecution permission for uploaded/community plugins (previously built-in only, #8205) behind the existing main-process consent dialog. Phase 1 of #8512; unblocks the Super Productivity MCP plugin (discussion #8385). - Main process sanitizes the attacker-controlled plugin id and the self-declared name/version before they reach the consent dialog or the grant map (control/bidi/whitespace rejected, length-capped). - Bundled vs uploaded is decided by the on-disk manifest, never a renderer-supplied flag, so uploaded code can't borrow a built-in plugin's verified name. - Uploaded-plugin dialog anchors on the validated id, flags the plugin as unverified third-party with full machine access / no sandbox, and defaults to Deny. - Revoke is main-authoritative by (pluginId, webContents) so a re-upload reusing an id can't inherit a live session grant. - Consent stays session-scoped: an in-memory, never-synced denied set prevents re-prompt storms; deny keeps the plugin enabled but fails node calls closed until re-enable or restart. Refs #8512 #8385 * fix(plugins): reject path-segment ids in nodeExecution consent gate Multi-agent review of the Phase 1 gate found the uploaded plugin id — used as a path component in the bundled-manifest existsSync probe — was not rejecting path separators or dot-segments. Impact was bounded (the verified-builtin branch re-validates with the strict kebab regex before any read, so no code exec / file read / dialog spoof), but `..` / `/` left a filesystem-existence oracle and rendered misleadingly as the dialog "Plugin ID". assertSafePluginId now rejects `/`, `\`, `.`, `..`. Also from review: factor the shared Allow/Deny dialog shell, and correct the denied-cache comment (the existing token short-circuit handles the multi-call-site case; the cache only makes a denial sticky across a later non-interactive grant re-entry). Documents the uploaded-id constraints. Refs #8512 * fix(plugins): harden uploaded nodeExecution consent gate (review follow-ups) Addresses multi-agent review findings on the uploaded-plugin nodeExecution consent gate: - id validation: use an allowlist (/^[A-Za-z0-9][A-Za-z0-9._-]*$/) instead of a Unicode denylist, closing bidi/zero-width/homoglyph dialog-anchor spoofing the range list missed (U+061C, U+2060, U+3164, fullwidth chars); strip all Unicode control+format chars from the self-declared display name/version. - never upgrade trust: describeVerifiedBuiltInDialog returns null on any imperfect on-disk verification (id mismatch, missing permission, unreadable manifest) and the grant handler falls back to the unverified dialog, so a colliding uploaded id can never borrow a built-in's verified dialog. - reserve the gitea/linear/trello/azure-devops issue-provider bundled ids (they had drifted out of BUNDLED_PLUGIN_IDS, leaving an impersonation gap) and guard the BUNDLED_PLUGIN_PATHS subset-of BUNDLED_PLUGIN_IDS invariant with a node test. - key the revoke and exec IPC handlers through the same assertSafePluginId as the grant handler so the "revoke by id on teardown/re-upload" guarantee can't drift. - clear the session nodeExecution denial when a plugin is uninstalled, so a fresh re-upload of the same id is prompted again rather than silently failing closed. - de-duplicate the PluginNodeExecutionElectronApi interface into a single electron/shared-with-frontend model (was copied byte-identically in two files). --- docs/plugin-development.md | 22 +- electron/bundled-plugin-ids.test.cjs | 149 ++++++++++++ electron/electronAPI.d.ts | 15 +- electron/plugin-node-executor.test.cjs | 225 +++++++++++++++++- electron/plugin-node-executor.ts | 198 ++++++++++++--- electron/preload.ts | 7 +- .../plugin-node-execution.model.ts | 24 ++ src/app/plugins/plugin-bridge.service.ts | 18 +- .../plugin.service.load-from-zip.spec.ts | 18 +- src/app/plugins/plugin.service.spec.ts | 35 ++- src/app/plugins/plugin.service.ts | 53 ++++- 11 files changed, 686 insertions(+), 78 deletions(-) create mode 100644 electron/bundled-plugin-ids.test.cjs create mode 100644 electron/shared-with-frontend/plugin-node-execution.model.ts diff --git a/docs/plugin-development.md b/docs/plugin-development.md index 47621514b8..85e42e4685 100644 --- a/docs/plugin-development.md +++ b/docs/plugin-development.md @@ -552,10 +552,24 @@ console.log(data); // '{ count: 42 }' Plugins with `"permissions": ["nodeExecution"]` can run Node.js scripts in the Electron desktop app after the user allows the desktop permission prompt. -The desktop grant is currently issued only for packaged built-in plugins whose -manifest can be verified by the main process. Uploaded plugins that request -`nodeExecution` are rejected until uploaded plugin installation is moved to a -main-process-owned verification path. +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). + +> **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 +> characters, `:`, path separators (`/`, `\`), and at most 100 characters. Lowercase +> kebab-case is recommended; dots and uppercase are accepted. + +> **Security note:** a granted `nodeExecution` plugin can run any program with full +> access to your files and system. The file/IPC channel a plugin uses to talk to a +> companion process is an open local channel — treat any data it reads as untrusted +> input (never `eval`/`require` its contents). ```javascript const result = await plugin.executeNodeScript({ diff --git a/electron/bundled-plugin-ids.test.cjs b/electron/bundled-plugin-ids.test.cjs new file mode 100644 index 0000000000..5e4be6b686 --- /dev/null +++ b/electron/bundled-plugin-ids.test.cjs @@ -0,0 +1,149 @@ +const test = require('node:test'); +const assert = require('node:assert/strict'); +const fs = require('node:fs'); +const path = require('node:path'); + +// SECURITY INVARIANT (cross-layer regression guard) +// -------------------------------------------------- +// `src/app/plugins/plugin.service.ts` declares two top-of-file lists: +// - BUNDLED_PLUGIN_PATHS: the on-disk asset dirs of the plugins we ship. +// - BUNDLED_PLUGIN_IDS: the reserved set of *manifest ids* that an uploaded +// plugin is forbidden from claiming. +// The renderer rejects an uploaded plugin whose manifest id is in +// BUNDLED_PLUGIN_IDS so unverified code cannot impersonate a built-in. With +// nodeExecution now openable to uploaded plugins, an unguarded id would also +// let an upload borrow a bundled dir's "verified built-in" consent dialog in +// the main process. PATHS are keyed by on-disk dir name; IDS by manifest id; +// the dir->id mapping is NOT identity (e.g. dir `yesterday-tasks-plugin` has +// manifest id `yesterday-tasks`). The two lists already drifted once (gitea / +// linear / trello / azure issue-providers were in PATHS but missing from IDS), +// which silently opened the impersonation gap for those ids. +// +// This test enforces PATHS ⊆ IDS: every bundled plugin's real manifest id MUST +// be reserved. It deliberately does NOT require equality — IDS may reserve ids +// for plugins not currently shipped via PATHS (e.g. `ai-productivity-prompts`), +// which only widens the reserved set and is harmless. +// +// A Karma/browser unit test cannot read the filesystem, and importing +// plugin.service.ts would drag in the whole Angular DI graph. So we parse the +// file as text here, in the filesystem-capable `node --test` (electron) suite, +// and read manifests straight off disk. + +const REPO_ROOT = path.resolve(__dirname, '..'); +const PLUGIN_SERVICE_PATH = path.join( + REPO_ROOT, + 'src/app/plugins/plugin.service.ts', +); +const PLUGIN_DEV_DIR = path.join(REPO_ROOT, 'packages/plugin-dev'); + +/** + * Extract a named array/Set literal's quoted string entries from the source + * text. We don't evaluate the file (no Angular import) — we slice the literal + * between its opening token and the matching close bracket, then pull every + * single/double-quoted string out of that slice. This stays robust to + * formatting (line breaks, trailing commas, `as const`) without executing code. + * + * @param {string} source full plugin.service.ts text + * @param {string} declStart the literal's opening, e.g. `BUNDLED_PLUGIN_PATHS = [` + * @param {string} closeChar the matching close bracket, `]` or `)` + * @returns {string[]} the quoted entries, in source order + */ +const extractStringLiteralList = (source, declStart, closeChar) => { + const startIdx = source.indexOf(declStart); + assert.notEqual( + startIdx, + -1, + `Could not find "${declStart}" in plugin.service.ts — the const may have been renamed; update this regression test.`, + ); + const contentStart = startIdx + declStart.length; + const closeIdx = source.indexOf(closeChar, contentStart); + assert.notEqual( + closeIdx, + -1, + `Could not find closing "${closeChar}" for "${declStart}" in plugin.service.ts.`, + ); + const slice = source.slice(contentStart, closeIdx); + const matches = slice.match(/['"]([^'"]+)['"]/g) || []; + return matches.map((m) => m.slice(1, -1)); +}; + +/** + * Locate a bundled plugin's manifest. Both layouts exist in the repo: + * packages/plugin-dev//manifest.json + * packages/plugin-dev//src/manifest.json + * Returns the first that exists, or null if neither does. + */ +const findManifestPath = (dirName) => { + const candidates = [ + path.join(PLUGIN_DEV_DIR, dirName, 'manifest.json'), + path.join(PLUGIN_DEV_DIR, dirName, 'src', 'manifest.json'), + ]; + return candidates.find((p) => fs.existsSync(p)) || null; +}; + +test('every BUNDLED_PLUGIN_PATHS plugin has its manifest id reserved in BUNDLED_PLUGIN_IDS', () => { + const source = fs.readFileSync(PLUGIN_SERVICE_PATH, 'utf8'); + + const bundledPaths = extractStringLiteralList( + source, + 'BUNDLED_PLUGIN_PATHS = [', + ']', + ); + const bundledIds = new Set( + extractStringLiteralList(source, 'BUNDLED_PLUGIN_IDS = new Set([', ']'), + ); + + // Sanity: if either list parsed empty, the source format changed and the + // guard is silently inert — fail loudly rather than pass vacuously. + assert.ok( + bundledPaths.length > 0, + 'Parsed zero entries from BUNDLED_PLUGIN_PATHS — the source format likely changed; update this test.', + ); + assert.ok( + bundledIds.size > 0, + 'Parsed zero entries from BUNDLED_PLUGIN_IDS — the source format likely changed; update this test.', + ); + + const missingIds = []; + const missingManifests = []; + + for (const assetPath of bundledPaths) { + const dirName = assetPath.split('/').pop(); + const manifestPath = findManifestPath(dirName); + + if (!manifestPath) { + missingManifests.push(dirName); + continue; + } + + const manifest = JSON.parse(fs.readFileSync(manifestPath, 'utf8')); + const id = manifest.id; + assert.ok( + typeof id === 'string' && id.length > 0, + `Manifest for "${dirName}" has no string "id" field (${manifestPath}).`, + ); + + if (!bundledIds.has(id)) { + missingIds.push(`${dirName} -> "${id}"`); + } + } + + // A missing manifest for a listed dir is itself drift/misconfig. + assert.equal( + missingManifests.length, + 0, + `No manifest.json (top-level or src/) found under packages/plugin-dev for bundled plugin dir(s): ${missingManifests.join( + ', ', + )}. Each BUNDLED_PLUGIN_PATHS entry must have a manifest so its id can be verified.`, + ); + + // The core invariant. List ALL offenders, not just the first. + assert.equal( + missingIds.length, + 0, + `SECURITY: the following bundled plugins' manifest ids are NOT reserved in ` + + `BUNDLED_PLUGIN_IDS (src/app/plugins/plugin.service.ts). An uploaded plugin ` + + `could claim these ids and impersonate a built-in. Add each missing id to ` + + `BUNDLED_PLUGIN_IDS:\n ${missingIds.join('\n ')}`, + ); +}); diff --git a/electron/electronAPI.d.ts b/electron/electronAPI.d.ts index 5053798c19..10c9a28932 100644 --- a/electron/electronAPI.d.ts +++ b/electron/electronAPI.d.ts @@ -10,26 +10,13 @@ import { AppDataCompleteLegacy } from '../src/app/imex/sync/sync.model'; import { Task } from '../src/app/features/tasks/task.model'; import { LocalBackupMeta } from '../src/app/imex/local-backup/local-backup.model'; import { AppDataComplete } from '../src/app/op-log/model/model-config'; -import { - PluginNodeScriptRequest, - PluginNodeScriptResult, -} from '../packages/plugin-api/src/types'; +import { PluginNodeExecutionElectronApi } from './shared-with-frontend/plugin-node-execution.model'; import { LocalRestApiRequestPayload, LocalRestApiResponsePayload, } from './shared-with-frontend/local-rest-api.model'; import { ElectronDistChannel } from './shared-with-frontend/get-dist-channel'; -export interface PluginNodeExecutionElectronApi { - requestGrant(pluginId: string): Promise<{ token: string } | null>; - executeScript( - pluginId: string, - grantToken: string, - request: PluginNodeScriptRequest, - ): Promise; - revokeGrant(pluginId: string, grantToken: string): Promise; -} - export interface ElectronAPI { on( channel: string, diff --git a/electron/plugin-node-executor.test.cjs b/electron/plugin-node-executor.test.cjs index 883aafdba5..0d31ba351b 100644 --- a/electron/plugin-node-executor.test.cjs +++ b/electron/plugin-node-executor.test.cjs @@ -221,7 +221,7 @@ test('does not mint a token when the sender navigates while consent is pending', assert.equal(await grantPromise, null); }); -test('revoke requires the issuing webContents and token', async () => { +test('revoke is scoped to the issuing webContents', async () => { loadModule(); const webContents = new FakeWebContents(6); const otherWebContents = new FakeWebContents(7); @@ -255,3 +255,226 @@ test('revoke requires the issuing webContents and token', async () => { /not authorized/, ); }); + +test('issues a grant to an uploaded (non-bundled) plugin and labels it unverified', async () => { + loadModule(); + const webContents = new FakeWebContents(10); + + const grant = await callIpc( + 'PLUGIN_REQUEST_NODE_EXECUTION_GRANT', + webContents, + 'uploaded-node-plugin', + { name: 'Uploaded Node Plugin', version: '1.2.3' }, + ); + + assert.equal(typeof grant.token, 'string'); + assert.equal(dialogCalls.length, 1); + const opts = dialogCalls[0][1]; + // Dialog anchors on the validated id and flags the self-declared name as unverified, + // and defaults to Deny. + assert.match(opts.detail, /Plugin ID: uploaded-node-plugin/); + assert.match(opts.detail, /self-declared, unverified/); + assert.equal(opts.defaultId, 1); +}); + +test('uploaded plugin executes after grant; a denied request leaves exec unauthorized', async () => { + loadModule(); + const allowWc = new FakeWebContents(11); + const grant = await callIpc( + 'PLUGIN_REQUEST_NODE_EXECUTION_GRANT', + allowWc, + 'uploaded-node-plugin', + { name: 'Uploaded', version: '1.0.0' }, + ); + const okResult = await callIpc( + 'PLUGIN_EXEC_NODE_SCRIPT', + allowWc, + 'uploaded-node-plugin', + grant.token, + { script: 'return args[0] + 1;', args: [4] }, + ); + assert.equal(okResult.success, true); + assert.equal(okResult.result, 5); + + // A denied request mints no token, so exec stays unauthorized. + nextDialogResult = { response: 1 }; + const denyWc = new FakeWebContents(12); + const denied = await callIpc( + 'PLUGIN_REQUEST_NODE_EXECUTION_GRANT', + denyWc, + 'denied-plugin', + { name: 'Denied', version: '1.0.0' }, + ); + assert.equal(denied, null); + await assert.rejects( + () => + callIpc('PLUGIN_EXEC_NODE_SCRIPT', denyWc, 'denied-plugin', 'made-up-token', { + script: 'return true;', + }), + /not authorized/, + ); +}); + +test('accepts a non-kebab uploaded id (dots/uppercase) the built-in rule would reject', async () => { + loadModule(); + const webContents = new FakeWebContents(13); + + const grant = await callIpc( + 'PLUGIN_REQUEST_NODE_EXECUTION_GRANT', + webContents, + 'Community.Plugin-2', + { name: 'Community Plugin', version: '2.0.0' }, + ); + + assert.equal(typeof grant.token, 'string'); +}); + +test('rejects unsafe ids and sanitizes self-declared display strings', async () => { + loadModule(); + const webContents = new FakeWebContents(14); + + // A newline in the id is rejected outright (it is a grant Map key + dialog text). + await assert.rejects( + () => + callIpc('PLUGIN_REQUEST_NODE_EXECUTION_GRANT', webContents, 'evil\nid', { + name: 'x', + version: '1', + }), + /Invalid pluginId/, + ); + + // Path separators / dot-segments are rejected (the id is used as a path component in + // the bundled-manifest existsSync probe). + for (const badId of ['../../etc', '..', '.', 'a/b', 'a\\b']) { + await assert.rejects( + () => + callIpc('PLUGIN_REQUEST_NODE_EXECUTION_GRANT', webContents, badId, { + name: 'x', + version: '1', + }), + /Invalid pluginId/, + ); + } + + // A crafted name cannot inject an extra dialog line and is length-capped. + const craftedName = `${'A'.repeat(500)}\nVerified by Super Productivity`; + const grant = await callIpc( + 'PLUGIN_REQUEST_NODE_EXECUTION_GRANT', + webContents, + 'crafted-name-plugin', + { name: craftedName, version: '1.0.0' }, + ); + assert.equal(typeof grant.token, 'string'); + const detail = dialogCalls[dialogCalls.length - 1][1].detail; + assert.equal(detail.includes('Verified by Super Productivity'), false); + assert.ok(detail.includes('…')); +}); + +test('revoke from the issuing webContents drops the grant even without the token', async () => { + loadModule(); + const webContents = new FakeWebContents(15); + const grant = await callIpc( + 'PLUGIN_REQUEST_NODE_EXECUTION_GRANT', + webContents, + 'uploaded-node-plugin', + { name: 'Uploaded', version: '1.0.0' }, + ); + + // Teardown/re-upload revokes by id without resupplying the token, so a re-uploaded + // plugin reusing the id cannot inherit this live grant. + await callIpc( + 'PLUGIN_REVOKE_NODE_EXECUTION_GRANT', + webContents, + 'uploaded-node-plugin', + '', + ); + await assert.rejects( + () => + callIpc( + 'PLUGIN_EXEC_NODE_SCRIPT', + webContents, + 'uploaded-node-plugin', + grant.token, + { + script: 'return true;', + }, + ), + /not authorized/, + ); +}); + +test('rejects bidi/zero-width/homoglyph and leading-dot ids the allowlist must exclude', async () => { + loadModule(); + const webContents = new FakeWebContents(16); + + // These are exactly the dialog-anchor spoofing vectors a denylist of explicit Unicode + // ranges tends to miss; the allowlist rejects every non-[A-Za-z0-9._-] id by construction. + const badIds = [ + `sync${String.fromCodePoint(0x061c)}md`, // U+061C ARABIC LETTER MARK (bidi) + `sync${String.fromCodePoint(0x2060)}md`, // U+2060 WORD JOINER (zero-width) + `sync${String.fromCodePoint(0x3164)}md`, // U+3164 HANGUL FILLER (invisible) + `${String.fromCodePoint(0xff53)}ync-md`, // U+FF53 fullwidth 's' (homoglyph) + '.hidden', // leading dot-segment + '-leading-dash', + 'a b', // whitespace + ]; + + for (const badId of badIds) { + await assert.rejects( + () => + callIpc('PLUGIN_REQUEST_NODE_EXECUTION_GRANT', webContents, badId, { + name: 'x', + version: '1', + }), + /Invalid pluginId/, + `expected ${JSON.stringify(badId)} to be rejected`, + ); + } +}); + +test('strips bidi/zero-width chars from self-declared display strings', async () => { + loadModule(); + const webContents = new FakeWebContents(17); + + const name = `Tru${String.fromCodePoint(0x061c)}sted${String.fromCodePoint(0x2060)} Plugin`; + const grant = await callIpc( + 'PLUGIN_REQUEST_NODE_EXECUTION_GRANT', + webContents, + 'display-sanitize-plugin', + { name, version: `1.0${String.fromCodePoint(0xfeff)}.0` }, + ); + + assert.equal(typeof grant.token, 'string'); + const detail = dialogCalls[dialogCalls.length - 1][1].detail; + // The control/format chars are gone; the visible text survives. + assert.equal(detail.includes(String.fromCodePoint(0x061c)), false); + assert.equal(detail.includes(String.fromCodePoint(0x2060)), false); + assert.equal(detail.includes(String.fromCodePoint(0xfeff)), false); + assert.match(detail, /Trusted Plugin/); +}); + +test('never upgrades trust: an on-disk match that does not cleanly verify uses the unverified dialog', async () => { + // Simulate a bundled dir whose manifest exists but is not a grantable nodeExecution + // built-in (here: missing the permission). The verified-built-in branch must return + // null and fall back to the unverified-uploaded dialog rather than throw or upgrade. + const originalPermissions = BUILT_IN_PLUGIN_MANIFEST.permissions; + BUILT_IN_PLUGIN_MANIFEST.permissions = []; + try { + loadModule(); + const webContents = new FakeWebContents(18); + const grant = await callIpc( + 'PLUGIN_REQUEST_NODE_EXECUTION_GRANT', + webContents, + BUILT_IN_PLUGIN_MANIFEST.id, + { name: 'Impersonator', version: '9.9.9' }, + ); + + assert.equal(typeof grant.token, 'string'); + const opts = dialogCalls[dialogCalls.length - 1][1]; + assert.match(opts.title, /run code on your machine/); + assert.match(opts.detail, /self-declared, unverified/); + assert.equal(opts.defaultId, 1); + } finally { + BUILT_IN_PLUGIN_MANIFEST.permissions = originalPermissions; + } +}); diff --git a/electron/plugin-node-executor.ts b/electron/plugin-node-executor.ts index 0c1502af83..a623b6d98e 100644 --- a/electron/plugin-node-executor.ts +++ b/electron/plugin-node-executor.ts @@ -16,11 +16,72 @@ const DEFAULT_TIMEOUT = 30000; // 30 seconds const MAX_TIMEOUT = 300000; // 5 minutes const BUILT_IN_PLUGIN_ID_RE = /^[a-z0-9][a-z0-9-]*$/; +// An uploaded (community) plugin id is attacker-controlled and used both as a grant Map +// key and as the consent dialog's trust anchor ("Plugin ID: ..."), and as a path segment +// in getBuiltInManifestPath(). It is NOT held to the strict built-in kebab rule — +// community ids may use dots/uppercase, e.g. `super-productivity-mcp` — but it must be a +// single safe ASCII token. We use an allowlist rather than a denylist on purpose: the +// allowlist rejects control/zero-width/bidi/homoglyph characters that could spoof the +// dialog, whitespace that could inject extra dialog lines, the ':' persistence delimiter, +// and path separators / leading-dot segments ('.', '..', '/', '\\') — all by construction, +// 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._-]*$/; +// 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) +// without enumerating ranges — then collapse whitespace so a crafted value cannot inject +// extra dialog lines. (Global flag is for replace, not test, so no lastIndex statefulness.) +const UNSAFE_DISPLAY_CHARS_RE = /[\p{Cc}\p{Cf}]/gu; + +const assertSafePluginId = (pluginId: unknown): string => { + if (typeof pluginId !== 'string' || pluginId.length === 0) { + throw new Error('Invalid pluginId'); + } + if (pluginId.length > MAX_UPLOADED_PLUGIN_ID_LENGTH) { + throw new Error('Invalid pluginId'); + } + // Allowlist match also rejects path separators ('/'/'\\'), leading-dot segments + // ('.', '..'), ':', whitespace and all non-ASCII (bidi/zero-width/homoglyph), so the id + // can neither escape the bundled-plugins dir in getBuiltInManifestPath() nor spoof the + // consent dialog's trust anchor. + if (!SAFE_UPLOADED_PLUGIN_ID_RE.test(pluginId)) { + throw new Error('Invalid pluginId'); + } + return pluginId; +}; + +const sanitizeDialogString = (value: unknown, maxLength: number): string => { + if (typeof value !== 'string') { + return ''; + } + const cleaned = value.replace(UNSAFE_DISPLAY_CHARS_RE, '').replace(/\s+/g, ' ').trim(); + return cleaned.length > maxLength ? `${cleaned.slice(0, maxLength)}…` : cleaned; +}; + +// Shared shell for both nodeExecution consent dialogs: a warning with Allow/Deny where +// Deny is the default + cancel action, so a reflexive Enter/Escape denies. +const NODE_CONSENT_DIALOG_BASE: Pick< + Electron.MessageBoxOptions, + 'type' | 'buttons' | 'defaultId' | 'cancelId' +> = { + type: 'warning', + buttons: ['Allow', 'Deny'], + defaultId: 1, + cancelId: 1, +}; + interface NodeExecutionGrant { token: string; webContentsId: number; } +/** Self-declared, unverified display metadata supplied by the renderer for uploaded plugins. */ +interface NodeExecutionGrantDisplayInfo { + name?: string; + version?: string; +} + interface WebContentsGrantCleanup { webContents: WebContents; cleanup: () => void; @@ -48,42 +109,42 @@ class PluginNodeExecutor { private setupIpcHandler(): void { ipcMain.handle( IPC.PLUGIN_REQUEST_NODE_EXECUTION_GRANT, - async (event, pluginId: string) => { + async (event, pluginId: string, displayInfo?: NodeExecutionGrantDisplayInfo) => { const window = BrowserWindow.fromWebContents(event.sender); if (!window) { throw new Error('No window found for event sender'); } + // Sanitize first: the id is used as a grant Map key AND shown in the consent + // dialog, and for uploaded plugins it is attacker-controlled. + const safeId = assertSafePluginId(pluginId); + const webContentsId = event.sender.id; - const existingGrant = this.grants.get(pluginId); + const existingGrant = this.grants.get(safeId); if (existingGrant) { if (existingGrant.webContentsId === webContentsId) { return { token: existingGrant.token }; } - this.grants.delete(pluginId); + this.grants.delete(safeId); this.releaseGrantCleanupIfUnused(existingGrant.webContentsId); } - const manifest = this.getVerifiedBuiltInNodeExecutionManifest(pluginId); + // Bundled vs uploaded is decided by the main-owned filesystem, never by a + // renderer-supplied flag, and only an id that resolves to a cleanly-verified + // on-disk manifest gets the trusted built-in dialog. A partial or colliding match + // (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 requestUrl = event.sender.getURL(); this.registerGrantCleanup(event.sender); - let result; + let result: Electron.MessageBoxReturnValue; try { - result = await dialog.showMessageBox(window, { - type: 'warning', - buttons: ['Allow', 'Deny'], - defaultId: 1, - cancelId: 1, - title: 'Allow plugin Node.js execution?', - message: `Allow "${manifest.name}" to run Node.js scripts?`, - detail: [ - `Plugin ID: ${pluginId}`, - `Version: ${manifest.version}`, - '', - 'This permission is valid for the current app session. Node.js execution can access local files and desktop APIs. Only allow plugins you trust.', - ].join('\n'), - }); + result = await dialog.showMessageBox(window, dialogOptions); } catch (error) { this.releaseGrantCleanupIfUnused(webContentsId); throw error; @@ -94,19 +155,19 @@ class PluginNodeExecutor { !this.grantCleanupByWebContents.has(webContentsId) || event.sender.getURL() !== requestUrl ) { - this.grants.delete(pluginId); + this.grants.delete(safeId); this.releaseGrantCleanupIfUnused(webContentsId); return null; } if (result.response !== 0) { - this.grants.delete(pluginId); + this.grants.delete(safeId); this.releaseGrantCleanupIfUnused(webContentsId); return null; } const token = randomBytes(32).toString('base64url'); - this.grants.set(pluginId, { + this.grants.set(safeId, { token, webContentsId, }); @@ -116,10 +177,25 @@ class PluginNodeExecutor { ipcMain.handle( IPC.PLUGIN_REVOKE_NODE_EXECUTION_GRANT, - (event, pluginId: string, grantToken: string) => { - const grant = this.grants.get(pluginId); - if (grant?.token === grantToken && grant.webContentsId === event.sender.id) { - this.grants.delete(pluginId); + // grantToken is accepted for signature compatibility but intentionally not + // required: revoking only removes a capability, and the issuing window must be + // able to drop its own grant during teardown even if it no longer holds the + // token (e.g. on re-upload) — otherwise a re-uploaded plugin reusing the id + // could inherit a live session grant. The webContents binding still prevents + // another window from revoking this one's grant. + (event, pluginId: string, _grantToken?: string) => { + // Key the lookup through the same validator the request handler uses, so the + // "always revoke by id on teardown/re-upload" guarantee holds even if the id + // canonicalisation ever changes (an unsafe id can never hold a grant anyway). + let safeId: string; + try { + safeId = assertSafePluginId(pluginId); + } catch { + return; + } + const grant = this.grants.get(safeId); + if (grant && grant.webContentsId === event.sender.id) { + this.grants.delete(safeId); } }, ); @@ -137,7 +213,15 @@ class PluginNodeExecutor { throw new Error('No window found for event sender'); } - const grant = this.grants.get(pluginId); + // Validate the id the same way the grant handler does so the Map keys match. + // An unsafe id can never hold a grant, so treat it as unauthorized. + let safeId: string; + try { + safeId = assertSafePluginId(pluginId); + } catch { + throw new Error('Plugin is not authorized for nodeExecution'); + } + const grant = this.grants.get(safeId); if ( !grant || grant.token !== grantToken || @@ -146,7 +230,7 @@ class PluginNodeExecutor { throw new Error('Plugin is not authorized for nodeExecution'); } - return await this.executeScript(pluginId, request); + return await this.executeScript(safeId, request); }, ); } @@ -213,6 +297,64 @@ class PluginNodeExecutor { this.unregisterGrantCleanup(webContentsId); } + /** + * 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. + */ + private describeVerifiedBuiltInDialog( + pluginId: string, + ): Electron.MessageBoxOptions | null { + let manifest: PluginManifest; + try { + manifest = this.getVerifiedBuiltInNodeExecutionManifest(pluginId); + } catch { + return null; + } + return { + ...NODE_CONSENT_DIALOG_BASE, + title: 'Allow plugin Node.js execution?', + message: `Allow "${manifest.name}" to run Node.js scripts?`, + detail: [ + `Plugin ID: ${pluginId}`, + `Version: ${manifest.version}`, + '', + 'This permission is valid for the current app session. Node.js execution can access local files and desktop APIs. Only allow plugins you trust.', + ].join('\n'), + }; + } + + /** + * 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 + * the renderer-supplied name/version as self-declared/unverified. Default = Deny. + */ + private describeUnverifiedUploadedDialog( + pluginId: string, + displayInfo?: NodeExecutionGrantDisplayInfo, + ): Electron.MessageBoxOptions { + const name = sanitizeDialogString(displayInfo?.name, 80) || '(unnamed)'; + const version = sanitizeDialogString(displayInfo?.version, 32) || '(unknown)'; + return { + ...NODE_CONSENT_DIALOG_BASE, + title: 'Allow this plugin to run code on your machine?', + message: `Plugin "${pluginId}" wants to run Node.js code`, + detail: [ + `Plugin ID: ${pluginId}`, + `Name (self-declared, unverified): ${name}`, + `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.', + '', + 'Only allow this if you trust the source of this plugin.', + ].join('\n'), + }; + } + private getVerifiedBuiltInNodeExecutionManifest(pluginId: string): PluginManifest { if (typeof pluginId !== 'string' || !pluginId) { throw new Error('Invalid pluginId'); diff --git a/electron/preload.ts b/electron/preload.ts index b034803142..0c9cfa6c30 100644 --- a/electron/preload.ts +++ b/electron/preload.ts @@ -245,8 +245,11 @@ const ea: ElectronAPI = { } pluginNodeExecutionApiConsumed = true; return { - requestGrant: (pluginId: string) => - _invoke('PLUGIN_REQUEST_NODE_EXECUTION_GRANT', pluginId) as Promise<{ + requestGrant: ( + pluginId: string, + displayInfo?: { name?: string; version?: string }, + ) => + _invoke('PLUGIN_REQUEST_NODE_EXECUTION_GRANT', pluginId, displayInfo) as Promise<{ token: string; } | null>, executeScript: ( diff --git a/electron/shared-with-frontend/plugin-node-execution.model.ts b/electron/shared-with-frontend/plugin-node-execution.model.ts new file mode 100644 index 0000000000..c3a9d91558 --- /dev/null +++ b/electron/shared-with-frontend/plugin-node-execution.model.ts @@ -0,0 +1,24 @@ +import { + PluginNodeScriptRequest, + PluginNodeScriptResult, +} from '../../packages/plugin-api/src/types'; + +/** + * Shape of the Electron main-process bridge the renderer uses to grant, run, and + * revoke Node script execution for a plugin. This is a host-internal IPC contract + * (not part of the public plugin API), shared between the renderer + * (`plugin-bridge.service.ts`) and the Electron API typing + * (`electron/electronAPI.d.ts`) so the two cannot drift. + */ +export interface PluginNodeExecutionElectronApi { + requestGrant( + pluginId: string, + displayInfo?: { name?: string; version?: string }, + ): Promise<{ token: string } | null>; + executeScript( + pluginId: string, + grantToken: string, + request: PluginNodeScriptRequest, + ): Promise; + revokeGrant(pluginId: string, grantToken: string): Promise; +} diff --git a/src/app/plugins/plugin-bridge.service.ts b/src/app/plugins/plugin-bridge.service.ts index ade1b59bbf..fe69d1e421 100644 --- a/src/app/plugins/plugin-bridge.service.ts +++ b/src/app/plugins/plugin-bridge.service.ts @@ -117,16 +117,7 @@ import { } from '../features/simple-counter/store/simple-counter.actions'; import { getDbDateStr } from '../util/get-db-date-str'; import { DataInitService } from '../core/data-init/data-init.service'; - -interface PluginNodeExecutionElectronApi { - requestGrant(pluginId: string): Promise<{ token: string } | null>; - executeScript( - pluginId: string, - grantToken: string, - request: PluginNodeScriptRequest, - ): Promise; - revokeGrant(pluginId: string, grantToken: string): Promise; -} +import { PluginNodeExecutionElectronApi } from '../../../electron/shared-with-frontend/plugin-node-execution.model'; type PluginDateFormat = 'short' | 'medium' | 'long' | 'time' | 'datetime'; @@ -1716,8 +1707,11 @@ export class PluginBridgeService implements OnDestroy { return this.#nodeExecutionGrantTokens.get(pluginId); } - async requestNodeExecutionGrant(pluginId: string): Promise<{ token: string } | null> { - return (await this.#nodeExecutionApi?.requestGrant(pluginId)) ?? null; + async requestNodeExecutionGrant( + pluginId: string, + displayInfo?: { name?: string; version?: string }, + ): Promise<{ token: string } | null> { + return (await this.#nodeExecutionApi?.requestGrant(pluginId, displayInfo)) ?? null; } revokeNodeExecutionGrantToken(pluginId: string): string | undefined { 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 5a6afef541..15ee80fb0e 100644 --- a/src/app/plugins/plugin.service.load-from-zip.spec.ts +++ b/src/app/plugins/plugin.service.load-from-zip.spec.ts @@ -220,7 +220,7 @@ describe('PluginService loadPluginFromZip iframe-only plugins', () => { expect(pluginCache.storePlugin).not.toHaveBeenCalled(); }); - it('rejects uploaded plugins that declare nodeExecution before storing or loading code', async () => { + it('accepts uploaded plugins that declare nodeExecution (gated later by main-process consent)', async () => { const nodeExecutionManifest: PluginManifest = { ...iframeManifest, id: 'uploaded-node-plugin', @@ -229,13 +229,17 @@ describe('PluginService loadPluginFromZip iframe-only plugins', () => { }; const files: Record = {}; files['manifest.json'] = JSON.stringify(nodeExecutionManifest); - files['plugin.js'] = 'PluginAPI.log.log("should not run")'; + files['plugin.js'] = 'PluginAPI.log.log("node plugin")'; const file = createZipFile(files); - await expectAsync(service.loadPluginFromZip(file)).toBeRejectedWithError( - T.PLUGINS.NODE_EXECUTION_BUILT_IN_ONLY, - ); - expect(pluginCache.storePlugin).not.toHaveBeenCalled(); - expect(pluginRunner.loadPlugin).not.toHaveBeenCalled(); + // Resolving (instead of throwing NODE_EXECUTION_BUILT_IN_ONLY) is the behaviour + // change: uploaded node plugins are no longer rejected at upload time. The + // nodeExecution capability is gated by the main-process consent dialog at grant + // time instead. + const instance = await service.loadPluginFromZip(file); + + expect(instance).toBeTruthy(); + expect(instance.manifest.id).toBe('uploaded-node-plugin'); + expect(pluginCache.storePlugin).toHaveBeenCalled(); }); }); diff --git a/src/app/plugins/plugin.service.spec.ts b/src/app/plugins/plugin.service.spec.ts index ad3c191275..ad088e7cb9 100644 --- a/src/app/plugins/plugin.service.spec.ts +++ b/src/app/plugins/plugin.service.spec.ts @@ -255,7 +255,10 @@ describe('PluginService', () => { const result = await service.enableAndActivatePlugin(manifest.id); expect(result).toBeNull(); - expect(pluginBridge.requestNodeExecutionGrant).toHaveBeenCalledOnceWith(manifest.id); + expect(pluginBridge.requestNodeExecutionGrant).toHaveBeenCalledOnceWith(manifest.id, { + name: manifest.name, + version: manifest.version, + }); expect(pluginMetaPersistenceService.setPluginEnabled).not.toHaveBeenCalled(); expect(pluginLoader.loadPluginAssets).not.toHaveBeenCalled(); expect(service.getAllPluginStates().get(manifest.id)).toEqual( @@ -266,6 +269,31 @@ describe('PluginService', () => { ); }); + it('does not re-prompt for nodeExecution after a denial within the same session', async () => { + const runtime = service as unknown as { _isElectronRuntime: () => boolean }; + spyOn(runtime, '_isElectronRuntime').and.returnValue(true); + const manifest: PluginManifest = { + ...mockManifest, + id: 'node-plugin', + name: 'Node Plugin', + permissions: ['nodeExecution'], + }; + const ensureGrant = ( + service as unknown as { + _ensureNodeExecutionGrant: (m: PluginManifest) => Promise; + } + )._ensureNodeExecutionGrant.bind(service); + + // First interactive attempt prompts and is denied (requestNodeExecutionGrant -> null). + await expectAsync(service.checkNodeExecutionPermission(manifest)).toBeResolvedTo( + false, + ); + // A later non-interactive grant attempt this session (e.g. startup re-entry via + // _fireOnReady) must NOT re-open the native prompt. + await expectAsync(ensureGrant(manifest)).toBeResolvedTo(false); + expect(pluginBridge.requestNodeExecutionGrant).toHaveBeenCalledTimes(1); + }); + it('stores main-issued nodeExecution grants for Electron plugins', async () => { pluginBridge.requestNodeExecutionGrant.and.resolveTo({ token: 'token-1' }); const runtime = service as unknown as { _isElectronRuntime: () => boolean }; @@ -281,7 +309,10 @@ describe('PluginService', () => { true, ); - expect(pluginBridge.requestNodeExecutionGrant).toHaveBeenCalledOnceWith(manifest.id); + expect(pluginBridge.requestNodeExecutionGrant).toHaveBeenCalledOnceWith(manifest.id, { + name: manifest.name, + version: manifest.version, + }); expect(pluginBridge.setNodeExecutionGrantToken).toHaveBeenCalledOnceWith( manifest.id, 'token-1', diff --git a/src/app/plugins/plugin.service.ts b/src/app/plugins/plugin.service.ts index 4283aeac31..8791c00c3a 100644 --- a/src/app/plugins/plugin.service.ts +++ b/src/app/plugins/plugin.service.ts @@ -62,18 +62,29 @@ const BUNDLED_PLUGIN_PATHS = [ 'assets/bundled-plugins/doc-mode', ] as const; +// Reserved ids: an uploaded plugin may not reuse a bundled plugin's manifest id (it would +// let unverified code impersonate a built-in — and, with nodeExecution now openable to +// uploaded plugins, claim a bundled dir's "verified built-in" consent dialog in the main +// process, which decides bundled-vs-uploaded by on-disk dir). This set MUST contain the +// manifest id of every entry in BUNDLED_PLUGIN_PATHS; the invariant is guarded by +// electron/bundled-plugin-ids.test.cjs (a filesystem-reading node test, since a browser +// Karma spec cannot read the manifests) so the two lists cannot silently drift again. const BUNDLED_PLUGIN_IDS = new Set([ 'ai-productivity-prompts', 'api-test-plugin', 'automations', + 'azure-devops-issue-provider', 'brain-dump', 'caldav-calendar-provider', 'clickup-issue-provider', 'doc-mode', + 'gitea-issue-provider', 'github-issue-provider', 'google-calendar-provider', + 'linear-issue-provider', 'procrastination-buster', 'sync-md', + 'trello-issue-provider', 'voice-reminder', 'yesterday-tasks', ]); @@ -109,6 +120,13 @@ export class PluginService implements OnDestroy { private _pluginIcons: Map = new Map(); // Store plugin ID -> SVG icon content private _pluginIframeGenerations: Map = new Map(); private _pluginIconsSignal = signal>(new Map()); + // Plugin ids the user denied nodeExecution for this app session. In-memory only — + // never persisted or synced (consent is session-scoped). Makes a denial sticky so a + // later non-interactive grant attempt (e.g. startup re-activation via _fireOnReady, + // which doesn't pass through checkNodeExecutionPermission) doesn't re-open the native + // prompt. Added on deny in _ensureNodeExecutionGrant; cleared only on an explicit + // user-initiated enable in checkNodeExecutionPermission (so re-enable always re-asks). + private readonly _nodeExecutionDeniedThisSession = new Set(); // Lazy loading state management private _pluginStates = signal>(new Map()); @@ -1561,6 +1579,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); + // Remove from plugin states this._deletePluginState(pluginId); @@ -1796,14 +1818,25 @@ export class PluginService implements OnDestroy { if (this._pluginBridge.hasNodeExecutionGrantToken(manifest.id)) { return true; } + // A single enable flow reaches this from several call-sites; once the user has + // denied this session, don't re-open the native prompt until they re-enable. + if (this._nodeExecutionDeniedThisSession.has(manifest.id)) { + return false; + } let grant: { token: string } | null; try { - grant = await this._pluginBridge.requestNodeExecutionGrant(manifest.id); + // name/version are sent for the consent dialog only; main treats them as + // self-declared/unverified for uploaded plugins (it never trusts them for auth). + grant = await this._pluginBridge.requestNodeExecutionGrant(manifest.id, { + name: manifest.name, + version: manifest.version, + }); } catch (error) { PluginLog.err(`Failed to get nodeExecution grant for ${manifest.id}:`, error); return false; } if (!grant) { + this._nodeExecutionDeniedThisSession.add(manifest.id); return false; } @@ -1813,10 +1846,13 @@ export class PluginService implements OnDestroy { private async _revokeNodeExecutionGrant(pluginId: string): Promise { const grantToken = this._pluginBridge.revokeNodeExecutionGrantToken(pluginId); - if (!grantToken || !this._isElectronRuntime()) { + if (!this._isElectronRuntime()) { return; } - await this._pluginBridge.revokeNodeExecutionGrant(pluginId, grantToken); + // Always tell main to drop the grant for this id, even if the renderer no longer + // holds the token (main revokes by pluginId + webContents), so a re-upload under + // the same id can never inherit a live session grant. + await this._pluginBridge.revokeNodeExecutionGrant(pluginId, grantToken ?? ''); } /** @@ -1836,6 +1872,9 @@ export class PluginService implements OnDestroy { return false; } + // This is the interactive (user-initiated) entry point, so an explicit enable + // attempt clears any earlier this-session denial and re-opens the prompt. + this._nodeExecutionDeniedThisSession.delete(manifest.id); return this._ensureNodeExecutionGrant(manifest); } @@ -1886,6 +1925,9 @@ export class PluginService implements OnDestroy { } private _assertUploadedPluginAllowed(manifest: PluginManifest): void { + // Uploaded plugins may not reuse a bundled plugin's id (it would let unverified + // code impersonate a built-in). nodeExecution is no longer blocked here: uploaded + // node plugins are gated by the main-process consent dialog at grant time instead. if (this._isBundledPluginId(manifest.id)) { throw new Error( this._translateService.instant(T.PLUGINS.PLUGIN_ID_RESERVED, { @@ -1893,11 +1935,6 @@ export class PluginService implements OnDestroy { }), ); } - if (manifest.permissions?.includes('nodeExecution')) { - throw new Error( - this._translateService.instant(T.PLUGINS.NODE_EXECUTION_BUILT_IN_ONLY), - ); - } } /**