COMMONSXML-1: DocumentBuilderFactory: capability-driven hardening - #5
Merged
ppkarwasz merged 8 commits intoJun 27, 2026
Merged
Conversation
This change replaces the per-implementation class-name dispatch for DOM with a single capability-driven recipe. Secure processing is required and the external-DTD subset is skipped where supported; whether the JAXP 1.5 accessExternal properties are honoured then decides whether the bare factory is already safe or needs a deny-all resolver wrapper, which is the only point where the stock JDK and the external Xerces distribution diverge. Android stays untouched because its parser exposes no hardening surface. An implementation is no longer rejected for being unrecognised. Any parser that accepts secure processing and either the access properties or the resolver wrapper satisfies the contract on its own, so only a parser that refuses secure processing now fails. A new test covers the previously unverified guarantee that an xsi:schemaLocation hint is not fetched during DOM-side XSD validation, gated on parsers that honour accessExternalSchema. Assisted-By: Claude Opus 4.8 <noreply@anthropic.com>
Expose DocumentBuilderHardener with a public newInstance() so consumers that only need a hardened DocumentBuilderFactory can shade the library and copy a minimal set of classes. jdependency (the engine maven-shade-plugin's minimizeJar uses) works at class granularity, so the shaded set is the transitive closure of this one class. To keep that closure small, install a deny-all EntityResolver as a lambda local to DocumentBuilderHardener instead of reusing Resolvers.DenyAll.ENTITY2. EntityResolver's resolveEntity(publicId, systemId) hook is consulted for the external DTD subset and every external entity, so it blocks all external fetches on the DOM path just as well, while dropping the whole Resolvers nested-class tree from the closure (12 -> 7 class files). Route XmlFactories.newDocumentBuilderFactory() through the new entry point, and add ShadingFootprintTest, which uses jdependency to pin the reachable set to exactly those 7 classes so the footprint cannot silently grow back toward the full library. Assisted-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
The androidTest source set compiles all of ../src/test/java against android.jar, which broke on two tests new to this branch: - ShadingFootprintTest needs org.vafer.jdependency (a Maven-only test dependency) and pins the shading footprint, which is meaningless on Android. Exclude it from the Android test compilation. - SchemaLocationDomTest referenced XMLConstants.ACCESS_EXTERNAL_SCHEMA, which android.jar does not expose. Inline the constant's value (verified equal to the JDK constant); the test still skips at runtime on Android, since the parser does not honour that property. Verified on the api33 (Pixel 6a) managed device: 77 tests, 0 failures, 0 errors, 17 assumption-gated skips. Assisted-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
…rnalSchema The test exercises JAXP 1.2 XSD validation, which requires the SCHEMA_LANGUAGE property; that, not accessExternalSchema, is the capability a parser must have to run it (Android lacks it). Gate both methods on supportsSchemaLanguage(). The hardened assertion also assumed the block was attributed to accessExternalSchema, which only holds on the stock JDK. External Xerces does not honour that property and instead blocks the schema fetch through the deny-all resolver, with a different message. Assert only that the failure references the external schema, not the mechanism. Two effects: - The test now runs and passes on external Xerces too (covering the resolver-based schema-block path), where it previously skipped. - It no longer references XMLConstants.ACCESS_EXTERNAL_SCHEMA, so the android.jar compile gap is resolved by this redesign rather than by inlining the constant. Verified: mvn -o test green across all executions; SchemaLocationDomTest runs 2/2 with 0 skips on both stock JDK and external Xerces. Assisted-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
elharo
suggested changes
Jun 21, 2026
elharo
left a comment
elharo
left a comment
There was a problem hiding this comment.
This needs an issue and discussion before PR for each change. This looks like more than one thing. Exactly one change per PR please.
Split per review: this PR (COMMONSXML-1) keeps only the capability-driven DocumentBuilderFactory hardening. The minimal shading entry point (public DocumentBuilderHardener.newInstance(), the inlined deny-all resolver, ShadingFootprintTest and the jdependency test dependency) will be filed as its own issue. Reverses 68ad1d1 and 90ef0ae: DocumentBuilderHardener is package-private again and harden() reuses Resolvers.DenyAll.ENTITY2; XmlFactories.newDocumentBuilderFactory() calls harden() directly. Assisted-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
ppkarwasz
changed the title
Member
Author
|
Hi @elharo, I rolled back the changes regarding shading and created a COMMONSXML-1 issue, where we can discuss the changes. |
elharo
approved these changes
Jun 21, 2026
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
3 participants
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.
Summary
Capability-driven hardening for DOM (
DocumentBuilderFactory). Tracked as COMMONSXML-1.1. Capability-driven hardening
Implements @elharo's suggestion in the
xml-commons-dev@xercesthread:Replaces the per-implementation class-name dispatch for DOM with a single capability-driven recipe:
accessExternal*properties are honoured then decides whether the bare factory is already safe or needs a deny-all resolver wrapper. This is the only point where the stock JDK and the external Xerces distribution diverge.xsi:schemaLocationhint is not fetched during DOM-side XSD validation (gated on parsers that honouraccessExternalSchema).2. Minimal shading entry pointAdds a publicDocumentBuilderHardener.newInstance()so consumers that only need a hardenedDocumentBuilderFactorycan shade the library and copy a minimal set of classes.jdependency(the enginemaven-shade-plugin'sminimizeJaruses) works at class granularity, so the shaded set is the transitive closure of this one class.A deny-allEntityResolveris installed as a local lambda instead of reusingResolvers, which drops the wholeResolversnested-class tree from the closure (12 -> 7 class files).XmlFactories.newDocumentBuilderFactory()is routed through the new entry point.ShadingFootprintTestusesjdependencyto pin the reachable set to those 7 classes, so the footprint cannot silently grow back toward the full library.Javadoc on both methods documents how to enable XInclude: it is held off by the deny-all external-fetch behavior, not the awareness flag, so enabling it also requires a customEntityResolver.Testing
mvn -o test(all 7 JAXP-combination executions) passes with 0 failures and 0 errors. The 2 skips areSchemaLocationDomTestunder external Xerces, which does not honouraccessExternalSchema.Follow-ups (separate PRs)
🤖 Generated with Claude Code