[COMMONSXML-9] Make the resolver floor non-removable across JAXP implementations - #18
Merged
Merged
Conversation
… Android) The Xerces DOM and SAX recipes install a deny-all EntityResolver to stand in for the ACCESS_EXTERNAL_* properties Xerces ignores. Unlike those properties, a resolver is the caller's own slot: DocumentBuilder.setEntityResolver or XMLReader.setEntityResolver replaces it outright, and SAXParser.parse(source, handler) silently installs the handler as the resolver. Either way the deny-all block is dropped and external fetches go through unhardened. The Android Expat path had the same bypass, since ExpatReader does resolve through a caller resolver. Make the resolver a floor the caller cannot remove. A new Resolvers.FallbackDenyResolver wraps an optional caller-supplied resolver: a resource the caller resolves (returns a non-null InputSource) is allowed, but anything it does not resolve (null, or no caller resolver) is denied instead of fetched. It extends DefaultHandler2 so a subclass can double as a LexicalHandler and deny through a protected onUnresolved() hook. FallbackDenyResolver(null) is the deny-all case, replacing the former DenyAll.ENTITY2. The hardened wrappers re-wrap whatever the caller sets rather than letting it replace the floor: - HardeningDocumentBuilder (over a new DelegatingDocumentBuilder) routes setEntityResolver through the floor's delegate and re-establishes the bare floor on reset(). - HardeningXMLReader does the same and reports the caller's resolver unwrapped from getEntityResolver(); a two-arg constructor accepts a provider-specific floor subclass. - SAXParserHardener's external-Xerces path returns new HardeningXMLReader(reader), and its Android path installs a DtdAwareDenyResolver (now a FallbackDenyResolver subclass that permits the declared external subset) as both lexical handler and floor. HardeningSAXParser already routes every parse(...) overload through getXMLReader(), so parse(source, handler) goes through the floor too. - DocumentBuilderHardener wraps each produced builder in HardeningDocumentBuilder. The stock JDK path is unchanged: it blocks through ACCESS_EXTERNAL_*, so the reader is returned as-is and the wrappers are transparent there. hardenReader is idempotent on an already-hardened HardeningXMLReader. Add EntityResolverFloorTest: an allow-list resolver resolves the permitted systemId and the floor denies an unlisted one (DOM and SAX), and parse(source, handler) no longer bypasses the floor. Assisted-By: Claude Opus 4.8 <noreply@anthropic.com>
Move HardeningDocumentBuilderFactory out of DocumentBuilderHardener into its own package-private top-level class so other hardeners can wrap a DocumentBuilderFactory with the deny-all resolver floor. Behaviour is unchanged: it still wraps every DocumentBuilder produced in a HardeningDocumentBuilder, which keeps a single FallbackDenyResolver floor and routes a caller-set resolver through its delegate. Assisted-By: Claude Opus 4.8 <noreply@anthropic.com>
Verify that a hardened, schema-validating DOM or SAX parser does not fetch a schema named only through the Xerces external-noNamespaceSchemaLocation or external-schemaLocation property (as opposed to an instance-document xsi:schemaLocation hint, already covered by SchemaLocationDomTest and SchemaLocationSaxTest). The permissive controls confirm the external schema is reachable when unhardened, so the hardened side throwing means the fetch was refused. Covered under both the stock JDK (accessExternalSchema="") and external Apache Xerces, which ignores that property and instead relies on the deny-all entity-resolver floor; this confirms the floor is consulted for schema-location resolution, not just entity and DTD resolution. Because not every parser accepts these knobs (Android's KXmlParser and Expat do not), the setup runs through a configure-or-skip helper so an unsupported parser skips rather than fails. Widen AttackTestSupport.strictDocumentBuilder to package-visible for reuse and add the leaked/no-namespace.xsd fixture. Assisted-By: Claude Opus 4.8 <noreply@anthropic.com>
SchemaFactory, the Validators and ValidatorHandlers it produces all exposed setResourceResolver as the caller's own slot, so a caller could replace the deny-all LSResourceResolver and re-enable xs:import/xs:include/xs:redefine and xsi:schemaLocation fetches. Make it a floor instead: a new Resolvers.FallbackDenyLSResourceResolver consults an optional caller resolver first and denies (throws) whatever the caller does not resolve. HardeningSchemaFactory and HardeningValidator hold one floor instance, install it on the delegate, and route setResourceResolver through its delegate rather than replacing it; getResourceResolver reports the caller's resolver unwrapped. HardeningSchema now wraps each produced ValidatorHandler in a new HardeningValidatorHandler (over a new DelegatingValidatorHandler) that keeps the same floor. A caller still opts specific lookups in by returning a non-null LSInput, but can no longer drop the block. ACCESS_EXTERNAL_* stays unset (the JDK 8 SchemaFactory keeps blocking through it even when a caller resolver would grant access). Assisted-By: Claude Opus 4.8 <noreply@anthropic.com>
TransformerFactory.setURIResolver and Transformer.setURIResolver were the caller's own slot, so a caller could replace the deny-all URIResolver (or set it to null on a produced transformer) and re-enable xsl:import/xsl:include at compile time and document() at runtime. Make it a floor: a new Resolvers.FallbackDenyURIResolver consults an optional caller resolver first and denies (throws) whatever the caller does not resolve. HardeningTransformerFactory installs the floor on the delegate factory and routes setURIResolver through its delegate; getURIResolver reports the caller's resolver unwrapped. Each produced Transformer (identity, compiled, or via Templates) gets its own floor seeded from the factory's caller resolver, installed by HardeningTransformer, which routes setURIResolver through it too. A caller still opts a specific URI in by returning a non-null Source, but can no longer drop the block, so the previous "replacing the URIResolver cancels the block" caveat no longer applies. Also prune the now-superseded Resolvers.DenyAll.URI and DenyAll.LS_RESOURCE singletons (the URIResolver and LSResourceResolver channels are covered by their floors). Assisted-By: Claude Opus 4.8 <noreply@anthropic.com>
StaxHardener returned the raw vendor XMLInputFactory, so a caller could call setXMLResolver (or setProperty for the resolver keys) and replace the deny-all resolver, re-opening external DTD and entity fetches. On Woodstox setXMLResolver even replaces both the entity and DTD-subset hooks at once. Make it a floor. Resolvers.FallbackDenyXMLResolver consults an optional caller resolver first and denies (throws) whatever it does not resolve; it is extensible through a protected onUnresolved hook, with a FallbackIgnoreXMLResolver variant that returns an empty input instead of throwing for the Woodstox DTD-subset and undeclared-entity hooks (where a missing resource must be skipped, not denied). This supersedes the former Resolvers.DenyAll and IgnoreAll singletons, which are removed. A new HardeningXMLInputFactory (over a new DelegatingXMLInputFactory) wraps the factory and routes every resolver-valued entry point uniformly (setXMLResolver, setProperty(XMLInputFactory.RESOLVER) and the three com.ctc.wstx.*Resolver keys): a caller who supplies their own FallbackDenyXMLResolver takes control; otherwise the caller's resolver is set as the delegate of the floor currently on that hook, or wrapped in a fresh floor if the hook is empty. This is necessary because Woodstox does not chain resolvers: on a null return, DefaultInputResolver fetches the systemId URL itself, so a caller-set resolver that returns null must still land behind the floor. getXMLResolver / getProperty report the caller's resolver unwrapped; the SUPPORT_DTD / IS_SUPPORTING_EXTERNAL_ENTITIES defaults are unchanged. Extend EntityResolverFloorTest into the single home for the floor contract across every resolver channel: the SAX/DOM EntityResolver, the StAX XMLResolver, the schema LSResourceResolver and the XSLT URIResolver. For each, a caller resolver resolves an allow-listed reference, the floor denies an unlisted one, and a resolver that resolves nothing cannot remove the block. Saxon reaches the same contract through its ALLOWED_PROTOCOLS restrictor rather than a floor (a null return still throws), so the Transformer cases run there too; interpretive Xalan needs the shared AttackTestSupport.StrictReporter as the ErrorListener to surface a blocked xsl:import as a throw. Assisted-By: Claude Opus 4.8 <noreply@anthropic.com>
The fallback-deny entity floor is invoked as an EntityResolver2, so it receives the literal (possibly relative) systemId plus a separate base URI. When it delegated to a caller-supplied plain EntityResolver it forwarded that relative systemId unchanged, even though the SAX2 EntityResolver contract promises an already-absolutized systemId. A caller therefore saw only the bare relative reference and could neither match nor open the resource, so an XInclude of a sibling file failed even with an allow-all resolver. Resolve the systemId against the base URI before consulting a plain EntityResolver, mirroring what a normal parser hands it. EntityResolver2 delegates still receive the literal systemId and base unchanged. Add DOM and SAX regression tests that XInclude a sibling by a relative href through an allow-all resolver; they are skipped on platforms whose JAXP does not support setXIncludeAware (e.g. Android). Assisted-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
State the two defining properties of every floor up front: it is non-removable and wraps the caller's resolver, and it supplies the default action for a lookup the caller leaves unresolved. Call out the departure from stock JAXP, where an unresolved lookup falls back to built-in resolution and fetches the resource, whereas a floor denies it. Assisted-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Installing a resolver no longer removes the hardening: the floor wraps the caller's resolver instead of being replaced. Reflect that across the threat model. Add a "Resolvers" entry under the settings a caller may modify, listing the typed set*Resolver methods, the DefaultHandler passed to SAXParser.parse, and the StAX resolver properties (javax.xml.stream.resolver and the com.ctc.wstx.*Resolver keys), with the caveat that the installed resolver must resolve every resource the caller needs, since the floor denies or ignores whatever it leaves unresolved. Move the Woodstox resolver properties out of the reserved list, correct the reserved-section note that previously called installing a resolver a loosening, and reframe the out-of-scope entry to the remaining caller responsibility: a resolver that resolves an untrusted resource fetches it. The raw Xerces internal entity-resolver property stays reserved, as the typed setter override does not wrap it. Assisted-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Member
Author
|
This PR includes the changes in copernik-eu/copernik-xml-factory#29, copernik-eu/copernik-xml-factory#30 and copernik-eu/copernik-xml-factory#31. |
On JDK 8 the two XInclude regression tests threw a NullPointerException inside the built-in Xerces XIncludeHandler.searchForRecursiveIncludes: the allow-all resolver returned an InputSource carrying only a byte stream, so the included document had no base URI, and JDK 8's recursion detection dereferences it. Newer JDKs tolerate the missing base. Set the system id on the returned InputSource, as a correct resolver does. Passes on Temurin 8 and 25. Assisted-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Contributor
There was a problem hiding this comment.
Pull request overview
This PR addresses COMMONSXML-9 by making the “deny/ignore resolver floor” non-removable across JAXP implementations, so that a caller installing a custom resolver can opt specific resources in but cannot re-open XXE/SSRF fallback fetching by returning null.
Changes:
- Introduces “fallback deny/ignore” resolver floor implementations and wraps resolver setters/getters so caller resolvers become delegates instead of replacements.
- Adds hardened wrapper types for key JAXP components (DOM/SAX readers/builders, SchemaFactory/Validator/ValidatorHandler, TransformerFactory/Templates/Transformer, StAX XMLInputFactory) to enforce the non-removable floor consistently.
- Adds focused regression tests and fixtures covering resolver delegation vs. fallback behavior across EntityResolver / LSResourceResolver / URIResolver / XMLResolver channels, plus schema-location and XInclude scenarios.
Reviewed changes
Copilot reviewed 25 out of 25 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/test/resources/leaked/with-xinclude.xml | Adds XInclude host fixture for relative-include resolution tests. |
| src/test/resources/leaked/no-namespace.xsd | Adds schema fixture for schema-location property validation tests. |
| src/test/java/org/apache/commons/xml/SchemaLocationPropertyTest.java | Verifies hardened parsers don’t fetch schemas referenced only via Xerces schema-location properties. |
| src/test/java/org/apache/commons/xml/EntityResolverFloorTest.java | Core regression coverage ensuring caller resolvers cannot remove the deny/ignore floor across channels. |
| src/test/java/org/apache/commons/xml/AttackTestSupport.java | Exposes shared strict reporter and adds a helper to skip platform-unsupported configuration. |
| src/site/markdown/threat_model.md | Updates threat model to allow caller-installed resolvers while documenting the non-removable floor semantics. |
| src/main/java/org/apache/commons/xml/TransformerHardener.java | Updates documentation/behavior to rely on a URIResolver floor that cannot be replaced. |
| src/main/java/org/apache/commons/xml/StaxHardener.java | Installs StAX resolver floors and wraps factories to prevent caller resolver replacement. |
| src/main/java/org/apache/commons/xml/SAXParserHardener.java | Wraps XMLReaders so caller entity resolvers cannot replace the deny floor (including handler-based override paths). |
| src/main/java/org/apache/commons/xml/Resolvers.java | Adds delegate-first, fallback deny/ignore resolver floor implementations plus URI absolutization helper. |
| src/main/java/org/apache/commons/xml/HardeningXMLReader.java | New XMLReader wrapper that keeps an EntityResolver floor non-overridable. |
| src/main/java/org/apache/commons/xml/HardeningXMLInputFactory.java | New XMLInputFactory wrapper that keeps XMLResolver floors non-removable across resolver entry points. |
| src/main/java/org/apache/commons/xml/HardeningValidatorHandler.java | New ValidatorHandler wrapper that keeps an LSResourceResolver deny floor non-removable. |
| src/main/java/org/apache/commons/xml/HardeningValidator.java | Updates Validator wrapper to keep an LSResourceResolver deny floor as a non-removable lower bound. |
| src/main/java/org/apache/commons/xml/HardeningTransformerFactory.java | Keeps a URIResolver deny floor at factory level and propagates resolver floors into produced Transformers/Templates. |
| src/main/java/org/apache/commons/xml/HardeningTransformer.java | Keeps a non-removable URIResolver floor for runtime document() and related fetches. |
| src/main/java/org/apache/commons/xml/HardeningTemplates.java | Ensures transformers produced from Templates get the resolver-floor behavior. |
| src/main/java/org/apache/commons/xml/HardeningSchemaFactory.java | Keeps an LSResourceResolver deny floor for schema compilation, routing caller resolvers through it. |
| src/main/java/org/apache/commons/xml/HardeningSchema.java | Ensures produced ValidatorHandlers keep the resolver floor via a wrapper. |
| src/main/java/org/apache/commons/xml/HardeningDocumentBuilderFactory.java | New DocumentBuilderFactory wrapper that produces builders with an entity-resolver floor. |
| src/main/java/org/apache/commons/xml/HardeningDocumentBuilder.java | New DocumentBuilder wrapper that keeps an EntityResolver deny floor non-overridable. |
| src/main/java/org/apache/commons/xml/DocumentBuilderHardener.java | Uses the new builder-factory wrapper when ACCESS_EXTERNAL_* isn’t supported. |
| src/main/java/org/apache/commons/xml/DelegatingXMLInputFactory.java | New delegating base class for XMLInputFactory wrappers. |
| src/main/java/org/apache/commons/xml/DelegatingValidatorHandler.java | New delegating base class for ValidatorHandler wrappers. |
| src/main/java/org/apache/commons/xml/DelegatingDocumentBuilder.java | New delegating base class for DocumentBuilder wrappers. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
Convert British spellings (honour, flavour, defence, behaviour, synchronise, standardised) to US spellings across Javadoc, comments, and the site documentation. Assisted-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
# Conflicts: # src/main/java/org/apache/commons/xml/DocumentBuilderHardener.java # src/main/java/org/apache/commons/xml/SAXParserHardener.java
Only cast a Woodstox resolver property value to XMLResolver when it actually is one (or null) on set, and when the underlying factory returns one on get, so a non-XMLResolver value passes through to the delegate instead of throwing ClassCastException. Assisted-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Member
Author
|
Should I merged the |
Each Hardening* wrapper had a paired Delegating* superclass holding the delegate field and the trivial forwarding methods. Fold that boilerplate into the single Hardening* subclass that used it and drop the separate Delegating* file, keeping the trivial forwarders at the end of the class in an "editor-fold" region so an IDE can collapse them. DelegatingXMLReader is kept: it has two subclasses (HardeningXMLReader and SAXParserHardener.ExpatReaderWrapper), so it is not merged. No behavior change: super.foo(...) calls that reached the Delegating forwarders now call delegate.foo(...) directly. Assisted-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
.github/dependabot.yml, .github/workflows/codeql-analysis.yml, .github/workflows/maven.yml and src/test/resources/leaked/with-xinclude.xml did not end with a newline; add one so every text file ends with EOL. Assisted-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
DelegatingXMLReader had only two subclasses left. Merge its forwarding into HardeningXMLReader (which now implements XMLReader directly, with the forwarders in a foldable region) and let the Android Expat wrapper extend HardeningXMLReader instead: ExpatReaderWrapper becomes HardeningExpatXMLReader, and the Android hardened path collapses from a double wrap to a single one. DelegatingXMLReader is deleted. The permissive Android control in AttackTestSupport reused the old wrapper as a non-hardening pass-through; give it a light test-only XMLFilterImpl-based wrapper (namespace-prefixes eager reject, no deny floor) so the positive controls stay permissive. Verified with the full JVM suite and the Android connectedAndroidTest run (129 tests) on the Pixel_6a emulator. Assisted-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
Member
Author
|
@elharo, could you take a look at this? Various implementations have different mechanisms to prevent external fetches, but is seems that only resolvers give a 100% guarantee. For example JAXP 1.5 introduces all these nice
Therefore the plan is:
|
Member
|
Hi @ppkarwasz |
elharo
reviewed
Jul 8, 2026
elharo
left a comment
elharo
left a comment
There was a problem hiding this comment.
can you configure your editor to avoid making whitespace changes in unrelated files and remove them from this PR?
I can't edit PR titles in this repo but it should be
[COMMONSXML-9] Make the resolver floor non-removable across JAXP implementations
so it autolinks to JIRA
I'm not super-concerned about this issue. If the caller calls set*Resolver that's their decision. They've chosen to install what they've chosen to install. Increasingly this all feels like leaky band-aids on top of band-aids. A more fundamental approach might be called for that operates much lower in the stack.
ppkarwasz
changed the title
Restore British spelling (synchronise, standardised) and drop the Oxford commas added on lines whose content is otherwise unchanged, to minimise the diff against apache/main.
Restore British spelling (Honoured, flavours, defence) and drop cosmetic javadoc/comment rewrites in DocumentBuilderHardener, keeping only the structural class-promotion changes and the EntityResolver floor wording, to minimise the diff against apache/main.
Member
Author
If you are referring to the EOL and British spelling, I couldn't resist the urge to fix them in the whole library, but I reverted those changes.
What approach do you propose? Since the hardening for Xerces used resolvers, this looked like a cheap feature to introduce. In COMMONSXML-2 I plan to remove the code that sets expansion limits uniformly, since this is useful for testing, but hardly needed in practice. For COMMONSXML-4 I was thinking about ignoring external fetches universally (not only for the external subset). With the two changes above the hardening of // Android exposes no FSP, ACCESS_EXTERNAL_* or attribute API, and KXmlParser drops user-defined entities; nothing to apply.
if (ANDROID_DOCUMENT_BUILDER_FACTORY.equals(factory.getClass().getName())) {
return factory;
}
// Required: enables the implementation's security manager, which carries the limits.
setFeature(factory, XMLConstants.FEATURE_SECURE_PROCESSING, true);
// Required: install an ignore-alll resolver on every DocumentBuilder.
return new HardeningDocumentBuilderFactory(factory, Resolvers.IgnoreAll.ENTITY2); |
Member
Author
This PR is a little bit about this: sometimes you want to access some external resources or resources by some criteria. In that case you can install a resolver and you don't need to remember what returning |
Member
|
Thanks for the reply. It's not clear to me if this PR is ready for merge. Should it have been a draft PR until now? I've normalized Commons (including this component) on US English spelling (color, not colour, and so on.) |
Member
Author
Yes, it should be ready to merge.
I saw that, which is why I also implemented some of the changes you missed in this PR. I should have made a separate PR instead. |
Member
Author
|
The whitespace spelling changes are in #19 now. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Fixes COMMONSXML-9.
Problem
A hardened factory from
XmlFactoriesblocks external DTDs, entities, schemas and stylesheets. On Apache Xerces, Woodstox, Xalan and the JDK's built-in StAX (Zephyr), installing a custom resolver silently removed that protection. Unlike the stock JDK (JEP 185ACCESS_EXTERNAL_*), Saxon (allowedProtocols) and Android (ignore-all), these implementations fall back to fetching whatever a resolver leaves unresolved, so a caller'sset*Resolveroverwrote the deny floor the hardening relied on and re-opened the XXE/SSRF surface.Change
Install a non-removable resolver floor on every resolver channel:
EntityResolver(DOM/SAX),LSResourceResolver(schema),URIResolver(XSLT) andXMLResolver(StAX). The hardened wrappers keep their own floor installed and route a caller's resolver through it as a delegate:nullvalue;nullreturn, or no caller resolver) takes the floor's default action, deny (or ignore for the Woodstox DTD-subset and undeclared-entity hooks), instead of the implementation's resolve-all fallback.Installing a resolver is now in scope: a custom resolver can add permitted resources but can no longer remove the block.
Threat model
The threat model is amended to explicitly allow users to install their own resolvers,
except implementation-specific types like Xerces
http://apache.org/xml/properties/internal/entity-resolver.Tests
EntityResolverFloorTestis the core coverage: on every channel (EntityResolver,LSResourceResolver,URIResolver,XMLResolver), an allow-list resolver still opts a specific resource in, while a resolver that resolves nothing cannot re-open a blocked fetch.setXIncludeAware(Android).