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