Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(3685)

Unified Diff: chrome/browser/download/chrome_download_manager_delegate.cc

Issue 8468020: Propagate the SafeBrowsing download protection verdict to the DownloadItem. (Closed) Base URL: http://git.chromium.org/git/chromium.git@trunk
Patch Set: Merge Created 9 years, 1 month ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
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 01d05695d348b7e5f2416b27d6c00154278f6817..2e4c160e88b94fa2d7a33c0cb49905d632d23c66 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)
- 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->GetDangerType() != DownloadStateInfo::NOT_DANGEROUS) {
+ // 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);
}
+#if defined(ENABLE_SAFE_BROWSING)
+DownloadProtectionService*
+ ChromeDownloadManagerDelegate::GetDownloadProtectionService() {
+ 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();
+ }
+ return NULL;
+}
+#endif
+
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);
@@ -415,10 +424,24 @@ void ChromeDownloadManagerDelegate::CheckVisitedReferrerBeforeDone(
state.suggested_path = state.force_file_name;
}
- if (!state.prompt_user_for_save_location && state.force_file_name.empty()) {
- state.is_dangerous_file =
- IsDangerousFile(*download, state, visited_referrer_before);
+ if (!state.prompt_user_for_save_location &&
+ state.force_file_name.empty() &&
+ IsDangerousFile(*download, state, visited_referrer_before)) {
+ state.danger = DownloadStateInfo::DANGEROUS_FILE;
+ }
+
+#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())) {
+ // TODO(noelutz): if the user changes the extension name in the UI to
+ // something like .exe SafeBrowsing will currently *not* check if the
+ // download is malicious.
+ state.danger = DownloadStateInfo::MAYBE_DANGEROUS_CONTENT;
}
+#endif
// We need to move over to the download thread because we don't want to stat
// the suggested path on the UI thread.
@@ -453,8 +476,8 @@ void ChromeDownloadManagerDelegate::CheckIfSuggestedPathExists(
state.suggested_path = state.suggested_path.Append(filename);
}
- // If the download is deemed dangerous, we'll use a temporary name for it.
- if (state.IsDangerous()) {
+ // If the download is possibly dangerous, we'll use a temporary name for it.
+ if (state.danger != DownloadStateInfo::NOT_DANGEROUS) {
state.target_name = FilePath(state.suggested_path).BaseName();
// Create a temporary file to hold the file until the user approves its
// download.
@@ -506,7 +529,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.danger != DownloadStateInfo::NOT_DANGEROUS)
file_util::WriteFile(state.suggested_path, "", 0);
else
file_util::WriteFile(download_util::GetCrDownloadPath(
@@ -541,10 +564,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();

Powered by Google App Engine
This is Rietveld 408576698