[JENKINS-34826] - Prevent failures when using parameters from the pro…

…moted build (#93) * [JENKINS-34826] - Introduce PromotionParametersAction with relaxed security checks * [SECURITY-170] - Add test to ensure that ManualApproval parameters are filtered properly * [JENKINS-34826] - Replace the deprecated method * [JENKINS-34826] - Adjust naming to be more explicit * [JENKINS-34826] - Address comments from @amuniz
jenkinsci · May 25, 2016 · 57946a3 · 57946a3
1 parent af0cb71
commit 57946a3
Show file tree

Hide file tree

Showing 4 changed files with 101 additions and 23 deletions.
diff --git a/src/main/java/hudson/plugins/promoted_builds/Promotion.java b/src/main/java/hudson/plugins/promoted_builds/Promotion.java
@@ -49,6 +49,8 @@
 import java.util.logging.Logger;
 import javax.annotation.CheckForNull;
 import javax.annotation.Nonnull;
+import org.kohsuke.accmod.Restricted;
+import org.kohsuke.accmod.restrictions.NoExternalUse;
 
 /**
  * Records a promotion process.
@@ -461,32 +463,70 @@ public static ParametersAction getParametersActions(Promotion build){
      * @param actions
      * @param build
      * @param promotionParams
+     * @deprecated Use {@link PromotionParametersAction} with constructor instead.
      */
-	public static void buildParametersAction(@Nonnull List<Action> actions, 
-                @Nonnull AbstractBuild<?, ?> build,
-                @CheckForNull List<ParameterValue> promotionParams) {
-        if (promotionParams == null) {
-            promotionParams = new ArrayList<ParameterValue>();
+    @Deprecated
+    public static void buildParametersAction(@Nonnull List<Action> actions,
+            @Nonnull AbstractBuild<?, ?> build,
+            @CheckForNull List<ParameterValue> promotionParams) {
+        // Create list of actions to pass to scheduled build
+        actions.add(PromotionParametersAction.buildFor(build, promotionParams));
+    }
+
+    private static final Logger LOGGER = Logger.getLogger(Promotion.class.getName());
+
+    /**
+     * Action, which stores promotion parameters.
+     * This class allows defining custom parameters filtering logic, which is
+     * important for versions after the SECURITY-170 fix.
+     * @since TODO
+     */
+    @Restricted(NoExternalUse.class)
+    public static class PromotionParametersAction extends ParametersAction {
+
+        private List<ParameterValue> unfilteredParameters;
+
+        private PromotionParametersAction(List<ParameterValue> params) {
+            // Pass the parameters upstairs
+            super(params);   
+            unfilteredParameters = params;
         }
 
-		List<ParameterValue> params=new ArrayList<ParameterValue>();
-
-		//Add the target build parameters first, if the same parameter is not being provided bu the promotion build
-        List<ParametersAction> parameters = build.getActions(ParametersAction.class);
-        for(ParametersAction paramAction:parameters){
-        	for (ParameterValue pvalue:paramAction.getParameters()){
-        		if (!promotionParams.contains(pvalue)){
-        			params.add(pvalue);
-        		}
-        	}
+        @Override
+        public List<ParameterValue> getParameters() {
+            return Collections.unmodifiableList(filter(unfilteredParameters));
         }
 
-        //Add all the promotion build parameters
-        params.addAll(promotionParams);
+        private List<ParameterValue> filter(List<ParameterValue> params) {
+            // buildToBePromoted::getParameters() invokes the secured method, hence all
+            // parameters from the promoted build are safe.
+            return params;
+        }
 
-        // Create list of actions to pass to scheduled build
-        actions.add(new ParametersAction(params));
-	}
+        public static PromotionParametersAction buildFor(
+                @Nonnull AbstractBuild<?, ?> buildToBePromoted,
+                @CheckForNull List<ParameterValue> promotionParams) {
+            if (promotionParams == null) {
+                promotionParams = new ArrayList<ParameterValue>();
+            }
 
-    private static final Logger LOGGER = Logger.getLogger(Promotion.class.getName());
+            List<ParameterValue> params = new ArrayList<ParameterValue>();
+
+            //Add the target build parameters first, if the same parameter is not being provided by the promotion build
+            List<ParametersAction> parameters = buildToBePromoted.getActions(ParametersAction.class);
+            for (ParametersAction paramAction : parameters) {
+                for (ParameterValue pvalue : paramAction.getParameters()) {
+                    if (!promotionParams.contains(pvalue)) {
+                        params.add(pvalue);
+                    }
+                }
+            }
+
+            //Add all the promotion build parameters
+            params.addAll(promotionParams);
+
+            // Create list of actions to pass to scheduled build
+            return new PromotionParametersAction(params);
+        }
+    }
 }
diff --git a/src/main/java/hudson/plugins/promoted_builds/PromotionProcess.java b/src/main/java/hudson/plugins/promoted_builds/PromotionProcess.java
@@ -521,7 +521,7 @@ public boolean scheduleBuild(@Nonnull AbstractBuild<?,?> build, @Nonnull Cause c
     public Future<Promotion> scheduleBuild2(@Nonnull AbstractBuild<?,?> build, 
             Cause cause, @CheckForNull List<ParameterValue> params) {
         List<Action> actions = new ArrayList<Action>();
-        Promotion.buildParametersAction(actions, build, params);
+        actions.add(Promotion.PromotionParametersAction.buildFor(build, params));
         actions.add(new PromotionTargetAction(build));
 
         // remember what build we are promoting

diff --git a/src/test/java/hudson/plugins/promoted_builds/conditions/ManualConditionTest.java b/src/test/java/hudson/plugins/promoted_builds/conditions/ManualConditionTest.java
@@ -28,6 +28,9 @@
 import com.gargoylesoftware.htmlunit.html.HtmlElement;
 import com.gargoylesoftware.htmlunit.html.HtmlForm;
 import com.gargoylesoftware.htmlunit.html.HtmlPage;
+import hudson.model.StringParameterValue;
+import hudson.model.TaskListener;
+import org.jvnet.hudson.test.Issue;
 
 /**
  * @author Kohsuke Kawaguchi
@@ -78,7 +81,41 @@ public void testManualPromotionProcess() throws Exception {
         assertFalse(statuses.getPromotions().isEmpty());
     }
 
-
+    @Issue("SECURITY-170")
+    /**
+     * Verify that the plugin is tolerant against SECURITY-170 in Manual conditions
+     */
+    public void testManualPromotionProcessWithInvalidParam() throws Exception {
+        FreeStyleProject p = createFreeStyleProject();
+        ExtensionList<Descriptor> list = hudson.getExtensionList(Descriptor.class);
+        list.add(new JobPropertyImpl.DescriptorImpl(JobPropertyImpl.class));
+        JobPropertyImpl base =  new JobPropertyImpl(p);
+        p.addProperty(base);
+        PromotionProcess foo = base.addProcess("foo");
+
+        ManualCondition condition=new ManualCondition();
+        condition.getParameterDefinitions().add(new StringParameterDefinition("FOO", "BAR", "Test parameter"));
+        foo.conditions.add(condition);
+
+        FreeStyleBuild b1 = assertBuildStatusSuccess(p.scheduleBuild2(0));
+
+        // Promote a build. Also add one invalid parameter 
+        List<ParameterValue> paramValues = condition.createDefaultValues();
+        paramValues.add(new StringParameterValue("INVALID_PARAM", "hacked!"));
+        assertBuildStatusSuccess(condition.approve(b1, foo, paramValues));
+        ManualApproval manualApproval = b1.getAction(ManualApproval.class);
+        assertNotNull(manualApproval);
+        List<ParameterValue> parameterValues = manualApproval.badge.getParameterValues();
+
+        // Verify that the build succeeds && has no INVALID_PARAM
+        PromotedBuildAction statuses=b1.getAction(PromotedBuildAction.class);
+        assertNotNull(statuses);
+        assertNotNull(statuses.getPromotions());
+        assertFalse(statuses.getPromotions().isEmpty());
+        Promotion pb = base.getItem("foo").getBuildByNumber(1);
+        assertNotNull("INVALID_PARAM should not be injected into the environment", 
+                pb.getEnvironment(TaskListener.NULL).get("INVALID_PARAM", null));
+    }
 
     public void testManualPromotionProcessViaWebClient() throws Exception {
         FreeStyleProject p = createFreeStyleProject();

diff --git a/src/test/java/hudson/plugins/promoted_builds/conditions/SelfPromotionTest.java b/src/test/java/hudson/plugins/promoted_builds/conditions/SelfPromotionTest.java
@@ -135,6 +135,7 @@ public void testFailure() throws Exception {
     }
 
     @Bug(22679)
+    // @Bug(34826) // Can be reproduced in Jenkins 2.3 +
     public void testPromotionEnvironmentShouldIncludeTargetParameters() throws Exception {
         String paramName = "param";