Steps to reproduce: Users visit https://some-integration.com GET https://some-integration.com HTTP/1.1 Because they're not logged in, user-agent redirects to https://accounts.spotify.com/authorize (standard OAuth 2.0 flow). HTTP/1.1 302 Found Location: https://accounts.spotify.com/authorize?client_id=<client_id>&redirect_uri=https%3A%2F%2Fsome-integration.com%2Fsuccess https://accounts.spotify.com/authorize returns the html form (for user to approve scopes) and sets csrf_token cookie. GET https://accounts.spotify.com/login?continue=https://*.spotify.net HTTP/1.1 HTTP/1.1 200 OK Content-Type: text/html;charset=UTF-8 Set-Cookie: csrf_token=<omitted>; Domain=.accounts.spotify.com; Path=/; Secure; SameSite=Strict ... Actual results: JS call document.cookie doesn't contain csrf_token cookie. Expected results: JS call document.cookie contains csrf_token=<omitted>
<rdar://problem/59701889>
Thanks for filing! Some screening questions: 1. Is this for Safari? WebKit supports multiple HTTP backends where cookies are implemented. 2. If for Safari, which OS and which version? 3. Has this worked before, i.e. is it a regression? 4. Do you see different behavior in other browsers? 5. I see both spotify.com and spotify.net in the HTTP metadata you provide. Is the .net domain ever loaded or just there for things that might happen after the CSRF token is read through document.cookie?
1. Yes. 2. Safari 13.0.5, MacOS 10.14.6 3. I don't know, didn't use Strict cookies before. 4. Yes, Google Chrome behaves as expected. Latest Firefox though also has this bug. 5. My bad. It's a typo. Line `GET https://accounts.spotify.com/login?continue=https://*.spotify.net HTTP/1.1` should be replaced with `GET https://accounts.spotify.com/authorize?client_id=<client_id>&redirect_uri=https%3A%2F%2Fsome-integration.com%2Fsuccess HTTP/1.1` So, spotify.net domain isn't importatnt in this case. Just a redirect from https://some-integration.com to https://accounts.spotify.com matters.
I've added a simple node app with poc https://github.com/anstick/strict-cookie-issue-poc
(In reply to Aleksei from comment #3) > 1. Yes. > 2. Safari 13.0.5, MacOS 10.14.6 > 3. I don't know, didn't use Strict cookies before. > 4. Yes, Google Chrome behaves as expected. Latest Firefox though also has > this bug. When you say “as expected,” do you mean by spec? I was a long while since I read the particulars of SameSite cookies. Two engines doing it one way and one doing it another may otherwise imply the opposite of what you’re saying. > 5. My bad. It's a typo. Line `GET > https://accounts.spotify.com/login?continue=https://*.spotify.net HTTP/1.1` > should be replaced with `GET > https://accounts.spotify.com/ > authorize?client_id=<client_id>&redirect_uri=https%3A%2F%2Fsome-integration. > com%2Fsuccess HTTP/1.1` > > So, spotify.net domain isn't importatnt in this case. Just a redirect from > https://some-integration.com to https://accounts.spotify.com matters. Got it. Thanks!
(In reply to John Wilander from comment #5) > > 4. Yes, Google Chrome behaves as expected. Latest Firefox though also has > > this bug. > > When you say “as expected,” do you mean by spec? I was a long while since I > read the particulars of SameSite cookies. Two engines doing it one way and > one doing it another may otherwise imply the opposite of what you’re saying. AFAIK the spec (https://tools.ietf.org/html/draft-ietf-httpbis-rfc6265bis-05) doesn't explicitly set the behavior of Document interface in browsers. The RFC a) defines the difference between "same-site" and "cross-site" requests and the algorithm to distinguish between them b) defines whether a Cookie header should be sent along with requests or not based on "SameSite" attribute. I dunno about any spec regarding JS access to cookies based on `SameSite` attribute (plz point them out if you know). However, if we look at the sequence of requests: 1. Request: GET https://some-integration.com HTTP/1.1 2. Response: HTTP/1.1 302 Found Location: https://accounts.spotify.com/authorize?client_id=<client_id>&redirect_uri=https%3A%2F%2Fsome-integration.com%2Fsuccess Request: GET https://accounts.spotify.com/authorize?client_id=<client_id>&redirect_uri=https%3A%2F%2Fsome-integration.com%2Fsuccess HTTP/1.1 3. Response: HTTP/1.1 200 OK https://accounts.spotify.com/authorize?client_id=<client_id>&redirect_uri=https%3A%2F%2Fsome-integration.com%2Fsuccess HTTP/1.1 ... (for example) Request: POST https://accounts.spotify.com/authorize and apply the algorithm from the RFC, the 1->2 will be a "cross-domain" request (I don't expect any Strict cookies to be sent to the backend with this request), however 2->3 is a "same-site" so there should not be any limitations on cookies (I don't consider using `iframe` in my statement). The current Safari behaviour makes it impossible to use `SameSite=Strict` cookies for us, because you never know how this page gonna be opened (via redirect or not). And I don't think it's only for us: any internet page using SPA and Double Submit Cookie CSRF pattern (https://owasp.org/www-chapter-london/assets/slides/David_Johansson-Double_Defeat_of_Double-Submit_Cookie.pdf) for form submission can't have `csrf` cookie being strict (or can't use strict cookies at all).
Created attachment 398178 [details] Cookies sent does not match document.cookies
Comment on attachment 398178 [details] Cookies sent does not match document.cookies I'm having the same issue The redirect sets the cookies via Set-Cookie Fetch requests are sending correct cookie headers But document.cookie on both sub-domains is an empty string Storage tab is also empty
Created attachment 398203 [details] Redirect chain in Chrome
Created attachment 398204 [details] Redirect chain in Safari
Created attachment 398205 [details] Fetch request with cookies So the redirect has finished and the cookies should be set and accessable through document.cookie, but it doesn't return the cookies I'm looking for both on abc & xzy.example.com But when I do a fetch request, the cookies which "haven't" been set are passed along with the request.
Comment on attachment 398205 [details] Fetch request with cookies -
Any updates on this issue?
We're seeing the same issue in Safari 14.01: same-site cookies set after a redirect from a third-party site aren't accessible via `document.cookie`. Similarly to OP, we rely on double-submit CSRF cookies. We noticed the issue when Sendgrid's click-tracking redirects to our site and our cookies are no longer accessible to JS. A few other things we've noticed: 1. Issue persists after refreshing the page. For example: Sendgrid redirects to first-party (ie xyz.com); cookie inaccessible to JS; refresh xyz.com; `Set-Cookie` header is in the response; cookie is included in subsequent xhr requests, yet is still inaccessible to JS. 2. Pre-existing cookies that were accessible to JS, after a redirect, are no longer accessible to JS. For example: cookie accessible to JS w/in xyz.com, but when user lands on xyz.com after a redirect, the same cookie is no longer accessible to JS. 3. In addition to not being accessible via `document.cookie`, the aforementioned cookies are also not shown in the "Storage" tab in the web inspector. In the meantime we've mitigated this issue by reducing same-site level to "Lax," which works given we don't use CSRF protection for GETs, but it'd be ideal if Strict cookies could be used in the same way. This was all tested on Mozilla/5.0 (Macintosh; Intel Mac OS X 10_15_6) AppleWebKit/605.1.15 (KHTML, like Gecko) Version/14.0.1 Safari/605.1.15
I think this is the same as https://bugs.chromium.org/p/chromium/issues/detail?id=925311#c26 The spec has changed and Firefox and Chrome both allow |document.cookie| access to cookies set with SameSite=Strict. Safari 14.2 Release 123 does not. Repro: https://debugtheweb.com/test/cookie/samesite/ Simply set the cookies on both of the first two domains, then flip between them using the links.
Thanks all! I'm looking at this.
Created attachment 428589 [details] Patch
Created attachment 428590 [details] Patch
Fixed typo in change log.
Comment on attachment 428589 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=428589&action=review > Source/WebCore/platform/network/SameSiteInfo.h:32 > +enum class CookieAccessForHTTP : bool { Yes, No }; I would reverse this and call it CookieAccessForJS. As it stands now, the use of CookieAccessForHTTP::Yes is more for the cache than HTTP, which is entirely done by CFNetwork. > Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:259 > // FIXME: Seems like this newer code path can be used for watchOS and tvOS too. We should probably fix this soon. I would be quite surprised if CFNetwork were different in this way.
(In reply to Alex Christensen from comment #20) > Comment on attachment 428589 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=428589&action=review > > > Source/WebCore/platform/network/SameSiteInfo.h:32 > > +enum class CookieAccessForHTTP : bool { Yes, No }; > > I would reverse this and call it CookieAccessForJS. As it stands now, the > use of CookieAccessForHTTP::Yes is more for the cache than HTTP, which is > entirely done by CFNetwork. I actually started out with it called CookieAccessForDOM which follows our code naming well. Then I switched to CookieAccessForHTTP to align with the spec language. See change log comment. I can go back to CookieAccessForDOM as long as we think that's good given how the spec frames it. > > Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:259 > > // FIXME: Seems like this newer code path can be used for watchOS and tvOS too. > > We should probably fix this soon. I would be quite surprised if CFNetwork > were different in this way. Me too. I was surprised to see this comment.
Comment on attachment 428590 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=428590&action=review r=me with changes. > Source/WebCore/platform/network/SameSiteInfo.h:32 > +enum class CookieAccessForHTTP : bool { Yes, No }; Please put No first so its value is 0. Since this is logically a boolean, I think it needs a prefix, like IsCookieAccessForHTTP. Honestly, if it were me, I would have gone the other way around and called this IsCookieAccessForDOM, since this is the case we're special-casing. Also, I think IsCookieAccessForHTTP is a bit confusing since WebKit doesn't really deal with HTTP cookies itself. We do use cookies for things like WebSocket (but then it's not really HTTP I think). > Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:266 > + sameSiteInfoForGetCookies = SameSiteInfo { While lie about being same site instead of calling the one in the else case (which does not seem to care about same-site)? [storage _getCookiesForURL:url mainDocumentURL:mainDocumentURL partition:partition completionHandler:completionHandler];
(In reply to Chris Dumez from comment #22) > Comment on attachment 428590 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=428590&action=review > > r=me with changes. > > > Source/WebCore/platform/network/SameSiteInfo.h:32 > > +enum class CookieAccessForHTTP : bool { Yes, No }; > > Please put No first so its value is 0. Will fix. > Since this is logically a boolean, I think it needs a prefix, like > IsCookieAccessForHTTP. I wanted to do that and looked at other boolean enums and couldn't find a clear pattern. Will fix though. Much better with Is prefix. > Honestly, if it were me, I would have gone the other way around and called > this IsCookieAccessForDOM, since this is the case we're special-casing. > Also, I think IsCookieAccessForHTTP is a bit confusing since WebKit doesn't > really deal with HTTP cookies itself. We do use cookies for things like > WebSocket (but then it's not really HTTP I think). We all agree it's better in WebKit code. I just went with HTTP because of the spec language. > > Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:266 > > + sameSiteInfoForGetCookies = SameSiteInfo { > > While lie about being same site instead of calling the one in the else case > (which does not seem to care about same-site)? > [storage _getCookiesForURL:url mainDocumentURL:mainDocumentURL > partition:partition completionHandler:completionHandler]; Not sure what you're saying here. :)
Will fix the unused parameter warning for tvOS and watchOS.
Comment on attachment 428590 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=428590&action=review > Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:265 > + if (accessForHTTP == CookieAccessForHTTP::No && sameSiteInfo && sameSiteInfo->isTopSite) { Seems wrong to overwrite the sameSiteInfo we're being given at super low-level here. Why can't we pass the right sameSiteInfo from the start? (Presumably in WebCookieJar::cookies() & CookieJar::cookies(), where the SameSiteInfo is constructed to retrieve DOM cookies).
Created attachment 428661 [details] Patch
Comment on attachment 428661 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=428661&action=review > Source/WebCore/loader/CookieJar.cpp:62 > +SameSiteInfo CookieJar::sameSiteInfo(const Document& document, IsCookieAccessForDOM isAccessForDOM) I would rename IsCookieAccessForDOM to IsForDOMCookieAccess. I think it makes for sense for a function called sameSiteInfo() which is really getting cookies. > Source/WebCore/loader/CookieJar.cpp:65 > + auto sameSiteInfo = SameSiteInfo::create(loader->request()); Should we consider passing the IsCookieAccessForDOM flag to SameSiteInfo::create() too? Then we wouldn't need to post-process the sameSiteInfo, we'd get the right one right away. Your call. > Source/WebCore/loader/CookieJar.cpp:98 > + result = session->cookiesForDOM(document.firstPartyForCookies(), sameSiteInfo(document, IsCookieAccessForDOM::Yes), url, frameID, pageID, includeSecureCookies, ShouldAskITP::Yes, shouldRelaxThirdPartyCookieBlocking(document)); Any reason we're not using IsCookieAccessForDOM::Yes in setCookiesFromDOM() ? Which the asymmetry?
Created attachment 428675 [details] Patch
Comment on attachment 428675 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=428675&action=review r=me with nit > Source/WebCore/loader/CookieJar.h:71 > + static SameSiteInfo sameSiteInfo(const Document&, IsForDOMCookieAccess isAccessForDOM = IsForDOMCookieAccess::No); Can simply be: `IsForDOMCookieAccess = IsForDOMCookieAccess::No`. No need for the parameter name.
(In reply to Chris Dumez from comment #29) > Comment on attachment 428675 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=428675&action=review > > r=me with nit > > > Source/WebCore/loader/CookieJar.h:71 > > + static SameSiteInfo sameSiteInfo(const Document&, IsForDOMCookieAccess isAccessForDOM = IsForDOMCookieAccess::No); > > Can simply be: `IsForDOMCookieAccess = IsForDOMCookieAccess::No`. No need > for the parameter name. Thanks, Chris! Will fix and land if EWS is happy.
mac-debug-wk1 test failure is unrelated.
Created attachment 428714 [details] Patch for landing
Committed r277534 (237762@main): <https://commits.webkit.org/237762@main> All reviewed patches have been landed. Closing bug and clearing flags on attachment 428714 [details].