From 676f7ab51a443dc6d8c60ca25aaeed61631f07c8 Mon Sep 17 00:00:00 2001 From: Darin Pope Date: Tue, 3 Dec 2024 17:06:28 -0600 Subject: [PATCH 1/3] Trim the environment variables keys and values Add in trimming of the node environment variables key and value values. If you enter values in either fields with leading spaces, trailing spaces or leading and trailing spaces, those spaces would stay and the environment variables would not load. This becomes a real problem when adding `BASE+EXTRA` values, such as `PATH+LOCAL_BIN`. This change trims the `key` and `value` values. --- .../EnvironmentVariablesNodeProperty.java | 5 +++-- .../EnvironmentVariableNodePropertyTest.java | 18 ++++++++++++++++++ 2 files changed, 21 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/hudson/slaves/EnvironmentVariablesNodeProperty.java b/core/src/main/java/hudson/slaves/EnvironmentVariablesNodeProperty.java index dd3ec84b131d..fd923d4f3cac 100644 --- a/core/src/main/java/hudson/slaves/EnvironmentVariablesNodeProperty.java +++ b/core/src/main/java/hudson/slaves/EnvironmentVariablesNodeProperty.java @@ -28,6 +28,7 @@ import hudson.EnvVars; import hudson.Extension; import hudson.Launcher; +import hudson.Util; import hudson.model.AbstractBuild; import hudson.model.BuildListener; import hudson.model.ComputerSet; @@ -118,8 +119,8 @@ private Entry(Map.Entry e) { @DataBoundConstructor public Entry(String key, String value) { - this.key = key; - this.value = value; + this.key = Util.fixEmptyAndTrim(key); + this.value = Util.fixEmptyAndTrim(value); } } diff --git a/test/src/test/java/hudson/slaves/EnvironmentVariableNodePropertyTest.java b/test/src/test/java/hudson/slaves/EnvironmentVariableNodePropertyTest.java index 258c43868d90..e3c102c342e4 100644 --- a/test/src/test/java/hudson/slaves/EnvironmentVariableNodePropertyTest.java +++ b/test/src/test/java/hudson/slaves/EnvironmentVariableNodePropertyTest.java @@ -141,6 +141,24 @@ public void testFormRoundTripForAgent() throws Exception { assertEquals("value", prop.getEnvVars().get("KEY")); } + @Test + public void testExtraSpacesForController() throws Exception { + j.jenkins.getGlobalNodeProperties().replaceBy( + Set.of(new EnvironmentVariablesNodeProperty( + new EnvironmentVariablesNodeProperty.Entry(" KEY ", " globalValue ")))); + + Map envVars = executeBuild(j.jenkins); + + assertEquals("globalValue", envVars.get("KEY")); + } + + @Test + public void testExtraSpacesForAgent() throws Exception { + setVariables(agent, new EnvironmentVariablesNodeProperty.Entry(" KEY ", " agentValue ")); + Map envVars = executeBuild(agent); + assertEquals("agentValue", envVars.get("KEY")); + } + // //////////////////////// setup ////////////////////////////////////////// @Before From 52fce3828105f0edb0a6a398c522a049c99e5929 Mon Sep 17 00:00:00 2001 From: Darin Pope Date: Wed, 4 Dec 2024 11:59:36 -0600 Subject: [PATCH 2/3] add in checks for only spaces and empty --- .../EnvironmentVariablesNodeProperty.java | 4 +-- .../EnvironmentVariableNodePropertyTest.java | 36 +++++++++++++++++++ 2 files changed, 38 insertions(+), 2 deletions(-) diff --git a/core/src/main/java/hudson/slaves/EnvironmentVariablesNodeProperty.java b/core/src/main/java/hudson/slaves/EnvironmentVariablesNodeProperty.java index fd923d4f3cac..69e4e2d2568d 100644 --- a/core/src/main/java/hudson/slaves/EnvironmentVariablesNodeProperty.java +++ b/core/src/main/java/hudson/slaves/EnvironmentVariablesNodeProperty.java @@ -119,8 +119,8 @@ private Entry(Map.Entry e) { @DataBoundConstructor public Entry(String key, String value) { - this.key = Util.fixEmptyAndTrim(key); - this.value = Util.fixEmptyAndTrim(value); + this.key = Util.fixNull(Util.fixEmptyAndTrim(key)); + this.value = Util.fixNull(Util.fixEmptyAndTrim(value)); } } diff --git a/test/src/test/java/hudson/slaves/EnvironmentVariableNodePropertyTest.java b/test/src/test/java/hudson/slaves/EnvironmentVariableNodePropertyTest.java index e3c102c342e4..a590ea8dc501 100644 --- a/test/src/test/java/hudson/slaves/EnvironmentVariableNodePropertyTest.java +++ b/test/src/test/java/hudson/slaves/EnvironmentVariableNodePropertyTest.java @@ -159,6 +159,42 @@ public void testExtraSpacesForAgent() throws Exception { assertEquals("agentValue", envVars.get("KEY")); } + @Test + public void testEmptyForController() throws Exception { + j.jenkins.getGlobalNodeProperties().replaceBy( + Set.of(new EnvironmentVariablesNodeProperty( + new EnvironmentVariablesNodeProperty.Entry("KEY", "")))); + + Map envVars = executeBuild(j.jenkins); + + assertEquals("", envVars.get("KEY")); + } + + @Test + public void testEmptyForAgent() throws Exception { + setVariables(agent, new EnvironmentVariablesNodeProperty.Entry("KEY", "")); + Map envVars = executeBuild(agent); + assertEquals("", envVars.get("KEY")); + } + + @Test + public void testOnlySpacesForController() throws Exception { + j.jenkins.getGlobalNodeProperties().replaceBy( + Set.of(new EnvironmentVariablesNodeProperty( + new EnvironmentVariablesNodeProperty.Entry("KEY", " ")))); + + Map envVars = executeBuild(j.jenkins); + + assertEquals("", envVars.get("KEY")); + } + + @Test + public void testOnlySpacesForAgent() throws Exception { + setVariables(agent, new EnvironmentVariablesNodeProperty.Entry("KEY", " ")); + Map envVars = executeBuild(agent); + assertEquals("", envVars.get("KEY")); + } + // //////////////////////// setup ////////////////////////////////////////// @Before From d38fc0c9d97dfe55f6d9abbdaba7b22d1178a2f7 Mon Sep 17 00:00:00 2001 From: Darin Pope Date: Wed, 4 Dec 2024 14:56:15 -0600 Subject: [PATCH 3/3] handle empty and spaces a little differently After reviewing the help, the last sentence states: ``` If the Value is empty or whitespace-only, it will not be added to the environment, nor will it override or unset any environment variable with the same name that may already exist (e.g. a variable defined by the system). ``` This wasn't true prior to my changes nor any of the commits up to this point. I think this covers all the cases. --- .../EnvironmentVariablesNodeProperty.java | 14 +- .../EnvironmentVariableNodePropertyTest.java | 137 +++++++++++++++++- 2 files changed, 142 insertions(+), 9 deletions(-) diff --git a/core/src/main/java/hudson/slaves/EnvironmentVariablesNodeProperty.java b/core/src/main/java/hudson/slaves/EnvironmentVariablesNodeProperty.java index 69e4e2d2568d..3986fbdd2c47 100644 --- a/core/src/main/java/hudson/slaves/EnvironmentVariablesNodeProperty.java +++ b/core/src/main/java/hudson/slaves/EnvironmentVariablesNodeProperty.java @@ -119,16 +119,20 @@ private Entry(Map.Entry e) { @DataBoundConstructor public Entry(String key, String value) { - this.key = Util.fixNull(Util.fixEmptyAndTrim(key)); - this.value = Util.fixNull(Util.fixEmptyAndTrim(value)); + this.key = Util.fixEmptyAndTrim(key); + this.value = Util.fixEmptyAndTrim(value); } } private static EnvVars toMap(List entries) { EnvVars map = new EnvVars(); - if (entries != null) - for (Entry entry : entries) - map.put(entry.key, entry.value); + if (entries != null) { + for (Entry entry : entries) { + if(entry.key != null && entry.value != null) { + map.put(entry.key, entry.value); + } + } + } return map; } diff --git a/test/src/test/java/hudson/slaves/EnvironmentVariableNodePropertyTest.java b/test/src/test/java/hudson/slaves/EnvironmentVariableNodePropertyTest.java index a590ea8dc501..fa5241710796 100644 --- a/test/src/test/java/hudson/slaves/EnvironmentVariableNodePropertyTest.java +++ b/test/src/test/java/hudson/slaves/EnvironmentVariableNodePropertyTest.java @@ -1,6 +1,7 @@ package hudson.slaves; import static org.junit.Assert.assertEquals; +import static org.junit.Assert.assertNull; import hudson.model.FreeStyleBuild; import hudson.model.FreeStyleProject; @@ -167,14 +168,14 @@ public void testEmptyForController() throws Exception { Map envVars = executeBuild(j.jenkins); - assertEquals("", envVars.get("KEY")); + assertNull(envVars.get("KEY")); } @Test public void testEmptyForAgent() throws Exception { setVariables(agent, new EnvironmentVariablesNodeProperty.Entry("KEY", "")); Map envVars = executeBuild(agent); - assertEquals("", envVars.get("KEY")); + assertNull(envVars.get("KEY")); } @Test @@ -185,14 +186,142 @@ public void testOnlySpacesForController() throws Exception { Map envVars = executeBuild(j.jenkins); - assertEquals("", envVars.get("KEY")); + assertNull(envVars.get("KEY")); } @Test public void testOnlySpacesForAgent() throws Exception { setVariables(agent, new EnvironmentVariablesNodeProperty.Entry("KEY", " ")); Map envVars = executeBuild(agent); - assertEquals("", envVars.get("KEY")); + assertNull(envVars.get("KEY")); + } + + @Test + public void testFormRoundTripForControllerWithEmpty() throws Exception { + j.jenkins.getGlobalNodeProperties().replaceBy( + Set.of(new EnvironmentVariablesNodeProperty( + new EnvironmentVariablesNodeProperty.Entry("", "")))); + + WebClient webClient = j.createWebClient(); + HtmlPage page = webClient.getPage(j.jenkins, "configure"); + HtmlForm form = page.getFormByName("config"); + j.submit(form); + + assertEquals(1, j.jenkins.getGlobalNodeProperties().toList().size()); + + EnvironmentVariablesNodeProperty prop = j.jenkins.getGlobalNodeProperties().get(EnvironmentVariablesNodeProperty.class); + assertEquals(0, prop.getEnvVars().size()); + } + + @Test + public void testFormRoundTripForAgentWithEmpty() throws Exception { + setVariables(agent, new EnvironmentVariablesNodeProperty.Entry("", "")); + + WebClient webClient = j.createWebClient(); + HtmlPage page = webClient.getPage(agent, "configure"); + HtmlForm form = page.getFormByName("config"); + j.submit(form); + + assertEquals(1, agent.getNodeProperties().toList().size()); + + EnvironmentVariablesNodeProperty prop = agent.getNodeProperties().get(EnvironmentVariablesNodeProperty.class); + assertEquals(0, prop.getEnvVars().size()); + } + + @Test + public void testFormRoundTripForControllerWithSpaces() throws Exception { + j.jenkins.getGlobalNodeProperties().replaceBy( + Set.of(new EnvironmentVariablesNodeProperty( + new EnvironmentVariablesNodeProperty.Entry(" ", " ")))); + + WebClient webClient = j.createWebClient(); + HtmlPage page = webClient.getPage(j.jenkins, "configure"); + HtmlForm form = page.getFormByName("config"); + j.submit(form); + + assertEquals(1, j.jenkins.getGlobalNodeProperties().toList().size()); + + EnvironmentVariablesNodeProperty prop = j.jenkins.getGlobalNodeProperties().get(EnvironmentVariablesNodeProperty.class); + assertEquals(0, prop.getEnvVars().size()); + } + + @Test + public void testFormRoundTripForAgentWithSpaces() throws Exception { + setVariables(agent, new EnvironmentVariablesNodeProperty.Entry(" ", " ")); + + WebClient webClient = j.createWebClient(); + HtmlPage page = webClient.getPage(agent, "configure"); + HtmlForm form = page.getFormByName("config"); + j.submit(form); + + assertEquals(1, agent.getNodeProperties().toList().size()); + + EnvironmentVariablesNodeProperty prop = agent.getNodeProperties().get(EnvironmentVariablesNodeProperty.class); + assertEquals(0, prop.getEnvVars().size()); + } + + @Test + public void testFormRoundTripForControllerWithSpacesAndKey() throws Exception { + j.jenkins.getGlobalNodeProperties().replaceBy( + Set.of(new EnvironmentVariablesNodeProperty( + new EnvironmentVariablesNodeProperty.Entry("KEY", " ")))); + + WebClient webClient = j.createWebClient(); + HtmlPage page = webClient.getPage(j.jenkins, "configure"); + HtmlForm form = page.getFormByName("config"); + j.submit(form); + + assertEquals(1, j.jenkins.getGlobalNodeProperties().toList().size()); + + EnvironmentVariablesNodeProperty prop = j.jenkins.getGlobalNodeProperties().get(EnvironmentVariablesNodeProperty.class); + assertEquals(0, prop.getEnvVars().size()); + } + + @Test + public void testFormRoundTripForAgentWithSpacesAndKey() throws Exception { + setVariables(agent, new EnvironmentVariablesNodeProperty.Entry("KEY", " ")); + + WebClient webClient = j.createWebClient(); + HtmlPage page = webClient.getPage(agent, "configure"); + HtmlForm form = page.getFormByName("config"); + j.submit(form); + + assertEquals(1, agent.getNodeProperties().toList().size()); + + EnvironmentVariablesNodeProperty prop = agent.getNodeProperties().get(EnvironmentVariablesNodeProperty.class); + assertEquals(0, prop.getEnvVars().size()); + } + + @Test + public void testFormRoundTripForControllerWithEmptyAndKey() throws Exception { + j.jenkins.getGlobalNodeProperties().replaceBy( + Set.of(new EnvironmentVariablesNodeProperty( + new EnvironmentVariablesNodeProperty.Entry("KEY", "")))); + + WebClient webClient = j.createWebClient(); + HtmlPage page = webClient.getPage(j.jenkins, "configure"); + HtmlForm form = page.getFormByName("config"); + j.submit(form); + + assertEquals(1, j.jenkins.getGlobalNodeProperties().toList().size()); + + EnvironmentVariablesNodeProperty prop = j.jenkins.getGlobalNodeProperties().get(EnvironmentVariablesNodeProperty.class); + assertEquals(0, prop.getEnvVars().size()); + } + + @Test + public void testFormRoundTripForAgentWithEmptyAndKey() throws Exception { + setVariables(agent, new EnvironmentVariablesNodeProperty.Entry("KEY", "")); + + WebClient webClient = j.createWebClient(); + HtmlPage page = webClient.getPage(agent, "configure"); + HtmlForm form = page.getFormByName("config"); + j.submit(form); + + assertEquals(1, agent.getNodeProperties().toList().size()); + + EnvironmentVariablesNodeProperty prop = agent.getNodeProperties().get(EnvironmentVariablesNodeProperty.class); + assertEquals(0, prop.getEnvVars().size()); } // //////////////////////// setup //////////////////////////////////////////