Skip to content

Commit

Permalink
[JENKINS-34858] - Listed Parameters should reflect what was used when…
Browse files Browse the repository at this point in the history 
… the build ran

And provided a way for plugins to define safe parameters
by extending ParametersAction
  • Loading branch information
@rsandell
rsandell committed May 16, 2016
1 parent 6ae6c2f commit 43f570f
Show file tree
Hide file tree
Showing 2 changed files with 155 additions and 9 deletions.
61 changes: 52 additions & 9 deletions core/src/main/java/hudson/model/ParametersAction.java
View file
Expand Up @@ -46,6 +46,7 @@
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;

Expand Down Expand Up @@ -74,7 +75,7 @@ public class ParametersAction implements RunAction2, Iterable<ParameterValue>, Q
public static final String SAFE_PARAMETERS_SYSTEM_PROPERTY_NAME = ParametersAction.class.getName() +
".safeParameters";

private transient List<String> safeParameters;
private Set<String> safeParameters;

private final List<ParameterValue> parameters;

Expand Down Expand Up @@ -203,7 +204,10 @@ public boolean shouldSchedule(List<Action> actions) {
@Nonnull
public ParametersAction createUpdated(Collection<? extends ParameterValue> overrides) {
if(overrides == null) {
return new ParametersAction(parameters);
ParametersAction parametersAction = new ParametersAction(parameters);
loadSafeParameters();
parametersAction.safeParameters = this.safeParameters;
return parametersAction;
}
List<ParameterValue> combinedParameters = newArrayList(overrides);
Set<String> names = newHashSet();
Expand All @@ -220,7 +224,10 @@ public ParametersAction createUpdated(Collection<? extends ParameterValue> overr
}
}

return new ParametersAction(combinedParameters);
ParametersAction parametersAction = new ParametersAction(combinedParameters);
loadSafeParameters();
parametersAction.safeParameters = this.safeParameters;
return parametersAction;
}

/*
Expand All @@ -231,9 +238,23 @@ public ParametersAction createUpdated(Collection<? extends ParameterValue> overr
@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<String> 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() {
Expand Down Expand Up @@ -302,15 +323,37 @@ public List<ParameterValue> getAllParameters() {
}

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<String> getAdditionalSafeParameters() {
return Collections.emptyList();
}

private static final Logger LOGGER = Logger.getLogger(ParametersAction.class.getName());
Expand Down
103 changes: 103 additions & 0 deletions test/src/test/java/hudson/model/ParametersActionTest2.java
View file
Expand Up @@ -10,6 +10,7 @@

import java.io.IOException;
import java.util.Arrays;
import java.util.Collection;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
Expand Down Expand Up @@ -149,6 +150,97 @@ public void whitelistedParameter() throws Exception {
}
}

@Test
@Issue("SECURITY-170")
public void whitelistedParameterByOverride() throws Exception {
FreeStyleProject p = j.createFreeStyleProject();
String name = p.getFullName();
p.addProperty(new ParametersDefinitionProperty(Arrays.<ParameterDefinition>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.<ParameterDefinition>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 {
Expand Down Expand Up @@ -194,6 +286,17 @@ public static boolean hasParameterWithName(Iterable<ParameterValue> values, Stri
return false;
}

public static class TestParametersAction extends ParametersAction {
public TestParametersAction(ParameterValue... parameters) {
super(parameters);
}

@Override
protected Collection<String> getAdditionalSafeParameters() {
return Arrays.asList("whitelisted1", "whitelisted2");
}
}

public static class ParametersCheckBuilder extends Builder {

private final boolean expectLegacyBehavior;
Expand Down

0 comments on commit 43f570f

Please sign in to comment.