Bug 226386 - Same-site lax cookies not sent by fetch event handler after page reload
Summary: Same-site lax cookies not sent by fetch event handler after page reload
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: Service Workers (show other bugs)
Version: Safari 14
Hardware: Unspecified Unspecified
: P2 Normal
Assignee: youenn fablet
URL:
Keywords: InRadar
: 232440 (view as bug list)
Depends on:
Blocks:
 
Reported: 2021-05-28 11:44 PDT by Steffen Weber
Modified: 2022-02-04 12:20 PST (History)
15 users (show)

See Also:


Attachments
Patch (5.65 KB, patch)
2021-06-07 03:04 PDT, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (12.50 KB, patch)
2021-12-07 06:34 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch (96.42 KB, patch)
2021-12-08 00:46 PST, youenn fablet
ews-feeder: commit-queue-
Details | Formatted Diff | Diff
Patch (16.08 KB, patch)
2021-12-08 01:28 PST, youenn fablet
no flags Details | Formatted Diff | Diff
Patch for landing (16.09 KB, patch)
2021-12-08 07:40 PST, youenn fablet
no flags Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Steffen Weber 2021-05-28 11:44:39 PDT
Requests sent by Safari lack SameSite=Lax cookies when a service worker with a fetch event handler is involved.

How to reproduce:

1. Fetch a SameSite=Lax demo cookie by visiting https://www.computerbase.de/api/samesite-demo-cookie (*)
2. Search Google for "computerbase"
3. Open the "Network" tab of the developer tools
4. Click on the search result that leads to our homepage (may not the first one depending on your location)
5. In the "Network" tab, find the "document" request and have a look at the cookies sent by Safari. For now, the "samesite-demo-cookie" is there.
6. Now reload our homepage. (Maybe this causes our Service Worker to fully initialize / activate?)
7. Repeat steps 2-4. Now the cookie doesn't get sent anymore.

(*) I've created this script because other cookies set by ComputerBase don't use SameSite=Lax in Safari anymore because of this bug.

The fetch event handler of our service worker doesn't handle all requests, for some requests it just returns "null" to let the browser handle the request (as if the service worker didn't exist). For example, the fetch event handler of our service worker returns "null" for all forum URLs (all URLs starting with "https://www.computerbase.de/forum/"). It turns out that forum URLs have no problem with same-site cookies: When I click on the search results entry for our forum (should be displayed when searching for "computerbase", if not then search for "computerbase forum") then the same-site demo cookie is always there as expected. If I then go back to the SERP and click on our homepage link again, the cookie is missing. Go back to the SERP and click on our forum link again, the cookie is there again. And so on...

So we have the following: 1) Same-site cookie is sent to our homepage on first try, bot not anymore after one or two reloads (probably after the service-worker has been initialized and our fetch event handler is used). 2) Same-site cookie is always sent to our forum pages who are ignored by the fetch event handler.

I think the combination of these two observations indicates that there is an issue with same-site cookies for requests sent by the fetch event handler.

Split from Bug #219650 as suggested by John Wilander.
Comment 1 John Wilander 2021-05-28 11:55:20 PDT
Thanks, Steffen! Cc’ing Youenn who knows most about Fetch and ServiceWorkers. Youenn, I can assist on the SameSite cookie part.
Comment 2 youenn fablet 2021-06-02 12:15:14 PDT
Hi Steffen, did you notice the lack of cookies from service worker by using web inspector or by noticing issues with the document being received or through server logs?
If it is web inspector only, I am wondering whether we do not have an issue retrieving  request/response cookie information when requests/responses go through the service worker.
Comment 3 youenn fablet 2021-06-02 12:15:50 PDT
Sadly, debugging service worker in STP seems broken, I do not see service workers be listed in the debug menu even though they are serving content.
Comment 4 youenn fablet 2021-06-02 12:17:04 PDT
Per Arne, I know there was a regression that we fixed some time ago where sandbox hardening broke service worker debugging. Maybe the issue came back.
Bisecting might be feasible here.
Comment 5 Steffen Weber 2021-06-03 12:26:28 PDT
Initially, I only looked at the Safari Developer Tools to check the sent cookies but I've now verified that they are indeed not sent (by temporarily modifying our homepage to dump all cookies for requests originating at a specific BrowserStack IP address).

Btw, I'm able to reproduce the issue by simply repeating step 6: The same-site cookie is gone after the second reload. Step 7 is not necessary.
Comment 6 Radar WebKit Bug Importer 2021-06-04 11:45:19 PDT
<rdar://problem/78878853>
Comment 7 youenn fablet 2021-06-07 03:04:57 PDT
Created attachment 430733 [details]
Patch
Comment 8 youenn fablet 2021-06-07 03:07:09 PDT
Steffen, I wrote the following test that is using same site cookie and everything seems to work as expected from this test.
I used 'secure; HttpOnly; SameSite=Lax' as seems to be the case in your website.

Let me know if you can spot a difference with your setup.

As can be seen, the cookies are not provided in web inspector when doing a fetch through service worker. This should probably be fixed in a different bug.
Comment 9 youenn fablet 2021-06-08 02:30:47 PDT
Comment on attachment 430733 [details]
Patch

Might be nice to have a test landed so adding r?
Steffen, please let me know if there is any difference with your setup.
Comment 10 Steffen Weber 2021-06-11 02:51:27 PDT
I'm on vacation. I'll have another look at the end of June. Sorry!
Comment 11 Steffen Weber 2021-06-24 04:58:54 PDT
I'm not familiar with Web Platform Tests but does the test actually simulate the process of navigating away from the site and back to the site via an external link? This is the crucial step that triggers the issue.

It definitely isn't just a cosmetic Developer Tools issue because the problem was reported to us by actual Safari users (who were logged-out after clicking on Google result links).
Comment 12 youenn fablet 2021-12-07 06:19:15 PST
*** Bug 232440 has been marked as a duplicate of this bug. ***
Comment 13 erik.witt 2021-12-07 06:31:20 PST
Hey Youenn, thanks for linking to this issue. Is there any chances this gets fixed? I was concerced about the progress on my ticket: https://bugs.webkit.org/show_bug.cgi?id=232440 but seeing this one is open since May, I am getting a bit anxious.

This is a huge issue on effected sites as I outlined in the ticket above. Happy to help if you need any more details
Comment 14 youenn fablet 2021-12-07 06:34:00 PST
Created attachment 446168 [details]
Patch
Comment 15 youenn fablet 2021-12-07 06:34:27 PST
(In reply to erik.witt from comment #13)
> Hey Youenn, thanks for linking to this issue. Is there any chances this gets
> fixed? I was concerced about the progress on my ticket:
> https://bugs.webkit.org/show_bug.cgi?id=232440 but seeing this one is open
> since May, I am getting a bit anxious.
> 
> This is a huge issue on effected sites as I outlined in the ticket above.
> Happy to help if you need any more details

I think I have a fix. You repro steps helped a lot.
Comment 16 youenn fablet 2021-12-08 00:46:40 PST
Created attachment 446330 [details]
Patch
Comment 17 youenn fablet 2021-12-08 01:28:58 PST
Created attachment 446332 [details]
Patch
Comment 18 Chris Dumez 2021-12-08 07:19:54 PST
Comment on attachment 446332 [details]
Patch

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

> Source/WebCore/loader/FrameLoader.h:320
> +    enum class IsServiceWorkerNavigationLoad { No, Yes };

: bool
Comment 19 youenn fablet 2021-12-08 07:40:33 PST
Created attachment 446364 [details]
Patch for landing
Comment 20 EWS 2021-12-08 08:28:21 PST
Committed r286656 (244969@main): <https://commits.webkit.org/244969@main>

All reviewed patches have been landed. Closing bug and clearing flags on attachment 446364 [details].
Comment 21 Brent Fulgham 2022-02-04 12:20:44 PST
This change should be present in STP 139, iOS 15.4 Beta, and macOS 12.3 Beta.