From db511f58c421967aa08a27ccf3464965b5271cdf Mon Sep 17 00:00:00 2001 From: "Ross A. Baker" Date: Fri, 1 Nov 2024 21:20:28 -0400 Subject: [PATCH] Fix Connection persistence according to RFC 9112 --- .../http4s/blaze/client/Http1Connection.scala | 7 ++-- .../org/http4s/blazecore/Http1Stage.scala | 39 ++++++++++++++++++- .../http4s/blazecore/util/NullWriter.scala | 27 +++++++++++++ .../blaze/server/Http1ServerStage.scala | 7 +--- .../blaze/server/ServerTestRoutes.scala | 14 +++++-- 5 files changed, 80 insertions(+), 14 deletions(-) create mode 100644 blaze-core/src/main/scala/org/http4s/blazecore/util/NullWriter.scala diff --git a/blaze-client/src/main/scala/org/http4s/blaze/client/Http1Connection.scala b/blaze-client/src/main/scala/org/http4s/blaze/client/Http1Connection.scala index 893e5e424..209ee3147 100644 --- a/blaze-client/src/main/scala/org/http4s/blaze/client/Http1Connection.scala +++ b/blaze-client/src/main/scala/org/http4s/blaze/client/Http1Connection.scala @@ -32,6 +32,7 @@ import org.http4s.blaze.pipeline.Command.EOF import org.http4s.blazecore.Http1Stage import org.http4s.blazecore.IdleTimeoutStage import org.http4s.blazecore.util.Http1Writer +import org.http4s.blazecore.util.NullWriter import org.http4s.client.RequestKey import org.http4s.headers.Host import org.http4s.headers.`Content-Length` @@ -200,10 +201,8 @@ private final class Http1Connection[F[_]]( if (userAgent.nonEmpty && !req.headers.contains[`User-Agent`]) rr << userAgent.get << "\r\n" - val mustClose: Boolean = req.headers.get[HConnection] match { - case Some(conn) => checkCloseConnection(conn, rr) - case None => getHttpMinor(req) == 0 - } + val mustClose: Boolean = + checkRequestCloseConnection(req.headers.get[HConnection], getHttpMinor(req), NullWriter) val writeRequest: F[Boolean] = getChunkEncoder(req, mustClose, rr) .write(rr, req.body) diff --git a/blaze-core/src/main/scala/org/http4s/blazecore/Http1Stage.scala b/blaze-core/src/main/scala/org/http4s/blazecore/Http1Stage.scala index bd779f527..66d8d1d1d 100644 --- a/blaze-core/src/main/scala/org/http4s/blazecore/Http1Stage.scala +++ b/blaze-core/src/main/scala/org/http4s/blazecore/Http1Stage.scala @@ -60,7 +60,8 @@ private[http4s] trait Http1Stage[F[_]] { self: TailStage[ByteBuffer] => protected def contentComplete(): Boolean /** Check Connection header and add applicable headers to response */ - protected final def checkCloseConnection(conn: Connection, rr: StringWriter): Boolean = + @deprecated("Use checkConnectionPersistence(Option[Connection], Int, Writer) instead", "0.23.17") + protected final def checkCloseConnection(conn: Connection, rr: Writer): Boolean = if (conn.hasKeepAlive) { // connection, look to the request logger.trace("Found Keep-Alive header") false @@ -76,6 +77,42 @@ private[http4s] trait Http1Stage[F[_]] { self: TailStage[ByteBuffer] => true } + /** Checks whether the connection should be closed per the request's Connection header + * and the HTTP version. + * + * As a side effect, writes a "Connection: close" header to the StringWriter if + * the request explicitly requests the connection is closed. + * + * @see [[https://datatracker.ietf.org/doc/html/rfc9112#name-persistence RFC 9112, Section 9.3, Persistence]] + */ + private[http4s] final def checkRequestCloseConnection( + conn: Option[Connection], + minorVersion: Int, + rr: Writer, + ): Boolean = + if (conn.fold(false)(_.hasClose)) { + logger.trace(s"Closing ${conn} due to explicit close option in request's Connection header") + // This side effect doesn't really belong here, but is relied + // upon in multiple places as we look for the encoder. The + // related problem of writing the keep-alive header in HTTP/1.0 + // is handled elsewhere. + if (minorVersion >= 1) { + rr << "Connection: close\r\n" + } + true + } else if (minorVersion >= 1) { + logger.trace(s"Keeping ${conn} alive per default behavior of HTTP >= 1.1") + false + } else if (conn.fold(false)(_.hasKeepAlive)) { + logger.trace( + s"Keeping ${conn} alive due to explicit keep-alive option in request's Connection header" + ) + false + } else { + logger.trace(s"Closing ${conn} per default behavior of HTTP/1.0") + true + } + /** Get the proper body encoder based on the request */ protected final def getEncoder( req: Request[F], diff --git a/blaze-core/src/main/scala/org/http4s/blazecore/util/NullWriter.scala b/blaze-core/src/main/scala/org/http4s/blazecore/util/NullWriter.scala new file mode 100644 index 000000000..96ba57876 --- /dev/null +++ b/blaze-core/src/main/scala/org/http4s/blazecore/util/NullWriter.scala @@ -0,0 +1,27 @@ +/* + * Copyright 2014 http4s.org + * + * 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 org.http4s +package blazecore.util + +import org.http4s.util.Writer + +/** A writer that does not write. Not to be confused with an + * [[EntityBodyWriter]]. + */ +private[http4s] object NullWriter extends Writer { + def append(s: String): NullWriter.type = NullWriter +} diff --git a/blaze-server/src/main/scala/org/http4s/blaze/server/Http1ServerStage.scala b/blaze-server/src/main/scala/org/http4s/blaze/server/Http1ServerStage.scala index b3d36a7b8..d0cffc80a 100644 --- a/blaze-server/src/main/scala/org/http4s/blaze/server/Http1ServerStage.scala +++ b/blaze-server/src/main/scala/org/http4s/blaze/server/Http1ServerStage.scala @@ -245,12 +245,9 @@ private[blaze] class Http1ServerStage[F[_]]( // Need to decide which encoder and if to close on finish val closeOnFinish = respConn .map(_.hasClose) - .orElse { - req.headers.get[Connection].map(checkCloseConnection(_, rr)) - } .getOrElse( - parser.minorVersion() == 0 - ) // Finally, if nobody specifies, http 1.0 defaults to close + checkRequestCloseConnection(req.headers.get[Connection], parser.minorVersion(), rr) + ) // choose a body encoder. Will add a Transfer-Encoding header if necessary val bodyEncoder: Http1Writer[F] = diff --git a/blaze-server/src/test/scala/org/http4s/blaze/server/ServerTestRoutes.scala b/blaze-server/src/test/scala/org/http4s/blaze/server/ServerTestRoutes.scala index 832001d32..c81bff726 100644 --- a/blaze-server/src/test/scala/org/http4s/blaze/server/ServerTestRoutes.scala +++ b/blaze-server/src/test/scala/org/http4s/blaze/server/ServerTestRoutes.scala @@ -57,13 +57,19 @@ object ServerTestRoutes extends Http4sDsl[IO] { // /////////////////////////////// ( "GET /get HTTP/1.0\r\nConnection:close\r\n\r\n", - (Status.Ok, Set(length(3), textPlain, connClose), "get"), + (Status.Ok, Set(length(3), textPlain), "get"), ), // /////////////////////////////// ( "GET /get HTTP/1.1\r\nConnection:close\r\n\r\n", (Status.Ok, Set(length(3), textPlain, connClose), "get"), ), + // /////////////////////////////// + // Don't close connection on an unrecognized Connection header + ( + "GET /get HTTP/1.1\r\nConnection: fiddle-faddle\r\n\r\n", + (Status.Ok, Set(length(3), textPlain), "get"), + ), ("GET /chunked HTTP/1.1\r\n\r\n", (Status.Ok, Set(textPlain, chunked), "chunk")), // /////////////////////////////// ( @@ -75,7 +81,7 @@ object ServerTestRoutes extends Http4sDsl[IO] { // /////////////////////////////// ( "GET /chunked HTTP/1.0\r\nConnection:Close\r\n\r\n", - (Status.Ok, Set(textPlain, connClose), "chunk"), + (Status.Ok, Set(textPlain), "chunk"), ), // ////////////////////////////// Requests with a body ////////////////////////////////////// ( @@ -90,7 +96,7 @@ object ServerTestRoutes extends Http4sDsl[IO] { // /////////////////////////////// ( "POST /post HTTP/1.0\r\nConnection:close\r\nContent-Length:3\r\n\r\nfoo", - (Status.Ok, Set(textPlain, length(4), connClose), "post"), + (Status.Ok, Set(textPlain, length(4)), "post"), ), // /////////////////////////////// ( @@ -119,7 +125,7 @@ object ServerTestRoutes extends Http4sDsl[IO] { // /////////////////////////////// Check corner cases ////////////////// ( "GET /twocodings HTTP/1.0\r\nConnection:Close\r\n\r\n", - (Status.Ok, Set(textPlain, length(3), connClose), "Foo"), + (Status.Ok, Set(textPlain, length(3)), "Foo"), ), // /////////////// Work with examples that don't have a body ////////////////////// ("GET /notmodified HTTP/1.1\r\n\r\n", (Status.NotModified, Set(), "")),