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

SameSite cookie attribute missing when using WebFlux #23693

Closed
essh opened this issue Sep 24, 2019 · 6 comments
Closed

SameSite cookie attribute missing when using WebFlux #23693

essh opened this issue Sep 24, 2019 · 6 comments
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: bug A general bug
Milestone

Comments

@essh
Copy link

essh commented Sep 24, 2019

As of Spring Framework 5.1 the "What’s New” wiki documentation states the following for Spring WebFlux (https://github.com/spring-projects/spring-framework/wiki/What%27s-New-in-Spring-Framework-5.x#spring-webflux-1):

"Session cookies now have SameSite=Lax to protect against CSRF attacks”

In practice this is not true when using any of the framework supplied HttpHandler adapters and no SameSite attribute is added to session cookies. This is dangerous as a user may read this note and assume that the SameSite attribute will be set to a value that will prevent CSRF on all modern browsers (https://caniuse.com/#feat=same-site-cookie-attribute) when using Spring WebFlux >= 5.1.

You can verify that the SameSite attribute is not being added to session cookies on WebFlux by default by creating a new Spring Boot WebFlux project on the Spring Initializr, creating a controller that sets an attribute on the session, and then making a HTTP request to this controller method and inspecting the returned session cookie.

The issue seems to be that internally the framework uses a ResponseCookie that does set the SameSite attribute to “Lax" by default. However, all of the ServerHttpResponse adapters for the various servers do not copy the SameSite attribute into the resulting cookie when calling their applyCookies() method:

ReactorServerHttpResponse -

protected void applyCookies() {
for (String name : getCookies().keySet()) {
for (ResponseCookie httpCookie : getCookies().get(name)) {
Cookie cookie = new DefaultCookie(name, httpCookie.getValue());
if (!httpCookie.getMaxAge().isNegative()) {
cookie.setMaxAge(httpCookie.getMaxAge().getSeconds());
}
if (httpCookie.getDomain() != null) {
cookie.setDomain(httpCookie.getDomain());
}
if (httpCookie.getPath() != null) {
cookie.setPath(httpCookie.getPath());
}
cookie.setSecure(httpCookie.isSecure());
cookie.setHttpOnly(httpCookie.isHttpOnly());
this.response.addCookie(cookie);
}
}
}

UndertowServerHttpResponse -

protected void applyCookies() {
for (String name : getCookies().keySet()) {
for (ResponseCookie httpCookie : getCookies().get(name)) {
Cookie cookie = new CookieImpl(name, httpCookie.getValue());
if (!httpCookie.getMaxAge().isNegative()) {
cookie.setMaxAge((int) httpCookie.getMaxAge().getSeconds());
}
if (httpCookie.getDomain() != null) {
cookie.setDomain(httpCookie.getDomain());
}
if (httpCookie.getPath() != null) {
cookie.setPath(httpCookie.getPath());
}
cookie.setSecure(httpCookie.isSecure());
cookie.setHttpOnly(httpCookie.isHttpOnly());
this.exchange.getResponseCookies().putIfAbsent(name, cookie);
}
}
}

ServletServerHttpResponse -

protected void applyCookies() {
for (String name : getCookies().keySet()) {
for (ResponseCookie httpCookie : getCookies().get(name)) {
Cookie cookie = new Cookie(name, httpCookie.getValue());
if (!httpCookie.getMaxAge().isNegative()) {
cookie.setMaxAge((int) httpCookie.getMaxAge().getSeconds());
}
if (httpCookie.getDomain() != null) {
cookie.setDomain(httpCookie.getDomain());
}
if (httpCookie.getPath() != null) {
cookie.setPath(httpCookie.getPath());
}
cookie.setSecure(httpCookie.isSecure());
cookie.setHttpOnly(httpCookie.isHttpOnly());
this.response.addCookie(cookie);
}
}
}

This may be somewhat problematic to fix using the existing logic that delegates to the server Cookie implementation as only Undertow currently has a method to set the SameSite attribute.

Also related to this issue is that in the near future Chrome will default cookies that do not have a SameSite attribute to “Lax” (https://www.chromestatus.com/feature/5088147346030592) which may break some applications if they have no ability to change the value of the SameSite attribute.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged or decided on label Sep 24, 2019
@rstoyanchev rstoyanchev self-assigned this Sep 25, 2019
@rstoyanchev rstoyanchev added in: web Issues in web modules (web, webmvc, webflux, websocket) type: bug A general bug and removed status: waiting-for-triage An issue we've not yet triaged or decided on labels Sep 25, 2019
@rstoyanchev rstoyanchev added this to the 5.1.10 milestone Sep 25, 2019
@chadlwilson
Copy link

chadlwilson commented Oct 5, 2019

It's worth noting for those who come across it; that among other things this change added additional validation so if (for example) the domain of your ResponseCookie previously contained a port number then it will fail validation and throw an IllegalArgumentException after this change; representing a change in behaviour of the framework between 5.1.9 and 5.1.10 (albeit seemingly supported by the underlying specs?)

 domain-value      = <subdomain>
                       ; defined in [RFC1034], Section 3.5, as
                       ; enhanced by [RFC1123], Section 2.1

https://tools.ietf.org/html/rfc1034#section-3.5
https://tools.ietf.org/html/rfc1123#section-2.1

I found it a bit strange that this additional validation wasn't noted in the release notes though since it might represent a breaking change for some.

@rstoyanchev
Copy link
Contributor

@chadlwilson this is reported in #23776 and will be fixed.

@rstoyanchev
Copy link
Contributor

rstoyanchev commented Oct 17, 2019

@chadlwilson actually never mind. The issue I referenced is for a leading dot. The syntax for the domain is from RFC 6265 (4.1.1) which does also refer to RFC 1034 (3.5) and 1123 (2.1) but I don't see anything in all that about port numbers.

@chadlwilson
Copy link

Yeah, I'm not disagreeing with the change or addition of validation, the validation is entirely reasonable. But it was a bit unexpected to come across this change in behaviour between a minor point release without reference in the release notes about validation being added to ResponseCookie construction. Took me a bit of digging to understand what had broken and why.

@rstoyanchev
Copy link
Contributor

Ok thanks for the feedback. I just wanted to double check since you pointed to RFCs. I've added a note to this wiki page. Let me know if you looking somewhere else.

@chadlwilson
Copy link

That's great, thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: bug A general bug
Projects
None yet
Development

No branches or pull requests

4 participants