[iOS] Handle Safe Browsing redirect chains with repeated URLs

A navigation redirect chain can include the same URL more than
once, for example, b.com -> a.com -> c.com -> a.com.

However, when mapping Safe Browsing decisions to pending
queries, the logic assumes there can only be at most one
pending query with a given URL. This means that, in the
above example, if the Safe Browsing decisions for a.com
arrive after there are two queries for a.com, only one
of these queries will be assigned a result, even after
both decisions are computed.

This CL modifies
SafeBrowsingTabHelper::PolicyDecider::GetPendingMainFrameQuery
to return the oldest matching query that does not yet have
a decision, and renames this method to
GetOldestPendingMainFrameQuery. With this change,
decisions for URLs in a redirect chain are sent to the
first query that doesn't yet have a decision.

This CL also relaxes a DCHECK in
SafeBrowsingTabHelper::PolicyDecider::HandleMainFrameResponsePolicy
that currently checks that the given URL (passed in by
ShouldAllowResponse) matches the pending query's URL (from
ShouldAllowRequest). There are at least two cases where this fails
to hold:

1) Sometimes when there's a server redirect, a ShouldAllowRequest
   call is never made for the target of the redirect. Instead,
   an additional ShouldAllowRequest call arrives for the source
   of the redirect, and WKWebView's URL is still the source URL
   when DidRedirectNavigation is called. This bug does not seem
   to reproduce in trunk WebKit, so may be fixed there.

2) When the request is handled by a ServiceWorker, the ServiceWorker
   is able to set a different URL on the response it produces. In
   these cases, the URL still has the same origin as the request.

Bug: 1144690
Change-Id: Idae3161c9ddee4fd6e1fe6e13c111a5e90b29c28
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2518330
Commit-Queue: Ali Juma <ajuma@chromium.org>
Reviewed-by: Ilya Sherman <isherman@chromium.org>
Reviewed-by: Weilun Shi <sweilun@chromium.org>
Reviewed-by: Eugene But <eugenebut@chromium.org>
Cr-Commit-Position: refs/heads/master@{#826400}

ios/chrome/browser/safe_browsing/safe_browsing_tab_helper.h

@@ -116,8 +116,10 @@ class SafeBrowsingTabHelper
         const GURL& url,
         web::WebStatePolicyDecider::PolicyDecisionCallback callback);
 
-    // Returns the pending main frame query for |url|.
-    MainFrameUrlQuery* GetPendingMainFrameQuery(const GURL& url);
+    // Returns the oldest query for |url| that has not yet received a decision.
+    // If there are no queries for |url| or if all such queries have already
+    // been decided, returns null.
+    MainFrameUrlQuery* GetOldestPendingMainFrameQuery(const GURL& url);
 
     // Callback invoked when a main frame query for |url| has finished with
     // |decision|.

ios/chrome/browser/safe_browsing/safe_browsing_tab_helper.mm

@@ -8,6 +8,7 @@
 
 #include "base/bind.h"
 #include "base/feature_list.h"
+#include "base/metrics/histogram_macros.h"
 #include "base/strings/sys_string_conversions.h"
 #include "base/task/post_task.h"
 #include "components/safe_browsing/core/browser/safe_browsing_url_checker_impl.h"
@@ -89,7 +90,7 @@ bool SafeBrowsingTabHelper::PolicyDecider::IsQueryStale(
   bool is_main_frame = query.IsMainFrame();
   const GURL& url = query.url;
   if (is_main_frame) {
-    return !GetPendingMainFrameQuery(url);
+    return !GetOldestPendingMainFrameQuery(url);
   } else {
     web::NavigationItem* last_committed_item =
         web_state()->GetNavigationManager()->GetLastCommittedItem();
@@ -241,7 +242,19 @@ void SafeBrowsingTabHelper::PolicyDecider::HandleMainFrameResponsePolicy(
     const GURL& url,
     web::WebStatePolicyDecider::PolicyDecisionCallback callback) {
   DCHECK(pending_main_frame_query_);
-  DCHECK_EQ(pending_main_frame_query_->url, url);
+  // When there's a server redirect, a ShouldAllowRequest call sometimes
+  // doesn't happen for the target of the redirection. This seems to be fixed
+  // in trunk WebKit.
+  if (pending_main_frame_redirect_chain_.empty()) {
+    // Only check that the hosts match, since a ServiceWorker that handles a
+    // request can set a different URL in the response that it produces, without
+    // triggering a redirect.
+    DCHECK_EQ(pending_main_frame_query_->url.host(), url.host());
+  } else {
+    bool matching_hosts = pending_main_frame_query_->url.host() == url.host();
+    UMA_HISTOGRAM_BOOLEAN(
+        "IOS.SafeBrowsing.RedirectedRequestResponseHostsMatch", matching_hosts);
+  }
   // If the previous query wasn't added to a pending redirect chain, the
   // pending chain is no longer active, since DidRedirectNavigation() is
   // guaranteed to be called before ShouldAllowResponse() is called for the
@@ -281,24 +294,26 @@ void SafeBrowsingTabHelper::PolicyDecider::HandleSubFrameResponsePolicy(
 #pragma mark URL Check Completion Helpers
 
 SafeBrowsingTabHelper::PolicyDecider::MainFrameUrlQuery*
-SafeBrowsingTabHelper::PolicyDecider::GetPendingMainFrameQuery(
+SafeBrowsingTabHelper::PolicyDecider::GetOldestPendingMainFrameQuery(
     const GURL& url) {
-  if (pending_main_frame_query_ && pending_main_frame_query_->url == url) {
-    return &pending_main_frame_query_.value();
-  } else {
-    for (auto& query : pending_main_frame_redirect_chain_) {
-      if (query.url == url) {
-        return &query;
-      }
+  for (auto& query : pending_main_frame_redirect_chain_) {
+    if (query.url == url && !query.decision) {
+      return &query;
     }
   }
+
+  if (pending_main_frame_query_ && pending_main_frame_query_->url == url &&
+      !pending_main_frame_query_->decision) {
+    return &pending_main_frame_query_.value();
+  }
+
   return nullptr;
 }
 
 void SafeBrowsingTabHelper::PolicyDecider::OnMainFrameUrlQueryDecided(
     const GURL& url,
     web::WebStatePolicyDecider::PolicyDecision decision) {
-  GetPendingMainFrameQuery(url)->decision = decision;
+  GetOldestPendingMainFrameQuery(url)->decision = decision;
 
   // If ShouldAllowResponse() has already been called for this URL, and if
   // an overall decision for the redirect chain can be computed, invoke this

ios/chrome/browser/safe_browsing/safe_browsing_tab_helper_unittest.mm

@@ -294,6 +294,22 @@ TEST_P(SafeBrowsingTabHelperTest, RequestAndResponseWithUnsupportedScheme) {
   EXPECT_TRUE(response_decision.ShouldAllowNavigation());
 }
 
+// Tests the case of a request and response that are not identical, but have
+// the same host.
+TEST_P(SafeBrowsingTabHelperTest, RequestAndResponseWithOnlyMatchingHost) {
+  GURL request_url("http://chromium.test/page1.html");
+  GURL response_url("http://chromium.test/page2.html");
+
+  EXPECT_TRUE(ShouldAllowRequestUrl(request_url).ShouldAllowNavigation());
+
+  if (SafeBrowsingDecisionArrivesBeforeResponse())
+    base::RunLoop().RunUntilIdle();
+
+  web::WebStatePolicyDecider::PolicyDecision response_decision =
+      ShouldAllowResponseUrl(response_url);
+  EXPECT_TRUE(response_decision.ShouldAllowNavigation());
+}
+
 // Tests the case of a single sub frame navigation request and response, for a
 // URL that is safe.
 TEST_P(SafeBrowsingTabHelperTest, SafeSubFrameRequestAndResponse) {
@@ -785,6 +801,92 @@ TEST_P(SafeBrowsingTabHelperTest, RedirectToSameUnsafeURL) {
   EXPECT_TRUE(response_decision.ShouldCancelNavigation());
 }
 
+// Tests the case of a redirection chain where all URLs in the chain are safe,
+// and one URL appears multiple times.
+TEST_P(SafeBrowsingTabHelperTest, SafeRedirectChainWithRepeatedURL) {
+  GURL url1("http://chromium1.test");
+  GURL url2("http://chromium2.test");
+  GURL url3("http://chromium3.test");
+  EXPECT_TRUE(ShouldAllowRequestUrl(url1).ShouldAllowNavigation());
+  if (SafeBrowsingDecisionArrivesBeforeResponse())
+    base::RunLoop().RunUntilIdle();
+
+  EXPECT_TRUE(ShouldAllowRequestUrl(url2).ShouldAllowNavigation());
+  if (SafeBrowsingDecisionArrivesBeforeResponse())
+    base::RunLoop().RunUntilIdle();
+  SimulateMainFrameRedirect();
+
+  EXPECT_TRUE(ShouldAllowRequestUrl(url1).ShouldAllowNavigation());
+  if (SafeBrowsingDecisionArrivesBeforeResponse())
+    base::RunLoop().RunUntilIdle();
+  SimulateMainFrameRedirect();
+
+  EXPECT_TRUE(ShouldAllowRequestUrl(url3).ShouldAllowNavigation());
+  if (SafeBrowsingDecisionArrivesBeforeResponse())
+    base::RunLoop().RunUntilIdle();
+  SimulateMainFrameRedirect();
+
+  EXPECT_TRUE(ShouldAllowRequestUrl(url1).ShouldAllowNavigation());
+  if (SafeBrowsingDecisionArrivesBeforeResponse())
+    base::RunLoop().RunUntilIdle();
+  SimulateMainFrameRedirect();
+
+  web::WebStatePolicyDecider::PolicyDecision response_decision =
+      ShouldAllowResponseUrl(url1);
+  EXPECT_TRUE(response_decision.ShouldAllowNavigation());
+}
+
+// Tests the case of a redirection chain where an unsafe URL appears multiple
+// times.
+TEST_P(SafeBrowsingTabHelperTest, UnsafeRedirectChainWithRepeatedURL) {
+  GURL url1("http://chromium1.test");
+  GURL url2("http://" + FakeSafeBrowsingService::kUnsafeHost);
+  GURL url3("http://chromium3.test");
+  EXPECT_TRUE(ShouldAllowRequestUrl(url1).ShouldAllowNavigation());
+  if (SafeBrowsingDecisionArrivesBeforeResponse())
+    base::RunLoop().RunUntilIdle();
+
+  EXPECT_TRUE(ShouldAllowRequestUrl(url2).ShouldAllowNavigation());
+  StoreUnsafeResource(url2);
+  base::RunLoop().RunUntilIdle();
+  SimulateMainFrameRedirect();
+
+  EXPECT_TRUE(ShouldAllowRequestUrl(url3).ShouldAllowNavigation());
+  if (SafeBrowsingDecisionArrivesBeforeResponse())
+    base::RunLoop().RunUntilIdle();
+  SimulateMainFrameRedirect();
+
+  EXPECT_TRUE(ShouldAllowRequestUrl(url2).ShouldAllowNavigation());
+  StoreUnsafeResource(url2);
+  if (SafeBrowsingDecisionArrivesBeforeResponse())
+    base::RunLoop().RunUntilIdle();
+  SimulateMainFrameRedirect();
+
+  web::WebStatePolicyDecider::PolicyDecision response_decision =
+      ShouldAllowResponseUrl(url2);
+  EXPECT_TRUE(response_decision.ShouldCancelNavigation());
+}
+
+// Tests the case of a redirection where ShouldAllowRequest is not called on
+// the target of the redirection but instead called a second time on the source.
+TEST_P(SafeBrowsingTabHelperTest, RedirectWithMissingShouldAllowRequest) {
+  GURL url1("http://chromium1.test/page1.html");
+  GURL url2("http://chromium2.test/page2.html");
+  EXPECT_TRUE(ShouldAllowRequestUrl(url1).ShouldAllowNavigation());
+  if (SafeBrowsingDecisionArrivesBeforeResponse())
+    base::RunLoop().RunUntilIdle();
+
+  EXPECT_TRUE(ShouldAllowRequestUrl(url1).ShouldAllowNavigation());
+
+  SimulateMainFrameRedirect();
+  if (SafeBrowsingDecisionArrivesBeforeResponse())
+    base::RunLoop().RunUntilIdle();
+
+  web::WebStatePolicyDecider::PolicyDecision response_decision =
+      ShouldAllowResponseUrl(url2);
+  EXPECT_TRUE(response_decision.ShouldAllowNavigation());
+}
+
 // Tests that prerendering is cancelled when the URL being prerendered is
 // unsafe.
 TEST_P(SafeBrowsingTabHelperTest, UnsafeMainFrameRequestCancelsPrerendering) {

tools/metrics/histograms/histograms_xml/ios/histograms.xml

@@ -656,6 +656,21 @@ reviews. Googlers can read more about this at go/gwsq-gerrit.
   </summary>
 </histogram>
 
+<histogram name="IOS.SafeBrowsing.RedirectedRequestResponseHostsMatch"
+    enum="BooleanMatched" expires_after="2021-11-06">
+  <owner>ajuma@chromium.org</owner>
+  <owner>eugenebut@chromium.org</owner>
+  <owner>gambard@chromium.org</owner>
+  <summary>
+    The URL in a navigation request should have the same host as the URL in the
+    corresponding response, but this sometimes doesn't hold after a server
+    redirect because of a WKWebView bug. This histogram counts the frequency of
+    this invariant violation, and is logged each time that a main-frame
+    navigation response is received after a server redirect. True means that the
+    request and response URLs have the same host.
+  </summary>
+</histogram>
+
 <histogram name="IOS.SearchExtension.Action" enum="IOSSearchExtensionAction"
     expires_after="2021-03-07">
   <owner>javierrobles@chromium.org</owner>