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

Author: nik9000

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