From 936b4007434c625df8cc1bb1ae91abe8dffad235 Mon Sep 17 00:00:00 2001
From: Denys Digtiar <ddigtiar@cloudbees.com>
Date: Tue, 5 Mar 2024 15:03:07 +1000
Subject: [PATCH] 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.
---
 .../plugins/workflow/steps/ToolStep.java         |  2 +-
 .../plugins/workflow/steps/ToolStepTest.java     | 16 ++++++++++++++++
 2 files changed, 17 insertions(+), 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
index dc8ff3fc..ead9a0c6 100644
--- 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
index 6b8576cd..4ca41a89 100644
--- 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);