jdaugherty commented on code in PR #16310:
URL: https://github.com/apache/grails-core/pull/16310#discussion_r3930694644
##########
grails-gradle/model/src/main/groovy/org/grails/io/support/SpringIOUtils.java:
##########
@@ -423,19 +423,24 @@ private static SAXParserFactory createParserFactory()
throws ParserConfiguration
saxParserFactory = FactorySupport.createSaxParserFactory();
saxParserFactory.setNamespaceAware(true);
saxParserFactory.setValidating(false);
+ try {
Review 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/grails-core#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`.
##########
grails-testing-support-http-client/src/test/groovy/org/apache/grails/testing/http/client/utils/XmlUtilsSpec.groovy:
##########
@@ -278,37 +276,26 @@ 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'()
{
Review 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.
##########
grails-gradle/model/src/main/groovy/org/grails/io/support/SpringIOUtils.java:
##########
@@ -423,19 +423,24 @@ private static SAXParserFactory createParserFactory()
throws ParserConfiguration
saxParserFactory = FactorySupport.createSaxParserFactory();
saxParserFactory.setNamespaceAware(true);
saxParserFactory.setValidating(false);
+ try {
+ saxParserFactory.setXIncludeAware(false);
+ } catch (UnsupportedOperationException e) {
+ // ignore, parser doesn't support
+ }
try {
-
saxParserFactory.setFeature("https://apache.org/xml/features/disallow-doctype-decl";,
false);
+
saxParserFactory.setFeature("http://apache.org/xml/features/disallow-doctype-decl";,
true);
Review 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
```xml
<!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/grails-core#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.
##########
grails-testing-support-http-client/src/test/groovy/org/apache/grails/testing/http/client/TestHttpResponseSpec.groovy:
##########
@@ -199,36 +198,32 @@ 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'() {
Review 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.
##########
grails-gradle/model/src/test/groovy/org/grails/io/support/SpringIOUtilsSpec.groovy:
##########
@@ -0,0 +1,55 @@
+/*
+ * 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)
+ }
+
+ void 'createXmlSlurper rejects doctype declarations with internal
entities'() {
+ when:
+ SpringIOUtils.createXmlSlurper().parseText('''<!DOCTYPE root [
+<!ENTITY msg "safe">
+]>
+<root>&msg;</root>''')
+
+ then:
+ thrown(SAXParseException)
+ }
+}
Review 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/grails-core#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.
##########
grails-testing-support-http-client/src/main/groovy/org/apache/grails/testing/http/client/utils/XmlUtils.groovy:
##########
@@ -118,8 +118,7 @@ class XmlUtils {
/**
* Creates an {@link XmlSlurper} with secure defaults.
* <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.
Review 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.
--
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]
