diff --git a/core/src/main/java/org/elasticsearch/rest/RestController.java b/core/src/main/java/org/elasticsearch/rest/RestController.java index 7ade8704be7..b645a20ace1 100644 --- a/core/src/main/java/org/elasticsearch/rest/RestController.java +++ b/core/src/main/java/org/elasticsearch/rest/RestController.java @@ -56,10 +56,12 @@ import java.util.function.Supplier; import java.util.function.UnaryOperator; 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.NOT_ACCEPTABLE; import static org.elasticsearch.rest.RestStatus.OK; +import static org.elasticsearch.rest.BytesRestResponse.TEXT_CONTENT_TYPE; public class RestController extends AbstractComponent implements HttpServerTransport.Dispatcher { @@ -141,11 +143,11 @@ public class RestController extends AbstractComponent implements HttpServerTrans } /** - * Registers a REST handler to be executed when the provided method and path match the request. + * Registers a REST handler to be executed when one of the provided methods and path match the request. * - * @param method GET, POST, etc. * @param path Path to handle (e.g., "/{index}/{type}/_bulk") * @param handler The handler to actually execute + * @param method GET, POST, etc. */ public void registerHandler(RestRequest.Method method, String path, RestHandler handler) { if (handler instanceof BaseRestHandler) { @@ -183,11 +185,8 @@ public class RestController extends AbstractComponent implements HttpServerTrans } @Override - public void dispatchBadRequest( - final RestRequest request, - final RestChannel channel, - final ThreadContext threadContext, - final Throwable cause) { + public void dispatchBadRequest(final RestRequest request, final RestChannel channel, + final ThreadContext threadContext, final Throwable cause) { try { final Exception e; if (cause == null) { @@ -211,7 +210,7 @@ public class RestController extends AbstractComponent implements HttpServerTrans * Dispatch the request, if possible, returning true if a response was sent or false otherwise. */ boolean dispatchRequest(final RestRequest request, final RestChannel channel, final NodeClient client, - ThreadContext threadContext, final Optional mHandler) throws Exception { + final Optional mHandler) throws Exception { final int contentLength = request.hasContent() ? request.content().length() : 0; RestChannel responseChannel = channel; @@ -228,6 +227,7 @@ public class RestController extends AbstractComponent implements HttpServerTrans "] does not support stream parsing. Use JSON or SMILE instead")); requestHandled = true; } else if (mHandler.isPresent()) { + try { if (canTripCircuitBreaker(mHandler)) { inFlightRequestsBreaker(circuitBreakerService).addEstimateBytesAndMaybeBreak(contentLength, ""); @@ -246,10 +246,19 @@ public class RestController extends AbstractComponent implements HttpServerTrans requestHandled = true; } } else { - if (request.method() == RestRequest.Method.OPTIONS) { - // when we have OPTIONS request, simply send OK by default (with the Access Control Origin header which gets automatically added) - - channel.sendResponse(new BytesRestResponse(OK, BytesRestResponse.TEXT_CONTENT_TYPE, BytesArray.EMPTY)); + // Get the map of matching handlers for a request, for the full set of HTTP methods. + final Set validMethodSet = getValidHandlerMethodSet(request); + if (validMethodSet.size() > 0 + && validMethodSet.contains(request.method()) == false + && request.method() != RestRequest.Method.OPTIONS) { + // 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); + requestHandled = true; + } else if (validMethodSet.contains(request.method()) == false + && (request.method() == RestRequest.Method.OPTIONS)) { + handleOptionsRequest(request, channel, validMethodSet); requestHandled = true; } else { requestHandled = false; @@ -263,9 +272,9 @@ public class RestController extends AbstractComponent implements HttpServerTrans * If a request contains content, this method will return {@code true} if the {@code Content-Type} header is present, matches an * {@link XContentType} or the handler supports a content stream and the content type header is for newline delimited JSON, */ - private boolean hasContentType(final RestRequest restRequest, final RestHandler restHandler) { + private static boolean hasContentType(final RestRequest restRequest, final RestHandler restHandler) { if (restRequest.getXContentType() == null) { - if (restHandler != null && restHandler.supportsContentStream() && restRequest.header("Content-Type") != null) { + if (restHandler.supportsContentStream() && restRequest.header("Content-Type") != null) { final String lowercaseMediaType = restRequest.header("Content-Type").toLowerCase(Locale.ROOT); // we also support newline delimited JSON: http://specs.okfnlabs.org/ndjson/ if (lowercaseMediaType.equals("application/x-ndjson")) { @@ -325,7 +334,7 @@ public class RestController extends AbstractComponent implements HttpServerTrans 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, threadContext, mHandler); + requestHandled = dispatchRequest(request, channel, client, mHandler); if (requestHandled) { break; } @@ -349,6 +358,47 @@ public class RestController extends AbstractComponent implements HttpServerTrans }); } + /** + * Handle requests to a valid REST endpoint using an unsupported HTTP + * method. A 405 HTTP response code is returned, and the response 'Allow' + * header includes a list of valid HTTP methods for the endpoint (see + * HTTP/1.1 - + * 10.4.6 - 405 Method Not Allowed). + */ + private void handleUnsupportedHttpMethod(RestRequest request, RestChannel channel, Set validMethodSet) { + 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, ",")); + channel.sendResponse(bytesRestResponse); + } catch (final IOException e) { + logger.warn("failed to send bad request response", e); + channel.sendResponse(new BytesRestResponse(INTERNAL_SERVER_ERROR, BytesRestResponse.TEXT_CONTENT_TYPE, BytesArray.EMPTY)); + } + } + + /** + * Handle HTTP OPTIONS requests to a valid REST endpoint. A 200 HTTP + * response code is returned, and the response 'Allow' header includes a + * list of valid HTTP methods for the endpoint (see + * HTTP/1.1 - 9.2 + * - Options). + */ + private void handleOptionsRequest(RestRequest request, RestChannel channel, Set validMethodSet) { + if (request.method() == RestRequest.Method.OPTIONS && validMethodSet.size() > 0) { + 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) { + /* + * When we have an OPTIONS HTTP request and no valid handlers, + * simply send OK by default (with the Access Control Origin header + * which gets automatically added). + */ + channel.sendResponse(new BytesRestResponse(OK, TEXT_CONTENT_TYPE, BytesArray.EMPTY)); + } + } + /** * Handle a requests with no candidate handlers (return a 400 Bad Request * error). diff --git a/core/src/test/java/org/elasticsearch/rest/RestControllerTests.java b/core/src/test/java/org/elasticsearch/rest/RestControllerTests.java index ede8eae1dfe..08cab9ea2e9 100644 --- a/core/src/test/java/org/elasticsearch/rest/RestControllerTests.java +++ b/core/src/test/java/org/elasticsearch/rest/RestControllerTests.java @@ -212,8 +212,7 @@ public class RestControllerTests extends ESTestCase { final RestController restController = new RestController(Settings.EMPTY, Collections.emptySet(), wrapper, null, circuitBreakerService, usageService); final ThreadContext threadContext = new ThreadContext(Settings.EMPTY); - restController.dispatchRequest(new FakeRestRequest.Builder(xContentRegistry()).build(), - null, null, threadContext, Optional.of(handler)); + restController.dispatchRequest(new FakeRestRequest.Builder(xContentRegistry()).build(), null, null, Optional.of(handler)); assertTrue(wrapperCalled.get()); assertFalse(handlerCalled.get()); } diff --git a/core/src/test/java/org/elasticsearch/rest/RestHttpResponseHeadersTests.java b/core/src/test/java/org/elasticsearch/rest/RestHttpResponseHeadersTests.java new file mode 100644 index 00000000000..ebe8ae00ac0 --- /dev/null +++ b/core/src/test/java/org/elasticsearch/rest/RestHttpResponseHeadersTests.java @@ -0,0 +1,159 @@ +/* + * Licensed to Elasticsearch under one or more contributor + * license agreements. See the NOTICE file distributed with + * this work for additional information regarding copyright + * ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may + * not use this file except in compliance with the License. + * You may obtain a copy of the License at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, + * software distributed under the License is distributed on an + * "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY + * KIND, either express or implied. See the License for the + * specific language governing permissions and limitations + * under the License. + */ + +package org.elasticsearch.rest; + +import org.elasticsearch.client.node.NodeClient; +import org.elasticsearch.common.bytes.BytesReference; +import org.elasticsearch.common.settings.ClusterSettings; +import org.elasticsearch.common.settings.Settings; +import org.elasticsearch.common.util.concurrent.ThreadContext; +import org.elasticsearch.indices.breaker.CircuitBreakerService; +import org.elasticsearch.indices.breaker.HierarchyCircuitBreakerService; +import org.elasticsearch.test.ESTestCase; +import org.elasticsearch.test.rest.FakeRestChannel; +import org.elasticsearch.test.rest.FakeRestRequest; +import org.elasticsearch.usage.UsageService; + +import java.util.ArrayList; +import java.util.Arrays; +import java.util.Collections; +import java.util.List; +import java.util.stream.Collectors; + +import static org.hamcrest.CoreMatchers.notNullValue; +import static org.hamcrest.Matchers.containsInAnyOrder; +import static org.hamcrest.Matchers.is; +import static org.mockito.Mockito.mock; + +public class RestHttpResponseHeadersTests extends ESTestCase { + + /** + * For requests to a valid REST endpoint using an unsupported HTTP method, + * verify that a 405 HTTP response code is returned, and that the response + * 'Allow' header includes a list of valid HTTP methods for the endpoint + * (see + * HTTP/1.1 - + * 10.4.6 - 405 Method Not Allowed). + */ + public void testUnsupportedMethodResponseHttpHeader() throws Exception { + + /* + * Generate a random set of candidate valid HTTP methods to register + * with the test RestController endpoint. Enums are returned in the + * order they are declared, so the first step is to shuffle the HTTP + * method list, passing in the RandomizedContext's Random instance, + * before picking out a candidate sublist. + */ + List validHttpMethodArray = new ArrayList(Arrays.asList(RestRequest.Method.values())); + validHttpMethodArray.remove(RestRequest.Method.OPTIONS); + Collections.shuffle(validHttpMethodArray, random()); + + /* + * The upper bound of the potential sublist is one less than the size of + * the array, so we are guaranteed at least one invalid method to test. + */ + validHttpMethodArray = validHttpMethodArray.subList(0, randomIntBetween(1, validHttpMethodArray.size() - 1)); + assert(validHttpMethodArray.size() > 0); + assert(validHttpMethodArray.size() < RestRequest.Method.values().length); + + /* + * Generate an inverse list of one or more candidate invalid HTTP + * methods, so we have a candidate method to fire at the test endpoint. + */ + List invalidHttpMethodArray = new ArrayList(Arrays.asList(RestRequest.Method.values())); + invalidHttpMethodArray.removeAll(validHttpMethodArray); + // Remove OPTIONS, or else we'll get a 200 instead of 405 + invalidHttpMethodArray.remove(RestRequest.Method.OPTIONS); + assert(invalidHttpMethodArray.size() > 0); + + // Initialize test candidate RestController + CircuitBreakerService circuitBreakerService = new HierarchyCircuitBreakerService(Settings.EMPTY, + new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS)); + + final Settings settings = Settings.EMPTY; + UsageService usageService = new UsageService(settings); + RestController restController = new RestController(settings, Collections.emptySet(), + null, null, circuitBreakerService, usageService); + + // A basic RestHandler handles requests to the endpoint + RestHandler restHandler = new RestHandler() { + + @Override + public void handleRequest(RestRequest request, RestChannel channel, NodeClient client) throws Exception { + channel.sendResponse(new TestResponse()); + } + + }; + + // Register valid test handlers with test RestController + for (RestRequest.Method method : validHttpMethodArray) { + restController.registerHandler(method, "/", restHandler); + } + + // Generate a test request with an invalid HTTP method + FakeRestRequest.Builder fakeRestRequestBuilder = new FakeRestRequest.Builder(xContentRegistry()); + fakeRestRequestBuilder.withMethod(invalidHttpMethodArray.get(0)); + RestRequest restRequest = fakeRestRequestBuilder.build(); + + // Send the request and verify the response status code + FakeRestChannel restChannel = new FakeRestChannel(restRequest, false, 1); + NodeClient client = mock(NodeClient.class); + restController.dispatchRequest(restRequest, restChannel, new ThreadContext(Settings.EMPTY)); + assertThat(restChannel.capturedResponse().status().getStatus(), is(405)); + + /* + * Verify the response allow header contains the valid methods for the + * test endpoint + */ + assertThat(restChannel.capturedResponse().getHeaders().get("Allow"), notNullValue()); + String responseAllowHeader = restChannel.capturedResponse().getHeaders().get("Allow").get(0); + List responseAllowHeaderArray = Arrays.asList(responseAllowHeader.split(",")); + assertThat(responseAllowHeaderArray.size(), is(validHttpMethodArray.size())); + assertThat(responseAllowHeaderArray, containsInAnyOrder(getMethodNameStringArray(validHttpMethodArray).toArray())); + } + + private static class TestResponse extends RestResponse { + + @Override + public String contentType() { + return null; + } + + @Override + public BytesReference content() { + return null; + } + + @Override + public RestStatus status() { + return RestStatus.OK; + } + + } + + /** + * Convert an RestRequest.Method array to a String array, so it can be + * compared with the expected 'Allow' header String array. + */ + private List getMethodNameStringArray(List methodArray) { + return methodArray.stream().map(method -> method.toString()).collect(Collectors.toList()); + } + +} diff --git a/docs/reference/migration/migrate_6_0/rest.asciidoc b/docs/reference/migration/migrate_6_0/rest.asciidoc index 45df33f08a9..99702cacaf4 100644 --- a/docs/reference/migration/migrate_6_0/rest.asciidoc +++ b/docs/reference/migration/migrate_6_0/rest.asciidoc @@ -61,7 +61,7 @@ In previous versions of Elasticsearch, delete by query requests without an expli were accepted, match_all was used as the default query and all documents were deleted as a result. From version 6.0.0, delete by query requests require an explicit query. -=== DELETE document calls now implicitly create the type +==== DELETE document calls now implicitly create the type Running `DELETE index/type/id` now implicitly creates `type` with a default mapping if it did not exist yet. @@ -76,8 +76,39 @@ removed.. `GET /_all` can be used to retrieve all aliases, settings, and mappings for all indices. In order to retrieve only the mappings for an index, `GET /myindex/_mappings` (or `_aliases`, or `_settings`). +==== Requests to existing endpoints with incorrect HTTP verb now return 405 responses + +Issuing a request to an endpoint that exists, but with an incorrect HTTP verb +(such as a `POST` request to `/myindex/_settings`) now returns an HTTP 405 +response instead of a 404. An `Allow` header is added to the 405 responses +containing the allowed verbs. For example: + +[source,text] +------------------------------------------- +$ curl -v -XPOST 'localhost:9200/my_index/_settings' +* Trying 127.0.0.1... +* TCP_NODELAY set +* Connected to localhost (127.0.0.1) port 9200 (#0) +> POST /my_index/_settings HTTP/1.1 +> Host: localhost:9200 +> User-Agent: curl/7.51.0 +> Accept: */* +> +< HTTP/1.1 405 Method Not Allowed +< Allow: PUT,GET +< content-type: application/json; charset=UTF-8 +< content-length: 134 +< +{ + "error" : "Incorrect HTTP method for uri [/my_index/_settings] and method [POST], allowed: [PUT, GET]", + "status" : 405 +} +* Curl_http_done: called premature == 0 +* Connection #0 to host localhost left intact +-------------------------------------------- + ==== Dissallow using `_cache` and `_cache_key` The `_cache` and `_cache_key` options in queries have been deprecated since version 2.0.0 and have been ignored since then, issuing a deprecation warning. These options have now been completely -removed, so using them now will throw an error. \ No newline at end of file +removed, so using them now will throw an error. diff --git a/qa/smoke-test-http/src/test/java/org/elasticsearch/http/RestHttpResponseHeadersIT.java b/qa/smoke-test-http/src/test/java/org/elasticsearch/http/RestHttpResponseHeadersIT.java new file mode 100644 index 00000000000..434ea094f46 --- /dev/null +++ b/qa/smoke-test-http/src/test/java/org/elasticsearch/http/RestHttpResponseHeadersIT.java @@ -0,0 +1,105 @@ +/* + * Licensed to Elasticsearch under one or more contributor license agreements. + * See the NOTICE file distributed with this work for additional information + * regarding copyright ownership. Elasticsearch licenses this file to you under + * the Apache License, Version 2.0 (the "License"); you may not use this file + * except in compliance with the License. You may obtain a copy of the License + * at + * + * http://www.apache.org/licenses/LICENSE-2.0 + * + * Unless required by applicable law or agreed to in writing, software + * distributed under the License is distributed on an "AS IS" BASIS, WITHOUT + * WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied. See the + * License for the specific language governing permissions and limitations under + * the License. + */ + +package org.elasticsearch.http; + +import org.apache.http.util.EntityUtils; +import org.elasticsearch.client.Response; +import org.elasticsearch.client.ResponseException; + +import java.util.Arrays; +import java.util.List; + +import static org.hamcrest.CoreMatchers.notNullValue; +import static org.hamcrest.Matchers.containsInAnyOrder; +import static org.hamcrest.Matchers.containsString; +import static org.hamcrest.Matchers.is; + +/** + * Refer to + * Unsupported + * methods on REST endpoints should respond with status code 405 for more + * information. + */ +public class RestHttpResponseHeadersIT extends HttpSmokeTestCase { + + /** + * For an OPTIONS request to a valid REST endpoint, verify that a 200 HTTP + * response code is returned, and that the response 'Allow' header includes + * a list of valid HTTP methods for the endpoint (see + * HTTP/1.1 - 9.2 + * - Options). + */ + public void testValidEndpointOptionsResponseHttpHeader() throws Exception { + Response response = getRestClient().performRequest("OPTIONS", "/_tasks"); + assertThat(response.getStatusLine().getStatusCode(), is(200)); + assertThat(response.getHeader("Allow"), notNullValue()); + List responseAllowHeaderStringArray = + Arrays.asList(response.getHeader("Allow").split(",")); + assertThat(responseAllowHeaderStringArray, containsInAnyOrder("GET")); + } + + /** + * For requests to a valid REST endpoint using an unsupported HTTP method, + * verify that a 405 HTTP response code is returned, and that the response + * 'Allow' header includes a list of valid HTTP methods for the endpoint + * (see + * HTTP/1.1 - + * 10.4.6 - 405 Method Not Allowed). + */ + public void testUnsupportedMethodResponseHttpHeader() throws Exception { + try { + getRestClient().performRequest("DELETE", "/_tasks"); + fail("Request should have failed with 405 error"); + } catch (ResponseException e) { + Response response = e.getResponse(); + assertThat(response.getStatusLine().getStatusCode(), is(405)); + assertThat(response.getHeader("Allow"), notNullValue()); + List responseAllowHeaderStringArray = + Arrays.asList(response.getHeader("Allow").split(",")); + assertThat(responseAllowHeaderStringArray, containsInAnyOrder("GET")); + assertThat(EntityUtils.toString(response.getEntity()), + containsString("Incorrect HTTP method for uri [/_tasks] and method [DELETE], allowed: [GET]")); + } + } + + /** + * Test if a POST request to /{index}/_settings matches the update settings + * handler for /{index}/_settings, and returns a 405 error (see + * Issue + * 17853 for more information). + */ + public void testIndexSettingsPostRequest() throws Exception { + try { + createIndex("testindex"); + getRestClient().performRequest("POST", "/testindex/_settings"); + fail("Request should have failed with 405 error"); + } catch (ResponseException e) { + Response response = e.getResponse(); + assertThat(response.getStatusLine().getStatusCode(), is(405)); + assertThat(response.getHeader("Allow"), notNullValue()); + List responseAllowHeaderStringArray = + Arrays.asList(response.getHeader("Allow").split(",")); + assertThat(responseAllowHeaderStringArray, containsInAnyOrder("PUT", "GET")); + assertThat(EntityUtils.toString(response.getEntity()), + containsString("Incorrect HTTP method for uri [/testindex/_settings] and method [POST], allowed:")); + assertThat(EntityUtils.toString(response.getEntity()), containsString("GET")); + assertThat(EntityUtils.toString(response.getEntity()), containsString("PUT")); + } + } + +} diff --git a/test/framework/src/main/java/org/elasticsearch/test/rest/FakeRestChannel.java b/test/framework/src/main/java/org/elasticsearch/test/rest/FakeRestChannel.java index bca4af7bd60..2dcadce2ed1 100644 --- a/test/framework/src/main/java/org/elasticsearch/test/rest/FakeRestChannel.java +++ b/test/framework/src/main/java/org/elasticsearch/test/rest/FakeRestChannel.java @@ -36,6 +36,7 @@ public final class FakeRestChannel extends AbstractRestChannel { private final CountDownLatch latch; private final AtomicInteger responses = new AtomicInteger(); private final AtomicInteger errors = new AtomicInteger(); + private RestResponse capturedRestResponse; public FakeRestChannel(RestRequest request, boolean detailedErrorsEnabled, int responseCount) { super(request, detailedErrorsEnabled); @@ -69,6 +70,7 @@ public final class FakeRestChannel extends AbstractRestChannel { @Override public void sendResponse(RestResponse response) { + this.capturedRestResponse = response; if (response.status() == RestStatus.OK) { responses.incrementAndGet(); } else { @@ -76,6 +78,10 @@ public final class FakeRestChannel extends AbstractRestChannel { } latch.countDown(); } + + public RestResponse capturedResponse() { + return capturedRestResponse; + } public boolean await() throws InterruptedException { return latch.await(10, TimeUnit.SECONDS);