From 29c8c637aeb5bc81a266a46308b2a612aaf9c48f Mon Sep 17 00:00:00 2001 From: "Matteo E. Minnai" Date: Fri, 16 Oct 2026 11:25:03 +0200 Subject: [PATCH 1/5] ESB-1294 Enforce OIDC state parameter --- .../keycloak/filter/KeycloakFilter.java | 63 +++++- .../keycloak/filter/KeycloakFilterTest.java | 206 ++++++++++++++++++ 2 files changed, 262 insertions(+), 7 deletions(-) diff --git a/keycloak-plugin/src/main/java/org/entando/entando/keycloak/filter/KeycloakFilter.java b/keycloak-plugin/src/main/java/org/entando/entando/keycloak/filter/KeycloakFilter.java index b1696174e..10c3b1727 100644 --- a/keycloak-plugin/src/main/java/org/entando/entando/keycloak/filter/KeycloakFilter.java +++ b/keycloak-plugin/src/main/java/org/entando/entando/keycloak/filter/KeycloakFilter.java @@ -25,6 +25,10 @@ import com.agiletec.aps.util.ApsTenantApplicationUtils; import com.fasterxml.jackson.databind.ObjectMapper; import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.security.MessageDigest; +import java.util.LinkedHashMap; +import java.util.Map; import java.util.Optional; import java.util.UUID; import jakarta.servlet.Filter; @@ -60,7 +64,11 @@ public class KeycloakFilter implements Filter { private static final EntLogger log = EntLogFactory.getSanitizedLogger(KeycloakFilter.class); + private static final int MAX_PENDING_STATES = 5; + private static final long STATE_TTL_MILLIS = 10 * 60 * 1000L; + public static final String SESSION_PARAM_STATE = "keycloak-plugin-state"; + public static final String SESSION_PARAM_STATES = "keycloak-plugin-states"; public static final String SESSION_PARAM_REDIRECT = "keycloak-plugin-redirectTo"; public static final String SESSION_PARAM_ACCESS_TOKEN = "keycloak-plugin-access-token"; public static final String SESSION_PARAM_ID_TOKEN = "keycloak-plugin-id-token"; @@ -238,7 +246,7 @@ private void doLogin(final HttpServletRequest request, final HttpServletResponse checkNoErrorOrThrow(request, error, errorDescription); if (authorizationCode != null) { - checkAndLogStateParameter(stateParameter, session); + consumeStateOrThrow(request, stateParameter, session); try { final ResponseEntity responseEntity = oidcService.requestToken(authorizationCode, redirectUri); @@ -295,7 +303,7 @@ private void doLogin(final HttpServletRequest request, final HttpServletResponse final String state = UUID.randomUUID().toString(); final String redirectUrl = oidcService.getRedirectUrl(redirectUri, state, fetchSuggestedIdp(request)); - session.setAttribute(SESSION_PARAM_STATE, state); + addPendingState(session, state); log.debug("doLogin sendRedirect redirectUrl:'{}'", redirectUrl); response.sendRedirect(redirectUrl); } @@ -319,13 +327,54 @@ private void checkNoErrorOrThrow(HttpServletRequest request, String error, Strin } - private void checkAndLogStateParameter(String stateParameter, HttpSession session){ - if (stateParameter == null) { - log.warn("State parameter not provided"); - } else if (!stateParameter.equals(session.getAttribute(SESSION_PARAM_STATE))) { - log.warn("State parameter '{}' is different than generated '{}'", stateParameter, session.getAttribute(SESSION_PARAM_STATE)); + private void addPendingState(HttpSession session, String state) { + LinkedHashMap states = readPendingStates(session); + states.put(state, System.currentTimeMillis()); + while (states.size() > MAX_PENDING_STATES) { + states.remove(states.keySet().iterator().next()); + } + session.setAttribute(SESSION_PARAM_STATES, states); + session.removeAttribute(SESSION_PARAM_STATE); + } + + private void consumeStateOrThrow(HttpServletRequest request, String stateParameter, HttpSession session) { + LinkedHashMap states = readPendingStates(session); + String matched = null; + if (stateParameter != null) { + byte[] received = stateParameter.getBytes(StandardCharsets.UTF_8); + for (String pending : states.keySet()) { + if (MessageDigest.isEqual(received, pending.getBytes(StandardCharsets.UTF_8))) { + matched = pending; + } + } + } + Long issued = (matched != null) ? states.remove(matched) : null; + session.setAttribute(SESSION_PARAM_STATES, states); + session.removeAttribute(SESSION_PARAM_STATE); + String reason = (stateParameter == null) ? "missing" + : (matched == null) ? "unknown" + : (issued != null && System.currentTimeMillis() - issued > STATE_TTL_MILLIS) ? "expired" + : null; + if (reason != null) { + log.warn("Login callback rejected: state {}", reason); + throw new EntandoTokenException("invalid state", request, SystemConstants.GUEST_USER_NAME); } + } + @SuppressWarnings("unchecked") + private LinkedHashMap readPendingStates(HttpSession session) { + LinkedHashMap states = new LinkedHashMap<>(); + Object current = session.getAttribute(SESSION_PARAM_STATES); + if (current instanceof Map) { + states.putAll((Map) current); + } + Object legacy = session.getAttribute(SESSION_PARAM_STATE); + if (legacy instanceof String) { + states.putIfAbsent((String) legacy, null); + } + long now = System.currentTimeMillis(); + states.values().removeIf(issued -> issued != null && now - issued > STATE_TTL_MILLIS); + return states; } private void buildAndSaveInSessionRedirectPath(String redirectTo, HttpServletRequest request, HttpSession session){ diff --git a/keycloak-plugin/src/test/java/org/entando/entando/keycloak/filter/KeycloakFilterTest.java b/keycloak-plugin/src/test/java/org/entando/entando/keycloak/filter/KeycloakFilterTest.java index a540620f7..8d0ca9df4 100644 --- a/keycloak-plugin/src/test/java/org/entando/entando/keycloak/filter/KeycloakFilterTest.java +++ b/keycloak-plugin/src/test/java/org/entando/entando/keycloak/filter/KeycloakFilterTest.java @@ -6,6 +6,7 @@ import static org.mockito.ArgumentMatchers.eq; import static org.mockito.ArgumentMatchers.isNull; import static org.mockito.ArgumentMatchers.same; +import static org.mockito.Mockito.doAnswer; import static org.mockito.Mockito.never; import static org.mockito.Mockito.reset; import static org.mockito.Mockito.times; @@ -28,6 +29,8 @@ import jakarta.servlet.http.HttpSession; import java.io.IOException; import java.io.StringWriter; +import java.util.LinkedHashMap; +import java.util.Map; import org.assertj.core.api.AbstractBooleanAssert; import org.assertj.core.api.AbstractCharSequenceAssert; import org.entando.entando.aps.system.services.tenants.ITenantManager; @@ -43,6 +46,7 @@ import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.ArgumentCaptor; import org.mockito.Mock; import org.mockito.MockedStatic; import org.mockito.Mockito; @@ -433,6 +437,7 @@ void testLoginWithAuthorizationCode() throws Exception { when(request.getServletPath()).thenReturn(path); when(request.getParameter("state")).thenReturn(""); when(request.getParameter("code")).thenReturn(""); + Mockito.lenient().when(session.getAttribute(KeycloakFilter.SESSION_PARAM_STATES)).thenReturn(pendingStates("")); AuthResponse authResponse = new AuthResponse(); authResponse.setAccessToken(""); @@ -452,6 +457,178 @@ void testLoginWithAuthorizationCode() throws Exception { verify(session, times(1)).setAttribute(eq("user"), same(userDetails)); } + @Test + void stateMatchingShouldConsumeStateAndSaveUser() throws Exception { + when(configuration.isEnabled()).thenReturn(true); + when(request.getServletPath()).thenReturn("/do/login"); + when(request.getParameter("code")).thenReturn(""); + when(request.getParameter("state")).thenReturn("S"); + Mockito.lenient().when(session.getAttribute(KeycloakFilter.SESSION_PARAM_STATES)).thenReturn(pendingStates("S")); + stubSuccessfulTokenExchange("", ""); + + try (MockedStatic wacUtil = Mockito.mockStatic(WebApplicationContextUtils.class)) { + wacUtil.when(() -> WebApplicationContextUtils.getWebApplicationContext(svCtx)).thenReturn(wac); + keycloakFilter.doFilter(request, response, filterChain); + } + + verify(oidcService, times(1)).requestToken(eq(""), any()); + verify(session, times(1)).setAttribute(eq("user"), same(userDetails)); + + ArgumentCaptor captor = ArgumentCaptor.forClass(LinkedHashMap.class); + verify(session).setAttribute(eq(KeycloakFilter.SESSION_PARAM_STATES), captor.capture()); + assertThat((Map) captor.getValue()).doesNotContainKey("S"); + } + + @Test + void stateMismatchShouldThrowAndNotRequestToken() throws Exception { + when(configuration.isEnabled()).thenReturn(true); + when(request.getServletPath()).thenReturn("/do/login"); + when(request.getParameter("code")).thenReturn(""); + when(request.getParameter("state")).thenReturn("X"); + Mockito.lenient().when(session.getAttribute(KeycloakFilter.SESSION_PARAM_STATES)).thenReturn(pendingStates("S")); + + try (MockedStatic wacUtil = Mockito.mockStatic(WebApplicationContextUtils.class)) { + wacUtil.when(() -> WebApplicationContextUtils.getWebApplicationContext(svCtx)).thenReturn(wac); + Assertions.assertThrows(EntandoTokenException.class, + () -> keycloakFilter.doFilter(request, response, filterChain)); + } + + verify(oidcService, never()).requestToken(any(), any()); + verify(session, never()).setAttribute(eq("user"), any()); + } + + @Test + void stateMissingShouldThrowAndNotRequestToken() throws Exception { + when(configuration.isEnabled()).thenReturn(true); + when(request.getServletPath()).thenReturn("/do/login"); + when(request.getParameter("code")).thenReturn(""); + Mockito.lenient().when(session.getAttribute(KeycloakFilter.SESSION_PARAM_STATES)).thenReturn(pendingStates("S")); + + try (MockedStatic wacUtil = Mockito.mockStatic(WebApplicationContextUtils.class)) { + wacUtil.when(() -> WebApplicationContextUtils.getWebApplicationContext(svCtx)).thenReturn(wac); + Assertions.assertThrows(EntandoTokenException.class, + () -> keycloakFilter.doFilter(request, response, filterChain)); + } + + verify(oidcService, never()).requestToken(any(), any()); + verify(session, never()).setAttribute(eq("user"), any()); + } + + @Test + void noPendingStateShouldThrowAndNotRequestToken() throws Exception { + when(configuration.isEnabled()).thenReturn(true); + when(request.getServletPath()).thenReturn("/do/login"); + when(request.getParameter("code")).thenReturn(""); + when(request.getParameter("state")).thenReturn("S"); + + try (MockedStatic wacUtil = Mockito.mockStatic(WebApplicationContextUtils.class)) { + wacUtil.when(() -> WebApplicationContextUtils.getWebApplicationContext(svCtx)).thenReturn(wac); + Assertions.assertThrows(EntandoTokenException.class, + () -> keycloakFilter.doFilter(request, response, filterChain)); + } + + verify(oidcService, never()).requestToken(any(), any()); + verify(session, never()).setAttribute(eq("user"), any()); + } + + @Test + void replayOfSameCallbackShouldThrowSecondTime() throws Exception { + when(configuration.isEnabled()).thenReturn(true); + when(request.getServletPath()).thenReturn("/do/login"); + when(request.getParameter("code")).thenReturn(""); + when(request.getParameter("state")).thenReturn("S"); + stubStatefulPendingStates(pendingStates("S")); + stubSuccessfulTokenExchange("", ""); + + try (MockedStatic wacUtil = Mockito.mockStatic(WebApplicationContextUtils.class)) { + wacUtil.when(() -> WebApplicationContextUtils.getWebApplicationContext(svCtx)).thenReturn(wac); + keycloakFilter.doFilter(request, response, filterChain); + Assertions.assertThrows(EntandoTokenException.class, + () -> keycloakFilter.doFilter(request, response, filterChain)); + } + + verify(oidcService, times(1)).requestToken(eq(""), any()); + verify(session, times(1)).setAttribute(eq("user"), same(userDetails)); + } + + @Test + void twoTabsShouldBothLogin() throws Exception { + when(configuration.isEnabled()).thenReturn(true); + when(request.getServletPath()).thenReturn("/do/login"); + when(request.getParameter("code")).thenReturn(""); + when(request.getParameter("state")).thenReturn("S1", "S2"); + stubStatefulPendingStates(pendingStates("S1", "S2")); + stubSuccessfulTokenExchange("", ""); + + try (MockedStatic wacUtil = Mockito.mockStatic(WebApplicationContextUtils.class)) { + wacUtil.when(() -> WebApplicationContextUtils.getWebApplicationContext(svCtx)).thenReturn(wac); + keycloakFilter.doFilter(request, response, filterChain); + keycloakFilter.doFilter(request, response, filterChain); + } + + verify(oidcService, times(2)).requestToken(eq(""), any()); + verify(session, times(2)).setAttribute(eq("user"), same(userDetails)); + } + + @Test + void sixLoginStartsShouldKeepOnlyFivePendingStates() throws Exception { + when(configuration.isEnabled()).thenReturn(true); + when(request.getServletPath()).thenReturn("/do/login"); + when(oidcService.getRedirectUrl(any(), any(), any())).thenReturn("redirect"); + final Object[] holder = {null}; + Mockito.lenient().when(session.getAttribute(KeycloakFilter.SESSION_PARAM_STATES)).thenAnswer(i -> holder[0]); + Mockito.lenient().doAnswer(i -> { + holder[0] = i.getArgument(1); + return null; + }).when(session).setAttribute(eq(KeycloakFilter.SESSION_PARAM_STATES), any()); + + try (MockedStatic wacUtil = Mockito.mockStatic(WebApplicationContextUtils.class)) { + wacUtil.when(() -> WebApplicationContextUtils.getWebApplicationContext(svCtx)).thenReturn(wac); + for (int i = 0; i < 6; i++) { + keycloakFilter.doFilter(request, response, filterChain); + } + } + + assertThat((Map) holder[0]).hasSize(5); + } + + @Test + void legacyStateAttributeShouldLoginAndBeRemoved() throws Exception { + when(configuration.isEnabled()).thenReturn(true); + when(request.getServletPath()).thenReturn("/do/login"); + when(request.getParameter("code")).thenReturn(""); + when(request.getParameter("state")).thenReturn("S"); + Mockito.lenient().when(session.getAttribute(KeycloakFilter.SESSION_PARAM_STATE)).thenReturn("S"); + stubSuccessfulTokenExchange("", ""); + + try (MockedStatic wacUtil = Mockito.mockStatic(WebApplicationContextUtils.class)) { + wacUtil.when(() -> WebApplicationContextUtils.getWebApplicationContext(svCtx)).thenReturn(wac); + keycloakFilter.doFilter(request, response, filterChain); + } + + verify(session, times(1)).setAttribute(eq("user"), same(userDetails)); + verify(session, times(1)).removeAttribute(KeycloakFilter.SESSION_PARAM_STATE); + } + + @Test + void keycloakErrorCallbackShouldThrowBeforeStateCheck() throws Exception { + when(configuration.isEnabled()).thenReturn(true); + when(request.getServletPath()).thenReturn("/do/login"); + when(request.getParameter("code")).thenReturn(""); + Mockito.lenient().when(request.getParameter("state")).thenReturn("X"); + Mockito.lenient().when(request.getParameter("error")).thenReturn("access_denied"); + Mockito.lenient().when(request.getParameter("error_description")).thenReturn("User denied access"); + + try (MockedStatic wacUtil = Mockito.mockStatic(WebApplicationContextUtils.class)) { + wacUtil.when(() -> WebApplicationContextUtils.getWebApplicationContext(svCtx)).thenReturn(wac); + Assertions.assertThrows(EntandoTokenException.class, + () -> keycloakFilter.doFilter(request, response, filterChain)); + } + + verify(oidcService, never()).requestToken(any(), any()); + verify(session, never()).removeAttribute(KeycloakFilter.SESSION_PARAM_STATE); + } + @Test void testKeycloakJsonEndpoint() throws IOException, ServletException { final String path = "/keycloak.json"; @@ -663,6 +840,35 @@ void shouldLoginWithRedirectToSameDomainWithDifferentSchemaExecuteFine() throws } + private static LinkedHashMap pendingStates(String... states) { + LinkedHashMap map = new LinkedHashMap<>(); + for (String state : states) { + map.put(state, System.currentTimeMillis()); + } + return map; + } + + private void stubStatefulPendingStates(LinkedHashMap initial) { + final Object[] holder = {initial}; + Mockito.lenient().when(session.getAttribute(KeycloakFilter.SESSION_PARAM_STATES)).thenAnswer(i -> holder[0]); + Mockito.lenient().doAnswer(i -> { + holder[0] = i.getArgument(1); + return null; + }).when(session).setAttribute(eq(KeycloakFilter.SESSION_PARAM_STATES), any()); + } + + private void stubSuccessfulTokenExchange(String code, String username) throws EntException { + AuthResponse authResponse = new AuthResponse(); + authResponse.setAccessToken(""); + when(oidcService.requestToken(eq(code), any())).thenReturn(ResponseEntity.ok().body(authResponse)); + + AccessToken token = new AccessToken(); + token.setActive(true); + token.setUsername(username); + when(oidcService.validateToken("")).thenReturn(ResponseEntity.ok().body(token)); + when(providerManager.getUser(username)).thenReturn(userDetails); + } + static class ServletOutputStreamWrapper extends ServletOutputStream { private final StringWriter writer; private ServletOutputStreamWrapper(final StringWriter aWriter) { From db362e1ea1d34a7a97efeb5ac1cbab376a702dfd Mon Sep 17 00:00:00 2001 From: "Matteo E. Minnai" Date: Fri, 16 Oct 2026 11:39:52 +0200 Subject: [PATCH 2/5] ESB-1294 One-liner logline when token exception occurs --- .../keycloak/filter/KeycloakFilter.java | 8 +++- .../keycloak/filter/KeycloakFilterTest.java | 37 +++++++++---------- 2 files changed, 24 insertions(+), 21 deletions(-) diff --git a/keycloak-plugin/src/main/java/org/entando/entando/keycloak/filter/KeycloakFilter.java b/keycloak-plugin/src/main/java/org/entando/entando/keycloak/filter/KeycloakFilter.java index 10c3b1727..f011e35bd 100644 --- a/keycloak-plugin/src/main/java/org/entando/entando/keycloak/filter/KeycloakFilter.java +++ b/keycloak-plugin/src/main/java/org/entando/entando/keycloak/filter/KeycloakFilter.java @@ -141,6 +141,11 @@ public void doFilter(final ServletRequest servletRequest, chain.doFilter(request, response); } + } catch (EntandoTokenException e) { + // Report the rejected authentication as a single log line instead of letting the + // exception propagate to the container, which would log a full stack trace. + log.warn("Login rejected: {}", e.getMessage()); + ((HttpServletResponse) servletResponse).sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); } finally { // moved here to clean context everytime also if we have an exception EntThreadLocal.destroy(); @@ -356,8 +361,7 @@ private void consumeStateOrThrow(HttpServletRequest request, String stateParamet : (issued != null && System.currentTimeMillis() - issued > STATE_TTL_MILLIS) ? "expired" : null; if (reason != null) { - log.warn("Login callback rejected: state {}", reason); - throw new EntandoTokenException("invalid state", request, SystemConstants.GUEST_USER_NAME); + throw new EntandoTokenException("invalid state (" + reason + ")", request, SystemConstants.GUEST_USER_NAME); } } diff --git a/keycloak-plugin/src/test/java/org/entando/entando/keycloak/filter/KeycloakFilterTest.java b/keycloak-plugin/src/test/java/org/entando/entando/keycloak/filter/KeycloakFilterTest.java index 8d0ca9df4..baf7a012b 100644 --- a/keycloak-plugin/src/test/java/org/entando/entando/keycloak/filter/KeycloakFilterTest.java +++ b/keycloak-plugin/src/test/java/org/entando/entando/keycloak/filter/KeycloakFilterTest.java @@ -40,8 +40,6 @@ import org.entando.entando.keycloak.services.oidc.OpenIDConnectService; import org.entando.entando.keycloak.services.oidc.model.AccessToken; import org.entando.entando.keycloak.services.oidc.model.AuthResponse; -import org.entando.entando.web.common.exceptions.EntandoTokenException; -import org.junit.jupiter.api.Assertions; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; @@ -174,7 +172,7 @@ void testAuthenticationFlow() throws IOException, ServletException, EntException } @Test - void testAuthenticationFlowWithError() { + void testAuthenticationFlowWithError() throws IOException, ServletException { final String state = "0ca97afd-f0b0-4860-820a-b7cd1414f69c"; final String authorizationCode = "the-authorization-code-from-keycloak"; @@ -200,8 +198,9 @@ void testAuthenticationFlowWithError() { Mockito.lenient().when(auth.getRefreshToken()).thenReturn("refresh-token-over-here"); try ( MockedStatic wacUtil = Mockito.mockStatic(WebApplicationContextUtils.class)) { wacUtil.when(() -> WebApplicationContextUtils.getWebApplicationContext(svCtx)).thenReturn(wac); - Assertions.assertThrows(EntandoTokenException.class, () -> keycloakFilter.doFilter(request, response, filterChain)); + keycloakFilter.doFilter(request, response, filterChain); } + verify(response, times(1)).sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); } @Test @@ -480,7 +479,7 @@ void stateMatchingShouldConsumeStateAndSaveUser() throws Exception { } @Test - void stateMismatchShouldThrowAndNotRequestToken() throws Exception { + void stateMismatchShouldBeRejectedAndNotRequestToken() throws Exception { when(configuration.isEnabled()).thenReturn(true); when(request.getServletPath()).thenReturn("/do/login"); when(request.getParameter("code")).thenReturn(""); @@ -489,16 +488,16 @@ void stateMismatchShouldThrowAndNotRequestToken() throws Exception { try (MockedStatic wacUtil = Mockito.mockStatic(WebApplicationContextUtils.class)) { wacUtil.when(() -> WebApplicationContextUtils.getWebApplicationContext(svCtx)).thenReturn(wac); - Assertions.assertThrows(EntandoTokenException.class, - () -> keycloakFilter.doFilter(request, response, filterChain)); + keycloakFilter.doFilter(request, response, filterChain); } + verify(response, times(1)).sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); verify(oidcService, never()).requestToken(any(), any()); verify(session, never()).setAttribute(eq("user"), any()); } @Test - void stateMissingShouldThrowAndNotRequestToken() throws Exception { + void stateMissingShouldBeRejectedAndNotRequestToken() throws Exception { when(configuration.isEnabled()).thenReturn(true); when(request.getServletPath()).thenReturn("/do/login"); when(request.getParameter("code")).thenReturn(""); @@ -506,16 +505,16 @@ void stateMissingShouldThrowAndNotRequestToken() throws Exception { try (MockedStatic wacUtil = Mockito.mockStatic(WebApplicationContextUtils.class)) { wacUtil.when(() -> WebApplicationContextUtils.getWebApplicationContext(svCtx)).thenReturn(wac); - Assertions.assertThrows(EntandoTokenException.class, - () -> keycloakFilter.doFilter(request, response, filterChain)); + keycloakFilter.doFilter(request, response, filterChain); } + verify(response, times(1)).sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); verify(oidcService, never()).requestToken(any(), any()); verify(session, never()).setAttribute(eq("user"), any()); } @Test - void noPendingStateShouldThrowAndNotRequestToken() throws Exception { + void noPendingStateShouldBeRejectedAndNotRequestToken() throws Exception { when(configuration.isEnabled()).thenReturn(true); when(request.getServletPath()).thenReturn("/do/login"); when(request.getParameter("code")).thenReturn(""); @@ -523,16 +522,16 @@ void noPendingStateShouldThrowAndNotRequestToken() throws Exception { try (MockedStatic wacUtil = Mockito.mockStatic(WebApplicationContextUtils.class)) { wacUtil.when(() -> WebApplicationContextUtils.getWebApplicationContext(svCtx)).thenReturn(wac); - Assertions.assertThrows(EntandoTokenException.class, - () -> keycloakFilter.doFilter(request, response, filterChain)); + keycloakFilter.doFilter(request, response, filterChain); } + verify(response, times(1)).sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); verify(oidcService, never()).requestToken(any(), any()); verify(session, never()).setAttribute(eq("user"), any()); } @Test - void replayOfSameCallbackShouldThrowSecondTime() throws Exception { + void replayOfSameCallbackShouldBeRejectedSecondTime() throws Exception { when(configuration.isEnabled()).thenReturn(true); when(request.getServletPath()).thenReturn("/do/login"); when(request.getParameter("code")).thenReturn(""); @@ -543,12 +542,12 @@ void replayOfSameCallbackShouldThrowSecondTime() throws Exception { try (MockedStatic wacUtil = Mockito.mockStatic(WebApplicationContextUtils.class)) { wacUtil.when(() -> WebApplicationContextUtils.getWebApplicationContext(svCtx)).thenReturn(wac); keycloakFilter.doFilter(request, response, filterChain); - Assertions.assertThrows(EntandoTokenException.class, - () -> keycloakFilter.doFilter(request, response, filterChain)); + keycloakFilter.doFilter(request, response, filterChain); } verify(oidcService, times(1)).requestToken(eq(""), any()); verify(session, times(1)).setAttribute(eq("user"), same(userDetails)); + verify(response, times(1)).sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); } @Test @@ -611,7 +610,7 @@ void legacyStateAttributeShouldLoginAndBeRemoved() throws Exception { } @Test - void keycloakErrorCallbackShouldThrowBeforeStateCheck() throws Exception { + void keycloakErrorCallbackShouldBeRejectedBeforeStateCheck() throws Exception { when(configuration.isEnabled()).thenReturn(true); when(request.getServletPath()).thenReturn("/do/login"); when(request.getParameter("code")).thenReturn(""); @@ -621,10 +620,10 @@ void keycloakErrorCallbackShouldThrowBeforeStateCheck() throws Exception { try (MockedStatic wacUtil = Mockito.mockStatic(WebApplicationContextUtils.class)) { wacUtil.when(() -> WebApplicationContextUtils.getWebApplicationContext(svCtx)).thenReturn(wac); - Assertions.assertThrows(EntandoTokenException.class, - () -> keycloakFilter.doFilter(request, response, filterChain)); + keycloakFilter.doFilter(request, response, filterChain); } + verify(response, times(1)).sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); verify(oidcService, never()).requestToken(any(), any()); verify(session, never()).removeAttribute(KeycloakFilter.SESSION_PARAM_STATE); } From 0447323ccdc75c30e38ce58b3001f9c54afe38a7 Mon Sep 17 00:00:00 2001 From: "Matteo E. Minnai" Date: Fri, 16 Oct 2026 11:47:12 +0200 Subject: [PATCH 3/5] ESB-1294 .gitignore --- .gitignore | 1 + 1 file changed, 1 insertion(+) diff --git a/.gitignore b/.gitignore index d8be0908a..b131955d9 100644 --- a/.gitignore +++ b/.gitignore @@ -11,3 +11,4 @@ derby.log /.REVIEW/ /.TESTING/ /.PLAN/ +/.RELEASE/ From b95541e0722f69cb87632db304979edc83fb5a9e Mon Sep 17 00:00:00 2001 From: "Matteo E. Minnai" Date: Fri, 16 Oct 2026 12:13:42 +0200 Subject: [PATCH 4/5] ESB-1294 Code quality --- .../keycloak/filter/KeycloakFilter.java | 22 ++++++++++++++----- .../keycloak/filter/KeycloakFilterTest.java | 6 ++--- 2 files changed, 19 insertions(+), 9 deletions(-) diff --git a/keycloak-plugin/src/main/java/org/entando/entando/keycloak/filter/KeycloakFilter.java b/keycloak-plugin/src/main/java/org/entando/entando/keycloak/filter/KeycloakFilter.java index f011e35bd..abb3e74db 100644 --- a/keycloak-plugin/src/main/java/org/entando/entando/keycloak/filter/KeycloakFilter.java +++ b/keycloak-plugin/src/main/java/org/entando/entando/keycloak/filter/KeycloakFilter.java @@ -356,15 +356,25 @@ private void consumeStateOrThrow(HttpServletRequest request, String stateParamet Long issued = (matched != null) ? states.remove(matched) : null; session.setAttribute(SESSION_PARAM_STATES, states); session.removeAttribute(SESSION_PARAM_STATE); - String reason = (stateParameter == null) ? "missing" - : (matched == null) ? "unknown" - : (issued != null && System.currentTimeMillis() - issued > STATE_TTL_MILLIS) ? "expired" - : null; + String reason = stateRejectionReason(stateParameter, matched, issued); if (reason != null) { throw new EntandoTokenException("invalid state (" + reason + ")", request, SystemConstants.GUEST_USER_NAME); } } + private String stateRejectionReason(String stateParameter, String matched, Long issued) { + if (stateParameter == null) { + return "missing"; + } + if (matched == null) { + return "unknown"; + } + if (issued != null && System.currentTimeMillis() - issued > STATE_TTL_MILLIS) { + return "expired"; + } + return null; + } + @SuppressWarnings("unchecked") private LinkedHashMap readPendingStates(HttpSession session) { LinkedHashMap states = new LinkedHashMap<>(); @@ -373,8 +383,8 @@ private LinkedHashMap readPendingStates(HttpSession session) { states.putAll((Map) current); } Object legacy = session.getAttribute(SESSION_PARAM_STATE); - if (legacy instanceof String) { - states.putIfAbsent((String) legacy, null); + if (legacy instanceof String legacyState) { + states.putIfAbsent(legacyState, null); } long now = System.currentTimeMillis(); states.values().removeIf(issued -> issued != null && now - issued > STATE_TTL_MILLIS); diff --git a/keycloak-plugin/src/test/java/org/entando/entando/keycloak/filter/KeycloakFilterTest.java b/keycloak-plugin/src/test/java/org/entando/entando/keycloak/filter/KeycloakFilterTest.java index baf7a012b..745d6f58e 100644 --- a/keycloak-plugin/src/test/java/org/entando/entando/keycloak/filter/KeycloakFilterTest.java +++ b/keycloak-plugin/src/test/java/org/entando/entando/keycloak/filter/KeycloakFilterTest.java @@ -857,9 +857,9 @@ private void stubStatefulPendingStates(LinkedHashMap initial) { } private void stubSuccessfulTokenExchange(String code, String username) throws EntException { - AuthResponse authResponse = new AuthResponse(); - authResponse.setAccessToken(""); - when(oidcService.requestToken(eq(code), any())).thenReturn(ResponseEntity.ok().body(authResponse)); + AuthResponse tokenExchangeBody = new AuthResponse(); + tokenExchangeBody.setAccessToken(""); + when(oidcService.requestToken(eq(code), any())).thenReturn(ResponseEntity.ok().body(tokenExchangeBody)); AccessToken token = new AccessToken(); token.setActive(true); From d083e743f0f8f8b8902e9cf8133470e3d2c67712 Mon Sep 17 00:00:00 2001 From: "Matteo E. Minnai" Date: Fri, 16 Oct 2026 14:18:06 +0200 Subject: [PATCH 5/5] ESB-1294 Limit the one-line login rejection handling to the /do/login callback --- .../keycloak/filter/KeycloakFilter.java | 28 ++++++++++--------- .../keycloak/filter/KeycloakFilterTest.java | 18 ++++++++++++ 2 files changed, 33 insertions(+), 13 deletions(-) diff --git a/keycloak-plugin/src/main/java/org/entando/entando/keycloak/filter/KeycloakFilter.java b/keycloak-plugin/src/main/java/org/entando/entando/keycloak/filter/KeycloakFilter.java index abb3e74db..2d36fd118 100644 --- a/keycloak-plugin/src/main/java/org/entando/entando/keycloak/filter/KeycloakFilter.java +++ b/keycloak-plugin/src/main/java/org/entando/entando/keycloak/filter/KeycloakFilter.java @@ -24,13 +24,6 @@ import com.agiletec.aps.system.services.user.UserDetails; import com.agiletec.aps.util.ApsTenantApplicationUtils; import com.fasterxml.jackson.databind.ObjectMapper; -import java.io.IOException; -import java.nio.charset.StandardCharsets; -import java.security.MessageDigest; -import java.util.LinkedHashMap; -import java.util.Map; -import java.util.Optional; -import java.util.UUID; import jakarta.servlet.Filter; import jakarta.servlet.FilterChain; import jakarta.servlet.FilterConfig; @@ -40,6 +33,13 @@ import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; import jakarta.servlet.http.HttpSession; +import java.io.IOException; +import java.nio.charset.StandardCharsets; +import java.security.MessageDigest; +import java.util.LinkedHashMap; +import java.util.Map; +import java.util.Optional; +import java.util.UUID; import org.apache.commons.lang3.StringUtils; import org.entando.entando.KeycloakWiki; import org.entando.entando.aps.servlet.security.GuestAuthentication; @@ -127,7 +127,14 @@ public void doFilter(final ServletRequest servletRequest, case "/do/doLogin": case "/do/login.action": case "/do/doLogin.action": - doLogin(request, response, chain); + try { + doLogin(request, response, chain); + } catch (EntandoTokenException e) { + // Report the rejected authentication as a single log line instead of letting the + // exception propagate to the container, which would log a full stack trace. + log.warn("Login rejected: {}", e.getMessage()); + response.sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); + } break; case "/do/logout": case "/do/logout.action": @@ -141,11 +148,6 @@ public void doFilter(final ServletRequest servletRequest, chain.doFilter(request, response); } - } catch (EntandoTokenException e) { - // Report the rejected authentication as a single log line instead of letting the - // exception propagate to the container, which would log a full stack trace. - log.warn("Login rejected: {}", e.getMessage()); - ((HttpServletResponse) servletResponse).sendError(HttpServletResponse.SC_INTERNAL_SERVER_ERROR); } finally { // moved here to clean context everytime also if we have an exception EntThreadLocal.destroy(); diff --git a/keycloak-plugin/src/test/java/org/entando/entando/keycloak/filter/KeycloakFilterTest.java b/keycloak-plugin/src/test/java/org/entando/entando/keycloak/filter/KeycloakFilterTest.java index 745d6f58e..073569eeb 100644 --- a/keycloak-plugin/src/test/java/org/entando/entando/keycloak/filter/KeycloakFilterTest.java +++ b/keycloak-plugin/src/test/java/org/entando/entando/keycloak/filter/KeycloakFilterTest.java @@ -40,6 +40,7 @@ import org.entando.entando.keycloak.services.oidc.OpenIDConnectService; import org.entando.entando.keycloak.services.oidc.model.AccessToken; import org.entando.entando.keycloak.services.oidc.model.AuthResponse; +import org.entando.entando.web.common.exceptions.EntandoTokenException; import org.junit.jupiter.api.BeforeEach; import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; @@ -628,6 +629,23 @@ void keycloakErrorCallbackShouldBeRejectedBeforeStateCheck() throws Exception { verify(session, never()).removeAttribute(KeycloakFilter.SESSION_PARAM_STATE); } + @Test + void tokenExceptionFromDownstreamApiShouldPropagate() throws Exception { + when(configuration.isEnabled()).thenReturn(true); + when(request.getServletPath()).thenReturn("/api"); + EntandoTokenException downstream = new EntandoTokenException("no token found", request, "guest"); + Mockito.doThrow(downstream).when(filterChain).doFilter(any(), any()); + + try (MockedStatic wacUtil = Mockito.mockStatic(WebApplicationContextUtils.class)) { + wacUtil.when(() -> WebApplicationContextUtils.getWebApplicationContext(svCtx)).thenReturn(wac); + EntandoTokenException thrown = org.junit.jupiter.api.Assertions.assertThrows(EntandoTokenException.class, + () -> keycloakFilter.doFilter(request, response, filterChain)); + assertThat(thrown).isSameAs(downstream); + } + + verify(response, never()).sendError(Mockito.anyInt()); + } + @Test void testKeycloakJsonEndpoint() throws IOException, ServletException { final String path = "/keycloak.json";