RESOLVED FIXED 182636
Pasting from Excel no longer provides text/html data
https://bugs.webkit.org/show_bug.cgi?id=182636
Summary Pasting from Excel no longer provides text/html data
Andrew Herron
Reported 2018-02-08 21:48:11 PST
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.
Attachments
simple excel document to replicate the issue (10.02 KB, application/vnd.openxmlformats-officedocument.spreadsheetml.sheet)
2018-02-08 21:48 PST, Andrew Herron
no flags
simple HTML page to print clipboard data (1.11 KB, text/html)
2018-02-08 21:49 PST, Andrew Herron
no flags
First pass (47.06 KB, patch)
2018-02-08 23:53 PST, Wenson Hsieh
no flags
Fix non-Cocoa platforms (49.08 KB, patch)
2018-02-09 00:03 PST, Wenson Hsieh
no flags
+2 more tests (51.09 KB, patch)
2018-02-09 08:53 PST, Wenson Hsieh
no flags
Adjust type order & test expectations. (50.75 KB, patch)
2018-02-09 10:44 PST, Wenson Hsieh
no flags
Address feedback (49.41 KB, patch)
2018-02-09 12:58 PST, Wenson Hsieh
rniwa: review+
One last EWS run (49.42 KB, patch)
2018-02-09 14:41 PST, Wenson Hsieh
commit-queue: commit-queue-
Andrew Herron
Comment 1 2018-02-08 21:49:24 PST
Created attachment 333445 [details] simple HTML page to print clipboard data
Wenson Hsieh
Comment 2 2018-02-08 21:53:40 PST
Wenson Hsieh
Comment 3 2018-02-08 23:53:44 PST
Created attachment 333451 [details] First pass
Wenson Hsieh
Comment 4 2018-02-08 23:55:39 PST
Patch out for review and EWS. Also, here's another test harness that exhibits the problem: https://whsieh.github.io/examples/datatransfer
Wenson Hsieh
Comment 5 2018-02-09 00:03:03 PST
Created attachment 333453 [details] Fix non-Cocoa platforms
Wenson Hsieh
Comment 6 2018-02-09 08:53:13 PST
Created attachment 333491 [details] +2 more tests
Wenson Hsieh
Comment 7 2018-02-09 10:44:14 PST
Created attachment 333499 [details] Adjust type order & test expectations.
Ryosuke Niwa
Comment 8 2018-02-09 12:14:58 PST
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.
Wenson Hsieh
Comment 9 2018-02-09 12:29:14 PST
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.
Wenson Hsieh
Comment 10 2018-02-09 12:58:11 PST
Created attachment 333513 [details] Address feedback
Wenson Hsieh
Comment 11 2018-02-09 13:04:26 PST
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.
Ryosuke Niwa
Comment 12 2018-02-09 13:53:57 PST
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.
Wenson Hsieh
Comment 13 2018-02-09 13:56:37 PST
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!
Wenson Hsieh
Comment 14 2018-02-09 14:41:43 PST
Created attachment 333519 [details] One last EWS run
WebKit Commit Bot
Comment 15 2018-02-09 15:36:22 PST
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
Wenson Hsieh
Comment 16 2018-02-09 15:43:20 PST
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
Ryosuke Niwa
Comment 17 2018-02-13 14:36:40 PST
Note You need to log in before you can comment on or make changes to this bug.