From b4276835d8a8212e7def2fa2b0838d86132fe150 Mon Sep 17 00:00:00 2001 From: Alexandre Lacheze Date: Mon, 3 Jun 2019 20:23:15 +0000 Subject: [PATCH] fix: lost window.opener after cross-origin navigation (#18173) * Get a site instance related to current one instead of creation a new one Using `GetRelatedSiteInstance` will keep the relation (same browsing instance) between the current and the new site instance. * Some relies on preloads in opened window The fact that, now, we always have an opener for opened windows diables note integration in opened windows, except if `nodeIntegrationInSubFrames` is enabled. * Add a test on window.opener after cross-orgin navigation * Make sure to unregisterProtocol in tests * Introduc and use a NetworkSandbox for tests * Modify tests about zoom persistence to properly simulate cross-origin navigation * Revert "Modify tests about zoom persistence to properly simulate cross-origin navigation" This reverts commit 0a7537f2eb7f183ddec16637e8a2e92a0d600321. --- .../common/chromium/frame_host_manager.patch | 81 ++++++++++++++++++- spec/api-browser-window-spec.js | 77 +++++++++++------- spec/fixtures/api/window-open-preload.js | 3 +- spec/network-helper.js | 75 +++++++++++++++++ 4 files changed, 206 insertions(+), 30 deletions(-) create mode 100644 spec/network-helper.js diff --git a/patches/common/chromium/frame_host_manager.patch b/patches/common/chromium/frame_host_manager.patch index 8c243d4d362f..c4010a56acac 100644 --- a/patches/common/chromium/frame_host_manager.patch +++ b/patches/common/chromium/frame_host_manager.patch @@ -7,8 +7,42 @@ Allows embedder to intercept site instances chosen by chromium and respond with custom instance. Also allows for us to at-runtime enable or disable this patch. +diff --git a/content/browser/browsing_instance.cc b/content/browser/browsing_instance.cc +index 12e1c5cff95aa6d0a907a249208e23371cf29785..3bc26b7870ff3bf6a69cb1e123fb372f763442ee 100644 +--- a/content/browser/browsing_instance.cc ++++ b/content/browser/browsing_instance.cc +@@ -79,6 +79,13 @@ scoped_refptr BrowsingInstance::GetSiteInstanceForURL( + return instance; + } + ++scoped_refptr BrowsingInstance::CreateSiteInstanceForURL( ++ const GURL& url) { ++ scoped_refptr instance = new SiteInstanceImpl(this); ++ instance->SetSite(url); ++ return instance; ++} ++ + void BrowsingInstance::GetSiteAndLockForURL(const GURL& url, + bool allow_default_instance, + GURL* site_url, +diff --git a/content/browser/browsing_instance.h b/content/browser/browsing_instance.h +index 775b64a8d20f89845812852a2904a1e6875c2b4a..5235b57bbf44fc7b30ca6943c43a290f07f003bf 100644 +--- a/content/browser/browsing_instance.h ++++ b/content/browser/browsing_instance.h +@@ -136,6 +136,11 @@ class CONTENT_EXPORT BrowsingInstance final + const GURL& url, + bool allow_default_instance); + ++ // Create a new SiteInstance for the given URL bound the current ++ // BrowsingInstance. ++ scoped_refptr CreateSiteInstanceForURL( ++ const GURL& url); ++ + // Adds the given SiteInstance to our map, to ensure that we do not create + // another SiteInstance for the same site. + void RegisterSiteInstance(SiteInstanceImpl* site_instance); diff --git a/content/browser/frame_host/render_frame_host_manager.cc b/content/browser/frame_host/render_frame_host_manager.cc -index b5301d164138f21ca8ae01abfb153efde51ec324..886b6d3beb3e2d7b13a15af830bea6fb5a567cba 100644 +index b5301d164138f21ca8ae01abfb153efde51ec324..55f87cc788c7eed13494ebe6ea6eb18027a04996 100644 --- a/content/browser/frame_host/render_frame_host_manager.cc +++ b/content/browser/frame_host/render_frame_host_manager.cc @@ -2127,6 +2127,20 @@ bool RenderFrameHostManager::InitRenderView( @@ -56,7 +90,7 @@ index b5301d164138f21ca8ae01abfb153efde51ec324..886b6d3beb3e2d7b13a15af830bea6fb + overriden_site_instance = + candidate_site_instance + ? candidate_site_instance -+ : SiteInstance::CreateForURL(browser_context, ++ : current_site_instance->CreateRelatedSiteInstance( + request.common_params().url); + break; + case ContentBrowserClient::SiteInstanceForNavigationType::FORCE_CURRENT: @@ -117,6 +151,33 @@ index b5301d164138f21ca8ae01abfb153efde51ec324..886b6d3beb3e2d7b13a15af830bea6fb return dest_site_instance; } +diff --git a/content/browser/site_instance_impl.cc b/content/browser/site_instance_impl.cc +index fd184108a7993094c29be3f7ebde658e259ede2c..75aa4a6b7d58a1bebe34efc923953c69348428a9 100644 +--- a/content/browser/site_instance_impl.cc ++++ b/content/browser/site_instance_impl.cc +@@ -342,6 +342,10 @@ bool SiteInstanceImpl::HasRelatedSiteInstance(const GURL& url) { + return browsing_instance_->HasSiteInstance(url); + } + ++scoped_refptr SiteInstanceImpl::CreateRelatedSiteInstance(const GURL& url) { ++ return browsing_instance_->CreateSiteInstanceForURL(url); ++} ++ + scoped_refptr SiteInstanceImpl::GetRelatedSiteInstance( + const GURL& url) { + return browsing_instance_->GetSiteInstanceForURL( +diff --git a/content/browser/site_instance_impl.h b/content/browser/site_instance_impl.h +index a46901055bdf17b6b0dab14edf753b234dc04a12..29c201b0c95eb0c7a35f47d6f3ab5b48c73dbf15 100644 +--- a/content/browser/site_instance_impl.h ++++ b/content/browser/site_instance_impl.h +@@ -83,6 +83,7 @@ class CONTENT_EXPORT SiteInstanceImpl final : public SiteInstance, + BrowserContext* GetBrowserContext() override; + const GURL& GetSiteURL() override; + scoped_refptr GetRelatedSiteInstance(const GURL& url) override; ++ scoped_refptr CreateRelatedSiteInstance(const GURL& url) override; + bool IsRelatedSiteInstance(const SiteInstance* instance) override; + size_t GetRelatedActiveContentsCount() override; + bool RequiresDedicatedProcess() override; diff --git a/content/public/browser/content_browser_client.cc b/content/public/browser/content_browser_client.cc index 787cd81b26508d3e04956721f0e7cec2f457aa60..8e62a5dd26595411757e03078ed0e44646c47a52 100644 --- a/content/public/browser/content_browser_client.cc @@ -187,3 +248,19 @@ index bf9b3a601fc16d5070e4467a258a047f47b059f3..3c35eddc2498157c2b98bab55991d8aa // Allows the embedder to set any number of custom BrowserMainParts // implementations for the browser startup code. See comments in // browser_main_parts.h. +diff --git a/content/public/browser/site_instance.h b/content/public/browser/site_instance.h +index a3e880e20e51d988175f0e8e2c42e7f5c1740104..61bbf88265e717934533117efbc2330661e32b11 100644 +--- a/content/public/browser/site_instance.h ++++ b/content/public/browser/site_instance.h +@@ -121,6 +121,11 @@ class CONTENT_EXPORT SiteInstance : public base::RefCounted { + // corresponds to a site URL with the host "example.com". + virtual const GURL& GetSiteURL() = 0; + ++ // Create a SiteInstance for the given URL that shares the current ++ // BrowsingInstance. ++ virtual scoped_refptr CreateRelatedSiteInstance( ++ const GURL& url) = 0; ++ + // Gets a SiteInstance for the given URL that shares the current + // BrowsingInstance, creating a new SiteInstance if necessary. This ensures + // that a BrowsingInstance only has one SiteInstance per site, so that pages diff --git a/spec/api-browser-window-spec.js b/spec/api-browser-window-spec.js index 6ccab009aa68..02de0e306046 100644 --- a/spec/api-browser-window-spec.js +++ b/spec/api-browser-window-spec.js @@ -9,6 +9,7 @@ const qs = require('querystring') const http = require('http') const { closeWindow } = require('./window-helpers') const { emittedOnce } = require('./events-helpers') +const { createNetworkSandbox } = require('./network-helper') const { ipcRenderer, remote } = require('electron') const { app, ipcMain, BrowserWindow, BrowserView, protocol, session, screen, webContents } = remote @@ -1997,17 +1998,29 @@ describe('BrowserWindow module', () => { }) describe('nativeWindowOpen option', () => { - beforeEach(() => { + const networkSandbox = createNetworkSandbox(protocol) + + beforeEach(async () => { + // used to create cross-origin navigation situations + await networkSandbox.serveFileFromProtocol('foo', path.join(fixtures, 'api', 'window-open-location-change.html')) + await networkSandbox.serveFileFromProtocol('bar', path.join(fixtures, 'api', 'window-open-location-final.html')) + w.destroy() w = new BrowserWindow({ show: false, webPreferences: { nodeIntegration: true, - nativeWindowOpen: true + nativeWindowOpen: true, + // tests relies on preloads in opened windows + nodeIntegrationInSubFrames: true } }) }) + afterEach(async () => { + await networkSandbox.reset() + }) + it('opens window of about:blank with cross-scripting enabled', (done) => { ipcMain.once('answer', (event, content) => { expect(content).to.equal('Hello') @@ -2052,7 +2065,9 @@ describe('BrowserWindow module', () => { w = new BrowserWindow({ show: false, webPreferences: { - nativeWindowOpen: true + nativeWindowOpen: true, + // test relies on preloads in opened window + nodeIntegrationInSubFrames: true } }) @@ -2069,7 +2084,9 @@ describe('BrowserWindow module', () => { w = new BrowserWindow({ show: false, webPreferences: { - nativeWindowOpen: true + nativeWindowOpen: true, + // test relies on preloads in opened window + nodeIntegrationInSubFrames: true } }) @@ -2083,14 +2100,13 @@ describe('BrowserWindow module', () => { w.loadFile(path.join(fixtures, 'api', 'new-window.html')) }) it('retains the original web preferences when window.location is changed to a new origin', async () => { - await serveFileFromProtocol('foo', path.join(fixtures, 'api', 'window-open-location-change.html')) - await serveFileFromProtocol('bar', path.join(fixtures, 'api', 'window-open-location-final.html')) - w.destroy() w = new BrowserWindow({ show: true, webPreferences: { - nativeWindowOpen: true + nativeWindowOpen: true, + // test relies on preloads in opened window + nodeIntegrationInSubFrames: true } }) @@ -2103,7 +2119,33 @@ describe('BrowserWindow module', () => { expect(typeofProcess).to.eql('undefined') }) + it('window.opener is not null when window.location is changed to a new origin', async () => { + w.destroy() + w = new BrowserWindow({ + show: true, + webPreferences: { + nativeWindowOpen: true, + // test relies on preloads in opened window + nodeIntegrationInSubFrames: true + } + }) + + ipcRenderer.send('set-web-preferences-on-next-new-window', w.webContents.id, 'preload', path.join(fixtures, 'api', 'window-open-preload.js')) + const p = emittedOnce(ipcMain, 'answer') + w.loadFile(path.join(fixtures, 'api', 'window-open-location-open.html')) + const [, , , windowOpenerIsNull] = await p + expect(windowOpenerIsNull).to.be.false('window.opener is null') + }) + it('should have nodeIntegration disabled in child windows', async () => { + w.destroy() + w = new BrowserWindow({ + show: false, + webPreferences: { + nodeIntegration: true, + nativeWindowOpen: true + } + }) const p = emittedOnce(ipcMain, 'answer') w.loadFile(path.join(fixtures, 'api', 'native-window-open-argv.html')) const [, typeofProcess] = await p @@ -3777,22 +3819,3 @@ const isScaleFactorRounding = () => { // Return true if scale factor is odd number above 2 return scaleFactor > 2 && scaleFactor % 2 === 1 } - -function serveFileFromProtocol (protocolName, filePath) { - return new Promise((resolve, reject) => { - protocol.registerBufferProtocol(protocolName, (request, callback) => { - // Disabled due to false positive in StandardJS - // eslint-disable-next-line standard/no-callback-literal - callback({ - mimeType: 'text/html', - data: fs.readFileSync(filePath) - }) - }, (error) => { - if (error != null) { - reject(error) - } else { - resolve() - } - }) - }) -} diff --git a/spec/fixtures/api/window-open-preload.js b/spec/fixtures/api/window-open-preload.js index fcb57a207c91..ced0c05b1bad 100644 --- a/spec/fixtures/api/window-open-preload.js +++ b/spec/fixtures/api/window-open-preload.js @@ -2,7 +2,8 @@ const { ipcRenderer } = require('electron') setImmediate(function () { if (window.location.toString() === 'bar://page') { - ipcRenderer.send('answer', process.argv, typeof global.process) + const windowOpenerIsNull = window.opener == null + ipcRenderer.send('answer', process.argv, typeof global.process, windowOpenerIsNull) window.close() } }) diff --git a/spec/network-helper.js b/spec/network-helper.js new file mode 100644 index 000000000000..873b5f79aeed --- /dev/null +++ b/spec/network-helper.js @@ -0,0 +1,75 @@ +const fs = require('fs') + +/** + * Test sandbox environment used to fake network responses. + */ +class NetworkSandbox { + constructor (protocol) { + this.protocol = protocol + this._resetFns = [] + } + + /** + * Reset the sandbox. + */ + async reset () { + for (const resetFn of this._resetFns) { + await resetFn() + } + this._resetFns = [] + } + + /** + * Will serve the content of file at `filePath` to network requests towards + * `protocolName` scheme. + * + * Example: `sandbox.serveFileFromProtocol('foo', 'index.html')` will serve the content + * of 'index.html' to `foo://page` requests. + * + * @param {string} protocolName + * @param {string} filePath + */ + serveFileFromProtocol (protocolName, filePath) { + return new Promise((resolve, reject) => { + this.protocol.registerBufferProtocol(protocolName, (request, callback) => { + // Disabled due to false positive in StandardJS + // eslint-disable-next-line standard/no-callback-literal + callback({ + mimeType: 'text/html', + data: fs.readFileSync(filePath) + }) + }, (error) => { + if (error != null) { + reject(error) + } else { + this._resetFns.push(() => this.unregisterProtocol(protocolName)) + resolve() + } + }) + }) + } + + unregisterProtocol (protocolName) { + return new Promise((resolve, reject) => { + this.protocol.unregisterProtocol(protocolName, (error) => { + if (error != null) { + reject(error) + } else { + resolve() + } + }) + }) + } +} + +/** + * Will create a NetworkSandbox that uses + * `protocol` as `Electron.Protocol`. + * + * @param {Electron.Protocol} protocol + */ +function createNetworkSandbox (protocol) { + return new NetworkSandbox(protocol) +} + +exports.createNetworkSandbox = createNetworkSandbox