-
Notifications
You must be signed in to change notification settings - Fork 628
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
ISPN-10224 Fix session fixation protection #6960
Conversation
Can one of the admins verify this patch? |
1 similar comment
Can one of the admins verify this patch? |
@thelateperseus I'm curious about what part of the maven build failed for you. |
@tristantarrant I had a lot of problems with our corporate proxy. Once I got past them, I got a failure on the Infinispan Core tests (I didn't specify |
Unfortunately there are 4-5 unstable tests (within over 25K tests). We are addressing these. The ideal way to test your changes is to do a full build with -DskipTests and then do a |
Thanks. I'll give that a try when I'm back in the office tomorrow. |
@tristantarrant I got an error about not being able to download a JAR when I ran
However, |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I opened a JIRA ticket to use it in the message.
I'm not sure about using deleteById method in the case of a sessionId changed (instead of deleting from infinispan) I need to check this with spring session people
if (valueWrapper == null) { | ||
return; | ||
} | ||
MapSession mapSession = (MapSession) valueWrapper.get(); | ||
if (mapSession != null) { | ||
applicationEventPublisher.emitSessionDeletedEvent(mapSession); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure we need to send a session deleted event when the id has changed. I need to double check this behavior with spring session people.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Grabbing a look to other code I know that we should not call deleteById but remove the entry from the repository using Infinispan remove and skip notifications. I will add a commit with the correction soon
if (valueWrapper == null) { | ||
return; | ||
} | ||
MapSession mapSession = (MapSession) valueWrapper.get(); | ||
if (mapSession != null) { | ||
applicationEventPublisher.emitSessionDeletedEvent(mapSession); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Grabbing a look to other code I know that we should not call deleteById but remove the entry from the repository using Infinispan remove and skip notifications. I will add a commit with the correction soon
closing in favor of #7025 |
org.infinispan.spring.common.session.AbstractInfinispanSessionRepository
implements thesave(MapSession)
method defined byorg.springframework.session.SessionRepository
. However, the implementation does not correctly handle when the session id changes. WhengetId()
returns a different value fromgetOriginalId()
, the original session should be deleted.Note that Spring Session's
MapSessionRepository.save
implementation has the correct behaviour.I couldn't get the Maven build working, so I can't guarantee it compiles.
https://issues.jboss.org/browse/ISPN-10245
This issue was discovered by @the-cartographer has been assigned CVE-2019-10158.