Copilot commented on code in PR #16328:
URL: https://github.com/apache/grails-core/pull/16328#discussion_r3969375461
##########
grails-gradle/model/src/main/groovy/org/grails/io/support/SpringIOUtils.java:
##########
@@ -416,50 +419,86 @@ public static SAXParser newSAXParser() throws
ParserConfigurationException, SAXE
return factory.newSAXParser();
}
- private static SAXParserFactory saxParserFactory = null;
+ /**
+ * Configuration key permitting {@code DOCTYPE} declarations in documents
parsed by this class.
+ *
+ * <p>Parsers handed out here reject a {@code DOCTYPE} by default. Set
+ * {@code grails.xml.allowDocTypeDeclaration} to {@code true} in {@code
application.yml}, or as
+ * a system property, to accept one.
+ *
+ * <p>Opting in does not reopen the XXE vector. External general entities,
external parameter
+ * entities and external DTDs stay refused whichever way this is set, so
an entity pointing at
+ * a file on disk still contributes nothing. What opting in changes is
only whether a document
+ * carrying a declaration is refused outright.
+ *
+ * <p>It exists because these parsers also read trusted descriptors from
the classpath, and
+ * some of those carry a {@code DOCTYPE}. JSP tag library descriptors are
the common case:
+ * {@code jakarta.servlet.jsp.jstl} ships several, among them {@code
c-1_0-rt.tld}, which the
+ * default {@code grails.gsp.tldScanPattern} scans.
+ */
+ public static final String ALLOW_DOCTYPE_DECLARATION =
"grails.xml.allowDocTypeDeclaration";
+
+ /**
+ * Parser features switched off for every parser this class hands out.
+ *
+ * <p>{@link XmlParserFeature#DISALLOW_DOCTYPE_DECL} is handled separately
because it is the
+ * one feature an application may turn off; see {@link
#ALLOW_DOCTYPE_DECLARATION}.
+ */
+ private static final XmlParserFeature[] DISABLED_PARSER_FEATURES = {
+ XmlParserFeature.EXTERNAL_GENERAL_ENTITIES,
+ XmlParserFeature.EXTERNAL_PARAMETER_ENTITIES,
+ XmlParserFeature.LOAD_DTD_GRAMMAR,
+ XmlParserFeature.LOAD_EXTERNAL_DTD
+ };
+
+ 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;
+ }
Review Comment:
The lazy initialization of the cached `SAXParserFactory` instances is not
thread-safe (no synchronization/`volatile`). Under concurrent access, multiple
factories can be created and there’s a risk of visibility issues; consider
using `volatile` + synchronized initialization, or an initialization-on-demand
holder to make the caches safely published.
##########
grails-testing-support-http-client/build.gradle:
##########
@@ -44,6 +44,7 @@ dependencies {
implementation platform(project(':grails-bom'))
implementation project(':grails-testing-support-core')
+ implementation 'org.apache.grails.gradle:grails-gradle-common' //
XmlParserFeature
Review Comment:
This dependency is declared without a version, so it relies on dependency
constraints (e.g., a BOM or constraints block) to supply one. If it isn’t
constrained, Gradle will fail resolution; even if it is, using a local project
dependency (like `:grails-gradle-common` used elsewhere in this PR) avoids
repository lookups/mismatch risk and keeps builds reproducible.
##########
grails-gradle/common/src/test/groovy/org/apache/grails/gradle/common/XmlParserFeatureSpec.groovy:
##########
@@ -0,0 +1,54 @@
+/*
+ * 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.apache.grails.gradle.common
+
+import javax.xml.parsers.SAXParserFactory
+
+import spock.lang.Specification
+import spock.lang.Unroll
+
+class XmlParserFeatureSpec extends Specification {
+
+ /**
+ * Every identifier must be one the parser actually registers.
+ *
+ * <p>Callers set these inside a catch that tolerates a parser lacking a
feature, so an
+ * unrecognised identifier disables hardening silently instead of failing.
Rewriting the
+ * {@code http} scheme to {@code https} is the way that happens in
practice. This spec derives
+ * the identifiers from {@link XmlParserFeature#values()} rather than
restating them, so the
+ * same rewrite cannot pass by changing the expectation to match.
+ */
+ @Unroll
+ void 'feature #feature is recognised by the parser'() {
+ given:
+ SAXParserFactory factory = SAXParserFactory.newInstance()
+
+ when:
+ factory.setFeature(feature.featureName, false)
Review Comment:
This spec uses `SAXParserFactory.newInstance()` directly, but production
code here uses `FactorySupport.createSaxParserFactory()` (in `SpringIOUtils`).
If those resolve to different JAXP providers in some environments, the test can
fail (or pass) without reflecting the real factory used by Grails; consider
constructing the factory via the same mechanism as production to keep the
assertion aligned.
--
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]
