From 31c93fec6771675745f0fb40b48310b1652595bc Mon Sep 17 00:00:00 2001 From: Milan Burda Date: Mon, 6 Jan 2020 22:23:03 +0100 Subject: [PATCH] fix: load window-setup in sandboxed renderer (#21416) --- filenames.auto.gni | 1 + lib/browser/guest-window-manager.js | 6 +- lib/browser/rpc-server.js | 4 + lib/renderer/window-setup.ts | 74 ++++++++++--------- lib/sandboxed_renderer/init.js | 24 +++++- .../atom_sandboxed_renderer_client.cc | 6 -- spec-main/chromium-spec.ts | 35 +++++---- 7 files changed, 89 insertions(+), 61 deletions(-) diff --git a/filenames.auto.gni b/filenames.auto.gni index 1eee5335546e..27af484c8c02 100644 --- a/filenames.auto.gni +++ b/filenames.auto.gni @@ -165,6 +165,7 @@ auto_filenames = { "lib/renderer/web-view/web-view-element.ts", "lib/renderer/web-view/web-view-impl.ts", "lib/renderer/web-view/web-view-init.ts", + "lib/renderer/window-setup.ts", "lib/sandboxed_renderer/api/exports/electron.ts", "lib/sandboxed_renderer/api/module-list.ts", "lib/sandboxed_renderer/init.js", diff --git a/lib/browser/guest-window-manager.js b/lib/browser/guest-window-manager.js index 0978ca132501..b0523ecb0d2e 100644 --- a/lib/browser/guest-window-manager.js +++ b/lib/browser/guest-window-manager.js @@ -75,8 +75,10 @@ const mergeBrowserWindowOptions = function (embedder, options) { } } - // Sets correct openerId here to give correct options to 'new-window' event handler - options.webPreferences.openerId = embedder.id + if (!webPreferences.nativeWindowOpen) { + // Sets correct openerId here to give correct options to 'new-window' event handler + options.webPreferences.openerId = embedder.id + } return options } diff --git a/lib/browser/rpc-server.js b/lib/browser/rpc-server.js index cfcbaa09926c..f2514df63e19 100644 --- a/lib/browser/rpc-server.js +++ b/lib/browser/rpc-server.js @@ -112,11 +112,15 @@ ipcMainUtils.handleSync('ELECTRON_BROWSER_SANDBOX_LOAD', async function (event) contentScripts = getContentScripts() } + const webPreferences = event.sender.getLastWebPreferences() || {} + return { contentScripts, preloadScripts: await Promise.all(preloadPaths.map(path => getPreloadScript(path))), isRemoteModuleEnabled: isRemoteModuleEnabled(event.sender), isWebViewTagEnabled: guestViewManager.isWebViewTagEnabled(event.sender), + guestInstanceId: webPreferences.guestInstanceId, + openerId: webPreferences.openerId, process: { arch: process.arch, platform: process.platform, diff --git a/lib/renderer/window-setup.ts b/lib/renderer/window-setup.ts index f8d5b6f3ae2b..9b27e606c9e7 100644 --- a/lib/renderer/window-setup.ts +++ b/lib/renderer/window-setup.ts @@ -177,7 +177,7 @@ class BrowserWindowProxy { export const windowSetup = ( guestInstanceId: number, openerId: number, isHiddenPage: boolean, usesNativeWindowOpen: boolean ) => { - if (guestInstanceId == null) { + if (!process.sandboxed && guestInstanceId == null) { // Override default window.close. window.close = function () { ipcRendererInternal.send('ELECTRON_BROWSER_WINDOW_CLOSE') @@ -197,10 +197,10 @@ export const windowSetup = ( return null } } + } - if (openerId != null) { - window.opener = getOrCreateProxy(openerId) - } + if (openerId != null) { + window.opener = getOrCreateProxy(openerId) } // But we do not support prompt(). @@ -208,43 +208,47 @@ export const windowSetup = ( throw new Error('prompt() is and will not be supported.') } - ipcRendererInternal.on('ELECTRON_GUEST_WINDOW_POSTMESSAGE', function ( - _event, sourceId: number, message: any, sourceOrigin: string - ) { - // Manually dispatch event instead of using postMessage because we also need to - // set event.source. - // - // Why any? We can't construct a MessageEvent and we can't - // use `as MessageEvent` because you're not supposed to override - // data, origin, and source - const event: any = document.createEvent('Event') - event.initEvent('message', false, false) + if (!usesNativeWindowOpen || openerId != null) { + ipcRendererInternal.on('ELECTRON_GUEST_WINDOW_POSTMESSAGE', function ( + _event, sourceId: number, message: any, sourceOrigin: string + ) { + // Manually dispatch event instead of using postMessage because we also need to + // set event.source. + // + // Why any? We can't construct a MessageEvent and we can't + // use `as MessageEvent` because you're not supposed to override + // data, origin, and source + const event: any = document.createEvent('Event') + event.initEvent('message', false, false) - event.data = message - event.origin = sourceOrigin - event.source = getOrCreateProxy(sourceId) + event.data = message + event.origin = sourceOrigin + event.source = getOrCreateProxy(sourceId) - window.dispatchEvent(event as MessageEvent) - }) - - window.history.back = function () { - ipcRendererInternal.send('ELECTRON_NAVIGATION_CONTROLLER_GO_BACK') + window.dispatchEvent(event as MessageEvent) + }) } - window.history.forward = function () { - ipcRendererInternal.send('ELECTRON_NAVIGATION_CONTROLLER_GO_FORWARD') - } + if (!process.sandboxed) { + window.history.back = function () { + ipcRendererInternal.send('ELECTRON_NAVIGATION_CONTROLLER_GO_BACK') + } - window.history.go = function (offset: number) { - ipcRendererInternal.send('ELECTRON_NAVIGATION_CONTROLLER_GO_TO_OFFSET', +offset) - } + window.history.forward = function () { + ipcRendererInternal.send('ELECTRON_NAVIGATION_CONTROLLER_GO_FORWARD') + } - Object.defineProperty(window.history, 'length', { - get: function () { - return ipcRendererInternal.sendSync('ELECTRON_NAVIGATION_CONTROLLER_LENGTH') - }, - set () {} - }) + window.history.go = function (offset: number) { + ipcRendererInternal.send('ELECTRON_NAVIGATION_CONTROLLER_GO_TO_OFFSET', +offset) + } + + Object.defineProperty(window.history, 'length', { + get: function () { + return ipcRendererInternal.sendSync('ELECTRON_NAVIGATION_CONTROLLER_LENGTH') + }, + set () {} + }) + } if (guestInstanceId != null) { // Webview `document.visibilityState` tracks window visibility (and ignores diff --git a/lib/sandboxed_renderer/init.js b/lib/sandboxed_renderer/init.js index 9bdd726992c6..624baac45273 100644 --- a/lib/sandboxed_renderer/init.js +++ b/lib/sandboxed_renderer/init.js @@ -30,7 +30,13 @@ const { ipcRendererInternal } = require('@electron/internal/renderer/ipc-rendere const ipcRendererUtils = require('@electron/internal/renderer/ipc-renderer-internal-utils') const { - contentScripts, preloadScripts, isRemoteModuleEnabled, isWebViewTagEnabled, process: processProps + contentScripts, + preloadScripts, + isRemoteModuleEnabled, + isWebViewTagEnabled, + guestInstanceId, + openerId, + process: processProps } = ipcRendererUtils.invokeSync('ELECTRON_BROWSER_SANDBOX_LOAD') process.isRemoteModuleEnabled = isRemoteModuleEnabled @@ -109,6 +115,11 @@ function preloadRequire (module) { const { hasSwitch } = process.electronBinding('command_line') const contextIsolation = hasSwitch('context-isolation') +const isHiddenPage = hasSwitch('hidden-page') +const usesNativeWindowOpen = true + +// The arguments to be passed to isolated world. +const isolatedWorldArgs = { ipcRendererInternal, guestInstanceId, isHiddenPage, openerId, usesNativeWindowOpen } switch (window.location.protocol) { case 'devtools:': { @@ -127,6 +138,10 @@ switch (window.location.protocol) { break } default: { + // Override default web functions. + const { windowSetup } = require('@electron/internal/renderer/window-setup') + windowSetup(guestInstanceId, openerId, isHiddenPage, usesNativeWindowOpen) + // Inject content scripts. if (!process.electronBinding('features').isExtensionsEnabled()) { require('@electron/internal/renderer/content-scripts-injector')(contentScripts) @@ -134,14 +149,17 @@ switch (window.location.protocol) { } } -const guestInstanceId = binding.guestInstanceId && parseInt(binding.guestInstanceId) - // Load webview tag implementation. if (process.isMainFrame) { const { webViewInit } = require('@electron/internal/renderer/web-view/web-view-init') webViewInit(contextIsolation, isWebViewTagEnabled, guestInstanceId) } +// Pass the arguments to isolatedWorld. +if (contextIsolation) { + v8Util.setHiddenValue(global, 'isolated-world-args', isolatedWorldArgs) +} + // Wrap the script into a function executed in global scope. It won't have // access to the current scope, so we'll expose a few objects as arguments: // diff --git a/shell/renderer/atom_sandboxed_renderer_client.cc b/shell/renderer/atom_sandboxed_renderer_client.cc index 58efed622b09..a781b1710333 100644 --- a/shell/renderer/atom_sandboxed_renderer_client.cc +++ b/shell/renderer/atom_sandboxed_renderer_client.cc @@ -141,12 +141,6 @@ void AtomSandboxedRendererClient::InitializeBindings( process.SetReadOnly("sandboxed", true); process.SetReadOnly("type", "renderer"); process.SetReadOnly("isMainFrame", is_main_frame); - - // Pass in CLI flags needed to setup the renderer - base::CommandLine* command_line = base::CommandLine::ForCurrentProcess(); - if (command_line->HasSwitch(switches::kGuestInstanceID)) - b.Set(options::kGuestInstanceID, - command_line->GetSwitchValueASCII(switches::kGuestInstanceID)); } void AtomSandboxedRendererClient::RenderFrameCreated( diff --git a/spec-main/chromium-spec.ts b/spec-main/chromium-spec.ts index d46cb0c489d3..e7372ed27054 100644 --- a/spec-main/chromium-spec.ts +++ b/spec-main/chromium-spec.ts @@ -766,22 +766,27 @@ describe('chromium features', () => { describe('when opened from main window', () => { for (const { parent, child, nodeIntegration, nativeWindowOpen, openerAccessible } of table) { - const description = `when parent=${s(parent)} opens child=${s(child)} with nodeIntegration=${nodeIntegration} nativeWindowOpen=${nativeWindowOpen}, child should ${openerAccessible ? '' : 'not '}be able to access opener` - it(description, async () => { - const w = new BrowserWindow({ show: false, webPreferences: { nodeIntegration: true, nativeWindowOpen } }) - await w.loadURL(parent) - const childOpenerLocation = await w.webContents.executeJavaScript(`new Promise(resolve => { - window.addEventListener('message', function f(e) { - resolve(e.data) + for (const sandboxPopup of [false, true]) { + const description = `when parent=${s(parent)} opens child=${s(child)} with nodeIntegration=${nodeIntegration} nativeWindowOpen=${nativeWindowOpen} sandboxPopup=${sandboxPopup}, child should ${openerAccessible ? '' : 'not '}be able to access opener` + it(description, async () => { + const w = new BrowserWindow({ show: false, webPreferences: { nodeIntegration: true, nativeWindowOpen } }) + w.webContents.once('new-window', (e, url, frameName, disposition, options) => { + options!.webPreferences!.sandbox = sandboxPopup }) - window.open(${JSON.stringify(child)}, "", "show=no,nodeIntegration=${nodeIntegration ? 'yes' : 'no'}") - })`) - if (openerAccessible) { - expect(childOpenerLocation).to.be.a('string') - } else { - expect(childOpenerLocation).to.be.null() - } - }) + await w.loadURL(parent) + const childOpenerLocation = await w.webContents.executeJavaScript(`new Promise(resolve => { + window.addEventListener('message', function f(e) { + resolve(e.data) + }) + window.open(${JSON.stringify(child)}, "", "show=no,nodeIntegration=${nodeIntegration ? 'yes' : 'no'}") + })`) + if (openerAccessible) { + expect(childOpenerLocation).to.be.a('string') + } else { + expect(childOpenerLocation).to.be.null() + } + }) + } } })