Bug 228754 – Add initial support for Cross-Origin-Embedder-Policy (COEP)

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Chris Dumez]

Add initial support for Cross-Origin-Embedder-Policy (COEP):
- https://html.spec.whatwg.org/multipage/origin.html#coep

[Chris Dumez]

Created attachment 434953 [details]
WIP patch

[Chris Dumez]

Created attachment 434987 [details]
WIP patch

[Chris Dumez]

Created attachment 434991 [details]
WIP patch

[Chris Dumez]

Created attachment 435012 [details]
WIP patch

[Chris Dumez]

Created attachment 435013 [details]
WIP patch

[Chris Dumez]

Created attachment 435034 [details]
Patch

[Chris Dumez]

Created attachment 435050 [details]
Patch

[Chris Dumez]

Created attachment 435054 [details]
Patch

[Radar WebKit Bug Importer]

<rdar://problem/81715011>

[Darin Adler]

Comment on attachment 435054 [details]
Patch

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

> Source/WebCore/loader/CrossOriginAccessControl.h:90
> +WEBCORE_EXPORT std::optional<ResourceError> validateCrossOriginResourcePolicy(const CrossOriginEmbedderPolicyValue&, const SecurityOrigin&, const URL&, const ResourceResponse&, ForNavigation);

A small scalar like CrossOriginEmbedderPolicyValue should just be passed by value; no reason to pass a const&.

[Chris Dumez]

Created attachment 435232 [details]
Patch

[Chris Dumez]

(In reply to Darin Adler from comment #10)
> Comment on attachment 435054 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=435054&action=review
> 
> > Source/WebCore/loader/CrossOriginAccessControl.h:90
> > +WEBCORE_EXPORT std::optional<ResourceError> validateCrossOriginResourcePolicy(const CrossOriginEmbedderPolicyValue&, const SecurityOrigin&, const URL&, const ResourceResponse&, ForNavigation);
> 
> A small scalar like CrossOriginEmbedderPolicyValue should just be passed by
> value; no reason to pass a const&.

Fixed, thanks. I was passing a CrossOriginEmbedderPolicy in an earlier iteration and failed to make this change when I started passing a CrossOriginEmbedderPolicyValue instead.

[Alex Christensen]

Comment on attachment 435232 [details]
Patch

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

> Source/WebCore/Modules/cache/DOMCacheEngine.h:51
> +    CORP

You may as well reduce the size of this enum class by adding : uint8_t.

> Source/WebCore/platform/network/HTTPParsers.h:64
>  enum class CrossOriginResourcePolicy {

ditto

> Source/WebCore/workers/service/server/RegistrationDatabase.cpp:53
> +static const uint64_t schemaVersion = 7;

Do we already have code that migrates old schemas or erases the old files?

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:597
> +            String errorMessage = "Refused to display '" + response.url().stringCenterEllipsizedToLength() + "' in a frame because of Cross-Origin-Embedder-Policy.";

I feel like I've been told that makeString is more efficient than repeated operator+'s, but I'm not sure if that's still true.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:613
> +            String errorMessage = "Refused to load '" + response.url().stringCenterEllipsizedToLength() + "' worker because of Cross-Origin-Embedder-Policy.";

ditto.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:668
> +            if (protectedThis->m_networkLoad)

I don't understand what this check accomplishes.  I see it used before one other call site of didFailLoading but it doesn't seem necessary here.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:669
> +                protectedThis->didFailLoading(ResourceError { String { }, 0, url, "Worker load was blocked by Cross-Origin-Embedder-Policy"_s, ResourceError::Type::AccessControl });

Although there are a few other places that use the empty string as a domain, I think it would be nicer to use errorDomainWebKitInternal instead.

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

Ditto.   Error needs a real domain.

> Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp:150
> +            if (auto error = validateCrossOriginResourcePolicy(m_loader.parameters().parentCrossOriginEmbedderPolicy.value, *parentOrigin, m_currentRequest.url(), response, ForNavigation::Yes)) {

You have several added calls to validateCrossOriginResourcePolicy which seem like they could change behavior even if COEP is off.  Can they indeed cause errors that weren't there before?

[Chris Dumez]

(In reply to Alex Christensen from comment #13)
> Comment on attachment 435232 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=435232&action=review
> 
> > Source/WebCore/Modules/cache/DOMCacheEngine.h:51
> > +    CORP
> 
> You may as well reduce the size of this enum class by adding : uint8_t.
> 
> > Source/WebCore/platform/network/HTTPParsers.h:64
> >  enum class CrossOriginResourcePolicy {
> 
> ditto
> 
> > Source/WebCore/workers/service/server/RegistrationDatabase.cpp:53
> > +static const uint64_t schemaVersion = 7;
> 
> Do we already have code that migrates old schemas or erases the old files?

There is already code that erases files for old schemas.

> 
> > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:597
> > +            String errorMessage = "Refused to display '" + response.url().stringCenterEllipsizedToLength() + "' in a frame because of Cross-Origin-Embedder-Policy.";
> 
> I feel like I've been told that makeString is more efficient than repeated
> operator+'s, but I'm not sure if that's still true.

Will switch.

> 
> > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:613
> > +            String errorMessage = "Refused to load '" + response.url().stringCenterEllipsizedToLength() + "' worker because of Cross-Origin-Embedder-Policy.";
> 
> ditto.

OK.

> 
> > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:668
> > +            if (protectedThis->m_networkLoad)
> 
> I don't understand what this check accomplishes.  I see it used before one
> other call site of didFailLoading but it doesn't seem necessary here.

I copied the logic from the other site. It is entirely possible this isn't necessary. I'll double check.

> 
> > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:669
> > +                protectedThis->didFailLoading(ResourceError { String { }, 0, url, "Worker load was blocked by Cross-Origin-Embedder-Policy"_s, ResourceError::Type::AccessControl });
> 
> Although there are a few other places that use the empty string as a domain,
> I think it would be nicer to use errorDomainWebKitInternal instead.
> 
> > Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:883
> > +        this->didFailLoading(ResourceError { String { }, 0, redirectRequest.url(), "Redirection was blocked by Cross-Origin-Embedder-Policy"_s, ResourceError::Type::AccessControl });
> 
> Ditto.   Error needs a real domain.

Will Fix.

> 
> > Source/WebKit/NetworkProcess/ServiceWorker/ServiceWorkerFetchTask.cpp:150
> > +            if (auto error = validateCrossOriginResourcePolicy(m_loader.parameters().parentCrossOriginEmbedderPolicy.value, *parentOrigin, m_currentRequest.url(), response, ForNavigation::Yes)) {
> 
> You have several added calls to validateCrossOriginResourcePolicy which seem
> like they could change behavior even if COEP is off.  Can they indeed cause
> errors that weren't there before?

No, I do not think it is possible for COEP to cause errors in the network process when the feature flag is off in WebCore. All the checks in the network process rely on the COEP of the current document (or parent document), which are provided by WebCore. obtainCrossOriginEmbedderPolicy() in WebCore has an early return when the feature flag is disabled and so COEP for the current (or parent document) will always be the default (unsafe-none) when the feature flag is off.

[Chris Dumez]

Comment on attachment 435232 [details]
Patch

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

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:594
> +    if (m_parameters.parentCrossOriginEmbedderPolicy.value != WebCore::CrossOriginEmbedderPolicyValue::UnsafeNone && m_parameters.sourceOrigin) {

Here m_parameters.parentCrossOriginEmbedderPolicy.value would be WebCore::CrossOriginEmbedderPolicyValue::UnsafeNone if the feature was disabled at runtime and therefore this function would be a no-op.

> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:610
> +    if (m_parameters.crossOriginEmbedderPolicy.value != WebCore::CrossOriginEmbedderPolicyValue::UnsafeNone && m_parameters.sourceOrigin) {

Here m_parameters.crossOriginEmbedderPolicy.value would be WebCore::CrossOriginEmbedderPolicyValue::UnsafeNone if the feature was disabled at runtime and therefore this function would be a no-op.

[Chris Dumez]

Created attachment 435380 [details]
Patch

[Alex Christensen]

Comment on attachment 435380 [details]
Patch

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

> Source/WebCore/loader/CrossOriginEmbedderPolicy.h:99
> +template<> struct EnumTraits<WebCore::CrossOriginEmbedderPolicyValue> {

This is unnecessary because it uses bool as its storage, and the decoders already know that there are only 2 valid values

[Chris Dumez]

(In reply to Alex Christensen from comment #17)
> Comment on attachment 435380 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=435380&action=review
> 
> > Source/WebCore/loader/CrossOriginEmbedderPolicy.h:99
> > +template<> struct EnumTraits<WebCore::CrossOriginEmbedderPolicyValue> {
> 
> This is unnecessary because it uses bool as its storage, and the decoders
> already know that there are only 2 valid values

Good to know, will fix. I had followed the pattern for COOP but COOP has more enum values.

Thanks for reviewing.

[Chris Dumez]

Created attachment 435383 [details]
Patch

[Darin Adler]

Comment on attachment 435232 [details]
Patch

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

>>> Source/WebKit/NetworkProcess/NetworkResourceLoader.cpp:597
>>> +            String errorMessage = "Refused to display '" + response.url().stringCenterEllipsizedToLength() + "' in a frame because of Cross-Origin-Embedder-Policy.";
>> 
>> I feel like I've been told that makeString is more efficient than repeated operator+'s, but I'm not sure if that's still true.
> 
> Will switch.

A call to makeString is more flexible than repeated operator+, since it can do things like format numbers without creating a temporary string, but for cases that both can handle, the efficiency should be exactly the same since operator+ does gather all the strings and then builds the result string only at the end.

I’d like to eventually deprecate operator+, though.

[Chris Dumez]

Created attachment 435390 [details]
Patch

[EWS]

Committed r280953 (240459@main): <https://commits.webkit.org/240459@main>

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