Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Watcher reporting: add email warning if CSV attachment contains values that may be interperted as formulas #44460

Merged
merged 6 commits into from
Aug 14, 2019
Merged

Watcher reporting: add email warning if CSV attachment contains values that may be interperted as formulas #44460

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
6 commits
Select commit Hold shift + click to select a range
ecb394b
initial commit for watcher reporting csv email warning
jakelandis Jul 16, 2019
024ef74
Merge branch 'master' into watcher_reporting_email_warning
jakelandis Aug 7, 2019
0acff82
add warning support for no body
jakelandis Aug 7, 2019
dea3635
Merge branch 'master' into watcher_reporting_email_warning
jakelandis Aug 14, 2019
ed5128f
add assert not null
jakelandis Aug 14, 2019
6261d53
fix formatting
jakelandis Aug 14, 2019
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
2 changes: 1 addition & 1 deletion x-pack/plugin/watcher/build.gradle
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -44,7 +44,7 @@ dependencies {

testCompile 'org.subethamail:subethasmtp:3.1.7'
// needed for subethasmtp, has @GuardedBy annotation
testCompile 'com.google.code.findbugs:jsr305:3.0.1'
testCompile 'com.google.code.findbugs:jsr305:3.0.2'
Copy link
Contributor Author

@jakelandis jakelandis Jul 16, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

this is to avoid JAR hell when running tests via IntelliJ (nothing specific to the changes here)

}

// classes are missing, e.g. com.ibm.icu.lang.UCharacter
Expand Down
6 changes: 3 additions & 3 deletions x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/Watcher.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -284,7 +284,8 @@ public Collection<Object> createComponents(Client client, ClusterService cluster
Map<String, EmailAttachmentParser> emailAttachmentParsers = new HashMap<>();
emailAttachmentParsers.put(HttpEmailAttachementParser.TYPE, new HttpEmailAttachementParser(httpClient, templateEngine));
emailAttachmentParsers.put(DataAttachmentParser.TYPE, new DataAttachmentParser());
emailAttachmentParsers.put(ReportingAttachmentParser.TYPE, new ReportingAttachmentParser(settings, httpClient, templateEngine));
emailAttachmentParsers.put(ReportingAttachmentParser.TYPE,
new ReportingAttachmentParser(settings, httpClient, templateEngine, clusterService.getClusterSettings()));
EmailAttachmentsParser emailAttachmentsParser = new EmailAttachmentsParser(emailAttachmentParsers);

// conditions
Expand Down Expand Up @@ -470,8 +471,7 @@ public List<Setting<?>> getSettings() {
settings.addAll(HtmlSanitizer.getSettings());
settings.addAll(JiraService.getSettings());
settings.addAll(PagerDutyService.getSettings());
settings.add(ReportingAttachmentParser.RETRIES_SETTING);
settings.add(ReportingAttachmentParser.INTERVAL_SETTING);
settings.addAll(ReportingAttachmentParser.getSettings());

// http settings
settings.addAll(HttpSettings.getSettings());
Expand Down
23 changes: 18 additions & 5 deletions .../watcher/src/main/java/org/elasticsearch/xpack/watcher/notification/email/Attachment.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -24,17 +24,26 @@
import java.io.InputStream;
import java.io.OutputStream;
import java.nio.file.Path;
import java.util.Collections;
import java.util.Set;

import static javax.mail.Part.ATTACHMENT;
import static javax.mail.Part.INLINE;

public abstract class Attachment extends BodyPartSource {

private final boolean inline;
private final Set<String> warnings;

protected Attachment(String id, String name, String contentType, boolean inline) {
Copy link
Contributor

Choose a reason for hiding this comment

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

is this constructor used anymore? Im ok w/ removing it if we dont use it anywhere. Also, if we dont want a NPE down in the EmailTemplate we should check for null assignment in the other constructor, maybe with a Objects.requireNonNull. Otherwise the user is free to put null there and receive a nice NPE during warnings.addAll(attachment.getWarnings()); below. Ill add a note with here is the NPE im talking about.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yeah, this constructor is still used in some cases.

I added an assert warnings != null, since this is an internal API, the variable is final, means that someone working on Watcher internals would be required to explicitly call the second constructor with null, I think the assert is sufficient to express "don't do that". ed5128f

this(id, name, contentType, inline, Collections.emptySet());
}

protected Attachment(String id, String name, String contentType, boolean inline, Set<String> warnings) {
super(id, name, contentType);
this.inline = inline;
assert warnings != null;
this.warnings = warnings;
}

@Override
Expand All @@ -53,6 +62,10 @@ public boolean isInline() {
return inline;
}

public Set<String> getWarnings() {
return warnings;
}

/**
* intentionally not emitting path as it may come as an information leak
*/
Expand Down Expand Up @@ -116,15 +129,15 @@ public static class Bytes extends Attachment {
private final byte[] bytes;

public Bytes(String id, byte[] bytes, String contentType, boolean inline) {
this(id, id, bytes, contentType, inline);
this(id, id, bytes, contentType, inline, Collections.emptySet());
}

public Bytes(String id, String name, byte[] bytes, boolean inline) {
this(id, name, bytes, fileTypeMap.getContentType(name), inline);
this(id, name, bytes, fileTypeMap.getContentType(name), inline, Collections.emptySet());
}

public Bytes(String id, String name, byte[] bytes, String contentType, boolean inline) {
super(id, name, contentType, inline);
public Bytes(String id, String name, byte[] bytes, String contentType, boolean inline, Set<String> warnings) {
super(id, name, contentType, inline, warnings);
this.bytes = bytes;
}

Expand Down Expand Up @@ -213,7 +226,7 @@ protected XContent(String id, ToXContent content, XContentType type) {
}

protected XContent(String id, String name, ToXContent content, XContentType type) {
super(id, name, bytes(name, content, type), mimeType(type), false);
super(id, name, bytes(name, content, type), mimeType(type), false, Collections.emptySet());
}

static String mimeType(XContentType type) {
Expand Down
38 changes: 34 additions & 4 deletions ...tcher/src/main/java/org/elasticsearch/xpack/watcher/notification/email/EmailTemplate.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -6,6 +6,7 @@
package org.elasticsearch.xpack.watcher.notification.email;

import org.elasticsearch.ElasticsearchParseException;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.xcontent.ToXContentObject;
import org.elasticsearch.common.xcontent.XContentBuilder;
import org.elasticsearch.common.xcontent.XContentParser;
Expand All @@ -16,9 +17,11 @@
import java.io.IOException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashSet;
import java.util.List;
import java.util.Map;
import java.util.Objects;
import java.util.Set;

public class EmailTemplate implements ToXContentObject {

Expand Down Expand Up @@ -110,19 +113,46 @@ public Email.Builder render(TextTemplateEngine engine, Map<String, Object> model
if (subject != null) {
builder.subject(engine.render(subject, model));
}
if (textBody != null) {
builder.textBody(engine.render(textBody, model));
}

Set<String> warnings = new HashSet<>(1);
if (attachments != null) {
for (Attachment attachment : attachments.values()) {
builder.attach(attachment);
warnings.addAll(attachment.getWarnings());
Copy link
Contributor

Choose a reason for hiding this comment

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

here is the NPE im talking about

}
}

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I just realized, that despite the current documenation, body is not actually required, will need to adjust this to always emit a warning even if the body is not defined.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

done in 0acff82

String htmlWarnings = "";
String textWarnings = "";
if(warnings.isEmpty() == false){
StringBuilder textWarningBuilder = new StringBuilder();
StringBuilder htmlWarningBuilder = new StringBuilder();
warnings.forEach(w ->
{
if(Strings.isNullOrEmpty(w) == false) {
textWarningBuilder.append(w).append("\n");
htmlWarningBuilder.append(w).append("<br>");
}
});
textWarningBuilder.append("\n");
htmlWarningBuilder.append("<br>");
htmlWarnings = htmlWarningBuilder.toString();
textWarnings = textWarningBuilder.toString();
}
if (textBody != null) {
builder.textBody(textWarnings + engine.render(textBody, model));
}

if (htmlBody != null) {
String renderedHtml = engine.render(htmlBody, model);
String renderedHtml = htmlWarnings + engine.render(htmlBody, model);
renderedHtml = htmlSanitizer.sanitize(renderedHtml);
builder.htmlBody(renderedHtml);
}

if(htmlBody == null && textBody == null && Strings.isNullOrEmpty(textWarnings) == false){
builder.textBody(textWarnings);
}

return builder;
}

Expand Down
79 changes: 74 additions & 5 deletions .../elasticsearch/xpack/watcher/notification/email/attachment/ReportingAttachmentParser.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,11 +7,13 @@

import org.apache.logging.log4j.LogManager;
import org.apache.logging.log4j.Logger;
import org.apache.logging.log4j.message.ParameterizedMessage;
import org.elasticsearch.ElasticsearchException;
import org.elasticsearch.common.ParseField;
import org.elasticsearch.common.Strings;
import org.elasticsearch.common.bytes.BytesReference;
import org.elasticsearch.common.logging.LoggerMessageFormat;
import org.elasticsearch.common.settings.ClusterSettings;
import org.elasticsearch.common.settings.Setting;
import org.elasticsearch.common.settings.Settings;
import org.elasticsearch.common.unit.TimeValue;
Expand All @@ -37,22 +39,39 @@
import java.io.IOException;
import java.io.InputStream;
import java.io.UncheckedIOException;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.HashSet;
import java.util.List;
import java.util.Locale;
import java.util.Map;
import java.util.Set;
import java.util.concurrent.ConcurrentHashMap;

public class ReportingAttachmentParser implements EmailAttachmentParser<ReportingAttachment> {

public static final String TYPE = "reporting";

// total polling of 10 minutes happens this way by default
public static final Setting<TimeValue> INTERVAL_SETTING =
static final Setting<TimeValue> INTERVAL_SETTING =
Setting.timeSetting("xpack.notification.reporting.interval", TimeValue.timeValueSeconds(15), Setting.Property.NodeScope);
public static final Setting<Integer> RETRIES_SETTING =
static final Setting<Integer> RETRIES_SETTING =
Setting.intSetting("xpack.notification.reporting.retries", 40, 0, Setting.Property.NodeScope);

static final Setting<Boolean> REPORT_WARNING_ENABLED_SETTING =
Setting.boolSetting("xpack.notification.reporting.warning.enabled", true, Setting.Property.NodeScope, Setting.Property.Dynamic);

static final Setting.AffixSetting<String> REPORT_WARNING_TEXT =
Setting.affixKeySetting("xpack.notification.reporting.warning.", "text",
key -> Setting.simpleString(key, Setting.Property.NodeScope, Setting.Property.Dynamic));

private static final ObjectParser<Builder, AuthParseContext> PARSER = new ObjectParser<>("reporting_attachment");
private static final ObjectParser<KibanaReportingPayload, Void> PAYLOAD_PARSER =
new ObjectParser<>("reporting_attachment_kibana_payload", true, null);

static final Map<String, String> WARNINGS = Map.of("kbn-csv-contains-formulas", "Warning: The attachment [%s] contains " +
"characters which spreadsheet applications may interpret as formulas. Please ensure that the attachment is safe prior to opening.");

static {
PARSER.declareInt(Builder::retries, ReportingAttachment.RETRIES);
PARSER.declareBoolean(Builder::inline, ReportingAttachment.INLINE);
Expand All @@ -63,18 +82,52 @@ public class ReportingAttachmentParser implements EmailAttachmentParser<Reportin
PAYLOAD_PARSER.declareString(KibanaReportingPayload::setPath, new ParseField("path"));
}

private static List<Setting<?>> getDynamicSettings() {
return Arrays.asList(REPORT_WARNING_ENABLED_SETTING, REPORT_WARNING_TEXT);
}

private static List<Setting<?>> getStaticSettings() {
return Arrays.asList(INTERVAL_SETTING, RETRIES_SETTING);
}

public static List<Setting<?>> getSettings() {
List<Setting<?>> allSettings = new ArrayList<Setting<?>>(getDynamicSettings());
allSettings.addAll(getStaticSettings());
return allSettings;
}
private final Logger logger;
private final TimeValue interval;
private final int retries;
private HttpClient httpClient;
private final TextTemplateEngine templateEngine;
private boolean warningEnabled = REPORT_WARNING_ENABLED_SETTING.getDefault(Settings.EMPTY);
private final Map<String, String> customWarnings = new ConcurrentHashMap<>(1);

public ReportingAttachmentParser(Settings settings, HttpClient httpClient, TextTemplateEngine templateEngine) {
public ReportingAttachmentParser(Settings settings, HttpClient httpClient, TextTemplateEngine templateEngine,
ClusterSettings clusterSettings) {
this.interval = INTERVAL_SETTING.get(settings);
this.retries = RETRIES_SETTING.get(settings);
this.httpClient = httpClient;
this.templateEngine = templateEngine;
this.logger = LogManager.getLogger(getClass());
clusterSettings.addSettingsUpdateConsumer(REPORT_WARNING_ENABLED_SETTING, this::setWarningEnabled);
clusterSettings.addAffixUpdateConsumer(REPORT_WARNING_TEXT, this::addWarningText, this::warningValidator);
}

void setWarningEnabled(boolean warningEnabled) {
this.warningEnabled = warningEnabled;
}

void addWarningText(String name, String value) {
customWarnings.put(name, value);
}

void warningValidator(String name, String value) {
if (WARNINGS.keySet().contains(name) == false) {
throw new IllegalArgumentException(new ParameterizedMessage(
"Warning [{}] is not supported. Only the following warnings are supported [{}]",
name, String.join(", ", WARNINGS.keySet())).getFormattedMessage());
}
}

@Override
Expand Down Expand Up @@ -139,8 +192,24 @@ public Attachment toAttachment(WatchExecutionContext context, Payload payload, R
"method[{}], path[{}], status[{}], body[{}]", context.watch().id(), attachment.id(), request.host(),
request.port(), request.method(), request.path(), response.status(), body);
} else if (response.status() == 200) {
return new Attachment.Bytes(attachment.id(), BytesReference.toBytes(response.body()),
response.contentType(), attachment.inline());
Set<String> warnings = new HashSet<>(1);
if (warningEnabled) {
WARNINGS.forEach((warningKey, defaultWarning) -> {
String[] text = response.header(warningKey);
if (text != null && text.length > 0) {
if (Boolean.valueOf(text[0])) {
String warning = String.format(Locale.ROOT, defaultWarning, attachment.id());
String customWarning = customWarnings.get(warningKey);
if (Strings.isNullOrEmpty(customWarning) == false) {
warning = String.format(Locale.ROOT, customWarning, attachment.id());
}
warnings.add(warning);
}
}
});
}
return new Attachment.Bytes(attachment.id(), attachment.id(), BytesReference.toBytes(response.body()),
response.contentType(), attachment.inline(), warnings);
} else {
String body = response.body() != null ? response.body().utf8ToString() : null;
String message = LoggerMessageFormat.format("", "Watch[{}] reporting[{}] Unexpected status code host[{}], port[{}], " +
Expand Down
Loading