This is an automated email from the ASF dual-hosted git repository. ppkarwasz pushed a commit to branch main in repository https://gitbox.apache.org/repos/asf/commons-secure-xml.git
commit ce48b10c7c05267821f799d7e00a8bb92bbdb44f Author: Piotr P. Karwasz <[email protected]> AuthorDate: Tue Sep 1 08:44:16 2026 +0200 Rework the DOM default parser selection test. Splits the explicit factory class case out, collapses the selection cases into one test that observes the wrapped delegate, and gates both on Android, where the secure factory returns the platform implementation unwrapped. Assisted-By: Claude Fable 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01CLnTBsvmYtxzNTWVGNyz33 --- .../xml/secure/SecureDocumentBuilderFactory.java | 2 +- .../secure/SecureDocumentBuilderFactoryTest.java | 67 +++++++++++++++++----- 2 files changed, 55 insertions(+), 14 deletions(-) diff --git a/src/main/java/org/apache/commons/xml/secure/SecureDocumentBuilderFactory.java b/src/main/java/org/apache/commons/xml/secure/SecureDocumentBuilderFactory.java index b51ea87..d693412 100644 --- a/src/main/java/org/apache/commons/xml/secure/SecureDocumentBuilderFactory.java +++ b/src/main/java/org/apache/commons/xml/secure/SecureDocumentBuilderFactory.java @@ -182,7 +182,7 @@ public void setXIncludeAware(final boolean state) { private static final String DOM_FACTORY_ID = "javax.xml.parsers.DocumentBuilderFactory"; /** Class name of the JDK's built-in default implementation, the Java 8 fallback for {@link #newDefaultInstance()}. */ - private static final String JDK_DOCUMENT_BUILDER_FACTORY = "com.sun.org.apache.xerces.internal.jaxp.DocumentBuilderFactoryImpl"; + static final String JDK_DOCUMENT_BUILDER_FACTORY = "com.sun.org.apache.xerces.internal.jaxp.DocumentBuilderFactoryImpl"; private static final MethodHandle MH_newDefaultInstance = MethodHandleFactory.findStatic(DocumentBuilderFactory.class, "newDefaultInstance"); diff --git a/src/test/java/org/apache/commons/xml/secure/SecureDocumentBuilderFactoryTest.java b/src/test/java/org/apache/commons/xml/secure/SecureDocumentBuilderFactoryTest.java index c2f8e57..e7899a8 100644 --- a/src/test/java/org/apache/commons/xml/secure/SecureDocumentBuilderFactoryTest.java +++ b/src/test/java/org/apache/commons/xml/secure/SecureDocumentBuilderFactoryTest.java @@ -24,6 +24,8 @@ import static org.junit.jupiter.api.Assertions.assertNull; import static org.junit.jupiter.api.Assertions.assertTrue; +import java.lang.reflect.Field; + import javax.xml.XMLConstants; import javax.xml.parsers.DocumentBuilderFactory; @@ -34,6 +36,37 @@ @Tag("dom") class SecureDocumentBuilderFactoryTest { + /** System property naming the {@link DocumentBuilderFactory} implementation, the JVM's mechanism for reconfiguring the default parser. */ + private static final String FACTORY_ID = "javax.xml.parsers.DocumentBuilderFactory"; + + /** + * Gets the implementation a secure factory delegates to, so the selection tests can observe which parser implementation a lookup picked. + * + * @param factory a secure factory returned by one of the {@code new*Instance} methods; never {@code null}. + * @return The wrapped factory. + */ + private static DocumentBuilderFactory getDelegate(final DocumentBuilderFactory factory) throws ReflectiveOperationException { + final Field delegate = factory.getClass().getDeclaredField("delegate"); + delegate.setAccessible(true); + return (DocumentBuilderFactory) delegate.get(factory); + } + + /** + * Selects the implementation {@link DocumentBuilderFactory#newInstance()} returns by setting the {@value #FACTORY_ID} system property. + * + * @param factoryClassName the implementation class name to install, or {@code null} to clear the property and restore the platform lookup. + * @return The previous property value, {@code null} if it was not set; pass it back here to restore the original lookup. + */ + private static String setFactoryIdProperty(final String factoryClassName) { + final String previous = System.getProperty(FACTORY_ID); + if (factoryClassName == null) { + System.clearProperty(FACTORY_ID); + } else { + System.setProperty(FACTORY_ID, factoryClassName); + } + return previous; + } + @Test void createsSecureBuildersFromEveryStaticEntryPoint() throws Exception { Assumptions.assumeTrue(AttackTestSupport.DOM_RESOLVES_INTERNAL_ENTITIES, "the platform DOM is left unwrapped: it does not resolve user-defined entities"); @@ -43,6 +76,15 @@ void createsSecureBuildersFromEveryStaticEntryPoint() throws Exception { assertInstanceOf(SecureDocumentBuilder.class, SecureDocumentBuilderFactory.newDefaultNSInstance().newDocumentBuilder()); } + @Test + void explicitFactoryClassSelectsThatImplementation() throws Exception { + Assumptions.assumeFalse(AttackTestSupport.IS_ANDROID, "Skipped on Android: the platform factory is used unwrapped"); + final Class<?> discovered = DocumentBuilderFactory.newInstance().getClass(); + final DocumentBuilderFactory factory = SecureDocumentBuilderFactory.newNSInstance(discovered.getName(), null); + assertEquals(discovered, getDelegate(factory).getClass()); + assertTrue(factory.isNamespaceAware()); + } + @Test void forwardsEverySupportedFactoryConfiguration() throws Exception { Assumptions.assumeTrue(AttackTestSupport.DOM_RESOLVES_INTERNAL_ENTITIES, "the platform DOM is left unwrapped: it does not resolve user-defined entities"); @@ -71,21 +113,20 @@ void forwardsEverySupportedFactoryConfiguration() throws Exception { } @Test - void honorsExplicitFactoryClassAndDefaultParserOverrides() throws Exception { - final String className = DocumentBuilderFactory.newInstance().getClass().getName(); - assertTrue(SecureDocumentBuilderFactory.newNSInstance(className, null).isNamespaceAware()); - assertTrue(SecureDocumentBuilderFactory.newNSInstance(true).isNamespaceAware()); - final String property = "javax.xml.parsers.DocumentBuilderFactory"; - final String previous = System.getProperty(property); + void newNSInstanceFollowsParserSelection() throws Exception { + Assumptions.assumeFalse(AttackTestSupport.IS_ANDROID, "Skipped on Android: the platform factory is used unwrapped"); + final Class<?> discovered = DocumentBuilderFactory.newInstance().getClass(); + // no property: the JDK built-in default, unless an override is requested + assertEquals(SecureDocumentBuilderFactory.JDK_DOCUMENT_BUILDER_FACTORY, + getDelegate(SecureDocumentBuilderFactory.newNSInstance(false)).getClass().getName()); + assertEquals(discovered, getDelegate(SecureDocumentBuilderFactory.newNSInstance(true)).getClass()); + // the factory id property is the JDK's own default reconfiguration; both selections honor it + final String previous = setFactoryIdProperty(discovered.getName()); try { - System.setProperty(property, className); - assertTrue(SecureDocumentBuilderFactory.newNSInstance(false).isNamespaceAware()); + assertEquals(discovered, getDelegate(SecureDocumentBuilderFactory.newNSInstance(false)).getClass()); + assertEquals(discovered, getDelegate(SecureDocumentBuilderFactory.newNSInstance(true)).getClass()); } finally { - if (previous == null) { - System.clearProperty(property); - } else { - System.setProperty(property, previous); - } + setFactoryIdProperty(previous); } } }
