[Backport] Do not use NavigationEntry to block history navigations.

This is no longer necessary after r477371.

BUG=777419
TEST=See bug for repro steps.
TBR=creis@chromium.org

(cherry picked from commit 56a84aa67bb071a33a48ac1481b555c48e0a9a59)

Cq-Include-Trybots: master.tryserver.chromium.linux:linux_site_isolation
Reviewed-on: https://chromium-review.googlesource.com/733959
Reviewed-on: https://chromium-review.googlesource.com/751765
(CVE-2017-777419)

Change-Id: I8852101950d5e9911a058307d4f612a4960769bc
Reviewed-by: Michael Brüning <michael.bruning@qt.io>

5 files changed, 103 insertions, 67 deletions

diff --git a/chromium/content/browser/browser_side_navigation_browsertest.cc b/chromium/content/browser/browser_side_navigation_browsertest.cc
index a33b095a604..9738e78a5dc 100644
--- a/chromium/content/browser/browser_side_navigation_browsertest.cc
+++ b/chromium/content/browser/browser_side_navigation_browsertest.cc
@@ -392,18 +392,15 @@ IN_PROC_BROWSER_TEST_F(BrowserSideNavigationBrowserTest,
   EXPECT_TRUE(ExecuteScript(shell(), "location.reload()"));
   reload_observer.Wait();
 
-  // The expectation is that about:blank was loaded and the virtual URL is set
-  // to the URL that was blocked.
-  //
-  // TODO(nasko): Now that the error commits on the previous URL, the blocked
-  // navigation logic is no longer needed. https://crbug.com/723796
+  // The expectation is that the blocked URL is present in the NavigationEntry,
+  // and shows up in both GetURL and GetVirtualURL.
   EXPECT_EQ(1, controller.GetLastCommittedEntryIndex());
   EXPECT_FALSE(
       controller.GetLastCommittedEntry()->GetURL().SchemeIs(url::kDataScheme));
   EXPECT_EQ(redirect_to_blank_url,
+            controller.GetLastCommittedEntry()->GetURL());
+  EXPECT_EQ(redirect_to_blank_url,
             controller.GetLastCommittedEntry()->GetVirtualURL());
-  EXPECT_EQ(url::kAboutBlankURL,
-            controller.GetLastCommittedEntry()->GetURL().spec());
 }
 
 class BrowserSideNavigationBrowserDisableWebSecurityTest
diff --git a/chromium/content/browser/frame_host/data_url_navigation_browsertest.cc b/chromium/content/browser/frame_host/data_url_navigation_browsertest.cc
index 7d48a946e72..036e7910b20 100644
--- a/chromium/content/browser/frame_host/data_url_navigation_browsertest.cc
+++ b/chromium/content/browser/frame_host/data_url_navigation_browsertest.cc
@@ -976,19 +976,17 @@ IN_PROC_BROWSER_TEST_F(DataUrlNavigationBrowserTest,
   WaitForLoadStop(new_shell->web_contents());
 
   // The window.open() should have resulted in an error page. The blocked
-  // URL should be in the virtual URL, not the actual URL.
-  //
-  // TODO(nasko): Now that the error commits on the previous URL, the blocked
-  // navigation logic is no longer needed. https://crbug.com/723796
+  // URL should be in both the actual and the virtual URL.
   {
     EXPECT_EQ(0, controller->GetLastCommittedEntryIndex());
     NavigationEntry* entry = controller->GetLastCommittedEntry();
     EXPECT_EQ(PAGE_TYPE_ERROR, entry->GetPageType());
     EXPECT_FALSE(entry->GetURL().SchemeIs(url::kDataScheme));
     EXPECT_TRUE(base::StartsWith(
-        entry->GetVirtualURL().spec(),
+        entry->GetURL().spec(),
         embedded_test_server()->GetURL("/server-redirect?").spec(),
         base::CompareCase::SENSITIVE));
+    EXPECT_EQ(entry->GetURL(), entry->GetVirtualURL());
   }
 
   // Navigate forward and then go back to ensure the navigation to data: URL
@@ -1006,10 +1004,10 @@ IN_PROC_BROWSER_TEST_F(DataUrlNavigationBrowserTest,
     EXPECT_EQ(0, controller->GetLastCommittedEntryIndex());
     EXPECT_FALSE(entry->GetURL().SchemeIs(url::kDataScheme));
     EXPECT_TRUE(base::StartsWith(
-        entry->GetVirtualURL().spec(),
+        entry->GetURL().spec(),
         embedded_test_server()->GetURL("/server-redirect?").spec(),
         base::CompareCase::SENSITIVE));
-    EXPECT_EQ(url::kAboutBlankURL, entry->GetURL().spec());
+    EXPECT_EQ(entry->GetURL(), entry->GetVirtualURL());
   }
 
   // Do another new navigation, but then use JavaScript to navigate back,
@@ -1026,10 +1024,10 @@ IN_PROC_BROWSER_TEST_F(DataUrlNavigationBrowserTest,
     EXPECT_EQ(0, controller->GetLastCommittedEntryIndex());
     EXPECT_FALSE(entry->GetURL().SchemeIs(url::kDataScheme));
     EXPECT_TRUE(base::StartsWith(
-        entry->GetVirtualURL().spec(),
+        entry->GetURL().spec(),
         embedded_test_server()->GetURL("/server-redirect?").spec(),
         base::CompareCase::SENSITIVE));
-    EXPECT_EQ(url::kAboutBlankURL, entry->GetURL().spec());
+    EXPECT_EQ(entry->GetURL(), entry->GetVirtualURL());
   }
 }
 
diff --git a/chromium/content/browser/frame_host/navigation_controller_impl.cc b/chromium/content/browser/frame_host/navigation_controller_impl.cc
index 4eb65b14dd4..708169c65e7 100644
--- a/chromium/content/browser/frame_host/navigation_controller_impl.cc
+++ b/chromium/content/browser/frame_host/navigation_controller_impl.cc
@@ -162,26 +162,6 @@ bool ShouldTreatNavigationAsReload(const NavigationEntry* entry) {
   return false;
 }
 
-// Returns true if the error code indicates an error condition that is not
-// recoverable or navigation is blocked. In such cases, session history
-// navigations to the same NavigationEntry should not attempt to load the
-// original URL.
-// TODO(nasko): Find a better way to distinguish blocked vs failed navigations,
-// as this is a very hacky way of accomplishing this. For now, a handful of
-// error codes are considered, which are more or less known to be cases of
-// blocked navigations.
-bool IsBlockedNavigation(net::Error error_code) {
-  switch (error_code) {
-    case net::ERR_BLOCKED_BY_CLIENT:
-    case net::ERR_BLOCKED_BY_RESPONSE:
-    case net::ERR_BLOCKED_BY_XSS_AUDITOR:
-    case net::ERR_UNSAFE_REDIRECT:
-      return true;
-    default:
-      return false;
-  }
-}
-
 }  // namespace
 
 // NavigationControllerImpl ----------------------------------------------------
@@ -953,24 +933,6 @@ bool NavigationControllerImpl::RendererDidNavigate(
     frame_entry->set_redirect_chain(params.redirects);
   }
 
-  // When a navigation in the main frame is blocked, it will display an error
-  // page. To avoid loading the blocked URL on back/forward navigations,
-  // do not store it in the FrameNavigationEntry's URL or PageState. Instead,
-  // make it visible to the user as the virtual URL. Store a safe URL
-  // (about:blank) as the one to load if the entry is revisited.
-  // TODO(nasko): Consider supporting similar behavior for subframe
-  // navigations, including AUTO_SUBFRAME.
-  if (!rfh->GetParent() &&
-      IsBlockedNavigation(navigation_handle->GetNetErrorCode())) {
-    DCHECK(params.url_is_unreachable);
-    active_entry->SetURL(GURL(url::kAboutBlankURL));
-    active_entry->SetVirtualURL(params.url);
-    if (frame_entry) {
-      frame_entry->SetPageState(
-          PageState::CreateFromURL(active_entry->GetURL()));
-    }
-  }
-
   // Use histogram to track memory impact of redirect chain because it's now
   // not cleared for committed entries.
   size_t redirect_chain_size = 0;
diff --git a/chromium/content/browser/frame_host/navigation_controller_impl_browsertest.cc b/chromium/content/browser/frame_host/navigation_controller_impl_browsertest.cc
index 292bb226b76..7c2de868092 100644
--- a/chromium/content/browser/frame_host/navigation_controller_impl_browsertest.cc
+++ b/chromium/content/browser/frame_host/navigation_controller_impl_browsertest.cc
@@ -6855,18 +6855,79 @@ IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
   controller.GoBack();
   back_load_observer.Wait();
 
-  // The expectation is that about:blank was loaded and the virtual URL is set
-  // to the URL that was blocked.
-  //
-  // TODO(nasko): Now that the error commits on the previous URL, the blocked
-  // navigation logic is no longer needed. https://crbug.com/723796
+  // The expectation is that the blocked URL is present in the NavigationEntry,
+  // and shows up in both GetURL and GetVirtualURL.
   EXPECT_EQ(1, controller.GetLastCommittedEntryIndex());
   EXPECT_FALSE(
       controller.GetLastCommittedEntry()->GetURL().SchemeIs(url::kDataScheme));
   EXPECT_EQ(redirect_to_unsafe_url,
+            controller.GetLastCommittedEntry()->GetURL());
+  EXPECT_EQ(redirect_to_unsafe_url,
             controller.GetLastCommittedEntry()->GetVirtualURL());
-  EXPECT_EQ(url::kAboutBlankURL,
-            controller.GetLastCommittedEntry()->GetURL().spec());
+}
+
+// Verifies that redirecting to a blocked URL and going back does not allow a
+// URL spoof.  See https://crbug.com/777419.
+IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
+                       PreventSpoofFromBlockedRedirect) {
+  GURL url1 = embedded_test_server()->GetURL(
+      "a.com", "/navigation_controller/simple_page_1.html");
+  EXPECT_TRUE(NavigateToURL(shell(), url1));
+  FrameTreeNode* root = static_cast<WebContentsImpl*>(shell()->web_contents())
+                            ->GetFrameTree()
+                            ->root();
+
+  // Pop open a new window.
+  ShellAddedObserver new_shell_observer;
+  EXPECT_TRUE(ExecuteScript(root, "var w = window.open()"));
+  Shell* new_shell = new_shell_observer.GetShell();
+  ASSERT_NE(new_shell->web_contents(), shell()->web_contents());
+  EXPECT_FALSE(
+      new_shell->web_contents()->GetController().GetLastCommittedEntry());
+
+  // Navigate it to a cross-site URL that redirects to a data: URL.  Since it is
+  // an unsafe redirect, it will result in a blocked navigation and error page.
+  GURL redirect_to_data_url(
+      embedded_test_server()->GetURL("/server-redirect?data:text/html,Hello!"));
+  TestNavigationObserver nav_observer(new_shell->web_contents(), 1);
+  EXPECT_TRUE(ExecuteScript(
+      root, "w.location.href = '" + redirect_to_data_url.spec() + "';"));
+  nav_observer.WaitForNavigationFinished();
+  EXPECT_FALSE(nav_observer.last_navigation_succeeded());
+  NavigationControllerImpl& controller = static_cast<NavigationControllerImpl&>(
+      new_shell->web_contents()->GetController());
+  EXPECT_EQ(0, controller.GetLastCommittedEntryIndex());
+  EXPECT_EQ(redirect_to_data_url, controller.GetLastCommittedEntry()->GetURL());
+  EXPECT_EQ(PAGE_TYPE_ERROR, controller.GetLastCommittedEntry()->GetPageType());
+
+  // Navigate to a new document, then go back in history trying to load the
+  // blocked URL.
+  EXPECT_TRUE(NavigateToURL(new_shell, url1));
+  EXPECT_EQ(1, controller.GetLastCommittedEntryIndex());
+  EXPECT_EQ(url1, controller.GetLastCommittedEntry()->GetURL());
+  TestNavigationObserver back_load_observer(new_shell->web_contents());
+  controller.GoBack();
+  back_load_observer.Wait();
+  EXPECT_EQ(redirect_to_data_url, controller.GetLastCommittedEntry()->GetURL());
+
+  // The opener should not be able to script the page, which should be another
+  // error message and not a blank page.
+  std::string result;
+  EXPECT_TRUE(ExecuteScriptAndExtractString(
+      shell(),
+      "domAutomationController.send((function() {\n"
+      "  try {\n"
+      "    return w.document.body.innerHTML;\n"
+      "  } catch (e) {\n"
+      "    return e.toString();\n"
+      "  }\n"
+      "})())",
+      &result));
+  DLOG(INFO) << "Result: " << result;
+  EXPECT_THAT(result,
+              ::testing::MatchesRegex("SecurityError: Blocked a frame with "
+                                      "origin \"http://a.com:\\d+\" from "
+                                      "accessing a cross-origin frame."));
 }
 
 // If the main frame does a load, it should not be reported as a subframe
diff --git a/chromium/content/browser/frame_host/navigation_handle_impl_browsertest.cc b/chromium/content/browser/frame_host/navigation_handle_impl_browsertest.cc
index 7fc4394de0d..8f82bfe86df 100644
--- a/chromium/content/browser/frame_host/navigation_handle_impl_browsertest.cc
+++ b/chromium/content/browser/frame_host/navigation_handle_impl_browsertest.cc
@@ -1403,20 +1403,38 @@ IN_PROC_BROWSER_TEST_F(PlzNavigateNavigationHandleImplBrowserTest,
   }
 
   {
-    // Reloading the blocked document should load about:blank and not transfer
-    // processes.
-    GURL about_blank_url(url::kAboutBlankURL);
-    NavigationHandleObserver observer(shell()->web_contents(), about_blank_url);
+    // Reloading the blocked document from the renderer process should not
+    // transfer processes.
+    NavigationHandleObserver observer(shell()->web_contents(), blocked_url);
     TestNavigationObserver navigation_observer(shell()->web_contents(), 1);
 
-    shell()->Reload();
+    EXPECT_TRUE(ExecuteScript(shell(), "location.reload()"));
     navigation_observer.Wait();
     EXPECT_TRUE(observer.has_committed());
-    EXPECT_FALSE(observer.is_error());
+    EXPECT_TRUE(observer.is_error());
     EXPECT_EQ(site_instance,
               shell()->web_contents()->GetMainFrame()->GetSiteInstance());
   }
 
+  {
+    // Reloading the blocked document from the browser process ends up
+    // transferring processes in --site-per-process.
+    NavigationHandleObserver observer(shell()->web_contents(), blocked_url);
+    TestNavigationObserver navigation_observer(shell()->web_contents(), 1);
+
+    shell()->Reload();
+    navigation_observer.Wait();
+    EXPECT_TRUE(observer.has_committed());
+    EXPECT_TRUE(observer.is_error());
+    if (AreAllSitesIsolatedForTesting()) {
+      EXPECT_NE(site_instance,
+                shell()->web_contents()->GetMainFrame()->GetSiteInstance());
+    } else {
+      EXPECT_EQ(site_instance,
+                shell()->web_contents()->GetMainFrame()->GetSiteInstance());
+    }
+  }
+
   installer.reset();
 
   {