-
Notifications
You must be signed in to change notification settings - Fork 15.3k
Commit
This commit does not belong to any branch on this repository, and may belong to a fork outside of the repository.
chore: cherry-pick 933cc81c6bad from chromium
- Loading branch information
Showing
2 changed files
with
325 additions
and
0 deletions.
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,324 @@ | ||
From 933cc81c6bad0bb1aaf1b07b7255500efc58de6e Mon Sep 17 00:00:00 2001 | ||
From: Xiaocheng Hu <xiaochengh@chromium.org> | ||
Date: Sat, 10 Sep 2022 05:53:49 +0000 | ||
Subject: [PATCH] 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} | ||
--- | ||
|
||
diff --git a/content/browser/web_contents/file_chooser_impl.cc b/content/browser/web_contents/file_chooser_impl.cc | ||
index 7a3ea45d..1aa19f7a 100644 | ||
--- a/content/browser/web_contents/file_chooser_impl.cc | ||
+++ b/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 @@ | ||
"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::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); | ||
diff --git a/content/browser/web_contents/file_chooser_impl.h b/content/browser/web_contents/file_chooser_impl.h | ||
index b9f11f9..b628b29 100644 | ||
--- a/content/browser/web_contents/file_chooser_impl.h | ||
+++ b/content/browser/web_contents/file_chooser_impl.h | ||
@@ -37,6 +37,8 @@ | ||
|
||
// 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 @@ | ||
|
||
~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 @@ | ||
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 @@ | ||
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 | ||
diff --git a/content/browser/web_contents/file_chooser_impl_browsertest.cc b/content/browser/web_contents/file_chooser_impl_browsertest.cc | ||
index 5aa120c..2acd221 100644 | ||
--- a/content/browser/web_contents/file_chooser_impl_browsertest.cc | ||
+++ b/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 @@ | ||
->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 | ||
diff --git a/content/test/content_browser_test_utils_internal.cc b/content/test/content_browser_test_utils_internal.cc | ||
index 6db08b6..8e1dfaf 100644 | ||
--- a/content/test/content_browser_test_utils_internal.cc | ||
+++ b/content/test/content_browser_test_utils_internal.cc | ||
@@ -445,9 +445,14 @@ | ||
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; | ||
|
||
@@ -455,16 +460,18 @@ | ||
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( | ||
diff --git a/content/test/content_browser_test_utils_internal.h b/content/test/content_browser_test_utils_internal.h | ||
index 6ae5e8f..3bf1bb93 100644 | ||
--- a/content/test/content_browser_test_utils_internal.h | ||
+++ b/content/test/content_browser_test_utils_internal.h | ||
@@ -176,9 +176,11 @@ | ||
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 @@ | ||
const blink::mojom::FileChooserParams& params() const { return *params_; } | ||
|
||
private: | ||
- base::FilePath file_; | ||
+ std::vector<base::FilePath> files_; | ||
base::OnceClosure callback_; | ||
blink::mojom::FileChooserParamsPtr params_; | ||
}; | ||
diff --git a/content/test/data/file_chooser/dir_with_symlink/symlink b/content/test/data/file_chooser/dir_with_symlink/symlink | ||
new file mode 120000 | ||
index 0000000..7857c689 | ||
--- /dev/null | ||
+++ b/content/test/data/file_chooser/dir_with_symlink/symlink | ||
@@ -0,0 +1 @@ | ||
+../linked_text_file.txt | ||
\ No newline at end of file | ||
diff --git a/content/test/data/file_chooser/dir_with_symlink/text_file.txt b/content/test/data/file_chooser/dir_with_symlink/text_file.txt | ||
new file mode 100644 | ||
index 0000000..8e27be7d | ||
--- /dev/null | ||
+++ b/content/test/data/file_chooser/dir_with_symlink/text_file.txt | ||
@@ -0,0 +1 @@ | ||
+text | ||
diff --git a/content/test/data/file_chooser/linked_text_file.txt b/content/test/data/file_chooser/linked_text_file.txt | ||
new file mode 100644 | ||
index 0000000..9a1f4bc | ||
--- /dev/null | ||
+++ b/content/test/data/file_chooser/linked_text_file.txt | ||
@@ -0,0 +1 @@ | ||
+linked text file | ||
diff --git a/content/test/data/file_input_webkitdirectory.html b/content/test/data/file_input_webkitdirectory.html | ||
new file mode 100644 | ||
index 0000000..5b7bb50 | ||
--- /dev/null | ||
+++ b/content/test/data/file_input_webkitdirectory.html | ||
@@ -0,0 +1 @@ | ||
+<input type="file" id="fileinput" webkitdirectory /> |