matrei opened a new issue, #16481:
URL: https://github.com/apache/grails-core/issues/16481
## Summary
#16463 taught the `GormUnsafeQueryString` check to tell constant query text
from values. A local that only ever holds constant text may be interpolated
into a query that is coerced to a `String` without a finding. Three problems
remain on `8.0.x`:
1. **Some locals holding a value are still treated as constant text.** The
check assumes every assignment to a local is a statement it walks in the order
it runs, and it tracks locals by name. Each case below compiles without an
error, although the interpolated local holds a parameter value at the query.
Before #16463, every one of them failed the build.
2. **Compile time grows exponentially with nesting.** Every loop and closure
body is walked at least twice, inside each walk of its enclosing body. The
check runs on every class compiled with `grails-datamapping-core` on the
classpath, so this affects any deeply nested code, not just code with queries.
3. **The guide and the error message describe constructs the check cannot
see.** The guide says query text joined from a `List` or returned from a helper
method "is reported" as a warning or an error. Assigned directly, it is not
reported at all. The error message recommends a `StringBuilder`, which the
check cannot follow.
## Reproduce
Each `$body` below goes into this class. Expected: the build fails with
`GormUnsafeQueryString`. Actual on `8.0.x`: it compiles cleanly.
```groovy
@Entity
class Book {
String title
static void sorted(String sort, List<String> sorts) {
String frag = ' title'
$body
}
}
```
| Construct | `$body` |
|---|---|
| Multiple assignment | `def other; (frag, other) = [sort, 1]; String q =
"from Book order by ${frag}"; executeQuery(q)` |
| Assignment in a ternary | `sort ? (frag = sort) : (frag = ' id'); String q
= "from Book order by ${frag}"; executeQuery(q)` |
| Assignment in `\|\|` | `frag = sort; sort.isEmpty() \|\| (frag = ' id');
String q = "from Book order by ${frag}"; executeQuery(q)` |
| Closure called later | `def setter = { frag = sort }; frag = ' id';
setter(); String q = "from Book order by ${frag}"; executeQuery(q)` |
| `do`/`while` condition | `do { String q = "from Book order by ${frag}";
executeQuery(q) } while ((frag = sorts.remove(0)) != null)` |
| Loop variable reusing a name | `try { String part = ' id' } finally { };
for (String part in sorts) { String q = "from Book order by ${part}";
executeQuery(q) }` |
| Closure parameter reusing a name | `if (sort) { String part = ' id' } else
{ String part = ' title' }; sorts.each { String part -> String q = "from Book
order by ${part}"; executeQuery(q) }` |
Time to compile a class with `depth` nested `list.each { ... }` closures,
one run each:
| depth | 8.0.x | check disabled (`-DprotectSqlInjectionAttacks=false`) |
before #16463 |
|---|---|---|---|
| 16 | 370-444 ms | 27 ms | 51 ms |
| 20 | 1415-1552 ms | 38 ms | 57 ms |
| 22 | 4621 ms | 30 ms | - |
The guide's claim can be checked by assigning a joined `List` or a helper
method's result to `String q` and passing `q` straight to `executeQuery`.
Neither produces an error or a warning, and neither does a `StringBuilder` that
a value was appended to.
--
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]
