CSP violation reports should bypass CSP checks
authoryouenn@apple.com <youenn@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 4 Jan 2019 21:22:30 +0000 (21:22 +0000)
committeryouenn@apple.com <youenn@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Fri, 4 Jan 2019 21:22:30 +0000 (21:22 +0000)
https://bugs.webkit.org/show_bug.cgi?id=192857
<rdar://problem/46887236>

Reviewed by Chris Dumez.

Source/WebCore:

For ping loads, pass the option to do CSP checks from PingLoader to LoaderStrategy.
This new option is unused by WebKit Legacy.
It is used by WebKit loader strategy to only send any CSP response header to network process
in case CSP checks should be done.

This option is used to disable CSP checks for Ping Loads that report CSP violations.

Test: http/wpt/fetch/csp-reports-bypass-csp-checks.html

* loader/LoaderStrategy.h:
* loader/PingLoader.cpp:
(WebCore::PingLoader::loadImage):
(WebCore::PingLoader::sendPing):
(WebCore::PingLoader::sendViolationReport):
(WebCore::PingLoader::startPingLoad):
* loader/PingLoader.h:
* loader/cache/CachedResource.cpp:
(WebCore::CachedResource::load):

Source/WebKit:

* WebProcess/Network/WebLoaderStrategy.cpp:
(WebKit::WebLoaderStrategy::startPingLoad):
* WebProcess/Network/WebLoaderStrategy.h:

Source/WebKitLegacy:

* WebCoreSupport/WebResourceLoadScheduler.cpp:
(WebResourceLoadScheduler::startPingLoad):
* WebCoreSupport/WebResourceLoadScheduler.h:

LayoutTests:

* http/wpt/fetch/csp-reports-bypass-csp-checks-expected.txt: Added.
* http/wpt/fetch/csp-reports-bypass-csp-checks.html: Added.
* http/wpt/fetch/csp-reports-bypass-csp-checks.html.headers: Added.
* http/wpt/fetch/resources/store-csp-report.py: Added.
(main):

git-svn-id: https://svn.webkit.org/repository/webkit/trunk@239634 268f45cc-cd09-0410-ab3c-d52691b4dbfc

16 files changed:
LayoutTests/ChangeLog
LayoutTests/http/wpt/fetch/csp-reports-bypass-csp-checks-expected.txt [new file with mode: 0644]
LayoutTests/http/wpt/fetch/csp-reports-bypass-csp-checks.html [new file with mode: 0644]
LayoutTests/http/wpt/fetch/csp-reports-bypass-csp-checks.html.headers [new file with mode: 0644]
LayoutTests/http/wpt/fetch/resources/store-csp-report.py [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/loader/LoaderStrategy.h
Source/WebCore/loader/PingLoader.cpp
Source/WebCore/loader/PingLoader.h
Source/WebCore/loader/cache/CachedResource.cpp
Source/WebKit/ChangeLog
Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp
Source/WebKit/WebProcess/Network/WebLoaderStrategy.h
Source/WebKitLegacy/ChangeLog
Source/WebKitLegacy/WebCoreSupport/WebResourceLoadScheduler.cpp
Source/WebKitLegacy/WebCoreSupport/WebResourceLoadScheduler.h

index 6c934d6..7720269 100644 (file)
@@ -1,3 +1,17 @@
+2019-01-04  Youenn Fablet  <youenn@apple.com>
+
+        CSP violation reports should bypass CSP checks
+        https://bugs.webkit.org/show_bug.cgi?id=192857
+        <rdar://problem/46887236>
+
+        Reviewed by Chris Dumez.
+
+        * http/wpt/fetch/csp-reports-bypass-csp-checks-expected.txt: Added.
+        * http/wpt/fetch/csp-reports-bypass-csp-checks.html: Added.
+        * http/wpt/fetch/csp-reports-bypass-csp-checks.html.headers: Added.
+        * http/wpt/fetch/resources/store-csp-report.py: Added.
+        (main):
+
 2019-01-04  Chris Fleizach  <cfleizach@apple.com>
 
         AX: String check: "Rule" does not reflect the meaning of the <hr> html tag
diff --git a/LayoutTests/http/wpt/fetch/csp-reports-bypass-csp-checks-expected.txt b/LayoutTests/http/wpt/fetch/csp-reports-bypass-csp-checks-expected.txt
new file mode 100644 (file)
index 0000000..c408fd6
--- /dev/null
@@ -0,0 +1,4 @@
+CONSOLE MESSAGE: Refused to connect to http://localhost:8888/test because it does not appear in the connect-src directive of the Content Security Policy.
+
+PASS CSP violation reports should not be CSP checked 
+
diff --git a/LayoutTests/http/wpt/fetch/csp-reports-bypass-csp-checks.html b/LayoutTests/http/wpt/fetch/csp-reports-bypass-csp-checks.html
new file mode 100644 (file)
index 0000000..db52182
--- /dev/null
@@ -0,0 +1,20 @@
+<!DOCTYPE html>
+<script src=/resources/testharness.js></script>
+<script src=/resources/testharnessreport.js></script>
+<title>CSP violation reports should not be CSP checked</title>
+<body>
+<script>
+document.addEventListener("securitypolicyviolation", async () => {
+    const response = await fetch("/WebKit/fetch/resources/store-csp-report.py?retrieve=true&file=reports-bypass-csp-checks");
+    let results = await response.json();
+
+    const report = results["csp-report"];
+    if (report["effective-directive"] === "connect-src") {
+        assert_equals(report["blocked-uri"], "http://localhost:8888");
+        done();
+    }
+});
+
+fetch("http://localhost:8888/test").catch(() => {});
+</script>
+</body>
diff --git a/LayoutTests/http/wpt/fetch/csp-reports-bypass-csp-checks.html.headers b/LayoutTests/http/wpt/fetch/csp-reports-bypass-csp-checks.html.headers
new file mode 100644 (file)
index 0000000..bed7464
--- /dev/null
@@ -0,0 +1 @@
+Content-Security-Policy: connect-src 'self'; style-src 'self'; script-src 'self' 'unsafe-inline'; default-src 'none'; report-uri http://localhost:8801/WebKit/fetch/resources/store-csp-report.py?file=reports-bypass-csp-checks
diff --git a/LayoutTests/http/wpt/fetch/resources/store-csp-report.py b/LayoutTests/http/wpt/fetch/resources/store-csp-report.py
new file mode 100644 (file)
index 0000000..75ce221
--- /dev/null
@@ -0,0 +1,38 @@
+import hashlib
+
+def main(request, response):
+  ## Get the query parameter (key) from URL ##
+  ## Tests will record POST requests (CSP Report) and GET (rest) ##
+  if request.GET:
+    key = request.GET['file']
+  elif request.POST:
+    key = request.POST['file']
+
+  ## Convert the key from String to UUID valid String ##
+  testId = hashlib.md5(key).hexdigest()
+
+  ## Handle the header retrieval request ##
+  if 'retrieve' in request.GET:
+    response.writer.write_status(200)
+    response.headers.set("Access-Control-Allow-Origin", "*")
+    response.headers.set("Cache-Control", "no-cache, no-store, must-revalidate")
+    response.headers.set("Pragma", "no-cache")
+    response.headers.set("Expires", "0")
+    response.writer.end_headers()
+    try:
+      value = request.server.stash.take(testId)
+      response.writer.write(value)
+    except (KeyError, ValueError) as e:
+      response.writer.write("No report has been recorded " + str(e))
+      pass
+
+    response.close_connection = True
+    return
+
+  request.server.stash.put(testId, request.body)
+  response.headers.set("Access-Control-Allow-Origin", "*")
+  response.headers.set("Cache-Control", "no-cache, no-store, must-revalidate")
+  response.headers.set("Pragma", "no-cache")
+  response.headers.set("Expires", "0")
+  response.writer.end_headers()
+
index 7b30489..e53c249 100644 (file)
@@ -1,3 +1,30 @@
+2019-01-04  Youenn Fablet  <youenn@apple.com>
+
+        CSP violation reports should bypass CSP checks
+        https://bugs.webkit.org/show_bug.cgi?id=192857
+        <rdar://problem/46887236>
+
+        Reviewed by Chris Dumez.
+
+        For ping loads, pass the option to do CSP checks from PingLoader to LoaderStrategy.
+        This new option is unused by WebKit Legacy.
+        It is used by WebKit loader strategy to only send any CSP response header to network process
+        in case CSP checks should be done.
+
+        This option is used to disable CSP checks for Ping Loads that report CSP violations.
+
+        Test: http/wpt/fetch/csp-reports-bypass-csp-checks.html
+
+        * loader/LoaderStrategy.h:
+        * loader/PingLoader.cpp:
+        (WebCore::PingLoader::loadImage):
+        (WebCore::PingLoader::sendPing):
+        (WebCore::PingLoader::sendViolationReport):
+        (WebCore::PingLoader::startPingLoad):
+        * loader/PingLoader.h:
+        * loader/cache/CachedResource.cpp:
+        (WebCore::CachedResource::load):
+
 2019-01-04  Wenson Hsieh  <wenson_hsieh@apple.com>
 
         [Cocoa] Merge WebEditCommandProxy::nameForEditAction and undoNameForEditAction into a single function
index 09637c8..b35c6fb 100644 (file)
@@ -68,7 +68,7 @@ public:
     virtual void resumePendingRequests() = 0;
 
     using PingLoadCompletionHandler = WTF::Function<void(const ResourceError&, const ResourceResponse&)>;
-    virtual void startPingLoad(Frame&, ResourceRequest&, const HTTPHeaderMap& originalRequestHeaders, const FetchOptions&, PingLoadCompletionHandler&& = { }) = 0;
+    virtual void startPingLoad(Frame&, ResourceRequest&, const HTTPHeaderMap& originalRequestHeaders, const FetchOptions&, ContentSecurityPolicyImposition, PingLoadCompletionHandler&& = { }) = 0;
 
     using PreconnectCompletionHandler = WTF::Function<void(const ResourceError&)>;
     virtual void preconnectTo(FrameLoader&, const URL&, StoredCredentialsPolicy, PreconnectCompletionHandler&&) = 0;
index 134e556..2c0b2a3 100644 (file)
@@ -107,7 +107,7 @@ void PingLoader::loadImage(Frame& frame, const URL& url)
         request.setHTTPReferrer(referrer);
     frame.loader().addExtraFieldsToSubresourceRequest(request);
 
-    startPingLoad(frame, request, WTFMove(originalRequestHeader), ShouldFollowRedirects::Yes);
+    startPingLoad(frame, request, WTFMove(originalRequestHeader), ShouldFollowRedirects::Yes, ContentSecurityPolicyImposition::DoPolicyCheck);
 }
 
 // http://www.whatwg.org/specs/web-apps/current-work/multipage/links.html#hyperlink-auditing
@@ -146,7 +146,7 @@ void PingLoader::sendPing(Frame& frame, const URL& pingURL, const URL& destinati
         }
     }
 
-    startPingLoad(frame, request, WTFMove(originalRequestHeader), ShouldFollowRedirects::Yes);
+    startPingLoad(frame, request, WTFMove(originalRequestHeader), ShouldFollowRedirects::Yes, ContentSecurityPolicyImposition::DoPolicyCheck);
 }
 
 void PingLoader::sendViolationReport(Frame& frame, const URL& reportURL, Ref<FormData>&& report, ViolationReportType reportType)
@@ -185,10 +185,10 @@ void PingLoader::sendViolationReport(Frame& frame, const URL& reportURL, Ref<For
     if (!referrer.isEmpty())
         request.setHTTPReferrer(referrer);
 
-    startPingLoad(frame, request, WTFMove(originalRequestHeader), ShouldFollowRedirects::No);
+    startPingLoad(frame, request, WTFMove(originalRequestHeader), ShouldFollowRedirects::No, ContentSecurityPolicyImposition::SkipPolicyCheck);
 }
 
-void PingLoader::startPingLoad(Frame& frame, ResourceRequest& request, HTTPHeaderMap&& originalRequestHeaders, ShouldFollowRedirects shouldFollowRedirects)
+void PingLoader::startPingLoad(Frame& frame, ResourceRequest& request, HTTPHeaderMap&& originalRequestHeaders, ShouldFollowRedirects shouldFollowRedirects, ContentSecurityPolicyImposition policyCheck)
 {
     unsigned long identifier = frame.page()->progress().createUniqueIdentifier();
     // FIXME: Why activeDocumentLoader? I would have expected documentLoader().
@@ -204,7 +204,7 @@ void PingLoader::startPingLoad(Frame& frame, ResourceRequest& request, HTTPHeade
     // FIXME: Move ping loads to normal subresource loading to get normal inspector request instrumentation hooks.
     InspectorInstrumentation::willSendRequestOfType(&frame, identifier, frame.loader().activeDocumentLoader(), request, InspectorInstrumentation::LoadType::Ping);
 
-    platformStrategies()->loaderStrategy()->startPingLoad(frame, request, WTFMove(originalRequestHeaders), options, [protectedFrame = makeRef(frame), identifier] (const ResourceError& error, const ResourceResponse& response) {
+    platformStrategies()->loaderStrategy()->startPingLoad(frame, request, WTFMove(originalRequestHeaders), options, policyCheck, [protectedFrame = makeRef(frame), identifier] (const ResourceError& error, const ResourceResponse& response) {
         if (!response.isNull())
             InspectorInstrumentation::didReceiveResourceResponse(protectedFrame, identifier, protectedFrame->loader().activeDocumentLoader(), response, nullptr);
         if (error.isNull()) {
index 6deef45..8d82d71 100644 (file)
@@ -47,6 +47,8 @@ enum class ViolationReportType {
     XSSAuditor,
 };
 
+enum class ContentSecurityPolicyImposition : uint8_t;
+
 class PingLoader {
 public:
     static void loadImage(Frame&, const URL&);
@@ -55,7 +57,7 @@ public:
 
 private:
     enum class ShouldFollowRedirects { No, Yes };
-    static void startPingLoad(Frame&, ResourceRequest&, HTTPHeaderMap&& originalRequestHeaders, ShouldFollowRedirects);
+    static void startPingLoad(Frame&, ResourceRequest&, HTTPHeaderMap&& originalRequestHeaders, ShouldFollowRedirects, ContentSecurityPolicyImposition);
 };
 
 } // namespace WebCore
index cbc87cc..d78ee5c 100644 (file)
@@ -288,7 +288,7 @@ void CachedResource::load(CachedResourceLoader& cachedResourceLoader)
             unsigned long identifier = frame.page()->progress().createUniqueIdentifier();
             InspectorInstrumentation::willSendRequestOfType(&frame, identifier, frameLoader.activeDocumentLoader(), request, InspectorInstrumentation::LoadType::Beacon);
 
-            platformStrategies()->loaderStrategy()->startPingLoad(frame, request, m_originalRequest->httpHeaderFields(), m_options, [this, protectedThis = WTFMove(protectedThis), protectedFrame = makeRef(frame), identifier] (const ResourceError& error, const ResourceResponse& response) {
+            platformStrategies()->loaderStrategy()->startPingLoad(frame, request, m_originalRequest->httpHeaderFields(), m_options, m_options.contentSecurityPolicyImposition, [this, protectedThis = WTFMove(protectedThis), protectedFrame = makeRef(frame), identifier] (const ResourceError& error, const ResourceResponse& response) {
                 if (!response.isNull())
                     InspectorInstrumentation::didReceiveResourceResponse(protectedFrame, identifier, protectedFrame->loader().activeDocumentLoader(), response, nullptr);
                 if (error.isNull()) {
index a61ab73..a378b23 100644 (file)
@@ -1,3 +1,15 @@
+2019-01-04  Youenn Fablet  <youenn@apple.com>
+
+        CSP violation reports should bypass CSP checks
+        https://bugs.webkit.org/show_bug.cgi?id=192857
+        <rdar://problem/46887236>
+
+        Reviewed by Chris Dumez.
+
+        * WebProcess/Network/WebLoaderStrategy.cpp:
+        (WebKit::WebLoaderStrategy::startPingLoad):
+        * WebProcess/Network/WebLoaderStrategy.h:
+
 2019-01-04  Alex Christensen  <achristensen@webkit.org>
 
         Use WebsiteDataStoreParameters instead of NetworkProcessCreationParameters for IndexedDB directories
index 58908cd..e0d0bfe 100644 (file)
@@ -575,7 +575,7 @@ static uint64_t generateLoadIdentifier()
     return ++identifier;
 }
 
-void WebLoaderStrategy::startPingLoad(Frame& frame, ResourceRequest& request, const HTTPHeaderMap& originalRequestHeaders, const FetchOptions& options, PingLoadCompletionHandler&& completionHandler)
+void WebLoaderStrategy::startPingLoad(Frame& frame, ResourceRequest& request, const HTTPHeaderMap& originalRequestHeaders, const FetchOptions& options, ContentSecurityPolicyImposition policyCheck, PingLoadCompletionHandler&& completionHandler)
 {
     auto* document = frame.document();
     if (!document) {
@@ -595,8 +595,8 @@ void WebLoaderStrategy::startPingLoad(Frame& frame, ResourceRequest& request, co
     loadParameters.originalRequestHeaders = originalRequestHeaders;
     loadParameters.shouldClearReferrerOnHTTPSToHTTPRedirect = shouldClearReferrerOnHTTPSToHTTPRedirect(&frame);
     loadParameters.shouldRestrictHTTPResponseAccess = shouldPerformSecurityChecks();
-    if (!document->shouldBypassMainWorldContentSecurityPolicy()) {
-        if (auto * contentSecurityPolicy = document->contentSecurityPolicy())
+    if (policyCheck == ContentSecurityPolicyImposition::DoPolicyCheck && !document->shouldBypassMainWorldContentSecurityPolicy()) {
+        if (auto* contentSecurityPolicy = document->contentSecurityPolicy())
             loadParameters.cspResponseHeaders = contentSecurityPolicy->responseHeaders();
     }
 
index 7e11820..1efc93c 100644 (file)
@@ -62,7 +62,7 @@ public:
     void suspendPendingRequests() final;
     void resumePendingRequests() final;
 
-    void startPingLoad(WebCore::Frame&, WebCore::ResourceRequest&, const WebCore::HTTPHeaderMap& originalRequestHeaders, const WebCore::FetchOptions&, PingLoadCompletionHandler&&) final;
+    void startPingLoad(WebCore::Frame&, WebCore::ResourceRequest&, const WebCore::HTTPHeaderMap& originalRequestHeaders, const WebCore::FetchOptions&, WebCore::ContentSecurityPolicyImposition, PingLoadCompletionHandler&&) final;
     void didFinishPingLoad(uint64_t pingLoadIdentifier, WebCore::ResourceError&&, WebCore::ResourceResponse&&);
 
     void preconnectTo(WebCore::FrameLoader&, const URL&, WebCore::StoredCredentialsPolicy, PreconnectCompletionHandler&&) final;
index 00bd70c..20d269c 100644 (file)
@@ -1,3 +1,15 @@
+2019-01-04  Youenn Fablet  <youenn@apple.com>
+
+        CSP violation reports should bypass CSP checks
+        https://bugs.webkit.org/show_bug.cgi?id=192857
+        <rdar://problem/46887236>
+
+        Reviewed by Chris Dumez.
+
+        * WebCoreSupport/WebResourceLoadScheduler.cpp:
+        (WebResourceLoadScheduler::startPingLoad):
+        * WebCoreSupport/WebResourceLoadScheduler.h:
+
 2018-12-27  Alex Christensen  <achristensen@webkit.org>
 
         Resurrect Mac CMake build
index f7aba48..9872c31 100644 (file)
@@ -370,7 +370,7 @@ bool WebResourceLoadScheduler::HostInformation::limitRequests(ResourceLoadPriori
     return m_requestsLoading.size() >= (webResourceLoadScheduler().isSerialLoadingEnabled() ? 1 : m_maxRequestsInFlight);
 }
 
-void WebResourceLoadScheduler::startPingLoad(Frame& frame, ResourceRequest& request, const HTTPHeaderMap&, const FetchOptions& options, PingLoadCompletionHandler&& completionHandler)
+void WebResourceLoadScheduler::startPingLoad(Frame& frame, ResourceRequest& request, const HTTPHeaderMap&, const FetchOptions& options, ContentSecurityPolicyImposition, PingLoadCompletionHandler&& completionHandler)
 {
     // PingHandle manages its own lifetime, deleting itself when its purpose has been fulfilled.
     new PingHandle(frame.loader().networkingContext(), request, options.credentials != FetchOptions::Credentials::Omit, options.redirect == FetchOptions::Redirect::Follow, WTFMove(completionHandler));
index 112f65f..55b3697 100644 (file)
 
 class WebResourceLoadScheduler;
 
-namespace WebCore {
-struct FetchOptions;
-class SecurityOrigin;
-}
-
 WebResourceLoadScheduler& webResourceLoadScheduler();
 
 class WebResourceLoadScheduler final : public WebCore::LoaderStrategy {
@@ -61,7 +56,7 @@ public:
     void suspendPendingRequests() final;
     void resumePendingRequests() final;
 
-    void startPingLoad(WebCore::Frame&, WebCore::ResourceRequest&, const WebCore::HTTPHeaderMap&, const WebCore::FetchOptions&, PingLoadCompletionHandler&&) final;
+    void startPingLoad(WebCore::Frame&, WebCore::ResourceRequest&, const WebCore::HTTPHeaderMap&, const WebCore::FetchOptions&, WebCore::ContentSecurityPolicyImposition, PingLoadCompletionHandler&&) final;
 
     void preconnectTo(WebCore::FrameLoader&, const URL&, WebCore::StoredCredentialsPolicy, PreconnectCompletionHandler&&) final;