GROOVY-12331: XmlParser/XmlSlurper: bound element nesting depth - #2858
Merged
Conversation
Secure processing does not bound element depth. The JAXP limit that does, jdk.xml.maxElementDepth, defaults to 0 meaning unlimited, and nothing in groovy-xml set it, so the depth a document could reach was unbounded by default. The SAX parse itself survives that, because nesting is tracked on the heap rather than the stack. The damage lands on the first consumer to walk the result recursively - Node.text(), XmlNodePrinter, GPathResult.toString(), XmlUtil.serialize - each of which runs a stack frame per level. A 350KB document 50,000 elements deep parsed cleanly and then killed every one of them with a StackOverflowError. That is an Error, so it escapes the catch(Exception) an application would reasonably use to handle a malformed document: the failure arrives somewhere the caller is not defending, well after the parse it would have attributed it to. The limit is now set to 1000 by default, matching groovy.json's nesting bound, so a document too deep to walk is refused by the parse that reads it. The JDK enforces the bound itself, which puts the check ahead of every consumer at once and reports the offending element with its depth and position rather than unwinding an anonymous stack. The limit cannot be set on a SAXParserFactory - it is a parser property - so FactorySupport.createSaxParser applies it to the parser it creates, and the four places that built a SAX parser now go through it. The DocumentBuilderFactory route takes it as a factory attribute, which covers DOMBuilder and the DOM paths too. Nothing is applied when jdk.xml.maxElementDepth is already set: that is the standard knob for this limit, and an explicitly set parser property would otherwise override the value a user chose - including a deliberate 0 to restore unlimited depth. A parser supplied by the caller is left alone, as with the other hardening here.
…nt depth The §6 parser table said secure processing and DOCTYPE rejection "cap entity expansion and depth via the JAXP limits". Entity expansion, yes; depth, no. The JAXP limit governing depth, jdk.xml.maxElementDepth, defaults to 0 meaning unlimited, and nothing set it, so the row asserted a bound that did not exist - the kind of claim a reader acts on by not bounding the input themselves. Correcting the row alone would leave the rest of the document inheriting the mistake. The half-sentence had propagated: §6's preamble named JsonSlurper as the only parser with a depth cap, §11a cited only GROOVY-12064 for "bounded by default in all of them", and the closing paragraph credited the JsonSlurper cap with closing the last gap when the gap it describes was the XML one, still open at the time. §5a gains the knob beside groovy.json.maxNestingDepth. All five now say the same thing. The branch caveat matters as much as the correction: 3.0.x, 4.0.x and 5.0.x have neither bound, so depth-bounding untrusted XML there stays a downstream responsibility, exactly as the document already said of JSON. P2 is deliberately untouched. It records XXE and entity expansion, and adding element depth to it would make an unbounded parser a violation of a provided property rather than a hardening obligation, changing how such a report is dispositioned under §13. That is the PMC's call, not a correction.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## master #2858 +/- ##
==================================================
- Coverage 70.8002% 70.7985% -0.0017%
- Complexity 36979 36984 +5
==================================================
Files 1576 1576
Lines 134792 134815 +23
Branches 24954 24956 +2
==================================================
+ Hits 95433 95447 +14
- Misses 30756 30765 +9
Partials 8603 8603
🚀 New features to boost your workflow:
|
✅ All tests passed ✅🏷️ Commit: 21777c6 Learn more about TestLens at testlens.app/docs. |
blackdrag
approved these changes
Sep 2, 2026
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
3 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Secure processing does not bound element depth. The JAXP limit that does, jdk.xml.maxElementDepth, defaults to 0 meaning unlimited, and nothing in groovy-xml set it, so the depth a document could reach was unbounded by default.
The SAX parse itself survives that, because nesting is tracked on the heap rather than the stack. The damage lands on the first consumer to walk the result recursively - Node.text(), XmlNodePrinter, GPathResult.toString(), XmlUtil.serialize - each of which runs a stack frame per level. A 350KB document 50,000 elements deep parsed cleanly and then killed every one of them with a StackOverflowError. That is an Error, so it escapes the catch(Exception) an application would reasonably use to handle a malformed document: the failure arrives somewhere the caller is not defending, well after the parse it would have attributed it to.
The limit is now set to 1000 by default, matching groovy.json's nesting bound, so a document too deep to walk is refused by the parse that reads it. The JDK enforces the bound itself, which puts the check ahead of every consumer at once and reports the offending element with its depth and position rather than unwinding an anonymous stack.
The limit cannot be set on a SAXParserFactory - it is a parser property - so FactorySupport.createSaxParser applies it to the parser it creates, and the four places that built a SAX parser now go through it. The DocumentBuilderFactory route takes it as a factory attribute, which covers DOMBuilder and the DOM paths too.
Nothing is applied when jdk.xml.maxElementDepth is already set: that is the standard knob for this limit, and an explicitly set parser property would otherwise override the value a user chose - including a deliberate 0 to restore unlimited depth. A parser supplied by the caller is left alone, as with the other hardening here.