Remove Servlet 2.5 Support for Session Fixation #6297
Conversation
jzheaux
added
in: web
...ringframework/security/web/authentication/session/ChangeSessionIdAuthenticationStrategy.java
Show resolved
Hide resolved
.../springframework/security/config/annotation/web/configurers/SessionManagementConfigurer.java
Show resolved
Hide resolved
...ramework/security/web/authentication/session/ChangeSessionIdAuthenticationStrategyTests.java
Show resolved
Hide resolved
raphaelDL
force-pushed
the
gh-6259
branch
5 times, most recently
from
5326f3c to
0080db0
Compare
There was a problem hiding this comment.
Looking good, @raphaelDL. I've commented inline about a little more polish in the comments and the tests. We should be just about there.
| @@ -24,8 +24,7 @@ | |||
| import javax.servlet.http.HttpSession; | |||
|
|
|||
| /** | |||
| * The default implementation of {@link SessionAuthenticationStrategy} when using < | |||
| * Servlet 3.1. | |||
| * The default implementation of {@link SessionAuthenticationStrategy}. | |||
There was a problem hiding this comment.
So, this actually is not the default implementation. It's tricky because of the encoded less than, but it said "The default implementation is X when using < Servlet 3.1". Since we no longer support < Servlet 3.1, this won't be the default anymore.
| verifyStatic(ReflectionUtils.class); | ||
| ReflectionUtils.invokeMethod(same(method), any(HttpServletRequest.class)); | ||
|
|
||
| Assert.assertNotEquals(id, request.getSession().getId()); |
There was a problem hiding this comment.
Would you do me a favor and please use AssertJ instead? assertThat(request.getSession().getId()).isEqualTo(id)
| ReflectionUtils.invokeMethod(same(method), any(HttpServletRequest.class)); | ||
|
|
||
| Assert.assertNotEquals(id, request.getSession().getId()); | ||
| Assert.assertEquals(request.getParameter(token.getParameterName()), token.getToken()); |
There was a problem hiding this comment.
What are you wanting to verify by checking the request parameters? Since they aren't part of the session, they wouldn't be affected by session changes.
Not sure if this is what you are going for, but to confirm that session fixation didn't squash anything the session, we'd need to add a session attribute, e.g. request.getSession().setAttribute("custom", "value")
There was a problem hiding this comment.
that was the idea, I changed that
| /** | ||
| * Uses {@code HttpServletRequest.changeSessionId()} to protect against session fixation | ||
| * attacks. This is the default implementation for Servlet 3.1+. | ||
| * attacks. |
There was a problem hiding this comment.
This is now the default implementation.
web/src/main/java/org/springframework/security/web/servletapi/HttpServletRequestFactory.java
Outdated
Show resolved
Hide resolved
| * <p> | ||
| * In a servlet 3 environment {@link SecurityContextHolderAwareRequestWrapper} is extended | ||
| * to provide the following additional methods: | ||
| * {@link SecurityContextHolderAwareRequestWrapper} is extended to provide the following |
There was a problem hiding this comment.
There is a separate ticket for this class, so let's go ahead and leave these changes out in this file out. Thank you, though!
There was a problem hiding this comment.
on it, wait a second, messed up my branch D:
raphaelDL
force-pushed
the
gh-6259
branch
3 times, most recently
from
7d98083 to
12f412f
Compare
|
I'm working to fix the red build |
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: spring-projectsgh-6259
|
Okay, @raphaelDL, I'm just going to do a quick polish, and then we'll merge. |
|
Thanks again, @raphaelDL! This is now merged into |
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