play1
play1 copied to clipboard
playframework
Support SameSite cookie (Strict, Lax, None)
Are you looking for help?
From Google Chrome 80, they are planning to released from february, 2020, they will force SameSite=None for all Cookies if we want to use for third-party context. If no SameSite is set, then Chrome will understand as SameSite=Lax. It seems to be reversed the result as before (SameSite=None by default). Please see: https://www.chromium.org/updates/same-site Therefore, we need to update our cookies with declarative setting SameSite from Playframework Controller.
Play Version (1.5.x / etc)
Playframework 1.5.x
Expected Behavior
- Support SameSite enum inside the Http.Cookie
- SameSite enum supports three values: Strict, Lax and None
@jvosloo I think what @kiennguyen is getting at is just extending the cookie interface to allow us to set the SameSite attribute just like we can currently set other cookie attributes like expiration or httpOnly.
Ah ok, I see in Play2 the default seems to be "lax": https://www.playframework.com/documentation/2.8.x/SettingsSession
Would that be a sensible default for Play1 as well then?
That is an interesting choice. I would think that not setting it all would be best default since that is the current behavior and it mimics things like expiration and httpOnly. Another thing to consider is not all browsers support the SameSite attribute so setting it lax by default may cause issues with some browsers.
I hear you. Check out this discussion: https://github.com/playframework/playframework/issues/7316
Thanks @miller11 . Yes, we just want to have possibility to set SameSite property like httpOnly or maxAge in Http.Cookie class. No need to have default value.
@jvosloo Is that clear? Do you know the plan to have this feature in the play 1.5.x?
@kiennguyen makes sense to me yes. I'm not not one of the committers, so can't answer on the plan. Perhaps @miller11 has an idea
Also not a committer, however I don't think this is a simple path forward. Play uses the Cookie interface from Netty to set cookies as seen in the PlayHandler.java. As you can see even in the newest 4.1 release of Netty they don't yet support the SameSite attribute (Play 1.5.3 is currently on Netty 3.10.6.Final .
There is an issue in the Netty repo and even a PR for SameSite support, but seems like there is some discussion when this would get merged.
One annoying issue to note about the SameSite attribute is that it doesn't seem to be respected uniformly across browsers yet.
Thanks everyone for your involvement and suggestions so far ! It seems that Netty will only include this in version 5.x and up for compatibility reasons. Regarding migration from Netty 3, I did some progress could not complete it : https://github.com/playframework/play1/issues/1284 <= some help will be needed here because Play core's code was starting to be too tricky for me to migrate, unfortunately.
Hi all. Is anyone working on samesite for play 1.x?
I have a working version / custom build based on Play 1.5.2 which has support for samesite
@miller11 ~Play classes is extending netty classes, so netty doesnt need to support samesite?~ edit: Sorry, I mixed my own files 🤣
@Alexandermjos very interested to see what you have, can you create a pull request?
@tazmaniax I have created a pull request. I ended up starting from scratch and not using the implementation I had based on Play 1.5.2, more info regarding that in the PR. Looking forward for feedback 😃 I have not added any as reviewers. Should I add someone, or do people of interests add themselves? This is my first PR to an open source project, so not sure if I did everything correct. 🤞
@Alexandermjos looks good. I can see there was some discussion above regarding what the default value should be and the consensus seemed to be that there shouldn't be any (aka "normal"). My preference would be "lax" like Play2 so without thinking there is some automatic protection - this is what "lax" was designed to be, a reasonable compromise that in most cases won't break anything. I realise this means people will need to consider carefully before upgrading to a version with this fix but I think in respect to security that is only a good thing and certainly clear warnings should be provided. Possibly this is a bit academic now because looking at Mozilla Developer Network SameSite, the spec has been updated such that not specifying SameSite is regarded as "lax" in any case.
On a code formatting consistency side, a space between the if and the ( would be nice :)
@tazmaniax if I am not mistaken, modern browsers behave like "lax" by default. If it's true, it's good enough to leave "SameSite" attribute empty by default.
@asolntsev right, I think I came to the same conclusion at the end of my last comment 😄
@tazmaniax Thank you for the feedback :) I fill fix the space in the next commit. Should I change lax to default or not? Is there room for further discussions? Who are the decision makers, and what is the next steps to proceed?
@Alexandermjos from @asolntsev's point and what I found, I don't think it's necessary to explicitly set the default to "lax" because browsers now just assume "lax" when SameSite is not defined. It would only be useful if there was a browser that didn't support the latest version of the spec but I think this is a very rare case so I think it's good to leave it to what you have now. Thanks!
I believe the final decision maker is @xael-fry but possibly also @asolntsev
I have committed some changes which @asolntsev requested 😄 Also added the space before '(' @tazmaniax 👍
Any progress on this issue, @xael-fry or @asolntsev? Could the pull request soon be merged, such that the issue can be fixed in an upcoming release?
