From 25a6820c956c6df1b2196cb1adcf9d356656c38a Mon Sep 17 00:00:00 2001 From: Santiago Pericas-Geertsen Date: Thu, 18 Apr 2024 18:20:48 -0400 Subject: [PATCH] 3.x: Improves handling of invalid Accept types (#8679) * Improves handling of invalid Accept types by returning 400 Bad Request instead of 500 Internal Error. New tests. See issue #8672. * Moved test to metrics module to avoid cyclic dependencies. Signed-off-by: Santiago Pericas-Geertsen --------- Signed-off-by: Santiago Pericas-Geertsen --- .../io/helidon/metrics/BadRequestTest.java | 167 ++++++++++++++++++ .../helidon/webserver/HashRequestHeaders.java | 41 +++-- .../java/io/helidon/webserver/Response.java | 6 +- 3 files changed, 194 insertions(+), 20 deletions(-) create mode 100644 metrics/metrics/src/test/java/io/helidon/metrics/BadRequestTest.java diff --git a/metrics/metrics/src/test/java/io/helidon/metrics/BadRequestTest.java b/metrics/metrics/src/test/java/io/helidon/metrics/BadRequestTest.java new file mode 100644 index 00000000000..62f045a7aed --- /dev/null +++ b/metrics/metrics/src/test/java/io/helidon/metrics/BadRequestTest.java @@ -0,0 +1,167 @@ +/* + * Copyright (c) 2024 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.metrics; + +import java.io.BufferedReader; +import java.io.IOException; +import java.io.InputStreamReader; +import java.io.OutputStreamWriter; +import java.io.PrintWriter; +import java.net.Socket; +import java.nio.charset.StandardCharsets; +import java.util.LinkedList; +import java.util.List; + +import io.helidon.common.http.Http; +import io.helidon.webserver.Handler; +import io.helidon.webserver.Routing; +import io.helidon.webserver.WebServer; +import io.helidon.media.jsonp.JsonpSupport; + +import jakarta.json.JsonObject; +import org.junit.jupiter.api.AfterAll; +import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.Test; + +import static org.hamcrest.MatcherAssert.assertThat; +import static org.hamcrest.Matchers.containsString; + +/** + * Tests bad types in Accept header. Cannot be placed in webserver module due to + * cyclic dependencies. It needs to use JSON and Metrics, and requires a "plain" + * HTTP client to issue the request. + */ +public class BadRequestTest { + + private static WebServer server; + + @BeforeAll + static void createAndStartServer() { + server = WebServer.builder() + .addMediaSupport(JsonpSupport.create()) + .addRouting(Routing.builder() + .register(MetricsSupport.create()) + .post("/echo", Handler.create(JsonObject.class, + (req, res, entity) -> res.status(Http.Status.OK_200).send(entity)))) + .build(); + server.start().await(); + } + + @AfterAll + static void stopServer() { + server.shutdown().await(); + } + + @Test + void testBadAcceptType() throws Exception { + try (SimpleHttpSocketClient c = new SimpleHttpSocketClient(server)) { + c.request("POST", "/echo", List.of("Accept: application.json"), "{}"); + String result = c.receive(); + assertThat(result, containsString("400 Bad Request")); + } + } + + @Test + void testBadAcceptTypeMetrics() throws Exception { + try (SimpleHttpSocketClient c = new SimpleHttpSocketClient(server)) { + c.request("GET", "/metrics", List.of("Accept: application.json"), ""); + String result = c.receive(); + assertThat(result, containsString("400 Bad Request")); + } + } + + /** + * Simple HTTP socket client to submit HTTP requests. Used by this class to create + * a request with an invalid Accept header. + */ + static class SimpleHttpSocketClient implements AutoCloseable { + static final String EOL = "\r\n"; + private final Socket socket; + private final BufferedReader socketReader; + + SimpleHttpSocketClient(WebServer webServer) throws IOException { + socket = new Socket("localhost", webServer.port()); + socket.setSoTimeout(10000); + socketReader = new BufferedReader(new InputStreamReader(socket.getInputStream(), StandardCharsets.UTF_8)); + } + + String receive() throws IOException { + StringBuilder sb = new StringBuilder(); + String t; + boolean ending = false; + int contentLength = -1; + while ((t = socketReader.readLine()) != null) { + if (t.toLowerCase().startsWith("content-length")) { + int k = t.indexOf(':'); + contentLength = Integer.parseInt(t.substring(k + 1).trim()); + } + sb.append(t).append("\n"); + if ("".equalsIgnoreCase(t) && contentLength >= 0) { + char[] content = new char[contentLength]; + socketReader.read(content); + sb.append(content); + break; + } + if (ending && "".equalsIgnoreCase(t)) { + break; + } + if (!ending && ("0".equalsIgnoreCase(t))) { + ending = true; + } + } + return sb.toString(); + } + + void request(String method, String path, Iterable headers, String payload) + throws IOException { + List usedHeaders = new LinkedList<>(); + if (headers != null) { + headers.forEach(usedHeaders::add); + } + usedHeaders.add(0, "Host: " + "localhost"); + PrintWriter pw = new PrintWriter(new OutputStreamWriter(socket.getOutputStream(), StandardCharsets.UTF_8)); + pw.print(method); + pw.print(" "); + pw.print(path); + pw.print(" "); + pw.print("http"); + pw.print(EOL); + for (String header : usedHeaders) { + pw.print(header); + pw.print(EOL); + } + sendPayload(pw, payload); + pw.print(EOL); + pw.print(EOL); + pw.flush(); + } + + void sendPayload(PrintWriter pw, String payload) { + if (payload != null) { + pw.print("Content-Length: " + payload.length()); + pw.print(EOL); + pw.print(EOL); + pw.print(payload); + pw.print(EOL); + } + } + + @Override + public void close() throws Exception { + socket.close(); + } + } +} diff --git a/webserver/webserver/src/main/java/io/helidon/webserver/HashRequestHeaders.java b/webserver/webserver/src/main/java/io/helidon/webserver/HashRequestHeaders.java index 27b0e14c11d..d2d0004fc27 100644 --- a/webserver/webserver/src/main/java/io/helidon/webserver/HashRequestHeaders.java +++ b/webserver/webserver/src/main/java/io/helidon/webserver/HashRequestHeaders.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2017, 2022 Oracle and/or its affiliates. + * Copyright (c) 2017, 2024 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. @@ -118,7 +118,6 @@ public List acceptedTypes() { if (acceptValues.size() == 1 && HUC_ACCEPT_DEFAULT.equals(acceptValues.get(0))) { result = HUC_ACCEPT_DEFAULT_TYPES; - } else { result = LazyList.create(acceptValues.stream() .flatMap(h -> Utils.tokenize(',', "\"", false, h).stream()) @@ -145,29 +144,33 @@ public Optional bestAccepted(MediaType... mediaTypes) { if (mediaTypes == null || mediaTypes.length == 0) { return Optional.empty(); } - List accepts = acceptedTypes(); - if (accepts == null || accepts.isEmpty()) { - return Optional.ofNullable(mediaTypes[0]); - } + try { + List accepts = acceptedTypes(); + if (accepts == null || accepts.isEmpty()) { + return Optional.ofNullable(mediaTypes[0]); + } - double best = 0; - MediaType result = null; - for (MediaType mt : mediaTypes) { - if (mt != null) { - for (MediaType acc : accepts) { - double q = acc.qualityFactor(); - if (q > best && acc.test(mt)) { - if (q == 1) { - return Optional.of(mt); - } else { - best = q; - result = mt; + double best = 0; + MediaType result = null; + for (MediaType mt : mediaTypes) { + if (mt != null) { + for (MediaType acc : accepts) { + double q = acc.qualityFactor(); + if (q > best && acc.test(mt)) { + if (q == 1) { + return Optional.of(mt); + } else { + best = q; + result = mt; + } } } } } + return Optional.ofNullable(result); + } catch (IllegalArgumentException e) { + throw new BadRequestException("Unable to parse Accept header", e); } - return Optional.ofNullable(result); } @Override diff --git a/webserver/webserver/src/main/java/io/helidon/webserver/Response.java b/webserver/webserver/src/main/java/io/helidon/webserver/Response.java index d480871712d..bb26eba6a56 100644 --- a/webserver/webserver/src/main/java/io/helidon/webserver/Response.java +++ b/webserver/webserver/src/main/java/io/helidon/webserver/Response.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2017, 2022 Oracle and/or its affiliates. + * Copyright (c) 2017, 2024 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. @@ -181,6 +181,10 @@ public Single send(T content) { sendPublisher.subscribe(bareResponse); }, content == null); return whenSent(); + } catch (IllegalStateException e) { + eventListener.finish(); + throw e.getCause() instanceof IllegalArgumentException + ? new BadRequestException("Unable to process request", e) : e; } catch (RuntimeException | Error e) { eventListener.finish(); throw e;