Add opt-in deny-by-default data binding and always honor bindable:false

[jamesfredley]

Description

Grails data binding stays compatible by default and gains an opt-in deny-by-default mode that hardens bindData against mass assignment (OWASP / CWE-915). Unconfigured applications keep binding exactly as they did in prior Grails versions; applications that want mass-assignment protection enable secure mode with a single property.

Independently of the default, an explicit bindable: false constraint is now honored in every mode - including the nested Map-constructor fallback and the explicit bind-all path - so a property a developer marked non-bindable can never be mass-assigned.

The contract

Case	Behavior
Unconfigured app (default)	Permissive - binds properties as before (backward compatible)
grails.databinding.legacyBindableDefault=false	Secure / deny-by-default - only allowlisted properties bind
bindable: false on a property	Never bound, in either mode (explicit developer denial)
App using an allowlist in secure mode	bindable: true (incl. importFrom / shared constraints), explicit include: lists, and @BindAllowed([...]) are all honored
Exclude-only bindData (compatibility mode)	Binds every eligible property except the excludes (no class-allowlist intersection)
Empty include list	Binds nothing, including for direct SimpleDataBinder callers (not restored by the compatibility flag)

What changed

Area	Change
Default	Permissive (compatible) is the unconfigured behavior; grails.databinding.legacyBindableDefault=false opts in to deny-by-default
bindable: false	Always honored in both modes, including the nested Map-constructor fallback and the explicit bind-all path
Allowlist sources (secure mode)	Generated bindable: true allowlist (compile-time and runtime importFrom / shared constraints, unioned), explicit include: lists, and @BindAllowed([...]) on action parameters
Nested enforcement (secure mode)	Allowlist applies through nested associations, collections/Lists, object arrays, indexed properties, typed Map<K,V> values, JSON-shaped nested objects, and the listener fallback
Typed Map values	Converted to the declared value type; beforeBinding can veto before mutation; getter-only maps are updated in place; conversion failures register binding errors
Hot path	Negative include-list results and class-derived unbindable names are cached; instance-derived constraints are not class-cached
Config	Settings.LEGACY_BINDABLE_DEFAULT is the public key; unrecognised values log a warning and fail closed to secure mode
Public API	bind(...) overloads normalize a null include to the resolved allowlist and an empty include to bind-nothing; intentional bind-all uses a private marker
Docs / tests	Permissive default + opt-in secure mode documented; suite verifies the shipping default on the primary bindData surface

Enabling secure (deny-by-default) mode

grails:
  databinding:
    legacyBindableDefault: false

Verification

:grails-web-databinding:test, :grails-databinding-core:test, and focused :grails-test-suite-web:test (including permissive-default, explicit-secure, bindable: false, exclude-only, and bind-all regressions) pass.

Related

Item	Status
#15808 (secureBindData explicit-API approach)	Superseded by this PR; close once this lands
#15950 (nullMissing / stale-data clearing)	Stacked on this PR's binding base

Contributor Checklist

• Compatible by default, with an opt-in deny-by-default mode; bindable: false always honored
• Shipping default covered by bindData regressions (not only secure mode)
• Nested/array/map/inheritance regressions plus permissive/secure/bind-all coverage
• Targets 8.0.x
• Apache License 2.0
• ai-generated-starting-point label applied

Assisted-by: Sisyphus:xai/grok-4.5 [gpt-coding]

[Copilot]

Pull request overview

This PR hardens Grails’ mass data binding to mitigate mass-assignment (CWE-915) by switching to an explicit allowlist model for bindData defaults and controller action auto-binding, while providing a legacy opt-out for migration.

Changes:

• Default binding behavior becomes deny-by-default unless properties are explicitly opted in (primarily via bindable: true allowlists generated at compile time).
• Introduces @BindAllowed([...]) for controller action parameters to provide action-specific binding allowlists.
• Adds a migration switch (grails.databinding.legacyBindableDefault=true), updates documentation, and expands test coverage for the new defaults and edge cases (including empty include).

Reviewed changes

Copilot reviewed 15 out of 15 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
grails-web-databinding/src/main/groovy/org/grails/web/databinding/DefaultASTDatabindingHelper.java	Generates both default and legacy binding allowlist fields during AST injection.
grails-web-databinding/src/main/groovy/grails/web/databinding/DataBindingUtils.java	Enforces deny-by-default include behavior and adds legacy-default config support.
grails-web-databinding/src/main/groovy/grails/web/databinding/DataBinder.groovy	Treats explicit empty include as “bind nothing” to match new semantics.
grails-web-databinding/src/main/groovy/grails/web/databinding/BindAllowed.java	Adds @BindAllowed annotation for controller action parameter allowlists.
grails-test-suite-web/src/test/groovy/org/grails/web/servlet/BindDataMethodTests.groovy	Updates and extends bindData tests for deny-by-default and empty-include behavior.
grails-test-suite-web/src/test/groovy/org/grails/web/commandobjects/SomeValidateableClass.groovy	Updates constraints to explicitly opt properties into binding.
grails-test-suite-web/src/test/groovy/org/grails/web/commandobjects/NonValidateableCommand.groovy	Adds constraints to opt properties into binding under the new default model.
grails-test-suite-web/src/test/groovy/org/grails/web/commandobjects/CommandObjectsSpec.groovy	Adds @BindAllowed coverage and updates command-object constraints for new defaults.
grails-test-suite-web/src/test/groovy/org/grails/web/binding/DefaultASTDatabindingHelperDomainClassSpecialPropertiesSpec.groovy	Expands coverage for default deny behavior and legacy flag behavior in domain inheritance/special properties.
grails-doc/src/en/ref/Controllers/bindData.adoc	Documents new default allowlist behavior, empty-include semantics, and @BindAllowed.
grails-doc/src/en/ref/Constraints/bindable.adoc	Updates bindable semantics to reflect explicit opt-in model and legacy migration option.
grails-doc/src/en/guide/upgrading.adoc	Adds upgrading notes for the new data binding defaults and migration flag.
grails-doc/src/en/guide/theWebLayer/controllers/dataBinding.adoc	Updates guide note for empty include behavior and documents allowlist options.
grails-controllers/src/main/groovy/org/grails/compiler/web/ControllerActionTransformer.java	Threads @BindAllowed allowlists into command object initialization during action parameter binding.
grails-controllers/src/main/groovy/grails/artefact/Controller.groovy	Adds an overload of initializeCommandObject that accepts allowlisted bind properties and applies them during binding.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[bito-code-review]

The issue described regarding Class.getDeclaredField(...) failing for proxied subclasses is a valid concern in the context of Grails data binding. Using Class.getField(...) or walking the class hierarchy is the correct approach to ensure inherited public static whitelist fields are properly resolved for proxied instances. Since the provided PR context does not include the DataBindingUtils.java file, I cannot provide a direct code update for that specific file, but the proposed logic change is technically sound for resolving inherited fields.

[codecov]

Codecov Report

❌ Patch coverage is 39.04236% with 331 lines in your changes missing coverage. Please review.
✅ Project coverage is 52.2724%. Comparing base (8c5b1cf) to head (b89e6a0).
⚠️ Report is 7 commits behind head on 8.0.x.

Files with missing lines	Patch %	Lines
.../grails/web/databinding/GrailsWebDataBinder.groovy	26.4151%	129 Missing and 27 partials ⚠️
...roovy/grails/web/databinding/DataBindingUtils.java	64.5161%	44 Missing and 22 partials ⚠️
...ails/compiler/web/ControllerActionTransformer.java	8.4746%	52 Missing and 2 partials ⚠️
...s/web/databinding/DefaultASTDatabindingHelper.java	28.7879%	42 Missing and 5 partials ⚠️
.../groovy/grails/databinding/SimpleDataBinder.groovy	63.1579%	3 Missing and 4 partials ⚠️
...tabinding/DataBindingEventMulticastListener.groovy	0.0000%	1 Missing ⚠️

Additional details and impacted files

@@                Coverage Diff                 @@
##                8.0.x     #15947        +/-   ##
==================================================
- Coverage     52.3566%   52.2724%   -0.0841%     
- Complexity      18300      18380        +80     
==================================================
  Files            2036       2036                
  Lines           96347      96856       +509     
  Branches        16829      16961       +132     
==================================================
+ Hits            50444      50629       +185     
- Misses          38481      38751       +270     
- Partials         7422       7476        +54     

Files with missing lines	Coverage Δ
.../src/main/groovy/grails/artefact/Controller.groovy	0.0000% <ø> (ø)
...core/src/main/groovy/grails/config/Settings.groovy	100.0000% <ø> (ø)
...tabinding/DataBindingEventMulticastListener.groovy	40.6250% <0.0000%> (-1.3105%)	⬇️
.../groovy/grails/databinding/SimpleDataBinder.groovy	74.6269% <63.1579%> (-0.6309%)	⬇️
...s/web/databinding/DefaultASTDatabindingHelper.java	47.3451% <28.7879%> (-7.7447%)	⬇️
...ails/compiler/web/ControllerActionTransformer.java	61.0672% <8.4746%> (-7.0132%)	⬇️
...roovy/grails/web/databinding/DataBindingUtils.java	56.4815% <64.5161%> (+8.4815%)	⬆️
.../grails/web/databinding/GrailsWebDataBinder.groovy	30.2406% <26.4151%> (-1.6856%)	⬇️

... and 7 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[jamesfredley]

Reworked to deny-by-default (this supersedes the earlier opt-in approach)

Per maintainer direction this PR now makes Grails data binding deny-by-default rather than an opt-in flag. It went through an extensive dual security review; here is the final shape.

The contract

• Unconfigured Grails 8 binds only allowlisted properties. grails.databinding.legacyBindableDefault=true restores the previous permissive behavior for the whole application (the documented breaking-change opt-out).
• Existing correct usage keeps working with no changes. bindable: true constraints (including those resolved at runtime via importFrom / shared constraints), explicit include: lists, and @BindAllowed on action parameters are all honored unchanged.
• Unsafe usage breaks with clear guidance. When deny-by-default drops a request parameter, a one-time warning names the property and class and the exact remedies: declare it bindable: true, add it to the include: list, annotate with @BindAllowed, or set legacyBindableDefault=true.

Security enforcement (the review closed each of these bypass paths, all with regression tests)

• Allowlist applied through every recursive path: nested domain associations, collections/Lists, object arrays (both indexed and non-indexed JSON children: [[...]]), indexed properties, typed Map<K,V> values, JSON-shaped nested objects, and the listener-rejection fallback - so a nested field (e.g. admin) cannot be bound past the child's own allowlist.
• Public bind(...) overloads normalized: a null include resolves the allowlist and an empty include binds nothing; intentional bind-all flows through a private marker (an empty list no longer silently means bind-all).
• Value types without a no-arg constructor fail closed in secure mode instead of mass-assigning through a Map constructor (legacy mode preserves the old behavior).
• Inherited allowlist fields resolve for proxy subclasses, and a generated allowlist is trusted only when both generated fields are co-declared on the same class (mixed-generation upgrade safety).
• Persisted domain array/collection elements resolve by id before binding.

Verification: :grails-databinding-core:test, :grails-web-databinding:test, :grails-test-suite-web:test, and :grails-test-suite-persistence:test all pass.

Follow-up (non-security, documented): the nested-collection branches (array / typed map) currently bind elements before the parent DataBindingListener veto and bypass the property bindProperty setter lifecycle - this is consistent with the pre-existing collection path and is a lifecycle-consistency cleanup for a later PR, not a mass-assignment concern.

Related: #15808 (the earlier secureBindData explicit-API approach) can be closed once this lands; #15950 tracks nullMissing / stale-data clearing separately.

[matrei]

There seems to be some static state leak of legacyBindableDefault.
In HalJsonRendererSpec and JsonRendererSpec the default value true does not seem to be active during data binding.

[jamesfredley]

@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 08bf781.

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:

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 the converted value is returned only when it is Groovy-truthy:

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.

[borinquenkid]

The TestLens failures for GrailsUtilStackFiltererSpec > installed DefaultStackTraceFilterer emits Full Stack Trace by default and GrailsBootstrapRegistryInitializerSpec > defaults logFullStackTraceOnFilter to true on the promoted DefaultStackTraceFilterer are a pre-existing bug on 8.0.x, unrelated to this PR's changes -- reproducible on plain 8.0.x today. DefaultStackTraceFilterer.STACK_LOG routes through a jcl-over-slf4j commons-logging binding, so the tests' System.setErr() capture never observes the emitted message.

Fix: #16067 (replaces the System.err capture with a Logback appender attached directly to the logger). Once that merges, rebasing onto 8.0.x should clear this failure here.

[jdaugherty]

Making this opt-in and keeping bindable: false unconditional is the right shape, and the nested/collection/map enforcement is thorough.

My main concern is that the suite no longer verifies the shipping default. BindDataMethodTests — the primary bindData regression spec — is flipped to legacyBindableDefault=false for every feature via a new setup(), and roughly 25 pre-existing fixture classes across grails-test-suite-web, grails-databinding and grails-test-suite-persistence gained bindable: true on every property.

I reverted the constraint-only fixture edits on this branch and re-ran the affected specs: 25 classes / 151 tests pass unchanged. So the new constraints are not required by the default. But adding them means those fixtures bind identically in both modes, so none of those specs can fail if the compatible path regresses — which is the claim this PR rests on.

Beyond the test coverage, there are a few behavior and cost changes that reach the default path as well. Details inline.

[jamesfredley]

@jdaugherty Addressed in 73a0555 (after merging latest 8.0.x).

Review responses

Item	Resolution
Suite flipped to secure mode	Removed the global setup() that forced legacyBindableDefault=false. Pre-existing bindData regressions run on the unconfigured permissive default again. Secure-mode cases call enableSecureBinding() in their own given:. Added unconfigured counterparts for the main compatibility claims.
Unnecessary bindable: true on fixtures	Dropped the constraint-only fixture edits across grails-test-suite-web, grails-databinding, and grails-test-suite-persistence. Shared fixtures no longer bind identically in both modes.
Cache bindable: false / unbindable names	Class-level ConcurrentHashMap for class-evaluated unbindables. Object-derived constraints are not class-cached (instance maps can differ).
Cache negative include-list results	Cache miss of "no allowlist field" is stored as a sentinel and returned as null on hit.
exclude == null dropped	Restored. Exclude-only in compatibility mode uses the bind-all marker so it does not intersect the class allowlist; secure mode still applies the allowlist when only excludes are supplied. Covered by default-mode tests.
SimpleDataBinder empty whiteList	Documented that empty include binds nothing for direct callers and that the compatibility flag does not restore the old empty-list meaning. Added SimpleDataBinderSpec coverage.
try covers recursive bind	Narrowed to getDeclaredConstructor().newInstance() in both SimpleDataBinder and GrailsWebDataBinder.
Silent fail-closed on bad config	Unrecognised legacyBindableDefault values log a WARN naming the property and value, then fail closed.
Settings.LEGACY_BINDABLE_DEFAULT unused	Single public definition in Settings; internal helper delegates to it; specs use Settings.
LegacyBindableDefaultConfigSpec	Rewritten to drive public bind APIs; clears binding caches and warning state in cleanup.
Typed Map branch	Builds converted entries only after beforeBinding on the source value; mutates the target map in place (getter-only safe); always calls afterBinding; conversion failures go through addBindingError. Upgrade notes + default-mode typed-map test added.

Verification

:grails-databinding-core:test, :grails-web-databinding:test, and focused :grails-test-suite-web:test (BindDataMethodTests, CommandObjectsSpec, DataBindingTests, DefaultASTDatabindingHelperDomainClassSpecialPropertiesSpec, LegacyBindableDefaultConfigSpec, SimpleDataBinderSpec) all green.

[testlens-app]

✅ All tests passed ✅

🏷️ Commit: b89e6a0
▶️ Tests: 58879 executed
⚪️ Checks: 60/60 completed

---

Learn more about TestLens at testlens.app.

[sbglasius]

Used AI to review and it had the following comment:

1. SimpleDataBinder.groovy:273 — isOkToBind(String,...) only checks the . wildcard suffix, but the PR's new whitelist generator also emits _ suffix markers, so underscore/indexed-convention properties get wrongly denied under deny-by-default mode.

2. DefaultASTDatabindingHelper.java:250 — getLegacyPropertyNamesToIncludeInWhiteList duplicates ~80 lines of getPropertyNamesToIncludeInWhiteList instead of sharing the traversal.

3. DataBindingUtils.java:556 — bindObjectToInstance and bindObjectToDomainInstance duplicate the same include-list normalization block, and the former's copy is redundant since it delegates straight into the latter.

But otherwise it looks good to me.