Skip to content

Commit 45e8d54

Browse files
tlrxtlrx
authored
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.
1 parent 5e668ad commit 45e8d54

File tree

6 files changed

+268
-58
lines changed

6 files changed

+268
-58
lines changed

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

Lines changed: 31 additions & 26 deletions
Original file line numberDiff line numberDiff line change
@@ -33,6 +33,7 @@
3333
import org.elasticsearch.rest.RestChannel;
3434
import org.elasticsearch.rest.RestRequest;
3535
import org.elasticsearch.rest.RestResponse;
36+
import org.elasticsearch.rest.RestStatus;
3637

3738
import java.util.ArrayList;
3839
import java.util.List;
@@ -76,49 +77,54 @@ protected BytesStreamOutput newBytesOutput() {
7677

7778
@Override
7879
public void sendResponse(RestResponse restResponse) {
79-
HttpResponse httpResponse;
80-
if (RestRequest.Method.HEAD == request.method()) {
81-
httpResponse = httpRequest.createResponse(restResponse.status(), BytesArray.EMPTY);
82-
} else {
83-
httpResponse = httpRequest.createResponse(restResponse.status(), restResponse.content());
80+
final ArrayList<Releasable> toClose = new ArrayList<>(3);
81+
if (isCloseConnection()) {
82+
toClose.add(() -> CloseableChannel.closeChannel(httpChannel));
8483
}
8584

86-
// TODO: Ideally we should move the setting of Cors headers into :server
87-
// NioCorsHandler.setCorsResponseHeaders(nettyRequest, resp, corsConfig);
85+
boolean success = false;
86+
try {
87+
final BytesReference content = restResponse.content();
88+
if (content instanceof Releasable) {
89+
toClose.add((Releasable) content);
90+
}
8891

89-
String opaque = request.header(X_OPAQUE_ID);
90-
if (opaque != null) {
91-
setHeaderField(httpResponse, X_OPAQUE_ID, opaque);
92-
}
92+
BytesReference finalContent = content;
93+
try {
94+
if (request.method() == RestRequest.Method.HEAD) {
95+
finalContent = BytesArray.EMPTY;
96+
}
97+
} catch (IllegalArgumentException ignored) {
98+
assert restResponse.status() == RestStatus.METHOD_NOT_ALLOWED :
99+
"request HTTP method is unsupported but HTTP status is not METHOD_NOT_ALLOWED(405)";
100+
}
93101

94-
// Add all custom headers
95-
addCustomHeaders(httpResponse, restResponse.getHeaders());
96-
addCustomHeaders(httpResponse, threadContext.getResponseHeaders());
102+
final HttpResponse httpResponse = httpRequest.createResponse(restResponse.status(), finalContent);
97103

98-
ArrayList<Releasable> toClose = new ArrayList<>(3);
104+
// TODO: Ideally we should move the setting of Cors headers into :server
105+
// NioCorsHandler.setCorsResponseHeaders(nettyRequest, resp, corsConfig);
106+
107+
String opaque = request.header(X_OPAQUE_ID);
108+
if (opaque != null) {
109+
setHeaderField(httpResponse, X_OPAQUE_ID, opaque);
110+
}
111+
112+
// Add all custom headers
113+
addCustomHeaders(httpResponse, restResponse.getHeaders());
114+
addCustomHeaders(httpResponse, threadContext.getResponseHeaders());
99115

100-
boolean success = false;
101-
try {
102116
// If our response doesn't specify a content-type header, set one
103117
setHeaderField(httpResponse, CONTENT_TYPE, restResponse.contentType(), false);
104118
// If our response has no content-length, calculate and set one
105119
setHeaderField(httpResponse, CONTENT_LENGTH, String.valueOf(restResponse.content().length()), false);
106120

107121
addCookies(httpResponse);
108122

109-
BytesReference content = restResponse.content();
110-
if (content instanceof Releasable) {
111-
toClose.add((Releasable) content);
112-
}
113123
BytesStreamOutput bytesStreamOutput = bytesOutputOrNull();
114124
if (bytesStreamOutput instanceof ReleasableBytesStreamOutput) {
115125
toClose.add((Releasable) bytesStreamOutput);
116126
}
117127

118-
if (isCloseConnection()) {
119-
toClose.add(() -> CloseableChannel.closeChannel(httpChannel));
120-
}
121-
122128
ActionListener<Void> listener = ActionListener.wrap(() -> Releasables.close(toClose));
123129
httpChannel.sendResponse(httpResponse, listener);
124130
success = true;
@@ -127,7 +133,6 @@ public void sendResponse(RestResponse restResponse) {
127133
Releasables.close(toClose);
128134
}
129135
}
130-
131136
}
132137

133138
private void setHeaderField(HttpResponse response, String headerField, String value) {

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -37,6 +37,12 @@ enum HttpVersion {
3737
HTTP_1_1
3838
}
3939

40+
/**
41+
* Returns the HTTP method used in the HTTP request.
42+
*
43+
* @return the {@link RestRequest.Method} used in the REST request
44+
* @throws IllegalArgumentException if the HTTP method is invalid
45+
*/
4046
RestRequest.Method method();
4147

4248
/**

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

Lines changed: 60 additions & 30 deletions
Original file line numberDiff line numberDiff line change
@@ -53,13 +53,12 @@
5353
import java.util.concurrent.atomic.AtomicBoolean;
5454
import java.util.function.UnaryOperator;
5555

56+
import static org.elasticsearch.rest.BytesRestResponse.TEXT_CONTENT_TYPE;
5657
import static org.elasticsearch.rest.RestStatus.BAD_REQUEST;
57-
import static org.elasticsearch.rest.RestStatus.METHOD_NOT_ALLOWED;
58-
import static org.elasticsearch.rest.RestStatus.FORBIDDEN;
5958
import static org.elasticsearch.rest.RestStatus.INTERNAL_SERVER_ERROR;
59+
import static org.elasticsearch.rest.RestStatus.METHOD_NOT_ALLOWED;
6060
import static org.elasticsearch.rest.RestStatus.NOT_ACCEPTABLE;
6161
import static org.elasticsearch.rest.RestStatus.OK;
62-
import static org.elasticsearch.rest.BytesRestResponse.TEXT_CONTENT_TYPE;
6362

6463
public class RestController implements HttpServerTransport.Dispatcher {
6564

@@ -253,7 +252,7 @@ boolean dispatchRequest(final RestRequest request, final RestChannel channel, fi
253252
// If an alternative handler for an explicit path is registered to a
254253
// different HTTP method than the one supplied - return a 405 Method
255254
// Not Allowed error.
256-
handleUnsupportedHttpMethod(request, channel, validMethodSet);
255+
handleUnsupportedHttpMethod(request, channel, validMethodSet, null);
257256
requestHandled = true;
258257
} else if (validMethodSet.contains(request.method()) == false
259258
&& (request.method() == RestRequest.Method.OPTIONS)) {
@@ -330,16 +329,28 @@ void tryAllHandlers(final RestRequest request, final RestChannel channel, final
330329
return;
331330
}
332331

333-
// Loop through all possible handlers, attempting to dispatch the request
334-
Iterator<MethodHandlers> allHandlers = getAllHandlers(request);
335-
for (Iterator<MethodHandlers> it = allHandlers; it.hasNext(); ) {
336-
final Optional<RestHandler> mHandler = Optional.ofNullable(it.next()).flatMap(mh -> mh.getHandler(request.method()));
337-
requestHandled = dispatchRequest(request, channel, client, mHandler);
338-
if (requestHandled) {
339-
break;
332+
try {
333+
// Resolves the HTTP method and fails if the method is invalid
334+
final RestRequest.Method requestMethod = request.method();
335+
336+
// Loop through all possible handlers, attempting to dispatch the request
337+
Iterator<MethodHandlers> allHandlers = getAllHandlers(request);
338+
for (Iterator<MethodHandlers> it = allHandlers; it.hasNext(); ) {
339+
Optional<RestHandler> mHandler = Optional.empty();
340+
if (requestMethod != null) {
341+
mHandler = Optional.ofNullable(it.next()).flatMap(mh -> mh.getHandler(requestMethod));
342+
}
343+
requestHandled = dispatchRequest(request, channel, client, mHandler);
344+
if (requestHandled) {
345+
break;
346+
}
340347
}
348+
} catch (final IllegalArgumentException e) {
349+
handleUnsupportedHttpMethod(request, channel, getValidHandlerMethodSet(request), e);
350+
requestHandled = true;
341351
}
342352

353+
343354
// If request has not been handled, fallback to a bad request error.
344355
if (requestHandled == false) {
345356
handleBadRequest(request, channel);
@@ -365,11 +376,25 @@ Iterator<MethodHandlers> getAllHandlers(final RestRequest request) {
365376
* <a href="https://tools.ietf.org/html/rfc2616#section-10.4.6">HTTP/1.1 -
366377
* 10.4.6 - 405 Method Not Allowed</a>).
367378
*/
368-
private void handleUnsupportedHttpMethod(RestRequest request, RestChannel channel, Set<RestRequest.Method> validMethodSet) {
379+
private void handleUnsupportedHttpMethod(final RestRequest request,
380+
final RestChannel channel,
381+
final Set<RestRequest.Method> validMethodSet,
382+
@Nullable final IllegalArgumentException exception) {
369383
try {
370-
BytesRestResponse bytesRestResponse = BytesRestResponse.createSimpleErrorResponse(channel, METHOD_NOT_ALLOWED,
371-
"Incorrect HTTP method for uri [" + request.uri() + "] and method [" + request.method() + "], allowed: " + validMethodSet);
372-
bytesRestResponse.addHeader("Allow", Strings.collectionToDelimitedString(validMethodSet, ","));
384+
final StringBuilder msg = new StringBuilder();
385+
if (exception != null) {
386+
msg.append(exception.getMessage());
387+
} else {
388+
msg.append("Incorrect HTTP method for uri [").append(request.uri());
389+
msg.append("] and method [").append(request.method()).append("]");
390+
}
391+
if (validMethodSet.isEmpty() == false) {
392+
msg.append(", allowed: ").append(validMethodSet);
393+
}
394+
BytesRestResponse bytesRestResponse = BytesRestResponse.createSimpleErrorResponse(channel, METHOD_NOT_ALLOWED, msg.toString());
395+
if (validMethodSet.isEmpty() == false) {
396+
bytesRestResponse.addHeader("Allow", Strings.collectionToDelimitedString(validMethodSet, ","));
397+
}
373398
channel.sendResponse(bytesRestResponse);
374399
} catch (final IOException e) {
375400
logger.warn("failed to send bad request response", e);
@@ -385,11 +410,12 @@ private void handleUnsupportedHttpMethod(RestRequest request, RestChannel channe
385410
* - Options</a>).
386411
*/
387412
private void handleOptionsRequest(RestRequest request, RestChannel channel, Set<RestRequest.Method> validMethodSet) {
388-
if (request.method() == RestRequest.Method.OPTIONS && validMethodSet.size() > 0) {
413+
assert request.method() == RestRequest.Method.OPTIONS;
414+
if (validMethodSet.isEmpty() == false) {
389415
BytesRestResponse bytesRestResponse = new BytesRestResponse(OK, TEXT_CONTENT_TYPE, BytesArray.EMPTY);
390416
bytesRestResponse.addHeader("Allow", Strings.collectionToDelimitedString(validMethodSet, ","));
391417
channel.sendResponse(bytesRestResponse);
392-
} else if (request.method() == RestRequest.Method.OPTIONS && validMethodSet.size() == 0) {
418+
} else {
393419
/*
394420
* When we have an OPTIONS HTTP request and no valid handlers,
395421
* simply send OK by default (with the Access Control Origin header
@@ -433,20 +459,25 @@ private String getPath(RestRequest request) {
433459
return request.rawPath();
434460
}
435461

436-
void handleFavicon(RestRequest request, RestChannel channel) {
437-
if (request.method() == RestRequest.Method.GET) {
438-
try {
439-
try (InputStream stream = getClass().getResourceAsStream("/config/favicon.ico")) {
440-
ByteArrayOutputStream out = new ByteArrayOutputStream();
441-
Streams.copy(stream, out);
442-
BytesRestResponse restResponse = new BytesRestResponse(RestStatus.OK, "image/x-icon", out.toByteArray());
443-
channel.sendResponse(restResponse);
462+
private void handleFavicon(final RestRequest request, final RestChannel channel) {
463+
try {
464+
if (request.method() != RestRequest.Method.GET) {
465+
handleUnsupportedHttpMethod(request, channel, Set.of(RestRequest.Method.GET), null);
466+
} else {
467+
try {
468+
try (InputStream stream = getClass().getResourceAsStream("/config/favicon.ico")) {
469+
ByteArrayOutputStream out = new ByteArrayOutputStream();
470+
Streams.copy(stream, out);
471+
BytesRestResponse restResponse = new BytesRestResponse(RestStatus.OK, "image/x-icon", out.toByteArray());
472+
channel.sendResponse(restResponse);
473+
}
474+
} catch (IOException e) {
475+
channel.sendResponse(
476+
new BytesRestResponse(INTERNAL_SERVER_ERROR, BytesRestResponse.TEXT_CONTENT_TYPE, BytesArray.EMPTY));
444477
}
445-
} catch (IOException e) {
446-
channel.sendResponse(new BytesRestResponse(INTERNAL_SERVER_ERROR, BytesRestResponse.TEXT_CONTENT_TYPE, BytesArray.EMPTY));
447478
}
448-
} else {
449-
channel.sendResponse(new BytesRestResponse(FORBIDDEN, BytesRestResponse.TEXT_CONTENT_TYPE, BytesArray.EMPTY));
479+
} catch (final IllegalArgumentException e) {
480+
handleUnsupportedHttpMethod(request, channel, Set.of(RestRequest.Method.GET), e);
450481
}
451482
}
452483

@@ -512,5 +543,4 @@ private static CircuitBreaker inFlightRequestsBreaker(CircuitBreakerService circ
512543
// We always obtain a fresh breaker to reflect changes to the breaker configuration.
513544
return circuitBreakerService.getBreaker(CircuitBreaker.IN_FLIGHT_REQUESTS);
514545
}
515-
516546
}

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

Lines changed: 6 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -153,6 +153,12 @@ public enum Method {
153153
GET, POST, PUT, DELETE, OPTIONS, HEAD, PATCH, TRACE, CONNECT
154154
}
155155

156+
/**
157+
* Returns the HTTP method used in the REST request.
158+
*
159+
* @return the {@link Method} used in the REST request
160+
* @throws IllegalArgumentException if the HTTP method is invalid
161+
*/
156162
public Method method() {
157163
return httpRequest.method();
158164
}

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

Lines changed: 78 additions & 2 deletions
Original file line numberDiff line numberDiff line change
@@ -22,10 +22,13 @@
2222
import org.elasticsearch.action.ActionListener;
2323
import org.elasticsearch.common.bytes.BytesArray;
2424
import org.elasticsearch.common.bytes.BytesReference;
25+
import org.elasticsearch.common.bytes.ReleasablePagedBytesReference;
2526
import org.elasticsearch.common.io.stream.BytesStreamOutput;
2627
import org.elasticsearch.common.io.stream.ReleasableBytesStreamOutput;
2728
import org.elasticsearch.common.lease.Releasable;
2829
import org.elasticsearch.common.settings.Settings;
30+
import org.elasticsearch.common.util.BigArrays;
31+
import org.elasticsearch.common.util.ByteArray;
2932
import org.elasticsearch.common.util.MockBigArrays;
3033
import org.elasticsearch.common.util.MockPageCacheRecycler;
3134
import org.elasticsearch.common.xcontent.XContentBuilder;
@@ -53,6 +56,7 @@
5356
import java.util.HashMap;
5457
import java.util.List;
5558
import java.util.Map;
59+
import java.util.Objects;
5660

5761
import static org.hamcrest.Matchers.equalTo;
5862
import static org.hamcrest.Matchers.hasItem;
@@ -303,6 +307,72 @@ public void testConnectionClose() throws Exception {
303307
}
304308
}
305309

310+
public void testUnsupportedHttpMethod() {
311+
final boolean close = randomBoolean();
312+
final HttpRequest.HttpVersion httpVersion = close ? HttpRequest.HttpVersion.HTTP_1_0 : HttpRequest.HttpVersion.HTTP_1_1;
313+
final String httpConnectionHeaderValue = close ? DefaultRestChannel.CLOSE : DefaultRestChannel.KEEP_ALIVE;
314+
final RestRequest request = RestRequest.request(xContentRegistry(), new TestRequest(httpVersion, null, "/") {
315+
@Override
316+
public RestRequest.Method method() {
317+
throw new IllegalArgumentException("test");
318+
}
319+
}, httpChannel);
320+
request.getHttpRequest().getHeaders().put(DefaultRestChannel.CONNECTION, Collections.singletonList(httpConnectionHeaderValue));
321+
322+
DefaultRestChannel channel = new DefaultRestChannel(httpChannel, request.getHttpRequest(), request, bigArrays,
323+
HttpHandlingSettings.fromSettings(Settings.EMPTY), threadPool.getThreadContext());
324+
325+
// ESTestCase#after will invoke ensureAllArraysAreReleased which will fail if the response content was not released
326+
final BigArrays bigArrays = new MockBigArrays(new MockPageCacheRecycler(Settings.EMPTY), new NoneCircuitBreakerService());
327+
final ByteArray byteArray = bigArrays.newByteArray(0, false);
328+
final BytesReference content = new ReleasablePagedBytesReference(byteArray, 0 , byteArray);
329+
channel.sendResponse(new TestRestResponse(RestStatus.METHOD_NOT_ALLOWED, content));
330+
331+
Class<ActionListener<Void>> listenerClass = (Class<ActionListener<Void>>) (Class) ActionListener.class;
332+
ArgumentCaptor<ActionListener<Void>> listenerCaptor = ArgumentCaptor.forClass(listenerClass);
333+
verify(httpChannel).sendResponse(any(), listenerCaptor.capture());
334+
ActionListener<Void> listener = listenerCaptor.getValue();
335+
if (randomBoolean()) {
336+
listener.onResponse(null);
337+
} else {
338+
listener.onFailure(new ClosedChannelException());
339+
}
340+
if (close) {
341+
verify(httpChannel, times(1)).close();
342+
} else {
343+
verify(httpChannel, times(0)).close();
344+
}
345+
}
346+
347+
public void testCloseOnException() {
348+
final boolean close = randomBoolean();
349+
final HttpRequest.HttpVersion httpVersion = close ? HttpRequest.HttpVersion.HTTP_1_0 : HttpRequest.HttpVersion.HTTP_1_1;
350+
final String httpConnectionHeaderValue = close ? DefaultRestChannel.CLOSE : DefaultRestChannel.KEEP_ALIVE;
351+
final RestRequest request = RestRequest.request(xContentRegistry(), new TestRequest(httpVersion, null, "/") {
352+
@Override
353+
public HttpResponse createResponse(RestStatus status, BytesReference content) {
354+
throw new IllegalArgumentException("test");
355+
}
356+
}, httpChannel);
357+
request.getHttpRequest().getHeaders().put(DefaultRestChannel.CONNECTION, Collections.singletonList(httpConnectionHeaderValue));
358+
359+
DefaultRestChannel channel = new DefaultRestChannel(httpChannel, request.getHttpRequest(), request, bigArrays,
360+
HttpHandlingSettings.fromSettings(Settings.EMPTY), threadPool.getThreadContext());
361+
362+
// ESTestCase#after will invoke ensureAllArraysAreReleased which will fail if the response content was not released
363+
final BigArrays bigArrays = new MockBigArrays(new MockPageCacheRecycler(Settings.EMPTY), new NoneCircuitBreakerService());
364+
final ByteArray byteArray = bigArrays.newByteArray(0, false);
365+
final BytesReference content = new ReleasablePagedBytesReference(byteArray, 0 , byteArray);
366+
367+
expectThrows(IllegalArgumentException.class, () -> channel.sendResponse(new TestRestResponse(RestStatus.OK, content)));
368+
369+
if (close) {
370+
verify(httpChannel, times(1)).close();
371+
} else {
372+
verify(httpChannel, times(0)).close();
373+
}
374+
}
375+
306376
private TestResponse executeRequest(final Settings settings, final String host) {
307377
return executeRequest(settings, null, host);
308378
}
@@ -424,10 +494,16 @@ public boolean containsHeader(String name) {
424494

425495
private static class TestRestResponse extends RestResponse {
426496

497+
private final RestStatus status;
427498
private final BytesReference content;
428499

500+
TestRestResponse(final RestStatus status, final BytesReference content) {
501+
this.status = Objects.requireNonNull(status);
502+
this.content = Objects.requireNonNull(content);
503+
}
504+
429505
TestRestResponse() {
430-
content = new BytesArray("content".getBytes(StandardCharsets.UTF_8));
506+
this(RestStatus.OK, new BytesArray("content".getBytes(StandardCharsets.UTF_8)));
431507
}
432508

433509
public String contentType() {
@@ -439,7 +515,7 @@ public BytesReference content() {
439515
}
440516

441517
public RestStatus status() {
442-
return RestStatus.OK;
518+
return status;
443519
}
444520
}
445521
}

0 commit comments

Comments
 (0)