Skip to content

Use hardened XmlService.newXMLInputFactory() consistently in extractModelId#12480

Open
gnodet wants to merge 1 commit into
apache:masterfrom
gnodet:quick-fix/use-hardened-xml-factory-in-extractModelId
Open

Use hardened XmlService.newXMLInputFactory() consistently in extractModelId#12480
gnodet wants to merge 1 commit into
apache:masterfrom
gnodet:quick-fix/use-hardened-xml-factory-in-extractModelId

Conversation

@gnodet

@gnodet gnodet commented Jul 13, 2026

Copy link
Copy Markdown
Contributor

Summary

  • Replace raw XMLInputFactory.newFactory() with XmlService.newXMLInputFactory() in DefaultModelXmlFactory.extractModelId to use the same hardened factory (DTD and external entity support disabled) that the rest of the codebase uses.
  • Remove the unused InputFactoryHolder inner class.
  • Add test for POM parsing with DOCTYPE declarations.

Test plan

  • Existing DefaultModelXmlFactoryTest tests pass (9/9)
  • New testReadPomWithDoctypeDeclaration test verifies graceful handling of DOCTYPEs during the pre-parse

🤖 Generated with Claude Code

…odelId

The extractModelId pre-parse methods were creating a raw
XMLInputFactory.newFactory() instead of using the hardened
XmlService.newXMLInputFactory() that the rest of the codebase uses.
Also removed the unused InputFactoryHolder inner class.

Co-Authored-By: Claude Opus 4.6 <noreply@anthropic.com>

@gnodet gnodet left a comment

Copy link
Copy Markdown
Contributor Author

Choose a reason for hiding this comment

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

Review: Use hardened XmlService.newXMLInputFactory() consistently in extractModelId

Verdict: LGTM

Good security fix. The two `extractModelId` overloads in `DefaultModelXmlFactory` were the only production code paths using raw `XMLInputFactory.newFactory()` — which does not disable DTD or external entity processing. This PR replaces them with the hardened `XmlService.newXMLInputFactory()` (which sets `SUPPORT_DTD=false` and `IS_SUPPORTING_EXTERNAL_ENTITIES=false`), closing an XXE vector during POM pre-parsing.

What's fixed:

  • A crafted POM with `>` could have triggered external entity resolution during the `extractModelId` pre-parse. Exploitation was somewhat limited (exceptions are caught → returns `null`), but the SSRF/file-read risk was still real depending on the XML parser implementation.

Dead code removal: The unused `InputFactoryHolder` inner class configured a factory that was never referenced — clean removal.

New test: `testReadPomWithDoctypeDeclaration` verifies that a POM with an external entity DOCTYPE is parsed successfully without attempting entity resolution.

Remaining raw `XMLInputFactory.newFactory()` in codebase (informational — not this PR's scope):

  • Test code (`XmlNodeBuilderTest`) — lower risk
  • Auto-generated Modello `*StaxReader.java` files — needs fix in Modello code generator
  • `XmlService.java` itself — intentional (it creates then hardens the factory)

This review was generated by an AI agent and may contain inaccuracies. Please verify all suggestions before applying.

Claude Code on behalf of Guillaume Nodet

gnodet added a commit to gnodet/maven that referenced this pull request Jul 13, 2026
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.

1 participant