[Use Case]: `Sort` QL escaping

[FroMage]

As a ...

• Application user/user of the configuration itself
• API user (application developer)
• SPI user (container or runtime developer)
• Specification implementer

I need to be able to ...

specify whether a Sort property should be escaped or not.

Which enables me to ...

make sure that property names I get from outside sources get escaped and will not cause SQL injection, while allowing calls to Sort.desc("count(foo)").

Additional information

No response

[njr-11]

One pattern I thought we might eventually offer is to sort by an Expression. For example,

Sort<Country> densityDesc = Sort.desc(_Country.population.dividedBy(_Country.area));

or if using .desc() directly from the metamodel,

Sort<Country> densityDesc = _Country.population.dividedBy(_Country.area).desc();

It would cover some cases where a user wants to sort by a computed value. However, it wouldn't cover the one you gave an example of, Sort.desc("count(foo)"). The API kind of already allows that but the spec doesn't say one way or the other whether that usage is required/allowed/illegal.

Regarding the responsibility for guarding against SQL injection/JPQL injection, wouldn't that be the responsibility of the application that is supplying the value to the Sort?

[FroMage]

One pattern I thought we might eventually offer is to sort by an Expression. For example,

Sort densityDesc = Sort.desc(_Country.population.dividedBy(_Country.area));
or if using .desc() directly from the metamodel,

Sort densityDesc = _Country.population.dividedBy(_Country.area).desc();

That would be nice indeed.

It would cover some cases where a user wants to sort by a computed value. However, it wouldn't cover the one you gave an example of, Sort.desc("count(foo)").

Why not?

_Country.population.count().desc();

The API kind of already allows that but the spec doesn't say one way or the other whether that usage is required/allowed/illegal.

Ah OK. I guess I'll have to try it on Hibernate ORM and see where it goes. I fear it may be rejected because the equivalent Order in ORM seems super type-safe using metamodel attributes, and this isn't one.

Regarding the responsibility for guarding against SQL injection/JPQL injection, wouldn't that be the responsibility of the application that is supplying the value to the Sort?

In Hibernate with Panache we've had our users ask that all strings provided to Sort should be automatically quoted, unless explicitly disabled. Not just for injection but to protect against reserved keywords too.

Although, perhaps this is a responsibility of the underlying implementation, especially if sort expressions that are not attribute names (such as count(foo)) are disallowed.

[njr-11]

Why not?

_Country.population.count().desc();

Good point. We don't currently have the count() operation represented in the metamodel, but it could be added, and then yes, that would work, too.

users ask that all strings provided to Sort should be automatically quoted, unless explicitly disabled. Not just for injection but to protect against reserved keywords too.

I can see the value of protecting against reserved keywords. I agree an application writing an entity and repository would likely prefer not to think about which entity attribute names might collide with query language reserved words and have it automatically handled for them. Quoting would be one way for an implementation to handle that. Qualifying the attribute names with an entity identifier variable (or the implicit this. which was added in Jakarta Persistence 3.2) might be another. In either case, I can see where that would become difficult and require parsing if count(foo) or population / area is supplied a the Sort property. An implementation would need to parse the value to insert quotes or this. in the proper places rather than blindly prepending/appending around the whole expression. And then there is also the possibility that the application is already doing one of these, and an implementation duplicating it inadvertently switches a working scenario into a failing one.

I would definitely prefer the metamodel/expressions based approach if can cover what is needed.

[gavinking]

I'm not sure I understand what you're asking for here @FroMage. The identifiers passed to Sort.desc() are not raw SQL identifiers. The argument to Sort.desc() is the name of an attribute of a Java entity. And if we ever generalized that to allow something like "count(foo)", then the string would be a fragment of JDQL/JPQL, and it would still not make sense to ask for quoting.

[FroMage]

I think the reasoning behind people asking for quoting in Panche Sort was due mostly to SQL injection, originally.

The expectation is that if you provide your HQL from untrusted sources, you're vulnerable, so don't do this. Instead, you pass query parameters to the HQL query and it's safe from injection:

@Find
public List<Book> findBooks(String name);

// equivalent to: FROM Book WHERE name = :name

Now, with sorting, it's not entirely clear whether it may or may not lead to injection:

@Find
public List<Book> findBooks(String name, Order order);

// equivalent to: FROM Book WHERE name = :name ORDER BY ???

I guess it depends on how it's implemented. Perhaps this is even specified that only a single attribute is allowed? So, just Order.by(Sort.desc("name")) is fine, but what happens with Order.by(Sort.desc("relation.name")) or Order.by(Sort.desc("select something from othertable")) (I just looked up the grammar of the sort clause and did not find how to append a limit 10 but noticed it allowed an <expression>, forgive me for not finding a better injection example).

Perhaps the spec mentions that the sort expression is checked at runtime when trying to executed the query, under this line:

A repository method throws DataException if the database is incapable of ordering the query results using the given sort criteria.

[njr-11]

did not find how to append a limit 10

Jakarta Data has two solutions for limits, depending on whether you want the repository author or the repository user to decide what the limit is.

Determined by repository author:

@Find
@First(10)
public List<Book> findBooks(String name, Order order);

Determined by repository user:

@Find
public List<Book> findBooks(String name, Order order, Limit limit);
...
first10 = books.findBooks(title,
                          Order.by(_Book.yearPublished.desc(),
                                   _Book.author.asc(),
                                   _Book.id.asc()),
                          Limit.of(10));

what happens with Order.by(Sort.desc("relation.name"))

This is covered under the section 3.2.2 of the Data 1.0 spec where it says that both . and _ are considered delimiters for Sort values. Some examples are also provided in Javadoc.

Additionally, the Data 1.0 Javadoc for StaticMetamodel shows how the . delimiter is present in the static metamodel and how Sorts obtained from the attributes of an embeddable from the metamodel can be supplied to Order.by. Jakarta Data 1.1 is working on a navigable approach using the metamodel as well, in which the Sort value will remain consistent with Data 1.0.

Order.by(Sort.desc("select something from othertable"))

Jakarta Data doesn't currently have a solution for supplying subqueries or expressions to Sort. The first line of the Javadoc for Sort states, "Requests sorting on a given entity attribute." The API currently only defines behavior for entity attributes (including attributes of embeddables/relations), not for sorting on subqueries, expressions, or other query fragments. Implementations, however, might have decided to allow additional behavior. I should also point out there has only be 1 release of Jakarta Data thus far, and many topics and feature ideas such as this have just not been discussed yet. It's helpful that you brought this up here to encourage discussion.

[FroMage]

Jakarta Data has two solutions for limits

I was not trying to specify a limit, but to show how HQL injection might lead to arbitrary valid HQL, and thought of using limit because it's a valid clause after order by 😅