sec: deprecate some webPreference defaults to be secure-by-default (#14284)

* feat: deprecate default value of nodeIntegration

* Use DeprecationStatus::Stable as the default instead of shadowing

* change wording of deprecations

* chore: also deprecate kWebviewTag and kContextIsolation

* chore: do as we preach, lets be secure-by-default in the default app

atom/browser/web_contents_preferences.cc

@@ -8,6 +8,7 @@
 #include <string>
 #include <vector>
 
+#include "atom/browser/api/atom_api_web_contents.h"
 #include "atom/browser/native_window.h"
 #include "atom/browser/web_view_manager.h"
 #include "atom/common/native_mate_converters/value_converter.h"
@@ -99,12 +100,14 @@ WebContentsPreferences::WebContentsPreferences(
   // Set WebPreferences defaults onto the JS object
   SetDefaultBoolIfUndefined(options::kPlugins, false);
   SetDefaultBoolIfUndefined(options::kExperimentalFeatures, false);
-  bool node = SetDefaultBoolIfUndefined(options::kNodeIntegration, true);
+  bool node = SetDefaultBoolIfUndefined(options::kNodeIntegration, true,
+                                        Status::Deprecated);
   SetDefaultBoolIfUndefined(options::kNodeIntegrationInWorker, false);
-  SetDefaultBoolIfUndefined(options::kWebviewTag, node);
+  SetDefaultBoolIfUndefined(options::kWebviewTag, node, Status::Deprecated);
   SetDefaultBoolIfUndefined(options::kSandbox, false);
   SetDefaultBoolIfUndefined(options::kNativeWindowOpen, false);
-  SetDefaultBoolIfUndefined(options::kContextIsolation, false);
+  SetDefaultBoolIfUndefined(options::kContextIsolation, false,
+                            Status::Deprecated);
   SetDefaultBoolIfUndefined("javascript", true);
   SetDefaultBoolIfUndefined("images", true);
   SetDefaultBoolIfUndefined("textAreasAreResizable", true);
@@ -134,15 +137,24 @@ WebContentsPreferences::~WebContentsPreferences() {
 
 bool WebContentsPreferences::SetDefaultBoolIfUndefined(
     const base::StringPiece& key,
-    bool val) {
+    bool val,
+    Status status) {
   auto* current_value =
       preference_.FindKeyOfType(key, base::Value::Type::BOOLEAN);
   if (current_value) {
     return current_value->GetBool();
   } else {
     preference_.SetKey(key, base::Value(val));
-    return val;
+
+    if (status == Status::Deprecated && web_contents_) {
+      auto internal_contents = atom::api::WebContents::CreateFrom(
+          v8::Isolate::GetCurrent(), web_contents_);
+      internal_contents->Emit("-deprecated-default",
+                              std::string("webPreferences.") + key.data(),
+                              /* oldDefault */ val, /* newDefault */ !val);
     }
+  }
+  return val;
 }
 
 bool WebContentsPreferences::IsEnabled(const base::StringPiece& name,

atom/browser/web_contents_preferences.h

@@ -66,11 +66,15 @@ class WebContentsPreferences
   friend class content::WebContentsUserData<WebContentsPreferences>;
   friend class AtomBrowserClient;
 
+  enum class Status { Deprecated, Stable };
+
   // Get WebContents according to process ID.
   static content::WebContents* GetWebContentsFromProcessID(int process_id);
 
   // Set preference value to given bool if user did not provide value
-  bool SetDefaultBoolIfUndefined(const base::StringPiece& key, bool val);
+  bool SetDefaultBoolIfUndefined(const base::StringPiece& key,
+                                 bool val,
+                                 Status status = Status::Stable);
 
   static std::vector<WebContentsPreferences*> instances_;
 

default_app/default_app.js

@@ -16,7 +16,10 @@ exports.load = (appUrl) => {
       autoHideMenuBar: true,
       backgroundColor: '#FFFFFF',
       webPreferences: {
-        nodeIntegrationInWorker: true
+        nodeIntegration: false,
+        webviewTag: false,
+        contextIsolation: true,
+        preload: path.resolve(__dirname, 'renderer.js')
       },
       useContentSize: true,
       show: false

default_app/index.html

@@ -83,8 +83,6 @@
       </div>
     </div>
   </nav>
-
-  <script src="./renderer.js"></script>
 </body>
 
 </html>

default_app/renderer.js

@@ -4,7 +4,8 @@ const path = require('path')
 const URL = require('url')
 const electronPath = path.relative(process.cwd(), remote.process.execPath)
 
-Array.from(document.querySelectorAll('a[href]')).forEach(link => {
+function initialize () {
+  Array.from(document.querySelectorAll('a[href]')).forEach(link => {
     // safely add `?utm_source=default_app
     let url = URL.parse(link.getAttribute('href'), true)
     url.query = Object.assign(url.query, {utm_source: 'default_app'})
@@ -14,15 +15,15 @@ Array.from(document.querySelectorAll('a[href]')).forEach(link => {
       e.preventDefault()
       shell.openExternal(url)
     })
-})
+  })
 
-document.querySelector('.electron-version').innerText = `Electron v${process.versions.electron}`
+  document.querySelector('.electron-version').innerText = `Electron v${process.versions.electron}`
-document.querySelector('.chrome-version').innerText = `Chromium v${process.versions.chrome}`
+  document.querySelector('.chrome-version').innerText = `Chromium v${process.versions.chrome}`
-document.querySelector('.node-version').innerText = `Node v${process.versions.node}`
+  document.querySelector('.node-version').innerText = `Node v${process.versions.node}`
-document.querySelector('.v8-version').innerText = `v8 v${process.versions.v8}`
+  document.querySelector('.v8-version').innerText = `v8 v${process.versions.v8}`
-document.querySelector('.command-example').innerText = `${electronPath} path-to-app`
+  document.querySelector('.command-example').innerText = `${electronPath} path-to-app`
 
-function getOcticonSvg (name) {
+  function getOcticonSvg (name) {
     const octiconPath = path.resolve(__dirname, 'node_modules', 'octicons', 'build', 'svg', `${name}.svg`)
     if (fs.existsSync(octiconPath)) {
       const content = fs.readFileSync(octiconPath, 'utf8')
@@ -31,9 +32,9 @@ function getOcticonSvg (name) {
       return div
     }
     return null
-}
+  }
 
-function loadSVG (element) {
+  function loadSVG (element) {
     for (const cssClass of element.classList) {
       if (cssClass.startsWith('octicon-')) {
         const icon = getOcticonSvg(cssClass.substr(8))
@@ -45,8 +46,18 @@ function loadSVG (element) {
         }
       }
     }
+  }
+
+  for (const element of document.querySelectorAll('.octicon')) {
+    loadSVG(element)
+  }
 }
 
-for (const element of document.querySelectorAll('.octicon')) {
+function onReadyStateChange () {
-  loadSVG(element)
+  if (document.readyState === 'complete') {
+    document.removeEventListener('readystatechange', onReadyStateChange)
+    initialize()
+  }
 }
+
+document.addEventListener('readystatechange', onReadyStateChange)

docs/api/breaking-changes.md

@@ -34,6 +34,18 @@ app.releaseSingleInstance()
 app.releaseSingleInstanceLock()
 ```
 
+## `new BrowserWindow({ webPreferences: { ... }})`
+
+```js
+// Deprecated defaults
+const webPreferences = {}
+new BrowserWindow({ webPreferences })
+
+// webPreferences.contextIsolation - Default was false, will be true
+// webPreferences.nodeIntegration - Default was true, will be false
+// webPreferences.webviewTag - Default was true, will be false
+```
+
 
 # Breaking API Changes (3.0)
 

lib/browser/api/web-contents.js

@@ -293,6 +293,10 @@ WebContents.prototype._init = function () {
     ipcMain.emit(channel, event, ...args)
   })
 
+  this.on('-deprecated-default', function (event, key, oldDefault, newDefault) {
+    deprecate.warnDefault(key, oldDefault, newDefault)
+  })
+
   // Handle context menu action request from pepper plugin.
   this.on('pepper-context-menu', function (event, params, callback) {
     // Access Menu via electron.Menu to prevent circular require.

lib/common/api/deprecate.js

@@ -31,6 +31,12 @@ deprecate.warn = (oldName, newName) => {
   return deprecate.log(`'${oldName}' is deprecated. Use '${newName}' instead.`)
 }
 
+deprecate.warnDefault = (propName, oldDefault, newDefault) => {
+  return deprecate.log(`The default value of '${propName}' is changing from \
+'${oldDefault}' to '${newDefault}' in a future release. If you want to keep \
+the current value, please explicitly declare the property.`)
+}
+
 let deprecationHandler = null
 
 // Print deprecation message.

lib/renderer/security-warnings.js

@@ -61,6 +61,13 @@ const getIsRemoteProtocol = function () {
  * @returns {boolean} Is a CSP with `unsafe-eval` set?
  */
 const isUnsafeEvalEnabled = function () {
+  // FIXME(MarshallOfSound): Although not exactly true, this warning is incorrect
+  // when contextIsolation is enabled
+  // FIXME(MarshallOfSound): Once remote issues have gone away we can remove
+  // the falsey check
+  const prefs = getWebPreferences()
+  if (prefs && prefs.contextIsolation) return false
+
   try {
     //eslint-disable-next-line
     new Function('');