From eea1c4bbbf146e39d4ef0599ec05a733cb4a1aab Mon Sep 17 00:00:00 2001
From: Ming-Ying Chung <[email protected]>
Date: Tue, 10 Jun 2025 06:34:30 -0700
Subject: [PATCH] Add safe browsing check to [`FileSystemHandle.move()`][1]

When the API is used together with `window.showSaveFilePicker()`,
e.g.

```js
(await window.showSaveFilePicker({suggestedName: `hello`})).move(`hello.swf`);
```

It may create a potentially ambiguous scenario that misleads users about
what the actual file is downloaded. In the above example, the final
downloaded file will be `hello.swf` (not `hello` from file picker UI).
Per [discussion][2], It's better to ask explicitly for confirmation.

This CL adds a call to `ConfirmSensitiveEntryAccess()` before calling
`FileSystemAccessSafeMoveHelper`, i.e. `DoFileSystemOperation()`. The
call in chrome embedder will check the destination path against
blocklist and [show prompt UI][3] for user permissions. Example screenshot: [4].


[1]: https://chromestatus.com/feature/5640802622504960
[2]: http://crbug.com/411544197#comment7
[3]: https://crsrc.org/c/chrome/browser/file_system_access/chrome_file_system_access_permission_context.cc;l=1982-2003;drc=1d737975521c1d4191937c2c659bd78d9f1681f4
[4]: http://screen/AUFXTfXZxvKufEJ

Bug: 411544197
Change-Id: I9a377b42495ef1f36bb02eb0be0fdfb6055d9d83
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/6475317
Reviewed-by: Austin Sullivan <[email protected]>
Commit-Queue: Ming-Ying Chung <[email protected]>
Cr-Commit-Position: refs/heads/main@{#1471747}
---

diff --git a/chrome/browser/file_system_access/chrome_file_system_access_permission_context_browsertest.cc b/chrome/browser/file_system_access/chrome_file_system_access_permission_context_browsertest.cc
index a5bc0a8..7db6c489c 100644
--- a/chrome/browser/file_system_access/chrome_file_system_access_permission_context_browsertest.cc
+++ b/chrome/browser/file_system_access/chrome_file_system_access_permission_context_browsertest.cc
@@ -72,8 +72,40 @@
     run_loop.Run();
   }
 
+  void ConfirmSensitiveEntryAccess(
+      const url::Origin& origin,
+      const content::PathInfo& path_info,
+      HandleType handle_type,
+      UserAction user_action,
+      content::GlobalRenderFrameHostId frame_id,
+      base::OnceCallback<void(SensitiveEntryResult)> callback) override {
+    confirm_sensitive_entry_access_ = true;
+    if (auto_abort_on_confirm_sensitive_entry_access_) {
+      std::move(callback).Run(SensitiveEntryResult::kAbort);
+      return;
+    }
+    ChromeFileSystemAccessPermissionContext::ConfirmSensitiveEntryAccess(
+        origin, path_info, handle_type, user_action, frame_id,
+        std::move(callback));
+  }
+
+  bool confirm_sensitive_entry_access() const {
+    return confirm_sensitive_entry_access_;
+  }
+
+  void set_auto_abort_on_confirm_sensitive_entry_access() {
+    auto_abort_on_confirm_sensitive_entry_access_ = true;
+  }
+
+  void reset() {
+    performed_after_write_checks_ = false;
+    confirm_sensitive_entry_access_ = false;
+  }
+
  private:
   bool performed_after_write_checks_ = false;
+  bool confirm_sensitive_entry_access_ = false;
+  bool auto_abort_on_confirm_sensitive_entry_access_ = false;
   base::OnceClosure quit_callback_;
 };
 
@@ -258,6 +290,77 @@
   ui::SelectFileDialog::SetFactory(nullptr);
 }
 
+// Tests that ConfirmSensitiveEntryAccess() is called by
+// 'FileSystemFileHandle.move()'.
+IN_PROC_BROWSER_TEST_F(
+    ChromeFileSystemAccessPermissionContextPrerenderingBrowserTest,
+    MoveFileAndConfirmSensitiveEntryAccess) {
+  const base::FilePath test_file = CreateTestFile("test.txt");
+  ui::SelectFileDialog::SetFactory(
+      std::make_unique<content::FakeSelectFileDialogFactory>(
+          std::vector<base::FilePath>{test_file}));
+
+  TestFileSystemAccessPermissionContext permission_context(
+      browser()->profile());
+  content::SetFileSystemAccessPermissionContext(browser()->profile(),
+                                                &permission_context);
+  FileSystemAccessPermissionRequestManager::FromWebContents(GetWebContents())
+      ->set_auto_response_for_test(permissions::PermissionAction::GRANTED);
+
+  // Initial navigation.
+  GURL initial_url = embedded_test_server()->GetURL("/empty.html");
+  ASSERT_NE(ui_test_utils::NavigateToURL(browser(), initial_url), nullptr);
+
+  // Expects no user interaction: showSaveFilePicker() automatically gets
+  // `test_file` from the fake file picker factory
+  // `FakeSelectFileDialogFactory` without the need for user interaction.
+  ASSERT_TRUE(ExecJs(GetWebContents(),
+                     R"(
+    var handle;
+    (async () =>{
+      handle = await self.showSaveFilePicker();
+    })()
+  )"));
+  EXPECT_EQ(test_file.BaseName().AsUTF8Unsafe(),
+            EvalJs(GetWebContents(), "handle.name"));
+  // Checks that PerformAfterWriteChecks() must not be called.
+  EXPECT_FALSE(permission_context.performed_after_write_checks());
+  // Checks that ConfirmSensitiveEntryAccess() is called within file picker,
+  // i.e. FileSystemAccessManagerImpl::DidChooseEntries.
+  EXPECT_TRUE(permission_context.confirm_sensitive_entry_access());
+
+  // Resets permission_context to receive new behavior.
+  permission_context.reset();
+
+  // Calling move() with '.swf' will trigger a SafeBrowsing check after calling
+  // `ConfirmSensitiveEntryAccess()`, which prompts the user to confirm saving
+  // such file.
+
+  // This line automatically aborts on calling ConfirmSensitiveEntryAccess() to
+  // bypass the SafeBrowsing dialog, as there is no way to accept the prompt
+  // in browser tests.
+  // Commenting this out will bring up the dialog and fail the test without a
+  // manual click.
+  permission_context.set_auto_abort_on_confirm_sensitive_entry_access();
+
+  EXPECT_THAT(
+      EvalJs(GetWebContents(),
+             R"(
+      handle.move("test.swf");
+  )"),
+      testing::Field(
+          &content::EvalJsResult::error,
+          testing::Eq(
+              "a JavaScript error: \"TypeError: Failed to execute 'move' on "
+              "'FileSystemFileHandle'\"\n")));
+  // Checks that ConfirmSensitiveEntryAccess() is called again to verify the
+  // move target file name.
+  EXPECT_TRUE(permission_context.confirm_sensitive_entry_access());
+
+  // Uninstall fake file picker factory.
+  ui::SelectFileDialog::SetFactory(nullptr);
+}
+
 class FileSystemChromeAppTest : public extensions::PlatformAppBrowserTest {
  public:
   FileSystemChromeAppTest() {
diff --git a/content/browser/file_system_access/file_system_access_file_handle_impl_unittest.cc b/content/browser/file_system_access/file_system_access_file_handle_impl_unittest.cc
index e96bd8c..174140ca 100644
--- a/content/browser/file_system_access/file_system_access_file_handle_impl_unittest.cc
+++ b/content/browser/file_system_access/file_system_access_file_handle_impl_unittest.cc
@@ -6,6 +6,7 @@
 
 #include <limits>
 #include <memory>
+#include <optional>
 #include <string>
 #include <utility>
 #include <vector>
@@ -16,6 +17,7 @@
 #include "base/files/file_util.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/memory/scoped_refptr.h"
+#include "base/strings/strcat.h"
 #include "base/strings/stringprintf.h"
 #include "base/task/sequenced_task_runner.h"
 #include "base/task/single_thread_task_runner.h"
@@ -67,6 +69,13 @@
 using blink::mojom::PermissionStatus;
 using storage::FileSystemURL;
 
+// Test fixture for FileSystemAccessFileHandleImpl.
+//
+// The permission context in this fixture is always null, so the relevant
+// permission checks are skipped.
+//
+// Tests with mock permission context are in
+// FileSystemAccessFileHandleImplMovePermissionsTest.
 class FileSystemAccessFileHandleImplTest : public testing::Test {
  public:
   FileSystemAccessFileHandleImplTest() = default;
@@ -244,6 +253,7 @@
   scoped_refptr<storage::MockQuotaManagerProxy> quota_manager_proxy_;
   scoped_refptr<storage::FileSystemContext> file_system_context_;
   scoped_refptr<ChromeBlobStorageContext> chrome_blob_context_;
+  testing::NiceMock<MockFileSystemAccessPermissionContext> permission_context_;
   scoped_refptr<FileSystemAccessManagerImpl> manager_;
 

diff --git a/chrome/browser/file_system_access/chrome_file_system_access_permission_context_browsertest.cc b/chrome/browser/file_system_access/chrome_file_system_access_permission_context_browsertest.cc
index a5bc0a8..7db6c489c 100644
--- a/chrome/browser/file_system_access/chrome_file_system_access_permission_context_browsertest.cc
+++ b/chrome/browser/file_system_access/chrome_file_system_access_permission_context_browsertest.cc
@@ -72,8 +72,40 @@
     run_loop.Run();
   }
 
+  void ConfirmSensitiveEntryAccess(
+      const url::Origin& origin,
+      const content::PathInfo& path_info,
+      HandleType handle_type,
+      UserAction user_action,
+      content::GlobalRenderFrameHostId frame_id,
+      base::OnceCallback<void(SensitiveEntryResult)> callback) override {
+    confirm_sensitive_entry_access_ = true;
+    if (auto_abort_on_confirm_sensitive_entry_access_) {
+      std::move(callback).Run(SensitiveEntryResult::kAbort);
+      return;
+    }
+    ChromeFileSystemAccessPermissionContext::ConfirmSensitiveEntryAccess(
+        origin, path_info, handle_type, user_action, frame_id,
+        std::move(callback));
+  }
+
+  bool confirm_sensitive_entry_access() const {
+    return confirm_sensitive_entry_access_;
+  }
+
+  void set_auto_abort_on_confirm_sensitive_entry_access() {
+    auto_abort_on_confirm_sensitive_entry_access_ = true;
+  }
+
+  void reset() {
+    performed_after_write_checks_ = false;
+    confirm_sensitive_entry_access_ = false;
+  }
+
  private:
   bool performed_after_write_checks_ = false;
+  bool confirm_sensitive_entry_access_ = false;
+  bool auto_abort_on_confirm_sensitive_entry_access_ = false;
   base::OnceClosure quit_callback_;
 };
 
@@ -258,6 +290,77 @@
   ui::SelectFileDialog::SetFactory(nullptr);
 }
 
+// Tests that ConfirmSensitiveEntryAccess() is called by
+// 'FileSystemFileHandle.move()'.
+IN_PROC_BROWSER_TEST_F(
+    ChromeFileSystemAccessPermissionContextPrerenderingBrowserTest,
+    MoveFileAndConfirmSensitiveEntryAccess) {
+  const base::FilePath test_file = CreateTestFile("test.txt");
+  ui::SelectFileDialog::SetFactory(
+      std::make_unique<content::FakeSelectFileDialogFactory>(
+          std::vector<base::FilePath>{test_file}));
+
+  TestFileSystemAccessPermissionContext permission_context(
+      browser()->profile());
+  content::SetFileSystemAccessPermissionContext(browser()->profile(),
+                                                &permission_context);
+  FileSystemAccessPermissionRequestManager::FromWebContents(GetWebContents())
+      ->set_auto_response_for_test(permissions::PermissionAction::GRANTED);
+
+  // Initial navigation.
+  GURL initial_url = embedded_test_server()->GetURL("/empty.html");
+  ASSERT_NE(ui_test_utils::NavigateToURL(browser(), initial_url), nullptr);
+
+  // Expects no user interaction: showSaveFilePicker() automatically gets
+  // `test_file` from the fake file picker factory
+  // `FakeSelectFileDialogFactory` without the need for user interaction.
+  ASSERT_TRUE(ExecJs(GetWebContents(),
+                     R"(
+    var handle;
+    (async () =>{
+      handle = await self.showSaveFilePicker();
+    })()
+  )"));
+  EXPECT_EQ(test_file.BaseName().AsUTF8Unsafe(),
+            EvalJs(GetWebContents(), "handle.name"));
+  // Checks that PerformAfterWriteChecks() must not be called.
+  EXPECT_FALSE(permission_context.performed_after_write_checks());
+  // Checks that ConfirmSensitiveEntryAccess() is called within file picker,
+  // i.e. FileSystemAccessManagerImpl::DidChooseEntries.
+  EXPECT_TRUE(permission_context.confirm_sensitive_entry_access());
+
+  // Resets permission_context to receive new behavior.
+  permission_context.reset();
+
+  // Calling move() with '.swf' will trigger a SafeBrowsing check after calling
+  // `ConfirmSensitiveEntryAccess()`, which prompts the user to confirm saving
+  // such file.
+
+  // This line automatically aborts on calling ConfirmSensitiveEntryAccess() to
+  // bypass the SafeBrowsing dialog, as there is no way to accept the prompt
+  // in browser tests.
+  // Commenting this out will bring up the dialog and fail the test without a
+  // manual click.
+  permission_context.set_auto_abort_on_confirm_sensitive_entry_access();
+
+  EXPECT_THAT(
+      EvalJs(GetWebContents(),
+             R"(
+      handle.move("test.swf");
+  )"),
+      testing::Field(
+          &content::EvalJsResult::error,
+          testing::Eq(
+              "a JavaScript error: \"TypeError: Failed to execute 'move' on "
+              "'FileSystemFileHandle'\"\n")));
+  // Checks that ConfirmSensitiveEntryAccess() is called again to verify the
+  // move target file name.
+  EXPECT_TRUE(permission_context.confirm_sensitive_entry_access());
+
+  // Uninstall fake file picker factory.
+  ui::SelectFileDialog::SetFactory(nullptr);
+}
+
 class FileSystemChromeAppTest : public extensions::PlatformAppBrowserTest {
  public:
   FileSystemChromeAppTest() {
diff --git a/content/browser/file_system_access/file_system_access_file_handle_impl_unittest.cc b/content/browser/file_system_access/file_system_access_file_handle_impl_unittest.cc
index e96bd8c..174140ca 100644
--- a/content/browser/file_system_access/file_system_access_file_handle_impl_unittest.cc
+++ b/content/browser/file_system_access/file_system_access_file_handle_impl_unittest.cc
@@ -6,6 +6,7 @@
 
 #include <limits>
 #include <memory>
+#include <optional>
 #include <string>
 #include <utility>
 #include <vector>
@@ -16,6 +17,7 @@
 #include "base/files/file_util.h"
 #include "base/files/scoped_temp_dir.h"
 #include "base/memory/scoped_refptr.h"
+#include "base/strings/strcat.h"
 #include "base/strings/stringprintf.h"
 #include "base/task/sequenced_task_runner.h"
 #include "base/task/single_thread_task_runner.h"
@@ -67,6 +69,13 @@
 using blink::mojom::PermissionStatus;
 using storage::FileSystemURL;
 
+// Test fixture for FileSystemAccessFileHandleImpl.
+//
+// The permission context in this fixture is always null, so the relevant
+// permission checks are skipped.
+//
+// Tests with mock permission context are in
+// FileSystemAccessFileHandleImplMovePermissionsTest.
 class FileSystemAccessFileHandleImplTest : public testing::Test {
  public:
   FileSystemAccessFileHandleImplTest() = default;
@@ -244,6 +253,7 @@
   scoped_refptr<storage::MockQuotaManagerProxy> quota_manager_proxy_;
   scoped_refptr<storage::FileSystemContext> file_system_context_;
   scoped_refptr<ChromeBlobStorageContext> chrome_blob_context_;
+  testing::NiceMock<MockFileSystemAccessPermissionContext> permission_context_;
   scoped_refptr<FileSystemAccessManagerImpl> manager_;
 
   raw_ptr<WebContents> web_contents_ = nullptr;
@@ -986,6 +996,13 @@
 }
 #endif  // BUILDFLAG(IS_MAC)
 
+struct MovePermissionsTestCase {
+  // Is there a file to be overwritten?
+  bool target_present;
+  // Does the site have user activation?
+  bool gesture_present;
+};
+
 // Uses a mock permission context to ensure the correct permission grant for the
 // target file (and parent, for renames) is used, since moves retrieve the
 // target's permission grant via GetSharedHandleStateForNonSandboxedPath() which
@@ -997,7 +1014,7 @@
 // access to the destination directory.
 class FileSystemAccessFileHandleImplMovePermissionsTest
     : public FileSystemAccessFileHandleImplTest,
-      public testing::WithParamInterface<std::tuple<bool, bool>> {
+      public testing::WithParamInterface<MovePermissionsTestCase> {
  public:
   void SetUp() override {
     base::CommandLine::ForCurrentProcess()->AppendSwitch(
@@ -1008,8 +1025,8 @@
     manager_->SetPermissionContextForTesting(&permission_context_);
   }
 
-  bool target_present() const { return std::get<0>(GetParam()); }
-  bool gesture_present() const { return std::get<1>(GetParam()); }
+  bool target_present() const { return GetParam().target_present; }
+  bool gesture_present() const { return GetParam().gesture_present; }
 
   std::pair<base::FilePath, base::FilePath> CreateSourceAndMaybeTarget() {
     base::FilePath source;
@@ -1053,6 +1070,15 @@
                     FileSystemAccessPermissionContext::HandleType::kFile,
                     FileSystemAccessPermissionContext::UserAction::kNone))
         .WillOnce(testing::Return(target_grant));
+    EXPECT_CALL(
+        permission_context_,
+        ConfirmSensitiveEntryAccess_(
+            origin, content::PathInfo(target),
+            FileSystemAccessPermissionContext::HandleType::kFile,
+            FileSystemAccessPermissionContext::UserAction::kSave,
+            web_contents_->GetPrimaryMainFrame()->GetGlobalId(), testing::_))
+        .WillOnce(base::test::RunOnceCallback<5>(
+            FileSystemAccessPermissionContext::SensitiveEntryResult::kAllowed));
 
     // These checks should only be called if the file is successfully moved.
 
@@ -1093,7 +1119,9 @@
       const base::FilePath& target,
       scoped_refptr<FixedFileSystemAccessPermissionGrant> target_grant,
       blink::mojom::FileSystemAccessStatus result,
-      bool expects_safe_name = true) {
+      bool expects_safe_name = true,
+      std::optional<FileSystemAccessPermissionContext::SensitiveEntryResult>
+          expected_sensitive_entry_result = std::nullopt) {
     auto source_handle =
         GetHandleWithPermissions(source, /*read_grant=*/allow_grant_,
                                  /*write_grant=*/allow_grant_);
@@ -1105,7 +1133,18 @@
 
     EXPECT_CALL(permission_context_,
                 IsFileTypeDangerous_(target_basename, origin))
-        .WillOnce(testing::Return(!expects_safe_name));
+        .WillRepeatedly(testing::Return(!expects_safe_name));
+    if (expected_sensitive_entry_result.has_value()) {
+      EXPECT_CALL(
+          permission_context_,
+          ConfirmSensitiveEntryAccess_(
+              origin, content::PathInfo(target),
+              FileSystemAccessPermissionContext::HandleType::kFile,
+              FileSystemAccessPermissionContext::UserAction::kSave,
+              web_contents_->GetPrimaryMainFrame()->GetGlobalId(), testing::_))
+          .WillOnce(
+              base::test::RunOnceCallback<5>(*expected_sensitive_entry_result));
+    }
     if (expects_safe_name) {
       // If safe name check has passed, it should be expected to get grants.
       EXPECT_CALL(permission_context_,
@@ -1156,6 +1195,15 @@
     EXPECT_CALL(permission_context_,
                 IsFileTypeDangerous_(target_basename, origin))
         .WillRepeatedly(testing::Return(false));
+    EXPECT_CALL(
+        permission_context_,
+        ConfirmSensitiveEntryAccess_(
+            origin, content::PathInfo(target),
+            FileSystemAccessPermissionContext::HandleType::kFile,
+            FileSystemAccessPermissionContext::UserAction::kSave,
+            web_contents_->GetPrimaryMainFrame()->GetGlobalId(), testing::_))
+        .WillOnce(base::test::RunOnceCallback<5>(
+            FileSystemAccessPermissionContext::SensitiveEntryResult::kAllowed));
 
     // These checks should only be called if the file is successfully moved.
     if (source != target) {
@@ -1195,7 +1243,9 @@
       const base::FilePath& target,
       scoped_refptr<FixedFileSystemAccessPermissionGrant> target_grant,
       blink::mojom::FileSystemAccessStatus result,
-      bool expects_safe_name = true) {
+      bool expects_safe_name = true,
+      std::optional<FileSystemAccessPermissionContext::SensitiveEntryResult>
+          expected_sensitive_entry_result = std::nullopt) {
     base::FilePath target_parent = target.DirName();
     // The site has write access to the destination directory.
     auto dest_dir_handle = GetDirectoryHandleWithPermissions(
@@ -1208,7 +1258,18 @@
 
     EXPECT_CALL(permission_context_,
                 IsFileTypeDangerous_(target_basename, origin))
-        .WillOnce(testing::Return(!expects_safe_name));
+        .WillRepeatedly(testing::Return(!expects_safe_name));
+    if (expected_sensitive_entry_result.has_value()) {
+      EXPECT_CALL(
+          permission_context_,
+          ConfirmSensitiveEntryAccess_(
+              origin, content::PathInfo(target),
+              FileSystemAccessPermissionContext::HandleType::kFile,
+              FileSystemAccessPermissionContext::UserAction::kSave,
+              web_contents_->GetPrimaryMainFrame()->GetGlobalId(), testing::_))
+          .WillOnce(
+              base::test::RunOnceCallback<5>(*expected_sensitive_entry_result));
+    }
 
     // No after-write checks needed since the file should not have been moved.
 
@@ -1230,6 +1291,24 @@
       permission_context_;
 };
 
+INSTANTIATE_TEST_SUITE_P(
+    All,
+    FileSystemAccessFileHandleImplMovePermissionsTest,
... (truncated)