Propagate the SafeBrowsing download protection verdict to the DownloadItem.

chrome/browser/download/chrome_download_manager_delegate.cc

Index: chrome/browser/download/chrome_download_manager_delegate.cc
diff --git a/chrome/browser/download/chrome_download_manager_delegate.cc b/chrome/browser/download/chrome_download_manager_delegate.cc
index 3fec07e7020ead50e96076ac5894ddd46e259064..77b44baefdf8e0a8c7f651fc55e2583aaade5628 100644
--- a/chrome/browser/download/chrome_download_manager_delegate.cc
+++ b/chrome/browser/download/chrome_download_manager_delegate.cc
@@ -87,13 +87,11 @@ bool ChromeDownloadManagerDelegate::ShouldStartDownload(int32 download_id) {
     return false;
 
 #if defined(ENABLE_SAFE_BROWSING)

[Randy Smith (Not in Mondays), 2011/11/16 18:04:04]

GetDownloadProtectionService() isn't inside of an #ifdef; does that mean we
could get rid of this ifdef as well?

[noelutz, 2011/11/16 23:15:07]

I think it's needed. changed.

[Randy Smith (Not in Mondays), 2011/11/17 16:49:31]

Hmmm.  So it would seem to me that we could also arrange for a stub definition
of DownloadProtectionService/GetDownloadProtectionService in the case where
ENABLE_SAFE_BROWSING wasn't defined, which would result in cleaner code at point
of call.  What's the problem with that approach?

Note that this is a nit: I'm ok with the code as written.  I'd just like it
better if more of the #ifdef was in SafeBrowsing specific code.

[noelutz, 2011/11/17 17:05:44]

Agreed.  If that's OK I would like to do that in a separate CL.

-  SafeBrowsingService* sb_service =
-      g_browser_process->safe_browsing_service();
-  if (sb_service && sb_service->download_protection_service() &&
-      profile_->GetPrefs()->GetBoolean(prefs::kSafeBrowsingEnabled)) {
+  DownloadProtectionService* service = GetDownloadProtectionService();
+  if (service) {
     VLOG(2) << __FUNCTION__ << "() Start SB URL check for download = "
             << download->DebugString(false);
-    sb_service->download_protection_service()->CheckDownloadUrl(
+    service->CheckDownloadUrl(
         DownloadProtectionService::DownloadInfo::FromDownloadItem(*download),
         base::Bind(
             &ChromeDownloadManagerDelegate::CheckDownloadUrlDone,
@@ -118,9 +116,9 @@ void ChromeDownloadManagerDelegate::ChooseDownloadPath(
 bool ChromeDownloadManagerDelegate::OverrideIntermediatePath(
     DownloadItem* item,
     FilePath* intermediate_path) {
-  if (item->IsDangerous()) {
-    // The download is not safe.  It's name is already set to an intermediate
-    // name, so no need to override.
+  if (item->needs_quarantine_file()) {
+    // The download might not be safe.  It's name is already set to an
+    // intermediate name, so no need to override.
     return false;
   }
 
@@ -164,13 +162,11 @@ bool ChromeDownloadManagerDelegate::ShouldCompleteDownload(DownloadItem* item) {
     return !state.pending;
   }
   // Begin the safe browsing download protection check.
-  SafeBrowsingService* sb_service =
-      g_browser_process->safe_browsing_service();
-  if (sb_service && sb_service->download_protection_service() &&
-      profile_->GetPrefs()->GetBoolean(prefs::kSafeBrowsingEnabled)) {
+  DownloadProtectionService* service = GetDownloadProtectionService();
+  if (service) {
     VLOG(2) << __FUNCTION__ << "() Start SB download check for download = "
             << item->DebugString(false);
-    sb_service->download_protection_service()->CheckClientDownload(
+    service->CheckClientDownload(
         DownloadProtectionService::DownloadInfo::FromDownloadItem(*item),
         base::Bind(
             &ChromeDownloadManagerDelegate::CheckClientDownloadDone,
@@ -299,6 +295,18 @@ void ChromeDownloadManagerDelegate::DownloadProgressUpdated() {
       download_count, progress_known, progress);
 }
 
+DownloadProtectionService*
+    ChromeDownloadManagerDelegate::GetDownloadProtectionService() {
+#if defined(ENABLE_SAFE_BROWSING)
+  SafeBrowsingService* sb_service = g_browser_process->safe_browsing_service();
+  if (sb_service && sb_service->download_protection_service() &&
+      profile_->GetPrefs()->GetBoolean(prefs::kSafeBrowsingEnabled)) {
+    return sb_service->download_protection_service();
+  }
+#endif
+  return NULL;
+}
+
 void ChromeDownloadManagerDelegate::CheckDownloadUrlDone(
     int32 download_id,
     DownloadProtectionService::DownloadCheckResult result) {
@@ -330,10 +338,11 @@ void ChromeDownloadManagerDelegate::CheckClientDownloadDone(
 
   VLOG(2) << __FUNCTION__ << "() download = " << item->DebugString(false)
           << " verdict = " << result;
-  // TODO(noelutz):
-  //  1) display a warning if the result is DANGEROUS.
-  //  2) make sure we haven't already displayed a warning for the URL.
-  //  3) disable the existing dangerous file warning for executables.
+  // We only mark the content as being dangerous if the download's safety state
+  // has not been set to DANGEROUS yet.  We don't want to show two warnings.
+  if (result == DownloadProtectionService::DANGEROUS &&
+      item->safety_state() == DownloadItem::SAFE)
+    item->MarkContentDangerous();
 
   SafeBrowsingStateMap::iterator it = safe_browsing_state_.find(item->id());
   DCHECK(it != safe_browsing_state_.end() && it->second.pending);
@@ -418,7 +427,19 @@ void ChromeDownloadManagerDelegate::CheckVisitedReferrerBeforeDone(
   if (!state.prompt_user_for_save_location && state.force_file_name.empty()) {
     state.is_dangerous_file =
         IsDangerousFile(*download, state, visited_referrer_before);
+    state.needs_quarantine_file = true;
+  }
+
+#if defined(ENABLE_SAFE_BROWSING)
+  DownloadProtectionService* service = GetDownloadProtectionService();
+  // Return false if this type of files is handled by the enhanced SafeBrowsing
+  // download protection.
+  if (service && service->enabled() &&
+      service->IsSupportedFileType(state.suggested_path.BaseName())) {

[Randy Smith (Not in Mondays), 2011/11/16 17:52:41]

Asanka: I just wanted to explicitly check with you (with your file name
determination hat on) that we have the correct file type (all hints incorporated
in) at this point?

[asanka, 2011/11/16 18:34:05]

Good question.

At this point state.suggested_path contains the filename derived from the
request/response data (URL, Content-Disposition, suggested name, etc.) but not
the filename that the user might choose from a "Save As" dialog.  Note that the
extension might not be what you expect for the content type.

Noe: If we were to determine an innocuous extension (because of a missing or
malicious C-D header, malicious download attribute etc.) and the user later
picks a dangerous extension from the Save As dialog, do we handle it correctly?

[noelutz, 2011/11/16 23:15:07]

I don't think so.  I think that's good enough for now though.  Added a TODO. 
Note that the dangerous file warning has the same issue afaik.

+    state.is_dangerous_file = false;
+    state.needs_quarantine_file = true;
   }
+#endif
 
   // We need to move over to the download thread because we don't want to stat
   // the suggested path on the UI thread.
@@ -454,7 +475,7 @@ void ChromeDownloadManagerDelegate::CheckIfSuggestedPathExists(
   }
 
   // If the download is deemed dangerous, we'll use a temporary name for it.
-  if (state.IsDangerous()) {
+  if (state.needs_quarantine_file) {
     state.target_name = FilePath(state.suggested_path).BaseName();
     // Create a temporary file to hold the file until the user approves its
     // download.
@@ -506,7 +527,7 @@ void ChromeDownloadManagerDelegate::CheckIfSuggestedPathExists(
   // See: http://code.google.com/p/chromium/issues/detail?id=3662
   if (!state.prompt_user_for_save_location &&
       state.force_file_name.empty()) {
-    if (state.IsDangerous())
+    if (state.needs_quarantine_file)
       file_util::WriteFile(state.suggested_path, "", 0);
     else
       file_util::WriteFile(download_util::GetCrDownloadPath(
@@ -541,10 +562,6 @@ bool ChromeDownloadManagerDelegate::IsDangerousFile(
   if (state.transition_type & content::PAGE_TRANSITION_FROM_ADDRESS_BAR)
     return false;
 
-  // Return false if this type of files is handled by the enhanced SafeBrowsing
-  // download protection.
-  // TODO(noelutz): implement that check.
-
   // Extensions that are not from the gallery are considered dangerous.
   if (IsExtensionDownload(&download)) {
     ExtensionService* service = profile_->GetExtensionService();