fix: Restore hardened state after reset() - #35
Closed
ppkarwasz wants to merge 20 commits into
Closed
Conversation
An external reference the caller's resolver does not resolve is now resolved to empty content rather than rejected with an exception. Nothing is fetched either way, so the security property is unchanged; the difference is that a parse now continues with empty content instead of failing. Rename the four floors accordingly and collapse the variants the deny/ignore split required: FallbackDenyEntityResolver2 -> FallbackIgnoreEntityResolver2 FallbackDenyXMLResolver -> FallbackIgnoreXMLResolver FallbackDenyLSResourceResolver -> FallbackIgnoreLSResourceResolver FallbackDenyURIResolver -> FallbackIgnoreURIResolver `SAXParserHardener.DtdAwareDenyResolver` and `StaxHardener.DtdSubsetFloor` existed only to exempt the external DTD subset from the deny, and the old `FallbackIgnoreXMLResolver` only to exempt Woodstox's undeclared entities; with ignore as the default all three are redundant. Removing them, together with the now-dead `HardeningException.forbidden`, takes the shade closure from 33 classes to 30. StAX collapses further: the floor moves into the `HardeningXMLInputFactory` constructor (as `HardeningXMLReader` already did), a single `setXMLResolver` covers both the JDK Zephyr and Woodstox (whose `setXMLResolver` fans out to its DTD-subset and entity resolvers), and the Zephyr `ignore-external-dtd` property is dropped because the floor already empties the subset. Only Woodstox's undeclared-entity hook stays separate: emptying the external subset leaves the entities it declared undeclared, and that hook is outside the fan-out. Tests that asserted an exception now assert the external resource does not leak. Where the outcome differs by implementation (Saxon still rejects through ALLOWED_PROTOCOLS, an emptied schema import fails to compile) they accept either outcome through the new assert*BlocksOrDoesNotLeak helpers. Assisted-By: Claude Opus 4.8 (1M context) <noreply@anthropic.com>
With the ignore-all floors as universal behavior, skipping the external DTD subset up front is redundant: when the parser requests it, the floor resolves it to empty content and the parse continues. Remove the feature from the DOM and SAX recipes together with the setOptionalFeature helpers it was the only user of. Actually requesting the subset exposed two gaps in the floors, fixed here: both now echo the requested identifiers on the empty source they return (Xerces derives the entity's base URI from the system id and fails on null), and the LSResourceResolver floor hands its empty content over as a character stream, because the JDK's DOMEntityResolverWrapper discards empty string data. The ignore outcome stays best-effort: Saxon's ALLOWED_PROTOCOLS restrictor sits ahead of the floor and rejects the subset lookup outright, so the affected TrAX tests accept either outcome through the assert*BlocksOrDoesNotLeak helpers and DoctypeOnlyTest documents the dual outcome. Assisted-By: Claude Fable 5 <noreply@anthropic.com>
Saxon was the odd one out: its ALLOWED_PROTOCOLS="" lockdown rejected every unresolved external reference (for example an external DTD subset) with an exception, where the other implementations resolve it to empty content through a floor. Drop the ALLOWED_PROTOCOLS setting and install a nature-aware ignore-all ResourceResolver on the HardenedConfiguration instead: - XML, XSLT and XSD natures resolve to Saxon's EmptySource, so an unresolved xsl:include/xsl:import compiles as an empty module and doc()/document() return the empty sequence. - Text and binary natures resolve to an empty StreamSource, so unparsed-text() yields the empty string. - External-entity and DTD natures return null so the lookup falls through to the hardened reader's entity-resolver floor, keeping caller allow-listing on the reader working. The floor backs every resolution chain ahead of Saxon's direct-fetch fallback. A setResourceResolver override re-wraps any resolver installed later (including through the plain-JAXP TransformerFactory.setURIResolver route, which replaces the Configuration resolver wholesale) with the floor as its fallback, and an empty CollectionFinder covers fn:collection, the one channel that bypasses the resource resolver. Saxon now passes the DOCTYPE-only and external-DTD suites like the other stacks; tests where implementations still diverge on unresolved xsl:import/include (XSLTC and Xalan reject the emptied module, Saxon compiles it) accept either outcome and assert no leak. Pin the new nested class in ShadingFootprintTest. Assisted-By: Claude Fable 5 <noreply@anthropic.com>
Shorten the floor and constructor comments and install the floor through the setResourceResolver override instead of calling super directly. Assisted-By: Claude Fable 5 <noreply@anthropic.com>
Make AttackTestSupport.captureCharacters package-private, add an InputSource overload and a capturingHandler factory, and replace the eleven anonymous DefaultHandler copies in EntityResolverFloorTest and XIncludeTest with them. Assisted-By: Claude Fable 5 <noreply@anthropic.com>
Fold CDATA capture into AttackTestSupport.captureStaxEventText, make it package-private and drop the duplicate readStaxText. Assisted-By: Claude Fable 5 <noreply@anthropic.com>
An empty character stream is not a well-formed XML document, so XSLTC rejects it for document() and both XSLTC and Xalan reject an ignored xsl:include or xsl:import at compile time. A well-formed empty document lets every implementation proceed and evaluate to no content, so the include, import and document() tests now assert the strict no-leak outcome on all implementations. Assisted-By: Claude Fable 5 <noreply@anthropic.com>
With Saxon's protocol restrictor gone, the external-DTD and external-general-entity TrAX tests assert the strict no-leak outcome. The parameter-entity tests stay strict through a caller-supplied error listener that accepts only the JDK parser's "referenced, but not declared" report, which XML 1.0 section 4.1 demotes to an unreported validity constraint when the internal subset contains a parameter entity reference. Drop the now-unused blocks-or-does-not-leak Templates and Transformer helpers. Assisted-By: Claude Fable 5 <noreply@anthropic.com>
Keep the pipeline strict and hand a thrown TransformerException to a caller-supplied ThrowingConsumer that rethrows it (the default) or returns to accept the block. This replaces the ErrorListener overloads and their swallowed-report tracking: with the strict listener installed the factories throw at the first report, so the half-built Templates path never runs. Assisted-By: Claude Fable 5 <noreply@anthropic.com>
ExternalGeneralEntityTest declares its entity in the internal subset, so nothing is ever undeclared and every surface asserts the strict no-leak outcome. ExternalParameterEntityTest is the one payload with a genuinely undeclared entity after hardening, so all its surfaces assert the dual blocks-or-does-not-leak contract, replacing the trax message-matching acceptance. Methods are renamed to state the assertion they make. Assisted-By: Claude Fable 5 <noreply@anthropic.com>
Emptying the external subset leaves any entity it declared undeclared, and Woodstox rejects such a reference unconditionally. The rejection keeps the resource just as unfetched as the empty resolution the other implementations produce, so the per-implementation hook is unnecessary; a caller can still opt references in by setting the property, which lands their resolver behind a floor like every other resolver hook. ExternalDtdTest's StAX case accepts the block accordingly. Assisted-By: Claude Fable 5 <noreply@anthropic.com>
Setting -Dorg.apache.commons.xml.throwOnUnresolved=true switches every resolver floor from resolving an unresolved external reference to empty content to rejecting it with the resolution hook's exception. The property is read at resolution time, so it also applies to existing factories, and references resolved by a caller-supplied resolver are unaffected. Requested by Gary in COMMONSXML-4. Assisted-By: Claude Fable 5 <noreply@anthropic.com>
Restore HardeningException.forbidden with its five-part identifier tuple, now also naming the enabling property, in place of the single-argument unresolvedDenied helper the toggle had introduced. Each floor passes the same identifiers the old deny floors did. Assisted-By: Claude Fable 5 <noreply@anthropic.com>
Add a Configuration section: external references resolve to empty streams, applications that want rejection tighten the factory themselves (a stricter feature or a throwing resolver, with no security weight either way), and org.apache.commons.xml.throwOnUnresolved is a temporary debugging switch. Align the Usage paragraph with the same contract. Assisted-By: Claude Fable 5 <noreply@anthropic.com>
The JAXP reset contract reverts a parser to its just-created state, which discards the reader-level configuration installed after creation, while the wrapper kept serving its stale cached views. HardeningSAXParser.reset() now drops the cached reader and parser views, so the next getXMLReader() or getParser() call runs the freshly reset reader through the hardening funnel again, matching HardeningDocumentBuilder.reset(). Assisted-By: Claude Fable 5 <noreply@anthropic.com>
The JAXP reset contract reverts a validator to its just-created state, which removes the resource-resolver floor installed by the constructor. HardeningValidator.reset() now re-establishes the bare floor, matching HardeningDocumentBuilder.reset(). Assisted-By: Claude Fable 5 <noreply@anthropic.com>
The JAXP reset contract reverts a transformer to its just-created state, and several implementations clear the URI resolver installed by the constructor. HardeningTransformer.reset() now re-establishes the floor, seeded again with the factory's compile-time resolver, matching the other hardened wrappers. Assisted-By: Claude Fable 5 <noreply@anthropic.com>
HardeningDocumentBuilder.reset() already re-establishes its floor, but no test exercised it; a regression there would go unnoticed. Assisted-By: Claude Fable 5 <noreply@anthropic.com>
Base automatically changed from
feature/ignore-all-resolver-floors
to
feature/reduce-shade-footprint
August 18, 2026 10:08
Member
|
Hi @ppkarwasz There is one conflict to resolve here. |
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
2 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.
What
The JAXP
reset()contract reverts a parser, validator, or transformer to its just-created state. The hardened wrappers install part of their protection after creation (resolver floors, cached hardened views), and several JAXP implementations take the contract literally and revert that state. Each wrapper now restores its just-constructed hardening onreset(), matching whatHardeningDocumentBuilder.reset()already did:HardeningSAXParser.reset()drops its cached reader and parser views, so the nextgetXMLReader()orgetParser()call runs the freshly reset reader through the hardening funnel again.HardeningValidator.reset()re-establishes the bare resource-resolver floor.HardeningTransformer.reset()re-establishes the URI-resolver floor, seeded again with the factory's compile-time resolver.Tests
The new
ResetHardeningTestcovers the three lifecycle paths: each test resets a hardened object and asserts that an external reference is still either blocked or resolved to empty content afterwards, and skips on platforms whose implementation does not supportreset()at all. All three were verified to fail before the fix on at least one supported stack.🤖 Generated with Claude Code