From e3a3a83ca3717556290c4338c320c34b3fb3f1ef Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Tue, 16 Oct 2018 16:37:28 -0400 Subject: [PATCH 1/4] XContent: Check for bad parsers Adds checks for misbehaving parsers. Closes #34351 --- .../common/xcontent/ObjectParser.java | 6 +++ .../common/xcontent/ObjectParserTests.java | 44 +++++++++++++++++++ 2 files changed, 50 insertions(+) diff --git a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ObjectParser.java b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ObjectParser.java index d0cc929b56d24..a19e56277c108 100644 --- a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ObjectParser.java +++ b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ObjectParser.java @@ -324,9 +324,15 @@ private void parseSub(XContentParser parser, FieldParser fieldParser, String cur switch (token) { case START_OBJECT: parseValue(parser, fieldParser, currentFieldName, value, context); + if (parser.currentToken() != XContentParser.Token.END_OBJECT) { + throw new IllegalStateException("parser for [" + currentFieldName + "] did not end on END_OBJECT"); + } break; case START_ARRAY: parseArray(parser, fieldParser, currentFieldName, value, context); + if (parser.currentToken() != XContentParser.Token.END_ARRAY) { + throw new IllegalStateException("parser for [" + currentFieldName + "] did not end on END_ARRAY"); + } break; case END_OBJECT: case END_ARRAY: diff --git a/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/ObjectParserTests.java b/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/ObjectParserTests.java index 42d53bf49859b..889f1619614aa 100644 --- a/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/ObjectParserTests.java +++ b/libs/x-content/src/test/java/org/elasticsearch/common/xcontent/ObjectParserTests.java @@ -34,6 +34,7 @@ import java.util.Arrays; import java.util.Collections; import java.util.List; +import java.util.concurrent.atomic.AtomicReference; import static org.hamcrest.Matchers.containsString; import static org.hamcrest.Matchers.hasSize; @@ -650,6 +651,49 @@ public void setArray(List testArray) { assertThat(ex.getMessage(), containsString("[foo] failed to parse field [int_array]")); } + public void testNoopDeclareObject() throws IOException { + ObjectParser, Void> parser = new ObjectParser<>("noopy", AtomicReference::new); + parser.declareString(AtomicReference::set, new ParseField("body")); + parser.declareObject((a,b) -> {}, (p, c) -> null, new ParseField("noop")); + + assertEquals("i", parser.parse(createParser(JsonXContent.jsonXContent, "{\"body\": \"i\"}"), null).get()); + Exception garbageException = expectThrows(IllegalStateException.class, () -> parser.parse( + createParser(JsonXContent.jsonXContent, "{\"noop\": {\"garbage\": \"shouldn't\"}}"), + null)); + assertEquals("parser for [noop] did not end on END_OBJECT", garbageException.getMessage()); + Exception sneakyException = expectThrows(IllegalStateException.class, () -> parser.parse( + createParser(JsonXContent.jsonXContent, "{\"noop\": {\"body\": \"shouldn't\"}}"), + null)); + assertEquals("parser for [noop] did not end on END_OBJECT", sneakyException.getMessage()); + } + + public void testNoopDeclareField() throws IOException { + ObjectParser, Void> parser = new ObjectParser<>("noopy", AtomicReference::new); + parser.declareString(AtomicReference::set, new ParseField("body")); + parser.declareField((a,b) -> {}, (p, c) -> null, new ParseField("noop"), ValueType.STRING_ARRAY); + + assertEquals("i", parser.parse(createParser(JsonXContent.jsonXContent, "{\"body\": \"i\"}"), null).get()); + Exception e = expectThrows(IllegalStateException.class, () -> parser.parse( + createParser(JsonXContent.jsonXContent, "{\"noop\": [\"ignored\"]}"), + null)); + assertEquals("parser for [noop] did not end on END_ARRAY", e.getMessage()); + } + + public void testNoopDeclareObjectArray() throws IOException { + ObjectParser, Void> parser = new ObjectParser<>("noopy", AtomicReference::new); + parser.declareString(AtomicReference::set, new ParseField("body")); + parser.declareObjectArray((a,b) -> {}, (p, c) -> null, new ParseField("noop")); + + XContentParseException garbageError = expectThrows(XContentParseException.class, () -> parser.parse( + createParser(JsonXContent.jsonXContent, "{\"noop\": [{\"garbage\": \"shouldn't\"}}]"), + null)); + assertEquals("expected value but got [FIELD_NAME]", garbageError.getCause().getMessage()); + XContentParseException sneakyError = expectThrows(XContentParseException.class, () -> parser.parse( + createParser(JsonXContent.jsonXContent, "{\"noop\": [{\"body\": \"shouldn't\"}}]"), + null)); + assertEquals("expected value but got [FIELD_NAME]", sneakyError.getCause().getMessage()); + } + static class NamedObjectHolder { public static final ObjectParser PARSER = new ObjectParser<>("named_object_holder", NamedObjectHolder::new); From a466a773fd79289dd6953c0c5fa7cf69d6bd8247 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Wed, 17 Oct 2018 11:08:43 -0400 Subject: [PATCH 2/4] Fix role mapping --- .../client/security/PutRoleMappingResponse.java | 9 ++++----- 1 file changed, 4 insertions(+), 5 deletions(-) diff --git a/client/rest-high-level/src/main/java/org/elasticsearch/client/security/PutRoleMappingResponse.java b/client/rest-high-level/src/main/java/org/elasticsearch/client/security/PutRoleMappingResponse.java index 04cdb14163e3e..4c6742db0c835 100644 --- a/client/rest-high-level/src/main/java/org/elasticsearch/client/security/PutRoleMappingResponse.java +++ b/client/rest-high-level/src/main/java/org/elasticsearch/client/security/PutRoleMappingResponse.java @@ -64,11 +64,10 @@ public int hashCode() { private static final ConstructingObjectParser PARSER = new ConstructingObjectParser<>( "put_role_mapping_response", true, args -> new PutRoleMappingResponse((boolean) args[0])); static { - PARSER.declareBoolean(constructorArg(), new ParseField("created")); - // To parse the "created" field we declare "role_mapping" field object. - // Once the nested field "created" is found parser constructs the target object and - // ignores the role_mapping object. - PARSER.declareObject((a,b) -> {}, (parser, context) -> null, new ParseField("role_mapping")); + ConstructingObjectParser roleMappingParser = new ConstructingObjectParser<>( + "put_role_mapping_response.role_mapping", true, args -> (boolean) args[0]); + roleMappingParser.declareBoolean(constructorArg(), new ParseField("created")); + PARSER.declareObject(constructorArg(), roleMappingParser::parse, new ParseField("role_mapping")); } public static PutRoleMappingResponse fromXContent(XContentParser parser) throws IOException { From f4ebb49144ef86b53c518eb5ed147198c49cf149 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Wed, 17 Oct 2018 11:46:48 -0400 Subject: [PATCH 3/4] The cast --- .../elasticsearch/client/security/PutRoleMappingResponse.java | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/client/rest-high-level/src/main/java/org/elasticsearch/client/security/PutRoleMappingResponse.java b/client/rest-high-level/src/main/java/org/elasticsearch/client/security/PutRoleMappingResponse.java index 4c6742db0c835..00039f1486e1f 100644 --- a/client/rest-high-level/src/main/java/org/elasticsearch/client/security/PutRoleMappingResponse.java +++ b/client/rest-high-level/src/main/java/org/elasticsearch/client/security/PutRoleMappingResponse.java @@ -65,7 +65,7 @@ public int hashCode() { "put_role_mapping_response", true, args -> new PutRoleMappingResponse((boolean) args[0])); static { ConstructingObjectParser roleMappingParser = new ConstructingObjectParser<>( - "put_role_mapping_response.role_mapping", true, args -> (boolean) args[0]); + "put_role_mapping_response.role_mapping", true, args -> (Boolean) args[0]); roleMappingParser.declareBoolean(constructorArg(), new ParseField("created")); PARSER.declareObject(constructorArg(), roleMappingParser::parse, new ParseField("role_mapping")); } From c08e1c43b9a23b2e9968d63da28dd8d958a7ed33 Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Wed, 17 Oct 2018 11:53:03 -0400 Subject: [PATCH 4/4] Comment --- .../common/xcontent/ObjectParser.java | 16 ++++++++++++++++ 1 file changed, 16 insertions(+) diff --git a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ObjectParser.java b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ObjectParser.java index a19e56277c108..219c3c5bbbae4 100644 --- a/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ObjectParser.java +++ b/libs/x-content/src/main/java/org/elasticsearch/common/xcontent/ObjectParser.java @@ -324,12 +324,28 @@ private void parseSub(XContentParser parser, FieldParser fieldParser, String cur switch (token) { case START_OBJECT: parseValue(parser, fieldParser, currentFieldName, value, context); + /* + * Well behaving parsers should consume the entire object but + * asserting that they do that is not something we can do + * efficiently here. Instead we can check that they end on an + * END_OBJECT. They could end on the *wrong* end object and + * this test won't catch them, but that is the price that we pay + * for having a cheap test. + */ if (parser.currentToken() != XContentParser.Token.END_OBJECT) { throw new IllegalStateException("parser for [" + currentFieldName + "] did not end on END_OBJECT"); } break; case START_ARRAY: parseArray(parser, fieldParser, currentFieldName, value, context); + /* + * Well behaving parsers should consume the entire array but + * asserting that they do that is not something we can do + * efficiently here. Instead we can check that they end on an + * END_ARRAY. They could end on the *wrong* end array and + * this test won't catch them, but that is the price that we pay + * for having a cheap test. + */ if (parser.currentToken() != XContentParser.Token.END_ARRAY) { throw new IllegalStateException("parser for [" + currentFieldName + "] did not end on END_ARRAY"); }