Bug 230812 – Move Cross-Origin-Opener-Policy handling to the NetworkProcess

• Component: WebKit2
• Status: RESOLVED FIXED

[Chris Dumez]

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.

[Chris Dumez]

<rdar://83504842>

[Chris Dumez]

Created attachment 439307 [details]
WIP patch

[EWS Watchlist]

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

[Chris Dumez]

Created attachment 439356 [details]
WIP patch

[Chris Dumez]

Created attachment 439365 [details]
Patch

[youenn fablet]

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&

[Chris Dumez]

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.

[Chris Dumez]

Created attachment 439475 [details]
Patch

[Chris Dumez]

Created attachment 439476 [details]
Patch

[EWS]

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