diff --git a/config/src/test/java/org/springframework/security/config/annotation/web/configuration/CompositeFilterChainProxyTests.java b/config/src/test/java/org/springframework/security/config/annotation/web/configuration/CompositeFilterChainProxyTests.java new file mode 100644 index 00000000000..1602e8790d8 --- /dev/null +++ b/config/src/test/java/org/springframework/security/config/annotation/web/configuration/CompositeFilterChainProxyTests.java @@ -0,0 +1,59 @@ +/* + * Copyright 2004-present 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.configuration; + +import java.util.List; + +import jakarta.servlet.Filter; +import org.junit.jupiter.api.Test; + +import org.springframework.mock.web.MockHttpServletRequest; +import org.springframework.security.web.DefaultSecurityFilterChain; +import org.springframework.security.web.FilterChainProxy; +import org.springframework.security.web.csrf.CsrfFilter; +import org.springframework.security.web.csrf.HttpSessionCsrfTokenRepository; +import org.springframework.security.web.util.matcher.AnyRequestMatcher; +import org.springframework.test.util.ReflectionTestUtils; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Regression tests for the reflective {@code getFilters(HttpServletRequest)} contract + * that {@code WebTestUtils.findFilter} relies on when invoked against a + * {@link WebSecurityConfiguration.CompositeFilterChainProxy}. + */ +class CompositeFilterChainProxyTests { + + @Test + void getFiltersWhenCompositeFilterChainProxyThenResolvesFilters() { + CsrfFilter sentinel = new CsrfFilter(new HttpSessionCsrfTokenRepository()); + DefaultSecurityFilterChain inner = new DefaultSecurityFilterChain(AnyRequestMatcher.INSTANCE, sentinel); + FilterChainProxy innerProxy = new FilterChainProxy(inner); + WebSecurityConfiguration.CompositeFilterChainProxy composite = new WebSecurityConfiguration.CompositeFilterChainProxy( + List.of(innerProxy)); + + MockHttpServletRequest request = new MockHttpServletRequest(); + + // Mirrors the exact reflective call WebTestUtils.findFilter makes at WebTestUtils.java:158. + // We invoke getFilters directly rather than via WebTestUtils.findFilter because that method + // is package-private and the CompositeFilterChainProxy class lives in a different package. + List filters = ReflectionTestUtils.invokeMethod(composite, "getFilters", request); + + assertThat(filters).contains(sentinel); + } + +} diff --git a/test/src/test/java/org/springframework/security/test/web/support/WebTestUtilsTests.java b/test/src/test/java/org/springframework/security/test/web/support/WebTestUtilsTests.java index 23849c391b7..d43d9c8e751 100644 --- a/test/src/test/java/org/springframework/security/test/web/support/WebTestUtilsTests.java +++ b/test/src/test/java/org/springframework/security/test/web/support/WebTestUtilsTests.java @@ -171,6 +171,18 @@ public void findFilterExplicitWithSecurityFilterInContext() { assertThat(WebTestUtils.findFilter(this.request, toFind.getClass())).isSameAs(toFind); } + @Test + void findFilterWhenFilterChainProxyThenResolvesFilters() { + CsrfFilter sentinel = new CsrfFilter(new HttpSessionCsrfTokenRepository()); + DefaultSecurityFilterChain chain = new DefaultSecurityFilterChain(AnyRequestMatcher.INSTANCE, sentinel); + FilterChainProxy fcp = new FilterChainProxy(chain); + this.request.getServletContext().setAttribute(BeanIds.SPRING_SECURITY_FILTER_CHAIN, fcp); + + CsrfFilter resolved = WebTestUtils.findFilter(this.request, CsrfFilter.class); + + assertThat(resolved).isSameAs(sentinel); + } + private void loadConfig(Class config) { AnnotationConfigWebApplicationContext context = new AnnotationConfigWebApplicationContext(); context.register(config); diff --git a/web/spring-security-web.gradle b/web/spring-security-web.gradle index f4ce686983a..b07cc74b764 100644 --- a/web/spring-security-web.gradle +++ b/web/spring-security-web.gradle @@ -54,6 +54,7 @@ dependencies { provided 'jakarta.servlet:jakarta.servlet-api' testImplementation project(path: ':spring-security-core', configuration: 'tests') + testImplementation 'ch.qos.logback:logback-classic' testImplementation 'io.projectreactor:reactor-test' testImplementation 'jakarta.xml.bind:jakarta.xml.bind-api' testImplementation 'jakarta.websocket:jakarta.websocket-api' diff --git a/web/src/main/java/org/springframework/security/web/DefaultSecurityFilterChain.java b/web/src/main/java/org/springframework/security/web/DefaultSecurityFilterChain.java index 9f1691f7817..a35f1b98a31 100644 --- a/web/src/main/java/org/springframework/security/web/DefaultSecurityFilterChain.java +++ b/web/src/main/java/org/springframework/security/web/DefaultSecurityFilterChain.java @@ -90,6 +90,17 @@ public boolean matches(HttpServletRequest request) { return this.requestMatcher.matches(request); } + /** + * Returns the Spring bean name of this chain when registered as a bean + * (via {@link BeanNameAware}); otherwise {@code null}. + * @return the bean name, or {@code null} if not registered as a bean + * @since 7.1 + */ + @Override + public @Nullable String getName() { + return this.beanName; + } + @Override public String toString() { List filterNames = new ArrayList<>(); diff --git a/web/src/main/java/org/springframework/security/web/FilterChainProxy.java b/web/src/main/java/org/springframework/security/web/FilterChainProxy.java index fb3aa11d586..077ff04037f 100644 --- a/web/src/main/java/org/springframework/security/web/FilterChainProxy.java +++ b/web/src/main/java/org/springframework/security/web/FilterChainProxy.java @@ -214,8 +214,8 @@ private void doFilterInternal(ServletRequest request, ServletResponse response, throws IOException, ServletException { FirewalledRequest firewallRequest = this.firewall.getFirewalledRequest((HttpServletRequest) request); HttpServletResponse firewallResponse = this.firewall.getFirewalledResponse((HttpServletResponse) response); - List filters = getFilters(firewallRequest); - if (filters == null || filters.isEmpty()) { + SecurityFilterChain matched = matchChain(firewallRequest); + if (matched == null || matched.getFilters().isEmpty()) { if (logger.isTraceEnabled()) { logger.trace(LogMessage.of(() -> "No security for " + requestLine(firewallRequest))); } @@ -223,12 +223,15 @@ private void doFilterInternal(ServletRequest request, ServletResponse response, this.filterChainDecorator.decorate(chain).doFilter(firewallRequest, firewallResponse); return; } + List filters = matched.getFilters(); + String chainName = matched.getName(); + String suffix = (chainName != null) ? " [" + chainName + "]" : ""; if (logger.isDebugEnabled()) { - logger.debug(LogMessage.of(() -> "Securing " + requestLine(firewallRequest))); + logger.debug(LogMessage.of(() -> "Securing " + requestLine(firewallRequest) + suffix)); } FilterChain reset = (req, res) -> { if (logger.isDebugEnabled()) { - logger.debug(LogMessage.of(() -> "Secured " + requestLine(firewallRequest))); + logger.debug(LogMessage.of(() -> "Secured " + requestLine(firewallRequest) + suffix)); } // Deactivate path stripping as we exit the security filter chain firewallRequest.reset(); @@ -238,24 +241,39 @@ private void doFilterInternal(ServletRequest request, ServletResponse response, } /** - * Returns the first filter chain matching the supplied URL. + * Returns the first filter chain matching the supplied request. * @param request the request to match - * @return an ordered array of Filters defining the filter chain + * @return the matching {@link SecurityFilterChain}, or {@code null} if none matches */ - private @Nullable List getFilters(HttpServletRequest request) { + private @Nullable SecurityFilterChain matchChain(HttpServletRequest request) { int count = 0; - for (SecurityFilterChain chain : this.filterChains) { + for (SecurityFilterChain candidate : this.filterChains) { if (logger.isTraceEnabled()) { - logger.trace(LogMessage.format("Trying to match request against %s (%d/%d)", chain, ++count, + logger.trace(LogMessage.format("Trying to match request against %s (%d/%d)", candidate, ++count, this.filterChains.size())); } - if (chain.matches(request)) { - return chain.getFilters(); + if (candidate.matches(request)) { + return candidate; } } return null; } + /** + * Returns the filters of the first chain matching the supplied request. + *

+ * NOTE: this method is invoked reflectively by Spring Security's test support + * (see {@code WebTestUtils.findFilter}). Renaming or changing its signature + * is a breaking change to that contract. + * @param request the request to match + * @return an ordered list of Filters defining the matched filter chain, or + * {@code null} if no chain matches + */ + private @Nullable List getFilters(HttpServletRequest request) { + SecurityFilterChain matched = matchChain(request); + return (matched != null) ? matched.getFilters() : null; + } + /** * Convenience method, mainly for testing. *

diff --git a/web/src/main/java/org/springframework/security/web/SecurityFilterChain.java b/web/src/main/java/org/springframework/security/web/SecurityFilterChain.java index 368845822bd..22768939212 100644 --- a/web/src/main/java/org/springframework/security/web/SecurityFilterChain.java +++ b/web/src/main/java/org/springframework/security/web/SecurityFilterChain.java @@ -20,6 +20,7 @@ import jakarta.servlet.Filter; import jakarta.servlet.http.HttpServletRequest; +import org.jspecify.annotations.Nullable; /** * Defines a filter chain which is capable of being matched against an @@ -36,4 +37,25 @@ public interface SecurityFilterChain { List getFilters(); + /** + * Returns a human-readable name identifying this chain, intended for + * diagnostics such as log messages. May be {@code null} when no + * meaningful name is available. + *

+ * The returned value is intended for diagnostics only; it is not + * stable across implementations and MUST NOT be used as a key for + * authorization, routing, or any other functional decision. + *

+ * Implementations should return quickly and should not throw exceptions. + * Implementations that wrap or delegate to another {@code SecurityFilterChain} + * are responsible for forwarding {@code getName()} appropriately when chain + * identity matters to consumers. + * + * @return the chain name, or {@code null} if unavailable + * @since 7.1 + */ + default @Nullable String getName() { + return null; + } + } diff --git a/web/src/test/java/org/springframework/security/web/DefaultSecurityFilterChainTests.java b/web/src/test/java/org/springframework/security/web/DefaultSecurityFilterChainTests.java new file mode 100644 index 00000000000..deeb1654869 --- /dev/null +++ b/web/src/test/java/org/springframework/security/web/DefaultSecurityFilterChainTests.java @@ -0,0 +1,47 @@ +/* + * Copyright 2004-present 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 java.util.Collections; + +import org.junit.jupiter.api.Test; + +import org.springframework.security.web.util.matcher.AnyRequestMatcher; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Tests for {@link DefaultSecurityFilterChain}. + */ +class DefaultSecurityFilterChainTests { + + @Test + void getNameWhenBeanNameNotSetThenReturnsNull() { + DefaultSecurityFilterChain chain = new DefaultSecurityFilterChain(AnyRequestMatcher.INSTANCE, + Collections.emptyList()); + assertThat(chain.getName()).isNull(); + } + + @Test + void getNameWhenBeanNameSetThenReturnsBeanName() { + DefaultSecurityFilterChain chain = new DefaultSecurityFilterChain(AnyRequestMatcher.INSTANCE, + Collections.emptyList()); + chain.setBeanName("apiSecurity"); + assertThat(chain.getName()).isEqualTo("apiSecurity"); + } + +} diff --git a/web/src/test/java/org/springframework/security/web/FilterChainProxyTests.java b/web/src/test/java/org/springframework/security/web/FilterChainProxyTests.java index bd64313687c..4f8a5db765e 100644 --- a/web/src/test/java/org/springframework/security/web/FilterChainProxyTests.java +++ b/web/src/test/java/org/springframework/security/web/FilterChainProxyTests.java @@ -22,6 +22,10 @@ import java.util.Iterator; import java.util.List; +import ch.qos.logback.classic.Level; +import ch.qos.logback.classic.Logger; +import ch.qos.logback.classic.spi.ILoggingEvent; +import ch.qos.logback.core.read.ListAppender; import io.micrometer.observation.Observation; import io.micrometer.observation.ObservationHandler; import io.micrometer.observation.ObservationRegistry; @@ -38,7 +42,9 @@ import org.junit.jupiter.api.Test; import org.mockito.ArgumentCaptor; import org.mockito.stubbing.Answer; +import org.slf4j.LoggerFactory; +import org.springframework.mock.web.MockFilterChain; import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.mock.web.MockHttpServletResponse; import org.springframework.security.authentication.TestingAuthenticationToken; @@ -49,6 +55,7 @@ import org.springframework.security.web.firewall.RequestRejectedException; import org.springframework.security.web.firewall.RequestRejectedHandler; import org.springframework.security.web.servlet.TestMockHttpServletMappings; +import org.springframework.security.web.util.matcher.AnyRequestMatcher; import org.springframework.security.web.util.matcher.RequestMatcher; import static org.assertj.core.api.Assertions.assertThat; @@ -499,6 +506,59 @@ public void doFilterWhenOneFilterDoesNotProceedThenObservationRegistryObserves() assertFilterChainObservation(contexts.next(), "after", 3); } + @Test + void doFilterWhenChainHasNameThenDebugLogIncludesNameSuffix() throws Exception { + MockHttpServletRequest request = new MockHttpServletRequest("GET", "/api/users"); + // MockHttpServletRequest.servletPath defaults to "" not null; UrlUtils.buildRequestUrl prefers it + request.setServletPath("/api/users"); + MockHttpServletResponse response = new MockHttpServletResponse(); + MockFilterChain mockChain = new MockFilterChain(); + + DefaultSecurityFilterChain named = new DefaultSecurityFilterChain(AnyRequestMatcher.INSTANCE, + new MockFilter()); + named.setBeanName("apiSecurity"); + FilterChainProxy fcp = new FilterChainProxy(named); + + ListAppender appender = attachAppender(Level.DEBUG); + try { + fcp.doFilter(request, response, mockChain); + assertThat(appender.list) + .extracting(ILoggingEvent::getFormattedMessage) + .anyMatch((msg) -> msg.equals("Securing GET /api/users [apiSecurity]")) + .anyMatch((msg) -> msg.equals("Secured GET /api/users [apiSecurity]")); + } + finally { + detachAppender(appender); + } + } + + @Test + void doFilterWhenChainHasNoNameThenDebugLogOmitsSuffix() throws Exception { + MockHttpServletRequest request = new MockHttpServletRequest("GET", "/api/users"); + // MockHttpServletRequest.servletPath defaults to "" not null; UrlUtils.buildRequestUrl prefers it + request.setServletPath("/api/users"); + MockHttpServletResponse response = new MockHttpServletResponse(); + MockFilterChain mockChain = new MockFilterChain(); + + DefaultSecurityFilterChain unnamed = new DefaultSecurityFilterChain(AnyRequestMatcher.INSTANCE, + new MockFilter()); + // beanName intentionally not set + FilterChainProxy fcp = new FilterChainProxy(unnamed); + + ListAppender appender = attachAppender(Level.DEBUG); + try { + fcp.doFilter(request, response, mockChain); + assertThat(appender.list) + .extracting(ILoggingEvent::getFormattedMessage) + .anyMatch((msg) -> msg.equals("Securing GET /api/users")) + .anyMatch((msg) -> msg.equals("Secured GET /api/users")) + .noneMatch((msg) -> msg.contains("[")); + } + finally { + detachAppender(appender); + } + } + static void assertFilterChainObservation(Observation.Context context, String filterSection, int chainPosition) { assertThat(context).isInstanceOf(ObservationFilterChainDecorator.FilterChainObservationContext.class); ObservationFilterChainDecorator.FilterChainObservationContext filterChainObservationContext = (ObservationFilterChainDecorator.FilterChainObservationContext) context; @@ -508,6 +568,23 @@ static void assertFilterChainObservation(Observation.Context context, String fil assertThat(filterChainObservationContext.getChainPosition()).isEqualTo(chainPosition); } + private ListAppender attachAppender(Level level) { + ListAppender appender = new ListAppender<>(); + appender.start(); + Logger fcpLogger = (Logger) LoggerFactory.getLogger(FilterChainProxy.class); + fcpLogger.setLevel(level); + fcpLogger.addAppender(appender); + return appender; + } + + private void detachAppender(ListAppender appender) { + Logger fcpLogger = (Logger) LoggerFactory.getLogger(FilterChainProxy.class); + fcpLogger.detachAppender(appender); + // Reset to no explicit level so it inherits from the parent + // (org.springframework.security = WARN per logback-test.xml). + fcpLogger.setLevel(null); + } + static Filter mockFilter() throws Exception { Filter filter = mock(Filter.class); willAnswer((invocation) -> { diff --git a/web/src/test/java/org/springframework/security/web/SecurityFilterChainTests.java b/web/src/test/java/org/springframework/security/web/SecurityFilterChainTests.java new file mode 100644 index 00000000000..c7dd3381515 --- /dev/null +++ b/web/src/test/java/org/springframework/security/web/SecurityFilterChainTests.java @@ -0,0 +1,49 @@ +/* + * Copyright 2004-present 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 java.util.Collections; +import java.util.List; + +import jakarta.servlet.Filter; +import jakarta.servlet.http.HttpServletRequest; +import org.junit.jupiter.api.Test; + +import static org.assertj.core.api.Assertions.assertThat; + +/** + * Tests for the {@link SecurityFilterChain} interface default behavior. + */ +class SecurityFilterChainTests { + + @Test + void getNameWhenInterfaceDefaultThenReturnsNull() { + SecurityFilterChain chain = new SecurityFilterChain() { + @Override + public boolean matches(HttpServletRequest request) { + return true; + } + + @Override + public List getFilters() { + return Collections.emptyList(); + } + }; + assertThat(chain.getName()).isNull(); + } + +}