[JENKINS-18938] Added validation for the variable suffix.

jenkinsci · Aug 10, 2015 · 0f3e114 · 0f3e114
1 parent 462bf8f
commit 0f3e114
Show file tree

Hide file tree

Showing 3 changed files with 85 additions and 1 deletion.
diff --git a/src/main/java/hudson/plugins/copyartifact/CopyArtifact.java b/src/main/java/hudson/plugins/copyartifact/CopyArtifact.java
@@ -48,6 +48,7 @@
 import hudson.tasks.Builder;
 import hudson.util.DescribableList;
 import hudson.util.FormValidation;
+import hudson.util.VariableResolver;
 import hudson.util.XStream2;
 
 import java.io.IOException;
@@ -528,6 +529,38 @@ private boolean perform(Run src, Run<?,?> dst, String expandedFilter, String exp
         }
     }
 
+    /**
+     * Tests whether specified variable name is valid.
+     * Package scope for testing purpose.
+     * 
+     * @param variableName
+     * @return
+     */
+    static boolean isValidVariableName(final String variableName) {
+        if(StringUtils.isBlank(variableName)) {
+            return false;
+        }
+
+        // The pattern for variables are defined in hudson.Util.VARIABLE.
+        // It's not exposed unfortunately and tests the variable
+        // by actually expanding that.
+        final String expected = "GOOD";
+        String expanded = Util.replaceMacro(
+            String.format("${%s}", variableName),
+            new VariableResolver<String>() {
+                @Override
+                public String resolve(String name) {
+                    if(variableName.equals(name)) {
+                        return expected;
+                    }
+                    return null;
+                }
+            }
+        );
+
+        return expected.equals(expanded);
+    }
+
     @Extension
     public static final class DescriptorImpl extends BuildStepDescriptor<Builder> {
 
@@ -566,6 +599,20 @@ else if (value.indexOf('$') >= 0)
             return result;
         }
 
+        public FormValidation doCheckResultVariableSuffix(@QueryParameter String value) {
+            value = Util.fixEmptyAndTrim(value);
+            if (value == null) {
+                // optional field.
+                return FormValidation.ok();
+            }
+
+            if (!isValidVariableName(value)) {
+                return FormValidation.error(Messages.CopyArtifact_InvalidVariableName());
+            }
+
+            return FormValidation.ok();
+        }
+
         public boolean isApplicable(Class<? extends AbstractProject> clazz) {
             return true;
         }
@@ -668,7 +715,7 @@ private void add(
         ) {
             if (data==null) return;
 
-            if (StringUtils.isEmpty(resultVariableSuffix)) {
+            if (!isValidVariableName(resultVariableSuffix)) {
                 resultVariableSuffix = calculateDefaultSuffix(build, src, projectName);
                 if (resultVariableSuffix == null) {
                     return;

diff --git a/src/main/resources/hudson/plugins/copyartifact/Messages.properties b/src/main/resources/hudson/plugins/copyartifact/Messages.properties
@@ -10,6 +10,7 @@ see help for project name in job configuration.
 CopyArtifact.MissingSrcArtifacts=Unable to access upstream artifacts area {0}. Does source project archive artifacts?
 CopyArtifact.MissingSrcWorkspace=Unable to access upstream workspace for artifact copy. Slave node offline?
 CopyArtifact.ParameterizedName=Value references a build parameter, so it cannot be validated.
+CopyArtifact.InvalidVariableName=Contains letters not applicable for variable names.
 PermalinkBuildSelector.DisplayName=Specified by permalink
 LastCompletedBuildSelector.DisplayName=Last completed build (ignoring build status)
 StatusBuildSelector.DisplayName=Latest successful build

diff --git a/src/test/java/hudson/plugins/copyartifact/CopyArtifactTest.java b/src/test/java/hudson/plugins/copyartifact/CopyArtifactTest.java
@@ -1828,6 +1828,15 @@ public void testSimpleBuildSelectorDescriptorInOtherPlugin() throws Exception {
         }
     }
 
+    public void testIsValidVariableName() throws Exception {
+        assertTrue(CopyArtifact.isValidVariableName("VarName"));
+        assertTrue(CopyArtifact.isValidVariableName("Var_Name"));
+        assertFalse(CopyArtifact.isValidVariableName(null));
+        assertFalse(CopyArtifact.isValidVariableName(""));
+        assertFalse(CopyArtifact.isValidVariableName("  "));
+        assertFalse(CopyArtifact.isValidVariableName("=/?!\""));
+    }
+
     public void testResultVariableSuffix() throws Exception {
         FreeStyleProject srcProject = createArtifactProject("SRC-PROJECT1");
         FreeStyleBuild srcBuild = srcProject.scheduleBuild2(0).get();
@@ -1887,5 +1896,32 @@ public void testResultVariableSuffix() throws Exception {
             );
             assertNull(ceb.getEnvVars().get("COPYARTIFACT_BUILD_NUMBER_SRC_PROJECT_"));
         }
+
+        // if result variable suffix is invalid,
+        // the default suffix (SRC_PROJECT) is used.
+        {
+            FreeStyleProject p = createFreeStyleProject();
+            p.getBuildersList().add(CopyArtifactUtil.createCopyArtifact(
+                    srcProject.getFullName(),
+                    null,       // parameters
+                    new PermalinkBuildSelector("lastStableBuild"),
+                    "*.txt",    // filter
+                    "",         // excludes
+                    "",         // target
+                    false,      // flatten
+                    false,      // optional
+                    true,       // fingerprintArtifacts
+                    "= =!?"     // resultVariableSuffix
+            ));
+            CaptureEnvironmentBuilder ceb = new CaptureEnvironmentBuilder();
+            p.getBuildersList().add(ceb);
+
+            assertBuildStatusSuccess(p.scheduleBuild2(0));
+
+            assertEquals(
+                    Integer.toString(srcBuild.getNumber()),
+                    ceb.getEnvVars().get("COPYARTIFACT_BUILD_NUMBER_SRC_PROJECT_")
+            );
+        }
     }
 }