Share SAX feature identifiers and make DOCTYPE rejection configurable - #16328
Merged
jamesfredley merged 1 commit intoSep 9, 2026
Merged
Conversation
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.
This was referenced Sep 9, 2026
Merged
Contributor
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
Centralizes SAX/Xerces feature identifiers to prevent accidental rewrites (e.g., http→https), makes DOCTYPE rejection configurable while keeping secure defaults, and adds behavior-driven tests + documentation for the XML hardening.
Changes:
- Introduce
XmlParserFeatureenum (and spec) to share feature identifiers across modules. - Update XML hardening in
SpringIOUtilsto support configurable DOCTYPE declarations with two cached factories. - Add behavioral tests and documentation for the new DOCTYPE configuration and security guarantees.
Reviewed changes
Copilot reviewed 9 out of 9 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 | Replaces duplicated string literals with shared XmlParserFeature identifiers. |
| grails-testing-support-http-client/build.gradle | Adds dependency on grails-gradle-common to access XmlParserFeature. |
| grails-gradle/model/src/test/groovy/org/grails/io/support/SpringIOUtilsSpec.groovy | Switches to behavioral XML hardening tests and adds configuration coverage. |
| grails-gradle/model/src/main/groovy/org/grails/io/support/SpringIOUtils.java | Adds configurable DOCTYPE handling and consolidates feature setup via XmlParserFeature. |
| grails-gradle/model/build.gradle | Adds project dependency on :grails-gradle-common. |
| grails-gradle/common/src/test/groovy/org/apache/grails/gradle/common/XmlParserFeatureSpec.groovy | Adds tests ensuring feature identifiers are recognized and distinct. |
| grails-gradle/common/src/main/groovy/org/apache/grails/gradle/common/XmlParserFeature.java | Introduces shared, documented feature identifiers. |
| grails-doc/src/en/guide/upgrading.adoc | Documents new XML parsing defaults and opt-in DOCTYPE setting. |
| grails-doc/src/en/guide/theWebLayer/gsp/taglibs/usingJSPTagLibraries.adoc | Documents the configuration needed for JSP TLD scanning. |
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
Comment on lines
+38
to
+41
| SAXParserFactory factory = SAXParserFactory.newInstance() | ||
|
|
||
| when: | ||
| factory.setFeature(feature.featureName, false) |
Comment on lines
+454
to
+469
| private static SAXParserFactory strictParserFactory = null; | ||
|
|
||
| private static SAXParserFactory docTypeParserFactory = null; | ||
|
|
||
| private static SAXParserFactory createParserFactory() throws ParserConfigurationException { | ||
| if (saxParserFactory == null) { | ||
| saxParserFactory = FactorySupport.createSaxParserFactory(); | ||
| saxParserFactory.setNamespaceAware(true); | ||
| saxParserFactory.setValidating(false); | ||
| try { | ||
| saxParserFactory.setXIncludeAware(false); | ||
| } catch (UnsupportedOperationException e) { | ||
| // ignore, parser doesn't support | ||
| if (isDocTypeDeclarationAllowed()) { | ||
| if (docTypeParserFactory == null) { | ||
| docTypeParserFactory = buildParserFactory(true); | ||
| } | ||
| return docTypeParserFactory; | ||
| } | ||
| if (strictParserFactory == null) { | ||
| strictParserFactory = buildParserFactory(false); | ||
| } | ||
| return strictParserFactory; | ||
| } |
|
|
||
| implementation platform(project(':grails-bom')) | ||
| implementation project(':grails-testing-support-core') | ||
| implementation 'org.apache.grails.gradle:grails-gradle-common' // XmlParserFeature |
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
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.
Suggested follow-up to #16310. Keeps the feature-URI correction and the strict DOCTYPE default from that PR, and addresses three things found while reviewing it.
Why the identifiers got rewritten
eed8df3594("chore: update url to HTTPS", #13478) rewrote them across 79 files in April 2024:These are registered identifiers matched by exact string comparison, not addresses — nothing is fetched from them. No parser recognises the
httpsspelling, sosetFeatureanswers withSAXNotRecognizedException, and because each call is wrapped in a catch that tolerates parsers lacking a feature, the hardening switched off silently and stayed off for two years.Changes
Collect the identifiers once. They were declared twice, in
SpringIOUtilsand in the HTTP test client'sXmlUtils, as bare literals in both — which is how one rewrite became two.XmlParserFeatureingrails-gradle-commonholds them, and carries the reason the scheme cannot be changed. That module is in thegrails-gradlebuild alongsideSpringIOUtils, andgrails-commonalready re-exposes it (api 'org.apache.grails.gradle:grails-gradle-common'), so the root build and the test client both see it.Make DOCTYPE rejection configurable, still off by default.
SpringIOUtils.createParserFactory()returns one cached factory shared with readers of trusted classpath descriptors —TldReader,WebXmlTagLibraryReader,PluginUtils— and TLDs routinely carry a DOCTYPE.jakarta.servlet.jsp.jstl:3.0.1ships eight, includingc-1_0-rt.tld, which the defaultgrails.gsp.tldScanPatterninGspAutoConfigurationscans.TagLibraryResolverImpl.initialize()does not catch, so rejecting DOCTYPE stops JSP tag library resolution from the firstresolveTagLibrary(uri)call.The default stays strict. Applications that need declarations opt in:
Read through
Metadata, so it works fromapplication.ymlor 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, which the tests assert. Documented in the upgrade notes and on the JSP tag library page.Test the hardening by behaviour, not by reading flags back. A test that asserts
getFeature(...)has to restate the identifiers, so the same sweep rewrites the test and the production code together; it fails, but at the test, with a name-lookup error whose tempting fix is to make the test tolerate it. Driving real documents through the parser keeps the assertions free of any identifier.XmlParserFeatureSpecadditionally derives the list fromvalues()and asserts each is one a parser actually registers, so an unrecognised name fails by name instead of silently.Verified both failure modes: re-enabling DOCTYPE rejection without the opt-in fails 5 of 7
SpringIOUtilsSpeccases including the descriptor test, and rewriting the enum tohttpsfails 4 of 7 plus all 5XmlParserFeatureSpeccases.On adopting nohttp
Not enabled here, but worth calling out, because it is directly connected and could break this again.
The sweep in #13478 was preparatory work toward adopting
io.spring.nohttp. The tool itself has never run over these modules, and would not have flagged these values: running its matcher with the default allowlist reports all five as allowed, because that allowlist carries^http://xml\.org/.*and^http://apache\.org/xml/features/.*. Spring hit the same problem and allowlisted it upstream.So the tool is not the hazard — the manual preparation for it was. Adopting nohttp needs no exception for these five, and its allowlist is a useful reference for which
http://strings in the codebase are identifiers rather than links. But if we make another run at adopting it, the same well-meant rewrite can land again, and the empty catches aroundsetFeaturewill hide it again exactly as they did for the last two years.That is what the comment on
XmlParserFeatureand the behavioural tests are for: the comment tells the next person why the scheme is fixed, and the tests fail loudly if it changes anyway rather than leaving the hardening quietly off.Testing
:grails-gradle-model:test,:grails-gradle-common:test,:grails-testing-support-http-client:testSpringIOUtils::grails-web-databinding:test,:grails-converters:test,:grails-test-core:test,:grails-core:test,:grails-web-jsp:test,:grails-gsp:testcodeStyleon each changed module