|Summary:||WebPageProxy should use the right path for sandbox extension|
|Product:||WebKit||Reporter:||youenn fablet <youennf>|
|Component:||Page Loading||Assignee:||youenn fablet <youennf>|
|Severity:||Normal||CC:||achristensen, andersca, ap, beidson, commit-queue, ews-watchlist, ggaren, rniwa, thorton, webkit-bug-importer|
|Version:||WebKit Nightly Build|
Description youenn fablet 2019-06-15 21:06:44 PDT
WebPageProxy should use the right path for sandbox extension
Comment 3 EWS Watchlist 2019-06-15 22:01:04 PDT
Comment on attachment 372216 [details] Patch Attachment 372216 [details] did not pass mac-wk2-ews (mac-wk2): Output: https://webkit-queues.webkit.org/results/12488522 Number of test failures exceeded the failure limit.
Comment 4 EWS Watchlist 2019-06-15 22:01:05 PDT
Created attachment 372218 [details] Archive of layout-test-results from ews104 for mac-highsierra-wk2 The attached test failures were seen while running run-webkit-tests on the mac-wk2-ews. Bot: ews104 Port: mac-highsierra-wk2 Platform: Mac OS X 10.13.6
Comment 5 EWS Watchlist 2019-06-15 23:08:56 PDT
Comment on attachment 372216 [details] Patch Attachment 372216 [details] did not pass ios-sim-ews (ios-simulator-wk2): Output: https://webkit-queues.webkit.org/results/12488608 New failing tests: http/tests/resourceLoadStatistics/website-data-removal-for-site-navigated-to-with-link-decoration.html
Comment 6 EWS Watchlist 2019-06-15 23:08:57 PDT
Created attachment 372219 [details] Archive of layout-test-results from ews124 for ios-simulator-wk2 The attached test failures were seen while running run-webkit-tests on the ios-sim-ews. Bot: ews124 Port: ios-simulator-wk2 Platform: Mac OS X 10.14.5
Comment 7 Alexey Proskuryakov 2019-06-16 00:09:10 PDT
How will be subresources loaded after going through this path?
Comment 11 Geoffrey Garen 2019-06-18 11:42:58 PDT
Comment on attachment 372350 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=372350&action=review > Source/WebKit/ChangeLog:10 > + When doing process swapping, a file based loading may end up being loaded through loadRequestWithNavigationShared. > + In that case, the sandbox extension path is set to '/' which might not be readable by the UIProcess. Why is it that PSON (and only PSON) forgets the appropriate sandbox extension path?
Comment 12 Geoffrey Garen 2019-06-18 11:44:59 PDT
(The reason I ask is that it's not clear that requesting access to the URL parent folder is the right policy in all cases. Our API allows our client to specify any folder for read access. If our client specified some folder that is not the URL parent folder, we need to honor that.)
Comment 13 youenn fablet 2019-06-18 12:54:22 PDT
> > Source/WebKit/ChangeLog:10 > > + When doing process swapping, a file based loading may end up being loaded through loadRequestWithNavigationShared. > > + In that case, the sandbox extension path is set to '/' which might not be readable by the UIProcess. > > Why is it that PSON (and only PSON) forgets the appropriate sandbox > extension path? PSON makes the issue more visible but this is probably a pre-existing issue. For instance in case of web process crash, we will not go through loadFile:, so will not provide the expected sandbox extension. A full fix should be to remember the appropriate sandbox extension path. > (The reason I ask is that it's not clear that requesting access to the URL > parent folder is the right policy in all cases. Our API allows our client to > specify any folder for read access. If our client specified some folder that > is not the URL parent folder, we need to honor that.) Agreed, the current patch seems good enough to me for the common case. I can have a go at a full fix if useful.
Comment 14 Geoffrey Garen 2019-06-19 10:57:49 PDT
> Agreed, the current patch seems good enough to me for the common case. > I can have a go at a full fix if useful. Let's try the full fix. I recently learned about all the special rules Safari uses to compute the "allowingReadAccessToURL" URL, and they were super surprising to me. And the fact that this is a regression even though it's been our behavior for months is also surprising. It would be nice to land on a design that avoids future surprises, and honors the stated meaning of the API.
Comment 18 EWS Watchlist 2019-06-20 11:47:08 PDT
Attachment 372574 [details] did not pass style-queue: ERROR: Source/WebKit/UIProcess/WebPageProxy.h:1901: The parameter name "url" adds no information, so it should be removed. [readability/parameter_name]  Total errors found: 1 in 12 files If any of these errors are false positives, please file a bug against check-webkit-style.
Comment 19 youenn fablet 2019-06-21 08:58:30 PDT
Patch ready for review. This patch does not tackle the following things: Resource directory could be saved/restored when saving/restoring state. The selection of a resource directory when adding a history item from WebProcess is racy.
Comment 21 WebKit Commit Bot 2019-06-21 13:37:03 PDT
Comment on attachment 372574 [details] Patch Clearing flags on attachment: 372574 Committed r246694: <https://trac.webkit.org/changeset/246694>
Comment 22 WebKit Commit Bot 2019-06-21 13:37:05 PDT
All reviewed patches have been landed. Closing bug.