Tweak lookalike URL blocking page behavior on DontProceed and close

This CL:
(1) Allows backward navigation on DontProceed if possible and there
is no suggested URL.
(2) Uses the button text 'Close page' instead of 'Back to safety'
in cases where the tab will be closed (no suggested URL and can't
go back).
(3) Records metrics when the interstitial is closed.
(4) Removes unused InterstitialParams reference, adds TODOs for
handling reloads/redirects and the DontProceed navigation, and
changes a function name for consistency.

Bug: 1058898
Change-Id: I5b017072a3b650cbf0479ab9df17ac91d9260a6a
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2274250
Commit-Queue: Livvie Lin <livvielin@chromium.org>
Reviewed-by: Mustafa Emre Acer <meacer@chromium.org>
Reviewed-by: Eugene But <eugenebut@chromium.org>
Cr-Commit-Position: refs/heads/master@{#785757}

components/lookalikes/core/lookalike_url_ui_util.cc

@@ -4,6 +4,7 @@
 
 #include "components/lookalikes/core/lookalike_url_ui_util.h"
 
+#include "build/build_config.h"
 #include "components/lookalikes/core/lookalike_url_util.h"
 #include "components/security_interstitials/core/common_string_util.h"
 #include "components/strings/grit/components_strings.h"
@@ -79,6 +80,17 @@ void PopulateLookalikeUrlBlockingPageStrings(
     load_time_data->SetString(
         "primaryButtonText",
         l10n_util::GetStringUTF16(IDS_LOOKALIKE_URL_BACK_TO_SAFETY));
+#if defined(OS_IOS)
+    // On iOS, offer to close the page instead of navigating to NTP when the
+    // safe URL is empty or invalid, and unable to go back.
+    bool show_close_page = false;
+    load_time_data->GetBoolean("cant_go_back", &show_close_page);
+    if (show_close_page) {
+      load_time_data->SetString(
+          "primaryButtonText",
+          l10n_util::GetStringUTF16(IDS_LOOKALIKE_URL_CLOSE_PAGE));
+    }
+#endif
   }
 }
 

components/security_interstitials_strings.grdp

@@ -86,6 +86,9 @@
   <message name="IDS_LOOKALIKE_URL_BACK_TO_SAFETY" desc="The text for the button that takes the user back to the new tab page.">
     Back to safety
   </message>
+  <message name="IDS_LOOKALIKE_URL_CLOSE_PAGE" desc="The text for the button that closes the tab.">
+    Close page
+  </message>
 
 
   <!-- Clock errors -->

components/security_interstitials_strings_grdp/IDS_LOOKALIKE_URL_CLOSE_PAGE.png.sha1

+ccc5930e89ccf72a2d7cdafbdfa8dcdc308a0fa5
\ No newline at end of file

ios/chrome/browser/web/chrome_web_client.mm

@@ -102,8 +102,8 @@ NSString* GetSafeBrowsingErrorPageHTML(web::WebState* web_state,
 }
 
 // Returns the lookalike error page HTML.
-NSString* GetLookalikeErrorPageHTML(web::WebState* web_state,
-                                    int64_t navigation_id) {
+NSString* GetLookalikeUrlErrorPageHtml(web::WebState* web_state,
+                                       int64_t navigation_id) {
   // Fetch the lookalike URL info from the WebState's container.
   LookalikeUrlContainer* container =
       LookalikeUrlContainer::FromWebState(web_state);
@@ -334,7 +334,7 @@ void ChromeWebClient::PrepareErrorPage(
     // Only kLookalikeUrlErrorCode is supported.
     DCHECK_EQ(kLookalikeUrlErrorCode, final_underlying_error.code);
     std::move(error_html_callback)
-        .Run(GetLookalikeErrorPageHTML(web_state, navigation_id));
+        .Run(GetLookalikeUrlErrorPageHtml(web_state, navigation_id));
   } else if (info.has_value()) {
     base::OnceCallback<void(bool)> proceed_callback;
     base::OnceCallback<void(NSString*)> blocking_page_callback =

ios/chrome/browser/web/chrome_web_client_unittest.mm

@@ -33,6 +33,7 @@
 #include "ios/web/common/features.h"
 #import "ios/web/common/web_view_creation_util.h"
 #import "ios/web/public/test/error_test_util.h"
+#import "ios/web/public/test/fakes/test_navigation_manager.h"
 #import "ios/web/public/test/fakes/test_web_state.h"
 #import "ios/web/public/test/js_test_util.h"
 #include "ios/web/public/test/scoped_testing_web_client.h"
@@ -381,12 +382,14 @@ TEST_F(ChromeWebClientTest, PrepareErrorPageForSafeBrowsingError) {
 
 // Tests PrepareErrorPage for a lookalike error, which results in a
 // committed lookalike interstitial.
-TEST_F(ChromeWebClientTest, PrepareErrorPageForLookalikeError) {
+TEST_F(ChromeWebClientTest, PrepareErrorPageForLookalikeUrlError) {
   web::TestWebState web_state;
   web_state.SetBrowserState(browser_state());
   LookalikeUrlContainer::CreateForWebState(&web_state);
   security_interstitials::IOSBlockingPageTabHelper::CreateForWebState(
       &web_state);
+  auto navigation_manager = std::make_unique<web::TestNavigationManager>();
+  web_state.SetNavigationManager(std::move(navigation_manager));
 
   LookalikeUrlContainer::FromWebState(&web_state)
       ->SetLookalikeUrlInfo(GURL("https://www.safe.test"), GURL(kTestUrl),
@@ -417,6 +420,51 @@ TEST_F(ChromeWebClientTest, PrepareErrorPageForLookalikeError) {
       << base::SysNSStringToUTF8(page);
 }
 
+// Tests PrepareErrorPage for a lookalike error with no suggested URL,
+// which results in a committed lookalike interstitial that has a 'Close page'
+// button instead of 'Back to safety' (when there is no back item).
+TEST_F(ChromeWebClientTest, PrepareErrorPageForLookalikeUrlErrorNoSuggestion) {
+  web::TestWebState web_state;
+  web_state.SetBrowserState(browser_state());
+  LookalikeUrlContainer::CreateForWebState(&web_state);
+  security_interstitials::IOSBlockingPageTabHelper::CreateForWebState(
+      &web_state);
+  auto navigation_manager = std::make_unique<web::TestNavigationManager>();
+  web_state.SetNavigationManager(std::move(navigation_manager));
+
+  LookalikeUrlContainer::FromWebState(&web_state)
+      ->SetLookalikeUrlInfo(GURL(""), GURL(kTestUrl),
+                            LookalikeUrlMatchType::kSkeletonMatchTop5k);
+
+  NSError* error = [NSError errorWithDomain:kLookalikeUrlErrorDomain
+                                       code:kLookalikeUrlErrorCode
+                                   userInfo:nil];
+  __block bool callback_called = false;
+  __block NSString* page = nil;
+  base::OnceCallback<void(NSString*)> callback =
+      base::BindOnce(^(NSString* error_html) {
+        callback_called = true;
+        page = error_html;
+      });
+
+  ChromeWebClient web_client;
+  web_client.PrepareErrorPage(&web_state, GURL(kTestUrl), error,
+                              /*is_post=*/false,
+                              /*is_off_the_record=*/false,
+                              /*info=*/base::Optional<net::SSLInfo>(),
+                              /*navigation_id=*/0, std::move(callback));
+
+  EXPECT_TRUE(callback_called);
+  NSString* close_page_string =
+      l10n_util::GetNSString(IDS_LOOKALIKE_URL_CLOSE_PAGE);
+  NSString* back_to_safety_string =
+      l10n_util::GetNSString(IDS_LOOKALIKE_URL_BACK_TO_SAFETY);
+  EXPECT_TRUE([page containsString:close_page_string])
+      << base::SysNSStringToUTF8(page);
+  EXPECT_FALSE([page containsString:back_to_safety_string])
+      << base::SysNSStringToUTF8(page);
+}
+
 // Tests the default user agent for different views.
 TEST_F(ChromeWebClientTest, DefaultUserAgent) {
   if (@available(iOS 13, *)) {

ios/components/security_interstitials/ios_blocking_page_controller_client.h

@@ -36,6 +36,7 @@ class IOSBlockingPageControllerClient
   // security_interstitials::ControllerClient implementation.
   void Proceed() override;
   void GoBack() override;
+  bool CanGoBack() override;
 
   void SetWebInterstitial(web::WebInterstitial* web_interstitial);
 
@@ -44,10 +45,6 @@ class IOSBlockingPageControllerClient
 
   const std::string& GetApplicationLocale() const override;
 
-  // Closes the tab. Called in cases where a user clicks "Back to safety" and
-  // it's not possible to go back.
-  void Close();
-
  protected:
   // The WebState passed on initialization.
   web::WebState* web_state() const { return web_state_; }
@@ -55,7 +52,6 @@ class IOSBlockingPageControllerClient
   // security_interstitials::ControllerClient implementation.
   bool CanLaunchDateAndTimeSettings() override;
   void LaunchDateAndTimeSettings() override;
-  bool CanGoBack() override;
   bool CanGoBackBeforeNavigation() override;
   void GoBackAfterNavigationCommitted() override;
   void Reload() override;
@@ -65,6 +61,10 @@ class IOSBlockingPageControllerClient
   const std::string GetExtendedReportingPrefName() const override;
 
  private:
+  // Closes the tab. Called in cases where a user clicks "Back to safety" and
+  // it's not possible to go back.
+  void Close();
+
   web::WebState* web_state_;
   web::WebInterstitial* web_interstitial_;
   const std::string app_locale_;

ios/components/security_interstitials/lookalikes/lookalike_url_blocking_page.h

@@ -45,7 +45,7 @@ class LookalikeUrlBlockingPage
   web::WebState* web_state_ = nullptr;
   std::unique_ptr<LookalikeUrlControllerClient> controller_;
   // The URL suggested to the user as the safe URL. Can be empty, in which case
-  // the default action on the interstitial is to close the tab.
+  // the default action on the interstitial is to go back or close the tab.
   const GURL safe_url_;
   ukm::SourceId source_id_;
   LookalikeUrlMatchType match_type_;

ios/components/security_interstitials/lookalikes/lookalike_url_blocking_page.mm

@@ -40,7 +40,12 @@ LookalikeUrlBlockingPage::LookalikeUrlBlockingPage(
   // chrome://interstitials) leaks memory, so don't create it here.
 }
 
-LookalikeUrlBlockingPage::~LookalikeUrlBlockingPage() = default;
+LookalikeUrlBlockingPage::~LookalikeUrlBlockingPage() {
+  // Update metrics when the interstitial is closed or user navigates away.
+  ReportUkmForLookalikeUrlBlockingPageIfNeeded(
+      source_id_, match_type_,
+      LookalikeUrlBlockingPageUserAction::kCloseOrBack);
+}
 
 bool LookalikeUrlBlockingPage::ShouldCreateNewNavigation() const {
   return true;
@@ -50,6 +55,13 @@ void LookalikeUrlBlockingPage::PopulateInterstitialStrings(
     base::DictionaryValue* load_time_data) const {
   CHECK(load_time_data);
 
+  // Set a value if backwards navigation is not available, used
+  // to change the button text to 'Close page' when there is no
+  // suggested URL.
+  if (!controller_->CanGoBack()) {
+    load_time_data->SetBoolean("cant_go_back", true);
+  }
+
   PopulateLookalikeUrlBlockingPageStrings(load_time_data, safe_url_);
 }
 
@@ -84,13 +96,7 @@ void LookalikeUrlBlockingPage::HandleScriptCommand(
     ReportUkmForLookalikeUrlBlockingPageIfNeeded(
         source_id_, match_type_,
         LookalikeUrlBlockingPageUserAction::kAcceptSuggestion);
-    // If the interstitial doesn't have a suggested URL (e.g. punycode
-    // interstitial), close the tab.
-    if (!safe_url_.is_valid()) {
-      controller_->Close();
-    } else {
       controller_->GoBack();
-    }
   } else if (command == security_interstitials::CMD_PROCEED) {
     controller_->metrics_helper()->RecordUserDecision(
         security_interstitials::MetricsHelper::PROCEED);

ios/components/security_interstitials/lookalikes/lookalike_url_blocking_page_unittest.mm

@@ -109,11 +109,35 @@ TEST_F(LookalikeUrlBlockingPageTest, HandleDontProceedCommand) {
   EXPECT_EQ(web_state_.GetVisibleURL(), safe_url);
 }
 
+// Tests that the blocking page handles the don't proceed command by going back
+// if there is no safe NavigationItem to navigate to.
+TEST_F(LookalikeUrlBlockingPageTest,
+       HandleDontProceedCommandWithoutSafeUrlGoBack) {
+  // Insert a safe navigation so that the page can navigate back to safety, then
+  // add a navigation for the committed interstitial page.
+  GURL safe_url("https://www.safe.test");
+  navigation_manager_->AddItem(safe_url, ui::PAGE_TRANSITION_TYPED);
+  navigation_manager_->AddItem(url_, ui::PAGE_TRANSITION_LINK);
+  ASSERT_EQ(1, navigation_manager_->GetLastCommittedItemIndex());
+  ASSERT_TRUE(navigation_manager_->CanGoBack());
+
+  page_ = CreateBlockingPage(&web_state_, GURL::EmptyGURL(), url_);
+
+  // Send the don't proceed command.
+  SendCommand(security_interstitials::CMD_DONT_PROCEED);
+
+  // Verify that the NavigationManager has navigated back.
+  EXPECT_EQ(0, navigation_manager_->GetLastCommittedItemIndex());
+  EXPECT_FALSE(navigation_manager_->CanGoBack());
+}
+
 // Tests that the blocking page handles the don't proceed command by closing the
-// WebState if there is no safe NavigationItem to navigate back to.
-TEST_F(LookalikeUrlBlockingPageTest, HandleDontProceedCommandWithoutSafeUrl) {
-  GURL empty_safe_url("");
-  page_ = CreateBlockingPage(&web_state_, empty_safe_url, url_);
+// WebState if there is no safe NavigationItem to navigate to and unable to go
+// back.
+TEST_F(LookalikeUrlBlockingPageTest,
+       HandleDontProceedCommandWithoutSafeUrlClose) {
+  page_ = CreateBlockingPage(&web_state_, GURL::EmptyGURL(), url_);
+  ASSERT_FALSE(navigation_manager_->CanGoBack());
 
   // Send the don't proceed command.
   SendCommand(security_interstitials::CMD_DONT_PROCEED);

ios/components/security_interstitials/lookalikes/lookalike_url_controller_client.h

@@ -5,7 +5,6 @@
 #ifndef IOS_COMPONENTS_SECURITY_INTERSTITIALS_LOOKALIKES_LOOKALIKE_URL_CONTROLLER_CLIENT_H_
 #define IOS_COMPONENTS_SECURITY_INTERSTITIALS_LOOKALIKES_LOOKALIKE_URL_CONTROLLER_CLIENT_H_
 
-#include "base/memory/weak_ptr.h"
 #include "ios/components/security_interstitials/ios_blocking_page_controller_client.h"
 #include "url/gurl.h"
 
@@ -29,17 +28,12 @@ class LookalikeUrlControllerClient
   void Proceed() override;
   void GoBack() override;
 
-  // Closes the tab. Called in cases where there is no suggested URL.
-  void Close();
-
  private:
   // The URL suggested to the user as the safe URL. Can be empty, in which case
   // the default action on the interstitial closes the tab.
   const GURL safe_url_;
   // The URL of the page causing the insterstitial.
   const GURL request_url_;
-
-  base::WeakPtrFactory<LookalikeUrlControllerClient> weak_factory_;
 };
 
 #endif  // IOS_COMPONENTS_SECURITY_INTERSTITIALS_LOOKALIKES_LOOKALIKE_URL_CONTROLLER_CLIENT_H_

ios/components/security_interstitials/lookalikes/lookalike_url_controller_client.mm

@@ -4,13 +4,9 @@
 
 #import "ios/components/security_interstitials/lookalikes/lookalike_url_controller_client.h"
 
-#include "base/bind.h"
-#include "base/task/post_task.h"
 #include "components/security_interstitials/core/metrics_helper.h"
 #include "ios/components/security_interstitials/ios_blocking_page_metrics_helper.h"
 #include "ios/components/security_interstitials/lookalikes/lookalike_url_tab_allow_list.h"
-#include "ios/web/public/thread/web_task_traits.h"
-#include "ios/web/public/thread/web_thread.h"
 #import "ios/web/public/web_state.h"
 
 #if !defined(__has_feature) || !__has_feature(objc_arc)
@@ -38,14 +34,23 @@ LookalikeUrlControllerClient::LookalikeUrlControllerClient(
           CreateMetricsHelper(web_state, request_url),
           app_locale),
       safe_url_(safe_url),
-      request_url_(request_url),
-      weak_factory_(this) {}
+      request_url_(request_url) {}
 
 LookalikeUrlControllerClient::~LookalikeUrlControllerClient() {}
 
 void LookalikeUrlControllerClient::GoBack() {
-  // Instead of a 'go back' option, redirect to the legitimate site.
-  OpenUrlInCurrentTab(safe_url_);
+  // If the interstitial doesn't have a suggested URL (e.g. punycode
+  // interstitial), either go back or close the tab. If there is a
+  // suggested URL, instead of a 'go back' option, redirect to the
+  // legitimate site.
+  if (!safe_url_.is_valid()) {
+    IOSBlockingPageControllerClient::GoBack();
+  } else {
+    // TODO(crbug.com/1058898): Replace the last committed navigation
+    // (the interstitial) with the safe URL navigation to prevent the
+    // back button from returning to the bad site.
+    OpenUrlInCurrentTab(safe_url_);
+  }
 }
 
 void LookalikeUrlControllerClient::Proceed() {
@@ -53,12 +58,3 @@ void LookalikeUrlControllerClient::Proceed() {
       ->AllowDomain(request_url_.host());
   Reload();
 }
-
-void LookalikeUrlControllerClient::Close() {
-  // Closing the tab synchronously is problematic since web state is heavily
-  // involved in the operation and CloseWebState interrupts it, so call
-  // CloseWebState asynchronously.
-  base::PostTask(FROM_HERE, {web::WebThread::UI},
-                 base::BindOnce(&IOSBlockingPageControllerClient::Close,
-                                weak_factory_.GetWeakPtr()));
-}

ios/components/security_interstitials/lookalikes/lookalike_url_tab_helper.mm

@@ -50,6 +50,7 @@ void LookalikeUrlTabHelper::ShouldAllowResponse(
     return;
   }
 
+  // TODO(crbug.com/1058898): Create container and ReleaseInterstitialParams.
   // Get stored interstitial parameters early. Doing so ensures that a
   // navigation to an irrelevant (for this interstitial's purposes) URL such as
   // chrome://settings while the lookalike interstitial is being shown clears
@@ -60,10 +61,6 @@ void LookalikeUrlTabHelper::ShouldAllowResponse(
   // If, after this, the user somehow ends up on site.tld with a reload (e.g.
   // with ReloadType::ORIGINAL_REQUEST_URL), this will correctly not show an
   // interstitial.
-  LookalikeUrlContainer* lookalike_container =
-      LookalikeUrlContainer::FromWebState(web_state());
-  std::unique_ptr<LookalikeUrlContainer::InterstitialParams>
-      interstitial_params = lookalike_container->ReleaseInterstitialParams();
 
   GURL response_url = net::GURLWithNSURL(response.URL);
 
@@ -81,6 +78,11 @@ void LookalikeUrlTabHelper::ShouldAllowResponse(
     return;
   }
 
+  // TODO(crbug.com/1058898): If this is a reload and if the current
+  // URL is the last URL of the stored redirect chain, the interstitial
+  // was probably reloaded. Stop the reload and navigate back to the
+  // original lookalike URL so that the full checks are exercised again.
+
   const DomainInfo navigated_domain = GetDomainInfo(response_url);
   // Empty domain_and_registry happens on private domains.
   if (navigated_domain.domain_and_registry.empty() ||
@@ -115,6 +117,8 @@ void LookalikeUrlTabHelper::ShouldAllowResponse(
     replace_host.SetHostStr(suggested_domain);
     const GURL suggested_url =
         response_url.ReplaceComponents(replace_host).GetWithEmptyPath();
+    LookalikeUrlContainer* lookalike_container =
+        LookalikeUrlContainer::FromWebState(web_state());
     lookalike_container->SetLookalikeUrlInfo(suggested_url, response_url,
                                              match_type);