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