fix: use appropriate site instance for cross-site nav's #15821

ppontes · 2018-11-23T23:47:37Z

Description of Change

When a window is opened with window.open to navigate to a cross-site destination, the new site instance should belong to the browsing instance of the opener window in order not to break scripting relationships between windows.

fixes #8100.

This change turns Chromium's site isolation on and depends on it being on in order to work properly. Turning on site isolation revealed the following latent problems that are / will be fixed also in this PR:

If a cross-site redirection happens during a navigation, the renderer process that will handle the final URL will be locked to the pre-redirection URL with Chromium's child process security. If the page tries to access local resources, Chromium will refuse and kill the renderer process.
Chromium now kills renderer processes that attempt to access resources for which they have no permissions. This means that the "custom non standard schemes cannot access localStorage" UT had to be adapted.

The fix also requires the following to be addressed:

If a cross-site redirection happens during a navigation, Chromium discards the speculative RFH that was created for the pre-redirection URL. This is harmless in Chromium but Electron reacts by asynchronously closing the associated BrowserWindow. User sees the new window navigating to the final URL and then being closed.

Checklist

PR description included and stakeholders cc'd
npm test passes
tests are changed or added
PR title follows semantic commit guidelines

Release Notes

Notes: Fixed a bug where window.opener of a window created with window.open from a sandboxed renderer was null.

ppontes · 2018-11-30T17:09:34Z

atom/browser/api/atom_api_web_contents.cc

+      render_view_host->GetProcess()->GetID() ==
+          currently_committed_process_id) {
+    currently_committed_process_id = -1;
+    Emit("current-render-view-deleted",


Better name for this event?

deepak1556

LGTM with some minor style changes.

deepak1556 · 2018-12-01T00:10:28Z

atom/browser/api/atom_api_web_contents.cc

@@ -781,8 +781,23 @@ void WebContents::RenderViewCreated(content::RenderViewHost* render_view_host) {
    impl->disable_hidden_ = !background_throttling_;
 }

+void WebContents::RenderViewHostChanged(content::RenderViewHost* old_host,
+                                        content::RenderViewHost* new_host) {
+  currently_committed_process_id = new_host->GetProcess()->GetID();


member variables should end with trailing _

Of course, fixed.

deepak1556 · 2018-12-01T00:13:26Z

atom/browser/api/atom_api_web_contents.cc

+                                        content::RenderViewHost* new_host) {
+  currently_committed_process_id = new_host->GetProcess()->GetID();
+}
+
 void WebContents::RenderViewDeleted(content::RenderViewHost* render_view_host) {
  Emit("render-view-deleted", render_view_host->GetProcess()->GetID());


A little comment stating that this event is necessary for tracking any states with respect to intermediate render view hosts aka speculative render view hosts. Currently used by object-registry.js to ref count remote objects.

Good point, added.

deepak1556 · 2018-12-01T00:17:47Z

atom/browser/atom_browser_client.cc

+    content::SiteInstance* speculative_instance,
+    const GURL& dest_url,
+    bool has_response_started) const {
+  bool navigationWasRedirected = false;


variable should be snake case, navigation_was_redirected

Of course, fixed.

deepak1556 · 2018-12-03T19:17:10Z

@ppontes can you rebase on latest master, it should be good to go. Thanks!

…vigating. When navigating to a new address, consider using Chromium's determined site instance for the new page as it should belong to an existing browsing instance when the navigation was triggered by window.open(). fixes 8100.

… when navigating." This reverts commit eb95f93.

… webContents.

deepak1556

👍

zcbenz

Can't find any problem, this is awesome.

zcbenz · 2018-12-05T08:03:32Z

Failing tests are the usual flaky ones, I'm merging with them ignored.

release-clerk · 2018-12-05T08:03:42Z

Release Notes Persisted

Fixed a bug where window.opener of a window created with window.open from a sandboxed renderer was null.

ppontes · 2018-12-05T11:05:53Z

/trop run backport

MarshallOfSound · 2018-12-05T11:09:57Z

@ppontes You need to be on an approved user list to run trop commands.

4.x is nearing the end of its beta cycle, the previous iteration of this change broke the release. How confident are we this won't destabilize 4.0?

ppontes · 2018-12-05T11:28:09Z

@MarshallOfSound Thanks for the warning, I suspected that but since the trop docs are outdated I still gave it a try.
I'm reasonably confident as we solved the issues found meanwhile and added UT's to verify them. Of course there's always some risk associated with turning on site isolation. Adding @deepak1556 for input.

deepak1556 · 2018-12-05T12:06:55Z

@MarshallOfSound I would like to see this backported to 4.x line, and get an early feedback on using site isolation. There is only so much we can test here wrt navigations. Also this PR fixes some really troublesome usages of site instances. The only reason the last PR got reverted was because it compromised a common navigation code path.

MarshallOfSound · 2018-12-05T12:12:18Z

Thanks @ppontes and @deepak1556

I'm tentatively going to land this ready for the next 4.x release but cc'ing @sofianguy to give a heads up to the AFP folks that this change is landing. Basically any weird navigation or window.open behavior in the next 4.x release is probably coming from this PR, although we don't expect anything bad to happen and we've tested the common paths as well as we can without sending it through apps.

We can always revert before the next 4.x release if someone finds something.

MarshallOfSound · 2018-12-05T12:12:25Z

/trop run backport

trop · 2018-12-05T12:12:28Z

The backport process for this PR has been manually initiated, here we go! :D

trop · 2018-12-05T12:12:52Z

I was unable to backport this PR to "4-0-x" cleanly;
you will need to perform this backport manually.

MarshallOfSound · 2018-12-05T12:13:36Z

cc @ppontes looks like the patch doesn't apply cleanly, can you take care of the manual backport?

ppontes · 2018-12-05T13:40:05Z

@MarshallOfSound yep, will do. Thanks.

ckerr · 2018-12-06T01:18:15Z

@ppontes, do you have an estimate on when you can do this?

I'm wondering if we should try to get this into a beta release later this week so that there will more public testing time leading up to 4.0.0

ppontes · 2018-12-06T10:08:55Z

@ckerr sorry for the delay, got some surprising UT failures as a setting failed to reach brightray in 4-0-x, as brightray is gone from master. Everything's fine now and the PR is ready: #15969

dataich · 2019-02-04T15:08:19Z

Do you have plan to release this fix on 4.x?

ppontes · 2019-02-05T15:12:56Z

@dataich , this is a considerable change that got ready just as 4.x was about to be released, so unfortunately we decided not to include it in 4.x after all. It is available from 5.x.

dataich · 2019-02-05T16:23:26Z

@ppontes thanks for your reply. I wait stable 5.x release😢

alexstrat · 2019-05-06T16:28:35Z

spec/api-browser-window-spec.js

@@ -1470,29 +1472,6 @@ describe('BrowserWindow module', () => {

      const preload = path.join(fixtures, 'module', 'preload-sandbox.js')

-      // http protocol to simulate accessing another domain. This is required


@ppontes is there any reason why you removed the usage of interceptStringProtocol? I'm considering using this again for #18173

Unfortunately, I don't remember the reason. Feel free to try to use it again according to your needs.

tarruda · 2020-04-07T10:11:00Z

lib/browser/guest-window-manager.js

  }
-  embedder.once('render-view-deleted', closedByEmbedder)
+  embedder.once('current-render-view-deleted', closedByEmbedder)


Are we sure that "RenderViewDeleted" implies that the window should be closed?

Our Electron app is failing to use Google Sign-in, with a similar behavior to what is described here, and I've traced the issue to the current-render-view-deleted event being emitted before the sign-in is ready causing it to close prematurely.

For our app, the current-render-view-deleted event is only emitted in this situation, so it was safe to simply comment this line. Do you know what are the general implications of removing this handler?

Did you understand what causes RenderViewDeleted to be fired in the first place?

@deepak1556 can confirm / correct me, I believe we need to depend on this event to cleanup our BrowserWindow stuff when we assume that something other than the user is triggering the window to close. So it'd be useful to understand why something's triggering the RenderView to be deleted while Google Sign-in still depends on the popup window being open.

FWIW @tarruda, Google is shutting down sign-in in non-browsers: https://security.googleblog.com/2019/04/better-protection-against-man-in-middle.html

It's quite possible there's a separate issue here, but for Google sign-in specifically, you should probably see if you can switch to the OAuth flow.

Did you understand what causes RenderViewDeleted to be fired in the first place?

Not certain, but it seemed to happen when the Google SSO window navigated to another URL (where it normally shows "please wait..." before closing the window).

Also, this didn't always happen: When I tried to sign in by opening our app login window directly with a BrowserWindow, the issue was not reproducible. It only happened during normal app usage, when the login window was opened with window.open.

FWIW @tarruda, Google is shutting down sign-in in non-browsers: https://security.googleblog.com/2019/04/better-protection-against-man-in-middle.html

It's quite possible there's a separate issue here, but for Google sign-in specifically, you should probably see if you can switch to the OAuth flow.

@nornagon I'm not familiar with the details of how theh Google sign-in works, but I assume we already use the OAuth flow since that announcement is already one year old and everything was working fine for the past couple of months.

Not necessarily; Google has been rolling it out on a percentage basis per-account since a few months ago. OAuth flow means you do sign-in in the browser and get a token back in your app.

Not necessarily; Google has been rolling it out on a percentage basis per-account since a few months ago. OAuth flow means you do sign-in in the browser and get a token back in your app.

Hmm now I remember having to change user-agent in order for Google Sign-in to work, that must have been the reason :)

Right, that seems to work for some people for now, but Google is pretty committed to preventing people from doing what you're doing, so I wouldn't be at all surprised if they break it in other, more difficult-to-work-around ways without warning in the future.

bughit · 2022-04-01T18:37:08Z

This change turns Chromium's site isolation on and depends on it being on in order to work properly.

Could someone please clarify whether the above means that disabling site isolation via --disable-site-isolation-trials is unsupported?

ppontes requested a review from a team November 23, 2018 23:47

ppontes added the wip ⚒ label Nov 29, 2018

ppontes mentioned this pull request Nov 29, 2018

feat: support mixed-sandbox mode on linux #15870

Merged

5 tasks

ppontes removed the wip ⚒ label Nov 30, 2018

ppontes changed the title ~~[WIP] fix: use appropriate site instance for cross-site nav's~~ fix: use appropriate site instance for cross-site nav's Nov 30, 2018

ppontes requested review from alexeykuzmin, deepak1556 and nornagon November 30, 2018 13:31

ppontes added wip ⚒ and removed wip ⚒ labels Nov 30, 2018

ppontes commented Nov 30, 2018

View reviewed changes

deepak1556 requested changes Dec 1, 2018

View reviewed changes

deepak1556 requested a review from zcbenz December 1, 2018 00:34

ppontes added 16 commits December 4, 2018 09:13

Revert "fix: use Chromium's determined new site instance as candidate…

7530af6

… when navigating." This reverts commit eb95f93.

fix: delegate site instance creation back to content when sandboxed.

6e03731

fix: ensure site isolation is on

694e51b

test: adapt ut for cross-site navigation

bd857e9

fix: register pending processes during a navigation.

e799fe0

refactor: dont call loadURL for a window constructed from an existing…

9271a39

… webContents.

test: add sandboxed affinity UT's.

5aae695

fix: check affinity before deciding if to force a new site instance.

2225c0b

chore: adapt subsequent patch.

6d02359

refactor: constify logically const methods.

f85ad5c

fix: do not reuse site instances when navigation redirects cross-site.

220b722

test: ensure localStorage accessible after x-site redirect.

f4e3036

test: adapt localStorage acess denied UT for site isolation.

c30f003

fix: do not send render-view-deleted for speculative frames.

d8d16a5

chore: amend tests after rebase.

9829b19

deepak1556 approved these changes Dec 4, 2018

View reviewed changes

zcbenz approved these changes Dec 5, 2018

View reviewed changes

zcbenz merged commit d5d1fa8 into electron:master Dec 5, 2018

ppontes added the target/4-0-x label Dec 5, 2018

ppontes deleted the ppontes/8100-use-proper-site-instance-candidate branch December 20, 2018 13:05

This was referenced Apr 29, 2019

window.opener opened from webview is null after cross site navigation #18032

Closed

Electron 5 webview with no contextIsolation and nativeWindowOpen fails to load a preload in a popup window in the correct context #17989

Closed

alexstrat mentioned this pull request May 6, 2019

fix: lost window.opener after cross-origin navigation #18173

Merged

5 tasks

alexstrat reviewed May 6, 2019

View reviewed changes

miniak mentioned this pull request Jun 7, 2019

feat: sandbox renderer processes for cross-origin frames #18650

Merged

6 tasks

tarruda reviewed Apr 7, 2020

View reviewed changes

zcbenz mentioned this pull request Aug 23, 2020

fix: window.open doesn't work correctly in child window #25080

Merged

4 tasks

		@@ -1470,29 +1472,6 @@ describe('BrowserWindow module', () => {

		const preload = path.join(fixtures, 'module', 'preload-sandbox.js')

		// http protocol to simulate accessing another domain. This is required

fix: use appropriate site instance for cross-site nav's #15821

fix: use appropriate site instance for cross-site nav's #15821

Conversation

ppontes commented Nov 23, 2018 • edited

Description of Change

Checklist

Release Notes

Choose a reason for hiding this comment

deepak1556 left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

deepak1556 commented Dec 3, 2018

deepak1556 left a comment

Choose a reason for hiding this comment

zcbenz left a comment

Choose a reason for hiding this comment

zcbenz commented Dec 5, 2018

release-clerk bot commented Dec 5, 2018

ppontes commented Dec 5, 2018

MarshallOfSound commented Dec 5, 2018

ppontes commented Dec 5, 2018

deepak1556 commented Dec 5, 2018

MarshallOfSound commented Dec 5, 2018

MarshallOfSound commented Dec 5, 2018

trop bot commented Dec 5, 2018

trop bot commented Dec 5, 2018

MarshallOfSound commented Dec 5, 2018

ppontes commented Dec 5, 2018

ckerr commented Dec 6, 2018

ppontes commented Dec 6, 2018 • edited

dataich commented Feb 4, 2019

ppontes commented Feb 5, 2019

dataich commented Feb 5, 2019

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bughit commented Apr 1, 2022

ppontes commented Nov 23, 2018 •

edited

ppontes commented Dec 6, 2018 •

edited