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 60a0d2a600814cad94fc165dbd87dbb14ea67ef1 Author: Piotr P. Karwasz <[email protected]> AuthorDate: Tue Sep 1 08:44:15 2026 +0200 Rework the SAX default parser selection tests. Collapses the selection cases into one test that observes the wrapped delegate, covers every exception declared on the newSAXParser().getXMLReader() chain, and gates the tests that rely on the javax.xml.parsers.SAXParserFactory system property on Android, where the standard lookup is pinned to the platform implementation. Assisted-By: Claude Fable 5 <[email protected]> Claude-Session: https://claude.ai/code/session_01CLnTBsvmYtxzNTWVGNyz33 --- .../commons/xml/secure/SecureSAXParserFactory.java | 2 +- .../xml/secure/SecureSAXParserFactoryTest.java | 131 ++++++++++++++++----- 2 files changed, 103 insertions(+), 30 deletions(-) diff --git a/src/main/java/org/apache/commons/xml/secure/SecureSAXParserFactory.java b/src/main/java/org/apache/commons/xml/secure/SecureSAXParserFactory.java index a4d3c1f..06d320c 100644 --- a/src/main/java/org/apache/commons/xml/secure/SecureSAXParserFactory.java +++ b/src/main/java/org/apache/commons/xml/secure/SecureSAXParserFactory.java @@ -164,7 +164,7 @@ public void setXIncludeAware(final boolean state) { private static final String ANDROID_SAX_PARSER_FACTORY = "org.apache.harmony.xml.parsers.SAXParserFactoryImpl"; /** Class name of the JDK's built-in default implementation, the Java 8 fallback for {@link #newDefaultInstance()}. */ - private static final String JDK_SAX_PARSER_FACTORY = "com.sun.org.apache.xerces.internal.jaxp.SAXParserFactoryImpl"; + static final String JDK_SAX_PARSER_FACTORY = "com.sun.org.apache.xerces.internal.jaxp.SAXParserFactoryImpl"; /** * The JDK feature governing whether an implementation's internal parser lookup may resolve a third-party parser. The secure wrappers parse every source diff --git a/src/test/java/org/apache/commons/xml/secure/SecureSAXParserFactoryTest.java b/src/test/java/org/apache/commons/xml/secure/SecureSAXParserFactoryTest.java index 14c06a7..c17f61d 100644 --- a/src/test/java/org/apache/commons/xml/secure/SecureSAXParserFactoryTest.java +++ b/src/test/java/org/apache/commons/xml/secure/SecureSAXParserFactoryTest.java @@ -17,6 +17,7 @@ package org.apache.commons.xml.secure; +import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertInstanceOf; import static org.junit.jupiter.api.Assertions.assertNotNull; @@ -28,6 +29,7 @@ import static org.mockito.Mockito.when; import java.io.StringReader; +import java.lang.reflect.Field; import javax.xml.XMLConstants; import javax.xml.parsers.ParserConfigurationException; @@ -38,9 +40,11 @@ import javax.xml.transform.sax.SAXSource; import javax.xml.transform.stream.StreamSource; +import org.junit.jupiter.api.Assumptions; import org.junit.jupiter.api.Tag; import org.junit.jupiter.api.Test; import org.xml.sax.InputSource; +import org.xml.sax.SAXException; import org.xml.sax.XMLReader; @Tag("sax") @@ -69,6 +73,49 @@ public void setFeature(final String name, final boolean value) { } } + /** System property naming the {@link SAXParserFactory} implementation, the JVM's mechanism for reconfiguring the default parser. */ + private static final String FACTORY_ID = "javax.xml.parsers.SAXParserFactory"; + + /** + * Asserts {@link SecureSAXParserFactory#newXMLReader(boolean)} on the given delegate throws {@link IllegalStateException} with the given cause. + * + * @param cause the checked exception the delegate is stubbed to throw. + * @param delegate the stubbed factory to route {@link MockSAXParserFactory} to. + */ + private static void assertNewXmlReaderWraps(final Exception cause, final SAXParserFactory delegate) { + MockSAXParserFactory.delegate = delegate; + final IllegalStateException exception = assertThrows(IllegalStateException.class, () -> SecureSAXParserFactory.newXMLReader(false)); + assertSame(cause, exception.getCause()); + } + + /** + * 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 SAXParserFactory getDelegate(final SAXParserFactory factory) throws ReflectiveOperationException { + final Field delegate = factory.getClass().getDeclaredField("delegate"); + delegate.setAccessible(true); + return (SAXParserFactory) delegate.get(factory); + } + + /** + * Selects the implementation {@link SAXParserFactory#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 createsSecureParsersFromEveryStaticEntryPoint() throws Exception { assertNotNull(SecureSAXParserFactory.newInstance().newSAXParser()); @@ -100,47 +147,73 @@ void forwardsFactoryConfigurationAndCreatesNamespaceAwareParsers() throws Except } @Test - void newXmlReaderWrapsParserConfigurationException() throws Exception { - final ParserConfigurationException cause = new ParserConfigurationException("test"); - MockSAXParserFactory.delegate = mock(SAXParserFactory.class); - when(MockSAXParserFactory.delegate.newSAXParser()).thenThrow(cause); - final String factoryId = "javax.xml.parsers.SAXParserFactory"; - final String previous = System.getProperty(factoryId); + void leavesReadersSecureOnlyOnce() { + final XMLReader reader = SecureSAXParserFactory.newXMLReader(false); + assertSame(reader, SecureSAXParserFactory.secure(reader)); + } + + @Test + void newNSInstanceFollowsParserSelection() throws Exception { + Assumptions.assumeFalse(AttackTestSupport.IS_ANDROID, "Skipped on Android: parser selection is pinned to the platform implementation"); + final Class<?> discovered = SAXParserFactory.newInstance().getClass(); + // no property: the JDK built-in default, unless an override is requested + assertEquals(SecureSAXParserFactory.JDK_SAX_PARSER_FACTORY, getDelegate(SecureSAXParserFactory.newNSInstance(false)).getClass().getName()); + assertEquals(discovered, getDelegate(SecureSAXParserFactory.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(factoryId, MockSAXParserFactory.class.getName()); - final IllegalStateException exception = assertThrows(IllegalStateException.class, () -> SecureSAXParserFactory.newXMLReader(false)); - assertSame(cause, exception.getCause()); + assertEquals(discovered, getDelegate(SecureSAXParserFactory.newNSInstance(false)).getClass()); + assertEquals(discovered, getDelegate(SecureSAXParserFactory.newNSInstance(true)).getClass()); } finally { - if (previous == null) { - System.clearProperty(factoryId); - } else { - System.setProperty(factoryId, previous); - } - MockSAXParserFactory.delegate = null; + setFactoryIdProperty(previous); } } @Test - void respectsDefaultParserSelectionAndLeavesReadersSecureOnlyOnce() throws Exception { - final String factoryId = "SAXParserFactory"; - final String previous = System.getProperty(factoryId); + void newXmlReaderFollowsParserSelection() throws Exception { + Assumptions.assumeFalse(AttackTestSupport.IS_ANDROID, "Skipped on Android: parser selection is pinned to the platform implementation"); + final Class<?> discovered = SAXParserFactory.newInstance().newSAXParser().getXMLReader().getClass(); + assertEquals(discovered, ((SecureXMLReader) SecureSAXParserFactory.newXMLReader(true)).getDelegate().getClass()); + final Class<?> jdkReader = + SAXParserFactory.newInstance(SecureSAXParserFactory.JDK_SAX_PARSER_FACTORY, null).newSAXParser().getXMLReader().getClass(); + assertEquals(jdkReader, ((SecureXMLReader) SecureSAXParserFactory.newXMLReader(false)).getDelegate().getClass()); + final String previous = setFactoryIdProperty(SAXParserFactory.newInstance().getClass().getName()); try { - System.setProperty(factoryId, SAXParserFactory.newInstance().getClass().getName()); - assertTrue(SecureSAXParserFactory.newNSInstance(false).isNamespaceAware()); + assertEquals(discovered, ((SecureXMLReader) SecureSAXParserFactory.newXMLReader(false)).getDelegate().getClass()); } finally { - if (previous == null) { - System.clearProperty(factoryId); - } else { - System.setProperty(factoryId, previous); - } + setFactoryIdProperty(previous); + } + } + + @Test + void newXmlReaderWrapsDeclaredExceptions() throws Exception { + Assumptions.assumeFalse(AttackTestSupport.IS_ANDROID, "Skipped on Android: parser selection is pinned to the platform implementation"); + final String previous = setFactoryIdProperty(MockSAXParserFactory.class.getName()); + try { + // SAXParserFactory.newSAXParser() declares ParserConfigurationException and SAXException + final ParserConfigurationException notConfigurable = new ParserConfigurationException("test"); + SAXParserFactory factory = mock(SAXParserFactory.class); + when(factory.newSAXParser()).thenThrow(notConfigurable); + assertNewXmlReaderWraps(notConfigurable, factory); + final SAXException noParser = new SAXException("test"); + factory = mock(SAXParserFactory.class); + when(factory.newSAXParser()).thenThrow(noParser); + assertNewXmlReaderWraps(noParser, factory); + // SAXParser.getXMLReader() declares SAXException + final SAXException noReader = new SAXException("test"); + factory = mock(SAXParserFactory.class); + final SAXParser parser = mock(SAXParser.class); + when(factory.newSAXParser()).thenReturn(parser); + when(parser.getXMLReader()).thenThrow(noReader); + assertNewXmlReaderWraps(noReader, factory); + } finally { + setFactoryIdProperty(previous); + MockSAXParserFactory.delegate = null; } - assertTrue(SecureSAXParserFactory.newNSInstance(true).isNamespaceAware()); - final XMLReader reader = SecureSAXParserFactory.newXMLReader(false); - assertSame(reader, SecureSAXParserFactory.secure(reader)); } @Test - void securesOnlySourcesThatNeedAReader() throws Exception { + void securesOnlySourcesThatNeedAReader() { final StreamSource stream = new StreamSource(new StringReader("<root/>")); final Source securedStream = SecureSAXParserFactory.secure(stream, false); assertInstanceOf(SAXSource.class, securedStream);
