Compile-time check for GORM query strings flattened from GString (alternative to #15968)

[borinquenkid]

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 Warn once on GString-interpolated GORM HQL queries #15968's branch. GORM binds the interpolated value as a real query parameter in this case; it is not exploitable.
2. Warn once on GString-interpolated GORM HQL queries #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

   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 Warn once on GString-interpolated GORM HQL queries #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).

[Copilot]

Pull request overview

This PR introduces a global Groovy AST transformation in grails-datamapping-core that fails compilation when a GORM query String argument is detected to have been flattened from an interpolated GString before reaching a GORM query method (closing an injection vector that runtime checks cannot see). It also documents the behavior and adds a dedicated spec suite to validate the detection and suppression behavior.

Changes:

• Add GormQuerySafetyTransformer + GlobalGormQuerySafetyASTTransformation as a global compile-time check, ordered via GroovyTransformOrder.
• Register the global AST transform via META-INF/services so it runs automatically wherever grails-datamapping-core is on the compile classpath.
• Add upgrade/security guide documentation and a new Spock spec suite covering key positive/negative cases and suppression.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 1 comment.

Show a summary per file

File	Description
grails-doc/src/en/guide/upgrading/upgrading80x.adoc	Adds an upgrade note describing the new compile-time GORM query safety compilation failure and suppression mechanism.
grails-doc/src/en/guide/security/securingAgainstAttacks.adoc	Adds a security guide section explaining the “flattened GString → String” injection risk and the compile-time enforcement.
grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/query/transform/GormQuerySafetyTransformerSpec.groovy	Adds new tests verifying compilation failures, safe cases, Neo4j argument indexing, closure traversal, and suppression.
grails-datamapping-core/src/main/resources/META-INF/services/org.codehaus.groovy.transform.ASTTransformation	Registers the new global AST transformation so it runs automatically.
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/query/transform/GormQuerySafetyTransformer.java	Implements the visitor that tracks unsafe flattening and reports compilation errors at call sites.
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/query/transform/GlobalGormQuerySafetyASTTransformation.java	Wires the visitor as a global transform at CANONICALIZATION with an explicit priority.
grails-common/src/main/groovy/org/apache/grails/common/compiler/GroovyTransformOrder.groovy	Introduces a new transform order constant to place the query-safety check in the global transform sequence.

---

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

[bito-code-review]

The current implementation of isUnsafeGStringCoercion indeed focuses on the immediate point of coercion (assignment or explicit cast/toString). As noted in the class Javadoc, the transformer is designed with deliberate limitations for its first version, specifically that it is intraprocedural and uses last-write-wins reassignment tracking rather than full dataflow analysis.

To address the scenario where a GString is stored in a local variable and later coerced or passed to a query method, the transformer would need to track the origin of variables beyond the immediate assignment expression. Currently, flattenedStringVars only stores the ASTNode of the declaration or assignment where the unsafe coercion was detected. Expanding this to detect cases like String q = g; Book.executeQuery(q) would require tracking variable aliases or propagating the "unsafe" state through variable assignments, which is significantly more complex than the current implementation.

grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/query/transform/GormQuerySafetyTransformer.java

private boolean isUnsafeGStringCoercion(Expression expression, ClassNode declaredType) {
        if (isInterpolatedGString(expression)) {
            return ClassHelper.STRING_TYPE.equals(declaredType);
        }
        if (expression instanceof CastExpression) {
            CastExpression cast = (CastExpression) expression;
            return ClassHelper.STRING_TYPE.equals(cast.getType()) && isInterpolatedGString(cast.getExpression());
        }
        if (expression instanceof MethodCallExpression) {
            MethodCallExpression call = (MethodCallExpression) expression;
            return "toString".equals(call.getMethodAsString()) && isInterpolatedGString(call.getObjectExpression());
        }
        return false;
    }

[borinquenkid]

@bito-code-review Good catch, and correct — the detector only matched the immediate coercion expression, so this slipped through:

def g = "from Book where name = ${x}"   // g: still a live GString
String q = g                             // flattened HERE, uncaught
Book.executeQuery(q)

Fixed in 46c4ba2: replaced the single flattenedStringVars set with a two-state model (LIVE_GSTRING / FLATTENED) that resolves through VariableExpression references, so the flattening point is found correctly regardless of how many variable-to-variable hops separate the original GString literal from the query call.

Added regression tests for the exact case above, a two-hop alias chain (def g = ...; def h = g; String q = h), .toString() called on an alias, and re-aliasing an already-flattened variable — all in GormQuerySafetyTransformerSpec. Full grails-datamapping-core, grails-data-hibernate7-core, and grails-data-hibernate5-core suites still pass with zero regressions.

Still out of scope by design (see the class Javadoc / PR description): a flattened String built inside a different method and returned is invisible to this check, since the analysis is intraprocedural.

[codecov]

Codecov Report

❌ Patch coverage is 84.53237% with 43 lines in your changes missing coverage. Please review.
✅ Project coverage is 51.5723%. Comparing base (06f5567) to head (cb3a575).
⚠️ Report is 230 commits behind head on 8.0.x.

Files with missing lines	Patch %	Lines
...rm/query/transform/GormQuerySafetyTransformer.java	84.2697%	17 Missing and 25 partials ⚠️
...nsform/GlobalGormQuerySafetyASTTransformation.java	90.9091%	1 Missing ⚠️

Additional details and impacted files

@@                Coverage Diff                 @@
##                8.0.x     #15971        +/-   ##
==================================================
+ Coverage     49.6039%   51.5723%   +1.9684%     
- Complexity      16956      17865       +909     
==================================================
  Files            1999       2041        +42     
  Lines           93791      95815      +2024     
  Branches        16421      16632       +211     
==================================================
+ Hits            46524      49414      +2890     
+ Misses          40100      39075      -1025     
- Partials         7167       7326       +159     

Files with missing lines	Coverage Δ
...nsform/GlobalGormQuerySafetyASTTransformation.java	90.9091% <90.9091%> (ø)
...rm/query/transform/GormQuerySafetyTransformer.java	84.2697% <84.2697%> (ø)

... and 234 files with indirect coverage changes

🚀 New features to boost your workflow:

• ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.
• 📦 JS Bundle Analysis: Save yourself from yourself by tracking and limiting bundle sizes in JS merges.

[davydotcom]

This supersedes #15968 and is a fairly elegant compile time safety check. It is important to note there are loopholes it cannot account for and it may be worth considering if this is even the best way to do sql injection protections long term since its hard to determine casting to a String. I approve this and recommend it for merge

[borinquenkid]

Pushed a follow-up commit (cceff551) extending this check, based on a review discussion about what the check does and doesn't cover, and how confidently it should act on each case: for patterns the detection can't verify with full confidence, warn instead of failing the build - the goal isn't to prevent every possible unsafe pattern, just to close the gap decisively where detection is precise, and surface the rest for review rather than staying silent on it.

Three changes:

1. Bug fix: branch-sensitive reassignment tracking. The existing local-variable tracking was last-write-wins across an if/else - a variable flattened in only one branch could be silently cleared if the other branch reassigned it safely, depending on which branch the visitor happened to traverse last (not which one actually runs at runtime). visitIfElse now walks each branch from the same starting state and merges pessimistically: unsafe in either branch stays unsafe after the statement. Still build-breaking, since this is the same precise local-variable case as before.

2. New warning: field-based flattening. A String-typed field assigned an interpolated GString (via its initializer or this.field = ...) that's later read through this.field in a query call is now flagged. This is a compile-time warning, not an error - a field can be reassigned from a constructor, another method, or a subclass that this check never visits, so the detection is inherently less certain than the local-variable case.

3. New warning: string concatenation. Query text built with + from a non-constant value and no GString involved at all (e.g. "select ... " + userInput) now warns. Also a warning rather than an error, since concatenation is common enough for benign, non-query purposes that a hard failure would be too blunt an instrument. One subtlety this turned up: concatenating a live GString with more text ("...${x}..." + " order by title") actually returns a plain String at runtime (GString.plus returns String), so that case is correctly promoted to the existing build-breaking error rather than this warning - it's the same irreversible coercion .toString() causes, not a lower-confidence pattern.

Both new warnings share the existing @SuppressWarnings("GormUnsafeQueryString") suppression.

Verification:

• 10 new cases added to GormQuerySafetyTransformerSpec (26 total, all passing) covering both warnings, the branch-sensitivity fix, and the GString-concatenation promotion.
• Full grails-datamapping-core suite green, codeStyle clean.
• Compiled grails-data-hibernate5-core and grails-data-hibernate7-core's test sources under the stricter checks - clean, confirming no new false positives against real GORM implementation code.
• Doc guide (securingAgainstAttacks.adoc, upgrading80x.adoc) updated and rendered via publishGuide to cover both new warnings.

[jdaugherty]

My largest concern is not having an opt out that can be configured - in the event of an AST related bug. Otherwise, I love this change.

[matrei]

Nice work @borinquenkid!

I agree with @jdaugherty, there should be a global opt-out to not run the check at all and strings created at the call site should also be flagged for consistency.

[jdaugherty]

re-reading my comments, i want to be clear, i'm ok for this on by default, i just want an opt out - mostly in case something goes wrong

[paulk-asert]

Groovy 6 throws a SqlException rather than just provide a warning in more cases with a system property for opt-out:

https://issues.apache.org/jira/browse/GROOVY-12118
apache/groovy#2647

Not directly relevant but confirms the direction being taken here will become familiar to Groovy folks.

But having said that, the case AI is showing looks weird to me:

String query = "from Book where name = ${userInput}"

As a String, would this still be well-formed? For SQL, this would go to: "from Book where name = someInput" but this wouldn't be well-formed unless it was "from Book where name = 'someInput'". I am not sure how HQL handles that. The case that Groovy looks for is:

String query = "from Book where name = '${userInput}'"

[testlens-app]

✅ All tests passed ✅

🏷️ Commit: cb3a575
▶️ Tests: 56784 executed
⚪️ Checks: 59/59 completed

---

Learn more about TestLens at testlens.app.

[jdaugherty]

All four points from my earlier rounds are addressed at cb3a575, verified locally against the PR head:

1. Global opt-out — protectSqlInjectionAttacks system property (defaults to true) short-circuits the global transform, with three tests covering unset/true/false and the behavior documented in upgrading80x.adoc.
2. @RestoreSystemProperties — adopted in GlobalGormQuerySafetyASTTransformationSpec.
3. Inline flattening at the call site — findUnsafeArgument now runs a non-variable argument through the CastExpression / .toString() checks, so all three forms I verified as slipping through previously ("...${x}".toString(), (String) "...${x}", "...${x}" as String) are flagged, each with its own test.
4. @since — reconciled to 8.0 in both GormQuerySafetyTransformer and GlobalGormQuerySafetyASTTransformation.

Verified: GormQuerySafetyTransformerSpec (30) and GlobalGormQuerySafetyASTTransformationSpec (3) all pass, and the full grails-datamapping-core suite is green (289 tests, 0 failures).

The documented gaps remain (interprocedural flattening, fields, plain concatenation, groovy.sql.Sql), but they are called out in both the class Javadoc and the guide, and the kill switch covers the AST-bug scenario that was my main concern. LGTM.