Bug 194906 – Same Site Lax cookies are not sent with cross-site redirect from client-initiated load

• Status: RESOLVED
• Resolution: FIXED
• Component: Page Loading

[Daniel Bates]

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.

[Daniel Bates]

<rdar://problem/44305947>

[Daniel Bates]

Created attachment 362618 [details]
Patch and layout test

[Brent Fulgham]

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.

[Daniel Bates]

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.

[Daniel Bates]

Committed r241918: <https://trac.webkit.org/changeset/241918>

[Truitt Savell]

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

[Daniel Bates]

(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.

[Chris Dumez]

*** Bug 196375 has been marked as a duplicate of this bug. ***