Skip to content

Harden the XPath InputSource evaluation surface - #44

Merged
garydgregory merged 16 commits into
apache:mainfrom
ppkarwasz:fix/xpath-inputsource-hardening
Aug 26, 2026
Merged

Harden the XPath InputSource evaluation surface#44
garydgregory merged 16 commits into
apache:mainfrom
ppkarwasz:fix/xpath-inputsource-hardening

Conversation

@ppkarwasz

Copy link
Copy Markdown
Member

Hardens the document parse behind the InputSource-taking XPath evaluation entry points.

Problem

FEATURE_SECURE_PROCESSING on an XPathFactory governs only the XPath engine. The stock JDK and Apache Xalan implement XPath.evaluate(String, InputSource) and XPathExpression.evaluate(InputSource) 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. The library's own DocumentBuilder/SAXParser refuse the identical document.

Changes

XPathHardener's generic branch now returns a HardeningXPathFactory instead of the bare factory (the Saxon branch is unchanged: SaxonProvider's Configuration.makeParser already hardens Saxon's document builds):

  • HardeningXPathFactory wraps newXPath() in a HardeningXPath, delegating everything else raw.
  • 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 inside the document resolves to empty on the resolver floor, like every other hardened parse. compile(String) wraps the compiled expression in a HardeningXPathExpression, which applies the same rewrite to evaluate(InputSource[, QName]). The evaluateExpression default methods added by Java 9 route through the overridden overloads, so the Java 8 build stays covered on newer runtimes.

The newXPathFactory() javadoc documents the covered entry points, and the threat model's Scope and intended use enumeration gains XPath objects (the report's tmd-2 correction).

Tests

New XPathInputSourceTest, tagged xpath, which populates the previously empty xpath surefire group; it runs under test-stockjdk, test-xalan and test-xalan-xerces (Saxon's separate path is covered by SaxonXPathExternalCallsTest under xpath3):

  • blocks-or-does-not-leak assertions for XPath.evaluate and the compiled XPathExpression.evaluate over a document whose external entity would otherwise surface in the result;
  • a positive control evaluating an entity-free document end to end through the hardened pre-parse;
  • a leak control proving the unconfigured engine resolves the entity, which also verified the new tests discriminate: with the wrapper removed, both hardened tests fail on all three executions.

ShadingFootprintTest expectations updated: the XPath closure gains the three wrappers plus the DOM hardener set it now parses through, and the whole-library count moves from 29 to 32.

Full mvn clean and plain mvn (checkstyle, spotbugs, pmd, javadoc, and the whole surefire matrix) green, plus a post-build non-vacuous spotbugs:check.

🤖 Generated with Claude Code

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 <noreply@anthropic.com>

@garydgregory garydgregory left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Hi @ppkarwasz
Please see my comments.
Thank you!

Comment thread src/main/java/org/apache/commons/xml/HardeningXPath.java Outdated
Comment thread src/main/java/org/apache/commons/xml/HardeningXPath.java Outdated
Comment thread src/main/java/org/apache/commons/xml/HardeningXPath.java Outdated
Comment thread src/test/java/org/apache/commons/xml/ShadingFootprintTest.java Outdated
Comment thread src/test/java/org/apache/commons/xml/ShadingFootprintTest.java Outdated
Comment thread src/test/java/org/apache/commons/xml/XPathInputSourceTest.java Outdated
Comment thread src/test/java/org/apache/commons/xml/XPathInputSourceTest.java Outdated
Comment thread src/main/java/org/apache/commons/xml/HardeningXPathExpression.java Outdated
Comment thread src/main/java/org/apache/commons/xml/HardeningXPathFactory.java Outdated
Comment thread src/main/java/org/apache/commons/xml/HardeningXPathFactory.java Outdated
@ppkarwasz
ppkarwasz requested a review from garydgregory August 25, 2026 19:16

@garydgregory garydgregory left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same as before and in other PRs: if there a is a non-deterministic failure, add a clearer comment explaining when it can happen; if it’s an invariant, use assertThrows() instead of using try with an empty catch.

Comment thread src/test/java/org/apache/commons/xml/XPathInputSourceTest.java Outdated
Comment thread src/test/java/org/apache/commons/xml/XPathInputSourceTest.java Outdated
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 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_013LCpUPjNPYVctKN6yBw9w7
@ppkarwasz

Copy link
Copy Markdown
Member Author

Applying the same analysis as in #42: I probed every combination that runs these tests (JDK 8–25 × the stock JDK and Xalan XPath engines, over both DOM parsers) and 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. Since not throwing is the invariant, c55c682 drops the tolerant try/catch and asserts the strict no-throw/no-leak outcome with assertDoesNotThrow.

@ppkarwasz
ppkarwasz requested a review from garydgregory August 25, 2026 21:29
Assisted-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_019U2W3bbZfMCw7tP4VpecKz
Comment thread src/test/java/org/apache/commons/xml/XPathInputSourceTest.java Outdated
@garydgregory
garydgregory merged commit b013746 into apache:main Aug 26, 2026
18 checks passed
@ppkarwasz
ppkarwasz deleted the fix/xpath-inputsource-hardening branch August 26, 2026 11:23
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants