From 3cde1356c1f2c9dca23be1ded4627093c0d3477c Mon Sep 17 00:00:00 2001 From: Nik Everett Date: Thu, 25 Oct 2018 17:03:42 -0400 Subject: [PATCH] XContent: Check for bad parsers (#34561) Adds checks for misbehaving parsers. The checks aren't perfect at all but they are simple and fast enough that we can do them all the time so they'll catch most badly behaving parsers. Closes #34351 --- .../security/PutRoleMappingResponse.java | 9 ++-- .../common/xcontent/ObjectParser.java | 22 ++++++++++ .../common/xcontent/ObjectParserTests.java | 44 +++++++++++++++++++ 3 files changed, 70 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..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 @@ -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 { 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..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,9 +324,31 @@ 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"); + } 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);