Use PlatformStrategies to switch between WebKit and WebKitLegacy checking of CSP frame-ancestors and X-Frame-Options
https://bugs.webkit.org/show_bug.cgi?id=185412
Reviewed by Ryosuke Niwa.
Source/WebCore:
Consolidate the knowledge on how to determine whether security checks were performed on a ResourceResponse
into LoaderStrategy::havePerformedSecurityChecks() (default implementation returns false) and query it
to determine whether CSP frame-ancestors and X-Frame-Options need to be checked for a ResourceResponse.
Additionally, rename LoaderStrategy::isDoingLoadingSecurityChecks() to shouldPerformSecurityChecks()
for consistency with havePerformedSecurityChecks(). Querying shouldPerformSecurityChecks() answers the
question of whether the loader strategy is responsible for performing security checks when building up
a ResourceRequest to have the loader strategy load. And LoaderStrategy::havePerformedSecurityChecks()
is used to determine whether the loader strategy performed these security checks for a given ResourceResponse.
* inspector/agents/InspectorNetworkAgent.cpp:
(WebCore::InspectorNetworkAgent::didReceiveResponse):
(WebCore::InspectorNetworkAgent::didFinishLoading):
(WebCore::isResponseProbablyComingFromNetworkProcess): Deleted.
* loader/DocumentLoader.cpp:
(WebCore::DocumentLoader::responseReceived):
* loader/DocumentThreadableLoader.cpp:
(WebCore::shouldPerformSecurityChecks):
(WebCore::DocumentThreadableLoader::shouldSetHTTPHeadersToKeep const):
(WebCore::DocumentThreadableLoader::makeCrossOriginAccessRequest):
(WebCore::DocumentThreadableLoader::makeSimpleCrossOriginAccessRequest):
(WebCore::DocumentThreadableLoader::redirectReceived):
(WebCore::DocumentThreadableLoader::didFail):
(WebCore::DocumentThreadableLoader::loadRequest):
(WebCore::isDoingSecurityChecksInNetworkProcess): Deleted.
(WebCore::isResponseComingFromNetworkProcess): Deleted.
* loader/LoaderStrategy.cpp:
* loader/LoaderStrategy.h:
* page/Settings.yaml: Remove setting networkProcessCSPFrameAncestorsCheckingEnabled as we now make
use of the loader strategy to determine whether to perform CSP frame-ancestors and X-Frame-Options
checking in DocumentLoader.
* platform/network/ResourceResponseBase.h:
(WebCore::ResourceResponseBase::setSource): Added an ASSERT to catch the programming error of setting
source to ResourceResponse::Source::Unknown. This source type represents an uninitialized ResourceResponse.
Source/WebKit:
Update code for renaming and write in terms of WebLoaderStrategy::shouldPerformSecurityChecks()
instead of explicitly querying RuntimeEnabledFeatures::sharedFeatures().restrictedHTTPResponseAccess().
* WebProcess/Network/WebLoaderStrategy.cpp:
(WebKit::WebLoaderStrategy::scheduleLoadFromNetworkProcess):
(WebKit::WebLoaderStrategy::loadResourceSynchronously):
(WebKit::WebLoaderStrategy::startPingLoad):
(WebKit::WebLoaderStrategy::preconnectTo):
(WebKit::WebLoaderStrategy::shouldPerformSecurityChecks const):
(WebKit::WebLoaderStrategy::havePerformedSecurityChecks const):
(WebKit::WebLoaderStrategy::isDoingLoadingSecurityChecks const): Deleted.
* WebProcess/Network/WebLoaderStrategy.h:
* WebProcess/WebPage/WebPage.cpp:
git-svn-id: http://svn.webkit.org/repository/webkit/trunk@231692 268f45cc-cd09-0410-ab3c-d52691b4dbfc
diff --git a/Source/WebCore/loader/DocumentThreadableLoader.cpp b/Source/WebCore/loader/DocumentThreadableLoader.cpp
index 79ae616..7e1b0d8 100644
--- a/Source/WebCore/loader/DocumentThreadableLoader.cpp
+++ b/Source/WebCore/loader/DocumentThreadableLoader.cpp
@@ -89,14 +89,14 @@
return create(document, client, WTFMove(request), options, nullptr, nullptr, WTFMove(referrer), ShouldLogError::Yes);
}
-static inline bool isDoingSecurityChecksInNetworkProcess()
+static inline bool shouldPerformSecurityChecks()
{
- return platformStrategies()->loaderStrategy()->isDoingLoadingSecurityChecks();
+ return platformStrategies()->loaderStrategy()->shouldPerformSecurityChecks();
}
bool DocumentThreadableLoader::shouldSetHTTPHeadersToKeep() const
{
- if (m_options.mode == FetchOptions::Mode::Cors && isDoingSecurityChecksInNetworkProcess())
+ if (m_options.mode == FetchOptions::Mode::Cors && shouldPerformSecurityChecks())
return true;
#if ENABLE(SERVICE_WORKER)
@@ -179,7 +179,7 @@
{
ASSERT(m_options.mode == FetchOptions::Mode::Cors);
- if ((m_options.preflightPolicy == PreflightPolicy::Consider && isSimpleCrossOriginAccessRequest(request.httpMethod(), request.httpHeaderFields())) || m_options.preflightPolicy == PreflightPolicy::Prevent || isDoingSecurityChecksInNetworkProcess()) {
+ if ((m_options.preflightPolicy == PreflightPolicy::Consider && isSimpleCrossOriginAccessRequest(request.httpMethod(), request.httpHeaderFields())) || m_options.preflightPolicy == PreflightPolicy::Prevent || shouldPerformSecurityChecks()) {
if (checkURLSchemeAsCORSEnabled(request.url()))
makeSimpleCrossOriginAccessRequest(WTFMove(request));
} else {
@@ -207,8 +207,8 @@
void DocumentThreadableLoader::makeSimpleCrossOriginAccessRequest(ResourceRequest&& request)
{
- ASSERT(m_options.preflightPolicy != PreflightPolicy::Force || isDoingSecurityChecksInNetworkProcess());
- ASSERT(m_options.preflightPolicy == PreflightPolicy::Prevent || isSimpleCrossOriginAccessRequest(request.httpMethod(), request.httpHeaderFields()) || isDoingSecurityChecksInNetworkProcess());
+ ASSERT(m_options.preflightPolicy != PreflightPolicy::Force || shouldPerformSecurityChecks());
+ ASSERT(m_options.preflightPolicy == PreflightPolicy::Prevent || isSimpleCrossOriginAccessRequest(request.httpMethod(), request.httpHeaderFields()) || shouldPerformSecurityChecks());
updateRequestForAccessControl(request, securityOrigin(), m_options.storedCredentialsPolicy);
loadRequest(WTFMove(request), DoSecurityCheck);
@@ -266,12 +266,6 @@
m_preflightChecker = std::nullopt;
}
-static inline bool isResponseComingFromNetworkProcess(const ResourceResponse& response)
-{
- auto source = response.source();
- return source == ResourceResponse::Source::Network || source == ResourceResponse::Source::DiskCache || source == ResourceResponse::Source::DiskCacheAfterValidation;
-}
-
void DocumentThreadableLoader::redirectReceived(CachedResource& resource, ResourceRequest&& request, const ResourceResponse& redirectResponse, CompletionHandler<void(ResourceRequest&&)>&& completionHandler)
{
ASSERT(m_client);
@@ -295,7 +289,7 @@
return completionHandler(WTFMove(request));
}
- if (isDoingSecurityChecksInNetworkProcess() && isResponseComingFromNetworkProcess(redirectResponse)) {
+ if (platformStrategies()->loaderStrategy()->havePerformedSecurityChecks(redirectResponse)) {
completionHandler(WTFMove(request));
return;
}
@@ -470,7 +464,7 @@
// NetworkProcess might return a CSP violation as an AccessControl error in case of redirection.
// Let's recheck CSP to generate the report if needed.
// FIXME: We should introduce an error dedicated to CSP violation.
- if (isDoingSecurityChecksInNetworkProcess() && error.isAccessControl() && error.failingURL().protocolIsInHTTPFamily() && !isAllowedByContentSecurityPolicy(error.failingURL(), ContentSecurityPolicy::RedirectResponseReceived::Yes)) {
+ if (shouldPerformSecurityChecks() && error.isAccessControl() && error.failingURL().protocolIsInHTTPFamily() && !isAllowedByContentSecurityPolicy(error.failingURL(), ContentSecurityPolicy::RedirectResponseReceived::Yes)) {
reportContentSecurityPolicyError(m_resource->resourceRequest().url());
return;
}
@@ -579,7 +573,7 @@
return;
}
- if (!isDoingSecurityChecksInNetworkProcess()) {
+ if (!shouldPerformSecurityChecks()) {
// FIXME: FrameLoader::loadSynchronously() does not tell us whether a redirect happened or not, so we guess by comparing the
// request and response URLs. This isn't a perfect test though, since a server can serve a redirect to the same URL that was
// requested. Also comparing the request and response URLs as strings will fail if the requestURL still has its credentials.
