PlzNavigate: add support for BLOCK_REQUEST during redirects

content/browser/frame_host/navigation_handle_impl_browsertest.cc

Index: content/browser/frame_host/navigation_handle_impl_browsertest.cc
diff --git a/content/browser/frame_host/navigation_handle_impl_browsertest.cc b/content/browser/frame_host/navigation_handle_impl_browsertest.cc
index 16ada54bff0d29a6d9703e4c0172077049b2a1a7..9d022bf4407c30b368b245cda50673d4f8d18c47 100644
--- a/content/browser/frame_host/navigation_handle_impl_browsertest.cc
+++ b/content/browser/frame_host/navigation_handle_impl_browsertest.cc
@@ -7,6 +7,7 @@
 #include "content/browser/web_contents/web_contents_impl.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/browser/web_contents_observer.h"
+#include "content/public/common/browser_side_navigation_policy.h"
 #include "content/public/common/request_context_type.h"
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/content_browser_test.h"
@@ -901,6 +902,20 @@ IN_PROC_BROWSER_TEST_F(NavigationHandleImplBrowserTest,
     EXPECT_FALSE(NavigateToURL(shell(), kUrl));
     EXPECT_EQ(net::ERR_BLOCKED_BY_CLIENT, observer.net_error_code());
   }
+
+  {
+    // Set up a NavigationThrottle that will block the navigation in
+    // WillRedirectRequest.
+    TestNavigationThrottleInstaller installer(
+        shell()->web_contents(), NavigationThrottle::PROCEED,
+        NavigationThrottle::BLOCK_REQUEST, NavigationThrottle::PROCEED);
+    NavigationHandleObserver observer(shell()->web_contents(), kRedirectingUrl);
+
+    // Try to navigate to the url. The navigation should be canceled and the
+    // NavigationHandle should have the right error code.
+    EXPECT_FALSE(NavigateToURL(shell(), kRedirectingUrl));
+    EXPECT_EQ(net::ERR_BLOCKED_BY_CLIENT, observer.net_error_code());
+  }
 }
 
 // Specialized test that verifies the NavigationHandle gets the HTTPS upgraded
@@ -1035,4 +1050,67 @@ IN_PROC_BROWSER_TEST_F(NavigationHandleImplBrowserTest,
   }
 }
 
+// Record and list the navigations that are started and finished.
+class NavigationLogger : public WebContentsObserver {
+ public:
+  NavigationLogger(WebContents* web_contents)
+      : WebContentsObserver(web_contents) {}
+
+  void DidStartNavigation(NavigationHandle* navigation_handle) override {
+    started_navigation_urls_.push_back(navigation_handle->GetURL());
+  }
+
+  void DidFinishNavigation(NavigationHandle* navigation_handle) override {
+    finished_navigation_urls_.push_back(navigation_handle->GetURL());
+  }
+
+  const std::vector<GURL>& started_navigation_urls() const {
+    return started_navigation_urls_;
+  }
+  const std::vector<GURL>& finished_navigation_urls() const {
+    return finished_navigation_urls_;
+  }
+
+ private:
+  std::vector<GURL> started_navigation_urls_;
+  std::vector<GURL> finished_navigation_urls_;
+};
+
+// There was a bug without PlzNavigate that happened when a navigation was
+// blocked after a redirect. Blink didn't know about the redirect and tried
+// to commit an error page to the post-redirect URL. The result was that the

[Charlie Reis, 2017/03/09 17:39:03]

Did you mean pre-redirect URL?

[arthursonzogni, 2017/03/10 11:55:28]

Yes thanks!

+// NavigationHandle was not found on the browser-side and a new NavigationHandle
+// created for committing the error page. This test makes sure that only one
+// NavigationHandle is used for committing the error page.
+// See crbug.com/695421
+IN_PROC_BROWSER_TEST_F(NavigationHandleImplBrowserTest, BlockedOnRedirect) {
+  const GURL kUrl = embedded_test_server()->GetURL("/title1.html");
+  const GURL kRedirectingUrl =
+      embedded_test_server()->GetURL("/server-redirect?" + kUrl.spec());
+
+  // Set up a NavigationThrottle that will block the navigation in
+  // WillRedirectRequest.
+  TestNavigationThrottleInstaller installer(
+      shell()->web_contents(), NavigationThrottle::PROCEED,
+      NavigationThrottle::BLOCK_REQUEST, NavigationThrottle::PROCEED);
+  NavigationHandleObserver observer(shell()->web_contents(), kRedirectingUrl);
+  NavigationLogger logger(shell()->web_contents());
+
+  // Try to navigate to the url. The navigation should be canceled and the
+  // NavigationHandle should have the right error code.
+  EXPECT_FALSE(NavigateToURL(shell(), kRedirectingUrl));
+  EXPECT_EQ(net::ERR_BLOCKED_BY_CLIENT, observer.net_error_code());
+
+  // Only one navigation is expected to happen.
+  std::vector<GURL> started_navigation = {kRedirectingUrl};
+  EXPECT_EQ(started_navigation, logger.started_navigation_urls());
+
+  std::vector<GURL> finished_navigation;
+  if (IsBrowserSideNavigationEnabled())
+    finished_navigation = {kUrl};
+  else
+    finished_navigation = {kRedirectingUrl};

[Charlie Reis, 2017/03/09 17:39:03]

I'm concerned that we have a difference in behavior based on which mode we're
in.  Ideally we'd decide what the right behavior is and make both modes act that
way.

After our discussion about the difference between blocked and failed navigations
(where blocked navigations are more associated with the previous page and their
error page stays in its process, vs failed navigations that might succeed on
reload and end up with an error page in the destination process), I wonder if
kRedirectingUrl is the right thing for both modes?  Otherwise, a malicious
redirect URL might be able to get an error page into a process of its choice.

Would it make sense for PlzNavigate to do kRedirectingUrl here, or are there
reasons that's worse?

[arthursonzogni, 2017/03/10 11:55:28]

The page is blocked on the post-redirect URL. So the right behavior is the
PlzNavigate one, but fixing the problem that occurs without PlzNavigate looks
very hard. What Camille did to mitigate it is dirty and we commit an error page
to the pre-redirect URL, but that the best solution I know.

If I understand, you suggest to do the wrong behavior with PlzNavigate too
(committing an error-page to the pre-redirect URL). Why not, but I don't
understand the benefits apart from the coherence in both mode.

Do you know what an attacker could do with an error page in the wrong process?
Whatever happens, when Nasko's patch:
https://codereview.chromium.org/2738643002/ will be landed, the blocking error
page will always be the process of its parent, no matter what is the url. Isn't
it a problem too? 

[Charlie Reis, 2017/03/13 20:48:02]

My argument there was that the pre-redirect URL isn't necessarily the wrong one.
 Yes, Chrome today reports the post-redirect URL, but there's unfortunate
aspects to that for blocked URLs.  A malicious page can try to navigate to an
off-limits page, and even though the navigation "fails", that URL still ends up
in the user's session history.  The user might later reload it, restore it, etc,
and we have to be extra careful that malicious page can't circumvent having its
navigation blocked this way.

By using the pre-redirect URL, we would avoid those problems.  On the flip side,
though, the user experience is quite different, and might make it look like the
pre-redirect URL was the one being blocked (which isn't true).  It's a
nontrivial change to make, and Nasko, Alex, and I have some thoughts about how
to do it down the road.

Given that, let's forget I suggested standardizing on the pre-redirect CL for
now.  :)  Instead, let's try to get both modes to report the post-redirect CL,
which is a minimal change from Chrome's behavior before this CL.

On your earlier version of this CL
(https://codereview.chromium.org/2712123002/), I wondered if there was a way to
send ResourceMsg_ReceivedRedirect in the blocked case so that the renderer did
the right thing.  Chatting with Nasko, it seems like we don't send that IPC at
the moment because we don't make it down to AsyncResourceHandler if the redirect
was blocked.  Maybe the blocking logic can send that IPC manually, since we know
we won't do it from AsyncResourceHandler?  It's a bit of a layering violation /
hack, but we'd only need it until PlzNavigate ships, and it would hopefully get
us consistent behavior across modes.

+  EXPECT_EQ(finished_navigation, logger.finished_navigation_urls());
+}
+
 }  // namespace content