Bug 25882

Summary: [chromium] User paths exposed on file drop
Product: WebKit Reporter: Eric Seidel (no email) <eric>
Component: New BugsAssignee: Nobody <webkit-unassigned>
Severity: Normal CC: abarth, commit-queue, dcheng, ian, jianli, mjs, noel.gordon, rolandsteiner, sam, tony
Priority: P2 Keywords: InRadar
Version: 528+ (Nightly build)   
Hardware: Macintosh   
OS: OS X 10.5   
Description Flags
test case
Don't add file paths to text/uri-list when dragging.
dcheng: review-, dcheng: commit-queue-
Patch with layout tests
jianli: review-
Addressing review comments.
Chromium patch
jianli: review-
ChromiumClipboard patch
Filter file:// URLs from event.dataTransfer.getData dcheng: review-, dcheng: commit-queue-

Description Eric Seidel (no email) 2009-05-19 20:28:14 PDT
User paths exposed on file drop

Sam seemed concerned on #webkit that this was a user privacy violation.  I'm not sure that it is (what do paths really matter?), but we certainly seem to take care not to expose them in other places (like the File object or on input.value).
Comment 1 Eric Seidel (no email) 2009-05-19 20:28:45 PDT
Created attachment 30491 [details]
test case
Comment 2 Adam Barth 2009-05-19 21:24:23 PDT
It's a minor privacy issue.  We should fix it.  We should fix input.value also (as speced in HTML 5).
Comment 3 Eric Seidel (no email) 2009-05-20 21:18:50 PDT
Maciej suggests we fix this by changing the clipboard to only expose filenames for local files.  This means that drag and drop onto a text area inserts just the name.  Also that getData("URL") would return just the name.
Comment 4 Eric Seidel (no email) 2009-06-02 13:11:08 PDT
This is also tracked by http://b/1862075 in Google's tracker.
Comment 5 Roland Steiner 2010-03-23 21:19:15 PDT
There are 2 sides to this:

The current implementation of ChromiumDataObject creates file:// URLS from file names contained in the clipboard data. As discussed between Noel Gordon and Adam Barth, this should be removed (which is trivial).

However, this is not the code path that sets the clipboard URL/uri-list. Rather, (on Chromium Windows) the file-name is converted by ClipboardUtil::GetUrl() in chrome/src/app/clipboard/clipboard_util_win.cc (lines 216ff).

So I'd guess we'd also need to remove those code parts in a 2nd Chromium-side patch, but I first wanted to confirm whether that's really what we want to do.
Comment 6 Roland Steiner 2010-04-22 20:49:00 PDT
*** Bug 38019 has been marked as a duplicate of this bug. ***
Comment 7 Daniel Cheng 2010-04-22 22:16:22 PDT
Created attachment 54131 [details]
Don't add file paths to text/uri-list when dragging.
Comment 8 Tony Chang 2010-04-22 23:57:36 PDT
(In reply to comment #7)
> Created an attachment (id=54131) [details]
> Don't add file paths to text/uri-list when dragging.

Is it possible to add a layout test for this?  There's eventSender.beginDragWithFiles which you might be able to use.
Comment 9 Daniel Cheng 2010-04-23 00:08:29 PDT
Comment on attachment 54131 [details]
Don't add file paths to text/uri-list when dragging.

Good point, I'll update the patch with a layout test.
Comment 10 David Kilzer (:ddkilzer) 2010-04-23 00:58:51 PDT
Comment 11 Roland Steiner 2010-04-23 01:08:23 PDT
(In reply to comment #9)

Minor style nit:

@@ -327,7 +314,6 @@ HashSet<String> ClipboardChromium::types() const
         return results;
     if (!m_dataObject->filenames.isEmpty()) {
-        results.add("text/uri-list");

You can also remove the brackets here since there's only 1 line left in the 'if'.
Comment 12 Daniel Cheng 2010-04-23 10:54:28 PDT
Created attachment 54175 [details]
Patch with layout tests

I'm ashamed of the way I "fixed" ClipboardMac. If anyone has suggestions on a better way to do it, I'm all ears.
Comment 13 Jian Li 2010-04-23 13:37:10 PDT
Comment on attachment 54175 [details]
Patch with layout tests

 +      bool m_filenamesSetBySelf;
Might be better to name it as m_isFilenamesSetBySelf.

 +  static void addHTMLClipboardTypesForCocoaType(HashSet<String>& resultTypes, const NSString *cocoaType, bool filenamesSetBySelf, NSPasteboard *pasteboard)
Since now we add only one type to resultTypes, how about changing the function to return a type string, instead of passing resultTypes? The caller could then add the type to the set.

 +      if (cocoaType == NSFilenamesPboardType)
You should say:
   if ( [cocoaType isEqualToString:NSFilenamesPboardType])
Comment 14 Daniel Cheng 2010-04-23 19:01:06 PDT
Created attachment 54209 [details]
Addressing review comments.

I think this should address your review comments.
Comment 15 Adam Barth 2010-04-23 20:09:47 PDT
Comment on attachment 54209 [details]
Addressing review comments.

 +              // Lame hack. Dragging files into the UA shouldn't expose file URIs, so we only add it
Why is this needed by Mac but not by chromium?

Who should we get to review this change?  I don't know enough here to be very useful.
Comment 16 Daniel Cheng 2010-04-23 23:14:15 PDT
> Why is this needed by Mac but not by chromium?

The logic is handled at a different layer in the Chromium port. See http://src.chromium.org/cgi-bin/gitweb.cgi?p=chromium.git;a=blob;f=chrome/browser/cocoa/web_drag_source.mm;h=70c144c452750df2715671a0280c89918df2af9d;hb=HEAD

I think Jian and/or Roland will be reviewing this change.
Comment 17 Daniel Cheng 2010-04-26 15:17:51 PDT
Created attachment 54340 [details]
Chromium patch

Per jianli's suggestion, splitting up the changes and moving the WebCore/platform/mac and layout test changes to a separate patch.
Comment 18 Jian Li 2010-04-26 15:21:31 PDT
Comment on attachment 54340 [details]
Chromium patch

 +          Don't make file paths available in text/uri-list when dragging files.
Could you please add "[chromium]" to the bug title and update the description here?
Comment 19 Jian Li 2010-04-26 15:23:54 PDT
A new bug will be filed by dcheng to cover the similar issue on Mac platform.
Comment 20 Daniel Cheng 2010-04-26 15:25:19 PDT
Created attachment 54341 [details]
ChromiumClipboard patch

Added Chromium to ChangeLog entry.
Comment 21 Jian Li 2010-04-26 15:27:25 PDT
Comment on attachment 54341 [details]
ChromiumClipboard patch

Comment 22 WebKit Commit Bot 2010-04-26 18:32:55 PDT
Comment on attachment 54341 [details]
ChromiumClipboard patch

Clearing flags on attachment: 54341

Committed r58276: <http://trac.webkit.org/changeset/58276>
Comment 23 WebKit Commit Bot 2010-04-26 18:33:02 PDT
All reviewed patches have been landed.  Closing bug.
Comment 24 Daniel Cheng 2010-04-27 16:03:11 PDT
Created attachment 54465 [details]
Filter file:// URLs from event.dataTransfer.getData
Comment 25 Daniel Cheng 2010-05-10 17:27:09 PDT
Discussion for fixing other ports (and addressing shortcomings in this patch) (might?) continue in https://bugs.webkit.org/show_bug.cgi?id=38876.