diff --git a/server/src/main/java/org/elasticsearch/http/DefaultRestChannel.java b/server/src/main/java/org/elasticsearch/http/DefaultRestChannel.java index 9d21896182c67..5191a8d85cef6 100644 --- a/server/src/main/java/org/elasticsearch/http/DefaultRestChannel.java +++ b/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,29 +77,42 @@ 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 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 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 @@ -106,19 +120,11 @@ public void sendResponse(RestResponse restResponse) { 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 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) { diff --git a/server/src/main/java/org/elasticsearch/http/HttpRequest.java b/server/src/main/java/org/elasticsearch/http/HttpRequest.java index 496fec23312b0..02a3a58d1702d 100644 --- a/server/src/main/java/org/elasticsearch/http/HttpRequest.java +++ b/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(); /** diff --git a/server/src/main/java/org/elasticsearch/rest/RestController.java b/server/src/main/java/org/elasticsearch/rest/RestController.java index a55005454f629..3f32d281918a3 100644 --- a/server/src/main/java/org/elasticsearch/rest/RestController.java +++ b/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 allHandlers = getAllHandlers(request); - for (Iterator it = allHandlers; it.hasNext(); ) { - final Optional 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 allHandlers = getAllHandlers(request); + for (Iterator it = allHandlers; it.hasNext(); ) { + Optional 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 getAllHandlers(final RestRequest request) { * HTTP/1.1 - * 10.4.6 - 405 Method Not Allowed). */ - private void handleUnsupportedHttpMethod(RestRequest request, RestChannel channel, Set validMethodSet) { + private void handleUnsupportedHttpMethod(final RestRequest request, + final RestChannel channel, + final Set 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). */ private void handleOptionsRequest(RestRequest request, RestChannel channel, Set 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); } - } diff --git a/server/src/main/java/org/elasticsearch/rest/RestRequest.java b/server/src/main/java/org/elasticsearch/rest/RestRequest.java index fe976ee4ddce6..4fd8515caba03 100644 --- a/server/src/main/java/org/elasticsearch/rest/RestRequest.java +++ b/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(); } diff --git a/server/src/test/java/org/elasticsearch/http/DefaultRestChannelTests.java b/server/src/test/java/org/elasticsearch/http/DefaultRestChannelTests.java index 1a19ccf9b4e08..c58ec6a4becbb 100644 --- a/server/src/test/java/org/elasticsearch/http/DefaultRestChannelTests.java +++ b/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> listenerClass = (Class>) (Class) ActionListener.class; + ArgumentCaptor> listenerCaptor = ArgumentCaptor.forClass(listenerClass); + verify(httpChannel).sendResponse(any(), listenerCaptor.capture()); + ActionListener 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; } } } diff --git a/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java b/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java index c04dc7be026b9..0f4944cb0c6da 100644 --- a/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java +++ b/server/src/test/java/org/elasticsearch/rest/RestControllerTests.java @@ -35,6 +35,8 @@ import org.elasticsearch.common.xcontent.XContentType; import org.elasticsearch.common.xcontent.yaml.YamlXContent; import org.elasticsearch.http.HttpInfo; +import org.elasticsearch.http.HttpRequest; +import org.elasticsearch.http.HttpResponse; import org.elasticsearch.http.HttpServerTransport; import org.elasticsearch.http.HttpStats; import org.elasticsearch.indices.breaker.HierarchyCircuitBreakerService; @@ -58,6 +60,8 @@ import java.util.function.UnaryOperator; import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.equalTo; +import static org.hamcrest.Matchers.hasItem; import static org.mockito.Matchers.any; import static org.mockito.Matchers.eq; import static org.mockito.Mockito.doCallRealMethod; @@ -470,6 +474,89 @@ public void testDispatchBadRequestUnknownCause() { assertThat(channel.getRestResponse().content().utf8ToString(), containsString("unknown cause")); } + public void testFavicon() { + final FakeRestRequest fakeRestRequest = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY) + .withMethod(RestRequest.Method.GET) + .withPath("/favicon.ico") + .build(); + final AssertingChannel channel = new AssertingChannel(fakeRestRequest, false, RestStatus.OK); + restController.dispatchRequest(fakeRestRequest, channel, new ThreadContext(Settings.EMPTY)); + assertTrue(channel.getSendResponseCalled()); + assertThat(channel.getRestResponse().contentType(), containsString("image/x-icon")); + } + + public void testFaviconWithWrongHttpMethod() { + final FakeRestRequest fakeRestRequest = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY) + .withMethod(randomValueOtherThan(RestRequest.Method.GET, () -> randomFrom(RestRequest.Method.values()))) + .withPath("/favicon.ico") + .build(); + final AssertingChannel channel = new AssertingChannel(fakeRestRequest, true, RestStatus.METHOD_NOT_ALLOWED); + restController.dispatchRequest(fakeRestRequest, channel, new ThreadContext(Settings.EMPTY)); + assertTrue(channel.getSendResponseCalled()); + assertThat(channel.getRestResponse().getHeaders().containsKey("Allow"), equalTo(true)); + assertThat(channel.getRestResponse().getHeaders().get("Allow"), hasItem(equalTo(RestRequest.Method.GET.toString()))); + } + + public void testDispatchUnsupportedHttpMethod() { + final boolean hasContent = randomBoolean(); + final RestRequest request = RestRequest.request(xContentRegistry(), new HttpRequest() { + @Override + public RestRequest.Method method() { + throw new IllegalArgumentException("test"); + } + + @Override + public String uri() { + return "/"; + } + + @Override + public BytesReference content() { + if (hasContent) { + return new BytesArray("test"); + } + return BytesArray.EMPTY; + } + + @Override + public Map> getHeaders() { + Map> headers = new HashMap<>(); + if (hasContent) { + headers.put("Content-Type", Collections.singletonList("text/plain")); + } + return headers; + } + + @Override + public List strictCookies() { + return null; + } + + @Override + public HttpVersion protocolVersion() { + return randomFrom(HttpVersion.values()); + } + + @Override + public HttpRequest removeHeader(String header) { + return this; + } + + @Override + public HttpResponse createResponse(RestStatus status, BytesReference content) { + return null; + } + }, null); + + final AssertingChannel channel = new AssertingChannel(request, true, RestStatus.METHOD_NOT_ALLOWED); + assertFalse(channel.getSendResponseCalled()); + restController.dispatchRequest(request, channel, new ThreadContext(Settings.EMPTY)); + assertTrue(channel.getSendResponseCalled()); + assertThat(channel.getRestResponse().getHeaders().containsKey("Allow"), equalTo(true)); + assertThat(channel.getRestResponse().getHeaders().get("Allow"), hasItem(equalTo(RestRequest.Method.GET.toString()))); + } + + private static final class TestHttpServerTransport extends AbstractLifecycleComponent implements HttpServerTransport {