From 4a99e7e50539b94162e6f432e10a328ed05521db Mon Sep 17 00:00:00 2001 From: Johannes Millan Date: Mon, 8 Jun 2026 20:20:17 +0200 Subject: [PATCH] =?UTF-8?q?fix(security):=20stored=20XSS=20=E2=86=92=20RCE?= =?UTF-8?q?=20chain=20=E2=80=94=20note=20image,=20plugin=20nodeExecution,?= =?UTF-8?q?=20CSP=20(GHSA-78rv-m663-4fph)=20(#8178)?= MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * 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 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 /) 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 | 5 ++ electron/plugin-node-executor.ts | 37 +++++++++- electron/preload.ts | 6 ++ .../shared-with-frontend/ipc-events.const.ts | 4 ++ src/app/plugins/plugin.service.ts | 34 ++++++++- .../enlarge-img/enlarge-img.directive.spec.ts | 69 +++++++++++++++++++ .../ui/enlarge-img/enlarge-img.directive.ts | 12 +++- src/index.html | 18 +++-- 8 files changed, 176 insertions(+), 9 deletions(-) create mode 100644 src/app/ui/enlarge-img/enlarge-img.directive.spec.ts diff --git a/electron/electronAPI.d.ts b/electron/electronAPI.d.ts index 8447daab9f..a9e9a1b827 100644 --- a/electron/electronAPI.d.ts +++ b/electron/electronAPI.d.ts @@ -232,6 +232,11 @@ export interface ElectronAPI { request: PluginNodeScriptRequest, ): Promise; + // 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; + // Plugin OAuth pluginOAuthPrepare(): Promise<{ port: number }>; pluginOAuthStart(url: string): void; diff --git a/electron/plugin-node-executor.ts b/electron/plugin-node-executor.ts index 9a01e7f7f3..a080a51cd5 100644 --- a/electron/plugin-node-executor.ts +++ b/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(); + 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'); } diff --git a/electron/preload.ts b/electron/preload.ts index f7ba75d213..3fb7494b3f 100644 --- a/electron/preload.ts +++ b/electron/preload.ts @@ -260,6 +260,12 @@ const ea: ElectronAPI = { request, ) as Promise, + // 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, + // Plugin OAuth pluginOAuthPrepare: () => _invoke('PLUGIN_OAUTH_PREPARE') as Promise<{ port: number }>, pluginOAuthStart: (url: string) => _send('PLUGIN_OAUTH_START', { url }), diff --git a/electron/shared-with-frontend/ipc-events.const.ts b/electron/shared-with-frontend/ipc-events.const.ts index c31e70911e..e0f452af3f 100644 --- a/electron/shared-with-frontend/ipc-events.const.ts +++ b/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', diff --git a/src/app/plugins/plugin.service.ts b/src/app/plugins/plugin.service.ts index b3f2574d28..2ed333ea5c 100644 --- a/src/app/plugins/plugin.service.ts +++ b/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 { + if (IS_ELECTRON) { + await window.ea.pluginSetNodeConsent(pluginId, isGranted); + } + } + /** * Check if a plugin requires and has consent for Node.js execution */ diff --git a/src/app/ui/enlarge-img/enlarge-img.directive.spec.ts b/src/app/ui/enlarge-img/enlarge-img.directive.spec.ts new file mode 100644 index 0000000000..b28e888d2f --- /dev/null +++ b/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: ``, + imports: [EnlargeImgDirective], +}) +class TestHostComponent { + url = ''; +} + +describe('EnlargeImgDirective', () => { + let fixture: ComponentFixture; + 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 ; 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); + }); +}); diff --git a/src/app/ui/enlarge-img/enlarge-img.directive.ts b/src/app/ui/enlarge-img/enlarge-img.directive.ts index d6c512ef7f..a06d25cfe4 100644 --- a/src/app/ui/enlarge-img/enlarge-img.directive.ts +++ b/src/app/ui/enlarge-img/enlarge-img.directive.ts @@ -92,9 +92,15 @@ export class EnlargeImgDirective { this.enlargedImgWrapperEl = this._htmlToElement( `
`, ); - this.newImageEl = this._htmlToElement( - ``, - ); + // SECURITY: build the 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; diff --git a/src/index.html b/src/index.html index 3e8c3ade3b..a747fb5eca 100644 --- a/src/index.html +++ b/src/index.html @@ -71,10 +71,19 @@ content="assets/icons/browserconfig.xml" name="msapplication-config" /> - +