Converted remaining remediators to new API and bugfixes (#462)

pixee · Nov 1, 2024 · d1bfcc9 · d1bfcc9
1 parent 0a7867c
commit d1bfcc9
Show file tree

Hide file tree

Showing 64 changed files with 1,476 additions and 1,501 deletions.
diff --git a/core-codemods/src/main/java/io/codemodder/codemods/SonarXXECodemod.java b/core-codemods/src/main/java/io/codemodder/codemods/SonarXXECodemod.java
@@ -7,11 +7,14 @@
 import io.codemodder.providers.sonar.RuleIssue;
 import io.codemodder.providers.sonar.SonarRemediatingJavaParserChanger;
 import io.codemodder.remediation.GenericRemediationMetadata;
+import io.codemodder.remediation.Remediator;
+import io.codemodder.remediation.WithoutScopePositionMatcher;
 import io.codemodder.remediation.xxe.XXERemediator;
 import io.codemodder.sonar.model.Issue;
 import io.codemodder.sonar.model.SonarFinding;
 import java.util.List;
 import java.util.Objects;
+import java.util.Optional;
 import javax.inject.Inject;
 
 @Codemod(
@@ -21,14 +24,14 @@
     executionPriority = CodemodExecutionPriority.HIGH)
 public final class SonarXXECodemod extends SonarRemediatingJavaParserChanger {
 
-  private final XXERemediator remediationStrategy;
+  private final Remediator<Issue> remediationStrategy;
   private final RuleIssue issues;
 
   @Inject
   public SonarXXECodemod(@ProvidedSonarScan(ruleId = "java:S2755") final RuleIssue issues) {
     super(GenericRemediationMetadata.XXE.reporter(), issues);
     this.issues = Objects.requireNonNull(issues);
-    this.remediationStrategy = XXERemediator.DEFAULT;
+    this.remediationStrategy = new XXERemediator<>(new WithoutScopePositionMatcher());
   }
 
   @Override
@@ -49,7 +52,14 @@ public CodemodFileScanningResult visit(
         detectorRule(),
         issuesForFile,
         SonarFinding::getKey,
-        f -> f.getTextRange() != null ? f.getTextRange().getStartLine() : f.getLine(),
-        f -> f.getTextRange().getStartOffset());
+        i -> i.getTextRange() != null ? i.getTextRange().getStartLine() : i.getLine(),
+        i ->
+            i.getTextRange() != null
+                ? Optional.of(i.getTextRange().getEndLine())
+                : Optional.empty(),
+        i ->
+            i.getTextRange() != null
+                ? Optional.of(i.getTextRange().getStartOffset() + 1)
+                : Optional.empty());
   }
 }
diff --git a/core-codemods/src/main/java/io/codemodder/codemods/codeql/CodeQLXXECodemod.java b/core-codemods/src/main/java/io/codemodder/codemods/codeql/CodeQLXXECodemod.java
@@ -1,11 +1,14 @@
 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.xxe.XXEIntermediateXMLStreamReaderRemediator;
+import java.util.Optional;
 import javax.inject.Inject;
 
 /** A codemod for automatically fixing SQL injection from CodeQL. */
@@ -16,12 +19,12 @@
     executionPriority = CodemodExecutionPriority.HIGH)
 public final class CodeQLXXECodemod extends CodeQLRemediationCodemod {
 
-  private final XXEIntermediateXMLStreamReaderRemediator remediator;
+  private final Remediator<Result> remediator;
 
   @Inject
   public CodeQLXXECodemod(@ProvidedCodeQLScan(ruleId = "java/xxe") final RuleSarif sarif) {
     super(GenericRemediationMetadata.XXE.reporter(), sarif);
-    this.remediator = XXEIntermediateXMLStreamReaderRemediator.DEFAULT;
+    this.remediator = new XXEIntermediateXMLStreamReaderRemediator<>();
   }
 
   @Override
@@ -42,7 +45,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 -> r.getLocations().get(0).getPhysicalLocation().getRegion().getStartColumn());
+        r ->
+            Optional.ofNullable(
+                r.getLocations().get(0).getPhysicalLocation().getRegion().getEndLine()),
+        r ->
+            Optional.ofNullable(
+                r.getLocations().get(0).getPhysicalLocation().getRegion().getStartColumn()));
   }
 }
diff --git a/...emods/src/main/java/io/codemodder/codemods/semgrep/SemgrepJavaDeserializationCodemod.java b/...emods/src/main/java/io/codemodder/codemods/semgrep/SemgrepJavaDeserializationCodemod.java
@@ -2,6 +2,8 @@
 
 import com.contrastsecurity.sarif.Result;
 import com.github.javaparser.ast.CompilationUnit;
+import com.github.javaparser.ast.expr.MethodCallExpr;
+import com.github.javaparser.ast.expr.VariableDeclarationExpr;
 import io.codemodder.Codemod;
 import io.codemodder.CodemodExecutionPriority;
 import io.codemodder.CodemodFileScanningResult;
@@ -12,9 +14,11 @@
 import io.codemodder.SarifFindingKeyUtil;
 import io.codemodder.codetf.DetectorRule;
 import io.codemodder.providers.sarif.semgrep.ProvidedSemgrepScan;
+import io.codemodder.remediation.FixCandidateSearcher;
 import io.codemodder.remediation.GenericRemediationMetadata;
 import io.codemodder.remediation.Remediator;
-import io.codemodder.remediation.javadeserialization.JavaDeserializationRemediator;
+import io.codemodder.remediation.SearcherStrategyRemediator;
+import io.codemodder.remediation.javadeserialization.JavaDeserializationFixStrategy;
 import java.util.Optional;
 import javax.inject.Inject;
 
@@ -37,7 +41,32 @@ public SemgrepJavaDeserializationCodemod(
               ruleId = "java.lang.security.audit.object-deserialization.object-deserialization")
           final RuleSarif sarif) {
     super(GenericRemediationMetadata.DESERIALIZATION.reporter(), sarif);
-    this.remediator = new JavaDeserializationRemediator<>();
+    this.remediator =
+        new SearcherStrategyRemediator.Builder<Result>()
+            .withSearcherStrategyPair(
+                // matches declarations
+                new FixCandidateSearcher.Builder<Result>()
+                    .withMatcher(
+                        n ->
+                            Optional.empty()
+                                .or(
+                                    () ->
+                                        Optional.of(n)
+                                            .map(
+                                                m ->
+                                                    m instanceof VariableDeclarationExpr vde
+                                                        ? vde
+                                                        : null)
+                                            .filter(JavaDeserializationFixStrategy::match))
+                                .or(
+                                    () ->
+                                        Optional.of(n)
+                                            .map(m -> m instanceof MethodCallExpr mce ? mce : null)
+                                            .filter(JavaDeserializationFixStrategy::match))
+                                .isPresent())
+                    .build(),
+                new JavaDeserializationFixStrategy())
+            .build();
   }
 
   @Override

diff --git a/.../src/main/java/io/codemodder/codemods/semgrep/SemgrepServletResponseWriterXSSCodemod.java b/.../src/main/java/io/codemodder/codemods/semgrep/SemgrepServletResponseWriterXSSCodemod.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.Remediator;
 import io.codemodder.remediation.xss.XSSRemediator;
+import java.util.Optional;
 import javax.inject.Inject;
 
 /**
@@ -26,15 +29,15 @@
     importance = Importance.MEDIUM)
 public final class SemgrepServletResponseWriterXSSCodemod extends SemgrepJavaParserChanger {
 
-  private final XSSRemediator remediator;
+  private final Remediator<Result> remediator;
 
   @Inject
   public SemgrepServletResponseWriterXSSCodemod(
       @ProvidedSemgrepScan(
               ruleId = "java.lang.security.servletresponse-writer-xss.servletresponse-writer-xss")
           final RuleSarif sarif) {
     super(GenericRemediationMetadata.XSS.reporter(), sarif);
-    this.remediator = XSSRemediator.DEFAULT;
+    this.remediator = new XSSRemediator<>();
   }
 
   @Override
@@ -48,13 +51,16 @@ public DetectorRule detectorRule() {
   @Override
   public CodemodFileScanningResult visit(
       final CodemodInvocationContext context, final CompilationUnit cu) {
-    return remediator.remediateJava(
+    return remediator.remediateAll(
         cu,
         context.path().toString(),
         detectorRule(),
         ruleSarif.getResultsByLocationPath(context.path()),
         SarifFindingKeyUtil::buildFindingId,
         r -> r.getLocations().get(0).getPhysicalLocation().getRegion().getStartLine(),
-        r -> r.getLocations().get(0).getPhysicalLocation().getRegion().getStartColumn());
+        r -> Optional.of(r.getLocations().get(0).getPhysicalLocation().getRegion().getEndLine()),
+        r ->
+            Optional.of(
+                r.getLocations().get(0).getPhysicalLocation().getRegion().getStartColumn()));
   }
 }
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
@@ -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;
@@ -11,7 +12,9 @@
 import io.codemodder.SarifFindingKeyUtil;
 import io.codemodder.codetf.DetectorRule;
 import io.codemodder.providers.sarif.semgrep.ProvidedSemgrepScan;
+import io.codemodder.remediation.Remediator;
 import io.codemodder.remediation.weakrandom.WeakRandomRemediator;
+import java.util.Optional;
 import javax.inject.Inject;
 
 /**
@@ -25,14 +28,14 @@
     importance = Importance.MEDIUM)
 public final class SemgrepWeakRandomCodemod extends SemgrepJavaParserChanger {
 
-  private final WeakRandomRemediator remediator;
+  private final Remediator<Result> remediator;
 
   @Inject
   public SemgrepWeakRandomCodemod(
       @ProvidedSemgrepScan(ruleId = "java.lang.security.audit.crypto.weak-random.weak-random")
           final RuleSarif sarif) {
     super(io.codemodder.remediation.GenericRemediationMetadata.WEAK_RANDOM.reporter(), sarif);
-    this.remediator = WeakRandomRemediator.DEFAULT;
+    this.remediator = new WeakRandomRemediator<>();
   }
 
   @Override
@@ -53,6 +56,9 @@ public CodemodFileScanningResult visit(
         ruleSarif.getResultsByLocationPath(context.path()),
         SarifFindingKeyUtil::buildFindingId,
         r -> r.getLocations().get(0).getPhysicalLocation().getRegion().getStartLine(),
-        r -> r.getLocations().get(0).getPhysicalLocation().getRegion().getStartColumn());
+        r -> Optional.of(r.getLocations().get(0).getPhysicalLocation().getRegion().getEndLine()),
+        r ->
+            Optional.of(
+                r.getLocations().get(0).getPhysicalLocation().getRegion().getStartColumn()));
   }
 }
diff --git a/core-codemods/src/main/java/io/codemodder/codemods/semgrep/SemgrepXXECodemod.java b/core-codemods/src/main/java/io/codemodder/codemods/semgrep/SemgrepXXECodemod.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;
@@ -13,7 +14,10 @@
 import io.codemodder.codetf.DetectorRule;
 import io.codemodder.providers.sarif.semgrep.ProvidedSemgrepScan;
 import io.codemodder.remediation.GenericRemediationMetadata;
+import io.codemodder.remediation.Remediator;
+import io.codemodder.remediation.WithoutScopePositionMatcher;
 import io.codemodder.remediation.xxe.XXERemediator;
+import java.util.Optional;
 import javax.inject.Inject;
 
 /** Fixes some Semgrep XXE issues. */
@@ -35,7 +39,7 @@ public SemgrepXXECodemod(
   }
 
   public static class SemgrepXXEDocumentBuilderFactoryCodemod extends SemgrepJavaParserChanger {
-    private final XXERemediator remediator;
+    private final Remediator<Result> remediator;
 
     @Inject
     public SemgrepXXEDocumentBuilderFactoryCodemod(
@@ -44,7 +48,7 @@ public SemgrepXXEDocumentBuilderFactoryCodemod(
                     "java.lang.security.audit.xxe.documentbuilderfactory-disallow-doctype-decl-missing.documentbuilderfactory-disallow-doctype-decl-missing")
             final RuleSarif sarif) {
       super(GenericRemediationMetadata.WEAK_RANDOM.reporter(), sarif);
-      this.remediator = XXERemediator.DEFAULT;
+      this.remediator = new XXERemediator<>(new WithoutScopePositionMatcher());
     }
 
     @Override
@@ -65,15 +69,14 @@ public CodemodFileScanningResult visit(
           ruleSarif.getResultsByLocationPath(context.path()),
           SarifFindingKeyUtil::buildFindingId,
           r -> r.getLocations().get(0).getPhysicalLocation().getRegion().getStartLine(),
-          // we don't pass the column because it's deceiving as the column points to beginning of
-          // statement, not call
-          r -> null);
+          r -> Optional.of(r.getLocations().get(0).getPhysicalLocation().getRegion().getEndLine()),
+          r -> Optional.empty());
     }
   }
 
   public static class SemgrepXXESaxParserFactoryCodemod extends SemgrepJavaParserChanger {
 
-    private final XXERemediator remediator;
+    private final Remediator<Result> remediator;
 
     @Inject
     public SemgrepXXESaxParserFactoryCodemod(
@@ -82,7 +85,7 @@ public SemgrepXXESaxParserFactoryCodemod(
                     "java.lang.security.audit.xxe.saxparserfactory-disallow-doctype-decl-missing.saxparserfactory-disallow-doctype-decl-missing")
             final RuleSarif sarif) {
       super(GenericRemediationMetadata.WEAK_RANDOM.reporter(), sarif);
-      this.remediator = XXERemediator.DEFAULT;
+      this.remediator = new XXERemediator<>();
     }
 
     @Override
@@ -103,9 +106,8 @@ public CodemodFileScanningResult visit(
           ruleSarif.getResultsByLocationPath(context.path()),
           SarifFindingKeyUtil::buildFindingId,
           r -> r.getLocations().get(0).getPhysicalLocation().getRegion().getStartLine(),
-          // we don't pass the column because it's deceiving as the column points to beginning of
-          // statement, not call
-          r -> null);
+          r -> Optional.of(r.getLocations().get(0).getPhysicalLocation().getRegion().getEndLine()),
+          r -> Optional.empty());
     }
   }
 }
diff --git a/core-codemods/src/test/resources/semgrep-missing-secure-flag/JWTVotesEndpoint.java.after b/core-codemods/src/test/resources/semgrep-missing-secure-flag/JWTVotesEndpoint.java.after
@@ -134,6 +134,7 @@ public class JWTVotesEndpoint extends AssignmentEndpoint {
       response.setContentType(MediaType.APPLICATION_JSON_VALUE);
     } else {
       Cookie cookie = new Cookie("access_token", "");
+      cookie.setSecure(true);
       response.addCookie(cookie);
       response.setStatus(HttpStatus.UNAUTHORIZED.value());
       response.setContentType(MediaType.APPLICATION_JSON_VALUE);

diff --git a/framework/codemodder-base/src/main/java/io/codemodder/ast/ASTs.java b/framework/codemodder-base/src/main/java/io/codemodder/ast/ASTs.java
@@ -883,6 +883,33 @@ public boolean hasNext() {
     }
   }
 
+  /**
+   * Checks if a node is a MethodCallExpr that is the initialization of a declaration with one of
+   * the types in assignedToTypes.
+   *
+   * @param node
+   * @param methodName
+   * @param assignedToTypes
+   * @return
+   */
+  public static Optional<MethodCallExpr> isInitializedToType(
+      final Node node, final String methodName, final List<String> assignedToTypes) {
+    return Optional.of(node)
+        .map(n -> n instanceof MethodCallExpr ? (MethodCallExpr) n : null)
+        .filter(m -> methodName.equals(m.getNameAsString()))
+        .filter(
+            m -> {
+              Optional<VariableDeclarator> newFactoryVariableRef =
+                  expect(m).toBeMethodCallExpression().initializingVariable().result();
+              if (newFactoryVariableRef.isEmpty()) {
+                return false;
+              }
+              String type = newFactoryVariableRef.get().getTypeAsString();
+              return assignedToTypes.contains(type)
+                  || assignedToTypes.stream().anyMatch(type::endsWith);
+            });
+  }
+
   /**
    * This finds all methods that match the given location, with the given name, and is assigned to a
    * variable of one of the given types.