This is an automated email from the ASF dual-hosted git repository. borinquenkid pushed a commit to branch feat/gorm-query-safety-ast-check in repository https://gitbox.apache.org/repos/asf/grails-core.git
commit 4aa9a0b51d2f784d1b0182c7848bad4348003b42 Author: Walter Duque de Estrada <[email protected]> AuthorDate: Fri Jul 10 21:52:04 2026 -0500 Add compile-time check for GORM query strings flattened from GString A GString passed directly to a GORM query method (find/findAll/executeQuery/ executeUpdate, and Neo4j's Cypher equivalents) is safe: GORM binds each interpolated value as a query parameter. But once that GString is assigned to a String-typed local, coerced via .toString(), or cast (all ordinary Groovy patterns), the interpolated value becomes raw, unescaped text with no trace of ever having been a GString - undetectable at the query-execution boundary because a String carries no metadata about its origin. Add a global AST transformation (grails-datamapping-core) that catches this at compile time instead, across every GORM implementation that shares this method-naming convention (Hibernate5, Hibernate7, Neo4j) without touching any of their source. Fails the build by default; a reviewed, safe call site can opt out per call with @SuppressWarnings("GormUnsafeQueryString"). Co-Authored-By: Claude Sonnet 5 <[email protected]> --- .../common/compiler/GroovyTransformOrder.groovy | 8 +- .../GlobalGormQuerySafetyASTTransformation.java | 57 ++++ .../transform/GormQuerySafetyTransformer.java | 291 +++++++++++++++++++++ ...org.codehaus.groovy.transform.ASTTransformation | 1 + .../GormQuerySafetyTransformerSpec.groovy | 227 ++++++++++++++++ .../en/guide/security/securingAgainstAttacks.adoc | 30 +++ .../src/en/guide/upgrading/upgrading80x.adoc | 19 ++ 7 files changed, 632 insertions(+), 1 deletion(-) diff --git a/grails-common/src/main/groovy/org/apache/grails/common/compiler/GroovyTransformOrder.groovy b/grails-common/src/main/groovy/org/apache/grails/common/compiler/GroovyTransformOrder.groovy index 6b6cddc7aa..8f0fd59e88 100644 --- a/grails-common/src/main/groovy/org/apache/grails/common/compiler/GroovyTransformOrder.groovy +++ b/grails-common/src/main/groovy/org/apache/grails/common/compiler/GroovyTransformOrder.groovy @@ -97,10 +97,16 @@ interface GroovyTransformOrder { */ static final int FINDER_ORDER = WHERE_ORDER + DECREMENT_PRIORITY + /** + * Detects GORM query strings that were GString-interpolated but coerced to a plain String + * before reaching a query method, losing GORM's automatic parameter binding + */ + static final int QUERY_SAFETY_ORDER = FINDER_ORDER + DECREMENT_PRIORITY + /** * Grails allows applying transforms by artefact type, this transformation is what implements that */ - static final int GLOBAL_GRAILS_TRANSFORM_ORDER = FINDER_ORDER + DECREMENT_PRIORITY + static final int GLOBAL_GRAILS_TRANSFORM_ORDER = QUERY_SAFETY_ORDER + DECREMENT_PRIORITY /** * Similar to Groovy's @Delegate AST transform but instead assumes the first diff --git a/grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/query/transform/GlobalGormQuerySafetyASTTransformation.java b/grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/query/transform/GlobalGormQuerySafetyASTTransformation.java new file mode 100644 index 0000000000..077f0f3cfc --- /dev/null +++ b/grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/query/transform/GlobalGormQuerySafetyASTTransformation.java @@ -0,0 +1,57 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.grails.datastore.gorm.query.transform; + +import java.util.List; + +import org.codehaus.groovy.ast.ASTNode; +import org.codehaus.groovy.ast.ClassNode; +import org.codehaus.groovy.ast.ModuleNode; +import org.codehaus.groovy.control.CompilePhase; +import org.codehaus.groovy.control.SourceUnit; +import org.codehaus.groovy.transform.ASTTransformation; +import org.codehaus.groovy.transform.GroovyASTTransformation; +import org.codehaus.groovy.transform.TransformWithPriority; + +import org.apache.grails.common.compiler.GroovyTransformOrder; + +/** + * Global version of {@link GormQuerySafetyTransformer} - runs automatically against every class + * in every Grails application that has {@code grails-datamapping-core} on its compile classpath, + * with no developer configuration required. + * + * @since 8.1 + */ +@GroovyASTTransformation(phase = CompilePhase.CANONICALIZATION) +public class GlobalGormQuerySafetyASTTransformation implements ASTTransformation, TransformWithPriority { + + public void visit(ASTNode[] nodes, SourceUnit source) { + GormQuerySafetyTransformer transformer = new GormQuerySafetyTransformer(source); + ModuleNode ast = source.getAST(); + List<ClassNode> classes = ast.getClasses(); + for (ClassNode aClass : classes) { + transformer.visitClass(aClass); + } + } + + @Override + public int priority() { + return GroovyTransformOrder.QUERY_SAFETY_ORDER; + } +} diff --git a/grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/query/transform/GormQuerySafetyTransformer.java b/grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/query/transform/GormQuerySafetyTransformer.java new file mode 100644 index 0000000000..f290ca94a7 --- /dev/null +++ b/grails-datamapping-core/src/main/groovy/org/grails/datastore/gorm/query/transform/GormQuerySafetyTransformer.java @@ -0,0 +1,291 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.grails.datastore.gorm.query.transform; + +import java.util.Arrays; +import java.util.Collections; +import java.util.HashMap; +import java.util.HashSet; +import java.util.List; +import java.util.Map; +import java.util.Set; + +import org.codehaus.groovy.ast.ASTNode; +import org.codehaus.groovy.ast.AnnotatedNode; +import org.codehaus.groovy.ast.AnnotationNode; +import org.codehaus.groovy.ast.ClassCodeVisitorSupport; +import org.codehaus.groovy.ast.ClassHelper; +import org.codehaus.groovy.ast.ClassNode; +import org.codehaus.groovy.ast.MethodNode; +import org.codehaus.groovy.ast.expr.ArgumentListExpression; +import org.codehaus.groovy.ast.expr.BinaryExpression; +import org.codehaus.groovy.ast.expr.CastExpression; +import org.codehaus.groovy.ast.expr.ClassExpression; +import org.codehaus.groovy.ast.expr.ConstantExpression; +import org.codehaus.groovy.ast.expr.DeclarationExpression; +import org.codehaus.groovy.ast.expr.Expression; +import org.codehaus.groovy.ast.expr.GStringExpression; +import org.codehaus.groovy.ast.expr.ListExpression; +import org.codehaus.groovy.ast.expr.MethodCallExpression; +import org.codehaus.groovy.ast.expr.StaticMethodCallExpression; +import org.codehaus.groovy.ast.expr.VariableExpression; +import org.codehaus.groovy.control.SourceUnit; +import org.codehaus.groovy.syntax.Types; + +import org.grails.datastore.mapping.reflect.AstUtils; + +/** + * {@link ClassCodeVisitorSupport} that detects GORM HQL/Cypher query text built from a + * {@link GStringExpression} that Groovy coerced to a plain {@code String} <em>before</em> it + * reaches a GORM query method, e.g.: + * + * <pre>{@code + * String query = "from Book where name = ${userInput}" // coerced to String right here + * Book.executeQuery(query) // -> raw, unescaped text, no binding + * }</pre> + * + * <p>When a {@link groovy.lang.GString} is passed directly to a GORM query method, GORM binds + * each interpolated value as a query parameter — safe. Once the {@code GString} has been coerced + * to a {@code String} (an explicit {@code String}-typed local, a {@code .toString()} call, or an + * {@code as String}/cast coercion), that information is gone: a {@code String} carries no trace + * of ever having been a {@code GString}, so this can only be caught here, before the coercion + * erases it — a runtime check at the query boundary is structurally blind to this case. + * + * <p><strong>Known limitations (deliberate v1 scope):</strong> + * <ul> + * <li>Intraprocedural only — a flattened {@code String} built inside a helper method and + * returned to the caller is invisible to this check.</li> + * <li>Reassignment tracking is last-write-wins, not full branch-sensitive dataflow.</li> + * <li>Only local variables are tracked, not fields.</li> + * <li>Does not detect plain string concatenation with no {@code GString} involved at all, raw + * JDBC via {@code groovy.sql.Sql}, or any datastore whose query methods use names outside + * {@link #CANDIDATE_METHODS}.</li> + * </ul> + * + * @since 8.1 + */ +public class GormQuerySafetyTransformer extends ClassCodeVisitorSupport { + + /** + * The {@code @SuppressWarnings} value that silences this check on the enclosing method (or, + * for calls outside any method, the enclosing class). + */ + public static final String SUPPRESS_WARNINGS_VALUE = "GormUnsafeQueryString"; + + private static final Set<String> CANDIDATE_METHODS = new HashSet<>(Arrays.asList( + "find", "findAll", "executeQuery", "executeUpdate", + "findAllWithSql", "cypherStatic", "findPath", "findPathTo")); + + /** + * The positional index of the query argument for each candidate method. Every candidate + * method takes the query as its first argument except Neo4j's + * {@code findPathTo(Class type, CharSequence query, Map params)}. + */ + private static final Map<String, Integer> QUERY_ARGUMENT_INDEX = buildQueryArgumentIndex(); + + private static Map<String, Integer> buildQueryArgumentIndex() { + Map<String, Integer> indexes = new HashMap<>(); + for (String method : CANDIDATE_METHODS) { + indexes.put(method, 0); + } + indexes.put("findPathTo", 1); + return Collections.unmodifiableMap(indexes); + } + + private final SourceUnit sourceUnit; + private final Map<String, ASTNode> flattenedStringVars = new HashMap<>(); + private ClassNode currentClassNode; + private MethodNode currentMethodNode; + + public GormQuerySafetyTransformer(SourceUnit sourceUnit) { + this.sourceUnit = sourceUnit; + } + + @Override + protected SourceUnit getSourceUnit() { + return this.sourceUnit; + } + + @Override + public void visitClass(ClassNode node) { + try { + this.currentClassNode = node; + super.visitClass(node); + } finally { + this.currentClassNode = null; + this.flattenedStringVars.clear(); + } + } + + @Override + public void visitMethod(MethodNode node) { + this.currentMethodNode = node; + try { + super.visitMethod(node); + } finally { + this.currentMethodNode = null; + this.flattenedStringVars.clear(); + } + } + + @Override + public void visitDeclarationExpression(DeclarationExpression expression) { + // getVariableExpression() is null for multiple-assignment declarations, e.g. def (a, b) = [...] + VariableExpression variableExpression = expression.isMultipleAssignmentDeclaration() ? + null : expression.getVariableExpression(); + if (variableExpression != null && + isUnsafeGStringCoercion(expression.getRightExpression(), variableExpression.getType())) { + flattenedStringVars.put(variableExpression.getName(), expression); + } + super.visitDeclarationExpression(expression); + } + + @Override + public void visitBinaryExpression(BinaryExpression expression) { + if (expression.getOperation().getType() == Types.ASSIGN && + expression.getLeftExpression() instanceof VariableExpression) { + VariableExpression leftVariable = (VariableExpression) expression.getLeftExpression(); + String variableName = leftVariable.getName(); + if (isUnsafeGStringCoercion(expression.getRightExpression(), leftVariable.getType())) { + flattenedStringVars.put(variableName, expression); + } else { + // Any other (safe) reassignment clears prior unsafe tracking - last write wins, + // not full branch-sensitive dataflow. See class Javadoc. + flattenedStringVars.remove(variableName); + } + } + super.visitBinaryExpression(expression); + } + + /** + * True when {@code expression} is the moment a {@code GString} becomes an ordinary + * {@code String}: either a bare interpolated {@link GStringExpression} assigned to a + * {@code String}-typed variable ({@code declaredType}), or an explicit + * {@code .toString()}/cast/{@code as String} coercion of one (which loses the binding + * regardless of what the result is then declared as). + */ + private boolean isUnsafeGStringCoercion(Expression expression, ClassNode declaredType) { + if (isInterpolatedGString(expression)) { + return ClassHelper.STRING_TYPE.equals(declaredType); + } + if (expression instanceof CastExpression) { + CastExpression cast = (CastExpression) expression; + return ClassHelper.STRING_TYPE.equals(cast.getType()) && isInterpolatedGString(cast.getExpression()); + } + if (expression instanceof MethodCallExpression) { + MethodCallExpression call = (MethodCallExpression) expression; + return "toString".equals(call.getMethodAsString()) && isInterpolatedGString(call.getObjectExpression()); + } + return false; + } + + private boolean isInterpolatedGString(Expression expression) { + return expression instanceof GStringExpression && !((GStringExpression) expression).getValues().isEmpty(); + } + + @Override + public void visitMethodCallExpression(MethodCallExpression call) { + String methodName = call.getMethodAsString(); + if (methodName != null && CANDIDATE_METHODS.contains(methodName) && + isFlattenedQueryArgument(methodName, call.getArguments()) && + isGormReceiver(call.getObjectExpression()) && + !isSuppressed()) { + reportUnsafeQuery(call, methodName); + } + super.visitMethodCallExpression(call); + } + + @Override + public void visitStaticMethodCallExpression(StaticMethodCallExpression call) { + String methodName = call.getMethod(); + if (CANDIDATE_METHODS.contains(methodName) && + isFlattenedQueryArgument(methodName, call.getArguments()) && + AstUtils.isDomainClass(call.getOwnerType()) && + !isSuppressed()) { + reportUnsafeQuery(call, methodName); + } + super.visitStaticMethodCallExpression(call); + } + + private boolean isGormReceiver(Expression objectExpression) { + if (objectExpression instanceof ClassExpression) { + return AstUtils.isDomainClass(((ClassExpression) objectExpression).getType()); + } + if (objectExpression instanceof VariableExpression && ((VariableExpression) objectExpression).isThisExpression()) { + return currentClassNode != null && AstUtils.isDomainClass(currentClassNode); + } + return false; + } + + private boolean isFlattenedQueryArgument(String methodName, Expression arguments) { + if (!(arguments instanceof ArgumentListExpression)) { + return false; + } + List<Expression> args = ((ArgumentListExpression) arguments).getExpressions(); + int index = QUERY_ARGUMENT_INDEX.get(methodName); + if (args.size() <= index || !(args.get(index) instanceof VariableExpression)) { + return false; + } + String variableName = ((VariableExpression) args.get(index)).getName(); + return flattenedStringVars.containsKey(variableName); + } + + private boolean isSuppressed() { + if (currentMethodNode != null && isSuppressedNode(currentMethodNode)) { + return true; + } + return currentClassNode != null && isSuppressedNode(currentClassNode); + } + + private boolean isSuppressedNode(AnnotatedNode node) { + for (AnnotationNode annotation : node.getAnnotations(ClassHelper.make(SuppressWarnings.class))) { + Expression value = annotation.getMember("value"); + if (containsSuppressionValue(value)) { + return true; + } + } + return false; + } + + private boolean containsSuppressionValue(Expression value) { + if (value instanceof ConstantExpression) { + return SUPPRESS_WARNINGS_VALUE.equals(((ConstantExpression) value).getValue()); + } + if (value instanceof ListExpression) { + for (Expression element : ((ListExpression) value).getExpressions()) { + if (containsSuppressionValue(element)) { + return true; + } + } + } + return false; + } + + private void reportUnsafeQuery(ASTNode node, String methodName) { + String message = "[GORM] The query string passed to '" + methodName + "' was built from a " + + "GString that Groovy already coerced to a plain String, so any interpolated " + + "values are now embedded as raw, unescaped text - this is a query injection " + + "risk. Keep the value as a GString when calling '" + methodName + "' (GORM turns " + + "GString interpolations into bound query parameters automatically), or pass " + + "named/positional parameters explicitly. To suppress this check for a reviewed, " + + "safe call site, add @SuppressWarnings(\"" + SUPPRESS_WARNINGS_VALUE + "\") to the " + + "enclosing method."; + sourceUnit.getErrorCollector().addErrorAndContinue(message, node, sourceUnit); + } +} diff --git a/grails-datamapping-core/src/main/resources/META-INF/services/org.codehaus.groovy.transform.ASTTransformation b/grails-datamapping-core/src/main/resources/META-INF/services/org.codehaus.groovy.transform.ASTTransformation index 7b1e2316fa..529b30e68a 100644 --- a/grails-datamapping-core/src/main/resources/META-INF/services/org.codehaus.groovy.transform.ASTTransformation +++ b/grails-datamapping-core/src/main/resources/META-INF/services/org.codehaus.groovy.transform.ASTTransformation @@ -1,2 +1,3 @@ org.grails.datastore.gorm.query.transform.GlobalDetachedCriteriaASTTransformation org.grails.compiler.gorm.GlobalJpaEntityTransform +org.grails.datastore.gorm.query.transform.GlobalGormQuerySafetyASTTransformation diff --git a/grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/query/transform/GormQuerySafetyTransformerSpec.groovy b/grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/query/transform/GormQuerySafetyTransformerSpec.groovy new file mode 100644 index 0000000000..33b5d79772 --- /dev/null +++ b/grails-datamapping-core/src/test/groovy/org/grails/datastore/gorm/query/transform/GormQuerySafetyTransformerSpec.groovy @@ -0,0 +1,227 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one + * or more contributor license agreements. See the NOTICE file + * distributed with this work for additional information + * regarding copyright ownership. The ASF licenses this file + * to you under the Apache License, Version 2.0 (the + * "License"); you may not use this file except in compliance + * with the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ +package org.grails.datastore.gorm.query.transform + +import org.codehaus.groovy.control.MultipleCompilationErrorsException + +import spock.lang.Specification +import spock.lang.Unroll + +class GormQuerySafetyTransformerSpec extends Specification { + + void "test String query flattened from an interpolated GString before executeQuery fails to compile"() { + when: + new GroovyClassLoader().parseClass(''' +import grails.gorm.annotation.Entity + +@Entity +class Book { + String title + + static List byTitle(String title) { + String q = "from Book where title = ${title}" + executeQuery(q) + } +} +''') + + then: + def e = thrown(MultipleCompilationErrorsException) + e.message.contains('GormUnsafeQueryString') + e.message.contains("passed to 'executeQuery'") + } + + @Unroll + void "test flattening via #description before find fails to compile"() { + when: + new GroovyClassLoader().parseClass(""" +import grails.gorm.annotation.Entity + +@Entity +class Book { + String title + + static Book byTitle(String title) { + $declaration + find(q) + } +} +""") + + then: + def e = thrown(MultipleCompilationErrorsException) + e.message.contains('GormUnsafeQueryString') + + where: + description | declaration + '.toString()' | 'def q = "from Book where title = ${title}".toString()' + 'as String' | 'String q = ("from Book where title = ${title}" as String)' + '(String) cast' | 'String q = (String) "from Book where title = ${title}"' + 'reassignment' | 'String q; q = "from Book where title = ${title}"' + } + + void "test a GString literal passed directly compiles cleanly"() { + when: + Class<?> bookClass = new GroovyClassLoader().parseClass(''' +import grails.gorm.annotation.Entity + +@Entity +class Book { + String title + + static List byTitle(String title) { + executeQuery("from Book where title = ${title}") + } +} +''') + + then: + bookClass != null + } + + void "test a plain non-interpolated String compiles cleanly"() { + when: + Class<?> bookClass = new GroovyClassLoader().parseClass(''' +import grails.gorm.annotation.Entity + +@Entity +class Book { + String title + + static List all() { + String q = "from Book" + executeQuery(q) + } +} +''') + + then: + bookClass != null + } + + void "test a flattened query suppressed with @SuppressWarnings compiles cleanly"() { + when: + Class<?> bookClass = new GroovyClassLoader().parseClass(''' +import grails.gorm.annotation.Entity + +@Entity +class Book { + String title + + @SuppressWarnings("GormUnsafeQueryString") + static List byTitle(String title) { + String q = "from Book where title = ${title}" + executeQuery(q) + } +} +''') + + then: + bookClass != null + } + + void "test a non-domain class with its own find/findAll(String) methods does not false-positive"() { + when: + Class<?> planClass = new GroovyClassLoader().parseClass(''' +class Plan { + String find(String query) { + return query + } + + List findAll(String query) { + return [query] + } + + static String byName(String name) { + String q = "plan ${name}" + new Plan().find(q) + } +} +''') + + then: + planClass != null + } + + void "test Collection.find/.findAll with a closure argument does not false-positive"() { + when: + Class<?> bookClass = new GroovyClassLoader().parseClass(''' +import grails.gorm.annotation.Entity + +@Entity +class Book { + String title + + static Object pickOne(List titles, String target) { + String q = "picking ${target}" + titles.find { it == q } + titles.findAll { it == q } + } +} +''') + + then: + bookClass != null + } + + void "test findPathTo detects a flattened query at argument index 1, not 0"() { + when: + new GroovyClassLoader().parseClass(''' +import grails.gorm.annotation.Entity + +@Entity +class Book { + String title + + static Object toOther(Class other, String title) { + String q = "MATCH (b:Book)-[*]->(o) WHERE b.title = ${title} RETURN o" + findPathTo(other, q, [:]) + } +} +''') + + then: + def e = thrown(MultipleCompilationErrorsException) + e.message.contains('GormUnsafeQueryString') + e.message.contains("passed to 'findPathTo'") + } + + void "test a flattened variable referenced inside a nested closure still fails to compile"() { + when: + new GroovyClassLoader().parseClass(''' +import grails.gorm.annotation.Entity + +@Entity +class Book { + String title + + static void byTitles(List titles) { + titles.each { String title -> + String q = "from Book where title = ${title}" + executeQuery(q) + } + } +} +''') + + then: + def e = thrown(MultipleCompilationErrorsException) + e.message.contains('GormUnsafeQueryString') + } +} diff --git a/grails-doc/src/en/guide/security/securingAgainstAttacks.adoc b/grails-doc/src/en/guide/security/securingAgainstAttacks.adoc index b9aa593d2c..545c846d08 100644 --- a/grails-doc/src/en/guide/security/securingAgainstAttacks.adoc +++ b/grails-doc/src/en/guide/security/securingAgainstAttacks.adoc @@ -18,6 +18,7 @@ under the License. //// +[[sqlInjection]] ==== SQL injection @@ -59,6 +60,35 @@ def safe() { } ---- +===== Compile-time detection of flattened GString queries + +Passing a `GString` *directly* to a GORM query method such as `find`, `findAll`, `executeQuery`, or `executeUpdate` is safe: GORM converts each interpolated value into a bound query parameter rather than embedding it as text. The risk is a subtler pattern that looks harmless: + +[source,groovy] +---- +def vulnerable() { + String query = "from Book as b where b.title = ${params.title}" // <1> + def books = Book.executeQuery(query) // <2> +} +---- +<1> Groovy coerces the `GString` to a plain `String` right here, because `query` is declared as `String`. +<2> By the time `executeQuery` receives it, it is an ordinary `String` with no trace of ever having been a `GString` — the interpolated value is now raw, unescaped text, and this call is genuinely vulnerable to HQL injection. + +This is dangerous precisely because it can't be caught at runtime: once a `GString` is flattened to a `String`, there is nothing left to distinguish it from HQL text that was always safe. Grails detects this pattern at *compile time* instead, and fails the build with an error identifying the call site. To fix it, keep the value as a `GString` (recommended) or pass named/positional parameters explicitly, as shown above. + +If a flagged call site has been reviewed and is genuinely safe, suppress the check on the enclosing method: + +[source,groovy] +---- +@SuppressWarnings("GormUnsafeQueryString") +def reviewedAndSafe() { + String query = "from Book as b where b.title = ${params.title}" + def books = Book.executeQuery(query) +} +---- + +This check runs automatically against every Grails application that has `grails-datamapping-core` on its compile classpath — no configuration is required, and it covers every GORM implementation whose query methods follow this convention (Hibernate and Neo4j today). + ==== Phishing diff --git a/grails-doc/src/en/guide/upgrading/upgrading80x.adoc b/grails-doc/src/en/guide/upgrading/upgrading80x.adoc index eb80fee140..eb4b757dcb 100644 --- a/grails-doc/src/en/guide/upgrading/upgrading80x.adoc +++ b/grails-doc/src/en/guide/upgrading/upgrading80x.adoc @@ -1513,4 +1513,23 @@ Both hooks may be used on the same plugin during migration; when a registrar and **Bean-definition overriding edge case.** Grails defaults `spring.main.allow-bean-definition-overriding` to `true`, and under that default nothing changes: an application bean (from the application class, `resources.groovy` or `resources.xml`) that uses the same name as a plugin bean still replaces the plugin's bean. However, because plugin beans now register earlier and application beans register in a separate, later step, an application that explicitly sets `spring.main.allow-bean-defi [...] + +==== 34. Compile-Time GORM Query Safety Check + +Grails 8 adds a compile-time check that can **break existing builds**: passing a GORM query method (`find`, `findAll`, `executeQuery`, `executeUpdate`, and the Neo4j Cypher equivalents) a `String`-typed variable that was built from an interpolated `GString` is now a compile error, not just a discouraged pattern. + +[source,groovy] +---- +def vulnerable() { + String query = "from Book as b where b.title = ${params.title}" // fails to compile + def books = Book.executeQuery(query) +} +---- + +This is flagged because passing the `GString` *directly* to `executeQuery` is safe — GORM binds the interpolated value as a query parameter — but assigning it to a `String`-typed local first causes Groovy to coerce it to a plain `String` at that point, silently discarding the safe binding; the interpolated value is then embedded as raw, unescaped query text. See <<sqlInjection,SQL injection>> for the full explanation and examples. + +If your application has this pattern, fix it by keeping the query a `GString` all the way to the call, or by using named/positional parameters instead. For a call site that has been reviewed and is genuinely safe, add `@SuppressWarnings("GormUnsafeQueryString")` to the enclosing method rather than restructuring the code. + +This check runs automatically wherever `grails-datamapping-core` is on the compile classpath — no opt-in configuration is required or possible beyond the per-call-site suppression above. + **Most applications and plugins need no action.** Behavior only changes where a plugin bean and a conditional Boot bean competed for the same name or type — the plugin bean now wins, which is almost always the intended outcome.
