This is an automated email from the ASF dual-hosted git repository. ppkarwasz pushed a commit to branch remove/harden-source in repository https://gitbox.apache.org/repos/asf/commons-xml.git
commit 751cb71690863617926f26306cddcc736060e58a Author: Piotr P. Karwasz <[email protected]> AuthorDate: Tue Aug 18 08:55:58 2026 +0200 Remove the harden(XMLReader) and harden(Source) methods from the public API Keep the public surface of XmlFactories minimal for the first release: callers who need a hardened XMLReader can obtain one from a hardened SAX factory, and rewriting a Source is an edge case that can live in another library. Both methods were one-line delegations to the package-private SAXParserHardener workhorses, which every internal caller already uses directly. Javadoc references now point at SAXParserHardener#hardenSource and SAXParserHardener#hardenReader, and the tests that exercised the hardened-in-place reader scenario (still relied on by SaxonProvider) call the package-private hardenReader directly. Assisted-By: Claude Fable 5 <[email protected]> --- .../org/apache/commons/xml/HardeningSchema.java | 2 +- .../apache/commons/xml/HardeningSchemaFactory.java | 4 +-- .../apache/commons/xml/HardeningTransformer.java | 2 +- .../commons/xml/HardeningTransformerFactory.java | 4 +-- .../org/apache/commons/xml/HardeningValidator.java | 2 +- .../org/apache/commons/xml/SAXParserHardener.java | 2 +- .../java/org/apache/commons/xml/SaxonProvider.java | 2 +- .../java/org/apache/commons/xml/XmlFactories.java | 29 ---------------------- .../org/apache/commons/xml/AttackTestSupport.java | 10 ++++---- .../java/org/apache/commons/xml/XIncludeTest.java | 14 +++++------ 10 files changed, 21 insertions(+), 50 deletions(-) diff --git a/src/main/java/org/apache/commons/xml/HardeningSchema.java b/src/main/java/org/apache/commons/xml/HardeningSchema.java index 87244f9..f3294c1 100644 --- a/src/main/java/org/apache/commons/xml/HardeningSchema.java +++ b/src/main/java/org/apache/commons/xml/HardeningSchema.java @@ -23,7 +23,7 @@ /** * {@link Schema} wrapper that hardens every {@link Validator} and {@link ValidatorHandler} the inner Schema produces: each {@link Validator} is wrapped in - * {@link HardeningValidator} (which rewrites the Source through {@link XmlFactories#harden(javax.xml.transform.Source)} and installs the resolver floor), and + * {@link HardeningValidator} (which rewrites the Source through {@link SAXParserHardener#hardenSource(javax.xml.transform.Source)} and installs the resolver floor), and * each {@link ValidatorHandler} is wrapped in a {@link HardeningValidatorHandler} that keeps the same ignore-all resolver floor so {@code xsi:schemaLocation} is * not resolved during SAX-driven validation. */ diff --git a/src/main/java/org/apache/commons/xml/HardeningSchemaFactory.java b/src/main/java/org/apache/commons/xml/HardeningSchemaFactory.java index ac000c2..a21ee58 100644 --- a/src/main/java/org/apache/commons/xml/HardeningSchemaFactory.java +++ b/src/main/java/org/apache/commons/xml/HardeningSchemaFactory.java @@ -38,13 +38,13 @@ * <ol> * <li>{@link HardeningSchemaFactory} installs an ignore-all {@link FallbackIgnoreLSResourceResolver} floor on the factory (blocking * {@code xs:import}/{@code xs:include}/{@code xs:redefine} at compile time) and rewrites the Source on every {@code newSchema(Source[])} entry point - * through {@link XmlFactories#harden(Source)}.</li> + * through {@link SAXParserHardener#hardenSource(Source)}.</li> * <li>{@link HardeningSchema} wraps every Validator/ValidatorHandler the inner Schema produces and re-installs the floor on each (blocking * {@code xsi:schemaLocation} at validation time), since neither the JDK nor Xerces reliably propagates it through {@code Schema}.</li> * <li>{@link HardeningValidator} rewrites the Source on every {@link Validator#validate(Source)} call.</li> * </ol> * - * <p>The hardened reader supplied by {@link XmlFactories#harden(Source)} already carries {@code FEATURE_SECURE_PROCESSING} and the processing limits, so a + * <p>The hardened reader supplied by {@link SAXParserHardener#hardenSource(Source)} already carries {@code FEATURE_SECURE_PROCESSING} and the processing limits, so a * DOCTYPE, external entity or Billion Laughs payload in the schema or instance document is bounded there rather than on this factory. The JAXP 1.5 * {@code ACCESS_EXTERNAL_*} properties are deliberately not set: the resolver floor already blocks the same fetches on every implementation, and the JDK 8 * {@code SchemaFactory} has a bug whereby those properties keep blocking even when a caller's own resolver would grant the access. The floor is a non-removable diff --git a/src/main/java/org/apache/commons/xml/HardeningTransformer.java b/src/main/java/org/apache/commons/xml/HardeningTransformer.java index a1b9322..ec16905 100644 --- a/src/main/java/org/apache/commons/xml/HardeningTransformer.java +++ b/src/main/java/org/apache/commons/xml/HardeningTransformer.java @@ -29,7 +29,7 @@ /** * {@link Transformer} wrapper that rewrites the Source on every {@link Transformer#transform(Source, Result)} call through - * {@link XmlFactories#harden(Source)} before delegating, and keeps an ignore-all {@link URIResolver} floor so runtime {@code document()} calls a caller does not + * {@link SAXParserHardener#hardenSource(Source)} before delegating, and keeps an ignore-all {@link URIResolver} floor so runtime {@code document()} calls a caller does not * resolve return empty rather than being fetched. * * <p>The floor is installed on the delegate transformer at construction, seeded with the factory's compile-time resolver; {@link #setURIResolver(URIResolver)} diff --git a/src/main/java/org/apache/commons/xml/HardeningTransformerFactory.java b/src/main/java/org/apache/commons/xml/HardeningTransformerFactory.java index ed8ac0f..8c7ebfa 100644 --- a/src/main/java/org/apache/commons/xml/HardeningTransformerFactory.java +++ b/src/main/java/org/apache/commons/xml/HardeningTransformerFactory.java @@ -32,7 +32,7 @@ import org.xml.sax.XMLReader; /** - * {@link javax.xml.transform.TransformerFactory} wrapper that rewrites every Source-taking entry point through {@link XmlFactories#harden(Source)} before + * {@link javax.xml.transform.TransformerFactory} wrapper that rewrites every Source-taking entry point through {@link SAXParserHardener#hardenSource(Source)} before * delegating. * * <p>Used by providers whose underlying TrAX implementation pulls a fresh {@code SAXParserFactory.newInstance()} for any Source that is not already a @@ -50,7 +50,7 @@ * <h2>Caveats</h2> * <ul> * <li>A {@link SAXSource} that carries its own {@link XMLReader} is trusted as-is: the caller is expected to supply a hardened reader (via - * {@link XmlFactories#newSAXParserFactory()} or {@link XmlFactories#harden(XMLReader)}) in that case.</li> + * {@link XmlFactories#newSAXParserFactory()}) in that case.</li> * <li>{@link TransformerHandler} returned from {@code newTransformerHandler} is not wrapped: it processes incoming SAX events instead of reading a Source, so * it has no inner Source-parsing path. A caller who pulls the inner {@link Transformer} via {@link TransformerHandler#getTransformer()} bypasses the * runtime source rewrite.</li> diff --git a/src/main/java/org/apache/commons/xml/HardeningValidator.java b/src/main/java/org/apache/commons/xml/HardeningValidator.java index cb30279..88222db 100644 --- a/src/main/java/org/apache/commons/xml/HardeningValidator.java +++ b/src/main/java/org/apache/commons/xml/HardeningValidator.java @@ -32,7 +32,7 @@ /** * {@link Validator} wrapper that rewrites the Source on every {@link Validator#validate(Source)} and {@link Validator#validate(Source, Result)} call through - * {@link XmlFactories#harden(Source)} before delegating, and keeps an ignore-all {@link LSResourceResolver} floor so {@code xsi:schemaLocation} is not resolved at + * {@link SAXParserHardener#hardenSource(Source)} before delegating, and keeps an ignore-all {@link LSResourceResolver} floor so {@code xsi:schemaLocation} is not resolved at * validation time. */ final class HardeningValidator extends Validator { diff --git a/src/main/java/org/apache/commons/xml/SAXParserHardener.java b/src/main/java/org/apache/commons/xml/SAXParserHardener.java index 4cd784f..5a309c2 100644 --- a/src/main/java/org/apache/commons/xml/SAXParserHardener.java +++ b/src/main/java/org/apache/commons/xml/SAXParserHardener.java @@ -103,7 +103,7 @@ static SAXParserFactory harden(final SAXParserFactory factory) { */ static XMLReader hardenReader(final XMLReader reader) { if (reader instanceof HardeningXMLReader) { - // Already hardened (for example, handed back through XmlFactories.harden(XMLReader)); the floor is already in place. + // Already hardened (for example, a reader from a hardened factory passed back through hardenReader); the floor is already in place. return reader; } if (ANDROID_EXPAT_READER.equals(reader.getClass().getName())) { diff --git a/src/main/java/org/apache/commons/xml/SaxonProvider.java b/src/main/java/org/apache/commons/xml/SaxonProvider.java index eaec0ca..9abc224 100644 --- a/src/main/java/org/apache/commons/xml/SaxonProvider.java +++ b/src/main/java/org/apache/commons/xml/SaxonProvider.java @@ -52,7 +52,7 @@ final class SaxonProvider { * * <ol> * <li><b>SAX layer.</b> {@link #makeParser} hands every {@link XMLReader} Saxon would otherwise use through - * {@link XmlFactories#harden(XMLReader)}, which routes it to the matching bundled hardening recipe. External DTDs, entities and XInclude + * {@link SAXParserHardener#hardenReader(XMLReader)}, which routes it to the matching bundled hardening recipe. External DTDs, entities and XInclude * resolve to empty content at parse time.</li> * <li><b>Resource-resolution layer.</b> A non-removable ignore-all {@link ResourceResolver} floor backs every resolution chain ({@code xsl:include}, * {@code xsl:import}, {@code xsl:source-document}, and the XPath/XSLT functions {@code fn:doc}, {@code fn:document}, {@code fn:unparsed-text}, diff --git a/src/main/java/org/apache/commons/xml/XmlFactories.java b/src/main/java/org/apache/commons/xml/XmlFactories.java index 270c830..a01b914 100644 --- a/src/main/java/org/apache/commons/xml/XmlFactories.java +++ b/src/main/java/org/apache/commons/xml/XmlFactories.java @@ -21,14 +21,10 @@ import javax.xml.parsers.DocumentBuilderFactory; import javax.xml.parsers.SAXParserFactory; import javax.xml.stream.XMLInputFactory; -import javax.xml.transform.Source; -import javax.xml.transform.TransformerConfigurationException; import javax.xml.transform.TransformerFactory; import javax.xml.validation.SchemaFactory; import javax.xml.xpath.XPathFactory; -import org.xml.sax.XMLReader; - /** * Entry point for obtaining hardened JAXP factories. * @@ -80,31 +76,6 @@ public final class XmlFactories { */ static final String THROW_ON_UNRESOLVED = "org.apache.commons.xml.throwOnUnresolved"; - /** - * Rewrites a {@link Source} so that any SAX parsing it triggers runs through an {@link XmlFactories}-hardened {@link XMLReader}. - * - * <p>Only a {@code StreamSource} or a {@code SAXSource} without a reader is enriched with a hardened, namespace-aware reader; other kinds of sources are - * returned as-is.</p> - * - * @param source The source to harden; never {@code null}. - * @return A hardened source. - * @throws TransformerConfigurationException if a hardened reader cannot be obtained. - */ - public static Source harden(final Source source) throws TransformerConfigurationException { - return SAXParserHardener.hardenSource(source); - } - - /** - * Hardens an existing {@link XMLReader}. - * - * @param reader The reader to harden; never {@code null}. - * @return A hardened reader. - * @throws IllegalStateException if a required hardening setting cannot be applied to the underlying implementation. - */ - public static XMLReader harden(final XMLReader reader) { - return SAXParserHardener.hardenReader(reader); - } - /** * Returns a fresh, hardened {@link DocumentBuilderFactory}. * diff --git a/src/test/java/org/apache/commons/xml/AttackTestSupport.java b/src/test/java/org/apache/commons/xml/AttackTestSupport.java index 8ca31ee..df2c334 100644 --- a/src/test/java/org/apache/commons/xml/AttackTestSupport.java +++ b/src/test/java/org/apache/commons/xml/AttackTestSupport.java @@ -691,7 +691,7 @@ static void assertValidatorValidates(final String xml) { /** * Asserts a hardened-in-place XMLReader parse of the payload throws. * - * <p>{@link XMLReader#parse(InputSource)} on a raw reader hardened via {@link XmlFactories#harden(XMLReader)}; only a thrown exception passes.</p> + * <p>{@link XMLReader#parse(InputSource)} on a raw reader hardened via {@link SAXParserHardener#hardenReader(XMLReader)}; only a thrown exception passes.</p> */ static void assertXmlReaderBlocks(final String payload) { assertParseFails(() -> consumeXmlReader(rawHardenedReader(), payload), "XMLReader", SAXException.class); @@ -707,7 +707,7 @@ static void assertXmlReaderBlocksOrDoesNotLeak(final String payload) { /** * Asserts a hardened-in-place XMLReader parse completes without throwing and without leaked content. * - * <p>{@link XMLReader#parse(InputSource)} on a raw reader hardened via {@link XmlFactories#harden(XMLReader)}; use this when the hardening contract + * <p>{@link XMLReader#parse(InputSource)} on a raw reader hardened via {@link SAXParserHardener#hardenReader(XMLReader)}; use this when the hardening contract * guarantees the parse succeeds but never resolves the external resource.</p> */ static void assertXmlReaderDoesNotLeak(final String payload) { @@ -717,7 +717,7 @@ static void assertXmlReaderDoesNotLeak(final String payload) { /** * Asserts a hardened-in-place XMLReader parse succeeds. * - * <p>{@link XMLReader#parse(InputSource)} on a raw reader hardened via {@link XmlFactories#harden(XMLReader)}; positive control for DOCTYPE-only + * <p>{@link XMLReader#parse(InputSource)} on a raw reader hardened via {@link SAXParserHardener#hardenReader(XMLReader)}; positive control for DOCTYPE-only * payloads.</p> */ static void assertXmlReaderParses(final String payload) { @@ -915,13 +915,13 @@ private static boolean probeDomResolvesInternalEntities() { } } - /** Builds a raw {@link XMLReader} from a deliberately permissive {@link SAXParserFactory} and hardens it via {@link XmlFactories#harden(XMLReader)}. */ + /** Builds a raw {@link XMLReader} from a deliberately permissive {@link SAXParserFactory} and hardens it via {@link SAXParserHardener#hardenReader(XMLReader)}. */ private static XMLReader rawHardenedReader() throws Exception { final SAXParserFactory factory = SAXParserFactory.newInstance(); if (!IS_ANDROID) { factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, false); } - return XmlFactories.harden(factory.newSAXParser().getXMLReader()); + return SAXParserHardener.hardenReader(factory.newSAXParser().getXMLReader()); } /** Opens the named test resource as a {@link StreamSource} preserving its system id, so relative includes/imports/redefines resolve normally. */ diff --git a/src/test/java/org/apache/commons/xml/XIncludeTest.java b/src/test/java/org/apache/commons/xml/XIncludeTest.java index a75a715..84a3933 100644 --- a/src/test/java/org/apache/commons/xml/XIncludeTest.java +++ b/src/test/java/org/apache/commons/xml/XIncludeTest.java @@ -331,7 +331,7 @@ void hardenedSaxNullResolverDoesNotLeak() throws Exception { //endregion - //region harden(XMLReader): reader with XInclude enabled before hardening is blocked + //region hardenReader: reader with XInclude enabled before hardening is blocked (the internal recipe behind the hardened factory and SaxonProvider) @Test @Tag("sax") @@ -342,9 +342,9 @@ void hardenReaderBlocksParseXml() throws Exception { final SAXParserFactory unhardenedFactory = SAXParserFactory.newInstance(); unhardenedFactory.setNamespaceAware(true); assumeXIncludeAware(unhardenedFactory); - final XMLReader reader = XmlFactories.harden(unhardenedFactory.newSAXParser().getXMLReader()); + final XMLReader reader = SAXParserHardener.hardenReader(unhardenedFactory.newSAXParser().getXMLReader()); assertThrows(SAXException.class, () -> reader.parse(input), - "harden(reader) should block XInclude parse=xml on reader with XInclude already enabled"); + "hardenReader should block XInclude parse=xml on reader with XInclude already enabled"); } @Test @@ -355,10 +355,10 @@ void hardenReaderBlocksParseText() throws Exception { final SAXParserFactory unhardenedFactory = SAXParserFactory.newInstance(); unhardenedFactory.setNamespaceAware(true); assumeXIncludeAware(unhardenedFactory); - final XMLReader reader = XmlFactories.harden(unhardenedFactory.newSAXParser().getXMLReader()); + final XMLReader reader = SAXParserHardener.hardenReader(unhardenedFactory.newSAXParser().getXMLReader()); final String captured = captureCharacters(reader, input); assertFalse(captured.contains(LEAKED_MARKER), - "harden(reader) parse=text must resolve the include to empty, not leak; got: " + captured); + "hardenReader parse=text must resolve the include to empty, not leak; got: " + captured); } @Test @@ -369,11 +369,11 @@ void hardenReaderAllowListResolvesParseXml() throws Exception { final SAXParserFactory unhardenedFactory = SAXParserFactory.newInstance(); unhardenedFactory.setNamespaceAware(true); assumeXIncludeAware(unhardenedFactory); - final XMLReader reader = XmlFactories.harden(unhardenedFactory.newSAXParser().getXMLReader()); + final XMLReader reader = SAXParserHardener.hardenReader(unhardenedFactory.newSAXParser().getXMLReader()); reader.setEntityResolver(new AllowListResolver()); final String captured = captureCharacters(reader, input); assertEquals(RESOLVED_MARKER, captured.trim(), - "harden(reader) + allow-list should resolve to the resolver's content on a reader with XInclude already enabled"); + "hardenReader + allow-list should resolve to the resolver's content on a reader with XInclude already enabled"); } //endregion
