Bug 198902 - WebPageProxy should use the right path for sandbox extension
Summary: WebPageProxy should use the right path for sandbox extension
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
Depends on:
Blocks:
 
Reported: 2019-06-15 21:06 PDT by youenn fablet
Modified: 2019-07-09 09:39 PDT (History)
10 users (show)

See Also:


Attachments
Patch (1.79 KB, patch)
2019-06-15 21:11 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Archive of layout-test-results from ews104 for mac-highsierra-wk2 (487.30 KB, application/zip)
2019-06-15 22:01 PDT, EWS Watchlist
no flags Details
Archive of layout-test-results from ews124 for ios-simulator-wk2 (2.69 MB, application/zip)
2019-06-15 23:08 PDT, EWS Watchlist
no flags Details
Patch (7.61 KB, patch)
2019-06-17 14:32 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (7.51 KB, patch)
2019-06-18 08:35 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (7.54 KB, patch)
2019-06-18 10:44 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (12.66 KB, patch)
2019-06-19 19:09 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (19.16 KB, patch)
2019-06-20 08:04 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (19.04 KB, patch)
2019-06-20 11:42 PDT, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description youenn fablet 2019-06-15 21:06:44 PDT
WebPageProxy should use the right path for sandbox extension
Comment 1 youenn fablet 2019-06-15 21:09:49 PDT
<rdar://problem/50772810>
Comment 2 youenn fablet 2019-06-15 21:11:54 PDT
Created attachment 372216 [details]
Patch
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 8 youenn fablet 2019-06-17 14:32:24 PDT
Created attachment 372275 [details]
Patch
Comment 9 youenn fablet 2019-06-18 08:35:59 PDT
Created attachment 372340 [details]
Patch
Comment 10 youenn fablet 2019-06-18 10:44:39 PDT
Created attachment 372350 [details]
Patch
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 15 youenn fablet 2019-06-19 19:09:28 PDT
Created attachment 372513 [details]
Patch
Comment 16 youenn fablet 2019-06-20 08:04:56 PDT
Created attachment 372556 [details]
Patch
Comment 17 youenn fablet 2019-06-20 11:42:02 PDT
Created attachment 372574 [details]
Patch
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] [5]
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 20 Geoffrey Garen 2019-06-21 13:20:42 PDT
Comment on attachment 372574 [details]
Patch

r=me
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.