You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by "garydgregory (via GitHub)" <gi...@apache.org> on 2026/08/27 13:27:45 UTC
[PR] Refactor XmlFactories into their hardening target factories. [commons-xml]
garydgregory opened a new pull request, #53:
URL: https://github.com/apache/commons-xml/pull/53
Refactor XmlFactories into their hardening target factories.
Missing Java 8 methods will be added in a follow up PR.
Before you push a pull request, review this list:
- [x] Read the [contribution guidelines](CONTRIBUTING.md) for this project.
- [x] Read the [ASF Generative Tooling Guidance](https://www.apache.org/legal/generative-tooling.html) if you use Artificial Intelligence (AI).
- ~~[ ] I used AI to create any part of, or all of, this pull request. Which AI tool was used to create this pull request, and to what extent did it contribute?~~
- [x] Run a successful build using the default [Maven](https://maven.apache.org/) goal with `mvn`; that's `mvn` on the command line by itself.
- [x] Write unit tests that match behavioral changes, where the tests fail if the changes to the runtime are not applied. This may not always be possible, but it is a best practice.
- [x] Write a pull request description that is detailed enough to understand what the pull request does, how, and why.
- [x] Each commit in the pull request should have a meaningful subject line and body. Note that a maintainer may squash commits during the merge process.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor XmlFactories into their hardening target factories. [commons-xml]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #53:
URL: https://github.com/apache/commons-xml/pull/53#discussion_r3873413830
##########
src/main/java/org/apache/commons/xml/HardeningDocumentBuilderFactory.java:
##########
@@ -19,21 +19,47 @@
import java.util.Objects;
+import javax.xml.XMLConstants;
import javax.xml.parsers.DocumentBuilder;
import javax.xml.parsers.DocumentBuilderFactory;
+import javax.xml.parsers.FactoryConfigurationError;
import javax.xml.parsers.ParserConfigurationException;
import javax.xml.validation.Schema;
import org.xml.sax.EntityResolver;
/**
* {@link DocumentBuilderFactory} wrapper that keeps an ignore-all {@link EntityResolver} floor on every {@link DocumentBuilder} produced.
- *
- * <p>Wraps each produced builder in a {@link HardeningDocumentBuilder}; required when the underlying factory carries no resolver of its own and does not honor
+ * <p>
+ * Wraps each produced builder in a {@link HardeningDocumentBuilder}; required when the underlying factory carries no resolver of its own and does not honor
* JAXP 1.5 {@code ACCESS_EXTERNAL_*} (e.g. the external Xerces distribution). A caller-set resolver is routed through the floor rather than replacing it. Kept
- * as a standalone wrapper so any hardener can reuse the floor.</p>
+ * as a standalone wrapper so any hardener can reuse the floor.
+ * </p>
+ *
+ * @see org.apache.commons.xml
*/
-final class HardeningDocumentBuilderFactory extends DocumentBuilderFactory {
+public final class HardeningDocumentBuilderFactory extends DocumentBuilderFactory {
Review Comment:
I have mixed feelings about using the wrapper classes as public entry points:
**Pros**:
- We save up on the number of classes.
**Cons**:
- Users can use `instanceof` on the implementation class of the wrapper. I am not sure if this is good or bad. Since users could always compare implementation by class name, this is not a blocker.
- This makes a Multi-Release JAR harder to produce without duplicating a lot of code. Therefore we are left with calling the Java 9 and Java 13 methods by reflection. Knowing what users do with our JARs (e.g. `log4j-api`), maybe it is better, since there are a lot of shaders out there that don't know how to shade an MRJ.
If we choose this model (i.e. wrapper class == public entry point and Java 9/13 methods called by reflection), I would recommend to merge this class with `DocumentBuilderHardener` in a follow-up PR.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor XmlFactories into their hardening target factories. [commons-xml]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #53:
URL: https://github.com/apache/commons-xml/pull/53#discussion_r3874396667
##########
src/main/java/org/apache/commons/xml/HardeningDocumentBuilderFactory.java:
##########
@@ -19,21 +19,47 @@
import java.util.Objects;
+import javax.xml.XMLConstants;
import javax.xml.parsers.DocumentBuilder;
import javax.xml.parsers.DocumentBuilderFactory;
+import javax.xml.parsers.FactoryConfigurationError;
import javax.xml.parsers.ParserConfigurationException;
import javax.xml.validation.Schema;
import org.xml.sax.EntityResolver;
/**
* {@link DocumentBuilderFactory} wrapper that keeps an ignore-all {@link EntityResolver} floor on every {@link DocumentBuilder} produced.
- *
- * <p>Wraps each produced builder in a {@link HardeningDocumentBuilder}; required when the underlying factory carries no resolver of its own and does not honor
+ * <p>
+ * Wraps each produced builder in a {@link HardeningDocumentBuilder}; required when the underlying factory carries no resolver of its own and does not honor
* JAXP 1.5 {@code ACCESS_EXTERNAL_*} (e.g. the external Xerces distribution). A caller-set resolver is routed through the floor rather than replacing it. Kept
- * as a standalone wrapper so any hardener can reuse the floor.</p>
+ * as a standalone wrapper so any hardener can reuse the floor.
+ * </p>
+ *
+ * @see org.apache.commons.xml
*/
-final class HardeningDocumentBuilderFactory extends DocumentBuilderFactory {
+public final class HardeningDocumentBuilderFactory extends DocumentBuilderFactory {
+
+ /**
+ * Returns a new, hardened {@link DocumentBuilderFactory}.
+ * <p>
+ * Beyond the three universal guarantees on {@link org.apache.commons.xml}, XInclude resolution is denied by default. When
+ * {@link DocumentBuilderFactory#setXIncludeAware(boolean) setXIncludeAware(true)} is called on the returned factory, the parser will process
+ * {@code xi:include} elements but every external resource lookup is rejected. To permit specific trusted resources, install an
+ * {@link org.xml.sax.EntityResolver EntityResolver} on the {@link DocumentBuilder} that allow-lists them; any href the resolver does not explicitly allow
+ * stays blocked.
+ * </p>
+ *
+ * @return A hardened factory.
+ * @throws IllegalStateException Thrown if a required hardening setting cannot be applied to the underlying implementation.
+ * @throws IllegalStateException Thrown if a (non-Andoid) factory cannot support the secure processing feature
+ * {@link XMLConstants#FEATURE_SECURE_PROCESSING}.
+ * @throws FactoryConfigurationError Thrown from a factory in case of a {@link java.util.ServiceConfigurationError service configuration error} or if the
+ * implementation is not available or cannot be instantiated.
+ */
+ public static DocumentBuilderFactory newInstance() {
+ return DocumentBuilderHardener.harden(DocumentBuilderFactory.newInstance());
+ }
Review Comment:
Like what? `HardeningDocumentBuilderFactory.newDocumentBuilder()` returns a safe `DocumentBuilder`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor XmlFactories into their hardening target factories. [commons-xml]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on PR #53:
URL: https://github.com/apache/commons-xml/pull/53#issuecomment-5442382158
I compared this design with #54 and I prefer this one: it keeps all (or nearly all) the code for a given JAXP factory type in a single class.
I would only change a few things:
- Move the instance methods of each wrapper into a nested `Wrapper` class, so that the public class no longer extends the JAXP factory:
```java
public final class HardeningDocumentBuilderFactory {
public static DocumentBuilderFactory newInstance() { ... }
private static final class Wrapper extends DocumentBuilderFactory {
... current code ...
}
}
```
This way no JAXP static method is hidden, and users cannot accidentally obtain a non-hardened factory by calling an inherited method such as `newInstance(String, ClassLoader)` or `newDefaultInstance()`.
- (follow-up PR) Add the remaining JAXP factory methods, as you proposed. For the Java 9 and Java 13 methods we can use `MethodHandles.publicLookup()` rather than core reflection.
- (follow-up PR) Merge `DocumentBuilderHardener` into `HardeningDocumentBuilderFactory`, and likewise for the other types.
- (optional PR) Rename the classes, e.g. `SafeDocumentBuilderFactory`.
I dropped the Multi-Release jar idea: users shade this library without setting `Multi-Release: true`, then report that methods which worked in their IDE fail in "production". We have seen plenty of such issue reports for Log4j.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor XmlFactories into their hardening target factories. [commons-xml]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz merged PR #53:
URL: https://github.com/apache/commons-xml/pull/53
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor XmlFactories into their hardening target factories. [commons-xml]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #53:
URL: https://github.com/apache/commons-xml/pull/53#discussion_r3874419770
##########
src/main/java/org/apache/commons/xml/HardeningDocumentBuilderFactory.java:
##########
@@ -19,21 +19,47 @@
import java.util.Objects;
+import javax.xml.XMLConstants;
import javax.xml.parsers.DocumentBuilder;
import javax.xml.parsers.DocumentBuilderFactory;
+import javax.xml.parsers.FactoryConfigurationError;
import javax.xml.parsers.ParserConfigurationException;
import javax.xml.validation.Schema;
import org.xml.sax.EntityResolver;
/**
* {@link DocumentBuilderFactory} wrapper that keeps an ignore-all {@link EntityResolver} floor on every {@link DocumentBuilder} produced.
- *
- * <p>Wraps each produced builder in a {@link HardeningDocumentBuilder}; required when the underlying factory carries no resolver of its own and does not honor
+ * <p>
+ * Wraps each produced builder in a {@link HardeningDocumentBuilder}; required when the underlying factory carries no resolver of its own and does not honor
* JAXP 1.5 {@code ACCESS_EXTERNAL_*} (e.g. the external Xerces distribution). A caller-set resolver is routed through the floor rather than replacing it. Kept
- * as a standalone wrapper so any hardener can reuse the floor.</p>
+ * as a standalone wrapper so any hardener can reuse the floor.
+ * </p>
+ *
+ * @see org.apache.commons.xml
*/
-final class HardeningDocumentBuilderFactory extends DocumentBuilderFactory {
+public final class HardeningDocumentBuilderFactory extends DocumentBuilderFactory {
Review Comment:
> without duplicating a lot of code.
That shouldn't be the case if we subclass. if that matters, for example a `HardeningDocumentBuilderFactory13` could extend `HardeningDocumentBuilderFactory`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor XmlFactories into their hardening target factories. [commons-xml]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on PR #53:
URL: https://github.com/apache/commons-xml/pull/53#issuecomment-5443053197
Arg, I meant to open a PR in my fork, not here, apologies.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor XmlFactories into their hardening target factories. [commons-xml]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #53:
URL: https://github.com/apache/commons-xml/pull/53#discussion_r3874419770
##########
src/main/java/org/apache/commons/xml/HardeningDocumentBuilderFactory.java:
##########
@@ -19,21 +19,47 @@
import java.util.Objects;
+import javax.xml.XMLConstants;
import javax.xml.parsers.DocumentBuilder;
import javax.xml.parsers.DocumentBuilderFactory;
+import javax.xml.parsers.FactoryConfigurationError;
import javax.xml.parsers.ParserConfigurationException;
import javax.xml.validation.Schema;
import org.xml.sax.EntityResolver;
/**
* {@link DocumentBuilderFactory} wrapper that keeps an ignore-all {@link EntityResolver} floor on every {@link DocumentBuilder} produced.
- *
- * <p>Wraps each produced builder in a {@link HardeningDocumentBuilder}; required when the underlying factory carries no resolver of its own and does not honor
+ * <p>
+ * Wraps each produced builder in a {@link HardeningDocumentBuilder}; required when the underlying factory carries no resolver of its own and does not honor
* JAXP 1.5 {@code ACCESS_EXTERNAL_*} (e.g. the external Xerces distribution). A caller-set resolver is routed through the floor rather than replacing it. Kept
- * as a standalone wrapper so any hardener can reuse the floor.</p>
+ * as a standalone wrapper so any hardener can reuse the floor.
+ * </p>
+ *
+ * @see org.apache.commons.xml
*/
-final class HardeningDocumentBuilderFactory extends DocumentBuilderFactory {
+public final class HardeningDocumentBuilderFactory extends DocumentBuilderFactory {
Review Comment:
> without duplicating a lot of code.
That shouldn't be the case if we subclass, if that matters that much, for example a `HardeningDocumentBuilderFactory13` could extend `HardeningDocumentBuilderFactory9` could extend `HardeningDocumentBuilderFactory` but I'm not sure an MRJ would allow that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor XmlFactories into their hardening target factories. [commons-xml]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #53:
URL: https://github.com/apache/commons-xml/pull/53#discussion_r3873732139
##########
src/main/java/org/apache/commons/xml/HardeningDocumentBuilderFactory.java:
##########
@@ -19,21 +19,47 @@
import java.util.Objects;
+import javax.xml.XMLConstants;
import javax.xml.parsers.DocumentBuilder;
import javax.xml.parsers.DocumentBuilderFactory;
+import javax.xml.parsers.FactoryConfigurationError;
import javax.xml.parsers.ParserConfigurationException;
import javax.xml.validation.Schema;
import org.xml.sax.EntityResolver;
/**
* {@link DocumentBuilderFactory} wrapper that keeps an ignore-all {@link EntityResolver} floor on every {@link DocumentBuilder} produced.
- *
- * <p>Wraps each produced builder in a {@link HardeningDocumentBuilder}; required when the underlying factory carries no resolver of its own and does not honor
+ * <p>
+ * Wraps each produced builder in a {@link HardeningDocumentBuilder}; required when the underlying factory carries no resolver of its own and does not honor
* JAXP 1.5 {@code ACCESS_EXTERNAL_*} (e.g. the external Xerces distribution). A caller-set resolver is routed through the floor rather than replacing it. Kept
- * as a standalone wrapper so any hardener can reuse the floor.</p>
+ * as a standalone wrapper so any hardener can reuse the floor.
+ * </p>
+ *
+ * @see org.apache.commons.xml
*/
-final class HardeningDocumentBuilderFactory extends DocumentBuilderFactory {
+public final class HardeningDocumentBuilderFactory extends DocumentBuilderFactory {
+
+ /**
+ * Returns a new, hardened {@link DocumentBuilderFactory}.
+ * <p>
+ * Beyond the three universal guarantees on {@link org.apache.commons.xml}, XInclude resolution is denied by default. When
+ * {@link DocumentBuilderFactory#setXIncludeAware(boolean) setXIncludeAware(true)} is called on the returned factory, the parser will process
+ * {@code xi:include} elements but every external resource lookup is rejected. To permit specific trusted resources, install an
+ * {@link org.xml.sax.EntityResolver EntityResolver} on the {@link DocumentBuilder} that allow-lists them; any href the resolver does not explicitly allow
+ * stays blocked.
+ * </p>
+ *
+ * @return A hardened factory.
+ * @throws IllegalStateException Thrown if a required hardening setting cannot be applied to the underlying implementation.
+ * @throws IllegalStateException Thrown if a (non-Andoid) factory cannot support the secure processing feature
+ * {@link XMLConstants#FEATURE_SECURE_PROCESSING}.
+ * @throws FactoryConfigurationError Thrown from a factory in case of a {@link java.util.ServiceConfigurationError service configuration error} or if the
+ * implementation is not available or cannot be instantiated.
+ */
+ public static DocumentBuilderFactory newInstance() {
+ return DocumentBuilderHardener.harden(DocumentBuilderFactory.newInstance());
+ }
Review Comment:
This method hides `DocumentBuilderFactory`, which is not a problem _per-se_, but is a problem for the methods that we are **not** hiding, which will return **not** hardened methods.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor XmlFactories into their hardening target factories. [commons-xml]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #53:
URL: https://github.com/apache/commons-xml/pull/53#discussion_r3874396667
##########
src/main/java/org/apache/commons/xml/HardeningDocumentBuilderFactory.java:
##########
@@ -19,21 +19,47 @@
import java.util.Objects;
+import javax.xml.XMLConstants;
import javax.xml.parsers.DocumentBuilder;
import javax.xml.parsers.DocumentBuilderFactory;
+import javax.xml.parsers.FactoryConfigurationError;
import javax.xml.parsers.ParserConfigurationException;
import javax.xml.validation.Schema;
import org.xml.sax.EntityResolver;
/**
* {@link DocumentBuilderFactory} wrapper that keeps an ignore-all {@link EntityResolver} floor on every {@link DocumentBuilder} produced.
- *
- * <p>Wraps each produced builder in a {@link HardeningDocumentBuilder}; required when the underlying factory carries no resolver of its own and does not honor
+ * <p>
+ * Wraps each produced builder in a {@link HardeningDocumentBuilder}; required when the underlying factory carries no resolver of its own and does not honor
* JAXP 1.5 {@code ACCESS_EXTERNAL_*} (e.g. the external Xerces distribution). A caller-set resolver is routed through the floor rather than replacing it. Kept
- * as a standalone wrapper so any hardener can reuse the floor.</p>
+ * as a standalone wrapper so any hardener can reuse the floor.
+ * </p>
+ *
+ * @see org.apache.commons.xml
*/
-final class HardeningDocumentBuilderFactory extends DocumentBuilderFactory {
+public final class HardeningDocumentBuilderFactory extends DocumentBuilderFactory {
+
+ /**
+ * Returns a new, hardened {@link DocumentBuilderFactory}.
+ * <p>
+ * Beyond the three universal guarantees on {@link org.apache.commons.xml}, XInclude resolution is denied by default. When
+ * {@link DocumentBuilderFactory#setXIncludeAware(boolean) setXIncludeAware(true)} is called on the returned factory, the parser will process
+ * {@code xi:include} elements but every external resource lookup is rejected. To permit specific trusted resources, install an
+ * {@link org.xml.sax.EntityResolver EntityResolver} on the {@link DocumentBuilder} that allow-lists them; any href the resolver does not explicitly allow
+ * stays blocked.
+ * </p>
+ *
+ * @return A hardened factory.
+ * @throws IllegalStateException Thrown if a required hardening setting cannot be applied to the underlying implementation.
+ * @throws IllegalStateException Thrown if a (non-Andoid) factory cannot support the secure processing feature
+ * {@link XMLConstants#FEATURE_SECURE_PROCESSING}.
+ * @throws FactoryConfigurationError Thrown from a factory in case of a {@link java.util.ServiceConfigurationError service configuration error} or if the
+ * implementation is not available or cannot be instantiated.
+ */
+ public static DocumentBuilderFactory newInstance() {
+ return DocumentBuilderHardener.harden(DocumentBuilderFactory.newInstance());
+ }
Review Comment:
Like what? `HardeningDocumentBuilderFactory`.newDocumentBuilder() returns a safe `DocumentBuilder`
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Refactor XmlFactories into their hardening target factories. [commons-xml]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #53:
URL: https://github.com/apache/commons-xml/pull/53#discussion_r3874419770
##########
src/main/java/org/apache/commons/xml/HardeningDocumentBuilderFactory.java:
##########
@@ -19,21 +19,47 @@
import java.util.Objects;
+import javax.xml.XMLConstants;
import javax.xml.parsers.DocumentBuilder;
import javax.xml.parsers.DocumentBuilderFactory;
+import javax.xml.parsers.FactoryConfigurationError;
import javax.xml.parsers.ParserConfigurationException;
import javax.xml.validation.Schema;
import org.xml.sax.EntityResolver;
/**
* {@link DocumentBuilderFactory} wrapper that keeps an ignore-all {@link EntityResolver} floor on every {@link DocumentBuilder} produced.
- *
- * <p>Wraps each produced builder in a {@link HardeningDocumentBuilder}; required when the underlying factory carries no resolver of its own and does not honor
+ * <p>
+ * Wraps each produced builder in a {@link HardeningDocumentBuilder}; required when the underlying factory carries no resolver of its own and does not honor
* JAXP 1.5 {@code ACCESS_EXTERNAL_*} (e.g. the external Xerces distribution). A caller-set resolver is routed through the floor rather than replacing it. Kept
- * as a standalone wrapper so any hardener can reuse the floor.</p>
+ * as a standalone wrapper so any hardener can reuse the floor.
+ * </p>
+ *
+ * @see org.apache.commons.xml
*/
-final class HardeningDocumentBuilderFactory extends DocumentBuilderFactory {
+public final class HardeningDocumentBuilderFactory extends DocumentBuilderFactory {
Review Comment:
> without duplicating a lot of code.
That shouldn't be the case if we subclass. if that matters, for example a `HardeningDocumentBuilderFactory13` could extend `HardeningDocumentBuilderFactory9` could extend `HardeningDocumentBuilderFactory` but I'm not sure an MRJ would allow that.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org