Fix SAX parser feature URIs and reject DOCTYPE declarations - #16310
Fix SAX parser feature URIs and reject DOCTYPE declarations#16310jamesfredley wants to merge 3 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.
There was a problem hiding this comment.
🟡 Changes recommended
New setXIncludeAware(false) calls should be guarded for parser compatibility, and the updated DOCTYPE tests should more directly validate “reject all DOCTYPE” behavior (including internal subsets) to prevent regressions.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Pull request overview
This PR hardens XML parsing defaults across SpringIOUtils and the HTTP test-client XmlUtils by fixing SAX feature URIs (using the registered http:// identifiers) and enforcing DOCTYPE rejection to reduce XXE/DTD-related attack surface.
Changes:
- Fix SAX feature URI schemes (
https://→http://) so secure parser features are actually applied. - Change defaults to reject DOCTYPE declarations and disable XInclude.
- Update HTTP test-client behavior and docs, and adjust/add tests for DOCTYPE rejection.
File summaries
| File | Description |
|---|---|
| grails-testing-support-http-client/src/test/groovy/org/apache/grails/testing/http/client/utils/XmlUtilsSpec.groovy | Updates XML slurper security tests for DOCTYPE rejection. |
| grails-testing-support-http-client/src/test/groovy/org/apache/grails/testing/http/client/TestHttpResponseSpec.groovy | Updates response XML parsing tests to reflect hardened defaults. |
| grails-testing-support-http-client/src/main/groovy/org/apache/grails/testing/http/client/utils/XmlUtils.groovy | Fixes feature URIs, rejects DOCTYPE, and disables XInclude in the default SAX factory. |
| grails-testing-support-http-client/README.md | Updates documentation to reflect DOCTYPE rejection behavior. |
| grails-gradle/model/src/test/groovy/org/grails/io/support/SpringIOUtilsSpec.groovy | Adds coverage for hardened SpringIOUtils.createXmlSlurper() behavior. |
| grails-gradle/model/src/main/groovy/org/grails/io/support/SpringIOUtils.java | Fixes feature URIs, enforces DOCTYPE rejection, and disables XInclude in SAX factory creation. |
| grails-doc/src/en/guide/testing/integrationTesting.adoc | Updates user docs for secure default XML parsing behavior in HTTP client testing. |
Review details
- Files reviewed: 7/7 changed files
- Comments generated: 5
- Review effort level: Lite
💡 Add a code-review agent skill or configure MCP servers for context-aware, tailored reviews. Learn more in the docs.
|
@copilot re-review please. Follow-up 6fdc508 guards XInclude setXIncludeAware and adds internal-subset DOCTYPE rejection tests. |
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.
jdaugherty
left a comment
jdaugherty
left a comment
There was a problem hiding this comment.
Confirmed the premise before anything else: against the JDK parser all six http:// identifiers are accepted and every https:// form is answered with SAXNotRecognizedException, so the previous values were setting nothing at all. The correction is real, and the strict DOCTYPE default you picked is the right one to ship. Everything below is about keeping it that way.
I've opened jamesfredley#4 against this branch with a suggested implementation of the three points below, so they're easy to take or drop.
Where the https spelling came from. eed8df3594 ("chore: update url to HTTPS", #13478) rewrote them across 79 files in April 2024. That sweep was preparatory work toward adopting io.spring.nohttp — the tool itself has never run over these modules and would not have flagged these: its default allowlist carries ^http://xml\.org/.* and ^http://apache\.org/xml/features/.*, so adopting it needs no exception for them. The hazard was the manual preparation, not the tool. Worth calling out because if we make another run at adopting nohttp the same well-meant rewrite can land again, and the empty catches around setFeature will hide it again exactly as they did for two years. For this reason, I'm suggesting we centralize the config into an enum constant that we document this risk explicitly.
DOCTYPE rejection stops JSP tag library resolution, with nothing configurable to get past it. createParserFactory() returns one cached factory shared with TldReader and WebXmlTagLibraryReader, which read classpath descriptors that legitimately carry a DOCTYPE. GspAutoConfiguration scans classpath*:/META-INF/c-1_0-rt.tld by default, and that file in org.glassfish.web:jakarta.servlet.jsp.jstl:3.0.1 opens with a JSP 1.2 DOCTYPE — eight of the twenty TLDs in that jar have one. TagLibraryResolverImpl.initialize() doesn't catch, so it surfaces from the first resolveTagLibrary(uri) call. No existing test covers it (the GSP tests use a fixture TLD without a DOCTYPE), so CI stays green.
Rather than weaken the default, #4 adds grails.xml.allowDocTypeDeclaration, read through Metadata so it works from application.yml or as a system property, defaulting to off. Applications opt in knowing what they're accepting, and it relaxes only whether a declaration is permitted — external entities and external DTDs stay refused either way. I'm really not sure this is a "true" vulnerability since we use this utility to parse developer code, but the config seems like a sensible default if we're going to continue to support JSP.
The external-entity features end up untested. The other four identifiers can't be reached by any test once DOCTYPE is rejected, and the deleted tests were their only coverage. Given this PR exists because a wrong identifier went unnoticed, worth not leaving those four in the same position.
The identifiers are declared twice, here and in the HTTP test client, as bare literals in both — which is how one rewrite became two. #4 collects them in an enum in grails-gradle-common (same build as SpringIOUtils; grails-common already re-exposes it via api 'org.apache.grails.gradle:grails-gradle-common', so the test client sees it too), with the reason the scheme is fixed written next to the values.
|
|
||
| try { | ||
| saxParserFactory.setFeature("https://apache.org/xml/features/disallow-doctype-decl", false); | ||
| saxParserFactory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); |
There was a problem hiding this comment.
This default is right, but nothing lets an application past it, and one common setup needs to get past it.
This factory is shared with TldReader and WebXmlTagLibraryReader, which read TLDs and web.xml from the classpath. GspAutoConfiguration sets the default grails.gsp.tldScanPattern to include classpath*:/META-INF/c-1_0-rt.tld, and that file in jakarta.servlet.jsp.jstl:3.0.1 begins
<!DOCTYPE taglib
PUBLIC "-//Sun Microsystems, Inc.//DTD JSP Tag Library 1.2//EN"
"http://java.sun.com/dtd/web-jsptaglibrary_1_2.dtd">Parsing it with this feature set gives SAXParseException: DOCTYPE is disallowed when the feature "http://apache.org/xml/features/disallow-doctype-decl" set to true. TagLibraryResolverImpl.initialize() doesn't catch, so it surfaces from the first resolveTagLibrary(uri) call.
A hint from our own code: TldReader falls back to tag.'tagclass'.text(), the JSP 1.1 element name, which only appears in DTD-era TLDs — the reader is written to handle exactly the documents this now rejects.
jamesfredley#4 keeps this true and adds grails.xml.allowDocTypeDeclaration, read via Metadata so it comes from application.yml or a system property, off by default. Opting in doesn't reopen XXE: with declarations permitted, a file:// external entity still resolves to empty rather than the file contents, which the tests there assert.
| saxParserFactory = FactorySupport.createSaxParserFactory(); | ||
| saxParserFactory.setNamespaceAware(true); | ||
| saxParserFactory.setValidating(false); | ||
| try { |
There was a problem hiding this comment.
These five are also declared in XmlUtils in the HTTP test client, as bare literals in both places. That duplication is how a single sweep became two broken call sites.
eed8df3594 (#13478) is where the https spelling came from — a 79-file pass done as preparation for adopting io.spring.nohttp. Since we still intend to adopt it, this can happen again, which is the argument for putting the values somewhere with the explanation attached rather than leaving them as bare literals in two files.
For what it's worth, nohttp itself would not have flagged these: its default allowlist already carries ^http://xml\.org/.* and ^http://apache\.org/xml/features/.*. Adopting the tool needs no exception for them, and its allowlist is a decent reference for which http:// strings here are identifiers rather than links.
jamesfredley#4 collects them in an enum in grails-gradle-common — same build as this module, and already visible to the root build through grails-common.
| then: | ||
| thrown(SAXParseException) | ||
| } | ||
| } |
There was a problem hiding this comment.
These assert DOCTYPE rejection, which leaves external-general-entities, external-parameter-entities, load-dtd-grammar and load-external-dtd with no coverage at all — once DOCTYPE is refused, no document can reach them. The tests removed from XmlUtilsSpec and TestHttpResponseSpec were the only thing pinning them, so the same wrong-identifier mistake in those four lines would still pass CI.
One thing worth knowing when adding coverage: a test that reads the flags back with getFeature(...) has to restate the identifiers, so a sweep rewrites the test and the production code together. It does fail, but at the test, with a name-lookup error whose tempting fix is to make the test tolerate it. Driving a document with an external entity through the parser and asserting the file contents don't appear keeps the assertion free of any identifier.
jamesfredley#4 does that, and adds a spec deriving the list from values() that asserts each identifier is one a parser actually registers — so an unrecognised name fails by name instead of silently. I checked both failure modes against it: rewriting the enum to https fails 4 of 7 behavioural cases plus all 5 of those.
| } | ||
|
|
||
| void 'newXmlSlurper allows inline doctype declarations with internal entities'() { | ||
| void 'newXmlSlurper rejects doctype declarations with external entities'() { |
There was a problem hiding this comment.
Small one: parsed is assigned but the block only asserts thrown(SAXParseException), so the variable is dead. Same in the internal-entity test below.
| } | ||
|
|
||
| void 'xml uses a secure default slurper that does not resolve external entities'() { | ||
| void 'xml rejects doctype declarations with external entities'() { |
There was a problem hiding this comment.
Dropping e.message.contains('External Entity') leaves both tests asserting a bare SAXParseException, which a merely malformed document would also satisfy. Asserting on 'DOCTYPE is disallowed' would keep them pinned to the behaviour they're named for.
Formatting nit: the fixture here indents <!ENTITY and ]> by one space while the internal-entity fixture below doesn't.
| * <p> | ||
| * The default parser is namespace aware, non-validating, permits inline DOCTYPE declarations, | ||
| * and disables external entity expansion plus external DTD loading. | ||
| * The default parser is namespace aware, non-validating, and rejects DOCTYPE declarations. |
There was a problem hiding this comment.
This javadoc drops "disables external entity expansion plus external DTD loading", but those features are still being set. The README and integrationTesting.adoc both kept the sentence, so it's only the javadoc that loses it.
No change needed to the strict behaviour here — a test client refusing DOCTYPE is the right call, and #4 leaves it exactly as you have it.
There was a problem hiding this comment.
🟡 Changes recommended
Mandatory DOCTYPE protections can still fail open when SAX providers reject the feature.
Once you've addressed the issues Copilot identified, you can request another Copilot review.
Review details
Suppressed comments (1)
grails-doc/src/en/guide/testing/integrationTesting.adoc:503
- This documents the new test-response behavior, but the same
SpringIOUtilschange also makes Grailsapplication/xml,text/xml, and HAL XML request binding reject DOCTYPEs. The REST binding documentation remains silent about that new input restriction; add it there (and a migration note if this targets an existing release) so application users are not surprised by rejected request bodies.
rejects `DOCTYPE` declarations, and disables external entity expansion plus external DTD loading.
- Files reviewed: 7/7 changed files
- Comments generated: 2
- Review effort level: Balanced
|
|
||
| try { | ||
| saxParserFactory.setFeature("https://apache.org/xml/features/disallow-doctype-decl", false); | ||
| saxParserFactory.setFeature("http://apache.org/xml/features/disallow-doctype-decl", true); |
|
|
||
| private static final Map<String, Boolean> SECURE_XML_SLURPER_FEATURES = [ | ||
| (DISALLOW_DOCTYPE_DECL): false, | ||
| (DISALLOW_DOCTYPE_DECL): true, |
|
All of my review feedback is implemented in jamesfredley#4 - I made this configurable given that it can break JSPs |
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]
|
|
Review follow-up that was mistakenly opened on the archive fork is now #16328 |
d4c5ac0 to
6fdc508
Compare
|
This PR now includes jdaugherty's review follow-up (previously jamesfredley#4 / #16328). |
|
Superseded by #16331 so the head branch is on apache/grails-core (not the archive fork). |
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