Add the ability to set the SameSite policy to the CRSF Cookie #12109
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add a new public method to the class org.springframework.security.web.csrf.CookieCsrfTokenRepository that allows setting the SameSite policy as an optional parameter; with this, when sending the CSRF token in a cookie instead of a header, no warning or error should be displayed in the browser's console. Removes unnecessary default no-parameters empty constructor. Fix gh-12086
spring-projects-issues
added
the
status: waiting-for-triage
jzheaux
requested changes
Nov 15, 2022
web/src/main/java/org/springframework/security/web/csrf/CookieCsrfTokenRepository.java
Outdated
Show resolved
Hide resolved
web/src/main/java/org/springframework/security/web/csrf/CookieCsrfTokenRepository.java
Outdated
Show resolved
Hide resolved
web/src/main/java/org/springframework/security/web/csrf/CookieCsrfTokenRepository.java
Outdated
Show resolved
Hide resolved
jzheaux
added
in: web
- Mark setCookieHttpOnly, setCookieDomain, setCookieMaxAge and setSecure as deprecated. - Add the method addCookieInitializer which allows to set properties to the ResponseCookieBuilder without having to add new setter methods. - Apply same changes to CookieServerCsrfTokenRepository. - Add unit tests to validate the use of same site policy.
|
Hi, @jzheaux; thanks for the comments. I made the requested changes. Please let me know if you consider any other changes required. |
jzheaux
requested changes
Nov 15, 2022
web/src/main/java/org/springframework/security/web/csrf/CookieCsrfTokenRepository.java
Outdated
Show resolved
Hide resolved
web/src/main/java/org/springframework/security/web/csrf/CookieCsrfTokenRepository.java
Outdated
Show resolved
Hide resolved
web/src/test/java/org/springframework/security/web/csrf/CookieCsrfTokenRepositoryTests.java
Show resolved
Hide resolved
web/src/test/java/org/springframework/security/web/csrf/CookieCsrfTokenRepositoryTests.java
Outdated
Show resolved
Hide resolved
.../java/org/springframework/security/web/server/csrf/CookieServerCsrfTokenRepositoryTests.java
Show resolved
Hide resolved
- Mark setCookieHttpOnly, setCookieDomain, setCookieMaxAge and setSecure as deprecated. - Add the method setCookieCustomizer which allows to set properties to the ResponseCookieBuilder without having to add new setter methods. - Apply same changes to CookieServerCsrfTokenRepository. - Add unit tests to validate the use of same site policy.
- Mark setCookieHttpOnly, setCookieDomain, setCookieMaxAge and setSecure as deprecated. - Add the method setCookieCustomizer which allows to set properties to the ResponseCookieBuilder without having to add new setter methods. - Apply same changes to CookieServerCsrfTokenRepository. - Add unit tests to validate the use of same site policy.
- Changes to use assertJ semantic methods as isTrue and isNull
|
Hi @jzheaux, thanks for the comments and suggestions. I addressed them. |
jzheaux
requested changes
Nov 17, 2022
There was a problem hiding this comment.
Awesome, @kumo829! I left two minor items for you inline.
In preparation for merging, will you please squash your commits into two; one for the feature and one for the Junit cleanup? In the feature commit, will you please format it to include the ticket like so:
Your Commit Title
Any additional description
Closes gh-12086
Doing it this way makes so that when the PR is merged, the ticket is automatically closed.
| .httpOnly(this.cookieHttpOnly) | ||
| .domain(this.cookieDomain); | ||
|
|
||
| this.cookieCustomizer.accept(cookieBuilder); |
There was a problem hiding this comment.
The indentation looks off here. Would you mind adjusting it, please?
| * @since 6.1 | ||
| */ | ||
| public void setCookieCustomizer(Consumer<ResponseCookie.ResponseCookieBuilder> cookieCustomizer) { | ||
| this.cookieCustomizer = cookieCustomizer; |
There was a problem hiding this comment.
Please check for null. You can use Assert.notNull(cookieCustomizer, "cookieCustomizer must not be null").
…enRepository - Mark setCookieHttpOnly, setCookieDomain, setCookieMaxAge and setSecure as deprecated. - Add the method setCookieCustomizer which allows to set properties to the ResponseCookieBuilder without having to add new setter methods. Closes gh-12086
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Add a new public method to the class org.springframework.security.web.csrf.CookieCsrfTokenRepository that allows setting the SameSite policy as an optional parameter; with this, when sending the CSRF token in a cookie instead of a header, no warning or error should be displayed in the browser's console.
Removes unnecessary default no-parameters empty constructor.
This could fix gh-12086