This CL integrates the new SafeBrowsing download service class

chrome/browser/safe_browsing/download_protection_service.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/download_protection_service.h"
 
 #include "base/bind.h"
+#include "base/format_macros.h"
 #include "base/memory/scoped_ptr.h"
 #include "base/metrics/histogram.h"
 #include "base/stl_util.h"
 #include "base/string_number_conversions.h"
 #include "base/string_util.h"
 #include "base/stringprintf.h"
 #include "base/time.h"
 #include "chrome/browser/safe_browsing/safe_browsing_service.h"
 #include "chrome/browser/safe_browsing/signature_util.h"
 #include "chrome/common/net/http_return.h"
@@ skipping 87 matching lines @@
   // ALWAYS ADD NEW VALUES BEFORE THIS ONE.
   DOWNLOAD_CHECKS_MAX
 };
 }  // namespace
 
 DownloadProtectionService::DownloadInfo::DownloadInfo()
     : total_bytes(0), user_initiated(false) {}
 
 DownloadProtectionService::DownloadInfo::~DownloadInfo() {}
 
+std::string DownloadProtectionService::DownloadInfo::DebugString() const {
+  std::string chain = "";

[Brian Ryner, 2011/11/14 22:10:39]

Not necessary to initialize strings to empty.

[noelutz, 2011/11/15 00:14:10]

Done.

+  for (size_t i = 0; i < download_url_chain.size(); ++i) {
+    chain += download_url_chain[i].spec();
+    if (i < download_url_chain.size() - 1) {
+      chain += " -> ";
+    }
+  }
+  return base::StringPrintf(
+      "DownloadInfo {download_url_chain:[%s], local_file:%s, "

[Brian Ryner, 2011/11/14 22:10:39]

It might be useful to include the address of the object (this) as well.

[noelutz, 2011/11/15 00:14:10]

Done.

+      "referrer_url:%s, sha256_hash:%s, total_bytes:%" PRId64 ", "
+      "user_initiated: %s}",
+      chain.c_str(),
+      local_file.value().c_str(),
+      referrer_url.spec().c_str(),
+      "TODO",
+      total_bytes,
+      user_initiated ? "true" : "false");
+}
+
 // static
 DownloadProtectionService::DownloadInfo
 DownloadProtectionService::DownloadInfo::FromDownloadItem(
     const DownloadItem& item) {
   DownloadInfo download_info;
-  download_info.local_file = item.full_path();
+  download_info.local_file = item.GetTargetFilePath();

[Brian Ryner, 2011/11/14 22:10:39]

Can you explain what this change is for?

[noelutz, 2011/11/15 00:14:10]

Woops.  I don't think this will work.  GetTargetFilePath() returns the file path
where the file will eventually written. full_path() is where there file is
written right now.  For exe those are different paths.

Changed since we need both.

   download_info.download_url_chain = item.url_chain();
   download_info.referrer_url = item.referrer_url();
   // TODO(bryner): Fill in the hash (we shouldn't compute it again)
   download_info.total_bytes = item.total_bytes();
   // TODO(bryner): Populate user_initiated
   return download_info;
 }
 
 // Parent SafeBrowsing::Client class used to lookup the bad binary
 // URL and digest list.  There are two sub-classes (one for each list).
@@ skipping 23 matching lines @@
 
   void CheckDone(SafeBrowsingService::UrlCheckResult sb_result) {
     DownloadProtectionService::DownloadCheckResult result =
         IsDangerous(sb_result) ?
         DownloadProtectionService::DANGEROUS :
         DownloadProtectionService::SAFE;
     BrowserThread::PostTask(BrowserThread::UI,
                             FROM_HERE,
                             base::Bind(callback_, result));
     UpdateDownloadCheckStats(total_type_);
-    if (IsDangerous(sb_result)) {
+    if (sb_result != SafeBrowsingService::SAFE) {
       UpdateDownloadCheckStats(dangerous_type_);
       BrowserThread::PostTask(
           BrowserThread::UI,
           FROM_HERE,
           base::Bind(&DownloadSBClient::ReportMalware,
                      this, sb_result));
     }
   }
 
   void ReportMalware(SafeBrowsingService::UrlCheckResult result) {
@@ skipping 87 matching lines @@
     if (!sb_service_ ||
         sb_service_->CheckDownloadHash(info_.sha256_hash, this)) {
       CheckDone(SafeBrowsingService::SAFE);
     } else {
       AddRef();  // SafeBrowsingService takes a pointer not a scoped_refptr.
     }
   }
 
   virtual bool IsDangerous(
       SafeBrowsingService::UrlCheckResult result) const OVERRIDE {
-    return result == SafeBrowsingService::BINARY_MALWARE_HASH;
+    // We always return false here because we don't want to warn based on
+    // a match with the digest list.  However, for UMA users, we want to

[Brian Ryner, 2011/11/14 22:10:39]

I'm not sure I follow the comment -- what part of the code ensures that we are
reporting the malware URL for UMA users?

[noelutz, 2011/11/15 00:14:10]

Updated the comment.  See CheckDone() above too.

+    // report the malware URL.
+    return false;
   }
 
   virtual void OnDownloadHashCheckResult(
       const std::string& hash,
       SafeBrowsingService::UrlCheckResult sb_result) OVERRIDE {
     CheckDone(sb_result);
     UMA_HISTOGRAM_TIMES("SB2.DownloadHashCheckDuration",
                         base::TimeTicks::Now() - start_time_);
     Release();
   }
@@ skipping 19 matching lines @@
       : info_(info),
         callback_(callback),
         service_(service),
         signature_util_(signature_util),
         sb_service_(sb_service),
         pingback_enabled_(service_->enabled()) {
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   }
 
   void Start() {
+    VLOG(2) << "Starting SafeBrowsing download check for: "
+            << info_.DebugString();
     DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
     // TODO(noelutz): implement some cache to make sure we don't issue the same
     // request over and over again if a user downloads the same binary multiple
     // times.
     if (info_.download_url_chain.empty()) {
       RecordImprovedProtectionStats(REASON_EMPTY_URL_CHAIN);
       PostFinishTask(SAFE);
       return;
     }
     const GURL& final_url = info_.download_url_chain.back();
     if (!final_url.is_valid() || final_url.is_empty()) {
       RecordImprovedProtectionStats(REASON_INVALID_URL);
       PostFinishTask(SAFE);
       return;
     }
     RecordFileExtensionType(info_.local_file);
 
-    if (!final_url.SchemeIs("http") || !IsBinaryFile(info_.local_file)) {
-      RecordImprovedProtectionStats(!final_url.SchemeIs("http") ?
+    if (final_url.SchemeIs("https") || !IsBinaryFile(info_.local_file)) {

[Brian Ryner, 2011/11/14 22:10:39]

So this might cause us to start checking non-HTTP urls that are not HTTPS, like
FTP.  I assume that's ok though?

[noelutz, 2011/11/15 00:14:10]

I think that's what we want.

+      RecordImprovedProtectionStats(final_url.SchemeIs("https") ?
                   REASON_HTTPS_URL : REASON_NOT_BINARY_FILE);
       BrowserThread::PostTask(
           BrowserThread::IO,
           FROM_HERE,
           base::Bind(&CheckClientDownloadRequest::CheckDigestList, this));
       return;
     }
 
     // Compute features from the file contents. Note that we record histograms
     // based on the result, so this runs regardless of whether the pingbacks
@@ skipping 278 matching lines @@
 
 void DownloadProtectionService::RequestFinished(
     CheckClientDownloadRequest* request) {
   DCHECK(BrowserThread::CurrentlyOn(BrowserThread::UI));
   std::set<scoped_refptr<CheckClientDownloadRequest> >::iterator it =
       download_requests_.find(request);
   DCHECK(it != download_requests_.end());
   download_requests_.erase(*it);
 }
 }  // namespace safe_browsing