xmlUtils: disable external entity handling (#175)

walmartlabs · Sep 26, 2024 · 643126b · 643126b
1 parent a3b23a9
commit 643126b
Show file tree

Hide file tree

Showing 5 changed files with 56 additions and 15 deletions.
diff --git a/tasks/xml/src/main/java/com/walmartlabs/concord/plugins/xmlutils/XmlUtilsTaskCommon.java b/tasks/xml/src/main/java/com/walmartlabs/concord/plugins/xmlutils/XmlUtilsTaskCommon.java
@@ -23,13 +23,17 @@
 import org.w3c.dom.Document;
 import org.w3c.dom.Node;
 import org.w3c.dom.NodeList;
+import org.xml.sax.SAXException;
 
 import javax.xml.namespace.QName;
 import javax.xml.parsers.DocumentBuilder;
 import javax.xml.parsers.DocumentBuilderFactory;
+import javax.xml.parsers.ParserConfigurationException;
 import javax.xml.xpath.XPath;
 import javax.xml.xpath.XPathConstants;
+import javax.xml.xpath.XPathExpressionException;
 import javax.xml.xpath.XPathFactory;
+import java.io.IOException;
 import java.nio.file.Files;
 import java.nio.file.Path;
 import java.util.ArrayList;
@@ -48,7 +52,7 @@ public XmlUtilsTaskCommon(Path workDir) {
     /**
      * Evaluates the expression and returns a single {@link String} value.
      */
-    public String xpathString(String file, String expression) throws Exception {
+    public String xpathString(String file, String expression) throws XPathExpressionException, ParserConfigurationException, IOException, SAXException {
         Node n = (Node) eval(workDir, file, expression, XPathConstants.NODE);
 
         if (n == null) {
@@ -66,7 +70,7 @@ public String xpathString(String file, String expression) throws Exception {
     /**
      * Evaluates the expression and returns a list of {@link String} values.
      */
-    public List<String> xpathListOfStrings(String file, String expression) throws Exception {
+    public List<String> xpathListOfStrings(String file, String expression) throws XPathExpressionException, ParserConfigurationException, IOException, SAXException {
         NodeList l = (NodeList) eval(workDir, file, expression, XPathConstants.NODESET);
 
         if (l == null) {
@@ -91,7 +95,7 @@ public List<String> xpathListOfStrings(String file, String expression) throws Ex
      * Uses XPath to return {@code groupId + artifactId + version} attributes from a Maven pom.xml file.
      * Knows how to handle the {@code <parent>} tag, i.e. parent GAV values are merged with the pom's own GAV.
      */
-    public Map<String, String> mavenGav(String file) throws Exception {
+    public Map<String, String> mavenGav(String file) throws ParserConfigurationException, IOException, SAXException, XPathExpressionException {
         Document document = assertDocument(workDir, file);
         XPath xpath = XPathFactory.newInstance().newXPath();
 
@@ -106,23 +110,29 @@ public Map<String, String> mavenGav(String file) throws Exception {
         return result;
     }
 
-    private static Object eval(Path workDir, String file, String expression, QName returnType) throws Exception {
+    private static Object eval(Path workDir, String file, String expression, QName returnType) throws ParserConfigurationException, IOException, SAXException, XPathExpressionException {
         Document document = assertDocument(workDir, file);
         XPath xpath = XPathFactory.newInstance().newXPath();
         return xpath.evaluate(expression, document, returnType);
     }
 
-    private static Document assertDocument(Path workDir, String file) throws Exception {
+    private static Document assertDocument(Path workDir, String file) throws ParserConfigurationException, IOException, SAXException {
         Path src = workDir.resolve(file);
         if (!Files.exists(src)) {
             throw new IllegalArgumentException("File not found: " + file);
         }
 
-        DocumentBuilder builder = DocumentBuilderFactory.newInstance().newDocumentBuilder();
+        DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance();
+        dbf.setFeature("http://apache.org/xml/features/disallow-doctype-decl",true);
+        dbf.setFeature("http://xml.org/sax/features/external-general-entities",false);
+        dbf.setFeature("http://xml.org/sax/features/external-parameter-entities",false);
+
+        DocumentBuilder builder = dbf.newDocumentBuilder();
+
         return builder.parse(src.toFile());
     }
 
-    private static Map<String, String> toGav(String file, XPath xpath, Document document, String expression) throws Exception {
+    private static Map<String, String> toGav(String file, XPath xpath, Document document, String expression) throws XPathExpressionException {
         NodeList l = (NodeList) xpath.evaluate(expression, document, XPathConstants.NODESET);
 
         Map<String, String> result = new HashMap<>(l.getLength());

diff --git a/tasks/xml/src/main/java/com/walmartlabs/concord/plugins/xmlutils/v2/XmlUtilsTaskV2.java b/tasks/xml/src/main/java/com/walmartlabs/concord/plugins/xmlutils/v2/XmlUtilsTaskV2.java
@@ -50,6 +50,7 @@ public String xpathString(String file, String expression) throws Exception {
     /**
      * @deprecated Use {@link #xpathString(String, String)} instead
      */
+    @Deprecated(since = "1.42.0")
     public String xpathString(String workDir, String file, String expression) throws Exception {
         return xpathString(file, expression);
     }
@@ -64,7 +65,7 @@ public List<String> xpathListOfStrings(String file, String expression) throws Ex
     /**
      * @deprecated Use {@link #xpathListOfStrings(String, String)} instead
      */
-    @Deprecated
+    @Deprecated(since = "1.42.0")
     public List<String> xpathListOfStrings(String workDir, String file, String expression) throws Exception {
         return xpathListOfStrings(file, expression);
     }
@@ -80,7 +81,7 @@ public Map<String, String> mavenGav(String file) throws Exception {
     /**
      * @deprecated Use {@link #mavenGav(String)} instead
      */
-    @Deprecated
+    @Deprecated(since = "1.42.0")
     public Map<String, String> mavenGav(String workDir, String file) throws Exception {
         return mavenGav(file);
     }

diff --git a/tasks/xml/src/test/java/com/walmartlabs/concord/plugins/xmlutils/XmlUtilsTaskTest.java b/tasks/xml/src/test/java/com/walmartlabs/concord/plugins/xmlutils/XmlUtilsTaskTest.java
@@ -28,12 +28,13 @@
 import java.util.Map;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
-public class XmlUtilsTaskTest {
+class XmlUtilsTaskTest {
 
     @Test
-    public void testXpathString() throws Exception {
+    void testXpathString() throws Exception {
         String file = "test.xml";
 
         URL src = ClassLoader.getSystemResource(file);
@@ -52,7 +53,7 @@ public void testXpathString() throws Exception {
     }
 
     @Test
-    public void testMavenGav() throws Exception {
+    void testMavenGav() throws Exception {
         URL src = ClassLoader.getSystemResource("test.xml");
         String workDir = Paths.get(src.toURI()).getParent().toAbsolutePath().toString();
 
@@ -70,4 +71,16 @@ public void testMavenGav() throws Exception {
         assertEquals("xml-tasks", m.get("artifactId"));
         assertEquals("1.27.1-SNAPSHOT", m.get("version"));
     }
+
+    @Test
+    void testExternalReference() throws Exception {
+        URL src = ClassLoader.getSystemResource("test_external.xml");
+        String workDir = Paths.get(src.toURI()).getParent().toAbsolutePath().toString();
+
+        XmlUtilsTask t = new XmlUtilsTask();
+
+        Exception e = assertThrows(Exception.class, () -> t.xpathString(workDir, "test_external.xml", "/foo"));
+        assertTrue(e.getMessage().contains("DOCTYPE is disallowed"));
+    }
+
 }
diff --git a/tasks/xml/src/test/java/com/walmartlabs/concord/plugins/xmlutils/XmlUtilsTaskV2Test.java b/tasks/xml/src/test/java/com/walmartlabs/concord/plugins/xmlutils/XmlUtilsTaskV2Test.java
@@ -34,12 +34,13 @@
 import java.util.UUID;
 
 import static org.junit.jupiter.api.Assertions.assertEquals;
+import static org.junit.jupiter.api.Assertions.assertThrows;
 import static org.junit.jupiter.api.Assertions.assertTrue;
 
-public class XmlUtilsTaskV2Test {
+class XmlUtilsTaskV2Test {
 
     @Test
-    public void testXpathString() throws Exception {
+    void testXpathString() throws Exception {
         String file = "test.xml";
 
         URL src = ClassLoader.getSystemResource(file);
@@ -57,7 +58,7 @@ public void testXpathString() throws Exception {
     }
 
     @Test
-    public void testMavenGav() throws Exception {
+    void testMavenGav() throws Exception {
         URL src = ClassLoader.getSystemResource("test.xml");
         Context ctx = new MockContextV2(Paths.get(src.toURI()).getParent().toAbsolutePath());
         XmlUtilsTaskV2 t = new XmlUtilsTaskV2(ctx);
@@ -75,6 +76,17 @@ public void testMavenGav() throws Exception {
         assertEquals("1.27.1-SNAPSHOT", m.get("version"));
     }
 
+    @Test
+    void testExternalReference() throws Exception {
+        URL src = ClassLoader.getSystemResource("test_external.xml");
+        Context ctx = new MockContextV2(Paths.get(src.toURI()).getParent().toAbsolutePath());
+
+        XmlUtilsTaskV2 t = new XmlUtilsTaskV2(ctx);
+
+        Exception e = assertThrows(Exception.class, () -> t.xpathString("test_external.xml", "/foo"));
+        assertTrue(e.getMessage().contains("DOCTYPE is disallowed"));
+    }
+
     private static class MockContextV2 implements Context {
         private final Path workDir;
 

diff --git a/tasks/xml/src/test/resources/test_external.xml b/tasks/xml/src/test/resources/test_external.xml
@@ -0,0 +1,5 @@
+<?xml version="1.0" encoding="ISO-8859-1"?>
+<!DOCTYPE foo [
+        <!ELEMENT foo ANY >
+        <!ENTITY xxe SYSTEM "file:///etc/passwd" >]>
+<foo>&xxe;</foo>