borinquenkid opened a new pull request, #15971:
URL: https://github.com/apache/grails-core/pull/15971
## Background
While reviewing #15968 ("Warn once on GString-interpolated GORM HQL
queries"), GitHub's Copilot reviewer left a comment on
`HibernateGormStaticApi.groovy` noting that the new runtime warning had no test
verifying it actually fires through the real Hibernate static API call path —
only through the isolated `GormQuerySafetyWarnings` helper spec. That comment
was the starting point for a deeper investigation (with Claude) into whether
the warning mechanism actually closes the gap it claims to.
That investigation found:
1. **The warning does fire correctly when a `GString` is passed directly**
to `find`/`findAll`/`executeQuery` — confirmed empirically by adding a
log-capturing assertion to the existing `HibernateGormStaticApiSpec`
"injection-safe" test and running it against #15968's branch. GORM binds the
interpolated value as a real query parameter in this case; it is not
exploitable.
2. **#15968 only wires the warning into one of three GORM implementations
with the identical code path.** `grails-data-hibernate5`'s
`AbstractHibernateGormStaticApi` and `grails-data-neo4j`'s `Neo4jGormStaticApi`
have the same `instanceof GString` / `buildNamedParameterQueryFromGString`
pattern (Neo4j's error message still literally says "HQL injection" even though
it's building Cypher) — neither references the new warning helper at all.
3. **The actual dangerous case is structurally invisible to any runtime
check.** GORM's own safe-binding mechanism only engages when the query argument
is *still* a `GString` at the point of the call. The extremely common Groovy
pattern
```groovy
String query = "from Book where name = ${userInput}" // GString ->
String, right here
Book.executeQuery(query)
```
causes Groovy to coerce the `GString` to a plain `String` at the
assignment, because `query` is declared `String`. By the time this reaches
GORM's query builder, `instanceof GString` is `false`, and the interpolated
value is used as raw, unescaped query text — a genuine injection vector. A
`java.lang.String` carries no trace of ever having been a `GString`, so no
amount of runtime checking at the query boundary can distinguish this from a
safely hand-written HQL string. The information needed to tell them apart only
exists at the source-code level, before compilation erases it.
That third point is the actual finding this PR addresses: a warning that
fires on the already-safe case and stays silent on the actually-dangerous case
is backwards from what a risk-reduction mechanism should do, regardless of
whether it warns or (as considered and rejected) throws at runtime.
## What this PR does
Adds a **global, compile-time Groovy AST transformation**
(`GormQuerySafetyTransformer` / `GlobalGormQuerySafetyASTTransformation`, in
`grails-datamapping-core`) that detects the flattened-`String` pattern above
and fails the build with a clear, actionable error — before the type
information that would let a runtime check catch it is lost.
Because the check is syntactic (method name + argument shape), not tied to a
specific datastore's runtime class, **one transform covers Hibernate5,
Hibernate7, and Neo4j simultaneously**, with zero changes to any of those three
modules and zero configuration required from application developers — it runs
automatically wherever `grails-datamapping-core` is on the compile classpath,
the same way `@GrailsCompileStatic` and domain-class enhancement already apply
themselves invisibly.
### Detection algorithm
- Tracks local variables (declarations and reassignments) that go from a
live, interpolated `GString` to a plain `String` — via explicit `String`
typing, `.toString()`, `(String)` cast, or `as String`.
- Flags a call to `find` / `findAll` / `executeQuery` / `executeUpdate` /
`findAllWithSql` / `cypherStatic` / `findPath` / `findPathTo` whose query
argument is one of those flattened variables.
- Gates on the receiver looking like a GORM domain class (reusing the
existing `AstUtils.isDomainClass`, the same utility
`DetachedCriteriaTransformer`'s `where{}` rewriting already relies on) to avoid
false positives on unrelated methods.
- A direct `GString` literal argument (`Book.executeQuery("... ${x} ...")`)
is never flagged — that path is already safe.
### Corner cases handled (with test coverage)
- **`.toString()` / `(String)` cast / `as String`** coercions of an
interpolated `GString` — all flagged, not just plain `String`-typed
declarations.
- **Reassignment**, not just declaration (`String q; q = "...${x}..."`) —
tracked via `visitBinaryExpression`, with a safe reassignment clearing prior
unsafe tracking (last-write-wins; see limitations below).
- **`Collection.find` / `Collection.findAll`** (GDK methods present on
nearly every `Iterable`) never collide with the GORM methods of the same name —
they take a `Closure`, and the flagged argument must be a tracked `String`
variable, so the two shapes can't overlap.
- **Non-domain classes that happen to define their own
`find`/`findAll(String)`** methods do not false-positive, because of the
`isDomainClass` receiver gate.
- **Closures capture the flattened state correctly** — a variable flattened
in an enclosing method and referenced inside a nested closure (`.each { ...
executeQuery(q) }`) still triggers, since closures are walked as part of their
enclosing method's traversal.
- **Neo4j's `findPathTo(Class type, CharSequence query, Map params)`** has
the query as its *second* argument, not first — handled as a special case in
the argument-index map, with a dedicated test.
- **Suppression**: a reviewed, genuinely safe call site can opt out
per-call-site with `@SuppressWarnings("GormUnsafeQueryString")` on the
enclosing method — a deliberate, auditable exception rather than a global kill
switch.
- **Multiple-assignment declarations** (`def (a, b) = [...]`) don't crash
the transform — `DeclarationExpression.getVariableExpression()` returns `null`
for these and is guarded.
### Known, deliberate limitations (documented in the class Javadoc and the
upgrade guide)
This is intentionally scoped, not a general SQL-injection solution:
- **Intraprocedural only** — a flattened `String` built inside a helper
method and returned is invisible to this check.
- **Last-write-wins reassignment tracking**, not full branch-sensitive
dataflow.
- **Locals only, not fields**, in this first version.
- **Does not catch plain string concatenation with no `GString` involved at
all** (`"select * ... " + userInput`) — arguably the more classic injection
shape, and untouched by this or #15968's runtime check, since there's no
`GStringExpression` node to detect.
- **Does not cover raw JDBC via `groovy.sql.Sql`**, which is outside GORM's
static API entirely.
- **Does not apply to MongoDB** — its GORM API has no `CharSequence`-based
raw query method at all (it inherits `GormStaticApi.executeQuery()`, which just
throws `unsupported('executeQuery')`), so it isn't vulnerable to this specific
pattern in the first place.
## Verification
- New `GormQuerySafetyTransformerSpec` — 12 tests, covering every case above
(error cases assert `MultipleCompilationErrorsException` with the expected
message; safe/suppressed/non-domain cases assert clean compilation).
- Full `grails-datamapping-core` test suite: pass.
- Full `grails-data-hibernate7-core` suite (2988 tests) and
`grails-data-hibernate5-core` suite: zero failures, confirming the global
transform introduces no false positives against real GORM/query source in
either module.
- `codeStyle` (Checkstyle + CodeNarc) clean on touched modules.
- Doc build (`publishGuide`) verified to render correctly, including the new
cross-reference between the upgrade guide and the security guide.
## Why this is opinionated, on purpose
Grails' own stated philosophy is "sensible defaults" — convention over
configuration, with escape hatches for the cases that genuinely need them. Not
permitting query-string injection by default is exactly that kind of sensible
default: an application shouldn't have to opt in to safety, and a framework
that discovers it's silently coercing a safely-parameterizable query into raw
unescaped text has an obligation to say so loudly, not log it once and move on.
That's why this fails the build rather than warning: for a compile-time check,
the "pre-prod environment" every responsible team already runs *is* the build
itself — failing to compile has no live-traffic blast radius, unlike a runtime
throw would. Teams that hit a false positive on a reviewed-safe call get an
explicit, auditable, per-call-site escape hatch
(`@SuppressWarnings("GormUnsafeQueryString")`) rather than a single switch that
quietly reopens the whole risk surface.
This is offered as an alternative to the runtime-only, single-module
approach in #15968 — happy to discuss reconciling the two (e.g. keeping
#15968's docs/warning as a softer signal for the cases this transform's
intraprocedural limits can't reach, while this closes the case that matters
most).
--
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]
