Skip to content

Rework exception propagation when securing sources - #60

Merged
garydgregory merged 4 commits into
apache:mainfrom
ppkarwasz:feat/source-exception-propagation
Aug 29, 2026
Merged

Rework exception propagation when securing sources#60
garydgregory merged 4 commits into
apache:mainfrom
ppkarwasz:feat/source-exception-propagation

Conversation

@ppkarwasz

Copy link
Copy Markdown
Member

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.ai/code/session_01RZSVucNBf5fsyd1uqamLuk

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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RZSVucNBf5fsyd1uqamLuk
@ppkarwasz
ppkarwasz force-pushed the feat/source-exception-propagation branch from 5558395 to 94ea207 Compare August 29, 2026 13:14

@garydgregory garydgregory left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

New tests passe if SAXException is NOT thrown, which would be a bug, which is means tests should fail but don't because they don't use assertThrows() when calling parse().

Comment thread src/test/java/org/apache/commons/xml/XMLFilterTest.java Outdated
Comment thread src/test/java/org/apache/commons/xml/XMLFilterTest.java Outdated
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RZSVucNBf5fsyd1uqamLuk
Assisted-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RZSVucNBf5fsyd1uqamLuk
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01RZSVucNBf5fsyd1uqamLuk
@ppkarwasz
ppkarwasz requested a review from garydgregory August 29, 2026 18:38
@garydgregory
garydgregory merged commit df65356 into apache:main Aug 29, 2026
18 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants