refactor: use IPC helpers in guest-window-manager / window-setup

electron · May 9, 2019 · 092f727 · 092f727
1 parent 2f926bf
commit 092f727
Show file tree

Hide file tree

Showing 3 changed files with 59 additions and 75 deletions.
diff --git a/filenames.auto.gni b/filenames.auto.gni
@@ -155,7 +155,9 @@ auto_filenames = {
 
   isolated_browserify_deps = [
     "lib/common/electron-binding-setup.ts",
+    "lib/common/error-utils.js",
     "lib/isolated_renderer/init.js",
+    "lib/renderer/ipc-renderer-internal-utils.ts",
     "lib/renderer/ipc-renderer-internal.ts",
     "lib/renderer/web-view/web-view-element.ts",
     "lib/renderer/window-setup.ts",

diff --git a/lib/browser/guest-window-manager.js b/lib/browser/guest-window-manager.js
@@ -3,6 +3,7 @@
 const { BrowserWindow, webContents } = require('electron')
 const { isSameOrigin } = process.electronBinding('v8_util')
 const { ipcMainInternal } = require('@electron/internal/browser/ipc-main-internal')
+const ipcMainUtils = require('@electron/internal/browser/ipc-main-internal-utils')
 const parseFeaturesString = require('@electron/internal/common/parse-features-string')
 
 const hasProp = {}.hasOwnProperty
@@ -153,25 +154,12 @@ const getGuestWindow = function (guestContents) {
       guestWindow = BrowserWindow.fromWebContents(hostContents)
     }
   }
+  if (!guestWindow) {
+    throw new Error('guestWindow failed')
+  }
   return guestWindow
 }
 
-// Checks whether |sender| can access the |target|:
-// 1. Check whether |sender| is the parent of |target|.
-// 2. Check whether |sender| has node integration, if so it is allowed to
-//    do anything it wants.
-// 3. Check whether the origins match.
-//
-// However it allows a child window without node integration but with same
-// origin to do anything it wants, when its opener window has node integration.
-// The W3C does not have anything on this, but from my understanding of the
-// security model of |window.opener|, this should be fine.
-const canAccessWindow = function (sender, target) {
-  return (target.getLastWebPreferences().openerId === sender.id) ||
-         (sender.getLastWebPreferences().nodeIntegration === true) ||
-         isSameOrigin(sender.getURL(), target.getURL())
-}
-
 // Routed window.open messages with raw options
 ipcMainInternal.on('ELECTRON_GUEST_WINDOW_MANAGER_WINDOW_OPEN', (event, url, frameName, features) => {
   if (url == null || url === '') url = 'about:blank'
@@ -260,53 +248,54 @@ ipcMainInternal.on('ELECTRON_GUEST_WINDOW_MANAGER_INTERNAL_WINDOW_OPEN', functio
   }
 })
 
-ipcMainInternal.on('ELECTRON_GUEST_WINDOW_MANAGER_WINDOW_CLOSE', function (event, guestId) {
-  const guestContents = webContents.fromId(guestId)
-  if (guestContents == null) return
+const isChildWindow = function (parent, child) {
+  return child.getLastWebPreferences().openerId === parent.id
+}
 
-  if (!canAccessWindow(event.sender, guestContents)) {
-    console.error(`Blocked ${event.sender.getURL()} from closing its opener.`)
-    return
-  }
+const isScriptableWindow = function (sender, target) {
+  return (isChildWindow(sender, target) || isChildWindow(target, sender)) && isSameOrigin(sender.getURL(), target.getURL())
+}
 
-  const guestWindow = getGuestWindow(guestContents)
-  if (guestWindow != null) guestWindow.destroy()
-})
+const canAccessWindow = function (sender, target) {
+  return isScriptableWindow(sender, target) || (sender.getLastWebPreferences().nodeIntegration === true)
+}
+
+const handleMessage = function (channel, handler) {
+  ipcMainUtils.handle(channel, (event, guestId, ...args) => {
+    const guestContents = webContents.fromId(guestId)
+    if (!guestContents) {
+      throw new Error(`Invalid guestId: ${guestId}`)
+    }
+
+    return handler(event, guestContents, ...args)
+  })
+}
 
 const windowMethods = new Set([
+  'close',
   'focus',
   'blur'
 ])
 
-ipcMainInternal.on('ELECTRON_GUEST_WINDOW_MANAGER_WINDOW_METHOD', function (event, guestId, method, ...args) {
-  const guestContents = webContents.fromId(guestId)
-  if (guestContents == null) {
-    event.returnValue = null
-    return
+handleMessage('ELECTRON_GUEST_WINDOW_MANAGER_WINDOW_METHOD', (event, guestContents, method, ...args) => {
+  if (!isChildWindow(event.sender, guestContents)) {
+    console.error(`Blocked ${event.sender.getURL()} from calling ${method}`)
+    throw new Error(`Access denied to guestId: ${guestContents.id}`)
   }
 
-  if (!canAccessWindow(event.sender, guestContents) || !windowMethods.has(method)) {
-    console.error(`Blocked ${event.sender.getURL()} from calling ${method} on its opener.`)
-    event.returnValue = null
-    return
+  if (!windowMethods.has(method)) {
+    console.error(`Blocked ${event.sender.getURL()} from calling ${method}`)
+    throw new Error(`Invalid method: ${method}`)
   }
 
-  const guestWindow = getGuestWindow(guestContents)
-  if (guestWindow != null) {
-    event.returnValue = guestWindow[method](...args)
-  } else {
-    event.returnValue = null
-  }
+  return getGuestWindow(guestContents)[method](...args)
 })
 
-ipcMainInternal.on('ELECTRON_GUEST_WINDOW_MANAGER_WINDOW_POSTMESSAGE', function (event, guestId, message, targetOrigin, sourceOrigin) {
+handleMessage('ELECTRON_GUEST_WINDOW_MANAGER_WINDOW_POSTMESSAGE', (event, guestContents, message, targetOrigin, sourceOrigin) => {
   if (targetOrigin == null) {
     targetOrigin = '*'
   }
 
-  const guestContents = webContents.fromId(guestId)
-  if (guestContents == null) return
-
   // The W3C does not seem to have word on how postMessage should work when the
   // origins do not match, so we do not do |canAccessWindow| check here since
   // postMessage across origins is useful and not harmful.
@@ -317,37 +306,29 @@ ipcMainInternal.on('ELECTRON_GUEST_WINDOW_MANAGER_WINDOW_POSTMESSAGE', function
 })
 
 const webContentsMethods = new Set([
-  'print',
-  'executeJavaScript'
+  'getURL',
+  'print'
 ])
 
-ipcMainInternal.on('ELECTRON_GUEST_WINDOW_MANAGER_WEB_CONTENTS_METHOD', function (event, guestId, method, ...args) {
-  const guestContents = webContents.fromId(guestId)
-  if (guestContents == null) return
+const webContentsMethodsIgnoreResult = new Set([
+  'loadURL',
+  'executeJavaScript'
+])
 
-  if (canAccessWindow(event.sender, guestContents) && webContentsMethods.has(method)) {
-    guestContents[method](...args)
-  } else {
-    console.error(`Blocked ${event.sender.getURL()} from calling ${method} on its opener.`)
+handleMessage('ELECTRON_GUEST_WINDOW_MANAGER_WEB_CONTENTS_METHOD', (event, guestContents, method, ...args) => {
+  if (!canAccessWindow(event.sender, guestContents)) {
+    console.error(`Blocked ${event.sender.getURL()} from calling ${method}`)
+    throw new Error(`Access denied to guestId: ${guestContents.id}`)
   }
-})
 
-const webContentsSyncMethods = new Set([
-  'getURL',
-  'loadURL'
-])
-
-ipcMainInternal.on('ELECTRON_GUEST_WINDOW_MANAGER_WEB_CONTENTS_METHOD_SYNC', function (event, guestId, method, ...args) {
-  const guestContents = webContents.fromId(guestId)
-  if (guestContents == null) {
-    event.returnValue = null
-    return
+  if (!webContentsMethods.has(method) && !webContentsMethodsIgnoreResult.has(method)) {
+    console.error(`Blocked ${event.sender.getURL()} from calling ${method}`)
+    throw new Error(`Invalid method: ${method}`)
   }
 
-  if (canAccessWindow(event.sender, guestContents) && webContentsSyncMethods.has(method)) {
-    event.returnValue = guestContents[method](...args)
+  if (webContentsMethodsIgnoreResult.has(method)) {
+    guestContents[method](...args)
   } else {
-    console.error(`Blocked ${event.sender.getURL()} from calling ${method} on its opener.`)
-    event.returnValue = null
+    return guestContents[method](...args)
   }
 })
diff --git a/lib/renderer/window-setup.ts b/lib/renderer/window-setup.ts
@@ -1,4 +1,5 @@
 import { ipcRendererInternal } from '@electron/internal/renderer/ipc-renderer-internal'
+import * as ipcRendererUtils from '@electron/internal/renderer/ipc-renderer-internal-utils'
 
 // This file implements the following APIs:
 // - window.history.back()
@@ -111,7 +112,7 @@ class LocationProxy {
   }
 
   private _invokeWebContentsMethodSync (method: string, ...args: any[]) {
-    return ipcRendererInternal.sendSync('ELECTRON_GUEST_WINDOW_MANAGER_WEB_CONTENTS_METHOD_SYNC', this.guestId, method, ...args)
+    return ipcRendererUtils.invokeSync('ELECTRON_GUEST_WINDOW_MANAGER_WEB_CONTENTS_METHOD', this.guestId, method, ...args)
   }
 }
 
@@ -143,7 +144,7 @@ class BrowserWindowProxy {
   }
 
   public close () {
-    ipcRendererInternal.send('ELECTRON_GUEST_WINDOW_MANAGER_WINDOW_CLOSE', this.guestId)
+    this._invokeWindowMethod('close')
   }
 
   public focus () {
@@ -159,23 +160,23 @@ class BrowserWindowProxy {
   }
 
   public postMessage (message: any, targetOrigin: any) {
-    ipcRendererInternal.send('ELECTRON_GUEST_WINDOW_MANAGER_WINDOW_POSTMESSAGE', this.guestId, message, toString(targetOrigin), window.location.origin)
+    ipcRendererUtils.invoke('ELECTRON_GUEST_WINDOW_MANAGER_WINDOW_POSTMESSAGE', this.guestId, message, toString(targetOrigin), window.location.origin)
   }
 
   public eval (code: string) {
     this._invokeWebContentsMethod('executeJavaScript', code)
   }
 
   private _invokeWindowMethod (method: string, ...args: any[]) {
-    return ipcRendererInternal.send('ELECTRON_GUEST_WINDOW_MANAGER_WINDOW_METHOD', this.guestId, method, ...args)
+    return ipcRendererUtils.invoke('ELECTRON_GUEST_WINDOW_MANAGER_WINDOW_METHOD', this.guestId, method, ...args)
   }
 
   private _invokeWebContentsMethod (method: string, ...args: any[]) {
-    return ipcRendererInternal.send('ELECTRON_GUEST_WINDOW_MANAGER_WEB_CONTENTS_METHOD', this.guestId, method, ...args)
+    return ipcRendererUtils.invoke('ELECTRON_GUEST_WINDOW_MANAGER_WEB_CONTENTS_METHOD', this.guestId, method, ...args)
   }
 
   private _invokeWebContentsMethodSync (method: string, ...args: any[]) {
-    return ipcRendererInternal.sendSync('ELECTRON_GUEST_WINDOW_MANAGER_WEB_CONTENTS_METHOD_SYNC', this.guestId, method, ...args)
+    return ipcRendererUtils.invokeSync('ELECTRON_GUEST_WINDOW_MANAGER_WEB_CONTENTS_METHOD', this.guestId, method, ...args)
   }
 }