Bug 194906 - Same Site Lax cookies are not sent with cross-site redirect from client-initiated load
Summary: Same Site Lax cookies are not sent with cross-site redirect from client-initi...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Page Loading (show other bugs)
Version: WebKit Local Build
Hardware: All All
: P2 Normal
Assignee: Daniel Bates
URL:
Keywords: InRadar
: 196375 (view as bug list)
Depends on:
Blocks:
 
Reported: 2019-02-21 10:08 PST by Daniel Bates
Modified: 2019-03-28 15:56 PDT (History)
9 users (show)

See Also:


Attachments
Patch and layout test (8.27 KB, patch)
2019-02-21 10:31 PST, Daniel Bates
bfulgham: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Daniel Bates 2019-02-21 10:08:41 PST
You can see this using Twitter. Here is one way to trigger this bug:

1. Sign into Twitter.com.
2. Visit <https://iosdevweekly.com/issues/388#HfB24p3>.
3. Command + click the twitter.com hyperlink

Then the opened page will display the error message "403 Forbidden: Then server understood the request, but is refusing to fulfill it." But you should see the linked tweet.
Comment 1 Daniel Bates 2019-02-21 10:08:54 PST
<rdar://problem/44305947>
Comment 2 Daniel Bates 2019-02-21 10:31:59 PST
Created attachment 362618 [details]
Patch and layout test
Comment 3 Brent Fulgham 2019-02-21 11:26:09 PST
Comment on attachment 362618 [details]
Patch and layout test

View in context: https://bugs.webkit.org/attachment.cgi?id=362618&action=review

Looks good. I thought of some ideas for possible future improvements (see comments).

> Source/WebCore/loader/FrameLoader.cpp:2889
> +    request.setIsTopSite(isMainResource && m_frame.isMainFrame());

It looks like the concept of 'isTopSite' is used elsewhere in FrameLoader (e.g., L2871). I wonder if we should have a helper:

bool isTopSite(bool isMainResource) const  { return isMainResource && m_frame.isMainFrame(); }

> LayoutTests/http/tests/cookies/same-site/user-load-cross-site-redirect-expected.txt:9
> +PASS Has cookie "normal" with value 27.

Do we have a test for Same-Site HTTP-only cookies that are visible servers, but are not exposed to DOM? That's not really relevant to this Lax cookie case, but would be good to make sure we never regress HTTP-only cookies being isolated from the WebContent process.
Comment 4 Daniel Bates 2019-02-21 15:53:19 PST
Comment on attachment 362618 [details]
Patch and layout test

View in context: https://bugs.webkit.org/attachment.cgi?id=362618&action=review

>> Source/WebCore/loader/FrameLoader.cpp:2889
>> +    request.setIsTopSite(isMainResource && m_frame.isMainFrame());
> 
> It looks like the concept of 'isTopSite' is used elsewhere in FrameLoader (e.g., L2871). I wonder if we should have a helper:
> 
> bool isTopSite(bool isMainResource) const  { return isMainResource && m_frame.isMainFrame(); }

Will clean up using a local variable, isMainResource, is a param specific to this function.

>> LayoutTests/http/tests/cookies/same-site/user-load-cross-site-redirect-expected.txt:9
>> +PASS Has cookie "normal" with value 27.
> 
> Do we have a test for Same-Site HTTP-only cookies that are visible servers, but are not exposed to DOM? That's not really relevant to this Lax cookie case, but would be good to make sure we never regress HTTP-only cookies being isolated from the WebContent process.

No, we don't.

$ grep -Rli "httpOnly" LayoutTests/http/
LayoutTests/http//tests/websocket/tests/hybi/cookie_wsh.py
LayoutTests/http//tests/websocket/tests/hybi/secure-cookie-secure-connection.pl
LayoutTests/http//tests/websocket/tests/hybi/httponly-cookie.pl
LayoutTests/http//tests/websocket/tests/hybi/contentextensions/block-cookies-worker.php
LayoutTests/http//tests/websocket/tests/hybi/contentextensions/block-cookies.php
LayoutTests/http//tests/websocket/tests/hybi/httponly-cookie-expected.txt
LayoutTests/http//tests/inspector/network/har/har-page.html
LayoutTests/http//tests/inspector/network/har/har-page-expected.txt
LayoutTests/http//tests/appcache/document-cookie-http-only-expected.txt
LayoutTests/http//tests/appcache/document-cookie-http-only.php
LayoutTests/http//tests/cookies/js-get-and-set-http-only-cookie.html
LayoutTests/http//tests/cookies/js-get-and-set-http-only-cookie-expected.txt

Lacking test coverage for HTTP only cookies in general. Anyway, this is tangential to this bug. So, I suggest we solve the test shortage in another. Feel free to open such a bug.
Comment 5 Daniel Bates 2019-02-21 15:54:48 PST
Committed r241918: <https://trac.webkit.org/changeset/241918>
Comment 6 Truitt Savell 2019-02-21 17:04:36 PST
The new test http/tests/cookies/same-site/user-load-cross-site-redirect.php

Added in https://trac.webkit.org/changeset/241918/webkit

is failing on Mojave WK1. History:
http://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.html#showAllRuns=true&tests=http%2Ftests%2Fcookies%2Fsame-site%2Fuser-load-cross-site-redirect.php

Diff:
--- /Volumes/Data/slave/mojave-release-tests-wk1/build/layout-test-results/http/tests/cookies/same-site/user-load-cross-site-redirect-expected.txt
+++ /Volumes/Data/slave/mojave-release-tests-wk1/build/layout-test-results/http/tests/cookies/same-site/user-load-cross-site-redirect-actual.txt
@@ -4,7 +4,7 @@
 
 
 Cookies sent with HTTP request:
-PASS Do not have cookie "strict".
+FAIL Should not have cookie "strict". But do with value 27.
 PASS Has cookie "lax" with value 27.
 PASS Has cookie "normal" with value 27.
 
@@ -13,6 +13,7 @@
 PASS Has DOM cookie "lax" with value 27.
 PASS Has DOM cookie "normal" with value 27.
 PASS successfullyParsed is true
+Some tests failed.
 
 TEST COMPLETE
Comment 7 Daniel Bates 2019-02-21 22:48:19 PST
(In reply to Truitt Savell from comment #6)
> The new test http/tests/cookies/same-site/user-load-cross-site-redirect.php
> 
> Added in https://trac.webkit.org/changeset/241918/webkit
> 
> is failing on Mojave WK1. History:
> http://webkit-test-results.webkit.org/dashboards/flakiness_dashboard.
> html#showAllRuns=true&tests=http%2Ftests%2Fcookies%2Fsame-site%2Fuser-load-
> cross-site-redirect.php
> 
> Diff:
> ---
> /Volumes/Data/slave/mojave-release-tests-wk1/build/layout-test-results/http/
> tests/cookies/same-site/user-load-cross-site-redirect-expected.txt
> +++
> /Volumes/Data/slave/mojave-release-tests-wk1/build/layout-test-results/http/
> tests/cookies/same-site/user-load-cross-site-redirect-actual.txt
> @@ -4,7 +4,7 @@
>  
>  
>  Cookies sent with HTTP request:
> -PASS Do not have cookie "strict".
> +FAIL Should not have cookie "strict". But do with value 27.
>  PASS Has cookie "lax" with value 27.
>  PASS Has cookie "normal" with value 27.
>  
> @@ -13,6 +13,7 @@
>  PASS Has DOM cookie "lax" with value 27.
>  PASS Has DOM cookie "normal" with value 27.
>  PASS successfullyParsed is true
> +Some tests failed.
>  
>  TEST COMPLETE

This failure reveals an existing bug. That is, this patch did not cause a regression. As the primary purpose of this test and this bug is the behavior of Same Site Lax cookies I have amended the test and test result to skip the failing sub-test for now to get the bots green. Committed fix in <https://trac.webkit.org/changeset/241931>. Will investigate the sub-test failure in bug #194933.
Comment 8 Chris Dumez 2019-03-28 15:56:42 PDT
*** Bug 196375 has been marked as a duplicate of this bug. ***