Bug 25882 – [chromium] User paths exposed on file drop

• Component: New Bugs
• Status: RESOLVED FIXED

[Eric Seidel (no email)]

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).

[Eric Seidel (no email)]

Created attachment 30491 [details]
test case

[Adam Barth]

It's a minor privacy issue.  We should fix it.  We should fix input.value also (as speced in HTML 5).

[Eric Seidel (no email)]

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.

[Eric Seidel (no email)]

This is also tracked by http://b/1862075 in Google's tracker.

[Roland Steiner]

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.

[Roland Steiner]

*** Bug 38019 has been marked as a duplicate of this bug. ***

[Daniel Cheng]

Created attachment 54131 [details]
Don't add file paths to text/uri-list when dragging.

[Tony Chang]

(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.

[Daniel Cheng]

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.

[David Kilzer (:ddkilzer)]

<rdar://problem/7898443>

[Roland Steiner]

(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");
         results.add("Files");
     }

You can also remove the brackets here since there's only 1 line left in the 'if'.

[Daniel Cheng]

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.

[Jian Li]

Comment on attachment 54175 [details]
Patch with layout tests

WebCore/platform/mac/ClipboardMac.h:89
 +      bool m_filenamesSetBySelf;
Might be better to name it as m_isFilenamesSetBySelf.

WebCore/platform/mac/ClipboardMac.mm:108
 +  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.

WebCore/platform/mac/ClipboardMac.mm:152
 +      if (cocoaType == NSFilenamesPboardType)
You should say:
   if ( [cocoaType isEqualToString:NSFilenamesPboardType])

[Daniel Cheng]

Created attachment 54209 [details]
Addressing review comments.

I think this should address your review comments.

[Adam Barth]

Comment on attachment 54209 [details]
Addressing review comments.

WebCore/platform/mac/ClipboardMac.mm:123
 +              // 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.

[Daniel Cheng]

> 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.

[Daniel Cheng]

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.

[Jian Li]

Comment on attachment 54340 [details]
Chromium patch

WebCore/ChangeLog:5
 +          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?

[Jian Li]

A new bug will be filed by dcheng to cover the similar issue on Mac platform.

[Daniel Cheng]

Created attachment 54341 [details]
ChromiumClipboard patch

Added Chromium to ChangeLog entry.

[Jian Li]

Comment on attachment 54341 [details]
ChromiumClipboard patch

r=me

[WebKit Commit Bot]

Comment on attachment 54341 [details]
ChromiumClipboard patch

Clearing flags on attachment: 54341

Committed r58276: <http://trac.webkit.org/changeset/58276>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Daniel Cheng]

Created attachment 54465 [details]
Filter file:// URLs from event.dataTransfer.getData

[Daniel Cheng]

Discussion for fixing other ports (and addressing shortcomings in this patch) (might?) continue in https://bugs.webkit.org/show_bug.cgi?id=38876.