From 54926ec3364a0465e8f2d196bf8c34867e6216e5 Mon Sep 17 00:00:00 2001 From: Brian Murphy Date: Wed, 13 May 2015 15:36:38 -0400 Subject: [PATCH] Fix escaping of mustache strings. I think the escaping done in XMustacheFactory (and by extension JsonEscapingMustacheFactory in core) is broken. You cannot just escape any control character by sticking a '\' in front of it. For example a new line character it '\n' but this will be rendered as a line break. Simply prepending a '\' to this just results in a '\' and then a new line ! Added support for different escaping strategies based on the XContentType of a template for XMustacheEngine. Currently only JSON escaping is supported using jackson.JsonStringEncoder. Templates will be prepended with ____:: when the content type is set. If this is set to JSON we will json escape the content. Fixes: elastic/elasticsearch#404 Original commit: elastic/x-pack-elasticsearch@1400cba6590932bfcc556283a856b605b9eded07 --- .../template/MustacheTemplateEngine.java | 5 +- .../template/xmustache/XMustacheFactory.java | 40 +++------- .../XMustacheScriptEngineService.java | 43 ++++++++++- .../xmustache/XMustacheScriptEngineTests.java | 74 +++++++----------- .../template/xmustache/XMustacheTests.java | 76 +++++++++++++++++++ 5 files changed, 159 insertions(+), 79 deletions(-) diff --git a/src/main/java/org/elasticsearch/watcher/support/template/MustacheTemplateEngine.java b/src/main/java/org/elasticsearch/watcher/support/template/MustacheTemplateEngine.java index 5c44264d6e3..c1a8f91c3ad 100644 --- a/src/main/java/org/elasticsearch/watcher/support/template/MustacheTemplateEngine.java +++ b/src/main/java/org/elasticsearch/watcher/support/template/MustacheTemplateEngine.java @@ -12,6 +12,7 @@ import org.elasticsearch.common.settings.Settings; import org.elasticsearch.script.ExecutableScript; import org.elasticsearch.script.mustache.MustacheScriptEngineService; import org.elasticsearch.watcher.support.init.proxy.ScriptServiceProxy; +import org.elasticsearch.watcher.support.template.xmustache.XMustacheScriptEngineService; import java.util.HashMap; import java.util.Map; @@ -34,7 +35,9 @@ public class MustacheTemplateEngine extends AbstractComponent implements Templat Map mergedModel = new HashMap<>(); mergedModel.putAll(template.getParams()); mergedModel.putAll(model); - ExecutableScript executable = service.executable(MustacheScriptEngineService.NAME, template.getTemplate(), template.getType(), mergedModel); + ExecutableScript executable = service.executable(MustacheScriptEngineService.NAME, + XMustacheScriptEngineService.prepareTemplate(template.getTemplate(), template.getContentType()), + template.getType(), mergedModel); Object result = executable.run(); if (result instanceof BytesReference) { return ((BytesReference) result).toUtf8(); diff --git a/src/main/java/org/elasticsearch/watcher/support/template/xmustache/XMustacheFactory.java b/src/main/java/org/elasticsearch/watcher/support/template/xmustache/XMustacheFactory.java index 5881b875617..49048538ae2 100644 --- a/src/main/java/org/elasticsearch/watcher/support/template/xmustache/XMustacheFactory.java +++ b/src/main/java/org/elasticsearch/watcher/support/template/xmustache/XMustacheFactory.java @@ -6,9 +6,11 @@ package org.elasticsearch.watcher.support.template.xmustache; import org.elasticsearch.common.collect.Iterables; +import org.elasticsearch.common.jackson.core.io.JsonStringEncoder; import org.elasticsearch.common.mustache.DefaultMustacheFactory; import org.elasticsearch.common.mustache.MustacheException; import org.elasticsearch.common.mustache.reflect.ReflectionObjectHandler; +import org.elasticsearch.common.xcontent.XContentType; import java.io.IOException; import java.io.Writer; @@ -22,7 +24,10 @@ import java.util.*; */ public class XMustacheFactory extends DefaultMustacheFactory { - public XMustacheFactory() { + final XContentType contentType; + + public XMustacheFactory(XContentType contentType) { + this.contentType = contentType; setObjectHandler(new ReflectionObjectHandler() { @Override public Object coerce(Object object) { @@ -41,39 +46,16 @@ public class XMustacheFactory extends DefaultMustacheFactory { @Override public void encode(String value, Writer writer) { try { - escape(value, writer); + if (contentType == XContentType.JSON) { + writer.write(JsonStringEncoder.getInstance().quoteAsString(value)); + } else { + writer.write(value); + } } catch (IOException e) { throw new MustacheException("Failed to encode value: " + value); } } - public static Writer escape(String value, Writer writer) throws IOException { - for (int i = 0; i < value.length(); i++) { - final char character = value.charAt(i); - if (isEscapeChar(character)) { - writer.write('\\'); - } - writer.write(character); - } - return writer; - } - - public static boolean isEscapeChar(char c) { - switch (c) { - case '\b': - case '\f': - case '\n': - case '\r': - case '"': - case '\\': - case '\u000B': // vertical tab - case '\t': - return true; - } - return false; - } - - static class ArrayMap extends AbstractMap implements Iterable { private final Object array; diff --git a/src/main/java/org/elasticsearch/watcher/support/template/xmustache/XMustacheScriptEngineService.java b/src/main/java/org/elasticsearch/watcher/support/template/xmustache/XMustacheScriptEngineService.java index 3cec3e3503e..a1dc983029e 100644 --- a/src/main/java/org/elasticsearch/watcher/support/template/xmustache/XMustacheScriptEngineService.java +++ b/src/main/java/org/elasticsearch/watcher/support/template/xmustache/XMustacheScriptEngineService.java @@ -13,6 +13,7 @@ import org.elasticsearch.common.io.UTF8StreamWriter; import org.elasticsearch.common.io.stream.BytesStreamOutput; import org.elasticsearch.common.mustache.Mustache; import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.script.CompiledScript; import org.elasticsearch.script.ExecutableScript; import org.elasticsearch.script.ScriptEngineService; @@ -22,6 +23,7 @@ import org.elasticsearch.search.lookup.SearchLookup; import java.io.IOException; import java.lang.ref.SoftReference; import java.util.Collections; +import java.util.Locale; import java.util.Map; /** @@ -50,7 +52,9 @@ public class XMustacheScriptEngineService extends AbstractComponent implements S @Override public Object compile(String template) { /** Factory to generate Mustache objects from. */ - return (new XMustacheFactory()).compile(new FastStringReader(template), "query-template"); + XContentType xContentType = detectContentType(template); + template = trimContentType(template); + return (new XMustacheFactory(xContentType)).compile(new FastStringReader(template), "query-template"); } /** @@ -125,6 +129,17 @@ public class XMustacheScriptEngineService extends AbstractComponent implements S // Nothing to do here } + public static String prepareTemplate(String template, @Nullable XContentType contentType) { + if (contentType == null) { + return template; + } + return new StringBuilder("__") + .append(contentType.shortName().toLowerCase(Locale.ROOT)) + .append("__::") + .append(template) + .toString(); + } + /** * Used at query execution time by script service in order to execute a query template. * */ @@ -188,4 +203,30 @@ public class XMustacheScriptEngineService extends AbstractComponent implements S writer.reset(); return writer; } + + private String trimContentType(String template) { + if (!template.startsWith("__")){ + return template; //Doesn't even start with __ so can't have a content type + } + int index = template.indexOf("__::", 3); //There must be a __= 0 && index < 12) { //Assume that the content type name is less than 10 characters long otherwise we may falsely detect strings that start with '__ and have '__::' somewhere in the content + if (template.length() == 6) { + template = ""; + } else { + template = template.substring(index + 4); + } + } + return template; + } + + private XContentType detectContentType(String template) { + if (template.startsWith("__")) { + int endOfContentName = template.indexOf("__::", 3); //There must be a __ vars = new HashMap<>(); vars.put("boost_val", "0.3"); @@ -45,7 +43,7 @@ public class XMustacheScriptEngineTests extends ElasticsearchTestCase { new String(o.toBytes(), Charset.forName("UTF-8"))); } { - String template = "GET _search {\"query\": " + "{\"boosting\": {" + "\"positive\": {\"match\": {\"body\": \"gift\"}}," + String template = "__json__::GET _search {\"query\": " + "{\"boosting\": {" + "\"positive\": {\"match\": {\"body\": \"gift\"}}," + "\"negative\": {\"term\": {\"body\": {\"value\": \"{{body_val}}\"}" + "}}, \"negative_boost\": {{boost_val}} } }}"; Map vars = new HashMap<>(); vars.put("boost_val", "0.3"); @@ -57,47 +55,27 @@ public class XMustacheScriptEngineTests extends ElasticsearchTestCase { } } - @Test - public void testEscapeJson() throws IOException { - { - StringWriter writer = new StringWriter(); - XMustacheFactory.escape("hello \n world", writer); - assertThat(writer.toString(), equalTo("hello \\\n world")); - } - { - StringWriter writer = new StringWriter(); - XMustacheFactory.escape("\n", writer); - assertThat(writer.toString(), equalTo("\\\n")); + @Test @Repeat(iterations = 100) + public void testInvalidPrefixes() throws Exception { + String[] specialStrings = new String[]{"\f", "\n", "\r", "\"", "\\", "\t", "\b", "__::", "__" }; + String prefix = randomFrom("", "__", "____::", "___::", "____", "::", "++json__::", "__json__", "+_json__::", "__json__:"); + String template = prefix + " {{test_var1}} {{test_var2}}"; + Map vars = new HashMap<>(); + Writer var1Writer = new StringWriter(); + Writer var2Writer = new StringWriter(); + + for(int i = 0; i < scaledRandomIntBetween(10,1000); ++i) { + var1Writer.write(randomRealisticUnicodeOfCodepointLengthBetween(0, 10)); + var2Writer.write(randomRealisticUnicodeOfCodepointLengthBetween(0, 10)); + var1Writer.append(randomFrom(specialStrings)); + var2Writer.append(randomFrom(specialStrings)); } - Character[] specialChars = new Character[]{'\f', '\n', '\r', '"', '\\', (char) 11, '\t', '\b' }; - int iters = scaledRandomIntBetween(100, 1000); - for (int i = 0; i < iters; i++) { - int rounds = scaledRandomIntBetween(1, 20); - StringWriter escaped = new StringWriter(); - StringWriter writer = new StringWriter(); - for (int j = 0; j < rounds; j++) { - String s = getChars(); - writer.write(s); - escaped.write(s); - char c = RandomPicks.randomFrom(getRandom(), specialChars); - writer.append(c); - escaped.append('\\'); - escaped.append(c); - } - StringWriter target = new StringWriter(); - assertThat(escaped.toString(), equalTo(XMustacheFactory.escape(writer.toString(), target).toString())); - } - } - - private String getChars() { - String string = randomRealisticUnicodeOfCodepointLengthBetween(0, 10); - for (int i = 0; i < string.length(); i++) { - if (XMustacheFactory.isEscapeChar(string.charAt(i))) { - return string.substring(0, i); - } - } - return string; - } - -} \ No newline at end of file + vars.put("test_var1", var1Writer.toString()); + vars.put("test_var2", var2Writer.toString()); + BytesReference o = (BytesReference) engine.execute(engine.compile(template), vars); + String s1 = o.toUtf8(); + String s2 = prefix + " " + var1Writer.toString() + " " + var2Writer.toString(); + assertEquals(s1, s2); + } +} diff --git a/src/test/java/org/elasticsearch/watcher/support/template/xmustache/XMustacheTests.java b/src/test/java/org/elasticsearch/watcher/support/template/xmustache/XMustacheTests.java index 3106d17fde0..4011bf9a683 100644 --- a/src/test/java/org/elasticsearch/watcher/support/template/xmustache/XMustacheTests.java +++ b/src/test/java/org/elasticsearch/watcher/support/template/xmustache/XMustacheTests.java @@ -10,12 +10,16 @@ import com.carrotsearch.randomizedtesting.annotations.Repeat; import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.collect.ImmutableMap; import org.elasticsearch.common.collect.ImmutableSet; +import org.elasticsearch.common.jackson.core.io.JsonStringEncoder; import org.elasticsearch.common.settings.ImmutableSettings; +import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.script.ScriptEngineService; import org.elasticsearch.test.ElasticsearchTestCase; import org.junit.Before; import org.junit.Test; +import java.io.IOException; +import java.io.StringWriter; import java.util.HashMap; import java.util.Map; @@ -84,4 +88,76 @@ public class XMustacheTests extends ElasticsearchTestCase { BytesReference bytes = (BytesReference) output; assertThat(bytes.toUtf8(), equalTo("foo bar")); } + + @Test @Repeat(iterations = 10) + public void testEscaping() throws Exception { + XContentType contentType = randomFrom(XContentType.values()); + if (rarely()) { + contentType = null; + } + Character[] specialChars = new Character[]{'\f', '\n', '\r', '"', '\\', (char) 11, '\t', '\b' }; + int iters = scaledRandomIntBetween(100, 1000); + for (int i = 0; i < iters; i++) { + int rounds = scaledRandomIntBetween(1, 20); + StringWriter escaped = new StringWriter(); //This will be escaped as it is constructed + StringWriter unescaped = new StringWriter(); //This will be escaped at the end + + for (int j = 0; j < rounds; j++) { + String s = getChars(); + unescaped.write(s); + if (contentType == XContentType.JSON) { + escaped.write(JsonStringEncoder.getInstance().quoteAsString(s)); + } else { + escaped.write(s); + } + + char c = randomFrom(specialChars); + unescaped.append(c); + + if (contentType == XContentType.JSON) { + escaped.write(JsonStringEncoder.getInstance().quoteAsString("" + c)); + } else { + escaped.append(c); + } + } + + if (contentType == XContentType.JSON) { + assertThat(escaped.toString(), equalTo(new String(JsonStringEncoder.getInstance().quoteAsString(unescaped.toString())))); + } + else { + assertThat(escaped.toString(), equalTo(unescaped.toString())); + } + + String template = XMustacheScriptEngineService.prepareTemplate("{{data}}", contentType); + + Map dataMap = new HashMap<>(); + dataMap.put("data", unescaped.toString()); + Object mustache = engine.compile(template); + Object output = engine.execute(mustache, dataMap); + + assertThat(output, notNullValue()); + assertThat(output, instanceOf(BytesReference.class)); + BytesReference bytes = (BytesReference) output; + String renderedTemplate = bytes.toUtf8(); + + if (contentType == XContentType.JSON) { + if (!escaped.toString().equals(renderedTemplate)) { + String escapedString = escaped.toString(); + for (int l = 0; l < renderedTemplate.length() && l < escapedString.length(); ++l) { + if (renderedTemplate.charAt(l) != escapedString.charAt(l)) { + logger.error("at [{}] expected [{}] but got [{}]", l, renderedTemplate.charAt(l), escapedString.charAt(l)); + } + } + } + assertThat(escaped.toString(), equalTo(renderedTemplate)); + } else { + assertThat(unescaped.toString(), equalTo(renderedTemplate)); + } + } + } + + private String getChars() throws IOException { + return randomRealisticUnicodeOfCodepointLengthBetween(0, 10); + } + }