Downgrade security state while displaying an SB interstitial

BUG=615123
TBR=jialiul@chromium.org
TEST=
1. Go to the following URLs in different tabs:
   http://ianfette.org
   http://parkerly.com/sb-tests/bad-subresources.html
   https://parkerly.com/sb-tests/bad-subresources.html
2. Verify that the omnibox security indicator shows a red triangle for
   all three warnings

Review-Url: https://codereview.chromium.org/2275123004
Cr-Commit-Position: refs/heads/master@{#414700}

chrome/browser/safe_browsing/safe_browsing_blocking_page_test.cc

@@ -571,6 +571,18 @@ class SafeBrowsingBlockingPageBrowserTest
     EXPECT_EQ(expected_tag_name, actual_resource.tag_name());
   }
 
+  void ExpectSecurityIndicatorDowngrade(content::WebContents* tab,
+                                        net::CertStatus cert_status) {
+    ChromeSecurityStateModelClient* model_client =
+        ChromeSecurityStateModelClient::FromWebContents(tab);
+    ASSERT_TRUE(model_client);
+    EXPECT_EQ(security_state::SecurityStateModel::SECURITY_ERROR,
+              model_client->GetSecurityInfo().security_level);
+    EXPECT_TRUE(model_client->GetSecurityInfo().fails_malware_check);
+    // TODO(felt): Restore this check when https://crbug.com/641187 is fixed.
+    // EXPECT_EQ(cert_status, model_client->GetSecurityInfo().cert_status);
+  }
+
  protected:
   TestThreatDetailsFactory details_factory_;
 
@@ -1049,59 +1061,56 @@ IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageBrowserTest, WhitelistUnsaved) {
 // Safe Browsing interstitial.
 IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageBrowserTest,
                        SecurityState_HTTP) {
+  // The security indicator should be downgraded while the interstitial shows.
   SetupWarningAndNavigate();
+  WebContents* error_tab = browser()->tab_strip_model()->GetActiveWebContents();
+  ASSERT_TRUE(error_tab);
+  ExpectSecurityIndicatorDowngrade(error_tab, 0u);
+
+  // The security indicator should still be downgraded post-interstitial.
   EXPECT_TRUE(ClickAndWaitForDetach("proceed-link"));
   AssertNoInterstitial(true);
-
-  WebContents* tab = browser()->tab_strip_model()->GetActiveWebContents();
-  ASSERT_TRUE(tab);
-  ChromeSecurityStateModelClient* model_client =
-      ChromeSecurityStateModelClient::FromWebContents(tab);
-  ASSERT_TRUE(model_client);
-  EXPECT_EQ(security_state::SecurityStateModel::SECURITY_ERROR,
-            model_client->GetSecurityInfo().security_level);
-  EXPECT_TRUE(model_client->GetSecurityInfo().fails_malware_check);
+  WebContents* post_tab = browser()->tab_strip_model()->GetActiveWebContents();
+  ASSERT_TRUE(post_tab);
+  ExpectSecurityIndicatorDowngrade(post_tab, 0u);
 }
 
 // Test that the security indicator is downgraded even if the website has valid
 // HTTPS (meaning that the SB state overrides the HTTPS state).
 IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageBrowserTest,
                        SecurityState_ValidHTTPS) {
+  // The security indicator should be downgraded while the interstitial shows.
   SetupWarningAndNavigateToValidHTTPS();
+  WebContents* error_tab = browser()->tab_strip_model()->GetActiveWebContents();
+  ASSERT_TRUE(error_tab);
+  ExpectSecurityIndicatorDowngrade(error_tab, 0u);
+
+  // The security indicator should still be downgraded post-interstitial.
   EXPECT_TRUE(ClickAndWaitForDetach("proceed-link"));
   AssertNoInterstitial(true);
-
-  WebContents* tab = browser()->tab_strip_model()->GetActiveWebContents();
-  ASSERT_TRUE(tab);
-  ChromeSecurityStateModelClient* model_client =
-      ChromeSecurityStateModelClient::FromWebContents(tab);
-  ASSERT_TRUE(model_client);
-  EXPECT_EQ(security_state::SecurityStateModel::SECURITY_ERROR,
-            model_client->GetSecurityInfo().security_level);
-  EXPECT_TRUE(model_client->GetSecurityInfo().fails_malware_check);
-  EXPECT_EQ(0u, model_client->GetSecurityInfo().cert_status);
+  WebContents* post_tab = browser()->tab_strip_model()->GetActiveWebContents();
+  ASSERT_TRUE(post_tab);
+  ExpectSecurityIndicatorDowngrade(post_tab, 0u);
 }
 
 // Test that the security indicator is still downgraded after two interstitials
 // are shown in a row (one for Safe Browsing, one for invalid HTTPS).
 IN_PROC_BROWSER_TEST_P(SafeBrowsingBlockingPageBrowserTest,
                        SecurityState_InvalidHTTPS) {
+  // The security indicator should be downgraded while the interstitial shows.
   SetupWarningAndNavigateToInvalidHTTPS();
+  WebContents* error_tab = browser()->tab_strip_model()->GetActiveWebContents();
+  ASSERT_TRUE(error_tab);
+  ExpectSecurityIndicatorDowngrade(error_tab, 0u);
+
+  // The security indicator should still be downgraded post-interstitial.
   EXPECT_TRUE(ClickAndWaitForDetach("proceed-link"));
   AssertNoInterstitial(true);
-
-  WebContents* tab = browser()->tab_strip_model()->GetActiveWebContents();
-  ASSERT_TRUE(tab);
-  ChromeSecurityStateModelClient* model_client =
-      ChromeSecurityStateModelClient::FromWebContents(tab);
-  ASSERT_TRUE(model_client);
-  EXPECT_EQ(security_state::SecurityStateModel::SECURITY_ERROR,
-            model_client->GetSecurityInfo().security_level);
-  EXPECT_TRUE(model_client->GetSecurityInfo().fails_malware_check);
-
-  // TODO(felt): In the testing framework, the cert status gets reset to 0
-  // after the malware interstitial and stays that way.
-  //EXPECT_NE(0u, model_client->GetSecurityInfo().cert_status);
+  WebContents* post_tab = browser()->tab_strip_model()->GetActiveWebContents();
+  ASSERT_TRUE(post_tab);
+  // TODO(felt): Sometimes the cert status here is 0u, which is wrong.
+  // Filed https://crbug.com/641187 to investigate.
+  ExpectSecurityIndicatorDowngrade(post_tab, net::CERT_STATUS_INVALID);
 }
 
 INSTANTIATE_TEST_CASE_P(

chrome/browser/safe_browsing/ui_manager.cc

@@ -43,19 +43,32 @@ namespace {
 
 const void* const kWhitelistKey = &kWhitelistKey;
 
+// A WhitelistUrlSet holds the set of URLs that have been whitelisted for a
+// specific WebContents, along with pending entries that are still undecided.
 class WhitelistUrlSet : public base::SupportsUserData::Data {
  public:
   WhitelistUrlSet() {}
 
   bool Contains(const GURL url) {
-    auto iter = set_.find(url.GetWithEmptyPath());
-    return iter != set_.end();
+    return set_.find(url.GetWithEmptyPath()) != set_.end();
   }
 
-  void Insert(const GURL url) { set_.insert(url.GetWithEmptyPath()); }
+  void Insert(const GURL url) {
+    set_.insert(url.GetWithEmptyPath());
+    pending_.erase(url.GetWithEmptyPath());
+  }
+
+  bool ContainsPending(const GURL url) {
+    return pending_.find(url.GetWithEmptyPath()) != pending_.end();
+  }
+
+  void InsertPending(const GURL url) {
+    pending_.insert(url.GetWithEmptyPath());
+  }
 
  private:
   std::set<GURL> set_;
+  std::set<GURL> pending_;
 
   DISALLOW_COPY_AND_ASSIGN(WhitelistUrlSet);
 };
@@ -148,7 +161,7 @@ void SafeBrowsingUIManager::OnBlockingPageDone(
     }
 
     if (proceed)
-      AddToWhitelist(resource);
+      AddToWhitelistUrlSet(resource, false /* Pending -> permanent */);
   }
 }
 
@@ -239,6 +252,7 @@ void SafeBrowsingUIManager::DisplayBlockingPage(
   if (resource.threat_type != SB_THREAT_TYPE_SAFE) {
     FOR_EACH_OBSERVER(Observer, observer_list_, OnSafeBrowsingHit(resource));
   }
+  AddToWhitelistUrlSet(resource, true /* A decision is now pending */);
   SafeBrowsingBlockingPage::ShowBlockingPage(this, resource);
 }
 
@@ -345,9 +359,11 @@ void SafeBrowsingUIManager::SendSerializedThreatDetails(
   }
 }
 
-// Whitelist this domain in the current WebContents. Either add the
-// domain to an existing WhitelistUrlSet, or create a new WhitelistUrlSet.
-void SafeBrowsingUIManager::AddToWhitelist(const UnsafeResource& resource) {
+// Record this domain in the current WebContents as either whitelisted or
+// pending whitelisting (if an interstitial is currently displayed). If an
+// existing WhitelistUrlSet does not yet exist, create a new WhitelistUrlSet.
+void SafeBrowsingUIManager::AddToWhitelistUrlSet(const UnsafeResource& resource,
+                                                 bool pending) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
   WebContents* web_contents = resource.web_contents_getter.Run();
@@ -368,7 +384,11 @@ void SafeBrowsingUIManager::AddToWhitelist(const UnsafeResource& resource) {
     whitelisted_url = resource.url;
   }
 
-  site_list->Insert(whitelisted_url);
+  if (pending) {
+    site_list->InsertPending(whitelisted_url);
+  } else {
+    site_list->Insert(whitelisted_url);
+  }
 }
 
 bool SafeBrowsingUIManager::IsWhitelisted(const UnsafeResource& resource) {
@@ -376,34 +396,41 @@ bool SafeBrowsingUIManager::IsWhitelisted(const UnsafeResource& resource) {
   if (resource.is_subresource) {
     entry = resource.GetNavigationEntryForResource();
   }
-  return IsUrlWhitelistedForWebContents(resource.url, resource.is_subresource,
-                                        entry,
-                                        resource.web_contents_getter.Run());
+  return IsUrlWhitelistedOrPendingForWebContents(
+      resource.url, resource.is_subresource, entry,
+      resource.web_contents_getter.Run(), true);
 }
 
-// Check if the user has already ignored a SB warning for this WebContents and
-// top-level domain.
-bool SafeBrowsingUIManager::IsUrlWhitelistedForWebContents(
+// Check if the user has already seen and/or ignored a SB warning for this
+// WebContents and top-level domain.
+bool SafeBrowsingUIManager::IsUrlWhitelistedOrPendingForWebContents(
     const GURL& url,
     bool is_subresource,
     NavigationEntry* entry,
-    content::WebContents* web_contents) {
+    content::WebContents* web_contents,
+    bool whitelist_only) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
-  GURL maybe_whitelisted_url;
+  GURL lookup_url;
   if (is_subresource) {
     if (!entry)
       return false;
-    maybe_whitelisted_url = entry->GetURL();
+    lookup_url = entry->GetURL();
   } else {
-    maybe_whitelisted_url = url;
+    lookup_url = url;
   }
 
   WhitelistUrlSet* site_list =
       static_cast<WhitelistUrlSet*>(web_contents->GetUserData(kWhitelistKey));
   if (!site_list)
     return false;
-  return site_list->Contains(maybe_whitelisted_url);
+
+  bool whitelisted = site_list->Contains(lookup_url);
+  if (whitelist_only) {
+    return whitelisted;
+  } else {
+    return whitelisted || site_list->ContainsPending(lookup_url);
+  }
 }
 
 }  // namespace safe_browsing

chrome/browser/safe_browsing/ui_manager.h

@@ -125,15 +125,20 @@ class SafeBrowsingUIManager
   // chain). Otherwise, |original_url| = |url|.
   virtual void DisplayBlockingPage(const UnsafeResource& resource);
 
-  // A wrapper method for IsUrlWhitelistedForWebContents, for convenience.
+  // A convenience wrapper method for IsUrlWhitelistedOrPendingForWebContents.
   bool IsWhitelisted(const UnsafeResource& resource);
 
-  // Returns true if we already displayed an interstitial for that top-level
-  // site in a given WebContents. Called on the UI thread.
-  bool IsUrlWhitelistedForWebContents(const GURL& url,
-                                      bool is_subresource,
-                                      content::NavigationEntry* entry,
-                                      content::WebContents* web_contents);
+  // Checks if we already displayed an interstitial for that top-level
+  // site in a given WebContents. If |whitelist_only|, it returns true only if
+  // the user chose to ignore the interstitial; otherwise it returns true as
+  // long as the user has seen an interstitial (regardless of response).
+  // Called on the UI thread.
+  bool IsUrlWhitelistedOrPendingForWebContents(
+      const GURL& url,
+      bool is_subresource,
+      content::NavigationEntry* entry,
+      content::WebContents* web_contents,
+      bool whitelist_only);
 
   // The blocking page on the UI thread has completed.
   void OnBlockingPageDone(const std::vector<UnsafeResource>& resources,
@@ -189,8 +194,8 @@ class SafeBrowsingUIManager
   void ReportPermissionActionOnIOThread(
       const PermissionReportInfo& report_info);
 
-  // Updates the whitelist state.  Called on the UI thread.
-  void AddToWhitelist(const UnsafeResource& resource);
+  // Updates the whitelist URL set.  Called on the UI thread.
+  void AddToWhitelistUrlSet(const UnsafeResource& resource, bool is_pending);
 
   // Safebrowsing service.
   scoped_refptr<SafeBrowsingService> sb_service_;

chrome/browser/safe_browsing/ui_manager_unittest.cc

@@ -79,7 +79,7 @@ class SafeBrowsingUIManagerTest : public ChromeRenderViewHostTestHarness {
   }
 
   void AddToWhitelist(SafeBrowsingUIManager::UnsafeResource resource) {
-    ui_manager_->AddToWhitelist(resource);
+    ui_manager_->AddToWhitelistUrlSet(resource, false);
   }
 
   SafeBrowsingUIManager::UnsafeResource MakeUnsafeResource(

chrome/browser/ssl/chrome_security_state_model_client.cc

@@ -154,6 +154,23 @@ void AddConnectionExplanation(
                                          description_replacements, nullptr))));
 }
 
+// Check to see whether the security state should be downgraded to reflect
+// a Safe Browsing verdict.
+void CheckSafeBrowsingStatus(content::NavigationEntry* entry,
+                             content::WebContents* web_contents,
+                             SecurityStateModel::VisibleSecurityState* state) {
+  safe_browsing::SafeBrowsingService* sb_service =
+      g_browser_process->safe_browsing_service();
+  if (!sb_service)
+    return;
+  scoped_refptr<SafeBrowsingUIManager> sb_ui_manager = sb_service->ui_manager();
+  if (sb_ui_manager->IsUrlWhitelistedOrPendingForWebContents(
+          entry->GetURL(), false, entry, web_contents, false)) {
+    state->fails_malware_check = true;
+    state->initial_security_level = SecurityStateModel::SECURITY_ERROR;
+  }
+}
+
 }  // namespace
 
 ChromeSecurityStateModelClient::ChromeSecurityStateModelClient(
@@ -296,9 +313,16 @@ void ChromeSecurityStateModelClient::GetVisibleSecurityState(
     SecurityStateModel::VisibleSecurityState* state) {
   content::NavigationEntry* entry =
       web_contents_->GetController().GetVisibleEntry();
-  if (!entry ||
-      entry->GetSSL().security_style == content::SECURITY_STYLE_UNKNOWN) {
+  if (!entry) {
+    *state = SecurityStateModel::VisibleSecurityState();
+    return;
+  }
+
+  if (entry->GetSSL().security_style == content::SECURITY_STYLE_UNKNOWN) {
     *state = SecurityStateModel::VisibleSecurityState();
+    // Connection security information is still being initialized, but malware
+    // status might already be known.
+    CheckSafeBrowsingStatus(entry, web_contents_, state);
     return;
   }
 
@@ -326,16 +350,5 @@ void ChromeSecurityStateModelClient::GetVisibleSecurityState(
   state->ran_content_with_cert_errors =
       !!(ssl.content_status & content::SSLStatus::RAN_CONTENT_WITH_CERT_ERRORS);
 
-  // Check to see whether the security state should be downgraded to reflect
-  // a Safe Browsing verdict.
-  safe_browsing::SafeBrowsingService* sb_service =
-      g_browser_process->safe_browsing_service();
-  if (!sb_service)
-    return;
-  scoped_refptr<SafeBrowsingUIManager> sb_ui_manager = sb_service->ui_manager();
-  if (sb_ui_manager->IsUrlWhitelistedForWebContents(entry->GetURL(), false,
-                                                    entry, web_contents_)) {
-    state->fails_malware_check = true;
-    state->initial_security_level = SecurityStateModel::SECURITY_ERROR;
-  }
+  CheckSafeBrowsingStatus(entry, web_contents_, state);
 }