jamesfredley commented on PR #15568:
URL: https://github.com/apache/grails-core/pull/15568#issuecomment-4735526695
## Note on SQL/HQL injection safety (single-argument HQL overloads)
While expanding the Hibernate 7 functional coverage, we examined an
injection-safety concern in the single-argument `find` / `findAll` /
`executeQuery` / `executeUpdate(CharSequence)` overloads.
At one point the Hibernate 7 path **rejected plain `String` HQL** in these
overloads - a `requireGString` guard that threw `UnsupportedOperationException`
unless a Groovy `GString` was passed, on the theory that this prevented HQL
injection. That diverged from Hibernate 5 and broke the natural calling
convention (e.g. `Book.executeQuery("from Book where inStock = true")` no
longer worked).
**What we found:** the guard was unnecessary, because GORM already makes the
idiomatic Groovy form injection-safe **by binding, not by blocking**. When a
`GString` is passed, `HqlQueryContext` extracts every `${value}` interpolation
and binds it as a **named parameter** (`:p0`, ...) rather than interpolating it
into the query text. So:
- `executeQuery("from Book where inStock = true")` - a plain, static
`String` - has no untrusted input and runs as written, exactly as on Hibernate
5.
- `executeQuery("from Book where title = ${userInput}")` - a `GString` - is
**bound**, not interpolated: `userInput` becomes a parameter value and can
never alter the query structure.
The only genuinely unsafe pattern is one the developer must write
deliberately - hand-concatenating untrusted input into a plain `String` (`"...
where title = '" + userInput + "'"`) - which is an injection risk in *any* ORM
and which the guard did **not** actually prevent (a pre-concatenated `String`
is still just a `String`).
**Resolution:** the `requireGString` guard was removed, so plain-String HQL
works as on Hibernate 5 while the existing GString-binding behavior keeps the
interpolated form injection-safe. This is locked with a regression test that
interpolates a SQL-injection-style value (`missing' or '1'='1`) into a
`GString` query and asserts it is bound as a parameter (matching no rows)
rather than executed as predicate logic. The Hibernate 7 HQL/upgrade docs were
updated to describe this binding-based safety rather than the (removed) hard
rejection.
--
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]
