Pr/issue 3701

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;
     }
 

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;
+    }
+}

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;
@@ -60,7 +59,21 @@ 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 | 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, pathAfterContext(request)), ex.getMessage());
+            response.sendRedirect(request.getContextPath() + "/login?error=invalid_login_request");
+            return;
+        }
 
         if (authenticationWasSuccessful(request, response)) {
             chain.doFilter(request, response);
@@ -69,18 +82,40 @@ public void doFilter(
 
     private void checkRequestStateParameter(final HttpServletRequest request)
             throws HttpSessionRequiredException {
-        final String originKey = UaaUrlUtils.extractPathVariableFromUrl(2, request.getServletPath());
-        final HttpSession session = request.getSession();
+        // 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);
         if (session == null) {
             throw new HttpSessionRequiredException("An HTTP Session is required to process 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.");
         }
     }
 
+    /**
+     * 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");
@@ -92,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");

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));
 

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<String> supersededStates = existing instanceof Deque
+                ? (Deque<String>) 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<String> deque = (Deque<String>) 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;
+    }
 }

server/src/main/resources/templates/web/external_auth_error.html

@@ -24,9 +24,15 @@
         </div>
         <img src="/resources/images/sad_cloud.png" th:src="@{/resources/images/sad_cloud.png}" role="presentation" />
     </div>
+    <th:block th:if="${oauth_concurrent_login}">
+        <h2>Another sign-in is already in progress</h2>
+        <p>A sign-in attempt from another browser tab or window has replaced this one.
+            To complete sign-in, please start a new login.</p>
+        <a th:href="@{/login}">Start a new login</a>
+    </th:block>
     <h2 th:if="${saml_error}" th:text="${saml_error}">
     </h2>
-    <h2 th:if="${oauth_error}" th:text="${oauth_error}">
+    <h2 th:if="${oauth_error != null and oauth_concurrent_login == null}" th:text="${oauth_error}">
     </h2>
 </div>
 </body>

server/src/test/java/org/cloudfoundry/identity/uaa/login/HomeControllerViewTests.java

@@ -171,6 +171,43 @@ 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_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"))
+                .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")))