From 36d0c12dba81fa6348a75f483d130e21723828f4 Mon Sep 17 00:00:00 2001 From: Thomas Callahan Date: Thu, 17 May 2018 12:40:15 -0400 Subject: [PATCH 1/4] Ensures watch definitions are valid json Currently, watches may be submitted and accepted by the cluster that do not consist of valid JSON. This is because in certain cases, WatchParser.java will not consume enough tokens from XContentParser to be able to encounter an underlying JSON parse error. This patch fixes this behavior and adds a test. Closes #29746 --- .../xpack/watcher/watch/WatchParser.java | 14 ++++++++ .../xpack/watcher/watch/WatchTests.java | 34 +++++++++++++++++++ 2 files changed, 48 insertions(+) diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/watch/WatchParser.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/watch/WatchParser.java index 524913105823a..2317cc2442788 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/watch/WatchParser.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/watch/WatchParser.java @@ -5,6 +5,7 @@ */ package org.elasticsearch.xpack.watcher.watch; +import com.fasterxml.jackson.core.JsonParseException; import org.elasticsearch.ElasticsearchParseException; import org.elasticsearch.common.Nullable; import org.elasticsearch.common.bytes.BytesReference; @@ -174,6 +175,19 @@ public Watch parse(String id, boolean includeStatus, WatcherXContentParser parse throw new ElasticsearchParseException("could not parse watch [{}]. unexpected field [{}]", id, currentFieldName); } } + + try { + /* Make sure we are at the end of the available input data -- certain types of JSON errors will not manifest + until we try to consume additional tokens. + */ + if (parser.nextToken() != null) { + throw new ElasticsearchParseException("Received non-null token beyond end of watch definition!"); + } + } catch (JsonParseException|ElasticsearchParseException e) { + throw new ElasticsearchParseException("could not parse watch [{}]. unexpected data beyond [line: {}, column: {}]", + e, id, parser.getTokenLocation().lineNumber, parser.getTokenLocation().columnNumber); + } + if (trigger == null) { throw new ElasticsearchParseException("could not parse watch [{}]. missing required field [{}]", id, WatchField.TRIGGER.getPreferredName()); diff --git a/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/watch/WatchTests.java b/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/watch/WatchTests.java index df96a802166e2..0ec01f459b670 100644 --- a/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/watch/WatchTests.java +++ b/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/watch/WatchTests.java @@ -11,6 +11,7 @@ import org.elasticsearch.action.support.WriteRequest; import org.elasticsearch.client.Client; import org.elasticsearch.common.ParseField; +import org.elasticsearch.common.bytes.BytesArray; import org.elasticsearch.common.bytes.BytesReference; import org.elasticsearch.common.logging.Loggers; import org.elasticsearch.common.settings.Settings; @@ -19,6 +20,7 @@ import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.common.xcontent.XContentType; +import org.elasticsearch.common.xcontent.json.JsonXContent; import org.elasticsearch.index.query.MatchAllQueryBuilder; import org.elasticsearch.index.query.QueryBuilder; import org.elasticsearch.index.query.ScriptQueryBuilder; @@ -296,6 +298,38 @@ public void testParserBadActions() throws Exception { } } + public void testParserConsumesEntireDefinition() throws Exception { + WatchParser wp = createWatchparser(); + String watchDef="{\n" + + " \"trigger\": {\n" + + " \"schedule\": {\n" + + " \"interval\": \"10s\"\n" + + " }\n" + + " },\n" + + " \"input\": {\n" + + " \"simple\": {}\n" + + " }},\n" + /* NOTICE EXTRA CLOSING CURLY BRACE */ + " \"condition\": {\n" + + " \"script\": {\n" + + " \"inline\": \"return false\"\n" + + " }\n" + + " },\n" + + " \"actions\": {\n" + + " \"logging\": {\n" + + " \"logging\": {\n" + + " \"text\": \"{{ctx.payload}}\"\n" + + " }\n" + + " }\n" + + " }\n" + + "}"; + try { + wp.parseWithSecrets("failure", false, new BytesArray(watchDef), new DateTime(), XContentType.JSON, true); + fail("This watch should fail to parse as there is an extra closing curly brace in the middle of the watch"); + } catch (ElasticsearchParseException pe) { + assertThat(pe.getMessage().contains("unexpected data beyond"), is(true)); + } + } + public void testParserDefaults() throws Exception { Schedule schedule = randomSchedule(); ScheduleRegistry scheduleRegistry = registry(schedule); From 6da9e8b5d52632cde783c27351316a24ab17eba0 Mon Sep 17 00:00:00 2001 From: Thomas Callahan Date: Thu, 17 May 2018 13:43:24 -0400 Subject: [PATCH 2/4] address reviewer feedback --- .../xpack/watcher/watch/WatchParser.java | 16 +++++----------- .../xpack/watcher/watch/WatchTests.java | 12 +++++------- 2 files changed, 10 insertions(+), 18 deletions(-) diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/watch/WatchParser.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/watch/WatchParser.java index 2317cc2442788..2f24d3923b57c 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/watch/WatchParser.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/watch/WatchParser.java @@ -175,19 +175,13 @@ public Watch parse(String id, boolean includeStatus, WatcherXContentParser parse throw new ElasticsearchParseException("could not parse watch [{}]. unexpected field [{}]", id, currentFieldName); } } - - try { - /* Make sure we are at the end of the available input data -- certain types of JSON errors will not manifest - until we try to consume additional tokens. - */ - if (parser.nextToken() != null) { - throw new ElasticsearchParseException("Received non-null token beyond end of watch definition!"); - } - } catch (JsonParseException|ElasticsearchParseException e) { + /* Make sure we are at the end of the available input data -- certain types of JSON errors will not manifest + until we try to consume additional tokens. + */ + if (parser.nextToken() != null) { throw new ElasticsearchParseException("could not parse watch [{}]. unexpected data beyond [line: {}, column: {}]", - e, id, parser.getTokenLocation().lineNumber, parser.getTokenLocation().columnNumber); + id, parser.getTokenLocation().lineNumber, parser.getTokenLocation().columnNumber); } - if (trigger == null) { throw new ElasticsearchParseException("could not parse watch [{}]. missing required field [{}]", id, WatchField.TRIGGER.getPreferredName()); diff --git a/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/watch/WatchTests.java b/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/watch/WatchTests.java index 0ec01f459b670..72306161a5cb4 100644 --- a/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/watch/WatchTests.java +++ b/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/watch/WatchTests.java @@ -20,7 +20,6 @@ import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.common.xcontent.XContentType; -import org.elasticsearch.common.xcontent.json.JsonXContent; import org.elasticsearch.index.query.MatchAllQueryBuilder; import org.elasticsearch.index.query.QueryBuilder; import org.elasticsearch.index.query.ScriptQueryBuilder; @@ -145,6 +144,8 @@ import static org.elasticsearch.xpack.watcher.input.InputBuilders.searchInput; import static org.elasticsearch.xpack.watcher.test.WatcherTestUtils.templateRequest; import static org.elasticsearch.xpack.watcher.trigger.TriggerBuilders.schedule; + +import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.equalTo; import static org.hamcrest.Matchers.hasSize; import static org.hamcrest.Matchers.instanceOf; @@ -322,12 +323,9 @@ public void testParserConsumesEntireDefinition() throws Exception { " }\n" + " }\n" + "}"; - try { - wp.parseWithSecrets("failure", false, new BytesArray(watchDef), new DateTime(), XContentType.JSON, true); - fail("This watch should fail to parse as there is an extra closing curly brace in the middle of the watch"); - } catch (ElasticsearchParseException pe) { - assertThat(pe.getMessage().contains("unexpected data beyond"), is(true)); - } + IOException e = expectThrows(IOException.class, () -> + wp.parseWithSecrets("failure", false, new BytesArray(watchDef), new DateTime(), XContentType.JSON, true)); + assertThat(e.getMessage(), containsString("could not parse watch")); } public void testParserDefaults() throws Exception { From 7252b0031dc4ddf49198bfe8f5c3d9332e0655bc Mon Sep 17 00:00:00 2001 From: Thomas Callahan Date: Thu, 17 May 2018 14:51:53 -0400 Subject: [PATCH 3/4] address reviewer feedback --- .../xpack/watcher/watch/WatchTests.java | 50 +++++++++---------- 1 file changed, 25 insertions(+), 25 deletions(-) diff --git a/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/watch/WatchTests.java b/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/watch/WatchTests.java index 72306161a5cb4..1ff59034831b9 100644 --- a/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/watch/WatchTests.java +++ b/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/watch/WatchTests.java @@ -16,10 +16,13 @@ import org.elasticsearch.common.logging.Loggers; import org.elasticsearch.common.settings.Settings; import org.elasticsearch.common.unit.TimeValue; +import org.elasticsearch.common.xcontent.DeprecationHandler; import org.elasticsearch.common.xcontent.NamedXContentRegistry; import org.elasticsearch.common.xcontent.XContentBuilder; import org.elasticsearch.common.xcontent.XContentParser; import org.elasticsearch.common.xcontent.XContentType; +import org.elasticsearch.common.xcontent.json.JsonXContent; +import org.elasticsearch.common.xcontent.json.JsonXContentParser; import org.elasticsearch.index.query.MatchAllQueryBuilder; import org.elasticsearch.index.query.QueryBuilder; import org.elasticsearch.index.query.ScriptQueryBuilder; @@ -122,6 +125,7 @@ import org.junit.Before; import java.io.IOException; +import java.nio.charset.StandardCharsets; import java.time.Clock; import java.time.Instant; import java.time.ZoneOffset; @@ -301,31 +305,27 @@ public void testParserBadActions() throws Exception { public void testParserConsumesEntireDefinition() throws Exception { WatchParser wp = createWatchparser(); - String watchDef="{\n" + - " \"trigger\": {\n" + - " \"schedule\": {\n" + - " \"interval\": \"10s\"\n" + - " }\n" + - " },\n" + - " \"input\": {\n" + - " \"simple\": {}\n" + - " }},\n" + /* NOTICE EXTRA CLOSING CURLY BRACE */ - " \"condition\": {\n" + - " \"script\": {\n" + - " \"inline\": \"return false\"\n" + - " }\n" + - " },\n" + - " \"actions\": {\n" + - " \"logging\": {\n" + - " \"logging\": {\n" + - " \"text\": \"{{ctx.payload}}\"\n" + - " }\n" + - " }\n" + - " }\n" + - "}"; - IOException e = expectThrows(IOException.class, () -> - wp.parseWithSecrets("failure", false, new BytesArray(watchDef), new DateTime(), XContentType.JSON, true)); - assertThat(e.getMessage(), containsString("could not parse watch")); + XContentBuilder jsonBuilder = jsonBuilder() + .startObject() + .startObject("trigger") + .startObject("schedule") + .field("interval", "10s") + .endObject() + .endObject() + .startObject("input") + .startObject("simple") + .endObject() + .endObject() + .startObject("condition") + .startObject("script") + .field("source", "return false") + .endObject() + .endObject() + .endObject(); + jsonBuilder.generator().writeBinary(",".getBytes(StandardCharsets.UTF_8)); + ElasticsearchParseException e = expectThrows(ElasticsearchParseException.class, () -> + wp.parseWithSecrets("failure", false, BytesReference.bytes(jsonBuilder), new DateTime(), XContentType.JSON, true)); + assertThat(e.getMessage(), containsString("unexpected data beyond")); } public void testParserDefaults() throws Exception { From 9a4db9ffadc0f75822cbe71b67daf36e2584c3a1 Mon Sep 17 00:00:00 2001 From: Tom Callahan Date: Wed, 23 May 2018 16:58:26 -0400 Subject: [PATCH 4/4] address reviewer feedback --- .../xpack/watcher/watch/WatchParser.java | 11 ++-- .../xpack/watcher/watch/WatchTests.java | 58 ++++++++++++------- 2 files changed, 43 insertions(+), 26 deletions(-) diff --git a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/watch/WatchParser.java b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/watch/WatchParser.java index 2f24d3923b57c..a825295c7c8a3 100644 --- a/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/watch/WatchParser.java +++ b/x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/watch/WatchParser.java @@ -175,12 +175,13 @@ public Watch parse(String id, boolean includeStatus, WatcherXContentParser parse throw new ElasticsearchParseException("could not parse watch [{}]. unexpected field [{}]", id, currentFieldName); } } - /* Make sure we are at the end of the available input data -- certain types of JSON errors will not manifest - until we try to consume additional tokens. - */ + + // Make sure we are at the end of the available input data -- certain types of JSON errors will not manifest + // until we try to consume additional tokens. + if (parser.nextToken() != null) { - throw new ElasticsearchParseException("could not parse watch [{}]. unexpected data beyond [line: {}, column: {}]", - id, parser.getTokenLocation().lineNumber, parser.getTokenLocation().columnNumber); + throw new ElasticsearchParseException("could not parse watch [{}]. expected end of payload, but received additional " + + "data at [line: {}, column: {}]", id, parser.getTokenLocation().lineNumber, parser.getTokenLocation().columnNumber); } if (trigger == null) { throw new ElasticsearchParseException("could not parse watch [{}]. missing required field [{}]", id, diff --git a/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/watch/WatchTests.java b/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/watch/WatchTests.java index 1ff59034831b9..09e94ed916505 100644 --- a/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/watch/WatchTests.java +++ b/x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/watch/WatchTests.java @@ -305,27 +305,43 @@ public void testParserBadActions() throws Exception { public void testParserConsumesEntireDefinition() throws Exception { WatchParser wp = createWatchparser(); - XContentBuilder jsonBuilder = jsonBuilder() - .startObject() - .startObject("trigger") - .startObject("schedule") - .field("interval", "10s") - .endObject() - .endObject() - .startObject("input") - .startObject("simple") - .endObject() - .endObject() - .startObject("condition") - .startObject("script") - .field("source", "return false") - .endObject() - .endObject() - .endObject(); - jsonBuilder.generator().writeBinary(",".getBytes(StandardCharsets.UTF_8)); - ElasticsearchParseException e = expectThrows(ElasticsearchParseException.class, () -> - wp.parseWithSecrets("failure", false, BytesReference.bytes(jsonBuilder), new DateTime(), XContentType.JSON, true)); - assertThat(e.getMessage(), containsString("unexpected data beyond")); + try (XContentBuilder builder = jsonBuilder()) { + builder.startObject(); + { + builder.startObject("trigger"); + { + builder.startObject("schedule"); + { + builder.field("interval", "10s"); + } + builder.endObject(); + } + builder.endObject(); + builder.startObject("input"); + { + builder.startObject("simple"); + { + } + builder.endObject(); + } + builder.endObject(); + builder.startObject("condition"); + { + builder.startObject("script"); + { + builder.field("source", "return false"); + } + builder.endObject(); + } + builder.endObject(); + } + builder.endObject(); + builder.generator().writeBinary(",".getBytes(StandardCharsets.UTF_8)); + ElasticsearchParseException e = expectThrows( + ElasticsearchParseException.class, + () -> wp.parseWithSecrets("failure", false, BytesReference.bytes(builder), new DateTime(), XContentType.JSON, true)); + assertThat(e.getMessage(), containsString("expected end of payload")); + } } public void testParserDefaults() throws Exception {