Copilot commented on code in PR #16331:
URL: https://github.com/apache/grails-core/pull/16331#discussion_r3969784743
##########
grails-gradle/model/src/main/groovy/org/grails/io/support/SpringIOUtils.java:
##########
@@ -416,45 +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";
- private static SAXParserFactory createParserFactory() throws
ParserConfigurationException {
- if (saxParserFactory == null) {
- saxParserFactory = FactorySupport.createSaxParserFactory();
- saxParserFactory.setNamespaceAware(true);
- saxParserFactory.setValidating(false);
+ /**
+ * 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
+ };
- try {
-
saxParserFactory.setFeature("https://apache.org/xml/features/disallow-doctype-decl";,
false);
- } catch (Exception pce) {
- // ignore, parser doesn't support
- }
- try {
-
saxParserFactory.setFeature("https://xml.org/sax/features/external-general-entities";,
false);
- } catch (Exception pce) {
- // ignore, parser doesn't support
- }
- try {
-
saxParserFactory.setFeature("https://xml.org/sax/features/external-parameter-entities";,
false);
- } catch (Exception pce) {
- // ignore, parser doesn't support
- }
- try {
-
saxParserFactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
- } catch (Exception e) {
- // ignore, parser doesn't support
- }
- try {
-
saxParserFactory.setFeature("https://apache.org/xml/features/nonvalidating/load-dtd-grammar";,
false);
- } catch (Exception e) {
- // ignore, parser doesn't support
+ private static SAXParserFactory strictParserFactory = null;
+
+ private static SAXParserFactory docTypeParserFactory = null;
+
+ private static SAXParserFactory createParserFactory() throws
ParserConfigurationException {
+ if (isDocTypeDeclarationAllowed()) {
+ if (docTypeParserFactory == null) {
+ docTypeParserFactory = buildParserFactory(true);
}
+ return docTypeParserFactory;
+ }
+ if (strictParserFactory == null) {
+ strictParserFactory = buildParserFactory(false);
+ }
+ return strictParserFactory;
+ }
+
+ private static boolean isDocTypeDeclarationAllowed() {
+ return Boolean.TRUE.equals(
+ Metadata.getCurrent().getProperty(ALLOW_DOCTYPE_DECLARATION,
Boolean.class, Boolean.FALSE));
+ }
+
+ private static SAXParserFactory buildParserFactory(boolean
allowDocTypeDeclaration) throws ParserConfigurationException {
+ SAXParserFactory factory = FactorySupport.createSaxParserFactory();
+ factory.setNamespaceAware(true);
+ factory.setValidating(false);
+ try {
+ factory.setXIncludeAware(false);
+ } catch (UnsupportedOperationException e) {
+ // ignore, parser doesn't support
+ }
+ try {
+ factory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
+ } catch (Exception e) {
+ // ignore, parser doesn't support
+ }
+ try {
+
factory.setFeature(XmlParserFeature.DISALLOW_DOCTYPE_DECL.getFeatureName(),
!allowDocTypeDeclaration);
+ } catch (Exception e) {
+ // ignore, parser doesn't support
+ }
+ for (XmlParserFeature feature : DISABLED_PARSER_FEATURES) {
try {
-
saxParserFactory.setFeature("https://apache.org/xml/features/nonvalidating/load-external-dtd";,
false);
+ factory.setFeature(feature.getFeatureName(), false);
} catch (Exception e) {
// ignore, parser doesn't support
}
}
Review Comment:
These `catch (Exception) { /* ignore */ }` blocks can silently disable
security hardening if a feature is misspelled, not recognized, or otherwise
fails to apply (which is the class of issue this PR is addressing). Consider
narrowing the caught exception types (e.g., `SAXNotRecognizedException` /
`SAXNotSupportedException`) and at minimum logging a warning when a
security-critical feature cannot be applied, or failing fast for features that
are required for safe parsing in your threat model.
##########
grails-gradle/model/src/main/groovy/org/grails/io/support/SpringIOUtils.java:
##########
@@ -416,45 +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";
- private static SAXParserFactory createParserFactory() throws
ParserConfigurationException {
- if (saxParserFactory == null) {
- saxParserFactory = FactorySupport.createSaxParserFactory();
- saxParserFactory.setNamespaceAware(true);
- saxParserFactory.setValidating(false);
+ /**
+ * 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
+ };
- try {
-
saxParserFactory.setFeature("https://apache.org/xml/features/disallow-doctype-decl";,
false);
- } catch (Exception pce) {
- // ignore, parser doesn't support
- }
- try {
-
saxParserFactory.setFeature("https://xml.org/sax/features/external-general-entities";,
false);
- } catch (Exception pce) {
- // ignore, parser doesn't support
- }
- try {
-
saxParserFactory.setFeature("https://xml.org/sax/features/external-parameter-entities";,
false);
- } catch (Exception pce) {
- // ignore, parser doesn't support
- }
- try {
-
saxParserFactory.setFeature(XMLConstants.FEATURE_SECURE_PROCESSING, true);
- } catch (Exception e) {
- // ignore, parser doesn't support
- }
- try {
-
saxParserFactory.setFeature("https://apache.org/xml/features/nonvalidating/load-dtd-grammar";,
false);
- } catch (Exception e) {
- // ignore, parser doesn't support
+ private static SAXParserFactory strictParserFactory = null;
+
+ private static SAXParserFactory docTypeParserFactory = null;
+
+ private static SAXParserFactory createParserFactory() throws
ParserConfigurationException {
+ if (isDocTypeDeclarationAllowed()) {
+ if (docTypeParserFactory == null) {
+ docTypeParserFactory = buildParserFactory(true);
}
+ return docTypeParserFactory;
+ }
+ if (strictParserFactory == null) {
+ strictParserFactory = buildParserFactory(false);
Review Comment:
The lazy-initialized static `SAXParserFactory` instances are published
without synchronization/`volatile`, which is not safe publication in Java and
can lead to callers observing a partially initialized factory under race. Make
the fields `volatile` and synchronize initialization (or use the
initialization-on-demand holder idiom) to ensure thread-safe publication.
##########
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:
Pulling `org.apache.grails.gradle:grails-gradle-common` into
`grails-testing-support-http-client` couples the HTTP client library to a
Gradle-oriented artifact, which can bloat the runtime classpath and introduce
transitive dependency conflicts (especially around Gradle APIs). Prefer moving
`XmlParserFeature` to a non-Gradle shared module (or a small shared
`grails-xml-*` module), or use a project dependency (e.g.,
`project(':grails-gradle-common')`) only if this module is guaranteed to be
built/composed within the same multi-project build.
--
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]
