Bug 25882 - [chromium] User paths exposed on file drop
Summary: [chromium] User paths exposed on file drop
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: New Bugs (show other bugs)
Version: 528+ (Nightly build)
Hardware: Macintosh OS X 10.5
: P2 Normal
Assignee: Nobody
URL:
Keywords: InRadar
: 38019 (view as bug list)
Depends on:
Blocks:
 
Reported: 2009-05-19 20:28 PDT by Eric Seidel (no email)
Modified: 2010-05-10 17:27 PDT (History)
10 users (show)

See Also:


Attachments
test case (1.35 KB, text/html)
2009-05-19 20:28 PDT, Eric Seidel (no email)
no flags Details
Don't add file paths to text/uri-list when dragging. (2.29 KB, patch)
2010-04-22 22:16 PDT, Daniel Cheng
dcheng: review-
dcheng: commit-queue-
Details | Formatted Diff | Diff
Patch with layout tests (18.68 KB, patch)
2010-04-23 10:54 PDT, Daniel Cheng
jianli: review-
Details | Formatted Diff | Diff
Addressing review comments. (19.30 KB, patch)
2010-04-23 19:01 PDT, Daniel Cheng
no flags Details | Formatted Diff | Diff
Chromium patch (2.52 KB, patch)
2010-04-26 15:17 PDT, Daniel Cheng
jianli: review-
Details | Formatted Diff | Diff
ChromiumClipboard patch (2.53 KB, patch)
2010-04-26 15:25 PDT, Daniel Cheng
no flags Details | Formatted Diff | Diff
Filter file:// URLs from event.dataTransfer.getData (3.32 KB, patch)
2010-04-27 16:03 PDT, Daniel Cheng
dcheng: review-
dcheng: commit-queue-
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
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
<rdar://problem/7898443>
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");
         results.add("Files");
     }

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

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

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

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

r=me
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.