jamesfredley commented on PR #15947: URL: https://github.com/apache/grails-core/pull/15947#issuecomment-5121172170
@matrei Good catch - you were right that the default was not active, and the cause was worse than the specs suggested. Fixed in 4c9062e and 08bf7818. ## What was actually happening It was not leaked state between specs - it reproduced with those two spec classes selected alone and `-PmaxTestParallel=1`. `DataBindingUtils.isLegacyBindableDefaultEnabled()` simply never returned the documented default. A runtime probe on the failing run: ``` [PROBE] branch=FLAT result=false valueClass=org.grails.config.NavigableMap$NullSafeNavigator value=null [PROBE] branch=FLAT result=true valueClass=null value=null ``` When there is no `GrailsApplication` in `Holders`, the value came from `Holders.getFlatConfig()`. For an **absent** key a navigable map answers with a `NavigableMap.NullSafeNavigator` placeholder, not `null` - and it renders as the text `null`. So: ```java value == null || Boolean.TRUE.equals(value) || "true".equalsIgnoreCase(String.valueOf(value)) ``` evaluated to `false` for an absent key, and any application without an explicit setting silently ran in secure deny-by-default mode. That is why the renderer specs only passed once `bindable: true` was added to their model classes - those constraints were masking the defect, so they have been removed and the specs now cover the unconfigured-application path again, as intended. ## A second, more serious defect found while reviewing this The `application != null` branch used `getConfig().getProperty(key, Boolean.class, true)`. In [`NavigableMapConfig.convertValueIfNecessary`](https://github.com/apache/grails-core/blob/8.0.x/grails-core/src/main/groovy/org/grails/config/NavigableMapConfig.java#L343) the converted value is returned only when it is Groovy-truthy: ```java return DefaultGroovyMethods.asBoolean(value) ? value : defaultValue; ``` A configured `false` that arrives as a **string** converts to `Boolean.FALSE`, which is falsy, so the `true` default was handed back instead. An explicit `grails.databinding.legacyBindableDefault=false` supplied through a properties file, a system property or an environment variable was therefore silently ignored and mass assignment stayed enabled. A YAML boolean happened to work only because it took the `targetType.isInstance` short-circuit. ## The fix Both branches now read the **raw** value and share one resolver: | Configured value | Result | | --- | --- | | absent (`null` or the `NullSafeNavigator` placeholder) | permissive (documented default) | | `Boolean` | its own value | | string `true` (trimmed, case-insensitive) | permissive | | anything else, including unparseable input | secure - fails closed | Explicitly-supplied values keep exactly their previous meaning; only the absent-key case changes, plus string-sourced values are now honored in the application branch. Unrecognised input deliberately fails closed rather than throwing, since raising at bind time on a typo'd setting would be a worse regression than choosing the safe mode. ## Coverage New `LegacyBindableDefaultConfigSpec` (19 iterations) pins all of it, including an assertion that the absent-key lookup really does return a `NullSafeNavigator`, application-supplied `false` / `'false'` / `'FALSE'` / `' false '` all selecting secure mode, and unrecognised values failing closed. It was verified to fail against the old expression and pass against the new one. Green: `:grails-web-databinding:test`, `:grails-databinding-core:test`, `:grails-test-suite-web:test`, `:grails-test-suite-persistence:test`, `:grails-rest-transforms:test`, `:grails-controllers:test`, plus `:grails-web-databinding:check` and `:grails-rest-transforms:check`. -- 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]
