Bug 191114

Summary: API test WKAttachmentTests.CopyAndPasteBetweenWebViews fails on macOS 10.13
Product: WebKit Reporter: Wenson Hsieh <wenson_hsieh>
Component: HTML EditingAssignee: Wenson Hsieh <wenson_hsieh>
Status: RESOLVED FIXED    
Severity: Normal CC: bdakin, bfulgham, commit-queue, dino, ews-watchlist, mitz, ryanhaddad, thorton, tsavell, webkit-bug-importer, wenson_hsieh
Priority: P2 Keywords: InRadar
Version: WebKit Nightly Build   
Hardware: Unspecified   
OS: Unspecified   
Attachments:
Description Flags
Speculative fix
none
Speculative fix
none
Patch
none
Patch
dino: review+, ews-watchlist: commit-queue-
Patch for landing
none
Archive of layout-test-results from ews121 for ios-simulator-wk2 none

Wenson Hsieh
Reported 2018-10-31 08:18:35 PDT
WKAttachmentTests.CopyAndPasteBetweenWebViews, the API test added with <https://trac.webkit.org/changeset/237624/webkit>, fails on macOS 10.13. It works on macOS 10.12 because USE_SECURE_ARCHIVER_API is 0, so we simply archive the NSFileWrapper; and it works on macOS 10.14 because USE_SECURE_ARCHIVER_API is 1, but NSFileWrapper is secure-coding-compliant. On macOS 10.13, we try to use the secure archiver despite NSFileWrapper not supporting it.
Attachments
Speculative fix (2.02 KB, patch)
2018-10-31 08:38 PDT, Wenson Hsieh
no flags
Speculative fix (2.28 KB, patch)
2018-10-31 09:18 PDT, Wenson Hsieh
no flags
Patch (2.28 KB, patch)
2018-10-31 09:21 PDT, Wenson Hsieh
no flags
Patch (1.75 KB, patch)
2018-10-31 17:58 PDT, Wenson Hsieh
dino: review+
ews-watchlist: commit-queue-
Patch for landing (1.84 KB, patch)
2018-10-31 18:34 PDT, Wenson Hsieh
no flags
Archive of layout-test-results from ews121 for ios-simulator-wk2 (2.32 MB, application/zip)
2018-10-31 20:02 PDT, EWS Watchlist
no flags
Wenson Hsieh
Comment 1 2018-10-31 08:28:41 PDT
I think we can address this by only requiring secure archival on macOS 10.14+ and iOS 12+
Radar WebKit Bug Importer
Comment 2 2018-10-31 08:29:55 PDT
Wenson Hsieh
Comment 3 2018-10-31 08:38:53 PDT
Created attachment 353495 [details] Speculative fix
Wenson Hsieh
Comment 4 2018-10-31 08:39:52 PDT
Comment on attachment 353495 [details] Speculative fix View in context: https://bugs.webkit.org/attachment.cgi?id=353495&action=review > Source/WebKit/ChangeLog:13 > + To fix this, we only use the secure archiver on ⥠macOS 10.14 and ⥠iOS 12. (Bugzilla garbled ≥ into â¥)
Wenson Hsieh
Comment 5 2018-10-31 09:18:47 PDT
Created attachment 353498 [details] Speculative fix
Wenson Hsieh
Comment 6 2018-10-31 09:21:05 PDT
WebKit Commit Bot
Comment 7 2018-10-31 13:16:57 PDT
Comment on attachment 353499 [details] Patch Clearing flags on attachment: 353499 Committed r237648: <https://trac.webkit.org/changeset/237648>
WebKit Commit Bot
Comment 8 2018-10-31 13:16:59 PDT
All reviewed patches have been landed. Closing bug.
Wenson Hsieh
Comment 9 2018-10-31 16:39:42 PDT
Wenson Hsieh
Comment 10 2018-10-31 17:58:53 PDT
Dean Jackson
Comment 11 2018-10-31 18:02:15 PDT
Comment on attachment 353563 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353563&action=review > Source/WebKit/ChangeLog:9 > + Followup to r237648: also make sure that we unsecurely unarchive NSFileWrapper on platforms that don't support typo: unsecurely > Source/WebKit/UIProcess/API/Cocoa/APIAttachmentCocoa.mm:161 > + NSFileWrapper *fileWrapper = insecurelyUnarchiveObjectFromData(serializedData.get()); Isn't this insecure? :)
Wenson Hsieh
Comment 12 2018-10-31 18:04:03 PDT
Comment on attachment 353563 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353563&action=review >> Source/WebKit/ChangeLog:9 >> + Followup to r237648: also make sure that we unsecurely unarchive NSFileWrapper on platforms that don't support > > typo: unsecurely Oops! Fixed.
Wenson Hsieh
Comment 13 2018-10-31 18:15:04 PDT
Comment on attachment 353563 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=353563&action=review >> Source/WebKit/UIProcess/API/Cocoa/APIAttachmentCocoa.mm:161 >> + NSFileWrapper *fileWrapper = insecurelyUnarchiveObjectFromData(serializedData.get()); > > Isn't this insecure? :) ...that's a good point, I think this additionally warrants a check that [fileWrapper isKindOfClass:NSFileWrapper.class]
Wenson Hsieh
Comment 14 2018-10-31 18:34:30 PDT
Created attachment 353568 [details] Patch for landing
WebKit Commit Bot
Comment 15 2018-10-31 19:11:28 PDT
Comment on attachment 353568 [details] Patch for landing Clearing flags on attachment: 353568 Committed r237667: <https://trac.webkit.org/changeset/237667>
EWS Watchlist
Comment 16 2018-10-31 20:01:59 PDT
Comment on attachment 353563 [details] Patch Attachment 353563 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/9805037 New failing tests: http/wpt/mediarecorder/MediaRecorder-mock-dataavailable.html
EWS Watchlist
Comment 17 2018-10-31 20:02:01 PDT
Created attachment 353573 [details] Archive of layout-test-results from ews121 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews121 Port: ios-simulator-wk2 Platform: Mac OS X 10.13.6
Note You need to log in before you can comment on or make changes to this bug.