Skip to content

Polish SecurityFilterChain Validation #8

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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 {

Expand All @@ -75,25 +75,35 @@ 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",
securityFilterChain, chains.next());
}
}
}
}

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.",
defaultChain, chain);
}
}
}
if (chain instanceof DefaultSecurityFilterChain defaultChain) {
filterChain = defaultChain;
}
}
}

Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -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;
Expand All @@ -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;
Expand Down Expand Up @@ -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));
}

}
Binary file not shown.
Original file line number Diff line number Diff line change
@@ -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 <code>UnreachableFilterChainException</code> 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;
}

}
Original file line number Diff line number Diff line change
@@ -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.
Expand All @@ -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;
Expand Down Expand Up @@ -90,6 +91,23 @@ public MatchResult matcher(HttpServletRequest request) {
return MatchResult.match(variables);
}

@Override
public boolean equals(Object o) {
Comment on lines +94 to +95
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question (bug_risk): Review order sensitivity in equals/hashCode for AndRequestMatcher.

The equals method compares this.requestMatchers using Objects.equals, which is order-sensitive. Ensure that the ordering of request matchers is significant for equality; otherwise, consider an order-insensitive comparison if the logical contract requires it.

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;
Expand Down
Original file line number Diff line number Diff line change
@@ -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.
Expand All @@ -18,6 +18,7 @@

import java.util.Arrays;
import java.util.List;
import java.util.Objects;

import jakarta.servlet.http.HttpServletRequest;

Expand Down Expand Up @@ -81,6 +82,23 @@ public MatchResult matcher(HttpServletRequest request) {
return MatchResult.notMatch();
}

@Override
public boolean equals(Object o) {
Comment on lines +85 to +86
Copy link

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

question: Evaluate equality semantics for OrRequestMatcher.

Similar to AndRequestMatcher, the equals implementation here uses an order-sensitive comparison on the requestMatchers list. Confirm that the ordering should affect equality, or consider an alternative approach if order should be ignored.

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;
Expand Down