GH-1328: Added SameSite support for session cookie. #1369
Conversation
| @@ -159,7 +159,7 @@ static void save() { | |||
| if (Validation.errors().isEmpty()) { | |||
| // Only send "delete cookie" header when the cookie was present in the request | |||
| if(Http.Request.current().cookies.containsKey(Scope.COOKIE_PREFIX + "_ERRORS") || !Scope.SESSION_SEND_ONLY_IF_CHANGED) { | |||
| Http.Response.current().setCookie(Scope.COOKIE_PREFIX + "_ERRORS", "", null, "/", 0, Scope.COOKIE_SECURE, Scope.SESSION_HTTPONLY); | |||
| Http.Response.current().setCookie(Scope.COOKIE_PREFIX + "_ERRORS", "", null, "/", 0, Scope.COOKIE_SECURE, Scope.SESSION_HTTPONLY, Scope.SESSION_SAMESITE); | |||
There was a problem hiding this comment.
Isn't it strange that we use SameSite setting which was introduced for session cookie also for some other cookies?
There was a problem hiding this comment.
Isn't it strange that we use SameSite setting which was introduced for session cookie also for some other cookies?
@asolntsev Maybe, idk. What do you suggest? null / ""?
It uses the same settings as session cookie for httpOnly and Secure, so I thought that it should also use same settings as session for samesite.
framework/src/play/mvc/Scope.java
Outdated
| @@ -32,6 +32,7 @@ | |||
| .equals("true"); | |||
| public static boolean SESSION_SEND_ONLY_IF_CHANGED = Play.configuration | |||
| .getProperty("application.session.sendOnlyIfChanged", "false").toLowerCase().equals("true"); | |||
| public static final String SESSION_SAMESITE = Play.configuration.getProperty("application.session.sameSite"); | |||
There was a problem hiding this comment.
How about renaming it to "application.session.cookie.sameSite"?
There was a problem hiding this comment.
How about renaming it to "application.session.cookie.sameSite"?
Sure, I can do that.
framework/src/play/mvc/Http.java
Outdated
| } | ||
|
|
||
| public void setCookie(String name, String value, String domain, String path, Integer maxAge, boolean secure) { | ||
| setCookie(name, value, domain, path, maxAge, secure, false); | ||
| public void setCookie(String name, String value, String domain, String path, Integer maxAge, boolean secure, String sameSite) { |
There was a problem hiding this comment.
I suggest using enum instead of String.
There was a problem hiding this comment.
I suggest using enum instead of String.
I can look into this.
Input in application.conf is string. What is best practise for mapping to enum? Creating enums based on config value in uppercase? How to deal with incorrect values?
… Enum. Dont set sameSite for ValidationPlugin
|
Updated PR with requested changes @asolntsev |
framework/src/play/mvc/Http.java
Outdated
| @@ -1018,4 +1022,20 @@ public WebSocketFrame(byte[] data) { | |||
|
|
|||
| public static class WebSocketClose extends WebSocketEvent { | |||
| } | |||
|
|
|||
| public enum SAMESITE { | |||
There was a problem hiding this comment.
It's probably a matter of taste, but I would prefer SameSite name. Upper case doesn't make it readable.
There was a problem hiding this comment.
I have updated the casing to SameSite
# Conflicts: # framework/src/play/data/validation/ValidationPlugin.java
|
Updated branch with master. Fixed merge conflict and updated failing testcases. |
|
I have updated PR with changes proposed by @asolntsev. Changed "SAMESITE" enum to "SameSite" |
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