You are viewing a plain text version of this content. The canonical link for it is here.
Posted to issues@commons.apache.org by "ppkarwasz (via GitHub)" <gi...@apache.org> on 2026/08/29 13:02:25 UTC
[PR] Rework exception propagation when securing sources [commons-xml]
ppkarwasz opened a new pull request, #60:
URL: https://github.com/apache/commons-xml/pull/60
Exceptions raised while provisioning a secure reader for a `Source` used to make round trips between the SAX and TrAX exception hierarchies:
- `SecureSAXParserFactory.secure(Source, boolean)` wrapped `ParserConfigurationException`/`SAXException` into a `TransformerConfigurationException`, which `SecureValidator.validate` and `SecureSchemaFactory` then wrapped back into a `SAXException` — a three-deep chain for what began as a `SAXException`.
- `SecureXMLFilter.parse` buried the parent reader's `SAXParseException` under `SAXException(TransformerException(...))`, which a downstream transformer consuming the filter wraps into yet another `TransformerException`.
- `newSecureXMLReader` declared (and documented) a `TransformerConfigurationException` it never threw.
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`; the validation and schema paths let a `SAXException` propagate untouched and wrap only `ParserConfigurationException`.
- `SecureTransformerFactory.secure(Source, boolean)` is the TrAX flavor, converting once to the `TransformerConfigurationException` the TrAX signatures demand.
- `FallbackIgnoreURIResolver` converts locally to `TransformerException` (the accurate type for a resolution-time failure), which also keeps the TrAX wrappers out of the XPath shading closure (`ShadingFootprintTest`).
- `SecureXMLFilter.parse` applies the standard TrAX-filter unwrap idiom: a `SAXException` or `IOException` cause of the transform's `TransformerException` is rethrown directly, so the original `SAXParseException` (with locator info) or a handler's own exception surfaces as-is.
Two new `XMLFilterTest` cases pin the unwrap down in the project's dual-contract style, tolerating implementations that swallow the error (Xalan) or hide it in an internal wrapper (XSLTC) while asserting the original exception is never re-wrapped where it does surface.
Full surefire matrix passes.
🤖 Generated with [Claude Code](https://claude.com/claude-code)
https://claude.ai/code/session_01RZSVucNBf5fsyd1uqamLuk
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Rework exception propagation when securing sources [commons-xml]
Posted by "ppkarwasz (via GitHub)" <gi...@apache.org>.
ppkarwasz commented on code in PR #60:
URL: https://github.com/apache/commons-xml/pull/60#discussion_r3887326733
##########
src/test/java/org/apache/commons/xml/XMLFilterTest.java:
##########
@@ -87,6 +97,19 @@ void secureFilterDoesNotLeakExternalEntity() throws Exception {
assertFalse(filterAndCapture(filter, entityPayload()).contains(AttackTestSupport.LEAKED_MARKER), "external entity through XMLFilter leaked");
}
+ @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("<root>")));
Review Comment:
Fixed in https://github.com/apache/commons-xml/pull/60/commits/4a8ea614182ddbb663bdf8c8d8c338042c8e7203
##########
src/test/java/org/apache/commons/xml/XMLFilterTest.java:
##########
@@ -96,6 +119,25 @@ void secureFilterFromTemplatesDoesNotLeakDocument() throws Exception {
assertFalse(filterAndCapture(filter, "<root/>").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 {
Review Comment:
Fixed in https://github.com/apache/commons-xml/pull/60/commits/4a8ea614182ddbb663bdf8c8d8c338042c8e7203
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Rework exception propagation when securing sources [commons-xml]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory commented on code in PR #60:
URL: https://github.com/apache/commons-xml/pull/60#discussion_r3887207486
##########
src/test/java/org/apache/commons/xml/XMLFilterTest.java:
##########
@@ -87,6 +97,19 @@ void secureFilterDoesNotLeakExternalEntity() throws Exception {
assertFalse(filterAndCapture(filter, entityPayload()).contains(AttackTestSupport.LEAKED_MARKER), "external entity through XMLFilter leaked");
}
+ @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("<root>")));
Review Comment:
This test passes if SAXException is NOT thrown, which would be a bug, which is means this test should fail but doesn't because it doesn't use `assertThrows()` when calling `parse()`.
##########
src/test/java/org/apache/commons/xml/XMLFilterTest.java:
##########
@@ -96,6 +119,25 @@ void secureFilterFromTemplatesDoesNotLeakDocument() throws Exception {
assertFalse(filterAndCapture(filter, "<root/>").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 {
Review Comment:
This test passes if SAXException is NOT thrown, which would be a bug, which is means this test should fail but doesn't because it doesn't use `assertThrows()` when calling `parse()`.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Rework exception propagation when securing sources [commons-xml]
Posted by "garydgregory (via GitHub)" <gi...@apache.org>.
garydgregory merged PR #60:
URL: https://github.com/apache/commons-xml/pull/60
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: issues-unsubscribe@commons.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org