Failed scans should not show DEEP_SCANNED_SAFE UX

We want to fail open, but we don't want to tell the user "Scan complete"
if the scan didn't actually complete.

Change-Id: I174a9cec446ade9282438d1b1dd240cbbe3153be
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/2132782
Commit-Queue: Daniel Rubery <drubery@chromium.org>
Reviewed-by: Dominique Fauteux-Chapleau <domfc@chromium.org>
Reviewed-by: Bettina Dea <bdea@chromium.org>
Cr-Commit-Position: refs/heads/master@{#756089}

chrome/browser/safe_browsing/cloud_content_scanning/deep_scanning_browsertest_base.cc

@@ -4,12 +4,14 @@
 
 #include "chrome/browser/safe_browsing/cloud_content_scanning/deep_scanning_browsertest_base.h"
 #include "base/bind_helpers.h"
+#include "chrome/browser/browser_process.h"
 #include "chrome/browser/safe_browsing/cloud_content_scanning/deep_scanning_dialog_views.h"
 #include "chrome/browser/safe_browsing/cloud_content_scanning/fake_deep_scanning_dialog_delegate.h"
 #include "chrome/browser/safe_browsing/dm_token_utils.h"
 #include "chrome/browser/ui/browser.h"
 #include "chrome/test/base/testing_browser_process.h"
 #include "components/prefs/pref_service.h"
+#include "components/prefs/scoped_user_pref_update.h"
 #include "components/safe_browsing/core/features.h"
 
 namespace safe_browsing {
@@ -131,6 +133,13 @@ void DeepScanningBrowserTestBase::SetUnsafeEventsReportingPolicy(bool report) {
       prefs::kUnsafeEventsReportingEnabled, report);
 }
 
+void DeepScanningBrowserTestBase::AddUrlToCheckComplianceOfDownloads(
+    const std::string& url) {
+  ListPrefUpdate(g_browser_process->local_state(),
+                 prefs::kURLsToCheckComplianceOfDownloadedContent)
+      ->Append(url);
+}
+
 void DeepScanningBrowserTestBase::SetUpDelegate() {
   SetDMTokenForTesting(policy::DMToken::CreateValidTokenForTesting(kDmToken));
   DeepScanningDialogDelegate::SetFactoryForTesting(base::BindRepeating(

chrome/browser/safe_browsing/cloud_content_scanning/deep_scanning_browsertest_base.h

@@ -30,6 +30,7 @@ class DeepScanningBrowserTestBase : public InProcessBrowserTest {
       BlockUnsupportedFiletypesValues state);
   void SetBlockLargeFileTransferPolicy(BlockLargeFileTransferValues state);
   void SetUnsafeEventsReportingPolicy(bool report);
+  void AddUrlToCheckComplianceOfDownloads(const std::string& url);
 
   // Sets up a FakeDeepScanningDialogDelegate to use this class's StatusCallback
   // and EncryptionStatusCallback. Also sets up a test DM token.

chrome/browser/safe_browsing/download_protection/deep_scanning_browsertest.cc

@@ -119,12 +119,29 @@ class DownloadDeepScanningBrowserTest
     SafeBrowsingService::RegisterFactory(nullptr);
   }
 
+  void SetUpOnMainThread() override {
+    embedded_test_server()->ServeFilesFromDirectory(GetTestDataDirectory());
+    ASSERT_TRUE(embedded_test_server()->Start());
+    AddUrlToCheckComplianceOfDownloads(
+        embedded_test_server()->base_url().spec());
+
+    SetBinaryUploadServiceTestFactory();
+    SetUrlLoaderInterceptor();
+    ObserveDownloadManager();
+    AuthorizeForDeepScanning();
+
+    SetDMTokenForTesting(
+        policy::DMToken::CreateValidTokenForTesting("dm_token"));
+    SetDlpPolicy(CheckContentComplianceValues::CHECK_DOWNLOADS);
+    SetMalwarePolicy(SendFilesForMalwareCheckValues::SEND_DOWNLOADS);
+  }
+
   void WaitForDownloadToFinish() {
     content::DownloadManager* download_manager =
         content::BrowserContext::GetDownloadManager(browser()->profile());
     content::DownloadTestObserverTerminal observer(
         download_manager, 1,
-        content::DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_IGNORE);
+        content::DownloadTestObserver::ON_DANGEROUS_DOWNLOAD_QUIT);
     observer.WaitForFinished();
   }
 
@@ -267,18 +284,6 @@ class DownloadDeepScanningBrowserTest
 
 IN_PROC_BROWSER_TEST_F(DownloadDeepScanningBrowserTest,
                        SafeDownloadHasCorrectDangerType) {
-  embedded_test_server()->ServeFilesFromDirectory(GetTestDataDirectory());
-  ASSERT_TRUE(embedded_test_server()->Start());
-
-  SetBinaryUploadServiceTestFactory();
-  SetUrlLoaderInterceptor();
-  ObserveDownloadManager();
-  AuthorizeForDeepScanning();
-
-  SetDMTokenForTesting(policy::DMToken::CreateValidTokenForTesting("dm_token"));
-  SetDlpPolicy(CheckContentComplianceValues::CHECK_DOWNLOADS);
-  SetMalwarePolicy(SendFilesForMalwareCheckValues::SEND_DOWNLOADS);
-
   // The file is SAFE according to the metadata check
   ClientDownloadResponse metadata_response;
   metadata_response.set_verdict(ClientDownloadResponse::SAFE);
@@ -317,4 +322,124 @@ IN_PROC_BROWSER_TEST_F(DownloadDeepScanningBrowserTest,
   EXPECT_EQ(item->GetState(), download::DownloadItem::COMPLETE);
 }
 
+IN_PROC_BROWSER_TEST_F(DownloadDeepScanningBrowserTest, FailedScanFailsOpen) {
+  // The file is SAFE according to the metadata check
+  ClientDownloadResponse metadata_response;
+  metadata_response.set_verdict(ClientDownloadResponse::SAFE);
+  ExpectMetadataResponse(metadata_response);
+
+  // The DLP scan runs synchronously, but doesn't find anything.
+  DeepScanningClientResponse sync_response;
+  sync_response.mutable_dlp_scan_verdict()->set_status(
+      DlpDeepScanningVerdict::SUCCESS);
+  ExpectDeepScanSynchronousResponse(/*is_advanced_protection=*/false,
+                                    sync_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);
+
+  WaitForDeepScanRequest(/*is_advanced_protection=*/false);
+
+  // The malware scan finishes asynchronously, and fails
+  DeepScanningClientResponse async_response;
+  async_response.set_token(last_enterprise_request().request_token());
+  async_response.mutable_malware_scan_verdict()->set_verdict(
+      MalwareDeepScanningVerdict::SCAN_FAILURE);
+  SendFcmMessage(async_response);
+
+  WaitForDownloadToFinish();
+
+  // The file should be safe, but not deep scanned.
+  ASSERT_EQ(download_items().size(), 1u);
+  download::DownloadItem* item = *download_items().begin();
+  EXPECT_EQ(item->GetDangerType(),
+            download::DownloadDangerType::DOWNLOAD_DANGER_TYPE_NOT_DANGEROUS);
+  EXPECT_EQ(item->GetState(), download::DownloadItem::COMPLETE);
+}
+
+IN_PROC_BROWSER_TEST_F(DownloadDeepScanningBrowserTest,
+                       PartialFailureShowsMalwareWarning) {
+  // The file is SAFE according to the metadata check
+  ClientDownloadResponse metadata_response;
+  metadata_response.set_verdict(ClientDownloadResponse::SAFE);
+  ExpectMetadataResponse(metadata_response);
+
+  // The DLP scan runs synchronously, and fails.
+  DeepScanningClientResponse sync_response;
+  sync_response.mutable_dlp_scan_verdict()->set_status(
+      DlpDeepScanningVerdict::FAILURE);
+  ExpectDeepScanSynchronousResponse(/*is_advanced_protection=*/false,
+                                    sync_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);
+
+  WaitForDeepScanRequest(/*is_advanced_protection=*/false);
+
+  // The malware scan finishes asynchronously, and finds malware.
+  DeepScanningClientResponse async_response;
+  async_response.set_token(last_enterprise_request().request_token());
+  async_response.mutable_malware_scan_verdict()->set_verdict(
+      MalwareDeepScanningVerdict::MALWARE);
+  SendFcmMessage(async_response);
+
+  WaitForDownloadToFinish();
+
+  // The file should be dangerous.
+  ASSERT_EQ(download_items().size(), 1u);
+  download::DownloadItem* item = *download_items().begin();
+  EXPECT_EQ(
+      item->GetDangerType(),
+      download::DownloadDangerType::DOWNLOAD_DANGER_TYPE_DANGEROUS_CONTENT);
+  EXPECT_EQ(item->GetState(), download::DownloadItem::IN_PROGRESS);
+}
+
+IN_PROC_BROWSER_TEST_F(DownloadDeepScanningBrowserTest,
+                       PartialFailureShowsDlpWarning) {
+  // The file is SAFE according to the metadata check
+  ClientDownloadResponse metadata_response;
+  metadata_response.set_verdict(ClientDownloadResponse::SAFE);
+  ExpectMetadataResponse(metadata_response);
+
+  // The DLP scan runs synchronously, and finds a violation.
+  DeepScanningClientResponse sync_response;
+  sync_response.mutable_dlp_scan_verdict()->set_status(
+      DlpDeepScanningVerdict::SUCCESS);
+  sync_response.mutable_dlp_scan_verdict()->add_triggered_rules()->set_action(
+      DlpDeepScanningVerdict::TriggeredRule::BLOCK);
+  ExpectDeepScanSynchronousResponse(/*is_advanced_protection=*/false,
+                                    sync_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);
+
+  WaitForDeepScanRequest(/*is_advanced_protection=*/false);
+
+  // The malware scan finishes asynchronously, and fails.
+  DeepScanningClientResponse async_response;
+  async_response.set_token(last_enterprise_request().request_token());
+  async_response.mutable_malware_scan_verdict()->set_verdict(
+      MalwareDeepScanningVerdict::SCAN_FAILURE);
+  SendFcmMessage(async_response);
+
+  WaitForDownloadToFinish();
+
+  // The file should be blocked for sensitive content.
+  ASSERT_EQ(download_items().size(), 1u);
+  download::DownloadItem* item = *download_items().begin();
+  EXPECT_EQ(item->GetDangerType(),
+            download::DownloadDangerType::
+                DOWNLOAD_DANGER_TYPE_SENSITIVE_CONTENT_BLOCK);
+  EXPECT_EQ(item->GetState(), download::DownloadItem::INTERRUPTED);
+}
+
 }  // namespace safe_browsing

chrome/browser/safe_browsing/download_protection/deep_scanning_request.cc

@@ -75,6 +75,19 @@ void DeepScanningClientResponseToDownloadCheckResult(
     }
   }
 
+  if (response.has_malware_scan_verdict() &&
+      response.malware_scan_verdict().verdict() ==
+          MalwareDeepScanningVerdict::SCAN_FAILURE) {
+    *download_result = DownloadCheckResult::UNKNOWN;
+    return;
+  }
+
+  if (response.has_dlp_scan_verdict() &&
+      response.dlp_scan_verdict().status() != DlpDeepScanningVerdict::SUCCESS) {
+    *download_result = DownloadCheckResult::UNKNOWN;
+    return;
+  }
+
   *download_result = DownloadCheckResult::DEEP_SCANNED_SAFE;
 }
 

chrome/browser/safe_browsing/download_protection/deep_scanning_request_unittest.cc

@@ -586,7 +586,7 @@ TEST_F(DeepScanningReportingTest, ProcessesResponseCorrectly) {
 
     request.Start();
 
-    EXPECT_EQ(DownloadCheckResult::DEEP_SCANNED_SAFE, last_result_);
+    EXPECT_EQ(DownloadCheckResult::UNKNOWN, last_result_);
   }
 
   {
@@ -617,7 +617,7 @@ TEST_F(DeepScanningReportingTest, ProcessesResponseCorrectly) {
 
     request.Start();
 
-    EXPECT_EQ(DownloadCheckResult::DEEP_SCANNED_SAFE, last_result_);
+    EXPECT_EQ(DownloadCheckResult::UNKNOWN, last_result_);
   }
 }