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