jdaugherty commented on code in PR #16312:
URL: https://github.com/apache/grails-core/pull/16312#discussion_r3931597831
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/query/HqlListQueryBuilder.java:
##########
@@ -111,13 +114,35 @@ private String buildSortClause() {
}
private String buildSortPart(String propertyName, String direction,
boolean ignoreCase) {
- if (propertyName == null) return "";
- String path = "e." + propertyName;
+ if (propertyName == null || propertyName.isBlank()) {
+ return "";
+ }
+ if (!PROPERTY_PATH.matcher(propertyName).matches()) {
Review Comment:
**The `fetch` keys in this same builder are still concatenated into HQL
unvalidated.** `buildListHql`, line 61:
```java
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.
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/finders/DynamicFinder.java:
##########
@@ -486,7 +490,9 @@ else if (sortObject instanceof Map) {
for (Object key : sortMap.keySet()) {
Object value = sortMap.get(key);
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;
Review 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`.
##########
grails-data-hibernate7/core/src/test/groovy/org/grails/orm/hibernate/query/HqlListQueryBuilderSpec.groovy:
##########
@@ -208,4 +208,39 @@ class HqlListQueryBuilderSpec extends Specification {
[offset: 5] | true
[max: 10, offset: 5] | true
}
+
+ void "test buildListHql rejects injected sort property"() {
Review 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')` and `list(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`).
##########
grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/finders/DynamicFinderCoverageSpec.groovy:
##########
@@ -251,6 +251,39 @@ class DynamicFinderCoverageSpec extends Specification {
then:
results*.name.sort() == ['Alice', 'Charlie']
}
+
+ void "list(sort) still sorts by a mapped property"() {
Review 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'`, and `sort: '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.
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/finders/DynamicFinder.java:
##########
@@ -767,7 +773,55 @@ private static void resetMethodExpressionPattern() {
methodExpressinPattern = Pattern.compile("\\p{Upper}[\\p{Lower}\\d]+("
+ expressionPattern + ")");
}
+ private static PersistentEntity resolvePersistentEntity(BuildableCriteria
query) {
+ if (query instanceof AbstractCriteriaBuilder) {
+ return ((AbstractCriteriaBuilder) query).getPersistentEntity();
+ }
+ if (query instanceof AbstractDetachedCriteria) {
+ return ((AbstractDetachedCriteria) query).getPersistentEntity();
+ }
+ return null;
+ }
+
+ /**
+ * Rejects sort keys that are not identifier-shaped property paths, and
when a
+ * mapping is available, keys that do not resolve to a persistent property.
+ *
+ * @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) {
+ if (sort == null || !SORT_PROPERTY_PATTERN.matcher(sort).matches()) {
+ throw new IllegalArgumentException("Invalid sort property: " +
sort);
+ }
+ if (entity == null) {
+ return;
+ }
+ PersistentEntity current = entity;
+ String[] parts = sort.split("\\.");
+ for (int i = 0; i < parts.length; i++) {
+ PersistentProperty prop = current.getPropertyByName(parts[i]);
+ if (prop == null) {
+ PersistentProperty identity = current.getIdentity();
+ if (identity != null && parts[i].equals(identity.getName()) &&
i == parts.length - 1) {
+ return;
+ }
+ throw new IllegalArgumentException("Unknown sort property: " +
sort);
Review 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:
```groovy
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.
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/finders/DynamicFinder.java:
##########
@@ -767,7 +773,55 @@ private static void resetMethodExpressionPattern() {
methodExpressinPattern = Pattern.compile("\\p{Upper}[\\p{Lower}\\d]+("
+ expressionPattern + ")");
}
+ private static PersistentEntity resolvePersistentEntity(BuildableCriteria
query) {
Review 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.
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/finders/DynamicFinder.java:
##########
@@ -91,6 +92,7 @@ public abstract class DynamicFinder extends AbstractFinder
implements QueryBuild
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_]*)*");
Review Comment:
Three notes on the pattern:
- It is byte-for-byte identical to `PROPERTY_PATH` in `HqlListQueryBuilder`.
`grails-data-hibernate7-core` declares `api
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` / `isJavaIdentifierPart` is 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.
##########
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/query/HqlListQueryBuilder.java:
##########
@@ -111,13 +114,35 @@ private String buildSortClause() {
}
private String buildSortPart(String propertyName, String direction,
boolean ignoreCase) {
- if (propertyName == null) return "";
- String path = "e." + propertyName;
+ if (propertyName == null || propertyName.isBlank()) {
Review Comment:
Two small edges in this method:
- A blank property name returns `""`. For a `String` sort that is harmless
-- `buildSortClause` returns empty and no `order by` is appended -- but in the
`Map` branch the empty string is still joined, producing `order by , e.name
asc`. Either reject blank alongside the other invalid shapes, or filter empty
parts before `String.join`.
- `normalizeDirection` does not trim, so `order: ' desc'` now throws where
it previously produced working HQL.
##########
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/finders/DynamicFinder.java:
##########
@@ -767,7 +773,55 @@ private static void resetMethodExpressionPattern() {
methodExpressinPattern = Pattern.compile("\\p{Upper}[\\p{Lower}\\d]+("
+ expressionPattern + ")");
}
+ private static PersistentEntity resolvePersistentEntity(BuildableCriteria
query) {
+ if (query instanceof AbstractCriteriaBuilder) {
+ return ((AbstractCriteriaBuilder) query).getPersistentEntity();
+ }
+ if (query instanceof AbstractDetachedCriteria) {
+ return ((AbstractDetachedCriteria) query).getPersistentEntity();
+ }
+ return null;
+ }
+
+ /**
+ * Rejects sort keys that are not identifier-shaped property paths, and
when a
+ * mapping is available, keys that do not resolve to a persistent property.
+ *
+ * @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) {
Review Comment:
Three smaller points on this method:
- It is only called from within `DynamicFinder`, so `private static` would
keep it off the public surface.
- The messages echo the caller-supplied value and distinguish `Invalid sort
property` from `Unknown 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()` returns
`null` for them and `getCompositeIdentity()` is never consulted. It works today
only because the composite id properties are left in `propertiesByName`
(`AbstractPersistentEntity` removes them from `persistentProperties` but not
from the by-name map), so it is leaning on an implementation detail.
--
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]
