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

Add Cookie's SameSite directive property #20912

Closed

Conversation

cleankod
Copy link

See #15047

Added the ability to configure the DefaultCookieSerializer's SameSite directive with an application property.

Fixes #15047

@pivotal-issuemaster
Copy link

@cleankod Please sign the Contributor License Agreement!

Click here to manually synchronize the status of this Pull Request.

See the FAQ for frequently asked questions.

@spring-projects-issues spring-projects-issues added the status: waiting-for-triage An issue we've not yet triaged label Apr 10, 2020
@pivotal-issuemaster
Copy link

@cleankod Thank you for signing the Contributor License Agreement!

@bclozel
Copy link
Member

bclozel commented Apr 11, 2020

Thanks for your contribution @cleankod - did you see @vpavic commenting on the related issue? It seems this change goes against the general opinion. See this comment.

Do you have new elements supporting this in the meantime?

@bclozel bclozel added the status: waiting-for-feedback We need additional information before we can continue label Apr 11, 2020
Copy link
Contributor

@vpavic vpavic left a comment

Choose a reason for hiding this comment

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

As discussed in #15047, until Servlet API rolls out SameSite support, binding a property from server.servlet.session.cookie namespace to something Spring Session specific isn't desirable IMO.

@cleankod
Copy link
Author

@bclozel, @vpavic
Since Google Chrome introduced the requirement for this attribute to be present, I really need to configure the application to include it in the Cookie. I have like 10 spring-boot-based web applications exposed to the browser.

I made this PR because when the time comes and the Servlet API releases the attribute as well, it would be completely transparent to me and all I'd need to do is upgrade Spring Boot.

Now, without the possibility to set this via property, it leaves me with the necessity to provide a DefaultCookieSerializer as a custom @Bean initialisation. So when the time comes with the Servlet API release, I'd need not only to upgrade Spring Boot, but also to remove all of the custom @Beans, checking of course that no one added anything to them after they were introduced.

I completely understand your reason, but in such cases I would consider leaning towards Spring Boot users since the update of Servlet API is surely coming because this directive became an official one and can be found in the specification. Waiting for Servlet API requires us to walk our way around this problem.

I'd therefore like to ask you to reconsider. If you stand by your decision anyway, then please consider merging my PR after the prerequisite Servlet API is released, so my work doesn't go to waste.

@spring-projects-issues spring-projects-issues added status: feedback-provided Feedback has been provided and removed status: waiting-for-feedback We need additional information before we can continue labels Apr 14, 2020
@vpavic
Copy link
Contributor

vpavic commented Apr 14, 2020

I don't think it's reasonable to expect Spring Boot to introduce a property under a Servlet API specific configuration property namespace for a thing that currently isn't by Servlet API and that requires an additional library (Spring Session) to be able to use it.

First and foremost, this is wrong as it's not usable if one's using Servlet container's default session management capabilities. Additionally, to my knowledge at least, the Servlet API support isn't imminent by any means and won't happen before 5.1.

I've proposed an alternative approach in #20961. This would add the capability to customize the SameSite setting of Spring Session's DefaultCookieSerializer by simply registering a bean like:

@Bean
CookieSerializerCustomizer cookieSerializerCustomizer() {
	return cookieSerializer -> cookieSerializer.setSameSite("None");
}

@adamklinkosz

This comment has been minimized.

@snicoll

This comment has been minimized.

@bclozel bclozel added status: superseded An issue that has been superseded by another and removed status: feedback-provided Feedback has been provided status: waiting-for-triage An issue we've not yet triaged labels Apr 15, 2020
@bclozel
Copy link
Member

bclozel commented Apr 15, 2020

I'm closing this PR in favor of #20961, as it is less tied to the Servlet spec - thanks @cleankod for your contribution!

@bclozel bclozel closed this Apr 15, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
status: superseded An issue that has been superseded by another
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants