diff --git a/doc/programmers-guide.rst b/doc/programmers-guide.rst index 93af2a0a..1e7d1e68 100644 --- a/doc/programmers-guide.rst +++ b/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. `_. -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 `_. +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 +`_. For "http" or "https" URIs, ":path" pseudo header fields must start with "/". The only exception is OPTIONS request, in that case, "*" is diff --git a/lib/includes/nghttp2/nghttp2.h b/lib/includes/nghttp2/nghttp2.h index 15901004..ef1d610e 100644 --- a/lib/includes/nghttp2/nghttp2.h +++ b/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, diff --git a/lib/nghttp2_session.c b/lib/nghttp2_session.c index d4408ab3..4fbbe72d 100644 --- a/lib/nghttp2_session.c +++ b/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); diff --git a/tests/nghttp2_session_test.c b/tests/nghttp2_session_test.c index 28ca0ac6..c1bc84ad 100644 --- a/tests/nghttp2_session_test.c +++ b/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);