search

palantir / gerrit-ci

Plugin for Gerrit enabling self-service continuous integration workflows with Jenkins.
Apache License 2.0
18 stars 9 forks source link

Error saving config with commands that contain '&' #18

Open bluekeyes opened 8 years ago

bluekeyes commented 8 years ago

We discovered yesterday that trying to save a build with an '&' in the publish or verify command will cause a 500 error and not save the change. From the errors, it seems like the command is not properly escaped in the XML files.

The command in question was export FOO=bar && ./script/publish.sh, but any command with the '&' character should fail.

The Gerrit logs have this error:

[2015-11-10 18:00:40,033] ERROR com.google.gerrit.pgm.http.jetty.HiddenErrorHandler : Error in PUT /plugins/gerrit-ci/jobs/xxxxxxx
java.lang.RuntimeException: Failed to update Jenkins job: gerrit-ci_xxxxxxx
        at com.palantir.gerrit.gerritci.providers.JenkinsProvider.createOrUpdateJob(JenkinsProvider.java:105)
        at com.palantir.gerrit.gerritci.servlets.JobsServlet.doPut(JobsServlet.java:301)
        at javax.servlet.http.HttpServlet.service(HttpServlet.java:647)
        at javax.servlet.http.HttpServlet.service(HttpServlet.java:725)
        [...snip...]
Caused by: org.apache.http.client.HttpResponseException: Server Error
        at com.offbytwo.jenkins.client.validator.HttpResponseValidator.validateResponse(HttpResponseValidator.java:11)
        at com.offbytwo.jenkins.client.JenkinsHttpClient.post_xml(JenkinsHttpClient.java:237)
        at com.offbytwo.jenkins.JenkinsServer.updateJob(JenkinsServer.java:226)
        at com.palantir.gerrit.gerritci.providers.JenkinsProvider.createOrUpdateJob(JenkinsProvider.java:103)
        ... 46 more

The Jenkins Enterprise logs have this error:

[Fatal Error] :140:17: The entity name must immediately follow the '&' in the entity reference.
ERROR:  'The entity name must immediately follow the '&' in the entity reference.'
Nov 10, 2015 6:20:10 PM org.eclipse.jetty.util.log.JavaUtilLog warn
WARNING: Error while serving https://server.domain/job/GerritCI/job/gerrit-ci_xxxxxxx/config.xml/api/json
java.lang.reflect.InvocationTargetException
        at sun.reflect.GeneratedMethodAccessor2113.invoke(Unknown Source)
        at sun.reflect.DelegatingMethodAccessorImpl.invoke(DelegatingMethodAccessorImpl.java:43)
        at java.lang.reflect.Method.invoke(Method.java:497)
        at org.kohsuke.stapler.Function$InstanceFunction.invoke(Function.java:298)
        [...snip...]
Caused by: java.io.IOException: Failed to persist config.xml
        at hudson.model.AbstractItem.updateByXml(AbstractItem.java:644)
        at hudson.model.AbstractItem.doConfigDotXml(AbstractItem.java:612)
        ... 98 more
Caused by: javax.xml.transform.TransformerException: org.xml.sax.SAXParseException; lineNumber: 140; columnNumber: 17; The entity name must immediately follow the '&' in the entity reference.
        at com.sun.org.apache.xalan.internal.xsltc.trax.TransformerImpl.transform(TransformerImpl.java:755)
        at com.sun.org.apache.xalan.internal.xsltc.trax.TransformerImpl.transform(TransformerImpl.java:357)
        at jenkins.util.xml.XMLUtils._transform(XMLUtils.java:96)
        at jenkins.util.xml.XMLUtils.safeTransform(XMLUtils.java:63)
        at hudson.model.AbstractItem.updateByXml(AbstractItem.java:641)
        ... 99 more
Caused by: org.xml.sax.SAXParseException; lineNumber: 140; columnNumber: 17; The entity name must immediately follow the '&' in the entity reference.
        at com.sun.org.apache.xerces.internal.parsers.AbstractSAXParser.parse(AbstractSAXParser.java:1239)
        at com.sun.org.apache.xalan.internal.xsltc.trax.TransformerImpl.transformIdentity(TransformerImpl.java:674)
        at com.sun.org.apache.xalan.internal.xsltc.trax.TransformerImpl.transform(TransformerImpl.java:743)
        ... 103 more
bluekeyes commented 8 years ago

Hmm, I just tested this locally with the master version of the plugin and it worked fine. It seems like this was fixed already or is only a problem with Jenkins Enterprise.

mass commented 8 years ago

It seems like we are escaping & with & for the prebuild script (https://github.com/palantir/gerrit-ci/blob/master/src/main/java/com/palantir/gerrit/gerritci/servlets/JobsServlet.java#L478), but I can't find a place where the escaping happens for other input. I could be missing something, but I think it's likely that a similar escaping is needed for other input fields to avoid this error.

newren commented 8 years ago

As I just learned from doing a code review of kniktas work on stashbot, apparently we can do: <command>$esc.xml($globalPrebuildCommand)</command> in the velocity template rather than attempting to escape it ourselves. Might be more robust.

(How annoying -- and ironic -- is it that I have to manually escape the less-than and greather-than symbols in my own github comment?)