Bug 182636 – Pasting from Excel no longer provides text/html data

• Status: RESOLVED
• Resolution: FIXED
• Component: HTML Editing

[Andrew Herron]

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.

[Andrew Herron]

Created attachment 333445 [details]
simple HTML page to print clipboard data

[Wenson Hsieh]

<rdar://problem/37087060>

[Wenson Hsieh]

Created attachment 333451 [details]
First pass

[Wenson Hsieh]

Patch out for review and EWS.

Also, here's another test harness that exhibits the problem: https://whsieh.github.io/examples/datatransfer

[Wenson Hsieh]

Created attachment 333453 [details]
Fix non-Cocoa platforms

[Wenson Hsieh]

Created attachment 333491 [details]
+2 more tests

[Wenson Hsieh]

Created attachment 333499 [details]
Adjust type order & test expectations.

[Ryosuke Niwa]

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

Created attachment 333513 [details]
Address feedback

[Wenson Hsieh]

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

Created attachment 333519 [details]
One last EWS run

[WebKit Commit Bot]

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]

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]

Landed in http://trac.webkit.org/changeset/228340/webkit.