Skip to content
Merged
Show file tree
Hide file tree
Changes from 2 commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
11 changes: 11 additions & 0 deletions src/main/java/jenkins/plugins/logstash/LogstashInstallation.java
Original file line number Diff line number Diff line change
Expand Up @@ -71,6 +71,7 @@ public static final class Descriptor extends ToolDescriptor<LogstashInstallation
public String username;
public String password;
public String key;
public String mimeType;

public Descriptor() {
super();
Expand Down Expand Up @@ -124,5 +125,15 @@ public FormValidation doCheckString(@QueryParameter("value") String value) {

return FormValidation.ok();
}
public FormValidation doCheckMimeType(@QueryParameter("value") String value) {
if (StringUtils.isBlank(value)) {
return FormValidation.error(Messages.ValueIsRequired());
}
// Mime type validation as per RFC-4288.
if (!value.matches("^[\\w#&+_$.\\-\\^]+/[\\w#&+_$.\\-\\^]+$")) {

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I did think of this earlier. But I did not end up using it in my code because I realized it does not actually parse Mime-Type. for e.g. ContentType.parse("application/json%%") (which is clearly not a valid mime-type) does not complain instead it returns ContentType object with this invalid mimetype.
For this reason I did not use it in the code and ended up validating by my own as per RFC-4288. This would potentially eliminate unwanted surprises that could cause with invalid mime-type.

Copy link

@jakub-bochenski jakub-bochenski Dec 28, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK, I assumed

    ParseException - if the given text does not represent a valid Content-Type value.

means it does validate it

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It just validates Content-Type as a whole and does not look for mime-type validity.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You could use javax.activation.MimeType
This does validate the mimetype. Though it seems & is a valid character in a mimetype. Mimetype fails when you use §
see RFC 2045

return FormValidation.error(Messages.ProvideValidMimeType());
}
return FormValidation.ok();
}
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -42,9 +42,15 @@
import java.io.PrintStream;
import java.net.URI;
import java.net.URISyntaxException;
import java.nio.charset.StandardCharsets;
import java.util.logging.Logger;

import com.google.common.collect.Range;

import jenkins.model.Jenkins;
import jenkins.plugins.logstash.LogstashInstallation;
import jenkins.plugins.logstash.LogstashInstallation.Descriptor;

/**
* Elastic Search Data Access Object.
*
Expand All @@ -56,7 +62,8 @@ public class ElasticSearchDao extends AbstractLogstashIndexerDao {
final URI uri;
final String auth;
final Range<Integer> successCodes = closedOpen(200,300);

private final static Logger log = Logger.getLogger(ElasticSearchDao.class.getName());

//primary constructor used by indexer factory
public ElasticSearchDao(String host, int port, String key, String username, String password) {
this(null, host, port, key, username, password);
Expand Down Expand Up @@ -94,9 +101,22 @@ public ElasticSearchDao(String host, int port, String key, String username, Stri
}

HttpPost getHttpPost(String data) {
HttpPost postRequest;
postRequest = new HttpPost(uri);
StringEntity input = new StringEntity(data, ContentType.APPLICATION_JSON);
String mimeType = null;
try {
Descriptor logstashPluginConfig = (Descriptor) Jenkins.getInstance().getDescriptor(LogstashInstallation.class);
mimeType = logstashPluginConfig.mimeType;
} catch (NullPointerException e) {
log.warning("Unable to read mimetype from jenkins logstash plugin configuration");
}
return getHttpPost(data, mimeType);
}
// Re-factored for unit-testing
HttpPost getHttpPost(String data, String mimeType) {
HttpPost postRequest = new HttpPost(uri);
// char encoding is set to UTF_8 since this request posts a JSON string
StringEntity input = new StringEntity(data, StandardCharsets.UTF_8);
mimeType = (mimeType != null) ? mimeType : ContentType.APPLICATION_JSON.toString();

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

mimeType will never be null. If you save the global configuration with no content for the mimeType you will get an empty string. Only when you update the plugin without ever saving the configuration it will be null.

input.setContentType(mimeType);
postRequest.setEntity(input);
if (auth != null) {
postRequest.addHeader("Authorization", "Basic " + auth);
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -22,6 +22,10 @@
checkUrl="'${resURL}/descriptorByName/LogstashInstallation/checkString?value='+escape(this.value)" />
</f:entry>
<f:advanced>
<f:entry title="${%MIME type}" field="mimeType">
<f:textbox value="${descriptor.mimeType}" default="application/json"

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not sure if the existing installations will get null or the default value.
Can we set the default to match the previous value?

Copy link
Author

@VamanKulkarni VamanKulkarni Dec 23, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I am not sure if I understand that correctly. Even in case of null, Content-Type will be set to ContentType.APPLICATION_JSON. Default value is no different then the previous one it is just that it is set in different way :) i.e. the default value is set to application/json and the String entity is initialized with charset=UTF-8.

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK I get it now.
Can we make it more explicit in the description that the mimeType input doesn't accept any parameters (incl. charset)?

checkUrl="'${resURL}/descriptorByName/LogstashInstallation/checkMimeType?value='+escape(this.value)" />
</f:entry>
<f:entry title="${%Syslog format}" field="syslogFormat">
<f:enum value="${descriptor.syslogFormat}">${it.name()}</f:enum>
</f:entry>
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
<div>
<p>
MIME type of the request body that is sent to ELASTICSEARCH indexer. It should be at least of the form <b>type/subtype</b><br>
For e.g. <i>application/json</i>
</p>
</div>
Original file line number Diff line number Diff line change
Expand Up @@ -24,3 +24,4 @@ DisplayName = Send console log to Logstash
ValueIsInt = Value must be an integer
ValueIsRequired = Value is required
PleaseProvideHost = Please set a valid host name
ProvideValidMimeType = Please provide valid MIME type. (e.g application/json)
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,6 @@ ElasticSearchDao createDao(String host, int port, String key, String username, S
public void before() throws Exception {
int port = (int) (Math.random() * 1000);
dao = createDao("http://localhost", port, "/jenkins/logstash", "username", "password");

when(mockClientBuilder.build()).thenReturn(mockHttpClient);
when(mockHttpClient.execute(any(HttpPost.class))).thenReturn(mockResponse);
when(mockResponse.getStatusLine()).thenReturn(mockStatusLine);
Expand Down Expand Up @@ -228,6 +227,22 @@ public void pushFailStatusCode() throws Exception {
e.getMessage().contains("Something bad happened.") && e.getMessage().contains("HTTP error code: 500"));
throw e;
}

}
@Test
public void getHttpPostSuccessWithUserInput() {
String json = "{ 'foo': 'bar' }";
dao = createDao("http://localhost", 8200, "/jenkins/logstash", "", "");
String mimeType = "application/json";
HttpPost post = dao.getHttpPost(json, mimeType);
HttpEntity entity = post.getEntity();
assertEquals("Content type do not match", mimeType, entity.getContentType().getValue());
}
@Test
public void getHttpPostWithFallbackInput() {
String json = "{ 'foo': 'bar' }";
dao = createDao("http://localhost", 8200, "/jenkins/logstash", "", "");
HttpPost post = dao.getHttpPost(json);
HttpEntity entity = post.getEntity();
assertEquals("Content type do not match", ContentType.APPLICATION_JSON.toString(), entity.getContentType().getValue());
}
}