Bug 177710 – Don't reveal file URL when pasting an image

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

[Ryosuke Niwa]

When pasting an image, we shouldn't reveal its local file path in text/plain or text/uri-list.
In fact, we should just hide all non-file types as we do for drag & drop.

[Radar WebKit Bug Importer]

<rdar://problem/34757924>

[Ryosuke Niwa]

Created attachment 322305 [details]
Fixes the bug

[Wenson Hsieh]

Comment on attachment 322305 [details]
Fixes the bug

View in context: https://bugs.webkit.org/attachment.cgi?id=322305&action=review

> Source/WebCore/platform/StaticPasteboard.h:61
> +    bool containsFiles() final { return 0; }

I prefer `return false`, since the return type is a bool.

> Source/WebCore/platform/cocoa/PasteboardCocoa.mm:163
> +    // Enforce changeCount ourselves for security. We check after reading instead of before to be

It seems this piece of code (and comment) keeps popping up in multiple places in this file :/ Now that we have a PasteboardCocoa.mm, it seems we should pull this out into a separate helper in this file?

[Ryosuke Niwa]

(In reply to Wenson Hsieh from comment #3)
> Comment on attachment 322305 [details]
> Fixes the bug
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=322305&action=review
> 
> > Source/WebCore/platform/StaticPasteboard.h:61
> > +    bool containsFiles() final { return 0; }
> 
> I prefer `return false`, since the return type is a bool.

Oops, I meant to use "false" there.

> > Source/WebCore/platform/cocoa/PasteboardCocoa.mm:163
> > +    // Enforce changeCount ourselves for security. We check after reading instead of before to be
> 
> It seems this piece of code (and comment) keeps popping up in multiple
> places in this file :/ Now that we have a PasteboardCocoa.mm, it seems we
> should pull this out into a separate helper in this file?

Perhaps. We probably need a function that takes lambda to do this.

[Ryosuke Niwa]

Created attachment 322312 [details]
Replaced 0 by false

[Wenson Hsieh]

Comment on attachment 322312 [details]
Replaced 0 by false

View in context: https://bugs.webkit.org/attachment.cgi?id=322312&action=review

> Source/WebCore/dom/DataTransfer.cpp:178
> +    if (m_pasteboard->containsFiles())

Heads up: with this change, we might need to rebaseline the API test DataInteractionTests.DataTransferGetDataWhenDroppingImageWithFileURL to remove the @"text/uri-list": @"" expectation.

[Ryosuke Niwa]

(In reply to Wenson Hsieh from comment #6)
> Comment on attachment 322312 [details]
> Replaced 0 by false
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=322312&action=review
> 
> > Source/WebCore/dom/DataTransfer.cpp:178
> > +    if (m_pasteboard->containsFiles())
> 
> Heads up: with this change, we might need to rebaseline the API test
> DataInteractionTests.DataTransferGetDataWhenDroppingImageWithFileURL to
> remove the @"text/uri-list": @"" expectation.

Oh that's a good point. Let me check that...

[Ryosuke Niwa]

Looks like there's a slight issue with PlatformPasteboard::numberOfFiles() in iOS. We need to replace numberOfFiles with droppedFileURLs's size instead. It claims to have a file even when it's just a text content and won't return a file URL.

[Wenson Hsieh]

(In reply to Ryosuke Niwa from comment #8)
> Looks like there's a slight issue with PlatformPasteboard::numberOfFiles()
> in iOS. We need to replace numberOfFiles with droppedFileURLs's size
> instead. It claims to have a file even when it's just a text content and
> won't return a file URL.

Hmm...-droppedFileURLs will return 0 prior to the drop. If we just return -droppedFileURLs in PlatformPasteboard::numberOfFiles, I suspect that we would break  uploading by dropping into inputs of type file.

[Ryosuke Niwa]

Actually, I think I was misreading the test. DataTransferGetDataWhenDroppingRespectsPresentationStyle should return "Files" now because it's doing registerFileRepresentationForTypeIdentifier.

[Ryosuke Niwa]

Rather, the actual observation is that there are two test cases in DataTransferGetDataWhenDroppingRespectsPresentationStyle, and the one with UIPreferredPresentationStyleAttachment needs to be rebaselined.

[Wenson Hsieh]

(In reply to Ryosuke Niwa from comment #11)
> Rather, the actual observation is that there are two test cases in
> DataTransferGetDataWhenDroppingRespectsPresentationStyle, and the one with
> UIPreferredPresentationStyleAttachment needs to be rebaselined.

Yes, that's correct. A bit more detail: this test involves two parts: (1) dropping an item provider containing text, with UIPreferredPresentationStyleAttachment set and (2) dropping an item provider containing text, with UIPreferredPresentationStyleInline set. The fact that -registerFileRepresentationForTypeIdentifier is used to supply the data is actually not important.

The former simulates dropping a text file (e.g. from Files.app) while the latter involves dropping a text selection. I would expect "Files" to appear in the list of types in the former case, but not the latter.

[Ryosuke Niwa]

Created attachment 322313 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 322313 [details]
Patch for landing

Clearing flags on attachment: 322313

Committed r222688: <http://trac.webkit.org/changeset/222688>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Ryan Haddad]

API test DataInteractionTests.ExternalSourceDataTransferItemGetPlainTextFileAsEntry is failing on iOS after this change:

/Volumes/Data/slave/ios-simulator-11-release/build/Tools/TestWebKitAPI/Tests/ios/DataInteractionTests.mm:1063
Value of: [webView stringByEvaluatingJavaScript:@"output.value"]
  Actual: "Found data transfer item (kind: 'string', type: 'text/plain')
Found data transfer item (kind: 'file', type: 'text/plain')
FILE: /foo.txt ('text/plain', 28 bytes)"
Expected: [expectedOutput componentsJoinedByString:@"\n"]
Which is: "Found data transfer item (kind: 'file', type: 'text/plain')
FILE: /foo.txt ('text/plain', 28 bytes)"

https://build.webkit.org/builders/Apple%20iOS%2011%20Simulator%20Release%20WK2%20%28Tests%29/builds/219

[Wenson Hsieh]

(In reply to Ryan Haddad from comment #16)
> API test
> DataInteractionTests.ExternalSourceDataTransferItemGetPlainTextFileAsEntry
> is failing on iOS after this change:
> 
> /Volumes/Data/slave/ios-simulator-11-release/build/Tools/TestWebKitAPI/Tests/
> ios/DataInteractionTests.mm:1063
> Value of: [webView stringByEvaluatingJavaScript:@"output.value"]
>   Actual: "Found data transfer item (kind: 'string', type: 'text/plain')
> Found data transfer item (kind: 'file', type: 'text/plain')
> FILE: /foo.txt ('text/plain', 28 bytes)"
> Expected: [expectedOutput componentsJoinedByString:@"\n"]
> Which is: "Found data transfer item (kind: 'file', type: 'text/plain')
> FILE: /foo.txt ('text/plain', 28 bytes)"
> 
> https://build.webkit.org/builders/
> Apple%20iOS%2011%20Simulator%20Release%20WK2%20%28Tests%29/builds/219

I suspect this is because the custom pasteboard data is not enabled for these iOS 11 bots; we should ifdef this test out, like the other DataTransfer(Get|Set)Data tests in this file. We still have coverage on internal bots, and API tests are not run in EWS anyways.

[Ryosuke Niwa]

The API tests have been skipped in https://trac.webkit.org/changeset/222743.