Skip to content

Commit

Permalink
Browse files Browse the repository at this point in the history
[FIXED JENKINS-41518] Validate env var names for validity
Specifically for validity as Java identifiers - otherwise everything
'splodes. This was never a problem from the Jenkinsfile side, since
there'd be compilation errors before getting to validation if you used
an invalid identifier, but it *is* a problem coming from JSON.
  • Loading branch information
abayer committed Jan 27, 2017
1 parent 516c8e6 commit e3d0a94
Show file tree
Hide file tree
Showing 6 changed files with 74 additions and 50 deletions.
Expand Up @@ -50,6 +50,7 @@ import org.jenkinsci.plugins.structs.describable.DescribableParameter
import org.jenkinsci.plugins.workflow.steps.StepDescriptor

import javax.annotation.Nonnull
import javax.lang.model.SourceVersion

/**
* Class for validating various AST elements. Contains the error collector as well as caches for steps, models, etc.
Expand Down Expand Up @@ -126,6 +127,12 @@ class ModelValidatorImpl implements ModelValidator {
errorCollector.error(env, Messages.ModelValidatorImpl_NoEnvVars())
valid = false
}
env.variables.each { k, v ->
if (!SourceVersion.isIdentifier(k.key)) {
errorCollector.error(k, Messages.ModelValidatorImpl_InvalidIdentifierInEnv(k.key))
valid = false
}
}

return valid
}
Expand Down
Expand Up @@ -117,5 +117,5 @@ ModelValidatorImpl.MissingAgentParameter=Missing required parameter for agent ty
ModelValidatorImpl.InvalidAgentParameter=Invalid config option "{0}" for agent type "{1}". Valid config options are {2}
ModelValidatorImpl.UnknownWhenConditional=Unknown conditional {0}. Valid conditionals are: {1}
ModelValidatorImpl.MultipleAgentParameters=Multiple parameters are required for agent type "{0}" - {1}. Please use a block instead.

ModelValidatorImpl.InvalidIdentifierInEnv="{0}" is not a valid Java identifier and cannot be used for an environment variable.
ModelInterpreter.NoNodeContext=Attempted to execute a step that requires a node context while 'agent none' was specified. Be sure to specify your own 'node { ... }' blocks when using 'agent none'.
Expand Up @@ -23,6 +23,9 @@
*/
package org.jenkinsci.plugins.pipeline.modeldefinition;

import com.fasterxml.jackson.databind.JsonNode;
import com.github.fge.jsonschema.tree.SimpleJsonTree;
import com.github.fge.jsonschema.util.JsonLoader;
import org.codehaus.groovy.control.ErrorCollector;
import org.codehaus.groovy.control.Janitor;
import org.codehaus.groovy.control.MultipleCompilationErrorsException;
Expand All @@ -35,6 +38,9 @@
import java.io.StringWriter;
import java.net.URL;

import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertTrue;
import static org.junit.Assert.fail;

public abstract class BaseParserLoaderTest extends AbstractModelDefTest {
Expand Down Expand Up @@ -115,4 +121,26 @@ protected String write(ErrorCollector ec) {
}
}

protected void findErrorInJSON(String expectedError, String jsonName) throws Exception {
try {
JsonNode json = JsonLoader.fromString(fileContentsFromResources("json/errors/" + jsonName + ".json"));

assertNotNull("Couldn't parse JSON for " + jsonName, json);
assertFalse("Couldn't parse JSON for " + jsonName, json.size() == 0);
assertFalse("Couldn't parse JSON for " + jsonName, json.isNull());

JSONParser jp = new JSONParser(new SimpleJsonTree(json));
jp.parse();

assertTrue(jp.getErrorCollector().getErrorCount() > 0);

assertTrue("Didn't find expected error in " + getJSONErrorReport(jp, jsonName),
foundExpectedErrorInJSON(jp.getErrorCollector().asJson(), expectedError));
} catch (Exception e) {
// If there's a straight-up parsing error, make sure it's what we expect.
assertTrue(e.getMessage(), e.getMessage().contains(expectedError));
}

}

}
Expand Up @@ -23,20 +23,12 @@
*/
package org.jenkinsci.plugins.pipeline.modeldefinition.validator;

import com.fasterxml.jackson.databind.JsonNode;
import com.github.fge.jsonschema.tree.SimpleJsonTree;
import com.github.fge.jsonschema.util.JsonLoader;
import org.jenkinsci.plugins.pipeline.modeldefinition.AbstractModelDefTest;
import org.jenkinsci.plugins.pipeline.modeldefinition.BaseParserLoaderTest;
import org.jenkinsci.plugins.pipeline.modeldefinition.parser.JSONParser;
import org.junit.Test;
import org.junit.runner.RunWith;
import org.junit.runners.Parameterized;

import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertTrue;

@RunWith(Parameterized.class)
public class ErrorsJSONParserTest extends BaseParserLoaderTest {
private String configName;
Expand All @@ -54,23 +46,6 @@ public static Iterable<Object[]> generateParameters() {

@Test
public void parseAndValidateJSONWithError() throws Exception {
try {
JsonNode json = JsonLoader.fromString(fileContentsFromResources("json/errors/" + configName + ".json"));

assertNotNull("Couldn't parse JSON for " + configName, json);
assertFalse("Couldn't parse JSON for " + configName, json.size() == 0);
assertFalse("Couldn't parse JSON for " + configName, json.isNull());

JSONParser jp = new JSONParser(new SimpleJsonTree(json));
jp.parse();

assertTrue(jp.getErrorCollector().getErrorCount() > 0);

assertTrue("Didn't find expected error in " + getJSONErrorReport(jp, configName),
foundExpectedErrorInJSON(jp.getErrorCollector().asJson(), expectedError));
} catch (Exception e) {
// If there's a straight-up parsing error, make sure it's what we expect.
assertTrue(e.getMessage(), e.getMessage().contains(expectedError));
}
findErrorInJSON(expectedError, configName);
}
}
Expand Up @@ -23,19 +23,14 @@
*/
package org.jenkinsci.plugins.pipeline.modeldefinition.validator;

import com.fasterxml.jackson.databind.JsonNode;
import com.github.fge.jsonschema.tree.SimpleJsonTree;
import com.github.fge.jsonschema.util.JsonLoader;
import net.sf.json.JSONArray;
import net.sf.json.JSONObject;
import org.jenkinsci.plugins.pipeline.modeldefinition.BaseParserLoaderTest;
import org.jenkinsci.plugins.pipeline.modeldefinition.Messages;
import org.jenkinsci.plugins.pipeline.modeldefinition.ast.ModelASTPipelineDef;
import org.jenkinsci.plugins.pipeline.modeldefinition.parser.JSONParser;
import org.junit.Test;

import static org.junit.Assert.assertEquals;
import static org.junit.Assert.assertFalse;
import static org.junit.Assert.assertNotNull;
import static org.junit.Assert.assertTrue;

Expand Down Expand Up @@ -76,25 +71,12 @@ public void scriptBlockIsAString() throws Exception {

@Test
public void parallelPipelineDuplicateNames() throws Exception {
String expectedError = Messages.ModelValidatorImpl_DuplicateParallelName("first");
try {
JsonNode json = JsonLoader.fromString(fileContentsFromResources("json/errors/parallelPipelineDuplicateNames.json"));

assertNotNull("Couldn't parse JSON for parallelPipelineDuplicateNames", json);
assertFalse("Couldn't parse JSON for parallelPipelineDuplicateNames", json.size() == 0);
assertFalse("Couldn't parse JSON for parallelPipelineDuplicateNames", json.isNull());

JSONParser jp = new JSONParser(new SimpleJsonTree(json));
jp.parse();

assertTrue(jp.getErrorCollector().getErrorCount() > 0);
findErrorInJSON(Messages.ModelValidatorImpl_DuplicateParallelName("first"), "parallelPipelineDuplicateNames");
}

assertTrue("Didn't find expected error in " + getJSONErrorReport(jp, "parallelPipelineDuplicateNames"),
foundExpectedErrorInJSON(jp.getErrorCollector().asJson(), expectedError));
} catch (Exception e) {
// If there's a straight-up parsing error, make sure it's what we expect.
assertTrue(e.getMessage(), e.getMessage().contains(expectedError));
}
@Test
public void invalidIdentifierInEnv() throws Exception {
findErrorInJSON(Messages.ModelValidatorImpl_InvalidIdentifierInEnv("F OO"), "invalidIdentifierInEnv");
}

}
@@ -0,0 +1,32 @@
{"pipeline": {
"stages": [ {
"name": "foo",
"branches": [ {
"name": "default",
"steps": [ {
"name": "sh",
"arguments": [ {
"key": "script",
"value": {
"isLiteral": true,
"value": "echo \"FOO is $FOO\""
}
}]
}]
}]
}],
"environment": [ {
"key": "F OO",
"value": {
"isLiteral": true,
"value": "BAR"
}
}],
"agent": {
"type": "label",
"argument": {
"isLiteral": true,
"value": "some-label"
}
}
}}

0 comments on commit e3d0a94

Please sign in to comment.