Polish SecurityFilterChain Validation

Closes gh-15982

Author: franticticktick

config/src/main/java/org/springframework/security/config/http/DefaultFilterChainValidator.java

@@ -39,6 +39,7 @@
 import org.springframework.security.web.FilterChainProxy;
 import org.springframework.security.web.FilterInvocation;
 import org.springframework.security.web.SecurityFilterChain;
+import org.springframework.security.web.UnreachableFilterChainException;
 import org.springframework.security.web.access.ExceptionTranslationFilter;
 import org.springframework.security.web.access.intercept.AuthorizationFilter;
 import org.springframework.security.web.access.intercept.FilterInvocationSecurityMetadataSource;
@@ -53,9 +54,8 @@
 import org.springframework.security.web.servletapi.SecurityContextHolderAwareRequestFilter;
 import org.springframework.security.web.session.SessionManagementFilter;
 import org.springframework.security.web.util.matcher.AnyRequestMatcher;
-import org.springframework.security.web.util.matcher.RequestMatcher;
 
-public class DefaultFilterChainValidator implements FilterChainProxy.FilterChainValidator {
+public final class DefaultFilterChainValidator implements FilterChainProxy.FilterChainValidator {
 
 	private static final Authentication TEST = new TestingAuthenticationToken("", "", Collections.emptyList());
 
@@ -75,25 +75,33 @@ private void checkPathOrder(List<SecurityFilterChain> filterChains) {
 		// Check that the universal pattern is listed at the end, if at all
 		Iterator<SecurityFilterChain> chains = filterChains.iterator();
 		while (chains.hasNext()) {
-			RequestMatcher matcher = ((DefaultSecurityFilterChain) chains.next()).getRequestMatcher();
-			if (AnyRequestMatcher.INSTANCE.equals(matcher) && chains.hasNext()) {
-				throw new IllegalArgumentException("A universal match pattern ('/**') is defined "
-						+ " before other patterns in the filter chain, causing them to be ignored. Please check the "
-						+ "ordering in your <security:http> namespace or FilterChainProxy bean configuration");
+			if (chains.next() instanceof DefaultSecurityFilterChain securityFilterChain) {
+				if (AnyRequestMatcher.INSTANCE.equals(securityFilterChain.getRequestMatcher()) && chains.hasNext()) {
+					throw new UnreachableFilterChainException("A universal match pattern ('/**') is defined "
+							+ " before other patterns in the filter chain, causing them to be ignored. Please check the "
+							+ "ordering in your <security:http> namespace or FilterChainProxy bean configuration");
+				}
 			}
 		}
 	}
 
 	private void checkForDuplicateMatchers(List<SecurityFilterChain> chains) {
-		while (chains.size() > 1) {
-			DefaultSecurityFilterChain chain = (DefaultSecurityFilterChain) chains.remove(0);
-			for (SecurityFilterChain test : chains) {
-				if (chain.getRequestMatcher().equals(((DefaultSecurityFilterChain) test).getRequestMatcher())) {
-					throw new IllegalArgumentException("The FilterChainProxy contains two filter chains using the"
-							+ " matcher " + chain.getRequestMatcher() + ". If you are using multiple <http> namespace "
-							+ "elements, you must use a 'pattern' attribute to define the request patterns to which they apply.");
+		DefaultSecurityFilterChain filterChain = null;
+		for (SecurityFilterChain chain : chains) {
+			if (filterChain != null) {
+				if (chain instanceof DefaultSecurityFilterChain defaultChain) {
+					if (defaultChain.getRequestMatcher().equals(filterChain.getRequestMatcher())) {
+						throw new UnreachableFilterChainException(
+								"The FilterChainProxy contains two filter chains using the" + " matcher "
+										+ defaultChain.getRequestMatcher()
+										+ ". If you are using multiple <http> namespace "
+										+ "elements, you must use a 'pattern' attribute to define the request patterns to which they apply.");
+					}
 				}
 			}
+			if (chain instanceof DefaultSecurityFilterChain defaultChain) {
+				filterChain = defaultChain;
+			}
 		}
 	}
 

config/src/test/java/org/springframework/security/config/http/DefaultFilterChainValidatorTests.java

@@ -16,7 +16,9 @@
 
 package org.springframework.security.config.http;
 
+import java.util.ArrayList;
 import java.util.Collection;
+import java.util.List;
 
 import jakarta.servlet.http.HttpServletRequest;
 import org.apache.commons.logging.Log;
@@ -33,16 +35,20 @@
 import org.springframework.security.web.AuthenticationEntryPoint;
 import org.springframework.security.web.DefaultSecurityFilterChain;
 import org.springframework.security.web.FilterChainProxy;
+import org.springframework.security.web.SecurityFilterChain;
+import org.springframework.security.web.UnreachableFilterChainException;
 import org.springframework.security.web.access.ExceptionTranslationFilter;
 import org.springframework.security.web.access.intercept.AuthorizationFilter;
 import org.springframework.security.web.access.intercept.DefaultFilterInvocationSecurityMetadataSource;
 import org.springframework.security.web.access.intercept.FilterInvocationSecurityMetadataSource;
 import org.springframework.security.web.access.intercept.FilterSecurityInterceptor;
 import org.springframework.security.web.authentication.AnonymousAuthenticationFilter;
 import org.springframework.security.web.authentication.LoginUrlAuthenticationEntryPoint;
+import org.springframework.security.web.util.matcher.AntPathRequestMatcher;
 import org.springframework.security.web.util.matcher.AnyRequestMatcher;
 import org.springframework.test.util.ReflectionTestUtils;
 
+import static org.assertj.core.api.Assertions.assertThatExceptionOfType;
 import static org.mockito.ArgumentMatchers.any;
 import static org.mockito.BDDMockito.given;
 import static org.mockito.BDDMockito.willThrow;
@@ -130,4 +136,21 @@ public void validateCustomMetadataSource() {
 		verify(customMetaDataSource, atLeastOnce()).getAttributes(any());
 	}
 
+	@Test
+	void validateWhenSameRequestMatchersArePresentThenUnreachableFilterChainException() {
+		AnonymousAuthenticationFilter authenticationFilter = mock(AnonymousAuthenticationFilter.class);
+		ExceptionTranslationFilter exceptionTranslationFilter = mock(ExceptionTranslationFilter.class);
+		SecurityFilterChain chain1 = new DefaultSecurityFilterChain(AntPathRequestMatcher.antMatcher("/api"),
+				authenticationFilter, exceptionTranslationFilter, this.authorizationInterceptor);
+		SecurityFilterChain chain2 = new DefaultSecurityFilterChain(AntPathRequestMatcher.antMatcher("/api"),
+				authenticationFilter, exceptionTranslationFilter, this.authorizationInterceptor);
+		List<SecurityFilterChain> chains = new ArrayList<>();
+		chains.add(chain2);
+		chains.add(chain1);
+		FilterChainProxy proxy = new FilterChainProxy(chains);
+
+		assertThatExceptionOfType(UnreachableFilterChainException.class)
+			.isThrownBy(() -> this.validator.validate(proxy));
+	}
+
 }

web/src/main/java/org/springframework/security/web/UnreachableFilterChainException.java

@@ -0,0 +1,36 @@
+/*
+ * Copyright 2002-2024 the original author or authors.
+ *
+ * Licensed under the Apache License, Version 2.0 (the "License");
+ * you may not use this file except in compliance with the License.
+ * You may obtain a copy of the License at
+ *
+ *      https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing, software
+ * distributed under the License is distributed on an "AS IS" BASIS,
+ * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+ * See the License for the specific language governing permissions and
+ * limitations under the License.
+ */
+
+package org.springframework.security.web;
+
+/**
+ * Thrown if {@link SecurityFilterChain securityFilterChain} is not valid
+ *
+ * @author Max Batischev
+ * @since 6.5
+ */
+public final class UnreachableFilterChainException extends RuntimeException {
+
+	/**
+	 * Constructs an <code>UnreachableFilterChainException</code> with the specified
+	 * message.
+	 * @param message the detail message
+	 */
+	public UnreachableFilterChainException(String message) {
+		super(message);
+	}
+
+}

web/src/main/java/org/springframework/security/web/util/matcher/OrRequestMatcher.java

@@ -1,5 +1,5 @@
 /*
- * Copyright 2002-2023 the original author or authors.
+ * Copyright 2002-2024 the original author or authors.
  *
  * Licensed under the Apache License, Version 2.0 (the "License");
  * you may not use this file except in compliance with the License.
@@ -18,6 +18,7 @@
 
 import java.util.Arrays;
 import java.util.List;
+import java.util.Objects;
 
 import jakarta.servlet.http.HttpServletRequest;
 
@@ -81,6 +82,23 @@ public MatchResult matcher(HttpServletRequest request) {
 		return MatchResult.notMatch();
 	}
 
+	@Override
+	public boolean equals(Object o) {
+		if (this == o) {
+			return true;
+		}
+		if (o == null || getClass() != o.getClass()) {
+			return false;
+		}
+		OrRequestMatcher that = (OrRequestMatcher) o;
+		return Objects.equals(this.requestMatchers, that.requestMatchers);
+	}
+
+	@Override
+	public int hashCode() {
+		return Objects.hash(this.requestMatchers);
+	}
+
 	@Override
 	public String toString() {
 		return "Or " + this.requestMatchers;