Bug 230812 - Move Cross-Origin-Opener-Policy handling to the NetworkProcess
Summary: Move Cross-Origin-Opener-Policy handling to the NetworkProcess
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: WebKit2 (show other bugs)
Version: WebKit Nightly Build
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: Chris Dumez
URL:
Keywords: InRadar
Depends on:
Blocks: 228755
  Show dependency treegraph
 
Reported: 2021-09-26 19:51 PDT by Chris Dumez
Modified: 2021-09-28 10:21 PDT (History)
19 users (show)

See Also:


Attachments
WIP patch (131.62 KB, patch)
2021-09-26 20:07 PDT, Chris Dumez
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
WIP patch (141.65 KB, patch)
2021-09-27 08:24 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (158.27 KB, patch)
2021-09-27 09:45 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (162.22 KB, patch)
2021-09-28 07:58 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff
Patch (162.09 KB, patch)
2021-09-28 08:12 PDT, Chris Dumez
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Chris Dumez 2021-09-26 19:51:12 PDT
Move Cross-Origin-Opener-Policy handling to from the WebContent process to the NetworkProcess. This is safer since the WebContent process is not a trusted process and it avoids having to send the ResourceResponse to the WebProcess in order to trigger cross-origin isolation.
Comment 1 Chris Dumez 2021-09-26 19:51:27 PDT
<rdar://83504842>
Comment 2 Chris Dumez 2021-09-26 20:07:57 PDT
Created attachment 439307 [details]
WIP patch
Comment 3 EWS Watchlist 2021-09-26 20:08:52 PDT
This patch modifies the imported WPT tests. Please ensure that any changes on the tests (not coming from a WPT import) are exported to WPT. Please see https://trac.webkit.org/wiki/WPTExportProcess
Comment 4 Chris Dumez 2021-09-27 08:24:36 PDT
Created attachment 439356 [details]
WIP patch
Comment 5 Chris Dumez 2021-09-27 09:45:44 PDT
Created attachment 439365 [details]
Patch
Comment 6 youenn fablet 2021-09-28 00:35:41 PDT
Comment on attachment 439365 [details]
Patch

LGTM with some nits below.
I would just make sure to cover all code paths (service worker, cache) now that we are moving checks to the network process.

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

> Source/WebCore/ChangeLog:28
> +        context group switch but currently do not involved the network process. I also had to add

s/involved/involve.

> Source/WebCore/loader/CrossOriginOpenerPolicy.cpp:239
> +// https://html.spec.whatwg.org/multipage/browsing-the-web.html#process-a-navigate-fetch (Step 12.5.6)

13.5.6?

> Source/WebCore/loader/DocumentLoader.cpp:745
>  // https://html.spec.whatwg.org/multipage/browsing-the-web.html#process-a-navigate-fetch (Step 12.5.6)

13.5.6?

> Source/WebCore/loader/NavigationRequester.h:81
> +    return NavigationRequester { WTFMove(*url), securityOrigin.releaseNonNull(), topOrigin.releaseNonNull(), WTFMove(*crossOriginOpenerPolicy), WTFMove(*globalFrameIdentifier) };

Probably do not need move for the last two.

> Source/WebCore/loader/ReportingEndpointsCache.cpp:70
> +    return addEndPointsFromReportToHeader(response.url(), response.httpHeaderField(HTTPHeaderName::ReportTo));

s/EndPoints/Endpoints/

> Source/WebCore/loader/ReportingEndpointsCache.cpp:73
> +void ReportingEndpointsCache::addEndPointsFromReportToHeader(const URL& responseURL, const String& reportToHeaderValue)

Ditto.

> Source/WebKit/NetworkProcess/NetworkResourceLoadParameters.cpp:326
> +    result.openerURL = *openerURL;

Could use a move

> Source/WebKit/NetworkProcess/NetworkResourceLoadParameters.h:77
> +    uint64_t navigationID { 0 };

Should we use an ObjectIdentifier?

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:1012
> +        didFailLoading(ResourceError { errorDomainWebKitInternal, 0, redirectRequest.url(), "Redirection was blocked by Cross-Origin-Opener-Policy"_s, ResourceError::Type::AccessControl });

It would be nice to have doCrossOriginOpenerHandlingOfResponse return an optional<ResourceError>.
Do we have a test case for this one?

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:1324
> +        didFailLoading(ResourceError { errorDomainWebKitInternal, 0, response.url(), "Navigation was blocked by Cross-Origin-Opener-Policy"_s, ResourceError::Type::AccessControl });

return missing.
It might be good to add a test case.

> Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp:163
> +    if (!m_loader.doCrossOriginOpenerHandlingOfResponse(response)) {

Do we have tests for that code path?

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:439
> +void NetworkProcessProxy::triggerBrowsingContextGroupSwitchForNavigation(WebPageProxyIdentifier pageID, uint64_t navigationID, BrowsingContextGroupSwitchDecision browsingContextGroupSwitchDecision, WebCore::RegistrableDomain responseDomain, NetworkResourceLoadIdentifier existingNetworkResourceLoadIdentifierToResume)

s/RegistrationDomain/const RegistrationDomain& or RegistrationDomain&&

> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:443
> +        page->triggerBrowsingContextGroupSwitchForNavigation(navigationID, browsingContextGroupSwitchDecision, responseDomain, existingNetworkResourceLoadIdentifierToResume);

It would be nice to have a 'cancel' switch for navigation that would just clear the entry from the pending switch loader map in network process.
As an async callback for instance. Not sure we can make it 100% working though

> Source/WebKit/UIProcess/WebPageProxy.cpp:5676
> +void WebPageProxy::triggerBrowsingContextGroupSwitchForNavigation(uint64_t navigationID, BrowsingContextGroupSwitchDecision browsingContextGroupSwitchDecision, WebCore::RegistrableDomain responseDomain, NetworkResourceLoadIdentifier existingNetworkResourceLoadIdentifierToResume)

const WebCore::RegistrableDomain&
Comment 7 Chris Dumez 2021-09-28 07:29:43 PDT
Comment on attachment 439365 [details]
Patch

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

>> Source/WebCore/loader/CrossOriginOpenerPolicy.cpp:239
>> +// https://html.spec.whatwg.org/multipage/browsing-the-web.html#process-a-navigate-fetch (Step 12.5.6)
> 
> 13.5.6?

Indeed, I guess the spec got updated.

>> Source/WebCore/loader/NavigationRequester.h:81
>> +    return NavigationRequester { WTFMove(*url), securityOrigin.releaseNonNull(), topOrigin.releaseNonNull(), WTFMove(*crossOriginOpenerPolicy), WTFMove(*globalFrameIdentifier) };
> 
> Probably do not need move for the last two.

CrossOriginOpenerPolicy is a struct containing Strings so I think it is useful. I agree for globalFrameIdentifier though.

>> Source/WebCore/loader/ReportingEndpointsCache.cpp:70
>> +    return addEndPointsFromReportToHeader(response.url(), response.httpHeaderField(HTTPHeaderName::ReportTo));
> 
> s/EndPoints/Endpoints/

Ah yes, I keep making this mistake.

>> Source/WebKit/NetworkProcess/NetworkResourceLoadParameters.cpp:326
>> +    result.openerURL = *openerURL;
> 
> Could use a move

Agreed.

>> Source/WebKit/NetworkProcess/NetworkResourceLoadParameters.h:77
>> +    uint64_t navigationID { 0 };
> 
> Should we use an ObjectIdentifier?

Well yes but probably not in this patch. NavigationID is currently typed as a uint64_t in WebKit.

>> Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp:163
>> +    if (!m_loader.doCrossOriginOpenerHandlingOfResponse(response)) {
> 
> Do we have tests for that code path?

Definitely, that's how I found out I was missing it in an earlier patch iteration (both here and in the cached resource case, which I had not thought about initially).

>> Source/WebKit/UIProcess/Network/NetworkProcessProxy.cpp:443
>> +        page->triggerBrowsingContextGroupSwitchForNavigation(navigationID, browsingContextGroupSwitchDecision, responseDomain, existingNetworkResourceLoadIdentifierToResume);
> 
> It would be nice to have a 'cancel' switch for navigation that would just clear the entry from the pending switch loader map in network process.
> As an async callback for instance. Not sure we can make it 100% working though

If I understand the request correctly, maybe we could add a CompletionHandler<void(bool success)> parameter and have the NetworkProcess destroy the cached loader when success == false?
Currently, it would naturally happen on a timer but I guess it is good to be pro-active.

>> Source/WebKit/UIProcess/WebPageProxy.cpp:5676
>> +void WebPageProxy::triggerBrowsingContextGroupSwitchForNavigation(uint64_t navigationID, BrowsingContextGroupSwitchDecision browsingContextGroupSwitchDecision, WebCore::RegistrableDomain responseDomain, NetworkResourceLoadIdentifier existingNetworkResourceLoadIdentifierToResume)
> 
> const WebCore::RegistrableDomain&

Indeed.
Comment 8 Chris Dumez 2021-09-28 07:58:04 PDT
Created attachment 439475 [details]
Patch
Comment 9 Chris Dumez 2021-09-28 08:12:10 PDT
Created attachment 439476 [details]
Patch
Comment 10 EWS 2021-09-28 10:21:24 PDT
Committed r283179 (242227@main): <https://commits.webkit.org/242227@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 439476 [details].