| Summary: | ASSERT([filteredCookies.get() count] <= 1) on imported/w3c/web-platform-tests/websockets/cookies/third-party-cookie-accepted.https.html | ||||||||||
|---|---|---|---|---|---|---|---|---|---|---|---|
| Product: | WebKit | Reporter: | Chris Dumez <cdumez> | ||||||||
| Component: | Page Loading | Assignee: | Chris Dumez <cdumez> | ||||||||
| Status: | RESOLVED FIXED | ||||||||||
| Severity: | Normal | CC: | aakash_jain, achristensen, ap, beidson, ggaren, webkit-bot-watchers-bugzilla, webkit-bug-importer, youennf | ||||||||
| Priority: | P2 | Keywords: | InRadar | ||||||||
| Version: | WebKit Nightly Build | ||||||||||
| Hardware: | Unspecified | ||||||||||
| OS: | Unspecified | ||||||||||
| See Also: | https://bugs.webkit.org/show_bug.cgi?id=214880 | ||||||||||
| Attachments: |
|
||||||||||
|
Description
Chris Dumez
2020-07-11 10:19:03 PDT
Updated expectations in <https://trac.webkit.org/changeset/264268>. Line 467 is this, which is a bit surprising.
ASSERT([filteredCookies.get() count] <= 1);
Looks like we use a different API for parsing cookies on macOS and iOS:
#if PLATFORM(MAC)
NSArray *unfilteredCookies = [NSHTTPCookie _parsedCookiesWithResponseHeaderFields:headerFields forURL:cookieURL];
#else
NSArray *unfilteredCookies = [NSHTTPCookie cookiesWithResponseHeaderFields:headerFields forURL:cookieURL];
#endif
The cookie string is: samesite-unspecified=0.2755897489430851; Path=/, samesite-lax=0.2755897489430851; Path=/; SameSite=Lax, samesite-strict=0.2755897489430851; Path=/; SameSite=Strict, samesite-none=0.2755897489430851; Path=/; SameSite=None; Secure (In reply to Chris Dumez from comment #5) > The cookie string is: > > samesite-unspecified=0.2755897489430851; Path=/, > samesite-lax=0.2755897489430851; Path=/; SameSite=Lax, > samesite-strict=0.2755897489430851; Path=/; SameSite=Strict, > samesite-none=0.2755897489430851; Path=/; SameSite=None; Secure the HTML spec says to treat the string set to document.cookie as a Set-Cookie header. Set-Cookie header is defined here: https://tools.ietf.org/html/rfc6265#section-4.1 It should only support one cookie indeed. Created attachment 404995 [details]
Patch
Created attachment 404996 [details]
Patch
Comment on attachment 404996 [details] Patch View in context: https://bugs.webkit.org/attachment.cgi?id=404996&action=review I think this is good, but this is a bad time to make a change like this. Could this wait until after the next branch? > Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:424 > + RetainPtr<NSDate> dateInAWeek = adoptNS([[NSDate alloc] initWithTimeIntervalSinceNow:cappedLifetime->seconds()]); auto > Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:459 > + setHTTPCookiesForURL(cookieStorage().get(), [NSArray arrayWithObject:cookie], cookieURL, firstParty, sameSiteInfo); @[cookie] (In reply to Alex Christensen from comment #9) > Comment on attachment 404996 [details] > Patch > > View in context: > https://bugs.webkit.org/attachment.cgi?id=404996&action=review > > I think this is good, but this is a bad time to make a change like this. > Could this wait until after the next branch? I guess it can but it is merely aligning iOS with shipping macOS. > > > Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:424 > > + RetainPtr<NSDate> dateInAWeek = adoptNS([[NSDate alloc] initWithTimeIntervalSinceNow:cappedLifetime->seconds()]); > > auto > > > Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:459 > > + setHTTPCookiesForURL(cookieStorage().get(), [NSArray arrayWithObject:cookie], cookieURL, firstParty, sameSiteInfo); > > @[cookie] (In reply to Chris Dumez from comment #10) > (In reply to Alex Christensen from comment #9) > > Comment on attachment 404996 [details] > > Patch > > > > View in context: > > https://bugs.webkit.org/attachment.cgi?id=404996&action=review > > > > I think this is good, but this is a bad time to make a change like this. > > Could this wait until after the next branch? > > I guess it can but it is merely aligning iOS with shipping macOS. It also only impact document.cookie setting. > > > > > > Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:424 > > > + RetainPtr<NSDate> dateInAWeek = adoptNS([[NSDate alloc] initWithTimeIntervalSinceNow:cappedLifetime->seconds()]); > > > > auto > > > > > Source/WebCore/platform/network/cocoa/NetworkStorageSessionCocoa.mm:459 > > > + setHTTPCookiesForURL(cookieStorage().get(), [NSArray arrayWithObject:cookie], cookieURL, firstParty, sameSiteInfo); > > > > @[cookie] Created attachment 405042 [details]
Patch
Committed r264943: <https://trac.webkit.org/changeset/264943> All reviewed patches have been landed. Closing bug and clearing flags on attachment 405042 [details]. (In reply to EWS from comment #13) > Committed r264943: <https://trac.webkit.org/changeset/264943> http/tests/cookies/document-cookie-multiple-cookies.html is failing consistently since this commit, as also warned by EWS. History: https://results.webkit.org/?suite=layout-tests&test=http%2Ftests%2Fcookies%2Fdocument-cookie-multiple-cookies.html e.g.: https://ews-build.s3-us-west-2.amazonaws.com/Windows-EWS/r405345-31731-clean-tree/results.html -PASS document.cookie is "samesite-unspecified=0" +FAIL document.cookie should be samesite-unspecified=0. Was samesite-lax=1; samesite-strict=2; samesite-unspecified=0. (In reply to Aakash Jain from comment #15) > e.g.: > https://ews-build.s3-us-west-2.amazonaws.com/Windows-EWS/r405345-31731-clean- > tree/results.html > > -PASS document.cookie is "samesite-unspecified=0" > +FAIL document.cookie should be samesite-unspecified=0. Was samesite-lax=1; > samesite-strict=2; samesite-unspecified=0. Ok. Windows still has the bug. I only fixed it for iOS. (In reply to Chris Dumez from comment #16) > (In reply to Aakash Jain from comment #15) > > e.g.: > > https://ews-build.s3-us-west-2.amazonaws.com/Windows-EWS/r405345-31731-clean- > > tree/results.html > > > > -PASS document.cookie is "samesite-unspecified=0" > > +FAIL document.cookie should be samesite-unspecified=0. Was samesite-lax=1; > > samesite-strict=2; samesite-unspecified=0. > > Ok. Windows still has the bug. I only fixed it for iOS. Fixing the same bug on Windows via Bug 214880. |