Hi all,
While reviewing #15968 ("Warn once on GString-interpolated GORM HQL queries"),
I traced through the actual mechanism it's checking and found some gaps worth
discussing before we merge either PR. I ended up with a second PR proposing a
different fix, so let's put both in front of the list ahead of Wednesday. This
analysis was done with AI assistance (Claude), consistent with how #15968
itself is labeled.
The problem: a GString passed directly to a GORM query method
(find/findAll/executeQuery/etc.) is safe — GORM binds each interpolated value
as a real parameter. But assigning it to a String-typed local first (String q =
"...${x}..."; Book.executeQuery(q)) makes Groovy coerce it to a plain String at
that assignment, and the safety silently disappears. By the time it reaches the
query call there's nothing left to distinguish it from hand-written HQL — the
interpolated value is now raw, unescaped text.
Approach A — #15968, runtime warning. Logs once when the query argument is
still a GString at the Hibernate7 query boundary. Non-breaking. Two gaps: it
only fires on the case that's already safe — a runtime check structurally can't
see the flattened-String case, since a String carries no trace of ever having
been a GString — and it's wired into Hibernate7 only. Hibernate5 and Neo4j have
the identical code shape and are untouched.
Approach B — #15971, compile-time AST check. A global Groovy transform that
catches the flattened-String pattern at compile time and fails the build by
default, with a per-call-site @SuppressWarnings("GormUnsafeQueryString") escape
hatch for reviewed-safe cases. Because it's a syntactic check rather than a
per-datastore patch, it covers Hibernate5, Hibernate7, and Neo4j at once with
no changes to any of their source. Tradeoffs: it's a breaking check for anyone
using this pattern today, and it's intraprocedural onilt in a helper method and
returned is invisible to it. It's not a general SQL-injection solution — no
plain concatenation, no raw JDBC, nothing outside HQL/Cypher.
Both have full test coverage and pass clean against the existing Hibernate5/7
suites.
Discussion points for Wednesday:
- Compile-time break, or is warn-only the rightin the 8.0.x cycle?
- If compile-time, hard error from day one, or a softer rollout?
- Worth keeping both — #15968 as a gentler sign backstop?
PRs: #15968 / #15971
Thanks,
Walter
