Summary: | Pasting from Excel no longer provides text/html data | ||||||||||||||||||||
---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|---|
Product: | WebKit | Reporter: | Andrew Herron <thespyder> | ||||||||||||||||||
Component: | HTML Editing | Assignee: | Wenson Hsieh <wenson_hsieh> | ||||||||||||||||||
Status: | RESOLVED FIXED | ||||||||||||||||||||
Severity: | Normal | CC: | aakash_jain, bdakin, cdumez, commit-queue, dbates, esprehn+autocc, ews-watchlist, kangil.han, lforschler, rniwa, thorton, webkit-bug-importer, wenson_hsieh | ||||||||||||||||||
Priority: | P2 | Keywords: | InRadar | ||||||||||||||||||
Version: | Safari Technology Preview | ||||||||||||||||||||
Hardware: | Unspecified | ||||||||||||||||||||
OS: | Unspecified | ||||||||||||||||||||
Attachments: |
|
Created attachment 333445 [details]
simple HTML page to print clipboard data
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 |
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.