Created attachment 333444 [details] simple excel document to replicate the issue Attached is a simple Excel document and a HTML page that hooks into the paste event and prints available clipboard data types to the clipboard when pasting into a ContentEditable. Copying from Excel and pasting into the page gives the following results. Safari 11.0.2 release: a big list of data including text/html Safari 11.1 (13605.1.25.1) and STP49: Only one entry, type "Files" with no data. In both cases the copied data does appear in the ContentEditable, there's no issue there, it's just the paste event data.
Created attachment 333445 [details] simple HTML page to print clipboard data
<rdar://problem/37087060>
Created attachment 333451 [details] First pass
Patch out for review and EWS. Also, here's another test harness that exhibits the problem: https://whsieh.github.io/examples/datatransfer
Created attachment 333453 [details] Fix non-Cocoa platforms
Created attachment 333491 [details] +2 more tests
Created attachment 333499 [details] Adjust type order & test expectations.
Comment on attachment 333499 [details] Adjust type order & test expectations. View in context: https://bugs.webkit.org/attachment.cgi?id=333499&action=review > Source/WebCore/ChangeLog:57 > + "text/plain". I opted for this because it seems less hacky than appending "text/html" separately, on top of > + "text/uri-list". If you're doing that here, then it's weird that getDataForItem has special code paths for "text/uri-list" and "text/html". Either we should always special case those two, or always remove plain text. Using two different strategies in those two places should lead to bugs. r- because I think we need to at least address this problem before landing this. > Source/WebCore/dom/DataTransfer.cpp:158 > + if (lowercaseType == "text/html" && RuntimeEnabledFeatures::sharedFeatures().customPasteboardDataEnabled()) { Why do we need to check RuntimeEnabledFeatures::sharedFeatures().customPasteboardDataEnabled() here? > Source/WebCore/dom/DataTransfer.cpp:175 > +String DataTransfer::readPasteboardDataForBindingsRespectingOrigin(Document& document, const String& lowercaseType, WebContentReadingPolicy policy) const DataTransfer object is for bindings only. There is no need to say that. Just say readStringFromPasteboard or something. > Source/WebCore/dom/DataTransfer.cpp:310 > + // We don't expose 'text/plain' to the page if the pasteboard contains files, because plain text data may contain file paths. Just say "Remove "text/plain" to avoid exposing local file paths". No need to say we don't expose it. It's obvious from the code. > Source/WebCore/platform/Pasteboard.h:67 > +enum class WebContentReadingPolicy { ReadFromAnyType, OnlyReadFromRichTextTypes }; Since the enum says "reading policy", we probably don't need to say "ReadFrom". Just AnyTime, OnlyRichTextTypes would suffice. > Source/WebCore/platform/ios/PasteboardIOS.mm:166 > +static bool typeIsAppropriateForWebContentReadingPolicy(NSString *type, WebContentReadingPolicy policy) How about just isTypeAllowedByReadingPolicy? I don't think we need to spell out the full enum name since it's in the argument list. > Source/WebCore/platform/ios/PasteboardIOS.mm:168 > + return policy == WebContentReadingPolicy::ReadFromAnyType || [type isEqualToString:WebArchivePboardType] || [type isEqualToString:(NSString *)kUTTypeHTML] || [type isEqualToString:(NSString *)kUTTypeRTF] || [type isEqualToString:(NSString *)kUTTypeFlatRTFD]; Please wrap this line at some point.
Comment on attachment 333499 [details] Adjust type order & test expectations. View in context: https://bugs.webkit.org/attachment.cgi?id=333499&action=review Thanks for taking a look! v2 forthcoming >> Source/WebCore/ChangeLog:57 >> + "text/uri-list". > > If you're doing that here, then it's weird that getDataForItem has special code paths for "text/uri-list" and "text/html". > Either we should always special case those two, or always remove plain text. > Using two different strategies in those two places should lead to bugs. > r- because I think we need to at least address this problem before landing this. That's a good point. I'll change DataTransfer::types to add "text/uri-list" and "text/html" as special cases, which is more similar to what we do now. >> Source/WebCore/dom/DataTransfer.cpp:158 >> + if (lowercaseType == "text/html" && RuntimeEnabledFeatures::sharedFeatures().customPasteboardDataEnabled()) { > > Why do we need to check RuntimeEnabledFeatures::sharedFeatures().customPasteboardDataEnabled() here? I'll change the comment below to explain this more clearly. We don't sanitize any markup if custom pasteboard data is disabled, so it would be a regression from previous releases that we could leak file paths in HTML markup when dropping. >> Source/WebCore/dom/DataTransfer.cpp:175 >> +String DataTransfer::readPasteboardDataForBindingsRespectingOrigin(Document& document, const String& lowercaseType, WebContentReadingPolicy policy) const > > DataTransfer object is for bindings only. There is no need to say that. Just say readStringFromPasteboard or something. Fixed! >> Source/WebCore/dom/DataTransfer.cpp:310 >> + // We don't expose 'text/plain' to the page if the pasteboard contains files, because plain text data may contain file paths. > > Just say "Remove "text/plain" to avoid exposing local file paths". > No need to say we don't expose it. It's obvious from the code. I'm reverting this to just add back "text/html" and "text/uri-list" instead. >> Source/WebCore/platform/Pasteboard.h:67 >> +enum class WebContentReadingPolicy { ReadFromAnyType, OnlyReadFromRichTextTypes }; > > Since the enum says "reading policy", we probably don't need to say "ReadFrom". Just AnyTime, OnlyRichTextTypes would suffice. Sounds good — `enum class WebContentReadingPolicy { AnyType, OnlyRichTextTypes };` >> Source/WebCore/platform/ios/PasteboardIOS.mm:166 >> +static bool typeIsAppropriateForWebContentReadingPolicy(NSString *type, WebContentReadingPolicy policy) > > How about just isTypeAllowedByReadingPolicy? I don't think we need to spell out the full enum name since it's in the argument list. Sure! Changed to isTypeAllowedByReadingPolicy >> Source/WebCore/platform/ios/PasteboardIOS.mm:168 >> + return policy == WebContentReadingPolicy::ReadFromAnyType || [type isEqualToString:WebArchivePboardType] || [type isEqualToString:(NSString *)kUTTypeHTML] || [type isEqualToString:(NSString *)kUTTypeRTF] || [type isEqualToString:(NSString *)kUTTypeFlatRTFD]; > > Please wrap this line at some point. Ok! Added line wrapping.
Created attachment 333513 [details] Address feedback
Comment on attachment 333513 [details] Address feedback View in context: https://bugs.webkit.org/attachment.cgi?id=333513&action=review > Source/WebCore/dom/DataTransfer.cpp:310 > + if (safeTypes.contains("text/html")) Hm, this should probably check for custom pasteboard data too to match getDataForItem.
Comment on attachment 333513 [details] Address feedback View in context: https://bugs.webkit.org/attachment.cgi?id=333513&action=review > LayoutTests/editing/pasteboard/paste-image-does-not-reveal-file-url.html:34 > - shouldBeEqualToString('JSON.stringify(event.clipboardData.types)', '["Files"]'); > + shouldNotBe('event.clipboardData.types.indexOf("Files")', '-1'); Use event.clipboardData.types.includes("Files") instead.
Comment on attachment 333513 [details] Address feedback View in context: https://bugs.webkit.org/attachment.cgi?id=333513&action=review >> LayoutTests/editing/pasteboard/paste-image-does-not-reveal-file-url.html:34 >> + shouldNotBe('event.clipboardData.types.indexOf("Files")', '-1'); > > Use event.clipboardData.types.includes("Files") instead. Good idea — fixed!
Created attachment 333519 [details] One last EWS run
Comment on attachment 333519 [details] One last EWS run Rejecting attachment 333519 [details] from commit-queue. Failed to run "['/Volumes/Data/EWS/WebKit/Tools/Scripts/webkit-patch', '--status-host=webkit-queues.webkit.org', '--bot-id=webkit-cq-01', 'apply-attachment', '--no-update', '--non-interactive', 333519, '--port=mac']" exit_code: 1 cwd: /Volumes/Data/EWS/WebKit Last 500 characters of output: d/mechanize/_urllib2_fork.py", line 332, in _call_chain result = func(*args) File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_urllib2_fork.py", line 1170, in https_open return self.do_open(conn_factory, req) File "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/thirdparty/autoinstalled/mechanize/_urllib2_fork.py", line 1118, in do_open raise URLError(err) urllib2.URLError: <urlopen error EOF occurred in violation of protocol (_ssl.c:590)> Full output: http://webkit-queues.webkit.org/results/6438005
Landed <http://trac.webkit.org/changeset/228340/webkit> manually. > "/Volumes/Data/EWS/WebKit/Tools/Scripts/webkitpy/thirdparty/autoinstalled/ > mechanize/_urllib2_fork.py", line 1118, in do_open > raise URLError(err) > urllib2.URLError: <urlopen error EOF occurred in violation of protocol > (_ssl.c:590)> > > Full output: http://webkit-queues.webkit.org/results/6438005 + Aakash, Lucas
Landed in http://trac.webkit.org/changeset/228340/webkit.