Ensures watch definitions are valid json #30692

tomcallahan · 2018-05-17T16:52:00Z

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

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 elastic#29746

elasticmachine · 2018-05-17T16:52:03Z

Pinging @elastic/es-core-infra

rjernst

I left some comments. More generally, can you explain why this is not a problem with all XContentParsers? I don't see what WatcherXContentParser is doing that causes it to have different behavior.

rjernst · 2018-05-17T17:03:40Z

x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/watch/WatchTests.java

@@ -296,6 +298,38 @@ public void testParserBadActions() throws Exception {
        }
    }

+    public void testParserConsumesEntireDefinition() throws Exception {
+        WatchParser wp = createWatchparser();
+        String watchDef="{\n" +


Typically we use XContentBuilder to build the json (see the test below for an example).

WatcherXContentParser is not different -- the real issue is that we do not consume enough tokens to encounter an actual error. The real crux of this PR is line 183 -- consuming an additional token beyond what we "think" is the end of the watch definition to ensure that we have looked at the whole thing.

In the example test, we have an extra closing curly brace. Up to and including that curly brace, the json is invalid -- we have to consume beyond the erroneous end brace to get the error.

Re: XContentBuilder -- XContentBuilder won't let me build an invalid JSON.

You can use XContentBuilder to build valid json, and then append an extra token (eg comma as you have). This will ensure there isn't malformed json in the middle which tricks the test.

rjernst · 2018-05-17T17:04:32Z

x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/watch/WatchParser.java

+            if (parser.nextToken() != null) {
+                throw new ElasticsearchParseException("Received non-null token beyond end of watch definition!");
+            }
+        } catch (JsonParseException|ElasticsearchParseException e) {


We don't catch JsonParseException in the loop condition when calling parser.nextToken(), do we really need to here?

Fair enough -- I interpreted a desire to bubble up ElasticParseExceptions where possible, but if it is fine to just let that go up that makes sense.

rjernst · 2018-05-17T17:05:07Z

x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/watch/WatchParser.java

+                until we try to consume additional tokens.
+             */
+            if (parser.nextToken() != null) {
+                throw new ElasticsearchParseException("Received non-null token beyond end of watch definition!");


Can we set up this exception with all the information it needs, so we do not need to catch it and wrap below? It needlessly creates extra stack frames that must be read (by a human).

rjernst · 2018-05-17T17:07:25Z

x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/watch/WatchTests.java

+            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));


This can use the containsString matcher instead of a boolean assert (the output on failure is much nicer).

rjernst · 2018-05-17T17:07:44Z

x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/watch/WatchTests.java

+            "}";
+        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");


Use expectThrows instead of try/fail/catch

spinscale · 2018-05-18T07:58:00Z

testing this PR with the original JSON from the referenced issue returns a different exception form the JsonParser. I assume due to some xcontent refactorings in the master branch this is now properly bubbled up, compared to when the issue was created.

This issue here needs fixing as well though.

spinscale

LGTM, even though I think this fixes a slightly different issue now than what the original issue was about. We should always strive to only index valid JSON though!

spinscale · 2018-05-18T07:58:53Z

x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/watch/WatchParser.java

@@ -174,6 +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


minor nit: can you use a // comment like the other comments in the methods as well and add new lines before and after?

spinscale · 2018-05-18T08:03:50Z

x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/watch/WatchParser.java

+             until we try to consume additional tokens.
+        */
+        if (parser.nextToken() != null) {
+            throw new ElasticsearchParseException("could not parse watch [{}].  unexpected data beyond [line: {}, column: {}]",


minor nit: maybe we can come up with a more explaining error message here, as unexpected data can mean anything - however I don't have a smarter idea as well to be honest. Is it unparseable/unknown or just too much data/unneeded additional data?

@spinscale Actually it is this code that enables the JsonParseException for the original test case. Ryan asked that the code for the test case be changed, which resulted in a different error path. However, the key factor is that we now attempt to grab the next token beyond what we believe to be the last token of the watch definition. A previous revision of this code made that fact clear, at the expense of some more complex code.

yea I think what @spinscale is saying is that he would like the error message changed. Maybe expected end of payload/data/json but received additional data

@hub-cap I was trying to respond to @spinscale's comment above in the LGTM -- I'll rework the error message

hub-cap

Minor nit about the syntax used in the test, its not the end of the world by any means, so feel free to disregard

hub-cap · 2018-05-18T17:01:24Z

x-pack/plugin/watcher/src/main/java/org/elasticsearch/xpack/watcher/watch/WatchParser.java

+             until we try to consume additional tokens.
+        */
+        if (parser.nextToken() != null) {
+            throw new ElasticsearchParseException("could not parse watch [{}].  unexpected data beyond [line: {}, column: {}]",


yea I think what @spinscale is saying is that he would like the error message changed. Maybe expected end of payload/data/json but received additional data

hub-cap · 2018-05-18T17:10:36Z

x-pack/plugin/watcher/src/test/java/org/elasticsearch/xpack/watcher/watch/WatchTests.java

+    public void testParserConsumesEntireDefinition() throws Exception {
+        WatchParser wp = createWatchparser();
+        XContentBuilder jsonBuilder = jsonBuilder()
+            .startObject()


Any reason you did not opt for the syntax (yea i know, its a builder, lets chain it!!!) used in other spots in this class? Imho its harder to read the nested thing than blocks of code with spaces that define start/endings. This is also how we do it in most of our toXContent functions as well.

b = jsonBuilder(); b.startObject(); b.startObject("trigger"); b.startObject("schedule"); b.field("interval", "10s"); b.endObject(); b.endObject(); ... b.endObject();

We prefer using braces like this:

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 1ff59034831..dcb48fbd3c1 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,44 @@ public class WatchTests extends ESTestCase { 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)); + 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("unexpected data beyond")); + } + } public void testParserDefaults() throws Exception {

Also note that the builder should be wrapped in a try-with-resources; it's good hygiene.

Oh, ive not see this before but i like it. I almost removed a builder.endObject() on one of my first XContent forays, and I dont think i would have even considered it if this syntax was used.

Also, WRT the try-with-resources and hygiene, this is specifically for tests? or should we be doing this in the XContent methods as well? I assumed the caller was responsible for it in our request/response objects.

Here we own the lifecycle of the builder so we are responsible for closing it.

@hub-cap The way the class does it is mixed, when i started this PR i based style on testParserBadActions(), as that seemed to be the closest example to what I wanted to do. I am happy to conform to whatever is preferred.

That's the old style before we introduced using braces for this.

hub-cap · 2018-05-18T17:12:33Z

@tomcallahan It looks like you might be using the wrong user to commit these changes? Its showing the CLA check failing as well as showing 2 authors per commit msg Thomas Callahan authored and Thomas Callahan committed a day ago

tomcallahan · 2018-05-18T19:16:15Z

@hub-cap thanks I need to fix my git config for my cloud box that I use for heavy lifting :)

…json

tomcallahan · 2018-05-25T14:21:35Z

@rjernst expressed a concern re: putting this fix in when we have a widespread problem (that is, we do not reliably close XContentParsers throughout the codebase). If we reliably closed XContentParsers, we could put some logic in close() that ensures we have read the entire stream.

On Ryan's suggestion, I spent a few hours investigating this approach. I believe doing so will be quite a large task, and furthermore be a very large backport if we want this fixed in previous versions. While I don't necessarily want to make small fixes like this across the entire codebase, I believe this particular bug is a rather insidious case as it can cause the cluster to go off and do things it shouldn't again, and again, and again, all on its own.

I see three courses of action:

decline this PR, discuss the larger issue in es-core-infra, and drive towards a comprehensive fix quickly
merge this PR, discuss the larger issue in es-core-infra, and make a plan for a comprehensive fix at a pace of our choosing
merge this PR and ignore the larger issue

My gut-check preference is (2), but I want to do the right thing for the codebase -- What do you all think?

hub-cap · 2018-05-25T15:21:11Z

is this a bug that has hit a lot of users? Id be inclined to assign someone to work on fixing it everywhere (like we have in the transport layer that does not allow extra bytes to not be consumed). If this bug was hit by a lot of people in watcher, then id be inclined to say #2 is a better option, but if its just one or two people, id rather see #1 be the course of action. But thats just my 2 pennies.

tomcallahan added >bug v7.0.0 :Data Management/Watcher v6.4.0 labels May 17, 2018

tomcallahan self-assigned this May 17, 2018

tomcallahan requested review from hub-cap and spinscale May 17, 2018 16:52

rjernst reviewed May 17, 2018

View reviewed changes

Thomas Callahan added 2 commits May 17, 2018 13:43

address reviewer feedback

6da9e8b

address reviewer feedback

7252b00

spinscale approved these changes May 18, 2018

View reviewed changes

hub-cap approved these changes May 18, 2018

View reviewed changes

tomcallahan added 2 commits May 23, 2018 16:58

address reviewer feedback

9a4db9f

Merge remote-tracking branch 'origin/master' into ensure-watch-valid-…

dcd9b86

…json

tomcallahan mentioned this pull request Jun 21, 2018

Close xcontent parsers (partial) #31513

Merged

lcawl added v6.4.1 and removed v6.4.0 labels Aug 23, 2018

colings86 added the team-discuss label Jan 11, 2019

tomcallahan closed this Jan 15, 2019

colings86 added v7.0.0-beta1 and removed v7.0.0 labels Feb 7, 2019

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Ensures watch definitions are valid json #30692

Ensures watch definitions are valid json #30692

tomcallahan commented May 17, 2018

elasticmachine commented May 17, 2018

rjernst left a comment

rjernst May 17, 2018

tomcallahan May 17, 2018

tomcallahan May 17, 2018

rjernst May 17, 2018

rjernst May 17, 2018

tomcallahan May 17, 2018

rjernst May 17, 2018

rjernst May 17, 2018

rjernst May 17, 2018

spinscale commented May 18, 2018

spinscale left a comment

spinscale May 18, 2018

spinscale May 18, 2018

tomcallahan May 18, 2018

hub-cap May 18, 2018

tomcallahan May 18, 2018

hub-cap left a comment

hub-cap May 18, 2018

hub-cap May 18, 2018

jasontedor May 18, 2018

hub-cap May 18, 2018

jasontedor May 18, 2018

tomcallahan May 18, 2018

jasontedor May 18, 2018

hub-cap commented May 18, 2018

tomcallahan commented May 18, 2018

tomcallahan commented May 25, 2018

hub-cap commented May 25, 2018

Ensures watch definitions are valid json #30692

Ensures watch definitions are valid json #30692

Conversation

tomcallahan commented May 17, 2018

elasticmachine commented May 17, 2018

rjernst left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

spinscale commented May 18, 2018

spinscale left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hub-cap left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

hub-cap commented May 18, 2018

tomcallahan commented May 18, 2018

tomcallahan commented May 25, 2018

hub-cap commented May 25, 2018