Treat incoming invalid regular header field as stream error

Previously, the incoming invalid regular header field was ignored by
default.  With this commit, they are now treated as stream error, and
the stream is reset by default.  The error code used is now
PROTOCOL_ERROR, instead of INTERNAL_ERROR.

doc/programmers-guide.rst

@@ -149,11 +149,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
+Additionally, all field name must be lower cased.  The invalid header
-header fields must satisfy these rules, we just ignore illegal regular
+fields are treated as stream error, and that stream is reset.  If
-headers (this means that these header fields are not passed to
+application wants to treat these headers in their own way, use
-application callback).  If application wants to treat these headers as
+`nghttp2_on_invalid_header_callback
-error, use `nghttp2_on_invalid_header_callback <https://nghttp2.org/documentation/types.html#c.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.
+ * received which is treated as stream error if this callback is not
- * Only invalid regular header field are passed to this callback.  In
+ * set.  Only invalid regular header field are passed to this
- * other words, invalid pseudo header field is not passed to this
+ * callback.  In other words, invalid pseudo header field is not
- * callback.  Also header fields which includes upper cased latter are
+ * passed to this callback.  Also header fields which includes upper
- * also treated as error without passing them to this callback.
+ * 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.
+  /* Without on_invalid_frame_recv_callback, bad header causes stream
-     So we have "bar" header field for 5th header. */
+     reset */
-  CU_ASSERT(nghttp2_nv_equal(&bad_ansnv[4], &ud.nv));
+  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);