Skip to content

GH-1328: Added SameSite support for session cookie. #1369

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.

Sign up for GitHub

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

Open
Alexandermjos wants to merge 5 commits into
base: master
Choose a base branch
from
Open

GH-1328: Added SameSite support for session cookie. #1369

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
0a8cb43
GH-1328: Added SameSite support for session cookie.
Alexandermjos Jan 5, 2022
6d0d0cc
Resolves #1328: Refactored sameSite value from String to Enum. Dont s…
Alexandermjos Jan 30, 2022
16d7f92
Merge branch 'master' into 1328-sync-1.8.0-v2
Alexandermjos Nov 3, 2022
183053a
Update unittest after syncing feature branch from master
Alexandermjos Nov 3, 2022
7f25a12
GH-1328: Change casing after feedback from pull request
Alexandermjos Nov 3, 2022
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
4 changes: 2 additions & 2 deletions framework/src/play/data/validation/ValidationPlugin.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Copy link
Copy Markdown
Contributor

@asolntsev asolntsev Jan 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Isn't it strange that we use SameSite setting which was introduced for session cookie also for some other cookies?

Copy link
Copy Markdown
Author

@Alexandermjos Alexandermjos Jan 5, 2022 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Copy Markdown
Author

@Alexandermjos Alexandermjos Jan 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated PR

}
return;
}
Expand All @@ -179,7 +179,7 @@ static void save() {
}
}
String errorsData = URLEncoder.encode(errors.toString(), "utf-8");
Http.Response.current().setCookie(Scope.COOKIE_PREFIX + "_ERRORS", errorsData, null, "/", null, Scope.COOKIE_SECURE, Scope.SESSION_HTTPONLY);
Http.Response.current().setCookie(Scope.COOKIE_PREFIX + "_ERRORS", errorsData, null, "/", null, Scope.COOKIE_SECURE, Scope.SESSION_HTTPONLY, Scope.SESSION_SAMESITE);
} catch (Exception e) {
throw new UnexpectedException("Errors serializationProblem", e);
}
Expand Down
4 changes: 2 additions & 2 deletions framework/src/play/i18n/Lang.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -82,7 +82,7 @@ public static void change(String locale) {
Response response = Response.current();
if (response != null) {
// We have a current response in scope - set the language-cookie to store the selected language for the next requests
response.setCookie(Play.configuration.getProperty("application.lang.cookie", "PLAY_LANG"), locale, null, "/", null, Scope.COOKIE_SECURE);
response.setCookie(Play.configuration.getProperty("application.lang.cookie", "PLAY_LANG"), locale, null, "/", null, Scope.COOKIE_SECURE, Scope.SESSION_SAMESITE);
}
}

Expand Down Expand Up @@ -155,7 +155,7 @@ private static void resolveFrom(Request request) {
return;
}
// could not use locale from cookie - clear the locale-cookie
Response.current().setCookie(cn, "", null, "/", null, Scope.COOKIE_SECURE);
Response.current().setCookie(cn, "", null, "/", null, Scope.COOKIE_SECURE, Scope.SESSION_SAMESITE);

}

Expand Down
6 changes: 3 additions & 3 deletions framework/src/play/mvc/CookieSessionStore.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -75,7 +75,7 @@ public void save(Session session) {
if (session.isEmpty()) {
// The session is empty: delete the cookie
if (Http.Request.current().cookies.containsKey(COOKIE_PREFIX + "_SESSION") || !SESSION_SEND_ONLY_IF_CHANGED) {
Http.Response.current().setCookie(COOKIE_PREFIX + "_SESSION", "", null, "/", 0, COOKIE_SECURE, SESSION_HTTPONLY);
Http.Response.current().setCookie(COOKIE_PREFIX + "_SESSION", "", null, "/", 0, COOKIE_SECURE, SESSION_HTTPONLY, SESSION_SAMESITE);
}
return;
}
Expand All @@ -84,10 +84,10 @@ public void save(Session session) {
String sign = Crypto.sign(sessionData, Play.secretKey.getBytes());
if (COOKIE_EXPIRE == null) {
Http.Response.current().setCookie(COOKIE_PREFIX + "_SESSION", sign + "-" + sessionData, null, "/", null, COOKIE_SECURE,
SESSION_HTTPONLY);
SESSION_HTTPONLY, SESSION_SAMESITE);
} else {
Http.Response.current().setCookie(COOKIE_PREFIX + "_SESSION", sign + "-" + sessionData, null, "/",
Time.parseDuration(COOKIE_EXPIRE), COOKIE_SECURE, SESSION_HTTPONLY);
Time.parseDuration(COOKIE_EXPIRE), COOKIE_SECURE, SESSION_HTTPONLY, SESSION_SAMESITE);
}
} catch (Exception e) {
throw new UnexpectedException("Session serializationProblem", e);
Expand Down
21 changes: 13 additions & 8 deletions framework/src/play/mvc/Http.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -163,6 +163,10 @@ public static class Cookie implements Serializable {
* See http://www.owasp.org/index.php/HttpOnly
*/
public boolean httpOnly = false;
/**
* See https://owasp.org/www-community/SameSite
*/
public String sameSite;
}

/**
Expand Down Expand Up @@ -707,8 +711,8 @@ public void setContentTypeIfNotSet(String contentType) {
* @param value
* Cookie value
*/
public void setCookie(String name, String value) {
setCookie(name, value, null, "/", null, false);
public void setCookie(String name, String value, String sameSite) {
setCookie(name, value, null, "/", null, false, sameSite);
}

/**
Expand All @@ -730,7 +734,7 @@ public void removeCookie(String name) {
* cookie path
*/
public void removeCookie(String name, String path) {
setCookie(name, "", null, path, 0, false);
setCookie(name, "", null, path, 0, false, null);
}

/**
Expand All @@ -743,15 +747,15 @@ public void removeCookie(String name, String path) {
* @param duration
* the cookie duration (Ex: 3d)
*/
public void setCookie(String name, String value, String duration) {
setCookie(name, value, null, "/", Time.parseDuration(duration), false);
public void setCookie(String name, String value, String duration, String sameSite) {
setCookie(name, value, null, "/", Time.parseDuration(duration), false, sameSite);
}

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) {
Copy link
Copy Markdown
Contributor

@asolntsev asolntsev Jan 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I suggest using enum instead of String.

Copy link
Copy Markdown
Author

@Alexandermjos Alexandermjos Jan 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Copy Markdown
Author

@Alexandermjos Alexandermjos Jan 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated PR

setCookie(name, value, domain, path, maxAge, secure, false, sameSite);
}

public void setCookie(String name, String value, String domain, String path, Integer maxAge, boolean secure, boolean httpOnly) {
public void setCookie(String name, String value, String domain, String path, Integer maxAge, boolean secure, boolean httpOnly, String sameSite) {
path = Play.ctxPath + path;
if (cookies.containsKey(name) && cookies.get(name).path.equals(path)
&& ((cookies.get(name).domain == null && domain == null) || (cookies.get(name).domain.equals(domain)))) {
Expand All @@ -765,6 +769,7 @@ public void setCookie(String name, String value, String domain, String path, Int
cookie.path = path;
cookie.secure = secure;
cookie.httpOnly = httpOnly;
cookie.sameSite = sameSite;
if (domain != null) {
cookie.domain = domain;
} else {
Expand Down
5 changes: 3 additions & 2 deletions framework/src/play/mvc/Scope.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -32,6 +32,7 @@ public class Scope {
.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");
Copy link
Copy Markdown
Contributor

@asolntsev asolntsev Jan 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about renaming it to "application.session.cookie.sameSite"?

Copy link
Copy Markdown
Author

@Alexandermjos Alexandermjos Jan 5, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How about renaming it to "application.session.cookie.sameSite"?

Sure, I can do that.

Copy link
Copy Markdown
Author

@Alexandermjos Alexandermjos Jan 30, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated PR


public static SessionStore sessionStore = createSessionStore();

Expand Down Expand Up @@ -78,13 +79,13 @@ void save() {
}
if (out.isEmpty()) {
if (Http.Request.current().cookies.containsKey(COOKIE_PREFIX + "_FLASH") || !SESSION_SEND_ONLY_IF_CHANGED) {
Http.Response.current().setCookie(COOKIE_PREFIX + "_FLASH", "", null, "/", 0, COOKIE_SECURE, SESSION_HTTPONLY);
Http.Response.current().setCookie(COOKIE_PREFIX + "_FLASH", "", null, "/", 0, COOKIE_SECURE, SESSION_HTTPONLY, SESSION_SAMESITE);
}
return;
}
try {
String flashData = CookieDataCodec.encode(out);
Http.Response.current().setCookie(COOKIE_PREFIX + "_FLASH", flashData, null, "/", null, COOKIE_SECURE, SESSION_HTTPONLY);
Http.Response.current().setCookie(COOKIE_PREFIX + "_FLASH", flashData, null, "/", null, COOKIE_SECURE, SESSION_HTTPONLY, SESSION_SAMESITE);
} catch (Exception e) {
throw new UnexpectedException("Flash serializationProblem", e);
}
Expand Down
14 changes: 10 additions & 4 deletions framework/src/play/server/PlayHandler.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -45,7 +45,6 @@
import java.security.MessageDigest;
import java.security.NoSuchAlgorithmException;

import java.text.ParseException;
import java.util.*;
import java.util.concurrent.ConcurrentHashMap;
import java.util.concurrent.ConcurrentLinkedQueue;
Expand Down Expand Up @@ -375,7 +374,11 @@ protected static void addToResponse(Response response, HttpResponse nettyRespons
c.setMaxAge(cookie.maxAge);
}
c.setHttpOnly(cookie.httpOnly);
nettyResponse.headers().add(SET_COOKIE, ServerCookieEncoder.STRICT.encode(c));
String encodedCookie = ServerCookieEncoder.STRICT.encode(c);
if(cookie.sameSite != null) {
encodedCookie += "; SameSite=" + cookie.sameSite;
}
nettyResponse.headers().add(SET_COOKIE, encodedCookie);
}

if (!response.headers.containsKey(CACHE_CONTROL) && !response.headers.containsKey(EXPIRES)
Expand Down Expand Up @@ -792,8 +795,11 @@ public static void serve500(Exception e, ChannelHandlerContext ctx, HttpRequest
c.setMaxAge(cookie.maxAge);
}
c.setHttpOnly(cookie.httpOnly);

nettyResponse.headers().add(SET_COOKIE, ServerCookieEncoder.STRICT.encode(c));
String encodedCookie = ServerCookieEncoder.STRICT.encode(c);
if(cookie.sameSite != null) {
encodedCookie += "; SameSite=" + cookie.sameSite;
}
nettyResponse.headers().add(SET_COOKIE, encodedCookie);
}

} catch (Exception exx) {
Expand Down
22 changes: 20 additions & 2 deletions framework/test-src/play/mvc/HttpResponseTest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -9,12 +9,30 @@ public class HttpResponseTest {
public void verifyDefaultCookieDomain() {
Http.Cookie.defaultDomain = null;
Http.Response response = new Http.Response();
response.setCookie("testCookie", "testValue");
response.setCookie("testCookie", "testValue", null);
assertThat(response.cookies.get("testCookie").domain).isNull();

Http.Cookie.defaultDomain = ".abc.com";
response = new Http.Response();
response.setCookie("testCookie", "testValue");
response.setCookie("testCookie", "testValue", null);
assertThat(response.cookies.get("testCookie").domain).isEqualTo(".abc.com");
}

@Test
public void verifySameSiteCookie() {
Http.Cookie.defaultDomain = null;
Http.Response response = new Http.Response();
response.setCookie("testCookie", "testValue", null);
assertThat(response.cookies.get("testCookie").sameSite).isNull();

Http.Cookie.defaultDomain = ".abc.com";
response = new Http.Response();
response.setCookie("testCookie", "testValue", "lax");
assertThat(response.cookies.get("testCookie").sameSite).isEqualTo("lax");

Http.Cookie.defaultDomain = ".abc.com";
response = new Http.Response();
response.setCookie("testCookie", "testValue", "strict");
assertThat(response.cookies.get("testCookie").sameSite).isEqualTo("strict");
}
}
1 change: 1 addition & 0 deletions resources/application-skel/conf/application.conf
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,6 +48,7 @@ date.format=yyyy-MM-dd
# application.session.cookie=PLAY
# application.session.maxAge=1h
# application.session.secure=false
# application.session.sameSite=lax

# Session/Cookie sharing between subdomain
# ~~~~~~~~~~~~~~~~~~~~~~
Expand Down