Fixes issue where multiple mixed type injections were only fixed partially (#434)

Author: andrecsilva

core-codemods/src/test/java/io/codemodder/codemods/SonarSQLInjectionCodemodTest.java

@@ -33,4 +33,13 @@ class SupportedTest implements CodemodTestMixin {}
       expectingFixesAtLines = {19, 25, 33, 40},
       dependencies = {})
   class SupportedTableInjectionTest implements CodemodTestMixin {}
+
+  @Nested
+  @Metadata(
+      codemodType = SonarSQLInjectionCodemod.class,
+      testResourceDir = "sonar-sql-injection-s2077/supportedMixedInjections",
+      renameTestFile = "core-codemods/src/main/java/io/codemodder/codemods/SQLTestMixed.java",
+      expectingFixesAtLines = {21},
+      dependencies = {})
+  class SupportedMixedInjectionTest implements CodemodTestMixin {}
 }

core-codemods/src/test/resources/sonar-sql-injection-s2077/supportedMixedInjections/SQLTestMixed.java.after

@@ -0,0 +1,32 @@
+package io.codemodder.codemods;
+
+import java.sql.Connection;
+import java.sql.ResultSet;
+import java.sql.SQLException;
+import java.sql.Statement;
+import java.sql.PreparedStatement;
+import java.util.Scanner;
+import java.util.regex.Pattern;
+
+public final class SQLTestMixed {
+
+    private Connection conn;
+
+    public ResultSet simpleIndirect() throws SQLException {
+        Scanner scanner = new Scanner(System.in);
+        String input = scanner.nextLine();
+        String sql = "SELECT * FROM " + validateTableName(input + "") + " where name=?" ;
+        PreparedStatement stmt = conn.prepareStatement(sql);
+        stmt.setString(1, scanner.nextLine());
+        return stmt.execute();
+    }
+    
+    String validateTableName(final String tablename) {
+        Pattern regex = Pattern.compile("[a-zA-Z0-9_]+(.[a-zA-Z0-9_]+)?");
+        if (!regex.matcher(tablename).matches()) {
+            throw new SecurityException("Supplied table name contains non-alphanumeric characters");
+        }
+        return tablename;
+    }
+
+}

core-codemods/src/test/resources/sonar-sql-injection-s2077/supportedMixedInjections/SQLTestMixed.java.before

@@ -0,0 +1,24 @@
+package io.codemodder.codemods;
+
+import java.sql.Connection;
+import java.sql.ResultSet;
+import java.sql.SQLException;
+import java.sql.Statement;
+import java.sql.PreparedStatement;
+import java.util.Scanner;
+import java.util.regex.Pattern;
+
+public final class SQLTestMixed {
+
+    private Connection conn;
+
+    public ResultSet simpleIndirect() throws SQLException {
+        Scanner scanner = new Scanner(System.in);
+        String input = scanner.nextLine();
+        String input2 = scanner.nextLine();
+        String sql = "SELECT * FROM " + input + " where name='" + input2 + "'" ;
+        Statement stmt = conn.createStatement();
+        return stmt.executeQuery(sql);
+    }
+
+}

core-codemods/src/test/resources/sonar-sql-injection-s2077/supportedMixedInjections/sonar-hotspots.json

@@ -0,0 +1,48 @@
+{
+  "paging": {
+    "pageIndex": 1,
+    "pageSize": 100,
+    "total": 1
+  },
+  "hotspots": [
+    {
+      "key": "AZEIpASKc7kK4RXktkeh",
+      "component": "pixee_codemodder-java:core-codemods/src/main/java/io/codemodder/codemods/SQLTestMixed.java",
+      "project": "pixee_codemodder-java",
+      "securityCategory": "sql-injection",
+      "vulnerabilityProbability": "HIGH",
+      "status": "TO_REVIEW",
+      "line": 21,
+      "message": "Make sure using a dynamically formatted SQL query is safe here.",
+      "creationDate": "2024-07-31T13:53:37+0200",
+      "updateDate": "2024-07-31T13:53:37+0200",
+      "textRange": {
+        "startLine": 21,
+        "endLine": 21,
+        "startOffset": 33,
+        "endOffset": 36
+      },
+      "flows": [],
+      "ruleKey": "java:S2077"
+    }
+  ],
+  "components": [
+    {
+      "organization": "pixee",
+      "key": "pixee_codemodder-java",
+      "qualifier": "TRK",
+      "name": "codemodder-java",
+      "longName": "codemodder-java",
+      "pullRequest": "434"
+    },
+    {
+      "organization": "pixee",
+      "key": "pixee_codemodder-java:core-codemods/src/main/java/io/codemodder/codemods/SQLTestMixed.java",
+      "qualifier": "FIL",
+      "name": "SQLTestMixed.java",
+      "longName": "core-codemods/src/main/java/io/codemodder/codemods/SQLTestMixed.java",
+      "path": "core-codemods/src/main/java/io/codemodder/codemods/SQLTestMixed.java",
+      "pullRequest": "434"
+    }
+  ]
+}

framework/codemodder-base/src/main/java/io/codemodder/ast/LinearizedStringExpression.java

@@ -6,7 +6,13 @@
 import com.github.javaparser.ast.expr.Expression;
 import com.github.javaparser.ast.expr.NameExpr;
 import com.github.javaparser.resolution.types.ResolvedType;
-import java.util.*;
+import java.util.ArrayDeque;
+import java.util.Deque;
+import java.util.IdentityHashMap;
+import java.util.List;
+import java.util.Map;
+import java.util.Objects;
+import java.util.Optional;
 import java.util.stream.Collectors;
 import java.util.stream.Stream;
 
@@ -20,7 +26,7 @@ public final class LinearizedStringExpression {
   private final Deque<Expression> linearized;
 
   public LinearizedStringExpression(final Expression expression) {
-    this.resolvedExpressions = new HashMap<>();
+    this.resolvedExpressions = new IdentityHashMap<>();
     this.root = expression;
     this.linearized = linearize(expression).collect(Collectors.toCollection(ArrayDeque::new));
   }

framework/codemodder-base/src/main/java/io/codemodder/remediation/sqlinjection/DefaultJavaParserSQLInjectionRemediatorStrategy.java

@@ -25,7 +25,7 @@
 final class DefaultJavaParserSQLInjectionRemediatorStrategy
     implements JavaParserSQLInjectionRemediatorStrategy {
 
-  private Map<Predicate<MethodCallExpr>, Predicate<MethodCallExpr>> remediationStrategies;
+  private final Map<Predicate<MethodCallExpr>, Predicate<MethodCallExpr>> remediationStrategies;
 
   /**
    * Builds a strategy from a matcher-fixer pair. A matcher is a predicate that matches the call,

framework/codemodder-base/src/main/java/io/codemodder/remediation/sqlinjection/JavaParserSQLInjectionRemediatorStrategy.java

@@ -35,9 +35,5 @@ <T> CodemodFileScanningResult remediateAll(
   /** A default implementation that should be used in all non-test scenarios. */
   JavaParserSQLInjectionRemediatorStrategy DEFAULT =
       new DefaultJavaParserSQLInjectionRemediatorStrategy(
-          Map.of(
-              SQLParameterizer::isSupportedJdbcMethodCall,
-              SQLParameterizerWithCleanup::checkAndFix,
-              SQLTableInjectionFilterTransform::matchCall,
-              SQLTableInjectionFilterTransform::fix));
+          Map.of(SQLInjectionFixComposer::match, SQLInjectionFixComposer::checkAndFix));
 }

framework/codemodder-base/src/main/java/io/codemodder/remediation/sqlinjection/SQLInjectionFixComposer.java

@@ -0,0 +1,37 @@
+package io.codemodder.remediation.sqlinjection;
+
+import com.github.javaparser.ast.expr.MethodCallExpr;
+
+/** Composes several transformations related to SQL injections. */
+public final class SQLInjectionFixComposer {
+
+  private SQLInjectionFixComposer() {}
+
+  /**
+   * Given a {@link MethodCallExpr} related to executing JDBC API SQL queries (i.e.
+   * prepareStatement(), executeQuery(), etc.), parameterize data injections or add a validation
+   * step for structural injections.
+   */
+  public static boolean checkAndFix(final MethodCallExpr methodCallExpr) {
+    // First, check if any data injection fixes apply
+    var maybeFixed = new SQLParameterizer(methodCallExpr).checkAndFix();
+    if (maybeFixed.isPresent()) {
+      // If yes, execute cleanup steps and check if any table injection remains.
+      SQLParameterizerWithCleanup.cleanup(maybeFixed.get());
+      SQLTableInjectionFilterTransform.findAndFix(maybeFixed.get());
+      return true;
+      // If not, try the table injection only
+    } else {
+      return SQLTableInjectionFilterTransform.findAndFix(methodCallExpr);
+    }
+  }
+
+  /**
+   * Check if the {@link MethodCallExpr} is a JDBC API query method that is a target of a SQL
+   * injection transformation.
+   */
+  public static boolean match(final MethodCallExpr methodCallExpr) {
+    return SQLParameterizer.isSupportedJdbcMethodCall(methodCallExpr)
+        || SQLTableInjectionFilterTransform.matchCall(methodCallExpr);
+  }
+}

framework/codemodder-base/src/main/java/io/codemodder/remediation/sqlinjection/SQLParameterizerWithCleanup.java

@@ -10,21 +10,19 @@ private SQLParameterizerWithCleanup() {}
 
   public static boolean checkAndFix(final MethodCallExpr methodCallExpr) {
     var maybeFixed = new SQLParameterizer(methodCallExpr).checkAndFix();
-    if (maybeFixed.isPresent()) {
-      // Cleanup
-      var maybeMethodDecl = methodCallExpr.findAncestor(CallableDeclaration.class);
+    maybeFixed.ifPresent(call -> cleanup(call));
+    return maybeFixed.isPresent();
+  }
+
+  public static void cleanup(final MethodCallExpr pstmtCall) {
+    var maybeMethodDecl = pstmtCall.findAncestor(CallableDeclaration.class);
 
-      // Remove concatenation with empty strings e.g "first" +  "" -> "first";
-      maybeMethodDecl.ifPresent(ASTTransforms::removeEmptyStringConcatenation);
-      // Remove potential unused variables left after transform
-      maybeMethodDecl.ifPresent(md -> ASTTransforms.removeUnusedLocalVariables(md));
+    // Remove concatenation with empty strings e.g "first" +  "" -> "first";
+    maybeMethodDecl.ifPresent(ASTTransforms::removeEmptyStringConcatenation);
+    // Remove potential unused variables left after transform
+    maybeMethodDecl.ifPresent(md -> ASTTransforms.removeUnusedLocalVariables(md));
 
-      // Merge concatenated literals, e.g. "first" + " and second" -> "first and second"
-      maybeFixed
-          .flatMap(mce -> mce.getArguments().getFirst())
-          .ifPresent(ASTTransforms::mergeConcatenatedLiterals);
-      return true;
-    }
-    return false;
+    // Merge concatenated literals, e.g. "first" + " and second" -> "first and second"
+    pstmtCall.getArguments().getFirst().ifPresent(ASTTransforms::mergeConcatenatedLiterals);
   }
 }