mirror of
https://github.com/johannesjo/super-productivity.git
synced 2026-07-17 16:37:43 +00:00
fix(electron): assert renderer IPC boundary at window creation (#9018)
* 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.
This commit is contained in:
parent
f665536ef3
commit
463709e2de
5 changed files with 319 additions and 25 deletions
|
|
@ -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[
|
||||
|
|
|
|||
|
|
@ -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,
|
||||
|
|
|
|||
|
|
@ -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'));
|
||||
|
|
|
|||
166
electron/web-preferences-guard.test.cjs
Normal file
166
electron/web-preferences-guard.test.cjs
Normal file
|
|
@ -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.',
|
||||
);
|
||||
});
|
||||
96
electron/web-preferences-guard.ts
Normal file
96
electron/web-preferences-guard.ts
Normal file
|
|
@ -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 `<webview>` 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 <webview> 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 <webview> 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');
|
||||
}
|
||||
};
|
||||
Loading…
Add table
Add a link
Reference in a new issue