Copilot commented on code in PR #16310:
URL: https://github.com/apache/grails-core/pull/16310#discussion_r3928957052
##########
grails-testing-support-http-client/src/main/groovy/org/apache/grails/testing/http/client/utils/XmlUtils.groovy:
##########
@@ -224,6 +223,7 @@ class XmlUtils {
def saxParserFactory = FactorySupport.createSaxParserFactory().tap {
it.namespaceAware = true
it.validating = false
+ it.XIncludeAware = false
}
Review Comment:
Setting `XIncludeAware` can throw `UnsupportedOperationException` on SAX
parser implementations that don't support XInclude, which would currently fail
XML parsing even though the rest of the security features are handled
defensively. Consider wrapping `XIncludeAware = false` in a try/catch like the
feature toggles so unsupported parsers still work (with XInclude simply left at
the default).
##########
grails-gradle/model/src/main/groovy/org/grails/io/support/SpringIOUtils.java:
##########
@@ -423,19 +423,20 @@ private static SAXParserFactory createParserFactory()
throws ParserConfiguration
saxParserFactory = FactorySupport.createSaxParserFactory();
saxParserFactory.setNamespaceAware(true);
saxParserFactory.setValidating(false);
+ saxParserFactory.setXIncludeAware(false);
Review Comment:
`SAXParserFactory#setXIncludeAware(false)` can throw
`UnsupportedOperationException` if XInclude isn't supported by the underlying
parser implementation. Since `createParserFactory()` currently doesn't guard
this call, XML parsing can fail entirely on such implementations; wrapping this
in a try/catch (similar to the feature setters) preserves compatibility while
still disabling XInclude where supported.
##########
grails-gradle/model/src/test/groovy/org/grails/io/support/SpringIOUtilsSpec.groovy:
##########
@@ -0,0 +1,44 @@
+/*
+ * Licensed to the Apache Software Foundation (ASF) under one
+ * or more contributor license agreements. See the NOTICE file
+ * distributed with this work for additional information
+ * regarding copyright ownership. The ASF licenses this file
+ * to you under the Apache License, Version 2.0 (the
+ * "License"); you may not use this file except in compliance
+ * with the License. You may obtain a copy of the License at
+ *
+ * https://www.apache.org/licenses/LICENSE-2.0
+ *
+ * Unless required by applicable law or agreed to in writing,
+ * software distributed under the License is distributed on an
+ * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+ * KIND, either express or implied. See the License for the
+ * specific language governing permissions and limitations
+ * under the License.
+ */
+package org.grails.io.support
+
+import org.xml.sax.SAXParseException
+import spock.lang.Specification
+
+class SpringIOUtilsSpec extends Specification {
+
+ void 'createXmlSlurper parses documents without a doctype'() {
+ when:
+ def xml =
SpringIOUtils.createXmlSlurper().parseText('<root><child>ok</child></root>')
+
+ then:
+ xml.child.text() == 'ok'
+ }
+
+ void 'createXmlSlurper rejects doctype declarations with external
entities'() {
+ when:
+ SpringIOUtils.createXmlSlurper().parseText('''<!DOCTYPE root [
+<!ENTITY ext SYSTEM "file:///not-resolved">
+]>
+<root>&ext;</root>''')
+
+ then:
+ thrown(SAXParseException)
+ }
Review Comment:
The new test only exercises a DOCTYPE that defines an external entity;
depending on parser behavior, that can fail even if DOCTYPE declarations are
still allowed. Adding an internal-subset DOCTYPE case helps ensure the intended
behavior (“reject DOCTYPE declarations”) is actually enforced by
`createXmlSlurper()`.
##########
grails-testing-support-http-client/src/test/groovy/org/apache/grails/testing/http/client/utils/XmlUtilsSpec.groovy:
##########
@@ -278,37 +276,15 @@ class XmlUtilsSpec extends Specification {
xml == "<?xml version='1.1' encoding='UTF-16'?><product />"
}
- void 'newXmlSlurper allows inline doctype declarations with internal
entities'() {
+ void 'newXmlSlurper rejects doctype declarations with external entities'()
{
when:
def parsed = XmlUtils.newXmlSlurper().parseText('''<!DOCTYPE root [
-<!ENTITY msg "safe">
-]>
-<root>&msg;</root>''')
+ <!ENTITY ext SYSTEM "file:///not-resolved">
+ ]>
Review Comment:
The updated DOCTYPE test only uses an external entity, which can still throw
for multiple reasons even if DOCTYPE declarations are not actually rejected
(e.g., unresolved entity handling). Adding a second test case with an internal
entity and asserting a parse failure more directly validates the intended
behavior change (“reject DOCTYPE declarations”) and guards against regressions.
##########
grails-testing-support-http-client/src/test/groovy/org/apache/grails/testing/http/client/TestHttpResponseSpec.groovy:
##########
@@ -199,36 +198,18 @@ class TestHttpResponseSpec extends Specification {
xmlResponse.xml().item.text() == 'value'
}
- void 'xml uses a secure default slurper that does not resolve external
entities'() {
+ void 'xml rejects doctype declarations with external entities'() {
given:
- def secretFile = Files.createTempFile('test-http-response-xml', '.txt')
- Files.writeString(secretFile, 'top-secret-token')
- def uri = secretFile.toUri().toASCIIString()
- def response = mockResponse(200, """<!DOCTYPE root [
-<!ENTITY ext SYSTEM '${uri}'>
-]>
-<root>&ext;</root>""")
+ def response = mockResponse(200, '''<!DOCTYPE root [
+ <!ENTITY ext SYSTEM "file:///not-resolved">
+ ]>
Review Comment:
This spec previously verified that the response XML slurper still allowed
internal DOCTYPEs; after the behavioral change to reject DOCTYPE declarations,
adding an explicit internal-subset DOCTYPE rejection test would better validate
the new default behavior (and is less dependent on external-entity resolution
mechanics).
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: [email protected]
For queries about this service, please contact Infrastructure at:
[email protected]
