Harden bindData against mass assignment

grails-controllers/src/main/groovy/grails/artefact/Controller.groovy

@@ -363,6 +363,11 @@ trait Controller implements ResponseRenderer, ResponseRedirector, RequestForward
      */
     @Generated
     def initializeCommandObject(final Class type, final String commandObjectParameterName) throws Exception {
+        initializeCommandObject(type, commandObjectParameterName, null)
+    }
+
+    @Generated
+    def initializeCommandObject(final Class type, final String commandObjectParameterName, final List bindAllowedProperties) throws Exception {
         final HttpServletRequest request = getRequest()
         def commandObjectInstance = null
         try {
@@ -434,7 +439,8 @@ trait Controller implements ResponseRenderer, ResponseRedirector, RequestForward
                 }
 
                 if (shouldDoDataBinding) {
-                    bindData(commandObjectInstance, commandObjectBindingSource, Collections.EMPTY_MAP, null)
+                    final Map includeExclude = bindAllowedProperties == null ? Collections.EMPTY_MAP : [include: bindAllowedProperties]
+                    bindData(commandObjectInstance, commandObjectBindingSource, includeExclude, null)
                 }
             }
         } catch (Exception e) {

grails-controllers/src/main/groovy/org/grails/compiler/web/ControllerActionTransformer.java

@@ -42,6 +42,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;
@@ -93,6 +94,7 @@
 import grails.web.Action;
 import grails.web.RequestParameter;
 import grails.web.controllers.ControllerMethod;
+import grails.web.databinding.BindAllowed;
 import org.grails.compiler.injection.GrailsASTUtils;
 import org.grails.compiler.injection.TraitInjectionUtils;
 import org.grails.core.DefaultGrailsControllerClass;
@@ -185,6 +187,7 @@ public class ControllerActionTransformer implements GrailsArtefactClassInjector,
             GrailsResourceUtils.GRAILS_APP_DIR + "/controllers/(.+)Controller\\.groovy");
     private static final String ALLOWED_METHODS_HANDLED_ATTRIBUTE_NAME = "ALLOWED_METHODS_HANDLED";
     private static final ClassNode OBJECT_CLASS = new ClassNode(Object.class);
+    private static final ClassNode BIND_ALLOWED_CLASS_NODE = new ClassNode(BindAllowed.class);
     public static final AnnotationNode ACTION_ANNOTATION_NODE = new AnnotationNode(
             new ClassNode(Action.class));
     private static final String ACTION_MEMBER_TARGET = "commandObjects";
@@ -722,6 +725,7 @@ protected void initializeMethodParameter(final ClassNode classNode, final BlockS
         String requestParameterName = paramName;
         List<AnnotationNode> requestParameters = param.getAnnotations(
                 new ClassNode(RequestParameter.class));
+        List<AnnotationNode> bindAllowedAnnotations = param.getAnnotations(BIND_ALLOWED_CLASS_NODE);
 
         //Check to see if the method was inherited from a trait
         if (actionNode instanceof MethodNode && paramName.startsWith("arg")) {
@@ -743,6 +747,7 @@ protected void initializeMethodParameter(final ClassNode classNode, final BlockS
                             //Set the request parameter name based off of the parameter in the trait helper method
                             requestParameterName = helperParam.getName();
                             requestParameters = helperParam.getAnnotations(new ClassNode(RequestParameter.class));
+                            bindAllowedAnnotations = helperParam.getAnnotations(BIND_ALLOWED_CLASS_NODE);
                         }
                     }
                 }
@@ -759,14 +764,15 @@ protected void initializeMethodParameter(final ClassNode classNode, final BlockS
         } else if (paramTypeClassNode.equals(new ClassNode(String.class))) {
             initializeStringParameter(classNode, wrapper, param, requestParameterName);
         } else if (!paramTypeClassNode.equals(OBJECT_CLASS)) {
+            final Expression bindAllowedExpression = getBindAllowedExpression(source, param, bindAllowedAnnotations);
             initializeAndValidateCommandObjectParameter(wrapper, classNode, paramTypeClassNode,
-                    actionNode, actionName, paramName, source, context);
+                    actionNode, actionName, paramName, bindAllowedExpression, source, context);
         }
     }
 
     protected void initializeAndValidateCommandObjectParameter(final BlockStatement wrapper,
             final ClassNode controllerNode, final ClassNode commandObjectNode,
-            final ASTNode actionNode, final String actionName, final String paramName,
+            final ASTNode actionNode, final String actionName, final String paramName, final Expression bindAllowedExpression,
             final SourceUnit source, final GeneratorContext context) {
         final DeclarationExpression declareCoExpression = declX(localVarX(paramName, commandObjectNode), new EmptyExpression());
         wrapper.addStatement(stmt(declareCoExpression));
@@ -778,7 +784,7 @@ protected void initializeAndValidateCommandObjectParameter(final BlockStatement
                     "].  Interface types and abstract class types are not supported as command objects.  This parameter will be ignored.";
             GrailsASTUtils.warning(source, actionNode, warningMessage);
         } else {
-            initializeCommandObjectParameter(wrapper, commandObjectNode, paramName, source);
+            initializeCommandObjectParameter(wrapper, commandObjectNode, paramName, bindAllowedExpression, source);
 
             @SuppressWarnings("unchecked")
             boolean argumentIsValidateable = GrailsASTUtils.hasAnyAnnotations(
@@ -854,14 +860,102 @@ protected void initializeAndValidateCommandObjectParameter(final BlockStatement
     }
 
     protected void initializeCommandObjectParameter(final BlockStatement wrapper,
-            final ClassNode commandObjectNode, final String paramName, SourceUnit source) {
+            final ClassNode commandObjectNode, final String paramName, final Expression bindAllowedExpression, SourceUnit source) {
         final ArgumentListExpression initializeCommandObjectArguments = args(classX(commandObjectNode), constX(paramName));
+        if (bindAllowedExpression != null) {
+            initializeCommandObjectArguments.addExpression(bindAllowedExpression);
+        }
         final MethodCallExpression initializeCommandObjectMethodCall = callThisX("initializeCommandObject", initializeCommandObjectArguments);
         applyDefaultMethodTarget(initializeCommandObjectMethodCall, commandObjectNode);
         final Expression assignCommandObjectToParameter = assignX(varX(paramName), initializeCommandObjectMethodCall);
         wrapper.addStatement(stmt(assignCommandObjectToParameter));
     }
 
+    private Expression getBindAllowedExpression(final SourceUnit source, final Parameter param, final List<AnnotationNode> bindAllowedAnnotations) {
+        if (bindAllowedAnnotations == null || bindAllowedAnnotations.isEmpty()) {
+            return null;
+        }
+        final AnnotationNode bindAllowedAnnotation = bindAllowedAnnotations.get(0);
+        final Expression valueExpression = bindAllowedAnnotation.getMember("value");
+        final ListExpression allowedProperties = resolveBindAllowedProperties(valueExpression);
+        if (allowedProperties == null) {
+            GrailsASTUtils.error(source, param, "@BindAllowed requires a literal list of property names or a static final constant list.");
+            return new ListExpression();
+        }
+        return allowedProperties;
+    }
+
+    private ListExpression resolveBindAllowedProperties(final Expression expression) {
+        if (expression instanceof ConstantExpression) {
+            final Object value = ((ConstantExpression) expression).getValue();
+            if (value instanceof String) {
+                final ListExpression listExpression = new ListExpression();
+                listExpression.addExpression(new ConstantExpression(value));
+                return listExpression;
+            }
+        }
+        if (expression instanceof ListExpression) {
+            return copyStringLiteralList((ListExpression) expression);
+        }
+        final FieldNode constantField = resolveStaticFinalField(expression);
+        if (constantField != null) {
+            return resolveBindAllowedProperties(constantField.getInitialExpression());
+        }
+        return null;
+    }
+
+    private ListExpression copyStringLiteralList(final ListExpression sourceList) {
+        final ListExpression listExpression = new ListExpression();
+        for (Expression element : sourceList.getExpressions()) {
+            final Object value = resolveBindAllowedStringValue(element);
+            if (!(value instanceof String)) {
+                return null;
+            }
+            listExpression.addExpression(new ConstantExpression(value));
+        }
+        return listExpression;
+    }
+
+    private Object resolveBindAllowedStringValue(final Expression expression) {
+        if (expression instanceof ConstantExpression) {
+            return ((ConstantExpression) expression).getValue();
+        }
+        final FieldNode constantField = resolveStaticFinalField(expression);
+        if (constantField != null && constantField.getInitialExpression() instanceof ConstantExpression) {
+            return ((ConstantExpression) constantField.getInitialExpression()).getValue();
+        }
+        return null;
+    }
+
+    private FieldNode resolveStaticFinalField(final Expression expression) {
+        if (expression instanceof VariableExpression) {
+            final Variable accessedVariable = ((VariableExpression) expression).getAccessedVariable();
+            if (accessedVariable instanceof FieldNode) {
+                return staticFinalFieldOrNull((FieldNode) accessedVariable);
+            }
+        }
+        if (expression instanceof PropertyExpression) {
+            final PropertyExpression propertyExpression = (PropertyExpression) expression;
+            final Expression objectExpression = propertyExpression.getObjectExpression();
+            if (objectExpression instanceof ClassExpression) {
+                final FieldNode fieldNode = ((ClassExpression) objectExpression).getType().getField(propertyExpression.getPropertyAsString());
+                return staticFinalFieldOrNull(fieldNode);
+            }
+        }
+        return null;
+    }
+
+    private FieldNode staticFinalFieldOrNull(final FieldNode fieldNode) {
+        if (fieldNode == null) {
+            return null;
+        }
+        final int modifiers = fieldNode.getModifiers();
+        if (Modifier.isStatic(modifiers) && Modifier.isFinal(modifiers)) {
+            return fieldNode;
+        }
+        return null;
+    }
+
     /**
      * Checks to see if a Module is defined at a path which includes the specified substring
      *

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

@@ -1085,6 +1085,6 @@ def p = new Person()
 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.
+NOTE: If an empty List is provided as a value for the `include` parameter then no fields will be bound.
 
-The link:{constraintsRef}bindable.html[bindable] constraint can be used to globally prevent data binding for certain properties.
+The link:{constraintsRef}bindable.html[bindable] constraint can be used to prevent data binding for sensitive properties by declaring `bindable: false`.  Applications may set `grails.databinding.denyByDefault = true` to require `bindable: true` before a property participates in default mass binding.  Controller action command object parameters may also be annotated with `grails.web.databinding.BindAllowed` to allow only the listed fields for that action.

grails-doc/src/en/guide/upgrading.adoc

@@ -17,3 +17,8 @@ specific language governing permissions and limitations
 under the License.
 ////
 
+=== Data Binding Defaults
+
+Grails 8 keeps the legacy mass data binding default so existing applications continue to bind statically typed instance properties unless they are marked `bindable: false`.  Empty `include` lists bind no properties, and controller action command object parameters may use `@BindAllowed` to allow only the listed properties for that action.
+
+Applications may opt in to a stricter default by setting `grails.databinding.denyByDefault = true`.  When enabled, properties must declare `bindable: true`, be supplied through `bindData(..., [include: ...])`, or be listed on a controller action parameter with `@BindAllowed` to participate in automatic binding.  Grails 9 may make this stricter default the standard behavior.

grails-doc/src/en/ref/Constraints/bindable.adoc

@@ -79,11 +79,12 @@ assert 'Percussion' == employee.department
 assert 42.0 == employee.salary
 ----
 
-Statically typed instance properties are bindable by default.  Properties which are not bindable by default are those related to transient fields, dynamically typed properties and static properties.
+Properties are bindable by default in Grails 8 unless they are marked with `bindable: false`, are transient fields, are dynamically typed properties, or are static properties.
+
+Applications may opt in to a stricter default by setting `grails.databinding.denyByDefault = true`.  When enabled, only properties declared with `bindable: true` or supplied through `bindData(..., [include: ...])` are included in default mass data binding.  Properties with `bindable: false` remain non-bindable.  Grails 9 may make this stricter default the standard behavior.
 
 See the link:{guidePath}theWebLayer.html#dataBinding[data binding] section for more details on data binding.
 
 NOTE: The bindable constraint must be assigned a literal boolean value.  Dynamic expressions are not valid values for the bindable constraint.  The value must be the literal `true` or `false`.
 
 The bindable constraint must be applied in the constraints closure which is defined in the relevant class.  This means that bindable may not be used as a shared constraint.
-

grails-doc/src/en/ref/Controllers/bindData.adoc

@@ -60,7 +60,29 @@ Arguments:
 * `includesExcludes` - (Optional) A map with 'include' and/or 'exclude' lists containing the names of properties to either include or exclude.
 * `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'.
 
-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.
+If no `include` list is supplied, `bindData` uses the target class default binding allowlist.  In Grails 8 the default remains compatible with earlier releases: statically typed instance properties are bindable unless marked `bindable: false`.  An empty `include` list binds no properties.
+
+Use `include` to allow only the properties needed for a request:
+
+[source,groovy]
+----
+bindData(target, params, [include: ['firstName', 'lastName']])
+----
+
+For controller action command object parameters, use `grails.web.databinding.BindAllowed` to allow request binding for only the listed properties:
+
+[source,groovy]
+----
+import grails.web.databinding.BindAllowed
+
+class PersonController {
+    def save(@BindAllowed(['firstName', 'lastName']) PersonCommand command) {
+        // command.email, command.admin, etc. are not bound by this action
+    }
+}
+----
+
+See the link:{constraintsRefFromRef}bindable.html[bindable] constraint documentation for more information on controlling default bindability.  Applications may set `grails.databinding.denyByDefault = true` to opt in to only binding properties declared with `bindable: true` when no `include` list is supplied.
 
 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.