fix(security): stored XSS → RCE chain — note image, plugin nodeExecution, CSP (GHSA-78rv-m663-4fph) (#8178)

* fix(ui): prevent stored XSS in enlarge-img directive

The enlarged-image element was built by interpolating the image URL into an innerHTML string, so a crafted synced/imported note.imgUrl could break out of the src attribute and inject an event handler (stored DOM-XSS). Build the <img> with createElement and property assignment instead, so the URL is never parsed as HTML. Adds a regression spec.

Refs: GHSA-78rv-m663-4fph

* fix(plugins): authorize nodeExecution from main-process state

The Electron main process authorized Node execution from the manifest the renderer passes on each IPC call, and window.ea.pluginExecNodeScript is callable by any renderer code — so injected renderer JS could forge {permissions:['nodeExecution']} and run arbitrary Node (CWE-501).

The main process now keeps its own grantedPlugins set, populated via a dedicated PLUGIN_SET_NODE_CONSENT channel. The renderer registers a grant in _fireOnReady (before the first node call, covering every load path including zip upload) and revokes it on teardown. The executor requires set membership.

Defense in depth, not a hard boundary: the registration channel is itself renderer-reachable, so a fully-compromised renderer could register a grant itself. It blocks the disclosed PoC (forged manifest for a never-loaded plugin) and removes per-call manifest trust. A hard boundary needs a main-process consent dialog.

Refs: GHSA-78rv-m663-4fph

* fix(security): add object-src 'none' and document CSP constraints

Adds object-src 'none' (the app embeds no <object>/<embed>) and replaces the stale TODO with an accurate note on why 'unsafe-eval'/'unsafe-inline' cannot be dropped yet: the plugin runtime and the inline bootstrap script use new Function, and the packaged app loads the renderer over file:// (opaque origin), so tightening script-src to 'self' is unreliable.

Refs: GHSA-78rv-m663-4fph

electron/electronAPI.d.ts

@@ -232,6 +232,11 @@ export interface ElectronAPI {
     request: PluginNodeScriptRequest,
   ): Promise<PluginNodeScriptResult>;
 
+  // Register/revoke a plugin's nodeExecution grant with the main process so the
+  // executor authorizes from its own state, not the per-call manifest.
+  // See GHSA-78rv-m663-4fph.
+  pluginSetNodeConsent(pluginId: string, isGranted: boolean): Promise<void>;
+
   // Plugin OAuth
   pluginOAuthPrepare(): Promise<{ port: number }>;
   pluginOAuthStart(url: string): void;

electron/plugin-node-executor.ts

@@ -12,11 +12,40 @@ const DEFAULT_TIMEOUT = 30000; // 30 seconds
 const MAX_TIMEOUT = 300000; // 5 minutes
 
 class PluginNodeExecutor {
+  /**
+   * Authoritative set of plugin IDs the renderer has registered as having
+   * user-granted nodeExecution consent. Held in the main process so node
+   * execution is authorized from our own state — NOT from the manifest the
+   * renderer passes on each call, which a compromised/XSS'd renderer could
+   * forge ({ permissions: ['nodeExecution'] }). See GHSA-78rv-m663-4fph.
+   *
+   * In-memory by design: it resets on app restart and the renderer re-registers
+   * each consented plugin when it re-activates it on boot.
+   */
+  private readonly grantedPlugins = new Set<string>();
+
   constructor() {
     this.setupIpcHandler();
   }
 
   private setupIpcHandler(): void {
+    // Trusted out-of-band channel: the renderer registers (or revokes) a
+    // plugin's nodeExecution grant here, separately from the exec call, after
+    // it has verified user consent.
+    ipcMain.handle(
+      IPC.PLUGIN_SET_NODE_CONSENT,
+      (_event, pluginId: string, isGranted: boolean) => {
+        if (typeof pluginId !== 'string' || !pluginId) {
+          throw new Error('Invalid pluginId');
+        }
+        if (isGranted) {
+          this.grantedPlugins.add(pluginId);
+        } else {
+          this.grantedPlugins.delete(pluginId);
+        }
+      },
+    );
+
     ipcMain.handle(
       IPC.PLUGIN_EXEC_NODE_SCRIPT,
       async (
@@ -30,7 +59,13 @@ class PluginNodeExecutor {
           throw new Error('No window found for event sender');
         }
 
-        // Check permissions
+        // SECURITY: authorize from main-process-held state, not the per-call
+        // manifest the renderer supplies (the renderer-supplied manifest is not
+        // trustworthy — CWE-501). The manifest check is kept only as a secondary
+        // sanity gate. See GHSA-78rv-m663-4fph.
+        if (!this.grantedPlugins.has(pluginId)) {
+          throw new Error('Plugin is not authorized for nodeExecution');
+        }
         if (!manifest.permissions?.includes('nodeExecution')) {
           throw new Error('Plugin does not have nodeExecution permission');
         }

electron/preload.ts

@@ -260,6 +260,12 @@ const ea: ElectronAPI = {
       request,
     ) as Promise<PluginNodeScriptResult>,
 
+  // Register/revoke a plugin's nodeExecution grant with the main process. The
+  // executor authorizes from this out-of-band state, not the per-call manifest.
+  // See GHSA-78rv-m663-4fph.
+  pluginSetNodeConsent: (pluginId: string, isGranted: boolean) =>
+    _invoke('PLUGIN_SET_NODE_CONSENT', pluginId, isGranted) as Promise<void>,
+
   // Plugin OAuth
   pluginOAuthPrepare: () => _invoke('PLUGIN_OAUTH_PREPARE') as Promise<{ port: number }>,
   pluginOAuthStart: (url: string) => _send('PLUGIN_OAUTH_START', { url }),

electron/shared-with-frontend/ipc-events.const.ts

@@ -83,6 +83,10 @@ export enum IPC {
 
   // Plugin Node Execution
   PLUGIN_EXEC_NODE_SCRIPT = 'PLUGIN_EXEC_NODE_SCRIPT',
+  // Renderer registers/revokes a plugin's nodeExecution grant in the main
+  // process, so the executor authorizes from its own state instead of trusting
+  // the per-call manifest. See GHSA-78rv-m663-4fph.
+  PLUGIN_SET_NODE_CONSENT = 'PLUGIN_SET_NODE_CONSENT',
 
   // Plugin OAuth
   PLUGIN_OAUTH_PREPARE = 'PLUGIN_OAUTH_PREPARE',

src/app/plugins/plugin.service.ts

@@ -501,7 +501,8 @@ export class PluginService implements OnDestroy {
       return null;
     }
 
-    // Load the plugin
+    // Load the plugin (nodeExecution authorization is registered in _fireOnReady,
+    // the common chokepoint for all load paths — see GHSA-78rv-m663-4fph).
     this._setPluginState(pluginId, {
       ...currentState,
       status: 'loading',
@@ -603,6 +604,12 @@ export class PluginService implements OnDestroy {
       return;
     }
     if (IS_ELECTRON && instance.manifest.permissions?.includes('nodeExecution')) {
+      // SECURITY: authorize this plugin for Node execution in the main process
+      // BEFORE the first node call (the ping). This is the single chokepoint for
+      // every load path (startup, manual activation, zip upload), and reaching
+      // onReady for a nodeExecution plugin means activation already verified
+      // consent. See GHSA-78rv-m663-4fph.
+      await this._setNodeExecutionGrant(instance.manifest.id, true);
       await this._pingNodeBridge(instance.manifest);
     }
     await this._pluginRunner.triggerReady(instance.manifest.id);
@@ -1538,6 +1545,14 @@ export class PluginService implements OnDestroy {
     this._pluginHooks.unregisterPluginHooks(pluginId);
     this._pluginI18nService.unloadPluginTranslations(pluginId);
     this._pluginRunner.unloadPlugin(pluginId);
+
+    // SECURITY: revoke the main-process nodeExecution grant on teardown, so a
+    // disabled/uninstalled plugin cannot run Node for the rest of the session.
+    // Best-effort and fire-and-forget (teardown is synchronous); revoking by id
+    // is a no-op for plugins that never held a grant. See GHSA-78rv-m663-4fph.
+    void this._setNodeExecutionGrant(pluginId, false).catch((e) =>
+      PluginLog.err(`Failed to revoke nodeExecution grant for ${pluginId}`, e),
+    );
   }
 
   unloadPlugin(pluginId: string): boolean {
@@ -1676,6 +1691,23 @@ export class PluginService implements OnDestroy {
     }
   }
 
+  /**
+   * Register (or revoke) a plugin's nodeExecution grant with the Electron main
+   * process, so the main process authorizes Node execution from its own state
+   * rather than the per-call manifest the renderer passes (CWE-501). The full
+   * rationale (and the "defense in depth, not a hard boundary" caveat) lives on
+   * the executor — see electron/plugin-node-executor.ts and GHSA-78rv-m663-4fph.
+   * No-op outside Electron.
+   */
+  private async _setNodeExecutionGrant(
+    pluginId: string,
+    isGranted: boolean,
+  ): Promise<void> {
+    if (IS_ELECTRON) {
+      await window.ea.pluginSetNodeConsent(pluginId, isGranted);
+    }
+  }
+
   /**
    * Check if a plugin requires and has consent for Node.js execution
    */

src/app/ui/enlarge-img/enlarge-img.directive.spec.ts

@@ -0,0 +1,69 @@
+import { Component } from '@angular/core';
+import { ComponentFixture, TestBed } from '@angular/core/testing';
+import { EnlargeImgDirective } from './enlarge-img.directive';
+
+@Component({
+  template: `<img
+    [enlargeImg]="url"
+    src="assets/icons/icon-128x128.png"
+  />`,
+  imports: [EnlargeImgDirective],
+})
+class TestHostComponent {
+  url = '';
+}
+
+describe('EnlargeImgDirective', () => {
+  let fixture: ComponentFixture<TestHostComponent>;
+  let hostImgEl: HTMLImageElement;
+
+  const getEnlargedImg = (): HTMLImageElement | null =>
+    document.getElementById('enlarged-img') as HTMLImageElement | null;
+
+  beforeEach(() => {
+    TestBed.configureTestingModule({ imports: [TestHostComponent] });
+    fixture = TestBed.createComponent(TestHostComponent);
+    hostImgEl = fixture.nativeElement.querySelector('img');
+  });
+
+  afterEach(() => {
+    // The directive appends the lightbox to <body>; remove it between tests.
+    document.querySelectorAll('.enlarged-image-wrapper').forEach((el) => el.remove());
+    delete (window as unknown as { __xssFired?: boolean }).__xssFired;
+  });
+
+  // Regression test for GHSA-78rv-m663-4fph: a crafted note.imgUrl must not be
+  // able to break out of the src attribute and inject an event handler.
+  it('keeps a malicious imgUrl as a literal src without injecting markup', () => {
+    const payload =
+      'assets/icons/icon-128x128.png#" onload="window.__xssFired = true" x="';
+    fixture.componentInstance.url = payload;
+    fixture.detectChanges();
+
+    hostImgEl.click();
+
+    const enlarged = getEnlargedImg();
+    expect(enlarged)
+      .withContext('enlarged image element should be created')
+      .not.toBeNull();
+    // The whole payload stays the literal src value (set as a DOM property);
+    // with the old innerHTML sink this would be truncated at the closing quote.
+    expect(enlarged!.getAttribute('src')).toBe(payload);
+    // No attacker-controlled attributes leaked out of the src string.
+    expect(enlarged!.hasAttribute('onload')).toBe(false);
+    expect(enlarged!.hasAttribute('x')).toBe(false);
+    expect((window as unknown as { __xssFired?: boolean }).__xssFired).toBeUndefined();
+  });
+
+  it('enlarges a normal image url unchanged', () => {
+    const url = 'assets/icons/icon-128x128.png';
+    fixture.componentInstance.url = url;
+    fixture.detectChanges();
+
+    hostImgEl.click();
+
+    const enlarged = getEnlargedImg();
+    expect(enlarged).not.toBeNull();
+    expect(enlarged!.getAttribute('src')).toBe(url);
+  });
+});

src/app/ui/enlarge-img/enlarge-img.directive.ts

@@ -92,9 +92,15 @@ export class EnlargeImgDirective {
     this.enlargedImgWrapperEl = this._htmlToElement(
       `<div class="enlarged-image-wrapper"></div>`,
     );
-    this.newImageEl = this._htmlToElement(
-      `<img src="${src}" class="enlarged-image" id=${LARGE_IMG_ID}>`,
-    );
+    // SECURITY: build the <img> via DOM properties, never innerHTML. `src` can
+    // originate from synced/imported note data (note.imgUrl); interpolating it
+    // into an HTML string let a crafted URL break out of the src attribute and
+    // inject an event handler (stored DOM-XSS). See GHSA-78rv-m663-4fph.
+    const newImageEl = document.createElement('img');
+    newImageEl.src = src;
+    newImageEl.className = 'enlarged-image';
+    newImageEl.id = LARGE_IMG_ID;
+    this.newImageEl = newImageEl;
     this._renderer.appendChild(this.enlargedImgWrapperEl, this.newImageEl);
     this._renderer.appendChild(this.lightboxParentEl, this.enlargedImgWrapperEl);
     this.zoomMode = 0;

src/index.html

@@ -71,10 +71,19 @@
       content="assets/icons/browserconfig.xml"
       name="msapplication-config"
     />
-    <!-- Enable all requests and inline styles for painless development.
-TODO configure more restrictive Content-Security-Policy
-@see https://content-security-policy.com/
-@see https://www.html5rocks.com/en/tutorials/security/content-security-policy/ -->
+    <!-- Content-Security-Policy. See GHSA-78rv-m663-4fph.
+      'unsafe-eval' and 'unsafe-inline' currently CANNOT be dropped:
+        - the plugin runtime runs plugin code via `new Function`
+          (src/app/plugins/plugin-runner.ts) → needs 'unsafe-eval';
+        - the inline browser-compat bootstrap <script> below also uses
+          `new Function` and is inline → needs both.
+      The packaged app loads the renderer over file:// (electron/main-window.ts),
+      where 'self' is an opaque origin, so tightening script-src to 'self' is
+      unreliable. A real lockdown (dropping unsafe-inline/unsafe-eval) needs:
+      a registered app:// scheme instead of file://, plugin execution moved into
+      a sandboxed iframe/worker, and the inline bootstrap externalized.
+      object-src 'none' is safe to set now (the app embeds no <object>/<embed>).
+      @see https://content-security-policy.com/ -->
     <meta
       content="default-src * uploaded:;
           connect-src * data: blob: filesystem: uploaded:;
@@ -82,6 +91,7 @@ TODO configure more restrictive Content-Security-Policy
           font-src * data:;
           img-src * data: blob: filesystem: file:;
           style-src * 'unsafe-inline' ;
+          object-src 'none';
           script-src * 'self' 'unsafe-inline' 'unsafe-eval' blob:"
       http-equiv="Content-Security-Policy"
     />