Fix SAX parser feature URIs and reject DOCTYPE declarations - #16331
Fix SAX parser feature URIs and reject DOCTYPE declarations#16331jamesfredley wants to merge 6 commits into
Conversation
The SAX XXE hardening used https://xml.org and https://apache.org feature identifiers. Xerces only recognizes the http:// forms, so the empty catch swallowed the misconfiguration. XmlDataBindingSourceCreator and the HTTP test client both now set the registered http:// identifiers, disable XInclude, and reject DOCTYPE declarations.
The SAX feature identifiers were declared twice, once in SpringIOUtils and once in the HTTP test client, as bare string literals. That duplication is how the https:// spelling was introduced and went unnoticed: setFeature answers an unrecognised name with SAXNotRecognizedException, and both call sites swallow it, so the hardening silently switched off. Collect the five identifiers in XmlParserFeature in grails-gradle-common, which sits in the grails-gradle build alongside SpringIOUtils and is already exposed to the root build by grails-common. The enum documents that the values are registered identifiers rather than addresses, and carries the reason the http scheme cannot be rewritten. Keep rejecting DOCTYPE declarations by default, and add grails.xml.allowDocTypeDeclaration for applications that must accept them. SpringIOUtils reads it through Metadata, so it is set in application.yml or as a system property. Opting in relaxes only whether a declaration is permitted; external general entities, external parameter entities and external DTDs stay refused either way, so it does not reopen the XXE vector. The setting is needed because this parser factory is shared with readers of trusted classpath descriptors -- TldReader, WebXmlTagLibraryReader and PluginUtils -- and TLDs routinely carry a DOCTYPE. jakarta.servlet.jsp.jstl ships eight, including c-1_0-rt.tld, which the default grails.gsp.tldScanPattern scans, so an application resolving JSP tag libraries from a GSP needs it enabled. Document that on the JSP tag library page and in the upgrade notes. Cover both modules with tests that assert observable behaviour rather than reading feature flags back, so no test holds a second copy of the identifiers that a rewrite could update in step with the production code. XmlParserFeatureSpec additionally asserts every identifier is one a parser actually registers, turning an unrecognised name into a named failure instead of a silent no-op.
|
from @matrei AI Review FindingsHead There is one blocking problem with the default this PR ships, and it is not covered by any test in the repository. The follow-up in jamesfredley#4 addresses it with a global opt-in; I would take that PR with one change in shape (see the review on jamesfredley#4). [P1] Rejecting DOCTYPE in the shared factory breaks JSP tag library resolution for every application that uses JSTLFiles:
The descriptors are where the strict default bites. I confirmed this with a spec in The documented custom pattern The user-visible symptom is the worst kind: a 500 on the first page that uses a JSP tag, with a parser error that names a feature URI nobody set in the application. What I would change. The two trust levels want two parsers, and the readers of trusted descriptors know they are reading trusted descriptors. Keep the strict default for request bodies and let the descriptor readers ask for DOCTYPE tolerance explicitly: // SpringIOUtils
public static XmlSlurper createXmlSlurper() throws ... { // strict, for request bodies
return createXmlSlurper(false);
}
public static XmlSlurper createXmlSlurper(boolean allowDocTypeDeclaration) throws ... {
return new XmlSlurper(createParserFactory(allowDocTypeDeclaration).newSAXParser());
}with jamesfredley#4 instead adds [P2] A rejected hardening feature is still swallowed silentlyFiles:
The PR exists because jamesfredley#4 adds [P2] The four external-entity features lose their only behavioural coverageFiles:
Every new test asserts DOCTYPE rejection, and once a DOCTYPE is refused no document reaches [P2]
|
There was a problem hiding this comment.
Warning
Copilot couldn't run its full agentic review because it didn't start before the timeout. Make sure your repository has a runner available, or add a copilot-code-review.yml file specifying one with the runs-on attribute. See the docs for more details.
Pull request overview
Hardens XML parsing across Grails utilities by fixing SAX feature URIs, rejecting DOCTYPE declarations by default, and aligning HTTP test-client parsing behavior with the hardened defaults.
Changes:
- Replace incorrect
https://...SAX/Xerces feature identifiers with registeredhttp://...URIs via a sharedXmlParserFeatureenum. - Default to rejecting
DOCTYPEdeclarations and disable XInclude in parser factories / XML slurpers. - Add/adjust specs and docs to validate and explain DOCTYPE rejection and the opt-in override.
Reviewed changes
Copilot reviewed 13 out of 13 changed files in this pull request and generated 3 comments.
Show a summary per file
| File | Description |
|---|---|
| grails-testing-support-http-client/src/main/groovy/org/apache/grails/testing/http/client/utils/XmlUtils.groovy | Uses shared feature constants, flips DOCTYPE handling default to “reject”, disables XInclude. |
| grails-testing-support-http-client/src/test/groovy/org/apache/grails/testing/http/client/utils/XmlUtilsSpec.groovy | Updates expectations to assert DOCTYPE rejection. |
| grails-testing-support-http-client/src/test/groovy/org/apache/grails/testing/http/client/TestHttpResponseSpec.groovy | Updates response XML parsing tests to assert DOCTYPE rejection. |
| grails-testing-support-http-client/build.gradle | Adds dependency used to share XML feature identifiers. |
| grails-testing-support-http-client/README.md | Updates documentation to reflect DOCTYPE rejection default. |
| grails-gradle/model/src/main/groovy/org/grails/io/support/SpringIOUtils.java | Fixes feature URIs, adds opt-in key for DOCTYPE, disables XInclude, refactors factory creation. |
| grails-gradle/model/src/test/groovy/org/grails/io/support/SpringIOUtilsSpec.groovy | Adds behavioral tests for DOCTYPE rejection and opt-in, plus XXE safeguards. |
| grails-gradle/model/build.gradle | Adds shared module dependency for feature enum. |
| grails-gradle/common/src/main/groovy/org/apache/grails/gradle/common/XmlParserFeature.java | Introduces centralized, documented feature identifiers. |
| grails-gradle/common/src/test/groovy/org/apache/grails/gradle/common/XmlParserFeatureSpec.groovy | Ensures feature identifiers are recognized by the parser and distinct. |
| grails-doc/src/en/guide/upgrading.adoc | Documents new DOCTYPE-rejection default and the opt-in setting. |
| grails-doc/src/en/guide/theWebLayer/gsp/taglibs/usingJSPTagLibraries.adoc | Documents enabling DOCTYPE for JSP taglib scanning. |
| grails-doc/src/en/guide/testing/integrationTesting.adoc | Updates test-client XML parsing docs to reflect DOCTYPE rejection. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Codecov Report❌ Patch coverage is Additional details and impacted files@@ Coverage Diff @@
## 8.0.x #16331 +/- ##
==================================================
+ Coverage 55.0810% 55.1123% +0.0314%
- Complexity 20804 20826 +22
==================================================
Files 2111 2112 +1
Lines 101378 101402 +24
Branches 18005 18010 +5
==================================================
+ Hits 55840 55885 +45
+ Misses 37490 37471 -19
+ Partials 8048 8046 -2
🚀 New features to boost your workflow:
|
AI Review Findings (round 3)Head The three replies on the Copilot threads are fine as answers to Copilot. None of the findings from the two earlier rounds has a reply or a code change, so they are listed again below with their current status. [P1] Still open: the opt-in is global, so using JSTL also relaxes request-body parsingFiles:
Unchanged from round 2. The default The ask is the same: give the three trusted-descriptor readers a If the maintainers decide the global switch is the intended shape, that is a legitimate call, but it should be said on the PR so the next reviewer does not re-raise it, and the JSP doc paragraph then needs to say plainly that the setting also affects request bodies. [P2] Still open: the setting is resolved through
|
|
I think this is the best solution for the DOCTYPE reading: What I would change. The two trust levels want two parsers, and the readers of trusted descriptors know they are reading trusted descriptors. Keep the strict default for request bodies and let the descriptor readers ask for DOCTYPE tolerance explicitly: // SpringIOUtils
public static XmlSlurper createXmlSlurper() throws ... { // strict, for request bodies
return createXmlSlurper(false);
}
public static XmlSlurper createXmlSlurper(boolean allowDocTypeDeclaration) throws ... {
return new XmlSlurper(createParserFactory(allowDocTypeDeclaration).newSAXParser());
}with TldReader, WebXmlTagLibraryReader and PluginUtils passing true. External general and parameter entities, DTD grammar loading and external DTD retrieval stay off on both factories, so the tolerant parser still resolves a file:// entity to nothing and skips the web-jsptaglibrary_1_2.dtd reference instead of fetching it. That needs no configuration key, no upgrade note for JSP users, and does not relax request-body parsing as a side effect of using JSTL. We could optionally check the config value and pass that in the no-param method. |
…E-tolerant for classpath descriptors
|
@matrei I believe I implemented your solution, and all feedback on this PR. Please re-review. |
🚨 TestLens detected 1 failed test 🚨Here is what you can do:
Test SummaryCI / Functional Tests (Java 21, indy=true) > :grails-test-examples-scaffolding:integrationTest
🏷️ Commit: e10b69c Test FailuresUserControllerSpec > User list (:grails-test-examples-scaffolding:integrationTest in CI / Functional Tests (Java 21, indy=true))Rerun ControlsSelect tests to mute in this pull request:
Reuse successful test results:
Click the checkbox to trigger a rerun:
Learn more about TestLens at testlens.app/docs. |
matrei
left a comment
matrei
left a comment
There was a problem hiding this comment.
Review Findings (round 4)
Head e10b69c4be on 8.0.x base e9cd00567b. One new commit on top of the round-3 head d88d15ea15 plus a merge of 8.0.x. It is the two-parser shape from round 2, applied in full. I re-ran everything on this head: :grails-gradle-common:test (12), :grails-gradle-model:test (61), :grails-web-databinding:test for the new binding spec (4), :grails-core:test for PluginDiscoverySpec (19), :grails-test-suite-uber:test for GrailsMockHttpServletResponseTests (3), :grails-gsp:test for the org.grails.gsp.jsp package (33, including the JSTL spec), :grails-testing-support-http-client:test (105), all green, and codeStyle on the seven touched modules, clean.
Resolved since round 3
Every open item from rounds 1 to 3 has a code or doc change on this head.
- [P1] Global opt-in. Gone.
SpringIOUtilsnow hands out two cached factories,createXmlSlurper(boolean)andnewSAXParser(boolean)select between them, the no-arg forms stay strict, andTldReader,WebXmlTagLibraryReaderandPluginUtilspasstrue.grails.xml.allowDocTypeDeclarationno longer exists anywhere in the tree,Metadatais untouched, and the JSP page carries one sentence instead of a configuration paragraph.XmlDataBindingSourceCreator,HalXmlDataBindingSourceCreatorandXML.parsestill call the no-arg form, so request bodies are strict with no switch. - [P2]
Metadataresolution. Moot; the flag is gone and nothing is resolved per parse any more. - [P2] Regression path untested.
JstlDocTypeTldSpecscansc.tldandc-1_0-rt.tldthroughTagLibraryResolverImpland resolves all eight JSP 1.2 JSTL URIs;TldReaderTestsandWebXmlTagLibraryReaderTestscover DOCTYPE plus the external-entity case at the reader level;PluginDiscoverySpecreads agrails-plugin.xmlwhose DOCTYPE names a missing DTD. - [P2] Rejected feature leaves no trace.
setFeatureinSpringIOUtilsand the loop inXmlUtilsnow catchParserConfigurationException | SAXExceptiononly and log a warning naming the factory class and the feature. The logger is looked up per call inSpringIOUtilswith a comment explaining why, which is the right call for a class used before logging is configured. - [P2]
THREAT_MODEL.mdcontradicts the code. §9 line removed, P10 added to §8 with the documented provenance, Q13 marked resolved, a false-friend entry scopes the claim to framework-parsed documents,threat-model.yamlmirrors all of it. - [P3] Small items.
XmlUtilsjavadoc, the unusedparsedlocals, the barethrown(SAXParseException)assertions, the fixture indentation and theupgrading80x.adocpointer are all fixed. The two DOCTYPE tests in the test client andXmlUtilsSpecnow assertDOCTYPE is disallowed. - [P3]
response.xmlin controller unit tests. Now tolerant, with a DOCTYPE case and an external-entity case inGrailsMockHttpServletResponseTests. - Copilot: unsafe publication. Both factory fields are
volatilenow.
[P3] New: the invalidRequestBody claim covers single-object binding only
Files:
grails-doc/src/en/guide/upgrading.adoc:40grails-doc/src/en/guide/upgrading/upgrading80x.adoc:3588-3589grails-web-databinding/src/main/groovy/org/grails/web/databinding/bindingsource/AbstractRequestBodyDataBindingSourceCreator.groovy:77-100grails-web-databinding/src/main/groovy/grails/web/databinding/DataBindingUtils.java:507-511
Both pages say a body that declares a DOCTYPE "fails with the invalidRequestBody error code and the target is left unpopulated". That is true for bindData and command-object binding, where createBindingSourceCreationException wraps the SAXParseException in InvalidRequestBodyException and DataBindingUtils.bindObjectToInstance turns it into the error code. It is not true for the collection path: createCollectionDataBindingSource in the abstract creator wraps every exception in a plain DataBindingSourceCreationException, and bindToCollection does not catch it, so a collection body carrying a DOCTYPE propagates as an exception. The new XmlDataBindingSourceCreatorSpec encodes exactly that difference in its third feature.
The asymmetry predates this PR; a malformed collection body behaved the same way. But the PR introduces a new input that hits it, and the upgrade note is where a reader will look. One clause saying that collection binding surfaces the failure as a DataBindingSourceCreationException rather than a binding error closes this. Routing the collection path through createBindingSourceCreationException would be the better fix but changes behaviour for JSON too, so it belongs in a follow-up.
[P3] New: XML.parse is named in the docs and P10 but has no direct test
Files:
grails-converters/src/main/groovy/grails/converters/XML.java:62,311,329
XML.parse(String) and XML.parse(HttpServletRequest) statically import the no-arg createXmlSlurper, so they are strict and SpringIOUtilsSpec covers the behaviour transitively. Both doc pages and the threat model name XML.parse explicitly though, and request.XML goes through it. A one-case spec in grails-converters that calls XML.parse on a DOCTYPE string and asserts DOCTYPE is disallowed pins the documented claim to the class that makes it. Optional.
Note: the compile-time plugin descriptor reader stays strict
GlobalGrailsClassInjectorTransformation.groovy:654 reads the plugin's own plugin.xml at compile time through the no-arg IOUtils.createXmlSlurper(), while PluginUtils now reads the same descriptor type at runtime with the tolerant parser. Grails generates plugin.xml itself and never writes a DOCTYPE, so nothing breaks and I am not asking for a change. Mentioning it only so the next reader does not take the PluginUtils comment as a statement that the descriptor is read tolerantly everywhere.
Verified
grails-gradle::grails-gradle-common:test12 cases,:grails-gradle-model:test61 cases (56 before this commit), bothcodeStyletasks, all green one10b69c4be.- Root:
XmlDataBindingSourceCreatorSpec4,PluginDiscoverySpec19,GrailsMockHttpServletResponseTests3,org.grails.gsp.jsp.*33,:grails-testing-support-http-client:test105, all green.codeStyleclean ongrails-web-databinding,grails-core,grails-gsp,grails-testing-support-http-clientandgrails-test-core. grep -rn allowDocTypeDeclarationover source, docs and yaml finds only the new method parameter inSpringIOUtils;git diff e9cd00567b HEAD -- grails-gradle/model/src/main/groovy/grails/util/Metadata.groovyis empty.- Every production caller of
createXmlSlurper/newSAXParserwas checked: the three descriptor readers andresponse.xmlpasstrue; the two binding-source creators,XML.parseand the compile-timeplugin.xmlreader use the strict default. grails-gradle-commonhas onlycompileOnlydependencies, so the newimplementationedge fromgrails-gradle-modeland the test client brings no Gradle API onto the runtime classpath, as the reply to Copilot says.- The
testlensfailure on the PR isUserControllerSpec > User list, a Geb login timeout unrelated to any file this PR touches.
Summary
ASF security review finding f002: SAX XXE hardening in
SpringIOUtils.createParserFactoryset parser features withhttps://xml.org/...andhttps://apache.org/.... The registered JAXP / Xerces identifiers are thehttp://forms, so the factory rejected the features and the empty catch hid the failure.XmlDataBindingSourceCreatorstill usesSpringIOUtils.createXmlSlurper()forapplication/xmlrequest bodies. The HTTP test-clientXmlUtilshad the same identifiers and leftdisallow-doctype-declfalse.This is hardening, not a HIGH CVE against the current threat model (THREAT_MODEL.md §9 disclaims parser configuration / XXE). No threat-model change in this PR.
Changes
http://xml.org/...andhttp://apache.org/...SAX feature URIs.disallow-doctype-decltotrueand disable XInclude.SpringIOUtilsSpec,XmlUtilsSpec, andTestHttpResponseSpec.Testing
:grails-gradle:grails-gradle-model:test:grails-testing-support-http-client:test:grails-gradle:grails-gradle-model:codeStyle:grails-testing-support-http-client:codeStyle