Merge branch 'invalid-header-field-error'

doc/programmers-guide.rst

@@ -152,11 +152,11 @@ header fields must not appear: "Connection", "Keep-Alive",
 Each header field name and value must obey the field-name and
 field-value production rules described in `RFC 7230, section
 3.2. <https://tools.ietf.org/html/rfc7230#section-3.2>`_.
-Additionally, all field name must be lower cased.  While the pseudo
-header fields must satisfy these rules, we just ignore illegal regular
-headers (this means that these header fields are not passed to
-application callback).  If application wants to treat these headers as
-error, use `nghttp2_on_invalid_header_callback <https://nghttp2.org/documentation/types.html#c.nghttp2_on_invalid_header_callback>`_.
+Additionally, all field name must be lower cased.  The invalid header
+fields are treated as stream error, and that stream is reset.  If
+application wants to treat these headers in their own way, use
+`nghttp2_on_invalid_header_callback
+<https://nghttp2.org/documentation/types.html#c.nghttp2_on_invalid_header_callback>`_.
 
 For "http" or "https" URIs, ":path" pseudo header fields must start
 with "/".  The only exception is OPTIONS request, in that case, "*" is

lib/includes/nghttp2/nghttp2.h

@@ -1741,11 +1741,12 @@ typedef int (*nghttp2_on_header_callback2)(nghttp2_session *session,
  * The parameter and behaviour are similar to
  * :type:`nghttp2_on_header_callback`.  The difference is that this
  * callback is only invoked when a invalid header name/value pair is
- * received which is silently ignored if this callback is not set.
- * Only invalid regular header field are passed to this callback.  In
- * other words, invalid pseudo header field is not passed to this
- * callback.  Also header fields which includes upper cased latter are
- * also treated as error without passing them to this callback.
+ * received which is treated as stream error if this callback is not
+ * set.  Only invalid regular header field are passed to this
+ * callback.  In other words, invalid pseudo header field is not
+ * passed to this callback.  Also header fields which includes upper
+ * cased latter are also treated as error without passing them to this
+ * callback.
  *
  * This callback is only considered if HTTP messaging validation is
  * turned on (which is on by default, see
@@ -1754,10 +1755,13 @@ typedef int (*nghttp2_on_header_callback2)(nghttp2_session *session,
  * With this callback, application inspects the incoming invalid
  * field, and it also can reset stream from this callback by returning
  * :enum:`NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE`.  By default, the
- * error code is :enum:`NGHTTP2_INTERNAL_ERROR`.  To change the error
+ * error code is :enum:`NGHTTP2_PROTOCOL_ERROR`.  To change the error
  * code, call `nghttp2_submit_rst_stream()` with the error code of
  * choice in addition to returning
  * :enum:`NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE`.
+ *
+ * If 0 is returned, the header field is ignored, and the stream is
+ * not reset.
  */
 typedef int (*nghttp2_on_invalid_header_callback)(
     nghttp2_session *session, const nghttp2_frame *frame, const uint8_t *name,

lib/nghttp2_session.c

@@ -3321,7 +3321,7 @@ static int session_call_on_invalid_header(nghttp2_session *session,
         session, frame, nv->name->base, nv->name->len, nv->value->base,
         nv->value->len, nv->flags, session->user_data);
   } else {
-    return 0;
+    return NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE;
   }
 
   if (rv == NGHTTP2_ERR_PAUSE || rv == NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE) {
@@ -3589,6 +3589,37 @@ static int inflate_header_block(nghttp2_session *session, nghttp2_frame *frame,
       if (subject_stream && session_enforce_http_messaging(session)) {
         rv = nghttp2_http_on_header(session, subject_stream, frame, &nv,
                                     trailer);
+
+        if (rv == NGHTTP2_ERR_IGN_HTTP_HEADER) {
+          /* Don't overwrite rv here */
+          int rv2;
+
+          rv2 = session_call_on_invalid_header(session, frame, &nv);
+          if (rv2 == NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE) {
+            rv = NGHTTP2_ERR_HTTP_HEADER;
+          } else {
+            if (rv2 != 0) {
+              return rv2;
+            }
+
+            /* header is ignored */
+            DEBUGF("recv: HTTP ignored: type=%u, id=%d, header %.*s: %.*s\n",
+                   frame->hd.type, frame->hd.stream_id, (int)nv.name->len,
+                   nv.name->base, (int)nv.value->len, nv.value->base);
+
+            rv2 = session_call_error_callback(
+                session,
+                "Ignoring received invalid HTTP header field: frame type: "
+                "%u, stream: %d, name: [%.*s], value: [%.*s]",
+                frame->hd.type, frame->hd.stream_id, (int)nv.name->len,
+                nv.name->base, (int)nv.value->len, nv.value->base);
+
+            if (nghttp2_is_fatal(rv2)) {
+              return rv2;
+            }
+          }
+        }
+
         if (rv == NGHTTP2_ERR_HTTP_HEADER) {
           DEBUGF("recv: HTTP error: type=%u, id=%d, header %.*s: %.*s\n",
                  frame->hd.type, frame->hd.stream_id, (int)nv.name->len,
@@ -3612,34 +3643,6 @@ static int inflate_header_block(nghttp2_session *session, nghttp2_frame *frame,
           }
           return NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE;
         }
-
-        if (rv == NGHTTP2_ERR_IGN_HTTP_HEADER) {
-          /* Don't overwrite rv here */
-          int rv2;
-
-          rv2 = session_call_on_invalid_header(session, frame, &nv);
-          /* This handles NGHTTP2_ERR_PAUSE and
-             NGHTTP2_ERR_TEMPORAL_CALLBACK_FAILURE as well */
-          if (rv2 != 0) {
-            return rv2;
-          }
-
-          /* header is ignored */
-          DEBUGF("recv: HTTP ignored: type=%u, id=%d, header %.*s: %.*s\n",
-                 frame->hd.type, frame->hd.stream_id, (int)nv.name->len,
-                 nv.name->base, (int)nv.value->len, nv.value->base);
-
-          rv2 = session_call_error_callback(
-              session,
-              "Ignoring received invalid HTTP header field: frame type: "
-              "%u, stream: %d, name: [%.*s], value: [%.*s]",
-              frame->hd.type, frame->hd.stream_id, (int)nv.name->len,
-              nv.name->base, (int)nv.value->len, nv.value->base);
-
-          if (nghttp2_is_fatal(rv2)) {
-            return rv2;
-          }
-        }
       }
       if (rv == 0) {
         rv = session_call_on_header(session, frame, &nv);

tests/nghttp2_session_test.c

@@ -11104,9 +11104,12 @@ void test_nghttp2_http_ignore_regular_header(void) {
   rv = nghttp2_session_mem_recv(session, bufs.head->buf.pos + proclen,
                                 nghttp2_buf_len(&bufs.head->buf) - proclen);
   CU_ASSERT_FATAL(rv > 0);
-  /* header field "foo" must be ignored because it has illegal value.
-     So we have "bar" header field for 5th header. */
-  CU_ASSERT(nghttp2_nv_equal(&bad_ansnv[4], &ud.nv));
+  /* Without on_invalid_frame_recv_callback, bad header causes stream
+     reset */
+  item = nghttp2_session_get_next_ob_item(session);
+
+  CU_ASSERT(NGHTTP2_RST_STREAM == item->frame.hd.type);
+
   proclen += (size_t)rv;
 
   CU_ASSERT(nghttp2_buf_len(&bufs.head->buf) == proclen);