Same Site Lax cookies are not sent with cross-site redirect from client-initiated...
authordbates@webkit.org <dbates@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 21 Feb 2019 23:54:40 +0000 (23:54 +0000)
committerdbates@webkit.org <dbates@webkit.org@268f45cc-cd09-0410-ab3c-d52691b4dbfc>
Thu, 21 Feb 2019 23:54:40 +0000 (23:54 +0000)
https://bugs.webkit.org/show_bug.cgi?id=194906
<rdar://problem/44305947>

Reviewed by Brent Fulgham.

Source/WebCore:

Ensure that a request for a top-level navigation is annotated as such regardless of whether
the request has a computed Same Site policy.

"New loads" initiated by a the client (Safari) either by API or a human either explicitly
typing a URL in the address bar or Command + clicking a hyperlink to open it in a new window/tab
are always considered Same Site. This is by definition from the spec. [1] as we aren't navigating
from an existing page. (Command + click should be thought of as a convenience to the user from
having to copy the hyperlink's URL, create a new window, and paste the URL into the address bar).
Currently the frame loader marks a request as a top-level navigation if and only if the request
does not have a pre-computed Same Site policy. However, "New loads" have a pre-computed Same Site
policy. So, these loads would never be marked as a top-level navigation by the frame loading code.
Therefore, if the "new load" turned out to be a cross-site redirect then WebKit would incorrectly
tell the networking stack that the load was a cross-site, non-top-level navigation, and per the
Same Site spec [2], the networking stack would not send Same Site Lax cookies. Instead,
WebKit should unconditionally ensure that requests are marked as a top-level navigation, if applicable.

[1] See Note for (1) in  <https://tools.ietf.org/html/draft-ietf-httpbis-rfc6265bis-02#section-5.2>
[2] <https://tools.ietf.org/html/draft-ietf-httpbis-rfc6265bis-02#section-5.3.7.1>

Test: http/tests/cookies/same-site/user-load-cross-site-redirect.php

* loader/FrameLoader.cpp:
(WebCore::FrameLoader::addExtraFieldsToRequest): Unconditionally update the request's top-
level navigation bit.
* platform/network/ResourceRequestBase.cpp:
(WebCore::ResourceRequestBase::setAsIsolatedCopy): Unconditionally copy a request's top-
level navigation bit.

LayoutTests:

Add a test that is representative of a user loading a cross-site page that redirects
to a page that expects Same Site Lax cookies.

* http/tests/cookies/same-site/user-load-cross-site-redirect-expected.txt: Added.
* http/tests/cookies/same-site/user-load-cross-site-redirect.php: Added.

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

LayoutTests/ChangeLog
LayoutTests/http/tests/cookies/same-site/user-load-cross-site-redirect-expected.txt [new file with mode: 0644]
LayoutTests/http/tests/cookies/same-site/user-load-cross-site-redirect.php [new file with mode: 0644]
Source/WebCore/ChangeLog
Source/WebCore/loader/FrameLoader.cpp
Source/WebCore/platform/network/ResourceRequestBase.cpp

index 38f2fa0..98fcb60 100644 (file)
@@ -1,3 +1,17 @@
+2019-02-21  Daniel Bates  <dabates@apple.com>
+
+        Same Site Lax cookies are not sent with cross-site redirect from client-initiated load
+        https://bugs.webkit.org/show_bug.cgi?id=194906
+        <rdar://problem/44305947>
+
+        Reviewed by Brent Fulgham.
+
+        Add a test that is representative of a user loading a cross-site page that redirects
+        to a page that expects Same Site Lax cookies.
+
+        * http/tests/cookies/same-site/user-load-cross-site-redirect-expected.txt: Added.
+        * http/tests/cookies/same-site/user-load-cross-site-redirect.php: Added.
+
 2019-02-21  Per Arne Vollan  <pvollan@apple.com>
 
         Layout Test fast/text/international/khmer-selection.html is crashing
diff --git a/LayoutTests/http/tests/cookies/same-site/user-load-cross-site-redirect-expected.txt b/LayoutTests/http/tests/cookies/same-site/user-load-cross-site-redirect-expected.txt
new file mode 100644 (file)
index 0000000..b0f29fd
--- /dev/null
@@ -0,0 +1,18 @@
+This test is representative of a user that loads a site, via the address bar or Command + clicking a hyperlink, that redirects to a cross-site page that expects its SameSite Lax cookies.
+
+On success, you will see a series of "PASS" messages, followed by "TEST COMPLETE".
+
+
+Cookies sent with HTTP request:
+PASS Do not have cookie "strict".
+PASS Has cookie "lax" with value 27.
+PASS Has cookie "normal" with value 27.
+
+Cookies visible in DOM:
+PASS Do not have DOM cookie "strict".
+PASS Has DOM cookie "lax" with value 27.
+PASS Has DOM cookie "normal" with value 27.
+PASS successfullyParsed is true
+
+TEST COMPLETE
+
diff --git a/LayoutTests/http/tests/cookies/same-site/user-load-cross-site-redirect.php b/LayoutTests/http/tests/cookies/same-site/user-load-cross-site-redirect.php
new file mode 100644 (file)
index 0000000..ca1ffa7
--- /dev/null
@@ -0,0 +1,45 @@
+<?php
+    include_once("../resources/cookie-utilities.php");
+
+    if (hostnameIsEqualToString("127.0.0.1") && empty($_SERVER["QUERY_STRING"])) {
+        wkSetCookie("strict", "27", Array("SameSite" => "Strict", "Max-Age" => 100, "path" => "/"));
+        wkSetCookie("lax", "27", Array("SameSite" => "Lax", "Max-Age" => 100, "path" => "/"));
+        wkSetCookie("normal", "27", Array("Max-Age" => 100, "path" => "/"));
+        header("Location: http://localhost:8000/resources/redirect.php?url=" . urlencode("http://127.0.0.1:8000" . $_SERVER["REQUEST_URI"] . "?check-cookies"));
+        exit(0);
+    }
+?>
+<!DOCTYPE html>
+<html>
+<head>
+<script src="/js-test-resources/js-test.js"></script>
+<script src="../resources/cookie-utilities.js"></script>
+<script>_setCachedCookiesJSON('<?php echo json_encode($_COOKIE); ?>')</script>
+</head>
+<body>
+<script>
+window.jsTestIsAsync = true;
+
+description("This test is representative of a user that loads a site, via the address bar or Command + clicking a hyperlink, that redirects to a cross-site page that expects its SameSite Lax cookies.");
+
+async function checkResult()
+{
+    debug("Cookies sent with HTTP request:");
+    await shouldNotHaveCookie("strict");
+    await shouldHaveCookieWithValue("lax", "27");
+    await shouldHaveCookieWithValue("normal", "27");
+
+    debug("<br>Cookies visible in DOM:");
+    shouldNotHaveDOMCookie("strict");
+    shouldHaveDOMCookieWithValue("lax", "27");
+    shouldHaveDOMCookieWithValue("normal", "27");
+
+    await resetCookies();
+    finishJSTest();
+}
+
+checkResult();
+</script>
+</body>
+</html>
+
index c21b2f1..cd9049c 100644 (file)
@@ -1,3 +1,39 @@
+2019-02-21  Daniel Bates  <dabates@apple.com>
+
+        Same Site Lax cookies are not sent with cross-site redirect from client-initiated load
+        https://bugs.webkit.org/show_bug.cgi?id=194906
+        <rdar://problem/44305947>
+
+        Reviewed by Brent Fulgham.
+
+        Ensure that a request for a top-level navigation is annotated as such regardless of whether
+        the request has a computed Same Site policy.
+
+        "New loads" initiated by a the client (Safari) either by API or a human either explicitly
+        typing a URL in the address bar or Command + clicking a hyperlink to open it in a new window/tab
+        are always considered Same Site. This is by definition from the spec. [1] as we aren't navigating
+        from an existing page. (Command + click should be thought of as a convenience to the user from
+        having to copy the hyperlink's URL, create a new window, and paste the URL into the address bar).
+        Currently the frame loader marks a request as a top-level navigation if and only if the request
+        does not have a pre-computed Same Site policy. However, "New loads" have a pre-computed Same Site
+        policy. So, these loads would never be marked as a top-level navigation by the frame loading code.
+        Therefore, if the "new load" turned out to be a cross-site redirect then WebKit would incorrectly
+        tell the networking stack that the load was a cross-site, non-top-level navigation, and per the
+        Same Site spec [2], the networking stack would not send Same Site Lax cookies. Instead,
+        WebKit should unconditionally ensure that requests are marked as a top-level navigation, if applicable.
+
+        [1] See Note for (1) in  <https://tools.ietf.org/html/draft-ietf-httpbis-rfc6265bis-02#section-5.2>
+        [2] <https://tools.ietf.org/html/draft-ietf-httpbis-rfc6265bis-02#section-5.3.7.1>
+
+        Test: http/tests/cookies/same-site/user-load-cross-site-redirect.php
+
+        * loader/FrameLoader.cpp:
+        (WebCore::FrameLoader::addExtraFieldsToRequest): Unconditionally update the request's top-
+        level navigation bit.
+        * platform/network/ResourceRequestBase.cpp:
+        (WebCore::ResourceRequestBase::setAsIsolatedCopy): Unconditionally copy a request's top-
+        level navigation bit.
+
 2019-02-21  Per Arne Vollan  <pvollan@apple.com>
 
         Layout Test fast/text/international/khmer-selection.html is crashing
index 109b7d3..766b7e1 100644 (file)
@@ -2867,8 +2867,9 @@ void FrameLoader::addExtraFieldsToRequest(ResourceRequest& request, FrameLoadTyp
 
     // Don't set the cookie policy URL if it's already been set.
     // But make sure to set it on all requests regardless of protocol, as it has significance beyond the cookie policy (<rdar://problem/6616664>).
+    bool isMainFrameMainResource = isMainResource && m_frame.isMainFrame();
     if (request.firstPartyForCookies().isEmpty()) {
-        if (isMainResource && m_frame.isMainFrame())
+        if (isMainFrameMainResource)
             request.setFirstPartyForCookies(request.url());
         else if (Document* document = m_frame.document())
             request.setFirstPartyForCookies(document->firstPartyForCookies());
@@ -2885,8 +2886,8 @@ void FrameLoader::addExtraFieldsToRequest(ResourceRequest& request, FrameLoadTyp
             ASSERT(ownerFrame || m_frame.isMainFrame());
         }
         addSameSiteInfoToRequestIfNeeded(request, initiator);
-        request.setIsTopSite(isMainResource && m_frame.isMainFrame());
     }
+    request.setIsTopSite(isMainFrameMainResource);
 
     Page* page = frame().page();
     bool hasSpecificCachePolicy = request.cachePolicy() != ResourceRequestCachePolicy::UseProtocolCachePolicy;
index 625ac51..d540f44 100644 (file)
@@ -70,10 +70,9 @@ void ResourceRequestBase::setAsIsolatedCopy(const ResourceRequest& other)
     if (auto inspectorInitiatorNodeIdentifier = other.inspectorInitiatorNodeIdentifier())
         setInspectorInitiatorNodeIdentifier(*inspectorInitiatorNodeIdentifier);
 
-    if (!other.isSameSiteUnspecified()) {
+    if (!other.isSameSiteUnspecified())
         setIsSameSite(other.isSameSite());
-        setIsTopSite(other.isTopSite());
-    }
+    setIsTopSite(other.isTopSite());
 
     updateResourceRequest();
     m_httpHeaderFields = other.httpHeaderFields().isolatedCopy();