Downgrade security state while displaying an SB interstitial

chrome/browser/safe_browsing/ui_manager.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "chrome/browser/safe_browsing/ui_manager.h"
 
 #include "base/bind.h"
 #include "base/bind_helpers.h"
 #include "base/callback.h"
 #include "base/debug/leak_tracker.h"
@@ skipping 25 matching lines @@
 
 using content::BrowserThread;
 using content::NavigationEntry;
 using content::WebContents;
 using safe_browsing::HitReport;
 
 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();
   }
 
-  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) {
+    auto iter = pending_.find(url.GetWithEmptyPath());

[estark, 2016/08/25 06:54:06]

optional nit: I'd have a slight preference for `return pending_.find(...) !=
pending_.end()`

[felt, 2016/08/25 15:17:27]

Sure. Also updated Contains() for consistency.

+    return iter != 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);
 };
 
 }  // namespace
 
 namespace safe_browsing {
 
 // SafeBrowsingUIManager::UnsafeResource ---------------------------------------
 
@@ skipping 72 matching lines @@
     bool proceed) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   for (const auto& resource : resources) {
     if (!resource.callback.is_null()) {
       DCHECK(resource.callback_thread);
       resource.callback_thread->PostTask(
           FROM_HERE, base::Bind(resource.callback, proceed));
     }
 
     if (proceed)
-      AddToWhitelist(resource);
+      AddToWhitelistUrlSet(resource, false /* Pending -> permanent */);

[estark, 2016/08/25 06:54:06]

I don't see pending URLs getting cleaned up anywhere in the event the user
doesn't proceed. Maybe an else block here that removes a URL from the pending
whitelist set?

[felt, 2016/08/25 15:17:27]

There isn't any need to explicitly remove them.

Costs of leaving the pending entries?:
* No memory cost -- the number of pending entries in any WC is going to be very
small.
* No behavioral difference from UI perspective.

Costs of removing the pending entries?:
* There is a small runtime cost to looking up the entry to remove them (run the
WC getter, get the user data, and redo the lookup logic). 

Weighing the pros/cons I figured I'd just leave them. If you strongly think they
should be cleaned up tho, I can add that; it isn't really that expensive to
remove them.

[estark, 2016/08/25 15:39:54]

Ah, ok, I see. That sounds fine to me. Thanks for the explanation.

   }
 }
 
 void SafeBrowsingUIManager::DisplayBlockingPage(
     const UnsafeResource& resource) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
   if (resource.is_subresource && !resource.is_subframe) {
     // Sites tagged as serving Unwanted Software should only show a warning for
     // main-frame or sub-frame resource. Similar warning restrictions should be
     // applied to malware sites tagged as "landing sites" (see "Types of
@@ skipping 70 matching lines @@
             prefs::kSafeBrowsingExtendedReportingEnabled);
     hit_report.is_metrics_reporting_active =
         ChromeMetricsServiceAccessor::IsMetricsAndCrashReportingEnabled();
 
     MaybeReportSafeBrowsingHit(hit_report);
   }
 
   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);
 }
 
 // A safebrowsing hit is sent after a blocking page for malware/phishing
 // or after the warning dialog for download urls, only for
 // UMA || extended_reporting users.
 void SafeBrowsingUIManager::MaybeReportSafeBrowsingHit(
     const HitReport& hit_report) {
   DCHECK_CURRENTLY_ON(BrowserThread::UI);
 
@@ skipping 86 matching lines @@
   // etc). This happens on the IO thread.
   if (sb_service_.get() == NULL || sb_service_->ping_manager() == NULL)
     return;
 
   if (!serialized.empty()) {
     DVLOG(1) << "Sending serialized threat details.";
     sb_service_->ping_manager()->ReportThreatDetails(serialized);
   }
 }
 
-// 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();
   WhitelistUrlSet* site_list =
       static_cast<WhitelistUrlSet*>(web_contents->GetUserData(kWhitelistKey));
   if (!site_list) {
     site_list = new WhitelistUrlSet;
     web_contents->SetUserData(kWhitelistKey, site_list);
   }
 
   GURL whitelisted_url;
   if (resource.is_subresource) {
     NavigationEntry* entry = resource.GetNavigationEntryForResource();
     if (!entry)
       return;
     whitelisted_url = entry->GetURL();
   } else {
     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) {
   NavigationEntry* entry = nullptr;
   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(), false);
 }
 
-// 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