From 2e43e37d054c25f342d8a7874a9a40b94a4a4181 Mon Sep 17 00:00:00 2001 From: Thomas Nymand Date: Thu, 18 Jun 2026 16:10:05 +0200 Subject: [PATCH] REF-8: Fix seconds/milliseconds mixup in in-memory session cleanup (issue #76) InMemorySessionHandler.cleanup received the timeout in seconds (per the SessionHandler#cleanup contract and the DatabaseSessionHandler) but passed it straight to TimeOutWrapper.isExpired(), which compares against System.currentTimeMillis() and therefore expects milliseconds. As a result sessions on the in-memory handler expired 1000x too early - a 30 minute timeout became 1.8 seconds - logging users out on almost every cleanup run. The database session handler was unaffected. Convert seconds to milliseconds before the expiry comparison, and fix the SessionCleanerTask debug log that printed the timeout inconsistently. Correct the misleading "Milliseconds" javadoc on the cleanup contract (SessionHandler) and both implementations - the value is seconds. Add an InMemorySessionHandlerTest regression test asserting a stored assertion survives until its real (seconds) timeout. Ports the fix from the Trifork fork (Jeppe Sommer, commits 6485c2b and bc7b004) and adds the previously missing test. Co-Authored-By: Claude Opus 4.8 (1M context) --- .../oio/saml/session/SessionCleanerTask.java | 2 +- .../dk/gov/oio/saml/session/SessionHandler.java | 2 +- .../database/DatabaseSessionHandler.java | 2 +- .../inmemory/InMemorySessionHandler.java | 15 ++++++++++----- .../inmemory/InMemorySessionHandlerTest.java | 17 +++++++++++++++++ 5 files changed, 30 insertions(+), 8 deletions(-) diff --git a/oiosaml/src/main/java/dk/gov/oio/saml/session/SessionCleanerTask.java b/oiosaml/src/main/java/dk/gov/oio/saml/session/SessionCleanerTask.java index 29b498f..ddad266 100644 --- a/oiosaml/src/main/java/dk/gov/oio/saml/session/SessionCleanerTask.java +++ b/oiosaml/src/main/java/dk/gov/oio/saml/session/SessionCleanerTask.java @@ -20,7 +20,7 @@ public SessionCleanerTask(long maxInactiveIntervalSeconds) { @Override public void run() { - log.debug("Cleaning session data, time: {}, timeout: {}", System.currentTimeMillis(), maxInactiveIntervalSeconds * 1000); + log.debug("Cleaning session data, time: {}, timeout (seconds): {}", System.currentTimeMillis(), maxInactiveIntervalSeconds); try { SessionHandler sessionHandler = OIOSAML3Service.getSessionHandlerFactory().getHandler(); sessionHandler.cleanup(maxInactiveIntervalSeconds); diff --git a/oiosaml/src/main/java/dk/gov/oio/saml/session/SessionHandler.java b/oiosaml/src/main/java/dk/gov/oio/saml/session/SessionHandler.java index 4c0b627..3c80cb4 100644 --- a/oiosaml/src/main/java/dk/gov/oio/saml/session/SessionHandler.java +++ b/oiosaml/src/main/java/dk/gov/oio/saml/session/SessionHandler.java @@ -134,7 +134,7 @@ default String getSessionId(HttpSession session) { /** * Clean stored ids and sessions. * - * @param maxInactiveIntervalSeconds Milliseconds to store session, before it should be invalidated. + * @param maxInactiveIntervalSeconds Seconds to store session, before it should be invalidated. */ void cleanup(long maxInactiveIntervalSeconds); } diff --git a/oiosaml/src/main/java/dk/gov/oio/saml/session/database/DatabaseSessionHandler.java b/oiosaml/src/main/java/dk/gov/oio/saml/session/database/DatabaseSessionHandler.java index 6c0390b..1a0b98c 100644 --- a/oiosaml/src/main/java/dk/gov/oio/saml/session/database/DatabaseSessionHandler.java +++ b/oiosaml/src/main/java/dk/gov/oio/saml/session/database/DatabaseSessionHandler.java @@ -351,7 +351,7 @@ public void logout(HttpSession session, AssertionWrapper assertion) { /** * Clean stored ids and sessions. * - * @param maxInactiveIntervalSeconds Milliseconds to store session, before it should be invalidated. + * @param maxInactiveIntervalSeconds Seconds to store session, before it should be invalidated. */ @Override public void cleanup(final long maxInactiveIntervalSeconds) { diff --git a/oiosaml/src/main/java/dk/gov/oio/saml/session/inmemory/InMemorySessionHandler.java b/oiosaml/src/main/java/dk/gov/oio/saml/session/inmemory/InMemorySessionHandler.java index 53b5773..6a1af6b 100644 --- a/oiosaml/src/main/java/dk/gov/oio/saml/session/inmemory/InMemorySessionHandler.java +++ b/oiosaml/src/main/java/dk/gov/oio/saml/session/inmemory/InMemorySessionHandler.java @@ -252,18 +252,23 @@ public void logout(HttpSession session, AssertionWrapper assertion) { /** * Clean stored ids and sessions. * - * @param maxInactiveIntervalSeconds Milliseconds to store session, before it should be invalidated. + * @param maxInactiveIntervalSeconds Seconds to store session, before it should be invalidated. */ @Override public void cleanup(long maxInactiveIntervalSeconds) { + // TimeOutWrapper.isExpired() compares against System.currentTimeMillis(), so the timeout + // must be in milliseconds. The interval is supplied in seconds (see SessionHandler#cleanup), + // so convert before comparing - otherwise sessions expire 1000x too early (issue #76). + long maxInactiveIntervalMillis = maxInactiveIntervalSeconds * 1000L; + // Trim usedAssertionIds to size with sessionHandlerNumTrackedSessionIds while (!usedAssertionIds.isEmpty() && usedAssertionIds.size() > sessionHandlerNumTrackedSessionIds) { usedAssertionIds.remove(usedAssertionIds.pollFirst()); } - cleanup(sessionIndexMap, maxInactiveIntervalSeconds, "SessionIndexMap"); - cleanup(assertions, maxInactiveIntervalSeconds, "Assertions"); - cleanup(authnRequests, maxInactiveIntervalSeconds, "AuthnRequests"); - cleanup(logoutRequests, maxInactiveIntervalSeconds, "LogoutRequests"); + cleanup(sessionIndexMap, maxInactiveIntervalMillis, "SessionIndexMap"); + cleanup(assertions, maxInactiveIntervalMillis, "Assertions"); + cleanup(authnRequests, maxInactiveIntervalMillis, "AuthnRequests"); + cleanup(logoutRequests, maxInactiveIntervalMillis, "LogoutRequests"); } private void cleanup(Map> map, long cleanupDelay, String msg) { diff --git a/oiosaml/src/test/java/dk/gov/oio/saml/session/inmemory/InMemorySessionHandlerTest.java b/oiosaml/src/test/java/dk/gov/oio/saml/session/inmemory/InMemorySessionHandlerTest.java index 3f50ec7..9547e3a 100644 --- a/oiosaml/src/test/java/dk/gov/oio/saml/session/inmemory/InMemorySessionHandlerTest.java +++ b/oiosaml/src/test/java/dk/gov/oio/saml/session/inmemory/InMemorySessionHandlerTest.java @@ -152,6 +152,23 @@ void testStoreAssertionTimeout() throws Exception { Assertions.assertNull(assertionWrapperOutput); } + @DisplayName("Test that stored Assertion survives until its timeout (seconds, not millis) - issue #76") + @Test + void testStoreAssertionNotRemovedBeforeTimeout() throws Exception { + AssertionWrapper assertionWrapperInput = new AssertionWrapper(createAssertion()); + + sessionHandler.storeAssertion(session, assertionWrapperInput); + + // Wait far longer than the timeout-interpreted-as-milliseconds (the #76 bug: 5s -> 5ms), + // but far less than the real 5 second timeout. The assertion must still be present. + Thread.sleep(100); + sessionHandler.cleanup(5); // 5 seconds + + AssertionWrapper assertionWrapperOutput = sessionHandler.getAssertion(session); + Assertions.assertNotNull(assertionWrapperOutput, + "Assertion must survive cleanup until its timeout elapses (timeout is in seconds, not milliseconds)"); + } + @DisplayName("Test that stored Assertion is removed after timeout and can not be retrieved using session index") @Test void testStoreAssertionTimeoutSessionIndex() throws Exception {