Reader mode is not offered on DANGEROUS pages.

When a page is detected as DANGEROUS, dom_distiller does not
send positive distillability results even if pages are technically
distillable.

Bug: 1061055
Change-Id: I12e902173770ff5335f85d4e794afb3508dae04c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2111075
Commit-Queue: Katie Dektar <katie@chromium.org>
Reviewed-by: Emily Stark <estark@chromium.org>
Reviewed-by: Wei-Yin Chen (陳威尹) <wychen@chromium.org>
Reviewed-by: Elly Fong-Jones <ellyjones@chromium.org>
Cr-Commit-Position: refs/heads/master@{#752654}

chrome/browser/chrome_browser_interface_binders.cc

@@ -25,6 +25,7 @@
 #include "chrome/browser/soda/soda_service.h"
 #include "chrome/browser/soda/soda_service_factory.h"
 #include "chrome/browser/ssl/insecure_sensitive_input_driver_factory.h"
+#include "chrome/browser/ssl/security_state_tab_helper.h"
 #include "chrome/browser/ui/webui/bluetooth_internals/bluetooth_internals.mojom.h"
 #include "chrome/browser/ui/webui/bluetooth_internals/bluetooth_internals_ui.h"
 #include "chrome/browser/ui/webui/engagement/site_engagement_ui.h"
@@ -47,6 +48,8 @@
 #include "components/performance_manager/performance_manager_tab_helper.h"
 #include "components/performance_manager/public/mojom/coordination_unit.mojom.h"
 #include "components/safe_browsing/buildflags.h"
+#include "components/security_state/content/content_utils.h"
+#include "components/security_state/core/security_state.h"
 #include "components/translate/content/common/translate.mojom.h"
 #include "content/public/browser/browser_context.h"
 #include "content/public/browser/render_process_host.h"
@@ -216,6 +219,14 @@ void BindDistillabilityService(
       dom_distiller::DistillabilityDriver::FromWebContents(web_contents);
   if (!driver)
     return;
+  driver->SetIsDangerousCallback(
+      base::BindRepeating([](content::WebContents* contents) {
+        // SecurityStateTabHelper uses chrome-specific
+        // GetVisibleSecurityState to determine if a page is DANGEROUS.
+        return SecurityStateTabHelper::FromWebContents(contents)
+                   ->GetSecurityLevel() !=
+               security_state::SecurityLevel::DANGEROUS;
+      }));
   driver->CreateDistillabilityService(std::move(receiver));
 }
 

chrome/browser/ui/views/reader_mode/reader_mode_icon_view_browsertest.cc

@@ -6,6 +6,8 @@
 #include "chrome/browser/ui/views/reader_mode/reader_mode_icon_view.h"
 
 #include "base/test/scoped_feature_list.h"
+#include "chrome/browser/interstitials/security_interstitial_page_test_utils.h"
+#include "chrome/browser/ssl/security_state_tab_helper.h"
 #include "chrome/browser/ui/page_action/page_action_icon_type.h"
 #include "chrome/browser/ui/views/frame/browser_view.h"
 #include "chrome/browser/ui/views/frame/toolbar_button_provider.h"
@@ -16,8 +18,12 @@
 #include "components/dom_distiller/core/dom_distiller_features.h"
 #include "components/dom_distiller/core/dom_distiller_switches.h"
 #include "components/dom_distiller/core/pref_names.h"
+#include "components/security_interstitials/content/security_interstitial_page.h"
+#include "components/security_interstitials/content/security_interstitial_tab_helper.h"
+#include "components/security_interstitials/core/controller_client.h"
 #include "content/public/browser/web_contents.h"
 #include "content/public/test/browser_test.h"
+#include "content/public/test/test_navigation_observer.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/views/test/button_test_api.h"
 
@@ -25,6 +31,7 @@ namespace {
 
 const char* kSimpleArticlePath = "/dom_distiller/simple_article.html";
 const char* kNonArticlePath = "/dom_distiller/non_og_article.html";
+const char* kArticleTitle = "Test Page Title";
 
 class TestDistillabilityObserver
     : public dom_distiller::DistillabilityObserver {
@@ -226,4 +233,62 @@ IN_PROC_BROWSER_TEST_F(ReaderModeIconViewBrowserTestWithSettings,
   EXPECT_FALSE(is_visible_after_navigation_back_to_non_distillable_page);
 }
 
+IN_PROC_BROWSER_TEST_F(ReaderModeIconViewBrowserTest,
+                       DangerousPagesNotDistillable) {
+  std::unique_ptr<net::EmbeddedTestServer> https_server_expired;
+  https_server_expired.reset(
+      new net::EmbeddedTestServer(net::EmbeddedTestServer::TYPE_HTTPS));
+  https_server_expired->SetSSLConfig(net::EmbeddedTestServer::CERT_EXPIRED);
+  https_server_expired->ServeFilesFromSourceDirectory(GetChromeTestDataDir());
+  content::WebContents* web_contents =
+      browser()->tab_strip_model()->GetActiveWebContents();
+  SecurityStateTabHelper* helper =
+      SecurityStateTabHelper::FromWebContents(web_contents);
+
+  TestDistillabilityObserver observer(web_contents);
+  dom_distiller::DistillabilityResult expected_result;
+  expected_result.is_distillable = true;
+  expected_result.is_last = false;
+  expected_result.is_mobile_friendly = false;
+
+  // Check test setup by ensuring the icon is shown with the normal test
+  // server.
+  ui_test_utils::NavigateToURL(
+      browser(), embedded_test_server()->GetURL(kSimpleArticlePath));
+  EXPECT_NE(security_state::DANGEROUS, helper->GetSecurityLevel());
+  observer.WaitForResult(expected_result);
+  EXPECT_TRUE(reader_mode_icon_->GetVisible());
+
+  // Set security state to DANGEROUS for the test by using an expired
+  // certificate.
+  ASSERT_TRUE(https_server_expired->Start());
+  content::TitleWatcher title_watcher(web_contents,
+                                      base::ASCIIToUTF16(kArticleTitle));
+  ui_test_utils::NavigateToURL(
+      browser(), https_server_expired->GetURL(kSimpleArticlePath));
+
+  // Proceed through the intersitial warning page.
+  web_contents = browser()->tab_strip_model()->GetActiveWebContents();
+  content::TestNavigationObserver nav_observer(web_contents, 1);
+  security_interstitials::SecurityInterstitialTabHelper* interstitial_helper =
+      security_interstitials::SecurityInterstitialTabHelper::FromWebContents(
+          web_contents);
+  interstitial_helper
+      ->GetBlockingPageForCurrentlyCommittedNavigationForTesting()
+      ->CommandReceived(
+          base::NumberToString(security_interstitials::CMD_PROCEED));
+  nav_observer.Wait();
+
+  // Check we are on the right page.
+  ASSERT_EQ(base::ASCIIToUTF16(kArticleTitle), title_watcher.WaitAndGetTitle());
+
+  // Check security state is DANGEROUS per https_server_expired_.
+  EXPECT_EQ(security_state::DANGEROUS, helper->GetSecurityLevel());
+  expected_result.is_distillable = false;
+
+  // The page should not be distillable.
+  observer.WaitForResult(expected_result);
+  EXPECT_FALSE(reader_mode_icon_->GetVisible());
+}
+
 }  // namespace

components/dom_distiller/content/browser/distillability_driver.cc

@@ -46,7 +46,7 @@ class DistillabilityServiceImpl : public mojom::DistillabilityService {
 };
 
 DistillabilityDriver::DistillabilityDriver(content::WebContents* web_contents)
-    : latest_result_(base::nullopt) {
+    : latest_result_(base::nullopt), web_contents_(web_contents) {
   if (!web_contents)
     return;
 }
@@ -60,8 +60,25 @@ void DistillabilityDriver::CreateDistillabilityService(
       std::move(receiver));
 }
 
+void DistillabilityDriver::SetIsDangerousCallback(
+    base::RepeatingCallback<bool(content::WebContents*)> is_dangerous_check) {
+  is_dangerous_check_ = std::move(is_dangerous_check);
+}
+
 void DistillabilityDriver::OnDistillability(
     const DistillabilityResult& result) {
+  if (result.is_distillable) {
+    if (!is_dangerous_check_ || !is_dangerous_check_.Run(web_contents_)) {
+      DistillabilityResult not_distillable;
+      not_distillable.is_distillable = false;
+      not_distillable.is_last = result.is_last;
+      not_distillable.is_mobile_friendly = result.is_mobile_friendly;
+      latest_result_ = not_distillable;
+      for (auto& observer : observers_)
+        observer.OnResult(not_distillable);
+      return;
+    }
+  }
   latest_result_ = result;
   for (auto& observer : observers_)
     observer.OnResult(result);

components/dom_distiller/content/browser/distillability_driver.h

@@ -34,6 +34,12 @@ class DistillabilityDriver
     return latest_result_;
   }
 
+  // Sets a callback which can be used to determine the security of a page,
+  // to decide whether it can be distilled. DANGEROUS pages are never
+  // distillable.
+  void SetIsDangerousCallback(
+      base::RepeatingCallback<bool(content::WebContents*)> is_dangerous_check);
+
   UMAHelper::DistillabilityDriverTimer& GetTimer() { return timer_; }
 
  private:
@@ -58,6 +64,9 @@ class DistillabilityDriver
   // metrics for the ReaderMode experiment.
   UMAHelper::DistillabilityDriverTimer timer_;
 
+  content::WebContents* web_contents_;
+  base::RepeatingCallback<bool(content::WebContents*)> is_dangerous_check_;
+
   base::WeakPtrFactory<DistillabilityDriver> weak_factory_{this};
 
   WEB_CONTENTS_USER_DATA_KEY_DECL();