From 2f3a8ecd42d70d6466aec5bb1748dfea8f08764e Mon Sep 17 00:00:00 2001 From: Cheng Zhao Date: Thu, 25 Oct 2018 15:31:07 +0900 Subject: [PATCH] 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 | 2 - atom/renderer/atom_renderer_client.cc | 25 ++++++------ docs/api/breaking-changes.md | 3 ++ docs/api/browser-window.md | 3 +- patches/common/chromium/.patches | 1 - patches/common/chromium/web_preferences.patch | 39 ------------------- spec/api-browser-window-spec.js | 10 +++++ .../fixtures/api/native-window-open-argv.html | 8 ++++ 8 files changed, 36 insertions(+), 55 deletions(-) delete mode 100644 patches/common/chromium/web_preferences.patch create mode 100644 spec/fixtures/api/native-window-open-argv.html diff --git a/atom/browser/web_contents_preferences.cc b/atom/browser/web_contents_preferences.cc index fc4bd0dc11..fd0112d6b7 100644 --- a/atom/browser/web_contents_preferences.cc +++ b/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 diff --git a/atom/renderer/atom_renderer_client.cc b/atom/renderer/atom_renderer_client.cc index 4243fec81a..711955aeda 100644 --- a/atom/renderer/atom_renderer_client.cc +++ b/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. // - // TODO(zcbenz): We shouldn't allow node integration even for the top frame. - if (!render_frame->GetWebkitPreferences().node_integration && - render_frame->GetWebFrame()->Opener()) + // 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): 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); diff --git a/docs/api/breaking-changes.md b/docs/api/breaking-changes.md index 6ed5394a3d..2ece6adcc6 100644 --- a/docs/api/breaking-changes.md +++ b/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) diff --git a/docs/api/browser-window.md b/docs/api/browser-window.md index d32fcc9925..075c99c2b4 100644 --- a/docs/api/browser-window.md +++ b/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 [`` tag](webview-tag.md). Defaults to the value of the `nodeIntegration` option. **Note:** The diff --git a/patches/common/chromium/.patches b/patches/common/chromium/.patches index 5ed16f1fdf..2a0f2a1df2 100644 --- a/patches/common/chromium/.patches +++ b/patches/common/chromium/.patches @@ -76,4 +76,3 @@ tts.patch color_chooser.patch printing.patch verbose_generate_breakpad_symbols.patch -web_preferences.patch diff --git a/patches/common/chromium/web_preferences.patch b/patches/common/chromium/web_preferences.patch deleted file mode 100644 index 84a26c786f..0000000000 --- a/patches/common/chromium/web_preferences.patch +++ /dev/null @@ -1,39 +0,0 @@ -From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001 -From: zcbenz -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 diff --git a/spec/api-browser-window-spec.js b/spec/api-browser-window-spec.js index ee5281e99d..8505ded51f 100644 --- a/spec/api-browser-window-spec.js +++ b/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')) + }) + }) }) }) diff --git a/spec/fixtures/api/native-window-open-argv.html b/spec/fixtures/api/native-window-open-argv.html new file mode 100644 index 0000000000..fa613b1bf7 --- /dev/null +++ b/spec/fixtures/api/native-window-open-argv.html @@ -0,0 +1,8 @@ + + + + +