Attachment 358424 Details for Bug 188248 – Patch

[patch] Patch

bug-188248-20190104192035.patch (text/plain), 17.75 KB, created by youenn fablet on 2019-01-04 19:20:36 PST

(hide)

Description:

Filename:

MIME Type:

Creator: youenn fablet

Created: 2019-01-04 19:20:36 PST

Size: 17.75 KB

patch

obsolete

>Subversion Revision: 239617
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 528289df65c0681c24fdaab6b985033357ee1e4b..fbbe1f6058b93efc3344ee96aea24246323bb3b6 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,23 @@
>+2019-01-04  Youenn Fablet  <youenn@apple.com>
>+
>+        service worker fetch handler results in bad referrer
>+        https://bugs.webkit.org/show_bug.cgi?id=188248
>+        <rdar://problem/47050478>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        Response sanitization was removing the ReferrerPolicy header from opaque redirect responses.
>+        Reduce sanitization of opaque redirect responses to opaque responses and allow Location header.
>+        Make sure referrer policy is updated for all load redirections, not only CORS loads.
>+
>+        Test: http/tests/security/referrer-policy-redirect-link-downgrade.html
>+
>+        * loader/SubresourceLoader.cpp:
>+        (WebCore::SubresourceLoader::checkRedirectionCrossOriginAccessControl):
>+        * platform/network/ResourceResponseBase.cpp:
>+        (WebCore::isSafeCrossOriginResponseHeader):
>+        (WebCore::ResourceResponseBase::sanitizeHTTPHeaderFieldsAccordingToTainting):
>+
> 2019-01-04  Youenn Fablet  <youenn@apple.com>
> 
>         [Fetch API] Implement abortable fetch
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index be400ab6f3fa578d5424bfa6be2c70e0bca10a9a..96898d639bb32cb128ada13e01c124a26c97fd09 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,22 @@
>+2019-01-04  Youenn Fablet  <youenn@apple.com>
>+
>+        service worker fetch handler results in bad referrer
>+        https://bugs.webkit.org/show_bug.cgi?id=188248
>+        <rdar://problem/47050478>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        NetworkDataTaskCocoa is sometimes updating the referrer on its own.
>+        Instead of updating the referrer when sending the request to WebProcess for evaluation,
>+        Update the referrer once the web process decides to follow the redirection.
>+        This ensures that any referrer that the WebProcess will set will be updated by NetworkDataTaskCocoa.
>+
>+        * NetworkProcess/cocoa/NetworkDataTaskCocoa.h:
>+        * NetworkProcess/cocoa/NetworkDataTaskCocoa.mm:
>+        (WebKit::NetworkDataTaskCocoa::NetworkDataTaskCocoa):
>+        (WebKit::NetworkDataTaskCocoa::restrictRequestReferrerToOriginIfNeeded):
>+        (WebKit::NetworkDataTaskCocoa::willPerformHTTPRedirection):
>+
> 2019-01-04  Chris Dumez  <cdumez@apple.com>
> 
>         Crash under WebProcessPool::addSuspendedPage()
>diff --git a/Source/WebCore/loader/SubresourceLoader.cpp b/Source/WebCore/loader/SubresourceLoader.cpp
>index b301bb9bf6f7ade6fcdb68219e804f32e0c76b8e..483ecd3c13d61e69a358eb81cca6e364cc452009 100644
>--- a/Source/WebCore/loader/SubresourceLoader.cpp
>+++ b/Source/WebCore/loader/SubresourceLoader.cpp
>@@ -567,19 +567,18 @@ bool SubresourceLoader::checkRedirectionCrossOriginAccessControl(const ResourceR
> 
>     ASSERT(options().mode != FetchOptions::Mode::SameOrigin || !m_resource->isCrossOrigin());
> 
>-    if (options().mode != FetchOptions::Mode::Cors)
>-        return true;
>+    // Implementing https://fetch.spec.whatwg.org/#concept-http-redirect-fetch step 7 & 8.
>+    if (options().mode == FetchOptions::Mode::Cors) {
>+        if (m_resource->isCrossOrigin() && !isValidCrossOriginRedirectionURL(newRequest.url())) {
>+            errorMessage = "URL is either a non-HTTP URL or contains credentials."_s;
>+            return false;
>+        }
> 
>-    // Implementing https://fetch.spec.whatwg.org/#concept-http-redirect-fetch step 8 & 9.
>-    if (m_resource->isCrossOrigin() && !isValidCrossOriginRedirectionURL(newRequest.url())) {
>-        errorMessage = "URL is either a non-HTTP URL or contains credentials."_s;
>-        return false;
>+        ASSERT(m_origin);
>+        if (crossOriginFlag && !passesAccessControlCheck(redirectResponse, options().storedCredentialsPolicy, *m_origin, errorMessage))
>+            return false;
>     }
> 
>-    ASSERT(m_origin);
>-    if (crossOriginFlag && !passesAccessControlCheck(redirectResponse, options().storedCredentialsPolicy, *m_origin, errorMessage))
>-        return false;
>-
>     bool redirectingToNewOrigin = false;
>     if (m_resource->isCrossOrigin()) {
>         if (!crossOriginFlag && isNextRequestCrossOrigin)
>@@ -592,9 +591,10 @@ bool SubresourceLoader::checkRedirectionCrossOriginAccessControl(const ResourceR
>     if (crossOriginFlag && redirectingToNewOrigin)
>         m_origin = SecurityOrigin::createUnique();
> 
>+    // Implementing https://fetch.spec.whatwg.org/#concept-http-redirect-fetch step 14.
>     updateReferrerPolicy(redirectResponse.httpHeaderField(HTTPHeaderName::ReferrerPolicy));
>     
>-    if (redirectingToNewOrigin) {
>+    if (options().mode == FetchOptions::Mode::Cors && redirectingToNewOrigin) {
>         cleanHTTPRequestHeadersForAccessControl(newRequest, options().httpHeadersToKeep);
>         updateRequestForAccessControl(newRequest, *m_origin, options().storedCredentialsPolicy);
>     }
>diff --git a/Source/WebCore/platform/network/ResourceResponseBase.cpp b/Source/WebCore/platform/network/ResourceResponseBase.cpp
>index c8909755e55ec48f11eed63c8a4e218fa2d01492..20ac64d57034c69ea92bc7a43536f16d795ffa29 100644
>--- a/Source/WebCore/platform/network/ResourceResponseBase.cpp
>+++ b/Source/WebCore/platform/network/ResourceResponseBase.cpp
>@@ -401,6 +401,7 @@ static bool isSafeCrossOriginResponseHeader(HTTPHeaderName name)
>         || name == HTTPHeaderName::LastEventID
>         || name == HTTPHeaderName::LastModified
>         || name == HTTPHeaderName::Link
>+        || name == HTTPHeaderName::Location
>         || name == HTTPHeaderName::Pragma
>         || name == HTTPHeaderName::Range
>         || name == HTTPHeaderName::ReferrerPolicy
>@@ -441,7 +442,8 @@ void ResourceResponseBase::sanitizeHTTPHeaderFieldsAccordingToTainting()
>         m_httpHeaderFields = WTFMove(filteredHeaders);
>         return;
>     }
>-    case ResourceResponse::Tainting::Opaque: {
>+    case ResourceResponse::Tainting::Opaque:
>+    case ResourceResponse::Tainting::Opaqueredirect: {
>         HTTPHeaderMap filteredHeaders;
>         for (auto& header : m_httpHeaderFields.commonHeaders()) {
>             if (isSafeCrossOriginResponseHeader(header.key))
>@@ -450,11 +452,6 @@ void ResourceResponseBase::sanitizeHTTPHeaderFieldsAccordingToTainting()
>         m_httpHeaderFields = WTFMove(filteredHeaders);
>         return;
>     }
>-    case ResourceResponse::Tainting::Opaqueredirect: {
>-        auto location = httpHeaderField(HTTPHeaderName::Location);
>-        m_httpHeaderFields.clear();
>-        m_httpHeaderFields.add(HTTPHeaderName::Location, WTFMove(location));
>-    }
>     }
> }
> 
>diff --git a/Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.h b/Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.h
>index 84242585719eb1add3af8c34d5bd3cac26ab7195..503f44501a001e741ca0ab2f8af825181f2f6525 100644
>--- a/Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.h
>+++ b/Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.h
>@@ -89,6 +89,8 @@ private:
>     bool tryPasswordBasedAuthentication(const WebCore::AuthenticationChallenge&, ChallengeCompletionHandler&);
>     void applySniffingPoliciesAndBindRequestToInferfaceIfNeeded(__strong NSURLRequest*&, bool shouldContentSniff, bool shouldContentEncodingSniff);
> 
>+    void restrictRequestReferrerToOriginIfNeeded(WebCore::ResourceRequest&, bool shouldBlockCookies);
>+
> #if ENABLE(RESOURCE_LOAD_STATISTICS)
>     static NSHTTPCookieStorage *statelessCookieStorage();
>     void applyCookieBlockingPolicy(bool shouldBlock);
>diff --git a/Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm b/Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm
>index 58e10384885954c8f1566459754a45000413f0a7..8bf1f7dbbbfc956827a6ef31beb5aa288ffc5c47 100644
>--- a/Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm
>+++ b/Source/WebKit/NetworkProcess/cocoa/NetworkDataTaskCocoa.mm
>@@ -197,8 +197,7 @@ NetworkDataTaskCocoa::NetworkDataTaskCocoa(NetworkSession& session, NetworkDataT
> #if ENABLE(RESOURCE_LOAD_STATISTICS)
>     shouldBlockCookies = session.networkStorageSession().shouldBlockCookies(request, frameID, pageID);
> #endif
>-    if (shouldBlockCookies || (m_session->sessionID().isEphemeral() && isThirdPartyRequest(request)))
>-        request.setExistingHTTPReferrerToOriginString();
>+    restrictRequestReferrerToOriginIfNeeded(request, shouldBlockCookies);
> 
>     NSURLRequest *nsRequest = request.nsURLRequest(WebCore::HTTPBodyUpdatePolicy::UpdateHTTPBody);
>     applySniffingPoliciesAndBindRequestToInferfaceIfNeeded(nsRequest, shouldContentSniff == WebCore::ContentSniffingPolicy::SniffContent && !url.isLocalFile(), shouldContentEncodingSniff == WebCore::ContentEncodingSniffingPolicy::Sniff);
>@@ -265,6 +264,12 @@ NetworkDataTaskCocoa::~NetworkDataTaskCocoa()
>     }
> }
> 
>+void NetworkDataTaskCocoa::restrictRequestReferrerToOriginIfNeeded(ResourceRequest& request, bool shouldBlockCookies)
>+{
>+    if (shouldBlockCookies || (m_session->sessionID().isEphemeral() && isThirdPartyRequest(request)))
>+        request.setExistingHTTPReferrerToOriginString();
>+}
>+
> void NetworkDataTaskCocoa::didSendData(uint64_t totalBytesSent, uint64_t totalBytesExpectedToSend)
> {
>     if (m_client)
>@@ -355,9 +360,6 @@ void NetworkDataTaskCocoa::willPerformHTTPRedirection(WebCore::ResourceResponse&
> #endif
> #endif
> 
>-    if (shouldBlockCookies || (m_session->sessionID().isEphemeral() && isThirdPartyRequest(request)))
>-        request.setExistingHTTPReferrerToOriginString();
>-
> #if ENABLE(RESOURCE_LOAD_STATISTICS)
>     // Always apply the policy since blocking may need to be turned on or off in a redirect.
>     applyCookieBlockingPolicy(shouldBlockCookies);
>@@ -375,7 +377,13 @@ void NetworkDataTaskCocoa::willPerformHTTPRedirection(WebCore::ResourceResponse&
>     updateTaskWithFirstPartyForSameSiteCookies(m_task.get(), request);
> 
>     if (m_client)
>-        m_client->willPerformHTTPRedirection(WTFMove(redirectResponse), WTFMove(request), WTFMove(completionHandler));
>+        m_client->willPerformHTTPRedirection(WTFMove(redirectResponse), WTFMove(request), [completionHandler = WTFMove(completionHandler), this, protectedThis = makeRef(*this)] (auto&& request) mutable {
>+            if (!request.isNull()) {
>+                bool shouldBlockCookies = m_session->networkStorageSession().shouldBlockCookies(request, m_frameID, m_pageID);
>+                restrictRequestReferrerToOriginIfNeeded(request, shouldBlockCookies);
>+            }
>+            completionHandler(WTFMove(request));
>+        });
>     else {
>         ASSERT_NOT_REACHED();
>         completionHandler({ });
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index e1a6f0d0d0d16c27415a52064109418baec8a584..4cd74da0da222db76eb33229df3db1f84edcdf5b 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,16 @@
>+2019-01-04  Youenn Fablet  <youenn@apple.com>
>+
>+        service worker fetch handler results in bad referrer
>+        https://bugs.webkit.org/show_bug.cgi?id=188248
>+        <rdar://problem/47050478>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * http/tests/security/referrer-policy-redirect-link-downgrade-expected.txt: Added.
>+        * http/tests/security/referrer-policy-redirect-link-downgrade.html: Added.
>+        * http/tests/security/resources/referrer-policy-redirect-link-downgrade.html: Added.
>+        * http/tests/security/resources/referrer-policy-redirect-link.html:
>+
> 2019-01-04  Youenn Fablet  <youenn@apple.com>
> 
>         [Fetch API] Implement abortable fetch
>diff --git a/LayoutTests/imported/w3c/ChangeLog b/LayoutTests/imported/w3c/ChangeLog
>index ce47ba8fcf13128e2c15b68bb509a55da53a28f8..4a57370caaa63ae0e3cf28209ee1fc1e7d9786c5 100644
>--- a/LayoutTests/imported/w3c/ChangeLog
>+++ b/LayoutTests/imported/w3c/ChangeLog
>@@ -1,3 +1,13 @@
>+2019-01-04  Youenn Fablet  <youenn@apple.com>
>+
>+        service worker fetch handler results in bad referrer
>+        https://bugs.webkit.org/show_bug.cgi?id=188248
>+        <rdar://problem/47050478>
>+
>+        Reviewed by NOBODY (OOPS!).
>+
>+        * web-platform-tests/service-workers/service-worker/referrer-policy-header.https-expected.txt:
>+
> 2019-01-04  Youenn Fablet  <youenn@apple.com>
> 
>         [Fetch API] Implement abortable fetch
>diff --git a/LayoutTests/http/tests/security/referrer-policy-redirect-link-downgrade-expected.txt b/LayoutTests/http/tests/security/referrer-policy-redirect-link-downgrade-expected.txt
>new file mode 100644
>index 0000000000000000000000000000000000000000..9faf90e78eb6ca6fe4a9bd6beb5bea6f47d76162
>--- /dev/null
>+++ b/LayoutTests/http/tests/security/referrer-policy-redirect-link-downgrade-expected.txt
>@@ -0,0 +1,11 @@
>+This test checks the referrer policy is obeyed along the redirect chain. The test passes if the referrer is empty as the redirect is going from HTTPS to HTTP.
>+
>+
>+
>+--------
>+Frame: 'iframe'
>+--------
>+If not running in DumpRenderTree, click this link
>+HTTP Referer header is empty
>+Referrer is empty
>+
>diff --git a/LayoutTests/http/tests/security/referrer-policy-redirect-link-downgrade.html b/LayoutTests/http/tests/security/referrer-policy-redirect-link-downgrade.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..8f66e7ec9c34e931f13a4bef91ab1d74749f98d4
>--- /dev/null
>+++ b/LayoutTests/http/tests/security/referrer-policy-redirect-link-downgrade.html
>@@ -0,0 +1,25 @@
>+<html>
>+<head>
>+<script>
>+if (window.testRunner) {
>+    testRunner.dumpAsText();
>+    testRunner.dumpChildFramesAsText();
>+    testRunner.waitUntilDone();
>+    testRunner.setCanOpenWindows();
>+    testRunner.setCloseRemainingWindowsWhenComplete(true);
>+}
>+
>+function runTest() {
>+    var iframe = document.getElementById("iframe");
>+    iframe.contentWindow.postMessage({"action": "click", "offsetLeft": iframe.offsetLeft, "offsetTop": iframe.offsetTop}, "*");
>+}
>+</script>
>+</head>
>+<body>
>+<p>
>+This test checks the referrer policy is obeyed along the redirect chain.
>+The test passes if the referrer is empty as the redirect is going from HTTPS to HTTP.
>+</p>
>+<iframe id="iframe" name="iframe" onload="runTest()" src="https://127.0.0.1:8443/security/resources/referrer-policy-redirect-link-downgrade.html"></iframe>
>+</body>
>+</html>
>diff --git a/LayoutTests/http/tests/security/resources/referrer-policy-redirect-link-downgrade.html b/LayoutTests/http/tests/security/resources/referrer-policy-redirect-link-downgrade.html
>new file mode 100644
>index 0000000000000000000000000000000000000000..fb1bf3368f509cc3ce7a48983e45d86f0293347c
>--- /dev/null
>+++ b/LayoutTests/http/tests/security/resources/referrer-policy-redirect-link-downgrade.html
>@@ -0,0 +1,27 @@
>+<html>
>+<head>
>+<meta name="referrer" content="origin" />
>+<script>
>+window.addEventListener("message", receiveMessage, false);
>+
>+function receiveMessage(evt) {
>+    if (evt.data == "done") {
>+        if (window.testRunner)
>+            testRunner.notifyDone();
>+    } else if (typeof(evt.data) == "object" && evt.data.action == "click")  {
>+        var link = document.getElementById("link");
>+        eventSender.mouseMoveTo(link.offsetLeft + evt.data.offsetLeft + 2,
>+                                link.offsetTop + evt.data.offsetTop + 2);
>+        eventSender.mouseDown();
>+        eventSender.mouseUp();
>+    } else {
>+        document.getElementById("log").innerHTML += evt.data + "<br>";
>+    }
>+}
>+</script>
>+</head>
>+<body>
>+<a id="link" target="_blank" href="https://127.0.0.1:8443/resources/redirect.php?url=http://127.0.0.1:8000/security/resources/referrer-policy-postmessage.php" rel="opener">If not running in DumpRenderTree, click this link</a>
>+<div id="log"></div>
>+</body>
>+</html>
>diff --git a/LayoutTests/http/tests/security/resources/referrer-policy-redirect-link.html b/LayoutTests/http/tests/security/resources/referrer-policy-redirect-link.html
>index fb1bf3368f509cc3ce7a48983e45d86f0293347c..23dd52624a69b7fac4a28c404ce1dc9365bf7b44 100644
>--- a/LayoutTests/http/tests/security/resources/referrer-policy-redirect-link.html
>+++ b/LayoutTests/http/tests/security/resources/referrer-policy-redirect-link.html
>@@ -21,7 +21,7 @@ function receiveMessage(evt) {
> </script>
> </head>
> <body>
>-<a id="link" target="_blank" href="https://127.0.0.1:8443/resources/redirect.php?url=http://127.0.0.1:8000/security/resources/referrer-policy-postmessage.php" rel="opener">If not running in DumpRenderTree, click this link</a>
>+<a id="link" target="_blank" href="https://127.0.0.1:8443/resources/redirect.php?url=https://127.0.0.1:8443/security/resources/referrer-policy-postmessage.php" rel="opener">If not running in DumpRenderTree, click this link</a>
> <div id="log"></div>
> </body>
> </html>
>diff --git a/LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/referrer-policy-header.https-expected.txt b/LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/referrer-policy-header.https-expected.txt
>index 812b46fec9a906b10d091f4a5b1d06ef1c3ca7e2..6df85a6f9adefeeb72ef31cc75a1a010f90059b3 100644
>--- a/LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/referrer-policy-header.https-expected.txt
>+++ b/LayoutTests/imported/w3c/web-platform-tests/service-workers/service-worker/referrer-policy-header.https-expected.txt
>@@ -1,7 +1,6 @@
> 
>-
> PASS Initialize global state (service worker registration) 
>-FAIL Referrer for a main resource redirected with referrer-policy (origin) should only have origin. assert_equals: expected "https://localhost:9443/" but got "https://localhost:9443/service-workers/service-worker/referrer-policy-header.https.html"
>+PASS Referrer for a main resource redirected with referrer-policy (origin) should only have origin. 
> FAIL Referrer for fetch requests initiated from a service worker with referrer-policy (origin) should only have origin. assert_equals: expected "finish" but got "failure:Referer for request-headers.py?url=request-headers.py must be https://localhost:9443/ but got https://localhost:9443/service-workers/service-worker/resources/fetch-rewrite-worker-referrer-policy.js"
> PASS Remove registration as a cleanup 
>

Flags:

ews-watchlist: commit-queue-

Actions: View | Formatted Diff | Diff

Attachments on bug 188248: 358364 | 358386 | 358395 | 358402 | 358410 | 358416 | 358419 | 358424 | 358428 | 358429 | 358452 | 358454