Remove symlinks from FileChooserImpl folder upload result

FileChooserImpl is the browser-side implementation of
<input type=file>. When uploading a whole folder, it
currently uses DirectoryLister to list all the files in a
directory. The result also includes resolved symbolic links
(which may even hide deep in some subfolder), which is not a
desired behavior.

Therefore, this patch removes all symbolic links from the
result by checking each file against `base::IsLink()`. Since
the function needs blocking calls to access file data, the
job is sent to a worker pool thread.

Fixed: 1345275
Change-Id: I8ab58214c87944408c64b177e915247a7485925b
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/3866767
Reviewed-by: Austin Sullivan <asully@chromium.org>
Commit-Queue: Xiaocheng Hu <xiaochengh@chromium.org>
Reviewed-by: Mason Freed <masonf@chromium.org>
Reviewed-by: Alex Moshchuk <alexmos@chromium.org>
Cr-Commit-Position: refs/heads/main@{#1045491}

content/browser/web_contents/file_chooser_impl.cc

@@ -4,8 +4,11 @@
 
 #include "content/browser/web_contents/file_chooser_impl.h"
 
+#include "base/files/file_util.h"
 #include "base/logging.h"
 #include "base/memory/ptr_util.h"
+#include "base/ranges/algorithm.h"
+#include "base/task/thread_pool.h"
 #include "content/browser/child_process_security_policy_impl.h"
 #include "content/browser/renderer_host/back_forward_cache_disable.h"
 #include "content/browser/renderer_host/render_frame_host_delegate.h"
@@ -18,6 +21,19 @@
 
 namespace content {
 
+namespace {
+
+std::vector<blink::mojom::FileChooserFileInfoPtr> RemoveSymlinks(
+    std::vector<blink::mojom::FileChooserFileInfoPtr> files) {
+  auto new_end = base::ranges::remove_if(
+      files, &base::IsLink,
+      [](const auto& file) { return file->get_native_file()->file_path; });
+  files.erase(new_end, files.end());
+  return files;
+}
+
+}  // namespace
+
 FileChooserImpl::FileSelectListenerImpl::~FileSelectListenerImpl() {
 #if DCHECK_IS_ON()
   if (!was_file_select_listener_function_called_) {
@@ -51,8 +67,20 @@ void FileChooserImpl::FileSelectListenerImpl::FileSelected(
          "FileSelectListener::FileSelectionCanceled()";
   was_file_select_listener_function_called_ = true;
 #endif
-  if (owner_)
-    owner_->FileSelected(std::move(files), base_dir, mode);
+  if (!owner_)
+    return;
+
+  if (mode != blink::mojom::FileChooserParams::Mode::kUploadFolder) {
+    owner_->FileSelected(base_dir, mode, std::move(files));
+    return;
+  }
+
+  base::ThreadPool::PostTaskAndReplyWithResult(
+      FROM_HERE,
+      {base::MayBlock(), base::TaskShutdownBehavior::CONTINUE_ON_SHUTDOWN},
+      base::BindOnce(&RemoveSymlinks, std::move(files)),
+      base::BindOnce(&FileChooserImpl::FileSelected, owner_->GetWeakPtr(),
+                     base_dir, mode));
 }
 
 void FileChooserImpl::FileSelectListenerImpl::FileSelectionCanceled() {
@@ -162,9 +190,9 @@ void FileChooserImpl::EnumerateChosenDirectory(
 }
 
 void FileChooserImpl::FileSelected(
-    std::vector<blink::mojom::FileChooserFileInfoPtr> files,
     const base::FilePath& base_dir,
-    blink::mojom::FileChooserParams::Mode mode) {
+    blink::mojom::FileChooserParams::Mode mode,
+    std::vector<blink::mojom::FileChooserFileInfoPtr> files) {
   listener_impl_ = nullptr;
   if (!render_frame_host_) {
     std::move(callback_).Run(nullptr);

content/browser/web_contents/file_chooser_impl.h

@@ -37,6 +37,8 @@ class CONTENT_EXPORT FileChooserImpl : public blink::mojom::FileChooser,
 
     // FileSelectListener overrides:
 
+    // TODO(xiaochengh): Move |file| to the end of the argument list to match
+    // the argument ordering of FileChooserImpl::FileSelected().
     void FileSelected(std::vector<blink::mojom::FileChooserFileInfoPtr> files,
                       const base::FilePath& base_dir,
                       blink::mojom::FileChooserParams::Mode mode) override;
@@ -68,9 +70,9 @@ class CONTENT_EXPORT FileChooserImpl : public blink::mojom::FileChooser,
 
   ~FileChooserImpl() override;
 
-  void FileSelected(std::vector<blink::mojom::FileChooserFileInfoPtr> files,
-                    const base::FilePath& base_dir,
-                    blink::mojom::FileChooserParams::Mode mode);
+  void FileSelected(const base::FilePath& base_dir,
+                    blink::mojom::FileChooserParams::Mode mode,
+                    std::vector<blink::mojom::FileChooserFileInfoPtr> files);
 
   void FileSelectionCanceled();
 
@@ -82,6 +84,10 @@ class CONTENT_EXPORT FileChooserImpl : public blink::mojom::FileChooser,
       const base::FilePath& directory_path,
       EnumerateChosenDirectoryCallback callback) override;
 
+  base::WeakPtr<FileChooserImpl> GetWeakPtr() {
+    return weak_factory_.GetWeakPtr();
+  }
+
  private:
   explicit FileChooserImpl(RenderFrameHostImpl* render_frame_host);
 
@@ -95,6 +101,8 @@ class CONTENT_EXPORT FileChooserImpl : public blink::mojom::FileChooser,
   raw_ptr<RenderFrameHostImpl> render_frame_host_;
   scoped_refptr<FileSelectListenerImpl> listener_impl_;
   base::OnceCallback<void(blink::mojom::FileChooserResultPtr)> callback_;
+
+  base::WeakPtrFactory<FileChooserImpl> weak_factory_{this};
 };
 
 }  // namespace content

content/browser/web_contents/file_chooser_impl_browsertest.cc

@@ -5,14 +5,18 @@
 #include "content/browser/web_contents/file_chooser_impl.h"
 
 #include "base/bind.h"
+#include "base/files/file_util.h"
+#include "base/path_service.h"
 #include "base/run_loop.h"
 #include "content/browser/renderer_host/render_frame_host_impl.h"
 #include "content/public/browser/web_contents_delegate.h"
+#include "content/public/common/content_paths.h"
 #include "content/public/test/browser_test.h"
 #include "content/public/test/browser_test_utils.h"
 #include "content/public/test/content_browser_test.h"
 #include "content/public/test/content_browser_test_utils.h"
 #include "content/shell/browser/shell.h"
+#include "content/test/content_browser_test_utils_internal.h"
 #include "url/gurl.h"
 #include "url/url_constants.h"
 
@@ -143,11 +147,52 @@ IN_PROC_BROWSER_TEST_F(FileChooserImplBrowserTest,
       ->SetListenerFunctionCalledTrueForTesting();
   std::vector<blink::mojom::FileChooserFileInfoPtr> files;
   files.emplace_back(blink::mojom::FileChooserFileInfoPtr(nullptr));
-  chooser->FileSelected(std::move(files), base::FilePath(),
-                        blink::mojom::FileChooserParams::Mode::kOpen);
+  chooser->FileSelected(base::FilePath(),
+                        blink::mojom::FileChooserParams::Mode::kOpen,
+                        std::move(files));
 
   // Test passes if this run_loop.Run() returns instead of timing out.
   run_loop.Run();
 }
 
+// https://crbug.com/1345275
+IN_PROC_BROWSER_TEST_F(FileChooserImplBrowserTest, UploadFolderWithSymlink) {
+  EXPECT_TRUE(NavigateToURL(
+      shell(), GetTestUrl(".", "file_input_webkitdirectory.html")));
+
+  // The folder contains a regular file and a symbolic link.
+  // When uploading the folder, the symbolic link should be excluded.
+  base::FilePath dir_test_data;
+  ASSERT_TRUE(base::PathService::Get(DIR_TEST_DATA, &dir_test_data));
+  base::FilePath folder_to_upload =
+      dir_test_data.AppendASCII("file_chooser").AppendASCII("dir_with_symlink");
+
+  base::FilePath text_file = folder_to_upload.AppendASCII("text_file.txt");
+  base::FilePath symlink_file = folder_to_upload.AppendASCII("symlink");
+
+  // Skip the test if symbolic links are not supported.
+  {
+    base::ScopedAllowBlockingForTesting allow_blocking;
+    if (!base::IsLink(symlink_file))
+      return;
+  }
+
+  std::unique_ptr<FileChooserDelegate> delegate(
+      new FileChooserDelegate({text_file, symlink_file}, base::OnceClosure()));
+  shell()->web_contents()->SetDelegate(delegate.get());
+  EXPECT_TRUE(ExecJs(shell(),
+                     "(async () => {"
+                     "  let listener = new Promise("
+                     "      resolve => fileinput.onchange = resolve);"
+                     "  fileinput.click();"
+                     "  await listener;"
+                     "})()"));
+
+  EXPECT_EQ(
+      1, EvalJs(shell(), "document.getElementById('fileinput').files.length;"));
+  EXPECT_EQ(
+      "text_file.txt",
+      EvalJs(shell(), "document.getElementById('fileinput').files[0].name;"));
+}
+
 }  // namespace content

content/test/content_browser_test_utils_internal.cc

@@ -445,26 +445,33 @@ Shell* OpenPopup(const ToRenderFrameHost& opener,
   return new_shell_observer.GetShell();
 }
 
+FileChooserDelegate::FileChooserDelegate(std::vector<base::FilePath> files,
+                                         base::OnceClosure callback)
+    : files_(std::move(files)), callback_(std::move(callback)) {}
+
 FileChooserDelegate::FileChooserDelegate(const base::FilePath& file,
                                          base::OnceClosure callback)
-    : file_(file), callback_(std::move(callback)) {}
+    : FileChooserDelegate(std::vector<base::FilePath>(1, file),
+                          std::move(callback)) {}
 
 FileChooserDelegate::~FileChooserDelegate() = default;
 
 void FileChooserDelegate::RunFileChooser(
     RenderFrameHost* render_frame_host,
     scoped_refptr<content::FileSelectListener> listener,
     const blink::mojom::FileChooserParams& params) {
-  // Send the selected file to the renderer process.
-  auto file_info = blink::mojom::FileChooserFileInfo::NewNativeFile(
-      blink::mojom::NativeFileInfo::New(file_, std::u16string()));
+  // Send the selected files to the renderer process.
   std::vector<blink::mojom::FileChooserFileInfoPtr> files;
-  files.push_back(std::move(file_info));
-  listener->FileSelected(std::move(files), base::FilePath(),
-                         blink::mojom::FileChooserParams::Mode::kOpen);
+  for (const auto& file : files_) {
+    auto file_info = blink::mojom::FileChooserFileInfo::NewNativeFile(
+        blink::mojom::NativeFileInfo::New(file, std::u16string()));
+    files.push_back(std::move(file_info));
+  }
+  listener->FileSelected(std::move(files), base::FilePath(), params.mode);
 
   params_ = params.Clone();
-  std::move(callback_).Run();
+  if (callback_)
+    std::move(callback_).Run();
 }
 
 FrameTestNavigationManager::FrameTestNavigationManager(

content/test/content_browser_test_utils_internal.h

@@ -176,9 +176,11 @@ Shell* OpenPopup(const ToRenderFrameHost& opener,
 class FileChooserDelegate : public WebContentsDelegate {
  public:
   // Constructs a WebContentsDelegate that mocks a file dialog.
-  // The mocked file dialog will always reply that the user selected |file|.
-  // |callback| is invoked when RunFileChooser() is called.
+  // The mocked file dialog will always reply that the user selected |file| or
+  // |files|. |callback| is invoked when RunFileChooser() is called.
   FileChooserDelegate(const base::FilePath& file, base::OnceClosure callback);
+  FileChooserDelegate(std::vector<base::FilePath> files,
+                      base::OnceClosure callback);
   ~FileChooserDelegate() override;
 
   // Implementation of WebContentsDelegate::RunFileChooser.
@@ -190,7 +192,7 @@ class FileChooserDelegate : public WebContentsDelegate {
   const blink::mojom::FileChooserParams& params() const { return *params_; }
 
  private:
-  base::FilePath file_;
+  std::vector<base::FilePath> files_;
   base::OnceClosure callback_;
   blink::mojom::FileChooserParamsPtr params_;
 };

content/test/data/file_chooser/dir_with_symlink/symlink

@@ -0,0 +1 @@
+../linked_text_file.txt

content/test/data/file_chooser/dir_with_symlink/text_file.txt

@@ -0,0 +1 @@
+text

content/test/data/file_chooser/linked_text_file.txt

@@ -0,0 +1 @@
+linked text file

content/test/data/file_input_webkitdirectory.html

@@ -0,0 +1 @@
+<input type="file" id="fileinput" webkitdirectory />