Skip to content

Commit 643126b

Browse files
benbroadawaybenbroadaway
authored
xmlUtils: disable external entity handling (#175)
1 parent a3b23a9 commit 643126b

File tree

5 files changed

+56
-15
lines changed

5 files changed

+56
-15
lines changed

‎tasks/xml/src/main/java/com/walmartlabs/concord/plugins/xmlutils/XmlUtilsTaskCommon.java‎

Lines changed: 17 additions & 7 deletions
Original file line numberDiff line numberDiff line change
@@ -23,13 +23,17 @@
2323
import org.w3c.dom.Document;
2424
import org.w3c.dom.Node;
2525
import org.w3c.dom.NodeList;
26+
import org.xml.sax.SAXException;
2627

2728
import javax.xml.namespace.QName;
2829
import javax.xml.parsers.DocumentBuilder;
2930
import javax.xml.parsers.DocumentBuilderFactory;
31+
import javax.xml.parsers.ParserConfigurationException;
3032
import javax.xml.xpath.XPath;
3133
import javax.xml.xpath.XPathConstants;
34+
import javax.xml.xpath.XPathExpressionException;
3235
import javax.xml.xpath.XPathFactory;
36+
import java.io.IOException;
3337
import java.nio.file.Files;
3438
import java.nio.file.Path;
3539
import java.util.ArrayList;
@@ -48,7 +52,7 @@ public XmlUtilsTaskCommon(Path workDir) {
4852
/**
4953
* Evaluates the expression and returns a single {@link String} value.
5054
*/
51-
public String xpathString(String file, String expression) throws Exception {
55+
public String xpathString(String file, String expression) throws XPathExpressionException, ParserConfigurationException, IOException, SAXException {
5256
Node n = (Node) eval(workDir, file, expression, XPathConstants.NODE);
5357

5458
if (n == null) {
@@ -66,7 +70,7 @@ public String xpathString(String file, String expression) throws Exception {
6670
/**
6771
* Evaluates the expression and returns a list of {@link String} values.
6872
*/
69-
public List<String> xpathListOfStrings(String file, String expression) throws Exception {
73+
public List<String> xpathListOfStrings(String file, String expression) throws XPathExpressionException, ParserConfigurationException, IOException, SAXException {
7074
NodeList l = (NodeList) eval(workDir, file, expression, XPathConstants.NODESET);
7175

7276
if (l == null) {
@@ -91,7 +95,7 @@ public List<String> xpathListOfStrings(String file, String expression) throws Ex
9195
* Uses XPath to return {@code groupId + artifactId + version} attributes from a Maven pom.xml file.
9296
* Knows how to handle the {@code <parent>} tag, i.e. parent GAV values are merged with the pom's own GAV.
9397
*/
94-
public Map<String, String> mavenGav(String file) throws Exception {
98+
public Map<String, String> mavenGav(String file) throws ParserConfigurationException, IOException, SAXException, XPathExpressionException {
9599
Document document = assertDocument(workDir, file);
96100
XPath xpath = XPathFactory.newInstance().newXPath();
97101

@@ -106,23 +110,29 @@ public Map<String, String> mavenGav(String file) throws Exception {
106110
return result;
107111
}
108112

109-
private static Object eval(Path workDir, String file, String expression, QName returnType) throws Exception {
113+
private static Object eval(Path workDir, String file, String expression, QName returnType) throws ParserConfigurationException, IOException, SAXException, XPathExpressionException {
110114
Document document = assertDocument(workDir, file);
111115
XPath xpath = XPathFactory.newInstance().newXPath();
112116
return xpath.evaluate(expression, document, returnType);
113117
}
114118

115-
private static Document assertDocument(Path workDir, String file) throws Exception {
119+
private static Document assertDocument(Path workDir, String file) throws ParserConfigurationException, IOException, SAXException {
116120
Path src = workDir.resolve(file);
117121
if (!Files.exists(src)) {
118122
throw new IllegalArgumentException("File not found: " + file);
119123
}
120124

121-
DocumentBuilder builder = DocumentBuilderFactory.newInstance().newDocumentBuilder();
125+
DocumentBuilderFactory dbf = DocumentBuilderFactory.newInstance();
126+
dbf.setFeature("http://apache.org/xml/features/disallow-doctype-decl",true);
127+
dbf.setFeature("http://xml.org/sax/features/external-general-entities",false);
128+
dbf.setFeature("http://xml.org/sax/features/external-parameter-entities",false);
129+
130+
DocumentBuilder builder = dbf.newDocumentBuilder();
131+
122132
return builder.parse(src.toFile());
123133
}
124134

125-
private static Map<String, String> toGav(String file, XPath xpath, Document document, String expression) throws Exception {
135+
private static Map<String, String> toGav(String file, XPath xpath, Document document, String expression) throws XPathExpressionException {
126136
NodeList l = (NodeList) xpath.evaluate(expression, document, XPathConstants.NODESET);
127137

128138
Map<String, String> result = new HashMap<>(l.getLength());

‎tasks/xml/src/main/java/com/walmartlabs/concord/plugins/xmlutils/v2/XmlUtilsTaskV2.java‎

Lines changed: 3 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -50,6 +50,7 @@ public String xpathString(String file, String expression) throws Exception {
5050
/**
5151
* @deprecated Use {@link #xpathString(String, String)} instead
5252
*/
53+
@Deprecated(since = "1.42.0")
5354
public String xpathString(String workDir, String file, String expression) throws Exception {
5455
return xpathString(file, expression);
5556
}
@@ -64,7 +65,7 @@ public List<String> xpathListOfStrings(String file, String expression) throws Ex
6465
/**
6566
* @deprecated Use {@link #xpathListOfStrings(String, String)} instead
6667
*/
67-
@Deprecated
68+
@Deprecated(since = "1.42.0")
6869
public List<String> xpathListOfStrings(String workDir, String file, String expression) throws Exception {
6970
return xpathListOfStrings(file, expression);
7071
}
@@ -80,7 +81,7 @@ public Map<String, String> mavenGav(String file) throws Exception {
8081
/**
8182
* @deprecated Use {@link #mavenGav(String)} instead
8283
*/
83-
@Deprecated
84+
@Deprecated(since = "1.42.0")
8485
public Map<String, String> mavenGav(String workDir, String file) throws Exception {
8586
return mavenGav(file);
8687
}

‎tasks/xml/src/test/java/com/walmartlabs/concord/plugins/xmlutils/XmlUtilsTaskTest.java‎

Lines changed: 16 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -28,12 +28,13 @@
2828
import java.util.Map;
2929

3030
import static org.junit.jupiter.api.Assertions.assertEquals;
31+
import static org.junit.jupiter.api.Assertions.assertThrows;
3132
import static org.junit.jupiter.api.Assertions.assertTrue;
3233

33-
public class XmlUtilsTaskTest {
34+
class XmlUtilsTaskTest {
3435

3536
@Test
36-
public void testXpathString() throws Exception {
37+
void testXpathString() throws Exception {
3738
String file = "test.xml";
3839

3940
URL src = ClassLoader.getSystemResource(file);
@@ -52,7 +53,7 @@ public void testXpathString() throws Exception {
5253
}
5354

5455
@Test
55-
public void testMavenGav() throws Exception {
56+
void testMavenGav() throws Exception {
5657
URL src = ClassLoader.getSystemResource("test.xml");
5758
String workDir = Paths.get(src.toURI()).getParent().toAbsolutePath().toString();
5859

@@ -70,4 +71,16 @@ public void testMavenGav() throws Exception {
7071
assertEquals("xml-tasks", m.get("artifactId"));
7172
assertEquals("1.27.1-SNAPSHOT", m.get("version"));
7273
}
74+
75+
@Test
76+
void testExternalReference() throws Exception {
77+
URL src = ClassLoader.getSystemResource("test_external.xml");
78+
String workDir = Paths.get(src.toURI()).getParent().toAbsolutePath().toString();
79+
80+
XmlUtilsTask t = new XmlUtilsTask();
81+
82+
Exception e = assertThrows(Exception.class, () -> t.xpathString(workDir, "test_external.xml", "/foo"));
83+
assertTrue(e.getMessage().contains("DOCTYPE is disallowed"));
84+
}
85+
7386
}

‎tasks/xml/src/test/java/com/walmartlabs/concord/plugins/xmlutils/XmlUtilsTaskV2Test.java‎

Lines changed: 15 additions & 3 deletions
Original file line numberDiff line numberDiff line change
@@ -34,12 +34,13 @@
3434
import java.util.UUID;
3535

3636
import static org.junit.jupiter.api.Assertions.assertEquals;
37+
import static org.junit.jupiter.api.Assertions.assertThrows;
3738
import static org.junit.jupiter.api.Assertions.assertTrue;
3839

39-
public class XmlUtilsTaskV2Test {
40+
class XmlUtilsTaskV2Test {
4041

4142
@Test
42-
public void testXpathString() throws Exception {
43+
void testXpathString() throws Exception {
4344
String file = "test.xml";
4445

4546
URL src = ClassLoader.getSystemResource(file);
@@ -57,7 +58,7 @@ public void testXpathString() throws Exception {
5758
}
5859

5960
@Test
60-
public void testMavenGav() throws Exception {
61+
void testMavenGav() throws Exception {
6162
URL src = ClassLoader.getSystemResource("test.xml");
6263
Context ctx = new MockContextV2(Paths.get(src.toURI()).getParent().toAbsolutePath());
6364
XmlUtilsTaskV2 t = new XmlUtilsTaskV2(ctx);
@@ -75,6 +76,17 @@ public void testMavenGav() throws Exception {
7576
assertEquals("1.27.1-SNAPSHOT", m.get("version"));
7677
}
7778

79+
@Test
80+
void testExternalReference() throws Exception {
81+
URL src = ClassLoader.getSystemResource("test_external.xml");
82+
Context ctx = new MockContextV2(Paths.get(src.toURI()).getParent().toAbsolutePath());
83+
84+
XmlUtilsTaskV2 t = new XmlUtilsTaskV2(ctx);
85+
86+
Exception e = assertThrows(Exception.class, () -> t.xpathString("test_external.xml", "/foo"));
87+
assertTrue(e.getMessage().contains("DOCTYPE is disallowed"));
88+
}
89+
7890
private static class MockContextV2 implements Context {
7991
private final Path workDir;
8092

‎tasks/xml/src/test/resources/test_external.xml‎

Lines changed: 5 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,5 @@
1+
<?xml version="1.0" encoding="ISO-8859-1"?>
2+
<!DOCTYPE foo [
3+
<!ELEMENT foo ANY >
4+
<!ENTITY xxe SYSTEM "file:///etc/passwd" >]>
5+
<foo>&xxe;</foo>

0 commit comments

Comments
 (0)