From 463709e2de7103448fbf4d9e56533f4e983f210c Mon Sep 17 00:00:00 2001 From: Johannes Millan Date: Wed, 15 Jul 2026 10:37:10 +0200 Subject: [PATCH] fix(electron): assert renderer IPC boundary at window creation (#9018) MIME-Version: 1.0 Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 8bit * docs(plugins): add microsoft 365 calendar provider plan * docs(plugins): add microsoft 365 calendar provider plan * docs(ios): plan internal testflight builds * fix(electron): assert renderer IPC boundary at window creation Every IPC trust boundary (Jira one-shot capability, plugin node-exec consent, the window.ea preload bridge) rests on the renderer main world not having require/ipcRenderer, which is guaranteed solely by contextIsolation: true + nodeIntegration: false and sub-frames not getting node integration. If that webPreferences ever silently regressed (a refactor spreading a shared options object, a bad merge), every gate would collapse at once while still looking correct in review. Add web-preferences-guard.ts (assertSecureWebPreferences) and fail closed before creating a window if the boundary is not intact. It rejects a non-true contextIsolation, a non-false nodeIntegration, and (fail-closed) a nodeIntegrationInSubFrames that is not explicitly false; it also directionally rejects an explicit sandbox: false, nodeIntegrationInWorker: true, and webviewTag: true (each off by default, so no call site is forced to set it). Wire it at all three new BrowserWindow sites (main window, task widget, full-screen blocker); the full-screen blocker previously relied on Electron defaults, so set its webPreferences explicitly. A *.test.cjs backs it with behavioral coverage plus a wiring guard that counts constructor sites vs guard calls per file, so a future window cannot silently ship without the check. Closes #9015 * fix(electron): extend webPreferences guard to webSecurity Follow-up hardening from the multi-agent review of #9018: - Reject an explicit `webSecurity: false` (directional, like the sandbox /worker/webviewTag trio). With the app's blanket Access-Control-Allow-Origin: *, disabling the same-origin policy in a node-bridged renderer would widen cross-origin reach — and no call site currently guards against it. - Broaden the wiring-guard test to also require the assert for `new BrowserView` / `new WebContentsView`, closing the tripwire's blind spot for future non-BrowserWindow renderers (none exist today). - Correct the fail() comment: the `throw` narrows the type regardless of return-vs-throw; fail() returns an Error only to DRY the message. 230/230 electron tests pass; checkFile + prettier clean. --- electron/full-screen-blocker.ts | 13 +- electron/main-window.ts | 38 ++++-- electron/task-widget/task-widget.ts | 31 +++-- electron/web-preferences-guard.test.cjs | 166 ++++++++++++++++++++++++ electron/web-preferences-guard.ts | 96 ++++++++++++++ 5 files changed, 319 insertions(+), 25 deletions(-) create mode 100644 electron/web-preferences-guard.test.cjs create mode 100644 electron/web-preferences-guard.ts diff --git a/electron/full-screen-blocker.ts b/electron/full-screen-blocker.ts index 0ce1772343..98dcc60b07 100644 --- a/electron/full-screen-blocker.ts +++ b/electron/full-screen-blocker.ts @@ -1,7 +1,8 @@ -import { BrowserWindow, ipcMain } from 'electron'; +import { BrowserWindow, BrowserWindowConstructorOptions, ipcMain } from 'electron'; import { IPC } from './shared-with-frontend/ipc-events.const'; import { TakeABreakConfig } from '../src/app/features/config/global-config.model'; import { join, normalize } from 'path'; +import { assertSecureWebPreferences } from './web-preferences-guard'; export const initFullScreenBlocker = (IS_DEV: boolean): void => { let isFullScreenWindowOpen = false; @@ -15,6 +16,15 @@ export const initFullScreenBlocker = (IS_DEV: boolean): void => { return; } let isClosable = false; + // This overlay loads a local file with no preload bridge, so it was + // relying on Electron's secure defaults. Set the boundary explicitly and + // assert it, so a future Electron default change can't silently open it. + const webPreferences: BrowserWindowConstructorOptions['webPreferences'] = { + contextIsolation: true, + nodeIntegration: false, + nodeIntegrationInSubFrames: false, + }; + assertSecureWebPreferences(webPreferences, 'full-screen-blocker'); const win = new BrowserWindow({ title: msg, fullscreen: true, @@ -22,6 +32,7 @@ export const initFullScreenBlocker = (IS_DEV: boolean): void => { transparent: true, skipTaskbar: true, frame: false, + webPreferences, }); const randomImgUrl = takeABreakCfg.motivationalImgs?.length ? takeABreakCfg.motivationalImgs[ diff --git a/electron/main-window.ts b/electron/main-window.ts index 8105c31dea..87b747f28f 100644 --- a/electron/main-window.ts +++ b/electron/main-window.ts @@ -31,6 +31,7 @@ import { loadSimpleStoreAll } from './simple-store'; import { SimpleStoreKey } from './shared-with-frontend/simple-store.const'; import { markGpuStartupSuccess } from './gpu-startup-guard'; import { isAppOriginUrl } from './navigation-guard'; +import { assertSecureWebPreferences } from './web-preferences-guard'; import { applyJiraImageAuth } from './jira-image-auth'; let mainWin: BrowserWindow; @@ -187,6 +188,28 @@ export const createWindow = async ({ // the env var the screenshot fixture sets so normal users still get // the default screen-clamping behavior. const isScreenshotMode = process.env.SP_SCREENSHOT_MODE === '1'; + const webPreferences: BrowserWindowConstructorOptions['webPreferences'] = { + scrollBounce: true, + backgroundThrottling: false, + webSecurity: true, + preload: path.join(__dirname, 'preload.js'), + nodeIntegration: false, + // make remote module work with those two settings + contextIsolation: true, + // Untrusted plugin code runs in sub-frame iframes; keep node integration out + // of them explicitly (already the default) so the assert below has a concrete + // value to guard. + nodeIntegrationInSubFrames: false, + // Additional settings for better Linux/Wayland compatibility + enableBlinkFeatures: 'OverlayScrollbar', + // Disable spell checker to prevent connections to Google services (#5314) + // This maintains our "offline-first with zero data collection" promise + spellcheck: false, + }; + // Fail closed if the renderer's IPC trust boundary ever silently regresses: + // contextIsolation/nodeIntegration are what keep require/ipcRenderer out of + // the main world, which every IPC gate (Jira, plugin node-exec) relies on. + assertSecureWebPreferences(webPreferences, 'main'); mainWin = new BrowserWindow({ x: mainWindowState.x, y: mainWindowState.y, @@ -199,20 +222,7 @@ export const createWindow = async ({ titleBarOverlay, enableLargerThanScreen: isScreenshotMode, show: false, - webPreferences: { - scrollBounce: true, - backgroundThrottling: false, - webSecurity: true, - preload: path.join(__dirname, 'preload.js'), - nodeIntegration: false, - // make remote module work with those two settings - contextIsolation: true, - // Additional settings for better Linux/Wayland compatibility - enableBlinkFeatures: 'OverlayScrollbar', - // Disable spell checker to prevent connections to Google services (#5314) - // This maintains our "offline-first with zero data collection" promise - spellcheck: false, - }, + webPreferences, icon: ICONS_FOLDER + '/icon_256x256.png', // Wayland compatibility: disable transparent/frameless features that can cause issues transparent: false, diff --git a/electron/task-widget/task-widget.ts b/electron/task-widget/task-widget.ts index 6739ad867a..57ef141af5 100644 --- a/electron/task-widget/task-widget.ts +++ b/electron/task-widget/task-widget.ts @@ -1,5 +1,11 @@ -import { BrowserWindow, ipcMain, screen } from 'electron'; +import { + BrowserWindow, + BrowserWindowConstructorOptions, + ipcMain, + screen, +} from 'electron'; import { join } from 'path'; +import { assertSecureWebPreferences } from '../web-preferences-guard'; import { TaskCopy } from '../../src/app/features/tasks/task.model'; import { TaskWidgetConfig } from '../../src/app/features/config/global-config.model'; import { info } from 'electron-log/main'; @@ -200,6 +206,19 @@ const createTaskWidgetWindowForGeneration = async ( return; } + const webPreferences: BrowserWindowConstructorOptions['webPreferences'] = { + preload: join(__dirname, 'task-widget-preload.js'), + contextIsolation: true, + nodeIntegration: false, + nodeIntegrationInSubFrames: false, + disableDialogs: true, + webSecurity: true, + allowRunningInsecureContent: false, + backgroundThrottling: false, // Prevent throttling when hidden + }; + // Keep the widget renderer's IPC boundary as tight as the main window's. + assertSecureWebPreferences(webPreferences, 'task-widget'); + // On macOS, transparent + frameless windows do not support native window // dragging or edge resizing (see Electron's BrowserWindow docs: "Transparent // windows are not resizable. Setting `resizable` to `true` may make a @@ -228,15 +247,7 @@ const createTaskWidgetWindowForGeneration = async ( hasShadow: IS_MAC, // Mac: solid window can keep native shadow autoHideMenuBar: true, roundedCorners: IS_MAC, // Mac: rely on OS-native rounded corners - webPreferences: { - preload: join(__dirname, 'task-widget-preload.js'), - contextIsolation: true, - nodeIntegration: false, - disableDialogs: true, - webSecurity: true, - allowRunningInsecureContent: false, - backgroundThrottling: false, // Prevent throttling when hidden - }, + webPreferences, }); taskWidgetWin.loadFile(join(__dirname, 'task-widget.html')); diff --git a/electron/web-preferences-guard.test.cjs b/electron/web-preferences-guard.test.cjs new file mode 100644 index 0000000000..ff07f698b5 --- /dev/null +++ b/electron/web-preferences-guard.test.cjs @@ -0,0 +1,166 @@ +const test = require('node:test'); +const assert = require('node:assert/strict'); +const path = require('node:path'); +const fs = require('node:fs'); + +require('ts-node/register/transpile-only'); + +// Resolve the .ts source via a computed path (matches the other *.test.cjs +// files) so tools/verify-electron-requires.js doesn't flag a literal relative +// require of a file excluded from app.asar. +const { assertSecureWebPreferences } = require( + path.resolve(__dirname, 'web-preferences-guard.ts'), +); + +const SECURE = Object.freeze({ + contextIsolation: true, + nodeIntegration: false, + nodeIntegrationInSubFrames: false, +}); + +test('accepts a fully specified secure webPreferences object', () => { + assert.doesNotThrow(() => assertSecureWebPreferences({ ...SECURE }, 'test')); +}); + +test('accepts extra unrelated keys (preload, webSecurity, etc.)', () => { + assert.doesNotThrow(() => + assertSecureWebPreferences( + { ...SECURE, preload: '/x/preload.js', webSecurity: true, spellcheck: false }, + 'test', + ), + ); +}); + +test('rejects missing webPreferences (relying on Electron defaults)', () => { + assert.throws(() => assertSecureWebPreferences(undefined, 'test'), /no webPreferences/); +}); + +test('rejects contextIsolation !== true (including omitted)', () => { + for (const bad of [false, undefined]) { + assert.throws( + () => assertSecureWebPreferences({ ...SECURE, contextIsolation: bad }, 'test'), + /contextIsolation must be true/, + `contextIsolation: ${String(bad)} must be rejected`, + ); + } +}); + +test('rejects nodeIntegration !== false (including omitted)', () => { + for (const bad of [true, undefined]) { + assert.throws( + () => assertSecureWebPreferences({ ...SECURE, nodeIntegration: bad }, 'test'), + /nodeIntegration must be false/, + `nodeIntegration: ${String(bad)} must be rejected`, + ); + } +}); + +test('rejects nodeIntegrationInSubFrames unless explicitly false', () => { + // Fail-closed: this governs whether the preload bridge reaches plugin iframes, + // so an omitted value (undefined) is rejected too, not just an explicit true. + for (const bad of [true, undefined]) { + assert.throws( + () => + assertSecureWebPreferences( + { ...SECURE, nodeIntegrationInSubFrames: bad }, + 'test', + ), + /nodeIntegrationInSubFrames must be false/, + `nodeIntegrationInSubFrames: ${String(bad)} must be rejected`, + ); + } +}); + +test('rejects an explicit sandbox: false, but allows it omitted', () => { + assert.throws( + () => assertSecureWebPreferences({ ...SECURE, sandbox: false }, 'test'), + /sandbox must not be explicitly false/, + ); + assert.doesNotThrow(() => assertSecureWebPreferences({ ...SECURE }, 'test')); + assert.doesNotThrow(() => + assertSecureWebPreferences({ ...SECURE, sandbox: true }, 'test'), + ); +}); + +test('rejects nodeIntegrationInWorker: true, allows it omitted', () => { + assert.throws( + () => + assertSecureWebPreferences({ ...SECURE, nodeIntegrationInWorker: true }, 'test'), + /nodeIntegrationInWorker must not be true/, + ); + assert.doesNotThrow(() => assertSecureWebPreferences({ ...SECURE }, 'test')); +}); + +test('rejects webviewTag: true, allows it omitted', () => { + assert.throws( + () => assertSecureWebPreferences({ ...SECURE, webviewTag: true }, 'test'), + /webviewTag must not be true/, + ); + assert.doesNotThrow(() => assertSecureWebPreferences({ ...SECURE }, 'test')); +}); + +test('rejects webSecurity: false, allows it omitted or true', () => { + assert.throws( + () => assertSecureWebPreferences({ ...SECURE, webSecurity: false }, 'test'), + /webSecurity must not be explicitly false/, + ); + assert.doesNotThrow(() => assertSecureWebPreferences({ ...SECURE }, 'test')); + assert.doesNotThrow(() => + assertSecureWebPreferences({ ...SECURE, webSecurity: true }, 'test'), + ); +}); + +test('error names the offending window', () => { + assert.throws( + () => assertSecureWebPreferences({ ...SECURE, nodeIntegration: true }, 'task-widget'), + /"task-widget" window/, + ); +}); + +// Wiring guard: every renderer-window constructor in electron/ — `new BrowserWindow`, +// `new BrowserView`, `new WebContentsView` (each carries its own webPreferences) — +// must route through assertSecureWebPreferences. This is the actual regression this +// feature exists to prevent — a NEW window creation site that silently ships without +// the boundary check. Text-scan the sources (importing them would drag in Electron). +// +// We count constructor sites vs guard calls PER FILE rather than a per-file +// boolean, so a second unguarded constructor in an already-guarded file is caught +// too. This stays a heuristic: it cannot see an aliased constructor +// (`const BW = BrowserWindow`) or a window created from a non-`.ts` source, and +// a guard call in a comment would count. Those are acceptable gaps for a tripwire. +test('every renderer-window constructor site has a matching assertSecureWebPreferences call', () => { + const electronDir = __dirname; + const tsFiles = []; + const walk = (dir) => { + for (const entry of fs.readdirSync(dir, { withFileTypes: true })) { + const full = path.join(dir, entry.name); + if (entry.isDirectory()) { + if (entry.name === 'node_modules') continue; + walk(full); + } else if (entry.name.endsWith('.ts') && !entry.name.endsWith('.d.ts')) { + tsFiles.push(full); + } + } + }; + walk(electronDir); + + const count = (src, re) => (src.match(re) || []).length; + const NEW_WINDOW_RE = /new\s+(?:BrowserWindow|BrowserView|WebContentsView)\s*\(/g; + const GUARD_CALL_RE = /assertSecureWebPreferences\s*\(/g; + + const offenders = tsFiles + .map((file) => { + const src = fs.readFileSync(file, 'utf8'); + const windows = count(src, NEW_WINDOW_RE); + const guards = count(src, GUARD_CALL_RE); + return { file: path.relative(electronDir, file), windows, guards }; + }) + .filter(({ windows, guards }) => windows > guards); + + assert.deepEqual( + offenders, + [], + 'These files create more BrowserWindows than they guard. Route each ' + + 'webPreferences through assertSecureWebPreferences() before creating the window.', + ); +}); diff --git a/electron/web-preferences-guard.ts b/electron/web-preferences-guard.ts new file mode 100644 index 0000000000..154d84bc7a --- /dev/null +++ b/electron/web-preferences-guard.ts @@ -0,0 +1,96 @@ +import { BrowserWindowConstructorOptions } from 'electron'; + +type WebPreferences = BrowserWindowConstructorOptions['webPreferences']; + +/** + * Fail-closed guard for a renderer's security-critical webPreferences. + * + * Every IPC trust boundary in the app — the Jira one-shot capability, plugin + * node-execution consent, the `window.ea` preload bridge — ultimately rests on + * the renderer main world NOT having `require` / `ipcRenderer`. That property is + * guaranteed solely by `contextIsolation: true` + `nodeIntegration: false`, plus + * sub-frames (where untrusted plugin iframes run) not getting node integration. + * If any of those silently regressed — a refactor spreading a shared options + * object, a bad merge, a copy-paste into a new window — every one of those gates + * would collapse at once while still looking correct in a diff. + * + * This asserts the invariant at window creation and throws BEFORE the window + * loads, so an accidental regression fails the app at startup / in CI instead of + * shipping a renderer that plugin code can fully own. It is a tripwire against + * accidental drift, not a defense against a developer who deliberately flips a + * flag (they would delete this call too). + * + * Two kinds of check: + * - The three core boundary flags — `contextIsolation`, `nodeIntegration`, + * `nodeIntegrationInSubFrames` — are **fail-closed**: an omitted/`undefined` + * value is rejected too, so the guard never depends on the Electron default + * staying safe across upgrades. (Sub-frames are included because that flag + * governs whether the preload bridge reaches plugin iframes.) + * - The additional insecure overrides — `sandbox`, `nodeIntegrationInWorker`, + * `webviewTag`, `webSecurity` — are checked **directionally**: only an explicit + * insecure value is rejected; an omitted key keeps Electron's secure default so + * no call site is forced to enumerate them. These stay default-dependent by choice. + * + * Scope notes: + * - Electron exposes no getter for a webContents' *effective* webPreferences, so + * this can only validate the options object we pass to the constructor. + * - The wiring-guard test requires this call for `new BrowserWindow`, + * `new BrowserView`, and `new WebContentsView`. A `` guest has no such + * constructor and would still need its own validation (e.g. a + * `will-attach-webview` handler) — none of these exist today. + */ +export const assertSecureWebPreferences = ( + webPreferences: WebPreferences, + windowLabel: string, +): void => { + // Returns an Error (callers `throw fail(...)`) so the shared message prefix/suffix + // is defined once — mirroring the `throw fail(...)` shape of the sibling guard + // `file-path-guard.ts` (that one also hardens the error for the renderer; here the + // error only ever surfaces in the main process, so it needs no such hardening). + const fail = (detail: string): Error => + new Error( + `Insecure webPreferences for the "${windowLabel}" window: ${detail}. ` + + 'This would collapse the renderer IPC trust boundary — refusing to create the window.', + ); + + if (!webPreferences) { + throw fail('no webPreferences set (relying on Electron defaults)'); + } + // Core boundary flags — fail-closed (reject omitted/undefined too). + if (webPreferences.contextIsolation !== true) { + throw fail( + `contextIsolation must be true (got ${String(webPreferences.contextIsolation)})`, + ); + } + if (webPreferences.nodeIntegration !== false) { + throw fail( + `nodeIntegration must be false (got ${String(webPreferences.nodeIntegration)})`, + ); + } + if (webPreferences.nodeIntegrationInSubFrames !== false) { + throw fail( + `nodeIntegrationInSubFrames must be false (got ${String(webPreferences.nodeIntegrationInSubFrames)})`, + ); + } + // Additional node-capability surfaces — directional (reject explicit insecure + // value only). Disabling the sandbox re-enables full Node in the preload; a + // Node-enabled worker or a guest would each open a path around the + // IPC/consent boundary. + if (webPreferences.sandbox === false) { + throw fail('sandbox must not be explicitly false'); + } + if (webPreferences.nodeIntegrationInWorker === true) { + throw fail('nodeIntegrationInWorker must not be true'); + } + if (webPreferences.webviewTag === true) { + throw fail( + 'webviewTag must not be true (a guest needs its own validation)', + ); + } + // webSecurity is the same-origin policy rather than a node capability, but with the + // app's blanket Access-Control-Allow-Origin: * an explicit `false` here would widen + // a node-bridged renderer's cross-origin reach — reject it (directional, like above). + if (webPreferences.webSecurity === false) { + throw fail('webSecurity must not be explicitly false'); + } +};