From 1919c8be7ee71152abd4f310100cd6e3e8669342 Mon Sep 17 00:00:00 2001 From: Pasqual Koschmieder Date: Wed, 9 Oct 2024 22:01:57 +0200 Subject: [PATCH 1/2] send bad request response on decode failure --- .../rest/netty/NettyHttpServerHandler.java | 7 ++-- .../ext/rest/netty/NettyHttpServerUtil.java | 40 +++++++++++++++++++ .../NettyOversizedClosingHttpAggregator.java | 14 +------ 3 files changed, 45 insertions(+), 16 deletions(-) create mode 100644 web-impl-netty/src/main/java/eu/cloudnetservice/ext/rest/netty/NettyHttpServerUtil.java diff --git a/web-impl-netty/src/main/java/eu/cloudnetservice/ext/rest/netty/NettyHttpServerHandler.java b/web-impl-netty/src/main/java/eu/cloudnetservice/ext/rest/netty/NettyHttpServerHandler.java index 20c46e5d..561a5633 100644 --- a/web-impl-netty/src/main/java/eu/cloudnetservice/ext/rest/netty/NettyHttpServerHandler.java +++ b/web-impl-netty/src/main/java/eu/cloudnetservice/ext/rest/netty/NettyHttpServerHandler.java @@ -127,9 +127,10 @@ public void channelReadComplete(@NonNull ChannelHandlerContext ctx) { protected void messageReceived(@NonNull ChannelHandlerContext ctx, @NonNull HttpRequest msg) { // validate that the request was actually decoded before processing if (msg.decoderResult().isFailure()) { - ctx.channel().close(); + NettyHttpServerUtil.sendResponseAndClose(ctx, HttpResponseStatus.BAD_REQUEST); return; } + // handle the message inside the executor from here on Send buffer; if (msg instanceof FullHttpRequest request) { @@ -159,9 +160,7 @@ private void handleMessage( // to the lack of path information which is the base of our internal handling) var uri = URI.create(httpRequest.uri()); if (uri.isOpaque()) { - channel - .writeAndFlush(new DefaultHttpResponse(httpRequest.protocolVersion(), HttpResponseStatus.BAD_REQUEST)) - .addListener(channel, ChannelFutureListeners.CLOSE); + NettyHttpServerUtil.sendResponseAndClose(channel, HttpResponseStatus.BAD_REQUEST); return; } diff --git a/web-impl-netty/src/main/java/eu/cloudnetservice/ext/rest/netty/NettyHttpServerUtil.java b/web-impl-netty/src/main/java/eu/cloudnetservice/ext/rest/netty/NettyHttpServerUtil.java new file mode 100644 index 00000000..d239d731 --- /dev/null +++ b/web-impl-netty/src/main/java/eu/cloudnetservice/ext/rest/netty/NettyHttpServerUtil.java @@ -0,0 +1,40 @@ +/* + * Copyright 2019-2024 CloudNetService team & contributors + * + * 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 eu.cloudnetservice.ext.rest.netty; + +import io.netty5.channel.ChannelOutboundInvoker; +import io.netty5.handler.codec.http.DefaultHttpResponse; +import io.netty5.handler.codec.http.HttpHeaderNames; +import io.netty5.handler.codec.http.HttpHeaderValues; +import io.netty5.handler.codec.http.HttpResponseStatus; +import io.netty5.handler.codec.http.HttpVersion; +import lombok.NonNull; + +final class NettyHttpServerUtil { + + private NettyHttpServerUtil() { + throw new UnsupportedOperationException(); + } + + public static void sendResponseAndClose(@NonNull ChannelOutboundInvoker channel, @NonNull HttpResponseStatus status) { + var response = new DefaultHttpResponse(HttpVersion.HTTP_1_1, status); + response.headers() + .set(HttpHeaderNames.CONNECTION, HttpHeaderValues.CLOSE) + .set(HttpHeaderNames.CONTENT_LENGTH, HttpHeaderValues.ZERO); + channel.writeAndFlush(response).addListener(ignored -> channel.close()); + } +} diff --git a/web-impl-netty/src/main/java/eu/cloudnetservice/ext/rest/netty/NettyOversizedClosingHttpAggregator.java b/web-impl-netty/src/main/java/eu/cloudnetservice/ext/rest/netty/NettyOversizedClosingHttpAggregator.java index dd436025..8c242276 100644 --- a/web-impl-netty/src/main/java/eu/cloudnetservice/ext/rest/netty/NettyOversizedClosingHttpAggregator.java +++ b/web-impl-netty/src/main/java/eu/cloudnetservice/ext/rest/netty/NettyOversizedClosingHttpAggregator.java @@ -17,10 +17,7 @@ package eu.cloudnetservice.ext.rest.netty; import io.netty5.channel.ChannelHandlerContext; -import io.netty5.handler.codec.http.DefaultFullHttpResponse; import io.netty5.handler.codec.http.HttpContent; -import io.netty5.handler.codec.http.HttpHeaderNames; -import io.netty5.handler.codec.http.HttpHeaderValues; import io.netty5.handler.codec.http.HttpObjectAggregator; import io.netty5.handler.codec.http.HttpRequest; import io.netty5.handler.codec.http.HttpResponseStatus; @@ -34,18 +31,11 @@ public NettyOversizedClosingHttpAggregator(int maxContentLength) { @Override protected void handleOversizedMessage(@NonNull ChannelHandlerContext ctx, @NonNull Object tooLarge) throws Exception { - if (tooLarge instanceof HttpRequest httpRequest) { + if (tooLarge instanceof HttpRequest) { // always the close the connection when the client sent a too large request body as this leaves // the decoder and this aggregator in a mismatched state which would cause the following request // with a too large body to hand forever - var response = new DefaultFullHttpResponse( - httpRequest.protocolVersion(), - HttpResponseStatus.REQUEST_ENTITY_TOO_LARGE, - ctx.bufferAllocator().allocate(0)); - response.headers() - .set(HttpHeaderNames.CONNECTION, HttpHeaderValues.CLOSE) - .set(HttpHeaderNames.CONTENT_LENGTH, HttpHeaderValues.ZERO); - ctx.writeAndFlush(response).addListener(ignored -> ctx.close()); + NettyHttpServerUtil.sendResponseAndClose(ctx, HttpResponseStatus.REQUEST_ENTITY_TOO_LARGE); } else { // not a request, use the default handling super.handleOversizedMessage(ctx, tooLarge); From 5a65bf568eddf7eaaebf7f19917248b2262a6f48 Mon Sep 17 00:00:00 2001 From: Pasqual Koschmieder Date: Fri, 11 Oct 2024 21:37:57 +0200 Subject: [PATCH 2/2] add test --- .../ext/rest/netty/HttpServerTestUtil.java | 39 ++++++++++++ .../ext/rest/netty/NettyHttpServerTest.java | 60 +++++++++++++++++++ 2 files changed, 99 insertions(+) create mode 100644 web-impl-netty/src/test/java/eu/cloudnetservice/ext/rest/netty/HttpServerTestUtil.java create mode 100644 web-impl-netty/src/test/java/eu/cloudnetservice/ext/rest/netty/NettyHttpServerTest.java diff --git a/web-impl-netty/src/test/java/eu/cloudnetservice/ext/rest/netty/HttpServerTestUtil.java b/web-impl-netty/src/test/java/eu/cloudnetservice/ext/rest/netty/HttpServerTestUtil.java new file mode 100644 index 00000000..d8aa5bf0 --- /dev/null +++ b/web-impl-netty/src/test/java/eu/cloudnetservice/ext/rest/netty/HttpServerTestUtil.java @@ -0,0 +1,39 @@ +/* + * Copyright 2019-2024 CloudNetService team & contributors + * + * 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 eu.cloudnetservice.ext.rest.netty; + +import eu.cloudnetservice.ext.rest.api.util.HostAndPort; +import java.io.IOException; +import java.net.InetSocketAddress; +import java.net.ServerSocket; + +public final class HttpServerTestUtil { + + private HttpServerTestUtil() { + throw new UnsupportedOperationException(); + } + + public static HostAndPort resolveFreeHost() { + try (var socket = new ServerSocket()) { + socket.setReuseAddress(true); + socket.bind(new InetSocketAddress("127.0.0.1", 0)); + return new HostAndPort("127.0.0.1", socket.getLocalPort()); + } catch (IOException exception) { + throw new IllegalStateException("Failed to find free port to bind to", exception); + } + } +} diff --git a/web-impl-netty/src/test/java/eu/cloudnetservice/ext/rest/netty/NettyHttpServerTest.java b/web-impl-netty/src/test/java/eu/cloudnetservice/ext/rest/netty/NettyHttpServerTest.java new file mode 100644 index 00000000..4a0144dc --- /dev/null +++ b/web-impl-netty/src/test/java/eu/cloudnetservice/ext/rest/netty/NettyHttpServerTest.java @@ -0,0 +1,60 @@ +/* + * Copyright 2019-2024 CloudNetService team & contributors + * + * 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 eu.cloudnetservice.ext.rest.netty; + +import eu.cloudnetservice.ext.rest.api.config.ComponentConfig; +import java.io.BufferedReader; +import java.io.InputStreamReader; +import java.net.InetSocketAddress; +import java.net.Socket; +import java.nio.charset.StandardCharsets; +import java.util.concurrent.Executors; +import org.junit.jupiter.api.Assertions; +import org.junit.jupiter.api.Test; + +public class NettyHttpServerTest { + + @Test + void testRejectsRequestWithBadChar() throws Exception { + var bindHost = HttpServerTestUtil.resolveFreeHost(); + var config = ComponentConfig.builder().executorService(Executors.newSingleThreadExecutor()).build(); + var server = new NettyHttpServer(config); + server.addListener(bindHost).join(); + + var socket = new Socket(); + socket.setReuseAddress(true); + socket.connect(new InetSocketAddress(bindHost.host(), bindHost.port())); + + var out = socket.getOutputStream(); + out.write( + """ + GET / HTTP/1.1 + Test: Hello\u0001World + Content-Type: application/json + Content-Length: 0 + + """.getBytes(StandardCharsets.UTF_8)); + out.flush(); + + var reader = new BufferedReader(new InputStreamReader(socket.getInputStream(), StandardCharsets.UTF_8)); + var lines = reader.lines().toList(); + Assertions.assertEquals("HTTP/1.1 400 Bad Request", lines.getFirst()); + Assertions.assertTrue(lines.contains("content-length: 0")); + Assertions.assertTrue(lines.contains("connection: close")); + Assertions.assertNull(reader.readLine()); // server should've closed the connection now + } +}