From 0269501d4f7038cfc0b45692e1b157dc9f6b9ac1 Mon Sep 17 00:00:00 2001 From: Travis Ralston Date: Wed, 20 May 2020 16:16:57 -0600 Subject: [PATCH] Enforce sandbox on all spawned BrowserWindow objects The docs (https://www.atom.pe/docs/api/sandbox-option/) say we should be using the browser-native `window.open` implementation, but in practice that appears very much false. Electron, no matter our set of options, appears to always make a hit to the ipcRenderer with `window.open` calls, causing the calling code to explode due to the sandbox making that impossible. By using `app.enableSandbox()`, it puts the sandbox in place over all BrowserWindow objects, including the temporary ones which empirically are being created for `window.open`. We do not need to specify `sandbox: true` to the BrowserWindow with this approach, though uncommenting and therefore reintroducing the flag causes our lovely ipcRenderer error again. As far as I can tell, the sandbox does actually get applied to the window though the fact that `sandbox: true` still does things despite the docs saying otherwise leaves me a bit uncomfortable. Fixes https://github.com/vector-im/riot-web/issues/13719 --- src/electron-main.js | 13 ++++++++++++- 1 file changed, 12 insertions(+), 1 deletion(-) diff --git a/src/electron-main.js b/src/electron-main.js index 8843f68..b4ceb0a 100644 --- a/src/electron-main.js +++ b/src/electron-main.js @@ -615,6 +615,17 @@ protocol.registerSchemesAsPrivileged([{ }, }]); +// Turn the sandbox on for *all* windows we might generate. Doing this means we don't +// have to specify a `sandbox: true` to each BrowserWindow. +// +// This also fixes an issue with window.open where if we only specified the sandbox +// on the main window we'd run into cryptic "ipc_renderer be broke" errors. Turns out +// it's trying to jump the sandbox and make some calls into electron, which it can't +// do when half of it is sandboxed. By turning on the sandbox for everything, the new +// window (no matter how temporary it may be) is also sandboxed, allowing for a clean +// transition into the user's browser. +app.enableSandbox(); + app.on('ready', async () => { try { await setupGlobals(); @@ -725,7 +736,7 @@ app.on('ready', async () => { webPreferences: { preload: preloadScript, nodeIntegration: false, - sandbox: true, + //sandbox: true, // We enable sandboxing from app.enableSandbox() above enableRemoteModule: false, // We don't use this: it's useful for the preload script to // share a context with the main page so we can give select