From 78772f1d4475ab57d442c68916293a5db0c911df Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Wed, 19 Aug 2026 13:12:02 +0200 Subject: [PATCH 01/10] feat: harden the XPath InputSource evaluation surface FEATURE_SECURE_PROCESSING on an XPathFactory governs only the XPath engine: the stock JDK and Apache Xalan implement the InputSource-taking evaluate entry points by provisioning an internal document parser the feature does not reach, so external references inside the evaluated document were resolved at that parser's defaults, addressing finding f007. The generic branch of XPathHardener now returns a HardeningXPathFactory (the Saxon branch is unchanged; its Configuration.makeParser already hardens Saxon's document builds): - HardeningXPath performs the document build behind evaluate(String, InputSource[, QName]) itself, through a hardened, namespace-aware DocumentBuilder, and evaluates the delegate against the parsed Document, so the engine's own parser never runs; an external reference resolves to empty on the resolver floor like every other hardened parse. - HardeningXPathExpression applies the same rewrite to the compiled evaluate(InputSource[, QName]); the Java 9 evaluateExpression default methods route through the overridden overloads. New XPathInputSourceTest populates the previously empty xpath surefire group (test-stockjdk, test-xalan, test-xalan-xerces) with blocks-or-does-not-leak assertions, a positive control and a leak control; discrimination verified with the wrapper removed. ShadingFootprintTest gains the three wrappers plus the DOM hardener set in the XPath closure (whole library 29 -> 32). The newXPathFactory javadoc and the threat model's enumerated hardened surface now name the XPath objects. Assisted-By: Claude Fable 5 --- .../apache/commons/xml/HardeningXPath.java | 141 ++++++++++++++++++ .../commons/xml/HardeningXPathExpression.java | 62 ++++++++ .../commons/xml/HardeningXPathFactory.java | 73 +++++++++ .../org/apache/commons/xml/XPathHardener.java | 6 +- .../org/apache/commons/xml/XmlFactories.java | 3 + src/site/markdown/threat_model.md | 2 +- .../commons/xml/ShadingFootprintTest.java | 12 +- .../commons/xml/XPathInputSourceTest.java | 88 +++++++++++ 8 files changed, 380 insertions(+), 7 deletions(-) create mode 100644 src/main/java/org/apache/commons/xml/HardeningXPath.java create mode 100644 src/main/java/org/apache/commons/xml/HardeningXPathExpression.java create mode 100644 src/main/java/org/apache/commons/xml/HardeningXPathFactory.java create mode 100644 src/test/java/org/apache/commons/xml/XPathInputSourceTest.java diff --git a/src/main/java/org/apache/commons/xml/HardeningXPath.java b/src/main/java/org/apache/commons/xml/HardeningXPath.java new file mode 100644 index 00000000..3fefc6e3 --- /dev/null +++ b/src/main/java/org/apache/commons/xml/HardeningXPath.java @@ -0,0 +1,141 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.commons.xml; + +import java.io.IOException; + +import javax.xml.namespace.NamespaceContext; +import javax.xml.namespace.QName; +import javax.xml.parsers.DocumentBuilderFactory; +import javax.xml.parsers.ParserConfigurationException; +import javax.xml.xpath.XPath; +import javax.xml.xpath.XPathExpression; +import javax.xml.xpath.XPathExpressionException; +import javax.xml.xpath.XPathFunctionResolver; +import javax.xml.xpath.XPathVariableResolver; + +import org.w3c.dom.Document; +import org.xml.sax.InputSource; +import org.xml.sax.SAXException; + +/** + * {@link XPath} wrapper that performs the document build behind every {@link InputSource}-taking {@code evaluate} call with a hardened, namespace-aware + * {@link javax.xml.parsers.DocumentBuilder} and evaluates the delegate against the parsed {@link Document}, so the engine's own parser never runs. + * + *

The JAXP contract for {@link XPath#evaluate(String, InputSource, QName)} is "build a document from the source, then evaluate against it", and both the + * stock JDK and Apache Xalan provision an internal parser for that build which {@link javax.xml.XMLConstants#FEATURE_SECURE_PROCESSING} on the + * {@link javax.xml.xpath.XPathFactory} does not reach. Parsing here puts the build on the library's resolver floor: an external reference inside the document + * resolves to empty content, so it is neither fetched nor leaked, and the evaluation proceeds on whatever the parse produced. {@link #compile(String)} wraps + * the compiled expression in a {@link HardeningXPathExpression} on the same terms.

+ * + *

The {@code evaluateExpression} default methods added to the interface by Java 9 route through the {@code evaluate} overloads overridden here, so they + * carry the same rewrite on newer runtimes even though this class targets Java 8.

+ */ +final class HardeningXPath implements XPath { + + /** + * Parses the source through a hardened, namespace-aware {@link javax.xml.parsers.DocumentBuilder}, mirroring the namespace awareness of the parser the + * engine would have provisioned. + * + * @param source The document to evaluate against. + * @return The parsed document. + * @throws NullPointerException if {@code source} is {@code null}, per the {@link XPath} contract. + * @throws XPathExpressionException if the source cannot be parsed. + */ + static Document parse(final InputSource source) throws XPathExpressionException { + if (source == null) { + throw new NullPointerException("source cannot be null"); + } + try { + final DocumentBuilderFactory factory = DocumentBuilderHardener.harden(DocumentBuilderFactory.newInstance()); + factory.setNamespaceAware(true); + return factory.newDocumentBuilder().parse(source); + } catch (final ParserConfigurationException | SAXException | IOException e) { + throw new XPathExpressionException(e); + } + } + + private final XPath delegate; + + HardeningXPath(final XPath delegate) { + this.delegate = delegate; + } + + @Override + public String evaluate(final String expression, final InputSource source) throws XPathExpressionException { + return delegate.evaluate(expression, parse(source)); + } + + @Override + public Object evaluate(final String expression, final InputSource source, final QName returnType) throws XPathExpressionException { + return delegate.evaluate(expression, parse(source), returnType); + } + + @Override + public XPathExpression compile(final String expression) throws XPathExpressionException { + final XPathExpression compiled = delegate.compile(expression); + return compiled == null ? null : new HardeningXPathExpression(compiled); + } + + // + @Override + public String evaluate(final String expression, final Object item) throws XPathExpressionException { + return delegate.evaluate(expression, item); + } + + @Override + public Object evaluate(final String expression, final Object item, final QName returnType) throws XPathExpressionException { + return delegate.evaluate(expression, item, returnType); + } + + @Override + public NamespaceContext getNamespaceContext() { + return delegate.getNamespaceContext(); + } + + @Override + public XPathFunctionResolver getXPathFunctionResolver() { + return delegate.getXPathFunctionResolver(); + } + + @Override + public XPathVariableResolver getXPathVariableResolver() { + return delegate.getXPathVariableResolver(); + } + + @Override + public void reset() { + delegate.reset(); + } + + @Override + public void setNamespaceContext(final NamespaceContext nsContext) { + delegate.setNamespaceContext(nsContext); + } + + @Override + public void setXPathFunctionResolver(final XPathFunctionResolver resolver) { + delegate.setXPathFunctionResolver(resolver); + } + + @Override + public void setXPathVariableResolver(final XPathVariableResolver resolver) { + delegate.setXPathVariableResolver(resolver); + } + // +} diff --git a/src/main/java/org/apache/commons/xml/HardeningXPathExpression.java b/src/main/java/org/apache/commons/xml/HardeningXPathExpression.java new file mode 100644 index 00000000..39317d65 --- /dev/null +++ b/src/main/java/org/apache/commons/xml/HardeningXPathExpression.java @@ -0,0 +1,62 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.commons.xml; + +import javax.xml.namespace.QName; +import javax.xml.xpath.XPathExpression; +import javax.xml.xpath.XPathExpressionException; + +import org.xml.sax.InputSource; + +/** + * {@link XPathExpression} wrapper that applies the same {@link InputSource} rewrite as {@link HardeningXPath} to the compiled evaluation entry points. + * + *

{@link HardeningXPath#compile(String)} returns one of these, so {@link #evaluate(InputSource)} and {@link #evaluate(InputSource, QName)} build the + * document through a hardened, namespace-aware parser instead of the engine's own; the {@code evaluateExpression} default methods added by Java 9 route + * through these overloads as well.

+ */ +final class HardeningXPathExpression implements XPathExpression { + + private final XPathExpression delegate; + + HardeningXPathExpression(final XPathExpression delegate) { + this.delegate = delegate; + } + + @Override + public String evaluate(final InputSource source) throws XPathExpressionException { + return delegate.evaluate(HardeningXPath.parse(source)); + } + + @Override + public Object evaluate(final InputSource source, final QName returnType) throws XPathExpressionException { + return delegate.evaluate(HardeningXPath.parse(source), returnType); + } + + // + @Override + public String evaluate(final Object item) throws XPathExpressionException { + return delegate.evaluate(item); + } + + @Override + public Object evaluate(final Object item, final QName returnType) throws XPathExpressionException { + return delegate.evaluate(item, returnType); + } + // +} diff --git a/src/main/java/org/apache/commons/xml/HardeningXPathFactory.java b/src/main/java/org/apache/commons/xml/HardeningXPathFactory.java new file mode 100644 index 00000000..9cfaa68b --- /dev/null +++ b/src/main/java/org/apache/commons/xml/HardeningXPathFactory.java @@ -0,0 +1,73 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.commons.xml; + +import javax.xml.xpath.XPath; +import javax.xml.xpath.XPathFactory; +import javax.xml.xpath.XPathFactoryConfigurationException; +import javax.xml.xpath.XPathFunctionResolver; +import javax.xml.xpath.XPathVariableResolver; + +/** + * {@link XPathFactory} wrapper that returns a {@link HardeningXPath} from {@link #newXPath()}. + * + *

Required because {@link javax.xml.XMLConstants#FEATURE_SECURE_PROCESSING} on the factory governs only the XPath engine: the stock JDK and Apache Xalan + * implement the {@link org.xml.sax.InputSource}-taking {@code evaluate} entry points by provisioning an internal document parser the feature does not reach. + * The wrapper performs that document build itself through a hardened parser instead; see {@link HardeningXPath}.

+ */ +final class HardeningXPathFactory extends XPathFactory { + + private final XPathFactory delegate; + + HardeningXPathFactory(final XPathFactory delegate) { + this.delegate = delegate; + } + + @Override + public XPath newXPath() { + final XPath xpath = delegate.newXPath(); + return xpath == null ? null : new HardeningXPath(xpath); + } + + // + @Override + public boolean getFeature(final String name) throws XPathFactoryConfigurationException { + return delegate.getFeature(name); + } + + @Override + public boolean isObjectModelSupported(final String objectModel) { + return delegate.isObjectModelSupported(objectModel); + } + + @Override + public void setFeature(final String name, final boolean value) throws XPathFactoryConfigurationException { + delegate.setFeature(name, value); + } + + @Override + public void setXPathFunctionResolver(final XPathFunctionResolver resolver) { + delegate.setXPathFunctionResolver(resolver); + } + + @Override + public void setXPathVariableResolver(final XPathVariableResolver resolver) { + delegate.setXPathVariableResolver(resolver); + } + // +} diff --git a/src/main/java/org/apache/commons/xml/XPathHardener.java b/src/main/java/org/apache/commons/xml/XPathHardener.java index 4a995cd8..b64ee913 100644 --- a/src/main/java/org/apache/commons/xml/XPathHardener.java +++ b/src/main/java/org/apache/commons/xml/XPathHardener.java @@ -40,6 +40,9 @@ * the bundled SAX parser, blocking a sysprop swap to a third-party parser (defense-in-depth); Xalan rejects the feature and is left unchanged. *
  • FSP ({@link XMLConstants#FEATURE_SECURE_PROCESSING}): required. It is the only knob both the stock JDK and Xalan XPath engines expose, * and switches on their secure-processing limits. {@link XPathFactory} has no attribute API for finer control.
    • + *
  • {@link HardeningXPathFactory}: required. FSP governs only the engine, not the parser it provisions internally for the + * {@link org.xml.sax.InputSource}-taking {@code evaluate} entry points; the wrapper performs that document build with a hardened parser instead, so + * the engine never parses.
    • * */ final class XPathHardener { @@ -67,7 +70,8 @@ static XPathFactory harden(final XPathFactory factory) { setOptionalFeature(factory, FEATURE_OVERRIDE_DEFAULT_PARSER, false); // Required: enables the engine's secure-processing limits; XPathFactory has no attribute API for finer control. setFeature(factory, XMLConstants.FEATURE_SECURE_PROCESSING, true); - return factory; + // Required: FSP does not reach the parser the engine provisions for InputSource-taking evaluate calls; the wrapper parses those itself. + return new HardeningXPathFactory(factory); } private static void setFeature(final XPathFactory factory, final String feature, final boolean value) { diff --git a/src/main/java/org/apache/commons/xml/XmlFactories.java b/src/main/java/org/apache/commons/xml/XmlFactories.java index fae5f9a8..6c204cfa 100644 --- a/src/main/java/org/apache/commons/xml/XmlFactories.java +++ b/src/main/java/org/apache/commons/xml/XmlFactories.java @@ -173,6 +173,9 @@ public static XMLInputFactory newXMLInputFactory() { *

    Beyond the three universal guarantees on {@link XmlFactories}, URI-fetching XPath 3.1+ functions ({@code doc()}, {@code collection()}, * {@code unparsed-text()}) are not resolved.

    * + *

    The guarantees also cover the document parse behind {@code XPath.evaluate(String, InputSource)} and {@code XPathExpression.evaluate(InputSource)}: + * the input document is built through a hardened, namespace-aware {@link javax.xml.parsers.DocumentBuilder} instead of the engine's internal parser.

    + * * @return A hardened factory. * @throws IllegalStateException if a required hardening setting cannot be applied to the underlying implementation. */ diff --git a/src/site/markdown/threat_model.md b/src/site/markdown/threat_model.md index fa5154ec..e046b697 100644 --- a/src/site/markdown/threat_model.md +++ b/src/site/markdown/threat_model.md @@ -47,7 +47,7 @@ makes is documented in the Javadoc: https://commons.apache.org/sandbox/commons-xml/apidocs/org/apache/commons/xml/factory/XmlFactories.html -The hardening applies to the factory and to the parsers, readers, transformers, validators and schemas it produces. +The hardening applies to the factory and to the parsers, readers, transformers, validators, schemas and XPath objects it produces. ### Adversary model and trust boundary diff --git a/src/test/java/org/apache/commons/xml/ShadingFootprintTest.java b/src/test/java/org/apache/commons/xml/ShadingFootprintTest.java index 06c4ab70..c1c42b07 100644 --- a/src/test/java/org/apache/commons/xml/ShadingFootprintTest.java +++ b/src/test/java/org/apache/commons/xml/ShadingFootprintTest.java @@ -43,8 +43,8 @@ * *

    Using {@code jdependency}, the same library {@code maven-shade-plugin}'s {@code minimizeJar} uses, this test computes each entry point's transitive class * closure over the compiled {@code target/classes} and pins it to an expected set. It keeps each hardener from silently regaining a dependency on classes it - * should not need (for example a sibling resolver floor or another hardener), so TrAX, XPath and schema build only on the shared SAX path while only the public - * {@link XmlFactories} entry pulls the whole library. Update the expected sets deliberately: a change here is a change to what a downstream shade includes.

    + * should not need (for example a sibling resolver floor or another hardener), so TrAX and schema build only on the shared SAX path, XPath additionally on the + * DOM path its InputSource rewrite parses through, while only the public {@link XmlFactories} entry pulls the whole library. Update the expected sets deliberately: a change here is a change to what a downstream shade includes.

    * *

    The test reads the compiled {@code .class} files from the code-source location, which only exists on a regular JVM: a native image carries no bytecode (and * nobody shades one), so the test is disabled there, just as it is excluded from the Android test compile.

    @@ -69,14 +69,16 @@ class ShadingFootprintTest { private static final Set STAX_HARDENER = set("StaxHardener", "HardeningXMLInputFactory", "FallbackIgnoreXMLResolver", HARDENING_EXCEPTION); /** - * TrAX, XPath and schema re-harden their sub-parsers through {@link SAXParserHardener#harden(Source)}, so each builds on the full SAX closure below. + * TrAX, XPath and schema re-harden their sub-parsers through {@link SAXParserHardener#harden(Source)}, so each builds on the full SAX closure below; XPath + * additionally parses InputSource-taking evaluate calls through the DOM hardener, so its closure carries that set too. */ private static final Set TRANSFORMER_HARDENER = saxParsersHardenerPlus("TransformerHardener", "HardeningTransformerFactory", "HardeningTransformer", "HardeningTemplates", "FallbackIgnoreURIResolver", "SaxonProvider", "SaxonProvider$1", "SaxonProvider$HardenedConfiguration" , "SaxonProvider$SaxonProviderConfigurer"); private static final Set XPATH_HARDENER = saxParsersHardenerPlus("XPathHardener", "SaxonProvider", "SaxonProvider$1", - "SaxonProvider$HardenedConfiguration", "SaxonProvider$SaxonProviderConfigurer"); + "SaxonProvider$HardenedConfiguration", "SaxonProvider$SaxonProviderConfigurer", "HardeningXPathFactory", "HardeningXPath", + "HardeningXPathExpression", "DocumentBuilderHardener", "HardeningDocumentBuilder", "HardeningDocumentBuilderFactory"); private static final Set SCHEMA_HARDENER = saxParsersHardenerPlus("SchemaHardener", "HardeningSchemaFactory", "HardeningValidator", "HardeningValidatorHandler", "HardeningSchema", "FallbackIgnoreLSResourceResolver"); @@ -84,7 +86,7 @@ class ShadingFootprintTest { /** * Only the public {@link XmlFactories} entry, which news up every hardener, still pulls the whole library; this is its class count. */ - private static final int WHOLE_LIBRARY_SIZE = 29; + private static final int WHOLE_LIBRARY_SIZE = 32; /** * Entry points reported by the {@link #reportFootprint()} diagnostic, most-focused first, ending with the whole library. diff --git a/src/test/java/org/apache/commons/xml/XPathInputSourceTest.java b/src/test/java/org/apache/commons/xml/XPathInputSourceTest.java new file mode 100644 index 00000000..2c5a4e1a --- /dev/null +++ b/src/test/java/org/apache/commons/xml/XPathInputSourceTest.java @@ -0,0 +1,88 @@ +/* + * Licensed to the Apache Software Foundation (ASF) under one or more + * contributor license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright ownership. + * The ASF licenses this file to You under the Apache License, Version 2.0 + * (the "License"); you may not use this file except in compliance with + * the License. You may obtain a copy of the License at + * + * https://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, + * WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. + * See the License for the specific language governing permissions and + * limitations under the License. + */ + +package org.apache.commons.xml; + +import static org.junit.jupiter.api.Assertions.assertEquals; +import static org.junit.jupiter.api.Assertions.assertFalse; +import static org.junit.jupiter.api.Assertions.assertTrue; + +import javax.xml.xpath.XPathExpressionException; +import javax.xml.xpath.XPathFactory; + +import org.junit.jupiter.api.Tag; +import org.junit.jupiter.api.Test; + +/** + * Checks that the document parse behind {@code XPath.evaluate(String, InputSource)} (and its compiled {@code XPathExpression} counterpart) cannot pull in an + * external general entity. + * + *

    The stock JDK and Apache Xalan implement the {@link org.xml.sax.InputSource}-taking {@code evaluate} entry points by provisioning an internal document + * parser that {@code FEATURE_SECURE_PROCESSING} on the {@link XPathFactory} does not reach. The {@link HardeningXPathFactory} wrapper parses the input + * through a hardened {@code DocumentBuilder} instead, so the external reference resolves to empty on the floor (or the parse is rejected outright), while the + * evaluation itself still works. Tagged {@code xpath}, so it runs under test-stockjdk, test-xalan and test-xalan-xerces; the Saxon engine takes the separate + * {@code SaxonProvider} path covered by {@code SaxonXPathExternalCallsTest}.

    + */ +@Tag("xpath") +class XPathInputSourceTest { + + private static final String EXPRESSION = "string(/root/child)"; + + /** {@link AttackTestSupport#xmlBody} content whose single entity reference resolves to {@link AttackTestSupport#LEAKED_MARKER} if the DTD is fetched. */ + private static String entityPayload() { + return "\n" + + "\n]>\n" + + AttackTestSupport.xmlBody("&xxe;"); + } + + @Test + void hardenedXPathEvaluateDoesNotLeak() throws Exception { + final String result; + try { + result = XmlFactories.newXPathFactory().newXPath().evaluate(EXPRESSION, AttackTestSupport.inputSource(entityPayload())); + } catch (final XPathExpressionException blocked) { + return; // Acceptable: the parse rejected the reference rather than resolving it to empty. + } + assertFalse(result.contains(AttackTestSupport.LEAKED_MARKER), "external entity leaked into the XPath result: " + result); + } + + @Test + void hardenedXPathExpressionEvaluateDoesNotLeak() throws Exception { + final String result; + try { + result = XmlFactories.newXPathFactory().newXPath().compile(EXPRESSION).evaluate(AttackTestSupport.inputSource(entityPayload())); + } catch (final XPathExpressionException blocked) { + return; // Acceptable: the parse rejected the reference rather than resolving it to empty. + } + assertFalse(result.contains(AttackTestSupport.LEAKED_MARKER), "external entity leaked into the compiled XPath result: " + result); + } + + @Test + void hardenedXPathEvaluatesPlainDocument() throws Exception { + // Positive control: the hardened pre-parse still evaluates an entity-free document end to end. + final String result = XmlFactories.newXPathFactory().newXPath().evaluate(EXPRESSION, + AttackTestSupport.inputSource(AttackTestSupport.xmlBody("plain text"))); + assertEquals("plain text", result, "hardened XPath should evaluate a plain document"); + } + + @Test + void unconfiguredXPathEvaluateLeaks() throws Exception { + // Leak control: the unconfigured engine's internal parser resolves the entity, which is exactly what the wrapper exists to prevent. + final String result = XPathFactory.newInstance().newXPath().evaluate(EXPRESSION, AttackTestSupport.inputSource(entityPayload())); + assertTrue(result.contains(AttackTestSupport.LEAKED_MARKER), "unconfigured XPath was expected to resolve the external entity, got: " + result); + } +} From 98125dc2ef0a2657554786aa72b4b36430fb11fc Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Tue, 25 Aug 2026 20:55:17 +0200 Subject: [PATCH 02/10] fix: remove comments --- src/main/java/org/apache/commons/xml/HardeningXPath.java | 2 -- .../java/org/apache/commons/xml/HardeningXPathExpression.java | 2 -- src/main/java/org/apache/commons/xml/HardeningXPathFactory.java | 2 -- 3 files changed, 6 deletions(-) diff --git a/src/main/java/org/apache/commons/xml/HardeningXPath.java b/src/main/java/org/apache/commons/xml/HardeningXPath.java index 3fefc6e3..da33bb55 100644 --- a/src/main/java/org/apache/commons/xml/HardeningXPath.java +++ b/src/main/java/org/apache/commons/xml/HardeningXPath.java @@ -92,7 +92,6 @@ public XPathExpression compile(final String expression) throws XPathExpressionEx return compiled == null ? null : new HardeningXPathExpression(compiled); } - // @Override public String evaluate(final String expression, final Object item) throws XPathExpressionException { return delegate.evaluate(expression, item); @@ -137,5 +136,4 @@ public void setXPathFunctionResolver(final XPathFunctionResolver resolver) { public void setXPathVariableResolver(final XPathVariableResolver resolver) { delegate.setXPathVariableResolver(resolver); } - // } diff --git a/src/main/java/org/apache/commons/xml/HardeningXPathExpression.java b/src/main/java/org/apache/commons/xml/HardeningXPathExpression.java index 39317d65..7a1446fc 100644 --- a/src/main/java/org/apache/commons/xml/HardeningXPathExpression.java +++ b/src/main/java/org/apache/commons/xml/HardeningXPathExpression.java @@ -48,7 +48,6 @@ public Object evaluate(final InputSource source, final QName returnType) throws return delegate.evaluate(HardeningXPath.parse(source), returnType); } - // @Override public String evaluate(final Object item) throws XPathExpressionException { return delegate.evaluate(item); @@ -58,5 +57,4 @@ public String evaluate(final Object item) throws XPathExpressionException { public Object evaluate(final Object item, final QName returnType) throws XPathExpressionException { return delegate.evaluate(item, returnType); } - // } diff --git a/src/main/java/org/apache/commons/xml/HardeningXPathFactory.java b/src/main/java/org/apache/commons/xml/HardeningXPathFactory.java index 9cfaa68b..974448ee 100644 --- a/src/main/java/org/apache/commons/xml/HardeningXPathFactory.java +++ b/src/main/java/org/apache/commons/xml/HardeningXPathFactory.java @@ -44,7 +44,6 @@ public XPath newXPath() { return xpath == null ? null : new HardeningXPath(xpath); } - // @Override public boolean getFeature(final String name) throws XPathFactoryConfigurationException { return delegate.getFeature(name); @@ -69,5 +68,4 @@ public void setXPathFunctionResolver(final XPathFunctionResolver resolver) { public void setXPathVariableResolver(final XPathVariableResolver resolver) { delegate.setXPathVariableResolver(resolver); } - // } From 6315ef84d6731d2fc57e2cc6355e354a0db85738 Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Tue, 25 Aug 2026 20:59:11 +0200 Subject: [PATCH 03/10] fix: add Objects.requireNonNull to improve documentation --- src/main/java/org/apache/commons/xml/HardeningXPath.java | 3 ++- .../java/org/apache/commons/xml/HardeningXPathFactory.java | 4 +++- 2 files changed, 5 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/apache/commons/xml/HardeningXPath.java b/src/main/java/org/apache/commons/xml/HardeningXPath.java index da33bb55..8db55869 100644 --- a/src/main/java/org/apache/commons/xml/HardeningXPath.java +++ b/src/main/java/org/apache/commons/xml/HardeningXPath.java @@ -18,6 +18,7 @@ package org.apache.commons.xml; import java.io.IOException; +import java.util.Objects; import javax.xml.namespace.NamespaceContext; import javax.xml.namespace.QName; @@ -73,7 +74,7 @@ static Document parse(final InputSource source) throws XPathExpressionException private final XPath delegate; HardeningXPath(final XPath delegate) { - this.delegate = delegate; + this.delegate = Objects.requireNonNull(delegate, "delegate"); } @Override diff --git a/src/main/java/org/apache/commons/xml/HardeningXPathFactory.java b/src/main/java/org/apache/commons/xml/HardeningXPathFactory.java index 974448ee..df49d1d0 100644 --- a/src/main/java/org/apache/commons/xml/HardeningXPathFactory.java +++ b/src/main/java/org/apache/commons/xml/HardeningXPathFactory.java @@ -23,6 +23,8 @@ import javax.xml.xpath.XPathFunctionResolver; import javax.xml.xpath.XPathVariableResolver; +import java.util.Objects; + /** * {@link XPathFactory} wrapper that returns a {@link HardeningXPath} from {@link #newXPath()}. * @@ -35,7 +37,7 @@ final class HardeningXPathFactory extends XPathFactory { private final XPathFactory delegate; HardeningXPathFactory(final XPathFactory delegate) { - this.delegate = delegate; + this.delegate = Objects.requireNonNull(delegate, "delegate"); } @Override From 0e2ea993890132398dd23ddadd633c4131b2c4b6 Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Tue, 25 Aug 2026 21:04:16 +0200 Subject: [PATCH 04/10] fix: rename `WHOLE_LIBRARY_SIZE` -> `LIBRARY_CLASS_COUNT` --- .../java/org/apache/commons/xml/ShadingFootprintTest.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/test/java/org/apache/commons/xml/ShadingFootprintTest.java b/src/test/java/org/apache/commons/xml/ShadingFootprintTest.java index f7e7e5f1..bcee8c09 100644 --- a/src/test/java/org/apache/commons/xml/ShadingFootprintTest.java +++ b/src/test/java/org/apache/commons/xml/ShadingFootprintTest.java @@ -89,7 +89,7 @@ class ShadingFootprintTest { /** * Only the public {@link XmlFactories} entry, which news up every hardener, still pulls the whole library; this is its class count. */ - private static final int WHOLE_LIBRARY_SIZE = 35; + private static final int LIBRARY_CLASS_COUNT = 35; /** * Entry points reported by the {@link #reportFootprint()} diagnostic, most-focused first, ending with the whole library. @@ -155,7 +155,7 @@ void schemaHardenerFootprint() { @Test void onlyXmlFactoriesPullsTheWholeLibrary() { - assertEquals(WHOLE_LIBRARY_SIZE, closureOf("XmlFactories").size(), "XmlFactories closure size drifted"); + assertEquals(LIBRARY_CLASS_COUNT, closureOf("XmlFactories").size(), "XmlFactories closure size drifted"); } /** From b914ebb8537636f08c417eb84a0a55b12a40e49c Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Tue, 25 Aug 2026 21:11:04 +0200 Subject: [PATCH 05/10] fix: *DoesNotLeak comments --- .../commons/xml/XPathInputSourceTest.java | 18 ++++++++---------- 1 file changed, 8 insertions(+), 10 deletions(-) diff --git a/src/test/java/org/apache/commons/xml/XPathInputSourceTest.java b/src/test/java/org/apache/commons/xml/XPathInputSourceTest.java index 2c5a4e1a..ffee1b8a 100644 --- a/src/test/java/org/apache/commons/xml/XPathInputSourceTest.java +++ b/src/test/java/org/apache/commons/xml/XPathInputSourceTest.java @@ -51,24 +51,22 @@ private static String entityPayload() { @Test void hardenedXPathEvaluateDoesNotLeak() throws Exception { - final String result; try { - result = XmlFactories.newXPathFactory().newXPath().evaluate(EXPRESSION, AttackTestSupport.inputSource(entityPayload())); - } catch (final XPathExpressionException blocked) { - return; // Acceptable: the parse rejected the reference rather than resolving it to empty. + final String result = XmlFactories.newXPathFactory().newXPath().evaluate(EXPRESSION, AttackTestSupport.inputSource(entityPayload())); + assertFalse(result.contains(AttackTestSupport.LEAKED_MARKER), "external entity leaked into the XPath result: " + result); + } catch (final XPathExpressionException ignored) { + // Throwing is an acceptable result, since it does not leak the marker. } - assertFalse(result.contains(AttackTestSupport.LEAKED_MARKER), "external entity leaked into the XPath result: " + result); } @Test void hardenedXPathExpressionEvaluateDoesNotLeak() throws Exception { - final String result; try { - result = XmlFactories.newXPathFactory().newXPath().compile(EXPRESSION).evaluate(AttackTestSupport.inputSource(entityPayload())); - } catch (final XPathExpressionException blocked) { - return; // Acceptable: the parse rejected the reference rather than resolving it to empty. + final String result = XmlFactories.newXPathFactory().newXPath().compile(EXPRESSION).evaluate(AttackTestSupport.inputSource(entityPayload())); + assertFalse(result.contains(AttackTestSupport.LEAKED_MARKER), "external entity leaked into the compiled XPath result: " + result); + } catch (final XPathExpressionException ignored) { + // Throwing is an acceptable result, since it does not leak the marker. } - assertFalse(result.contains(AttackTestSupport.LEAKED_MARKER), "external entity leaked into the compiled XPath result: " + result); } @Test From 5a2472c794ad7ff2d9b495b8cc49af9ef7370d6b Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Tue, 25 Aug 2026 21:12:02 +0200 Subject: [PATCH 06/10] fix: checkstyle --- .../java/org/apache/commons/xml/HardeningXPathFactory.java | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/src/main/java/org/apache/commons/xml/HardeningXPathFactory.java b/src/main/java/org/apache/commons/xml/HardeningXPathFactory.java index df49d1d0..c6d4cb09 100644 --- a/src/main/java/org/apache/commons/xml/HardeningXPathFactory.java +++ b/src/main/java/org/apache/commons/xml/HardeningXPathFactory.java @@ -17,14 +17,14 @@ package org.apache.commons.xml; +import java.util.Objects; + import javax.xml.xpath.XPath; import javax.xml.xpath.XPathFactory; import javax.xml.xpath.XPathFactoryConfigurationException; import javax.xml.xpath.XPathFunctionResolver; import javax.xml.xpath.XPathVariableResolver; -import java.util.Objects; - /** * {@link XPathFactory} wrapper that returns a {@link HardeningXPath} from {@link #newXPath()}. * From eb53959e3703f57263f3c349b3d1d74e94928d20 Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Tue, 25 Aug 2026 21:13:46 +0200 Subject: [PATCH 07/10] fix: Javadoc --- src/test/java/org/apache/commons/xml/ShadingFootprintTest.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/src/test/java/org/apache/commons/xml/ShadingFootprintTest.java b/src/test/java/org/apache/commons/xml/ShadingFootprintTest.java index bcee8c09..4972cbac 100644 --- a/src/test/java/org/apache/commons/xml/ShadingFootprintTest.java +++ b/src/test/java/org/apache/commons/xml/ShadingFootprintTest.java @@ -87,7 +87,7 @@ class ShadingFootprintTest { "HardeningValidatorHandler", "HardeningSchema", "FallbackIgnoreLSResourceResolver"); /** - * Only the public {@link XmlFactories} entry, which news up every hardener, still pulls the whole library; this is its class count. + * Only the public {@link XmlFactories} entry, which references every hardener, still pulls the whole library; this is its class count. */ private static final int LIBRARY_CLASS_COUNT = 35; From c55c682af86bf26c0255ba11634773ea8c478b0b Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Tue, 25 Aug 2026 23:24:41 +0200 Subject: [PATCH 08/10] fix: assert the strict no-throw outcome in XPathInputSourceTest Probing every combination that runs the xpath tag (JDK 8-25 with the stock JDK and Xalan engines over both DOM parsers) shows the hardened evaluation never throws: the entity is declared in the internal subset, so the floor only resolves its external content to an empty stream - a legal empty replacement text no parser can reject. Replace the tolerant try/catch with assertDoesNotThrow and drop the stale "or the parse is rejected outright" javadoc parenthetical. Assisted-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_013LCpUPjNPYVctKN6yBw9w7 --- .../commons/xml/XPathInputSourceTest.java | 25 ++++++++----------- 1 file changed, 11 insertions(+), 14 deletions(-) diff --git a/src/test/java/org/apache/commons/xml/XPathInputSourceTest.java b/src/test/java/org/apache/commons/xml/XPathInputSourceTest.java index ffee1b8a..ed5dd6b6 100644 --- a/src/test/java/org/apache/commons/xml/XPathInputSourceTest.java +++ b/src/test/java/org/apache/commons/xml/XPathInputSourceTest.java @@ -17,11 +17,11 @@ package org.apache.commons.xml; +import static org.junit.jupiter.api.Assertions.assertDoesNotThrow; import static org.junit.jupiter.api.Assertions.assertEquals; import static org.junit.jupiter.api.Assertions.assertFalse; import static org.junit.jupiter.api.Assertions.assertTrue; -import javax.xml.xpath.XPathExpressionException; import javax.xml.xpath.XPathFactory; import org.junit.jupiter.api.Tag; @@ -33,7 +33,7 @@ * *

    The stock JDK and Apache Xalan implement the {@link org.xml.sax.InputSource}-taking {@code evaluate} entry points by provisioning an internal document * parser that {@code FEATURE_SECURE_PROCESSING} on the {@link XPathFactory} does not reach. The {@link HardeningXPathFactory} wrapper parses the input - * through a hardened {@code DocumentBuilder} instead, so the external reference resolves to empty on the floor (or the parse is rejected outright), while the + * through a hardened {@code DocumentBuilder} instead, so the external reference resolves to empty on the floor, while the * evaluation itself still works. Tagged {@code xpath}, so it runs under test-stockjdk, test-xalan and test-xalan-xerces; the Saxon engine takes the separate * {@code SaxonProvider} path covered by {@code SaxonXPathExternalCallsTest}.

    */ @@ -51,22 +51,19 @@ private static String entityPayload() { @Test void hardenedXPathEvaluateDoesNotLeak() throws Exception { - try { - final String result = XmlFactories.newXPathFactory().newXPath().evaluate(EXPRESSION, AttackTestSupport.inputSource(entityPayload())); - assertFalse(result.contains(AttackTestSupport.LEAKED_MARKER), "external entity leaked into the XPath result: " + result); - } catch (final XPathExpressionException ignored) { - // Throwing is an acceptable result, since it does not leak the marker. - } + // Deterministic on every engine: the entity is declared in the internal subset and the floor resolves only its + // external content — to empty replacement text — so the pre-parse completes and the reference expands to nothing. + final String result = assertDoesNotThrow( + () -> XmlFactories.newXPathFactory().newXPath().evaluate(EXPRESSION, AttackTestSupport.inputSource(entityPayload()))); + assertFalse(result.contains(AttackTestSupport.LEAKED_MARKER), "external entity leaked into the XPath result: " + result); } @Test void hardenedXPathExpressionEvaluateDoesNotLeak() throws Exception { - try { - final String result = XmlFactories.newXPathFactory().newXPath().compile(EXPRESSION).evaluate(AttackTestSupport.inputSource(entityPayload())); - assertFalse(result.contains(AttackTestSupport.LEAKED_MARKER), "external entity leaked into the compiled XPath result: " + result); - } catch (final XPathExpressionException ignored) { - // Throwing is an acceptable result, since it does not leak the marker. - } + // Same declared-entity outcome as above on the compiled-expression entry point. + final String result = assertDoesNotThrow( + () -> XmlFactories.newXPathFactory().newXPath().compile(EXPRESSION).evaluate(AttackTestSupport.inputSource(entityPayload()))); + assertFalse(result.contains(AttackTestSupport.LEAKED_MARKER), "external entity leaked into the compiled XPath result: " + result); } @Test From 9eea4a244331ef55f6aad7898eb58a117d71c8fc Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Wed, 26 Aug 2026 09:05:53 +0200 Subject: [PATCH 09/10] docs: add changelog entry Assisted-By: Claude Fable 5 Claude-Session: https://claude.ai/code/session_019U2W3bbZfMCw7tP4VpecKz --- src/changes/changes.xml | 1 + 1 file changed, 1 insertion(+) diff --git a/src/changes/changes.xml b/src/changes/changes.xml index 2f0a67e9..8cc43d18 100644 --- a/src/changes/changes.xml +++ b/src/changes/changes.xml @@ -36,6 +36,7 @@ The type attribute can be add, update, fix, or remove. Secure-by-default JAXP factory creation via XmlFactories, with implementation-specific hardening recipes for the stock JDK, Android, Apache Xalan, Apache Xerces, Woodstox, and Saxon-HE. + Harden the document parse behind the InputSource-taking XPath evaluation entry points. From 1ae58fea7c67a231f4e5d81e896cea4e8c8b4d89 Mon Sep 17 00:00:00 2001 From: "Piotr P. Karwasz" Date: Wed, 26 Aug 2026 12:32:19 +0200 Subject: [PATCH 10/10] fix: remove `assertDoesNotThrow` from tests --- .../java/org/apache/commons/xml/XPathInputSourceTest.java | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/src/test/java/org/apache/commons/xml/XPathInputSourceTest.java b/src/test/java/org/apache/commons/xml/XPathInputSourceTest.java index ed5dd6b6..2c18720f 100644 --- a/src/test/java/org/apache/commons/xml/XPathInputSourceTest.java +++ b/src/test/java/org/apache/commons/xml/XPathInputSourceTest.java @@ -53,16 +53,14 @@ private static String entityPayload() { void hardenedXPathEvaluateDoesNotLeak() throws Exception { // Deterministic on every engine: the entity is declared in the internal subset and the floor resolves only its // external content — to empty replacement text — so the pre-parse completes and the reference expands to nothing. - final String result = assertDoesNotThrow( - () -> XmlFactories.newXPathFactory().newXPath().evaluate(EXPRESSION, AttackTestSupport.inputSource(entityPayload()))); + final String result = XmlFactories.newXPathFactory().newXPath().evaluate(EXPRESSION, AttackTestSupport.inputSource(entityPayload())); assertFalse(result.contains(AttackTestSupport.LEAKED_MARKER), "external entity leaked into the XPath result: " + result); } @Test void hardenedXPathExpressionEvaluateDoesNotLeak() throws Exception { // Same declared-entity outcome as above on the compiled-expression entry point. - final String result = assertDoesNotThrow( - () -> XmlFactories.newXPathFactory().newXPath().compile(EXPRESSION).evaluate(AttackTestSupport.inputSource(entityPayload()))); + final String result = XmlFactories.newXPathFactory().newXPath().compile(EXPRESSION).evaluate(AttackTestSupport.inputSource(entityPayload())); assertFalse(result.contains(AttackTestSupport.LEAKED_MARKER), "external entity leaked into the compiled XPath result: " + result); }