From 90570c59a52bac35b9db33ad93bdd5c178c256ed Mon Sep 17 00:00:00 2001 From: nfawcett Date: Fri, 1 Aug 2025 15:00:50 +0100 Subject: [PATCH 1/3] patch infinite session expiration bug Signed-off-by: nfawcett --- .../redis/RedisIndexedSessionRepository.java | 16 +++++++++++----- 1 file changed, 11 insertions(+), 5 deletions(-) diff --git a/spring-session-data-redis/src/main/java/org/springframework/session/data/redis/RedisIndexedSessionRepository.java b/spring-session-data-redis/src/main/java/org/springframework/session/data/redis/RedisIndexedSessionRepository.java index 891ff88f9..e62075de0 100644 --- a/spring-session-data-redis/src/main/java/org/springframework/session/data/redis/RedisIndexedSessionRepository.java +++ b/spring-session-data-redis/src/main/java/org/springframework/session/data/redis/RedisIndexedSessionRepository.java @@ -929,9 +929,12 @@ private void saveDelta() { createShadowKey(sessionExpireInSeconds); - long fiveMinutesAfterExpires = sessionExpireInSeconds + TimeUnit.MINUTES.toSeconds(5); - RedisIndexedSessionRepository.this.sessionRedisOperations.boundHashOps(getSessionKey(getId())) - .expire(fiveMinutesAfterExpires, TimeUnit.SECONDS); + if (sessionExpireInSeconds > 0) { + long fiveMinutesAfterExpires = sessionExpireInSeconds + TimeUnit.MINUTES.toSeconds(5); + RedisIndexedSessionRepository.this.sessionRedisOperations.boundHashOps(getSessionKey(getId())) + .expire(fiveMinutesAfterExpires, TimeUnit.SECONDS); + } + RedisIndexedSessionRepository.this.expirationStore.save(this); this.delta = new HashMap<>(this.delta.size()); @@ -1036,8 +1039,11 @@ public void save(RedisSession session) { String expirationsKey = getExpirationsKey(toExpire); long sessionExpireInSeconds = session.getMaxInactiveInterval().getSeconds(); - long fiveMinutesAfterExpires = sessionExpireInSeconds + TimeUnit.MINUTES.toSeconds(5); - this.redis.boundSetOps(expirationsKey).expire(fiveMinutesAfterExpires, TimeUnit.SECONDS); + + if (sessionExpireInSeconds > 0) { + long fiveMinutesAfterExpires = sessionExpireInSeconds + TimeUnit.MINUTES.toSeconds(5); + this.redis.boundSetOps(expirationsKey).expire(fiveMinutesAfterExpires, TimeUnit.SECONDS); + } String expireKey = getExpirationKey(toExpire); BoundSetOperations expireOperations = this.redis.boundSetOps(expireKey); From 68a3ab0028b2a04d3692cfdec102f70c9f52686a Mon Sep 17 00:00:00 2001 From: nfawcett Date: Fri, 1 Aug 2025 15:44:29 +0100 Subject: [PATCH 2/3] test coverage Signed-off-by: nfawcett --- .../RedisIndexedSessionRepositoryITests.java | 17 +++++++++++++++++ .../RedisIndexedSessionRepositoryTests.java | 16 ++++++++++++++++ 2 files changed, 33 insertions(+) diff --git a/spring-session-data-redis/src/integration-test/java/org/springframework/session/data/redis/RedisIndexedSessionRepositoryITests.java b/spring-session-data-redis/src/integration-test/java/org/springframework/session/data/redis/RedisIndexedSessionRepositoryITests.java index 89188f6e5..8048050e6 100644 --- a/spring-session-data-redis/src/integration-test/java/org/springframework/session/data/redis/RedisIndexedSessionRepositoryITests.java +++ b/spring-session-data-redis/src/integration-test/java/org/springframework/session/data/redis/RedisIndexedSessionRepositoryITests.java @@ -17,6 +17,7 @@ package org.springframework.session.data.redis; import java.nio.charset.StandardCharsets; +import java.time.Duration; import java.util.Map; import java.util.UUID; import java.util.concurrent.TimeUnit; @@ -151,6 +152,22 @@ void saveThenSaveSessionKeyAndShadowKeyWith5MinutesDifference() { assertThat(differenceInSeconds).isEqualTo(300); } + + @Test + void saveNonExpiringThenSaveSessionKeyAndShadowKeyWithNoDifference() { + RedisSession toSave = this.repository.createSession(); + String expectedAttributeName = "a"; + String expectedAttributeValue = "b"; + toSave.setAttribute(expectedAttributeName, expectedAttributeValue); + toSave.setMaxInactiveInterval(Duration.ofMillis(-1)); + + this.repository.save(toSave); + + Long sessionKeyExpire = this.redis.getExpire("RedisIndexedSessionRepositoryITests:sessions:" + toSave.getId(), + TimeUnit.SECONDS); + assertThat(sessionKeyExpire).isEqualTo(-1); + } + @Test void putAllOnSingleAttrDoesNotRemoveOld() { RedisSession toSave = this.repository.createSession(); diff --git a/spring-session-data-redis/src/test/java/org/springframework/session/data/redis/RedisIndexedSessionRepositoryTests.java b/spring-session-data-redis/src/test/java/org/springframework/session/data/redis/RedisIndexedSessionRepositoryTests.java index 1ac304e65..992a3ff65 100644 --- a/spring-session-data-redis/src/test/java/org/springframework/session/data/redis/RedisIndexedSessionRepositoryTests.java +++ b/spring-session-data-redis/src/test/java/org/springframework/session/data/redis/RedisIndexedSessionRepositoryTests.java @@ -271,6 +271,22 @@ void saveSetAttribute() { map(RedisIndexedSessionRepository.getSessionAttrNameKey(attrName), session.getAttribute(attrName))); } + @Test + void saveNonExpiring() { + RedisSession session = this.redisRepository.new RedisSession(new MapSession(), false); + session.setMaxInactiveInterval(Duration.ofSeconds(-1)); + + given(this.redisOperations.boundHashOps(anyString())).willReturn(this.boundHashOperations); + given(this.redisOperations.boundSetOps(anyString())).willReturn(this.boundSetOperations); + given(this.redisOperations.boundValueOps(anyString())).willReturn(this.boundValueOperations); + + this.redisRepository.save(session); + + verify(this.boundHashOperations, never()).expire(-1, TimeUnit.SECONDS); + assertThat(getDelta()) + .isEqualTo(map(RedisSessionMapper.MAX_INACTIVE_INTERVAL_KEY, -1)); + } + @Test void saveRemoveAttribute() { String attrName = "attrName"; From 23bbf60b2c83d1efe806fa724fb10a561fe8c9c6 Mon Sep 17 00:00:00 2001 From: nfawcett Date: Fri, 1 Aug 2025 16:01:32 +0100 Subject: [PATCH 3/3] format Signed-off-by: nfawcett --- .../data/redis/RedisIndexedSessionRepositoryITests.java | 1 - .../session/data/redis/RedisIndexedSessionRepository.java | 1 - .../session/data/redis/RedisIndexedSessionRepositoryTests.java | 3 +-- 3 files changed, 1 insertion(+), 4 deletions(-) diff --git a/spring-session-data-redis/src/integration-test/java/org/springframework/session/data/redis/RedisIndexedSessionRepositoryITests.java b/spring-session-data-redis/src/integration-test/java/org/springframework/session/data/redis/RedisIndexedSessionRepositoryITests.java index 8048050e6..899aa4b2b 100644 --- a/spring-session-data-redis/src/integration-test/java/org/springframework/session/data/redis/RedisIndexedSessionRepositoryITests.java +++ b/spring-session-data-redis/src/integration-test/java/org/springframework/session/data/redis/RedisIndexedSessionRepositoryITests.java @@ -152,7 +152,6 @@ void saveThenSaveSessionKeyAndShadowKeyWith5MinutesDifference() { assertThat(differenceInSeconds).isEqualTo(300); } - @Test void saveNonExpiringThenSaveSessionKeyAndShadowKeyWithNoDifference() { RedisSession toSave = this.repository.createSession(); diff --git a/spring-session-data-redis/src/main/java/org/springframework/session/data/redis/RedisIndexedSessionRepository.java b/spring-session-data-redis/src/main/java/org/springframework/session/data/redis/RedisIndexedSessionRepository.java index e62075de0..de2b6c685 100644 --- a/spring-session-data-redis/src/main/java/org/springframework/session/data/redis/RedisIndexedSessionRepository.java +++ b/spring-session-data-redis/src/main/java/org/springframework/session/data/redis/RedisIndexedSessionRepository.java @@ -935,7 +935,6 @@ private void saveDelta() { .expire(fiveMinutesAfterExpires, TimeUnit.SECONDS); } - RedisIndexedSessionRepository.this.expirationStore.save(this); this.delta = new HashMap<>(this.delta.size()); } diff --git a/spring-session-data-redis/src/test/java/org/springframework/session/data/redis/RedisIndexedSessionRepositoryTests.java b/spring-session-data-redis/src/test/java/org/springframework/session/data/redis/RedisIndexedSessionRepositoryTests.java index 992a3ff65..d26fe7668 100644 --- a/spring-session-data-redis/src/test/java/org/springframework/session/data/redis/RedisIndexedSessionRepositoryTests.java +++ b/spring-session-data-redis/src/test/java/org/springframework/session/data/redis/RedisIndexedSessionRepositoryTests.java @@ -283,8 +283,7 @@ void saveNonExpiring() { this.redisRepository.save(session); verify(this.boundHashOperations, never()).expire(-1, TimeUnit.SECONDS); - assertThat(getDelta()) - .isEqualTo(map(RedisSessionMapper.MAX_INACTIVE_INTERVAL_KEY, -1)); + assertThat(getDelta()).isEqualTo(map(RedisSessionMapper.MAX_INACTIVE_INTERVAL_KEY, -1)); } @Test