sbglasius commented on PR #16463:
URL: https://github.com/apache/grails-core/pull/16463#issuecomment-5938011231
**Constant-text tracking is last-write-wins outside `if`/`else`, which
creates a new false negative**
Only `visitIfElse` merges state per branch. In `try`/`catch`, `switch` and
loops the AST is walked in source order, so a data assignment followed by a
constant one leaves the variable classified as constant text:
```groovy
String frag = ""
try { frag = sort } catch (Exception ex) { frag = "" }
String q = "from Book order by ${frag}"
executeQuery(q) // not flagged: frag is in constantTextVars
```
Before this PR any GString with an interpolation assigned to a `String` was
a build error, so this is a regression rather than the already-documented
limitation (that limitation only used to hide unsafe-to-safe transitions for
the `flattened` state, not the direct-interpolation check).
`GormQuerySafetyTransformer.isConstantTextVariable` ends in
`constantTextVars.contains(...)`.
I found this by reading the code and have not run the spec below; the
`try/catch` and `switch` rows should currently fail with "Expected exception
... but no exception was thrown", and the `if/else` row is the control that
should pass.
```groovy
@Unroll
void "test variable assigned from data on one path of #construct is not
treated as constant text"() {
when:
new GroovyClassLoader().parseClass("""
import grails.gorm.annotation.Entity
@Entity
class Book {
String title
static List sorted(String sort) {
String frag = ""
$body
String q = "from Book order by \${frag}"
executeQuery(q)
}
}
""")
then:
def e = thrown(MultipleCompilationErrorsException)
e.message.contains('GormUnsafeQueryString')
e.message.contains("passed to 'executeQuery'")
where:
construct | body
'if/else' | 'if (sort) { frag = sort } else { frag = " title" }'
'try/catch' | 'try { frag = sort } catch (Exception ex) { frag = "" }'
'switch' | 'switch (sort) { case "a": frag = sort; break; default:
frag = " title" }'
}
```
A use-before-write loop (`frag` interpolated, then `frag = sort` later in
the same loop body) has the same problem and is worth a row too.
Possible fix: treat any assignment inside a `try`, `switch` or loop as
making the variable non-constant for the rest of the method, or override those
visitors to merge pessimistically like `visitIfElse` does.
--
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]
