Move service worker response validation from the service worker client to the service...
authoryouenn@apple.com <youenn@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 26 Feb 2019 23:53:22 +0000 (23:53 +0000)
committeryouenn@apple.com <youenn@apple.com@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Tue, 26 Feb 2019 23:53:22 +0000 (23:53 +0000)
https://bugs.webkit.org/show_bug.cgi?id=194716

Reviewed by Geoffrey Garen.

LayoutTests/imported/w3c:

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:

Source/WebCore:

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):

Source/WebKit:

Removed response validation as it is now done in service worker side.

* WebProcess/Storage/ServiceWorkerClientFetch.cpp:
(WebKit::ServiceWorkerClientFetch::didReceiveRedirectResponse):
(WebKit::ServiceWorkerClientFetch::didReceiveResponse):

LayoutTests:

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:

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

LayoutTests/ChangeLog
LayoutTests/http/tests/inspector/network/resource-response-service-worker-expected.txt
LayoutTests/http/tests/workers/service/basic-fetch.https-expected.txt
LayoutTests/http/tests/workers/service/service-worker-crossorigin-fetch-expected.txt
LayoutTests/imported/w3c/ChangeLog
LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/fetch-canvas-tainting-image.https-expected.txt
Source/WebCore/ChangeLog
Source/WebCore/workers/service/context/ServiceWorkerFetch.cpp
Source/WebKit/ChangeLog
Source/WebKit/WebProcess/Storage/ServiceWorkerClientFetch.cpp

index 2de8989..6535c3e 100644 (file)
@@ -1,3 +1,16 @@
+2019-02-26  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 Geoffrey Garen.
+
+        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-26  Takashi Komori  <Takashi.Komori@sony.com>
 
         [Curl] Load HTTP body of 401 response when AuthenticationChange is cancelled.
index 1e41258..6200f7b 100644 (file)
@@ -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
index c7553e0..f427483 100644 (file)
@@ -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 
index 3c2554d..fe45560 100644 (file)
@@ -1,3 +1,14 @@
+2019-02-26  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 Geoffrey Garen.
+
+        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-26  Frederic Wang  <fwang@igalia.com>
 
         Synchronize MathML WPT tests
index ce3e0f9..4c2440f 100644 (file)
@@ -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.
index c698cb0..1abc75e 100644 (file)
@@ -1,3 +1,20 @@
+2019-02-26  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 Geoffrey Garen.
+
+        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-26  Sihui Liu  <sihui_liu@apple.com>
 
         [Mac WK2] storage/indexeddb/IDBObject-leak.html is flaky
index b8d4d12..32a56b7 100644 (file)
@@ -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);
index 4c1e5ad..f586931 100644 (file)
@@ -1,3 +1,16 @@
+2019-02-26  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 Geoffrey Garen.
+
+        Removed response validation as it is now done in service worker side.
+
+        * WebProcess/Storage/ServiceWorkerClientFetch.cpp:
+        (WebKit::ServiceWorkerClientFetch::didReceiveRedirectResponse):
+        (WebKit::ServiceWorkerClientFetch::didReceiveResponse):
+
 2019-02-26  Zalan Bujtas  <zalan@apple.com>
 
         [ContentChangeObserver] clearContentChangeObservers should be internal to ContentChangeObserver class
index c9a3f34..324466a 100644 (file)
@@ -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, [this, protectedThis = protectedThis.copyRef()](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(), "."));
         }