diff --git a/pom.xml b/pom.xml index 92ae785b6..c60d288e4 100644 --- a/pom.xml +++ b/pom.xml @@ -139,6 +139,11 @@ + + org.apache.commons + commons-secure-xml + 1.0.0 + commons-logging commons-logging diff --git a/src/changes/changes.xml b/src/changes/changes.xml index 4110d4c65..082f08c5a 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -30,6 +30,8 @@ [18-10-2018] Before executing invoke handlers after a macrostep all internal events must have been processed Fix Apache RAT plugin console warnings. + Create XML parsers, stream readers and transformers through org.apache.commons:commons-secure-xml, so external entities and DTDs are no longer fetched by default. + ContentParser.parseXml now parses its argument as XML content instead of interpreting it as a URI. [10-10-2018] Clear up exception handling in tests diff --git a/src/main/java/org/apache/commons/scxml2/io/ContentParser.java b/src/main/java/org/apache/commons/scxml2/io/ContentParser.java index 875364bab..eb6880f0a 100644 --- a/src/main/java/org/apache/commons/scxml2/io/ContentParser.java +++ b/src/main/java/org/apache/commons/scxml2/io/ContentParser.java @@ -18,17 +18,16 @@ import java.io.IOException; import java.io.InputStream; +import java.io.StringReader; import java.io.StringWriter; import java.net.URL; import java.nio.charset.StandardCharsets; import java.util.Properties; -import javax.xml.parsers.DocumentBuilderFactory; import javax.xml.parsers.ParserConfigurationException; import javax.xml.transform.OutputKeys; import javax.xml.transform.Transformer; import javax.xml.transform.TransformerException; -import javax.xml.transform.TransformerFactory; import javax.xml.transform.dom.DOMSource; import javax.xml.transform.stream.StreamResult; @@ -37,8 +36,11 @@ import org.apache.commons.scxml2.model.NodeValue; import org.apache.commons.scxml2.model.ParsedValue; import org.apache.commons.scxml2.model.TextValue; +import org.apache.commons.xml.secure.SecureDocumentBuilderFactory; +import org.apache.commons.xml.secure.SecureTransformerFactory; import org.w3c.dom.Document; import org.w3c.dom.Node; +import org.xml.sax.InputSource; import org.xml.sax.SAXException; import com.fasterxml.jackson.core.JsonParser; @@ -224,7 +226,8 @@ public ParsedValue parseResource(final String resourceURL) throws IOException { public Node parseXml(final String xmlString) throws IOException { Document doc; try { - doc = DocumentBuilderFactory.newInstance().newDocumentBuilder().parse(xmlString); + // Wrap in an InputSource: DocumentBuilder.parse(String) would interpret the content as a URI. + doc = SecureDocumentBuilderFactory.newInstance().newDocumentBuilder().parse(new InputSource(new StringReader(xmlString))); } catch (SAXException | ParserConfigurationException e) { throw new IOException(e); } @@ -252,7 +255,7 @@ public String toJson(final Object jsonObject) throws IOException { public String toXml(final Node node) throws IOException { try { final StringWriter writer = new StringWriter(); - final Transformer transformer = TransformerFactory.newInstance().newTransformer(); + final Transformer transformer = SecureTransformerFactory.newInstance().newTransformer(); final Properties outputProps = new Properties(); outputProps.put(OutputKeys.OMIT_XML_DECLARATION, "no"); outputProps.put(OutputKeys.STANDALONE, "no"); diff --git a/src/main/java/org/apache/commons/scxml2/io/SCXMLReader.java b/src/main/java/org/apache/commons/scxml2/io/SCXMLReader.java index 52c0cde5d..c8e36b9e6 100644 --- a/src/main/java/org/apache/commons/scxml2/io/SCXMLReader.java +++ b/src/main/java/org/apache/commons/scxml2/io/SCXMLReader.java @@ -31,7 +31,6 @@ import java.util.List; import java.util.Map; -import javax.xml.parsers.DocumentBuilderFactory; import javax.xml.parsers.ParserConfigurationException; import javax.xml.stream.Location; import javax.xml.stream.XMLInputFactory; @@ -98,6 +97,9 @@ import org.apache.commons.scxml2.model.TransitionType; import org.apache.commons.scxml2.model.TransitionalState; import org.apache.commons.scxml2.model.Var; +import org.apache.commons.xml.secure.SecureDocumentBuilderFactory; +import org.apache.commons.xml.secure.SecureSchemaFactory; +import org.apache.commons.xml.secure.SecureXMLInputFactory; import org.w3c.dom.Attr; import org.w3c.dom.Document; import org.w3c.dom.Element; @@ -589,9 +591,11 @@ private static XMLStreamReader getReader(final Configuration configuration, fina throws IOException, XMLStreamException { // Instantiate the XMLInputFactory - XMLInputFactory factory = XMLInputFactory.newInstance(); + final XMLInputFactory factory; if (configuration.factoryId != null && configuration.factoryClassLoader != null) { - factory = XMLInputFactory.newFactory(configuration.factoryId, configuration.factoryClassLoader); + factory = SecureXMLInputFactory.newFactory(configuration.factoryId, configuration.factoryClassLoader); + } else { + factory = SecureXMLInputFactory.newInstance(); } factory.setEventAllocator(configuration.allocator); if (factory.isPropertySupported(XMLInputFactory_JDK_PROP_REPORT_CDATA)) { @@ -623,7 +627,7 @@ private static XMLStreamReader getReader(final Configuration configuration, fina // Validation requires us to use a Source final URL scxmlSchema = new URL("TODO"); // TODO, point to appropriate location - final SchemaFactory schemaFactory = SchemaFactory.newInstance("http://www.w3.org/2001/XMLSchema"); + final SchemaFactory schemaFactory = SecureSchemaFactory.newInstance("http://www.w3.org/2001/XMLSchema"); Schema schema; try { schema = schemaFactory.newSchema(scxmlSchema); @@ -1341,7 +1345,7 @@ private static Element readElement(final XMLStreamReader reader) // Create a document in which to build the DOM node Document document; try { - document = DocumentBuilderFactory.newInstance().newDocumentBuilder().newDocument(); + document = SecureDocumentBuilderFactory.newInstance().newDocumentBuilder().newDocument(); } catch (final ParserConfigurationException pce) { throw new XMLStreamException(ERR_PARSER_CFG); } diff --git a/src/main/java/org/apache/commons/scxml2/io/SCXMLWriter.java b/src/main/java/org/apache/commons/scxml2/io/SCXMLWriter.java index 4cf3e4c6d..347b1e706 100644 --- a/src/main/java/org/apache/commons/scxml2/io/SCXMLWriter.java +++ b/src/main/java/org/apache/commons/scxml2/io/SCXMLWriter.java @@ -81,6 +81,7 @@ import org.apache.commons.scxml2.model.Transition; import org.apache.commons.scxml2.model.TransitionTarget; import org.apache.commons.scxml2.model.Var; +import org.apache.commons.xml.secure.SecureTransformerFactory; import org.w3c.dom.Node; /** @@ -329,25 +330,34 @@ private static String escapeXML(final String str) { /** * Gets a {@link Transformer} instance that pretty prints the output. + *

+ * A failure here can only be caused by the TrAX implementation available on the class path: either no + * {@link TransformerFactory} can be instantiated at all, or the one that is instantiated rejects the + * output properties this writer requires. + *

* * @return Transformer The indenting {@link Transformer} instance. + * @throws IllegalStateException if no suitable {@link Transformer} can be created. */ private static Transformer getTransformer() { - Transformer transformer; final Properties outputProps = new Properties(); outputProps.put(OutputKeys.OMIT_XML_DECLARATION, "yes"); outputProps.put(OutputKeys.STANDALONE, "no"); outputProps.put(OutputKeys.INDENT, "yes"); + TransformerFactory factory = null; try { - final TransformerFactory tfFactory = TransformerFactory.newInstance(); - transformer = tfFactory.newTransformer(); + factory = SecureTransformerFactory.newInstance(); + final Transformer transformer = factory.newTransformer(); transformer.setOutputProperties(outputProps); - } catch (TransformerFactoryConfigurationError | TransformerConfigurationException t) { - final org.apache.commons.logging.Log log = LogFactory.getLog(SCXMLWriter.class); - log.error(t.getMessage(), t); - return null; + return transformer; + } catch (final IllegalArgumentException | TransformerConfigurationException | TransformerFactoryConfigurationError t) { + final String message = "Unable to create the XML transformer used to pretty print SCXML documents: " + + (factory != null + ? "the TrAX implementation " + factory.getClass().getName() + " does not support the required output properties." + : "no TrAX implementation is available on the class path."); + LogFactory.getLog(SCXMLWriter.class).error(message, t); + throw new IllegalStateException(message, t); } - return transformer; } /** @@ -1130,7 +1140,7 @@ private static void writePretty(final Configuration configuration, final OutputS prettyPrintResult = scxmlResult; } - final TransformerFactory factory = TransformerFactory.newInstance(); + final TransformerFactory factory = SecureTransformerFactory.newInstance(); try { final Transformer transformer = factory.newTransformer(); if (configuration.encoding != null) { diff --git a/src/test/java/org/apache/commons/scxml2/io/ContentParserTest.java b/src/test/java/org/apache/commons/scxml2/io/ContentParserTest.java index 2df1731dd..18a96d6ac 100644 --- a/src/test/java/org/apache/commons/scxml2/io/ContentParserTest.java +++ b/src/test/java/org/apache/commons/scxml2/io/ContentParserTest.java @@ -17,12 +17,20 @@ package org.apache.commons.scxml2.io; import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertInstanceOf; import static org.junit.jupiter.api.Assertions.assertNull; +import static org.junit.jupiter.api.Assertions.assertTrue; import java.util.ArrayList; import java.util.LinkedHashMap; +import javax.xml.parsers.DocumentBuilder; + +import org.apache.commons.scxml2.model.NodeValue; +import org.apache.commons.scxml2.model.ParsedValue; import org.junit.jupiter.api.Test; +import org.w3c.dom.Element; +import org.w3c.dom.Node; import com.fasterxml.jackson.core.JsonParser; import com.fasterxml.jackson.databind.ObjectMapper; @@ -54,6 +62,31 @@ void testParseJson() throws Exception { assertEquals(jsonArray, contentParser.parseJson(jsonArrayString)); } + /** + * The XML string must be parsed as content. + * + *

{@link DocumentBuilder#parse(String)}, previously used, interpreted it as a URI.

+ */ + @Test + void testParseXml() throws Exception { + final ContentParser contentParser = new ContentParser(); + + final Node node = contentParser.parseXml("text"); + assertInstanceOf(Element.class, node); + assertEquals("root", node.getNodeName()); + assertEquals("value", ((Element) node).getAttribute("attr")); + assertEquals("text", node.getTextContent()); + + final ParsedValue parsedValue = contentParser.parseContent("text"); + assertInstanceOf(NodeValue.class, parsedValue); + assertEquals("root", ((Node) parsedValue.getValue()).getNodeName()); + + // Round trip: the serialized node parses back into an equivalent node + final String xml = contentParser.toXml(node); + assertTrue(xml.contains("text"), xml); + assertEquals("text", contentParser.parseXml(xml).getTextContent()); + } + @Test void testSpaceNormalizeContent() { assertNull(ContentParser.spaceNormalizeContent(null));