From 94ea2072560afedd4df295dd91e7af1f8369e8e1 Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Sat, 29 Aug 2026 15:00:56 +0200 Subject: [PATCH 1/4] Rework exception propagation when securing sources Source-securing failures used to make round trips between the SAX and TrAX exception hierarchies: a SAXException raised while provisioning a secure reader was wrapped into a TransformerConfigurationException and back into a SAXException on the validation and schema paths, and SecureXMLFilter.parse buried the parent reader's SAXParseException under SAXException(TransformerException(...)) for a downstream transformer to wrap yet again. Each hierarchy conversion now happens exactly once, at the API boundary whose checked signature demands it: - SecureSAXParserFactory.newXMLReader (formerly newSecureXMLReader) and secure(Source, boolean) declare their natural ParserConfigurationException/SAXException instead of a never-thrown TransformerConfigurationException. - SecureTransformerFactory.secure(Source, boolean) is the TrAX flavor, converting once to TransformerConfigurationException for the TrAX wrappers. - FallbackIgnoreURIResolver converts locally to TransformerException, keeping the TrAX wrappers out of the XPath shading closure. - SecureXMLFilter.parse rethrows a SAXException or IOException cause of the transform's TransformerException directly, TrAX-filter style, so the original SAXParseException or handler exception surfaces as-is. Assisted-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01RZSVucNBf5fsyd1uqamLuk --- .../xml/FallbackIgnoreURIResolver.java | 8 +++- .../commons/xml/SecureSAXParserFactory.java | 26 ++++++------ .../commons/xml/SecureSchemaFactory.java | 4 +- .../apache/commons/xml/SecureTransformer.java | 4 +- .../commons/xml/SecureTransformerFactory.java | 36 ++++++++++++---- .../apache/commons/xml/SecureValidator.java | 4 +- .../apache/commons/xml/SecureXMLFilter.java | 13 +++++- .../xml/OverrideDefaultParserTest.java | 4 +- .../org/apache/commons/xml/XMLFilterTest.java | 42 +++++++++++++++++++ 9 files changed, 108 insertions(+), 33 deletions(-) diff --git a/src/main/java/org/apache/commons/xml/FallbackIgnoreURIResolver.java b/src/main/java/org/apache/commons/xml/FallbackIgnoreURIResolver.java index ccef795d..ae6fc543 100644 --- a/src/main/java/org/apache/commons/xml/FallbackIgnoreURIResolver.java +++ b/src/main/java/org/apache/commons/xml/FallbackIgnoreURIResolver.java @@ -29,6 +29,7 @@ import javax.xml.transform.dom.DOMSource; import org.w3c.dom.Document; +import org.xml.sax.SAXException; /** * {@link URIResolver} floor: consults an optional caller-supplied resolver and ignores (resolves to empty) whatever the caller does not resolve. @@ -125,7 +126,12 @@ public Source resolve(final String href, final String base) throws TransformerEx final Source resolved = delegate != null ? delegate.resolve(href, base) : null; if (resolved != null) { // The implementation parses the opted-in handle with an internal reader at its own defaults; the rewrite hands it a secure reader instead. - return SecureSAXParserFactory.secure(resolved, overrideDefaultParser.getAsBoolean()); + // Converted locally, not via SecureTransformerFactory.secure: this class is in the XPath shading closure, which must not pull the TrAX wrappers. + try { + return SecureSAXParserFactory.secure(resolved, overrideDefaultParser.getAsBoolean()); + } catch (ParserConfigurationException | SAXException e) { + throw new TransformerException(e); + } } if (SecureException.throwOnUnresolved()) { throw new TransformerException(SecureException.forbidden("uri", null, null, href, base)); diff --git a/src/main/java/org/apache/commons/xml/SecureSAXParserFactory.java b/src/main/java/org/apache/commons/xml/SecureSAXParserFactory.java index d12b188a..9db1f16a 100644 --- a/src/main/java/org/apache/commons/xml/SecureSAXParserFactory.java +++ b/src/main/java/org/apache/commons/xml/SecureSAXParserFactory.java @@ -27,7 +27,6 @@ import javax.xml.parsers.SAXParser; import javax.xml.parsers.SAXParserFactory; import javax.xml.transform.Source; -import javax.xml.transform.TransformerConfigurationException; import javax.xml.transform.sax.SAXSource; import javax.xml.transform.stream.StreamSource; import javax.xml.validation.Schema; @@ -301,16 +300,13 @@ public static SAXParserFactory newNSInstance(final String factoryClassName, fina * * @param overrideDefaultParser whether {@value #OVERRIDE_DEFAULT_PARSER} on the originating factory asks to override the JDK's default parser. * @return a secure reader. - * @throws TransformerConfigurationException if a secure reader cannot be obtained. + * @throws ParserConfigurationException Thrown if the factory cannot produce a parser satisfying its configuration. + * @throws SAXException Thrown if the parser cannot provide a reader. * @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. */ - static XMLReader newSecureXMLReader(final boolean overrideDefaultParser) throws TransformerConfigurationException { - try { - return newNSInstance(overrideDefaultParser).newSAXParser().getXMLReader(); - } catch (final ParserConfigurationException | SAXException e) { - throw new TransformerConfigurationException("Failed to obtain a secure XMLReader for source parsing", e); - } + static XMLReader newXMLReader(final boolean overrideDefaultParser) throws ParserConfigurationException, SAXException { + return newNSInstance(overrideDefaultParser).newSAXParser().getXMLReader(); } /** @@ -349,20 +345,22 @@ static SAXParserFactory secure(final SAXParserFactory factory) { * Rewrites a {@link Source} so that any SAX parsing it triggers runs through a secure {@link XMLReader}. *

* Only a {@link StreamSource} or a {@link SAXSource} without a reader is enriched with a secure, namespace-aware reader; other source kinds are returned - * as-is. Used by the TrAX and schema wrappers to route every source they parse through the SAX secure path. + * as-is. Used by the schema wrappers to route every source they parse through the SAX secure path; the TrAX wrappers convert the exceptions through + * {@link SecureTransformerFactory#secure(Source, boolean)}. *

* * @param source the source to secure; never {@code null}. * @param overrideDefaultParser whether {@value #OVERRIDE_DEFAULT_PARSER} on the originating factory asks to override the JDK's default parser. * @return a secure source. - * @throws TransformerConfigurationException if a secure reader cannot be obtained. - * @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. + * @throws ParserConfigurationException Thrown if the factory cannot produce a parser satisfying its configuration. + * @throws SAXException Thrown if the parser cannot provide a reader. + * @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. */ - static Source secure(final Source source, final boolean overrideDefaultParser) throws TransformerConfigurationException { + static Source secure(final Source source, final boolean overrideDefaultParser) throws ParserConfigurationException, SAXException { if (source instanceof StreamSource || source instanceof SAXSource && ((SAXSource) source).getXMLReader() == null) { final InputSource inputSource = SAXSource.sourceToInputSource(source); - return inputSource == null ? source : new SAXSource(newSecureXMLReader(overrideDefaultParser), inputSource); + return inputSource == null ? source : new SAXSource(newXMLReader(overrideDefaultParser), inputSource); } return source; } diff --git a/src/main/java/org/apache/commons/xml/SecureSchemaFactory.java b/src/main/java/org/apache/commons/xml/SecureSchemaFactory.java index a16cd2e5..4fbbd728 100644 --- a/src/main/java/org/apache/commons/xml/SecureSchemaFactory.java +++ b/src/main/java/org/apache/commons/xml/SecureSchemaFactory.java @@ -23,8 +23,8 @@ import javax.xml.XMLConstants; import javax.xml.parsers.FactoryConfigurationError; +import javax.xml.parsers.ParserConfigurationException; import javax.xml.transform.Source; -import javax.xml.transform.TransformerConfigurationException; import javax.xml.validation.Schema; import javax.xml.validation.SchemaFactory; import javax.xml.validation.SchemaFactoryConfigurationError; @@ -176,7 +176,7 @@ private Source[] secure(final Source[] schemas) throws SAXException { for (int i = 0; i < schemas.length; i++) { secure[i] = SecureSAXParserFactory.secure(schemas[i], overrideDefaultParser); } - } catch (final TransformerConfigurationException e) { + } catch (final ParserConfigurationException e) { throw new SAXException("Failed to secure schema source", e); } return secure; diff --git a/src/main/java/org/apache/commons/xml/SecureTransformer.java b/src/main/java/org/apache/commons/xml/SecureTransformer.java index bfcdeaa7..bcc79479 100644 --- a/src/main/java/org/apache/commons/xml/SecureTransformer.java +++ b/src/main/java/org/apache/commons/xml/SecureTransformer.java @@ -32,7 +32,7 @@ /** * {@link Transformer} wrapper that rewrites the Source on every {@link Transformer#transform(Source, Result)} call through - * {@link SecureSAXParserFactory#secure(Source, boolean)} before delegating, and keeps an ignore-all {@link URIResolver} floor so runtime {@code document()} calls a + * {@link SecureTransformerFactory#secure(Source, boolean)} before delegating, and keeps an ignore-all {@link URIResolver} floor so runtime {@code document()} calls a * caller does not resolve return empty rather than being fetched. *

* The floor is installed on the delegate transformer at construction, seeded with the factory's compile-time resolver; {@link #setURIResolver(URIResolver)} @@ -145,6 +145,6 @@ public void setURIResolver(final URIResolver resolver) { */ @Override public void transform(final Source xmlSource, final Result outputTarget) throws TransformerException { - delegate.transform(SecureSAXParserFactory.secure(xmlSource, overrideDefaultParser), outputTarget); + delegate.transform(SecureTransformerFactory.secure(xmlSource, overrideDefaultParser), outputTarget); } } diff --git a/src/main/java/org/apache/commons/xml/SecureTransformerFactory.java b/src/main/java/org/apache/commons/xml/SecureTransformerFactory.java index f6db179f..946890e4 100644 --- a/src/main/java/org/apache/commons/xml/SecureTransformerFactory.java +++ b/src/main/java/org/apache/commons/xml/SecureTransformerFactory.java @@ -77,7 +77,7 @@ public final class SecureTransformerFactory { /** - * {@link TransformerFactory} wrapper that rewrites every Source-taking entry point through {@link SecureSAXParserFactory#secure(Source, boolean)} before + * {@link TransformerFactory} wrapper that rewrites every Source-taking entry point through {@link SecureTransformerFactory#secure(Source, boolean)} before * delegating. * *

Used by providers whose underlying TrAX implementation pulls a new {@code SAXParserFactory.newInstance()} for any Source that is not already a @@ -190,7 +190,7 @@ private Wrapper(final SAXTransformerFactory delegate, final Supplier emp public Source getAssociatedStylesheet(final Source source, final String media, final String title, final String charset) throws TransformerConfigurationException { // Xalan's getAssociatedStylesheet drops a SAXSource's reader and self-provisions its own to scan for xml-stylesheet PIs (XALANJ-2849). - final Source secure = isXalan(delegate) ? secureSourceToDom(source) : SecureSAXParserFactory.secure(source, overrideDefaultParser()); + final Source secure = isXalan(delegate) ? secureSourceToDom(source) : SecureTransformerFactory.secure(source, overrideDefaultParser()); return delegate.getAssociatedStylesheet(secure, media, title, charset); } @@ -221,10 +221,10 @@ private TransformerHandler secure(final TransformerHandler handler) { /** * Parses a reader-less source into a DOM through a secure, namespace-aware {@link javax.xml.parsers.DocumentBuilder} and returns a {@link DOMSource} * carrying its system id, so the consumer walks the tree instead of provisioning its own reader. Any other source is left to - * {@link SecureSAXParserFactory#secure(Source, boolean)}. + * {@link SecureTransformerFactory#secure(Source, boolean)}. * * @param source The source to scan for an associated stylesheet. - * @return A {@link DOMSource} for a reader-less source, otherwise the result of {@link SecureSAXParserFactory#secure(Source, boolean)}. + * @return A {@link DOMSource} for a reader-less source, otherwise the result of {@link SecureTransformerFactory#secure(Source, boolean)}. * @throws TransformerConfigurationException if the source cannot be parsed. * @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. @@ -243,7 +243,7 @@ private Source secureSourceToDom(final Source source) throws TransformerConfigur } } } - return SecureSAXParserFactory.secure(source, overrideDefaultParser()); + return SecureTransformerFactory.secure(source, overrideDefaultParser()); } /** @@ -254,7 +254,7 @@ private Source secureSourceToDom(final Source source) throws TransformerConfigur */ @Override public Templates newTemplates(final Source source) throws TransformerConfigurationException { - final Templates templates = delegate.newTemplates(SecureSAXParserFactory.secure(source, overrideDefaultParser())); + final Templates templates = delegate.newTemplates(SecureTransformerFactory.secure(source, overrideDefaultParser())); return templates == null ? null : new SecureTemplates(templates, getURIResolver(), emptySource, overrideDefaultParser()); } @@ -279,7 +279,7 @@ public Transformer newTransformer() throws TransformerConfigurationException { */ @Override public Transformer newTransformer(final Source source) throws TransformerConfigurationException { - final Transformer transformer = delegate.newTransformer(SecureSAXParserFactory.secure(source, overrideDefaultParser())); + final Transformer transformer = delegate.newTransformer(SecureTransformerFactory.secure(source, overrideDefaultParser())); return transformer == null ? null : new SecureTransformer(transformer, getURIResolver(), emptySource, overrideDefaultParser()); } @@ -296,7 +296,7 @@ public TransformerHandler newTransformerHandler() throws TransformerConfiguratio */ @Override public TransformerHandler newTransformerHandler(final Source source) throws TransformerConfigurationException { - return secure(delegate.newTransformerHandler(SecureSAXParserFactory.secure(source, overrideDefaultParser()))); + return secure(delegate.newTransformerHandler(SecureTransformerFactory.secure(source, overrideDefaultParser()))); } @Override @@ -430,6 +430,26 @@ public static TransformerFactory newInstance(final String factoryClassName, fina * @param factory the factory to secure; never {@code null}. * @return a secure factory. */ + /** + * TrAX flavor of {@link SecureSAXParserFactory#secure(Source, boolean)}: the same rewrite, with the SAX-side exceptions converted once into the + * {@link TransformerConfigurationException} the TrAX signatures demand. + * + * @param source the source to secure; never {@code null}. + * @param overrideDefaultParser whether {@value SecureSAXParserFactory#OVERRIDE_DEFAULT_PARSER} on the originating factory asks to override the JDK's + * default parser. + * @return a secure source. + * @throws TransformerConfigurationException if a secure reader cannot be obtained. + * @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. + */ + static Source secure(final Source source, final boolean overrideDefaultParser) throws TransformerConfigurationException { + try { + return SecureSAXParserFactory.secure(source, overrideDefaultParser); + } catch (ParserConfigurationException | SAXException e) { + throw new TransformerConfigurationException(e); + } + } + static TransformerFactory secure(final TransformerFactory factory) { // Required: enables secure processing (XSLTC runtime limits; Xalan's extension-function block). setFeature(factory, XMLConstants.FEATURE_SECURE_PROCESSING, true); diff --git a/src/main/java/org/apache/commons/xml/SecureValidator.java b/src/main/java/org/apache/commons/xml/SecureValidator.java index 6497b2f2..033ddf9f 100644 --- a/src/main/java/org/apache/commons/xml/SecureValidator.java +++ b/src/main/java/org/apache/commons/xml/SecureValidator.java @@ -21,9 +21,9 @@ import java.util.Objects; import javax.xml.parsers.FactoryConfigurationError; +import javax.xml.parsers.ParserConfigurationException; import javax.xml.transform.Result; import javax.xml.transform.Source; -import javax.xml.transform.TransformerConfigurationException; import javax.xml.validation.Validator; import org.w3c.dom.ls.LSResourceResolver; @@ -122,7 +122,7 @@ public void setResourceResolver(final LSResourceResolver resourceResolver) { public void validate(final Source source, final Result result) throws SAXException, IOException { try { delegate.validate(SecureSAXParserFactory.secure(source, overrideDefaultParser), result); - } catch (final TransformerConfigurationException e) { + } catch (final ParserConfigurationException e) { throw new SAXException("Failed to secure source for validation", e); } } diff --git a/src/main/java/org/apache/commons/xml/SecureXMLFilter.java b/src/main/java/org/apache/commons/xml/SecureXMLFilter.java index 56c294be..3504699d 100644 --- a/src/main/java/org/apache/commons/xml/SecureXMLFilter.java +++ b/src/main/java/org/apache/commons/xml/SecureXMLFilter.java @@ -21,6 +21,7 @@ import java.util.Objects; import javax.xml.parsers.FactoryConfigurationError; +import javax.xml.parsers.ParserConfigurationException; import javax.xml.transform.Transformer; import javax.xml.transform.TransformerException; import javax.xml.transform.sax.SAXResult; @@ -70,8 +71,8 @@ public void parse(final InputSource input) throws SAXException, IOException { } if (getParent() == null) { try { - setParent(SecureSAXParserFactory.newSecureXMLReader(templates.overrideDefaultParser)); - } catch (final TransformerException e) { + setParent(SecureSAXParserFactory.newXMLReader(templates.overrideDefaultParser)); + } catch (final ParserConfigurationException e) { throw new SAXException(e); } } @@ -84,6 +85,14 @@ public void parse(final InputSource input) throws SAXException, IOException { final Transformer transformer = templates.newTransformer(); transformer.transform(new SAXSource(getParent(), input), result); } catch (final TransformerException e) { + // The parent reader's parse errors and the handler's own exceptions arrive wrapped; rethrow the original rather than nesting the hierarchies. + final Throwable cause = e.getCause(); + if (cause instanceof SAXException) { + throw (SAXException) cause; + } + if (cause instanceof IOException) { + throw (IOException) cause; + } throw new SAXException(e); } } diff --git a/src/test/java/org/apache/commons/xml/OverrideDefaultParserTest.java b/src/test/java/org/apache/commons/xml/OverrideDefaultParserTest.java index cf3e5d39..95af0d80 100644 --- a/src/test/java/org/apache/commons/xml/OverrideDefaultParserTest.java +++ b/src/test/java/org/apache/commons/xml/OverrideDefaultParserTest.java @@ -83,9 +83,9 @@ void schemaFactoryReadsFeatureAtCreation() throws Exception { @Test void secureReaderFollowsFlag() throws Exception { assumeFalse(AttackTestSupport.IS_ANDROID); - final XMLReader pinned = ((SecureXMLReader) SecureSAXParserFactory.newSecureXMLReader(false)).getDelegate(); + final XMLReader pinned = ((SecureXMLReader) SecureSAXParserFactory.newXMLReader(false)).getDelegate(); assertTrue(pinned.getClass().getName().startsWith(JDK_INTERNAL_PREFIX), pinned.getClass().getName()); - final XMLReader pluggable = ((SecureXMLReader) SecureSAXParserFactory.newSecureXMLReader(true)).getDelegate(); + final XMLReader pluggable = ((SecureXMLReader) SecureSAXParserFactory.newXMLReader(true)).getDelegate(); final XMLReader lookedUp = ((SecureXMLReader) SecureSAXParserFactory.newNSInstance().newSAXParser().getXMLReader()).getDelegate(); assertEquals(lookedUp.getClass(), pluggable.getClass()); if (xercesOnClasspath()) { diff --git a/src/test/java/org/apache/commons/xml/XMLFilterTest.java b/src/test/java/org/apache/commons/xml/XMLFilterTest.java index fde4ea98..3470c22b 100644 --- a/src/test/java/org/apache/commons/xml/XMLFilterTest.java +++ b/src/test/java/org/apache/commons/xml/XMLFilterTest.java @@ -19,6 +19,7 @@ import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; +import static org.junit.jupiter.api.Assertions.assertSame; import static org.junit.jupiter.api.Assertions.assertTrue; import java.io.StringReader; @@ -32,7 +33,9 @@ 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.XMLFilter; +import org.xml.sax.helpers.DefaultHandler; /** * {@link XMLFilter} products of the secure factory: the input document is parsed by a secure reader (never a self-provisioned permissive one), and the @@ -48,6 +51,13 @@ class XMLFilterTest { + " \n" + ""; + /** Asserts no SAXException is buried beneath the thrown one, proving {@code parse} rethrows originals instead of re-wrapping them. */ + private static void assertNotReWrapped(final SAXException thrown) { + for (Throwable cause = thrown.getCause(); cause != null; cause = cause.getCause()) { + assertFalse(cause instanceof SAXException, "original SAXException should be rethrown, not re-wrapped: " + thrown); + } + } + private static String entityPayload() { return "\n" + ""))); + } catch (final SAXException e) { + assertNotReWrapped(e); + } + } + @Test void secureFilterFromTemplatesDoesNotLeakDocument() throws Exception { final SAXTransformerFactory factory = SaxSurfaceTestSupport.secureFactory(); @@ -96,6 +119,25 @@ void secureFilterFromTemplatesDoesNotLeakDocument() throws Exception { assertFalse(filterAndCapture(filter, "").contains(AttackTestSupport.LEAKED_MARKER), "document() through XMLFilter(Templates) leaked"); } + @Test + void secureFilterRethrowsHandlerSAXException() throws Exception { + // Dual contract: the delegate transformer either swallows the handler's exception (Xalan) or surfaces it wrapped in a TransformerException; when it + // surfaces, parse must rethrow the original instance, not nest it under a new SAXException. + final XMLFilter filter = SaxSurfaceTestSupport.secureFactory().newXMLFilter(AttackTestSupport.streamSource(IDENTITY_XSLT)); + final SAXException handlerFailure = new SAXException("handler failure"); + filter.setContentHandler(new DefaultHandler() { + @Override + public void startDocument() throws SAXException { + throw handlerFailure; + } + }); + try { + filter.parse(new InputSource(new StringReader(""))); + } catch (final SAXException e) { + assertSame(handlerFailure, e, "the handler's own SAXException should surface unwrapped"); + } + } + @Test void unconfiguredFilterLeaksDocument() throws Exception { final SAXTransformerFactory factory = (SAXTransformerFactory) TransformerFactory.newInstance(); From 4a8ea614182ddbb663bdf8c8d8c338042c8e7203 Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Sat, 29 Aug 2026 20:19:51 +0200 Subject: [PATCH 2/4] Wrap reader-provisioning failures in SecureException; simplify newXMLReader (formerly newSecureXMLReader) does not normally throw: every supported implementation provides a reader as a routine capability, so a ParserConfigurationException or SAXException there signals a broken environment, not a per-parse condition. Wrap it in the unchecked SecureException instead of TransformerConfigurationException and drop the checked-exception plumbing this branch had introduced: the secure/secureTraX split, the SecureTransformerFactory.secure(Source) wrapper and the per-caller try/catch in SecureValidator, SecureSchemaFactory and FallbackIgnoreURIResolver all revert to plain calls. SecureXMLFilter now performs the XMLFilterImpl.setupParse wiring for the resolver, DTD and error callbacks (the transformer owns the parent's ContentHandler), implements ErrorListener to forward TrAX error reports to the caller-set ErrorHandler, and rethrows the SAXException or IOException cause of a transform failure instead of nesting the hierarchies, so the original SAXParseException surfaces as-is and no implementation can end the parse silently. Assisted-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01RZSVucNBf5fsyd1uqamLuk --- .../xml/FallbackIgnoreURIResolver.java | 8 +- .../apache/commons/xml/SecureException.java | 16 +++- .../commons/xml/SecureSAXParserFactory.java | 20 ++--- .../commons/xml/SecureSchemaFactory.java | 13 +--- .../apache/commons/xml/SecureTransformer.java | 10 +-- .../commons/xml/SecureTransformerFactory.java | 36 ++------- .../apache/commons/xml/SecureValidator.java | 7 +- .../apache/commons/xml/SecureXMLFilter.java | 77 ++++++++++++++++--- .../org/apache/commons/xml/XMLFilterTest.java | 27 ++++--- 9 files changed, 126 insertions(+), 88 deletions(-) diff --git a/src/main/java/org/apache/commons/xml/FallbackIgnoreURIResolver.java b/src/main/java/org/apache/commons/xml/FallbackIgnoreURIResolver.java index ae6fc543..ccef795d 100644 --- a/src/main/java/org/apache/commons/xml/FallbackIgnoreURIResolver.java +++ b/src/main/java/org/apache/commons/xml/FallbackIgnoreURIResolver.java @@ -29,7 +29,6 @@ import javax.xml.transform.dom.DOMSource; import org.w3c.dom.Document; -import org.xml.sax.SAXException; /** * {@link URIResolver} floor: consults an optional caller-supplied resolver and ignores (resolves to empty) whatever the caller does not resolve. @@ -126,12 +125,7 @@ public Source resolve(final String href, final String base) throws TransformerEx final Source resolved = delegate != null ? delegate.resolve(href, base) : null; if (resolved != null) { // The implementation parses the opted-in handle with an internal reader at its own defaults; the rewrite hands it a secure reader instead. - // Converted locally, not via SecureTransformerFactory.secure: this class is in the XPath shading closure, which must not pull the TrAX wrappers. - try { - return SecureSAXParserFactory.secure(resolved, overrideDefaultParser.getAsBoolean()); - } catch (ParserConfigurationException | SAXException e) { - throw new TransformerException(e); - } + return SecureSAXParserFactory.secure(resolved, overrideDefaultParser.getAsBoolean()); } if (SecureException.throwOnUnresolved()) { throw new TransformerException(SecureException.forbidden("uri", null, null, href, base)); diff --git a/src/main/java/org/apache/commons/xml/SecureException.java b/src/main/java/org/apache/commons/xml/SecureException.java index fe4848e0..67a45b14 100644 --- a/src/main/java/org/apache/commons/xml/SecureException.java +++ b/src/main/java/org/apache/commons/xml/SecureException.java @@ -20,10 +20,11 @@ /** * Thrown when a factory cannot be made secure. * - *

Two failure modes share this type:

+ *

Three failure modes share this type:

* * *

The message names the unsupported factory class or the specific feature, attribute or property that failed; the cause, when present, is the original @@ -71,6 +72,19 @@ static String forbidden(final String type, final String namespace, final String SecureException.THROW_ON_UNRESOLVED, type, namespace, publicId, systemId, baseURI); } + /** + * Builds the standard exception for a failed internal reader provisioning. + * + *

Every supported implementation provides a reader as a routine capability, so the wrapped {@code ParserConfigurationException} or + * {@code SAXException} signals a broken environment, not a per-parse condition — hence unchecked.

+ * + * @param cause the original checked exception from the JAXP implementation. + * @return the exception to throw. + */ + static SecureException readerFailed(final Throwable cause) { + return new SecureException("Failed to create a secure XMLReader", cause); + } + /** * Whether unresolved external references must be rejected instead of resolved to empty content. * diff --git a/src/main/java/org/apache/commons/xml/SecureSAXParserFactory.java b/src/main/java/org/apache/commons/xml/SecureSAXParserFactory.java index 9db1f16a..9c1275fb 100644 --- a/src/main/java/org/apache/commons/xml/SecureSAXParserFactory.java +++ b/src/main/java/org/apache/commons/xml/SecureSAXParserFactory.java @@ -300,13 +300,17 @@ public static SAXParserFactory newNSInstance(final String factoryClassName, fina * * @param overrideDefaultParser whether {@value #OVERRIDE_DEFAULT_PARSER} on the originating factory asks to override the JDK's default parser. * @return a secure reader. - * @throws ParserConfigurationException Thrown if the factory cannot produce a parser satisfying its configuration. - * @throws SAXException Thrown if the parser cannot provide a reader. + * @throws IllegalStateException Thrown if the underlying implementation cannot provide a secure reader; providing one is a routine capability of every + * supported implementation, so a failure signals a broken environment, not a per-parse condition. * @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. */ - static XMLReader newXMLReader(final boolean overrideDefaultParser) throws ParserConfigurationException, SAXException { - return newNSInstance(overrideDefaultParser).newSAXParser().getXMLReader(); + static XMLReader newXMLReader(final boolean overrideDefaultParser) { + try { + return newNSInstance(overrideDefaultParser).newSAXParser().getXMLReader(); + } catch (ParserConfigurationException | SAXException e) { + throw SecureException.readerFailed(e); + } } /** @@ -345,19 +349,17 @@ static SAXParserFactory secure(final SAXParserFactory factory) { * Rewrites a {@link Source} so that any SAX parsing it triggers runs through a secure {@link XMLReader}. *

* Only a {@link StreamSource} or a {@link SAXSource} without a reader is enriched with a secure, namespace-aware reader; other source kinds are returned - * as-is. Used by the schema wrappers to route every source they parse through the SAX secure path; the TrAX wrappers convert the exceptions through - * {@link SecureTransformerFactory#secure(Source, boolean)}. + * as-is. Used by the TrAX and schema wrappers to route every source they parse through the SAX secure path. *

* * @param source the source to secure; never {@code null}. * @param overrideDefaultParser whether {@value #OVERRIDE_DEFAULT_PARSER} on the originating factory asks to override the JDK's default parser. * @return a secure source. - * @throws ParserConfigurationException Thrown if the factory cannot produce a parser satisfying its configuration. - * @throws SAXException Thrown if the parser cannot provide a reader. + * @throws IllegalStateException Thrown if the underlying implementation cannot provide a secure reader. * @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. */ - static Source secure(final Source source, final boolean overrideDefaultParser) throws ParserConfigurationException, SAXException { + static Source secure(final Source source, final boolean overrideDefaultParser) { if (source instanceof StreamSource || source instanceof SAXSource && ((SAXSource) source).getXMLReader() == null) { final InputSource inputSource = SAXSource.sourceToInputSource(source); return inputSource == null ? source : new SAXSource(newXMLReader(overrideDefaultParser), inputSource); diff --git a/src/main/java/org/apache/commons/xml/SecureSchemaFactory.java b/src/main/java/org/apache/commons/xml/SecureSchemaFactory.java index 4fbbd728..76b47bc0 100644 --- a/src/main/java/org/apache/commons/xml/SecureSchemaFactory.java +++ b/src/main/java/org/apache/commons/xml/SecureSchemaFactory.java @@ -23,7 +23,6 @@ import javax.xml.XMLConstants; import javax.xml.parsers.FactoryConfigurationError; -import javax.xml.parsers.ParserConfigurationException; import javax.xml.transform.Source; import javax.xml.validation.Schema; import javax.xml.validation.SchemaFactory; @@ -165,19 +164,15 @@ private boolean overrideDefaultParser() { * * @param schemas the schema sources to secure; must not be {@code null}. * @return a new array of secure sources. - * @throws SAXException if any source cannot be secure. + * @throws IllegalStateException Thrown if the underlying implementation cannot provide a secure reader. * @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. */ - private Source[] secure(final Source[] schemas) throws SAXException { + private Source[] secure(final Source[] schemas) { final Source[] secure = new Source[schemas.length]; final boolean overrideDefaultParser = overrideDefaultParser(); - try { - for (int i = 0; i < schemas.length; i++) { - secure[i] = SecureSAXParserFactory.secure(schemas[i], overrideDefaultParser); - } - } catch (final ParserConfigurationException e) { - throw new SAXException("Failed to secure schema source", e); + for (int i = 0; i < schemas.length; i++) { + secure[i] = SecureSAXParserFactory.secure(schemas[i], overrideDefaultParser); } return secure; } diff --git a/src/main/java/org/apache/commons/xml/SecureTransformer.java b/src/main/java/org/apache/commons/xml/SecureTransformer.java index bcc79479..fc4898a5 100644 --- a/src/main/java/org/apache/commons/xml/SecureTransformer.java +++ b/src/main/java/org/apache/commons/xml/SecureTransformer.java @@ -32,7 +32,7 @@ /** * {@link Transformer} wrapper that rewrites the Source on every {@link Transformer#transform(Source, Result)} call through - * {@link SecureTransformerFactory#secure(Source, boolean)} before delegating, and keeps an ignore-all {@link URIResolver} floor so runtime {@code document()} calls a + * {@link SecureSAXParserFactory#secure(Source, boolean)} before delegating, and keeps an ignore-all {@link URIResolver} floor so runtime {@code document()} calls a * caller does not resolve return empty rather than being fetched. *

* The floor is installed on the delegate transformer at construction, seeded with the factory's compile-time resolver; {@link #setURIResolver(URIResolver)} @@ -139,12 +139,12 @@ public void setURIResolver(final URIResolver resolver) { /** * {@inheritDoc} * - * @throws TransformerConfigurationException Thrown if a secure reader cannot be obtained. - * @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. + * @throws IllegalStateException Thrown if the underlying implementation cannot provide a secure reader. + * @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. */ @Override public void transform(final Source xmlSource, final Result outputTarget) throws TransformerException { - delegate.transform(SecureTransformerFactory.secure(xmlSource, overrideDefaultParser), outputTarget); + delegate.transform(SecureSAXParserFactory.secure(xmlSource, overrideDefaultParser), outputTarget); } } diff --git a/src/main/java/org/apache/commons/xml/SecureTransformerFactory.java b/src/main/java/org/apache/commons/xml/SecureTransformerFactory.java index 946890e4..f6db179f 100644 --- a/src/main/java/org/apache/commons/xml/SecureTransformerFactory.java +++ b/src/main/java/org/apache/commons/xml/SecureTransformerFactory.java @@ -77,7 +77,7 @@ public final class SecureTransformerFactory { /** - * {@link TransformerFactory} wrapper that rewrites every Source-taking entry point through {@link SecureTransformerFactory#secure(Source, boolean)} before + * {@link TransformerFactory} wrapper that rewrites every Source-taking entry point through {@link SecureSAXParserFactory#secure(Source, boolean)} before * delegating. * *

Used by providers whose underlying TrAX implementation pulls a new {@code SAXParserFactory.newInstance()} for any Source that is not already a @@ -190,7 +190,7 @@ private Wrapper(final SAXTransformerFactory delegate, final Supplier emp public Source getAssociatedStylesheet(final Source source, final String media, final String title, final String charset) throws TransformerConfigurationException { // Xalan's getAssociatedStylesheet drops a SAXSource's reader and self-provisions its own to scan for xml-stylesheet PIs (XALANJ-2849). - final Source secure = isXalan(delegate) ? secureSourceToDom(source) : SecureTransformerFactory.secure(source, overrideDefaultParser()); + final Source secure = isXalan(delegate) ? secureSourceToDom(source) : SecureSAXParserFactory.secure(source, overrideDefaultParser()); return delegate.getAssociatedStylesheet(secure, media, title, charset); } @@ -221,10 +221,10 @@ private TransformerHandler secure(final TransformerHandler handler) { /** * Parses a reader-less source into a DOM through a secure, namespace-aware {@link javax.xml.parsers.DocumentBuilder} and returns a {@link DOMSource} * carrying its system id, so the consumer walks the tree instead of provisioning its own reader. Any other source is left to - * {@link SecureTransformerFactory#secure(Source, boolean)}. + * {@link SecureSAXParserFactory#secure(Source, boolean)}. * * @param source The source to scan for an associated stylesheet. - * @return A {@link DOMSource} for a reader-less source, otherwise the result of {@link SecureTransformerFactory#secure(Source, boolean)}. + * @return A {@link DOMSource} for a reader-less source, otherwise the result of {@link SecureSAXParserFactory#secure(Source, boolean)}. * @throws TransformerConfigurationException if the source cannot be parsed. * @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. @@ -243,7 +243,7 @@ private Source secureSourceToDom(final Source source) throws TransformerConfigur } } } - return SecureTransformerFactory.secure(source, overrideDefaultParser()); + return SecureSAXParserFactory.secure(source, overrideDefaultParser()); } /** @@ -254,7 +254,7 @@ private Source secureSourceToDom(final Source source) throws TransformerConfigur */ @Override public Templates newTemplates(final Source source) throws TransformerConfigurationException { - final Templates templates = delegate.newTemplates(SecureTransformerFactory.secure(source, overrideDefaultParser())); + final Templates templates = delegate.newTemplates(SecureSAXParserFactory.secure(source, overrideDefaultParser())); return templates == null ? null : new SecureTemplates(templates, getURIResolver(), emptySource, overrideDefaultParser()); } @@ -279,7 +279,7 @@ public Transformer newTransformer() throws TransformerConfigurationException { */ @Override public Transformer newTransformer(final Source source) throws TransformerConfigurationException { - final Transformer transformer = delegate.newTransformer(SecureTransformerFactory.secure(source, overrideDefaultParser())); + final Transformer transformer = delegate.newTransformer(SecureSAXParserFactory.secure(source, overrideDefaultParser())); return transformer == null ? null : new SecureTransformer(transformer, getURIResolver(), emptySource, overrideDefaultParser()); } @@ -296,7 +296,7 @@ public TransformerHandler newTransformerHandler() throws TransformerConfiguratio */ @Override public TransformerHandler newTransformerHandler(final Source source) throws TransformerConfigurationException { - return secure(delegate.newTransformerHandler(SecureTransformerFactory.secure(source, overrideDefaultParser()))); + return secure(delegate.newTransformerHandler(SecureSAXParserFactory.secure(source, overrideDefaultParser()))); } @Override @@ -430,26 +430,6 @@ public static TransformerFactory newInstance(final String factoryClassName, fina * @param factory the factory to secure; never {@code null}. * @return a secure factory. */ - /** - * TrAX flavor of {@link SecureSAXParserFactory#secure(Source, boolean)}: the same rewrite, with the SAX-side exceptions converted once into the - * {@link TransformerConfigurationException} the TrAX signatures demand. - * - * @param source the source to secure; never {@code null}. - * @param overrideDefaultParser whether {@value SecureSAXParserFactory#OVERRIDE_DEFAULT_PARSER} on the originating factory asks to override the JDK's - * default parser. - * @return a secure source. - * @throws TransformerConfigurationException if a secure reader cannot be obtained. - * @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. - */ - static Source secure(final Source source, final boolean overrideDefaultParser) throws TransformerConfigurationException { - try { - return SecureSAXParserFactory.secure(source, overrideDefaultParser); - } catch (ParserConfigurationException | SAXException e) { - throw new TransformerConfigurationException(e); - } - } - static TransformerFactory secure(final TransformerFactory factory) { // Required: enables secure processing (XSLTC runtime limits; Xalan's extension-function block). setFeature(factory, XMLConstants.FEATURE_SECURE_PROCESSING, true); diff --git a/src/main/java/org/apache/commons/xml/SecureValidator.java b/src/main/java/org/apache/commons/xml/SecureValidator.java index 033ddf9f..f49645ac 100644 --- a/src/main/java/org/apache/commons/xml/SecureValidator.java +++ b/src/main/java/org/apache/commons/xml/SecureValidator.java @@ -21,7 +21,6 @@ import java.util.Objects; import javax.xml.parsers.FactoryConfigurationError; -import javax.xml.parsers.ParserConfigurationException; import javax.xml.transform.Result; import javax.xml.transform.Source; import javax.xml.validation.Validator; @@ -120,10 +119,6 @@ public void setResourceResolver(final LSResourceResolver resourceResolver) { */ @Override public void validate(final Source source, final Result result) throws SAXException, IOException { - try { - delegate.validate(SecureSAXParserFactory.secure(source, overrideDefaultParser), result); - } catch (final ParserConfigurationException e) { - throw new SAXException("Failed to secure source for validation", e); - } + delegate.validate(SecureSAXParserFactory.secure(source, overrideDefaultParser), result); } } diff --git a/src/main/java/org/apache/commons/xml/SecureXMLFilter.java b/src/main/java/org/apache/commons/xml/SecureXMLFilter.java index 3504699d..0dc2e415 100644 --- a/src/main/java/org/apache/commons/xml/SecureXMLFilter.java +++ b/src/main/java/org/apache/commons/xml/SecureXMLFilter.java @@ -21,7 +21,8 @@ import java.util.Objects; import javax.xml.parsers.FactoryConfigurationError; -import javax.xml.parsers.ParserConfigurationException; +import javax.xml.transform.ErrorListener; +import javax.xml.transform.SourceLocator; import javax.xml.transform.Transformer; import javax.xml.transform.TransformerException; import javax.xml.transform.sax.SAXResult; @@ -30,7 +31,9 @@ import org.xml.sax.ContentHandler; import org.xml.sax.InputSource; import org.xml.sax.SAXException; +import org.xml.sax.SAXParseException; import org.xml.sax.XMLFilter; +import org.xml.sax.XMLReader; import org.xml.sax.ext.LexicalHandler; import org.xml.sax.helpers.XMLFilterImpl; @@ -41,9 +44,10 @@ * unsecured reader for the input (the stock JDK's does so as early as {@code setContentHandler}) and cast a supplied {@link javax.xml.transform.Templates} to * their own type, which a wrapped Templates is not. Here the input is parsed by the parent reader, a secure one installed on first {@code parse} when the * caller has not set a parent (a caller-set parent is trusted configuration, used as-is), and the transformation runs on a {@link SecureTransformer}, so - * runtime {@code document()} sits on the resolver floor.

+ * runtime {@code document()} sits on the resolver floor. The filter is also the transformer's {@link ErrorListener}, forwarding TrAX error reports to the + * caller-set {@link org.xml.sax.ErrorHandler} the way the parent reader's SAX reports are.

*/ -final class SecureXMLFilter extends XMLFilterImpl { +final class SecureXMLFilter extends XMLFilterImpl implements ErrorListener { private final SecureTemplates templates; @@ -57,6 +61,33 @@ final class SecureXMLFilter extends XMLFilterImpl { this.templates = Objects.requireNonNull(templates, "templates"); } + /** + * Forwards a recoverable transformation error to the caller-set {@link org.xml.sax.ErrorHandler}, mirroring the SAX contract: the transformation continues + * unless that handler throws. + */ + @Override + public void error(final TransformerException e) throws TransformerException { + try { + error(toSAXParseException(e)); + } catch (final SAXException se) { + throw new TransformerException(se); + } + } + + /** + * Forwards a fatal transformation error to the caller-set {@link org.xml.sax.ErrorHandler}, then fails the parse like a SAX parser does after + * {@code fatalError}: some implementations' lenient default listeners would otherwise only print and truncate the parse silently. + */ + @Override + public void fatalError(final TransformerException e) throws TransformerException { + try { + fatalError(toSAXParseException(e)); + } catch (final SAXException se) { + throw new TransformerException(se); + } + throw e; + } + /** * {@inheritDoc} * @@ -70,12 +101,14 @@ public void parse(final InputSource input) throws SAXException, IOException { throw new SAXException("No ContentHandler set on the XMLFilter to receive the transformation result"); } if (getParent() == null) { - try { - setParent(SecureSAXParserFactory.newXMLReader(templates.overrideDefaultParser)); - } catch (final ParserConfigurationException e) { - throw new SAXException(e); - } + setParent(SecureSAXParserFactory.newXMLReader(templates.overrideDefaultParser)); } + final XMLReader parent = getParent(); + // Like XMLFilterImpl.setupParse, minus the ContentHandler: the transformer owns the parent's content events and delivers the transformed stream to + // the caller's handler through the SAXResult instead. + parent.setEntityResolver(this); + parent.setDTDHandler(this); + parent.setErrorHandler(this); final SAXResult result = new SAXResult(handler); if (handler instanceof LexicalHandler) { result.setLexicalHandler((LexicalHandler) handler); @@ -83,7 +116,9 @@ public void parse(final InputSource input) throws SAXException, IOException { try { // A new SecureTransformer per parse: the floor is installed on it, and transformers are not reusable across concurrent parses. final Transformer transformer = templates.newTransformer(); - transformer.transform(new SAXSource(getParent(), input), result); + // The filter is the listener, so TrAX error reports reach the caller-set ErrorHandler like the parent reader's SAX reports do. + transformer.setErrorListener(this); + transformer.transform(new SAXSource(parent, input), result); } catch (final TransformerException e) { // The parent reader's parse errors and the handler's own exceptions arrive wrapped; rethrow the original rather than nesting the hierarchies. final Throwable cause = e.getCause(); @@ -96,4 +131,28 @@ public void parse(final InputSource input) throws SAXException, IOException { throw new SAXException(e); } } + + /** Bridges a TrAX report to the SAX callback shape: the original {@link SAXParseException} where one is the cause, otherwise a synthetic one carrying the locator. */ + private static SAXParseException toSAXParseException(final TransformerException e) { + final Throwable cause = e.getCause(); + if (cause instanceof SAXParseException) { + return (SAXParseException) cause; + } + // Embed the cause rather than the TrAX wrapper, so the originating exception stays directly reachable in the reported chain. + final Exception embedded = cause instanceof Exception ? (Exception) cause : e; + final SourceLocator locator = e.getLocator(); + return locator == null + ? new SAXParseException(e.getMessage(), null, null, -1, -1, embedded) + : new SAXParseException(e.getMessage(), locator.getPublicId(), locator.getSystemId(), locator.getLineNumber(), locator.getColumnNumber(), embedded); + } + + /** Forwards a transformation warning to the caller-set {@link org.xml.sax.ErrorHandler}; the transformation continues unless that handler throws. */ + @Override + public void warning(final TransformerException e) throws TransformerException { + try { + warning(toSAXParseException(e)); + } catch (final SAXException se) { + throw new TransformerException(se); + } + } } diff --git a/src/test/java/org/apache/commons/xml/XMLFilterTest.java b/src/test/java/org/apache/commons/xml/XMLFilterTest.java index 3470c22b..2e570956 100644 --- a/src/test/java/org/apache/commons/xml/XMLFilterTest.java +++ b/src/test/java/org/apache/commons/xml/XMLFilterTest.java @@ -19,13 +19,14 @@ import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; -import static org.junit.jupiter.api.Assertions.assertSame; +import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import java.io.StringReader; import javax.xml.XMLConstants; import javax.xml.transform.Templates; +import javax.xml.transform.TransformerException; import javax.xml.transform.TransformerFactory; import javax.xml.transform.sax.SAXTransformerFactory; @@ -99,15 +100,11 @@ void secureFilterDoesNotLeakExternalEntity() throws Exception { @Test void secureFilterDoesNotReWrapParseError() throws Exception { - // Dual contract: a malformed input's SAXParseException is swallowed (Xalan), hidden inside an implementation wrapper (XSLTC), or surfaces; when the - // cause chain carries it, parse must rethrow it directly rather than bury it under a fresh SAXException. final XMLFilter filter = SaxSurfaceTestSupport.secureFactory().newXMLFilter(AttackTestSupport.streamSource(IDENTITY_XSLT)); filter.setContentHandler(AttackTestSupport.capturingHandler(new StringBuilder())); - try { - filter.parse(new InputSource(new StringReader(""))); - } catch (final SAXException e) { - assertNotReWrapped(e); - } + filter.setErrorHandler(AttackTestSupport.STRICT_REPORTER); + final SAXException e = assertThrows(SAXException.class, () -> filter.parse(new InputSource(new StringReader("")))); + assertNotReWrapped(e); } @Test @@ -121,8 +118,6 @@ void secureFilterFromTemplatesDoesNotLeakDocument() throws Exception { @Test void secureFilterRethrowsHandlerSAXException() throws Exception { - // Dual contract: the delegate transformer either swallows the handler's exception (Xalan) or surfaces it wrapped in a TransformerException; when it - // surfaces, parse must rethrow the original instance, not nest it under a new SAXException. final XMLFilter filter = SaxSurfaceTestSupport.secureFactory().newXMLFilter(AttackTestSupport.streamSource(IDENTITY_XSLT)); final SAXException handlerFailure = new SAXException("handler failure"); filter.setContentHandler(new DefaultHandler() { @@ -131,11 +126,15 @@ public void startDocument() throws SAXException { throw handlerFailure; } }); - try { - filter.parse(new InputSource(new StringReader(""))); - } catch (final SAXException e) { - assertSame(handlerFailure, e, "the handler's own SAXException should surface unwrapped"); + filter.setErrorHandler(AttackTestSupport.STRICT_REPORTER); + final SAXException e = assertThrows(SAXException.class, () -> filter.parse(new InputSource(new StringReader("")))); + // Xalan wraps the handler's exception in its own SAXParseException, so assert on the chain: the original must be present and no TrAX wrapper above it. + boolean found = false; + for (Throwable cause = e; cause != null; cause = cause.getCause()) { + assertFalse(cause instanceof TransformerException, "handler failure should not come back wrapped in TrAX exceptions: " + e); + found |= cause == handlerFailure; } + assertTrue(found, "the handler's own SAXException should surface in the cause chain: " + e); } @Test From 1e5e545cf39ecc1b487fbdf58f970e177d57c8b8 Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Sat, 29 Aug 2026 20:22:11 +0200 Subject: [PATCH 3/4] Fix Checkstyle violations Assisted-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01RZSVucNBf5fsyd1uqamLuk --- .../java/org/apache/commons/xml/SecureTransformer.java | 1 - src/main/java/org/apache/commons/xml/SecureXMLFilter.java | 7 ++++++- 2 files changed, 6 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/apache/commons/xml/SecureTransformer.java b/src/main/java/org/apache/commons/xml/SecureTransformer.java index fc4898a5..f9cf9f21 100644 --- a/src/main/java/org/apache/commons/xml/SecureTransformer.java +++ b/src/main/java/org/apache/commons/xml/SecureTransformer.java @@ -26,7 +26,6 @@ import javax.xml.transform.Result; import javax.xml.transform.Source; import javax.xml.transform.Transformer; -import javax.xml.transform.TransformerConfigurationException; import javax.xml.transform.TransformerException; import javax.xml.transform.URIResolver; diff --git a/src/main/java/org/apache/commons/xml/SecureXMLFilter.java b/src/main/java/org/apache/commons/xml/SecureXMLFilter.java index 0dc2e415..87e5e334 100644 --- a/src/main/java/org/apache/commons/xml/SecureXMLFilter.java +++ b/src/main/java/org/apache/commons/xml/SecureXMLFilter.java @@ -132,7 +132,12 @@ public void parse(final InputSource input) throws SAXException, IOException { } } - /** Bridges a TrAX report to the SAX callback shape: the original {@link SAXParseException} where one is the cause, otherwise a synthetic one carrying the locator. */ + /** + * Bridges a TrAX report to the SAX callback shape. + * + * @param e the reported exception. + * @return The original {@link SAXParseException} where one is the cause, otherwise a synthetic one carrying the locator. + */ private static SAXParseException toSAXParseException(final TransformerException e) { final Throwable cause = e.getCause(); if (cause instanceof SAXParseException) { From 7caa9f0ea61cf5e93da31d885384569f567e69a0 Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Sat, 29 Aug 2026 20:34:01 +0200 Subject: [PATCH 4/4] Test the setupParse-style wiring in SecureXMLFilter.parse A recording parent reader asserts parse wires the filter as the parent's EntityResolver, DTDHandler and ErrorHandler (the wiring calls themselves: which of them the TrAX implementation later consults or overwrites varies, so delivery cannot be asserted uniformly). A second test proves the EntityResolver route end-to-end: a caller-set resolver opts an external entity in through the parent's floor. The cause-chain walks now follow SAXException.getException(), which Android's SAXException does not link into Throwable.getCause(). Assisted-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_01RZSVucNBf5fsyd1uqamLuk --- .../org/apache/commons/xml/XMLFilterTest.java | 93 ++++++++++++++++++- 1 file changed, 91 insertions(+), 2 deletions(-) diff --git a/src/test/java/org/apache/commons/xml/XMLFilterTest.java b/src/test/java/org/apache/commons/xml/XMLFilterTest.java index 2e570956..7230e57f 100644 --- a/src/test/java/org/apache/commons/xml/XMLFilterTest.java +++ b/src/test/java/org/apache/commons/xml/XMLFilterTest.java @@ -17,12 +17,15 @@ package org.apache.commons.xml; +import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertNotNull; import static org.junit.jupiter.api.Assertions.assertThrows; import static org.junit.jupiter.api.Assertions.assertTrue; import java.io.StringReader; +import java.util.ArrayList; +import java.util.List; import javax.xml.XMLConstants; import javax.xml.transform.Templates; @@ -33,10 +36,16 @@ import org.junit.jupiter.api.Assumptions; import org.junit.jupiter.api.Tag; import org.junit.jupiter.api.Test; +import org.xml.sax.ContentHandler; +import org.xml.sax.DTDHandler; +import org.xml.sax.EntityResolver; +import org.xml.sax.ErrorHandler; import org.xml.sax.InputSource; import org.xml.sax.SAXException; import org.xml.sax.XMLFilter; +import org.xml.sax.helpers.AttributesImpl; import org.xml.sax.helpers.DefaultHandler; +import org.xml.sax.helpers.XMLFilterImpl; /** * {@link XMLFilter} products of the secure factory: the input document is parsed by a secure reader (never a self-provisioned permissive one), and the @@ -54,11 +63,19 @@ class XMLFilterTest { /** Asserts no SAXException is buried beneath the thrown one, proving {@code parse} rethrows originals instead of re-wrapping them. */ private static void assertNotReWrapped(final SAXException thrown) { - for (Throwable cause = thrown.getCause(); cause != null; cause = cause.getCause()) { + for (Throwable cause = causeOf(thrown); cause != null; cause = causeOf(cause)) { assertFalse(cause instanceof SAXException, "original SAXException should be rethrown, not re-wrapped: " + thrown); } } + /** Follows {@link SAXException#getException()} where present: Android's SAXException does not link the embedded exception into {@code getCause()}. */ + private static Throwable causeOf(final Throwable throwable) { + if (throwable instanceof SAXException && ((SAXException) throwable).getException() != null) { + return ((SAXException) throwable).getException(); + } + return throwable.getCause(); + } + private static String entityPayload() { return "\n" + " filter.parse(new InputSource(new StringReader("")))); // Xalan wraps the handler's exception in its own SAXParseException, so assert on the chain: the original must be present and no TrAX wrapper above it. boolean found = false; - for (Throwable cause = e; cause != null; cause = cause.getCause()) { + for (Throwable cause = e; cause != null; cause = causeOf(cause)) { assertFalse(cause instanceof TransformerException, "handler failure should not come back wrapped in TrAX exceptions: " + e); found |= cause == handlerFailure; } assertTrue(found, "the handler's own SAXException should surface in the cause chain: " + e); } + @Test + void secureFilterRoutesEntityResolverToParent() throws Exception { + // parse must wire the caller-set EntityResolver to the parent reader, chaining it onto the floor so a caller can opt a specific entity in. + Assumptions.assumeFalse(AttackTestSupport.IS_ANDROID, "Android's Expat does not resolve the external general entity here"); + final XMLFilter filter = SaxSurfaceTestSupport.secureFactory().newXMLFilter(AttackTestSupport.streamSource(IDENTITY_XSLT)); + filter.setEntityResolver((publicId, systemId) -> new InputSource(new StringReader("resolved-by-caller"))); + final String output = filterAndCapture(filter, entityPayload()); + assertTrue(output.contains("resolved-by-caller"), "caller-set EntityResolver should opt the external entity in through the parent"); + assertFalse(output.contains(AttackTestSupport.LEAKED_MARKER), "the real external resource must not be fetched"); + } + + @Test + void secureFilterWiresCallbacksToParent() throws Exception { + // parse must perform the XMLFilterImpl.setupParse wiring on the parent for the resolver, DTD and error callbacks (the ContentHandler is owned by the + // transformer). The wiring calls are asserted directly on a recording parent: which of them the implementation later consults or overwrites varies. + final XMLFilter filter = SaxSurfaceTestSupport.secureFactory().newXMLFilter(AttackTestSupport.streamSource(IDENTITY_XSLT)); + final List wired = new ArrayList<>(); + final XMLFilterImpl parent = new XMLFilterImpl() { + + @Override + public boolean getFeature(final String name) { + // Accept the namespace probes implementations make on a SAXSource reader; there is no parent to delegate to. + return "http://xml.org/sax/features/namespaces".equals(name); + } + + @Override + public Object getProperty(final String name) { + return null; + } + + @Override + public void setFeature(final String name, final boolean value) { + } + + @Override + public void setProperty(final String name, final Object value) { + } + + @Override + public void parse(final InputSource input) throws SAXException { + // Minimal well-formed document for the transformation to consume; no real parser behind this parent. + final ContentHandler handler = getContentHandler(); + handler.startDocument(); + handler.startElement("", "root", "root", new AttributesImpl()); + handler.endElement("", "root", "root"); + handler.endDocument(); + } + + @Override + public void setDTDHandler(final DTDHandler handler) { + wired.add(handler); + super.setDTDHandler(handler); + } + + @Override + public void setEntityResolver(final EntityResolver resolver) { + wired.add(resolver); + super.setEntityResolver(resolver); + } + + @Override + public void setErrorHandler(final ErrorHandler handler) { + wired.add(handler); + super.setErrorHandler(handler); + } + }; + filter.setParent(parent); + assertEquals("", filterAndCapture(filter, "")); + assertEquals(3, wired.stream().filter(callback -> callback == filter).count(), + "parse should wire the filter as the parent's EntityResolver, DTDHandler and ErrorHandler: " + wired); + } + @Test void unconfiguredFilterLeaksDocument() throws Exception { final SAXTransformerFactory factory = (SAXTransformerFactory) TransformerFactory.newInstance();