RestRequest parameter parsing should not cause 500 error (elastic#112605

) Currently, RestRequest throws IllegalStateExceptions in a few places which leads to 500 errors on the client side and e.g. turn up as warnings in the "rest.suppressed" error log. By changing them to IllegalArgumentExceptions we can change these to 400 (Bad Request) error codes. Closes elastic#112604
cbuescher · Sep 9, 2024 · a587c7d · a587c7d
1 parent fc2760c
commit a587c7d
Show file tree

Hide file tree

Showing 3 changed files with 17 additions and 8 deletions.
diff --git a/server/src/main/java/org/elasticsearch/rest/RestRequest.java b/server/src/main/java/org/elasticsearch/rest/RestRequest.java
@@ -12,6 +12,7 @@
 import org.elasticsearch.ElasticsearchParseException;
 import org.elasticsearch.ElasticsearchStatusException;
 import org.elasticsearch.common.Strings;
+import org.elasticsearch.common.ValidationException;
 import org.elasticsearch.common.bytes.BytesArray;
 import org.elasticsearch.common.bytes.BytesReference;
 import org.elasticsearch.common.unit.ByteSizeValue;
@@ -321,11 +322,17 @@ public final BytesReference requiredContent() {
         if (hasContent() == false) {
             throw new ElasticsearchParseException("request body is required");
         } else if (xContentType.get() == null) {
-            throw new IllegalStateException("unknown content type");
+            throwValidationException("unknown content type");
         }
         return content();
     }
 
+    private static void throwValidationException(String msg) {
+        ValidationException unknownContentType = new ValidationException();
+        unknownContentType.addValidationError(msg);
+        throw unknownContentType;
+    }
+
     /**
      * Get the value of the header or {@code null} if not found. This method only retrieves the first header value if multiple values are
      * sent. Use of {@link #getAllHeaderValues(String)} should be preferred
@@ -585,12 +592,12 @@ public final Tuple<XContentType, BytesReference> contentOrSourceParam() {
         String source = param("source");
         String typeParam = param("source_content_type");
         if (source == null || typeParam == null) {
-            throw new IllegalStateException("source and source_content_type parameters are required");
+            throwValidationException("source and source_content_type parameters are required");
         }
         BytesArray bytes = new BytesArray(source);
         final XContentType xContentType = parseContentType(Collections.singletonList(typeParam));
         if (xContentType == null) {
-            throw new IllegalStateException("Unknown value for source_content_type [" + typeParam + "]");
+            throwValidationException("Unknown value for source_content_type [" + typeParam + "]");
         }
         return new Tuple<>(xContentType, bytes);
     }

diff --git a/server/src/test/java/org/elasticsearch/rest/RestRequestTests.java b/server/src/test/java/org/elasticsearch/rest/RestRequestTests.java
@@ -9,6 +9,7 @@
 package org.elasticsearch.rest;
 
 import org.elasticsearch.ElasticsearchParseException;
+import org.elasticsearch.common.ValidationException;
 import org.elasticsearch.common.bytes.BytesArray;
 import org.elasticsearch.common.bytes.BytesReference;
 import org.elasticsearch.core.CheckedConsumer;
@@ -33,6 +34,7 @@
 import static java.util.Collections.singletonMap;
 import static org.elasticsearch.rest.RestRequest.OPERATOR_REQUEST;
 import static org.elasticsearch.rest.RestRequest.SERVERLESS_REQUEST;
+import static org.hamcrest.Matchers.containsString;
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.instanceOf;
 import static org.hamcrest.Matchers.is;
@@ -130,8 +132,8 @@ public void testContentOrSourceParam() throws IOException {
                 .contentOrSourceParam()
                 .v2()
         );
-        e = expectThrows(IllegalStateException.class, () -> contentRestRequest("", Map.of("source", "stuff2")).contentOrSourceParam());
-        assertEquals("source and source_content_type parameters are required", e.getMessage());
+        e = expectThrows(ValidationException.class, () -> contentRestRequest("", Map.of("source", "stuff2")).contentOrSourceParam());
+        assertThat(e.getMessage(), containsString("source and source_content_type parameters are required"));
     }
 
     public void testHasContentOrSourceParam() throws IOException {
@@ -246,8 +248,8 @@ public void testRequiredContent() {
                 .requiredContent()
         );
         assertEquals("request body is required", e.getMessage());
-        e = expectThrows(IllegalStateException.class, () -> contentRestRequest("test", null, Collections.emptyMap()).requiredContent());
-        assertEquals("unknown content type", e.getMessage());
+        e = expectThrows(ValidationException.class, () -> contentRestRequest("test", null, Collections.emptyMap()).requiredContent());
+        assertThat(e.getMessage(), containsString("unknown content type"));
     }
 
     public void testIsServerlessRequest() {

diff --git a/.../security/src/test/java/org/elasticsearch/xpack/security/rest/RestRequestFilterTests.java b/.../security/src/test/java/org/elasticsearch/xpack/security/rest/RestRequestFilterTests.java
@@ -115,7 +115,7 @@ public void testFilterUnknownContentTypeThrows() throws IOException {
         }
         RestRequestFilter filter = () -> Collections.singleton("root.second.third");
         RestRequest filtered = filter.getFilteredRequest(restRequest);
-        IllegalStateException e = expectThrows(IllegalStateException.class, () -> filtered.content());
+        Exception e = expectThrows(IllegalArgumentException.class, () -> filtered.content());
         assertThat(e.getMessage(), containsString("unknown content type"));
     }