JENKINS-61474 Improve null safety of tool step

Tools page doesn't prevent submission of empty tool names. Some tools (e.g. Allure Commandline) use `Util.fixEmptyAndTrim` on the name, so there is a possibility of NPE when comparing the tool names. By flipping the comparison we avoid NPE since `tool` step requires `name` argument.
jenkinsci · Mar 5, 2024 · 936b400 · 936b400
1 parent 2e7111a
commit 936b400
Show file tree

Hide file tree

Showing 2 changed files with 17 additions and 1 deletion.
diff --git a/src/main/java/org/jenkinsci/plugins/workflow/steps/ToolStep.java b/src/main/java/org/jenkinsci/plugins/workflow/steps/ToolStep.java
@@ -152,7 +152,7 @@ public static final class Execution extends SynchronousNonBlockingStepExecution<
                     continue;
                 }
                 for (ToolInstallation tool : desc.getInstallations()) {
-                    if (tool.getName().equals(name)) {
+                    if (name.equals(tool.getName())) {
                         if (tool instanceof NodeSpecific) {
                             tool = (ToolInstallation) ((NodeSpecific<?>) tool).forNode(getContext().get(Node.class), getContext().get(TaskListener.class));
                         }

diff --git a/src/test/java/org/jenkinsci/plugins/workflow/steps/ToolStepTest.java b/src/test/java/org/jenkinsci/plugins/workflow/steps/ToolStepTest.java
@@ -48,6 +48,7 @@
 import org.junit.Test;
 import org.junit.rules.TemporaryFolder;
 import org.jvnet.hudson.test.BuildWatcher;
+import org.jvnet.hudson.test.Issue;
 import org.jvnet.hudson.test.JenkinsRule;
 import org.jvnet.hudson.test.TestExtension;
 import org.jvnet.hudson.test.ToolInstallations;
@@ -120,6 +121,21 @@ public class ToolStepTest {
                 r.assertBuildStatusSuccess(p.scheduleBuild2(0)));
     }
 
+    @Issue("JENKINS-61474") @Test public void toolWithoutName() throws Exception {
+        File toolHome = folder.newFolder("mockTools");
+        MockToolWithSymbol misconfiguredTool = new MockToolWithSymbol(null, toolHome.getAbsolutePath(), JenkinsRule.NO_PROPERTIES);
+        MockToolWithSymbol tool = new MockToolWithSymbol("mock-tool-with-symbol", toolHome.getAbsolutePath(), JenkinsRule.NO_PROPERTIES);
+        r.jenkins.getDescriptorByType(MockToolWithSymbol.MockToolWithSymbolDescriptor.class).setInstallations(misconfiguredTool, tool);
+
+        WorkflowJob p = r.jenkins.createProject(WorkflowJob.class, "p");
+        p.setDefinition(new CpsFlowDefinition("node {def home = tool \"" + tool.getName() + "\"\n"
+                +"echo \"${home}\"}",
+                true));
+
+        r.assertLogContains(toolHome.getAbsolutePath(),
+                r.assertBuildStatusSuccess(p.scheduleBuild2(0)));
+    }
+
     public static class MockToolWithSymbol extends ToolInstallation {
         public MockToolWithSymbol(String name, String home, List<? extends ToolProperty<?>> properties) {
             super(name, home, properties);