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

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

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(

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: [

src/app/plugins/plugin.service.ts

@@ -110,9 +110,11 @@ export class PluginService implements OnDestroy {
   private _pluginIframeGenerations: Map<string, number> = new Map();
   private _pluginIconsSignal = signal<Map<string, string>>(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<string>();
 
   // Lazy loading state management