finagle-http: Remove body (and Content-Length) from 1xx, 204 and 304 …

…responses

Problem

We currently have two problems. Firstly, as described in RFC2616 and
RFC7230, 1xx, 204 and 304 responses must not contain a message-body. This
requirement was not enforced in finagle, i.e., a message-body can be
added. Moreover, a Content-Length header field can be added to 1xx and
204 responses, which is not allowed as mentioned in RFC7230#section-3.3.2.

Solution

Firstly, we have to clear a message-body, which we call content in finagle,
if the response status code is either 1xx, 204 or 304 to satisfy RFC7230.
Then, we need to remove a Content-Length header field in 1xx and 204 responses
to follow RFC7230#section-3.3.2. With regard to a 304 response, we don't
intentionally remove it even though a Content-Length header field is set.

Result

We follow the specification of RFC with regard to a message-header and
message-body except that we have not added some header fields required in a
response with status code 304 to satisfy RFC7232#section-4.1; Any of the
following header fields must be generated: Cache-Control, Content-Location,
Date, ETag, Expires, and Vary.

Signed-off-by: Bryce Anderson <banderson@twitter.com>

RB_ID=917827

CHANGES

@@ -74,6 +74,12 @@ Runtime Behavior Changes
   * finagle-netty4: `poolReceiveBuffers` toggle is removed (suppressed by `UsePooling`).
     ``RB_ID=917912``
 
+  * finagle-http: To conform to RFC 2616, a message body is NO LONGER sent when 1xx, 204
+    and 304 responses are returned. To conform with RFC 7230, a Content-Length header field
+    is NOT sent for 1xx and 204 responses. Both rules are enforced even if users intentionally
+    add body data or the header field for these responses. If violation of these rules is
+    detected then an error message is logged. ``RB_ID=917827``
+
 Breaking API Changes
 ~~~~~~~~~~~~~~~~~~~~
 

finagle-http/src/main/scala/com/twitter/finagle/http/codec/ResponseConformanceFilter.scala

@@ -1,7 +1,8 @@
 package com.twitter.finagle.http.codec
 
 import com.twitter.finagle.{Service, SimpleFilter}
-import com.twitter.finagle.http._
+import com.twitter.finagle.http.{Fields, Method, Request, Response, Status}
+import com.twitter.finagle.http.Status._
 import com.twitter.logging.Logger
 import com.twitter.util.Future
 
@@ -36,13 +37,64 @@ private[codec] object ResponseConformanceFilter extends SimpleFilter[Request, Re
   private[this] def validate(req: Request, rep: Response): Unit = {
     if (req.method == Method.Head) {
       handleHeadResponse(req, rep)
+    } else if (mustNotIncludeMessageBody(rep.status)) {
+      handleNoMessageResponse(rep)
     } else if (rep.isChunked) {
       handleChunkedResponse(rep)
     } else {
       handleFullyBufferedResponse(rep)
     }
   }
 
+  /**
+   * 1. To conform to the RFC, a message body is removed if a status code is
+   *    either 1xx, 204 or 304.
+   *
+   * RFC7230 section-3.3: (https://tools.ietf.org/html/rfc7230#section-3.3)
+   * "All 1xx (Informational), 204 (No Content), and 304 (Not Modified) responses
+   * do not include a message body."
+   *
+   * 2. Additionally, a Content-Length header field is dropped for 1xx and 204 responses
+   * as described in RFC7230 section-3.3.2. It, however, is allowed to send a Content-Length
+   * header field in a 304 response. To follow the section, we don't remove the header field
+   * from a 304 response but its value is not checked nor corrected.
+   *
+   * RFC7230 section-3.3.2: (https://tools.ietf.org/html/rfc7230#section-3.3.2)
+   * "A server MUST NOT send a Content-Length header field in any response with a status code
+   * of 1xx (Informational) or 204 (No Content)."
+   *
+   * "A server MAY send a Content-Length header field in a 304 (Not Modified) response to
+   * a conditional GET request (Section 4.1 of [RFC7232]); a server MUST NOT send Content-Length
+   * in such a response unless its field-value equals the decimal number of octets that would have
+   * been sent in the payload body of a 200 (OK) response to the same request."
+   */
+  private[this] def handleNoMessageResponse(rep: Response): Unit = {
+    val contentLength = rep.length
+    if (contentLength > 0) {
+      rep.clearContent()
+      logger.error(
+        "Response with a status code of %d must not have a body-message but it has " +
+          "a %d-byte payload, thus the content has been removed.",
+        rep.statusCode, contentLength)
+    }
+
+    if (rep.status != NotModified && mustNotIncludeMessageBody(rep.status)) {
+      if (rep.contentLength.isDefined) {
+        rep.headerMap.remove(Fields.ContentLength)
+        logger.error(
+          "Response with a status code of %d must not have a Content-Length header field " +
+            "thus the field has been removed.",
+          rep.statusCode)
+      }
+    }
+  }
+
+  private def mustNotIncludeMessageBody(status: Status): Boolean = status match {
+    case NoContent | NotModified => true
+    case _ if 100 <= status.code && status.code < 200 => true
+    case _ => false
+  }
+
   private[this] def handleFullyBufferedResponse(rep: Response): Unit = {
     // Set the Content-Length header to the length of the body
     // if it is not already defined. Examples of reasons that a service might

finagle-http/src/test/scala/com/twitter/finagle/http/AbstractEndToEndTest.scala

@@ -37,6 +37,7 @@ abstract class AbstractEndToEndTest extends FunSuite
   object ClientAbort extends Feature
   object HeaderFields extends Feature
   object ReaderClose extends Feature
+  object NoBodyMessage extends Feature
 
   var saveBase: Dtab = Dtab.empty
   val statsRecv: InMemoryStatsReceiver = new InMemoryStatsReceiver()
@@ -1432,4 +1433,134 @@ abstract class AbstractEndToEndTest extends FunSuite
     testMethodBuilderRetries(stats, server, builder)
   }
 
+  testIfImplemented(NoBodyMessage)(
+    "response with status code {1xx, 204 and 304} must not have a message body nor Content-Length header field"
+  ) {
+    def check(resStatus: Status): Unit = {
+      val svc = new Service[Request, Response] {
+        def apply(request: Request) = {
+          val response = Response(Version.Http11, resStatus)
+
+          Future.value(response)
+        }
+      }
+      val server = serverImpl()
+        .serve("localhost:*", svc)
+
+      val addr = server.boundAddress.asInstanceOf[InetSocketAddress]
+      val client = clientImpl()
+        .newService(s"${addr.getHostName}:${addr.getPort}", "client")
+
+      val res = await(client(Request(Method.Get, "/")))
+      assert(res.status == resStatus)
+      assert(!res.isChunked)
+      assert(res.content.isEmpty)
+      assert(res.contentLength.isEmpty)
+      await(client.close())
+      await(server.close())
+    }
+
+    List(Status.Continue, /*Status.SwitchingProtocols,*/ Status.Processing, Status.NoContent, Status.NotModified).foreach {
+      check(_)
+    }
+  }
+
+  testIfImplemented(NoBodyMessage)(
+    "response with status code {1xx, 204 and 304} must not have a message body nor Content-Length header field" +
+    "when non-empty body is returned"
+  ) {
+    def check(resStatus: Status): Unit = {
+      val svc = new Service[Request, Response] {
+        def apply(request: Request) = {
+          val body = Buf.Utf8("some data")
+          val response = Response(Version.Http11, resStatus)
+          response.content = body
+
+          Future.value(response)
+        }
+      }
+      val server = serverImpl()
+        .serve("localhost:*", svc)
+
+      val addr = server.boundAddress.asInstanceOf[InetSocketAddress]
+      val client = clientImpl()
+        .newService(s"${addr.getHostName}:${addr.getPort}", "client")
+
+      val res = await(client(Request(Method.Get, "/")))
+      assert(res.status == resStatus)
+      assert(!res.isChunked)
+      assert(res.content.isEmpty)
+      assert(res.contentLength.isEmpty)
+      await(client.close())
+      await(server.close())
+    }
+
+    List(Status.Continue, /*Status.SwitchingProtocols,*/ Status.Processing, Status.NoContent, Status.NotModified).foreach {
+      check(_)
+    }
+  }
+
+  // We exclude SwitchingProtocols(101) since it should only be sent in response to a upgrade request
+  List(Status.Continue, Status.Processing, Status.NoContent)
+    .foreach { resStatus =>
+      testIfImplemented(NoBodyMessage)(
+        s"response with status code ${resStatus.code} must not have a message body nor " +
+          "Content-Length header field when non-empty body with explicit Content-Length is returned"
+      ) {
+        val svc = new Service[Request, Response] {
+          def apply(request: Request) = {
+            val body = Buf.Utf8("some data")
+            val response = Response(Version.Http11, resStatus)
+            response.content = body
+            response.headerMap.set(Fields.ContentLength, body.length.toString)
+
+            Future.value(response)
+          }
+        }
+        val server = serverImpl()
+          .serve("localhost:*", svc)
+
+        val addr = server.boundAddress.asInstanceOf[InetSocketAddress]
+        val client = clientImpl()
+          .newService(s"${addr.getHostName}:${addr.getPort}", "client")
+
+        val res = await(client(Request(Method.Get, "/")))
+        assert(res.status == resStatus)
+        assert(!res.isChunked)
+        assert(res.length == 0)
+        assert(res.contentLength.isEmpty)
+        await(client.close())
+        await(server.close())
+      }
+    }
+
+  testIfImplemented(NoBodyMessage)(
+    "response with status code 304 must not have a message body *BUT* Content-Length " +
+      "header field when non-empty body with explicit Content-Length is returned"
+  ) {
+    val body = Buf.Utf8("some data")
+    val svc = new Service[Request, Response] {
+      def apply(request: Request) = {
+        val response = Response(Version.Http11, Status.NotModified)
+        response.content = body
+        response.headerMap.set(Fields.ContentLength, body.length.toString)
+
+        Future.value(response)
+      }
+    }
+    val server = serverImpl()
+      .serve("localhost:*", svc)
+
+    val addr = server.boundAddress.asInstanceOf[InetSocketAddress]
+    val client = clientImpl()
+      .newService(s"${addr.getHostName}:${addr.getPort}", "client")
+
+    val res = await(client(Request(Method.Get, "/")))
+    assert(res.status == Status.NotModified)
+    assert(!res.isChunked)
+    assert(res.length == 0)
+    assert(res.contentLength.contains(body.length.toLong))
+    await(client.close())
+    await(server.close())
+  }
 }

finagle-http/src/test/scala/com/twitter/finagle/http/ClientFailHttp2UpgradeTest.scala

@@ -13,7 +13,7 @@ class ClientFailHttp2UpgradeTest extends AbstractHttp1EndToEndTest {
 
   def serverImpl(): finagle.Http.Server = finagle.Http.server
 
-  def featureImplemented(feature: Feature): Boolean = true
+  def featureImplemented(feature: Feature): Boolean = feature != NoBodyMessage
 
   test("Upgrade counters are not incremented") {
     val client = nonStreamingConnect(Service.mk { req: Request =>

finagle-http/src/test/scala/com/twitter/finagle/http/Netty4EndToEndTest.scala

@@ -11,5 +11,6 @@ class Netty4EndToEndTest extends AbstractEndToEndTest {
 
   def serverImpl(): FinagleHttp.Server = FinagleHttp.server.configured(FinagleHttp.Netty4Impl)
 
-  def featureImplemented(feature: Feature): Boolean = true
+  def featureImplemented(feature: Feature): Boolean =
+    feature != NoBodyMessage
 }

finagle-http/src/test/scala/com/twitter/finagle/http/ServerFailHttp2UpgradeTest.scala

@@ -13,7 +13,7 @@ class ServerFailHttp2UpgradeTest extends AbstractHttp1EndToEndTest {
 
   def serverImpl(): finagle.Http.Server = finagle.Http.server.configuredParams(finagle.Http.Http2)
 
-  def featureImplemented(feature: Feature): Boolean = true
+  def featureImplemented(feature: Feature): Boolean = feature != NoBodyMessage
 
   test("Upgrade counters are not incremented") {
     val client = nonStreamingConnect(Service.mk { req: Request =>

finagle-http/src/test/scala/com/twitter/finagle/http/codec/ResponseConformanceFilterTest.scala

@@ -2,7 +2,8 @@ package com.twitter.finagle.http.codec
 
 import com.twitter.conversions.time._
 import com.twitter.finagle.Service
-import com.twitter.finagle.http.{Fields, Method, Request, Response, Status}
+import com.twitter.finagle.http.{Fields, Method, Request, Response, Status, Version}
+import com.twitter.finagle.http.Status._
 import com.twitter.io.Buf
 import com.twitter.io.Reader.ReaderDiscarded
 import com.twitter.util.{Await, Future}
@@ -77,6 +78,70 @@ class ResponseConformanceFilterTest extends FunSuite {
     intercept[ReaderDiscarded] { Await.result(res.writer.write(Buf.Empty), 5.seconds) }
   }
 
+  List(Continue, SwitchingProtocols, Processing, NoContent, NotModified).foreach { status =>
+    test(s"response with status code ${status.code} must not have a message body nor " +
+      "Content-Length header field"
+    ) {
+      val res = Response(Version.Http11, status)
+      val response = fetchResponse(res)
+
+      assert(response.status == status)
+      assert(response.length == 0)
+      assert(!response.isChunked)
+      assert(response.headerMap.get(Fields.ContentLength).isEmpty)
+    }
+  }
+
+  List(Continue, SwitchingProtocols, Processing, NoContent, NotModified).foreach { status =>
+    test(s"response with status code ${status.code} must not have a message body nor " +
+      "Content-Length header field when non-empty body is returned"
+    ) {
+      val body = Buf.Utf8("some data")
+      val res = Response(Version.Http11, status)
+      res.content = body
+
+      val response = fetchResponse(res)
+
+      assert(response.status == status)
+      assert(response.length == 0)
+      assert(!response.isChunked)
+      assert(response.headerMap.get(Fields.ContentLength).isEmpty)
+    }
+  }
+
+  List(Continue, SwitchingProtocols, Processing, NoContent).foreach { status =>
+    test(s"response with status code ${status.code} must not have a message body nor " +
+      "Content-Length header field when non-empty body with explicit Content-Length is returned"
+    ) {
+      val body = Buf.Utf8("some data")
+      val res = Response(Version.Http11, status)
+      res.content = body
+      res.contentLength = body.length.toLong
+
+      val response = fetchResponse(res)
+
+      assert(response.status == status)
+      assert(response.length == 0)
+      assert(!response.isChunked)
+      assert(response.headerMap.get(Fields.ContentLength).isEmpty)
+    }
+  }
+
+  test("response with status code 304 must not have a message body *BUT* Content-Length " +
+    "header field when non-empty body with explicit Content-Length is returned") {
+    val body = Buf.Utf8("some data")
+    val res = Response(Version.Http11, Status.NotModified)
+    res.content = body
+    res.contentLength = body.length.toLong
+
+    val response = fetchResponse(res)
+
+    assert(response.status == Status.NotModified)
+    assert(response.length == 0)
+    assert(!response.isChunked)
+    assert(response.headerMap.get(Fields.ContentLength).contains(body.length.toString))
+  }
+
   def fetchResponse(res: Response): Response = {
     val request = Request(uri = "/")
     runFilter(request, res)