[NativeFS] Move creation/truncation of save files till after restricted folder check.

We shouldn't be creating or truncating files if the access to the directory
is going to be blocked by the restricted folder checks. So swap around the
order of the two operations.

Bug: 999788
Change-Id: I0b7519e079f79f4cb031f8f5d1d905fee1bbbb60
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1783725
Commit-Queue: Marijn Kruisselbrink <mek@chromium.org>
Commit-Queue: Victor Costan <pwnall@chromium.org>
Reviewed-by: Victor Costan <pwnall@chromium.org>
Cr-Commit-Position: refs/heads/master@{#692940}

content/browser/native_file_system/file_system_chooser.cc

@@ -180,44 +180,6 @@ void FileSystemChooser::MultiFilesSelected(
   DCHECK(isolated_context);
 
   RecordFileSelectionResult(type_, files.size());
-
-  if (type_ == blink::mojom::ChooseFileSystemEntryType::kSaveFile) {
-    // Create files if they don't yet exist, and truncate files if they do
-    // exist.
-    base::PostTask(
-        FROM_HERE,
-        {base::ThreadPool(), base::TaskPriority::USER_BLOCKING,
-         base::MayBlock()},
-        base::BindOnce(
-            [](const std::vector<base::FilePath>& files,
-               scoped_refptr<base::TaskRunner> callback_runner,
-               ResultCallback callback) {
-              for (const auto& path : files) {
-                int creation_flags =
-                    base::File::FLAG_CREATE_ALWAYS | base::File::FLAG_WRITE;
-                base::File file(path, creation_flags);
-
-                if (!file.IsValid()) {
-                  callback_runner->PostTask(
-                      FROM_HERE,
-                      base::BindOnce(std::move(callback),
-                                     native_file_system_error::FromStatus(
-                                         blink::mojom::NativeFileSystemStatus::
-                                             kOperationFailed,
-                                         "Failed to create file"),
-                                     std::vector<base::FilePath>()));
-                  return;
-                }
-              }
-              callback_runner->PostTask(
-                  FROM_HERE, base::BindOnce(std::move(callback),
-                                            native_file_system_error::Ok(),
-                                            std::move(files)));
-            },
-            files, callback_runner_, std::move(callback_)));
-    delete this;
-    return;
-  }
   callback_runner_->PostTask(
       FROM_HERE,
       base::BindOnce(std::move(callback_), native_file_system_error::Ok(),

content/browser/native_file_system/file_system_chooser_browsertest.cc

@@ -245,6 +245,85 @@ IN_PROC_BROWSER_TEST_F(FileSystemChooserBrowserTest, OpenDirectory_DenyAccess) {
       << result.error;
 }
 
+IN_PROC_BROWSER_TEST_F(FileSystemChooserBrowserTest,
+                       SaveFile_SensitiveDirectory_ExistingFile) {
+  const std::string file_contents = "Hello World";
+  const base::FilePath test_file = CreateTestFile(file_contents);
+
+  SelectFileDialogParams dialog_params;
+  ui::SelectFileDialog::SetFactory(
+      new FakeSelectFileDialogFactory({test_file}, &dialog_params));
+
+  testing::StrictMock<MockNativeFileSystemPermissionContext> permission_context;
+  static_cast<NativeFileSystemManagerImpl*>(
+      BrowserContext::GetStoragePartition(
+          shell()->web_contents()->GetBrowserContext(),
+          shell()->web_contents()->GetSiteInstance())
+          ->GetNativeFileSystemEntryFactory())
+      ->SetPermissionContextForTesting(&permission_context);
+
+  EXPECT_CALL(permission_context, ConfirmSensitiveDirectoryAccess_(
+                                      testing::_, testing::_, testing::_,
+                                      testing::_, testing::_, testing::_))
+      .WillOnce(RunOnceCallback<5>(SensitiveDirectoryResult::kAbort));
+
+  ASSERT_TRUE(
+      NavigateToURL(shell(), embedded_test_server()->GetURL("/title1.html")));
+  auto result =
+      EvalJs(shell(), "self.chooseFileSystemEntries({type: 'saveFile'})");
+  EXPECT_TRUE(result.error.find("aborted") != std::string::npos)
+      << result.error;
+
+  {
+    // File should still exist, and be unmodified.
+    base::ScopedAllowBlockingForTesting allow_blocking;
+    std::string read_contents;
+    EXPECT_TRUE(base::ReadFileToString(test_file, &read_contents));
+    EXPECT_EQ(file_contents, read_contents);
+  }
+}
+
+IN_PROC_BROWSER_TEST_F(FileSystemChooserBrowserTest,
+                       SaveFile_SensitiveDirectory_NonExistingFile) {
+  const base::FilePath test_file = CreateTestFile("");
+  {
+    // Delete file, since SaveFile should be able to deal with non-existing
+    // files.
+    base::ScopedAllowBlockingForTesting allow_blocking;
+    ASSERT_TRUE(base::DeleteFile(test_file, false));
+  }
+
+  SelectFileDialogParams dialog_params;
+  ui::SelectFileDialog::SetFactory(
+      new FakeSelectFileDialogFactory({test_file}, &dialog_params));
+
+  testing::StrictMock<MockNativeFileSystemPermissionContext> permission_context;
+  static_cast<NativeFileSystemManagerImpl*>(
+      BrowserContext::GetStoragePartition(
+          shell()->web_contents()->GetBrowserContext(),
+          shell()->web_contents()->GetSiteInstance())
+          ->GetNativeFileSystemEntryFactory())
+      ->SetPermissionContextForTesting(&permission_context);
+
+  EXPECT_CALL(permission_context, ConfirmSensitiveDirectoryAccess_(
+                                      testing::_, testing::_, testing::_,
+                                      testing::_, testing::_, testing::_))
+      .WillOnce(RunOnceCallback<5>(SensitiveDirectoryResult::kAbort));
+
+  ASSERT_TRUE(
+      NavigateToURL(shell(), embedded_test_server()->GetURL("/title1.html")));
+  auto result =
+      EvalJs(shell(), "self.chooseFileSystemEntries({type: 'saveFile'})");
+  EXPECT_TRUE(result.error.find("aborted") != std::string::npos)
+      << result.error;
+
+  {
+    // File should not have been created.
+    base::ScopedAllowBlockingForTesting allow_blocking;
+    EXPECT_FALSE(base::PathExists(test_file));
+  }
+}
+
 IN_PROC_BROWSER_TEST_F(FileSystemChooserBrowserTest, AcceptsOptions) {
   SelectFileDialogParams dialog_params;
   ui::SelectFileDialog::SetFactory(

content/browser/native_file_system/native_file_system_manager_impl.cc

@@ -106,6 +106,12 @@ bool HasTransientUserActivation(int render_process_id, int frame_id) {
   return rfh->HasTransientUserActivation();
 }
 
+bool CreateOrTruncateFile(const base::FilePath& path) {
+  int creation_flags = base::File::FLAG_CREATE_ALWAYS | base::File::FLAG_WRITE;
+  base::File file(path, creation_flags);
+  return file.IsValid();
+}
+
 }  // namespace
 
 NativeFileSystemManagerImpl::SharedHandleState::SharedHandleState(
@@ -469,17 +475,44 @@ void NativeFileSystemManagerImpl::DidVerifySensitiveDirectoryAccess(
     return;
   }
 
+  if (options.type() == blink::mojom::ChooseFileSystemEntryType::kSaveFile) {
+    DCHECK_EQ(entries.size(), 1u);
+    // Create file if it doesn't yet exist, and truncate file if it does exist.
+    base::PostTaskAndReplyWithResult(
+        FROM_HERE,
+        {base::ThreadPool(), base::TaskPriority::USER_BLOCKING,
+         base::MayBlock()},
+        base::BindOnce(&CreateOrTruncateFile, entries.front()),
+        base::BindOnce(
+            &NativeFileSystemManagerImpl::DidCreateOrTruncateSaveFile, this,
+            binding_context, entries.front(), std::move(callback)));
+    return;
+  }
+
   std::vector<blink::mojom::NativeFileSystemEntryPtr> result_entries;
   result_entries.reserve(entries.size());
-  for (const auto& entry : entries) {
-    if (options.type() == blink::mojom::ChooseFileSystemEntryType::kSaveFile) {
-      result_entries.push_back(
-          CreateWritableFileEntryFromPath(binding_context, entry));
-    } else {
+  for (const auto& entry : entries)
     result_entries.push_back(CreateFileEntryFromPath(binding_context, entry));
-    }
-  }
+  std::move(callback).Run(native_file_system_error::Ok(),
+                          std::move(result_entries));
+}
 
+void NativeFileSystemManagerImpl::DidCreateOrTruncateSaveFile(
+    const BindingContext& binding_context,
+    const base::FilePath& path,
+    ChooseEntriesCallback callback,
+    bool success) {
+  std::vector<blink::mojom::NativeFileSystemEntryPtr> result_entries;
+  if (!success) {
+    std::move(callback).Run(
+        native_file_system_error::FromStatus(
+            blink::mojom::NativeFileSystemStatus::kOperationFailed,
+            "Failed to create or truncate file"),
+        std::move(result_entries));
+    return;
+  }
+  result_entries.push_back(
+      CreateWritableFileEntryFromPath(binding_context, path));
   std::move(callback).Run(native_file_system_error::Ok(),
                           std::move(result_entries));
 }

content/browser/native_file_system/native_file_system_manager_impl.h

@@ -187,6 +187,10 @@ class CONTENT_EXPORT NativeFileSystemManagerImpl
       ChooseEntriesCallback callback,
       std::vector<base::FilePath> entries,
       NativeFileSystemPermissionContext::SensitiveDirectoryResult result);
+  void DidCreateOrTruncateSaveFile(const BindingContext& binding_context,
+                                   const base::FilePath& path,
+                                   ChooseEntriesCallback callback,
+                                   bool success);
   void DidChooseDirectory(
       const BindingContext& binding_context,
       const base::FilePath& path,