From 57d2ae1aec141ef41f265ea49558861a4454d4f6 Mon Sep 17 00:00:00 2001 From: Pedro Pontes Date: Tue, 20 Nov 2018 21:28:26 +0100 Subject: [PATCH] revert: "fix: window.open site instance should belong to same browsing instance (#15216)" (#15757) This reverts commit 8f35198bfb0311530d390f5894502489298d5b25. --- atom/browser/atom_browser_client.cc | 158 +++++++----------- atom/browser/atom_browser_client.h | 36 ++-- atom/browser/atom_browser_main_parts.cc | 2 - lib/browser/guest-window-manager.js | 22 ++- .../common/chromium/frame_host_manager.patch | 117 ++----------- spec/api-browser-window-affinity-spec.js | 104 ++++++------ spec/api-browser-window-spec.js | 62 +++---- spec/chromium-spec.js | 6 +- spec/fixtures/api/sandbox.html | 5 +- spec/fixtures/module/preload-sandbox.js | 11 +- 10 files changed, 190 insertions(+), 333 deletions(-) diff --git a/atom/browser/atom_browser_client.cc b/atom/browser/atom_browser_client.cc index e11e32d6a3b37..0b2f30ae26731 100644 --- a/atom/browser/atom_browser_client.cc +++ b/atom/browser/atom_browser_client.cc @@ -179,7 +179,7 @@ AtomBrowserClient::~AtomBrowserClient() { content::WebContents* AtomBrowserClient::GetWebContentsFromProcessID( int process_id) { // If the process is a pending process, we should use the web contents - // for the frame host passed into RegisterPendingProcess. + // for the frame host passed into OverrideSiteInstanceForNavigation. if (base::ContainsKey(pending_processes_, process_id)) return pending_processes_[process_id]; @@ -188,21 +188,17 @@ content::WebContents* AtomBrowserClient::GetWebContentsFromProcessID( return WebContentsPreferences::GetWebContentsFromProcessID(process_id); } -bool AtomBrowserClient::ShouldForceNewSiteInstance( +bool AtomBrowserClient::ShouldCreateNewSiteInstance( content::RenderFrameHost* render_frame_host, content::BrowserContext* browser_context, content::SiteInstance* current_instance, - const GURL& url) const { + const GURL& url) { if (url.SchemeIs(url::kJavaScriptScheme)) // "javacript:" scheme should always use same SiteInstance return false; int process_id = current_instance->GetProcess()->GetID(); - if (IsRendererSandboxed(process_id)) { - // Renderer is sandboxed, delegate the decision to the content layer for all - // origins. - return false; - } else { + if (!IsRendererSandboxed(process_id)) { if (!RendererUsesNativeWindowOpen(process_id)) { // non-sandboxed renderers without native window.open should always create // a new SiteInstance @@ -234,65 +230,25 @@ void AtomBrowserClient::RemoveProcessPreferences(int process_id) { process_preferences_.erase(process_id); } -bool AtomBrowserClient::IsProcessObserved(int process_id) const { +bool AtomBrowserClient::IsProcessObserved(int process_id) { return process_preferences_.find(process_id) != process_preferences_.end(); } -bool AtomBrowserClient::IsRendererSandboxed(int process_id) const { +bool AtomBrowserClient::IsRendererSandboxed(int process_id) { auto it = process_preferences_.find(process_id); return it != process_preferences_.end() && it->second.sandbox; } -bool AtomBrowserClient::RendererUsesNativeWindowOpen(int process_id) const { +bool AtomBrowserClient::RendererUsesNativeWindowOpen(int process_id) { auto it = process_preferences_.find(process_id); return it != process_preferences_.end() && it->second.native_window_open; } -bool AtomBrowserClient::RendererDisablesPopups(int process_id) const { +bool AtomBrowserClient::RendererDisablesPopups(int process_id) { auto it = process_preferences_.find(process_id); return it != process_preferences_.end() && it->second.disable_popups; } -std::string AtomBrowserClient::GetAffinityPreference( - content::RenderFrameHost* rfh) const { - auto* web_contents = content::WebContents::FromRenderFrameHost(rfh); - auto* web_preferences = WebContentsPreferences::From(web_contents); - std::string affinity; - if (web_preferences && - web_preferences->GetPreference("affinity", &affinity) && - !affinity.empty()) { - affinity = base::ToLowerASCII(affinity); - } - - return affinity; -} - -content::SiteInstance* AtomBrowserClient::GetSiteInstanceFromAffinity( - content::BrowserContext* browser_context, - const GURL& url, - content::RenderFrameHost* rfh) const { - std::string affinity = GetAffinityPreference(rfh); - if (!affinity.empty()) { - auto iter = site_per_affinities_.find(affinity); - GURL dest_site = content::SiteInstance::GetSiteForURL(browser_context, url); - if (iter != site_per_affinities_.end() && - IsSameWebSite(browser_context, iter->second->GetSiteURL(), dest_site)) { - return iter->second; - } - } - - return nullptr; -} - -void AtomBrowserClient::ConsiderSiteInstanceForAffinity( - content::RenderFrameHost* rfh, - content::SiteInstance* site_instance) { - std::string affinity = GetAffinityPreference(rfh); - if (!affinity.empty()) { - site_per_affinities_[affinity] = site_instance; - } -} - void AtomBrowserClient::RenderProcessWillLaunch( content::RenderProcessHost* host, service_manager::mojom::ServiceRequest* service_request) { @@ -358,59 +314,62 @@ void AtomBrowserClient::OverrideWebkitPrefs(content::RenderViewHost* host, web_preferences->OverrideWebkitPrefs(prefs); } -content::ContentBrowserClient::SiteInstanceForNavigationType -AtomBrowserClient::ShouldOverrideSiteInstanceForNavigation( +void AtomBrowserClient::OverrideSiteInstanceForNavigation( content::RenderFrameHost* rfh, content::BrowserContext* browser_context, const GURL& url, bool has_request_started, - content::SiteInstance** affinity_site_instance) const { + content::SiteInstance* candidate_instance, + content::SiteInstance** new_instance) { if (g_suppress_renderer_process_restart) { g_suppress_renderer_process_restart = false; - return SiteInstanceForNavigationType::ASK_CHROMIUM; - } - - // Do we have an affinity site to manage ? - content::SiteInstance* site_instance_from_affinity = - GetSiteInstanceFromAffinity(browser_context, url, rfh); - if (site_instance_from_affinity) { - *affinity_site_instance = site_instance_from_affinity; - return SiteInstanceForNavigationType::FORCE_AFFINITY; + return; } content::SiteInstance* current_instance = rfh->GetSiteInstance(); - if (!ShouldForceNewSiteInstance(rfh, browser_context, current_instance, - url)) { - return SiteInstanceForNavigationType::ASK_CHROMIUM; - } - - // ShouldOverrideSiteInstanceForNavigation will be called more than once - // during a navigation (currently twice, on request and when it's about - // to commit in the renderer), look at - // RenderFrameHostManager::GetFrameHostForNavigation. - // In the default mode we should reuse the same site instance until the - // request commits otherwise it will get destroyed. Currently there is no - // unique lifetime tracker for a navigation request during site instance - // creation. We check for the state of the request, which should be one of - // (WAITING_FOR_RENDERER_RESPONSE, STARTED, RESPONSE_STARTED, FAILED) along - // with the availability of a speculative render frame host. - if (has_request_started) { - return SiteInstanceForNavigationType::FORCE_CURRENT; - } - - return SiteInstanceForNavigationType::FORCE_CANDIDATE_OR_NEW; -} - -void AtomBrowserClient::RegisterPendingSiteInstance( - content::RenderFrameHost* rfh, - content::SiteInstance* pending_site_instance) { - // Do we have an affinity site to manage? - ConsiderSiteInstanceForAffinity(rfh, pending_site_instance); + if (!ShouldCreateNewSiteInstance(rfh, browser_context, current_instance, url)) + return; - // Remember the original web contents for the pending renderer process. + // Do we have an affinity site to manage ? auto* web_contents = content::WebContents::FromRenderFrameHost(rfh); - auto* pending_process = pending_site_instance->GetProcess(); - pending_processes_[pending_process->GetID()] = web_contents; + auto* web_preferences = WebContentsPreferences::From(web_contents); + std::string affinity; + if (web_preferences && + web_preferences->GetPreference("affinity", &affinity) && + !affinity.empty()) { + affinity = base::ToLowerASCII(affinity); + auto iter = site_per_affinities.find(affinity); + GURL dest_site = content::SiteInstance::GetSiteForURL(browser_context, url); + if (iter != site_per_affinities.end() && + IsSameWebSite(browser_context, iter->second->GetSiteURL(), dest_site)) { + *new_instance = iter->second; + } else { + site_per_affinities[affinity] = candidate_instance; + *new_instance = candidate_instance; + // Remember the original web contents for the pending renderer process. + auto* pending_process = candidate_instance->GetProcess(); + pending_processes_[pending_process->GetID()] = web_contents; + } + } else { + // OverrideSiteInstanceForNavigation will be called more than once during a + // navigation (currently twice, on request and when it's about to commit in + // the renderer), look at RenderFrameHostManager::GetFrameHostForNavigation. + // In the default mode we should resuse the same site instance until the + // request commits otherwise it will get destroyed. Currently there is no + // unique lifetime tracker for a navigation request during site instance + // creation. We check for the state of the request, which should be one of + // (WAITING_FOR_RENDERER_RESPONSE, STARTED, RESPONSE_STARTED, FAILED) along + // with the availability of a speculative render frame host. + if (has_request_started) { + *new_instance = current_instance; + return; + } + + *new_instance = candidate_instance; + // Remember the original web contents for the pending renderer process. + auto* pending_process = candidate_instance->GetProcess(); + pending_processes_[pending_process->GetID()] = web_contents; + } } void AtomBrowserClient::AppendExtraCommandLineSwitches( @@ -588,10 +547,10 @@ void AtomBrowserClient::SiteInstanceDeleting( content::SiteInstance* site_instance) { // We are storing weak_ptr, is it fundamental to maintain the map up-to-date // when an instance is destroyed. - for (auto iter = site_per_affinities_.begin(); - iter != site_per_affinities_.end(); ++iter) { + for (auto iter = site_per_affinities.begin(); + iter != site_per_affinities.end(); ++iter) { if (iter->second == site_instance) { - site_per_affinities_.erase(iter); + site_per_affinities.erase(iter); break; } } @@ -825,9 +784,4 @@ std::string AtomBrowserClient::GetApplicationLocale() { return *g_application_locale; } -bool AtomBrowserClient::ShouldEnableStrictSiteIsolation() { - // Enable site isolation. It is off by default in Chromium <= 69. - return true; -} - } // namespace atom diff --git a/atom/browser/atom_browser_client.h b/atom/browser/atom_browser_client.h index 6105ebafdcb31..e3cd4aa247c85 100644 --- a/atom/browser/atom_browser_client.h +++ b/atom/browser/atom_browser_client.h @@ -64,9 +64,6 @@ class AtomBrowserClient : public content::ContentBrowserClient, // content::ContentBrowserClient: std::string GetApplicationLocale() override; - // content::ContentBrowserClient: - bool ShouldEnableStrictSiteIsolation() override; - protected: void RenderProcessWillLaunch( content::RenderProcessHost* host, @@ -75,15 +72,13 @@ class AtomBrowserClient : public content::ContentBrowserClient, CreateSpeechRecognitionManagerDelegate() override; void OverrideWebkitPrefs(content::RenderViewHost* render_view_host, content::WebPreferences* prefs) override; - SiteInstanceForNavigationType ShouldOverrideSiteInstanceForNavigation( + void OverrideSiteInstanceForNavigation( content::RenderFrameHost* render_frame_host, content::BrowserContext* browser_context, const GURL& dest_url, bool has_request_started, - content::SiteInstance** affinity_instance) const override; - void RegisterPendingSiteInstance( - content::RenderFrameHost* render_frame_host, - content::SiteInstance* pending_site_instance) override; + content::SiteInstance* candidate_instance, + content::SiteInstance** new_instance) override; void AppendExtraCommandLineSwitches(base::CommandLine* command_line, int child_process_id) override; void DidCreatePpapiPlugin(content::BrowserPpapiHost* browser_host) override; @@ -171,23 +166,16 @@ class AtomBrowserClient : public content::ContentBrowserClient, bool disable_popups = false; }; - bool ShouldForceNewSiteInstance(content::RenderFrameHost* render_frame_host, - content::BrowserContext* browser_context, - content::SiteInstance* current_instance, - const GURL& dest_url) const; + bool ShouldCreateNewSiteInstance(content::RenderFrameHost* render_frame_host, + content::BrowserContext* browser_context, + content::SiteInstance* current_instance, + const GURL& dest_url); void AddProcessPreferences(int process_id, ProcessPreferences prefs); void RemoveProcessPreferences(int process_id); - bool IsProcessObserved(int process_id) const; - bool IsRendererSandboxed(int process_id) const; - bool RendererUsesNativeWindowOpen(int process_id) const; - bool RendererDisablesPopups(int process_id) const; - std::string GetAffinityPreference(content::RenderFrameHost* rfh) const; - content::SiteInstance* GetSiteInstanceFromAffinity( - content::BrowserContext* browser_context, - const GURL& url, - content::RenderFrameHost* rfh) const; - void ConsiderSiteInstanceForAffinity(content::RenderFrameHost* rfh, - content::SiteInstance* site_instance); + bool IsProcessObserved(int process_id); + bool IsRendererSandboxed(int process_id); + bool RendererUsesNativeWindowOpen(int process_id); + bool RendererDisablesPopups(int process_id); // pending_render_process => web contents. std::map pending_processes_; @@ -196,7 +184,7 @@ class AtomBrowserClient : public content::ContentBrowserClient, std::map render_process_host_pids_; // list of site per affinity. weak_ptr to prevent instance locking - std::map site_per_affinities_; + std::map site_per_affinities; std::unique_ptr resource_dispatcher_host_delegate_; diff --git a/atom/browser/atom_browser_main_parts.cc b/atom/browser/atom_browser_main_parts.cc index a384174c03ce0..e77a1261c6bd3 100644 --- a/atom/browser/atom_browser_main_parts.cc +++ b/atom/browser/atom_browser_main_parts.cc @@ -211,8 +211,6 @@ void AtomBrowserMainParts::InitializeFeatureList() { // Chromium drops support for the old sandbox implmentation. disable_features += std::string(",") + features::kMacV2Sandbox.name; #endif - disable_features += - std::string(",") + features::kSpareRendererForSitePerProcess.name; auto feature_list = std::make_unique(); feature_list->InitializeFromCommandLine(enable_features, disable_features); base::FeatureList::SetInstance(std::move(feature_list)); diff --git a/lib/browser/guest-window-manager.js b/lib/browser/guest-window-manager.js index 441d69ca888a3..5bc5d26be6664 100644 --- a/lib/browser/guest-window-manager.js +++ b/lib/browser/guest-window-manager.js @@ -118,12 +118,28 @@ const createGuest = function (embedder, url, referrer, frameName, options, postD } guest = new BrowserWindow(options) - if (!options.webContents) { + if (!options.webContents || url !== 'about:blank') { // We should not call `loadURL` if the window was constructed from an - // existing webContents (window.open in a sandboxed renderer). + // existing webContents(window.open in a sandboxed renderer) and if the url + // is not 'about:blank'. // // Navigating to the url when creating the window from an existing - // webContents is not necessary (it will navigate there anyway). + // webContents would not be necessary(it will navigate there anyway), but + // apparently there's a bug that allows the child window to be scripted by + // the opener, even when the child window is from another origin. + // + // That's why the second condition(url !== "about:blank") is required: to + // force `OverrideSiteInstanceForNavigation` to be called and consequently + // spawn a new renderer if the new window is targeting a different origin. + // + // If the URL is "about:blank", then it is very likely that the opener just + // wants to synchronously script the popup, for example: + // + // let popup = window.open() + // popup.document.body.write('

hello

') + // + // The above code would not work if a navigation to "about:blank" is done + // here, since the window would be cleared of all changes in the next tick. const loadOptions = { httpReferrer: referrer } diff --git a/patches/common/chromium/frame_host_manager.patch b/patches/common/chromium/frame_host_manager.patch index 721cb31bd948a..d99af3f3f144b 100644 --- a/patches/common/chromium/frame_host_manager.patch +++ b/patches/common/chromium/frame_host_manager.patch @@ -5,10 +5,10 @@ Subject: frame_host_manager.patch diff --git a/content/browser/frame_host/render_frame_host_manager.cc b/content/browser/frame_host/render_frame_host_manager.cc -index 872e4609c94f1e052d623ae57c1279c72eb2c3f4..ce6ea215b841d381477258417edaef4afd834bb7 100644 +index 872e4609c94f1e052d623ae57c1279c72eb2c3f4..a59676004f2411631418bf12e2978623b9b27b53 100644 --- a/content/browser/frame_host/render_frame_host_manager.cc +++ b/content/browser/frame_host/render_frame_host_manager.cc -@@ -1960,6 +1960,17 @@ RenderFrameHostManager::GetSiteInstanceForNavigationRequest( +@@ -1960,6 +1960,18 @@ RenderFrameHostManager::GetSiteInstanceForNavigationRequest( bool was_server_redirect = request.navigation_handle() && request.navigation_handle()->WasServerRedirect(); @@ -21,12 +21,13 @@ index 872e4609c94f1e052d623ae57c1279c72eb2c3f4..ce6ea215b841d381477258417edaef4a + scoped_refptr candidate_site_instance = + speculative_render_frame_host_ + ? speculative_render_frame_host_->GetSiteInstance() -+ : nullptr; ++ : content::SiteInstance::CreateForURL(browser_context, ++ request.common_params().url); + if (frame_tree_node_->IsMainFrame()) { // Renderer-initiated main frame navigations that may require a // SiteInstance swap are sent to the browser via the OpenURL IPC and are -@@ -1979,6 +1990,51 @@ RenderFrameHostManager::GetSiteInstanceForNavigationRequest( +@@ -1979,6 +1991,19 @@ RenderFrameHostManager::GetSiteInstanceForNavigationRequest( request.common_params().url)); no_renderer_swap_allowed |= request.from_begin_navigation() && !can_renderer_initiate_transfer; @@ -36,49 +37,17 @@ index 872e4609c94f1e052d623ae57c1279c72eb2c3f4..ce6ea215b841d381477258417edaef4a + request.state() == NavigationRequest::FAILED) && + !speculative_render_frame_host_; + // Gives user a chance to choose a custom site instance. -+ SiteInstance* affinity_site_instance = nullptr; -+ scoped_refptr overriden_site_instance; -+ ContentBrowserClient::SiteInstanceForNavigationType siteInstanceType = -+ GetContentClient()->browser()->ShouldOverrideSiteInstanceForNavigation( -+ render_frame_host_.get(), browser_context, -+ request.common_params().url, has_response_started, -+ &affinity_site_instance); -+ switch (siteInstanceType) { -+ case ContentBrowserClient::SiteInstanceForNavigationType:: -+ FORCE_CANDIDATE_OR_NEW: -+ overriden_site_instance = -+ candidate_site_instance -+ ? candidate_site_instance -+ : SiteInstance::CreateForURL(browser_context, -+ request.common_params().url); -+ break; -+ case ContentBrowserClient::SiteInstanceForNavigationType::FORCE_CURRENT: -+ overriden_site_instance = render_frame_host_->GetSiteInstance(); -+ break; -+ case ContentBrowserClient::SiteInstanceForNavigationType::FORCE_AFFINITY: -+ DCHECK(affinity_site_instance); -+ overriden_site_instance = -+ scoped_refptr(affinity_site_instance); -+ break; -+ case ContentBrowserClient::SiteInstanceForNavigationType::ASK_CHROMIUM: -+ DCHECK(!affinity_site_instance); -+ break; -+ default: -+ break; -+ } -+ if (overriden_site_instance) { -+ if (siteInstanceType == -+ ContentBrowserClient::SiteInstanceForNavigationType:: -+ FORCE_CANDIDATE_OR_NEW) { -+ GetContentClient()->browser()->RegisterPendingSiteInstance( -+ render_frame_host_.get(), overriden_site_instance.get()); -+ } -+ return overriden_site_instance; -+ } ++ SiteInstance* client_custom_instance = nullptr; ++ GetContentClient()->browser()->OverrideSiteInstanceForNavigation( ++ render_frame_host_.get(), browser_context, request.common_params().url, ++ has_response_started, candidate_site_instance.get(), ++ &client_custom_instance); ++ if (client_custom_instance) ++ return scoped_refptr(client_custom_instance); } else { // Subframe navigations will use the current renderer, unless specifically // allowed to swap processes. -@@ -1990,23 +2046,17 @@ RenderFrameHostManager::GetSiteInstanceForNavigationRequest( +@@ -1990,18 +2015,9 @@ RenderFrameHostManager::GetSiteInstanceForNavigationRequest( if (no_renderer_swap_allowed) return scoped_refptr(current_site_instance); @@ -98,72 +67,22 @@ index 872e4609c94f1e052d623ae57c1279c72eb2c3f4..ce6ea215b841d381477258417edaef4a request.common_params().transition, request.state() == NavigationRequest::FAILED, request.restore_type() != RestoreType::NONE, request.is_view_source(), - was_server_redirect); - -+ GetContentClient()->browser()->RegisterPendingSiteInstance( -+ render_frame_host_.get(), dest_site_instance.get()); -+ - return dest_site_instance; - } - -diff --git a/content/public/browser/content_browser_client.cc b/content/public/browser/content_browser_client.cc -index bb54b89bef5c6f32e7b4a056336c85494e2a04de..2d0633f38ddfc0fa1999674903b1d5e8952e22d5 100644 ---- a/content/public/browser/content_browser_client.cc -+++ b/content/public/browser/content_browser_client.cc -@@ -47,6 +47,16 @@ void OverrideOnBindInterface(const service_manager::BindSourceInfo& remote_info, - handle); - } - -+ContentBrowserClient::SiteInstanceForNavigationType -+ContentBrowserClient::ShouldOverrideSiteInstanceForNavigation( -+ RenderFrameHost* render_frame_host, -+ BrowserContext* browser_context, -+ const GURL& dest_url, -+ bool has_response_started, -+ SiteInstance** affinity_instance) const { -+ return SiteInstanceForNavigationType::ASK_CHROMIUM; -+} -+ - BrowserMainParts* ContentBrowserClient::CreateBrowserMainParts( - const MainFunctionParams& parameters) { - return nullptr; diff --git a/content/public/browser/content_browser_client.h b/content/public/browser/content_browser_client.h -index 3be31602689cb93b965729cc4e35cf6d23a8ec2f..0554de92d81e54367e6f430bfe0c93e3a486ea66 100644 +index 3be31602689cb93b965729cc4e35cf6d23a8ec2f..2c22cb1cfe0dddc97c00e5f4ff89de6b18bc232f 100644 --- a/content/public/browser/content_browser_client.h +++ b/content/public/browser/content_browser_client.h -@@ -194,8 +194,36 @@ CONTENT_EXPORT void OverrideOnBindInterface( - // the observer interfaces.) - class CONTENT_EXPORT ContentBrowserClient { +@@ -196,6 +196,15 @@ class CONTENT_EXPORT ContentBrowserClient { public: -+ // Identifies the type of site instance to use for a navigation. -+ enum SiteInstanceForNavigationType { -+ // Use either the candidate site instance or, if it doesn't exist -+ // a new, unrelated site instance for the navigation. -+ FORCE_CANDIDATE_OR_NEW = 0, -+ -+ // Use the current site instance for the navigation. -+ FORCE_CURRENT, -+ -+ // Use the provided affinity site instance for the navigation. -+ FORCE_AFFINITY, -+ -+ // Delegate the site instance creation to Chromium. -+ ASK_CHROMIUM -+ }; virtual ~ContentBrowserClient() {} + // Electron: Allows overriding the SiteInstance when navigating. -+ virtual SiteInstanceForNavigationType ShouldOverrideSiteInstanceForNavigation( ++ virtual void OverrideSiteInstanceForNavigation( + RenderFrameHost* render_frame_host, + BrowserContext* browser_context, + const GURL& dest_url, + bool has_response_started, -+ SiteInstance** affinity_instance) const; -+ -+ // Electron: Registers a pending site instance during a navigation. -+ virtual void RegisterPendingSiteInstance( -+ content::RenderFrameHost* rfh, -+ content::SiteInstance* pending_site_instance){}; ++ SiteInstance* candidate_site_instance, ++ SiteInstance** new_instance) {} + // Allows the embedder to set any number of custom BrowserMainParts // implementations for the browser startup code. See comments in diff --git a/spec/api-browser-window-affinity-spec.js b/spec/api-browser-window-affinity-spec.js index 2108b5f0e5162..21d6d769bcfad 100644 --- a/spec/api-browser-window-affinity-spec.js +++ b/spec/api-browser-window-affinity-spec.js @@ -26,73 +26,67 @@ describe('BrowserWindow with affinity module', () => { }) } - function testAffinityProcessIds (name, webPreferences = {}) { - describe(name, () => { - let mAffinityWindow - before(done => { - createWindowWithWebPrefs({ affinity: myAffinityName, ...webPreferences }) - .then((w) => { - mAffinityWindow = w - done() - }) - }) - - after(done => { - closeWindow(mAffinityWindow, { assertSingleWindow: false }).then(() => { - mAffinityWindow = null + describe(`BrowserWindow with an affinity '${myAffinityName}'`, () => { + let mAffinityWindow + before(done => { + createWindowWithWebPrefs({ affinity: myAffinityName }) + .then((w) => { + mAffinityWindow = w done() }) - }) - - it('should have a different process id than a default window', done => { - createWindowWithWebPrefs({ ...webPreferences }) - .then(w => { - const affinityID = mAffinityWindow.webContents.getOSProcessId() - const wcID = w.webContents.getOSProcessId() + }) - expect(affinityID).to.not.equal(wcID, 'Should have different OS process IDs') - closeWindow(w, { assertSingleWindow: false }).then(() => { done() }) - }) + after(done => { + closeWindow(mAffinityWindow, { assertSingleWindow: false }).then(() => { + mAffinityWindow = null + done() }) + }) - it(`should have a different process id than a window with a different affinity '${anotherAffinityName}'`, done => { - createWindowWithWebPrefs({ affinity: anotherAffinityName, ...webPreferences }) - .then(w => { - const affinityID = mAffinityWindow.webContents.getOSProcessId() - const wcID = w.webContents.getOSProcessId() + it('should have a different process id than a default window', done => { + createWindowWithWebPrefs({}) + .then(w => { + const affinityID = mAffinityWindow.webContents.getOSProcessId() + const wcID = w.webContents.getOSProcessId() - expect(affinityID).to.not.equal(wcID, 'Should have different OS process IDs') - closeWindow(w, { assertSingleWindow: false }).then(() => { done() }) - }) - }) + expect(affinityID).to.not.equal(wcID, 'Should have different OS process IDs') + closeWindow(w, { assertSingleWindow: false }).then(() => { done() }) + }) + }) - it(`should have the same OS process id than a window with the same affinity '${myAffinityName}'`, done => { - createWindowWithWebPrefs({ affinity: myAffinityName, ...webPreferences }) - .then(w => { - const affinityID = mAffinityWindow.webContents.getOSProcessId() - const wcID = w.webContents.getOSProcessId() + it(`should have a different process id than a window with a different affinity '${anotherAffinityName}'`, done => { + createWindowWithWebPrefs({ affinity: anotherAffinityName }) + .then(w => { + const affinityID = mAffinityWindow.webContents.getOSProcessId() + const wcID = w.webContents.getOSProcessId() - expect(affinityID).to.equal(wcID, 'Should have the same OS process ID') - closeWindow(w, { assertSingleWindow: false }).then(() => { done() }) - }) - }) + expect(affinityID).to.not.equal(wcID, 'Should have different OS process IDs') + closeWindow(w, { assertSingleWindow: false }).then(() => { done() }) + }) + }) - it(`should have the same OS process id than a window with an equivalent affinity '${myAffinityNameUpper}' (case insensitive)`, done => { - createWindowWithWebPrefs({ affinity: myAffinityNameUpper, ...webPreferences }) - .then(w => { - const affinityID = mAffinityWindow.webContents.getOSProcessId() - const wcID = w.webContents.getOSProcessId() + it(`should have the same OS process id than a window with the same affinity '${myAffinityName}'`, done => { + createWindowWithWebPrefs({ affinity: myAffinityName }) + .then(w => { + const affinityID = mAffinityWindow.webContents.getOSProcessId() + const wcID = w.webContents.getOSProcessId() - expect(affinityID).to.equal(wcID, 'Should have the same OS process ID') - closeWindow(w, { assertSingleWindow: false }).then(() => { done() }) - }) - }) + expect(affinityID).to.equal(wcID, 'Should have the same OS process ID') + closeWindow(w, { assertSingleWindow: false }).then(() => { done() }) + }) }) - } - testAffinityProcessIds(`BrowserWindow with an affinity '${myAffinityName}'`) - testAffinityProcessIds(`BrowserWindow with an affinity '${myAffinityName}' and sandbox enabled`, { sandbox: true }) - testAffinityProcessIds(`BrowserWindow with an affinity '${myAffinityName}' and nativeWindowOpen enabled`, { nativeWindowOpen: true }) + it(`should have the same OS process id than a window with an equivalent affinity '${myAffinityNameUpper}' (case insensitive)`, done => { + createWindowWithWebPrefs({ affinity: myAffinityNameUpper }) + .then(w => { + const affinityID = mAffinityWindow.webContents.getOSProcessId() + const wcID = w.webContents.getOSProcessId() + + expect(affinityID).to.equal(wcID, 'Should have the same OS process ID') + closeWindow(w, { assertSingleWindow: false }).then(() => { done() }) + }) + }) + }) describe(`BrowserWindow with an affinity : nodeIntegration=false`, () => { const preload = path.join(fixtures, 'module', 'send-later.js') diff --git a/spec/api-browser-window-spec.js b/spec/api-browser-window-spec.js index 1e4bc7180d21c..b9b64537ef2ae 100644 --- a/spec/api-browser-window-spec.js +++ b/spec/api-browser-window-spec.js @@ -90,8 +90,6 @@ describe('BrowserWindow module', () => { res.end() } else if (req.url === '/navigate-302') { res.end(``) - } else if (req.url === '/cross-site') { - res.end(`

${req.url}

`) } else { res.end() } @@ -1470,6 +1468,29 @@ describe('BrowserWindow module', () => { const preload = path.join(fixtures, 'module', 'preload-sandbox.js') + // http protocol to simulate accessing another domain. This is required + // because the code paths for cross domain popups is different. + function crossDomainHandler (request, callback) { + // Disabled due to false positive in StandardJS + // eslint-disable-next-line standard/no-callback-literal + callback({ + mimeType: 'text/html', + data: `

${request.url}

` + }) + } + + before((done) => { + protocol.interceptStringProtocol('http', crossDomainHandler, () => { + done() + }) + }) + + after((done) => { + protocol.uninterceptProtocol('http', () => { + done() + }) + }) + it('exposes ipcRenderer to preload script', (done) => { ipcMain.once('answer', function (event, test) { assert.strictEqual(test, 'preload') @@ -1564,49 +1585,32 @@ describe('BrowserWindow module', () => { }) ipcRenderer.send('set-web-preferences-on-next-new-window', w.webContents.id, 'preload', preload) - w.loadFile( - path.join(fixtures, 'api', 'sandbox.html'), - { search: 'window-open-external' } - ) - - // Wait for a message from the main window saying that it's ready. - await emittedOnce(ipcMain, 'opener-loaded') - - // Ask the opener to open a popup with window.opener. - const expectedPopupUrl = - `${server.url}/cross-site` // Set in "sandbox.html". - w.webContents.send('open-the-popup', expectedPopupUrl) + w.loadFile(path.join(fixtures, 'api', 'sandbox.html'), { search: 'window-open-external' }) + const expectedPopupUrl = 'http://www.google.com/#q=electron' // Set in the "sandbox.html". // The page is going to open a popup that it won't be able to close. // We have to close it from here later. // XXX(alexeykuzmin): It will leak if the test fails too soon. const [, popupWindow] = await emittedOnce(app, 'browser-window-created') - // Ask the popup window for details. - popupWindow.webContents.send('provide-details') - const [, openerIsNull, , locationHref] = - await emittedOnce(ipcMain, 'child-loaded') - expect(openerIsNull).to.be.false('window.opener is null') + // Wait for a message from the popup's preload script. + const [, openerIsNull, html, locationHref] = await emittedOnce(ipcMain, 'child-loaded') + expect(openerIsNull).to.be.true('window.opener is not null') + expect(html).to.equal(`

${expectedPopupUrl}

`, + 'looks like a http: request has not been intercepted locally') expect(locationHref).to.equal(expectedPopupUrl) // Ask the page to access the popup. w.webContents.send('touch-the-popup') - const [, popupAccessMessage] = await emittedOnce(ipcMain, 'answer') - - // Ask the popup to access the opener. - popupWindow.webContents.send('touch-the-opener') - const [, openerAccessMessage] = await emittedOnce(ipcMain, 'answer') + const [, exceptionMessage] = await emittedOnce(ipcMain, 'answer') // We don't need the popup anymore, and its parent page can't close it, // so let's close it from here before we run any checks. await closeWindow(popupWindow, { assertSingleWindow: false }) - expect(popupAccessMessage).to.be.a('string', + expect(exceptionMessage).to.be.a('string', `child's .document is accessible from its parent window`) - expect(popupAccessMessage).to.match(/^Blocked a frame with origin/) - expect(openerAccessMessage).to.be.a('string', - `opener .document is accessible from a popup window`) - expect(openerAccessMessage).to.match(/^Blocked a frame with origin/) + expect(exceptionMessage).to.match(/^Blocked a frame with origin/) }) it('should inherit the sandbox setting in opened windows', (done) => { diff --git a/spec/chromium-spec.js b/spec/chromium-spec.js index 278be74e26273..3362b9f6b23e9 100644 --- a/spec/chromium-spec.js +++ b/spec/chromium-spec.js @@ -992,11 +992,7 @@ describe('chromium feature', () => { contents = null }) - // FIXME: Disabled with site isolation ON - // Localstorage area is accessed on the browser process - // before checking accessibility on the renderer side, - // causing illegal origin access renderer termination. - xit('cannot access localStorage', (done) => { + it('cannot access localStorage', (done) => { ipcMain.once('local-storage-response', (event, error) => { assert.strictEqual( error, diff --git a/spec/fixtures/api/sandbox.html b/spec/fixtures/api/sandbox.html index e997159d6ecb5..10263e0f57857 100644 --- a/spec/fixtures/api/sandbox.html +++ b/spec/fixtures/api/sandbox.html @@ -81,9 +81,6 @@ }, 'window-open-external': () => { addEventListener('load', () => { - ipcRenderer.once('open-the-popup', (event, url) => { - popup = open(url, '', 'top=65,left=55,width=505,height=605') - }) ipcRenderer.once('touch-the-popup', () => { let errorMessage = null try { @@ -93,7 +90,7 @@ } ipcRenderer.send('answer', errorMessage) }) - ipcRenderer.send('opener-loaded') + popup = open('http://www.google.com/#q=electron', '', 'top=65,left=55,width=505,height=605') }) }, 'verify-ipc-sender': () => { diff --git a/spec/fixtures/module/preload-sandbox.js b/spec/fixtures/module/preload-sandbox.js index 92cba0b3eb6b1..628f5c20270a7 100644 --- a/spec/fixtures/module/preload-sandbox.js +++ b/spec/fixtures/module/preload-sandbox.js @@ -22,16 +22,7 @@ } } else if (location.href !== 'about:blank') { addEventListener('DOMContentLoaded', () => { - ipcRenderer.on('touch-the-opener', () => { - let errorMessage = null - try { - const openerDoc = opener.document // eslint-disable-line no-unused-vars - } catch (error) { - errorMessage = error.message - } - ipcRenderer.send('answer', errorMessage) - }) ipcRenderer.send('child-loaded', window.opener == null, document.body.innerHTML, location.href) - }) + }, false) } })()