From e6c4b53bf88be32c1728ac346ebf06137e77e13a Mon Sep 17 00:00:00 2001 From: Brian Murphy Date: Sun, 3 May 2015 23:45:19 -0400 Subject: [PATCH] Move email html body sanitization to the EmailTemplate.render This change moves the sanitization of the html body of emails to render time instead of at message send time. Move the sanitization code the the EmailTemplate class. Add
tag to allowed html tags. Add global setting `watcher.actions.email.sanitize_html` that defaults to true. If this is set to false html sanitization will be disabled Add documentation for sanitization disable setting. Enhance email tests to verify that sanitization is happening at construction time. Fixes elastic/elasticsearch#356 Original commit: elastic/x-pack-elasticsearch@282a2d85c2cc476417913b1e8258d992af4fa526 --- .../watcher/actions/email/EmailAction.java | 4 +- .../actions/email/EmailActionFactory.java | 6 +- .../actions/email/ExecutableEmailAction.java | 14 +- .../actions/email/service/Account.java | 1 - .../watcher/actions/email/service/Email.java | 6 +- .../actions/email/service/EmailTemplate.java | 106 +++++++++++---- .../actions/email/service/Profile.java | 59 +------- .../actions/email/EmailActionTests.java | 2 +- .../email/service/EmailTemplateTests.java | 127 ++++++++++++++++++ .../actions/email/service/EmailTest.java | 3 +- .../email/service/HtmlSanitizeTests.java | 16 ++- 11 files changed, 238 insertions(+), 106 deletions(-) create mode 100644 src/test/java/org/elasticsearch/watcher/actions/email/service/EmailTemplateTests.java diff --git a/src/main/java/org/elasticsearch/watcher/actions/email/EmailAction.java b/src/main/java/org/elasticsearch/watcher/actions/email/EmailAction.java index 01e49d04cff..51b0d992346 100644 --- a/src/main/java/org/elasticsearch/watcher/actions/email/EmailAction.java +++ b/src/main/java/org/elasticsearch/watcher/actions/email/EmailAction.java @@ -113,8 +113,8 @@ public class EmailAction implements Action { return builder.endObject(); } - public static EmailAction parse(String watchId, String actionId, XContentParser parser) throws IOException { - EmailTemplate.Parser emailParser = new EmailTemplate.Parser(); + public static EmailAction parse(String watchId, String actionId, XContentParser parser, boolean sanitizeHtmlBody) throws IOException { + EmailTemplate.Parser emailParser = new EmailTemplate.Parser(sanitizeHtmlBody); String account = null; String user = null; Secret password = null; diff --git a/src/main/java/org/elasticsearch/watcher/actions/email/EmailActionFactory.java b/src/main/java/org/elasticsearch/watcher/actions/email/EmailActionFactory.java index 65b9735fb00..b8220583a97 100644 --- a/src/main/java/org/elasticsearch/watcher/actions/email/EmailActionFactory.java +++ b/src/main/java/org/elasticsearch/watcher/actions/email/EmailActionFactory.java @@ -23,12 +23,16 @@ public class EmailActionFactory extends ActionFactory model = Variables.createCtxModel(ctx, payload); - Email.Builder email = action.getEmail().render(templateEngine, model); - email.id(ctx.id().value()); - + Map attachmentMap = new HashMap<>(); + Attachment.Bytes attachment = null; if (action.getAttachData()) { - Attachment.Bytes attachment = new Attachment.XContent.Yaml("data", "data.yml", new Payload.Simple(model)); + attachment = new Attachment.XContent.Yaml("data", "data.yml", new Payload.Simple(model)); + attachmentMap.put(attachment.id(), attachment); + } + + Email.Builder email = action.getEmail().render(templateEngine, model, attachmentMap); + email.id(ctx.id().value()); + if (attachment != null) { email.attach(attachment); } diff --git a/src/main/java/org/elasticsearch/watcher/actions/email/service/Account.java b/src/main/java/org/elasticsearch/watcher/actions/email/service/Account.java index 14d3b3086eb..c4699ca1837 100644 --- a/src/main/java/org/elasticsearch/watcher/actions/email/service/Account.java +++ b/src/main/java/org/elasticsearch/watcher/actions/email/service/Account.java @@ -86,7 +86,6 @@ public class Account { transport.connect(config.smtp.host, config.smtp.port, user, password); try { - MimeMessage message = profile.toMimeMessage(email, session); String mid = message.getMessageID(); message.saveChanges(); diff --git a/src/main/java/org/elasticsearch/watcher/actions/email/service/Email.java b/src/main/java/org/elasticsearch/watcher/actions/email/service/Email.java index 08e97ceb615..bab79e5b92e 100644 --- a/src/main/java/org/elasticsearch/watcher/actions/email/service/Email.java +++ b/src/main/java/org/elasticsearch/watcher/actions/email/service/Email.java @@ -218,6 +218,7 @@ public class Email implements ToXContent { private String htmlBody; private ImmutableMap.Builder attachments = ImmutableMap.builder(); private ImmutableMap.Builder inlines = ImmutableMap.builder(); + private boolean sanitizeHtmlBody = true; private Builder() { } @@ -236,6 +237,8 @@ public class Email implements ToXContent { htmlBody = email.htmlBody; attachments.putAll(email.attachments); inlines.putAll(email.inlines); + //The builder will already have sanitized the html when the email was built originally so don't double sanitize + sanitizeHtmlBody = false; return this; } @@ -330,7 +333,8 @@ public class Email implements ToXContent { public Email build() { assert id != null : "email id should not be null (should be set to the watch id"; - return new Email(id, from, replyTo, priority, sentDate, to, cc, bcc, subject, textBody, htmlBody, attachments.build(), inlines.build()); + ImmutableMap attachmentsMap = attachments.build(); + return new Email(id, from, replyTo, priority, sentDate, to, cc, bcc, subject, textBody, htmlBody, attachmentsMap, inlines.build()); } } diff --git a/src/main/java/org/elasticsearch/watcher/actions/email/service/EmailTemplate.java b/src/main/java/org/elasticsearch/watcher/actions/email/service/EmailTemplate.java index 22bc439b386..104decbee89 100644 --- a/src/main/java/org/elasticsearch/watcher/actions/email/service/EmailTemplate.java +++ b/src/main/java/org/elasticsearch/watcher/actions/email/service/EmailTemplate.java @@ -10,7 +10,9 @@ import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.watcher.support.template.Template; import org.elasticsearch.watcher.support.template.TemplateEngine; +import org.owasp.html.*; +import javax.annotation.ParametersAreNonnullByDefault; import javax.mail.internet.AddressException; import java.io.IOException; import java.util.*; @@ -29,10 +31,11 @@ public class EmailTemplate implements ToXContent { final Template subject; final Template textBody; final Template htmlBody; + final boolean sanitizeHtmlBody; public EmailTemplate(Template from, Template[] replyTo, Template priority, Template[] to, Template[] cc, Template[] bcc, Template subject, Template textBody, - Template htmlBody) { + Template htmlBody, boolean sanitizeHtmlBody) { this.from = from; this.replyTo = replyTo; this.priority = priority; @@ -42,6 +45,7 @@ public class EmailTemplate implements ToXContent { this.subject = subject; this.textBody = textBody; this.htmlBody = htmlBody; + this.sanitizeHtmlBody = sanitizeHtmlBody; } public Template from() { @@ -80,7 +84,11 @@ public class EmailTemplate implements ToXContent { return htmlBody; } - public Email.Builder render(TemplateEngine engine, Map model) throws AddressException { + public boolean sanitizeHtmlBody() { + return sanitizeHtmlBody; + } + + public Email.Builder render(TemplateEngine engine, Map model, Map attachmentsMap) throws AddressException { Email.Builder builder = Email.builder(); if (from != null) { builder.from(engine.render(from, model)); @@ -111,7 +119,11 @@ public class EmailTemplate implements ToXContent { builder.textBody(engine.render(textBody, model)); } if (htmlBody != null) { - builder.htmlBody(engine.render(htmlBody, model)); + String renderedHtml = engine.render(htmlBody, model); + if (sanitizeHtmlBody && htmlBody != null) { + renderedHtml = sanitizeHtml(attachmentsMap, renderedHtml); + } + builder.htmlBody(renderedHtml); } return builder; } @@ -199,31 +211,10 @@ public class EmailTemplate implements ToXContent { return builder; } - public static EmailTemplate parse(XContentParser parser) throws IOException{ - EmailTemplate.Parser templateParser = parser(); - - String currentFieldName = null; - XContentParser.Token token; - while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { - if (token == XContentParser.Token.FIELD_NAME) { - currentFieldName = parser.currentName(); - } else if ((token.isValue() || token == XContentParser.Token.START_OBJECT || token == XContentParser.Token.START_ARRAY) && currentFieldName != null) { - if (!templateParser.handle(currentFieldName, parser)) { - throw new EmailException("could not parse email template. unrecognized field [" + currentFieldName + "]"); - } - } - } - return templateParser.parsedTemplate(); - } - public static Builder builder() { return new Builder(); } - public static Parser parser() { - return new Parser(); - } - public static class Builder { private Template from; @@ -235,6 +226,7 @@ public class EmailTemplate implements ToXContent { private Template subject; private Template textBody; private Template htmlBody; + private boolean sanitizeHtmlBody = true; private Builder() { } @@ -327,17 +319,18 @@ public class EmailTemplate implements ToXContent { return this; } - public Builder htmlBody(String html) { - return htmlBody(new Template(html)); + public Builder htmlBody(String html, boolean sanitizeHtmlBody) { + return htmlBody(new Template(html), sanitizeHtmlBody); } - public Builder htmlBody(Template html) { + public Builder htmlBody(Template html, boolean sanitizeHtmlBody) { this.htmlBody = html; + this.sanitizeHtmlBody = sanitizeHtmlBody; return this; } public EmailTemplate build() { - return new EmailTemplate(from, replyTo, priority, to, cc, bcc, subject, textBody, htmlBody); + return new EmailTemplate(from, replyTo, priority, to, cc, bcc, subject, textBody, htmlBody, sanitizeHtmlBody); } } @@ -345,6 +338,11 @@ public class EmailTemplate implements ToXContent { public static class Parser { private final EmailTemplate.Builder builder = builder(); + private final boolean sanitizeHtmlBody; + + public Parser(boolean sanitizeHtmlBody) { + this.sanitizeHtmlBody = sanitizeHtmlBody; + } public boolean handle(String fieldName, XContentParser parser) throws IOException { if (Email.Field.FROM.match(fieldName)) { @@ -396,7 +394,7 @@ public class EmailTemplate implements ToXContent { } else if (Email.Field.TEXT_BODY.match(fieldName)) { builder.textBody(Template.parse(parser)); } else if (Email.Field.HTML_BODY.match(fieldName)) { - builder.htmlBody(Template.parse(parser)); + builder.htmlBody(Template.parse(parser), sanitizeHtmlBody); } else { return false; } @@ -407,4 +405,54 @@ public class EmailTemplate implements ToXContent { return builder.build(); } } + + static String sanitizeHtml(final Map attachments, String html){ + ElementPolicy onlyCIDImgPolicy = new AttachementVerifyElementPolicy(attachments); + PolicyFactory policy = Sanitizers.FORMATTING + .and(new HtmlPolicyBuilder() + .allowElements("img", "table", "tr", "td", "style", "body", "head", "hr") + .allowAttributes("src").onElements("img") + .allowAttributes("class").onElements("style") + .allowUrlProtocols("cid") + .allowCommonInlineFormattingElements() + .allowElements(onlyCIDImgPolicy, "img") + .allowStyling(CssSchema.DEFAULT) + .toFactory()) + .and(Sanitizers.LINKS) + .and(Sanitizers.BLOCKS); + return policy.sanitize(html); + } + + private static class AttachementVerifyElementPolicy implements ElementPolicy { + + private final Map attachments; + + AttachementVerifyElementPolicy(Map attachments) { + this.attachments = attachments; + } + + @Override + public String apply(@ParametersAreNonnullByDefault String elementName, @ParametersAreNonnullByDefault List attrs) { + if (attrs.size() == 0) { + return elementName; + } + for (int i = 0; i < attrs.size(); ++i) { + if(attrs.get(i).equals("src") && i < attrs.size() - 1) { + String srcValue = attrs.get(i+1); + if (!srcValue.startsWith("cid:")) { + return null; //Disallow anything other than content ids + } + String contentId = srcValue.substring(4); + if (attachments.containsKey(contentId)) { + return elementName; + } else { + return null; //This cid wasn't found + } + } + } + return elementName; + } + } + + } diff --git a/src/main/java/org/elasticsearch/watcher/actions/email/service/Profile.java b/src/main/java/org/elasticsearch/watcher/actions/email/service/Profile.java index 405041ddec0..5f79a48e0fd 100644 --- a/src/main/java/org/elasticsearch/watcher/actions/email/service/Profile.java +++ b/src/main/java/org/elasticsearch/watcher/actions/email/service/Profile.java @@ -6,13 +6,9 @@ package org.elasticsearch.watcher.actions.email.service; import org.elasticsearch.common.base.Charsets; -import org.elasticsearch.common.collect.ImmutableMap; import org.elasticsearch.common.xcontent.ToXContent; import org.elasticsearch.common.xcontent.XContentBuilder; -import org.owasp.html.*; -import javax.annotation.Nullable; -import javax.annotation.ParametersAreNonnullByDefault; import javax.mail.Message; import javax.mail.MessagingException; import javax.mail.Session; @@ -20,7 +16,6 @@ import javax.mail.internet.MimeBodyPart; import javax.mail.internet.MimeMessage; import javax.mail.internet.MimeMultipart; import java.io.IOException; -import java.util.List; import java.util.Locale; /** @@ -92,8 +87,7 @@ public enum Profile implements ToXContent { if (email.htmlBody != null) { MimeBodyPart html = new MimeBodyPart(); - String sanitizedHtml = sanitizeHtml(email.attachments, email.htmlBody); - html.setText(sanitizedHtml, Charsets.UTF_8.name(), "html"); + html.setText(email.htmlBody, Charsets.UTF_8.name(), "html"); alternative.addBodyPart(html); } @@ -222,55 +216,4 @@ public enum Profile implements ToXContent { } return part; } - - static String sanitizeHtml(final ImmutableMap attachments, String html){ - ElementPolicy onlyCIDImgPolicy = new AttachementVerifyElementPolicy(attachments); - PolicyFactory policy = Sanitizers.FORMATTING - .and(new HtmlPolicyBuilder() - .allowElements("img", "table", "tr", "td", "style", "body", "head") - .allowAttributes("src").onElements("img") - .allowAttributes("class").onElements("style") - .allowUrlProtocols("cid") - .allowCommonInlineFormattingElements() - .allowElements(onlyCIDImgPolicy, "img") - .allowStyling(CssSchema.DEFAULT) - .toFactory()) - .and(Sanitizers.LINKS) - .and(Sanitizers.BLOCKS); - return policy.sanitize(html); - } - - - - private static class AttachementVerifyElementPolicy implements ElementPolicy { - - private final ImmutableMap attachments; - - AttachementVerifyElementPolicy(ImmutableMap attchments) { - this.attachments = attchments; - } - - @Nullable - @Override - public String apply(@ParametersAreNonnullByDefault String elementName, @ParametersAreNonnullByDefault List attrs) { - if (attrs.size() == 0) { - return elementName; - } - for (int i = 0; i < attrs.size(); ++i) { - if(attrs.get(i).equals("src") && i < attrs.size() - 1) { - String srcValue = attrs.get(i+1); - if (!srcValue.startsWith("cid:")) { - return null; //Disallow anything other than content ids - } - String contentId = srcValue.substring(4); - if (attachments.containsKey(contentId)) { - return elementName; - } else { - return null; //This cid wasn't found - } - } - } - return elementName; - } - } } diff --git a/src/test/java/org/elasticsearch/watcher/actions/email/EmailActionTests.java b/src/test/java/org/elasticsearch/watcher/actions/email/EmailActionTests.java index 08a570025f0..2ecbbc90f2d 100644 --- a/src/test/java/org/elasticsearch/watcher/actions/email/EmailActionTests.java +++ b/src/test/java/org/elasticsearch/watcher/actions/email/EmailActionTests.java @@ -72,7 +72,7 @@ public class EmailActionTests extends ElasticsearchTestCase { Template htmlBody = null; if (randomBoolean()) { htmlBody = new Template("_html_body"); - emailBuilder.htmlBody(htmlBody); + emailBuilder.htmlBody(htmlBody, true); } EmailTemplate email = emailBuilder.build(); diff --git a/src/test/java/org/elasticsearch/watcher/actions/email/service/EmailTemplateTests.java b/src/test/java/org/elasticsearch/watcher/actions/email/service/EmailTemplateTests.java new file mode 100644 index 00000000000..aa0ba302c7e --- /dev/null +++ b/src/test/java/org/elasticsearch/watcher/actions/email/service/EmailTemplateTests.java @@ -0,0 +1,127 @@ +/* + * Copyright Elasticsearch B.V. and/or licensed to Elasticsearch B.V. under one + * or more contributor license agreements. Licensed under the Elastic License; + * you may not use this file except in compliance with the Elastic License. + */ +package org.elasticsearch.watcher.actions.email.service; + +import com.carrotsearch.randomizedtesting.annotations.Repeat; +import org.elasticsearch.common.xcontent.ToXContent; +import org.elasticsearch.common.xcontent.XContentBuilder; +import org.elasticsearch.common.xcontent.XContentFactory; +import org.elasticsearch.common.xcontent.XContentParser; +import org.elasticsearch.common.xcontent.json.JsonXContent; +import org.elasticsearch.test.ElasticsearchTestCase; +import org.elasticsearch.watcher.support.template.Template; +import org.elasticsearch.watcher.support.template.TemplateEngine; +import org.junit.Test; + +import java.util.ArrayList; +import java.util.HashMap; +import java.util.List; +import java.util.Map; + +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.is; +import static org.mockito.Mockito.mock; +import static org.mockito.Mockito.when; + +/** + */ +public class EmailTemplateTests extends ElasticsearchTestCase { + + @Test @Repeat(iterations = 100) + public void testEmailTemplate_Parser_SelfGenerated() throws Exception { + Template from = randomFrom(new Template("from@from.com"), null); + List