Merge pull request #723 from nghttp2/strict-http-framing

Strict http framing

doc/programmers-guide.rst

@@ -173,6 +173,11 @@ parsed as 64 bit signed integer.  The sum of data length in the
 following DATA frames must match with the number in "Content-Length"
 header field if it is present (this does not include padding bytes).
 
+RFC 7230 says that server must not send "Content-Length" in any
+response with 1xx, and 204 status code.  It also says that
+"Content-Length" is not allowed in any response with 200 status code
+to a CONNECT request.  nghttp2 enforces them as well.
+
 Any deviation results in stream error of type PROTOCOL_ERROR.  If
 error is found in PUSH_PROMISE frame, stream error is raised against
 promised stream.

lib/nghttp2_http.c

@@ -250,6 +250,11 @@ static int http_response_on_header(nghttp2_stream *stream, nghttp2_hd_nv *nv,
     break;
   }
   case NGHTTP2_TOKEN_CONTENT_LENGTH: {
+    if (stream->status_code == 204 || stream->status_code / 100 == 1 ||
+        (stream->status_code == 200 &&
+         (stream->http_flags & NGHTTP2_HTTP_FLAG_METH_CONNECT))) {
+      return NGHTTP2_ERR_HTTP_HEADER;
+    }
     if (stream->content_length != -1) {
       return NGHTTP2_ERR_HTTP_HEADER;
     }

src/shrpx_http_downstream_connection.cc

@@ -701,6 +701,18 @@ int htp_hdrs_completecb(http_parser *htp) {
   downstream->set_downstream_addr_group(dconn->get_downstream_addr_group());
   downstream->set_addr(dconn->get_addr());
 
+  // Server MUST NOT send Transfer-Encoding with a status code 1xx or
+  // 204.  Also server MUST NOT send Transfer-Encoding with a status
+  // code 200 to a CONNECT request.  Same holds true with
+  // Content-Length.
+  if (resp.http_status == 204 || resp.http_status / 100 == 1 ||
+      (resp.http_status == 200 && req.method == HTTP_CONNECT)) {
+    if (resp.fs.header(http2::HD_CONTENT_LENGTH) ||
+        resp.fs.header(http2::HD_TRANSFER_ENCODING)) {
+      return -1;
+    }
+  }
+
   if (resp.fs.parse_content_length() != 0) {
     downstream->set_response_state(Downstream::MSG_BAD_HEADER);
     return -1;

src/shrpx_mruby_module_response.cc

@@ -201,6 +201,7 @@ namespace {
 mrb_value response_return(mrb_state *mrb, mrb_value self) {
   auto data = static_cast<MRubyAssocData *>(mrb->ud);
   auto downstream = data->downstream;
+  auto &req = downstream->request();
   auto &resp = downstream->response();
   int rv;
 
@@ -226,16 +227,28 @@ mrb_value response_return(mrb_state *mrb, mrb_value self) {
     bodylen = vallen;
   }
 
-  auto content_length = util::make_string_ref_uint(balloc, bodylen);
-
   auto cl = resp.fs.header(http2::HD_CONTENT_LENGTH);
-  if (cl) {
-    cl->value = content_length;
+
+  if (resp.http_status == 204 ||
+      (resp.http_status == 200 && req.method == HTTP_CONNECT)) {
+    if (cl) {
+      // Delete content-length here
+      cl->name = StringRef{};
+    }
+
+    resp.fs.content_length = -1;
   } else {
-    resp.fs.add_header_token(StringRef::from_lit("content-length"),
-                             content_length, false, http2::HD_CONTENT_LENGTH);
+    auto content_length = util::make_string_ref_uint(balloc, vallen);
+
+    if (cl) {
+      cl->value = content_length;
+    } else {
+      resp.fs.add_header_token(StringRef::from_lit("content-length"),
+                               content_length, false, http2::HD_CONTENT_LENGTH);
+    }
+
+    resp.fs.content_length = vallen;
   }
-  resp.fs.content_length = bodylen;
 
   auto date = resp.fs.header(http2::HD_DATE);
   if (!date) {

tests/nghttp2_session_test.c

@@ -10200,6 +10200,10 @@ void test_nghttp2_http_mandatory_headers(void) {
                                     MAKE_NV("content-length", "0")};
   const nghttp2_nv badhd_resnv[] = {MAKE_NV(":status", "200"),
                                     MAKE_NV("connection", "close")};
+  const nghttp2_nv cl1xx_resnv[] = {MAKE_NV(":status", "100"),
+                                    MAKE_NV("content-length", "0")};
+  const nghttp2_nv cl204_resnv[] = {MAKE_NV(":status", "204"),
+                                    MAKE_NV("content-length", "0")};
 
   /* test case for request */
   const nghttp2_nv nopath_reqnv[] = {MAKE_NV(":scheme", "https"),
@@ -10298,6 +10302,16 @@ void test_nghttp2_http_mandatory_headers(void) {
                                        NGHTTP2_STREAM_OPENING, badhd_resnv,
                                        ARRLEN(badhd_resnv));
 
+  /* response header has content-length with 100 status code */
+  check_nghttp2_http_recv_headers_fail(session, &deflater, 17,
+                                       NGHTTP2_STREAM_OPENING, cl1xx_resnv,
+                                       ARRLEN(cl1xx_resnv));
+
+  /* response header has content-length with 204 status code */
+  check_nghttp2_http_recv_headers_fail(session, &deflater, 19,
+                                       NGHTTP2_STREAM_OPENING, cl204_resnv,
+                                       ARRLEN(cl204_resnv));
+
   nghttp2_hd_deflate_free(&deflater);
 
   nghttp2_session_del(session);
@@ -11001,6 +11015,7 @@ void test_nghttp2_http_record_request_method(void) {
   nghttp2_bufs bufs;
   nghttp2_hd_deflater deflater;
   nghttp2_mem *mem;
+  nghttp2_outbound_item *item;
 
   mem = nghttp2_mem_default();
   frame_pack_bufs_init(&bufs);
@@ -11033,6 +11048,14 @@ void test_nghttp2_http_record_request_method(void) {
   CU_ASSERT((NGHTTP2_HTTP_FLAG_METH_CONNECT & stream->http_flags) > 0);
   CU_ASSERT(-1 == stream->content_length);
 
+  /* content-length is now allowed in 200 response to a CONNECT
+     request */
+  item = nghttp2_session_get_next_ob_item(session);
+
+  CU_ASSERT(NULL != item);
+  CU_ASSERT(NGHTTP2_RST_STREAM == item->frame.hd.type);
+  CU_ASSERT(NGHTTP2_PROTOCOL_ERROR == item->frame.rst_stream.error_code);
+
   nghttp2_hd_deflate_free(&deflater);
   nghttp2_session_del(session);
   nghttp2_bufs_free(&bufs);