From 0d1a70f4cb43497dee283a7518a6de76b8c860f4 Mon Sep 17 00:00:00 2001 From: Santiago Pericas-Geertsen Date: Wed, 17 Apr 2024 09:17:51 -0400 Subject: [PATCH 1/2] Proper handling of invalid accept types in metrics feature. If unable to parse any Accept header type, immediately return a 406 Not Acceptable error code. --- .../io/helidon/http/ServerRequestHeaders.java | 37 ++++++++------- .../observe/metrics/MetricsFeature.java | 3 +- .../metrics/TestInvalidAcceptType.java | 46 +++++++++++++++++++ 3 files changed, 68 insertions(+), 18 deletions(-) create mode 100644 webserver/observe/metrics/src/test/java/io/helidon/webserver/observe/metrics/TestInvalidAcceptType.java diff --git a/http/http/src/main/java/io/helidon/http/ServerRequestHeaders.java b/http/http/src/main/java/io/helidon/http/ServerRequestHeaders.java index b97ad8353a0..13044dd4e94 100644 --- a/http/http/src/main/java/io/helidon/http/ServerRequestHeaders.java +++ b/http/http/src/main/java/io/helidon/http/ServerRequestHeaders.java @@ -136,27 +136,30 @@ default Optional bestAccepted(MediaType... mediaTypes) { if (mediaTypes.length == 0) { return Optional.empty(); } - List accepted = acceptedTypes(); - if (accepted.isEmpty()) { - return Optional.of(mediaTypes[0]); - } - double best = 0; - MediaType result = null; - for (MediaType mt : mediaTypes) { - for (HttpMediaType acc : accepted) { - double q = acc.qualityFactor(); - if (q > best && acc.test(mt)) { - if (q == 1) { - return Optional.of(mt); - } else { - best = q; - result = mt; + try { + List accepted = acceptedTypes(); + if (accepted.isEmpty()) { + return Optional.of(mediaTypes[0]); + } + double best = 0; + MediaType result = null; + for (MediaType mt : mediaTypes) { + for (HttpMediaType acc : accepted) { + 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) { + return Optional.empty(); // invalid accept type } - - return Optional.ofNullable(result); } /** diff --git a/webserver/observe/metrics/src/main/java/io/helidon/webserver/observe/metrics/MetricsFeature.java b/webserver/observe/metrics/src/main/java/io/helidon/webserver/observe/metrics/MetricsFeature.java index 6ea0a52b410..453f6c1977e 100644 --- a/webserver/observe/metrics/src/main/java/io/helidon/webserver/observe/metrics/MetricsFeature.java +++ b/webserver/observe/metrics/src/main/java/io/helidon/webserver/observe/metrics/MetricsFeature.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2022, 2023 Oracle and/or its affiliates. + * Copyright (c) 2022, 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. @@ -186,6 +186,7 @@ private void getMatching(ServerRequest req, if (mediaType == null) { res.status(Status.NOT_ACCEPTABLE_406); res.send(); + return; } getOrOptionsMatching(mediaType, res, () -> output(mediaType, diff --git a/webserver/observe/metrics/src/test/java/io/helidon/webserver/observe/metrics/TestInvalidAcceptType.java b/webserver/observe/metrics/src/test/java/io/helidon/webserver/observe/metrics/TestInvalidAcceptType.java new file mode 100644 index 00000000000..654fd23c918 --- /dev/null +++ b/webserver/observe/metrics/src/test/java/io/helidon/webserver/observe/metrics/TestInvalidAcceptType.java @@ -0,0 +1,46 @@ +/* + * 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.webserver.observe.metrics; + +import java.util.List; + +import io.helidon.common.testing.http.junit5.SocketHttpClient; +import io.helidon.http.Method; +import io.helidon.webserver.testing.junit5.ServerTest; + +import org.junit.jupiter.api.Test; + +import static org.hamcrest.CoreMatchers.containsString; +import static org.hamcrest.MatcherAssert.assertThat; + +@ServerTest +class TestInvalidAcceptType { + + private final SocketHttpClient client; + + TestInvalidAcceptType(SocketHttpClient client) { + this.client = client; + } + + @Test + void checkResponseCode406() { + String response = client.sendAndReceive(Method.GET, + "/observe/metrics", + null, + List.of("Accept: application.json")); // invalid media type + assertThat(response, containsString("406 Not Acceptable")); + } +} From 25572c6d7276c1cb8896a3efe07af8a214dc9a0b Mon Sep 17 00:00:00 2001 From: Santiago Pericas-Geertsen Date: Wed, 17 Apr 2024 13:28:35 -0400 Subject: [PATCH 2/2] Generalizes handling of invalid accept types to normal request, not just metrics endpoints. Uses 400 Bad Request for both cases now. New tests. --- .../io/helidon/http/ServerRequestHeaders.java | 2 +- .../http/ServerRequestHeadersImpl.java | 16 ++++++++----- .../metrics/TestInvalidAcceptType.java | 24 +++++++++++++++++-- 3 files changed, 33 insertions(+), 9 deletions(-) diff --git a/http/http/src/main/java/io/helidon/http/ServerRequestHeaders.java b/http/http/src/main/java/io/helidon/http/ServerRequestHeaders.java index 13044dd4e94..38cd32c9b9a 100644 --- a/http/http/src/main/java/io/helidon/http/ServerRequestHeaders.java +++ b/http/http/src/main/java/io/helidon/http/ServerRequestHeaders.java @@ -158,7 +158,7 @@ default Optional bestAccepted(MediaType... mediaTypes) { } return Optional.ofNullable(result); } catch (IllegalArgumentException e) { - return Optional.empty(); // invalid accept type + throw new BadRequestException("Unable to parse Accept header", e); } } diff --git a/http/http/src/main/java/io/helidon/http/ServerRequestHeadersImpl.java b/http/http/src/main/java/io/helidon/http/ServerRequestHeadersImpl.java index 4570ebaca08..20b5d066645 100644 --- a/http/http/src/main/java/io/helidon/http/ServerRequestHeadersImpl.java +++ b/http/http/src/main/java/io/helidon/http/ServerRequestHeadersImpl.java @@ -1,5 +1,5 @@ /* - * Copyright (c) 2022, 2023 Oracle and/or its affiliates. + * Copyright (c) 2022, 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. @@ -82,13 +82,17 @@ public List acceptedTypes() { } else { acceptedTypes = new ArrayList<>(5); - for (String acceptValue : acceptValues) { - List tokenized = HeaderHelper.tokenize(',', acceptValue); - for (String token : tokenized) { - acceptedTypes.add(HttpMediaType.create(token.trim())); + try { + for (String acceptValue : acceptValues) { + List tokenized = HeaderHelper.tokenize(',', acceptValue); + for (String token : tokenized) { + acceptedTypes.add(HttpMediaType.create(token.trim())); + } } + Collections.sort(acceptedTypes); + } catch (IllegalArgumentException e) { + throw new BadRequestException("Unable to parse Accept header", e); } - Collections.sort(acceptedTypes); } cachedAccepted = acceptedTypes; diff --git a/webserver/observe/metrics/src/test/java/io/helidon/webserver/observe/metrics/TestInvalidAcceptType.java b/webserver/observe/metrics/src/test/java/io/helidon/webserver/observe/metrics/TestInvalidAcceptType.java index 654fd23c918..ae015af719f 100644 --- a/webserver/observe/metrics/src/test/java/io/helidon/webserver/observe/metrics/TestInvalidAcceptType.java +++ b/webserver/observe/metrics/src/test/java/io/helidon/webserver/observe/metrics/TestInvalidAcceptType.java @@ -19,8 +19,11 @@ import io.helidon.common.testing.http.junit5.SocketHttpClient; import io.helidon.http.Method; +import io.helidon.webserver.http.HttpRouting; import io.helidon.webserver.testing.junit5.ServerTest; +import io.helidon.webserver.testing.junit5.SetUpRoute; +import jakarta.json.JsonObject; import org.junit.jupiter.api.Test; import static org.hamcrest.CoreMatchers.containsString; @@ -31,16 +34,33 @@ class TestInvalidAcceptType { private final SocketHttpClient client; + @SetUpRoute + static void routing(HttpRouting.Builder router) { + router.post("/echo", (req, res) -> { + JsonObject message = req.content().as(JsonObject.class); + res.send(message); + }); + } + TestInvalidAcceptType(SocketHttpClient client) { this.client = client; } @Test - void checkResponseCode406() { + void checkResponseCode() { + String response = client.sendAndReceive(Method.POST, + "/echo", + "{ }", + List.of("Accept: application.json")); // invalid media type + assertThat(response, containsString("400 Bad Request")); + } + + @Test + void checkResponseCodeMetrics() { String response = client.sendAndReceive(Method.GET, "/observe/metrics", null, List.of("Accept: application.json")); // invalid media type - assertThat(response, containsString("406 Not Acceptable")); + assertThat(response, containsString("400 Bad Request")); } }