Skip to content

Commit

Permalink
Make Blink compatible with url::Origin::Resolve(...'about:blank'...).
Browse files Browse the repository at this point in the history
Main change: blink::SecurityOrigin::CreateWithReferenceOrigin
=============================================================

The main change in this CL is changing how blink::SecurityOrigin's
CreateWithReferenceOrigin handles "about:blank" URLs.  Before this CL,
an opaque origin would be returned;  after this CL, the reference origin
is returned.

This change is desirable, because:

1. It makes blink::SecurityOrigin::CreateWithReferenceOrigin
   compatible with url::Origin::Resolve.  This lets us copy additional
   test assertions from OriginTest.ConstructFromGURL testcase in
   //url/origin_unittest.cc (url::Origin-specific tests) to
   AbstractOriginTest::VerifyOriginInvariants in
   //url/origin_abstract_tests.h (tests shared across
   blink::SecurityOrigin and url::Origin)

2. It causes "about:blank" to commit with the correct origin (based on
   the origin of the initiator of the navigation) when the
   `owner_document` is missing (e.g. when the parent is a remote frame,
   see also a follow-up CL: https://crrev.com/c/2639656).  This lets us
   have mostly correct test expectations in the new
   NavigationBrowserTest*ToAboutBlank* tests added in this CL, as well
   as fix test expectations in some already existing tests:
   - ChromeNavigationBrowserTest.
     NavigationInitiatedByCrossSiteSubframeRedirectedToAboutBlank
   - ExtensionApiTest.TabsUpdate_WebToAboutBlank

   - MultiOriginSessionRestoreTest.BackToAboutBlank1
   - MultiOriginSessionRestoreTest.BackToAboutBlank2
   - TabRestoreTest.BackToAboutBlank

   - SitePerProcessBrowserTest.SandboxFlagsNotInheritedBeforeNavigation
   - SitePerProcessBrowserTest.SubframeBlankUrlsAfterRestore


Supplementary changes
=====================

To make sure that the new origin of "about:blank" remains compatible
with the process lock, some additional changes need to be made:

- ExtensionApiTabTest.Tabs2 navigates to about:blank in noopener mode.
  We are preserving the initiator_origin (if url were http then the
  initiator would control SameSite cookies, etc).  But if we preserve
  the initiator origin then "about:blank" needs to commit with the same
  origin as the initiator.  And if the initiator origin requires process
  locks, then we can’t commit in an unlocked process.  Therefore we need
  to call NavigationRequest::SetSourceSiteInstanceToInitiatorIfNeeded
  for non-history navigations in no-opener mode.

- ExtensionApiTabTest.HostPermission hits a browsing-instance-swap case
  in DetermineSiteInstanceForURL (swap forced because of going from
  WebUI to non-WebUI: current_effective_url = chrome://new-tab-page/;
  destination_effective_url = about:blank).  This forces the CL to
  modify `force_browsing_instance_swap` case in
  DetermineSiteInstanceForURL to use `source_instance` if the
  `source_instance` already comes from a different browsing instance.
  Without this tweak, about:blank would commit in an unlocked site
  instance and fail with CANNOT_COMMIT_ORIGIN url 'about:blank' origin
  'chrome-extension://...'.


Summary of new tests added in this CL
=====================================

- NavigationBrowserTest.GrandchildToAboutBlank_ABA_SameSite
  (no process swaps, parent of the navigated frame is a local frame)

- NavigationBrowserTest.GrandchildToAboutBlank_ABA_CrossSite
  (no process swaps, parent of the navigated frame is a remote frame)

- NavigationBrowserTest.GrandchildToAboutBlank_ABB_CrossSite
  (process swap required, initially parent of the navigated frame is a
  local frame, when committing parent of the navigated frame is a remote
  frame)

- NavigationBrowserTest.TopToAboutBlank_CrossSite
  (the initiator of the navigation is destroyed as the result of the
  navigation)

- SameSiteSiblingToAboutBlank_CrossSiteTop
  (initiator of the navigation is not an ancestor nor a child of the
  navigated frame; the initiator has to be same-origin as the target
  frame - a cross-origin initiator would be blocked from navigating the
  target frame)

Bug: 585649
Change-Id: Ibc4a25a3af96e65b4d7759a09c3cc47d6fb51356
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2637187
Reviewed-by: Tom Sepez <tsepez@chromium.org>
Reviewed-by: Avi Drissman <avi@chromium.org>
Reviewed-by: Nasko Oskov <nasko@chromium.org>
Reviewed-by: Daniel Cheng <dcheng@chromium.org>
Commit-Queue: Łukasz Anforowicz <lukasza@chromium.org>
Cr-Commit-Position: refs/heads/master@{#847405}
  • Loading branch information
anforowicz authored and Chromium LUCI CQ committed Jan 27, 2021
1 parent 4289f0c commit e9db265
Show file tree
Hide file tree
Showing 12 changed files with 372 additions and 74 deletions.
19 changes: 2 additions & 17 deletions chrome/browser/chrome_navigation_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -793,11 +793,13 @@ IN_PROC_BROWSER_TEST_F(
// this navigation to an about:blank URL.
//
// This step would have hit the CHECK from https://crbug.com/1026738.
url::Origin cross_site_origin = cross_site_subframe->GetLastCommittedOrigin();
content::TestNavigationObserver nav_observer(popup, 1);
ASSERT_TRUE(ExecJs(cross_site_subframe,
content::JsReplace("top.location = $1", kRedirectedUrl)));
nav_observer.Wait();
EXPECT_EQ(url::kAboutBlankURL, popup->GetLastCommittedURL());
EXPECT_EQ(cross_site_origin, popup->GetMainFrame()->GetLastCommittedOrigin());

// 5. Verify that the about:blank URL is hosted in the same process
// as the navigation initiator (and separate from the opener and the old
Expand All @@ -819,23 +821,6 @@ IN_PROC_BROWSER_TEST_F(
EXPECT_NE(url::kDataScheme,
popup->GetSiteInstance()->GetSiteURL().scheme());
}

// 6. Verify the origin of the about:blank URL in the popup.
//
// Blink calculates the origin of an about:blank frame based on the
// opener-or-parent (rather than based on the navigation initiator's origin
// as required by the spec).
// TODO(lukasza): https://crbug.com/585649: Once Blink is fixed, adjust test
// expectations below to make sure the initiator's origin has been committed.
// Consider also adding verification that the about:blank page can be scripted
// by other frames with the initiator's origin.
if (content::AreAllSitesIsolatedForTesting()) {
// If the opener is a blink::RemoteFrame, then Blink uses an opaque origin.
EXPECT_TRUE(popup->GetMainFrame()->GetLastCommittedOrigin().opaque());
} else {
EXPECT_EQ(url::Origin::Create(kOpenerUrl),
popup->GetMainFrame()->GetLastCommittedOrigin());
}
}

// This test covers a navigation that:
Expand Down
13 changes: 3 additions & 10 deletions chrome/browser/extensions/api/tabs/tabs_test.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2397,16 +2397,9 @@ IN_PROC_BROWSER_TEST_F(ExtensionApiTest, TabsUpdate_WebToAboutBlank) {
EXPECT_EQ(about_blank_url, test_frame->GetLastCommittedURL());
EXPECT_EQ(extension_contents->GetMainFrame()->GetProcess(),
test_contents->GetMainFrame()->GetProcess());

// The expectations below preserve the behavior at r704251. It is not clear
// whether these are the right expectations - maybe about:blank should commit
// with an extension origin? OTOH, committing with the extension origin
// wouldn't be possible when targeting an incognito window (see also
// IncognitoApiTest.Incognito test).
EXPECT_TRUE(test_frame->GetLastCommittedOrigin().opaque());
EXPECT_EQ(
extension_origin.GetTupleOrPrecursorTupleIfOpaque(),
test_frame->GetLastCommittedOrigin().GetTupleOrPrecursorTupleIfOpaque());
// Note that committing with the extension origin wouldn't be possible when
// targeting an incognito window (see also IncognitoApiTest.Incognito test).
EXPECT_EQ(extension_origin, test_frame->GetLastCommittedOrigin());
}

// Tests updating a URL of a web tab to an about:newtab. Verify that the new
Expand Down
18 changes: 6 additions & 12 deletions chrome/browser/sessions/session_restore_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -2292,10 +2292,8 @@ IN_PROC_BROWSER_TEST_F(MultiOriginSessionRestoreTest, BackToAboutBlank1) {
}
EXPECT_EQ(GURL(url::kAboutBlankURL),
new_popup->GetMainFrame()->GetLastCommittedURL());
// TODO(lukasza): https://crbug.com/888079: The browser process should tell
// the renderer which (initiator-based) origin to commit. Right now, Blink
// just falls back to an opaque origin.
EXPECT_TRUE(new_popup->GetMainFrame()->GetLastCommittedOrigin().opaque());
EXPECT_EQ(initial_origin,
new_popup->GetMainFrame()->GetLastCommittedOrigin());
}

// Test that it is possible to navigate back to a restored about:blank history
Expand Down Expand Up @@ -2380,10 +2378,8 @@ IN_PROC_BROWSER_TEST_F(MultiOriginSessionRestoreTest, BackToAboutBlank2) {
// Verify that the restored popup hosts about:blank#foo.
EXPECT_EQ(GURL("about:blank#foo"),
new_popup->GetMainFrame()->GetLastCommittedURL());
// TODO(lukasza): https://crbug.com/888079: The browser process should tell
// the renderer which (initiator-based) origin to commit. Right now, Blink
// just falls back to an opaque origin.
EXPECT_TRUE(new_popup->GetMainFrame()->GetLastCommittedOrigin().opaque());
EXPECT_EQ(initial_origin,
new_popup->GetMainFrame()->GetLastCommittedOrigin());

// Navigate the popup to another site.
GURL other_url = embedded_test_server()->GetURL("bar.com", "/title1.html");
Expand All @@ -2406,10 +2402,8 @@ IN_PROC_BROWSER_TEST_F(MultiOriginSessionRestoreTest, BackToAboutBlank2) {
}
EXPECT_EQ(GURL("about:blank#foo"),
new_popup->GetMainFrame()->GetLastCommittedURL());
// TODO(lukasza): https://crbug.com/888079: The browser process should tell
// the renderer which (initiator-based) origin to commit. Right now, Blink
// just falls back to an opaque origin.
EXPECT_TRUE(new_popup->GetMainFrame()->GetLastCommittedOrigin().opaque());
EXPECT_EQ(initial_origin,
new_popup->GetMainFrame()->GetLastCommittedOrigin());
}

// Test that it is possible to navigate back to a subframe with a restored
Expand Down
6 changes: 2 additions & 4 deletions chrome/browser/sessions/tab_restore_browsertest.cc
Original file line number Diff line number Diff line change
Expand Up @@ -1501,8 +1501,6 @@ IN_PROC_BROWSER_TEST_F(TabRestoreTest, BackToAboutBlank) {
GoBack(browser());
EXPECT_EQ(GURL(url::kAboutBlankURL),
new_popup->GetMainFrame()->GetLastCommittedURL());
// TODO(lukasza): https://crbug.com/888079: The browser process should tell
// the renderer which (initiator-based) origin to commit. Right now, Blink
// just falls back to an opaque origin.
EXPECT_TRUE(new_popup->GetMainFrame()->GetLastCommittedOrigin().opaque());
EXPECT_EQ(initial_origin,
new_popup->GetMainFrame()->GetLastCommittedOrigin());
}
Loading

0 comments on commit e9db265

Please sign in to comment.