From 1d98961cf8c093c80ee5154f14f3201256788b9a Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adam=20Ostro=C5=BEl=C3=ADk?= Date: Sat, 11 Dec 2021 15:27:45 +0100 Subject: [PATCH 1/7] Support multiple RequestRejectedHandler beans. --- .../annotation/web/builders/WebSecurity.java | 33 +++++++++++-------- .../security/web/FilterChainProxy.java | 27 +++++++++++---- .../web/firewall/RequestRejectedHandler.java | 13 ++++++++ .../security/web/FilterChainProxyTests.java | 24 +++++++++++--- 4 files changed, 73 insertions(+), 24 deletions(-) diff --git a/config/src/main/java/org/springframework/security/config/annotation/web/builders/WebSecurity.java b/config/src/main/java/org/springframework/security/config/annotation/web/builders/WebSecurity.java index 953d8f57758..700da3f4a77 100644 --- a/config/src/main/java/org/springframework/security/config/annotation/web/builders/WebSecurity.java +++ b/config/src/main/java/org/springframework/security/config/annotation/web/builders/WebSecurity.java @@ -16,15 +16,10 @@ package org.springframework.security.config.annotation.web.builders; -import java.util.ArrayList; -import java.util.List; - import jakarta.servlet.Filter; import jakarta.servlet.http.HttpServletRequest; - import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; - import org.springframework.beans.BeansException; import org.springframework.beans.factory.NoSuchBeanDefinitionException; import org.springframework.context.ApplicationContext; @@ -60,6 +55,11 @@ import org.springframework.util.Assert; import org.springframework.web.filter.DelegatingFilterProxy; +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collection; +import java.util.List; + /** *

* The {@link WebSecurity} is created by {@link WebSecurityConfiguration} to create the @@ -95,7 +95,7 @@ public final class WebSecurity extends AbstractConfiguredSecurityBuilder requestRejectedHandlers; private boolean debugEnabled; @@ -268,6 +268,17 @@ public WebSecurity postBuildAction(Runnable postBuildAction) { return this; } + /** + * Sets the handlers to handle {@link org.springframework.security.web.firewall.RequestRejectedException} + * @param requestRejectedHandlers + * @return the {@link WebSecurity} for further customizations + */ + public WebSecurity requestRejectedHandlers(RequestRejectedHandler... requestRejectedHandlers) { + Assert.notNull(requestRejectedHandlers, "requestRejectedHandlers cannot be null"); + this.requestRejectedHandlers = Arrays.asList(requestRejectedHandlers); + return this; + } + @Override protected Filter performBuild() throws Exception { Assert.state(!this.securityFilterChainBuilders.isEmpty(), @@ -288,8 +299,8 @@ protected Filter performBuild() throws Exception { if (this.httpFirewall != null) { filterChainProxy.setFirewall(this.httpFirewall); } - if (this.requestRejectedHandler != null) { - filterChainProxy.setRequestRejectedHandler(this.requestRejectedHandler); + if (this.requestRejectedHandlers != null) { + filterChainProxy.setRequestRejectedHandlers(this.requestRejectedHandlers); } filterChainProxy.afterPropertiesSet(); @@ -326,11 +337,7 @@ public void setApplicationContext(ApplicationContext applicationContext) throws } catch (NoSuchBeanDefinitionException ex) { } - try { - this.requestRejectedHandler = applicationContext.getBean(RequestRejectedHandler.class); - } - catch (NoSuchBeanDefinitionException ex) { - } + this.requestRejectedHandlers = applicationContext.getBeansOfType(RequestRejectedHandler.class).values(); } /** 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 2a562fcf8cf..78f213855d8 100644 --- a/web/src/main/java/org/springframework/security/web/FilterChainProxy.java +++ b/web/src/main/java/org/springframework/security/web/FilterChainProxy.java @@ -18,6 +18,7 @@ import java.io.IOException; import java.util.Arrays; +import java.util.Collection; import java.util.Collections; import java.util.List; @@ -152,7 +153,7 @@ public class FilterChainProxy extends GenericFilterBean { private HttpFirewall firewall = new StrictHttpFirewall(); - private RequestRejectedHandler requestRejectedHandler = new DefaultRequestRejectedHandler(); + private Collection requestRejectedHandlers = List.of(new DefaultRequestRejectedHandler()); public FilterChainProxy() { } @@ -183,7 +184,7 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha doFilterInternal(request, response, chain); } catch (RequestRejectedException ex) { - this.requestRejectedHandler.handle((HttpServletRequest) request, (HttpServletResponse) response, ex); + handleRequestRejectedException((HttpServletRequest) request, (HttpServletResponse) response, ex); } finally { SecurityContextHolder.clearContext(); @@ -191,6 +192,17 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha } } + private void handleRequestRejectedException(HttpServletRequest request, HttpServletResponse response, RequestRejectedException ex) throws ServletException, IOException { + var handlerOpt = this.requestRejectedHandlers.stream() + .filter(handler -> handler.shouldHandle(request)) + .findFirst(); + if (handlerOpt.isPresent()) { + handlerOpt.get().handle(request, response, ex); + } else { + new DefaultRequestRejectedHandler().handle(request, response, ex); + } + } + private void doFilterInternal(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException { FirewalledRequest firewallRequest = this.firewall.getFirewalledRequest((HttpServletRequest) request); @@ -270,12 +282,15 @@ public void setFirewall(HttpFirewall firewall) { /** * Sets the {@link RequestRejectedHandler} to be used for requests rejected by the * firewall. - * @param requestRejectedHandler the {@link RequestRejectedHandler} + * @param requestRejectedHandlers the {@link RequestRejectedHandler} * @since 5.2 */ - public void setRequestRejectedHandler(RequestRejectedHandler requestRejectedHandler) { - Assert.notNull(requestRejectedHandler, "requestRejectedHandler may not be null"); - this.requestRejectedHandler = requestRejectedHandler; + public void setRequestRejectedHandlers(Collection requestRejectedHandlers) { + Assert.notNull(requestRejectedHandlers, "requestRejectedHandlers may not be null"); + if (requestRejectedHandlers.isEmpty()) { + return; + } + this.requestRejectedHandlers = requestRejectedHandlers; } @Override diff --git a/web/src/main/java/org/springframework/security/web/firewall/RequestRejectedHandler.java b/web/src/main/java/org/springframework/security/web/firewall/RequestRejectedHandler.java index dba6a51f30b..a7995fd7b3a 100644 --- a/web/src/main/java/org/springframework/security/web/firewall/RequestRejectedHandler.java +++ b/web/src/main/java/org/springframework/security/web/firewall/RequestRejectedHandler.java @@ -25,8 +25,13 @@ /** * Used by {@link org.springframework.security.web.FilterChainProxy} to handle an * RequestRejectedException. + * Supports multiple beans of this handler. The handler is chosen by the result of + * shouldHandle method - true means it is chosen. + * If there are more than one handler to choose from, the first matching is used. + * If no suitable handler is found, the {@link DefaultRequestRejectedHandler} is used. * * @author Leonard Brünings + * @author Adam Ostrožlík * @since 5.4 */ public interface RequestRejectedHandler { @@ -42,4 +47,12 @@ public interface RequestRejectedHandler { void handle(HttpServletRequest request, HttpServletResponse response, RequestRejectedException requestRejectedException) throws IOException, ServletException; + /** + * Determines if this handler should be invoked. + * First available handler is invoked if there are multiple suitables ones. + * @param request that resulted in an RequestRejectedException + */ + default boolean shouldHandle(HttpServletRequest request) { + return true; + } } 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 398f639b8ac..20223b6e82e 100644 --- a/web/src/test/java/org/springframework/security/web/FilterChainProxyTests.java +++ b/web/src/test/java/org/springframework/security/web/FilterChainProxyTests.java @@ -49,9 +49,7 @@ import static org.mockito.ArgumentMatchers.eq; import static org.mockito.BDDMockito.given; import static org.mockito.BDDMockito.willAnswer; -import static org.mockito.Mockito.mock; -import static org.mockito.Mockito.verify; -import static org.mockito.Mockito.verifyZeroInteractions; +import static org.mockito.Mockito.*; /** * @author Luke Taylor @@ -237,19 +235,35 @@ public void doFilterClearsSecurityContextHolderOnceOnForwards() throws Exception @Test public void setRequestRejectedHandlerDoesNotAcceptNull() { - assertThatIllegalArgumentException().isThrownBy(() -> this.fcp.setRequestRejectedHandler(null)); + assertThatIllegalArgumentException().isThrownBy(() -> this.fcp.setRequestRejectedHandlers(null)); } @Test public void requestRejectedHandlerIsCalledIfFirewallThrowsRequestRejectedException() throws Exception { HttpFirewall fw = mock(HttpFirewall.class); RequestRejectedHandler rjh = mock(RequestRejectedHandler.class); + when(rjh.shouldHandle(this.request)).thenReturn(true); this.fcp.setFirewall(fw); - this.fcp.setRequestRejectedHandler(rjh); + this.fcp.setRequestRejectedHandlers(List.of(rjh)); RequestRejectedException requestRejectedException = new RequestRejectedException("Contains illegal chars"); given(fw.getFirewalledRequest(this.request)).willThrow(requestRejectedException); this.fcp.doFilter(this.request, this.response, this.chain); verify(rjh).handle(eq(this.request), eq(this.response), eq((requestRejectedException))); } + @Test + public void oneOfRequestRejectedHandlerIsCalledIfFirewallThrowsRequestRejectedException() throws Exception { + HttpFirewall fw = mock(HttpFirewall.class); + RequestRejectedHandler rjh1 = mock(RequestRejectedHandler.class); + RequestRejectedHandler rjh2 = mock(RequestRejectedHandler.class); + when(rjh1.shouldHandle(this.request)).thenReturn(false); + when(rjh2.shouldHandle(this.request)).thenReturn(true); + this.fcp.setFirewall(fw); + this.fcp.setRequestRejectedHandlers(List.of(rjh1, rjh2)); + RequestRejectedException requestRejectedException = new RequestRejectedException("Contains illegal chars"); + given(fw.getFirewalledRequest(this.request)).willThrow(requestRejectedException); + this.fcp.doFilter(this.request, this.response, this.chain); + verify(rjh2).handle(eq(this.request), eq(this.response), eq((requestRejectedException))); + } + } From de5bfa1ba1c3590608200db55ab791824926f736 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adam=20Ostro=C5=BEl=C3=ADk?= Date: Tue, 14 Dec 2021 07:29:59 +0100 Subject: [PATCH 2/7] Support multiple RequestRejectedHandler beans by using CompositeRequestRejectedHandler --- .../annotation/web/builders/WebSecurity.java | 13 ----- .../security/web/FilterChainProxy.java | 29 +++------- .../CompositeRequestRejectedHandler.java | 53 +++++++++++++++++++ .../web/firewall/RequestRejectedHandler.java | 13 ----- .../security/web/FilterChainProxyTests.java | 26 +++------ 5 files changed, 66 insertions(+), 68 deletions(-) create mode 100644 web/src/main/java/org/springframework/security/web/firewall/CompositeRequestRejectedHandler.java diff --git a/config/src/main/java/org/springframework/security/config/annotation/web/builders/WebSecurity.java b/config/src/main/java/org/springframework/security/config/annotation/web/builders/WebSecurity.java index 700da3f4a77..9ef8f4cb954 100644 --- a/config/src/main/java/org/springframework/security/config/annotation/web/builders/WebSecurity.java +++ b/config/src/main/java/org/springframework/security/config/annotation/web/builders/WebSecurity.java @@ -95,8 +95,6 @@ public final class WebSecurity extends AbstractConfiguredSecurityBuilder requestRejectedHandlers; - private boolean debugEnabled; private WebInvocationPrivilegeEvaluator privilegeEvaluator; @@ -268,17 +266,6 @@ public WebSecurity postBuildAction(Runnable postBuildAction) { return this; } - /** - * Sets the handlers to handle {@link org.springframework.security.web.firewall.RequestRejectedException} - * @param requestRejectedHandlers - * @return the {@link WebSecurity} for further customizations - */ - public WebSecurity requestRejectedHandlers(RequestRejectedHandler... requestRejectedHandlers) { - Assert.notNull(requestRejectedHandlers, "requestRejectedHandlers cannot be null"); - this.requestRejectedHandlers = Arrays.asList(requestRejectedHandlers); - return this; - } - @Override protected Filter performBuild() throws Exception { Assert.state(!this.securityFilterChainBuilders.isEmpty(), 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 78f213855d8..3222c9c6101 100644 --- a/web/src/main/java/org/springframework/security/web/FilterChainProxy.java +++ b/web/src/main/java/org/springframework/security/web/FilterChainProxy.java @@ -1,5 +1,5 @@ /* - * Copyright 2004, 2005, 2006 Acegi Technology Pty Limited + * Copyright 2002-2020 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,7 +18,6 @@ import java.io.IOException; import java.util.Arrays; -import java.util.Collection; import java.util.Collections; import java.util.List; @@ -153,7 +152,7 @@ public class FilterChainProxy extends GenericFilterBean { private HttpFirewall firewall = new StrictHttpFirewall(); - private Collection requestRejectedHandlers = List.of(new DefaultRequestRejectedHandler()); + private RequestRejectedHandler requestRejectedHandler = new DefaultRequestRejectedHandler(); public FilterChainProxy() { } @@ -184,7 +183,7 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha doFilterInternal(request, response, chain); } catch (RequestRejectedException ex) { - handleRequestRejectedException((HttpServletRequest) request, (HttpServletResponse) response, ex); + this.requestRejectedHandler.handle((HttpServletRequest) request, (HttpServletResponse) response, ex); } finally { SecurityContextHolder.clearContext(); @@ -192,17 +191,6 @@ public void doFilter(ServletRequest request, ServletResponse response, FilterCha } } - private void handleRequestRejectedException(HttpServletRequest request, HttpServletResponse response, RequestRejectedException ex) throws ServletException, IOException { - var handlerOpt = this.requestRejectedHandlers.stream() - .filter(handler -> handler.shouldHandle(request)) - .findFirst(); - if (handlerOpt.isPresent()) { - handlerOpt.get().handle(request, response, ex); - } else { - new DefaultRequestRejectedHandler().handle(request, response, ex); - } - } - private void doFilterInternal(ServletRequest request, ServletResponse response, FilterChain chain) throws IOException, ServletException { FirewalledRequest firewallRequest = this.firewall.getFirewalledRequest((HttpServletRequest) request); @@ -282,15 +270,12 @@ public void setFirewall(HttpFirewall firewall) { /** * Sets the {@link RequestRejectedHandler} to be used for requests rejected by the * firewall. - * @param requestRejectedHandlers the {@link RequestRejectedHandler} + * @param requestRejectedHandler the {@link RequestRejectedHandler} * @since 5.2 */ - public void setRequestRejectedHandlers(Collection requestRejectedHandlers) { - Assert.notNull(requestRejectedHandlers, "requestRejectedHandlers may not be null"); - if (requestRejectedHandlers.isEmpty()) { - return; - } - this.requestRejectedHandlers = requestRejectedHandlers; + public void setRequestRejectedHandler(RequestRejectedHandler requestRejectedHandler) { + Assert.notNull(requestRejectedHandler, "requestRejectedHandler may not be null"); + this.requestRejectedHandler = requestRejectedHandler; } @Override diff --git a/web/src/main/java/org/springframework/security/web/firewall/CompositeRequestRejectedHandler.java b/web/src/main/java/org/springframework/security/web/firewall/CompositeRequestRejectedHandler.java new file mode 100644 index 00000000000..f178028faea --- /dev/null +++ b/web/src/main/java/org/springframework/security/web/firewall/CompositeRequestRejectedHandler.java @@ -0,0 +1,53 @@ +/* + * Copyright 2002-2020 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.firewall; + +import jakarta.servlet.ServletException; +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; +import org.springframework.security.web.header.HeaderWriter; +import org.springframework.util.Assert; + +import java.io.IOException; +import java.util.List; + +/** + * A {@link RequestRejectedHandler} that delegates to several other {@link RequestRejectedHandler}s. + * + * @author Adam Ostrožlík + * @since 5.7 + */ +public class CompositeRequestRejectedHandler implements RequestRejectedHandler { + + private final List requestRejectedhandlers; + + /** + * Creates a new instance. + * @param requestRejectedhandlers the {@link RequestRejectedHandler} instances to handle {@link org.springframework.security.web.firewall.RequestRejectedException} + */ + public CompositeRequestRejectedHandler(List requestRejectedhandlers) { + Assert.notEmpty(requestRejectedhandlers, "requestRejectedhandlers cannot be empty"); + this.requestRejectedhandlers = requestRejectedhandlers; + } + + @Override + public void handle(HttpServletRequest request, HttpServletResponse response, RequestRejectedException requestRejectedException) throws IOException, ServletException { + for (RequestRejectedHandler requestRejectedhandler : requestRejectedhandlers) { + requestRejectedhandler.handle(request, response, requestRejectedException); + } + } +} diff --git a/web/src/main/java/org/springframework/security/web/firewall/RequestRejectedHandler.java b/web/src/main/java/org/springframework/security/web/firewall/RequestRejectedHandler.java index a7995fd7b3a..dba6a51f30b 100644 --- a/web/src/main/java/org/springframework/security/web/firewall/RequestRejectedHandler.java +++ b/web/src/main/java/org/springframework/security/web/firewall/RequestRejectedHandler.java @@ -25,13 +25,8 @@ /** * Used by {@link org.springframework.security.web.FilterChainProxy} to handle an * RequestRejectedException. - * Supports multiple beans of this handler. The handler is chosen by the result of - * shouldHandle method - true means it is chosen. - * If there are more than one handler to choose from, the first matching is used. - * If no suitable handler is found, the {@link DefaultRequestRejectedHandler} is used. * * @author Leonard Brünings - * @author Adam Ostrožlík * @since 5.4 */ public interface RequestRejectedHandler { @@ -47,12 +42,4 @@ public interface RequestRejectedHandler { void handle(HttpServletRequest request, HttpServletResponse response, RequestRejectedException requestRejectedException) throws IOException, ServletException; - /** - * Determines if this handler should be invoked. - * First available handler is invoked if there are multiple suitables ones. - * @param request that resulted in an RequestRejectedException - */ - default boolean shouldHandle(HttpServletRequest request) { - return true; - } } 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 20223b6e82e..d05767e288f 100644 --- a/web/src/test/java/org/springframework/security/web/FilterChainProxyTests.java +++ b/web/src/test/java/org/springframework/security/web/FilterChainProxyTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2016 the original author or authors. + * Copyright 2002-2020 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. @@ -49,7 +49,9 @@ import static org.mockito.ArgumentMatchers.eq; import static org.mockito.BDDMockito.given; import static org.mockito.BDDMockito.willAnswer; -import static org.mockito.Mockito.*; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.verify; +import static org.mockito.Mockito.verifyZeroInteractions; /** * @author Luke Taylor @@ -235,35 +237,19 @@ public void doFilterClearsSecurityContextHolderOnceOnForwards() throws Exception @Test public void setRequestRejectedHandlerDoesNotAcceptNull() { - assertThatIllegalArgumentException().isThrownBy(() -> this.fcp.setRequestRejectedHandlers(null)); + assertThatIllegalArgumentException().isThrownBy(() -> this.fcp.setRequestRejectedHandler(null)); } @Test public void requestRejectedHandlerIsCalledIfFirewallThrowsRequestRejectedException() throws Exception { HttpFirewall fw = mock(HttpFirewall.class); RequestRejectedHandler rjh = mock(RequestRejectedHandler.class); - when(rjh.shouldHandle(this.request)).thenReturn(true); this.fcp.setFirewall(fw); - this.fcp.setRequestRejectedHandlers(List.of(rjh)); + this.fcp.setRequestRejectedHandler(rjh); RequestRejectedException requestRejectedException = new RequestRejectedException("Contains illegal chars"); given(fw.getFirewalledRequest(this.request)).willThrow(requestRejectedException); this.fcp.doFilter(this.request, this.response, this.chain); verify(rjh).handle(eq(this.request), eq(this.response), eq((requestRejectedException))); } - @Test - public void oneOfRequestRejectedHandlerIsCalledIfFirewallThrowsRequestRejectedException() throws Exception { - HttpFirewall fw = mock(HttpFirewall.class); - RequestRejectedHandler rjh1 = mock(RequestRejectedHandler.class); - RequestRejectedHandler rjh2 = mock(RequestRejectedHandler.class); - when(rjh1.shouldHandle(this.request)).thenReturn(false); - when(rjh2.shouldHandle(this.request)).thenReturn(true); - this.fcp.setFirewall(fw); - this.fcp.setRequestRejectedHandlers(List.of(rjh1, rjh2)); - RequestRejectedException requestRejectedException = new RequestRejectedException("Contains illegal chars"); - given(fw.getFirewalledRequest(this.request)).willThrow(requestRejectedException); - this.fcp.doFilter(this.request, this.response, this.chain); - verify(rjh2).handle(eq(this.request), eq(this.response), eq((requestRejectedException))); - } - } From f9ff7e93227ff402f62bcfc4d62911a7d82ffe2b Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adam=20Ostro=C5=BEl=C3=ADk?= Date: Tue, 14 Dec 2021 07:33:30 +0100 Subject: [PATCH 3/7] Set RequestRejectedHandler via WebSecurity --- .../annotation/web/builders/WebSecurity.java | 26 +++++++++++++++---- 1 file changed, 21 insertions(+), 5 deletions(-) diff --git a/config/src/main/java/org/springframework/security/config/annotation/web/builders/WebSecurity.java b/config/src/main/java/org/springframework/security/config/annotation/web/builders/WebSecurity.java index 9ef8f4cb954..f2cb2bbe5b2 100644 --- a/config/src/main/java/org/springframework/security/config/annotation/web/builders/WebSecurity.java +++ b/config/src/main/java/org/springframework/security/config/annotation/web/builders/WebSecurity.java @@ -56,8 +56,6 @@ import org.springframework.web.filter.DelegatingFilterProxy; import java.util.ArrayList; -import java.util.Arrays; -import java.util.Collection; import java.util.List; /** @@ -95,6 +93,8 @@ public final class WebSecurity extends AbstractConfiguredSecurityBuilder Date: Tue, 14 Dec 2021 08:42:25 +0100 Subject: [PATCH 4/7] fix formatting --- .../config/annotation/web/builders/WebSecurity.java | 3 ++- .../web/firewall/CompositeRequestRejectedHandler.java | 10 +++++++--- 2 files changed, 9 insertions(+), 4 deletions(-) diff --git a/config/src/main/java/org/springframework/security/config/annotation/web/builders/WebSecurity.java b/config/src/main/java/org/springframework/security/config/annotation/web/builders/WebSecurity.java index f2cb2bbe5b2..e74c88259ff 100644 --- a/config/src/main/java/org/springframework/security/config/annotation/web/builders/WebSecurity.java +++ b/config/src/main/java/org/springframework/security/config/annotation/web/builders/WebSecurity.java @@ -267,7 +267,8 @@ public WebSecurity postBuildAction(Runnable postBuildAction) { } /** - * Sets the handler to handle {@link org.springframework.security.web.firewall.RequestRejectedException} + * Sets the handler to handle + * {@link org.springframework.security.web.firewall.RequestRejectedException} * @param requestRejectedHandler * @return the {@link WebSecurity} for further customizations * @since 5.7 diff --git a/web/src/main/java/org/springframework/security/web/firewall/CompositeRequestRejectedHandler.java b/web/src/main/java/org/springframework/security/web/firewall/CompositeRequestRejectedHandler.java index f178028faea..90fd213bf48 100644 --- a/web/src/main/java/org/springframework/security/web/firewall/CompositeRequestRejectedHandler.java +++ b/web/src/main/java/org/springframework/security/web/firewall/CompositeRequestRejectedHandler.java @@ -26,7 +26,8 @@ import java.util.List; /** - * A {@link RequestRejectedHandler} that delegates to several other {@link RequestRejectedHandler}s. + * A {@link RequestRejectedHandler} that delegates to several other + * {@link RequestRejectedHandler}s. * * @author Adam Ostrožlík * @since 5.7 @@ -37,7 +38,8 @@ public class CompositeRequestRejectedHandler implements RequestRejectedHandler { /** * Creates a new instance. - * @param requestRejectedhandlers the {@link RequestRejectedHandler} instances to handle {@link org.springframework.security.web.firewall.RequestRejectedException} + * @param requestRejectedhandlers the {@link RequestRejectedHandler} instances to + * handle {@link org.springframework.security.web.firewall.RequestRejectedException} */ public CompositeRequestRejectedHandler(List requestRejectedhandlers) { Assert.notEmpty(requestRejectedhandlers, "requestRejectedhandlers cannot be empty"); @@ -45,9 +47,11 @@ public CompositeRequestRejectedHandler(List requestRejec } @Override - public void handle(HttpServletRequest request, HttpServletResponse response, RequestRejectedException requestRejectedException) throws IOException, ServletException { + public void handle(HttpServletRequest request, HttpServletResponse response, + RequestRejectedException requestRejectedException) throws IOException, ServletException { for (RequestRejectedHandler requestRejectedhandler : requestRejectedhandlers) { requestRejectedhandler.handle(request, response, requestRejectedException); } } + } From 61ca3bd9a8c1a4f098bc4a6cb4ac872da43521e0 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adam=20Ostro=C5=BEl=C3=ADk?= Date: Wed, 15 Dec 2021 07:29:23 +0100 Subject: [PATCH 5/7] Code review --- .../config/annotation/web/builders/WebSecurity.java | 4 ++-- .../springframework/security/web/FilterChainProxy.java | 2 +- .../web/firewall/CompositeRequestRejectedHandler.java | 10 +++++----- .../security/web/FilterChainProxyTests.java | 2 +- 4 files changed, 9 insertions(+), 9 deletions(-) diff --git a/config/src/main/java/org/springframework/security/config/annotation/web/builders/WebSecurity.java b/config/src/main/java/org/springframework/security/config/annotation/web/builders/WebSecurity.java index e74c88259ff..e9231464240 100644 --- a/config/src/main/java/org/springframework/security/config/annotation/web/builders/WebSecurity.java +++ b/config/src/main/java/org/springframework/security/config/annotation/web/builders/WebSecurity.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2020 the original author or authors. + * Copyright 2002-2021 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. @@ -274,7 +274,7 @@ public WebSecurity postBuildAction(Runnable postBuildAction) { * @since 5.7 */ public WebSecurity requestRejectedHandler(RequestRejectedHandler requestRejectedHandler) { - Assert.notNull(this.requestRejectedHandler, "requestRejectedHandlers cannot be null"); + Assert.notNull(requestRejectedHandler, "requestRejectedHandler cannot be null"); this.requestRejectedHandler = requestRejectedHandler; return this; } 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 3222c9c6101..2a562fcf8cf 100644 --- a/web/src/main/java/org/springframework/security/web/FilterChainProxy.java +++ b/web/src/main/java/org/springframework/security/web/FilterChainProxy.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2020 the original author or authors. + * Copyright 2004, 2005, 2006 Acegi Technology Pty Limited * * Licensed under the Apache License, Version 2.0 (the "License"); * you may not use this file except in compliance with the License. diff --git a/web/src/main/java/org/springframework/security/web/firewall/CompositeRequestRejectedHandler.java b/web/src/main/java/org/springframework/security/web/firewall/CompositeRequestRejectedHandler.java index 90fd213bf48..bc7d658e653 100644 --- a/web/src/main/java/org/springframework/security/web/firewall/CompositeRequestRejectedHandler.java +++ b/web/src/main/java/org/springframework/security/web/firewall/CompositeRequestRejectedHandler.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2020 the original author or authors. + * Copyright 2002-2021 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. @@ -19,10 +19,10 @@ import jakarta.servlet.ServletException; import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; -import org.springframework.security.web.header.HeaderWriter; import org.springframework.util.Assert; import java.io.IOException; +import java.util.Arrays; import java.util.List; /** @@ -32,7 +32,7 @@ * @author Adam Ostrožlík * @since 5.7 */ -public class CompositeRequestRejectedHandler implements RequestRejectedHandler { +public final class CompositeRequestRejectedHandler implements RequestRejectedHandler { private final List requestRejectedhandlers; @@ -41,9 +41,9 @@ public class CompositeRequestRejectedHandler implements RequestRejectedHandler { * @param requestRejectedhandlers the {@link RequestRejectedHandler} instances to * handle {@link org.springframework.security.web.firewall.RequestRejectedException} */ - public CompositeRequestRejectedHandler(List requestRejectedhandlers) { + public CompositeRequestRejectedHandler(RequestRejectedHandler... requestRejectedhandlers) { Assert.notEmpty(requestRejectedhandlers, "requestRejectedhandlers cannot be empty"); - this.requestRejectedhandlers = requestRejectedhandlers; + this.requestRejectedhandlers = Arrays.asList(requestRejectedhandlers); } @Override 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 d05767e288f..398f639b8ac 100644 --- a/web/src/test/java/org/springframework/security/web/FilterChainProxyTests.java +++ b/web/src/test/java/org/springframework/security/web/FilterChainProxyTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2020 the original author or authors. + * Copyright 2002-2016 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. From d0e8d29f54f27110738421440d20f6b4dc148989 Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adam=20Ostro=C5=BEl=C3=ADk?= Date: Wed, 15 Dec 2021 18:12:58 +0100 Subject: [PATCH 6/7] RequestRejectedHandler tests --- .../web/builders/WebSecurityTests.java | 24 +++++++++++ .../CompositeRequestRejectedHandlerTests.java | 43 +++++++++++++++++++ 2 files changed, 67 insertions(+) create mode 100644 web/src/test/java/org/springframework/security/web/firewall/CompositeRequestRejectedHandlerTests.java diff --git a/config/src/test/java/org/springframework/security/config/annotation/web/builders/WebSecurityTests.java b/config/src/test/java/org/springframework/security/config/annotation/web/builders/WebSecurityTests.java index 3076cb5d2ba..278f0bb6e25 100644 --- a/config/src/test/java/org/springframework/security/config/annotation/web/builders/WebSecurityTests.java +++ b/config/src/test/java/org/springframework/security/config/annotation/web/builders/WebSecurityTests.java @@ -16,6 +16,7 @@ package org.springframework.security.config.annotation.web.builders; +import jakarta.servlet.ServletException; import jakarta.servlet.http.HttpServletResponse; import org.junit.jupiter.api.AfterEach; @@ -24,6 +25,7 @@ import org.springframework.beans.factory.annotation.Autowired; import org.springframework.context.annotation.Configuration; +import org.springframework.http.HttpStatus; import org.springframework.mock.web.MockFilterChain; import org.springframework.mock.web.MockHttpServletRequest; import org.springframework.mock.web.MockHttpServletResponse; @@ -32,6 +34,7 @@ import org.springframework.security.config.annotation.web.configuration.EnableWebSecurity; import org.springframework.security.config.annotation.web.configuration.WebSecurityConfigurerAdapter; import org.springframework.security.web.FilterChainProxy; +import org.springframework.security.web.firewall.HttpStatusRequestRejectedHandler; import org.springframework.web.bind.annotation.RequestMapping; import org.springframework.web.bind.annotation.RestController; import org.springframework.web.context.support.AnnotationConfigWebApplicationContext; @@ -39,6 +42,8 @@ import org.springframework.web.servlet.config.annotation.PathMatchConfigurer; import org.springframework.web.servlet.config.annotation.WebMvcConfigurer; +import java.io.IOException; + import static org.assertj.core.api.Assertions.assertThat; /** @@ -92,6 +97,15 @@ public void ignoringMvcMatcher() throws Exception { assertThat(this.response.getStatus()).isEqualTo(HttpServletResponse.SC_UNAUTHORIZED); } + @Test + public void requestRejectedHandlerInvoked() throws ServletException, IOException { + loadConfig(RequestRejectedHandlerConfig.class); + this.request.setServletPath("/spring"); + this.request.setRequestURI("/spring/\u0019path"); + this.springSecurityFilterChain.doFilter(this.request, this.response, this.chain); + assertThat(this.response.getStatus()).isEqualTo(HttpServletResponse.SC_BAD_REQUEST); + } + @Test public void ignoringMvcMatcherServletPath() throws Exception { loadConfig(MvcMatcherServletPathConfig.class, LegacyMvcMatchingConfig.class); @@ -223,4 +237,14 @@ public void configurePathMatch(PathMatchConfigurer configurer) { } + @EnableWebSecurity + static class RequestRejectedHandlerConfig extends WebSecurityConfigurerAdapter { + + @Override + public void configure(WebSecurity web) throws Exception { + web.requestRejectedHandler(new HttpStatusRequestRejectedHandler(HttpStatus.BAD_REQUEST.value())); + } + + } + } diff --git a/web/src/test/java/org/springframework/security/web/firewall/CompositeRequestRejectedHandlerTests.java b/web/src/test/java/org/springframework/security/web/firewall/CompositeRequestRejectedHandlerTests.java new file mode 100644 index 00000000000..08c03e4f77c --- /dev/null +++ b/web/src/test/java/org/springframework/security/web/firewall/CompositeRequestRejectedHandlerTests.java @@ -0,0 +1,43 @@ +/* + * Copyright 2002-2020 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.firewall; + +import jakarta.servlet.http.HttpServletRequest; +import jakarta.servlet.http.HttpServletResponse; +import org.junit.jupiter.api.Test; + +import static org.assertj.core.api.Assertions.assertThatExceptionOfType; +import static org.mockito.Mockito.mock; + +public class CompositeRequestRejectedHandlerTests { + + @Test + void compositeRequestRejectedHandlerRethrowsTheException() { + RequestRejectedException requestRejectedException = new RequestRejectedException("rejected"); + DefaultRequestRejectedHandler sut = new DefaultRequestRejectedHandler(); + CompositeRequestRejectedHandler crrh = new CompositeRequestRejectedHandler(sut); + assertThatExceptionOfType(RequestRejectedException.class).isThrownBy(() -> crrh + .handle(mock(HttpServletRequest.class), mock(HttpServletResponse.class), requestRejectedException)) + .withMessage("rejected"); + } + + @Test + void compositeRequestRejectedHandlerForbidsEmptyHandlers() { + assertThatExceptionOfType(IllegalArgumentException.class).isThrownBy(CompositeRequestRejectedHandler::new); + } + +} From 2cacde4d4e6328a91f173ad5c53d048e61a7973d Mon Sep 17 00:00:00 2001 From: =?UTF-8?q?Adam=20Ostro=C5=BEl=C3=ADk?= Date: Thu, 16 Dec 2021 09:40:40 +0100 Subject: [PATCH 7/7] Update CompositeRequestRejectedHandlerTests.java --- .../web/firewall/CompositeRequestRejectedHandlerTests.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/web/src/test/java/org/springframework/security/web/firewall/CompositeRequestRejectedHandlerTests.java b/web/src/test/java/org/springframework/security/web/firewall/CompositeRequestRejectedHandlerTests.java index 08c03e4f77c..3868aad98ea 100644 --- a/web/src/test/java/org/springframework/security/web/firewall/CompositeRequestRejectedHandlerTests.java +++ b/web/src/test/java/org/springframework/security/web/firewall/CompositeRequestRejectedHandlerTests.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2020 the original author or authors. + * Copyright 2002-2021 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.