Attachment 358452 Details for Bug 188248 – Patch

[patch] Patch

bug-188248-20190105190657.patch (text/plain), 18.66 KB, created by youenn fablet on 2019-01-05 19:06:58 PST

(hide)

Description:

Filename:

MIME Type:

Creator: youenn fablet

Created: 2019-01-05 19:06:58 PST

Size: 18.66 KB

patch

obsolete

>Subversion Revision: 239660
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 55b71dc9deb21074a32e10417dfd9dbcea258561..81a0a1d578c7c312229f59a16d70986d06067efa 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -1,3 +1,23 @@
>+2019-01-05  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-05  Youenn Fablet  <youenn@apple.com>
> 
>         Service Worker fetch should obey its referrer policy
>diff --git a/Source/WebKit/ChangeLog b/Source/WebKit/ChangeLog
>index 0335e45c08e7d7881ce142c900c30d6847633919..f0458f361c98a846de88c0bb34bc4aef39c04688 100644
>--- a/Source/WebKit/ChangeLog
>+++ b/Source/WebKit/ChangeLog
>@@ -1,3 +1,22 @@
>+2019-01-05  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  Joseph Pecoraro  <pecoraro@apple.com>
> 
>         Web Inspector: Use save sheet instead of dialog where possible
>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 1ce9b3abfe75aca76d0c1be9a9caec47c2092082..144549179235f2b2a95a3306d0fcd51751657023 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -1,3 +1,18 @@
>+2019-01-05  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:
>+        * platform/ios-wk2/TestExpectations: Skip referrer-policy-redirect-link-downgrade.html
>+        as it is equivalent to previously skipped referrer-policy-redirect-link.html.
>+
> 2019-01-05  Commit Queue  <commit-queue@webkit.org>
> 
>         Unreviewed, rolling out r239607.
>diff --git a/LayoutTests/imported/w3c/ChangeLog b/LayoutTests/imported/w3c/ChangeLog
>index 60ce6517d77823c4bbca6d055fad9a3c78e9e667..b6d8407610d29411711a0679c9086b8af9108db1 100644
>--- a/LayoutTests/imported/w3c/ChangeLog
>+++ b/LayoutTests/imported/w3c/ChangeLog
>@@ -1,3 +1,13 @@
>+2019-01-05  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-05  Youenn Fablet  <youenn@apple.com>
> 
>         Service Worker fetch should obey its referrer policy
>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 2ea4aded04f513ea08ac60820891f62bf0390210..a4f692904444ee0a7555f83d516ce48adaad1031 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. 
> PASS Referrer for fetch requests initiated from a service worker with referrer-policy (origin) should only have origin. 
> PASS Remove registration as a cleanup 
> 
>diff --git a/LayoutTests/platform/ios-wk2/TestExpectations b/LayoutTests/platform/ios-wk2/TestExpectations
>index c36183dda7a10cd08b0519fd31dabc5bc8df56bf..5b06c3c501af9b98091f33cba6f85e0f3bbabac7 100644
>--- a/LayoutTests/platform/ios-wk2/TestExpectations
>+++ b/LayoutTests/platform/ios-wk2/TestExpectations
>@@ -354,7 +354,7 @@ http/tests/security/javascriptURL/xss-ALLOWED-from-javascript-url-window-open.ht
> http/tests/security/javascriptURL/xss-ALLOWED-to-javascript-url-window-open.html
> http/tests/security/mixedContent/about-blank-iframe-in-main-frame.html
> http/tests/security/mixedContent/redirect-http-to-https-iframe-in-main-frame.html
>-http/tests/security/referrer-policy-redirect-link.html
>+http/tests/security/referrer-policy-redirect-link-downgrade.html
> http/tests/security/referrer-policy-rel-noreferrer.html
> http/tests/security/video-cross-origin-accessfailure.html
> http/tests/security/video-cross-origin-accesssameorigin.html

Actions: View | Formatted Diff | Diff

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