Skip to content

Commit

Permalink
Escape HTML special characters in the error message of no handler fou…
Browse files Browse the repository at this point in the history
…nd for a REST request

Signed-off-by: Tianli Feng <ftianli@amazon.com>
  • Loading branch information
Tianli Feng committed May 27, 2022
1 parent e647525 commit ef4e190
Show file tree
Hide file tree
Showing 2 changed files with 35 additions and 1 deletion.
24 changes: 23 additions & 1 deletion server/src/main/java/org/opensearch/rest/RestController.java
Original file line number Diff line number Diff line change
Expand Up @@ -488,7 +488,9 @@ 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 + "]");
// Escaping HTML special characters in the error message only aims to satisfy common security scanners.
// There is no vulnerability without escaping, because the response MIME type is application/json, no scripts will be run.
builder.field("error", "no handler found for uri [" + escapeHtml(uri) + "] and method [" + method + "]");
}
builder.endObject();
channel.sendResponse(new BytesRestResponse(BAD_REQUEST, builder));
Expand Down Expand Up @@ -578,4 +580,24 @@ private static CircuitBreaker inFlightRequestsBreaker(CircuitBreakerService circ
// We always obtain a fresh breaker to reflect changes to the breaker configuration.
return circuitBreakerService.getBreaker(CircuitBreaker.IN_FLIGHT_REQUESTS);
}

/**
* Perform an HTML escape operation on a String input to prevent XSS vulnerability.
* @param text the String to be escaped.
* @return The escaped result String.
*/
private String escapeHtml(String text) {
StringBuilder out = new StringBuilder();
for (int i = 0; i < text.length(); i++) {
char c = text.charAt(i);
if (c == '"' || c == '\'' || c == '<' || c == '>' || c == '&') {
out.append("&#");
out.append((int) c);
out.append(';');
} else {
out.append(c);
}
}
return out.toString();
}
}
12 changes: 12 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,18 @@ 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("/&#60;script&#62;alert(&#39;xss&#39;);alert(&#34;&#38;#x6A&#38;#x61&#38;#x76&#38;#x61&#34;);&#60;/script&#62;")
);
}

public void testDispatchUnsupportedHttpMethod() {
final boolean hasContent = randomBoolean();
final RestRequest request = RestRequest.request(xContentRegistry(), new HttpRequest() {
Expand Down

0 comments on commit ef4e190

Please sign in to comment.