Change Reader Mode to only run on SECURE pages for experiment on Desktop.

HTTP pages can change from WARNING to DANGEROUS if you interact with a
form. There is some chance that a Reader Mode page could load a form, and
Reader Mode doesn't have a good way to convey this security state change,
so limiting Reader Mode to SECURE pages avoids this case.

It is hard to communicate to users the difference between SECURE,
WARNING and Reader Mode in a single security chip, so it is easier to
ensure that pages will be SECURE. (A future CL will avoid security
state changes for SECURE pages by disallowing loading of insecure
resources.)

Also allows Android reader mode to continue running on all pages
as it has done already (feature launched on Android). Android was
restricted to not run on DANGEROUS pages in
https://chromium-review.googlesource.com/c/chromium/src/+/2111075,
but that should only have applied to desktop.

Bug: 1069907
Change-Id: I7895e06132508e5cd37509d890140de167f25f7b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2146077
Commit-Queue: Katie Dektar <katie@chromium.org>
Reviewed-by: Charlie Reis <creis@chromium.org>
Reviewed-by: Aran Gilman <gilmanmh@google.com>
Reviewed-by: Emily Stark <estark@chromium.org>
Cr-Commit-Position: refs/heads/master@{#760602}

chrome/browser/chrome_browser_interface_binders.cc

@@ -225,13 +225,13 @@ void BindDistillabilityService(
       dom_distiller::DistillabilityDriver::FromWebContents(web_contents);
   if (!driver)
     return;
-  driver->SetIsDangerousCallback(
+  driver->SetIsSecureCallback(
       base::BindRepeating([](content::WebContents* contents) {
         // SecurityStateTabHelper uses chrome-specific
-        // GetVisibleSecurityState to determine if a page is DANGEROUS.
+        // GetVisibleSecurityState to determine if a page is SECURE.
         return SecurityStateTabHelper::FromWebContents(contents)
-                   ->GetSecurityLevel() !=
-               security_state::SecurityLevel::DANGEROUS;
+                   ->GetSecurityLevel() ==
+               security_state::SecurityLevel::SECURE;
       }));
   driver->CreateDistillabilityService(std::move(receiver));
 }

chrome/browser/dom_distiller/distillable_page_utils_browsertest.cc

@@ -82,7 +82,10 @@ class TestOption : public InProcessBrowserTest {
   }
 
   void SetUpOnMainThread() override {
-    ASSERT_TRUE(embedded_test_server()->Start());
+    https_server_ = std::make_unique<net::EmbeddedTestServer>(
+        net::EmbeddedTestServer::TYPE_HTTPS);
+    https_server_->ServeFilesFromSourceDirectory(GetChromeTestDataDir());
+    ASSERT_TRUE(https_server_->Start());
     web_contents_ = browser()->tab_strip_model()->GetActiveWebContents();
     AddObserver(web_contents_, &holder_);
   }
@@ -92,7 +95,7 @@ class TestOption : public InProcessBrowserTest {
 
     GURL article_url(url);
     if (base::StartsWith(url, "/", base::CompareCase::SENSITIVE)) {
-      article_url = embedded_test_server()->GetURL(url);
+      article_url = https_server()->GetURL(url);
     }
 
     // This blocks until the navigation has completely finished.
@@ -114,9 +117,14 @@ class TestOption : public InProcessBrowserTest {
         FROM_HERE, run_loop_->QuitClosure(), delta);
   }
 
+  net::test_server::EmbeddedTestServer* https_server() {
+    return https_server_.get();
+  }
+
   std::unique_ptr<base::RunLoop> run_loop_;
   MockObserver holder_;
   content::WebContents* web_contents_ = nullptr;
+  std::unique_ptr<net::test_server::EmbeddedTestServer> https_server_;
 };
 
 MATCHER(IsDistillable,

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

@@ -2,6 +2,7 @@
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
+#include <memory>
 #include "base/macros.h"
 #include "chrome/browser/ui/views/reader_mode/reader_mode_icon_view.h"
 
@@ -24,6 +25,7 @@
 #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/browser_test_utils.h"
 #include "content/public/test/test_navigation_observer.h"
 #include "testing/gtest/include/gtest/gtest.h"
 #include "ui/views/test/button_test_api.h"
@@ -38,10 +40,13 @@ class ReaderModeIconViewBrowserTest : public InProcessBrowserTest {
  protected:
   ReaderModeIconViewBrowserTest() {
     feature_list_.InitAndEnableFeature(dom_distiller::kReaderMode);
+    https_server_secure_ = std::make_unique<net::EmbeddedTestServer>(
+        net::EmbeddedTestServer::TYPE_HTTPS);
+    https_server_secure_->ServeFilesFromSourceDirectory(GetChromeTestDataDir());
   }
 
   void SetUpOnMainThread() override {
-    ASSERT_TRUE(embedded_test_server()->Start());
+    ASSERT_TRUE(https_server_secure()->Start());
     reader_mode_icon_ =
         BrowserView::GetBrowserViewForBrowser(browser())
             ->toolbar_button_provider()
@@ -49,7 +54,12 @@ class ReaderModeIconViewBrowserTest : public InProcessBrowserTest {
     ASSERT_NE(nullptr, reader_mode_icon_);
   }
 
+  net::EmbeddedTestServer* https_server_secure() {
+    return https_server_secure_.get();
+  }
+
   PageActionIconView* reader_mode_icon_;
+  std::unique_ptr<net::EmbeddedTestServer> https_server_secure_;
 
  private:
   base::test::ScopedFeatureList feature_list_;
@@ -77,7 +87,7 @@ IN_PROC_BROWSER_TEST_F(ReaderModeIconViewBrowserTest,
 
   // The icon should be hidden on pages that aren't distillable
   ui_test_utils::NavigateToURL(browser(),
-                               embedded_test_server()->GetURL(kNonArticlePath));
+                               https_server_secure()->GetURL(kNonArticlePath));
   observer.WaitForResult(expected_result);
   const bool is_visible_on_non_distillable_page =
       reader_mode_icon_->GetVisible();
@@ -85,7 +95,7 @@ IN_PROC_BROWSER_TEST_F(ReaderModeIconViewBrowserTest,
 
   // The icon should appear after navigating to a distillable article.
   ui_test_utils::NavigateToURL(
-      browser(), embedded_test_server()->GetURL(kSimpleArticlePath));
+      browser(), https_server_secure()->GetURL(kSimpleArticlePath));
   expected_result.is_distillable = true;
   observer.WaitForResult(expected_result);
   const bool is_visible_on_article = reader_mode_icon_->GetVisible();
@@ -93,7 +103,7 @@ IN_PROC_BROWSER_TEST_F(ReaderModeIconViewBrowserTest,
 
   // Navigating back to a non-distillable page hides the icon again.
   ui_test_utils::NavigateToURL(browser(),
-                               embedded_test_server()->GetURL(kNonArticlePath));
+                               https_server_secure()->GetURL(kNonArticlePath));
   expected_result.is_distillable = false;
   observer.WaitForResult(expected_result);
   const bool is_visible_after_navigation_back_to_non_distillable_page =
@@ -136,14 +146,14 @@ IN_PROC_BROWSER_TEST_F(ReaderModeIconViewBrowserTestWithSettings,
   // The icon should not appear after navigating to a distillable article,
   // because the setting to offer reader mode is disabled.
   ui_test_utils::NavigateToURL(
-      browser(), embedded_test_server()->GetURL(kSimpleArticlePath));
+      browser(), https_server_secure()->GetURL(kSimpleArticlePath));
   observer.WaitForResult(expected_result);
   bool is_visible_on_article = reader_mode_icon_->GetVisible();
   EXPECT_FALSE(is_visible_on_article);
 
   // It continues to not show up when navigating to a non-distillable page.
   ui_test_utils::NavigateToURL(browser(),
-                               embedded_test_server()->GetURL(kNonArticlePath));
+                               https_server_secure()->GetURL(kNonArticlePath));
   expected_result.is_distillable = false;
   observer.WaitForResult(expected_result);
   bool is_visible_after_navigation_back_to_non_distillable_page =
@@ -154,7 +164,7 @@ IN_PROC_BROWSER_TEST_F(ReaderModeIconViewBrowserTestWithSettings,
   // distillable page.
   SetOfferReaderModeSetting(true);
   ui_test_utils::NavigateToURL(
-      browser(), embedded_test_server()->GetURL(kSimpleArticlePath));
+      browser(), https_server_secure()->GetURL(kSimpleArticlePath));
   expected_result.is_distillable = true;
   observer.WaitForResult(expected_result);
   is_visible_on_article = reader_mode_icon_->GetVisible();
@@ -162,7 +172,7 @@ IN_PROC_BROWSER_TEST_F(ReaderModeIconViewBrowserTestWithSettings,
 
   // But it still turns off when navigating to a non-distillable page.
   ui_test_utils::NavigateToURL(browser(),
-                               embedded_test_server()->GetURL(kNonArticlePath));
+                               https_server_secure()->GetURL(kNonArticlePath));
   expected_result.is_distillable = false;
   observer.WaitForResult(expected_result);
   is_visible_after_navigation_back_to_non_distillable_page =
@@ -171,7 +181,7 @@ IN_PROC_BROWSER_TEST_F(ReaderModeIconViewBrowserTestWithSettings,
 }
 
 IN_PROC_BROWSER_TEST_F(ReaderModeIconViewBrowserTest,
-                       DangerousPagesNotDistillable) {
+                       NonSecurePagesNotDistillable) {
   std::unique_ptr<net::EmbeddedTestServer> https_server_expired;
   https_server_expired.reset(
       new net::EmbeddedTestServer(net::EmbeddedTestServer::TYPE_HTTPS));
@@ -188,14 +198,23 @@ IN_PROC_BROWSER_TEST_F(ReaderModeIconViewBrowserTest,
   expected_result.is_last = false;
   expected_result.is_mobile_friendly = false;
 
-  // Check test setup by ensuring the icon is shown with the normal test
+  // Check test setup by ensuring the icon is shown with the secure test
   // server.
   ui_test_utils::NavigateToURL(
-      browser(), embedded_test_server()->GetURL(kSimpleArticlePath));
-  EXPECT_NE(security_state::DANGEROUS, helper->GetSecurityLevel());
+      browser(), https_server_secure()->GetURL(kSimpleArticlePath));
+  EXPECT_EQ(security_state::SECURE, helper->GetSecurityLevel());
   observer.WaitForResult(expected_result);
   EXPECT_TRUE(reader_mode_icon_->GetVisible());
 
+  // The icon should not be shown with a http test server.
+  ASSERT_TRUE(embedded_test_server()->Start());
+  ui_test_utils::NavigateToURL(
+      browser(), embedded_test_server()->GetURL(kSimpleArticlePath));
+  EXPECT_NE(security_state::SECURE, helper->GetSecurityLevel());
+  expected_result.is_distillable = false;
+  observer.WaitForResult(expected_result);
+  EXPECT_FALSE(reader_mode_icon_->GetVisible());
+
   // Set security state to DANGEROUS for the test by using an expired
   // certificate.
   ASSERT_TRUE(https_server_expired->Start());
@@ -221,7 +240,6 @@ IN_PROC_BROWSER_TEST_F(ReaderModeIconViewBrowserTest,
 
   // 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);

components/dom_distiller/content/browser/distillability_driver.cc

@@ -8,6 +8,7 @@
 #include <utility>
 
 #include "base/bind.h"
+#include "build/build_config.h"
 #include "content/public/browser/navigation_handle.h"
 #include "content/public/browser/render_frame_host.h"
 #include "content/public/browser/web_contents.h"
@@ -60,15 +61,16 @@ 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::SetIsSecureCallback(
+    base::RepeatingCallback<bool(content::WebContents*)> is_secure_check) {
+  is_secure_check_ = std::move(is_secure_check);
 }
 
 void DistillabilityDriver::OnDistillability(
     const DistillabilityResult& result) {
+#if !defined(OS_ANDROID)
   if (result.is_distillable) {
-    if (!is_dangerous_check_ || !is_dangerous_check_.Run(web_contents_)) {
+    if (!is_secure_check_ || !is_secure_check_.Run(web_contents_)) {
       DistillabilityResult not_distillable;
       not_distillable.is_distillable = false;
       not_distillable.is_last = result.is_last;
@@ -79,6 +81,7 @@ void DistillabilityDriver::OnDistillability(
       return;
     }
   }
+#endif  // !defined(OS_ANDROID)
   latest_result_ = result;
   for (auto& observer : observers_)
     observer.OnResult(result);

components/dom_distiller/content/browser/distillability_driver.h

@@ -34,10 +34,11 @@ class DistillabilityDriver
   }
 
   // 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);
+  // to decide whether it can be distilled. Only SECURE pages are currently
+  // distillable on Desktop. Android does not check page security status
+  // before distilling.
+  void SetIsSecureCallback(
+      base::RepeatingCallback<bool(content::WebContents*)> is_secure_check);
 
   UMAHelper::DistillabilityDriverTimer& GetTimer() { return timer_; }
 
@@ -67,7 +68,7 @@ class DistillabilityDriver
   UMAHelper::DistillabilityDriverTimer timer_;
 
   content::WebContents* web_contents_;
-  base::RepeatingCallback<bool(content::WebContents*)> is_dangerous_check_;
+  base::RepeatingCallback<bool(content::WebContents*)> is_secure_check_;
 
   base::WeakPtrFactory<DistillabilityDriver> weak_factory_{this};
 

components/security_state/core/security_state.cc

@@ -182,8 +182,9 @@ SecurityLevel GetSecurityLevel(
       // Display ReaderMode pages as neutral even if the original URL was
       // secure, because Chrome has modified the content so we don't want to
       // present it as the actual content that the server sent. Distilled pages
-      // do not contain forms, payment handlers, or other JS from the original
-      // URL, so they won't be affected by a downgraded security level.
+      // should not contain forms, payment handlers, or other JS from the
+      // original URL, so they won't be affected by a downgraded security level.
+      // Reader Mode is only run on SECURE pages.
       if (visible_security_state.is_reader_mode) {
         return NONE;
       }