Skip to content

Commit

Permalink
Filter out invalid URI and HTTP method in the error message of no han…
Browse files Browse the repository at this point in the history
…dler found for a REST request (opensearch-project#3459)

Filter out invalid URI and HTTP method of a error message, which shown when there is no handler found for a REST request sent by user, so that HTML special characters <>&"' will not shown in the error message.

The error message is return as mine-type `application/json`, which can't contain active (script) content, so it's not a vulnerability. Besides, no browsers are going to render as html when the mine-type is that.
While the common security scanners will raise a false-positive alarm for having HTML tags in the response without escaping the HTML special characters, so the solution only aims to satisfy the code security scanners.

Signed-off-by: Tianli Feng <ftianli@amazon.com>
  • Loading branch information
Tianli Feng authored and Bukhtawar committed Jun 21, 2022
1 parent 1a6c11a commit 2651637
Show file tree
Hide file tree
Showing 2 changed files with 25 additions and 2 deletions.
14 changes: 12 additions & 2 deletions server/src/main/java/org/opensearch/rest/RestController.java
Original file line number Diff line number Diff line change
Expand Up @@ -56,6 +56,7 @@
import java.io.ByteArrayOutputStream;
import java.io.IOException;
import java.io.InputStream;
import java.net.URI;
import java.util.HashMap;
import java.util.HashSet;
import java.util.Iterator;
Expand Down Expand Up @@ -447,7 +448,9 @@ private void handleUnsupportedHttpMethod(
msg.append("Incorrect HTTP method for uri [").append(uri);
msg.append("] and method [").append(method).append("]");
} else {
msg.append(exception.getMessage());
// Not using the error message directly from 'exception.getMessage()' to avoid unescaped HTML special characters,
// in case false-positive cross site scripting vulnerability is detected by common security scanners.
msg.append("Unexpected HTTP method");
}
if (validMethodSet.isEmpty() == false) {
msg.append(", allowed: ").append(validMethodSet);
Expand Down Expand Up @@ -488,7 +491,14 @@ private void handleBadRequest(String uri, RestRequest.Method method, RestChannel
try (XContentBuilder builder = channel.newErrorBuilder()) {
builder.startObject();
{
builder.field("error", "no handler found for uri [" + uri + "] and method [" + method + "]");
try {
// Validate input URI to filter out HTML special characters in the error message,
// in case false-positive cross site scripting vulnerability is detected by common security scanners.
uri = new URI(uri).getPath();
builder.field("error", "no handler found for uri [" + uri + "] and method [" + method + "]");
} catch (Exception e) {
builder.field("error", "invalid uri has been requested");
}
}
builder.endObject();
channel.sendResponse(new BytesRestResponse(BAD_REQUEST, builder));
Expand Down
13 changes: 13 additions & 0 deletions server/src/test/java/org/opensearch/rest/RestControllerTests.java
Original file line number Diff line number Diff line change
Expand Up @@ -553,6 +553,15 @@ public void testFaviconWithWrongHttpMethod() {
assertThat(channel.getRestResponse().getHeaders().get("Allow"), hasItem(equalTo(RestRequest.Method.GET.toString())));
}

public void testHandleBadRequestWithHtmlSpecialCharsInUri() {
final FakeRestRequest fakeRestRequest = new FakeRestRequest.Builder(NamedXContentRegistry.EMPTY).withPath(
"/<script>alert('xss');alert(\"&#x6A&#x61&#x76&#x61\");</script>"
).build();
final AssertingChannel channel = new AssertingChannel(fakeRestRequest, true, RestStatus.BAD_REQUEST);
restController.dispatchRequest(fakeRestRequest, channel, client.threadPool().getThreadContext());
assertThat(channel.getRestResponse().content().utf8ToString(), containsString("invalid uri has been requested"));
}

public void testDispatchUnsupportedHttpMethod() {
final boolean hasContent = randomBoolean();
final RestRequest request = RestRequest.request(xContentRegistry(), new HttpRequest() {
Expand Down Expand Up @@ -623,6 +632,10 @@ public Exception getInboundException() {
assertTrue(channel.getSendResponseCalled());
assertThat(channel.getRestResponse().getHeaders().containsKey("Allow"), equalTo(true));
assertThat(channel.getRestResponse().getHeaders().get("Allow"), hasItem(equalTo(RestRequest.Method.GET.toString())));
assertThat(
channel.getRestResponse().content().utf8ToString(),
equalTo("{\"error\":\"Unexpected HTTP method, allowed: [GET]\",\"status\":405}")
);
}

private static final class TestHttpServerTransport extends AbstractLifecycleComponent implements HttpServerTransport {
Expand Down

0 comments on commit 2651637

Please sign in to comment.