Skip to content

[COMMONSXML-10] Block XInclude (xi:include) href resolution by default#21

Open
ppkarwasz wants to merge 8 commits into
apache:mainfrom
ppkarwasz:fix/xinclude
Open

[COMMONSXML-10] Block XInclude (xi:include) href resolution by default#21
ppkarwasz wants to merge 8 commits into
apache:mainfrom
ppkarwasz:fix/xinclude

Conversation

@ppkarwasz

Copy link
Copy Markdown
Member

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:

  • On Apache Xerces, XInclude resolution goes through the parser's EntityResolver, so the deny-all resolver installed by the hardening already blocks all inclusions.
  • On the stock JDK implementation, XInclude resolution is governed by neither ACCESS_EXTERNAL_DTD nor ACCESS_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:

DocumentBuilderFactory factory = XmlFactories.newDocumentBuilderFactory();
factory.setNamespaceAware(true);
factory.setXIncludeAware(true);
DocumentBuilder builder = factory.newDocumentBuilder();
// Delegates to the deny-all floor for anything it does not resolve
builder.setEntityResolver((publicId, systemId) -> {
    if (isTrusted(systemId)) {
        return new InputSource(systemId);
    }
    return null; // falls through to the floor, which blocks the fetch
});

Note that returning null does 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.

thientd and others added 6 commits July 8, 2026 23:07
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]>
Comment thread src/test/java/org/apache/commons/xml/XIncludeTest.java Outdated

Copilot AI left a comment

Copy link
Copy Markdown

Choose a reason for hiding this comment

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

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 XIncludeTest coverage for DOM/SAX behavior (baseline leak vs hardened block, plus allow-list resolver behavior).
  • Update DOM/SAX hardeners to always enforce a deny-all EntityResolver floor (rather than conditionally relying on ACCESS_EXTERNAL_* support).
  • Simplify transformer hardening by removing the ACCESS_EXTERNAL_DTD attribute 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.

Comment thread src/test/java/org/apache/commons/xml/XIncludeTest.java Outdated
Comment thread src/test/java/org/apache/commons/xml/XIncludeTest.java Outdated
Comment thread src/test/java/org/apache/commons/xml/XIncludeTest.java Outdated
@garydgregory

Copy link
Copy Markdown
Member

@ppkarwasz
Any thoughts on the copilot review findings?

Co-authored-by: Copilot Autofix powered by AI <[email protected]>
@ppkarwasz

Copy link
Copy Markdown
Member Author

Any thoughts on the copilot review findings?

They make sense: XmlFactories.harden(XMLReader) usually returns the argument, but could also wrap it. I applied the fixes in ecda422.

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.

4 participants