Drag and Drop: Don't extract virtual files if url data present

Fix for:
Issue 963392: Regression:'Drop and Install' Message is seen when a
bookmarked item is dragged in chrome://extension page.

On initiating a bookmark drag, the UniformResourceLocatorW clipboard
format is added to the data object, as well as a "virtual" .url file
using CFSTR_FILEDESCRIPTORW/CFSTR_FILECONTENTS formats, which represents
an internet shortcut intended to be dropped on the desktop. Before my
introduction of virtual file support:

https://chromium.googlesource.com/chromium/src/+/e524176b0fb387f1c3e509364cde09af045b8a91

a Chromium drop target ignored the virtual file content, but now it is
recognized on drag enter and drop. The chrome:://extensions page "sees"
the presence of file data and assumes the intent is to install an
extension and thus displays the Drop to Install feedback--you see the
same thing without my fix when dragging an arbitrary file in from the
desktop into the extensions page.

The least risky fix is to ignore virtual files if there is also url data
present. Note you can't just check if the "chromium/x-bookmark-entries"
custom format is present, since this bug also repros dragging the favicon
or site info icon out of the address bar, and in that scenario
"chromium/x-bookmark-entries" is not added. In my experimentation
dragging items out of Outlook.exe, no url data is added to the data
object, so this fix won't break the primary scenario addressed by my
original change.

Bug: 963392
Change-Id: I902cecedca49e1fc6f39cbf9a62de8895dfd0d10
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1615962
Commit-Queue: Bruce Long <brlong@microsoft.com>
Reviewed-by: Darwin Huang <huangdarwin@chromium.org>
Reviewed-by: Scott Violet <sky@chromium.org>
Cr-Commit-Position: refs/heads/master@{#661006}

content/browser/web_contents/web_contents_view_aura.cc

@@ -280,6 +280,38 @@ void PrepareDragData(const DropData& drop_data,
   }
 }
 
+#if defined(OS_WIN)
+// Function returning whether this drop target should extract virtual file data
+// from the data store.
+// (1) As with real files, only add virtual files if the drag did not originate
+// in the renderer process. Without this, if an anchor element is dragged and
+// then dropped on the same page, the browser will navigate to the URL
+// referenced by the anchor. That is because virtual ".url" file data
+// (internet shortcut) is added to the data object on drag start, and if
+// script doesn't handle the drop, the browser behaves just as if a .url file
+// were dragged in from the desktop. Filtering out virtual files if the drag
+// is renderer tainted also prevents the possibility of a compromised renderer
+// gaining access to the backing temp file paths.
+// (2) Even if the drag is not renderer tainted, also exclude virtual files
+// if the UniformResourceLocatorW clipboard format is found in the data object.
+// Drags initiated in the browser process, such as dragging a bookmark from
+// the bookmark bar, will add a virtual .url file to the data object using the
+// CFSTR_FILEDESCRIPTORW/CFSTR_FILECONTENTS formats, which represents an
+// internet shortcut intended to be  dropped on the desktop. But this causes a
+// regression in the behavior of the extensions page (see
+// https://crbug.com/963392). The primary scenario for introducing virtual file
+// support was for dragging items out of Outlook.exe for upload to a file
+// hosting service. The Outlook drag source does not add url data to the data
+// object.
+// TODO(https://crbug.com/958273): DragDrop: Extend virtual filename support
+// to DropData, for parity with real filename support.
+// TODO(https://crbug.com/964461): Drag and drop: Should support both virtual
+// file and url data on drop.
+bool ShouldIncludeVirtualFiles(const DropData& drop_data) {
+  return !drop_data.did_originate_from_renderer && drop_data.url.is_empty();
+}
+#endif
+
 // Utility to fill a DropData object from ui::OSExchangeData.
 void PrepareDropData(DropData* drop_data, const ui::OSExchangeData& data) {
   drop_data->did_originate_from_renderer = data.DidOriginateFromRenderer();
@@ -312,7 +344,8 @@ void PrepareDropData(DropData* drop_data, const ui::OSExchangeData& data) {
   // Get a list of virtual files for later retrieval when a drop is performed
   // (will return empty vector if there are any non-virtual files in the data
   // store).
-  data.GetVirtualFilenames(&drop_data->filenames);
+  if (ShouldIncludeVirtualFiles(*drop_data))
+    data.GetVirtualFilenames(&drop_data->filenames);
 #endif
 
   base::Pickle pickle;
@@ -1331,18 +1364,7 @@ int WebContentsViewAura::OnPerformDrop(const ui::DropTargetEvent& event) {
 
   const int key_modifiers = ui::EventFlagsToWebEventModifiers(event.flags());
 #if defined(OS_WIN)
-  // As with real files, only add virtual files if the drag did not originate in
-  // the renderer process. Without this, if an anchor element is dragged and
-  // then dropped on the same page, the browser will navigate to the URL
-  // referenced by the anchor. That is because virtual ".url" file data
-  // (internet shortcut) is added to the data object on drag start, and if
-  // script doesn't handle the drop, the browser behaves just as if a .url file
-  // were dragged in from the desktop. Filtering out virtual files if the drag
-  // is renderer tainted also prevents the possibility of a compromised renderer
-  // gaining access to the backing temp file paths.
-  // TODO(https://crbug.com/958273): DragDrop: Extend virtual filename support
-  // to DropData, for parity with real filename support.
-  if (!current_drop_data_->did_originate_from_renderer &&
+  if (ShouldIncludeVirtualFiles(*current_drop_data_) &&
       event.data().HasVirtualFilenames()) {
     // Asynchronously retrieve the actual content of any virtual files now (this
     // step is not needed for "real" files already on the file system, e.g.

content/browser/web_contents/web_contents_view_aura.h

@@ -72,6 +72,7 @@ class CONTENT_EXPORT WebContentsViewAura
   FRIEND_TEST_ALL_PREFIXES(WebContentsViewAuraTest, DragDropVirtualFiles);
   FRIEND_TEST_ALL_PREFIXES(WebContentsViewAuraTest,
                            DragDropVirtualFilesOriginateFromRenderer);
+  FRIEND_TEST_ALL_PREFIXES(WebContentsViewAuraTest, DragDropUrlData);
 
   class WindowObserver;
 

content/browser/web_contents/web_contents_view_aura_unittest.cc

@@ -447,6 +447,57 @@ TEST_F(WebContentsViewAuraTest, DragDropVirtualFilesOriginateFromRenderer) {
 
   ASSERT_TRUE(drop_complete_data_->drop_data.filenames.empty());
 }
+
+TEST_F(WebContentsViewAuraTest, DragDropUrlData) {
+  WebContentsViewAura* view = GetView();
+  ui::OSExchangeData data;
+
+  const std::string url_spec = "https://www.wikipedia.org/";
+  const GURL url(url_spec);
+  const base::string16 url_title = base::ASCIIToUTF16("Wikipedia");
+  data.SetURL(url, url_title);
+
+  // SetUrl should also add a virtual .url (internet shortcut) file.
+  std::vector<ui::FileInfo> file_infos;
+  EXPECT_TRUE(data.GetVirtualFilenames(&file_infos));
+  ASSERT_EQ(1ULL, file_infos.size());
+  EXPECT_EQ(base::FilePath(url_title + base::ASCIIToUTF16(".url")),
+            file_infos[0].display_name);
+
+  ui::DropTargetEvent event(data, kClientPt, kScreenPt,
+                            ui::DragDropTypes::DRAG_COPY);
+
+  // Simulate drag enter.
+  EXPECT_EQ(nullptr, view->current_drop_data_);
+  view->OnDragEntered(event);
+  ASSERT_NE(nullptr, view->current_drop_data_);
+
+  EXPECT_EQ(url_spec, view->current_drop_data_->url);
+  EXPECT_EQ(url_title, view->current_drop_data_->url_title);
+
+  // Virtual files should not have been retrieved if url data present.
+  ASSERT_TRUE(view->current_drop_data_->filenames.empty());
+
+  // Simulate drop (completes asynchronously since virtual file data is
+  // present).
+  auto callback = base::BindOnce(&WebContentsViewAuraTest::OnDropComplete,
+                                 base::Unretained(this));
+  view->RegisterDropCallbackForTesting(std::move(callback));
+
+  base::RunLoop run_loop;
+  async_drop_closure_ = run_loop.QuitClosure();
+
+  view->OnPerformDrop(event);
+  run_loop.Run();
+
+  CheckDropData(view);
+
+  EXPECT_EQ(url_spec, drop_complete_data_->drop_data.url);
+  EXPECT_EQ(url_title, drop_complete_data_->drop_data.url_title);
+
+  // Virtual files should not have been retrieved if url data present.
+  ASSERT_TRUE(drop_complete_data_->drop_data.filenames.empty());
+}
 #endif
 
 }  // namespace content