Add a unit test that filenames aren't unintentionally converted to URLs.

ui/base/clipboard/clipboard_util_win.cc

 // Copyright (c) 2012 The Chromium Authors. All rights reserved.
 // Use of this source code is governed by a BSD-style license that can be
 // found in the LICENSE file.
 
 #include "ui/base/clipboard/clipboard_util_win.h"
 
 #include <shellapi.h>
 #include <shlwapi.h>
 #include <wininet.h>  // For INTERNET_MAX_URL_LENGTH.
 
 #include "base/basictypes.h"
 #include "base/logging.h"
 #include "base/strings/string_util.h"
 #include "base/strings/stringprintf.h"
+#include "base/strings/sys_string_conversions.h"
 #include "base/strings/utf_string_conversions.h"
 #include "base/win/scoped_hglobal.h"
+#include "net/base/filename_util.h"
 #include "ui/base/clipboard/clipboard.h"
 #include "ui/base/clipboard/custom_data_helper.h"
+#include "url/gurl.h"
 
 namespace ui {
 
 namespace {
 
 bool HasData(IDataObject* data_object, const Clipboard::FormatType& format) {
   FORMATETC format_etc = format.ToFormatEtc();
   return SUCCEEDED(data_object->QueryGetData(&format_etc));
 }
 
 bool GetData(IDataObject* data_object,
              const Clipboard::FormatType& format,
              STGMEDIUM* medium) {
   FORMATETC format_etc = format.ToFormatEtc();
   return SUCCEEDED(data_object->GetData(&format_etc, medium));
 }
 
 bool GetUrlFromHDrop(IDataObject* data_object,
-                     base::string16* url,
+                     GURL* url,
                      base::string16* title) {
   DCHECK(data_object && url && title);
 
   STGMEDIUM medium;
   if (!GetData(data_object, Clipboard::GetCFHDropFormatType(), &medium))
     return false;
 
   HDROP hdrop = static_cast<HDROP>(GlobalLock(medium.hGlobal));
 
   if (!hdrop)
     return false;
 
   bool success = false;
   wchar_t filename[MAX_PATH];
   if (DragQueryFileW(hdrop, 0, filename, arraysize(filename))) {
     wchar_t url_buffer[INTERNET_MAX_URL_LENGTH];
     if (0 == _wcsicmp(PathFindExtensionW(filename), L".url") &&
         GetPrivateProfileStringW(L"InternetShortcut", L"url", 0, url_buffer,
                                  arraysize(url_buffer), filename)) {
-      url->assign(url_buffer);
+      *url = GURL(url_buffer);
       PathRemoveExtension(filename);
       title->assign(PathFindFileName(filename));
-      success = true;
+      success = url->is_valid();
     }
   }
 
   DragFinish(hdrop);
   GlobalUnlock(medium.hGlobal);
   // We don't need to call ReleaseStgMedium here because as far as I can tell,
   // DragFinish frees the hGlobal for us.
   return success;
 }
 
 void SplitUrlAndTitle(const base::string16& str,
-                      base::string16* url,
+                      GURL* url,
                       base::string16* title) {
   DCHECK(url && title);
   size_t newline_pos = str.find('\n');
   if (newline_pos != base::string16::npos) {
-    url->assign(str, 0, newline_pos);
+    *url = GURL(base::string16(str, 0, newline_pos));
     title->assign(str, newline_pos + 1, base::string16::npos);
   } else {
-    url->assign(str);
+    *url = GURL(str);
     title->assign(str);
   }
 }
 
-bool GetFileUrl(IDataObject* data_object, base::string16* url,
-                base::string16* title) {
-  STGMEDIUM store;
-  if (GetData(data_object, Clipboard::GetFilenameWFormatType(), &store)) {
-    bool success = false;
-    {
-      // filename using unicode
-      base::win::ScopedHGlobal<wchar_t> data(store.hGlobal);
-      if (data.get() && data.get()[0] &&
-          (PathFileExists(data.get()) || PathIsUNC(data.get()))) {
-        wchar_t file_url[INTERNET_MAX_URL_LENGTH];
-        DWORD file_url_len = arraysize(file_url);
-        if (SUCCEEDED(::UrlCreateFromPathW(data.get(), file_url, &file_url_len,
-                                           0))) {
-          url->assign(file_url);
-          title->assign(file_url);
-          success = true;
-        }
-      }
-    }
-    ReleaseStgMedium(&store);
-    if (success)
-      return true;
-  }
-
-  if (GetData(data_object, Clipboard::GetFilenameFormatType(), &store)) {
-    bool success = false;
-    {
-      // filename using ascii
-      base::win::ScopedHGlobal<char> data(store.hGlobal);
-      if (data.get() && data.get()[0] && (PathFileExistsA(data.get()) ||
-                                          PathIsUNCA(data.get()))) {
-        char file_url[INTERNET_MAX_URL_LENGTH];
-        DWORD file_url_len = arraysize(file_url);
-        if (SUCCEEDED(::UrlCreateFromPathA(data.get(), file_url, &file_url_len,
-                                           0))) {
-          url->assign(base::UTF8ToWide(file_url));
-          title->assign(*url);
-          success = true;
-        }
-      }
-    }
-    ReleaseStgMedium(&store);
-    if (success)
-      return true;
-  }
-  return false;
-}
-
 }  // namespace
 
 bool ClipboardUtil::HasUrl(IDataObject* data_object, bool convert_filenames) {
   DCHECK(data_object);
   return HasData(data_object, Clipboard::GetMozUrlFormatType()) ||
          HasData(data_object, Clipboard::GetUrlWFormatType()) ||
          HasData(data_object, Clipboard::GetUrlFormatType()) ||
-         (convert_filenames && (
-             HasData(data_object, Clipboard::GetFilenameWFormatType()) ||
-             HasData(data_object, Clipboard::GetFilenameFormatType())));
+         (convert_filenames && HasFilenames(data_object));
 }
 
 bool ClipboardUtil::HasFilenames(IDataObject* data_object) {
   DCHECK(data_object);
-  return HasData(data_object, Clipboard::GetCFHDropFormatType());
+  return HasData(data_object, Clipboard::GetCFHDropFormatType()) ||
+         HasData(data_object, Clipboard::GetFilenameWFormatType()) ||
+         HasData(data_object, Clipboard::GetFilenameFormatType());
 }
 
 bool ClipboardUtil::HasFileContents(IDataObject* data_object) {
   DCHECK(data_object);
   return HasData(data_object, Clipboard::GetFileContentZeroFormatType());
 }
 
 bool ClipboardUtil::HasHtml(IDataObject* data_object) {
   DCHECK(data_object);
   return HasData(data_object, Clipboard::GetHtmlFormatType()) ||
          HasData(data_object, Clipboard::GetTextHtmlFormatType());
 }
 
 bool ClipboardUtil::HasPlainText(IDataObject* data_object) {
   DCHECK(data_object);
   return HasData(data_object, Clipboard::GetPlainTextWFormatType()) ||
          HasData(data_object, Clipboard::GetPlainTextFormatType());
 }
 
 bool ClipboardUtil::GetUrl(IDataObject* data_object,
-    base::string16* url, base::string16* title, bool convert_filenames) {
+                           GURL* url,
+                           base::string16* title,
+                           bool convert_filenames) {
   DCHECK(data_object && url && title);
   if (!HasUrl(data_object, convert_filenames))
     return false;
 
   // Try to extract a URL from |data_object| in a variety of formats.
   STGMEDIUM store;
   if (GetUrlFromHDrop(data_object, url, title))
     return true;
 
   if (GetData(data_object, Clipboard::GetMozUrlFormatType(), &store) ||
       GetData(data_object, Clipboard::GetUrlWFormatType(), &store)) {
     {
       // Mozilla URL format or unicode URL
       base::win::ScopedHGlobal<wchar_t> data(store.hGlobal);
       SplitUrlAndTitle(data.get(), url, title);
     }
     ReleaseStgMedium(&store);
-    return true;
+    return url->is_valid();

[sky, 2014/07/10 22:44:33]

Under what circumstance would url not be valid here and in other places? This
seems like it should be a DCHECK with a return true.

[dcheng, 2014/07/11 22:32:58]

Other apps may have written a malformed URL to the clipboard.

   }
 
   if (GetData(data_object, Clipboard::GetUrlFormatType(), &store)) {
     {
       // URL using ascii
       base::win::ScopedHGlobal<char> data(store.hGlobal);
       SplitUrlAndTitle(base::UTF8ToWide(data.get()), url, title);
     }
     ReleaseStgMedium(&store);
-    return true;
+    return url->is_valid();
   }
 
   if (convert_filenames) {
-    return GetFileUrl(data_object, url, title);
-  } else {
-    return false;
+    std::vector<base::string16> filenames;
+    if (!GetFilenames(data_object, &filenames))
+      return false;
+    DCHECK_GT(filenames.size(), 0U);
+    *url = net::FilePathToFileURL(base::FilePath(filenames[0]));
+    return url->is_valid();
   }
+
+  return false;
 }
 
 bool ClipboardUtil::GetFilenames(IDataObject* data_object,
                                  std::vector<base::string16>* filenames) {
   DCHECK(data_object && filenames);
   if (!HasFilenames(data_object))
     return false;
 
   STGMEDIUM medium;
-  if (!GetData(data_object, Clipboard::GetCFHDropFormatType(), &medium))
-    return false;
+  if (GetData(data_object, Clipboard::GetCFHDropFormatType(), &medium)) {
+    HDROP hdrop = static_cast<HDROP>(GlobalLock(medium.hGlobal));
+    if (!hdrop)
+      return false;
 
-  HDROP hdrop = static_cast<HDROP>(GlobalLock(medium.hGlobal));
-  if (!hdrop)
-    return false;
+    const int kMaxFilenameLen = 4096;
+    const unsigned num_files = DragQueryFileW(hdrop, 0xffffffff, 0, 0);
+    for (unsigned int i = 0; i < num_files; ++i) {
+      wchar_t filename[kMaxFilenameLen];
+      if (!DragQueryFileW(hdrop, i, filename, kMaxFilenameLen))
+        continue;
+      filenames->push_back(filename);
+    }
 
-  const int kMaxFilenameLen = 4096;
-  const unsigned num_files = DragQueryFileW(hdrop, 0xffffffff, 0, 0);
-  for (unsigned int i = 0; i < num_files; ++i) {
-    wchar_t filename[kMaxFilenameLen];
-    if (!DragQueryFileW(hdrop, i, filename, kMaxFilenameLen))
-      continue;
-    filenames->push_back(filename);
+    DragFinish(hdrop);
+    GlobalUnlock(medium.hGlobal);

[sky, 2014/07/10 22:44:34]

I don't like having to worry if GlobalUnlock is balanced. How about a scoper?

[dcheng, 2014/07/11 22:32:58]

So it turns out ScopedHGlobal is poorly suited for this task. Internally, it
adds a pointer qualifier to the template argument, so it ends up thinking that
its referring to a HDROP*--but HDROP itself is actually already an opaque
handle/pointer to the actual data. I hacked around this for now by
reinterpret_casting the result of get() back to a HDROP but I'm trying to think
of a better solution...

[dcheng, 2014/07/11 22:45:30]

Actually I just forced everyone who wants a T* to explicitly say so in the
template argument. PTAL. I also found out something somewhat concerning: some of
the unit tests don't seem to be actually getting built at all:
https://code.google.com/p/chromium/issues/detail?id=393364

+    // We don't need to call ReleaseStgMedium here because as far as I can tell,
+    // DragFinish frees the hGlobal for us.
+    return true;
   }
 
-  DragFinish(hdrop);
-  GlobalUnlock(medium.hGlobal);
-  // We don't need to call ReleaseStgMedium here because as far as I can tell,
-  // DragFinish frees the hGlobal for us.
-  return true;
+  if (GetData(data_object, Clipboard::GetFilenameWFormatType(), &medium)) {
+    {
+      // filename using unicode
+      base::win::ScopedHGlobal<wchar_t> data(medium.hGlobal);
+      if (data.get() && data.get()[0])
+        filenames->push_back(data.get());
+    }
+    ReleaseStgMedium(&medium);
+    return true;
+  }
+
+  if (GetData(data_object, Clipboard::GetFilenameFormatType(), &medium)) {
+    {
+      // filename using ascii
+      base::win::ScopedHGlobal<char> data(medium.hGlobal);
+      if (data.get() && data.get()[0])
+        filenames->push_back(base::SysNativeMBToWide(data.get()));
+    }
+    ReleaseStgMedium(&medium);
+    return true;
+  }
+
+  return false;
 }
 
 bool ClipboardUtil::GetPlainText(IDataObject* data_object,
                                  base::string16* plain_text) {
   DCHECK(data_object && plain_text);
   if (!HasPlainText(data_object))
     return false;
 
   STGMEDIUM store;
   if (GetData(data_object, Clipboard::GetPlainTextWFormatType(), &store)) {
@@ skipping 11 matching lines @@
       // ascii text
       base::win::ScopedHGlobal<char> data(store.hGlobal);
       plain_text->assign(base::UTF8ToWide(data.get()));
     }
     ReleaseStgMedium(&store);
     return true;
   }
 
   // If a file is dropped on the window, it does not provide either of the
   // plain text formats, so here we try to forcibly get a url.
+  GURL url;
   base::string16 title;
-  return GetUrl(data_object, plain_text, &title, false);
+  if (GetUrl(data_object, &url, &title, false)) {
+    *plain_text = base::UTF8ToUTF16(url.spec());
+    return true;
+  }
+  return false;
 }
 
 bool ClipboardUtil::GetHtml(IDataObject* data_object,
                             base::string16* html, std::string* base_url) {
   DCHECK(data_object && html && base_url);
 
   STGMEDIUM store;
   if (HasData(data_object, Clipboard::GetHtmlFormatType()) &&
       GetData(data_object, Clipboard::GetHtmlFormatType(), &store)) {
     {
@@ skipping 225 matching lines @@
           end_fragment_start + end_fragment_str.length()));
     }
   } else {
     *fragment_start = cf_html.find('>', tag_start) + 1;
     size_t tag_end = cf_html.rfind("<!--EndFragment", std::string::npos);
     *fragment_end = cf_html.rfind('<', tag_end);
   }
 }
 
 }  // namespace ui