diff --git a/core/src/main/java/hudson/model/ParametersAction.java b/core/src/main/java/hudson/model/ParametersAction.java index f62d11873d4565d17334c639c8c79bf3415e48ce..128457307db67050aff73556921957997e724e68 100644 --- a/core/src/main/java/hudson/model/ParametersAction.java +++ b/core/src/main/java/hudson/model/ParametersAction.java @@ -46,6 +46,7 @@ import java.util.HashSet; import java.util.Iterator; import java.util.List; import java.util.Set; +import java.util.TreeSet; import java.util.logging.Level; import java.util.logging.Logger; @@ -74,7 +75,7 @@ public class ParametersAction implements RunAction2, Iterable, Q public static final String SAFE_PARAMETERS_SYSTEM_PROPERTY_NAME = ParametersAction.class.getName() + ".safeParameters"; - private transient List safeParameters; + private Set safeParameters; private final List parameters; @@ -203,7 +204,10 @@ public class ParametersAction implements RunAction2, Iterable, Q @Nonnull public ParametersAction createUpdated(Collection overrides) { if(overrides == null) { - return new ParametersAction(parameters); + ParametersAction parametersAction = new ParametersAction(parameters); + loadSafeParameters(); + parametersAction.safeParameters = this.safeParameters; + return parametersAction; } List combinedParameters = newArrayList(overrides); Set names = newHashSet(); @@ -220,7 +224,10 @@ public class ParametersAction implements RunAction2, Iterable, Q } } - return new ParametersAction(combinedParameters); + ParametersAction parametersAction = new ParametersAction(combinedParameters); + loadSafeParameters(); + parametersAction.safeParameters = this.safeParameters; + return parametersAction; } /* @@ -231,9 +238,23 @@ public class ParametersAction implements RunAction2, Iterable, Q @Nonnull public ParametersAction merge(@CheckForNull ParametersAction overrides) { if (overrides == null) { - return new ParametersAction(parameters); + ParametersAction parametersAction = new ParametersAction(parameters); + loadSafeParameters(); + parametersAction.safeParameters = this.safeParameters; + return parametersAction; } - return createUpdated(overrides.parameters); + ParametersAction parametersAction = createUpdated(overrides.parameters); + Set safe = new TreeSet<>(); + if (parametersAction.safeParameters != null) { + //loadSafeParameters() should have been called by createUpdated + safe.addAll(this.safeParameters); + } + overrides.loadSafeParameters(); + if (overrides.safeParameters != null) { + safe.addAll(overrides.safeParameters); + } + parametersAction.safeParameters = safe; + return parametersAction; } private Object readResolve() { @@ -302,15 +323,37 @@ public class ParametersAction implements RunAction2, Iterable, Q } private boolean isSafeParameter(String name) { + loadSafeParameters(); + return safeParameters.contains(name); + } + + /** + * Combines the contents of {@link #SAFE_PARAMETERS_SYSTEM_PROPERTY_NAME} + * and {@link #getAdditionalSafeParameters()} into {@link #safeParameters}. + * @since TODO + */ + private void loadSafeParameters() { if (safeParameters == null) { String paramNames = SystemProperties.getString(SAFE_PARAMETERS_SYSTEM_PROPERTY_NAME); + safeParameters = new TreeSet<>(); if (paramNames != null) { - safeParameters = Arrays.asList(paramNames.split(",")); - } else { - safeParameters = Collections.emptyList(); + safeParameters.addAll(Arrays.asList(paramNames.split(","))); } + safeParameters.addAll(getAdditionalSafeParameters()); } - return safeParameters.contains(name); + } + + /** + * Provides a list of parameter names considered safe by the class overriding this action. + * Plugins can extend this when scheduling a build with the built in parameters it has. + * Whatever the user provides in {@link #SAFE_PARAMETERS_SYSTEM_PROPERTY_NAME} or + * {@link #KEEP_UNDEFINED_PARAMETERS_SYSTEM_PROPERTY_NAME} still counts. + * + * @return an additional list of safe parameter names + * @since TODO + */ + protected Collection getAdditionalSafeParameters() { + return Collections.emptyList(); } private static final Logger LOGGER = Logger.getLogger(ParametersAction.class.getName()); diff --git a/test/src/test/java/hudson/model/ParametersActionTest2.java b/test/src/test/java/hudson/model/ParametersActionTest2.java index df8590e48f6f3b916f8b06dc41f20ace00473d45..841a10190ec43ec56a7c380ce708cc19c91b2a16 100644 --- a/test/src/test/java/hudson/model/ParametersActionTest2.java +++ b/test/src/test/java/hudson/model/ParametersActionTest2.java @@ -10,6 +10,7 @@ import org.jvnet.hudson.test.recipes.LocalData; import java.io.IOException; import java.util.Arrays; +import java.util.Collection; import static org.junit.Assert.assertEquals; import static org.junit.Assert.assertFalse; @@ -149,6 +150,97 @@ public class ParametersActionTest2 { } } + @Test + @Issue("SECURITY-170") + public void whitelistedParameterByOverride() throws Exception { + FreeStyleProject p = j.createFreeStyleProject(); + String name = p.getFullName(); + p.addProperty(new ParametersDefinitionProperty(Arrays.asList( + new StringParameterDefinition("foo", "foo"), + new StringParameterDefinition("bar", "bar")))); + + try { + ParametersAction action = new TestParametersAction( + new StringParameterValue("foo", "baz"), + new StringParameterValue("bar", "bar"), + new StringParameterValue("whitelisted1", "x"), + new StringParameterValue("whitelisted2", "y"), + new StringParameterValue("whitelisted3", "y")); + FreeStyleBuild build = j.assertBuildStatusSuccess(p.scheduleBuild2(0, new Cause.UserIdCause(), action)); + + assertTrue("whitelisted1 parameter is listed in getParameters", + hasParameterWithName(build.getAction(ParametersAction.class), "whitelisted1")); + assertTrue("whitelisted2 parameter is listed in getParameters", + hasParameterWithName(build.getAction(ParametersAction.class), "whitelisted2")); + assertFalse("whitelisted3 parameter is listed in getParameters", + hasParameterWithName(build.getAction(ParametersAction.class), "whitelisted3")); + p = null; + build = null; + j.jenkins.reload(); + //Test again after reload + p = j.jenkins.getItemByFullName(name, FreeStyleProject.class); + build = p.getLastBuild(); + assertTrue("whitelisted1 parameter is listed in getParameters", + hasParameterWithName(build.getAction(ParametersAction.class), "whitelisted1")); + assertTrue("whitelisted2 parameter is listed in getParameters", + hasParameterWithName(build.getAction(ParametersAction.class), "whitelisted2")); + assertFalse("whitelisted3 parameter is listed in getParameters", + hasParameterWithName(build.getAction(ParametersAction.class), "whitelisted3")); + } finally { + System.clearProperty(ParametersAction.SAFE_PARAMETERS_SYSTEM_PROPERTY_NAME); + } + } + + @Test + @Issue("SECURITY-170") + public void whitelistedParameterSameAfterChange() throws Exception { + FreeStyleProject p = j.createFreeStyleProject(); + String name = p.getFullName(); + p.addProperty(new ParametersDefinitionProperty(Arrays.asList( + new StringParameterDefinition("foo", "foo"), + new StringParameterDefinition("bar", "bar")))); + + try { + System.setProperty(ParametersAction.SAFE_PARAMETERS_SYSTEM_PROPERTY_NAME, "whitelisted1,whitelisted2"); + FreeStyleBuild build = j.assertBuildStatusSuccess(p.scheduleBuild2(0, new Cause.UserIdCause(), new ParametersAction( + new StringParameterValue("foo", "baz"), + new StringParameterValue("bar", "bar"), + new StringParameterValue("whitelisted1", "x"), + new StringParameterValue("whitelisted2", "y"), + new StringParameterValue("whitelisted3", "z"), + new StringParameterValue("whitelisted4", "w") + ))); + assertTrue("whitelisted1 parameter is listed in getParameters", + hasParameterWithName(build.getAction(ParametersAction.class), "whitelisted1")); + assertTrue("whitelisted2 parameter is listed in getParameters", + hasParameterWithName(build.getAction(ParametersAction.class), "whitelisted2")); + assertFalse("whitelisted3 parameter is listed in getParameters", + hasParameterWithName(build.getAction(ParametersAction.class), "whitelisted3")); + assertFalse("whitelisted4 parameter is listed in getParameters", + hasParameterWithName(build.getAction(ParametersAction.class), "whitelisted4")); + + System.setProperty(ParametersAction.SAFE_PARAMETERS_SYSTEM_PROPERTY_NAME, "whitelisted3,whitelisted4"); + p = null; + build = null; + j.jenkins.reload(); + p = j.jenkins.getItemByFullName(name, FreeStyleProject.class); + build = p.getLastBuild(); + assertTrue("whitelisted1 parameter is listed in getParameters", + hasParameterWithName(build.getAction(ParametersAction.class), "whitelisted1")); + assertTrue("whitelisted2 parameter is listed in getParameters", + hasParameterWithName(build.getAction(ParametersAction.class), "whitelisted2")); + assertFalse("whitelisted3 parameter is listed in getParameters", + hasParameterWithName(build.getAction(ParametersAction.class), "whitelisted3")); + assertFalse("whitelisted4 parameter is listed in getParameters", + hasParameterWithName(build.getAction(ParametersAction.class), "whitelisted4")); + + } finally { + System.clearProperty(ParametersAction.SAFE_PARAMETERS_SYSTEM_PROPERTY_NAME); + } + } + + + @Test @Issue("SECURITY-170") public void nonParameterizedJob() throws Exception { @@ -194,6 +286,17 @@ public class ParametersActionTest2 { return false; } + public static class TestParametersAction extends ParametersAction { + public TestParametersAction(ParameterValue... parameters) { + super(parameters); + } + + @Override + protected Collection getAdditionalSafeParameters() { + return Arrays.asList("whitelisted1", "whitelisted2"); + } + } + public static class ParametersCheckBuilder extends Builder { private final boolean expectLegacyBehavior;