From a9b5911348e55196a229b516125c69ae32bf9a41 Mon Sep 17 00:00:00 2001 From: James Fredley Date: Tue, 30 Jun 2026 18:05:41 -0400 Subject: [PATCH 01/10] Add secure explicit data binding API Assisted-by: Hephaestus:openai/gpt-5.5 --- .../CollectionBindDataMethodSpec.groovy | 28 ++ .../web/servlet/BindDataMethodTests.groovy | 178 ++++++++++++ .../grails/web/databinding/DataBinder.groovy | 60 +++++ .../web/databinding/DataBindingUtils.java | 255 +++++++++++++++++- 4 files changed, 515 insertions(+), 6 deletions(-) diff --git a/grails-test-suite-web/src/test/groovy/org/grails/web/metaclass/CollectionBindDataMethodSpec.groovy b/grails-test-suite-web/src/test/groovy/org/grails/web/metaclass/CollectionBindDataMethodSpec.groovy index 4be3e866d35..bb31d49adcf 100644 --- a/grails-test-suite-web/src/test/groovy/org/grails/web/metaclass/CollectionBindDataMethodSpec.groovy +++ b/grails-test-suite-web/src/test/groovy/org/grails/web/metaclass/CollectionBindDataMethodSpec.groovy @@ -157,6 +157,26 @@ class CollectionBindDataMethodSpec extends Specification implements ControllerUn people[3].firstName == 'Maynard' people[3].lastName == 'Keenan' } + + void 'Test secureBindData with the request using JSON only binds allowed properties'() { + when: + request.json = ''' + [{"firstName": "Danny", "lastName" : "Carey"}, + {"firstName": "Adam", "lastName" : "Jones"}] +''' + def model = controller.secureCreatePeopleWithRequest() + def people = model.people + + then: + people instanceof List + people.size() == 2 + people[0] instanceof Person + people[0].firstName == 'Danny' + people[0].lastName == null + people[1] instanceof Person + people[1].firstName == 'Adam' + people[1].lastName == null + } } @Artefact('Controller') @@ -182,6 +202,14 @@ class DemoController { [people: listOfPeople] } + + def secureCreatePeopleWithRequest() { + def listOfPeople = [] + + secureBindData Person, listOfPeople, request, ['firstName'] + + [people: listOfPeople] + } } class Person { 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 d1b9fbfee64..9d591b916fb 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 @@ -120,6 +120,122 @@ class BindDataMethodTests extends Specification implements ControllerUnitTest void bindToCollection(final Class targetType, final Collection collectionToPopulate, final CollectionDataBindingSource collectionBindingSource) throws InstantiationException, IllegalAccessException { + bindToCollection(targetType, collectionToPopulate, collectionBindingSource, null); + } + + public static void bindToCollection(final Class targetType, final Collection collectionToPopulate, final CollectionDataBindingSource collectionBindingSource, final List include) throws InstantiationException, IllegalAccessException { final GrailsApplication application = Holders.findApplication(); PersistentEntity entity = null; if (application != null) { @@ -189,15 +196,23 @@ public static void bindToCollection(final Class targetType, final Collect ); } - bindObjectToDomainInstance(entity, newObject, dataBindingSource, getBindingIncludeList(newObject), Collections.emptyList(), null); + DataBindingSource sourceToBind = dataBindingSource; + if (include != null) { + sourceToBind = createSecureDataBindingSource(dataBindingSource, include, null); + } + bindObjectToDomainInstance(entity, newObject, sourceToBind, include == null ? getBindingIncludeList(newObject) : include, Collections.emptyList(), null); collectionToPopulate.add(newObject); } } public static void bindToCollection(final Class targetType, final Collection collectionToPopulate, final ServletRequest request) throws InstantiationException, IllegalAccessException { + bindToCollection(targetType, collectionToPopulate, request, null); + } + + public static void bindToCollection(final Class targetType, final Collection collectionToPopulate, final ServletRequest request, final List include) throws InstantiationException, IllegalAccessException { final GrailsApplication grailsApplication = Holders.findApplication(); final CollectionDataBindingSource collectionDataBindingSource = createCollectionDataBindingSource(grailsApplication, targetType, request); - bindToCollection(targetType, collectionToPopulate, collectionDataBindingSource); + bindToCollection(targetType, collectionToPopulate, collectionDataBindingSource, include); } /** @@ -216,15 +231,239 @@ public static BindingResult bindObjectToInstance(Object object, Object source, L include = getBindingIncludeList(object); } GrailsApplication application = Holders.findApplication(); - PersistentEntity entity = null; + PersistentEntity entity = findPersistentEntity(application, object); + return bindObjectToDomainInstance(entity, object, source, include, exclude, filter); + } + + private static PersistentEntity findPersistentEntity(GrailsApplication application, Object object) { if (application != null) { try { - entity = application.getMappingContext().getPersistentEntity(object.getClass().getName()); + return application.getMappingContext().getPersistentEntity(object.getClass().getName()); } catch (GrailsConfigurationException e) { - //no-op + return null; } } - return bindObjectToDomainInstance(entity, object, source, include, exclude, filter); + return null; + } + + public static DataBindingSource createSecureDataBindingSource(Object object, Object source, List allowedParams, String filter) { + GrailsApplication application = Holders.findApplication(); + DataBindingSource bindingSource = createDataBindingSource(application, object.getClass(), source); + return createSecureDataBindingSource(bindingSource, allowedParams, filter); + } + + public static BindingResult secureBindObjectToInstance(Object object, Object source, List allowedParams, String filter, boolean nullMissing) { + BindingResult bindingResult = null; + GrailsApplication grailsApplication = Holders.findApplication(); + PersistentEntity entity = findPersistentEntity(grailsApplication, object); + + try { + final DataBindingSource bindingSource = createDataBindingSource(grailsApplication, object.getClass(), source); + final DataBindingSource secureBindingSource = createSecureDataBindingSource(bindingSource, allowedParams, filter); + final DataBinder grailsWebDataBinder = getGrailsWebDataBinder(grailsApplication); + grailsWebDataBinder.bind(object, secureBindingSource, null, allowedParams, Collections.emptyList()); + if (nullMissing) { + assignNullToMissingAllowedProperties(object, secureBindingSource, allowedParams); + } + } catch (InvalidRequestBodyException e) { + String messageCode = "invalidRequestBody"; + Class objectType = object.getClass(); + String defaultMessage = "An error occurred parsing the body of the request"; + String[] codes = getMessageCodes(messageCode, objectType); + bindingResult = new BeanPropertyBindingResult(object, objectType.getName()); + bindingResult.addError(new ObjectError(bindingResult.getObjectName(), codes, null, defaultMessage)); + } catch (Exception e) { + bindingResult = new BeanPropertyBindingResult(object, object.getClass().getName()); + bindingResult.addError(new ObjectError(bindingResult.getObjectName(), e.getMessage())); + } + + return processBindingResult(entity, object, bindingResult); + } + + public static DataBindingSource createSecureDataBindingSource(DataBindingSource bindingSource, List allowedParams, String filter) { + Map secureSource = new LinkedHashMap(); + for (Object allowedParam : allowedParams) { + if (allowedParam instanceof CharSequence) { + String propertyName = allowedParam.toString(); + String sourcePropertyName = filter == null ? propertyName : filter + "." + propertyName; + copyAllowedProperty(secureSource, propertyName, bindingSource, sourcePropertyName); + copyAllowedCheckboxMarker(secureSource, propertyName, bindingSource, sourcePropertyName); + } + } + return new SimpleMapDataBindingSource(secureSource); + } + + private static void copyAllowedCheckboxMarker(Map secureSource, String targetPropertyName, Object source, String sourcePropertyName) { + String targetMarkerPropertyName = checkboxMarkerPropertyName(targetPropertyName); + String sourceMarkerPropertyName = checkboxMarkerPropertyName(sourcePropertyName); + copyAllowedProperty(secureSource, targetMarkerPropertyName, source, sourceMarkerPropertyName); + } + + private static String checkboxMarkerPropertyName(String propertyName) { + int separator = propertyName.lastIndexOf('.'); + if (separator == -1) { + return "_" + propertyName; + } + return propertyName.substring(0, separator + 1) + "_" + propertyName.substring(separator + 1); + } + + private static boolean copyAllowedProperty(Map secureSource, String targetPropertyName, Object source, String sourcePropertyName) { + if (containsSourceProperty(source, sourcePropertyName)) { + putNestedValue(secureSource, targetPropertyName, getSourcePropertyValue(source, sourcePropertyName)); + return true; + } + int separator = sourcePropertyName.indexOf('.'); + if (separator == -1) { + return false; + } + String sourceRootPropertyName = sourcePropertyName.substring(0, separator); + if (!containsSourceProperty(source, sourceRootPropertyName)) { + return false; + } + Object nestedSource = getSourcePropertyValue(source, sourceRootPropertyName); + String nestedSourcePropertyName = sourcePropertyName.substring(separator + 1); + if (nestedSource instanceof Collection) { + return copyAllowedCollectionProperty(secureSource, targetPropertyName, (Collection) nestedSource, nestedSourcePropertyName); + } + return copyAllowedProperty(secureSource, targetPropertyName, nestedSource, nestedSourcePropertyName); + } + + private static boolean copyAllowedCollectionProperty(Map secureSource, String targetPropertyName, Collection collection, String sourcePropertyName) { + int separator = targetPropertyName.indexOf('.'); + if (separator == -1) { + return false; + } + String targetRootPropertyName = targetPropertyName.substring(0, separator); + String nestedTargetPropertyName = targetPropertyName.substring(separator + 1); + List filteredCollection = getOrCreateNestedCollection(secureSource, targetRootPropertyName, collection.size()); + int index = 0; + boolean copied = false; + for (Object item : collection) { + Map filteredItem = (Map) filteredCollection.get(index); + if (copyAllowedProperty(filteredItem, nestedTargetPropertyName, item, sourcePropertyName)) { + copied = true; + } + index++; + } + return copied; + } + + private static List getOrCreateNestedCollection(Map secureSource, String propertyName, int size) { + Object existingValue = secureSource.get(propertyName); + List collection; + if (existingValue instanceof List) { + collection = (List) existingValue; + } + else { + collection = new ArrayList(size); + secureSource.put(propertyName, collection); + } + while (collection.size() < size) { + collection.add(new LinkedHashMap()); + } + return collection; + } + + private static void putNestedValue(Map secureSource, String propertyName, Object value) { + int separator = propertyName.indexOf('.'); + if (separator == -1) { + secureSource.put(propertyName, value); + return; + } + String rootPropertyName = propertyName.substring(0, separator); + Map nestedSource = getOrCreateNestedMap(secureSource, rootPropertyName); + putNestedValue(nestedSource, propertyName.substring(separator + 1), value); + } + + private static Map getOrCreateNestedMap(Map secureSource, String propertyName) { + Object existingValue = secureSource.get(propertyName); + if (existingValue instanceof Map) { + return (Map) existingValue; + } + Map nestedSource = new LinkedHashMap(); + secureSource.put(propertyName, nestedSource); + return nestedSource; + } + + private static boolean containsSourceProperty(Object source, String propertyName) { + if (source instanceof DataBindingSource) { + return ((DataBindingSource) source).containsProperty(propertyName); + } + if (source instanceof Map) { + return ((Map) source).containsKey(propertyName); + } + return false; + } + + private static Object getSourcePropertyValue(Object source, String propertyName) { + if (source instanceof DataBindingSource) { + return ((DataBindingSource) source).getPropertyValue(propertyName); + } + return ((Map) source).get(propertyName); + } + + public static void assignNullToMissingAllowedProperties(Object object, Object source, List allowedParams) { + assignNullToMissingAllowedProperties(object, source, allowedParams, null); + } + + public static void assignNullToMissingAllowedProperties(Object object, Object source, List allowedParams, String filter) { + GrailsApplication application = Holders.findApplication(); + DataBindingSource bindingSource = createDataBindingSource(application, object.getClass(), source); + assignNullToMissingAllowedProperties(object, bindingSource, allowedParams, filter); + } + + private static void assignNullToMissingAllowedProperties(Object object, DataBindingSource bindingSource, List allowedParams, String filter) { + for (Object allowedParam : allowedParams) { + if (allowedParam instanceof CharSequence) { + String propertyName = allowedParam.toString(); + if (propertyName.indexOf('*') == -1 && !bindingSourceContainsProperty(bindingSource, propertyName, filter)) { + setPropertyToNull(object, propertyName); + } + } + } + } + + private static boolean bindingSourceContainsProperty(DataBindingSource bindingSource, String propertyName, String filter) { + String sourcePropertyName = filter == null ? propertyName : filter + "." + propertyName; + return containsPropertyPath(bindingSource, sourcePropertyName) || containsPropertyPath(bindingSource, checkboxMarkerPropertyName(sourcePropertyName)); + } + + private static boolean containsPropertyPath(Object source, String propertyName) { + if (containsSourceProperty(source, propertyName)) { + return true; + } + int separator = propertyName.indexOf('.'); + if (separator == -1) { + return false; + } + String rootPropertyName = propertyName.substring(0, separator); + if (!containsSourceProperty(source, rootPropertyName)) { + return false; + } + Object nestedSource = getSourcePropertyValue(source, rootPropertyName); + String nestedPropertyName = propertyName.substring(separator + 1); + if (nestedSource instanceof Collection) { + for (Object item : (Collection) nestedSource) { + if (containsPropertyPath(item, nestedPropertyName)) { + return true; + } + } + return false; + } + return containsPropertyPath(nestedSource, nestedPropertyName); + } + + private static void setPropertyToNull(Object object, String propertyName) { + String[] propertyNames = propertyName.split("\\."); + Object currentObject = object; + for (int i = 0; i < propertyNames.length - 1 && currentObject != null; i++) { + MetaClass mc = GroovySystem.getMetaClassRegistry().getMetaClass(currentObject.getClass()); + currentObject = mc.getProperty(currentObject, propertyNames[i]); + } + if (currentObject != null) { + MetaClass mc = GroovySystem.getMetaClassRegistry().getMetaClass(currentObject.getClass()); + mc.setProperty(currentObject, propertyNames[propertyNames.length - 1], null); + } } /** @@ -263,6 +502,10 @@ public static BindingResult bindObjectToDomainInstance(PersistentEntity entity, bindingResult.addError(new ObjectError(bindingResult.getObjectName(), e.getMessage())); } + return processBindingResult(entity, object, bindingResult); + } + + private static BindingResult processBindingResult(PersistentEntity entity, Object object, BindingResult bindingResult) { if (entity != null && bindingResult != null) { BindingResult newResult = new ValidationErrors(object); for (Object error : bindingResult.getAllErrors()) { From 662acc5be1aac26511924a461987136c0c9455d4 Mon Sep 17 00:00:00 2001 From: James Fredley Date: Tue, 30 Jun 2026 23:42:14 -0400 Subject: [PATCH 02/10] Add secure binding controller validation Require controller secureBindData calls to provide an explicit allowlist at compile time, including trait helper and inherited action paths. Document secureBindData in the controller reference and data binding guide. Assisted-by: Hephaestus:gpt-5.5 --- .../web/ControllerActionTransformer.java | 251 +++++++++++ ...ionTransformerCompilationErrorsSpec.groovy | 422 ++++++++++++++++++ grails-doc/src/en/guide/index.adoc | 6 +- grails-doc/src/en/guide/reference.adoc | 5 + .../theWebLayer/controllers/dataBinding.adoc | 42 ++ .../src/en/ref/Controllers/bindData.adoc | 2 + .../en/ref/Controllers/secureBindData.adoc | 68 +++ 7 files changed, 795 insertions(+), 1 deletion(-) create mode 100644 grails-doc/src/en/ref/Controllers/secureBindData.adoc diff --git a/grails-controllers/src/main/groovy/org/grails/compiler/web/ControllerActionTransformer.java b/grails-controllers/src/main/groovy/org/grails/compiler/web/ControllerActionTransformer.java index 280249d6fed..bfb607867f0 100644 --- a/grails-controllers/src/main/groovy/org/grails/compiler/web/ControllerActionTransformer.java +++ b/grails-controllers/src/main/groovy/org/grails/compiler/web/ControllerActionTransformer.java @@ -25,8 +25,10 @@ import java.util.ArrayList; import java.util.Collection; import java.util.HashMap; +import java.util.HashSet; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.regex.Pattern; import groovy.lang.Closure; @@ -36,6 +38,7 @@ import org.codehaus.groovy.ast.AnnotationNode; import org.codehaus.groovy.ast.ClassHelper; import org.codehaus.groovy.ast.ClassNode; +import org.codehaus.groovy.ast.CodeVisitorSupport; import org.codehaus.groovy.ast.ConstructorNode; import org.codehaus.groovy.ast.FieldNode; import org.codehaus.groovy.ast.MethodNode; @@ -189,6 +192,7 @@ public class ControllerActionTransformer implements GrailsArtefactClassInjector, new ClassNode(Action.class)); private static final String ACTION_MEMBER_TARGET = "commandObjects"; public static final String EXCEPTION_HANDLER_META_DATA_FIELD_NAME = "$exceptionHandlerMetaData"; + private static final String SECURE_BIND_DATA_METHOD_NAME = "secureBindData"; private static final TupleExpression EMPTY_TUPLE = new TupleExpression(); @SuppressWarnings({"unchecked"}) @@ -237,6 +241,7 @@ public void performInjection(SourceUnit source, GeneratorContext context, ClassN public void performInjectionOnAnnotatedClass(SourceUnit source, GeneratorContext context, ClassNode classNode) { final String className = classNode.getName(); if (className.endsWith(ControllerArtefactHandler.TYPE)) { + validateSecureBindDataCalls(source, classNode); processMethods(classNode, source, context); processClosures(classNode, source, context); } @@ -259,6 +264,252 @@ private boolean isExceptionHandlingMethod(MethodNode methodNode) { return isExceptionHandler; } + private void validateSecureBindDataCalls(SourceUnit source, ClassNode classNode) { + Set visitedClasses = new HashSet<>(); + Set visitedMethods = new HashSet<>(); + Set visitedTraitHelpers = new HashSet<>(); + validateSecureBindDataCalls(source, classNode, classNode, visitedClasses, visitedMethods, visitedTraitHelpers); + } + + private void validateSecureBindDataCalls(SourceUnit source, ClassNode controllerClassNode, ClassNode classNode, Set visitedClasses, Set visitedMethods, Set visitedTraitHelpers) { + if (classNode == null || OBJECT_CLASS.equals(classNode) || !visitedClasses.add(classNode)) { + return; + } + SecureBindDataCallVisitor visitor = new SecureBindDataCallVisitor(source, controllerClassNode, classNode); + for (MethodNode method : classNode.getMethods()) { + if (method.getCode() != null && visitedMethods.add(method)) { + method.getCode().visit(visitor); + } + validateTraitHelperSecureBindDataCalls(source, method, controllerClassNode, visitedTraitHelpers); + } + for (PropertyNode property : classNode.getProperties()) { + Expression initialExpression = property.getInitialExpression(); + if (initialExpression instanceof ClosureExpression) { + ((ClosureExpression) initialExpression).getCode().visit(visitor); + } + } + validateSecureBindDataCalls(source, controllerClassNode, classNode.getSuperClass(), visitedClasses, visitedMethods, visitedTraitHelpers); + } + + private void validateTraitHelperSecureBindDataCalls(SourceUnit source, MethodNode method, ClassNode controllerClassNode, Set visitedTraitHelpers) { + List traitBridges = method.getAnnotations(new ClassNode(Traits.TraitBridge.class)); + if (traitBridges.size() == 1) { + Expression traitClass = traitBridges.get(0).getMember("traitClass"); + if (traitClass instanceof ClassExpression) { + ClassNode helperClass = Traits.findHelper(traitClass.getType()); + if (helperClass != null && visitedTraitHelpers.add(helperClass)) { + SecureBindDataCallVisitor visitor = new SecureBindDataCallVisitor(source, controllerClassNode, traitClass.getType(), traitClass.getType()); + for (MethodNode helperMethod : helperClass.getMethods()) { + if (helperMethod.getCode() != null) { + helperMethod.getCode().visit(visitor); + } + } + } + } + } + } + + private static class SecureBindDataCallVisitor extends CodeVisitorSupport { + + private final SourceUnit source; + private final ClassNode controllerClassNode; + private final ClassNode visitedClassNode; + private final ClassNode traitClassNode; + + SecureBindDataCallVisitor(SourceUnit source, ClassNode controllerClassNode, ClassNode visitedClassNode) { + this(source, controllerClassNode, visitedClassNode, null); + } + + SecureBindDataCallVisitor(SourceUnit source, ClassNode controllerClassNode, ClassNode visitedClassNode, ClassNode traitClassNode) { + this.source = source; + this.controllerClassNode = controllerClassNode; + this.visitedClassNode = visitedClassNode; + this.traitClassNode = traitClassNode; + } + + @Override + public void visitMethodCallExpression(MethodCallExpression call) { + super.visitMethodCallExpression(call); + if (isControllerSecureBindDataCall(call) && !hasAllowedParamsArgument(call)) { + GrailsASTUtils.error(source, call, + "secureBindData requires an explicit allowedParams list. Use secureBindData(target, params, allowedParams)."); + } + } + + private boolean isControllerSecureBindDataCall(MethodCallExpression call) { + if (!SECURE_BIND_DATA_METHOD_NAME.equals(call.getMethodAsString())) { + return false; + } + if (matchesDeclaredControllerMethod(call)) { + return false; + } + Expression objectExpression = call.getObjectExpression(); + return call.isImplicitThis() || + (objectExpression instanceof VariableExpression && isControllerReceiver((VariableExpression) objectExpression)); + } + + private boolean matchesDeclaredControllerMethod(MethodCallExpression call) { + MethodNode methodTarget = call.getMethodTarget(); + if (methodTarget != null && !methodTarget.isStatic() && declaresControllerMethod(methodTarget)) { + return true; + } + + List arguments = getArguments(call); + if (arguments == null) { + return false; + } + ClassNode currentClassNode = controllerClassNode; + while (currentClassNode != null && !OBJECT_CLASS.equals(currentClassNode)) { + List declaredMethods = currentClassNode.getDeclaredMethods(SECURE_BIND_DATA_METHOD_NAME); + if (declaredMethods != null) { + for (MethodNode method : declaredMethods) { + if ((currentClassNode.equals(visitedClassNode) || !method.isPrivate()) && acceptsArguments(method, arguments)) { + return true; + } + } + } + currentClassNode = currentClassNode.getSuperClass(); + } + if (traitClassNode != null) { + List declaredTraitMethods = traitClassNode.getDeclaredMethods(SECURE_BIND_DATA_METHOD_NAME); + if (declaredTraitMethods != null) { + for (MethodNode method : declaredTraitMethods) { + if (acceptsArguments(method, arguments)) { + return true; + } + } + } + } + return false; + } + + private boolean declaresControllerMethod(MethodNode methodTarget) { + ClassNode currentClassNode = controllerClassNode; + while (currentClassNode != null && !OBJECT_CLASS.equals(currentClassNode)) { + if (currentClassNode.equals(methodTarget.getDeclaringClass())) { + return true; + } + currentClassNode = currentClassNode.getSuperClass(); + } + return false; + } + + private List getArguments(MethodCallExpression call) { + Expression arguments = call.getArguments(); + if (!(arguments instanceof TupleExpression)) { + return null; + } + return ((TupleExpression) arguments).getExpressions(); + } + + private boolean acceptsArguments(MethodNode method, List arguments) { + if (method.isStatic()) { + return false; + } + Parameter[] parameters = method.getParameters(); + int argumentCount = arguments.size(); + int requiredParameterCount = 0; + for (Parameter parameter : parameters) { + if (!parameter.hasInitialExpression()) { + requiredParameterCount++; + } + } + if (parameters.length > 0 && parameters[parameters.length - 1].getType().isArray()) { + if (argumentCount < requiredParameterCount - 1) { + return false; + } + } + else if (argumentCount < requiredParameterCount || argumentCount > parameters.length) { + return false; + } + + for (int i = 0; i < argumentCount; i++) { + if (!acceptsArgument(parameters, i, arguments.get(i))) { + return false; + } + } + return true; + } + + private boolean acceptsArgument(Parameter[] parameters, int argumentIndex, Expression argument) { + ClassNode parameterType = getParameterType(parameters, argumentIndex); + if (parameterType == null || OBJECT_CLASS.equals(parameterType) || ClassHelper.OBJECT_TYPE.equals(parameterType)) { + return true; + } + ClassNode argumentType = argument.getType(); + if (argumentType == null || OBJECT_CLASS.equals(argumentType) || ClassHelper.OBJECT_TYPE.equals(argumentType)) { + return isParamsArgument(argument) && acceptsMapArgument(parameterType); + } + return argumentType.equals(parameterType) || argumentType.isDerivedFrom(parameterType) || argumentType.implementsInterface(parameterType); + } + + private boolean isParamsArgument(Expression argument) { + return argument instanceof VariableExpression && "params".equals(((VariableExpression) argument).getName()); + } + + private boolean acceptsMapArgument(ClassNode parameterType) { + return parameterType.equals(ClassHelper.MAP_TYPE) || parameterType.isDerivedFrom(ClassHelper.MAP_TYPE) || parameterType.implementsInterface(ClassHelper.MAP_TYPE); + } + + private ClassNode getParameterType(Parameter[] parameters, int argumentIndex) { + int parameterIndex = Math.min(argumentIndex, parameters.length - 1); + ClassNode parameterType = parameters[parameterIndex].getType(); + if (parameters.length > 0 && parameterIndex == parameters.length - 1 && parameterType.isArray()) { + return parameterType.getComponentType(); + } + return parameterType; + } + + private boolean isControllerReceiver(VariableExpression objectExpression) { + String variableName = objectExpression.getName(); + return "this".equals(variableName) || "$self".equals(variableName); + } + + private boolean hasAllowedParamsArgument(MethodCallExpression call) { + Expression arguments = call.getArguments(); + if (!(arguments instanceof TupleExpression)) { + return false; + } + List expressions = ((TupleExpression) arguments).getExpressions(); + if (expressions.isEmpty()) { + return false; + } + + return hasAllowedParamsAt(expressions, 2) || hasAllowedParamsAt(expressions, 3); + } + + private boolean hasAllowedParamsAt(List expressions, int allowedParamsIndex) { + if (allowedParamsIndex >= expressions.size()) { + return false; + } + Expression allowedParams = expressions.get(allowedParamsIndex); + if (allowedParams instanceof ConstantExpression || allowedParams instanceof MapExpression) { + return false; + } + if (allowedParams instanceof VariableExpression) { + return isAllowedParamsVariable((VariableExpression) allowedParams); + } + return true; + } + + private boolean isAllowedParamsVariable(VariableExpression allowedParams) { + String variableName = allowedParams.getName(); + if ("params".equals(variableName) || "request".equals(variableName)) { + return false; + } + + ClassNode variableType = allowedParams.getType(); + if (variableType == null || OBJECT_CLASS.equals(variableType) || ClassHelper.OBJECT_TYPE.equals(variableType)) { + return true; + } + return acceptsAllowedParamsType(variableType, ClassHelper.LIST_TYPE) || acceptsAllowedParamsType(variableType, new ClassNode(Collection.class)); + } + + private boolean acceptsAllowedParamsType(ClassNode variableType, ClassNode allowedParamsType) { + return variableType.equals(allowedParamsType) || variableType.isDerivedFrom(allowedParamsType) || variableType.implementsInterface(allowedParamsType); + } + } + private void processMethods(ClassNode classNode, SourceUnit source, GeneratorContext context) { 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 ccca958f22f..397ff29386a 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 @@ -65,4 +65,426 @@ class ControllerActionTransformerCompilationErrorsSpec extends Specification { MultipleCompilationErrorsException e = thrown() e.message.contains 'Parameter [i] to method [methodAction] has default value [42]. Default parameter values are not allowed in controller action methods.' } + + void 'Test secureBindData requires explicit allowed params'() { + when: 'A controller calls secureBindData without an allowed params list' + gcl.parseClass(''' + class TestController { + def save() { + def target = new Person() + secureBindData(target, params) + } + } + + class Person { + String name + } + ''') + + then: + MultipleCompilationErrorsException e = thrown() + e.message.contains 'secureBindData requires an explicit allowedParams list. Use secureBindData(target, params, allowedParams).' + } + + void 'Test secureBindData requires explicit allowed params in controller trait'() { + when: 'A controller trait calls secureBindData without an allowed params list' + gcl.parseClass(''' + trait SecureBindingActions { + def save() { + def target = new Person() + secureBindData(target, params) + } + } + + class TraitSecureBindingController implements SecureBindingActions { + } + + class Person { + String name + } + ''') + + then: + MultipleCompilationErrorsException e = thrown() + e.message.contains 'secureBindData requires an explicit allowedParams list. Use secureBindData(target, params, allowedParams).' + } + + void 'Test secureBindData requires explicit allowed params in controller trait helper'() { + when: 'A controller trait action delegates to a helper that calls secureBindData without an allowed params list' + gcl.parseClass(''' + trait SecureBindingHelperActions { + def save() { + bindWithoutAllowlist() + } + + private bindWithoutAllowlist() { + def target = new Person() + secureBindData(target, params) + } + } + + class TraitHelperSecureBindingController implements SecureBindingHelperActions { + } + + class Person { + String name + } + ''') + + then: + MultipleCompilationErrorsException e = thrown() + e.message.contains 'secureBindData requires an explicit allowedParams list. Use secureBindData(target, params, allowedParams).' + } + + void 'Test controller trait may declare a secureBindData helper method'() { + when: 'A controller trait declares its own secureBindData helper method' + gcl.parseClass(''' + trait HelperSecureBindingActions { + def save() { + def target = new Person() + secureBindData(target, params) + } + + def secureBindData(Object target, Object source) { + target.name = source.name + } + } + + class HelperTraitSecureBindingController implements HelperSecureBindingActions { + } + + class Person { + String name + } + ''') + + then: + noExceptionThrown() + } + + void 'Test secureBindData requires explicit allowed params in inherited controller action'() { + when: 'A controller inherits an action that calls secureBindData without an allowed params list' + gcl.parseClass(''' + class SecureBindingBase { + def save() { + def target = new Person() + secureBindData(target, params) + } + } + + class InheritedSecureBindingController extends SecureBindingBase { + } + + class Person { + String name + } + ''') + + then: + MultipleCompilationErrorsException e = thrown() + e.message.contains 'secureBindData requires an explicit allowedParams list. Use secureBindData(target, params, allowedParams).' + } + + void 'Test controller may declare a secureBindData helper method'() { + when: 'A controller declares its own secureBindData helper method' + gcl.parseClass(''' + class HelperSecureBindingController { + def save() { + def target = new Person() + secureBindData(target, params) + } + + private secureBindData(Object target, Object source) { + target.name = source.name + } + } + + class Person { + String name + } + ''') + + then: + noExceptionThrown() + } + + void 'Test controller may inherit a secureBindData helper method'() { + when: 'A controller inherits its own secureBindData helper method' + gcl.parseClass(''' + class HelperSecureBindingBase { + protected secureBindData(Object target, Object source) { + target.name = source.name + } + } + + class InheritedHelperSecureBindingController extends HelperSecureBindingBase { + def save() { + def target = new Person() + secureBindData(target, params) + } + } + + class Person { + String name + } + ''') + + then: + noExceptionThrown() + } + + void 'Test inherited action may call private secureBindData helper declared in same class'() { + when: 'A superclass action calls its own private helper method' + gcl.parseClass(''' + class PrivateActionHelperSecureBindingBase { + def save() { + def target = new Person() + secureBindData(target, params) + } + + private secureBindData(Object target, Object source) { + target.name = source.name + } + } + + class PrivateActionHelperSecureBindingController extends PrivateActionHelperSecureBindingBase { + } + + class Person { + String name + } + ''') + + then: + noExceptionThrown() + } + + void 'Test inherited action may call concrete controller secureBindData helper method'() { + when: 'An inherited action calls a helper method declared by the concrete controller' + gcl.parseClass(''' + class ConcreteHelperSecureBindingBase { + def save() { + def target = new Person() + secureBindData(target, params) + } + } + + class ConcreteHelperSecureBindingController extends ConcreteHelperSecureBindingBase { + protected secureBindData(Object target, Object source) { + target.name = source.name + } + } + + class Person { + String name + } + ''') + + then: + noExceptionThrown() + } + + void 'Test controller may declare a secureBindData helper method with params argument'() { + when: 'A helper can accept the dynamic params object as a Map argument' + gcl.parseClass(''' + class SpecificHelperSecureBindingController { + def save() { + def target = new Person() + secureBindData(target, params) + } + + protected secureBindData(Object target, Map source) { + target.name = source.name + } + } + + class Person { + String name + } + ''') + + then: + noExceptionThrown() + } + + void 'Test Object typed target with narrowed secureBindData helper still requires framework validation'() { + when: 'A narrowed helper might not accept the runtime target value' + gcl.parseClass(''' + class DynamicTargetHelperSecureBindingController { + def save() { + Object target = new Person() + secureBindData(target, params) + } + + protected secureBindData(Person target, Map source) { + target.name = source.name + } + } + + class Person { + String name + } + ''') + + then: + MultipleCompilationErrorsException e = thrown() + e.message.contains 'secureBindData requires an explicit allowedParams list. Use secureBindData(target, params, allowedParams).' + } + + void 'Test private inherited secureBindData helper does not suppress framework validation'() { + when: 'A private inherited helper is not visible to the controller action call' + gcl.parseClass(''' + class PrivateHelperSecureBindingBase { + private secureBindData(Object target, Object source) { + target.name = source.name + } + } + + class PrivateInheritedHelperSecureBindingController extends PrivateHelperSecureBindingBase { + def save() { + def target = new Person() + secureBindData(target, params) + } + } + + class Person { + String name + } + ''') + + then: + MultipleCompilationErrorsException e = thrown() + e.message.contains 'secureBindData requires an explicit allowedParams list. Use secureBindData(target, params, allowedParams).' + } + + void 'Test incompatible secureBindData helper signature does not suppress framework validation'() { + when: 'A helper with the same arity cannot accept the controller action call arguments' + gcl.parseClass(''' + class IncompatibleHelperSecureBindingController { + def save() { + def target = new Person() + secureBindData(target, params) + } + + private secureBindData(String target, String source) { + target + source + } + } + + class Person { + String name + } + ''') + + then: + MultipleCompilationErrorsException e = thrown() + e.message.contains 'secureBindData requires an explicit allowedParams list. Use secureBindData(target, params, allowedParams).' + } + + void 'Test secureBindData accepts explicit allowed params forms'() { + when: 'A controller calls secureBindData with explicit allowed params lists' + gcl.parseClass(''' + class ExplicitSecureBindingController { + def bindParams() { + def target = new Person() + secureBindData(target, params, ['name']) + } + + def bindParamsWithOptions() { + def target = new Person() + secureBindData([nullMissing: true], target, params, ['name']) + } + + def bindParamsWithVariableOptions() { + def target = new Person() + def options = [nullMissing: true] + secureBindData(options, target, params, ['name']) + } + + def bindParamsWithPrefix() { + def target = new Person() + secureBindData(target, params, ['name'], 'person') + } + + def bindParamsWithVariableAllowedParams() { + def target = new Person() + List allowedParams = ['name'] + secureBindData(target, params, allowedParams) + } + + def bindParamsWithCollectionAllowedParams() { + def target = new Person() + Collection allowedParams = ['name'] + secureBindData(target, params, allowedParams) + } + + def bindCollection() { + def people = [] + secureBindData(Person, people, request, ['name']) + } + + def bindCollectionWithVariableTargetType() { + def people = [] + def targetType = Person + secureBindData(targetType, people, request, ['name']) + } + + def bindMapTarget() { + secureBindData([:], params, ['name']) + } + + def bindMapTargetWithNullMissingKey() { + secureBindData([nullMissing: true], params, ['name']) + } + } + + class Person { + String name + } + ''') + + then: + noExceptionThrown() + } + + void 'Test secureBindData options map still requires explicit allowed params'() { + when: 'A controller calls secureBindData with options but without an allowed params list' + gcl.parseClass(''' + class OptionsWithoutAllowedParamsController { + def save() { + def target = new Person() + secureBindData([nullMissing: true], target, params) + } + } + + class Person { + String name + } + ''') + + then: + MultipleCompilationErrorsException e = thrown() + e.message.contains 'secureBindData requires an explicit allowedParams list. Use secureBindData(target, params, allowedParams).' + } + + void 'Test secureBindData rejects non-collection allowed params variable'() { + when: 'A controller passes a non-collection variable where allowed params are required' + gcl.parseClass(''' + class NonListAllowedParamsController { + def save() { + def target = new Person() + String filter = 'name' + secureBindData(target, params, filter) + } + } + + class Person { + String name + } + ''') + + then: + MultipleCompilationErrorsException e = thrown() + e.message.contains 'secureBindData requires an explicit allowedParams list. Use secureBindData(target, params, allowedParams).' + } } diff --git a/grails-doc/src/en/guide/index.adoc b/grails-doc/src/en/guide/index.adoc index 2e7fa47e869..47e90e36224 100644 --- a/grails-doc/src/en/guide/index.adoc +++ b/grails-doc/src/en/guide/index.adoc @@ -1557,6 +1557,11 @@ include::ref/Controllers/allowedMethods.adoc[] include::ref/Controllers/bindData.adoc[] +[[ref-controllers-secureBindData]] +==== secureBindData + +include::ref/Controllers/secureBindData.adoc[] + [[ref-controllers-chain]] ==== chain @@ -2542,4 +2547,3 @@ include::ref/Tags/uploadForm.adoc[] ==== while include::ref/Tags/while.adoc[] - diff --git a/grails-doc/src/en/guide/reference.adoc b/grails-doc/src/en/guide/reference.adoc index e69395e216a..00e2b921106 100644 --- a/grails-doc/src/en/guide/reference.adoc +++ b/grails-doc/src/en/guide/reference.adoc @@ -352,6 +352,11 @@ include::ref/Controllers/allowedMethods.adoc[] include::ref/Controllers/bindData.adoc[] +[[ref-controllers-secureBindData]] +==== secureBindData + +include::ref/Controllers/secureBindData.adoc[] + [[ref-controllers-chain]] ==== chain diff --git a/grails-doc/src/en/guide/theWebLayer/controllers/dataBinding.adoc b/grails-doc/src/en/guide/theWebLayer/controllers/dataBinding.adoc index 017661bfb4e..97a556d5c1b 100644 --- a/grails-doc/src/en/guide/theWebLayer/controllers/dataBinding.adoc +++ b/grails-doc/src/en/guide/theWebLayer/controllers/dataBinding.adoc @@ -1087,4 +1087,46 @@ bindData(p, params, [include: ['firstName', 'lastName']]) NOTE: If an empty List is provided as a value for the `include` parameter then all fields will be subject to binding if they are not explicitly excluded. +For new code that binds request parameters, prefer link:{controllersRef}secureBindData.html[secureBindData]. It requires the list of allowed properties: + +[source,groovy] +---- +def p = new Person() +secureBindData(p, params, ['firstName', 'lastName']) +---- + +Use nested property paths when allowing nested binding: + +[source,groovy] +---- +def p = new Person() +secureBindData(p, params, ['firstName', 'homeAddress.street', 'homeAddress.city']) +---- + +Prefix filtering is supported: + +[source,groovy] +---- +def p = new Person() +secureBindData(p, params, ['firstName', 'lastName'], 'author') +---- + +You can also clear allowed properties that are missing from the request: + +[source,groovy] +---- +def p = Person.get(1) +secureBindData(p, params, ['firstName', 'lastName'], nullMissing: true) +---- + +When binding a request body into a collection, `secureBindData` also requires the allowed properties for each created item: + +[source,groovy] +---- +def people = [] +secureBindData(Person, people, request, ['firstName', 'lastName']) +---- + +An empty allowlist binds no properties. This differs from `bindData` with an empty `include` list, which leaves all otherwise-bindable properties eligible for binding. + The link:{constraintsRef}bindable.html[bindable] constraint can be used to globally prevent data binding for certain properties. diff --git a/grails-doc/src/en/ref/Controllers/bindData.adoc b/grails-doc/src/en/ref/Controllers/bindData.adoc index a809e63528d..075bf4e56e2 100644 --- a/grails-doc/src/en/ref/Controllers/bindData.adoc +++ b/grails-doc/src/en/ref/Controllers/bindData.adoc @@ -62,6 +62,8 @@ Arguments: NOTE: Note that if an empty List or no List is provided as a value for the `include` parameter then all statically typed instance properties will be subject to binding if they are not explicitly excluded. See the link:{constraintsRefFromRef}bindable.html[bindable] constraint documentation for more information on how to control what is bindable and what is not. +For new code that binds untrusted request parameters, prefer link:secureBindData.html[secureBindData]. It requires an explicit allowlist, treats an empty allowlist as binding no properties, and fails controller compilation if the allowlist is omitted. + The underlying implementation uses Spring's Data Binding framework. If the target is a domain class, type conversion errors are stored in the `errors` property of the domain class. Refer to the section on link:{guidePath}theWebLayer.html#dataBinding[data binding] in the user guide for more information. diff --git a/grails-doc/src/en/ref/Controllers/secureBindData.adoc b/grails-doc/src/en/ref/Controllers/secureBindData.adoc new file mode 100644 index 00000000000..3c44fda0aea --- /dev/null +++ b/grails-doc/src/en/ref/Controllers/secureBindData.adoc @@ -0,0 +1,68 @@ +//// +Licensed to the Apache Software Foundation (ASF) under one +or more contributor license agreements. See the NOTICE file +distributed with this work for additional information +regarding copyright ownership. The ASF licenses this file +to you under the Apache License, Version 2.0 (the +"License"); you may not use this file except in compliance +with the License. You may obtain a copy of the License at + +https://www.apache.org/licenses/LICENSE-2.0 + +Unless required by applicable law or agreed to in writing, +software distributed under the License is distributed on an +"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY +KIND, either express or implied. See the License for the +specific language governing permissions and limitations +under the License. +//// + +== secureBindData + +=== Purpose + +Binds request parameters to an object or collection using an explicit allowlist of property names. + +=== Examples + +[source,groovy] +---- +// binds only firstName and lastName +secureBindData(target, params, ['firstName', 'lastName']) + +// binds only allowed author.* parameters +secureBindData(target, params, ['firstName', 'lastName'], 'author') + +// clears allowed properties that are missing from the request +secureBindData(target, params, ['firstName', 'lastName'], nullMissing: true) + +// binds request body items into a collection using an allowlist for each item +secureBindData(Person, people, request, ['firstName', 'lastName']) +---- + +=== Description + +Usage: `secureBindData(target, params, allowedParams, prefix*)` + +Usage: `secureBindData(options, target, params, allowedParams, prefix*)` + +Usage: `secureBindData(targetType, collectionToPopulate, request, allowedParams)` + +Arguments: + +* `target` - The target object to bind to +* `params` - A `Map` of source parameters, often the link:params.html[params] object when used in a controller +* `allowedParams` - A required list of property names or nested property paths that may be bound +* `prefix` - (Optional) A string representing a prefix to use to filter parameters. The method will automatically append a '.' when matching the prefix to parameters, so you can use 'author' to filter for parameters such as 'author.name'. +* `options` - (Optional) A map of binding options. The `nullMissing` option clears allowed properties that are missing from the source. +* `targetType` - The type of object to create when binding to a collection +* `collectionToPopulate` - The collection to populate +* `request` - The request body source for collection binding + +The `allowedParams` list is required. In controllers, calling `secureBindData` without an explicit allowlist is a compilation error. + +An empty allowlist binds no properties. This differs from link:bindData.html[bindData] with an empty `include` list, which leaves all otherwise-bindable properties eligible for binding. + +Use nested property paths such as `homeAddress.street` to allow nested binding. Properties not named in `allowedParams` are ignored. + +Refer to the section on link:{guidePath}theWebLayer.html#dataBinding[data binding] in the user guide for more information. From 0b15d0c9748dbab287997442b44a15b92dd2a5ef Mon Sep 17 00:00:00 2001 From: James Fredley Date: Wed, 1 Jul 2026 09:43:34 -0400 Subject: [PATCH 03/10] Support secure binding for indexed properties Assisted-by: opencode:gpt-5.5 --- .../theWebLayer/controllers/dataBinding.adoc | 8 + .../en/ref/Controllers/secureBindData.adoc | 7 + .../AdvancedDataBindingController.groovy | 70 +- .../binding/AdvancedDataBindingSpec.groovy | 89 +++ .../web/servlet/BindDataMethodTests.groovy | 344 ++++++++- .../web/databinding/DataBindingUtils.java | 721 +++++++++++++++++- 6 files changed, 1212 insertions(+), 27 deletions(-) diff --git a/grails-doc/src/en/guide/theWebLayer/controllers/dataBinding.adoc b/grails-doc/src/en/guide/theWebLayer/controllers/dataBinding.adoc index 97a556d5c1b..7beac7eb2be 100644 --- a/grails-doc/src/en/guide/theWebLayer/controllers/dataBinding.adoc +++ b/grails-doc/src/en/guide/theWebLayer/controllers/dataBinding.adoc @@ -1103,6 +1103,14 @@ def p = new Person() secureBindData(p, params, ['firstName', 'homeAddress.street', 'homeAddress.city']) ---- +Use logical collection and map paths in the allowlist. The same allowlist path applies to indexed request parameters, so `members.name` allows `members[0].name` and `contributors.expertise` allows `contributors[lead].expertise`: + +[source,groovy] +---- +def team = new Team() +secureBindData(team, params, ['name', 'members.name', 'members.role']) +---- + Prefix filtering is supported: [source,groovy] diff --git a/grails-doc/src/en/ref/Controllers/secureBindData.adoc b/grails-doc/src/en/ref/Controllers/secureBindData.adoc index 3c44fda0aea..f4d8cec2aef 100644 --- a/grails-doc/src/en/ref/Controllers/secureBindData.adoc +++ b/grails-doc/src/en/ref/Controllers/secureBindData.adoc @@ -30,6 +30,9 @@ Binds request parameters to an object or collection using an explicit allowlist // binds only firstName and lastName secureBindData(target, params, ['firstName', 'lastName']) +// indexed request parameters use logical allowlist paths +secureBindData(team, params, ['name', 'members.name', 'members.role']) + // binds only allowed author.* parameters secureBindData(target, params, ['firstName', 'lastName'], 'author') @@ -44,6 +47,8 @@ secureBindData(Person, people, request, ['firstName', 'lastName']) Usage: `secureBindData(target, params, allowedParams, prefix*)` +Usage: `secureBindData(target, params, allowedParams, Map options)` + Usage: `secureBindData(options, target, params, allowedParams, prefix*)` Usage: `secureBindData(targetType, collectionToPopulate, request, allowedParams)` @@ -65,4 +70,6 @@ An empty allowlist binds no properties. This differs from link:bindData.html[bin Use nested property paths such as `homeAddress.street` to allow nested binding. Properties not named in `allowedParams` are ignored. +Use logical collection and map paths in `allowedParams`. For example, `members.name` allows request parameters such as `members[0].name`, and `contributors.expertise` allows map parameters such as `contributors[lead].expertise`. The same logical paths are used with `nullMissing`; missing allowed nested values are cleared on the matching indexed collection or map element. + Refer to the section on link:{guidePath}theWebLayer.html#dataBinding[data binding] in the user guide for more information. diff --git a/grails-test-examples/app1/grails-app/controllers/functionaltests/binding/AdvancedDataBindingController.groovy b/grails-test-examples/app1/grails-app/controllers/functionaltests/binding/AdvancedDataBindingController.groovy index 78ed103c0a4..97eb43b9275 100644 --- a/grails-test-examples/app1/grails-app/controllers/functionaltests/binding/AdvancedDataBindingController.groovy +++ b/grails-test-examples/app1/grails-app/controllers/functionaltests/binding/AdvancedDataBindingController.groovy @@ -109,7 +109,75 @@ class AdvancedDataBindingController { contributors: contributorsMap ] as JSON) } - + + def secureBindEmployee() { + def employee = new Employee() + secureBindData(employee, params, ['firstName', 'homeAddress.street', 'homeAddress.city']) + render([ + firstName: employee.firstName, + email: employee.email, + homeAddress: employee.homeAddress ? [ + street: employee.homeAddress.street, + city: employee.homeAddress.city, + state: employee.homeAddress.state + ] : null, + workAddress: employee.workAddress ? [ + street: employee.workAddress.street, + city: employee.workAddress.city, + state: employee.workAddress.state + ] : null + ] as JSON) + } + + def secureBindTeamWithMembers() { + def team = new Team() + secureBindData(team, params, ['name', 'members.name', 'members.role']) + render([ + name: team.name, + members: team.members?.findAll { it != null }?.collect { [name: it.name, role: it.role] } ?: [] + ] as JSON) + } + + def secureBindProjectWithContributors() { + def project = new Project() + secureBindData(project, params, ['name', 'contributors.name', 'contributors.expertise']) + def contributorsMap = project.contributors?.collectEntries { k, v -> + [k, [name: v?.name, expertise: v?.expertise]] + } ?: [:] + render([ + name: project.name, + contributors: contributorsMap + ] as JSON) + } + + def secureBindWithPrefix() { + def employee = new Employee() + secureBindData(employee, params, ['firstName', 'lastName'], 'employee') + render([ + firstName: employee.firstName, + lastName: employee.lastName, + email: employee.email + ] as JSON) + } + + def secureBindWithNullMissing() { + def employee = new Employee(firstName: 'Existing', email: 'existing@example.com') + secureBindData(employee, params, ['firstName', 'email'], nullMissing: true) + render([ + firstName: employee.firstName, + email: employee.email + ] as JSON) + } + + def secureBindWithEmptyAllowlist() { + def employee = new Employee(firstName: 'Existing', email: 'existing@example.com') + secureBindData(employee, params, []) + render([ + firstName: employee.firstName, + email: employee.email + ] as JSON) + } + /** * Test binding with @RequestParameter annotation. */ diff --git a/grails-test-examples/app1/src/integration-test/groovy/functionaltests/binding/AdvancedDataBindingSpec.groovy b/grails-test-examples/app1/src/integration-test/groovy/functionaltests/binding/AdvancedDataBindingSpec.groovy index c90ee6c7646..e0f9f9929d6 100644 --- a/grails-test-examples/app1/src/integration-test/groovy/functionaltests/binding/AdvancedDataBindingSpec.groovy +++ b/grails-test-examples/app1/src/integration-test/groovy/functionaltests/binding/AdvancedDataBindingSpec.groovy @@ -233,6 +233,95 @@ class AdvancedDataBindingSpec extends Specification implements HttpClientSupport } } + def "test secureBindData binds nested allowed properties and blocks unlisted properties"() { + when: + def response = http( + '/advancedDataBinding/secureBindEmployee?firstName=Secure&email=blocked%40example.com&homeAddress.street=123+Secure+St&homeAddress.city=Portland&workAddress.street=999+Blocked+Ave', + ) + + then: + response.assertStatus(200) + with(response.json()) { + firstName == 'Secure' + email == null + homeAddress.street == '123 Secure St' + homeAddress.city == 'Portland' + homeAddress.state == null + workAddress == null + } + } + + def "test secureBindData binds allowed List collection properties"() { + when: + def response = http( + '/advancedDataBinding/secureBindTeamWithMembers?name=Security&members%5B0%5D.name=Alice&members%5B0%5D.role=Lead&members%5B1%5D.name=Bob&members%5B1%5D.role=Developer', + ) + + then: + response.assertStatus(200) + with(response.json()) { + name == 'Security' + members.size() == 2 + members[0].name == 'Alice' + members[0].role == 'Lead' + members[1].name == 'Bob' + members[1].role == 'Developer' + } + } + + def "test secureBindData binds allowed Map collection properties"() { + when: + def response = http( + '/advancedDataBinding/secureBindProjectWithContributors?name=SecureCore&contributors%5Blead%5D.name=John&contributors%5Blead%5D.expertise=Architecture&contributors%5Bdev%5D.name=Jane&contributors%5Bdev%5D.expertise=Testing', + ) + + then: + response.assertStatus(200) + with(response.json()) { + name == 'SecureCore' + contributors.lead.name == 'John' + contributors.lead.expertise == 'Architecture' + contributors.dev.name == 'Jane' + contributors.dev.expertise == 'Testing' + } + } + + def "test secureBindData supports prefix filtering"() { + when: + def response = http( + '/advancedDataBinding/secureBindWithPrefix?employee.firstName=Prefix&employee.lastName=Allowed&employee.email=blocked%40example.com&other.firstName=Ignored', + ) + + then: + response.assertJson(200, [ + firstName: 'Prefix', + lastName : 'Allowed', + email : null + ]) + } + + def "test secureBindData nullMissing clears missing allowed properties"() { + when: + def response = http('/advancedDataBinding/secureBindWithNullMissing?firstName=Updated') + + then: + response.assertJson(200, [ + firstName: 'Updated', + email : null + ]) + } + + def "test secureBindData with empty allowlist binds no properties"() { + when: + def response = http('/advancedDataBinding/secureBindWithEmptyAllowlist?firstName=Blocked&email=blocked%40example.com') + + then: + response.assertJson(200, [ + firstName: 'Existing', + email : 'existing@example.com' + ]) + } + // ========== Selective Property Binding Tests ========== def "test selective property binding using subscript operator"() { 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 9d591b916fb..c330188ca74 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 @@ -236,6 +236,211 @@ class BindDataMethodTests extends Specification implements ControllerUnitTest members = [] + List departments = [] + Map contributors = [:] + Map preferences = [:] + Map dates = [:] } class Address { String country String city + Map preferences = [:] +} + +class Member { + String name + String role + String email + List
addresses = [] +} + +class Department { + List members = [] + Map contributors = [:] } diff --git a/grails-web-databinding/src/main/groovy/grails/web/databinding/DataBindingUtils.java b/grails-web-databinding/src/main/groovy/grails/web/databinding/DataBindingUtils.java index 3252bdf77f2..15cce6b9356 100644 --- a/grails-web-databinding/src/main/groovy/grails/web/databinding/DataBindingUtils.java +++ b/grails-web-databinding/src/main/groovy/grails/web/databinding/DataBindingUtils.java @@ -21,17 +21,22 @@ import java.lang.reflect.Field; import java.lang.reflect.InvocationTargetException; import java.lang.reflect.Modifier; +import java.lang.reflect.ParameterizedType; +import java.lang.reflect.Type; import java.util.ArrayList; import java.util.Collection; import java.util.Collections; import java.util.HashMap; import java.util.LinkedHashMap; +import java.util.LinkedHashSet; import java.util.List; import java.util.Map; +import java.util.Set; import java.util.concurrent.ConcurrentHashMap; import groovy.lang.GroovySystem; import groovy.lang.MetaClass; +import groovy.lang.MetaProperty; import jakarta.servlet.ServletRequest; @@ -198,7 +203,7 @@ public static void bindToCollection(final Class targetType, final Collect DataBindingSource sourceToBind = dataBindingSource; if (include != null) { - sourceToBind = createSecureDataBindingSource(dataBindingSource, include, null); + sourceToBind = createSecureDataBindingSource(newObject, dataBindingSource, include, null); } bindObjectToDomainInstance(entity, newObject, sourceToBind, include == null ? getBindingIncludeList(newObject) : include, Collections.emptyList(), null); collectionToPopulate.add(newObject); @@ -249,7 +254,7 @@ private static PersistentEntity findPersistentEntity(GrailsApplication applicati public static DataBindingSource createSecureDataBindingSource(Object object, Object source, List allowedParams, String filter) { GrailsApplication application = Holders.findApplication(); DataBindingSource bindingSource = createDataBindingSource(application, object.getClass(), source); - return createSecureDataBindingSource(bindingSource, allowedParams, filter); + return createSecureDataBindingSource(object, bindingSource, allowedParams, filter); } public static BindingResult secureBindObjectToInstance(Object object, Object source, List allowedParams, String filter, boolean nullMissing) { @@ -259,7 +264,7 @@ public static BindingResult secureBindObjectToInstance(Object object, Object sou try { final DataBindingSource bindingSource = createDataBindingSource(grailsApplication, object.getClass(), source); - final DataBindingSource secureBindingSource = createSecureDataBindingSource(bindingSource, allowedParams, filter); + final DataBindingSource secureBindingSource = createSecureDataBindingSource(object, bindingSource, allowedParams, filter); final DataBinder grailsWebDataBinder = getGrailsWebDataBinder(grailsApplication); grailsWebDataBinder.bind(object, secureBindingSource, null, allowedParams, Collections.emptyList()); if (nullMissing) { @@ -281,12 +286,16 @@ public static BindingResult secureBindObjectToInstance(Object object, Object sou } public static DataBindingSource createSecureDataBindingSource(DataBindingSource bindingSource, List allowedParams, String filter) { + return createSecureDataBindingSource(null, bindingSource, allowedParams, filter); + } + + private static DataBindingSource createSecureDataBindingSource(Object object, DataBindingSource bindingSource, List allowedParams, String filter) { Map secureSource = new LinkedHashMap(); for (Object allowedParam : allowedParams) { if (allowedParam instanceof CharSequence) { String propertyName = allowedParam.toString(); String sourcePropertyName = filter == null ? propertyName : filter + "." + propertyName; - copyAllowedProperty(secureSource, propertyName, bindingSource, sourcePropertyName); + copyAllowedProperty(secureSource, propertyName, propertyName, object, object == null ? null : object.getClass(), bindingSource, sourcePropertyName); copyAllowedCheckboxMarker(secureSource, propertyName, bindingSource, sourcePropertyName); } } @@ -300,7 +309,7 @@ private static void copyAllowedCheckboxMarker(Map secureSource, String targetPro } private static String checkboxMarkerPropertyName(String propertyName) { - int separator = propertyName.lastIndexOf('.'); + int separator = lastPropertyPathSeparator(propertyName); if (separator == -1) { return "_" + propertyName; } @@ -308,28 +317,440 @@ private static String checkboxMarkerPropertyName(String propertyName) { } private static boolean copyAllowedProperty(Map secureSource, String targetPropertyName, Object source, String sourcePropertyName) { + return copyAllowedProperty(secureSource, targetPropertyName, targetPropertyName, null, null, source, sourcePropertyName); + } + + private static boolean copyAllowedProperty(Map secureSource, String targetPropertyName, String targetLookupPropertyName, Object target, Class targetType, Object source, String sourcePropertyName) { if (containsSourceProperty(source, sourcePropertyName)) { - putNestedValue(secureSource, targetPropertyName, getSourcePropertyValue(source, sourcePropertyName)); - return true; + if (shouldCopyExactSourceProperty(target, targetType, targetLookupPropertyName, sourcePropertyName)) { + putNestedValue(secureSource, targetPropertyName, getSourcePropertyValue(source, sourcePropertyName)); + return true; + } } - int separator = sourcePropertyName.indexOf('.'); + int separator = propertyPathSeparator(sourcePropertyName); if (separator == -1) { return false; } + if (copyAllowedIndexedPathProperty(secureSource, targetPropertyName, source, sourcePropertyName)) { + return true; + } + if (copyAllowedIndexedRootProperty(secureSource, targetPropertyName, source, sourcePropertyName)) { + return true; + } String sourceRootPropertyName = sourcePropertyName.substring(0, separator); + String nestedSourcePropertyName = sourcePropertyName.substring(separator + 1); if (!containsSourceProperty(source, sourceRootPropertyName)) { - return false; + return copyAllowedIndexedProperty(secureSource, targetPropertyName, source, sourceRootPropertyName, nestedSourcePropertyName); } Object nestedSource = getSourcePropertyValue(source, sourceRootPropertyName); - String nestedSourcePropertyName = sourcePropertyName.substring(separator + 1); + String nestedTargetLookupPropertyName = targetLookupPropertyName; + Object nestedTarget = target; + Class nestedTargetType = targetType; + Class collectionElementType = null; + Class mapValueType = null; + boolean expandMapEntries = false; + if (splitPropertyPath(sourcePropertyName).length <= splitPropertyPath(targetLookupPropertyName).length) { + int targetSeparator = propertyPathSeparator(targetLookupPropertyName); + if (targetSeparator == -1) { + return false; + } + String targetRootPropertyName = targetLookupPropertyName.substring(0, targetSeparator); + nestedTargetLookupPropertyName = targetLookupPropertyName.substring(targetSeparator + 1); + nestedTargetType = getTargetPropertyType(target, targetType, targetRootPropertyName); + collectionElementType = getCollectionElementType(target, targetType, targetRootPropertyName); + mapValueType = getMapValueType(target, targetType, targetRootPropertyName); + expandMapEntries = shouldExpandMapEntries(target, targetType, targetRootPropertyName); + nestedTarget = getTargetPropertyValue(target, targetRootPropertyName); + } if (nestedSource instanceof Collection) { - return copyAllowedCollectionProperty(secureSource, targetPropertyName, (Collection) nestedSource, nestedSourcePropertyName); + return copyAllowedCollectionProperty(secureSource, targetPropertyName, (Collection) nestedSource, nestedSourcePropertyName, nestedTargetLookupPropertyName, nestedTarget, collectionElementType); + } + if (nestedSource instanceof Map) { + if (expandMapEntries) { + return copyAllowedMapProperty(secureSource, targetPropertyName, (Map) nestedSource, nestedSourcePropertyName, mapValueType); + } + if (copyAllowedDirectScalarMapProperty(secureSource, targetPropertyName, (Map) nestedSource, nestedSourcePropertyName)) { + return true; + } + if (copyAllowedDirectMapProperty(secureSource, targetPropertyName, (Map) nestedSource, nestedSourcePropertyName)) { + return true; + } + } + return copyAllowedProperty(secureSource, targetPropertyName, nestedTargetLookupPropertyName, nestedTarget, nestedTargetType, nestedSource, nestedSourcePropertyName); + } + + private static boolean shouldCopyExactSourceProperty(Object target, Class targetType, String targetLookupPropertyName, String sourcePropertyName) { + if (propertyPathSeparator(sourcePropertyName) == -1) { + return true; + } + int targetSeparator = propertyPathSeparator(targetLookupPropertyName); + if (targetSeparator == -1) { + return true; + } + String targetRootPropertyName = targetLookupPropertyName.substring(0, targetSeparator); + return !shouldExpandMapEntries(target, targetType, targetRootPropertyName); + } + + private static boolean shouldExpandMapEntries(Object target, Class targetType, String propertyName) { + Object value = getTargetPropertyValue(target, propertyName); + if (value instanceof Map && hasStructuredTargetMapValues((Map) value)) { + return true; + } + + Class mapValueType = getMapValueType(target, targetType, propertyName); + return mapValueType != null && isStructuredMapValueType(mapValueType); + } + + private static boolean hasStructuredTargetMapValues(Map map) { + for (Object value : map.values()) { + if (value != null && isStructuredMapValueType(value.getClass())) { + return true; + } + } + return false; + } + + private static boolean isStructuredMapValueType(Class valueType) { + Package valuePackage = valueType.getPackage(); + return !valueType.isPrimitive() + && (valuePackage == null || !valuePackage.getName().startsWith("java.")) + && !CharSequence.class.isAssignableFrom(valueType) + && !Number.class.isAssignableFrom(valueType) + && !Boolean.class.isAssignableFrom(valueType) + && !Enum.class.isAssignableFrom(valueType) + && !Map.class.isAssignableFrom(valueType) + && !Collection.class.isAssignableFrom(valueType) + && !Object.class.equals(valueType); + } + + private static Class getMapValueType(Object target, Class targetType, String propertyName) { + Class resolvedTargetType = getTargetType(target, targetType); + if (resolvedTargetType == null) { + return null; + } + + MetaClass mc = GroovySystem.getMetaClassRegistry().getMetaClass(resolvedTargetType); + MetaProperty metaProperty = mc.getMetaProperty(propertyName); + if (metaProperty == null || !Map.class.isAssignableFrom(metaProperty.getType())) { + return null; + } + + Field field = findField(resolvedTargetType, propertyName); + if (field == null) { + return null; + } + return getMapValueType(field.getGenericType()); + } + + private static Class getCollectionElementType(Object target, Class targetType, String propertyName) { + Object value = getTargetPropertyValue(target, propertyName); + if (value instanceof Collection && !((Collection) value).isEmpty()) { + Object firstValue = ((Collection) value).iterator().next(); + if (firstValue != null) { + return firstValue.getClass(); + } + } + + Class resolvedTargetType = getTargetType(target, targetType); + if (resolvedTargetType == null) { + return null; + } + Field field = findField(resolvedTargetType, propertyName); + if (field == null) { + return null; + } + return getCollectionElementType(field.getGenericType()); + } + + private static Class getCollectionElementType(Type type) { + if (!(type instanceof ParameterizedType)) { + return null; + } + + Type[] typeArguments = ((ParameterizedType) type).getActualTypeArguments(); + if (typeArguments.length == 0) { + return null; + } + Type valueType = typeArguments[0]; + if (valueType instanceof Class) { + return (Class) valueType; + } + if (valueType instanceof ParameterizedType && ((ParameterizedType) valueType).getRawType() instanceof Class) { + return (Class) ((ParameterizedType) valueType).getRawType(); + } + return null; + } + + private static Class getTargetPropertyType(Object target, Class targetType, String propertyName) { + Object value = getTargetPropertyValue(target, propertyName); + if (value != null) { + return value.getClass(); + } + + Class resolvedTargetType = getTargetType(target, targetType); + if (resolvedTargetType == null) { + return null; + } + MetaClass mc = GroovySystem.getMetaClassRegistry().getMetaClass(resolvedTargetType); + MetaProperty metaProperty = mc.getMetaProperty(propertyName); + if (metaProperty != null) { + return metaProperty.getType(); + } + Field field = findField(resolvedTargetType, propertyName); + return field == null ? null : field.getType(); + } + + private static Class getTargetType(Object target, Class targetType) { + if (target != null) { + return target.getClass(); + } + return targetType; + } + + private static Class getMapValueType(Type type) { + if (!(type instanceof ParameterizedType)) { + return null; + } + + Type[] typeArguments = ((ParameterizedType) type).getActualTypeArguments(); + if (typeArguments.length < 2) { + return null; + } + Type valueType = typeArguments[1]; + if (valueType instanceof Class) { + return (Class) valueType; + } + if (valueType instanceof ParameterizedType && ((ParameterizedType) valueType).getRawType() instanceof Class) { + return (Class) ((ParameterizedType) valueType).getRawType(); + } + return null; + } + + private static Object getTargetPropertyValue(Object target, String propertyName) { + if (target == null) { + return null; + } + + try { + MetaClass mc = GroovySystem.getMetaClassRegistry().getMetaClass(target.getClass()); + return mc.getProperty(target, propertyName); + } + catch (Exception e) { + return null; + } + } + + private static Field findField(Class type, String propertyName) { + Class currentType = type; + while (currentType != null) { + try { + return currentType.getDeclaredField(propertyName); + } + catch (NoSuchFieldException e) { + currentType = currentType.getSuperclass(); + } + } + return null; + } + + private static boolean copyAllowedIndexedProperty(Map secureSource, String targetPropertyName, Object source, String sourceRootPropertyName, String nestedSourcePropertyName) { + int separator = propertyPathSeparator(targetPropertyName); + if (separator == -1) { + return false; + } + + String targetRootPropertyName = targetPropertyName.substring(0, separator); + if (!sourceRootPropertyName.equals(targetRootPropertyName)) { + return false; + } + String nestedTargetPropertyName = targetPropertyName.substring(separator + 1); + boolean copied = false; + String indexedSourcePropertyPrefix = sourceRootPropertyName + "["; + for (String indexedSourcePropertyName : getIndexedSourcePropertyNames(source, indexedSourcePropertyPrefix)) { + String targetIndexedPropertyName = targetRootPropertyName + indexedSourcePropertyName.substring(sourceRootPropertyName.length()); + String targetIndexedNestedPropertyName = targetIndexedPropertyName + "." + nestedTargetPropertyName; + if (containsSourceProperty(source, indexedSourcePropertyName)) { + Object nestedSource = getSourcePropertyValue(source, indexedSourcePropertyName); + if (copyAllowedProperty(secureSource, targetIndexedNestedPropertyName, nestedSource, nestedSourcePropertyName)) { + copied = true; + } + } + else if (copyAllowedProperty(secureSource, targetIndexedNestedPropertyName, source, indexedSourcePropertyName + "." + nestedSourcePropertyName)) { + copied = true; + } + } + return copied; + } + + private static boolean copyAllowedIndexedPathProperty(Map secureSource, String targetPropertyName, Object source, String sourcePropertyName) { + boolean copied = false; + String[] targetSegments = splitPropertyPath(targetPropertyName); + String[] sourceSegments = splitPropertyPath(sourcePropertyName); + int exactPrefixSegments = sourceSegments.length - targetSegments.length; + for (String indexedSourcePropertyName : getSourcePropertyNames(source)) { + if (indexedPropertyPathMatches(indexedSourcePropertyName, sourcePropertyName, exactPrefixSegments)) { + putNestedValue(secureSource, indexedTargetPropertyName(targetPropertyName, sourcePropertyName, indexedSourcePropertyName), getSourcePropertyValue(source, indexedSourcePropertyName)); + copied = true; + } + } + return copied; + } + + private static boolean copyAllowedIndexedRootProperty(Map secureSource, String targetPropertyName, Object source, String sourcePropertyName) { + int sourceSeparator = lastPropertyPathSeparator(sourcePropertyName); + int targetSeparator = lastPropertyPathSeparator(targetPropertyName); + if (sourceSeparator == -1 || targetSeparator == -1) { + return false; + } + + String sourceRootPropertyName = sourcePropertyName.substring(0, sourceSeparator); + String targetRootPropertyName = targetPropertyName.substring(0, targetSeparator); + String nestedSourcePropertyName = sourcePropertyName.substring(sourceSeparator + 1); + String nestedTargetPropertyName = targetPropertyName.substring(targetSeparator + 1); + String[] targetSegments = splitPropertyPath(targetRootPropertyName); + String[] sourceSegments = splitPropertyPath(sourceRootPropertyName); + int exactPrefixSegments = sourceSegments.length - targetSegments.length; + boolean copied = false; + for (String indexedSourcePropertyName : getSourcePropertyNames(source)) { + if (indexedPropertyPathMatches(indexedSourcePropertyName, sourceRootPropertyName, exactPrefixSegments) && containsSourceProperty(source, indexedSourcePropertyName)) { + String targetIndexedPropertyName = indexedTargetPropertyName(targetRootPropertyName, sourceRootPropertyName, indexedSourcePropertyName); + Object nestedSource = getSourcePropertyValue(source, indexedSourcePropertyName); + if (copyAllowedProperty(secureSource, targetIndexedPropertyName + "." + nestedTargetPropertyName, nestedSource, nestedSourcePropertyName)) { + copied = true; + } + } + } + return copied; + } + + private static int propertyPathSeparator(String propertyName) { + return propertyPathSeparator(propertyName, false); + } + + private static int lastPropertyPathSeparator(String propertyName) { + return propertyPathSeparator(propertyName, true); + } + + private static int propertyPathSeparator(String propertyName, boolean last) { + int separator = -1; + int bracketDepth = 0; + for (int i = 0; i < propertyName.length(); i++) { + char character = propertyName.charAt(i); + if (character == '[') { + bracketDepth++; + } + else if (character == ']' && bracketDepth > 0) { + bracketDepth--; + } + else if (character == '.' && bracketDepth == 0) { + if (!last) { + return i; + } + separator = i; + } } - return copyAllowedProperty(secureSource, targetPropertyName, nestedSource, nestedSourcePropertyName); + return separator; + } + + private static String indexedTargetPropertyName(String targetPropertyName, String sourcePropertyName, String indexedSourcePropertyName) { + String[] targetSegments = splitPropertyPath(targetPropertyName); + String[] sourceSegments = splitPropertyPath(sourcePropertyName); + String[] indexedSourceSegments = splitPropertyPath(indexedSourcePropertyName); + int sourceOffset = Math.max(0, sourceSegments.length - targetSegments.length); + int targetOffset = Math.max(0, targetSegments.length - sourceSegments.length); + StringBuilder indexedTargetPropertyName = new StringBuilder(); + for (int i = 0; i < targetSegments.length; i++) { + if (i > 0) { + indexedTargetPropertyName.append('.'); + } + String targetSegment = targetSegments[i]; + int sourceIndex = i - targetOffset + sourceOffset; + if (sourceIndex < 0 || sourceIndex >= indexedSourceSegments.length || sourceIndex >= sourceSegments.length) { + indexedTargetPropertyName.append(targetSegment); + continue; + } + + String indexedSourceSegment = indexedSourceSegments[sourceIndex]; + String sourceSegment = sourceSegments[sourceIndex]; + if (indexedSegmentMatches(indexedSourceSegment, sourceSegment)) { + indexedTargetPropertyName.append(targetSegment).append(indexedSourceSegment.substring(sourceSegment.length())); + } + else { + indexedTargetPropertyName.append(targetSegment); + } + } + return indexedTargetPropertyName.toString(); + } + + private static boolean indexedPropertyPathMatches(String indexedPropertyName, String propertyName, int exactPrefixSegments) { + String[] indexedPropertySegments = splitPropertyPath(indexedPropertyName); + String[] propertySegments = splitPropertyPath(propertyName); + if (indexedPropertySegments.length != propertySegments.length) { + return false; + } + boolean indexed = false; + for (int i = 0; i < propertySegments.length; i++) { + if (indexedPropertySegments[i].equals(propertySegments[i])) { + continue; + } + if (i < exactPrefixSegments) { + return false; + } + if (!indexedSegmentMatches(indexedPropertySegments[i], propertySegments[i])) { + return false; + } + indexed = true; + } + return indexed; + } + + private static String[] splitPropertyPath(String propertyName) { + List segments = new ArrayList<>(); + StringBuilder segment = new StringBuilder(); + int bracketDepth = 0; + for (int i = 0; i < propertyName.length(); i++) { + char character = propertyName.charAt(i); + if (character == '.' && bracketDepth == 0) { + segments.add(segment.toString()); + segment.setLength(0); + } + else { + if (character == '[') { + bracketDepth++; + } + else if (character == ']' && bracketDepth > 0) { + bracketDepth--; + } + segment.append(character); + } + } + segments.add(segment.toString()); + return segments.toArray(new String[0]); + } + + private static boolean indexedSegmentMatches(String indexedSegment, String segment) { + return indexedSegment.startsWith(segment + "[") && indexedSegment.endsWith("]"); + } + + private static Set getIndexedSourcePropertyNames(Object source, String indexedSourcePropertyPrefix) { + Set indexedSourcePropertyNames = new LinkedHashSet<>(); + for (String propertyName : getSourcePropertyNames(source)) { + if (propertyName.startsWith(indexedSourcePropertyPrefix)) { + int closingIndex = propertyName.indexOf(']', indexedSourcePropertyPrefix.length()); + if (closingIndex > -1) { + indexedSourcePropertyNames.add(propertyName.substring(0, closingIndex + 1)); + } + } + } + return indexedSourcePropertyNames; } private static boolean copyAllowedCollectionProperty(Map secureSource, String targetPropertyName, Collection collection, String sourcePropertyName) { - int separator = targetPropertyName.indexOf('.'); + return copyAllowedCollectionProperty(secureSource, targetPropertyName, collection, sourcePropertyName, sourcePropertyName, null, null); + } + + private static boolean copyAllowedCollectionProperty(Map secureSource, String targetPropertyName, Collection collection, String sourcePropertyName, String targetLookupPropertyName, Object targetCollection, Class targetElementType) { + int separator = propertyPathSeparator(targetPropertyName); if (separator == -1) { return false; } @@ -340,7 +761,9 @@ private static boolean copyAllowedCollectionProperty(Map secureSource, String ta boolean copied = false; for (Object item : collection) { Map filteredItem = (Map) filteredCollection.get(index); - if (copyAllowedProperty(filteredItem, nestedTargetPropertyName, item, sourcePropertyName)) { + Object targetItem = getCollectionValue(targetCollection, index); + Class itemTargetType = targetItem == null ? targetElementType : targetItem.getClass(); + if (copyAllowedProperty(filteredItem, nestedTargetPropertyName, targetLookupPropertyName, targetItem, itemTargetType, item, sourcePropertyName)) { copied = true; } index++; @@ -348,6 +771,77 @@ private static boolean copyAllowedCollectionProperty(Map secureSource, String ta return copied; } + private static Object getCollectionValue(Object targetCollection, int index) { + if (targetCollection instanceof List && ((List) targetCollection).size() > index) { + return ((List) targetCollection).get(index); + } + if (targetCollection instanceof Collection) { + int currentIndex = 0; + for (Object value : (Collection) targetCollection) { + if (currentIndex == index) { + return value; + } + currentIndex++; + } + } + return null; + } + + private static boolean copyAllowedDirectScalarMapProperty(Map secureSource, String targetPropertyName, Map map, String sourcePropertyName) { + if (propertyPathSeparator(sourcePropertyName) > -1 || !containsSourceProperty(map, sourcePropertyName)) { + return false; + } + Object directValue = getSourcePropertyValue(map, sourcePropertyName); + if (isNestedSource(directValue)) { + return false; + } + putNestedValue(secureSource, targetPropertyName, directValue); + return true; + } + + private static boolean copyAllowedDirectMapProperty(Map secureSource, String targetPropertyName, Map map, String sourcePropertyName) { + if (propertyPathSeparator(sourcePropertyName) > -1 || !containsSourceProperty(map, sourcePropertyName)) { + return false; + } + putNestedValue(secureSource, targetPropertyName, getSourcePropertyValue(map, sourcePropertyName)); + return true; + } + + private static boolean copyAllowedMapProperty(Map secureSource, String targetPropertyName, Map map, String sourcePropertyName, Class targetValueType) { + if (!hasNestedSourceEntries(map)) { + return false; + } + + int separator = propertyPathSeparator(targetPropertyName); + if (separator == -1) { + return false; + } + String targetRootPropertyName = targetPropertyName.substring(0, separator); + String nestedTargetPropertyName = targetPropertyName.substring(separator + 1); + boolean copied = false; + for (Object entryObject : map.entrySet()) { + Map.Entry entry = (Map.Entry) entryObject; + String targetIndexedPropertyName = targetRootPropertyName + "[" + entry.getKey() + "]"; + if (copyAllowedProperty(secureSource, targetIndexedPropertyName + "." + nestedTargetPropertyName, nestedTargetPropertyName, null, targetValueType, entry.getValue(), sourcePropertyName)) { + copied = true; + } + } + return copied; + } + + private static boolean isNestedSource(Object value) { + return value instanceof Map || value instanceof Collection || value instanceof DataBindingSource; + } + + private static boolean hasNestedSourceEntries(Map map) { + for (Object value : map.values()) { + if (isNestedSource(value)) { + return true; + } + } + return false; + } + private static List getOrCreateNestedCollection(Map secureSource, String propertyName, int size) { Object existingValue = secureSource.get(propertyName); List collection; @@ -365,7 +859,7 @@ private static List getOrCreateNestedCollection(Map secureSource, String propert } private static void putNestedValue(Map secureSource, String propertyName, Object value) { - int separator = propertyName.indexOf('.'); + int separator = propertyPathSeparator(propertyName); if (separator == -1) { secureSource.put(propertyName, value); return; @@ -395,6 +889,19 @@ private static boolean containsSourceProperty(Object source, String propertyName return false; } + private static Set getSourcePropertyNames(Object source) { + Set propertyNames = new LinkedHashSet<>(); + if (source instanceof DataBindingSource) { + propertyNames.addAll(((DataBindingSource) source).getPropertyNames()); + } + else if (source instanceof Map) { + for (Object key : ((Map) source).keySet()) { + propertyNames.add(key.toString()); + } + } + return propertyNames; + } + private static Object getSourcePropertyValue(Object source, String propertyName) { if (source instanceof DataBindingSource) { return ((DataBindingSource) source).getPropertyValue(propertyName); @@ -416,29 +923,138 @@ private static void assignNullToMissingAllowedProperties(Object object, DataBind for (Object allowedParam : allowedParams) { if (allowedParam instanceof CharSequence) { String propertyName = allowedParam.toString(); - if (propertyName.indexOf('*') == -1 && !bindingSourceContainsProperty(bindingSource, propertyName, filter)) { - setPropertyToNull(object, propertyName); + if (propertyName.indexOf('*') == -1) { + if (assignNullToMissingIndexedProperties(object, bindingSource, propertyName, filter)) { + continue; + } + if (!bindingSourceContainsProperty(bindingSource, propertyName, filter)) { + setPropertyToNull(object, propertyName); + } } } } } + private static boolean assignNullToMissingIndexedProperties(Object object, DataBindingSource bindingSource, String propertyName, String filter) { + String sourcePropertyName = filter == null ? propertyName : filter + "." + propertyName; + return assignNullToMissingIndexedProperties(object, bindingSource, BLANK, sourcePropertyName, propertyName); + } + + private static boolean assignNullToMissingIndexedProperties(Object object, Object source, String targetPathPrefix, String sourcePropertyName, String targetPropertyName) { + int sourceSeparator = propertyPathSeparator(sourcePropertyName); + int targetSeparator = propertyPathSeparator(targetPropertyName); + if (sourceSeparator == -1 || targetSeparator == -1) { + return false; + } + + String sourceRootPropertyName = sourcePropertyName.substring(0, sourceSeparator); + String targetRootPropertyName = targetPropertyName.substring(0, targetSeparator); + String nestedSourcePropertyName = sourcePropertyName.substring(sourceSeparator + 1); + String nestedTargetPropertyName = targetPropertyName.substring(targetSeparator + 1); + if (splitPropertyPath(sourcePropertyName).length > splitPropertyPath(targetPropertyName).length) { + if (containsSourceProperty(source, sourceRootPropertyName)) { + return assignNullToMissingIndexedProperties(object, getSourcePropertyValue(source, sourceRootPropertyName), targetPathPrefix, nestedSourcePropertyName, targetPropertyName); + } + return false; + } + + if (containsSourceProperty(source, sourceRootPropertyName)) { + Object nestedSource = getSourcePropertyValue(source, sourceRootPropertyName); + if (nestedSource instanceof Collection) { + return assignNullToMissingCollectionProperties(object, (Collection) nestedSource, targetPathPrefix, targetRootPropertyName, nestedSourcePropertyName, nestedTargetPropertyName); + } + Object targetObject = getTargetObject(object, targetPathPrefix); + if (nestedSource instanceof Map && hasNestedSourceEntries((Map) nestedSource) && shouldExpandMapEntries(targetObject, targetObject == null ? null : targetObject.getClass(), targetRootPropertyName)) { + return assignNullToMissingMapProperties(object, (Map) nestedSource, targetPathPrefix, targetRootPropertyName, nestedSourcePropertyName, nestedTargetPropertyName); + } + } + + boolean indexed = false; + String indexedSourcePropertyPrefix = sourceRootPropertyName + "["; + for (String indexedSourcePropertyName : getIndexedSourcePropertyNames(source, indexedSourcePropertyPrefix)) { + if (containsSourceProperty(source, indexedSourcePropertyName)) { + indexed = true; + String targetIndexedPropertyName = appendPropertyPath(targetPathPrefix, targetRootPropertyName + indexedSourcePropertyName.substring(sourceRootPropertyName.length())); + Object nestedSource = getSourcePropertyValue(source, indexedSourcePropertyName); + if (!assignNullToMissingIndexedProperties(object, nestedSource, targetIndexedPropertyName, nestedSourcePropertyName, nestedTargetPropertyName) && !containsPropertyPath(nestedSource, nestedSourcePropertyName)) { + setPropertyToNull(object, targetIndexedPropertyName + "." + nestedTargetPropertyName); + } + } + } + return indexed; + } + + private static boolean assignNullToMissingCollectionProperties(Object object, Collection collection, String targetPathPrefix, String targetRootPropertyName, String nestedSourcePropertyName, String nestedTargetPropertyName) { + int index = 0; + for (Object item : collection) { + String targetIndexedPropertyName = appendPropertyPath(targetPathPrefix, targetRootPropertyName + "[" + index + "]"); + assignNullToMissingNestedProperty(object, item, targetIndexedPropertyName, nestedSourcePropertyName, nestedTargetPropertyName); + index++; + } + return true; + } + + private static boolean assignNullToMissingMapProperties(Object object, Map map, String targetPathPrefix, String targetRootPropertyName, String nestedSourcePropertyName, String nestedTargetPropertyName) { + for (Object entryObject : map.entrySet()) { + Map.Entry entry = (Map.Entry) entryObject; + String targetIndexedPropertyName = appendPropertyPath(targetPathPrefix, targetRootPropertyName + "[" + entry.getKey() + "]"); + assignNullToMissingNestedProperty(object, entry.getValue(), targetIndexedPropertyName, nestedSourcePropertyName, nestedTargetPropertyName); + } + return true; + } + + private static Object getTargetObject(Object object, String targetPathPrefix) { + if (targetPathPrefix == null || targetPathPrefix.length() == 0) { + return object; + } + + Object targetObject = object; + for (String propertyName : splitPropertyPath(targetPathPrefix)) { + if (targetObject == null) { + return null; + } + targetObject = getPropertyValue(targetObject, propertyName); + } + return targetObject; + } + + private static void assignNullToMissingNestedProperty(Object object, Object nestedSource, String targetIndexedPropertyName, String nestedSourcePropertyName, String nestedTargetPropertyName) { + if (!assignNullToMissingIndexedProperties(object, nestedSource, targetIndexedPropertyName, nestedSourcePropertyName, nestedTargetPropertyName) && !containsPropertyPath(nestedSource, nestedSourcePropertyName)) { + setPropertyToNull(object, targetIndexedPropertyName + "." + nestedTargetPropertyName); + } + } + + private static String appendPropertyPath(String parentPath, String propertyName) { + if (parentPath == null || parentPath.length() == 0) { + return propertyName; + } + return parentPath + "." + propertyName; + } + private static boolean bindingSourceContainsProperty(DataBindingSource bindingSource, String propertyName, String filter) { String sourcePropertyName = filter == null ? propertyName : filter + "." + propertyName; - return containsPropertyPath(bindingSource, sourcePropertyName) || containsPropertyPath(bindingSource, checkboxMarkerPropertyName(sourcePropertyName)); + int exactPrefixSegments = filter == null ? 0 : splitPropertyPath(filter).length; + return containsPropertyPath(bindingSource, sourcePropertyName, exactPrefixSegments) || containsPropertyPath(bindingSource, checkboxMarkerPropertyName(sourcePropertyName), exactPrefixSegments); } private static boolean containsPropertyPath(Object source, String propertyName) { + return containsPropertyPath(source, propertyName, 0); + } + + private static boolean containsPropertyPath(Object source, String propertyName, int exactPrefixSegments) { if (containsSourceProperty(source, propertyName)) { return true; } - int separator = propertyName.indexOf('.'); + if (containsIndexedPropertyPath(source, propertyName, exactPrefixSegments)) { + return true; + } + int separator = propertyPathSeparator(propertyName); if (separator == -1) { return false; } String rootPropertyName = propertyName.substring(0, separator); if (!containsSourceProperty(source, rootPropertyName)) { - return false; + return containsIndexedNestedPropertyPath(source, rootPropertyName, propertyName.substring(separator + 1)); } Object nestedSource = getSourcePropertyValue(source, rootPropertyName); String nestedPropertyName = propertyName.substring(separator + 1); @@ -453,16 +1069,71 @@ private static boolean containsPropertyPath(Object source, String propertyName) return containsPropertyPath(nestedSource, nestedPropertyName); } + private static boolean containsIndexedNestedPropertyPath(Object source, String rootPropertyName, String nestedPropertyName) { + String indexedSourcePropertyPrefix = rootPropertyName + "["; + for (String indexedSourcePropertyName : getIndexedSourcePropertyNames(source, indexedSourcePropertyPrefix)) { + if (containsSourceProperty(source, indexedSourcePropertyName) && containsPropertyPath(getSourcePropertyValue(source, indexedSourcePropertyName), nestedPropertyName)) { + return true; + } + } + return false; + } + + private static boolean containsIndexedPropertyPath(Object source, String propertyName, int exactPrefixSegments) { + for (String indexedPropertyName : getSourcePropertyNames(source)) { + if (indexedPropertyPathMatches(indexedPropertyName, propertyName, exactPrefixSegments)) { + return true; + } + } + return false; + } + private static void setPropertyToNull(Object object, String propertyName) { - String[] propertyNames = propertyName.split("\\."); + String[] propertyNames = splitPropertyPath(propertyName); Object currentObject = object; for (int i = 0; i < propertyNames.length - 1 && currentObject != null; i++) { - MetaClass mc = GroovySystem.getMetaClassRegistry().getMetaClass(currentObject.getClass()); - currentObject = mc.getProperty(currentObject, propertyNames[i]); + currentObject = getPropertyValue(currentObject, propertyNames[i]); } if (currentObject != null) { - MetaClass mc = GroovySystem.getMetaClassRegistry().getMetaClass(currentObject.getClass()); - mc.setProperty(currentObject, propertyNames[propertyNames.length - 1], null); + setPropertyValueToNull(currentObject, propertyNames[propertyNames.length - 1]); + } + } + + private static Object getPropertyValue(Object object, String propertyName) { + int bracket = propertyName.indexOf('['); + if (bracket == -1) { + MetaClass mc = GroovySystem.getMetaClassRegistry().getMetaClass(object.getClass()); + return mc.getProperty(object, propertyName); + } + + MetaClass mc = GroovySystem.getMetaClassRegistry().getMetaClass(object.getClass()); + Object indexedProperty = mc.getProperty(object, propertyName.substring(0, bracket)); + return getIndexedValue(indexedProperty, propertyName.substring(bracket + 1, propertyName.indexOf(']', bracket))); + } + + private static Object getIndexedValue(Object indexedProperty, String index) { + if (indexedProperty instanceof List) { + return ((List) indexedProperty).get(Integer.parseInt(index)); + } + return ((Map) indexedProperty).get(index); + } + + private static void setPropertyValueToNull(Object object, String propertyName) { + int bracket = propertyName.indexOf('['); + if (bracket == -1) { + MetaClass mc = GroovySystem.getMetaClassRegistry().getMetaClass(object.getClass()); + mc.setProperty(object, propertyName, null); + return; + } + + MetaClass mc = GroovySystem.getMetaClassRegistry().getMetaClass(object.getClass()); + Object indexedProperty = mc.getProperty(object, propertyName.substring(0, bracket)); + String index = propertyName.substring(bracket + 1, propertyName.indexOf(']', bracket)); + if (indexedProperty instanceof List) { + ((List) indexedProperty).set(Integer.parseInt(index), null); + } + else { + ((Map) indexedProperty).put(index, null); } } From e6c318ea11394ee43c62ea509e70ec14c7d067da Mon Sep 17 00:00:00 2001 From: James Fredley Date: Wed, 1 Jul 2026 11:54:25 -0400 Subject: [PATCH 04/10] Add Hibernate 7 secure binding examples Assisted-by: opencode:gpt-5.5 --- .../AdvancedDataBindingController.groovy | 70 ++++++++++++++- .../binding/AdvancedDataBindingSpec.groovy | 89 +++++++++++++++++++ 2 files changed, 158 insertions(+), 1 deletion(-) diff --git a/grails-test-examples/hibernate7/app1/grails-app/controllers/functionaltests/binding/AdvancedDataBindingController.groovy b/grails-test-examples/hibernate7/app1/grails-app/controllers/functionaltests/binding/AdvancedDataBindingController.groovy index 78ed103c0a4..97eb43b9275 100644 --- a/grails-test-examples/hibernate7/app1/grails-app/controllers/functionaltests/binding/AdvancedDataBindingController.groovy +++ b/grails-test-examples/hibernate7/app1/grails-app/controllers/functionaltests/binding/AdvancedDataBindingController.groovy @@ -109,7 +109,75 @@ class AdvancedDataBindingController { contributors: contributorsMap ] as JSON) } - + + def secureBindEmployee() { + def employee = new Employee() + secureBindData(employee, params, ['firstName', 'homeAddress.street', 'homeAddress.city']) + render([ + firstName: employee.firstName, + email: employee.email, + homeAddress: employee.homeAddress ? [ + street: employee.homeAddress.street, + city: employee.homeAddress.city, + state: employee.homeAddress.state + ] : null, + workAddress: employee.workAddress ? [ + street: employee.workAddress.street, + city: employee.workAddress.city, + state: employee.workAddress.state + ] : null + ] as JSON) + } + + def secureBindTeamWithMembers() { + def team = new Team() + secureBindData(team, params, ['name', 'members.name', 'members.role']) + render([ + name: team.name, + members: team.members?.findAll { it != null }?.collect { [name: it.name, role: it.role] } ?: [] + ] as JSON) + } + + def secureBindProjectWithContributors() { + def project = new Project() + secureBindData(project, params, ['name', 'contributors.name', 'contributors.expertise']) + def contributorsMap = project.contributors?.collectEntries { k, v -> + [k, [name: v?.name, expertise: v?.expertise]] + } ?: [:] + render([ + name: project.name, + contributors: contributorsMap + ] as JSON) + } + + def secureBindWithPrefix() { + def employee = new Employee() + secureBindData(employee, params, ['firstName', 'lastName'], 'employee') + render([ + firstName: employee.firstName, + lastName: employee.lastName, + email: employee.email + ] as JSON) + } + + def secureBindWithNullMissing() { + def employee = new Employee(firstName: 'Existing', email: 'existing@example.com') + secureBindData(employee, params, ['firstName', 'email'], nullMissing: true) + render([ + firstName: employee.firstName, + email: employee.email + ] as JSON) + } + + def secureBindWithEmptyAllowlist() { + def employee = new Employee(firstName: 'Existing', email: 'existing@example.com') + secureBindData(employee, params, []) + render([ + firstName: employee.firstName, + email: employee.email + ] as JSON) + } + /** * Test binding with @RequestParameter annotation. */ diff --git a/grails-test-examples/hibernate7/app1/src/integration-test/groovy/functionaltests/binding/AdvancedDataBindingSpec.groovy b/grails-test-examples/hibernate7/app1/src/integration-test/groovy/functionaltests/binding/AdvancedDataBindingSpec.groovy index c90ee6c7646..e0f9f9929d6 100644 --- a/grails-test-examples/hibernate7/app1/src/integration-test/groovy/functionaltests/binding/AdvancedDataBindingSpec.groovy +++ b/grails-test-examples/hibernate7/app1/src/integration-test/groovy/functionaltests/binding/AdvancedDataBindingSpec.groovy @@ -233,6 +233,95 @@ class AdvancedDataBindingSpec extends Specification implements HttpClientSupport } } + def "test secureBindData binds nested allowed properties and blocks unlisted properties"() { + when: + def response = http( + '/advancedDataBinding/secureBindEmployee?firstName=Secure&email=blocked%40example.com&homeAddress.street=123+Secure+St&homeAddress.city=Portland&workAddress.street=999+Blocked+Ave', + ) + + then: + response.assertStatus(200) + with(response.json()) { + firstName == 'Secure' + email == null + homeAddress.street == '123 Secure St' + homeAddress.city == 'Portland' + homeAddress.state == null + workAddress == null + } + } + + def "test secureBindData binds allowed List collection properties"() { + when: + def response = http( + '/advancedDataBinding/secureBindTeamWithMembers?name=Security&members%5B0%5D.name=Alice&members%5B0%5D.role=Lead&members%5B1%5D.name=Bob&members%5B1%5D.role=Developer', + ) + + then: + response.assertStatus(200) + with(response.json()) { + name == 'Security' + members.size() == 2 + members[0].name == 'Alice' + members[0].role == 'Lead' + members[1].name == 'Bob' + members[1].role == 'Developer' + } + } + + def "test secureBindData binds allowed Map collection properties"() { + when: + def response = http( + '/advancedDataBinding/secureBindProjectWithContributors?name=SecureCore&contributors%5Blead%5D.name=John&contributors%5Blead%5D.expertise=Architecture&contributors%5Bdev%5D.name=Jane&contributors%5Bdev%5D.expertise=Testing', + ) + + then: + response.assertStatus(200) + with(response.json()) { + name == 'SecureCore' + contributors.lead.name == 'John' + contributors.lead.expertise == 'Architecture' + contributors.dev.name == 'Jane' + contributors.dev.expertise == 'Testing' + } + } + + def "test secureBindData supports prefix filtering"() { + when: + def response = http( + '/advancedDataBinding/secureBindWithPrefix?employee.firstName=Prefix&employee.lastName=Allowed&employee.email=blocked%40example.com&other.firstName=Ignored', + ) + + then: + response.assertJson(200, [ + firstName: 'Prefix', + lastName : 'Allowed', + email : null + ]) + } + + def "test secureBindData nullMissing clears missing allowed properties"() { + when: + def response = http('/advancedDataBinding/secureBindWithNullMissing?firstName=Updated') + + then: + response.assertJson(200, [ + firstName: 'Updated', + email : null + ]) + } + + def "test secureBindData with empty allowlist binds no properties"() { + when: + def response = http('/advancedDataBinding/secureBindWithEmptyAllowlist?firstName=Blocked&email=blocked%40example.com') + + then: + response.assertJson(200, [ + firstName: 'Existing', + email : 'existing@example.com' + ]) + } + // ========== Selective Property Binding Tests ========== def "test selective property binding using subscript operator"() { From 755daa848f0a324a0cf9034708badaddebd54b4a Mon Sep 17 00:00:00 2001 From: Walter Duque de Estrada Date: Wed, 1 Jul 2026 19:26:17 -0500 Subject: [PATCH 05/10] 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 --- ...ionTransformerCompilationErrorsSpec.groovy | 21 +++++++++ .../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 397ff29386a..235964fa958 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 c330188ca74..e3cd17f3c9b 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 Date: Wed, 1 Jul 2026 21:55:20 -0500 Subject: [PATCH 06/10] fix: address secureBindData review findings (GAP tests now pass) - Enforce that secureBindData's allowedParams resolves to a compile-time literal list (or a reference to a static final constant list), not a dynamically built one. A method call, ternary, non-final field, or variable reassigned from request data is now rejected at compile time, closing the loophole where a controller could pass an attacker-influenced allow-list past the framework validation. - Guard DataBindingUtils.getIndexedValue/setPropertyValueToNull so an out-of-range indexed property (one the binder silently skipped for exceeding autoGrowCollectionLimit) is treated as absent instead of throwing IndexOutOfBoundsException during secureBindData's nullMissing processing. - Guard the pre-existing SimpleDataBinder.processIndexedProperty against a non-numeric bracket index on a List/array property, which previously threw an uncaught NumberFormatException and caused the generic catch in DataBindingUtils to discard the whole BindingResult, reachable via both plain bindData and secureBindData. Co-Authored-By: Claude Sonnet 5 --- .../web/ControllerActionTransformer.java | 105 ++++++++++++---- ...ionTransformerCompilationErrorsSpec.groovy | 112 +++++++++++++++++- .../databinding/SimpleDataBinder.groovy | 19 ++- .../databinding/SimpleDataBinderSpec.groovy | 26 ++++ .../web/databinding/DataBindingUtils.java | 44 +++++-- 5 files changed, 269 insertions(+), 37 deletions(-) diff --git a/grails-controllers/src/main/groovy/org/grails/compiler/web/ControllerActionTransformer.java b/grails-controllers/src/main/groovy/org/grails/compiler/web/ControllerActionTransformer.java index bfb607867f0..d01cb8bf53c 100644 --- a/grails-controllers/src/main/groovy/org/grails/compiler/web/ControllerActionTransformer.java +++ b/grails-controllers/src/main/groovy/org/grails/compiler/web/ControllerActionTransformer.java @@ -45,6 +45,7 @@ import org.codehaus.groovy.ast.ModuleNode; import org.codehaus.groovy.ast.Parameter; import org.codehaus.groovy.ast.PropertyNode; +import org.codehaus.groovy.ast.Variable; import org.codehaus.groovy.ast.expr.ArgumentListExpression; import org.codehaus.groovy.ast.expr.BinaryExpression; import org.codehaus.groovy.ast.expr.BooleanExpression; @@ -275,17 +276,16 @@ private void validateSecureBindDataCalls(SourceUnit source, ClassNode controller if (classNode == null || OBJECT_CLASS.equals(classNode) || !visitedClasses.add(classNode)) { return; } - SecureBindDataCallVisitor visitor = new SecureBindDataCallVisitor(source, controllerClassNode, classNode); for (MethodNode method : classNode.getMethods()) { if (method.getCode() != null && visitedMethods.add(method)) { - method.getCode().visit(visitor); + method.getCode().visit(new SecureBindDataCallVisitor(source, controllerClassNode, classNode)); } validateTraitHelperSecureBindDataCalls(source, method, controllerClassNode, visitedTraitHelpers); } for (PropertyNode property : classNode.getProperties()) { Expression initialExpression = property.getInitialExpression(); if (initialExpression instanceof ClosureExpression) { - ((ClosureExpression) initialExpression).getCode().visit(visitor); + ((ClosureExpression) initialExpression).getCode().visit(new SecureBindDataCallVisitor(source, controllerClassNode, classNode)); } } validateSecureBindDataCalls(source, controllerClassNode, classNode.getSuperClass(), visitedClasses, visitedMethods, visitedTraitHelpers); @@ -298,9 +298,9 @@ private void validateTraitHelperSecureBindDataCalls(SourceUnit source, MethodNod if (traitClass instanceof ClassExpression) { ClassNode helperClass = Traits.findHelper(traitClass.getType()); if (helperClass != null && visitedTraitHelpers.add(helperClass)) { - SecureBindDataCallVisitor visitor = new SecureBindDataCallVisitor(source, controllerClassNode, traitClass.getType(), traitClass.getType()); for (MethodNode helperMethod : helperClass.getMethods()) { if (helperMethod.getCode() != null) { + SecureBindDataCallVisitor visitor = new SecureBindDataCallVisitor(source, controllerClassNode, traitClass.getType(), traitClass.getType()); helperMethod.getCode().visit(visitor); } } @@ -311,10 +311,22 @@ private void validateTraitHelperSecureBindDataCalls(SourceUnit source, MethodNod private static class SecureBindDataCallVisitor extends CodeVisitorSupport { + private enum AllowedParamsCheck { + NOT_PRESENT, NOT_APPLICABLE, LITERAL, NON_LITERAL + } + + private static final String MISSING_ALLOWED_PARAMS_MESSAGE = + "secureBindData requires an explicit allowedParams list. Use secureBindData(target, params, allowedParams)."; + + private static final String NON_LITERAL_ALLOWED_PARAMS_MESSAGE = + "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."; + private final SourceUnit source; private final ClassNode controllerClassNode; private final ClassNode visitedClassNode; private final ClassNode traitClassNode; + private final Map localVariableInitializers = new HashMap<>(); SecureBindDataCallVisitor(SourceUnit source, ClassNode controllerClassNode, ClassNode visitedClassNode) { this(source, controllerClassNode, visitedClassNode, null); @@ -327,12 +339,27 @@ private static class SecureBindDataCallVisitor extends CodeVisitorSupport { this.traitClassNode = traitClassNode; } + @Override + public void visitBinaryExpression(BinaryExpression expression) { + super.visitBinaryExpression(expression); + if (expression.getOperation().getType() == Types.ASSIGN && expression.getLeftExpression() instanceof VariableExpression) { + String variableName = ((VariableExpression) expression.getLeftExpression()).getName(); + localVariableInitializers.put(variableName, expression.getRightExpression()); + } + } + @Override public void visitMethodCallExpression(MethodCallExpression call) { super.visitMethodCallExpression(call); - if (isControllerSecureBindDataCall(call) && !hasAllowedParamsArgument(call)) { - GrailsASTUtils.error(source, call, - "secureBindData requires an explicit allowedParams list. Use secureBindData(target, params, allowedParams)."); + if (!isControllerSecureBindDataCall(call)) { + return; + } + AllowedParamsCheck check = checkAllowedParamsArgument(call); + if (check == AllowedParamsCheck.NOT_PRESENT || check == AllowedParamsCheck.NOT_APPLICABLE) { + GrailsASTUtils.error(source, call, MISSING_ALLOWED_PARAMS_MESSAGE); + } + else if (check == AllowedParamsCheck.NON_LITERAL) { + GrailsASTUtils.error(source, call, NON_LITERAL_ALLOWED_PARAMS_MESSAGE); } } @@ -465,49 +492,85 @@ private boolean isControllerReceiver(VariableExpression objectExpression) { return "this".equals(variableName) || "$self".equals(variableName); } - private boolean hasAllowedParamsArgument(MethodCallExpression call) { + private AllowedParamsCheck checkAllowedParamsArgument(MethodCallExpression call) { Expression arguments = call.getArguments(); if (!(arguments instanceof TupleExpression)) { - return false; + return AllowedParamsCheck.NOT_PRESENT; } List expressions = ((TupleExpression) arguments).getExpressions(); if (expressions.isEmpty()) { - return false; + return AllowedParamsCheck.NOT_PRESENT; } - return hasAllowedParamsAt(expressions, 2) || hasAllowedParamsAt(expressions, 3); + AllowedParamsCheck atArgumentTwo = checkAllowedParamsAt(expressions, 2); + if (atArgumentTwo == AllowedParamsCheck.LITERAL || atArgumentTwo == AllowedParamsCheck.NON_LITERAL) { + return atArgumentTwo; + } + return checkAllowedParamsAt(expressions, 3); } - private boolean hasAllowedParamsAt(List expressions, int allowedParamsIndex) { + private AllowedParamsCheck checkAllowedParamsAt(List expressions, int allowedParamsIndex) { if (allowedParamsIndex >= expressions.size()) { - return false; + return AllowedParamsCheck.NOT_PRESENT; } Expression allowedParams = expressions.get(allowedParamsIndex); if (allowedParams instanceof ConstantExpression || allowedParams instanceof MapExpression) { - return false; + return AllowedParamsCheck.NOT_APPLICABLE; + } + if (allowedParams instanceof ListExpression) { + return isLiteralListExpression(allowedParams) ? AllowedParamsCheck.LITERAL : AllowedParamsCheck.NON_LITERAL; } if (allowedParams instanceof VariableExpression) { - return isAllowedParamsVariable((VariableExpression) allowedParams); + return checkAllowedParamsVariable((VariableExpression) allowedParams); } - return true; + // Any other expression shape (method calls, ternaries, property access, GStrings, etc.) + // could be built from request-controlled data, so it can never be treated as a safe literal. + return AllowedParamsCheck.NON_LITERAL; } - private boolean isAllowedParamsVariable(VariableExpression allowedParams) { + private AllowedParamsCheck checkAllowedParamsVariable(VariableExpression allowedParams) { String variableName = allowedParams.getName(); if ("params".equals(variableName) || "request".equals(variableName)) { - return false; + return AllowedParamsCheck.NOT_APPLICABLE; } ClassNode variableType = allowedParams.getType(); - if (variableType == null || OBJECT_CLASS.equals(variableType) || ClassHelper.OBJECT_TYPE.equals(variableType)) { - return true; + boolean plausibleAllowedParamsType = variableType == null || OBJECT_CLASS.equals(variableType) || + ClassHelper.OBJECT_TYPE.equals(variableType) || + acceptsAllowedParamsType(variableType, ClassHelper.LIST_TYPE) || + acceptsAllowedParamsType(variableType, new ClassNode(Collection.class)); + if (!plausibleAllowedParamsType) { + return AllowedParamsCheck.NOT_APPLICABLE; } - return acceptsAllowedParamsType(variableType, ClassHelper.LIST_TYPE) || acceptsAllowedParamsType(variableType, new ClassNode(Collection.class)); + + return resolvesToLiteralList(allowedParams) ? AllowedParamsCheck.LITERAL : AllowedParamsCheck.NON_LITERAL; } private boolean acceptsAllowedParamsType(ClassNode variableType, ClassNode allowedParamsType) { return variableType.equals(allowedParamsType) || variableType.isDerivedFrom(allowedParamsType) || variableType.implementsInterface(allowedParamsType); } + + private boolean resolvesToLiteralList(VariableExpression allowedParams) { + Variable accessedVariable = allowedParams.getAccessedVariable(); + if (accessedVariable instanceof FieldNode) { + FieldNode field = (FieldNode) accessedVariable; + boolean isConstantField = Modifier.isStatic(field.getModifiers()) && Modifier.isFinal(field.getModifiers()); + return isConstantField && isLiteralListExpression(field.getInitialExpression()); + } + return isLiteralListExpression(localVariableInitializers.get(allowedParams.getName())); + } + + private boolean isLiteralListExpression(Expression expression) { + if (!(expression instanceof ListExpression)) { + return false; + } + for (Expression element : ((ListExpression) expression).getExpressions()) { + if (!(element instanceof ConstantExpression)) { + return false; + } + } + return true; + } } private void processMethods(ClassNode classNode, SourceUnit source, 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 235964fa958..7415002454b 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 @@ -488,7 +488,7 @@ class ControllerActionTransformerCompilationErrorsSpec extends Specification { 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'() { + void 'Test secureBindData rejects 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 { @@ -508,4 +508,114 @@ class ControllerActionTransformerCompilationErrorsSpec extends Specification { 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.' } + + void 'Test secureBindData rejects an allowedParams list built from a method call'() { + when: 'A controller passes the result of a method call directly as allowedParams' + gcl.parseClass(''' + class MethodCallAllowedParamsController { + def save() { + def target = new Person() + secureBindData(target, params, computeAllowedParams()) + } + + List computeAllowedParams() { + ['name'] + } + } + + class Person { + String name + } + ''') + + then: + 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.' + } + + void 'Test secureBindData rejects an allowedParams list literal with a non-constant element'() { + when: 'A controller mixes a literal list with a variable-derived element' + gcl.parseClass(''' + class MixedLiteralAllowedParamsController { + def save() { + def target = new Person() + def extraField = params.extraField + secureBindData(target, params, ['name', extraField]) + } + } + + class Person { + String name + } + ''') + + then: + 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.' + } + + void 'Test secureBindData rejects an allowedParams variable reassigned from request data'() { + when: 'A controller reassigns an allowedParams variable after its initial literal declaration' + gcl.parseClass(''' + class ReassignedAllowedParamsController { + def save() { + def target = new Person() + List allowedParams = ['name'] + allowedParams = params.fields?.tokenize(',') + secureBindData(target, params, allowedParams) + } + } + + class Person { + String name + } + ''') + + then: + 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.' + } + + void 'Test secureBindData accepts an allowedParams reference to a static final constant list'() { + when: 'A controller reuses a compile-time constant list of allowed fields across actions' + gcl.parseClass(''' + class ConstantAllowedParamsController { + static final List ALLOWED_FIELDS = ['name'] + + def save() { + def target = new Person() + secureBindData(target, params, ALLOWED_FIELDS) + } + } + + class Person { + String name + } + ''') + + then: + noExceptionThrown() + } + + void 'Test secureBindData rejects an allowedParams reference to a non-final field'() { + when: 'A controller declares a mutable field intended to hold the allowed field list' + gcl.parseClass(''' + class MutableFieldAllowedParamsController { + static List allowedFields = ['name'] + + def save() { + def target = new Person() + secureBindData(target, params, allowedFields) + } + } + + class Person { + String name + } + ''') + + then: + 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-databinding-core/src/main/groovy/grails/databinding/SimpleDataBinder.groovy b/grails-databinding-core/src/main/groovy/grails/databinding/SimpleDataBinder.groovy index 3f3ed13bae3..c569f1818cb 100755 --- a/grails-databinding-core/src/main/groovy/grails/databinding/SimpleDataBinder.groovy +++ b/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 = parseIndex(indexedPropertyReferenceDescriptor.index) + 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 = parseIndex(indexedPropertyReferenceDescriptor.index) + if (index == null) { + return + } Collection collectionInstance = initializeCollection(obj, propName, propertyType) def indexedInstance = null if (!(Set.isAssignableFrom(propertyType))) { @@ -394,6 +400,15 @@ class SimpleDataBinder implements DataBinder { } } + protected Integer parseIndex(String index) { + try { + Integer.parseInt(index) + } + catch (NumberFormatException e) { + null + } + } + @CompileStatic(TypeCheckingMode.SKIP) protected initializeArray(obj, String propertyName, Class arrayType, int index) { Object[] array = obj[propertyName] diff --git a/grails-databinding-core/src/test/groovy/grails/databinding/SimpleDataBinderSpec.groovy b/grails-databinding-core/src/test/groovy/grails/databinding/SimpleDataBinderSpec.groovy index 67c2e7cff3c..80a8b9b6947 100755 --- a/grails-databinding-core/src/test/groovy/grails/databinding/SimpleDataBinderSpec.groovy +++ b/grails-databinding-core/src/test/groovy/grails/databinding/SimpleDataBinderSpec.groovy @@ -559,6 +559,32 @@ class SimpleDataBinderSpec extends Specification { widget.names[2] == 'two' } + void 'Test binding a non-numeric indexed property to a List does not throw'() { + given: + def binder = new SimpleDataBinder() + def widget = new Widget(names: ['existing']) + + when: + binder.bind widget, new SimpleMapDataBindingSource(['names[abc]': 'hacked']) + + then: + noExceptionThrown() + widget.names == ['existing'] + } + + void 'Test binding a non-numeric indexed property to an array does not throw'() { + given: + def binder = new SimpleDataBinder() + def widget = new Widget() + + when: + binder.bind widget, new SimpleMapDataBindingSource(['integers[abc]': 42]) + + then: + noExceptionThrown() + widget.integers == null + } + void 'Test @BindUsing on a List'() { given: def binder = new SimpleDataBinder() diff --git a/grails-web-databinding/src/main/groovy/grails/web/databinding/DataBindingUtils.java b/grails-web-databinding/src/main/groovy/grails/web/databinding/DataBindingUtils.java index 15cce6b9356..e25cf050eb7 100644 --- a/grails-web-databinding/src/main/groovy/grails/web/databinding/DataBindingUtils.java +++ b/grails-web-databinding/src/main/groovy/grails/web/databinding/DataBindingUtils.java @@ -412,15 +412,15 @@ private static boolean hasStructuredTargetMapValues(Map map) { private static boolean isStructuredMapValueType(Class valueType) { Package valuePackage = valueType.getPackage(); - return !valueType.isPrimitive() - && (valuePackage == null || !valuePackage.getName().startsWith("java.")) - && !CharSequence.class.isAssignableFrom(valueType) - && !Number.class.isAssignableFrom(valueType) - && !Boolean.class.isAssignableFrom(valueType) - && !Enum.class.isAssignableFrom(valueType) - && !Map.class.isAssignableFrom(valueType) - && !Collection.class.isAssignableFrom(valueType) - && !Object.class.equals(valueType); + return !valueType.isPrimitive() && + (valuePackage == null || !valuePackage.getName().startsWith("java.")) && + !CharSequence.class.isAssignableFrom(valueType) && + !Number.class.isAssignableFrom(valueType) && + !Boolean.class.isAssignableFrom(valueType) && + !Enum.class.isAssignableFrom(valueType) && + !Map.class.isAssignableFrom(valueType) && + !Collection.class.isAssignableFrom(valueType) && + !Object.class.equals(valueType); } private static Class getMapValueType(Object target, Class targetType, String propertyName) { @@ -1113,9 +1113,14 @@ private static Object getPropertyValue(Object object, String propertyName) { private static Object getIndexedValue(Object indexedProperty, String index) { if (indexedProperty instanceof List) { - return ((List) indexedProperty).get(Integer.parseInt(index)); + List list = (List) indexedProperty; + Integer parsedIndex = parseIndex(index); + return parsedIndex != null && parsedIndex >= 0 && parsedIndex < list.size() ? list.get(parsedIndex) : null; } - return ((Map) indexedProperty).get(index); + if (indexedProperty instanceof Map) { + return ((Map) indexedProperty).get(index); + } + return null; } private static void setPropertyValueToNull(Object object, String propertyName) { @@ -1130,13 +1135,26 @@ private static void setPropertyValueToNull(Object object, String propertyName) { Object indexedProperty = mc.getProperty(object, propertyName.substring(0, bracket)); String index = propertyName.substring(bracket + 1, propertyName.indexOf(']', bracket)); if (indexedProperty instanceof List) { - ((List) indexedProperty).set(Integer.parseInt(index), null); + List list = (List) indexedProperty; + Integer parsedIndex = parseIndex(index); + if (parsedIndex != null && parsedIndex >= 0 && parsedIndex < list.size()) { + list.set(parsedIndex, null); + } } - else { + else if (indexedProperty instanceof Map) { ((Map) indexedProperty).put(index, null); } } + private static Integer parseIndex(String index) { + try { + return Integer.valueOf(index); + } + catch (NumberFormatException e) { + return null; + } + } + /** * Binds the given source object to the given target object performing type conversion if necessary * From f5bfc9296ed2b413583b5509e59b8215b87f9fe7 Mon Sep 17 00:00:00 2001 From: Walter Duque de Estrada Date: Thu, 2 Jul 2026 00:57:32 -0500 Subject: [PATCH 07/10] docs+test: address remaining medium-severity secureBindData review findings - Warn in both the guide and reference docs that allowedParams must be developer-controlled (a literal list or a static final constant), never built from params/request data, matching the compile-time enforcement. - Add secureBindData coverage proving @BindUsing and @BindingFormat properties bind correctly when allowed and stay unbound when not in the allowlist. - Rename the three now-fixed "GAP:" test titles to describe the guaranteed behavior instead of the prior defect. Co-Authored-By: Claude Sonnet 5 --- .../theWebLayer/controllers/dataBinding.adoc | 2 + .../en/ref/Controllers/secureBindData.adoc | 2 + .../web/servlet/BindDataMethodTests.groovy | 78 ++++++++++++++++++- 3 files changed, 79 insertions(+), 3 deletions(-) diff --git a/grails-doc/src/en/guide/theWebLayer/controllers/dataBinding.adoc b/grails-doc/src/en/guide/theWebLayer/controllers/dataBinding.adoc index 7beac7eb2be..6bba07c2cfe 100644 --- a/grails-doc/src/en/guide/theWebLayer/controllers/dataBinding.adoc +++ b/grails-doc/src/en/guide/theWebLayer/controllers/dataBinding.adoc @@ -1095,6 +1095,8 @@ def p = new Person() secureBindData(p, params, ['firstName', 'lastName']) ---- +WARNING: The allowlist must be developer-controlled: a literal list of property names, or a reference to a `static final` constant list. Building it from `params`, `request`, or other request-derived data is a compilation error, since it would let an attacker choose which properties are bindable. + Use nested property paths when allowing nested binding: [source,groovy] diff --git a/grails-doc/src/en/ref/Controllers/secureBindData.adoc b/grails-doc/src/en/ref/Controllers/secureBindData.adoc index f4d8cec2aef..d9fb7c3ea63 100644 --- a/grails-doc/src/en/ref/Controllers/secureBindData.adoc +++ b/grails-doc/src/en/ref/Controllers/secureBindData.adoc @@ -66,6 +66,8 @@ Arguments: The `allowedParams` list is required. In controllers, calling `secureBindData` without an explicit allowlist is a compilation error. +WARNING: `allowedParams` must be developer-controlled. It must be a literal list of property names, such as `['firstName', 'lastName']`, or a reference to a `static final` constant list. Building the list from `params`, `request`, or any other request-derived data is a compilation error in controllers, since it would let an attacker choose which properties are bindable and defeat the purpose of `secureBindData`. + An empty allowlist binds no properties. This differs from link:bindData.html[bindData] with an empty `include` list, which leaves all otherwise-bindable properties eligible for binding. Use nested property paths such as `homeAddress.street` to allow nested binding. Properties not named in `allowedParams` are ignored. 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 e3cd17f3c9b..d54b5a14d2a 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 @@ -19,6 +19,8 @@ package org.grails.web.servlet import grails.artefact.Artefact +import grails.databinding.BindUsing +import grails.databinding.BindingFormat import grails.testing.web.controllers.ControllerUnitTest import spock.lang.Specification @@ -87,7 +89,7 @@ class BindDataMethodTests extends Specification implements ControllerUnitTest members = [] Map contributors = [:] } + +class SecureBindFormattedCommand { + @BindUsing({ obj, source -> + source['loudName']?.toString()?.toUpperCase() + }) + String loudName + + @BindingFormat('MMddyyyy') + Date formattedDate +} From fb2bb35879826ef1beefcff0f2d17ae044dcfd0b Mon Sep 17 00:00:00 2001 From: James Fredley Date: Thu, 2 Jul 2026 09:45:46 -0400 Subject: [PATCH 08/10] Fix null binding error message in secure/domain bind fallback The generic exception fallback in secureBindObjectToInstance and bindObjectToDomainInstance passed e.getMessage() straight into ObjectError, which can be null and yields an empty/unhelpful binding error. Add resolveBindingErrorMessage to fall back to the exception class name when the message is null, keeping binding errors actionable. Addresses GitHub Copilot review findings on PR #15808. Assisted-by: claude-code:claude-4.8-opus --- .../groovy/grails/web/databinding/DataBindingUtils.java | 9 +++++++-- 1 file changed, 7 insertions(+), 2 deletions(-) diff --git a/grails-web-databinding/src/main/groovy/grails/web/databinding/DataBindingUtils.java b/grails-web-databinding/src/main/groovy/grails/web/databinding/DataBindingUtils.java index e25cf050eb7..f4f9df7a84c 100644 --- a/grails-web-databinding/src/main/groovy/grails/web/databinding/DataBindingUtils.java +++ b/grails-web-databinding/src/main/groovy/grails/web/databinding/DataBindingUtils.java @@ -279,7 +279,7 @@ public static BindingResult secureBindObjectToInstance(Object object, Object sou bindingResult.addError(new ObjectError(bindingResult.getObjectName(), codes, null, defaultMessage)); } catch (Exception e) { bindingResult = new BeanPropertyBindingResult(object, object.getClass().getName()); - bindingResult.addError(new ObjectError(bindingResult.getObjectName(), e.getMessage())); + bindingResult.addError(new ObjectError(bindingResult.getObjectName(), resolveBindingErrorMessage(e))); } return processBindingResult(entity, object, bindingResult); @@ -1155,6 +1155,11 @@ private static Integer parseIndex(String index) { } } + private static String resolveBindingErrorMessage(Exception e) { + String message = e.getMessage(); + return message != null ? message : e.getClass().getName(); + } + /** * Binds the given source object to the given target object performing type conversion if necessary * @@ -1188,7 +1193,7 @@ public static BindingResult bindObjectToDomainInstance(PersistentEntity entity, bindingResult.addError(new ObjectError(bindingResult.getObjectName(), codes, null, defaultMessage)); } catch (Exception e) { bindingResult = new BeanPropertyBindingResult(object, object.getClass().getName()); - bindingResult.addError(new ObjectError(bindingResult.getObjectName(), e.getMessage())); + bindingResult.addError(new ObjectError(bindingResult.getObjectName(), resolveBindingErrorMessage(e))); } return processBindingResult(entity, object, bindingResult); From 8d4a2b2c4f8b2960b39ea1f03f4650184c13bca6 Mon Sep 17 00:00:00 2001 From: Walter Duque de Estrada Date: Thu, 2 Jul 2026 08:56:14 -0500 Subject: [PATCH 09/10] style: revert explicit type declarations in SimpleDataBinder Local variable types are inferable from parseIndex's return type; keep the existing def-typed style used throughout this file. Per review from @jdaugherty on PR #15808. --- .../main/groovy/grails/databinding/SimpleDataBinder.groovy | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/grails-databinding-core/src/main/groovy/grails/databinding/SimpleDataBinder.groovy b/grails-databinding-core/src/main/groovy/grails/databinding/SimpleDataBinder.groovy index c569f1818cb..014f002ec01 100755 --- a/grails-databinding-core/src/main/groovy/grails/databinding/SimpleDataBinder.groovy +++ b/grails-databinding-core/src/main/groovy/grails/databinding/SimpleDataBinder.groovy @@ -333,7 +333,7 @@ class SimpleDataBinder implements DataBinder { } if (propertyType.isArray()) { - Integer index = parseIndex(indexedPropertyReferenceDescriptor.index) + def index = parseIndex(indexedPropertyReferenceDescriptor.index) if (index == null) { return } @@ -342,7 +342,7 @@ class SimpleDataBinder implements DataBinder { addElementToArrayAt(array, index, val) } } else if (Collection.isAssignableFrom(propertyType)) { - Integer index = parseIndex(indexedPropertyReferenceDescriptor.index) + def index = parseIndex(indexedPropertyReferenceDescriptor.index) if (index == null) { return } From 3f61e1c024f9b9cddefc407ba5a5a1fd113de823 Mon Sep 17 00:00:00 2001 From: James Fredley Date: Thu, 2 Jul 2026 10:15:05 -0400 Subject: [PATCH 10/10] Add regression test for null binding error message fallback Verify that when the underlying data binder throws an exception whose message is null, both bindData and secureBindData surface a non-null ObjectError default message (the exception class name) rather than a null/empty one. The spec registers a throwing grailsWebDataBinder in its own ControllerUnitTest context so it does not affect sibling specs. Locks in the resolveBindingErrorMessage fallback added for the Copilot review findings on PR #15808. Assisted-by: claude-code:claude-4.8-opus --- .../BindDataBinderExceptionSpec.groovy | 113 ++++++++++++++++++ 1 file changed, 113 insertions(+) create mode 100644 grails-test-suite-web/src/test/groovy/org/grails/web/servlet/BindDataBinderExceptionSpec.groovy diff --git a/grails-test-suite-web/src/test/groovy/org/grails/web/servlet/BindDataBinderExceptionSpec.groovy b/grails-test-suite-web/src/test/groovy/org/grails/web/servlet/BindDataBinderExceptionSpec.groovy new file mode 100644 index 00000000000..0e8c04c7487 --- /dev/null +++ b/grails-test-suite-web/src/test/groovy/org/grails/web/servlet/BindDataBinderExceptionSpec.groovy @@ -0,0 +1,113 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.grails.web.servlet + +import grails.artefact.Artefact +import grails.databinding.DataBinder +import grails.databinding.DataBindingSource +import grails.databinding.events.DataBindingListener +import grails.testing.web.controllers.ControllerUnitTest +import groovy.xml.slurpersupport.GPathResult +import org.grails.spring.beans.factory.InstanceFactoryBean +import spock.lang.Specification + +/** + * Verifies that when the underlying data binder throws an exception whose message is null, + * bindData and secureBindData still produce a non-null, actionable ObjectError default message + * rather than a null/empty one. + */ +class BindDataBinderExceptionSpec extends Specification implements ControllerUnitTest { + + Closure doWithSpring() {{ -> + grailsWebDataBinder(InstanceFactoryBean, new ThrowingDataBinder(), DataBinder) + }} + + void 'Test bindData falls back to the exception class name when the binder throws with a null message'() { + when: + def model = controller.bindDataWithThrowingBinder() + def bindingResult = model.bindingResult + + then: + bindingResult.hasErrors() + bindingResult.errorCount == 1 + bindingResult.allErrors[0].defaultMessage == 'java.lang.RuntimeException' + } + + void 'Test secureBindData falls back to the exception class name when the binder throws with a null message'() { + when: + def model = controller.secureBindDataWithThrowingBinder() + def bindingResult = model.bindingResult + + then: + bindingResult.hasErrors() + bindingResult.errorCount == 1 + bindingResult.allErrors[0].defaultMessage == 'java.lang.RuntimeException' + } +} + +@Artefact('Controller') +class ExceptionBindingController { + + def bindDataWithThrowingBinder() { + def target = new ExceptionCommandObject() + def bindingResult = bindData(target, [name: 'Marc Palmer']) + [bindingResult: bindingResult] + } + + def secureBindDataWithThrowingBinder() { + def target = new ExceptionCommandObject() + def bindingResult = secureBindData(target, [name: 'Marc Palmer'], ['name']) + [bindingResult: bindingResult] + } +} + +class ExceptionCommandObject { + String name +} + +class ThrowingDataBinder implements DataBinder { + + void bind(Object obj, DataBindingSource source, String filter, List whiteList, List blackList, DataBindingListener listener) { + throw new RuntimeException() + } + + void bind(Object obj, DataBindingSource source, String filter, List whiteList, List blackList) { + throw new RuntimeException() + } + + void bind(Object obj, GPathResult gpath) { + throw new RuntimeException() + } + + void bind(Object obj, DataBindingSource source, List whiteList, List blackList) { + throw new RuntimeException() + } + + void bind(Object obj, DataBindingSource source, List whiteList) { + throw new RuntimeException() + } + + void bind(Object obj, DataBindingSource source, DataBindingListener listener) { + throw new RuntimeException() + } + + void bind(Object obj, DataBindingSource source) { + throw new RuntimeException() + } +}