diff --git a/framework-docs/modules/ROOT/pages/core/validation/data-binding.adoc b/framework-docs/modules/ROOT/pages/core/validation/data-binding.adoc index c0ca55f2418..cd13e7ea599 100644 --- a/framework-docs/modules/ROOT/pages/core/validation/data-binding.adoc +++ b/framework-docs/modules/ROOT/pages/core/validation/data-binding.adoc @@ -61,7 +61,8 @@ corresponding implementation (`BeanWrapperImpl`). As quoted from the javadoc, th `BeanWrapper` offers functionality to set and get property values (individually or in bulk), get property descriptors, and query properties to determine if they are readable or writable. Also, the `BeanWrapper` offers support for nested properties, -enabling the setting of properties on sub-properties to an unlimited depth. The +enabling the setting of properties on sub-properties up to a +<>. The `BeanWrapper` also supports the ability to add standard JavaBeans `PropertyChangeListeners` and `VetoableChangeListeners`, without the need for supporting code in the target class. Last but not least, the `BeanWrapper` provides support for setting indexed properties. @@ -236,6 +237,48 @@ Kotlin:: ====== +[[data-binding-nested-path-depth]] +=== Maximum Nesting Depth for Nested Property Paths + +A nested property path such as `managingDirector.salary` is resolved recursively, one +level per nested property. The nesting depth of a property path therefore corresponds to +the number of intermediate properties that must be traversed in order to reach the final +property -- for example, `address.country.name` has a nesting depth of 2, since the +`address` and `country` properties must be traversed in order to reach the `name` +property. + +The nesting depth of a property path cannot exceed 100 by default; however, the +`maxNestedPathDepth` value is configurable. You can specify a custom value via +`setMaxNestedPathDepth(...)` on a `ConfigurablePropertyAccessor` such as +`BeanWrapperImpl`, or on a `DataBinder` -- and therefore also on a `WebDataBinder`, for +example within an `@InitBinder` method. If a property path exceeds the configured limit, +an `InvalidPropertyException` is thrown. Specify `0` to disable support for nested +property paths altogether, while continuing to allow simple, indexed, and mapped property +access -- for example, `name`, `accounts[2]`, or `accounts[KEY]`. + +Note that this limit applies to property binding as well as to +<> via `DataBinder.construct`, +since a constructor parameter which is itself an object is constructed recursively +through a nested property path. + +[NOTE] +==== +Without such a limit, a sufficiently deeply nested property path can drive the recursive +resolution of nested property paths to exhaust the current thread's call stack, resulting +in a `StackOverflowError` instead of a descriptive exception. + +The `maxNestedPathDepth` limit improves diagnostics for that common case by converting +it into a clear `InvalidPropertyException`; however, it is not a guaranteed defense against +`StackOverflowError` under every possible JVM thread stack size configuration, since the +amount of stack space consumed per level of nesting depends on the JVM, its JIT +compilation state, and the platform. + +When binding untrusted input, you should additionally constrain binding to the expected +input as described in +xref:web/webmvc/mvc-data-binding.adoc#mvc-data-binding-design[Model Design]. +==== + + [[data-binding-conversion]] == ``PropertyEditor``s diff --git a/spring-beans/src/main/java/org/springframework/beans/AbstractNestablePropertyAccessor.java b/spring-beans/src/main/java/org/springframework/beans/AbstractNestablePropertyAccessor.java index 0f48323807b..29996a6c912 100644 --- a/spring-beans/src/main/java/org/springframework/beans/AbstractNestablePropertyAccessor.java +++ b/spring-beans/src/main/java/org/springframework/beans/AbstractNestablePropertyAccessor.java @@ -85,6 +85,9 @@ public abstract class AbstractNestablePropertyAccessor extends AbstractPropertyA /** Map with cached nested Accessors: nested path -> Accessor instance. */ private @Nullable Map nestedPropertyAccessors; + /** The number of nested properties traversed to reach the wrapped object. */ + private int nestedPathDepth; + /** * Create a new empty accessor. Wrapped instance needs to be set afterwards. @@ -150,6 +153,7 @@ public abstract class AbstractNestablePropertyAccessor extends AbstractPropertyA setExtractOldValueForEditor(parent.isExtractOldValueForEditor()); setAutoGrowNestedPaths(parent.isAutoGrowNestedPaths()); setAutoGrowCollectionLimit(parent.getAutoGrowCollectionLimit()); + setMaxNestedPathDepth(parent.getMaxNestedPathDepth()); setConversionService(parent.getConversionService()); } @@ -176,6 +180,7 @@ public abstract class AbstractNestablePropertyAccessor extends AbstractPropertyA this.nestedPath = (nestedPath != null ? nestedPath : ""); this.rootObject = (!this.nestedPath.isEmpty() ? rootObject : this.wrappedObject); this.nestedPropertyAccessors = null; + this.nestedPathDepth = 0; this.typeConverterDelegate = new TypeConverterDelegate(this, this.wrappedObject); } @@ -798,6 +803,10 @@ public abstract class AbstractNestablePropertyAccessor extends AbstractPropertyA /** * Recursively navigate to return a property accessor for the nested property path. + *

The default implementation rejects a property path which contains unbalanced + * brackets as well as one which exceeds the {@linkplain #getMaxNestedPathDepth() + * maximum nesting depth}. An override which does not delegate to {@code super} is + * therefore responsible for performing equivalent validation itself. * @param propertyPath property path, which may be nested * @return a property accessor for the target bean */ @@ -809,6 +818,11 @@ public abstract class AbstractNestablePropertyAccessor extends AbstractPropertyA int pos = PropertyAccessorUtils.getFirstNestedPropertySeparatorIndex(propertyPath); // Handle nested properties recursively. if (pos > -1) { + int maxNestedPathDepth = getMaxNestedPathDepth(); + if (this.nestedPathDepth >= maxNestedPathDepth) { + throw new InvalidPropertyException(getRootClass(), this.nestedPath + propertyPath, + "Nesting depth of property path exceeds the maximum of " + maxNestedPathDepth); + } String nestedProperty = propertyPath.substring(0, pos); String nestedPath = propertyPath.substring(pos + 1); AbstractNestablePropertyAccessor nestedPa = getNestedPropertyAccessor(nestedProperty); @@ -853,6 +867,10 @@ public abstract class AbstractNestablePropertyAccessor extends AbstractPropertyA logger.trace("Creating new nested " + getClass().getSimpleName() + " for property '" + canonicalName + "'"); } nestedPa = newNestedPropertyAccessor(value, this.nestedPath + canonicalName + NESTED_PROPERTY_SEPARATOR); + // Track the nesting depth here rather than in a constructor, so that the + // depth is assigned even if a subclass creates the nested property accessor + // without copying the configuration of this accessor. + nestedPa.nestedPathDepth = this.nestedPathDepth + 1; // Inherit all type-specific PropertyEditors. copyDefaultEditorsTo(nestedPa); copyCustomEditorsTo(nestedPa, canonicalName); diff --git a/spring-beans/src/main/java/org/springframework/beans/AbstractPropertyAccessor.java b/spring-beans/src/main/java/org/springframework/beans/AbstractPropertyAccessor.java index 7f00c47698d..7bacca2d12f 100644 --- a/spring-beans/src/main/java/org/springframework/beans/AbstractPropertyAccessor.java +++ b/spring-beans/src/main/java/org/springframework/beans/AbstractPropertyAccessor.java @@ -23,13 +23,17 @@ import java.util.Map; import org.jspecify.annotations.Nullable; +import org.springframework.util.Assert; + /** * Abstract implementation of the {@link PropertyAccessor} interface. - * Provides base implementations of all convenience methods, with the + * + *

Provides base implementations of all convenience methods, with the * implementation of actual property access left to subclasses. * * @author Juergen Hoeller * @author Stephane Nicoll + * @author Sam Brannen * @since 2.0 * @see #getPropertyValue * @see #setPropertyValue @@ -42,6 +46,8 @@ public abstract class AbstractPropertyAccessor extends TypeConverterSupport impl private int autoGrowCollectionLimit = Integer.MAX_VALUE; + private int maxNestedPathDepth = DEFAULT_MAX_NESTED_PATH_DEPTH; + boolean suppressNotWritablePropertyException = false; @@ -75,6 +81,17 @@ public abstract class AbstractPropertyAccessor extends TypeConverterSupport impl return this.autoGrowCollectionLimit; } + @Override + public void setMaxNestedPathDepth(int maxNestedPathDepth) { + Assert.isTrue(maxNestedPathDepth >= 0, "'maxNestedPathDepth' must not be negative"); + this.maxNestedPathDepth = maxNestedPathDepth; + } + + @Override + public int getMaxNestedPathDepth() { + return this.maxNestedPathDepth; + } + @Override public void setPropertyValue(PropertyValue pv) throws BeansException { diff --git a/spring-beans/src/main/java/org/springframework/beans/ConfigurablePropertyAccessor.java b/spring-beans/src/main/java/org/springframework/beans/ConfigurablePropertyAccessor.java index bcf6c3c86d2..ff7148160c4 100644 --- a/spring-beans/src/main/java/org/springframework/beans/ConfigurablePropertyAccessor.java +++ b/spring-beans/src/main/java/org/springframework/beans/ConfigurablePropertyAccessor.java @@ -21,19 +21,39 @@ import org.jspecify.annotations.Nullable; import org.springframework.core.convert.ConversionService; /** - * Interface that encapsulates configuration methods for a PropertyAccessor. - * Also extends the PropertyEditorRegistry interface, which defines methods - * for PropertyEditor management. + * Interface that encapsulates configuration methods for a {@link PropertyAccessor}. + * + *

Also extends the {@link PropertyEditorRegistry} interface, which defines methods + * for {@link java.beans.PropertyEditor} management. * *

Serves as base interface for {@link BeanWrapper}. * * @author Juergen Hoeller * @author Stephane Nicoll + * @author Sam Brannen * @since 2.0 * @see BeanWrapper */ public interface ConfigurablePropertyAccessor extends PropertyAccessor, PropertyEditorRegistry, TypeConverter { + /** + * Default maximum nesting depth permitted for a nested property path: {@value}. + *

This limit guards against deeply nested property paths that could otherwise + * drive the recursive resolution of a nested property path to exhaust the current + * thread's call stack. + *

NOTE: This limit improves diagnostics for the common case + * by converting what would otherwise be an opaque {@link StackOverflowError} + * into a descriptive {@link InvalidPropertyException}, but it is not + * a guaranteed defense against {@code StackOverflowError} under every possible + * JVM thread stack size configuration. The amount of stack space consumed per + * level of nesting depends on the JVM, its current JIT compilation state, and + * the platform. + * @since 7.1 + * @see #setMaxNestedPathDepth(int) + */ + int DEFAULT_MAX_NESTED_PATH_DEPTH = 100; + + /** * Specify a {@link ConversionService} to use for converting * property values, as an alternative to JavaBeans PropertyEditors. @@ -87,4 +107,23 @@ public interface ConfigurablePropertyAccessor extends PropertyAccessor, Property */ int getAutoGrowCollectionLimit(); + /** + * Specify the maximum nesting depth permitted for a nested property path. + *

The nesting depth corresponds to the number of intermediate properties + * traversed to reach the final property — for example, + * {@code "address.country.name"} has a nesting depth of 2. + *

Specify {@code 0} to disable nested property paths altogether, while + * still allowing simple, indexed, and mapped property access. + *

Default is {@link #DEFAULT_MAX_NESTED_PATH_DEPTH}. + * @param maxNestedPathDepth the maximum nesting depth; must not be negative + * @since 7.1 + */ + void setMaxNestedPathDepth(int maxNestedPathDepth); + + /** + * Return the maximum nesting depth permitted for a nested property path. + * @since 7.1 + */ + int getMaxNestedPathDepth(); + } diff --git a/spring-beans/src/test/java/org/springframework/beans/AbstractPropertyAccessorTests.java b/spring-beans/src/test/java/org/springframework/beans/AbstractPropertyAccessorTests.java index 3d04fe0a8b7..330fca4e4c5 100644 --- a/spring-beans/src/test/java/org/springframework/beans/AbstractPropertyAccessorTests.java +++ b/spring-beans/src/test/java/org/springframework/beans/AbstractPropertyAccessorTests.java @@ -34,7 +34,9 @@ import java.util.SortedSet; import java.util.TreeMap; import java.util.TreeSet; +import org.assertj.core.api.ThrowableAssert.ThrowingCallable; import org.jspecify.annotations.Nullable; +import org.junit.jupiter.api.Nested; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.ValueSource; @@ -58,6 +60,7 @@ import static org.assertj.core.api.Assertions.assertThat; import static org.assertj.core.api.Assertions.assertThatExceptionOfType; import static org.assertj.core.api.Assertions.assertThatIllegalArgumentException; import static org.assertj.core.api.Assertions.within; +import static org.springframework.beans.ConfigurablePropertyAccessor.DEFAULT_MAX_NESTED_PATH_DEPTH; /** * Shared tests for property accessors. @@ -226,10 +229,7 @@ abstract class AbstractPropertyAccessorTests { @Test void getAnotherNestedDeepProperty() { - ITestBean target = new TestBean("rod", 31); - ITestBean kerry = new TestBean("kerry", 35); - target.setSpouse(kerry); - kerry.setSpouse(target); + ITestBean target = createSpouseCycle(); AbstractPropertyAccessor accessor = createAccessor(target); Integer KA = (Integer) accessor.getPropertyValue("spouse.age"); assertThat(KA).as("kerry is 35").isEqualTo(35); @@ -1611,10 +1611,128 @@ abstract class AbstractPropertyAccessorTests { } + @Nested // gh-37252 + class MaxNestedPathDepthTests { + + @ParameterizedTest + @ValueSource(ints = {-1, Integer.MIN_VALUE}) + void setMaxNestedPathDepthPreconditions(int depth) { + AbstractPropertyAccessor accessor = createAccessor(createSpouseCycle()); + + assertThatIllegalArgumentException() + .isThrownBy(() -> accessor.setMaxNestedPathDepth(depth)) + .withMessage("'maxNestedPathDepth' must not be negative"); + } + + @Test + void maxNestedPathDepthOfZeroOnlyDisablesNestedPropertyPaths() { + AbstractPropertyAccessor accessor = createAccessor(createSpouseCycle()); + accessor.setMaxNestedPathDepth(0); + + // Simple property access is still supported. + assertThat(accessor.getPropertyValue("name")).isEqualTo("rod"); + accessor.setPropertyValue("name", "ROD"); + assertThat(accessor.getPropertyValue("name")).isEqualTo("ROD"); + + // As is indexed property access. + accessor.setPropertyValue("stringArray", new String[] {"a", "b"}); + assertThat(accessor.getPropertyValue("stringArray[1]")).isEqualTo("b"); + accessor.setPropertyValue("stringArray[1]", "B"); + assertThat(accessor.getPropertyValue("stringArray[1]")).isEqualTo("B"); + + // As is mapped property access. + accessor.setPropertyValue("someMap[key]", "value"); + assertThat(accessor.getPropertyValue("someMap[key]")).isEqualTo("value"); + + // Nested property paths, however, are not. + assertNestedPathDepthExceeded(() -> accessor.getPropertyValue(nestedSpousePath(1)), 0); + assertNestedPathDepthExceeded(() -> accessor.getPropertyValue(nestedSpousePath(2)), 0); + assertNestedPathDepthExceeded(() -> accessor.setPropertyValue(nestedSpousePath(1), "Joe"), 0); + assertNestedPathDepthExceeded(() -> accessor.setPropertyValue(nestedSpousePath(2), "Joe"), 0); + } + + @Test + void defaultMaxNestedPathDepth() { + AbstractPropertyAccessor accessor = createAccessor(createSpouseCycle()); + + assertThat(accessor.getMaxNestedPathDepth()).isEqualTo(DEFAULT_MAX_NESTED_PATH_DEPTH); + + // depth == max + assertThat(accessor.getPropertyValue(nestedSpousePath(DEFAULT_MAX_NESTED_PATH_DEPTH))).isEqualTo("rod"); + + // depth > max + assertNestedPathDepthExceeded( + () -> accessor.getPropertyValue(nestedSpousePath(DEFAULT_MAX_NESTED_PATH_DEPTH + 1))); + } + + @Test + void getNestedPropertyWithCustomMaxNestedPathDepth() { + AbstractPropertyAccessor accessor = createAccessor(createSpouseCycle()); + accessor.setMaxNestedPathDepth(10); + + // depth < max + assertThat(accessor.getPropertyValue(nestedSpousePath(9))).isEqualTo("kerry"); + + // depth == max + assertThat(accessor.getPropertyValue(nestedSpousePath(10))).isEqualTo("rod"); + + // depth > max + assertNestedPathDepthExceeded(() -> accessor.getPropertyValue(nestedSpousePath(11)), 10); + assertNestedPathDepthExceeded(() -> accessor.getPropertyValue(nestedSpousePath(100)), 10); + } + + @Test + void setNestedPropertyExceedingMaxNestedPathDepth() { + AbstractPropertyAccessor accessor = createAccessor(createSpouseCycle()); + accessor.setMaxNestedPathDepth(10); + + assertNestedPathDepthExceeded(() -> accessor.setPropertyValue(nestedSpousePath(11), "Jane"), 10); + } + + @Test + void maxNestedPathDepthProtectsAgainstStackOverflow() { + AbstractPropertyAccessor accessor = createAccessor(createSpouseCycle()); + accessor.setAutoGrowNestedPaths(true); + + assertNestedPathDepthExceeded(() -> accessor.getPropertyValue(nestedSpousePath(100_000))); + assertNestedPathDepthExceeded(() -> accessor.setPropertyValue(nestedSpousePath(100_000), "Jane")); + } + + + private static String nestedSpousePath(int depth) { + return "spouse.".repeat(depth) + "name"; + } + + private static void assertNestedPathDepthExceeded(ThrowingCallable throwingCallable) { + assertNestedPathDepthExceeded(throwingCallable, DEFAULT_MAX_NESTED_PATH_DEPTH); + } + + private static void assertNestedPathDepthExceeded(ThrowingCallable throwingCallable, int maxDepth) { + assertThatExceptionOfType(InvalidPropertyException.class) + .isThrownBy(throwingCallable) + .withMessageEndingWith("Nesting depth of property path exceeds the maximum of " + maxDepth); + } + } + + private Person createPerson(String name, String city, String country) { return new Person(name, new Address(city, country)); } + /** + * Create two beans that are each other's spouse, so that a nested property + * path consisting of any number of {@code spouse} segments can be traversed. + * @return a {@code "rod"} bean, whose spouse is a {@code "kerry"} bean + */ + private static ITestBean createSpouseCycle() { + ITestBean rod = new TestBean("rod", 31); + ITestBean kerry = new TestBean("kerry", 35); + rod.setSpouse(kerry); + kerry.setSpouse(rod); + return rod; + } + + @SuppressWarnings("unused") private static class Simple { diff --git a/spring-beans/src/test/java/org/springframework/beans/BeanWrapperTests.java b/spring-beans/src/test/java/org/springframework/beans/BeanWrapperTests.java index 1cd9b19f489..f0d6892eb08 100644 --- a/spring-beans/src/test/java/org/springframework/beans/BeanWrapperTests.java +++ b/spring-beans/src/test/java/org/springframework/beans/BeanWrapperTests.java @@ -20,6 +20,7 @@ import java.time.Duration; import java.util.Collections; import java.util.Map; import java.util.Optional; +import java.util.concurrent.atomic.AtomicInteger; import org.junit.jupiter.api.Test; @@ -299,6 +300,20 @@ class BeanWrapperTests extends AbstractPropertyAccessorTests { .satisfies(ex -> assertThat(ex.getPossibleMatches()).isNull()); } + @Test // gh-37252 + void overriddenGetPropertyAccessorForPropertyPathIsInvokedForEachNestedLevel() { + TestBean rod = new TestBean("rod", 31); + TestBean kerry = new TestBean("kerry", 35); + rod.setSpouse(kerry); + kerry.setSpouse(rod); + + CountingBeanWrapper accessor = new CountingBeanWrapper(rod); + + assertThat(accessor.getPropertyValue("spouse.spouse.name")).isEqualTo("rod"); + // Once for "spouse.spouse.name", once for "spouse.name", and once for "name". + assertThat(accessor.invocations).hasValue(3); + } + private interface BaseProperty { @@ -446,4 +461,35 @@ class BeanWrapperTests extends AbstractPropertyAccessorTests { } } + /** + * A {@link BeanWrapperImpl} which tracks how often + * {@link #getPropertyAccessorForPropertyPath(String)} is invoked, in order to + * verify that an override is applied to each level of a nested property path. + */ + private static class CountingBeanWrapper extends BeanWrapperImpl { + + private final AtomicInteger invocations; + + CountingBeanWrapper(Object target) { + super(target); + this.invocations = new AtomicInteger(); + } + + private CountingBeanWrapper(Object object, String nestedPath, CountingBeanWrapper parent) { + super(object, nestedPath, parent.getRootInstance()); + this.invocations = parent.invocations; + } + + @Override + protected BeanWrapperImpl newNestedPropertyAccessor(Object object, String nestedPath) { + return new CountingBeanWrapper(object, nestedPath, this); + } + + @Override + protected AbstractNestablePropertyAccessor getPropertyAccessorForPropertyPath(String propertyPath) { + this.invocations.incrementAndGet(); + return super.getPropertyAccessorForPropertyPath(propertyPath); + } + } + } diff --git a/spring-context/src/main/java/org/springframework/validation/BeanPropertyBindingResult.java b/spring-context/src/main/java/org/springframework/validation/BeanPropertyBindingResult.java index c20da306f3a..1ebd328757a 100644 --- a/spring-context/src/main/java/org/springframework/validation/BeanPropertyBindingResult.java +++ b/spring-context/src/main/java/org/springframework/validation/BeanPropertyBindingResult.java @@ -23,6 +23,7 @@ import org.jspecify.annotations.Nullable; import org.springframework.beans.BeanWrapper; import org.springframework.beans.ConfigurablePropertyAccessor; import org.springframework.beans.PropertyAccessorFactory; +import org.springframework.util.Assert; /** * Default implementation of the {@link Errors} and {@link BindingResult} @@ -36,6 +37,7 @@ import org.springframework.beans.PropertyAccessorFactory; * {@link DataBinder#getBindingResult()}. * * @author Juergen Hoeller + * @author Sam Brannen * @since 2.0 * @see DataBinder#getBindingResult() * @see DataBinder#initBeanPropertyAccess() @@ -50,6 +52,8 @@ public class BeanPropertyBindingResult extends AbstractPropertyBindingResult imp private final int autoGrowCollectionLimit; + private final int maxNestedPathDepth; + private transient @Nullable BeanWrapper beanWrapper; @@ -68,14 +72,34 @@ public class BeanPropertyBindingResult extends AbstractPropertyBindingResult imp * @param objectName the name of the target object * @param autoGrowNestedPaths whether to "auto-grow" a nested path that contains a null value * @param autoGrowCollectionLimit the limit for array and collection auto-growing + * @since 3.1 */ public BeanPropertyBindingResult(@Nullable Object target, String objectName, boolean autoGrowNestedPaths, int autoGrowCollectionLimit) { + this(target, objectName, autoGrowNestedPaths, autoGrowCollectionLimit, + ConfigurablePropertyAccessor.DEFAULT_MAX_NESTED_PATH_DEPTH); + } + + /** + * Create a new {@code BeanPropertyBindingResult} for the given target. + * @param target the target bean to bind onto + * @param objectName the name of the target object + * @param autoGrowNestedPaths whether to "auto-grow" a nested path that contains a null value + * @param autoGrowCollectionLimit the limit for array and collection auto-growing + * @param maxNestedPathDepth the maximum nesting depth permitted for a nested + * property path; must not be negative + * @since 7.1 + */ + public BeanPropertyBindingResult(@Nullable Object target, String objectName, + boolean autoGrowNestedPaths, int autoGrowCollectionLimit, int maxNestedPathDepth) { + super(objectName); + Assert.isTrue(maxNestedPathDepth >= 0, "'maxNestedPathDepth' must not be negative"); this.target = target; this.autoGrowNestedPaths = autoGrowNestedPaths; this.autoGrowCollectionLimit = autoGrowCollectionLimit; + this.maxNestedPathDepth = maxNestedPathDepth; } @@ -96,6 +120,7 @@ public class BeanPropertyBindingResult extends AbstractPropertyBindingResult imp this.beanWrapper.setExtractOldValueForEditor(true); this.beanWrapper.setAutoGrowNestedPaths(this.autoGrowNestedPaths); this.beanWrapper.setAutoGrowCollectionLimit(this.autoGrowCollectionLimit); + this.beanWrapper.setMaxNestedPathDepth(this.maxNestedPathDepth); } return this.beanWrapper; } 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 52b653bc9fb..009fbfe3fa7 100644 --- a/spring-context/src/main/java/org/springframework/validation/DataBinder.java +++ b/spring-context/src/main/java/org/springframework/validation/DataBinder.java @@ -41,6 +41,7 @@ import org.jspecify.annotations.Nullable; import org.springframework.beans.BeanInstantiationException; import org.springframework.beans.BeanUtils; import org.springframework.beans.ConfigurablePropertyAccessor; +import org.springframework.beans.InvalidPropertyException; import org.springframework.beans.MutablePropertyValues; import org.springframework.beans.PropertyAccessException; import org.springframework.beans.PropertyAccessorUtils; @@ -167,6 +168,8 @@ public class DataBinder implements PropertyEditorRegistry, TypeConverter { private int autoGrowCollectionLimit = DEFAULT_AUTO_GROW_COLLECTION_LIMIT; + private int maxNestedPathDepth = ConfigurablePropertyAccessor.DEFAULT_MAX_NESTED_PATH_DEPTH; + private String @Nullable [] allowedFields; private String @Nullable [] disallowedFields; @@ -292,6 +295,37 @@ public class DataBinder implements PropertyEditorRegistry, TypeConverter { return this.autoGrowCollectionLimit; } + /** + * Specify the maximum nesting depth permitted for a nested property path. + *

The nesting depth of a property path corresponds to the number of + * intermediate properties that must be traversed in order to reach the final + * property. For example, {@code "address.country.name"} has a nesting depth + * of 2, since the {@code address} and {@code country} properties must be + * traversed in order to reach the {@code name} property. + *

Default is {@link ConfigurablePropertyAccessor#DEFAULT_MAX_NESTED_PATH_DEPTH}. + *

Applies to setter and field injection via {@link #bind(PropertyValues)} + * as well as to constructor binding via {@link #construct}, since a + * constructor parameter which is itself an object is constructed recursively + * through a nested property path. + * @param maxNestedPathDepth the maximum nesting depth; must not be negative + * @since 7.1 + * @see ConfigurablePropertyAccessor#setMaxNestedPathDepth(int) + */ + public void setMaxNestedPathDepth(int maxNestedPathDepth) { + Assert.state(this.bindingResult == null, + "DataBinder is already initialized - call setMaxNestedPathDepth before other configuration methods"); + Assert.isTrue(maxNestedPathDepth >= 0, "'maxNestedPathDepth' must not be negative"); + this.maxNestedPathDepth = maxNestedPathDepth; + } + + /** + * Return the maximum nesting depth permitted for a nested property path. + * @since 7.1 + */ + public int getMaxNestedPathDepth() { + return this.maxNestedPathDepth; + } + /** * Initialize standard JavaBean property access for this DataBinder. *

This is the default; an explicit call just leads to eager initialization. @@ -310,8 +344,8 @@ public class DataBinder implements PropertyEditorRegistry, TypeConverter { * @since 4.2.1 */ protected AbstractPropertyBindingResult createBeanPropertyBindingResult() { - BeanPropertyBindingResult result = new BeanPropertyBindingResult(getTarget(), - getObjectName(), isAutoGrowNestedPaths(), getAutoGrowCollectionLimit()); + BeanPropertyBindingResult result = new BeanPropertyBindingResult(getTarget(), getObjectName(), + isAutoGrowNestedPaths(), getAutoGrowCollectionLimit(), getMaxNestedPathDepth()); if (this.conversionService != null) { result.initConversion(this.conversionService); @@ -343,8 +377,8 @@ public class DataBinder implements PropertyEditorRegistry, TypeConverter { * @since 4.2.1 */ protected AbstractPropertyBindingResult createDirectFieldBindingResult() { - DirectFieldBindingResult result = new DirectFieldBindingResult(getTarget(), - getObjectName(), isAutoGrowNestedPaths(), getAutoGrowCollectionLimit()); + DirectFieldBindingResult result = new DirectFieldBindingResult(getTarget(), getObjectName(), + isAutoGrowNestedPaths(), getAutoGrowCollectionLimit(), getMaxNestedPathDepth()); if (this.conversionService != null) { result.initConversion(this.conversionService); @@ -887,7 +921,7 @@ public class DataBinder implements PropertyEditorRegistry, TypeConverter { Assert.state(this.target == null, "Target instance already available"); Assert.state(this.targetType != null, "Target type not set"); - this.target = createObject(this.targetType, "", valueResolver); + this.target = createObject(this.targetType, "", valueResolver, 0); if (!getBindingResult().hasErrors()) { this.bindingResult = null; @@ -897,7 +931,9 @@ public class DataBinder implements PropertyEditorRegistry, TypeConverter { } } - private @Nullable Object createObject(ResolvableType objectType, String nestedPath, ValueResolver valueResolver) { + private @Nullable Object createObject(ResolvableType objectType, String nestedPath, + ValueResolver valueResolver, int depth) { + Class clazz = objectType.resolve(); boolean isOptional = (clazz == Optional.class); clazz = (isOptional ? objectType.resolveGeneric(0) : clazz); @@ -906,6 +942,14 @@ public class DataBinder implements PropertyEditorRegistry, TypeConverter { "Insufficient type information to create instance of " + objectType); } + int maxNestedPathDepth = getMaxNestedPathDepth(); + if (depth > maxNestedPathDepth) { + // The nested path always ends with a separator at this point, since the + // depth can only exceed the limit for a nested constructor argument. + throw new InvalidPropertyException(clazz, nestedPath.substring(0, nestedPath.length() - 1), + "Nesting depth of property path exceeds the maximum of " + maxNestedPathDepth); + } + Object result = null; Constructor ctor = BeanUtils.getResolvableConstructor(clazz); @@ -938,18 +982,18 @@ public class DataBinder implements PropertyEditorRegistry, TypeConverter { if (value == null) { if (List.class.isAssignableFrom(paramType)) { - value = createList(paramPath, paramType, resolvableType, valueResolver); + value = createList(paramPath, paramType, resolvableType, valueResolver, depth); } else if (Map.class.isAssignableFrom(paramType)) { - value = createMap(paramPath, paramType, resolvableType, valueResolver); + value = createMap(paramPath, paramType, resolvableType, valueResolver, depth); } else if (paramType.isArray()) { - value = createArray(paramPath, paramType, resolvableType, valueResolver); + value = createArray(paramPath, paramType, resolvableType, valueResolver, depth); } } if (value == null && shouldConstructArgument(param) && hasValuesFor(paramPath, valueResolver)) { - args[i] = createObject(resolvableType, paramPath + ".", valueResolver); + args[i] = createObject(resolvableType, paramPath + ".", valueResolver, depth + 1); } else { try { @@ -1026,8 +1070,8 @@ public class DataBinder implements PropertyEditorRegistry, TypeConverter { return false; } - private @Nullable List createList( - String paramPath, Class paramType, ResolvableType type, ValueResolver valueResolver) { + private @Nullable List createList(String paramPath, Class paramType, ResolvableType type, + ValueResolver valueResolver, int depth) { ResolvableType elementType = type.getNested(2); SortedSet indexes = getIndexes(paramPath, valueResolver); @@ -1045,14 +1089,14 @@ public class DataBinder implements PropertyEditorRegistry, TypeConverter { for (int index : indexes) { String indexedPath = paramPath + "[" + (index != NO_INDEX ? index : "") + "]"; list.set(Math.max(index, 0), - createIndexedValue(paramPath, paramType, elementType, indexedPath, valueResolver)); + createIndexedValue(paramPath, paramType, elementType, indexedPath, valueResolver, depth)); } return list; } - private @Nullable Map createMap( - String paramPath, Class paramType, ResolvableType type, ValueResolver valueResolver) { + private @Nullable Map createMap(String paramPath, Class paramType, ResolvableType type, + ValueResolver valueResolver, int depth) { ResolvableType elementType = type.getNested(2); Map map = null; @@ -1074,15 +1118,15 @@ public class DataBinder implements PropertyEditorRegistry, TypeConverter { } String indexedPath = name.substring(0, endIdx + 1); - map.put(key, createIndexedValue(paramPath, paramType, elementType, indexedPath, valueResolver)); + map.put(key, createIndexedValue(paramPath, paramType, elementType, indexedPath, valueResolver, depth)); } return map; } @SuppressWarnings("unchecked") - private @Nullable V @Nullable [] createArray( - String paramPath, Class paramType, ResolvableType type, ValueResolver valueResolver) { + private @Nullable V @Nullable [] createArray(String paramPath, Class paramType, ResolvableType type, + ValueResolver valueResolver, int depth) { ResolvableType elementType = type.getNested(2); SortedSet indexes = getIndexes(paramPath, valueResolver); @@ -1097,7 +1141,7 @@ public class DataBinder implements PropertyEditorRegistry, TypeConverter { for (int index : indexes) { String indexedPath = paramPath + "[" + (index != NO_INDEX ? index : "") + "]"; array[Math.max(index, 0)] = - createIndexedValue(paramPath, paramType, elementType, indexedPath, valueResolver); + createIndexedValue(paramPath, paramType, elementType, indexedPath, valueResolver, depth); } return array; @@ -1129,19 +1173,19 @@ public class DataBinder implements PropertyEditorRegistry, TypeConverter { @SuppressWarnings("unchecked") private @Nullable V createIndexedValue( String paramPath, Class containerType, ResolvableType elementType, - String indexedPath, ValueResolver valueResolver) { + String indexedPath, ValueResolver valueResolver, int depth) { Object value = null; Class elementClass = elementType.resolve(Object.class); if (List.class.isAssignableFrom(elementClass)) { - value = createList(indexedPath, elementClass, elementType, valueResolver); + value = createList(indexedPath, elementClass, elementType, valueResolver, depth); } else if (Map.class.isAssignableFrom(elementClass)) { - value = createMap(indexedPath, elementClass, elementType, valueResolver); + value = createMap(indexedPath, elementClass, elementType, valueResolver, depth); } else if (elementClass.isArray()) { - value = createArray(indexedPath, elementClass, elementType, valueResolver); + value = createArray(indexedPath, elementClass, elementType, valueResolver, depth); } else { Object rawValue = valueResolver.resolveValue(indexedPath, elementClass); @@ -1154,7 +1198,7 @@ public class DataBinder implements PropertyEditorRegistry, TypeConverter { } } else { - value = createObject(elementType, indexedPath + ".", valueResolver); + value = createObject(elementType, indexedPath + ".", valueResolver, depth + 1); } } diff --git a/spring-context/src/main/java/org/springframework/validation/DirectFieldBindingResult.java b/spring-context/src/main/java/org/springframework/validation/DirectFieldBindingResult.java index 54e5b9616a0..a303c5eac5d 100644 --- a/spring-context/src/main/java/org/springframework/validation/DirectFieldBindingResult.java +++ b/spring-context/src/main/java/org/springframework/validation/DirectFieldBindingResult.java @@ -20,6 +20,7 @@ import org.jspecify.annotations.Nullable; import org.springframework.beans.ConfigurablePropertyAccessor; import org.springframework.beans.PropertyAccessorFactory; +import org.springframework.util.Assert; /** * Special implementation of the Errors and BindingResult interfaces, @@ -29,6 +30,7 @@ import org.springframework.beans.PropertyAccessorFactory; *

Since Spring 4.1 this implementation is able to traverse nested fields. * * @author Juergen Hoeller + * @author Sam Brannen * @since 2.0 * @see DataBinder#getBindingResult() * @see DataBinder#initDirectFieldAccess() @@ -43,6 +45,8 @@ public class DirectFieldBindingResult extends AbstractPropertyBindingResult { private final int autoGrowCollectionLimit; + private final int maxNestedPathDepth; + private transient @Nullable ConfigurablePropertyAccessor directFieldAccessor; @@ -62,7 +66,8 @@ public class DirectFieldBindingResult extends AbstractPropertyBindingResult { * @param autoGrowNestedPaths whether to "auto-grow" a nested path that contains a null value */ public DirectFieldBindingResult(@Nullable Object target, String objectName, boolean autoGrowNestedPaths) { - this(target, objectName, autoGrowNestedPaths, Integer.MAX_VALUE); + this(target, objectName, autoGrowNestedPaths, Integer.MAX_VALUE, + ConfigurablePropertyAccessor.DEFAULT_MAX_NESTED_PATH_DEPTH); } /** @@ -71,15 +76,19 @@ public class DirectFieldBindingResult extends AbstractPropertyBindingResult { * @param objectName the name of the target object * @param autoGrowNestedPaths whether to "auto-grow" a nested path that contains a null value * @param autoGrowCollectionLimit the limit for array and collection auto-growing + * @param maxNestedPathDepth the maximum nesting depth permitted for a nested + * property path; must not be negative * @since 7.1 */ public DirectFieldBindingResult(@Nullable Object target, String objectName, - boolean autoGrowNestedPaths, int autoGrowCollectionLimit) { + boolean autoGrowNestedPaths, int autoGrowCollectionLimit, int maxNestedPathDepth) { super(objectName); + Assert.isTrue(maxNestedPathDepth >= 0, "'maxNestedPathDepth' must not be negative"); this.target = target; this.autoGrowNestedPaths = autoGrowNestedPaths; this.autoGrowCollectionLimit = autoGrowCollectionLimit; + this.maxNestedPathDepth = maxNestedPathDepth; } @@ -100,6 +109,7 @@ public class DirectFieldBindingResult extends AbstractPropertyBindingResult { this.directFieldAccessor.setExtractOldValueForEditor(true); this.directFieldAccessor.setAutoGrowNestedPaths(this.autoGrowNestedPaths); this.directFieldAccessor.setAutoGrowCollectionLimit(this.autoGrowCollectionLimit); + this.directFieldAccessor.setMaxNestedPathDepth(this.maxNestedPathDepth); } return this.directFieldAccessor; } diff --git a/spring-context/src/test/java/org/springframework/validation/DataBinderConstructTests.java b/spring-context/src/test/java/org/springframework/validation/DataBinderConstructTests.java index 3eac155ad32..1bfb821daf0 100644 --- a/spring-context/src/test/java/org/springframework/validation/DataBinderConstructTests.java +++ b/spring-context/src/test/java/org/springframework/validation/DataBinderConstructTests.java @@ -27,16 +27,20 @@ import jakarta.validation.constraints.NotNull; import org.jspecify.annotations.Nullable; import org.junit.jupiter.api.Test; +import org.springframework.beans.ConfigurablePropertyAccessor; +import org.springframework.beans.InvalidPropertyException; import org.springframework.core.ResolvableType; import org.springframework.format.support.DefaultFormattingConversionService; import org.springframework.util.Assert; import static org.assertj.core.api.Assertions.assertThat; +import static org.assertj.core.api.Assertions.assertThatExceptionOfType; /** * Tests for {@link DataBinder} with constructor binding. * * @author Rossen Stoyanchev + * @author Sam Brannen */ class DataBinderConstructTests { @@ -271,6 +275,35 @@ class DataBinderConstructTests { assertThat(target.integerMapList().get(1)).containsOnly(Map.entry("a", 3), Map.entry("b", 4)); } + @Test // gh-37252 + void maxNestedPathDepth() { + DataBinder binder = initDataBinder(NodeRecord.class); + binder.setMaxNestedPathDepth(2); + + // depth == max + binder.construct(new MapValueResolver(Map.of("next.next.value", "enigma"))); + NodeRecord target = getTarget(binder); + assertThat(target.next().next().value()).isEqualTo("enigma"); + + // depth > max + DataBinder tooDeep = initDataBinder(NodeRecord.class); + tooDeep.setMaxNestedPathDepth(2); + assertThatExceptionOfType(InvalidPropertyException.class) + .isThrownBy(() -> tooDeep.construct(new MapValueResolver(Map.of("next.next.next.value", "enigma")))) + .withMessageEndingWith("Nesting depth of property path exceeds the maximum of 2"); + } + + @Test // gh-37252 + void maxNestedPathDepthProtectsAgainstStackOverflow() { + DataBinder binder = initDataBinder(NodeRecord.class); + + String propertyPath = "next.".repeat(100_000) + "value"; + assertThatExceptionOfType(InvalidPropertyException.class) + .isThrownBy(() -> binder.construct(new MapValueResolver(Map.of(propertyPath, "enigma")))) + .withMessageEndingWith("Nesting depth of property path exceeds the maximum of " + + ConfigurablePropertyAccessor.DEFAULT_MAX_NESTED_PATH_DEPTH); + } + @SuppressWarnings("SameParameterValue") private static DataBinder initDataBinder(Class targetType) { @@ -341,6 +374,14 @@ class DataBinderConstructTests { } + /** + * A self-referential record, for which the nesting depth of a property path is + * bounded by the supplied input rather than by the structure of the type. + */ + private record NodeRecord(@Nullable NodeRecord next, @Nullable String value) { + } + + private record DataClassListRecord(List dataClassList) { } diff --git a/spring-context/src/test/java/org/springframework/validation/DataBinderFieldAccessTests.java b/spring-context/src/test/java/org/springframework/validation/DataBinderFieldAccessTests.java index c2c925b02e3..65bacc9602b 100644 --- a/spring-context/src/test/java/org/springframework/validation/DataBinderFieldAccessTests.java +++ b/spring-context/src/test/java/org/springframework/validation/DataBinderFieldAccessTests.java @@ -155,6 +155,29 @@ class DataBinderFieldAccessTests { binder.bind(outOfBounds)); } + @Test // gh-37252 + void directFieldAccessHonorsMaxNestedPathDepth() { + TestBean rod = new TestBean("rod", 31); + TestBean kerry = new TestBean("kerry", 35); + rod.setSpouse(kerry); + kerry.setSpouse(rod); + + DataBinder binder = new DataBinder(rod); + binder.setMaxNestedPathDepth(2); + binder.initDirectFieldAccess(); + + MutablePropertyValues pvs = new MutablePropertyValues(); + pvs.add("spouse.spouse.name", "Jane"); + binder.bind(pvs); + assertThat(rod.getName()).isEqualTo("Jane"); + + MutablePropertyValues tooDeep = new MutablePropertyValues(); + tooDeep.add("spouse.spouse.spouse.name", "Joe"); + assertThatExceptionOfType(InvalidPropertyException.class) + .isThrownBy(() -> binder.bind(tooDeep)) + .withMessageEndingWith("Nesting depth of property path exceeds the maximum of 2"); + } + @Test void bindingWithErrorsAndCustomEditors() { FieldAccessBean rod = new FieldAccessBean(); 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 bb821e3f4fc..4b2203ab7e2 100644 --- a/spring-context/src/test/java/org/springframework/validation/DataBinderTests.java +++ b/spring-context/src/test/java/org/springframework/validation/DataBinderTests.java @@ -40,6 +40,7 @@ import org.jspecify.annotations.Nullable; import org.junit.jupiter.api.Test; import org.springframework.beans.BeanWrapper; +import org.springframework.beans.ConfigurablePropertyAccessor; import org.springframework.beans.InvalidPropertyException; import org.springframework.beans.MethodInvocationException; import org.springframework.beans.MutablePropertyValues; @@ -2062,6 +2063,51 @@ class DataBinderTests { .withMessageContaining("DataBinder is already initialized - call setAutoGrowCollectionLimit before other configuration methods"); } + @Test // gh-37252 + void defaultMaxNestedPathDepthIsAppliedToPropertyAccessor() { + DataBinder binder = new DataBinder(new TestBean(), "testBean"); + + assertThat(binder.getMaxNestedPathDepth()) + .isEqualTo(ConfigurablePropertyAccessor.DEFAULT_MAX_NESTED_PATH_DEPTH); + assertThat(binder.getInternalBindingResult().getPropertyAccessor().getMaxNestedPathDepth()) + .isEqualTo(ConfigurablePropertyAccessor.DEFAULT_MAX_NESTED_PATH_DEPTH); + } + + @Test // gh-37252 + void setMaxNestedPathDepth() { + TestBean rod = new TestBean("rod", 31); + TestBean kerry = new TestBean("kerry", 35); + rod.setSpouse(kerry); + kerry.setSpouse(rod); + + DataBinder binder = new DataBinder(rod); + binder.setMaxNestedPathDepth(2); + + MutablePropertyValues pvs = new MutablePropertyValues(); + pvs.add("spouse.spouse.name", "Jane"); + binder.bind(pvs); + assertThat(rod.getName()).isEqualTo("Jane"); + + MutablePropertyValues tooDeep = new MutablePropertyValues(); + tooDeep.add("spouse.spouse.spouse.name", "Joe"); + assertThatExceptionOfType(InvalidPropertyException.class) + .isThrownBy(() -> binder.bind(tooDeep)) + .withMessageEndingWith("Nesting depth of property path exceeds the maximum of 2"); + } + + @Test // gh-37252 + void setMaxNestedPathDepthAfterInitialization() { + DataBinder binder = new DataBinder(new TestBean()); + binder.registerCustomEditor(String.class, new StringTrimmerEditor(true)); + + assertThatIllegalStateException() + .isThrownBy(() -> binder.setMaxNestedPathDepth(2)) + .withMessageContaining(""" + DataBinder is already initialized - \ + call setMaxNestedPathDepth before other configuration methods\ + """); + } + @Test // SPR-15009 void setCustomMessageCodesResolverBeforeInitializeBindingResultForBeanPropertyAccess() { TestBean testBean = new TestBean();