Warn once on GString-interpolated GORM HQL queries

[jamesfredley]

Description

What was found

Problem	Impact
Developers may interpolate values into HQL via GString	Unsafe-looking query patterns (values are actually parameter-bound on Hibernate 7)
Google Doc 2.6 listed GORM query safety as not covered	No current open PR
Hard failures would break existing apps	Prefer warn-first starter

What changed

Area	Change
Utility	GormQuerySafetyWarnings (in shared grails-datamapping-core) detects GString queries with interpolated values
Behavior	Logs a one-time, datastore-neutral warning per operation/query shape; the dedup cache is bounded (MAX_WARNED_QUERY_SHAPES = 1000) and thread-safe, and the shape is only built when warn logging is enabled
Hibernate path	Wired into the Hibernate 7 GORM static query APIs (find/findAll/executeQuery/executeUpdate)
Docs	Securing-against-attacks guide + executeQuery/find refs + upgrade note, all scoped to "when using Hibernate 7"; removed a misleading example that mislabeled a parameter-bound GString as injectable
Tests	Warn-once / no-warn-for-plain-string / bounded-cache reset unit tests + a Hibernate ListAppender integration test asserting one warning that omits the interpolated value

Review feedback addressed (follow-up commits 244cecd5a5, 7ae397fd94)

Source	Item	Resolution
Copilot	Unbounded static set / shape built when warn disabled	warnEnabled checked first; cache bounded + synchronized; reset test added
Copilot	Message hard-codes "HQL" though also used for native SQL	Neutral wording: "GString-interpolated query passed to [...]"
Copilot	No Hibernate-module test for the warning	Added HibernateGormStaticApiSpec Logback ListAppender test
@borinquenkid	Only works on Hibernate 7 (H5/Neo4j not covered)	Docs scoped to Hibernate 7 (no over-promise); shared helper left reusable for a future cross-datastore / compile-time pass

Out of scope / follow-up

Topic	Status
Compile-time AST ban of unsafe GString HQL	Follow-up (deferring to @borinquenkid's compile-time solution)
Wiring the shared helper into Hibernate 5 / Neo4j static APIs	Follow-up
Throwing by default	Not this PR (warn only)
THREAT_MODEL.md GString consistency pass	Follow-up (security-sensitive; outside this PR's file set)

Contributor Checklist

Issue and Scope

• Background explains query-safety gap.
• Warn-only, non-breaking starter.
• Single focused change.
• Targets 8.0.x.

Code Quality

• Unit tests for warning helper (incl. bounded-cache reset).
• Hibernate integration test asserting one warning without interpolated values.
• No mass reformatting.
• AI starting point labeled.

Licensing and Attribution

• Apache License 2.0.
• Contributor rights confirmed.
• ai-generated-starting-point label applied.

Documentation

• Security and ref docs updated and scoped to Hibernate 7.
• PR description explains what changed and why.

Assisted-by: Sisyphus:openai/gpt-5.6-terra [gpt-coding]

[Copilot]

Pull request overview

This PR adds a warn-first safety mechanism for GORM HQL queries expressed as GString (interpolated) to encourage migration toward explicit named parameters, while preserving the existing behavior of binding interpolated values as query parameters. It wires the warning into Hibernate GORM static HQL query execution paths and updates the Grails 8 docs to explain the new warning and recommended patterns.

Changes:

• Introduces GormQuerySafetyWarnings to detect GString HQL queries and warn once per operation/query shape.
• Hooks the warning into Hibernate GORM static HQL query preparation/execution.
• Updates reference/security/upgrade docs with safer query examples and migration guidance.

Reviewed changes

Copilot reviewed 7 out of 7 changed files in this pull request and generated 3 comments.

Show a summary per file

File	Description
grails-doc/src/en/ref/Domain Classes/find.adoc	Adds guidance to prefer named params; documents the new GString warning behavior.
grails-doc/src/en/ref/Domain Classes/executeQuery.adoc	Adds guidance to prefer named params; documents the new GString warning behavior.
grails-doc/src/en/guide/upgrading/upgrading80x.adoc	Adds an upgrade note explaining the new warning and recommended query patterns.
grails-doc/src/en/guide/security/securingAgainstAttacks.adoc	Refreshes SQL/HQL injection guidance and references the new warning behavior.
grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/query/GormQuerySafetyWarningsSpec.groovy	Adds unit tests for “warn once” and for avoiding logging interpolated values.
grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/query/GormQuerySafetyWarnings.groovy	Implements one-time warning logic and query-shape redaction for GString queries.
grails-data-hibernate7/core/src/main/groovy/org/grails/orm/hibernate/HibernateGormStaticApi.groovy	Wires the warning helper into Hibernate static HQL query preparation.

---

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

[codecov]

Codecov Report

❌ Patch coverage is 91.66667% with 2 lines in your changes missing coverage. Please review.
✅ Project coverage is 51.1124%. Comparing base (b980413) to head (2439fb3).

Files with missing lines	Patch %	Lines
...atastore/gorm/query/GormQuerySafetyWarnings.groovy	91.6667%	2 Missing ⚠️

Additional details and impacted files

@@               Coverage Diff                @@
##             8.0.x     #15968         +/-   ##
================================================
+ Coverage         0   51.1124%   +51.1124%     
- Complexity       0      17614      +17614     
================================================
  Files            0       2042       +2042     
  Lines            0      95517      +95517     
  Branches         0      16592      +16592     
================================================
+ Hits             0      48821      +48821     
- Misses           0      39413      +39413     
- Partials         0       7283       +7283     

Files with missing lines	Coverage Δ
...atastore/gorm/query/GormQuerySafetyWarnings.groovy	91.6667% <91.6667%> (ø)

... and 2041 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.

[borinquenkid]

This solution only works on H7, it needs to work also H5 and neo4j. I am building a compile time solution but if you want to expand the scope to all the other datastores that is a choice.

[jamesfredley]

Review feedback addressed

Pushed 244cecd5a5 (code) and 7ae397fd94 (docs), after merging the latest 8.0.x.

Copilot review comments (all three resolved):

1. Unbounded static set / builds the query shape even when warn logging is disabled - warnIfGStringQuery now checks logger.warnEnabled before building the query shape or warning key and returns early on the disabled path. WARNED_GSTRING_QUERY_SHAPES is bounded by MAX_WARNED_QUERY_SHAPES (1000); the size-check / clear-on-overflow / add run inside a synchronized block so it stays thread-safe and cannot grow without limit. Added a GormQuerySafetyWarningsSpec case that floods >1000 distinct shapes and asserts a previously-warned shape warns again after the bounded reset.
2. Message hard-codes "HQL" though the helper also runs for native SQL - the warning now reads "GString-interpolated query passed to [...]" (datastore-neutral).
3. No Hibernate-module test asserting the warning fires once without interpolated values - added HibernateGormStaticApiSpec "Test GString query warning is emitted once without interpolated values" using a Logback ListAppender; it runs the same GString executeQuery twice and asserts exactly one WARN, that it contains GString-interpolated query, and that it does not contain the interpolated secret. The module's test binding moved to logback-classic with a logback-test.xml mirroring the previous slf4j-simple levels so suite log output is unchanged.

@borinquenkid - Hibernate 5 / Neo4j scope: you're right that the wiring is Hibernate 7-only, so I did not over-promise it. The user-facing docs (securingAgainstAttacks.adoc, executeQuery.adoc, find.adoc) are now explicitly scoped to "when using Hibernate 7", and the misleading GString example that was mislabeled as SQL-injection-vulnerable was removed (an unquoted GString value is parameter-bound by GORM, not spliced into the HQL text). The warn-only helper itself lives in shared grails-datamapping-core (org.grails.datastore.gorm.query.GormQuerySafetyWarnings), so it can be reused by the Hibernate 5 / Neo4j static APIs later. I've intentionally left the cross-datastore + compile-time direction to your solution rather than expanding this PR's scope; happy to wire the shared helper into the other AbstractGormStaticApi paths in a follow-up if you'd prefer that over the compile-time approach.

Note for a follow-up: THREAT_MODEL.md (11 / the SQL-injection non-finding) still describes generic GString interpolation as injectable, which is now slightly inconsistent with the clarified guide wording. I left it untouched here since it's outside this PR's file set and is security-sensitive - worth a small consistency pass to distinguish a cleanly parameter-bound GString from real string concatenation/coercion.

Local verification: :grails-datamapping-core:test --tests GormQuerySafetyWarningsSpec and :grails-data-hibernate7-core:test --tests HibernateGormStaticApiSpec both pass.

[borinquenkid]

Review feedback addressed

Pushed 244cecd5a5 (code) and 7ae397fd94 (docs), after merging the latest 8.0.x.

Copilot review comments (all three resolved):

1. Unbounded static set / builds the query shape even when warn logging is disabled - warnIfGStringQuery now checks logger.warnEnabled before building the query shape or warning key and returns early on the disabled path. WARNED_GSTRING_QUERY_SHAPES is bounded by MAX_WARNED_QUERY_SHAPES (1000); the size-check / clear-on-overflow / add run inside a synchronized block so it stays thread-safe and cannot grow without limit. Added a GormQuerySafetyWarningsSpec case that floods >1000 distinct shapes and asserts a previously-warned shape warns again after the bounded reset.
2. Message hard-codes "HQL" though the helper also runs for native SQL - the warning now reads "GString-interpolated query passed to [...]" (datastore-neutral).
3. No Hibernate-module test asserting the warning fires once without interpolated values - added HibernateGormStaticApiSpec "Test GString query warning is emitted once without interpolated values" using a Logback ListAppender; it runs the same GString executeQuery twice and asserts exactly one WARN, that it contains GString-interpolated query, and that it does not contain the interpolated secret. The module's test binding moved to logback-classic with a logback-test.xml mirroring the previous slf4j-simple levels so suite log output is unchanged.

@borinquenkid - Hibernate 5 / Neo4j scope: you're right that the wiring is Hibernate 7-only, so I did not over-promise it. The user-facing docs (securingAgainstAttacks.adoc, executeQuery.adoc, find.adoc) are now explicitly scoped to "when using Hibernate 7", and the misleading GString example that was mislabeled as SQL-injection-vulnerable was removed (an unquoted GString value is parameter-bound by GORM, not spliced into the HQL text). The warn-only helper itself lives in shared grails-datamapping-core (org.grails.datastore.gorm.query.GormQuerySafetyWarnings), so it can be reused by the Hibernate 5 / Neo4j static APIs later. I've intentionally left the cross-datastore + compile-time direction to your solution rather than expanding this PR's scope; happy to wire the shared helper into the other AbstractGormStaticApi paths in a follow-up if you'd prefer that over the compile-time approach.

Note for a follow-up: THREAT_MODEL.md (11 / the SQL-injection non-finding) still describes generic GString interpolation as injectable, which is now slightly inconsistent with the clarified guide wording. I left it untouched here since it's outside this PR's file set and is security-sensitive - worth a small consistency pass to distinguish a cleanly parameter-bound GString from real string concatenation/coercion.

Local verification: :grails-datamapping-core:test --tests GormQuerySafetyWarningsSpec and :grails-data-hibernate7-core:test --tests HibernateGormStaticApiSpec both pass.

@jamesfredley Understood, but I would prefer to tie down as much as possible sql injection threats one philosophical way or the other completely. This PR Warns while 15971 is a compile time error. This solution is partial while 15971 covers all bindings affected by SQL Injection. If the PR extends to H5 and neo4j then they would be comparable in range and then the committers can make an apples to apples decision.

[testlens-app]

✅ All tests passed ✅

🏷️ Commit: 2439fb3
▶️ Tests: 13657 executed
⚪️ Checks: 59/59 completed

---

Learn more about TestLens at testlens.app.

[davydotcom]

I think #15971 is a much better security approach on this one. The warning wont capture all cases and doesn't prevent it overall. The compile time transformation is a solid way to help reduce risk.

[davydotcom]

closing for now, if we want to go this way, please do open it back up. just housekeeping @jamesfredley