Allow "sameSite" cookie property to have value "default".#942
Allow "sameSite" cookie property to have value "default".#942
Conversation
whimboo
left a comment
whimboo
left a comment
There was a problem hiding this comment.
Just a nit. Otherwise it looks fine. Thanks!
index.bs
Outdated
| otherwise. | ||
|
|
||
| 1. Let |same site| be "<code>none</code>" if |stored cookie|'s samesite-flag is | ||
| 1. Let |same site| be null if not set, otherwise "<code>none</code>" if |stored cookie|'s samesite-flag is |
There was a problem hiding this comment.
I don't understand the if not set? Does it mean if |stored cookie|'s samesite-flag is not set?
As I mentioned on bugzilla, at the moment I feel like the samesite cookie spec does not really allow the samesite flag to not be set (https://datatracker.ietf.org/doc/html/draft-ietf-httpbis-cookie-same-site#section-4.2) ? But maybe I'm not reading this correctly.
There was a problem hiding this comment.
I don't understand the
if not set? Does it mean if|stored cookie|'s samesite-flagis not set?
I've updated the sentence, does it sound better now?
As I mentioned on bugzilla, at the moment I feel like the samesite cookie spec does not really allow the samesite flag to not be set (https://datatracker.ietf.org/doc/html/draft-ietf-httpbis-cookie-same-site#section-4.2) ? But maybe I'm not reading this correctly.
I think it's probably right that computed value of SameSite should be equal to none/lax/strict. And that we expose now an internal value (similar to web-extensions where it's called unspecified). I'm not sure how critical it's, if we maybe should add something to make it clear that we use a different kind of SameSite (and if it's ok in general). So summoning @jgraham to take a look as well.
There was a problem hiding this comment.
OK, so it looks like that draft has now been folded into https://www.ietf.org/archive/id/draft-ietf-httpbis-rfc6265bis-20.html so we should update the reference. That seems to store the values None/Lax/Strict/Default (see step 17 in https://www.ietf.org/archive/id/draft-ietf-httpbis-rfc6265bis-20.html#name-storage-model), so I think Default corresponds to Gecko's "Unset" state, and is probably what we want here.
jgraham
left a comment
jgraham
left a comment
There was a problem hiding this comment.
Setting "Request changes" for updating to the new Cookie spec and using that; I know that ends up as potential scope creep for this PR, but I don't think we can do it correctly with the old draft.
Co-authored-by: Henrik Skupin <[email protected]>
Co-authored-by: Henrik Skupin <[email protected]>
lutien
force-pushed
the
allow-samesite-being-optional
branch
from
d1e4289 to
f0e54f2
Compare
lutien
changed the title
index.bs
Outdated
| Note: The definitions of |stored cookie|'s fields are from [[COOKIES]], except | ||
| samesite-flag, which is from [[SAME-SITE-COOKIES]]. | ||
| same-site-flag, which is from [[SAME-SITE-COOKIES]]. |
There was a problem hiding this comment.
This is wrong now, I think; we're just following RFC6265bis everywhere?
There was a problem hiding this comment.
Ah, alright, I removed it now. I was just not sure how all these overrides work
|
@sadym-chromium, do you want to take another look? We changed the approach, so would like to confirm that you're still fine with these changes |
SHA: 8bc9c3c Reason: push, by lutien Co-authored-by: github-actions[bot] <41898282+github-actions[bot]@users.noreply.github.com>
5 participants
In case, setting the cookie with
document.cookieFirefox doesn't setsameSiteproperty. At the moment for BiDistorage.getCookiesAPI we fall back tosameSiteproperty toNonebut we would like to have a separate value "default" in this case to better reflect the actual browser behavior.This PR allows
sameSiteproperty to have the valuedefaultin this case.Preview | Diff