From f06eff069e5705bb35036f47a3b722a4a233e4c1 Mon Sep 17 00:00:00 2001 From: Filip Hanik Date: Wed, 17 Jun 2026 11:44:04 -0700 Subject: [PATCH 1/6] fix: show clear error message when concurrent browser-tab login attempt detected (#3701) When a user initiates external OAuth login from two browser tabs, UAA stores only the most-recent state in the HTTP session (overwriting the older one). If the older tab's IDP callback arrives, the state check fails with a cryptic error. This detects the specific case (session has a non-null state that differs from the incoming state) and surfaces a human-readable "Another sign-in is already in progress" message with a "Start a new login" link, while keeping the existing CSRF protection intact for all other state-failure cases. Co-Authored-By: Claude Sonnet 4.6 --- .../identity/uaa/home/HomeController.java | 6 +- .../ConcurrentLoginAttemptException.java | 20 ++ .../ExternalOAuthAuthenticationFilter.java | 14 +- .../ExternalOAuthProviderConfigurator.java | 10 +- .../identity/uaa/util/SessionUtils.java | 64 +++++ .../templates/web/external_auth_error.html | 6 + .../uaa/login/HomeControllerViewTests.java | 27 ++ ...ExternalOAuthAuthenticationFilterTest.java | 76 ++++- ...ernalOAuthConcurrentLoginScenarioTest.java | 263 ++++++++++++++++++ .../identity/uaa/util/SessionUtilsTest.java | 68 +++++ .../feature/ConcurrentExternalLoginIT.java | 218 +++++++++++++++ .../pageObjects/OAuthErrorPage.java | 54 ++++ 12 files changed, 816 insertions(+), 10 deletions(-) create mode 100644 server/src/main/java/org/cloudfoundry/identity/uaa/provider/oauth/ConcurrentLoginAttemptException.java create mode 100644 server/src/test/java/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthConcurrentLoginScenarioTest.java create mode 100644 uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/feature/ConcurrentExternalLoginIT.java create mode 100644 uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/pageObjects/OAuthErrorPage.java diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/home/HomeController.java b/server/src/main/java/org/cloudfoundry/identity/uaa/home/HomeController.java index c064ed4952e..2ddf876f056 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/home/HomeController.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/home/HomeController.java @@ -172,11 +172,15 @@ public String error401(Model model, HttpServletRequest request) { public String error_oauth(Model model, HttpServletRequest request) { String oauthError = "oauth_error"; String exception = (String) request.getSession().getAttribute(oauthError); - if (hasText(exception)) { model.addAttribute(oauthError, exception); request.getSession().removeAttribute(oauthError); } + + if ("concurrent_login".equals(request.getParameter("reason"))) { + model.addAttribute("oauth_concurrent_login", Boolean.TRUE); + } + return EXTERNAL_AUTH_ERROR; } diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/provider/oauth/ConcurrentLoginAttemptException.java b/server/src/main/java/org/cloudfoundry/identity/uaa/provider/oauth/ConcurrentLoginAttemptException.java new file mode 100644 index 00000000000..40b33e8388c --- /dev/null +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/provider/oauth/ConcurrentLoginAttemptException.java @@ -0,0 +1,20 @@ +package org.cloudfoundry.identity.uaa.provider.oauth; + +/** + * Thrown when a callback arrives with a state parameter that doesn't match the session's state + * because a newer login attempt has already replaced it (e.g. two browser tabs both initiated login + * for the same external IDP, and the older tab's callback arrived after the newer tab's state was stored). + */ +class ConcurrentLoginAttemptException extends RuntimeException { + + private final String originKey; + + ConcurrentLoginAttemptException(String originKey) { + super("Concurrent login attempt detected for origin: " + originKey); + this.originKey = originKey; + } + + String getOriginKey() { + return originKey; + } +} diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthAuthenticationFilter.java b/server/src/main/java/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthAuthenticationFilter.java index 699e98e19a2..385ab094abc 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthAuthenticationFilter.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthAuthenticationFilter.java @@ -60,7 +60,16 @@ public void doFilter( return; } - checkRequestStateParameter(request); + try { + checkRequestStateParameter(request); + } catch (ConcurrentLoginAttemptException ex) { + logger.warn("Concurrent login attempt detected for origin [{}]; a newer login superseded this one", ex.getOriginKey()); + response.sendRedirect(request.getContextPath() + "/oauth_error?reason=concurrent_login"); + return; + } catch (CsrfException ex) { + response.sendRedirect(request.getContextPath() + "/login?error=invalid_login_request"); + return; + } if (authenticationWasSuccessful(request, response)) { chain.doFilter(request, response); @@ -77,6 +86,9 @@ private void checkRequestStateParameter(final HttpServletRequest request) final Object stateInSession = SessionUtils.getStateParam(session, SessionUtils.stateParameterAttributeKeyForIdp(originKey)); final String stateFromParameters = request.getParameter("state"); if (UaaStringUtils.isEmpty(stateFromParameters) || !stateFromParameters.equals(stateInSession)) { + if (SessionUtils.consumeSupersededState(session, originKey, stateFromParameters)) { + throw new ConcurrentLoginAttemptException(originKey); + } throw new CsrfException("Invalid State Param in request."); } } diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthProviderConfigurator.java b/server/src/main/java/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthProviderConfigurator.java index d56ff476d49..dafa7d61b00 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthProviderConfigurator.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthProviderConfigurator.java @@ -84,7 +84,15 @@ public String getIdpAuthenticationUrl( var relyingPartyId = definition.getRelyingPartyId(); var state = generateStateParam(); - SessionUtils.setStateParam(request.getSession(), SessionUtils.stateParameterAttributeKeyForIdp(idpOriginKey), state); + var stateAttributeKey = SessionUtils.stateParameterAttributeKeyForIdp(idpOriginKey); + var previousState = SessionUtils.getStateParam(request.getSession(), stateAttributeKey); + if (previousState instanceof String previous && !previous.equals(state)) { + // A login flow for this IDP was already in progress in this session (e.g. another + // browser tab). Remember the state we are about to overwrite so a late callback from + // that older tab can be recognised as a concurrent login rather than a CSRF attempt. + SessionUtils.recordSupersededState(request.getSession(), idpOriginKey, previous); + } + SessionUtils.setStateParam(request.getSession(), stateAttributeKey, state); SessionUtils.setStateParam(request.getSession(), SessionUtils.redirectUriParameterAttributeKeyForIdp(idpOriginKey), URLDecoder.decode(callbackUrl, StandardCharsets.UTF_8)); diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/util/SessionUtils.java b/server/src/main/java/org/cloudfoundry/identity/uaa/util/SessionUtils.java index 60bc0cacb1a..e22b3689404 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/util/SessionUtils.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/util/SessionUtils.java @@ -7,6 +7,8 @@ import org.springframework.security.web.savedrequest.SavedRequest; import jakarta.servlet.http.HttpSession; +import java.util.ArrayDeque; +import java.util.Deque; public final class SessionUtils { public static final String PASSWORD_CHANGE_REQUIRED = "PASSWORD_CHANGE_REQUIRED"; @@ -27,6 +29,13 @@ public final class SessionUtils { private static final String EXTERNAL_OAUTH_STATE_ATTRIBUTE_PREFIX = "external-oauth-state-"; private static final String EXTERNAL_OAUTH_CODE_VERIFIER_ATTRIBUTE_PREFIX = "external-oauth-verifier-"; private static final String EXTERNAL_OAUTH_REDIRECT_URI_ATTRIBUTE_PREFIX = "external-oauth-redirect-uri-"; + private static final String EXTERNAL_OAUTH_SUPERSEDED_STATE_ATTRIBUTE_PREFIX = "external-oauth-superseded-state-"; + + /** + * Upper bound on how many recently-superseded state values we remember per IDP origin. + * Keeps the session footprint bounded while still tolerating a handful of concurrent tabs. + */ + private static final int MAX_SUPERSEDED_STATES = 5; private SessionUtils() { } @@ -65,6 +74,57 @@ public static Object getStateParam(HttpSession session, String stateParamKey) { return session.getAttribute(stateParamKey); } + /** + * Records a previously-issued state value that has just been overwritten by a newer login + * attempt for the same IDP origin (e.g. a second browser tab started the login flow). Only + * states UAA actually issued end up here, so the callback filter can distinguish a genuine + * concurrent-login race from a forged/tampered state (which is a CSRF attempt). + */ + @SuppressWarnings("unchecked") + public static void recordSupersededState(HttpSession session, String idpOriginKey, String supersededState) { + if (supersededState == null) { + return; + } + String key = supersededStateParameterAttributeKeyForIdp(idpOriginKey); + Object existing = session.getAttribute(key); + Deque supersededStates = existing instanceof Deque + ? (Deque) existing + : new ArrayDeque<>(); + supersededStates.remove(supersededState); + supersededStates.addFirst(supersededState); + while (supersededStates.size() > MAX_SUPERSEDED_STATES) { + supersededStates.removeLast(); + } + session.setAttribute(key, supersededStates); + } + + /** + * Checks whether {@code state} was a state UAA issued for this IDP origin that has since been + * superseded by a newer login attempt, and if so removes it from the superseded list (one-time + * consumption). After this call the same state value is no longer recognised as a known-stale + * state, so a replay of the same callback will be treated as CSRF. + * + * @return {@code true} if {@code state} was found in (and removed from) the superseded list + */ + @SuppressWarnings("unchecked") + public static boolean consumeSupersededState(HttpSession session, String idpOriginKey, String state) { + if (state == null) { + return false; + } + String key = supersededStateParameterAttributeKeyForIdp(idpOriginKey); + Object existing = session.getAttribute(key); + if (!(existing instanceof Deque raw)) { + return false; + } + Deque deque = (Deque) raw; + boolean removed = deque.remove(state); + if (removed) { + // Persist the mutated deque so external session stores (e.g. Redis) see the change. + session.setAttribute(key, deque); + } + return removed; + } + public static void setSecurityContext(HttpSession session, SecurityContext context) { session.setAttribute(SPRING_SECURITY_CONTEXT, context); } @@ -92,4 +152,8 @@ public static String codeVerifierParameterAttributeKeyForIdp(String idpOriginKey public static String redirectUriParameterAttributeKeyForIdp(String idpOriginKey) { return EXTERNAL_OAUTH_REDIRECT_URI_ATTRIBUTE_PREFIX + idpOriginKey; } + + public static String supersededStateParameterAttributeKeyForIdp(String idpOriginKey) { + return EXTERNAL_OAUTH_SUPERSEDED_STATE_ATTRIBUTE_PREFIX + idpOriginKey; + } } diff --git a/server/src/main/resources/templates/web/external_auth_error.html b/server/src/main/resources/templates/web/external_auth_error.html index 7bc51a25f8f..1dad36181de 100644 --- a/server/src/main/resources/templates/web/external_auth_error.html +++ b/server/src/main/resources/templates/web/external_auth_error.html @@ -24,6 +24,12 @@ + +

Another sign-in is already in progress

+

A sign-in attempt from another browser tab or window has replaced this one. + To complete sign-in, please start a new login.

+ Start a new login +

diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/login/HomeControllerViewTests.java b/server/src/test/java/org/cloudfoundry/identity/uaa/login/HomeControllerViewTests.java index 091ed78ae03..83230b3cb26 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/login/HomeControllerViewTests.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/login/HomeControllerViewTests.java @@ -171,6 +171,33 @@ void errorOauthWithExceptionString() throws Exception { .andExpect(content().string(containsString(base64ProductLogo))); } + /** + * Verifies that {@code /oauth_error?reason=concurrent_login} displays a human-readable + * "Another sign-in is already in progress" message and a link that lets the user restart + * the login flow — rather than showing a cryptic technical error. + */ + @Test + void oauthError_withConcurrentLoginReason_showsFriendlyMessageAndRestartLink() throws Exception { + mockMvc.perform(get("/oauth_error").param("reason", "concurrent_login")) + .andExpect(status().isOk()) + .andExpect(content().string(containsString("Another sign-in is already in progress"))) + .andExpect(content().string(containsString("Start a new login"))); + } + + @Test + void oauthError_withConcurrentLoginReason_doesNotShowGenericOAuthError() throws Exception { + mockMvc.perform(get("/oauth_error").param("reason", "concurrent_login")) + .andExpect(status().isOk()) + .andExpect(content().string(org.hamcrest.Matchers.not(containsString("There was an error when authenticating")))); + } + + @Test + void oauthError_withoutConcurrentLoginReason_doesNotShowFriendlyMessage() throws Exception { + mockMvc.perform(get("/oauth_error")) + .andExpect(status().isOk()) + .andExpect(content().string(org.hamcrest.Matchers.not(containsString("Another sign-in is already in progress")))); + } + @Test void error500WithGenericException() throws Exception { mockMvc.perform(get("/error500").requestAttr(RequestDispatcher.ERROR_EXCEPTION, new Exception("bad"))) diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthAuthenticationFilterTest.java b/server/src/test/java/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthAuthenticationFilterTest.java index 9a21438d1bc..8917bcaab21 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthAuthenticationFilterTest.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthAuthenticationFilterTest.java @@ -1,4 +1,5 @@ package org.cloudfoundry.identity.uaa.provider.oauth; + import org.assertj.core.api.InstanceOfAssertFactories; import org.cloudfoundry.identity.uaa.extensions.PollutionPreventionExtension; import org.cloudfoundry.identity.uaa.login.AccountSavingAuthenticationSuccessHandler; @@ -9,7 +10,6 @@ import org.junit.jupiter.api.extension.ExtendWith; import org.springframework.security.authentication.BadCredentialsException; import org.springframework.security.core.Authentication; -import org.springframework.security.web.csrf.CsrfException; import org.springframework.web.HttpSessionRequiredException; import jakarta.servlet.FilterChain; @@ -17,6 +17,8 @@ import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; import jakarta.servlet.http.HttpSession; +import java.util.ArrayDeque; +import java.util.List; import java.util.function.Consumer; import static org.assertj.core.api.Assertions.assertThatThrownBy; @@ -39,6 +41,10 @@ void setUp() { mockFilterChain = mock(FilterChain.class); } + private ExternalOAuthAuthenticationFilter buildFilter(AccountSavingAuthenticationSuccessHandler successHandler) { + return new ExternalOAuthAuthenticationFilter(externalOAuthAuthenticationManager, successHandler); + } + @Nested @ExtendWith(PollutionPreventionExtension.class) class WhenAuthenticationSucceeds { @@ -129,8 +135,9 @@ void itThrowsIfNoStateInSession() throws Exception { }); HttpServletResponse mockResponse = mock(HttpServletResponse.class); - assertThatThrownBy(() -> - externalOAuthAuthenticationFilter.doFilter(mockRequest, mockResponse, mockFilterChain)).asInstanceOf(InstanceOfAssertFactories.throwable(CsrfException.class)); + externalOAuthAuthenticationFilter.doFilter(mockRequest, mockResponse, mockFilterChain); + + verify(mockResponse).sendRedirect("/uaa/login?error=invalid_login_request"); verify(mockFilterChain, never()).doFilter(mockRequest, mockResponse); } @@ -142,8 +149,9 @@ void itThrowsIfNoStateInRequest() throws Exception { }); HttpServletResponse mockResponse = mock(HttpServletResponse.class); - assertThatThrownBy(() -> - externalOAuthAuthenticationFilter.doFilter(mockRequest, mockResponse, mockFilterChain)).asInstanceOf(InstanceOfAssertFactories.throwable(CsrfException.class)); + externalOAuthAuthenticationFilter.doFilter(mockRequest, mockResponse, mockFilterChain); + + verify(mockResponse).sendRedirect("/uaa/login?error=invalid_login_request"); verify(mockFilterChain, never()).doFilter(mockRequest, mockResponse); } @@ -156,12 +164,61 @@ void itThrowsIfStateIsMismatched() throws Exception { }); HttpServletResponse mockResponse = mock(HttpServletResponse.class); - assertThatThrownBy(() -> - externalOAuthAuthenticationFilter.doFilter(mockRequest, mockResponse, mockFilterChain)).asInstanceOf(InstanceOfAssertFactories.throwable(CsrfException.class)); + externalOAuthAuthenticationFilter.doFilter(mockRequest, mockResponse, mockFilterChain); + + verify(mockResponse).sendRedirect("/uaa/login?error=invalid_login_request"); verify(mockFilterChain, never()).doFilter(mockRequest, mockResponse); } } + @Nested + @ExtendWith(PollutionPreventionExtension.class) + class WhenConcurrentLoginAttemptDetected { + + private static final String STATE_FROM_TAB_1 = "state-generated-when-tab1-clicked-idp-link"; + private static final String STATE_FROM_TAB_2 = "state-generated-when-tab2-clicked-idp-link"; + + @BeforeEach + void setUp() { + externalOAuthAuthenticationFilter = buildFilter(null); + } + + @Test + void itRedirectsToConcurrentLoginErrorPage() throws Exception { + HttpServletRequest mockRequest = buildConcurrentLoginRequest(); + HttpServletResponse mockResponse = mock(HttpServletResponse.class); + + externalOAuthAuthenticationFilter.doFilter(mockRequest, mockResponse, mockFilterChain); + + verify(mockResponse).sendRedirect("/uaa/oauth_error?reason=concurrent_login"); + verify(mockFilterChain, never()).doFilter(any(), any()); + } + + @Test + void itRemovesConsumedStateFromSupersededListInSession() throws Exception { + HttpServletRequest mockRequest = buildConcurrentLoginRequest(); + HttpServletResponse mockResponse = mock(HttpServletResponse.class); + + externalOAuthAuthenticationFilter.doFilter(mockRequest, mockResponse, mockFilterChain); + + // The filter must persist the mutated deque so external session stores (e.g. Redis) see it. + String supersededKey = SessionUtils.supersededStateParameterAttributeKeyForIdp(ORIGIN_KEY); + verify(mockRequest.getSession()).setAttribute( + org.mockito.ArgumentMatchers.eq(supersededKey), + org.mockito.ArgumentMatchers.argThat(deque -> + deque instanceof java.util.Deque d && !d.contains(STATE_FROM_TAB_1))); + } + + private HttpServletRequest buildConcurrentLoginRequest() { + return mockRedirectRequest(ORIGIN_KEY, request -> { + mockAuthenticationInRequest(request); + mockStateParamInRequest(request, STATE_FROM_TAB_1); + mockStateParamInSession(request.getSession(), ORIGIN_KEY, STATE_FROM_TAB_2); + mockSupersededStatesInSession(request.getSession(), ORIGIN_KEY, STATE_FROM_TAB_1); + }); + } + } + @Nested @ExtendWith(PollutionPreventionExtension.class) class WhenNoCredentialsPresent { @@ -232,4 +289,9 @@ private void mockStateParamInRequest(HttpServletRequest request, String state) { private void mockStateParamInSession(HttpSession session, String origin, String state) { when(session.getAttribute(SessionUtils.stateParameterAttributeKeyForIdp(origin))).thenReturn(state); } + + private void mockSupersededStatesInSession(HttpSession session, String origin, String... states) { + when(session.getAttribute(SessionUtils.supersededStateParameterAttributeKeyForIdp(origin))) + .thenReturn(new ArrayDeque<>(List.of(states))); + } } diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthConcurrentLoginScenarioTest.java b/server/src/test/java/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthConcurrentLoginScenarioTest.java new file mode 100644 index 00000000000..9351f3ad558 --- /dev/null +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthConcurrentLoginScenarioTest.java @@ -0,0 +1,263 @@ +package org.cloudfoundry.identity.uaa.provider.oauth; + +import org.cloudfoundry.identity.uaa.util.SessionUtils; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.springframework.mock.web.MockHttpSession; +import org.springframework.security.authentication.BadCredentialsException; + +import jakarta.servlet.FilterChain; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatThrownBy; +import static org.mockito.ArgumentMatchers.any; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.never; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.when; + +/** + * Scenario tests for the multi-browser-tab concurrent external OAuth login problem. + * + *

Background

+ * UAA stores one {@code state} parameter per IDP in the HTTP session. When a user + * triggers the login flow in two browser tabs for the same IDP, the session ends up + * holding only the most-recently generated state. If the older tab's callback arrives + * first, the state check fails. + * + *

Scenarios covered

+ *
    + *
  1. Happy path – a single tab completes login; state matches; filter chain continues.
    2. + *
  2. No session – session has been destroyed (e.g. timeout); filter throws + * {@link org.springframework.web.HttpSessionRequiredException}.
    3. + *
  3. No state in session – CSRF / replay attempt; filter redirects to + * {@code /login?error=invalid_login_request}.
    4. + *
  4. No state in callback request – malformed or tampered callback; filter redirects to + * {@code /login?error=invalid_login_request}.
    5. + *
  5. Concurrent login (two tabs) – both tabs started login; session holds the newer + * state; older tab's callback arrives with a stale state. The filter removes the stale + * state from the superseded list and redirects to + * {@code /oauth_error?reason=concurrent_login} (friendly "Another sign-in is already in + * progress" message). End-to-end behaviour is covered by {@code ConcurrentExternalLoginIT}.
    6. + *
  6. Authentication failure – state is valid but the IDP token exchange fails; filter + * stores an {@code oauth_error} in the session and redirects to {@code /oauth_error}.
    7. + *
+ */ +class ExternalOAuthConcurrentLoginScenarioTest { + + private static final String IDP_ORIGIN = "my-external-idp"; + private static final String STATE_FROM_TAB_1 = "state-generated-by-tab-1"; + private static final String STATE_FROM_TAB_2 = "state-generated-by-tab-2"; + + private ExternalOAuthAuthenticationManager mockManager; + private FilterChain mockChain; + private ExternalOAuthAuthenticationFilter filter; + + @BeforeEach + void setUp() { + mockManager = mock(ExternalOAuthAuthenticationManager.class); + mockChain = mock(FilterChain.class); + filter = new ExternalOAuthAuthenticationFilter(mockManager, null); + } + + // ------------------------------------------------------------------------- + // Scenario 1: happy path — single tab, state matches + // ------------------------------------------------------------------------- + + @Test + void scenario_singleTab_stateMatches_filterChainContinues() throws Exception { + MockHttpSession session = new MockHttpSession(); + session.setAttribute(SessionUtils.stateParameterAttributeKeyForIdp(IDP_ORIGIN), STATE_FROM_TAB_1); + + HttpServletRequest req = buildRequest(STATE_FROM_TAB_1, session); + HttpServletResponse res = mock(HttpServletResponse.class); + + filter.doFilter(req, res, mockChain); + + verify(mockChain).doFilter(req, res); + } + + // ------------------------------------------------------------------------- + // Scenario 2: no session (timeout or session fixation protection) + // ------------------------------------------------------------------------- + + @Test + void scenario_noSession_throwsHttpSessionRequiredException() { + HttpServletRequest req = buildRequestWithNoSession(STATE_FROM_TAB_1); + HttpServletResponse res = mock(HttpServletResponse.class); + + assertThatThrownBy(() -> filter.doFilter(req, res, mockChain)) + .isInstanceOf(org.springframework.web.HttpSessionRequiredException.class); + } + + // ------------------------------------------------------------------------- + // Scenario 3: state missing from session (CSRF / replay) + // ------------------------------------------------------------------------- + + @Test + void scenario_noStateInSession_redirectsToLogin() throws Exception { + MockHttpSession session = new MockHttpSession(); // empty session — no state stored + + HttpServletRequest req = buildRequest(STATE_FROM_TAB_1, session); + HttpServletResponse res = mock(HttpServletResponse.class); + + filter.doFilter(req, res, mockChain); + + verify(res).sendRedirect("/uaa/login?error=invalid_login_request"); + verify(mockChain, never()).doFilter(any(), any()); + } + + // ------------------------------------------------------------------------- + // Scenario 4: state missing from callback request (malformed / tampered) + // ------------------------------------------------------------------------- + + @Test + void scenario_noStateInCallback_redirectsToLogin() throws Exception { + MockHttpSession session = new MockHttpSession(); + session.setAttribute(SessionUtils.stateParameterAttributeKeyForIdp(IDP_ORIGIN), STATE_FROM_TAB_1); + + HttpServletRequest req = buildRequest(null, session); + HttpServletResponse res = mock(HttpServletResponse.class); + + filter.doFilter(req, res, mockChain); + + verify(res).sendRedirect("/uaa/login?error=invalid_login_request"); + verify(mockChain, never()).doFilter(any(), any()); + } + + // ------------------------------------------------------------------------- + // Scenario 5: concurrent login — the key new scenario + // + // Tab 1 starts → state_1 stored in session + // Tab 2 starts → state_2 OVERWRITES state_1 in session + // Tab 1 returns → callback carries state_1, but session has state_2 + // ------------------------------------------------------------------------- + + @Test + void scenario_concurrentTabs_olderTabCallback_redirectsToConcurrentLoginErrorPage() throws Exception { + HttpServletRequest req = buildConcurrentLoginRequest(); + HttpServletResponse res = mock(HttpServletResponse.class); + + filter.doFilter(req, res, mockChain); + + verify(res).sendRedirect("/uaa/oauth_error?reason=concurrent_login"); + verify(mockChain, never()).doFilter(any(), any()); + } + + // ------------------------------------------------------------------------- + // Scenario 5b: mismatched state that UAA never issued (CSRF / tampering). + // + // The session holds a valid state, the callback carries a different state that was never + // issued for this origin. This must still be treated as CSRF — NOT downgraded to the + // friendly concurrent-login redirect. + // ------------------------------------------------------------------------- + + @Test + void scenario_mismatchedStateNeverIssued_redirectsToLogin() throws Exception { + MockHttpSession session = new MockHttpSession(); + session.setAttribute(SessionUtils.stateParameterAttributeKeyForIdp(IDP_ORIGIN), STATE_FROM_TAB_2); + + HttpServletRequest req = buildRequest("forged-state-from-attacker", session); + HttpServletResponse res = mock(HttpServletResponse.class); + + filter.doFilter(req, res, mockChain); + + verify(res).sendRedirect("/uaa/login?error=invalid_login_request"); + verify(mockChain, never()).doFilter(any(), any()); + } + + // ------------------------------------------------------------------------- + // Scenario 5c: superseded state is consumed (one-time use). + // + // Tab 1's callback arrives, UAA recognises state_1 as superseded and redirects to the + // friendly error page — AND removes state_1 from the superseded list. + // A second callback arriving with the same state_1 must now be treated as CSRF. + // ------------------------------------------------------------------------- + + @Test + void scenario_supersededState_isConsumedOnFirstUse_secondAttemptRedirectsToLogin() throws Exception { + MockHttpSession session = sessionWithConcurrentLoginState(); + HttpServletResponse res = mock(HttpServletResponse.class); + + // First callback: state_1 is recognised as superseded → friendly error redirect. + HttpServletRequest firstReq = buildRequest(STATE_FROM_TAB_1, session); + filter.doFilter(firstReq, res, mockChain); + verify(res).sendRedirect("/uaa/oauth_error?reason=concurrent_login"); + + // Second callback: state_1 has been removed from the superseded list → CSRF redirect. + HttpServletRequest secondReq = buildRequest(STATE_FROM_TAB_1, session); + filter.doFilter(secondReq, res, mockChain); + verify(res).sendRedirect("/uaa/login?error=invalid_login_request"); + } + + // ------------------------------------------------------------------------- + // Scenario 6: authentication failure (valid state, but token exchange fails) + // ------------------------------------------------------------------------- + + @Test + void scenario_authenticationFailure_storesErrorInSessionAndRedirects() throws Exception { + when(mockManager.authenticate(any())) + .thenThrow(new BadCredentialsException("token exchange rejected by IDP")); + + MockHttpSession session = new MockHttpSession(); + session.setAttribute(SessionUtils.stateParameterAttributeKeyForIdp(IDP_ORIGIN), STATE_FROM_TAB_1); + + HttpServletRequest req = buildRequest(STATE_FROM_TAB_1, session); + HttpServletResponse res = mock(HttpServletResponse.class); + + filter.doFilter(req, res, mockChain); + + assertThat((String) session.getAttribute("oauth_error")) + .contains("token exchange rejected by IDP"); + verify(res).sendRedirect("/uaa/oauth_error"); + verify(mockChain, never()).doFilter(any(), any()); + } + + // ------------------------------------------------------------------------- + // Helpers + // ------------------------------------------------------------------------- + + private HttpServletRequest buildConcurrentLoginRequest() { + return buildRequest(STATE_FROM_TAB_1, sessionWithConcurrentLoginState()); + } + + /** + * Builds a session in the state it would be in after tab 2 superseded tab 1: the session holds + * tab 2's state and tab 1's (now stale, but UAA-issued) state is recorded as superseded. + */ + private MockHttpSession sessionWithConcurrentLoginState() { + MockHttpSession session = new MockHttpSession(); + session.setAttribute(SessionUtils.stateParameterAttributeKeyForIdp(IDP_ORIGIN), STATE_FROM_TAB_2); + SessionUtils.recordSupersededState(session, IDP_ORIGIN, STATE_FROM_TAB_1); + return session; + } + + private HttpServletRequest buildRequest(String stateInCallback, MockHttpSession session) { + HttpServletRequest req = mock(HttpServletRequest.class); + when(req.getContextPath()).thenReturn("/uaa"); + when(req.getRequestURI()).thenReturn("/uaa/login/callback/" + IDP_ORIGIN); + when(req.getServletPath()).thenReturn("login/callback/" + IDP_ORIGIN); + when(req.getRequestURL()).thenReturn(new StringBuffer("http://localhost/uaa/login/callback/" + IDP_ORIGIN)); + when(req.getParameter("code")).thenReturn("some-auth-code"); + when(req.getParameter("state")).thenReturn(stateInCallback); + when(req.getSession()).thenReturn(session); + when(req.getSession(false)).thenReturn(session); + + return req; + } + + private HttpServletRequest buildRequestWithNoSession(String stateInCallback) { + HttpServletRequest req = mock(HttpServletRequest.class); + when(req.getContextPath()).thenReturn("/uaa"); + when(req.getRequestURI()).thenReturn("/uaa/login/callback/" + IDP_ORIGIN); + when(req.getServletPath()).thenReturn("login/callback/" + IDP_ORIGIN); + when(req.getRequestURL()).thenReturn(new StringBuffer("http://localhost/uaa/login/callback/" + IDP_ORIGIN)); + when(req.getParameter("code")).thenReturn("some-auth-code"); + when(req.getParameter("state")).thenReturn(stateInCallback); + when(req.getSession()).thenReturn(null); + + return req; + } +} diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/util/SessionUtilsTest.java b/server/src/test/java/org/cloudfoundry/identity/uaa/util/SessionUtilsTest.java index 7b4e0f618aa..474364bf00e 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/util/SessionUtilsTest.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/util/SessionUtilsTest.java @@ -2,9 +2,12 @@ import org.assertj.core.api.InstanceOfAssertFactories; import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; import org.springframework.mock.web.MockHttpSession; +import java.util.Deque; + import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatThrownBy; @@ -38,4 +41,69 @@ void isPasswordChangeRequiredIfSetNotBoolean() { mockHttpSession.setAttribute(SessionUtils.PASSWORD_CHANGE_REQUIRED, "true"); assertThatThrownBy(() -> SessionUtils.isPasswordChangeRequired(mockHttpSession)).asInstanceOf(InstanceOfAssertFactories.throwable(IllegalArgumentException.class)); } + + @Nested + class ConsumeSupersededState { + + private static final String IDP_ORIGIN = "test-idp"; + private static final String STATE_A = "state-aaa"; + private static final String STATE_B = "state-bbb"; + + @Test + void returnsTrueWhenStateIsInSupersededList() { + SessionUtils.recordSupersededState(mockHttpSession, IDP_ORIGIN, STATE_A); + + assertThat(SessionUtils.consumeSupersededState(mockHttpSession, IDP_ORIGIN, STATE_A)).isTrue(); + } + + @Test + void removesStateFromSupersededListAfterConsumption() { + SessionUtils.recordSupersededState(mockHttpSession, IDP_ORIGIN, STATE_A); + SessionUtils.consumeSupersededState(mockHttpSession, IDP_ORIGIN, STATE_A); + + @SuppressWarnings("unchecked") + Deque remaining = (Deque) mockHttpSession.getAttribute( + SessionUtils.supersededStateParameterAttributeKeyForIdp(IDP_ORIGIN)); + assertThat(remaining).doesNotContain(STATE_A); + } + + @Test + void secondCallWithSameStateReturnsFalse() { + SessionUtils.recordSupersededState(mockHttpSession, IDP_ORIGIN, STATE_A); + SessionUtils.consumeSupersededState(mockHttpSession, IDP_ORIGIN, STATE_A); + + assertThat(SessionUtils.consumeSupersededState(mockHttpSession, IDP_ORIGIN, STATE_A)).isFalse(); + } + + @Test + void returnsFalseForUnknownState() { + SessionUtils.recordSupersededState(mockHttpSession, IDP_ORIGIN, STATE_A); + + assertThat(SessionUtils.consumeSupersededState(mockHttpSession, IDP_ORIGIN, STATE_B)).isFalse(); + } + + @Test + void doesNotRemoveOtherStatesWhenConsumingOne() { + SessionUtils.recordSupersededState(mockHttpSession, IDP_ORIGIN, STATE_A); + SessionUtils.recordSupersededState(mockHttpSession, IDP_ORIGIN, STATE_B); + SessionUtils.consumeSupersededState(mockHttpSession, IDP_ORIGIN, STATE_A); + + @SuppressWarnings("unchecked") + Deque remaining = (Deque) mockHttpSession.getAttribute( + SessionUtils.supersededStateParameterAttributeKeyForIdp(IDP_ORIGIN)); + assertThat(remaining).contains(STATE_B); + } + + @Test + void returnsFalseWhenNoSupersededListExists() { + assertThat(SessionUtils.consumeSupersededState(mockHttpSession, IDP_ORIGIN, STATE_A)).isFalse(); + } + + @Test + void returnsFalseForNullState() { + SessionUtils.recordSupersededState(mockHttpSession, IDP_ORIGIN, STATE_A); + + assertThat(SessionUtils.consumeSupersededState(mockHttpSession, IDP_ORIGIN, null)).isFalse(); + } + } } \ No newline at end of file diff --git a/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/feature/ConcurrentExternalLoginIT.java b/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/feature/ConcurrentExternalLoginIT.java new file mode 100644 index 00000000000..61c69a7abae --- /dev/null +++ b/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/feature/ConcurrentExternalLoginIT.java @@ -0,0 +1,218 @@ +package org.cloudfoundry.identity.uaa.integration.feature; + +import org.cloudfoundry.identity.uaa.constants.OriginKeys; +import org.cloudfoundry.identity.uaa.integration.pageObjects.LoginPage; +import org.cloudfoundry.identity.uaa.integration.pageObjects.OAuthErrorPage; +import org.cloudfoundry.identity.uaa.integration.util.IntegrationTestUtils; +import org.cloudfoundry.identity.uaa.integration.util.ScreenshotOnFailExtension; +import org.cloudfoundry.identity.uaa.provider.IdentityProvider; +import org.cloudfoundry.identity.uaa.provider.OIDCIdentityProviderDefinition; +import org.cloudfoundry.identity.uaa.test.UaaWebDriver; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.BeforeEach; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.junit.jupiter.api.extension.RegisterExtension; +import org.openqa.selenium.By; +import org.openqa.selenium.WebElement; +import org.springframework.beans.factory.annotation.Autowired; +import org.springframework.beans.factory.annotation.Value; +import org.springframework.test.context.junit.jupiter.SpringJUnitConfig; + +import java.net.URI; +import java.net.URL; +import java.util.List; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Integration test demonstrating the concurrent multi-tab external login scenario. + * + *

Scenario

+ *
    + *
  1. Tab 1 loads the UAA login page — UAA generates {@code state=STATE_1} and embeds it in + * the IDP link href; the session stores {@code STATE_1}.
    2. + *
  2. Tab 2 loads the UAA login page — UAA generates {@code state=STATE_2}, which + * overwrites {@code STATE_1} in the session; the link href now carries {@code STATE_2}.
    3. + *
  3. Tab 1 returns from the IDP with {@code STATE_1}. The session holds {@code STATE_2} + * — mismatch — UAA detects the concurrent login attempt and shows a friendly error page + * with a "Start a new login" link instead of a cryptic security error.
    4. + *
+ * + *

To run: {@code ./gradlew :cloudfoundry-identity-uaa:integrationTest --tests "*ConcurrentExternalLoginIT*"} + * (requires a running UAA, e.g. started via {@code ./gradlew run}) + */ +@SpringJUnitConfig(classes = DefaultIntegrationTestConfig.class) +@ExtendWith(ScreenshotOnFailExtension.class) +class ConcurrentExternalLoginIT { + + private static final String IDP_ORIGIN = "concurrent-login-test-idp"; + private static final String IDP_LINK_TEXT = "Concurrent Login Test IDP"; + + @Autowired + @RegisterExtension + private IntegrationTestExtension integrationTestExtension; + + @Autowired + UaaWebDriver webDriver; + + @Value("${integration.test.base_url}") + String baseUrl; + + private IdentityProvider testIdp; + private String adminToken; + + @BeforeEach + void setUp() throws Exception { + adminToken = IntegrationTestUtils.getClientCredentialsToken(baseUrl, "admin", "adminsecret"); + testIdp = registerTestOidcIdp(); + logout(); + } + + @AfterEach + void tearDown() { + if (testIdp != null) { + IntegrationTestUtils.deleteProvider(adminToken, baseUrl, OriginKeys.UAA, IDP_ORIGIN); + } + logout(); + } + + /** + * Verifies that the concurrent login error page is shown with a friendly message and a + * link to restart the login flow. + */ + @Test + void concurrentLogin_showsFriendlyErrorPage() throws Exception { + // Step 1 — Load the login page once. + // UAA renders the IDP link with state=STATE_1 in the href and stores STATE_1 in the session. + webDriver.get(baseUrl + "/login"); + WebElement idpLink = webDriver.findElement(By.linkText(IDP_LINK_TEXT)); + String state1 = extractStateFromUrl(idpLink.getAttribute("href")); + assertThat(state1).as("state param must be present in the IDP link href").isNotBlank(); + + // Step 2 — Load the login page AGAIN (simulating a second browser tab). + // UAA generates STATE_2 and overwrites STATE_1 in the session. + webDriver.get(baseUrl + "/login"); + + // Step 3 — Simulate Tab 1 returning from the IDP with the now-stale STATE_1. + // The session holds STATE_2 → UAA detects concurrent login. + webDriver.get(baseUrl + "/login/callback/" + IDP_ORIGIN + "?code=fake-code&state=" + state1); + + // Step 4 — Verify the friendly error page. + OAuthErrorPage errorPage = new OAuthErrorPage(webDriver); + errorPage.assertConcurrentLoginMessageShown(); + errorPage.assertRestartLoginLinkPresent(); + } + + /** + * Verifies that clicking "Start a new login" on the error page returns the user to the + * normal login page. + */ + @Test + void concurrentLogin_startNewLoginLink_navigatesToLoginPage() throws Exception { + webDriver.get(baseUrl + "/login"); + String state1 = extractStateFromUrl(webDriver.findElement(By.linkText(IDP_LINK_TEXT)).getAttribute("href")); + webDriver.get(baseUrl + "/login"); + webDriver.get(baseUrl + "/login/callback/" + IDP_ORIGIN + "?code=fake-code&state=" + state1); + + OAuthErrorPage errorPage = new OAuthErrorPage(webDriver); + LoginPage loginPage = errorPage.clickStartNewLogin(baseUrl); + loginPage.assertThatLoginPageShown(); + } + + /** + * Verifies that the superseded state is consumed on first use: a second callback arriving + * with the same stale state (after the concurrent-login error was already shown once) is + * treated as a CSRF attempt, not as another concurrent login, and the user is redirected to + * the standard login page rather than shown the friendly error page again. + */ + @Test + void concurrentLogin_replayOfSameStaleState_redirectsToLoginPageNotFriendlyError() throws Exception { + // Step 1 — Produce the concurrent-login scenario (two tabs, stale state recorded). + webDriver.get(baseUrl + "/login"); + String state1 = extractStateFromUrl(webDriver.findElement(By.linkText(IDP_LINK_TEXT)).getAttribute("href")); + webDriver.get(baseUrl + "/login"); // generates state_2, records state_1 as superseded + + // Step 2 — First callback: stale state is recognised and consumed; friendly error shown. + webDriver.get(baseUrl + "/login/callback/" + IDP_ORIGIN + "?code=fake-code&state=" + state1); + new OAuthErrorPage(webDriver).assertConcurrentLoginMessageShown(); + + // Step 3 — Second callback with the same stale state: state_1 was already removed from + // the superseded list, so this is treated as CSRF → redirect to /login. + webDriver.get(baseUrl + "/login/callback/" + IDP_ORIGIN + "?code=fake-code&state=" + state1); + new LoginPage(webDriver).assertThatLoginPageShown(); + } + + /** + * Verifies that a completely forged (never-issued) state does not trigger the friendly + * concurrent-login error page — it is treated as a CSRF attempt and the user is redirected + * to the standard login page. + */ + @Test + void invalidState_neverIssuedByUaa_redirectsToLoginPageNotFriendlyError() { + // Navigate to the callback with a state value UAA never generated for this session. + webDriver.get(baseUrl + "/login/callback/" + IDP_ORIGIN + "?code=fake-code&state=totally-forged-state-value"); + + // Must end up at /login, NOT at /oauth_error with the concurrent-login message. + new LoginPage(webDriver).assertThatLoginPageShown(); + } + + // ───────────────────────────────────────────────────────────────────────────── + // Helpers + // ───────────────────────────────────────────────────────────────────────────── + + private IdentityProvider registerTestOidcIdp() throws Exception { + OIDCIdentityProviderDefinition config = new OIDCIdentityProviderDefinition(); + // Point at the local UAA as the "external" IDP — the test never completes the actual + // auth flow, so the IDP endpoints only need to pass config validation. + config.setAuthUrl(new URL(baseUrl + "/oauth/authorize")); + config.setTokenUrl(new URL(baseUrl + "/oauth/token")); + config.setTokenKeyUrl(new URL(baseUrl + "/token_key")); + config.setIssuer(baseUrl + "/oauth/token"); + config.setUserInfoUrl(new URL(baseUrl + "/userinfo")); + config.setShowLinkText(true); + config.setLinkText(IDP_LINK_TEXT); + config.setSkipSslValidation(true); + config.setRelyingPartyId("identity"); + config.setRelyingPartySecret("identitysecret"); + config.setScopes(List.of("openid")); + + IdentityProvider provider = new IdentityProvider<>(); + provider.setName(IDP_LINK_TEXT); + provider.setOriginKey(IDP_ORIGIN); + provider.setIdentityZoneId(OriginKeys.UAA); + provider.setActive(true); + provider.setConfig(config); + + return IntegrationTestUtils.createOrUpdateProvider(adminToken, baseUrl, provider); + } + + /** + * Parses the {@code state} query-parameter value from a URL string. + */ + private static String extractStateFromUrl(String url) { + try { + String query = new URI(url).getQuery(); + if (query == null) { + return null; + } + for (String param : query.split("&")) { + if (param.startsWith("state=")) { + return param.substring("state=".length()); + } + } + } catch (Exception e) { + throw new IllegalArgumentException("Failed to parse state from URL: " + url, e); + } + return null; + } + + private void logout() { + try { + webDriver.get(baseUrl + "/logout.do"); + } catch (org.openqa.selenium.TimeoutException _) { + webDriver.get(baseUrl + "/logout.do"); + } + webDriver.manage().deleteAllCookies(); + } +} diff --git a/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/pageObjects/OAuthErrorPage.java b/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/pageObjects/OAuthErrorPage.java new file mode 100644 index 00000000000..8cfd686c786 --- /dev/null +++ b/uaa/src/test/java/org/cloudfoundry/identity/uaa/integration/pageObjects/OAuthErrorPage.java @@ -0,0 +1,54 @@ +package org.cloudfoundry.identity.uaa.integration.pageObjects; + +import org.openqa.selenium.By; +import org.openqa.selenium.WebDriver; + +/** + * The OAuthErrorPage class represents the external OAuth authentication error page on the UAA server. + * It has url matching: {@code /oauth_error}. + */ +public class OAuthErrorPage extends Page { + + private static final String URL_PATH = "/oauth_error"; + + public OAuthErrorPage(WebDriver driver) { + super(driver); + assertThatUrlEventuallySatisfies(assertUrl -> assertUrl.contains(URL_PATH)); + } + + /** + * Asserts that the page displays the user-friendly "concurrent login" message shown when + * the state parameter from an older browser tab's IDP callback no longer matches the + * session's current state (because a newer login attempt replaced it). + */ + public OAuthErrorPage assertConcurrentLoginMessageShown() { + assertThatPageSource().contains("Another sign-in is already in progress"); + return this; + } + + /** + * Asserts that the page provides a link to restart the login flow. + */ + public OAuthErrorPage assertRestartLoginLinkPresent() { + assertThatPageSource().contains("Start a new login"); + return this; + } + + /** + * Asserts that the page does NOT display the concurrent-login-specific message. + * Use this to verify that a stale-state replay or a forged state lands on the generic + * error path rather than the friendly concurrent-login path. + */ + public OAuthErrorPage assertConcurrentLoginMessageNotShown() { + assertThatPageSource().doesNotContain("Another sign-in is already in progress"); + return this; + } + + /** + * Clicks the "Start a new login" link and returns the resulting LoginPage. + */ + public LoginPage clickStartNewLogin(String baseUrl) { + driver.findElement(By.linkText("Start a new login")).click(); + return new LoginPage(driver, baseUrl); + } +} From 657ca1956f53e790bf854c7a790c5b403ad29eb3 Mon Sep 17 00:00:00 2001 From: Filip Hanik Date: Fri, 19 Jun 2026 09:42:34 -0700 Subject: [PATCH 2/6] Add MockMvc tests for new concurrent login error page --- .../identity/uaa/login/LoginMockMvcTests.java | 92 +++++++++++++++++++ .../uaa/login/LoginMockMvcZonePathTests.java | 92 ++++++++++++++++++- 2 files changed, 183 insertions(+), 1 deletion(-) diff --git a/uaa/src/test/java/org/cloudfoundry/identity/uaa/login/LoginMockMvcTests.java b/uaa/src/test/java/org/cloudfoundry/identity/uaa/login/LoginMockMvcTests.java index 535efb049df..5b3461acc1a 100644 --- a/uaa/src/test/java/org/cloudfoundry/identity/uaa/login/LoginMockMvcTests.java +++ b/uaa/src/test/java/org/cloudfoundry/identity/uaa/login/LoginMockMvcTests.java @@ -1521,6 +1521,98 @@ void oauthRedirect_stateParameterPassedGetsReturned() throws Exception { IdentityZoneHolder.clear(); } + @Test + void oauthCallback_withForgedState_redirectsToLogin() throws Exception { + UaaClientDetails zoneAdminClient = new UaaClientDetails("admin", null, "openid", "client_credentials,authorization_code", "clients.admin,scim.read,scim.write", "http://test.redirect.com"); + zoneAdminClient.setClientSecret("admin-secret"); + IdentityZoneCreationResult identityZoneCreationResult = MockMvcUtils.createOtherIdentityZoneAndReturnResult("puppy-" + new RandomValueStringGenerator().generate(), mockMvc, webApplicationContext, zoneAdminClient, false, IdentityZoneHolder.getCurrentZoneId()); + IdentityZone identityZone = identityZoneCreationResult.getIdentityZone(); + String oauthAlias = createOIDCProviderInZone(jdbcIdentityProviderProvisioning, identityZone, null); + IdentityZoneHolder.set(identityZone); + IdentityProvider uaaIdentityProvider = jdbcIdentityProviderProvisioning.retrieveByOriginIgnoreActiveFlag(UAA, identityZone.getId()); + uaaIdentityProvider.setActive(false); + jdbcIdentityProviderProvisioning.update(uaaIdentityProvider, uaaIdentityProvider.getIdentityZoneId()); + + MvcResult loginResult = mockMvc.perform(get("/login").accept(TEXT_HTML) + .servletPath("/login") + .with(new SetServerNameRequestPostProcessor(identityZone.getSubdomain() + ".localhost"))) + .andExpect(status().isFound()) + .andReturn(); + MockHttpSession session = (MockHttpSession) loginResult.getRequest().getSession(false); + + mockMvc.perform(get("/login/callback/" + oauthAlias) + .param("code", "some-code") + .param("state", "forged-state-never-issued-by-uaa") + .session(session) + .servletPath("/login/callback/" + oauthAlias) + .with(new SetServerNameRequestPostProcessor(identityZone.getSubdomain() + ".localhost"))) + .andExpect(status().isFound()) + .andExpect(redirectedUrl("/login?error=invalid_login_request")); + + IdentityZoneHolder.clear(); + } + + @Test + void oauthCallback_withSupersededState_redirectsToConcurrentLoginError() throws Exception { + UaaClientDetails zoneAdminClient = new UaaClientDetails("admin", null, "openid", "client_credentials,authorization_code", "clients.admin,scim.read,scim.write", "http://test.redirect.com"); + zoneAdminClient.setClientSecret("admin-secret"); + IdentityZoneCreationResult identityZoneCreationResult = MockMvcUtils.createOtherIdentityZoneAndReturnResult("puppy-" + new RandomValueStringGenerator().generate(), mockMvc, webApplicationContext, zoneAdminClient, false, IdentityZoneHolder.getCurrentZoneId()); + IdentityZone identityZone = identityZoneCreationResult.getIdentityZone(); + String oauthAlias = createOIDCProviderInZone(jdbcIdentityProviderProvisioning, identityZone, null); + IdentityZoneHolder.set(identityZone); + IdentityProvider uaaIdentityProvider = jdbcIdentityProviderProvisioning.retrieveByOriginIgnoreActiveFlag(UAA, identityZone.getId()); + uaaIdentityProvider.setActive(false); + jdbcIdentityProviderProvisioning.update(uaaIdentityProvider, uaaIdentityProvider.getIdentityZoneId()); + + SetServerNameRequestPostProcessor zone = new SetServerNameRequestPostProcessor(identityZone.getSubdomain() + ".localhost"); + + // Tab 1 starts login — state1 stored in session + MvcResult tab1Login = mockMvc.perform(get("/login").accept(TEXT_HTML) + .servletPath("/login") + .with(zone)) + .andExpect(status().isFound()) + .andReturn(); + MockHttpSession session = (MockHttpSession) tab1Login.getRequest().getSession(false); + String state1 = UriComponentsBuilder.fromUriString(tab1Login.getResponse().getHeader("Location")) + .build().getQueryParams().getFirst("state"); + + // Tab 2 starts login with same session — state2 overwrites state1; state1 moved to superseded list + mockMvc.perform(get("/login").accept(TEXT_HTML) + .servletPath("/login") + .session(session) + .with(zone)) + .andExpect(status().isFound()); + + // Tab 1's callback arrives with stale state1 — concurrent login detected + mockMvc.perform(get("/login/callback/" + oauthAlias) + .param("code", "some-code") + .param("state", state1) + .session(session) + .servletPath("/login/callback/" + oauthAlias) + .with(zone)) + .andExpect(status().isFound()) + .andExpect(redirectedUrl("/oauth_error?reason=concurrent_login")); + + IdentityZoneHolder.clear(); + } + + @Test + void oauthErrorPage_withConcurrentLoginReason_showsFriendlyMessageAndRestartLink() throws Exception { + UaaClientDetails zoneAdminClient = new UaaClientDetails("admin", null, "openid", "client_credentials,authorization_code", "clients.admin,scim.read,scim.write", "http://test.redirect.com"); + zoneAdminClient.setClientSecret("admin-secret"); + IdentityZoneCreationResult identityZoneCreationResult = MockMvcUtils.createOtherIdentityZoneAndReturnResult("puppy-" + new RandomValueStringGenerator().generate(), mockMvc, webApplicationContext, zoneAdminClient, false, IdentityZoneHolder.getCurrentZoneId()); + IdentityZone identityZone = identityZoneCreationResult.getIdentityZone(); + + mockMvc.perform(get("/oauth_error") + .param("reason", "concurrent_login") + .accept(TEXT_HTML) + .servletPath("/oauth_error") + .with(new SetServerNameRequestPostProcessor(identityZone.getSubdomain() + ".localhost"))) + .andExpect(status().isOk()) + .andExpect(content().string(containsString("Another sign-in is already in progress"))) + .andExpect(content().string(containsString("Start a new login"))); + } + @Test void loginHintRedirect() throws Exception { final String zoneAdminClientId = "admin"; diff --git a/uaa/src/test/java/org/cloudfoundry/identity/uaa/login/LoginMockMvcZonePathTests.java b/uaa/src/test/java/org/cloudfoundry/identity/uaa/login/LoginMockMvcZonePathTests.java index ceb93eabc09..6549f0ddfbb 100644 --- a/uaa/src/test/java/org/cloudfoundry/identity/uaa/login/LoginMockMvcZonePathTests.java +++ b/uaa/src/test/java/org/cloudfoundry/identity/uaa/login/LoginMockMvcZonePathTests.java @@ -35,7 +35,6 @@ import org.cloudfoundry.identity.uaa.util.SessionUtils; import org.cloudfoundry.identity.uaa.web.UaaSavedRequestCache; import org.cloudfoundry.identity.uaa.mock.util.ZoneResolutionMode; -import org.hamcrest.CoreMatchers; import org.junit.jupiter.api.Disabled; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.EnumSource; @@ -1889,6 +1888,97 @@ void oauthRedirect_stateParameterPassedGetsReturned(ZoneResolutionMode mode) thr IdentityZoneHolder.clear(); } + @ParameterizedTest + @EnumSource(ZoneResolutionMode.class) + void oauthCallback_withForgedState_redirectsToLogin(ZoneResolutionMode mode) throws Exception { + UaaClientDetails zoneAdminClient = new UaaClientDetails("admin", null, "openid", "client_credentials,authorization_code", "clients.admin,scim.read,scim.write", "http://test.redirect.com"); + zoneAdminClient.setClientSecret("admin-secret"); + IdentityZoneCreationResult identityZoneCreationResult = MockMvcUtils.createOtherIdentityZoneAndReturnResult("puppy-" + new RandomValueStringGenerator().generate(), mockMvc, webApplicationContext, zoneAdminClient, false, IdentityZoneHolder.getCurrentZoneId()); + IdentityZone identityZone = identityZoneCreationResult.getIdentityZone(); + String oauthAlias = createOIDCProviderInZone(jdbcIdentityProviderProvisioning, identityZone, null); + IdentityZoneHolder.set(identityZone); + IdentityProvider uaaIdentityProvider = jdbcIdentityProviderProvisioning.retrieveByOriginIgnoreActiveFlag(UAA, identityZone.getId()); + uaaIdentityProvider.setActive(false); + jdbcIdentityProviderProvisioning.update(uaaIdentityProvider, uaaIdentityProvider.getIdentityZoneId()); + + String subdomain = identityZone.getSubdomain(); + String expectedLoginRedirect = mode == ZoneResolutionMode.ZONE_PATH ? "/z/" + subdomain + "/login" : "/login"; + + MvcResult loginResult = mockMvc.perform(mode.createRequestBuilder(subdomain, HttpMethod.GET, "/login") + .accept(TEXT_HTML)) + .andExpect(status().isFound()) + .andReturn(); + MockHttpSession session = (MockHttpSession) loginResult.getRequest().getSession(false); + + mockMvc.perform(mode.createRequestBuilder(subdomain, HttpMethod.GET, "/login/callback/" + oauthAlias) + .param("code", "some-code") + .param("state", "forged-state-never-issued-by-uaa") + .session(session)) + .andExpect(status().isFound()) + .andExpect(redirectedUrl(expectedLoginRedirect + "?error=invalid_login_request")); + + IdentityZoneHolder.clear(); + } + + @ParameterizedTest + @EnumSource(ZoneResolutionMode.class) + void oauthCallback_withSupersededState_redirectsToConcurrentLoginError(ZoneResolutionMode mode) throws Exception { + UaaClientDetails zoneAdminClient = new UaaClientDetails("admin", null, "openid", "client_credentials,authorization_code", "clients.admin,scim.read,scim.write", "http://test.redirect.com"); + zoneAdminClient.setClientSecret("admin-secret"); + IdentityZoneCreationResult identityZoneCreationResult = MockMvcUtils.createOtherIdentityZoneAndReturnResult("puppy-" + new RandomValueStringGenerator().generate(), mockMvc, webApplicationContext, zoneAdminClient, false, IdentityZoneHolder.getCurrentZoneId()); + IdentityZone identityZone = identityZoneCreationResult.getIdentityZone(); + String oauthAlias = createOIDCProviderInZone(jdbcIdentityProviderProvisioning, identityZone, null); + IdentityZoneHolder.set(identityZone); + IdentityProvider uaaIdentityProvider = jdbcIdentityProviderProvisioning.retrieveByOriginIgnoreActiveFlag(UAA, identityZone.getId()); + uaaIdentityProvider.setActive(false); + jdbcIdentityProviderProvisioning.update(uaaIdentityProvider, uaaIdentityProvider.getIdentityZoneId()); + + String subdomain = identityZone.getSubdomain(); + String expectedOauthErrorRedirect = mode == ZoneResolutionMode.ZONE_PATH ? "/z/" + subdomain + "/oauth_error" : "/oauth_error"; + + // Tab 1 starts login — state1 stored in session + MvcResult tab1Login = mockMvc.perform(mode.createRequestBuilder(subdomain, HttpMethod.GET, "/login") + .accept(TEXT_HTML)) + .andExpect(status().isFound()) + .andReturn(); + MockHttpSession session = (MockHttpSession) tab1Login.getRequest().getSession(false); + String state1 = UriComponentsBuilder.fromUriString(tab1Login.getResponse().getHeader("Location")) + .build().getQueryParams().getFirst("state"); + + // Tab 2 starts login with same session — state2 overwrites state1; state1 moved to superseded list + mockMvc.perform(mode.createRequestBuilder(subdomain, HttpMethod.GET, "/login") + .accept(TEXT_HTML) + .session(session)) + .andExpect(status().isFound()); + + // Tab 1's callback arrives with stale state1 — concurrent login detected + mockMvc.perform(mode.createRequestBuilder(subdomain, HttpMethod.GET, "/login/callback/" + oauthAlias) + .param("code", "some-code") + .param("state", state1) + .session(session)) + .andExpect(status().isFound()) + .andExpect(redirectedUrl(expectedOauthErrorRedirect + "?reason=concurrent_login")); + + IdentityZoneHolder.clear(); + } + + @ParameterizedTest + @EnumSource(ZoneResolutionMode.class) + void oauthErrorPage_withConcurrentLoginReason_showsFriendlyMessageAndRestartLink(ZoneResolutionMode mode) throws Exception { + UaaClientDetails zoneAdminClient = new UaaClientDetails("admin", null, "openid", "client_credentials,authorization_code", "clients.admin,scim.read,scim.write", "http://test.redirect.com"); + zoneAdminClient.setClientSecret("admin-secret"); + IdentityZoneCreationResult identityZoneCreationResult = MockMvcUtils.createOtherIdentityZoneAndReturnResult("puppy-" + new RandomValueStringGenerator().generate(), mockMvc, webApplicationContext, zoneAdminClient, false, IdentityZoneHolder.getCurrentZoneId()); + IdentityZone identityZone = identityZoneCreationResult.getIdentityZone(); + String subdomain = identityZone.getSubdomain(); + + mockMvc.perform(mode.createRequestBuilder(subdomain, HttpMethod.GET, "/oauth_error") + .param("reason", "concurrent_login") + .accept(TEXT_HTML)) + .andExpect(status().isOk()) + .andExpect(content().string(containsString("Another sign-in is already in progress"))) + .andExpect(content().string(containsString("Start a new login"))); + } + @Disabled("ZONE_PATH: redirect URL assertion differs for zone path") @ParameterizedTest @EnumSource(ZoneResolutionMode.class) From 1067e00925a82a3703d37b9f55c1ad073650930b Mon Sep 17 00:00:00 2001 From: Filip Hanik Date: Fri, 19 Jun 2026 09:58:16 -0700 Subject: [PATCH 3/6] Feedback from Copilot. Ensure the error page is not ambigious displaying two messages --- .../resources/templates/web/external_auth_error.html | 2 +- .../identity/uaa/login/HomeControllerViewTests.java | 10 ++++++++++ 2 files changed, 11 insertions(+), 1 deletion(-) diff --git a/server/src/main/resources/templates/web/external_auth_error.html b/server/src/main/resources/templates/web/external_auth_error.html index 1dad36181de..a5d74e76182 100644 --- a/server/src/main/resources/templates/web/external_auth_error.html +++ b/server/src/main/resources/templates/web/external_auth_error.html @@ -32,7 +32,7 @@

Another sign-in is already in progress

-

+

diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/login/HomeControllerViewTests.java b/server/src/test/java/org/cloudfoundry/identity/uaa/login/HomeControllerViewTests.java index 83230b3cb26..eeba94afcf6 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/login/HomeControllerViewTests.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/login/HomeControllerViewTests.java @@ -191,6 +191,16 @@ void oauthError_withConcurrentLoginReason_doesNotShowGenericOAuthError() throws .andExpect(content().string(org.hamcrest.Matchers.not(containsString("There was an error when authenticating")))); } + @Test + void oauthError_withConcurrentLoginReasonAndLeftoverSessionError_suppressesGenericOAuthError() throws Exception { + mockMvc.perform(get("/oauth_error") + .param("reason", "concurrent_login") + .sessionAttr("oauth_error", "There was an error when authenticating against the external identity provider: leftover")) + .andExpect(status().isOk()) + .andExpect(content().string(containsString("Another sign-in is already in progress"))) + .andExpect(content().string(org.hamcrest.Matchers.not(containsString("leftover")))); + } + @Test void oauthError_withoutConcurrentLoginReason_doesNotShowFriendlyMessage() throws Exception { mockMvc.perform(get("/oauth_error")) From ef7fe94690950bf98cb1aad12ff00ac100af3337 Mon Sep 17 00:00:00 2001 From: strehle Date: Fri, 19 Jun 2026 19:14:49 +0200 Subject: [PATCH 4/6] fix: reject external OAuth callback with missing session as invalid login Use getSession(false) in the callback state check so an expired/missing session is rejected as an invalid login request instead of silently creating a new stateless session, and handle HttpSessionRequiredException in the filter alongside CsrfException. Add a security-monitoring log for rejected callbacks and update the affected unit tests. Co-authored-by: Cursor --- .../ExternalOAuthAuthenticationFilter.java | 11 +++++++++-- .../ExternalOAuthAuthenticationFilterTest.java | 14 ++++++++------ ...ternalOAuthConcurrentLoginScenarioTest.java | 18 +++++++++++------- 3 files changed, 28 insertions(+), 15 deletions(-) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthAuthenticationFilter.java b/server/src/main/java/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthAuthenticationFilter.java index 385ab094abc..fb53fc7de4f 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthAuthenticationFilter.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthAuthenticationFilter.java @@ -66,7 +66,12 @@ public void doFilter( logger.warn("Concurrent login attempt detected for origin [{}]; a newer login superseded this one", ex.getOriginKey()); response.sendRedirect(request.getContextPath() + "/oauth_error?reason=concurrent_login"); return; - } catch (CsrfException ex) { + } catch (CsrfException | HttpSessionRequiredException ex) { + // No existing session, or no matching state: reject as an invalid login request without + // authenticating. A genuine state mismatch here may be a CSRF/tampering attempt, so keep + // a signal for security monitoring. + logger.warn("Rejecting external OAuth callback for origin [{}] as an invalid login request: {}", + UaaUrlUtils.extractPathVariableFromUrl(2, request.getServletPath()), ex.getMessage()); response.sendRedirect(request.getContextPath() + "/login?error=invalid_login_request"); return; } @@ -79,7 +84,9 @@ public void doFilter( private void checkRequestStateParameter(final HttpServletRequest request) throws HttpSessionRequiredException { final String originKey = UaaUrlUtils.extractPathVariableFromUrl(2, request.getServletPath()); - final HttpSession session = request.getSession(); + // Use getSession(false): a callback with an expired/missing session must be rejected as an + // invalid login request, not silently given a fresh (stateless) session. + final HttpSession session = request.getSession(false); if (session == null) { throw new HttpSessionRequiredException("An HTTP Session is required to process request."); } diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthAuthenticationFilterTest.java b/server/src/test/java/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthAuthenticationFilterTest.java index 8917bcaab21..fc1b021329b 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthAuthenticationFilterTest.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthAuthenticationFilterTest.java @@ -1,6 +1,5 @@ package org.cloudfoundry.identity.uaa.provider.oauth; -import org.assertj.core.api.InstanceOfAssertFactories; import org.cloudfoundry.identity.uaa.extensions.PollutionPreventionExtension; import org.cloudfoundry.identity.uaa.login.AccountSavingAuthenticationSuccessHandler; import org.cloudfoundry.identity.uaa.util.SessionUtils; @@ -10,7 +9,6 @@ import org.junit.jupiter.api.extension.ExtendWith; import org.springframework.security.authentication.BadCredentialsException; import org.springframework.security.core.Authentication; -import org.springframework.web.HttpSessionRequiredException; import jakarta.servlet.FilterChain; import jakarta.servlet.RequestDispatcher; @@ -21,7 +19,6 @@ import java.util.List; import java.util.function.Consumer; -import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; @@ -115,15 +112,19 @@ void setUp() { } @Test - void itThrowsIfNoSession() throws Exception { + void itRedirectsIfNoSession() throws Exception { + // No existing session (getSession(false) returns null) must be rejected as an invalid + // login request without creating a new session. HttpServletRequest mockRequest = mockRedirectRequest(false, ORIGIN_KEY, request -> { mockAuthenticationInRequest(request); mockStateParamInRequest(request, OAUTH_STATE); }); HttpServletResponse mockResponse = mock(HttpServletResponse.class); - assertThatThrownBy(() -> - externalOAuthAuthenticationFilter.doFilter(mockRequest, mockResponse, mockFilterChain)).asInstanceOf(InstanceOfAssertFactories.throwable(HttpSessionRequiredException.class)); + externalOAuthAuthenticationFilter.doFilter(mockRequest, mockResponse, mockFilterChain); + + verify(mockRequest, never()).getSession(); + verify(mockResponse).sendRedirect("/uaa/login?error=invalid_login_request"); verify(mockFilterChain, never()).doFilter(mockRequest, mockResponse); } @@ -272,6 +273,7 @@ private HttpServletRequest mockRedirectRequest(boolean includeSession, String or if (includeSession) { HttpSession mockHttpSession = mock(HttpSession.class); when(mockRequest.getSession()).thenReturn(mockHttpSession); + when(mockRequest.getSession(false)).thenReturn(mockHttpSession); } config.accept(mockRequest); diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthConcurrentLoginScenarioTest.java b/server/src/test/java/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthConcurrentLoginScenarioTest.java index 9351f3ad558..28590129378 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthConcurrentLoginScenarioTest.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthConcurrentLoginScenarioTest.java @@ -11,7 +11,6 @@ import jakarta.servlet.http.HttpServletResponse; import static org.assertj.core.api.Assertions.assertThat; -import static org.assertj.core.api.Assertions.assertThatThrownBy; import static org.mockito.ArgumentMatchers.any; import static org.mockito.Mockito.mock; import static org.mockito.Mockito.never; @@ -30,8 +29,8 @@ *

Scenarios covered

*
    *
  1. Happy path – a single tab completes login; state matches; filter chain continues.
    2. - *
  2. No session – session has been destroyed (e.g. timeout); filter throws - * {@link org.springframework.web.HttpSessionRequiredException}.
    3. + *
  3. No session – session has been destroyed (e.g. timeout); filter redirects to + * {@code /login?error=invalid_login_request} without creating a new session.
    4. *
  4. No state in session – CSRF / replay attempt; filter redirects to * {@code /login?error=invalid_login_request}.
    5. *
  5. No state in callback request – malformed or tampered callback; filter redirects to @@ -84,12 +83,17 @@ void scenario_singleTab_stateMatches_filterChainContinues() throws Exception { // ------------------------------------------------------------------------- @Test - void scenario_noSession_throwsHttpSessionRequiredException() { + void scenario_noSession_redirectsToLogin() throws Exception { HttpServletRequest req = buildRequestWithNoSession(STATE_FROM_TAB_1); HttpServletResponse res = mock(HttpServletResponse.class); - assertThatThrownBy(() -> filter.doFilter(req, res, mockChain)) - .isInstanceOf(org.springframework.web.HttpSessionRequiredException.class); + filter.doFilter(req, res, mockChain); + + // A missing/expired session must be rejected as an invalid login request without creating + // a new session (the filter uses getSession(false)). + verify(req, never()).getSession(); + verify(res).sendRedirect("/uaa/login?error=invalid_login_request"); + verify(mockChain, never()).doFilter(any(), any()); } // ------------------------------------------------------------------------- @@ -256,7 +260,7 @@ private HttpServletRequest buildRequestWithNoSession(String stateInCallback) { when(req.getRequestURL()).thenReturn(new StringBuffer("http://localhost/uaa/login/callback/" + IDP_ORIGIN)); when(req.getParameter("code")).thenReturn("some-auth-code"); when(req.getParameter("state")).thenReturn(stateInCallback); - when(req.getSession()).thenReturn(null); + when(req.getSession(false)).thenReturn(null); return req; } From 0059cdbc878792b7b3f08d3eaece9417d35ae8f3 Mon Sep 17 00:00:00 2001 From: Filip Hanik Date: Fri, 19 Jun 2026 11:09:41 -0700 Subject: [PATCH 5/6] Final fixes. Correctly extract the IDP alias in all zone scenarios --- .../ExternalOAuthAuthenticationFilter.java | 21 +++++++++++++++++-- .../templates/web/external_auth_error.html | 2 +- ...ExternalOAuthAuthenticationFilterTest.java | 6 +++--- 3 files changed, 23 insertions(+), 6 deletions(-) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthAuthenticationFilter.java b/server/src/main/java/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthAuthenticationFilter.java index fb53fc7de4f..955fc33277d 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthAuthenticationFilter.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthAuthenticationFilter.java @@ -71,7 +71,7 @@ public void doFilter( // authenticating. A genuine state mismatch here may be a CSRF/tampering attempt, so keep // a signal for security monitoring. logger.warn("Rejecting external OAuth callback for origin [{}] as an invalid login request: {}", - UaaUrlUtils.extractPathVariableFromUrl(2, request.getServletPath()), ex.getMessage()); + UaaUrlUtils.extractPathVariableFromUrl(2, pathAfterContext(request)), ex.getMessage()); response.sendRedirect(request.getContextPath() + "/login?error=invalid_login_request"); return; } @@ -83,7 +83,10 @@ public void doFilter( private void checkRequestStateParameter(final HttpServletRequest request) throws HttpSessionRequiredException { - final String originKey = UaaUrlUtils.extractPathVariableFromUrl(2, request.getServletPath()); + // Strip the context path from the request URI before extracting the origin key so that both + // subdomain-based and zone-path-based deployments resolve the same path index. getServletPath() + // is unreliable in subdomain mode where the MockMvc harness does not set it explicitly. + final String originKey = UaaUrlUtils.extractPathVariableFromUrl(2, pathAfterContext(request)); // Use getSession(false): a callback with an expired/missing session must be rejected as an // invalid login request, not silently given a fresh (stateless) session. final HttpSession session = request.getSession(false); @@ -100,6 +103,20 @@ private void checkRequestStateParameter(final HttpServletRequest request) } } + /** + * Returns the portion of the request URI that follows the context path. + * Using the request URI (minus context path) rather than getServletPath() ensures correct + * origin extraction in subdomain-based deployments where the servlet path may not be set. + */ + private static String pathAfterContext(HttpServletRequest request) { + String requestUri = request.getRequestURI(); + String contextPath = request.getContextPath(); + if (contextPath != null && !contextPath.isEmpty() && requestUri.startsWith(contextPath)) { + return requestUri.substring(contextPath.length()); + } + return requestUri; + } + private boolean containsCredentials(final HttpServletRequest request) { final String code = request.getParameter("code"); final String idToken = request.getParameter("id_token"); diff --git a/server/src/main/resources/templates/web/external_auth_error.html b/server/src/main/resources/templates/web/external_auth_error.html index a5d74e76182..00707c93df5 100644 --- a/server/src/main/resources/templates/web/external_auth_error.html +++ b/server/src/main/resources/templates/web/external_auth_error.html @@ -32,7 +32,7 @@

    Another sign-in is already in progress

    -

    +

    diff --git a/server/src/test/java/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthAuthenticationFilterTest.java b/server/src/test/java/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthAuthenticationFilterTest.java index fc1b021329b..d872b157879 100644 --- a/server/src/test/java/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthAuthenticationFilterTest.java +++ b/server/src/test/java/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthAuthenticationFilterTest.java @@ -129,7 +129,7 @@ void itRedirectsIfNoSession() throws Exception { } @Test - void itThrowsIfNoStateInSession() throws Exception { + void itRedirectsToInvalidLoginWhenNoStateInSession() throws Exception { HttpServletRequest mockRequest = mockRedirectRequest(ORIGIN_KEY, request -> { mockAuthenticationInRequest(request); mockStateParamInRequest(request, OAUTH_STATE); @@ -143,7 +143,7 @@ void itThrowsIfNoStateInSession() throws Exception { } @Test - void itThrowsIfNoStateInRequest() throws Exception { + void itRedirectsToInvalidLoginWhenNoStateInRequest() throws Exception { HttpServletRequest mockRequest = mockRedirectRequest(ORIGIN_KEY, request -> { mockAuthenticationInRequest(request); mockStateParamInSession(request.getSession(), ORIGIN_KEY, OAUTH_STATE); @@ -157,7 +157,7 @@ void itThrowsIfNoStateInRequest() throws Exception { } @Test - void itThrowsIfStateIsMismatched() throws Exception { + void itRedirectsToInvalidLoginWhenStateIsMismatched() throws Exception { HttpServletRequest mockRequest = mockRedirectRequest(ORIGIN_KEY, request -> { mockAuthenticationInRequest(request); mockStateParamInRequest(request, "surprise"); From f45cf9267866899fcfb3bd559c35397fd8b4b1a5 Mon Sep 17 00:00:00 2001 From: strehle Date: Sun, 21 Jun 2026 10:13:09 +0200 Subject: [PATCH 6/6] origin fix --- .../provider/oauth/ExternalOAuthAuthenticationFilter.java | 6 ++++-- 1 file changed, 4 insertions(+), 2 deletions(-) diff --git a/server/src/main/java/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthAuthenticationFilter.java b/server/src/main/java/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthAuthenticationFilter.java index 955fc33277d..e8778e6b4a3 100644 --- a/server/src/main/java/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthAuthenticationFilter.java +++ b/server/src/main/java/org/cloudfoundry/identity/uaa/provider/oauth/ExternalOAuthAuthenticationFilter.java @@ -1,6 +1,5 @@ package org.cloudfoundry.identity.uaa.provider.oauth; -import org.apache.commons.io.FilenameUtils; import org.cloudfoundry.identity.uaa.authentication.UaaAuthenticationDetails; import org.cloudfoundry.identity.uaa.login.AccountSavingAuthenticationSuccessHandler; import org.cloudfoundry.identity.uaa.util.SessionUtils; @@ -128,7 +127,10 @@ private boolean containsCredentials(final HttpServletRequest request) { private boolean authenticationWasSuccessful( final HttpServletRequest request, final HttpServletResponse response) throws IOException { - final String origin = FilenameUtils.getName(request.getRequestURI()); + // Derive the origin the same way as checkRequestStateParameter so the state we validated and + // the IDP we authenticate against always refer to the same origin key (robust across + // subdomain- and zone-path-based deployments). + final String origin = UaaUrlUtils.extractPathVariableFromUrl(2, pathAfterContext(request)); final String code = request.getParameter("code"); final String idToken = request.getParameter("id_token"); final String accessToken = request.getParameter("access_token");