From f78f92e97f8638b72e31b33d277b02e25ba66c4d Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Fri, 29 Apr 2022 21:47:57 +0200 Subject: [PATCH 1/2] Fixes #7935 - Review HTTP/2 error handling Now returning error handling code as a Runnable. Signed-off-by: Simone Bordet --- .../http2/server/HttpChannelOverHTTP2.java | 12 +- .../jetty/http2/server/BadURITest.java | 153 ++++++++++++++++++ 2 files changed, 157 insertions(+), 8 deletions(-) create mode 100644 jetty-http2/http2-server/src/test/java/org/eclipse/jetty/http2/server/BadURITest.java diff --git a/jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HttpChannelOverHTTP2.java b/jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HttpChannelOverHTTP2.java index 7154ca97f4c1..3a06483c1bac 100644 --- a/jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HttpChannelOverHTTP2.java +++ b/jetty-http2/http2-server/src/main/java/org/eclipse/jetty/http2/server/HttpChannelOverHTTP2.java @@ -158,13 +158,11 @@ public Runnable onRequest(HeadersFrame frame) { if (LOG.isDebugEnabled()) LOG.debug("onRequest", x); - onBadMessage(x); - return null; + return () -> onBadMessage(x); } catch (Throwable x) { - onBadMessage(new BadMessageException(HttpStatus.INTERNAL_SERVER_ERROR_500, null, x)); - return null; + return () -> onBadMessage(new BadMessageException(HttpStatus.INTERNAL_SERVER_ERROR_500, null, x)); } } @@ -190,13 +188,11 @@ public Runnable onPushRequest(MetaData.Request request) } catch (BadMessageException x) { - onBadMessage(x); - return null; + return () -> onBadMessage(x); } catch (Throwable x) { - onBadMessage(new BadMessageException(HttpStatus.INTERNAL_SERVER_ERROR_500, null, x)); - return null; + return () -> onBadMessage(new BadMessageException(HttpStatus.INTERNAL_SERVER_ERROR_500, null, x)); } } diff --git a/jetty-http2/http2-server/src/test/java/org/eclipse/jetty/http2/server/BadURITest.java b/jetty-http2/http2-server/src/test/java/org/eclipse/jetty/http2/server/BadURITest.java new file mode 100644 index 000000000000..1f19868f550f --- /dev/null +++ b/jetty-http2/http2-server/src/test/java/org/eclipse/jetty/http2/server/BadURITest.java @@ -0,0 +1,153 @@ +// +// ======================================================================== +// Copyright (c) 1995-2022 Mort Bay Consulting Pty Ltd and others. +// +// This program and the accompanying materials are made available under the +// terms of the Eclipse Public License v. 2.0 which is available at +// https://www.eclipse.org/legal/epl-2.0, or the Apache License, Version 2.0 +// which is available at https://www.apache.org/licenses/LICENSE-2.0. +// +// SPDX-License-Identifier: EPL-2.0 OR Apache-2.0 +// ======================================================================== +// + +package org.eclipse.jetty.http2.server; + +import java.io.OutputStream; +import java.net.Socket; +import java.nio.ByteBuffer; +import java.util.HashMap; +import java.util.concurrent.CountDownLatch; +import java.util.concurrent.TimeUnit; +import javax.servlet.http.HttpServletRequest; +import javax.servlet.http.HttpServletResponse; + +import org.eclipse.jetty.http.HostPortHttpField; +import org.eclipse.jetty.http.HttpFields; +import org.eclipse.jetty.http.HttpMethod; +import org.eclipse.jetty.http.HttpScheme; +import org.eclipse.jetty.http.HttpVersion; +import org.eclipse.jetty.http.MetaData; +import org.eclipse.jetty.http2.frames.HeadersFrame; +import org.eclipse.jetty.http2.frames.PrefaceFrame; +import org.eclipse.jetty.http2.frames.SettingsFrame; +import org.eclipse.jetty.http2.generator.Generator; +import org.eclipse.jetty.io.ByteBufferPool; +import org.eclipse.jetty.server.Handler; +import org.eclipse.jetty.server.HttpConfiguration; +import org.eclipse.jetty.server.Request; +import org.eclipse.jetty.server.Server; +import org.eclipse.jetty.server.ServerConnector; +import org.eclipse.jetty.server.handler.AbstractHandler; +import org.eclipse.jetty.server.handler.ErrorHandler; +import org.eclipse.jetty.util.BufferUtil; +import org.eclipse.jetty.util.component.LifeCycle; +import org.junit.jupiter.api.AfterEach; +import org.junit.jupiter.api.Test; + +import static org.junit.jupiter.api.Assertions.assertTrue; + +public class BadURITest +{ + private Server server; + private ServerConnector connector; + + protected void startServer(Handler handler) throws Exception + { + server = new Server(); + connector = new ServerConnector(server, 1, 1, new HTTP2CServerConnectionFactory(new HttpConfiguration())); + server.addConnector(connector); + server.setHandler(handler); + server.start(); + } + + @AfterEach + public void dispose() + { + LifeCycle.stop(server); + } + + @Test + public void testBadURI() throws Exception + { + CountDownLatch handlerLatch = new CountDownLatch(1); + startServer(new AbstractHandler() + { + @Override + public void handle(String target, Request jettyRequest, HttpServletRequest request, HttpServletResponse response) + { + jettyRequest.setHandled(true); + handlerLatch.countDown(); + } + }); + + // Remove existing ErrorHandlers. + while (true) + { + ErrorHandler errorHandler = server.getBean(ErrorHandler.class); + if (errorHandler == null) + break; + server.removeBean(errorHandler); + } + + server.addBean(new ErrorHandler() + { + @Override + public ByteBuffer badMessageError(int status, String reason, HttpFields.Mutable fields) + { + // TODO: flow control should be enough. + // Return a very large buffer that will cause HTTP/2 flow control exhaustion and/or TCP congestion. + return ByteBuffer.allocateDirect(128 * 1024 * 1024); + } + }); + + ByteBufferPool byteBufferPool = connector.getByteBufferPool(); + Generator generator = new Generator(byteBufferPool); + + // Craft a request with a bad URI, it will not hit the Handler. + MetaData.Request metaData1 = new MetaData.Request( + HttpMethod.GET.asString(), + HttpScheme.HTTP.asString(), + new HostPortHttpField("localhost:" + connector.getLocalPort()), + // Use an ambiguous path parameter so that the URI is invalid. + "/foo/..;/bar", + HttpVersion.HTTP_2, + HttpFields.EMPTY, + -1 + ); + ByteBufferPool.Lease lease = new ByteBufferPool.Lease(byteBufferPool); + generator.control(lease, new PrefaceFrame()); + generator.control(lease, new SettingsFrame(new HashMap<>(), false)); + generator.control(lease, new HeadersFrame(1, metaData1, null, true)); + + try (Socket client = new Socket("localhost", connector.getLocalPort())) + { + OutputStream output = client.getOutputStream(); + for (ByteBuffer buffer : lease.getByteBuffers()) + { + output.write(BufferUtil.toArray(buffer)); + } + + // Wait for the first request be processed on the server. + Thread.sleep(1000); + + // Send a second request and verify that it hits the Handler. + lease.recycle(); + MetaData.Request metaData2 = new MetaData.Request( + HttpMethod.GET.asString(), + HttpScheme.HTTP.asString(), + new HostPortHttpField("localhost:" + connector.getLocalPort()), + "/valid", + HttpVersion.HTTP_2, + HttpFields.EMPTY, + -1 + ); + generator.control(lease, new HeadersFrame(3, metaData2, null, true)); + for (ByteBuffer buffer : lease.getByteBuffers()) + { + output.write(BufferUtil.toArray(buffer)); + } + assertTrue(handlerLatch.await(5, TimeUnit.SECONDS)); + } + } +} From 569be447c941637606fa3b781ae21b8f880ded70 Mon Sep 17 00:00:00 2001 From: Simone Bordet Date: Sun, 1 May 2022 09:00:03 +0200 Subject: [PATCH 2/2] Fixes #7935 - Review HTTP/2 error handling Updates after review. Signed-off-by: Simone Bordet --- .../java/org/eclipse/jetty/http2/server/BadURITest.java | 6 +----- 1 file changed, 1 insertion(+), 5 deletions(-) diff --git a/jetty-http2/http2-server/src/test/java/org/eclipse/jetty/http2/server/BadURITest.java b/jetty-http2/http2-server/src/test/java/org/eclipse/jetty/http2/server/BadURITest.java index 1f19868f550f..5ff2dced496d 100644 --- a/jetty-http2/http2-server/src/test/java/org/eclipse/jetty/http2/server/BadURITest.java +++ b/jetty-http2/http2-server/src/test/java/org/eclipse/jetty/http2/server/BadURITest.java @@ -82,11 +82,8 @@ public void handle(String target, Request jettyRequest, HttpServletRequest reque }); // Remove existing ErrorHandlers. - while (true) + for (ErrorHandler errorHandler : server.getBeans(ErrorHandler.class)) { - ErrorHandler errorHandler = server.getBean(ErrorHandler.class); - if (errorHandler == null) - break; server.removeBean(errorHandler); } @@ -95,7 +92,6 @@ public void handle(String target, Request jettyRequest, HttpServletRequest reque @Override public ByteBuffer badMessageError(int status, String reason, HttpFields.Mutable fields) { - // TODO: flow control should be enough. // Return a very large buffer that will cause HTTP/2 flow control exhaustion and/or TCP congestion. return ByteBuffer.allocateDirect(128 * 1024 * 1024); }