Changeset 245441 in webkit

Timestamp:

May 17, 2019, 3:14:50 AM (7 years ago)

Author:

Carlos Garcia Campos

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.

Location:

releases/WebKitGTK/webkit-2.24

Files:

• LayoutTests/ChangeLog (modified) (1 diff)
• LayoutTests/http/tests/cookies/same-site/user-load-cross-site-redirect-expected.txt (added)
• LayoutTests/http/tests/cookies/same-site/user-load-cross-site-redirect.php (added)
• Source/WebCore/ChangeLog (modified) (1 diff)
• Source/WebCore/loader/FrameLoader.cpp (modified) (2 diffs)
• Source/WebCore/platform/network/ResourceRequestBase.cpp (modified) (1 diff)

releases/WebKitGTK/webkit-2.24/LayoutTests/ChangeLog

@@ +1 @@
+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-04-03  Myles C. Maxfield  <mmaxfield@apple.com>
 

releases/WebKitGTK/webkit-2.24/Source/WebCore/ChangeLog

@@ +1 @@
+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-04-03  Myles C. Maxfield  <mmaxfield@apple.com>
 

releases/WebKitGTK/webkit-2.24/Source/WebCore/loader/FrameLoader.cpp

@@ -2868 +2868 @@
     // 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())
@@ -2886 +2887 @@
         }
         addSameSiteInfoToRequestIfNeeded(request, initiator);
-        request.setIsTopSite(isMainResource && m_frame.isMainFrame());
-    }
+    }
+    request.setIsTopSite(isMainFrameMainResource);
 
     Page* page = frame().page();

releases/WebKitGTK/webkit-2.24/Source/WebCore/platform/network/ResourceRequestBase.cpp

@@ -71 +71 @@
         setInspectorInitiatorNodeIdentifier(*inspectorInitiatorNodeIdentifier);
 
-    if (!other.isSameSiteUnspecified()) {
+    if (!other.isSameSiteUnspecified())
         setIsSameSite(other.isSameSite());
-        setIsTopSite(other.isTopSite());
-    }
+    setIsTopSite(other.isTopSite());
 
     updateResourceRequest();