Populate detectionTool metadata for CodeQL codemods (#356)

carlosu7 · nahsra · web-flow · commit 189133b1f2be · 2024-04-18T19:41:39.000-04:00
add finding to CodeQL codemods issue #313 --------- Co-authored-by: Arshan Dabirsiaghi <arshan.dabirsiaghi@gmail.com>
diff --git a/core-codemods/src/main/java/io/codemodder/codemods/InputResourceLeakCodemod.java b/core-codemods/src/main/java/io/codemodder/codemods/InputResourceLeakCodemod.java
@@ -4,7 +4,9 @@
 import com.github.javaparser.ast.CompilationUnit;
 import com.github.javaparser.ast.expr.Expression;
 import io.codemodder.*;
+import io.codemodder.codetf.DetectorRule;
 import io.codemodder.javaparser.ChangesResult;
+import io.codemodder.providers.sarif.codeql.CodeQLSarifJavaParserChanger;
 import io.codemodder.providers.sarif.codeql.ProvidedCodeQLScan;
 import javax.inject.Inject;
 
@@ -17,7 +19,7 @@
     reviewGuidance = ReviewGuidance.MERGE_WITHOUT_REVIEW,
     importance = Importance.MEDIUM,
     executionPriority = CodemodExecutionPriority.HIGH)
-public final class InputResourceLeakCodemod extends SarifPluginJavaParserChanger<Expression> {
+public final class InputResourceLeakCodemod extends CodeQLSarifJavaParserChanger<Expression> {
 
   @Inject
   public InputResourceLeakCodemod(
@@ -35,4 +37,12 @@ public ChangesResult onResultFound(
         ? ChangesResult.changesApplied
         : ChangesResult.noChanges;
   }
+
+  @Override
+  public DetectorRule detectorRule() {
+    return new DetectorRule(
+        "input-resource-leak",
+        "Prevent resource leaks",
+        "https://codeql.github.com/codeql-query-help/java/java-input-resource-leak/");
+  }
 }
diff --git a/core-codemods/src/main/java/io/codemodder/codemods/InsecureCookieCodemod.java b/core-codemods/src/main/java/io/codemodder/codemods/InsecureCookieCodemod.java
@@ -10,7 +10,9 @@
 import com.github.javaparser.ast.stmt.Statement;
 import io.codemodder.*;
 import io.codemodder.ast.ASTTransforms;
+import io.codemodder.codetf.DetectorRule;
 import io.codemodder.javaparser.ChangesResult;
+import io.codemodder.providers.sarif.codeql.CodeQLSarifJavaParserChanger;
 import io.codemodder.providers.sarif.codeql.ProvidedCodeQLScan;
 import java.util.Optional;
 import javax.inject.Inject;
@@ -21,7 +23,7 @@
     reviewGuidance = ReviewGuidance.MERGE_WITHOUT_REVIEW,
     importance = Importance.LOW,
     executionPriority = CodemodExecutionPriority.HIGH)
-public class InsecureCookieCodemod extends SarifPluginJavaParserChanger<MethodCallExpr> {
+public final class InsecureCookieCodemod extends CodeQLSarifJavaParserChanger<MethodCallExpr> {
 
   @Inject
   public InsecureCookieCodemod(
@@ -61,4 +63,12 @@ public ChangesResult onResultFound(
     }
     return ChangesResult.noChanges;
   }
+
+  @Override
+  public DetectorRule detectorRule() {
+    return new DetectorRule(
+        "insecure-cookie",
+        "Add secure flag to HTTP cookies",
+        "https://codeql.github.com/codeql-query-help/java/java-input-resource-leak/");
+  }
 }
diff --git a/core-codemods/src/main/java/io/codemodder/codemods/JDBCResourceLeakCodemod.java b/core-codemods/src/main/java/io/codemodder/codemods/JDBCResourceLeakCodemod.java
@@ -4,7 +4,9 @@
 import com.github.javaparser.ast.CompilationUnit;
 import com.github.javaparser.ast.expr.MethodCallExpr;
 import io.codemodder.*;
+import io.codemodder.codetf.DetectorRule;
 import io.codemodder.javaparser.ChangesResult;
+import io.codemodder.providers.sarif.codeql.CodeQLSarifJavaParserChanger;
 import io.codemodder.providers.sarif.codeql.ProvidedCodeQLScan;
 import javax.inject.Inject;
 
@@ -17,7 +19,7 @@
     reviewGuidance = ReviewGuidance.MERGE_WITHOUT_REVIEW,
     importance = Importance.MEDIUM,
     executionPriority = CodemodExecutionPriority.HIGH)
-public final class JDBCResourceLeakCodemod extends SarifPluginJavaParserChanger<MethodCallExpr> {
+public final class JDBCResourceLeakCodemod extends CodeQLSarifJavaParserChanger<MethodCallExpr> {
 
   @Inject
   public JDBCResourceLeakCodemod(
@@ -35,4 +37,12 @@ public ChangesResult onResultFound(
         ? ChangesResult.changesApplied
         : ChangesResult.noChanges;
   }
+
+  @Override
+  public DetectorRule detectorRule() {
+    return new DetectorRule(
+        "database-resource-leak",
+        "Prevent database resource leaks (CodeQL)",
+        "https://codeql.github.com/codeql-query-help/java/java-database-resource-leak/");
+  }
 }
diff --git a/core-codemods/src/main/java/io/codemodder/codemods/JEXLInjectionCodemod.java b/core-codemods/src/main/java/io/codemodder/codemods/JEXLInjectionCodemod.java
@@ -17,7 +17,9 @@
 import io.codemodder.*;
 import io.codemodder.ast.ASTTransforms;
 import io.codemodder.ast.ASTs;
+import io.codemodder.codetf.DetectorRule;
 import io.codemodder.javaparser.ChangesResult;
+import io.codemodder.providers.sarif.codeql.CodeQLSarifJavaParserChanger;
 import io.codemodder.providers.sarif.codeql.ProvidedCodeQLScan;
 import io.github.pixee.security.UnwantedTypes;
 import java.util.List;
@@ -36,7 +38,7 @@
     reviewGuidance = ReviewGuidance.MERGE_WITHOUT_REVIEW,
     importance = Importance.MEDIUM,
     executionPriority = CodemodExecutionPriority.HIGH)
-public final class JEXLInjectionCodemod extends SarifPluginJavaParserChanger<Expression> {
+public final class JEXLInjectionCodemod extends CodeQLSarifJavaParserChanger<Expression> {
 
   @Inject
   public JEXLInjectionCodemod(
@@ -175,4 +177,12 @@ private static Optional<MethodCallExpr> findJEXLBuilderCreate(final MethodCallEx
     }
     return Optional.empty();
   }
+
+  @Override
+  public DetectorRule detectorRule() {
+    return new DetectorRule(
+        "jexl-expression-injection",
+        "Expression language injection",
+        "https://codeql.github.com/codeql-query-help/java/java-jexl-expression-injection/");
+  }
 }
diff --git a/core-codemods/src/main/java/io/codemodder/codemods/MavenSecureURLCodemod.java b/core-codemods/src/main/java/io/codemodder/codemods/MavenSecureURLCodemod.java
@@ -1,7 +1,10 @@
 package io.codemodder.codemods;
 
+import com.contrastsecurity.sarif.Region;
 import com.contrastsecurity.sarif.Result;
 import io.codemodder.*;
+import io.codemodder.codetf.DetectorRule;
+import io.codemodder.codetf.FixedFinding;
 import io.codemodder.providers.sarif.codeql.ProvidedCodeQLScan;
 import java.io.IOException;
 import java.nio.file.Files;
@@ -11,7 +14,6 @@
 import java.util.Objects;
 import java.util.Optional;
 import java.util.Set;
-import java.util.stream.Collectors;
 import javax.inject.Inject;
 import javax.xml.stream.XMLEventFactory;
 import javax.xml.stream.XMLEventReader;
@@ -29,7 +31,8 @@
     reviewGuidance = ReviewGuidance.MERGE_WITHOUT_REVIEW,
     importance = Importance.MEDIUM,
     executionPriority = CodemodExecutionPriority.HIGH)
-public final class MavenSecureURLCodemod extends SarifPluginRawFileChanger {
+public final class MavenSecureURLCodemod extends SarifPluginRawFileChanger
+    implements FixOnlyCodeChanger {
 
   private final XPathStreamProcessor processor;
 
@@ -41,18 +44,31 @@ public final class MavenSecureURLCodemod extends SarifPluginRawFileChanger {
     this.processor = Objects.requireNonNull(processor);
   }
 
+  @Override
+  public String vendorName() {
+    return "CodeQL";
+  }
+
+  @Override
+  public DetectorRule detectorRule() {
+    return new DetectorRule(
+        "non-https-url",
+        "Failure to use HTTPS or SFTP URL in Maven artifact upload/download",
+        "https://codeql.github.com/codeql-query-help/java/java-maven-non-https-url");
+  }
+
   @Override
   public CodemodFileScanningResult onFileFound(
       final CodemodInvocationContext context, final List<Result> results) {
     try {
-      return processXml(context.path());
+      return processXml(context.path(), results);
     } catch (SAXException | DocumentException | IOException | XMLStreamException e) {
       LOG.error("Problem transforming xml file: {}", context.path());
       return CodemodFileScanningResult.none();
     }
   }
 
-  private CodemodFileScanningResult processXml(final Path file)
+  private CodemodFileScanningResult processXml(final Path file, final List<Result> results)
       throws SAXException, IOException, DocumentException, XMLStreamException {
     Optional<XPathStreamProcessChange> change =
         processor.process(
@@ -70,7 +86,31 @@ private CodemodFileScanningResult processXml(final Path file)
     Set<Integer> linesAffected = xmlChange.linesAffected();
 
     List<CodemodChange> allWeaves =
-        linesAffected.stream().map(CodemodChange::from).collect(Collectors.toList());
+        linesAffected.stream()
+            .map(
+                line -> {
+                  Optional<Result> matchingResult =
+                      results.stream()
+                          .filter(
+                              result -> {
+                                Region region =
+                                    result.getLocations().get(0).getPhysicalLocation().getRegion();
+                                Integer resultStartLine = region.getStartLine();
+                                Integer resultEndLine = region.getEndLine();
+                                return resultStartLine == line
+                                    || (resultStartLine <= line
+                                        && resultEndLine != null
+                                        && resultEndLine >= line);
+                              })
+                          .findFirst();
+                  if (matchingResult.isPresent()) {
+                    String id =
+                        SarifFindingKeyUtil.buildFindingId(matchingResult.get(), file, line);
+                    return CodemodChange.from(line, new FixedFinding(id, detectorRule()));
+                  }
+                  return CodemodChange.from(line);
+                })
+            .toList();
 
     // overwrite the previous web.xml with the new one
     Files.copy(xmlChange.transformedXml(), file, StandardCopyOption.REPLACE_EXISTING);
diff --git a/core-codemods/src/main/java/io/codemodder/codemods/OutputResourceLeakCodemod.java b/core-codemods/src/main/java/io/codemodder/codemods/OutputResourceLeakCodemod.java
@@ -4,7 +4,9 @@
 import com.github.javaparser.ast.CompilationUnit;
 import com.github.javaparser.ast.expr.Expression;
 import io.codemodder.*;
+import io.codemodder.codetf.DetectorRule;
 import io.codemodder.javaparser.ChangesResult;
+import io.codemodder.providers.sarif.codeql.CodeQLSarifJavaParserChanger;
 import io.codemodder.providers.sarif.codeql.ProvidedCodeQLScan;
 import javax.inject.Inject;
 
@@ -17,7 +19,7 @@
     reviewGuidance = ReviewGuidance.MERGE_WITHOUT_REVIEW,
     importance = Importance.MEDIUM,
     executionPriority = CodemodExecutionPriority.HIGH)
-public final class OutputResourceLeakCodemod extends SarifPluginJavaParserChanger<Expression> {
+public final class OutputResourceLeakCodemod extends CodeQLSarifJavaParserChanger<Expression> {
 
   @Inject
   public OutputResourceLeakCodemod(
@@ -35,4 +37,12 @@ public ChangesResult onResultFound(
         ? ChangesResult.changesApplied
         : ChangesResult.noChanges;
   }
+
+  @Override
+  public DetectorRule detectorRule() {
+    return new DetectorRule(
+        "output-resource-leak",
+        "Prevent resource leaks",
+        "https://codeql.github.com/codeql-query-help/java/java-output-resource-leak/");
+  }
 }
diff --git a/core-codemods/src/main/java/io/codemodder/codemods/StackTraceExposureCodemod.java b/core-codemods/src/main/java/io/codemodder/codemods/StackTraceExposureCodemod.java
@@ -7,7 +7,9 @@
 import com.github.javaparser.ast.expr.MethodCallExpr;
 import io.codemodder.*;
 import io.codemodder.ast.ASTs;
+import io.codemodder.codetf.DetectorRule;
 import io.codemodder.javaparser.ChangesResult;
+import io.codemodder.providers.sarif.codeql.CodeQLSarifJavaParserChanger;
 import io.codemodder.providers.sarif.codeql.ProvidedCodeQLScan;
 import javax.inject.Inject;
 
@@ -17,7 +19,7 @@
     reviewGuidance = ReviewGuidance.MERGE_WITHOUT_REVIEW,
     importance = Importance.MEDIUM,
     executionPriority = CodemodExecutionPriority.HIGH)
-public class StackTraceExposureCodemod extends SarifPluginJavaParserChanger<Expression> {
+public final class StackTraceExposureCodemod extends CodeQLSarifJavaParserChanger<Expression> {
 
   @Inject
   public StackTraceExposureCodemod(
@@ -51,4 +53,12 @@ public ChangesResult onResultFound(
     // cover the most common usage
     return ChangesResult.noChanges;
   }
+
+  @Override
+  public DetectorRule detectorRule() {
+    return new DetectorRule(
+        "stack-trace-exposure",
+        "Prevent information leak of stack trace details to HTTP responses",
+        "https://codeql.github.com/codeql-query-help/java/java-stack-trace-exposure/");
+  }
 }
diff --git a/core-codemods/src/main/java/io/codemodder/codemods/UnverifiedJwtCodemod.java b/core-codemods/src/main/java/io/codemodder/codemods/UnverifiedJwtCodemod.java
@@ -9,7 +9,9 @@
 import io.codemodder.ast.ASTTransforms;
 import io.codemodder.ast.ASTs;
 import io.codemodder.ast.LocalVariableDeclaration;
+import io.codemodder.codetf.DetectorRule;
 import io.codemodder.javaparser.ChangesResult;
+import io.codemodder.providers.sarif.codeql.CodeQLSarifJavaParserChanger;
 import io.codemodder.providers.sarif.codeql.ProvidedCodeQLScan;
 import javax.inject.Inject;
 
@@ -19,7 +21,7 @@
     reviewGuidance = ReviewGuidance.MERGE_WITHOUT_REVIEW,
     importance = Importance.MEDIUM,
     executionPriority = CodemodExecutionPriority.HIGH)
-public class UnverifiedJwtCodemod extends SarifPluginJavaParserChanger<Expression> {
+public final class UnverifiedJwtCodemod extends CodeQLSarifJavaParserChanger<Expression> {
 
   @Inject
   public UnverifiedJwtCodemod(
@@ -94,4 +96,12 @@ public ChangesResult onResultFound(
 
     return ChangesResult.noChanges;
   }
+
+  @Override
+  public DetectorRule detectorRule() {
+    return new DetectorRule(
+        "missing-jwt-signature-check",
+        "Switch JWT calls to versions that enforce signature validity",
+        "https://codeql.github.com/codeql-query-help/java/java-missing-jwt-signature-check/");
+  }
 }
diff --git a/framework/codemodder-base/src/main/java/io/codemodder/SarifFindingKeyUtil.java b/framework/codemodder-base/src/main/java/io/codemodder/SarifFindingKeyUtil.java
@@ -6,25 +6,35 @@
 import java.util.Collection;
 import java.util.HashMap;
 import java.util.Map;
+import org.apache.commons.lang3.StringUtils;
 
 /** Utility class for building keys for SARIF findings. */
-final class SarifFindingKeyUtil {
+public final class SarifFindingKeyUtil {
 
   private SarifFindingKeyUtil() {}
 
-  /** Builds a key for a SARIF finding based on the provided result, file path, and line number. */
-  public static String buildKey(final Result result, final Path path, final int line) {
-    final Fingerprints fingerprints = result.getFingerprints();
+  /**
+   * Builds a finding ID for a SARIF finding based on the provided result, file path, and line
+   * number.
+   */
+  public static String buildFindingId(final Result result, final Path path, final int line) {
+    // prefer the guid, then the correlation guid
+    if (!StringUtils.isBlank(result.getGuid())) {
+      return result.getGuid().trim();
+    } else if (!StringUtils.isBlank(result.getCorrelationGuid())) {
+      return result.getCorrelationGuid().trim();
+    }
 
+    // use a fingerprint, as at least that is some guarantee of uniqueness
+    final Fingerprints fingerprints = result.getFingerprints();
     final Map<String, String> fingerPrintProperties =
         fingerprints != null ? fingerprints.getAdditionalProperties() : new HashMap<>();
-
-    if (fingerPrintProperties.isEmpty()) {
-      return String.format("%s-%s-%d", result.getRuleId(), path.getFileName(), line);
+    if (!fingerPrintProperties.isEmpty()) {
+      Collection<String> values = fingerPrintProperties.values();
+      return values.iterator().next();
     }
 
-    Collection<String> values = fingerPrintProperties.values();
-
-    return values.iterator().next();
+    // ultimate fallback, some composite ID that will not represent anything
+    return String.format("%s-%s-%d", result.getRuleId(), path.getFileName(), line);
   }
 }
diff --git a/framework/codemodder-base/src/main/java/io/codemodder/SarifPluginJavaParserChanger.java b/framework/codemodder-base/src/main/java/io/codemodder/SarifPluginJavaParserChanger.java
@@ -159,7 +159,8 @@ private CodemodChange buildCodemodChange(
           line,
           dependencies,
           new FixedFinding(
-              SarifFindingKeyUtil.buildKey(result, path, line), fixOnlyCodeChanger.detectorRule()));
+              SarifFindingKeyUtil.buildFindingId(result, path, line),
+              fixOnlyCodeChanger.detectorRule()));
     }
 
     return CodemodChange.from(line, dependencies);
diff --git a/framework/codemodder-base/src/test/java/io/codemodder/SarifFindingKeyUtilTest.java b/framework/codemodder-base/src/test/java/io/codemodder/SarifFindingKeyUtilTest.java
diff --git a/plugins/codemodder-plugin-codeql/src/main/java/io/codemodder/providers/sarif/codeql/CodeQLSarifJavaParserChanger.java b/plugins/codemodder-plugin-codeql/src/main/java/io/codemodder/providers/sarif/codeql/CodeQLSarifJavaParserChanger.java

Original file line number	Diff line number	Diff line change
`@@ -159,7 +159,8 @@ private CodemodChange buildCodemodChange(`
`159`	`159`	`line,`
`160`	`160`	`dependencies,`
`161`	`161`	`new FixedFinding(`
`162`		`- SarifFindingKeyUtil.buildKey(result, path, line), fixOnlyCodeChanger.detectorRule()));`
	`162`	`+ SarifFindingKeyUtil.buildFindingId(result, path, line),`
	`163`	`+ fixOnlyCodeChanger.detectorRule()));`
`163`	`164`	`}`
`164`	`165`
`165`	`166`	`return CodemodChange.from(line, dependencies);`