Skip to content

Reject invalid databinding indexes #15804

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Closed
jamesfredley wants to merge 3 commits into from
Closed

Reject invalid databinding indexes #15804

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
3 commits
Select commit Hold shift + click to select a range
91073c8
Reject invalid databinding indexes
jamesfredley Jul 1, 2026
e2e5674
Document databinding index validation
jamesfredley Jul 2, 2026
e0d3624
Merge remote-tracking branch 'origin/8.0.x' into fix/databinding-nega…
jamesfredley Jul 16, 2026
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
26 changes: 24 additions & 2 deletions grails-databinding-core/src/main/groovy/grails/databinding/SimpleDataBinder.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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))) {
Expand Down Expand Up @@ -394,6 +400,22 @@ class SimpleDataBinder implements DataBinder {
}
}

protected Integer parseIndexedPropertyIndex(obj, IndexedPropertyReferenceDescriptor indexedPropertyReferenceDescriptor,

@jdaugherty jdaugherty Jul 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Rejecting negative indexes is the correct behavior: the JavaBeans specification (v1.01, section 7.2) defines indexed properties as array-typed properties with paired int-indexed accessors, where an invalid index may throw ArrayIndexOutOfBoundsException. Grails' indexed binding to collections is an extension of that model, and this change aligns the extension with the spec's array semantics - the prior [-1] behavior was Groovy list semantics leaking through, never valid under the beans model. Please reference the spec section in this method's groovydoc and in the PR description so the rationale is on record.

val, DataBindingListener listener, errors) {

try {
Integer index = Integer.parseInt(indexedPropertyReferenceDescriptor.index)
if (index < 0) {
throw new NumberFormatException(indexedPropertyReferenceDescriptor.index)

@jdaugherty jdaugherty Jul 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Since this path binds untrusted request data, the reported binding error must stay generic - indistinguishable from the malformed-index case - so the source cannot tell we handle negative indexes explicitly. The current shape achieves that, but only by accident of throwing NumberFormatException as control flow. Please add a code comment documenting that the uniform error is intentional, so a future cleanup does not "improve" it into a distinct, more descriptive message.

}
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]
Expand Down
71 changes: 69 additions & 2 deletions grails-databinding-core/src/test/groovy/grails/databinding/CollectionBindingSpec.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -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 {
Expand Down Expand Up @@ -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()
Expand Down Expand Up @@ -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
}
}
60 changes: 60 additions & 0 deletions ...t-suite-persistence/src/test/groovy/grails/web/databinding/GrailsWebDataBinderSpec.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -956,6 +956,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:
Expand Down
11 changes: 9 additions & 2 deletions grails-web-databinding/src/main/groovy/grails/web/databinding/GrailsWebDataBinder.groovy
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -434,6 +434,10 @@ class GrailsWebDataBinder extends SimpleDataBinder {
if (referencedType != null && isDomainClass(referencedType)) {
needsBinding = false
if (Set.isAssignableFrom(metaProperty.type)) {
Integer index = parseIndexedPropertyIndex(obj, indexedPropertyReferenceDescriptor, val, listener, errors)

@jdaugherty jdaugherty Jul 16, 2026

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This changes documented behavior that the JavaBeans rationale does not cover in either direction: spec indexed properties are array-typed with int accessors, so Set binding keys have no spec standing at all - the arbitrary-unique-key convention here is purely Grails' own documented contract (the guide says the values "can be anything as long as they are unique within the Map"). The old code only reached Integer.parseInt in the add-queried-instance path, so updating an existing Set element by id (e.g. albums[foo]: [id: 1, ...]) never parsed the index and worked with non-numeric keys; parsing at the top of the branch now rejects those previously valid paths. Requiring a non-negative integer here also falsely implies the key is positional when a Set has no positions. Either defer the parse to the addElementToCollectionAt call as before, or treat Set keys like map keys.

if (index == null) {
return
}
def collection = initializeCollection(obj, propName, metaProperty.type)
def instance
if (collection != null) {
Expand All @@ -448,7 +452,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, index, instance)
}
}
if (instance != null) {
Expand All @@ -459,8 +463,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]
Expand Down
Loading