Skip to content

Commit 2d1200b

Browse files
klouds27klouds27
committed
Fix SessionRegistryImpl not tracking session ID changes without HttpSessionEventPublisher
When ChangeSessionIdAuthenticationStrategy rotates the session ID during authentication, SessionRegistryImpl only noticed the change if HttpSessionEventPublisher was registered as a servlet listener to publish HttpSessionIdChangedEvent. Without it, the old session ID remained in the registry as a ghost entry, causing ConcurrentSessionControlAuthenticationStrategy to count one extra session per login and incorrectly expire valid sessions. Fix: AbstractSessionFixationProtectionStrategy.onSessionChange() now also publishes a SessionFixationProtectionSessionIdChangedEvent (extending SessionIdChangedEvent), which SessionRegistryImpl already handles. This removes the dependency on HttpSessionEventPublisher for session-fixation-induced ID changes; HttpSessionEventPublisher is still required for expiry notifications. Closes gh-19007 Signed-off-by: admin <adalwolfad@gmail.com>
1 parent 6555199 commit 2d1200b

6 files changed

Lines changed: 161 additions & 23 deletions

File tree

‎core/src/main/java/org/springframework/security/core/session/SessionRegistryImpl.java‎

Lines changed: 11 additions & 5 deletions
Original file line numberDiff line numberDiff line change
@@ -38,12 +38,18 @@
3838
* Default implementation of
3939
* {@link org.springframework.security.core.session.SessionRegistry SessionRegistry} which
4040
* listens for {@link org.springframework.security.core.session.SessionDestroyedEvent
41-
* SessionDestroyedEvent}s published in the Spring application context.
41+
* SessionDestroyedEvent}s and
42+
* {@link org.springframework.security.core.session.SessionIdChangedEvent
43+
* SessionIdChangedEvent}s published in the Spring application context.
4244
* <p>
43-
* For this class to function correctly in a web application, it is important that you
44-
* register an <a href="
45-
* {@docRoot}/org/springframework/security/web/session/HttpSessionEventPublisher.html">HttpSessionEventPublisher</a> in
46-
* the <tt>web.xml</tt> file so that this class is notified of sessions that expire.
45+
* For this class to be notified of sessions that expire, you must register an <a href="
46+
* {@docRoot}/org/springframework/security/web/session/HttpSessionEventPublisher.html">HttpSessionEventPublisher</a>
47+
* in the {@code web.xml} file (or equivalent servlet container configuration).
48+
* <p>
49+
* Session ID changes that occur as part of session fixation protection (e.g. via
50+
* {@link org.springframework.security.web.authentication.session.ChangeSessionIdAuthenticationStrategy})
51+
* are tracked automatically without requiring
52+
* {@code HttpSessionEventPublisher} to be registered.
4753
*
4854
* @author Ben Alex
4955
* @author Luke Taylor

‎core/src/test/java/org/springframework/security/core/session/SessionRegistryImplTests.java‎

Lines changed: 23 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -192,6 +192,29 @@ public String getNewSessionId() {
192192
assertThat(this.sessionRegistry.getSessionInformation(newSessionId)).isNull();
193193
}
194194

195+
@Test
196+
public void sessionIdChangedEventWhenOldSessionRegisteredThenMigratesSessionWithoutHttpSessionEventPublisher() {
197+
Object principal = "Some principal object";
198+
final String oldSessionId = "old-session-id";
199+
final String newSessionId = "new-session-id";
200+
this.sessionRegistry.registerNewSession(oldSessionId, principal);
201+
this.sessionRegistry.onApplicationEvent(new SessionIdChangedEvent("") {
202+
@Override
203+
public String getOldSessionId() {
204+
return oldSessionId;
205+
}
206+
207+
@Override
208+
public String getNewSessionId() {
209+
return newSessionId;
210+
}
211+
});
212+
assertThat(this.sessionRegistry.getSessionInformation(oldSessionId)).isNull();
213+
assertThat(this.sessionRegistry.getSessionInformation(newSessionId)).isNotNull();
214+
assertThat(this.sessionRegistry.getSessionInformation(newSessionId).getPrincipal()).isEqualTo(principal);
215+
assertThat(this.sessionRegistry.getAllSessions(principal, false)).hasSize(1);
216+
}
217+
195218
private boolean contains(String sessionId, Object principal) {
196219
List<SessionInformation> info = this.sessionRegistry.getAllSessions(principal, false);
197220
for (SessionInformation sessionInformation : info) {

‎web/src/main/java/org/springframework/security/web/authentication/session/AbstractSessionFixationProtectionStrategy.java‎

Lines changed: 10 additions & 4 deletions
Original file line numberDiff line numberDiff line change
@@ -116,17 +116,23 @@ public void onAuthentication(Authentication authentication, HttpServletRequest r
116116
* subclasses to plug in additional behaviour. *
117117
* <p>
118118
* The default implementation of this method publishes a
119-
* {@link SessionFixationProtectionEvent} to notify the application that the session
120-
* ID has changed. If you override this method and still wish these events to be
121-
* published, you should call {@code super.onSessionChange()} within your overriding
122-
* method.
119+
* {@link SessionFixationProtectionEvent} and a
120+
* {@link SessionFixationProtectionSessionIdChangedEvent} to notify the application
121+
* that the session ID has changed. The latter allows
122+
* {@link org.springframework.security.core.session.SessionRegistryImpl} to track the
123+
* session ID change without requiring
124+
* {@link org.springframework.security.web.session.HttpSessionEventPublisher} to be
125+
* registered. If you override this method and still wish these events to be published,
126+
* you should call {@code super.onSessionChange()} within your overriding method.
123127
* @param originalSessionId the original session identifier
124128
* @param newSession the newly created session
125129
* @param auth the token for the newly authenticated principal
126130
*/
127131
protected void onSessionChange(String originalSessionId, HttpSession newSession, Authentication auth) {
128132
this.applicationEventPublisher
129133
.publishEvent(new SessionFixationProtectionEvent(auth, originalSessionId, newSession.getId()));
134+
this.applicationEventPublisher
135+
.publishEvent(new SessionFixationProtectionSessionIdChangedEvent(newSession, originalSessionId));
130136
}
131137

132138
/**

‎web/src/main/java/org/springframework/security/web/authentication/session/SessionFixationProtectionSessionIdChangedEvent.java‎

Lines changed: 62 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,62 @@
1+
/*
2+
* Copyright 2004-present the original author or authors.
3+
*
4+
* Licensed under the Apache License, Version 2.0 (the "License");
5+
* you may not use this file except in compliance with the License.
6+
* You may obtain a copy of the License at
7+
*
8+
* https://www.apache.org/licenses/LICENSE-2.0
9+
*
10+
* Unless required by applicable law or agreed to in writing, software
11+
* distributed under the License is distributed on an "AS IS" BASIS,
12+
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
13+
* See the License for the specific language governing permissions and
14+
* limitations under the License.
15+
*/
16+
17+
package org.springframework.security.web.authentication.session;
18+
19+
import java.io.Serial;
20+
21+
import jakarta.servlet.http.HttpSession;
22+
23+
import org.springframework.security.core.session.SessionIdChangedEvent;
24+
25+
/**
26+
* Published by {@link AbstractSessionFixationProtectionStrategy} when a session ID
27+
* changes during session fixation protection. This allows
28+
* {@link org.springframework.security.core.session.SessionRegistryImpl} to track the
29+
* session ID change without requiring
30+
* {@link org.springframework.security.web.session.HttpSessionEventPublisher} to be
31+
* registered.
32+
*
33+
* @author Adolfo Gonzalez
34+
* @since 6.5
35+
* @see AbstractSessionFixationProtectionStrategy
36+
*/
37+
class SessionFixationProtectionSessionIdChangedEvent extends SessionIdChangedEvent {
38+
39+
@Serial
40+
private static final long serialVersionUID = 1L;
41+
42+
private final String oldSessionId;
43+
44+
private final String newSessionId;
45+
46+
SessionFixationProtectionSessionIdChangedEvent(HttpSession newSession, String oldSessionId) {
47+
super(newSession);
48+
this.oldSessionId = oldSessionId;
49+
this.newSessionId = newSession.getId();
50+
}
51+
52+
@Override
53+
public String getOldSessionId() {
54+
return this.oldSessionId;
55+
}
56+
57+
@Override
58+
public String getNewSessionId() {
59+
return this.newSessionId;
60+
}
61+
62+
}

‎web/src/test/java/org/springframework/security/web/authentication/session/ChangeSessionIdAuthenticationStrategyTests.java‎

Lines changed: 30 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -16,11 +16,22 @@
1616

1717
package org.springframework.security.web.authentication.session;
1818

19+
import java.util.List;
20+
1921
import org.junit.jupiter.api.Test;
22+
import org.mockito.ArgumentCaptor;
2023

24+
import org.springframework.context.ApplicationEvent;
25+
import org.springframework.context.ApplicationEventPublisher;
2126
import org.springframework.mock.web.MockHttpServletRequest;
27+
import org.springframework.mock.web.MockHttpServletResponse;
28+
import org.springframework.security.core.Authentication;
29+
import org.springframework.security.core.session.SessionIdChangedEvent;
2230

2331
import static org.assertj.core.api.Assertions.assertThat;
32+
import static org.mockito.Mockito.mock;
33+
import static org.mockito.Mockito.times;
34+
import static org.mockito.Mockito.verify;
2435

2536
/**
2637
* @author Rob Winch
@@ -36,4 +47,23 @@ public void applySessionFixation() {
3647
assertThat(request.getSession().getId()).isNotEqualTo(id);
3748
}
3849

50+
@Test
51+
public void onAuthenticationPublishesSessionIdChangedEventWithoutHttpSessionEventPublisher() {
52+
ChangeSessionIdAuthenticationStrategy strategy = new ChangeSessionIdAuthenticationStrategy();
53+
MockHttpServletRequest request = new MockHttpServletRequest();
54+
String oldSessionId = request.getSession().getId();
55+
ApplicationEventPublisher eventPublisher = mock(ApplicationEventPublisher.class);
56+
strategy.setApplicationEventPublisher(eventPublisher);
57+
strategy.onAuthentication(mock(Authentication.class), request, new MockHttpServletResponse());
58+
ArgumentCaptor<ApplicationEvent> captor = ArgumentCaptor.forClass(ApplicationEvent.class);
59+
verify(eventPublisher, times(2)).publishEvent(captor.capture());
60+
List<ApplicationEvent> events = captor.getAllValues();
61+
assertThat(events.get(0)).isInstanceOf(SessionFixationProtectionEvent.class);
62+
assertThat(events.get(1)).isInstanceOf(SessionIdChangedEvent.class);
63+
SessionIdChangedEvent idChangedEvent = (SessionIdChangedEvent) events.get(1);
64+
assertThat(idChangedEvent.getOldSessionId()).isEqualTo(oldSessionId);
65+
assertThat(idChangedEvent.getNewSessionId()).isEqualTo(request.getSession().getId());
66+
assertThat(idChangedEvent.getNewSessionId()).isNotEqualTo(oldSessionId);
67+
}
68+
3969
}

‎web/src/test/java/org/springframework/security/web/session/DefaultSessionAuthenticationStrategyTests.java‎

Lines changed: 25 additions & 14 deletions
Original file line numberDiff line numberDiff line change
@@ -29,8 +29,11 @@
2929
import org.springframework.security.web.authentication.session.SessionFixationProtectionEvent;
3030
import org.springframework.security.web.authentication.session.SessionFixationProtectionStrategy;
3131

32+
import org.springframework.security.core.session.SessionIdChangedEvent;
33+
3234
import static org.assertj.core.api.Assertions.assertThat;
3335
import static org.mockito.Mockito.mock;
36+
import static org.mockito.Mockito.times;
3437
import static org.mockito.Mockito.verify;
3538

3639
/**
@@ -69,16 +72,20 @@ public void newSessionIsCreatedIfSessionAlreadyExistsWithEventPublisher() {
6972
Authentication mockAuthentication = mock(Authentication.class);
7073
strategy.onAuthentication(mockAuthentication, request, new MockHttpServletResponse());
7174
ArgumentCaptor<ApplicationEvent> eventArgumentCaptor = ArgumentCaptor.forClass(ApplicationEvent.class);
72-
verify(eventPublisher).publishEvent(eventArgumentCaptor.capture());
75+
verify(eventPublisher, times(2)).publishEvent(eventArgumentCaptor.capture());
7376
assertThat(oldSessionId.equals(request.getSession().getId())).isFalse();
7477
assertThat(request.getSession().getAttribute("blah")).isNotNull();
7578
assertThat(request.getSession().getAttribute("SPRING_SECURITY_SAVED_REQUEST_KEY")).isNotNull();
76-
assertThat(eventArgumentCaptor.getValue()).isNotNull();
77-
assertThat(eventArgumentCaptor.getValue() instanceof SessionFixationProtectionEvent).isTrue();
78-
SessionFixationProtectionEvent event = (SessionFixationProtectionEvent) eventArgumentCaptor.getValue();
79-
assertThat(event.getOldSessionId()).isEqualTo(oldSessionId);
80-
assertThat(event.getNewSessionId()).isEqualTo(request.getSession().getId());
81-
assertThat(event.getAuthentication()).isSameAs(mockAuthentication);
79+
assertThat(eventArgumentCaptor.getAllValues().get(0)).isInstanceOf(SessionFixationProtectionEvent.class);
80+
SessionFixationProtectionEvent fixationEvent = (SessionFixationProtectionEvent) eventArgumentCaptor.getAllValues()
81+
.get(0);
82+
assertThat(fixationEvent.getOldSessionId()).isEqualTo(oldSessionId);
83+
assertThat(fixationEvent.getNewSessionId()).isEqualTo(request.getSession().getId());
84+
assertThat(fixationEvent.getAuthentication()).isSameAs(mockAuthentication);
85+
assertThat(eventArgumentCaptor.getAllValues().get(1)).isInstanceOf(SessionIdChangedEvent.class);
86+
SessionIdChangedEvent idChangedEvent = (SessionIdChangedEvent) eventArgumentCaptor.getAllValues().get(1);
87+
assertThat(idChangedEvent.getOldSessionId()).isEqualTo(oldSessionId);
88+
assertThat(idChangedEvent.getNewSessionId()).isEqualTo(request.getSession().getId());
8289
}
8390

8491
// See SEC-1077
@@ -110,15 +117,19 @@ public void onlySavedRequestAttributeIsMigratedIfMigrateAttributesIsFalseWithEve
110117
Authentication mockAuthentication = mock(Authentication.class);
111118
strategy.onAuthentication(mockAuthentication, request, new MockHttpServletResponse());
112119
ArgumentCaptor<ApplicationEvent> eventArgumentCaptor = ArgumentCaptor.forClass(ApplicationEvent.class);
113-
verify(eventPublisher).publishEvent(eventArgumentCaptor.capture());
120+
verify(eventPublisher, times(2)).publishEvent(eventArgumentCaptor.capture());
114121
assertThat(request.getSession().getAttribute("blah")).isNull();
115122
assertThat(request.getSession().getAttribute("SPRING_SECURITY_SAVED_REQUEST_KEY")).isNotNull();
116-
assertThat(eventArgumentCaptor.getValue()).isNotNull();
117-
assertThat(eventArgumentCaptor.getValue() instanceof SessionFixationProtectionEvent).isTrue();
118-
SessionFixationProtectionEvent event = (SessionFixationProtectionEvent) eventArgumentCaptor.getValue();
119-
assertThat(event.getOldSessionId()).isEqualTo(oldSessionId);
120-
assertThat(event.getNewSessionId()).isEqualTo(request.getSession().getId());
121-
assertThat(event.getAuthentication()).isSameAs(mockAuthentication);
123+
assertThat(eventArgumentCaptor.getAllValues().get(0)).isInstanceOf(SessionFixationProtectionEvent.class);
124+
SessionFixationProtectionEvent fixationEvent = (SessionFixationProtectionEvent) eventArgumentCaptor.getAllValues()
125+
.get(0);
126+
assertThat(fixationEvent.getOldSessionId()).isEqualTo(oldSessionId);
127+
assertThat(fixationEvent.getNewSessionId()).isEqualTo(request.getSession().getId());
128+
assertThat(fixationEvent.getAuthentication()).isSameAs(mockAuthentication);
129+
assertThat(eventArgumentCaptor.getAllValues().get(1)).isInstanceOf(SessionIdChangedEvent.class);
130+
SessionIdChangedEvent idChangedEvent = (SessionIdChangedEvent) eventArgumentCaptor.getAllValues().get(1);
131+
assertThat(idChangedEvent.getOldSessionId()).isEqualTo(oldSessionId);
132+
assertThat(idChangedEvent.getNewSessionId()).isEqualTo(request.getSession().getId());
122133
}
123134

124135
@Test

0 commit comments

Comments
 (0)