Skip to content

Commit

Permalink
Merge pull request #2353 from rsandell/safe-parameters
Browse files Browse the repository at this point in the history 
[JENKINS-34858] - Listed Parameters should reflect what was used when the build ran
  • Loading branch information
@rsandell
rsandell committed May 19, 2016
2 parents d08eec1 + c3942a1 commit 74d0412
Show file tree
Hide file tree
Showing 2 changed files with 140 additions and 13 deletions.
57 changes: 44 additions & 13 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 All @@ -90,6 +91,29 @@ public class ParametersAction implements RunAction2, Iterable<ParameterValue>, Q

public ParametersAction(List<ParameterValue> parameters) {
this.parameters = parameters;
String paramNames = SystemProperties.getString(SAFE_PARAMETERS_SYSTEM_PROPERTY_NAME);
safeParameters = new TreeSet<>();
if (paramNames != null) {
safeParameters.addAll(Arrays.asList(paramNames.split(",")));
}
}

/**
* Constructs a new action with additional safe parameters.
* The additional safe parameters should be only those considered safe to override the environment
* and what is declared in the project config in addition to those specified by the user in
* {@link #SAFE_PARAMETERS_SYSTEM_PROPERTY_NAME}.
* See <a href="https://issues.jenkins-ci.org/browse/SECURITY-170">SECURITY-170</a>
*
* @param parameters the parameters
* @param additionalSafeParameters additional safe parameters
* @since TODO

This comment has been minimized.

Sign in to view

Copy link
@jtnord

jtnord Jun 1, 2016

Member

this was never updated after the merge.
*/
public ParametersAction(List<ParameterValue> parameters, Collection<String> additionalSafeParameters) {
this(parameters);
if (additionalSafeParameters != null) {
safeParameters.addAll(additionalSafeParameters);
}
}

public ParametersAction(ParameterValue... parameters) {
Expand Down Expand Up @@ -203,7 +227,9 @@ 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);
parametersAction.safeParameters = this.safeParameters;
return parametersAction;
}
List<ParameterValue> combinedParameters = newArrayList(overrides);
Set<String> names = newHashSet();
Expand All @@ -220,7 +246,7 @@ public ParametersAction createUpdated(Collection<? extends ParameterValue> overr
}
}

return new ParametersAction(combinedParameters);
return new ParametersAction(combinedParameters, this.safeParameters);
}

/*
Expand All @@ -231,14 +257,27 @@ 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, this.safeParameters);
return parametersAction;
}
return createUpdated(overrides.parameters);
ParametersAction parametersAction = createUpdated(overrides.parameters);
Set<String> safe = new TreeSet<>();
if (parametersAction.safeParameters != null && this.safeParameters != null) {
safe.addAll(this.safeParameters);
}
if (overrides.safeParameters != null) {
safe.addAll(overrides.safeParameters);
}
parametersAction.safeParameters = safe;
return parametersAction;
}

private Object readResolve() {
if (build != null)
OldDataMonitor.report(build, "1.283");
if (safeParameters == null) {
safeParameters = Collections.emptySet();
}
return this;
}

Expand Down Expand Up @@ -302,14 +341,6 @@ public List<ParameterValue> getAllParameters() {
}

private boolean isSafeParameter(String name) {
if (safeParameters == null) {
String paramNames = SystemProperties.getString(SAFE_PARAMETERS_SYSTEM_PROPERTY_NAME);
if (paramNames != null) {
safeParameters = Arrays.asList(paramNames.split(","));
} else {
safeParameters = Collections.emptyList();
}
}
return safeParameters.contains(name);
}

Expand Down
96 changes: 96 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,100 @@ 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 ParametersAction(
Arrays.<ParameterValue>asList(
new StringParameterValue("foo", "baz"),
new StringParameterValue("bar", "bar"),
new StringParameterValue("whitelisted1", "x"),
new StringParameterValue("whitelisted2", "y"),
new StringParameterValue("whitelisted3", "y")
),
Arrays.asList("whitelisted1", "whitelisted2"));
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 +289,7 @@ public static boolean hasParameterWithName(Iterable<ParameterValue> values, Stri
return false;
}


public static class ParametersCheckBuilder extends Builder {

private final boolean expectLegacyBehavior;
Expand Down

0 comments on commit 74d0412

Please sign in to comment.