Fix SessionRegistryImpl losing track of session when ChangeSessionIdAuthenticationStrategy runs without HttpSessionEventPublisher#19214
Open
klouds27 wants to merge 2 commits into
Conversation
There was a problem hiding this comment.
Pull request overview
Note
Copilot was unable to run its full agentic suite in this review.
Adds publication of a SessionIdChangedEvent during session fixation protection so SessionRegistryImpl can track session ID migrations without requiring HttpSessionEventPublisher.
Changes:
- Publish an additional
SessionIdChangedEventalongsideSessionFixationProtectionEventduring session fixation protection. - Add a dedicated internal event type to carry old/new session IDs from the fixation strategy.
- Update/add tests and javadocs to reflect the new event behavior and
SessionRegistryImplsupport.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| web/src/main/java/org/springframework/security/web/authentication/session/AbstractSessionFixationProtectionStrategy.java | Publishes an additional session-id-changed event on session change. |
| web/src/main/java/org/springframework/security/web/authentication/session/SessionFixationProtectionSessionIdChangedEvent.java | New internal SessionIdChangedEvent subclass carrying stable old/new IDs. |
| web/src/test/java/org/springframework/security/web/session/DefaultSessionAuthenticationStrategyTests.java | Updates assertions to expect two published events and validate both. |
| web/src/test/java/org/springframework/security/web/authentication/session/ChangeSessionIdAuthenticationStrategyTests.java | Adds coverage for publishing SessionIdChangedEvent when an app event publisher is set. |
| core/src/main/java/org/springframework/security/core/session/SessionRegistryImpl.java | Updates class javadoc to mention SessionIdChangedEvent support and fixation tracking. |
| core/src/test/java/org/springframework/security/core/session/SessionRegistryImplTests.java | Adds test ensuring SessionIdChangedEvent migrates the registered session. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Comment on lines
131
to
136
| protected void onSessionChange(String originalSessionId, HttpSession newSession, Authentication auth) { | ||
| this.applicationEventPublisher | ||
| .publishEvent(new SessionFixationProtectionEvent(auth, originalSessionId, newSession.getId())); | ||
| this.applicationEventPublisher | ||
| .publishEvent(new SessionFixationProtectionSessionIdChangedEvent(newSession, originalSessionId)); | ||
| } |
Comment on lines
+61
to
+63
| assertThat(events.get(0)).isInstanceOf(SessionFixationProtectionEvent.class); | ||
| assertThat(events.get(1)).isInstanceOf(SessionIdChangedEvent.class); | ||
| SessionIdChangedEvent idChangedEvent = (SessionIdChangedEvent) events.get(1); |
| } | ||
|
|
||
| @Test | ||
| public void onAuthenticationPublishesSessionIdChangedEventWithoutHttpSessionEventPublisher() { |
spring-projects-issues
added
the
status: waiting-for-triage
klouds27
force-pushed
the
fix/session-registry-fixation-without-event-publisher
branch
from
862267d to
274d5d3
Compare
…essionEventPublisher 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 spring-projectsgh-19007 Signed-off-by: admin <adalwolfad@gmail.com> Signed-off-by: klouds27 <adalwolf@gmail.com>
Signed-off-by: admin <adalwolfad@gmail.com> Signed-off-by: klouds27 <adalwolf@gmail.com>
klouds27
force-pushed
the
fix/session-registry-fixation-without-event-publisher
branch
2 times, most recently
from
830fed3 to
9545081
Compare
This file contains hidden or 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
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.
Problem
When concurrency control is enabled and
ChangeSessionIdAuthenticationStrategyrotates the session ID during authentication,SessionRegistryImplonly detects the change ifHttpSessionEventPublisheris registered as a servlet listener. Without it,HttpSessionIdChangedEventis never published, so the registry retains the old session ID as a ghost entry.On the next authentication,
ConcurrentSessionControlAuthenticationStrategycounts one extra session per login and incorrectly expires the valid session, breaking concurrency control entirely.Root cause
AbstractSessionFixationProtectionStrategy.onSessionChange()publishesSessionFixationProtectionEvent, which extendsAbstractAuthenticationEvent.SessionRegistryImplonly listens forAbstractSessionEvent, so it never sees the ID change.SessionRegistryImplalready handlesSessionIdChangedEvent(anAbstractSessionEventsubtype) correctly — it removes the old entry and re-registers under the new ID — but that event is only published byHttpSessionEventPublishervia the servlet container'sHttpSessionIdListenercallback.Fix
AbstractSessionFixationProtectionStrategy.onSessionChange()now also publishes aSessionFixationProtectionSessionIdChangedEvent(a new package-private class extendingSessionIdChangedEvent).SessionRegistryImplhandlesSessionIdChangedEventinstances already, so it correctly migrates the entry on every session fixation rotation without any additional configuration.HttpSessionEventPublisheris still required to receive session-destroyed notifications for expired sessions.Tests
ChangeSessionIdAuthenticationStrategyTests#onAuthenticationPublishesSessionIdChangedEventWithoutHttpSessionEventPublisher— verifies bothSessionFixationProtectionEventandSessionIdChangedEventare publishedSessionRegistryImplTests#sessionIdChangedEventWhenOldSessionRegisteredThenMigratesSessionWithoutHttpSessionEventPublisher— verifies the registry migrates correctly and the principal ends up with exactly one sessionDefaultSessionAuthenticationStrategyTeststo assert both published eventsFixes gh-19007