Show an error page if a URL redirects to a javascript: URL.

BUG=935175

Change-Id: Id4a9198d5dff823bc3d324b9de9bff2ee86dc499
Reviewed-on: https://chromium-review.googlesource.com/c/1488152
Commit-Queue: Charlie Reis <creis@chromium.org>
Reviewed-by: Arthur Sonzogni <arthursonzogni@chromium.org>
Cr-Commit-Position: refs/heads/master@{#635848}

chrome/browser/chrome_navigation_browsertest.cc

@@ -630,18 +630,6 @@ IN_PROC_BROWSER_TEST_F(ChromeNavigationBrowserTest,
   content::WaitForLoadStop(web_contents);
   EXPECT_EQ(url, web_contents->GetLastCommittedURL());
 
-  // Now try navigating to a URL that tries to redirect to the error page URL,
-  // and make sure the redirect is blocked.  Note that DidStopLoading will
-  // still fire after the redirect is canceled, so TestNavigationObserver can
-  // be used to wait for it.
-  GURL redirect_to_error_url(
-      embedded_test_server()->GetURL("/server-redirect?" + error_url.spec()));
-  content::TestNavigationObserver observer(web_contents);
-  EXPECT_TRUE(ExecuteScript(
-      web_contents, "location.href = '" + redirect_to_error_url.spec() + "';"));
-  observer.Wait();
-  EXPECT_EQ(url, web_contents->GetLastCommittedURL());
-
   // Also ensure that a page can't embed an iframe for an error page URL.
   EXPECT_TRUE(ExecuteScript(web_contents,
                             "var frame = document.createElement('iframe');\n"
@@ -652,6 +640,22 @@ IN_PROC_BROWSER_TEST_F(ChromeNavigationBrowserTest,
       ChildFrameAt(web_contents->GetMainFrame(), 0);
   // The new subframe should remain blank without a committed URL.
   EXPECT_TRUE(subframe_host->GetLastCommittedURL().is_empty());
+
+  // Now try navigating to a URL that tries to redirect to the error page URL
+  // and make sure the redirect is blocked, resulting in an error page for the
+  // redirect URL and not the error_url destination. Note that DidStopLoading
+  // will still fire after the redirect causes its own error page, so
+  // TestNavigationObserver can be used to wait for it.
+  GURL redirect_to_error_url(
+      embedded_test_server()->GetURL("/server-redirect?" + error_url.spec()));
+  content::TestNavigationObserver observer(web_contents);
+  EXPECT_TRUE(ExecuteScript(
+      web_contents, "location.href = '" + redirect_to_error_url.spec() + "';"));
+  observer.Wait();
+  EXPECT_EQ(redirect_to_error_url, web_contents->GetLastCommittedURL());
+  EXPECT_EQ(
+      content::PAGE_TYPE_ERROR,
+      web_contents->GetController().GetLastCommittedEntry()->GetPageType());
 }
 
 // This test ensures that navigating to a page that returns an error code and

content/browser/frame_host/navigation_controller_impl_browsertest.cc

@@ -7733,6 +7733,26 @@ IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
             controller.GetLastCommittedEntry()->GetVirtualURL());
 }
 
+// Verifies that unsafe redirects to javascript: or other URLs create an error
+// page and don't make a spoof possible. See https://crbug.com/935175.
+IN_PROC_BROWSER_TEST_F(NavigationControllerBrowserTest,
+                       UnsafeRedirectCreatesErrorPage) {
+  NavigationControllerImpl& controller = static_cast<NavigationControllerImpl&>(
+      shell()->web_contents()->GetController());
+
+  GURL start_url(embedded_test_server()->GetURL("/title1.html"));
+  EXPECT_TRUE(NavigateToURL(shell(), start_url));
+  EXPECT_EQ(0, controller.GetLastCommittedEntryIndex());
+
+  // Navigating to URLs with unsafe redirects should create an error page so
+  // that the pending URL is not left in the address bar.
+  GURL redirect_to_unsafe_url(
+      embedded_test_server()->GetURL("/server-redirect?javascript:Hello!"));
+  EXPECT_FALSE(NavigateToURL(shell(), redirect_to_unsafe_url));
+  EXPECT_EQ(1, controller.GetLastCommittedEntryIndex());
+  EXPECT_EQ(PAGE_TYPE_ERROR, controller.GetLastCommittedEntry()->GetPageType());
+}
+
 // 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,

content/browser/frame_host/navigation_request.cc

@@ -894,8 +894,13 @@ void NavigationRequest::OnRequestRedirected(
           redirect_info.new_url)) {
     DVLOG(1) << "Denied redirect for "
              << redirect_info.new_url.possibly_invalid_spec();
-    navigation_handle_->set_net_error_code(net::ERR_UNSAFE_REDIRECT);
-    frame_tree_node_->ResetNavigationRequest(false, true);
+    // Show an error page rather than leaving the previous page in place.
+    OnRequestFailedInternal(
+        network::URLLoaderCompletionStatus(net::ERR_UNSAFE_REDIRECT),
+        false /* skip_throttles */, base::nullopt /* error_page_content */,
+        false /* collapse_frame */);
+    // DO NOT ADD CODE after this. The previous call to OnRequestFailedInternal
+    // has destroyed the NavigationRequest.
     return;
   }
 
@@ -908,8 +913,13 @@ void NavigationRequest::OnRequestRedirected(
           redirect_info.new_url)) {
     DVLOG(1) << "Denied unauthorized redirect for "
              << redirect_info.new_url.possibly_invalid_spec();
-    navigation_handle_->set_net_error_code(net::ERR_UNSAFE_REDIRECT);
-    frame_tree_node_->ResetNavigationRequest(false, true);
+    // Show an error page rather than leaving the previous page in place.
+    OnRequestFailedInternal(
+        network::URLLoaderCompletionStatus(net::ERR_UNSAFE_REDIRECT),
+        false /* skip_throttles */, base::nullopt /* error_page_content */,
+        false /* collapse_frame */);
+    // DO NOT ADD CODE after this. The previous call to OnRequestFailedInternal
+    // has destroyed the NavigationRequest.
     return;
   }