Browse Source

Do not decode path when sending error

Today when sending a REST error to a client, we send the decoded
path. But decoding that path can already be the cause of the error in
which case decoding it again will just throw an exception leading to us
never sending an error back to the client. It would be better to send
the entire raw path to the client and that is what we do in this commit.

Relates #18477
Jason Tedor 9 years ago
parent
commit
61f40156d3

+ 2 - 2
core/src/main/java/org/elasticsearch/rest/BytesRestResponse.java

@@ -123,9 +123,9 @@ public class BytesRestResponse extends RestResponse {
                 params =  new ToXContent.DelegatingMapParams(Collections.singletonMap(ElasticsearchException.REST_EXCEPTION_SKIP_STACK_TRACE, "false"), channel.request());
             } else {
                 if (status.getStatus() < 500) {
-                    SUPPRESSED_ERROR_LOGGER.debug("{} Params: {}", t, channel.request().path(), channel.request().params());
+                    SUPPRESSED_ERROR_LOGGER.debug("path: {}, params: {}", t, channel.request().rawPath(), channel.request().params());
                 } else {
-                    SUPPRESSED_ERROR_LOGGER.warn("{} Params: {}", t, channel.request().path(), channel.request().params());
+                    SUPPRESSED_ERROR_LOGGER.warn("path: {}, params: {}", t, channel.request().rawPath(), channel.request().params());
                 }
                 params = channel.request();
             }

+ 30 - 0
core/src/test/java/org/elasticsearch/rest/BytesRestResponseTests.java

@@ -25,6 +25,7 @@ import org.elasticsearch.action.search.SearchPhaseExecutionException;
 import org.elasticsearch.action.search.ShardSearchFailure;
 import org.elasticsearch.common.ParsingException;
 import org.elasticsearch.index.Index;
+import org.elasticsearch.rest.support.RestUtils;
 import org.elasticsearch.search.SearchShardTarget;
 import org.elasticsearch.test.ESTestCase;
 import org.elasticsearch.test.rest.FakeRestRequest;
@@ -35,8 +36,11 @@ import java.io.IOException;
 
 import static org.hamcrest.Matchers.contains;
 import static org.hamcrest.Matchers.containsString;
+import static org.hamcrest.Matchers.equalTo;
 import static org.hamcrest.Matchers.not;
 import static org.hamcrest.Matchers.notNullValue;
+import static org.mockito.Mockito.mock;
+import static org.mockito.Mockito.when;
 
 /**
  *
@@ -147,6 +151,32 @@ public class BytesRestResponseTests extends ESTestCase {
         assertTrue(stackTrace.contains("Caused by: ParsingException[foobar]"));
     }
 
+    public void testResponseWhenPathContainsEncodingError() throws IOException {
+        final String path = "%a";
+        final RestRequest request = mock(RestRequest.class);
+        when(request.rawPath()).thenReturn(path);
+        final IllegalArgumentException e = expectThrows(IllegalArgumentException.class, () -> RestUtils.decodeComponent(request.rawPath()));
+        final RestChannel channel = new DetailedExceptionRestChannel(request);
+        // if we try to decode the path, this will throw an IllegalArgumentException again
+        final BytesRestResponse response = new BytesRestResponse(channel, e);
+        assertNotNull(response.content());
+        final String content = response.content().toUtf8();
+        assertThat(content, containsString("\"type\":\"illegal_argument_exception\""));
+        assertThat(content, containsString("\"reason\":\"partial escape sequence at end of string: %a\""));
+        assertThat(content, containsString("\"status\":" + 400));
+    }
+
+    public void testResponseWhenInternalServerError() throws IOException {
+        final RestRequest request = new FakeRestRequest();
+        final RestChannel channel = new DetailedExceptionRestChannel(request);
+        final BytesRestResponse response = new BytesRestResponse(channel, new ElasticsearchException("simulated"));
+        assertNotNull(response.content());
+        final String content = response.content().toUtf8();
+        assertThat(content, containsString("\"type\":\"exception\""));
+        assertThat(content, containsString("\"reason\":\"simulated\""));
+        assertThat(content, containsString("\"status\":" + 500));
+    }
+
     public static class WithHeadersException extends ElasticsearchException {
 
         WithHeadersException() {