Reject invalid databinding indexes (continues #15804)

grails-databinding-core/src/main/groovy/grails/databinding/SimpleDataBinder.groovy

@@ -333,13 +333,19 @@ class SimpleDataBinder implements DataBinder {
         }
 
         if (propertyType.isArray()) {
-            def index = Integer.parseInt(indexedPropertyReferenceDescriptor.index)
+            Integer index = parseIndexedPropertyIndex(obj, indexedPropertyReferenceDescriptor, val, listener, errors)
+            if (index == null) {
+                return
+            }
             def array = initializeArray(obj, propName, propertyType.componentType, index)
             if (array != null) {
                 addElementToArrayAt(array, index, val)
             }
         } else if (Collection.isAssignableFrom(propertyType)) {
-            def index = Integer.parseInt(indexedPropertyReferenceDescriptor.index)
+            Integer index = parseIndexedPropertyIndex(obj, indexedPropertyReferenceDescriptor, val, listener, errors)
+            if (index == null) {
+                return
+            }
             Collection collectionInstance = initializeCollection(obj, propName, propertyType)
             def indexedInstance = null
             if (!(Set.isAssignableFrom(propertyType))) {
@@ -394,6 +400,37 @@ class SimpleDataBinder implements DataBinder {
         }
     }
 
+    /**
+     * Parses an indexed binding path segment as a non-negative integer.
+     * <p>
+     * Indexed properties follow the JavaBeans model (spec v1.01, section 7.2):
+     * array-typed properties with paired {@code int}-indexed accessors. Grails
+     * extends that model to positional collection binding; negative indexes are
+     * rejected because they are not part of the beans model (the prior
+     * {@code [-1]} behavior was Groovy list semantics leaking through the binder).
+     * </p>
+     *
+     * @return the parsed index, or {@code null} when the segment is rejected as a binding error
+     */
+    protected Integer parseIndexedPropertyIndex(obj, IndexedPropertyReferenceDescriptor indexedPropertyReferenceDescriptor,
+        val, DataBindingListener listener, errors) {
+
+        try {
+            Integer index = Integer.parseInt(indexedPropertyReferenceDescriptor.index)
+            if (index < 0) {
+                // Intentional: report the same generic NumberFormatException as a
+                // malformed index so untrusted request data cannot distinguish that
+                // negative indexes are handled explicitly.
+                throw new NumberFormatException(indexedPropertyReferenceDescriptor.index)
+            }
+            index
+        }
+        catch (NumberFormatException e) {
+            addBindingError(obj, indexedPropertyReferenceDescriptor.toString(), val, e, listener, errors)
+            null
+        }
+    }
+
     @CompileStatic(TypeCheckingMode.SKIP)
     protected initializeArray(obj, String propertyName, Class arrayType, int index) {
         Object[] array = obj[propertyName]

grails-databinding-core/src/test/groovy/grails/databinding/CollectionBindingSpec.groovy

@@ -18,8 +18,8 @@
  */
 package grails.databinding
 
-import grails.databinding.SimpleDataBinder;
-import grails.databinding.SimpleMapDataBindingSource;
+import grails.databinding.errors.BindingError
+import grails.databinding.events.DataBindingListenerAdapter
 import spock.lang.Specification
 
 class CollectionBindingSpec extends Specification {
@@ -111,6 +111,60 @@ class CollectionBindingSpec extends Specification {
         company.departments[2].numberOfEmployees == 99
     }
 
+    void 'Test negative indexed binding to a List is rejected'() {
+        given:
+        def binder = new SimpleDataBinder()
+        def company = new Company()
+        def listener = new CollectionBindingListener()
+
+        when:
+        binder.bind company, new SimpleMapDataBindingSource([
+                'departments[-1]': [name: 'Bad Department'],
+                'departments[0]': [name: 'Department Zero']]), listener
+
+        then:
+        company.departments.size() == 1
+        company.departments[0].name == 'Department Zero'
+        listener.bindingErrors.size() == 1
+        listener.bindingErrors[0].propertyName == 'departments[-1]'
+        listener.bindingErrors[0].rejectedValue == [name: 'Bad Department']
+    }
+
+    void 'Test malformed indexed binding to a List is rejected'() {
+        given:
+        def binder = new SimpleDataBinder()
+        def company = new Company()
+        def listener = new CollectionBindingListener()
+
+        when:
+        binder.bind company, new SimpleMapDataBindingSource([
+                'departments[bad]': [name: 'Bad Department'],
+                'departments[0]': [name: 'Department Zero']]), listener
+
+        then:
+        company.departments.size() == 1
+        company.departments[0].name == 'Department Zero'
+        listener.bindingErrors.size() == 1
+        listener.bindingErrors[0].propertyName == 'departments[bad]'
+        listener.bindingErrors[0].rejectedValue == [name: 'Bad Department']
+    }
+
+    void 'Test negative indexed binding to an array is rejected'() {
+        given:
+        def binder = new SimpleDataBinder()
+        def library = new Library()
+        def listener = new CollectionBindingListener()
+
+        when:
+        binder.bind library, new SimpleMapDataBindingSource(['codes[-1]': 'bad', 'codes[0]': 'good']), listener
+
+        then:
+        library.codes as List == ['good']
+        listener.bindingErrors.size() == 1
+        listener.bindingErrors[0].propertyName == 'codes[-1]'
+        listener.bindingErrors[0].rejectedValue == 'bad'
+    }
+
     void 'Test binding to an untyped List'() {
         given:
         def binder = new SimpleDataBinder()
@@ -156,9 +210,22 @@ class Company {
     List<Department> departments
 }
 
+class Library {
+    String[] codes
+}
+
 class Department {
     String name
     Integer numberOfEmployees
     List listOfCodes
     Set setOfCodes
 }
+
+class CollectionBindingListener extends DataBindingListenerAdapter {
+
+    List<BindingError> bindingErrors = []
+
+    void bindingError(BindingError error, errors) {
+        bindingErrors << error
+    }
+}

grails-doc/src/en/guide/theWebLayer/controllers/dataBinding.adoc

@@ -128,13 +128,15 @@ assert band.albums[1].numberOfTracks == 7
 
 That code would work in the same way if `albums` were an array instead of a `List`.
 
+NOTE: Array and positional collection binding follow the JavaBeans indexed-property model (spec v1.01, section 7.2), which only defines non-negative `int` indexes with array semantics. Entries such as `albums[-1]` or `albums[bogus]` are rejected as binding errors. The error field name includes the offending indexed segment, that binding path is skipped, and the target array, collection, or association is not changed by that entry. The JavaBeans specification does not cover `Set` binding; `Set` association keys remain arbitrary grouping keys (see below). Map keys are not interpreted as numeric indexes, so keys such as `players[guitar]` remain valid map keys.
+
 Note that when binding to a `Set` the structure of the `Map` being bound to the `Set` is the same as that of a `Map` being bound to a `List` but since a `Set` is unordered, the indexes don't necessarily correspond to the order of elements in the `Set`.  In the code example above, if `albums` were a `Set` instead of a `List`, the `bindingMap` could look exactly the same but 'Foxtrot' might be the first album in the `Set` or it might be the second.  When updating existing elements in a `Set` the `Map` being assigned to the `Set` must have `id` elements in it which represent the element in the `Set` being updated, as in the following example:
 
 [source,groovy]
 ----
 /*
- * The value of the indexes 0 and 1 in albums[0] and albums[1] are arbitrary
- * values that can be anything as long as they are unique within the Map.
+ * The value of the indexes used in albums[0] and albums[1] are
+ * arbitrary values that only need to be unique within the Map.
  * They do not correspond to the order of elements in albums because albums
  * is a Set.
  */
@@ -516,7 +518,7 @@ class AccountingController {
 ==== Data binding and type conversion errors
 
 
-Sometimes when performing data binding it is not possible to convert a particular String into a particular target type. This results in a type conversion error. Grails will retain type conversion errors inside the link:{domainClassesRef}errors.html[errors] property of a Grails domain class. For example:
+Sometimes when performing data binding it is not possible to convert a particular String into a particular target type. This results in a type conversion error. Grails will retain type conversion errors inside the link:{domainClassesRef}errors.html[errors] property of a Grails domain class. Invalid indexed array, List, and positional many-ended association binding paths, such as negative or non-integer indexes, are also retained as binding errors, and the field name identifies the indexed path that was rejected. For example:
 
 [source,groovy]
 ----

grails-test-suite-persistence/src/test/groovy/grails/web/databinding/GrailsWebDataBinderSpec.groovy

@@ -800,6 +800,34 @@ class GrailsWebDataBinderSpec extends Specification implements DataTest {
         updatedA3.name == 'Author Tres'
     }
 
+    void 'Test updating Set elements by id with non-numeric grouping keys'() {
+
+        given:
+        def publisher = new Publisher(name: 'Some Publisher')
+
+        when:
+        def a1 = new Author(name: 'Author One').save()
+        def a2 = new Author(name: 'Author Two').save()
+        publisher.addToAuthors(a1)
+        publisher.addToAuthors(a2)
+
+        then:
+        a1.id != null
+        a2.id != null
+
+        when:
+        binder.bind(publisher, new SimpleMapDataBindingSource([
+            'authors[foo]': [id: a2.id, name: 'Author Dos'],
+            'authors[bar]': [id: a1.id, name: 'Author Uno']
+        ]))
+        def updatedA1 = publisher.authors.find { it.id == a1.id }
+        def updatedA2 = publisher.authors.find { it.id == a2.id }
+
+        then:
+        updatedA1.name == 'Author Uno'
+        updatedA2.name == 'Author Dos'
+    }
+
     void 'Test updating Set elements by id'() {
 
         given:
@@ -957,6 +985,66 @@ class GrailsWebDataBinderSpec extends Specification implements DataTest {
         publisher.publications[0].title == 'Definitive Guide To Grails 2'
     }
 
+    void 'Test negative indexed domain association binding to a List is rejected'() {
+
+        given:
+        def bindingErrors = [] as List<BindingError>
+        def listener = new DataBindingListenerAdapter() {
+            @Override void bindingError(BindingError error, Object errors) {
+                bindingErrors << error
+            }
+        }
+        def publication = new Publication(title: 'Definitive Guide To Grails', author: new Author(name: 'Author Name'))
+        def publisher = new Publisher(name: 'Apress').save()
+        publisher.addToPublications(publication)
+        publisher.save(flush: true)
+
+        when:
+        binder.bind(publisher, new SimpleMapDataBindingSource([
+            'publications[-1]': [
+                id: publication.id,
+                title: 'Definitive Guide To Grails 2'
+            ]
+        ]), listener)
+
+        then:
+        publisher.publications.size() == 1
+        publisher.publications[0].title == 'Definitive Guide To Grails'
+        bindingErrors.size() == 1
+        bindingErrors[0].propertyName == 'publications[-1]'
+        bindingErrors[0].rejectedValue == [id: publication.id, title: 'Definitive Guide To Grails 2']
+    }
+
+    void 'Test malformed indexed domain association binding to a List is rejected'() {
+
+        given:
+        def bindingErrors = [] as List<BindingError>
+        def listener = new DataBindingListenerAdapter() {
+            @Override void bindingError(BindingError error, Object errors) {
+                bindingErrors << error
+            }
+        }
+        def publication = new Publication(title: 'Definitive Guide To Grails', author: new Author(name: 'Author Name'))
+        def publisher = new Publisher(name: 'Apress').save()
+        publisher.addToPublications(publication)
+        publisher.save(flush: true)
+
+        when:
+        binder.bind(publisher, new SimpleMapDataBindingSource([
+            'publications[bad]': [
+                id: publication.id,
+                title: 'Definitive Guide To Grails 2'
+            ]
+        ]), listener)
+
+        then:
+        publisher.publications.size() == 1
+        publisher.publications[0].title == 'Definitive Guide To Grails'
+        bindingErrors.size() == 1
+        bindingErrors[0].propertyName == 'publications[bad]'
+        bindingErrors[0].rejectedValue == [id: publication.id, title: 'Definitive Guide To Grails 2']
+    }
+
     void 'Test using @BindUsing to initialize property with a type other than the declared type'() {
 
         given:

grails-web-databinding/src/main/groovy/grails/web/databinding/GrailsWebDataBinder.groovy

@@ -434,6 +434,8 @@ class GrailsWebDataBinder extends SimpleDataBinder {
                 if (referencedType != null && isDomainClass(referencedType)) {
                     needsBinding = false
                     if (Set.isAssignableFrom(metaProperty.type)) {
+                        // Set association keys are grouping keys (not positions); do not
+                        // require a numeric index. Selection is by id.
                         def collection = initializeCollection(obj, propName, metaProperty.type)
                         def instance
                         if (collection != null) {
@@ -448,7 +450,7 @@ class GrailsWebDataBinder extends SimpleDataBinder {
                                 Exception e = new IllegalArgumentException(message)
                                 addBindingError(obj, propName, idValue, e, listener, errors)
                             } else {
-                                addElementToCollectionAt(obj, propName, collection, Integer.parseInt(indexedPropertyReferenceDescriptor.index), instance)
+                                addElementToCollectionAt(obj, propName, collection, 0, instance)
                             }
                         }
                         if (instance != null) {
@@ -459,8 +461,11 @@ class GrailsWebDataBinder extends SimpleDataBinder {
                             }
                         }
                     } else if (Collection.isAssignableFrom(metaProperty.type)) {
+                        Integer idx = parseIndexedPropertyIndex(obj, indexedPropertyReferenceDescriptor, val, listener, errors)
+                        if (idx == null) {
+                            return
+                        }
                         def collection = initializeCollection(obj, propName, metaProperty.type)
-                        def idx = Integer.parseInt(indexedPropertyReferenceDescriptor.index)
                         if ('null' == idValue) {
                             if (idx < collection.size()) {
                                 def element = collection[idx]