Attachment 362618 Details for Bug 194906 – Patch and layout test

[patch] Patch and layout test

bug-194906-20190221103156.patch (text/plain), 8.27 KB, created by Daniel Bates on 2019-02-21 10:31:59 PST

(hide)

Description:

Filename:

MIME Type:

Creator: Daniel Bates

Created: 2019-02-21 10:31:59 PST

Size: 8.27 KB

patch

obsolete

>Subversion Revision: 241860
>diff --git a/Source/WebCore/ChangeLog b/Source/WebCore/ChangeLog
>index 26277ea5e6422ed0ab34dfff421fa7cbc3266123..d6657dcb35346b6b1ba53b5c4b0e41867076f0b3 100644
>--- a/Source/WebCore/ChangeLog
>+++ b/Source/WebCore/ChangeLog
>@@ -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 NOBODY (OOPS!).
>+
>+        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-20  Simon Fraser  <simon.fraser@apple.com>
> 
>         REGRESSION (240698): Fixed position banners flicker and move when scrolling on iOS
>diff --git a/Source/WebCore/loader/FrameLoader.cpp b/Source/WebCore/loader/FrameLoader.cpp
>index 109b7d355d142531c42506ee71f5fd620cace02b..a611dc7437380220d933b554d7963cfcf5152cfa 100644
>--- a/Source/WebCore/loader/FrameLoader.cpp
>+++ b/Source/WebCore/loader/FrameLoader.cpp
>@@ -2885,8 +2885,8 @@ void FrameLoader::addExtraFieldsToRequest(ResourceRequest& request, FrameLoadTyp
>             ASSERT(ownerFrame || m_frame.isMainFrame());
>         }
>         addSameSiteInfoToRequestIfNeeded(request, initiator);
>-        request.setIsTopSite(isMainResource && m_frame.isMainFrame());
>     }
>+    request.setIsTopSite(isMainResource && m_frame.isMainFrame());
> 
>     Page* page = frame().page();
>     bool hasSpecificCachePolicy = request.cachePolicy() != ResourceRequestCachePolicy::UseProtocolCachePolicy;
>diff --git a/Source/WebCore/platform/network/ResourceRequestBase.cpp b/Source/WebCore/platform/network/ResourceRequestBase.cpp
>index 625ac51f7e39c19e32fbcb52d62a74ffecc1b273..d540f44c3bd2944408e67dc5227a47ec341c0527 100644
>--- a/Source/WebCore/platform/network/ResourceRequestBase.cpp
>+++ b/Source/WebCore/platform/network/ResourceRequestBase.cpp
>@@ -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();
>diff --git a/LayoutTests/ChangeLog b/LayoutTests/ChangeLog
>index f344270aabe0148ae93eef4e820766545e8b4ee9..82163afd343f35c7240ed2558ae2cb663e9a250f 100644
>--- a/LayoutTests/ChangeLog
>+++ b/LayoutTests/ChangeLog
>@@ -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 NOBODY (OOPS!).
>+
>+        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-20  Antti Koivisto  <antti@apple.com>
> 
>         Make programmatic frame scrolling work on iOS
>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
>index 0000000000000000000000000000000000000000..b0f29fd449f22bc5def7f34ec77d765f5144daea
>--- /dev/null
>+++ b/LayoutTests/http/tests/cookies/same-site/user-load-cross-site-redirect-expected.txt
>@@ -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
>index 0000000000000000000000000000000000000000..ca1ffa7b7bd5a8fd7114283756c951b4b0baee56
>--- /dev/null
>+++ b/LayoutTests/http/tests/cookies/same-site/user-load-cross-site-redirect.php
>@@ -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>
>+

Flags:

bfulgham: review+

Actions: View | Formatted Diff | Diff

Attachments on bug 194906: 362618