(grails-core) branch feat/secure-bind-data updated: Add failing regression tests for secureBindData review findings

[bo...@apache.org]

This is an automated email from the ASF dual-hosted git repository.

borinquenkid pushed a commit to branch feat/secure-bind-data
in repository https://gitbox.apache.org/repos/asf/grails-core.git


The following commit(s) were added to refs/heads/feat/secure-bind-data by this push:
     new 755daa848f Add failing regression tests for secureBindData review findings
755daa848f is described below

commit 755daa848f0a324a0cf9034708badaddebd54b4a
Author: Walter Duque de Estrada <wb...@mac.com>
AuthorDate: Wed Jul 1 19:26:17 2026 -0500

    Add failing regression tests for secureBindData review findings
    
    Proves four issues found during review of the secure data binding
    feature with red tests before fixing them:
    - the compile-time allowedParams check accepts a list built from
      request data instead of requiring a literal
    - an indexed collection path beyond autoGrowCollectionLimit crashes
      DataBindingUtils.getIndexedValue during nullMissing processing
    - a non-numeric indexed bracket against a List property crashes the
      pre-existing SimpleDataBinder indexed-property parsing, reachable
      via both plain bindData and secureBindData nullMissing
    
    Co-Authored-By: Claude Sonnet 5 <no...@anthropic.com>
---
 ...erActionTransformerCompilationErrorsSpec.groovy | 21 ++++++++++
 .../grails/web/servlet/BindDataMethodTests.groovy  | 45 ++++++++++++++++++++++
 2 files changed, 66 insertions(+)

diff --git a/grails-controllers/src/test/groovy/org/grails/compiler/web/ControllerActionTransformerCompilationErrorsSpec.groovy b/grails-controllers/src/test/groovy/org/grails/compiler/web/ControllerActionTransformerCompilationErrorsSpec.groovy
index 397ff29386..235964fa95 100644
--- a/grails-controllers/src/test/groovy/org/grails/compiler/web/ControllerActionTransformerCompilationErrorsSpec.groovy
+++ b/grails-controllers/src/test/groovy/org/grails/compiler/web/ControllerActionTransformerCompilationErrorsSpec.groovy
@@ -487,4 +487,25 @@ class ControllerActionTransformerCompilationErrorsSpec extends Specification {
             MultipleCompilationErrorsException e = thrown()
             e.message.contains 'secureBindData requires an explicit allowedParams list. Use secureBindData(target, params, allowedParams).'
     }
+
+    void 'GAP: secureBindData does not reject an allowedParams list built from request data'() {
+        when: 'A controller builds its allowedParams list from request-controlled data rather than a literal'
+            gcl.parseClass('''
+            class TaintedAllowedParamsController {
+                def save() {
+                    def target = new Person()
+                    List allowedParams = params.fields?.tokenize(',')
+                    secureBindData(target, params, allowedParams)
+                }
+            }
+
+            class Person {
+                String name
+            }
+            ''')
+
+        then: 'the compiler should reject an allowedParams list that is not a compile-time literal, since callers can smuggle attacker-controlled field names past the framework validation otherwise'
+            MultipleCompilationErrorsException e = thrown()
+            e.message.contains 'secureBindData requires allowedParams to be a literal list of property names, or a reference to a constant list. A dynamically built list can be attacker-controlled and defeats the purpose of secureBindData.'
+    }
 }
diff --git a/grails-test-suite-web/src/test/groovy/org/grails/web/servlet/BindDataMethodTests.groovy b/grails-test-suite-web/src/test/groovy/org/grails/web/servlet/BindDataMethodTests.groovy
index c330188ca7..e3cd17f3c9 100644
--- a/grails-test-suite-web/src/test/groovy/org/grails/web/servlet/BindDataMethodTests.groovy
+++ b/grails-test-suite-web/src/test/groovy/org/grails/web/servlet/BindDataMethodTests.groovy
@@ -87,6 +87,19 @@ class BindDataMethodTests extends Specification implements ControllerUnitTest<Bi
         target.email == 'lee@mail.com'
     }
 
+    void 'GAP: bindData does not crash on a non-numeric indexed parameter for a List property'() {
+        when: 'a crafted request uses a non-numeric bracket index against a plain bindData call (no secureBindData involved)'
+        params.'members[abc].name' = 'Hacked'
+        def model = controller.bindWithNonNumericIndex()
+        def target = model.target
+
+        then: 'the malformed index should be ignored rather than blowing up the whole binding result'
+        model.bindingResult == null || !model.bindingResult.hasErrors()
+        target.members.size() == 1
+        target.members[0].name == 'Existing'
+        target.members[0].role == 'Existing'
+    }
+
     void 'Test bindData With Disallowed And GrailsParameterMap'() {
         when:
         params.name = 'Marc Palmer'
@@ -426,6 +439,32 @@ class BindDataMethodTests extends Specification implements ControllerUnitTest<Bi
         model.bindingResult == null || !model.bindingResult.hasErrors()
     }
 
+    void 'GAP: secureBindData nullMissing does not crash on a non-numeric indexed parameter root'() {
+        when: 'a crafted request uses a non-numeric bracket index against a List-typed allowed property'
+        params.'members[abc].role' = 'Lead'
+        def model = controller.secureBindWithIndexedParamsAndNullMissing()
+        def target = model.target
+
+        then: 'the malformed index should be ignored rather than blowing up the whole binding result'
+        model.bindingResult == null || !model.bindingResult.hasErrors()
+        target.members.size() == 1
+        target.members[0].name == 'Existing'
+        target.members[0].role == 'Existing'
+    }
+
+    void 'GAP: secureBindData nullMissing does not crash on an out-of-range indexed parameter root'() {
+        when: 'a crafted request uses an indexed root beyond the auto-grow limit, which the binder silently ignores but nullMissing still tries to null out'
+        params.'members[300].name' = 'Alice'
+        def model = controller.secureBindWithIndexedParamsAndNullMissing()
+        def target = model.target
+
+        then: 'the out-of-range index should be ignored rather than blowing up the whole binding result'
+        model.bindingResult == null || !model.bindingResult.hasErrors()
+        target.members.size() == 1
+        target.members[0].name == 'Existing'
+        target.members[0].role == 'Existing'
+    }
+
     void 'Test secureBindData nullMissing supports JSON collection roots'() {
         given:
         request.method = 'POST'
@@ -489,6 +528,12 @@ class BindingController {
         [target: target]
     }
 
+    def bindWithNonNumericIndex() {
+        def target = new CommandObject(members: [new Member(name: 'Existing', role: 'Existing')])
+        def bindingResult = bindData(target, params)
+        [target: target, bindingResult: bindingResult]
+    }
+
     def bindWithPrefixFilterAndDisallowed() {
         def target = new CommandObject()
         def filter = "lee"