Merge pull request #22036 Adds support for EnableExternalDTDLoad prop…

…erty to Checkstyle Plugin

<!--- The issue this PR addresses -->
Fixes #21624

Co-authored-by: Sterling Greene <sterling@gradle.com>

subprojects/code-quality/src/integTest/groovy/org/gradle/api/plugins/quality/checkstyle/CheckstylePluginExternalDTDOptionIntegrationTest.groovy

@@ -0,0 +1,158 @@
+/*
+ * Copyright 2022 the original author or authors.
+ *
+ * Licensed 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
+ *
+ *      http://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.gradle.api.plugins.quality.checkstyle
+
+import org.gradle.integtests.fixtures.AbstractIntegrationSpec
+import spock.lang.Issue
+import spock.lang.See
+
+import static org.gradle.api.plugins.quality.checkstyle.CheckstylePluginMultiProjectTest.containsClass
+import static org.gradle.api.plugins.quality.checkstyle.CheckstylePluginMultiProjectTest.multilineJavaClass
+
+@Issue("https://github.com/gradle/gradle/issues/21624")
+@See("https://checkstyle.sourceforge.io/config_system_properties.html#Enable_External_DTD_load")
+class CheckstylePluginExternalDTDOptionIntegrationTest extends AbstractIntegrationSpec {
+    def setup() {
+        buildFile << """
+            plugins {
+                id 'java'
+                id 'checkstyle'
+            }
+
+            ${mavenCentralRepository()}
+        """
+
+        file('src/main/java/org/sample/MyClass.java') << multilineJavaClass()
+        file('config/checkstyle/checkstyle-common.xml') << checkStyleCommonXml()
+        file('config/checkstyle/checkstyle.xml') << checkStyleMainXml()
+    }
+
+    def "can use enable_external_dtd_load feature on extension"() {
+        given:
+        buildFile """
+            checkstyle {
+                enableExternalDtdLoad = true
+            }
+        """
+
+        when:
+        fails 'checkstyleMain'
+
+        then:
+        assertFailedWithCheckstyleVerificationErrors()
+    }
+
+    def "can use enable_external_dtd_load feature on task"() {
+        given:
+        buildFile """
+            tasks.withType(Checkstyle).configureEach {
+                enableExternalDtdLoad = true
+            }
+        """
+
+        when:
+        fails 'checkstyleMain'
+
+        then:
+        assertFailedWithCheckstyleVerificationErrors()
+    }
+
+    def "can use enable_external_dtd_load feature on task to override extension value for a task"() {
+        given:
+        buildFile """
+            checkstyle {
+                enableExternalDtdLoad = false
+            }
+
+            tasks.withType(Checkstyle).configureEach {
+                enableExternalDtdLoad = true
+            }
+        """
+
+        when:
+        fails 'checkstyleMain'
+
+        then:
+        assertFailedWithCheckstyleVerificationErrors()
+    }
+
+    def "if use enable_external_dtd_load feature NOT enabled, error if feature used in rules XML"() {
+        given:
+        buildFile """
+            checkstyle {
+                enableExternalDtdLoad = false
+            }
+        """
+
+        when:
+        fails 'checkstyleMain'
+
+        then:
+        assertFailedWithDtdProcessingError()
+    }
+
+    def "enable_external_dtd_load feature NOT enabled by default"() {
+        when:
+        fails 'checkstyleMain'
+
+        then:
+        assertFailedWithDtdProcessingError()
+    }
+
+    private void assertFailedWithCheckstyleVerificationErrors() {
+        result.assertHasErrorOutput("Checkstyle rule violations were found. See the report at:")
+        result.assertHasErrorOutput("Checkstyle files with violations: 1")
+        result.assertHasErrorOutput("Checkstyle violations by severity: [error:2]")
+        file("build/reports/checkstyle/main.xml").assertContents(containsClass("org.sample.MyClass"))
+    }
+
+    private void assertFailedWithDtdProcessingError(String taskName = 'checkstyleMain') {
+        failure.assertHasCause("A failure occurred while executing org.gradle.api.plugins.quality.internal.CheckstyleAction")
+        failure.assertHasCause("An unexpected error occurred configuring and executing Checkstyle.")
+        failure.assertHasCause("java.lang.NullPointerException")
+    }
+
+    private String checkStyleCommonXml() {
+        return """
+            <module name="FileLength">
+                <property name="max" value="1"/>
+            </module>
+            """
+    }
+
+    private String checkStyleMainXml() {
+        // Leave the XML processing instruction at the very first position in the file or else the XML is not valid
+        return """<?xml version="1.0"?>
+            <!DOCTYPE module PUBLIC
+                      "-//Checkstyle//DTD Checkstyle Configuration 1.3//EN"
+                      "https://checkstyle.org/dtds/configuration_1_3.dtd" [
+                <!ENTITY common SYSTEM "checkstyle-common.xml">
+            ]>
+            <module name="Checker">
+
+                &common;
+
+                <module name="TreeWalker">
+                    <module name="MemberName">
+                        <property name="format" value="^[a-z][a-zA-Z]+\$"/>
+                    </module>
+                </module>
+
+            </module>
+            """
+    }
+}

subprojects/code-quality/src/integTest/groovy/org/gradle/api/plugins/quality/checkstyle/CheckstylePluginIntegrationTest.groovy

@@ -27,6 +27,10 @@ class CheckstylePluginIntegrationTest extends WellBehavedPluginTest {
         return "check"
     }
 
+    /**
+     * To ensure the plugins fails (as expected) with configuration cache, do NOT add a repository to the build here,
+     * the tests in the base class are relying on a failure during eager dependency resolution with CC.
+     */
     def setup() {
         buildFile << """
             apply plugin: 'groovy'
@@ -39,13 +43,15 @@ class CheckstylePluginIntegrationTest extends WellBehavedPluginTest {
         buildFile """
             apply plugin: 'checkstyle'
 
-            dependencies { implementation localGroovy() }
             ${mavenCentralRepository()}
 
+            dependencies { implementation localGroovy() }
+
             checkstyle {
                 configProperties["some"] = new URL("https://gradle.org/")
             }
         """
+
         file('src/main/java/Dummy.java') << javaClassWithNewLineAtEnd()
         file('config/checkstyle/checkstyle.xml') << simpleCheckStyleConfig()
 

subprojects/code-quality/src/integTest/groovy/org/gradle/api/plugins/quality/checkstyle/CheckstylePluginMultiProjectTest.groovy

@@ -17,8 +17,11 @@ package org.gradle.api.plugins.quality.checkstyle
 
 import org.gradle.integtests.fixtures.AbstractIntegrationSpec
 import org.gradle.test.fixtures.file.TestFile
+import org.hamcrest.Matcher
 
+import static org.gradle.util.Matchers.containsLine
 import static org.gradle.util.internal.TextUtil.getPlatformLineSeparator
+import static org.hamcrest.CoreMatchers.containsString
 
 class CheckstylePluginMultiProjectTest extends AbstractIntegrationSpec {
 
@@ -130,4 +133,18 @@ class CheckstylePluginMultiProjectTest extends AbstractIntegrationSpec {
     static String javaClassWithNewLineAtEnd() {
         "public class Dummy {}${getPlatformLineSeparator()}"
     }
+
+    static String multilineJavaClass() {
+        return """
+            package org.sample;
+
+            class MyClass {
+              int i = 0;
+            }
+            """
+    }
+
+    static Matcher<String> containsClass(String className) {
+        containsLine(containsString(className.replace(".", File.separator)))
+    }
 }

subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/Checkstyle.java

@@ -76,12 +76,14 @@ public class Checkstyle extends SourceTask implements VerificationTask, Reportin
     private final Property<JavaLauncher> javaLauncher;
     private final Property<String> minHeapSize;
     private final Property<String> maxHeapSize;
+    private final Property<Boolean> enableExternalDtdLoad;
 
     public Checkstyle() {
         this.configDirectory = getObjectFactory().directoryProperty();
         this.reports = getObjectFactory().newInstance(CheckstyleReportsImpl.class, this);
         this.minHeapSize = getObjectFactory().property(String.class);
         this.maxHeapSize = getObjectFactory().property(String.class);
+        this.enableExternalDtdLoad = getObjectFactory().property(Boolean.class).convention(false);
         // Set default JavaLauncher to current JVM in case
         // CheckstylePlugin that sets Java launcher convention is not applied
         this.javaLauncher = getCurrentJvmLauncher();
@@ -198,6 +200,7 @@ private void runWithProcessIsolation() {
             spec.getForkOptions().setMinHeapSize(minHeapSize.getOrNull());
             spec.getForkOptions().setMaxHeapSize(maxHeapSize.getOrNull());
             spec.getForkOptions().setExecutable(javaLauncher.get().getExecutablePath().getAsFile().getAbsolutePath());
+            spec.getForkOptions().getSystemProperties().put("checkstyle.enableExternalDtdLoad", getEnableExternalDtdLoad().get());
             spec.getClasspath().from(getCheckstyleClasspath());
         });
         workQueue.submit(CheckstyleAction.class, this::setupParameters);
@@ -446,4 +449,19 @@ public Property<String> getMinHeapSize() {
     public Property<String> getMaxHeapSize() {
         return maxHeapSize;
     }
+
+    /**
+     * Enable the use of external DTD files in configuration files.
+     * <strong>Disabled by default because this may be unsafe.</strong>
+     * See <a href="https://checkstyle.org/config_system_properties.html#Enable_External_DTD_load">Checkstyle documentation</a> for more details.
+     *
+     * @return property to enable the use of external DTD files
+     *
+     * @since 7.6
+     */
+    @Incubating
+    @Input
+    public Property<Boolean> getEnableExternalDtdLoad() {
+        return enableExternalDtdLoad;
+    }
 }

subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/CheckstyleExtension.java

@@ -15,9 +15,13 @@
  */
 package org.gradle.api.plugins.quality;
 
+import org.gradle.api.Incubating;
 import org.gradle.api.Project;
 import org.gradle.api.file.DirectoryProperty;
+import org.gradle.api.provider.Property;
 import org.gradle.api.resources.TextResource;
+import org.gradle.api.tasks.Input;
+import org.gradle.api.tasks.Optional;
 
 import java.io.File;
 import java.util.LinkedHashMap;
@@ -38,10 +42,12 @@ public class CheckstyleExtension extends CodeQualityExtension {
     private int maxWarnings = Integer.MAX_VALUE;
     private boolean showViolations = true;
     private final DirectoryProperty configDirectory;
+    private final Property<Boolean> enableExternalDtdLoad;
 
     public CheckstyleExtension(Project project) {
         this.project = project;
         this.configDirectory = project.getObjects().directoryProperty();
+        this.enableExternalDtdLoad = project.getObjects().property(Boolean.class).convention(false);
     }
 
     /**
@@ -166,4 +172,20 @@ public boolean isShowViolations() {
     public void setShowViolations(boolean showViolations) {
         this.showViolations = showViolations;
     }
+
+    /**
+     * Enable the ability to use custom DTD files in config and load them from some location on all checkstyle tasks in this project.
+     * <strong>Disabled by default due to security concerns.</strong>
+     * See <a href="https://checkstyle.org/config_system_properties.html#Enable_External_DTD_load">Checkstyle documentation</a> for more details.
+     *
+     * @return The property controlling whether to enable the ability to use custom DTD files
+     *
+     * @since 7.6
+     */
+    @Incubating
+    @Optional
+    @Input
+    public Property<Boolean> getEnableExternalDtdLoad() {
+        return enableExternalDtdLoad;
+    }
 }

subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/CheckstylePlugin.java

@@ -105,6 +105,7 @@ private void configureTaskConventionMapping(Configuration configuration, Checkst
         taskMapping.map("maxErrors", (Callable<Integer>) () -> extension.getMaxErrors());
         taskMapping.map("maxWarnings", (Callable<Integer>) () -> extension.getMaxWarnings());
         task.getConfigDirectory().convention(extension.getConfigDirectory());
+        task.getEnableExternalDtdLoad().convention(extension.getEnableExternalDtdLoad());
     }
 
     private void configureReportsConventionMapping(Checkstyle task, final String baseName) {

subprojects/code-quality/src/main/groovy/org/gradle/api/plugins/quality/internal/CheckstyleInvocationException.java

@@ -0,0 +1,27 @@
+/*
+ * Copyright 2022 the original author or authors.
+ *
+ * Licensed 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
+ *
+ *      http://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.gradle.api.plugins.quality.internal;
+
+import org.gradle.api.GradleException;
+import org.gradle.internal.exceptions.Contextual;
+
+@Contextual
+class CheckstyleInvocationException extends GradleException {
+    CheckstyleInvocationException(String message, Throwable cause) {
+        super(message, cause);
+    }
+}