Merge pull request #285 from jenkinsci/JENKINS-34753

[JENKINS-34753] Provide safe parameters to ParametersAction
jenkinsci · May 30, 2016 · 8b46525 · 8b46525
2 parents 644d75d + d8878ba
commit 8b46525
Show file tree

Hide file tree

Showing 11 changed files with 313 additions and 48 deletions.
diff --git a/pom.xml b/pom.xml
@@ -54,6 +54,7 @@
 
     <properties>
         <jenkins.version>1.609.3</jenkins.version>
+        <!--<jenkins.version>2.5-SNAPSHOT</jenkins.version>-->
         <java.level>6</java.level>
         <project.build.sourceEncoding>UTF-8</project.build.sourceEncoding>
         <findbugs.failOnError>false</findbugs.failOnError>

diff --git a/...java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/ParameterExpander.java b/...java/com/sonyericsson/hudson/plugins/gerrit/trigger/gerritnotifier/ParameterExpander.java
@@ -40,14 +40,14 @@
 import hudson.model.Result;
 import hudson.model.Run;
 import hudson.model.TaskListener;
-import hudson.model.Hudson;
 
 import java.util.Arrays;
 import java.util.Collections;
 import java.util.Comparator;
 import java.util.HashMap;
 import java.util.Map;
 
+import jenkins.model.Jenkins;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
@@ -72,24 +72,24 @@ public class ParameterExpander {
 
     private static final Logger logger = LoggerFactory.getLogger(ParameterExpander.class);
     private IGerritHudsonTriggerConfig config;
-    private Hudson hudson;
+    private Jenkins jenkins;
 
     /**
      * Constructor.
      * @param config the global config.
-     * @param hudson the Hudson instance.
+     * @param jenkins the Hudson instance.
      */
-    public ParameterExpander(IGerritHudsonTriggerConfig config, Hudson hudson) {
+    public ParameterExpander(IGerritHudsonTriggerConfig config, Jenkins jenkins) {
         this.config = config;
-        this.hudson = hudson;
+        this.jenkins = jenkins;
     }
 
     /**
      * Constructor.
      * @param config the global config.
      */
     public ParameterExpander(IGerritHudsonTriggerConfig config) {
-        this(config, Hudson.getInstance());
+        this(config, Jenkins.getInstance());
     }
 
     /**
@@ -240,7 +240,7 @@ private Map<String, String> createStandardParameters(Run r, GerritTriggeredEvent
             }
         }
         if (r != null) {
-            map.put("BUILDURL", hudson.getRootUrl() + r.getUrl());
+            map.put("BUILDURL", jenkins.getRootUrl() + r.getUrl());
         }
         map.put("VERIFIED", String.valueOf(verified));
         map.put("CODE_REVIEW", String.valueOf(codeReview));
@@ -554,7 +554,7 @@ public String getBuildCompletedCommand(MemoryImprint memoryImprint, TaskListener
     private String createBuildsStats(MemoryImprint memoryImprint, TaskListener listener,
             Map<String, String> parameters) {
         StringBuilder str = new StringBuilder("");
-        final String rootUrl = hudson.getRootUrl();
+        final String rootUrl = jenkins.getRootUrl();
 
         String unsuccessfulMessage = null;
 

diff --git a/...main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/hudsontrigger/EventListener.java b/...main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/hudsontrigger/EventListener.java
@@ -43,13 +43,19 @@
 import hudson.model.ParametersDefinitionProperty;
 import jenkins.model.Jenkins;
 import jenkins.model.ParameterizedJobMixIn;
+import org.apache.commons.lang3.StringUtils;
 import org.slf4j.Logger;
 import org.slf4j.LoggerFactory;
 
 import javax.annotation.CheckForNull;
 import javax.annotation.Nonnull;
+import java.lang.reflect.Constructor;
+import java.lang.reflect.Field;
+import java.lang.reflect.Modifier;
 import java.util.ArrayList;
+import java.util.Collection;
 import java.util.List;
+import java.util.Locale;
 import java.util.concurrent.Future;
 
 import static com.sonyericsson.hudson.plugins.gerrit.trigger.PluginImpl.getServerConfig;
@@ -243,6 +249,10 @@ protected Job asJob() {
 
     /**
      * Creates a ParameterAction and fills it with the project's default parameters + the Standard Gerrit parameters.
+     * If running on a core version that let's us specify safeParameters for the ParameterAction
+     * the Gerrit specific parameters will be specified in the safeParameters list in addition to anything the admin
+     * might have set.
+     * A warning will be printed to the log if that is not possible but SECURITY-170 appears to be in effect.
      *
      * @param event   the event.
      * @param project the project.
@@ -251,6 +261,42 @@ protected Job asJob() {
     protected ParametersAction createParameters(GerritTriggeredEvent event, Job project) {
         List<ParameterValue> parameters = getDefaultParametersValues(project);
         setOrCreateParameters(event, project, parameters);
+        try {
+            Constructor<ParametersAction> constructor = ParametersAction.class.getConstructor(List.class,
+                                                                                              Collection.class);
+            return constructor.newInstance(parameters, GerritTriggerParameters.getNamesSet());
+        } catch (NoSuchMethodException e) {
+            ParametersActionInspection inspection = getParametersInspection();
+            if (inspection.isInspectionFailure()) {
+                logger.warn("Failed to inspect ParametersAction to determine "
+                                    + "if we can behave normally around SECURITY-170.\nSee "
+                                    + "https://wiki.jenkins-ci.org/display/SECURITY/Jenkins+Security+Advisory+2016-05-11"
+                                    + " for information.");
+            } else if (inspection.isHasSafeParameterConfig()) {
+                StringBuilder txt = new StringBuilder(
+                        "Running on a core with SECURITY-170 fixed but no direct way for Gerrit Trigger"
+                                + " to self-specify safe parameters.");
+                txt.append(" You should consider upgrading to a new Jenkins core version.\n");
+                if (inspection.isKeepUndefinedParameters()) {
+                    txt.append(".keepUndefinedParameters is set so the trigger should behave normally.");
+                } else if (inspection.isSafeParametersSet()) {
+                    txt.append("All Gerrit related parameters are set in .safeParameters");
+                    txt.append(" so the trigger should behave normally.");
+                } else {
+                    txt.append("No overriding system properties appears to be set,");
+                    txt.append(" your builds might not work as expected.\n");
+                    txt.append("See https://wiki.jenkins-ci.org/display/SECURITY/Jenkins+Security+Advisory+2016-05-11");
+                    txt.append(" for information.");
+                }
+                logger.warn(txt.toString());
+            } else {
+                logger.debug("Running on an old core before safe parameters, we should be safe.", e);
+            }
+        } catch (IllegalAccessException e) {
+            logger.warn("Running on a core with safe parameters fix available, but not allowed to specify them", e);
+        } catch (Exception e) {
+            logger.warn("Running on a core with safe parameters fix available, but failed to provide them", e);
+        }
         return new ParametersAction(parameters);
     }
 
@@ -332,4 +378,113 @@ public int hashCode() {
     public boolean equals(Object obj) {
         return obj instanceof EventListener && ((EventListener)obj).job.equals(job);
     }
+
+    /**
+     * Inspects {@link ParametersAction} to see what kind of capabilities it has in regards to SECURITY-170.
+     * Assuming the safeParameters constructor could not be found.
+     *
+     * @return the inspection result
+     * @see #createParameters(GerritTriggeredEvent, Job)
+     */
+    private static synchronized ParametersActionInspection getParametersInspection() {
+        if (parametersInspectionCache == null) {
+            parametersInspectionCache = new ParametersActionInspection();
+        }
+        return parametersInspectionCache;
+    }
+
+    /**
+     * Stored cache of the inspection.
+     * @see #getParametersInspection()
+     */
+    private static volatile ParametersActionInspection parametersInspectionCache = null;
+
+    /**
+     * Data structure with information regarding what kind of capabilities {@link ParametersAction} has.
+     * @see #getParametersInspection()
+     * @see #createParameters(GerritTriggeredEvent, Job)
+     */
+    private static class ParametersActionInspection {
+        private static final Class<ParametersAction> KLASS = ParametersAction.class;
+        private boolean inspectionFailure;
+        private boolean safeParametersSet = false;
+        private boolean keepUndefinedParameters = false;
+        private boolean hasSafeParameterConfig = false;
+
+        /**
+         * Constructor that performs the inspection.
+         */
+        ParametersActionInspection() {
+            try {
+                for (Field field : KLASS.getDeclaredFields()) {
+                    if (Modifier.isStatic(field.getModifiers())
+                            &&  (
+                                 field.getName().equals("KEEP_UNDEFINED_PARAMETERS_SYSTEM_PROPERTY_NAME")
+                                 || field.getName().equals("SAFE_PARAMETERS_SYSTEM_PROPERTY_NAME")
+                                )
+                       ) {
+                        this.hasSafeParameterConfig = true;
+                        break;
+                    }
+                }
+                if (hasSafeParameterConfig) {
+                    if (Boolean.getBoolean(KLASS.getName() + ".keepUndefinedParameters")) {
+                        this.keepUndefinedParameters = true;
+                    }
+                    String safeParameters = System.getProperty(KLASS.getName() + ".safeParameters");
+                    if (!StringUtils.isBlank(safeParameters)) {
+                        safeParameters = safeParameters.toUpperCase(Locale.ENGLISH);
+                        boolean declared = true;
+                        for (GerritTriggerParameters parameter : GerritTriggerParameters.values()) {
+                            if (!safeParameters.contains(parameter.name())) {
+                                declared = false;
+                                break;
+                            }
+                        }
+                        this.safeParametersSet = declared;
+                    } else {
+                        this.safeParametersSet = false;
+                    }
+                }
+                this.inspectionFailure = false;
+            } catch (Exception e) {
+                this.inspectionFailure = true;
+            }
+        }
+
+        /**
+         * If the system property .safeParameters is set and contains all Gerrit related parameters.
+         * @return true if so.
+         */
+        boolean isSafeParametersSet() {
+            return safeParametersSet;
+        }
+
+        /**
+         * If the system property .keepUndefinedParameters is set and set to true.
+         *
+         * @return true if so.
+         */
+        boolean isKeepUndefinedParameters() {
+            return keepUndefinedParameters;
+        }
+
+        /**
+         * If any of the constant fields regarding safeParameters are declared in {@link ParametersAction}.
+         *
+         * @return true if so.
+         */
+        boolean isHasSafeParameterConfig() {
+            return hasSafeParameterConfig;
+        }
+
+        /**
+         * If there was an exception when inspecting the class.
+         *
+         * @return true if so.
+         */
+        public boolean isInspectionFailure() {
+            return inspectionFailure;
+        }
+    }
 }
diff --git a/src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/hudsontrigger/GerritCause.java b/src/main/java/com/sonyericsson/hudson/plugins/gerrit/trigger/hudsontrigger/GerritCause.java
@@ -233,7 +233,10 @@ public boolean equals(Object o) {
 
     @Override
     public int hashCode() {
-        int result = tEvent.hashCode();
+        int result = 1;
+        if (tEvent != null) {
+            result = tEvent.hashCode();
+        }
         result = 31 * result + (silentMode ? 1 : 0);
         return result;
     }

diff --git a/...com/sonyericsson/hudson/plugins/gerrit/trigger/hudsontrigger/GerritTriggerParameters.java b/...com/sonyericsson/hudson/plugins/gerrit/trigger/hudsontrigger/GerritTriggerParameters.java
@@ -51,6 +51,8 @@
 import java.lang.reflect.Constructor;
 import java.nio.charset.Charset;
 import java.util.List;
+import java.util.Set;
+import java.util.TreeSet;
 
 /**
  * The parameters to add to a build.
@@ -218,6 +220,20 @@ public enum GerritTriggerParameters {
 
     private static final Logger logger = LoggerFactory.getLogger(GerritTriggerParameters.class);
 
+    /**
+     * A set of all the declared parameter names.
+     * @return the names of the parameters
+     * @see #values()
+     * @see #name()
+     */
+    public static Set<String> getNamesSet() {
+        Set<String> names = new TreeSet<String>();
+        for (GerritTriggerParameters p : GerritTriggerParameters.values()) {
+            names.add(p.name());
+        }
+        return names;
+    }
+
     /**
      * Creates a {@link hudson.model.ParameterValue} and adds it to the provided list.
      * If the parameter with the same name already exists in the list it will be replaced by the new parameter,

diff --git a/...sson/hudson/plugins/gerrit/trigger/gerritnotifier/ParameterExpanderParameterizedTest.java b/...sson/hudson/plugins/gerrit/trigger/gerritnotifier/ParameterExpanderParameterizedTest.java
@@ -30,10 +30,16 @@
 
 import hudson.model.Result;
 
+import jenkins.model.Jenkins;
+import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.Parameterized;
 import org.junit.runners.Parameterized.Parameters;
+import org.powermock.api.mockito.PowerMockito;
+import org.powermock.core.classloader.annotations.PrepareForTest;
+import org.powermock.modules.junit4.PowerMockRunner;
+import org.powermock.modules.junit4.PowerMockRunnerDelegate;
 
 import java.util.Collection;
 import java.util.LinkedList;
@@ -51,7 +57,9 @@
  * and {@link ParameterExpander#getVerifiedValue(hudson.model.Result, GerritTrigger)}
  * @author Robert Sandell &lt;robert.sandell@sonyericsson.com&gt;
  */
-@RunWith(Parameterized.class)
+@RunWith(PowerMockRunner.class)
+@PowerMockRunnerDelegate(Parameterized.class)
+@PrepareForTest(Jenkins.class)
 public class ParameterExpanderParameterizedTest {
 
     private TestParameters parameters;
@@ -64,6 +72,16 @@ public ParameterExpanderParameterizedTest(TestParameters parameters) {
         this.parameters = parameters;
     }
 
+    /**
+     * Mock Jenkins.
+     */
+    @Before
+    public void setup() {
+        PowerMockito.mockStatic(Jenkins.class);
+        Jenkins jenkins = PowerMockito.mock(Jenkins.class);
+        PowerMockito.when(Jenkins.getInstance()).thenReturn(jenkins);
+    }
+
     /**
      * test.
      */

diff --git a/.../hudson/plugins/gerrit/trigger/gerritnotifier/ParameterExpanderSkipVoteParameterTest.java b/.../hudson/plugins/gerrit/trigger/gerritnotifier/ParameterExpanderSkipVoteParameterTest.java
@@ -30,9 +30,15 @@
 
 import hudson.model.Result;
 
+import jenkins.model.Jenkins;
+import org.junit.Before;
 import org.junit.Test;
 import org.junit.runner.RunWith;
 import org.junit.runners.Parameterized;
+import org.powermock.api.mockito.PowerMockito;
+import org.powermock.core.classloader.annotations.PrepareForTest;
+import org.powermock.modules.junit4.PowerMockRunner;
+import org.powermock.modules.junit4.PowerMockRunnerDelegate;
 
 import java.util.Collection;
 import java.util.LinkedList;
@@ -51,7 +57,9 @@
  *
  * @author Robert Sandell &lt;robert.sandell@sonymobile.com&gt;
  */
-@RunWith(Parameterized.class)
+@RunWith(PowerMockRunner.class)
+@PowerMockRunnerDelegate(Parameterized.class)
+@PrepareForTest(Jenkins.class)
 public class ParameterExpanderSkipVoteParameterTest {
 
     private TestParameter parameter;
@@ -65,6 +73,16 @@ public ParameterExpanderSkipVoteParameterTest(TestParameter parameter) {
         this.parameter = parameter;
     }
 
+    /**
+     * Mock Jenkins.
+     */
+    @Before
+    public void setup() {
+        PowerMockito.mockStatic(Jenkins.class);
+        Jenkins jenkins = PowerMockito.mock(Jenkins.class);
+        PowerMockito.when(Jenkins.getInstance()).thenReturn(jenkins);
+    }
+
     /**
      * Tests that {@link ParameterExpander#getMinimumCodeReviewValue(BuildMemory.MemoryImprint, boolean)}
      * returns {@link TestParameter#expectedCodeReview}.