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

CHANGES

@@ -62,6 +62,11 @@ Bug Fixes
      had a bug when toggled on. That toggle is no longer used and is superseded by
     `com.twitter.finagle.http.serverErrorsAsFailuresV2`. ``RB_ID=882151``
 
+  * finagle-http: To conform to the RFC, a message body is NO LONGER sent when 1xx, 204
+    and 304 responses are returned. Additionally, a Content-Length header field is NOT
+    sent for 1xx and 204 responses. Both rules are enforced even though users intentionally
+    add body data or the header field for these responses.
+
 Deprecations
 ~~~~~~~~~~~~
 

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

@@ -1,10 +1,13 @@
 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
 
+import org.jboss.netty.buffer.ChannelBuffers
+
 /**
  * Ensure that the `Response` is a legal response for the request that generated it
  *
@@ -36,13 +39,57 @@ 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 = {
+      if (rep.getContent() != ChannelBuffers.EMPTY_BUFFER) {
+        rep.clearContent()
+        logger.error(
+          "Response with a status code of %d must not have a body-message thus the content has been removed.",
+          rep.statusCode)
+      }
+
+      rep.status match {
+        case Continue | SwitchingProtocols | Processing | NoContent 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)
+        case _ =>
+      }
+  }
+
+  private def mustNotIncludeMessageBody(status: Status): Boolean = status match {
+    case Continue | SwitchingProtocols | Processing | NoContent | NotModified => 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

@@ -17,6 +17,7 @@ import com.twitter.io.{Buf, Reader, Writer}
 import com.twitter.util._
 import java.io.{PrintWriter, StringWriter}
 import java.net.InetSocketAddress
+import org.jboss.netty.buffer.ChannelBuffers
 import org.scalatest.{BeforeAndAfter, FunSuite}
 import org.scalatest.concurrent.{Eventually, IntegrationPatience}
 import scala.language.reflectiveCalls
@@ -36,6 +37,7 @@ abstract class AbstractEndToEndTest extends FunSuite
   object Streaming extends Feature
   object StreamFixed extends Feature
   object TooLongStream extends Feature
+  object NoBodyMessage extends Feature
 
   var saveBase: Dtab = Dtab.empty
   val statsRecv: InMemoryStatsReceiver = new InMemoryStatsReceiver()
@@ -916,4 +918,138 @@ abstract class AbstractEndToEndTest extends FunSuite
     await(server.close())
     await(client.close())
   }
+
+  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.httpMessage.isChunked)
+      assert(res.httpMessage.getContent == ChannelBuffers.EMPTY_BUFFER)
+      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.httpMessage.isChunked)
+      assert(res.httpMessage.getContent == ChannelBuffers.EMPTY_BUFFER)
+      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 and 204} must not have a message body nor Content-Length header field" +
+    " when non-empty body with explicit Content-Length 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
+          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.httpMessage.isChunked)
+      assert(res.httpMessage.getContent == ChannelBuffers.EMPTY_BUFFER)
+      assert(res.contentLength.isEmpty)
+      await(client.close())
+      await(server.close())
+    }
+
+    List(Status.Continue, /*Status.SwitchingProtocols,*/ Status.Processing, Status.NoContent).foreach {
+      check(_)
+    }
+  }
+
+
+  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.httpMessage.isChunked)
+      assert(res.httpMessage.getContent == ChannelBuffers.EMPTY_BUFFER)
+      assert(res.contentLength.contains(body.length.toLong))
+      await(client.close())
+      await(server.close())
+  }
 }

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}
+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}
@@ -78,6 +79,69 @@ class ResponseConformanceFilterTest extends FunSuite {
     intercept[ReaderDiscarded] { Await.result(res.writer.write(Buf.Empty), 5.seconds) }
   }
 
+  test("response with status code {1xx, 204 and 304} must not have a message body nor Content-Length header field") {
+    def validate(status: Status) = {
+      val res = Response(Version.Http11, status)
+      val response = fetchResponse(res)
+
+      assert(response.status == status)
+      assert(response.getContent.readableBytes() == 0)
+      assert(!response.isChunked)
+      assert(response.headers().get(Fields.ContentLength) == null)
+    }
+
+    List(Continue, SwitchingProtocols, Processing, NoContent, NotModified).foreach(validate(_))
+  }
+
+  test("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 validate(status: Status) = {
+      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.getContent().readableBytes() == 0)
+      assert(!response.isChunked)
+      assert(response.headers().get(Fields.ContentLength) == null)
+    }
+
+    List(Continue, SwitchingProtocols, Processing, NoContent, NotModified).foreach(validate(_))
+  }
+
+  test("response with status code {1xx and 204} must not have a message body nor Content-Length header field when non-empty body with explicit Content-Length is returned") {
+    def validate(status: Status) = {
+      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.getContent().readableBytes() == 0)
+      assert(!response.isChunked)
+      assert(response.headers().get(Fields.ContentLength) == null)
+    }
+
+    List(Continue, SwitchingProtocols, Processing, NoContent).foreach(validate(_))
+  }
+
+  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.getContent().readableBytes() == 0)
+    assert(!response.isChunked)
+    assert(response.headers().get(Fields.ContentLength) == body.length.toString)
+  }
+
   def fetchResponse(res: Response): Response = {
     val request = Request(uri = "/")
     runFilter(request, res)

finagle-http2/src/test/scala/com/twitter/finagle/http2/ClientFailUpgradeTest.scala

@@ -25,7 +25,8 @@ class ClientFailUpgradeTest extends AbstractHttp1EndToEndTest {
     TooLongStream,
     Streaming,
     CloseStream,
-    StreamFixed
+    StreamFixed,
+    NoBodyMessage
   )
 
   def featureImplemented(feature: Feature): Boolean = !unsupported.contains(feature)