Merge pull request #439 from jenkinsci/JENKINS-34762-GHissue-352

Fix for empty parameters in SECURITY-170 updates
jenkinsci · Nov 27, 2016 · 17ad7df · 17ad7df
2 parents 2a26b52 + 009d8ec
commit 17ad7df
Show file tree

Hide file tree

Showing 4 changed files with 116 additions and 81 deletions.
diff --git a/...main/java/org/jenkinsci/plugins/ghprb/GhprbAdditionalParameterEnvironmentContributor.java b/...main/java/org/jenkinsci/plugins/ghprb/GhprbAdditionalParameterEnvironmentContributor.java
diff --git a/src/main/java/org/jenkinsci/plugins/ghprb/GhprbParametersAction.java b/src/main/java/org/jenkinsci/plugins/ghprb/GhprbParametersAction.java
@@ -0,0 +1,70 @@
+package org.jenkinsci.plugins.ghprb;
+
+import hudson.EnvVars;
+import hudson.Extension;
+import hudson.model.EnvironmentContributor;
+import hudson.model.ParameterValue;
+import hudson.model.ParametersAction;
+import hudson.model.Run;
+import hudson.model.TaskListener;
+import org.kohsuke.accmod.Restricted;
+import org.kohsuke.accmod.restrictions.NoExternalUse;
+
+import javax.annotation.Nonnull;
+
+import java.io.IOException;
+import java.util.Arrays;
+import java.util.Collections;
+import java.util.List;
+
+
+@Restricted(NoExternalUse.class)
+public class GhprbParametersAction extends ParametersAction {
+
+    private List<ParameterValue> parameters;
+
+    public GhprbParametersAction(List<ParameterValue> parameters) {
+        this.parameters = parameters;
+    }
+
+    public GhprbParametersAction(ParameterValue... parameters) {
+        this(Arrays.asList(parameters));
+    }
+
+    @Override
+    public List<ParameterValue> getParameters() {
+        return Collections.unmodifiableList(parameters);
+    }
+
+    @Override
+    public ParameterValue getParameter(String name) {
+        for (ParameterValue parameter : parameters) {
+            if (parameter != null && parameter.getName().equals(name)) {
+                return parameter;
+            }
+        }
+
+        return null;
+    }
+
+    @Extension
+    public static final class GhprbAdditionalParameterEnvironmentContributor extends EnvironmentContributor {
+
+        // See SECURITY-170
+
+        @Override
+        @SuppressWarnings("rawtypes")
+        public void buildEnvironmentFor(@Nonnull Run run,
+                                        @Nonnull EnvVars envs,
+                                        @Nonnull TaskListener listener) throws IOException, InterruptedException {
+
+            GhprbParametersAction gpa = run.getAction(GhprbParametersAction.class);
+            if (gpa != null) {
+                for (ParameterValue p : gpa.getParameters()) {
+                    envs.put(p.getName(), String.valueOf(p.getValue()));
+                }
+            }
+        }
+    }
+}
+
diff --git a/src/main/java/org/jenkinsci/plugins/ghprb/GhprbTrigger.java b/src/main/java/org/jenkinsci/plugins/ghprb/GhprbTrigger.java
@@ -373,7 +373,7 @@ public QueueTaskFuture<?> scheduleBuild(GhprbCause cause, GhprbRepository repo)
         // add the previous pr BuildData as an action so that the correct change log is generated by the GitSCM plugin
         // note that this will be removed from the Actions list after the job is completed so that the old (and incorrect)
         // one isn't there
-        return this.job.scheduleBuild2(job.getQuietPeriod(), cause, new ParametersAction(values), buildData);
+        return this.job.scheduleBuild2(job.getQuietPeriod(), cause, new GhprbParametersAction(values), buildData);
     }
 
     private void setCommitAuthor(GhprbCause cause, ArrayList<ParameterValue> values) {

diff --git a/src/test/java/org/jenkinsci/plugins/ghprb/GhprbIT.java b/src/test/java/org/jenkinsci/plugins/ghprb/GhprbIT.java
@@ -2,7 +2,11 @@
 
 import com.google.common.collect.Lists;
 
-import hudson.model.FreeStyleProject;
+import hudson.model.*;
+
+import java.util.ArrayList;
+import java.util.List;
+import java.util.Map;
 
 import org.joda.time.DateTime;
 import org.junit.Before;
@@ -22,6 +26,7 @@
 import static org.mockito.BDDMockito.given;
 import static org.mockito.Mockito.any;
 
+
 @RunWith(MockitoJUnitRunner.class)
 public class GhprbIT extends GhprbITBaseTestCase {
 
@@ -104,7 +109,45 @@ public void shouldNotBuildDisabledBuild() throws Exception {
 
         Mockito.verify(ghRepository, Mockito.times(0)).createCommitStatus(any(String.class), any(GHCommitState.class), any(String.class), any(String.class));
     }
-
+
+    @Test
+    public void shouldContainParamsWhenDone() throws Exception {
+        // GIVEN
+        // This test confirms env vars are populated. It only tests one env var
+        // under the premise that if one is populated then all are populated.
+
+        String canaryVar = "ghprbActualCommit";
+
+        given(ghPullRequest.getNumber()).willReturn(1);
+
+        GhprbTestUtil.triggerRunAndWait(10, trigger, project);
+
+        assertThat(project.getBuilds().toArray().length).isEqualTo(1);
+
+        hudson.util.RunList builds = project.getBuilds();
+        Run build = builds.getLastBuild();
+        Map envVars = build.getEnvVars();
+
+        // Ensure that the var is contained in the environment
+        assertThat(envVars.get(canaryVar)).isNotNull();
+
+
+        ArrayList<String> paramsList = newArrayList();
+        List<? extends Action> actions = build.getAllActions();
+        for (Action a : actions) { // SECURITY-170
+            if (a instanceof GhprbParametersAction) {
+                List<ParameterValue> parameterValues = ((GhprbParametersAction) a).getParameters();
+                for (ParameterValue pv : parameterValues) {
+                    paramsList.add(pv.getName());
+                }
+            }
+        }
+
+        // Ensure that the var is contained in the parameters
+        assertThat(paramsList).contains(canaryVar);
+
+    }
+
     @Test
     public void triggerIsRemovedFromListWhenProjectChanges() {