- Revision
- 245441
- Author
- [email protected]
- Date
- 2019-05-17 03:14:50 -0700 (Fri, 17 May 2019)
Log Message
Merge r241918 - 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.
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.
Modified Paths
Added Paths
Diff
Modified: releases/WebKitGTK/webkit-2.24/LayoutTests/ChangeLog (245440 => 245441)
--- releases/WebKitGTK/webkit-2.24/LayoutTests/ChangeLog 2019-05-17 10:14:44 UTC (rev 245440)
+++ releases/WebKitGTK/webkit-2.24/LayoutTests/ChangeLog 2019-05-17 10:14:50 UTC (rev 245441)
@@ -1,3 +1,17 @@
+2019-02-21 Daniel Bates <[email protected]>
+
+ 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-04-03 Myles C. Maxfield <[email protected]>
-apple-trailing-word is needed for browser detection
Added: releases/WebKitGTK/webkit-2.24/LayoutTests/http/tests/cookies/same-site/user-load-cross-site-redirect-expected.txt (0 => 245441)
--- releases/WebKitGTK/webkit-2.24/LayoutTests/http/tests/cookies/same-site/user-load-cross-site-redirect-expected.txt (rev 0)
+++ releases/WebKitGTK/webkit-2.24/LayoutTests/http/tests/cookies/same-site/user-load-cross-site-redirect-expected.txt 2019-05-17 10:14:50 UTC (rev 245441)
@@ -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
+
Added: releases/WebKitGTK/webkit-2.24/LayoutTests/http/tests/cookies/same-site/user-load-cross-site-redirect.php (0 => 245441)
--- releases/WebKitGTK/webkit-2.24/LayoutTests/http/tests/cookies/same-site/user-load-cross-site-redirect.php (rev 0)
+++ releases/WebKitGTK/webkit-2.24/LayoutTests/http/tests/cookies/same-site/user-load-cross-site-redirect.php 2019-05-17 10:14:50 UTC (rev 245441)
@@ -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=""
+<script src=""
+<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>
+
Modified: releases/WebKitGTK/webkit-2.24/Source/WebCore/ChangeLog (245440 => 245441)
--- releases/WebKitGTK/webkit-2.24/Source/WebCore/ChangeLog 2019-05-17 10:14:44 UTC (rev 245440)
+++ releases/WebKitGTK/webkit-2.24/Source/WebCore/ChangeLog 2019-05-17 10:14:50 UTC (rev 245441)
@@ -1,3 +1,39 @@
+2019-02-21 Daniel Bates <[email protected]>
+
+ 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-04-03 Myles C. Maxfield <[email protected]>
-apple-trailing-word is needed for browser detection
Modified: releases/WebKitGTK/webkit-2.24/Source/WebCore/loader/FrameLoader.cpp (245440 => 245441)
--- releases/WebKitGTK/webkit-2.24/Source/WebCore/loader/FrameLoader.cpp 2019-05-17 10:14:44 UTC (rev 245440)
+++ releases/WebKitGTK/webkit-2.24/Source/WebCore/loader/FrameLoader.cpp 2019-05-17 10:14:50 UTC (rev 245441)
@@ -2867,8 +2867,9 @@
// 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 @@
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;
Modified: releases/WebKitGTK/webkit-2.24/Source/WebCore/platform/network/ResourceRequestBase.cpp (245440 => 245441)
--- releases/WebKitGTK/webkit-2.24/Source/WebCore/platform/network/ResourceRequestBase.cpp 2019-05-17 10:14:44 UTC (rev 245440)
+++ releases/WebKitGTK/webkit-2.24/Source/WebCore/platform/network/ResourceRequestBase.cpp 2019-05-17 10:14:50 UTC (rev 245441)
@@ -70,10 +70,9 @@
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();
