If we show a SafeBrowsing warning we always send the client-side detection

chrome/browser/safe_browsing/client_side_detection_host.cc

 // Copyright (c) 2011 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/client_side_detection_host.h"
 
 #include <vector>
 
 #include "base/logging.h"
 #include "base/memory/ref_counted.h"
@@ skipping 251 matching lines @@
 
 // static
 ClientSideDetectionHost* ClientSideDetectionHost::Create(
     TabContents* tab) {
   return new ClientSideDetectionHost(tab);
 }
 
 ClientSideDetectionHost::ClientSideDetectionHost(TabContents* tab)
     : TabContentsObserver(tab),
       csd_service_(NULL),
-      cb_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)) {
+      cb_factory_(ALLOW_THIS_IN_INITIALIZER_LIST(this)),
+      unsafe_unique_page_id_(-1) {
   DCHECK(tab);
   csd_service_ = g_browser_process->safe_browsing_detection_service();
   feature_extractor_.reset(new BrowserFeatureExtractor(tab, csd_service_));
   sb_service_ = g_browser_process->safe_browsing_service();
   // Note: csd_service_ and sb_service_ will be NULL here in testing.
   registrar_.Add(this, content::NOTIFICATION_RESOURCE_RESPONSE_STARTED,
                  Source<RenderViewHostDelegate>(tab));
+  if (sb_service_) {
+    sb_service_->AddObserver(this);
+  }
 }
 
-ClientSideDetectionHost::~ClientSideDetectionHost() {}
+ClientSideDetectionHost::~ClientSideDetectionHost() {
+  if (sb_service_) {
+    sb_service_->RemoveObserver(this);
+  }
+}
 
 bool ClientSideDetectionHost::OnMessageReceived(const IPC::Message& message) {
   bool handled = true;
   IPC_BEGIN_MESSAGE_MAP(ClientSideDetectionHost, message)
-    IPC_MESSAGE_HANDLER(SafeBrowsingHostMsg_DetectedPhishingSite,
-                        OnDetectedPhishingSite)
+    IPC_MESSAGE_HANDLER(SafeBrowsingHostMsg_PhishingDetectionDone,
+                        OnPhishingDetectionDone)
     IPC_MESSAGE_UNHANDLED(handled = false)
   IPC_END_MESSAGE_MAP()
   return handled;
 }
 
 void ClientSideDetectionHost::DidNavigateMainFramePostCommit(
     const content::LoadCommittedDetails& details,
     const ViewHostMsg_FrameNavigate_Params& params) {
   // TODO(noelutz): move this DCHECK to TabContents and fix all the unit tests
   // that don't call this method on the UI thread.
@@ skipping 26 matching lines @@
 
   // Notify the renderer if it should classify this URL.
   classification_request_ = new ShouldClassifyUrlRequest(params,
                                                          tab_contents(),
                                                          csd_service_,
                                                          sb_service_,
                                                          this);
   classification_request_->Start();
 }
 
+void ClientSideDetectionHost::OnSafeBrowsingHit(
+    const SafeBrowsingService::UnsafeResource& resource) {
+  // Check that this notification is really for us and that it corresponds to
+  // either a malware or phishing hit.  In this case we store the unique page
+  // ID and store it for later.  For phishing hits we also require that the

[Garrett Casto, 2011/07/19 21:18:48]

The "In this case ..." sentence seems to have two clauses that say the same
thing. Also, do you want to mention why we only send back information if the top
level url matches for phishing hits?

[noelutz, 2011/07/19 22:28:08]

Removed the top level phishing check.  We can check that on the server. 

+  // top level URL match the phishing list.
+  if (tab_contents() &&
+      tab_contents()->GetRenderProcessHost()->id() ==
+          resource.render_process_host_id &&
+      tab_contents()->render_view_host()->routing_id() ==
+          resource.render_view_id &&
+      ((resource.threat_type == SafeBrowsingService::URL_PHISHING &&
+        !resource.is_subresource) ||
+       resource.threat_type == SafeBrowsingService::URL_MALWARE) &&
+      tab_contents()->controller().GetActiveEntry()) {
+    unsafe_unique_page_id_ =
+        tab_contents()->controller().GetActiveEntry()->unique_id();
+  }
+}
+
 void ClientSideDetectionHost::TabContentsDestroyed(TabContents* tab) {
   DCHECK(tab);
   // Tell any pending classification request that it is being canceled.
   if (classification_request_.get()) {
     classification_request_->Cancel();
   }
   // Cancel all pending feature extractions.
   feature_extractor_.reset();
 }
 
-void ClientSideDetectionHost::OnDetectedPhishingSite(
+void ClientSideDetectionHost::OnPhishingDetectionDone(
     const std::string& verdict_str) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   // There is something seriously wrong if there is no service class but
   // this method is called.  The renderer should not start phishing detection
   // if there isn't any service class in the browser.
   DCHECK(csd_service_);
   // There shouldn't be any pending requests because we revoke them everytime
   // we navigate away.
   DCHECK(!cb_factory_.HasPendingCallbacks());
   DCHECK(browse_info_.get());
 
   // We parse the protocol buffer here.  If we're unable to parse it we won't
   // send the verdict further.
   scoped_ptr<ClientPhishingRequest> verdict(new ClientPhishingRequest);
   if (csd_service_ &&
       !cb_factory_.HasPendingCallbacks() &&
       browse_info_.get() &&
       verdict->ParseFromString(verdict_str) &&
-      verdict->IsInitialized()) {
+      verdict->IsInitialized() &&
+      // We only send the verdict to the server if the verdict is phishing or if
+      // a SafeBrowsing interstitial was already shown for this site.  E.g., a
+      // malware or phishing interstitial was shown but the user clicked
+      // through.
+      (verdict->is_phishing() || DidShowSBInterstitial())) {
     if (browse_info_->url.spec() != verdict->url()) {
       // I'm not sure we can DCHECK on this one so we keep stats around to see
       // whether this actually happens in practice.
       UMA_HISTOGRAM_COUNTS("SBClientPhishing.BrowserRendererUrlMismatch", 1);
       VLOG(2) << "Browser and renderer URL do not match: "
               << browse_info_->url.spec() << " vs. " << verdict->url();
     }
     // Start browser-side feature extraction.  Once we're done it will send
     // the client verdict request.
     feature_extractor_->ExtractFeatures(
@@ skipping 34 matching lines @@
 
 void ClientSideDetectionHost::FeatureExtractionDone(
     bool success,
     ClientPhishingRequest* request) {
   if (!request) {
     DLOG(FATAL) << "Invalid request object in FeatureExtractionDone";
     return;
   }
   VLOG(2) << "Feature extraction done (success:" << success << ") for URL: "
           << request->url() << ". Start sending client phishing request.";
-  // Send ping no matter what - even if the browser feature extraction failed.
+  ClientSideDetectionService::ClientReportPhishingRequestCallback* cb = NULL;
+  // If the client-side verdict isn't phishing we don't care about the server
+  // response because we aren't going to display a warning.
+  if (request->is_phishing()) {

[Garrett Casto, 2011/07/19 21:18:48]

So we probably don't want to show a warning if we previously showed a SB
interstitial, regardless of if the client model thinks that it's phishing. It
would just leading to a confusing experience. It would probably also be good if
we didn't show a warning if we had previously disregarded the server SB
interstitial, but we can worry about that later.

[noelutz, 2011/07/19 22:28:08]

I already implemented that a while back in the SafeBrowsingService class (see
whitelist code).

+    cb = cb_factory_.NewCallback(
+        &ClientSideDetectionHost::MaybeShowPhishingWarning);
+  }
+  // Send ping even if the browser feature extraction failed.
   csd_service_->SendClientReportPhishingRequest(
       request,  // The service takes ownership of the request object.
-      cb_factory_.NewCallback(
-          &ClientSideDetectionHost::MaybeShowPhishingWarning));
+      cb);
 }
 
 void ClientSideDetectionHost::Observe(int type,
                                       const NotificationSource& source,
                                       const NotificationDetails& details) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   DCHECK_EQ(type, content::NOTIFICATION_RESOURCE_RESPONSE_STARTED);
   const ResourceRequestDetails* req = Details<ResourceRequestDetails>(
       details).ptr();
   if (req && browse_info_.get()) {
     browse_info_->ips.insert(req->socket_address().host());
   }
 }
 
+bool ClientSideDetectionHost::DidShowSBInterstitial() {
+  return (unsafe_unique_page_id_ > 0 &&
+          tab_contents() &&
+          tab_contents()->controller().GetActiveEntry() &&
+          tab_contents()->controller().GetActiveEntry()->unique_id() ==

[Brian Ryner, 2011/07/19 21:27:10]

You could avoid some of the repetition in this method by using temporary
variables, but up to you.

[noelutz, 2011/07/19 22:28:08]

Done.

+          unsafe_unique_page_id_);
+}
+
 void ClientSideDetectionHost::set_client_side_detection_service(
     ClientSideDetectionService* service) {
   csd_service_ = service;
 }
 
 void ClientSideDetectionHost::set_safe_browsing_service(
     SafeBrowsingService* service) {
+  if (sb_service_) {
+    sb_service_->RemoveObserver(this);
+  }
   sb_service_ = service;
+  if (sb_service_) {
+    sb_service_->AddObserver(this);
+  }
 }
 
 }  // namespace safe_browsing