Report metadata check veridcts when DLP scans occur

This regression was caused by crrev.com/c/2339282, which suppresses with
a bad condition. The fix is to check for existing malware verdicts
before suppressing the metadata check verdict instead of just checking
if an async scan took place.

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

chrome/browser/enterprise/connectors/common.cc

@@ -87,4 +87,11 @@ ScanResult::ScanResult(const ContentAnalysisResponse& response)
     : response(response) {}
 ScanResult::~ScanResult() = default;
 
+bool ContainsMalwareVerdict(const ContentAnalysisResponse& response) {
+  const auto& results = response.results();
+  return std::any_of(results.begin(), results.end(), [](const auto& result) {
+    return result.tag() == "malware" && !result.triggered_rules().empty();
+  });
+}
+
 }  // namespace enterprise_connectors

chrome/browser/enterprise/connectors/common.h

@@ -90,6 +90,9 @@ struct ScanResult : public base::SupportsUserData::Data {
   ContentAnalysisResponse response;
 };
 
+// Checks if |response| contains a negative malware verdict.
+bool ContainsMalwareVerdict(const ContentAnalysisResponse& response);
+
 }  // namespace enterprise_connectors
 
 #endif  // CHROME_BROWSER_ENTERPRISE_CONNECTORS_COMMON_H_

chrome/browser/safe_browsing/cloud_content_scanning/deep_scanning_test_utils.cc

@@ -253,6 +253,44 @@ void EventReportValidator::
           });
 }
 
+void EventReportValidator::
+    ExpectSensitiveDataEventAndDangerousDeepScanningResult(
+        const std::string& expected_url,
+        const std::string& expected_filename,
+        const std::string& expected_sha256,
+        const std::string& expected_threat_type,
+        const std::string& expected_trigger,
+        const enterprise_connectors::ContentAnalysisResponse::Result&
+            expected_dlp_verdict,
+        const std::set<std::string>* expected_mimetypes,
+        int expected_content_size,
+        const std::string& expected_result) {
+  event_key_ = SafeBrowsingPrivateEventRouter::kKeySensitiveDataEvent;
+  url_ = expected_url;
+  filename_ = expected_filename;
+  sha256_ = expected_sha256;
+  trigger_ = expected_trigger;
+  mimetypes_ = expected_mimetypes;
+  content_size_ = expected_content_size;
+  result_ = expected_result;
+  dlp_verdict_ = expected_dlp_verdict;
+  EXPECT_CALL(*client_, UploadRealtimeReport_(_, _))
+      .WillOnce([this](base::Value& report,
+                       base::OnceCallback<void(bool)>& callback) {
+        ValidateReport(&report);
+      })
+      .WillOnce([this, expected_threat_type](
+                    base::Value& report,
+                    base::OnceCallback<void(bool)>& callback) {
+        event_key_ = SafeBrowsingPrivateEventRouter::kKeyDangerousDownloadEvent;
+        threat_type_ = expected_threat_type;
+        dlp_verdict_ = base::nullopt;
+        ValidateReport(&report);
+        if (!done_closure_.is_null())
+          done_closure_.Run();
+      });
+}
+
 void EventReportValidator::ExpectDangerousDownloadEvent(
     const std::string& expected_url,
     const std::string& expected_filename,

chrome/browser/safe_browsing/cloud_content_scanning/deep_scanning_test_utils.h

@@ -67,6 +67,18 @@ class EventReportValidator {
       int expected_content_size,
       const std::string& expected_result);
 
+  void ExpectSensitiveDataEventAndDangerousDeepScanningResult(
+      const std::string& expected_url,
+      const std::string& expected_filename,
+      const std::string& expected_sha256,
+      const std::string& expected_threat_type,
+      const std::string& expected_trigger,
+      const enterprise_connectors::ContentAnalysisResponse::Result&
+          expected_dlp_verdict,
+      const std::set<std::string>* expected_mimetypes,
+      int expected_content_size,
+      const std::string& expected_result);
+
   void ExpectUnscannedFileEvent(const std::string& expected_url,
                                 const std::string& expected_filename,
                                 const std::string& expected_sha256,

chrome/browser/safe_browsing/download_protection/deep_scanning_browsertest.cc

@@ -688,6 +688,75 @@ IN_PROC_BROWSER_TEST_F(DownloadDeepScanningBrowserTest, MultipleFCMResponses) {
                                 true, 1);
 }
 
+IN_PROC_BROWSER_TEST_F(DownloadDeepScanningBrowserTest,
+                       DlpAndMalwareViolations) {
+  SetUpReporting();
+  base::HistogramTester histograms;
+
+  // The file is DANGEROUS_HOST according to the metadata check
+  ClientDownloadResponse metadata_response;
+  metadata_response.set_verdict(ClientDownloadResponse::DANGEROUS_HOST);
+  ExpectMetadataResponse(metadata_response);
+
+  GURL url = embedded_test_server()->GetURL(
+      "/safe_browsing/download_protection/zipfile_two_archives.zip");
+  ui_test_utils::NavigateToURLWithDisposition(
+      browser(), url, WindowOpenDisposition::CURRENT_TAB,
+      ui_test_utils::BROWSER_TEST_WAIT_FOR_LOAD_STOP);
+
+  // The DLP scan finishes synchronously and find a violation.
+  enterprise_connectors::ContentAnalysisResponse sync_response;
+  auto* result = sync_response.add_results();
+  result->set_tag("dlp");
+  result->set_status(
+      enterprise_connectors::ContentAnalysisResponse::Result::SUCCESS);
+  auto* dlp_rule = result->add_triggered_rules();
+  dlp_rule->set_action(enterprise_connectors::TriggeredRule::WARN);
+  dlp_rule->set_rule_name("dlp_rule_name");
+  ExpectContentAnalysisSynchronousResponse(/*is_advanced_protection=*/false,
+                                           sync_response, {"dlp"});
+
+  WaitForMetadataCheck();
+  WaitForDeepScanRequest(/*is_advanced_protection=*/false);
+
+  // Both the DLP and malware violations generate an event.
+  std::set<std::string> zip_types = {"application/zip",
+                                     "application/x-zip-compressed"};
+  EventReportValidator validator(client());
+  validator.ExpectSensitiveDataEventAndDangerousDeepScanningResult(
+      /*url*/ url.spec(),
+      /*filename*/
+      (*download_items().begin())->GetTargetFilePath().AsUTF8Unsafe(),
+      // sha256sum chrome/test/data/safe_browsing/download_protection/\
+      // zipfile_two_archives.zip |  tr '[:lower:]' '[:upper:]'
+      /*sha*/
+      "339C8FFDAE735C4F1846D0E6FF07FBD85CAEE6D96045AAEF5B30F3220836643C",
+      /*threat_type*/ "DANGEROUS_HOST",
+      /*trigger*/
+      extensions::SafeBrowsingPrivateEventRouter::kTriggerFileDownload,
+      /*dlp_verdict*/ *result,
+      /*mimetypes*/ &zip_types,
+      /*size*/ 276,
+      /*result*/ EventResultToString(EventResult::WARNED));
+  WaitForDownloadToFinish();
+
+  // The file should be blocked.
+  ASSERT_EQ(download_items().size(), 1u);
+  download::DownloadItem* item = *download_items().begin();
+  EXPECT_EQ(item->GetDangerType(),
+            download::DownloadDangerType::DOWNLOAD_DANGER_TYPE_DANGEROUS_HOST);
+  EXPECT_EQ(item->GetState(), download::DownloadItem::IN_PROGRESS);
+
+  // UMAs for this request should only be recorded once. The malware metric
+  // should not be recorded since no deep malware scan occurred.
+  histograms.ExpectUniqueSample("SafeBrowsingBinaryUploadRequest.Result",
+                                BinaryUploadService::Result::SUCCESS, 1);
+  histograms.ExpectUniqueSample("SafeBrowsingBinaryUploadRequest.DlpResult",
+                                true, 1);
+  histograms.ExpectUniqueSample("SafeBrowsingBinaryUploadRequest.MalwareResult",
+                                true, 0);
+}
+
 class DownloadRestrictionsDeepScanningBrowserTest
     : public DownloadDeepScanningBrowserTest {
  public:

chrome/browser/safe_browsing/download_protection/download_reporter.cc

@@ -54,7 +54,16 @@ std::string DangerTypeToThreatType(download::DownloadDangerType danger_type) {
   }
 }
 
-void ReportDangerousDownloadWarning(download::DownloadItem* download) {
+void MaybeReportDangerousDownloadWarning(download::DownloadItem* download) {
+  // If |download| has a deep scanning malware verdict, then it means the
+  // dangerous file has already been reported.
+  auto* scan_result = static_cast<enterprise_connectors::ScanResult*>(
+      download->GetUserData(enterprise_connectors::ScanResult::kKey));
+  if (scan_result &&
+      enterprise_connectors::ContainsMalwareVerdict(scan_result->response)) {
+    return;
+  }
+
   content::BrowserContext* browser_context =
       content::DownloadItemUtils::GetBrowserContext(download);
   Profile* profile = Profile::FromBrowserContext(browser_context);
@@ -160,9 +169,8 @@ void DownloadReporter::OnDownloadUpdated(download::DownloadItem* download) {
   download::DownloadDangerType current_danger_type = download->GetDangerType();
 
   if (!DangerTypeIsDangerous(old_danger_type) &&
-      old_danger_type != download::DOWNLOAD_DANGER_TYPE_ASYNC_SCANNING &&
       DangerTypeIsDangerous(current_danger_type)) {
-    ReportDangerousDownloadWarning(download);
+    MaybeReportDangerousDownloadWarning(download);
   }
 
   if (DangerTypeIsDangerous(old_danger_type) &&