Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged

Conversation

Projects
None yet
7 participants
@ppontes
Copy link
Member

commented Nov 23, 2018

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

Release Notes

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

@ppontes ppontes requested a review from as a code owner Nov 23, 2018

@ppontes ppontes force-pushed the ppontes:ppontes/8100-use-proper-site-instance-candidate branch 4 times, most recently from 757c318 to 6e5a902 Nov 25, 2018

@ppontes ppontes added the wip label Nov 29, 2018

@ppontes ppontes referenced this pull request Nov 29, 2018

Merged

feat: support mixed-sandbox mode on linux #15870

2 of 5 tasks complete

@ppontes ppontes force-pushed the ppontes:ppontes/8100-use-proper-site-instance-candidate branch 3 times, most recently from 940d4f5 to e7ba370 Nov 29, 2018

@ppontes ppontes removed the wip label Nov 30, 2018

@ppontes 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 ppontes force-pushed the ppontes:ppontes/8100-use-proper-site-instance-candidate branch from e7ba370 to 0179bbd Nov 30, 2018

@ppontes ppontes requested review from alexeykuzmin, deepak1556 and nornagon Nov 30, 2018

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

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

This comment has been minimized.

Copy link
@ppontes

ppontes Nov 30, 2018

Author Member

Better name for this event?

@deepak1556
Copy link
Member

left a comment

LGTM with some minor style changes.

@@ -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();

This comment has been minimized.

Copy link
@deepak1556

deepak1556 Dec 1, 2018

Member

member variables should end with trailing _

This comment has been minimized.

Copy link
@ppontes

ppontes Dec 1, 2018

Author Member

Of course, fixed.

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());

This comment has been minimized.

Copy link
@deepak1556

deepak1556 Dec 1, 2018

Member

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.

This comment has been minimized.

Copy link
@ppontes

ppontes Dec 1, 2018

Author Member

Good point, added.

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

This comment has been minimized.

Copy link
@deepak1556

deepak1556 Dec 1, 2018

Member

variable should be snake case, navigation_was_redirected

This comment has been minimized.

Copy link
@ppontes

ppontes Dec 1, 2018

Author Member

Of course, fixed.

@deepak1556 deepak1556 requested a review from zcbenz Dec 1, 2018

@ppontes ppontes force-pushed the ppontes:ppontes/8100-use-proper-site-instance-candidate branch 2 times, most recently from bcc76d2 to 36c6c7f Dec 1, 2018

@deepak1556

This comment has been minimized.

Copy link
Member

commented Dec 3, 2018

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

ppontes added some commits Oct 17, 2018

fix: use Chromium's determined new site instance as candidate when na…
…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.

@ppontes ppontes force-pushed the ppontes:ppontes/8100-use-proper-site-instance-candidate branch from 36c6c7f to 8b4e030 Dec 4, 2018

@ppontes

This comment has been minimized.

Copy link
Member Author

commented Dec 4, 2018

@deepak1556 done!

@deepak1556
Copy link
Member

left a comment

👍

@zcbenz

zcbenz approved these changes Dec 5, 2018

Copy link
Member

left a comment

Can't find any problem, this is awesome.

@zcbenz

This comment has been minimized.

Copy link
Member

commented Dec 5, 2018

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

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

21 of 23 checks passed

ci/circleci: mas-testing-tests Your tests failed on CircleCI
Details
ci/circleci: osx-testing-tests Your tests failed on CircleCI
Details
Absolute Zero
Artifact Comparison No Changes
Details
Semantic Pull Request ready to be squashed
Details
WIP ready for review
Details
appveyor: win-ia32-testing-pr AppVeyor build succeeded
Details
appveyor: win-x64-testing-pr AppVeyor build succeeded
Details
ci/circleci: linux-arm-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-arm-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-arm64-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-arm64-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-checkout Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-ia32-testing-tests Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-debug Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-testing Your tests passed on CircleCI!
Details
ci/circleci: linux-x64-testing-tests Your tests passed on CircleCI!
Details
ci/circleci: mas-testing Your tests passed on CircleCI!
Details
ci/circleci: osx-testing Your tests passed on CircleCI!
Details
electron-lint Build #20181204.9 succeeded
Details
release-notes Release notes found
@release-clerk

This comment has been minimized.

Copy link

commented Dec 5, 2018

Release Notes Persisted

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

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

@ppontes

This comment has been minimized.

Copy link
Member Author

commented Dec 5, 2018

/trop run backport

@MarshallOfSound

This comment has been minimized.

Copy link
Member

commented Dec 5, 2018

@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

This comment has been minimized.

Copy link
Member Author

commented Dec 5, 2018

@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

This comment has been minimized.

Copy link
Member

commented Dec 5, 2018

@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

This comment has been minimized.

Copy link
Member

commented Dec 5, 2018

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

This comment has been minimized.

Copy link
Member

commented Dec 5, 2018

/trop run backport

@trop

This comment has been minimized.

Copy link
Contributor

commented Dec 5, 2018

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

@trop

This comment has been minimized.

Copy link
Contributor

commented Dec 5, 2018

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

@MarshallOfSound

This comment has been minimized.

Copy link
Member

commented Dec 5, 2018

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

@ppontes

This comment has been minimized.

Copy link
Member Author

commented Dec 5, 2018

@MarshallOfSound yep, will do. Thanks.

@ckerr

This comment has been minimized.

Copy link
Member

commented Dec 6, 2018

@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

This comment has been minimized.

Copy link
Member Author

commented Dec 6, 2018

@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

@ppontes ppontes deleted the ppontes:ppontes/8100-use-proper-site-instance-candidate branch Dec 20, 2018

@dataich

This comment has been minimized.

Copy link
Contributor

commented Feb 4, 2019

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

@ppontes

This comment has been minimized.

Copy link
Member Author

commented Feb 5, 2019

@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

This comment has been minimized.

Copy link
Contributor

commented Feb 5, 2019

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

@@ -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

This comment has been minimized.

Copy link
@alexstrat

alexstrat May 6, 2019

Contributor

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

This comment has been minimized.

Copy link
@ppontes

ppontes May 7, 2019

Author Member

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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.