Validate dynamic finder and HQL list sort property names - #16333
Validate dynamic finder and HQL list sort property names#16333jamesfredley wants to merge 6 commits into
Conversation
Sort keys from list(sort:) and Hibernate HQL list queries were interpolated without checking that they were identifier-shaped persistent properties. Reject injected or unknown sort names, and only allow asc or desc for HQL sort direction.
Fixes the regression and the gaps found in review of the dynamic finder and HQL list sort validation: - Sorting by a criteria or where-query alias regressed: the validator rejected any first path segment that is not a persistent property, which is exactly what an alias is, so WhereQueryWithAssociationSortSpec failed in both Hibernate modules. A first segment that is not a persistent property is now accepted on the shape check alone and left to the query implementation; segments beneath a known property must still resolve through the mapping. - fetch keys in HqlListQueryBuilder were still concatenated into HQL unchecked and are now validated the same way as sort keys. - Composite identities are recognised explicitly instead of relying on the by-name property map, and the identity fallback no longer depends on the segment position. - validateSortProperty is private, the duplicated pattern is replaced by a shared NameUtils.isValidPropertyPath that follows Java identifier rules ($ and non-ASCII letters), and messages no longer echo the untrusted value or distinguish malformed from unknown names. - Blank sort properties are rejected instead of producing an empty order-by part, and sort directions are trimmed before checking. - The BuildableCriteria overload takes each sort map entry's direction from its value with the same asc fallback as applySortForMap, and the entity resolution helper is reused for the identity default. SortArgumentValidationSpec exercises every entry point against real Hibernate mappings: identity and version, inherited, embedded and association paths, composite identities, the mapping default sort, alias roots, map directions, and the rejected sort, order and fetch values. The datamapping-core, NameUtils and mock builder specs cover the remaining branches. The list() reference and finder docs describe the accepted values.
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Hardens GORM sorting by validating caller-supplied sort property paths (and Hibernate list sort directions) to prevent query/HQL order-by interpolation with non-property tokens.
Changes:
- Added
NameUtils.isValidPropertyPathplus unit tests to validate identifier-shaped dotted property paths. - Updated
DynamicFinderto validate sort keys (including traversing mapped associations/identities when mapping metadata is available). - Updated Hibernate
HqlListQueryBuilderto require mapped sort/fetch properties and to normalize/validate sort direction; added/expanded specs and documentation.
Reviewed changes
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| grails-doc/src/en/ref/Domain Classes/list.adoc | Documents new validation rules for sort/order/fetch in list() arguments. |
| grails-datastore-core/src/main/groovy/org/grails/datastore/mapping/reflect/NameUtils.java | Adds isValidPropertyPath for identifier-shaped dotted property paths. |
| grails-datastore-core/src/test/groovy/org/grails/datastore/mapping/reflect/NameUtilsSpec.groovy | Adds coverage for valid/invalid property-path shapes. |
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/finders/DynamicFinder.java | Validates sort keys against shape + mapping traversal (assoc/identity handling) and centralizes order building. |
| grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/finders/DynamicFinderCoverageSpec.groovy | Expands coverage for sort acceptance/rejection (including mapping traversal and composite ids). |
| grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/query/HqlListQueryBuilder.java | Requires mapped sort/fetch properties and strictly normalizes/validates order to asc/desc. |
| grails-data-hibernate7/core/src/test/groovy/org/grails/orm/hibernate/query/HqlListQueryBuilderSpec.groovy | Adds tests for rejecting injected sort keys/directions and invalid fetch keys. |
| grails-data-hibernate7/core/src/test/groovy/grails/gorm/tests/SortArgumentValidationSpec.groovy | New integration-style spec exercising validation against real Hibernate mappings across entry points. |
| grails-data-hibernate7/docs/src/docs/asciidoc/querying/finders.adoc | Documents sort-path validation and alias behavior for Hibernate 7 finders. |
| grails-data-hibernate5/docs/src/docs/asciidoc/querying/finders.adoc | Documents sort-path validation and alias behavior for Hibernate 5 finders. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 8.0.x #16333 +/- ##
==================================================
+ Coverage 55.0810% 55.1275% +0.0465%
- Complexity 20804 20842 +38
==================================================
Files 2111 2111
Lines 101378 101443 +65
Branches 18005 18016 +11
==================================================
+ Hits 55840 55923 +83
+ Misses 37490 37478 -12
+ Partials 8048 8042 -6
🚀 New features to boost your workflow:
|
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Copilot reviewed 10 out of 10 changed files in this pull request and generated 3 comments.
Suppressed comments (1)
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/query/HqlListQueryBuilder.java:92
- Casting
sorttoMap<String, String>can throwClassCastExceptionat runtime when callers supply a map with non-Stringvalues (common with request params or GroovyGStringvalues). Treat the map asMap<?, ?>(orMap<String, ?>) and convertprop/directionviatoString()(with null-handling) before callingbuildSortPartso invalid input fails with the intendedIllegalArgumentExceptioninstead of a cast error.
} else if (sort instanceof Map) {
List<String> parts = new ArrayList<>();
((Map<String, String>) sort).forEach((prop, direction) -> {
parts.add(buildSortPart(prop, direction, isIgnoreCase));
});
| Map<String, Object> fetchMap = (Map<String, Object>) fetchObj; | ||
| fetchMap.forEach((prop, type) -> { | ||
| if (HibernateQueryArgument.JOIN.value().equals(type) || HibernateQueryArgument.EAGER.value().equals(type)) { | ||
| requireMappedProperty(prop, HibernateQueryArgument.FETCH.value()); | ||
| hql.append(" join fetch e.").append(prop); | ||
| } | ||
| }); |
DynamicFinder.buildOrder treated any order other than exact
equalsIgnoreCase("desc") as ascending, so values like " DESC "
silently sorted ascending. Trim whitespace, accept asc/desc
case-insensitively, and reject anything else with
IllegalArgumentException("Invalid sort direction"), matching
HqlListQueryBuilder.normalizeDirection.
Assisted-by: opencode:xai/grok-4.6
matrei
left a comment
matrei
left a comment
There was a problem hiding this comment.
Review Findings (round 2)
Head 42e951fc1c on 8.0.x base 0980623481. This is the same-repo continuation of #16312 and carries the earlier follow-up (035f9197) plus the direction fix 91885f07ed from the Copilot thread. The branch is at the tip of 8.0.x and git merge-tree is clean. This round is a status check of the round-1 findings on #16312 plus a fresh pass over the entry points the direction change is meant to cover.
Re-ran on this head with --continue, including codeStyle on grails-datastore-core, grails-datamapping-core and grails-data-hibernate7-core: no violations.
| Module | Tests | Failures | Skipped |
|---|---|---|---|
grails-datastore-core (NameUtilsSpec) |
36 | 0 | 0 |
| grails-datamapping-core | 962 | 0 | 1 |
| grails-data-hibernate7-core | 3132 | 0 | 29 |
| grails-data-hibernate5-core | 838 | 0 | 39 |
The TestLens failure on CI is UserControllerSpec > User list in the scaffolding example, tracked as flaky in #16030 and unrelated to these files.
Round-1 status
| Round-1 finding | Status |
|---|---|
| Direction trimming differs between entry points | Fixed in 91885f07ed. DynamicFinder.normalizeDirection now mirrors the HQL builder, and the behaviour is pinned for list(), dynamic finders and where queries in DynamicFinderCoverageSpec and SortArgumentValidationSpec. Note that the fix goes further than the nit asked for: a direction other than asc/desc now throws IllegalArgumentException on finders and where queries where it used to sort ascending silently. That is the right call, but it is a user-visible change and belongs in the PR description. |
| Alias pass-through is wider than it needs to be | Not addressed and not answered. A single-segment sort key that is not a property still reaches Hibernate and fails with the raw NPE rather than Invalid sort property, see below. Still non-blocking, but it deserves an explicit yes or no. |
fetch wording in list.adoc is Hibernate 7 specific |
Not addressed. Fine for the 8.0.x guide, but the scope is wider than fetch: Hibernate 5 list() goes through GrailsHibernateQueryUtils.populateArgumentsForCriteria (grails-data-hibernate5/core/.../query/GrailsHibernateQueryUtils.java:128-141), which validates neither sort names nor direction, so the whole new paragraph in list.adoc describes Hibernate 7 only. |
| PR description | Still describes the original behaviour ("keys that do not resolve to a persistent property" are rejected). It should mention the first-segment pass-through for aliases, the sort-map direction change, and the new direction rejection. |
Confirmed on this head, with a throwaway spec against the real Hibernate 7 mapping (SavClub), so the figures below are observed, not inferred:
findAllByNameLike('%', [sort: 'notAProperty']) NPE: Expression.getJavaType() because "expression" is null
where { name != null }.list(sort: 'notAProperty') NPE (same)
createCriteria().list(sort: 'notAProperty') { } NPE (same)
Findings on this head
None of these block the merge. Every one is pre-existing behaviour on an entry point the PR did not touch, but two of them contradict what the Copilot thread was closed with ("matching Hibernate normalization" across entry points), so they should either be folded in, which is small, or filed as a follow-up and referenced from the PR.
Low: the criteria builder is the one entry point left out
References:
grails-data-hibernate7/core/src/main/groovy/grails/orm/CriteriaMethodInvoker.java:118-133grails-data-hibernate5/core/src/main/groovy/org/grails/orm/hibernate/query/AbstractHibernateCriteriaBuilder.java:1759-1768(same shape)
createCriteria().list(sort:, order:) { } does not go through DynamicFinder at all. CriteriaMethodInvoker reads sort and order from the argument map and builds Query.Order directly, so neither validateSortProperty nor normalizeDirection runs:
createCriteria().list(sort: 'name', order: 'sideways') { }*.name == [Arsenal, United] (silently asc)
createCriteria().list(sort: 'name', order: ' DESC ') { }*.name == [Arsenal, United] (silently asc)
createCriteria().list(sort: 'name, e.id') { } NPE from Hibernate
Same result with max: set (the HibernatePagedResultList branch). This is not an injection sink, the value ends up in the JPA Criteria API, but it is exactly the inconsistency Copilot described as "criteria paths silently coerce", and the reply that closed that thread only covers DynamicFinder. SortArgumentValidationSpec exercises criteria only for the alias happy path (line 95), so nothing pins either outcome.
Suggested fix: make normalizeDirection and validateSortProperty reachable (a small public static helper on DynamicFinder, or a new class next to NameUtils) and call them from CriteriaMethodInvoker before constructing the Query.Order, then add criteria rows to the rejection cases in SortArgumentValidationSpec.
Low: listOrderBy* still coerces the direction
References:
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/finders/ListOrderByFinder.java:89-90
listOrderByName(order: 'sideways')*.name == [Arsenal, United] (silently asc)
listOrderByName(order: ' DESC ')*.name == [Arsenal, United] (silently asc)
The property name comes from the method name, so no injection concern, only the direction inconsistency. listOrderBy.adoc documents order, so the same expectation applies. One line with the shared helper.
Nit: order on Hibernate 7 list() is only validated when sort is present
References:
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/query/HqlListQueryBuilder.java:80(buildSortClause)grails-doc/src/en/ref/Domain Classes/list.adoc
normalizeDirection is only reached from buildSortPart, so SavClub.list(order: 'sideways') returns rows in insertion order without an error, and SavClub.list(order: 'desc') is ignored too. A dynamic finder with order alone sorts by the identity and rejects sideways. Pre-existing, but the new sentence in list.adoc ("order must be asc or desc ... any other value is rejected") over-promises for that case. Either normalize order at the top of buildSortClause regardless of sort, or qualify the sentence.
Nit: two identical normalizeDirection copies
References:
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/finders/DynamicFinder.java:843grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/query/HqlListQueryBuilder.java:150
Byte-for-byte the same private method in both modules. If the criteria and listOrderBy items above are taken, one shared helper resolves all three call sites and removes the copy.
Nit: coverage gaps in DynamicFinder
Local Jacoco for grails-datamapping-core shows the following new or touched lines not executed by any spec in the module, which lines up with the 13 missing lines Codecov reports:
validateSortPropertyentity == nullearly return (lines 789-790)resolvePersistentEntitydetached-criteria andnullbranches (lines 763-766)- composite-identity default sort in the
BuildableCriteriaoverload (lines 457-463) - composite-identity lookup in
resolveProperty(lines 820-827, covered only from the Hibernate 7 module)
Not blocking, the Hibernate 7 spec covers the composite case end to end. A DynamicFinderCoverageSpec row with a composite-identity entity would close most of it.
Verdict
Approve. The sort-name validation and the direction fix are correct, tested against real mappings, and green across all four modules. Before merge: update the PR description as noted above, and decide whether the criteria-builder and listOrderBy direction handling go into this PR (small, and it would make the "consistent across entry points" claim true) or into a referenced follow-up.
|
@matrei other than updating the PR description, I believe everything is done. I did narrow the path after review since hibernate 7 would error anyhow. |
✅ All tests passed ✅🏷️ Commit: a7647b2 Learn more about TestLens at testlens.app/docs. |
matrei
left a comment
matrei
left a comment
There was a problem hiding this comment.
Review Findings (round 3)
Head a7647b2731 on 8.0.x base e9cd00567b. New since round 2 is 9cd088f435 ("Validate sort arguments on every query entry point") plus a merge of 8.0.x. The branch is at the tip of 8.0.x.
Re-ran locally on this head with --continue, including codeStyle on all four modules: no violations. Also ran the changed TCK spec against Neo4j (Docker), since grails-data-neo4j-core consumes grails-datamapping-tck and the ListOrderByFinder change is shared by every datastore.
| Module | Tests | Failures | Skipped |
|---|---|---|---|
| grails-datastore-core | 202 | 0 | 0 |
| grails-datamapping-core | 968 | 0 | 1 |
| grails-data-hibernate5-core | 854 | 0 | 39 |
| grails-data-hibernate7-core | 3149 | 0 | 29 |
grails-data-neo4j-core (ListOrderBySpec) |
2 | 0 | 0 |
TestLens on CI is green for this head (73418 tests, 89/89 checks).
Round-2 status
| Round-2 finding | Status |
|---|---|
| Criteria builder was the one entry point left out | Fixed. Hibernate 7 CriteriaMethodInvoker and Hibernate 5 AbstractHibernateCriteriaBuilder now call validateSortProperty and normalizeDirection before building the order; pinned for paged and unpaged createCriteria().list() in both SortArgumentValidationSpecs and in CriteriaMethodInvokerSpec. |
listOrderBy* still coerced the direction |
Fixed in ListOrderByFinder; pinned in AbstractFinderSpec, both Hibernate specs and the TCK ListOrderBySpec (which also passes on Neo4j). |
order on Hibernate 7 list() only validated when sort present |
Fixed, and applied consistently: HqlListQueryBuilder, the three Hibernate 5 populateArgumentsForCriteria overloads and CriteriaMethodInvoker all normalize order before looking at sort. Pinned with "an order argument is validated even when there is no sort key" on both Hibernate versions. |
Two identical normalizeDirection copies |
Fixed. The HqlListQueryBuilder copy is gone; DynamicFinder.normalizeDirection and validateSortProperty are the single public helpers behind every path. |
| Alias pass-through wider than needed | Addressed: a bare name that is not a persistent property is now rejected, only a dotted key gets the alias pass-through. Pinned for list(), finders, where queries and criteria on both Hibernate versions and in DynamicFinderCoverageSpec. |
fetch wording in list.adoc was Hibernate 7 specific |
Half addressed. The new upgrade-guide section correctly scopes fetch to Hibernate 7, but list.adoc:71 still states unconditionally that "a fetch key must name a persistent property". Nit, see below. |
| PR description | Still the original text. jdaugherty confirmed on the PR that this is the one remaining item. See below for what it needs to say. |
Coverage gaps in DynamicFinder |
Partly closed. The entity == null early return, the resolvePersistentEntity fallback and the bare-name rejection are now covered in-module. The composite-identity branches (DynamicFinder.java:457-463 and 831-837) are still only executed from the Hibernate 7 module. Not blocking. |
Hibernate 5 coverage is new in this round and was not on the round-2 list: both populateArgumentsForCriteria overloads in GrailsHibernateQueryUtils and the one in GrailsHibernateUtil now validate sort names and directions, with a dedicated SortArgumentValidationSpec against real Hibernate 5 mappings. Good.
Findings on this head
None block the merge.
Must do before merge: PR description
The description still describes the first iteration only. It should state, since all of these are user-visible changes:
orderother thanasc/desc(trimmed, case-insensitive) now throwsIllegalArgumentException("Invalid sort direction")on every entry point, includinglistOrderBy*and criterialist(), where it used to sort ascending silently. It is checked even when nosortis given.- A bare
sortname that is not a persistent property is rejected withInvalid sort property; a dotted key whose first segment is not a property is still passed through for aliases. - Hibernate 5 is covered as well as Hibernate 7.
- Link to the new upgrade-guide section (58) so the behaviour change is discoverable.
Low: the criteria DSL order(property, direction) still coerces
References:
grails-data-hibernate7/core/src/main/groovy/grails/orm/HibernateCriteriaBuilder.java:1040-1045grails-data-hibernate5/core/src/main/groovy/org/grails/orm/hibernate/query/AbstractHibernateCriteriaBuilder.java:1469grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/query/criteria/AbstractCriteriaBuilder.java:1046-1052grails-datamapping-core/src/main/groovy/grails/gorm/DetachedCriteria.groovy:242grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/query/criteria/AbstractDetachedCriteria.groovy:342
These are the last equalsIgnoreCase("desc")-style coercions on a caller-facing method in the touched modules. They are DSL calls rather than argument-map keys, so they are outside what this PR set out to cover, but order(params.sort, params.order) inside a criteria closure is a common Grails idiom, and the new Javadoc on normalizeDirection says it is "shared by every entry point that accepts an order argument". Neither value is interpolated into a string on these paths, so there is no injection concern, only the same silent-ascending inconsistency the PR fixes elsewhere. Suggest a referenced follow-up rather than growing this PR further: one call to DynamicFinder.normalizeDirection in each of the five implementations, plus a row in the two SortArgumentValidationSpecs.
Nit: list.adoc fetch sentence is still unconditional
Reference: grails-doc/src/en/ref/Domain Classes/list.adoc:71
The sort/order half of the paragraph is now true on both Hibernate versions. The fetch half is Hibernate 7 only, as the upgrade guide correctly says ("With Hibernate 7, list() additionally requires ..."). Either qualify it the same way here or drop fetch from this sentence.
Nit: validateSortProperty accepts a bare unknown name when the entity is unknown
Reference: grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/finders/DynamicFinder.java:794-806
With entity == null the check is shape-only, so notAProperty passes. Every public entry point resolves the entity, so this is unreachable from user code and the docs' "a bare name must be a property of the domain class" holds in practice. Worth one sentence in the Javadoc's @param entity line so the next reader does not take the null branch for a hole.
Verdict
Approve. Every round-2 item that touched code is resolved, the Hibernate 5 paths are now covered too, and everything is green locally on all four modules plus the Neo4j TCK. Merge once the PR description is updated; the DSL order() item can go into a follow-up issue referenced from the description.
Summary
ASF security review follow-up: validate sort names in
DynamicFinderand the Hibernate HQL list builder.Book.list(sort: params.sort)andHqlListQueryBuilderinterpolated the sort key into query / HQL order-by clauses. A client-supplied value such asname, e.idor a non-property token was not checked against the persistent mapping.This is hardening, not an advisory, unless the threat model is expanded. Apps that concatenate HQL themselves remain out of scope.
Changes
DynamicFinderrejects sort keys that are not identifier-shaped property paths, and when a mapping is available, keys that do not resolve to a persistent property (including nested associations and identity).HqlListQueryBuilderapplies the same property-path check, requires the property to exist on the Hibernate mapping, and only allowsasc/desc.Testing
:grails-datamapping-core:test:grails-data-hibernate7-core:test --tests org.grails.orm.hibernate.query.HqlListQueryBuilderSpec