From cd36173af4fdd2a0699395f15d3f5c7d4dcf5a77 Mon Sep 17 00:00:00 2001 From: Johannes Millan Date: Wed, 24 Jun 2026 11:45:25 +0200 Subject: [PATCH] fix(plugins): reject path-segment ids in nodeExecution consent gate MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit 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 --- docs/plugin-development.md | 5 ++++ electron/plugin-node-executor.test.cjs | 13 ++++++++++ electron/plugin-node-executor.ts | 33 +++++++++++++++++++------- src/app/plugins/plugin.service.ts | 8 ++++--- 4 files changed, 48 insertions(+), 11 deletions(-) diff --git a/docs/plugin-development.md b/docs/plugin-development.md index 35e868965e..85e42e4685 100644 --- a/docs/plugin-development.md +++ b/docs/plugin-development.md @@ -561,6 +561,11 @@ defaults to **Deny** — only allow plugins whose source you trust. If the user 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 diff --git a/electron/plugin-node-executor.test.cjs b/electron/plugin-node-executor.test.cjs index d6ced9c417..c43242a1eb 100644 --- a/electron/plugin-node-executor.test.cjs +++ b/electron/plugin-node-executor.test.cjs @@ -343,6 +343,19 @@ test('rejects unsafe ids and sanitizes self-declared display strings', async () /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( diff --git a/electron/plugin-node-executor.ts b/electron/plugin-node-executor.ts index f51c479945..79282330d8 100644 --- a/electron/plugin-node-executor.ts +++ b/electron/plugin-node-executor.ts @@ -40,6 +40,17 @@ const assertSafePluginId = (pluginId: unknown): string => { if (UNSAFE_ID_CHARS_RE.test(pluginId)) { throw new Error('Invalid pluginId'); } + // The id is used as a path segment in getBuiltInManifestPath() (existsSync probe), + // so reject path separators and dot-segments — otherwise an id like '..' could probe + // outside the bundled-plugins dir and render misleadingly in the dialog. + if ( + pluginId.includes('/') || + pluginId.includes('\\') || + pluginId === '.' || + pluginId === '..' + ) { + throw new Error('Invalid pluginId'); + } return pluginId; }; @@ -51,6 +62,18 @@ const sanitizeDialogString = (value: unknown, maxLength: number): string => { 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; @@ -261,10 +284,7 @@ class PluginNodeExecutor { private describeVerifiedBuiltInDialog(pluginId: string): Electron.MessageBoxOptions { const manifest = this.getVerifiedBuiltInNodeExecutionManifest(pluginId); return { - type: 'warning', - buttons: ['Allow', 'Deny'], - defaultId: 1, - cancelId: 1, + ...NODE_CONSENT_DIALOG_BASE, title: 'Allow plugin Node.js execution?', message: `Allow "${manifest.name}" to run Node.js scripts?`, detail: [ @@ -288,10 +308,7 @@ class PluginNodeExecutor { const name = sanitizeDialogString(displayInfo?.name, 80) || '(unnamed)'; const version = sanitizeDialogString(displayInfo?.version, 32) || '(unknown)'; return { - type: 'warning', - buttons: ['Allow', 'Deny'], - defaultId: 1, - cancelId: 1, + ...NODE_CONSENT_DIALOG_BASE, title: 'Allow this plugin to run code on your machine?', message: `Plugin "${pluginId}" wants to run Node.js code`, detail: [ diff --git a/src/app/plugins/plugin.service.ts b/src/app/plugins/plugin.service.ts index 1925a35b09..6ebd642bbf 100644 --- a/src/app/plugins/plugin.service.ts +++ b/src/app/plugins/plugin.service.ts @@ -110,9 +110,11 @@ export class PluginService implements OnDestroy { 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). Prevents re-prompting from - // the multiple grant call-sites of a single enable flow, and keeps a denial sticky - // until the user explicitly re-enables the plugin. Cleared in checkNodeExecutionPermission. + // 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