From 943a678afd97f11f662203ebe33b30b7601163f9 Mon Sep 17 00:00:00 2001 From: James Nord Date: Fri, 3 Mar 2023 13:21:13 +0000 Subject: [PATCH 1/3] simple validation for file based parameters if a File based paramter is set (excluding workingDir which we create) then parsing the arg will fail early rather than continuing and then erroring (or silently swallowing it) later in the run fixes #449 --- .../tools/test/PluginCompatTesterCli.java | 18 ++++++++--- .../picocli/ExistingFileTypeConverter.java | 18 +++++++++++ .../ExistingFileTypeConverterTest.java | 31 +++++++++++++++++++ 3 files changed, 62 insertions(+), 5 deletions(-) create mode 100644 src/main/java/org/jenkins/tools/test/picocli/ExistingFileTypeConverter.java create mode 100644 src/test/java/org/jenkins/tools/test/picocli/ExistingFileTypeConverterTest.java diff --git a/src/main/java/org/jenkins/tools/test/PluginCompatTesterCli.java b/src/main/java/org/jenkins/tools/test/PluginCompatTesterCli.java index 824052833..27bef83d1 100644 --- a/src/main/java/org/jenkins/tools/test/PluginCompatTesterCli.java +++ b/src/main/java/org/jenkins/tools/test/PluginCompatTesterCli.java @@ -37,6 +37,7 @@ import org.jenkins.tools.test.exception.PluginCompatibilityTesterException; import org.jenkins.tools.test.logging.LoggingConfiguration; import org.jenkins.tools.test.model.PluginCompatTesterConfig; +import org.jenkins.tools.test.picocli.ExistingFileTypeConverter; import picocli.CommandLine; @CommandLine.Command( @@ -57,7 +58,8 @@ public class PluginCompatTesterCli implements Callable { @CommandLine.Option( names = {"-w", "--war"}, required = true, - description = "Path to the WAR file to be used by the PCT.") + description = "Path to the WAR file to be used by the PCT.", + converter = ExistingFileTypeConverter.class) private File war; @CommandLine.Option( @@ -104,13 +106,17 @@ public class PluginCompatTesterCli implements Callable { private String fallbackGitHubOrganization; @CheckForNull - @CommandLine.Option(names = "--mvn", description = "The path to the Maven executable.") + @CommandLine.Option( + names = "--mvn", + description = "The path to the Maven executable.", + converter = ExistingFileTypeConverter.class) private File externalMaven; @CheckForNull @CommandLine.Option( names = "--maven-settings", - description = "Settings file to use when executing Maven.") + description = "Settings file to use when executing Maven.", + converter = ExistingFileTypeConverter.class) private File mavenSettings; @CheckForNull @@ -136,14 +142,16 @@ public class PluginCompatTesterCli implements Callable { split = ",", arity = "1", paramLabel = "jar", - description = "Comma-separated list of paths to external hooks JARs.") + description = "Comma-separated list of paths to external hooks JARs.", + converter = ExistingFileTypeConverter.class) private List externalHooksJars; @CheckForNull @CommandLine.Option( names = "--local-checkout-dir", description = - "Folder containing either a local (possibly modified) clone of a plugin repository or a set of local clones of different plugins.") + "Folder containing either a local (possibly modified) clone of a plugin repository or a set of local clones of different plugins.", + converter = ExistingFileTypeConverter.class) private File localCheckoutDir; @CommandLine.Option( diff --git a/src/main/java/org/jenkins/tools/test/picocli/ExistingFileTypeConverter.java b/src/main/java/org/jenkins/tools/test/picocli/ExistingFileTypeConverter.java new file mode 100644 index 000000000..2067bae80 --- /dev/null +++ b/src/main/java/org/jenkins/tools/test/picocli/ExistingFileTypeConverter.java @@ -0,0 +1,18 @@ +package org.jenkins.tools.test.picocli; + +import java.io.File; +import picocli.CommandLine.ITypeConverter; +import picocli.CommandLine.TypeConversionException; + +/** Converter that converts to a File that must exist (either as a directory or a file) */ +public class ExistingFileTypeConverter implements ITypeConverter { + + @Override + public File convert(String value) throws Exception { + File f = new File(value); + if (!f.exists()) { + throw new TypeConversionException("Specified file " + value + " does not exist"); + } + return f; + } +} diff --git a/src/test/java/org/jenkins/tools/test/picocli/ExistingFileTypeConverterTest.java b/src/test/java/org/jenkins/tools/test/picocli/ExistingFileTypeConverterTest.java new file mode 100644 index 000000000..1a663c9c1 --- /dev/null +++ b/src/test/java/org/jenkins/tools/test/picocli/ExistingFileTypeConverterTest.java @@ -0,0 +1,31 @@ +package org.jenkins.tools.test.picocli; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.is; +import static org.junit.jupiter.api.Assertions.assertThrows; + +import java.io.File; +import org.junit.jupiter.api.Test; +import org.junit.jupiter.api.io.TempDir; +import picocli.CommandLine.TypeConversionException; + +class ExistingFileTypeConverterTest { + + @Test + void testValidFile(@TempDir File f) throws Exception { + ExistingFileTypeConverter converter = new ExistingFileTypeConverter(); + File converted = converter.convert(f.getPath()); + assertThat(converted, is(f)); + } + + @Test + void testMissingFile(@TempDir File f) throws Exception { + ExistingFileTypeConverter converter = new ExistingFileTypeConverter(); + TypeConversionException tce = + assertThrows( + TypeConversionException.class, + () -> converter.convert(new File(f, "whatever").getPath())); + assertThat(tce.getMessage(), containsString("whatever")); + } +} From 0077ac67347978cc1ee77ed47d329045102788d5 Mon Sep 17 00:00:00 2001 From: James Nord Date: Fri, 3 Mar 2023 15:17:13 +0000 Subject: [PATCH 2/3] Address spotbugs warning --- .../jenkins/tools/test/picocli/ExistingFileTypeConverter.java | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/src/main/java/org/jenkins/tools/test/picocli/ExistingFileTypeConverter.java b/src/main/java/org/jenkins/tools/test/picocli/ExistingFileTypeConverter.java index 2067bae80..caa093a1e 100644 --- a/src/main/java/org/jenkins/tools/test/picocli/ExistingFileTypeConverter.java +++ b/src/main/java/org/jenkins/tools/test/picocli/ExistingFileTypeConverter.java @@ -1,5 +1,6 @@ package org.jenkins.tools.test.picocli; +import edu.umd.cs.findbugs.annotations.SuppressFBWarnings; import java.io.File; import picocli.CommandLine.ITypeConverter; import picocli.CommandLine.TypeConversionException; @@ -7,6 +8,9 @@ /** Converter that converts to a File that must exist (either as a directory or a file) */ public class ExistingFileTypeConverter implements ITypeConverter { + @SuppressFBWarnings( + value = "PATH_TRAVERSAL_IN", + justification = "by deign, we are converting an argument from the CLI") @Override public File convert(String value) throws Exception { File f = new File(value); From 7be7b5e71bc1024299e3f80293a8bae458119b4e Mon Sep 17 00:00:00 2001 From: James Nord Date: Fri, 3 Mar 2023 16:05:54 +0000 Subject: [PATCH 3/3] try and constrain the JVM to avoid OOMs --- .mvn/jvm.config | 1 + pom.xml | 10 ++++++++++ 2 files changed, 11 insertions(+) create mode 100644 .mvn/jvm.config diff --git a/.mvn/jvm.config b/.mvn/jvm.config new file mode 100644 index 000000000..a5a4d7d40 --- /dev/null +++ b/.mvn/jvm.config @@ -0,0 +1 @@ +-Xmx128m -XX:+HeapDumpOnOutOfMemoryError -XX:+TieredCompilation -XX:TieredStopAtLevel=1 \ No newline at end of file diff --git a/pom.xml b/pom.xml index 65c7e2b58..04d95a5de 100644 --- a/pom.xml +++ b/pom.xml @@ -30,6 +30,7 @@ 4.7.1 2.0.6 Max + @@ -296,9 +297,18 @@ maven-surefire-plugin + @{argLine} -Xmx128m -XX:+HeapDumpOnOutOfMemoryError -XX:+TieredCompilation -XX:TieredStopAtLevel=1 org.jenkins.tools.test.logging.LoggingConfiguration + + + -Xmx256m -XX:+TieredCompilation -XX:TieredStopAtLevel=1 + + false alphabetical