From cb9672c2d2e336e5c51fa91d50ddcda7088583fb Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Mon, 31 Aug 2026 15:30:31 +0200 Subject: [PATCH 1/6] Harden XML parsing via commons-secure-xml Create XML stream readers, parsers, schema factories and transformers through org.apache.commons:commons-secure-xml. The secure factories enable FEATURE_SECURE_PROCESSING and install a non-removable entity-resolver floor on every parser they produce: external DTD, entity, schema and XInclude lookups that a caller-set resolver does not resolve are resolved to empty content instead of being fetched, and internal entity expansion is bounded, regardless of the JAXP implementation on the classpath. Changes: - Add the commons-secure-xml dependency (1.0.0-SNAPSHOT until its first release). - Route factory creation through SecureXMLInputFactory, SecureSchemaFactory and SecureDocumentBuilderFactory in SCXMLReader, and through SecureTransformerFactory in SCXMLWriter and ContentParser. A Configuration-supplied XMLResolver still takes precedence: the floor only handles lookups the resolver leaves unresolved, and the factoryId/factoryClassLoader override still selects the underlying implementation. - ContentParser.parseXml now wraps its argument in an InputSource: DocumentBuilder.parse(String) interprets its argument as a URI, so the method never actually parsed the XML content it was documented to parse. - Run the CI and CodeQL builds with -Puse-apache-snapshots (inherited from the org.apache:apache parent POM) so the commons-secure-xml SNAPSHOT resolves; CodeQL's autobuild receives the profile through MAVEN_ARGS. Assisted-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01MHgnMnGWHQoH2zD2jFdoMT --- .github/workflows/codeql-analysis.yml | 2 ++ .github/workflows/maven.yml | 2 +- pom.xml | 5 +++++ src/changes/changes.xml | 2 ++ .../org/apache/commons/scxml2/io/ContentParser.java | 11 +++++++---- .../org/apache/commons/scxml2/io/SCXMLReader.java | 12 +++++++----- .../org/apache/commons/scxml2/io/SCXMLWriter.java | 5 +++-- 7 files changed, 27 insertions(+), 12 deletions(-) diff --git a/.github/workflows/codeql-analysis.yml b/.github/workflows/codeql-analysis.yml index 6a0376283..a758ed6bb 100644 --- a/.github/workflows/codeql-analysis.yml +++ b/.github/workflows/codeql-analysis.yml @@ -64,6 +64,8 @@ jobs: # If this step fails, then you should remove it and run the build manually (see below) - name: Autobuild uses: github/codeql-action/autobuild@cdf488f595d80d6e07e03d4674febd5ab45fa938 # v4.37.9 + env: + MAVEN_ARGS: -Puse-apache-snapshots # ℹī¸ Command-line programs to run using the OS shell. # 📚 https://git.io/JvXDl diff --git a/.github/workflows/maven.yml b/.github/workflows/maven.yml index 529756551..ea82e4eac 100644 --- a/.github/workflows/maven.yml +++ b/.github/workflows/maven.yml @@ -56,4 +56,4 @@ jobs: distribution: 'temurin' java-version: ${{ matrix.java }} - name: Build with Maven - run: mvn --errors --show-version --batch-mode --no-transfer-progress + run: mvn --errors --show-version --batch-mode --no-transfer-progress -Puse-apache-snapshots diff --git a/pom.xml b/pom.xml index dc552c616..d4a8b8505 100644 --- a/pom.xml +++ b/pom.xml @@ -139,6 +139,11 @@ + + org.apache.commons + commons-secure-xml + 1.0.0-SNAPSHOT + commons-logging commons-logging diff --git a/src/changes/changes.xml b/src/changes/changes.xml index 07d9edd3a..5503526e5 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..265497816 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,9 @@ private static XMLStreamReader getReader(final Configuration configuration, fina throws IOException, XMLStreamException { // Instantiate the XMLInputFactory - XMLInputFactory factory = XMLInputFactory.newInstance(); + XMLInputFactory factory = SecureXMLInputFactory.newInstance(); if (configuration.factoryId != null && configuration.factoryClassLoader != null) { - factory = XMLInputFactory.newFactory(configuration.factoryId, configuration.factoryClassLoader); + factory = SecureXMLInputFactory.newFactory(configuration.factoryId, configuration.factoryClassLoader); } factory.setEventAllocator(configuration.allocator); if (factory.isPropertySupported(XMLInputFactory_JDK_PROP_REPORT_CDATA)) { @@ -623,7 +625,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 +1343,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..b42a9c0d8 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; /** @@ -339,7 +340,7 @@ private static Transformer getTransformer() { outputProps.put(OutputKeys.STANDALONE, "no"); outputProps.put(OutputKeys.INDENT, "yes"); try { - final TransformerFactory tfFactory = TransformerFactory.newInstance(); + final TransformerFactory tfFactory = SecureTransformerFactory.newInstance(); transformer = tfFactory.newTransformer(); transformer.setOutputProperties(outputProps); } catch (TransformerFactoryConfigurationError | TransformerConfigurationException t) { @@ -1130,7 +1131,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) { From d48f14424e864a2d1535ccec16350ce8aa683ee4 Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Thu, 3 Sep 2026 07:04:09 +0200 Subject: [PATCH 2/6] Use the Commons Secure XML 1.0.0 release candidate Bump org.apache.commons:commons-secure-xml from 1.0.0-SNAPSHOT to 1.0.0 and add the temporary staging repository https://repository.apache.org/content/repositories/orgapachecommons-1962/ after Central, so the vote gets downstream CI results. Drop the -Puse-apache-snapshots profile from the CI workflows, which the release version no longer needs. Remove the staging repository once 1.0.0 is released. Assisted-By: Claude Fable 5.1 Claude-Session: https://claude.ai/code/session_0167e29ScPEdfzJnEFm95imK --- .github/workflows/codeql-analysis.yml | 2 -- .github/workflows/maven.yml | 2 +- pom.xml | 23 ++++++++++++++++++++++- 3 files changed, 23 insertions(+), 4 deletions(-) diff --git a/.github/workflows/codeql-analysis.yml b/.github/workflows/codeql-analysis.yml index a758ed6bb..6a0376283 100644 --- a/.github/workflows/codeql-analysis.yml +++ b/.github/workflows/codeql-analysis.yml @@ -64,8 +64,6 @@ jobs: # If this step fails, then you should remove it and run the build manually (see below) - name: Autobuild uses: github/codeql-action/autobuild@cdf488f595d80d6e07e03d4674febd5ab45fa938 # v4.37.9 - env: - MAVEN_ARGS: -Puse-apache-snapshots # ℹī¸ Command-line programs to run using the OS shell. # 📚 https://git.io/JvXDl diff --git a/.github/workflows/maven.yml b/.github/workflows/maven.yml index ea82e4eac..529756551 100644 --- a/.github/workflows/maven.yml +++ b/.github/workflows/maven.yml @@ -56,4 +56,4 @@ jobs: distribution: 'temurin' java-version: ${{ matrix.java }} - name: Build with Maven - run: mvn --errors --show-version --batch-mode --no-transfer-progress -Puse-apache-snapshots + run: mvn --errors --show-version --batch-mode --no-transfer-progress diff --git a/pom.xml b/pom.xml index d4a8b8505..84f6f86dc 100644 --- a/pom.xml +++ b/pom.xml @@ -138,11 +138,32 @@ + + + + central + Central Repository + https://repo.maven.apache.org/maven2 + + false + + + + + apache.commons.staging + Apache Commons Secure XML 1.0.0 release candidate + https://repository.apache.org/content/repositories/orgapachecommons-1962/ + + false + + + + org.apache.commons commons-secure-xml - 1.0.0-SNAPSHOT + 1.0.0 commons-logging From 27e4b6971b000b8b88a8049bd21019f7fcb58571 Mon Sep 17 00:00:00 2001 From: Gary Gregory Date: Sun, 6 Sep 2026 08:39:37 -0400 Subject: [PATCH 3/6] Bump Apache Commons Secure XML from 1.0.0-SNAPSHOT to 1.0.0 --- pom.xml | 21 --------------------- src/changes/changes.xml | 2 +- 2 files changed, 1 insertion(+), 22 deletions(-) diff --git a/pom.xml b/pom.xml index 84f6f86dc..ff3c2d526 100644 --- a/pom.xml +++ b/pom.xml @@ -138,27 +138,6 @@ - - - - central - Central Repository - https://repo.maven.apache.org/maven2 - - false - - - - - apache.commons.staging - Apache Commons Secure XML 1.0.0 release candidate - https://repository.apache.org/content/repositories/orgapachecommons-1962/ - - false - - - - org.apache.commons diff --git a/src/changes/changes.xml b/src/changes/changes.xml index 5503526e5..3b5d27dd6 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -30,7 +30,7 @@ [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. + 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. From abccffcab8bd9c027c6946b6363e1b56bccddc5d Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Fri, 11 Sep 2026 09:26:39 +0200 Subject: [PATCH 4/6] Fail loudly when no usable XML transformer is available SCXMLWriter.getTransformer() logged the failure and returned null, so the static XFORMER field silently became null and every writeNode() call failed later with a NullPointerException far from the actual cause. Throw an IllegalStateException instead, with a message that names the reason: either the TrAX implementation found on the class path rejects the output properties the writer needs, or no implementation could be instantiated at all. Catch IllegalArgumentException from setOutputProperties() as well, and keep TransformerFactoryConfigurationError in the catch so a missing implementation is reported through the same message rather than as a bare Error from the class initializer. The failure can only be caused by the TrAX implementation in use, so there is nothing for a caller to recover from. Assisted-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0127cAFjN5DYE8pLAysQps4f --- .../apache/commons/scxml2/io/SCXMLWriter.java | 25 +++++++++++++------ 1 file changed, 17 insertions(+), 8 deletions(-) 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 b42a9c0d8..347b1e706 100644 --- a/src/main/java/org/apache/commons/scxml2/io/SCXMLWriter.java +++ b/src/main/java/org/apache/commons/scxml2/io/SCXMLWriter.java @@ -330,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 = SecureTransformerFactory.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; } /** From dc967684ea0d3a07475adb53c961b0a53ba50be9 Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Fri, 11 Sep 2026 09:29:17 +0200 Subject: [PATCH 5/6] Do not create a throwaway XMLInputFactory in SCXMLReader getReader() always called SecureXMLInputFactory.newInstance() and then discarded the result when the Configuration supplied a factoryId and a class loader, replacing it with the factory created by newFactory(). Create only the factory that is actually used, and make the local final. Assisted-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0127cAFjN5DYE8pLAysQps4f --- src/main/java/org/apache/commons/scxml2/io/SCXMLReader.java | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-) 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 265497816..c8e36b9e6 100644 --- a/src/main/java/org/apache/commons/scxml2/io/SCXMLReader.java +++ b/src/main/java/org/apache/commons/scxml2/io/SCXMLReader.java @@ -591,9 +591,11 @@ private static XMLStreamReader getReader(final Configuration configuration, fina throws IOException, XMLStreamException { // Instantiate the XMLInputFactory - XMLInputFactory factory = SecureXMLInputFactory.newInstance(); + final XMLInputFactory factory; if (configuration.factoryId != null && configuration.factoryClassLoader != null) { factory = SecureXMLInputFactory.newFactory(configuration.factoryId, configuration.factoryClassLoader); + } else { + factory = SecureXMLInputFactory.newInstance(); } factory.setEventAllocator(configuration.allocator); if (factory.isPropertySupported(XMLInputFactory_JDK_PROP_REPORT_CDATA)) { From 5b1f1901a086d8ce26d76fcd73aea17f879b42d0 Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Fri, 11 Sep 2026 09:46:50 +0200 Subject: [PATCH 6/6] Test that ContentParser.parseXml() parses XML content The class had no coverage for its XML methods. Assert that parseXml() treats its argument as an XML document rather than as a URI, which is what DocumentBuilder.parse(String) did before the method started wrapping the string in an InputSource, and that parseContent() maps an XML document to a NodeValue and toXml() round trips back into it. Assisted-By: Claude Opus 5 (1M context) Claude-Session: https://claude.ai/code/session_0127cAFjN5DYE8pLAysQps4f --- .../commons/scxml2/io/ContentParserTest.java | 33 +++++++++++++++++++ 1 file changed, 33 insertions(+) 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));