Attachment 358322 Details for Bug 192857 – Patch

[patch] Patch

bug-192857-20190104090009.patch (text/plain), 19.41 KB, created by youenn fablet on 2019-01-04 09:00:10 PST

(hide)

Description:

Filename:

MIME Type:

Creator: youenn fablet

Created: 2019-01-04 09:00:10 PST

Size: 19.41 KB

patch

obsolete

>Subversion Revision: 239594
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index dc1f8d982cc8394940734728052ad8004df6fa69..efc892fd04c895ddff935c291eb1e50faa5497ca 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,30 @@
>+2019-01-03  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 NOBODY (OOPS!).
>+
>+        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-02  Simon Fraser  <simon.fraser@apple.com>
> 
>         REGRESSION (r239306): Don't disable font smoothing in transparent layers on macOS Mojave and later
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index a4835ba42b6cfd6fdc471db3989d1a8e4f216af9..566d0d38f356f627a13e1ed3911eb0a1ed7ea612 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,15 @@
>+2019-01-03  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 NOBODY (OOPS!).
>+
>+        * WebProcess/Network/WebLoaderStrategy.cpp:
>+        (WebKit::WebLoaderStrategy::startPingLoad):
>+        * WebProcess/Network/WebLoaderStrategy.h:
>+
> 2019-01-03  Brent Fulgham  <bfulgham@apple.com>
> 
>         Remove logic handling DNT header during redirects
>diff --git a/Source/WebKitLegacy/ChangeLog b/Source/WebKitLegacy/ChangeLog
>index 00bd70c1dfb82069c4f39fc4235db36bedd89859..8d0aec2b7dfc2d66d813c5d3ec31eb6cf132d003 100644
>--- a/Source/WebKitLegacy/ChangeLog
>+++ b/Source/WebKitLegacy/ChangeLog
>@@ -1,3 +1,15 @@
>+2019-01-03  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 NOBODY (OOPS!).
>+
>+        * WebCoreSupport/WebResourceLoadScheduler.cpp:
>+        (WebResourceLoadScheduler::startPingLoad):
>+        * WebCoreSupport/WebResourceLoadScheduler.h:
>+
> 2018-12-27  Alex Christensen  <achristensen@webkit.org>
> 
>         Resurrect Mac CMake build
>diff --git a/Source/WebCore/loader/LoaderStrategy.h b/Source/WebCore/loader/LoaderStrategy.h
>index 09637c8141eaec163233f954cfaceeeeeeb69734..b35c6fb0e2864d5f8efc4b80fac7a3ff0411193e 100644
>--- a/Source/WebCore/loader/LoaderStrategy.h
>+++ b/Source/WebCore/loader/LoaderStrategy.h
>@@ -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;
>diff --git a/Source/WebCore/loader/PingLoader.cpp b/Source/WebCore/loader/PingLoader.cpp
>index 134e5560e66f2dea45ac0018c3616065172dcb2e..2c0b2a3be0de12b3ca4e413a8b6f7847f4b37e24 100644
>--- a/Source/WebCore/loader/PingLoader.cpp
>+++ b/Source/WebCore/loader/PingLoader.cpp
>@@ -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()) {
>diff --git a/Source/WebCore/loader/PingLoader.h b/Source/WebCore/loader/PingLoader.h
>index 6deef45169a9e5ca30a9f2f83342379d399b1db1..db2a8d2360a5b1296771b44e0bdef10dff364b72 100644
>--- a/Source/WebCore/loader/PingLoader.h
>+++ b/Source/WebCore/loader/PingLoader.h
>@@ -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
>diff --git a/Source/WebCore/loader/cache/CachedResource.cpp b/Source/WebCore/loader/cache/CachedResource.cpp
>index cbc87ccb53f2ed8b018fe38f9f5453f77a26eb60..d78ee5ce5751093f4856a17ad89d0963248e8ea9 100644
>--- a/Source/WebCore/loader/cache/CachedResource.cpp
>+++ b/Source/WebCore/loader/cache/CachedResource.cpp
>@@ -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()) {
>diff --git a/Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp b/Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp
>index f9857c94ad4620cbeff88e3a9df0c5f8c0b0d99e..07aae505cc5809d82caf0c37f7eea567bcc12ae6 100644
>--- a/Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp
>+++ b/Source/WebKit/WebProcess/Network/WebLoaderStrategy.cpp
>@@ -572,7 +572,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) {
>@@ -591,8 +591,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::SkipPolicyCheck && !document->shouldBypassMainWorldContentSecurityPolicy()) {
>+        if (auto* contentSecurityPolicy = document->contentSecurityPolicy())
>             loadParameters.cspResponseHeaders = contentSecurityPolicy->responseHeaders();
>     }
> 
>diff --git a/Source/WebKit/WebProcess/Network/WebLoaderStrategy.h b/Source/WebKit/WebProcess/Network/WebLoaderStrategy.h
>index 7e11820866a52c69bfe29800f3b2f810f3839507..1efc93c02a3ca2b9bf0edf0aa905e5f9bb0d3bd3 100644
>--- a/Source/WebKit/WebProcess/Network/WebLoaderStrategy.h
>+++ b/Source/WebKit/WebProcess/Network/WebLoaderStrategy.h
>@@ -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;
>diff --git a/Source/WebKitLegacy/WebCoreSupport/WebResourceLoadScheduler.cpp b/Source/WebKitLegacy/WebCoreSupport/WebResourceLoadScheduler.cpp
>index f7aba489aa24d2363cee92321353e9b7e79755aa..9872c31ba584d71da15911b458f5111d1a791c1f 100644
>--- a/Source/WebKitLegacy/WebCoreSupport/WebResourceLoadScheduler.cpp
>+++ b/Source/WebKitLegacy/WebCoreSupport/WebResourceLoadScheduler.cpp
>@@ -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));
>diff --git a/Source/WebKitLegacy/WebCoreSupport/WebResourceLoadScheduler.h b/Source/WebKitLegacy/WebCoreSupport/WebResourceLoadScheduler.h
>index 112f65ff98b54d9d1fb39b3f16130c40769c6c98..55b3697530658214bc979a8792b660726d8296dd 100644
>--- a/Source/WebKitLegacy/WebCoreSupport/WebResourceLoadScheduler.h
>+++ b/Source/WebKitLegacy/WebCoreSupport/WebResourceLoadScheduler.h
>@@ -37,11 +37,6 @@
> 
> 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;
> 
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 5567ab5c5be5bfe9b7cad00eabcb879cfb18a470..20a1a561dbad261eace6ea1c71e71bf73ad00a84 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,17 @@
>+2019-01-03  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 NOBODY (OOPS!).
>+
>+        * 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-03  Youenn Fablet  <youenn@apple.com>
> 
>         Resync WPT fetch tests to 834eac4
>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
>index 0000000000000000000000000000000000000000..c026f6e09611cc18b8b830168c321776aa17f950
>--- /dev/null
>+++ b/LayoutTests/http/wpt/fetch/csp-reports-bypass-csp-checks-expected.txt
>@@ -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 Untitled 
>+
>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
>index 0000000000000000000000000000000000000000..6bbca9051eaafa460ce18d80e146202a02d345dc
>--- /dev/null
>+++ b/LayoutTests/http/wpt/fetch/csp-reports-bypass-csp-checks.html
>@@ -0,0 +1,19 @@
>+<!DOCTYPE html>
>+<script src=/resources/testharness.js></script>
>+<script src=/resources/testharnessreport.js></script>
>+<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
>index 0000000000000000000000000000000000000000..bed74643d35d748d3bf08643b13d35bf2f3e6a0f
>--- /dev/null
>+++ b/LayoutTests/http/wpt/fetch/csp-reports-bypass-csp-checks.html.headers
>@@ -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
>index 0000000000000000000000000000000000000000..75ce221e72b59e6f00b2a9604882f8257300be68
>--- /dev/null
>+++ b/LayoutTests/http/wpt/fetch/resources/store-csp-report.py
>@@ -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()
>+

Actions: View | Formatted Diff | Diff

Attachments on bug 192857: 357693 | 358305 | 358310 | 358322 | 358328 | 358330