diff --git a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpGenerator.java b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpGenerator.java index b1282d67bd49..9b6c70bfc0da 100644 --- a/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpGenerator.java +++ b/jetty-core/jetty-http/src/main/java/org/eclipse/jetty/http/HttpGenerator.java @@ -575,6 +575,7 @@ private void generateHeaders(ByteBuffer header, ByteBuffer content, boolean last // default field values HttpField transferEncoding = null; + boolean http10 = _info.getHttpVersion() == HttpVersion.HTTP_1_0; boolean http11 = _info.getHttpVersion() == HttpVersion.HTTP_1_1; boolean close = false; boolean chunkedHint = _info.getTrailersSupplier() != null; @@ -627,23 +628,36 @@ else if (contentLength != field.getLongValue()) case CONNECTION: { - boolean keepAlive = field.contains(HttpHeaderValue.KEEP_ALIVE.asString()); - if (keepAlive && _info.getHttpVersion() == HttpVersion.HTTP_1_0 && _persistent == null) + boolean hasValue = StringUtil.isNotBlank(field.getValue()); + boolean hasKeepAlive = field.contains(HttpHeaderValue.KEEP_ALIVE.asString()); + boolean hasClose = field.contains(HttpHeaderValue.CLOSE.asString()); + + if (http10 && hasKeepAlive) { - _persistent = true; + // set persistence if unset + if (_persistent == null) + _persistent = true; } - if (field.contains(HttpHeaderValue.CLOSE.asString())) + else if (hasClose) { close = true; _persistent = false; } - if (keepAlive && _persistent == Boolean.FALSE) + + if (_persistent == Boolean.FALSE && hasKeepAlive) { - field = new HttpField(HttpHeader.CONNECTION, - Stream.of(field.getValues()).filter(s -> !HttpHeaderValue.KEEP_ALIVE.is(s)) - .collect(Collectors.joining(", "))); + // we need to strip the keep-alive values + String resultingValue = Stream.of(field.getValues()) + .filter(s -> !HttpHeaderValue.KEEP_ALIVE.is(s)) + .collect(Collectors.joining(", ")); + if (StringUtil.isBlank(resultingValue)) + hasValue = false; // all values stripped, do not produce a Connection header with no values + else + field = new HttpField(HttpHeader.CONNECTION, resultingValue); } - putTo(field, header); + + if (hasValue) + putTo(field, header); break; } diff --git a/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/HttpGeneratorServerTest.java b/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/HttpGeneratorServerTest.java index dd71d8658348..bbfcd62315d3 100644 --- a/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/HttpGeneratorServerTest.java +++ b/jetty-core/jetty-http/src/test/java/org/eclipse/jetty/http/HttpGeneratorServerTest.java @@ -819,6 +819,7 @@ public void testConnectionKeepAliveWithAdditionalCustomValue() throws Exception public void testKeepAliveWithClose() throws Exception { HttpGenerator generator = new HttpGenerator(); + generator.setPersistent(false); HttpFields.Mutable fields = HttpFields.build(); fields.put(HttpHeader.CONNECTION, HttpHeaderValue.KEEP_ALIVE.asString() + ", other, " + HttpHeaderValue.CLOSE.asString()); MetaData.Response info = new MetaData.Response(200, "OK", HttpVersion.HTTP_1_0, fields); diff --git a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java index 3d479670005f..1abea8b0dd76 100644 --- a/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java +++ b/jetty-core/jetty-server/src/main/java/org/eclipse/jetty/server/internal/HttpConnection.java @@ -25,6 +25,7 @@ import java.util.concurrent.atomic.AtomicLong; import java.util.concurrent.atomic.AtomicReference; import java.util.concurrent.atomic.LongAdder; +import java.util.stream.Collectors; import org.eclipse.jetty.http.BadMessageException; import org.eclipse.jetty.http.ComplianceViolation; @@ -1382,10 +1383,58 @@ public void demand() } @Override - public void prepareResponse(HttpFields.Mutable headers) + public void prepareResponse(HttpFields.Mutable responseHeaders) { - if (_connectionKeepAlive && _version == HttpVersion.HTTP_1_0 && !headers.contains(HttpHeader.CONNECTION, HttpHeaderValue.CLOSE.asString())) - headers.add(HttpFields.CONNECTION_KEEPALIVE); + boolean hasConnectionClose = responseHeaders.contains(HttpHeader.CONNECTION, HttpHeaderValue.CLOSE.asString()); + boolean hasConnectionKeepAlive = responseHeaders.contains(HttpHeader.CONNECTION, HttpHeaderValue.KEEP_ALIVE.toString()); + + if (_version == HttpVersion.HTTP_1_0 && _generator.isPersistent() && _connectionKeepAlive && !hasConnectionClose) + { + // attempt to cover HTTP/1.0 case where the Connection response header + // doesn't have a close value (set by the application) and it needs to + // be represented as a `Connection: keep-alive` response instead due + // to the existence of the `Connection: keep-alive` request header + responseHeaders.add(HttpFields.CONNECTION_KEEPALIVE); + } + + if (!_generator.isPersistent()) + { + if (!hasConnectionClose) + { + // Add missing "Connection: close" if not persistent + responseHeaders.add(HttpHeader.CONNECTION, HttpHeaderValue.CLOSE); + } + + if (hasConnectionKeepAlive) + { + // if generator is not persistent, strip any keep-alive + // response header values set by the application regardless of + // HTTP/1.0 and HTTP/1.1 differences + String resultingValue = responseHeaders.getValuesList(HttpHeader.CONNECTION) + .stream() + .filter(s -> !HttpHeaderValue.KEEP_ALIVE.is(s)) + .collect(Collectors.joining(", ")); + if (StringUtil.isBlank(resultingValue)) + responseHeaders.remove(HttpHeader.CONNECTION); + else + responseHeaders.put(HttpHeader.CONNECTION, resultingValue); + } + } + else + { + // Strip "keep-alive" when using HTTP/1.1 on persistent connections + if (_version == HttpVersion.HTTP_1_1 && hasConnectionKeepAlive) + { + String resultingValue = responseHeaders.getValuesList(HttpHeader.CONNECTION) + .stream() + .filter(s -> !HttpHeaderValue.KEEP_ALIVE.is(s)) + .collect(Collectors.joining(", ")); + if (StringUtil.isBlank(resultingValue)) + responseHeaders.remove(HttpHeader.CONNECTION); + else + responseHeaders.put(HttpHeader.CONNECTION, resultingValue); + } + } } @Override diff --git a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/HttpChannel.java b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/HttpChannel.java index f705b229362d..cd2e87e04c83 100644 --- a/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/HttpChannel.java +++ b/jetty-ee9/jetty-ee9-nested/src/main/java/org/eclipse/jetty/ee9/nested/HttpChannel.java @@ -694,7 +694,6 @@ public void ensureConsumeAllOrNotPersistent() if (!HttpField.contains(coalesced, HttpHeaderValue.CLOSE.asString())) coalesced += ", close"; - // Returns a single Cookie header with all cookies. return new HttpField(HttpHeader.CONNECTION, coalesced); }); diff --git a/jetty-ee9/jetty-ee9-nested/src/test/java/org/eclipse/jetty/ee9/nested/ErrorHandlerTest.java b/jetty-ee9/jetty-ee9-nested/src/test/java/org/eclipse/jetty/ee9/nested/ErrorHandlerTest.java index f9ec70681b74..7eecefcdac89 100644 --- a/jetty-ee9/jetty-ee9-nested/src/test/java/org/eclipse/jetty/ee9/nested/ErrorHandlerTest.java +++ b/jetty-ee9/jetty-ee9-nested/src/test/java/org/eclipse/jetty/ee9/nested/ErrorHandlerTest.java @@ -31,6 +31,7 @@ import org.eclipse.jetty.http.BadMessageException; import org.eclipse.jetty.http.HttpHeader; import org.eclipse.jetty.http.HttpTester; +import org.eclipse.jetty.io.Content; import org.eclipse.jetty.logging.StacklessLogging; import org.eclipse.jetty.server.LocalConnector; import org.eclipse.jetty.server.Server; @@ -38,7 +39,6 @@ import org.eclipse.jetty.util.ajax.JSON; import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.BeforeEach; -import org.junit.jupiter.api.Disabled; import org.junit.jupiter.api.Test; import org.junit.jupiter.params.ParameterizedTest; import org.junit.jupiter.params.provider.ValueSource; @@ -55,7 +55,6 @@ import static org.hamcrest.Matchers.nullValue; import static org.junit.jupiter.api.Assertions.assertTrue; -@Disabled // TODO public class ErrorHandlerTest { StacklessLogging stacklessLogging; @@ -281,22 +280,26 @@ public void test404PostHttp11() throws Exception @Test public void test404PostCantConsumeHttp10() throws Exception { - String rawResponse = connector.getResponse( - "POST / HTTP/1.0\r\n" + - "Host: Localhost\r\n" + - "Accept: text/html\r\n" + - "Content-Length: 100\r\n" + - "Connection: keep-alive\r\n" + - "\r\n" + - "0123456789"); - + String rawRequest = """ + POST / HTTP/1.0\r + Host: Localhost\r + Accept: text/html\r + Content-Length: 100\r + Connection: keep-alive\r + \r + 0123456789 + """; + + String rawResponse = connector.getResponse(rawRequest); HttpTester.Response response = HttpTester.parseResponse(rawResponse); + dump(response); + assertThat(response.getStatus(), is(404)); assertThat(response.getField(HttpHeader.CONTENT_LENGTH).getIntValue(), greaterThan(0)); assertThat(response.get(HttpHeader.CONTENT_TYPE), containsString("text/html;charset=ISO-8859-1")); assertThat(response.getContent(), containsString("content=\"text/html;charset=ISO-8859-1\"")); - assertThat(response.getField(HttpHeader.CONNECTION), nullValue()); + assertThat(response.get(HttpHeader.CONNECTION), is("close")); assertContent(response); } @@ -308,7 +311,6 @@ public void test404PostCantConsumeHttp11() throws Exception "Host: Localhost\r\n" + "Accept: text/html\r\n" + "Content-Length: 100\r\n" + - "Connection: keep-alive\r\n" + // This is not need by HTTP/1.1 but sometimes sent anyway "\r\n" + "0123456789"); @@ -703,7 +705,7 @@ public void testErrorContextRecycle() throws Exception context.setErrorHandler(new ErrorHandler() { @Override - public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException + public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException { baseRequest.setHandled(true); response.getOutputStream().println("Context Error"); @@ -717,35 +719,32 @@ public void handle(String target, Request baseRequest, HttpServletRequest reques response.sendError(444); } }); - - context.setErrorHandler(new ErrorHandler() + server.setErrorHandler((request, response, callback) -> { - @Override - public void handle(String target, Request baseRequest, HttpServletRequest request, HttpServletResponse response) throws IOException, ServletException - { - baseRequest.setHandled(true); - response.getOutputStream().println("Server Error"); - } + Content.Sink.write(response, true, "Server Error", callback); + return true; }); server.start(); - LocalConnector.LocalEndPoint connection = connector.connect(); - connection.addInputAndExecute(BufferUtil.toBuffer( - "GET /foo/test HTTP/1.1\r\n" + - "Host: Localhost\r\n" + - "\r\n")); - String response = connection.getResponse(); - - assertThat(response, containsString("HTTP/1.1 444 444")); - assertThat(response, containsString("Context Error")); - - connection.addInputAndExecute(BufferUtil.toBuffer( - "GET /test HTTP/1.1\r\n" + - "Host: Localhost\r\n" + - "\r\n")); - response = connection.getResponse(); - assertThat(response, containsString("HTTP/1.1 404 Not Found")); - assertThat(response, containsString("Server Error")); + try (LocalConnector.LocalEndPoint connection = connector.connect()) + { + connection.addInputAndExecute(BufferUtil.toBuffer( + "GET /foo/test HTTP/1.1\r\n" + + "Host: Localhost\r\n" + + "\r\n")); + String response = connection.getResponse(); + + assertThat(response, containsString("HTTP/1.1 444 444")); + assertThat(response, containsString("Context Error")); + + connection.addInputAndExecute(BufferUtil.toBuffer( + "GET /test HTTP/1.1\r\n" + + "Host: Localhost\r\n" + + "\r\n")); + response = connection.getResponse(); + assertThat(response, containsString("HTTP/1.1 404 Not Found")); + assertThat(response, containsString("Server Error")); + } } } diff --git a/jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/ResponseHeadersTest.java b/jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/ResponseHeadersTest.java index f24a77fa5e5b..133a4e6cda25 100644 --- a/jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/ResponseHeadersTest.java +++ b/jetty-ee9/jetty-ee9-servlet/src/test/java/org/eclipse/jetty/ee9/servlet/ResponseHeadersTest.java @@ -19,7 +19,11 @@ import java.nio.ByteBuffer; import java.nio.charset.StandardCharsets; import java.nio.file.Path; +import java.util.ArrayList; import java.util.EnumSet; +import java.util.List; +import java.util.function.Consumer; +import java.util.stream.Stream; import jakarta.servlet.DispatcherType; import jakarta.servlet.FilterChain; @@ -31,6 +35,7 @@ import jakarta.servlet.http.HttpServletRequest; import jakarta.servlet.http.HttpServletResponse; import jakarta.servlet.http.HttpServletResponseWrapper; +import org.eclipse.jetty.http.HttpHeader; import org.eclipse.jetty.http.HttpTester; import org.eclipse.jetty.http.HttpVersion; import org.eclipse.jetty.http.UriCompliance; @@ -39,12 +44,15 @@ import org.eclipse.jetty.server.Server; import org.eclipse.jetty.toolchain.test.MavenPaths; import org.eclipse.jetty.util.StringUtil; -import org.junit.jupiter.api.AfterAll; -import org.junit.jupiter.api.BeforeAll; +import org.junit.jupiter.api.AfterEach; import org.junit.jupiter.api.Test; +import org.junit.jupiter.params.ParameterizedTest; +import org.junit.jupiter.params.provider.Arguments; +import org.junit.jupiter.params.provider.MethodSource; import static org.hamcrest.MatcherAssert.assertThat; import static org.hamcrest.Matchers.is; +import static org.hamcrest.Matchers.nullValue; import static org.junit.jupiter.api.Assertions.assertTrue; public class ResponseHeadersTest @@ -150,11 +158,24 @@ protected void doGet(HttpServletRequest request, HttpServletResponse response) t } } - private static Server server; - private static LocalConnector connector; + private Server server; + private LocalConnector connector; - @BeforeAll - public static void startServer() throws Exception + public void startServer() throws Exception + { + startServer((context) -> + { + context.addServlet(new ServletHolder(new DefaultServlet()), "/default/*"); + context.addFilter(new FilterHolder(new WrappingFilter()), "/default/*", EnumSet.allOf(DispatcherType.class)); + context.addServlet(new ServletHolder(new SimulateUpgradeServlet()), "/ws/*"); + context.addServlet(new ServletHolder(new MultilineResponseValueServlet()), "/multiline/*"); + context.addServlet(CharsetResetToJsonMimeTypeServlet.class, "/charset/json-reset/*"); + context.addServlet(CharsetChangeToJsonMimeTypeServlet.class, "/charset/json-change/*"); + context.addServlet(CharsetChangeToJsonMimeTypeSetCharsetToNullServlet.class, "/charset/json-change-null/*"); + }); + } + + public void startServer(Consumer configureServletContext) throws Exception { Path staticContentPath = MavenPaths.findTestResourceDir("contextResources"); server = new Server(); @@ -165,21 +186,15 @@ public static void startServer() throws Exception context.setContextPath("/"); context.setBaseResourceAsPath(staticContentPath); context.setInitParameter("org.eclipse.jetty.servlet.Default.pathInfoOnly", "TRUE"); - server.setHandler(context); - context.addServlet(new ServletHolder(new DefaultServlet()), "/default/*"); - context.addFilter(new FilterHolder(new WrappingFilter()), "/default/*", EnumSet.allOf(DispatcherType.class)); - context.addServlet(new ServletHolder(new SimulateUpgradeServlet()), "/ws/*"); - context.addServlet(new ServletHolder(new MultilineResponseValueServlet()), "/multiline/*"); - context.addServlet(CharsetResetToJsonMimeTypeServlet.class, "/charset/json-reset/*"); - context.addServlet(CharsetChangeToJsonMimeTypeServlet.class, "/charset/json-change/*"); - context.addServlet(CharsetChangeToJsonMimeTypeSetCharsetToNullServlet.class, "/charset/json-change-null/*"); + configureServletContext.accept(context); + server.setHandler(context); server.start(); } - @AfterAll - public static void stopServer() + @AfterEach + public void stopServer() { try { @@ -194,6 +209,7 @@ public static void stopServer() @Test public void testWrappedResponseWithStaticContent() throws Exception { + startServer(); HttpTester.Request request = new HttpTester.Request(); request.setMethod("GET"); request.setURI("/default/test.txt"); @@ -208,6 +224,7 @@ public void testWrappedResponseWithStaticContent() throws Exception @Test public void testResponseWebSocketHeaderFormat() throws Exception { + startServer(); HttpTester.Request request = new HttpTester.Request(); request.setMethod("GET"); request.setURI("/ws/"); @@ -226,6 +243,7 @@ public void testResponseWebSocketHeaderFormat() throws Exception @Test public void testMultilineResponseHeaderValue() throws Exception { + startServer(); connector.getConnectionFactory(HttpConnectionFactory.class).getHttpConfiguration().setUriCompliance(UriCompliance.UNSAFE); String actualPathInfo = "%0a%20Content-Type%3a%20image/png%0a%20Content-Length%3a%208%0A%20%0A%20yuck