[COMMONSXML-10] Block XInclude (xi:include) href resolution by default#21
[COMMONSXML-10] Block XInclude (xi:include) href resolution by default#21ppkarwasz wants to merge 8 commits into
Conversation
The ACCESS_EXTERNAL_DTD/ACCESS_EXTERNAL_SCHEMA attributes do not govern XInclude href resolution in the stock JDK's XInclude processor, so a factory that relied on them alone left xi:include fetches open. Install the deny-all EntityResolver floor on every DocumentBuilder and XMLReader regardless of ACCESS_EXTERNAL_* support (the resolver-floor wrappers were previously skipped on the stock-JDK path), so XInclude-enabled parses fail closed and a caller can allow-list specific resources through a custom EntityResolver. Also updates the newDocumentBuilderFactory()/newSAXParserFactory() Javadoc accordingly and adds XIncludeTest covering the advisory GHSA-xm28-xvqc-gxxg. Ported from copernik-xml-factory 2fa042c. Co-authored-by: Piotr P. Karwasz <[email protected]>
Drop the redundant ACCESS_EXTERNAL_* attributes from the DOM and SAX hardening paths: the deny-all EntityResolver floor already installed on every DocumentBuilder and XMLReader blocks external DTD, entity, schema and xi:include fetches in one place, so the JAXP 1.5 attributes add nothing there. ACCESS_EXTERNAL_DTD stays on the transformer compile path, where XSLTC copies it onto the reader it provisions for the stylesheet and the floor does not reach. Inline the now single-use JaxpSetters.trySetAttribute into setOptionalAttribute(DocumentBuilderFactory), matching the other setOptional* helpers, and update the hardener Javadoc accordingly. Ported from copernik-xml-factory e5febc6. Assisted-By: Claude Opus 4.8 (1M context) <[email protected]>
Replace the per-test temp-file fixtures with the shared leaked/ resources and tighten the XInclude-awareness gate so the tests skip cleanly where the platform refuses XInclude rather than failing. Ported from copernik-xml-factory 2bbcb89.
Have the allow-list tests assert the resolver's exact in-memory content is what lands in the parse result, proving the caller's resolver was consulted rather than merely that the parse succeeded. Ported from copernik-xml-factory 5d96ed6.
XSLTC copies the factory's ACCESS_EXTERNAL_DTD onto the reader it uses to parse the stylesheet, but it does not touch that reader's deny-all EntityResolver floor. HardeningTransformerFactory already routes every stylesheet Source through an XmlFactories-hardened reader, so the floor blocks the external-DTD/entity channel during stylesheet compilation on its own, making the factory attribute redundant. This was the last ACCESS_EXTERNAL_* setter in the hardening code: the deny-all resolver floor now covers every DOM, SAX and TrAX external-fetch channel. Verified against the full multi-JAXP suite, including test-stockjdk running the trax tests against XSLTC. Assisted-By: Claude Opus 4.8 (1M context) <[email protected]>
apache#21 (comment) Co-authored-by: Ta Duc Thien <[email protected]>
There was a problem hiding this comment.
Pull request overview
This PR ports the GHSA-xm28-xvqc-gxxg fix by ensuring XInclude xi:include href resolution is blocked by default when using hardened factories from XmlFactories, aligning behavior across the JDK and Apache Xerces implementations. It shifts the hardening strategy to consistently rely on a non-removable deny-all resolver “floor” so callers can only opt-in trusted resources via explicit allow-listing resolvers.
Changes:
- Add
XIncludeTestcoverage for DOM/SAX behavior (baseline leak vs hardened block, plus allow-list resolver behavior). - Update DOM/SAX hardeners to always enforce a deny-all
EntityResolverfloor (rather than conditionally relying onACCESS_EXTERNAL_*support). - Simplify transformer hardening by removing the
ACCESS_EXTERNAL_DTDattribute dependency and relying on hardened source parsing plus the URIResolver floor.
Reviewed changes
Copilot reviewed 6 out of 6 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/java/org/apache/commons/xml/XIncludeTest.java | New tests covering baseline XInclude leakage and hardened deny-by-default behavior plus allow-listing. |
| src/main/java/org/apache/commons/xml/XmlFactories.java | Documentation update clarifying XInclude is denied by default and opt-in requires an allow-list resolver. |
| src/main/java/org/apache/commons/xml/TransformerHardener.java | Removes ACCESS_EXTERNAL_DTD handling; relies on hardened parsing and locked-down URI resolution. |
| src/main/java/org/apache/commons/xml/SAXParserHardener.java | Always wraps readers with a non-removable deny-all resolver floor to block external fetches (including XInclude). |
| src/main/java/org/apache/commons/xml/JaxpSetters.java | Simplifies optional attribute setting for DocumentBuilderFactory (no boolean return path). |
| src/main/java/org/apache/commons/xml/DocumentBuilderHardener.java | Always returns a factory wrapper that enforces a deny-all resolver floor on produced builders. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
|
@ppkarwasz |
Co-authored-by: Copilot Autofix powered by AI <[email protected]>
They make sense: |
This PR ports the fix for GHSA-xm28-xvqc-gxxg to Commons XML.
Problem
The behavior of
setXIncludeAware(true)on a hardened factory currently depends on the JAXP implementation:EntityResolver, so the deny-all resolver installed by the hardening already blocks all inclusions.ACCESS_EXTERNAL_DTDnorACCESS_EXTERNAL_SCHEMA, the attributes used by the hardening. Enabling XInclude awareness therefore allows arbitrary inclusions, bypassing the intended hardening.Fix
After this change both implementations behave the same: XInclude fails closed by default. To use XInclude, users must opt trusted resources in through an explicit
EntityResolver:Note that returning
nulldoes not restore the default behavior of fetching the URL: the caller's resolver runs as a delegate of the deny-all floor, so unresolved hrefs stay blocked.