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

CookieWebSessionIdResolver should leverage SameSite Cookie Attribute [SPR-16418] #20964

Closed
spring-projects-issues opened this issue Jan 25, 2018 · 11 comments
Assignees
Labels
in: web Issues in web modules (web, webmvc, webflux, websocket) type: enhancement A general enhancement
Milestone

Comments

@spring-projects-issues
Copy link
Collaborator

spring-projects-issues commented Jan 25, 2018

Rob Winch opened SPR-16418 and commented

It may be a little early for this given the limited Browser support, but we should consider setting the SameSite attribute on session cookies as this will prevent CSRF attacks without the need for any special code.

See https://tools.ietf.org/html/draft-west-first-party-cookies-07


Issue Links:

0 votes, 5 watchers

@spring-projects-issues
Copy link
Collaborator Author

Juergen Hoeller commented

This seems to be supported in Chrome already and in the works for Firefox, so I suppose our 5.1 timeline might be a good fit.

@spring-projects-issues
Copy link
Collaborator Author

spring-projects-issues commented Jun 29, 2018

Rossen Stoyanchev commented

As part of #20964, I've added a Consumer based method that can customize anything about a cookie. Considering that SameSite is set by default, perhaps we could remove the new SameSite-related methods that were just added? The same can easily be achieved now without them.

@spring-projects-issues
Copy link
Collaborator Author

Rob Winch commented

I think it is quite a bit less obvious if we remove the SameSite related methods, so I'd personally prefer to keep them there.

@spring-projects-issues
Copy link
Collaborator Author

Rossen Stoyanchev commented

The only reason it occurred to me is because SameSite is set to "Strict" by default and perhaps I thought setting it explicitly might be a less common, and more advanced option?

@spring-projects-issues
Copy link
Collaborator Author

Rob Winch commented

I guess I'm missing what exactly would be done. It sounded as though these methods would be removed which means the property would be removed too.

If that is removed, it appears that sameSite will be null since it is null in the builder by default. However, you were saying that SameSite is set to "Strict" by default. I think I'm missing something?

@spring-projects-issues
Copy link
Collaborator Author

Rossen Stoyanchev commented

Good point that the sameSite property of ResponseCookie is not set by default. Is that intentional or is it simply not an issue since CookieWebSessionidResolver sets it anyway? Effectively right now "Strict" is the default, and if that's the preferred default setting, we can make sure it's set that way at the level of ResponseCookie.

@spring-projects-issues
Copy link
Collaborator Author

Rob Winch commented

I believe it is just not an issue since CookieWebSessionIdResolver sets it anyways. If the value were defaulted in the builder, then I think that would also achieve the goal of this ticket.

@spring-projects-issues
Copy link
Collaborator Author

Rossen Stoyanchev commented

This is the resulting update, in the end keeping everything the same except the sameSite property on CookieWebSessionIdResolver is effectively superceded by addCookieInitializer.

@spring-projects-issues
Copy link
Collaborator Author

Vedran Pavic commented

Isn't Strict as a default value for SameSite attribute bit too aggressive? If site A issues a session cookie with SameSite set to Strict that would effectively mean that if users follows a link to site A from any other site, they wouldn't be logged in even though they have a valid session. This can be a bit confusing.

See this article for some considerations around the topic. The owasp.org also supports Lax as default since it provides a reasonable balance between security and usability.

@spring-projects-issues
Copy link
Collaborator Author

Rossen Stoyanchev commented

There is now a suggested change #1889 along with comments above.

Rob Winch

@spring-projects-issues
Copy link
Collaborator Author

Rob Winch commented

Thanks for pinging me Rossen. I agree with Vedran's suggestion. Thanks for the PR and comments Vedran!

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: enhancement A general enhancement
Projects
None yet
Development

No branches or pull requests

2 participants