From 34ddaa7478a261a9e4a1df81b985924de4aa18b5 Mon Sep 17 00:00:00 2001 From: Lee Hinman Date: Mon, 12 Dec 2022 16:02:37 -0700 Subject: [PATCH] Tweak lang-mustache factory (#92211) This commits makes a few very minor tweaks to our Mustache scripting capabilities. 1. Switches from `DefaultMustacheFactory` to `SafeMustacheFactory` In the event that the security manager were disabled (or removed, as is threatened in subsequent JDK releases), Mustache's "partial template" (`{{>partial}}`) support is a security risk because it would allow reading from an arbitrary URL or file on disk. Switching to the "Safe" version and passing in an empty set into the parent constructor disallows using partial Mustache templates. This also switches from `Function` to the `TemplateFunction` value for the built-in functions. 3. Minor internal optimization This removes useless grouping for one of the built-in mustache functions, and removes the call to `CollectionUtils.ensureNoSelfReferences` used in `CustomReflectionObjectHandler`. This check during stringification should not be necessary because Mustache templates without "partial" support cannot be self-referencing. --- .../mustache/CustomMustacheFactory.java | 17 ++++---- .../CustomReflectionObjectHandler.java | 7 ---- .../xpack/core/watcher/watch/Payload.java | 2 + x-pack/plugin/watcher/qa/rest/build.gradle | 1 + .../test/painless/40_exception.yml | 39 +++---------------- 5 files changed, 18 insertions(+), 48 deletions(-) diff --git a/modules/lang-mustache/src/main/java/org/elasticsearch/script/mustache/CustomMustacheFactory.java b/modules/lang-mustache/src/main/java/org/elasticsearch/script/mustache/CustomMustacheFactory.java index 37abffed2fed2..6c432c7306d37 100644 --- a/modules/lang-mustache/src/main/java/org/elasticsearch/script/mustache/CustomMustacheFactory.java +++ b/modules/lang-mustache/src/main/java/org/elasticsearch/script/mustache/CustomMustacheFactory.java @@ -14,7 +14,9 @@ import com.github.mustachejava.Mustache; import com.github.mustachejava.MustacheException; import com.github.mustachejava.MustacheVisitor; +import com.github.mustachejava.SafeMustacheFactory; import com.github.mustachejava.TemplateContext; +import com.github.mustachejava.TemplateFunction; import com.github.mustachejava.codes.DefaultMustache; import com.github.mustachejava.codes.IterableCode; import com.github.mustachejava.codes.WriteCode; @@ -34,12 +36,11 @@ import java.util.Map; import java.util.Objects; import java.util.StringJoiner; -import java.util.function.Function; import java.util.function.Supplier; import java.util.regex.Matcher; import java.util.regex.Pattern; -public class CustomMustacheFactory extends DefaultMustacheFactory { +public class CustomMustacheFactory extends SafeMustacheFactory { static final String V7_JSON_MEDIA_TYPE_WITH_CHARSET = "application/json; charset=UTF-8"; static final String JSON_MEDIA_TYPE_WITH_CHARSET = "application/json;charset=utf-8"; static final String JSON_MEDIA_TYPE = "application/json"; @@ -64,7 +65,7 @@ public class CustomMustacheFactory extends DefaultMustacheFactory { private final Encoder encoder; public CustomMustacheFactory(String mediaType) { - super(); + super(Collections.emptySet(), "."); setObjectHandler(new CustomReflectionObjectHandler()); this.encoder = createEncoder(mediaType); } @@ -145,7 +146,7 @@ protected void tag(Writer writer, String tag) throws IOException { writer.write(tc.endChars()); } - protected abstract Function createFunction(Object resolved); + protected abstract TemplateFunction createFunction(Object resolved); /** * At compile time, this function extracts the name of the variable: @@ -188,7 +189,7 @@ static class ToJsonCode extends CustomCode { @Override @SuppressWarnings("unchecked") - protected Function createFunction(Object resolved) { + protected TemplateFunction createFunction(Object resolved) { return s -> { if (resolved == null) { return null; @@ -238,7 +239,7 @@ static class JoinerCode extends CustomCode { } @Override - protected Function createFunction(Object resolved) { + protected TemplateFunction createFunction(Object resolved) { return s -> { if (s == null) { return null; @@ -260,7 +261,7 @@ static boolean match(String variable) { static class CustomJoinerCode extends JoinerCode { - private static final Pattern PATTERN = Pattern.compile("^(?:" + CODE + " delimiter='(.*)')$"); + private static final Pattern PATTERN = Pattern.compile("^" + CODE + " delimiter='(.*)'$"); CustomJoinerCode(TemplateContext tc, DefaultMustacheFactory df, Mustache mustache, String variable) { super(tc, df, mustache, extractDelimiter(variable)); @@ -357,7 +358,7 @@ static class UrlEncoder implements Encoder { @Override public void encode(String s, Writer writer) throws IOException { - writer.write(URLEncoder.encode(s, StandardCharsets.UTF_8.name())); + writer.write(URLEncoder.encode(s, StandardCharsets.UTF_8)); } } } diff --git a/modules/lang-mustache/src/main/java/org/elasticsearch/script/mustache/CustomReflectionObjectHandler.java b/modules/lang-mustache/src/main/java/org/elasticsearch/script/mustache/CustomReflectionObjectHandler.java index d3ec96f68af59..12634c0534122 100644 --- a/modules/lang-mustache/src/main/java/org/elasticsearch/script/mustache/CustomReflectionObjectHandler.java +++ b/modules/lang-mustache/src/main/java/org/elasticsearch/script/mustache/CustomReflectionObjectHandler.java @@ -10,7 +10,6 @@ import com.github.mustachejava.reflect.ReflectionObjectHandler; -import org.elasticsearch.common.util.CollectionUtils; import org.elasticsearch.common.util.Maps; import org.elasticsearch.common.util.iterable.Iterables; @@ -144,10 +143,4 @@ public Iterator iterator() { return col.iterator(); } } - - @Override - public String stringify(Object object) { - CollectionUtils.ensureNoSelfReferences(object, "CustomReflectionObjectHandler stringify"); - return super.stringify(object); - } } diff --git a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/watcher/watch/Payload.java b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/watcher/watch/Payload.java index efe22fcab9d73..46803619f562e 100644 --- a/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/watcher/watch/Payload.java +++ b/x-pack/plugin/core/src/main/java/org/elasticsearch/xpack/core/watcher/watch/Payload.java @@ -7,6 +7,7 @@ package org.elasticsearch.xpack.core.watcher.watch; import org.elasticsearch.common.collect.MapBuilder; +import org.elasticsearch.common.util.CollectionUtils; import org.elasticsearch.xcontent.ToXContentObject; import org.elasticsearch.xcontent.XContentBuilder; @@ -37,6 +38,7 @@ public Simple(String key, Object value) { } public Simple(Map data) { + CollectionUtils.ensureNoSelfReferences(data, "watcher action payload"); this.data = data; } diff --git a/x-pack/plugin/watcher/qa/rest/build.gradle b/x-pack/plugin/watcher/qa/rest/build.gradle index 007c11c1d4b69..026ec7edb0fb0 100644 --- a/x-pack/plugin/watcher/qa/rest/build.gradle +++ b/x-pack/plugin/watcher/qa/rest/build.gradle @@ -40,6 +40,7 @@ tasks.named("yamlRestTestV7CompatTransform").configure{ task -> task.skipTest("mustache/30_search_input/Test search input mustache integration (using request body and rest_total_hits_as_int)", "remove JodaCompatibleDateTime -- ZonedDateTime doesn't output millis/nanos if they're 0 (#78417)") task.skipTest("mustache/30_search_input/Test search input mustache integration (using request body)", "remove JodaCompatibleDateTime -- ZonedDateTime doesn't output millis/nanos if they're 0 (#78417)") task.skipTest("mustache/40_search_transform/Test search transform mustache integration (using request body)", "remove JodaCompatibleDateTime -- ZonedDateTime doesn't output millis/nanos if they're 0 (#78417)") + task.skipTest("painless/40_exception/Test painless exceptions are returned when logging a broken response", "Exceptions are no longer thrown from Mustache, but from the transform action itself") task.replaceKeyInDo("watcher.ack_watch", "xpack-watcher.ack_watch") task.replaceKeyInDo("watcher.activate_watch", "xpack-watcher.activate_watch") task.replaceKeyInDo("watcher.deactivate_watch", "xpack-watcher.deactivate_watch") diff --git a/x-pack/plugin/watcher/qa/rest/src/yamlRestTest/resources/rest-api-spec/test/painless/40_exception.yml b/x-pack/plugin/watcher/qa/rest/src/yamlRestTest/resources/rest-api-spec/test/painless/40_exception.yml index 411ef8426552a..e65d7280e1173 100644 --- a/x-pack/plugin/watcher/qa/rest/src/yamlRestTest/resources/rest-api-spec/test/painless/40_exception.yml +++ b/x-pack/plugin/watcher/qa/rest/src/yamlRestTest/resources/rest-api-spec/test/painless/40_exception.yml @@ -37,6 +37,10 @@ --- "Test painless exceptions are returned when logging a broken response": + - skip: + version: " - 8.6.99" + reason: "self-referencing objects were fixed to be in Painless instead of Mustache in 8.7" + - do: cluster.health: wait_for_status: green @@ -75,36 +79,5 @@ - match: { watch_record.trigger_event.type: "manual" } - match: { watch_record.state: "executed" } - match: { watch_record.result.actions.0.status: "failure" } - - match: { watch_record.result.actions.0.error.caused_by.caused_by.type: "illegal_argument_exception" } - - match: { watch_record.result.actions.0.error.caused_by.caused_by.reason: "Iterable object is self-referencing itself (CustomReflectionObjectHandler stringify)" } - - - do: - catch: bad_request - watcher.execute_watch: - body: > - { - "watch": { - "trigger": { - "schedule": { - "interval": "10s" - } - }, - "input": { - "simple": { - "foo": "bar" - } - }, - "actions": { - "my-logging": { - "transform": { - "script": { - "source": "def x = [:] ; def y = [:] ; x.a = y ; y.a = x ; return x" - } - }, - "logging": { - "text": "{{#join}}ctx.payload{{/join}}" - } - } - } - } - } + - match: { watch_record.result.actions.0.reason: "Failed to transform payload" } + - match: { watch_record.result.actions.0.transform.error.reason: "Iterable object is self-referencing itself (watcher action payload)" }