jdaugherty commented on code in PR #15947:
URL: https://github.com/apache/grails-core/pull/15947#discussion_r3699902021
##########
grails-test-suite-web/src/test/groovy/org/grails/web/servlet/BindDataMethodTests.groovy:
##########
@@ -28,6 +36,10 @@ import spock.lang.Specification
*/
class BindDataMethodTests extends Specification implements
ControllerUnitTest<BindingController> {
+ void setup() {
+
grailsApplication.config.setAt(DefaultASTDatabindingHelper.LEGACY_BINDABLE_DEFAULT,
false)
Review Comment:
This flips the whole spec into the opt-in secure mode, including the eight
pre-existing `bindData` regressions below it (`Test bindData with Map`, `With
Excludes`, `With Includes`, `Overriding Included With Excluded`, `With Prefix
Filter`, `With Disallowed And GrailsParameterMap`, `With Prefix Filter And
Disallowed`, `Converts Single String In Map To List`).
Those were the regression coverage for the default binding path. After this
change nothing in the file exercises the shipping default except the handful of
specs that explicitly set the flag back to `true`/`null`.
Please leave the pre-existing features running on the unconfigured default
and scope the secure-mode configuration to the new specs — either a separate
spec class, or set the flag in the `given:` block of the tests that need it.
The new secure-mode cases should also have unconfigured-default counterparts,
since "an unconfigured application binds as before" is the compatibility
guarantee this PR makes and it currently has no assertion covering the
`bindData` surface.
##########
grails-databinding-core/src/main/groovy/grails/databinding/SimpleDataBinder.groovy:
##########
@@ -269,7 +271,18 @@ class SimpleDataBinder implements DataBinder {
}
protected boolean isOkToBind(String propName, List whiteList, List
blackList) {
Review Comment:
`SimpleDataBinder` is public API in `grails-databinding-core` and is used
outside the Grails web binding stack.
Changing `!whiteList` to `whiteList == null` flips an empty include list
from "no restriction" to "bind nothing" for every caller of `bind(obj, source,
whiteList)`. Unlike the rest of this PR, that flip is not gated by
`grails.databinding.legacyBindableDefault` — an application that stays on the
compatible default still gets the new behavior, and setting the flag to `true`
does not restore the old one.
The upgrade note says "Empty `include` lists bind no properties" but does
not say it reaches direct `SimpleDataBinder` callers or that the compatibility
flag does not cover it. Please state both, and add a `SimpleDataBinderSpec`
case pinning the new semantics at this level — the current coverage only goes
through `bindData`.
##########
grails-web-databinding/src/main/groovy/grails/web/databinding/DataBindingUtils.java:
##########
@@ -119,30 +126,290 @@ public static BindingResult bindObjectToInstance(Object
object, Object source) {
}
protected static List getBindingIncludeList(final Object object) {
- List includeList = Collections.emptyList();
+ final boolean legacyBindableDefaultEnabled =
isLegacyBindableDefaultEnabled();
+ final Map<Class, List> includeListCache = legacyBindableDefaultEnabled
?
+ CLASS_TO_LEGACY_BINDING_INCLUDE_LIST :
CLASS_TO_BINDING_INCLUDE_LIST;
+ List includeList = null;
try {
final Class<? extends Object> objectClass = object.getClass();
- if (CLASS_TO_BINDING_INCLUDE_LIST.containsKey(objectClass)) {
- includeList = CLASS_TO_BINDING_INCLUDE_LIST.get(objectClass);
+ if (includeListCache.containsKey(objectClass)) {
+ includeList = includeListCache.get(objectClass);
} else {
- final Field whiteListField =
objectClass.getDeclaredField(DefaultASTDatabindingHelper.DEFAULT_DATABINDING_WHITELIST);
- if (whiteListField != null) {
- if ((whiteListField.getModifiers() & Modifier.STATIC) !=
0) {
- final Object whiteListValue =
whiteListField.get(objectClass);
- if (whiteListValue instanceof List) {
- includeList = (List) whiteListValue;
- }
+ // Resolve the runtime-derived bindable names only on a cache
miss - this walks the
+ // target's constraints/metaclass and would otherwise run on
every bind of a cached class.
+ final List runtimeBindableNames = legacyBindableDefaultEnabled
? null : getBindablePropertyNames(object);
+ includeList = runtimeBindableNames;
+ final Field legacyWhiteListField = getField(objectClass,
DefaultASTDatabindingHelper.LEGACY_DATABINDING_WHITELIST);
+ final Field defaultWhiteListField =
legacyBindableDefaultEnabled ?
+ getField(objectClass,
DefaultASTDatabindingHelper.DEFAULT_DATABINDING_WHITELIST) :
+ getPairedField(objectClass,
DefaultASTDatabindingHelper.DEFAULT_DATABINDING_WHITELIST,
+
DefaultASTDatabindingHelper.LEGACY_DATABINDING_WHITELIST);
+ if (legacyBindableDefaultEnabled) {
+ includeList =
getStaticListFieldValue(legacyWhiteListField);
+ if (includeList == null) {
+ includeList =
getStaticListFieldValue(defaultWhiteListField);
+ }
+ } else if (defaultWhiteListField != null) {
+ final List generatedIncludeList =
getStaticListFieldValue(defaultWhiteListField);
+ final Collection combinedIncludeList = new LinkedHashSet();
+ if (generatedIncludeList != null) {
+ combinedIncludeList.addAll(generatedIncludeList);
}
+ if (runtimeBindableNames != null) {
+ combinedIncludeList.addAll(runtimeBindableNames);
+ }
+ includeList = new ArrayList(combinedIncludeList);
+ }
+ if (!legacyBindableDefaultEnabled) {
+ includeList = asGeneratedBindingIncludeList(includeList);
}
- if (!Environment.getCurrent().isReloadEnabled()) {
- CLASS_TO_BINDING_INCLUDE_LIST.put(objectClass,
includeList);
+ if (includeList != null &&
!Environment.getCurrent().isReloadEnabled()) {
+ includeListCache.put(objectClass, includeList);
}
}
} catch (Exception e) {
}
+ if (!legacyBindableDefaultEnabled) {
+ includeList = asGeneratedBindingIncludeList(includeList);
+ }
return includeList;
}
+ static List asGeneratedBindingIncludeList(final List includeList) {
+ if (includeList instanceof GeneratedBindingIncludeList) {
+ return includeList;
+ }
+ final Collection values = includeList == null || includeList.isEmpty()
?
+
Collections.singletonList(DefaultASTDatabindingHelper.NO_BINDABLE_PROPERTIES) :
includeList;
+ return new GeneratedBindingIncludeList(values);
+ }
+
+ static boolean isGeneratedBindingIncludeList(final List includeList) {
+ return includeList instanceof GeneratedBindingIncludeList;
+ }
+
+ private static final class GeneratedBindingIncludeList extends ArrayList {
+ private GeneratedBindingIncludeList(final Collection values) {
+ super(values);
+ }
+ }
+
+ private static Field getField(final Class objectClass, final String
fieldName) {
+ Class currentClass = objectClass;
+ while (currentClass != null) {
+ final Field field = getPublicDeclaredField(currentClass,
fieldName);
+ if (field != null) {
+ return field;
+ }
+ currentClass = currentClass.getSuperclass();
+ }
+ return null;
+ }
+
+ private static Field getPairedField(final Class objectClass, final String
fieldName, final String pairedFieldName) {
+ Class currentClass = objectClass;
+ while (currentClass != null) {
+ final Field field = getPublicDeclaredField(currentClass,
fieldName);
+ final Field pairedField = getPublicDeclaredField(currentClass,
pairedFieldName);
+ if (field != null && pairedField != null) {
+ return field;
+ }
+ currentClass = currentClass.getSuperclass();
+ }
+ return null;
+ }
+
+ private static Field getPublicDeclaredField(final Class objectClass, final
String fieldName) {
+ try {
+ final Field field = objectClass.getDeclaredField(fieldName);
+ return Modifier.isPublic(field.getModifiers()) ? field : null;
+ } catch (NoSuchFieldException ignored) {
+ return null;
+ }
+ }
+
+ private static List getStaticListFieldValue(final Field field) throws
IllegalAccessException {
+ if (field != null && (field.getModifiers() & Modifier.STATIC) != 0) {
+ final Object value = field.get(null);
+ if (value instanceof List) {
+ return (List) value;
+ }
+ }
+ return null;
+ }
+
+ static List getBindablePropertyNames(final Object object) {
+ return getPropertyNamesWithBindableValue(object, Boolean.TRUE);
+ }
+
+ static List getUnbindablePropertyNames(final Object object) {
+ return getPropertyNamesWithBindableValue(object, Boolean.FALSE);
+ }
+
+ static List getUnbindablePropertyNames(final Class objectClass) {
+ return
getPropertyNamesWithBindableValue(evaluateConstrainedProperties(objectClass),
Boolean.FALSE);
+ }
+
+ static List getPropertyNamesWithBindableValue(final Object object, final
Boolean bindableValue) {
+ return
getPropertyNamesWithBindableValue(getConstrainedProperties(object),
bindableValue);
+ }
+
+ private static List getPropertyNamesWithBindableValue(final Map
constrainedProperties, final Boolean bindableValue) {
+ if (constrainedProperties == null || constrainedProperties.isEmpty()) {
+ return Collections.emptyList();
+ }
+ final List propertyNames = new ArrayList();
+ for (Object entryObject : constrainedProperties.entrySet()) {
+ Map.Entry entry = (Map.Entry) entryObject;
+ if
(bindableValue.equals(getBindableConstraintValue(entry.getValue()))) {
+ String propertyName = String.valueOf(entry.getKey());
+ propertyNames.add(propertyName);
+ if (Boolean.TRUE.equals(bindableValue) &&
!isSimpleType(getConstrainedPropertyType(entry.getValue()))) {
+ propertyNames.add(propertyName + "_*");
+ propertyNames.add(propertyName + ".*");
+ }
+ }
+ }
+ return propertyNames;
+ }
+
+ private static Class getConstrainedPropertyType(final Object
constrainedProperty) {
+ MetaClass metaClass =
GroovySystem.getMetaClassRegistry().getMetaClass(constrainedProperty.getClass());
+ try {
+ Object propertyType = metaClass.invokeMethod(constrainedProperty,
"getPropertyType", new Object[0]);
+ if (propertyType instanceof Class) {
+ return (Class) propertyType;
+ }
+ } catch (Exception ignored) {
+ }
+ return null;
+ }
+
+ private static boolean isSimpleType(final Class propertyType) {
+ return propertyType != null && (propertyType.isPrimitive() ||
String.class.equals(propertyType) ||
+ Boolean.class.equals(propertyType) ||
Character.class.equals(propertyType) ||
Number.class.isAssignableFrom(propertyType) ||
+ BigInteger.class.equals(propertyType) ||
BigDecimal.class.equals(propertyType) || URL.class.equals(propertyType));
+ }
+
+ static Map getConstrainedProperties(final Object object) {
+ MetaClass metaClass =
GroovySystem.getMetaClassRegistry().getMetaClass(object.getClass());
+ try {
+ Object constrainedProperties = metaClass.getProperty(object,
"constraintsMap");
+ if (constrainedProperties instanceof Map) {
+ return (Map) constrainedProperties;
+ }
+ } catch (Exception ignored) {
+ }
+ try {
+ Object constrainedProperties = metaClass.invokeMethod(object,
"getConstraintsMap", new Object[0]);
+ if (constrainedProperties instanceof Map) {
+ return (Map) constrainedProperties;
+ }
+ } catch (Exception ignored) {
+ }
+ try {
+ Object constrainedProperties = metaClass.getProperty(object,
"constraints");
+ if (constrainedProperties instanceof Map) {
+ return (Map) constrainedProperties;
+ }
+ } catch (Exception ignored) {
+ }
+ try {
+ Map constrainedProperties =
evaluateConstrainedProperties(object.getClass());
+ if (constrainedProperties != null) {
+ return constrainedProperties;
+ }
+ } catch (Exception ignored) {
+ }
+ return Collections.emptyMap();
+ }
+
+ private static Map evaluateConstrainedProperties(final Class objectClass) {
+ try {
+ Class<?> validationSupport =
Class.forName("org.grails.web.plugins.support.ValidationSupport");
+ Object constrainedProperties =
validationSupport.getMethod("getConstrainedPropertiesForClass", Class.class,
boolean.class).invoke(null, objectClass, false);
+ if (constrainedProperties instanceof Map) {
+ return (Map) constrainedProperties;
+ }
+ } catch (Exception ignored) {
+ }
+ return Collections.emptyMap();
+ }
+
+ static Object getBindableConstraintValue(final Object constrainedProperty)
{
+ MetaClass metaClass =
GroovySystem.getMetaClassRegistry().getMetaClass(constrainedProperty.getClass());
+ try {
+ Object value = metaClass.invokeMethod(constrainedProperty,
"getMetaConstraintValue", new Object[] {
DefaultASTDatabindingHelper.BINDABLE_CONSTRAINT_NAME });
+ if (value != null) {
+ return value;
+ }
+ } catch (Exception ignored) {
+ }
+ try {
+ Object metaConstraints =
metaClass.getProperty(constrainedProperty, "metaConstraints");
+ if (metaConstraints instanceof Map) {
+ return ((Map)
metaConstraints).get(DefaultASTDatabindingHelper.BINDABLE_CONSTRAINT_NAME);
+ }
+ } catch (Exception ignored) {
+ }
+ try {
+ Object delegate = metaClass.getProperty(constrainedProperty,
"property");
+ if (delegate != null && delegate != constrainedProperty) {
+ return getBindableConstraintValue(delegate);
+ }
+ } catch (Exception ignored) {
+ }
+ return null;
+ }
+
+ static List addUnbindablePropertyNames(final Object object, final List
exclude) {
+ final List unbindablePropertyNames =
getUnbindablePropertyNames(object);
+ if (unbindablePropertyNames.isEmpty()) {
+ return exclude;
+ }
+ if (exclude == null || exclude.isEmpty()) {
+ return unbindablePropertyNames;
+ }
+ final List combinedExcludes = new ArrayList(exclude);
+ combinedExcludes.addAll(unbindablePropertyNames);
+ return combinedExcludes;
+ }
+
+ static boolean isLegacyBindableDefaultEnabled() {
+ GrailsApplication application = Holders.findApplication();
+ if (application != null) {
+ return resolveLegacyBindableDefault(
+
application.getConfig().getProperty(DefaultASTDatabindingHelper.LEGACY_BINDABLE_DEFAULT,
Object.class, null));
+ }
+ return
resolveLegacyBindableDefault(Holders.getFlatConfig().get(DefaultASTDatabindingHelper.LEGACY_BINDABLE_DEFAULT));
+ }
+
+ /**
+ * Resolves the configured value of {@code
grails.databinding.legacyBindableDefault} against the
+ * permissive default.
+ * <p>
+ * The raw value must be resolved here rather than through a typed {@code
Boolean} config lookup:
+ * a config value that converts to {@code Boolean.FALSE} is discarded in
favour of the supplied
+ * default, which would silently ignore an explicit opt-in to the secure
deny-by-default mode from
+ * any string-valued source such as a properties file, a system property
or an environment variable.
+ * <p>
+ * A navigable config answers an absent key with a placeholder object
rather than {@code null}, so
+ * only a genuinely absent key may fall back to the permissive default.
Any other unrecognised value
+ * fails closed, because this switch governs mass-assignment protection.
+ *
+ * @param value the raw configured value, which may be {@code null} or an
absent-key placeholder
+ * @return true when the legacy (permissive) binding default applies
+ */
+ static boolean resolveLegacyBindableDefault(final Object value) {
Review Comment:
Failing closed on a value that cannot be parsed is defensible, but it fails
closed silently.
`LegacyBindableDefaultConfigSpec` pins `''`, `0`, `1`, `'off'` and `'no'` as
switching the application into deny-by-default binding; by the same rule so do
`'yes'` and `'on'`, which a reader would expect to mean *true*. The only
feedback is a per-property WARN, emitted after requests have already started
dropping fields.
`spring-configuration-metadata.json` declares the property as
`java.lang.Boolean`, which is what IDE completion shows and what Spring's
relaxed binding accepts, so `on`/`yes` are plausible things for someone to
write. Please log a warning naming the property and the unrecognised value when
the raw value does not resolve, so a typo is visible immediately rather than
inferred from missing data.
##########
grails-web-databinding/src/main/groovy/grails/web/databinding/DataBindingUtils.java:
##########
@@ -119,30 +126,290 @@ public static BindingResult bindObjectToInstance(Object
object, Object source) {
}
protected static List getBindingIncludeList(final Object object) {
- List includeList = Collections.emptyList();
+ final boolean legacyBindableDefaultEnabled =
isLegacyBindableDefaultEnabled();
+ final Map<Class, List> includeListCache = legacyBindableDefaultEnabled
?
+ CLASS_TO_LEGACY_BINDING_INCLUDE_LIST :
CLASS_TO_BINDING_INCLUDE_LIST;
+ List includeList = null;
try {
final Class<? extends Object> objectClass = object.getClass();
- if (CLASS_TO_BINDING_INCLUDE_LIST.containsKey(objectClass)) {
- includeList = CLASS_TO_BINDING_INCLUDE_LIST.get(objectClass);
+ if (includeListCache.containsKey(objectClass)) {
+ includeList = includeListCache.get(objectClass);
} else {
- final Field whiteListField =
objectClass.getDeclaredField(DefaultASTDatabindingHelper.DEFAULT_DATABINDING_WHITELIST);
- if (whiteListField != null) {
- if ((whiteListField.getModifiers() & Modifier.STATIC) !=
0) {
- final Object whiteListValue =
whiteListField.get(objectClass);
- if (whiteListValue instanceof List) {
- includeList = (List) whiteListValue;
- }
+ // Resolve the runtime-derived bindable names only on a cache
miss - this walks the
+ // target's constraints/metaclass and would otherwise run on
every bind of a cached class.
+ final List runtimeBindableNames = legacyBindableDefaultEnabled
? null : getBindablePropertyNames(object);
+ includeList = runtimeBindableNames;
+ final Field legacyWhiteListField = getField(objectClass,
DefaultASTDatabindingHelper.LEGACY_DATABINDING_WHITELIST);
+ final Field defaultWhiteListField =
legacyBindableDefaultEnabled ?
+ getField(objectClass,
DefaultASTDatabindingHelper.DEFAULT_DATABINDING_WHITELIST) :
+ getPairedField(objectClass,
DefaultASTDatabindingHelper.DEFAULT_DATABINDING_WHITELIST,
+
DefaultASTDatabindingHelper.LEGACY_DATABINDING_WHITELIST);
+ if (legacyBindableDefaultEnabled) {
+ includeList =
getStaticListFieldValue(legacyWhiteListField);
+ if (includeList == null) {
+ includeList =
getStaticListFieldValue(defaultWhiteListField);
+ }
+ } else if (defaultWhiteListField != null) {
+ final List generatedIncludeList =
getStaticListFieldValue(defaultWhiteListField);
+ final Collection combinedIncludeList = new LinkedHashSet();
+ if (generatedIncludeList != null) {
+ combinedIncludeList.addAll(generatedIncludeList);
}
+ if (runtimeBindableNames != null) {
+ combinedIncludeList.addAll(runtimeBindableNames);
+ }
+ includeList = new ArrayList(combinedIncludeList);
+ }
+ if (!legacyBindableDefaultEnabled) {
+ includeList = asGeneratedBindingIncludeList(includeList);
}
- if (!Environment.getCurrent().isReloadEnabled()) {
- CLASS_TO_BINDING_INCLUDE_LIST.put(objectClass,
includeList);
+ if (includeList != null &&
!Environment.getCurrent().isReloadEnabled()) {
Review Comment:
Guarding the cache write on `includeList != null` means the "this class has
no allowlist field" result is never cached.
In the default mode that is the outcome for every target the AST helper did
not enhance, so each bind re-runs `getField` twice over the whole superclass
chain, and `getPublicDeclaredField` throws and swallows a
`NoSuchFieldException` for each class/field pair that misses. The previous
implementation cached `Collections.emptyList()` and did the reflection once per
class.
Please cache the negative result as well — either a sentinel value, or keep
returning an empty list for "nothing found" and reserve `null` for a genuine
resolution failure.
##########
grails-test-suite-web/src/test/groovy/org/grails/web/binding/DataBindingTests.groovy:
##########
@@ -454,6 +454,11 @@ class DataBindingTests extends Specification implements
ControllerUnitTest<TestC
class Clown {
String name
String hairColour
+
+ static constraints = {
+ name bindable: true
Review Comment:
These `bindable: true` additions are not required by the new default.
I reverted the constraint-only fixture edits on this branch —
`DataBindingTests`, `JSONBindingToNullSpec`, `BindCommandObjectsSpec`,
`BindToObjectWithEmbeddableTests`, `GrailsParameterMapBindingSpec`,
`BindingToNullableTests`, `DirtyCheckBaseBindDataSpec`,
`ControllerExceptionHandlerSpec`, `JSONConverterTests`, `RespondMethodSpec`,
`SomeValidateableClass`, the `org.grails.web.binding.*` / `commandobjects` /
`json` / `mime` specs, and `DataBindingConfigurationSpec` — and re-ran them: 25
classes, 151 tests, all green.
Please drop them. As written, every one of these fixtures binds identically
with the flag on or off, so the specs can no longer fail if the compatible path
regresses. Where a fixture genuinely needs an allowlist to demonstrate secure
mode, it belongs in a new spec rather than in a shared fixture that other tests
rely on for default-mode behavior.
##########
grails-core/src/main/groovy/grails/config/Settings.groovy:
##########
@@ -392,6 +392,8 @@ interface Settings {
String DATE_LENIENT_PARSING = 'grails.databinding.dateParsingLenient'
+ String LEGACY_BINDABLE_DEFAULT = 'grails.databinding.legacyBindableDefault'
Review Comment:
This constant is added but never referenced. `DataBindingUtils`,
`GrailsWebDataBinder`, `BindDataMethodTests` and
`LegacyBindableDefaultConfigSpec` all use
`DefaultASTDatabindingHelper.LEGACY_BINDABLE_DEFAULT`, which lives in an
internal `org.grails.web.databinding` class.
Please keep the single public definition here and have the internal code and
the specs reference it, so the key has one definition and applications have a
supported constant to use.
##########
grails-web-databinding/src/main/groovy/grails/web/databinding/GrailsWebDataBinder.groovy:
##########
@@ -397,6 +556,39 @@ class GrailsWebDataBinder extends SimpleDataBinder {
}
}
}
+ } else if (Map.isAssignableFrom(metaProperty.type) && val
instanceof Map) {
+ def referencedType = getReferencedTypeForCollection(propName,
obj)
+ if (referencedType != null) {
+ needsBinding = false
+ Map map = initializeMap(obj, propName)
+ map.clear()
Review Comment:
This branch is new behavior for any `Map`-typed property with a resolvable
value type: previously the raw source map was assigned through
`bindProperty`/`setPropertyValue`, and now entries are converted or recursively
bound. Two follow-ups:
- It bypasses `bindProperty`, so `DataBindingListener` callbacks and error
registration do not fire for this property. The object-array branch a few lines
above does call `bindProperty`. A conversion failure inside this loop is
invisible to listeners and to the `BindingResult`.
- Typed-map value conversion is a user-visible change independent of the
mass-assignment work, so it needs an upgrade-note line and a default-mode test.
`Test bindData in explicit secure mode applies the nested target allowlist`
only covers it with the flag on.
##########
grails-web-databinding/src/main/groovy/grails/web/databinding/GrailsWebDataBinder.groovy:
##########
@@ -162,14 +209,77 @@ class GrailsWebDataBinder extends SimpleDataBinder {
boolean bind = listenerWrapper.beforeBinding(object, bindingResult)
if (bind) {
- super.doBind(object, source, filter, whiteList, blackList,
listenerWrapper, bindingResult)
+ List previousIncludeList = bindingIncludeList.get()
+ Class<?> previousTargetType = bindingTargetType.get()
+ bindingIncludeList.set(whiteList)
+ bindingTargetType.set(object.getClass())
+ try {
+ super.doBind(object, source, filter, whiteList,
addUnbindablePropertyNames(object, blackList), listenerWrapper, bindingResult)
Review Comment:
`addUnbindablePropertyNames` runs on every bind, in both modes, and nothing
about it is cached.
It calls `getConstrainedProperties(object)`, which tries up to four
strategies in order — the `constraintsMap` property, `getConstraintsMap()`, the
`constraints` property, then
`Class.forName("org.grails.web.plugins.support.ValidationSupport")` plus a
reflective `getConstrainedPropertiesForClass` — each wrapped in `catch
(Exception ignored)`. For a target with no constraints, which is the common
case for plain command objects, that is three thrown-and-discarded exceptions
plus a reflective class load per bind. `getBindableConstraintValue` then
repeats the same three-fallback pattern for every constrained property.
`bindable: false` is a per-class, compile-time-known fact. Please resolve it
once per class into a `ConcurrentHashMap`, the way `getBindingIncludeList`
already caches its result, so request binding does not pay this on the hot path.
##########
grails-databinding-core/src/main/groovy/grails/databinding/SimpleDataBinder.groovy:
##########
@@ -394,6 +415,16 @@ class SimpleDataBinder implements DataBinder {
}
}
+ protected Object instantiateAndBindOrUseMapConstructor(Class
referencedType, Map values, DataBindingListener listener) {
Review Comment:
The `try` covers the recursive `bind(...)` call as well as the instantiation.
A `NoSuchMethodException` or `IllegalAccessException` raised anywhere inside
nested binding is therefore caught here and retried through the `Map`
constructor, binding the same source twice into two different instances and
returning the second.
`GrailsWebDataBinder.instantiateAndBindNestedOrUseMapConstructor` has the same
shape; there the consequence in secure mode is that the element is dropped and
the original cause is lost entirely.
Please narrow the guarded region to
`referencedType.getDeclaredConstructor().newInstance()` and move the `bind`
call outside the catch.
##########
grails-web-databinding/src/main/groovy/grails/web/databinding/DataBindingUtils.java:
##########
@@ -212,9 +479,12 @@ public static <T> void bindToCollection(final Class<T>
targetType, final Collect
* @return A BindingResult if there were errors or null if it was
successful
*/
public static BindingResult bindObjectToInstance(Object object, Object
source, List include, List exclude, String filter) {
- if (include == null && exclude == null) {
+ if (include == null) {
Review Comment:
Dropping `&& exclude == null` changes the default mode too, not only secure
mode.
`bindData(target, params, [exclude: ['email']])` — and the `bindData(target,
bindingSource, List excludes)` overload that delegates to it — previously bound
every property except the excludes, with no allowlist applied. It now
intersects with the class allowlist as well, so anything absent from the
generated list silently stops binding: dynamically typed properties,
`id`/`version` on a domain class, and properties the AST helper did not record.
`bindable: false` is already enforced independently by
`addUnbindablePropertyNames` in `GrailsWebDataBinder.doBind`, so this line is
not what closes that gap. If the narrowing is intentional it needs an entry in
the upgrade notes and a default-mode test; otherwise please restore the
`exclude == null` condition. Note that `Test bindData With Excludes`, which
would have caught this, now runs in secure mode.
##########
grails-web-databinding/src/test/groovy/grails/web/databinding/LegacyBindableDefaultConfigSpec.groovy:
##########
@@ -0,0 +1,168 @@
+/*
+ * 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 grails.web.databinding
+
+import grails.config.Config
+import grails.core.GrailsApplication
+import grails.util.Holders
+import org.grails.config.NavigableMap
+import org.grails.config.PropertySourcesConfig
+import org.grails.web.databinding.DefaultASTDatabindingHelper
+import spock.lang.Specification
+import spock.lang.Unroll
+
+class LegacyBindableDefaultConfigSpec extends Specification {
+
+ private Config originalConfig
+ private GrailsApplication originalApplication
+
+ void setup() {
+ originalConfig = Holders.config
+ originalApplication = Holders.findApplication()
+ Holders.grailsApplication = null
+ Holders.setConfig(null)
+ }
+
+ void cleanup() {
+ Holders.setConfig(originalConfig)
+ Holders.grailsApplication = originalApplication
+ }
+
+ void 'an absent legacy bindable default in a PropertySourcesConfig remains
permissive'() {
+ given:
+ Holders.setConfig(new PropertySourcesConfig([:]))
+
+ when:
+ Object value =
Holders.flatConfig.get(DefaultASTDatabindingHelper.LEGACY_BINDABLE_DEFAULT)
+ boolean legacyDefaultEnabled =
DataBindingUtils.isLegacyBindableDefaultEnabled()
Review Comment:
`isLegacyBindableDefaultEnabled()` is package-private internal API, and this
spec sits in `grails.web.databinding` only in order to reach it. Per the
repository's testing rule, specs should drive behavior through the API an
application actually calls.
Every case here can be written as "given this config, bind this source,
assert which properties were set", which covers the resolution *and* that it is
wired up correctly. As it stands, a change that resolved the flag correctly but
applied it to the wrong code path would still pass this spec.
Separately: `setup()` nulls `Holders.grailsApplication` and `Holders.config`
process-wide. `cleanup()` restores them, but together with the static caches in
`DataBindingUtils` and `GrailsWebDataBinder.WARNED_BINDING_SHAPES` — neither of
which is reset — this leaves mode-dependent state that later specs in the same
fork inherit.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
