From d4eff73be21d4a1fb67f7c2f21a143a39123c60f Mon Sep 17 00:00:00 2001 From: Max Batischev Date: Tue, 17 Dec 2024 14:00:29 +0300 Subject: [PATCH] Polish SecurityFilterChain Validation Closes gh-15982 --- .../http/DefaultFilterChainValidator.java | 36 ++++++++----- .../DefaultFilterChainValidatorTests.java | 23 +++++++++ .../web/UnreachableFilterChainException.java | 51 +++++++++++++++++++ .../web/util/matcher/AndRequestMatcher.java | 20 +++++++- .../web/util/matcher/OrRequestMatcher.java | 20 +++++++- 5 files changed, 135 insertions(+), 15 deletions(-) create mode 100644 web/src/main/java/org/springframework/security/web/UnreachableFilterChainException.java diff --git a/config/src/main/java/org/springframework/security/config/http/DefaultFilterChainValidator.java b/config/src/main/java/org/springframework/security/config/http/DefaultFilterChainValidator.java index ce7c50be584..74b6fbb7cab 100644 --- a/config/src/main/java/org/springframework/security/config/http/DefaultFilterChainValidator.java +++ b/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,7 +54,6 @@ 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 { @@ -75,25 +75,35 @@ private void checkPathOrder(List filterChains) { // Check that the universal pattern is listed at the end, if at all Iterator 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 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 namespace or FilterChainProxy bean configuration", + securityFilterChain, chains.next()); + } } } } private void checkForDuplicateMatchers(List 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 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 namespace " + + "elements, you must use a 'pattern' attribute to define the request patterns to which they apply.", + defaultChain, chain); + } } } + if (chain instanceof DefaultSecurityFilterChain defaultChain) { + filterChain = defaultChain; + } } } diff --git a/config/src/test/java/org/springframework/security/config/http/DefaultFilterChainValidatorTests.java b/config/src/test/java/org/springframework/security/config/http/DefaultFilterChainValidatorTests.java index a5b899db48c..f120b9d0581 100644 --- a/config/src/test/java/org/springframework/security/config/http/DefaultFilterChainValidatorTests.java +++ b/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,6 +35,8 @@ 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; @@ -40,9 +44,11 @@ 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 chains = new ArrayList<>(); + chains.add(chain2); + chains.add(chain1); + FilterChainProxy proxy = new FilterChainProxy(chains); + + assertThatExceptionOfType(UnreachableFilterChainException.class) + .isThrownBy(() -> this.validator.validate(proxy)); + } + } diff --git a/web/src/main/java/org/springframework/security/web/UnreachableFilterChainException.java b/web/src/main/java/org/springframework/security/web/UnreachableFilterChainException.java new file mode 100644 index 00000000000..c9145ed213c --- /dev/null +++ b/web/src/main/java/org/springframework/security/web/UnreachableFilterChainException.java @@ -0,0 +1,51 @@ +/* + * 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 class UnreachableFilterChainException extends IllegalArgumentException { + + private final SecurityFilterChain filterChain; + + private final SecurityFilterChain unreachableFilterChain; + + /** + * Constructs an UnreachableFilterChainException with the specified + * message. + * @param message the detail message + */ + public UnreachableFilterChainException(String message, SecurityFilterChain filterChain, + SecurityFilterChain unreachableFilterChain) { + super(message); + this.filterChain = filterChain; + this.unreachableFilterChain = unreachableFilterChain; + } + + public SecurityFilterChain getFilterChain() { + return this.filterChain; + } + + public SecurityFilterChain getUnreachableFilterChain() { + return this.unreachableFilterChain; + } + +} diff --git a/web/src/main/java/org/springframework/security/web/util/matcher/AndRequestMatcher.java b/web/src/main/java/org/springframework/security/web/util/matcher/AndRequestMatcher.java index b28b69bbba2..b585db8eceb 100644 --- a/web/src/main/java/org/springframework/security/web/util/matcher/AndRequestMatcher.java +++ b/web/src/main/java/org/springframework/security/web/util/matcher/AndRequestMatcher.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. @@ -20,6 +20,7 @@ import java.util.LinkedHashMap; import java.util.List; import java.util.Map; +import java.util.Objects; import jakarta.servlet.http.HttpServletRequest; import org.apache.commons.logging.Log; @@ -90,6 +91,23 @@ public MatchResult matcher(HttpServletRequest request) { return MatchResult.match(variables); } + @Override + public boolean equals(Object o) { + if (this == o) { + return true; + } + if (o == null || getClass() != o.getClass()) { + return false; + } + AndRequestMatcher that = (AndRequestMatcher) o; + return Objects.equals(this.requestMatchers, that.requestMatchers); + } + + @Override + public int hashCode() { + return Objects.hash(this.requestMatchers); + } + @Override public String toString() { return "And " + this.requestMatchers; diff --git a/web/src/main/java/org/springframework/security/web/util/matcher/OrRequestMatcher.java b/web/src/main/java/org/springframework/security/web/util/matcher/OrRequestMatcher.java index e3add8edf3a..53c0af8d922 100644 --- a/web/src/main/java/org/springframework/security/web/util/matcher/OrRequestMatcher.java +++ b/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;