fix: child window with nativeWindowOpen should disable node integration (#15213)

* fix: child window with nativeWindowOpen should disable node integration

* Revert "fix: do not enable node integration in child window if not enabled (#15076)"

This reverts commit 0252d7686ce19c04f711d82f63590ff51bb5efc1.

This patch is not needed anymore since we are force disabling node integration
for child windows.

atom/browser/web_contents_preferences.cc

@@ -405,8 +405,6 @@ void WebContentsPreferences::OverrideWebkitPrefs(
   std::string encoding;
   if (GetAsString(&preference_, "defaultEncoding", &encoding))
     prefs->default_encoding = encoding;
-
-  prefs->node_integration = IsEnabled(options::kNodeIntegration);
 }
 
 }  // namespace atom

atom/renderer/atom_renderer_client.cc

@@ -16,7 +16,6 @@
 #include "atom/renderer/atom_render_frame_observer.h"
 #include "atom/renderer/web_worker_observer.h"
 #include "base/command_line.h"
-#include "content/public/common/web_preferences.h"
 #include "content/public/renderer/render_frame.h"
 #include "native_mate/dictionary.h"
 #include "third_party/blink/public/web/web_document.h"
@@ -82,18 +81,20 @@ void AtomRendererClient::DidCreateScriptContext(
     content::RenderFrame* render_frame) {
   RendererClientBase::DidCreateScriptContext(context, render_frame);
 
-  // Only allow node integration for the main frame, unless it is a devtools
-  // extension page.
-  if (!render_frame->IsMainFrame() && !IsDevToolsExtension(render_frame))
-    return;
-
-  // Don't allow node integration if this is a child window and it does not have
-  // node integration enabled.  Otherwise we would have memory leak in the child
-  // window since we don't clean up node environments.
+  // Only allow node integration for the main frame of the top window, unless it
+  // is a devtools extension page. Allowing child frames or child windows to
+  // have node integration would result in memory leak, since we don't destroy
+  // node environment when script context is destroyed.
+  //
+  // DevTools extensions do not follow this rule because our implementation
+  // requires node integration in iframes to work. And usually DevTools
+  // extensions do not dynamically add/remove iframes.
   //
-  // TODO(zcbenz): We shouldn't allow node integration even for the top frame.
-  if (!render_frame->GetWebkitPreferences().node_integration &&
-      render_frame->GetWebFrame()->Opener())
+  // TODO(zcbenz): Do not create Node environment if node integration is not
+  // enabled.
+  if (!(render_frame->IsMainFrame() &&
+        !render_frame->GetWebFrame()->Opener()) &&
+      !IsDevToolsExtension(render_frame))
     return;
 
   injected_frames_.insert(render_frame);

docs/api/breaking-changes.md

@@ -18,6 +18,9 @@ The following `webPreferences` option default values are deprecated in favor of
 | `nodeIntegration` | `true` | `false` |
 | `webviewTag` | `nodeIntegration` if set else `true` | `false` |
 
+## `nativeWindowOpen`
+
+Child windows opened with the `nativeWindowOpen` option will always have Node.js integration disabled.
 
 # Planned Breaking API Changes (4.0)
 

docs/api/browser-window.md

@@ -351,7 +351,8 @@ It creates a new `BrowserWindow` with native properties as set by the `options`.
       Console tab. **Note:** This option is currently experimental and may
       change or be removed in future Electron releases.
     * `nativeWindowOpen` Boolean (optional) - Whether to use native
-      `window.open()`. Defaults to `false`. **Note:** This option is currently
+      `window.open()`. Defaults to `false`. Child windows will always have node
+      integration disabled. **Note:** This option is currently
       experimental.
     * `webviewTag` Boolean (optional) - Whether to enable the [`<webview>` tag](webview-tag.md).
       Defaults to the value of the `nodeIntegration` option. **Note:** The

patches/common/chromium/.patches

@@ -76,4 +76,3 @@ tts.patch
 color_chooser.patch
 printing.patch
 verbose_generate_breakpad_symbols.patch
-web_preferences.patch

patches/common/chromium/web_preferences.patch

@@ -1,39 +0,0 @@
-From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
-From: zcbenz <[email protected]>
-Date: Thu, 18 Oct 2018 17:08:33 -0700
-Subject: web_preferences.patch
-
-Add a node_integration field to WebPreferences so we can determine whether
-a frame has node integration in renderer process.
-
-This is required by the nativeWindowOpen option, which put multiple main
-frames in one renderer process.
-
-diff --git a/content/public/common/common_param_traits_macros.h b/content/public/common/common_param_traits_macros.h
-index 57f03dc1ed38f0f7f644615dc7eb7a6f9e568760..7c4409e6e9abf4d02e50c31d480c3e3792e9e655 100644
---- a/content/public/common/common_param_traits_macros.h
-+++ b/content/public/common/common_param_traits_macros.h
-@@ -198,6 +198,7 @@ IPC_STRUCT_TRAITS_BEGIN(content::WebPreferences)
-   IPC_STRUCT_TRAITS_MEMBER(animation_policy)
-   IPC_STRUCT_TRAITS_MEMBER(user_gesture_required_for_presentation)
-   IPC_STRUCT_TRAITS_MEMBER(text_track_margin_percentage)
-+  IPC_STRUCT_TRAITS_MEMBER(node_integration)
-   IPC_STRUCT_TRAITS_MEMBER(save_previous_document_resources)
-   IPC_STRUCT_TRAITS_MEMBER(text_autosizing_enabled)
-   IPC_STRUCT_TRAITS_MEMBER(double_tap_to_zoom_enabled)
-diff --git a/content/public/common/web_preferences.h b/content/public/common/web_preferences.h
-index 78cbf5f3db8624ad3a346406da47d0ec1a2b4e53..b33ac2894c7c305a7e303f323db22caf64d30468 100644
---- a/content/public/common/web_preferences.h
-+++ b/content/public/common/web_preferences.h
-@@ -222,6 +222,9 @@ struct CONTENT_EXPORT WebPreferences {
-   // Cues will not be placed in this margin area.
-   float text_track_margin_percentage;
- 
-+  // Electron: Whether the frame has node integration.
-+  bool node_integration = false;
-+
-   bool immersive_mode_enabled;
- 
-   bool text_autosizing_enabled;
--- 
-2.17.0

spec/api-browser-window-spec.js

@@ -1997,6 +1997,16 @@ describe('BrowserWindow module', () => {
           w.loadFile(path.join(fixtures, 'api', 'window-open-location-open.html'))
         })
       })
+
+      it('should have nodeIntegration disabled in child windows', async () => {
+        return new Promise((resolve, reject) => {
+          ipcMain.once('answer', (event, typeofProcess) => {
+            assert.strictEqual(typeofProcess, 'undefined')
+            resolve()
+          })
+          w.loadFile(path.join(fixtures, 'api', 'native-window-open-argv.html'))
+        })
+      })
     })
   })
 

spec/fixtures/api/native-window-open-argv.html

@@ -0,0 +1,8 @@
+<html>
+<body>
+<script type="text/javascript" charset="utf-8">
+  const popup = window.open()
+  require('electron').ipcRenderer.send('answer', typeof popup.process)
+</script>
+</body>
+</html>