From 086b105273734adb0115c8f5d360a335b6132cb4 Mon Sep 17 00:00:00 2001 From: Rafael Dominguez <5624449+raphaelDL@users.noreply.github.com> Date: Mon, 17 Dec 2018 13:03:18 -0600 Subject: [PATCH] Remove Servlet 2.5 Support for Session Fixation This commit removes existence validation of a method only available in Servlet 3.1. Spring Framework baseline is Servlet 3.1 so is not longer required. Fixes: gh-6259 --- .../SessionManagementConfigurer.java | 14 ++----- .../config/http/HttpConfigurationBuilder.java | 8 +--- ...ionManagementConfigurerServlet31Tests.java | 23 +++-------- ...SessionManagementConfigServlet31Tests.java | 34 +++++++--------- ...ChangeSessionIdAuthenticationStrategy.java | 19 +-------- .../SessionFixationProtectionStrategy.java | 2 +- ...eSessionIdAuthenticationStrategyTests.java | 39 +++---------------- 7 files changed, 32 insertions(+), 107 deletions(-) diff --git a/config/src/main/java/org/springframework/security/config/annotation/web/configurers/SessionManagementConfigurer.java b/config/src/main/java/org/springframework/security/config/annotation/web/configurers/SessionManagementConfigurer.java index e128c7a7d8..3fb87b818f 100644 --- a/config/src/main/java/org/springframework/security/config/annotation/web/configurers/SessionManagementConfigurer.java +++ b/config/src/main/java/org/springframework/security/config/annotation/web/configurers/SessionManagementConfigurer.java @@ -212,8 +212,7 @@ public final class SessionManagementConfigurer> /** * Allows explicitly specifying the {@link SessionAuthenticationStrategy}. - * The default is to use {@link SessionFixationProtectionStrategy} for Servlet 3.1 or - * {@link ChangeSessionIdAuthenticationStrategy} for Servlet 3.1+. + * The default is to use {@link ChangeSessionIdAuthenticationStrategy}. * If restricting the maximum number of sessions is configured, then * {@link CompositeSessionAuthenticationStrategy} delegating to * {@link ConcurrentSessionControlAuthenticationStrategy}, @@ -305,13 +304,11 @@ public final class SessionManagementConfigurer> /** * Specifies that the Servlet container-provided session fixation protection - * should be used. When a session authenticates, the Servlet 3.1 method + * should be used. When a session authenticates, the Servlet method * {@code HttpServletRequest#changeSessionId()} is called to change the session ID - * and retain all session attributes. Using this option in a Servlet 3.0 or older - * container results in an {@link IllegalStateException}. + * and retain all session attributes. * * @return the {@link SessionManagementConfigurer} for further customizations - * @throws IllegalStateException if the container is not Servlet 3.1 or newer. */ public SessionManagementConfigurer changeSessionId() { setSessionFixationAuthenticationStrategy( @@ -664,11 +661,6 @@ public final class SessionManagementConfigurer> * @return the default {@link SessionAuthenticationStrategy} for session fixation */ private static SessionAuthenticationStrategy createDefaultSessionFixationProtectionStrategy() { - try { return new ChangeSessionIdAuthenticationStrategy(); - } - catch (IllegalStateException e) { - return new SessionFixationProtectionStrategy(); - } } } diff --git a/config/src/main/java/org/springframework/security/config/http/HttpConfigurationBuilder.java b/config/src/main/java/org/springframework/security/config/http/HttpConfigurationBuilder.java index 3c077c3f7b..62377644f5 100644 --- a/config/src/main/java/org/springframework/security/config/http/HttpConfigurationBuilder.java +++ b/config/src/main/java/org/springframework/security/config/http/HttpConfigurationBuilder.java @@ -15,12 +15,10 @@ */ package org.springframework.security.config.http; -import java.lang.reflect.Method; import java.util.ArrayList; import java.util.List; import javax.servlet.ServletRequest; -import javax.servlet.http.HttpServletRequest; import org.w3c.dom.Element; @@ -73,7 +71,6 @@ import org.springframework.security.web.session.SimpleRedirectInvalidSessionStra import org.springframework.security.web.session.SimpleRedirectSessionInformationExpiredStrategy; import org.springframework.security.web.util.matcher.AntPathRequestMatcher; import org.springframework.util.ClassUtils; -import org.springframework.util.ReflectionUtils; import org.springframework.util.StringUtils; import org.springframework.util.xml.DomUtils; @@ -350,10 +347,7 @@ class HttpConfigurationBuilder { } if (!StringUtils.hasText(sessionFixationAttribute)) { - Method changeSessionIdMethod = ReflectionUtils.findMethod( - HttpServletRequest.class, "changeSessionId"); - sessionFixationAttribute = changeSessionIdMethod == null ? OPT_SESSION_FIXATION_MIGRATE_SESSION - : OPT_CHANGE_SESSION_ID; + sessionFixationAttribute = OPT_CHANGE_SESSION_ID; } else if (StringUtils.hasText(sessionAuthStratRef)) { pc.getReaderContext().error( diff --git a/config/src/test/java/org/springframework/security/config/annotation/web/configurers/SessionManagementConfigurerServlet31Tests.java b/config/src/test/java/org/springframework/security/config/annotation/web/configurers/SessionManagementConfigurerServlet31Tests.java index 83b236cd2e..c1c7e7f29e 100644 --- a/config/src/test/java/org/springframework/security/config/annotation/web/configurers/SessionManagementConfigurerServlet31Tests.java +++ b/config/src/test/java/org/springframework/security/config/annotation/web/configurers/SessionManagementConfigurerServlet31Tests.java @@ -17,7 +17,6 @@ package org.springframework.security.config.annotation.web.configurers; import java.lang.reflect.Method; import javax.servlet.Filter; -import javax.servlet.http.HttpServletRequest; import org.junit.After; import org.junit.Before; @@ -25,7 +24,6 @@ import org.junit.Test; import org.junit.runner.RunWith; import org.mockito.Mock; import org.powermock.core.classloader.annotations.PowerMockIgnore; -import org.powermock.core.classloader.annotations.PrepareForTest; import org.powermock.modules.junit4.PowerMockRunner; import org.springframework.context.ConfigurableApplicationContext; @@ -44,21 +42,14 @@ import org.springframework.security.web.context.HttpRequestResponseHolder; import org.springframework.security.web.context.HttpSessionSecurityContextRepository; import org.springframework.security.web.csrf.CsrfToken; import org.springframework.security.web.csrf.HttpSessionCsrfTokenRepository; -import org.springframework.util.ReflectionUtils; -import static org.mockito.Matchers.any; -import static org.mockito.Matchers.same; -import static org.powermock.api.mockito.PowerMockito.mock; -import static org.powermock.api.mockito.PowerMockito.spy; -import static org.powermock.api.mockito.PowerMockito.verifyStatic; -import static org.powermock.api.mockito.PowerMockito.when; +import static org.assertj.core.api.AssertionsForInterfaceTypes.assertThat; /** * * @author Rob Winch */ @RunWith(PowerMockRunner.class) -@PrepareForTest({ ReflectionUtils.class, Method.class }) @PowerMockIgnore({ "org.w3c.dom.*", "org.xml.sax.*", "org.apache.xerces.*", "javax.xml.parsers.*" }) public class SessionManagementConfigurerServlet31Tests { @Mock @@ -87,10 +78,9 @@ public class SessionManagementConfigurerServlet31Tests { } @Test - public void changeSessionIdDefaultsInServlet31Plus() throws Exception { - spy(ReflectionUtils.class); - Method method = mock(Method.class); + public void changeSessionIdThenPreserveParameters() throws Exception { MockHttpServletRequest request = new MockHttpServletRequest("GET", ""); + String id = request.getSession().getId(); request.getSession(); request.setServletPath("/login"); request.setMethod("POST"); @@ -100,15 +90,14 @@ public class SessionManagementConfigurerServlet31Tests { CsrfToken token = repository.generateToken(request); repository.saveToken(token, request, response); request.setParameter(token.getParameterName(), token.getToken()); - when(ReflectionUtils.findMethod(HttpServletRequest.class, "changeSessionId")) - .thenReturn(method); + request.getSession().setAttribute("attribute1", "value1"); loadConfig(SessionManagementDefaultSessionFixationServlet31Config.class); springSecurityFilterChain.doFilter(request, response, chain); - verifyStatic(ReflectionUtils.class); - ReflectionUtils.invokeMethod(same(method), any(HttpServletRequest.class)); + assertThat(!request.getSession().getId().equals(id)); + assertThat(request.getSession().getAttribute("attribute1").equals("value1")); } @EnableWebSecurity diff --git a/config/src/test/java/org/springframework/security/config/http/SessionManagementConfigServlet31Tests.java b/config/src/test/java/org/springframework/security/config/http/SessionManagementConfigServlet31Tests.java index b4a1c79d39..08c694081c 100644 --- a/config/src/test/java/org/springframework/security/config/http/SessionManagementConfigServlet31Tests.java +++ b/config/src/test/java/org/springframework/security/config/http/SessionManagementConfigServlet31Tests.java @@ -17,7 +17,6 @@ package org.springframework.security.config.http; import java.lang.reflect.Method; import javax.servlet.Filter; -import javax.servlet.http.HttpServletRequest; import org.junit.After; import org.junit.Before; @@ -39,12 +38,7 @@ import org.springframework.security.web.context.HttpRequestResponseHolder; import org.springframework.security.web.context.HttpSessionSecurityContextRepository; import org.springframework.util.ReflectionUtils; -import static org.mockito.Matchers.any; -import static org.mockito.Matchers.same; -import static org.powermock.api.mockito.PowerMockito.mock; -import static org.powermock.api.mockito.PowerMockito.spy; -import static org.powermock.api.mockito.PowerMockito.verifyStatic; -import static org.powermock.api.mockito.PowerMockito.when; +import static org.assertj.core.api.AssertionsForClassTypes.assertThat; /** * @author Rob Winch @@ -86,17 +80,17 @@ public class SessionManagementConfigServlet31Tests { } @Test - public void changeSessionIdDefaultsInServlet31Plus() throws Exception { - spy(ReflectionUtils.class); - Method method = mock(Method.class); + public void changeSessionIdThenPreserveParameters() throws Exception { MockHttpServletRequest request = new MockHttpServletRequest("GET", ""); request.getSession(); request.setServletPath("/login"); request.setMethod("POST"); request.setParameter("username", "user"); request.setParameter("password", "password"); - when(ReflectionUtils.findMethod(HttpServletRequest.class, "changeSessionId")) - .thenReturn(method); + + request.getSession().setAttribute("attribute1", "value1"); + + String id = request.getSession().getId(); loadContext("\n" + " \n" + " \n" + " \n" @@ -104,22 +98,22 @@ public class SessionManagementConfigServlet31Tests { springSecurityFilterChain.doFilter(request, response, chain); - verifyStatic(ReflectionUtils.class); - ReflectionUtils.invokeMethod(same(method), any(HttpServletRequest.class)); + + assertThat(!request.getSession().getId().equals(id)); + assertThat(request.getSession().getAttribute("attribute1").equals("value1")); } @Test public void changeSessionId() throws Exception { - spy(ReflectionUtils.class); - Method method = mock(Method.class); + MockHttpServletRequest request = new MockHttpServletRequest("GET", ""); request.getSession(); request.setServletPath("/login"); request.setMethod("POST"); request.setParameter("username", "user"); request.setParameter("password", "password"); - when(ReflectionUtils.findMethod(HttpServletRequest.class, "changeSessionId")) - .thenReturn(method); + + String id = request.getSession().getId(); loadContext("\n" + " \n" @@ -129,8 +123,8 @@ public class SessionManagementConfigServlet31Tests { springSecurityFilterChain.doFilter(request, response, chain); - verifyStatic(ReflectionUtils.class); - ReflectionUtils.invokeMethod(same(method), any(HttpServletRequest.class)); + assertThat(!request.getSession().getId().equals(id)); + } private void loadContext(String context) { diff --git a/web/src/main/java/org/springframework/security/web/authentication/session/ChangeSessionIdAuthenticationStrategy.java b/web/src/main/java/org/springframework/security/web/authentication/session/ChangeSessionIdAuthenticationStrategy.java index 3b5b332083..269a3c4f7c 100644 --- a/web/src/main/java/org/springframework/security/web/authentication/session/ChangeSessionIdAuthenticationStrategy.java +++ b/web/src/main/java/org/springframework/security/web/authentication/session/ChangeSessionIdAuthenticationStrategy.java @@ -15,33 +15,18 @@ */ package org.springframework.security.web.authentication.session; -import java.lang.reflect.Method; - import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpSession; -import org.springframework.util.ReflectionUtils; - /** * Uses {@code HttpServletRequest.changeSessionId()} to protect against session fixation - * attacks. This is the default implementation for Servlet 3.1+. + * attacks. This is the default implementation. * * @author Rob Winch * @since 3.2 */ public final class ChangeSessionIdAuthenticationStrategy extends AbstractSessionFixationProtectionStrategy { - private final Method changeSessionIdMethod; - - public ChangeSessionIdAuthenticationStrategy() { - Method changeSessionIdMethod = ReflectionUtils - .findMethod(HttpServletRequest.class, "changeSessionId"); - if (changeSessionIdMethod == null) { - throw new IllegalStateException( - "HttpServletRequest.changeSessionId is undefined. Are you using a Servlet 3.1+ environment?"); - } - this.changeSessionIdMethod = changeSessionIdMethod; - } /* * (non-Javadoc) @@ -52,7 +37,7 @@ public final class ChangeSessionIdAuthenticationStrategy */ @Override HttpSession applySessionFixation(HttpServletRequest request) { - ReflectionUtils.invokeMethod(this.changeSessionIdMethod, request); + request.changeSessionId(); return request.getSession(); } } diff --git a/web/src/main/java/org/springframework/security/web/authentication/session/SessionFixationProtectionStrategy.java b/web/src/main/java/org/springframework/security/web/authentication/session/SessionFixationProtectionStrategy.java index f51ddd1059..ad76962b6a 100644 --- a/web/src/main/java/org/springframework/security/web/authentication/session/SessionFixationProtectionStrategy.java +++ b/web/src/main/java/org/springframework/security/web/authentication/session/SessionFixationProtectionStrategy.java @@ -24,7 +24,7 @@ import javax.servlet.http.HttpServletRequest; import javax.servlet.http.HttpSession; /** - * The default implementation of {@link SessionAuthenticationStrategy} when using < + * The implementation of {@link SessionAuthenticationStrategy} when using < * Servlet 3.1. *

* Creates a new session for the newly authenticated user if they already have a session diff --git a/web/src/test/java/org/springframework/security/web/authentication/session/ChangeSessionIdAuthenticationStrategyTests.java b/web/src/test/java/org/springframework/security/web/authentication/session/ChangeSessionIdAuthenticationStrategyTests.java index 24f68c3e62..2ed85347f1 100644 --- a/web/src/test/java/org/springframework/security/web/authentication/session/ChangeSessionIdAuthenticationStrategyTests.java +++ b/web/src/test/java/org/springframework/security/web/authentication/session/ChangeSessionIdAuthenticationStrategyTests.java @@ -15,55 +15,26 @@ */ package org.springframework.security.web.authentication.session; -import static org.mockito.Matchers.*; -import static org.powermock.api.mockito.PowerMockito.*; - -import java.lang.reflect.Method; - -import javax.servlet.http.HttpServletRequest; - +import org.junit.Assert; import org.junit.Test; import org.junit.runner.RunWith; -import org.mockito.Mock; -import org.powermock.core.classloader.annotations.PrepareForTest; import org.powermock.modules.junit4.PowerMockRunner; import org.springframework.mock.web.MockHttpServletRequest; -import org.springframework.util.ReflectionUtils; /** * @author Rob Winch * */ @RunWith(PowerMockRunner.class) -@PrepareForTest({ ReflectionUtils.class, Method.class }) public class ChangeSessionIdAuthenticationStrategyTests { - @Mock - private Method method; - - @Test(expected = IllegalStateException.class) - public void constructChangeIdMethodNotFound() { - spy(ReflectionUtils.class); - MockHttpServletRequest request = new MockHttpServletRequest(); - request.getSession(); - when(ReflectionUtils.findMethod(HttpServletRequest.class, "changeSessionId")) - .thenReturn(null); - - new ChangeSessionIdAuthenticationStrategy(); - } @Test - public void applySessionFixation() throws Exception { - spy(ReflectionUtils.class); - Method method = mock(Method.class); + public void applySessionFixation() { MockHttpServletRequest request = new MockHttpServletRequest(); - request.getSession(); - when(ReflectionUtils.findMethod(HttpServletRequest.class, "changeSessionId")) - .thenReturn(method); + String id = request.getSession().getId(); - new ChangeSessionIdAuthenticationStrategy().applySessionFixation(request); + new ChangeSessionIdAuthenticationStrategy().applySessionFixation(request); - verifyStatic(ReflectionUtils.class); - ReflectionUtils.invokeMethod(same(method), eq(request)); + Assert.assertNotEquals(id, request.getSession().getId()); } - }