play1 icon indicating copy to clipboard operation
play1 copied to clipboard

GH-1328: Added SameSite support for session cookie.

Open Alexandermjos opened this issue 3 years ago • 3 comments

Pull Request Checklist

Helpful things

Fixes

Fixes #1328

Purpose

What does this PR do? This PR add support for SameSite attribute on the session cookie (https://owasp.org/www-community/SameSite)

Background Context

Why did you take this approach? At first, I created PlayCookie.java which extended Cookie.java, and DefaultPlayCookie.java extending DefaultCookie and implementing PlayCookie because Netty does not support SameSite attribute. I realized that for this to work, I had to implement PlayServerCookieEncoder to override encode methods in ServerCookieEncoder and for the encoder to work I had to copy CookieUtil from Netty in to the Play source code as it was package private. It worked, but it was a lot of code copy and duplication.

I refactored the code and ended up with this solution. Instead of implement a new version of ServerCookieEncoder, I call the original encoder (in Playhandler) from Netty and add SameSite at the end of the encoded string if the SameSite attribute is defined.

The reason for adding SameSite at the end of the encoded String and not implementing new classes is so it will be easier to adopt if we migrate to a new version of Netty with SameSite support in the future.

The SameSite attribute can be defined in "application.conf" with "application.session.sameSite" Can ble not defined, blank, "None", "lax", "strict". Not sure if casing is browser independent. "Lax" and "lax" resulted in "Lax" in Chrome, but "none" was only registered with capital N ("None").

References

Are there any relevant issues / PRs / mailing lists discussions? Not that IO

Alexandermjos avatar Jan 05 '22 01:01 Alexandermjos

Updated PR with requested changes @asolntsev

Alexandermjos avatar Jan 30 '22 23:01 Alexandermjos

Updated branch with master. Fixed merge conflict and updated failing testcases.

Alexandermjos avatar Nov 03 '22 14:11 Alexandermjos

I have updated PR with changes proposed by @asolntsev. Changed "SAMESITE" enum to "SameSite"

Alexandermjos avatar Nov 03 '22 15:11 Alexandermjos