Add more tests and remove codemod we're not ready for (#459)

nahsra · web-flow · commit a64d95c0f883 · 2024-10-18T14:52:24.000Z
* Moves remediators that were for some reason in core-codemods to the
framework
* Removes a codemod we're not ready to do yet (CodeQL reports
vulnerabilities at odds locations in comparison to other tools)
* Made tests harder to pass
* Styling cleanup
diff --git a/core-codemods/src/intTest/java/io/codemodder/integration/WebGoat20238Test.java b/core-codemods/src/intTest/java/io/codemodder/integration/WebGoat20238Test.java
@@ -68,10 +68,10 @@ void it_remediates_webgoat_2023_8() throws Exception {
     assertThat(jwtChange.getChanges().get(0).getLineNumber(), equalTo(113));
     assertThat(jwtChange.getChanges().get(1).getLineNumber(), equalTo(140));
 
-    verifyCodemodsHitWithChangesetCount(report, "codeql:java/insecure-randomness", 0);
     verifyCodemodsHitWithChangesetCount(report, "codeql:java/ssrf", 1);
     verifyCodemodsHitWithChangesetCount(report, "codeql:java/xxe", 1);
     verifyCodemodsHitWithChangesetCount(report, "codeql:java/sql-injection", 6);
     verifyCodemodsHitWithChangesetCount(report, "codeql:java/insecure-cookie", 2);
+    verifyCodemodsHitWithChangesetCount(report, "codeql:java/unsafe-deserialization", 2);
   }
 }
diff --git a/core-codemods/src/intTest/java/io/codemodder/integration/WebGoat822Test.java b/core-codemods/src/intTest/java/io/codemodder/integration/WebGoat822Test.java
@@ -171,7 +171,6 @@ void it_transforms_webgoat_with_codeql() throws Exception {
     assertThat(ajaxJwtChange.getChanges().size(), equalTo(1));
     assertThat(ajaxJwtChange.getChanges().get(0).getLineNumber(), equalTo(53));
 
-    verifyCodemodsHitWithChangesetCount(report, "codeql:java/insecure-randomness", 0);
     verifyCodemodsHitWithChangesetCount(report, "codeql:java/ssrf", 3);
     verifyCodemodsHitWithChangesetCount(report, "codeql:java/sql-injection", 5);
     verifyCodemodsHitWithChangesetCount(report, "codeql:java/insecure-cookie", 1);
diff --git a/core-codemods/src/main/java/io/codemodder/codemods/DefaultCodemods.java b/core-codemods/src/main/java/io/codemodder/codemods/DefaultCodemods.java
@@ -30,12 +30,12 @@ public static List<Class<? extends CodeChanger>> asList() {
         CodeQLHttpResponseSplittingCodemod.class,
         CodeQLInputResourceLeakCodemod.class,
         CodeQLInsecureCookieCodemod.class,
-        CodeQLInsecureRandomnessCodemod.class,
         CodeQLJDBCResourceLeakCodemod.class,
         CodeQLJEXLInjectionCodemod.class,
         CodeQLJNDIInjectionCodemod.class,
         CodeQLMavenSecureURLCodemod.class,
         CodeQLOutputResourceLeakCodemod.class,
+        CodeQLPredictableSeedCodemod.class,
         CodeQLSQLInjectionCodemod.class,
         CodeQLSSRFCodemod.class,
         CodeQLStackTraceExposureCodemod.class,
diff --git a/core-codemods/src/main/java/io/codemodder/codemods/SonarSSRFCodemod.java b/core-codemods/src/main/java/io/codemodder/codemods/SonarSSRFCodemod.java
@@ -2,12 +2,12 @@
 
 import com.github.javaparser.ast.CompilationUnit;
 import io.codemodder.*;
-import io.codemodder.codemods.remediators.ssrf.SSRFRemediator;
 import io.codemodder.codetf.DetectorRule;
 import io.codemodder.providers.sonar.ProvidedSonarScan;
 import io.codemodder.providers.sonar.RuleIssue;
 import io.codemodder.providers.sonar.SonarRemediatingJavaParserChanger;
 import io.codemodder.remediation.GenericRemediationMetadata;
+import io.codemodder.remediation.ssrf.SSRFRemediator;
 import io.codemodder.sonar.model.Issue;
 import io.codemodder.sonar.model.SonarFinding;
 import java.util.List;
diff --git a/core-codemods/src/main/java/io/codemodder/codemods/codeql/CodeQLInsecureRandomnessCodemod.java b/core-codemods/src/main/java/io/codemodder/codemods/codeql/CodeQLInsecureRandomnessCodemod.java
diff --git a/core-codemods/src/main/java/io/codemodder/codemods/codeql/CodeQLPredictableSeedCodemod.java b/core-codemods/src/main/java/io/codemodder/codemods/codeql/CodeQLPredictableSeedCodemod.java
@@ -0,0 +1,56 @@
+package io.codemodder.codemods.codeql;
+
+import com.contrastsecurity.sarif.Result;
+import com.github.javaparser.ast.CompilationUnit;
+import io.codemodder.*;
+import io.codemodder.codetf.DetectorRule;
+import io.codemodder.providers.sarif.codeql.ProvidedCodeQLScan;
+import io.codemodder.remediation.GenericRemediationMetadata;
+import io.codemodder.remediation.Remediator;
+import io.codemodder.remediation.predictableseed.PredictableSeedRemediator;
+import java.util.Optional;
+import javax.inject.Inject;
+
+/** A codemod for automatically fixing predictable seeds reported by CodeQL. */
+@Codemod(
+    id = "codeql:java/predictable-seed",
+    reviewGuidance = ReviewGuidance.MERGE_WITHOUT_REVIEW,
+    importance = Importance.MEDIUM,
+    executionPriority = CodemodExecutionPriority.HIGH)
+public final class CodeQLPredictableSeedCodemod extends CodeQLRemediationCodemod {
+
+  private final Remediator<Result> remediator;
+
+  @Inject
+  public CodeQLPredictableSeedCodemod(
+      @ProvidedCodeQLScan(ruleId = "java/predictable-seed") final RuleSarif sarif) {
+    super(GenericRemediationMetadata.PREDICTABLE_SEED.reporter(), sarif);
+    this.remediator = new PredictableSeedRemediator<>();
+  }
+
+  @Override
+  public DetectorRule detectorRule() {
+    return new DetectorRule(
+        "predictable-seed",
+        "Use of a predictable seed in a secure random number generator",
+        "https://codeql.github.com/codeql-query-help/java/java-predictable-seed/");
+  }
+
+  @Override
+  public CodemodFileScanningResult visit(
+      final CodemodInvocationContext context, final CompilationUnit cu) {
+    return remediator.remediateAll(
+        cu,
+        context.path().toString(),
+        detectorRule(),
+        ruleSarif.getResultsByLocationPath(context.path()),
+        SarifFindingKeyUtil::buildFindingId,
+        result -> result.getLocations().get(0).getPhysicalLocation().getRegion().getStartLine(),
+        result ->
+            Optional.ofNullable(
+                result.getLocations().get(0).getPhysicalLocation().getRegion().getEndLine()),
+        result ->
+            Optional.ofNullable(
+                result.getLocations().get(0).getPhysicalLocation().getRegion().getStartColumn()));
+  }
+}
diff --git a/core-codemods/src/main/java/io/codemodder/codemods/codeql/CodeQLSSRFCodemod.java b/core-codemods/src/main/java/io/codemodder/codemods/codeql/CodeQLSSRFCodemod.java
@@ -2,10 +2,10 @@
 
 import com.github.javaparser.ast.CompilationUnit;
 import io.codemodder.*;
-import io.codemodder.codemods.remediators.ssrf.SSRFRemediator;
 import io.codemodder.codetf.DetectorRule;
 import io.codemodder.providers.sarif.codeql.ProvidedCodeQLScan;
 import io.codemodder.remediation.GenericRemediationMetadata;
+import io.codemodder.remediation.ssrf.SSRFRemediator;
 import javax.inject.Inject;
 
 /** A codemod for automatically fixing SQL injection from CodeQL. */
diff --git a/core-codemods/src/main/java/io/codemodder/codemods/semgrep/SemgrepSSRFCodemod.java b/core-codemods/src/main/java/io/codemodder/codemods/semgrep/SemgrepSSRFCodemod.java
@@ -9,10 +9,10 @@
 import io.codemodder.ReviewGuidance;
 import io.codemodder.RuleSarif;
 import io.codemodder.SarifFindingKeyUtil;
-import io.codemodder.codemods.remediators.ssrf.SSRFRemediator;
 import io.codemodder.codetf.DetectorRule;
 import io.codemodder.providers.sarif.semgrep.ProvidedSemgrepScan;
 import io.codemodder.remediation.GenericRemediationMetadata;
+import io.codemodder.remediation.ssrf.SSRFRemediator;
 import javax.inject.Inject;
 
 /**
diff --git a/core-codemods/src/main/java/io/codemodder/codemods/semgrep/SemgrepWeakRandomCodemod.java b/core-codemods/src/main/java/io/codemodder/codemods/semgrep/SemgrepWeakRandomCodemod.java
@@ -9,9 +9,9 @@
 import io.codemodder.ReviewGuidance;
 import io.codemodder.RuleSarif;
 import io.codemodder.SarifFindingKeyUtil;
-import io.codemodder.codemods.remediators.weakrandom.WeakRandomRemediator;
 import io.codemodder.codetf.DetectorRule;
 import io.codemodder.providers.sarif.semgrep.ProvidedSemgrepScan;
+import io.codemodder.remediation.weakrandom.WeakRandomRemediator;
 import javax.inject.Inject;
 
 /**
diff --git a/framework/codemodder-base/src/main/java/io/codemodder/CodemodFileScanningResult.java b/framework/codemodder-base/src/main/java/io/codemodder/CodemodFileScanningResult.java
@@ -30,7 +30,7 @@ public List<UnfixedFinding> unfixedFindings() {
   static CodemodFileScanningResult from(
       final List<CodemodChange> changes,
       final List<UnfixedFinding> unfixedFindings,
-      CodeTFAiMetadata codeTFAiMetadata) {
+      final CodeTFAiMetadata codeTFAiMetadata) {
     return new AICodemodFileScanningResult(changes, unfixedFindings, codeTFAiMetadata);
   }
 
diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/GenericRemediationMetadata.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/GenericRemediationMetadata.java
@@ -14,7 +14,9 @@ public enum GenericRemediationMetadata {
   MISSING_SECURE_FLAG("missing-secure-flag"),
   SQL_INJECTION("sql-injection"),
   SSRF("ssrf"),
-  WEAK_RANDOM("weak-random");
+  WEAK_RANDOM("weak-random"),
+  PREDICTABLE_SEED("predictable-seed"),
+  ZIP_SLIP("zip-slip");
 
   private final CodemodReporterStrategy reporter;
 
diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/Remediator.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/Remediator.java
@@ -14,6 +14,7 @@
  */
 public interface Remediator<T> {
 
+  /** Performs the work of performing all remediation, and returning reporting metadata. */
   CodemodFileScanningResult remediateAll(
       final CompilationUnit cu,
       final String path,
@@ -22,5 +23,5 @@ CodemodFileScanningResult remediateAll(
       final Function<T, String> findingIdExtractor,
       final Function<T, Integer> findingStartLineExtractor,
       final Function<T, Optional<Integer>> findingEndLineExtractor,
-      final Function<T, Optional<Integer>> findingColumnExtractor);
+      final Function<T, Optional<Integer>> findingStartColumnExtractor);
 }
diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/SearcherStrategyRemediator.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/SearcherStrategyRemediator.java
@@ -135,7 +135,7 @@ public CodemodFileScanningResult remediateAll(
       final Function<T, String> findingIdExtractor,
       final Function<T, Integer> findingStartLineExtractor,
       final Function<T, Optional<Integer>> findingEndLineExtractor,
-      final Function<T, Optional<Integer>> findingColumnExtractor) {
+      final Function<T, Optional<Integer>> findingStartColumnExtractor) {
 
     List<CodemodChange> allChanges = new ArrayList<>();
     List<UnfixedFinding> allUnfixed = new ArrayList<>();
@@ -150,7 +150,7 @@ public CodemodFileScanningResult remediateAll(
               findingIdExtractor,
               findingStartLineExtractor,
               findingEndLineExtractor,
-              findingColumnExtractor,
+              findingStartColumnExtractor,
               searcherAndStrategy.getKey(),
               searcherAndStrategy.getValue());
       allChanges.addAll(pairResult.getValue0());
diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/jndiinjection/JNDIInjectionRemediator.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/jndiinjection/JNDIInjectionRemediator.java
@@ -9,7 +9,8 @@
 import java.util.Optional;
 import java.util.function.Function;
 
-public class JNDIInjectionRemediator<T> implements Remediator<T> {
+/** Remediates JNDI injection vulnerabilities. */
+public final class JNDIInjectionRemediator<T> implements Remediator<T> {
 
   private final SearcherStrategyRemediator<T> searchStrategyRemediator;
 
@@ -51,7 +52,7 @@ public CodemodFileScanningResult remediateAll(
       Function<T, String> findingIdExtractor,
       Function<T, Integer> findingStartLineExtractor,
       Function<T, Optional<Integer>> findingEndLineExtractor,
-      Function<T, Optional<Integer>> findingColumnExtractor) {
+      Function<T, Optional<Integer>> findingStartColumnExtractor) {
     return searchStrategyRemediator.remediateAll(
         cu,
         path,
@@ -60,6 +61,6 @@ public CodemodFileScanningResult remediateAll(
         findingIdExtractor,
         findingStartLineExtractor,
         findingEndLineExtractor,
-        findingColumnExtractor);
+        findingStartColumnExtractor);
   }
 }
diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/predictableseed/PredictableSeedRemediator.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/predictableseed/PredictableSeedRemediator.java
@@ -0,0 +1,80 @@
+package io.codemodder.remediation.predictableseed;
+
+import com.github.javaparser.ast.CompilationUnit;
+import com.github.javaparser.ast.NodeList;
+import com.github.javaparser.ast.expr.Expression;
+import com.github.javaparser.ast.expr.LiteralExpr;
+import com.github.javaparser.ast.expr.MethodCallExpr;
+import com.github.javaparser.ast.expr.NameExpr;
+import io.codemodder.CodemodChange;
+import io.codemodder.CodemodFileScanningResult;
+import io.codemodder.codetf.DetectorRule;
+import io.codemodder.codetf.FixedFinding;
+import io.codemodder.remediation.*;
+import java.util.ArrayList;
+import java.util.Collection;
+import java.util.List;
+import java.util.Optional;
+import java.util.function.Function;
+
+/** Remediator for predictable seed weaknesses. */
+public final class PredictableSeedRemediator<T> implements Remediator<T> {
+
+  @Override
+  public CodemodFileScanningResult remediateAll(
+      final CompilationUnit cu,
+      final String path,
+      final DetectorRule detectorRule,
+      final Collection<T> findingsForPath,
+      final Function<T, String> findingIdExtractor,
+      final Function<T, Integer> findingStartLineExtractor,
+      final Function<T, Optional<Integer>> findingEndLineExtractor,
+      final Function<T, Optional<Integer>> findingStartColumnExtractor) {
+
+    FixCandidateSearcher<T> searcher =
+        new FixCandidateSearcher.Builder<T>()
+            .withMatcher(
+                n ->
+                    Optional.of(n)
+                        .map(MethodOrConstructor::new)
+                        .filter(mce -> mce.isMethodCallWithName("setSeed"))
+                        .filter(mce -> mce.asNode().hasScope())
+                        .filter(mce -> mce.getArguments().size() == 1)
+                        // technically, we don't need this, just to prevent a silly tool from
+                        // reporting on hardcoded data
+                        .filter(mce -> !(mce.getArguments().get(0) instanceof LiteralExpr))
+                        .isPresent())
+            .build();
+
+    FixCandidateSearchResults<T> candidateSearchResults =
+        searcher.search(
+            cu,
+            path,
+            detectorRule,
+            List.copyOf(findingsForPath),
+            findingIdExtractor,
+            findingStartLineExtractor,
+            findingEndLineExtractor,
+            findingStartColumnExtractor);
+
+    List<CodemodChange> changes = new ArrayList<>();
+    for (FixCandidate<T> fixCandidate : candidateSearchResults.fixCandidates()) {
+      MethodCallExpr setSeedCall = (MethodCallExpr) fixCandidate.node();
+      MethodCallExpr safeExpression =
+          new MethodCallExpr(new NameExpr(System.class.getSimpleName()), "currentTimeMillis");
+      NodeList<Expression> arguments = setSeedCall.getArguments();
+      arguments.set(0, safeExpression);
+
+      // should only ever be one, but just in case
+      List<FixedFinding> fixedFindings =
+          fixCandidate.issues().stream()
+              .map(issue -> new FixedFinding(findingIdExtractor.apply(issue), detectorRule))
+              .toList();
+
+      int reportedLine = setSeedCall.getRange().get().begin.line;
+      changes.add(CodemodChange.from(reportedLine, List.of(), fixedFindings));
+    }
+
+    return CodemodFileScanningResult.from(changes, candidateSearchResults.unfixableFindings());
+  }
+}
diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/sqlinjection/SQLInjectionRemediator.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/sqlinjection/SQLInjectionRemediator.java
@@ -36,7 +36,7 @@ public CodemodFileScanningResult remediateAll(
       Function<T, String> findingIdExtractor,
       Function<T, Integer> findingStartLineExtractor,
       Function<T, Optional<Integer>> findingEndLineExtractor,
-      Function<T, Optional<Integer>> findingColumnExtractor) {
+      Function<T, Optional<Integer>> findingStartColumnExtractor) {
     return searchStrategyRemediator.remediateAll(
         cu,
         path,
@@ -45,6 +45,6 @@ public CodemodFileScanningResult remediateAll(
         findingIdExtractor,
         findingStartLineExtractor,
         findingEndLineExtractor,
-        findingColumnExtractor);
+        findingStartColumnExtractor);
   }
 }
diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/ssrf/DefaultSSRFRemediator.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/ssrf/DefaultSSRFRemediator.java
@@ -1,4 +1,4 @@
-package io.codemodder.codemods.remediators.ssrf;
+package io.codemodder.remediation.ssrf;
 
 import static io.codemodder.ast.ASTTransforms.addImportIfMissing;
 
diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/ssrf/SSRFRemediator.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/ssrf/SSRFRemediator.java
@@ -1,4 +1,4 @@
-package io.codemodder.codemods.remediators.ssrf;
+package io.codemodder.remediation.ssrf;
 
 import com.github.javaparser.ast.CompilationUnit;
 import io.codemodder.CodemodFileScanningResult;
diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/weakrandom/DefaultWeakRandomRemediator.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/weakrandom/DefaultWeakRandomRemediator.java
@@ -1,4 +1,4 @@
-package io.codemodder.codemods.remediators.weakrandom;
+package io.codemodder.remediation.weakrandom;
 
 import static io.codemodder.ast.ASTTransforms.addImportIfMissing;
 
diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/weakrandom/WeakRandomRemediator.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/weakrandom/WeakRandomRemediator.java
@@ -1,4 +1,4 @@
-package io.codemodder.codemods.remediators.weakrandom;
+package io.codemodder.remediation.weakrandom;
 
 import com.github.javaparser.ast.CompilationUnit;
 import io.codemodder.CodemodFileScanningResult;
diff --git a/framework/codemodder-base/src/main/resources/generic-remediation-reports/predictable-seed/description.md b/framework/codemodder-base/src/main/resources/generic-remediation-reports/predictable-seed/description.md
diff --git a/framework/codemodder-base/src/main/resources/generic-remediation-reports/predictable-seed/report.json b/framework/codemodder-base/src/main/resources/generic-remediation-reports/predictable-seed/report.json
diff --git a/framework/codemodder-base/src/main/resources/generic-remediation-reports/zip-slip/description.md b/framework/codemodder-base/src/main/resources/generic-remediation-reports/zip-slip/description.md
diff --git a/framework/codemodder-base/src/main/resources/generic-remediation-reports/zip-slip/report.json b/framework/codemodder-base/src/main/resources/generic-remediation-reports/zip-slip/report.json
diff --git a/plugins/codemodder-plugin-llm/src/main/java/io/codemodder/plugins/llm/LLMDiffs.java b/plugins/codemodder-plugin-llm/src/main/java/io/codemodder/plugins/llm/LLMDiffs.java
diff --git a/plugins/codemodder-plugin-llm/src/main/java/io/codemodder/plugins/llm/SarifPluginLLMCodemod.java b/plugins/codemodder-plugin-llm/src/main/java/io/codemodder/plugins/llm/SarifPluginLLMCodemod.java

Original file line number	Diff line number	Diff line change
`@@ -68,10 +68,10 @@ void it_remediates_webgoat_2023_8() throws Exception {`
`68`	`68`	`assertThat(jwtChange.getChanges().get(0).getLineNumber(), equalTo(113));`
`69`	`69`	`assertThat(jwtChange.getChanges().get(1).getLineNumber(), equalTo(140));`
`70`	`70`
`71`		`- verifyCodemodsHitWithChangesetCount(report, "codeql:java/insecure-randomness", 0);`
`72`	`71`	`verifyCodemodsHitWithChangesetCount(report, "codeql:java/ssrf", 1);`
`73`	`72`	`verifyCodemodsHitWithChangesetCount(report, "codeql:java/xxe", 1);`
`74`	`73`	`verifyCodemodsHitWithChangesetCount(report, "codeql:java/sql-injection", 6);`
`75`	`74`	`verifyCodemodsHitWithChangesetCount(report, "codeql:java/insecure-cookie", 2);`
	`75`	`+ verifyCodemodsHitWithChangesetCount(report, "codeql:java/unsafe-deserialization", 2);`
`76`	`76`	`}`
`77`	`77`	`}`
Original file line number	Diff line number	Diff line change
`@@ -30,7 +30,7 @@ public List<UnfixedFinding> unfixedFindings() {`
`30`	`30`	`static CodemodFileScanningResult from(`
`31`	`31`	`final List<CodemodChange> changes,`
`32`	`32`	`final List<UnfixedFinding> unfixedFindings,`
`33`		`- CodeTFAiMetadata codeTFAiMetadata) {`
	`33`	`+ final CodeTFAiMetadata codeTFAiMetadata) {`
`34`	`34`	`return new AICodemodFileScanningResult(changes, unfixedFindings, codeTFAiMetadata);`
`35`	`35`	`}`
`36`	`36`