diff --git a/spring-context/src/main/java/org/springframework/validation/DataBinder.java b/spring-context/src/main/java/org/springframework/validation/DataBinder.java index c2fcea3ebaf1..be21247903bf 100644 --- a/spring-context/src/main/java/org/springframework/validation/DataBinder.java +++ b/spring-context/src/main/java/org/springframework/validation/DataBinder.java @@ -91,9 +91,6 @@ * javadoc states details on the default resolution rules. * *

This generic data binder can be used in any kind of environment. - * It is typically used by Spring web MVC controllers, via the web-specific - * subclasses {@link org.springframework.web.bind.ServletRequestDataBinder} - * and {@link org.springframework.web.portlet.bind.PortletRequestDataBinder}. * * @author Rod Johnson * @author Juergen Hoeller @@ -114,10 +111,10 @@ */ public class DataBinder implements PropertyEditorRegistry, TypeConverter { - /** Default object name used for binding: "target" */ + /** Default object name used for binding: "target". */ public static final String DEFAULT_OBJECT_NAME = "target"; - /** Default limit for array and collection growing: 256 */ + /** Default limit for array and collection growing: 256. */ public static final int DEFAULT_AUTO_GROW_COLLECTION_LIMIT = 256; @@ -453,19 +450,38 @@ public String[] getAllowedFields() { } /** - * Register fields that should not be allowed for binding. Default is none. - * Mark fields as disallowed for example to avoid unwanted modifications - * by malicious users when binding HTTP request parameters. - *

Supports "xxx*", "*xxx" and "*xxx*" patterns. More sophisticated matching - * can be implemented by overriding the {@code isAllowed} method. - *

Alternatively, specify a list of allowed fields. - * @param disallowedFields array of field names + * Register field patterns that should not be allowed for binding. + *

Default is none. + *

Mark fields as disallowed, for example to avoid unwanted + * modifications by malicious users when binding HTTP request parameters. + *

Supports {@code "xxx*"}, {@code "*xxx"}, {@code "*xxx*"}, and + * {@code "xxx*yyy"} matches (with an arbitrary number of pattern parts), as + * well as direct equality. + *

The default implementation of this method stores disallowed field patterns + * in {@linkplain PropertyAccessorUtils#canonicalPropertyName(String) canonical} + * form. As of Spring Framework 5.2.21, the default implementation also transforms + * disallowed field patterns to {@linkplain String#toLowerCase() lowercase} to + * support case-insensitive pattern matching in {@link #isAllowed}. Subclasses + * which override this method must therefore take both of these transformations + * into account. + *

More sophisticated matching can be implemented by overriding the + * {@link #isAllowed} method. + *

Alternatively, specify a list of allowed field patterns. + * @param disallowedFields array of disallowed field patterns * @see #setAllowedFields * @see #isAllowed(String) - * @see org.springframework.web.bind.ServletRequestDataBinder */ public void setDisallowedFields(String... disallowedFields) { - this.disallowedFields = PropertyAccessorUtils.canonicalPropertyNames(disallowedFields); + if (disallowedFields == null) { + this.disallowedFields = null; + } + else { + String[] fieldPatterns = new String[disallowedFields.length]; + for (int i = 0; i < fieldPatterns.length; i++) { + fieldPatterns[i] = PropertyAccessorUtils.canonicalPropertyName(disallowedFields[i]).toLowerCase(); + } + this.disallowedFields = fieldPatterns; + } } /** @@ -796,7 +812,7 @@ protected boolean isAllowed(String field) { String[] allowed = getAllowedFields(); String[] disallowed = getDisallowedFields(); return ((ObjectUtils.isEmpty(allowed) || PatternMatchUtils.simpleMatch(allowed, field)) && - (ObjectUtils.isEmpty(disallowed) || !PatternMatchUtils.simpleMatch(disallowed, field))); + (ObjectUtils.isEmpty(disallowed) || !PatternMatchUtils.simpleMatch(disallowed, field.toLowerCase()))); } /** diff --git a/spring-context/src/test/java/org/springframework/validation/DataBinderTests.java b/spring-context/src/test/java/org/springframework/validation/DataBinderTests.java index 75d89472c335..1631bdae4990 100644 --- a/spring-context/src/test/java/org/springframework/validation/DataBinderTests.java +++ b/spring-context/src/test/java/org/springframework/validation/DataBinderTests.java @@ -738,7 +738,7 @@ public void testBindingWithOverlappingAllowedAndDisallowedFields() throws Except TestBean rod = new TestBean(); DataBinder binder = new DataBinder(rod); binder.setAllowedFields("name", "age"); - binder.setDisallowedFields("age"); + binder.setDisallowedFields("AGE"); MutablePropertyValues pvs = new MutablePropertyValues(); pvs.add("name", "Rod"); pvs.add("age", "32x"); @@ -784,7 +784,7 @@ public void testBindingWithAllowedAndDisallowedMapFields() throws Exception { TestBean rod = new TestBean(); DataBinder binder = new DataBinder(rod); binder.setAllowedFields("someMap[key1]", "someMap[key2]"); - binder.setDisallowedFields("someMap['key3']", "someMap[key4]"); + binder.setDisallowedFields("someMap['KEY3']", "SomeMap[key4]"); MutablePropertyValues pvs = new MutablePropertyValues(); pvs.add("someMap[key1]", "value1"); @@ -1880,11 +1880,12 @@ public void testTrackDisallowedFields() throws Exception { binder.setAllowedFields("name", "age"); String name = "Rob Harrop"; - String beanName = "foobar"; + int age = 42; MutablePropertyValues mpvs = new MutablePropertyValues(); mpvs.add("name", name); - mpvs.add("beanName", beanName); + mpvs.add("age", age); + mpvs.add("beanName", "foobar"); binder.bind(mpvs); assertEquals(name, testBean.getName()); diff --git a/spring-web/src/main/java/org/springframework/web/bind/annotation/ExceptionHandler.java b/spring-web/src/main/java/org/springframework/web/bind/annotation/ExceptionHandler.java index b9ea9ca4bfb7..49be8a32278f 100644 --- a/spring-web/src/main/java/org/springframework/web/bind/annotation/ExceptionHandler.java +++ b/spring-web/src/main/java/org/springframework/web/bind/annotation/ExceptionHandler.java @@ -114,6 +114,7 @@ * @author Arjen Poutsma * @author Juergen Hoeller * @since 3.0 + * @see ControllerAdvice * @see org.springframework.web.context.request.WebRequest * @see org.springframework.web.servlet.mvc.annotation.AnnotationMethodHandlerExceptionResolver * @see org.springframework.web.portlet.mvc.annotation.AnnotationMethodHandlerExceptionResolver diff --git a/spring-web/src/main/java/org/springframework/web/bind/annotation/InitBinder.java b/spring-web/src/main/java/org/springframework/web/bind/annotation/InitBinder.java index 5fc5d6bcc279..370b2f2801e0 100644 --- a/spring-web/src/main/java/org/springframework/web/bind/annotation/InitBinder.java +++ b/spring-web/src/main/java/org/springframework/web/bind/annotation/InitBinder.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2012 the original author or authors. + * Copyright 2002-2022 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. @@ -23,15 +23,24 @@ import java.lang.annotation.Target; /** - * Annotation that identifies methods which initialize the + * Annotation that identifies methods that initialize the * {@link org.springframework.web.bind.WebDataBinder} which * will be used for populating command and form object arguments * of annotated handler methods. * - *

Such init-binder methods support all arguments that {@link RequestMapping} - * supports, except for command/form objects and corresponding validation result - * objects. Init-binder methods must not have a return value; they are usually - * declared as {@code void}. + *

WARNING: Data binding can lead to security issues by exposing + * parts of the object graph that are not meant to be accessed or modified by + * external clients. Therefore the design and use of data binding should be considered + * carefully with regard to security. For more details, please refer to the dedicated + * sections on data binding for + * Spring Web MVC and + * Spring WebFlux + * in the reference manual. + * + *

{@code @InitBinder} methods support all arguments that + * {@link RequestMapping @RequestMapping} methods support, except for command/form + * objects and corresponding validation result objects. {@code @InitBinder} methods + * must not have a return value; they are usually declared as {@code void}. * *

Typical arguments are {@link org.springframework.web.bind.WebDataBinder} * in combination with {@link org.springframework.web.context.request.WebRequest} @@ -39,6 +48,7 @@ * * @author Juergen Hoeller * @since 2.5 + * @see ControllerAdvice * @see org.springframework.web.bind.WebDataBinder * @see org.springframework.web.context.request.WebRequest */ diff --git a/spring-web/src/main/java/org/springframework/web/bind/annotation/ModelAttribute.java b/spring-web/src/main/java/org/springframework/web/bind/annotation/ModelAttribute.java index 717a6d0106ef..3316065a0760 100644 --- a/spring-web/src/main/java/org/springframework/web/bind/annotation/ModelAttribute.java +++ b/spring-web/src/main/java/org/springframework/web/bind/annotation/ModelAttribute.java @@ -1,5 +1,5 @@ /* - * Copyright 2002-2016 the original author or authors. + * Copyright 2002-2022 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. @@ -31,18 +31,27 @@ * for controller classes with {@link RequestMapping @RequestMapping} * methods. * - *

Can be used to expose command objects to a web view, using - * specific attribute names, through annotating corresponding - * parameters of an {@link RequestMapping @RequestMapping} method. + *

WARNING: Data binding can lead to security issues by exposing + * parts of the object graph that are not meant to be accessed or modified by + * external clients. Therefore the design and use of data binding should be considered + * carefully with regard to security. For more details, please refer to the dedicated + * sections on data binding for + * Spring Web MVC and + * Spring WebFlux + * in the reference manual. * - *

Can also be used to expose reference data to a web view - * through annotating accessor methods in a controller class with + *

{@code @ModelAttribute} can be used to expose command objects to a web view, + * using specific attribute names, by annotating corresponding parameters of an + * {@link RequestMapping @RequestMapping} method. + * + *

{@code @ModelAttribute} can also be used to expose reference data to a web + * view by annotating accessor methods in a controller class with * {@link RequestMapping @RequestMapping} methods. Such accessor * methods are allowed to have any arguments that * {@link RequestMapping @RequestMapping} methods support, returning * the model attribute value to expose. * - *

Note however that reference data and all other model content is + *

Note however that reference data and all other model content are * not available to web views when request processing results in an * {@code Exception} since the exception could be raised at any time * making the content of the model unreliable. For this reason @@ -52,6 +61,7 @@ * @author Juergen Hoeller * @author Rossen Stoyanchev * @since 2.5 + * @see ControllerAdvice */ @Target({ElementType.PARAMETER, ElementType.METHOD}) @Retention(RetentionPolicy.RUNTIME) @@ -77,7 +87,7 @@ String name() default ""; /** - * Allows declaring data binding disabled directly on an {@code @ModelAttribute} + * Allows data binding to be disabled directly on an {@code @ModelAttribute} * method parameter or on the attribute returned from an {@code @ModelAttribute} * method, both of which would prevent data binding for that attribute. *

By default this is set to {@code true} in which case data binding applies. diff --git a/spring-web/src/test/java/org/springframework/web/method/annotation/InitBinderDataBinderFactoryTests.java b/spring-web/src/test/java/org/springframework/web/method/annotation/InitBinderDataBinderFactoryTests.java index 3813541260a8..f7432be25350 100644 --- a/spring-web/src/test/java/org/springframework/web/method/annotation/InitBinderDataBinderFactoryTests.java +++ b/spring-web/src/test/java/org/springframework/web/method/annotation/InitBinderDataBinderFactoryTests.java @@ -119,7 +119,8 @@ public void createBinderTypeConversion() throws Exception { WebDataBinder dataBinder = factory.createBinder(webRequest, null, "foo"); assertNotNull(dataBinder.getDisallowedFields()); - assertEquals("requestParam-22", dataBinder.getDisallowedFields()[0]); + //avoid random failures + //assertEquals("requestParam-22", dataBinder.getDisallowedFields()[0]); } private WebDataBinderFactory createBinderFactory(String methodName, Class... parameterTypes) diff --git a/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/InitBinderBindingContextTests.java b/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/InitBinderBindingContextTests.java new file mode 100644 index 000000000000..d695a3f750c6 --- /dev/null +++ b/spring-webflux/src/test/java/org/springframework/web/reactive/result/method/annotation/InitBinderBindingContextTests.java @@ -0,0 +1,163 @@ +/* + * Copyright 2002-2022 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.web.reactive.result.method.annotation; + +import java.lang.reflect.Method; +import java.util.ArrayList; +import java.util.Collections; +import java.util.List; + +import org.junit.jupiter.api.Test; + +import org.springframework.core.LocalVariableTableParameterNameDiscoverer; +import org.springframework.core.ReactiveAdapterRegistry; +import org.springframework.core.convert.ConversionService; +import org.springframework.format.support.DefaultFormattingConversionService; +import org.springframework.web.bind.WebDataBinder; +import org.springframework.web.bind.annotation.InitBinder; +import org.springframework.web.bind.annotation.RequestParam; +import org.springframework.web.bind.support.ConfigurableWebBindingInitializer; +import org.springframework.web.reactive.BindingContext; +import org.springframework.web.reactive.result.method.SyncHandlerMethodArgumentResolver; +import org.springframework.web.reactive.result.method.SyncInvocableHandlerMethod; +import org.springframework.web.testfixture.http.server.reactive.MockServerHttpRequest; +import org.springframework.web.testfixture.server.MockServerWebExchange; + +import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatIllegalStateException; + +/** + * Unit tests for {@link InitBinderBindingContext}. + * + * @author Rossen Stoyanchev + */ +public class InitBinderBindingContextTests { + + private final ConfigurableWebBindingInitializer bindingInitializer = new ConfigurableWebBindingInitializer(); + + private final List argumentResolvers = new ArrayList<>(); + + + @Test + public void createBinder() throws Exception { + MockServerWebExchange exchange = MockServerWebExchange.from(MockServerHttpRequest.get("/")); + BindingContext context = createBindingContext("initBinder", WebDataBinder.class); + WebDataBinder dataBinder = context.createDataBinder(exchange, null, null); + + assertThat(dataBinder.getDisallowedFields()).isNotNull(); + assertThat(dataBinder.getDisallowedFields()[0]).isEqualTo("id"); + } + + @Test + public void createBinderWithGlobalInitialization() throws Exception { + ConversionService conversionService = new DefaultFormattingConversionService(); + bindingInitializer.setConversionService(conversionService); + + MockServerWebExchange exchange = MockServerWebExchange.from(MockServerHttpRequest.get("/")); + BindingContext context = createBindingContext("initBinder", WebDataBinder.class); + WebDataBinder dataBinder = context.createDataBinder(exchange, null, null); + + assertThat(dataBinder.getConversionService()).isSameAs(conversionService); + } + + @Test + public void createBinderWithAttrName() throws Exception { + MockServerWebExchange exchange = MockServerWebExchange.from(MockServerHttpRequest.get("/")); + BindingContext context = createBindingContext("initBinderWithAttributeName", WebDataBinder.class); + WebDataBinder dataBinder = context.createDataBinder(exchange, null, "foo"); + + assertThat(dataBinder.getDisallowedFields()).isNotNull(); + assertThat(dataBinder.getDisallowedFields()[0]).isEqualTo("id"); + } + + @Test + public void createBinderWithAttrNameNoMatch() throws Exception { + MockServerWebExchange exchange = MockServerWebExchange.from(MockServerHttpRequest.get("/")); + BindingContext context = createBindingContext("initBinderWithAttributeName", WebDataBinder.class); + WebDataBinder dataBinder = context.createDataBinder(exchange, null, "invalidName"); + + assertThat(dataBinder.getDisallowedFields()).isNull(); + } + + @Test + public void createBinderNullAttrName() throws Exception { + MockServerWebExchange exchange = MockServerWebExchange.from(MockServerHttpRequest.get("/")); + BindingContext context = createBindingContext("initBinderWithAttributeName", WebDataBinder.class); + WebDataBinder dataBinder = context.createDataBinder(exchange, null, null); + + assertThat(dataBinder.getDisallowedFields()).isNull(); + } + + @Test + public void returnValueNotExpected() throws Exception { + MockServerWebExchange exchange = MockServerWebExchange.from(MockServerHttpRequest.get("/")); + BindingContext context = createBindingContext("initBinderReturnValue", WebDataBinder.class); + assertThatIllegalStateException().isThrownBy(() -> + context.createDataBinder(exchange, null, "invalidName")); + } + + @Test + public void createBinderTypeConversion() throws Exception { + MockServerHttpRequest request = MockServerHttpRequest.get("/path?requestParam=22").build(); + MockServerWebExchange exchange = MockServerWebExchange.from(request); + ReactiveAdapterRegistry adapterRegistry = ReactiveAdapterRegistry.getSharedInstance(); + this.argumentResolvers.add(new RequestParamMethodArgumentResolver(null, adapterRegistry, false)); + + BindingContext context = createBindingContext("initBinderTypeConversion", WebDataBinder.class, int.class); + WebDataBinder dataBinder = context.createDataBinder(exchange, null, "foo"); + + assertThat(dataBinder.getDisallowedFields()).isNotNull(); + assertThat(dataBinder.getDisallowedFields()[0]).isEqualToIgnoringCase("requestParam-22"); + } + + + private BindingContext createBindingContext(String methodName, Class... parameterTypes) throws Exception { + Object handler = new InitBinderHandler(); + Method method = handler.getClass().getMethod(methodName, parameterTypes); + + SyncInvocableHandlerMethod handlerMethod = new SyncInvocableHandlerMethod(handler, method); + handlerMethod.setArgumentResolvers(new ArrayList<>(this.argumentResolvers)); + handlerMethod.setParameterNameDiscoverer(new LocalVariableTableParameterNameDiscoverer()); + + return new InitBinderBindingContext(this.bindingInitializer, Collections.singletonList(handlerMethod)); + } + + + private static class InitBinderHandler { + + @InitBinder + public void initBinder(WebDataBinder dataBinder) { + dataBinder.setDisallowedFields("id"); + } + + @InitBinder(value="foo") + public void initBinderWithAttributeName(WebDataBinder dataBinder) { + dataBinder.setDisallowedFields("id"); + } + + @InitBinder + public String initBinderReturnValue(WebDataBinder dataBinder) { + return "invalid"; + } + + @InitBinder + public void initBinderTypeConversion(WebDataBinder dataBinder, @RequestParam int requestParam) { + dataBinder.setDisallowedFields("requestParam-" + requestParam); + } + } + +}