Bug 181199 – [Attachment Support] Support dragging attachment elements out as files on iOS

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

[Wenson Hsieh]

The iOS side of dragging attachment elements. After this, one would be able to drag an attachment element inserted via paste, drop, or WKAttachment SPI out into, say, the Files app or Notes.

[Radar WebKit Bug Importer]

<rdar://problem/36299316>

[Wenson Hsieh]

Created attachment 330502 [details]
Blocked on https://bugs.webkit.org/show_bug.cgi?id=181236

[Wenson Hsieh]

Comment on attachment 330502 [details]
Blocked on https://bugs.webkit.org/show_bug.cgi?id=181236

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

> Tools/TestWebKitAPI/Tests/WebKitCocoa/WKAttachmentTests.mm:348
> +    [self loadDataRepresentationForTypeIdentifier:(NSString *)kUTTypePDF completionHandler:^(NSData *observedData, NSError *error) {

s/(NSString *)kUTTypePDF/type/

I'll fix this when I next rebase this patch on trunk.

[Wenson Hsieh]

Created attachment 330611 [details]
Rebase on master

[Wenson Hsieh]

Comment on attachment 330611 [details]
Rebase on master

I think this will need a +3 review, due to filesystem manipulation.

[Tim Horton]

Comment on attachment 330611 [details]
Rebase on master

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

> Source/WebKit/ChangeLog:78
> -2018-01-05  Wenson Hsieh  <[e-mail]>
> [phone]  Wenson Hsieh  <[e-mail]>

?

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:4559
> +        NSURL *destinationURL = [NSURL fileURLWithPath:[temporaryBlobDirectory stringByAppendingPathComponent:[NSUUID UUID].UUIDString]];

Minor concern here as expressed in person, but hopefully someone knows better than me.

[Wenson Hsieh]

Comment on attachment 330611 [details]
Rebase on master

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

>> Source/WebKit/ChangeLog:78
>> [phone]  Wenson Hsieh  <[e-mail]>
> 
> ?

Oops — fixed!

>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:4559
>> +        NSURL *destinationURL = [NSURL fileURLWithPath:[temporaryBlobDirectory stringByAppendingPathComponent:[NSUUID UUID].UUIDString]];
> 
> Minor concern here as expressed in person, but hopefully someone knows better than me.

Indeed…to rephrase, the concern here is that this URL is generated by the UI process but the writing is done out-of-process, but really, we prefer the temporary file to be generated and the blob data to be written atomically.

[Wenson Hsieh]

(In reply to Wenson Hsieh from comment #7)
> Comment on attachment 330611 [details]
> Rebase on master
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=330611&action=review
> 
> >> Source/WebKit/ChangeLog:78
> >> [phone]  Wenson Hsieh  <[e-mail]>
> > 
> > ?
> 
> Oops — fixed!
> 
> >> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:4559
> >> +        NSURL *destinationURL = [NSURL fileURLWithPath:[temporaryBlobDirectory stringByAppendingPathComponent:[NSUUID UUID].UUIDString]];
> > 
> > Minor concern here as expressed in person, but hopefully someone knows better than me.
> 
> Indeed…to rephrase, the concern here is that this URL is generated by the UI
> process but the writing is done out-of-process, but really, we prefer the
> temporary file to be generated and the blob data to be written atomically.

I wonder if we can address this by using writeBlobsToTemporaryFiles for iOS, which generates and immediately writes to temporary files out of process. It could then then deliver the file paths containing the blob data to the UI process.

As an aside, I think we'd still need the mechanism added in r226470 (writeBlobToFilePath) for macOS, where we're given a designated file path we need to write to, so we can't just have the network process generate a temp path.

Tim/Brady/Andy, what do you guys think?

[Wenson Hsieh]

(In reply to Wenson Hsieh from comment #8)
> (In reply to Wenson Hsieh from comment #7)
> > Comment on attachment 330611 [details]
> > Rebase on master
> > 
> > View in context:
> > https://bugs.webkit.org/attachment.cgi?id=330611&action=review
> > 
> > >> Source/WebKit/ChangeLog:78
> > >> [phone]  Wenson Hsieh  <[e-mail]>
> > > 
> > > ?
> > 
> > Oops — fixed!
> > 
> > >> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:4559
> > >> +        NSURL *destinationURL = [NSURL fileURLWithPath:[temporaryBlobDirectory stringByAppendingPathComponent:[NSUUID UUID].UUIDString]];
> > > 
> > > Minor concern here as expressed in person, but hopefully someone knows better than me.
> > 
> > Indeed…to rephrase, the concern here is that this URL is generated by the UI
> > process but the writing is done out-of-process, but really, we prefer the
> > temporary file to be generated and the blob data to be written atomically.
> 
> I wonder if we can address this by using writeBlobsToTemporaryFiles for iOS,
> which generates and immediately writes to temporary files out of process. It
> could then then deliver the file paths containing the blob data to the UI
> process.
> 
> As an aside, I think we'd still need the mechanism added in r226470
> (writeBlobToFilePath) for macOS, where we're given a designated file path we
> need to write to, so we can't just have the network process generate a temp
> path.
> 
> Tim/Brady/Andy, what do you guys think?

After talking to Tim and Brady in person: the exploit in question is only possible from the app process. Our security model is such that we trust the app, so the approach of saving the blob to a designated path (coming from the web process) will work.

[Wenson Hsieh]

Bump :)

Anyone have a moment to look at this?

[Wenson Hsieh]

> app, so the approach of saving the blob to a designated path (coming from
> the web process) will work.

(Heh, I meant to write "a designated path coming from the _UI_ process", not the web process. A very important difference!)

[Andy Estes]

Comment on attachment 330611 [details]
Rebase on master

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

r=me

> Source/WebCore/platform/ios/WebItemProviderPasteboard.mm:59
> +@property (nonatomic, readonly, strong) NSString *typeIdentifier;
> +@property (nonatomic, readonly, strong) NSData *data;

These properties are strong by default, so you don't need to specify it.

> Source/WebCore/platform/ios/WebItemProviderPasteboard.mm:73
>      if (self = [super init]) {
> -        _representingObject = representingObject;
> -        _typeIdentifier = typeIdentifier;
>          _data = data;
> +        _typeIdentifier = utiType;
>      }
>      return self;

WebKit style is to return early if [super init] returns nil.

> Source/WebCore/platform/ios/WebItemProviderPasteboard.mm:113
> +@property (nonatomic, readonly, strong) id <UIItemProviderWriting> representingObject;

No need for strong.

> Source/WebCore/platform/ios/WebItemProviderPasteboard.mm:165
> +    if (self = [super init]) {
> +        _typeIdentifier = utiType;
> +        _callback = callback;
> +    }
> +    return self;

Ditto about early return.

> Source/WebCore/platform/ios/WebItemProviderPasteboard.mm:170
> +    [itemProvider registerFileRepresentationForTypeIdentifier:_typeIdentifier.get() fileOptions:0 visibility:NSItemProviderRepresentationVisibilityAll loadHandler:[callback = _callback] (void (^completionHandler)(NSURL *, BOOL, NSError *)) -> NSProgress * {

This would be more readable if you created a typedef for "void (^)(NSURL *, BOOL, NSError *)" like we did with ItemProviderDataLoadCompletionHandler.

[Wenson Hsieh]

Comment on attachment 330611 [details]
Rebase on master

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

Thanks for the review, Andy!

>> Source/WebCore/platform/ios/WebItemProviderPasteboard.mm:59
>> +@property (nonatomic, readonly, strong) NSData *data;
> 
> These properties are strong by default, so you don't need to specify it.

Oh, good point. Removed the strong.

>> Source/WebCore/platform/ios/WebItemProviderPasteboard.mm:73
>>      return self;
> 
> WebKit style is to return early if [super init] returns nil.

Sounds good — changed to return early.

>> Source/WebCore/platform/ios/WebItemProviderPasteboard.mm:113
>> +@property (nonatomic, readonly, strong) id <UIItemProviderWriting> representingObject;
> 
> No need for strong.

Removed the strong.

>> Source/WebCore/platform/ios/WebItemProviderPasteboard.mm:165
>> +    return self;
> 
> Ditto about early return.

Done!

>> Source/WebCore/platform/ios/WebItemProviderPasteboard.mm:170
>> +    [itemProvider registerFileRepresentationForTypeIdentifier:_typeIdentifier.get() fileOptions:0 visibility:NSItemProviderRepresentationVisibilityAll loadHandler:[callback = _callback] (void (^completionHandler)(NSURL *, BOOL, NSError *)) -> NSProgress * {
> 
> This would be more readable if you created a typedef for "void (^)(NSURL *, BOOL, NSError *)" like we did with ItemProviderDataLoadCompletionHandler.

Good idea, changed to use a typedef.

[Wenson Hsieh]

Created attachment 330949 [details]
Rebase & address comments

[Wenson Hsieh]

As per tradition, this needs one more review (in particular, around the parts of the patch that affect the filesystem).

[EWS Watchlist]

Comment on attachment 330949 [details]
Rebase & address comments

Attachment 330949 [details] did not pass mac-ews (mac):
Output: http://webkit-queues.webkit.org/results/6026022

New failing tests:
http/tests/misc/slow-loading-animated-image.html

[EWS Watchlist]

Created attachment 330977 [details]
Archive of layout-test-results from ews100 for mac-sierra

The attached test failures were seen while running run-webkit-tests on the mac-ews.
Bot: ews100  Port: mac-sierra  Platform: Mac OS X 10.12.6

[Joseph Pecoraro]

Comment on attachment 330949 [details]
Rebase & address comments

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

Looks good to me as well.

> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:501
> +        RELEASE_LOG(DragAndDrop, "Removed temporary download directory: %@ with error: %@", directory, error);

I suspect that all of these RELEASE_LOGs with non-scalar values going through os_log on macOS would have the non-scalar values <redacted> unless they were specified with %{public}@. Not sure what your actual intention with these logs are, whether you expect them to be redacted or if you really want them to show up on all systems.

[Wenson Hsieh]

Comment on attachment 330949 [details]
Rebase & address comments

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

>> Source/WebKit/UIProcess/ios/WKContentViewInteraction.mm:501
>> +        RELEASE_LOG(DragAndDrop, "Removed temporary download directory: %@ with error: %@", directory, error);
> 
> I suspect that all of these RELEASE_LOGs with non-scalar values going through os_log on macOS would have the non-scalar values <redacted> unless they were specified with %{public}@. Not sure what your actual intention with these logs are, whether you expect them to be redacted or if you really want them to show up on all systems.

Interesting point! So far, I've been mostly using these RELEASE_LOGs to help diagnose drag and drop issues on other (apple internal) devices, so I think leaving these redacted on public builds should be OK.

[Wenson Hsieh]

Created attachment 331038 [details]
Trying EWS again?

[WebKit Commit Bot]

Comment on attachment 331038 [details]
Trying EWS again?

Clearing flags on attachment: 331038

Committed r226779: <https://trac.webkit.org/changeset/226779>