feat: add security warnings to sandboxed renderers (#14869)

Also refactor not to use the remote module.

lib/browser/rpc-server.js

@@ -475,3 +475,11 @@ ipcMain.on('ELECTRON_BROWSER_SANDBOX_LOAD', function (event) {
     }
   }
 })
+
+ipcMain.on('ELECTRON_BROWSER_GET_LAST_WEB_PREFERENCES', function (event) {
+  try {
+    event.returnValue = [null, event.sender.getLastWebPreferences()]
+  } catch (error) {
+    event.returnValue = [errorUtils.serialize(error)]
+  }
+})

lib/renderer/init.js

@@ -27,18 +27,6 @@ v8Util.setHiddenValue(global, 'ipc', new events.EventEmitter())
 // Use electron module after everything is ready.
 const { ipcRenderer } = require('electron')
 
-const {
-  warnAboutNodeWithRemoteContent,
-  warnAboutDisabledWebSecurity,
-  warnAboutInsecureContentAllowed,
-  warnAboutExperimentalFeatures,
-  warnAboutEnableBlinkFeatures,
-  warnAboutInsecureResources,
-  warnAboutInsecureCSP,
-  warnAboutAllowedPopups,
-  shouldLogSecurityWarnings
-} = require('@electron/internal/renderer/security-warnings')
-
 require('@electron/internal/renderer/web-frame-init')()
 
 // Process command line arguments.
@@ -168,23 +156,7 @@ for (const preloadScript of preloadScripts) {
 }
 
 // Warn about security issues
-window.addEventListener('load', function loadHandler () {
+require('@electron/internal/renderer/security-warnings')(nodeIntegration === 'true')
-  if (shouldLogSecurityWarnings()) {
-    if (nodeIntegration === 'true') {
-      warnAboutNodeWithRemoteContent()
-    }
-
-    warnAboutDisabledWebSecurity()
-    warnAboutInsecureResources()
-    warnAboutInsecureContentAllowed()
-    warnAboutExperimentalFeatures()
-    warnAboutEnableBlinkFeatures()
-    warnAboutInsecureCSP()
-    warnAboutAllowedPopups()
-  }
-
-  window.removeEventListener('load', loadHandler)
-})
 
 // Report focus/blur events of webview to browser.
 // Note that while Chromium content APIs have observer for focus/blur, they

lib/renderer/security-warnings.js

@@ -77,204 +77,212 @@ const isUnsafeEvalEnabled = function () {
   })
 }
 
-/**
- * Attempts to get the current webContents. Returns null
- * if that fails
- *
- * @returns {Object|null} webPreferences
- */
-let webPreferences
-const getWebPreferences = function () {
-  try {
-    if (webPreferences) {
-      return webPreferences
-    }
-
-    const { remote } = require('electron')
-    webPreferences = remote.getCurrentWebContents().getLastWebPreferences()
-    return webPreferences
-  } catch (error) {
-    return null
-  }
-}
-
 const moreInformation = `\nFor more information and help, consult
 https://electronjs.org/docs/tutorial/security.\n This warning will not show up
 once the app is packaged.`
 
-module.exports = {
+/**
-  shouldLogSecurityWarnings,
+ * #1 Only load secure content
-
+ *
-  /**
+ * Checks the loaded resources on the current page and logs a
-   * #1 Only load secure content
+ * message about all resources loaded over HTTP or FTP.
-   *
+ */
-   * Checks the loaded resources on the current page and logs a
+const warnAboutInsecureResources = function () {
-   * message about all resources loaded over HTTP or FTP.
+  if (!window || !window.performance || !window.performance.getEntriesByType) {
-   */
+    return
-  warnAboutInsecureResources: () => {
-    if (!window || !window.performance || !window.performance.getEntriesByType) {
-      return
-    }
-
-    const resources = window.performance
-      .getEntriesByType('resource')
-      .filter(({ name }) => /^(http|ftp):/gi.test(name || ''))
-      .map(({ name }) => `- ${name}`)
-      .join('\n')
-
-    if (!resources || resources.length === 0) {
-      return
-    }
-
-    const warning = `This renderer process loads resources using insecure
-    protocols.This exposes users of this app to unnecessary security risks.
-    Consider loading the following resources over HTTPS or FTPS. \n ${resources}
-    \n ${moreInformation}`
-
-    console.warn('%cElectron Security Warning (Insecure Resources)',
-      'font-weight: bold;', warning)
-  },
-
-  /**
-   * #2 on the checklist: Disable the Node.js integration in all renderers that
-   * display remote content
-   *
-   * Logs a warning message about Node integration.
-   */
-  warnAboutNodeWithRemoteContent: () => {
-    if (getIsRemoteProtocol()) {
-      const warning = `This renderer process has Node.js integration enabled
-      and attempted to load remote content from '${window.location}'. This
-      exposes users of this app to severe security risks.\n ${moreInformation}`
-
-      console.warn('%cElectron Security Warning (Node.js Integration with Remote Content)',
-        'font-weight: bold;', warning)
-    }
-  },
-
-  // Currently missing since it has ramifications and is still experimental:
-  //   #3 Enable context isolation in all renderers that display remote content
-  //
-  // Currently missing since we can't easily programmatically check for those cases:
-  //   #4 Use ses.setPermissionRequestHandler() in all sessions that load remote content
-
-  /**
-   * #5 on the checklist: Do not disable websecurity
-   *
-   * Logs a warning message about disabled webSecurity.
-   */
-  warnAboutDisabledWebSecurity: () => {
-    const webPreferences = getWebPreferences()
-    if (!webPreferences || webPreferences.webSecurity !== false) return
-
-    const warning = `This renderer process has "webSecurity" disabled. This
-    exposes users of this app to severe security risks.\n ${moreInformation}`
-
-    console.warn('%cElectron Security Warning (Disabled webSecurity)',
-      'font-weight: bold;', warning)
-  },
-
-  /**
-   * #6 on the checklist: Define a Content-Security-Policy and use restrictive
-   * rules (i.e. script-src 'self')
-   *
-   * #7 on the checklist: Disable eval
-   *
-   * Logs a warning message about unset or insecure CSP
-   */
-  warnAboutInsecureCSP: () => {
-    isUnsafeEvalEnabled().then((enabled) => {
-      if (!enabled) return
-
-      const warning = `This renderer process has either no Content Security
-      Policy set or a policy with "unsafe-eval" enabled. This exposes users of
-      this app to unnecessary security risks.\n ${moreInformation}`
-
-      console.warn('%cElectron Security Warning (Insecure Content-Security-Policy)',
-        'font-weight: bold;', warning)
-    })
-  },
-
-  /**
-   * #8 on the checklist: Do not set allowRunningInsecureContent to true
-   *
-   * Logs a warning message about disabled webSecurity.
-   */
-  warnAboutInsecureContentAllowed: () => {
-    const webPreferences = getWebPreferences()
-    if (!webPreferences || !webPreferences.allowRunningInsecureContent) return
-
-    const warning = `This renderer process has "allowRunningInsecureContent"
-    enabled. This exposes users of this app to severe security risks.\n
-    ${moreInformation}`
-
-    console.warn('%cElectron Security Warning (allowRunningInsecureContent)',
-      'font-weight: bold;', warning)
-  },
-
-  /**
-   * #9 on the checklist: Do not enable experimental features
-   *
-   * Logs a warning message about experimental features.
-   */
-  warnAboutExperimentalFeatures: () => {
-    const webPreferences = getWebPreferences()
-    if (!webPreferences || (!webPreferences.experimentalFeatures)) {
-      return
-    }
-
-    const warning = `This renderer process has "experimentalFeatures" enabled.
-    This exposes users of this app to some security risk. If you do not need
-    this feature, you should disable it.\n ${moreInformation}`
-
-    console.warn('%cElectron Security Warning (experimentalFeatures)',
-      'font-weight: bold;', warning)
-  },
-
-  /**
-   * #10 on the checklist: Do not use enableBlinkFeatures
-   *
-   * Logs a warning message about enableBlinkFeatures
-   */
-  warnAboutEnableBlinkFeatures: () => {
-    const webPreferences = getWebPreferences()
-    if (webPreferences === null ||
-      !webPreferences.hasOwnProperty('enableBlinkFeatures') ||
-      webPreferences.enableBlinkFeatures.length === 0) {
-      return
-    }
-
-    const warning = `This renderer process has additional "enableBlinkFeatures"
-    enabled. This exposes users of this app to some security risk. If you do not
-    need this feature, you should disable it.\n ${moreInformation}`
-
-    console.warn('%cElectron Security Warning (enableBlinkFeatures)',
-      'font-weight: bold;', warning)
-  },
-
-  /**
-   * #11 on the checklist: Do Not Use allowpopups
-   *
-   * Logs a warning message about allowed popups
-   */
-  warnAboutAllowedPopups: () => {
-    if (document && document.querySelectorAll) {
-      const domElements = document.querySelectorAll('[allowpopups]')
-
-      if (!domElements || domElements.length === 0) {
-        return
-      }
-
-      const warning = `A <webview> has "allowpopups" set to true. This exposes
-      users of this app to some security risk, since popups are just
-      BrowserWindows. If you do not need this feature, you should disable it.\n
-      ${moreInformation}`
-
-      console.warn('%cElectron Security Warning (allowpopups)',
-        'font-weight: bold;', warning)
-    }
   }
 
-  // Currently missing since we can't easily programmatically check for it:
+  const resources = window.performance
-  //   #12WebViews: Verify the options and params of all `<webview>` tags
+    .getEntriesByType('resource')
+    .filter(({ name }) => /^(http|ftp):/gi.test(name || ''))
+    .map(({ name }) => `- ${name}`)
+    .join('\n')
+
+  if (!resources || resources.length === 0) {
+    return
+  }
+
+  const warning = `This renderer process loads resources using insecure
+  protocols.This exposes users of this app to unnecessary security risks.
+  Consider loading the following resources over HTTPS or FTPS. \n ${resources}
+  \n ${moreInformation}`
+
+  console.warn('%cElectron Security Warning (Insecure Resources)',
+    'font-weight: bold;', warning)
+}
+
+/**
+ * #2 on the checklist: Disable the Node.js integration in all renderers that
+ * display remote content
+ *
+ * Logs a warning message about Node integration.
+ */
+const warnAboutNodeWithRemoteContent = function (nodeIntegration) {
+  if (!nodeIntegration) return
+
+  if (getIsRemoteProtocol()) {
+    const warning = `This renderer process has Node.js integration enabled
+    and attempted to load remote content from '${window.location}'. This
+    exposes users of this app to severe security risks.\n ${moreInformation}`
+
+    console.warn('%cElectron Security Warning (Node.js Integration with Remote Content)',
+      'font-weight: bold;', warning)
+  }
+}
+
+// Currently missing since it has ramifications and is still experimental:
+//   #3 Enable context isolation in all renderers that display remote content
+//
+// Currently missing since we can't easily programmatically check for those cases:
+//   #4 Use ses.setPermissionRequestHandler() in all sessions that load remote content
+
+/**
+ * #5 on the checklist: Do not disable websecurity
+ *
+ * Logs a warning message about disabled webSecurity.
+ */
+const warnAboutDisabledWebSecurity = function (webPreferences) {
+  if (!webPreferences || webPreferences.webSecurity !== false) return
+
+  const warning = `This renderer process has "webSecurity" disabled. This
+  exposes users of this app to severe security risks.\n ${moreInformation}`
+
+  console.warn('%cElectron Security Warning (Disabled webSecurity)',
+    'font-weight: bold;', warning)
+}
+
+/**
+ * #6 on the checklist: Define a Content-Security-Policy and use restrictive
+ * rules (i.e. script-src 'self')
+ *
+ * #7 on the checklist: Disable eval
+ *
+ * Logs a warning message about unset or insecure CSP
+ */
+const warnAboutInsecureCSP = function () {
+  isUnsafeEvalEnabled().then((enabled) => {
+    if (!enabled) return
+
+    const warning = `This renderer process has either no Content Security
+    Policy set or a policy with "unsafe-eval" enabled. This exposes users of
+    this app to unnecessary security risks.\n ${moreInformation}`
+
+    console.warn('%cElectron Security Warning (Insecure Content-Security-Policy)',
+      'font-weight: bold;', warning)
+  })
+}
+
+/**
+ * #8 on the checklist: Do not set allowRunningInsecureContent to true
+ *
+ * Logs a warning message about disabled webSecurity.
+ */
+const warnAboutInsecureContentAllowed = function (webPreferences) {
+  if (!webPreferences || !webPreferences.allowRunningInsecureContent) return
+
+  const warning = `This renderer process has "allowRunningInsecureContent"
+  enabled. This exposes users of this app to severe security risks.\n
+  ${moreInformation}`
+
+  console.warn('%cElectron Security Warning (allowRunningInsecureContent)',
+    'font-weight: bold;', warning)
+}
+
+/**
+ * #9 on the checklist: Do not enable experimental features
+ *
+ * Logs a warning message about experimental features.
+ */
+const warnAboutExperimentalFeatures = function (webPreferences) {
+  if (!webPreferences || (!webPreferences.experimentalFeatures)) {
+    return
+  }
+
+  const warning = `This renderer process has "experimentalFeatures" enabled.
+  This exposes users of this app to some security risk. If you do not need
+  this feature, you should disable it.\n ${moreInformation}`
+
+  console.warn('%cElectron Security Warning (experimentalFeatures)',
+    'font-weight: bold;', warning)
+}
+
+/**
+ * #10 on the checklist: Do not use enableBlinkFeatures
+ *
+ * Logs a warning message about enableBlinkFeatures
+ */
+const warnAboutEnableBlinkFeatures = function (webPreferences) {
+  if (webPreferences === null ||
+    !webPreferences.hasOwnProperty('enableBlinkFeatures') ||
+    webPreferences.enableBlinkFeatures.length === 0) {
+    return
+  }
+
+  const warning = `This renderer process has additional "enableBlinkFeatures"
+  enabled. This exposes users of this app to some security risk. If you do not
+  need this feature, you should disable it.\n ${moreInformation}`
+
+  console.warn('%cElectron Security Warning (enableBlinkFeatures)',
+    'font-weight: bold;', warning)
+}
+
+/**
+ * #11 on the checklist: Do Not Use allowpopups
+ *
+ * Logs a warning message about allowed popups
+ */
+const warnAboutAllowedPopups = function () {
+  if (document && document.querySelectorAll) {
+    const domElements = document.querySelectorAll('[allowpopups]')
+
+    if (!domElements || domElements.length === 0) {
+      return
+    }
+
+    const warning = `A <webview> has "allowpopups" set to true. This exposes
+    users of this app to some security risk, since popups are just
+    BrowserWindows. If you do not need this feature, you should disable it.\n
+    ${moreInformation}`
+
+    console.warn('%cElectron Security Warning (allowpopups)',
+      'font-weight: bold;', warning)
+  }
+}
+
+// Currently missing since we can't easily programmatically check for it:
+//   #12WebViews: Verify the options and params of all `<webview>` tags
+
+const logSecurityWarnings = function (webPreferences, nodeIntegration) {
+  warnAboutNodeWithRemoteContent(nodeIntegration)
+  warnAboutDisabledWebSecurity(webPreferences)
+  warnAboutInsecureResources()
+  warnAboutInsecureContentAllowed(webPreferences)
+  warnAboutExperimentalFeatures(webPreferences)
+  warnAboutEnableBlinkFeatures(webPreferences)
+  warnAboutInsecureCSP()
+  warnAboutAllowedPopups()
+}
+
+const getWebPreferences = function () {
+  const { ipcRenderer } = require('electron')
+  const errorUtils = require('@electron/internal/common/error-utils')
+
+  const [ error, result ] = ipcRenderer.sendSync('ELECTRON_BROWSER_GET_LAST_WEB_PREFERENCES')
+
+  if (error) {
+    console.warn(`getLastWebPreferences() failed: ${errorUtils.deserialize(error)}`)
+    return null
+  } else {
+    return result
+  }
+}
+
+module.exports = function (nodeIntegration) {
+  const loadHandler = function () {
+    if (shouldLogSecurityWarnings()) {
+      const webPreferences = getWebPreferences()
+      logSecurityWarnings(webPreferences, nodeIntegration)
+    }
+  }
+  window.addEventListener('load', loadHandler, { once: true })
 }

lib/sandboxed_renderer/init.js

@@ -115,3 +115,6 @@ if (preloadSrc) {
 } else if (preloadError) {
   console.error(preloadError.stack)
 }
+
+// Warn about security issues
+require('@electron/internal/renderer/security-warnings')()

spec/security-warnings-spec.js

@@ -84,6 +84,22 @@ describe('security warnings', () => {
     w.loadURL(`http://127.0.0.1:8881/base-page-security.html`)
   })
 
+  it('should warn about disabled webSecurity (sandboxed)', (done) => {
+    w = new BrowserWindow({
+      show: false,
+      webPreferences: {
+        webSecurity: false,
+        sandbox: true
+      }
+    })
+    w.webContents.once('console-message', (e, level, message) => {
+      assert(message.includes('Disabled webSecurity'), message)
+      done()
+    })
+
+    w.loadURL(`http://127.0.0.1:8881/base-page-security.html`)
+  })
+
   it('should warn about insecure Content-Security-Policy', (done) => {
     w = new BrowserWindow({
       show: false,
@@ -101,6 +117,23 @@ describe('security warnings', () => {
     w.loadURL(`http://127.0.0.1:8881/base-page-security.html`)
   })
 
+  it('should warn about insecure Content-Security-Policy (sandboxed)', (done) => {
+    w = new BrowserWindow({
+      show: false,
+      webPreferences: {
+        sandbox: true
+      }
+    })
+
+    w.webContents.once('console-message', (e, level, message) => {
+      assert(message.includes('Insecure Content-Security-Policy'), message)
+      done()
+    })
+
+    useCsp = false
+    w.loadURL(`http://127.0.0.1:8881/base-page-security.html`)
+  })
+
   it('should warn about allowRunningInsecureContent', (done) => {
     w = new BrowserWindow({
       show: false,
@@ -117,6 +150,22 @@ describe('security warnings', () => {
     w.loadURL(`http://127.0.0.1:8881/base-page-security.html`)
   })
 
+  it('should warn about allowRunningInsecureContent (sandboxed)', (done) => {
+    w = new BrowserWindow({
+      show: false,
+      webPreferences: {
+        allowRunningInsecureContent: true,
+        sandbox: true
+      }
+    })
+    w.webContents.once('console-message', (e, level, message) => {
+      assert(message.includes('allowRunningInsecureContent'), message)
+      done()
+    })
+
+    w.loadURL(`http://127.0.0.1:8881/base-page-security.html`)
+  })
+
   it('should warn about experimentalFeatures', (done) => {
     w = new BrowserWindow({
       show: false,
@@ -133,6 +182,22 @@ describe('security warnings', () => {
     w.loadURL(`http://127.0.0.1:8881/base-page-security.html`)
   })
 
+  it('should warn about experimentalFeatures (sandboxed)', (done) => {
+    w = new BrowserWindow({
+      show: false,
+      webPreferences: {
+        experimentalFeatures: true,
+        sandbox: true
+      }
+    })
+    w.webContents.once('console-message', (e, level, message) => {
+      assert(message.includes('experimentalFeatures'), message)
+      done()
+    })
+
+    w.loadURL(`http://127.0.0.1:8881/base-page-security.html`)
+  })
+
   it('should warn about enableBlinkFeatures', (done) => {
     w = new BrowserWindow({
       show: false,
@@ -149,6 +214,22 @@ describe('security warnings', () => {
     w.loadURL(`http://127.0.0.1:8881/base-page-security.html`)
   })
 
+  it('should warn about enableBlinkFeatures (sandboxed)', (done) => {
+    w = new BrowserWindow({
+      show: false,
+      webPreferences: {
+        enableBlinkFeatures: ['my-cool-feature'],
+        sandbox: true
+      }
+    })
+    w.webContents.once('console-message', (e, level, message) => {
+      assert(message.includes('enableBlinkFeatures'), message)
+      done()
+    })
+
+    w.loadURL(`http://127.0.0.1:8881/base-page-security.html`)
+  })
+
   it('should warn about allowpopups', (done) => {
     w = new BrowserWindow({
       show: false,
@@ -164,9 +245,24 @@ describe('security warnings', () => {
     w.loadURL(`http://127.0.0.1:8881/webview-allowpopups.html`)
   })
 
+  it('should warn about allowpopups (sandboxed)', (done) => {
+    w = new BrowserWindow({
+      show: false,
+      webPreferences: {
+        sandbox: true
+      }
+    })
+    w.webContents.once('console-message', (e, level, message) => {
+      assert(message.includes('allowpopups'), message)
+      done()
+    })
+
+    w.loadURL(`http://127.0.0.1:8881/webview-allowpopups.html`)
+  })
+
   it('should warn about insecure resources', (done) => {
     w = new BrowserWindow({
-      show: true,
+      show: false,
       webPreferences: {
         nodeIntegration: false
       }
@@ -179,4 +275,20 @@ describe('security warnings', () => {
     w.loadURL(`http://127.0.0.1:8881/insecure-resources.html`)
     w.webContents.openDevTools()
   })
+
+  it('should warn about insecure resources (sandboxed)', (done) => {
+    w = new BrowserWindow({
+      show: false,
+      webPreferences: {
+        sandbox: true
+      }
+    })
+    w.webContents.once('console-message', (e, level, message) => {
+      assert(message.includes('Insecure Resources'), message)
+      done()
+    })
+
+    w.loadURL(`http://127.0.0.1:8881/insecure-resources.html`)
+    w.webContents.openDevTools()
+  })
 })