From dd9e97817fe16e0f35223b9940bac101e020157b Mon Sep 17 00:00:00 2001
From: "opensearch-trigger-bot[bot]"
 <98922864+opensearch-trigger-bot[bot]@users.noreply.github.com>
Date: Thu, 2 Jun 2022 13:15:51 -0400
Subject: [PATCH] Filter out invalid URI and HTTP method in the error message
 of no handler found for a REST request (#3459) (#3485)

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>
(cherry picked from commit 2bfe8b31af354e5d00e4ac67d544ce664f083934)

Co-authored-by: Tianli Feng <ftianli@amazon.com>
---
 .../java/org/opensearch/rest/RestController.java   | 14 ++++++++++++--
 .../org/opensearch/rest/RestControllerTests.java   | 13 +++++++++++++
 2 files changed, 25 insertions(+), 2 deletions(-)

diff --git a/server/src/main/java/org/opensearch/rest/RestController.java b/server/src/main/java/org/opensearch/rest/RestController.java
index b576f8b83e5a0..78bebcb9a0af1 100644
--- a/server/src/main/java/org/opensearch/rest/RestController.java
+++ b/server/src/main/java/org/opensearch/rest/RestController.java
@@ -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;
@@ -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);
@@ -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));
diff --git a/server/src/test/java/org/opensearch/rest/RestControllerTests.java b/server/src/test/java/org/opensearch/rest/RestControllerTests.java
index 6004613c0ed17..bd4c7c9a4f824 100644
--- a/server/src/test/java/org/opensearch/rest/RestControllerTests.java
+++ b/server/src/test/java/org/opensearch/rest/RestControllerTests.java
@@ -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() {
@@ -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 {