From 921afba134032472e05069de60ec778f2b83a938 Mon Sep 17 00:00:00 2001 From: Marcus Hert Da Coregio Date: Tue, 14 Nov 2023 10:07:10 -0300 Subject: [PATCH] Use addCookie instead of addHeader in CookieCsrfTokenRepository By using addCookie we make sure that configured Tomcat's CookieProcessors are invoked Closes gh-14131 --- .../web/csrf/CookieCsrfTokenRepository.java | 19 +++++++++++++++-- .../csrf/CookieCsrfTokenRepositoryTests.java | 21 ++++++++++++++----- 2 files changed, 33 insertions(+), 7 deletions(-) diff --git a/web/src/main/java/org/springframework/security/web/csrf/CookieCsrfTokenRepository.java b/web/src/main/java/org/springframework/security/web/csrf/CookieCsrfTokenRepository.java index 86811c7fd4..7463f6aa15 100644 --- a/web/src/main/java/org/springframework/security/web/csrf/CookieCsrfTokenRepository.java +++ b/web/src/main/java/org/springframework/security/web/csrf/CookieCsrfTokenRepository.java @@ -23,7 +23,6 @@ import jakarta.servlet.http.Cookie; import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; -import org.springframework.http.HttpHeaders; import org.springframework.http.ResponseCookie; import org.springframework.util.Assert; import org.springframework.util.StringUtils; @@ -98,7 +97,8 @@ public final class CookieCsrfTokenRepository implements CsrfTokenRepository { this.cookieCustomizer.accept(cookieBuilder); - response.addHeader(HttpHeaders.SET_COOKIE, cookieBuilder.build().toString()); + Cookie cookie = mapToCookie(cookieBuilder.build()); + response.addCookie(cookie); // Set request attribute to signal that response has blank cookie value, // which allows loadToken to return null when token has been removed @@ -187,6 +187,21 @@ public final class CookieCsrfTokenRepository implements CsrfTokenRepository { return UUID.randomUUID().toString(); } + private Cookie mapToCookie(ResponseCookie responseCookie) { + Cookie cookie = new Cookie(responseCookie.getName(), responseCookie.getValue()); + cookie.setSecure(responseCookie.isSecure()); + cookie.setPath(responseCookie.getPath()); + cookie.setMaxAge((int) responseCookie.getMaxAge().getSeconds()); + cookie.setHttpOnly(responseCookie.isHttpOnly()); + if (StringUtils.hasLength(responseCookie.getDomain())) { + cookie.setDomain(responseCookie.getDomain()); + } + if (StringUtils.hasText(responseCookie.getSameSite())) { + cookie.setAttribute("SameSite", responseCookie.getSameSite()); + } + return cookie; + } + /** * Set the path that the Cookie will be created with. This will override the default * functionality which uses the request context as the path. diff --git a/web/src/test/java/org/springframework/security/web/csrf/CookieCsrfTokenRepositoryTests.java b/web/src/test/java/org/springframework/security/web/csrf/CookieCsrfTokenRepositoryTests.java index eb66521f89..05f73a0d89 100644 --- a/web/src/test/java/org/springframework/security/web/csrf/CookieCsrfTokenRepositoryTests.java +++ b/web/src/test/java/org/springframework/security/web/csrf/CookieCsrfTokenRepositoryTests.java @@ -21,12 +21,14 @@ import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Test; import org.springframework.http.HttpHeaders; -import org.springframework.mock.web.MockCookie; import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.mock.web.MockHttpServletResponse; import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.spy; +import static org.mockito.Mockito.verify; import static org.springframework.security.web.csrf.CsrfTokenAssert.assertThatCsrfToken; /** @@ -85,6 +87,15 @@ class CookieCsrfTokenRepositoryTests { assertThat(tokenCookie.isHttpOnly()).isTrue(); } + // gh-14131 + @Test + void saveTokenShouldUseResponseAddCookie() { + CsrfToken token = this.repository.generateToken(this.request); + MockHttpServletResponse spyResponse = spy(this.response); + this.repository.saveToken(token, this.request, spyResponse); + verify(spyResponse).addCookie(any(Cookie.class)); + } + @Test void saveTokenSecure() { this.request.setSecure(true); @@ -268,7 +279,7 @@ class CookieCsrfTokenRepositoryTests { CsrfToken token = this.repository.generateToken(this.request); this.repository.saveToken(token, this.request, this.response); Cookie tokenCookie = this.response.getCookie(CookieCsrfTokenRepository.DEFAULT_CSRF_COOKIE_NAME); - assertThat(((MockCookie) tokenCookie).getSameSite()).isNull(); + assertThat(tokenCookie.getAttribute("SameSite")).isNull(); } @Test @@ -278,7 +289,7 @@ class CookieCsrfTokenRepositoryTests { CsrfToken token = this.repository.generateToken(this.request); this.repository.saveToken(token, this.request, this.response); Cookie tokenCookie = this.response.getCookie(CookieCsrfTokenRepository.DEFAULT_CSRF_COOKIE_NAME); - assertThat(((MockCookie) tokenCookie).getSameSite()).isEqualTo(sameSitePolicy); + assertThat(tokenCookie.getAttribute("SameSite")).isEqualTo(sameSitePolicy); } @Test @@ -288,7 +299,7 @@ class CookieCsrfTokenRepositoryTests { CsrfToken token = this.repository.generateToken(this.request); this.repository.saveToken(token, this.request, this.response); Cookie tokenCookie = this.response.getCookie(CookieCsrfTokenRepository.DEFAULT_CSRF_COOKIE_NAME); - assertThat(((MockCookie) tokenCookie).getSameSite()).isEqualTo(sameSitePolicy); + assertThat(tokenCookie.getAttribute("SameSite")).isEqualTo(sameSitePolicy); } // gh-13075 @@ -420,7 +431,7 @@ class CookieCsrfTokenRepositoryTests { assertThat(tokenCookie.getDomain()).isEqualTo(domainName); assertThat(tokenCookie.getPath()).isEqualTo(customPath); assertThat(tokenCookie.isHttpOnly()).isEqualTo(Boolean.TRUE); - assertThat(((MockCookie) tokenCookie).getSameSite()).isEqualTo(sameSitePolicy); + assertThat(tokenCookie.getAttribute("SameSite")).isEqualTo(sameSitePolicy); } // gh-13659