diff --git a/config/src/main/java/org/springframework/security/config/annotation/web/builders/WebSecurityFilterChainValidator.java b/config/src/main/java/org/springframework/security/config/annotation/web/builders/WebSecurityFilterChainValidator.java index cc11cdef400..b0bf43fb3b7 100644 --- a/config/src/main/java/org/springframework/security/config/annotation/web/builders/WebSecurityFilterChainValidator.java +++ b/config/src/main/java/org/springframework/security/config/annotation/web/builders/WebSecurityFilterChainValidator.java @@ -18,28 +18,45 @@ import java.util.List; +import jakarta.servlet.Filter; +import org.apache.commons.logging.Log; +import org.apache.commons.logging.LogFactory; + 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.intercept.AuthorizationFilter; +import org.springframework.security.web.access.intercept.FilterSecurityInterceptor; import org.springframework.security.web.util.matcher.AnyRequestMatcher; /** * A filter chain validator for filter chains built by {@link WebSecurity} * + * @author Josh Cummings + * @author Max Batischev * @since 6.5 */ final class WebSecurityFilterChainValidator implements FilterChainProxy.FilterChainValidator { + private final Log logger = LogFactory.getLog(getClass()); + @Override public void validate(FilterChainProxy filterChainProxy) { List chains = filterChainProxy.getFilterChains(); + checkForAnyRequestRequestMatcher(chains); + checkForDuplicateMatchers(chains); + checkAuthorizationFilters(chains); + } + + private void checkForAnyRequestRequestMatcher(List chains) { DefaultSecurityFilterChain anyRequestFilterChain = null; for (SecurityFilterChain chain : chains) { if (anyRequestFilterChain != null) { String message = "A filter chain that matches any request [" + anyRequestFilterChain + "] has already been configured, which means that this filter chain [" + chain + "] will never get invoked. Please use `HttpSecurity#securityMatcher` to ensure that there is only one filter chain configured for 'any request' and that the 'any request' filter chain is published last."; - throw new IllegalArgumentException(message); + throw new UnreachableFilterChainException(message, anyRequestFilterChain, chain); } if (chain instanceof DefaultSecurityFilterChain defaultChain) { if (defaultChain.getRequestMatcher() instanceof AnyRequestMatcher) { @@ -49,4 +66,48 @@ public void validate(FilterChainProxy filterChainProxy) { } } + private void checkForDuplicateMatchers(List chains) { + 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(), + filterChain, defaultChain); + } + } + } + if (chain instanceof DefaultSecurityFilterChain defaultChain) { + filterChain = defaultChain; + } + } + } + + private void checkAuthorizationFilters(List chains) { + Filter authorizationFilter = null; + Filter filterSecurityInterceptor = null; + for (SecurityFilterChain chain : chains) { + for (Filter filter : chain.getFilters()) { + if (filter instanceof AuthorizationFilter) { + authorizationFilter = filter; + } + if (filter instanceof FilterSecurityInterceptor) { + filterSecurityInterceptor = filter; + } + } + if (authorizationFilter != null && filterSecurityInterceptor != null) { + this.logger.warn( + "It is not recommended to use authorizeRequests in the configuration. Please only use authorizeHttpRequests"); + } + if (filterSecurityInterceptor != null) { + this.logger.warn( + "Usage of authorizeRequests is deprecated. Please use authorizeHttpRequests in the configuration"); + } + authorizationFilter = null; + filterSecurityInterceptor = null; + } + } + } 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..8f2baeb4c68 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 { @@ -69,31 +69,67 @@ public void validate(FilterChainProxy fcp) { } checkPathOrder(new ArrayList<>(fcp.getFilterChains())); checkForDuplicateMatchers(new ArrayList<>(fcp.getFilterChains())); + checkAuthorizationFilters(new ArrayList<>(fcp.getFilterChains())); } 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; + } + } + } + + private void checkAuthorizationFilters(List chains) { + Filter authorizationFilter = null; + Filter filterSecurityInterceptor = null; + for (SecurityFilterChain chain : chains) { + for (Filter filter : chain.getFilters()) { + if (filter instanceof AuthorizationFilter) { + authorizationFilter = filter; + } + if (filter instanceof FilterSecurityInterceptor) { + filterSecurityInterceptor = filter; + } + } + if (authorizationFilter != null && filterSecurityInterceptor != null) { + this.logger.warn( + "It is not recommended to use authorizeRequests in the configuration. Please only use authorizeHttpRequests"); + } + if (filterSecurityInterceptor != null) { + this.logger.warn( + "Usage of authorizeRequests is deprecated. Please use authorizeHttpRequests in the configuration"); + } + authorizationFilter = null; + filterSecurityInterceptor = null; } } diff --git a/config/src/test/java/org/springframework/security/config/annotation/web/builders/WebSecurityFilterChainValidatorTests.java b/config/src/test/java/org/springframework/security/config/annotation/web/builders/WebSecurityFilterChainValidatorTests.java new file mode 100644 index 00000000000..450a3dfdc17 --- /dev/null +++ b/config/src/test/java/org/springframework/security/config/annotation/web/builders/WebSecurityFilterChainValidatorTests.java @@ -0,0 +1,119 @@ +/* + * 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.config.annotation.web.builders; + +import java.util.ArrayList; +import java.util.List; + +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.extension.ExtendWith; +import org.mockito.Mock; +import org.mockito.junit.jupiter.MockitoExtension; + +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.FilterSecurityInterceptor; +import org.springframework.security.web.authentication.AnonymousAuthenticationFilter; +import org.springframework.security.web.util.matcher.AntPathRequestMatcher; +import org.springframework.security.web.util.matcher.AnyRequestMatcher; +import org.springframework.security.web.util.matcher.RequestMatcher; +import org.springframework.security.web.util.matcher.RequestMatchers; + +import static org.assertj.core.api.Assertions.assertThatExceptionOfType; +import static org.assertj.core.api.Assertions.assertThatNoException; + +/** + * Tests for {@link WebSecurityFilterChainValidator} + * + * @author Max Batischev + */ +@ExtendWith(MockitoExtension.class) +public class WebSecurityFilterChainValidatorTests { + + private final WebSecurityFilterChainValidator validator = new WebSecurityFilterChainValidator(); + + @Mock + private AnonymousAuthenticationFilter authenticationFilter; + + @Mock + private ExceptionTranslationFilter exceptionTranslationFilter; + + @Mock + private FilterSecurityInterceptor authorizationInterceptor; + + @Test + void validateWhenFilterSecurityInterceptorConfiguredThenValidates() { + SecurityFilterChain chain = new DefaultSecurityFilterChain(AntPathRequestMatcher.antMatcher("/api"), + this.authenticationFilter, this.exceptionTranslationFilter, this.authorizationInterceptor); + FilterChainProxy proxy = new FilterChainProxy(List.of(chain)); + + assertThatNoException().isThrownBy(() -> this.validator.validate(proxy)); + } + + @Test + void validateWhenAnyRequestMatcherIsPresentThenUnreachableFilterChainException() { + SecurityFilterChain chain1 = new DefaultSecurityFilterChain(AntPathRequestMatcher.antMatcher("/api"), + this.authenticationFilter, this.exceptionTranslationFilter, this.authorizationInterceptor); + SecurityFilterChain chain2 = new DefaultSecurityFilterChain(AnyRequestMatcher.INSTANCE, + this.authenticationFilter, this.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)); + } + + @Test + void validateWhenSameRequestMatchersArePresentThenUnreachableFilterChainException() { + SecurityFilterChain chain1 = new DefaultSecurityFilterChain(AntPathRequestMatcher.antMatcher("/api"), + this.authenticationFilter, this.exceptionTranslationFilter, this.authorizationInterceptor); + SecurityFilterChain chain2 = new DefaultSecurityFilterChain(AntPathRequestMatcher.antMatcher("/api"), + this.authenticationFilter, this.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)); + } + + @Test + void validateWhenSameComposedRequestMatchersArePresentThenUnreachableFilterChainException() { + RequestMatcher matcher1 = RequestMatchers.anyOf(RequestMatchers.allOf(AntPathRequestMatcher.antMatcher("/api"), + AntPathRequestMatcher.antMatcher("*.do")), AntPathRequestMatcher.antMatcher("/admin")); + RequestMatcher matcher2 = RequestMatchers.anyOf(RequestMatchers.allOf(AntPathRequestMatcher.antMatcher("/api"), + AntPathRequestMatcher.antMatcher("*.do")), AntPathRequestMatcher.antMatcher("/admin")); + SecurityFilterChain chain1 = new DefaultSecurityFilterChain(matcher1, this.authenticationFilter, + this.exceptionTranslationFilter, this.authorizationInterceptor); + SecurityFilterChain chain2 = new DefaultSecurityFilterChain(matcher2, this.authenticationFilter, + this.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/config/src/test/java/org/springframework/security/config/annotation/web/configuration/WebSecurityConfigurationTests.java b/config/src/test/java/org/springframework/security/config/annotation/web/configuration/WebSecurityConfigurationTests.java index 326e2bda108..f6a53bff458 100644 --- a/config/src/test/java/org/springframework/security/config/annotation/web/configuration/WebSecurityConfigurationTests.java +++ b/config/src/test/java/org/springframework/security/config/annotation/web/configuration/WebSecurityConfigurationTests.java @@ -323,7 +323,7 @@ public void loadConfigWhenTwoSecurityFilterChainsPresentAndSecondWithAnyRequestT assertThatExceptionOfType(BeanCreationException.class) .isThrownBy(() -> this.spring.register(MultipleAnyRequestSecurityFilterChainConfig.class).autowire()) .havingRootCause() - .isExactlyInstanceOf(IllegalArgumentException.class); + .isInstanceOf(IllegalArgumentException.class); } private void assertAnotherUserPermission(WebInvocationPrivilegeEvaluator privilegeEvaluator) { 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..d75ce815d58 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,12 @@ 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.assertj.core.api.Assertions.assertThatNoException; import static org.mockito.ArgumentMatchers.any; import static org.mockito.BDDMockito.given; import static org.mockito.BDDMockito.willThrow; @@ -97,6 +104,11 @@ public void setUp() { ReflectionTestUtils.setField(this.validator, "logger", this.logger); } + @Test + void validateWhenFilterSecurityInterceptorConfiguredThenValidates() { + assertThatNoException().isThrownBy(() -> this.validator.validate(this.chain)); + } + // SEC-1878 @SuppressWarnings("unchecked") @Test @@ -130,4 +142,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/config/src/test/resources/serialized/6.5.x/org.springframework.security.web.UnreachableFilterChainException.serialized b/config/src/test/resources/serialized/6.5.x/org.springframework.security.web.UnreachableFilterChainException.serialized new file mode 100644 index 00000000000..418c3b8ece1 Binary files /dev/null and b/config/src/test/resources/serialized/6.5.x/org.springframework.security.web.UnreachableFilterChainException.serialized differ 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..ff697bd07d9 --- /dev/null +++ b/web/src/main/java/org/springframework/security/web/UnreachableFilterChainException.java @@ -0,0 +1,55 @@ +/* + * 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; + +import org.springframework.security.core.SpringSecurityCoreVersion; + +/** + * Thrown if {@link SecurityFilterChain securityFilterChain} is not valid. + * + * @author Max Batischev + * @since 6.5 + */ +public class UnreachableFilterChainException extends IllegalArgumentException { + + private static final long serialVersionUID = SpringSecurityCoreVersion.SERIAL_VERSION_UID; + + 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;