Skip to content

Commit

Permalink
[JENKINS-42640] Fix validation of wrappers and some types
Browse files Browse the repository at this point in the history 
In validating the options contents, we didn't actually look for a
StepDescriptor, just a symbol-driven Descriptor. Which won't work for
steps! So we never actually validated the contents of wrappers in
options. Besides that, we needed to reject converting to int from
String/Boolean - that gets through ScriptByteCodeAdapter.castToType,
but not runtime.
  • Loading branch information
@abayer
abayer committed Mar 10, 2017
1 parent 741fc9f commit 2b20c58
Show file tree
Hide file tree
Showing 9 changed files with 88 additions and 28 deletions.
24 changes: 22 additions & 2 deletions ...i/src/main/java/org/jenkinsci/plugins/pipeline/modeldefinition/DescriptorLookupCache.java
View file
Expand Up @@ -112,11 +112,15 @@ public synchronized Descriptor<? extends Describable> lookupFunction(String n) {
return describableMap.get(n);
}

public synchronized Descriptor<? extends Describable> lookupStepOrFunction(String name) {
public synchronized Descriptor<? extends Describable> lookupStepFirstThenFunction(String name) {
return lookupStepDescriptor(name) != null ? lookupStepDescriptor(name) : lookupFunction(name);
}

public synchronized DescribableModel<? extends Describable> modelForStepOrFunction(String name) {
public synchronized Descriptor<? extends Describable> lookupFunctionFirstThenStep(String name) {
return lookupFunction(name) != null ? lookupFunction(name) : lookupStepDescriptor(name);
}

public synchronized DescribableModel<? extends Describable> modelForStepFirstThenFunction(String name) {
Descriptor<? extends Describable> desc = lookupStepDescriptor(name);
DescribableModel<? extends Describable> model = null;

Expand All @@ -132,6 +136,22 @@ public synchronized DescribableModel<? extends Describable> modelForStepOrFuncti
return model;
}

public synchronized DescribableModel<? extends Describable> modelForFunctionFirstThenStep(String name) {
Descriptor<? extends Describable> desc = lookupFunction(name);
DescribableModel<? extends Describable> model = null;

if (desc != null) {
model = modelForDescribable(name);
} else {
desc = lookupStepDescriptor(name);
if (desc != null) {
model = modelForStep(name);
}
}

return model;
}

public boolean stepTakesClosure(Descriptor d) {
if (d instanceof StepDescriptor) {
return ((StepDescriptor)d).takesImplicitBlockArgument();
Expand Down
4 changes: 2 additions & 2 deletions ...el-api/src/main/java/org/jenkinsci/plugins/pipeline/modeldefinition/ast/ModelASTStep.java
View file
Expand Up @@ -73,8 +73,8 @@ public String toGroovy() {
// If the named list is exactly 1 long...
if (namedArgs.getArguments().size() == 1) {
DescriptorLookupCache lookup = DescriptorLookupCache.getPublicCache();
Descriptor<? extends Describable> desc = lookup.lookupStepOrFunction(name);
DescribableModel<? extends Describable> model = lookup.modelForStepOrFunction(name);
Descriptor<? extends Describable> desc = lookup.lookupStepFirstThenFunction(name);
DescribableModel<? extends Describable> model = lookup.modelForStepFirstThenFunction(name);

// If we can lookup the model for this step or function...
if (model != null) {
Expand Down
4 changes: 2 additions & 2 deletions .../src/main/groovy/org/jenkinsci/plugins/pipeline/modeldefinition/parser/ModelParser.groovy
View file
Expand Up @@ -711,8 +711,8 @@ class ModelParser implements Parser {
if (Jenkins.getInstance() != null && origArgs instanceof ModelASTSingleArgument) {
ModelASTValue singleArgValue = ((ModelASTSingleArgument)origArgs).value
ModelASTNamedArgumentList namedArgs = new ModelASTNamedArgumentList(origArgs.sourceLocation)
Descriptor<? extends Describable> desc = lookup.lookupStepOrFunction(step.name)
DescribableModel<? extends Describable> model = lookup.modelForStepOrFunction(step.name)
Descriptor<? extends Describable> desc = lookup.lookupStepFirstThenFunction(step.name)
DescribableModel<? extends Describable> model = lookup.modelForStepFirstThenFunction(step.name)

if (model != null) {
DescribableParameter p = model.soleRequiredParameter
Expand Down
26 changes: 11 additions & 15 deletions ...groovy/org/jenkinsci/plugins/pipeline/modeldefinition/validator/ModelValidatorImpl.groovy
View file
Expand Up @@ -334,8 +334,8 @@ class ModelValidatorImpl implements ModelValidator {
} else {
// We can't do step validation without a Jenkins instance, so move on.
if (Jenkins.getInstance() != null) {
Descriptor desc = lookup.lookupStepOrFunction(step.name)
DescribableModel<? extends Describable> model = lookup.modelForStepOrFunction(step.name)
Descriptor desc = lookup.lookupStepFirstThenFunction(step.name)
DescribableModel<? extends Describable> model = lookup.modelForStepFirstThenFunction(step.name)

if (model != null) {
valid = validateStep(step, model, desc)
Expand All @@ -354,10 +354,10 @@ class ModelValidatorImpl implements ModelValidator {
valid = false
}
if (Jenkins.getInstance() != null) {
Descriptor desc = lookup.lookupFunction(meth.name)
Descriptor desc = lookup.lookupFunctionFirstThenStep(meth.name)
DescribableModel<? extends Describable> model
if (desc != null) {
model = lookup.modelForDescribable(meth.name)
model = lookup.modelForFunctionFirstThenStep(meth.name)
}

if (model != null) {
Expand Down Expand Up @@ -448,8 +448,6 @@ class ModelValidatorImpl implements ModelValidator {
&& !trig.args.every { it instanceof ModelASTKeyValueOrMethodCallPair }) {
errorCollector.error(trig, Messages.ModelValidatorImpl_MixedNamedAndUnnamedParameters())
valid = false
} else {
valid = validateElement((ModelASTMethodCall)trig)
}
return valid
}
Expand Down Expand Up @@ -485,8 +483,6 @@ class ModelValidatorImpl implements ModelValidator {
&& !param.args.every { it instanceof ModelASTKeyValueOrMethodCallPair }) {
errorCollector.error(param, Messages.ModelValidatorImpl_MixedNamedAndUnnamedParameters())
valid = false
} else {
valid = validateElement((ModelASTMethodCall)param)
}
return valid
}
Expand Down Expand Up @@ -515,26 +511,26 @@ class ModelValidatorImpl implements ModelValidator {
&& !opt.args.every { it instanceof ModelASTKeyValueOrMethodCallPair }) {
errorCollector.error(opt, Messages.ModelValidatorImpl_MixedNamedAndUnnamedParameters())
valid = false
} else {
valid = validateElement((ModelASTMethodCall)opt)
}
return valid
}

private boolean validateParameterType(ModelASTValue v, Class erasedType, ModelASTKey k = null) {
if (v.isLiteral()) {
try {
// Converting from boolean or int to string at runtime doesn't work, but does pass castToType. So.
if (erasedType.equals(String.class)
&& (v.value instanceof Integer || v.value instanceof Boolean)) {
// Converting amongst boolean, string and int at runtime doesn't work, but does pass castToType. So.
if ((erasedType.equals(String.class) && (v.value instanceof Integer || v.value instanceof Boolean)) ||
(erasedType.equals(int.class) && (v.value instanceof String || v.value instanceof Boolean))) {
throw new RuntimeException("Ignore")
}
ScriptBytecodeAdapter.castToType(v.value, erasedType);
} catch (Exception e) {
if (k != null) {
errorCollector.error(v, Messages.ModelValidatorImpl_InvalidParameterType(erasedType, k.key, v.value.toString()))
errorCollector.error(v, Messages.ModelValidatorImpl_InvalidParameterType(erasedType, k.key, v.value.toString(),
v.value.getClass()))
} else {
errorCollector.error(v, Messages.ModelValidatorImpl_InvalidUnnamedParameterType(erasedType, v.value.toString()))
errorCollector.error(v, Messages.ModelValidatorImpl_InvalidUnnamedParameterType(erasedType, v.value.toString(),
v.value.getClass()))
}
return false
}
Expand Down
4 changes: 2 additions & 2 deletions ...ion/src/main/resources/org/jenkinsci/plugins/pipeline/modeldefinition/Messages.properties
View file
Expand Up @@ -106,8 +106,8 @@ ModelValidatorImpl.WrongNumberOfStepParameters="{0}" should have {1} arguments b
ModelValidatorImpl.NotSingleRequiredParameter=Step does not take a single required parameter - use named parameters instead
ModelValidatorImpl.MixedNamedAndUnnamedParameters=Can't mix named and unnamed arguments
ModelValidatorImpl.WrongBuildParameterType=Invalid type for parameter "{0}"
ModelValidatorImpl.InvalidParameterType=Expecting "{0}" for parameter "{1}" but got "{2}" instead
ModelValidatorImpl.InvalidUnnamedParameterType=Expecting "{0}" but got "{1}" instead
ModelValidatorImpl.InvalidParameterType=Expecting "{0}" for parameter "{1}" but got "{2}" of type {3} instead
ModelValidatorImpl.InvalidUnnamedParameterType=Expecting "{0}" but got "{1}" of type {2} instead
ModelValidatorImpl.NoSteps=No steps specified for branch
ModelValidatorImpl.RequiredSection=Missing required section "{0}"
ModelValidatorImpl.NoStageName=Stage does not have a name
Expand Down
4 changes: 2 additions & 2 deletions ...on/src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/AbstractModelDefTest.java
View file
Expand Up @@ -176,7 +176,7 @@ public static Iterable<Object[]> configsWithErrors() {
Messages.ModelValidatorImpl_BlockedStep("properties", ModelASTStep.getBlockedSteps().get("properties"))});

result.add(new Object[]{"wrongParameterNameMethodCall", Messages.ModelValidatorImpl_InvalidStepParameter("namd", "name")});
result.add(new Object[]{"invalidParameterTypeMethodCall", Messages.ModelValidatorImpl_InvalidParameterType("class java.lang.String", "name", "1234")});
result.add(new Object[]{"invalidParameterTypeMethodCall", Messages.ModelValidatorImpl_InvalidParameterType("class java.lang.String", "name", "1234", Integer.class)});

result.add(new Object[]{"perStageConfigEmptySteps", Messages.JSONParser_TooFewItems(0, 1)});
result.add(new Object[]{"perStageConfigMissingSteps", Messages.JSONParser_MissingRequiredProperties("'steps'")});
Expand All @@ -191,7 +191,7 @@ public static Iterable<Object[]> configsWithErrors() {
result.add(new Object[]{"notificationsSectionRemoved", "additional properties are not allowed"});
result.add(new Object[]{"unknownWhenConditional", Messages.ModelValidatorImpl_UnknownWhenConditional("banana",
"allOf, anyOf, branch, environment, expression, not")});
result.add(new Object[]{"whenInvalidParameterType", Messages.ModelValidatorImpl_InvalidUnnamedParameterType("class java.lang.String", 4)});
result.add(new Object[]{"whenInvalidParameterType", Messages.ModelValidatorImpl_InvalidUnnamedParameterType("class java.lang.String", 4, Integer.class)});
result.add(new Object[]{"whenMissingRequiredParameter", Messages.ModelValidatorImpl_MissingRequiredStepParameter("value")});
result.add(new Object[]{"whenUnknownParameter", Messages.ModelValidatorImpl_InvalidStepParameter("banana", "name")});

Expand Down
6 changes: 3 additions & 3 deletions ...efinition/src/test/java/org/jenkinsci/plugins/pipeline/modeldefinition/ValidatorTest.java
View file
Expand Up @@ -102,7 +102,7 @@ public void emptyTriggers() throws Exception {
@Test
public void whenInvalidParameterType() throws Exception {
expectError("whenInvalidParameterType")
.logContains(Messages.ModelValidatorImpl_InvalidUnnamedParameterType("class java.lang.String", 4))
.logContains(Messages.ModelValidatorImpl_InvalidUnnamedParameterType("class java.lang.String", 4, Integer.class))
.go();
}

Expand Down Expand Up @@ -196,7 +196,7 @@ public void wrongParameterNameMethodCall() throws Exception {
@Test
public void invalidParameterTypeMethodCall() throws Exception {
expectError("invalidParameterTypeMethodCall")
.logContains(Messages.ModelValidatorImpl_InvalidParameterType("class java.lang.String", "name", "1234"))
.logContains(Messages.ModelValidatorImpl_InvalidParameterType("class java.lang.String", "name", "1234", Integer.class))
.go();
}

Expand Down Expand Up @@ -259,7 +259,7 @@ public void missingRequiredStepParameters() throws Exception {
@Test
public void invalidStepParameterType() throws Exception {
expectError("invalidStepParameterType")
.logContains(Messages.ModelValidatorImpl_InvalidParameterType("int", "time", "someTime"))
.logContains(Messages.ModelValidatorImpl_InvalidParameterType("int", "time", "someTime", String.class))
.go();
}

Expand Down
6 changes: 6 additions & 0 deletions ...est/java/org/jenkinsci/plugins/pipeline/modeldefinition/validator/JSONValidationTest.java
View file
Expand Up @@ -29,6 +29,7 @@
import org.jenkinsci.plugins.pipeline.modeldefinition.Messages;
import org.jenkinsci.plugins.pipeline.modeldefinition.ast.ModelASTPipelineDef;
import org.junit.Test;
import org.jvnet.hudson.test.Issue;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertNotNull;
Expand Down Expand Up @@ -80,4 +81,9 @@ public void invalidIdentifierInEnv() throws Exception {
findErrorInJSON(Messages.ModelValidatorImpl_InvalidIdentifierInEnv("F$OO"), "invalidIdentifierInEnv");
}

@Issue("JENKINS-42640")
@Test
public void jsonParameterTypeCoercion() throws Exception {
findErrorInJSON(Messages.ModelValidatorImpl_InvalidParameterType(int.class, "time", "5", String.class), "jsonParameterTypeCoercion");
}
}
38 changes: 38 additions & 0 deletions pipeline-model-definition/src/test/resources/json/errors/jsonParameterTypeCoercion.json
View file
@@ -0,0 +1,38 @@
{"pipeline": {
"stages": [ {
"name": "foo",
"branches": [ {
"name": "default",
"steps": [ {
"name": "echo",
"arguments": [ {
"key": "message",
"value": {
"isLiteral": true,
"value": "hello"
}
}]
}]
}]
}],
"agent": {"type": "none"},
"options": {"options": [ {
"name": "timeout",
"arguments": [
{
"key": "time",
"value": {
"isLiteral": true,
"value": "5"
}
},
{
"key": "unit",
"value": {
"isLiteral": true,
"value": "MINUTES"
}
}
]
}]}
}}

0 comments on commit 2b20c58

Please sign in to comment.