feature: Hot security tips (#11810)

* 🔧 Add security issue detection (and logs)

* 🔧 Check for it on load

* 👷 Add some tests

* 👷 Make the linter happy

* 🔧 Allow them to be enabled by force

* 📝 Make message slightly prettier

* 🔧 Fix a typo in the code comment

* 🔧 Classic mistake

* 🚀 Optimize things a bit more

* 👷 Add tests, fix tests

* 📝 Document things

* 🔧 Make linter happy

* 🔧 One more piece of cleanup

docs/tutorial/security.md

@@ -54,9 +54,20 @@ Node.js integration enabled. Instead, use only local files (packaged together
 with your application) to execute Node.js code. To display remote content, use
 the [`webview`][web-view] tag and make sure to disable the `nodeIntegration`.
 
-#### Checklist: Security Recommendations
+## Electron Security Warnings
 
-This is not bulletproof, but at the least, you should attempt the following:
+From Electron 2.0 on, developers will see warnings and recommendations printed
+to the developer console. They only show op when the binary's name is Electron,
+indicating that a developer is currently looking at the console.
+
+You can force-enable or force-disable these warnings by setting
+`ELECTRON_ENABLE_SECURITY_WARNINGS` or `ELECTRON_DISABLE_SECURITY_WARNINGS` on
+either `process.env` or the `window` object.
+
+## Checklist: Security Recommendations
+
+This is not bulletproof, but at the least, you should follow these steps to
+improve the security of your application.
 
 1) [Only load secure content](#only-load-secure-content)
 2) [Disable the Node.js integration in all renderers that display remote content](#disable-node.js-integration-for-remote-content)

filenames.gypi

@@ -63,6 +63,7 @@
       'lib/renderer/init.js',
       'lib/renderer/inspector.js',
       'lib/renderer/override.js',
+      'lib/renderer/security-warnings.js',
       'lib/renderer/window-setup.js',
       'lib/renderer/web-view/guest-view-internal.js',
       'lib/renderer/web-view/web-view.js',

lib/renderer/init.js

@@ -28,6 +28,18 @@ v8Util.setHiddenValue(global, 'ipc', new events.EventEmitter())
 // Use electron module after everything is ready.
 const electron = require('electron')
 
+const {
+  warnAboutNodeWithRemoteContent,
+  warnAboutDisabledWebSecurity,
+  warnAboutInsecureContentAllowed,
+  warnAboutExperimentalFeatures,
+  warnAboutBlinkFeatures,
+  warnAboutInsecureResources,
+  warnAboutInsecureCSP,
+  warnAboutAllowedPopups,
+  shouldLogSecurityWarnings
+} = require('./security-warnings')
+
 // Call webFrame method.
 electron.ipcRenderer.on('ELECTRON_INTERNAL_RENDERER_WEB_FRAME_METHOD', (event, method, args) => {
   electron.webFrame[method](...args)
@@ -148,17 +160,6 @@ if (nodeIntegration === 'true') {
     }
   }
 
-  if (window.location.protocol === 'https:' ||
-      window.location.protocol === 'http:' ||
-      window.location.protocol === 'ftp:') {
-    let warning = 'This renderer process has Node.js integration enabled '
-    warning += 'and attempted to load remote content. This exposes users of this app to severe '
-    warning += 'security risks.\n'
-    warning += 'For more information and help, consult https://electronjs.org/docs/tutorial/security'
-
-    console.warn('%cElectron Security Warning', 'font-weight: bold;', warning)
-  }
-
   // Redirect window.onerror to uncaughtException.
   window.onerror = function (message, filename, lineno, colno, error) {
     if (global.process.listeners('uncaughtException').length > 0) {
@@ -188,3 +189,22 @@ for (const preloadScript of preloadScripts) {
     console.error(error.stack || error.message)
   }
 }
+
+// Warn about security issues
+window.addEventListener('load', function loadHandler () {
+  if (shouldLogSecurityWarnings()) {
+    if (nodeIntegration === 'true') {
+      warnAboutNodeWithRemoteContent()
+    }
+
+    warnAboutDisabledWebSecurity()
+    warnAboutInsecureResources()
+    warnAboutInsecureContentAllowed()
+    warnAboutExperimentalFeatures()
+    warnAboutBlinkFeatures()
+    warnAboutInsecureCSP()
+    warnAboutAllowedPopups()
+  }
+
+  window.removeEventListener('load', loadHandler)
+})

lib/renderer/security-warnings.js

@@ -0,0 +1,277 @@
+let shouldLog = null
+
+/**
+ * This method checks if a security message should be logged.
+ * It does so by determining whether we're running as Electron,
+ * which indicates that a developer is currently looking at the
+ * app.
+ *
+ * @returns {boolean} - Should we log?
+ */
+const shouldLogSecurityWarnings = function () {
+  if (shouldLog !== null) {
+    return shouldLog
+  }
+
+  const { platform, execPath, env } = process
+
+  switch (platform) {
+    case 'darwin':
+      shouldLog = execPath.endsWith('MacOS/Electron') ||
+                  execPath.includes('Electron.app/Contents/Frameworks/')
+      break
+    case 'freebsd':
+    case 'linux':
+      shouldLog = execPath.endsWith('/electron')
+      break
+    case 'win32':
+      shouldLog = execPath.endsWith('\\electron.exe')
+      break
+    default:
+      shouldLog = false
+  }
+
+  if ((env && env.ELECTRON_DISABLE_SECURITY_WARNINGS) ||
+      (window && window.ELECTRON_DISABLE_SECURITY_WARNINGS)) {
+    shouldLog = false
+  }
+
+  if ((env && env.ELECTRON_ENABLE_SECURITY_WARNINGS) ||
+      (window && window.ELECTRON_ENABLE_SECURITY_WARNINGS)) {
+    shouldLog = true
+  }
+
+  return shouldLog
+}
+
+/**
+ * Check's if the current window is remote.
+ *
+ * @returns {boolean} - Is this a remote protocol?
+ */
+const getIsRemoteProtocol = function () {
+  if (window && window.location && window.location.protocol) {
+    return /^(http|ftp)s?/gi.test(window.location.protocol)
+  }
+}
+
+/**
+ * Tries to determine whether a CSP without `unsafe-eval` is set.
+ *
+ * @returns {boolean} Is a CSP with `unsafe-eval` set?
+ */
+const isUnsafeEvalEnabled = function () {
+  try {
+    //eslint-disable-next-line
+    new Function('');
+    return true
+  } catch (error) {
+    return false
+  }
+}
+
+/**
+ * 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.getCurrentWindow().webContents.getWebPreferences()
+    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
+   * message about all resources loaded over HTTP or FTP.
+   */
+  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
+    }
+
+    let 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()) {
+      let warning = 'This renderer process has Node.js integration enabled ' +
+                    'and attempted to load remote content. 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
+
+    let 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: () => {
+    if (isUnsafeEvalEnabled()) {
+      let 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
+
+    let 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 &&
+        !webPreferences.experimentalCanvasFeatures)) {
+      return
+    }
+
+    let 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 blinkFeatures
+   *
+   * Logs a warning message about blinkFeatures
+   */
+  warnAboutBlinkFeatures: () => {
+    const webPreferences = getWebPreferences()
+    if (!webPreferences || !webPreferences.blinkFeatures ||
+        (webPreferences.blinkFeatures.length && webPreferences.blinkFeatures.length === 0)) {
+      return
+    }
+
+    let warning = 'This renderer process has additional "blinkFeatures" ' +
+                  '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 (blinkFeatures)',
+      '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
+      }
+
+      let 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
+}

script/test.py

@@ -62,6 +62,7 @@ def main():
   try:
     if args.use_instrumented_asar:
       install_instrumented_asar_file(resources_path)
+    os.environ["ELECTRON_DISABLE_SECURITY_WARNINGS"] = "1"
     subprocess.check_call([electron, 'spec'] + sys.argv[1:])
   except subprocess.CalledProcessError as e:
     returncode = e.returncode

spec/fixtures/pages/base-page-security.html

@@ -0,0 +1,9 @@
+<html>
+  <head>
+    <script type="text/javascript">
+      window.ELECTRON_ENABLE_SECURITY_WARNINGS = true
+    </script>
+  </head>
+<body>
+</body>
+</html>

spec/fixtures/pages/insecure-resources.html

@@ -0,0 +1,10 @@
+<html>
+  <head>
+    <link rel="stylesheet" href="http://127.0.0.1:8881/save_page/test.css">
+    <script type="text/javascript">
+      window.ELECTRON_ENABLE_SECURITY_WARNINGS = true
+    </script>
+  </head>
+<body>
+</body>
+</html>

spec/fixtures/pages/webview-allowpopups.html

@@ -0,0 +1,10 @@
+<html>
+  <head>
+    <script type="text/javascript">
+      window.ELECTRON_ENABLE_SECURITY_WARNINGS = true
+    </script>
+  </head>
+  <body>
+    <webview allowpopups></webview>
+  </body>
+</html>

spec/security-warnings-spec.js

@@ -0,0 +1,185 @@
+const assert = require('assert')
+const http = require('http')
+const fs = require('fs')
+const path = require('path')
+const url = require('url')
+
+const {remote} = require('electron')
+const {BrowserWindow} = remote
+
+const {closeWindow} = require('./window-helpers')
+
+describe('security warnings', () => {
+  let server
+  let w = null
+  let useCsp = true
+
+  before(() => {
+    // Create HTTP Server
+    server = http.createServer((request, response) => {
+      const uri = url.parse(request.url).pathname
+      let filename = path.join(__dirname, './fixtures/pages', uri)
+
+      fs.stat(filename, (error, stats) => {
+        if (error) {
+          response.writeHead(404, { 'Content-Type': 'text/plain' })
+          response.end()
+          return
+        }
+
+        if (stats.isDirectory()) {
+          filename += '/index.html'
+        }
+
+        fs.readFile(filename, 'binary', (err, file) => {
+          if (err) {
+            response.writeHead(404, { 'Content-Type': 'text/plain' })
+            response.end()
+            return
+          }
+
+          const cspHeaders = { 'Content-Security-Policy': `script-src 'self' 'unsafe-inline'` }
+          response.writeHead(200, useCsp ? cspHeaders : undefined)
+          response.write(file, 'binary')
+          response.end()
+        })
+      })
+    }).listen(8881)
+  })
+
+  after(() => {
+    // Close server
+    server.close()
+    server = null
+  })
+
+  afterEach(() => {
+    closeWindow(w).then(() => { w = null })
+
+    useCsp = true
+  })
+
+  it('should warn about Node.js integration with remote content', (done) => {
+    w = new BrowserWindow({ show: false })
+    w.webContents.on('console-message', (e, level, message) => {
+      assert(message.includes('Node.js Integration with Remote Content'))
+      done()
+    })
+
+    w.loadURL(`http://127.0.0.1:8881/base-page-security.html`)
+  })
+
+  it('should warn about disabled webSecurity', (done) => {
+    w = new BrowserWindow({
+      show: false,
+      webPreferences: {
+        webSecurity: false,
+        nodeIntegration: false
+      }
+    })
+    w.webContents.on('console-message', (e, level, message) => {
+      assert(message.includes('Disabled webSecurity'))
+      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,
+      webPreferences: {
+        nodeIntegration: false
+      }
+    })
+
+    w.webContents.on('console-message', (e, level, message) => {
+      assert(message.includes('Insecure Content-Security-Policy'))
+      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,
+      webPreferences: {
+        allowRunningInsecureContent: true,
+        nodeIntegration: false
+      }
+    })
+    w.webContents.on('console-message', (e, level, message) => {
+      assert(message.includes('allowRunningInsecureContent'))
+      done()
+    })
+
+    w.loadURL(`http://127.0.0.1:8881/base-page-security.html`)
+  })
+
+  it('should warn about experimentalFeatures', (done) => {
+    w = new BrowserWindow({
+      show: false,
+      webPreferences: {
+        experimentalFeatures: true,
+        nodeIntegration: false
+      }
+    })
+    w.webContents.on('console-message', (e, level, message) => {
+      assert(message.includes('experimentalFeatures'))
+      done()
+    })
+
+    w.loadURL(`http://127.0.0.1:8881/base-page-security.html`)
+  })
+
+  it('should warn about blinkFeatures', (done) => {
+    w = new BrowserWindow({
+      show: false,
+      webPreferences: {
+        blinkFeatures: ['my-cool-feature'],
+        nodeIntegration: false
+      }
+    })
+    w.webContents.on('console-message', (e, level, message) => {
+      assert(message.includes('blinkFeatures'))
+      done()
+    })
+
+    w.loadURL(`http://127.0.0.1:8881/base-page-security.html`)
+  })
+
+  it('should warn about allowpopups', (done) => {
+    w = new BrowserWindow({
+      show: false,
+      webPreferences: {
+        nodeIntegration: false
+      }
+    })
+    w.webContents.on('console-message', (e, level, message) => {
+      console.log(message)
+      assert(message.includes('allowpopups'))
+      done()
+    })
+
+    w.loadURL(`http://127.0.0.1:8881/webview-allowpopups.html`)
+  })
+
+  it('should warn about insecure resources', (done) => {
+    w = new BrowserWindow({
+      show: true,
+      webPreferences: {
+        nodeIntegration: false
+      }
+    })
+    w.webContents.on('console-message', (e, level, message) => {
+      console.log(message)
+      assert(message.includes('Insecure Resources'))
+      done()
+    })
+
+    w.loadURL(`http://127.0.0.1:8881/insecure-resources.html`)
+    w.webContents.openDevTools()
+  })
+})

spec/static/main.js

@@ -29,6 +29,9 @@ app.commandLine.appendSwitch('js-flags', '--expose_gc')
 app.commandLine.appendSwitch('ignore-certificate-errors')
 app.commandLine.appendSwitch('disable-renderer-backgrounding')
 
+// Disable security warnings (the security warnings test will enable them)
+process.env.ELECTRON_DISABLE_SECURITY_WARNINGS = true
+
 // Accessing stdout in the main process will result in the process.stdout
 // throwing UnknownSystemError in renderer process sometimes. This line makes
 // sure we can reproduce it in renderer process.