Refactors SQL Remediator and Codemods & HQL Transformation Bugfix (#456)

Refactors SQL injection codemods to use the new remediator API and fixes a HQL transformation bug.
pixee · Oct 15, 2024 · 364702d · 364702d
1 parent 4b58536
commit 364702d
Show file tree

Hide file tree

Showing 12 changed files with 199 additions and 252 deletions.
diff --git a/core-codemods/src/main/java/io/codemodder/codemods/DefectDojoSqlInjectionCodemod.java b/core-codemods/src/main/java/io/codemodder/codemods/DefectDojoSqlInjectionCodemod.java
@@ -7,9 +7,11 @@
 import io.codemodder.providers.defectdojo.DefectDojoScan;
 import io.codemodder.providers.defectdojo.Finding;
 import io.codemodder.providers.defectdojo.RuleFindings;
-import io.codemodder.remediation.sqlinjection.JavaParserSQLInjectionRemediatorStrategy;
+import io.codemodder.remediation.Remediator;
+import io.codemodder.remediation.sqlinjection.SQLInjectionRemediator;
 import java.util.List;
 import java.util.Objects;
+import java.util.Optional;
 import javax.inject.Inject;
 
 /**
@@ -25,15 +27,15 @@ public final class DefectDojoSqlInjectionCodemod extends JavaParserChanger
     implements FixOnlyCodeChanger {
 
   private final RuleFindings findings;
-  private final JavaParserSQLInjectionRemediatorStrategy remediatorStrategy;
+  private final Remediator<Finding> remediatorStrategy;
 
   @Inject
   public DefectDojoSqlInjectionCodemod(
       @DefectDojoScan(ruleId = "java.lang.security.audit.sqli.jdbc-sqli.jdbc-sqli")
           RuleFindings findings) {
     super(CodemodReporterStrategy.fromClasspath(SQLParameterizerCodemod.class));
     this.findings = Objects.requireNonNull(findings);
-    this.remediatorStrategy = JavaParserSQLInjectionRemediatorStrategy.DEFAULT;
+    this.remediatorStrategy = new SQLInjectionRemediator<>();
   }
 
   @Override
@@ -60,6 +62,7 @@ public CodemodFileScanningResult visit(
         findingsForThisPath,
         finding -> String.valueOf(finding.getId()),
         Finding::getLine,
-        f -> null);
+        f -> Optional.empty(),
+        f -> Optional.empty());
   }
 }
diff --git a/core-codemods/src/main/java/io/codemodder/codemods/HQLParameterizationCodemod.java b/core-codemods/src/main/java/io/codemodder/codemods/HQLParameterizationCodemod.java
@@ -56,7 +56,7 @@ public CodemodFileScanningResult visit(
     return CodemodFileScanningResult.withOnlyChanges(changes);
   }
 
-  private static final String queryParameterNamePrefix = ":parameter";
+  private static final String queryParameterNamePrefix = "parameter";
 
   private boolean isQueryCreation(final MethodCallExpr methodCallExpr) {
     final Predicate<MethodCallExpr> isQueryCall =
@@ -86,7 +86,8 @@ private List<Expression> fixInjections(
       final var builder = new StringBuilder(startString);
       final int lastQuoteIndex = startString.lastIndexOf('\'') + 1;
       final var prepend = startString.substring(lastQuoteIndex);
-      builder.replace(lastQuoteIndex - 1, startString.length(), queryParameterNamePrefix + count);
+      builder.replace(
+          lastQuoteIndex - 1, startString.length(), ":" + queryParameterNamePrefix + count);
       start.asStringLiteralExpr().setValue(builder.toString());
 
       // fix end

diff --git a/core-codemods/src/main/java/io/codemodder/codemods/SonarSQLInjectionCodemod.java b/core-codemods/src/main/java/io/codemodder/codemods/SonarSQLInjectionCodemod.java
@@ -7,11 +7,14 @@
 import io.codemodder.providers.sonar.RuleHotspot;
 import io.codemodder.providers.sonar.SonarRemediatingJavaParserChanger;
 import io.codemodder.remediation.GenericRemediationMetadata;
-import io.codemodder.remediation.sqlinjection.JavaParserSQLInjectionRemediatorStrategy;
+import io.codemodder.remediation.Remediator;
+import io.codemodder.remediation.sqlinjection.SQLInjectionRemediator;
 import io.codemodder.sonar.model.Hotspot;
 import io.codemodder.sonar.model.SonarFinding;
+import io.codemodder.sonar.model.TextRange;
 import java.util.List;
 import java.util.Objects;
+import java.util.Optional;
 import javax.inject.Inject;
 
 @Codemod(
@@ -21,15 +24,15 @@
     executionPriority = CodemodExecutionPriority.HIGH)
 public final class SonarSQLInjectionCodemod extends SonarRemediatingJavaParserChanger {
 
-  private final JavaParserSQLInjectionRemediatorStrategy remediationStrategy;
+  private final Remediator<Hotspot> remediationStrategy;
   private final RuleHotspot hotspots;
 
   @Inject
   public SonarSQLInjectionCodemod(
       @ProvidedSonarScan(ruleId = "java:S2077") final RuleHotspot hotspots) {
     super(GenericRemediationMetadata.SQL_INJECTION.reporter(), hotspots);
     this.hotspots = Objects.requireNonNull(hotspots);
-    this.remediationStrategy = JavaParserSQLInjectionRemediatorStrategy.DEFAULT;
+    this.remediationStrategy = new SQLInjectionRemediator<>();
   }
 
   @Override
@@ -51,6 +54,7 @@ public CodemodFileScanningResult visit(
         hotspotsForFile,
         SonarFinding::getKey,
         i -> i.getTextRange() != null ? i.getTextRange().getStartLine() : i.getLine(),
-        i -> i.getTextRange() != null ? i.getTextRange().getEndLine() : null);
+        i -> Optional.ofNullable(i.getTextRange()).map(TextRange::getEndLine),
+        i -> Optional.ofNullable(i.getTextRange()).map(tr -> tr.getStartOffset() + 1));
   }
 }
diff --git a/core-codemods/src/main/java/io/codemodder/codemods/semgrep/SemgrepSQLInjectionCodemod.java b/core-codemods/src/main/java/io/codemodder/codemods/semgrep/SemgrepSQLInjectionCodemod.java
@@ -1,5 +1,6 @@
 package io.codemodder.codemods.semgrep;
 
+import com.contrastsecurity.sarif.Result;
 import com.github.javaparser.ast.CompilationUnit;
 import io.codemodder.Codemod;
 import io.codemodder.CodemodExecutionPriority;
@@ -12,7 +13,9 @@
 import io.codemodder.codetf.DetectorRule;
 import io.codemodder.providers.sarif.semgrep.ProvidedSemgrepScan;
 import io.codemodder.remediation.GenericRemediationMetadata;
-import io.codemodder.remediation.sqlinjection.JavaParserSQLInjectionRemediatorStrategy;
+import io.codemodder.remediation.Remediator;
+import io.codemodder.remediation.sqlinjection.SQLInjectionRemediator;
+import java.util.Optional;
 import javax.inject.Inject;
 
 /**
@@ -26,14 +29,14 @@
     importance = Importance.HIGH)
 public final class SemgrepSQLInjectionCodemod extends SemgrepJavaParserChanger {
 
-  private final JavaParserSQLInjectionRemediatorStrategy remediator;
+  private final Remediator<Result> remediator;
 
   @Inject
   public SemgrepSQLInjectionCodemod(
       @ProvidedSemgrepScan(ruleId = "java.lang.security.audit.sqli.jdbc-sqli.jdbc-sqli")
           final RuleSarif sarif) {
     super(GenericRemediationMetadata.SQL_INJECTION.reporter(), sarif);
-    this.remediator = JavaParserSQLInjectionRemediatorStrategy.DEFAULT;
+    this.remediator = new SQLInjectionRemediator<>();
   }
 
   @Override
@@ -54,6 +57,11 @@ public CodemodFileScanningResult visit(
         ruleSarif.getResultsByLocationPath(context.path()),
         SarifFindingKeyUtil::buildFindingId,
         r -> r.getLocations().get(0).getPhysicalLocation().getRegion().getStartLine(),
-        r -> r.getLocations().get(0).getPhysicalLocation().getRegion().getEndLine());
+        r ->
+            Optional.ofNullable(
+                r.getLocations().get(0).getPhysicalLocation().getRegion().getEndLine()),
+        r ->
+            Optional.ofNullable(
+                r.getLocations().get(0).getPhysicalLocation().getRegion().getStartColumn()));
   }
 }
diff --git a/...ain/java/io/codemodder/codemods/semgrep/SemgrepSQLInjectionFormattedSqlStringCodemod.java b/...ain/java/io/codemodder/codemods/semgrep/SemgrepSQLInjectionFormattedSqlStringCodemod.java
@@ -1,5 +1,6 @@
 package io.codemodder.codemods.semgrep;
 
+import com.contrastsecurity.sarif.Result;
 import com.github.javaparser.ast.CompilationUnit;
 import io.codemodder.Codemod;
 import io.codemodder.CodemodExecutionPriority;
@@ -12,7 +13,9 @@
 import io.codemodder.codetf.DetectorRule;
 import io.codemodder.providers.sarif.semgrep.ProvidedSemgrepScan;
 import io.codemodder.remediation.GenericRemediationMetadata;
-import io.codemodder.remediation.sqlinjection.JavaParserSQLInjectionRemediatorStrategy;
+import io.codemodder.remediation.Remediator;
+import io.codemodder.remediation.sqlinjection.SQLInjectionRemediator;
+import java.util.Optional;
 import javax.inject.Inject;
 
 /**
@@ -26,15 +29,15 @@
     importance = Importance.HIGH)
 public final class SemgrepSQLInjectionFormattedSqlStringCodemod extends SemgrepJavaParserChanger {
 
-  private final JavaParserSQLInjectionRemediatorStrategy remediator;
+  private final Remediator<Result> remediator;
 
   @Inject
   public SemgrepSQLInjectionFormattedSqlStringCodemod(
       @ProvidedSemgrepScan(
               ruleId = "java.lang.security.audit.formatted-sql-string.formatted-sql-string")
           final RuleSarif sarif) {
     super(GenericRemediationMetadata.SQL_INJECTION.reporter(), sarif);
-    this.remediator = JavaParserSQLInjectionRemediatorStrategy.DEFAULT;
+    this.remediator = new SQLInjectionRemediator<>();
   }
 
   @Override
@@ -55,6 +58,11 @@ public CodemodFileScanningResult visit(
         ruleSarif.getResultsByLocationPath(context.path()),
         SarifFindingKeyUtil::buildFindingId,
         r -> r.getLocations().get(0).getPhysicalLocation().getRegion().getStartLine(),
-        r -> r.getLocations().get(0).getPhysicalLocation().getRegion().getEndLine());
+        r ->
+            Optional.ofNullable(
+                r.getLocations().get(0).getPhysicalLocation().getRegion().getEndLine()),
+        r ->
+            Optional.ofNullable(
+                r.getLocations().get(0).getPhysicalLocation().getRegion().getStartColumn()));
   }
 }
diff --git a/core-codemods/src/test/resources/hql-parameterizer/Test.java.after b/core-codemods/src/test/resources/hql-parameterizer/Test.java.after
@@ -8,22 +8,22 @@ public final class Test {
     private Session session;
 
     void direct(String tainted) {
-      Query<User> hqlQuery = session.createQuery("select p from Person p where p.name like :parameter0").setParameter(":parameter0", tainted);
+      Query<User> hqlQuery = session.createQuery("select p from Person p where p.name like :parameter0").setParameter("parameter0", tainted);
     }
 
     void indirect(String tainted) {
       String query = "select p from Person p where p.name like :parameter0";
-      Query<User> hqlQuery = session.createQuery(query).setParameter(":parameter0", tainted);
+      Query<User> hqlQuery = session.createQuery(query).setParameter("parameter0", tainted);
     }
 
     void indirectMultiple(String tainted, String tainted2) {
       String query = "select p from Person p where p.name like :parameter0" + " and p.surname like :parameter1";
-      Query<User> hqlQuery = session.createQuery(query).setParameter(":parameter0", tainted).setParameter(":parameter1", tainted2);
+      Query<User> hqlQuery = session.createQuery(query).setParameter("parameter0", tainted).setParameter("parameter1", tainted2);
     }
 
     void indirectMultipeString(String tainted, String tainted2) {
       String second = " and p.surname like :parameter1";
       String first = "select p from Person p where p.name like :parameter0" + second;
-      Query<User> hqlQuery = session.createQuery(first).setParameter(":parameter0", tainted).setParameter(":parameter1", tainted2);
+      Query<User> hqlQuery = session.createQuery(first).setParameter("parameter0", tainted).setParameter("parameter1", tainted2);
     }
 }
diff --git a/.../codemodder/remediation/sqlinjection/DefaultJavaParserSQLInjectionRemediatorStrategy.java b/.../codemodder/remediation/sqlinjection/DefaultJavaParserSQLInjectionRemediatorStrategy.java