From fe93640e439ed790c5c1e174beb9812e5302eee5 Mon Sep 17 00:00:00 2001 From: Alexander Reelsen Date: Wed, 2 Nov 2016 10:37:41 +0100 Subject: [PATCH] Watcher: Be strict with chain input parsing (elastic/elasticsearch#3873) When parsing chain inputs there were possibilities to write invalid JSON that resulting in losing the order of the inputs without any exception being thrown. This commit makes the parsing more strict. Closes elastic/elasticsearch#3736 Original commit: elastic/x-pack-elasticsearch@963641ee2bd3fc49e3502eed9966f2931eac49e1 --- .../xpack/watcher/input/chain/ChainInput.java | 35 +++++++--- .../watcher/input/chain/ChainInputTests.java | 66 ++++++++++++++++++- 2 files changed, 89 insertions(+), 12 deletions(-) diff --git a/elasticsearch/src/main/java/org/elasticsearch/xpack/watcher/input/chain/ChainInput.java b/elasticsearch/src/main/java/org/elasticsearch/xpack/watcher/input/chain/ChainInput.java index bdca63e9377..6c33045d322 100644 --- a/elasticsearch/src/main/java/org/elasticsearch/xpack/watcher/input/chain/ChainInput.java +++ b/elasticsearch/src/main/java/org/elasticsearch/xpack/watcher/input/chain/ChainInput.java @@ -5,6 +5,7 @@ */ package org.elasticsearch.xpack.watcher.input.chain; +import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.common.ParseField; import org.elasticsearch.common.collect.Tuple; import org.elasticsearch.common.xcontent.XContentBuilder; @@ -21,7 +22,9 @@ import java.util.List; public class ChainInput implements Input { public static final String TYPE = "chain"; - private List> inputs; + public static final ParseField INPUTS = new ParseField("inputs"); + + private final List> inputs; public ChainInput(List> inputs) { this.inputs = inputs; @@ -35,7 +38,7 @@ public class ChainInput implements Input { @Override public XContentBuilder toXContent(XContentBuilder builder, Params params) throws IOException { builder.startObject(); - builder.startArray("inputs"); + builder.startArray(INPUTS.getPreferredName()); for (Tuple tuple : inputs) { builder.startObject().startObject(tuple.v1()); builder.field(tuple.v2().type(), tuple.v2()); @@ -56,20 +59,15 @@ public class ChainInput implements Input { String currentFieldName; XContentParser.Token token; - ParseField inputsField = new ParseField("inputs"); - while ((token = parser.nextToken()) != XContentParser.Token.END_OBJECT) { if (token == XContentParser.Token.FIELD_NAME) { currentFieldName = parser.currentName(); token = parser.nextToken(); - if (token == XContentParser.Token.START_ARRAY && inputsField.getPreferredName().equals(currentFieldName)) { - String currentInputFieldName = null; + if (token == XContentParser.Token.START_ARRAY && INPUTS.getPreferredName().equals(currentFieldName)) { while ((token = parser.nextToken()) != XContentParser.Token.END_ARRAY) { if (token == XContentParser.Token.FIELD_NAME) { - currentInputFieldName = parser.currentName(); - } else if (currentInputFieldName != null && token == XContentParser.Token.START_OBJECT) { - inputs.add(new Tuple<>(currentInputFieldName, inputRegistry.parse(watchId, parser).input())); - currentInputFieldName = null; + String inputName = parser.currentName(); + inputs.add(new Tuple<>(inputName, parseSingleInput(watchId, inputName, parser, inputRegistry))); } } } @@ -79,6 +77,23 @@ public class ChainInput implements Input { return new ChainInput(inputs); } + private static Input parseSingleInput(String watchId, String name, XContentParser parser, + InputRegistry inputRegistry) throws IOException { + if (parser.nextToken() != XContentParser.Token.START_OBJECT) { + throw new ElasticsearchParseException("Expected starting JSON object after [{}] in watch [{}]", name, watchId); + } + + Input input = inputRegistry.parse(watchId, parser).input(); + + // expecting closing of two json object to start the next element in the array + if (parser.currentToken() != XContentParser.Token.END_OBJECT || parser.nextToken() != XContentParser.Token.END_OBJECT) { + throw new ElasticsearchParseException("Expected closing JSON object after parsing input [{}] named [{}] in watch [{}]", + input.type(), name, watchId); + } + + return input; + } + public static ChainInput.Builder builder() { return new Builder(); } diff --git a/elasticsearch/src/test/java/org/elasticsearch/xpack/watcher/input/chain/ChainInputTests.java b/elasticsearch/src/test/java/org/elasticsearch/xpack/watcher/input/chain/ChainInputTests.java index ef53f8eef74..c4c6da46cab 100644 --- a/elasticsearch/src/test/java/org/elasticsearch/xpack/watcher/input/chain/ChainInputTests.java +++ b/elasticsearch/src/test/java/org/elasticsearch/xpack/watcher/input/chain/ChainInputTests.java @@ -6,6 +6,7 @@ package org.elasticsearch.xpack.watcher.input.chain; import org.elasticsearch.ElasticsearchException; +import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.unit.TimeValue; import org.elasticsearch.common.xcontent.ToXContent; @@ -101,8 +102,8 @@ public class ChainInputTests extends ESTestCase { assertThat(payload.data().get("second"), instanceOf(Map.class)); // final payload check - Map firstPayload = (Map) payload.data().get("first"); - Map secondPayload = (Map) payload.data().get("second"); + Map firstPayload = (Map) payload.data().get("first"); + Map secondPayload = (Map) payload.data().get("second"); assertThat(firstPayload, hasEntry("foo", "bar")); assertThat(secondPayload, hasEntry("spam", "eggs")); } @@ -167,6 +168,67 @@ public class ChainInputTests extends ESTestCase { assertThat(builder.bytes().utf8ToString(), containsString("\"reason\":\"ElasticsearchException[foo]\"")); } + /* https://github.com/elastic/x-plugins/issues/3736 + the issue here is, that first/second in this setup do not have a guaranteed order, so we have to throw an exception + { + "inputs" : [ + { + "first" : { "simple" : { "foo" : "bar" } }, + "second" : { "simple" : { "spam" : "eggs" } } + } + ] + } + */ + public void testParsingShouldBeStrictWhenClosingInputs() throws Exception { + Map factories = new HashMap<>(); + factories.put("simple", new SimpleInputFactory(Settings.EMPTY)); + + InputRegistry inputRegistry = new InputRegistry(Settings.EMPTY, factories); + ChainInputFactory chainInputFactory = new ChainInputFactory(Settings.EMPTY, inputRegistry); + factories.put("chain", chainInputFactory); + + XContentBuilder builder = jsonBuilder().startObject().startArray("inputs").startObject() + .startObject("first").startObject("simple").field("foo", "bar").endObject().endObject() + .startObject("second").startObject("simple").field("spam", "eggs").endObject().endObject() + .endObject().endArray().endObject(); + + XContentParser parser = XContentFactory.xContent(builder.bytes()).createParser(builder.bytes()); + parser.nextToken(); + ElasticsearchParseException e = + expectThrows(ElasticsearchParseException.class, () -> chainInputFactory.parseInput("test", parser, false)); + assertThat(e.getMessage(), + containsString("Expected closing JSON object after parsing input [simple] named [first] in watch [test]")); + } + + /* https://github.com/elastic/x-plugins/issues/3736 + make sure that after the name of a chained input there is always an object + { + "inputs" : [ + { "first" : [ { "simple" : { "foo" : "bar" } } ] } + ] + } + */ + public void testParsingShouldBeStrictWhenStartingInputs() throws Exception { + Map factories = new HashMap<>(); + factories.put("simple", new SimpleInputFactory(Settings.EMPTY)); + + InputRegistry inputRegistry = new InputRegistry(Settings.EMPTY, factories); + ChainInputFactory chainInputFactory = new ChainInputFactory(Settings.EMPTY, inputRegistry); + factories.put("chain", chainInputFactory); + + XContentBuilder builder = jsonBuilder().startObject().startArray("inputs") + .startObject().startArray("first").startObject() + .startObject("simple").field("foo", "bar").endObject() + .endObject().endArray().endObject() + .endArray().endObject(); + + XContentParser parser = XContentFactory.xContent(builder.bytes()).createParser(builder.bytes()); + parser.nextToken(); + ElasticsearchParseException e = + expectThrows(ElasticsearchParseException.class, () -> chainInputFactory.parseInput("test", parser, false)); + assertThat(e.getMessage(), containsString("Expected starting JSON object after [first] in watch [test]")); + } + private WatchExecutionContext createContext() { Watch watch = new Watch("test-watch", new ScheduleTrigger(new IntervalSchedule(new IntervalSchedule.Interval(1, IntervalSchedule.Interval.Unit.MINUTES))),