[Lookalikes] Change how the lookalike interstitial handles redirects

This CL changes what domains we consider for the lookalike interstitial.
Previously, if Chrome saw a navigation that looked like
    A -> B -> C -> D
(where site A is redirecting to B, etc.), the lookalike logic would
inspect each stage, A through D, for lookalikes. It accomplished this by
implementing DidRedirectNavigation() and DidFinishNavigation(). This
added a bunch of complexity, but allowed us to stop a navigation
mid-redirect.

This CL changes that behavior to wait until the entire redirect chain
has completed, and then inspect only sites A and D. These domains are
the only ones visible to the user, and thus the only ones wherein a
lookalike URL might be effective. This change also means that we no
longer implement DidRedirectNavigation().

Bug: 1092690
Change-Id: I2cfa724b9793b60dd5200d1bf6847cc6919fa469
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2438888Reviewed-by: Livvie Lin <livvielin@chromium.org>
Reviewed-by: Mustafa Emre Acer <meacer@chromium.org>
Commit-Queue: Joe DeBlasio <jdeblasio@chromium.org>
Cr-Commit-Position: refs/heads/master@{#812173}

chrome/browser/lookalikes/lookalike_url_navigation_throttle.h

@@ -45,7 +45,6 @@ class LookalikeUrlNavigationThrottle : public content::NavigationThrottle {
 
   // content::NavigationThrottle:
   ThrottleCheckResult WillProcessResponse() override;
-  ThrottleCheckResult WillRedirectRequest() override;
   const char* GetNameForLogging() override;
 
   static std::unique_ptr<LookalikeUrlNavigationThrottle>
@@ -56,24 +55,20 @@ class LookalikeUrlNavigationThrottle : public content::NavigationThrottle {
   void SetUseTestProfileForTesting() { use_test_profile_ = true; }
 
  private:
-  // Checks whether the navigation to |url| can proceed. If
-  // |check_safe_redirect| is true, will check if a safe redirect led to |url|.
-  ThrottleCheckResult HandleThrottleRequest(const GURL& url,
-                                            bool check_safe_redirect);
-
   // Performs synchronous top domain and engaged site checks on the navigated
-  // |url|. Uses |engaged_sites| for the engaged site checks.
+  // and redirected urls. Uses |engaged_sites| for the engaged site checks.
   ThrottleCheckResult PerformChecks(
-      const GURL& url,
-      const DomainInfo& navigated_domain,
-      bool check_safe_redirect,
       const std::vector<DomainInfo>& engaged_sites);
 
   // A void-returning variant, only used with deferred throttle results.
-  void PerformChecksDeferred(const GURL& url,
-                             const DomainInfo& navigated_domain,
-                             bool check_safe_redirect,
-                             const std::vector<DomainInfo>& engaged_sites);
+  void PerformChecksDeferred(const std::vector<DomainInfo>& engaged_sites);
+
+  // Returns whether |url| is a lookalike, setting |match_type| and
+  // |suggested_url| appropriately. Used in PerformChecks() on a per-URL basis.
+  bool IsLookalikeUrl(const GURL& url,
+                      const std::vector<DomainInfo>& engaged_sites,
+                      LookalikeUrlMatchType* match_type,
+                      GURL* suggested_url);
 
   ThrottleCheckResult ShowInterstitial(const GURL& safe_domain,
                                        const GURL& url,

chrome/browser/lookalikes/lookalike_url_navigation_throttle_browsertest.cc

@@ -579,14 +579,13 @@ IN_PROC_BROWSER_TEST_P(LookalikeUrlNavigationThrottleBrowserTest,
 
   if (!punycode_interstitial_enabled()) {
     TestInterstitialNotShown(browser(), kNavigatedUrl);
-    CheckNoUkm();
   } else {
     TestPunycodeInterstitialShown(
         browser(), kNavigatedUrl,
         NavigationSuggestionEvent::kFailedSpoofChecks);
-    CheckUkm({kNavigatedUrl}, "MatchType",
-             LookalikeUrlMatchType::kFailedSpoofChecks);
   }
+  CheckUkm({kNavigatedUrl}, "MatchType",
+           LookalikeUrlMatchType::kFailedSpoofChecks);
 }
 
 // The navigated domain will fall back to punycode because it fails spoof checks
@@ -890,20 +889,18 @@ IN_PROC_BROWSER_TEST_P(LookalikeUrlNavigationThrottleBrowserTest,
 
 // The site redirects to the matched site, but the redirect chain has more than
 // two redirects.
-// TODO(meacer): Consider allowing this case.
 IN_PROC_BROWSER_TEST_P(LookalikeUrlNavigationThrottleBrowserTest,
-                       Idn_SiteEngagement_UnsafeRedirect) {
-  const GURL kExpectedSuggestedUrl = GetURLWithoutPath("site1.com");
+                       Idn_SiteEngagement_MidRedirectSpoofsIgnored) {
+  const GURL kFinalUrl = GetURLWithoutPath("site1.com");
   const GURL kMidUrl = embedded_test_server()->GetURL(
-      "sité1.com", "/server-redirect?" + kExpectedSuggestedUrl.spec());
+      "sité1.com", "/server-redirect?" + kFinalUrl.spec());
   const GURL kNavigatedUrl = embedded_test_server()->GetURL(
       "other-site.test", "/server-redirect?" + kMidUrl.spec());
 
   SetEngagementScore(browser(), kNavigatedUrl, kLowEngagement);
-  SetEngagementScore(browser(), kExpectedSuggestedUrl, kHighEngagement);
-  TestMetricsRecordedAndInterstitialShown(
-      browser(), kNavigatedUrl, kExpectedSuggestedUrl,
-      NavigationSuggestionEvent::kMatchSiteEngagement);
+  SetEngagementScore(browser(), kFinalUrl, kHighEngagement);
+  TestInterstitialNotShown(browser(), kNavigatedUrl);
+  CheckNoUkm();
 }
 
 // The site is allowed by the component updater.
@@ -1126,17 +1123,17 @@ IN_PROC_BROWSER_TEST_P(LookalikeUrlNavigationThrottleBrowserTest,
   NavigateToURLSync(browser(), GetURL("example.com"));
 
   {
-    // ...or when it's later in the chain
+    // ...but not when it's in the middle of the chain
     const GURL kNavigatedUrl =
         GetLongRedirect("example.net", "googlé.com", "example.com");
     SetEngagementScore(browser(), kNavigatedUrl, kLowEngagement);
-    LoadAndCheckInterstitialAt(browser(), kNavigatedUrl);
+    TestInterstitialNotShown(browser(), kNavigatedUrl);
   }
 
   NavigateToURLSync(browser(), GetURL("example.com"));
 
   {
-    // ...or when it's last in the chain
+    // ...but definitely when it's last in the chain.
     const GURL kNavigatedUrl =
         GetLongRedirect("example.net", "example.com", "googlé.com");
     SetEngagementScore(browser(), kNavigatedUrl, kLowEngagement);

chrome/browser/lookalikes/lookalike_url_navigation_throttle_unittest.cc

@@ -16,74 +16,6 @@
 
 namespace lookalikes {
 
-// These redirects are safe:
-// - http[s]://sité.test -> http[s]://site.test
-// - http[s]://sité.test/path -> http[s]://site.test
-// - http[s]://subdomain.sité.test -> http[s]://site.test
-// - http[s]://random.test -> http[s]://sité.test -> http[s]://site.test
-// - http://sité.test/path -> https://sité.test/path -> https://site.test ->
-// <any_url>
-// - "subdomain" on either side.
-//
-// These are not safe:
-// - http[s]://[subdomain.]sité.test -> http[s]://[subdomain.]site.test/path
-// because the redirected URL has a path.
-TEST(LookalikeUrlNavigationThrottleTest, IsSafeRedirect) {
-  EXPECT_TRUE(IsSafeRedirect(
-      "example.com", {GURL("http://éxample.com"), GURL("http://example.com")}));
-  EXPECT_TRUE(IsSafeRedirect(
-      "example.com", {GURL("http://éxample.com"), GURL("http://example.com")}));
-  EXPECT_TRUE(IsSafeRedirect(
-      "example.com",
-      {GURL("http://éxample.com"), GURL("http://subdomain.example.com")}));
-  EXPECT_TRUE(IsSafeRedirect(
-      "example.com", {GURL("http://éxample.com"), GURL("http://example.com"),
-                      GURL("https://example.com")}));
-  // Original site redirects to HTTPS.
-  EXPECT_TRUE(IsSafeRedirect(
-      "example.com", {GURL("http://éxample.com"), GURL("https://éxample.com"),
-                      GURL("https://example.com")}));
-  // Original site redirects to HTTPS which redirects to HTTP which redirects
-  // back to HTTPS of the non-IDN version.
-  EXPECT_TRUE(IsSafeRedirect(
-      "example.com",
-      {GURL("http://éxample.com/redir1"), GURL("https://éxample.com/redir1"),
-       GURL("http://éxample.com/redir2"), GURL("https://example.com/")}));
-  // Same as above, but there is another redirect at the end of the chain.
-  EXPECT_TRUE(IsSafeRedirect(
-      "example.com",
-      {GURL("http://éxample.com/redir1"), GURL("https://éxample.com/redir1"),
-       GURL("http://éxample.com/redir2"), GURL("https://example.com/"),
-       GURL("https://totallydifferentsite.com/somepath")}));
-
-  // Not a redirect, the chain is too short.
-  EXPECT_FALSE(IsSafeRedirect("example.com", {GURL("http://éxample.com")}));
-  // Not safe: Redirected site is not the same as the matched site.
-  EXPECT_FALSE(IsSafeRedirect("example.com", {GURL("http://éxample.com"),
-                                              GURL("http://other-site.com")}));
-  // Not safe: Initial URL doesn't redirect to the root of the suggested domain.
-  EXPECT_FALSE(IsSafeRedirect(
-      "example.com",
-      {GURL("http://éxample.com"), GURL("http://example.com/path")}));
-  // Not safe: The first redirect away from éxample.com is not to the matching
-  // non-IDN site.
-  EXPECT_FALSE(IsSafeRedirect("example.com", {GURL("http://éxample.com"),
-                                              GURL("http://intermediate.com"),
-                                              GURL("http://example.com")}));
-
-  // Not safe: The redirect stays unsafe from éxample.com to éxample.com.
-  EXPECT_FALSE(IsSafeRedirect(
-      "example.com", {GURL("http://éxample.com"), GURL("http://éxample.com")}));
-  // Not safe: Same, but to a path on the bad domain
-  EXPECT_FALSE(IsSafeRedirect(
-      "example.com",
-      {GURL("http://éxample.com"), GURL("http://éxample.com/path")}));
-  // Not safe: Same, but with an intermediary domain.
-  EXPECT_FALSE(IsSafeRedirect("example.com", {GURL("http://éxample.com/path"),
-                                              GURL("http://intermediate.com/p"),
-                                              GURL("http://éxample.com/dir")}));
-}
-
 class LookalikeThrottleTest : public ChromeRenderViewHostTestHarness {};
 
 // Tests that spoofy hostnames are properly handled in the throttle.
@@ -146,6 +78,7 @@ TEST_F(LookalikeThrottleTest, SpoofsBlocked) {
 
     GURL url(std::string("http://") + test_case.hostname);
     content::MockNavigationHandle handle(url, main_rfh());
+    handle.set_redirect_chain({url});
     handle.set_page_transition(ui::PAGE_TRANSITION_TYPED);
 
     auto throttle =

chrome/browser/reputation/local_heuristics.cc

@@ -69,7 +69,7 @@ bool ShouldTriggerSafetyTipFromLookalike(
   }
 
   // If we're already displaying an interstitial, don't warn again.
-  if (ShouldBlockLookalikeUrlNavigation(match_type, navigated_domain)) {
+  if (ShouldBlockLookalikeUrlNavigation(match_type)) {
     return false;
   }
 

components/lookalikes/core/lookalike_url_util.cc

@@ -452,8 +452,7 @@ bool IsTopDomain(const DomainInfo& domain_info) {
   return false;
 }
 
-bool ShouldBlockLookalikeUrlNavigation(LookalikeUrlMatchType match_type,
-                                       const DomainInfo& navigated_domain) {
+bool ShouldBlockLookalikeUrlNavigation(LookalikeUrlMatchType match_type) {
   if (match_type == LookalikeUrlMatchType::kSiteEngagement) {
     return true;
   }
@@ -462,6 +461,11 @@ bool ShouldBlockLookalikeUrlNavigation(LookalikeUrlMatchType match_type,
           lookalikes::features::kDetectTargetEmbeddingLookalikes)) {
     return true;
   }
+  if (match_type == LookalikeUrlMatchType::kFailedSpoofChecks &&
+      base::FeatureList::IsEnabled(
+          lookalikes::features::kLookalikeInterstitialForPunycode)) {
+    return true;
+  }
   return match_type == LookalikeUrlMatchType::kSkeletonMatchTop500;
 }
 

components/lookalikes/core/lookalike_url_util.h

@@ -152,8 +152,7 @@ bool IsTopDomain(const DomainInfo& domain_info);
 std::string GetETLDPlusOne(const std::string& hostname);
 
 // Returns true if a lookalike interstitial should be shown.
-bool ShouldBlockLookalikeUrlNavigation(LookalikeUrlMatchType match_type,
-                                       const DomainInfo& navigated_domain);
+bool ShouldBlockLookalikeUrlNavigation(LookalikeUrlMatchType match_type);
 
 // Returns true if a domain is visually similar to the hostname of |url|. The
 // matching domain can be a top domain or an engaged site. Similarity

ios/components/security_interstitials/lookalikes/lookalike_url_tab_helper.mm

@@ -114,7 +114,7 @@ void LookalikeUrlTabHelper::ShouldAllowResponse(
 
   RecordUMAFromMatchType(match_type);
 
-  if (ShouldBlockLookalikeUrlNavigation(match_type, navigated_domain)) {
+  if (ShouldBlockLookalikeUrlNavigation(match_type)) {
     const std::string suggested_domain = GetETLDPlusOne(matched_domain);
     DCHECK(!suggested_domain.empty());
     GURL::Replacements replace_host;