Revise "Skip binding entirely when field is not allowed"

This commit reverts the changes made to WebDataBinder's doBind()
implementation in e4d03f6625 and instead implements the skipping logic
directly in checkFieldDefaults(), checkFieldMarkers(), and
adaptEmptyArrayIndices() by preemptively checking if the corresponding
field is allowed.

This commit also improves the Javadoc and adds missing tests.

See gh-36625
Fixes gh-36627

(cherry picked from commit 68c575ab14)

spring-web/src/main/java/org/springframework/web/bind/WebDataBinder.java

@@ -54,6 +54,7 @@ import org.springframework.web.multipart.MultipartFile;
  * @author Juergen Hoeller
  * @author Scott Andrews
  * @author Brian Clozel
+ * @author Sam Brannen
  * @since 1.2
  * @see #registerCustomEditor
  * @see #setAllowedFields
@@ -222,28 +223,28 @@ public class WebDataBinder extends DataBinder {
 	}
 
 	/**
-	 * This implementation performs a field default and marker check
-	 * before delegating to the superclass binding process.
-	 * @see #checkFieldDefaults
-	 * @see #checkFieldMarkers
+	 * This implementation checks default fields and marker fields and then adapts
+	 * empty array indices before delegating to the superclass binding process.
+	 * @see #checkFieldDefaults(MutablePropertyValues)
+	 * @see #checkFieldMarkers(MutablePropertyValues)
+	 * @see #adaptEmptyArrayIndices(MutablePropertyValues)
 	 */
 	@Override
 	protected void doBind(MutablePropertyValues mpvs) {
-		checkAllowedFields(mpvs);
 		checkFieldDefaults(mpvs);
 		checkFieldMarkers(mpvs);
 		adaptEmptyArrayIndices(mpvs);
-		checkRequiredFields(mpvs);
-		applyPropertyValues(mpvs);
+		super.doBind(mpvs);
 	}
 
 	/**
-	 * Check the given property values for field defaults,
-	 * i.e. for fields that start with the field default prefix.
-	 * <p>The existence of a field defaults indicates that the specified
-	 * value should be used if the field is otherwise not present.
+	 * Check the given property values for fields that start with the field default
+	 * prefix.
+	 * <p>The existence of a field default indicates that the specified value should
+	 * be used if the field is allowed and is otherwise not present.
 	 * @param mpvs the property values to be bound (can be modified)
-	 * @see #getFieldDefaultPrefix
+	 * @see #getFieldDefaultPrefix()
+	 * @see #isAllowed(String)
 	 */
 	protected void checkFieldDefaults(MutablePropertyValues mpvs) {
 		String fieldDefaultPrefix = getFieldDefaultPrefix();
@@ -252,7 +253,7 @@ public class WebDataBinder extends DataBinder {
 			for (PropertyValue pv : pvArray) {
 				if (pv.getName().startsWith(fieldDefaultPrefix)) {
 					String field = pv.getName().substring(fieldDefaultPrefix.length());
-					if (getPropertyAccessor().isWritableProperty(field) && !mpvs.contains(field)) {
+					if (isAllowed(field) && getPropertyAccessor().isWritableProperty(field) && !mpvs.contains(field)) {
 						mpvs.add(field, pv.getValue());
 					}
 					mpvs.removePropertyValue(pv);
@@ -262,14 +263,15 @@ public class WebDataBinder extends DataBinder {
 	}
 
 	/**
-	 * Check the given property values for field markers,
-	 * i.e. for fields that start with the field marker prefix.
-	 * <p>The existence of a field marker indicates that the specified
-	 * field existed in the form. If the property values do not contain
-	 * a corresponding field value, the field will be considered as empty
-	 * and will be reset appropriately.
+	 * Check the given property values for fields that start with the field marker
+	 * prefix.
+	 * <p>The existence of a field marker indicates that the specified field existed
+	 * in the form.
+	 * <p>If the field is allowed and the property values do not contain a corresponding
+	 * field value, the field will be considered as empty and will be reset appropriately.
 	 * @param mpvs the property values to be bound (can be modified)
-	 * @see #getFieldMarkerPrefix
+	 * @see #getFieldMarkerPrefix()
+	 * @see #isAllowed(String)
 	 * @see #getEmptyValue(String, Class)
 	 */
 	protected void checkFieldMarkers(MutablePropertyValues mpvs) {
@@ -279,7 +281,7 @@ public class WebDataBinder extends DataBinder {
 			for (PropertyValue pv : pvArray) {
 				if (pv.getName().startsWith(fieldMarkerPrefix)) {
 					String field = pv.getName().substring(fieldMarkerPrefix.length());
-					if (getPropertyAccessor().isWritableProperty(field) && !mpvs.contains(field)) {
+					if (isAllowed(field) && getPropertyAccessor().isWritableProperty(field) && !mpvs.contains(field)) {
 						Class<?> fieldType = getPropertyAccessor().getPropertyType(field);
 						mpvs.add(field, getEmptyValue(field, fieldType));
 					}
@@ -290,19 +292,22 @@ public class WebDataBinder extends DataBinder {
 	}
 
 	/**
-	 * Check for property values with names that end on {@code "[]"}. This is
-	 * used by some clients for array syntax without an explicit index value.
-	 * If such values are found, drop the brackets to adapt to the expected way
-	 * of expressing the same for data binding purposes.
+	 * Check the given property values for fields that end with empty brackets
+	 * ({@code []}).
+	 * <p>This is used by some clients for array syntax without an explicit index
+	 * value.
+	 * <p>If such a field is allowed, the brackets will be removed in order to adapt
+	 * to the expected format for expressing the same for data binding purposes.
 	 * @param mpvs the property values to be bound (can be modified)
 	 * @since 5.3
+	 * @see #isAllowed(String)
 	 */
 	protected void adaptEmptyArrayIndices(MutablePropertyValues mpvs) {
 		for (PropertyValue pv : mpvs.getPropertyValues()) {
 			String name = pv.getName();
 			if (name.endsWith("[]")) {
 				String field = name.substring(0, name.length() - 2);
-				if (getPropertyAccessor().isWritableProperty(field) && !mpvs.contains(field)) {
+				if (isAllowed(field) && getPropertyAccessor().isWritableProperty(field) && !mpvs.contains(field)) {
 					mpvs.add(field, pv.getValue());
 				}
 				mpvs.removePropertyValue(pv);

spring-web/src/test/java/org/springframework/web/bind/support/WebRequestDataBinderTests.java

@@ -23,7 +23,12 @@ import java.util.List;
 import java.util.Map;
 import java.util.Set;
 
+import org.junit.jupiter.api.Nested;
 import org.junit.jupiter.api.Test;
+import org.junit.jupiter.params.Parameter;
+import org.junit.jupiter.params.ParameterizedClass;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.ValueSource;
 
 import org.springframework.beans.PropertyValue;
 import org.springframework.beans.PropertyValues;
@@ -37,9 +42,15 @@ import org.springframework.web.testfixture.servlet.MockMultipartFile;
 import org.springframework.web.testfixture.servlet.MockMultipartHttpServletRequest;
 
 import static org.assertj.core.api.Assertions.assertThat;
+import static org.springframework.web.bind.WebDataBinder.DEFAULT_FIELD_DEFAULT_PREFIX;
+import static org.springframework.web.bind.WebDataBinder.DEFAULT_FIELD_MARKER_PREFIX;
 
 /**
+ * Tests for {@link WebRequestDataBinder}.
+ *
  * @author Juergen Hoeller
+ * @author Brian Clozel
+ * @author Sam Brannen
  */
 class WebRequestDataBinderTests {
 
@@ -79,10 +90,13 @@ class WebRequestDataBinderTests {
 		assertThat(tb.getSpouse().getName()).isEqualTo("test");
 	}
 
-	@Test
-	void fieldPrefixCausesFieldReset() {
+	@ParameterizedTest
+	@ValueSource(booleans = { true, false })
+	void markerPrefixCausesFieldReset(boolean ignoreUnknownFields) {
 		TestBean target = new TestBean();
+
 		WebRequestDataBinder binder = new WebRequestDataBinder(target);
+		binder.setIgnoreUnknownFields(ignoreUnknownFields);
 
 		MockHttpServletRequest request = new MockHttpServletRequest();
 		request.addParameter("_postProcessed", "visible");
@@ -96,23 +110,30 @@ class WebRequestDataBinderTests {
 	}
 
 	@Test
-	void fieldPrefixCausesFieldResetWithIgnoreUnknownFields() {
+	void fieldWithArrayIndices() {
 		TestBean target = new TestBean();
 		WebRequestDataBinder binder = new WebRequestDataBinder(target);
-		binder.setIgnoreUnknownFields(false);
 
 		MockHttpServletRequest request = new MockHttpServletRequest();
-		request.addParameter("_postProcessed", "visible");
-		request.addParameter("postProcessed", "on");
+		request.addParameter("stringArray[0]", "ONE");
+		request.addParameter("stringArray[1]", "TWO");
 		binder.bind(new ServletWebRequest(request));
-		assertThat(target.isPostProcessed()).isTrue();
-
-		request.removeParameter("postProcessed");
-		binder.bind(new ServletWebRequest(request));
-		assertThat(target.isPostProcessed()).isFalse();
+		assertThat(target.getStringArray()).containsExactly("ONE", "TWO");
 	}
 
-	@Test // gh-25836
+	@Test
+	void fieldWithMissingArrayIndex() {
+		TestBean target = new TestBean();
+		WebRequestDataBinder binder = new WebRequestDataBinder(target);
+
+		MockHttpServletRequest request = new MockHttpServletRequest();
+		request.addParameter("stringArray", "ONE");
+		request.addParameter("stringArray", "TWO");
+		binder.bind(new ServletWebRequest(request));
+		assertThat(target.getStringArray()).containsExactly("ONE", "TWO");
+	}
+
+	@Test  // gh-25836
 	void fieldWithEmptyArrayIndex() {
 		TestBean target = new TestBean();
 		WebRequestDataBinder binder = new WebRequestDataBinder(target);
@@ -140,8 +161,7 @@ class WebRequestDataBinderTests {
 		assertThat(target.isPostProcessed()).isFalse();
 	}
 
-	// SPR-13502
-	@Test
+	@Test  // SPR-13502
 	void collectionFieldsDefault() {
 		TestBean target = new TestBean();
 		target.setSomeSet(null);
@@ -332,7 +352,7 @@ class WebRequestDataBinderTests {
 		for (PropertyValue pv : pvArray) {
 			Object val = m.get(pv.getName());
 			assertThat(val).as("Can't have unexpected value").isNotNull();
-			assertThat(val).as("Val i string").isInstanceOf(String.class);
+			assertThat(val).as("Val is string").isInstanceOf(String.class);
 			assertThat(val).as("val matches expected").isEqualTo(pv.getValue());
 			m.remove(pv.getName());
 		}
@@ -359,21 +379,80 @@ class WebRequestDataBinderTests {
 		assertThat(Arrays.asList(original)).as("Correct values").isEqualTo(Arrays.asList(values));
 	}
 
-	@Test
-	void defaultArgumentShouldNotTriggerAutoGrowWhenDisallowed() {
-		TestBean tb = new TestBean();
-		tb.setSomeMap(null);
 
-		WebRequestDataBinder binder = new WebRequestDataBinder(tb, "person");
-		binder.setAllowedFields("name");
+	@ParameterizedClass  // gh-36625
+	@ValueSource(strings = { DEFAULT_FIELD_DEFAULT_PREFIX, DEFAULT_FIELD_MARKER_PREFIX })
+	@Nested
+	class DefaultAndMarkerPrefixTests {
 
-		MockHttpServletRequest request = new MockHttpServletRequest();
-		request.addParameter("name", "spring");
-		request.addParameter("!someMap[key1]", "test");
-		binder.bind(new ServletWebRequest(request));
+		@Parameter
+		String prefix;
 
-		assertThat(tb.getName()).isEqualTo("spring");
-		assertThat(tb.getSomeMap()).isNull();
+		@Test
+		void shouldNotTriggerBindingWhenFieldIsNotAllowed() {
+			TestBean tb = new TestBean();
+
+			WebRequestDataBinder binder = new WebRequestDataBinder(tb, "person");
+			binder.setAllowedFields("name");
+
+			MockHttpServletRequest request = new MockHttpServletRequest();
+			request.addParameter("name", "spring");
+			request.addParameter(prefix + "country", "test");
+			binder.bind(new ServletWebRequest(request));
+
+			assertThat(tb.getName()).isEqualTo("spring");
+			assertThat(tb.getCountry()).isNull();
+		}
+
+		@Test
+		void shouldNotTriggerBindingWhenFieldIsDisallowed() {
+			TestBean tb = new TestBean();
+
+			WebRequestDataBinder binder = new WebRequestDataBinder(tb, "person");
+			binder.setDisallowedFields("country");
+
+			MockHttpServletRequest request = new MockHttpServletRequest();
+			request.addParameter("name", "spring");
+			request.addParameter(prefix + "country", "test");
+			binder.bind(new ServletWebRequest(request));
+
+			assertThat(tb.getName()).isEqualTo("spring");
+			assertThat(tb.getCountry()).isNull();
+		}
+
+		@Test
+		void shouldNotTriggerAutoGrowWhenFieldIsNotAllowed() {
+			TestBean tb = new TestBean();
+			tb.setSomeMap(null);
+
+			WebRequestDataBinder binder = new WebRequestDataBinder(tb, "person");
+			binder.setAllowedFields("name");
+
+			MockHttpServletRequest request = new MockHttpServletRequest();
+			request.addParameter("name", "spring");
+			request.addParameter(prefix + "someMap[key1]", "test");
+			binder.bind(new ServletWebRequest(request));
+
+			assertThat(tb.getName()).isEqualTo("spring");
+			assertThat(tb.getSomeMap()).isNull();
+		}
+
+		@Test
+		void shouldNotTriggerAutoGrowWhenFieldIsDisallowed() {
+			TestBean tb = new TestBean();
+			tb.setSomeMap(null);
+
+			WebRequestDataBinder binder = new WebRequestDataBinder(tb, "person");
+			binder.setDisallowedFields("someMap[key1]");
+
+			MockHttpServletRequest request = new MockHttpServletRequest();
+			request.addParameter("name", "spring");
+			request.addParameter(prefix + "someMap[key1]", "test");
+			binder.bind(new ServletWebRequest(request));
+
+			assertThat(tb.getName()).isEqualTo("spring");
+			assertThat(tb.getSomeMap()).isNull();
+		}
 	}
 
 
@@ -404,5 +483,4 @@ class WebRequestDataBinderTests {
 		}
 	}
 
-
 }