[PATCH 2 of 2] HTTP/2: finalize request as bad if header validation fails

[Roman Arutyunyan]

Hi,

On Wed, Feb 22, 2023 at 03:55:28PM +0300, Maxim Dounin wrote:
> # HG changeset patch
> # User Maxim Dounin <mdounin at mdounin.ru>
> # Date 1677070454 -10800
> #      Wed Feb 22 15:54:14 2023 +0300
> # Node ID ec3819e66f40924efad183c291c850d9ccef16e7
> # Parent  61bd779a868c4021c232dddfe7abda7e8ad32575
> HTTP/2: finalize request as bad if header validation fails.
> 
> Similarly to 7192:d5a535774861, this avoids spurious zero statuses
> in access.log, and in line with other header-related errors.

This is what RFC 9113, 8.2.1. Field Validity, says about header field
validation:

  A request or response that contains a field that violates
  any of these conditions MUST be treated as malformed (Section 8.1.1).

Also (the previous RFC 7540 did not have this statement):

  When a request message violates one of these requirements, an implementation
  SHOULD generate a 400 (Bad Request) status code (see Section 15.5.1 of [HTTP])

Considering this, responding with HTTP 400 is certainly an improvement compared
to the current code.  Also, it allows to set a custom error page handler.

However, not resetting the stream as before is a less obvious change.  If we
consider nginx an intermediary, the RFC 9113 prescribes us to send stream reset:

  Intermediaries that process HTTP requests or responses (i.e., any
  intermediary not acting as a tunnel) MUST NOT forward a malformed
  request or response. Malformed requests or responses that are detected
  MUST be treated as a stream error (Section 5.4.2) of type PROTOCOL_ERROR.

This restriction probably makes sense for the intermediaries unable to handle
the error in a more flexible way.

I like the change, but I'd like to hear what you think about the reset issue.

> diff --git a/src/http/v2/ngx_http_v2.c b/src/http/v2/ngx_http_v2.c
> --- a/src/http/v2/ngx_http_v2.c
> +++ b/src/http/v2/ngx_http_v2.c
> @@ -1794,14 +1794,7 @@ ngx_http_v2_state_process_header(ngx_htt
>  
>      /* TODO Optimization: validate headers while parsing. */
>      if (ngx_http_v2_validate_header(r, header) != NGX_OK) {
> -        if (ngx_http_v2_terminate_stream(h2c, h2c->state.stream,
> -                                         NGX_HTTP_V2_PROTOCOL_ERROR)
> -            == NGX_ERROR)
> -        {
> -            return ngx_http_v2_connection_error(h2c,
> -                                                NGX_HTTP_V2_INTERNAL_ERROR);
> -        }
> -
> +        ngx_http_finalize_request(r, NGX_HTTP_BAD_REQUEST);
>          goto error;
>      }
>  
> _______________________________________________
> nginx-devel mailing list
> nginx-devel at nginx.org
> https://mailman.nginx.org/mailman/listinfo/nginx-devel

--
Roman Arutyunyan