honeymoose
/
OpenSearch
mirror of https://github.com/honeymoose/OpenSearch.git
1
0
Fork 0
Code Issues Packages Projects Releases Wiki Activity

Do not hang on unsupported HTTP methods (#43362) 

Unsupported HTTP methods are detected during requests dispatching
which generates an appropriate error response. Sadly, this error is
never sent back to the client because the method of the original
request is checked again in DefaultRestChannel which throws again
an IllegalArgumentException that is never handled.

This pull request changes the DefaultRestChannel so that the latest
exception is swallowed, allowing the error message to be sent back
to the client. It also eagerly adds the objects to close to the toClose
list so that resources are more likely to be released if something
 goes wrong during the response creation and sending.
This commit is contained in:
Tanguy Leroux 2019-06-24 12:59:41 +02:00
parent 19520d4640
commit 41ebaf57b5
6 changed files with 271 additions and 60 deletions
Show Stats Download Patch File Download Diff File Expand all files Collapse all files

41
server/src/main/java/org/elasticsearch/http/DefaultRestChannel.java
View File

@ -33,6 +33,7 @@ import org.elasticsearch.rest.AbstractRestChannel;
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,13 +77,30 @@ public class DefaultRestChannel extends AbstractRestChannel implements RestChann
@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));
}
boolean success = false;
try {
final BytesReference content = restResponse.content();
if (content instanceof Releasable) {
toClose.add((Releasable) content);
}
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)";
}
final HttpResponse httpResponse = httpRequest.createResponse(restResponse.status(), finalContent);
// TODO: Ideally we should move the setting of Cors headers into :server
// NioCorsHandler.setCorsResponseHeaders(nettyRequest, resp, corsConfig);
@ -95,10 +113,6 @@ public class DefaultRestChannel extends AbstractRestChannel implements RestChann
addCustomHeaders(httpResponse, restResponse.getHeaders());
addCustomHeaders(httpResponse, threadContext.getResponseHeaders());
ArrayList<Releasable> toClose = new ArrayList<>(3);
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 class DefaultRestChannel extends AbstractRestChannel implements RestChann
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 class DefaultRestChannel extends AbstractRestChannel implements RestChann
Releasables.close(toClose);
}
}
}
private void setHeaderField(HttpResponse response, String headerField, String value) {

6
server/src/main/java/org/elasticsearch/http/HttpRequest.java
View File

@ -37,6 +37,12 @@ public interface HttpRequest {
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();
/**

63
server/src/main/java/org/elasticsearch/rest/RestController.java
View File

@ -42,6 +42,7 @@ import org.elasticsearch.usage.UsageService;
import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.io.InputStream;
import java.util.Collections;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Iterator;
@ -53,13 +54,12 @@ import java.util.Set;
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 +253,7 @@ public class RestController implements HttpServerTransport.Dispatcher {
// 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,15 +330,27 @@ public class RestController implements HttpServerTransport.Dispatcher {
return;
}
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(); ) {
final Optional<RestHandler> mHandler = Optional.ofNullable(it.next()).flatMap(mh -> mh.getHandler(request.method()));
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) {
@ -365,11 +377,25 @@ public class RestController implements HttpServerTransport.Dispatcher {
* <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);
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 +411,12 @@ public class RestController implements HttpServerTransport.Dispatcher {
* - 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,8 +460,11 @@ public class RestController implements HttpServerTransport.Dispatcher {
return request.rawPath();
}
void handleFavicon(RestRequest request, RestChannel channel) {
if (request.method() == RestRequest.Method.GET) {
private void handleFavicon(final RestRequest request, final RestChannel channel) {
try {
if (request.method() != RestRequest.Method.GET) {
handleUnsupportedHttpMethod(request, channel, Collections.singleton(RestRequest.Method.GET), null);
} else {
try {
try (InputStream stream = getClass().getResourceAsStream("/config/favicon.ico")) {
ByteArrayOutputStream out = new ByteArrayOutputStream();
@ -443,10 +473,12 @@ public class RestController implements HttpServerTransport.Dispatcher {
channel.sendResponse(restResponse);
}
} catch (IOException e) {
channel.sendResponse(new BytesRestResponse(INTERNAL_SERVER_ERROR, BytesRestResponse.TEXT_CONTENT_TYPE, BytesArray.EMPTY));
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, Collections.singleton(RestRequest.Method.GET), e);
}
}
@ -512,5 +544,4 @@ public class RestController implements HttpServerTransport.Dispatcher {
// We always obtain a fresh breaker to reflect changes to the breaker configuration.
return circuitBreakerService.getBreaker(CircuitBreaker.IN_FLIGHT_REQUESTS);
}
}

6
server/src/main/java/org/elasticsearch/rest/RestRequest.java
View File

@ -153,6 +153,12 @@ public class RestRequest implements ToXContent.Params {
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();
}

80
server/src/test/java/org/elasticsearch/http/DefaultRestChannelTests.java
View File

@ -22,10 +22,13 @@ package org.elasticsearch.http;
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.Collections;
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 class DefaultRestChannelTests extends ESTestCase {
}
}
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 class DefaultRestChannelTests extends ESTestCase {
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 class DefaultRestChannelTests extends ESTestCase {
}
public RestStatus status() {
return RestStatus.OK;
return status;
}
}
}

87
server/src/test/java/org/elasticsearch/rest/RestControllerTests.java
View File

@ -35,6 +35,8 @@ import org.elasticsearch.common.xcontent.NamedXContentRegistry;
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.concurrent.atomic.AtomicReference;
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 class RestControllerTests extends ESTestCase {
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<String, List<String>> getHeaders() {
Map<String, List<String>> headers = new HashMap<>();
if (hasContent) {
headers.put("Content-Type", Collections.singletonList("text/plain"));
}
return headers;
}
@Override
public List<String> 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 {