Filter DocumentBuilder#parse() calls more accurately (#465)

.. so it doesn't recognize `XMLReader#parse()` calls as well.
pixee · Nov 11, 2024 · 3ee3ad9 · 3ee3ad9
1 parent 0ecf40e
commit 3ee3ad9
Show file tree

Hide file tree

Showing 3 changed files with 45 additions and 9 deletions.
diff --git a/framework/codemodder-base/src/main/java/io/codemodder/remediation/NodePositionMatcher.java b/framework/codemodder-base/src/main/java/io/codemodder/remediation/NodePositionMatcher.java
@@ -5,7 +5,7 @@
 /** Provides methods for matching nodes to given coordinates. */
 public interface NodePositionMatcher {
 
-  static final NodePositionMatcher DEFAULT = new DefaultNodePositionMatcher();
+  NodePositionMatcher DEFAULT = new DefaultNodePositionMatcher();
 
   /**
    * Matches a node with a range against a line

diff --git a/...src/main/java/io/codemodder/remediation/xxe/DocumentBuilderFactoryAtParseFixStrategy.java b/...src/main/java/io/codemodder/remediation/xxe/DocumentBuilderFactoryAtParseFixStrategy.java
@@ -101,6 +101,20 @@ public boolean match(final Node node) {
         .map(n -> n instanceof MethodCallExpr ? (MethodCallExpr) n : null)
         .filter(m -> "parse".equals(m.getNameAsString()))
         .filter(m -> m.getScope().filter(Expression::isNameExpr).isPresent())
+        .filter(
+            m -> {
+              Optional<Node> sourceRef =
+                  ASTs.findNonCallableSimpleNameSource(m.getScope().get().asNameExpr().getName());
+              if (sourceRef.isEmpty()) {
+                return false;
+              }
+              Node source = sourceRef.get();
+              if (source instanceof NodeWithType<?, ?>) {
+                return Set.of("DocumentBuilder", "javax.xml.parsers.DocumentBuilder")
+                    .contains(((NodeWithType<?, ?>) source).getTypeAsString());
+              }
+              return false;
+            })
         .isPresent();
   }
 }
diff --git a/...odemodder-base/src/test/java/io/codemodder/remediation/xxe/XMLReaderAtParseFixerTest.java b/...odemodder-base/src/test/java/io/codemodder/remediation/xxe/XMLReaderAtParseFixerTest.java
@@ -9,12 +9,38 @@
 import io.codemodder.remediation.SearcherStrategyRemediator;
 import java.util.List;
 import java.util.Optional;
-import org.junit.jupiter.api.Test;
+import java.util.stream.Stream;
+import org.junit.jupiter.params.ParameterizedTest;
+import org.junit.jupiter.params.provider.Arguments;
+import org.junit.jupiter.params.provider.MethodSource;
 
 final class XMLReaderAtParseFixerTest {
 
-  @Test
-  void it_fixes_xmlreaders_at_parse_call() {
+  private static Stream<Arguments> provideRemediators() {
+    return Stream.of(
+        Arguments.of(
+            new SearcherStrategyRemediator.Builder<>()
+                .withMatchAndFixStrategy(new XMLReaderAtParseFixStrategy())
+                .build()),
+
+        // now try with the previously conflicting parser strategy inline as well
+        Arguments.of(
+            new SearcherStrategyRemediator.Builder<>()
+                .withMatchAndFixStrategy(new DocumentBuilderFactoryAtParseFixStrategy())
+                .withMatchAndFixStrategy(new XMLReaderAtParseFixStrategy())
+                .build()),
+
+        // reverse the order to confirm no ordering issues
+        Arguments.of(
+            new SearcherStrategyRemediator.Builder<>()
+                .withMatchAndFixStrategy(new XMLReaderAtParseFixStrategy())
+                .withMatchAndFixStrategy(new DocumentBuilderFactoryAtParseFixStrategy())
+                .build()));
+  }
+
+  @MethodSource("provideRemediators")
+  @ParameterizedTest
+  void it_fixes_xmlreaders_at_parse_call(final SearcherStrategyRemediator remediator) {
     String vulnerableCode =
         """
                     public class MyCode {
@@ -43,12 +69,8 @@ public void foo() {
     CompilationUnit cu = StaticJavaParser.parse(vulnerableCode);
     LexicalPreservingPrinter.setup(cu);
 
-    var searcherRemediator =
-        new SearcherStrategyRemediator.Builder<>()
-            .withMatchAndFixStrategy(new XMLReaderAtParseFixStrategy())
-            .build();
     var result =
-        searcherRemediator.remediateAll(
+        remediator.remediateAll(
             cu,
             "path",
             new DetectorRule("", "", ""),