Rework exception propagation when securing sources

src/main/java/org/apache/commons/xml/SecureException.java

@@ -20,10 +20,11 @@
 /**
  * Thrown when a factory cannot be made secure.
  *
- * <p>Two failure modes share this type:</p>
+ * <p>Three failure modes share this type:</p>
  * <ul>
  *   <li>No bundled secure recipe matches the concrete factory class.</li>
  *   <li>A recipe tried to apply a secure setting and the implementation rejected it.</li>
+ *   <li>The implementation could not provide the internal secure reader the Source-rewriting wrappers parse with.</li>
  * </ul>
  *
  * <p>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.
+     *
+     * <p>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.</p>
+     *
+     * @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.
      *

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,15 +300,16 @@ 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 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 newSecureXMLReader(final boolean overrideDefaultParser) throws TransformerConfigurationException {
+    static XMLReader newXMLReader(final boolean overrideDefaultParser) {
         try {
             return newNSInstance(overrideDefaultParser).newSAXParser().getXMLReader();
-        } catch (final ParserConfigurationException | SAXException e) {
-            throw new TransformerConfigurationException("Failed to obtain a secure XMLReader for source parsing", e);
+        } catch (ParserConfigurationException | SAXException e) {
+            throw SecureException.readerFailed(e);
         }
     }
 
@@ -355,14 +355,14 @@ static SAXParserFactory secure(final SAXParserFactory factory) {
      * @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 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 TransformerConfigurationException {
+    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(newSecureXMLReader(overrideDefaultParser), inputSource);
+            return inputSource == null ? source : new SAXSource(newXMLReader(overrideDefaultParser), inputSource);
         }
         return source;
     }

src/main/java/org/apache/commons/xml/SecureSchemaFactory.java

@@ -24,7 +24,6 @@
 import javax.xml.XMLConstants;
 import javax.xml.parsers.FactoryConfigurationError;
 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;
@@ -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 TransformerConfigurationException 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;
         }

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;
 
@@ -139,9 +138,9 @@ 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 {

src/main/java/org/apache/commons/xml/SecureValidator.java

@@ -23,7 +23,6 @@
 import javax.xml.parsers.FactoryConfigurationError;
 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;
@@ -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 TransformerConfigurationException e) {
-            throw new SAXException("Failed to secure source for validation", e);
-        }
+        delegate.validate(SecureSAXParserFactory.secure(source, overrideDefaultParser), result);
     }
 }

src/main/java/org/apache/commons/xml/SecureXMLFilter.java

@@ -21,6 +21,8 @@
 import java.util.Objects;
 
 import javax.xml.parsers.FactoryConfigurationError;
+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;
@@ -29,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;
 
@@ -40,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.</p>
+ * 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.</p>
  */
-final class SecureXMLFilter extends XMLFilterImpl {
+final class SecureXMLFilter extends XMLFilterImpl implements ErrorListener {
 
     private final SecureTemplates templates;
 
@@ -56,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}
      *
@@ -69,22 +101,63 @@ 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.newSecureXMLReader(templates.overrideDefaultParser));
-            } catch (final TransformerException 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);
         }
         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();
+            if (cause instanceof SAXException) {
+                throw (SAXException) cause;
+            }
+            if (cause instanceof IOException) {
+                throw (IOException) cause;
+            }
             throw new SAXException(e);
         }
     }
+
+    /**
+     * 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) {
+            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);
+        }
+    }
 }

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()) {