Skip to content

Commit 2d8ec07

Browse files
GaneshSPatilzabil
GaneshSPatil
authored and
zabil
committed
added a validation for starting and trailing spaces of environmentVariable name. Added a xsl transformation to trim the existing environment variable name from cruise-config.xml file (#2109)
1 parent d5d3569 commit 2d8ec07

File tree

9 files changed

+1090
-4
lines changed

9 files changed

+1090
-4
lines changed

‎base/src/com/thoughtworks/go/util/GoConstants.java‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -58,7 +58,7 @@ public class GoConstants {
5858

5959
public static final String PRODUCT_NAME = "go";
6060

61-
public static final int CONFIG_SCHEMA_VERSION = 84;
61+
public static final int CONFIG_SCHEMA_VERSION = 85;
6262

6363
public static final String APPROVAL_SUCCESS = "success";
6464
public static final String APPROVAL_MANUAL = "manual";

‎config/config-api/src/com/thoughtworks/go/config/EnvironmentVariableConfig.java‎

Lines changed: 4 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -172,6 +172,10 @@ public void validateName(Map<String, EnvironmentVariableConfig> variableConfigMa
172172
String currentVariableName = name.toLowerCase();
173173
String parentDisplayName = validationContext.getParentDisplayName();
174174
CaseInsensitiveString parentName = getParentNameFrom(validationContext);
175+
if(currentVariableName.startsWith(" ") || currentVariableName.endsWith(" ")){
176+
configErrors.add(NAME, String.format("Environment Variable cannot start or end with spaces for %s '%s'.", parentDisplayName, parentName));
177+
return;
178+
}
175179
if (StringUtil.isBlank(currentVariableName)) {
176180
configErrors.add(NAME, String.format("Environment Variable cannot have an empty name for %s '%s'.", parentDisplayName, parentName));
177181
return;

‎config/config-api/test/com/thoughtworks/go/config/domain/EnvironmentVariablesConfigTest.java‎

Lines changed: 24 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -98,6 +98,30 @@ public void shouldPopulateErrorWhenVariableNameIsEmpty() {
9898
assertThat(one.errors().on(EnvironmentVariableConfig.NAME), contains("Environment Variable cannot have an empty name for pipeline 'some-pipeline'."));
9999
}
100100

101+
@Test
102+
public void shouldPopulateErrorWhenVariableNameStartsWithSpace() {
103+
environmentVariablesConfig = new EnvironmentVariablesConfig();
104+
EnvironmentVariableConfig one = new EnvironmentVariableConfig(" foo", "BAR");
105+
environmentVariablesConfig.add(one);
106+
107+
environmentVariablesConfig.validate(context);
108+
109+
assertThat(one.errors().isEmpty(), is(false));
110+
assertThat(one.errors().on(EnvironmentVariableConfig.NAME), contains("Environment Variable cannot start or end with spaces for pipeline 'some-pipeline'."));
111+
}
112+
113+
@Test
114+
public void shouldPopulateErrorWhenVariableNameEndsWithSpace() {
115+
environmentVariablesConfig = new EnvironmentVariablesConfig();
116+
EnvironmentVariableConfig one = new EnvironmentVariableConfig("FOO ", "BAR");
117+
environmentVariablesConfig.add(one);
118+
119+
environmentVariablesConfig.validate(context);
120+
121+
assertThat(one.errors().isEmpty(), is(false));
122+
assertThat(one.errors().on(EnvironmentVariableConfig.NAME), contains("Environment Variable cannot start or end with spaces for pipeline 'some-pipeline'."));
123+
}
124+
101125
@Test
102126
public void shouldClearEnvironmentVariablesWhenTheMapIsNull() {
103127
environmentVariablesConfig = new EnvironmentVariablesConfig();

‎config/config-server/resources/cruise-config.xsd‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -77,7 +77,7 @@
7777
</xsd:unique>
7878
</xsd:element>
7979
</xsd:sequence>
80-
<xsd:attribute name="schemaVersion" type="xsd:int" use="required" fixed="84"/>
80+
<xsd:attribute name="schemaVersion" type="xsd:int" use="required" fixed="85"/>
8181
</xsd:complexType>
8282
<xsd:unique name="uniquePipelines">
8383
<xsd:selector xpath="pipelines"/>

‎config/config-server/resources/schemas/84_cruise-config.xsd‎

Lines changed: 995 additions & 0 deletions
Large diffs are not rendered by default.

‎config/config-server/resources/upgrades/85.xsl‎

Lines changed: 37 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,37 @@
1+
<?xml version="1.0"?>
2+
<!--
3+
~ Copyright 2016 ThoughtWorks, Inc.
4+
~
5+
~ Licensed under the Apache License, Version 2.0 (the "License");
6+
~ you may not use this file except in compliance with the License.
7+
~ You may obtain a copy of the License at
8+
~
9+
~ http://www.apache.org/licenses/LICENSE-2.0
10+
~
11+
~ Unless required by applicable law or agreed to in writing, software
12+
~ distributed under the License is distributed on an "AS IS" BASIS,
13+
~ WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
14+
~ See the License for the specific language governing permissions and
15+
~ limitations under the License.
16+
-->
17+
18+
<xsl:stylesheet xmlns:xsl="http://www.w3.org/1999/XSL/Transform" version="1.0">
19+
<xsl:template match="/cruise/@schemaVersion">
20+
<xsl:attribute name="schemaVersion">85</xsl:attribute>
21+
</xsl:template>
22+
<!-- Copy everything -->
23+
<xsl:template match="@*|node()">
24+
<xsl:copy>
25+
<xsl:apply-templates select="@*|node()"/>
26+
</xsl:copy>
27+
</xsl:template>
28+
29+
<xsl:template match="//environmentvariables/variable">
30+
<xsl:copy>
31+
<xsl:attribute name="name">
32+
<xsl:value-of select="translate(@name, ' ','')"/>
33+
</xsl:attribute>
34+
<xsl:apply-templates />
35+
</xsl:copy>
36+
</xsl:template>
37+
</xsl:stylesheet>

‎server/config/cruise-config.xml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -1,5 +1,5 @@
11
<?xml version="1.0" encoding="utf-8"?>
2-
<cruise xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="cruise-config.xsd" schemaVersion="84">
2+
<cruise xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="cruise-config.xsd" schemaVersion="85">
33
<server artifactsdir="logs" agentAutoRegisterKey="041b5c7e-dab2-11e5-a908-13f95f3c6ef6" commandRepositoryLocation="default" serverId="dev-id">
44
<security>
55
<passwordFile path="../manual-testing/ant_hg/password.properties" />

‎server/test-resources/cruise-config-with-encrypted-with-flawed-cipher.xml‎

Lines changed: 1 addition & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -16,7 +16,7 @@
1616
~
1717
-->
1818

19-
<cruise xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="cruise-config.xsd" schemaVersion="81">
19+
<cruise xmlns:xsi="http://www.w3.org/2001/XMLSchema-instance" xsi:noNamespaceSchemaLocation="cruise-config.xsd" schemaVersion="85">
2020
<server artifactsdir="logs" commandRepositoryLocation="default" serverId="dev-id">
2121
<security>
2222
<ldap uri="ldap://ldap-server" managerDn="manager-dn" encryptedManagerPassword="ruRUF0mi2ia/BWpWMISbjQ==" searchFilter="(sAMAccountName={0})">

‎server/test/integration/com/thoughtworks/go/config/GoConfigMigrationIntegrationTest.java‎

Lines changed: 26 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -1182,6 +1182,32 @@ public void shouldRemoveEmptyTagsRecursively_asPartOfMigration78() throws Except
11821182
assertThat(migratedXml, not(containsString("<authorization>")));
11831183
}
11841184

1185+
@Test
1186+
public void ShouldTrimEnvironmentVariables_asPartOfMigration85() throws Exception {
1187+
String configXml =
1188+
"<cruise schemaVersion='84'>"
1189+
+" <pipelines group='first'>"
1190+
+" <authorization>"
1191+
+" <view>"
1192+
+" <user></user>"
1193+
+" </view>"
1194+
+" </authorization>"
1195+
+" <pipeline name='up42'>"
1196+
+" <environmentvariables>"
1197+
+" <variable name=\"test \">"
1198+
+" <value>abcd</value>"
1199+
+" </variable>"
1200+
+" </environmentvariables>"
1201+
+" <materials>"
1202+
+" <hg url='../manual-testing/ant_hg/dummy' />"
1203+
+" </materials>"
1204+
+" </pipeline>"
1205+
+" </pipelines>"
1206+
+"</cruise>";
1207+
String migratedXml = migrateXmlString(configXml, 84);
1208+
assertThat(migratedXml, not(containsString("test ")));
1209+
assertThat(migratedXml, containsString("test"));
1210+
}
11851211

11861212
private void assertStringsIgnoringCarriageReturnAreEqual(String expected, String actual) {
11871213
assertEquals(expected.replaceAll("\\r", ""), actual.replaceAll("\\r", ""));

0 commit comments

Comments
 (0)