[iOS] Use canonicalized URLs in SafeBrowsingTabHelper.

The SafeBrowsing URL checker strips refs and uses canonicalized URLs,
so responses with the same canonicalized URL as a previous request
should use the same decision.

Bug: 1079493
Change-Id: Id696ce278dc0235bb677b4762a7a33c9fce490b3
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2202496
Auto-Submit: Kurt Horimoto <kkhorimoto@chromium.org>
Reviewed-by: Ali Juma <ajuma@chromium.org>
Commit-Queue: Ali Juma <ajuma@chromium.org>
Cr-Commit-Position: refs/heads/master@{#768887}

ios/chrome/browser/safe_browsing/safe_browsing_tab_helper.mm

@@ -38,6 +38,26 @@ web::WebStatePolicyDecider::PolicyDecision CreateSafeBrowsingErrorDecision() {
                           code:kUnsafeResourceErrorCode
                       userInfo:nil]);
 }
+
+// Returns a canonicalized version of |url| as used by the SafeBrowsingService.
+GURL GetCanonicalizedUrl(const GURL& url) {
+  std::string hostname;
+  std::string path;
+  std::string query;
+  safe_browsing::V4ProtocolManagerUtil::CanonicalizeUrl(url, &hostname, &path,
+                                                        &query);
+
+  GURL::Replacements replacements;
+  if (hostname.length())
+    replacements.SetHostStr(hostname);
+  if (path.length())
+    replacements.SetPathStr(path);
+  if (query.length())
+    replacements.SetQueryStr(query);
+  replacements.ClearRef();
+
+  return url.ReplaceComponents(replacements);
+}
 }  // namespace
 
 #pragma mark - SafeBrowsingTabHelper
@@ -141,7 +161,7 @@ SafeBrowsingTabHelper::PolicyDecider::ShouldAllowRequest(
     NSURLRequest* request,
     const web::WebStatePolicyDecider::RequestInfo& request_info) {
   // Allow navigations for URLs that cannot be checked by the service.
-  GURL request_url = net::GURLWithNSURL(request.URL);
+  GURL request_url = GetCanonicalizedUrl(net::GURLWithNSURL(request.URL));
   SafeBrowsingService* safe_browsing_service =
       GetApplicationContext()->GetSafeBrowsingService();
   if (!safe_browsing_service->CanCheckUrl(request_url))
@@ -216,7 +236,7 @@ void SafeBrowsingTabHelper::PolicyDecider::ShouldAllowResponse(
   // Allow navigations for URLs that cannot be checked by the service.
   SafeBrowsingService* safe_browsing_service =
       GetApplicationContext()->GetSafeBrowsingService();
-  GURL response_url = net::GURLWithNSURL(response.URL);
+  GURL response_url = GetCanonicalizedUrl(net::GURLWithNSURL(response.URL));
   if (!safe_browsing_service->CanCheckUrl(response_url)) {
     std::move(callback).Run(
         web::WebStatePolicyDecider::PolicyDecision::Allow());

ios/chrome/browser/safe_browsing/safe_browsing_tab_helper_unittest.mm

@@ -141,6 +141,36 @@ TEST_P(SafeBrowsingTabHelperTest, SingleUnsafeRequestAndResponse) {
   EXPECT_TRUE(response_decision.ShouldCancelNavigation());
 }
 
+// Tests the case of a single safe navigation where the response URL has a
+// different ref than the request URL.
+TEST_P(SafeBrowsingTabHelperTest, SafeRequestAndResponseWithDifferingRef) {
+  GURL request_url("http://chromium.test");
+  GURL response_url("http://chromium.test#ref");
+  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 unsafe navigation where the response URL has a
+// different ref than the request URL.
+TEST_P(SafeBrowsingTabHelperTest, UnsafeRequestAndResponseWithDifferingRef) {
+  GURL request_url("http://" + FakeSafeBrowsingService::kUnsafeHost);
+  GURL response_url("http://" + FakeSafeBrowsingService::kUnsafeHost + "#ref");
+  EXPECT_TRUE(ShouldAllowRequestUrl(request_url).ShouldAllowNavigation());
+
+  if (SafeBrowsingDecisionArrivesBeforeResponse())
+    base::RunLoop().RunUntilIdle();
+
+  web::WebStatePolicyDecider::PolicyDecision response_decision =
+      ShouldAllowResponseUrl(response_url);
+  EXPECT_TRUE(response_decision.ShouldCancelNavigation());
+}
+
 // Tests the case of a single navigation request followed by multiple responses
 // for the same URL.
 TEST_P(SafeBrowsingTabHelperTest, RepeatedResponse) {
@@ -238,6 +268,28 @@ TEST_P(SafeBrowsingTabHelperTest, SafeSubFrameRequestAndResponse) {
   EXPECT_TRUE(sub_frame_response_decision.ShouldAllowNavigation());
 }
 
+// Tests the case of a single safe sub frame navigation request and response,
+// where the response URL has a different hash fragment than the request.
+TEST_P(SafeBrowsingTabHelperTest,
+       SafeSubFrameRequestAndResponseWithDifferingRef) {
+  GURL request_url("http://chromium_sub_frame.test");
+  GURL response_url("http://chromium_sub_frame.test#different_hash");
+  SimulateSafeMainFrameLoad();
+
+  // Execute ShouldAllowRequest() for a safe subframe navigation.
+  auto sub_frame_request_decision =
+      ShouldAllowRequestUrl(request_url, /*for_main_frame=*/false);
+  EXPECT_TRUE(sub_frame_request_decision.ShouldAllowNavigation());
+
+  if (SafeBrowsingDecisionArrivesBeforeResponse())
+    base::RunLoop().RunUntilIdle();
+
+  // Verify that the sub frame navigation is allowed.
+  web::WebStatePolicyDecider::PolicyDecision sub_frame_response_decision =
+      ShouldAllowResponseUrl(response_url, /*for_main_frame=*/false);
+  EXPECT_TRUE(sub_frame_response_decision.ShouldAllowNavigation());
+}
+
 // Tests the case where multiple sub frames navigating to safe URLs are all
 // allowed.
 TEST_P(SafeBrowsingTabHelperTest, RepeatedSafeSubFrameResponses) {
@@ -352,6 +404,65 @@ TEST_P(SafeBrowsingTabHelperTest, UnsafeSubFrameRequestAndResponse) {
   EXPECT_EQ(kUnsafeResourceErrorCode, error.code);
 }
 
+// Tests the case of a single unsafesafe sub frame navigation request and
+// response, where the response URL has a different hash fragment than the
+// request.
+TEST_P(SafeBrowsingTabHelperTest,
+       UnsafeSubFrameRequestAndResponseWithDifferingRef) {
+  GURL request_url("http://" + FakeSafeBrowsingService::kUnsafeHost);
+  GURL response_url("http://" + FakeSafeBrowsingService::kUnsafeHost + "#ref");
+  ASSERT_FALSE(navigation_manager_->ReloadWasCalled());
+  web::NavigationItem* main_frame_item = SimulateSafeMainFrameLoad();
+
+  // Execute ShouldAllowRequest() for an unsafe subframe navigation.
+  auto sub_frame_request_decision =
+      ShouldAllowRequestUrl(request_url, /*for_main_frame=*/false);
+  EXPECT_TRUE(sub_frame_request_decision.ShouldAllowNavigation());
+
+  // The tab helper expects that the sub frame's UnsafeResource is stored in the
+  // container before the URL check completes.
+  security_interstitials::UnsafeResource resource;
+  resource.url = request_url;
+  resource.resource_type = safe_browsing::ResourceType::kSubFrame;
+  resource.web_state_getter = web_state_.CreateDefaultGetter();
+  SafeBrowsingUnsafeResourceContainer::FromWebState(&web_state_)
+      ->StoreUnsafeResource(resource);
+
+  if (SafeBrowsingDecisionArrivesBeforeResponse()) {
+    // If a sub frame navigation is deemed unsafe before its response policy
+    // decision is requested, the last committed NavigationItem is immediately
+    // reloaded.  This would cancel future sub frame loads, so
+    // ShouldAllowResponse() is not expected to be executed in this case.
+    base::RunLoop().RunUntilIdle();
+  } else {
+    // Verify that the sub frame navigation is not allowed.
+    web::WebStatePolicyDecider::PolicyDecision sub_frame_response_decision =
+        ShouldAllowResponseUrl(response_url, /*for_main_frame=*/false);
+    EXPECT_FALSE(sub_frame_response_decision.ShouldAllowNavigation());
+  }
+
+  // The unsafe sub frame should trigger a reload.
+  EXPECT_TRUE(navigation_manager_->ReloadWasCalled());
+
+  // Simulate the main frame reload caused by the unsafe sub frame resource.
+  navigation_manager_->SetPendingItem(main_frame_item);
+  auto main_frame_reload_request_decision =
+      ShouldAllowRequestUrl(main_frame_item->GetURL(), /*for_main_frame=*/true,
+                            ui::PageTransition::PAGE_TRANSITION_RELOAD);
+  EXPECT_TRUE(main_frame_reload_request_decision.ShouldAllowNavigation());
+
+  // The URL check is skipped for safe browsing error pages caused by reloading
+  // the main frame for unsafe sub frame resources, so there is no need to run
+  // the runloop.
+  auto main_frame_reload_response_decision =
+      ShouldAllowResponseUrl(main_frame_item->GetURL());
+  EXPECT_TRUE(main_frame_reload_response_decision.ShouldCancelNavigation());
+  EXPECT_TRUE(main_frame_reload_response_decision.ShouldDisplayError());
+  NSError* error = main_frame_reload_response_decision.GetDisplayError();
+  EXPECT_NSEQ(kSafeBrowsingErrorDomain, error.domain);
+  EXPECT_EQ(kUnsafeResourceErrorCode, error.code);
+}
+
 INSTANTIATE_TEST_SUITE_P(
     /* No InstantiationName */,
     SafeBrowsingTabHelperTest,