Bug 198902 – WebPageProxy should use the right path for sandbox extension

• Component: Page Loading
• Status: RESOLVED FIXED

[youenn fablet]

WebPageProxy should use the right path for sandbox extension

[youenn fablet]

<rdar://problem/50772810>

[youenn fablet]

Created attachment 372216 [details]
Patch

[EWS Watchlist]

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.

[EWS Watchlist]

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

[EWS Watchlist]

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

[EWS Watchlist]

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

[Alexey Proskuryakov]

How will be subresources loaded after going through this path?

[youenn fablet]

Created attachment 372275 [details]
Patch

[youenn fablet]

Created attachment 372340 [details]
Patch

[youenn fablet]

Created attachment 372350 [details]
Patch

[Geoffrey Garen]

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?

[Geoffrey Garen]

(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.)

[youenn fablet]

> > 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.

[Geoffrey Garen]

> 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.

[youenn fablet]

Created attachment 372513 [details]
Patch

[youenn fablet]

Created attachment 372556 [details]
Patch

[youenn fablet]

Created attachment 372574 [details]
Patch

[EWS Watchlist]

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.

[youenn fablet]

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.

[Geoffrey Garen]

Comment on attachment 372574 [details]
Patch

r=me

[WebKit Commit Bot]

Comment on attachment 372574 [details]
Patch

Clearing flags on attachment: 372574

Committed r246694: <https://trac.webkit.org/changeset/246694>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.