Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Make SameSite cookie attribute optional #3257

Merged
merged 2 commits into from Mar 23, 2020
Merged

Conversation

mrdziuban
Copy link
Contributor

@mrdziuban mrdziuban commented Mar 17, 2020

I mentioned this in gitter and figured I'd open a PR with the change. To get the behavior of SameSite=None in all browsers, it's necessary to completely exclude the SameSite attribute for certain clients that handle it incorrectly. This updates the field to be optional to allow for this.

@mrdziuban
Copy link
Contributor Author

@mrdziuban mrdziuban commented Mar 17, 2020

Now that I see the tests this raises a somewhat interesting question -- is the correct default still Some(SomeSite.Lax), including when parsing a cookie that doesn't include the SameSite attribute, or should it be None in any/all cases?

@hamnis
Copy link
Contributor

@hamnis hamnis commented Mar 17, 2020

We try to avoid breaking clients, so this seems reasonable.
This is a breaking incompatible change, so not sure if we should target 0.21 for that. Maybe @rossabaker or @ChristopherDavenport has some input on this.

Generating Some(SameSite.Lax) would keep the most forward-compatible default, but allowing to opt-out.

Copy link
Member

@rossabaker rossabaker left a comment

I wondered whether people would not want to implement this. It looks like a good idea, but I'd never heard of it before the original PR.

I don't see that we've left ourselves any wiggle room to fix this in a binary compatible way. We can't introduce a SameSite.Unspecified because it's a sealed trait, and we can't add another parameter to the cookie because it's a case class. The only compatible fix I can imagine is to tolerate and handle nulls, which would be very unhttp4slike.

I think this is a fine approach for master.

@mrdziuban
Copy link
Contributor Author

@mrdziuban mrdziuban commented Mar 19, 2020

Thanks @rossabaker, any thoughts on the default behavior? It feels wrong to me to use Some(SameSite.Lax) as the default when parsing, e.g. in this case -- if a client sends a cookie without the SameSite attribute, http4s should respect that right?

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Mar 20, 2020

I think that's true. Once it becomes optional, we could and should respect its absence as None.

@rossabaker
Copy link
Member

@rossabaker rossabaker commented Mar 20, 2020

A thought I'm embarrassed to bring up: if this is rendering is hugely problematic for people in 0.21, we could disable the feature with a system property.

Or this version is not too binary incompatible yet, so we could at least offer a milestone in the next series once this is merged.

@rossabaker rossabaker added the enhancement label Mar 20, 2020
@mrdziuban
Copy link
Contributor Author

@mrdziuban mrdziuban commented Mar 20, 2020

Updated the default to None. Either of those options (sys prop or milestone) would be fine for my needs!

hamnis
hamnis approved these changes Mar 23, 2020
@hamnis hamnis merged commit f774529 into http4s:master Mar 23, 2020
8 checks passed
@mrdziuban mrdziuban deleted the samesite-option branch Mar 23, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants