[4.x] Avoid reflecting back user data in exception messages

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() {
      * <p>
      * 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.
+     * <p>
+     * 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());
     }
 

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<Builder, Reques
         private Http.Status status;
         private Boolean keepAlive;
         private final ServerResponseHeaders responseHeaders = ServerResponseHeaders.create();
+        private boolean safeMessage = true;
 
         private Builder() {
         }
@@ -209,5 +222,17 @@ public Builder header(Http.HeaderValue header) {
             this.responseHeaders.set(header);
             return this;
         }
+
+        /**
+         * Safe message flag that indicates if it safe to return message
+         * as part of the response. Defaults to {@code true}.
+         *
+         * @param safeMessage whether is safe to return message
+         * @return updated builder
+         */
+        public Builder safeMessage(boolean safeMessage) {
+            this.safeMessage = safeMessage;
+            return this;
+        }
     }
 }

nima/tests/integration/webserver/webserver/src/test/java/io/helidon/nima/tests/integration/server/ExceptionMessageTest.java

@@ -0,0 +1,69 @@
+/*
+ * Copyright (c) 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.
+ * 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.nima.tests.integration.server;
+
+import java.util.Collections;
+
+import io.helidon.common.http.Http;
+import io.helidon.common.testing.http.junit5.SocketHttpClient;
+import io.helidon.nima.testing.junit5.webserver.ServerTest;
+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;
+
+/**
+ * Test that no unwanted request data is leaked back (reflected) in response to a
+ * bad request. There are no routes defined for this test.
+ */
+@ServerTest
+class ExceptionMessageTest {
+
+    private final SocketHttpClient socketClient;
+
+    ExceptionMessageTest(SocketHttpClient socketClient) {
+        this.socketClient = socketClient;
+    }
+
+    @Test
+    void testNoUrlReflect() {
+        String response = socketClient.sendAndReceive("/anyjavascript%3a/*%3c/script%3e%3cimg/onerror%3d'\\''" +
+                        "-/%22/-/%20onmouseover%d1/-/[%60*/[]/[(new(Image)).src%3d(/%3b/%2b/255t6qeelp23xlr08hn1uv" +
+                        "vnkeqae02stgk87yvnX%3b.oastifycom/).replace(/.%3b/g%2c[])]//'\\''src%3d%3e",
+                Http.Method.GET,
+                "");
+        Http.Status status = SocketHttpClient.statusFromResponse(response);
+        String entity = SocketHttpClient.entityFromResponse(response, false);
+        assertThat(status, is(Http.Status.BAD_REQUEST_400));
+        assertThat(entity, containsString("see server log"));
+        assertThat(entity, not(containsString("javascript")));
+    }
+
+    @Test
+    void testNoHeaderReflect() {
+        String response = socketClient.sendAndReceive("/",
+                Http.Method.GET,
+                "",
+                Collections.singletonList("<Content-Type>: <javascript/>"));
+        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")));
+    }
+}

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();

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();
     }