Validate dynamic finder and HQL list sort property names - #16312
jamesfredley wants to merge 2 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.
There was a problem hiding this comment.
🟢 Approval recommended
The changes consistently enforce sort property/direction validation at the relevant entry points and include targeted tests that cover the new rejection behavior.
Pull request overview
This PR hardens query sorting in the Grails GORM dynamic finder list APIs and the Hibernate HQL list query builder by validating user-supplied sort property paths (and, for HQL, sort direction) to prevent unsafe interpolation into order by clauses.
Changes:
- Added sort property-path validation to
DynamicFinderand applied it to both string and map-based sort inputs, with persistent-mapping resolution when available. - Added sort property validation + “asc/desc only” normalization/validation to
HqlListQueryBuilderto prevent invalid properties/directions from being interpolated into HQL. - Added regression tests to ensure injected/unknown sort keys (and injected directions for HQL) are rejected.
File summaries
| File | Description |
|---|---|
| grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/finders/DynamicFinder.java | Validates sort keys against an identifier-shaped property path and (when resolvable) the persistent mapping before applying ordering. |
| grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/finders/DynamicFinderCoverageSpec.groovy | Adds tests covering valid sort usage and rejecting injected/unknown sort keys (including map-form keys). |
| grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/query/HqlListQueryBuilder.java | Enforces identifier-shaped property paths, requires the mapped property to exist, and restricts direction to asc/desc for HQL list sorting. |
| grails-data-hibernate7/core/src/test/groovy/org/grails/orm/hibernate/query/HqlListQueryBuilderSpec.groovy | Adds tests ensuring injected/unknown sort properties and injected sort directions are rejected with clear errors. |
Review details
- Files reviewed: 4/4 changed files
- Comments generated: 0
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
Copilot's approval is valid. No further code changes are required: DynamicFinder and HqlListQueryBuilder already reject injected or unknown sort keys and invalid HQL directions. |
jdaugherty
left a comment
jdaugherty
left a comment
There was a problem hiding this comment.
Reviewed at 0d5b3167, with the affected modules built and run locally. Two blocking items, one of them confirmed by failing tests. The tests aren't running on this PR because the actions haven't been updated ...
1. Alias sorting regresses. validateSortProperty rejects any first path segment that is not a persistent property of the root entity, which is exactly what a criteria / where-query alias is. WhereQueryWithAssociationSortSpec now fails in both grails-data-hibernate7-core and grails-data-hibernate5-core with IllegalArgumentException: Unknown sort property: c1.name, and the pattern it covers is the one whereQueries.adoc documents as the way to sort on an association. Details inline.
2. The fetch map keys in HqlListQueryBuilder are still concatenated into HQL unvalidated. Same method, same reachability through Book.list(params); verified against the real builder. Pre-existing, but it leaves the hardening incomplete.
Beyond that: the sort-map values now drive direction where they were previously discarded (a real, unrelated behaviour change), and the new validation branches -- nested association traversal, the identity fallback, non-association mid-path -- have no tests. HqlListQueryBuilderSpec is mock-only, so the riskiest new rule is not exercised against a real mapping.
What I verified as not regressing, on real entities: sort: 'id', sort: 'version', a subclass sorted by an inherited property, an embedded path, an association path, and the mapping { sort '...' } default. :grails-datamapping-core:test is fully green, and codeStyle passes on both modules.
| if (identity != null && parts[i].equals(identity.getName()) && i == parts.length - 1) { | ||
| return; | ||
| } | ||
| throw new IllegalArgumentException("Unknown sort property: " + sort); |
There was a problem hiding this comment.
Regression: this breaks alias-based sorting, which is a documented feature.
The first path segment is resolved against the root entity's persistent properties, but criteria / where-query aliases are not persistent properties. Two existing specs fail on this branch:
:grails-data-hibernate7-core:test -> 3070 tests, 1 failed
:grails-data-hibernate5-core:test -> 812 tests, 1 failed
WhereQueryWithAssociationSortSpec > Test sort with where query that queries association FAILED
java.lang.IllegalArgumentException: Unknown sort property: c1.name
at DynamicFinder.validateSortProperty(DynamicFinder.java:809)
at DynamicFinder.addSimpleSort(DynamicFinder.java:824)
at DynamicFinder.populateArgumentsForCriteria(DynamicFinder.java:591)
at grails.gorm.DetachedCriteria.withPopulatedQuery(DetachedCriteria.groovy:741)
This is the pattern grails-data-hibernate7/docs/src/docs/asciidoc/querying/whereQueries.adoc (and the hibernate5 copy) recommends for sorting on an association:
def query = Pet.where {
def o1 = owner
o1.firstName == "Fred"
}.list(sort: 'o1.lastName')createCriteria().list(sort: 'a.name') { createAlias('author', 'a') } breaks the same way.
Suggestion: when the first segment does not resolve to a property, fall back to the shape check instead of throwing -- SORT_PROPERTY_PATTERN already rejects every injection payload (commas, whitespace, parens, quotes). Alternatively, collect the aliases declared on the query / detached criteria and accept those as roots.
Worth weighing the cost/benefit as well: on the paths in this repo Query.Order is never string-concatenated. Hibernate 7 goes through the JPA Criteria API (JpaCriteriaQueryCreator.assignOrderBy) and Hibernate 5 through org.hibernate.criterion.Order, and both resolve the name against the mapping. So this half is defence in depth for downstream GORM implementations that do build query strings, rather than a fix for a live sink here.
| * @param entity the entity being queried, or {@code null} when it cannot be resolved | ||
| * @param sort the requested sort property | ||
| */ | ||
| public static void validateSortProperty(PersistentEntity entity, String sort) { |
There was a problem hiding this comment.
Three smaller points on this method:
- It is only called from within
DynamicFinder, soprivate staticwould keep it off the public surface. - The messages echo the caller-supplied value and distinguish
Invalid sort propertyfromUnknown sort property. The value here is untrusted request input, so a single generic message avoids both reflecting the input back and letting a client probe which names are real properties. - Composite-identity entities are not covered:
getIdentity()returnsnullfor them andgetCompositeIdentity()is never consulted. It works today only because the composite id properties are left inpropertiesByName(AbstractPersistentEntityremoves them frompersistentPropertiesbut not from the by-name map), so it is leaning on an implementation detail.
| String sort = key.toString(); | ||
| final Query.Order order = ORDER_DESC.equalsIgnoreCase(orderParam) ? Query.Order.desc(sort) : Query.Order.asc(sort); | ||
| validateSortProperty(sortEntity, sort); | ||
| String direction = value != null ? value.toString() : orderParam; |
There was a problem hiding this comment.
This is a behaviour change beyond the stated scope of the PR. value was previously read and discarded, so every entry in a sort map took its direction from order. Now each entry uses its own value, so list(sort: [name: 'asc'], order: 'desc') flips from descending to ascending.
It is probably the correct behaviour -- it matches applySortForMap -- but it is undocumented, untested, and unrelated to sort-name validation. Note also that the null fallback differs between the two: orderParam here, ORDER_ASC in applySortForMap.
| methodExpressinPattern = Pattern.compile("\\p{Upper}[\\p{Lower}\\d]+(" + expressionPattern + ")"); | ||
| } | ||
|
|
||
| private static PersistentEntity resolvePersistentEntity(BuildableCriteria query) { |
There was a problem hiding this comment.
This duplicates the inline block a little further up in populateArgumentsForCriteria -- the sortObject == null && orderParam != null branch does the same two instanceof checks to resolve the entity. Worth calling the new helper there too.
| public static final String ARGUMENT_IGNORE_CASE = "ignoreCase"; | ||
| public static final String ARGUMENT_CACHE = "cache"; | ||
| public static final String ARGUMENT_LOCK = "lock"; | ||
| private static final Pattern SORT_PROPERTY_PATTERN = Pattern.compile("[A-Za-z_][A-Za-z0-9_]*(\\.[A-Za-z_][A-Za-z0-9_]*)*"); |
There was a problem hiding this comment.
Three notes on the pattern:
- It is byte-for-byte identical to
PROPERTY_PATHinHqlListQueryBuilder.grails-data-hibernate7-coredeclaresapi project(':grails-datamapping-core'), so a single shared constant would keep the two from drifting. [A-Za-z_][A-Za-z0-9_]*rejects$and non-ASCII characters, both legal in Groovy/Java identifiers and therefore in domain property names.Character.isJavaIdentifierStart/isJavaIdentifierPartis the faithful check.- It sits in the middle of the
public static final String ARGUMENT_*block; moving it above or below keeps that group intact.
| if (propertyName == null || propertyName.isBlank()) { | ||
| return ""; | ||
| } | ||
| if (!PROPERTY_PATH.matcher(propertyName).matches()) { |
There was a problem hiding this comment.
The fetch keys in this same builder are still concatenated into HQL unvalidated. buildListHql, line 61:
hql.append(" join fetch e.").append(prop);prop is a raw map key, reachable exactly the way sort is: Book.list(params) with ?fetch.<injected>=join. GrailsParameterMap.processNestedKeys turns dotted request parameters into a nested Map, which satisfies the fetchObj instanceof Map check, and the value is attacker-controlled too.
Confirmed against the real builder:
input : [fetch: ['books left join fetch e.secretNotes': 'join']]
output: "from Person e join fetch e.books left join fetch e.secretNotes"
Pre-existing rather than introduced here, but hardening sort while leaving the sibling sink in the same method leaves the fix incomplete.
| private String buildSortPart(String propertyName, String direction, boolean ignoreCase) { | ||
| if (propertyName == null) return ""; | ||
| String path = "e." + propertyName; | ||
| if (propertyName == null || propertyName.isBlank()) { |
There was a problem hiding this comment.
Two small edges in this method:
- A blank property name returns
"". For aStringsort that is harmless --buildSortClausereturns empty and noorder byis appended -- but in theMapbranch the empty string is still joined, producingorder by , e.name asc. Either reject blank alongside the other invalid shapes, or filter empty parts beforeString.join. normalizeDirectiondoes not trim, soorder: ' desc'now throws where it previously produced working HQL.
| [max: 10, offset: 5] | true | ||
| } | ||
|
|
||
| void "test buildListHql rejects injected sort property"() { |
There was a problem hiding this comment.
These specs run entirely against Mock(GrailsHibernatePersistentEntity), so the new "throw when getHibernatePropertyByPath returns null" rule is never exercised against a real Hibernate mapping -- which is the part most likely to regress.
I ran the following against real entities on this branch and they all still pass, but they are what needs pinning here so a future mapping change cannot silently break list():
list(sort: 'id')andlist(sort: 'version')- a subclass sorted by a property inherited from its superclass
- an embedded path, e.g.
sort: 'address.city' - the default sort from
mapping { sort 'label' } - an association path, e.g.
sort: 'club.name'
Also missing: a spec for injected fetch keys (see the comment on buildSortPart).
| results*.name.sort() == ['Alice', 'Charlie'] | ||
| } | ||
|
|
||
| void "list(sort) still sorts by a mapped property"() { |
There was a problem hiding this comment.
The added coverage stops at the happy path and the two rejection cases. The new branches in validateSortProperty are untested:
- nested association traversal (
sort: 'assoc.prop'accepted) - a non-association property mid-path (rejected)
- an association whose
getAssociatedEntity()is null (rejected) - the identity fallback (
sort: 'id', andsort: 'assoc.id') - the direction-from-map-value change in
populateArgumentsForCriteria
For the checklist: the PR ran :grails-data-hibernate7-core:test --tests HqlListQueryBuilderSpec only. A full run of the affected modules is what surfaces the WhereQueryWithAssociationSortSpec failure.
There was a problem hiding this comment.
🟡 Changes recommended
The critical association-alias regression must be fixed before approval.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/finders/DynamicFinder.java:795
- The public
list(sort:)contract now rejects malformed and unknown paths withIllegalArgumentException, but this user-facing behavior change has no accompanyinggrails-docupdate. Document the accepted property-path format, nested/identity handling, and failure behavior as required for user-facing framework changes.
public static void validateSortProperty(PersistentEntity entity, String sort) {
if (sort == null || !SORT_PROPERTY_PATTERN.matcher(sort).matches()) {
throw new IllegalArgumentException("Invalid sort property: " + sort);
- Files reviewed: 4/4 changed files
- Comments generated: 1
- Review effort level: Balanced
| } | ||
|
|
||
| private static void addSimpleSort(Query q, String sort, String order, boolean ignoreCase) { | ||
| validateSortProperty(q.getEntity(), sort); |
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.
|
All of my feedback is implemented in jamesfredley#5 |
AI Review FindingsReviewed together with the follow-up at jamesfredley#5 (one commit, Verified locally on the follow-up head:
Everything below is against the follow-up head. None of it is blocking. Suggestion: the alias pass-through is wider than it needs to beReferences:
A first segment that is not a persistent property is now accepted on the shape check alone so that criteria and where-query aliases keep working. An alias is only ever used as What the pass-through costs today, probed against a real Hibernate 7 mapping: That NPE is pre-existing rather than introduced here, but the validation now has everything it needs to turn it into the generic Nit: direction trimming differs between entry pointsReferences:
The new sentence in Nit:
|
|
Review follow-up that was mistakenly opened on the archive fork is now #16329 |
a391b66 to
0d5b316
Compare
|
This PR now includes jdaugherty's review follow-up (previously jamesfredley#5 / #16329). |
|
Superseded by #16333 so the head branch is on apache/grails-core (not the archive fork). |
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