From ade24699eeba445f5c671a73103f60442a1539ed Mon Sep 17 00:00:00 2001
From: Min Qin <qinmin@chromium.org>
Date: Wed, 22 Oct 2025 11:39:53 -0700
Subject: [PATCH] Run quarantine for all file save items

Currently, quarantine is only run for files saved from the network.
This CL make Chrome to also run quarantine for files that is
saved from DOM since the DOM content is also from the web. This CL
adds a quarantine callback to each SaveFile, so when SaveFile is
finished, the quarantine will be invoked before completing the
SavePackage.

Bug: 429140219
Change-Id: Id1b2891a4ed48a17b77f83f0de671e8baa6e5159
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/7046052
Reviewed-by: Shakti Sahu <shaktisahu@chromium.org>
Commit-Queue: Min Qin <qinmin@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1533793}
---

diff --git a/chrome/browser/download/save_page_browsertest.cc b/chrome/browser/download/save_page_browsertest.cc
index a2385a2..e038e24 100644
--- a/chrome/browser/download/save_page_browsertest.cc
+++ b/chrome/browser/download/save_page_browsertest.cc
@@ -69,6 +69,7 @@
 #include "ui/shell_dialogs/fake_select_file_dialog.h"
 
 #if BUILDFLAG(IS_CHROMEOS)
+#include "base/test/test_future.h"
 #include "chromeos/dbus/dlp/dlp_client.h"
 #endif  // BUILDFLAG(IS_CHROMEOS)
 
@@ -1712,27 +1713,40 @@
 
   chromeos::DlpClient::Shutdown();
   chromeos::DlpClient::InitializeFake();
-  base::test::RepeatingTestFuture<
-      dlp::AddFilesRequest, base::OnceCallback<void(dlp::AddFilesResponse)>>
-      add_file_cb;
-  chromeos::DlpClient::Get()->GetTestInterface()->SetAddFilesMock(
-      add_file_cb.GetCallback());
 
-  url = NavigateToMockURL("a");
+  // Use page "b" which has subresources (1.png, 1.css). Total 3 files.
+  url = NavigateToMockURL("b");
 
-  SaveCurrentTab(url, content::SAVE_PAGE_TYPE_AS_COMPLETE_HTML, "a", 1, &dir,
+  SaveCurrentTab(url, content::SAVE_PAGE_TYPE_AS_COMPLETE_HTML, "b", 3, &dir,
                  &full_file_name);
 
   ASSERT_FALSE(HasFailure());
 
-  auto request = std::get<0>(add_file_cb.Take());
-  ASSERT_EQ(1, request.add_file_requests().size());
-  EXPECT_EQ(full_file_name.value(), request.add_file_requests(0).file_path());
-  EXPECT_EQ(request.add_file_requests(0).source_url(), url.spec());
+  // Asynchronously get the recorded requests from the fake client.
+  base::test::TestFuture<const dlp::GetDatabaseEntriesResponse> future;
+  chromeos::DlpClient::Get()->GetDatabaseEntries(future.GetCallback());
+  const auto& response = future.Get();
+  const auto& requests = response.files_entries();
 
-  base::ScopedAllowBlockingForTesting allow_blocking;
-  EXPECT_TRUE(base::PathExists(full_file_name));
-  EXPECT_FALSE(base::PathExists(dir));
+  // There is a total of 6 requests, 3 for temporary files and 3 for final
+  // destination.
+  ASSERT_EQ(6, requests.size());
+
+  // The order of subresource saving is not guaranteed, so we use a set
+  // to verify the presence of each expected file path.
+  std::set<std::string> expected_paths;
+  expected_paths.insert(full_file_name.value());
+  expected_paths.insert(dir.AppendASCII("1.png").value());
+  expected_paths.insert(dir.AppendASCII("1.css").value());
+
+  std::set<std::string> actual_paths;
+  for (const auto& request : requests) {
+    actual_paths.insert(request.path());
+  }
+
+  for (const auto& expected_path : expected_paths) {
+    EXPECT_TRUE(base::Contains(actual_paths, expected_path));
+  }
 }
 
 IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SaveMHTMLWithDlp) {
diff --git a/content/browser/download/save_file.cc b/content/browser/download/save_file.cc
index 6eed86d5..89dfc43 100644
--- a/content/browser/download/save_file.cc
+++ b/content/browser/download/save_file.cc
@@ -91,4 +91,10 @@
   return file_.DebugString();
 }
 
+void SaveFile::RunQuarantineCallback() {
+  if (!info_->quarantine_callback.is_null()) {
+    std::move(info_->quarantine_callback).Run();
+  }
+}
+
 }  // namespace content
diff --git a/content/browser/download/save_file.h b/content/browser/download/save_file.h
index 77dde846..71497ef 100644
--- a/content/browser/download/save_file.h
+++ b/content/browser/download/save_file.h
@@ -56,6 +56,8 @@
   }
   const SaveFileCreateInfo& create_info() const { return *info_; }
 
+  void RunQuarantineCallback();
+
  private:
   download::BaseFile file_;
   std::unique_ptr<SaveFileCreateInfo> info_;
diff --git a/content/browser/download/save_file_manager.cc b/content/browser/download/save_file_manager.cc
index 41f27959..5e1057c 100644
--- a/content/browser/download/save_file_manager.cc
+++ b/content/browser/download/save_file_manager.cc
@@ -71,11 +71,13 @@
       const net::NetworkTrafficAnnotationTag& annotation_tag,
       network::mojom::URLLoaderFactory* url_loader_factory,
       SaveFileManager* save_file_manager,
+      base::OnceClosure quarantine_callback,
       URLLoaderCompleteCallback on_complete_cb) {
     return std::unique_ptr<SimpleURLLoaderHelper>(new SimpleURLLoaderHelper(
         std::move(resource_request), save_item_id, save_package_id,
         render_process_id, render_frame_routing_id, annotation_tag,
-        url_loader_factory, save_file_manager, std::move(on_complete_cb)));
+        url_loader_factory, save_file_manager, std::move(quarantine_callback),
+        std::move(on_complete_cb)));
   }
 
   SimpleURLLoaderHelper(const SimpleURLLoaderHelper&) = delete;
@@ -93,10 +95,12 @@
       const net::NetworkTrafficAnnotationTag& annotation_tag,
       network::mojom::URLLoaderFactory* url_loader_factory,
       SaveFileManager* save_file_manager,
+      base::OnceClosure quarantine_callback,
       URLLoaderCompleteCallback on_complete_cb)
       : save_file_manager_(save_file_manager),
         save_item_id_(save_item_id),
         save_package_id_(save_package_id),
+        quarantine_callback_(std::move(quarantine_callback)),
         on_complete_cb_(std::move(on_complete_cb)) {
     GURL url = resource_request->url;
     url_loader_ = network::SimpleURLLoader::Create(std::move(resource_request),
@@ -124,6 +128,7 @@
     auto info = std::make_unique<SaveFileCreateInfo>(
         url, final_url, save_item_id_, save_package_id_, render_process_id,
         render_frame_routing_id, content_disposition);
+    info->quarantine_callback = std::move(quarantine_callback_);
     download::GetDownloadTaskRunner()->PostTask(
         FROM_HERE, base::BindOnce(&SaveFileManager::StartSave,
                                   save_file_manager_, std::move(info)));
@@ -155,6 +160,7 @@
   SaveItemId save_item_id_;
   SavePackageId save_package_id_;
   std::unique_ptr<network::SimpleURLLoader> url_loader_;
+  base::OnceClosure quarantine_callback_;
   URLLoaderCompleteCallback on_complete_cb_;
 };
 
@@ -227,6 +233,12 @@
   DCHECK(!base::Contains(packages_, save_item_id));
   packages_[save_item_id] = save_package;
 
+  base::OnceClosure quarantine_callback = base::BindOnce(
+      &SaveFileManager::QuarantineItem, this, save_item_id, save_package->id(),
+      context->IsOffTheRecord() ? GURL() : url,
+      context->IsOffTheRecord() ? GURL() : referrer.url, client_guid,
+      std::move(remote_quarantine));
+
   // Register a saving job.
   if (save_source == SaveFileCreateInfo::SAVE_FILE_FROM_NET) {
     DCHECK(url.is_valid());
@@ -317,22 +329,20 @@
     }
 
     base::OnceCallback<void(bool /*success*/)> save_finished_cb =
-        base::BindOnce(&SaveFileManager::OnURLLoaderComplete, this,
-                       save_item_id, save_package->id(),
-                       context->IsOffTheRecord() ? GURL() : url,
-                       context->IsOffTheRecord() ? GURL() : referrer.url,
-                       client_guid, std::move(remote_quarantine));
-
+        base::BindOnce(&SaveFileManager::SaveFinished, this, save_item_id,
+                       save_package->id());
     url_loader_helpers_[save_item_id] =
         SimpleURLLoaderHelper::CreateAndStartDownload(
             std::move(request), save_item_id, save_package->id(),
             render_process_host_id, render_frame_routing_id, traffic_annotation,
-            factory, this, std::move(save_finished_cb));
+            factory, this, std::move(quarantine_callback),
+            std::move(save_finished_cb));
   } else {
     // We manually start the save job.
     auto info = std::make_unique<SaveFileCreateInfo>(
         file_full_path, url, save_item_id, save_package->id(),
         render_process_host_id, render_frame_routing_id, save_source);

diff --git a/chrome/browser/download/save_page_browsertest.cc b/chrome/browser/download/save_page_browsertest.cc
index a2385a2..e038e24 100644
--- a/chrome/browser/download/save_page_browsertest.cc
+++ b/chrome/browser/download/save_page_browsertest.cc
@@ -69,6 +69,7 @@
 #include "ui/shell_dialogs/fake_select_file_dialog.h"
 
 #if BUILDFLAG(IS_CHROMEOS)
+#include "base/test/test_future.h"
 #include "chromeos/dbus/dlp/dlp_client.h"
 #endif  // BUILDFLAG(IS_CHROMEOS)
 
@@ -1712,27 +1713,40 @@
 
   chromeos::DlpClient::Shutdown();
   chromeos::DlpClient::InitializeFake();
-  base::test::RepeatingTestFuture<
-      dlp::AddFilesRequest, base::OnceCallback<void(dlp::AddFilesResponse)>>
-      add_file_cb;
-  chromeos::DlpClient::Get()->GetTestInterface()->SetAddFilesMock(
-      add_file_cb.GetCallback());
 
-  url = NavigateToMockURL("a");
+  // Use page "b" which has subresources (1.png, 1.css). Total 3 files.
+  url = NavigateToMockURL("b");
 
-  SaveCurrentTab(url, content::SAVE_PAGE_TYPE_AS_COMPLETE_HTML, "a", 1, &dir,
+  SaveCurrentTab(url, content::SAVE_PAGE_TYPE_AS_COMPLETE_HTML, "b", 3, &dir,
                  &full_file_name);
 
   ASSERT_FALSE(HasFailure());
 
-  auto request = std::get<0>(add_file_cb.Take());
-  ASSERT_EQ(1, request.add_file_requests().size());
-  EXPECT_EQ(full_file_name.value(), request.add_file_requests(0).file_path());
-  EXPECT_EQ(request.add_file_requests(0).source_url(), url.spec());
+  // Asynchronously get the recorded requests from the fake client.
+  base::test::TestFuture<const dlp::GetDatabaseEntriesResponse> future;
+  chromeos::DlpClient::Get()->GetDatabaseEntries(future.GetCallback());
+  const auto& response = future.Get();
+  const auto& requests = response.files_entries();
 
-  base::ScopedAllowBlockingForTesting allow_blocking;
-  EXPECT_TRUE(base::PathExists(full_file_name));
-  EXPECT_FALSE(base::PathExists(dir));
+  // There is a total of 6 requests, 3 for temporary files and 3 for final
+  // destination.
+  ASSERT_EQ(6, requests.size());
+
+  // The order of subresource saving is not guaranteed, so we use a set
+  // to verify the presence of each expected file path.
+  std::set<std::string> expected_paths;
+  expected_paths.insert(full_file_name.value());
+  expected_paths.insert(dir.AppendASCII("1.png").value());
+  expected_paths.insert(dir.AppendASCII("1.css").value());
+
+  std::set<std::string> actual_paths;
+  for (const auto& request : requests) {
+    actual_paths.insert(request.path());
+  }
+
+  for (const auto& expected_path : expected_paths) {
+    EXPECT_TRUE(base::Contains(actual_paths, expected_path));
+  }
 }
 
 IN_PROC_BROWSER_TEST_F(SavePageBrowserTest, SaveMHTMLWithDlp) {