Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Handle incorrect header values #64708

Merged
merged 5 commits into from
Nov 10, 2020
Merged

Handle incorrect header values #64708

Show file tree
Hide file tree
Changes from 1 commit
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
a6d89c2
Handle incorrect header values
pgomulka Nov 6, 2020
12de009
surface header name value
pgomulka Nov 9, 2020
81e160b
reformat
pgomulka Nov 9, 2020
a838410
Merge branch 'master' into compat/handle_incorrect_accept_content_type
pgomulka Nov 9, 2020
c12fd96
fix tests after rename
pgomulka Nov 9, 2020
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
9 changes: 6 additions & 3 deletions server/src/main/java/org/elasticsearch/http/AbstractHttpServerTransport.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -347,7 +347,7 @@ private void handleIncomingRequest(final HttpRequest httpRequest, final HttpChan
innerRestRequest = RestRequest.request(xContentRegistry, httpRequest, httpChannel);
} catch (final RestRequest.MediaTypeHeaderException e) {
badRequestCause = ExceptionsHelper.useOrSuppress(badRequestCause, e);
innerRestRequest = requestWithoutContentTypeHeader(httpRequest, httpChannel, badRequestCause);
innerRestRequest = requestWithoutFailedHeader(httpRequest, httpChannel, badRequestCause, e.getFailedHeaderName());
} catch (final RestRequest.BadParameterException e) {
badRequestCause = ExceptionsHelper.useOrSuppress(badRequestCause, e);
innerRestRequest = RestRequest.requestWithoutParameters(xContentRegistry, httpRequest, httpChannel);
Expand Down Expand Up @@ -384,8 +384,11 @@ private void handleIncomingRequest(final HttpRequest httpRequest, final HttpChan
dispatchRequest(restRequest, channel, badRequestCause);
}

private RestRequest requestWithoutContentTypeHeader(HttpRequest httpRequest, HttpChannel httpChannel, Exception badRequestCause) {
HttpRequest httpRequestWithoutContentType = httpRequest.removeHeader("Content-Type");
private RestRequest requestWithoutFailedHeader(HttpRequest httpRequest,
HttpChannel httpChannel,
Exception badRequestCause,
String failedHeaderName) {
HttpRequest httpRequestWithoutContentType = httpRequest.removeHeader(failedHeaderName);
try {
return RestRequest.request(xContentRegistry, httpRequestWithoutContentType, httpChannel);
} catch (final RestRequest.BadParameterException e) {
Expand Down
14 changes: 12 additions & 2 deletions server/src/main/java/org/elasticsearch/rest/RestRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -90,12 +90,16 @@ private RestRequest(NamedXContentRegistry xContentRegistry, Map<String, String>
Map<String, List<String>> headers, HttpRequest httpRequest, HttpChannel httpChannel, long requestId) {
try {
this.parsedAccept = parseHeaderWithMediaType(httpRequest.getHeaders(), "Accept");
} catch (IllegalArgumentException e) {
throw new MediaTypeHeaderException(e, "Accept");
}
try {
this.parsedContentType = parseHeaderWithMediaType(httpRequest.getHeaders(), "Content-Type");
if (parsedContentType != null) {
this.xContentType.set(parsedContentType.toMediaType(XContentType.MEDIA_TYPE_REGISTRY));
}
} catch (IllegalArgumentException e) {
throw new MediaTypeHeaderException(e);
throw new MediaTypeHeaderException(e, "Content-Type");
}
this.xContentRegistry = xContentRegistry;
this.httpRequest = httpRequest;
Expand Down Expand Up @@ -551,10 +555,16 @@ public static XContentType parseContentType(List<String> header) {

public static class MediaTypeHeaderException extends RuntimeException {

MediaTypeHeaderException(final IllegalArgumentException cause) {
private String failedHeaderName;

MediaTypeHeaderException(final IllegalArgumentException cause, String failedHeaderName) {
super(cause);
this.failedHeaderName = failedHeaderName;
}

public String getFailedHeaderName() {
return failedHeaderName;
}
}

public static class BadParameterException extends RuntimeException {
Expand Down
61 changes: 61 additions & 0 deletions server/src/test/java/org/elasticsearch/http/AbstractHttpServerTransportTests.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,19 +47,23 @@
import org.elasticsearch.threadpool.TestThreadPool;
import org.elasticsearch.threadpool.ThreadPool;
import org.junit.After;
import org.junit.Assert;
import org.junit.Before;

import java.net.InetSocketAddress;
import java.net.UnknownHostException;
import java.util.ArrayList;
import java.util.Collections;
import java.util.HashMap;
import java.util.List;
import java.util.Map;

import static java.net.InetAddress.getByName;
import static java.util.Arrays.asList;
import static org.elasticsearch.http.AbstractHttpServerTransport.resolvePublishPort;
import static org.hamcrest.Matchers.containsString;
import static org.hamcrest.Matchers.equalTo;
import static org.hamcrest.Matchers.instanceOf;

public class AbstractHttpServerTransportTests extends ESTestCase {

Expand Down Expand Up @@ -173,6 +177,63 @@ public HttpStats stats() {
}
}

public void testIncorrectHeaderHandling() {

final ClusterSettings clusterSettings = new ClusterSettings(Settings.EMPTY, ClusterSettings.BUILT_IN_CLUSTER_SETTINGS);
try (AbstractHttpServerTransport transport =
new AbstractHttpServerTransport(Settings.EMPTY, networkService, bigArrays, threadPool, xContentRegistry(),
new HttpServerTransport.Dispatcher() {
@Override
public void dispatchRequest(RestRequest request, RestChannel channel, ThreadContext threadContext) {
Assert.fail();
}

@Override
public void dispatchBadRequest(RestChannel channel, ThreadContext threadContext, Throwable cause) {
assertThat(cause, instanceOf(RestRequest.MediaTypeHeaderException.class));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Is there anywhere that we can assert that the header name at fault is included in the error sent back to the user ?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

great you noticed this. in fact the header name was not surfaced on a response.
I refactored this to return something like

{
  "error": {
    "root_cause": [
      {
        "type": "media_type_header_exception",
        "reason": "Invalid media-type value on header [Accept]"
      }
    ],
    "type": "media_type_header_exception",
    "reason": "Invalid media-type value on header [Accept]",
    "caused_by": {
      "type": "illegal_argument_exception",
      "reason": "invalid media type [texffff]"
    }
  },
  "status": 400
}

}
}, clusterSettings) {
@Override
protected HttpServerChannel bind(InetSocketAddress hostAddress) {
return null;
}

@Override
protected void doStart() {
}

@Override
protected void stopInternal() {
}

@Override
public HttpStats stats() {
return null;
}
}) {

{
Map<String, List<String>> headers = new HashMap<>();
headers.put("Accept", Collections.singletonList("incorrectheader"));

FakeRestRequest.FakeHttpRequest fakeHttpRequest =
new FakeRestRequest.FakeHttpRequest(RestRequest.Method.GET, "/", null, headers);

transport.incomingRequest(fakeHttpRequest, null);
}
{
Map<String, List<String>> headers = new HashMap<>();
headers.put("Accept", Collections.singletonList("application/json"));
headers.put("Content-Type", Collections.singletonList("incorrectheader"));

FakeRestRequest.FakeHttpRequest fakeHttpRequest =
new FakeRestRequest.FakeHttpRequest(RestRequest.Method.GET, "/", null, headers);

transport.incomingRequest(fakeHttpRequest, null);
}
}
}

@TestLogging(
value = "org.elasticsearch.http.HttpTracer:trace",
reason = "to ensure we log REST requests on TRACE level")
Expand Down
4 changes: 2 additions & 2 deletions test/framework/src/main/java/org/elasticsearch/test/rest/FakeRestRequest.java
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -48,15 +48,15 @@ private FakeRestRequest(NamedXContentRegistry xContentRegistry, HttpRequest http
super(xContentRegistry, params, httpRequest.uri(), httpRequest.getHeaders(), httpRequest, httpChannel);
}

private static class FakeHttpRequest implements HttpRequest {
public static class FakeHttpRequest implements HttpRequest {

private final Method method;
private final String uri;
private final BytesReference content;
private final Map<String, List<String>> headers;
private final Exception inboundException;

private FakeHttpRequest(Method method, String uri, BytesReference content, Map<String, List<String>> headers) {
public FakeHttpRequest(Method method, String uri, BytesReference content, Map<String, List<String>> headers) {
this(method, uri, content, headers, null);
}

Expand Down