Add initial support for Cross-Origin-Embedder-Policy (COEP): - https://html.spec.whatwg.org/multipage/origin.html#coep
Created attachment 434953 [details] WIP patch
Created attachment 434987 [details] WIP patch
Created attachment 434991 [details] WIP patch
Created attachment 435012 [details] WIP patch
Created attachment 435013 [details] WIP patch
Created attachment 435034 [details] Patch
Created attachment 435050 [details] Patch
Created attachment 435054 [details] Patch
<rdar://problem/81715011>
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&.
Created attachment 435232 [details] Patch
(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.
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?
(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.
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.
Created attachment 435380 [details] Patch
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
(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.
Created attachment 435383 [details] Patch
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.
Created attachment 435390 [details] Patch
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].