Do not hang on unsupported HTTP methods

server/src/main/java/org/elasticsearch/http/DefaultRestChannel.java

@@ -33,6 +33,7 @@
 import org.elasticsearch.rest.RestChannel;
 import org.elasticsearch.rest.RestRequest;
 import org.elasticsearch.rest.RestResponse;
+import org.elasticsearch.rest.RestStatus;
 
 import java.util.ArrayList;
 import java.util.List;
@@ -76,49 +77,54 @@ protected BytesStreamOutput newBytesOutput() {
 
     @Override
     public void sendResponse(RestResponse restResponse) {
-        HttpResponse httpResponse;
-        if (RestRequest.Method.HEAD == request.method()) {
-            httpResponse = httpRequest.createResponse(restResponse.status(), BytesArray.EMPTY);
-        } else {
-            httpResponse = httpRequest.createResponse(restResponse.status(), restResponse.content());
+        final ArrayList<Releasable> toClose = new ArrayList<>(3);
+        if (isCloseConnection()) {
+            toClose.add(() -> CloseableChannel.closeChannel(httpChannel));
         }
 
-        // TODO: Ideally we should move the setting of Cors headers into :server
-        // NioCorsHandler.setCorsResponseHeaders(nettyRequest, resp, corsConfig);
+        boolean success = false;
+        try {
+            final BytesReference content = restResponse.content();
+            if (content instanceof Releasable) {
+                toClose.add((Releasable) content);
+            }
 
-        String opaque = request.header(X_OPAQUE_ID);
-        if (opaque != null) {
-            setHeaderField(httpResponse, X_OPAQUE_ID, opaque);
-        }
+            BytesReference finalContent = content;
+            try {
+                if (request.method() == RestRequest.Method.HEAD) {
+                    finalContent = BytesArray.EMPTY;
+                }
+            } catch (IllegalArgumentException ignored) {
+                assert restResponse.status() == RestStatus.METHOD_NOT_ALLOWED :
+                    "request HTTP method is unsupported but HTTP status is not METHOD_NOT_ALLOWED(405)";
+            }
 
-        // Add all custom headers
-        addCustomHeaders(httpResponse, restResponse.getHeaders());
-        addCustomHeaders(httpResponse, threadContext.getResponseHeaders());
+            final HttpResponse httpResponse = httpRequest.createResponse(restResponse.status(), finalContent);
 
-        ArrayList<Releasable> toClose = new ArrayList<>(3);
+            // TODO: Ideally we should move the setting of Cors headers into :server
+            // NioCorsHandler.setCorsResponseHeaders(nettyRequest, resp, corsConfig);
+
+            String opaque = request.header(X_OPAQUE_ID);
+            if (opaque != null) {
+                setHeaderField(httpResponse, X_OPAQUE_ID, opaque);
+            }
+
+            // Add all custom headers
+            addCustomHeaders(httpResponse, restResponse.getHeaders());
+            addCustomHeaders(httpResponse, threadContext.getResponseHeaders());
 
-        boolean success = false;
-        try {
             // If our response doesn't specify a content-type header, set one
             setHeaderField(httpResponse, CONTENT_TYPE, restResponse.contentType(), false);
             // If our response has no content-length, calculate and set one
             setHeaderField(httpResponse, CONTENT_LENGTH, String.valueOf(restResponse.content().length()), false);
 
             addCookies(httpResponse);
 
-            BytesReference content = restResponse.content();
-            if (content instanceof Releasable) {
-                toClose.add((Releasable) content);
-            }
             BytesStreamOutput bytesStreamOutput = bytesOutputOrNull();
             if (bytesStreamOutput instanceof ReleasableBytesStreamOutput) {
                 toClose.add((Releasable) bytesStreamOutput);
             }
 
-            if (isCloseConnection()) {
-                toClose.add(() -> CloseableChannel.closeChannel(httpChannel));
-            }
-
             ActionListener<Void> listener = ActionListener.wrap(() -> Releasables.close(toClose));
             httpChannel.sendResponse(httpResponse, listener);
             success = true;
@@ -127,7 +133,6 @@ public void sendResponse(RestResponse restResponse) {
                 Releasables.close(toClose);
             }
         }
-
     }
 
     private void setHeaderField(HttpResponse response, String headerField, String value) {

server/src/main/java/org/elasticsearch/http/HttpRequest.java

@@ -37,6 +37,12 @@ enum HttpVersion {
         HTTP_1_1
     }
 
+    /**
+     * Returns the HTTP method used in the HTTP request.
+     *
+     * @return the {@link RestRequest.Method} used in the REST request
+     * @throws IllegalArgumentException if the HTTP method is invalid
+     */
     RestRequest.Method method();
 
     /**

server/src/main/java/org/elasticsearch/rest/RestController.java

@@ -53,13 +53,12 @@
 import java.util.concurrent.atomic.AtomicBoolean;
 import java.util.function.UnaryOperator;
 
+import static org.elasticsearch.rest.BytesRestResponse.TEXT_CONTENT_TYPE;
 import static org.elasticsearch.rest.RestStatus.BAD_REQUEST;
-import static org.elasticsearch.rest.RestStatus.METHOD_NOT_ALLOWED;
-import static org.elasticsearch.rest.RestStatus.FORBIDDEN;
 import static org.elasticsearch.rest.RestStatus.INTERNAL_SERVER_ERROR;
+import static org.elasticsearch.rest.RestStatus.METHOD_NOT_ALLOWED;
 import static org.elasticsearch.rest.RestStatus.NOT_ACCEPTABLE;
 import static org.elasticsearch.rest.RestStatus.OK;
-import static org.elasticsearch.rest.BytesRestResponse.TEXT_CONTENT_TYPE;
 
 public class RestController implements HttpServerTransport.Dispatcher {
 
@@ -253,7 +252,7 @@ boolean dispatchRequest(final RestRequest request, final RestChannel channel, fi
                 // If an alternative handler for an explicit path is registered to a
                 // different HTTP method than the one supplied - return a 405 Method
                 // Not Allowed error.
-                handleUnsupportedHttpMethod(request, channel, validMethodSet);
+                handleUnsupportedHttpMethod(request, channel, validMethodSet, null);
                 requestHandled = true;
             } else if (validMethodSet.contains(request.method()) == false
                 && (request.method() == RestRequest.Method.OPTIONS)) {
@@ -330,16 +329,28 @@ void tryAllHandlers(final RestRequest request, final RestChannel channel, final
             return;
         }
 
-        // Loop through all possible handlers, attempting to dispatch the request
-        Iterator<MethodHandlers> allHandlers = getAllHandlers(request);
-        for (Iterator<MethodHandlers> it = allHandlers; it.hasNext(); ) {
-            final Optional<RestHandler> mHandler = Optional.ofNullable(it.next()).flatMap(mh -> mh.getHandler(request.method()));
-            requestHandled = dispatchRequest(request, channel, client, mHandler);
-            if (requestHandled) {
-                break;
+        try {
+            // Resolves the HTTP method and fails if the method is invalid
+            final RestRequest.Method requestMethod = request.method();
+
+            // Loop through all possible handlers, attempting to dispatch the request
+            Iterator<MethodHandlers> allHandlers = getAllHandlers(request);
+            for (Iterator<MethodHandlers> it = allHandlers; it.hasNext(); ) {
+                Optional<RestHandler> mHandler = Optional.empty();
+                if (requestMethod != null) {
+                    mHandler = Optional.ofNullable(it.next()).flatMap(mh -> mh.getHandler(requestMethod));
+                }
+                requestHandled = dispatchRequest(request, channel, client, mHandler);
+                if (requestHandled) {
+                    break;
+                }
             }
+        } catch (final IllegalArgumentException e) {
+            handleUnsupportedHttpMethod(request, channel, getValidHandlerMethodSet(request), e);
+            requestHandled = true;
         }
 
+
         // If request has not been handled, fallback to a bad request error.
         if (requestHandled == false) {
             handleBadRequest(request, channel);
@@ -365,11 +376,25 @@ Iterator<MethodHandlers> getAllHandlers(final RestRequest request) {
      * <a href="https://tools.ietf.org/html/rfc2616#section-10.4.6">HTTP/1.1 -
      * 10.4.6 - 405 Method Not Allowed</a>).
      */
-    private void handleUnsupportedHttpMethod(RestRequest request, RestChannel channel, Set<RestRequest.Method> validMethodSet) {
+    private void handleUnsupportedHttpMethod(final RestRequest request,
+                                             final RestChannel channel,
+                                             final Set<RestRequest.Method> validMethodSet,
+                                             @Nullable final IllegalArgumentException exception) {
         try {
-            BytesRestResponse bytesRestResponse = BytesRestResponse.createSimpleErrorResponse(channel, METHOD_NOT_ALLOWED,
-                "Incorrect HTTP method for uri [" + request.uri() + "] and method [" + request.method() + "], allowed: " + validMethodSet);
-            bytesRestResponse.addHeader("Allow", Strings.collectionToDelimitedString(validMethodSet, ","));
+            final StringBuilder msg = new StringBuilder();
+            if (exception != null) {
+                msg.append(exception.getMessage());
+            } else {
+                msg.append("Incorrect HTTP method for uri [").append(request.uri());
+                msg.append("] and method [").append(request.method()).append("]");
+            }
+            if (validMethodSet.isEmpty() == false) {
+                msg.append(", allowed: ").append(validMethodSet);
+            }
+            BytesRestResponse bytesRestResponse = BytesRestResponse.createSimpleErrorResponse(channel, METHOD_NOT_ALLOWED, msg.toString());
+            if (validMethodSet.isEmpty() == false) {
+                bytesRestResponse.addHeader("Allow", Strings.collectionToDelimitedString(validMethodSet, ","));
+            }
             channel.sendResponse(bytesRestResponse);
         } catch (final IOException e) {
             logger.warn("failed to send bad request response", e);
@@ -385,11 +410,12 @@ private void handleUnsupportedHttpMethod(RestRequest request, RestChannel channe
      * - Options</a>).
      */
     private void handleOptionsRequest(RestRequest request, RestChannel channel, Set<RestRequest.Method> validMethodSet) {
-        if (request.method() == RestRequest.Method.OPTIONS && validMethodSet.size() > 0) {
+        assert request.method() == RestRequest.Method.OPTIONS;
+        if (validMethodSet.isEmpty() == false) {
             BytesRestResponse bytesRestResponse = new BytesRestResponse(OK, TEXT_CONTENT_TYPE, BytesArray.EMPTY);
             bytesRestResponse.addHeader("Allow", Strings.collectionToDelimitedString(validMethodSet, ","));
             channel.sendResponse(bytesRestResponse);
-        } else if (request.method() == RestRequest.Method.OPTIONS && validMethodSet.size() == 0) {
+        } else {
             /*
              * When we have an OPTIONS HTTP request and no valid handlers,
              * simply send OK by default (with the Access Control Origin header
@@ -433,20 +459,25 @@ private String getPath(RestRequest request) {
         return request.rawPath();
     }
 
-    void handleFavicon(RestRequest request, RestChannel channel) {
-        if (request.method() == RestRequest.Method.GET) {
-            try {
-                try (InputStream stream = getClass().getResourceAsStream("/config/favicon.ico")) {
-                    ByteArrayOutputStream out = new ByteArrayOutputStream();
-                    Streams.copy(stream, out);
-                    BytesRestResponse restResponse = new BytesRestResponse(RestStatus.OK, "image/x-icon", out.toByteArray());
-                    channel.sendResponse(restResponse);
+    private void handleFavicon(final RestRequest request, final RestChannel channel) {
+        try {
+            if (request.method() != RestRequest.Method.GET) {
+                handleUnsupportedHttpMethod(request, channel, Set.of(RestRequest.Method.GET), null);
+            } else {
+                try {
+                    try (InputStream stream = getClass().getResourceAsStream("/config/favicon.ico")) {
+                        ByteArrayOutputStream out = new ByteArrayOutputStream();
+                        Streams.copy(stream, out);
+                        BytesRestResponse restResponse = new BytesRestResponse(RestStatus.OK, "image/x-icon", out.toByteArray());
+                        channel.sendResponse(restResponse);
+                    }
+                } catch (IOException e) {
+                    channel.sendResponse(
+                        new BytesRestResponse(INTERNAL_SERVER_ERROR, BytesRestResponse.TEXT_CONTENT_TYPE, BytesArray.EMPTY));
                 }
-            } catch (IOException e) {
-                channel.sendResponse(new BytesRestResponse(INTERNAL_SERVER_ERROR, BytesRestResponse.TEXT_CONTENT_TYPE, BytesArray.EMPTY));
             }
-        } else {
-            channel.sendResponse(new BytesRestResponse(FORBIDDEN, BytesRestResponse.TEXT_CONTENT_TYPE, BytesArray.EMPTY));
+        } catch (final IllegalArgumentException e) {
+            handleUnsupportedHttpMethod(request, channel, Set.of(RestRequest.Method.GET), e);
         }
     }
 
@@ -512,5 +543,4 @@ private static CircuitBreaker inFlightRequestsBreaker(CircuitBreakerService circ
         // We always obtain a fresh breaker to reflect changes to the breaker configuration.
         return circuitBreakerService.getBreaker(CircuitBreaker.IN_FLIGHT_REQUESTS);
     }
-
 }

server/src/main/java/org/elasticsearch/rest/RestRequest.java

@@ -153,6 +153,12 @@ public enum Method {
         GET, POST, PUT, DELETE, OPTIONS, HEAD, PATCH, TRACE, CONNECT
     }
 
+    /**
+     * Returns the HTTP method used in the REST request.
+     *
+     * @return the {@link Method} used in the REST request
+     * @throws IllegalArgumentException if the HTTP method is invalid
+     */
     public Method method() {
         return httpRequest.method();
     }

server/src/test/java/org/elasticsearch/http/DefaultRestChannelTests.java

@@ -22,10 +22,13 @@
 import org.elasticsearch.action.ActionListener;
 import org.elasticsearch.common.bytes.BytesArray;
 import org.elasticsearch.common.bytes.BytesReference;
+import org.elasticsearch.common.bytes.ReleasablePagedBytesReference;
 import org.elasticsearch.common.io.stream.BytesStreamOutput;
 import org.elasticsearch.common.io.stream.ReleasableBytesStreamOutput;
 import org.elasticsearch.common.lease.Releasable;
 import org.elasticsearch.common.settings.Settings;
+import org.elasticsearch.common.util.BigArrays;
+import org.elasticsearch.common.util.ByteArray;
 import org.elasticsearch.common.util.MockBigArrays;
 import org.elasticsearch.common.util.MockPageCacheRecycler;
 import org.elasticsearch.common.xcontent.XContentBuilder;
@@ -53,6 +56,7 @@
 import java.util.HashMap;
 import java.util.List;
 import java.util.Map;
+import java.util.Objects;
 
 import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.hasItem;
@@ -303,6 +307,72 @@ public void testConnectionClose() throws Exception {
         }
     }
 
+    public void testUnsupportedHttpMethod() {
+        final boolean close = randomBoolean();
+        final HttpRequest.HttpVersion httpVersion = close ? HttpRequest.HttpVersion.HTTP_1_0 : HttpRequest.HttpVersion.HTTP_1_1;
+        final String httpConnectionHeaderValue = close ? DefaultRestChannel.CLOSE : DefaultRestChannel.KEEP_ALIVE;
+        final RestRequest request = RestRequest.request(xContentRegistry(), new TestRequest(httpVersion, null, "/") {
+            @Override
+            public RestRequest.Method method() {
+                throw new IllegalArgumentException("test");
+            }
+        }, httpChannel);
+        request.getHttpRequest().getHeaders().put(DefaultRestChannel.CONNECTION, Collections.singletonList(httpConnectionHeaderValue));
+
+        DefaultRestChannel channel = new DefaultRestChannel(httpChannel, request.getHttpRequest(), request, bigArrays,
+            HttpHandlingSettings.fromSettings(Settings.EMPTY), threadPool.getThreadContext());
+
+        // ESTestCase#after will invoke ensureAllArraysAreReleased which will fail if the response content was not released
+        final BigArrays bigArrays = new MockBigArrays(new MockPageCacheRecycler(Settings.EMPTY), new NoneCircuitBreakerService());
+        final ByteArray byteArray = bigArrays.newByteArray(0, false);
+        final BytesReference content = new ReleasablePagedBytesReference(byteArray, 0 , byteArray);
+        channel.sendResponse(new TestRestResponse(RestStatus.METHOD_NOT_ALLOWED, content));
+
+        Class<ActionListener<Void>> listenerClass = (Class<ActionListener<Void>>) (Class) ActionListener.class;
+        ArgumentCaptor<ActionListener<Void>> listenerCaptor = ArgumentCaptor.forClass(listenerClass);
+        verify(httpChannel).sendResponse(any(), listenerCaptor.capture());
+        ActionListener<Void> listener = listenerCaptor.getValue();
+        if (randomBoolean()) {
+            listener.onResponse(null);
+        } else {
+            listener.onFailure(new ClosedChannelException());
+        }
+        if (close) {
+            verify(httpChannel, times(1)).close();
+        } else {
+            verify(httpChannel, times(0)).close();
+        }
+    }
+
+    public void testCloseOnException() {
+        final boolean close = randomBoolean();
+        final HttpRequest.HttpVersion httpVersion = close ? HttpRequest.HttpVersion.HTTP_1_0 : HttpRequest.HttpVersion.HTTP_1_1;
+        final String httpConnectionHeaderValue = close ? DefaultRestChannel.CLOSE : DefaultRestChannel.KEEP_ALIVE;
+        final RestRequest request = RestRequest.request(xContentRegistry(), new TestRequest(httpVersion, null, "/") {
+            @Override
+            public HttpResponse createResponse(RestStatus status, BytesReference content) {
+                throw new IllegalArgumentException("test");
+            }
+        }, httpChannel);
+        request.getHttpRequest().getHeaders().put(DefaultRestChannel.CONNECTION, Collections.singletonList(httpConnectionHeaderValue));
+
+        DefaultRestChannel channel = new DefaultRestChannel(httpChannel, request.getHttpRequest(), request, bigArrays,
+            HttpHandlingSettings.fromSettings(Settings.EMPTY), threadPool.getThreadContext());
+
+        // ESTestCase#after will invoke ensureAllArraysAreReleased which will fail if the response content was not released
+        final BigArrays bigArrays = new MockBigArrays(new MockPageCacheRecycler(Settings.EMPTY), new NoneCircuitBreakerService());
+        final ByteArray byteArray = bigArrays.newByteArray(0, false);
+        final BytesReference content = new ReleasablePagedBytesReference(byteArray, 0 , byteArray);
+
+        expectThrows(IllegalArgumentException.class, () -> channel.sendResponse(new TestRestResponse(RestStatus.OK, content)));
+
+        if (close) {
+            verify(httpChannel, times(1)).close();
+        } else {
+            verify(httpChannel, times(0)).close();
+        }
+    }
+
     private TestResponse executeRequest(final Settings settings, final String host) {
         return executeRequest(settings, null, host);
     }
@@ -424,10 +494,16 @@ public boolean containsHeader(String name) {
 
     private static class TestRestResponse extends RestResponse {
 
+        private final RestStatus status;
         private final BytesReference content;
 
+        TestRestResponse(final RestStatus status, final BytesReference content) {
+            this.status = Objects.requireNonNull(status);
+            this.content = Objects.requireNonNull(content);
+        }
+
         TestRestResponse() {
-            content = new BytesArray("content".getBytes(StandardCharsets.UTF_8));
+            this(RestStatus.OK, new BytesArray("content".getBytes(StandardCharsets.UTF_8)));
         }
 
         public String contentType() {
@@ -439,7 +515,7 @@ public BytesReference content() {
         }
 
         public RestStatus status() {
-            return RestStatus.OK;
+            return status;
         }
     }
 }