This is an automated email from the ASF dual-hosted git repository. ppkarwasz pushed a commit to branch feat/use-commons-xml in repository https://gitbox.apache.org/repos/asf/commons-jelly.git
commit 7278e2475494d9f95ca80bb23724c8e5b7e52e1a Author: Piotr P. Karwasz <[email protected]> AuthorDate: Mon Aug 31 15:51:17 2026 +0200 Harden XML parsing via commons-secure-xml Create SAX parsers and readers through org.apache.commons:commons-secure-xml. The secure factory enables FEATURE_SECURE_PROCESSING and installs a non-removable entity-resolver floor on every parser it produces: external DTD and entity 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) to core, jelly-tags/xml and jelly-tags/xmlunit. - core XMLParser keeps the documented JellyContext.setAllowDtdToCallExternalEntities(true) opt-in working by using a plain factory on that path; the default path uses the secure factory, and a factory assigned to the protected static field still wins. The flag-dependent choice is no longer cached in that field. - core ParseTag (which had no hardening at all) now creates its reader through the secure factory. - jelly-tags/xml: TransformTag's readers and ParseTag's dom4j SAXReader are built from the secure factory; dom4j and XMLReaderFactory otherwise provision readers through JAXP at their own defaults, and the deprecated org.xml.sax.driver system property no longer selects the reader class. The TransformerFactory itself stays unsecured for now: Xalan, which this module puts on the class path, drops the attributes of xsl:namespace-alias literal result elements under secure processing (XSLTElementProcessor rejects "foreign" attributes as non-fatal errors), silently breaking stylesheets such as the Schematron skeleton. - jelly-tags/xmlunit: the assertion tags' dom4j SAXReaders are built from the secure factory. - jelly-tags/html is unchanged: NekoHTML is an HTML scanner, not an XML parser. - 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 <[email protected]> Claude-Session: https://claude.ai/code/session_01MHgnMnGWHQoH2zD2jFdoMT --- .github/workflows/codeql-analysis.yml | 2 ++ .github/workflows/maven.yml | 2 +- core/pom.xml | 5 +++++ .../org/apache/commons/jelly/parser/XMLParser.java | 16 +++++++++----- .../apache/commons/jelly/tags/core/ParseTag.java | 3 ++- jelly-tags/xml/pom.xml | 5 +++++ .../apache/commons/jelly/tags/xml/ParseTag.java | 13 +++++++++-- .../commons/jelly/tags/xml/TransformTag.java | 25 +++++++++++++++++----- jelly-tags/xmlunit/pom.xml | 5 +++++ .../commons/jelly/tags/xmlunit/ActualTag.java | 11 +++++++++- .../tags/xmlunit/AssertDocumentsEqualTag.java | 11 +++++++++- .../commons/jelly/tags/xmlunit/ExpectedTag.java | 11 +++++++++- src/changes/changes.xml | 1 + 13 files changed, 93 insertions(+), 17 deletions(-) diff --git a/.github/workflows/codeql-analysis.yml b/.github/workflows/codeql-analysis.yml index 503c256a..45ad2146 100644 --- a/.github/workflows/codeql-analysis.yml +++ b/.github/workflows/codeql-analysis.yml @@ -69,6 +69,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@0499de31b99561a6d14a36a5f662c2a54f91beee # 3.29.5 + 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 14f64b72..2579ef2e 100644 --- a/.github/workflows/maven.yml +++ b/.github/workflows/maven.yml @@ -49,4 +49,4 @@ jobs: distribution: 'temurin' java-version: ${{ matrix.java }} - name: Build with Maven - run: mvn --errors --show-version --batch-mode --no-transfer-progress -Ddoclint=none -Darguments=-Xdoclint:none -Dcommons.javadoc.failOnWarnings=false + run: mvn --errors --show-version --batch-mode --no-transfer-progress -Ddoclint=none -Darguments=-Xdoclint:none -Dcommons.javadoc.failOnWarnings=false -Puse-apache-snapshots diff --git a/core/pom.xml b/core/pom.xml index 90b03b9c..240a3574 100644 --- a/core/pom.xml +++ b/core/pom.xml @@ -52,6 +52,11 @@ </plugins> </build> <dependencies> + <dependency> + <groupId>org.apache.commons</groupId> + <artifactId>commons-secure-xml</artifactId> + <version>1.0.0-SNAPSHOT</version> + </dependency> <dependency> <groupId>javax.servlet</groupId> <artifactId>servlet-api</artifactId> diff --git a/core/src/main/java/org/apache/commons/jelly/parser/XMLParser.java b/core/src/main/java/org/apache/commons/jelly/parser/XMLParser.java index 4027e1c6..cd1763bc 100644 --- a/core/src/main/java/org/apache/commons/jelly/parser/XMLParser.java +++ b/core/src/main/java/org/apache/commons/jelly/parser/XMLParser.java @@ -51,6 +51,7 @@ import org.apache.commons.jelly.impl.TextScript; import org.apache.commons.jelly.util.ClassLoaderUtils; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; +import org.apache.commons.xml.secure.SecureSAXParserFactory; import org.xml.sax.Attributes; import org.xml.sax.ErrorHandler; import org.xml.sax.InputSource; @@ -812,12 +813,17 @@ public class XMLParser extends DefaultHandler { // Create and return a new parser synchronized (this) { try { - if (factory == null) { - factory = SAXParserFactory.newInstance(); + SAXParserFactory parserFactory = factory; + if (parserFactory == null) { + // The secure factory's resolver floor would ignore external entities, so the + // documented opt-in keeps using a plain factory; do not cache the per-instance choice. + parserFactory = allowDtdToCallExternalEntities + ? SAXParserFactory.newInstance() + : SecureSAXParserFactory.newInstance(); } - factory.setNamespaceAware(true); - factory.setValidating(validating); - parser = factory.newSAXParser(); + parserFactory.setNamespaceAware(true); + parserFactory.setValidating(validating); + parser = parserFactory.newSAXParser(); return parser; } catch (final Exception e) { diff --git a/core/src/main/java/org/apache/commons/jelly/tags/core/ParseTag.java b/core/src/main/java/org/apache/commons/jelly/tags/core/ParseTag.java index 57936905..e868549a 100644 --- a/core/src/main/java/org/apache/commons/jelly/tags/core/ParseTag.java +++ b/core/src/main/java/org/apache/commons/jelly/tags/core/ParseTag.java @@ -30,6 +30,7 @@ import org.apache.commons.jelly.XMLOutput; import org.apache.commons.jelly.parser.XMLParser; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; +import org.apache.commons.xml.secure.SecureSAXParserFactory; import org.xml.sax.ContentHandler; import org.xml.sax.InputSource; import org.xml.sax.SAXException; @@ -76,7 +77,7 @@ public class ParseTag extends TagSupport { * Factory method to create a new XMLReader */ protected XMLReader createXMLReader() throws ParserConfigurationException, SAXException { - final SAXParserFactory factory = SAXParserFactory.newInstance(); + final SAXParserFactory factory = SecureSAXParserFactory.newInstance(); factory.setNamespaceAware(true); final SAXParser parser = factory.newSAXParser(); return parser.getXMLReader(); diff --git a/jelly-tags/xml/pom.xml b/jelly-tags/xml/pom.xml index 507d5783..17102fbc 100644 --- a/jelly-tags/xml/pom.xml +++ b/jelly-tags/xml/pom.xml @@ -31,6 +31,11 @@ <description>The Jelly XML Tag Library.</description> <dependencies> + <dependency> + <groupId>org.apache.commons</groupId> + <artifactId>commons-secure-xml</artifactId> + <version>1.0.0-SNAPSHOT</version> + </dependency> <dependency> <groupId>commons-jelly</groupId> <artifactId>commons-jelly</artifactId> diff --git a/jelly-tags/xml/src/main/java/org/apache/commons/jelly/tags/xml/ParseTag.java b/jelly-tags/xml/src/main/java/org/apache/commons/jelly/tags/xml/ParseTag.java index 333ee42b..ea7b155e 100644 --- a/jelly-tags/xml/src/main/java/org/apache/commons/jelly/tags/xml/ParseTag.java +++ b/jelly-tags/xml/src/main/java/org/apache/commons/jelly/tags/xml/ParseTag.java @@ -16,13 +16,17 @@ */ package org.apache.commons.jelly.tags.xml; +import javax.xml.parsers.ParserConfigurationException; + import org.apache.commons.jelly.JellyTagException; import org.apache.commons.jelly.MissingAttributeException; import org.apache.commons.jelly.XMLOutput; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; +import org.apache.commons.xml.secure.SecureSAXParserFactory; import org.dom4j.Document; import org.dom4j.io.SAXReader; +import org.xml.sax.SAXException; /** A tag which parses some XML and defines a variable with the parsed Document. * The XML can either be specified as its body or can be passed in via the @@ -47,8 +51,13 @@ public class ParseTag extends ParseTagSupport { * Factory method to create a new SAXReader */ @Override - protected SAXReader createSAXReader() { - return new SAXReader(validate); + protected SAXReader createSAXReader() throws SAXException { + // dom4j builds its reader through JAXP internally; hand it one from the secure factory instead. + try { + return new SAXReader(SecureSAXParserFactory.newNSInstance().newSAXParser().getXMLReader(), validate); + } catch (final ParserConfigurationException e) { + throw new SAXException(e); + } } // Tag interface diff --git a/jelly-tags/xml/src/main/java/org/apache/commons/jelly/tags/xml/TransformTag.java b/jelly-tags/xml/src/main/java/org/apache/commons/jelly/tags/xml/TransformTag.java index 990692d2..0bc37f50 100644 --- a/jelly-tags/xml/src/main/java/org/apache/commons/jelly/tags/xml/TransformTag.java +++ b/jelly-tags/xml/src/main/java/org/apache/commons/jelly/tags/xml/TransformTag.java @@ -27,6 +27,7 @@ import java.net.URL; import java.util.Iterator; import java.util.List; +import javax.xml.parsers.ParserConfigurationException; import javax.xml.transform.Result; import javax.xml.transform.TransformerConfigurationException; import javax.xml.transform.TransformerFactory; @@ -48,6 +49,7 @@ import org.apache.commons.jelly.impl.StaticTagScript; import org.apache.commons.jelly.impl.TagScript; import org.apache.commons.logging.Log; import org.apache.commons.logging.LogFactory; +import org.apache.commons.xml.secure.SecureSAXParserFactory; import org.dom4j.Document; import org.dom4j.io.DocumentResult; import org.dom4j.io.DocumentSource; @@ -61,7 +63,6 @@ import org.xml.sax.SAXNotRecognizedException; import org.xml.sax.SAXNotSupportedException; import org.xml.sax.XMLReader; import org.xml.sax.ext.LexicalHandler; -import org.xml.sax.helpers.XMLReaderFactory; /** A tag which parses some XML, applies an xslt transform to it * and defines a variable with the transformed Document. @@ -129,7 +130,7 @@ public class TransformTag extends ParseTag { private void doInvokeBody() throws SAXException { try { if (this.shouldParseBody()) { - final XMLReader anXMLReader = XMLReaderFactory.createXMLReader(); + final XMLReader anXMLReader = newSecureXMLReader(); anXMLReader.setContentHandler(this.xmlOutput); anXMLReader.setProperty(LEXICAL_HANDLER_PROPERTY,this.xmlOutput); final StringWriter writer = new StringWriter(); @@ -408,6 +409,10 @@ public class TransformTag extends ParseTag { * Constructor for TransformTag. */ public TransformTag() { + // Not the secure factory: Xalan (on the class path here) drops the attributes of + // xsl:namespace-alias literal result elements under secure processing (XSLTElementProcessor + // rejects "foreign" attributes), silently breaking stylesheets like the Schematron skeleton. + // The stylesheet is part of the script; the readers parsing the transform INPUT are secured. this.tf = (SAXTransformerFactory) TransformerFactory.newInstance(); } @@ -462,9 +467,19 @@ public class TransformTag extends ParseTag { * * @return XMLReader for the transform input * @throws SAXException - * If the value of the "org.xml.sax.driver" system property - * is null, or if the class cannot be loaded and instantiated. + * If the reader cannot be created. */ + /** + * Creates a namespace-aware XMLReader through the secure SAX parser factory. + */ + private static XMLReader newSecureXMLReader() throws SAXException { + try { + return SecureSAXParserFactory.newNSInstance().newSAXParser().getXMLReader(); + } catch (final ParserConfigurationException e) { + throw new SAXException(e); + } + } + protected XMLReader createXMLReader() throws SAXException { XMLReader xmlReader = null; final Object xmlReaderSourceObj = this.getXml(); @@ -474,7 +489,7 @@ public class TransformTag extends ParseTag { xmlReader = new TagBodyXMLReader(this); } else { - xmlReader = XMLReaderFactory.createXMLReader(); + xmlReader = newSecureXMLReader(); } return xmlReader; diff --git a/jelly-tags/xmlunit/pom.xml b/jelly-tags/xmlunit/pom.xml index f578cd10..e86f2a5b 100644 --- a/jelly-tags/xmlunit/pom.xml +++ b/jelly-tags/xmlunit/pom.xml @@ -31,6 +31,11 @@ <dependencies> <!-- START for compilation --> + <dependency> + <groupId>org.apache.commons</groupId> + <artifactId>commons-secure-xml</artifactId> + <version>1.0.0-SNAPSHOT</version> + </dependency> <dependency> <groupId>xmlunit</groupId> <artifactId>xmlunit</artifactId> diff --git a/jelly-tags/xmlunit/src/main/java/org/apache/commons/jelly/tags/xmlunit/ActualTag.java b/jelly-tags/xmlunit/src/main/java/org/apache/commons/jelly/tags/xmlunit/ActualTag.java index 6bcab434..383f8c73 100644 --- a/jelly-tags/xmlunit/src/main/java/org/apache/commons/jelly/tags/xmlunit/ActualTag.java +++ b/jelly-tags/xmlunit/src/main/java/org/apache/commons/jelly/tags/xmlunit/ActualTag.java @@ -17,16 +17,25 @@ package org.apache.commons.jelly.tags.xmlunit; +import javax.xml.parsers.ParserConfigurationException; + import org.apache.commons.jelly.JellyTagException; import org.apache.commons.jelly.XMLOutput; +import org.apache.commons.xml.secure.SecureSAXParserFactory; import org.dom4j.Document; import org.dom4j.io.SAXReader; +import org.xml.sax.SAXException; public class ActualTag extends XMLUnitTagSupport { @Override protected SAXReader createSAXReader() { - return new SAXReader(); + // dom4j builds its reader through JAXP internally; hand it one from the secure factory instead. + try { + return new SAXReader(SecureSAXParserFactory.newNSInstance().newSAXParser().getXMLReader()); + } catch (final ParserConfigurationException | SAXException e) { + throw new IllegalStateException("Unable to create a new XML reader", e); + } } @Override diff --git a/jelly-tags/xmlunit/src/main/java/org/apache/commons/jelly/tags/xmlunit/AssertDocumentsEqualTag.java b/jelly-tags/xmlunit/src/main/java/org/apache/commons/jelly/tags/xmlunit/AssertDocumentsEqualTag.java index 1097b80a..a1a93c42 100644 --- a/jelly-tags/xmlunit/src/main/java/org/apache/commons/jelly/tags/xmlunit/AssertDocumentsEqualTag.java +++ b/jelly-tags/xmlunit/src/main/java/org/apache/commons/jelly/tags/xmlunit/AssertDocumentsEqualTag.java @@ -17,12 +17,16 @@ package org.apache.commons.jelly.tags.xmlunit; +import javax.xml.parsers.ParserConfigurationException; + import org.apache.commons.jelly.JellyTagException; import org.apache.commons.jelly.XMLOutput; +import org.apache.commons.xml.secure.SecureSAXParserFactory; import org.custommonkey.xmlunit.Diff; import org.custommonkey.xmlunit.XMLUnit; import org.dom4j.Document; import org.dom4j.io.SAXReader; +import org.xml.sax.SAXException; /** * Compares two XML documents using XMLUnit (http://xmlunit.sourceforge.net/). @@ -46,7 +50,12 @@ public class AssertDocumentsEqualTag extends XMLUnitTagSupport { @Override protected SAXReader createSAXReader() { - return new SAXReader(); + // dom4j builds its reader through JAXP internally; hand it one from the secure factory instead. + try { + return new SAXReader(SecureSAXParserFactory.newNSInstance().newSAXParser().getXMLReader()); + } catch (final ParserConfigurationException | SAXException e) { + throw new IllegalStateException("Unable to create a new XML reader", e); + } } @Override diff --git a/jelly-tags/xmlunit/src/main/java/org/apache/commons/jelly/tags/xmlunit/ExpectedTag.java b/jelly-tags/xmlunit/src/main/java/org/apache/commons/jelly/tags/xmlunit/ExpectedTag.java index ffd2ae10..0913c149 100644 --- a/jelly-tags/xmlunit/src/main/java/org/apache/commons/jelly/tags/xmlunit/ExpectedTag.java +++ b/jelly-tags/xmlunit/src/main/java/org/apache/commons/jelly/tags/xmlunit/ExpectedTag.java @@ -17,16 +17,25 @@ package org.apache.commons.jelly.tags.xmlunit; +import javax.xml.parsers.ParserConfigurationException; + import org.apache.commons.jelly.JellyTagException; import org.apache.commons.jelly.XMLOutput; +import org.apache.commons.xml.secure.SecureSAXParserFactory; import org.dom4j.Document; import org.dom4j.io.SAXReader; +import org.xml.sax.SAXException; public class ExpectedTag extends XMLUnitTagSupport { @Override protected SAXReader createSAXReader() { - return new SAXReader(); + // dom4j builds its reader through JAXP internally; hand it one from the secure factory instead. + try { + return new SAXReader(SecureSAXParserFactory.newNSInstance().newSAXParser().getXMLReader()); + } catch (final ParserConfigurationException | SAXException e) { + throw new IllegalStateException("Unable to create a new XML reader", e); + } } @Override diff --git a/src/changes/changes.xml b/src/changes/changes.xml index 6fdce0b2..b9c6dc0f 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -48,6 +48,7 @@ The <action> type attribute can be add,update,fix,remove. <!-- ADD --> <action type="add" dev="ggregory" due-to="Gary Gregory">Extract commons version into new POM property ${mx4j.version}.</action> <!-- FIX --> + <action type="fix" dev="pkarwasz">Create XML parsers and readers through org.apache.commons:commons-secure-xml, so external entities and DTDs are no longer fetched unless JellyContext.setAllowDtdToCallExternalEntities(true) is used.</action> <action type="fix" dev="ggregory" due-to="Gary Gregory">Throw specialized RuntimeExceptions instead of plain RuntimeExceptions.</action> <action type="fix" dev="ggregory" due-to="Gary Gregory">Deprecate NestedRuntimeException for RuntimeException.</action> <action type="fix" dev="ggregory" due-to="Gary Gregory">Fix building on modern Java.</action>
