Copilot commented on code in PR #15971: URL: https://github.com/apache/grails-core/pull/15971#discussion_r3562998897
########## 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) { Review Comment: `isUnsafeGStringCoercion` only treats a *literal* `GStringExpression` (or a cast/`toString()` of a literal) as the source of an unsafe flattening. That misses common variants where the interpolated GString is first stored in a local and only later coerced to `String`, e.g. `def g = "... ${x} ..."; String q = g; Book.executeQuery(q)` or `String q = g.toString()`. In those cases the RHS is a `VariableExpression`, so the assignment is not tracked and the call site won’t be flagged, leaving a real injection vector undetected. -- 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]
