fix: set nativeWindowOpen when sandboxed (#18273) (#18797)

electron · Jun 20, 2019 · 5f30c61 · 5f30c61
1 parent 5d67ec3
commit 5f30c61
Show file tree

Hide file tree

Showing 6 changed files with 44 additions and 24 deletions.
diff --git a/atom/browser/atom_browser_client.cc b/atom/browser/atom_browser_client.cc
@@ -466,11 +466,6 @@ bool AtomBrowserClient::CanCreateWindow(
 
   int opener_render_process_id = opener->GetProcess()->GetID();
 
-  if (IsRendererSandboxed(opener_render_process_id)) {
-    *no_javascript_access = false;
-    return true;
-  }
-
   if (RendererUsesNativeWindowOpen(opener_render_process_id)) {
     if (RendererDisablesPopups(opener_render_process_id)) {
       // <webview> without allowpopups attribute should return

diff --git a/atom/browser/web_contents_preferences.cc b/atom/browser/web_contents_preferences.cc
@@ -136,6 +136,8 @@ WebContentsPreferences::WebContentsPreferences(
 #endif
   SetDefaultBoolIfUndefined(options::kOffscreen, false);
 
+  SetDefaults();
+
   last_preference_ = preference_.Clone();
 }
 
@@ -144,6 +146,12 @@ WebContentsPreferences::~WebContentsPreferences() {
                    instances_.end());
 }
 
+void WebContentsPreferences::SetDefaults() {
+  if (IsEnabled(options::kSandbox)) {
+    SetBool(options::kNativeWindowOpen, true);
+  }
+}
+
 bool WebContentsPreferences::SetDefaultBoolIfUndefined(
     const base::StringPiece& key,
     bool val) {
@@ -157,6 +165,10 @@ bool WebContentsPreferences::SetDefaultBoolIfUndefined(
   }
 }
 
+void WebContentsPreferences::SetBool(const base::StringPiece& key, bool value) {
+  preference_.SetKey(key, base::Value(value));
+}
+
 bool WebContentsPreferences::IsEnabled(const base::StringPiece& name,
                                        bool default_value) const {
   auto* current_value =
@@ -169,6 +181,8 @@ bool WebContentsPreferences::IsEnabled(const base::StringPiece& name,
 void WebContentsPreferences::Merge(const base::DictionaryValue& extend) {
   if (preference_.is_dict())
     static_cast<base::DictionaryValue*>(&preference_)->MergeDictionary(&extend);
+
+  SetDefaults();
 }
 
 void WebContentsPreferences::Clear() {

diff --git a/atom/browser/web_contents_preferences.h b/atom/browser/web_contents_preferences.h
@@ -36,6 +36,9 @@ class WebContentsPreferences
                          const mate::Dictionary& web_preferences);
   ~WebContentsPreferences() override;
 
+  // Set WebPreferences defaults onto the JS object.
+  void SetDefaults();
+
   // A simple way to know whether a Boolean property is enabled.
   bool IsEnabled(const base::StringPiece& name,
                  bool default_value = false) const;
@@ -75,6 +78,9 @@ class WebContentsPreferences
   // Set preference value to given bool if user did not provide value
   bool SetDefaultBoolIfUndefined(const base::StringPiece& key, bool val);
 
+  // Set preference value to given bool
+  void SetBool(const base::StringPiece& key, bool value);
+
   static std::vector<WebContentsPreferences*> instances_;
 
   content::WebContents* web_contents_;

diff --git a/lib/browser/guest-view-manager.js b/lib/browser/guest-view-manager.js
@@ -118,7 +118,6 @@ const createGuest = function (embedder, params) {
       }
       this.loadURL(params.src, opts)
     }
-    guest.allowPopups = params.allowpopups
     embedder.emit('did-attach-webview', event, guest)
   })
 
@@ -220,6 +219,7 @@ const attachGuest = function (event, embedderFrameId, elementInstanceId, guestIn
     enableRemoteModule: params.enableremotemodule,
     plugins: params.plugins,
     zoomFactor: embedder._getZoomFactor(),
+    disablePopups: !params.allowpopups,
     webSecurity: !params.disablewebsecurity,
     enableBlinkFeatures: params.blinkfeatures,
     disableBlinkFeatures: params.disableblinkfeatures
@@ -241,11 +241,6 @@ const attachGuest = function (event, embedderFrameId, elementInstanceId, guestIn
     webPreferences.preloadURL = params.preload
   }
 
-  // Return null from native window.open if allowpopups is unset
-  if (webPreferences.nativeWindowOpen === true && !params.allowpopups) {
-    webPreferences.disablePopups = true
-  }
-
   // Security options that guest will always inherit from embedder
   const inheritedWebPreferences = new Map([
     ['contextIsolation', true],

diff --git a/lib/browser/guest-window-manager.js b/lib/browser/guest-window-manager.js
@@ -259,7 +259,7 @@ ipcMain.on('ELECTRON_GUEST_WINDOW_MANAGER_INTERNAL_WINDOW_OPEN', function (event
   options = mergeBrowserWindowOptions(event.sender, options)
   event.sender.emit('new-window', event, url, frameName, disposition, options, additionalFeatures, referrer)
   const { newGuest } = event
-  if ((event.sender.isGuest() && !event.sender.allowPopups) || event.defaultPrevented) {
+  if ((event.sender.isGuest() && event.sender.getLastWebPreferences().disablePopups) || event.defaultPrevented) {
     if (newGuest != null) {
       if (options.webContents === newGuest.webContents) {
         // the webContents is not changed, so set defaultPrevented to false to

diff --git a/spec/webview-spec.js b/spec/webview-spec.js
@@ -522,20 +522,30 @@ describe('<webview> tag', function () {
   })
 
   describe('allowpopups attribute', () => {
-    it('can not open new window when not set', async () => {
-      const message = await startLoadingWebViewAndWaitForMessage(webview, {
-        src: `file://${fixtures}/pages/window-open-hide.html`
-      })
-      expect(message).to.equal('null')
-    })
+    const generateSpecs = (description, webpreferences = '') => {
+      describe(description, () => {
+        it('can not open new window when not set', async () => {
+          const message = await startLoadingWebViewAndWaitForMessage(webview, {
+            webpreferences,
+            src: `file://${fixtures}/pages/window-open-hide.html`
+          })
+          expect(message).to.equal('null')
+        })
 
-    it('can open new window when set', async () => {
-      const message = await startLoadingWebViewAndWaitForMessage(webview, {
-        allowpopups: 'on',
-        src: `file://${fixtures}/pages/window-open-hide.html`
+        it('can open new window when set', async () => {
+          const message = await startLoadingWebViewAndWaitForMessage(webview, {
+            webpreferences,
+            allowpopups: 'on',
+            src: `file://${fixtures}/pages/window-open-hide.html`
+          })
+          expect(message).to.equal('window')
+        })
       })
-      expect(message).to.equal('window')
-    })
+    }
+
+    generateSpecs('without sandbox')
+    generateSpecs('with sandbox', 'sandbox=yes')
+    generateSpecs('with nativeWindowOpen', 'nativeWindowOpen=yes')
   })
 
   describe('webpreferences attribute', () => {