From 82444ebaa339b102180f2444c4e62cef3425566d Mon Sep 17 00:00:00 2001 From: Santiago Pericasgeertsen Date: Mon, 12 Jun 2023 15:23:56 -0400 Subject: [PATCH] Avoid reflecting back user data in exception messages. There is a new flag in RequestException that can be used to control when it is safe to include a message in a response entity and when it is not. It is by default set to true (safe) but is now set to false in Http1Prologue to avoid data leaks. New test added. See issue #6986. --- .../io/helidon/common/http/DirectHandler.java | 41 ++++++++++- .../helidon/common/http/RequestException.java | 27 +++++++- .../server/ExceptionMessageTest.java | 69 +++++++++++++++++++ .../nima/webserver/http1/Http1Connection.java | 3 +- .../nima/webserver/http1/Http1Prologue.java | 3 +- 5 files changed, 138 insertions(+), 5 deletions(-) create mode 100644 nima/tests/integration/webserver/webserver/src/test/java/io/helidon/nima/tests/integration/server/ExceptionMessageTest.java diff --git a/common/http/src/main/java/io/helidon/common/http/DirectHandler.java b/common/http/src/main/java/io/helidon/common/http/DirectHandler.java index 5d8017de0fe..c0a4f7d423c 100644 --- a/common/http/src/main/java/io/helidon/common/http/DirectHandler.java +++ b/common/http/src/main/java/io/helidon/common/http/DirectHandler.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2021, 2022 Oracle and/or its affiliates. + * Copyright (c) 2021, 2023 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. @@ -16,6 +16,7 @@ package io.helidon.common.http; +import java.lang.System.Logger.Level; import java.nio.charset.StandardCharsets; import java.util.Arrays; import java.util.List; @@ -29,11 +30,11 @@ */ @FunctionalInterface public interface DirectHandler { + /** * Default handler will HTML encode the message (if any), * use the default status code for the event type, and copy all headers configured. * - * * @return default direct handler */ static DirectHandler defaultHandler() { @@ -45,6 +46,7 @@ static DirectHandler defaultHandler() { *

* This method should be used to return custom status, header and possible entity. * If there is a need to handle more details, please redirect the client to a proper endpoint to handle them. + * This method shall not send an unsafe message back as an entity to avoid potential data leaks. * * @param request request as received with as much known information as possible * @param eventType type of the event @@ -58,6 +60,41 @@ default TransportResponse handle(TransportRequest request, Http.Status defaultStatus, ServerResponseHeaders responseHeaders, Throwable thrown) { + return handle(request, eventType, defaultStatus, responseHeaders, thrown, null); + } + + /** + * Handler of responses that bypass router. + *

+ * This method should be used to return custom status, header and possible entity. + * If there is a need to handle more details, please redirect the client to a proper endpoint to handle them. + * This method shall not send an unsafe message back as an entity to avoid potential data leaks. + * + * @param request request as received with as much known information as possible + * @param eventType type of the event + * @param defaultStatus default status expected to be returned + * @param responseHeaders headers to be added to response + * @param thrown throwable caught as part of processing with possible additional details about the reason of failure + * @param logger Possibly null logger to use for unsafe messages + * @return response to use to return to original request + */ + default TransportResponse handle(TransportRequest request, + EventType eventType, + Http.Status defaultStatus, + ServerResponseHeaders responseHeaders, + Throwable thrown, + System.Logger logger) { + if (thrown instanceof RequestException re) { + if (re.safeMessage()) { + return handle(request, eventType, defaultStatus, responseHeaders, thrown.getMessage()); + } else { + if (logger != null) { + logger.log(Level.ERROR, thrown); + } + return handle(request, eventType, defaultStatus, responseHeaders, + "Bad request, see server log for more information"); + } + } return handle(request, eventType, defaultStatus, responseHeaders, thrown.getMessage()); } diff --git a/common/http/src/main/java/io/helidon/common/http/RequestException.java b/common/http/src/main/java/io/helidon/common/http/RequestException.java index 737a96a2425..0f2e1b4fa37 100644 --- a/common/http/src/main/java/io/helidon/common/http/RequestException.java +++ b/common/http/src/main/java/io/helidon/common/http/RequestException.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2022 Oracle and/or its affiliates. + * Copyright (c) 2022, 2023 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. @@ -27,6 +27,7 @@ public class RequestException extends RuntimeException { private final DirectHandler.TransportRequest transportRequest; private final boolean keepAlive; private final ServerResponseHeaders responseHeaders; + private final boolean safeMessage; /** * A new exception with a predefined status, even type. @@ -41,6 +42,7 @@ protected RequestException(Builder builder) { this.transportRequest = builder.request; this.keepAlive = builder.keepAlive; this.responseHeaders = builder.responseHeaders; + this.safeMessage = builder.safeMessage; } /** @@ -97,6 +99,16 @@ public ServerResponseHeaders responseHeaders() { return responseHeaders; } + /** + * Safe message flag used to control which messages can be sent as + * part of a response and which should only be logged by the server. + * + * @return safe message flag + */ + public boolean safeMessage() { + return safeMessage; + } + /** * Fluent API builder for {@link RequestException}. */ @@ -108,6 +120,7 @@ public static class Builder implements io.helidon.common.Builder: ")); + Http.Status status = SocketHttpClient.statusFromResponse(response); + String entity = SocketHttpClient.entityFromResponse(response, false); + assertThat(status, is(Http.Status.BAD_REQUEST_400)); + assertThat(entity, not(containsString("javascript"))); + } +} diff --git a/nima/webserver/webserver/src/main/java/io/helidon/nima/webserver/http1/Http1Connection.java b/nima/webserver/webserver/src/main/java/io/helidon/nima/webserver/http1/Http1Connection.java index 66312047bf7..3e9dbfb2147 100644 --- a/nima/webserver/webserver/src/main/java/io/helidon/nima/webserver/http1/Http1Connection.java +++ b/nima/webserver/webserver/src/main/java/io/helidon/nima/webserver/http1/Http1Connection.java @@ -364,7 +364,8 @@ private void handleRequestException(RequestException e) { e.eventType(), e.status(), e.responseHeaders(), - e); + e, + LOGGER); BufferData buffer = BufferData.growing(128); ServerResponseHeaders headers = response.headers(); diff --git a/nima/webserver/webserver/src/main/java/io/helidon/nima/webserver/http1/Http1Prologue.java b/nima/webserver/webserver/src/main/java/io/helidon/nima/webserver/http1/Http1Prologue.java index f64b45640e6..7e0fcfd7a9e 100644 --- a/nima/webserver/webserver/src/main/java/io/helidon/nima/webserver/http1/Http1Prologue.java +++ b/nima/webserver/webserver/src/main/java/io/helidon/nima/webserver/http1/Http1Prologue.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2022 Oracle and/or its affiliates. + * Copyright (c) 2022, 2023 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. @@ -74,6 +74,7 @@ private static RequestException badRequest(String message, String method, String .type(DirectHandler.EventType.BAD_REQUEST) .request(DirectTransportRequest.create(protocolAndVersion, method, path)) .message(message) + .safeMessage(false) .build(); }