Skip to content

parse Source through hardened parser in JAXPXPathEngine - #333

Merged
bodewig merged 3 commits into
xmlunit:mainfrom
jmestwa-coder:xpath-source-external-entities
Jun 20, 2026
Merged

parse Source through hardened parser in JAXPXPathEngine#333
bodewig merged 3 commits into
xmlunit:mainfrom
jmestwa-coder:xpath-source-external-entities

Conversation

@jmestwa-coder

@jmestwa-coder jmestwa-coder commented Jun 13, 2026

Copy link
Copy Markdown
Contributor

JAXPXPathEngine's Source-based xpath methods bypassed the secure parser:

  • selectNodes(String, Source) and evaluate(String, Source) handed Convert.toInputSource(s) to javax.xml.xpath.XPath.evaluate, which parses with JAXP's own default factory, so a DOCTYPE declaring an external SYSTEM entity in the source was resolved during evaluation
  • the Node-based overloads and the HasXPathMatcher/EvaluateXPathMatcher matchers already route through Convert.toNode, which uses DocumentBuilderFactoryConfigurer.Default
  • both Source methods now do the same and delegate to those overloads
    Added a regression test that feeds an external entity through both methods.

@jmestwa-coder

jmestwa-coder commented Jun 18, 2026

Copy link
Copy Markdown
Contributor Author

any update?

@bodewig

bodewig commented Jun 20, 2026

Copy link
Copy Markdown
Member

I forgot to thank you for your contributions when commenting on the other two requests, I really appreciate it.

What would you think about allowing people to explicitly specify a DocumentBuilderFactory in JAXPXPathEngine's constructor (maybe add two new constructors) which the no-arg constructor creating one with a hardened factory by default - and then pass the configured factory down to Convert.toNode?

@jmestwa-coder

jmestwa-coder commented Jun 20, 2026

Copy link
Copy Markdown
Contributor Author

good idea, that reads cleaner. pushed:

  • added JAXPXPathEngine(XPathFactory, DocumentBuilderFactory) and JAXPXPathEngine(DocumentBuilderFactory)
  • the no-arg and XPathFactory-only constructors default to a factory hardened with DocumentBuilderFactoryConfigurer.Default
  • the Source overloads now pass the stored factory to Convert.toNode

also added a test that a caller-supplied factory is honored.

bodewig
bodewig reviewed Jun 20, 2026
View reviewed changes

@bodewig bodewig 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.

Looks good overall, just small nits about the tests you've added.

XMLUnit uses hamcrest matchers in its tests which should provide better error messages than assertTrue in case the assertion fails.

Comment thread xmlunit-core/src/test/java/org/xmlunit/xpath/JAXPXPathEngineTest.java Outdated
Comment thread xmlunit-core/src/test/java/org/xmlunit/xpath/JAXPXPathEngineTest.java Outdated
@bodewig

bodewig commented Jun 20, 2026

Copy link
Copy Markdown
Member

thank you

@bodewig
bodewig merged commit c3b7c7e into xmlunit:main Jun 20, 2026
bodewig added a commit that referenced this pull request Jun 20, 2026
@bodewig
Merge pull request #334 from xmlunit/add-release-notes
f0a6fe6 
add release notes for #331 #332 #333 by @jmestwa-coder
pull Bot pushed a commit to boost-mw-poc/xmlunit_xmlunit that referenced this pull request Jun 20, 2026
@bodewig
add release notes for xmlunit#331 xmlunit#332 xmlunit#333 by @jmestwa…
4765a05 
…-coder
@bodewig bodewig added this to the 2.13.0 milestone Jul 31, 2026
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@bodewig bodewig bodewig left review comments

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

2.13.0

Development

Successfully merging this pull request may close these issues.

2 participants

@jmestwa-coder @bodewig