Attachment 362203 Details for Bug 194716 – Patch

[patch] Patch

bug-194716-20190215204343.patch (text/plain), 19.71 KB, created by youenn fablet on 2019-02-15 20:43:43 PST

(hide)

Description:

Filename:

MIME Type:

Creator: youenn fablet

Created: 2019-02-15 20:43:43 PST

Size: 19.71 KB

patch

obsolete

>Subversion Revision: 241548
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 3d11497e5c1b847acc18d81b1929b1ce25651a3e..0996c4cda9fdb186f17282afe96de5d4887da770 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,20 @@
>+2019-02-15  Youenn Fablet  <youenn@apple.com>
>+
>+        Move service worker response validation from the service worker client to the service worker itself
>+        https://bugs.webkit.org/show_bug.cgi?id=194716
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Added response validation at service worker side.
>+
>+        No change of behavior except for now logging validation error messages in the console.
>+        Covered by rebased tests.
>+
>+        * workers/service/context/ServiceWorkerFetch.cpp:
>+        (WebCore::ServiceWorkerFetch::validateResponse):
>+        (WebCore::ServiceWorkerFetch::processResponse):
>+        (WebCore::ServiceWorkerFetch::dispatchFetchEvent):
>+
> 2019-02-14  Youenn Fablet  <youenn@apple.com>
> 
>         Make ServiceWorkerClientFetch closer to WebResourceLoader
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index afcb46e96be578b72d9f3d48e5b7449ad86ce9d3..2899fcf63dd978730d468831ae42f33782916341 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,16 @@
>+2019-02-15  Youenn Fablet  <youenn@apple.com>
>+
>+        Move service worker response validation from the service worker client to the service worker itself
>+        https://bugs.webkit.org/show_bug.cgi?id=194716
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Removed response validation as it is now done in service worker side.
>+
>+        * WebProcess/Storage/ServiceWorkerClientFetch.cpp:
>+        (WebKit::ServiceWorkerClientFetch::didReceiveRedirectResponse):
>+        (WebKit::ServiceWorkerClientFetch::didReceiveResponse):
>+
> 2019-02-14  Youenn Fablet  <youenn@apple.com>
> 
>         Make ServiceWorkerClientFetch closer to WebResourceLoader
>diff --git a/Source/WebCore/workers/service/context/ServiceWorkerFetch.cpp b/Source/WebCore/workers/service/context/ServiceWorkerFetch.cpp
>index b8d4d123a9f08ff89a9d956c2d54303fe4b81759..32a56b7bba0e435b3a4b5493ae3109604201ed15 100644
>--- a/Source/WebCore/workers/service/context/ServiceWorkerFetch.cpp
>+++ b/Source/WebCore/workers/service/context/ServiceWorkerFetch.cpp
>@@ -33,6 +33,7 @@
> #include "FetchEvent.h"
> #include "FetchRequest.h"
> #include "FetchResponse.h"
>+#include "MIMETypeRegistry.h"
> #include "ReadableStreamChunk.h"
> #include "ResourceRequest.h"
> #include "ServiceWorker.h"
>@@ -43,7 +44,26 @@ namespace WebCore {
> 
> namespace ServiceWorkerFetch {
> 
>-static void processResponse(Ref<Client>&& client, Expected<Ref<FetchResponse>, ResourceError>&& result)
>+// https://fetch.spec.whatwg.org/#http-fetch step 3.3
>+static inline Optional<ResourceError> validateResponse(const ResourceResponse& response, FetchOptions::Mode mode, FetchOptions::Redirect redirect)
>+{
>+    if (response.type() == ResourceResponse::Type::Error)
>+        return ResourceError { errorDomainWebKitInternal, 0, response.url(), "Response served by service worker is an error"_s, ResourceError::Type::General };
>+
>+    if (mode != FetchOptions::Mode::NoCors && response.tainting() == ResourceResponse::Tainting::Opaque)
>+        return ResourceError { errorDomainWebKitInternal, 0, response.url(), "Response served by service worker is opaque"_s, ResourceError::Type::AccessControl };
>+
>+    // Navigate mode induces manual redirect.
>+    if (redirect != FetchOptions::Redirect::Manual && mode != FetchOptions::Mode::Navigate && response.tainting() == ResourceResponse::Tainting::Opaqueredirect)
>+        return ResourceError { errorDomainWebKitInternal, 0, response.url(), "Response served by service worker is opaque redirect"_s, ResourceError::Type::AccessControl };
>+
>+    if ((redirect != FetchOptions::Redirect::Follow || mode == FetchOptions::Mode::Navigate) && response.isRedirected())
>+        return ResourceError { errorDomainWebKitInternal, 0, response.url(), "Response served by service worker has redirections"_s, ResourceError::Type::AccessControl };
>+
>+    return { };
>+}
>+
>+static void processResponse(Ref<Client>&& client, Expected<Ref<FetchResponse>, ResourceError>&& result, FetchOptions::Mode mode, FetchOptions::Redirect redirect, const URL& requestURL)
> {
>     if (!result.has_value()) {
>         client->didFail(result.error());
>@@ -58,11 +78,31 @@ static void processResponse(Ref<Client>&& client, Expected<Ref<FetchResponse>, R
>     }
> 
>     auto resourceResponse = response->resourceResponse();
>+    if (auto error = validateResponse(resourceResponse, mode, redirect)) {
>+        client->didFail(error.value());
>+        return;
>+    }
>+
>     if (resourceResponse.isRedirection() && resourceResponse.httpHeaderFields().contains(HTTPHeaderName::Location)) {
>         client->didReceiveRedirection(resourceResponse);
>         return;
>     }
> 
>+    resourceResponse.setSource(ResourceResponse::Source::ServiceWorker);
>+
>+    // In case of main resource and mime type is the default one, we set it to text/html to pass more service worker WPT tests.
>+    // FIXME: We should refine our MIME type sniffing strategy for synthetic responses.
>+    if (mode == FetchOptions::Mode::Navigate) {
>+        if (resourceResponse.mimeType() == defaultMIMEType()) {
>+            resourceResponse.setMimeType("text/html"_s);
>+            resourceResponse.setTextEncodingName("UTF-8"_s);
>+        }
>+    }
>+
>+    // As per https://fetch.spec.whatwg.org/#main-fetch step 9, copy request's url list in response's url list if empty.
>+    if (resourceResponse.url().isNull())
>+        resourceResponse.setURL(requestURL);
>+
>     client->didReceiveResponse(resourceResponse);
> 
>     if (response->isBodyReceivedByChunk()) {
>@@ -95,6 +135,9 @@ void dispatchFetchEvent(Ref<Client>&& client, ServiceWorkerGlobalScope& globalSc
> {
>     auto requestHeaders = FetchHeaders::create(FetchHeaders::Guard::Immutable, HTTPHeaderMap { request.httpHeaderFields() });
> 
>+    FetchOptions::Mode mode = options.mode;
>+    FetchOptions::Redirect redirect = options.redirect;
>+
>     bool isNavigation = options.mode == FetchOptions::Mode::Navigate;
>     bool isNonSubresourceRequest = WebCore::isNonSubresourceRequest(options.destination);
> 
>@@ -129,8 +172,8 @@ void dispatchFetchEvent(Ref<Client>&& client, ServiceWorkerGlobalScope& globalSc
>     init.cancelable = true;
>     auto event = FetchEvent::create(eventNames().fetchEvent, WTFMove(init), Event::IsTrusted::Yes);
> 
>-    event->onResponse([client = client.copyRef()] (auto&& result) mutable {
>-        processResponse(WTFMove(client), WTFMove(result));
>+    event->onResponse([client = client.copyRef(), mode, redirect, requestURL] (auto&& result) mutable {
>+        processResponse(WTFMove(client), WTFMove(result), mode, redirect, requestURL);
>     });
> 
>     globalScope.dispatchEvent(event);
>diff --git a/Source/WebKit/WebProcess/Storage/ServiceWorkerClientFetch.cpp b/Source/WebKit/WebProcess/Storage/ServiceWorkerClientFetch.cpp
>index f07473c855bcf1217265fc82155c291cdcb57306..381de8cf9a1da08f364fde6950ac56403697d2ab 100644
>--- a/Source/WebKit/WebProcess/Storage/ServiceWorkerClientFetch.cpp
>+++ b/Source/WebKit/WebProcess/Storage/ServiceWorkerClientFetch.cpp
>@@ -34,7 +34,6 @@
> #include <WebCore/CrossOriginAccessControl.h>
> #include <WebCore/Document.h>
> #include <WebCore/Frame.h>
>-#include <WebCore/MIMETypeRegistry.h>
> #include <WebCore/NotImplemented.h>
> #include <WebCore/ResourceError.h>
> #include <WebCore/SharedBuffer.h>
>@@ -78,41 +77,12 @@ void ServiceWorkerClientFetch::start()
>     m_connection->startFetch(m_identifier, m_serviceWorkerRegistrationIdentifier, request, options, referrer);
> }
> 
>-// https://fetch.spec.whatwg.org/#http-fetch step 3.3
>-Optional<ResourceError> ServiceWorkerClientFetch::validateResponse(const ResourceResponse& response)
>-{
>-    // FIXME: make a better error reporting.
>-    if (response.type() == ResourceResponse::Type::Error)
>-        return ResourceError { ResourceError::Type::General };
>-
>-    auto& options = m_loader->options();
>-    if (options.mode != FetchOptions::Mode::NoCors && response.tainting() == ResourceResponse::Tainting::Opaque)
>-        return ResourceError { errorDomainWebKitInternal, 0, response.url(), "Response served by service worker is opaque"_s, ResourceError::Type::AccessControl };
>-
>-    // Navigate mode induces manual redirect.
>-    if (options.redirect != FetchOptions::Redirect::Manual && options.mode != FetchOptions::Mode::Navigate && response.tainting() == ResourceResponse::Tainting::Opaqueredirect)
>-        return ResourceError { errorDomainWebKitInternal, 0, response.url(), "Response served by service worker is opaque redirect"_s, ResourceError::Type::AccessControl };
>-
>-    if ((options.redirect != FetchOptions::Redirect::Follow || options.mode == FetchOptions::Mode::Navigate) && response.isRedirected())
>-        return ResourceError { errorDomainWebKitInternal, 0, response.url(), "Response served by service worker has redirections"_s, ResourceError::Type::AccessControl };
>-
>-    return WTF::nullopt;
>-}
>-
> void ServiceWorkerClientFetch::didReceiveRedirectResponse(ResourceResponse&& response)
> {
>     callOnMainThread([this, protectedThis = makeRef(*this), response = WTFMove(response)]() mutable {
>         if (!m_loader)
>             return;
> 
>-        if (auto error = validateResponse(response)) {
>-            m_loader->didFail(error.value());
>-            ASSERT(!m_loader);
>-            if (auto callback = WTFMove(m_callback))
>-                callback(Result::Succeeded);
>-
>-            return;
>-        }
>         response.setSource(ResourceResponse::Source::ServiceWorker);
> 
>         m_loader->willSendRequest(m_loader->request().redirectedRequest(response, m_shouldClearReferrerOnHTTPSToHTTPRedirect), response, [protectedThis = makeRef(*this), this](ResourceRequest&& request) {
>@@ -133,32 +103,10 @@ void ServiceWorkerClientFetch::didReceiveResponse(ResourceResponse&& response, b
>         if (!m_loader)
>             return;
> 
>-        if (auto error = validateResponse(response)) {
>-            m_loader->didFail(error.value());
>-            ASSERT(!m_loader);
>-            if (auto callback = WTFMove(m_callback))
>-                callback(Result::Succeeded);
>-
>-            return;
>-        }
>-        response.setSource(ResourceResponse::Source::ServiceWorker);
>-        ASSERT(!response.isRedirection() || !response.httpHeaderFields().contains(HTTPHeaderName::Location));
>-
>         if (auto callback = WTFMove(m_callback))
>             callback(Result::Succeeded);
> 
>-        // In case of main resource and mime type is the default one, we set it to text/html to pass more service worker WPT tests.
>-        // FIXME: We should refine our MIME type sniffing strategy for synthetic responses.
>-        if (m_loader->originalRequest().requester() == ResourceRequest::Requester::Main) {
>-            if (response.mimeType() == defaultMIMEType()) {
>-                response.setMimeType("text/html"_s);
>-                response.setTextEncodingName("UTF-8"_s);
>-            }
>-        }
>-
>-        // As per https://fetch.spec.whatwg.org/#main-fetch step 9, copy request's url list in response's url list if empty.
>-        if (response.url().isNull())
>-            response.setURL(m_loader->request().url());
>+        ASSERT(!response.isRedirection() || !response.httpHeaderFields().contains(HTTPHeaderName::Location));
> 
>         if (!needsContinueDidReceiveResponseMessage) {
>             m_loader->didReceiveResponse(response, [] { });
>@@ -213,7 +161,8 @@ void ServiceWorkerClientFetch::didFail(ResourceError&& error)
> 
>         auto* document = m_loader->frame() ? m_loader->frame()->document() : nullptr;
>         if (document) {
>-            document->addConsoleMessage(MessageSource::JS, MessageLevel::Error, error.localizedDescription());
>+            if (m_loader->options().destination != FetchOptions::Destination::EmptyString || error.isGeneral())
>+                document->addConsoleMessage(MessageSource::JS, MessageLevel::Error, error.localizedDescription());
>             if (m_loader->options().destination != FetchOptions::Destination::EmptyString)
>                 document->addConsoleMessage(MessageSource::JS, MessageLevel::Error, makeString("Cannot load ", error.failingURL().string(), "."));
>         }
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index 32bd451e1ce8ad432dea54bd49ed6d80cc7d6550..4dc81f2376eb617560aab39b8c6b830a1c40d0fd 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,16 @@
>+2019-02-15  Youenn Fablet  <youenn@apple.com>
>+
>+        Move service worker response validation from the service worker client to the service worker itself
>+        https://bugs.webkit.org/show_bug.cgi?id=194716
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Rebased tests as we now report to the console log any service worker response validation erorr.
>+
>+        * http/tests/inspector/network/resource-response-service-worker-expected.txt:
>+        * http/tests/workers/service/basic-fetch.https-expected.txt:
>+        * http/tests/workers/service/service-worker-crossorigin-fetch-expected.txt:
>+
> 2019-02-14  Zalan Bujtas  <zalan@apple.com>
> 
>         [LFC][BFC][MarginCollapse] Replaced boxes don't collapse through their margins
>diff --git a/LayoutTests/imported/w3c/ChangeLog b/LayoutTests/imported/w3c/ChangeLog
>index ce0540cfd917401df3c0d21ea179995c63b0ba92..23cc983f3e1ac6015b4033b1fe315c5233ef32a2 100644
>--- a/LayoutTests/imported/w3c/ChangeLog
>+++ b/LayoutTests/imported/w3c/ChangeLog
>@@ -1,3 +1,14 @@
>+2019-02-15  Youenn Fablet  <youenn@apple.com>
>+
>+        Move service worker response validation from the service worker client to the service worker itself
>+        https://bugs.webkit.org/show_bug.cgi?id=194716
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Rebased tests as we now report to the console log any service worker response validation erorr.
>+
>+        * web-platform-tests/service-workers/service-worker/fetch-canvas-tainting-image.https-expected.txt:
>+
> 2019-02-12  Rob Buis  <rbuis@igalia.com>
> 
>         Align with Fetch on data: URLs
>diff --git a/LayoutTests/http/tests/inspector/network/resource-response-service-worker-expected.txt b/LayoutTests/http/tests/inspector/network/resource-response-service-worker-expected.txt
>index 09d2da7f4826f1bff5fbe0e7dc5959c52eeeb6a6..e86860bc4bafd0cb7064469a4fb15c5fc5eb09ed 100644
>--- a/LayoutTests/http/tests/inspector/network/resource-response-service-worker-expected.txt
>+++ b/LayoutTests/http/tests/inspector/network/resource-response-service-worker-expected.txt
>@@ -1,3 +1,4 @@
>+CONSOLE MESSAGE: Response served by service worker is an error
> Test for `Resource.ResponseSource.ServiceWorker`.
> 
> 
>diff --git a/LayoutTests/http/tests/workers/service/basic-fetch.https-expected.txt b/LayoutTests/http/tests/workers/service/basic-fetch.https-expected.txt
>index 1e4125881d09a392fd178fdb9f4239e93afa501e..6200f7ba858dec2927c76a43d56254c52f644a9e 100644
>--- a/LayoutTests/http/tests/workers/service/basic-fetch.https-expected.txt
>+++ b/LayoutTests/http/tests/workers/service/basic-fetch.https-expected.txt
>@@ -1,5 +1,7 @@
>+CONSOLE MESSAGE: Response served by service worker is an error
> CONSOLE MESSAGE: Fetch event was canceled
> CONSOLE MESSAGE: Fetch API cannot load https://127.0.0.1:8443/workers/service/resources/test5.
>+CONSOLE MESSAGE: Response served by service worker is an error
> 
> test1 url: https://127.0.0.1:8443/workers/service/resources/test1
> test1 status code: 200
>diff --git a/LayoutTests/http/tests/workers/service/service-worker-crossorigin-fetch-expected.txt b/LayoutTests/http/tests/workers/service/service-worker-crossorigin-fetch-expected.txt
>index c7553e0db70fd9ca6f03b08539a3030ad2806ba9..f42748376085d7621cae794f8277e44421dad0a9 100644
>--- a/LayoutTests/http/tests/workers/service/service-worker-crossorigin-fetch-expected.txt
>+++ b/LayoutTests/http/tests/workers/service/service-worker-crossorigin-fetch-expected.txt
>@@ -1,5 +1,6 @@
> CONSOLE MESSAGE: Origin http://127.0.0.1:8000 is not allowed by Access-Control-Allow-Origin.
> CONSOLE MESSAGE: Fetch API cannot load http://localhost:8080/resources/square100.png.fromserviceworker due to access control checks.
>+CONSOLE MESSAGE: Response served by service worker is an error
> 
> 
> PASS Testing unintercepted fetch with preflight, fetch should fail 
>diff --git a/LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/fetch-canvas-tainting-image.https-expected.txt b/LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/fetch-canvas-tainting-image.https-expected.txt
>index ce3e0f90000bb9e8fe187b085b84385971398bfd..4c2440fd0e8124f98d268f20c1831d18d27bc002 100644
>--- a/LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/fetch-canvas-tainting-image.https-expected.txt
>+++ b/LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/fetch-canvas-tainting-image.https-expected.txt
>@@ -21,13 +21,21 @@ CONSOLE MESSAGE: Credentials flag is true, but Access-Control-Allow-Credentials
> CONSOLE MESSAGE: Cannot load image https://127.0.0.1:9443/service-workers/service-worker/resources/fetch-access-control.py?PNGIMAGE&Auth&ACAOrigin=https://localhost:9443&ignore due to access control checks.
> CONSOLE MESSAGE: line 26: Unable to get image data from canvas because the canvas has been tainted by cross-origin data.
> CONSOLE MESSAGE: Response served by service worker is opaque
>+CONSOLE MESSAGE: Cannot load https://127.0.0.1:9443/service-workers/service-worker/resources/fetch-access-control.py?PNGIMAGE.
>+CONSOLE MESSAGE: Response served by service worker is opaque
> CONSOLE MESSAGE: Cannot load image https://localhost:9443/service-workers/service-worker/resources/fetch-access-control.py?PNGIMAGE&mode=no-cors&url=https%3A%2F%2F127.0.0.1%3A9443%2Fservice-workers%2Fservice-worker%2Fresources%2Ffetch-access-control.py%3FPNGIMAGE due to access control checks.
> CONSOLE MESSAGE: Response served by service worker is opaque
>+CONSOLE MESSAGE: Cannot load https://127.0.0.1:9443/service-workers/service-worker/resources/fetch-access-control.py?PNGIMAGE.
>+CONSOLE MESSAGE: Response served by service worker is opaque
> CONSOLE MESSAGE: Cannot load image https://localhost:9443/service-workers/service-worker/resources/fetch-access-control.py?PNGIMAGE&mode=no-cors&url=https%3A%2F%2F127.0.0.1%3A9443%2Fservice-workers%2Fservice-worker%2Fresources%2Ffetch-access-control.py%3FPNGIMAGE due to access control checks.
> CONSOLE MESSAGE: line 26: Unable to get image data from canvas because the canvas has been tainted by cross-origin data.
> CONSOLE MESSAGE: Response served by service worker is opaque
>+CONSOLE MESSAGE: Cannot load https://127.0.0.1:9443/service-workers/service-worker/resources/fetch-access-control.py?PNGIMAGE.
>+CONSOLE MESSAGE: Response served by service worker is opaque
> CONSOLE MESSAGE: Cannot load image https://127.0.0.1:9443/service-workers/service-worker/resources/fetch-access-control.py?PNGIMAGE&mode=no-cors&url=https%3A%2F%2F127.0.0.1%3A9443%2Fservice-workers%2Fservice-worker%2Fresources%2Ffetch-access-control.py%3FPNGIMAGE due to access control checks.
> CONSOLE MESSAGE: Response served by service worker is opaque
>+CONSOLE MESSAGE: Cannot load https://127.0.0.1:9443/service-workers/service-worker/resources/fetch-access-control.py?PNGIMAGE.
>+CONSOLE MESSAGE: Response served by service worker is opaque
> CONSOLE MESSAGE: Cannot load image https://127.0.0.1:9443/service-workers/service-worker/resources/fetch-access-control.py?PNGIMAGE&mode=no-cors&url=https%3A%2F%2F127.0.0.1%3A9443%2Fservice-workers%2Fservice-worker%2Fresources%2Ffetch-access-control.py%3FPNGIMAGE due to access control checks.
> CONSOLE MESSAGE: FetchEvent.respondWith received an error: TypeError: Credentials flag is true, but Access-Control-Allow-Credentials is not "true".
> CONSOLE MESSAGE: Cannot load https://localhost:9443/service-workers/service-worker/resources/fetch-access-control.py?PNGIMAGE&mode=cors&url=https%3A%2F%2F127.0.0.1%3A9443%2Fservice-workers%2Fservice-worker%2Fresources%2Ffetch-access-control.py%3FPNGIMAGE%26ACAOrigin%3Dhttps%3A%2F%2Flocalhost%3A9443.

Actions: View | Formatted Diff | Diff

Attachments on bug 194716: 362141 | 362159 | 362174 | 362203