Bug 181616 – Blob conversion and sanitization doesn't work with Microsoft Word for Mac 2011

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

[Ryosuke Niwa]

The blob conversion & sanitation upon paste doesn't work
when copying & pasting from Microsoft Word for Mac 2011
as reported by the latest comment on the bug 124391.

I can reproduce this issue locally as well.

[Radar WebKit Bug Importer]

<rdar://problem/36484908>

[Ryosuke Niwa]

P1 due to the security/privacy implications.

[Ryosuke Niwa]

Unfortunately, we can't make the image available in this case because Microsoft Word is putting only file URLs in the HTML content, and we can't grant access those files in the web content process.

Supporting this would involve starting a new web content process, parse the HTML to be pasted, then UI process to add sandbox extensions for those files in the web content process in which the content is pasted.

[Ryosuke Niwa]

For now, I'm going to address the bug that we're expoing file URLs in the pasted content.

Since Microsoft Word for Mac 2016 has been out for two years, the recommended path for users who are using 2011 version is to upgrade to 2016 version, which does put webarchive into the pasteboard, and image pasting works as expected in the latest version of Safari.

[Ryosuke Niwa]

Created attachment 331838 [details]
WIP

[Andrew Herron]

That's interesting. I had assumed you solved bug 124391 by reading in the files referenced by the file URLs :)

[Ryosuke Niwa]

Created attachment 331895 [details]
Fixes the bug

[Ryosuke Niwa]

Comment on attachment 331895 [details]
Fixes the bug

Oops, I uploaded this patch too early.

[Ryosuke Niwa]

Created attachment 331897 [details]
Fixes the bug

[Wenson Hsieh]

Comment on attachment 331897 [details]
Fixes the bug

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

> Source/WebCore/ChangeLog:27
> +        (WebCore::WebContentReader::readHTML): Fixed the bug by sanitizing the markup, and stripping away file URLs.

Hm...this is a bit more aggressive than just stripping away file URLs, since we're stripping away all URLs that are not one of { http:, https:, data: } by using shouldReplaceSubresourceURL as the filter.

I understand that maintaining a whitelist of URLs to allow is safer than the reverse approach, though...but let's make it clear in the ChangeLog.

> Source/WebCore/editing/cocoa/WebContentReaderCocoa.mm:533
> +            removeSubresourceURLAttributes(fragment, [] (const URL& url ) {

Nit - extra space after URL& url

> Source/WebCore/editing/cocoa/WebContentReaderCocoa.mm:552
> +            removeSubresourceURLAttributes(fragment, [] (const URL& url ) {

Nit - extra space after URL& url

> Source/WebCore/editing/markup.h:54
> +String sanitizeMarkup(const String&, std::optional<std::function<void(DocumentFragment&)>> fragmentSanitizer = std::nullopt);

Nit - I think we generally prefer to use WTF::Function over std::function.

> Tools/TestWebKitAPI/Tests/WebKitCocoa/PasteHTML.mm:117
> +TEST(PasteHTML, StripsHTTPURLs)

Did you mean "StripsFileURLs"?

> Tools/TestWebKitAPI/Tests/WebKitCocoa/PasteHTML.mm:131
> +TEST(PasteHTML, DoesNotStripHTTPURLsWhenCustomPasteboardDataIsDisabled)

Ditto, looks like this was meant to be "DoesNotStripFileURLsWhenCustomPasteboardDataIsDisabled"

[Ryosuke Niwa]

Created attachment 331940 [details]
Patch for landing

[Ryosuke Niwa]

(In reply to Wenson Hsieh from comment #10)
> Comment on attachment 331897 [details]
> Fixes the bug
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=331897&action=review
> 
> > Source/WebCore/ChangeLog:27
> > +        (WebCore::WebContentReader::readHTML): Fixed the bug by sanitizing the markup, and stripping away file URLs.
> 
> Hm...this is a bit more aggressive than just stripping away file URLs, since
> we're stripping away all URLs that are not one of { http:, https:, data: }
> by using shouldReplaceSubresourceURL as the filter.

Good point. Fixed the change log.

> I understand that maintaining a whitelist of URLs to allow is safer than the
> reverse approach, though...but let's make it clear in the ChangeLog.
> 
> > Source/WebCore/editing/cocoa/WebContentReaderCocoa.mm:533
> > +            removeSubresourceURLAttributes(fragment, [] (const URL& url ) {
> 
> Nit - extra space after URL& url

Fixed.

> > Source/WebCore/editing/cocoa/WebContentReaderCocoa.mm:552
> > +            removeSubresourceURLAttributes(fragment, [] (const URL& url ) {
> 
> Nit - extra space after URL& url

Fixed.

> > Source/WebCore/editing/markup.h:54
> > +String sanitizeMarkup(const String&, std::optional<std::function<void(DocumentFragment&)>> fragmentSanitizer = std::nullopt);
> 
> Nit - I think we generally prefer to use WTF::Function over std::function.

Really? I thought we wanted to use std::function instead of WTF::Function going forward. Whatever. I'd use WTF::Function.

> > Tools/TestWebKitAPI/Tests/WebKitCocoa/PasteHTML.mm:117
> > +TEST(PasteHTML, StripsHTTPURLs)
> 
> Did you mean "StripsFileURLs"?

Fixed.

> > Tools/TestWebKitAPI/Tests/WebKitCocoa/PasteHTML.mm:131
> > +TEST(PasteHTML, DoesNotStripHTTPURLsWhenCustomPasteboardDataIsDisabled)
> 
> Ditto, looks like this was meant to be
> "DoesNotStripFileURLsWhenCustomPasteboardDataIsDisabled"

Fixed.

[Wenson Hsieh]

Comment on attachment 331897 [details]
Fixes the bug

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

>>> Source/WebCore/editing/markup.h:54
>>> +String sanitizeMarkup(const String&, std::optional<std::function<void(DocumentFragment&)>> fragmentSanitizer = std::nullopt);
>> 
>> Nit - I think we generally prefer to use WTF::Function over std::function.
> 
> Really? I thought we wanted to use std::function instead of WTF::Function going forward. Whatever. I'd use WTF::Function.

Just asked Fil Pizlo — it's WTF::Function.

[WebKit Commit Bot]

Comment on attachment 331940 [details]
Patch for landing

Rejecting attachment 331940 [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', 'build', '--no-clean', '--no-update', '--build-style=release', '--port=mac']" exit_code: 2 cwd: /Volumes/Data/EWS/WebKit

Last 500 characters of output:
urce22.cpp -o /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/UnifiedSource22.o

** BUILD FAILED **


The following build commands failed:
	CompileC /Volumes/Data/EWS/WebKit/WebKitBuild/WebCore.build/Release/WebCore.build/Objects-normal/x86_64/UnifiedSource8-mm.o /Volumes/Data/EWS/WebKit/WebKitBuild/Release/DerivedSources/WebCore/unified-sources/UnifiedSource8-mm.mm normal x86_64 objective-c++ com.apple.compilers.llvm.clang.1_0.compiler
(1 failure)

Full output: http://webkit-queues.webkit.org/results/6171151

[Ryosuke Niwa]

Created attachment 331949 [details]
Patch for landing

[WebKit Commit Bot]

Comment on attachment 331949 [details]
Patch for landing

Clearing flags on attachment: 331949

Committed r227351: <https://trac.webkit.org/changeset/227351>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.