Make POTENTIALLY_UNWANTED and UNCOMMON precede over safe deep scan

Bug: 1068786
Change-Id: I464ecae57cd2e2922bd5e1176e0cea10db4c109c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2295999Reviewed-by: Daniel Rubery <drubery@chromium.org>
Commit-Queue: Dominique Fauteux-Chapleau <domfc@chromium.org>
Cr-Commit-Position: refs/heads/master@{#788349}

chrome/browser/safe_browsing/download_protection/check_client_download_request.cc

@@ -54,15 +54,23 @@ void MaybeOverrideDlpScanResult(DownloadCheckResultReason reason,
                                 CheckDownloadRepeatingCallback callback,
                                 DownloadCheckResult deep_scan_result) {
   if (reason == REASON_DOWNLOAD_DANGEROUS ||
-      reason == REASON_DOWNLOAD_DANGEROUS_HOST) {
+      reason == REASON_DOWNLOAD_DANGEROUS_HOST ||
+      reason == REASON_DOWNLOAD_POTENTIALLY_UNWANTED ||
+      reason == REASON_DOWNLOAD_UNCOMMON) {
+    // Don't let safe deep scanning results override these previous
+    // dangerous reasons.
     switch (deep_scan_result) {
       case DownloadCheckResult::UNKNOWN:
       case DownloadCheckResult::SENSITIVE_CONTENT_WARNING:
       case DownloadCheckResult::DEEP_SCANNED_SAFE:
         if (reason == REASON_DOWNLOAD_DANGEROUS)
           callback.Run(DownloadCheckResult::DANGEROUS);
-        else
+        else if (reason == REASON_DOWNLOAD_DANGEROUS_HOST)
           callback.Run(DownloadCheckResult::DANGEROUS_HOST);
+        else if (reason == REASON_DOWNLOAD_POTENTIALLY_UNWANTED)
+          callback.Run(DownloadCheckResult::POTENTIALLY_UNWANTED);
+        else
+          callback.Run(DownloadCheckResult::UNCOMMON);
         return;
 
       case DownloadCheckResult::ASYNC_SCANNING:
@@ -287,7 +295,8 @@ void CheckClientDownloadRequest::UploadBinary(
     enterprise_connectors::AnalysisSettings settings) {
   if (reason == REASON_DOWNLOAD_DANGEROUS ||
       reason == REASON_DOWNLOAD_DANGEROUS_HOST ||
-      reason == REASON_WHITELISTED_URL) {
+      reason == REASON_DOWNLOAD_POTENTIALLY_UNWANTED ||
+      reason == REASON_DOWNLOAD_UNCOMMON || reason == REASON_WHITELISTED_URL) {
     service()->UploadForDeepScanning(
         item_,
         base::BindRepeating(&MaybeOverrideDlpScanResult, reason, callback_),

chrome/browser/safe_browsing/download_protection/download_protection_service_unittest.cc

@@ -10,6 +10,7 @@
 #include <map>
 #include <memory>
 #include <string>
+#include <utility>
 
 #include "base/base_paths.h"
 #include "base/bind.h"
@@ -4024,6 +4025,65 @@ TEST_P(DeepScanningDownloadTest, PolicyDisabled) {
   }
 }
 
+TEST_P(DeepScanningDownloadTest, SafeVerdictPrecedence) {
+  // These responses have precedence over safe deep scanning results.
+  std::vector<std::pair<ClientDownloadResponse::Verdict, DownloadCheckResult>>
+      responses = {
+          {ClientDownloadResponse::DANGEROUS, DownloadCheckResult::DANGEROUS},
+          {ClientDownloadResponse::DANGEROUS_HOST,
+           DownloadCheckResult::DANGEROUS_HOST},
+          {ClientDownloadResponse::POTENTIALLY_UNWANTED,
+           DownloadCheckResult::POTENTIALLY_UNWANTED},
+          {ClientDownloadResponse::UNCOMMON, DownloadCheckResult::UNCOMMON},
+      };
+  for (const auto& response : responses) {
+    NiceMockDownloadItem item;
+    PrepareBasicDownloadItem(&item, {"http://www.evil.com/a.exe"},  // url_chain
+                             "http://www.google.com/",              // referrer
+                             FILE_PATH_LITERAL("a.tmp"),            // tmp_path
+                             FILE_PATH_LITERAL("a.exe"));  // final_path
+    content::DownloadItemUtils::AttachInfo(&item, profile(), nullptr);
+    EXPECT_CALL(*sb_service_->mock_database_manager(),
+                MatchDownloadWhitelistUrl(_))
+        .WillRepeatedly(Return(false));
+    EXPECT_CALL(*binary_feature_extractor_.get(), CheckSignature(tmp_path_, _));
+    EXPECT_CALL(*binary_feature_extractor_.get(),
+                ExtractImageFeatures(
+                    tmp_path_, BinaryFeatureExtractor::kDefaultOptions, _, _));
+
+    SetSendFilesForMalwareCheckPref(
+        SendFilesForMalwareCheckValues::SEND_DOWNLOADS);
+    SetCheckContentCompliancePref(
+        CheckContentComplianceValues::CHECK_DOWNLOADS);
+    SetUrlToCheckContentCompliance("evil.com");
+
+    TestBinaryUploadService* test_upload_service =
+        static_cast<TestBinaryUploadService*>(
+            BinaryUploadServiceFactory::GetForProfile(profile()));
+
+    PrepareResponse(response.first, net::HTTP_OK, net::OK);
+    if (use_legacy_policies_) {
+      test_upload_service->SetResponse(BinaryUploadService::Result::SUCCESS,
+                                       DeepScanningClientResponse());
+    } else {
+      test_upload_service->SetResponse(
+          BinaryUploadService::Result::SUCCESS,
+          enterprise_connectors::ContentAnalysisResponse());
+    }
+
+    RunLoop run_loop;
+    download_service_->CheckClientDownload(
+        &item,
+        base::BindRepeating(&DownloadProtectionServiceTest::CheckDoneCallback,
+                            base::Unretained(this), run_loop.QuitClosure()));
+    run_loop.Run();
+
+    EXPECT_TRUE(IsResult(response.second));
+    EXPECT_TRUE(HasClientDownloadRequest());
+    EXPECT_EQ(test_upload_service->was_called(), flag_enabled());
+  }
+}
+
 INSTANTIATE_TEST_SUITE_P(, DownloadProtectionServiceTest, testing::Bool());
 INSTANTIATE_TEST_SUITE_P(,
                          DeepScanningDownloadTest,