jamesfredley commented on PR #15968: URL: https://github.com/apache/grails-core/pull/15968#issuecomment-4985547352
## 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. -- 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]
