Skip to content

Commit

Permalink
Config to prevent reflection of user input when reporting errors (hel…
Browse files Browse the repository at this point in the history
…idon-io#9811)

* New config section in listeners for error handling. Default settings prevent any entity from being returned to avoid reflecting any data from a request. Default settings can be modified to return safe messages and to log all messages.

Signed-off-by: Santiago Pericas-Geertsen <[email protected]>
  • Loading branch information
spericas authored Feb 25, 2025
1 parent 621cdfc commit 888377d
Show file tree
Hide file tree
Showing 10 changed files with 282 additions and 19 deletions.
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2021, 2024 Oracle and/or its affiliates.
* Copyright (c) 2021, 2025 Oracle and/or its affiliates.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -19,6 +19,7 @@
import io.helidon.http.Header;
import io.helidon.http.HeaderValues;
import io.helidon.http.Status;
import io.helidon.microprofile.testing.AddConfig;
import io.helidon.microprofile.testing.junit5.AddBean;
import io.helidon.microprofile.testing.junit5.HelidonTest;
import io.helidon.webclient.api.WebClient;
Expand All @@ -37,6 +38,7 @@

@HelidonTest
@AddBean(BadHostHeaderTest.TestResource.class)
@AddConfig(key = "server.error-handling.include-entity", value = "true")
public class BadHostHeaderTest {
private static final Header BAD_HOST_HEADER = HeaderValues.create("Host", "localhost:808a");

Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2022, 2024 Oracle and/or its affiliates.
* Copyright (c) 2022, 2025 Oracle and/or its affiliates.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand Down Expand Up @@ -61,12 +61,14 @@
import io.helidon.http.http2.WindowSize;
import io.helidon.webserver.CloseConnectionException;
import io.helidon.webserver.ConnectionContext;
import io.helidon.webserver.ErrorHandling;
import io.helidon.webserver.Router;
import io.helidon.webserver.ServerConnectionException;
import io.helidon.webserver.http.HttpRouting;
import io.helidon.webserver.http2.spi.Http2SubProtocolSelector;
import io.helidon.webserver.http2.spi.SubProtocolResult;

import static java.lang.System.Logger.Level.DEBUG;
import static java.lang.System.Logger.Level.TRACE;

/**
Expand Down Expand Up @@ -307,28 +309,44 @@ public void run() {
writer.write(rst.toFrameData(serverSettings, streamId, Http2Flag.NoFlags.create()));
// no sense in throwing an exception, as this is invoked from an executor service directly
} catch (RequestException e) {
// gather error handling properties
ErrorHandling errorHandling = ctx.listenerContext()
.config()
.errorHandling();

// log message in DEBUG mode
if (LOGGER.isLoggable(DEBUG) && (e.safeMessage() || errorHandling.logAllMessages())) {
LOGGER.log(DEBUG, e);
}

// create message to return based on settings
String message = null;
if (errorHandling.includeEntity()) {
message = e.safeMessage() ? e.getMessage() : "Bad request, see server log for more information";
}

DirectHandler handler = ctx.listenerContext()
.directHandlers()
.handler(e.eventType());
DirectHandler.TransportResponse response = handler.handle(e.request(),
e.eventType(),
e.status(),
e.responseHeaders(),
e);
message);

ServerResponseHeaders headers = response.headers();
byte[] message = response.entity().orElse(BufferData.EMPTY_BYTES);
if (message.length != 0) {
headers.set(HeaderValues.create(HeaderNames.CONTENT_LENGTH, String.valueOf(message.length)));
byte[] entity = response.entity().orElse(BufferData.EMPTY_BYTES);
if (entity.length != 0) {
headers.set(HeaderValues.create(HeaderNames.CONTENT_LENGTH, String.valueOf(entity.length)));
}
Http2Headers http2Headers = Http2Headers.create(headers);
if (message.length == 0) {
if (entity.length == 0) {
writer.writeHeaders(http2Headers,
streamId,
Http2Flag.HeaderFlags.create(Http2Flag.END_OF_HEADERS | Http2Flag.END_OF_STREAM),
flowControl.outbound());
} else {
Http2FrameHeader dataHeader = Http2FrameHeader.create(message.length,
Http2FrameHeader dataHeader = Http2FrameHeader.create(entity.length,
Http2FrameTypes.DATA,
Http2Flag.DataFlags.create(Http2Flag.END_OF_STREAM),
streamId);
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2024 Oracle and/or its affiliates.
* Copyright (c) 2024, 2025 Oracle and/or its affiliates.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -22,10 +22,13 @@
import io.helidon.http.Status;
import io.helidon.webclient.api.ClientResponseTyped;
import io.helidon.webclient.http1.Http1Client;
import io.helidon.webserver.ErrorHandling;
import io.helidon.webserver.WebServerConfig;
import io.helidon.webserver.http.HttpRouting;
import io.helidon.webserver.http1.Http1Route;
import io.helidon.webserver.testing.junit5.ServerTest;
import io.helidon.webserver.testing.junit5.SetUpRoute;
import io.helidon.webserver.testing.junit5.SetUpServer;

import org.junit.jupiter.api.Test;

Expand All @@ -42,6 +45,13 @@ class BadHostTest {
this.client = client;
}

@SetUpServer
static void setupServer(WebServerConfig.Builder builder) {
builder.errorHandling(ErrorHandling.builder()
.includeEntity(true) // enable error message entities
.build());
}

@SetUpRoute
static void routing(HttpRouting.Builder builder) {
builder.route(Http1Route.route(Method.GET,
Expand Down
Original file line number Diff line number Diff line change
@@ -0,0 +1,130 @@
/*
* Copyright (c) 2024, 2025 Oracle and/or its affiliates.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
* You may obtain a copy of the License at
*
* http://www.apache.org/licenses/LICENSE-2.0
*
* Unless required by applicable law or agreed to in writing, software
* distributed under the License is distributed on an "AS IS" BASIS,
* WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
* See the License for the specific language governing permissions and
* limitations under the License.
*/

package io.helidon.webserver.tests;

import java.util.List;

import io.helidon.common.testing.http.junit5.SocketHttpClient;
import io.helidon.http.HttpPrologue;
import io.helidon.http.Method;
import io.helidon.webclient.http1.Http1Client;
import io.helidon.webserver.http.HttpRouting;
import io.helidon.webserver.http1.Http1Route;
import io.helidon.webserver.testing.junit5.ServerTest;
import io.helidon.webserver.testing.junit5.SetUpRoute;

import org.junit.jupiter.api.Test;

import static org.hamcrest.CoreMatchers.containsString;
import static org.hamcrest.CoreMatchers.is;
import static org.hamcrest.CoreMatchers.not;
import static org.hamcrest.MatcherAssert.assertThat;

/**
* Verifies that 400 bad request is returned when a bad prologue is sent. Response
* entities must be empty given that {@link io.helidon.webserver.ErrorHandling#includeEntity()}
* is {@code false} by default.
*/
@ServerTest
class BadPrologueNoEntityTest {
private final Http1Client client;
private final SocketHttpClient socketClient;

BadPrologueNoEntityTest(Http1Client client, SocketHttpClient socketClient) {
this.client = client;
this.socketClient = socketClient;
}

@SetUpRoute
static void routing(HttpRouting.Builder builder) {
builder.route(Http1Route.route(Method.GET,
"/",
(req, res) -> {
HttpPrologue prologue = req.prologue();
String fragment = prologue.fragment().hasValue()
? prologue.fragment().rawValue()
: "";
res.send("path: " + prologue.uriPath().rawPath()
+ ", query: " + prologue.query().rawValue()
+ ", fragment: " + fragment);
}));
}

@Test
void testOk() {
String response = client.method(Method.GET)
.requestEntity(String.class);

assertThat(response, is("path: /, query: , fragment: "));
}

@Test
void testBadQuery() {
String response = socketClient.sendAndReceive(Method.GET,
"/?a=<a%20href='/bad-uri.com'/>bad</a>",
null,
List.of());

assertThat(response, containsString("400 Bad Request"));
// beginning of message to the first double quote
assertThat(response, not(containsString("Query contains invalid char: ")));
// end of message from double quote, index of bad char, and bad char
assertThat(response, not(containsString(", index: 2, char: 0x3c")));
assertThat(response, not(containsString(">")));
}

@Test
void testBadQueryCurly() {
String response = socketClient.sendAndReceive(Method.GET,
"/?name=test1{{",
null,
List.of());

assertThat(response, containsString("400 Bad Request"));
// beginning of message to the first double quote
assertThat(response, not(containsString("Query contains invalid char: ")));
// end of message from double quote, index of bad char, and bad char
assertThat(response, not(containsString(", index: 10, char: 0x7b")));
}

@Test
void testBadPath() {
String response = socketClient.sendAndReceive(Method.GET,
"/name{{{{{{{Sdsds<Dhttps:--www.example.com",
null,
List.of());

assertThat(response, containsString("400 Bad Request"));
// for path we are on the safe side, and never return it back (even HTML encoded)
assertThat(response, not(containsString("Bad request, see server log for more information")));
}

@Test
void testBadFragment() {
String response = socketClient.sendAndReceive(Method.GET,
"/?a=b#invalid-fragment>",
null,
List.of());

assertThat(response, containsString("400 Bad Request"));
// beginning of message to the first double quote
assertThat(response, not(containsString("Fragment contains invalid char: ")));
// end of message from double quote, index of bad char, and bad char
assertThat(response, not(containsString(", index: 16, char: 0x3e")));
assertThat(response, not(containsString(">")));
}
}
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2024 Oracle and/or its affiliates.
* Copyright (c) 2024, 2025 Oracle and/or its affiliates.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -22,10 +22,13 @@
import io.helidon.http.HttpPrologue;
import io.helidon.http.Method;
import io.helidon.webclient.http1.Http1Client;
import io.helidon.webserver.ErrorHandling;
import io.helidon.webserver.WebServerConfig;
import io.helidon.webserver.http.HttpRouting;
import io.helidon.webserver.http1.Http1Route;
import io.helidon.webserver.testing.junit5.ServerTest;
import io.helidon.webserver.testing.junit5.SetUpRoute;
import io.helidon.webserver.testing.junit5.SetUpServer;

import org.junit.jupiter.api.Test;

Expand All @@ -34,6 +37,11 @@
import static org.hamcrest.CoreMatchers.not;
import static org.hamcrest.MatcherAssert.assertThat;

/**
* Verifies that 400 bad request is returned when a bad prologue is sent. Enables
* entities in {@link io.helidon.webserver.ErrorHandling} to get details about
* the errors in entities.
*/
@ServerTest
class BadPrologueTest {
private final Http1Client client;
Expand All @@ -44,6 +52,13 @@ class BadPrologueTest {
this.socketClient = socketClient;
}

@SetUpServer
static void setupServer(WebServerConfig.Builder builder) {
builder.errorHandling(ErrorHandling.builder()
.includeEntity(true) // enable error message entities
.build());
}

@SetUpRoute
static void routing(HttpRouting.Builder builder) {
builder.route(Http1Route.route(Method.GET,
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2023 Oracle and/or its affiliates.
* Copyright (c) 2023, 2025 Oracle and/or its affiliates.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -20,6 +20,7 @@
import io.helidon.http.Status;
import io.helidon.webclient.http1.Http1Client;
import io.helidon.webclient.http1.Http1ClientResponse;
import io.helidon.webserver.ErrorHandling;
import io.helidon.webserver.WebServerConfig;
import io.helidon.webserver.http1.Http1Config;
import io.helidon.webserver.http1.Http1ConnectionSelector;
Expand Down Expand Up @@ -49,6 +50,10 @@ class ContentEncodingDisabledTest extends ContentEncodingDisabledAbstract {
super(client);
}

@SetUpServer
static void setupServer(WebServerConfig.Builder builder) {

}
@SetUpServer
static void server(WebServerConfig.Builder server) {
ServerConnectionSelector http1 = Http1ConnectionSelector.builder()
Expand All @@ -58,6 +63,9 @@ static void server(WebServerConfig.Builder server) {
server.addConnectionSelector(http1)
// Content encoding needs to be completely disabled
.contentEncoding(emptyEncodingContext());
server.errorHandling(ErrorHandling.builder()
.includeEntity(true) // enable error message entities
.build());
}

@Test
Expand Down
Original file line number Diff line number Diff line change
@@ -1,5 +1,5 @@
/*
* Copyright (c) 2023 Oracle and/or its affiliates.
* Copyright (c) 2023, 2025 Oracle and/or its affiliates.
*
* Licensed under the Apache License, Version 2.0 (the "License");
* you may not use this file except in compliance with the License.
Expand All @@ -21,7 +21,10 @@
import io.helidon.common.testing.http.junit5.SocketHttpClient;
import io.helidon.http.Method;
import io.helidon.http.Status;
import io.helidon.webserver.ErrorHandling;
import io.helidon.webserver.WebServerConfig;
import io.helidon.webserver.testing.junit5.ServerTest;
import io.helidon.webserver.testing.junit5.SetUpServer;

import org.junit.jupiter.api.Test;

Expand All @@ -43,6 +46,13 @@ class ExceptionMessageTest {
this.socketClient = socketClient;
}

@SetUpServer
static void setupServer(WebServerConfig.Builder builder) {
builder.errorHandling(ErrorHandling.builder()
.includeEntity(true) // enable error message entities
.build());
}

@Test
void testNoUrlReflect() {
String path = "/anyjavascript%3a/*%3c/script%3e%3cimg/onerror%3d'\\''"
Expand Down
Loading

0 comments on commit 888377d

Please sign in to comment.