escape double quotes in env variables under windows

[schribl]

Problem description:
For Windows there is a problem with Javas ProcessBuilder and arguments that contain spaces and contain double quotes as ProcessBuilder adds double quotes around such argument.
See e.g. https://bugs.java.com/bugdatabase/view_bug.do?bug_id=6511002 and https://stackoverflow.com/questions/12124935/processbuilder-adds-extra-quotes-to-command-line

Solution:
For Windows all environment variables (only values) are escaped (currently only escapes quotes). No changes made to the Linux (and others) code as I could only reproduce this behavior with a Windows node.

Notes:

• This is nearly the same as Escape environment variables before pass it to docker run #215 but does not add additional quotes around every environment variable value and also applies this do Windows only.
• I build this changes locally and tested it in some Jenkins test instance and for my minimal testcases it resolved the issue and did not introduce new issues.

[car-roll]

bouncing to run against updated CI

[schribl]

Can I help the process with getting this in, like providing more details/documentation or adding tests for such special characters in environment variables?

[car-roll]

Hi @schribl if you could add some tests for the special characters that would be great!

[schribl]

HI @car-roll I tried implementing some tests for this change (escaping arguments for Windows). But a real test would require the CI to have windows nodes with docker installed IMO.

Doing some other tests require changes of the DockerClient design and will test implementation details and not the real function.

So I do not think this can really be tested without changes to the infrastructure or at least I do not see a good way for doing so.

[car-roll]

Hi @schribl I think what you have is fine as is then. I am curious, do you know if the changes in #215 solve your issue?

[car-roll]

So the biggest concern is if this change breaks some other use cases. If you look at this method https://github.com/jenkinsci/jenkins/blob/449c5aced523a6e66fe3d6a804e5dbfd5c5c67c6/core/src/main/java/hudson/os/WindowsUtil.java#L53 you can see a bit of checking that goes on here. It might be possible to replace escapeQuotes with this WindowsUtil.quoteArgument method and see how it handles your local test cases. At the very least, we probably shouldn't do anything if there are no spaces in the argument.

Would you happen to have a few examples that this PR is addressing?

Thanks @dwnusbaum for the link to the code and basically this response.

[schribl]

@car-roll @dwnusbaum Thank you! The link indeed looks good and I can understand the concern to break something else with this change.

I will do some more tests and provide the feedback including the results for WindowsUtil.quoteArgument. When I did the change back then I think I tried #215 but it did not work as the change was in DockerClient and not WindowsDockerClient and did not work for this reason (I remember trying both build with some test Jenkins setup).

A simple example I tested with was:

withEnv(['TEST="test test2"']) {
    println(env.TEST)
    docker.image("mcr.microsoft.com/dotnet/framework/runtime:4.8").inside() {
        println(env.TEST)
    }
}

And for the background what use case is affected: We are using GitLab and when the job is triggered the GitLab Plugin sets environment variables (including MR description). And it often occurs that quotes are used inside the MR description and this will result in job failures with "strange" error messages.

[schribl]

I did some more tests and can now confirm that #215 did not resolve the issue as it does change DockerClient and not WindowsDockerClient. With the link to WindowsUtil I added the .java file to the src/main/java/hudson/os folder and changed the code to:

if (variable.getValue().contains(" ") {
    argb.addMasked(WindowsUtil.quoteArgument(variable.getKey() + "=" + variable.getValue()));
} else {
    argb.addMasked(variable.getKey() + "=" + variable.getValue());
}

This also worked for my simple test case but only quoting the value did not work as the function adds quotes around the string and as a result has to quote the full argument. I also think the if condition should also check if the key contains a space but this I did not test yet.

@car-roll Is there a better way then copying the WindowsUtil.java to the plugin itself?

[car-roll]

@schribl yes, in this case you will have to bump the jenkins version in the pom from 2.176.4 to 2.190.1 (you might have to resolve some dependencies in that case). WindowsUtil was not introduced until after 2.176.4. Adding the space for the if condition sounds good as well.

update: just a quick correction, apparently WindowsUtil was in 2.176.4, it just lived in the test folder. Still probably better to just update the jenkins version instead of pulling in any test dependencies from core.

[schribl]

Thanks @car-roll I did the changes for the proper update and it is using WindowUtil now if there are spaces in either key or value of the variable.

[schribl]

@dwnusbaum Can you check the changes I made regarding your review comments? Thank you!

[schribl]

@car-roll and @dwnusbaum Friendly reminder on this PR :)