jamesfredley commented on PR #15947: URL: https://github.com/apache/grails-core/pull/15947#issuecomment-4997457318
## 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/`List`s, 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. -- 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]
