You are viewing a plain text version of this content. The canonical link for it is here.
Posted to commits@commons.apache.org by pk...@apache.org on 2026/08/31 14:23:46 UTC
(commons-jelly) branch feat/use-commons-xml created (now 7278e247)
This is an automated email from the ASF dual-hosted git repository.
ppkarwasz pushed a change to branch feat/use-commons-xml
in repository https://gitbox.apache.org/repos/asf/commons-jelly.git
at 7278e247 Harden XML parsing via commons-secure-xml
This branch includes the following new commits:
new 7278e247 Harden XML parsing via commons-secure-xml
The 1 revisions listed above as "new" are entirely new to this
repository and will be described in separate emails. The revisions
listed as "add" were already present in the repository and have only
been added to this reference.
(commons-jelly) 01/01: Harden XML parsing via commons-secure-xml
Posted by pk...@apache.org.
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 <pi...@github.copernik.eu>
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 <no...@anthropic.com>
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>