XContent: Check for bad parsers

client/rest-high-level/src/main/java/org/elasticsearch/client/security/PutRoleMappingResponse.java

@@ -64,11 +64,10 @@ public int hashCode() {
     private static final ConstructingObjectParser<PutRoleMappingResponse, Void> 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<Boolean, Void> 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 {

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:

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<Object> testArray) {
         assertThat(ex.getMessage(), containsString("[foo] failed to parse field [int_array]"));
     }
 
+    public void testNoopDeclareObject() throws IOException {
+        ObjectParser<AtomicReference<String>, 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<AtomicReference<String>, 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<AtomicReference<String>, 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<NamedObjectHolder, Void> PARSER = new ObjectParser<>("named_object_holder",
                 NamedObjectHolder::new);